refactor(rebuild): add durable transaction store by jyaunches · Pull Request #6468 · NVIDIA/NemoClaw

jyaunches · 2026-07-08T13:02:54Z

Summary

Adds the persistence foundation for resumable sandbox rebuilds. The V1 store records explicit intent, phase, receipts, failures, and revisions without changing public CLI behavior.

Related Issue

Fixes #6434.

Changes

Add a strict, versioned RebuildTransactionRecordV1 with prepared, old_deleted, replacement_created, and completed phases.
Add create, load, transition, failure, completion, and redacted diagnostic operations.
Acquire the existing per-sandbox lifecycle lock inside every mutation before revision validation and publication; nested rebuild calls remain reentrant.
Persist one record per sandbox with hashed paths, no-follow reads, atomic publication, file and directory fsync, and 0700/0600 permissions.
Reject corrupt/future schemas, invalid phase/status combinations, identity mismatches, stale revisions, receipt replacement, traversal-shaped names, and symlink redirection.
Split concurrency and schema-validation coverage from lifecycle/security coverage so each focused test file remains below the monolith threshold.

Advisor resolution

Competing transition, failure, and completion writers now serialize through the cross-process lifecycle lock; exactly one advances and the stale writer receives REVISION_CONFLICT without overwriting the winner.
Completion with a stale revision on an already-completed record is covered.
NOT_FOUND behavior is covered for every mutation method.
Candidate publication verifies the candidate and canonical directory share a device; an unexpected EXDEV now fails with an explicit invariant error instead of risking a non-atomic fallback.
Present-but-malformed backup, deletion, and replacement receipts are rejected; timestamps require canonical UTC or the bijectively validated product file-safe form, and receipt timestamps must be ordered.
Durable source intent now records the narrowly scoped legacy managed-image recovery authority and original shields posture needed by a fresh recovery process; credential environment names are schema-validated.
A real two-process race test proves cross-process revision exclusion and canonical winner persistence.
Linux directory fsync, macOS best-effort directory durability, same-directory publication, and the HOME-scoped symlink test dependency are documented in code.

Type of Change

Code change (feature, bug fix, or refactor)
Code change with doc updates
Doc only (prose changes, no code sample modifications)

Quality Gates

Tests added or updated for changed behavior
Docs not applicable — internal state primitive only; public CLI behavior is unchanged
Sensitive paths changed
Sensitive-path review completed — coverage includes traversal, symlinks, permissions, atomicity, schema corruption, competing writers, redacted diagnostics, and secret-shaped sentinel exclusion

Verification

DCO sign-off declaration present and commits signed
npm run check:diff
npx tsc -p tsconfig.cli.json --noEmit
npx vitest run --project cli src/lib/state/rebuild-transaction.test.ts src/lib/state/rebuild-transaction-validation.test.ts src/lib/state/rebuild-transaction-concurrency.test.ts — 37 passed; cross-process integration: 1 passed
No secrets, API keys, or credentials committed

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

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

copy-pr-bot · 2026-07-08T13:02:58Z

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:03:04Z

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: b39c5b61-ee3b-4ee0-ad90-32df8d089b5b

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/6434-rebuild-transaction-store

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

github-code-quality · 2026-07-08T13:03:37Z

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

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

Show a code coverage summary of the most covered files.

File	`main`	`refactor/6434-rebuil...` 21467fd	+/-
`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/6434-rebuil... branch is 76%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.

File	`main`	`refactor/6434-rebuil...` 21467fd	+/-
`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`	—	75%	—
`src/lib/onboard...er-gpu-patch.ts`	—	69%	—
`src/lib/policy/index.ts`	—	65%	—
`src/lib/shields/index.ts`	—	60%	—
`src/lib/onboard.ts`	—	28%	—

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

github-actions · 2026-07-08T13:06:04Z

PR Review Advisor (Nemotron Ultra) — Blocked

Merge posture: Do not merge until addressed
Primary next action: Fix PRA-2: EXDEV cross-device link failure has no copy+rename fallback; then add or justify PRA-T1.
Open items: 4 required · 12 warnings · 7 suggestions · 8 test follow-ups
Since last review: 2 prior items resolved · 20 still apply · 3 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`	Required	security	`src/lib/state/rebuild-transaction.ts:545`	Implement copy+rename fallback in createOnly branch: open target with O_CREAT\|O_EXCL, write, fsync, close, then rename. Or add startup runtime check verifying stateDir and candidate parent share same device (stat.dev) and document requirement.
`PRA-3`	Required	security	`src/lib/state/rebuild-transaction.ts:503`	Add platform check in RebuildTransactionStore constructor: if (process.platform === 'darwin') emit warning log that power-loss durability is best-effort. Consider native binding for F_FULLFSYNC as future improvement.
`PRA-4`	Required	architecture	`src/lib/state/rebuild-transaction.ts:1`	Extract into 4 modules: types (interfaces), validation (normalize*), durable (durablePublish, syncDirectory, readStrictRecord), store (RebuildTransactionStore). Keep exports consolidated in index.ts. Do in follow-up PR but track as blocker for this merge.
`PRA-5`	Required	security	`src/lib/state/rebuild-transaction.ts:549`	Throw RebuildTransactionError with a specific code (e.g., 'CROSS_DEVICE') so callers can handle it distinctly. Include actionable guidance in message.
`PRA-6`	Resolve/justify	security	`src/lib/state/rebuild-transaction.ts:101`	Add allowlist of known credential env vars or document that credentialEnv is operator-controlled configuration not user input.
`PRA-7`	Resolve/justify	security	`src/lib/state/rebuild-transaction.ts:328`	Add allowlist validation against known gateway providers, or document that gatewayName is operator-controlled configuration not user input.
`PRA-8`	Resolve/justify	security	`src/lib/state/rebuild-transaction.test.ts:328`	Extend diagnostic test to verify all sensitive fields absent from both serialized record and diagnostic output. At minimum: credentialEnv, all 6 fingerprints, agent, model, gatewayName, gatewayPort.
`PRA-9`	Resolve/justify	security	`src/lib/state/rebuild-transaction.ts:535`	Document why current approach is sufficient, or consider fs.openSync with O_TMPFILE on Linux (kernel 3.11+). Note: requires filesystem support.
`PRA-10`	Resolve/justify	tests	`test/rebuild-transaction-store-process.test.ts:1`	Add cross-process tests for create() and transition() races. Can be deferred to follow-up PR but should be tracked in issue.
`PRA-11`	Resolve/justify	correctness	`src/lib/state/rebuild-transaction.ts:1`	Coordinate with PR #6470 author. Ensure transaction store and journal recovery are complementary. Consider merging in sequence or rebasing.
`PRA-12`	Resolve/justify	architecture	`src/lib/state/rebuild-transaction.ts:545`	Implement copy+rename fallback per PRA-5, or document same-device requirement as deployment constraint with runtime check.
`PRA-13`	Resolve/justify	architecture	`src/lib/state/rebuild-transaction.ts:503`	Implement runtime warning per PRA-6.
`PRA-14`	Resolve/justify	architecture	`src/lib/state/rebuild-transaction.ts:578`	Implement periodic cleanup on load() removing stale .tmp files older than threshold (e.g., 24 hours), or add retry loop with backoff in finally.
`PRA-15`	Resolve/justify	architecture	`src/lib/state/rebuild-transaction.ts:578`	Move cleanup to retry loop in finally (3 retries with backoff), or add periodic cleanup on load() that removes stale .tmp files older than threshold (e.g., 24 hours).
`PRA-16`	Improvement	correctness	`src/lib/state/rebuild-transaction.ts:618`	Update JSDoc to explicitly state sibling async calls in same process are NOT reentrant — they contend on the filesystem lock.
`PRA-17`	Improvement	correctness	`src/lib/state/rebuild-transaction.ts:416`	Extract phase-specific validators as private functions in follow-up PR. Verify all existing tests pass.
`PRA-18`	Improvement	tests	`src/lib/state/rebuild-transaction.test.ts:366`	Parameterize test to use same resolution logic as paths.ts, or add comment noting HOME dependency. Consider adding test variant stubbing XDG_CONFIG_HOME.
`PRA-19`	Improvement	tests	`src/lib/state/rebuild-transaction.test.ts:426`	Parameterize test to cover additional loose permission modes (0o755, 0o644, 0o775, 0o666) verifying all are repaired to 0o700/0o600.
`PRA-20`	Improvement	correctness	`src/lib/state/rebuild-transaction.ts:652`	Add test generating 10,000 transaction IDs and verifying no duplicates.

🚨 Required before merge

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

`PRA-2` Required — EXDEV cross-device link failure has no copy+rename fallback

Location: src/lib/state/rebuild-transaction.ts:545
Category: security
Problem: durablePublish createOnly branch uses fs.linkSync which throws EXDEV when candidate and target are on different filesystems (common with container bind mounts, NFS, overlayfs). The error is treated as an invariant failure with no fallback, causing permanent rebuild failures on valid deployments.
Impact: Sandbox rebuilds permanently fail on cross-device mounts. This is an availability and sandbox escape risk — valid deployments cannot complete rebuilds.
Required action: Implement copy+rename fallback in createOnly branch: open target with O_CREAT|O_EXCL, write, fsync, close, then rename. Or add startup runtime check verifying stateDir and candidate parent share same device (stat.dev) and document requirement.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: In durablePublish createOnly branch (lines 545-555), fs.linkSync throws EXDEV → throws 'atomic-publication invariant failed'. No fallback path exists. Simulate with bind mount across devices.
Missing regression test: Add test simulating EXDEV on create (mock linkSync to throw EXDEV) verifying either copy+rename fallback succeeds or error is actionable with guidance to configure same-device state directory.
Done when: The required change is committed and verification passes: In durablePublish createOnly branch (lines 545-555), fs.linkSync throws EXDEV → throws 'atomic-publication invariant failed'. No fallback path exists. Simulate with bind mount across devices.
Evidence: durablePublish lines 545-555: try { fs.linkSync(candidatePath, filePath) } catch (error) { if (EXDEV) throw invariant error }

`PRA-3` Required — macOS directory fsync lacks F_FULLFSYNC; no runtime warning for best-effort durability

Location: src/lib/state/rebuild-transaction.ts:503
Category: security
Problem: syncDirectory uses fs.fsyncSync which on macOS does not expose F_FULLFSYNC; strict persistence through sudden power loss is unsupported and best-effort. The code comment acknowledges this but there is no runtime warning emitted when running on macOS.
Impact: Operators on macOS are unaware that power-loss durability is degraded, creating false sense of security.
Required action: Add platform check in RebuildTransactionStore constructor: if (process.platform === 'darwin') emit warning log that power-loss durability is best-effort. Consider native binding for F_FULLFSYNC as future improvement.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Run on macOS: no warning logged at startup. syncDirectory uses fs.fsyncSync without F_FULLFSYNC. Comment at lines 503-520 and JSDoc at line 618 acknowledge gap but no runtime signal.
Missing regression test: Add test that instantiates RebuildTransactionStore on macOS (or mocks process.platform) and verifies a warning is logged about best-effort durability.
Done when: The required change is committed and verification passes: Run on macOS: no warning logged at startup. syncDirectory uses fs.fsyncSync without F_FULLFSYNC. Comment at lines 503-520 and JSDoc at line 618 acknowledge gap but no runtime signal.
Evidence: syncDirectory function lines 503-520; class JSDoc lines 618-620

