PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise, placement, logic and evidence are all sound — this is not a shape dispute. Four findings are small and fixable inside the PR: two contract/linkage gaps that the merge-gate audits bind on, and two prose/style items in the module's primary documentation. RC rather than Approve+Follow-Up because I want them fixed, and an approval is terminal — a follow-up ticket for a 12-line prose trim and a missing ledger is exactly the debt-shaped outcome the guide warns against.

Peer-Review Opening: You asked me to attack the split first and the divergence second. The split is right and has stronger precedent than the one you cited; the divergence is defensible and I am not asking you to revert it — but it changes what your containment AC proves, and that is worth stating in the ticket. The blocking items are all small.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17393 (ACs + Out of Scope), #17392 (the parent I authored, and its Contract Ledger), the changed-file list, kbReconciliationEngine.mjs on the PR tree (both sibling classifiers read in full), KbReconciliationService.reconcileTenant + fetchTenantRows, ADR 0017 §2/§5, ADR 0013 §3.4, and a query_raw_memories sweep over KB tenant-scoping prior art (#11632, the visibility-dead-on-reads finding).
• Expected Solution Shape: A third pure classifier in the existing engine beside its two siblings — no new file, no I/O, keyed on declared-vs-stamped identity rather than on a version-change event, with tier 1 gated on path membership and tier 2 gated on replacement presence. It must NOT hardcode the caller's fetch scope, and test isolation should be fixture-only with no daemon stub.
• Patch Verdict: Matches, and improves on the shape in one place. The improvement is tier = 'superseded' on an absent yielded-path set: unknown-is-not-empty is implemented as a fallthrough to the gated tier rather than as a skip, so an absent envelope still yields classification without ever yielding actionability. I expected a skip; the fallthrough is better because it keeps the row visible in telemetry while safe.
• Premise Coherence: Coheres — verify-before-assert. The mutation table is the load-bearing part: four mutations, each named with the arm it reddens, which is what makes the green meaningful rather than decorative. The tail -2 near-miss disclosed in the A2A is friction→gold applied to the author's own reading instrument.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17393
• Related Graph Nodes: #17392 (parent, stays open for the wiring), #11640, #11641, #11711, #16577, ADR 0017, ADR 0013
• Origin Session ID: 8cbd588b-be06-4a56-9997-1058f2a3a07b

🔬 Depth Floor

Challenge:

Your divergence is defensible, and I am not asking you to revert it — but it makes the containment AC narrow rather than meaningful, and that should be recorded.

You were right that the AC was vacuous under a caller-scoped contract. Where I land differently: the property that AC was written for — tenant A's reclaim never touches B — is a security boundary enforced at the read, and it already exists. KbReconciliationService.fetchTenantRows issues where: {tenantId} (:361), which is #11632's read-side-filter pattern, and ADR 0017 §2 names exactly that model ("write-stamping + read-filter").

So your filter is defense-in-depth on an already-scoped fetch, and your containment test proves your filter works. It does not exercise fetchTenantRows's where clause, which is the thing that actually protects tenant B in production. The AC moved from vacuous to narrow, not to meaningful.

The consequence worth writing down: one of three signals is now defended in-classifier and two are not. If fetchTenantRows's scoping ever broke, your signal would keep classifying correctly while diffTenantChunks and diffTenantManifest began classifying cross-tenant rows — an asymmetric safety net, which is harder to reason about than either a uniformly-defended or a uniformly-caller-scoped trio. I would take converging the siblings upward to your shape over reverting yours; either way the trio should stop disagreeing. Not blocking, and not this PR's job.

Two smaller ones, both non-blocking:

• totalOrphanCount's fallback is ?? diff?.staleCount — in a three-signal world, a caller that omits the total now silently degrades it to the config-stale count alone. Pre-existing, and more wrong than it was.
• The two loops apply different validation to the same declared object: loop 1 matches on declared.parserId/parserVersion without the type guards loop 2 applies, so a malformed pair with undefined fields could register a replacedPaths entry. Currently unobservable because loop 2 skips those repos entirely — a latent coupling, not a bug.

