PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The unit-only transport refusal is the right containment and the production-derived failure fixture is stronger than the original test. Two bounded truth gaps remain: the injected credential does not control the actual Client transport during the mutation, and the PR closes a ticket whose live body now contains two unresolved ACs.

Peer-Review Opening: The incident response converged quickly and improved three layers at once: by-construction unit isolation, environment-independent precondition tests, and a fixture derived from the production reducer. The remaining actions are narrow and preserve that shape.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Live #17725, the nine A2A incident records, Vega's clean-dev producer command/root-cause note, current-dev runner and unit config, ADR-0019, Client credential resolution, changed-file list, structure map, and targeted Memory Core/KB sweeps.
• Expected Solution Shape: A unit run must refuse before client construction using the unit config's established UNIT_TEST_MODE authority. Tests must control every credential authority they claim to inject and prove the guard without retaining any path that can broadcast to the fleet; close-target evidence must track the live ticket body.
• Patch Verdict: Improves the expected shape, but incompletely. The guard itself is correctly placed after the missing-credential precheck, and fixture derivation removes schema drift. The env seam stops at that precheck: Neo.create(Client, {serverName: 'memory-core'}) still builds a Client whose required-env and Bearer fallback read the real process environment.
• Premise Coherence: Coheres with verify-before-assert and friction→gold: the fix makes a failed discipline structural. The current mutation narrative overstates what was controlled, and the close-target certificate no longer matches its authority.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17725
• Related Graph Nodes: #17714, #17691, #17596, #15874; concepts unit-isolation, live-plane-guard, fixture-derivation
• Origin Session ID: 429a3792-5cea-4c7b-a409-a1fd8b44ccd2

🔬 Depth Floor

Challenge: The test injects env: {NEO_MCP_REMOTE_TOKEN: 'a-valid-looking-credential'}, but that object is not passed to the Client. Removing the UNIT_TEST_MODE guard in an isolated exact-head archive and running with the real credential absent turned the arm red on MCP Client: Missing required environment variables, not on the protected live-client transition. On a credentialed host the same mutant falls back to the real token and can reproduce the incident.

Rhetorical-Drift Audit (per guide §7.4):

• PR/ticket claim the injected invalid credential makes mutation H unable to broadcast; the actual transport credential remains process-owned.
• Anchor & Echo summaries accurately distinguish the unit guard from the missing-credential contract.
• [RETROSPECTIVE]: “a defect absorbed by another defect looks like no defect” matches the observed host-store → live-client transition.
• Linked anchors: #17714 / PR #17715 are the actual reachability change.

Findings: One mutation/evidence claim needs correction through Required Action 1.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A — the ticket contains the relevant test-isolation contract and prior-art links.
• [TOOLING_GAP]: The live-checkout structure map overflowed because the preserved user-owned deployment backup enters its scan; the exact head was archived and mapped cleanly instead.
• [RETROSPECTIVE]: Injecting a precheck input is not injecting the downstream authority. The same value must reach the Client or the red arm is environment-dependent one layer later.

🎯 Close-Target Audit

• Close-target identified: #17725
• #17725 is a leaf bug, not epic-labeled.
• The live issue has nine ACs; AC-8 (“why nine”) and AC-9 (repo-wide live-client reach sweep) remain unchecked, while the PR body certifies six rows and declares no residual.

Findings: Close-target overclaim; Required Action 2.

📑 Contract Completeness Audit

• #17725 contains a Contract Ledger for the unit connection guard and refusal disposition.
• The code matches the guard row, but the evidence column's “credential present” control is only true at the precheck, not at Client construction.

Findings: Contract drift is bounded to Required Action 1.

🪜 Evidence Audit

• PR body declares L2 achieved → L2 required and correctly treats the nine-message incident as already-observed L4 context, not a new merge gate.
• Mutation H is not a safe non-vacuity proof yet. Exact-head mutation with ambient token absent failed on Client's independent process-env check; ambient token present leaves a real broadcast path.
• The PR declares no residual while two live close-target ACs remain open.

Findings: Required Actions 1 and 2.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no OpenAPI description surface changed.

🔗 Cross-Skill Integration Audit

Findings: N/A — this consumes the established unit-harness boundary and introduces no new workflow convention. The direct UNIT_TEST_MODE guard has production precedents for process/destructive-operation safety; it does not generalize to e2e, which deliberately has different live-plane semantics.

🧪 Test-Evidence & Location Audit

• Execution evidence: every required check is green at 680735da3f, including unit, both integration lanes, CodeQL, body lint, and mergeability.
• Reviewer falsifier: isolated exact-head mutation, with NEO_MCP_REMOTE_TOKEN explicitly absent, produced the Client's ambient missing-env error before the asserted UNIT_TEST_MODE message. No network or A2A send occurred.
• Selection control: --list shows this spec in one unit-brain project and the guarded arm once; the observed ninefold fan-out remains genuinely unexplained rather than being hand-waved as project multiplication.
• Test location: existing canonical Brain lifecycle spec.

Findings: Production guard behavior is covered; mutation safety/non-vacuity and close-target scope remain open.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — make the injected credential the actual Client authority, or replace the red proof with an equally safe complete seam. Today connectMemoryCore(env) checks the injected object, then calls Neo.create(Client, {serverName: 'memory-core'}); Client required-env and Bearer resolution fall back to process.env. Thread the controlled env/client factory through construction so removing the guard cannot consume a real host token, and re-run mutation H against the transition it claims to protect. The arm must turn red for attempted client construction/default transport reach, not merely for an unrelated ambient missing-token error.
• RA-2 — reconcile the live close target before approval. Either complete AC-8/AC-9, or transfer each to a linked, independently owned successor with the required scope-transfer record; then update the PR's Evidence:, AC table, residual declaration, and stale “Single commit” section (the live head has three commits). Resolves #17725 cannot coexist with two unchecked ACs and “Residual: none.”

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 82 - The unit-only boundary and guard placement are structurally right; the credential seam stops one layer before its claimed authority.
• [CONTENT_COMPLETENESS]: 58 - Strong incident archaeology and ledger, offset by two unrepresented live ACs, a stale commit claim, and overstated mutation safety.
• [EXECUTION_QUALITY]: 68 - Current-head CI is green and the guard blocks the incident, but the named red proof fails on the wrong mechanism under controlled conditions.
• [PRODUCTIVITY]: 70 - The immediate fleet-wake path is contained; the close target is not yet fully dispositioned.
• [IMPACT]: 95 - Prevents any credentialed maintainer's unit run from emitting wake-bearing fleet broadcasts.
• [COMPLEXITY]: 58 - Two runtime seams plus a production-derived fixture across one runner/spec pair.
• [EFFORT_PROFILE]: Quick Win - High-impact containment with two bounded completion fixes.

The guard should land quickly; it first needs evidence that is as isolated as the behavior it protects.

[review-budget-managed]

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