fix(cli): address onboard FSM follow-up hardening#4133
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds a post-verification recorder and defers session completion, centralizes safe Docker-driver gateway log/spawn, tightens OpenShell version handling, normalizes known-hosts entries, treats removed messaging tokens as rotation, prefers stored Brave API keys in non-interactive web-search, and centralizes model-router process ownership checks. ChangesOnboarding System Enhancements
Sequence DiagramsequenceDiagram
participant Finalizer as handleFinalizationState
participant Boundary as OnboardRuntimeBoundary
participant Runtime as RuntimeState
Finalizer->>Boundary: recordPostVerifyStarted()
Boundary->>Runtime: read session + state
alt state == "finalizing"
Runtime->>Runtime: transition "post_verify"
end
Boundary-->>Finalizer: return session
Finalizer->>Finalizer: build verify chain
Finalizer->>Finalizer: execute verification
Finalizer->>Finalizer: log diagnostics + print dashboard
Finalizer->>Boundary: recordSessionComplete()
Boundary->>Runtime: transition completion
Boundary-->>Finalizer: return session
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
Follow-up to feedback from #3883 and #3919. Local verification performed:
I intentionally left the full |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 0 needs attention, 1 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/onboard.ts (1)
2672-2697:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClose parent copy of
logFdafterspawn()(fd leak)
src/lib/onboard.tsopenslogFdfor the gateway log and passes it tospawn(..., stdio: ["ignore", logFd, logFd]), but the parent never closeslogFdanywhere in the file (nocloseSync/fs.closeSyncfound), leaking an fd on each restart attempt (and ifspawnthrows synchronously).Suggested fix
let logFd: number; try { logFd = fs.openSync(logPath, appendNoFollow, 0o600); } catch (error) { console.error(` Failed to open OpenShell Docker-driver gateway log '${logPath}': ${String(error)}`); if (exitOnFailure) process.exit(1); throw error; } console.log(" Starting OpenShell Docker-driver gateway..."); console.log(` Gateway log: ${logPath}`); const launch = gatewayLaunch ?? { command: gatewayBin, args: [], env: { ...process.env, ...gatewayEnv }, mode: "host" as const, processGatewayBin: gatewayBin, }; dockerDriverGatewayLaunch.prepareAndLogDockerDriverGatewayLaunch(launch); - const child = spawn(launch.command, launch.args, { - detached: true, - stdio: ["ignore", logFd, logFd], - env: launch.env, - }); + let child; + try { + child = spawn(launch.command, launch.args, { + detached: true, + stdio: ["ignore", logFd, logFd], + env: launch.env, + }); + } finally { + fs.closeSync(logFd); + }🤖 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 2672 - 2697, The parent process currently opens logFd and passes it to spawn(...) but never closes its copy, leaking a file descriptor; wrap the spawn call in a try/catch and after a successful spawn call immediately call fs.closeSync(logFd) to close the parent's copy (but only after spawn returns), and if spawn throws synchronously, ensure you closeSync(logFd) in the catch before rethrowing or exiting; reference logFd and the spawn(...) call in onboard.ts when making these changes.
🤖 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.ts`:
- Around line 2672-2697: The parent process currently opens logFd and passes it
to spawn(...) but never closes its copy, leaking a file descriptor; wrap the
spawn call in a try/catch and after a successful spawn call immediately call
fs.closeSync(logFd) to close the parent's copy (but only after spawn returns),
and if spawn throws synchronously, ensure you closeSync(logFd) in the catch
before rethrowing or exiting; reference logFd and the spawn(...) call in
onboard.ts when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8cc00749-6055-4f3a-9214-41555d94193d
📒 Files selected for processing (14)
src/lib/onboard.tssrc/lib/onboard/known-hosts.tssrc/lib/onboard/machine/handlers/finalization.test.tssrc/lib/onboard/machine/handlers/finalization.tssrc/lib/onboard/messaging-credentials.tssrc/lib/onboard/model-router.test.tssrc/lib/onboard/model-router.tssrc/lib/onboard/openshell-version.tssrc/lib/onboard/runtime-boundary.tssrc/lib/onboard/web-search-flow.tstest/credential-rotation.test.tstest/onboard-brave-validation.test.tstest/onboard-openshell-version.test.tstest/ssh-known-hosts.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26338293715
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/model-router.ts (1)
423-468: Run the onboarding/sandbox E2E set for this reconciliation-path hardening.The new PID-ownership gate changes restart vs. abort behavior on healthy-port detection, so this is worth validating via the recommended nightly E2E job subset before merge.
As per coding guidelines: "E2E test recommendation: cloud-e2e ... sandbox-operations-e2e ... rebuild-openclaw-e2e ... channels-stop-start-e2e ... messaging-compatible-endpoint-e2e ... bedrock-runtime-compatible-anthropic-e2e ... hermes-discord-e2e ... hermes-slack-e2e ... openshell-gateway-upgrade-e2e".
🤖 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/model-router.ts` around lines 423 - 468, The reconcileModelRouter change adds a PID-ownership gate that alters restart vs abort behavior when a healthy router is found (see reconcileModelRouter, isRouterHealthy, doesModelRouterProcessOwnPort, startModelRouter, stopModelRouterProcess); run the recommended E2E subset before merging to validate both branches (recorded PID owns port -> restart, recorded PID absent/different -> abort) by executing the nightly cloud-e2e list: sandbox-operations-e2e, rebuild-openclaw-e2e, channels-stop-start-e2e, messaging-compatible-endpoint-e2e, bedrock-runtime-compatible-anthropic-e2e, hermes-discord-e2e, hermes-slack-e2e and openshell-gateway-upgrade-e2e, and verify session updates (routerPid, routerCredentialHash) and correct process stop/start behavior in each scenario.
🤖 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/model-router.ts`:
- Around line 423-468: The reconcileModelRouter change adds a PID-ownership gate
that alters restart vs abort behavior when a healthy router is found (see
reconcileModelRouter, isRouterHealthy, doesModelRouterProcessOwnPort,
startModelRouter, stopModelRouterProcess); run the recommended E2E subset before
merging to validate both branches (recorded PID owns port -> restart, recorded
PID absent/different -> abort) by executing the nightly cloud-e2e list:
sandbox-operations-e2e, rebuild-openclaw-e2e, channels-stop-start-e2e,
messaging-compatible-endpoint-e2e, bedrock-runtime-compatible-anthropic-e2e,
hermes-discord-e2e, hermes-slack-e2e and openshell-gateway-upgrade-e2e, and
verify session updates (routerPid, routerCredentialHash) and correct process
stop/start behavior in each scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bd38f7b8-2944-4d05-8afc-32af4ce11b9e
📒 Files selected for processing (6)
src/lib/onboard.tssrc/lib/onboard/docker-driver-gateway-launch.tssrc/lib/onboard/model-router-process.tssrc/lib/onboard/model-router.test.tssrc/lib/onboard/model-router.tssrc/lib/onboard/openshell-version.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/onboard/model-router.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26339241455
|
…ort, function or class' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26339435452
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26339533418
|
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
Address follow-up findings from the final onboarding FSM and onboard-shell extraction reviews. This PR keeps final session completion behind successful verification, hardens extracted onboarding helpers, and prevents stale Model Router PID reuse from killing unrelated processes.
Related Issue
Refs #3883
Refs #3919
Changes
post_verifybefore deployment verification and recordscompleteonly after verification and dashboard output succeed.model-router proxy --port <port>before restart/kill.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Bug Fixes
Improvements
Tests