`PRA-4` Required — Monolith growth: 840-line file mixing types, validation, durable I/O, and store class

Location: src/lib/state/rebuild-transaction.ts:1
Category: architecture
Problem: Single 840-line file mixes types/interfaces (~120 lines), validation/normalization (~200 lines), durable I/O (syncDirectory, durablePublish, readStrictRecord ~100 lines), and RebuildTransactionStore class (~200+ lines). Violates single-responsibility and makes review/audit difficult.
Impact: Impedes auditability, review, and future maintenance. Security review of 840 interleaved concerns is error-prone.
Required action: Extract into 4 modules: types (interfaces), validation (normalize*), durable (durablePublish, syncDirectory, readStrictRecord), store (RebuildTransactionStore). Keep exports consolidated in index.ts. Do in follow-up PR but track as blocker for this merge.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: File is 840 lines with all concerns interleaved. No separate modules exist under src/lib/state/ for these concerns.
Missing regression test: No test needed; architectural. Verify follow-up PR splits modules and all existing tests pass.
Done when: The required change is committed and verification passes: File is 840 lines with all concerns interleaved. No separate modules exist under src/lib/state/ for these concerns.
Evidence: File line count 840; drift context monolithDeltas severity blocker

`PRA-5` Required — EXDEV throws plain Error not RebuildTransactionError — callers cannot handle programmatically

Location: src/lib/state/rebuild-transaction.ts:549
Category: security
Problem: When linkSync throws EXDEV, the code throws a plain Error ('Rebuild transaction atomic-publication invariant failed') instead of a RebuildTransactionError with a typed code. Callers cannot distinguish this from other failures.
Impact: Callers cannot catch EXDEV specifically to provide actionable guidance or fallback logic. Error handling is inconsistent with rest of module.
Required action: Throw RebuildTransactionError with a specific code (e.g., 'CROSS_DEVICE') so callers can handle it distinctly. Include actionable guidance in message.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Lines 549-555: throw new Error(...) not transactionError(...). Catch block in create() at line 660-667 only handles EEXIST.
Missing regression test: Test verifying EXDEV throws RebuildTransactionError with CROSS_DEVICE code.
Done when: The required change is committed and verification passes: Lines 549-555: throw new Error(...) not transactionError(...). Catch block in create() at line 660-667 only handles EEXIST.
Evidence: durablePublish lines 549-555; create() catch block lines 660-667

Review findings by urgency: 4 required fixes, 12 items to resolve/justify, 7 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: Persistent .tmp file leak if unlinkSync fails repeatedly

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 persistent unlink failure
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: Lines 578-583

`PRA-6` Resolve/justify — credentialEnv validation only checks ENV_NAME_PATTERN, no allowlist of known credential variables

Location: src/lib/state/rebuild-transaction.ts:101
Category: security
Problem: credentialEnv only validates against ENV_NAME_PATTERN (^[A-Za-z_][A-Za-z0-9_]*$), no allowlist of known credential variables (NVIDIA_API_KEY, BRAVE_API_KEY, TAVILY_API_KEY, etc.). An operator could specify any environment variable name.
Impact: Potential for sandbox to read unintended secrets from environment if misconfigured.
Recommended action: Add allowlist of known credential env vars or document that credentialEnv is operator-controlled configuration not user input.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: nullableEnvName function at lines 98-101 only validates pattern. No allowlist check. normalizeIntent at line 305-307 passes through.
Missing regression test: Test attempting to set credentialEnv to 'AWS_SECRET_ACCESS_KEY' (valid pattern, not in allowlist) — should be rejected if allowlist added, or documented as operator responsibility.
Done when: The risk is fixed or explicitly justified in the PR. Verification: nullableEnvName function at lines 98-101 only validates pattern. No allowlist check. normalizeIntent at line 305-307 passes through.
Evidence: Lines 98-101, 305-307

`PRA-7` Resolve/justify — gatewayName is free-form string with no allowlist or injection prevention

Location: src/lib/state/rebuild-transaction.ts:328
Category: security
Problem: gatewayName is validated only as non-empty string ≤1024 chars. No pattern or allowlist validation. Used in network policy construction; a malicious or misconfigured value could affect routing or policy decisions.
Impact: Potential injection or misrouting if gatewayName contains unexpected characters.
Recommended action: Add allowlist validation against known gateway providers, or document that gatewayName is operator-controlled configuration not user input.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Line 328: gatewayName: requiredString(...) — only validates non-empty string ≤1024 chars. No pattern or allowlist.
Missing regression test: Test with gatewayName containing injection characters (e.g., 'gateway; rm -rf /') — should be rejected or sanitized if allowlist added.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Line 328: gatewayName: requiredString(...) — only validates non-empty string ≤1024 chars. No pattern or allowlist.
Evidence: Line 328

`PRA-8` Resolve/justify — Diagnostic projection test incomplete — does not verify redaction of all sensitive fields

Location: src/lib/state/rebuild-transaction.test.ts:328
Category: security
Problem: Test verifies redaction of NVIDIA_API_KEY and 'provider' but not credentialEnv, all 6 fingerprints (registryFingerprint, endpointFingerprint, imageFingerprint, configurationFingerprint, identityFingerprint, manifestFingerprint), agent, model, gatewayName, gatewayPort.
Impact: Sensitive fields may leak in diagnostic output without detection.
Recommended action: Extend diagnostic test to verify all sensitive fields absent from both serialized record and diagnostic output. At minimum: credentialEnv, all 6 fingerprints, agent, model, gatewayName, gatewayPort.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Test at lines 328-350 only checks secret, endpointUrl, environment, NVIDIA_API_KEY, provider. Fingerprints, agent, model, gatewayName, gatewayPort not verified.
Missing regression test: Parameterized test verifying each sensitive field in intent and receipts is absent from serialized JSON and diagnostic output.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Test at lines 328-350 only checks secret, endpointUrl, environment, NVIDIA_API_KEY, provider. Fingerprints, agent, model, gatewayName, gatewayPort not verified.
Evidence: rebuild-transaction.test.ts lines 328-350

`PRA-9` Resolve/justify — candidatePath uses predictable PID + randomUUID but no O_TMPFILE on Linux

Location: src/lib/state/rebuild-transaction.ts:535
Category: security
Problem: candidatePath uses PID + randomUUID for uniqueness. O_TMPFILE (kernel 3.11+) creates unnamed temporary files invisible to directory traversal, eliminating symlink/race risk on the candidate file itself. Current approach relies on O_EXCL + random name which is strong but not defense-in-depth.
Impact: Local attacker with write access to 0700 directory could theoretically guess and create conflicting file (though directory permissions mitigate).
Recommended action: Document why current approach is sufficient, or consider fs.openSync with O_TMPFILE on Linux (kernel 3.11+). Note: requires filesystem support.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Line 535-537: candidatePath uses PID + randomUUID. No O_TMPFILE usage. O_TMPFILE would require fd-based write and linkat(AT_EMPTY_PATH) for publication.
Missing regression test: Not directly testable; document decision. If O_TMPFILE adopted, test that candidate file is not visible in directory listing.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Line 535-537: candidatePath uses PID + randomUUID. No O_TMPFILE usage. O_TMPFILE would require fd-based write and linkat(AT_EMPTY_PATH) for publication.
Evidence: Lines 535-537

`PRA-10` Resolve/justify — Cross-process concurrency test only covers recordFailure race

Location: test/rebuild-transaction-store-process.test.ts:1
Category: tests
Problem: Only one cross-process test exists, covering recordFailure race. No cross-process tests for create() and transition() races, which have different code paths (createOnly=true vs false in durablePublish).
Impact: Cross-process create/transition races untested; different atomic publication paths (linkSync vs renameSync) not verified under contention.
Recommended action: Add cross-process tests for create() and transition() races. Can be deferred to follow-up PR but should be tracked in issue.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Only one test in rebuild-transaction-store-process.test.ts covering recordFailure. create() and transition() use different durablePublish paths (linkSync vs renameSync).
Missing regression test: Two new cross-process tests: (1) concurrent create() from two processes, (2) concurrent transition() from two processes.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Only one test in rebuild-transaction-store-process.test.ts covering recordFailure. create() and transition() use different durablePublish paths (linkSync vs renameSync).
Evidence: test/rebuild-transaction-store-process.test.ts only has recordFailure test

`PRA-11` Resolve/justify — Overlapping PR #6470 modifies same file — integration risk

Location: src/lib/state/rebuild-transaction.ts:1
Category: correctness
Problem: PR refactor(rebuild): journal destructive recovery #6470 'refactor(rebuild): journal destructive recovery' modifies the same file (rebuild-transaction.ts). Both PRs implement durable state for rebuild; they must be coordinated to avoid conflicting designs or merge conflicts.
Impact: Conflicting designs, merge conflicts, or duplicate/contradictory durable state implementations.
Recommended action: Coordinate with PR refactor(rebuild): journal destructive recovery #6470 author. Ensure transaction store and journal recovery are complementary. Consider merging in sequence or rebasing.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Drift context shows openPrOverlaps with PR refactor(rebuild): journal destructive recovery #6470, sameFiles: ['src/lib/state/rebuild-transaction.ts'].
Missing regression test: Integration test covering both transaction store and journal recovery working together.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Drift context shows openPrOverlaps with PR refactor(rebuild): journal destructive recovery #6470, sameFiles: ['src/lib/state/rebuild-transaction.ts'].
Evidence: Drift context openPrOverlaps PR refactor(rebuild): journal destructive recovery #6470

`PRA-12` Resolve/justify — Source-of-truth: EXDEV cross-device link failure handling — documented as invariant failure but copy+rename fallback feasible

Location: src/lib/state/rebuild-transaction.ts:545
Category: architecture
Problem: Invalid state: cross-device link fails on valid deployments. Source boundary: filesystem/device topology outside NemoClaw. Copy+rename with O_EXCL provides equivalent atomic no-replace safety for create-only path — this IS fixable here.
Impact: Policy choice documented as physical constraint; fallback implementation would resolve availability issue.
Recommended action: Implement copy+rename fallback per PRA-5, or document same-device requirement as deployment constraint with runtime check.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: README lines 9-12 state cross-device is invariant failure. Test at rebuild-transaction.test.ts:292-307 expects EXDEV to throw.
Missing regression test: Already have test expecting failure; need test verifying fallback works once implemented.
Done when: The risk is fixed or explicitly justified in the PR. Verification: README lines 9-12 state cross-device is invariant failure. Test at rebuild-transaction.test.ts:292-307 expects EXDEV to throw.
Evidence: README lines 9-12; test lines 292-307

