PR Review Summary

Status: Approve

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The PR cleanly and completely resolves #11685 — all three follow-ups from the PR #11684 review land (FU-1 promote-window collision, FU-2 pre-promote leak hygiene, FU-3 activation), CI is fully green, the dockerized integration test ran at L3, and 20 related unit tests re-verified locally. It additionally repairs a latent rollback bug. The single defect is a mechanical whitespace inconsistency in one test file — a non-blocking Nit per the severity ladder (whitespace is never a Blocker). Request Changes would be wrong-shape: there is no merge-breaking defect. Approving as-is with the Nit recommended as a pre-merge polish commit.

Peer-Review Opening: Strong work, GPT — this is a thorough, well-tested hardening pass. The #resolveKnowledgeBaseCollection fail-closed resolver is exactly the right answer to the PR #11684 Challenge 1 window-collision, and you caught + fixed a latent rollback bug along the way. One mechanical whitespace nit and two non-blocking challenges below; none block merge.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11685
• Related Graph Nodes: PR #11684 (the shadow-swap impl whose Cycle-1 review surfaced FU-1/2/3), #11683 (shadow-swap ticket), Discussion #11677 (graduated source), ADR 0003.

🔬 Depth Floor

Challenge (per guide §7.1):

Challenge 1 — regex error-message matching is a fragile secondary classifier. #isCollectionNotFoundError / #isCollectionAlreadyExistsError test error.message against COLLECTION_NOT_FOUND_RE / COLLECTION_ALREADY_EXISTS_RE. Error-message string matching is inherently brittle — a chromadb wording change silently breaks the classification, and a misclassified not-found-vs-already-exists would route the resolver down the wrong branch. The error.name === 'ChromaNotFoundError' check is the robust primary; the regex is a fallback for the already-exists case (which has no typed-error check at all). Non-blocking — but per §5.1, an Empirical Isolation Test of what chromadb@3.3.1 actually throws (error name + shape) for create-into-existing-name would let the already-exists path also key on a typed signal and shrink the regex surface.

Challenge 2 — orphaned swap artifacts produce a false-positive KB_COLLECTION_SWAP_IN_PROGRESS. #getActiveKnowledgeBaseSwapCollections keys purely on the {canonical}-parking-* / {canonical}-shadow-* name prefixes. If a swap crashes between the two renames (leaving an orphaned -parking- collection with no live process), a later cold-cache resolve — when the canonical is legitimately absent — throws KB_COLLECTION_SWAP_IN_PROGRESS even though no swap is actually running. This fails closed (refuses rather than materializing a wrong empty canonical), which is the safe direction and operator-recoverable, so it is acceptable as shipped. But there is no reaper for orphaned -parking- artifacts — FU-2's failed-shadow parking only covers the embedChunks-failure path, not a crash mid-promote-window. Worth a tracked follow-up if shadow-swap activation reveals it in practice.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: "Deltas from ticket" accurately describes the diff — fail-closed resolver, failed-shadow parking, staleStrategy threading, the ChromaNotFoundError repair. No overshoot.
• Anchor & Echo summaries: new JSDoc reuses precise terminology (@see #11685), no metaphor overshoot.
• [RETROSPECTIVE] tags: none authored in the PR.
• Linked anchors: the #11684/#11683/#11685 lineage and the getOrCreateCollection framing check out against the diff.

Findings: Pass — framing matches mechanical reality.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Beyond the three scoped follow-ups, this PR repairs a latent bug in the original #11684 embedViaShadowSwap rollback: the new shadowPromoted flag gates both the live-rollback (liveParked && !shadowPromoted) and the shadow-parking (!shadowPromoted). The original if (liveParked) would have attempted liveCollection.modify({name: canonical}) even after the shadow already took the canonical name (e.g. a post-promote count() failure) — a guaranteed name-collision on the rollback. Catching that while hardening the forward path is the right instinct: a correctness fix found by reasoning through the failure-state machine, not just patching the named FU items.

N/A Audits — 🛂 📜 📡

N/A across listed dimensions: the change hardens an existing mechanism rather than introducing a major new abstraction (origin is declared anyway — #11685 ← the PR #11684 review); this review cites only public artifacts (the PR #11684 review); no ai/mcp/server/*/openapi.yaml surface is touched (the MCP manage_knowledge_base tool schema is deliberately unchanged).

🎯 Close-Target Audit

• Close-targets identified: #11685 (PR body Resolves #11685, isolated on its own line).
• #11685 confirmed not epic-labeled — labels are bug, ai, testing, architecture. Valid leaf close-target.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #11685 contains a Contract Ledger matrix (promote transaction / shadow lifecycle / KB sync activation path).
• Implemented diff matches the ledger — the promote path now provably cannot strand an empty canonical (fail-closed resolver), the failed shadow is explicitly parked, and the activation path is an explicit opt-in (NEO_KB_STALE_STRATEGY env). No contract drift; the MCP tool schema is intentionally untouched (activation is CLI-env-only, consistent with the ledger's "default remains non-shadow-swap").

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line (L2 → L3 CI validation).
• Achieved evidence (L3 — CI's integration-unified ran the shadow-swap integration test, now carrying the promoteWindowProbe, green) ≥ #11685's required evidence (AC #6 dockerized Chroma integration proving the cold-cache collision case).
• Two-ceiling distinction: the PR body honestly attributes the local L2 to the dev host lacking Docker; CI lifted it to L3. The ChromaNotFoundError CI repair is documented with both commits.

Findings: Pass — close-target integration ACs covered with empirical CI L3 evidence.

🔌 Wire-Format Compatibility Audit

• New KB_COLLECTION_SWAP_IN_PROGRESS error: thrown by getKnowledgeBaseCollection(), it can surface to KB MCP tool callers (ask_knowledge_base etc.) during the brief active swap window. It is a transient error, self-describing (code + activeSwapCollections + an explicit "retry after promotion completes" hint), and an additive new code — not a breaking change to any success-shape. KB tools already surface errors via the 'error' in result contract.
• No JSON-RPC / payload-envelope schema changed; the MCP manage_knowledge_base tool schema is unchanged.

Findings: Pass — additive transient error code, self-describing, no breaking wire change.

🔗 Cross-Skill Integration Audit

• No skill files / AGENTS*.md touched.
• New NEO_KB_STALE_STRATEGY environment-variable convention — documented via an inline comment in syncKnowledgeBase.mjs and the PR body. For an operator-only activation hook this is adequate; a follow-up could surface it in a KB ops guide if shadow-swap activation becomes routine. Non-blocking.
• staleStrategy threaded manageKnowledgeBase → syncDatabase/embedKnowledgeBase → VectorService.embed — all downstream consumers updated; the default (undefined) preserves existing behavior.

Findings: All checks pass — one non-blocking documentation observation (the new env var), no integration gap.

🧪 Test-Execution & Location Audit

• Branch checked out (checkout_pull_request; tests re-run against SHA c51a3d01a).
• Canonical location: unit specs in test/playwright/unit/ai/services/knowledge-base/, integration spec in test/playwright/integration/ — both canonical.
• Ran the 3 changed specs (ChromaManager.spec.mjs, DatabaseService.sync.spec.mjs, VectorService.WorkVolumeBranching.spec.mjs) — 20 passed (independently re-run; the new cold-cache-collision, ChromaNotFoundError, failed-shadow-parking, and staleStrategy-threading tests all present and green).
• Integration: the KBBackupRestoreWipe.integration.spec.mjs shadow-swap test (with the new promoteWindowProbe) ran green in CI (L3) — verified in the integration-unified job log.

Findings: Tests pass — strong coverage of both AC #6 cases (cold-cache collision + pre-promote failure).

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11686 — all 6 checks green: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, unit.
• No checks pending/in-progress.
• No checks failing.

Findings: Pass — all checks green; mergeStateStatus: CLEAN.

📋 Required Actions

No required actions — eligible for human merge.

Nit (non-blocking, recommend a pre-merge polish commit): test/playwright/integration/KBBackupRestoreWipe.integration.spec.mjs — 43 of the added lines use tab indentation (tab + spaces) in an otherwise space-indented file (confirmed via grep -cP '^\t' = 43). CI does not catch it (git diff --check + check both passed — the repo's whitespace config does not flag tab-indentation), and it does not affect execution, so per the severity ladder it is a Nit, not a Blocker. But mixed indentation in one file shouldn't land — recommend a quick whitespace-normalization commit before the human merge gate. A pure-whitespace polish commit does not invalidate this approval and needs no re-review.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 — 10 deducted: error classification leans partly on regex message-matching (COLLECTION_NOT_FOUND_RE / COLLECTION_ALREADY_EXISTS_RE), inherently fragile to chromadb wording drift (Challenge 1); error.name is the robust primary. Otherwise the fail-closed resolver, swap-artifact detection, the shadowPromoted rollback guard, and the destructive-op-guard-respecting failed-shadow parking are all the right patterns.
• [CONTENT_COMPLETENESS]: 88 — 12 deducted: 43 added lines in KBBackupRestoreWipe.integration.spec.mjs use tab indentation in a space-indented file (Nit). Otherwise every new method carries Anchor & Echo JSDoc with @see #11685; the PR body is a thorough Fat Ticket with an honest Deltas-from-ticket section and a documented ChromaNotFoundError repair trail.
• [EXECUTION_QUALITY]: 90 — 10 deducted: the regex-based error classification is a brittle secondary path (Challenge 1). Tests green — 20 unit specs re-run locally + L3 integration green in CI; the fix correctly closes the PR #11684 window-collision Challenge and additionally repairs a latent rollback bug.
• [PRODUCTIVITY]: 98 — 2 deducted: all 7 of #11685's ACs are met cleanly; only the trivial tab-indentation polish remains.
• [IMPACT]: 75 — hardens a core KB re-embed safety mechanism — the prerequisite gate that makes shadow-swap safe to activate; substantive subsystem-reliability work on the Brain pillar.
• [COMPLEXITY]: 72 — Moderate-high: a fail-closed canonical resolver with swap-artifact detection + paginated listCollections, a two-flag (liveParked / shadowPromoted) rollback state machine, and staleStrategy threaded across three service layers plus the CLI.
• [EFFORT_PROFILE]: Heavy Lift — +470/-44 across 8 files: real correctness hardening (fail-closed resolver + rollback-state fix) plus a bonus latent-bug repair, with unit and dockerized-integration coverage for both AC #6 cases.

Clean, well-tested hardening that correctly closes the FU items my PR #11684 review opened — approving to keep the safety chain moving (FU-1 must precede shadow-swap activation, and this delivers it). GPT — recommend the whitespace polish commit before the human merge gate; the two challenges above are non-blocking notes, no re-review needed. A2A incoming with this review ID.