refactor(rebuild): journal destructive recovery by jyaunches · Pull Request #6470 · NVIDIA/NemoClaw

jyaunches · 2026-07-08T13:42:39Z

Stacked PR

Depends on #6468. Review this PR against refactor/6434-rebuild-transaction-store; it contains the destructive-boundary integration.

Summary

persist prepared after validated backup and before MCP detach or sandbox deletion
advance durable receipts through old_deleted, replacement_created, and completed
recover prepared or old_deleted in a fresh process and skip duplicate deletion
take a fresh backup and refresh a still-prepared journal when a failed-delete retry finds the old sandbox live
persist scoped legacy managed-image recovery authority and the original shields posture so a fresh process can resume safely after deletion
persist the original registry row and default-selection ownership; restore only a missing row and never overwrite a replacement or steal a later default
roll back ownership-proven destroyPrepared MCP state non-destructively before rebuild and fail closed for ambiguous/post-delete markers
keep recreate failures active at old_deleted with retry guidance

Architectural simplification

The registry row remains the primary rebuild source contract instead of being removed during normal rebuild. The journal carries a recovery capsule for the exceptional case where that preserved row disappears. Transaction transitions now live in RebuildTransactionCoordinator; rebuild-pipeline.ts is 364 lines and focuses on phase sequencing. This removes the 97-line registry rollback participant, its 174-line test, obsolete removal plumbing, and inline journal transition branches.

Advisor resolution

Prepared live retry: takes a fresh backup before any detach/delete and atomically refreshes only a prepared transaction's intent/backup receipts.
Registry recovery: the journal persists the original row, prior default ownership, and default-selection revision; restoration is atomic, never overwrites a replacement, and reclaims default only while the captured revision still owns it.
Incomplete MCP destroy: a live destroyPrepared marker with an exact durable ownership manifest is restored non-destructively; destroyPending, absent-sandbox, or incomplete ownership fails before backup/delete.
Recovery errors: corrupt or mismatched journals use the normal redacted preflight bail path; a failed retry-metadata write no longer suppresses registry/MCP recovery and backup guidance.
Interruption hook: requires the shared E2E gates, a Vitest process, and a private fixture root equal to the child temporary HOME, so leaked environment variables cannot stop an ordinary CLI process.
Durable source intent: fresh-process recovery preserves the original shields posture and therefore the correct relock guidance.
MCP recovery invariant: the registry marker source of truth, compensation boundary, regression coverage, and removal condition are recorded beside the preflight.
replacement_created: remains the explicit refactor(rebuild): journal the destructive boundary and resume absent-sandbox recovery #6435 non-goal, but now reports the validated backup and a concrete snapshot-restore command; crash-after-create coverage proves the fail-closed boundary.
Backup ordering: immutable backup intentionally precedes journal creation, so failure can leave only a harmless unused backup—not an active transaction without recovery data.
Structure: transaction coordination is extracted; store concurrency, schema-validation, and prepared-refresh tests are split; the main store test is 372 lines, below the advisor's verification threshold.

Tests

42 transaction-store/state-machine and cross-process tests, including competing-writer and prepared-refresh coverage
16 destructive-boundary tests covering ordering, live retry backup refresh, recovery error handling, failure persistence, drift, and fresh-process recovery
7 MCP rebuild phase tests, including non-destructive prepared-destroy recovery and replacement-row preservation
2 default-inert interruption-hook tests
2 real SIGKILL integration tests at prepared and external-delete-before-journal checkpoints
68 registry tests, including missing-row restoration and changed-default preservation
existing prepared-recovery, DCode pre-delete/recovery, stale-recovery, destroy, and recreate regressions
npm run build:cli
npm run checks
npm run check:diff
npx tsc -p tsconfig.cli.json --noEmit

Line budget

Against the updated stacked base: +1,704 / -681, net +1,023 total; production is +685 / -220, net +465. The additional safety branches and process/concurrency regression coverage mean this phase is not line-neutral. The coordinator extraction and rollback deletion contain the growth, and later forward-recovery/finalization phases should continue retiring compatibility paths toward epic-level neutrality.

Closes #6435

Signed-off-by: Julie Yaunches jyaunches@nvidia.com

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

copy-pr-bot · 2026-07-08T13:42:43Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

coderabbitai · 2026-07-08T13:42:50Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9b5526a8-8e1a-4b79-80d9-b93637670557

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch refactor/6435-rebuild-destructive-journal

_{Comment @coderabbitai help to get the list of available commands.}

github-code-quality · 2026-07-08T13:43:21Z

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the refactor/6435-rebuil... branch is 96%. Coverage data for the refactor/6434-rebuil... branch is not yet available.

Show a code coverage summary of the most covered files.

File	`refactor/6434-rebuil...`	`refactor/6435-rebuil...` 3fb3cbc	+/-
`nemoclaw/src/se...cret-scanner.ts`	—	100%	—
`nemoclaw/src/commands/slash.ts`	—	100%	—
`nemoclaw/src/li...bprocess-env.ts`	—	100%	—
`nemoclaw/src/bl...eprint/state.ts`	—	98%	—
`nemoclaw/src/onboard/config.ts`	—	98%	—
`nemoclaw/src/bl...int/snapshot.ts`	—	97%	—
`nemoclaw/src/bl...print/runner.ts`	—	95%	—
`nemoclaw/src/co...ration-state.ts`	—	94%	—
`nemoclaw/src/bl...ate-networks.ts`	—	94%	—
`nemoclaw/src/index.ts`	—	94%	—

TypeScript / code-coverage/cli

The overall coverage in the refactor/6435-rebuil... branch is 76%. Coverage data for the refactor/6434-rebuil... branch is not yet available.

Show a code coverage summary of the most covered files.

File	`refactor/6434-rebuil...`	`refactor/6435-rebuil...` 3fb3cbc	+/-
`src/lib/onboard/preflight.ts`	—	82%	—
`src/lib/state/o...oard-session.ts`	—	82%	—
`src/lib/actions...all/run-plan.ts`	—	81%	—
`src/lib/actions...licy-channel.ts`	—	79%	—
`src/lib/actions...box/snapshot.ts`	—	79%	—
`src/lib/state/sandbox.ts`	—	72%	—
`src/lib/onboard...er-gpu-patch.ts`	—	69%	—
`src/lib/policy/index.ts`	—	66%	—
`src/lib/shields/index.ts`	—	61%	—
`src/lib/onboard.ts`	—	28%	—

_{Updated July 08, 2026 18:08 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.}

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

github-actions · 2026-07-08T13:45:09Z

E2E Advisor Recommendation

Required E2E: sandbox-rebuild, rebuild-openclaw, rebuild-hermes, mcp-bridge
Optional E2E: state-backup-restore, sandbox-operations, registry-targets

Dispatch hint: sandbox-rebuild,rebuild-openclaw,rebuild-hermes,mcp-bridge

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/refactor/6434-rebuild-transaction-store
Head: HEAD
Confidence: high

Required E2E

sandbox-rebuild (high): Merge-blocking coverage for the core changed path: real onboard, live OpenShell sandbox rebuild, workspace marker preservation, registry metadata refresh, and rebuild-backup credential hygiene.
rebuild-openclaw (high): The transaction and registry changes affect real OpenClaw rebuild user flows, including old-base/old-OpenClaw migration, state preservation, gateway token rotation, session/registry files, backup manifests, and config hash refresh.
rebuild-hermes (high): The generic rebuild pipeline now owns transaction journaling, delete/recreate callbacks, and registry recovery for every agent. Hermes has distinct install/base-image/messaging-placeholder/backup behavior that should be validated against the new transaction flow.
mcp-bridge (very high): Required because MCP rebuild preparation/recovery and registry preservation changed. This live job exercises MCP add/remove, provider-backed credentials, secret absence after rotation, and rebuild across OpenClaw, Hermes, and Deep Agents Code sandboxes.

Optional E2E

state-backup-restore (medium): Useful adjacent confidence for backup/restore artifacts because the rebuild transaction now journals backup manifests and changes recovery behavior after recreate failures.
sandbox-operations (medium): Optional collateral check for destroy/registry behavior because destroy.ts and registry.ts changed, even though the main behavioral risk is rebuild rather than ordinary destroy.
registry-targets (medium): Optional targeted confidence for registry/default-selection semantics touched by reversible removal and rebuild recovery metadata.

New E2E recommendations

rebuild transaction process-death recovery (high): No existing live E2E appears to kill or stop the real CLI at the new rebuild_prepared and rebuild_delete_unjournaled checkpoints and then rerun rebuild against real OpenShell/Docker state. Unit/process fixtures do not fully prove live recovery from these destructive-boundary windows.
- Suggested test: Add a workflow-dispatched rebuild-transaction-resume live E2E that enables the private rebuild interruption gate, stops/kills the CLI at rebuild_prepared and rebuild_delete_unjournaled, reruns nemoclaw <sandbox> rebuild --yes, and verifies registry/session/backup/workspace/MCP recovery.
MCP destroy-marker and rebuild interlock (medium): The PR adds fail-closed logic for incomplete MCP destroy transactions before rebuild. Existing mcp-bridge live coverage exercises MCP rebuild, but not a live prepared/pending destroy marker handoff into rebuild.
- Suggested test: Extend the MCP bridge live suite or add a focused mcp-rebuild-destroy-interlock E2E that creates an MCP bridge, leaves a controlled prepared/pending destroy marker, invokes rebuild, and verifies safe recovery or fail-closed behavior without deleting the sandbox unexpectedly.

Dispatch hint

Workflow: .github/workflows/e2e.yaml
jobs input: sandbox-rebuild,rebuild-openclaw,rebuild-hermes,mcp-bridge

github-actions · 2026-07-08T13:48:38Z

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-5: revalidatePreparedRecoveryBeforeDelete does not re-validate MCP state at delete edge; then add or justify PRA-T1.
Open items: 3 required · 19 warnings · 8 suggestions · 8 test follow-ups
Since last review: 1 prior item resolved · 9 still apply · 8 new items found

Action checklist

Findings index

ID	Severity	Category	Location	Required action
`PRA-1`	Resolve/justify	architecture	—	Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
`PRA-2`	Resolve/justify	architecture	—	Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
`PRA-3`	Resolve/justify	architecture	—	Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
`PRA-4`	Resolve/justify	architecture	—	Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
`PRA-5`	Required	acceptance	`src/lib/actions/sandbox/rebuild-prepared-recovery.ts:54`	Either add MCP state revalidation to revalidatePreparedRecoveryBeforeDelete (compare sandbox.mcp bridges/fingerprints against preflight snapshot), or add a SOURCE_OF_TRUTH comment documenting why the MCP lifecycle lock prevents drift at this boundary, with a regression test that simulates concurrent MCP modification.
`PRA-6`	Required	correctness	`src/lib/state/rebuild-transaction.ts:570`	Either ensure the state directory is always on the same filesystem as the temp file (document the constraint in README/ops), or implement a verified cross-filesystem atomic write fallback (write to target filesystem, fsync, rename).
`PRA-7`	Required	security	`src/lib/state/rebuild-transaction.ts:532`	Add a regression test that simulates torn directory entry (or documents the gap in CI), and track F_FULLFSYNC availability in Node.js. Consider a native adapter for macOS durability when Node exposes F_FULLFSYNC.
`PRA-8`	Resolve/justify	security	`src/lib/state/rebuild-transaction.ts:200`	Consider redacting credentialEnv in the journal (store only a hash or boolean flag), or document that the state directory must be protected at the OS level. The current 0600/0700 permissions are a reasonable baseline.
`PRA-9`	Resolve/justify	correctness	`src/lib/actions/sandbox/rebuild-mcp-phase.ts:30`	Add a test that simulates partial failure in restoreMcpBridgesAfterDestroyAbort and verifies the sandbox is left in a recoverable state (or that the rebuild fails closed with clear guidance).
`PRA-10`	Resolve/justify	correctness	`src/lib/actions/sandbox/rebuild-preflight-phase.ts:100`	Either add automatic cleanup of invalid transaction journals (with user confirmation), or document that manual cleanup is required and add a CLI command for it. At minimum, improve error message to include journal path and cleanup guidance.
`PRA-11`	Resolve/justify	tests	`test/rebuild-transaction-process-death.test.ts:1`	Add a platform check (process.platform !== 'win32') and skip the interruption on Windows in rebuild-e2e-interruption.ts. Add test annotations (e.g., @vitest-environment node, skip on Windows) or implement cross-platform PID recycle simulation for macOS in the test fixture.
`PRA-12`	Resolve/justify	security	`src/lib/state/registry.ts:145`	Document the macOS/Windows limitation clearly. Consider using a lock file with embedded start timestamp (already done) plus a secondary identity marker (e.g., random token written at lock acquisition) to detect PID recycle without /proc.
`PRA-13`	Resolve/justify	tests	`src/lib/state/rebuild-transaction-validation.test.ts:1`	Add parameterized tests for fingerprint/timestamp pattern boundaries: 63/64/65 hex chars, timestamps with second/millisecond precision, backup timestamp format variations (HH-MM-SS-mmm vs HH:MM:SS.mmm).
`PRA-14`	Resolve/justify	scope	`src/lib/actions/sandbox/rebuild-e2e-interruption.ts:12`	Add a platform check (process.platform !== 'win32') and skip the interruption on Windows, or document that process-death tests are Unix-only.
`PRA-15`	Resolve/justify	architecture	`src/lib/state/rebuild-transaction.ts:922`	Extract validation functions (normalizeRecord, normalizeIntent, normalizeReceipts, normalizeFailure, fingerprint/timestamp validators) to a separate rebuild-transaction-validation.ts module to reduce monolith size.
`PRA-16`	Resolve/justify	architecture	`src/lib/state/registry.ts:775`	Extract rebuild-related registry functions (restoreRebuildRegistryRecoveryIfMissing, restorePreservedSandboxEntryIfMissing) to a rebuild-registry-recovery.ts module.
`PRA-17`	Improvement	architecture	`src/lib/state/rebuild-transaction.ts:145`	Consider using HMAC-SHA256 with a fixed per-installation secret (derived from state directory) for filename generation, or document why plain SHA-256 is acceptable.
`PRA-18`	Improvement	docs	`ci/env-var-doc-allowlist.json:80`	Add an 'internal': true field to the allowlist schema for vars that are never user-facing, or ensure the documentation generator renders these distinctly.
`PRA-19`	Improvement	architecture	`src/lib/state/rebuild-transaction.ts:721`	Verify that syncDirectory(dirPath) in durablePublish is called for the parent transaction directory on first publish. The create() method calls durablePublish with createOnly=true when existing===null, which should fsync the directory after linkSync.
`PRA-20`	Improvement	architecture	`src/lib/actions/sandbox/rebuild-registry-rollback.ts:1`	Verify that all rollback scenarios from the old rebuild-registry-rollback.test.ts are covered by the new transaction boundary tests. The old tests covered: prepared backup recovery restore, ordinary removal receipt restore, stale recovery with MCP, replacement row preservation, and early no-op handling.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

`PRA-5` Required — revalidatePreparedRecoveryBeforeDelete does not re-validate MCP state at delete edge

