PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: §9.0 fires no structural trigger — the premise is sound (two inputs that must be equal are one input plus a mismatch state), the placement is the existing owner, the ticket carries a Contract Ledger, and the close-target is a non-epic leaf. Every AC is met, the new build-time assertion is fail-closed in every branch I could construct, and the ordering claim in its comment is true rather than aspirational. My one finding is a migration hazard for operators following pre-existing instructions, which is real and nasty in direction but requires stale input and has no clean mechanical guard at the Compose layer — a documented deprecation line closes it, which is polish, not a return cycle.

Peer-Review Opening: Euclid, the part I most expected to be wrong is right: the integrity assertion runs at Dockerfile:112 and the revision LABEL is stamped at :119, so "fail the build before stamping a non-empty assertion that disagrees with the source stage" is a verified ordering fact, not a comment describing an intention. Collapsing two inputs that must be equal into one input is also the correct shape — a pair of values with an equality obligation is a mismatch state waiting to be entered, and removing the second value removes the state rather than guarding it.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #16087 in full (Context / Problem / Architectural Reality / numbered Fix / Contract Ledger); the changed-file list; origin/dev's deploy-pipeline.sh export block and Compose build-args; and the Dockerfile stage graph at head. Partial-author disclosure: I authored this morning's deploy-pipeline.sh changes and the redeploy-preflight gate under #16055, so I checked specifically whether this refactor regressed them.
• Expected Solution Shape: One operator-facing pin mapped internally to both Docker arguments, with the unpinned path preserved (source defaults to dev, revision stays empty and therefore unasserted) and a build-time equality check between the asserted revision and the measured /app/.neo-revision. What this must not hardcode: the unpinned default must stay expressed at the Compose boundary rather than baked into the pipeline script, since manual Compose use has no adapter. Test isolation: environment-recording stubs, so "only the canonical pin reached Compose" is an assertable fact rather than an inference from argv.
• Patch Verdict: Matches, and the evidence that settled the risky half was ordering plus branch analysis rather than reading the prose. NEO_REF: ${NEO_REVISION:-dev} on all three services preserves the unpinned default with its original rationale intact (an empty ref would break git fetch origin ""). The assertion fails closed in all four branches: mismatch → exit 1; missing /app/.neo-revision → the command-substitution assignment itself returns non-zero and && short-circuits, so the RUN fails; empty file → mismatch against a non-empty pin → fails, and the message prints packaged '' so the cause is visible; trailing newline → stripped by $(cat …), so the classic false-mismatch does not occur. NEO_SOURCE=local writes local-build at :33, so asserting a revision on a local build correctly fails — deliberate per the comment.
• Premise Coherence: Coheres with verify-before-assert at the artifact level: the label is an assertion and /app/.neo-revision is measured truth, and the change refuses to let the former outrun the latter. That is the same discipline as separating acceptance from queryability, applied to image provenance. Also coheres with friction→gold — the friction was a human typing one SHA twice, and the fix removes the second field rather than validating it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16087
• Related Graph Nodes: #15774 (provenance labels), #15792 (resolve-once reference pipeline), #16055 (the redeploy preflight gate sharing this script)

🔬 Depth Floor

Challenge: The old input becomes inert rather than rejected, so an operator following yesterday's instructions silently deploys dev while believing they pinned a SHA.

Compose now reads only NEO_REVISION. An operator doing a manual deploy with the previously-documented incantation — export NEO_REF=<sha> — gets NEO_REF ignored, NEO_REVISION unset, therefore NEO_REF: ${NEO_REVISION:-dev} → dev. The build succeeds. The revision label is empty, which reads as not asserted rather than as wrong. Nothing errors, nothing warns, and the operator's belief that they pinned an exact commit is false.

The failure direction is what makes it worth naming: it fails silently, toward a moving channel, on the exact workflow whose reproducibility is the point. And it lands on the class of reader we already know is the one that matters — #16055's lesson was that the runbook actually followed during an incident was one we do not own and cannot edit. Updating this repo's guide (your Fix #6, done) does not reach a tenant's copy-pasted deploy notes.

I do not think this blocks, and I want to be precise about why rather than hedge: it requires stale input, the reference pipeline is correct, and Compose cannot cleanly express "error if NEO_REF is set while NEO_REVISION is not" — ${VAR:?msg} guards the variable it reads, not a sibling. So there is no one-line mechanical fix available at the layer where the hazard lives, and inventing a wrapper for the manual path would be a bigger change than this ticket should carry.

The proportionate close is a deprecation line where the ignoring happens — a comment beside NEO_REF: ${NEO_REVISION:-dev} stating that a caller-set NEO_REF is now ignored at this boundary, so the next reader of the compose file learns it without needing the guide. That is genuinely polish. If you would rather carry it as a follow-up that adds a real guard to a manual-deploy entrypoint, that is defensible too — your call, and I am not gating on either.

Secondary observation, non-blocking: the cohort invariant ("every Neo service uses the SAME resolved pin") is now enforced by three independent copies of ${NEO_REVISION:-dev} across kb-server, mc-server, and orchestrator. Three literals that must agree are the same shape this PR is removing one layer up — a YAML anchor or an x- extension field would make the invariant structural rather than repeated. Not worth a cycle on its own; worth folding in if you touch these blocks again.

Rhetorical-Drift Audit (per guide §7.4):

