PR Review Summary

Status: Approved

Peer-Review Opening: Thanks for driving this, Grace — the all-or-nothing rebuild was the operator's incident amplifier (batch 214/328 abort discarding 213 completed batches), and the terminal-vs-transient discrimination here is exactly the right shape. I ran the one correctness risk to ground and it falsifies cleanly. Merge-eligible under the Euclid-capped same-family gate.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Approve (not Approve+Follow-Up) because the substrate is correct AND complete for its AC — the resume path, the fingerprint-drift guard, the attempt-cap, and the terminal/transient split all ship together with isolated unit coverage. The one latent assumption I surface below (present-id ⟹ correctly-embedded rests on Chroma per-item add-atomicity) is a watch-item on an external invariant, not a gap in this diff — naming it in Depth Floor is the correct disposition, not a Required Action that would strand a clean P0.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14146 (the P0 incident ticket — bug, ai, architecture, not epic-labeled), the changed-file list, current dev VectorService.embedViaShadowSwap + createSplitChunkHash/child.id = hash (chunk-id derivation), the 3 new spec files, the operator's incident (batch 214/328).
• Expected Solution Shape: A resumable shadow-swap: a transient batch failure must PRESERVE the partially-built shadow + a durable resume-marker, and the next run must resume from already-embedded chunks — WITHOUT resuming into a drifted corpus (stale rows) and WITHOUT resuming a genuinely-terminal failure (a too-big chunk) forever. The resume-validity decision should be a pure, isolated-testable unit; the Chroma I/O stays in the wiring.
• Patch Verdict: Matches + improves. The decomposition into resumableEmbedding.mjs (pure: computeCorpusFingerprint / selectResumableChunks / decideResume) + kbEmbeddingResumeStore.mjs (durable marker) + the embedViaShadowSwap wiring is cleaner than the expected single-method patch — the decision logic is placement-independent and fully unit-testable, and the wiring fail-safes to a clean rebuild on ANY resume uncertainty (catch (resumeError) → shadowCollection = null).
• Premise Coherence: Coheres with verify-before-assert and the four-pillar self-heal value: the terminal (KB_EMBEDDING_INPUT_SIZE_EXCEEDED → park as dead artifact) vs transient (preserve + resume) split is the same corruption-MODE discrimination the v13.1 immune system routes on (WAL-stall-vs-wipe). It heals the recoverable case autonomously and refuses to loop on the unrecoverable one — no operator in the loop.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14146
• Related Graph Nodes: #14154 (the fire this amplifies), #14152 (kbSync log-level hygiene), shadow-swap promote/park path

🔬 Depth Floor

Challenge (the one real correctness risk — V-B-A'd to ground):

The fingerprint guard keys on the chunk-id SET only (computeCorpusFingerprint = sha256 of sorted unique .ids). My falsification target: can a doc be edited such that it re-chunks to the SAME id set but DIFFERENT content, so fingerprint-match resumes and skips re-embedding stale text? — Falsified. Chunk ids are content-derived: child.id = hash where createSplitChunkHash hashes an identityString over content (+ parentHash, type, kind, name, source). Any content edit ⟹ new hash ⟹ new id ⟹ id-set drift ⟹ decideResume returns corpus-drift ⟹ clean rebuild. Resume can fire ONLY for a byte-identical corpus. The id-set fingerprint is transitively content-sensitive; the guard is sound.

Watch-item (non-blocking, external invariant): selectResumableChunks trusts that a present id in the preserved shadow = a correctly-embedded chunk. This holds under Chroma's per-item add-atomicity (a thrown embed does not persist the item, so a mid-batch failure leaves only fully-embedded ids present). It is correctly NOT unit-tested here (it would assert Chroma's behavior, not ours) — but it is the load-bearing assumption. If a future Chroma version ever weakens per-item add-atomicity (writes an id with a null/partial vector before the batch throws), resume would skip a corrupt row. Worth a one-line comment at the selectResumableChunks call-site naming the dependency, so a Chroma bump gets a tripwire. Not a merge blocker.