Location: src/lib/actions/sandbox/rebuild-prepared-recovery.ts:54
Category: acceptance
Problem: revalidatePreparedRecoveryBeforeDelete re-loads registry and validates backup manifest but does not re-validate MCP state. The prior review (PRA-7) required either re-running MCP preflight at the delete edge or documenting why MCP state cannot drift. The new code moves MCP validation to preflight (preflightMcpRebuildState) but the delete-edge revalidation only checks registry entry deep-equality and backup manifest identity. The MCP lifecycle lock (withMcpLifecycleLock) is held across the rebuild, but it is advisory — a process that does not acquire the lock could modify MCP registry state between preflight and delete edge.
Impact: MCP bridges/providers could be in an inconsistent state at the delete edge. If another process modifies MCP registry state (adds/removes bridges) between preflight and sandbox deletion, the rebuild proceeds with stale MCP preparation.
Required action: Either add MCP state revalidation to revalidatePreparedRecoveryBeforeDelete (compare sandbox.mcp bridges/fingerprints against preflight snapshot), or add a SOURCE_OF_TRUTH comment documenting why the MCP lifecycle lock prevents drift at this boundary, with a regression test that simulates concurrent MCP modification.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Compare src/lib/actions/sandbox/rebuild-prepared-recovery.ts:54-87 (revalidatePreparedRecoveryBeforeDelete) with src/lib/actions/sandbox/rebuild-preflight-phase.ts:140-145 (mcpPreflight call). Check if MCP lifecycle lock prevents concurrent MCP changes.
Missing regression test: Add a test that modifies MCP state (e.g., adds/removes an MCP bridge entry in registry) between preflight and delete edge and verifies the rebuild fails closed or re-prepares MCP state.
Done when: The required change is committed and verification passes: Compare src/lib/actions/sandbox/rebuild-prepared-recovery.ts:54-87 (revalidatePreparedRecoveryBeforeDelete) with src/lib/actions/sandbox/rebuild-preflight-phase.ts:140-145 (mcpPreflight call). Check if MCP lifecycle lock prevents concurrent MCP changes.
Evidence: rebuild-preflight-phase.ts calls preflightMcpRebuildState at line 142. rebuild-prepared-recovery.ts revalidatePreparedRecoveryBeforeDelete only checks registry entry deep equality and backup manifest identity. The MCP lifecycle lock (withMcpLifecycleLock) is held across the rebuild but MCP state could be modified by another process if the lock is advisory.

`PRA-6` Required — durablePublish throws on EXDEV instead of cross-filesystem atomic write fallback

Location: src/lib/state/rebuild-transaction.ts:570
Category: correctness
Problem: durablePublish() throws 'candidate and record must share a filesystem' on EXDEV (cross-filesystem link failure) rather than falling back to a cross-filesystem atomic write (write to target filesystem, fsync, rename). If the state directory (~/.nemoclaw/state) and temp file (/tmp) are on different filesystems (common: tmpfs vs disk), first transaction creation fails closed.
Impact: Availability/DoS: rebuild fails on first attempt when state directory and /tmp are on different filesystems. No fallback, no actionable guidance for operators.
Required action: Either ensure the state directory is always on the same filesystem as the temp file (document the constraint in README/ops), or implement a verified cross-filesystem atomic write fallback (write to target filesystem, fsync, rename).
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Read src/lib/state/rebuild-transaction.ts:570-590 — verify the EXDEV handling and whether the same-filesystem constraint is documented or enforced.
Missing regression test: Add a test that injects EXDEV (e.g., by mocking fs.linkSync to throw EXDEV) and verifies the fallback behavior or documents the constraint with actionable error message.
Done when: The required change is committed and verification passes: Read src/lib/state/rebuild-transaction.ts:570-590 — verify the EXDEV handling and whether the same-filesystem constraint is documented or enforced.
Evidence: durablePublish at line 570-590 uses fs.linkSync for atomic publication on createOnly=true path. On EXDEV it throws 'Rebuild transaction atomic-publication invariant failed: candidate and record must share a filesystem' with no fallback.

`PRA-7` Required — syncDirectory lacks F_FULLFSYNC on macOS — torn directory entry on power loss

Location: src/lib/state/rebuild-transaction.ts:532
Category: security
Problem: syncDirectory() uses fs.fsyncSync() on the directory file descriptor, which on macOS does not guarantee directory entry persistence after power loss (requires F_FULLFSYNC). The SOURCE_OF_TRUTH comment acknowledges this gap but the boundary is not surfaced in test coverage or operational documentation.
Impact: On macOS, a power loss after durablePublish() but before the directory entry is physically persisted could leave the transaction file unreachable (torn directory entry), breaking rebuild recovery. This is a data-loss risk for macOS users.
Required action: Add a regression test that simulates torn directory entry (or documents the gap in CI), and track F_FULLFSYNC availability in Node.js. Consider a native adapter for macOS durability when Node exposes F_FULLFSYNC.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Read src/lib/state/rebuild-transaction.ts:532-548 — verify the SOURCE_OF_TRUTH comment documents the macOS gap and removal condition. Check test/rebuild-transaction-process-death.test.ts for any macOS-specific power-loss simulation.
Missing regression test: Add a test (or CI annotation) that verifies the durability boundary on macOS, or documents the known gap with a removal condition: 'use F_FULLFSYNC when Node exposes it (or a native adapter is adopted)'.
Done when: The required change is committed and verification passes: Read src/lib/state/rebuild-transaction.ts:532-548 — verify the SOURCE_OF_TRUTH comment documents the macOS gap and removal condition. Check test/rebuild-transaction-process-death.test.ts for any macOS-specific power-loss simulation.
Evidence: syncDirectory at line 532-548 opens directory fd and calls fs.fsyncSync(fd). The SOURCE_OF_TRUTH comment explicitly states macOS requires F_FULLFSYNC which Node does not expose. No test covers this.

Review findings by urgency: 3 required fixes, 19 items to resolve/justify, 8 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

`PRA-1` Resolve/justify — Source-of-truth review needed: src/lib/state/rebuild-transaction.ts:532

Location: not file-specific
Category: architecture
Problem: The advisor marked localized patch analysis as needs_followup.
Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Missing regression test: No automated test for torn directory entry on macOS
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Evidence: SOURCE_OF_TRUTH comment at lines 532-548 documents all five fields

`PRA-2` Resolve/justify — Source-of-truth review needed: src/lib/state/rebuild-transaction.ts:570

Location: not file-specific
Category: architecture
Problem: The advisor marked localized patch analysis as needs_followup.
Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Missing regression test: No test for EXDEV fallback
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Evidence: PUBLICATION_BOUNDARY comment at lines 560-580 documents constraint; EXDEV throws with no fallback

`PRA-3` Resolve/justify — Source-of-truth review needed: src/lib/state/registry.ts:145

Location: not file-specific
Category: architecture
Problem: The advisor marked localized patch analysis as needs_followup.
Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Missing regression test: No test simulating PID recycle on non-Linux
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Evidence: readProcessStartMs returns null on non-Linux; classifyExistingLock falls back to age-only (LOCK_STALE_MS=10s)

`PRA-4` Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/rebuild-preflight-phase.ts:98

Location: not file-specific
Category: architecture
Problem: The advisor marked localized patch analysis as needs_followup.
Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Missing regression test: No test for stale journal cleanup or guidance
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Evidence: loadRebuildRecovery throws on mismatch but leaves journal; rebuild-preflight-phase.ts:100-110 bails without cleanup

`PRA-8` Resolve/justify — Transaction journal stores credentialEnv and endpointFingerprint — expanded attack surface

Location: src/lib/state/rebuild-transaction.ts:200
Category: security
Problem: RebuildTransactionIntentV1 stores credentialEnv (environment variable name like NVIDIA_API_KEY) and endpointFingerprint (SHA-256 of endpoint URL) in the transaction journal at ~/.nemoclaw/state/rebuild-transactions/<hash>.json (0600). While permissions are restrictive, the journal persists credential environment variable names and hashed endpoints across rebuild attempts.
Impact: If an attacker gains read access to the state directory, they learn which env vars hold credentials and can infer endpoint structure from fingerprints. Not a direct credential leak but expands attack surface.
Recommended action: Consider redacting credentialEnv in the journal (store only a hash or boolean flag), or document that the state directory must be protected at the OS level. The current 0600/0700 permissions are a reasonable baseline.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/state/rebuild-transaction.ts:200-250 (RebuildTransactionIntentV1) — verify credentialEnv and endpointFingerprint are stored in the intent.target and intent.source sections.
Missing regression test: Add a test that verifies transaction file permissions are 0600 and directory is 0700.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/state/rebuild-transaction.ts:200-250 (RebuildTransactionIntentV1) — verify credentialEnv and endpointFingerprint are stored in the intent.target and intent.source sections.
Evidence: RebuildTransactionIntentV1 at lines 200-250 includes credentialEnv in target and endpointFingerprint in target. Files created with 0600 mode in durablePublish.

`PRA-9` Resolve/justify — Partial MCP restore on restoreMcpBridgesAfterDestroyAbort failure leaves inconsistent state

Location: src/lib/actions/sandbox/rebuild-mcp-phase.ts:30
Category: correctness
Problem: preflightMcpRebuildState handles MCP destroy recovery by calling prepareMcpBridgesForDestroy and restoreMcpBridgesAfterDestroyAbort. If restoreMcpBridgesAfterDestroyAbort fails partway through, the sandbox may be left in an inconsistent state (some providers restored, some not). The function catches errors and calls bail(), but the MCP state may be partially restored.
Impact: Sandbox MCP state could be partially restored after a failed recovery attempt, leaving bridges/providers in an undefined state that may require manual intervention.
Recommended action: Add a test that simulates partial failure in restoreMcpBridgesAfterDestroyAbort and verifies the sandbox is left in a recoverable state (or that the rebuild fails closed with clear guidance).
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/actions/sandbox/rebuild-mcp-phase.ts:30-80 — verify error handling in preflightMcpRebuildState and whether partial restore is rolled back.
Missing regression test: Add a test that mocks restoreMcpBridgesAfterDestroyAbort to fail partway and verifies the rebuild fails closed with actionable guidance.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/actions/sandbox/rebuild-mcp-phase.ts:30-80 — verify error handling in preflightMcpRebuildState and whether partial restore is rolled back.
Evidence: preflightMcpRebuildState at lines 30-65 calls restoreMcpBridgesAfterDestroyAbort in a try/catch but does not attempt to roll back partial provider restoration on error.

`PRA-10` Resolve/justify — Stale transaction journal not cleaned up on fingerprint mismatch

Location: src/lib/actions/sandbox/rebuild-preflight-phase.ts:100
Category: correctness
Problem: runRebuildPreflightPhase calls loadRebuildRecovery which validates the transaction journal against the current registry and latest backup. If validation fails (fingerprint mismatch), it prints a failure and returns null. However, the function does not clean up a stale transaction journal that no longer matches reality — it leaves the corrupt journal on disk. A subsequent rebuild attempt would hit the same validation failure.
Impact: User gets repeated 'Rebuild transaction recovery failed' errors with no automatic recovery path. Manual journal deletion required but not guided.
Recommended action: Either add automatic cleanup of invalid transaction journals (with user confirmation), or document that manual cleanup is required and add a CLI command for it. At minimum, improve error message to include journal path and cleanup guidance.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/actions/sandbox/rebuild-preflight-phase.ts:50-80 — verify loadRebuildRecovery error handling and whether stale journals are cleaned up or guidance provided.
Missing regression test: Add a test that creates a stale transaction journal (mismatched fingerprint) and verifies the rebuild fails with clear guidance on how to clean up (journal path, rm command).
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/actions/sandbox/rebuild-preflight-phase.ts:50-80 — verify loadRebuildRecovery error handling and whether stale journals are cleaned up or guidance provided.
Evidence: loadRebuildRecovery at rebuild-transaction-coordinator.ts:50-70 throws on fingerprint mismatch but does not delete the journal file. rebuild-preflight-phase.ts:100-110 catches and bails without cleanup.

`PRA-11` Resolve/justify — Process-death recovery tests only run on Linux/Unix — no Windows support

Location: test/rebuild-transaction-process-death.test.ts:1
Category: tests
Problem: The new test/rebuild-transaction-process-death.test.ts spawns child processes and uses SIGKILL/SIGSTOP, which are Unix-only. The rebuild-e2e-interruption.ts hook uses process.kill(process.pid, 'SIGSTOP') which is not portable to Windows. No platform guards or conditional test execution.
Impact: Test suite cannot validate process-death recovery on Windows. Windows CI would either skip these tests (gap) or crash on SIGSTOP.
Recommended action: Add a platform check (process.platform !== 'win32') and skip the interruption on Windows in rebuild-e2e-interruption.ts. Add test annotations (e.g., @vitest-environment node, skip on Windows) or implement cross-platform PID recycle simulation for macOS in the test fixture.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Check test/rebuild-transaction-process-death.test.ts for platform conditionals. Check src/lib/actions/sandbox/rebuild-e2e-interruption.ts for platform guards.
Missing regression test: Add a test annotation (e.g., skip on Windows) or implement cross-platform PID recycle simulation. Add a test that verifies the hook is inert on Windows.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Check test/rebuild-transaction-process-death.test.ts for platform conditionals. Check src/lib/actions/sandbox/rebuild-e2e-interruption.ts for platform guards.
Evidence: test/rebuild-transaction-process-death.test.ts uses spawn, SIGKILL, /proc for PID detection. rebuild-e2e-interruption.ts:12 calls process.kill(process.pid, 'SIGSTOP') without platform check.

`PRA-12` Resolve/justify — PID recycle detection falls back to age-only on non-Linux

Location: src/lib/state/registry.ts:145
Category: security
Problem: classifyExistingLock PID recycle detection uses /proc/<pid>/stat to detect PID reuse; on macOS/Windows readProcessStartMs returns null and the lock relies solely on age-based staleness (LOCK_STALE_MS = 10s). A fast PID recycle on non-Linux could cause a stale lock to be incorrectly treated as valid.
Impact: On macOS/Windows, a crashed process holding the lock could have its PID recycled within 10 seconds, causing a new process to incorrectly treat the stale lock as valid and wait instead of reclaiming it, or vice versa.
Recommended action: Document the macOS/Windows limitation clearly. Consider using a lock file with embedded start timestamp (already done) plus a secondary identity marker (e.g., random token written at lock acquisition) to detect PID recycle without /proc.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/state/registry.ts:145-180 — verify the PID recycle detection logic and fallback behavior in classifyExistingLock and readProcessStartMs.
Missing regression test: Add a test that simulates PID recycle on non-Linux (mock readProcessStartMs to return null) and verifies the lock behavior with secondary identity marker.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/state/registry.ts:145-180 — verify the PID recycle detection logic and fallback behavior in classifyExistingLock and readProcessStartMs.
Evidence: readProcessStartMs at line 145 reads /proc/<pid>/stat and /proc/stat. Returns null on error (non-Linux). classifyExistingLock falls back to age-only check when start time unavailable.

`PRA-13` Resolve/justify — Validation tests lack fingerprint/timestamp pattern edge cases

