refactor(onboard): extract web search support helper - #3302
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtracts web-search detection into ChangesWeb Search Support Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
|
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. |
This reverts commit d113704.
…ture-layout # Conflicts: # src/lib/actions/sandbox/destroy.ts # src/lib/inference/health.ts
…d-selection-drift
…d-web-search-support
…d-selection-drift
…d-web-search-support
…ture-layout # Conflicts: # src/lib/list-command-deps.ts
cjagwani
left a comment
There was a problem hiding this comment.
lgtm — clean 1:1 extraction, behavior preserved at both call sites in onboard.ts (both pass ROOT explicitly).
one nit not worth blocking on: rootDir = process.cwd() default is a small semantic drift from the old code which closed over ROOT (resolved via __dirname). prod is fine since callers pass ROOT, but test/onboard.test.ts:2150-2152 now relies on cwd === repo root. consider importing ROOT from state/paths.ts as the default, or dropping the default entirely so callers must be explicit. either way, follow-up not blocker.
new unit tests cover hermes / override / fallback / no-arg — meaningful, and the existing integration tests in test/onboard.test.ts still exercise the re-exported symbol via onboard.ts. approving.
## Summary Extract the Brave web-search configuration flow out of the large onboarding module. This continues the onboarding cleanup stack by moving validation, prompting, credential persistence, and configuration selection into a focused helper module with direct coverage. ## Changes - Add `src/lib/onboard/web-search-config.ts` for Brave Search validation, recovery prompting, credential setup, and web-search config selection. - Update `src/lib/onboard.ts` to use the extracted helper factory while preserving existing exports and call sites. - Add unit tests covering Brave validation curl arguments, unsupported-agent skip behavior, existing config reuse, non-interactive env setup, interactive credential prompting, and skip recovery. ## 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) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make 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: Carlos Villela <cvillela@nvidia.com>
E2E Advisor RecommendationRequired E2E: None Dispatch hint: Full advisor summaryPi Semantic E2E AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/web-search-support.test.ts`:
- Line 58: Replace the hardcoded absolute path "/missing" in the test call to
agentSupportsWebSearch with a repo-local guaranteed-nonexistent path derived
from the existing root test fixture (e.g., use path.join(root, "nonexistent") or
path.resolve(root, "does-not-exist")) so the test uses a repo-local missing path
that is platform-independent; update the expect invocation that currently calls
agentSupportsWebSearch({ name: "openclaw", dockerfilePath: "/missing" }, null,
root) to use the constructed path instead.
🪄 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: e2fe2a1b-6547-4056-9ea1-2cafb0cf3ce3
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/onboard/web-search-support.test.tssrc/lib/onboard/web-search-support.ts
Summary
Extract the web-search support detection helper out of the large onboarding module. This keeps the onboarding cleanup stack moving with a small behavior-preserving split and direct coverage for Dockerfile support detection.
Changes
src/lib/onboard/web-search-support.tsfor determining whether an agent image supports theNEMOCLAW_WEB_SEARCH_ENABLEDDockerfile ARG.src/lib/onboard.tsto import the support helper while preserving existingagentSupportsWebSearchexports 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
Tests