web_search robustness + chat UX: collapse, clipboard, scroll-pin, proactive search#218
Merged
Conversation
…on, 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.
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.)
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.
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.
…rding 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.
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.
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.
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. |
WalkthroughAdds conditional ChangesWeb Search Tool Hardening
CLI Provider Fallback on Startup
TUI Tool-Card Collapse, MCP Name Cleaning, Scroll-Pin, and Clipboard
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/setup_fallback_test.go (1)
1-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFormat this test file with
gofmtto unblock CI.Both smoke jobs report this file as unformatted. Please run
gofmt -w internal/cli/setup_fallback_test.go.🤖 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/cli/setup_fallback_test.go` around lines 1 - 54, The test file setup_fallback_test.go is not properly formatted according to Go standards and is causing CI failures. Run the gofmt tool with the write flag to automatically format the entire file to comply with standard Go formatting conventions.Source: Pipeline failures
🧹 Nitpick comments (1)
internal/agent/system_prompt.md (1)
74-78: ⚡ Quick winScope this rule to search-capable tools, not
web_fetchalone.Because
web_fetchis always present, this instruction becomes effectively always-on and can still drive unnecessary external-call attempts when no search backend exists. Recommend narrowing to built-inweb_search(or MCP search-equivalent) availability.🤖 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/agent/system_prompt.md` around lines 74 - 78, The instruction in the system prompt for web search behavior references both web search and web_fetch tools as a condition, but since web_fetch is always present, this makes the rule effectively always-on and can trigger unnecessary external calls even when no search backend exists. Revise the condition to scope the rule specifically to when built-in web_search (or equivalent search-capable tools like MCP search tools) is available, removing the reference to web_fetch as a trigger condition, so the search-before-answering behavior only activates when actual search capability exists rather than just fetch capability.
🤖 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/cli/setup.go`:
- Around line 331-332: The current implementation at line 332 uses
strings.Contains on the BaseURL string, which can incorrectly match domains
containing "localhost" as a substring (e.g., "notlocalhost.com"). Instead, parse
profile.BaseURL using url.Parse() to extract the hostname component, then
perform exact equality checks against the loopback addresses "localhost",
"127.0.0.1", and "[::1]". This ensures only actual loopback hostnames are
detected as local endpoints, not domains that happen to contain these strings as
substrings.
- Around line 304-315: The profile selection logic in the setup flow skips
profiles with missing credentials but does not validate whether the catalog
profile itself is actually valid, allowing stale or invalid profiles with
unknown CatalogID values to be selected as fallback options. This causes
failures later during provider construction. Add an additional validation check
after the setupMissingCredentialEnv call to skip profiles with invalid or
unknown CatalogID values before they can be assigned to localFallback or
returned, ensuring only valid catalog profiles proceed through the setup
process.
In `@internal/tools/registry.go`:
- Around line 225-227: The condition at line 225 in the defaultSearchBackend()
check only validates if the base URL is set, which prevents registration when
only the ZERO_WEBSEARCH_BACKEND environment variable is configured. Replace the
defaultSearchBackend() != nil check with a condition that verifies whether
either ZERO_WEBSEARCH_BASE_URL or ZERO_WEBSEARCH_BACKEND is set, ensuring that
NewWebSearchTool() is registered in both configuration scenarios (base URL only,
backend only, or both).
In `@internal/tools/web_search.go`:
- Around line 317-322: The score parser accepts any successfully parsed float
without validating that it is finite, allowing NaN and Inf values to bypass the
documented filter. After strconv.ParseFloat successfully parses the float into
variable f, add a check to ensure the value is finite before calling
lenientScore(f). Use math.IsInf or math.IsNaN to validate that f represents a
finite number, and only proceed with the lenientScore assignment if the value is
finite; otherwise, leave the score at its default zero value.
In `@internal/tui/model.go`:
- Around line 1305-1310: The switch statement handling scroll offset adjustments
only compensates for growth when current is greater than m.chatBodyLines, but
does not handle the shrink case when current is less than m.chatBodyLines. Add a
new case in this switch block to handle when current shrinks below
m.chatBodyLines by adjusting m.chatScrollOffset downward to prevent the pinned
view from jumping unexpectedly. Additionally, add a regression test in
internal/tui/scroll_pin_test.go that specifically tests the shrink path to
ensure that when the body line count decreases, the scroll offset is correctly
adjusted to maintain the pinned viewport behavior.
In `@internal/tui/transcript_selection.go`:
- Around line 433-444: The copyTranscriptSelectionCmd function in the
copyTranscriptSelectionCmd returns a transcriptCopiedMsg regardless of whether
either clipboard method succeeds. When clipboard.WriteAll fails, it falls back
to OSC52 via ansi.SetSystemClipboard, but both failures are silently ignored and
the function still reports success. Create a new message type called
transcriptCopyFailedMsg to represent copy failures. Modify
copyTranscriptSelectionCmd to check if the OSC52 fallback method also fails
(check if ansi.SetSystemClipboard returns an error or verify the write was
actually successful), and return transcriptCopyFailedMsg instead of
transcriptCopiedMsg when both methods fail. Then update the model.Update method
to handle transcriptCopyFailedMsg and display a failure status instead of the
success message.
---
Outside diff comments:
In `@internal/cli/setup_fallback_test.go`:
- Around line 1-54: The test file setup_fallback_test.go is not properly
formatted according to Go standards and is causing CI failures. Run the gofmt
tool with the write flag to automatically format the entire file to comply with
standard Go formatting conventions.
---
Nitpick comments:
In `@internal/agent/system_prompt.md`:
- Around line 74-78: The instruction in the system prompt for web search
behavior references both web search and web_fetch tools as a condition, but
since web_fetch is always present, this makes the rule effectively always-on and
can trigger unnecessary external calls even when no search backend exists.
Revise the condition to scope the rule specifically to when built-in web_search
(or equivalent search-capable tools like MCP search tools) is available,
removing the reference to web_fetch as a trigger condition, so the
search-before-answering behavior only activates when actual search capability
exists rather than just fetch capability.
🪄 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: d15e4e9f-23e4-4aac-97c9-fd23b31f8e70
📒 Files selected for processing (18)
go.modinternal/agent/system_prompt.mdinternal/agent/system_prompt_test.gointernal/cli/app.gointernal/cli/setup.gointernal/cli/setup_fallback_test.gointernal/specialist/manifest_test.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/web_search.gointernal/tools/web_search_test.gointernal/tui/collapse_tools_test.gointernal/tui/model.gointernal/tui/rendering.gointernal/tui/scroll_pin_test.gointernal/tui/tool_display_test.gointernal/tui/transcript_selection.gointernal/tui/transcript_view_test.go
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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/agent/system_prompt_test.go (1)
22-23: ⚡ Quick winStrengthen the new “do not recognize” assertion to reduce false positives.
Line 23’s
"do not recognize"check is very broad and can pass on unrelated wording. Consider asserting a longer, behavior-tied phrase (or paired phrase) so this test fails when the intended web-search instruction regresses.🤖 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/agent/system_prompt_test.go` around lines 22 - 23, The assertion checking for "do not recognize" in the system prompt test is too broad and could produce false positives that don't verify the intended web-search instruction is actually present. Replace this generic string check with a longer, behavior-tied phrase or paired phrase that directly captures the specific web-search instruction content. This will ensure the test reliably fails when the actual web-search instruction regresses or 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.
Nitpick comments:
In `@internal/agent/system_prompt_test.go`:
- Around line 22-23: The assertion checking for "do not recognize" in the system
prompt test is too broad and could produce false positives that don't verify the
intended web-search instruction is actually present. Replace this generic string
check with a longer, behavior-tied phrase or paired phrase that directly
captures the specific web-search instruction content. This will ensure the test
reliably fails when the actual web-search instruction regresses or changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d843074-9db1-4f0c-ad64-74236c07754c
📒 Files selected for processing (2)
internal/agent/system_prompt.mdinternal/agent/system_prompt_test.go
Vasanthdev2004
approved these changes
Jun 16, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: APPROVED ✅
Reviewed locally in worktree review-pr218 at 6b33ad5. gofmt -l . flagged internal/cli/setup_fallback_test.go (fixed locally; author should re-run gofmt before merge). go vet and go build clean. go test ./internal/tui/... and ./internal/tools/... pass. The pre-existing TestRunExecUsesProjectConfigAndOpenAICompatibleProvider is not in the touched packages and isn't exercised.
What it does:
internal/tools/web_search.go— promotes the score field to a typedlenientScorethat accepts JSON numbers, JSON strings, and a few string-shaped variants (e.g."0.91"). A single bad score on one result no longer drops the whole response — it just leaves that result's score at the zero value. The renderer rounds to 2 decimal places and skips any score that rounds to< 0.01so the common case stays tidy and a noisy "score 0.00" never appears.internal/cli/setup.go— addsfirstUsableProvider: walks the saved provider profiles and picks the first one that has an inline credential (or is no-auth/local) so the CLI falls back to an already-configured login when the active provider happens to lack one, instead of re-running onboarding every launch. A local (loopback orLocalcatalog flag) provider is preferred as a fallback only when no remote keyed provider is reachable.internal/cli/setup_fallback_test.go— covers the new fallback logic. (Author: pleasegofmt -wthis file before merge — it was the onlygofmt -lhit in the PR.)- TUI UX — tool-card collapse (long tool output gets a "show more" affordance), clipboard copy on assistant messages, scroll-pin (auto-scroll only when the user is at the bottom), proactive search hint in the system prompt.
internal/tools/registry.go— registers the newweb_searchbehavior under a stable name and adds a small test.internal/specialist/manifest_test.go— one assertion update for the new tool label.
Security invariants: no credential-handling changes, no logging changes, no auth changes. The lenientScore parser rejects values that overflow float64 by leaving the field at zero — i.e. it fails closed at the field level, not the result level, which is the correct call (one bad score shouldn't cost you all results). The setup fallback only consults existing profiles; it never reads or prompts for new credentials.
One small nitpick (not a blocker): the system-prompt hint for proactive search is a good idea but it's a hint in a system_prompt.md that no model is required to follow. If the proactive-search behavior matters, consider gating it on a config flag (zero search proactive: true) so users can opt out — but that's a product call, not a code one.
Merging once gofmt is clean.
🤖 Generated with Claude Code
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/cli/setup_fallback_test.go (3)
21-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against environment variable pollution.
Line 23 assumes
xaiis not usable. IfXAI_API_KEYis set in the test environment, the test assumption breaks.🛡️ Proposed fix to isolate environment
func TestFirstUsableProviderFallsBackToLocal(t *testing.T) { + t.Setenv("XAI_API_KEY", "") providers := []config.ProviderProfile{🤖 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/cli/setup_fallback_test.go` around lines 21 - 30, The test TestFirstUsableProviderFallsBackToLocal assumes the xai provider is not usable because XAI_API_KEY environment variable is unset, but if this variable is set in the actual test environment, the assumption breaks. Guard against this environment variable pollution by clearing the XAI_API_KEY environment variable at the beginning of the test to ensure consistent test behavior regardless of the host environment. Use Go's t.Setenv() function to temporarily unset or clear the XAI_API_KEY variable, which will automatically be restored after the test completes.
32-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against environment variable pollution.
Lines 34-35 assume both providers are not usable. If either
XAI_API_KEYorOPENAI_API_KEYis set in the test environment, the test breaks.🛡️ Proposed fix to isolate environment
func TestFirstUsableProviderNoneUsable(t *testing.T) { + t.Setenv("XAI_API_KEY", "") + t.Setenv("OPENAI_API_KEY", "") providers := []config.ProviderProfile{🤖 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/cli/setup_fallback_test.go` around lines 32 - 40, The test TestFirstUsableProviderNoneUsable assumes that the XAI_API_KEY and OPENAI_API_KEY environment variables are not set, but if they are present in the test environment, the test will fail. Use t.Setenv() to explicitly set both XAI_API_KEY and OPENAI_API_KEY to empty strings at the start of the test to isolate it from environment pollution and ensure that firstUsableProvider() correctly returns ok=false when no credentials are available.
9-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against environment variable pollution.
Line 13 assumes the
xaiprovider is not usable because it has onlyAPIKeyEnvand no inlineAPIKey. IfXAI_API_KEYis set in the test environment, this assumption breaks and the test may fail or return unexpected results.🛡️ Proposed fix to isolate environment
func TestFirstUsableProviderPrefersRemoteKeyed(t *testing.T) { + t.Setenv("XAI_API_KEY", "") providers := []config.ProviderProfile{🤖 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/cli/setup_fallback_test.go` around lines 9 - 19, The test TestFirstUsableProviderPrefersRemoteKeyed assumes that the xai provider is unusable because it only has APIKeyEnv set without an inline APIKey, but if the XAI_API_KEY environment variable is set in the test environment, this assumption fails and the test produces unreliable results. Guard against this by explicitly unsetting or clearing the XAI_API_KEY environment variable at the start of the test to ensure it remains unset during the test execution, or use t.Setenv to temporarily override it to an empty value to isolate the test from environment pollution.
🤖 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.
Outside diff comments:
In `@internal/cli/setup_fallback_test.go`:
- Around line 21-30: The test TestFirstUsableProviderFallsBackToLocal assumes
the xai provider is not usable because XAI_API_KEY environment variable is
unset, but if this variable is set in the actual test environment, the
assumption breaks. Guard against this environment variable pollution by clearing
the XAI_API_KEY environment variable at the beginning of the test to ensure
consistent test behavior regardless of the host environment. Use Go's t.Setenv()
function to temporarily unset or clear the XAI_API_KEY variable, which will
automatically be restored after the test completes.
- Around line 32-40: The test TestFirstUsableProviderNoneUsable assumes that the
XAI_API_KEY and OPENAI_API_KEY environment variables are not set, but if they
are present in the test environment, the test will fail. Use t.Setenv() to
explicitly set both XAI_API_KEY and OPENAI_API_KEY to empty strings at the start
of the test to isolate it from environment pollution and ensure that
firstUsableProvider() correctly returns ok=false when no credentials are
available.
- Around line 9-19: The test TestFirstUsableProviderPrefersRemoteKeyed assumes
that the xai provider is unusable because it only has APIKeyEnv set without an
inline APIKey, but if the XAI_API_KEY environment variable is set in the test
environment, this assumption fails and the test produces unreliable results.
Guard against this by explicitly unsetting or clearing the XAI_API_KEY
environment variable at the start of the test to ensure it remains unset during
the test execution, or use t.Setenv to temporarily override it to an empty value
to isolate the test from environment pollution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b8df3e9-8a11-4b7e-b2b3-8664073f8558
📒 Files selected for processing (1)
internal/cli/setup_fallback_test.go
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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cli/setup_fallback_test.go (1)
69-91: ⚡ Quick winAdd table cases for catalog-local and malformed URL paths.
Line 69–91 currently exercises only the
BaseURLbranch. Please add cases that hit the catalogLocalbranch and malformed URL parsing so both decision paths are locked by tests.As per coding guidelines in the PR context,
providerProfileIsLocalis expected to classify using catalogLocalor localhostBaseURL, so both branches should be covered.Proposed test-shape update
func TestProviderProfileIsLocal(t *testing.T) { cases := []struct { - name string - baseURL string - want bool + name string + profile config.ProviderProfile + want bool }{ - {"loopback name", "http://localhost:11434/v1", true}, - {"loopback v4", "http://127.0.0.1:8080", true}, - {"loopback v6", "http://[::1]:10531/v1", true}, - {"remote", "https://api.moonshot.ai/v1", false}, - {"contains-localhost-substring", "https://notlocalhost.com/v1", false}, - {"host-with-127-substring", "https://api127.0.0.1.example.com", false}, - {"empty", "", false}, + {"loopback name", config.ProviderProfile{BaseURL: "http://localhost:11434/v1"}, true}, + {"loopback v4", config.ProviderProfile{BaseURL: "http://127.0.0.1:8080"}, true}, + {"loopback v6", config.ProviderProfile{BaseURL: "http://[::1]:10531/v1"}, true}, + {"remote", config.ProviderProfile{BaseURL: "https://api.moonshot.ai/v1"}, false}, + {"contains-localhost-substring", config.ProviderProfile{BaseURL: "https://notlocalhost.com/v1"}, false}, + {"host-with-127-substring", config.ProviderProfile{BaseURL: "https://api127.0.0.1.example.com"}, false}, + {"empty", config.ProviderProfile{}, false}, + {"malformed-url", config.ProviderProfile{BaseURL: "http://[::1"}, false}, + {"catalog-local-without-baseurl", config.ProviderProfile{CatalogID: "ollama"}, true}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got := providerProfileIsLocal(config.ProviderProfile{BaseURL: tc.baseURL}) + got := providerProfileIsLocal(tc.profile) if got != tc.want { - t.Fatalf("providerProfileIsLocal(%q) = %v, want %v", tc.baseURL, got, tc.want) + t.Fatalf("providerProfileIsLocal(%+v) = %v, want %v", tc.profile, got, tc.want) } }) } }🤖 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/cli/setup_fallback_test.go` around lines 69 - 91, The TestProviderProfileIsLocal function currently only tests the BaseURL branch of providerProfileIsLocal. Add test cases to the cases slice that exercise the catalog Local branch by creating test cases with Local set to true in the config.ProviderProfile, and add test cases with malformed or invalid URL paths to ensure error handling is covered. Update the test case struct to include a Local field alongside name, baseURL, and want so these new scenarios can be properly tested.
🤖 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/cli/setup_fallback_test.go`:
- Around line 69-91: The TestProviderProfileIsLocal function currently only
tests the BaseURL branch of providerProfileIsLocal. Add test cases to the cases
slice that exercise the catalog Local branch by creating test cases with Local
set to true in the config.ProviderProfile, and add test cases with malformed or
invalid URL paths to ensure error handling is covered. Update the test case
struct to include a Local field alongside name, baseURL, and want so these new
scenarios can be properly tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 04407276-dbfb-4f01-952a-41a52c3e958d
📒 Files selected for processing (8)
internal/cli/setup.gointernal/cli/setup_fallback_test.gointernal/specialist/manifest_test.gointernal/tools/registry_test.gointernal/tools/web_search.gointernal/tools/web_search_test.gointernal/tui/model.gointernal/tui/transcript_selection.go
💤 Files with no reviewable changes (2)
- internal/specialist/manifest_test.go
- internal/tools/registry_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/cli/setup.go
- internal/tui/model.go
- internal/tui/transcript_selection.go
- internal/tools/web_search_test.go
- internal/tools/web_search.go
Collaborator
Author
|
Pushed
Added tests for the local-host parsing, the catalog/BaseURL skip, and the non-finite score. gofmt / vet / build / @coderabbitai review |
|
✅ Action performedReview finished.
|
Collaborator
Author
|
@CodeRabbit approvable ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get open review comments on PR `#218`
gh pr view 218 --repo Gitlawb/zero --json reviews,reviewDecision,state
echo "---"
# List review comments still open (not resolved)
gh api repos/Gitlawb/zero/pulls/218/comments --jq '.[] | {id, user: .user.login, body: .body[:200], position: .position}' 2>/dev/null | head -100
echo "---"
# Check review threads
gh api repos/Gitlawb/zero/pulls/218/reviews --jq '.[] | {id, user: .user.login, state, submitted_at}' 2>/dev/nullLength of output: 33050
Addressed in
Remaining open items (all nitpicks, not blockers):
None of the above block correctness or safety. Approving now. [approve] |
Collaborator
Author
|
@CodeRabbit approve |
✅ Action performedComments resolved and changes approved. |
gnanam1990
added a commit
that referenced
this pull request
Jun 16, 2026
Bring the branch current with main (which now carries #213's AES-256-GCM encrypted token store, #218, #219, #204). No conflicts: #217's OAuth-login code does not overlap #213's store/auth changes, so auth.go and store.go auto-merge cleanly — the OAuth feature now sits alongside the encrypted-file backend. Build, vet, cross-compile (host/linux/windows) and the oauth/cli/providerio/tui suites pass against the merged tree.
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.
Summary
A batch of independent fixes that harden
web_searchand make the chat surface much nicer to live in. Each is small, tested, and self-contained.web_search
web_searchwhen no backend is configured.CoreNetworkToolsregistered it unconditionally, so a model with an MCP search tool (e.g. Exa) burned several calls + high-risk permission prompts on a tool that can only return "no backend configured" before falling back. Now it's registered only whenZERO_WEBSEARCH_BASE_URL(orZERO_WEBSEARCH_BACKEND) is set.scoredecode — a stringified / malformed / null providerscoreno longer makesjson.Unmarshaldiscard the whole response; it degrades to "absent".>= 0.01), so tiny positives and negatives no longer print a noisyscore 0.00.redactWebSearchText, matching the other error paths.react.dev.andreact.devnormalize the same on the allowlist and result sides.Chat UX
▸ N lines — click to expand(click while live to expand▾), so the transcript stays scannable. Diff tools (edit_file/apply_patch/write_file), the uncapped detailed view (Ctrl+O), and short output are never collapsed. Reuses the existing reasoning-row collapse, so collapsed rows flush to scrollback clean.mcp_<server>_<tool>;toolDisplayNamenow renders a clean label (mcp_exa_web_search_exa→web search) with the query beside it. Built-in names are unchanged.pbcopy/clip.exe/xclip) with OSC52 as the SSH/remote fallback. (atotto/clipboardwas already in the module graph; promoted to a direct import — no new dependency.)Provider setup
zero providers use <name>.Agent behavior
Testing
go vet, build (host + linux + windows),go test ./... -race— green except 3 pre-existinginternal/clifailures (doctor/exec, network/provider-dependent) that fail identically onmain.Notes
Summary by CodeRabbit
Release Notes
New Features
Improvements
web_searchis more resilient and handles domain matching more consistently (including trailing dots).Bug Fixes
web_searchis no longer available when no search backend is configured.