better web search, improve parsing, error handling, logging, anthropic models#113
Merged
Conversation
Replace Exa-specific tooling with a generic AI-services web tools API. Renamed ExaEndpoint to ToolEndpoint and introduced aiServicesToolURL to build /tools/{tool} endpoints; fetch now reports fetch_method as "web_tool" and FetchContents/FetchResult were adapted to a new request/response schema (snake_case fields, markdown/text mapping, request_id variants, extras/metadata). Search tooling and schemas were updated to use new parameter names (search_context_size, allowed_domains, freshness, limit) and to tolerate both snake_case and camelCase provider fields; SearchRequestOptions and related types were adjusted accordingly. README, connector wiring, source collection, and tests were updated to match the new endpoints and payload formats, and fetch text limits/behavior were tuned to the new tool semantics.
Add structured citation extraction and routing plus a new "open" web tool and readable-open logic. - Introduces ai.Citation and plumbing to collect provider-native citations across providers (OpenAI, Google, Anthropic) via pkg/ai/providers/citations.go and hooks in provider stream handlers. - Replaces the old fetch tool surface with an "open" tool that accepts ref_id (or full URL) and integrates a per-run reference store (refs.go) populated by web_search results so search items can be opened by ref_id. - Enhances Fetch/Open logic to prefer readable representations (Markdown/plain/JSON/XML/CSV), honor Link headers and <link rel=alternate> HTML alternates, set an Accept header for direct opens, and fall back to the tool extraction endpoint when appropriate. Adds HTML/link header parsing and content-type helpers. - Updates tests to cover Markdown alternates, ref_id roundtrips, Accept header behavior, and provider citation parsing. README updated to reflect the new "open" semantics and SSRF note wording. This change surfaces citable sources in assistant messages, enables compact search->open workflows via ref_id, and improves direct-open heuristics for readable content.
|
|
Warning Review limit reached
More reviews will be available in 25 minutes and 55 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![indent[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/connector/slash_commands_limits.go (1)
225-241:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRaw table currently mislabels unreported limits as exhausted
For zero-value windows, the raw table prints
0%and0values, which reads as “out of quota” rather than “not reported”. This is misleading for diagnostics.Suggested fix
func appendRawLimitWindow(text *strings.Builder, label string, window aiServicesLimitWindow, now time.Time) { + left := fmt.Sprintf("`%d%%`", window.PercentageLeft) + if window.Limit == 0 && window.Used == 0 && window.Remaining == 0 { + left = "Not reported" + } + limit := formatInt(window.Limit) + if window.Limit < 0 { + limit = "Unlimited" + } fmt.Fprintf( text, "| %s | `%d%%` | `%s` | `%s` | `%s` | %s |\n", label, - window.PercentageLeft, + left, formatInt(window.Used), - formatInt(window.Limit), + limit, formatInt(window.Remaining), formatRawLimitReset(window, now), ) }🤖 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 `@pkg/connector/slash_commands_limits.go` around lines 225 - 241, The raw table currently prints zero values for unreported windows; update appendRawLimitWindow to detect unreported windows (e.g., when window.Limit == 0) and render user-friendly "Not reported" placeholders instead of `0%`/`0` values: change the fmt.Fprintf call in appendRawLimitWindow to compute display strings for PercentageLeft, Used, Limit, and Remaining (using formatInt when Limit>0, otherwise the literal "Not reported") and pass those computed strings to the table row; leave formatRawLimitReset(window, now) unchanged.pkg/chattools/search.go (1)
145-219:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize the remaining response aliases.
This parser already accepts both camelCase and snake_case for most fields, but
searchContextSizeandpublished_datestill fall through. Endpoints that emit either spelling will silently lose metadata even though the rest of the response is normalized.Suggested patch
type searchResponse struct { Query string `json:"query"` RequestID string `json:"requestId"` RequestIDSnake string `json:"request_id"` + SearchContextSizeC string `json:"searchContextSize"` ResolvedSearchType string `json:"resolvedSearchType"` SearchType string `json:"searchType"` SearchContextSize string `json:"search_context_size"` Context string `json:"context"` CostDollars map[string]any `json:"costDollars"` @@ Published string `json:"published"` PublishedAt string `json:"published_at"` PublishedDate string `json:"publishedDate"` + PublishedDateS string `json:"published_date"` SiteName string `json:"siteName"` SiteNameSnake string `json:"site_name"` @@ result := SearchResult{ Query: body.Query, RequestID: firstNonEmpty(body.RequestID, body.RequestIDSnake), RequestIDSnake: firstNonEmpty(body.RequestIDSnake, body.RequestID), ResolvedSearchType: body.ResolvedSearchType, SearchType: body.SearchType, - SearchContextSize: body.SearchContextSize, + SearchContextSize: firstNonEmpty(body.SearchContextSize, body.SearchContextSizeC), Context: body.Context, Output: body.Output, Results: make([]SearchItem, 0, len(body.Results)), } for _, item := range body.Results { snippet := firstNonEmpty(item.Snippet, firstString(item.Highlights), item.Summary, item.Text) - published := firstNonEmpty(item.Published, item.PublishedAt, item.PublishedDate) + published := firstNonEmpty(item.Published, item.PublishedAt, item.PublishedDate, item.PublishedDateS) result.Results = append(result.Results, SearchItem{ @@ Description: item.Description, Published: published, PublishedAt: firstNonEmpty(item.PublishedAt, published), - PublishedDate: item.PublishedDate, + PublishedDate: firstNonEmpty(item.PublishedDate, item.PublishedDateS), SiteName: firstNonEmpty(item.SiteName, item.SiteNameSnake),🤖 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 `@pkg/chattools/search.go` around lines 145 - 219, The parser misses camel/snake aliases for searchContextSize and published_date: add a second field to searchResponse to accept the camelCase form (e.g., add SearchContextSizeCamel with json:"searchContextSize") and add a snake-case alias on searchResponseItem (e.g., PublishedDateSnake with json:"published_date"); then update result() to normalize using firstNonEmpty for SearchContextSize (firstNonEmpty(body.SearchContextSize, body.SearchContextSizeCamel)) and include both PublishedDate and PublishedDateSnake when computing published and when assigning PublishedDate (e.g., published := firstNonEmpty(item.Published, item.PublishedAt, item.PublishedDate, item.PublishedDateSnake) and PublishedDate: firstNonEmpty(item.PublishedDate, item.PublishedDateSnake)).
🧹 Nitpick comments (1)
pkg/connector/sources_test.go (1)
118-126: ⚡ Quick winThis doesn't cover the new
rawType/nested citation parser.Lines 118-126 go through the typed
ai.Message.Citationsfast path on Lines 193-205 inpkg/connector/sources.go, soproviderCitationSourcenever runs here. A raw map payload withrawType: "url_context"or a wrapperurl_citationobject would give you regression coverage for the new normalization logic.🤖 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 `@pkg/connector/sources_test.go` around lines 118 - 126, The test uses ai.Message.Citations which triggers the typed fast path and doesn't exercise the new raw/nested citation normalization; update the test in pkg/connector/sources_test.go to add a raw-map citation payload that mimics rawType:"url_context" (or a wrapper url_citation object) so providerCitationSource's parsing logic is invoked; specifically, call newSourceCollector().addProviderSources with an ai.Message containing a raw map citation (including rawType:"url_context" and URL) to verify the normalization produces a source with "url" => "https://example.com/url-context" and thus cover the new code path.
🤖 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 `@pkg/ai/providers/openai_shared.go`:
- Around line 804-811: The function responsesNativeToolResult currently
hardcodes "provider":"openai"; update it to use the provider value from the
incoming item (e.g., derive provider :=
strings.TrimSpace(stringFromAny(item["provider"])) or similar) so other
Responses-compatible providers (Azure etc.) are labeled correctly; modify
responsesNativeToolResult (and the other similar occurrence) to set "provider"
from item["provider"] instead of the literal "openai".
- Around line 475-479: The current code calls
stream.Push(ai.AssistantMessageEvent{Type:"source", Partial: output}) even when
providerCitationsFromAny returns zero new citations; change the logic in the
annotations handling to capture the returned slice from
providerCitationsFromAny(annotations, model.Provider, index), append it to
output.Citations only if its length > 0, and only then call stream.Push; apply
the same guard to the other annotations path that uses ensureText, output,
providerCitationsFromAny, and stream.Push so no-op source events are emitted.
In `@pkg/chattools/fetch.go`:
- Around line 69-81: The current flow treats any directResult with directErr ==
nil as a successful fetch even for non-2xx HTTP responses; update the logic
after fetchDirect (and similarly after fetchDirectURL when checking alternates)
to only treat results as successful if the HTTP status is in the 2xx range
(e.g., directResult.StatusCode >= 200 && directResult.StatusCode < 300) before
calling shouldReturnDirectResult or proceeding to HTML/alternate handling; this
prevents 404/500 pages from being treated as valid content and lets the web-tool
fallback run.
- Around line 229-254: The current FetchResult population prefers body.Markdown
for Text and then overwrites Markdown during truncation, losing plaintext;
change the assignment to set Text = firstNonEmpty(body.Text, body.Markdown) and
Markdown = firstNonEmpty(body.Markdown, body.Text) (use the FetchResult creation
block where result is built), and update the truncation logic (referencing
result, textMaxChars) to truncate Text and Markdown independently (only truncate
each field to textMaxChars if it exceeds it) and set result.Truncated = true if
either field was shortened so plaintext and markdown remain distinct.
In `@pkg/chattools/types.go`:
- Around line 61-67: Change the Freshness field from a value to a pointer so
omitempty works: update the struct field declaration Freshness SearchFreshness
`json:"freshness,omitempty"` to use *SearchFreshness, and then audit any code
that constructs or reads that field (places that set Freshness,
marshal/unmarshal, or access nested fields) to handle nil checks or allocate a
&SearchFreshness when needed; ensure any helper constructors or defaults
initialize the pointer where semantics require a non-nil value.
In `@pkg/connector/sources.go`:
- Around line 572-584: The code currently keeps the outer sourceType when
non-empty, causing nested/raw citation types to be ignored; update the logic
around sourceType (the initial assignment using sourceString and the later use
of firstSourceString) so that when nested (url_citation or urlCitation) has a
non-empty type/rawType it takes precedence over the wrapper's type;
specifically, prefer the nested type/rawType returned by sourceString(nested,
"type", "rawType") before falling back to the outer sourceType (and ensure
mergeSourceMaps(citation, nested) still happens to build citation and rawURL is
read from citation) so the guard that checks sourceType includes nested/raw
citation kinds correctly.
In `@pkg/connector/timezone.go`:
- Around line 67-79: The normalizeLastKnownTimezone function uses
time.LoadLocation which will fail when the runtime image lacks IANA zoneinfo;
update the runtime and/or code so time.LoadLocation succeeds: add tzdata to the
runtime image (e.g., apk add --no-cache tzdata in the Dockerfile) and/or embed
zoneinfo by adding import _ "time/tzdata" to the Go module (any package that
runs at init, e.g., in pkg/connector or main) so normalizeLastKnownTimezone and
other callers of time.LoadLocation return real locations instead of always
falling back to ("", false).
---
Outside diff comments:
In `@pkg/chattools/search.go`:
- Around line 145-219: The parser misses camel/snake aliases for
searchContextSize and published_date: add a second field to searchResponse to
accept the camelCase form (e.g., add SearchContextSizeCamel with
json:"searchContextSize") and add a snake-case alias on searchResponseItem
(e.g., PublishedDateSnake with json:"published_date"); then update result() to
normalize using firstNonEmpty for SearchContextSize
(firstNonEmpty(body.SearchContextSize, body.SearchContextSizeCamel)) and include
both PublishedDate and PublishedDateSnake when computing published and when
assigning PublishedDate (e.g., published := firstNonEmpty(item.Published,
item.PublishedAt, item.PublishedDate, item.PublishedDateSnake) and
PublishedDate: firstNonEmpty(item.PublishedDate, item.PublishedDateSnake)).
In `@pkg/connector/slash_commands_limits.go`:
- Around line 225-241: The raw table currently prints zero values for unreported
windows; update appendRawLimitWindow to detect unreported windows (e.g., when
window.Limit == 0) and render user-friendly "Not reported" placeholders instead
of `0%`/`0` values: change the fmt.Fprintf call in appendRawLimitWindow to
compute display strings for PercentageLeft, Used, Limit, and Remaining (using
formatInt when Limit>0, otherwise the literal "Not reported") and pass those
computed strings to the table row; leave formatRawLimitReset(window, now)
unchanged.
---
Nitpick comments:
In `@pkg/connector/sources_test.go`:
- Around line 118-126: The test uses ai.Message.Citations which triggers the
typed fast path and doesn't exercise the new raw/nested citation normalization;
update the test in pkg/connector/sources_test.go to add a raw-map citation
payload that mimics rawType:"url_context" (or a wrapper url_citation object) so
providerCitationSource's parsing logic is invoked; specifically, call
newSourceCollector().addProviderSources with an ai.Message containing a raw map
citation (including rawType:"url_context" and URL) to verify the normalization
produces a source with "url" => "https://example.com/url-context" and thus cover
the new code path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79bb03df-3235-4c95-bf53-8b4f18990a39
📒 Files selected for processing (40)
README.mdpkg/agent/agent_loop.gopkg/agent/agent_loop_test.gopkg/ai/providers/anthropic.gopkg/ai/providers/anthropic_test.gopkg/ai/providers/citations.gopkg/ai/providers/google.gopkg/ai/providers/google_vertex.gopkg/ai/providers/openai_completions.gopkg/ai/providers/openai_conversion_test.gopkg/ai/providers/openai_responses.gopkg/ai/providers/openai_shared.gopkg/ai/providers/openai_stream_test.gopkg/ai/types.gopkg/aiid/ids_test.gopkg/aiid/metadata.gopkg/chattools/chattools.gopkg/chattools/chattools_test.gopkg/chattools/fetch.gopkg/chattools/search.gopkg/chattools/session.gopkg/chattools/types.gopkg/connector/builtin_tools.gopkg/connector/builtin_tools_test.gopkg/connector/chat_tools.gopkg/connector/chat_tools_test.gopkg/connector/client.gopkg/connector/room_state.gopkg/connector/room_state_test.gopkg/connector/slash_commands.gopkg/connector/slash_commands_limits.gopkg/connector/slash_commands_session.gopkg/connector/slash_commands_test.gopkg/connector/slash_commands_tools.gopkg/connector/sources.gopkg/connector/sources_test.gopkg/connector/stream_test.gopkg/connector/timezone.gopkg/connector/timezone_test.gopkg/connector/tool_modes.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-docker
🔇 Additional comments (31)
pkg/connector/slash_commands_limits.go (1)
59-62: LGTM!Also applies to: 141-160, 245-250
README.md (1)
377-384: LGTM!Also applies to: 391-394, 525-525
pkg/ai/providers/openai_stream_test.go (1)
170-215: LGTM!Also applies to: 269-279
pkg/agent/agent_loop.go (1)
598-605: LGTM!pkg/agent/agent_loop_test.go (1)
331-334: LGTM!pkg/connector/client.go (1)
280-280: LGTM!Also applies to: 391-391, 1200-1200, 1253-1257, 1678-1685, 1756-1756, 2433-2433
pkg/connector/stream_test.go (1)
592-636: LGTM!Also applies to: 638-680, 855-896, 991-993
pkg/ai/types.go (1)
251-269: LGTM!pkg/ai/providers/citations.go (1)
1-268: LGTM!pkg/ai/providers/google.go (1)
100-104: LGTM!pkg/ai/providers/openai_conversion_test.go (1)
120-212: LGTM!pkg/aiid/metadata.go (1)
22-24: LGTM!pkg/ai/providers/anthropic.go (1)
22-23: LGTM!Also applies to: 132-136, 389-391, 553-557, 561-567, 590-603, 616-636, 638-651, 683-691, 720-841
pkg/ai/providers/google_vertex.go (1)
110-114: LGTM!pkg/ai/providers/openai_completions.go (1)
208-210: LGTM!pkg/ai/providers/openai_responses.go (1)
180-180: LGTM!pkg/ai/providers/anthropic_test.go (1)
4-4: LGTM!Also applies to: 40-102, 104-152, 165-174
pkg/connector/slash_commands_session.go (1)
43-64: LGTM!Also applies to: 189-191
pkg/chattools/chattools.go (1)
10-15: LGTM!pkg/connector/chat_tools_test.go (1)
37-45: LGTM!pkg/connector/slash_commands.go (1)
91-106: LGTM!pkg/aiid/ids_test.go (1)
69-69: LGTM!Also applies to: 86-88
pkg/chattools/session.go (1)
19-20: LGTM!Also applies to: 26-28
pkg/connector/timezone_test.go (1)
13-44: LGTM!pkg/connector/tool_modes.go (1)
1-60: LGTM!pkg/connector/room_state.go (1)
30-31: LGTM!Also applies to: 85-94
pkg/connector/room_state_test.go (1)
31-53: LGTM!pkg/connector/slash_commands_test.go (1)
360-368: LGTM!Also applies to: 381-381, 463-507, 593-593, 619-622, 656-665, 670-672
pkg/connector/builtin_tools.go (1)
7-7: LGTM!Also applies to: 10-10, 13-13, 15-31, 36-37, 44-64, 66-90, 92-104, 106-125, 127-141, 143-154, 156-168, 177-193, 195-217
pkg/connector/builtin_tools_test.go (1)
3-7: LGTM!Also applies to: 18-18, 50-50, 59-59, 66-149, 151-212
pkg/connector/slash_commands_tools.go (1)
1-46: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/ai-stream/message.go (1)
692-702:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOpenRouter native web_search deduplication still skipped.
The
nativeOpenAIWebSearchQueryhelper only returns a query whenoutput["provider"] == "openai"(line 697), so OpenRouter passthroughs stamped withprovider: "openrouter"will not be deduplicated. The UI will continue to show duplicateweb_searchtool-call rows for OpenRouter native searches.Per the earlier review, consider either:
- Dropping the provider check entirely:
output["native"] == true && name == "web_search".- Adding
"openrouter"alongside"openai"in the guard.🤖 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 `@pkg/ai-stream/message.go` around lines 692 - 702, The helper nativeOpenAIWebSearchQuery currently only treats native web_search as dedupable when output["provider"] == "openai", skipping OpenRouter passthroughs; update the guard in nativeOpenAIWebSearchQuery so that it accepts native web_search for either provider (e.g., allow "openai" OR "openrouter") or remove the provider check entirely and only require output["native"] == true and part name == "web_search"; keep the rest of the logic (casting output/input and returning the query via firstString(output["query"], input["query"])).
🧹 Nitpick comments (1)
pkg/ai-stream/run.go (1)
306-322: ⚖️ Poor tradeoffHandle non-prefix content snapshots.
If
contentdoesn't start withprevious, the method silently does nothing (no else branch after line 319). This could drop content if the snapshot represents a non-incremental change (e.g., content was edited or truncated).Consider adding:
} else { // Content diverged; emit full replacement w.ReasoningDelta(index, content) w.reasoningContent[index] = content }🤖 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 `@pkg/ai-stream/run.go` around lines 306 - 322, ReasoningContentSnapshot currently ignores snapshots when new content doesn’t have previous as a prefix, losing non-incremental edits; update the function (ReasoningContentSnapshot) to handle the divergence by calling w.ReasoningDelta(index, content) to emit a full replacement and then update w.reasoningContent[index] = content (ensure w.initState() is still called at the top and preserve the early-return checks for empty content and equality).
🤖 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 `@pkg/connector/chat_tools.go`:
- Around line 182-206: The configuredReasoningMode/reasoningModeForModel flow
needs to normalize RoomConfig.ReasoningMode the same way validateReasoningMode
does: trim and lowercase before comparing to ""/"default"/"adaptive" so inputs
like "Default", " ADAPTIVE " are treated consistently; update
configuredReasoningMode (and/or reasoningModeForModel) to use
strings.ToLower(strings.TrimSpace(roomConfig.ReasoningMode)), return "" or
string(model.ReasoningMode) for default/empty, and map "adaptive" to
string(ai.ModelReasoningModeAdaptive) only if model.ReasoningMode supports it,
matching the validation logic (reference configuredReasoningMode,
reasoningModeForModel, validateReasoningMode, RoomConfig.ReasoningMode, and
model.ReasoningMode).
- Around line 189-191: The repository defines reasoningModeForModel and
validateReasoningMode in pkg/connector/chat_tools.go but never uses them; update
the chatTools/room-request flow to apply and validate RoomConfig.ReasoningMode
by calling cl.reasoningModeForModel(...) (or cl.validateReasoningMode(...))
where the request/room config is constructed — e.g., alongside the existing
SelectedReasoning assignment that uses cl.reasoningLevelForModel(...) — so the
chosen reasoning mode is validated and included in the outgoing request, or if
you prefer to remove unused code, delete reasoningModeForModel and
validateReasoningMode and any associated RoomConfig fields access to avoid dead
code.
---
Duplicate comments:
In `@pkg/ai-stream/message.go`:
- Around line 692-702: The helper nativeOpenAIWebSearchQuery currently only
treats native web_search as dedupable when output["provider"] == "openai",
skipping OpenRouter passthroughs; update the guard in nativeOpenAIWebSearchQuery
so that it accepts native web_search for either provider (e.g., allow "openai"
OR "openrouter") or remove the provider check entirely and only require
output["native"] == true and part name == "web_search"; keep the rest of the
logic (casting output/input and returning the query via
firstString(output["query"], input["query"])).
---
Nitpick comments:
In `@pkg/ai-stream/run.go`:
- Around line 306-322: ReasoningContentSnapshot currently ignores snapshots when
new content doesn’t have previous as a prefix, losing non-incremental edits;
update the function (ReasoningContentSnapshot) to handle the divergence by
calling w.ReasoningDelta(index, content) to emit a full replacement and then
update w.reasoningContent[index] = content (ensure w.initState() is still called
at the top and preserve the early-return checks for empty content and equality).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c00935c8-ec57-478c-8069-747f852b5f75
📒 Files selected for processing (28)
pkg/ai-stream/message.gopkg/ai-stream/run.gopkg/ai-stream/stream_test.gopkg/ai/providers/anthropic.gopkg/ai/providers/anthropic_test.gopkg/ai/providers/citations.gopkg/ai/providers/openai_codex_responses.gopkg/ai/providers/openai_codex_responses_test.gopkg/ai/providers/openai_completions.gopkg/ai/providers/openai_conversion_test.gopkg/ai/providers/openai_responses.gopkg/ai/providers/openai_shared.gopkg/ai/providers/openai_stream_test.gopkg/ai/types.gopkg/ai/utils/http_logging.gopkg/ai/utils/http_logging_test.gopkg/ai/utils/overflow.gopkg/ai/utils/overflow_test.gopkg/connector/builtin_tools.gopkg/connector/builtin_tools_test.gopkg/connector/chat_tools.gopkg/connector/client.gopkg/connector/contacts.gopkg/connector/contacts_test.gopkg/connector/room_state.gopkg/connector/sources.gopkg/connector/sources_test.gopkg/connector/stream_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/ai/providers/openai_responses.go
- pkg/connector/builtin_tools_test.go
- pkg/ai/providers/anthropic.go
- pkg/connector/stream_test.go
- pkg/ai/providers/citations.go
- pkg/ai/providers/openai_shared.go
- pkg/connector/sources_test.go
- pkg/connector/builtin_tools.go
- pkg/connector/sources.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker
- GitHub Check: build-docker
🔇 Additional comments (36)
pkg/ai/utils/overflow_test.go (1)
18-21: LGTM!pkg/ai/types.go (1)
11-11: LGTM!Also applies to: 73-73, 145-145, 255-273
pkg/connector/contacts.go (1)
448-448: LGTM!Also applies to: 530-530, 663-668
pkg/connector/room_state.go (1)
29-40: LGTM!Also applies to: 69-77, 91-100, 231-233
pkg/ai/providers/openai_stream_test.go (1)
217-347: LGTM!Also applies to: 401-411
pkg/ai/providers/openai_codex_responses.go (2)
689-689: LGTM!
675-675: ⚡ Quick winCodex now consistently sends
session-id(no remainingsession_idheader usage in codex SSE/WS); still confirm backend acceptance
BuildCodexSSEHeadersandBuildCodexWebSocketHeadersboth sendsession-id(and tests assert it);session_idonly appears elsewhere (e.g., OpenAI completions compat and non-header JSON/DB fields).- Logging supports both
session-idandsession_id, but Codex backend compatibility withsession-idcan’t be proven from this repo alone—check Codex API/docs or observe backend behavior.pkg/ai/utils/overflow.go (1)
32-32: LGTM!pkg/ai/utils/http_logging.go (1)
68-68: LGTM!pkg/connector/contacts_test.go (2)
173-173: LGTM!
215-217: LGTM!pkg/ai-stream/run.go (3)
188-188: LGTM!
237-237: LGTM!
754-756: LGTM!pkg/ai/utils/http_logging_test.go (2)
35-35: LGTM!
50-52: LGTM!pkg/connector/client.go (8)
280-280: LGTM!
391-391: LGTM!
1200-1200: LGTM!Also applies to: 1253-1257
1665-1665: LGTM!
1679-1686: LGTM!
1757-1757: LGTM!
1101-1101: LGTM!
2442-2447: LGTM!pkg/ai/providers/openai_codex_responses_test.go (1)
107-111: LGTM!pkg/ai-stream/message.go (4)
175-181: LGTM!
462-471: LGTM!
652-666: LGTM!
821-821: LGTM!pkg/ai-stream/stream_test.go (3)
327-362: LGTM!
364-387: LGTM!
389-425: LGTM!pkg/ai/providers/openai_completions.go (2)
207-210: LGTM!
539-554: LGTM!pkg/ai/providers/anthropic_test.go (1)
12-307: LGTM!pkg/ai/providers/openai_conversion_test.go (1)
101-608: LGTM!
Stream initial argument deltas and redacted thinking updates, and harden function-argument handling. anthropic: emit thinking_delta for redacted reasoning and toolcall_delta when tool arguments are present. openai_shared: properly extract function_call arguments (handles nil/non-string cases) and emit toolcall_delta if arguments exist. Tests updated to assert the presence of these delta events.
Add handling for activity snapshots and activity delta events in FinalBeeperAIMessage: track activityParts and activityContents, apply snapshots and JSON Patch-style deltas, and update message parts accordingly. Introduce helper functions (activityPartID, applyActivityContent, activityDisplay, activityTitle, cloneMap, applyJSONPatch, set/remove JSON pointer, jsonPointerParts) and include activity event types in isActivityEventType. Add unit test TestFinalBeeperAIMessagePreservesActivitySnapshots to verify snapshot+delta application.
batuhan
changed the title
Multiple changes to improve AI streaming, provider citation handling, fetch fallbacks, source extraction, and connector behavior. Highlights: - Dockerfile: add tzdata to runtime image. - ai-stream: introduce message ordinal offsets, deterministic text/reasoning message IDs, HasTextContent(), fix reasoning snapshot logic and ordinal discovery across existing events to avoid ID collisions. - Providers: dedupe citations, prefer nested/rawType when present, add intPointerKey helper, accept additional web search/raw result types, and adjust OpenAI completions headers (x-client-request-id placement). - openai_shared: better native tool keying and handling when item IDs are missing so native tool outputs are tracked reliably. - chattools/fetch: refactor direct vs tool fetch ordering and fallback logic (directFetchResult helper, isHTTPSuccess), adjust content mapping and truncation behavior, preserve markdown/plain text, improve URL extraction and trimming to keep parenthesized URLs. - search/types: make freshness optional pointer and handle nil freshness in payload/params. - connector: export toolresult sources to stream metadata, use HasTextContent() to decide final text fallback, adjust OpenRouter route mapping to default to openai-completions unless responses explicitly requested, and expand source parsing (sourceSlice accepts []map[string]any). - slash commands: improve limits formatting and only include reported sections, with helper to append sections conditionally. - Tests: update and add tests across packages to reflect behavior changes (fetch fallbacks, citation mapping, streaming/tool interactions, limits output, sources parsing). These changes make stream/message ID assignment more robust, prevent duplicate citation/source entries, improve fetch reliability and HTML/markdown handling, and add better tooling metadata propagation.
Switch AI Services handling from a fixed proxy path to a generic base URL derived from the user's homeserver and support AI Services "runtime" metadata. Key changes:
- Replace hardcoded proxy path usage and defaultAIServicesProxyPath with base URL semantics across connector code (defaultAIServicesBaseURL, aiServices*URL functions now accept baseURL). Removed trimAIProxyProviderPath and updated URL joining logic.
- Support AI Services catalog runtime payloads: parse runtime.{provider,model,api,baseUrl} and apply runtime compat into model. Added aiServicesModelCompat struct and helpers to map compat flags into model. Introduced aiServicesRuntimeBaseURL to build runtime-specific base URLs.
- Update model/avatar/provider logic to prefer runtime_model and runtime base URLs; adjust related helpers and tests to the new runtime format.
- Fix Anthropics message conversion to replay empty tool input as an object (avoid nil input) and add a unit test for it.
- Refactor and clarify user-facing slash-command messages about model/reasoning names and modes.
- Update README text to reflect AI Services base URL behavior.
Tests updated to match new runtime schema and updated URL expectations.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/sources.go (1)
424-463:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim markdown wrappers after extracted URLs.
The new regex will also capture trailing markdown delimiters, but
trimExtractedURLonly removes sentence punctuation plus unmatched)/]. Inputs likeSee `https://example.com`orhttps://example.com**will be stored as distinct bogus source URLs instead of the canonical link.🤖 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 `@pkg/connector/sources.go` around lines 424 - 463, trimExtractedURL currently only strips sentence punctuation and unmatched closing brackets, so markdown wrappers (backticks, asterisks, underscores, tildes, angle brackets) remain and produce bogus URLs; update trimExtractedURL to iteratively strip surrounding markdown delimiter characters/pairs from both ends (e.g., `, *, **, _, __, ~, ~~`, and surrounding <>), while preserving the existing unmatched-paren/bracket logic, then fall back to the existing punctuation trimming; reference trimExtractedURL (called from extractMessageURLs) and ensure normalized URLs still get passed to normalizeSourceURL unchanged except for these wrapper removals.
🤖 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 `@pkg/connector/client.go`:
- Around line 1261-1266: The branch that handles evt.Type == "toolresult" and
re-emits the tool call (creating toolOutputEvent and calling
streamSources.addToolOutput + writer.Custom) duplicates events already sent via
active.publishToolOutput(...). Remove or short-circuit this branch so we do not
re-emit tool results: either delete the evt.Type == "toolresult" handling
entirely or add a guard (e.g., check streamSources/active state for an existing
tool output by evt.ToolCall.ID) and only call
streamSources.addToolOutput/writer.Custom when the tool output has not already
been published; reference evt.Type, evt.ToolCall, toolOutputEvent,
streamSources.addToolOutput, writer.Custom and active.publishToolOutput to
locate the relevant code.
In `@pkg/connector/contacts.go`:
- Around line 383-389: The current logic replaces a non-default provider's
configured Models with the shared catalog by assigning catalogProvider :=
cl.Main.defaultProviderConfig(...) which discards provider.Models; instead,
preserve the provider's configured models and merge in catalog metadata for
matching model IDs (and add new catalog-only entries only if desired). Update
the code paths that set catalogProvider and later assign provider.Models (see
identifiers: catalogProvider, provider, provider.Models,
cl.Main.defaultProviderConfig, ResolveProvider) so you load the shared catalog
metadata and then: iterate provider.Models and enrich each model with catalog
fields when IDs match, and only append catalog entries that do not conflict if
you want them available; apply the same merge fix for the similar block around
the 418-443 region.
In `@pkg/connector/provider_routes.go`:
- Around line 99-102: The current call after loginForProviderRequest uses
providerErrorStatus which can turn invalid login_id input into a 500; instead
detect login-resolution/validation errors and return HTTP 400. Update the error
handling after loginForProviderRequest (in provider_routes.go) to check the
error returned (e.g., using errors.Is or type assertion against the provisioning
validation error like provisioning.ErrInvalidLoginID or whatever sentinel/type
your loginForProviderRequest returns) and call writeProviderError(w,
http.StatusBadRequest, err) for those cases; otherwise fall back to the existing
writeProviderError(w, providerErrorStatus(err), err) path.
In `@pkg/connector/room_state.go`:
- Around line 139-149: ResolveProvider currently falls back to returning the
original modelID even when resolveProviderModelID fails, which bypasses the
tightened allowlist; update ResolveProvider (the function that calls
resolveProviderModelID) to reject unresolvable model IDs by returning an error
instead of "return provider, modelID, nil" when resolveProviderModelID returns
ok==false, matching the behavior of providerAllowsModel; ensure you reference
resolveProviderModelID and providerAllowsModel so callers receive an error for
disallowed model IDs rather than silently accepting them.
---
Outside diff comments:
In `@pkg/connector/sources.go`:
- Around line 424-463: trimExtractedURL currently only strips sentence
punctuation and unmatched closing brackets, so markdown wrappers (backticks,
asterisks, underscores, tildes, angle brackets) remain and produce bogus URLs;
update trimExtractedURL to iteratively strip surrounding markdown delimiter
characters/pairs from both ends (e.g., `, *, **, _, __, ~, ~~`, and surrounding
<>), while preserving the existing unmatched-paren/bracket logic, then fall back
to the existing punctuation trimming; reference trimExtractedURL (called from
extractMessageURLs) and ensure normalized URLs still get passed to
normalizeSourceURL unchanged except for these wrapper removals.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00e4ede6-b301-4b95-8b71-09acff13b5c0
📒 Files selected for processing (58)
DockerfileREADME.mdcmd/generate-models-go/main.gocmd/generate-models-go/main_test.gopkg/ai-stream/message.gopkg/ai-stream/run.gopkg/ai-stream/stream_test.gopkg/ai/image_models.gopkg/ai/image_models_generated.gopkg/ai/images_test.gopkg/ai/modelcatalog/catalog.gopkg/ai/modelcatalog/catalog_test.gopkg/ai/modelcatalog/sources.gopkg/ai/models.gopkg/ai/models_generated.gopkg/ai/models_test.gopkg/ai/providers/anthropic.gopkg/ai/providers/anthropic_test.gopkg/ai/providers/citations.gopkg/ai/providers/images/register_builtins_test.gopkg/ai/providers/openai_completions.gopkg/ai/providers/openai_conversion_test.gopkg/ai/providers/openai_shared.gopkg/ai/providers/openai_stream_test.gopkg/chattools/chattools_test.gopkg/chattools/fetch.gopkg/chattools/search.gopkg/chattools/types.gopkg/connector/approvals.gopkg/connector/bridge_commands.gopkg/connector/chat_tools.gopkg/connector/chat_tools_test.gopkg/connector/chatgpt_device_login.gopkg/connector/client.gopkg/connector/compaction.gopkg/connector/config.gopkg/connector/connector.gopkg/connector/contacts.gopkg/connector/contacts_test.gopkg/connector/login.gopkg/connector/model_avatar.gopkg/connector/model_avatar_test.gopkg/connector/provider.gopkg/connector/provider_lifecycle.gopkg/connector/provider_routes.gopkg/connector/provider_test.gopkg/connector/room_state.gopkg/connector/room_state_events.gopkg/connector/room_state_test.gopkg/connector/slash_commands.gopkg/connector/slash_commands_limits.gopkg/connector/slash_commands_model.gopkg/connector/slash_commands_state.gopkg/connector/slash_commands_test.gopkg/connector/sources.gopkg/connector/sources_test.gopkg/connector/stream_test.goscripts/generate-image-models-go.mjs
💤 Files with no reviewable changes (13)
- pkg/ai/providers/images/register_builtins_test.go
- pkg/connector/chatgpt_device_login.go
- pkg/ai/image_models_generated.go
- pkg/ai/modelcatalog/sources.go
- cmd/generate-models-go/main_test.go
- pkg/ai/modelcatalog/catalog.go
- pkg/ai/image_models.go
- scripts/generate-image-models-go.mjs
- pkg/ai/modelcatalog/catalog_test.go
- cmd/generate-models-go/main.go
- pkg/ai/models_test.go
- pkg/connector/config.go
- pkg/ai/images_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/connector/slash_commands.go
- pkg/ai-stream/stream_test.go
- pkg/ai/providers/anthropic.go
- pkg/connector/sources_test.go
- pkg/ai/providers/openai_completions.go
- pkg/connector/chat_tools.go
- pkg/ai/providers/citations.go
- pkg/chattools/types.go
- pkg/chattools/fetch.go
- pkg/chattools/search.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker
- GitHub Check: build-docker
🔇 Additional comments (22)
Dockerfile (1)
29-29: LGTM!pkg/connector/approvals.go (1)
326-327: LGTM!Also applies to: 331-331
pkg/connector/compaction.go (1)
74-76: LGTM!pkg/ai-stream/run.go (1)
7-7: LGTM!Also applies to: 187-193, 234-249, 277-283, 705-710, 717-717, 786-860
pkg/connector/login.go (1)
22-23: LGTM!Also applies to: 48-56, 73-76, 158-158, 176-193, 275-275, 283-292, 301-306, 343-353
pkg/ai/providers/anthropic_test.go (1)
6-6: LGTM!Also applies to: 70-138, 140-375
pkg/connector/stream_test.go (1)
77-119: LGTM!Also applies to: 249-346, 904-918, 999-1142, 1225-1261
pkg/ai/models.go (1)
47-47: LGTM!Also applies to: 56-56, 62-62, 85-92
pkg/ai-stream/message.go (1)
315-317: LGTM!Also applies to: 605-635, 729-729, 881-883, 941-1062
pkg/ai/providers/openai_shared.go (3)
133-133: LGTM!Also applies to: 162-163, 175-175
752-857: LGTM!Also applies to: 858-866, 1000-1042, 1046-1047, 1053-1058, 1067-1142, 1145-1257, 1260-1260, 1269-1300, 1305-1308, 1310-1310, 1325-1332, 1336-1340, 1343-1346, 1351-1355, 1357-1359, 1362-1366, 1368-1373
949-953: LGTM!pkg/connector/slash_commands_test.go (1)
35-36: LGTM!Also applies to: 192-239, 432-437, 475-475, 503-503, 549-549, 614-618, 623-623, 629-629, 646-646, 656-656
pkg/ai/providers/openai_stream_test.go (1)
170-311: LGTM!Also applies to: 360-415, 698-707
README.md (1)
304-304: LGTM!Also applies to: 320-325, 328-329, 415-417, 465-465, 532-533, 541-542
pkg/chattools/chattools_test.go (1)
168-174: LGTM!Also applies to: 191-192, 211-211, 218-218, 271-289
pkg/connector/connector.go (1)
90-90: LGTM!Also applies to: 101-127
pkg/connector/model_avatar.go (1)
139-169: LGTM!Also applies to: 190-190
pkg/connector/bridge_commands.go (1)
42-42: LGTM!Also applies to: 226-234
pkg/connector/chat_tools_test.go (1)
38-38: LGTM!pkg/connector/model_avatar_test.go (1)
24-24: LGTM!Also applies to: 73-73, 97-97, 105-105
pkg/connector/provider_lifecycle.go (1)
45-50: LGTM!Also applies to: 52-130
Prevent duplicate tool-result source emission and improve provider model handling. - Avoid re-emitting sources for toolresult events by adding hasPriorToolResult and checking it in streamPublisherWithEndFrom. - Replace shallow maps.Clone usage with deep cloning (clonePayloadMap -> cloneAnyMap/cloneAny) to avoid shared mutable payloads when building built-in tools. - Add mergeProviderCatalogModels and mergeProviderCatalogModel to combine configured provider models with catalog metadata (preserve custom configured models, inherit catalog metadata for matching models). - Change provider resolution: look up provider from Connector.providersForLogin, load catalog models, default model selection from provider, and return an error when a configured model is not offered by the provider. - Improve provider routes error handling by introducing errInvalidProviderLoginID and providerRequestErrorStatus to map login lookup failures to HTTP 400 where appropriate. - Update README security note to explicitly state the direct fetch path bypasses AI-services and has no SSRF guard and recommend upstream deny-lists or network policies. - Add unit tests covering canonical model resolution, catalog merge behavior, provider validation, and duplicate toolresult source suppression, plus a helper for counting source custom events. Also includes minor import/cleanup adjustments.
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
1 participant
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.
No description provided.