PR Review Summary

Status: Approve (one substantive, non-blocking recommendation)

🪜 Strategic-Fit Decision

Approve. Resolves #14293 (the local-orchestrator KB-sync stability lane — disclosure: I authored that ticket, so this is the spec-author verifying the implementation). The shape honors the thin-orchestrator direction (lease inspection delegated to heavyMaintenanceLeasePrimitives) and ADR-0019 (retry policy as reactive KB-config leaves read at the use site). Cross-family reviewer note: with Ada+Vega out, this is the sole cross-family review / merge-basis — reviewed at exact head accordingly.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14293 body + ACs (self-authored), #14294 live state + CI, the full source diff (Orchestrator.mjs recycle path, ChromaManager.mjs resolve-retry, config.template.mjs leaves), and the config.mjs-overlay-vs-template generation architecture.
• Expected Solution Shape: (a) gate the chroma recycle (and the post-recycle defrag) on the heavy-maintenance lease so a running kbSync keeps chroma alive until the lease clears; (b) bounded retry on ChromaConnectionError while resolving the canonical collection, preserving the not-found / shadow-swap paths; ADR-0019-reactive, env-backed config.
• Patch Verdict: Matches. Recycle and defrag both defer on an active lease, with a fail-safe (defer if the lease inspection throws). The retry is bounded + exponential-backoff, ChromaConnectionError-only, and the outer resolve still routes not-found → swap-check/create. Config leaves are reactive + env-backed + read at the use site. One robustness gap (below), not a contract miss.
• Premise Coherence: Coherent — the patch realizes #14293's two-part design faithfully; the sole divergence is fail-loud-on-missing-config vs fail-safe-default, a robustness choice.

🕸️ Context & Graph Linking

• Target: Resolves #14293.
• Related: Orchestrator.poll chroma recycle/defrag, heavyMaintenanceLease, ChromaManager#resolveKnowledgeBaseCollection, #14273 (the lease-fairness epic that surfaced the regression).

🔬 Depth Floor

Verified correct: (1) recycle defers while the lease is active — and the lease-inspection failure path returns true (defer), the safe direction. (2) #isChromaConnectionError matches both error.name and error.constructor.name; retry never swallows a not-found (it re-throws non-connection errors, and the caller keeps the swap/create path). (3) backoff is capped by maxDelayMs and the cumulative maxTotalDelayMs.

Substantive finding (non-blocking recommendation): #getCollectionResolveRetryPolicy() throws when aiConfig.collectionResolveRetry leaves are missing/non-finite. A gitignored config.mjs overlay generated before this PR lacks those leaves → undefined → throw → #resolveKnowledgeBaseCollection re-throws (not a not-found) → KB resolve fails on every call, not just transient blips. That is strictly worse than pre-PR behavior for a stale-overlay clone, and CI can't catch it (CI regenerates the overlay via prepare). It is gated behind the PR's own rollout-note step 1 (refresh overlay + restart) and fails loud with a precise remediation message — so it's documented + recoverable, hence non-blocking. Recommendation: fall back to the leaf defaults (the template's 5 / 500 / 2000 / 5000) when the leaves are missing, instead of throwing — this removes the rollout-step dependency, matches the config silent-fall-through convention, and makes the resilience feature itself resilient. Small change; the operator's call whether to amend pre-merge or fast-follow.

Rhetorical-Drift Audit: PR body ↔ implementation consistent; ADR-0019 "aligned" claim holds (use-site reads, no re-derivation); Evidence ladder matches the tests.

🎯 Close-Target Audit

• Close-target #14293 identified; not epic-labeled.
• AC mapping: AC1 (recycle defers while lease held) ✅; AC2 (bounded ChromaConnectionError retry, not-found/swap unchanged) ✅; AC3 (no exit 1 from a single transient blip) ✅ — with the stale-overlay caveat above.

🪜 Evidence Audit

Evidence: L2 (lease-overlap + retry + config-template unit coverage) → L2 required by #14293. Aligned. 72 unit pass; CI fully green. Gap: no test exercises the missing-config (stale-overlay) policy path — adding one would lock in whichever behavior is chosen.

🧪 Test-Execution Audit

Did not check out locally (budget-conserving per operator capacity guidance); relied on green CI (72 passed: Orchestrator + ChromaManager + config.template specs) + a full source-diff read + the verified config-overlay architecture.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: Memory Core prior-art sweep degraded again (query_summaries collection probe timeout) — prior-art here is satisfied by my authorship of #14293.
• [RETROSPECTIVE]: the config.template.mjs import + queryScoreWeights realignment in this diff is the intentional boy-scout block-alignment paydown (whole-touched-file), not incidental churn.

N/A Audits — 📡 🔗 📑

No MCP tool-surface or skill/convention change; #14293 carries no Contract-Ledger row for these internal helpers (behavioral fix).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 — recycle joins the existing lease-fairness coordination; retry at the use site; ADR-0019-aligned; orchestrator stays thin.
• [CONTENT_COMPLETENESS]: 86 — strong body (Evidence ladder, ADR-0019 impact, rollout note); one missing test (stale-overlay policy path).
• [EXECUTION_QUALITY]: 82 — fail-safe recycle, correct error-classification, bounded backoff; the stale-overlay throw is the one robustness gap.
• [PRODUCTIVITY]: 88 — resolves the regression in one well-tested PR.
• [IMPACT]: 88 — removes a real local-orchestrator KB-sync break (recycle preempting a long sync).
• [COMPLEXITY]: 60 — moderate multi-file coordination + config + retry, not a rewrite.
• [EFFORT_PROFILE]: Maintenance — targeted regression fix with focused coverage.

📋 Required Actions

None blocking. One recommended hardening (fail-safe the retry-policy read on a missing/stale overlay) — take it as a quick amend or a fast-follow, your call.

Authored by Grace (Claude Opus 4.8, Claude Code).