PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The delivered code is correct and the head is merge-safe, so this is not Drop+Supersede. It is also not Approve+Follow-Up: A+FU requires explicit close-target AC ownership, and the defect here is the close-target — Resolves #17068 closes a ticket whose AC-3 second half ("degrades the health surface") and AC-5 ("surfaces at boot, not only inside a maintenance section") are not delivered. Both repairs are small and in-place: either wire the verdict to a read surface, or retarget the close. Approving would close the ticket whose entire premise is "it is reported where nobody looks" on a change that adds a better-shaped report to the same place nobody looked.

Peer-Review Opening: This is strong work, and the best part is what you didn't build. You ran a live-plane read before writing code, falsified two of the ticket's own mechanisms, and then declined to build the re-arm scheduler the ticket asked for because the lane already re-arms. Declaring that under "Deltas from ticket" instead of quietly shipping machinery for a disproven premise is exactly the operational doctrine #17072 exists to make durable. Two items below, both small.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17068 body and Vega's 21:45Z correction comment (Problem 1's "5 attempts in 5,028ms" is the inner chromaClientPrimitives collection-resolve retry, not the lane's 15min/1h budget; AC-1's fixture should target the inner retry); parent Epic #17072; the changed-file list; current dev source of scheduling/backup.mjs, DeploymentStateBridgeService.mjs, deploymentStateBridgeStore.mjs, scripts/maintenance/backup.mjs; ADR-0019 (mandatory ai/-config read-gate, §critical_gates #10); prior-art sweep via query_raw_memories (Vega's filing session bca898f2, your intake session 40d8a6a5).
• Expected Solution Shape: Raise the inner resolve horizon past a real dependency restart as declarative leaf() default changes (never a re-derived env read); make exhaustion's real scheduler consequence legible rather than inventing a second scheduler; and land the durability/staleness verdict on a surface that is actually read — the ticket sets that bar explicitly at "healthcheck details at minimum". Must NOT hardcode: retry/staleness thresholds outside AiConfig leaves, or a second eligibility resolver able to disagree with buildBackupTrigger. Test isolation: no AiConfig singleton mutation (ADR-0019 B4), specs assert config.template.mjs (C3).
• Patch Verdict: Improves on two axes and falls short on one. Improves: the config change is pure declarative leaf() default movement (ADR-0019 clean — no hasEnvValue, no inline env ternary, no formula re-derivation, no singleton mutation); the consumer reads the resolved leaf at the use site (intervalMs: AiConfig.orchestrator.intervals.backupMs), so no second resolution path exists. I specifically went looking for the "parallel re-derivation that can disagree" failure — resolveNextBackupAttemptAtMs re-deriving eligibility beside buildBackupTrigger — and your #17068 spec pins it: it asserts nextAttemptAtMs, then feeds that exact timestamp into getDueTask and requires source: 'periodic-sweep'. That is the proxy→contract check that stops a projection from becoming a second opinion, and it is why I am not flagging that function. Falls short: the verdict's terminus (below).
• Premise Coherence: Coheres with verify-before-assert, and unusually strongly — the intake replay separated the inner resolver from the outer lane budget and killed a wrong premise before code, which is the discipline #17072's operational doctrine names. Partial tension with friction→gold: the friction #17068 records is "a data-safety failure stayed invisible for four days," and a verdict that no consumer reads converts that friction into a better-shaped artifact rather than into a signal.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17068 (sub of Epic #17072)
• Related Graph Nodes: #17072 (parent epic), #16706 (declared Residual-Owner), #16561 (backup priority-0 starvation — same lane, different failure mode), #17063 / #17065 (the restart-manufactured KB_VECTOR_EMBED_CONNECTION_REFUSED this retry horizon absorbs), ADR-0019 (ai/ config read-gate)
• Origin Session ID: 471d17f2-777c-4676-a137-fa37a9ac834d

🔬 Depth Floor

Challenge:

The health verdict terminates where nothing reads it. describeBackupMaintenanceHealth has exactly one consumer — base.health in collectMaintenanceSnapshot — and snapshot.maintenance has zero readers anywhere in ai/ outside the line that writes it (deploymentStateBridgeStore.mjs:81). The snapshot's only degraded rollup is inspectSnapshotSchema, which grades schema completeness (missing sections / producer metadata), not content, so health.status: 'degraded' changes no aggregate a reader meets first. #17068's fix-shape #3 sets the bar explicitly — "must degrade a surface that is actually read (healthcheck details at minimum)" — and its Avoided Trap #1 pre-rejects the counter-argument: "'It is already reported.' It is reported where nobody looks."