`PRA-13` Resolve/justify — Source-of-truth: macOS directory fsync durability — documented but no runtime warning

Location: src/lib/state/rebuild-transaction.ts:503
Category: architecture
Problem: Invalid state: directory entry lost after power loss on macOS. Source boundary: macOS F_FULLFSYNC not exposed by Node. Why not fixable: requires Node runtime change or native adapter. Regression evidence: atomicity/torn-record tests pass; physical power-loss persistence not testable in CI.
Impact: Operators unaware of degraded durability on macOS.
Recommended action: Implement runtime warning per PRA-6.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: README lines 19-22. Code comments at lines 503-520 and 618-620.
Missing regression test: See PRA-6.
Done when: The risk is fixed or explicitly justified in the PR. Verification: README lines 19-22. Code comments at lines 503-520 and 618-620.
Evidence: README lines 19-22; syncDirectory comments; class JSDoc

`PRA-14` Resolve/justify — Source-of-truth: temporary .tmp file cleanup best-effort — no periodic cleanup or retry loop

Location: src/lib/state/rebuild-transaction.ts:578
Category: architecture
Problem: Invalid state: .tmp file left behind if unlinkSync fails persistently. Source boundary: filesystem errors during cleanup. Why not fixable: cleanup is best-effort by design. Periodic cleanup on load() feasible.
Impact: Stale .tmp files accumulate over time (though inert).
Recommended action: Implement periodic cleanup on load() removing stale .tmp files older than threshold (e.g., 24 hours), or add retry loop with backoff in finally.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Finally block at lines 578-583 catches and ignores unlinkSync failure. Comment says 'finally retries cleanup' but only tries once.
Missing regression test: Test simulating persistent unlinkSync failure (mock unlinkSync to always throw) verifying periodic cleanup on load() removes stale .tmp files.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Finally block at lines 578-583 catches and ignores unlinkSync failure. Comment says 'finally retries cleanup' but only tries once.
Evidence: Lines 578-583; comments at lines 552-553

`PRA-15` Resolve/justify — Source-of-truth: .tmp file leak if linkSync succeeds but unlinkSync fails persistently

Location: src/lib/state/rebuild-transaction.ts:578
Category: architecture
Problem: The finally block attempts cleanup once; if it fails repeatedly (e.g., permission issue, filesystem error), a 0600 dotfile hard link remains in the 0700 state directory. While inert, it represents a cleanup gap.
Impact: Persistent .tmp files accumulate; cleanup only attempted once.
Recommended action: Move cleanup to retry loop in finally (3 retries with backoff), or add periodic cleanup on load() that removes stale .tmp files older than threshold (e.g., 24 hours).
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Finally block lines 578-583: single try/catch with no retry. Comment acknowledges persistent failure can leave dotfile.
Missing regression test: Test with unlinkSync mocked to fail 3x then succeed, verifying retry loop cleans up. Or test load() removes .tmp files older than 24h.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Finally block lines 578-583: single try/catch with no retry. Comment acknowledges persistent failure can leave dotfile.
Evidence: Lines 578-583

`PRA-23` Resolve/justify — Diagnostic test injects non-existent fields via type assertion — tests redaction of fields that would be stripped anyway

Location: src/lib/state/rebuild-transaction.test.ts:328
Category: tests
Problem: Test injects secret via endpointUrl and environment but the intent type RebuildTransactionIntentV1 does not have these fields — they are added via type assertion (as unknown as RebuildTransactionIntentV1). The test checks redaction of fields that would be stripped by normalizeIntent anyway, not fields that actually persist.
Impact: Test gives false confidence; actual sensitive fields (credentialEnv, fingerprints, agent, model, gatewayName, gatewayPort) not verified.
Recommended action: Fix test to only test redaction of fields that actually exist in the intent type and survive normalization. Or add those fields to the type if they are intended to be accepted.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Test lines 333-340: untrustedIntent adds token, endpointUrl, environment which are not in RebuildTransactionIntentV1. normalizeIntent only extracts allow-listed fields.
Missing regression test: Test should verify redaction of actual persisted fields: credentialEnv, fingerprints, agent, model, gatewayName, gatewayPort.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Test lines 333-340: untrustedIntent adds token, endpointUrl, environment which are not in RebuildTransactionIntentV1. normalizeIntent only extracts allow-listed fields.
Evidence: Lines 333-340

💡 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-16` Improvement — JSDoc reentrancy claim imprecise — sibling async calls are NOT reentrant

Location: src/lib/state/rebuild-transaction.ts:618
Category: correctness
Problem: JSDoc states 'Reentrancy is intra-process only via AsyncLocalStorage; cross-process callers always contend on the filesystem lock.' However, sibling async calls in the same process are NOT reentrant — they contend on the filesystem lock.
Impact: Misleading documentation could cause callers to assume intra-process concurrency works when it serializes.
Suggested action: Update JSDoc to explicitly state sibling async calls in same process are NOT reentrant — they contend on the filesystem lock.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: JSDoc at lines 616-620. withMcpLifecycleLock uses AsyncLocalStorage for nested calls only; concurrent calls in same process queue on filesystem lock.
Missing regression test: Test verifying two concurrent create() calls in same process serialize (not reentrant).
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Lines 616-620

`PRA-17` Improvement — normalizeRecord is large; extract phase-specific validators

Location: src/lib/state/rebuild-transaction.ts:416
Category: correctness
Problem: normalizeRecord spans ~80 lines mixing version check, transactionId validation, revision, phase/status, receipts cross-phase validation, timestamp validation. Hard to audit and maintain.
Impact: Reduced maintainability and auditability of validation logic.
Suggested action: Extract phase-specific validators as private functions in follow-up PR. Verify all existing tests pass.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Function spans lines 416-495 with multiple validation concerns interleaved.
Missing regression test: All existing tests should pass after refactor; no new test needed.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Lines 416-495

`PRA-18` Improvement — Symlink attack test stubs HOME but not XDG_CONFIG_HOME

Location: src/lib/state/rebuild-transaction.test.ts:366
Category: tests
Problem: The rejectSymlinksOnPath function in config-io.ts uses HOME to scope user-controlled components. Test should use same resolution logic or add variant stubbing XDG_CONFIG_HOME.
Impact: Test may not catch symlink attacks via XDG_CONFIG_HOME if resolution logic differs.
Suggested action: Parameterize test to use same resolution logic as paths.ts, or add comment noting HOME dependency. Consider adding test variant stubbing XDG_CONFIG_HOME.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Test at lines 366-392 uses vi.stubEnv('HOME', root). config-io.ts rejectSymlinksOnPath uses process.env.HOME || os.homedir().
Missing regression test: Add test variant with XDG_CONFIG_HOME set, verifying symlink rejection still works.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Test lines 366-392; config-io.ts rejectSymlinksOnPath

`PRA-19` Improvement — Permission repair test only covers 0o777/0o666

Location: src/lib/state/rebuild-transaction.test.ts:426
Category: tests
Problem: Test only sets 0o777 and 0o666. Does not test other loose permissions (e.g., 0o755, 0o644, 0o775) or group/world writable bits.
Impact: Other loose permission modes may not be repaired.
Suggested action: Parameterize test to cover additional loose permission modes (0o755, 0o644, 0o775, 0o666) verifying all are repaired to 0o700/0o600.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Test at lines 426-436 only sets 0o777 and 0o666.
Missing regression test: Parametrized test with matrix of loose modes verifying repair to 0o700/0o600.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Lines 426-436

`PRA-20` Improvement — Transaction ID generation has no uniqueness test

Location: src/lib/state/rebuild-transaction.ts:652
Category: correctness
Problem: Transaction ID generation uses crypto.randomUUID() with no uniqueness test. While UUIDv4 collision probability is negligible, a test verifying the generator produces unique IDs under load would increase confidence.
Impact: Theoretical collision risk undetected.
Suggested action: Add test generating 10,000 transaction IDs and verifying no duplicates.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Constructor at line 652: transactionId: () => crypto.randomUUID(). No test for uniqueness.
Missing regression test: Test generating N transaction IDs and verifying Set.size === N.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Line 652

`PRA-21` Improvement — RebuildTransactionStore not yet integrated into rebuild-pipeline

Location: src/lib/state/rebuild-transaction.ts:1
Category: architecture
Problem: The store exists in isolation; integration with the actual rebuild coordinator is pending.
Impact: API may not match pipeline needs; integration work deferred.
Suggested action: Track integration as follow-up work. Ensure this PR's API matches pipeline needs.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: No imports of RebuildTransactionStore outside test files. Grep shows only test imports.
Missing regression test: Integration test once wired into rebuild-pipeline.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: No production imports of RebuildTransactionStore

`PRA-22` Improvement — tempDir uses os.tmpdir() which may be cross-device from stateDir in production

Location: test/helpers/rebuild-transaction-store.ts:29
Category: tests
Problem: tempDir uses os.tmpdir() which may be cross-device from stateDir in production. This means tests may not catch EXDEV issues (PRA-5) because test temp dir and state dir happen to be on same device in CI.
Impact: EXDEV fallback (PRA-5) not exercised in CI; test environment differs from production.
Suggested action: Document that test environment may not reproduce cross-device scenarios. Consider adding a test variant that forces cross-device temp dir (if feasible) or document as known test gap.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Line 29: path.join(os.tmpdir(), 'nemoclaw-rebuild-transaction-'). Production stateDir is ~/.nemoclaw/state which may be on different filesystem.
Missing regression test: Test with bind-mounted temp dir on different device, or document as known gap.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Line 29

Simplification opportunities: 2 possible cuts, net -20 lines possible

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

PRA-4 shrink (src/lib/state/rebuild-transaction.ts:1): Split rebuild-transaction.ts into 4 modules: types, validation, durable, store
- Replacement: Create src/lib/state/rebuild-transaction-types.ts, rebuild-transaction-validation.ts, rebuild-transaction-durable.ts, rebuild-transaction-store.ts with index.ts re-exporting
- Net: 0 lines
- Safety boundary: All existing tests must pass; public API surface unchanged
PRA-17 shrink (src/lib/state/rebuild-transaction.ts:416): Extract phase-specific validation helpers from normalizeRecord
- Replacement: Private functions: validateVersion, validateTransactionId, validateRevision, validatePhaseStatus, validateReceiptsPhaseConsistency, validateTimestamps
- Net: -20 lines
- Safety boundary: All existing validation tests must pass; no behavior change

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 — EXDEV fallback test: mock linkSync to throw EXDEV, verify copy+rename fallback succeeds or actionable error. New durable state primitive with atomic publication, cross-process locking, and platform-specific durability semantics requires runtime validation for EXDEV behavior on cross-device mounts, macOS durability warning, and cross-process lock contention.
PRA-T2 Runtime validation — macOS runtime warning test: mock process.platform='darwin', instantiate store, verify warning logged. New durable state primitive with atomic publication, cross-process locking, and platform-specific durability semantics requires runtime validation for EXDEV behavior on cross-device mounts, macOS durability warning, and cross-process lock contention.
PRA-T3 Runtime validation — Periodic .tmp cleanup test: create stale .tmp files >24h old, call load(), verify cleanup. New durable state primitive with atomic publication, cross-process locking, and platform-specific durability semantics requires runtime validation for EXDEV behavior on cross-device mounts, macOS durability warning, and cross-process lock contention.
PRA-T4 Runtime validation — Diagnostic redaction completeness: parameterized test for all sensitive fields (credentialEnv, 6 fingerprints, agent, model, gatewayName, gatewayPort). New durable state primitive with atomic publication, cross-process locking, and platform-specific durability semantics requires runtime validation for EXDEV behavior on cross-device mounts, macOS durability warning, and cross-process lock contention.
PRA-T5 Runtime validation — Cross-process create() race test: two processes concurrent create() for same sandbox. New durable state primitive with atomic publication, cross-process locking, and platform-specific durability semantics requires runtime validation for EXDEV behavior on cross-device mounts, macOS durability warning, and cross-process lock contention.
PRA-T6 Cross-process concurrency test only covers recordFailure race — Add cross-process tests for create() and transition() races. Can be deferred to follow-up PR but should be tracked in issue.
PRA-T7 Symlink attack test stubs HOME but not XDG_CONFIG_HOME — Parameterize test to use same resolution logic as paths.ts, or add comment noting HOME dependency. Consider adding test variant stubbing XDG_CONFIG_HOME.
PRA-T8 Permission repair test only covers 0o777/0o666 — Parameterize test to cover additional loose permission modes (0o755, 0o644, 0o775, 0o666) verifying all are repaired to 0o700/0o600.

Since last review details

Current findings, using the urgency labels above:

`PRA-1` Resolve/justify — Source-of-truth review needed: Persistent .tmp file leak if unlinkSync fails repeatedly

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 persistent unlink failure
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: Lines 578-583

`PRA-2` Required — EXDEV cross-device link failure has no copy+rename fallback

Location: src/lib/state/rebuild-transaction.ts:545
Category: security
Problem: durablePublish createOnly branch uses fs.linkSync which throws EXDEV when candidate and target are on different filesystems (common with container bind mounts, NFS, overlayfs). The error is treated as an invariant failure with no fallback, causing permanent rebuild failures on valid deployments.
Impact: Sandbox rebuilds permanently fail on cross-device mounts. This is an availability and sandbox escape risk — valid deployments cannot complete rebuilds.
Required action: Implement copy+rename fallback in createOnly branch: open target with O_CREAT|O_EXCL, write, fsync, close, then rename. Or add startup runtime check verifying stateDir and candidate parent share same device (stat.dev) and document requirement.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: In durablePublish createOnly branch (lines 545-555), fs.linkSync throws EXDEV → throws 'atomic-publication invariant failed'. No fallback path exists. Simulate with bind mount across devices.
Missing regression test: Add test simulating EXDEV on create (mock linkSync to throw EXDEV) verifying either copy+rename fallback succeeds or error is actionable with guidance to configure same-device state directory.
Done when: The required change is committed and verification passes: In durablePublish createOnly branch (lines 545-555), fs.linkSync throws EXDEV → throws 'atomic-publication invariant failed'. No fallback path exists. Simulate with bind mount across devices.
Evidence: durablePublish lines 545-555: try { fs.linkSync(candidatePath, filePath) } catch (error) { if (EXDEV) throw invariant error }

`PRA-3` Required — macOS directory fsync lacks F_FULLFSYNC; no runtime warning for best-effort durability

Location: src/lib/state/rebuild-transaction.ts:503
Category: security
Problem: syncDirectory uses fs.fsyncSync which on macOS does not expose F_FULLFSYNC; strict persistence through sudden power loss is unsupported and best-effort. The code comment acknowledges this but there is no runtime warning emitted when running on macOS.
Impact: Operators on macOS are unaware that power-loss durability is degraded, creating false sense of security.
Required action: Add platform check in RebuildTransactionStore constructor: if (process.platform === 'darwin') emit warning log that power-loss durability is best-effort. Consider native binding for F_FULLFSYNC as future improvement.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Run on macOS: no warning logged at startup. syncDirectory uses fs.fsyncSync without F_FULLFSYNC. Comment at lines 503-520 and JSDoc at line 618 acknowledge gap but no runtime signal.
Missing regression test: Add test that instantiates RebuildTransactionStore on macOS (or mocks process.platform) and verifies a warning is logged about best-effort durability.
Done when: The required change is committed and verification passes: Run on macOS: no warning logged at startup. syncDirectory uses fs.fsyncSync without F_FULLFSYNC. Comment at lines 503-520 and JSDoc at line 618 acknowledge gap but no runtime signal.
Evidence: syncDirectory function lines 503-520; class JSDoc lines 618-620

`PRA-4` Required — Monolith growth: 840-line file mixing types, validation, durable I/O, and store class

Location: src/lib/state/rebuild-transaction.ts:1
Category: architecture
Problem: Single 840-line file mixes types/interfaces (~120 lines), validation/normalization (~200 lines), durable I/O (syncDirectory, durablePublish, readStrictRecord ~100 lines), and RebuildTransactionStore class (~200+ lines). Violates single-responsibility and makes review/audit difficult.
Impact: Impedes auditability, review, and future maintenance. Security review of 840 interleaved concerns is error-prone.
Required action: Extract into 4 modules: types (interfaces), validation (normalize*), durable (durablePublish, syncDirectory, readStrictRecord), store (RebuildTransactionStore). Keep exports consolidated in index.ts. Do in follow-up PR but track as blocker for this merge.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: File is 840 lines with all concerns interleaved. No separate modules exist under src/lib/state/ for these concerns.
Missing regression test: No test needed; architectural. Verify follow-up PR splits modules and all existing tests pass.
Done when: The required change is committed and verification passes: File is 840 lines with all concerns interleaved. No separate modules exist under src/lib/state/ for these concerns.
Evidence: File line count 840; drift context monolithDeltas severity blocker

`PRA-5` Required — EXDEV throws plain Error not RebuildTransactionError — callers cannot handle programmatically

Location: src/lib/state/rebuild-transaction.ts:549
Category: security
Problem: When linkSync throws EXDEV, the code throws a plain Error ('Rebuild transaction atomic-publication invariant failed') instead of a RebuildTransactionError with a typed code. Callers cannot distinguish this from other failures.
Impact: Callers cannot catch EXDEV specifically to provide actionable guidance or fallback logic. Error handling is inconsistent with rest of module.
Required action: Throw RebuildTransactionError with a specific code (e.g., 'CROSS_DEVICE') so callers can handle it distinctly. Include actionable guidance in message.
Expected follow-up: Fix before merge or get explicit maintainer override.
Verification: Lines 549-555: throw new Error(...) not transactionError(...). Catch block in create() at line 660-667 only handles EEXIST.
Missing regression test: Test verifying EXDEV throws RebuildTransactionError with CROSS_DEVICE code.
Done when: The required change is committed and verification passes: Lines 549-555: throw new Error(...) not transactionError(...). Catch block in create() at line 660-667 only handles EEXIST.
Evidence: durablePublish lines 549-555; create() catch block lines 660-667

`PRA-6` Resolve/justify — credentialEnv validation only checks ENV_NAME_PATTERN, no allowlist of known credential variables

Location: src/lib/state/rebuild-transaction.ts:101
Category: security
Problem: credentialEnv only validates against ENV_NAME_PATTERN (^[A-Za-z_][A-Za-z0-9_]*$), no allowlist of known credential variables (NVIDIA_API_KEY, BRAVE_API_KEY, TAVILY_API_KEY, etc.). An operator could specify any environment variable name.
Impact: Potential for sandbox to read unintended secrets from environment if misconfigured.
Recommended action: Add allowlist of known credential env vars or document that credentialEnv is operator-controlled configuration not user input.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: nullableEnvName function at lines 98-101 only validates pattern. No allowlist check. normalizeIntent at line 305-307 passes through.
Missing regression test: Test attempting to set credentialEnv to 'AWS_SECRET_ACCESS_KEY' (valid pattern, not in allowlist) — should be rejected if allowlist added, or documented as operator responsibility.
Done when: The risk is fixed or explicitly justified in the PR. Verification: nullableEnvName function at lines 98-101 only validates pattern. No allowlist check. normalizeIntent at line 305-307 passes through.
Evidence: Lines 98-101, 305-307

`PRA-7` Resolve/justify — gatewayName is free-form string with no allowlist or injection prevention

Location: src/lib/state/rebuild-transaction.ts:328
Category: security
Problem: gatewayName is validated only as non-empty string ≤1024 chars. No pattern or allowlist validation. Used in network policy construction; a malicious or misconfigured value could affect routing or policy decisions.
Impact: Potential injection or misrouting if gatewayName contains unexpected characters.
Recommended action: Add allowlist validation against known gateway providers, or document that gatewayName is operator-controlled configuration not user input.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Line 328: gatewayName: requiredString(...) — only validates non-empty string ≤1024 chars. No pattern or allowlist.
Missing regression test: Test with gatewayName containing injection characters (e.g., 'gateway; rm -rf /') — should be rejected or sanitized if allowlist added.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Line 328: gatewayName: requiredString(...) — only validates non-empty string ≤1024 chars. No pattern or allowlist.
Evidence: Line 328

`PRA-8` Resolve/justify — Diagnostic projection test incomplete — does not verify redaction of all sensitive fields

Location: src/lib/state/rebuild-transaction.test.ts:328
Category: security
Problem: Test verifies redaction of NVIDIA_API_KEY and 'provider' but not credentialEnv, all 6 fingerprints (registryFingerprint, endpointFingerprint, imageFingerprint, configurationFingerprint, identityFingerprint, manifestFingerprint), agent, model, gatewayName, gatewayPort.
Impact: Sensitive fields may leak in diagnostic output without detection.
Recommended action: Extend diagnostic test to verify all sensitive fields absent from both serialized record and diagnostic output. At minimum: credentialEnv, all 6 fingerprints, agent, model, gatewayName, gatewayPort.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Test at lines 328-350 only checks secret, endpointUrl, environment, NVIDIA_API_KEY, provider. Fingerprints, agent, model, gatewayName, gatewayPort not verified.
Missing regression test: Parameterized test verifying each sensitive field in intent and receipts is absent from serialized JSON and diagnostic output.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Test at lines 328-350 only checks secret, endpointUrl, environment, NVIDIA_API_KEY, provider. Fingerprints, agent, model, gatewayName, gatewayPort not verified.
Evidence: rebuild-transaction.test.ts lines 328-350

`PRA-9` Resolve/justify — candidatePath uses predictable PID + randomUUID but no O_TMPFILE on Linux

Location: src/lib/state/rebuild-transaction.ts:535
Category: security
Problem: candidatePath uses PID + randomUUID for uniqueness. O_TMPFILE (kernel 3.11+) creates unnamed temporary files invisible to directory traversal, eliminating symlink/race risk on the candidate file itself. Current approach relies on O_EXCL + random name which is strong but not defense-in-depth.
Impact: Local attacker with write access to 0700 directory could theoretically guess and create conflicting file (though directory permissions mitigate).
Recommended action: Document why current approach is sufficient, or consider fs.openSync with O_TMPFILE on Linux (kernel 3.11+). Note: requires filesystem support.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Line 535-537: candidatePath uses PID + randomUUID. No O_TMPFILE usage. O_TMPFILE would require fd-based write and linkat(AT_EMPTY_PATH) for publication.
Missing regression test: Not directly testable; document decision. If O_TMPFILE adopted, test that candidate file is not visible in directory listing.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Line 535-537: candidatePath uses PID + randomUUID. No O_TMPFILE usage. O_TMPFILE would require fd-based write and linkat(AT_EMPTY_PATH) for publication.
Evidence: Lines 535-537

`PRA-10` Resolve/justify — Cross-process concurrency test only covers recordFailure race

Location: test/rebuild-transaction-store-process.test.ts:1
Category: tests
Problem: Only one cross-process test exists, covering recordFailure race. No cross-process tests for create() and transition() races, which have different code paths (createOnly=true vs false in durablePublish).
Impact: Cross-process create/transition races untested; different atomic publication paths (linkSync vs renameSync) not verified under contention.
Recommended action: Add cross-process tests for create() and transition() races. Can be deferred to follow-up PR but should be tracked in issue.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Only one test in rebuild-transaction-store-process.test.ts covering recordFailure. create() and transition() use different durablePublish paths (linkSync vs renameSync).
Missing regression test: Two new cross-process tests: (1) concurrent create() from two processes, (2) concurrent transition() from two processes.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Only one test in rebuild-transaction-store-process.test.ts covering recordFailure. create() and transition() use different durablePublish paths (linkSync vs renameSync).
Evidence: test/rebuild-transaction-store-process.test.ts only has recordFailure test

`PRA-11` Resolve/justify — Overlapping PR #6470 modifies same file — integration risk

Location: src/lib/state/rebuild-transaction.ts:1
Category: correctness
Problem: PR refactor(rebuild): journal destructive recovery #6470 'refactor(rebuild): journal destructive recovery' modifies the same file (rebuild-transaction.ts). Both PRs implement durable state for rebuild; they must be coordinated to avoid conflicting designs or merge conflicts.
Impact: Conflicting designs, merge conflicts, or duplicate/contradictory durable state implementations.
Recommended action: Coordinate with PR refactor(rebuild): journal destructive recovery #6470 author. Ensure transaction store and journal recovery are complementary. Consider merging in sequence or rebasing.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Drift context shows openPrOverlaps with PR refactor(rebuild): journal destructive recovery #6470, sameFiles: ['src/lib/state/rebuild-transaction.ts'].
Missing regression test: Integration test covering both transaction store and journal recovery working together.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Drift context shows openPrOverlaps with PR refactor(rebuild): journal destructive recovery #6470, sameFiles: ['src/lib/state/rebuild-transaction.ts'].
Evidence: Drift context openPrOverlaps PR refactor(rebuild): journal destructive recovery #6470

`PRA-12` Resolve/justify — Source-of-truth: EXDEV cross-device link failure handling — documented as invariant failure but copy+rename fallback feasible

Location: src/lib/state/rebuild-transaction.ts:545
Category: architecture
Problem: Invalid state: cross-device link fails on valid deployments. Source boundary: filesystem/device topology outside NemoClaw. Copy+rename with O_EXCL provides equivalent atomic no-replace safety for create-only path — this IS fixable here.
Impact: Policy choice documented as physical constraint; fallback implementation would resolve availability issue.
Recommended action: Implement copy+rename fallback per PRA-5, or document same-device requirement as deployment constraint with runtime check.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: README lines 9-12 state cross-device is invariant failure. Test at rebuild-transaction.test.ts:292-307 expects EXDEV to throw.
Missing regression test: Already have test expecting failure; need test verifying fallback works once implemented.
Done when: The risk is fixed or explicitly justified in the PR. Verification: README lines 9-12 state cross-device is invariant failure. Test at rebuild-transaction.test.ts:292-307 expects EXDEV to throw.
Evidence: README lines 9-12; test lines 292-307

`PRA-13` Resolve/justify — Source-of-truth: macOS directory fsync durability — documented but no runtime warning

Location: src/lib/state/rebuild-transaction.ts:503
Category: architecture
Problem: Invalid state: directory entry lost after power loss on macOS. Source boundary: macOS F_FULLFSYNC not exposed by Node. Why not fixable: requires Node runtime change or native adapter. Regression evidence: atomicity/torn-record tests pass; physical power-loss persistence not testable in CI.
Impact: Operators unaware of degraded durability on macOS.
Recommended action: Implement runtime warning per PRA-6.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: README lines 19-22. Code comments at lines 503-520 and 618-620.
Missing regression test: See PRA-6.
Done when: The risk is fixed or explicitly justified in the PR. Verification: README lines 19-22. Code comments at lines 503-520 and 618-620.
Evidence: README lines 19-22; syncDirectory comments; class JSDoc

`PRA-14` Resolve/justify — Source-of-truth: temporary .tmp file cleanup best-effort — no periodic cleanup or retry loop

Location: src/lib/state/rebuild-transaction.ts:578
Category: architecture
Problem: Invalid state: .tmp file left behind if unlinkSync fails persistently. Source boundary: filesystem errors during cleanup. Why not fixable: cleanup is best-effort by design. Periodic cleanup on load() feasible.
Impact: Stale .tmp files accumulate over time (though inert).
Recommended action: Implement periodic cleanup on load() removing stale .tmp files older than threshold (e.g., 24 hours), or add retry loop with backoff in finally.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Finally block at lines 578-583 catches and ignores unlinkSync failure. Comment says 'finally retries cleanup' but only tries once.
Missing regression test: Test simulating persistent unlinkSync failure (mock unlinkSync to always throw) verifying periodic cleanup on load() removes stale .tmp files.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Finally block at lines 578-583 catches and ignores unlinkSync failure. Comment says 'finally retries cleanup' but only tries once.
Evidence: Lines 578-583; comments at lines 552-553

`PRA-15` Resolve/justify — Source-of-truth: .tmp file leak if linkSync succeeds but unlinkSync fails persistently

Location: src/lib/state/rebuild-transaction.ts:578
Category: architecture
Problem: The finally block attempts cleanup once; if it fails repeatedly (e.g., permission issue, filesystem error), a 0600 dotfile hard link remains in the 0700 state directory. While inert, it represents a cleanup gap.
Impact: Persistent .tmp files accumulate; cleanup only attempted once.
Recommended action: Move cleanup to retry loop in finally (3 retries with backoff), or add periodic cleanup on load() that removes stale .tmp files older than threshold (e.g., 24 hours).
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Finally block lines 578-583: single try/catch with no retry. Comment acknowledges persistent failure can leave dotfile.
Missing regression test: Test with unlinkSync mocked to fail 3x then succeed, verifying retry loop cleans up. Or test load() removes .tmp files older than 24h.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Finally block lines 578-583: single try/catch with no retry. Comment acknowledges persistent failure can leave dotfile.
Evidence: Lines 578-583

`PRA-16` Improvement — JSDoc reentrancy claim imprecise — sibling async calls are NOT reentrant

Location: src/lib/state/rebuild-transaction.ts:618
Category: correctness
Problem: JSDoc states 'Reentrancy is intra-process only via AsyncLocalStorage; cross-process callers always contend on the filesystem lock.' However, sibling async calls in the same process are NOT reentrant — they contend on the filesystem lock.
Impact: Misleading documentation could cause callers to assume intra-process concurrency works when it serializes.
Suggested action: Update JSDoc to explicitly state sibling async calls in same process are NOT reentrant — they contend on the filesystem lock.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: JSDoc at lines 616-620. withMcpLifecycleLock uses AsyncLocalStorage for nested calls only; concurrent calls in same process queue on filesystem lock.
Missing regression test: Test verifying two concurrent create() calls in same process serialize (not reentrant).
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Lines 616-620

`PRA-17` Improvement — normalizeRecord is large; extract phase-specific validators

Location: src/lib/state/rebuild-transaction.ts:416
Category: correctness
Problem: normalizeRecord spans ~80 lines mixing version check, transactionId validation, revision, phase/status, receipts cross-phase validation, timestamp validation. Hard to audit and maintain.
Impact: Reduced maintainability and auditability of validation logic.
Suggested action: Extract phase-specific validators as private functions in follow-up PR. Verify all existing tests pass.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Function spans lines 416-495 with multiple validation concerns interleaved.
Missing regression test: All existing tests should pass after refactor; no new test needed.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Lines 416-495

`PRA-18` Improvement — Symlink attack test stubs HOME but not XDG_CONFIG_HOME

Location: src/lib/state/rebuild-transaction.test.ts:366
Category: tests
Problem: The rejectSymlinksOnPath function in config-io.ts uses HOME to scope user-controlled components. Test should use same resolution logic or add variant stubbing XDG_CONFIG_HOME.
Impact: Test may not catch symlink attacks via XDG_CONFIG_HOME if resolution logic differs.
Suggested action: Parameterize test to use same resolution logic as paths.ts, or add comment noting HOME dependency. Consider adding test variant stubbing XDG_CONFIG_HOME.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Test at lines 366-392 uses vi.stubEnv('HOME', root). config-io.ts rejectSymlinksOnPath uses process.env.HOME || os.homedir().
Missing regression test: Add test variant with XDG_CONFIG_HOME set, verifying symlink rejection still works.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Test lines 366-392; config-io.ts rejectSymlinksOnPath

`PRA-19` Improvement — Permission repair test only covers 0o777/0o666

Location: src/lib/state/rebuild-transaction.test.ts:426
Category: tests
Problem: Test only sets 0o777 and 0o666. Does not test other loose permissions (e.g., 0o755, 0o644, 0o775) or group/world writable bits.
Impact: Other loose permission modes may not be repaired.
Suggested action: Parameterize test to cover additional loose permission modes (0o755, 0o644, 0o775, 0o666) verifying all are repaired to 0o700/0o600.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Test at lines 426-436 only sets 0o777 and 0o666.
Missing regression test: Parametrized test with matrix of loose modes verifying repair to 0o700/0o600.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Lines 426-436

`PRA-20` Improvement — Transaction ID generation has no uniqueness test

Location: src/lib/state/rebuild-transaction.ts:652
Category: correctness
Problem: Transaction ID generation uses crypto.randomUUID() with no uniqueness test. While UUIDv4 collision probability is negligible, a test verifying the generator produces unique IDs under load would increase confidence.
Impact: Theoretical collision risk undetected.
Suggested action: Add test generating 10,000 transaction IDs and verifying no duplicates.
Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
Verification: Constructor at line 652: transactionId: () => crypto.randomUUID(). No test for uniqueness.
Missing regression test: Test generating N transaction IDs and verifying Set.size === N.
Done when: The local improvement is applied, or the PR notes why it should be deferred.
Evidence: Line 652

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:06:24Z

E2E Advisor Recommendation

Required E2E: None
Optional E2E: None

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

None. No existing E2E is recommended for this PR because the only runtime source change is a new standalone rebuild transaction state module that is not currently imported by CLI, onboarding, or rebuild user-flow code. The PR adds focused unit and cross-process tests for the new state machine; existing live rebuild E2Es such as sandbox-rebuild/rebuild-openclaw/rebuild-hermes would not exercise this implementation until it is wired into the public rebuild path.

Optional E2E

None.

New E2E recommendations

sandbox-rebuild-lifecycle (high): Once RebuildTransactionStore is wired into the real nemoclaw <sandbox> rebuild path, existing rebuild E2Es should gain or be complemented by crash/resume coverage that leaves a transaction in prepared/old_deleted/replacement_created state and verifies the next CLI invocation repairs or resumes without losing sandbox state or leaking credentials.
- Suggested test: Add a live rebuild transaction recovery E2E that injects failures between backup, old sandbox deletion, replacement creation, and completion, then reruns the public rebuild command and validates state preservation, transaction diagnostics, and cleanup.

github-actions · 2026-07-08T13:06:25Z

E2E Target Recommendation

Required E2E targets: None
Optional E2E targets: None

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

None. The PR adds an isolated rebuild transaction store plus non-E2E tests/docs, but the new store is not wired into the live rebuild/onboarding paths or the E2E target workflow. No dispatchable E2E target would exercise this changed surface yet.

Optional E2E targets

None.

Relevant changed files

src/lib/state/rebuild-transaction.ts

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

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Source-of-truth review needed: Custom RebuildTransactionStore state root validation.
Open items: 0 required · 3 warnings · 0 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 2 still apply · 1 new item 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	security	`src/lib/state/rebuild-transaction.ts:142`	For RebuildTransactionStore state roots, validate every existing path component from an appropriate trusted anchor to stateDir, or reject custom stateDir values whose ancestors cannot be proven non-symlink directories. Revalidate this root before load/create/mutation as well, so a post-constructor swap cannot bypass the boundary. Keep the existing final-root, leaf-directory, O_NOFOLLOW, hashed-path, 0700/0600, and schema-validation defenses.
`PRA-3`	Resolve/justify	architecture	`src/lib/state/rebuild-transaction.ts:1`	Either extract cohesive private helpers such as a schema codec and publication/storage helper, or document why keeping these concerns together is necessary for this security boundary. Do not move or remove trust-boundary checks unless the extracted helper owns and tests the same validation contract.

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: Custom RebuildTransactionStore state root validation

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: Missing outside-HOME symlinked-ancestor and post-constructor swap tests; existing tests cover only final-root symlinks and HOME-scoped transaction subpath symlinks.
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: ensureTransactionStateRoot() only lstatSyncs stateDir before ensureConfigDir(); config-io.ts returns from rejectSymlinksOnPath() when dirPath is outside HOME.

`PRA-2` Resolve/justify — Reject symlinked ancestors for custom rebuild state roots

Location: src/lib/state/rebuild-transaction.ts:142
Category: security
Problem: The constructor hardens only the final configured stateDir with lstatSync(stateDir), then calls ensureConfigDir(stateDir). For paths outside HOME, ensureConfigDir's rejectSymlinksOnPath() intentionally returns without walking ancestors, so a custom root such as /tmp/link/state can be accepted when /tmp/link is a symlink into an attacker-controlled tree. The same stateDir is later used for both rebuild transaction records and mcp-lifecycle-locks.
Impact: A local attacker or compromised same-host process that controls a symlinked ancestor of a caller-provided stateDir can redirect, deny, or tamper with the transaction and lifecycle-lock tree that future destructive rebuild/recovery code will trust.
Recommended action: For RebuildTransactionStore state roots, validate every existing path component from an appropriate trusted anchor to stateDir, or reject custom stateDir values whose ancestors cannot be proven non-symlink directories. Revalidate this root before load/create/mutation as well, so a post-constructor swap cannot bypass the boundary. Keep the existing final-root, leaf-directory, O_NOFOLLOW, hashed-path, 0700/0600, and schema-validation defenses.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read ensureTransactionStateRoot() in src/lib/state/rebuild-transaction.ts and rejectSymlinksOnPath() in src/lib/state/config-io.ts; confirm that only fs.lstatSync(stateDir) is checked for a custom outside-HOME root and that rejectSymlinksOnPath() returns when path.relative(HOME, stateDir) escapes HOME.
Missing regression test: Add a runtime test that stubs HOME to an unrelated directory, creates realRoot/attacker and realRoot/link -> attacker, sets stateDir = realRoot/link/state, and asserts new RebuildTransactionStore({ stateDir }) or the first create/load fails closed before creating rebuild-transactions or mcp-lifecycle-locks. Add a second test that replaces an accepted custom stateDir with a symlink after construction and asserts create, load, transition, recordFailure, and complete fail closed if operation-time revalidation is added.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read ensureTransactionStateRoot() in src/lib/state/rebuild-transaction.ts and rejectSymlinksOnPath() in src/lib/state/config-io.ts; confirm that only fs.lstatSync(stateDir) is checked for a custom outside-HOME root and that rejectSymlinksOnPath() returns when path.relative(HOME, stateDir) escapes HOME.
Evidence: The existing test "secures a custom state root and rejects a symlinked root" covers loose final-root permissions and a final-component symlink. The existing test "rejects symlinked transaction directories and record files" explicitly stubs HOME so rejectSymlinksOnPath() walks HOME-scoped components, but no changed test covers a custom stateDir outside HOME with a symlinked ancestor.

`PRA-3` Resolve/justify — Split or justify the 840-line transaction store hotspot

Location: src/lib/state/rebuild-transaction.ts:1
Category: architecture
Problem: This PR introduces an 840-line source file that combines state-root trust decisions, schema normalization, durable publication, file reads, transaction state transitions, diagnostics, and public API methods. The large single module makes it harder to audit the host-state security boundary that this PR depends on.
Impact: Future changes to serialization, publication, or lifecycle-lock behavior are more likely to accidentally weaken security checks because unrelated responsibilities are interleaved in one large file.
Recommended action: Either extract cohesive private helpers such as a schema codec and publication/storage helper, or document why keeping these concerns together is necessary for this security boundary. Do not move or remove trust-boundary checks unless the extracted helper owns and tests the same validation contract.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect src/lib/state/rebuild-transaction.ts and confirm whether schema validation, durable publication, and state-machine logic remain interleaved in one 840-line file after the final patch.
Missing regression test: Existing behavior tests should remain unchanged after extraction: round-trip every phase, corruption/future-version fail-closed, atomic publication failure preserves the prior record, symlink/path protections, redaction, and same/cross-process revision races.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect src/lib/state/rebuild-transaction.ts and confirm whether schema validation, durable publication, and state-machine logic remain interleaved in one 840-line file after the final patch.
Evidence: The diff adds src/lib/state/rebuild-transaction.ts as a new 840-line file; the drift scan flagged it as a large-file hotspot.

💡 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: 1 possible cut

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

PRA-3 shrink (src/lib/state/rebuild-transaction.ts:1): Cut the monolithic store shape by extracting pure schema normalization/validation and durable publication helpers from RebuildTransactionStore.
- Replacement: Keep RebuildTransactionStore focused on public API/state-machine orchestration, with storage and codec helpers retaining the same fail-closed validations and tests.
- Safety boundary: Do not simplify away stateDir validation, symlink rejection, O_NOFOLLOW reads, 0700/0600 permissions, atomic create-only publication, revision checks, or redacted diagnostics.

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 — Custom stateDir outside HOME with a symlinked ancestor fails closed before creating or loading rebuild-transactions and mcp-lifecycle-locks.. The PR already adds substantial unit, filesystem, and cross-process tests, but this is a host-state/sandbox lifecycle boundary and one custom-root symlink scenario remains untested.
PRA-T2 Runtime validation — Replacing an accepted custom stateDir with a symlink after store construction causes create, load, transition, recordFailure, and complete to fail closed before trusting state.. The PR already adds substantial unit, filesystem, and cross-process tests, but this is a host-state/sandbox lifecycle boundary and one custom-root symlink scenario remains untested.
PRA-T3 Runtime validation — Custom outside-HOME symlinked rebuild-transactions and mcp-lifecycle-locks subdirectories are rejected, not only HOME-scoped symlinked subdirectories.. The PR already adds substantial unit, filesystem, and cross-process tests, but this is a host-state/sandbox lifecycle boundary and one custom-root symlink scenario remains untested.
PRA-T4 Acceptance clause — Store one active record per sandbox under a host-owned NemoClaw state directory with directory mode `0700` and file mode `0600`. — add test evidence or identify existing coverage. Create-only hard-link publication rejects a second active transaction, and tests assert the transaction directory is 0700 and record file is 0600. The host-owned custom state-root boundary remains incomplete for outside-HOME symlinked ancestors, tracked by the security finding.
PRA-T5 Acceptance clause — Reject path traversal, symlink redirection, transaction/sandbox identity mismatch, invalid phase/status combinations, and malformed timestamps/revisions. — add test evidence or identify existing coverage. Tests cover traversal-shaped sandbox names, symlinked transaction directories/files, copied record under another sandbox key, invalid phase/status, malformed timestamps, and bad revisions. Symlink redirection remains incomplete for custom stateDir ancestors outside HOME, tracked by the security finding.
PRA-T6 Acceptance clause — Verify file/directory permissions and symlink/path protections on supported local filesystems. — add test evidence or identify existing coverage. Tests cover final root repair/rejection, transaction leaf/file modes, loose leaf/file repair, symlinked transaction directories/files under HOME, and traversal names. A custom stateDir outside HOME with a symlinked ancestor is not covered.
PRA-T7 Acceptance clause — Transaction paths and permissions meet the same host-state security expectations as registry/session files. — add test evidence or identify existing coverage. Hashed paths, no-follow reads, 0700/0600 permissions, and HOME-scoped symlink protections are tested. Custom roots outside HOME remain vulnerable to symlinked ancestors.
PRA-T8 Acceptance clause — `npm run typecheck:cli`, the targeted `cli` tests, repository checks, and coverage ratchet pass. — add test evidence or identify existing coverage. The PR body claims verification commands, but this advisory review did not execute commands or evaluate external check surfaces.

Since last review details

Current findings, using the urgency labels above:

`PRA-1` Resolve/justify — Source-of-truth review needed: Custom RebuildTransactionStore state root validation

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: Missing outside-HOME symlinked-ancestor and post-constructor swap tests; existing tests cover only final-root symlinks and HOME-scoped transaction subpath symlinks.
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: ensureTransactionStateRoot() only lstatSyncs stateDir before ensureConfigDir(); config-io.ts returns from rejectSymlinksOnPath() when dirPath is outside HOME.

`PRA-2` Resolve/justify — Reject symlinked ancestors for custom rebuild state roots

Location: src/lib/state/rebuild-transaction.ts:142
Category: security
Problem: The constructor hardens only the final configured stateDir with lstatSync(stateDir), then calls ensureConfigDir(stateDir). For paths outside HOME, ensureConfigDir's rejectSymlinksOnPath() intentionally returns without walking ancestors, so a custom root such as /tmp/link/state can be accepted when /tmp/link is a symlink into an attacker-controlled tree. The same stateDir is later used for both rebuild transaction records and mcp-lifecycle-locks.
Impact: A local attacker or compromised same-host process that controls a symlinked ancestor of a caller-provided stateDir can redirect, deny, or tamper with the transaction and lifecycle-lock tree that future destructive rebuild/recovery code will trust.
Recommended action: For RebuildTransactionStore state roots, validate every existing path component from an appropriate trusted anchor to stateDir, or reject custom stateDir values whose ancestors cannot be proven non-symlink directories. Revalidate this root before load/create/mutation as well, so a post-constructor swap cannot bypass the boundary. Keep the existing final-root, leaf-directory, O_NOFOLLOW, hashed-path, 0700/0600, and schema-validation defenses.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Read ensureTransactionStateRoot() in src/lib/state/rebuild-transaction.ts and rejectSymlinksOnPath() in src/lib/state/config-io.ts; confirm that only fs.lstatSync(stateDir) is checked for a custom outside-HOME root and that rejectSymlinksOnPath() returns when path.relative(HOME, stateDir) escapes HOME.
Missing regression test: Add a runtime test that stubs HOME to an unrelated directory, creates realRoot/attacker and realRoot/link -> attacker, sets stateDir = realRoot/link/state, and asserts new RebuildTransactionStore({ stateDir }) or the first create/load fails closed before creating rebuild-transactions or mcp-lifecycle-locks. Add a second test that replaces an accepted custom stateDir with a symlink after construction and asserts create, load, transition, recordFailure, and complete fail closed if operation-time revalidation is added.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Read ensureTransactionStateRoot() in src/lib/state/rebuild-transaction.ts and rejectSymlinksOnPath() in src/lib/state/config-io.ts; confirm that only fs.lstatSync(stateDir) is checked for a custom outside-HOME root and that rejectSymlinksOnPath() returns when path.relative(HOME, stateDir) escapes HOME.
Evidence: The existing test "secures a custom state root and rejects a symlinked root" covers loose final-root permissions and a final-component symlink. The existing test "rejects symlinked transaction directories and record files" explicitly stubs HOME so rejectSymlinksOnPath() walks HOME-scoped components, but no changed test covers a custom stateDir outside HOME with a symlinked ancestor.

`PRA-3` Resolve/justify — Split or justify the 840-line transaction store hotspot

Location: src/lib/state/rebuild-transaction.ts:1
Category: architecture
Problem: This PR introduces an 840-line source file that combines state-root trust decisions, schema normalization, durable publication, file reads, transaction state transitions, diagnostics, and public API methods. The large single module makes it harder to audit the host-state security boundary that this PR depends on.
Impact: Future changes to serialization, publication, or lifecycle-lock behavior are more likely to accidentally weaken security checks because unrelated responsibilities are interleaved in one large file.
Recommended action: Either extract cohesive private helpers such as a schema codec and publication/storage helper, or document why keeping these concerns together is necessary for this security boundary. Do not move or remove trust-boundary checks unless the extracted helper owns and tests the same validation contract.
Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
Verification: Inspect src/lib/state/rebuild-transaction.ts and confirm whether schema validation, durable publication, and state-machine logic remain interleaved in one 840-line file after the final patch.
Missing regression test: Existing behavior tests should remain unchanged after extraction: round-trip every phase, corruption/future-version fail-closed, atomic publication failure preserves the prior record, symlink/path protections, redaction, and same/cross-process revision races.
Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect src/lib/state/rebuild-transaction.ts and confirm whether schema validation, durable publication, and state-machine logic remain interleaved in one 840-line file after the final patch.
Evidence: The diff adds src/lib/state/rebuild-transaction.ts as a new 840-line file; the drift scan flagged it as a large-file hotspot.

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.

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

jyaunches · 2026-07-08T16:31:42Z

Advisor follow-up at `417beffe8`

All required findings and warnings from the prior advisor pass are addressed or explicitly resolved as follows:

Create-only publication / EXDEV: candidate and canonical names are constructed in the same dirPath, so link(2) is same-filesystem by construction. The redundant device stat precheck was removed. Injected EXDEV fails closed with an invariant error and cleans the candidate. A copy/rename fallback is intentionally rejected because Node does not expose renameat2(RENAME_NOREPLACE) and ordinary copy/rename would allow a competing creator to overwrite the winner. The invariant, regression evidence, constraint, and removal condition are now documented in code and src/lib/state/README.md.
macOS durability: Apple documents fsync and the stricter F_FULLFSYNC; Node exposes fsyncSync but not fcntl(F_FULLFSYNC). Strict sudden-power-loss durability on macOS is now explicitly unsupported/best-effort in the store API documentation and state-module README. The filesystem boundary, untestable physical-fault condition, and removal condition are documented beside syncDirectory.
Reentrancy: JSDoc now states verbatim that reentrancy is intra-process only through AsyncLocalStorage, while cross-process callers always contend on the filesystem lock.
Persistent temp cleanup failure: documented as an inert 0600 dotfile hard link inside the 0700 state directory; only the canonical hashed path is loaded. Transient cleanup retry remains tested.
Concurrency coverage: same-process tests exercise transition, failure, and completion through the shared mutation-lock path; the real two-process test exercises the filesystem-lock/revision boundary. Create-only EEXIST and injected EXDEV behavior are covered separately. Additional process permutations would repeat those same primitives and are not required for this foundation PR.
HOME/XDG: resolveNemoclawStateDir is intentionally HOME-scoped and does not consult XDG_CONFIG_HOME; the symlink test matches the production resolver.
Permissions: the repair regression now starts from maximally loose 0777/0666 modes.
Size/extraction suggestions: phase validation remains cohesive with the versioned V1 normalizer for this foundation. The next stacked PR (refactor(rebuild): journal destructive recovery #6470) performs the pipeline integration and extracts transaction coordination; shared validation will be extracted when a second schema/consumer establishes a reusable boundary.

Verification: 30 focused CLI tests, the real cross-process integration test, Markdown lint, and npm run check:diff pass. The PR remains draft.

jyaunches · 2026-07-08T16:58:42Z

Final advisor disposition at `ad173d08f`

The latest actionable GPT findings are implemented: mutation-input failures are now classified separately from corrupt persisted state; both documented backup timestamp shapes have explicit tests and source/removal documentation; schema-validation tests are split from lifecycle/security tests (356 and 131 lines respectively).

The remaining Nemotron “required” platform findings are explicitly overridden for these reasons:

The EXDEV scenario assumes the candidate is created in /tmp; it is actually created directly in the canonical record directory. Both hard-link names therefore share a filesystem by construction. Injected EXDEV fails closed. A copy/rename fallback would lose atomic no-replace semantics because Node does not expose renameat2(RENAME_NOREPLACE).
macOS strict sudden-power-loss durability is explicitly unsupported in the class contract and src/lib/state/README.md, which is one of the advisor's stated resolution options. Node exposes fsyncSync, not fcntl(F_FULLFSYNC); adding a native dependency is outside this foundation PR.
A persistent candidate cleanup failure can leave only a 0600 dotfile hard link inside the 0700 state directory. The canonical inode is already published and synced; only the hashed canonical path is loaded.
The store implementation remains cohesive around one V1 schema and one publication primitive. Splitting it now would require moving the public contract/error types or introducing circular runtime dependencies. Validation and test concerns have been separated; implementation-helper extraction remains a bounded follow-up once V2 or a second consumer establishes a stable boundary.

Cross-process coverage exercises the common filesystem-lock/revision primitive; intra-process tests cover transition, failure, and completion, while create separately covers atomic EEXIST and fail-closed EXDEV. The PR remains draft.

jyaunches · 2026-07-08T17:09:41Z

Custom-root boundary rationale at `b6c333b83`

The final GPT pass has zero required findings. For its remaining custom-root ancestor warning: RebuildTransactionStoreOptions.stateDir is an internal/test dependency-injection seam, not CLI/user input. The production default remains beneath HOME, where ensureConfigDir walks and rejects every user-controlled symlink component. For injected roots, the store now rejects a symlinked/non-directory root, creates or repairs it to 0700, and applies the existing no-follow/hashed-path/0600 leaf controls.

Walking from filesystem root for arbitrary injected paths would reject legitimate platform symlinks (notably macOS /var to /private/var) without establishing a stronger portable trust anchor. The caller supplying the injection remains responsible for the ancestors; the store owns and validates the injected root downward. The regression covers loose-root repair and direct-root symlink rejection. This is the explicit source boundary and removal condition: broaden ancestor enforcement if this option ever becomes public/user-controlled or the platform layer supplies a portable trusted-root primitive.

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

Exact-head advisor disposition at `21467fd`

GPT reports zero required findings. Its remaining custom-root warning is covered by the explicit internal dependency-injection trust boundary and removal condition in the prior maintainer rationale; the implementation validates the injected root downward while production HOME resolution validates user-controlled ancestors. The module split remains the bounded V1 follow-up already recorded.

Nemotron repeated the same overridden EXDEV, macOS F_FULLFSYNC, and immediate module-split requests. Candidate publication is same-directory by construction and injected EXDEV fails closed; strict sudden-power-loss durability on macOS is outside the documented contract; the cohesive V1 store remains intact for this security review. No new correctness finding remains on this head. The PR remains draft.

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

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

refactor(rebuild): add durable transaction store

f7f9e4a

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

jyaunches added 3 commits July 8, 2026 09:10

fix(rebuild): enforce transaction revision checks

bf6b29b

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

fix(rebuild): accept persisted backup identifiers

50e03b4

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

refactor(rebuild): keep registry outside transaction receipts

18e0034

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

jyaunches mentioned this pull request Jul 8, 2026

refactor(rebuild): journal destructive recovery #6470

Closed

fix(rebuild): serialize transaction mutations

59e2b99

github-code-quality Bot found potential problems Jul 8, 2026

View reviewed changes

Comment thread src/lib/state/rebuild-transaction.test.ts Fixed

github-advanced-security Bot found potential problems Jul 8, 2026

View reviewed changes

Comment thread src/lib/state/rebuild-transaction.test.ts Fixed

jyaunches added 4 commits July 8, 2026 12:17

fix(rebuild): validate transaction receipts

42a356a

test(rebuild): keep process race linear

b98da84

test(rebuild): remove stale fixture import

f7eccf4

docs(rebuild): define durability boundaries

417beff

jyaunches added 3 commits July 8, 2026 12:34

fix(rebuild): require canonical receipt timestamps

6c5c518

feat(rebuild): persist legacy recovery authority

7ec1122

refactor(rebuild): isolate record validation tests

ad173d0

fix(rebuild): secure custom state roots

b6c333b

feat(rebuild): record original shields posture

21467fd

jyaunches closed this Jul 10, 2026

jyaunches deleted the refactor/6434-rebuild-transaction-store 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!

Summary

Related Issue

Changes

Advisor resolution

Type of Change

Quality Gates

Verification

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!

PR Review Advisor (Nemotron Ultra) — Blocked

Action checklist

Findings index

🚨 Required before merge

PRA-2 Required — EXDEV cross-device link failure has no copy+rename fallback

PRA-3 Required — macOS directory fsync lacks F_FULLFSYNC; no runtime warning for best-effort durability

PRA-4 Required — Monolith growth: 840-line file mixing types, validation, durable I/O, and store class

PRA-5 Required — EXDEV throws plain Error not RebuildTransactionError — callers cannot handle programmatically

⚠️ Resolve or justify before merge

PRA-1 Resolve/justify — Source-of-truth review needed: Persistent .tmp file leak if unlinkSync fails repeatedly

PRA-6 Resolve/justify — credentialEnv validation only checks ENV_NAME_PATTERN, no allowlist of known credential variables

PRA-7 Resolve/justify — gatewayName is free-form string with no allowlist or injection prevention

PRA-8 Resolve/justify — Diagnostic projection test incomplete — does not verify redaction of all sensitive fields

PRA-9 Resolve/justify — candidatePath uses predictable PID + randomUUID but no O_TMPFILE on Linux

PRA-10 Resolve/justify — Cross-process concurrency test only covers recordFailure race

PRA-11 Resolve/justify — Overlapping PR #6470 modifies same file — integration risk

PRA-12 Resolve/justify — Source-of-truth: EXDEV cross-device link failure handling — documented as invariant failure but copy+rename fallback feasible

PRA-13 Resolve/justify — Source-of-truth: macOS directory fsync durability — documented but no runtime warning

PRA-14 Resolve/justify — Source-of-truth: temporary .tmp file cleanup best-effort — no periodic cleanup or retry loop

PRA-15 Resolve/justify — Source-of-truth: .tmp file leak if linkSync succeeds but unlinkSync fails persistently

PRA-23 Resolve/justify — Diagnostic test injects non-existent fields via type assertion — tests redaction of fields that would be stripped anyway

💡 In-scope improvements

PRA-16 Improvement — JSDoc reentrancy claim imprecise — sibling async calls are NOT reentrant

PRA-17 Improvement — normalizeRecord is large; extract phase-specific validators

PRA-18 Improvement — Symlink attack test stubs HOME but not XDG_CONFIG_HOME

PRA-19 Improvement — Permission repair test only covers 0o777/0o666

PRA-20 Improvement — Transaction ID generation has no uniqueness test

PRA-21 Improvement — RebuildTransactionStore not yet integrated into rebuild-pipeline

PRA-22 Improvement — tempDir uses os.tmpdir() which may be cross-device from stateDir in production

PRA-1 Resolve/justify — Source-of-truth review needed: Persistent .tmp file leak if unlinkSync fails repeatedly

PRA-2 Required — EXDEV cross-device link failure has no copy+rename fallback

PRA-3 Required — macOS directory fsync lacks F_FULLFSYNC; no runtime warning for best-effort durability

PRA-4 Required — Monolith growth: 840-line file mixing types, validation, durable I/O, and store class

PRA-5 Required — EXDEV throws plain Error not RebuildTransactionError — callers cannot handle programmatically

PRA-6 Resolve/justify — credentialEnv validation only checks ENV_NAME_PATTERN, no allowlist of known credential variables

PRA-7 Resolve/justify — gatewayName is free-form string with no allowlist or injection prevention

PRA-8 Resolve/justify — Diagnostic projection test incomplete — does not verify redaction of all sensitive fields

PRA-9 Resolve/justify — candidatePath uses predictable PID + randomUUID but no O_TMPFILE on Linux

PRA-10 Resolve/justify — Cross-process concurrency test only covers recordFailure race

PRA-11 Resolve/justify — Overlapping PR #6470 modifies same file — integration risk

PRA-12 Resolve/justify — Source-of-truth: EXDEV cross-device link failure handling — documented as invariant failure but copy+rename fallback feasible

PRA-13 Resolve/justify — Source-of-truth: macOS directory fsync durability — documented but no runtime warning

PRA-14 Resolve/justify — Source-of-truth: temporary .tmp file cleanup best-effort — no periodic cleanup or retry loop

PRA-15 Resolve/justify — Source-of-truth: .tmp file leak if linkSync succeeds but unlinkSync fails persistently

PRA-16 Improvement — JSDoc reentrancy claim imprecise — sibling async calls are NOT reentrant

PRA-17 Improvement — normalizeRecord is large; extract phase-specific validators

PRA-18 Improvement — Symlink attack test stubs HOME but not XDG_CONFIG_HOME

PRA-19 Improvement — Permission repair test only covers 0o777/0o666

PRA-20 Improvement — Transaction ID generation has no uniqueness test

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

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

`PRA-2` Required — EXDEV cross-device link failure has no copy+rename fallback

`PRA-3` Required — macOS directory fsync lacks F_FULLFSYNC; no runtime warning for best-effort durability

`PRA-4` Required — Monolith growth: 840-line file mixing types, validation, durable I/O, and store class

`PRA-5` Required — EXDEV throws plain Error not RebuildTransactionError — callers cannot handle programmatically

`PRA-1` Resolve/justify — Source-of-truth review needed: Persistent .tmp file leak if unlinkSync fails repeatedly

`PRA-6` Resolve/justify — credentialEnv validation only checks ENV_NAME_PATTERN, no allowlist of known credential variables

`PRA-7` Resolve/justify — gatewayName is free-form string with no allowlist or injection prevention

`PRA-8` Resolve/justify — Diagnostic projection test incomplete — does not verify redaction of all sensitive fields

`PRA-9` Resolve/justify — candidatePath uses predictable PID + randomUUID but no O_TMPFILE on Linux

`PRA-10` Resolve/justify — Cross-process concurrency test only covers recordFailure race

`PRA-11` Resolve/justify — Overlapping PR #6470 modifies same file — integration risk

`PRA-12` Resolve/justify — Source-of-truth: EXDEV cross-device link failure handling — documented as invariant failure but copy+rename fallback feasible

`PRA-13` Resolve/justify — Source-of-truth: macOS directory fsync durability — documented but no runtime warning

`PRA-14` Resolve/justify — Source-of-truth: temporary .tmp file cleanup best-effort — no periodic cleanup or retry loop

`PRA-15` Resolve/justify — Source-of-truth: .tmp file leak if linkSync succeeds but unlinkSync fails persistently

`PRA-23` Resolve/justify — Diagnostic test injects non-existent fields via type assertion — tests redaction of fields that would be stripped anyway

`PRA-16` Improvement — JSDoc reentrancy claim imprecise — sibling async calls are NOT reentrant

`PRA-17` Improvement — normalizeRecord is large; extract phase-specific validators

`PRA-18` Improvement — Symlink attack test stubs HOME but not XDG_CONFIG_HOME

`PRA-19` Improvement — Permission repair test only covers 0o777/0o666

`PRA-20` Improvement — Transaction ID generation has no uniqueness test

`PRA-21` Improvement — RebuildTransactionStore not yet integrated into rebuild-pipeline

`PRA-22` Improvement — tempDir uses os.tmpdir() which may be cross-device from stateDir in production

`PRA-1` Resolve/justify — Source-of-truth review needed: Persistent .tmp file leak if unlinkSync fails repeatedly

`PRA-2` Required — EXDEV cross-device link failure has no copy+rename fallback

`PRA-3` Required — macOS directory fsync lacks F_FULLFSYNC; no runtime warning for best-effort durability

`PRA-4` Required — Monolith growth: 840-line file mixing types, validation, durable I/O, and store class

`PRA-5` Required — EXDEV throws plain Error not RebuildTransactionError — callers cannot handle programmatically

`PRA-6` Resolve/justify — credentialEnv validation only checks ENV_NAME_PATTERN, no allowlist of known credential variables

`PRA-7` Resolve/justify — gatewayName is free-form string with no allowlist or injection prevention

`PRA-8` Resolve/justify — Diagnostic projection test incomplete — does not verify redaction of all sensitive fields

`PRA-9` Resolve/justify — candidatePath uses predictable PID + randomUUID but no O_TMPFILE on Linux

`PRA-10` Resolve/justify — Cross-process concurrency test only covers recordFailure race

`PRA-11` Resolve/justify — Overlapping PR #6470 modifies same file — integration risk

`PRA-12` Resolve/justify — Source-of-truth: EXDEV cross-device link failure handling — documented as invariant failure but copy+rename fallback feasible

`PRA-13` Resolve/justify — Source-of-truth: macOS directory fsync durability — documented but no runtime warning

`PRA-14` Resolve/justify — Source-of-truth: temporary .tmp file cleanup best-effort — no periodic cleanup or retry loop

`PRA-15` Resolve/justify — Source-of-truth: .tmp file leak if linkSync succeeds but unlinkSync fails persistently

`PRA-16` Improvement — JSDoc reentrancy claim imprecise — sibling async calls are NOT reentrant

`PRA-17` Improvement — normalizeRecord is large; extract phase-specific validators

`PRA-18` Improvement — Symlink attack test stubs HOME but not XDG_CONFIG_HOME

`PRA-19` Improvement — Permission repair test only covers 0o777/0o666

`PRA-20` Improvement — Transaction ID generation has no uniqueness test

github-actions 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-1` Resolve/justify — Source-of-truth review needed: Custom RebuildTransactionStore state root validation

`PRA-2` Resolve/justify — Reject symlinked ancestors for custom rebuild state roots

`PRA-3` Resolve/justify — Split or justify the 840-line transaction store hotspot

`PRA-1` Resolve/justify — Source-of-truth review needed: Custom RebuildTransactionStore state root validation

`PRA-2` Resolve/justify — Reject symlinked ancestors for custom rebuild state roots

`PRA-3` Resolve/justify — Split or justify the 840-line transaction store hotspot

Advisor follow-up at `417beffe8`

Final advisor disposition at `ad173d08f`

Custom-root boundary rationale at `b6c333b83`

Exact-head advisor disposition at `21467fd`