Skip to content

Require a service template's base to be one the preflight can actually probe, including in the final stage #382

Description

@swinney

Objective

Make templates_missing_base_reference report a service template as covered only when the
preflight can actually probe the base that template will build on — rejecting both an
a2rchi-*-base name outside the preflight's placeable set and a multistage template whose
final stage leaves that base behind.

Context you need

Where this came from. Codex review of PR #380 (fix/issue-361-declare-service-templates),
two P1 findings on src/cli/managers/base_image_preflight.py:119, raised against head
6a50effc and reproduced there. Comment IDs 3879446497 and 3879446522. Both were verified
and deferred by the 4AM review pass because that pass had reached its round bound, and because
neither is a live defect — see "Present-day risk" below.

The abstraction that over-promises. PR #380 introduced a declared service-template set in
src/cli/managers/base_image_preflight.py:

  • service_templates(template_dir=None) (:86) — every Dockerfile* not in
    NON_SERVICE_TEMPLATES.
  • templates_missing_base_reference(template_dir=None) (:109) — set members whose text has
    no _FROM_BASE_RE match. Its docstring says the deploy preflight "cannot cover these
    templates", so a non-empty return is a refusal.
  • _refuse_uncoverable_templates(template_dir=None) (:521) — raises on a non-empty list,
    called from required_base_images (:158) and from enforce_base_images (:574), the
    function src/cli/cli_main.py:282 calls.

The promise is "the preflight can place this template's base". The check is only
_FROM_BASE_RE.search(...)^FROM\s+(?P<ref>\S*a2rchi-\w+-base\S*) at :43. Two ways a
match fails to deliver the promise:

  1. \w+ matches any base name. enforce_base_images probes only what
    required_base_image_names returns (:184) — PYTHON_BASE (a2rchi-python-base) and
    optionally PYTORCH_BASE (a2rchi-pytorch-base). A template on a third
    a2rchi-<something>-base matches the regex, so the template counts as covered while its
    base is never probed.
  2. search returns the first match anywhere in the file. In a multistage template, an
    early FROM ... a2rchi-python-base AS builder satisfies the check even when the final
    stage is FROM docker.io/library/debian:12. The image the deployment actually runs is
    never probed.

In both cases base_reference (:122) still resolves the required names from the other
healthy templates, so enforce_base_images returns a complete-looking answer and
archi create --force proceeds through remove_existing_deployment()
(src/cli/cli_main.py:294) before the build fails. That is the ordering contract from
#287 that this whole module exists to hold.

Measured at 6a50effc, each fixture being one digest-pinned Dockerfile-chat plus the
offending template:

[unknown a2rchi base (a2rchi-node-base)]
  templates_missing_base_reference: []
  enforce_base_images: NO REFUSAL -> ['ghcr.io/fasrc/a2rchi-python-base@sha256:aaaa...']

[multistage, final stage third-party]
  templates_missing_base_reference: []
  enforce_base_images: NO REFUSAL -> ['ghcr.io/fasrc/a2rchi-python-base@sha256:aaaa...']

No existing guard catches either. test_two_image_rule_still_matches_every_template
(tests/unit/test_base_image_preflight.py:261) is the closest, and it does not: it flags only
base == "pytorch" on a non--gpu non-grader template and base == "python" on a -gpu
template. Read the two if statements at :277-281 — a base of node matches neither
branch, so the test passes in silence. Re-derive this before assuming otherwise.

Present-day risk: none. Both are gaps in the guarantee, not current faults. Verify:

$ grep -ho 'a2rchi-[a-z]*-base' src/cli/templates/dockerfiles/Dockerfile* | sort -u
a2rchi-python-base
a2rchi-pytorch-base

$ for f in src/cli/templates/dockerfiles/Dockerfile*; do n=$(grep -c '^FROM' "$f"); \
    [ "$n" -gt 1 ] && echo "$f: $n"; done
(no output — no template is multistage)

So the trigger for either is a future change: someone introducing a third base image, or
converting a service template to multistage. Both are exactly the "silently outside every
guard" failure #361 existed to end, which is why this is worth closing rather than dropping.

Do not widen _FROM_BASE_RE itself. base_reference (:122) shares it and must keep
matching any a2rchi reference, because its job is to find the pinned reference for a name it
was already given. Put the new strictness in templates_missing_base_reference.