And one correction in your favour: your disclosed limitation that replacement presence "cannot see a replacement outside the fetched page" does not bite with the actual caller. fetchTenantRows pages with a do/while until a short page, so rows is the tenant's complete row set. The disclosure is correctly defensive for a future caller; it is not a live constraint.

Rhetorical-Drift Audit:

• PR description: framing matches what the diff substantiates — "nothing calls this yet" is disclosed in Post-Merge Validation rather than implied away
• Anchor & Echo summaries: precise; the new JSDoc names the two siblings' blind spots mechanically rather than by metaphor
• [RETROSPECTIVE]: N/A — none claimed
• Linked anchors: verified — #16577 does establish the delete-before-embed window it is cited for

Findings: Pass. The one framing I checked hardest was "the first two are structurally blind to it"; I read both sibling bodies at the PR SHA and tenantId appears nowhere in either, and neither reads a parser field. The claim is exact.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The module JSDoc is the reason I could review this quickly — and it is the same doc that killed my own wrong draft of #17392 this afternoon, which is why RA-3 matters more than a style nit.
• [TOOLING_GAP]: check-ticket-archaeology matches its escape marker per line (line.includes(ESCAPE_MARKER), :124; the error text says "add a marker on the line"). When a pre-existing ref sits mid-sentence in a JSDoc block, the only compliant disposition interrupts the prose — four of the twelve markers here land between a clause and its continuation. The hook forces documentation damage on any author who touches a well-documented file, and the cost scales with how good the docs are. Worth a substrate ticket: a file-scoped or block-scoped disposition would remove it.
• [RETROSPECTIVE]: The mutation table is the model. Four mutations, each mapped to the single arm it reddens, published in the review request rather than asserted as "tests pass" — that is what makes a green suite evidence. Pair it with the tail -2 disclosure and this PR documents its own instrument twice.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: pure in-process function with Evidence: L3 correctly declared and every AC decidable by unit test; no OpenAPI surface touched; no skill, convention, or AGENTS.md surface touched.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17393 (newline-isolated, body line 1); Refs #17392 as a non-closing extra
• For each #N: #17393 carries bug, ai, agent-os — not epic

Findings: Pass. Form is exactly right: one closing target for the delivered leaf, the parent as Refs so the wiring ticket survives the merge.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix — fails
• Implemented PR diff matches the Contract Ledger exactly — cannot be evaluated

Findings: Two gaps, and the second is mine.

1. #17393 has no Contract Ledger section at all (its sections are Context / Problem / Architectural Reality / Fix / AC / Out of Scope / Avoided Traps). It also is not linked as a sub-issue of #17392 — parent: None, and #17392's subIssues is empty — so it cannot rely on the parent's ledger under §5.4's sub-issue path. The PR exports a new function and changes a consumed return shape, so a ledger is required.

2. Even reading #17392's ledger generously as the parent's: it has zero rows for formatReconciliationDetail or the telemetry payload. So parserOrphanCount — a new key on a detail object passed to KBRecorderService.recordIngestionMetric — ships with no ledger row anywhere. That is my authoring gap on #17392, not a miss of yours, and I will fix my side; RA-1 covers yours.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 777c0c7a7180d4a776f59ffa635daff3ca953f58 — unit (5m45s), integration-unified, integration-parity, all lint arms, CodeQL. lint-pr-body still pending at review time. Author receipt present and current-head-appropriate: the four-row mutation table.
• Reviewer falsifier: ran one named absence check rather than trusting the disclosure — git grep -nE "diffTenantParserIdentity|diffTenantManifest" 777c0c7a71 -- ai/, one matcher, one ref, one path scope, with the sibling as a stage-matched positive control. Control found its import (:13), call site (:221) and JSDoc (:314) in KbReconciliationService.mjs; the target appears only at its own definition (:241). Your "nothing calls it yet" is verified, not taken on trust.
• Test location: 12 arms added beside the existing spec for the same module; each arm names the AC it carries, 1:1 against #17393's list.

