fix: stop waiting on a stuck sandbox create stream

[kjw3]

Summary

• detach the non-root gateway path into its own log file
• stop waiting forever on an attached openshell sandbox create stream once the sandbox itself is already Ready
• add regression coverage for a create stream that never exits on its own

Root Cause

After #846, sandbox creation on a Brev CPU Linux box could appear to hang after image upload/import even though the sandbox had already reached Ready. NemoClaw was waiting for the attached openshell sandbox create stream to exit before it ever reached its own readiness polling.

Validation

• npx vitest run test/onboard.test.js test/onboard-readiness.test.js test/nemoclaw-start.test.js test/credential-exposure.test.js
• end-to-end validated on a Brev CPU Linux instance: nemoclaw onboard now continues past sandbox creation instead of hanging after the image import step

Summary by CodeRabbit

• New Features

  • Sandbox creation now completes when the sandbox is ready, rather than waiting for the full create command to exit, enabling faster startup.
  • Gateway output is now isolated to a dedicated log file in non-root mode for improved observability and debugging.

• Tests

  • Updated tests to validate sandbox readiness detection and log handling behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR enhances the sandbox creation flow to support early completion when a sandbox becomes ready, regardless of whether the underlying create process has exited. It adds log file isolation for gateway processes and updates test assertions accordingly.

Changes

Cohort / File(s)	Summary
Early-Exit Readiness Logic bin/lib/onboard.js	Extended streamSandboxCreate signature to accept options parameter (default {}). Added polling mechanism with configurable pollIntervalMs (default 2000ms) that invokes optional readyCheck callback. When readiness is detected, the function logs a forced-ready detail, sends SIGTERM to child process, cleans up resources via new detachChild() helper, and resolves early with {status: 0, sawProgress: true, forcedReady: true}. Refactored promise resolution via new finish() helper to centralize completion and prevent multiple resolutions. Updated createSandbox to pass readyCheck that runs openshell sandbox list and uses isSandboxReady() for detection.
Gateway Process Log Isolation scripts/nemoclaw-start.sh	In non-root mode, explicitly creates /tmp/gateway.log and /tmp/auto-pair.log with 600 permissions. Changed gateway launch from direct backgrounding to nohup "$OPENCLAW" gateway run >/tmp/gateway.log 2>&1 & to decouple gateway output from sandbox creation exit.
Test Assertion Updates test/credential-exposure.test.js	Updated regex pattern to accept optional third argument in streamSandboxCreate call assertion, permitting the new options parameter.
Nemoclaw Start Validation test/nemoclaw-start.test.js	New test file validating non-root behavior: verifies script contains non-root conditional, creates gateway log file, and launches gateway with output isolation via nohup.
Onboard Readiness Flow Tests test/onboard.test.js	Refactored JSON extraction to scan stdout lines in reverse for JSON payload (matching lines starting with { and ending with }). Added comprehensive test case for early sandbox-ready scenario: simulates long-lived child process, verifies SIGTERM delivery, confirms unref() and stdout/stderr destroy() called exactly once.

Sequence Diagram

sequenceDiagram
    participant Client
    participant streamSandboxCreate
    participant ChildProcess
    participant ReadyCheck
    
    Client->>streamSandboxCreate: start with options.readyCheck
    streamSandboxCreate->>ChildProcess: spawn create command
    ChildProcess-->>streamSandboxCreate: stdout/stderr data
    
    par Polling Loop
        streamSandboxCreate->>streamSandboxCreate: setInterval(pollIntervalMs)
        loop Every poll interval
            streamSandboxCreate->>ReadyCheck: invoke readyCheck()
            ReadyCheck-->>streamSandboxCreate: ready = true/false
            alt Sandbox ready
                streamSandboxCreate->>ChildProcess: send SIGTERM
                streamSandboxCreate->>streamSandboxCreate: detachChild()
                streamSandboxCreate->>Client: resolve early with forcedReady: true
            end
        end
    and Child Exit Handling
        ChildProcess-->>streamSandboxCreate: close event
        streamSandboxCreate->>streamSandboxCreate: finish() helper
    end

Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A sandbox springs to life with grace,
We check its readiness, set the pace,
When ready arrives, we kindly part—
No waiting games, just an early start! ✨
Logs float free, detached and sound,
Where cleanup and care are always found.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: stop waiting on a stuck sandbox create stream' directly and accurately describes the main change: detecting sandbox readiness and terminating the create stream early rather than waiting indefinitely for it to exit.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/846-sandbox-create-ready

---

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

[coderabbitai]

🧹 Nitpick comments (1)

bin/lib/onboard.js (1)

259-289: Potential issue: return inside finally block swallows early returns.

In the polling interval callback (lines 263-286), when options.readyCheck() throws or returns false, the code uses return to exit early. However, return statements within the try block are executed before finally, but the finally block runs unconditionally. This works correctly here since finally only sets polling = false, but the return on line 268 and 270 inside the try block could be confusing.

More importantly, if readyCheck() throws, line 268 catches it and returns, which is correct. But if other code in the try block after readyCheck() throws (e.g., child.kill throws something unexpected that isn't caught), the finally will still run and set polling = false, which is the desired behavior.

The logic is sound, but consider adding a brief comment explaining the early-return pattern for future maintainers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bin/lib/onboard.js` around lines 259 - 289, The polling callback around
readyTimer uses early returns inside the try blocks (e.g., after calling
options.readyCheck() and when ready is false) which can be confusing to future
maintainers; add a concise comment above the inner try/return pattern
(referencing the readyTimer callback, options.readyCheck(), polling flag, and
the finally that resets polling) explaining that early returns are intentional
and that the finally block must always clear polling, so subsequent iterations
can proceed; mention that child.kill(), detachChild(), and finish(...) are
executed only when ready is true to clarify control flow.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 259-289: The polling callback around readyTimer uses early returns
inside the try blocks (e.g., after calling options.readyCheck() and when ready
is false) which can be confusing to future maintainers; add a concise comment
above the inner try/return pattern (referencing the readyTimer callback,
options.readyCheck(), polling flag, and the finally that resets polling)
explaining that early returns are intentional and that the finally block must
always clear polling, so subsequent iterations can proceed; mention that
child.kill(), detachChild(), and finish(...) are executed only when ready is
true to clarify control flow.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b75e6d1e-b7bd-4732-bcce-e00ab50d3daa

📥 Commits

Reviewing files that changed from the base of the PR and between 36fa334 and 0029bb7.

📒 Files selected for processing (5)

• bin/lib/onboard.js
• scripts/nemoclaw-start.sh
• test/credential-exposure.test.js
• test/nemoclaw-start.test.js
• test/onboard.test.js

[ericksoa]

LGTM — nice fix for the stuck create stream. The readiness poll + SIGTERM + detach approach is clean, and the test coverage verifying kill/unref/destroy behavior is thorough.