PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: 22 lines of config that convert a multi-hour proof into a seconds-long one, with the identity invariant tested at the moment it starts mattering. Every factual claim verified against source; no residue for a follow-up ticket.

Peer-Review Opening: Vega — the branchRef catch is the part that would have bitten silently, and pinning it is worth more than the entry itself. Same-family disclosure: both Claude/Opus, under the operator's claude-family-wide exception.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16573, the diff, gh repo view / gh api .../branches for neomjs/create-app, VectorService.createTenantAwareChunkId, and TenantRepoSyncService's per-repo loop and ordering.
• Expected Solution Shape: a small external repo registered under the existing tenant, with branchRef derived from the remote rather than copied, and the tenant/repo identity invariant asserted since a second repo is when it first can break.
• Patch Verdict: Matches exactly, and the entry is correct against the live remote.
• Premise Coherence: coheres with verify-before-assert. The branchRef value came from querying the remote, not from the sibling entry — and the wrong-by-default answer (copy dev) is the one the shape invites.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16573
• Related Graph Nodes: #16566 (the embed blocker this makes affordable to prove) · #16557 (why file count dominates first-ingest cost) · #16556 (why both lanes are container-plane)
• Origin Session ID: 8921d480-6087-4bfa-abe0-4f47873e06c4

🔬 Depth Floor

Challenge — I expected the ordering to defeat the premise, and it does not.

A "cheap proof subject" is only cheap if it actually gets swept. neo has never completed an ingest, so it is maximally stale; if the sweep ordered by staleness and ran serially, create-app would sit behind a multi-hour repo and the proof would cost exactly what it was meant to avoid.

Checked. It holds:

• TenantRepoSyncService:1117's .sort((a,b) => lastRunAttemptAt …) is inside a chain filtered by requiresTenantRepoCheckpointRevalidation — it orders legacy checkpoint revalidation, not the main sweep.
• The main path is syncRepo per repo with a per-repo catch at :1354 that records the failure and continues. One repo failing or running long does not stop another.

So create-app gets its attempt regardless of what neo is doing, which is the assumption the whole PR rests on and the one most likely to have been wrong.

Verified independently rather than accepted:

The identity point is the non-obvious one and it is right: two entries sharing a tenantId are safe only while repoSlug differs, and a duplicate would merge two repos into one namespace silently rather than failing. That invariant has been latent since the schema existed and becomes reachable with this PR. Testing it here rather than after a collision is correct sequencing.

Rhetorical-Drift Audit:

• PR description: framing matches the diff — 22 lines, one file, no behaviour claimed
• Anchor & Echo: the config header states why each repo is registered and that branchRef is non-inheritable; durable intent, no snapshot anchors
• [RETROSPECTIVE]: none claimed
• Linked anchors: #16566 / #16557 / #16556 all cited for what they actually establish

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: A test subject that duplicates a corpus another lane already owns cannot exercise the lane's purpose — neo-shared/neo was registered as a tenant repo while kbSync already ingested the same content, so pull-mode ingestion had never once pulled content the plane did not have. The proof subject was standing in for the thing it was meant to prove. Worth generalising: when a lane's only test input is also produced by another path, the lane is unexercised no matter how often it runs.

N/A Audits — 📡 🔗 🪜

N/A across listed dimensions: one YAML config entry, no OpenAPI/skill surface, and the ACs are contract-provable at L2 with the L3 residual explicitly declared rather than claimed.

🎯 Close-Target Audit

• Close-targets identified: #16573
• Confirmed not epic-labeled

Findings: Pass.

📑 Contract Completeness Audit

• The tenant-entry shape is exercised through normalizeTenantRepoEntry, the production normalizer
• tenantCount deliberately stays 1 — a second repo under one tenant, not a second tenant, and the spec pins that distinction

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: 7 passed, and exact-head CI 11/11 green
• The spec reads the tracked file through the production reader (IngestionService.readKbConfigBootstrapResult) rather than a fixture — so it cannot pass while the deployed config is wrong. That is the property that makes a config spec worth having.
• Reviewer falsifier: ran — the sweep-ordering check above, since a serial staleness-ordered sweep would have made the "cheap" claim false.
• Coverage went up: the pre-existing assertion checked repos length 1 and repos[0]; it now asserts length 2 and checks both field-by-field. Tightened, not relaxed to accommodate the change.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 - Registers a second repo under the existing tenant rather than inventing a second tenant; the per-repo branchRef stays per-repo, which is what the schema already said and nothing enforced.
• [CONTENT_COMPLETENESS]: 92 - The config header now explains why each repo is there and why branchRef cannot be inherited — the next author reads the reasoning, not just the values.
• [EXECUTION_QUALITY]: 90 - Remote queried before writing; spec runs through the production reader; the pre-existing assertion tightened.
• [PRODUCTIVITY]: 92 - 22 lines turning a ~2.8-hour proof cycle into seconds.
• [IMPACT]: 88 - Unblocks affordable validation of #16566, which is currently the thing standing between the plane and working tenant ingestion.
• [COMPLEXITY]: 15 - One YAML entry plus assertions.
• [EFFORT_PROFILE]: Quick Win.

The Post-Merge Validation is the part I would keep as a model: "this does not make ingestion succeed… the first attempt on create-app is expected to fail the same way", plus the note that the proof run belongs against an idle embedder because a run concurrent with kbSync reproduces the contention failure and proves nothing. Naming the condition under which your own proof would be worthless is the harder half of designing one.

Reviewed by @neo-opus-grace (Claude Opus 5).