Constraints

  • Branch base depends on whether PR feat(#361): declare which Dockerfile templates are service templates #380 has merged. Check first:
    gh pr view 380 --repo fasrc/archi --json state,mergedAt.
  • PR targets fasrc/archi:devgh pr create --repo fasrc/archi --base dev. NOT
    upstream/dev, which is archi-physics/archi and 100+ commits diverged.
  • TDD is mandatory. Write each failing test first and watch it fail. A test that passes
    before your change proves nothing.
  • bash scripts/gate.sh must exit 0 before every commit. Never --no-verify.
  • Patch coverage vs the branch base must clear 80%. New lines in
    src/cli/managers/base_image_preflight.py are coverage-measured (--cov=src).
  • No Co-Authored-By trailers.
  • Put closes #<this issue> in the PR body, not the title — a closing keyword in the
    title does not link the issue.
  • Do not edit the Dockerfile templates, .github/workflows/**, or
    scripts/dev/update_service_base_images.py. This is one function's strictness.
  • Do not merge. A human merges.
  • One PR. Both findings live in the same function and share a test fixture shape.

Plan

  1. RED, finding 1. In tests/unit/test_base_image_preflight.py, beside
    test_templates_missing_base_reference_reports_replaced_line (:1280), add a test whose
    fixture holds a digest-pinned Dockerfile-chat (reuse _PINNED_FROM, :1273) plus
    Dockerfile-node on FROM ghcr.io/fasrc/a2rchi-node-base@sha256:<64 hex>. Assert
    templates_missing_base_reference reports Dockerfile-node. Watch it fail — it currently
    returns [].

  2. RED, finding 1 at the deploy entry point. Add the enforce_base_images counterpart,
    modelled on test_enforce_base_images_refuses_an_uncoverable_service_template (near the end
    of the file). Assert BaseImagePreflightError naming the template. This is the assertion
    that matters: the unit-level check is not what protects the operator.

  3. RED, finding 3. Same two levels, with a multistage fixture:

    FROM ghcr.io/fasrc/a2rchi-python-base@sha256:<64 hex> AS builder
    RUN pip wheel .
    FROM docker.io/library/debian:12
    COPY --from=builder /wheels /wheels
    

    Assert the template is reported and that enforce_base_images refuses.

  4. GREEN. Rewrite the comprehension in templates_missing_base_reference (:115-119) to
    decide per template rather than on a single search:

    • Collect every _FROM_BASE_RE match with finditer.
    • Determine the final stage's base. A template is covered only when the base the final
      stage resolves to is an a2rchi base whose name is in the placeable set.
    • Introduce the placeable set as a module constant beside PYTHON_BASE / PYTORCH_BASE
      (:26-27) — one name for the idea, so required_base_image_names and this check cannot
      disagree about which bases exist.
    • A template with no match at all must still be reported, as today.

    Multistage resolution needs care and a comment saying what it does and does not handle: a
    final stage may be FROM <earlier-stage-alias>, in which case follow the alias back to its
    base. State the bound you implement rather than implying totality — that habit is what let
    the original defect ship.

  5. Confirm nothing else regressed. templates_missing_base_reference gets stricter, so any
    fixture whose template names an a2rchi base that is not python or pytorch, or that is
    multistage, now reports. Check every caller and fixture:
    grep -rn "templates_missing_base_reference\|_refuse_uncoverable_templates" src/ tests/.
    If a fixture becomes invalid, that is a real finding about the fixture — fix the fixture, do
    not weaken the check. Say in the PR body what you found, even if nothing changed.

  6. test_templates_missing_base_reference_on_real_directory_is_empty (:1303) must still
    pass — the 15 real service templates all name python or pytorch and none is multistage
    (measured above). If it fails, stop: a template changed under you and that is the more
    interesting news.

Commands

Re-derive the state before you start:

git fetch origin
gh pr view 380 --repo fasrc/archi --json state,mergedAt
grep -n "_FROM_BASE_RE\|^PYTHON_BASE\|^PYTORCH_BASE" src/cli/managers/base_image_preflight.py
sed -n '109,125p' src/cli/managers/base_image_preflight.py
grep -ho 'a2rchi-[a-z]*-base' src/cli/templates/dockerfiles/Dockerfile* | sort -u
for f in src/cli/templates/dockerfiles/Dockerfile*; do n=$(grep -c '^FROM' "$f"); \
  [ "$n" -gt 1 ] && echo "$f: $n FROM lines"; done

Reproduce both gaps (writes only under a temp directory; never edit
src/cli/templates/dockerfiles/). Pipe via stdin so the import resolves to this checkout
rather than an installed copy:

python - <<'EOF'
import pathlib, sys, tempfile
sys.path.insert(0, ".")
from src.cli.managers import base_image_preflight as pf
print("module:", pf.__file__)

PINNED = "FROM ghcr.io/fasrc/a2rchi-python-base@sha256:" + "a"*64 + "\n"

class Probe:
    container_tool = "docker"
    def runtime_available(self): return True
    def image_present(self, ref): return True
    def pull(self, ref): return None
    def python_version(self, ref): return "Python 3.11.9"
    def reachable(self, ref): return None

class Plan:
    gpu_ids = None
    def get_service(self, name): raise ValueError(name)

def case(label, files):
    d = pathlib.Path(tempfile.mkdtemp())
    for n, t in files.items():
        (d / n).write_text(t)
    print(f"\n[{label}]")
    print("  missing:", [p.name for p in pf.templates_missing_base_reference(d)])
    try:
        out = pf.enforce_base_images(Plan(), probe=Probe(), template_dir=d)
        print("  enforce: NO REFUSAL ->", [o.reference for o in out])
    except pf.BaseImagePreflightError as e:
        print("  enforce: REFUSED:", str(e).splitlines()[0])

case("unknown a2rchi base", {
    "Dockerfile-chat": PINNED,
    "Dockerfile-node": "FROM ghcr.io/fasrc/a2rchi-node-base@sha256:" + "b"*64 + "\n",
})
case("multistage, final stage third-party", {
    "Dockerfile-chat": PINNED,
    "Dockerfile-multi": (
        "FROM ghcr.io/fasrc/a2rchi-python-base@sha256:" + "c"*64 + " AS builder\n"
        "RUN pip wheel .\n"
        "FROM docker.io/library/debian:12\n"
        "COPY --from=builder /wheels /wheels\n"
    ),
})
EOF

Before the fix both cases print missing: [] and enforce: NO REFUSAL. After it, both must
print the offending template and enforce: REFUSED.

Test and gate:

python -m pytest tests/unit/test_base_image_preflight.py -v
python -m pytest tests/unit/test_base_image_preflight.py \
  tests/unit/test_cli_create_dev_smoke.py \
  tests/unit/test_python_version_declaration.py -q
bash scripts/gate.sh

Acceptance criteria

  • The reproduction script above prints the offending template and enforce: REFUSED for
    both cases.
  • A test asserts templates_missing_base_reference reports a template naming an
    a2rchi-*-base outside the placeable set, and another asserts enforce_base_images
    refuses and names it.
  • A test asserts the same two things for a multistage template whose final stage is not an
    a2rchi base.
  • Each new test failed before the implementation change; state the observed failure in
    the commit message.
  • The placeable set is a single named constant, referenced by both
    required_base_image_names and the coverage check —
    grep -n "a2rchi-python-base\"" src/cli/managers/base_image_preflight.py shows no new
    literal duplicating PYTHON_BASE.
  • _FROM_BASE_RE is unchanged: git diff origin/dev -- src/cli/managers/base_image_preflight.py | grep '^[-+].*_FROM_BASE_RE = '
    prints nothing.
  • test_templates_missing_base_reference_on_real_directory_is_empty still passes.
  • bash scripts/gate.sh exits 0, no --no-verify anywhere.
  • Patch coverage vs the branch base is at least 80%.
  • A PR is open against fasrc/archi:dev with closes #<this issue> in the body. Not
    merged.

Start here

Run the re-derivation commands and the reproduction script. Confirm you see missing: [] and
enforce: NO REFUSAL on both cases, and read the two if statements in
test_two_image_rule_still_matches_every_template to confirm for yourself that neither catches
an unknown base name. Do not write implementation until you have watched the first new test
fail.

Metadata

Metadata

Assignees

No one assigned

    Labels

    P3Priority: when possibleai-wipClaimed by an automated run; in flightauto-okOpt-in: the nightly run may pick this issue up and open a PRbugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions