Skip to content

Harden the base-image pin guards against partial swaps, and reconcile the spec with them #375

Description

@swinney

Objective

Close three gaps in the base-image pin guards that let a partial or unauthorized change to the service Dockerfiles pass CI, and remove the contradiction between the service-base-images spec and the guards that implement it.

Context you need

Deferred from the review of PR #370 (fix/issue-335-pin-service-dockerfiles-to-digests, head d149623d). Codex raised all three as P2 (comment ids 3870762672, 3870762680, 3870762687). Each was verified against the code. They were deferred because #370's unattended review run had reached its round bound, not because they are wrong.

All three live in tests/unit/test_base_image_preflight.py and the change's OpenSpec artifacts.

1. The exact-digest assertion covers 2 templates, not 15. test_base_references_are_pinned_to_the_expected_digests calls preflight.base_reference(preflight.PYTHON_BASE) and the pytorch equivalent. base_reference() returns only the first sorted match per image (src/cli/managers/base_image_preflight.py), so replacing the digest in, say, Dockerfile-redmine with another well-formed 64-hex digest — annotation intact — leaves the suite green. A partial or unauthorized digest swap passes CI.

2. The count is of FROM matches, not of files. test_all_templates_share_one_pin_state asserts len(references) == TEMPLATE_COUNT, where references is one entry per matching FROM line. Removing the base FROM from Dockerfile-redmine and adding a second annotated digest FROM to Dockerfile-redmine-gpu still yields 15, and Dockerfile-redmine can no longer build. The requirement is "15 files are pinned", so assert the expected set of filenames and exactly one managed base reference per file.

3. The spec contradicts the guards. openspec/changes/fix-issue-335-pin-service-dockerfiles-to-digests/specs/service-base-images/spec.md:5 states the digest requirement unconditionally and carries a "No template names a base image by tag" scenario. #370 taught _pin_state() to accept the CalVer release tag and made test_base_references_are_pinned_to_the_expected_digests skip on a release-retargeted tree. So the capability spec now describes a rule the implementation deliberately does not enforce, and which the canonical post-release tree violates.

Related: #373 proposes re-pinning to digests during the release. If #373 lands first, item 3 resolves by deletion — the release state stops existing and _RELEASE_REF can be removed entirely. Check #373's status before writing spec text you will then have to unwrite.

Constraints

  • Branch from origin/dev. Open the PR against fasrc/archi:dev — never upstream/dev.
  • No Co-Authored-By or session trailers on commits.
  • TDD: failing test first, then the minimum code, then the gate.
  • bash scripts/gate.sh must exit 0 before every commit. Never --no-verify.
  • Do not weaken test_no_template_carries_a_mutable_base_reference; a dev-<sha> tag must stay rejected.
  • Both guards must keep passing on a release-retargeted tree unless Re-pin the service Dockerfiles to base-image digests during the release, not to the CalVer tag #373 has landed. That is the P1 Pin the 15 service Dockerfile templates to ghcr digests #370 fixed; re-breaking it makes the released branch red.
  • This repo uses OpenSpec. Run ls openspec/specs/ first: if service-base-images is still unarchived under openspec/changes/, a delta must use ADDED, not MODIFIED.

Plan

  1. Give each mutation its own failing test before fixing it. tests/unit/test_base_image_preflight.py already has the pattern — _materialize_release_retargeted_tree builds a throwaway copy of the templates, and test_the_release_guard_detects_a_rewrite_to_the_wrong_base mutates that copy and asserts the check catches it. Reuse both.
  2. Item 1: assert every collected reference's digest against the expected digest for the base image that reference names, over all 15, rather than calling base_reference() twice. Add a discrimination test that swaps one template's digest for another valid-looking one and asserts the guard fails.
  3. Item 2: assert the exact set of pinned filenames and exactly one managed base reference per file. Add a discrimination test that moves a FROM between two templates and asserts the guard fails.
  4. Item 3: decide with reference to Re-pin the service Dockerfiles to base-image digests during the release, not to the CalVer tag #373. Either scope the spec requirement to the development state and add a scenario for the release-retargeted tree, or wait for Re-pin the service Dockerfiles to base-image digests during the release, not to the CalVer tag #373 and then delete the release state from both spec and guards. Do not leave the spec and the guards disagreeing.

Commands

git fetch origin && git checkout -b fix/issue-<this>-harden-pin-guards origin/dev
ls openspec/specs/
python -m pytest tests/unit/test_base_image_preflight.py -q
bash scripts/gate.sh                    # run bare; it refuses to run piped or redirected

Prove the current gaps before fixing them, then restore:

# item 1 — swap one digest, suite stays green
sed -i 's/@sha256:c068f17b8cba96682e7007c9dd5511f43fea86c796f3cbeee44e2766c5a9b8e8/@sha256:'"$(printf 'a%.0s' {1..64})"'/' src/cli/templates/dockerfiles/Dockerfile-redmine
python -m pytest tests/unit/test_base_image_preflight.py -q
git checkout -- src/cli/templates/dockerfiles/

Acceptance criteria

  • bash scripts/gate.sh exits 0.
  • The digest swap shown above makes the suite fail.
  • Moving a base FROM from one template to another makes the suite fail.
  • The suite still passes on a release-retargeted tree, unless Re-pin the service Dockerfiles to base-image digests during the release, not to the CalVer tag #373 landed and the release state was removed — in which case say so in the PR body.
  • A dev-<sha> retarget still fails test_no_template_carries_a_mutable_base_reference.
  • The service-base-images spec and the guards agree; the PR body states which of the two options in step 4 was taken and why.
  • openspec validate --strict passes (each requirement's SHALL on its first physical line).
  • The PR targets fasrc/archi:dev and its body carries a Closes #<this> line (the body, not the title).

Start here

Check whether #373 is open, merged, or declined. That decides step 4 and can turn part of this issue into a deletion instead of an addition. Then run the digest-swap reproduction above and confirm the suite really does stay green — that is the evidence the fix is real.

Metadata

Metadata

Assignees

No one assigned

    Labels

    P2Priority: this cyclebugSomething isn't workingneeds-humanBlocked: needs a human design decision

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions