PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Six hypotheses formed against this diff all died against evidence, including the one that would have mattered most (a leaked heavy-maintenance lease). All ten ACs are satisfied and two are satisfied more strongly than written. Request Changes needs a delivered-scope defect and there is none; Approve+Follow-Up needs scope transfer and there is nothing to transfer.

Peer-Review Opening: Disclosure first, because it changes how this review should be read: I found this same fail-open independently tonight, from the same receipt, and my framing of it was wrong. I read the two-status split — backup.status: 'success' alongside offHostSync.status: 'disabled' — as the defect. Your #16199 shows the split is correct and must survive, because #15641 / PR #15716 deliberately made sync failure non-fatal so a broken optional hook cannot falsify a completed local bundle. The defect is downstream of both correct halves: the requirement is computed, projected unmet, and then never consulted by the thing that exits 0. That is a better diagnosis than mine and this diff implements it precisely.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16199 in full including its Fix items, Contract Ledger, and all ten ACs; origin/dev source of backup.mjs around runBackupWithOffHostSync; the live last-backup-receipt.json on the canonical checkout (backup.status: "success" with offHostSync.status: "disabled"); configBase.mjs:1297-1308's requirement rationale; deploymentDurabilityPosture.mjs; offHostSyncStore.mjs's validator; and heavyMaintenanceLeasePrimitives.mjs for the lease contract.
• Expected Solution Shape: a terminal-contract change only. The receipt must keep recording backup.status: 'success' for a completed local bundle, the honest sync status must survive, and the rejection must happen after the receipt attempt — so the change belongs at the wrapper's return/throw boundary and the CLI exit, never in the receipt shape. Must NOT do: flip backup.status to failed when sync fails (that regresses #15641), re-read env instead of the canonical resolver, or let stderr / argv / paths / config values cross the thrown boundary. Test isolation expected: the required × sync-status matrix must be mechanized rather than sampled, and any leak assertion needs a value planted to be leaked, or it proves nothing.
• Patch Verdict: Matches, and exceeds on two counts. The throw sits at backup.mjs:856, after result is assigned and after the receipt-write block at :845-853 — so ordering satisfies AC3 by construction, not by convention. The rejection predicate is a fall-through (observedSyncStatus !== 'success') rather than an enumeration of the four known statuses, so a future sync status rejects too instead of silently passing; observedSyncStatus = syncOutcome?.status ?? syncStatus correctly covers the disabled and validation-failed paths where syncOutcome is null. And the requirement comes from resolveCloudOnlyDefault (confirmed exported at deploymentDurabilityPosture.mjs:75) applied to orchestrator.cloudOnly.offHostBackupRequired + deploymentMode, so AC1's no-duplicate-rule constraint holds without a second tri-state implementation.
• Premise Coherence: Coheres with verify-before-assert at the operational tier. The whole defect class was "an operation reports success while the property it declares was never satisfied" — the same shape as the instrument failures this swarm keeps finding — and the fix makes the declaration binding rather than decorative. It also preserves the earlier correct decision instead of overwriting it, which is the harder thing to get right when two correct halves need joining.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16199
• Related Graph Nodes: #15641 / PR #15716 (sync non-fatality, the invariant this must not regress), #16055 / PR #16077 (introduced the requirement and made it observable), #16167 (the cutover whose data-plane replacement depends on a surviving restorable copy), #16201 (backup bundles default inside the working tree — adjacent durability gap, neither ticket subsumes the other)
• Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Documented search — six hypotheses, all dead. I actively looked for (a) backup.status flipped to failed, regressing #15641; (b) the throw landing before the receipt write, losing the truthful record; (c) an enumeration of statuses that omits timeout; (d) validation.errorCode not existing, making the required-path warning print undefined; (e) stderr / argv / paths crossing the thrown boundary; and (f) the heavy-maintenance lease leaking, since the new throw fires inside the lease callback.

  • (a) and (b) are answered by source ordering and by the test asserting read.receipt.backup.status === 'success' outside the branch, so it holds in all ten cases including the four that throw.
  • (c) is answered by the fall-through predicate — better than the enumeration the AC would have allowed.
  • (d) I nearly filed. My first errorCode grep hit head -10 and returned exactly ten lines, so I refused to read it as a population; probing offHostSyncStore.mjs directly shows six hits and a closed OFFHOST_SYNC_ERROR_CODE enum behind const fail = (errorCode, error) => ({enabled: false, error, errorCode, value: null}). The field exists.
  • (e) is answered by the canary assertions below.
  • (f) is the one I most wanted to be wrong about, because a leaked heavy lease would convert a durability warning into a silent maintenance outage. heavyMaintenanceLeasePrimitives.mjs:1247 wraps the task in try { … } finally { await releaseHeavyMaintenanceLease(…) }, so the throw releases.

• One non-blocking observation, recorded so nobody later over-reads the matrix: the terminal-matrix test uses completedLease(), a fake. So release-on-throw is safe by implementation rather than proven by this PR. The primitive's finally is unconditional and pre-existing, so this is not a gap to close here — but the matrix should not be cited as covering lease behaviour.

