Skip to content

fix(anthropicapi): carry top_p/user in typed fields + architecture-review quick wins#472

Merged
SantiagoDePolonia merged 4 commits into
mainfrom
chore/architecture
Jul 4, 2026
Merged

fix(anthropicapi): carry top_p/user in typed fields + architecture-review quick wins#472
SantiagoDePolonia merged 4 commits into
mainfrom
chore/architecture

Conversation

@SantiagoDePolonia

@SantiagoDePolonia SantiagoDePolonia commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Summary

First batch of fixes from a full architecture review (the review itself is included at docs/dev/2026-07-04_architecture-review.md).

Bug fix

  • /v1/messages ingress wrote top_p and metadata.user_id into ExtraFields even though core.ChatRequest has typed TopP/User fields. Wire output was identical, but internal consumers of the typed fields (Responses-via-chat lowering, the compatible provider's user copy) saw zero values for Anthropic-dialect traffic, and the two-channel split armed the duplicate-JSON-key hazard in the extras merge. They now land in the typed fields; stop stays in ExtraFields (no typed equivalent).

User-visible impact

  • None on the wire: typed fields marshal to the same top_p/user keys for OpenAI-family providers, and the Anthropic egress adapter already preferred the typed TopP (it ignores a user extra either way). Requests that lower /v1/messages into the Responses path now correctly carry user.

Cleanups

Docs

  • Full architecture review with file:line evidence and a sequenced roadmap.
  • CLAUDE.md guardrails claim corrected: definitions are persisted in the guardrail_definitions store with admin CRUD; config.yaml only seeds it.
  • .gitattributes marks the checked-in swagger artifacts linguist-generated so the 8k-line docs.go collapses in review diffs.

Testing

  • go build ./..., full go test ./..., and golangci-lint (0 issues) pass; pre-commit make test-race ran on each commit.
  • TestToChatRequestExtraFields updated to pin the typed-field behavior and assert top_p/user no longer ride in ExtraFields.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of request metadata so supported fields are now set correctly in outgoing chat requests.
    • Standardized client request ID validation across providers, enforcing a consistent length and character limit.
    • Removed an obsolete cache type option from the product’s supported set.
  • Documentation

    • Updated guardrail guidance to reflect the current startup behavior and feature gate.
    • Added a new architecture review document and marked a completed refactoring item as done.

SantiagoDePolonia and others added 3 commits July 4, 2026 17:51
/v1/messages ingress wrote top_p and metadata.user_id into ExtraFields even
though core.ChatRequest has typed TopP and User fields. Wire output was
identical, but internal consumers of the typed fields (Responses lowering,
compatible-provider user copy) saw zero values for Anthropic-dialect traffic,
and the split armed the duplicate-JSON-key hazard in the extras merge.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- providers.IsValidClientRequestID replaces three verbatim copies in the
  openai, openrouter, and azure packages; its table test moves next to it.
- Remove dead responsecache.CacheTypeBoth (possible-refactoring #1) and the
  unreferenced circuitBreaker.Allow().
- Rename gateway/refactor_findings_test.go to edge_cases_test.go so the file
  is named for its content, not the review session that produced it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed files

- docs/dev/2026-07-04_architecture-review.md: full findings with file:line
  evidence, blast-radius analysis, and a sequenced refactoring roadmap.
- CLAUDE.md: guardrail definitions are persisted in the guardrail_definitions
  store with admin CRUD; config.yaml only seeds it (was described as
  config-only).
- .gitattributes: mark checked-in swagger artifacts linguist-generated so the
  8k-line docs.go collapses in review diffs.
- possible-refactoring.md: mark item 1 (CacheTypeBoth) done.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mintlify

mintlify Bot commented Jul 4, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
gomodel 🟢 Ready View Preview Jul 4, 2026, 3:54 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@SantiagoDePolonia, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 36 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: eef7b8da-c126-426c-8275-923beeee8490

📥 Commits

Reviewing files that changed from the base of the PR and between b9afb74 and 1eabb3f.

📒 Files selected for processing (2)
  • internal/providers/auth_headers.go
  • internal/providers/auth_headers_test.go
📝 Walkthrough

Walkthrough

This PR consolidates client request-ID validation into a shared providers.IsValidClientRequestID helper used by azure, openai, and openrouter, propagates top_p/user into typed core.ChatRequest fields in Anthropic request translation, removes dead code (circuitBreaker.Allow, CacheTypeBoth), and adds documentation.

Changes

Provider Request-ID Validation

Layer / File(s) Summary
Shared validator and tests
internal/providers/auth_headers.go, internal/providers/auth_headers_test.go
Adds exported IsValidClientRequestID (max 512 chars, ASCII-only) with unit tests covering valid/invalid cases.
Provider wiring
internal/providers/azure/azure.go, internal/providers/openai/openai.go, internal/providers/openai/openai_test.go, internal/providers/openrouter/openrouter.go
Switches setHeaders in azure, openai, and openrouter to call the shared validator, removing local duplicate implementations and the redundant openai test.

Anthropic Request Typed Fields

Layer / File(s) Summary
ToChatRequest and buildExtraFields
internal/anthropicapi/request.go, internal/anthropicapi/request_test.go
Sets chat.TopP and chat.User from typed fields instead of ExtraFields, refactors buildExtraFields stop-field handling, and updates test assertions.

Dead Code Removal

Layer / File(s) Summary
Circuit breaker and cache constant
internal/llmclient/circuit_breaker.go, internal/responsecache/semantic.go, docs/dev/possible-refactoring.md
Removes unused Allow() method and CacheTypeBoth constant, marking the refactoring item as done.

Documentation

Layer / File(s) Summary
Architecture review and misc docs
docs/dev/2026-07-04_architecture-review.md, CLAUDE.md, .gitattributes
Adds a new architecture-review document, updates guardrails documentation, and marks generated docs as linguist-generated.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Provider as "azure/openai/openrouter setHeaders"
  participant Shared as "providers.IsValidClientRequestID"
  Provider->>Shared: ValidateRequestID(requestID)
  Shared-->>Provider: valid bool
Loading

Possibly related PRs

  • ENTERPILOT/GoModel#200: Both PRs touch internal/responsecache/semantic.go's cache-type definitions/usage.
  • ENTERPILOT/GoModel#202: Both PRs touch internal/responsecache caching behavior related to cache-type constants.
  • ENTERPILOT/GoModel#361: Both PRs refactor provider request-ID/auth handling across azure, openai, and openrouter setHeaders.

Poem

A rabbit hopped through code so neat,
Trimmed the fat from stale, dead feat.
One helper now checks IDs true,
Top_p and user get typed homes too.
With docs in tow and old bits gone —
🐇 thump-thump, the refactor's on!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main Anthropic request-field fix and the additional cleanup/documentation work.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/architecture

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@greptile-apps

greptile-apps Bot commented Jul 4, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

Safe to merge with low risk.

The functional changes are small, covered by focused tests, and preserve existing wire behavior while correcting typed-field propagation.

No files require special attention.

T-Rex T-Rex Logs

What T-Rex did

  • Ran the general contract-validation tests for the anthropic API and providers to verify core test coverage.
  • Reviewed the anthropic API test log to confirm TestToChatRequestExtraFields ran, passed, and reported exit code 0.
  • Reviewed the providers test log to confirm TestIsValidClientRequestID ran and all subtests passed with the package PASS.

View all artifacts

T-Rex Ran code and verified through T-Rex

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant AnthropicIngress as /v1/messages ingress
participant ToChat as anthropicapi.ToChatRequest
participant Core as core.ChatRequest
participant Gateway as standard chat pipeline
participant Provider as provider adapter

Client->>AnthropicIngress: "Anthropic Messages request<br/>top_p, metadata.user_id, stop_sequences"
AnthropicIngress->>ToChat: Decode + normalize request
ToChat->>Core: Set typed TopP and User fields
ToChat->>Core: Preserve stop_sequences in ExtraFields["stop"]
Core->>Gateway: Route canonical chat request
Gateway->>Provider: Provider reads typed fields / extras
Provider-->>Client: Compatible provider response
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client
participant AnthropicIngress as /v1/messages ingress
participant ToChat as anthropicapi.ToChatRequest
participant Core as core.ChatRequest
participant Gateway as standard chat pipeline
participant Provider as provider adapter

Client->>AnthropicIngress: "Anthropic Messages request<br/>top_p, metadata.user_id, stop_sequences"
AnthropicIngress->>ToChat: Decode + normalize request
ToChat->>Core: Set typed TopP and User fields
ToChat->>Core: Preserve stop_sequences in ExtraFields["stop"]
Core->>Gateway: Route canonical chat request
Gateway->>Provider: Provider reads typed fields / extras
Provider-->>Client: Compatible provider response
Loading

Reviews (1): Last reviewed commit: "docs: add architecture review, correct g..." | Re-trigger Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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 `@docs/dev/2026-07-04_architecture-review.md`:
- Around line 142-149: The architecture review note still presents the Anthropic
typed-field gap as unresolved, but this branch already fixes it, so update the
affected finding around the Anthropic request handling and any related follow-up
sections to reflect current branch state. Either remove the stale issue or
rewrite it as historical context, and make sure references to
`anthropicapi/request.go`, `core.ChatRequest.TopP`, `core.ChatRequest.User`,
`providers/responses_adapter.go`, and `providers/openai/compatible_provider.go`
no longer imply an open bug.

In `@internal/providers/auth_headers_test.go`:
- Line 6: Add table-driven test coverage for embedded control characters in the
auth header value handling, since the current cases only verify length and
multi-byte/emoji rejection. Update the relevant tests in the auth header test
suite to include inputs such as a CRLF-injection string and a NUL-byte string,
and assert they are rejected by the validation logic used in auth_headers.go.
Keep the new cases alongside the existing boundary tests so the mismatch between
the doc comment and implementation is covered.

In `@internal/providers/auth_headers.go`:
- Around line 29-42: The IsValidClientRequestID helper currently only excludes
bytes above 127, so it still accepts control characters and DEL instead of
matching the printable ASCII contract. Update the validation in
IsValidClientRequestID to allow only the printable range used for forwarded
request IDs (0x20 through 0x7E), while keeping the existing length check and
using the same loop over id bytes.
🪄 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: ASSERTIVE

Plan: Pro

Run ID: 67d8d133-8847-40aa-ab48-c14988240e5e

📥 Commits

Reviewing files that changed from the base of the PR and between 5b885dd and b9afb74.

📒 Files selected for processing (15)
  • .gitattributes
  • CLAUDE.md
  • docs/dev/2026-07-04_architecture-review.md
  • docs/dev/possible-refactoring.md
  • internal/anthropicapi/request.go
  • internal/anthropicapi/request_test.go
  • internal/gateway/edge_cases_test.go
  • internal/llmclient/circuit_breaker.go
  • internal/providers/auth_headers.go
  • internal/providers/auth_headers_test.go
  • internal/providers/azure/azure.go
  • internal/providers/openai/openai.go
  • internal/providers/openai/openai_test.go
  • internal/providers/openrouter/openrouter.go
  • internal/responsecache/semantic.go
💤 Files with no reviewable changes (3)
  • internal/responsecache/semantic.go
  • internal/providers/openai/openai_test.go
  • internal/llmclient/circuit_breaker.go

Comment on lines +142 to +149
**[BUG] `/v1/messages` ingress bypasses typed IR fields.**
`anthropicapi/request.go:443–460` writes `top_p` and `user` into `ExtraFields`
even though `core.ChatRequest` has typed `TopP` (`types.go:26`) and `User`
(`types.go:37`). Wire output is correct (merged at marshal), but internal readers
of the typed fields see zero values — `providers/responses_adapter.go:46` and
`providers/openai/compatible_provider.go:330` copy `req.User`, and any future
consumer of `req.TopP` silently misses Anthropic-dialect traffic. One field, two
channels, chosen per-ingress.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Refresh findings that this branch already resolves.

These sections still describe the Anthropic typed-field gap and the CacheTypeBoth / circuitBreaker.Allow() removals as open work, but the PR stack already lands those fixes. Please convert them to historical notes or remove them so the document matches current branch state.

Also applies to: 501-507, 520-522

🤖 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 `@docs/dev/2026-07-04_architecture-review.md` around lines 142 - 149, The
architecture review note still presents the Anthropic typed-field gap as
unresolved, but this branch already fixes it, so update the affected finding
around the Anthropic request handling and any related follow-up sections to
reflect current branch state. Either remove the stale issue or rewrite it as
historical context, and make sure references to `anthropicapi/request.go`,
`core.ChatRequest.TopP`, `core.ChatRequest.User`,
`providers/responses_adapter.go`, and `providers/openai/compatible_provider.go`
no longer imply an open bug.

Comment thread internal/providers/auth_headers_test.go
Comment thread internal/providers/auth_headers.go
…tion

IsValidClientRequestID documented a printable-ASCII contract but accepted any
byte <= 127, letting control characters and DEL through; a control byte in a
header value makes Go's transport reject the whole outbound request. Restrict
to 0x20-0x7E and cover control/NUL/tab/DEL cases in the table test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants