refactor(onboard): extract config sync helpers#3299
Conversation
|
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)
📝 WalkthroughWalkthroughThis PR extracts selection-drift detection and config-sync script generation from ChangesOnboarding Helper Modularization
Sequence Diagram(s)sequenceDiagram
participant Caller
participant getSelectionDrift
participant readSandboxSelectionConfig
participant OpenShell
participant findSelectionConfigPath
Caller->>getSelectionDrift: sandboxName, requestedProvider, requestedModel, {runOpenshell}
getSelectionDrift->>readSandboxSelectionConfig: sandboxName, deps
readSandboxSelectionConfig->>OpenShell: download sandbox config
alt download succeeds
OpenShell-->>readSandboxSelectionConfig: temp directory
readSandboxSelectionConfig->>findSelectionConfigPath: locate config.json
findSelectionConfigPath-->>readSandboxSelectionConfig: config path or null
alt config found and valid
readSandboxSelectionConfig-->>getSelectionDrift: ProviderSelectionConfig
getSelectionDrift->>getSelectionDrift: compare provider/model
getSelectionDrift-->>Caller: SelectionDrift with changed flags
else config invalid
readSandboxSelectionConfig-->>getSelectionDrift: null
getSelectionDrift-->>Caller: SelectionDrift with unknown=true
end
else download fails
OpenShell-->>readSandboxSelectionConfig: null
readSandboxSelectionConfig-->>getSelectionDrift: null
getSelectionDrift-->>Caller: SelectionDrift with unknown=true
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
…ture-layout # Conflicts: # src/lib/actions/sandbox/destroy.ts # src/lib/inference/health.ts
…d-selection-drift
…d-selection-drift
…ture-layout # Conflicts: # src/lib/list-command-deps.ts
…d-selection-drift
…/NVIDIA/NemoClaw into refactor/cli-architecture-layout
…d-selection-drift
…d-selection-drift
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryPi Semantic E2E AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
5959-5959: Consider a real onboarding E2E for the newgetSelectionDrift()wiring.This branch controls sandbox reuse vs. recreation, so an end-to-end pass like
cloud-e2eorsandbox-operations-e2ewould add confidence that the extracted helper still preserves the old decision path under a live gateway/sandbox.As per coding guidelines,
src/lib/onboard.ts: "This file contains core onboarding logic. Changes here affect the full sandbox creation and configuration flow."🤖 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` at line 5959, Add a focused end-to-end test that validates the new getSelectionDrift wiring preserves the original sandbox reuse vs recreation decision path under a live gateway; specifically exercise the onboarding flow that calls getSelectionDrift(sandboxName, provider, model, { runOpenshell }) and assert the sandbox is reused or recreated as expected. Create or extend an existing E2E suite (e.g., cloud-e2e or sandbox-operations-e2e) to run the full onboarding flow against a real gateway/sandbox, mock only external dependencies not part of the gateway, and cover both branches (reuse and recreate) to ensure behavior parity with the previous implementation. Ensure the test reproduces conditions that lead to each branch by controlling inputs (sandboxName, provider, model, runOpenshell) and verifies side effects (sandbox ID, creation timestamp, or gateway responses) rather than internal implementation details. Run the new test in CI alongside other onboarding E2Es to catch regressions in src/lib/onboard.ts affecting sandbox lifecycle decisions.
🤖 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/selection-drift.test.ts`:
- Line 51: The test's assertion hardcodes a forward slash and will fail on
Windows; update the expectation in selection-drift.test.ts (the
expect.stringMatching(...) for the path returned from the function that uses
`${tmpDir}${path.sep}`) to be platform-agnostic by either using a regex that
accepts both separators (e.g. match [\/\\] at the end) or, better, compare the
basename (using path.basename) instead of the full path so the test does not
depend on path.sep.
In `@src/lib/onboard/selection-drift.ts`:
- Around line 51-52: Move the fs.mkdtempSync call into the try block so creation
failures are caught and handled by getSelectionDrift's existing error path;
declare tmpDir (e.g., let tmpDir: string | undefined) before the try, assign it
inside the try via fs.mkdtempSync(...), and in the finally block check tmpDir is
defined before attempting cleanup to avoid referencing an uninitialized
variable.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Line 5959: Add a focused end-to-end test that validates the new
getSelectionDrift wiring preserves the original sandbox reuse vs recreation
decision path under a live gateway; specifically exercise the onboarding flow
that calls getSelectionDrift(sandboxName, provider, model, { runOpenshell }) and
assert the sandbox is reused or recreated as expected. Create or extend an
existing E2E suite (e.g., cloud-e2e or sandbox-operations-e2e) to run the full
onboarding flow against a real gateway/sandbox, mock only external dependencies
not part of the gateway, and cover both branches (reuse and recreate) to ensure
behavior parity with the previous implementation. Ensure the test reproduces
conditions that lead to each branch by controlling inputs (sandboxName,
provider, model, runOpenshell) and verifies side effects (sandbox ID, creation
timestamp, or gateway responses) rather than internal implementation details.
Run the new test in CI alongside other onboarding E2Es to catch regressions in
src/lib/onboard.ts affecting sandbox lifecycle decisions.
🪄 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: 7d8e1238-d387-44ff-82f9-8419ab6b2c63
📒 Files selected for processing (8)
src/lib/onboard.tssrc/lib/onboard/compatible-endpoint-smoke.test.tssrc/lib/onboard/compatible-endpoint-smoke.tssrc/lib/onboard/config-sync.test.tssrc/lib/onboard/config-sync.tssrc/lib/onboard/selection-drift.test.tssrc/lib/onboard/selection-drift.tstest/onboard.test.ts
|
Addressed CodeRabbit's actionable inline comments in e776625:
Validation run locally:
I did not add a new live gateway E2E for the nitpick in this refactor commit; the PR already has focused compiled-onboard coverage for reuse/recreate behavior, and the E2E advisor has identified |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
5967-5967: Optional pre-merge safeguard: run the recommended onboarding E2Es.Given this touches core onboarding flow, the targeted nightly E2E set is a good final confidence check.
As per coding guidelines,
src/lib/onboard.tsis core onboarding logic and includes an explicit E2E test recommendation list.🤖 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` at line 5967, This change touches core onboarding logic (the getSelectionDrift call using sandboxName, provider, model), so add a short pre-merge safeguard: insert a clear comment or PR checklist entry adjacent to the getSelectionDrift invocation reminding reviewers to run the recommended onboarding nightly E2E suite (or link the specific E2E list), and/or update CI config to include that targeted E2E job for changes under the onboarding module; ensure the comment references getSelectionDrift, sandboxName, provider, and model so reviewers know exactly where the E2E verification is required.
🤖 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.ts`:
- Line 5967: This change touches core onboarding logic (the getSelectionDrift
call using sandboxName, provider, model), so add a short pre-merge safeguard:
insert a clear comment or PR checklist entry adjacent to the getSelectionDrift
invocation reminding reviewers to run the recommended onboarding nightly E2E
suite (or link the specific E2E list), and/or update CI config to include that
targeted E2E job for changes under the onboarding module; ensure the comment
references getSelectionDrift, sandboxName, provider, and model so reviewers know
exactly where the E2E verification is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 85225b66-f66d-48ca-8fa4-83bb32b5a1e0
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/onboard/selection-drift.test.tssrc/lib/onboard/selection-drift.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/selection-drift.test.ts
- src/lib/onboard/selection-drift.ts
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Reviewed the current head e6d0c46. The helper extraction preserves the existing onboarding behavior while improving testability by injecting runOpenshell into selection-drift reads; the config sync script is unchanged in behavior and still avoids rewriting OpenClaw config. CodeRabbit's earlier findings are resolved on this head.\n\nLocal validation passed: npm run build:cli, npm run typecheck:cli, npm run format:check, npm run checks, npx vitest run src/lib/onboard/config-sync.test.ts src/lib/onboard/selection-drift.test.ts --testTimeout 60000, and npx vitest run test/onboard.test.ts --testTimeout 60000. Required GitHub checks are green.
4 participants
Summary
Extract sandbox config sync script helpers out of the large onboarding module. This continues the onboarding cleanup stack by isolating selection-config script generation and temp-file writing behind a focused module with direct tests.
Changes
src/lib/onboard/config-sync.tsfor sandbox config sync script construction and sync script temp-file creation.src/lib/onboard.tsto import the config sync helpers while preserving existing exports and call sites.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
Refactor
New Features
Tests