PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The config is correct — I verified every claim in it against the live remotes rather than the body — and the vacuity fix is the right repair. One thing is unstated and it is a live-plane hazard: this must not merge before #16590. Registering two additional tenant repos onto a plane where kbSync still sweeps unscoped means three corpora get deleted every 30 minutes instead of one, and three of this ticket's four post-merge ACs would fail in a way that looks like this PR's fault. One line in the body fixes it; CI is pending anyway.

Peer-Review Opening: Catching the vacuity trap yourself is the part worth naming — undefined !== 'main' passing green is exactly the failure that survives review, because nothing turns red. Reading the raw yaml instead of the normalized entry is the sharper half of that fix.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16592's ACs, #16566's ordered question 1 (which this answers), the current kb-config.yaml and its contract spec, #16590's scoping fix and the residual we folded into its Post-Merge Validation, and — before reading the diff as evidence — the three tenant remotes themselves.
• Expected Solution Shape: remove the entry whose stamp collides with kbSync's default, add external repos small enough that a multi-repo witness costs nothing, and keep the contract spec pinned to the tracked bytes. It must not hardcode a branchRef that the remote does not have, and the spec must not go vacuous when the entry it referenced disappears.
• Patch Verdict: Matches, and the config's factual claims hold under independent check. Verified against the live remotes rather than the PR body: all three repos exist, are public (so credentialRef: none is correct), and each has main as its default branch — create-app 50 tracked files, devindex-opt-in 4, devindex-opt-out 3. The 50 matches the chunk count from the live incident exactly, which is a useful corroboration of the whole diagnosis.
• Premise Coherence: Coheres with the operator's stated direction. The config comment says neo "returns as a tenant once sources and parsers are declarable per tenant… that is sequencing, not a retreat from the goal" — which is the right framing: this removes a duplicate untyped corpus, it does not abandon neo-as-tenant.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16592
• Related Graph Nodes: epic #16566 (this answers its ordered question 1) · #16584 / PR #16590 (merge-order dependency, below) · #16587 / PR #16583 · #16557 (held) · D#15605 (hub) · D#12034
• Origin Session ID: 8921d480-6087-4bfa-abe0-4f47873e06c4

🔬 Depth Floor

Challenge:

1. Merge order is load-bearing and unstated.

kbSync resolves to {neo-shared, neo} and, before #16590, gathers stale ids across the whole collection — so it classifies every tenant repo's rows as stale. That is the mechanism that deleted create-app's 50 rows at 08:23.

Today one tenant repo is exposed to that. After this PR, three are. If #16593 lands first:

• all three corpora are wiped on each kbSync interval, indefinitely;
• AC 4 ("create-app's rows survive the following kbSync"), AC 5 ("no lane deletes another's rows") and effectively AC 3 cannot pass;
• the failure presents as "the new roster does not work", which is the wrong diagnosis and costs someone a cycle.

Nothing in the body, the ACs, or the Related: line says #16590 is a prerequisite. It is approved and ready, so the natural order is probably fine — but "probably fine" is not a merge instruction, and this is a live plane. One line stating the dependency turns an ordering assumption into a checkable one.

2. The spec is now stricter than the code contract, deliberately — worth being explicit that it is a policy, not a mirror.

tenantRepoAccessContract.mjs documents branchRef as "only present when configured", with an unset value following the remote default. The new assertion requires every entry to declare it explicitly, so a future entry that legally omits branchRef fails this spec.

I think that is the right call — the config comment's reasoning ("copying a sibling's branchRef is a clone failure waiting to happen") applies equally to inheriting a default silently. But the test currently reads as though it pins the contract, when it pins a stricter deployment policy on top of it. Half a sentence in the existing comment would stop the next author reading a policy failure as a contract violation. Non-blocking.

Documented search: I also checked that the two mount assertions in this spec are untouched (they are — AC 2's "both mounts still pinned" holds), that no other spec or fixture references neo-shared/neo as a tenant entry, and that the removal does not orphan neo's coverage — kbSync ingests it through the source extractors and, per #16566's own measurement, already accounts for the entire collection.

Rhetorical-Drift Audit (per guide §7.4):

• "verified against each remote" — independently confirmed for all three, including visibility, default branch and file counts.
• "no clone cost… ~7 files total" — accurate: 4 + 3 for the new pair.
• The accepted coverage regression is stated in both the config comment and the spec comment, rather than only where the author was working.
• "sequencing, not a retreat from the goal" — matches operator direction; the entry is removed pending per-tenant source declaration, not abandoned.

Findings: Pass. No drift.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The vacuity trap is the transferable item, and its shape is specific: the old assertion was a proxy ("neo says dev, create-app says main, so they differ") for the invariant ("each entry declares its own"). Deleting the entity a proxy references does not make the proxy fail — undefined !== 'main' is still true — so the test goes green and meaningless rather than red. A proxy assertion degrades silently when its subject is removed; the invariant it stood for does not. The general guard: when a change removes an entity, grep the specs for assertions that mention it, not only ones that would break.
• [KB_GAP]: None. [TOOLING_GAP]: None new.

🎯 Close-Target Audit

• Findings: Pass. Resolves #16592, a leaf (bug,ai,architecture, no epic label). Its two pre-merge ACs read against the diff: neo absent with both devindex repos present at branchRef: main / credentialRef: none ✅ (and verified against the remotes, not just the yaml); contract spec passes with the new roster and both mount assertions still pinned ✅. The four post-merge ACs are correctly marked as such — and three of them are the ones the merge-order item above protects.

🧪 Test-Evidence & Location Audit

• Execution evidence: CI incomplete at 7b815143b3 — unit still pending, everything else green. I will confirm before any approval.
• Author receipt: 234 passed, and the contract spec RED-proven at 3 failed / 4 passed with only the config reverted — which is what makes it track the bytes rather than restate them.
• Reviewer falsifier: the three remotes queried directly for existence, visibility, default branch and tracked-file count. All four claims hold per repo.
• Test location: unchanged; the spec stays beside the config it guards.

Findings: Pass on content; CI must be green before approval.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: a deployment config roster and its contract spec — no consumed API surface, no OpenAPI surface, no skill or convention change.

📋 Required Actions

To proceed with merging, please address the following:

• State the dependency on #16590 in the body — this must not merge first. Without the scoped stale-id gathering, kbSync deletes all three tenant corpora every interval, and ACs 3/4/5 fail in a way that reads as a defect in this roster.

Non-blocking: half a sentence in the branchRef test noting it enforces a stricter deployment policy than tenantRepoAccessContract's "only present when configured", so a future legal omission is not misread as a contract break.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — the fix is at the layer that owns the problem: an identity collision created by configuration is removed in configuration, rather than worked around in code. The comment records why neo returns later, so the removal cannot be mistaken for a scope retreat. 4 deducted because the roster now has no entry exercising non-default-branch resolution, which is disclosed but still a real hole in what the config can prove.
• [CONTENT_COMPLETENESS]: 88 — the config comment explains the collision mechanism, the file-count rationale, and the return condition; the accepted regression is stated in two places rather than one. 12 deducted for the unstated merge-order dependency, which is the one thing a reader acting on this PR most needs.
• [EXECUTION_QUALITY]: 96 — the vacuity trap was caught and repaired with the invariant rather than an updated proxy, the raw-yaml read closes the normalizer escape, and the RED proof pins the bytes. 4 deducted for the spec/contract strictness ambiguity above.
• [PRODUCTIVITY]: 96 — both pre-merge ACs delivered and independently verified; the four post-merge ACs are marked rather than claimed.
• [IMPACT]: 88 — this removes the collision that #16590's scoping explicitly cannot separate, so together they close the residual completely rather than narrowing it. It also gives the lane its first multi-repo witness, which per-repo scheduling, jitter and backoff have never had.
• [COMPLEXITY]: 30 — a roster edit and a spec rewrite; the reader load is in knowing why neo's absence is correct, which the comment supplies.
• [EFFORT_PROFILE]: Quick Win — small diff, removes a live recurring data-loss vector, and answers an epic-level open question.

One line, and CI. Ping me and I will turn cycle 2 immediately.

🖖 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