Location: src/lib/state/rebuild-transaction-validation.test.ts:1
Category: tests
Problem: Validation tests have parameterized tests for some timestamp shapes but do not cover: fingerprint boundaries (63/64/65 hex chars), timestamps with second vs millisecond precision, backup timestamp format variations (BACKUP_TIMESTAMP_PATTERN expects HH-MM-SS-mmm).
Impact: Edge cases in fingerprint/timestamp validation may accept invalid formats or reject valid ones. No regression coverage for boundary conditions.
Recommended action: Add parameterized tests for fingerprint/timestamp pattern boundaries: 63/64/65 hex chars, timestamps with second/millisecond precision, backup timestamp format variations (HH-MM-SS-mmm vs HH:MM:SS.mmm).
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/state/rebuild-transaction-validation.test.ts — check for parameterized tests covering pattern boundaries.
Missing regression test: Add parameterized tests for: fingerprint length boundaries (63/64/65 chars), ISO timestamp with/without milliseconds, backup timestamp format (HH-MM-SS-mmm vs HH:MM:SS.mmm).
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/state/rebuild-transaction-validation.test.ts — check for parameterized tests covering pattern boundaries.
Evidence: rebuild-transaction-validation.test.ts has timestamp shape tests but no fingerprint length boundary tests. BACKUP_TIMESTAMP_PATTERN at rebuild-transaction.ts:15 expects HH-MM-SS-mmm format.

`PRA-14` Resolve/justify — maybePauseForRebuildInterruption uses SIGSTOP without Windows guard

Location: src/lib/actions/sandbox/rebuild-e2e-interruption.ts:12
Category: scope
Problem: maybePauseForRebuildInterruption uses process.kill(process.pid, 'SIGSTOP') which is not portable to Windows. The function is testing-only (guarded by VITEST=1 and NEMOCLAW_E2E_FAILURE_INJECTION=1) but the lack of platform guard means it would crash on Windows if the test fixture were ever run there.
Impact: Process crash on Windows if test fixture environment variables are accidentally set. Low risk in practice but violates cross-platform compatibility.
Recommended action: Add a platform check (process.platform !== 'win32') and skip the interruption on Windows, or document that process-death tests are Unix-only.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/actions/sandbox/rebuild-e2e-interruption.ts:1-15 — verify the SIGSTOP call and lack of platform guard.
Missing regression test: Add a test that verifies the hook is inert on Windows (or document Unix-only scope).
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/actions/sandbox/rebuild-e2e-interruption.ts:1-15 — verify the SIGSTOP call and lack of platform guard.
Evidence: Line 12: process.kill(process.pid, 'SIGSTOP') with no process.platform check.

`PRA-15` Resolve/justify — rebuild-transaction.ts monolith growth (922 lines) — extract validation module

Location: src/lib/state/rebuild-transaction.ts:922
Category: architecture
Problem: rebuild-transaction.ts grew by 82 lines (now 922 lines), exceeding the monolith growth threshold. The file contains transaction record types, validation, durable publication, and the RebuildTransactionStore class.
Impact: Maintainability risk. Validation logic is coupled with storage logic. Harder to test validation independently.
Recommended action: Extract validation functions (normalizeRecord, normalizeIntent, normalizeReceipts, normalizeFailure, fingerprint/timestamp validators) to a separate rebuild-transaction-validation.ts module to reduce monolith size.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Check file line count: src/lib/state/rebuild-transaction.ts is 922 lines. Validation functions span lines 200-450 approximately.
Missing regression test: Not applicable — refactoring suggestion. Existing validation tests would cover extracted module.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Check file line count: src/lib/state/rebuild-transaction.ts is 922 lines. Validation functions span lines 200-450 approximately.
Evidence: File grew from 840 to 922 lines. Validation functions (normalizeRecord, normalizeIntent, normalizeReceipts, normalizeFailure, fingerprint, timestamp, backupManifestTimestamp) are 200+ lines.

`PRA-16` Resolve/justify — registry.ts monolith growth (775 lines) — extract rebuild recovery functions

Location: src/lib/state/registry.ts:775
Category: architecture
Problem: registry.ts grew by 62 lines (now 775 lines), exceeding the monolith growth threshold. The file added restoreRebuildRegistryRecoveryIfMissing and related functions.
Impact: Maintainability risk. Rebuild-specific recovery logic mixed with core registry operations.
Recommended action: Extract rebuild-related registry functions (restoreRebuildRegistryRecoveryIfMissing, restorePreservedSandboxEntryIfMissing) to a rebuild-registry-recovery.ts module.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Check file line count: src/lib/state/registry.ts is 775 lines. Rebuild recovery functions at lines 650-690.
Missing regression test: Not applicable — refactoring suggestion. Existing registry tests cover these functions.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Check file line count: src/lib/state/registry.ts is 775 lines. Rebuild recovery functions at lines 650-690.
Evidence: restoreRebuildRegistryRecoveryIfMissing (lines 650-690) and restorePreservedSandboxEntryIfMissing (lines 615-635) are rebuild-specific recovery logic in core registry module.

`PRA-22` Resolve/justify — replacement_created phase blocks automatic resume — user must intervene

Location: src/lib/actions/sandbox/rebuild-transaction-coordinator.ts:50
Category: security
Problem: loadRebuildRecovery throws an error if transaction.phase === 'replacement_created', preventing automatic resume. The error message instructs the user to inspect the replacement and run snapshot restore manually. This is a deliberate design choice (boundary documented in comment), but it means process-death recovery cannot automatically resume after replacement creation — the user must intervene.
Impact: Process-death after replacement creation requires manual recovery (snapshot restore). User must run CLI command with specific timestamp. No automated resume path.
Recommended action: Document this limitation clearly in user-facing docs. Consider whether a future PR should implement automatic resume after replacement creation once OpenShell provides atomic replacement verification.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/actions/sandbox/rebuild-transaction-coordinator.ts:50-70 — verify the replacement_created phase handling and error message.
Missing regression test: Test already exists in rebuild-transaction-boundary.test.ts: 'fails closed with recovery guidance after replacement creation'.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/actions/sandbox/rebuild-transaction-coordinator.ts:50-70 — verify the replacement_created phase handling and error message.
Evidence: loadRebuildRecovery at rebuild-transaction-coordinator.ts:50-70 throws 'Rebuild transaction already created a replacement, so this phase cannot be resumed automatically in refactor(rebuild): journal the destructive boundary and resume absent-sandbox recovery #6435. Inspect the replacement before changing it. Validated backup: ... If state restoration is needed, run nemoclaw <sandbox> snapshot restore <timestamp>.'

`PRA-23` Resolve/justify — MCP lifecycle lock uses advisory file lock — not mandatory

Location: src/lib/state/mcp-lifecycle-lock-storage.ts:1
Category: correctness
Problem: The MCP lifecycle lock (withMcpLifecycleLock) uses a file-based lock with PID/token ownership and stale detection. It is advisory — processes that don't acquire the lock (direct registry manipulation, buggy CLI) can bypass it. This enables the PRA-7 MCP drift issue.
Impact: Lock only protects cooperating processes. A malicious or buggy process can modify registry/MCP state during rebuild.
Recommended action: Document that the lock is advisory and only serializes cooperating NemoClaw processes. For PRA-7 resolution, either make MCP validation at delete edge independent of lock assumption, or add kernel-enforced lock (flock) if available.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/state/mcp-lifecycle-lock-acquisition.ts and mcp-lifecycle-lock-storage.ts — verify lock is file-based advisory with PID/token ownership.
Missing regression test: Add a test that demonstrates advisory lock bypass (direct registry write during rebuild) and verifies MCP drift detection or documents the assumption.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/state/mcp-lifecycle-lock-acquisition.ts and mcp-lifecycle-lock-storage.ts — verify lock is file-based advisory with PID/token ownership.
Evidence: withMcpLifecycleLock uses AsyncLocalStorage for intra-process reentrancy and file lock for inter-process. No flock/fcntl mandatory lock. Lock file at ~/.nemoclaw/state/mcp-lifecycle-locks/<hash>.lock.

`PRA-25` Resolve/justify — Forced local cleanup preserves registry entry with MCP ownership when gateway unreachable

Location: src/lib/actions/sandbox/destroy.ts:190
Category: security
Problem: When gateway is unreachable and --force is used with MCP ownership, destroy preserves local state because it contains MCP ownership required for exact provider cleanup. The registry entry is not removed. This is existing behavior but the transaction journal now adds a durable record of this state.
Impact: Local registry entry persists with MCP markers. Subsequent rebuild must handle this state. The transaction journal may capture this as a prepared recovery scenario.
Recommended action: Verify that the transaction journal correctly handles forced-local-cleanup state (MCP ownership preserved, registry entry present). The destroy flow should not create a transaction journal since it's a separate command path.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/actions/sandbox/destroy.ts:190-220 — verify forcedLocalCleanup logic and that it doesn't create a rebuild transaction.
Missing regression test: Add a test that destroys a sandbox with MCP ownership when gateway unreachable and --force, then verifies rebuild handles the preserved state correctly.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/actions/sandbox/destroy.ts:190-220 — verify forcedLocalCleanup logic and that it doesn't create a rebuild transaction.
Evidence: destroy.ts:190-220 shows forcedLocalCleanup preserves registry entry when mcpOwnershipRequiresGateway. destroy command is separate from rebuild so no transaction journal created.

`PRA-26` Resolve/justify — Legacy managed-image recovery authority persisted from prior transaction

Location: src/lib/actions/sandbox/rebuild-preflight-phase.ts:141
Category: correctness
Problem: allowLegacyManagedImageRecovery is derived from opts.allowLegacyManagedImageRecovery OR recovery.transaction.intent.source.legacyManagedImageRecoveryAuthorized. This persists the legacy recovery capability across process restarts via the transaction journal.
Impact: Legacy recovery authorization (granted via explicit user confirmation in installer) is now durable in the transaction journal. A resumed rebuild after process death retains this capability without re-prompting.
Recommended action: Verify this is intentional and documented. The capability should only be granted after explicit user confirmation in the original installer flow, which is preserved in the journal.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read rebuild-preflight-phase.ts:141-145 — verify allowLegacyManagedImageRecovery derivation. Check installer flow that sets this capability.
Missing regression test: Add a test that resumes a prepared legacy recovery transaction and verifies the capability is preserved without re-prompting.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read rebuild-preflight-phase.ts:141-145 — verify allowLegacyManagedImageRecovery derivation. Check installer flow that sets this capability.
Evidence: Line 141: allowLegacyManagedImageRecovery = (opts.recoveryManifest !== undefined && opts.allowLegacyManagedImageRecovery === true) || (recovery.transaction?.status === 'active' && recovery.transaction.intent.source.legacyManagedImageRecoveryAuthorized)

`PRA-29` Resolve/justify — Add fingerprint length boundary tests (63/64/65 hex chars)

Location: src/lib/state/rebuild-transaction-validation.test.ts:1
Category: tests
Problem: FINGERPRINT_PATTERN expects ^sha256:[0-9a-f]{64}$ (64 hex chars). No tests for 63 or 65 char boundaries.
Impact: Boundary validation untested. Could accept invalid fingerprints or reject valid ones at edges.
Recommended action: Add parameterized tests for fingerprint length: 63 chars (reject), 64 chars (accept), 65 chars (reject).
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/state/rebuild-transaction-validation.test.ts — add fingerprint length parameterized tests.
Missing regression test: Add test: 'normalizeRecord rejects fingerprint with 63 hex chars', 'accepts 64 hex chars', 'rejects 65 hex chars'.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/state/rebuild-transaction-validation.test.ts — add fingerprint length parameterized tests.
Evidence: FINGERPRINT_PATTERN at rebuild-transaction.ts:15 requires exactly 64 hex chars. Validation test file has timestamp tests but no fingerprint length tests.

`PRA-30` Resolve/justify — Add backup timestamp format variation tests

Location: src/lib/state/rebuild-transaction-validation.test.ts:1
Category: tests
Problem: BACKUP_TIMESTAMP_PATTERN expects HH-MM-SS-mmm format (e.g., 2026-07-08T00-00-00-000Z). No tests for colon-separated variant (HH:MM:SS.mmm) or other variations.
Impact: Backup timestamp parsing may reject valid timestamps generated by different tools or locales.
Recommended action: Add parameterized tests for backup timestamp format: accept documented format, reject colon-separated variant.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/state/rebuild-transaction.ts:15 for BACKUP_TIMESTAMP_PATTERN. Add tests in rebuild-transaction-validation.test.ts.
Missing regression test: Add test: 'backupManifestTimestamp accepts HH-MM-SS-mmm format', 'rejects HH:MM:SS.mmm format'.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/state/rebuild-transaction.ts:15 for BACKUP_TIMESTAMP_PATTERN. Add tests in rebuild-transaction-validation.test.ts.
Evidence: BACKUP_TIMESTAMP_PATTERN = /^(\d{4}-\d{2}-\d{2}T)(\d{2})-(\d{2})-(\d{2})-(\d{3})Z$/. Only matches dash-separated time components.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

`PRA-17` Improvement — transactionFileStem uses plain SHA-256 — consider keyed hash (HMAC-SHA256)

Location: src/lib/state/rebuild-transaction.ts:145
Category: architecture
Problem: transactionFileStem uses SHA-256 of sandbox name for filename generation. Collision resistance is adequate but could use keyed hash (HMAC-SHA256) with a fixed per-installation secret (derived from state directory) to prevent intentional collision attacks.
Impact: Low — defense-in-depth. Intentional collision would require attacker to control sandbox name and know the hash algorithm.
Suggested action: Consider using HMAC-SHA256 with a fixed per-installation secret (derived from state directory) for filename generation, or document why plain SHA-256 is acceptable.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Read src/lib/state/rebuild-transaction.ts:145 — verify transactionFileStem implementation.
Missing regression test: Not applicable — defense-in-depth improvement.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: transactionFileStem at line 145: crypto.createHash('sha256').update(sandboxName).digest('hex')

`PRA-18` Improvement — E2E env vars not marked internal-only in allowlist

Location: ci/env-var-doc-allowlist.json:80
Category: docs
Problem: NEMOCLAW_REBUILD_PROCESS_FIXTURE and related E2E env vars (NEMOCLAW_REBUILD_PROCESS_PHASE, NEMOCLAW_REBUILD_PROCESS_STATE_DIR, NEMOCLAW_REBUILD_PROCESS_EVENTS, NEMOCLAW_REBUILD_PROCESS_ROLE) are documented but not marked as internal-only in the allowlist. The prior review (PRA-6) requested an 'internal': true field for vars that are never user-facing.
Impact: Documentation generator may render these as user-configurable. No functional impact but misleading.
Suggested action: Add an 'internal': true field to the allowlist schema for vars that are never user-facing, or ensure the documentation generator renders these distinctly.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Read ci/env-var-doc-allowlist.json — check for 'internal': true on E2E/test-only vars.
Missing regression test: Not applicable — documentation improvement.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: New entry at line 80 for NEMOCLAW_REBUILD_PROCESS_FIXTURE lacks 'internal': true. Other E2E vars in allowlist also lack it.

`PRA-19` Improvement — Verify RebuildTransactionStore.create fsyncs parent directory on first publish

Location: src/lib/state/rebuild-transaction.ts:721
Category: architecture
Problem: RebuildTransactionStore.create() calls durablePublish with createOnly=true when existing===null. durablePublish calls syncDirectory(dirPath) after linkSync, where dirPath is the transaction directory. This may address PRA-11, but verify the first create path also fsyncs the parent transaction directory.
Impact: If parent directory not fsynced on first create, a power loss could lose the transaction directory entry.
Suggested action: Verify that syncDirectory(dirPath) in durablePublish is called for the parent transaction directory on first publish. The create() method calls durablePublish with createOnly=true when existing===null, which should fsync the directory after linkSync.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Read src/lib/state/rebuild-transaction.ts:721 (create method) and 650 (durablePublish) — verify syncDirectory is called for the transaction directory on first create.
Missing regression test: Add a test that verifies directory fsync on first transaction creation (or document that it's covered by existing durablePublish tests).
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: create() at line 721 calls durablePublish(this.path(sandboxName), record, existing === null). durablePublish at line 650 calls syncDirectory(dirPath) after linkSync/renameSync regardless of createOnly.

`PRA-20` Improvement — Verify deleted registry rollback tests covered by new transaction boundary tests

Location: src/lib/actions/sandbox/rebuild-registry-rollback.ts:1
Category: architecture
Problem: rebuild-registry-rollback.ts and its test were deleted. The registry rollback logic is now handled by the transaction journal (RebuildTransactionStore/Coordinator). This is a significant architectural change that replaces a rollback mechanism with a durable transaction log.
Impact: Risk of regression if old rollback scenarios not fully covered by new tests.
Suggested action: Verify that all rollback scenarios from the old rebuild-registry-rollback.test.ts are covered by the new transaction boundary tests. The old tests covered: prepared backup recovery restore, ordinary removal receipt restore, stale recovery with MCP, replacement row preservation, and early no-op handling.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Compare deleted test src/lib/actions/sandbox/rebuild-registry-rollback.test.ts with new tests in src/lib/actions/sandbox/rebuild-transaction-boundary.test.ts and test/rebuild-transaction-process-death.test.ts.
Missing regression test: Ensure all 5 old rollback test scenarios have equivalent coverage in the new transaction boundary tests.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Deleted files: rebuild-registry-rollback.ts (97 lines) and rebuild-registry-rollback.test.ts (174 lines). New transaction boundary tests cover: normal completion, process interruption, resume, registry recovery, MCP destroy recovery, legacy recovery, shields posture.

`PRA-21` Improvement — Test infrastructure growth adds maintenance burden

Location: src/lib/actions/sandbox/rebuild-transaction-boundary.test.ts:1
Category: tests
Problem: New test file rebuild-transaction-boundary.test.ts is 475 lines and covers transaction boundary scenarios comprehensively. However, it uses a complex test harness (createRebuildFlowHarness) with many spies. The test infrastructure growth (rebuild-flow-test-harness.ts +108 lines, rebuild-flow-test-support.ts +11 lines, rebuild-transaction-store.ts +5 lines) adds maintenance burden.
Impact: Increased test maintenance cost. Complex harness may hide brittle assumptions.
Suggested action: Review test infrastructure growth. The new transaction boundary tests are valuable but the harness complexity should be monitored. Consider extracting common test patterns to reduce duplication.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Check test/helpers/rebuild-flow-test-harness.ts line count and complexity.
Missing regression test: Not applicable — test architecture observation.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: rebuild-transaction-boundary.test.ts: 475 lines. rebuild-flow-test-harness.ts grew by 108 lines. harness uses many vi.spyOn mocks for OpenShell, registry, MCP, shields, backup, onboard.

`PRA-24` Improvement — Add CI annotation for macOS durability gap

Location: src/lib/state/rebuild-transaction.ts:532
Category: workflow
Problem: The SOURCE_OF_TRUTH comment documents the macOS F_FULLFSYNC gap but no CI annotation or test marks this known limitation. Operators on macOS are unaware of the power-loss durability boundary.
Impact: macOS users may assume full durability guarantees that don't exist. No operational visibility.
Suggested action: Add a CI annotation (e.g., GitHub Actions job annotation) or startup log warning on macOS noting the durability boundary. Track F_FULLFSYNC availability in Node.js for future native adapter.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Check .github/workflows for macOS-specific annotations. Check CLI startup for platform warnings.
Missing regression test: Add a CI annotation or test that documents the macOS durability gap with removal condition.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: SOURCE_OF_TRUTH comment at rebuild-transaction.ts:532-548 documents gap. No CI or runtime visibility.

`PRA-27` Improvement — MCP destroy markers as recovery source of truth — SOURCE_OF_TRUTH documented

Location: src/lib/actions/sandbox/rebuild-mcp-phase.ts:22
Category: architecture
Problem: The registry's MCP destroy markers (destroyPreparedAt, destroyPendingAt) are the recovery source of truth. Only an ownership-proven 'prepared' marker with the sandbox still present is safe to compensate; 'pending', an absent sandbox, or an incomplete manifest may have crossed the delete boundary and must fail closed.
Impact: Well-documented source-of-truth boundary. Prevents automatic recovery of ambiguous MCP destroy state.
Suggested action: No action needed — this is a correctly documented SOURCE_OF_TRUTH workaround with regression tests and removal condition (OpenShell atomic recreate).
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Read src/lib/actions/sandbox/rebuild-mcp-phase.ts:22-28 — verify SOURCE_OF_TRUTH comment contains all five fields.
Missing regression test: Already covered: rebuild-mcp-phase.test.ts:76-100 tests ownership-proven prepared destroy recovery.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: SOURCE_OF_TRUTH comment at lines 22-28: invalid state (incomplete destroy crosses delete boundary), source boundary (registry markers), source-fix constraint (OpenShell atomic recreate), regression test (recovery tests below), removal condition (until MCP destroy joins rebuild transaction journal).

`PRA-28` Improvement — Test-only interruption hook — acceptable as-is

Location: src/lib/actions/sandbox/rebuild-e2e-interruption.ts:1
Category: architecture
Problem: maybePauseForRebuildInterruption is testing-only (guarded by VITEST=1, NEMOCLAW_E2E_FAILURE_INJECTION=1, NEMOCLAW_REBUILD_PROCESS_FIXTURE=HOME). The multiple guards ensure it is inert in production. Single-use config pattern is appropriate for a test-only hook.
Impact: No production impact. Test infrastructure for process-death injection.
Suggested action: No simplification needed. The guard chain is appropriate for a test-only feature. Add Windows platform guard (PRA-9) and it's complete.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Read src/lib/actions/sandbox/rebuild-e2e-interruption.ts:1-15 — verify all five guards.
Missing regression test: Already covered: rebuild-e2e-interruption.test.ts tests inert without fixture, stops at selected checkpoint.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Five guards: VITEST=true, fixtureRoot exists, fixtureRoot===HOME, NEMOCLAW_E2E_FAILURE_INJECTION=1, NEMOCLAW_E2E_FORCE_FAIL_AT_STEP matches phase.

Simplification opportunities: 4 possible cuts, net -250 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

PRA-15 shrink (src/lib/state/rebuild-transaction.ts:922): Validation functions (normalizeRecord, normalizeIntent, normalizeReceipts, normalizeFailure, fingerprint, timestamp, backupManifestTimestamp, requiredString, nullableString, nullableEnvName, isExactIsoTimestamp, safeRevision)
- Replacement: Import from new rebuild-transaction-validation.ts module
- Net: -200 lines
- Safety boundary: Normalization logic must remain fail-closed and identical; all existing validation tests must pass unchanged.
PRA-16 shrink (src/lib/state/registry.ts:775): restoreRebuildRegistryRecoveryIfMissing, restorePreservedSandboxEntryIfMissing
- Replacement: Import from new rebuild-registry-recovery.ts module
- Net: -50 lines
- Safety boundary: Registry recovery logic must preserve atomic default-selection revision semantics; all existing registry tests must pass unchanged.
PRA-17 shrink (src/lib/state/rebuild-transaction.ts:145): Plain SHA-256 for transaction file stem
- Replacement: HMAC-SHA256 with per-installation secret derived from state directory
- Net: 0 lines
- Safety boundary: Filename generation must remain deterministic and collision-resistant; existing transaction files must remain loadable (migration not needed as stem is per-sandbox).
PRA-28 yagni (src/lib/actions/sandbox/rebuild-e2e-interruption.ts:1): Consider removing if process-death tests are refactored to not need SIGSTOP injection
- Replacement: N/A — keep for now
- Net: 0 lines
- Safety boundary: Must remain inert in production (all five guards). Must not affect non-test processes.

Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

PRA-T1 Runtime validation — durablePublish handles EXDEV with cross-filesystem atomic write fallback OR fails with actionable guidance when state dir crosses filesystems. Runtime/sandbox/infrastructure paths need behavioral runtime validation: destroy.ts, mcp-bridge-rebuild.ts, mcp-bridge.ts, rebuild-destroy-phase.ts, rebuild-e2e-interruption.ts, rebuild-flow-helpers.ts, rebuild-mcp-phase.ts. The SIGKILL integration tests (test/rebuild-transaction-process-death.test.ts) provide runtime validation for transaction coordinator and process-death recovery. MCP/destroy/rebuild flow tests use harness mocking OpenShell/Docker — existing pattern.
PRA-T2 Runtime validation — syncDirectory durability boundary: documents macOS F_FULLFSYNC gap in CI annotation. Runtime/sandbox/infrastructure paths need behavioral runtime validation: destroy.ts, mcp-bridge-rebuild.ts, mcp-bridge.ts, rebuild-destroy-phase.ts, rebuild-e2e-interruption.ts, rebuild-flow-helpers.ts, rebuild-mcp-phase.ts. The SIGKILL integration tests (test/rebuild-transaction-process-death.test.ts) provide runtime validation for transaction coordinator and process-death recovery. MCP/destroy/rebuild flow tests use harness mocking OpenShell/Docker — existing pattern.
PRA-T3 Runtime validation — normalizeRecord rejects fingerprint with 63 hex chars, accepts 64 hex chars, rejects 65 hex chars. Runtime/sandbox/infrastructure paths need behavioral runtime validation: destroy.ts, mcp-bridge-rebuild.ts, mcp-bridge.ts, rebuild-destroy-phase.ts, rebuild-e2e-interruption.ts, rebuild-flow-helpers.ts, rebuild-mcp-phase.ts. The SIGKILL integration tests (test/rebuild-transaction-process-death.test.ts) provide runtime validation for transaction coordinator and process-death recovery. MCP/destroy/rebuild flow tests use harness mocking OpenShell/Docker — existing pattern.
PRA-T4 Runtime validation — backupManifestTimestamp accepts documented HH-MM-SS-mmm format, rejects HH:MM:SS.mmm variant. Runtime/sandbox/infrastructure paths need behavioral runtime validation: destroy.ts, mcp-bridge-rebuild.ts, mcp-bridge.ts, rebuild-destroy-phase.ts, rebuild-e2e-interruption.ts, rebuild-flow-helpers.ts, rebuild-mcp-phase.ts. The SIGKILL integration tests (test/rebuild-transaction-process-death.test.ts) provide runtime validation for transaction coordinator and process-death recovery. MCP/destroy/rebuild flow tests use harness mocking OpenShell/Docker — existing pattern.
PRA-T5 Runtime validation — loadRebuildRecovery cleans up or guides manual cleanup when fingerprint mismatches. Runtime/sandbox/infrastructure paths need behavioral runtime validation: destroy.ts, mcp-bridge-rebuild.ts, mcp-bridge.ts, rebuild-destroy-phase.ts, rebuild-e2e-interruption.ts, rebuild-flow-helpers.ts, rebuild-mcp-phase.ts. The SIGKILL integration tests (test/rebuild-transaction-process-death.test.ts) provide runtime validation for transaction coordinator and process-death recovery. MCP/destroy/rebuild flow tests use harness mocking OpenShell/Docker — existing pattern.
PRA-T6 Process-death recovery tests only run on Linux/Unix — no Windows support — Add a platform check (process.platform !== 'win32') and skip the interruption on Windows in rebuild-e2e-interruption.ts. Add test annotations (e.g., @vitest-environment node, skip on Windows) or implement cross-platform PID recycle simulation for macOS in the test fixture.
PRA-T7 Validation tests lack fingerprint/timestamp pattern edge cases — Add parameterized tests for fingerprint/timestamp pattern boundaries: 63/64/65 hex chars, timestamps with second/millisecond precision, backup timestamp format variations (HH-MM-SS-mmm vs HH:MM:SS.mmm).
PRA-T8 Test infrastructure growth adds maintenance burden — Review test infrastructure growth. The new transaction boundary tests are valuable but the harness complexity should be monitored. Consider extracting common test patterns to reduce duplication.

Since last review details

Current findings, using the urgency labels above:

`PRA-1` Resolve/justify — Source-of-truth review needed: src/lib/state/rebuild-transaction.ts:532

Location: not file-specific
Category: architecture
Problem: The advisor marked localized patch analysis as needs_followup.
Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Missing regression test: No automated test for torn directory entry on macOS
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Evidence: SOURCE_OF_TRUTH comment at lines 532-548 documents all five fields

`PRA-2` Resolve/justify — Source-of-truth review needed: src/lib/state/rebuild-transaction.ts:570

Location: not file-specific
Category: architecture
Problem: The advisor marked localized patch analysis as needs_followup.
Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Missing regression test: No test for EXDEV fallback
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Evidence: PUBLICATION_BOUNDARY comment at lines 560-580 documents constraint; EXDEV throws with no fallback

`PRA-3` Resolve/justify — Source-of-truth review needed: src/lib/state/registry.ts:145

Location: not file-specific
Category: architecture
Problem: The advisor marked localized patch analysis as needs_followup.
Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Missing regression test: No test simulating PID recycle on non-Linux
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Evidence: readProcessStartMs returns null on non-Linux; classifyExistingLock falls back to age-only (LOCK_STALE_MS=10s)

`PRA-4` Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/rebuild-preflight-phase.ts:98

Location: not file-specific
Category: architecture
Problem: The advisor marked localized patch analysis as needs_followup.
Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Missing regression test: No test for stale journal cleanup or guidance
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Evidence: loadRebuildRecovery throws on mismatch but leaves journal; rebuild-preflight-phase.ts:100-110 bails without cleanup

`PRA-5` Required — revalidatePreparedRecoveryBeforeDelete does not re-validate MCP state at delete edge

Location: src/lib/actions/sandbox/rebuild-prepared-recovery.ts:54
Category: acceptance
Problem: revalidatePreparedRecoveryBeforeDelete re-loads registry and validates backup manifest but does not re-validate MCP state. The prior review (PRA-7) required either re-running MCP preflight at the delete edge or documenting why MCP state cannot drift. The new code moves MCP validation to preflight (preflightMcpRebuildState) but the delete-edge revalidation only checks registry entry deep-equality and backup manifest identity. The MCP lifecycle lock (withMcpLifecycleLock) is held across the rebuild, but it is advisory — a process that does not acquire the lock could modify MCP registry state between preflight and delete edge.
Impact: MCP bridges/providers could be in an inconsistent state at the delete edge. If another process modifies MCP registry state (adds/removes bridges) between preflight and sandbox deletion, the rebuild proceeds with stale MCP preparation.
Required action: Either add MCP state revalidation to revalidatePreparedRecoveryBeforeDelete (compare sandbox.mcp bridges/fingerprints against preflight snapshot), or add a SOURCE_OF_TRUTH comment documenting why the MCP lifecycle lock prevents drift at this boundary, with a regression test that simulates concurrent MCP modification.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Compare src/lib/actions/sandbox/rebuild-prepared-recovery.ts:54-87 (revalidatePreparedRecoveryBeforeDelete) with src/lib/actions/sandbox/rebuild-preflight-phase.ts:140-145 (mcpPreflight call). Check if MCP lifecycle lock prevents concurrent MCP changes.
Missing regression test: Add a test that modifies MCP state (e.g., adds/removes an MCP bridge entry in registry) between preflight and delete edge and verifies the rebuild fails closed or re-prepares MCP state.
Done when: The required change is committed and verification passes: Compare src/lib/actions/sandbox/rebuild-prepared-recovery.ts:54-87 (revalidatePreparedRecoveryBeforeDelete) with src/lib/actions/sandbox/rebuild-preflight-phase.ts:140-145 (mcpPreflight call). Check if MCP lifecycle lock prevents concurrent MCP changes.
Evidence: rebuild-preflight-phase.ts calls preflightMcpRebuildState at line 142. rebuild-prepared-recovery.ts revalidatePreparedRecoveryBeforeDelete only checks registry entry deep equality and backup manifest identity. The MCP lifecycle lock (withMcpLifecycleLock) is held across the rebuild but MCP state could be modified by another process if the lock is advisory.

`PRA-6` Required — durablePublish throws on EXDEV instead of cross-filesystem atomic write fallback

Location: src/lib/state/rebuild-transaction.ts:570
Category: correctness
Problem: durablePublish() throws 'candidate and record must share a filesystem' on EXDEV (cross-filesystem link failure) rather than falling back to a cross-filesystem atomic write (write to target filesystem, fsync, rename). If the state directory (~/.nemoclaw/state) and temp file (/tmp) are on different filesystems (common: tmpfs vs disk), first transaction creation fails closed.
Impact: Availability/DoS: rebuild fails on first attempt when state directory and /tmp are on different filesystems. No fallback, no actionable guidance for operators.
Required action: Either ensure the state directory is always on the same filesystem as the temp file (document the constraint in README/ops), or implement a verified cross-filesystem atomic write fallback (write to target filesystem, fsync, rename).
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Read src/lib/state/rebuild-transaction.ts:570-590 — verify the EXDEV handling and whether the same-filesystem constraint is documented or enforced.
Missing regression test: Add a test that injects EXDEV (e.g., by mocking fs.linkSync to throw EXDEV) and verifies the fallback behavior or documents the constraint with actionable error message.
Done when: The required change is committed and verification passes: Read src/lib/state/rebuild-transaction.ts:570-590 — verify the EXDEV handling and whether the same-filesystem constraint is documented or enforced.
Evidence: durablePublish at line 570-590 uses fs.linkSync for atomic publication on createOnly=true path. On EXDEV it throws 'Rebuild transaction atomic-publication invariant failed: candidate and record must share a filesystem' with no fallback.

`PRA-7` Required — syncDirectory lacks F_FULLFSYNC on macOS — torn directory entry on power loss

Location: src/lib/state/rebuild-transaction.ts:532
Category: security
Problem: syncDirectory() uses fs.fsyncSync() on the directory file descriptor, which on macOS does not guarantee directory entry persistence after power loss (requires F_FULLFSYNC). The SOURCE_OF_TRUTH comment acknowledges this gap but the boundary is not surfaced in test coverage or operational documentation.
Impact: On macOS, a power loss after durablePublish() but before the directory entry is physically persisted could leave the transaction file unreachable (torn directory entry), breaking rebuild recovery. This is a data-loss risk for macOS users.
Required action: Add a regression test that simulates torn directory entry (or documents the gap in CI), and track F_FULLFSYNC availability in Node.js. Consider a native adapter for macOS durability when Node exposes F_FULLFSYNC.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Read src/lib/state/rebuild-transaction.ts:532-548 — verify the SOURCE_OF_TRUTH comment documents the macOS gap and removal condition. Check test/rebuild-transaction-process-death.test.ts for any macOS-specific power-loss simulation.
Missing regression test: Add a test (or CI annotation) that verifies the durability boundary on macOS, or documents the known gap with a removal condition: 'use F_FULLFSYNC when Node exposes it (or a native adapter is adopted)'.
Done when: The required change is committed and verification passes: Read src/lib/state/rebuild-transaction.ts:532-548 — verify the SOURCE_OF_TRUTH comment documents the macOS gap and removal condition. Check test/rebuild-transaction-process-death.test.ts for any macOS-specific power-loss simulation.
Evidence: syncDirectory at line 532-548 opens directory fd and calls fs.fsyncSync(fd). The SOURCE_OF_TRUTH comment explicitly states macOS requires F_FULLFSYNC which Node does not expose. No test covers this.

`PRA-8` Resolve/justify — Transaction journal stores credentialEnv and endpointFingerprint — expanded attack surface

Location: src/lib/state/rebuild-transaction.ts:200
Category: security
Problem: RebuildTransactionIntentV1 stores credentialEnv (environment variable name like NVIDIA_API_KEY) and endpointFingerprint (SHA-256 of endpoint URL) in the transaction journal at ~/.nemoclaw/state/rebuild-transactions/<hash>.json (0600). While permissions are restrictive, the journal persists credential environment variable names and hashed endpoints across rebuild attempts.
Impact: If an attacker gains read access to the state directory, they learn which env vars hold credentials and can infer endpoint structure from fingerprints. Not a direct credential leak but expands attack surface.
Recommended action: Consider redacting credentialEnv in the journal (store only a hash or boolean flag), or document that the state directory must be protected at the OS level. The current 0600/0700 permissions are a reasonable baseline.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/state/rebuild-transaction.ts:200-250 (RebuildTransactionIntentV1) — verify credentialEnv and endpointFingerprint are stored in the intent.target and intent.source sections.
Missing regression test: Add a test that verifies transaction file permissions are 0600 and directory is 0700.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/state/rebuild-transaction.ts:200-250 (RebuildTransactionIntentV1) — verify credentialEnv and endpointFingerprint are stored in the intent.target and intent.source sections.
Evidence: RebuildTransactionIntentV1 at lines 200-250 includes credentialEnv in target and endpointFingerprint in target. Files created with 0600 mode in durablePublish.

`PRA-9` Resolve/justify — Partial MCP restore on restoreMcpBridgesAfterDestroyAbort failure leaves inconsistent state

Location: src/lib/actions/sandbox/rebuild-mcp-phase.ts:30
Category: correctness
Problem: preflightMcpRebuildState handles MCP destroy recovery by calling prepareMcpBridgesForDestroy and restoreMcpBridgesAfterDestroyAbort. If restoreMcpBridgesAfterDestroyAbort fails partway through, the sandbox may be left in an inconsistent state (some providers restored, some not). The function catches errors and calls bail(), but the MCP state may be partially restored.
Impact: Sandbox MCP state could be partially restored after a failed recovery attempt, leaving bridges/providers in an undefined state that may require manual intervention.
Recommended action: Add a test that simulates partial failure in restoreMcpBridgesAfterDestroyAbort and verifies the sandbox is left in a recoverable state (or that the rebuild fails closed with clear guidance).
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/actions/sandbox/rebuild-mcp-phase.ts:30-80 — verify error handling in preflightMcpRebuildState and whether partial restore is rolled back.
Missing regression test: Add a test that mocks restoreMcpBridgesAfterDestroyAbort to fail partway and verifies the rebuild fails closed with actionable guidance.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/actions/sandbox/rebuild-mcp-phase.ts:30-80 — verify error handling in preflightMcpRebuildState and whether partial restore is rolled back.
Evidence: preflightMcpRebuildState at lines 30-65 calls restoreMcpBridgesAfterDestroyAbort in a try/catch but does not attempt to roll back partial provider restoration on error.

`PRA-10` Resolve/justify — Stale transaction journal not cleaned up on fingerprint mismatch

Location: src/lib/actions/sandbox/rebuild-preflight-phase.ts:100
Category: correctness
Problem: runRebuildPreflightPhase calls loadRebuildRecovery which validates the transaction journal against the current registry and latest backup. If validation fails (fingerprint mismatch), it prints a failure and returns null. However, the function does not clean up a stale transaction journal that no longer matches reality — it leaves the corrupt journal on disk. A subsequent rebuild attempt would hit the same validation failure.
Impact: User gets repeated 'Rebuild transaction recovery failed' errors with no automatic recovery path. Manual journal deletion required but not guided.
Recommended action: Either add automatic cleanup of invalid transaction journals (with user confirmation), or document that manual cleanup is required and add a CLI command for it. At minimum, improve error message to include journal path and cleanup guidance.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/actions/sandbox/rebuild-preflight-phase.ts:50-80 — verify loadRebuildRecovery error handling and whether stale journals are cleaned up or guidance provided.
Missing regression test: Add a test that creates a stale transaction journal (mismatched fingerprint) and verifies the rebuild fails with clear guidance on how to clean up (journal path, rm command).
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/actions/sandbox/rebuild-preflight-phase.ts:50-80 — verify loadRebuildRecovery error handling and whether stale journals are cleaned up or guidance provided.
Evidence: loadRebuildRecovery at rebuild-transaction-coordinator.ts:50-70 throws on fingerprint mismatch but does not delete the journal file. rebuild-preflight-phase.ts:100-110 catches and bails without cleanup.

`PRA-11` Resolve/justify — Process-death recovery tests only run on Linux/Unix — no Windows support

Location: test/rebuild-transaction-process-death.test.ts:1
Category: tests
Problem: The new test/rebuild-transaction-process-death.test.ts spawns child processes and uses SIGKILL/SIGSTOP, which are Unix-only. The rebuild-e2e-interruption.ts hook uses process.kill(process.pid, 'SIGSTOP') which is not portable to Windows. No platform guards or conditional test execution.
Impact: Test suite cannot validate process-death recovery on Windows. Windows CI would either skip these tests (gap) or crash on SIGSTOP.
Recommended action: Add a platform check (process.platform !== 'win32') and skip the interruption on Windows in rebuild-e2e-interruption.ts. Add test annotations (e.g., @vitest-environment node, skip on Windows) or implement cross-platform PID recycle simulation for macOS in the test fixture.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Check test/rebuild-transaction-process-death.test.ts for platform conditionals. Check src/lib/actions/sandbox/rebuild-e2e-interruption.ts for platform guards.
Missing regression test: Add a test annotation (e.g., skip on Windows) or implement cross-platform PID recycle simulation. Add a test that verifies the hook is inert on Windows.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Check test/rebuild-transaction-process-death.test.ts for platform conditionals. Check src/lib/actions/sandbox/rebuild-e2e-interruption.ts for platform guards.
Evidence: test/rebuild-transaction-process-death.test.ts uses spawn, SIGKILL, /proc for PID detection. rebuild-e2e-interruption.ts:12 calls process.kill(process.pid, 'SIGSTOP') without platform check.

`PRA-12` Resolve/justify — PID recycle detection falls back to age-only on non-Linux

Location: src/lib/state/registry.ts:145
Category: security
Problem: classifyExistingLock PID recycle detection uses /proc/<pid>/stat to detect PID reuse; on macOS/Windows readProcessStartMs returns null and the lock relies solely on age-based staleness (LOCK_STALE_MS = 10s). A fast PID recycle on non-Linux could cause a stale lock to be incorrectly treated as valid.
Impact: On macOS/Windows, a crashed process holding the lock could have its PID recycled within 10 seconds, causing a new process to incorrectly treat the stale lock as valid and wait instead of reclaiming it, or vice versa.
Recommended action: Document the macOS/Windows limitation clearly. Consider using a lock file with embedded start timestamp (already done) plus a secondary identity marker (e.g., random token written at lock acquisition) to detect PID recycle without /proc.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/state/registry.ts:145-180 — verify the PID recycle detection logic and fallback behavior in classifyExistingLock and readProcessStartMs.
Missing regression test: Add a test that simulates PID recycle on non-Linux (mock readProcessStartMs to return null) and verifies the lock behavior with secondary identity marker.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/state/registry.ts:145-180 — verify the PID recycle detection logic and fallback behavior in classifyExistingLock and readProcessStartMs.
Evidence: readProcessStartMs at line 145 reads /proc/<pid>/stat and /proc/stat. Returns null on error (non-Linux). classifyExistingLock falls back to age-only check when start time unavailable.

`PRA-13` Resolve/justify — Validation tests lack fingerprint/timestamp pattern edge cases

Location: src/lib/state/rebuild-transaction-validation.test.ts:1
Category: tests
Problem: Validation tests have parameterized tests for some timestamp shapes but do not cover: fingerprint boundaries (63/64/65 hex chars), timestamps with second vs millisecond precision, backup timestamp format variations (BACKUP_TIMESTAMP_PATTERN expects HH-MM-SS-mmm).
Impact: Edge cases in fingerprint/timestamp validation may accept invalid formats or reject valid ones. No regression coverage for boundary conditions.
Recommended action: Add parameterized tests for fingerprint/timestamp pattern boundaries: 63/64/65 hex chars, timestamps with second/millisecond precision, backup timestamp format variations (HH-MM-SS-mmm vs HH:MM:SS.mmm).
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/state/rebuild-transaction-validation.test.ts — check for parameterized tests covering pattern boundaries.
Missing regression test: Add parameterized tests for: fingerprint length boundaries (63/64/65 chars), ISO timestamp with/without milliseconds, backup timestamp format (HH-MM-SS-mmm vs HH:MM:SS.mmm).
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/state/rebuild-transaction-validation.test.ts — check for parameterized tests covering pattern boundaries.
Evidence: rebuild-transaction-validation.test.ts has timestamp shape tests but no fingerprint length boundary tests. BACKUP_TIMESTAMP_PATTERN at rebuild-transaction.ts:15 expects HH-MM-SS-mmm format.

`PRA-14` Resolve/justify — maybePauseForRebuildInterruption uses SIGSTOP without Windows guard

Location: src/lib/actions/sandbox/rebuild-e2e-interruption.ts:12
Category: scope
Problem: maybePauseForRebuildInterruption uses process.kill(process.pid, 'SIGSTOP') which is not portable to Windows. The function is testing-only (guarded by VITEST=1 and NEMOCLAW_E2E_FAILURE_INJECTION=1) but the lack of platform guard means it would crash on Windows if the test fixture were ever run there.
Impact: Process crash on Windows if test fixture environment variables are accidentally set. Low risk in practice but violates cross-platform compatibility.
Recommended action: Add a platform check (process.platform !== 'win32') and skip the interruption on Windows, or document that process-death tests are Unix-only.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/actions/sandbox/rebuild-e2e-interruption.ts:1-15 — verify the SIGSTOP call and lack of platform guard.
Missing regression test: Add a test that verifies the hook is inert on Windows (or document Unix-only scope).
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/actions/sandbox/rebuild-e2e-interruption.ts:1-15 — verify the SIGSTOP call and lack of platform guard.
Evidence: Line 12: process.kill(process.pid, 'SIGSTOP') with no process.platform check.

`PRA-15` Resolve/justify — rebuild-transaction.ts monolith growth (922 lines) — extract validation module

Location: src/lib/state/rebuild-transaction.ts:922
Category: architecture
Problem: rebuild-transaction.ts grew by 82 lines (now 922 lines), exceeding the monolith growth threshold. The file contains transaction record types, validation, durable publication, and the RebuildTransactionStore class.
Impact: Maintainability risk. Validation logic is coupled with storage logic. Harder to test validation independently.
Recommended action: Extract validation functions (normalizeRecord, normalizeIntent, normalizeReceipts, normalizeFailure, fingerprint/timestamp validators) to a separate rebuild-transaction-validation.ts module to reduce monolith size.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Check file line count: src/lib/state/rebuild-transaction.ts is 922 lines. Validation functions span lines 200-450 approximately.
Missing regression test: Not applicable — refactoring suggestion. Existing validation tests would cover extracted module.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Check file line count: src/lib/state/rebuild-transaction.ts is 922 lines. Validation functions span lines 200-450 approximately.
Evidence: File grew from 840 to 922 lines. Validation functions (normalizeRecord, normalizeIntent, normalizeReceipts, normalizeFailure, fingerprint, timestamp, backupManifestTimestamp) are 200+ lines.

`PRA-16` Resolve/justify — registry.ts monolith growth (775 lines) — extract rebuild recovery functions

Location: src/lib/state/registry.ts:775
Category: architecture
Problem: registry.ts grew by 62 lines (now 775 lines), exceeding the monolith growth threshold. The file added restoreRebuildRegistryRecoveryIfMissing and related functions.
Impact: Maintainability risk. Rebuild-specific recovery logic mixed with core registry operations.
Recommended action: Extract rebuild-related registry functions (restoreRebuildRegistryRecoveryIfMissing, restorePreservedSandboxEntryIfMissing) to a rebuild-registry-recovery.ts module.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Check file line count: src/lib/state/registry.ts is 775 lines. Rebuild recovery functions at lines 650-690.
Missing regression test: Not applicable — refactoring suggestion. Existing registry tests cover these functions.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Check file line count: src/lib/state/registry.ts is 775 lines. Rebuild recovery functions at lines 650-690.
Evidence: restoreRebuildRegistryRecoveryIfMissing (lines 650-690) and restorePreservedSandboxEntryIfMissing (lines 615-635) are rebuild-specific recovery logic in core registry module.

`PRA-17` Improvement — transactionFileStem uses plain SHA-256 — consider keyed hash (HMAC-SHA256)

Location: src/lib/state/rebuild-transaction.ts:145
Category: architecture
Problem: transactionFileStem uses SHA-256 of sandbox name for filename generation. Collision resistance is adequate but could use keyed hash (HMAC-SHA256) with a fixed per-installation secret (derived from state directory) to prevent intentional collision attacks.
Impact: Low — defense-in-depth. Intentional collision would require attacker to control sandbox name and know the hash algorithm.
Suggested action: Consider using HMAC-SHA256 with a fixed per-installation secret (derived from state directory) for filename generation, or document why plain SHA-256 is acceptable.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Read src/lib/state/rebuild-transaction.ts:145 — verify transactionFileStem implementation.
Missing regression test: Not applicable — defense-in-depth improvement.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: transactionFileStem at line 145: crypto.createHash('sha256').update(sandboxName).digest('hex')

`PRA-18` Improvement — E2E env vars not marked internal-only in allowlist

Location: ci/env-var-doc-allowlist.json:80
Category: docs
Problem: NEMOCLAW_REBUILD_PROCESS_FIXTURE and related E2E env vars (NEMOCLAW_REBUILD_PROCESS_PHASE, NEMOCLAW_REBUILD_PROCESS_STATE_DIR, NEMOCLAW_REBUILD_PROCESS_EVENTS, NEMOCLAW_REBUILD_PROCESS_ROLE) are documented but not marked as internal-only in the allowlist. The prior review (PRA-6) requested an 'internal': true field for vars that are never user-facing.
Impact: Documentation generator may render these as user-configurable. No functional impact but misleading.
Suggested action: Add an 'internal': true field to the allowlist schema for vars that are never user-facing, or ensure the documentation generator renders these distinctly.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Read ci/env-var-doc-allowlist.json — check for 'internal': true on E2E/test-only vars.
Missing regression test: Not applicable — documentation improvement.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: New entry at line 80 for NEMOCLAW_REBUILD_PROCESS_FIXTURE lacks 'internal': true. Other E2E vars in allowlist also lack it.

`PRA-19` Improvement — Verify RebuildTransactionStore.create fsyncs parent directory on first publish

Location: src/lib/state/rebuild-transaction.ts:721
Category: architecture
Problem: RebuildTransactionStore.create() calls durablePublish with createOnly=true when existing===null. durablePublish calls syncDirectory(dirPath) after linkSync, where dirPath is the transaction directory. This may address PRA-11, but verify the first create path also fsyncs the parent transaction directory.
Impact: If parent directory not fsynced on first create, a power loss could lose the transaction directory entry.
Suggested action: Verify that syncDirectory(dirPath) in durablePublish is called for the parent transaction directory on first publish. The create() method calls durablePublish with createOnly=true when existing===null, which should fsync the directory after linkSync.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Read src/lib/state/rebuild-transaction.ts:721 (create method) and 650 (durablePublish) — verify syncDirectory is called for the transaction directory on first create.
Missing regression test: Add a test that verifies directory fsync on first transaction creation (or document that it's covered by existing durablePublish tests).
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: create() at line 721 calls durablePublish(this.path(sandboxName), record, existing === null). durablePublish at line 650 calls syncDirectory(dirPath) after linkSync/renameSync regardless of createOnly.

`PRA-20` Improvement — Verify deleted registry rollback tests covered by new transaction boundary tests

Location: src/lib/actions/sandbox/rebuild-registry-rollback.ts:1
Category: architecture
Problem: rebuild-registry-rollback.ts and its test were deleted. The registry rollback logic is now handled by the transaction journal (RebuildTransactionStore/Coordinator). This is a significant architectural change that replaces a rollback mechanism with a durable transaction log.
Impact: Risk of regression if old rollback scenarios not fully covered by new tests.
Suggested action: Verify that all rollback scenarios from the old rebuild-registry-rollback.test.ts are covered by the new transaction boundary tests. The old tests covered: prepared backup recovery restore, ordinary removal receipt restore, stale recovery with MCP, replacement row preservation, and early no-op handling.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Compare deleted test src/lib/actions/sandbox/rebuild-registry-rollback.test.ts with new tests in src/lib/actions/sandbox/rebuild-transaction-boundary.test.ts and test/rebuild-transaction-process-death.test.ts.
Missing regression test: Ensure all 5 old rollback test scenarios have equivalent coverage in the new transaction boundary tests.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Deleted files: rebuild-registry-rollback.ts (97 lines) and rebuild-registry-rollback.test.ts (174 lines). New transaction boundary tests cover: normal completion, process interruption, resume, registry recovery, MCP destroy recovery, legacy recovery, shields posture.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

github-actions · 2026-07-08T13:50:32Z

E2E Target Recommendation

Required E2E targets: sandbox-rebuild, mcp-bridge, ubuntu-repo-cloud-langchain-deepagents-code
Optional E2E targets: rebuild-openclaw, rebuild-hermes, mcp-bridge-dev

Dispatch required E2E targets:

gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=sandbox-rebuild
gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=mcp-bridge
gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-cloud-langchain-deepagents-code

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/refactor/6434-rebuild-transaction-store
Head: HEAD
Confidence: high

Required E2E targets

sandbox-rebuild: The PR changes the core sandbox rebuild transaction, delete/recreate boundary, registry preservation, and recovery paths. The sandbox-rebuild free-standing job exercises a real OpenClaw onboard followed by nemoclaw <sandbox> rebuild --yes, validating state preservation, registry refresh, and backup hygiene through the live Docker/OpenShell boundary.
- Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=sandbox-rebuild
mcp-bridge: The PR changes MCP rebuild preparation/recovery and registry handling for MCP-bearing rebuilds. The mcp-bridge free-standing job runs the live MCP bridge workflow and includes rebuild-with-provider-backed-MCP coverage, proving MCP bridge credentials/adapters survive rebuild without requiring host MCP secrets.
- Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=mcp-bridge
ubuntu-repo-cloud-langchain-deepagents-code: The PR changes DCode rebuild preflight, delete-edge validation, prepared-image drift handling, and transaction recovery. This live-supported typed target is the smallest registry target that exercises the Deep Agents Code rebuild lifecycle surface.
- Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-cloud-langchain-deepagents-code

Optional E2E targets

rebuild-openclaw: Adjacent OpenClaw rebuild coverage for old-version/seeded registry-session state, gateway token rotation, policy restoration, and backup manifest behavior. Useful because the PR changes durable rebuild transaction and registry recovery semantics beyond the current-version sandbox-rebuild path.
- Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=rebuild-openclaw
rebuild-hermes: Adjacent Hermes rebuild coverage for the same common transaction/delete/recreate pipeline on a different agent and registry/session shape. Recommended as optional because the primary changed surface is already covered by sandbox-rebuild and mcp-bridge.
- Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=rebuild-hermes
mcp-bridge-dev: Optional expanded MCP bridge matrix coverage, including additional agent variants under the explicit dev lane. This is adjacent to the MCP rebuild changes but uses moving OpenShell dev artifacts, so it should remain optional unless maintainers need broader compatibility evidence.
- Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=mcp-bridge-dev

Relevant changed files

src/lib/actions/sandbox/destroy.ts
src/lib/actions/sandbox/mcp-bridge-rebuild.ts
src/lib/actions/sandbox/mcp-bridge.ts
src/lib/actions/sandbox/rebuild-destroy-phase.ts
src/lib/actions/sandbox/rebuild-e2e-interruption.ts
src/lib/actions/sandbox/rebuild-flow-helpers.ts
src/lib/actions/sandbox/rebuild-mcp-phase.ts
src/lib/actions/sandbox/rebuild-pipeline.ts
src/lib/actions/sandbox/rebuild-preflight-phase.ts
src/lib/actions/sandbox/rebuild-prepared-recovery.ts
src/lib/actions/sandbox/rebuild-recreate-phase.ts
src/lib/actions/sandbox/rebuild-registry-rollback.ts
src/lib/actions/sandbox/rebuild-transaction-coordinator.ts
src/lib/state/rebuild-transaction.ts
src/lib/state/registry-reversible-removal.ts
src/lib/state/registry.ts

github-actions · 2026-07-08T13:52:49Z

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Source-of-truth review needed: src/lib/actions/sandbox/rebuild-e2e-interruption.ts.
Open items: 0 required · 3 warnings · 0 suggestions · 5 test follow-ups
Since last review: 0 prior items resolved · 3 still apply · 0 new items found

Action checklist

PRA-1 Resolve or justify: Source-of-truth review needed: src/lib/actions/sandbox/rebuild-e2e-interruption.ts
PRA-2 Resolve or justify: Production rebuild path can still be paused by forgeable environment variables in src/lib/actions/sandbox/rebuild-e2e-interruption.ts:5
PRA-3 Resolve or justify: Rebuild transaction store hotspot keeps growing in src/lib/state/rebuild-transaction.ts
PRA-T1 Add or justify test follow-up: Runtime validation
PRA-T2 Add or justify test follow-up: Runtime validation
PRA-T3 Add or justify test follow-up: Acceptance clause
PRA-T4 Add or justify test follow-up: Acceptance clause
PRA-T5 Add or justify test follow-up: src/lib/actions/sandbox/rebuild-e2e-interruption.ts

Findings index

ID	Severity	Category	Location	Required action
`PRA-1`	Resolve/justify	architecture	—	Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
`PRA-2`	Resolve/justify	security	`src/lib/actions/sandbox/rebuild-e2e-interruption.ts:5`	Move the pause behavior behind a fixture-only injected callback or non-forgeable test capability that ordinary production rebuild imports cannot reach. Keep the SIGKILL/process-death regression coverage, but make environment variables alone insufficient to stop the process. If any production import remains, add the invalid state, source boundary, source-fix constraint, regression proof, and removal condition next to the hook.
`PRA-3`	Resolve/justify	architecture	`src/lib/state/rebuild-transaction.ts`	If practical in this PR, extract one cohesive internal helper such as record normalization/validation or durable file publication/path handling behind a narrow internal API. Preserve all current safety boundaries: sandbox-name validation, 0600 permissions, O_NOFOLLOW reads where available, max-size checks, fsync/link publication semantics, MCP lifecycle lock usage, and revision checks. If extraction is intentionally deferred for coupling reasons, add a concise justification specific to this phase.

Review findings by urgency: 0 required fixes, 3 items to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

`PRA-1` Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/rebuild-e2e-interruption.ts

Location: not file-specific
Category: architecture
Problem: The advisor marked localized patch analysis as needs_followup.
Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Missing regression test: rebuild-e2e-interruption.test.ts covers inertness without the fixture env and selected-phase activation; test/rebuild-transaction-process-death.test.ts uses the hook for SIGKILL recovery.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Evidence: The file contains only a short testing-only comment before env checks and process.kill(process.pid, "SIGSTOP").

`PRA-2` Resolve/justify — Production rebuild path can still be paused by forgeable environment variables

Location: src/lib/actions/sandbox/rebuild-e2e-interruption.ts:5
Category: security
Problem: The interruption helper is imported by production rebuild modules and calls process.kill(process.pid, "SIGSTOP") when ordinary process.env values line up: VITEST=true, NEMOCLAW_REBUILD_PROCESS_FIXTURE equal to HOME, NEMOCLAW_E2E_FAILURE_INJECTION=1, and NEMOCLAW_E2E_FORCE_FAIL_AT_STEP matching the phase. The fixture-root equality reduces accidental activation, but it is still an env-only capability, and the file still only has a short testing-only comment rather than a full source-of-truth explanation for why a production-imported process-stop branch is necessary.
Impact: A local wrapper, inherited test shell, or malicious environment can make a normal CLI rebuild hang at the prepared or delete-adjacent checkpoint. The transaction journal limits data-loss risk, but sandbox lifecycle code should not expose an env-only process pause primitive from production imports.
Recommended action: Move the pause behavior behind a fixture-only injected callback or non-forgeable test capability that ordinary production rebuild imports cannot reach. Keep the SIGKILL/process-death regression coverage, but make environment variables alone insufficient to stop the process. If any production import remains, add the invalid state, source boundary, source-fix constraint, regression proof, and removal condition next to the hook.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/actions/sandbox/rebuild-e2e-interruption.ts and the call sites in src/lib/actions/sandbox/rebuild-pipeline.ts and src/lib/actions/sandbox/rebuild-destroy-phase.ts; confirm whether process.kill(process.pid, "SIGSTOP") can still be reached solely through process.env values.
Missing regression test: Add a negative test named like "production rebuild ignores forged interruption environment without injected fixture capability" that sets VITEST=true, HOME, NEMOCLAW_REBUILD_PROCESS_FIXTURE=HOME, NEMOCLAW_E2E_FAILURE_INJECTION=1, and NEMOCLAW_E2E_FORCE_FAIL_AT_STEP=rebuild_prepared on the normal rebuild path and asserts process.kill is not called unless the fixture-only capability is injected.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/actions/sandbox/rebuild-e2e-interruption.ts and the call sites in src/lib/actions/sandbox/rebuild-pipeline.ts and src/lib/actions/sandbox/rebuild-destroy-phase.ts; confirm whether process.kill(process.pid, "SIGSTOP") can still be reached solely through process.env values.
Evidence: maybePauseForRebuildInterruption() checks only process.env values before writing an E2E marker and sending SIGSTOP. It is called from rebuild-pipeline.ts at the prepared checkpoint and rebuild-destroy-phase.ts at the delete_unjournaled checkpoint. The current tests cover default inertness without the fixture env and activation when those env vars are set.

`PRA-3` Resolve/justify — Rebuild transaction store hotspot keeps growing

Location: src/lib/state/rebuild-transaction.ts
Category: architecture
Problem: src/lib/state/rebuild-transaction.ts remains a large security-critical module and grew to 922 lines in this PR. It still combines schema normalization, strict file loading, durable publication, directory fsync handling, diagnostics, lifecycle locking, revision checks, prepared refresh, state transitions, failure recording, and completion logic. The new coordinator extraction helps, but the store itself remains a broad hotspot in destructive sandbox lifecycle code.
Impact: Future changes to transaction schema, filesystem safety, or state-machine behavior will be harder to review independently, increasing the chance of missing durability, recovery, permission, or race-condition regressions.
Recommended action: If practical in this PR, extract one cohesive internal helper such as record normalization/validation or durable file publication/path handling behind a narrow internal API. Preserve all current safety boundaries: sandbox-name validation, 0600 permissions, O_NOFOLLOW reads where available, max-size checks, fsync/link publication semantics, MCP lifecycle lock usage, and revision checks. If extraction is intentionally deferred for coupling reasons, add a concise justification specific to this phase.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/state/rebuild-transaction.ts and identify whether validation-only or durable-I/O-only code can move behind a narrow helper without changing public behavior; confirm the extracted helper remains covered through src/lib/state/rebuild-transaction-validation.test.ts, src/lib/state/rebuild-transaction-refresh.test.ts, src/lib/actions/sandbox/rebuild-transaction-boundary.test.ts, and test/rebuild-transaction-process-death.test.ts.
Missing regression test: Existing transaction validation, prepared-refresh, boundary, competing-writer, and process-death tests already cover the safety boundaries that must remain after a pure extraction; no new behavior test is needed if the change only moves code behind equivalent helpers.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/state/rebuild-transaction.ts and identify whether validation-only or durable-I/O-only code can move behind a narrow helper without changing public behavior; confirm the extracted helper remains covered through src/lib/state/rebuild-transaction-validation.test.ts, src/lib/state/rebuild-transaction-refresh.test.ts, src/lib/actions/sandbox/rebuild-transaction-boundary.test.ts, and test/rebuild-transaction-process-death.test.ts.
Evidence: The changed file is 922 lines after this PR and inspection shows validation, durable I/O, lock and revision checks, diagnostics, and state transitions still live in the same transaction store module.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

None.

Simplification opportunities: 2 possible cuts

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

PRA-2 shrink (src/lib/actions/sandbox/rebuild-e2e-interruption.ts:5): The production-imported env-gated SIGSTOP branch in rebuild-e2e-interruption.ts.
- Replacement: Inject a test-only interruption callback or capability from the process-death fixture/transaction-store seam, leaving production rebuild phases free of env-only process-stop behavior.
- Safety boundary: Do not weaken SIGKILL/process-death coverage or transaction recovery checks; only remove ordinary production reachability of the pause primitive.
PRA-3 shrink (src/lib/state/rebuild-transaction.ts): One cohesive slice of src/lib/state/rebuild-transaction.ts, preferably record validation/normalization or durable file publication.
- Replacement: Move the slice to a narrow internal helper module while keeping the public RebuildTransactionStore API and all validation/durability semantics unchanged.
- Net: 0 lines
- Safety boundary: Do not simplify away sandbox-name validation, file permission hardening, O_NOFOLLOW reads, max-size checks, atomic publication, fsync/link semantics, lifecycle locking, receipt history checks, or revision conflicts.

Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

PRA-T1 Runtime validation — Add or identify a negative test named "production rebuild ignores forged interruption environment without injected fixture capability" that sets the full interruption env on a normal rebuild path and asserts process.kill is not called.. This PR changes sandbox destructive lifecycle, durable transaction recovery, registry restoration, MCP recovery, and child-process SIGKILL integration paths. Static coverage is broad and behavior-specific, but runtime/integration validation remains important for these infrastructure boundaries.
PRA-T2 Runtime validation — Keep the existing child-process SIGKILL integration coverage for prepared and delete_unjournaled checkpoints as the runtime proof that fresh-process recovery does not share in-memory state or delete twice.. This PR changes sandbox destructive lifecycle, durable transaction recovery, registry restoration, MCP recovery, and child-process SIGKILL integration paths. Static coverage is broad and behavior-specific, but runtime/integration validation remains important for these infrastructure boundaries.
PRA-T3 Acceptance clause — Existing rebuild-flow, prepared-recovery, registry-rollback, MCP, shields, and DCode regression suites remain green. — add test evidence or identify existing coverage. Static inventory shows the relevant suites were updated or replaced by transaction/registry tests, but this advisor does not evaluate external CI pass/fail.
PRA-T4 Acceptance clause — Targeted `cli`/`integration` tests, `npm run typecheck:cli`, repository checks, and coverage ratchet pass. — add test evidence or identify existing coverage. Runtime command results are outside this advisor review surface; static review found targeted integration and component tests but did not execute commands.
PRA-T5 src/lib/actions/sandbox/rebuild-e2e-interruption.ts — rebuild-e2e-interruption.test.ts covers inertness without the fixture env and selected-phase activation; test/rebuild-transaction-process-death.test.ts uses the hook for SIGKILL recovery.. The file contains only a short testing-only comment before env checks and process.kill(process.pid, "SIGSTOP").

Since last review details

Current findings, using the urgency labels above:

`PRA-1` Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/rebuild-e2e-interruption.ts

Location: not file-specific
Category: architecture
Problem: The advisor marked localized patch analysis as needs_followup.
Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Missing regression test: rebuild-e2e-interruption.test.ts covers inertness without the fixture env and selected-phase activation; test/rebuild-transaction-process-death.test.ts uses the hook for SIGKILL recovery.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
Evidence: The file contains only a short testing-only comment before env checks and process.kill(process.pid, "SIGSTOP").

`PRA-2` Resolve/justify — Production rebuild path can still be paused by forgeable environment variables

Location: src/lib/actions/sandbox/rebuild-e2e-interruption.ts:5
Category: security
Problem: The interruption helper is imported by production rebuild modules and calls process.kill(process.pid, "SIGSTOP") when ordinary process.env values line up: VITEST=true, NEMOCLAW_REBUILD_PROCESS_FIXTURE equal to HOME, NEMOCLAW_E2E_FAILURE_INJECTION=1, and NEMOCLAW_E2E_FORCE_FAIL_AT_STEP matching the phase. The fixture-root equality reduces accidental activation, but it is still an env-only capability, and the file still only has a short testing-only comment rather than a full source-of-truth explanation for why a production-imported process-stop branch is necessary.
Impact: A local wrapper, inherited test shell, or malicious environment can make a normal CLI rebuild hang at the prepared or delete-adjacent checkpoint. The transaction journal limits data-loss risk, but sandbox lifecycle code should not expose an env-only process pause primitive from production imports.
Recommended action: Move the pause behavior behind a fixture-only injected callback or non-forgeable test capability that ordinary production rebuild imports cannot reach. Keep the SIGKILL/process-death regression coverage, but make environment variables alone insufficient to stop the process. If any production import remains, add the invalid state, source boundary, source-fix constraint, regression proof, and removal condition next to the hook.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/actions/sandbox/rebuild-e2e-interruption.ts and the call sites in src/lib/actions/sandbox/rebuild-pipeline.ts and src/lib/actions/sandbox/rebuild-destroy-phase.ts; confirm whether process.kill(process.pid, "SIGSTOP") can still be reached solely through process.env values.
Missing regression test: Add a negative test named like "production rebuild ignores forged interruption environment without injected fixture capability" that sets VITEST=true, HOME, NEMOCLAW_REBUILD_PROCESS_FIXTURE=HOME, NEMOCLAW_E2E_FAILURE_INJECTION=1, and NEMOCLAW_E2E_FORCE_FAIL_AT_STEP=rebuild_prepared on the normal rebuild path and asserts process.kill is not called unless the fixture-only capability is injected.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/actions/sandbox/rebuild-e2e-interruption.ts and the call sites in src/lib/actions/sandbox/rebuild-pipeline.ts and src/lib/actions/sandbox/rebuild-destroy-phase.ts; confirm whether process.kill(process.pid, "SIGSTOP") can still be reached solely through process.env values.
Evidence: maybePauseForRebuildInterruption() checks only process.env values before writing an E2E marker and sending SIGSTOP. It is called from rebuild-pipeline.ts at the prepared checkpoint and rebuild-destroy-phase.ts at the delete_unjournaled checkpoint. The current tests cover default inertness without the fixture env and activation when those env vars are set.

`PRA-3` Resolve/justify — Rebuild transaction store hotspot keeps growing

Location: src/lib/state/rebuild-transaction.ts
Category: architecture
Problem: src/lib/state/rebuild-transaction.ts remains a large security-critical module and grew to 922 lines in this PR. It still combines schema normalization, strict file loading, durable publication, directory fsync handling, diagnostics, lifecycle locking, revision checks, prepared refresh, state transitions, failure recording, and completion logic. The new coordinator extraction helps, but the store itself remains a broad hotspot in destructive sandbox lifecycle code.
Impact: Future changes to transaction schema, filesystem safety, or state-machine behavior will be harder to review independently, increasing the chance of missing durability, recovery, permission, or race-condition regressions.
Recommended action: If practical in this PR, extract one cohesive internal helper such as record normalization/validation or durable file publication/path handling behind a narrow internal API. Preserve all current safety boundaries: sandbox-name validation, 0600 permissions, O_NOFOLLOW reads where available, max-size checks, fsync/link publication semantics, MCP lifecycle lock usage, and revision checks. If extraction is intentionally deferred for coupling reasons, add a concise justification specific to this phase.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read src/lib/state/rebuild-transaction.ts and identify whether validation-only or durable-I/O-only code can move behind a narrow helper without changing public behavior; confirm the extracted helper remains covered through src/lib/state/rebuild-transaction-validation.test.ts, src/lib/state/rebuild-transaction-refresh.test.ts, src/lib/actions/sandbox/rebuild-transaction-boundary.test.ts, and test/rebuild-transaction-process-death.test.ts.
Missing regression test: Existing transaction validation, prepared-refresh, boundary, competing-writer, and process-death tests already cover the safety boundaries that must remain after a pure extraction; no new behavior test is needed if the change only moves code behind equivalent helpers.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read src/lib/state/rebuild-transaction.ts and identify whether validation-only or durable-I/O-only code can move behind a narrow helper without changing public behavior; confirm the extracted helper remains covered through src/lib/state/rebuild-transaction-validation.test.ts, src/lib/state/rebuild-transaction-refresh.test.ts, src/lib/actions/sandbox/rebuild-transaction-boundary.test.ts, and test/rebuild-transaction-process-death.test.ts.
Evidence: The changed file is 922 lines after this PR and inspection shows validation, durable I/O, lock and revision checks, diagnostics, and state transitions still live in the same transaction store module.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

…6435-rebuild-destructive-journal # Conflicts: # src/lib/state/rebuild-transaction.test.ts # src/lib/state/rebuild-transaction.ts

…6435-rebuild-destructive-journal

…6435-rebuild-destructive-journal # Conflicts: # src/lib/state/rebuild-transaction.ts

…6435-rebuild-destructive-journal

jyaunches · 2026-07-08T17:05:21Z

Advisor disposition at `436d68650`

The substantive GPT findings are implemented: legacy managed-image recovery authority is durable across processes; the private process-death hook requires a fixture root equal to temporary HOME; replacement_created crash recovery is exercised end to end; mutation inputs are classified separately from corrupt state; and custom transaction roots enforce the host-state permission/symlink boundary.

Remaining Nemotron findings are resolved/overridden as follows:

Create TOCTOU: every create() call enters the store's private withMutationLock, which acquires the per-sandbox filesystem lifecycle lock. Active-generation checks and completed-generation replacement occur inside that lock. Initial publication still uses atomic no-replace link; the real process race exercises the same lock primitive. The report's statement that the class does not enforce locking is incorrect.
replacement_created regression: fails closed with recovery guidance after replacement creation first leaves the durable record at replacement_created, then invokes a fresh rebuild coordinator with the same store. That second invocation calls loadRebuildRecovery, asserts the generic preflight failure, asserts the detailed snapshot-restore guidance printed to the user, and proves backup/delete are not called. This is the requested path, not merely a mocked unit branch.
Store size: refactor(rebuild): journal destructive recovery #6470 inherits the V1 store from refactor(rebuild): add durable transaction store #6468 and adds only the prepared-refresh generation behavior. Schema-validation, concurrency, refresh, and lifecycle/security tests are now split; pipeline transaction coordination is already extracted. Moving the public contract/error types plus validation and durable I/O again in this stacked destructive-boundary PR would enlarge its review surface and create circular-runtime risk. Helper extraction remains a bounded follow-up once V2 or a second consumer establishes the stable module boundary.
Backup-before-journal: the ordering is intentional and commented at the call site: an immutable orphan backup is harmless, while a journal without recovery data is unsafe. Backup failure and prepare/retry behavior are covered.
Registry/MCP behavior: focused regressions cover replacement-row preservation, non-MCP recreate failure, exact-ownership MCP rollback, and ambiguous MCP fail-closed behavior. registry.ts does not import RebuildTransactionStore; the reported dependency cycle is not present.
Process-death CI: the tests are in the repository's Vitest integration project and pass as real child-process SIGKILL tests at both checkpoints. The production hook is inert without the test-only fixture capability.

Focused verification: 66 CLI tests plus three real-process tests passed before the final inherited root-boundary change; the affected store suite now passes 38/38. npm run check:diff and CLI typecheck pass. The PR remains draft.

…6435-rebuild-destructive-journal

jyaunches · 2026-07-08T17:36:11Z

Advisor resolution at `cc6858a`

The GPT required finding about missing durable registry/default ownership is implemented:

the transaction intent now contains the original normalized registry row, whether it owned the default, and the captured default-selection revision;
fresh recovery verifies the capsule fingerprint, atomically restores only a missing row, and fails closed when a replacement owns the name;
default ownership is reclaimed only when the current pointer is invalid and its revision still equals the captured revision, so a later explicit default remains authoritative;
boundary coverage proves missing-row recovery and replacement-row refusal; registry coverage proves changed-default preservation and exact-revision reclamation.

The remaining interruption-hook warning is explicitly bounded: it requires all shared E2E gates, a Vitest process, and a fixture marker equal to the child temporary HOME. Forging all variables can only suspend the invoking user process; it cannot signal another process or cross a privilege boundary. The two inert-by-default tests and two real child-process SIGKILL tests protect the boundary. Replace this localized hook when the coordinator exposes a test-only phase observer that can drive a real child process without production environment plumbing.

The store module extraction remains a follow-up after this V1 contract settles; validation/concurrency tests and transaction coordination are already separated without moving the audited publication and schema boundary during this safety change. Both PRs remain draft.

jyaunches · 2026-07-08T17:44:01Z

Follow-up Advisor fix at `cbdca27`

The exact-head GPT warning about recreate failure handling is fixed. Errors from the failure-metadata callback are now redacted and contained, so session failure marking, registry and MCP retry recovery, backup location output, retry commands, shields guidance, and the final bail still run. A destructive-boundary regression forces the journal write to fail and proves the transaction remains active at old_deleted while backup guidance is still emitted. The PR remains draft.

jyaunches · 2026-07-08T17:57:23Z

Exact-head advisor disposition at `04089ed`

GPT reports zero required findings and zero new items. Its remaining interruption-hook and store-extraction warnings are covered by the explicit boundary/removal rationale already posted.

Nemotron newly labels MCP revalidation as required, but its premise is incorrect. rebuildSandbox holds the per-sandbox cross-process MCP lifecycle lock from before preflight through the delete edge, and every NemoClaw MCP add, remove, restart, destroy, and rebuild path acquires that same lock. In addition, revalidatePreparedRecoveryBeforeDelete deep-compares the complete registry entry, including its MCP markers. The actual provider detach/ownership preparation occurs later inside the locked destroy phase, immediately before deletion; preflightMcpRebuildState only resolves or blocks an already-incomplete destroy transaction. Re-running a second preparation inside prepared-backup validation would duplicate a later phase and widen side effects before the final delete checks. This item is therefore explicitly overridden with the existing lock and registry-drift regressions as proof.

The CI shard failure on the preceding head was a stale expectation for the retired registry rewrite call. The final test-only commit now asserts the guarded preserved-row path and unchanged default-selection revision. Both PRs remain draft.

jyaunches · 2026-07-10T02:05:49Z

Closing this draft stack as requested while the broader human-and-agent lifecycle architecture is reconsidered.

jyaunches added 5 commits July 8, 2026 09:36

refactor(rebuild): journal destructive recovery

16cd46e

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

fix(rebuild): block incomplete MCP teardown early

cfd12f3

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

test(rebuild): assert registry preservation

ace1be3

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

merge: sync rebuild transaction foundation

05dd89b

fix(test): type rebuild process fixtures

092e298

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

jyaunches added area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery area: architecture Architecture, design debt, major refactors, or maintainability labels Jul 8, 2026

jyaunches self-assigned this Jul 8, 2026

test(rebuild): keep process recovery cases linear

26f07e8

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>

jyaunches added 7 commits July 8, 2026 11:53

Merge branch 'refactor/6434-rebuild-transaction-store' into refactor/…

1a225e3

…6435-rebuild-destructive-journal # Conflicts: # src/lib/state/rebuild-transaction.test.ts # src/lib/state/rebuild-transaction.ts

fix(rebuild): harden transaction recovery

84c39ae

Merge branch 'refactor/6434-rebuild-transaction-store' into refactor/…

3ffe737

…6435-rebuild-destructive-journal

Merge branch 'refactor/6434-rebuild-transaction-store' into refactor/…

851d774

…6435-rebuild-destructive-journal

Merge branch 'refactor/6434-rebuild-transaction-store' into refactor/…

9c43c42

…6435-rebuild-destructive-journal

test(rebuild): isolate prepared refresh coverage

9eb08a6

Merge branch 'refactor/6434-rebuild-transaction-store' into refactor/…

b918b8b

…6435-rebuild-destructive-journal

jyaunches mentioned this pull request Jul 8, 2026

refactor(rebuild): add durable transaction store #6468

Closed

12 tasks

jyaunches added 6 commits July 8, 2026 12:35

Merge branch 'refactor/6434-rebuild-transaction-store' into refactor/…

baaa72c

…6435-rebuild-destructive-journal

Merge branch 'refactor/6434-rebuild-transaction-store' into refactor/…

b35cab2

…6435-rebuild-destructive-journal

fix(rebuild): persist legacy recovery authority

f2f2c45

test(rebuild): isolate process-death hook

7f4b09c

Merge branch 'refactor/6434-rebuild-transaction-store' into refactor/…

3235d55

…6435-rebuild-destructive-journal # Conflicts: # src/lib/state/rebuild-transaction.ts

Merge branch 'refactor/6434-rebuild-transaction-store' into refactor/…

436d686

…6435-rebuild-destructive-journal

jyaunches added 4 commits July 8, 2026 13:13

Merge branch 'refactor/6434-rebuild-transaction-store' into refactor/…

a8c538a

…6435-rebuild-destructive-journal

fix(rebuild): preserve original shields posture

0f36eda

docs(rebuild): record MCP recovery invariant

b188969

fix(rebuild): persist registry recovery ownership

cc6858a

fix(rebuild): preserve recreate failure guidance

cbdca27

test(rebuild): assert preserved registry recovery

04089ed

test(rebuild): provide locked registry snapshot

3fb3cbc

This was referenced Jul 9, 2026

refactor(rebuild): resume transaction-owned replacements #6522

Closed

refactor(rebuild): collapse recovery planning layers #6585

Closed

jyaunches closed this Jul 10, 2026

jyaunches deleted the refactor/6435-rebuild-destructive-journal branch July 10, 2026 02:05

Uh oh!

Conversation

jyaunches commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Stacked PR

Summary

Architectural simplification

Advisor resolution

Tests

Line budget

Uh oh!

copy-pr-bot Bot commented Jul 8, 2026

Uh oh!

coderabbitai Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-code-quality Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Overview

TypeScript / code-coverage/plugin

TypeScript / code-coverage/cli

Uh oh!

github-actions Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Advisor Recommendation

E2E Recommendation Advisor

Required E2E

Optional E2E

New E2E recommendations

Dispatch hint

Uh oh!

github-actions Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Advisor (Nemotron Ultra) — Changes requested

Action checklist

Findings index

🚨 Required before merge

PRA-5 Required — revalidatePreparedRecoveryBeforeDelete does not re-validate MCP state at delete edge

PRA-6 Required — durablePublish throws on EXDEV instead of cross-filesystem atomic write fallback

PRA-7 Required — syncDirectory lacks F_FULLFSYNC on macOS — torn directory entry on power loss

⚠️ Resolve or justify before merge

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/state/rebuild-transaction.ts:532

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/state/rebuild-transaction.ts:570

PRA-3 Resolve/justify — Source-of-truth review needed: src/lib/state/registry.ts:145

PRA-4 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/rebuild-preflight-phase.ts:98

PRA-8 Resolve/justify — Transaction journal stores credentialEnv and endpointFingerprint — expanded attack surface

PRA-9 Resolve/justify — Partial MCP restore on restoreMcpBridgesAfterDestroyAbort failure leaves inconsistent state

PRA-10 Resolve/justify — Stale transaction journal not cleaned up on fingerprint mismatch

PRA-11 Resolve/justify — Process-death recovery tests only run on Linux/Unix — no Windows support

PRA-12 Resolve/justify — PID recycle detection falls back to age-only on non-Linux

PRA-13 Resolve/justify — Validation tests lack fingerprint/timestamp pattern edge cases

PRA-14 Resolve/justify — maybePauseForRebuildInterruption uses SIGSTOP without Windows guard

PRA-15 Resolve/justify — rebuild-transaction.ts monolith growth (922 lines) — extract validation module

PRA-16 Resolve/justify — registry.ts monolith growth (775 lines) — extract rebuild recovery functions

PRA-22 Resolve/justify — replacement_created phase blocks automatic resume — user must intervene

PRA-23 Resolve/justify — MCP lifecycle lock uses advisory file lock — not mandatory

PRA-25 Resolve/justify — Forced local cleanup preserves registry entry with MCP ownership when gateway unreachable

PRA-26 Resolve/justify — Legacy managed-image recovery authority persisted from prior transaction

PRA-29 Resolve/justify — Add fingerprint length boundary tests (63/64/65 hex chars)

PRA-30 Resolve/justify — Add backup timestamp format variation tests

💡 In-scope improvements

PRA-17 Improvement — transactionFileStem uses plain SHA-256 — consider keyed hash (HMAC-SHA256)

PRA-18 Improvement — E2E env vars not marked internal-only in allowlist

PRA-19 Improvement — Verify RebuildTransactionStore.create fsyncs parent directory on first publish

PRA-20 Improvement — Verify deleted registry rollback tests covered by new transaction boundary tests

PRA-21 Improvement — Test infrastructure growth adds maintenance burden

PRA-24 Improvement — Add CI annotation for macOS durability gap

PRA-27 Improvement — MCP destroy markers as recovery source of truth — SOURCE_OF_TRUTH documented

PRA-28 Improvement — Test-only interruption hook — acceptable as-is

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/state/rebuild-transaction.ts:532

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/state/rebuild-transaction.ts:570

PRA-3 Resolve/justify — Source-of-truth review needed: src/lib/state/registry.ts:145

PRA-4 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/rebuild-preflight-phase.ts:98

PRA-5 Required — revalidatePreparedRecoveryBeforeDelete does not re-validate MCP state at delete edge

PRA-6 Required — durablePublish throws on EXDEV instead of cross-filesystem atomic write fallback

PRA-7 Required — syncDirectory lacks F_FULLFSYNC on macOS — torn directory entry on power loss

PRA-8 Resolve/justify — Transaction journal stores credentialEnv and endpointFingerprint — expanded attack surface

PRA-9 Resolve/justify — Partial MCP restore on restoreMcpBridgesAfterDestroyAbort failure leaves inconsistent state

PRA-10 Resolve/justify — Stale transaction journal not cleaned up on fingerprint mismatch

jyaunches commented Jul 8, 2026 •

edited

Loading

coderabbitai Bot commented Jul 8, 2026 •

edited

Loading

github-code-quality Bot commented Jul 8, 2026 •

edited

Loading

github-actions Bot commented Jul 8, 2026 •

edited

Loading

github-actions Bot commented Jul 8, 2026 •

edited

Loading

`PRA-5` Required — revalidatePreparedRecoveryBeforeDelete does not re-validate MCP state at delete edge

`PRA-6` Required — durablePublish throws on EXDEV instead of cross-filesystem atomic write fallback

`PRA-7` Required — syncDirectory lacks F_FULLFSYNC on macOS — torn directory entry on power loss

`PRA-1` Resolve/justify — Source-of-truth review needed: src/lib/state/rebuild-transaction.ts:532

`PRA-2` Resolve/justify — Source-of-truth review needed: src/lib/state/rebuild-transaction.ts:570

`PRA-3` Resolve/justify — Source-of-truth review needed: src/lib/state/registry.ts:145

`PRA-4` Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/rebuild-preflight-phase.ts:98

`PRA-8` Resolve/justify — Transaction journal stores credentialEnv and endpointFingerprint — expanded attack surface

`PRA-9` Resolve/justify — Partial MCP restore on restoreMcpBridgesAfterDestroyAbort failure leaves inconsistent state

`PRA-10` Resolve/justify — Stale transaction journal not cleaned up on fingerprint mismatch

`PRA-11` Resolve/justify — Process-death recovery tests only run on Linux/Unix — no Windows support

`PRA-12` Resolve/justify — PID recycle detection falls back to age-only on non-Linux

`PRA-13` Resolve/justify — Validation tests lack fingerprint/timestamp pattern edge cases

`PRA-14` Resolve/justify — maybePauseForRebuildInterruption uses SIGSTOP without Windows guard

`PRA-15` Resolve/justify — rebuild-transaction.ts monolith growth (922 lines) — extract validation module

`PRA-16` Resolve/justify — registry.ts monolith growth (775 lines) — extract rebuild recovery functions

`PRA-22` Resolve/justify — replacement_created phase blocks automatic resume — user must intervene

`PRA-23` Resolve/justify — MCP lifecycle lock uses advisory file lock — not mandatory

`PRA-25` Resolve/justify — Forced local cleanup preserves registry entry with MCP ownership when gateway unreachable

`PRA-26` Resolve/justify — Legacy managed-image recovery authority persisted from prior transaction

`PRA-29` Resolve/justify — Add fingerprint length boundary tests (63/64/65 hex chars)

`PRA-30` Resolve/justify — Add backup timestamp format variation tests

`PRA-17` Improvement — transactionFileStem uses plain SHA-256 — consider keyed hash (HMAC-SHA256)

`PRA-18` Improvement — E2E env vars not marked internal-only in allowlist

`PRA-19` Improvement — Verify RebuildTransactionStore.create fsyncs parent directory on first publish

`PRA-20` Improvement — Verify deleted registry rollback tests covered by new transaction boundary tests

`PRA-21` Improvement — Test infrastructure growth adds maintenance burden

`PRA-24` Improvement — Add CI annotation for macOS durability gap

`PRA-27` Improvement — MCP destroy markers as recovery source of truth — SOURCE_OF_TRUTH documented

`PRA-28` Improvement — Test-only interruption hook — acceptable as-is

`PRA-1` Resolve/justify — Source-of-truth review needed: src/lib/state/rebuild-transaction.ts:532

`PRA-2` Resolve/justify — Source-of-truth review needed: src/lib/state/rebuild-transaction.ts:570

`PRA-3` Resolve/justify — Source-of-truth review needed: src/lib/state/registry.ts:145

`PRA-4` Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/rebuild-preflight-phase.ts:98

`PRA-5` Required — revalidatePreparedRecoveryBeforeDelete does not re-validate MCP state at delete edge

`PRA-6` Required — durablePublish throws on EXDEV instead of cross-filesystem atomic write fallback

`PRA-7` Required — syncDirectory lacks F_FULLFSYNC on macOS — torn directory entry on power loss

`PRA-8` Resolve/justify — Transaction journal stores credentialEnv and endpointFingerprint — expanded attack surface

`PRA-9` Resolve/justify — Partial MCP restore on restoreMcpBridgesAfterDestroyAbort failure leaves inconsistent state

`PRA-10` Resolve/justify — Stale transaction journal not cleaned up on fingerprint mismatch

`PRA-11` Resolve/justify — Process-death recovery tests only run on Linux/Unix — no Windows support

`PRA-12` Resolve/justify — PID recycle detection falls back to age-only on non-Linux

`PRA-13` Resolve/justify — Validation tests lack fingerprint/timestamp pattern edge cases

`PRA-14` Resolve/justify — maybePauseForRebuildInterruption uses SIGSTOP without Windows guard

`PRA-15` Resolve/justify — rebuild-transaction.ts monolith growth (922 lines) — extract validation module

`PRA-16` Resolve/justify — registry.ts monolith growth (775 lines) — extract rebuild recovery functions

`PRA-17` Improvement — transactionFileStem uses plain SHA-256 — consider keyed hash (HMAC-SHA256)

`PRA-18` Improvement — E2E env vars not marked internal-only in allowlist

`PRA-19` Improvement — Verify RebuildTransactionStore.create fsyncs parent directory on first publish

`PRA-20` Improvement — Verify deleted registry rollback tests covered by new transaction boundary tests

github-actions Bot commented Jul 8, 2026 •

edited

Loading

github-actions Bot commented Jul 8, 2026 •

edited

Loading

`PRA-1` Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/rebuild-e2e-interruption.ts

`PRA-2` Resolve/justify — Production rebuild path can still be paused by forgeable environment variables

`PRA-3` Resolve/justify — Rebuild transaction store hotspot keeps growing

`PRA-1` Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/rebuild-e2e-interruption.ts

`PRA-2` Resolve/justify — Production rebuild path can still be paused by forgeable environment variables

`PRA-3` Resolve/justify — Rebuild transaction store hotspot keeps growing

Advisor disposition at `436d68650`

Advisor resolution at `cc6858a`

Follow-up Advisor fix at `cbdca27`

Exact-head advisor disposition at `04089ed`