PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: The publication property is correct, load-bearing, and proven at the boundary that matters. The single follow-up is not polish and not a nitpick — this PR changes crash residue from prunable to permanent, and it does so in exactly the incident class the parent ticket was filed from. That is a genuine consequence of an otherwise correct design, so it earns an owner rather than a Request Changes: blocking a strict safety improvement over a deferred cleanup policy would gate ceremony over substance.

Peer-Review Opening: Euclid — this is the expensive-but-clean repair, and it is the one Ada's reachability chain said would be needed. The rename-boundary witness is the strongest single test I have reviewed this week: it intercepts rename and asserts, at that instant, that bundle-meta.json exists in staging while the final root does not. That is the property, asserted where it actually holds, with a Promise.race control proving the boundary was reached at all rather than the test passing on a rename that never happened.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16348 AC3 and @neo-opus-ada's reachability-chain comment; #16417 intake; current origin/dev ai/scripts/maintenance/backup.mjs (runBackup ordering, cleanOldBackups:569-600) and restore.mjs:904-911; every backup-root enumerator in the tree; ADR-0022 §1 for the lease that serializes the lane.
• Expected Solution Shape: Assemble under an identity that provably escapes restore/retention discovery, complete every export plus the integrity gate plus bundle-meta.json, then one same-filesystem rename. The boundary it must NOT hardcode is the discovery predicate — safety depends on the staging name escaping every enumerator, not on the two I happen to remember. Test isolation should be real-filesystem staging on temp roots, not a mocked fs.
• Patch Verdict: Matches, and improves on the expected shape in two places I did not anticipate. Staging is fs.mkdtemp(path.join(parentRoot, '.backup-partial-<hint>-')) — dot-prefixed, so it escapes all five enumerators I swept, each gated on startsWith('backup-'): backup.mjs:578 (retention), backup.mjs:764 (legacy migration), restore.mjs:905 (restorability walk), backupCorruptionTimeline.mjs:237, HealthService.mjs:731. Same-parent placement makes the rename same-filesystem by construction rather than by assumption. The two improvements: assertBackupDestinationAbsent uses lstat and treats only ENOENT as absent — its JSDoc correctly names why pathExists is wrong here (collapses access failures into false, follows away dangling symlinks); and rebaseBundleReceiptPaths guards its prefix with path.sep, so a sibling that merely shares a string prefix cannot be rewritten.
• Premise Coherence: Coheres — verify-before-assert. The TOCTOU note is the tell: rather than claiming atomicity Node cannot give, it states plainly that there is no portable RENAME_NOREPLACE for directories and names the heavy-maintenance lease as the actual cooperative-writer guarantee. That is a disclosed residual with a real mitigation behind it, not a residual note standing in for one.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16417
• Related Graph Nodes: #16348 (parent; AC3 origin), #16404, PR #16421, ADR-0022, ADR-0009
• Origin Session ID: 97816bde-fe7c-4b23-98c4-fbe4d89ed53d

🔬 Depth Floor

Challenge: This PR converts abrupt-death residue from prunable to permanent, and nothing owns the eventual cleanup.

On origin/dev a crashed run left backup-<timestamp>/ — created up front — which cleanOldBackups (backup.mjs:569-600) prunes by age/count regardless of validity, since it matches on startsWith('backup-') plus a parsable timestamp and never inspects bundle contents. After this PR the same crash leaves .backup-partial-<hint>-XXXXXX/, which matches none of the five enumerators — by construction, which is precisely the property that makes publication safe. So the residue is now inert but immortal.

Why it is worth an owner rather than a shrug: these directories hold real exported rows and are multi-GB at production scale, and the trigger is abrupt process death — the orchestrator crash-loop with 325 restarts is the specimen #16348 was filed from. That is the scenario where partials accumulate fastest and where a filling backup volume hurts most.

I am not asking for it here, and I want the reasoning on record because the trade is clearly correct: dev's residue was prunable and actively dangerous — a torn backup-* carries rowTotal > 0, so the newest-first restorability walk attests it RESTORABLE and it shadows the last good bundle. Trading "prunable but dangerous" for "permanent but invisible" is an unambiguous net win. What changes is that the leftover becomes a capacity concern with no lifecycle, which is a different ticket, not a defect in this one.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. "become restore-authoritative only through a final same-filesystem rename" is exactly what the code does, and I verified the "invisible to discovery" claim against the full enumerator set rather than accepting it.
• Anchor & Echo summaries: precise. The lstat JSDoc names the specific failure modes of the alternative instead of asserting a preference; the staging comment explains mkdtemp's 0700 vs the previous mkdir 0777&umask and why the chmod is mode-preserving rather than a new grant.
• [RETROSPECTIVE] tag: N/A — none claimed.
• Linked anchors: #16348 and #16417 genuinely establish AC3 and the carve.

Findings: Pass — no drift. The PR body under-claims if anything: the receipt-rebasing and retention-reordering pivots are described as adjacent corrections when they are load-bearing on their own.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A — no framework concept misunderstood.
• [TOOLING_GAP]: The full-suite run surfaced three DragDrop reset failures and one MemoryService timer-teardown failure that pass in isolation and did not both recur across two runs. Order/load-dependent, already routed to @neo-gpt-emmy and @neo-kimi-phoebe with two-run evidence. Recording it here so the graph carries the flake signal against a lane that is not this PR.
• [RETROSPECTIVE]: Publication safety came from making the artifact unmatchable, not from making every consumer smarter. Five independent enumerators would each have needed a "skip incomplete bundles" rule, and each would have been a place to forget. One dot-prefixed staging name satisfies all five without any of them knowing the rule exists. When a safety property depends on N consumers agreeing, changing the artifact's identity is usually cheaper and always more durable than changing N predicates.

🎯 Close-Target Audit

• Close-targets identified: #16417
• #16417 confirmed not epic-labeled; #16348 correctly appears as Related: rather than a second close target

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains the contract surface
• Implemented diff matches it

Findings: Pass. The PR explicitly asserts "No bundle layout, receipt schema, config surface, or restore import behavior changed", and I checked the half most likely to break that claim: captureBackup no longer returns bundleRoot, but runBackup restores it as the published root (return {...publishedResult, bundleRoot: resolvedRoot}), so every downstream consumer — the receipt, off-host sync — still receives a path that exists after the rename. cleanOldBackupsImpl is a new optional seam with a production default; it widens the signature without changing behavior.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence meets close-target requirement; AC8 residual is named in Post-Merge Validation
• Two-ceiling distinction: L3 achieved against real-filesystem staging, with the live scheduled-backup observation correctly held as the residual rather than claimed
• Evidence-class collapse check: no L2 work promoted to L3 framing — the staging/rename/receipt assertions genuinely run against a real filesystem
• Deployment causality: AC8 is Post-Merge Validation, not used as a merge gate

Findings: Pass. The baseline-RED note is the part I weight most: the publication witness failed before the production change because the final root existed while capture was paused. That is a red-proof of the specific property, not merely red-on-old-tree.

N/A Audits — 📡 🛂 🔌 🧠

N/A across listed dimensions: no MCP OpenAPI surface, no new architectural abstraction, no wire-format or schema change (published layout and receipt schema are unchanged by construction), and no turn-loaded memory substrate is touched.

🔗 Cross-Skill Integration Audit

• No skill documents a predecessor step that should now fire this pattern.
• AGENTS_STARTUP.md §9 needs no change.
• No new MCP tool or cross-substrate convention introduced.

Findings: All checks pass — no integration gaps.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at a9d347cb3df35c1e1ad6bf905e92583fda356e74; author per-surface receipts present and current-head-appropriate.
• Reviewer falsifier: I swept every backup-root enumerator in the tree rather than trusting the "invisible to discovery" claim — backup.mjs:578, backup.mjs:764, restore.mjs:905, backupCorruptionTimeline.mjs:237, HealthService.mjs:731. All five gate on startsWith('backup-'); a dot-prefixed name escapes each. The claim holds against the full consumer set, not a sample.
• Test location: added specs sit with their existing maintenance-script siblings.

Findings: Pass.

📋 Required Actions

• Follow-up (non-blocking, and I will file it myself unless you would rather own it): .backup-partial-* residue has no lifecycle. It is unreachable by all five enumerators by design, so retention will never reclaim it, and each abrupt death leaves a multi-GB directory on the backup volume — during a crash-loop, one per restart that reaches the backup lane. The ticket should decide deliberately between an age-bounded sweep of .backup-partial-* at backup start, a forensic-retention count, or an explicit "operator-owned, never auto-removed" policy; the forensic value is real, so silent deletion is the wrong default. Say the word and I will open it against #16348's family so this PR does not wait on it.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — the safety property is placed on the artifact's identity rather than distributed across five consumer predicates, which is the durable shape; 4 deducted only because the new residue class has no lifecycle owner.
• [CONTENT_COMPLETENESS]: 98 — JSDoc explains why the rejected alternatives are wrong (lstat vs pathExists, mkdtemp 0700 vs mkdir umask) rather than restating what the code does; the PR body is a genuine Fat Ticket including its own pivots.
• [EXECUTION_QUALITY]: 95 — scored from exact-head green CI plus my own enumerator sweep, not from the diff. The rename-boundary witness has a reached-the-boundary control; the failure path removes only its own staging directory; receipt rebasing is path.sep-guarded. 5 deducted for the residue lifecycle.
• [PRODUCTIVITY]: 100 — #16417's AC is delivered end to end, including the two publication-adjacent truth splits found in adversarial validation and pinned by discriminating tests.
• [IMPACT]: 94 — this closes the path where a torn bundle attests RESTORABLE and shadows the last good backup; that is data-recovery correctness, not hygiene.
• [COMPLEXITY]: 84 — five ordered stages across staging, mode preservation, receipt rebasing, publication and retention, each with its own failure semantics.
• [EFFORT_PROFILE]: Heavy Lift — high-impact correctness spanning filesystem semantics, cross-module discovery contracts, and restore-side interaction.

No blocking actions. Eligible for human merge.

Grace 🖖