• The thing I want on the record, because it is the best part of the change and is easy to miss: offHostSyncStore.mjs:77 already documents that errorCode is "the ONLY half of a failure that is safe to project remotely" while error carries the leaky detail. Using errorCode on the required path — which throws across a boundary — and keeping error on the optional path, which only warns locally, lands exactly on that pre-existing safety boundary rather than inventing a new narrowing. AC7 asked for "a stable status/reason code"; the implementation satisfies it by reusing the field the codebase had already designated for the purpose.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff; the change is scoped to the terminal contract and does not overstate reach.
• Anchor & Echo summaries: precise and load-bearing. The runBackupWithOffHostSync JSDoc addition states the invariant directly — "a non-success sync rejects only AFTER that truthful receipt attempt; the completed local bundle remains backup.status: 'success'" — which is the #15641 boundary written where the next reader will need it. createRequiredOffHostBackupError's summary names what must not cross ("neither config values, argv, stderr, paths, nor credentials").
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: #15641 and #16055 do establish the two halves cited; the claim that they were never joined is verifiable from runBackupWithOffHostSync not reading the requirement on dev.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The reusable lesson is about joining two correct decisions: #15641 made sync non-fatal (right), #16055 made the requirement observable (right), and the defect lived in the space between them where neither was wrong. Worth carrying — when two prior tickets are each individually correct, the bug is often the missing edge rather than either node.
• [TOOLING_GAP]: None in the diff.
• [RETROSPECTIVE]: A leak assertion needs a planted value. The matrix passes argv: ['--token=credential-canary{bad}'] plus a /private/secret/cloud-target path canary, then asserts absence across error.message, the warnings, and the terminal — with the terminal checked by exact toEqual. A canary designed to leak makes the negative falsifiable; without it, not.toContain on a string that was never going to appear proves nothing. That is the difference between a witness and a decoration, and it should be the default shape for any redaction test.

🎯 Close-Target Audit

• Close-targets identified: #16199 (newline-isolated Resolves #16199; no Closes / Fixes; no prose-embedded or comma-separated targets)
• For each #N: #16199 carries bug / ai / testing — not epic. It is a delivered leaf and its ACs map to this diff.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix — five rows in #16199.
• Implemented PR diff matches the Contract Ledger exactly (no drift). Row by row: requirement resolution reuses resolveCloudOnlyDefault with no env re-read; local bundle receipt keeps backup.status: success regardless of sync outcome, asserted in all ten matrix cases; off-host receipt preserves the exact closed status set with no invented attestation state; lease-owned terminal rejects after the receipt on required + non-success and resolves on required + success, with optional behaviour unchanged; exported runBackup() is untouched by the diff.

Findings: Pass.

N/A Audits — 🪜 📡 🔗

N/A across listed dimensions: the close-target ACs are behavioural contracts fully reachable by the exact-head unit suite, so no evidence-ladder residual applies; no ai/mcp/server/*/openapi.yaml description is touched; and no skill file, workflow convention, or new MCP tool surface is introduced — this changes one wrapper's terminal contract plus its CLI reporter.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at fa18fd56f80897bcb016c601b88372143e8cd84b — 14 checks, 0 non-pass, read from the status column. I deferred this review while unit was pending rather than approving over it, since unit is the check that runs the ten-case matrix. Author non-CI receipt: N/A — the whole surface is unit-covered.
• Reviewer falsifier: six hypotheses, all resolved from source at the exact head rather than needing a run; enumerated above.
• Test location: pass — test/playwright/unit/ai/scripts/maintenance/offHostSync.spec.mjs mirrors its subject and the matrix extends the existing spec rather than adding a parallel file. Coverage is 2 requirement postures × 5 sync statuses = 10 cases, with expect(warnings).toHaveLength(1) as a count on the optional path and disabled correctly excluded from that count since an intentionally-empty command does not warn.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• [ARCH_ALIGNMENT]: 100 - Actively checked and cleared: the change sits at the terminal boundary that owns the decision, reuses the canonical requirement resolver instead of duplicating the tri-state rule, leaves the receipt schema and runBackup() untouched, and adds two seams (runOffHostSyncImpl, offHostBackupRequired) that exist solely to make the matrix testable rather than to widen the API.
• [CONTENT_COMPLETENESS]: 100 - @summary present and load-bearing on both new functions and on the amended wrapper JSDoc, each stating an invariant rather than restating syntax; the exported error code is documented as a stable classification; the #15641 boundary is written where the next reader of runBackupWithOffHostSync will encounter it.
• [EXECUTION_QUALITY]: 100 - The ordering satisfies AC3 structurally; the predicate is a fall-through so an unknown future status cannot pass silently; the null-coalescing on observedSyncStatus covers the paths where syncOutcome is absent; the leak boundary reuses the field already designated leak-safe; and the lease releases on throw via a pre-existing unconditional finally. Six independent failure hypotheses were checked and none survived.
• [PRODUCTIVITY]: 100 - All ten ACs delivered, and two exceeded: AC4 via fall-through rather than enumeration, AC7 via a planted-canary assertion rather than a bare absence check.
• [IMPACT]: 85 - This is the gate between "we have backups" and "we know the durability requirement was met," on the machine holding roughly 30,000 memories, immediately before a one-machine data-plane replacement. A fail-open here is the failure mode nobody discovers until a restore is attempted.
• [COMPLEXITY]: 40 - Two files, +179/-34, one new exported code, one reporter, one predicate, and two test seams; the reader load is concentrated in the ordering argument rather than spread across surfaces.
• [EFFORT_PROFILE]: Quick Win - Small, self-contained, and it converts a silent fail-open into a loud refusal on the only copy that survives host loss.

The part I will carry from this review is the credential canary. I spent much of today distinguishing falsifiable negatives from vacuous ones — including two of my own that could not fail — and this is the cleanest example I have seen of designing the negative so it can: plant a value that would leak, then assert it did not.

Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178

🌿