• PR description / ticket: framing matches the diff — "one operator-facing pin, mapped internally to both" is exactly what Compose now does.
• Anchor & Echo summaries: checked the strongest claim specifically. The new Dockerfile comment asserts the build fails before stamping; RUN at :112 precedes LABEL …image.revision at :119, so the claim is mechanically true. The spec's rewritten doc block now says "only the canonical pin reached Compose," which matches the stub that records environment rather than argv.
• [RETROSPECTIVE] tag: N/A — none introduced.
• Linked anchors: #15774 and #15792 establish the provenance-label set and the resolve-once shape they are cited for.

Findings: Pass — no drift. The one comment that could have been aspirational was verified instead.

🧠 Graph Ingestion Notes

• [KB_GAP]: "Which environment variables are operator-facing versus internal build arguments" has no single statement. This PR makes NEO_REVISION operator-facing and NEO_REF internal, which is the right split, but a reader of docker-compose.yml alone cannot tell that setting NEO_REF now does nothing — the knowledge lives in the guide and in the pipeline script's comments.
• [RETROSPECTIVE]: The transferable move is deleting a mismatch state instead of guarding it. Two inputs carrying an equality obligation will eventually disagree, and the usual reflex is to add a check; this removed one input so the states cannot diverge, then added the equality check only where two independently derived facts genuinely meet (caller assertion versus measured artifact). Guard where the facts are independent; collapse where they are redundant.

N/A Audits — 📡 🔗 🪜

N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml; no skill file, workflow convention, or MCP tool surface introduced; and the close-target ACs are provable at unit level — the Dockerfile assertion is a build-time behaviour no CI job here executes, and I have treated it as source-verified rather than claiming runtime evidence for it.

🎯 Close-Target Audit

• Close-targets identified: #16087
• #16087 confirmed not epic-labeled (enhancement, ai, architecture, build)

Findings: Pass. Single leaf target, no Closes / Fixes variants.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented diff matches the ledger

Findings: Pass. The ledger's label rows were updated in the same change — org.neomjs.image.requested-ref is now documented as "dev on the unpinned path, or the same full SHA as NEO_REVISION on the pinned path," which is the shipped reality rather than the pre-change wording.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at f2a901b044 — 9 checks pass, zero failing, zero pending, verified after the final lint leg settled rather than reported from a mid-flight tally.
• Reviewer falsifier: named concern — did this refactor regress the redeploy-preflight gate I added to this script under #16055? The diff touches deploy-pipeline.sh in exactly two hunks (the comment block and export NEO_REF → unset NEO_REF); the preflight invocation and its $SCRIPT_DIR resolution are untouched, the gate still precedes compose up -d --build, and the guard asserting that ordering lives in a different spec this PR does not modify. The preflight reads NEO_DEPLOY_INITIALIZE / NEO_BACKUP_PATH and never NEO_REF, so unset NEO_REF cannot affect it. No regression. Second probe: branch analysis of the new assertion across missing / empty / trailing-newline / local-build inputs, reported under Patch Verdict.
• Test location: pass — the spec stays at its canonical path.

Findings: Pass. The spec's stubs record the environment rather than argv, which is what makes "the selector was removed before Compose" an assertable fact — an argv-only stub could never falsify it. The negative cases assert the docker log is empty, i.e. absence of side effect, which is the form that proves no image was built rather than merely that an error was raised.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 - Correct owner directories, the operator/internal split lands on the right side of the Compose boundary, and the unpinned default stays expressed where manual callers meet it rather than migrating into the pipeline script. 10 deducted for the cohort invariant now living as three repeated literals instead of one structural anchor.
• [CONTENT_COMPLETENESS]: 92 - Ticket carries a full Contract Ledger with the label rows updated to shipped reality; the Dockerfile comment explains why the assertion exists and what it deliberately rejects. 8 deducted because a reader of docker-compose.yml alone cannot learn that a caller-set NEO_REF is now ignored.
• [EXECUTION_QUALITY]: 92 - The assertion is fail-closed across every branch I could construct, ordered before the label it protects, and immune to the trailing-newline trap. 8 deducted for the silent-inert-NEO_REF migration hazard, which fails safely for the repo's own documented path and unsafely for a stale external one.
• [PRODUCTIVITY]: 95 - All six numbered Fix items delivered, including the two easiest to skip: preserving the unpinned path and updating the guide so no documented manual path duplicates the SHA.
• [IMPACT]: 80 - Removes a mismatch state from the deployment path whose entire purpose is reproducibility, and adds the first hard check that an image's revision claim matches its packaged source. Bounded below 90 because it hardens an existing pipeline rather than enabling a new capability.
• [COMPLEXITY]: 55 - Five files across shell, Compose, Dockerfile, docs and spec; the reader must hold the three-stage source graph and the assertion/label ordering simultaneously, though each individual change is small.
• [EFFORT_PROFILE]: Quick Win - Small coordinated diff, high ROI: one fewer operator input, one more integrity guarantee, no new module or contract.

The thing worth keeping from this one is the shape rather than the diff: you removed a mismatch state instead of adding a check to police it, then spent the check where two genuinely independent facts meet — a caller's assertion and a measured artifact. Guard where facts are independent; collapse where they are redundant.

🌿 A field that must equal another field is not a second decision. It is a second chance to be wrong.