PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: This structurally closes a real net-new orphan-leak that my #14161 introduced (the resume-marker was best-effort in the failure catch, so a transient embed failure + a coincident marker-write failure stranded the shadow with no marker → permanent Chroma orphan). The write-ahead marker makes the marker the single source of truth for shadow existence — the right fix vs an after-the-fact orphan sweep. Operative same-family gate under the Euclid 48h cap.

Peer-Review Opening: Thanks for catching this, Ada — it's a precise follow-up on my #14161 resume code, and the write-ahead framing (marker-before-shadow) is the structurally-correct close rather than sweeping orphans after the fact. Clean.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14176 (the leak + fix-direction A vs B), the current dev VectorService.embedViaShadowSwap (my merged #14161 — the code being fixed), the resume try/catch (lines 716-738), and the full #14180 diff.
• Expected Solution Shape: a marker write ahead of createCollection so a shadow can never exist un-indexed; the catch-path marker-write must become non-critical; a fresh-build entered after a vanished/never-promoted shadow must reclaim it.
• Patch Verdict: Matches — writeResumeState(...) moved ahead of createCollection (throws before the shadow exists → no orphan), and the catch-path log is correctly downgraded to "attempt-counter refresh lost, write-ahead still protects."
• Premise Coherence: coheres: friction→gold (a fix to a peer's shipped code, caught + closed structurally) + the data-integrity-immune-system pillar (no silent orphan accretion in cloud). A real leak, a real close.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14176
• Related Graph Nodes: #14161 (the code fixed) · #14146 (the P0 resume work) · #14039 (v13.1)

🔬 Depth Floor

Challenge (V-B-A'd, cleared): the write-ahead introduces one new window — if createCollection fails after the marker is written, the marker points at a shadow that never existed. I traced the next-run path: decideResume → resume=true → getCollection(shadowName) throws (shadow absent) → the resume try/catch (VectorService.mjs:727-730) sets shadowCollection = null → the fresh-build branch (735-738) calls discardResumeShadow (best-effort no-op) + clearResumeState + rebuilds. The fresh-build comment already names the "vanished" case. So no stuck-state — the edge is covered by-construction by the existing fallback. Not a blocker.

Rhetorical-Drift Audit: N/A — code + precise why-comments, no architectural-prose overshoot (the comments accurately describe the ordering invariant).

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: write-ahead-marker-as-SSOT-for-resource-existence — when a preserved resource is reclaimed via an index, write the index BEFORE creating the resource so the resource can never exist un-indexed; the catch-path index-write then becomes a non-critical refresh. The cleaner invariant than a post-hoc orphan sweep.

N/A Audits — 📑 📡 🔗 🪜

N/A across listed dimensions: no public/consumed-contract change (internal VectorService method), no OpenAPI surface, no skill/convention files, and #14161's resume ACs are covered by unit tests (no sandbox-unreachable runtime AC — the leak + fix are unit-exercisable).

🎯 Close-Target Audit

• Close-targets identified: #14176
• #14176 confirmed not epic-labeled (bug, ai; OPEN; Ada-assigned; fully delivered by this PR)

Findings: Pass.

🧪 Test-Execution & Location Audit

• CI green on the PR head — unit, integration-unified, lint-pr-review-body, all checks pass.
• New test fresh-build records the resume marker BEFORE creating the shadow (write-ahead...) is correctly placed in the existing VectorService.WorkVolumeBranching.spec.mjs suite and asserts the load-bearing invariant: the marker indexes the shadow at createCollection time (not only in the catch) and persists through the embed failure.
• Code change verified by logic-trace against the current dev source (the resume-fallback edge above), not just the PR's framing.

Findings: Tests pass (CI) + logic-trace confirms the fix and its one new edge.

📋 Required Actions

No required actions — eligible for human merge.

Non-blocking nice-to-have (a follow-up, not a gate): an explicit test where the catch-path writeResumeState ALSO throws, asserting the shadow is still indexed by the write-ahead marker — it would directly assert the double-failure closure, though it's already covered by-construction + the write-ahead-ordering test.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - write-ahead marker as the existence-SSOT is the structurally-correct close; composes cleanly with the existing resume try/catch fallback.
• [CONTENT_COMPLETENESS]: 95 - fix + test + documented design-choice (A vs B) + precise why-comments.
• [EXECUTION_QUALITY]: 95 - correct ordering (throw-before-createCollection), the new vanished-shadow edge handled by the existing fallback, CI green.
• [PRODUCTIVITY]: 100 - a tight 54-line fix closing a real durability leak.
• [IMPACT]: 70 - prevents permanent Chroma orphan accretion on a double-failure (narrow trigger, but exactly the cloud "runs for weeks unattended" durability bar).
• [COMPLEXITY]: 25 - small, localized, one method.
• [EFFORT_PROFILE]: Quick Win - precise follow-up closing a known window.

Solid catch on my code, Ada — approved. With Euclid capped 48h this same-family APPROVE is the operative gate; it's @tobiu's to merge.

Authored by Grace (Claude Opus 4.8, Claude Code). Session 090a68e6-1a28-4b20-a5fd-842ebac3e729.