Repository navigation
feat(proxy): attribute usage to the CLI that spent it - #1458
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
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 |
✅ 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 |
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds per-CLI attribution for proxy usage tracking, enabling operators to see which CLI tools are consuming quota from pooled Claude accounts.
Changes Made
- New module:
src/lib/proxy/clientAttribution.ts- Extracts client names from User-Agent headers - Type updates: Added
clientAppanduserAgentfields toRequestLogEntry, addedCliClientUsageTotalstype - Ledger logic: Updated
accountLedger.tsto aggregate usage by client while maintaining account-level totals - Route integration: Three proxy routes (Claude, Codex, OpenAI bridge) now capture client attribution from headers
- Tests: Added comprehensive tests for attribution detection and ledger splitting
Impact Analysis
- Blast radius: ~9,300 nodes affected (primarily proxy accounting system)
- Affected flows: Request logging, usage aggregation, account quota tracking
- Backward compatible: Pre-attribution traffic falls into "unattributed" bucket without breaking existing dashboards
- Risk level: Low - additive feature with isolated scope to proxy accounting
Review Scope
Focused on:
- ✅ Security (no secrets or auth issues introduced)
- ✅ Architecture (follows existing factory/registry patterns)
- ✅ Type safety (proper typing throughout)
- ✅ Correctness (logic matches design intent)
- ✅ Testing (E2E tests verify live behavior)
- ✅ Backward compatibility (existing account totals preserved)
Verdict: APPROVED
No CRITICAL or MAJOR issues found. The implementation is clean, well-tested, and follows established patterns in the codebase.
GET /accounts could say an account burned $40 today. It could not say whether that was Claude Code, OpenCode, or a runaway script — every figure was per-account only, and one pooled account is routinely shared by several CLIs. The signal already existed and was thrown away. detectClientApp() derived a label from User-Agent, wrote it into an OTel span attribute, and stopped there; RequestLogEntry never carried it, so the ledger had nothing to group by. Requests now record the calling client and the raw User-Agent, and the ledger splits each account's totals into byClient alongside them. The split reconciles with the account total — summing byClient requests and cost gives back requests and costUsd, asserted in a test — so a dashboard showing both cannot contradict itself. Two decisions worth stating. The raw header is stored, not just the derived name. The classifier only knows prefixes seen in real traffic: claude-cli/ and ai/, both verified against this machine's proxy logs. Guessing prefixes for the other CLIs would be indistinguishable from working until one collided and filed a client under the wrong name. Keeping the raw header means an unclassified client is still attributable by its own User-Agent instead of collapsing into one bucket with every other unknown, and naming it later is a one-line addition. "unknown" and "unattributed" are separate buckets. The first is a client we saw and could not name; the second is a row logged before attribution existed and carries no header at all. Folding them together would make months of history look like an unidentified tool. Verified through the surface a CLI uses: real POSTs to a spawned proxy carrying real User-Agent headers, with the attribution read back out of the request log the proxy itself wrote — including the case where the classifier cannot name the caller and must preserve its header verbatim. Rebased onto current release, which had added the Codex configurator round-trip test and the Codex model-discovery route in the same files. Both conflicts were unions — every test must survive — so the suite now registers the Codex round-trip alongside both attribution cases, each with its own category. Verified after the rebase: typecheck 4831 files 0 errors, eslint 0 errors, prettier clean repo-wide, proxy suite 68 passed / 0 failed / 6 skipped, with both attribution cases and the Codex round-trip that came in from release passing together. The Yama check on the previous push failed for infrastructure reasons, not a finding: its LiteLLM upstream answered 502 on all three retries, so no review was generated.
71558a2 to
cc15be2
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 |
|
🔍 Review Summary This PR adds client attribution tracking for the Claude proxy subscription system. It enables operators to see which CLI (Claude Code, OpenCode, etc.) is consuming tokens from a shared subscription account. Changes Made
Review ScopeReviewed all 9 changed files systematically:
Impact Analysis
Key Design Decisions Verified
Final DecisionAPPROVED - No blocking issues found. The implementation is clean, well-tested, and follows all project standards. |
|
🎉 This PR is included in version 11.18.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
GET /accountscould say an account burned $40 today. It could not say whether that was Claude Code, OpenCode, or a runaway script — every figure was per-account only, and one pooled account is routinely shared by several CLIs.The signal already existed and was discarded.
detectClientApp()derived a label fromUser-Agent, wrote it into an OTel span attribute, and stopped.RequestLogEntrynever carried it, so the ledger had nothing to group by.What lands
Requests record the calling client and the raw User-Agent. The ledger splits each account's totals into
byClient:The split reconciles with the total — summing
byClientrequests and cost returnsrequestsandcostUsd, asserted in a test — so a dashboard showing both side by side cannot contradict itself.Two decisions worth challenging
The raw header is stored, not just the derived name. The classifier knows only prefixes observed in real traffic:
claude-cli/andai/, both verified against this machine's proxy logs. I deliberately did not guess prefixes for OpenCode, Qwen or Copilot — a guessed mapping is indistinguishable from a working one until it collides and files a client under the wrong name. Keeping the raw header means an unclassified client stays attributable by its own User-Agent instead of collapsing into one bucket, and naming it later is one line inclientAttribution.tsonce the header has actually been seen.unknownandunattributedare separate buckets. The first is a client we saw and could not name; the second is a row logged before attribution existed, carrying no header at all. Folding them together would make months of history look like an unidentified tool.Verification
Driven through the surface a CLI uses — real POSTs to a spawned proxy carrying real User-Agent headers, with attribution read back out of the request log the proxy itself wrote:
That case includes a caller the classifier cannot name, asserting its raw header survives verbatim — the scenario where this feature is most needed and easiest to get wrong.
tsc --noEmit --stricteslint src testpnpm run buildcontinuous-test-suite-proxycontinuous-test-suite-codexIndependent of #1453, #1454, #1455 — no overlapping files with the first two; touches
accountLedger.tswhich #1455 does not.