PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise and the implementation are both right, and I want to be clear that this is not a shape objection. The single blocker is a one-line correctness defect inside the file this PR edits: the guide's first runnable example still omits the input this PR makes mandatory, so the canonical wiring doc now ships a copy-pasteable command that aborts. That is a debt-creating quick win rather than follow-up-ticket fuel — it is in-scope, one line, and its audience is exactly the external teams this reference script exists for.

Peer-Review Opening: Strong PR. The premise is the good kind of inversion — a "compatible default" that was quietly choosing an incomplete security posture is not a default, it is a trap, and removing it is worth the breakage. The git-stub-exit-97 mutation witness is the detail I want to call out: it turns "stopped" into "stopped before revision resolution", which a non-zero exit alone could never prove. One doc line to fix and this is ready.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16671 (title + labels), the changed-file list, PipelineWiring.md at pr16674 head, both invocation examples in that guide, WindowsSupport.md's reference to the script, and a repo-wide sweep for callers and for NEO_DEPLOY_COMPOSE_FILE writers.
• Expected Solution Shape: Remove the base-file fallback so the caller must name its composition; abort before any external transaction work; keep explicit single-file callers byte-identical; and update every surface that documents an invocation. It must NOT hardcode a plane's file set, and the abort must be provable as preceding Docker and revision resolution, not merely as a non-zero exit.
• Patch Verdict: Matches, and improves on the expected shape in one place. I expected the abort proof to stop at "Docker never invoked" (what the prior test asserted); the failing git stub extends the boundary proof upstream to revision resolution, which is the earlier and more meaningful line. The one place it falls short of the shape is the doc-surface sweep — see Required Actions.
• Premise Coherence: Coheres — verify-before-assert. The justification is not "explicit is better"; it is a measured 80-line rendering delta on the canonical plane, naming NEO_AUTH_MODE absent, NEO_MODEL_PROVIDER empty rather than openAiCompatible, and NEO_MCP_HEALTHCHECK_TOKEN_FILE gone, under a different project name. The fail-closed choice is derived from that measurement rather than from a general preference.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16671
• Related Graph Nodes: #16458, #16167, #16206, deploy-pipeline.sh, PipelineWiring.md, fail-closed-vs-compatible-default
• Origin Session ID: 4141258c-36d3-4788-b0c2-ab3ebe0867be

🔬 Depth Floor

Challenge: The deliberate inversion of a documented prior decision deserves a durable marker, and right now it only lives in a comment that a future reader may "correct". The previous code went to real lengths — ${VAR+set} plus a paragraph — to keep UNSET and EXPLICITLY-EMPTY apart, because collapsing them meant an empty value silently deploying the base contract. This PR collapses them with ${VAR:-}, and that is right, because the reasoning was conditional on a default existing and no default exists now. My concern is that the old rationale was equally well-argued in-place, so the next reader has two confident comments and no way to tell which is current. The new test covers all three inputs, so a restoration would fail — but consider one clause in the comment saying the distinction was deliberate and is now obsolete because the fallback is gone. That converts the test from an accident-catcher into the stated guard.

Second, smaller: this is a breaking change for any external CI job relying on the default. In-repo I verified nothing breaks — deploymentMigrationCore supplies the variable explicitly, and both specs are updated. Externally it breaks loudly at deploy time, which for ai/examples/ is the correct trade, and the new abort message is good migration guidance in its own right. Worth a line in the PR body naming it as an intentional break so the release notes inherit it.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: precise codebase terminology, no metaphor or overshooting anchor
• [RETROSPECTIVE] tag: N/A — none claimed
• Linked anchors: cited tickets actually establish the claimed pattern

Findings: Pass. The prose is unusually disciplined about separating what was measured (the 80-line delta, the named absent variables) from what is inferred (the posture conclusion).

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The reusable idea here is the mutation witness for a boundary claim: to prove an abort happens before stage N, install a stub at stage N that fails loudly if reached, then assert both the exit code and the empty call log. Asserting "Docker was not called" proves a weaker property than "nothing downstream ran", and the two are easy to conflate. Also worth remembering: a default that selects an incomplete security posture is not a compatible default — the compatibility is real and the safety is the thing being traded away silently.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no public/consumed contract surface is introduced or modified beyond the env-var contract already documented in PipelineWiring.md, no openapi.yaml is touched, and no skill file, convention, or architectural primitive is involved.

