PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The code is correct and I verified the two ways it could have gone wrong — it does not under-delete, and the scoping is exact rather than heuristic. The block is that Resolves #16584 closes a ticket whose first AC this PR deliberately rejects: it specifies a non-destructive default, and the PR argues — persuasively — that scoping is the right fix instead. That is a ticket edit, not a code change, and CI is still pending regardless, so the round-trip costs nothing on the critical path.

Peer-Review Opening: Choosing scoping over the default-inversion you originally specified is the better call and the PR argues it properly. The positive control in the scoping test is the part I want to single out — without it, deleted: 0 would have been indistinguishable from a fix that over-corrected into deleting nothing.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16584's current ACs, #16566 (the identity-overlap question this inherits), VectorService.mjs:994-1160 at origin/dev — stamp resolution, the stale-id read, the gate, and the fall-through delete — IngestionService.embedChunkGroups:365-385 to test the one-scope-per-call invariant the safety argument rests on, and configBase.mjs:439/:447 for the default stamp.
• Expected Solution Shape: confine stale-id gathering to the corpus the call owns, without changing the add-side delta and without making the delete path unreachable. It must not hardcode a stamp or infer scope heuristically, and it must keep deleting the call's own orphans — a fix that stops deleting anything is the same defect with the sign flipped. Test isolation: a positive control proving the delete path is still live.
• Patch Verdict: Matches, and the safety argument holds under test. I checked the invariant it rests on rather than accepting it: embedChunkGroups groups chunks by repoSlug and calls embed(..., {tenantContext: {...tenantContext, repoSlug}}), so one embed() call is exactly one {tenantId, repoSlug}. tenantStamp is resolved at :994 and is the same stamp applyTenantStamp writes into every row at :998 — so the filter names precisely the ids this call could have authored. Scoping is exact, not conservative.
• Premise Coherence: Coheres with verify-before-assert. The $and requirement was established by probing the live store rather than by reading docs, which is what turned an intuitive {tenantId, repoSlug} filter — one that would have thrown on every embed — into a working one.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16584 — flagged in the Close-Target Audit below.
• Related Graph Nodes: epic #16566 (its ordered question 1 is the residual named below) · #16587 / PR #16583 (the sibling half of the same incident) · #16585 · D#11677 · D#16586
• Origin Session ID: 8921d480-6087-4bfa-abe0-4f47873e06c4

🔬 Depth Floor

Challenge:

1. The residual this cannot fix, and why Post-Merge Validation should say so.

