Repository navigation
Conversation
…g row anthropicNativePassthrough never set logCtx.upstreamError. Every failure it answered without a stream (upstream error responses, a header timeout, a fetch failure, a body stall or byte-cap overflow, a client cancel) was logged with only a status, so /api/logs showed no reason and usage.jsonl persisted no failure diagnostics. The managed native Messages lane and the adapter lanes record a reason for the same upstream. - An upstream error response is still relayed verbatim; the row now gets "Provider error <status>: <type>: <message>" from formatAnthropicErrorBody (redacted, bounded, the same helper the managed lane uses), or "Provider error <status>" when the body is not an Anthropic error. - Local answers go through one fail() that logs the message the client receives, redacted, before finalizing.
…ields Review follow-up: - Parse an upstream error body for the log reason only within 64 KiB, the managed lane's error read bound. The passthrough body cap is 64 MB by default (unbounded at 0), and JSON.parse plus redaction over a large body would block the event loop; a larger body logs "Provider error <status>". - Only 4xx/5xx answers get a reason; a relayed 3xx is not an error row. - The unreachable-upstream test stubs the refused origin, as the existing reject-path activation test does, instead of relying on a real refusal inside cfg()'s 250 ms header deadline. - New tests pin the body-stall and client-cancel reasons, the oversized body, the 3xx case, and the 403 errorCode that a reason now drives (invalid_api_key for an authentication-shaped 403, as on other lanes). - The structure doc and comments say "the same formatAnthropicErrorBody the managed lane falls back to" instead of claiming the same format.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughNative Anthropic passthrough records bounded details for upstream error responses and local failures. It continues to relay upstream response bodies unchanged. Integration tests cover request logs, usage history, error-code classification, and failure conditions. ChangesNative Anthropic passthrough diagnostics
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds redacted, bounded failure reasons to native passthrough request logs without altering what clients receive. No actionable merge-blocking risk was identified; the author's full-suite run was still pending at the time of review. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 3 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)
✨ 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 review |
|
✅ Deterministic PR hygiene checks passed. |
✅ Action performedReview finished.
|
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
리뷰 · 우선순위 63 / 80Claude Code가 Anthropic으로 그대로 나가는 native passthrough에서, 스트림이 아닌 실패가 로그에 이유 없이 남던 구멍을 막습니다. 예전에는 상태 숫자만 있어서 라인 - 메인테이너의 판단이 필요한 지점 이유를 넣으면 너의 추천 구멍은 실측과 맞고, 클라이언트 응답은 그대로 두고 로그에만 이유를 넣는 쪽이 맞습니다. 바이트/문자 주석을 고치고 403 코드 변화를 한 번 확인한 뒤 draft를 풀면 됩니다. types.ts/config.ts 분할이나 미리보기 배포는 이 변경과 무관합니다. 이 댓글은 grok-bot이 작성했습니다 |
Maintainer review follow-up: - fail() now sends the redacted message to the client as well as the log, as the managed lane's fail() does, so a fetch error that ever carries a URL or key cannot reach the client. New test: a key in a stubbed fetch error is absent from both the response and the row. - The error-detail bound is 64 Ki characters, and the comment and doc now say so: parse cost scales with characters, and the bound is only of the order of the managed lane's 64 KiB byte read limit.
|
Thanks for the review. Both line comments are addressed in
Bytes vs. characters. The bound is characters, since 403 At |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…g row (#6330) Carries #6295 with a maintainer privacy rework: the stored reason is limited to the HTTP status plus an allowlisted Anthropic error type, so upstream message text (which can echo account identifiers or request content) never reaches the request log or usage history. Client responses are unchanged. Co-authored-by: sh940701 <visioner2168@gmail.com>
|
Landed on through maintainer carry #6330, with a Co-authored-by trailer for you. One change from this PR: review found that the upstream error message can echo account identifiers or request content, which secret redaction does not remove, so the stored reason is now limited to the HTTP status plus an allowlisted Anthropic error type. Thank you for the fix! |
2 participants
Summary
A failed native Anthropic passthrough request is logged with a status and no reason.
anthropicNativePassthrough(src/server/claude-messages.ts) never setslogCtx.upstreamError. As a result,/api/logsshows no cause for any failure it answers without a stream, and usage.jsonl persists no failure diagnostics for those rows (diagnostics are written for rows at status 400 and above). The affected failures are:The managed native Messages lane and the adapter lanes record a reason for the same upstream.
On my own install, all 378 of the 378
anthropic-nativerows at status 400 or above since 2026-09-20 carry noupstreamError: 179 × 400, 173 × 502, 12 × 429, 11 × 499, 2 × 504, 1 × 401. Over the same period, 867 of 871 failure rows from other providers have one. Nothing in the log says why those 179 Claude Code requests were rejected.Follow-up to #6235, which covered the streamed side of the same function.
Change
Content-TypeandRetry-Afterunchanged). The row now getsProvider error <status>: <type>: <message>fromformatAnthropicErrorBodyinsrc/adapters/anthropic.ts. That is the same helper the managed lane'snativeMessagesErrorResponsefalls back to; it redacts secrets and bounds the detail to 400 characters.Provider error <status>.Provider error <status>. That is the order of the managed lane's 64 KiB error read bound (MAX_NATIVE_MESSAGES_ERROR_BYTES); it counts characters because parse cost scales with string length. The passthrough body cap is 64 MB by default, or unbounded withbodyMaxBytes: 0, and parsing plus redacting a large body blocks the event loop: about 1.4 s for a 1 MB message in a local measurement.fail(). It redacts the message once and sends it to both the log and the client, as the managed lane'sfail()does. It then finalizes with the same status andcloseReasonas before. Five duplicated return blocks become one helper. The client message only changes if it contains a secret-shaped substring (maintainer review follow-up,f9953e17c).structure/clients/claude-desktop.md: documents the reasons, the 64 KiB bound, and theerrorCodeeffect below.Side effect, intended and pinned. With a reason present,
requestLogErrorCodeclassifies passthrough rows the way it already classifies the other lanes. An authentication-shaped 403 ("OAuth authentication is currently not supported.") now getserrorCode: "invalid_api_key"instead of the blanketpermission_denied. Nothing that drives account or key health readsupstreamError, and failure attribution ignores it.Privacy. Every stored reason goes through
redactSecretString, and one test pins ansk-ant-api03-…key echoed in an upstream message. Bun's fetch errors do not include the URL, query string or proxy userinfo inerr.message; checked locally for refused, userinfo-URL, DNS, bad-scheme and credentialed-proxy cases on Bun 1.4.0.Verification
Head
f9953e17condevd4480b7a4(7 behind), Bun 1.4.0 (package.json).f9953e17cadds client-side redaction infail()and its test, and rewords the bound as characters; the checks below were rerun on it unless marked9c1277236.src/server/claude-messages.tsreverted todev, 10 of the first 11 new tests fail becauseupstreamErrorisundefined: upstream 400 relayed + usage.jsonl, non-JSON 502, redacted credential, unreachable upstream, header timeout, byte-cap overflow, oversized error body, 403errorCode, body stall, client cancel. The 11th,a 3xx answer is not logged with an error reason, guards the 4xx/5xx condition; it passes ondevand failed against the first version of this change.claude-messages-endpoint.test.tsdoes, so it does not rely on a real refusal insidecfg()'s 250 ms header deadline.Bun.serveholds a streamed body's headers until the first chunk, which would trip the header deadline instead.a credential in a local failure message is redacted for the client as well as the log, failed at9c1277236: the key reached the client.bun test tests/lab/core-lab-boundary.test.ts tests/claude-integration/claude-native-passthrough.test.ts tests/claude-integration/claude-messages-endpoint.test.ts tests/claude-integration/messages-native.test.ts tests/claude-integration/messages-native-oauth.test.ts tests/claude-integration/messages-native-opaque-state.test.ts→ 163 pass, 0 fail.bun test tests/claude-integration→ 1232 pass, 1 fail.POST /v1/messages?beta=true streams an Anthropic-shaped turn end to end, a translated route this change does not touch.devand passes when its file runs alone (see fix(claude): end a passthrough stream on an upstream reset with an Anthropic error event #6235).bun x tsc --noEmit→ exit 0.bun run structure:check→ passed.bun run privacy:scan→ passed.bun run teston a Mac mini with the Codex app not running, at9c1277236): exit 1 after 560 s with 7 failures, none in Claude or Messages code.devd4480b7a4on the same machine:deferToNewerServiceRuntime(2),ocx launcher graceful shutdown(3) andturning Codex off. Thatdevrun had 23 failures in all, because the machine was under load.ocx export --json … points at the live proxy port, passes when rerun atf9953e17ctogether with the Claude files:bun test tests/cli/cli-export-command.test.ts tests/config/client-config-export.test.ts tests/claude-integration/claude-native-passthrough.test.ts tests/claude-integration/messages-native.test.ts tests/claude-integration/claude-messages-endpoint.test.ts→ 221 pass, 0 fail.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Required local validation passed; commands, results, and any full-suite exception are documented.
I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit