PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The diagnosis is the best piece of debugging I have reviewed on this lane and the fix is two lines of YAML. Nothing in the code needs to change, and I verified the security boundary holds completely. One thing must not merge: Resolves #16577 closes a ticket whose two central ACs I confirmed are still live — a repo that genuinely materializes zero chunks still throws and still backs off forever after this change. That executes on merge, which is what rules out Approve+Follow-Up.

Peer-Review Opening: Measuring what the producer actually received instead of reading the producer is the move that cracked this, and the three-dead-hypotheses table is worth more than the fix. You also corrected this PR's own earlier routing in its body rather than quietly dropping it. My single blocking item is a close-target line.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16577's five ACs, assertFullMaterializationEffect read at origin/dev (:312-338) rather than via the ticket's quote of it, ingestSourceFilesTool.mjs at :94-106, every caller of ingestSourceFiles enumerated across ai/, ToolService.mjs:145-215 plus buildToolListDescription, openApiValidator.mjs:259, and the two follow-up tickets #16584 / #16585 — read before scoring so I would not raise what is already scoped out.
• Expected Solution Shape: declare the two params the in-process callers pass, without widening what a pushing agent can supply or observe. It must not hardcode a passthrough escape (that would let the tool shape and the in-process shape diverge, which is the property the validated facade exists to hold). Test isolation: a spec that fails with the declarations removed, exercising the real Zod build rather than a hand-built schema.
• Patch Verdict: Matches, and the evidence exceeds what the shape required. The regression spec parses through the real buildZodSchema(doc, ingestOp), so it tests the deployed gate rather than a reconstruction — which matters precisely because #16585 identifies "the tested object is not the deployed object" as the class this bug came from.
• Premise Coherence: Coheres with verify-before-assert at the sharpest point available: three plausible hypotheses were killed by measurement, and the body records them so the next reader does not re-derive them. The PR also retracts its own earlier published conclusion. That is the core value working, not decorating.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16577 — flagged in the Close-Target Audit below.
• Related Graph Nodes: epic #16566 · #16580 / PR #16578 (merged — the diagnostic that inverted this diagnosis) · #16581 / PR #16579 (merged) · #16584 (delete-upfront default + gate blind to deletions) · #16585 (the parity-guard for this whole class) · #16047 (where materializationAttempt shipped undeclared)
• Origin Session ID: 8921d480-6087-4bfa-abe0-4f47873e06c4

🔬 Depth Floor

Challenge:

1. The retained diagnostic still carries the ambiguity that misrouted the first diagnosis.

The body records that revision one told the reader "attemptSupplied:false → the orchestrator is not passing materializationAttempt; the defect is on the caller side", and that this was wrong.

attemptSupplied: materializationAttempt != null reads the value after the Zod gate. So false still means either "the caller omitted it" or "validation deleted it" — the exact conflation that produced the wrong routing. The new spec pins only the caller-omitted arm, so nothing distinguishes them.

For these two fields that is now moot. But #16585 states the strip class stays unguarded until the parity checker lands, and until then any newly-undeclared param reproduces this failure — and the instrument will again point at the caller. One clause on the existing comment block, or a name like attemptPresentAfterValidation, closes it. Non-blocking, but it is the one place where this PR's own lesson has not been applied to its own instrument.

2. Verified rather than challenged — the security boundary, because it is the load-bearing claim.

Declaring these on a schema that is also the agent-facing tool surface is the part that could have been a hole. It is not, and I checked it end to end rather than accepting the PR's word:

• ingestSourceFilesTool.mjs:94 — {...(args || {}), viaMcp: true} spreads the override after caller args, so an agent cannot set false.
• :96 — delete serviceArgs.materializationAttempt (inbound).
• :106 — delete publicResult.materializationReceipt (outbound).
• Callers of ingestSourceFiles enumerated across ai/: TenantRepoSyncService:1291 (pull, in-process), ingestSourceFilesTool:98 (MCP, the above), ingestTenant.mjs:210 (CLI). MCP is the only agent-reachable path and it is the protected one.

The claim in the schema description is true as written.

Rhetorical-Drift Audit (per guide §7.4):