🎯 Close-Target Audit

• Close-targets identified: #16671
• For each #N: confirmed not epic-labeled — #16671 carries bug, ai, build

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence — L2 achieved, L2 required, "No residuals" declared
• If residuals exist: N/A — none declared
• Two-ceiling distinction: L2 is the honest ceiling here, not an unprobed stop. The subject is a shell script's control flow, and recording stubs exercise it directly; an L3 receipt would require a real plane and would not test anything the stubs cannot.
• Evidence-class collapse check: the PR does not promote stub-based evidence to deployment-receipt framing
• Deployment causality: N/A — no external runtime receipt is used as a merge gate

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: author receipt present and current-head-appropriate; the replaced test is removed rather than left asserting the deleted default
• Reviewer falsifier: command + named concern + result — see below
• Test location: pass; both specs stay beside their existing siblings

Reviewer falsifier. My named concern was that the doc's invocation examples would go stale, since the PR changes a contract the guide teaches. I ran it against the PR head rather than my checkout:

git fetch origin pull/16674/head:pr16674
git grep -n "deploy-pipeline.sh" pr16674 -- learn/agentos/cloud-deployment/PipelineWiring.md
git show pr16674:learn/agentos/cloud-deployment/PipelineWiring.md | grep -n -B 4 "deploy-pipeline.sh$"

Result: the concern reproduced. Two runnable examples exist. Line 163-167 correctly sets NEO_DEPLOY_COMPOSE_FILE. Line 40-42 does not:

# tag-triggered job: deploy the tag that fired it, not the default channel
NEO_REF="$CI_COMMIT_TAG" ai/examples/cloud-deployment/deploy-pipeline.sh

At this head that command aborts. Before this PR it worked. I also checked the only other surface citing the script, learn/agentos/tooling/WindowsSupport.md:46 — a path mention inside a shell-file inventory, no invocation, so no change needed there.

Findings: Author evidence gap — the code contract and its tests are complete; the doc surface teaching that contract is not.

📋 Required Actions

To proceed with merging, please address the following:

• learn/agentos/cloud-deployment/PipelineWiring.md:40-42 — the tag-triggered example omits the now-mandatory NEO_DEPLOY_COMPOSE_FILE and aborts as written. Add it, or point the snippet at the complete example below it. The surrounding prose at line 38 is about NEO_REF specifically, so the minimal honest fix is adding the composition line to the snippet rather than rewriting the paragraph.

Optional, not blocking, from the Depth Floor above: one clause marking the UNSET-vs-EMPTY collapse as a deliberate supersession of the prior distinction, and a PR-body line naming the external breaking change as intentional.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - Fail-closed at the input boundary, in the layer that owns the input. No plane specifics leak into the reference script; the caller names its own composition, which is the correct ownership line for an ai/examples/ reference.
• [CONTENT_COMPLETENESS]: 78 - Code, comments, and tests are complete and mutually consistent. The doc surface teaching the changed contract is one example short, which is the entire gap.
• [EXECUTION_QUALITY]: 92 - The git-stub boundary proof and the three-input table (unset / empty / delimiter-only) are better than the bar. Removing the obsolete test rather than weakening it is the right instinct.
• [PRODUCTIVITY]: 90 - +64/-62 across four files for a security-posture correction, with the net line count essentially flat.
• [IMPACT]: 88 - Removes a silent path to deploying an auth-incomplete composition. Bounded by being a reference script rather than the boot path.
• [COMPLEXITY]: 30 - Small, well-contained shell and spec changes.
• [EFFORT_PROFILE]: Quick Win - Small diff closing a real safety gap, with the test design carrying more value than the line count suggests.

Nice work — fix the one doc example and I will flip this to approved. The mutation-witness pattern is the part I am stealing.

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z