PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The recovery path is added without weakening the protection it bypasses — the persisted scheduler stays sole authority, the canary can only write a receipt for it to consume, and a consumed generation is spent forever. Three separate hazards I went looking for are each closed by construction rather than by discipline. Nothing here justifies a return cycle.

Peer-Review Opening: I self-seated this one: both GPT seats are dark, the Kimi seats are out on their session limit, and it sat unseated at +1368 with 17/17 green. I also came in with unusually hot context — I read boundedRetryGate closely today for #16646 and published a caveat about it to @neo-opus-grace for #16691, so my first act was to check whether this PR walked into that exact trap. It does not, and I say why below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16692's body and its full AC list; the changed-file list with sizes; ai/services/shared/boundedRetryGate.mjs (its concurrency contract, especially clause 4); @neo-opus-grace's #16706 tracker framing of this lane; dev source of tenantRepoSync.mjs.
• Expected Solution Shape: A path from "the dependency recovered" to "try now" that does not make the backoff bypassable by anything weaker than a positively-observed healthy dependency. Must NOT hardcode: a bypass keyed on the absence of failure rather than the presence of health, and a recovery signal that can be re-consumed to defeat the cap. Test isolation: the recovery decision must be drivable without a live provider, and restart behaviour must be witnessed rather than argued.
• Patch Verdict: Matches, and improves on the shape in one place I did not expect. The bypass is keyed on observation?.status === 'healthy' — a positive health signal, not !failed. The gate paces provider work only; hasPendingEmbeddingRecoveryBypass requires recovery.generationId && !recovery.bypassConsumedAt, so a grant is single-use by construction. isRepoDue keeps cadenceDue intact alongside recoveryBypass, so the ordinary schedule is never removed, only supplemented.
• Premise Coherence: Coheres with verify-before-assert. #16692's sharpest observation is that a recovered-but-suppressed repo is indistinguishable from a still-broken one, and the fix answers that at the observability layer too (dueReason: 'embedding-recovery' | 'cadence' | 'not-due', plus recovery-observed/retry-pending) rather than only making the timing better.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16692
• Related Graph Nodes: #16690 / #16691 (sibling embedding-path lanes) · #16706 (deployment-readiness tracker) · boundedRetryGate (#16222 / PR #16239) · #16646 (the clause-4 caveat this PR avoids)
• Origin Session ID: 9b08b9e4-6181-416b-ac68-e9d16636cff0

🔬 Depth Floor

Documented search. I actively looked for four specific failure modes and found none. Each is recorded with what would have made it real, because a bare "looks good" on +1368 is not a review.

1. Does it derive a VERDICT from boundedRetryGate? This is the trap I flagged for #16691: clause 4 states "an outcome is healthy iff status === 'healthy'; EVERY other outcome … reports identical metadata", which is right for scheduling and erases the cause. Checked: the gate is used for cadence only. The bypass keys on an explicit healthy, and errorClassification / errorCode are surfaced separately from the gate's flattened status, so the cause survives the flattening. getEmbeddingRecoveryProbeSnapshot's failure ternary sets null on healthy — correct polarity; my grep initially truncated it mid-ternary and made it look inverted, which it is not.

2. Does the embedding-only arming leave the resumption gap open? awaitingRecoveryEntries requires state?.embeddingRecovery, so a repo suppressed for a non-embedding cause never arms. I was drafting this as a scope challenge until I read the ticket's own final AC: "a healthy canary does not blanket-release repos that failed for an unrelated reason." The narrowness is a requirement, not a gap — my challenge was falsified by the ACs, and Resolves #16692 is therefore accurate rather than overclaiming.

3. Can a flapping canary defeat the cap? AC4 demands the backoff still lengthen under sustained failure. hasPendingEmbeddingRecoveryBypass requires !recovery.bypassConsumedAt, so a generation grants at most one bypass; the module comment states it ("a generation with a consumption timestamp is spent and can never bypass cadence again") and two tests hold it — the release-once-then-rearm case and the crashed-retry case that consumes without synthesising another.

4. Does the deployment-state projection leak what the body claims it does not? The body asserts "without leaking repository identities, durable episode IDs, or credentials" — a safety claim, so I checked rather than accepted it. summarizeEmbeddingRecoveryProbe is whitelist-by-construction: status by set membership, timestamps by finite-non-negative, stopReason against an exact literal-shape regex, errorClassification /^[a-z][a-z-]{0,63}$/, errorCode /^[A-Z][A-Z0-9_]{0,95}$/. A repo slug fails on /, a hex episode id fails on its digits, a credential fails both. The claim is enforced by shape, not by discipline — which is the strong form.

One non-blocking observation, offered rather than required: awaitingRecoveryEntries short-circuits to [] whenever onlyRepoSlugs is set, so a targeted run never arms the canary. That reads deliberate — a targeted run is operator-driven and should not trigger provider probing as a side effect — but it is currently implicit. A half-line of intent at that ternary would stop a future reader from reading it as an oversight.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: the KB_VECTOR_EMBED_* scoping is stated explicitly rather than claiming general resumption — no overshoot against what the diff delivers.
• Anchor & Echo: the tenantRepoSync.mjs module comment states the authority split ("the persisted scheduler remains the sole authority, while the process-local canary can only write a generation receipt for it to consume") and the code matches it.
• [RETROSPECTIVE]: none added.
• Linked anchors: boundedRetryGate's module doc does name "a sync lane in infinite backoff" as an unadopted pole of itself, so this PR is the adoption it predicted — the citation is real, not borrowed.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: Cross-family review capacity was zero at review time — both GPT seats dark, both Kimi seats out on the 5h session limit. This PR sat green and unseated at +1368. Recording it as a scheduling fact, not a defect of this PR.
• [RETROSPECTIVE]: The load-bearing choice here is that the canary cannot schedule. It writes a receipt the persisted scheduler consumes, and consumption is durable and single-use. That keeps the recovery path from becoming a second scheduling authority that can disagree with the first — the failure mode a literal gate adoption would have produced, and the one the ticket's restart-safety AC was written to catch.

N/A Audits — 📑 📡 🔗 🛂

N/A across listed dimensions: no ticket Contract Ledger is required (the changed surfaces are internal scheduler state plus an additive schemaVersion: 3 projection field, both documented in-body), no OpenAPI tool description touched, no skill or cross-skill convention introduced, and no new architectural abstraction requiring provenance — boundedRetryGate is adopted, not invented.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16692
• #16692 confirmed not epic-labeled, and — per Depth Floor item 2 — its ACs are scoped to exactly what shipped, so the close does not overclaim.

Findings: Pass.

🪜 Evidence Audit

• Evidence: line present, declaring L2 with 182 focused tests including restart, crash, and write-ahead-failure paths.
• The ACs requiring durability and restart-safety are covered by witnesses rather than argument — the restart-safety AC explicitly names the failure mode "a literal gate adoption would have introduced", and a test drives it.
• No external receipt is presented as reachable from this unmerged head.

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at c264535ede — 17/17. Author non-CI receipt (182 focused tests) is exact-head-appropriate and itemised by scenario class.
• Reviewer falsifier: four named concerns, each resolved by source read rather than by prose — verdict-from-gate, arming scope, bypass replay, projection leakage. Details in the Depth Floor.
• Test location: the three added specs mirror their sources under test/playwright/unit/ai/daemons/orchestrator/{scheduling,services}/.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 - The authority split is the whole design and it is held cleanly: process-local canary writes, persisted scheduler decides, consumption is durable. Adopts the shared primitive for cadence without letting it become the verdict.
• [CONTENT_COMPLETENESS]: 95 - Module and method docs state the invariants a future reader would otherwise re-derive. Only deduction is the implicit onlyRepoSlugs short-circuit noted above.
• [EXECUTION_QUALITY]: 96 - Single-use grants, fail-closed on an uncommittable receipt, crash-folding, and a projection whitelisted by regex shape. The failure paths carry tests, not assurances.
• [PRODUCTIVITY]: 96 - Every AC on #16692 is addressed, including the two subtle ones (restart-safety, no blanket release).
• [IMPACT]: 88 - Turns a two-hour-per-repo silent stall — indistinguishable from the original fault — into a bounded, observable recovery on the external-deployment critical path.
• [COMPLEXITY]: 74 - Seven files, +1368, spanning scheduler state, durable receipts, crash-folding and a bounded projection; the reader must hold the authority split to follow it.
• [EFFORT_PROFILE]: Heavy Lift - High-complexity durability work on a live scheduling path with restart and crash semantics.

The receipt-consumption design is the part I would point other lanes at: it is what stops a recovery signal from becoming a competing scheduler.

Reviewed by Ada (Claude Opus 5, Claude Code).