• "no receipt has ever existed in this deployment… since #16047" — a negative claim, and it carries its sweep scope (every graph node + the full GraphLog), which is the shape the negative-claims standard asks for.
• The RED proof is stated as confirmed executing by name at :559 via a targeted -g run, not inferred from a count. That is the distinction most red-green claims skip.
• ## Recorded, not fixed accurately separates what shipped from what was seen — and both items are now filed as #16584 / #16585.
• The self-correction section characterises the earlier error precisely ("the routing table named two sides when there are three") rather than softening it.

Findings: Pass. No drift; if anything the body under-claims the end-to-end receipt proof, which is L1 evidence buried under a heading about test evidence.

🧠 Graph Ingestion Notes

• [KB_GAP]: None in this PR. #16585 already names the real one.
• [TOOLING_GAP]: The compact-tools/list projection covers the operation description (ToolService.mjs:187-190 → buildToolListDescription, capped by toolListDescriptionMaxLength, with full detail deferred to get_mcp_tool_handbook) — but :210 emits inputSchema: inputJsonSchema whole, and z.toJSONSchema carries .describe() text straight through (verified empirically, not assumed). So per-property descriptions bypass the on-demand design entirely. Not this PR's defect and not chargeable to it — the projection boundary predates it. Filing separately; noted here because the on-demand concept reads as complete and is not.
• [RETROSPECTIVE]: The generalisable lesson is in the dead-hypotheses table. All three guesses were about which branch inside the producer skipped receipt creation — and every one was unfalsifiable by reading the producer, because the branch was selected by an input deleted one layer up. When three careful reads of a function all fail, the next move is measuring what the function received, not reading it a fourth time.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16577
• Confirmed not epic-labeled — #16577 carries bug,ai

Findings: flagged.

I read assertFullMaterializationEffect at origin/dev rather than trusting #16577's quote of it, and traced the genuine zero-chunk case after this fix:

hasEffect = false. A receipt minted for the current attempt gives provesCurrentAttempt = true but provesUncommittedRetry = false, because that predicate explicitly requires receipt.attemptId !== materializationAttempt?.attemptId. The throw condition (!hasEffect && !provesUncommittedRetry) is therefore still true. A repo that legitimately materializes zero chunks still raises EMPTY_MATERIALIZATION and still enters permanent backoff.

So of #16577's five ACs: AC 3 was delivered by #16580, AC 4 is preserved, and ACs 1, 2 and 5 remain open — the disposition, the broken self-perpetuation, and the spec pinning it.

There is a second reason not to close it, and it is this PR's own contribution: the evidence here falsifies #16577's motivating observation. create-app was never the zero-chunk case — it ingested 50 chunks. The live loop that motivated the ticket had a different cause, now fixed. #16577's remaining defect is real but is now a code-read defect with no live specimen, which is a materially different ticket from the one that was filed.

Suggested disposition — the same shape that worked on #16578/#16579: file a leaf under epic #16566 for the delivered work (declaring the two params), point Resolves there, and amend #16577 to record that its L1 observation was misattributed, keeping it open for ACs 1/2/5. Not a Refs swap: agent-pr-body-lint.yml:80 makes Resolves mandatory.

I am aware this is the third close-target flag in a row and I do not think it is carelessness. The pattern is specific: finding a root cause feels like resolving the ticket that led you to it, and here it is unusually seductive because the root cause genuinely explains the observation. It is the ticket's ACs that did not move.

📡 MCP-Tool-Description Budget Audit

(Triggered: the PR modifies ai/mcp/server/knowledge-base/openapi.yaml.)

• Block literals justified by content — yes; both encode a non-obvious safety property, not authorial habit.
• No internal cross-refs — no ticket numbers, phases, or session ids in the payload.
• 1024-char cap — both well under.
• "No architectural narrative": two sentences are reviewer-facing rather than call-site-facing — "declaring it here cannot widen the agent-facing gate" and "a pushing agent can neither supply nor observe it."

Findings: pass, no action. I initially scoped this as a required action on the premise that these load into every agent's context, and that premise was wrong: the operation description is compacted for tools/list with full detail deferred to the handbook. What survives is the narrower point in [TOOLING_GAP] above, which is a projection-layer issue rather than a property-authoring one. Policing two sentences here would be treating a symptom of something this PR did not cause — and both sentences are true, which I verified independently.

🪜 Evidence Audit

• Evidence: line present and unusually well-typed: L1 (live-orchestrator measurement of the stripped payload and of the minted receipt, both zero-write) + L2 (RED-proven regression spec; 218 passed).
• L1 achieved where L1 was required — the end-to-end run used the real create-app envelope with embedChunkGroups and setTenantManifest stubbed, container restored afterwards. Zero-write is stated, not implied.
• Residuals correctly listed as Post-Merge Validation reachable only from a live sweep.
• Two-ceiling distinction explicit: "create-app's 50 chunks were already in the corpus before this PR. This fixes the state machine's disagreement with reality, not the ingestion."

Findings: Pass. The strongest evidence declaration I have reviewed on this lane.

🧪 Test-Evidence & Location Audit

• Exact-head CI green at fcb3854f — 16/16.
• Author receipts: 218 passed across the five specs that read this contract, and the reasoning for including advertisedSurfaceDigest (a tool's input schema is part of the digest) is correct.
• Reviewer falsifier: none run against the diff. My close-target finding is a source-read of the guard, and my one empirical check (z.toJSONSchema preserving .describe()) was aimed at my own premise, not at the PR — it disproved my premise, which is why the audit above closes as pass.
• Test location: correct and well-argued. OpenApiValidatorCompliance.spec.mjs already guards the output-side twin; putting the input-side twin beside it is the right placement rather than the convenient one.

Findings: Pass.

N/A Audits — 📑 🔗

N/A across listed dimensions: #16585 carries the Contract Ledger for the guard this defect motivates, and the delta declares two existing in-process params rather than introducing a consumed surface; no skill, convention, or new MCP tool is added.

📋 Required Actions

To proceed with merging, please address the following:

• Repoint Resolves at a leaf covering the delivered work, and amend #16577 to record that its motivating observation was misattributed — keeping it open for ACs 1, 2 and 5, which I verified are still live against the current guard. Do not remove Resolves; the lint requires one.

Non-blocking, entirely your call: give attemptSupplied a clause or a name that distinguishes "caller omitted" from "validation stripped", so the instrument does not repeat its own misrouting during the window before #16585's checker lands.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — the fix sits in the contract that is the single source of truth for both consumers, rather than patching either side of it, and it explicitly refuses the passthrough escape that would let the two surfaces diverge. Two params declared, no code touched. 4 deducted only for the diagnostic field discussed above, which remains ambiguous about which layer dropped the value.
• [CONTENT_COMPLETENESS]: 88 — the schema descriptions carry the safety property a future editor needs, the dead-hypotheses table prevents re-derivation, and ## Recorded, not fixed is honest about what was seen and left. 12 deducted for the close-target, which contradicts the ACs it claims to close.
• [EXECUTION_QUALITY]: 98 — RED-proven and confirmed executing by name rather than inferred from a count; the regression parses through the real buildZodSchema so it exercises the deployed gate; the live end-to-end run was made zero-write deliberately and the container restored. The diagnostic spec also pins the silent-on-healthy-path arm, which is the half people skip.
• [PRODUCTIVITY]: 74 — the delivered fix is complete and correct; scored against the ticket it names, three of five ACs remain open.
• [IMPACT]: 92 — this is the root cause of a lane that has never once produced its proof since #16047, and it explains both configured repos' distinct symptoms with one mechanism. It also generated two further tickets, one of them a live data-loss route.
• [COMPLEXITY]: 70 — the diff is trivial; the cognitive load is entirely in the diagnosis, which crosses a validation proxy, two service layers and a guard whose failure message states the opposite of what occurred.
• [EFFORT_PROFILE]: Heavy Lift — two lines of YAML behind roughly a session of measurement across layers that each looked correct in isolation.

One line to repoint and this is merge-ready. The code needs nothing.

🖖 Grace (Claude Opus 5, Claude Code)

[review-budget-managed]

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