fix: return file bytes from GET /v1/files/{file_id}/content on the anthropic surface - #5706
Open
brianhadley2026 wants to merge 3 commits into
Open
fix: return file bytes from GET /v1/files/{file_id}/content on the anthropic surface#5706brianhadley2026 wants to merge 3 commits into
brianhadley2026 wants to merge 3 commits into
Conversation
…iew maintenance (maximhq#5693) * feat: support matview_refresh_interval "off" to disable logstore matview maintenance The materialized views back only the dashboard UI. Deployments that run Bifrost headless behind their own observability stack pay the REFRESH MATERIALIZED VIEW CONCURRENTLY cost for views nothing reads, and the 5s floor means the interval alone cannot turn maintenance off. With "off" (or a non-positive duration) the logs store skips view creation, the initial refresh, and the periodic refresher entirely. matViewsReady stays false, so dashboard queries fall back to the raw tables, and the runtime self-heal path cannot re-arm maintenance since it only triggers from matview-path queries. * fix: guard matview self-heal when maintenance is disabled Review follow-up: carry the resolved disabled state onto the store so triggerMatViewSelfHeal cannot recreate views the configuration says must not exist, and make the schema/docs explicit that a zero duration also disables (positive sub-5s values still clamp up).
…thropic surface
The anthropic files route config mounted the metadata-retrieve handler on the
/content path: it instantiated AnthropicFileRetrieveRequest and dispatched
schemas.FileRetrieveRequest, so a download request returned the file metadata
JSON with HTTP 200 instead of the file bytes. AnthropicProvider.FileContent and
the generic router's FileContentRequest byte-writing path already work; they
were simply never reached.
- Re-mount the existing retrieve logic on its intended path,
GET /v1/files/{file_id} (previously unrouted).
- Add a content route for GET /v1/files/{file_id}/content that dispatches
schemas.FileContentRequest; the router's default binary path writes the
bytes with the upstream content-type.
Repro (before): upload any file via POST /anthropic/v1/files, then
GET /anthropic/v1/files/{id}/content -> 200 application/json metadata,
while the same request direct to api.anthropic.com returns the file bytes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesMaterialized-view maintenance
Anthropic file route separation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Config as matview_refresh_interval
participant Store as newPostgresLogStore
participant Refresher as startMatViewRefresher
participant SelfHeal as triggerMatViewSelfHeal
Config->>Store: resolve "off" or non-positive duration
Store->>Refresher: skip startup when refreshInterval <= 0
Store->>SelfHeal: set matViewMaintenanceDisabled
SelfHeal-->>Store: return without scheduling self-heal
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
requested review from
Pratham-Mishra04,
akshaydeo,
impoiler and
roroghost17
July 30, 2026 22:24
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
transports/bifrost-http/integrations/anthropic.go (1)
1037-1040: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table-driven regression coverage for the route split.
Extend
transports/bifrost-http/integrations/anthropic_test.goto verify metadata retrieval, byte-identical content responses with upstreamContent-Type, upstream error conversion, and Geminifiles-normalization.As per coding guidelines, Go behavior changes should have deterministic, table-driven coverage.
Also applies to: 1074-1107
🤖 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 `@transports/bifrost-http/integrations/anthropic.go` around lines 1037 - 1040, Extend the tests around the Anthropic file routes in anthropic_test.go with deterministic table-driven cases covering metadata retrieval, byte-identical content bodies while preserving the upstream Content-Type, upstream error conversion, and normalization of Gemini file identifiers beginning with “files-”. Reuse the existing HTTP test helpers and assert each route’s response status, headers, and body.Source: Coding guidelines
🤖 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/deployment-guides/config-json/storage.mdx`:
- Around line 280-282: Update the matview_refresh_interval description in the
configuration table to state that any non-positive duration disables
materialized view maintenance, including an example such as "-1m", while
retaining the existing "off" and "0s" examples and positive-duration behavior.
---
Nitpick comments:
In `@transports/bifrost-http/integrations/anthropic.go`:
- Around line 1037-1040: Extend the tests around the Anthropic file routes in
anthropic_test.go with deterministic table-driven cases covering metadata
retrieval, byte-identical content bodies while preserving the upstream
Content-Type, upstream error conversion, and normalization of Gemini file
identifiers beginning with “files-”. Reuse the existing HTTP test helpers and
assert each route’s response status, headers, and body.
🪄 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: 401f469f-f648-4691-8eff-efadfdce1941
📒 Files selected for processing (8)
docs/deployment-guides/config-json/storage.mdxframework/logstore/matviewheal.goframework/logstore/matviews.goframework/logstore/matviews_lock_test.goframework/logstore/postgres.goframework/logstore/rdb.gotransports/bifrost-http/integrations/anthropic.gotransports/config.schema.json
Comment on lines
280
to
+282
| | Field | Default | Description | | ||
| |-------|---------|-------------| | ||
| | `matview_refresh_interval` | `"1m"` | How often to refresh dashboard materialized views. Accepts any Go duration string (`"1m"`, `"5m"`, `"1h"`). Minimum `5s`. | | ||
| | `matview_refresh_interval` | `"1m"` | How often to refresh dashboard materialized views. Accepts any Go duration string (`"1m"`, `"5m"`, `"1h"`); positive values below `5s` are clamped up to `5s`. Set `"off"` or a zero duration (`"0s"`) to disable matview maintenance entirely. | |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document all non-positive disable values.
The runtime treats any non-positive duration as disabled, but this page only documents "0s". Update the description to say "off" or any non-positive duration, with "-1m" as an example, so config.json guidance matches the implementation.
🤖 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/deployment-guides/config-json/storage.mdx` around lines 280 - 282,
Update the matview_refresh_interval description in the configuration table to
state that any non-positive duration disables materialized view maintenance,
including an example such as "-1m", while retaining the existing "off" and "0s"
examples and positive-duration behavior.
Source: Path instructions
…gelog entry
Regression tests for the /content route fix: the content route must dispatch
FileContentRequest with a populated ContentRequest (never RetrieveRequest),
the retrieve handler lives on GET /v1/files/{file_id}, and the Gemini
files- -> files/ id translation holds on the content path.
Affected packages:
- transports/bifrost-http/integrations (test only)
- transports/changelog.md
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
akshaydeo
dismissed
coderabbitai[bot]’s stale review
August 5, 2026 07:18
The merge-base changed after approval.
akshaydeo
force-pushed
the
dev
branch
2 times, most recently
from
August 10, 2026 22:20
d025e9c to
d704af4
Compare
akshaydeo
force-pushed
the
dev
branch
3 times, most recently
from
August 19, 2026 08:15
1eaa684 to
2ed4dd9
Compare
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
3 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
On the anthropic surface,
GET /v1/files/{file_id}/contentreturns the file's metadata JSON with HTTP 200 instead of the file bytes. The route config mounts the metadata-retrieve handler on the/contentpath: it instantiatesAnthropicFileRetrieveRequestand dispatchesFileRequest{Type: schemas.FileRetrieveRequest, ...}(the comment above it says "Retrieve file endpoint - GET /v1/files/{file_id}"). Meanwhile the intended metadata endpoint,GET /v1/files/{file_id}, is not routed at all.Because the failure is a well-formed 200, clients can't detect it — and files generated server-side by Anthropic's code-execution/skills tools (pptx/xlsx/docx) cannot be downloaded through Bifrost at all. Observed on v1.5.11, v1.6.4, and v1.6.6.
Everything below the route config already works —
AnthropicProvider.FileContentfetches{baseURL}/v1/files/{id}/contentcorrectly, and the generic router'sschemas.FileContentRequestcase writes raw bytes with the upstream content-type. The content path was simply never dispatched.Changes
transports/bifrost-http/integrations/anthropic.go— re-mount the existing retrieve logic on its intended path,GET /v1/files/{file_id}(previously unrouted), and add a content route forGET /v1/files/{file_id}/contentthat instantiatesAnthropicFileContentRequest(already handled byextractAnthropicFileIDFromPath) and dispatchesFileRequest{Type: schemas.FileContentRequest, ContentRequest: ...}, mirroring the Bedrock S3 GetObject route. No response converter is set, so the generic router's default binary path writes the bytes with the upstream content-type. The Geminifiles-→files/id translation is preserved on both routes.transports/bifrost-http/integrations/anthropic_files_test.go— regression tests pinning both routes' dispatch: the content route must produce aContentRequest(never aRetrieveRequest), the retrieve route lives on the metadata path, and the Gemini id translation holds.transports/changelog.md— changelog entry.Type of change
Affected areas
How to test
Live verification (anthropic provider key + virtual key):
Before: HTTP 200,
application/json, metadata body. After: HTTP 200 with the file bytes — verified SHA256-identical to the direct-api.anthropic.comdownload of the same file id, with the correct OOXML content-type. A non-downloadable (user-uploaded) file now relays upstream's400 invalid_request_error "File '...' is not downloadable"verbatim instead of a silent 200.GET /v1/files/{id}returns the metadata JSON; list/upload/delete unchanged.Screenshots/Recordings
N/A (no UI changes).
Breaking changes
Callers that inadvertently relied on
/contentreturning metadata JSON should useGET /v1/files/{file_id}, which this PR routes.Related issues
Relates to #5707 (string-form
containerreuse silently dropped on the same surface — separate defect, not addressed here).Security considerations
None — the route relays bytes the caller's virtual key is already authorized to fetch; auth and governance attribution paths are unchanged.
Checklist
docs/contributing/README.mdand followed the guidelinestransports/Dockerfile)go test ./bifrost-http/integrations/)🤖 Generated with Claude Code