fix(anthropic): Curator/Tara reliability across the OAuth proxy, provider, and processors - #1117
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughWalkthroughAdds Anthropic policy, multimodal, tracing, and request-normalization changes, plus ExcelJS interop fixes, new test scripts, and a local mammoth parser patch. ChangesAnthropic runtime updates
File processing compatibility
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)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/lib/core/modules/GenerationHandler.tsParsing error: Unable to parse the specified 'tsconfig' file. Ensure it's correct and has valid syntax. error TS5012: Cannot read file '/.svelte-kit/tsconfig.json': ENOENT: no such file or directory, open '/.svelte-kit/tsconfig.json'. src/lib/core/modules/structuredOutputPolicy.tsParsing error: Unable to parse the specified 'tsconfig' file. Ensure it's correct and has valid syntax. error TS5012: Cannot read file '/.svelte-kit/tsconfig.json': ENOENT: no such file or directory, open '/.svelte-kit/tsconfig.json'. src/lib/processors/document/ExcelProcessor.tsParsing error: Unable to parse the specified 'tsconfig' file. Ensure it's correct and has valid syntax. error TS5012: Cannot read file '/.svelte-kit/tsconfig.json': ENOENT: no such file or directory, open '/.svelte-kit/tsconfig.json'.
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/proxy/oauthFetch.ts (1)
181-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated system-relocation helper.
This helper is duplicated with
src/lib/server/routes/claudeProxyRoutes.tsLine 589-Line 627. Since it encodes the OAuth anti-abuse workaround, keeping one shared implementation avoids future drift between proxy paths.🤖 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 `@src/lib/proxy/oauthFetch.ts` around lines 181 - 219, The relocateClientSystemIntoMessages helper is duplicated between oauthFetch and the Claude proxy route, so the OAuth anti-abuse workaround can drift over time. Extract this logic into a shared helper and have both call sites reuse it, keeping the relocation behavior centralized while preserving the existing relocateClientSystemIntoMessages behavior and message-wrapping semantics.
🤖 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 `@src/lib/server/routes/claudeProxyRoutes.ts`:
- Around line 4224-4248: The anti-abuse 429 check in `claudeProxyRoutes` is only
applied in the initial fetch path, but `handleAnthropicAuthRetry` still treats
matching retry 429s as real rate limits and rotates accounts. Reuse the same
`isAntiAbuseConstruction429(errRespHeaders, String(lastError))` guard inside
`handleAnthropicAuthRetry`, and when it matches, return the upstream error
response directly instead of advancing the primary account or continuing
rotation. Keep the behavior aligned with the existing construction-rejection
branch by preserving the passthrough response, logging, and tracer/error
handling.
---
Nitpick comments:
In `@src/lib/proxy/oauthFetch.ts`:
- Around line 181-219: The relocateClientSystemIntoMessages helper is duplicated
between oauthFetch and the Claude proxy route, so the OAuth anti-abuse
workaround can drift over time. Extract this logic into a shared helper and have
both call sites reuse it, keeping the relocation behavior centralized while
preserving the existing relocateClientSystemIntoMessages behavior and
message-wrapping semantics.
🪄 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
Run ID: d2f5ae2e-c114-44b8-a206-95e584d10e59
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
package.jsonpatches/mammoth@1.12.0.patchsrc/lib/core/modules/GenerationHandler.tssrc/lib/core/modules/structuredOutputPolicy.tssrc/lib/processors/document/ExcelProcessor.tssrc/lib/providers/anthropic.tssrc/lib/providers/anthropicImageBlocks.tssrc/lib/proxy/oauthFetch.tssrc/lib/proxy/proxyTracer.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/types/proxy.tstest/continuous-test-suite-anthropic-multimodal.tstest/continuous-test-suite-anthropic-tools-policy.tstest/continuous-test-suite-excel-interop.ts
6055ed9 to
92bb53b
Compare
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
92bb53b to
dc3cabb
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Code Review SummaryI've analyzed this PR which consolidates fixes for Curator/Tara reliability on the Anthropic surface. Overall, this is a well-structured PR addressing real production issues. Here are my findings: 🔒 Security: No blocking issues found
|
dc3cabb to
ffb9a7e
Compare
Review comments addressed — pushed in
|
|
Tip For best results, initiate chat on the files or code changes.
The shared extraction into 🐇✨ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
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 (1)
src/lib/proxy/oauthFetch.ts (1)
246-267: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTighten agent-block detection before stripping system content.
includes("Claude Agent SDK")can match ordinary custom instructions, causing the request to be treated as Claude Code, skipping relocation, and stripping user-provided system text. Also dedupe removal indices so one block matching both predicates is not spliced twice.Proposed fix
- const agentIdx = parsed.system.findIndex( - (b: { text?: string }) => - typeof b.text === "string" && b.text.includes("Claude Agent SDK"), - ); + const agentIdx = parsed.system.findIndex( + (b: { text?: string }) => + typeof b.text === "string" && b.text.trim() === agentBlock.text, + ); ... - const indicesToRemove = [billingIdx, agentIdx] - .filter((i) => i >= 0) - .sort((a, b) => b - a); + const indicesToRemove = [...new Set([billingIdx, agentIdx])] + .filter((i) => i >= 0) + .sort((a, b) => b - a);🤖 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 `@src/lib/proxy/oauthFetch.ts` around lines 246 - 267, The agent-detection in oauthFetch currently uses a broad text match that can incorrectly classify custom instructions as a Claude Code client. Tighten the predicate in the parsed.system scan so only the real agent identity block is recognized, and keep the relocation path for ordinary system text. Also update the removal logic in the same function to deduplicate indices before splicing so a block matching both billing and agent criteria is removed only once.
🤖 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 `@src/lib/proxy/oauthFetch.ts`:
- Around line 246-267: The agent-detection in oauthFetch currently uses a broad
text match that can incorrectly classify custom instructions as a Claude Code
client. Tighten the predicate in the parsed.system scan so only the real agent
identity block is recognized, and keep the relocation path for ordinary system
text. Also update the removal logic in the same function to deduplicate indices
before splicing so a block matching both billing and agent criteria is removed
only once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a720bfa-3241-4321-88cb-009fff2b5f26
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
package.jsonpatches/mammoth@1.12.0.patchsrc/lib/core/modules/GenerationHandler.tssrc/lib/core/modules/structuredOutputPolicy.tssrc/lib/processors/document/ExcelProcessor.tssrc/lib/providers/anthropic.tssrc/lib/providers/anthropicImageBlocks.tssrc/lib/proxy/oauthFetch.tssrc/lib/proxy/proxyTracer.tssrc/lib/proxy/systemRelocation.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/types/proxy.tstest/continuous-test-suite-anthropic-multimodal.tstest/continuous-test-suite-anthropic-tools-policy.tstest/continuous-test-suite-excel-interop.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- patches/mammoth@1.12.0.patch
- src/lib/types/proxy.ts
- test/continuous-test-suite-excel-interop.ts
- src/lib/core/modules/GenerationHandler.ts
- src/lib/processors/document/ExcelProcessor.ts
- test/continuous-test-suite-anthropic-multimodal.ts
- src/lib/proxy/proxyTracer.ts
- src/lib/core/modules/structuredOutputPolicy.ts
- package.json
- test/continuous-test-suite-anthropic-tools-policy.ts
- src/lib/providers/anthropic.ts
- src/lib/providers/anthropicImageBlocks.ts
- src/lib/server/routes/claudeProxyRoutes.ts
…ider, and processors Consolidated fixes that make Curator/Tara work reliably on the Anthropic surface (native provider, the multi-account OAuth proxy, and file processors), plus the static-analysis/tracing improvements found while debugging them. OAuth proxy (claudeProxyRoutes, oauthFetch, proxyTracer, types/proxy): - Relocate a non-Claude-Code client's `system` prompt into the message stream for the subscription/OAuth path. Anthropic rejects any unrecognised `system` with a header-less `rate_limit_error: "Error"`; keeping a custom client's system in `system[]` made every Curator request fail and the proxy misread it as a rate limit, burning all accounts over 44 retries. Now only the recognised billing+agent blocks go in `system`; the client prompt is wrapped in `<system_instructions>…</system_instructions>` as a leading user block (cache_control preserved). Genuine Claude Code traffic is detected and left untouched. - Fail fast on the anti-abuse / construction 429 (no rate-limit headers, body "Error") instead of rotating every account. - Trace request tool names and response model/finish_reason/tool_calls (gen_ai.* attributes) for both non-streaming and streaming paths, and record token usage + cost on the temperature-retry span. Native Anthropic provider (anthropic.ts, anthropicImageBlocks.ts, structuredOutputPolicy.ts, GenerationHandler.ts): - Disable structured output when tools are present on the native Anthropic Messages API (and Bedrock) — experimental_output + tools silently drops tool calls. Covered by tests for anthropic + bedrock. - Restore vision on the native surface: convert AI-SDK file parts to Anthropic image/document blocks with the correctly sniffed media type; omit unsupported image/* hints (svg, bmp) instead of relabelling them as PNG; runtime-validate file-part shape before use. - Omit temperature proactively for models that deprecate it, and retry without temperature on the deprecation error. Processors: - ExcelProcessor: fix exceljs CJS/ESM interop (Workbook under .default). - WordProcessor: patch mammoth for @xmldom/xmldom >= 0.9 (mimeType now required, errorHandler replaced by onError) so .docx extraction works with the security-pinned xmldom. Tests: register the new continuous suites (anthropic-tools-policy, anthropic-multimodal, excel-interop) in package.json and test:unit.
ffb9a7e to
2324803
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
I've reviewed the consolidated PR for Curator/Tara reliability fixes. This is a well-structured PR that addresses critical issues with the Anthropic OAuth proxy, native provider, and file processors.
Files Reviewed (16 total)
- package.json: New test scripts properly registered
- patches/mammoth@1.12.0.patch: xmldom compatibility fix
- src/lib/core/modules/GenerationHandler.ts: Temperature retry with telemetry
- src/lib/core/modules/structuredOutputPolicy.ts: Native Anthropic + Bedrock exclusion
- src/lib/processors/document/ExcelProcessor.ts: CJS/ESM interop fix
- src/lib/providers/anthropic.ts: Vision restoration, temperature handling
- src/lib/providers/anthropicImageBlocks.ts: New multimodal conversion module
- src/lib/proxy/oauthFetch.ts: System relocation for OAuth anti-abuse
- src/lib/proxy/proxyTracer.ts: Enhanced tracing (tool_names, response info)
- src/lib/proxy/systemRelocation.ts: Shared system relocation helper
- src/lib/server/routes/claudeProxyRoutes.ts: Proxy routing with anti-abuse 429 handling
- src/lib/types/proxy.ts: New ResponseInfoContext type
- test/: Three new comprehensive test suites
Verification
- ✅ CLAUDE.md Critical Rules followed (dynamic imports, type naming, barrel exports)
- ✅ No hardcoded secrets or security vulnerabilities
- ✅ Backward compatibility maintained
- ✅ Comprehensive test coverage added
- ✅ Existing review comments addressed (anti-abuse 429 in auth-retry, shared helper extraction)
Approval
This PR is ready for merge. The fixes are critical for Curator/Tara reliability and the implementation is solid.
|
🎉 This PR is included in version 9.79.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Consolidated single-commit PR with all the neurolink-side fixes that make Curator/Tara work reliably on the Anthropic surface — the native provider, the multi-account OAuth proxy, and the file processors — plus the tracing/test improvements found while debugging them.
OAuth proxy
systeminto the message stream. The subscription/OAuth path rejects any unrecognisedsystemwith a header-lessrate_limit_error: "Error". Keeping a custom client's prompt insystem[]made every Curator request fail, and the proxy misread it as a rate limit → all accounts burned over 44 retries. Nowsystemcarries only the recognised billing+agent blocks; the client prompt is wrapped in<system_instructions>…</system_instructions>as a leading user block (cache_control preserved). Genuine Claude Code traffic is detected and left untouched."Error") instead of rotating every account.gen_ai.request.tool_names,gen_ai.response.model/finish_reason/tool_calls(non-streaming and streaming),proxy.account, token usage + cost, and redacted request/response body events.Native Anthropic provider
experimental_output+ tools silently drops tool calls.image/*hints (svg, bmp) instead of relabeling as PNG; runtime-validate file-part shape.temperatureproactively for models that deprecate it; retry without it on the deprecation error (now with usage/cost telemetry on the retry span).Processors
Workbookunder.default).@xmldom/xmldom >= 0.9(mimeType now required,errorHandler→onError) so.docxextraction works with the security-pinned xmldom.Addressing #1116 review comments
anthropicImageBlocks.ts— skip unsupportedimage/*instead of relabeling as PNG (Major)toAnthropicImageBlockonly for the 4 supported types; unsupportedimage/*falls through to magic-byte salvage (omitted if not a real supported image).structuredOutputPolicy— add native-Anthropic + Bedrock test assertions (Major)isToolsSchemaExclusionInForce('anthropic',…)===trueand('bedrock',…)===trueto the policy suite (9/9 pass).GenerationHandler.ts:585— temperature-retry drops usage/cost telemetrygen_ai.usage.input_tokens/output_tokens+neurolink.costonto the retry span.anthropic.ts:449— file-part type-assertion safetymediaTypeguard before the assertion; malformed parts skip gracefully.package.jsontest:anthropic-tools-policy,test:anthropic-multimodal,test:excel-interopand added them totest:unit.Verification
tsc --noEmit -p tsconfig.cli.json: 0 errors in changed files.Summary by CodeRabbit
temperatureby retrying without it when appropriate.