feat(doctor): add WebSearch backend diagnostics - #1884
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (5)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**⚙️ CodeRabbit configuration file
Files:
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughAdds web-search diagnostics to ChangesWebSearch Diagnostics
WebSearch Test Coverage
Firecrawl URL Handling
Brave timeout test
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 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 `@scripts/system-check.ts`:
- Around line 240-247: The native web search check in buildNativeWebSearchCheck
is only treating openai as supported, so codex resolves fall through to the
unsupported-provider failure. Update the provider guard to include codex
alongside openai, and make the same change in the other Codex responses native
check path referenced by this review so both pass branches recognize Codex
models returned by getAPIProvider(). Preserve the existing
isCodexResponsesWebSearchEnabledForDoctor gating and the current pass message
behavior.
- Around line 329-331: The host check in isFirecrawlCloudApiUrl is too broad
because it uses includes and can match non-cloud/proxy URLs accidentally. Update
the logic to parse the apiUrl hostname and compare it exactly against the
Firecrawl cloud host (with the default fallback still treated as cloud), so only
the intended cloud endpoint is classified as such.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 3d9f1209-4a82-48cf-a1a6-2d1a1d5cf1cf
📒 Files selected for processing (2)
scripts/system-check.test.tsscripts/system-check.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports in source and test files.
Files:
scripts/system-check.test.tsscripts/system-check.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
In the files you touch, preserve the existing code style.
Files:
scripts/system-check.test.tsscripts/system-check.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
scripts/system-check.test.tsscripts/system-check.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
scripts/system-check.test.tsscripts/system-check.ts
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
scripts/system-check.test.tsscripts/system-check.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
scripts/system-check.test.ts
🪛 ast-grep (0.44.1)
scripts/system-check.test.ts
[error] 117-120: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. if (key === "__proto__" || key === "constructor" || key === "prototype") continue;), use a null-prototype object (Object.create(null)), or use a safe merge utility instead.
Context: for (const key of ENV_KEYS) {
originalEnv[key] = process.env[key]
delete process.env[key]
}
Note: [CWE-1321] Improperly Controlled Modification of Object Prototype Attributes ('Prototype Pollution').
(prototype-pollution-recursive-merge-typescript)
🔇 Additional comments (2)
scripts/system-check.test.ts (1)
2-9: LGTM!Also applies to: 18-124, 135-139, 349-613
scripts/system-check.ts (1)
39-238: LGTM!Also applies to: 250-275, 287-328, 334-537, 1384-1384
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
- [P2] Keep the Firecrawl cloud URL check aligned with runtime
scripts/system-check.ts:329
The new diagnostic now treats only the exactapi.firecrawl.devhostname as Firecrawl cloud, and the added test saysFIRECRAWL_API_URL=https://proxy.example.com/api.firecrawl.devis configured without an API key. However the runtime Firecrawl client still checksapiUrl.includes('api.firecrawl.dev'), so that same URL still throwsFirecrawl API key is required for the cloud APIbefore it can search. Please complete CodeRabbit's Firecrawl host-matching request in the runtime Firecrawl config path as well, or keep the doctor diagnostic using the same classification as runtime, sodoctor:runtimedoes not report a backend as healthy when WebSearch will reject it.
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/tools/firecrawl/client.ts`:
- Around line 45-56: The fallback path in isFirecrawlCloudApiUrl is too narrow
and case-sensitive for schemeless inputs, so normalize the hostname comparison
in the catch branch. Update the logic in isFirecrawlCloudApiUrl to treat the
bare host equivalently regardless of case (and after trimming trailing slashes),
and keep the DEFAULT_FIRECRAWL_API_URL check only if you still need it for
non-URL inputs. Ensure the URL parsing path and fallback branch both classify
api.firecrawl.dev consistently.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f2758763-ed3f-4869-9727-f838d20cf17d
📒 Files selected for processing (3)
scripts/system-check.tssrc/tools/firecrawl/client.test.tssrc/tools/firecrawl/client.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with strict mode and ESM imports in source and test files.
Files:
src/tools/firecrawl/client.test.tssrc/tools/firecrawl/client.tsscripts/system-check.ts
src/{commands,components,services,tools,utils,integrations,entrypoints,tasks}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer the existing service, provider, settings, permission, and UI patterns over introducing new abstractions.
Files:
src/tools/firecrawl/client.test.tssrc/tools/firecrawl/client.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
In the files you touch, preserve the existing code style.
Files:
src/tools/firecrawl/client.test.tssrc/tools/firecrawl/client.tsscripts/system-check.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/tools/firecrawl/client.test.tssrc/tools/firecrawl/client.tsscripts/system-check.ts
**
⚙️ CodeRabbit configuration file
**: # AGENTS.md - AI Agent Coding GuideThis guide is for AI coding agents working in the OpenClaude repository. Read it before changing code, and also follow CONTRIBUTING.md for contributor policy, PR expectations, review follow-up, and project scope.
Project Snapshot
OpenClaude is a coding-agent CLI for cloud and local model providers. It supports OpenAI-compatible APIs, Anthropic, Gemini, DeepSeek, Ollama, MCP, local backends, slash commands, tools, agents, and a React/Ink terminal UI.
The installed CLI runs on Node.js
>=22.0.0. Bun is used for source builds, scripts, dependency management, and tests.Work Style
- Keep changes focused on one problem.
- Prefer existing patterns in the file or nearby module.
- Avoid unrelated formatting, renames, dependency changes, or broad rewrites.
- Add or update tests when behavior changes.
- Update docs when setup, commands, provider behavior, or user-facing behavior changes.
- For new features, larger refactors, dependencies, or runtime changes, follow the issue-first guidance in CONTRIBUTING.md.
Stack And Conventions
- TypeScript with strict mode and ESM imports.
- React + Ink for terminal UI.
- Bun lockfile and Bun scripts for development workflows.
- Node runtime for the built CLI.
Common libraries and patterns:
chalkfor terminal color.commanderfor CLI argument parsing.execafor child processes.- Existing service, provider, settings, permission, and UI patterns over new abstractions.
Repository Map
src/commands/- slash and CLI command implementations.src/components/- React/Ink UI components.src/services/- API, MCP, OAuth, wiki, voice, and other service integrations.src/tools/- tool implementations.src/utils/- shared utilities.src/integrations/- provider and model integration metadata.src/entrypoints/- CLI, MCP, SDK, and generated public types.src/tasks/- local, remote, workflow, and monitor tas...
Files:
src/tools/firecrawl/client.test.tssrc/tools/firecrawl/client.tsscripts/system-check.ts
src/{components/permissions,utils/permissions,hooks/toolPermission,tools,entrypoints/sdk}/**
⚙️ CodeRabbit configuration file
src/{components/permissions,utils/permissions,hooks/toolPermission,tools,entrypoints/sdk}/**: Review permission prompts, auto-allow logic, sandbox behavior, SDK permission schemas, shell/PowerShell execution, and background execution paths as security-sensitive. Block on bypasses, unclear trust boundaries, unsafe path handling, missing user visibility, or changes that broaden allowed behavior without an explicit maintainer decision.
Files:
src/tools/firecrawl/client.test.tssrc/tools/firecrawl/client.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/tools/firecrawl/client.test.ts
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
scripts/system-check.ts
🔇 Additional comments (3)
scripts/system-check.ts (1)
46-46: LGTM!src/tools/firecrawl/client.ts (1)
58-62: Cloud-detection gating logic looks correct.Switching from a substring check to hostname-based classification correctly prevents proxy URLs like
https://proxy.example.com/api.firecrawl.devfrom being misclassified as cloud (previously.includes('api.firecrawl.dev')would have falsely required an API key here). This aligns with the downstreamsystem-check.tsusage shown in the context snippets.src/tools/firecrawl/client.test.ts (1)
90-113: 🩺 Stability & AvailabilityEnv cleanup is already isolated in this test file. The file restores both
FIRECRAWL_API_KEYandFIRECRAWL_API_URLinafterEach, so this mutation does not leak into other tests.> Likely an incorrect or invalid review comment.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
- [P2] Keep auto-mode diagnostics aligned with fallback providers
scripts/system-check.ts:467
InWEB_SEARCH_PROVIDER=auto, this returns the Firecrawl missing-key failure before considering the rest of the configured provider chain. That makesdoctor:runtimefail for a usable setup such asFIRECRAWL_API_URL=https://api.firecrawl.devwithoutFIRECRAWL_API_KEYbut withBRAVE_API_KEYconfigured: runtimerunSearch()catches the Firecrawl error and falls through to Brave, while the new diagnostic reports the whole Web search backend as failed. Please only fail this Firecrawl cloud-key case when there is no later runnable provider, or include the fallback provider in the pass/warning so the diagnostic matches what WebSearch will actually do in auto mode.
chioarub
changed the title
f8645b7 to
b26b3e0
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.
Findings
- [P2] Keep custom preset diagnostics aligned with runtime parsing
scripts/system-check.ts:389
The new custom-mode diagnostic trimsWEB_PROVIDERbefore matching built-in presets, but the runtime custom provider still reads the rawWEB_PROVIDERvalue. WithWEB_SEARCH_PROVIDER=customandWEB_PROVIDER='brave 'orWEB_PROVIDER='serpapi ', plusWEB_KEY,doctor:runtimereports the preset credentials as configured, whilecustomProvider.search()does not find that preset and fails before the request with"" cannot be parsed as a URL.The same mismatch affectsWEB_PROVIDER='google 'whenWEB_KEYandGOOGLE_CSE_IDare set. Please either normalizeWEB_PROVIDERthe same way in the runtime custom provider or have the diagnostic fail for the unrecognized raw preset value, so the doctor report does not mark a backend healthy when WebSearch cannot use it.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
@kevincodex1 LGTM
* feat(doctor): add WebSearch backend diagnostics * fix(doctor): tighten Firecrawl cloud URL diagnostics * fix(firecrawl): align cloud URL detection * test(websearch): stabilize Brave timeout assertion * fix(firecrawl): handle bare cloud host casing * fix(doctor): align WebSearch auto diagnostics with fallback * fix(doctor): align custom preset diagnostics
OpenCC cherry-pick of Gitlawb#1884 (WebSearch backend diagnostics, 4ad0f0e) ported scripts/system-check.ts but missed the required firecrawl/client.ts implementation, leaving 2 broken imports: - scripts/system-check.ts:30 → ../src/tools/firecrawl/client.js (doctor crash) - src/tools/WebSearchTool/providers/firecrawl.ts:5 → ../firecrawl/client.js (only triggered when firecrawl is active web search backend) Fix: - Port src/tools/firecrawl/client.ts from upstream main (183 lines) - Update WebSearchTool/providers/firecrawl.ts import to canonical ../../firecrawl/client.js path Verification: - bun run typecheck: 0 errors - bun run doctor:runtime: 12/12 PASS (was 0/0 before fix)
3 participants
Summary
doctor:runtimeWebSearch backend diagnostic that reports the resolved provider mode and configured API-backed providers.WEB_SEARCH_TIMEOUT_SECapplies.Implementation
Tests
bun test scripts/system-check.test.tsenv CLAUDE_CODE_USE_BEDROCK=1 bun test scripts/system-check.test.tsenv XAI_API_KEY=xai-test-key bun test scripts/system-check.test.tsbun test src/tools/WebSearchTool/providers/timeout.test.tsbun run typecheckbun run buildbun run doctor:runtimebun run doctor:runtime:jsongit diff --checkRisk
Summary by CodeRabbit