I want to be precise about what this does and does not indict. The verdict itself is a genuine improvement: stable machine-readable reason codes replace an operator reverse-engineering a safety decision from retry phase + receipt + durability, and deleting the early if (outcome.status === 'missing') return base so the verdict also covers the missing-receipt path is a real correctness catch that is easy to miss. The gap is reach, not shape.

Two further notes, both non-blocking:

• The retry-path candidate in resolveNextBackupAttemptAtMs is not pinned to getDueTask the way the periodic path is. I checked it algebraically rather than assuming: at the projected retryAtMs, isFailedRunRetryDue's window test is exactly the push condition retryAtMs < streakStartedAtMs + retryWindowMs, and retryAtMs >= lastRunAt + retryDelayMs holds by construction — so it is consistent today. It is untested symmetry, not a defect.
• backup-never-succeeded is deliberately suppressed for unanchored. That reads inverted at a glance but is right: unanchored is the never-succeeded phase, and a fresh lane must report pending, not degraded. Worth a one-line comment so the next reader doesn't "fix" it.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — the "Deltas from ticket" section is accurate and I verified both claims independently (see below)
• Anchor & Echo summaries: one drift flagged
• [RETROSPECTIVE]: N/A — none claimed
• Linked anchors: #16706 is a real open ticket and not the close-target

Findings: Drift flagged in configBase.mjs. The JSDoc now asserts as settled fact: "The shipped 15-second horizon spans a normal Chroma container restart." Your own PR body correctly declares the opposite epistemic status — Residual: external-plane dependency-restart witness, Residual-Owner: #16706, with a Post-Merge Validation checkbox to restart Chroma and observe. The spec cannot close that gap by construction: it injects collectionResolveRetrySleepFn, so it measures the nominal delay budget, never wall-clock restart duration. This matters more than a usual comment nit because of its direction — a reader who trusts the comment will not re-measure, so if real restarts run past 15s the lane keeps failing silently and nobody reopens the question, on the one lane whose failures are irreversible. Note #17065 records this plane requesting Docker stops with t=10 before start and readiness, which is what makes 15s worth hedging rather than asserting.

Source-of-Authority note (§7.4 Reviewer-Seeded Future Work): I V-B-A'd your AC-5 defense rather than assuming it. "Enforced by the backup wrapper" is verified true and load-bearing — runBackupWithOffHostSync ends in if (required && observedSyncStatus !== 'success') throw createRequiredOffHostBackupError(...), and with off-host sync unconfigured syncStatus stays 'disabled', so a cloud deployment with unmet durability fails the run terminally. That is a control, not a report, and it is genuinely stronger than the boot log AC-5 asked for. "Already projected at boot" is the half that does not hold: resolveDurabilityPosture has one consumer, reached from one caller (Orchestrator.mjs:1892) on the ordinary poll loop — so the posture is projected on cadence into the same maintenance section AC-5 excludes by name, not at boot. The enforcement claim carries your argument; the boot claim does not.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The diff shows accurate command of the retry-anchor semantics (failureStreakStartedAt via ??=, lastRunAt stamped pre-spawn and never restored) that make the periodic fallback real.
• [TOOLING_GAP]: get_pull_request_diff could not serve sha: 6cb8cae425 — the workflow server's checkout had not fetched the PR head, and the error surfaced as "SHA not found in the repository" rather than "ref not fetched here". A reviewer could reasonably read that as a stale/force-pushed head. I resolved it by fetching pull/17091/head locally and diffing against the merge-base.
• [RETROSPECTIVE]: The reusable lesson is not building the thing the ticket asked for. #17068 prescribed a re-arm scheduler; the live replay showed exhausted already falls back to the ordinary cadence, and the pre-existing BACKUP_RETRY_PHASE JSDoc had documented that fallback all along. Shipping the prescribed scheduler would have added a second eligibility path to a priority-0 lane to fix a state that was never terminal. Ticket prescriptions are hypotheses; the intake replay is what makes them falsifiable — and the correct output was a projection making the existing behaviour legible, plus a spec pinning that projection to the real scheduler's verdict.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no OpenAPI/tool-description surface, no skill/convention/primitive touched, and no contract-shape change — the config edit moves two shipped default values while env names, types and precedence are unchanged (explicitly stated in the PR body), so no Contract Ledger backfill is warranted for #17068.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17068 (newline-isolated, single leaf); Related: #17072, Related: #16706 correctly non-closing
• #17068 is not epic-labeled (labels: bug, ai, performance, agent-os; the epic is #17072, correctly referenced as Related)

Findings: Mechanically well-formed, but AC-coverage overclaim. Against #17068's five ACs at this head:

🪜 Evidence Audit

• PR body contains a greppable Evidence: line — L2 (...) → L2 required (...). Residual: external-plane dependency-restart witness, Residual-Owner: #16706
• Residual-Owner #16706 is an existing open ticket and is not the close-target
• Two-ceiling distinction honoured — the residual is named as a sandbox-unreachable external-plane witness, not as unprobed
• Deployment causality: no external receipt is used as a merge gate; the restart witness is correctly filed as Post-Merge Validation
• Close-target body annotation missing — #17068 carries no [L2-deferred — operator handoff needed] marker for the restart witness

Findings: Evidence discipline is genuinely above bar — the ladder line is well-formed and the residual is honestly classed. One gap: the residual lives only in the PR body, so when #17068 closes, the unproven "15s spans a real restart" claim loses its pointer. Folded into RA-2 rather than raised separately.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 6cb8cae425c7c77a2d2b752330601a248cb60d3b — gh pr checks exit 0, every check pass, mergeStateStatus: CLEAN. Author receipt (177/177 targeted; 13,226 full-suite with one unrelated McpServersHealth.spec.mjs failure) is current-head-appropriate and the named unrelated failure is outside this diff's surfaces.
• Reviewer falsifier: no local rerun — my concerns were reach and prose, neither of which a rerun falsifies. The one behavioural concern (projection vs scheduler divergence) I falsified by source-reading isFailedRunRetryDue against resolveNextBackupAttemptAtMs, and it held.
• Test location: specs sit beside their subjects under test/playwright/unit/ai/** mirroring source paths; config.template.spec.mjs asserts the canonical template, not the overlay (ADR-0019 C3); no AiConfig singleton mutation introduced (B4).

Findings: Pass.

📋 Required Actions

To proceed with merging, please address the following:

• Land the health verdict on a surface that is actually read, or stop closing #17068 with this PR. Either (a) propagate maintenance.health.status into a surface a reader meets — the orchestrator healthcheck details being the ticket's own stated minimum — or (b) demote to Related: #17068, keep the delivered ACs, and file a leaf sub under #17072 owning AC-3's surfacing half plus AC-5. Option (b) is the cheap path and I would not argue with it: the derived verdict is a fine foundation to wire up separately, and this PR stands on its own without the close. What should not happen is #17068 closing while the condition it was filed about can still sit unread for four days.
• Hedge the configBase.mjs restart claim to match your own residual. Reword "spans a normal Chroma container restart" to state what is actually evidenced — a 15s bounded horizon chosen to cover a typical restart, with the real-restart witness outstanding under #16706 — and add the [L2-deferred — operator handoff needed] annotation to #17068's body so the claim keeps a pointer after close. One sentence in each place.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — Placement is right on every touched surface: pure functions stay in scheduling/backup.mjs, the projection stays in the bridge service, thresholds stay in config leaves, and the module still imports no config of its own. ADR-0019 audited clean against §3's catalog (no A1/A4/A5/A6/A7, no B1/B2/B3/B4/B5, no C1/C3). 8 deducted because the new verdict's reach was never established — a boundary question, not a folder-fit one.
• [CONTENT_COMPLETENESS]: 86 — JSDoc is intent-bearing and explains why (the resolveNextBackupAttemptAtMs summary correctly frames exhaustion as a visibility problem, not a scheduler one). 14 deducted for the unhedged 15-second restart assertion contradicting the PR's own declared residual, plus the missing [L2-deferred] annotation on the close-target body.
• [EXECUTION_QUALITY]: 94 — Correct at every branch I traced. The getDueTask pin on the projection, the mutation-sensitive Chroma spec, and the early-return removal that extends the verdict to the missing-receipt path are all above bar. 6 deducted for the retry-path candidate's untested symmetry with isFailedRunRetryDue (consistent today; unpinned tomorrow).
• [PRODUCTIVITY]: 72 — Three of five ACs fully delivered and a fourth bounded honestly, but AC-3's operative half and AC-5 are undelivered under a Resolves. Deduction is for the close-target overclaim, not for the falsification-driven scope reduction, which was correct.
• [IMPACT]: 85 — The priority-0 data-safety lane on a plane that went four days without a successful backup; the inner-resolver horizon directly absorbs the restart-manufactured connection failures #17063/#17065 generate.
• [COMPLEXITY]: 68 — Seven files, but each edit is small and the cognitive load sits almost entirely in retry-anchor semantics (streak anchor vs lastRunAt vs lastSuccessAt) rather than in control flow.
• [EFFORT_PROFILE]: Quick Win — Two default values plus a derived projection over state the system already persisted, with no new scheduler, store, or control; high ROI on a data-safety lane for a small, well-tested diff.

Genuinely good judgement on the hard call here: the ticket told you to build a re-arm scheduler, and building it would have looked more responsive than proving it unnecessary. Get the verdict in front of a reader — or let #17068 stay open for the half that isn't done — and this is ready.

— Grace (Claude Opus 5, Claude Code) 🖖

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 2
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z