feat: enforce pinned image digest for madengine run - #180
Merged
Conversation
Proposes capturing the pushed image digest at build time (always on) and gating enforcement of digest-pinned pulls behind an opt-in --require-pinned-image flag, addressing a run that pulled a different image than the one the build pushed due to a mutable-tag race.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ment helpers Add madengine/core/image_digest.py with four pure helpers: - parse_push_digest: extract sha256 digest from `docker push` output - parse_repo_digest: extract digest from a repo@sha256:... reference - build_pinned_reference: build repo@sha256:... , stripping any tag/digest - resolve_pinned_image: pass through, pin, or raise ConfigurationError Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
push_image() now captures the push output and records the resulting
sha256 digest in self.pushed_digests, falling back to
`docker image inspect --format '{{index .RepoDigests 0}}'` when the
registry does not print a digest line. Best-effort: a missing digest is
noted at dim level and never fails the build.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both push call sites (single-arch and per-GPU-arch) copy the digest recorded by push_image into build_info["image_digest"]. build_info is serialized wholesale into build_manifest.json, so the key is purely additive for existing manifest consumers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The CLI flag and the require_pinned_image additional-context key are equivalent entry points. The key is also persisted into manifest["context"] so the nested `madengine run` that SLURM job scripts execute on each compute node inherits the setting. Two pre-existing tests built args as a bare MagicMock and asserted exact-equality on additional_context; auto-vivified attributes are truthy, so the new flag leaked in. Pin the attribute in those mocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The resolve_pinned_image call sits outside the pull try/except so a missing digest aborts rather than falling back to the local image tag. The container runs the same pinned reference that was pulled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pod spec image field resolves through resolve_pinned_image, so a moved tag surfaces as an ImagePullBackOff rather than a silent wrong-image run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
slurm_multi runs the model's own script on the head node with no nested `madengine run` on the compute nodes, so enforcement happens here. Pinning DOCKER_IMAGE_NAME covers both the parallel `srun docker pull` (which interpolates it) and the `docker run` inside the model script. prepare()'s launcher peek wrapped the whole slurm_multi dispatch in a bare `except Exception: pass`, which would have swallowed the enforcement error and generated an unpinned script instead. Re-raise ConfigurationError so deliberate aborts propagate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Characterization tests: _docker_image_ref_for_log_naming already strips @sha256:..., so pinned references produce the same log/tar filenames as tags. Locks that in against future refactors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The build-on-compute-node path writes built_images entries carrying both a truthy local_image and a registry reference in docker_image. That branch runs before the registry branch in run_models_from_manifest, so --require-pinned-image was silently a no-op for those manifests -- the exact bypass the flag exists to prevent, and contrary to the documented fail-fast behaviour. Resolve the pin in the local_image branch too. resolve_pinned_image now passes through references that are already digest-pinned, so an explicitly pinned MAD_CONTAINER_IMAGE is accepted rather than rejected for lacking a manifest digest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…refs
Container names were built straight from the run image reference, mapping
only "/" and ":" to "_". Under --require-pinned-image the run image becomes
repo@sha256:..., so the "@" survived into --name and the daemon rejected it:
docker: Error response from daemon: Invalid container name
(container_***_mad-private@sha256_af99a16c...), only
[a-zA-Z0-9][a-zA-Z0-9_.-] are allowed
The pull succeeded because a digest reference is a valid image ref; only the
name derived from it was invalid.
Add container_name_from_image_ref(), which strips the digest, applies the same
"/" and ":" mapping as before, and sanitizes any remaining out-of-charset
byte so the invariant holds instead of failing at `docker run`.
_docker_image_ref_for_log_naming() is deliberately not reused: it collapses
CI-style refs to the bare tag, which would rename containers on every existing
non-pinned path. Keeping the tag also keeps different tags of one repo
distinct. Verified byte-identical output against the previous expression for
60 non-digest refs (bare tags, registry hosts, localhost:5000 ports, CI tags,
dotted tags), including the name the e2e suite asserts.
Also drops the dead re.sub(".*:", "", ...) at the call site: ":" had already
been replaced with "_" on the preceding call, so it never matched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Under --require-pinned-image a failed registry pull fell back to run_image = image_name. Local tags are mutable too, so that fallback broke the pinned-image guarantee in the failure mode it matters most in: a digest/tag mismatch or an auth error would silently run whatever the local tag happened to point at. The pull failure is now fatal for that model. The surrounding handler already records it as a failed run and continues with the rest of the manifest, so one unpinnable model does not abort the whole run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
push_image() called Console.sh() with the default 60s timeout, so pushing a multi-GB image failed an otherwise successful build with "Console script timeout". build_image() already passes timeout=None for docker build; do the same for the push. Console.sh's timeout was annotated int while five call sites already pass None (which subprocess accepts as "wait indefinitely"), so widen the annotation to Optional[int] to match the behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
coketaste
requested review from
Cemberk,
Rohan138,
gargrahul and
leconcio
as code owners
September 1, 2026 20:13
There was a problem hiding this comment.
Pull request overview
This PR adds optional enforcement for digest-pinned registry images during madengine run, by recording the pushed image digest at build time (image_digest in the build manifest) and introducing a --require-pinned-image flag / require_pinned_image context key to force pulls by repo@sha256:... across local Docker, Kubernetes, and SLURM (including slurm_multi).
Changes:
- Record pushed image digests during
docker pushand persist them intobuild_manifest.jsonasimage_digest. - Add
--require-pinned-imageand propagate it through orchestrator/manifest context for nested runs (SLURM compute-node flows). - Enforce pinned references in local pulls, K8s pod specs, and SLURM
slurm_multi, plus add unit/integration coverage and documentation.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_slurm_multi.py | Adds slurm_multi enforcement tests (pinned DOCKER_IMAGE_NAME + failure on missing digest). |
| tests/unit/test_orchestration.py | Tests CLI flag/context propagation and persistence into manifest context. |
| tests/unit/test_k8s.py | Tests K8s pod image resolution under pinned-image enforcement. |
| tests/unit/test_image_digest.py | New unit tests for digest parsing, pinned reference building, and enforcement policy. |
| tests/unit/test_execution.py | Adds coverage for log naming behavior with pinned refs; adds container name tests. |
| tests/unit/test_docker_builder.py | Tests digest capture during push and propagation into build_info. |
| tests/unit/test_container_runner.py | Tests local Docker enforcement (pinned pulls, no fallback, manifest context inheritance). |
| tests/integration/test_orchestrator_workflows.py | Pins MagicMock flags to prevent truthy attribute leakage into context. |
| src/madengine/orchestration/run_orchestrator.py | Wires CLI flag into additional_context and persists key into manifest context. |
| src/madengine/execution/docker_builder.py | Captures push output, records digest best-effort, writes image_digest into manifest entries. |
| src/madengine/execution/container_runner.py | Enforces pinned image policy for local and registry paths; updates container naming to handle @. |
| src/madengine/execution/container_runner_helpers.py | Adds helper to generate Docker-legal container names from image refs. |
| src/madengine/deployment/slurm.py | Enforces pinned image in slurm_multi and ensures ConfigurationError isn’t swallowed. |
| src/madengine/deployment/k8s_template_context.py | Resolves/pins the pod image field when enforcement is enabled. |
| src/madengine/core/image_digest.py | New helper module for digest parsing, pinned ref construction, and enforcement policy. |
| src/madengine/core/console.py | Allows Console.sh(..., timeout=None) to support long docker push. |
| src/madengine/cli/commands/run.py | Adds --require-pinned-image flag and passes it into args namespace. |
| docs/superpowers/specs/2026-08-27-pinned-image-digest-design.md | Design spec for digest capture + opt-in enforcement. |
| docs/superpowers/plans/2026-08-27-pinned-image-digest.md | Implementation plan for the feature. |
| docs/configuration.md | Documents pinned image digests and the enforcement flag/context key. |
| docs/cli-reference.md | Adds CLI reference entry for --require-pinned-image. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1364
to
+1367
| # Generate container name. docker_image may be digest-pinned | ||
| # (repo@sha256:...) under require_pinned_image, and "@" is not a legal | ||
| # container-name character, so this must not use the raw reference. | ||
| base_container_name = container_name_from_image_ref(docker_image) |
Comment on lines
+87
to
+91
| # The CLI flag and the require_pinned_image context key are equivalent; | ||
| # the key lets CI pipelines that drive madengine through | ||
| # --additional-context opt in the same way as for k8s/slurm/tools. | ||
| if getattr(args, "require_pinned_image", False): | ||
| self.additional_context["require_pinned_image"] = True |
Comment on lines
+2886
to
+2890
| raise RuntimeError( | ||
| f"require_pinned_image: failed to pull " | ||
| f"{pull_target} for model " | ||
| f"{model_info.get('name', image_name)}: {pull_error}" | ||
| ) from pull_error |
coketaste
added a commit
that referenced
this pull request
Sep 1, 2026
Two PRs merged to develop after the 2.2.0 section was written had no changelog entry: per-model docker_build_arg support (#175) and pinned image digest enforcement (#180). Adds Added entries for both features and Fixed entries for the bugs bundled in them: the 60s Console.sh timeout on docker push, the invalid container name derived from repo@sha256 references, the local-tag fallback that defeated pinning on pull failure, and the get_build_arg KeyError on a context missing docker_build_arg. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sha256digest asimage_digeston eachbuilt_imagesmanifest entry (always on, no behavior change by default).--require-pinned-imageflag (and equivalentrequire_pinned_imageadditional-context key) makesmadengine runpull registry images byrepo@sha256:...instead of by tag, across local Docker, Kubernetes, and SLURM (includingslurm_multi). If the manifest has no recorded digest, the run fails immediately with no tag fallback — a moved/mutable tag can no longer silently resolve to the wrong image.contextblock so nestedmadengine runinvocations on compute nodes inherit it; build-on-compute-node manifests are also covered.Test plan
pytest tests/unit -q— 638 passedpytest tests/integration -q— 151 passed, 1 skipped (needs non-AMD GPU)mypy src/madengine/core/image_digest.py— cleanjob.sh.j2to confirmcontextsurvives re-dumpdocs/superpowers/🤖 Generated with Claude Code