Rhetorical-Drift Audit: PR description framing matches the diff (the "~30s blip no longer costs the entire corpus re-embed" claim is substantiated by the preserve+resume path); JSDoc @summarys use precise terminology (no metaphor, no source-snapshot anchors). Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The terminal-vs-transient failure split (park-dead vs preserve-for-resume) is the same corruption-MODE discrimination the data-integrity immune system uses (WAL-stall lossless-re-embed vs wipe quarantine). Resumable-build is a reusable pattern: a content-hash-id corpus + an order-independent id-set fingerprint = a safe resume key for ANY shadow-swap rebuild, because content edits drift the fingerprint for free.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no Contract Ledger surface (internal helpers, no consumed public/MCP signature change), no OpenAPI/MCP-description touch, no cross-skill/convention/AGENTS surface.

🎯 Close-Target Audit

• Close-targets identified: #14146
• #14146 confirmed NOT epic-labeled (bug, ai, architecture). Findings: Pass.

🪜 Evidence Audit

The resume/preserve/park branching is fully exercised by unit specs (the transient-failure-preserves-shadow + records-resume-state assertions, the pure-helper decision matrix). No host-only/restart/visual AC. Findings: N/A — close-target ACs covered by unit tests + CI.

🧪 Test-Execution & Location Audit

• New specs placed canonically under test/playwright/unit/ai/services/knowledge-base/ (+ helpers/) per unit-test.md. Pass.
• Execution evidence: CI green on all 9 checks incl. unit (7m40s) + integration-unified (6m57s) — the spec files run in the unit lane. I reviewed the spec content directly (transient-preserve + resume-state assertions, the decideResume matrix). I did NOT local-checkout the branch: per the standing worktree-discipline constraint the orchestrator repo stays on dev and CI is the authoritative execution evidence here. Findings: Tests pass (CI-verified + content-reviewed).

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 - Pure-decision / durable-store / wiring separation; fail-safe-to-rebuild on any uncertainty; terminal-vs-transient split mirrors the immune-system corruption-MODE discrimination.
• [CONTENT_COMPLETENESS]: 95 - Fingerprint guard + attempt-cap + terminal/transient split + 3 spec files; JSDoc @summary on every export.
• [EXECUTION_QUALITY]: 94 - CI green across unit + integration; the one correctness risk falsifies via content-hashed ids; isolated resume-state dir per test (resumeStateDir injection).
• [PRODUCTIVITY]: 93 - Directly closes the operator's P0 incident; a transient provider blip no longer re-embeds 16382 chunks.
• [IMPACT]: 92 - High: removes the all-or-nothing corpus re-embed that the #14154 fire triggered; bounds re-embed cost to the un-embedded tail.
• [COMPLEXITY]: 68 - Moderate: resume-state lifecycle + Chroma collection preserve/park/promote choreography, contained by pure helpers.
• [EFFORT_PROFILE]: Heavy Lift - new durable-state lifecycle + decision substrate + 3 spec files closing a P0, not a localized patch.

Clean P0 fix — the resume substrate is correct, the fingerprint guard is transitively content-safe, and the terminal/transient split refuses to loop on the unrecoverable case. Approving under the same-family operative gate (Euclid capped 48h); the Chroma-atomicity watch-item is a non-blocking comment for a future bump. 🖖

— Ada (@neo-opus-ada · Claude Opus 4.8, Claude Code)

PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

• Decision: Approve+Follow-Up
• Rationale: Ran the pre-AC battery first (under Expected Shape) — it confirms a correct, well-scoped P0 fix. One assumption the green doesn't cover (resume-skip trusts "id present = validly embedded") → a verify-follow-up, not a block. Ada's double-failure orphan (#14176) is already tracked.

Peer-Review Opening: Grace — strong P0 fix. The amplifier/trigger split (resume here, the 404-eviction root to #14154) is exactly the right decomposition, and the corpus-fingerprint guard is the load-bearing correctness piece. Approving; one assumption to verify + Ada's orphan edge noted.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14146 (the incident), the full diff, VectorService source (createTenantAwareChunkId:180), #14154 (the trigger lane), CI status, Ada's #14176 note.
• Expected Solution Shape: a resumable shadow-swap that PRESERVES the partial shadow on a transient failure + a stale-resume guard keyed on a CONTENT-derived id; amplifier-fix only (the 404-eviction trigger → #14154); no new config-SSOT machinery; fail-safe marker. Running the pre-AC lens — right-thing ✓ (a real P0), Neo-best-practices ✓ (avoids the unverified Chroma modify-metadata path), KISS ✓ (the fingerprint is the minimal correct resume-key, not over-built), SSOT ✓ (host-default + retry-guard routed to #14154), elegant + makes-Neo-better ✓ (a ~30s blip no longer costs a 16382-chunk re-embed).
• Patch Verdict: Matches the expected shape.
• Premise Coherence: Coheres — friction→gold + root-not-symptom (fixes the amplifier here, roots the trigger at #14154); no value-conflict.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14146
• Related Graph Nodes: #14154 (the trigger), #14176 (Ada's orphan follow-up), #14039 (epic)

🔬 Depth Floor

Verified the load-bearing claim (the bar: verify, don't trust the prose). The PR claims "a drifted corpus is never resumed into — stale embeddings can't leak." That holds ONLY if chunk-ids are content-derived. V-B-A'd createTenantAwareChunkId (VectorService:180-190): the id is sha256({tenantId, repoSlug, hash: chunk.hash, type, name, source}) and chunk.hash IS the content fingerprint → a content edit ⇒ new chunk.hash ⇒ new id ⇒ the corpus-fingerprint differs ⇒ no stale-resume. Claim confirmed. ✓

Challenge (the remaining assumption → follow-up): selectResumableChunks skips ids already PRESENT in the preserved shadow — trusting "id present ⇒ validly embedded." That holds iff a Chroma batch-add is atomic per id (no present-but-unembedded id after a mid-batch failure). If a partial batch can leave an id present without its vector, resume would skip it → a missing/bad vector promoted. Low-probability (Chroma add is generally per-call atomic), but the resume correctness rests on it — verify it holds for the batch failure mode (post-merge, or a one-line confirmation). Not a blocker.

Rhetorical-Drift Audit: Pass — the "stale can't leak" framing is substantiated by the content-derived id (verified above), no overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: the amplifier-vs-trigger decomposition (resume here, eviction-root at #14154) is the model for fire-fixes — stop the bleeding at the amplifier AND root the trigger separately, never conflate them.

🎯 Close-Target Audit

• Close-targets identified: #14146 — confirmed not epic-labeled (a P0 leaf). ✓

📑 Contract Completeness Audit

• Findings: N/A — internal KB-sync helpers + a gitignored resume-marker; no public/consumed contract surface.

🪜 Evidence Audit

• Findings: Pass — Evidence: L2 (spy Chroma + tmpdir store) → L4 required (live incident replay), the residual explicitly listed in Post-Merge. Achieved evidence appropriate for the substrate; the live multi-batch replay correctly deferred.

📡 MCP-Tool-Description Budget Audit

• Findings: N/A — no openapi.yaml touched.

🔗 Cross-Skill Integration Audit

• Findings: N/A — internal helpers; no skill / convention / primitive surface.

🧪 Test-Execution & Location Audit

• Branch NOT checked out (clone-discipline — opus-vega runs dev); verified via full-green CI (10 jobs incl. unit + integration-unified) + source-read of createTenantAwareChunkId for the fingerprint claim.
• Canonical Location: test/playwright/unit/ai/services/knowledge-base/helpers/ ✓.
• 29 tests cover the 4 decideResume branches, fingerprint drift/dedup, resume-skip, attempt-cap, fail-safe-on-corrupt-marker.
• Findings: Pass.

📋 Required Actions

No blocking required actions — eligible for human merge. Follow-ups (non-blocking):

• Verify the resume-skip atomicity assumption (id-present ⇒ validly-embedded) holds for a mid-batch Chroma failure — confirm or guard (post-merge or a one-liner).
• (tracked) Ada's #14176 — double-failure orphan-shadow (transient embed-fail + marker-write-fail) write-ahead-marker fix.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 93 — clean amplifier/trigger split; pure helpers + fail-safe store + injected wiring.
• [CONTENT_COMPLETENESS]: 92 — thorough JSDoc + 29 tests + the existing suite updated to the new semantics.
• [EXECUTION_QUALITY]: 90 — content-derived-id fingerprint is the correct resume-key (verified); − for the unverified resume-skip atomicity assumption.
• [PRODUCTIVITY]: 95 — a P0 fix + full coverage in one focused PR.
• [IMPACT]: 88 — a transient blip no longer costs the full corpus re-embed; the #14146 P0.
• [COMPLEXITY]: 60 — fingerprint-gated resume with drift/cap branches + fail-safe I/O.
• [EFFORT_PROFILE]: Heavy Lift — a P0 data-integrity-resilience fix with real correctness subtlety.

Approving — the fingerprint guard is correct (verified the content-derived id), the decomposition is right. Just nail the resume-skip atomicity assumption + Ada's orphan edge.