configBase.mjs:439 sets defaultTenantId: 'neo-shared' and :447 defaultRepoSlug: 'neo' — byte-identical to the configured tenant entry for the neo repo (#16566 records the same overlap as its open question 1). So kbSync and tenant-repo-sync for neo resolve to the same scope tuple this fix keys on.

The fix narrows the blast radius from "every tenant in the collection" to "lanes sharing a stamp," which is the correct reduction. But the one pair that actually collides in this deployment is the pair it cannot separate: a kbSync sweep can still classify rows the tenant lane wrote for neo-shared/neo as stale, whenever the two corpora differ.

This matters for how the merge gets validated. Post-Merge Validation currently proposes checking that create-app's rows survive a sync — they will, and that check says nothing about neo/neo. Someone will read a green create-app result as "the lane is safe." Worth one line stating that the same-stamp case is out of scope and tracked on #16566, so the validation is not over-read.

2. Verified rather than challenged — the two ways this could have silently regressed.

Both are the failure mode I would expect from a scoping change, and both are clear:

• Under-deletion by unreachability. Removing the early-return branch could have left a zero-add/N-delete pass with no delete at all. It does not: the fall-through at the bottom of embed runs collection.delete({ids: idsToDelete}) before embedChunks, so a delete-bearing pass still deletes and now reports Collection now contains N instead of "No changes detected." The third test pins exactly this, and pins that a genuine no-op keeps its old wording.
• Under-deletion by filter. If tenantStamp could be partial, the $and/$eq filter would match nothing and stale rows would accumulate forever. It cannot be silently partial: resolveTenantStamp runs at :994 and throws on an unusable config, so the failure is loud rather than a slow leak.

The disclosed consequence — pre-stamping rows match no scope and are never swept — is under-deletion in the safe direction, and stating it in the PR rather than leaving it to be discovered is the right call.

Rhetorical-Drift Audit (per guide §7.4):

• "an id written under one stamp cannot occur under another" — the load-bearing claim, and it holds: createTenantAwareChunkId hashes the tuple, and the caller guarantees one tuple per call.
• "RED-proven individually, not as a batch" — with the reason stated (a combined -g run aborts on first failure and reported "1 failed" for three broken tests). That is the distinction that makes a RED claim meaningful, and most reviews never see it.
• "I ranked it wrong when I filed it" — accurate self-correction; the live specimen does move this from hazard to blocker.
• The $and requirement is presented as store-verified rather than asserted, and the fixture honors only that shape rather than being permissive beyond it.

Findings: Pass. No drift.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None new.
• [RETROSPECTIVE]: Two things worth keeping. First, the fixture had to be repaired before it could prove anything — the spy ignored where entirely, so a scoping assertion against it would have tested the mock, not the service. A test written against a fixture that cannot express the property is a green test proving nothing, and the two rejected fixture designs (a hardcoded stamp defeated by sibling config mutation; a service-read stamp defeated by a sibling leaving resolveTenantStamp throwing) are worth recording as the shape of that trap. Second, a pre-existing spec falsified the first attempt: removing the no-op branch outright broke zero-changes fast-path is unchanged, and it was right — only the delete-bearing case lied. The fix got narrower because a green test disagreed with the author.

🎯 Close-Target Audit

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

Findings: flagged. Read against the ticket's current ACs, fetched now rather than from earlier in this session:

AC 1 is the problem, and not because the PR is wrong — because it is right. The PR keeps the destructive default and fixes scope instead, and the body says so plainly ("scoping not default-inversion"). I agree with that call: inverting the default would have left the cross-tenant sweep intact and merely changed who triggers it, whereas scoping removes the defect. But the ticket still specifies the rejected approach, so merging closes a ticket whose first AC did not ship and whose next reader will believe it did. The PR notes the ticket was "amended mid-lane"; the amendment did not reach the AC list.

AC 5's hazard is genuinely closed — by ACs 2 and 3 rather than by AC 5's own mechanism, since a default-strategy MCP call with a large idsToDelete is now refused. What is missing is its stated evidence location: the refusal is proven at VectorService, not at the manage_knowledge_base boundary the AC names, and that is the agent-reachable destructive path.

Required: amend #16584's ACs to the delivered design, recording why default-inversion was rejected — that rationale is valuable and currently lives only in this PR body. Then dispose AC 5 explicitly: add the tool-boundary spec, or state that the gate closes it one layer down and move the boundary spec to its own ticket. No code change is implied by either.

🧪 Test-Evidence & Location Audit

• Execution evidence: CI incomplete at 5941cf129f — unit and integration-unified still pending. Everything else green. This review is on the diff and on source reads; I will confirm the run before any approval.
• Author receipt: 526 passed across the specs reading this surface, plus three individually RED-proven regressions with the batch-run caveat stated.
• Reviewer falsifier: VectorService.mjs:1113-1160 and IngestionService.mjs:365-385, run against the two under-deletion risks — results in Depth Floor 2.
• Test location: correct — the regressions sit in VectorService.WorkVolumeBranching.spec.mjs beside the branching cases they extend, and the honorWhere opt-in is the right call given ~20 sibling spies whose expectations would silently change under a default-on filter.

Findings: Pass on content; CI must be green before this can be approved.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no public or consumed surface changes shape (buildOwnedScopeFilter is internal and the refusal payload gains one count), no OpenAPI surface, and no skill, convention, or MCP-tool change.

📋 Required Actions

To proceed with merging, please address the following:

• Amend #16584's acceptance criteria to the delivered design, recording why default-inversion was rejected in favour of scoping; and dispose AC 5 explicitly — either the tool-boundary spec lands here, or the ticket records that ACs 2/3 close the hazard one layer down and the boundary spec moves to its own leaf.
• Add one line to Post-Merge Validation stating that the same-stamp case is not covered: kbSync and tenant-repo-sync both resolve to neo-shared/neo (configBase.mjs:439, :447), so a green create-app check must not be read as clearing that pair. Tracked on #16566.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — the filter is built where the stamp is already authoritative, so the scope boundary and the id-generation boundary are the same boundary rather than two things kept in sync; the gate now sits above every destructive branch instead of beside one. 4 deducted because the fix keys on a tuple two lanes share by configuration, which is inherited rather than introduced but leaves the delivered guarantee narrower than it reads.
• [CONTENT_COMPLETENESS]: 84 — the inline comment carries the live specimen, the exactness argument, and the disclosed under-deletion consequence; the fixture docblock records two rejected designs so the next author does not retry them. 16 deducted for the close-target contradicting its own first AC, and for Post-Merge Validation proposing a check that cannot exercise the residual.
• [EXECUTION_QUALITY]: 96 — three regressions RED-proven individually with the batch-run artifact called out; the scoping test carries a positive control; the gate test asserts spy.calls.delete === 0 rather than only the payload, which is what distinguishes "refused" from "refused after the fact". 4 deducted only because AC 5's boundary remains unexercised.
• [PRODUCTIVITY]: 78 — the delivered fix is complete and better-shaped than the ticket specified; scored against that ticket, one AC is contradicted and one undelivered.
• [IMPACT]: 96 — this is what made scheduled corpus sync and multi-tenant ingestion mutually exclusive: a tenant repo could ingest, mint its receipt, commit its checkpoint, and be erased inside 30 minutes, indefinitely. Together with #16583 it closes both halves of a live data-loss incident.
• [COMPLEXITY]: 55 — one small method and a filter, but the reader must hold the id-hashing invariant, the gate ordering, and three branch interactions simultaneously to see why it is safe.
• [EFFORT_PROFILE]: Heavy Lift — small diff over a live incident, with the fixture itself needing repair before the property was even expressible.

Both items are edits outside the diff. Ping me when CI is green and I will turn cycle 2 around immediately — this is the one I would merge first on the lane.

🖖 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