perf(onboard): build sandbox image with BuildKit + no-silent-progress heartbeats (#6002)#6166
Merged
Merged
Conversation
…silent >60s On slow hosts (e.g. Brev GPU instances) onboarding could sit silently for minutes during gateway startup, the in-sandbox smoke test, and OpenClaw setup — `create-stream` only heartbeats *inside* the Docker build, leaving the surrounding sequence phases with no output. Users could not tell whether the install had stalled, and no timing was ever surfaced. Add a phase-progress reporter wired at the single onboarding sequence-runner seam (`buildOnboardSequenceHandlers`), so every onboarding phase: - emits a periodic "Still working on <phase>… (Ns elapsed)" heartbeat for wait-heavy non-interactive phases (default 30s, well under the 60s ceiling), guaranteeing the terminal never goes silent for longer than the interval; - records its wall-clock duration and emits an `onboard.phase.timing` trace event (collectable in E2E trace artifacts); - contributes to a "Phase timings" summary printed once before the ready banner, giving an actionable per-phase + total breakdown. Interactive phases (provider selection, policies) are timed but excluded from heartbeats so prompts aren't interrupted. The reporter is inert under the Vitest runner (so existing unit tests keep their exact output) and active in real CLI/E2E runs; `NEMOCLAW_ONBOARD_PROGRESS=1|0` forces either state and `NEMOCLAW_ONBOARD_HEARTBEAT_MS` tunes the cadence. All logic lives under `src/lib/onboard/`; `onboard.ts` is unchanged. Verified end-to-end through a real `nemoclaw onboard` run (docker driver): heartbeats fired every interval throughout the ~4m sandbox build, followed by the phase-timings summary (Total 4m 39s) and the ready dashboard. Fixes #6002 Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThis PR adds onboarding phase progress reporting, wires it into sequence execution and resume flows, introduces sandbox image prebuild before sandbox creation, refactors sandbox readiness polling, propagates ChangesOnboarding progress and sandbox flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant sequence_runner as sequence-runner
participant phase_progress as createPhaseProgressReporter
participant onboard_phase as OnboardSequencePhase
sequence_runner->>phase_progress: wrap(phase)
phase_progress->>onboard_phase: run()
onboard_phase-->>phase_progress: success or failure
phase_progress-->>sequence_runner: addTraceEvent(onboard.phase.timing)
sequenceDiagram
participant onboard as src/lib/onboard.ts
participant sandbox_prebuild as sandbox-prebuild
participant docker as Docker BuildKit
participant sandbox_create_launch as sandboxCreateLaunch.prepareSandboxCreateLaunch
onboard->>sandbox_prebuild: prebuildSandboxImageIfEligible()
sandbox_prebuild->>docker: stream build command
docker-->>sandbox_prebuild: build result
sandbox_prebuild-->>onboard: createArgs / imageRef
onboard->>sandbox_create_launch: prepareSandboxCreateLaunch(createArgs)
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 69%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
![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: 1
🧹 Nitpick comments (1)
src/lib/onboard/machine/phase-progress.ts (1)
35-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the label map exhaustively, and add a guard for heartbeat coverage
ONBOARD_PHASE_LABELSshould be keyed byOnboardNonTerminalMachineStateso new states can’t fall back to raw names.HEARTBEAT_PHASE_STATEScan stay a partial allowlist, but add a targeted test if the goal is to keep new wait-heavy phases on the heartbeat path.🤖 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/onboard/machine/phase-progress.ts` around lines 35 - 60, Type ONBOARD_PHASE_LABELS with OnboardNonTerminalMachineState instead of a generic string key map so new machine states must be added explicitly, and keep HEARTBEAT_PHASE_STATES as the selective allowlist. Update the phase-progress module around ONBOARD_PHASE_LABELS and HEARTBEAT_PHASE_STATES, and add a focused test that verifies known wait-heavy states remain covered by the heartbeat path when new phases are introduced.
🤖 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/machine/handlers/finalization.ts`:
- Around line 93-102: `printPhaseTimingsSummary` is resetting the shared timing
registry too early inside `handleFinalizationState`, so the finalizing phase’s
own timing is lost and an entry is left behind for the next onboarding run. Move
the summary print/reset out of the finalizing phase’s `run` path and into the
sequence runner after `createPhaseProgressReporter` has finished the phase
(after `finish()`/`phase.run` completes), so the summary includes finalizing and
the registry is cleared only once all phase timings have been recorded.
---
Nitpick comments:
In `@src/lib/onboard/machine/phase-progress.ts`:
- Around line 35-60: Type ONBOARD_PHASE_LABELS with
OnboardNonTerminalMachineState instead of a generic string key map so new
machine states must be added explicitly, and keep HEARTBEAT_PHASE_STATES as the
selective allowlist. Update the phase-progress module around
ONBOARD_PHASE_LABELS and HEARTBEAT_PHASE_STATES, and add a focused test that
verifies known wait-heavy states remain covered by the heartbeat path when new
phases are introduced.
🪄 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: 62191a0e-5819-413f-8c28-1ccc39075178
📒 Files selected for processing (6)
src/lib/onboard/machine/handlers/finalization.tssrc/lib/onboard/machine/phase-progress.test.tssrc/lib/onboard/machine/phase-progress.tssrc/lib/onboard/machine/sequence-runner.tssrc/lib/onboard/phase-timings.test.tssrc/lib/onboard/phase-timings.ts
Comment on lines
+93
to
+102
| // Emit the accumulated per-phase timing summary once, just before the "ready" | ||
| // block, so the user sees where onboarding wall-clock went (#6002). No-op when | ||
| // no phases were recorded (e.g. the progress reporter was disabled). Resets the | ||
| // registry so a subsequent onboard in the same process starts clean. | ||
| function printPhaseTimingsSummary(log: (message?: string) => void): void { | ||
| const summary = formatPhaseTimingsSummary(); | ||
| if (!summary) return; | ||
| log(summary); | ||
| resetPhaseTimings(); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
printPhaseTimingsSummary resets the registry before the finalizing phase's own duration is recorded, dropping it and leaking a stray entry into the next run.
printPhaseTimingsSummary is invoked from inside handleFinalizationState, which is the run function for the "finalizing" phase. Per createPhaseProgressReporter's wrap() (phase-progress.ts), the phase's own timing is only pushed to the registry via record(...) in finish(), which runs after await phase.run(context) resolves:
const result = await phase.run(context);
finish("completed");
Since printPhaseTimingsSummary calls resetPhaseTimings() while still inside phase.run (i.e., before finish() executes), two things happen:
- The "finalizing" phase's own duration never appears in the printed summary.
- After
handleFinalizationStatereturns,finish()pushes the finalizing-phase entry into the now-empty registry, where it sits unprinted until the nextresetPhaseTimings()call — polluting a later onboarding run's summary in the same process (per the module's own docstring, the three sequence runs share this registry).
Consider printing/resetting the summary from the sequence runner after the finalizing phase completes (post-finish()), rather than from within the finalizing phase's own handler, so its own duration is captured and no orphaned entry survives into the next run.
Also applies to: 199-203
🤖 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/onboard/machine/handlers/finalization.ts` around lines 93 - 102,
`printPhaseTimingsSummary` is resetting the shared timing registry too early
inside `handleFinalizationState`, so the finalizing phase’s own timing is lost
and an entry is left behind for the next onboarding run. Move the summary
print/reset out of the finalizing phase’s `run` path and into the sequence
runner after `createPhaseProgressReporter` has finished the phase (after
`finish()`/`phase.run` completes), so the summary includes finalizing and the
registry is cleared only once all phase timings have been recorded.
…review Rework the phase-timings summary emission in response to the PR review advisors and CodeRabbit (all flagged the same registry-reset ordering): - Emit the summary from the phase-progress reporter itself, right after the terminal (finalizing) phase's own duration is recorded, instead of from inside the finalization handler. The summary now includes the finalization phase and the registry is reset only once every phase timing has been written, so no stray entry leaks into a later onboard in the same process. - Reset the timing registry at the start of each onboard run (runInitialOnboardFlowSequence) so a run that failed before finalization can't leak stale timings into the next run. - Thread an injectable `env` through createPhaseProgressReporter so the heartbeat-interval override reads the same env as resolvePhaseProgressEnabled rather than process.env directly. - Type ONBOARD_PHASE_LABELS by OnboardNonTerminalMachineState so a new FSM state must be given a label rather than silently falling back to its raw id. - Tests: summary emitted once after finalizing (incl. finalization) then reset; no summary on finalizing failure; heartbeat-interval env resolution; heartbeat-coverage guard over all wait-heavy phases; interactive exclusion for policies as well as provider_selection; all-labels non-empty; seam wiring via buildOnboardSequenceHandlers. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
…nal seam Second review round (PR review advisors + CodeRabbit): - Make phase-progress telemetry best-effort and finish() run in a `finally` block: a throwing recorder/tracer/logger can no longer reclassify a successful phase as failed, double-record, or mask the phase's real result/error (advisor PRA-8 / security). - Move the "Phase timings" summary emission out of the reporter and into the final flow slice's terminal seam (runFinalOnboardFlowSequence), emitted after every wrapped phase (finalization AND any post_verify) has recorded, then reset. This is the true "all phases complete" boundary, so the summary can't miss the last phase or leave a stray entry regardless of which phase runs last (advisor PRA-5). The reporter now only records timings. - resolveHeartbeatIntervalMs takes a default env parameter (process.env) for parity with resolvePhaseProgressEnabled. - Tests: telemetry-throws does not flip success→failure and preserves a real phase failure; parametrized invalid/too-low heartbeat env → default 30s; terminal-seam summary emits once then resets; start-of-run reset clears stale timings from an earlier failed run. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Contributor
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 (2)
src/lib/onboard/machine/flow-slices.ts (1)
68-90: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSummary emission/reset is skipped on the failure path and isn't best-effort.
If
runOnboardSequenceWithRunnerthrows (the final phase fails), the function returns before reaching the summary block — the accumulated timing summary is silently dropped for that run, which is exactly the kind of diagnostic info a user would want after a long, previously-silent phase eventually fails. Separately,emitSummary(defaultconsole.log) is invoked without a guard; if it throws (e.g. EPIPE on a closed stdout), it would mask the already-successfulresult, contradicting the best-effort telemetry design this same PR establishes inphase-progress.ts'sfinish().🛡️ Proposed fix
const { emitSummary = (line: string) => console.log(line), ...runnerOptions } = options; - const result = await runOnboardSequenceWithRunner({ - ...runnerOptions, - phases: finalOnboardFlowPhases(options.phases), - }); - // Emit the accumulated per-phase timing summary from the terminal seam — - // after every wrapped phase in the final slice (finalization, and any - // post_verify) has recorded — then clear the registry. This is the true - // "all phases complete" boundary, so the summary can't miss the last phase - // or leave a stray entry behind, regardless of which phase runs last (`#6002`). - const summary = formatPhaseTimingsSummary(); - if (summary) { - emitSummary(summary); - resetPhaseTimings(); - } - return result; + try { + return await runOnboardSequenceWithRunner({ + ...runnerOptions, + phases: finalOnboardFlowPhases(options.phases), + }); + } finally { + // Best-effort: emit + reset regardless of success/failure, and never let + // a throwing emitSummary mask the real phase outcome (`#6002`). + try { + const summary = formatPhaseTimingsSummary(); + if (summary) { + emitSummary(summary); + resetPhaseTimings(); + } + } catch { + // ignore telemetry errors + } + } }Also worth adding a failure-path test (final phase throws) mirroring the existing coverage in
flow-slices.test.tsLines 279-317.🤖 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/onboard/machine/flow-slices.ts` around lines 68 - 90, runFinalOnboardFlowSequence currently only emits and clears the phase timing summary on the success path, so the final-run diagnostics are lost if runOnboardSequenceWithRunner throws. Update runFinalOnboardFlowSequence to make summary handling best-effort in a finally-style path: always call formatPhaseTimingsSummary() after the runner settles, emit it via emitSummary, and resetPhaseTimings() even when the final phase fails; also guard emitSummary so logging failures do not mask the main result or error. Add a failure-path test around runFinalOnboardFlowSequence in flow-slices.test.ts to cover a thrown final phase.src/lib/onboard/machine/phase-progress.ts (1)
154-211: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHeartbeat
logLinecall isn't covered by the new best-effort guarantee.
finish()now wrapsclearTimer,record,traceEvent, and the completionlogLinein try/catch so telemetry errors never affect the phase outcome (per the comment at Lines 171-174). But the heartbeat callback'slogLine(...)call (Lines 166-168) is invoked directly inside the rawsetIntervalcallback with no guard. If the injected/defaultlogLinethrows — e.g. the defaultconsole.loghittingEPIPEwhen stdout is piped to something that closes early (a well-documented Node.js crash scenario) — it becomes an uncaught exception outside thewrap()'s try/catch scope and can crash the whole onboarding process. Heartbeats fire repeatedly during exactly the long, silent phases this PR targets, so this is a real, easily-triggered code path, not a hypothetical one.🛡️ Proposed fix
const timer = heartbeatPhaseStates.has(phase.state) ? setTimer(() => { heartbeats += 1; const elapsedSeconds = Math.max(0, Math.round((now() - startedAt) / 1000)); - logLine(` ⏳ Still working on ${label}… (${elapsedSeconds}s elapsed)`); + try { + logLine(` ⏳ Still working on ${label}… (${elapsedSeconds}s elapsed)`); + } catch { + // Best-effort: heartbeat logging must never crash onboarding. + } }, heartbeatIntervalMs) : null;Consider also adding a test mirroring
phase-progress.test.tsLines 258-272 ("does not reclassify a successful phase when telemetry throws") for a throwing heartbeatlogLine.🤖 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/onboard/machine/phase-progress.ts` around lines 154 - 211, The heartbeat logging in wrap() is not protected by the new best-effort error handling, so a throwing logLine inside the setTimer callback can still crash onboarding. Update the heartbeat callback in phase-progress.ts so the logLine call is wrapped in its own try/catch (or otherwise made best-effort like finish()), while keeping the existing behavior in wrap(), finish(), and phase.run unchanged. Consider adding a test alongside the existing phase-progress best-effort coverage to verify a throwing heartbeat logger does not fail the phase.
🤖 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/onboard/machine/flow-slices.ts`:
- Around line 68-90: runFinalOnboardFlowSequence currently only emits and clears
the phase timing summary on the success path, so the final-run diagnostics are
lost if runOnboardSequenceWithRunner throws. Update runFinalOnboardFlowSequence
to make summary handling best-effort in a finally-style path: always call
formatPhaseTimingsSummary() after the runner settles, emit it via emitSummary,
and resetPhaseTimings() even when the final phase fails; also guard emitSummary
so logging failures do not mask the main result or error. Add a failure-path
test around runFinalOnboardFlowSequence in flow-slices.test.ts to cover a thrown
final phase.
In `@src/lib/onboard/machine/phase-progress.ts`:
- Around line 154-211: The heartbeat logging in wrap() is not protected by the
new best-effort error handling, so a throwing logLine inside the setTimer
callback can still crash onboarding. Update the heartbeat callback in
phase-progress.ts so the logLine call is wrapped in its own try/catch (or
otherwise made best-effort like finish()), while keeping the existing behavior
in wrap(), finish(), and phase.run unchanged. Consider adding a test alongside
the existing phase-progress best-effort coverage to verify a throwing heartbeat
logger does not fail the phase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f1a02404-b54b-4503-846a-2a78b1774140
📒 Files selected for processing (4)
src/lib/onboard/machine/flow-slices.test.tssrc/lib/onboard/machine/flow-slices.tssrc/lib/onboard/machine/phase-progress.test.tssrc/lib/onboard/machine/phase-progress.ts
…PRA-7) The resume-repair compatibility path in runLiveOnboardFlowSlice runs raw phases without the phase-progress reporter by design; heartbeats/timing are scoped to fresh onboarding. Document why this cannot leak stale timing state (per-process registry + start-of-run reset), addressing the review's resolve-or-justify item. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
…2.8x (#6002) The 8–10 min first-run onboarding on Brev is dominated by the sandbox image build. openshell builds it with the **classic** Docker builder, which commits one image layer per instruction; the sandbox Dockerfile has ~100 instructions, so the build is dominated by per-layer commit overhead, not real work. openshell exposes no way to enable BuildKit. Fix: on the Docker-driver path (where the gateway shares the local Docker daemon), NemoClaw now builds the staged image locally with BuildKit and hands openshell the resulting image ref via `--from <ref>`, so openshell skips its slow build. Measured on a 4-vCPU host building the identical image: - classic (openshell today): 6m 31s - BuildKit (this change): 2m 18s cold / ~20s warm-cache → build under 3 min Real end-to-end `nemoclaw onboard` (docker driver): full onboard dropped from ~7m 26s to 1m 10s–1m 44s, "Deployment verified — gateway and dashboard are healthy", ONBOARD_EXIT=0. BuildKit build output replaces the classic one; the create-stream progress parser already understands BuildKit output. Safety: - Docker-driver only (a local image is not visible to a k3s/remote gateway). - Any ineligibility or build failure falls back to the existing openshell build — a slow onboard, never a broken one. Opt out with NEMOCLAW_SANDBOX_PREBUILD=0. - New module src/lib/onboard/sandbox-prebuild.ts holds the logic; the fatal create-failure reporting was extracted to sandbox-create-failure.ts so onboard.ts stays net-negative (growth guardrail). Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
yimoj
changed the title
yimoj
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
2907-2915: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrebuilt local image is never cleaned up on recreate.
dockerRmi(previousEntry.imageTag, ...)removes the sandbox's build image, but the separatenemoclaw-sandbox-local:<sandboxName>tag produced bysandboxPrebuild.prebuildSandboxImageIfEligible(Line 3026) has no analogous cleanup path — itsimageRefis discarded at the call site. Each recreate/re-onboard on the docker-driver path leaves the previous build's layers dangling under that reused tag, accumulating disk usage over time.♻️ Suggested addition (approximate location)
if (previousEntry?.imageTag) { const rmiResult = dockerRmi(previousEntry.imageTag, { ignoreError: true, suppressOutput: true, }); if (rmiResult.status !== 0) { console.warn(` Warning: failed to remove old sandbox image '${previousEntry.imageTag}'.`); } } + dockerRmi(sandboxPrebuild.sandboxLocalImageRef(sandboxName), { + ignoreError: true, + suppressOutput: true, + });🤖 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/onboard.ts` around lines 2907 - 2915, The recreate cleanup in onboard.ts only removes previousEntry.imageTag, so the prebuilt local tag from sandboxPrebuild.prebuildSandboxImageIfEligible is left behind and accumulates disk usage. Update the recreate/re-onboard cleanup path near the existing dockerRmi call to also track and remove the discarded sandbox local imageRef (the nemoclaw-sandbox-local:<sandboxName> tag) when it exists, and keep the warning handling consistent with the current image cleanup in onboard.ts.
🤖 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.ts`:
- Around line 3079-3086: The failure diagnostics are using stale sandbox create
arguments after the prebuild rewrite. Update the `createResult` failure path in
`onboard.ts` so
`sandboxCreateFailureDiagnostics.handleSandboxCreateResultFailure` receives the
same args actually used to run `createCommand`/produce `createResult` (the
`launchCreateArgs` value), not the original `createArgs`, so
`printSandboxCreateRecoveryHints` shows the correct local image ref instead of
the rewritten staged-Dockerfile reference.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 2907-2915: The recreate cleanup in onboard.ts only removes
previousEntry.imageTag, so the prebuilt local tag from
sandboxPrebuild.prebuildSandboxImageIfEligible is left behind and accumulates
disk usage. Update the recreate/re-onboard cleanup path near the existing
dockerRmi call to also track and remove the discarded sandbox local imageRef
(the nemoclaw-sandbox-local:<sandboxName> tag) when it exists, and keep the
warning handling consistent with the current image cleanup in onboard.ts.
🪄 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: 250c4d28-9a7d-420f-9bea-d5b47968c9ff
📒 Files selected for processing (5)
src/lib/onboard.tssrc/lib/onboard/sandbox-create-failure-result.test.tssrc/lib/onboard/sandbox-create-failure.tssrc/lib/onboard/sandbox-prebuild.test.tssrc/lib/onboard/sandbox-prebuild.ts
The BuildKit prebuild rewrites the create `--from` from the staged Dockerfile path to a local image ref. Onboard integration tests drive the real create flow and inspect the Dockerfile/build-context *through* that `--from` arg, so on CI (Linux + docker) the prebuild ran and broke onboard-messaging / onboard-custom-dockerfile / onboard-terminal-dashboard assertions. Gate `resolveSandboxPrebuildEnabled` off under `VITEST` / `NODE_ENV=test` (mirrors the phase-progress reporter) unless `NEMOCLAW_SANDBOX_PREBUILD=1` forces it. Real CLI/E2E runs have no VITEST and still get the speedup. Verified the full onboard test surface (1993 tests) is green. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
…ure hints Address PR review findings on the BuildKit prebuild: - Security (advisor): the default prebuild subprocess ran with raw process.env. Run it under buildSubprocessEnv() (the same allowlist the openshell create uses: PATH/HOME/DOCKER_HOST/...), so host secrets (e.g. NVIDIA_API_KEY) never enter the build subprocess. DOCKER_BUILDKIT is set inline, so BuildKit is still used. Verified end-to-end (real onboard, ONBOARD_EXIT=0, ~1m07s). - Correctness (CodeRabbit): handleSandboxCreateResultFailure now receives the create args actually used (launchCreateArgs, which the prebuild may have rewritten to `--from <image-ref>`), so recovery hints don't reference a stale staged-Dockerfile path. - Add a trust-boundary comment on shellSingleQuote noting the interpolated values are internal (mkdtemp dir + name-derived tag) and quoted defensively. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
…; doc + tests Address blocking PR-review-advisor items: - Inference/policy silence (advisor: "provider/inference setup can still be silent in the real core flow"): the provider_selection and policies phases own wait-heavy non-interactive work (provider validation, first-inference setup, policy application) but were excluded from heartbeats to protect interactive prompts. Now they are excluded only in interactive mode; in non-interactive runs (installer / Brev / --yes) — where there is no prompt — they get heartbeats too, so that window is no longer silent. Derived from NEMOCLAW_NON_INTERACTIVE; overridable via the reporter's `interactive` option. - Document the phase-timings module-level registry's removal condition (advisor: "lacks documented removal condition"): remove once onboarding timings are first-class FSM state with a per-run collector threaded through the runtime. - Tests: non-interactive heartbeat coverage for provider_selection/policies, NEMOCLAW_NON_INTERACTIVE derivation, and a real-flow heartbeat test that drives runOnboardSequenceWithRunner end-to-end (advisor: "no E2E test exercises heartbeats in the real onboarding flow"). Verified: full onboard test surface (1927 tests) green. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
…E env Advisor found that `--non-interactive` (the flag) set the module-level NON_INTERACTIVE but not the env var, so env-based consumers — including the new phase-progress heartbeat gating (which heartbeats provider/policy phases only in non-interactive mode) as well as sandbox-create-plan and policy presets — missed the flag and still treated the run as interactive. Propagate the flag to process.env.NEMOCLAW_NON_INTERACTIVE so the flag and the env var are equivalent for all downstream checks. Verified full onboard surface (1997 tests) green. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Advisor: the prebuild used a generic host subprocess env. Match the openshell create path and strip the host-infrastructure credentials a docker build never needs (KUBECONFIG, SSH_AUTH_SOCK) from the sanitized env. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
…get (#6002) Adds a maintained e2e-live acceptance artifact (the last blocking advisor item: "no E2E test validates heartbeats in the real onboarding flow" / "missing maintained runtime validation for the [1/8]-to-first-inference and no-silent-gap acceptance"). It drives a real ./bin/nemoclaw.js onboard against a fake OpenAI-compatible endpoint (no hosted key needed), on the Docker driver, and asserts: - a progress heartbeat is emitted during the long build (no silent phase); - the BuildKit prebuild ran and no classic per-instruction build steps appeared (the speed fix is exercised); - the end-of-onboard "Phase timings" summary is printed; - the onboard completes within a wall-clock budget — records the elapsed time and fails over NEMOCLAW_E2E_ONBOARD_BUDGET_SECS (generous default so it doesn't flake on arbitrary hardware; the canonical Docker-driver/Brev runner sets 180 to enforce the ≤3-minute goal). Opt-in via NEMOCLAW_RUN_LIVE_E2E=1. Validated locally: passes in ~85s. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
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 `@test/e2e/live/onboard-progress-budget.test.ts`:
- Around line 34-35: The ONBOARD_TIMEOUT_MS value is being treated as seconds
even though the env var name uses an _MS suffix. Update the timeout parsing in
onboard-progress-budget.test.ts so NEMOCLAW_E2E_PHASE_TIMEOUT_MS is consumed as
milliseconds directly, matching HEARTBEAT_MS and the naming convention, and only
apply unit conversion if the variable name or default is changed accordingly.
- Around line 108-125: The new inline precondition checks inside the liveTest
callback trigger the test-conditionals guardrail, so move the CLI build and
docker availability gating out of the test body in
onboard-progress-budget.test.ts. Use the test harness’s precondition helpers
such as test.skipIf/test.runIf (or split into separate test cases) before the
callback that contains the heartbeat and phase-timings assertions, and keep the
liveTest body free of conditional skip branches.
🪄 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: e8bb7499-3a43-4378-ab94-173bd8605750
📒 Files selected for processing (1)
test/e2e/live/onboard-progress-budget.test.ts
Comment on lines
+108
to
+125
| liveTest( | ||
| "onboard emits heartbeats + a phase-timings summary and completes within budget (#6002)", | ||
| { timeout: TEST_TIMEOUT_MS }, | ||
| async ({ artifacts, cleanup, host, sandbox, skip }) => { | ||
| if (!fs.existsSync(CLI_DIST_ENTRYPOINT)) { | ||
| skip("run `npm run build:cli` before live repo CLI targets"); | ||
| return; | ||
| } | ||
|
|
||
| const docker = await host.command("docker", ["info"], { | ||
| artifactName: "prereq-docker-info", | ||
| env: buildAvailabilityProbeEnv(), | ||
| timeoutMs: 30_000, | ||
| }); | ||
| if (docker.exitCode !== 0) { | ||
| skip("docker is required for the onboard progress/budget acceptance test"); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move these skip checks out of the test body. The new inline if branches trip the test-conditionals guardrail and block CI; decide the build/docker preconditions before entering the test callback, using test.skipIf/test.runIf or separate test cases.
🤖 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 `@test/e2e/live/onboard-progress-budget.test.ts` around lines 108 - 125, The
new inline precondition checks inside the liveTest callback trigger the
test-conditionals guardrail, so move the CLI build and docker availability
gating out of the test body in onboard-progress-budget.test.ts. Use the test
harness’s precondition helpers such as test.skipIf/test.runIf (or split into
separate test cases) before the callback that contains the heartbeat and
phase-timings assertions, and keep the liveTest body free of conditional skip
branches.
…6002) - codebase-growth-guardrails: changed test files may not add `if` statements. Move the built-CLI gate into the test declaration (skipIf-style) and assert docker availability with expect() instead of an in-body if/skip. 0 added ifs. - advisor: enforce the acceptance budget by default. NEMOCLAW_E2E_ONBOARD_BUDGET_SECS now defaults to 180 (the issue's ≤3-minute goal) so the test fails over budget by default; constrained/cold-cache runners can raise it. Re-validated locally: passes in ~85s (< 180s budget). Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
…eams Address advisor findings: - One reporter per process (getDefaultPhaseProgressReporter, memoized) used as the buildOnboardSequenceHandlers default, instead of a fresh reporter per sequence run (PRA: "three independent reporter instances created per run"). - The resume-repair compatibility branch (live-flow-slice) now wraps its phases with the same shared reporter, so heartbeats + per-phase timing cover resume too, not only fresh onboarding (PRA: "compatibility branch runs raw phases without phase-progress reporter"). Inert under Vitest; shares the per-process timing registry. Verified: onboard test surface (1916 tests) green. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
) Advisor PRA-2: the acceptance test measured only onboard-completion, not the issue's actual "[1/8] to first response" path. A stub endpoint can't drive a full agent turn (LLM request times out), so the test now uses real hosted inference (NVIDIA_INFERENCE_API_KEY) and, after onboard, drives one headless `openclaw agent` turn and asserts a real non-empty first response. The ≤3-minute budget (default 180s) now covers the whole [1/8]-to-first-response path. Validated locally end-to-end: onboard + first response in ~76s (< 180s). Kept the body linear (no `if`) per the changed-test-file guardrail. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
wscurran
added
area: onboarding
The BuildKit prebuild is a workaround for openshell building the sandbox image with the classic Docker builder and exposing no BuildKit toggle. Per the repo convention for documented workarounds (see phase-timings.ts, withLocalNoProxy), state its removal condition: when openshell builds with BuildKit by default or accepts a builder flag on `sandbox create`, delete the module and pass the Dockerfile straight to `--from` again. Also clarify that the `NEMOCLAW_SANDBOX_PREBUILD=1` force-on override only bypasses the Vitest-inert gate — a locally-built image is still invisible to a remote gateway, so `=1` is meaningful only on a Docker-driver host (resolves PR Review Advisor removal-condition + force-on visibility findings). Doc-only. Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/sandbox-prebuild.ts (1)
31-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink a tracking issue for the documented removal condition.
The comment clearly documents why this module is transitional and when it should be deleted, which is good, but path instructions for
src/lib/**require that intentional partial migrations "link the remaining work in GitHub" and that retained old paths "link the retirement issue or PR in GitHub" with observable exit criteria. Consider adding an issue/PR reference here so the removal condition is tracked rather than only documented in a comment.As per path instructions,
src/lib/**: "Retain an old path only for a demonstrated external/persisted-data contract or a bounded confidence/rollback window... link the retirement issue or PR in GitHub, and state observable exit criteria."🤖 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/onboard/sandbox-prebuild.ts` around lines 31 - 35, The sandbox prebuild module is intentionally transitional, but the removal condition is only documented in a comment and not linked to any tracked work. Update the comment near the sandbox prebuild logic in sandbox-prebuild.ts to reference the retirement tracking issue or PR, and make sure the related call site in onboard.ts is covered too so the remaining work has an explicit GitHub link and observable exit criteria.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/onboard/sandbox-prebuild.ts`:
- Around line 31-35: The sandbox prebuild module is intentionally transitional,
but the removal condition is only documented in a comment and not linked to any
tracked work. Update the comment near the sandbox prebuild logic in
sandbox-prebuild.ts to reference the retirement tracking issue or PR, and make
sure the related call site in onboard.ts is covered too so the remaining work
has an explicit GitHub link and observable exit criteria.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f65aa709-a6b5-4b85-bccc-f2a0dfe3e49a
📒 Files selected for processing (2)
src/lib/onboard/machine/phase-progress-seam.test.tssrc/lib/onboard/sandbox-prebuild.ts
The BuildKit prebuild rewrites the sandbox create `--from <Dockerfile>` to a local image ref, but every recovery-hint test fed a Dockerfile path — the post-prebuild case was untested. Add two cases: - build-context: printSandboxCreateRecoveryHints renders the pushed registry ref (never a Dockerfile path, never the stale local ref as --from) when the create args already carry a rewritten --from image ref. - sandbox-create-failure: handleSandboxCreateResultFailure forwards those post-rewrite image-ref args verbatim to the recovery hints. Resolves PR Review Advisor PRA-10 / PRA-T4 (recovery hints must reflect post-prebuild args). The related PRA-4 'complete the extraction' has no code change: classifySandboxCreateFailure lives in validation.ts and printSandboxCreateRecoveryHints in build-context.ts (not onboard.ts, which only imports and dependency-injects them via the delegation comments at the destructure sites) — the split is already complete. Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
21 tasks
Resolve PR conflicts while preserving current-main onboarding and prepared DCode behavior. Use stateless heartbeats, an async BuildKit prebuild, and build-unique image tags. Run the live onboarding budget assertion in the existing E2E job. Co-authored-by: Yimo Jiang <yimoj@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Preserve main's authoritative gateway and MCP onboarding changes. Retain the streamlined BuildKit prebuild and heartbeat path. Co-authored-by: Yimo Jiang <yimoj@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Merge the latest terminal-agent version enforcement from main. Keep the streamlined onboarding progress path unchanged. Co-authored-by: Yimo Jiang <yimoj@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Merge the latest tunnel and onboarding port-release changes from main. Keep the streamlined onboarding progress path unchanged. Co-authored-by: Yimo Jiang <yimoj@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/e2e.yaml (1)
2765-2786: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the second Vitest invocation into its own step.
Running
onboard-progress-budget.test.tsin the samerun:block asfull-e2e.test.tsmeans: (1) a failure in the first test short-circuits the second underset -euo pipefail, so the new budget test silently never executes on that run, and (2) the GitHub Actions UI can't distinguish which test failed since both share one step's pass/fail signal.♻️ Proposed fix: separate step
- name: Run full-e2e live Vitest test env: NVIDIA_INFERENCE_API_KEY: ${{ secrets.NVIDIA_INFERENCE_API_KEY }} run: | set -euo pipefail export PATH="$HOME/.local/bin:$HOME/.npm-global/bin:$PATH" if command -v openshell >/dev/null 2>&1; then OPENSHELL_BIN="$(command -v openshell)" elif [ -x "$HOME/.local/bin/openshell" ]; then OPENSHELL_BIN="$HOME/.local/bin/openshell" else echo "::error::OpenShell CLI not found after install" exit 1 fi export OPENSHELL_BIN "$OPENSHELL_BIN" --version npx vitest run --project e2e-live \ test/e2e/live/full-e2e.test.ts \ --silent=false --reporter=default - npx vitest run --project e2e-live \ - test/e2e/live/onboard-progress-budget.test.ts \ - --silent=false --reporter=default + + - name: Run onboard progress/budget live Vitest test + env: + NVIDIA_INFERENCE_API_KEY: ${{ secrets.NVIDIA_INFERENCE_API_KEY }} + run: | + set -euo pipefail + npx vitest run --project e2e-live \ + test/e2e/live/onboard-progress-budget.test.ts \ + --silent=false --reporter=default🤖 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 @.github/workflows/e2e.yaml around lines 2765 - 2786, Split the two Vitest runs in the e2e workflow into separate GitHub Actions steps so each test has its own pass/fail signal and the second test is not skipped by set -euo pipefail if the first fails. Keep the existing OpenShell setup logic in the step that runs full-e2e.test.ts, then add a new step for onboard-progress-budget.test.ts with its own run block and the same e2e-live project settings. Use the step names around Run full-e2e live Vitest test as the anchor when updating the workflow.
🤖 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 @.github/workflows/e2e.yaml:
- Around line 2765-2786: Split the two Vitest runs in the e2e workflow into
separate GitHub Actions steps so each test has its own pass/fail signal and the
second test is not skipped by set -euo pipefail if the first fails. Keep the
existing OpenShell setup logic in the step that runs full-e2e.test.ts, then add
a new step for onboard-progress-budget.test.ts with its own run block and the
same e2e-live project settings. Use the step names around Run full-e2e live
Vitest test as the anchor when updating the workflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9b5a846f-4b76-41d4-a5ce-fd63e8148d50
📒 Files selected for processing (2)
.github/workflows/e2e.yamlsrc/lib/build-context.test.ts
💤 Files with no reviewable changes (1)
- src/lib/build-context.test.ts
Move prebuild launch coordination, non-interactive environment scoping, and readiness polling behind focused onboarding module boundaries. Co-authored-by: Yimo Jiang <yimoj@nvidia.com> Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
approved these changes
Jul 3, 2026
21 tasks
cv
added a commit
that referenced
this pull request
Jul 4, 2026
<!-- markdownlint-disable MD041 --> ## Summary This follow-up to #6166 validates the staged context before invoking host BuildKit and moves the hard #6002 cold-path acceptance assertions into the existing `full-e2e` lifecycle. It removes the redundant second onboarding run while preserving a distinct merge-failing signal alongside the advisory warm-system scorecard budget. ## Related Issue Follow-up to #6166 and #6002. ## Changes - Resolve and validate BuildKit contexts as private direct `os.tmpdir()/nemoclaw-build-*` staging directories with a no-follow regular Dockerfile, falling back to the gateway builder when validation fails. - Carry generated/custom provenance into the handoff so user-supplied `--from` Dockerfiles remain on the OpenShell gateway-builder trust boundary. - Cover custom provenance, outside-temp, wrong-prefix, writable-directory, symlink, non-regular-file, path-escape, inspection-error logging, environment sanitization, and compatibility-heartbeat behavior. - Measure BuildKit success, fallback absence, output silence, and the first real agent response during the job's first `full-e2e` onboarding instead of running a second warm-cache onboarding test. - Remove the redundant Vitest invocation from `e2e.yaml` and document the hard 180-second cold-path contract separately from the advisory 390-second warm-system scorecard budget. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Reviewed every production context stager against the new contract; explicit provenance keeps custom Dockerfiles off host BuildKit, focused negative tests cover each trust-boundary case, and the unavoidable post-validation Docker pathname reopen is mitigated by private `mkdtemp` staging. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [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) - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added optional cold-onboarding performance measurement to the live end-to-end flow, including trace-based evidence output. * **Bug Fixes** * Hardened local prebuild handling for NemoClaw-generated staged build contexts and tightened eligibility/`--from` validation behavior. * Improved sandbox build-context metadata propagation and reused Docker-driver gateway detection. * **Tests** * Expanded/refocused live end-to-end coverage and performance assertions; added trace/probe parsing fixtures; improved test isolation and cleanup. * Updated e2e workflow and release-gate checks to target the correct live Vitest test. * **Documentation** * Clarified `--from` build-context routing vs local BuildKit behavior on local Docker-driver gateways. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
21 tasks
ericksoa
added a commit
that referenced
this pull request
Jul 4, 2026
<!-- markdownlint-disable MD041 --> ## Summary This PR prepares the user-facing documentation for v0.0.74 before the release plan is frozen. It expands the release notes across the 56-commit train and closes durable documentation gaps found during the pre-tag commit scan. ## Changes - Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed MCP, progressive tool disclosure, LangChain Deep Agents Code, onboarding, local inference, messaging, recovery, and contributor workflows. - Correct the `destroy` contract for retained per-name volumes, gateway-unreachable `--force` cleanup, managed MCP ownership, and same-name recovery. - Document separate remediation for an unreachable container DNS resolver versus one that answers with `NXDOMAIN` or `REFUSED`. - Document the Windows on Arm N1X automatic Ollama safeguard and its remaining large-model limitations. - State that messaging conflicts abort rebuild before backup or deletion, leaving the original sandbox intact. - Link the agent-runnable value benchmark from the contributor task index. - Synchronize generated agent command variants. - Validate with `npm run docs:sync-agent-variants` and `npm run docs`; Fern completed with 0 errors and 2 existing warnings. - Source summary: - [#6020](#6020) and [#5876](#5876) -> `docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy boundary and managed MCP lifecycle. - [#6251](#6251) and [#5989](#5989) -> `docs/about/release-notes.mdx`: Summarize progressive tool disclosure and sandbox-first inference controls. - [#6232](#6232), [#6082](#6082), [#6219](#6219), [#6214](#6214), [#6215](#6215), [#6230](#6230), and [#6260](#6260) -> `docs/about/release-notes.mdx`: Summarize the experimental LangChain Deep Agents Code status, secret, version, rebuild, snapshot, and MCP boundaries. - [#6166](#6166), [#6254](#6254), [#6265](#6265), [#6164](#6164), and [#6017](#6017) -> `docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated image reuse, bounded readiness, and preflight improvements. - [#6150](#6150) -> `docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`: Separate unreachable-resolver remediation from reachable-but-rejected DNS responses. - [#6234](#6234) -> `docs/about/release-notes.mdx`, `docs/inference/use-local-inference.mdx`, and `docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B selection and the remaining explicit-large-model boundary. - [#6129](#6129), [#5987](#5987), [#5955](#5955), and [#6220](#6220) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/commands.mdx`, and `docs/reference/commands-nemohermes.mdx`: Document messaging policy persistence, status, and the pre-destructive conflict check. - [#5963](#5963), [#6050](#6050), [#6094](#6094), [#6238](#6238), [#5988](#5988), [#6235](#6235), [#6181](#6181), and [#5986](#5986) -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and `docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and clarify retained-volume and local-only destroy semantics. - [#6200](#6200), [#6248](#6248), [#6168](#6168), [#6270](#6270), and [#5649](#5649) -> `docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize contributor setup and verification improvements and expose the advisory value benchmark. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: documentation-only release preparation; generated-variant synchronization and the Fern docs build validate the changed pages and routes. - [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: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed 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: tests are not applicable to this documentation-only change; `npm run docs` validates the source and generated routes. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded setup guidance for Windows on Arm devices with safer default local model selection. * Clarified local inference and sandbox messaging behavior, including conflict checks before rebuilds and safer recovery steps. * Updated destroy/rebuild/reference docs with more detailed warnings, failure handling, and volume-retention guidance. * Improved troubleshooting instructions for Docker DNS issues with clearer paths for unreachable vs. blocked resolvers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Merged
21 tasks
jyaunches
added a commit
that referenced
this pull request
Jul 9, 2026
…e step into modules (#6444) <!-- markdownlint-disable MD041 --> ## Summary Extracts cohesive units of the sandbox create/register orchestration out of the ~4,900-line `src/lib/onboard.ts` entrypoint into focused modules under `src/lib/onboard/`, following the injected-deps boundary style established by `created-sandbox-finalization.ts` (#6332). The primary goal is maintainability and independent unit-test coverage for the create path, with intentional safety/behavior refinements discovered during review: redacted create-output failure echoing, preservation of non-zero create-stream status when readiness fails, direct argv spawning for trusted create paths, fail-closed Docker-GPU create-poll side-effect handling, and redacted trace reporting for poll/readiness errors. Note on the issue's premise: #6258 cites a `+27 net lines` growth-guard violation from #6166. That premise is stale — the merged #6166 left `onboard.ts` net-smaller, and #6276/#6332 shrank it further, so the guard (`.github/workflows/codebase-growth-guardrails.yaml`, a per-PR net-neutral diff gate) is not currently red. This PR is therefore incremental maintainability work; it keeps `onboard.ts` net-neutral-or-smaller so the gate stays green. ## Related Issue Refs #6258 <!-- Refs (not Fixes): this PR lands two increments of a larger extraction; the remaining create/finalize wiring is left for follow-ups, so the issue should stay open. --> ## Changes - Add `src/lib/onboard/created-sandbox-failure.ts`: `reportSandboxCreateFailure` (warns-and-continues on an incomplete create; otherwise prints diagnostics + recovery hints and exits) and `reportSandboxReadinessFailure` (prints the readiness failure, defers cleanup to the Docker-GPU patch or deletes the failed sandbox, then exits). `onboard.ts` replaces the two inline blocks with module calls — net −2 lines. - Add `src/lib/onboard/sandbox-create-step.ts`: `runSandboxCreateStep` encapsulates the BuildKit prebuild handoff → Docker-GPU create-patch provisioning → create-stream behind a context + injected-deps boundary. This move is **line-neutral** on `onboard.ts`; its value is a named, unit-testable boundary (the `prepare → patch → stream` sequence is now testable without standing up the entrypoint), not a size reduction. Build-context and exit-listener cleanup stay with the caller that armed them. - Add focused unit tests across `created-sandbox-failure.test.ts`, `sandbox-create-step.test.ts`, `sandbox-create-launch.test.ts`, `create-stream.test.ts`, `create-stream-argv.test.ts`, and `create-stream-ready-gate.test.ts` covering failure branches, redaction, exit-code preservation, GPU vs non-GPU readiness cleanup, prebuild/patch/stream wiring, direct argv spawn boundaries, terminal-agent/default-driver ready-check gating, fail-closed `onPoll` error handling, and redacted poll/readiness trace behavior. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: no CLI flags, commands, configuration, or documented user workflow changed. The user-visible differences are limited to safer failure-path diagnostics (credential redaction), more accurate readiness-failure exit status, and fail-closed create-poll error handling. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: onboarding/sandbox path. Extraction boundaries were verified against the pre-extraction source; intentional safety refinements are explicitly covered by tests: create output is redacted before failure logging, readiness failure preserves a non-zero create-stream status instead of flattening to `1`, Docker-GPU during-create polling is isolated from readiness detection via `onPoll`, escaping poll errors abort create with classified/generic failure text plus redacted trace emission, and ready-check exceptions emit redacted trace evidence without falsely forcing Ready. Requesting maintainer sensitive-path review. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed 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: `npx vitest run --project cli src/lib/actions/sandbox/snapshot.test.ts src/lib/onboard/created-sandbox-failure.test.ts src/lib/onboard/sandbox-create-launch.test.ts src/lib/onboard/sandbox-create-step.test.ts src/lib/sandbox/create-stream.test.ts src/lib/sandbox/create-stream-ready-gate.test.ts src/lib/sandbox/create-stream-argv.test.ts` → 107 passed; `npx tsc --noEmit --pretty false --project tsconfig.cli.json` → clean; `npm run test-size:check` → passed; `npx prek run --all-files --stage pre-commit --skip source-shape-test-budget --skip test-skills-yaml` → passed; `npm run test-conditionals:scan -- --top 25` → no new changed-file conditional failures; `git diff --check` → clean. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Required live E2E run [29021019725](https://github.com/NVIDIA/NemoClaw/actions/runs/29021019725) passed on exact head `82518f7e6f69c39d9bc04f63a365753aac7b1b6d`: `cloud-onboard`, `onboard-repair`, `onboard-resume`, `state-backup-restore`, `upgrade-stale-sandbox`, and `snapshot-commands` all succeeded. PR CI checks also passed on the same head after rerunning flaky `policy-channel-list.test.ts` shard timeout (`cli-test-shards (3)`). - [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) - [ ] 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) --- Signed-off-by: Dongni Yang <dongniy@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved sandbox onboarding using a dedicated creation step that centralizes prebuild handoff, streaming create execution, GPU patch wiring, and readiness capture. * **Bug Fixes** * More consistent, centralized handling for both create failures and readiness failures, including redacted output, clearer diagnostics, and reliable retry guidance. * Safer cleanup on readiness failures to avoid same-name collisions, with correct behavior for GPU-enabled flows. * **Tests** * Added coverage for create/readiness failure reporting, exit-code fallback, cleanup/command messaging, and early-detach behavior in create-stream. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Dongni Yang <dongniy@nvidia.com> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Julie Yaunches <jyaunches@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
2 tasks
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
… heartbeats (NVIDIA#6002) (NVIDIA#6166) ## Summary Speed up local Docker-driver onboarding by prebuilding the staged sandbox image with BuildKit, while adding bounded progress heartbeats for wait-heavy phases. This revision resolves the branch conflicts through current main and reduces the original change from 1,972 to 1,240 added lines; production additions fall from 722 to 414, and the top-level `src/lib/onboard.ts` entrypoint is 22 lines smaller than main. ## Related Issue Fixes NVIDIA#6002 ## Changes - Build the already-staged sandbox context with an asynchronous, argv-based Docker BuildKit process so heartbeat timers continue to run. - Hand the build-qualified local image ref to OpenShell only for a local Docker-driver gateway; remote and ineligible paths retain the existing OpenShell build, and build failures fall back safely. - Persist the exact build-qualified image identity so recreating a sandbox cannot retarget existing snapshot clones. - Emit stateless 30-second heartbeats around wait-heavy onboarding phases, including resume and repair compatibility paths. - Run the live onboarding acceptance test sequentially in the existing full-e2e job. It verifies the literal [1/8] anchor, timestamped output gaps of at most 60 seconds, a real first response, BuildKit use, and the 3-minute budget. - Move prebuild launch coordination, non-interactive environment scoping, and readiness polling behind focused onboarding modules so the top-level entrypoint satisfies the growth guard. - Remove the earlier shared timing registry, singleton reporter, aggregate timing summary, duplicated seam support, and separate CI job. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: the optimization is transparent and falls back automatically; heartbeat output is self-describing. A documentation reviewer confirmed existing architecture and command docs remain accurate. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: independent final diff review found no blocking security or correctness issue; Docker uses argv with shell disabled, a filtered environment, local-driver gating, exact staged-image handoff, and build-unique tags. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub - [ ] Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes - [x] Targeted tests pass for changed behavior - [ ] Full npm test passes (broad runtime changes only) - [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) - [ ] Doc pages follow the style guide (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Targeted verification includes CLI type-check and build; focused onboarding, prebuild, progress, launch, and readiness suites (79 passing); prepared-context integrations (5 passing); 17 E2E workflow-contract tests; 93 latest-main overlap tests; 576 E2E-support tests; live-test collection; repository, source-shape, test-size, title-style, project-overlap, environment-documentation, and gitleaks gates. Commit and pre-push hooks passed. The combined local CLI/integration coverage hook was also attempted, but macOS cannot execute the Linux `script -qec` PTY contract; GitHub Linux CI is the authoritative full-suite result. --- Signed-off-by: Yimo Jiang <yimoj@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added clearer onboarding progress with periodic “Still working…” heartbeats and per-phase timing summaries. * Improved sandbox startup in eligible setups by prebuilding a local image and using it during sandbox creation. * **Bug Fixes** * Preserved real onboarding errors correctly while keeping failure reporting and telemetry accurate. * Refined non-interactive/interactive heartbeat behavior and improved sandbox readiness waiting and recovery-hint accuracy. * **Tests** * Added/extended onboarding progress, sandbox prebuild, and live e2e coverage to validate the end-to-end timing budget. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary This follow-up to NVIDIA#6166 validates the staged context before invoking host BuildKit and moves the hard NVIDIA#6002 cold-path acceptance assertions into the existing `full-e2e` lifecycle. It removes the redundant second onboarding run while preserving a distinct merge-failing signal alongside the advisory warm-system scorecard budget. ## Related Issue Follow-up to NVIDIA#6166 and NVIDIA#6002. ## Changes - Resolve and validate BuildKit contexts as private direct `os.tmpdir()/nemoclaw-build-*` staging directories with a no-follow regular Dockerfile, falling back to the gateway builder when validation fails. - Carry generated/custom provenance into the handoff so user-supplied `--from` Dockerfiles remain on the OpenShell gateway-builder trust boundary. - Cover custom provenance, outside-temp, wrong-prefix, writable-directory, symlink, non-regular-file, path-escape, inspection-error logging, environment sanitization, and compatibility-heartbeat behavior. - Measure BuildKit success, fallback absence, output silence, and the first real agent response during the job's first `full-e2e` onboarding instead of running a second warm-cache onboarding test. - Remove the redundant Vitest invocation from `e2e.yaml` and document the hard 180-second cold-path contract separately from the advisory 390-second warm-system scorecard budget. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Reviewed every production context stager against the new contract; explicit provenance keeps custom Dockerfiles off host BuildKit, focused negative tests cover each trust-boundary case, and the unavoidable post-validation Docker pathname reopen is mitigated by private `mkdtemp` staging. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [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) - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added optional cold-onboarding performance measurement to the live end-to-end flow, including trace-based evidence output. * **Bug Fixes** * Hardened local prebuild handling for NemoClaw-generated staged build contexts and tightened eligibility/`--from` validation behavior. * Improved sandbox build-context metadata propagation and reused Docker-driver gateway detection. * **Tests** * Expanded/refocused live end-to-end coverage and performance assertions; added trace/probe parsing fixtures; improved test isolation and cleanup. * Updated e2e workflow and release-gate checks to target the correct live Vitest test. * **Documentation** * Clarified `--from` build-context routing vs local BuildKit behavior on local Docker-driver gateways. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary This PR prepares the user-facing documentation for v0.0.74 before the release plan is frozen. It expands the release notes across the 56-commit train and closes durable documentation gaps found during the pre-tag commit scan. ## Changes - Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed MCP, progressive tool disclosure, LangChain Deep Agents Code, onboarding, local inference, messaging, recovery, and contributor workflows. - Correct the `destroy` contract for retained per-name volumes, gateway-unreachable `--force` cleanup, managed MCP ownership, and same-name recovery. - Document separate remediation for an unreachable container DNS resolver versus one that answers with `NXDOMAIN` or `REFUSED`. - Document the Windows on Arm N1X automatic Ollama safeguard and its remaining large-model limitations. - State that messaging conflicts abort rebuild before backup or deletion, leaving the original sandbox intact. - Link the agent-runnable value benchmark from the contributor task index. - Synchronize generated agent command variants. - Validate with `npm run docs:sync-agent-variants` and `npm run docs`; Fern completed with 0 errors and 2 existing warnings. - Source summary: - [NVIDIA#6020](NVIDIA#6020) and [NVIDIA#5876](NVIDIA#5876) -> `docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy boundary and managed MCP lifecycle. - [NVIDIA#6251](NVIDIA#6251) and [NVIDIA#5989](NVIDIA#5989) -> `docs/about/release-notes.mdx`: Summarize progressive tool disclosure and sandbox-first inference controls. - [NVIDIA#6232](NVIDIA#6232), [NVIDIA#6082](NVIDIA#6082), [NVIDIA#6219](NVIDIA#6219), [NVIDIA#6214](NVIDIA#6214), [NVIDIA#6215](NVIDIA#6215), [NVIDIA#6230](NVIDIA#6230), and [NVIDIA#6260](NVIDIA#6260) -> `docs/about/release-notes.mdx`: Summarize the experimental LangChain Deep Agents Code status, secret, version, rebuild, snapshot, and MCP boundaries. - [NVIDIA#6166](NVIDIA#6166), [NVIDIA#6254](NVIDIA#6254), [NVIDIA#6265](NVIDIA#6265), [NVIDIA#6164](NVIDIA#6164), and [NVIDIA#6017](NVIDIA#6017) -> `docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated image reuse, bounded readiness, and preflight improvements. - [NVIDIA#6150](NVIDIA#6150) -> `docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`: Separate unreachable-resolver remediation from reachable-but-rejected DNS responses. - [NVIDIA#6234](NVIDIA#6234) -> `docs/about/release-notes.mdx`, `docs/inference/use-local-inference.mdx`, and `docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B selection and the remaining explicit-large-model boundary. - [NVIDIA#6129](NVIDIA#6129), [NVIDIA#5987](NVIDIA#5987), [NVIDIA#5955](NVIDIA#5955), and [NVIDIA#6220](NVIDIA#6220) -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/commands.mdx`, and `docs/reference/commands-nemohermes.mdx`: Document messaging policy persistence, status, and the pre-destructive conflict check. - [NVIDIA#5963](NVIDIA#5963), [NVIDIA#6050](NVIDIA#6050), [NVIDIA#6094](NVIDIA#6094), [NVIDIA#6238](NVIDIA#6238), [NVIDIA#5988](NVIDIA#5988), [NVIDIA#6235](NVIDIA#6235), [NVIDIA#6181](NVIDIA#6181), and [NVIDIA#5986](NVIDIA#5986) -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and `docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and clarify retained-volume and local-only destroy semantics. - [NVIDIA#6200](NVIDIA#6200), [NVIDIA#6248](NVIDIA#6248), [NVIDIA#6168](NVIDIA#6168), [NVIDIA#6270](NVIDIA#6270), and [NVIDIA#5649](NVIDIA#5649) -> `docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize contributor setup and verification improvements and expose the advisory value benchmark. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: documentation-only release preparation; generated-variant synchronization and the Fern docs build validate the changed pages and routes. - [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: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed 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: tests are not applicable to this documentation-only change; `npm run docs` validates the source and generated routes. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) - [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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded setup guidance for Windows on Arm devices with safer default local model selection. * Clarified local inference and sandbox messaging behavior, including conflict checks before rebuilds and safer recovery steps. * Updated destroy/rebuild/reference docs with more detailed warnings, failure handling, and volume-retention guidance. * Improved troubleshooting instructions for Docker DNS issues with clearer paths for unreachable vs. blocked resolvers. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
…e step into modules (NVIDIA#6444) <!-- markdownlint-disable MD041 --> ## Summary Extracts cohesive units of the sandbox create/register orchestration out of the ~4,900-line `src/lib/onboard.ts` entrypoint into focused modules under `src/lib/onboard/`, following the injected-deps boundary style established by `created-sandbox-finalization.ts` (NVIDIA#6332). The primary goal is maintainability and independent unit-test coverage for the create path, with intentional safety/behavior refinements discovered during review: redacted create-output failure echoing, preservation of non-zero create-stream status when readiness fails, direct argv spawning for trusted create paths, fail-closed Docker-GPU create-poll side-effect handling, and redacted trace reporting for poll/readiness errors. Note on the issue's premise: NVIDIA#6258 cites a `+27 net lines` growth-guard violation from NVIDIA#6166. That premise is stale — the merged NVIDIA#6166 left `onboard.ts` net-smaller, and NVIDIA#6276/NVIDIA#6332 shrank it further, so the guard (`.github/workflows/codebase-growth-guardrails.yaml`, a per-PR net-neutral diff gate) is not currently red. This PR is therefore incremental maintainability work; it keeps `onboard.ts` net-neutral-or-smaller so the gate stays green. ## Related Issue Refs NVIDIA#6258 <!-- Refs (not Fixes): this PR lands two increments of a larger extraction; the remaining create/finalize wiring is left for follow-ups, so the issue should stay open. --> ## Changes - Add `src/lib/onboard/created-sandbox-failure.ts`: `reportSandboxCreateFailure` (warns-and-continues on an incomplete create; otherwise prints diagnostics + recovery hints and exits) and `reportSandboxReadinessFailure` (prints the readiness failure, defers cleanup to the Docker-GPU patch or deletes the failed sandbox, then exits). `onboard.ts` replaces the two inline blocks with module calls — net −2 lines. - Add `src/lib/onboard/sandbox-create-step.ts`: `runSandboxCreateStep` encapsulates the BuildKit prebuild handoff → Docker-GPU create-patch provisioning → create-stream behind a context + injected-deps boundary. This move is **line-neutral** on `onboard.ts`; its value is a named, unit-testable boundary (the `prepare → patch → stream` sequence is now testable without standing up the entrypoint), not a size reduction. Build-context and exit-listener cleanup stay with the caller that armed them. - Add focused unit tests across `created-sandbox-failure.test.ts`, `sandbox-create-step.test.ts`, `sandbox-create-launch.test.ts`, `create-stream.test.ts`, `create-stream-argv.test.ts`, and `create-stream-ready-gate.test.ts` covering failure branches, redaction, exit-code preservation, GPU vs non-GPU readiness cleanup, prebuild/patch/stream wiring, direct argv spawn boundaries, terminal-agent/default-driver ready-check gating, fail-closed `onPoll` error handling, and redacted poll/readiness trace behavior. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: no CLI flags, commands, configuration, or documented user workflow changed. The user-visible differences are limited to safer failure-path diagnostics (credential redaction), more accurate readiness-failure exit status, and fail-closed create-poll error handling. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: onboarding/sandbox path. Extraction boundaries were verified against the pre-extraction source; intentional safety refinements are explicitly covered by tests: create output is redacted before failure logging, readiness failure preserves a non-zero create-stream status instead of flattening to `1`, Docker-GPU during-create polling is isolated from readiness detection via `onPoll`, escaping poll errors abort create with classified/generic failure text plus redacted trace emission, and ready-check exceptions emit redacted trace evidence without falsely forcing Ready. Requesting maintainer sensitive-path review. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed 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: `npx vitest run --project cli src/lib/actions/sandbox/snapshot.test.ts src/lib/onboard/created-sandbox-failure.test.ts src/lib/onboard/sandbox-create-launch.test.ts src/lib/onboard/sandbox-create-step.test.ts src/lib/sandbox/create-stream.test.ts src/lib/sandbox/create-stream-ready-gate.test.ts src/lib/sandbox/create-stream-argv.test.ts` → 107 passed; `npx tsc --noEmit --pretty false --project tsconfig.cli.json` → clean; `npm run test-size:check` → passed; `npx prek run --all-files --stage pre-commit --skip source-shape-test-budget --skip test-skills-yaml` → passed; `npm run test-conditionals:scan -- --top 25` → no new changed-file conditional failures; `git diff --check` → clean. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Required live E2E run [29021019725](https://github.com/NVIDIA/NemoClaw/actions/runs/29021019725) passed on exact head `82518f7e6f69c39d9bc04f63a365753aac7b1b6d`: `cloud-onboard`, `onboard-repair`, `onboard-resume`, `state-backup-restore`, `upgrade-stale-sandbox`, and `snapshot-commands` all succeeded. PR CI checks also passed on the same head after rerunning flaky `policy-channel-list.test.ts` shard timeout (`cli-test-shards (3)`). - [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) - [ ] 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) --- Signed-off-by: Dongni Yang <dongniy@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved sandbox onboarding using a dedicated creation step that centralizes prebuild handoff, streaming create execution, GPU patch wiring, and readiness capture. * **Bug Fixes** * More consistent, centralized handling for both create failures and readiness failures, including redacted output, clearer diagnostics, and reliable retry guidance. * Safer cleanup on readiness failures to avoid same-name collisions, with correct behavior for GPU-enabled flows. * **Tests** * Added coverage for create/readiness failure reporting, exit-code fallback, cleanup/command messaging, and early-detach behavior in create-stream. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Dongni Yang <dongniy@nvidia.com> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Julie Yaunches <jyaunches@nvidia.com> Co-authored-by: Carlos Villela <cvillela@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
4 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
Speed up local Docker-driver onboarding by prebuilding the staged sandbox image with BuildKit, while adding bounded progress heartbeats for wait-heavy phases. This revision resolves the branch conflicts through current main and reduces the original change from 1,972 to 1,240 added lines; production additions fall from 722 to 414, and the top-level
src/lib/onboard.tsentrypoint is 22 lines smaller than main.Related Issue
Fixes #6002
Changes
Type of Change
Quality Gates
Verification
Targeted verification includes CLI type-check and build; focused onboarding, prebuild, progress, launch, and readiness suites (79 passing); prepared-context integrations (5 passing); 17 E2E workflow-contract tests; 93 latest-main overlap tests; 576 E2E-support tests; live-test collection; repository, source-shape, test-size, title-style, project-overlap, environment-documentation, and gitleaks gates. Commit and pre-push hooks passed. The combined local CLI/integration coverage hook was also attempted, but macOS cannot execute the Linux
script -qecPTY contract; GitHub Linux CI is the authoritative full-suite result.Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit