Closed
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSplit and modularized the onboarding implementation into many focused modules (host, inference, sandbox, runtime, policy, orchestrator), added a typed FSM and session v2 with runtime-step sync, extracted gateway/dashboard/runtime helpers, added hadolint wrapper script, and introduced extensive tests across the new modules. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(100,149,237,0.5)
participant CLI
end
rect rgba(34,139,34,0.5)
participant Entrypoint as runOnboardingEntry
participant Orchestrator as runOnboardingOrchestrator
end
rect rgba(255,165,0,0.5)
participant Host
participant Inference
participant Sandbox
participant Runtime
participant Policy
end
CLI->>Entrypoint: invoke onboard flow
Entrypoint->>Orchestrator: initialize & start
Orchestrator->>Host: host.run() -> detect GPU / gateway
Orchestrator->>Inference: inference.run() -> provider/model selection
Orchestrator->>Sandbox: sandbox.run() -> create/restore sandbox
Orchestrator->>Runtime: runtime.run() -> agent/OpenClaw setup
Orchestrator->>Policy: policy.run() -> apply/persist presets
Orchestrator-->>Entrypoint: return final result
Entrypoint-->>CLI: exit / print dashboard
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 9
🧹 Nitpick comments (6)
src/lib/onboard-fsm.ts (1)
196-214: Derive the empty ledger fromONBOARD_SESSION_STEPS.This initializer duplicates the step list in a second place. The next step/alias addition can update the unions above and still forget this object, leaving the mismatch to surface later at runtime.
Suggested refactor
export function createEmptyStepLedger(): OnboardStepLedger { const emptyStep = (): OnboardStepState => ({ status: "pending", startedAt: null, completedAt: null, error: null, }); - return { - preflight: emptyStep(), - gateway: emptyStep(), - provider_selection: emptyStep(), - inference: emptyStep(), - messaging: emptyStep(), - sandbox: emptyStep(), - runtime_setup: emptyStep(), - policies: emptyStep(), - openclaw: emptyStep(), - agent_setup: emptyStep(), - }; + return Object.fromEntries( + ONBOARD_SESSION_STEPS.map((stepName) => [stepName, emptyStep()]), + ) as OnboardStepLedger; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-fsm.ts` around lines 196 - 214, createEmptyStepLedger duplicates the step list; derive it from ONBOARD_SESSION_STEPS instead to avoid drift. Replace the hardcoded object in createEmptyStepLedger with logic that builds the ledger from ONBOARD_SESSION_STEPS (e.g., create emptyStep() and use Object.fromEntries(ONBOARD_SESSION_STEPS.map(step => [step, emptyStep()]))) and cast/ensure the result matches the OnboardStepLedger type so keys align with the ONBOARD_SESSION_STEPS union; keep the emptyStep helper and returned shape (status, startedAt, completedAt, error).src/lib/onboard-recorders.test.ts (1)
11-36: Consider extracting cache-clear boilerplate into a helper.
beforeEach/afterEachrepeat the same dist module cache cleanup logic; a tiny helper would reduce duplication and keep future module-list edits safer.♻️ Proposed cleanup
const require = createRequire(import.meta.url); const recordersDistPath = require.resolve("../../dist/lib/onboard-recorders"); const driverDistPath = require.resolve("../../dist/lib/onboard-persistent-driver"); const sessionDistPath = require.resolve("../../dist/lib/onboard-session"); +const distModulePaths = [recordersDistPath, driverDistPath, sessionDistPath] as const; + +const clearDistModuleCache = () => { + for (const modulePath of distModulePaths) delete require.cache[modulePath]; +}; beforeEach(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-recorders-")); process.env.HOME = tmpDir; - delete require.cache[recordersDistPath]; - delete require.cache[driverDistPath]; - delete require.cache[sessionDistPath]; + clearDistModuleCache(); }); afterEach(() => { - delete require.cache[recordersDistPath]; - delete require.cache[driverDistPath]; - delete require.cache[sessionDistPath]; + clearDistModuleCache(); fs.rmSync(tmpDir, { recursive: true, force: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-recorders.test.ts` around lines 11 - 36, Extract the repeated cache cleanup into a small helper function (e.g., clearDistCache) and call it from both beforeEach and afterEach instead of duplicating the delete require.cache[...] lines; locate the three module identifiers recordersDistPath, driverDistPath, sessionDistPath and replace the three delete statements in beforeEach and afterEach with a single call to clearDistCache (and keep the other setup/teardown logic in place).src/lib/onboard-orchestrator-deps.test.ts (1)
15-35: Isolate env mutations and make the clear-env assertion deterministic.Line 130 mutates process env and Line 149 asserts clearing
NVIDIA_API_KEY, but the test never seeds that key first and does not restore all mutated env vars after execution.Suggested hardening patch
const originalHome = process.env.HOME; +const originalOpenshellBin = process.env.NEMOCLAW_OPENSHELL_BIN; +const originalNvidiaApiKey = process.env.NVIDIA_API_KEY; let tmpDir: string; @@ afterEach(() => { @@ if (originalHome === undefined) { delete process.env.HOME; } else { process.env.HOME = originalHome; } + if (originalOpenshellBin === undefined) { + delete process.env.NEMOCLAW_OPENSHELL_BIN; + } else { + process.env.NEMOCLAW_OPENSHELL_BIN = originalOpenshellBin; + } + if (originalNvidiaApiKey === undefined) { + delete process.env.NVIDIA_API_KEY; + } else { + process.env.NVIDIA_API_KEY = originalNvidiaApiKey; + } }); @@ deps.host.getNamedGatewayInfo(); deps.host.getActiveGatewayInfo(); deps.host.stopDashboardForward(); deps.inference.setOpenshellBinary("/tmp/openshell"); + process.env.NVIDIA_API_KEY = "test-key"; deps.inference.clearSensitiveEnv();Also applies to: 129-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-orchestrator-deps.test.ts` around lines 15 - 35, The test mutates process.env and asserts that NVIDIA_API_KEY was cleared without seeding or fully restoring env; fix by snapshotting and restoring the whole env around each test: at top capture const originalEnv = { ...process.env }, in beforeEach set up tmpDir and seed any keys the test expects (e.g. process.env.NVIDIA_API_KEY = 'test-key') and set process.env.HOME = tmpDir, and in afterEach restore environment deterministically by clearing current process.env keys and copying originalEnv back (while still cleaning tmpDir and clearing require.cache for bootstrapDistPath/contextDistPath/depsDistPath) so the clear-env assertion is deterministic; reference beforeEach, afterEach, tmpDir, originalHome, and the NVIDIA_API_KEY assertion when making changes.src/lib/onboard-dashboard.ts (1)
14-27: Recursive search may be unnecessary given known download path.Since the download command specifies a known source path
/sandbox/.openclaw/openclaw.json, the recursive search could potentially be replaced with a direct path check. However, ifopenshell sandbox downloadrecreates the directory structure or has variable output paths, the recursive search is appropriate.♻️ Potential simplification if output path is predictable
-function findOpenclawJsonPath(dir: string): string | null { - if (!fs.existsSync(dir)) return null; - const entries = fs.readdirSync(dir, { withFileTypes: true }); - for (const entry of entries) { - const filePath = path.join(dir, entry.name); - if (entry.isDirectory()) { - const found = findOpenclawJsonPath(filePath); - if (found) return found; - } else if (entry.name === "openclaw.json") { - return filePath; - } - } - return null; -} +function findOpenclawJsonPath(dir: string): string | null { + // Check common paths first + const directPath = path.join(dir, "openclaw.json"); + if (fs.existsSync(directPath)) return directPath; + + const nestedPath = path.join(dir, "sandbox", ".openclaw", "openclaw.json"); + if (fs.existsSync(nestedPath)) return nestedPath; + + // Fall back to recursive search if needed + // ... existing recursive logic ... +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-dashboard.ts` around lines 14 - 27, The recursive finder findOpenclawJsonPath currently walks dir trees to locate openclaw.json; since the download command writes to a known location (/sandbox/.openclaw/openclaw.json), replace the full recursive walk with a direct existence check for that expected path first (e.g., construct path.join(dir, '.openclaw', 'openclaw.json') and return it if fs.existsSync), and keep the current recursive fallback only if the direct-check fails; update the function name usages if necessary and ensure it still returns string|null.src/lib/onboard-driver.ts (1)
262-297: Consider extracting valid failure phases to a Set for clarity.The phase validation could be slightly cleaner with a Set lookup, though the current implementation is functionally correct.
♻️ Optional simplification
+const VALID_FAILURE_PHASES = new Set([ + "preflight", + "gateway", + "provider_selection", + "inference", + "messaging", + "sandbox", + "runtime_setup", + "policies", +] as const); + fail(message: string, code = "driver_failure"): this { const failurePhase = this.#state.phase === "boot" ? "preflight" : this.#state.phase; - if ( - failurePhase === "preflight" || - failurePhase === "gateway" || - failurePhase === "provider_selection" || - failurePhase === "inference" || - failurePhase === "messaging" || - failurePhase === "sandbox" || - failurePhase === "runtime_setup" || - failurePhase === "policies" - ) { + if (VALID_FAILURE_PHASES.has(failurePhase as typeof VALID_FAILURE_PHASES extends Set<infer T> ? T : never)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-driver.ts` around lines 262 - 297, The phase-check in fail(message, code) is verbose; extract the allowed failure phases into a Set (e.g., const validFailurePhases = new Set([...])) and replace the long OR chain with validFailurePhases.has(this.#state.phase); update the local variable failurePhase and subsequent logic (including the runtime_setup ternary that determines failureStep), then call applyStepFailed(this.#session, failureStep, message) and deriveOnboardFlowState(this.#session, { resume: true, requestedSandboxName: this.#requestedSandboxName }) exactly as before to preserve behavior.src/lib/onboard-requests.ts (1)
64-80: DefaultisSafeModelIdalways returnstrue— callers must inject validation.The default
isSafeModelId: () => trueon line 71 means model validation is effectively skipped unless the caller explicitly provides a validator. This is likely intentional for flexibility, but consider documenting this expectation in the interface or function JSDoc to prevent accidental use without validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-requests.ts` around lines 64 - 80, Update the function documentation for getNonInteractiveModel to explicitly state that the default isSafeModelId provided in NonInteractiveRequestDeps is intentionally permissive (returns true) so validation is skipped unless the caller injects their own validator; mention the exact symbol names (getNonInteractiveModel, NonInteractiveRequestDeps, isSafeModelId) and instruct callers to pass a validator that enforces the allowed characters (letters, numbers, '.', '_', ':', '/', and '-') or to use a shared helper validator if you add one. Ensure the JSDoc sits immediately above getNonInteractiveModel and briefly describes the recommended validation behavior and the security implication of leaving isSafeModelId as the default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 21: Add the repo-standard SPDX license header to the top of
CONTRIBUTING.md (same header used across the repository for files matching
**/*.{js,ts,tsx,jsx,sh,md}); insert the exact SPDX line the project uses (e.g.
SPDX-License-Identifier: <LICENSE_IDENTIFIER>) as the first non-blank content in
the file so the markdown file complies with the repo's SPDX header requirement.
In `@src/lib/onboard-flow-state.ts`:
- Around line 21-23: The runtimeTarget assignment incorrectly treats any truthy
session.agent as an agent path, so sessions with session.agent === "openclaw"
take the wrong branch; update the condition in the runtimeTarget logic (the
expression that currently checks session.agent) to explicitly treat "openclaw"
as the default runtime by using a predicate like session.agent && session.agent
!== "openclaw" so that runtimeTarget becomes { kind: "agent", agentName:
session.agent } only for real agent names and falls back to { kind: "openclaw" }
for legacy "openclaw".
In `@src/lib/onboard-fsm.ts`:
- Around line 415-420: The code assumes a valid phase->event handler exists but
can get bad runtime data; update the logic around ONBOARD_TRANSITIONS,
phaseTransitions and handler so you explicitly detect and throw a clear error
when a transition is unsupported: verify ONBOARD_TRANSITIONS[state.phase] yields
a valid phaseTransitions object and that phaseTransitions[event.type] is a
function, and if not throw an Error that includes the offending state.phase and
event.type (and optionally the allowed event keys via
Object.keys(phaseTransitions)) before returning handler(state, event); reference
the symbols ONBOARD_TRANSITIONS, phaseTransitions, handler, state.phase and
event.type when making the change.
In `@src/lib/onboard-inference-loop.ts`:
- Around line 39-42: The cleanup hook currently can't remove the
provider-specific env var because clearSensitiveEnv has no parameter; change
clearSensitiveEnv to accept the active credentialEnv (string | null), update the
caller in the onboard-inference-loop to pass the same credentialEnv used by
hydrateCredentialEnv, and update all implementations of clearSensitiveEnv to
delete process.env[credentialEnv] when credentialEnv is non-null; keep
hydrateCredentialEnv, getOpenshellBinary, setOpenshellBinary signatures but
ensure the loop calls clearSensitiveEnv(credentialEnv) where credentialEnv was
set.
In `@src/lib/onboard-openshell.ts`:
- Around line 64-79: The install success check uses resolveOpenshell()
(openshellBinary) which relies on the current PATH and can return null
immediately after a fresh install; change the installed determination to
consider the actual file presence at openshellPath (use
deps.existsSync(openshellPath)) in addition to resolveOpenshell so fresh
installs report installed: true. Concretely, set installed to
deps.existsSync(openshellPath) || openshellBinary !== null, keep
futureShellPathHint and updatedPathValue logic based on
deps.existsSync(openshellPath) as before, and ensure you reference
openshellPath, openshellBinary, futureShellPathHint, and updatedPathValue when
making this change.
In `@src/lib/onboard-remediation.ts`:
- Around line 55-60: The darwin branch currently emits a literal `launchctl
unload ${launchAgentPlist}` even when launchAgentPlist is an empty string;
update the code in the platform === "darwin" block (where launchAgentPlist is
used) to skip or omit the `launchctl unload` line when launchAgentPlist is falsy
(empty string), e.g. only push/return the unload command if launchAgentPlist
truthy or replace it with a safe conditional message like "launchctl unload
<path-to-plist>" when not provided so users aren't shown a broken `launchctl
unload ` command.
In `@src/lib/onboard-resume.ts`:
- Around line 111-114: In the fromDockerfile conflict branch (check for
conflict.field === "fromDockerfile"), flip the guidance for the
!conflict.recorded case: when conflict.recorded is null the session was started
without --from, so instruct the user to remove the --from flag (or resume
without --from) instead of telling them to add it; use the existing symbols
conflict.recorded and conflict.requested to build the corrected message and
ensure the other branch (when conflict.recorded is truthy) still provides the
appropriate add --from guidance.
In `@src/lib/onboard-shell.ts`:
- Around line 41-42: The command builder buildOnboardLockCommand currently
interpolates state.requestedFromDockerfile unquoted, which breaks copy-paste
when the path contains spaces; update the string to wrap the --from value in
quotes (e.g., --from "…") and ensure the quoting only applies when
requestedFromDockerfile is truthy so the produced command remains identical
otherwise; modify the template expression referencing
state.requestedFromDockerfile in buildOnboardLockCommand to include surrounding
quotes.
In `@test/gateway-liveness-probe.test.ts`:
- Around line 61-65: The current test aggregates matches from content and
helperContent which can false-pass if both matches are in one file; update the
assertions to check each site independently by asserting preflightDowngrades
(from content) has length >= 1 and helperDowngrades (from helperContent) has
length >= 1 so both preflight and helper sites verify the presence of
containerState === "missing" (use the existing variables preflightDowngrades,
helperDowngrades, content, and helperContent to locate and change the
assertions).
---
Nitpick comments:
In `@src/lib/onboard-dashboard.ts`:
- Around line 14-27: The recursive finder findOpenclawJsonPath currently walks
dir trees to locate openclaw.json; since the download command writes to a known
location (/sandbox/.openclaw/openclaw.json), replace the full recursive walk
with a direct existence check for that expected path first (e.g., construct
path.join(dir, '.openclaw', 'openclaw.json') and return it if fs.existsSync),
and keep the current recursive fallback only if the direct-check fails; update
the function name usages if necessary and ensure it still returns string|null.
In `@src/lib/onboard-driver.ts`:
- Around line 262-297: The phase-check in fail(message, code) is verbose;
extract the allowed failure phases into a Set (e.g., const validFailurePhases =
new Set([...])) and replace the long OR chain with
validFailurePhases.has(this.#state.phase); update the local variable
failurePhase and subsequent logic (including the runtime_setup ternary that
determines failureStep), then call applyStepFailed(this.#session, failureStep,
message) and deriveOnboardFlowState(this.#session, { resume: true,
requestedSandboxName: this.#requestedSandboxName }) exactly as before to
preserve behavior.
In `@src/lib/onboard-fsm.ts`:
- Around line 196-214: createEmptyStepLedger duplicates the step list; derive it
from ONBOARD_SESSION_STEPS instead to avoid drift. Replace the hardcoded object
in createEmptyStepLedger with logic that builds the ledger from
ONBOARD_SESSION_STEPS (e.g., create emptyStep() and use
Object.fromEntries(ONBOARD_SESSION_STEPS.map(step => [step, emptyStep()]))) and
cast/ensure the result matches the OnboardStepLedger type so keys align with the
ONBOARD_SESSION_STEPS union; keep the emptyStep helper and returned shape
(status, startedAt, completedAt, error).
In `@src/lib/onboard-orchestrator-deps.test.ts`:
- Around line 15-35: The test mutates process.env and asserts that
NVIDIA_API_KEY was cleared without seeding or fully restoring env; fix by
snapshotting and restoring the whole env around each test: at top capture const
originalEnv = { ...process.env }, in beforeEach set up tmpDir and seed any keys
the test expects (e.g. process.env.NVIDIA_API_KEY = 'test-key') and set
process.env.HOME = tmpDir, and in afterEach restore environment
deterministically by clearing current process.env keys and copying originalEnv
back (while still cleaning tmpDir and clearing require.cache for
bootstrapDistPath/contextDistPath/depsDistPath) so the clear-env assertion is
deterministic; reference beforeEach, afterEach, tmpDir, originalHome, and the
NVIDIA_API_KEY assertion when making changes.
In `@src/lib/onboard-recorders.test.ts`:
- Around line 11-36: Extract the repeated cache cleanup into a small helper
function (e.g., clearDistCache) and call it from both beforeEach and afterEach
instead of duplicating the delete require.cache[...] lines; locate the three
module identifiers recordersDistPath, driverDistPath, sessionDistPath and
replace the three delete statements in beforeEach and afterEach with a single
call to clearDistCache (and keep the other setup/teardown logic in place).
In `@src/lib/onboard-requests.ts`:
- Around line 64-80: Update the function documentation for
getNonInteractiveModel to explicitly state that the default isSafeModelId
provided in NonInteractiveRequestDeps is intentionally permissive (returns true)
so validation is skipped unless the caller injects their own validator; mention
the exact symbol names (getNonInteractiveModel, NonInteractiveRequestDeps,
isSafeModelId) and instruct callers to pass a validator that enforces the
allowed characters (letters, numbers, '.', '_', ':', '/', and '-') or to use a
shared helper validator if you add one. Ensure the JSDoc sits immediately above
getNonInteractiveModel and briefly describes the recommended validation behavior
and the security implication of leaving isSafeModelId as the default.
🪄 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: Pro Plus
Run ID: 33e67765-f152-43e6-b6c1-61282fdd4186
📒 Files selected for processing (55)
.agents/skills/nemoclaw-user-reference/references/troubleshooting.md.pre-commit-config.yamlCONTRIBUTING.mdscripts/run-hadolint.shsrc/lib/onboard-bootstrap.test.tssrc/lib/onboard-bootstrap.tssrc/lib/onboard-dashboard-print.test.tssrc/lib/onboard-dashboard-print.tssrc/lib/onboard-dashboard.test.tssrc/lib/onboard-dashboard.tssrc/lib/onboard-driver.test.tssrc/lib/onboard-driver.tssrc/lib/onboard-entry.test.tssrc/lib/onboard-entry.tssrc/lib/onboard-flow-state.test.tssrc/lib/onboard-flow-state.tssrc/lib/onboard-fsm.test.tssrc/lib/onboard-fsm.tssrc/lib/onboard-host-flow.test.tssrc/lib/onboard-host-flow.tssrc/lib/onboard-inference-loop.test.tssrc/lib/onboard-inference-loop.tssrc/lib/onboard-openshell.test.tssrc/lib/onboard-openshell.tssrc/lib/onboard-orchestrator-deps.test.tssrc/lib/onboard-orchestrator-deps.tssrc/lib/onboard-orchestrator.test.tssrc/lib/onboard-orchestrator.tssrc/lib/onboard-persistent-driver.test.tssrc/lib/onboard-persistent-driver.tssrc/lib/onboard-policy-flow.test.tssrc/lib/onboard-policy-flow.tssrc/lib/onboard-recorders.test.tssrc/lib/onboard-recorders.tssrc/lib/onboard-remediation.test.tssrc/lib/onboard-remediation.tssrc/lib/onboard-requests.test.tssrc/lib/onboard-requests.tssrc/lib/onboard-resume.test.tssrc/lib/onboard-resume.tssrc/lib/onboard-run-context.test.tssrc/lib/onboard-run-context.tssrc/lib/onboard-runtime-flow.test.tssrc/lib/onboard-runtime-flow.tssrc/lib/onboard-sandbox-flow.test.tssrc/lib/onboard-sandbox-flow.tssrc/lib/onboard-session.test.tssrc/lib/onboard-session.tssrc/lib/onboard-shell.test.tssrc/lib/onboard-shell.tssrc/lib/onboard.tstest/e2e/test-onboard-resume.shtest/gateway-liveness-probe.test.tstest/onboard.test.tstest/runner.test.ts
Contributor
Author
|
Follow-up on the latest CodeRabbit nitpicks that were summarized in the review body rather than posted as separate line threads: in 2ea3206 I also (1) extracted the shared ANSI regex into src/lib/ansi-utils.ts and wired onboard.ts + onboard-gateway-runtime.ts to use it, (2) switched the CoreDNS patch invocation in onboard-gateway-runtime.ts to argv-style execution so the gateway name is not shell-interpolated, and (3) simplified getGatewayStartEnv() to check only stableGatewayImage. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/lib/onboard-shell.ts (1)
41-46:⚠️ Potential issue | 🟠 MajorUse shell-safe quoting for
--from(double quotes still allow expansion).Line 43 uses
JSON.stringify, which produces double-quoted shell args;$VAR/$(...)still expand when users copy-paste this command, so it is not a reliable lock/debug reproduction command.Proposed fix
export function buildOnboardLockCommand(state: Pick<OnboardShellState, "resume" | "nonInteractive" | "requestedFromDockerfile">): string { + const quoteShellArg = (value: string) => `'${value.replace(/'/g, `'\\''`)}'`; const fromArg = state.requestedFromDockerfile - ? ` --from ${JSON.stringify(state.requestedFromDockerfile)}` + ? ` --from ${quoteShellArg(state.requestedFromDockerfile)}` : ""; return `nemoclaw onboard${state.resume ? " --resume" : ""}${state.nonInteractive ? " --non-interactive" : ""}${fromArg}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-shell.ts` around lines 41 - 46, The current buildOnboardLockCommand uses JSON.stringify for the --from value which creates double-quoted args that still allow shell expansion; change the quoting logic in buildOnboardLockCommand (specifically the fromArg computation that reads state.requestedFromDockerfile) to produce a shell-safe single-quoted value by escaping any single quotes in the string (replace ' with '\'' pattern) and wrapping the final value in single quotes so that the returned command is safe from shell expansion when copied.
🧹 Nitpick comments (1)
src/lib/onboard-driver.ts (1)
117-285: Consider extracting a shared post-step state update helper.The transition-vs-derive resume branch repeats in each
finish*method. A single helper would reduce duplication and lower maintenance risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-driver.ts` around lines 117 - 285, Multiple finish* methods (finishPreflight, finishGateway, finishProviderSelection, finishInference, finishMessaging, finishSandbox, finishRuntimeSetup, finishPolicies) duplicate the same "if (this.#state.phase === ... ) { transitionOnboardState(...) } else { deriveOnboardFlowState(this.#session, { resume: true, requestedSandboxName: this.#requestedSandboxName }) }" pattern; extract a private helper (e.g., applyStepAndAdvance or updateStateAfterStep) that takes the current step phase check value and the transition payload (or a callback to build it), calls applyStepComplete / applySessionComplete where needed, performs the conditional state update using transitionOnboardState or deriveOnboardFlowState, and assigns the result to this.#state; then replace each finish* method's duplicated branch with a single call to that helper passing the unique transition payloads and any session-specific metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/onboard-dashboard.ts`:
- Around line 218-220: ensureDashboardForward currently hard-codes the fallback
chatUiUrl to "http://127.0.0.1:${CONTROL_UI_PORT}" which bypasses
process.env.CHAT_UI_URL used elsewhere; update the chatUiUrl assignment in
ensureDashboardForward to use deps.chatUiUrl || process.env.CHAT_UI_URL ||
`http://127.0.0.1:${CONTROL_UI_PORT}` so getDashboardForwardPort(chatUiUrl) and
getDashboardForwardTarget(chatUiUrl) operate on the same configured URL,
preserving callers that set CHAT_UI_URL.
- Around line 160-166: The code computes chatUiUrl from options.chatUiUrl but
still calls buildControlUiUrls() which reads process.env.CHAT_UI_URL, so custom
options.chatUiUrl is not used when building the URL list; update the call to
buildControlUiUrls to use the computed chatUiUrl (e.g., add a parameter or
overload so you call buildControlUiUrls(token, dashboardPort, chatUiUrl)) or
change buildControlUiUrls to accept a baseUrl argument and pass chatUiUrl
through; keep getDashboardForwardPort(chatUiUrl) and
buildAuthenticatedDashboardUrl(url, null) as-is so the returned dashboardAccess
uses the correct base URL and port.
In `@src/lib/onboard-driver.ts`:
- Around line 198-209: finishMessaging stores the incoming messagingChannels
array by reference which allows external mutation to affect persisted
session/state; update finishMessaging to defensively copy the array (e.g.,
create a new array from messagingChannels) before passing it into
applyStepComplete and the transitionOnboardState call, and do the same defensive
copy pattern for the similar usage of policyPresets in the other method
(referenced in the review) so neither applyStepComplete nor
transitionOnboardState receive the original mutable array.
- Around line 105-106: The state getter currently returns the internal mutable
object (this.#state), so change the getter OnboardDriver.state to return an
immutable copy instead: produce a deep/structured clone of this.#state (e.g.,
using structuredClone or JSON deep clone) and freeze it (Object.freeze or deep
freeze) before returning to ensure callers cannot mutate internal state; update
the getter implementation in the OnboardDriver class (the state accessor that
returns this.#state) to return the cloned/frozen OnboardFlowState instead.
- Around line 287-309: The fail method currently re-inserts the raw message into
this.#state.error.message after calling applyStepFailed; instead use the
sanitized/redacted message produced by applyStepFailed/deriveOnboardFlowState.
Change the block in fail (after applyStepFailed and deriveOnboardFlowState) so
that when setting this.#state.error you assign message:
this.#state.error?.message ?? message (or otherwise read the sanitized text from
the updated this.#state/session failure object) rather than the original raw
message, keeping code paths around fail, applyStepFailed, and
deriveOnboardFlowState intact.
In `@src/lib/onboard-flow-state.ts`:
- Around line 152-153: The code currently coerces missing persisted fields
(session.sandboxName, session.provider, session.model, session.policyPresets) to
empty strings/arrays in the complete-branch; remove those ?? ""/?? [] coercions
and instead validate that the required fields exist before returning phase:
"complete" — if any are missing, return a non-complete flow state (e.g., rewind
to an earlier phase such as the provider/sandbox selection phase or return an
explicit resume/error phase) so the UI can surface persistence issues; update
the branches that reference session.status and session.steps.policies.status to
perform this presence check and only spread values into ctx when they are
present (keep original values when present, do not manufacture empty
placeholders).
In `@src/lib/onboard-inference-loop.ts`:
- Around line 83-85: The credential environment is being hydrated via
deps.hydrateCredentialEnv(state.credentialEnv) but not reliably cleared on early
exits or exceptions; wrap the hydrate/use sequence in a try/finally (or ensure a
finally-like cleanup) so deps.clearCredentialEnv() is always called, and add
explicit calls to deps.clearCredentialEnv() on the onSkip early-return paths
(the provider-selection skip and the other skip/return branches around the
setupInference flow referenced in lines 118-129 and 137-145) so sensitive values
never remain in process.env even if setupInference throws or inference is
skipped.
---
Duplicate comments:
In `@src/lib/onboard-shell.ts`:
- Around line 41-46: The current buildOnboardLockCommand uses JSON.stringify for
the --from value which creates double-quoted args that still allow shell
expansion; change the quoting logic in buildOnboardLockCommand (specifically the
fromArg computation that reads state.requestedFromDockerfile) to produce a
shell-safe single-quoted value by escaping any single quotes in the string
(replace ' with '\'' pattern) and wrapping the final value in single quotes so
that the returned command is safe from shell expansion when copied.
---
Nitpick comments:
In `@src/lib/onboard-driver.ts`:
- Around line 117-285: Multiple finish* methods (finishPreflight, finishGateway,
finishProviderSelection, finishInference, finishMessaging, finishSandbox,
finishRuntimeSetup, finishPolicies) duplicate the same "if (this.#state.phase
=== ... ) { transitionOnboardState(...) } else {
deriveOnboardFlowState(this.#session, { resume: true, requestedSandboxName:
this.#requestedSandboxName }) }" pattern; extract a private helper (e.g.,
applyStepAndAdvance or updateStateAfterStep) that takes the current step phase
check value and the transition payload (or a callback to build it), calls
applyStepComplete / applySessionComplete where needed, performs the conditional
state update using transitionOnboardState or deriveOnboardFlowState, and assigns
the result to this.#state; then replace each finish* method's duplicated branch
with a single call to that helper passing the unique transition payloads and any
session-specific metadata.
🪄 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: Pro Plus
Run ID: f158e217-07a8-4753-968f-0229af6af869
📒 Files selected for processing (27)
CONTRIBUTING.mdsrc/lib/ansi-utils.tssrc/lib/onboard-dashboard.tssrc/lib/onboard-driver.tssrc/lib/onboard-flow-state.test.tssrc/lib/onboard-flow-state.tssrc/lib/onboard-fsm.test.tssrc/lib/onboard-fsm.tssrc/lib/onboard-gateway-runtime.test.tssrc/lib/onboard-gateway-runtime.tssrc/lib/onboard-inference-loop.test.tssrc/lib/onboard-inference-loop.tssrc/lib/onboard-openshell.test.tssrc/lib/onboard-openshell.tssrc/lib/onboard-orchestrator-deps.test.tssrc/lib/onboard-orchestrator-deps.tssrc/lib/onboard-recorders.test.tssrc/lib/onboard-remediation.test.tssrc/lib/onboard-remediation.tssrc/lib/onboard-requests.tssrc/lib/onboard-resume.test.tssrc/lib/onboard-resume.tssrc/lib/onboard-shell.test.tssrc/lib/onboard-shell.tssrc/lib/onboard.tstest/gateway-liveness-probe.test.tstest/runner.test.ts
✅ Files skipped from review due to trivial changes (11)
- test/runner.test.ts
- src/lib/onboard-shell.test.ts
- src/lib/onboard-resume.test.ts
- src/lib/onboard-remediation.test.ts
- src/lib/onboard-recorders.test.ts
- src/lib/onboard-orchestrator-deps.test.ts
- CONTRIBUTING.md
- src/lib/onboard-inference-loop.test.ts
- src/lib/onboard-orchestrator-deps.ts
- src/lib/onboard-remediation.ts
- src/lib/onboard-gateway-runtime.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/gateway-liveness-probe.test.ts
- src/lib/onboard-flow-state.test.ts
- src/lib/onboard-requests.ts
- src/lib/onboard-openshell.ts
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 (1)
src/lib/dashboard.ts (1)
40-54:⚠️ Potential issue | 🔴 CriticalUpdate interfaces in
onboard-dashboard-print.tsandagent-onboard.tsto match the new function signature.The function
buildControlUiUrlsnow accepts three parameters (token,port,chatUiUrl), as evidenced by the call atsrc/lib/onboard-dashboard.ts:163. However, the interface definitions insrc/lib/onboard-dashboard-print.ts(lines 36, 39) andsrc/lib/agent-onboard.ts(line 225) still declare the old two-parameter signature:(token: string | null, port: number) => string[]. This creates a type mismatch. Update both interfaces to(token: string | null, port: number, chatUiUrl?: string | null) => string[]to reflect the new signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dashboard.ts` around lines 40 - 54, The interfaces for buildControlUiUrls used in onboard-dashboard-print.ts and agent-onboard.ts still declare the old signature (token: string | null, port: number) => string[]; update those interface/type declarations to match the new buildControlUiUrls(token: string | null, port: number, chatUiUrl?: string | null) => string[] signature so the call sites (e.g., where buildControlUiUrls is invoked in onboard-dashboard.ts) type-check correctly.
🧹 Nitpick comments (3)
src/lib/onboard-driver.test.ts (1)
108-120: Strengthen the “cloned snapshot” part of this test.Right now this proves immutability (
Object.isFrozen) but not snapshot independence after subsequent driver transitions.Suggested test tightening
it("returns an immutable cloned state snapshot", () => { const driver = InMemoryOnboardDriver.fresh({ requestedSandboxName: "alpha" }); driver.enterWorkflow().finishPreflight(); - const state = driver.state as { phase: string }; + const state = driver.state as { phase: string }; expect(Object.isFrozen(state)).toBe(true); try { state.phase = "boot"; } catch { // expected in strict mode } expect(driver.state.phase).toBe("gateway"); + + // Validate snapshot independence from future transitions. + const snapshot = driver.state; + driver.finishGateway(); + expect(snapshot.phase).toBe("gateway"); + expect(driver.state.phase).toBe("provider_selection"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-driver.test.ts` around lines 108 - 120, The test currently only asserts immutability; update it to also verify the snapshot is an independent clone by: call InMemoryOnboardDriver.fresh and run enterWorkflow().finishPreflight(), capture the returned snapshot from driver.state into a local variable (e.g., snapshot), assert snapshot is frozen and not the same object identity as driver.state ( !== ), then perform additional driver transitions (e.g., driver.enterWorkflow().someNextStep() or any method that changes driver.state), assert driver.state has changed (e.g., phase is different) while the original snapshot still has the original values, and finally attempt a mutation on snapshot to ensure it throws or does not change driver.state; reference InMemoryOnboardDriver, enterWorkflow, finishPreflight, and the state property to locate the code.src/lib/onboard-dashboard.ts (2)
56-58: Consider using optional chaining for cleaner property access.The
&&chain works but optional chaining is more idiomatic in modern TypeScript.♻️ Suggested simplification
const cfg = JSON.parse(fs.readFileSync(jsonPath, "utf-8")); - const token = cfg && cfg.gateway && cfg.gateway.auth && cfg.gateway.auth.token; + const token = cfg?.gateway?.auth?.token; return typeof token === "string" && token.length > 0 ? token : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-dashboard.ts` around lines 56 - 58, Replace the manual && property checks when extracting token with optional chaining to simplify and modernize the code: change the token assignment (where cfg, jsonPath and token are used) to use cfg?.gateway?.auth?.token and keep the existing typeof and length checks to return the string or null; update the expression that currently reads `cfg && cfg.gateway && cfg.gateway.auth && cfg.gateway.auth.token` to the optional chaining equivalent.
14-29: Consider adding symlink and depth protection for defensive coding.The recursive search operates on a temp directory populated from sandbox download, so risk is low. However, symlink loops or deeply nested structures could cause issues.
♻️ Optional: Add basic protection
-function findOpenclawJsonPath(dir: string): string | null { +function findOpenclawJsonPath(dir: string, depth = 0, maxDepth = 10): string | null { + if (depth > maxDepth) return null; const directPath = path.join(dir, ".openclaw", "openclaw.json"); if (fs.existsSync(directPath)) return directPath; if (!fs.existsSync(dir)) return null; - const entries = fs.readdirSync(dir, { withFileTypes: true }); + const entries = fs.readdirSync(dir, { withFileTypes: true }).filter((e) => !e.isSymbolicLink()); for (const entry of entries) { const filePath = path.join(dir, entry.name); if (entry.isDirectory()) { - const found = findOpenclawJsonPath(filePath); + const found = findOpenclawJsonPath(filePath, depth + 1, maxDepth); if (found) return found;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-dashboard.ts` around lines 14 - 29, The recursive finder findOpenclawJsonPath should guard against symlink loops and excessive recursion: add a maxDepth parameter (e.g. default 10) and return null when depth exceeds it, and skip following symbolic links by using fs.lstatSync to detect entry.isSymbolicLink() (or compare lstat.isDirectory vs stat) before recursing; additionally track visited real paths with fs.realpathSync in a Set to avoid revisiting the same inode/path and skip entries already seen. Update all recursive calls to pass depth+1 and consult the visited set so findOpenclawJsonPath(dir: string, visited = new Set<string>(), depth = 0, maxDepth = 10) will safely return null on symlink loops or too-deep trees.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/dashboard.ts`:
- Around line 40-54: The interfaces for buildControlUiUrls used in
onboard-dashboard-print.ts and agent-onboard.ts still declare the old signature
(token: string | null, port: number) => string[]; update those interface/type
declarations to match the new buildControlUiUrls(token: string | null, port:
number, chatUiUrl?: string | null) => string[] signature so the call sites
(e.g., where buildControlUiUrls is invoked in onboard-dashboard.ts) type-check
correctly.
---
Nitpick comments:
In `@src/lib/onboard-dashboard.ts`:
- Around line 56-58: Replace the manual && property checks when extracting token
with optional chaining to simplify and modernize the code: change the token
assignment (where cfg, jsonPath and token are used) to use
cfg?.gateway?.auth?.token and keep the existing typeof and length checks to
return the string or null; update the expression that currently reads `cfg &&
cfg.gateway && cfg.gateway.auth && cfg.gateway.auth.token` to the optional
chaining equivalent.
- Around line 14-29: The recursive finder findOpenclawJsonPath should guard
against symlink loops and excessive recursion: add a maxDepth parameter (e.g.
default 10) and return null when depth exceeds it, and skip following symbolic
links by using fs.lstatSync to detect entry.isSymbolicLink() (or compare
lstat.isDirectory vs stat) before recursing; additionally track visited real
paths with fs.realpathSync in a Set to avoid revisiting the same inode/path and
skip entries already seen. Update all recursive calls to pass depth+1 and
consult the visited set so findOpenclawJsonPath(dir: string, visited = new
Set<string>(), depth = 0, maxDepth = 10) will safely return null on symlink
loops or too-deep trees.
In `@src/lib/onboard-driver.test.ts`:
- Around line 108-120: The test currently only asserts immutability; update it
to also verify the snapshot is an independent clone by: call
InMemoryOnboardDriver.fresh and run enterWorkflow().finishPreflight(), capture
the returned snapshot from driver.state into a local variable (e.g., snapshot),
assert snapshot is frozen and not the same object identity as driver.state ( !==
), then perform additional driver transitions (e.g.,
driver.enterWorkflow().someNextStep() or any method that changes driver.state),
assert driver.state has changed (e.g., phase is different) while the original
snapshot still has the original values, and finally attempt a mutation on
snapshot to ensure it throws or does not change driver.state; reference
InMemoryOnboardDriver, enterWorkflow, finishPreflight, and the state property to
locate the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5861253b-eddc-4103-8864-f1f61014eddb
📒 Files selected for processing (10)
src/lib/dashboard.test.tssrc/lib/dashboard.tssrc/lib/onboard-dashboard.test.tssrc/lib/onboard-dashboard.tssrc/lib/onboard-driver.test.tssrc/lib/onboard-driver.tssrc/lib/onboard-flow-state.test.tssrc/lib/onboard-flow-state.tssrc/lib/onboard-inference-loop.test.tssrc/lib/onboard-inference-loop.ts
✅ Files skipped from review due to trivial changes (3)
- src/lib/onboard-inference-loop.test.ts
- src/lib/onboard-flow-state.ts
- src/lib/onboard-inference-loop.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/onboard-flow-state.test.ts
- src/lib/onboard-dashboard.test.ts
- src/lib/onboard-driver.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/lib/onboard.ts (1)
5274-5281: Derive total steps from metadata to prevent drift
TOTAL_ONBOARD_STEPS = 8duplicates step metadata and can silently desync progress output if step definitions change.♻️ Proposed refactor
-const TOTAL_ONBOARD_STEPS = 8; +const TOTAL_ONBOARD_STEPS = Math.max( + ...Object.values(ONBOARD_STEP_META).map((meta) => meta.number), +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 5274 - 5281, The hardcoded TOTAL_ONBOARD_STEPS duplicates ONBOARD_STEP_META and can drift; update skippedStepMessage to derive the total from ONBOARD_STEP_META (e.g., compute a count/length of entries or keys) instead of using TOTAL_ONBOARD_STEPS, remove or stop using the constant TOTAL_ONBOARD_STEPS where skippedStepMessage calls step(...), and ensure the derived total reflects the actual number of visible/defined steps (use the same visibility logic as isOnboardStepName/toVisibleStepName if some entries are filtered). Reference functions/values: skippedStepMessage, TOTAL_ONBOARD_STEPS, ONBOARD_STEP_META, isOnboardStepName, toVisibleStepName, and step.src/lib/onboard-sandbox-name.ts (1)
47-49: Use the validated value for reserved-name checksChecking
RESERVED_SANDBOX_NAMESagainstvalidatedSandboxNameavoids future mismatch if validation ever canonicalizes input.♻️ Proposed refactor
- if (RESERVED_SANDBOX_NAMES.has(sandboxName)) { + if (RESERVED_SANDBOX_NAMES.has(validatedSandboxName)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-sandbox-name.ts` around lines 47 - 49, Use the validated value when checking reserved names: replace the direct reference to sandboxName with validatedSandboxName in the RESERVED_SANDBOX_NAMES membership check (after calling deps.validateName in this function), and update any corresponding errorWriter messages to use validatedSandboxName; this ensures validateName (function validateName / variable validatedSandboxName) canonicalization is respected when testing against RESERVED_SANDBOX_NAMES and reporting errors.test/onboard.test.ts (3)
2193-2195: Split the mega-regex into smaller intent checksThe single multi-branch regex is brittle and harder to debug when it fails. A few focused assertions for branch entry + key calls will be more stable and clearer.
♻️ Suggested rewrite
- assert.match( - helperSource, - /if \(deps\.dangerouslySkipPermissions\) \{\s*deps\.onShowHeader\(\);\s*if \(!deps\.waitForSandboxReady\(state\.sandboxName\)\) \{[\s\S]*?\}\s*deps\.applyPermissivePolicy\(state\.sandboxName\);/, - ); + assert.match(helperSource, /if \(deps\.dangerouslySkipPermissions\)/); + assert.match(helperSource, /deps\.onShowHeader\(\)/); + assert.match(helperSource, /deps\.waitForSandboxReady\(state\.sandboxName\)/); + assert.match(helperSource, /deps\.applyPermissivePolicy\(state\.sandboxName\)/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 2193 - 2195, Replace the single mega-regex assertion against helperSource with several focused assertions: check that the branch condition referencing deps.dangerouslySkipPermissions exists, assert that deps.onShowHeader() is called when that branch is taken, assert that deps.waitForSandboxReady(state.sandboxName) is invoked and its negative-branch behavior is handled, and assert that deps.applyPermissivePolicy(state.sandboxName) is called; locate these checks around helperSource and the strings "deps.dangerouslySkipPermissions", "deps.onShowHeader(", "deps.waitForSandboxReady(state.sandboxName)", and "deps.applyPermissivePolicy(state.sandboxName)" to implement the smaller intent-focused assertions.
2145-2163: Avoid pinning the test to a literal step countLocking to
TOTAL_ONBOARD_STEPS = 8makes this test fail on unrelated future onboarding step additions. Prefer asserting presence/usage ofTOTAL_ONBOARD_STEPSinstead of a fixed literal.♻️ Suggested tweak
- assert.match(onboardSource, /const TOTAL_ONBOARD_STEPS = 8;/); + assert.match(onboardSource, /const TOTAL_ONBOARD_STEPS = \d+;/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 2145 - 2163, The test is pinned to the literal "const TOTAL_ONBOARD_STEPS = 8;" which will break when steps change; instead update the assertion to check for the presence/usage of the TOTAL_ONBOARD_STEPS symbol (e.g., match /TOTAL_ONBOARD_STEPS/ or /const\s+TOTAL_ONBOARD_STEPS/) in onboardSource rather than the exact value, keeping the other assertion for skippedStepMessage intact; locate the assertions referencing onboardSource and replace the literal-value regex with a regex that only verifies the symbol declaration or usage.
5228-5230: Function-body extraction regex is overly format-sensitiveThis pattern is fragile to harmless signature/format changes. Prefer extracting by start marker + balanced-brace scan (or AST) to reduce false failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 5228 - 5230, The current regex used to extract the function body from shellSource (the fnMatch that targets "export async function promptValidatedSandboxName") is too format-sensitive; instead locate the function by the start marker "export async function promptValidatedSandboxName(" and perform a balanced-brace scan from the first "{" to its matching "}" (or parse shellSource with a TypeScript/JS AST and find the FunctionDeclaration/FunctionExpression named promptValidatedSandboxName) to reliably extract the entire body; update the test to replace the brittle regex with this marker+balanced-brace approach (or AST lookup) so variations in whitespace/signature do not break extraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/onboard-sandbox-name.test.ts`:
- Line 6: Add a "pretest" npm script so tests always run against a fresh build:
update package.json scripts to include "pretest": "npm run build:cli" (so
running npm test will trigger the existing build:cli target); this prevents
importing stale artifacts like the distilled module used by
promptValidatedSandboxName in tests and ensures the dist/ output is rebuilt
before vitest runs locally.
---
Nitpick comments:
In `@src/lib/onboard-sandbox-name.ts`:
- Around line 47-49: Use the validated value when checking reserved names:
replace the direct reference to sandboxName with validatedSandboxName in the
RESERVED_SANDBOX_NAMES membership check (after calling deps.validateName in this
function), and update any corresponding errorWriter messages to use
validatedSandboxName; this ensures validateName (function validateName /
variable validatedSandboxName) canonicalization is respected when testing
against RESERVED_SANDBOX_NAMES and reporting errors.
In `@src/lib/onboard.ts`:
- Around line 5274-5281: The hardcoded TOTAL_ONBOARD_STEPS duplicates
ONBOARD_STEP_META and can drift; update skippedStepMessage to derive the total
from ONBOARD_STEP_META (e.g., compute a count/length of entries or keys) instead
of using TOTAL_ONBOARD_STEPS, remove or stop using the constant
TOTAL_ONBOARD_STEPS where skippedStepMessage calls step(...), and ensure the
derived total reflects the actual number of visible/defined steps (use the same
visibility logic as isOnboardStepName/toVisibleStepName if some entries are
filtered). Reference functions/values: skippedStepMessage, TOTAL_ONBOARD_STEPS,
ONBOARD_STEP_META, isOnboardStepName, toVisibleStepName, and step.
In `@test/onboard.test.ts`:
- Around line 2193-2195: Replace the single mega-regex assertion against
helperSource with several focused assertions: check that the branch condition
referencing deps.dangerouslySkipPermissions exists, assert that
deps.onShowHeader() is called when that branch is taken, assert that
deps.waitForSandboxReady(state.sandboxName) is invoked and its negative-branch
behavior is handled, and assert that
deps.applyPermissivePolicy(state.sandboxName) is called; locate these checks
around helperSource and the strings "deps.dangerouslySkipPermissions",
"deps.onShowHeader(", "deps.waitForSandboxReady(state.sandboxName)", and
"deps.applyPermissivePolicy(state.sandboxName)" to implement the smaller
intent-focused assertions.
- Around line 2145-2163: The test is pinned to the literal "const
TOTAL_ONBOARD_STEPS = 8;" which will break when steps change; instead update the
assertion to check for the presence/usage of the TOTAL_ONBOARD_STEPS symbol
(e.g., match /TOTAL_ONBOARD_STEPS/ or /const\s+TOTAL_ONBOARD_STEPS/) in
onboardSource rather than the exact value, keeping the other assertion for
skippedStepMessage intact; locate the assertions referencing onboardSource and
replace the literal-value regex with a regex that only verifies the symbol
declaration or usage.
- Around line 5228-5230: The current regex used to extract the function body
from shellSource (the fnMatch that targets "export async function
promptValidatedSandboxName") is too format-sensitive; instead locate the
function by the start marker "export async function promptValidatedSandboxName("
and perform a balanced-brace scan from the first "{" to its matching "}" (or
parse shellSource with a TypeScript/JS AST and find the
FunctionDeclaration/FunctionExpression named promptValidatedSandboxName) to
reliably extract the entire body; update the test to replace the brittle regex
with this marker+balanced-brace approach (or AST lookup) so variations in
whitespace/signature do not break extraction.
🪄 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: Pro Plus
Run ID: e81636e6-fbb3-4d20-94af-7d5344ceede1
📒 Files selected for processing (4)
src/lib/onboard-sandbox-name.test.tssrc/lib/onboard-sandbox-name.tssrc/lib/onboard.tstest/onboard.test.ts
|
|
||
| import { describe, expect, it, vi } from "vitest"; | ||
| // Import from compiled dist/ so coverage is attributed correctly. | ||
| import { promptValidatedSandboxName } from "../../dist/lib/onboard-sandbox-name"; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package scripts =="
if [[ -f package.json ]]; then
jq '.scripts' package.json
else
echo "package.json not found"
fi
echo
echo "== tests importing dist artifacts =="
rg -n --type=ts --type=js 'from\s+["'\'']\.\./\.\./dist/|from\s+["'\'']\.\./dist/|dist/lib/'
echo
echo "== build/test hook presence =="
rg -n '"(pretest|test|build|prepare)"\s*:' package.jsonRepository: NVIDIA/NemoClaw
Length of output: 17141
🏁 Script executed:
# Check for vitest config
fd -e config.{ts,js} -e vite.config.{ts,js} | head -20
# Check if there's a build step in CI
fd -e {yml,yaml} | grep -E '(github|gitlab|ci)' | head -10
# Look for any build setup in vitest config if it exists
if [ -f vitest.config.ts ] || [ -f vitest.config.js ]; then
echo "=== vitest config ==="
cat vitest.config.ts vitest.config.js 2>/dev/null | head -50
fiRepository: NVIDIA/NemoClaw
Length of output: 1201
🏁 Script executed:
# Find CI workflow files
find . -path './.git' -prune -o -type f \( -name '*.yml' -o -name '*.yaml' \) -print | grep -E '(\.github|\.gitlab|ci)' | head -20
# Check README
if [ -f README.md ]; then
echo "=== README test/build section ==="
grep -A 10 -B 2 -i '\(test\|build\)' README.md | head -40
fi
# Check CONTRIBUTING
if [ -f CONTRIBUTING.md ]; then
echo "=== CONTRIBUTING test section ==="
grep -A 10 -B 2 -i 'test\|build' CONTRIBUTING.md | head -40
fiRepository: NVIDIA/NemoClaw
Length of output: 4439
🏁 Script executed:
# Check main CI workflow which likely runs tests
echo "=== .github/workflows/main.yaml ==="
if [ -f .github/workflows/main.yaml ]; then
cat .github/workflows/main.yaml | head -80
fiRepository: NVIDIA/NemoClaw
Length of output: 881
🏁 Script executed:
# Check the basic-checks action
echo "=== .github/actions/basic-checks/action.yaml ==="
if [ -f .github/actions/basic-checks/action.yaml ]; then
cat .github/actions/basic-checks/action.yaml
fiRepository: NVIDIA/NemoClaw
Length of output: 1572
🏁 Script executed:
# Check if tests are run in the sandbox-images-and-e2e workflow
echo "=== Checking if npm test is run anywhere in CI ==="
grep -r "npm test\|npm run test\|vitest" .github/workflows/ 2>/dev/null | head -20Repository: NVIDIA/NemoClaw
Length of output: 687
🏁 Script executed:
# Check macos-e2e.yaml to see the full step sequence
echo "=== macos-e2e.yaml (steps before vitest) ==="
if [ -f .github/workflows/macos-e2e.yaml ]; then
cat .github/workflows/macos-e2e.yaml
fi
echo
echo "=== wsl-e2e.yaml (steps before vitest) ==="
if [ -f .github/workflows/wsl-e2e.yaml ]; then
cat .github/workflows/wsl-e2e.yaml
fiRepository: NVIDIA/NemoClaw
Length of output: 11161
Add pretest hook to ensure dist is rebuilt before tests run locally
Importing from ../../dist/ can validate stale output if developers run npm test without rebuilding. While CI pipelines explicitly rebuild before testing (npm run build:cli before npx vitest run), there is no pretest hook in package.json to enforce this locally. Add "pretest": "npm run build:cli" to ensure fresh builds during local development.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/onboard-sandbox-name.test.ts` at line 6, Add a "pretest" npm script
so tests always run against a fresh build: update package.json scripts to
include "pretest": "npm run build:cli" (so running npm test will trigger the
existing build:cli target); this prevents importing stale artifacts like the
distilled module used by promptValidatedSandboxName in tests and ensures the
dist/ output is rebuilt before vitest runs locally.
Signed-off-by: Carlos Villela <[email protected]>
Signed-off-by: Carlos Villela <[email protected]>
wscurran
added
NemoClaw CLI
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
2 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
This PR substantially advances the onboarding FSM migration by turning
src/lib/onboard.tsfrom a large inline wizard into a typed, testable composition root built on explicit state, drivers, orchestrators, and focused helper modules. It preservesonboard.tsas the CLI command entrypoint and compatibility surface, but moves the bulk of host preparation, inference selection, sandbox creation, runtime setup, policy application, dashboard access, and resume logic into dedicated modules so TypeScript and targeted tests can validate onboarding behavior in smaller units.Changes
src/lib/onboard-fsm.tswith canonical visible steps, aliases, transitions, and context-aware flow state definitions, plussrc/lib/onboard-fsm.test.tscoverage for happy-path, failure, and reset transitions.src/lib/onboard-session.tsto a typed v2 schema with canonicalmessaging/runtime_setupsteps,messagingChannelspersistence, safe reducers, and legacy migration support, and expandsrc/lib/onboard-session.test.tsaccordingly.src/lib/onboard-flow-state.tsand tests to derive canonical resume phases from persisted checkpoints and expose reusable progress helpers.src/lib/onboard-driver.ts+ tests for in-memory fresh/resume behaviorsrc/lib/onboard-persistent-driver.ts+ tests for persisted progresssrc/lib/onboard-recorders.ts+ tests for tracked run/session recordingsrc/lib/onboard-bootstrap.ts+ tests for fresh/resume initializationsrc/lib/onboard-run-context.ts+ tests for tracked context mutationsrc/lib/onboard-shell.ts+ tests for shell-state parsing, banner text, warnings, and lock formattingsrc/lib/onboard-entry.ts+ tests for the CLI entry shell (notice, lock, initialization, orchestrator handoff)src/lib/onboard-resume.ts+ tests for pure resume-conflict helperssrc/lib/onboard-orchestrator.ts+ tests for the typed onboarding coordinatorsrc/lib/onboard-orchestrator-deps.ts+ tests for translating legacy onboard helpers into orchestrator dependenciessrc/lib/onboard-host-flow.ts+ tests for preflight/gateway orchestration and stale-gateway handlingsrc/lib/onboard-inference-loop.ts+ tests for provider-selection / inference setup loopssrc/lib/onboard-sandbox-flow.ts+ tests for messaging + sandbox provisioning / resume behaviorsrc/lib/onboard-runtime-flow.ts+ tests for OpenClaw-vs-agent runtime setup handlingsrc/lib/onboard-policy-flow.ts+ tests for policy application, permissive mode, and resume semanticsonboard.ts:src/lib/onboard-openshell.ts+ tests for OpenShell installation and sandbox readiness pollingsrc/lib/onboard-gateway-runtime.ts+ tests for gateway startup/recovery helpers and diagnosticssrc/lib/onboard-gateway-start-stream.tsfor streamed gateway-start progress reporting and timeout handlingsrc/lib/onboard-gateway-liveness.tsfor gateway container liveness probing before reusing cached metadatasrc/lib/onboard-remediation.ts+ tests for runtime detection, remediation text, PATH hints, and port-conflict service hintssrc/lib/onboard-openshell-version.tsfor OpenShell version parsing, semver comparison, and blueprint min/max version readssrc/lib/onboard-runtime-helpers.tsfor shared sleep / runtime / sandbox-reuse / gateway-destroy / inference-route helperssrc/lib/onboard-nim-setup.tsfor the large provider-selection / local-model-selection flowsrc/lib/onboard-inference-provider.tsfor applying configured inference routessrc/lib/onboard-inference-validation.tsfor retry UX, probe auth mode, responses/tool-call validation, and custom endpoint checkssrc/lib/onboard-remote-provider-config.tsfor provider endpoint/config constants used by the onboarding selectorsrc/lib/onboard-ollama-proxy.tsfor auth-proxy lifecycle and recoverysrc/lib/onboard-ollama-models.tsfor Ollama model prompting, pulling, warmup, and warningssrc/lib/onboard-sandbox-name.ts+ tests for sandbox-name prompting / validationsrc/lib/onboard-sandbox-create.tsfor the large sandbox creation / reuse / rebuild / provider attachment flowsrc/lib/onboard-sandbox-build-config.tsfor sandbox inference config derivation, Dockerfile patching, and base-image digest pinningsrc/lib/onboard-openclaw-setup.tsfor sandbox config sync script generation, temp file staging, readiness checks, and OpenClaw runtime initializationsrc/lib/onboard-web-search-config.tsfor Brave Search credential validation and interactive/non-interactive web-search setupsrc/lib/onboard-provider-management.tsfor provider create/update argument building, provider upserts, credential hashing, rotation detection, and messaging-conflict probessrc/lib/onboard-messaging.ts+ tests for messaging channel prompts and token/guild/allowlist capturesrc/lib/onboard-telegram.ts+ tests for Telegram reachability checks and curl-code handlingsrc/lib/onboard-policy-suggestions.ts+ tests for suggested policy preset selection, including explicit enabled-channel forwarding into tier-based suggestionssrc/lib/onboard-policy-ui.tsfor tier selection, preset access-mode UI, legacy policy setup, and resume-aware preset applicationsrc/lib/onboard-dashboard.ts+ tests for dashboard token/forward/access helperssrc/lib/onboard-dashboard-print.ts+ tests for dashboard summary renderingsrc/lib/onboard-step-api.tsfor host/gateway/inference/messaging/OpenClaw wrapper compositionsrc/lib/onboard-ui-api.tsfor dashboard and policy-UI wrapper compositionsrc/lib/ansi-utils.tsand use it from extracted onboarding/gateway helpers where ANSI stripping is needed.src/lib/onboard.tsso it now primarily contains:onboard()entrypoint that callsrunOnboardingEntry(...)onboard.ts, including updates to:test/gateway-liveness-probe.test.tstest/gateway-cleanup.test.tstest/runner.test.tstest/shellquote-sandbox.test.tstest/credential-exposure.test.tstest/gemini-probe-auth.test.tstest/wsl2-probe-timeout.test.tstest/onboard.test.tssrc/lib/onboard-*.test.tsso behavior formerly exercised only via the monolithic wizard now has direct unit/integration coverage at the extracted-module level.dist/artifacts where appropriate.test/e2e/test-onboard-resume.shand related onboarding test harness expectations.maininto the branch and resolve conflicts insrc/lib/onboard-session.ts,src/lib/onboard.ts, andtest/onboard.test.ts, preserving the extracted onboarding architecture while incorporating newer mainline changes around:#2069)mainhadolintusage viascripts/run-hadolint.shand.pre-commit-config.yaml, and document the local/Docker hook expectations inCONTRIBUTING.md.#2020)#1881)#1904)#1925)#1932,#1960,#987)#429,#1155)Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Carlos Villela [email protected]