fix(onboard): report an interrupted onboard instead of an invalid transition - #8000
Merged
Conversation
…nsition The incomplete-exit backstop writes a terminal failed state while a step can still be running, so a sandbox that finished successfully then tried a sandbox to openclaw transition and raised an invalid-transition error. The backstop now records the interrupted step, and both the runtime and the boundary report an interrupted onboard that resumes cleanly. The create stream also spawned its child in the CLI process group, so the ready-gate SIGTERM could reach the CLI itself. It now spawns the child in its own group, signals that group, and kills it if the host exits first. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe change records interrupted onboarding failures, blocks continuation from interrupted sessions, and improves sandbox child cleanup. Native non-Windows sandbox processes now run in detached groups. Tests cover interruption state, transition guards, process-group termination, signal handling, and listener cleanup. ChangesOnboarding interruption handling
Sandbox process-group cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RuntimeBoundary
participant OnboardRuntime
participant SessionState
RuntimeBoundary->>OnboardRuntime: submit completion or transition result
OnboardRuntime->>SessionState: read failed state and interruption marker
SessionState-->>OnboardRuntime: return failure metadata
OnboardRuntime-->>RuntimeBoundary: throw OnboardInterruptedError
sequenceDiagram
participant HostProcess
participant CreateStream
participant SandboxProcessGroup
HostProcess->>CreateStream: receive SIGINT, SIGTERM, or exit
CreateStream->>SandboxProcessGroup: terminate child process group
SandboxProcessGroup-->>CreateStream: settle stream
CreateStream->>HostProcess: remove signal and exit listeners
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
laitingsheng
added
NV QA
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit a695af6 in the TypeScript / code-coverage/cliThe overall coverage in commit a695af6 in the Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/onboard/exit-step-failure.test.ts`:
- Around line 121-136: Wrap the listener invocation in the test around
registerIncompleteOnboardExitFailureHandler with a try/finally block, and move
errorSpy.mockRestore() into the finally clause so the console.error spy is
restored even when listeners[0](1) throws.
In `@src/lib/onboard/machine/interrupt-state.ts`:
- Around line 4-10: Persist the interruption marker and interrupted step through
the onboarding session in interrupt-state.ts, and update transitions.ts to throw
OnboardInterruptedError only for failed sessions carrying that durable marker.
In src/lib/onboard/runtime-boundary.test.ts lines 657-664, persist and reload an
interrupted session before asserting the error, and add coverage showing an
ordinary failed session does not throw; ensure resume, persisted-state migration
paths, and public entrypoints use the authoritative persisted state.
In `@src/lib/onboard/runtime-boundary.ts`:
- Around line 139-140: Guard the failed-session fallback in
recordSessionComplete() with assertOnboardNotInterrupted before calling
runtime.completeSession(updates), matching the existing completion transition
checks. Add a boundary test through the public recordSessionComplete()
entrypoint proving an interrupted failed session is rejected and
runtime.completeSession is not invoked.
In `@src/lib/sandbox/create-stream-process-group.test.ts`:
- Around line 92-95: Move the readStartedPids(markerPath) call before the result
assertion in the process-group termination test, so teardown can access recorded
child and grandchild PIDs even when expect(result) fails. Keep the existing
waitForExit checks unchanged.
In `@src/lib/sandbox/create-stream.ts`:
- Around line 348-351: Update terminateChildOnHostExit to also handle SIGINT and
SIGTERM, terminating the owned child group before allowing normal host signal
termination to proceed. Register these handlers alongside the existing exit
handler, remove all of them in finish, and add a regression test covering host
SIGTERM during a pending create.
In `@test/onboard-sandbox-recreation.test.ts`:
- Around line 1197-1202: Update the process.kill mock and related assertions
around process-group termination to record both pid and signal, restrict the
mock or validation to the expected child process group, and assert the payload
targets exactly -child.pid with the expected signal. Apply the same change to
the additional process.kill coverage noted in the comment.
🪄 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: 4e985cc0-2b76-46e8-957e-14b780246f37
📒 Files selected for processing (10)
src/lib/onboard/exit-step-failure.test.tssrc/lib/onboard/exit-step-failure.tssrc/lib/onboard/machine/interrupt-state.tssrc/lib/onboard/machine/runtime.tssrc/lib/onboard/machine/transitions.tssrc/lib/onboard/runtime-boundary.test.tssrc/lib/onboard/runtime-boundary.tssrc/lib/sandbox/create-stream-process-group.test.tssrc/lib/sandbox/create-stream.tstest/onboard-sandbox-recreation.test.ts
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. Since last review: 0 prior items resolved · 0 still apply · 0 new items found E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
…state-race Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
An interrupted run recorded the marker only in process memory, so the guard compensated by rejecting every failed session and a replacement process lost the in-flight step. Record it on the session failure, guard the recordSessionComplete fallback, and reap the owned create process group on host SIGINT and SIGTERM. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/sandbox/create-stream-process-group.test.ts (1)
142-155: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert listener identity instead of listener count.
Equal counts can pass if cleanup retains a new signal listener and removes a pre-existing listener. Snapshot
process.listeners()before the call and compare the listener arrays after settlement.Proposed test change
- const sigintBefore = process.listenerCount("SIGINT"); - const sigtermBefore = process.listenerCount("SIGTERM"); + const sigintBefore = process.listeners("SIGINT"); + const sigtermBefore = process.listeners("SIGTERM"); ... - expect(process.listenerCount("SIGINT")).toBe(sigintBefore); - expect(process.listenerCount("SIGTERM")).toBe(sigtermBefore); + expect(process.listeners("SIGINT")).toEqual(sigintBefore); + expect(process.listeners("SIGTERM")).toEqual(sigtermBefore);As per path instructions, review tests for behavioral confidence rather than implementation lock-in.
🤖 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/sandbox/create-stream-process-group.test.ts` around lines 142 - 155, Update the test around streamSandboxCreate to snapshot process.listeners("SIGINT") and process.listeners("SIGTERM") before the call, then assert the post-settlement listener arrays match those snapshots by identity. Replace the listener-count assertions while preserving the existing setup and create-stream invocation.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.
Nitpick comments:
In `@src/lib/sandbox/create-stream-process-group.test.ts`:
- Around line 142-155: Update the test around streamSandboxCreate to snapshot
process.listeners("SIGINT") and process.listeners("SIGTERM") before the call,
then assert the post-settlement listener arrays match those snapshots by
identity. Replace the listener-count assertions while preserving the existing
setup and create-stream invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8d4c86d7-60fc-478b-87d6-f3c6418fa14b
📒 Files selected for processing (11)
src/lib/onboard/exit-step-failure.test.tssrc/lib/onboard/exit-step-failure.tssrc/lib/onboard/machine/runtime.tssrc/lib/onboard/machine/transitions.tssrc/lib/onboard/runtime-boundary.test.tssrc/lib/onboard/runtime-boundary.tssrc/lib/sandbox/create-stream-process-group.test.tssrc/lib/sandbox/create-stream.tssrc/lib/state/onboard-session.tstest/helpers/rebuild-flow-target-image-cases.tstest/onboard-sandbox-recreation.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/onboard-sandbox-recreation.test.ts
- src/lib/onboard/runtime-boundary.ts
- src/lib/onboard/exit-step-failure.test.ts
- src/lib/onboard/machine/runtime.ts
- src/lib/sandbox/create-stream.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Collaborator
|
Automated review disposition for head
An independent nine-category security review passed for this exact head/base pair with no findings. The documentation writer review also passed with |
senthilr-nv
approved these changes
Jul 31, 2026
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Collaborator
|
Automated review disposition for head
An independent nine-category security review passed for this exact head/base pair with no findings. The documentation writer review also passed with |
senthilr-nv
approved these changes
Jul 31, 2026
Collaborator
|
Automated review disposition for head
|
Collaborator
|
Automated review disposition for head
An independent nine-category security review passed for this exact head/base pair with no findings. The documentation writer review also passed with |
23 tasks
senthilr-nv
added a commit
that referenced
this pull request
Aug 1, 2026
<!-- markdownlint-disable MD041 --> ## Summary Adds the canonical dated changelog entry for `v0.0.100` so the maintainer release plan can verify the pre-tag documentation prerequisite. The entry summarizes the user-facing changes merged since `v0.0.99` and links to the relevant guides. ## Changes - Add `docs/changelog/2026-07-31.mdx` with the exact `## v0.0.100` heading. - Cover restored OpenClaw pairing, transactional replacement, Deep Agents Code, onboarding recovery, lifecycle cleanup, Hermes builds, host provenance, documentation, and trusted E2E evidence. - Distinguish active Docker and Kubernetes runtime-bundle enforcement from the still-inactive managed shared-state transaction foundation. ## Source Coverage The release entry maps the doc-impacting merged PRs in the `v0.0.99..main` release range to `docs/changelog/2026-07-31.mdx`: #8021, #8024, #7973, #8028, #7947, #7788, #7884, #8023, #7969, #8020, #7989, #8000, #7907, #7942, #7567, #8013, #7955, #8017, #8014, #8015, #7629, #7644, #7821, #7971, and #7991. PR #7974 was reviewed after the final rebase and excluded because it changes internal maintainer-skill attribution policy and tests only; it does not change a user-facing product or documentation surface. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: the changelog contract test validates the dated entry, version heading, SPDX form, and route constraints. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/changelog/2026-07-31.mdx`; exact-head review passed for `6093f44f`; writing rules and documentation style reviewed; `npx vitest run test/changelog-docs.test.ts` passed 6/6; `npm run docs` passed with zero Fern errors and two generic Fern upgrade notices. - Agent: Codex Desktop <!-- docs-review-head-sha: 6093f44 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; no DGX Station host script changed. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run test/changelog-docs.test.ts` passed 6/6 at `6093f44f`. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to a dated prose-only release entry. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — validation passed with zero errors; Fern emitted two generic upgrade notices. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) — the changelog entry has the required parser-safe MDX SPDX header; dated changelog entries intentionally do not use page frontmatter. --- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.100. * Documented improvements to restore pairing, sandbox replacement, onboarding recovery, lifecycle cleanup, runtime handling, build support, host readiness, and end-to-end validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A sandbox that reaches
Readyno longer lets its create-stream termination signal reach the NemoClaw CLI. If a separate host interruption ends onboarding while a step is in flight, onboarding now reports the existing resumable recovery path instead of an invalid machine transition.Related Issue
Fixes #7982
Changes
failure.interruptedon the onboarding session when the incomplete-exit backstop records a failed step. The durable session is the only interruption authority, so a later process and an in-process transition use the same state.OnboardInterruptedError. Ordinary failed sessions keep the existing invalid-transition behavior.Type of Change
Quality Gates
onboard --resumerecovery workflow. It adds no command, flag, configuration, default, supported surface, or workflow.a695af6eaand base60b33abec; no actionable findings remain.Product scope: approved independently of GitHub merge state. This PR repairs existing onboarding and sandbox process cleanup behavior. It creates no integration, solution recipe, image, configuration surface, or new lifecycle commitment.
Exact reviewed pair: head
a695af6eafc2dc50ec8170ac068ef8773ffdac09; base60b33abeca789b35aa8970d169948a279147376e. The refreshed PR patch SHA-256 is46a967b1331ebf11ba29800b71f440b61b6e3b592892484a20c7bdaf35cc512d.Documentation Writer Review
no-docs-needed60b33abecthrough heada695af6ea. The refreshed patch matches the prior reviewed patch. It corrects onboarding interruption handling and process-group cleanup while preserving the documentedonboard --resumerecovery workflow. It adds no command, flag, configuration, default, supported surface, or workflow. Existing recovery guidance indocs/reference/commands.mdx,docs/reference/troubleshooting.mdx, and the quickstarts remains accurate.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project cli src/lib/onboard/exit-step-failure.test.ts src/lib/onboard/runtime-boundary.test.ts src/lib/onboard/runtime-boundary-record-only.test.ts src/lib/sandbox/create-stream-process-group.test.ts src/lib/sandbox/create-stream.test.ts src/lib/sandbox/create-stream-argv.test.ts src/lib/sandbox/create-stream-ready-gate.test.ts src/lib/state/onboard-session.test.tspassed 150/150.npx vitest run --project integration test/onboard-sandbox-recreation.test.tspassed 11/11.npm run build:cli,npm run typecheck:cli, andnpm run checks:repositorypassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Signed-off-by: Senthil Ravichandran senthilr@nvidia.com