Fix LLM logging and cost finalization#93
Merged
Conversation
📝 WalkthroughWalkthroughThis PR refines request-log initialization to skip non-LLM proxy traffic, adds resilient pricing fetching with OpenRouter fallback, prevents token-count regression during retries, and makes daily-usage updates conditional on completed request status. ChangesRequest Lifecycle & Pricing Resilience
Documentation & Test Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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)
package.json (1)
51-51: ⚡ Quick winDe-duplicate
test:e2e:mockcommand env prefix.Repeating the same
MOCK_CLI_PROXY/CLI_PROXY_API_URLprefix for everybun testrun is brittle; Bun supports passing multiple explicit test paths as positional arguments to a singlebun testinvocation.♻️ Proposed simplification
- "test:e2e:mock": "MOCK_CLI_PROXY=1 CLI_PROXY_API_URL=http://localhost:18317 bun test tests/e2e/admin-auth.test.ts && MOCK_CLI_PROXY=1 CLI_PROXY_API_URL=http://localhost:18317 bun test tests/e2e/body-size-limit.test.ts && MOCK_CLI_PROXY=1 CLI_PROXY_API_URL=http://localhost:18317 bun test tests/e2e/ready.test.ts && MOCK_CLI_PROXY=1 CLI_PROXY_API_URL=http://localhost:18317 bun test tests/e2e/proxy.test.ts && MOCK_CLI_PROXY=1 CLI_PROXY_API_URL=http://localhost:18317 bun test tests/e2e/transforms.test.ts", + "test:e2e:mock": "MOCK_CLI_PROXY=1 CLI_PROXY_API_URL=http://localhost:18317 bun test tests/e2e/admin-auth.test.ts tests/e2e/body-size-limit.test.ts tests/e2e/ready.test.ts tests/e2e/proxy.test.ts tests/e2e/transforms.test.ts",🤖 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 `@package.json` at line 51, The npm script "test:e2e:mock" repeats the environment prefix for each bun test invocation; refactor it to set MOCK_CLI_PROXY and CLI_PROXY_API_URL once and call bun test with all test file paths as positional arguments (e.g., bun test tests/e2e/admin-auth.test.ts tests/e2e/body-size-limit.test.ts ...), so update the "test:e2e:mock" entry in package.json to a single env prefix followed by one bun test command listing all the e2e test paths.
🤖 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/storage/pricing.ts`:
- Around line 214-219: The current success path treats a 200 JSON response with
zero usable pricing entries as valid and caches empty data; change the logic
after parsing the response (the call to res.json() and buildPricingMap) to
detect if buildPricingMap(raw) returns an empty map (no entries) and, in that
case, treat it as a failure by logging a warning via logger.warn and calling the
fallback fetchOpenRouterPricing() (or throwing to trigger the existing catch)
instead of returning the empty map; apply the same check and behavior
modification to the analogous code block around the buildPricingMap usage at the
other location (lines 235–250) so empty payloads from remote providers fall back
to OpenRouter rather than being cached as fresh data.
---
Nitpick comments:
In `@package.json`:
- Line 51: The npm script "test:e2e:mock" repeats the environment prefix for
each bun test invocation; refactor it to set MOCK_CLI_PROXY and
CLI_PROXY_API_URL once and call bun test with all test file paths as positional
arguments (e.g., bun test tests/e2e/admin-auth.test.ts
tests/e2e/body-size-limit.test.ts ...), so update the "test:e2e:mock" entry in
package.json to a single env prefix followed by one bun test command listing all
the e2e test paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dbd6237f-1840-4900-9f26-c32a5c95b098
📒 Files selected for processing (11)
CONTRIBUTING.mdREADME.mdpackage.jsonsrc/server/pass-through.tssrc/storage/pricing.tssrc/storage/repo.tssrc/storage/service.tstests/e2e/proxy.test.tstests/unit/cost.test.tstests/unit/lifecycle.test.tstests/unit/storage-lifecycle-cost.test.ts
Comment on lines
+214
to
+219
| const raw = await res.json() as Record<string, ModelsDevProvider>; | ||
| return buildPricingMap(raw); | ||
| } catch (err) { | ||
| logger.warn("models.dev pricing fetch failed, trying OpenRouter", { err, source: "models.dev" }); | ||
| return fetchOpenRouterPricing(); | ||
| } |
There was a problem hiding this comment.
Fail refresh when remote payload parses to zero usable pricing entries.
Right now, an HTTP 200 with no usable models is treated as success and cached as fresh data. That can suppress pricing resolution for a full TTL window.
💡 Suggested fix
async function fetchRemotePricing(): Promise<PricingMap> {
try {
const res = await fetch(MODELS_DEV_URL, { signal: AbortSignal.timeout(30_000) });
if (!res.ok) throw new Error(`models.dev returned HTTP ${res.status}`);
const raw = await res.json() as Record<string, ModelsDevProvider>;
- return buildPricingMap(raw);
+ const map = buildPricingMap(raw);
+ if (map.size === 0) throw new Error("models.dev returned no usable pricing entries");
+ return map;
} catch (err) {
logger.warn("models.dev pricing fetch failed, trying OpenRouter", { err, source: "models.dev" });
return fetchOpenRouterPricing();
}
}
@@
async function fetchOpenRouterPricing(): Promise<PricingMap> {
const res = await fetch(OPENROUTER_MODELS_URL, { signal: AbortSignal.timeout(30_000) });
if (!res.ok) throw new Error(`OpenRouter returned HTTP ${res.status}`);
const raw = await res.json() as { data?: OpenRouterModel[] };
const map: PricingMap = new Map();
@@
- logger.info("loaded pricing aliases", { aliases: map.size, source: "openrouter" });
+ if (map.size === 0) throw new Error("OpenRouter returned no usable pricing entries");
+ logger.info("loaded pricing aliases", { aliases: map.size, source: "openrouter" });
return map;
}Also applies to: 235-250
🤖 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/storage/pricing.ts` around lines 214 - 219, The current success path
treats a 200 JSON response with zero usable pricing entries as valid and caches
empty data; change the logic after parsing the response (the call to res.json()
and buildPricingMap) to detect if buildPricingMap(raw) returns an empty map (no
entries) and, in that case, treat it as a failure by logging a warning via
logger.warn and calling the fallback fetchOpenRouterPricing() (or throwing to
trigger the existing catch) instead of returning the empty map; apply the same
check and behavior modification to the analogous code block around the
buildPricingMap usage at the other location (lines 235–250) so empty payloads
from remote providers fall back to OpenRouter rather than being cached as fresh
data.
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
1 participant
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
Verification
Summary by cubic
Limit request logging to LLM generation endpoints and harden cost finalization to keep failed/aborted requests out of daily usage while preserving audit data.
New Features
models.devis unavailable.Bug Fixes
Written for commit 64df79b. Summary will update on new commits.
Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Documentation