refactor(onboard): extract web search verification probe#3305
Conversation
This reverts commit d113704.
|
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:
📝 WalkthroughWalkthroughExtracts web-search verification into a dependency-injected helper with agent-specific probes (Hermes/OpenClaw), adds unit tests covering success and failure cases, and refactors onboard.ts to delegate verification to the new helper; tests updated to include the new module in probe-command assertions. ChangesWeb Search Verification Modularization
Sequence Diagram(s)sequenceDiagram
participant Onboard
participant Verify as verifyWebSearchInsideSandbox
participant Runner as runCaptureOpenshell
participant FS as filesystem
participant Logger
Onboard->>Verify: verifyWebSearchInsideSandbox(sandboxName, agent, deps)
Verify->>Runner: run "hermes dump" (when agent=hermes)
Verify->>FS: read /sandbox/.openclaw/openclaw.json (when agent=openclaw)
Runner-->>Verify: command output or error
FS-->>Verify: file contents or read/parse error
Verify->>Logger: log success or warn on failure
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 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-web-search-support
…ard-web-search-config
…rd-web-search-verify
…d-selection-drift
cjagwani
left a comment
There was a problem hiding this comment.
lgtm. clean 1:1 extraction — diffed the new module against the deleted block and it's identical modulo console.* → log/warn indirection (same regexes, same argv arrays, same 10s timeout, same json path).
single caller at onboard.ts:6101 still matches the 2-arg wrapper, and the wrapper forwards runCaptureOpenshell + cliName so prod logging behavior is preserved by the console.* defaults.
tests are a real upgrade — the original probe had zero direct coverage and we now hit all 5 branches (active hermes, missing backend, active openclaw, malformed+disabled openclaw, unknown agent + throwing probe). source-shape regression keeps the "sandbox","exec","-n",...,"hermes"|"cat" invariant by concatenating both files, nice touch.
no SSRF surface here — argv is hardcoded, sandboxName flows positionally, catch blocks emit static strings so no creds leak via error.message.
coderabbit skipped on draft, no human comments to reconcile. ready to mark ready-for-review whenever #3304 lands.
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-config.ts`:
- Around line 167-171: The non-interactive branch currently only reads
env[BRAVE_API_KEY_ENV] via deps.normalizeCredentialValue, causing web search to
be disabled even if a saved credential exists; modify the branch in the
isNonInteractive() block to first attempt to retrieve a stored credential using
deps.getCredential(BRAVE_API_KEY_ENV) (falling back to env[BRAVE_API_KEY_ENV] if
getCredential returns nothing), then normalize that value with
deps.normalizeCredentialValue and proceed as before—this mirrors the behavior of
ensureValidatedBraveSearchCredential(...) so saved keys are honored in
non-interactive mode.
🪄 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: b9839ac6-2e30-4021-b29c-f09d32dab7c9
📒 Files selected for processing (6)
src/lib/onboard.tssrc/lib/onboard/web-search-config.test.tssrc/lib/onboard/web-search-config.tssrc/lib/onboard/web-search-verify.test.tssrc/lib/onboard/web-search-verify.tstest/onboard.test.ts
| if (deps.isNonInteractive()) { | ||
| const braveApiKey = deps.normalizeCredentialValue(env[BRAVE_API_KEY_ENV]); | ||
| if (!braveApiKey) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Use saved credential fallback in non-interactive mode before disabling web search.
This branch only reads env[BRAVE_API_KEY_ENV]. If a key was previously stored via saveCredential, non-interactive onboarding still returns null and disables web search unexpectedly. Reuse getCredential(...) here, consistent with ensureValidatedBraveSearchCredential(...).
Suggested patch
- const braveApiKey = deps.normalizeCredentialValue(env[BRAVE_API_KEY_ENV]);
+ const braveApiKey = deps.normalizeCredentialValue(
+ deps.getCredential(BRAVE_API_KEY_ENV) ?? env[BRAVE_API_KEY_ENV],
+ );
if (!braveApiKey) {
return null;
}🤖 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/web-search-config.ts` around lines 167 - 171, The
non-interactive branch currently only reads env[BRAVE_API_KEY_ENV] via
deps.normalizeCredentialValue, causing web search to be disabled even if a saved
credential exists; modify the branch in the isNonInteractive() block to first
attempt to retrieve a stored credential using
deps.getCredential(BRAVE_API_KEY_ENV) (falling back to env[BRAVE_API_KEY_ENV] if
getCredential returns nothing), then normalize that value with
deps.normalizeCredentialValue and proceed as before—this mirrors the behavior of
ensureValidatedBraveSearchCredential(...) so saved keys are honored in
non-interactive mode.
cjagwani
left a comment
There was a problem hiding this comment.
re-reviewed at head 1404c87 after my earlier approval at 14494ce.
the 47 new commits break down as: 45 already-merged-on-main PRs (zero review burden) plus the merge of the parent-stack #3304 branch into this one. against main the actual PR diff is 6 files / +496/-230 — verify.ts and verify.test.ts are byte-identical to what i approved, and the config.ts/config.test.ts content is the same #3304 work that github says "merged" but whose merge commit (8bb073b) is not actually on main HEAD (17325de). so practically, #3305 is the only path to ship #3304 to main right now.
coderabbit flagged a missing getCredential() fallback in the non-interactive branch at config.ts:167 — i diffed it against main onboard.ts:2505 and that branch already only reads process.env on main. not a regression, pre-existing behavior preserved by the extraction. fine to handle in a follow-up if we want it.
ci is clean (19 success, 2 in-progress on wsl-e2e + advisor). re-approving.
…ort, function or class' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
cjagwani
left a comment
There was a problem hiding this comment.
re-reviewed the two new commits on top of 1404c87:
5aabaffextracts a sibling gateway module and removes the old web-search-config; touches onboard.ts plumbing but leavesweb-search-verify.{ts,test.ts}byte-identical to what i approved, and the wrapper + guarded call site are unchanged.2a30b20is a clean main rebase pulling in destroy/shields/timer work; zero web-search-verify churn.
probe semantics, DI shape, and call site all preserved. re-approving at 9273b30.
E2E Advisor RecommendationRequired E2E: None Dispatch hint: Full advisor summaryPi Semantic E2E AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Summary
Extract the post-create web-search verification probe out of the large onboarding module. This keeps web-search setup and verification in focused helper modules while leaving onboarding flow orchestration unchanged.
Changes
src/lib/onboard/web-search-verify.tsfor best-effort Hermes/OpenClaw web-search runtime verification.src/lib/onboard.tsto delegate web-search verification through the extracted helper.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
New Features
Tests