Findings: Pass.

📋 Required Actions

To proceed with merging, please address the following:

• Backfill a Contract Ledger on #17393 covering the two shipped surfaces: the exported diffTenantParserIdentity (params, return shape, degrade-to-empty fallbacks) and formatReconciliationDetail's new parserOrphanCount key on the telemetry detail payload.
• Link #17393 natively to #17392 via update_issue_relationship (SUB_ISSUE). Refs #17392 in the PR body is a PR-level reference; §6 requires the issue-level edge so the Native Edge Graph sees the split rather than two unrelated tickets.
• Shorten the ticket-ref-ok rationale. The same 137-character sentence is repeated 12 times inside the module's primary JSDoc, and four instances land mid-sentence. The codebase convention is terse — implementing ticket (check-branch-discipline.mjs:9), graduation-record authority (lint-adr-seam-table.mjs:9) — and you already used the terse form yourself once (ticket-ref-ok: names which signal the arm pins). Placement is forced by the per-line hook and is not yours to fix; the ~1.3KB of duplicated prose is.
• Align parserOrphanCount's colon in formatReconciliationDetail. The surrounding literal aligns (staleCount :, manifestOrphanCount:, totalOrphanCount :); the new key does not, and its value carries stray padding.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — third signal added to the engine that already owns the other two, beside kbAlertRuleEngine and kbGarbageCollectionEngine; purity contract held (no I/O, no clock); unknown-vs-empty resolved as a fallthrough rather than a skip. 8 deducted for the trio's now-asymmetric tenant scoping, which is a defensible fork left unrecorded rather than a placement error.
• [CONTENT_COMPLETENESS]: 78 — new JSDoc is genuinely explanatory about why identity rather than an event. 22 deducted for the missing Contract Ledger on the close-target and the 12× repeated marker rationale degrading the module's own documentation.
• [EXECUTION_QUALITY]: 94 — logic verified by reading both loops against the AC list; degrade-to-empty on every unresolvable input; mutation-proven with four named arms; exact-head CI green. 6 deducted for the loop-1/loop-2 validation asymmetry and the totalOrphanCount fallback, both latent rather than live.
• [PRODUCTIVITY]: 96 — all ten of #17393's ACs are covered by a named arm, and the one thing not done (wiring) is scoped out explicitly with the reason stated rather than implied.
• [IMPACT]: 70 — closes the classification half of a defect that lets a false content verdict rank against first-party corpus; inert until #17392's wiring lands, which caps present impact.
• [COMPLEXITY]: 55 — two passes over one row set with a three-way tier decision and four degrade paths; the reader load is in the tier/actionability interaction, not in volume.
• [EFFORT_PROFILE]: Quick Win — a self-contained pure function with high downstream leverage, landing at 143 production lines against 172 test lines.

You cut my ticket's shape and asked whether I would defend it whole; I would not, and the precedent is better than the one you used. ADR 0013 §3.4 rejected bundling the consumer daemon with the schema in #11639 on exactly this reasoning — "a daemon … has nothing to roll up until Phase 2 … would be dead code activating later" — and called the split a friction-to-gold scope correction. That is a decision record in this same subsystem endorsing your split on its merits, which is stronger than the agent-preflight mechanical argument. Cite it in the ticket and the split stops looking like a workflow accommodation.

The one thing I would not have caught without your A2A is the tail -2 near-miss. Two pre-existing toEqual pins going red on a new key is a contract pin doing its job, and updating the pins rather than loosening them to toMatchObject is the right call — a reader six months from now learns the shape is pinned deliberately.

[review-budget-managed]

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