web_search: add domains allowlist + score field#209
Merged
Conversation
The web_search tool now accepts an optional 'domains' array that filters results to the listed hostnames before they reach the model. This is the zero-only prompt-injection defense that none of the major CLIs ship: a model that the user has constrained to a known-good set of domains cannot be tricked into fetching arbitrary pages from search results. Also surfaces the provider's optional 'score' field on each result row when present (zero/absent scores are not rendered, to keep the common case tidy). The 'score' field is parsed from the generic JSON shape already understood by the httpSearchBackend. Filters are applied after the search returns, before the empty-result short-circuit, so a non-empty allowlist that ate every hit surfaces as a clear 'no results matched domains' error rather than a misleading 'no results' message. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
Walkthrough
Changesweb_search: domains allowlist and score field
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 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 |
![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)
internal/tools/web_search_test.go (1)
365-374: ⚡ Quick winAdd regression tests for invalid-only
domainsandhost:portnormalization.Current tests don’t cover the case where
domainsis provided but normalizes to empty, or where allowlist input includeshost:port. Add both so these security/correctness contracts stay locked in.Also applies to: 418-471
🤖 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 `@internal/tools/web_search_test.go` around lines 365 - 374, Add two regression test functions to cover critical security and correctness contracts: first, add a test near TestWebSearchDomainsFilterRejectsBadArgType (around 365-374) that verifies the tool properly handles the case where domains are provided but normalize to empty after validation, and second, add another test around the 418-471 range that verifies the allowlist properly normalizes and handles host:port formatted inputs. These tests ensure that edge cases in domain filtering and host:port normalization are locked in and don't regress in future 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.
Inline comments:
In `@internal/tools/web_search.go`:
- Around line 395-401: The host canonicalization uses parsed.Host which can
include the port (e.g., "react.dev:443"), while another location extracts hosts
using Hostname() which excludes the port. This inconsistency causes allowlist
entries with ports to fail matching. Replace the call to parsed.Host with
parsed.Hostname() in the canonicalization logic to ensure both sides
consistently compare the hostname without the port component, allowing allowlist
entries like "https://react.dev:443/path" to match against
"https://react.dev/...".
- Around line 134-137: The domains filtering is bypassed when all domain entries
are invalid and normalized to empty, causing unrestricted results to be returned
instead of failing safely. Modify the logic to detect when domains are
originally provided by the caller but normalize to empty (in the normalization
logic that drops invalid entries), and return an error in this case. The
stringListArgWebSearch call at line 134-137 retrieves the domains argument,
which is then normalized (dropping invalid entries) before being checked for
filtering at line 153-159. Add a validation check that errors if domains were
provided but became empty after normalization, ensuring the fail-closed
guarantee is maintained.
---
Nitpick comments:
In `@internal/tools/web_search_test.go`:
- Around line 365-374: Add two regression test functions to cover critical
security and correctness contracts: first, add a test near
TestWebSearchDomainsFilterRejectsBadArgType (around 365-374) that verifies the
tool properly handles the case where domains are provided but normalize to empty
after validation, and second, add another test around the 418-471 range that
verifies the allowlist properly normalizes and handles host:port formatted
inputs. These tests ensure that edge cases in domain filtering and host:port
normalization are locked in and don't regress in future changes.
🪄 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: Pro Plus
Run ID: bf9d0f85-116e-4d75-aa80-72b3ab65f6ec
📒 Files selected for processing (2)
internal/tools/web_search.gointernal/tools/web_search_test.go
…calize CodeRabbit review fixes: 1. canonicalizeWebSearchHost: switch parsed.Host -> parsed.Hostname() so an allowlist entry like 'https://react.dev:443/path' normalizes the same as 'react.dev'. The previous code left ':443' in the string, which silently broke every match against a result URL on the default port. 2. stringListArgWebSearch: return a 'provided' bool so Run can distinguish 'no allowlist' from 'allowlist that normalized to empty'. The previous code returned len(domains)==0 in both cases, and Run fell through to the unfiltered path — defeating the prompt-injection defense the parameter exists to provide. Now we error fail-closed. 3. Tests: - TestWebSearchDomainsFilterRejectsAllInvalidInputs: locks in the fail-closed contract for all-whitespace, all-empty, embedded-space, and mixed-invalid allowlists. - TestWebSearchDomainsFilterAcceptsHostPort: locks in the Hostname()-strips-port fix for the scheme'd case and asserts that scheme-less 'host:port' is rejected. - TestCanonicalizeWebSearchHostStripsPort: the direct unit-level regression test for canonicalizeWebSearchHost. Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
Fixed all three issues in 037317f: Issue 1 (port stripping): Issue 2 (fail-closed on all-invalid allowlist): Issue 3 (host:port regression test): All 23 web_search tests pass. Re-running the review, please. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tools/web_search.go (1)
361-361: 💤 Low valueIneffectual assignment to
provided(static analysis).The assignment
provided = trueis never read because every return path after this point explicitly returnstrueas the second value. You could either remove the assignment or use named return values to simplify the returns.Current code is correct but the linter flags it as dead. Consider:
Option: remove the redundant assignment
- provided = true raw, ok := value.([]any)🤖 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 `@internal/tools/web_search.go` at line 361, The variable `provided` is assigned the value `true` in the web_search.go file but this assignment is never actually used because every return statement after this point explicitly returns `true` as the second return value. Remove the redundant assignment `provided = true` statement since it has no effect on the function's behavior - the explicit `true` values in the return statements already handle what needs to be returned.Source: Linters/SAST tools
🤖 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 `@internal/tools/web_search.go`:
- Line 361: The variable `provided` is assigned the value `true` in the
web_search.go file but this assignment is never actually used because every
return statement after this point explicitly returns `true` as the second return
value. Remove the redundant assignment `provided = true` statement since it has
no effect on the function's behavior - the explicit `true` values in the return
statements already handle what needs to be returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d99cbf3-4717-49d8-b6fc-03c51ee564fa
📒 Files selected for processing (2)
internal/tools/web_search.gointernal/tools/web_search_test.go
gnanam1990
approved these changes
Jun 15, 2026
gnanam1990
left a comment
gnanam1990
left a comment
Collaborator
There was a problem hiding this comment.
Review — web_search domains allowlist + score
Nice, security-minded addition. The domains allowlist is a genuine prompt-injection mitigation and the fail-closed handling is exactly right:
- domains provided but all entries normalize away → hard error (not a silent unconstrained search);
- allowlist eats every result → a clear "no results matched domains" error instead of a misleading "no results";
- unparseable result URLs are dropped when an allowlist is set.
Host canonicalization (scheme/port/path/www. stripping, lowercase, dedup, []any and []string tolerance) is thorough, and the tests cover the meaningful paths. score is cleanly optional (zero = absent). LGTM.
Non-blocking notes
- Exact-host vs subdomain semantics.
filterWebSearchByDomainsmatches the exact host (afterwww.strip), sodomains:["react.dev"]keepsreact.dev/www.react.devbut notblog.react.dev. For a security allowlist that's the safer default — worth a one-line doc note in the param description so callers aren't surprised (or consider opt-in suffix matching later). - Merge overlap with #208. Both PRs edit the
convertclosure inparseSearchResults. #208 (staticcheck nits) rewrote that struct literal assearchResult(item)(S1016); this PR rewrites it as an explicit literal withScore. Whichever lands second needs a trivial conflict fixup — sincerawResultandsearchResultwill share field order incl.Score,searchResult(item)still compiles, so collapsing to that post-merge is cleanest. Just flagging so it isn't a surprise.
Gate-wise the change is stdlib-only and looks consistent with the existing tool conventions.
gnanam1990
added a commit
that referenced
this pull request
Jun 16, 2026
…active search (#218) * web_search: fix #209 follow-ups (tolerant score, render gate, redaction, FQDN) Verified-and-fixed defects from a review of #209: - Tolerant score decode (lenientScore): a stringified/odd/null "score" no longer makes json.Unmarshal discard the entire response; it degrades to absent. - Score render gate rounds first (>= 0.01) so tiny positives and negatives no longer print a noisy "score 0.00". - Domain-filter error now goes through redactWebSearchText, matching the other error paths. - Trailing-dot FQDN symmetry: "react.dev." and "react.dev" normalize the same on both the allowlist and result sides. Left as-is (intended, tested): scheme-less host:port allowlist entries. Includes mixed-type domains-array tests. * web_search: only register the built-in tool when a backend is configured When no search backend is set (ZERO_WEBSEARCH_BASE_URL / ZERO_WEBSEARCH_BACKEND), CoreNetworkTools no longer offers the built-in web_search. Previously it was always registered, so a model with an MCP search tool (e.g. Exa) would burn several calls + high-risk permission prompts on the built-in tool — which can only return "no backend configured" — before falling back to the working MCP tool. Now: no backend -> only web_fetch (+ any MCP search tool) is offered, so the model uses the MCP search directly. A configured backend behaves exactly as before. Tests: registry/web_search/specialist tests that asserted web_search is always in CoreTools now configure a backend; added a test that it is omitted when none is set. (Pre-existing, unrelated: internal/cli doctor/exec tests fail in the sandbox on origin/main too — network/provider-dependent, not touched here.) * tui: copy selected transcript text to the native OS clipboard Selecting transcript text auto-copies on mouse release, but the copy went only through OSC52 (ansi.SetSystemClipboard), which macOS Terminal.app doesn't support at all and iTerm2 / some Windows Terminal builds have off by default — so the selection silently never reached the clipboard. Copy via the native clipboard (pbcopy / clip.exe / xclip) first, which works on local terminals regardless of OSC52, and fall back to OSC52 for remote/SSH sessions where no local clipboard utility is reachable. atotto/clipboard was already in the module graph (indirect); promoted to a direct import — no new dependency. Gates: gofmt/vet/build(host+linux+windows)/tui test -race/staticcheck all pass. * tui: collapse long tool output by default with click-to-expand Long, noisy tool output (web-search / MCP / read dumps) flooded the transcript. Now a tool result whose output exceeds the live body cap renders collapsed by default — head + a "▸ N lines — click to expand" footer — and you click the card (while it is live) to expand (▾) / collapse. Mirrors the existing reasoning-row collapse, so collapsed rows also flush to scrollback clean. Scoped to avoid hiding what matters: - diff tools (edit_file / apply_patch / write_file) never collapse — their body must stay reviewable (matches the deeper flush cap intent); - the uncapped detailed transcript view (Ctrl+O, bodyCap==0) never collapses — it still shows full output; - short output (<= cardBodyMaxLines) stays inline as before. Reuses transcriptRow.expanded + the row toggle (toggleReasoningRow generalized to toggleTranscriptRow); the card head is a clickable toggle line. Running tool cards (spinner) are unchanged. Tests: collapse/expand, short-output inline, diff tools never collapse, toggle, and the head exposes a click toggle; existing diff + detailed-view tests updated. Gates: gofmt/vet/build(host+linux+windows)/tui -race/staticcheck all clean. * cli: fall back to a usable saved provider instead of re-running onboarding If the active provider lacked a credential (e.g. it relies on an env var that isn't set, or an OAuth login that isn't stored), the TUI forced first-run onboarding on every launch — even when other saved providers were fully configured. Each onboarding then appended yet another provider. Now, when the active provider isn't usable, fall back to the first usable saved provider (prefer a remote keyed one over a local endpoint) for the session, and skip onboarding. Onboarding only runs when no configured provider is usable — a genuinely fresh setup. Saved logins persist; switch the active provider with `zero provider use <name>`. Tests: firstUsableProvider prefers remote keyed, falls back to local, accepts a keyless local proxy, and reports none-usable. Gates: gofmt/vet/build (host+linux+windows)/staticcheck clean; the only failing cli tests (doctor/exec, network-dependent) are pre-existing on origin/main. * tui: hold the scroll position while output streams in The chat viewport scroll offset is measured from the bottom, so when the transcript grew (a streaming turn) the window followed the new bottom and yanked the user off whatever they had scrolled up to read. syncChatScroll now pins the viewport: while the user is scrolled up, it bumps the offset by however many lines the body grew, so the absolute view holds where they are reading; at the bottom (offset 0) it follows the tail as before. Hooked into the single Update funnel; only the scrolled-up path renders the body, so the common (at-bottom) case stays cheap. Tests: pins by the growth delta while scrolled up; resets/follows at the bottom. Gates: gofmt/vet/build(host+linux+windows)/tui -race/staticcheck clean. * agent: search the web proactively instead of answering "I don't know" When a web search/fetch tool is available and the user asks about something external the model is unsure of or that may be current (a product, library, company, price, version, recent event), the system prompt now tells it to search and read results before answering, rather than replying that it doesn't know without checking. Addresses the model saying "I don't know about that" until the user explicitly asked it to search. Tests: prompt includes the new guidance ("search the web and read"). Existing prompt assertions use Contains, so the addition is non-breaking. * tui: show clean labels for MCP tool cards (mcp_exa_web_search_exa → web search) * agent: sharpen web-search guidance (disambiguate, refine, scale) Strengthen the search rule so the model searches before answering about an external entity/product/library/model/version/release it doesn't recognize, doesn't assume the most common meaning of an ambiguous name (disambiguates by the conversation's domain), and refines + re-searches when results look off-topic — rather than confidently answering the wrong interpretation (e.g. reading "Fable 5" as a video game instead of an AI model). Also: don't web-search timeless facts or workspace-answerable questions (use the file tools), keep queries short, and scale the number of searches to the question. Tests: prompt includes the new guidance ("search the web before answering", "do not recognize"). Existing assertions use Contains, so non-breaking. * test: gofmt setup_fallback_test.go (comment alignment) * Address review feedback on web_search + chat UX Resolve CodeRabbit review comments on this PR: - firstUsableProvider: skip a profile whose CatalogID no longer resolves *and* that has no explicit BaseURL (no endpoint → unusable). A stale CatalogID with a BaseURL still works as a custom endpoint and is kept. - providerProfileIsLocal: classify by parsed URL hostname (exact localhost / 127.0.0.1 / ::1) instead of substring matching, so hosts like "notlocalhost.com" are no longer misread as local. - lenientScore: reject non-finite scores (ParseFloat accepts "NaN"/"Inf") so the documented score filter actually holds and the renderer never emits a non-finite value. - syncChatScroll: shift the pinned from-bottom offset by the signed line delta so the view also holds when the body shrinks (card collapse, transcript clear), clamped at zero; previously only growth was handled. - copyTranscriptSelectionCmd: report "Copy failed" when both the native clipboard and the OSC52 fallback fail, instead of claiming success; keep the selection so the user can retry. - Drop vestigial ZERO_WEBSEARCH_BACKEND test env: the native backend was removed (MCP path), so the var is read by no product code and the references misrepresented it as a config knob. Adds tests for the local-host parsing, the catalog/BaseURL skip, and the non-finite score rejection.
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
2 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.
Enriches the existing web_search tool (merged in #175) with two model-facing additions: a domains allowlist (the zero-only prompt-injection defense) and provider score surfacing. See commit body for details.
Summary by CodeRabbit
New Features
Bug Fixes
Tests