fix(rebuild): emit actionable diagnostic on all preflight failures (#7794) - #7812
fix(rebuild): emit actionable diagnostic on all preflight failures (#7794)#7812kagura-agent wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesRebuild preflight diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/rebuild-preflight-error.test.ts`:
- Line 8: Update the describe suite title for printRebuildPreflightFailure to
append the linked issue suffix (`#7794`), preserving the existing title text.
- Around line 18-22: Extend the diagnostic assertions in the rebuild preflight
error test to verify the required sandbox-safety guarantee, matching the
user-facing text that sandbox is untouched and no data was lost. Keep the
existing “Aborting rebuild” and bail assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0a32c41-0151-4462-9e27-ee25a7edd5d6
📒 Files selected for processing (5)
src/lib/actions/sandbox/rebuild-preflight-confirmation.test.tssrc/lib/actions/sandbox/rebuild-preflight-error.test.tssrc/lib/actions/sandbox/rebuild-preflight-error.tssrc/lib/actions/sandbox/rebuild-preflight-guards.tssrc/lib/actions/sandbox/rebuild-preflight-phase.ts
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: Blockers
|
wscurran
added
area: cli
878fbaf to
2530f58
Compare
|
Rebased on latest upstream/main to resolve merge conflict. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/actions/sandbox/rebuild-preflight-guards.ts (1)
312-333: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing early return after failed lock acquisition lets a non-acquired lock still be "released".
Unlike the other three updated guards in this file (each returns its sentinel immediately after
printRebuildPreflightFailure),acquireRebuildOnboardLockfalls through when!lock.acquiredand still returns a workingreleaseclosure. Ifbaildoesn't synchronously terminate execution (e.g., a test double, or a future refactor ofbail), the caller ends up holding areleasefunction that will callonboardSession.releaseOnboardLock()for a lock this call never actually acquired — potentially releasing the other in-progress run's lock.🔒 Proposed fix: return a no-op release on failure
if (!lock.acquired) { const pidDetail = lock.holderPid ? ` Lock holder PID: ${lock.holderPid}.` : ""; printRebuildPreflightFailure( `another ${CLI_NAME} onboarding run is already in progress.`, `Wait for the other run to finish or remove the stale lock.${pidDetail}`, "Could not acquire onboard lock before rebuild", bail, ); + return () => {}; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/rebuild-preflight-guards.ts` around lines 312 - 333, Update acquireRebuildOnboardLock so the !lock.acquired branch returns a no-op release function immediately after printRebuildPreflightFailure. Ensure the normal released-guarded closure and process.once("exit", release) registration remain limited to successfully acquired locks.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/rebuild-preflight-guards.ts (1)
39-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving the pure route-decision helpers to a domain module.
normalizedRoute,missingCredentialIdentity,sameRoute, andhardRouteConflictare pure decision functions with no host/process/registry I/O — they mostly wrap../../inference/gateway-route-compatibilityand../../inference/selection. Per the layering rules, domain modules should own pure decisions while thisactions/sandboxfile should stay focused on orchestration (locking, load/save). Extracting these helpers alongsidegateway-route-compatibility.tswould tighten the dependency direction.As per path instructions, "
src/lib/{actions,domain,adapters,state}/**: ... domain modules make pure decisions ... Flag cross-layer cycles, duplicate sources of truth, and forwarding wrappers that add a new layer without retiring the old owner."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/rebuild-preflight-guards.ts` around lines 39 - 94, Move the pure helpers normalizedRoute, missingCredentialIdentity, sameRoute, and hardRouteConflict out of rebuild-preflight-guards.ts into the domain inference module alongside gateway-route-compatibility.ts, preserving their current behavior and dependencies. Update the sandbox orchestration code to import and use the extracted domain symbols, removing the local definitions so there is one source of truth.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/actions/sandbox/rebuild-preflight-guards.ts`:
- Around line 312-333: Update acquireRebuildOnboardLock so the !lock.acquired
branch returns a no-op release function immediately after
printRebuildPreflightFailure. Ensure the normal released-guarded closure and
process.once("exit", release) registration remain limited to successfully
acquired locks.
---
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-preflight-guards.ts`:
- Around line 39-94: Move the pure helpers normalizedRoute,
missingCredentialIdentity, sameRoute, and hardRouteConflict out of
rebuild-preflight-guards.ts into the domain inference module alongside
gateway-route-compatibility.ts, preserving their current behavior and
dependencies. Update the sandbox orchestration code to import and use the
extracted domain symbols, removing the local definitions so there is one source
of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3d01f3f6-40d7-4c8d-9ebc-e5a2388e7a85
📒 Files selected for processing (5)
src/lib/actions/sandbox/rebuild-preflight-confirmation.test.tssrc/lib/actions/sandbox/rebuild-preflight-error.test.tssrc/lib/actions/sandbox/rebuild-preflight-error.tssrc/lib/actions/sandbox/rebuild-preflight-guards.tssrc/lib/actions/sandbox/rebuild-preflight-phase.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/rebuild-preflight-error.ts
2530f58 to
bb8d34d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/rebuild-preflight-guards.test.ts (1)
143-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the lock-specific diagnostic and bail message.
The test only checks shared safety text. It does not check the lock holder PID, remediation text, or the bail message. A regression that removes these lock-specific values can pass.
Proposed test assertions
expect(output).toContain("Aborting rebuild"); expect(output).toContain("sandbox is untouched, no data was lost."); + expect(output).toContain("Wait for the other run to finish or remove the stale lock."); + expect(output).toContain("Lock holder PID: 12345."); + expect(bail).toHaveBeenCalledWith("Could not acquire onboard lock before rebuild");As per path instructions, tests must validate observable outcomes through the public boundary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/rebuild-preflight-guards.test.ts` around lines 143 - 158, Strengthen the test for acquireRebuildOnboardLock so it asserts the lock-holder PID and remediation guidance in console.error, and verifies bail was called with the expected lock-specific message and exit code. Keep the assertions through the public acquireRebuildOnboardLock boundary and retain the existing safety-text checks.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild-preflight-guards.test.ts`:
- Around line 143-158: Strengthen the test for acquireRebuildOnboardLock so it
asserts the lock-holder PID and remediation guidance in console.error, and
verifies bail was called with the expected lock-specific message and exit code.
Keep the assertions through the public acquireRebuildOnboardLock boundary and
retain the existing safety-text checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0d80fdf-7a60-4c66-b9bf-98e536b5464b
📒 Files selected for processing (7)
src/lib/actions/sandbox/rebuild-preflight-confirmation.test.tssrc/lib/actions/sandbox/rebuild-preflight-error.test.tssrc/lib/actions/sandbox/rebuild-preflight-error.tssrc/lib/actions/sandbox/rebuild-preflight-guards.test.tssrc/lib/actions/sandbox/rebuild-preflight-guards.tssrc/lib/actions/sandbox/rebuild-preflight-phase.tssrc/lib/actions/sandbox/rebuild-preflight-target-phase.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts
- src/lib/actions/sandbox/rebuild-preflight-error.ts
- src/lib/actions/sandbox/rebuild-preflight-error.test.ts
f82af63 to
c720468
Compare
…lures (NVIDIA#7794) Route all preflight failure paths through printRebuildPreflightFailure so the output consistently includes 'Aborting rebuild', making the diagnostic actionable and matchable by CI assertions. Co-Authored-By: Claude <noreply@anthropic.com>
c720468 to
6de69d1
Compare
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed commit 3692bbaf8. acquireRebuildOnboardLock now routes a failed acquisition through printRebuildPreflightFailure, but it does not return afterward. If the injected bail does not terminate, the function registers an exit handler and returns a release closure for a lock it never acquired. That closure can release another onboarding run's lock. Return a no-op release immediately after the failure helper, and add a public-boundary test that proves the failed path does not register or invoke release while retaining the PID, remediation, safety, and bail diagnostics.
Contributor compliance also blocks approval: the PR body has no valid Signed-off-by: declaration. Add the contributor's declaration. The branch has been updated from main; obtain current required CI/E2E evidence and complete a documentation-writer review receipt for the final commit after the code fix.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the requested lock-failure control-flow issue in the current revision. A failed onboarding-lock acquisition now returns immediately, and the caller stops before target preparation; the regression test proves that no exit handler is registered and no release is invoked for another operation’s lock. I also preserved the requested exit code for pending-policy failures. The focused suite passes 34 tests, CLI type-checking passes, and changed-file hooks pass. The documentation and sensitive-path reviews are recorded in the PR body; fresh CI and E2E are running. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
4 participants
Summary
When
nemoclaw rebuildfails during the preflight phase (e.g. a pending policy transaction or missing sandbox), the command previously exited with code 1 without producing output matching the expected diagnostic pattern/Failed to back up|Aborting rebuild/i. Users only saw the confirmation line and an unexplained exit.Changes
rebuild-preflight-error.ts: Changed the safety line from "Sandbox is untouched — no data was lost." to "Aborting rebuild — sandbox is untouched, no data was lost." so the output matches the CI assertion pattern.rebuild-preflight-guards.ts: Routed all four guard functions throughprintRebuildPreflightFailureinstead of rawconsole.error+bail():getRebuildSandboxEntryOrBailblockRebuildOnPendingBaselineTransitionisSingleAgentRebuildSupportedacquireRebuildOnboardLockrebuild-preflight-phase.ts: TheassertMcpDestroyNotPendingcatch block now routes throughprintRebuildPreflightFailureinstead of callingbail()directly.Gateway schema guard:
checkRebuildGatewaySchemaPreflightnow routes schema incompatibility through the same structured safe-abort diagnostic before bailing.Tests:
rebuild-preflight-error.test.tsverifies the structured diagnostic and exit-code forwarding. The lock-acquisition regression test verifies that a failed acquisition stops immediately without registering or invoking a release for another operation's lock. Gateway-drift coverage verifies the summary, remediation, safe-abort line, and bail reason with a non-throwing bail.Result
Every preflight failure path now emits a structured diagnostic:
This matches the expected
/Aborting rebuild/ipattern and tells users their sandbox data is safe. A failed onboarding-lock acquisition also returns before rebuild target preparation or destructive work.Testing
npx vitest run --project cli src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts src/lib/actions/sandbox/rebuild-preflight-error.test.ts src/lib/actions/sandbox/rebuild-baseline-transition-preflight.test.ts src/lib/actions/sandbox/rebuild-shields-finally.test.ts— 4 files and 21 tests passednpx vitest run --project integration test/mcp-bridge-destroy-marker-recovery.test.ts— 1 file and 13 tests passednpm run typecheck:cli— passedprekhooks — passedCloses #7794
Quality Gates
a15e3c9aae91e4d7decb2162e9f0ce8440f44716against base87fcfe40db6b0fe72622290866d17af701693e8a; all nine repository categories passed with no findings. The change only standardizes failure reporting, preserves fail-closed control flow and exit behavior, performs no mutation before the abort, and adds no credential, dependency, network, authentication, or authorization surface.Documentation Writer Review
no-docs-neededa15e3c9aae91e4d7decb2162e9f0ce8440f44716. The change standardizes existing preflight failure output without changing the incompatibility condition, remediation, command, configuration, mutation boundary, or rebuild workflow. Existing rebuild documentation already covers safe preflight aborts and gateway-drift recovery. Focused gateway-drift tests passed: 7 tests.git diff --checkpassed.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHuba15e3c9aae91e4d7decb2162e9f0ce8440f44716Summary by CodeRabbit
Signed-off-by: kagura-agent kagura.agent.ai@gmail.com