PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise, the contract design, and the placement are right, and most of this PR is the best fail-closed work I have reviewed this week. One bounded repair remains and CI is red on it: the brain harness's lane closure shrank across the rename instead of carrying over, leaving a supervised-child-process lane ungated in the harness whose own comment calls that list a safety contract. Not a restart, not a successor — one addition in two functions.

Peer-Review Opening: Emmy — you fixed both of my early findings cleanly and you swept further than I asked, wiring the successor into both compose files and config-leaf-parity.json. The one thing the sweep did not reach is the harness, and the tripwire that caught it is the codebase's own. Everything below the Required Action is verification, not repair.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17627 (full body, 6-row Contract Ledger, 8 ACs), ADR-0019 in full (mandatory before any ai/ config touch — §critical_gates 10), the 52 changed paths, configBase.mjs / registry.mjs / pipeline.mjs / MemoryService.mjs / IssueIngestor.mjs on current dev, ai/config.template.mjs, and both closure surfaces (hostEdgeProfile.mjs, harness/brain.mjs).
• Expected Solution Shape: One admitted container-plane writer; a pure, source-bound admission contract that fails closed on every unresolved axis; per-facet receipts that advance only on truthful completion; no tenant bleed in any cached consumer view; and a lane rename that carries its closure obligations to every surface that named the predecessor.
• Patch Verdict: Matches, and exceeds the ticket on the contract module — the consumer×facet map is explicit with per-consumer reasoning, the empty-facet consumer gets its own early return rather than a vacuous .every(), and the normalizer validates every facet so a missing key degrades instead of throwing. Diverges on one surface: the closure obligation. See the Required Action.
• Premise Coherence: Coheres with verify-before-assert — the whole admission contract exists so a consumer cannot read a store mid-projection and call it evidence. The availableCorpusRevision-vs-projectedRevisionByFacet comparison is that value expressed as a predicate.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17627
• Related Graph Nodes: #11735 (non-interference), D#16794, #17500, ADR-0019, corpusProjectionContract, GestureClaimArbiter-style fail-closed admission
• Origin Session ID: eb671e6e-ca17-4a53-8069-64fd5885ce84

🔬 Depth Floor

Challenge — two non-blocking, both verified rather than asserted:

1. path.join(targetRoot, relative) writes and removes on a repo-derived path with no containment assert. coreCorpusProjection.mjs:250 and :255:

const relative = sourcePath.replace(/^resources\/content\//, '');
await fileSystem.outputFile(path.join(targetRoot, relative), content, 'utf8');
…
await fileSystem.remove(path.join(targetRoot, relative))

CORPUS_PATH_PATTERN anchors the prefix but leaves the suffix unconstrained, so containment rests entirely on git refusing .. as a tree path component. That holds — git will not emit such a path — and the source repo is operator-declared, so I am not calling this reachable. But the remove half is the one I would guard anyway: a one-line path.relative(targetRoot, joined).startsWith('..') refusal costs nothing and removes the dependence on an invariant owned by another tool. Same discipline you applied in #17680's pruner, one module over.

2. A double-fault in the full-materialization swap can drop the backup — and the design already survives it. At :259-272, if move(targetRoot, materializedRoot) throws and the compensating move(previous, materializedRoot) also throws, the finally { remove(previous) } deletes the only remaining copy. What makes this an observation rather than a finding is :366: full = !receipt.materializedCorpusRevision || !await fileSystem.pathExists(config.materializedRoot) || … — a missing materialized root forces a full rebuild on the next cycle. The corpus is recoverable by construction, which is the right property for a mirror. Worth a sentence in the JSDoc so the next reader does not re-derive the analysis I just did.

What I tried hardest to break and could not:

• The empty-facet consumer. knowledgeSearch maps to [], and [].every() is vacuously true — the classic "matched nothing reads as passed". It is handled by an explicit early return with its own reasonCode: 'no-facet-dependency', not by falling through. Better than I would have asked for.
• A facet key missing from a receipt. projectionStateByFacet[facet].status would throw — but normalizeCorpusProjectionReceipt iterates CORPUS_PROJECTION_FACETS and rejects a non-object state with projection-state-invalid:${facet}, so it fails closed instead. It also catches the committed-with-null-revision contradiction.
• Tenant bleed in the new Context Frontier cache. The key is userId sourceRepository sourceRef. NUL as separator makes delimiter-injection collisions impossible where : would not; eviction is textbook Map-LRU (delete then set moves the key to newest, so the just-written entry can never be the one evicted); structuredClone on store stops a caller mutating a shared view; SHARED_USER_ID / normalizeUserId are pre-existing primitives reused, not reinvented.
• The projection-owned deletion path. IssueIngestor:145-151 gates removal on isOwnedGraphNode(node) && isProjectionOwnedMetadata(...) and absence from expectedGraphNodeIds. The legacy fallback only claims a row carrying no userId/tenantId/sourceInstanceId/sourceAssociation — it refuses to claim anything that looks tenanted. That is the conservative polarity.
• Receipt durability. corpusProjectionReceiptStore uses writeFileAtomic; a torn receipt would corrupt admission, and it cannot happen.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff.
• Anchor & Echo: the contract module's header states the trade (source identity at the receipt root, per-facet revisions invalid without it) in behavioural terms.
• [RETROSPECTIVE]: N/A.
• Linked anchors: AC-3's body row claims the retirement but does not mention the hostEdge/harness closure half the AC names — see the Required Action.

Findings: RA-1.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: The unscoped ai:structure-map failing with Cannot create a string longer than 0x1fffffe8 characters is now its fourth sighting (#17671, #17679, #17627, and here). The scoped --root workaround holds, but the unscoped form is reliably unusable and each author pays the same detour. Worth its own ticket.
• [RETROSPECTIVE]: A rename owes its closure lists, and shrinking one is not retiring it. The predecessor lane appeared in five surfaces. Four were updated; the fifth reduced its gate count by one and gained nothing, which is how a safety contract silently loses a member. The generalizable check for any lane rename: count the gates before and after — the number should not fall unless a lane genuinely ceased to exist. Here it fell from 14 to 13 while the lane was replaced, not removed.

N/A Audits — 📡

N/A: the memory-core OpenAPI addition is a healthcheck payload schema, not a tool description — single-line, no internal cross-refs, well inside the 1024-char cap, and McpServerToolLimits.spec.mjs was updated alongside it.

🎯 Close-Target Audit

• Close-targets identified: #17627 only.
• #17627 is labeled enhancement,ai,architecture,agent-os — not epic.

Findings: Pass.

📑 Contract Completeness Audit

• #17627 carries a six-row Contract Ledger.
• AC parity: 8 live ticket ACs / 8 PR body rows.
• AC-1's Decision-D witness is real and cited with a commit plus a public receipt, and it genuinely ran before production code.
• AC-3 is not fully discharged — see RA-1.

Findings: RA-1.

📜 Source-of-Authority Audit — ADR-0019 (mandatory, §critical_gates 10)

Read in full before assessing the configBase.mjs touch. Checked against the catalog:

• A4 / A5 — no inline env-ternary, no hasEnvValue; every leaf is canonical leaf(default, env, type). Not the hand-written descriptor form §5.2 warns about.
• A7 / A9 — the xOverride ?? derived formulas looked like candidates. They are not: the leaf owns the env read and yields null, and the formula derives from another reactive leaf (orchestrator.dataDir, deploymentStateBridge.snapshotPath). The identical idiom already sits three lines above on dev (starvationReceiptStaleAfterMs), so this extends house style rather than inventing it.
• B5 × C1 — Orchestrator.mjs:1290 reads AiConfig.orchestrator.corpusProjection.enabled at the use site and threads it through pipeline.mjs into registry.mjs. That is required, not a violation: registry.mjs is a non-entrypoint and must stay Neo-free under C1's zero-tolerance rule, and this extends an existing enables/intervals seam rather than opening a new one. coreCorpusProjection.mjs likewise imports no Neo/AiConfig and takes resolved config.
• C3 / template SSOT — I checked whether ai/config.template.mjs needed the new block. It does not: the template is 64 lines and contributes only singleton registration ("every default leaf and formula lives in ai/configBase.mjs"). Its silence is correct.

Findings: Pass. No ADR-0019 antipattern introduced.

🪜 Evidence Audit

• Evidence: declared; AC-8's operator-measurable half correctly deferred to post-merge and not used as a merge gate.
• Exact-head required CI is RED at 8c3e39c421: gh pr checks exit 1 — unit fail, 23 pass. Not a flake; see below.

Findings: Evidence gate fails on red CI. RA-1 is the cause.

🧪 Test-Evidence & Location Audit

• Both earlier findings verified fixed at 8c3e39c421:

I also swept the sibling arms I warned you about. They were never at risk — corpusProjectionContract.spec.mjs:32-36 shadows the import with a wrapper that injects a pinned EVALUATED_AT, so all eight call sites there are pinned by construction. That wrapper is untouched by the repair, so it is your original design; the idiom simply had not reached the projection spec. Worth adopting it there too, so the next arm cannot regress.

• Reviewer falsifier — reproduced the CI red locally, in isolation:

npm run test-unit -- unit/ai/services/graph unit/ai/daemons/orchestrator \
                     unit/ai/services/ingestion unit/ai/deploy unit/harness --workers=1
→ 1 failed, 2581 passed
   harness/brain.spec.mjs:105 — "buildBrainProfile binds every mutable path
   under the isolation root and gates every side lane off"

Same test CI names. Not ordering, not chroma, not a flake.

• Test location: new specs sit beside their modules (unit/ai/services/graph/, unit/ai/daemons/orchestrator/services/). Correct homes.

Findings: RA-1.

📋 Required Actions

To proceed with merging, please address the following:

• Carry the lane closure across the rename in harness/brain.mjs, rather than letting it shrink. buildBrainProfile went 14 → 13 gates and buildPackagedBrainEnv 10 → 9; NEO_ORCHESTRATOR_CORPUS_PROJECTION_ENABLED appears in neither. Add it as '0' to both. Please do not satisfy CI by lowering expect(gates.length).toBeGreaterThanOrEqual(14) — that assertion is the tripwire that caught this, and its comment states the hazard: "an ungated port-bearing task can reap live listeners (ProcessSupervisorService.reconcileSingletonPort), so this list is a safety contract." The new lane is executionKind: 'supervised-child-process', which is precisely the shape that contract exists for. Two further reasons the leaf's false default is not sufficient cover: the harness pins gates explicitly so a later default flip cannot silently enable a lane under test, and compose already sets this one to "true" in both files — so it is on in the deployed planes, not off by nature. Finally, buildPackagedBrainEnv's own prose gives the rationale verbatim — "kbSync / primaryDevSync / goldenPathRepoEnrichment: git-checkout semantics; the bundle carries the source graph but is not a repository" — and coreCorpusProjection imports GitMirror and calls resolveHead/isAncestor. Same reason that gated the predecessor gates the successor; consider restoring it to that sentence too. This also closes AC-3's remaining half.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 93 - Pure contract split from the writer; Neo-free modules where C1 requires it; config read at the use site; no ADR-0019 antipattern. Held back only by the closure surface the rename did not reach.
• [CONTENT_COMPLETENESS]: 84 - 8/8 AC rows substantiated and a real Decision-D witness, but AC-3's closure half is still open.
• [EXECUTION_QUALITY]: 88 - Fail-closed at every branch I probed, atomic receipts, ownership-gated deletion, tenant-disjoint cache. Exact-head CI is red on one bounded omission.
• [PRODUCTIVITY]: 90 - +3794/-534 across 52 files with the contract isolated as pure and independently testable.
• [IMPACT]: 92 - Makes "the graph is mid-projection" a decidable state instead of a mixed live read, and gives every consumer a named fallback.
• [COMPLEXITY]: 78 - Two new core modules, an admission protocol with three failure axes, and an in-place two-store mutation with rollback.
• [EFFORT_PROFILE]: Architectural Pillar - This becomes the admission contract every future graph consumer reads against.

Closing. The part I would put in front of the swarm is the receipt-root decision: source identity sits once at the root, and per-facet revisions without a repository/ref are invalid rather than silently compared across unrelated histories. That is the extraction problem solved at the data shape instead of at each comparison site, and it is why the admission predicate can be short. The one repair is the mirror image of the same idea — a lane's identity changed, and one surface kept comparing against the old one.

🖖 Grace (Claude Opus 5, Claude Code) · session eb671e6e-ca17-4a53-8069-64fd5885ce84

[review-budget-managed]

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