feat(combos): experimental economy combo routing - #1498
Conversation
Introduce the economy combo strategy with shared allowances, cached snapshots, pre-dispatch reservations, settlement (incl. stream/credits), off-path usage-log refresh, and request-path lifecycle wiring.
Add combo explain and economic-allowance list/snapshot endpoints with validation, redaction, and clearReservations conflict semantics.
Expose economy combo configure/explain CLI, allowance snapshot CLI, and GUI round-trip preservation for economy fields.
Add focused suites for selection, races, settlement, refresh, windows, management/CLI APIs, GUI round-trips, and hostile-review blindspots.
Describe day-one behavior, hard/soft signals, snapshot/CLI operations, and deferred adapters honestly.
Reserve all-or-nothing across target allowances, omit reservationId for PAYG, and filter usage-log refresh via optional usageMatch providers/models.
Honest explain reasons/rankingBand, hardExclusions alias, allowance help, louder salvage warnings, and experimental multi-instance docs (docs-site build green).
|
github-actions
Bot
added
intake: hygiene-blocked
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. Hygiene
|
📝 WalkthroughWalkthroughAdds experimental economy-based combo routing with allowance-aware target ranking, quota snapshots, reservations, settlement, cancellation handling, management APIs, CLI commands, GUI persistence, validation, tests, and documentation. ChangesEconomy combo routing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c69c4266f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (invalidAllowance) dropped.push(id); | ||
| else kept[id] = allowance; |
There was a problem hiding this comment.
When a malformed allowance is referenced by an existing combo, this sanitizer drops the allowance but leaves the target reference intact. The subsequent schema parse rejects that reference as unknown, the default-merge retry cannot repair it, and loadConfig() falls back to the complete default config, discarding unrelated providers and accounts despite the warning claiming they were preserved. Remove or disable affected combo references as part of recovery instead of leaving the config invalid.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| if (match.providers && match.providers.length > 0 && !match.providers.includes(entry.provider)) return false; | ||
| if (match.models && match.models.length > 0 && !match.models.includes(entry.model)) return false; |
There was a problem hiding this comment.
For traffic sent through a combo, addFinalRequestLog() records the top-level row as provider combo and the requested combo model (src/server/request-log.ts:804-814), while the physical provider/model only appear in attempts. Comparing usageMatch solely with entry.provider and entry.model therefore makes a scope such as providers: ["included"] ignore all included-target usage; scoping to the combo instead can charge PAYG fallback attempts to the allowance. Refresh must match and sum the applicable physical attempts.
Useful? React with 👍 / 👎.
| updatedAt: now, | ||
| source: "usage-log", | ||
| confidence: "estimated", | ||
| ...(allowance.window.kind === "rolling" ? { windowStart: now } : {}), |
There was a problem hiding this comment.
A source: "usage-log" calendar allowance never receives resetAt here, but snapshotFreshness() classifies every calendar snapshot without that field as unknown. Consequently the documented monthly/day/week usage-log configuration is always deprioritized or rejected, regardless of the calculated remaining value; the refresh also lacks a current-calendar-window start. Compute the timezone-aware current boundary, filter from its start, and publish its next reset.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
| switch (reservation.unit) { | ||
| case "inputTokens": return actual.inputTokens; | ||
| case "outputTokens": return actual.outputTokens; | ||
| case "totalTokens": return actual.totalTokens; |
There was a problem hiding this comment.
OcxUsage.totalTokens is optional, and usageFromResponsesPayload() accepts upstream usage containing input/output token counts without total_tokens. In that common case this returns undefined, so settlement deletes the reservation without decrementing a totalTokens allowance at all. Fall back to inputTokens + outputTokens when both component counts are available.
Useful? React with 👍 / 👎.
| if (item.strategy === "round-robin") roundRobin.push(item); | ||
| else if (item.strategy === "economy") economy.push(item); | ||
| else failover.push(item); |
There was a problem hiding this comment.
This moves economy combos into a new sections.economy bucket, but gui/src/components/ComboWorkspace.tsx:128-131 still renders only sections.failover and sections.roundRobin. A normally configured economy combo therefore contributes to the total count but has no rail row and cannot be selected or managed, defeating the claimed GUI round-trip preservation. Render the new section or keep these items in an existing visible group.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| amount, | ||
| ...(allowance.rates ? { rates: { ...allowance.rates } } : {}), | ||
| ...(result.target.pricing ? { pricing: { ...result.target.pricing } } : {}), | ||
| expiresAt: now + RESERVATION_TTL_MS, |
There was a problem hiding this comment.
For an upstream request or stream lasting more than ten minutes, the periodic sweeper removes this reservation before the terminal usage arrives. settleEconomicReservation() then finds no entries and returns without debiting actual usage, so long-running successful requests consume no allowance and can also release their concurrency headroom prematurely. Expire only the active hold while retaining enough metadata to settle the eventual terminal usage.
Useful? React with 👍 / 👎.
| const snapshot = snapshots.get(allowanceId); | ||
| // Atomic: missing def/snapshot/headroom fails the whole target — never partial holds. | ||
| if (!allowance || !snapshot) return failTarget(); |
There was a problem hiding this comment.
With unknownQuota: "allow", selection deliberately leaves a missing-snapshot target eligible, but this unconditional snapshot check immediately rejects that same target during reservation and falls through to another target. Thus the allow policy can never dispatch an allowance-backed target whose quota is unknown and behaves no differently from rejection on the actual request path. Either permit an unreserved dispatch for this explicit policy or reject/remove the unsupported policy value.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
|
Hey @H-H-E, I am already working on something very similar. It is actually already live, it just needs some polishing. I would be happy to take a closer look at your PR after I am finished though. Just to be sure, you are talking about some type of economic / quota-aware routing, correct? What we currently have is Routing Profiles, where policy/ acts as a virtual model. It takes an explicit candidate list, applies hard requirements first, then ranks eligible candidates using evidence such as health, quota, cost, capabilities, and Compatibility Lab results. It also produces route-decision traces so the selection is explainable. From what I can see, your economy combo strategy overlaps with that quite a bit: hard candidate eligibility The part of your PR that looks particularly interesting and that we do not currently have in the same form is the economic allowance layer: multiple allowance windows, reservations before dispatch, settlement against actual usage, reserve thresholds, expiry pressure, and manual / usage-log snapshots. So I think there may be a good opportunity to combine the ideas instead of ending up with two separate evidence-based routers, one under combo/economy and another under policy/*. My first thought would be to make the allowance / reservation system another evidence source for Routing Profiles, while keeping Combos focused on explicit failover / round-robin behaviour. But I want to properly review your implementation before suggesting that as the right direction. I will take a closer look once I am done with the remaining Routing Profiles / Compatibility Lab polishing. 👍🏼 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🤖 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-site/src/content/docs/guides/combos.md`:
- Around line 433-437: Remove the invalid cmd.exe file-input example from the
Windows guidance in the combo documentation, or replace it with a command that
passes the file’s JSON contents as the --combo-json argument using syntax
supported by the CLI. Keep the PowerShell example and valid escaped JSON usage
unchanged.
- Around line 404-409: Update the combo explanation documentation near the
settlement statement to explicitly describe reservation outcomes for clean
completion, cancellation, transport failure, and incomplete streams when no
terminal usage event arrives. Distinguish adapter-reported usage from estimated
usage, and state whether each case settles actual or estimated consumption,
releases the reservation, or applies another defined outcome.
In `@docs-site/src/content/docs/reference/configuration/routing.md`:
- Around line 242-251: Update the economy strategy documentation near the
runtime snapshot description to state that stale or incomplete quota snapshots
are treated as quota-unknown. Clarify that these snapshots follow the configured
unknownQuota policy during selection, while preserving the existing
process-local caching behavior.
In `@src/cli/allowance.ts`:
- Around line 96-102: Update handleAllowanceCommand so a leading "--json" is
treated as the default list command’s argument, allowing list() to consume the
flag instead of reporting an unknown subcommand. Preserve existing list,
snapshot, and unknown-command behavior, and add a regression test covering
handleAllowanceCommand(["--json"]).
In `@src/combos/economy-refresh.ts`:
- Around line 84-101: Update performRefresh and the snapshot data flow around
readRecentUsageEntries and snapshotFor to detect when the 2000-entry read is
truncated, preserve that state in each affected OcxEconomicSnapshot, and expose
it to operators and usableHeadroom ranking. Ensure incomplete estimates are
identifiable rather than treated as normal quota headroom, while leaving fully
read windows unchanged.
In `@src/combos/economy.ts`:
- Around line 476-487: Use winner.configIndex directly for targetIndex in
selectEconomicTarget instead of re-deriving it with findIndex. In
src/combos/economy.ts lines 476-487, return the exact candidate position
recorded by candidateFor. In src/combos/resolve.ts lines 106-121, widen the
economy target-index guard to reject values below zero as well as null, and call
releaseEconomicReservation(economic.reservationId) before every economy-branch
return null.
- Around line 621-632: Update the settlement loop in the allowance snapshot
handling to preserve the existing snapshot.updatedAt when applying locally
measured usage. Remove the updatedAt: now rewrite from the snapshots.set call
while keeping the remaining calculation and other snapshot fields unchanged, so
snapshotFreshness continues measuring the original upstream observation time.
- Around line 605-615: Update settleEconomicReservation to release the
reservation and record the invalid-usage anomaly without throwing a TypeError,
so response finalization and failover continue even when adapter usage is
invalid. Preserve the existing validation and settled-ID behavior, and use the
module’s established anomaly-recording mechanism.
In `@src/config.ts`:
- Around line 2065-2073: Update the economic allowance sanitization flow around
the dropped allowance handling and combo validation so any economy combo
referencing a dropped allowance is pruned or disabled before the subsequent
safeParse checks. Preserve the allowance reference semantics—do not silently
convert affected targets to PAYG—and add a regression test covering a combo that
references a malformed, dropped allowance while ensuring the rest of the
configuration is preserved.
In `@src/lib/state-store-sweeper.ts`:
- Line 11: Make allowanceIds required in GenerationContext, matching the other
required fields. In reconcileEconomicState, remove the nullish fallback and
construct liveAllowanceIds directly from context.allowanceIds, preserving the
existing reconciliation behavior.
In `@src/server/management/combo-routes.ts`:
- Around line 72-87: Gate the explain route before calling explainEconomicCombo:
after resolving combo in the GET explain handler, reject any combo whose
strategy is not "economy" with a 400 or 409 response. Only economy combos should
reach explainEconomicCombo; preserve the existing validation and explanation
behavior for them.
- Around line 79-80: Update the combo lookup guard near the route’s combo
handling to use Object.hasOwn(config.combos ?? {}, comboId) before reading or
accepting the combo. Preserve the existing 404 response for IDs that are not own
configured combo properties, including inherited names such as constructor,
toString, and valueOf, and keep the valid combo explanation flow unchanged.
In `@src/server/management/economic-snapshot-routes.ts`:
- Around line 41-52: The collection listing in
src/server/management/economic-snapshot-routes.ts:41-52 must use the same
current-time handling as the conflict paths when calling
countEconomicReservationsForAllowance; pass Date.now() explicitly at this site
unless the function’s default is confirmed to provide the current time. Extend
the collection test in tests/economic-manual-snapshot-api.test.ts:391-398 to
create a live reservation with reserveEconomicSelection and assert
activeReservations matches the reservation count that causes the PUT/DELETE
paths to return 409.
- Around line 99-101: Update the remaining validation in the economic snapshot
route after resolving the allowance to also reject values greater than
config.economicAllowances[allowanceId].capacity. Return a 400 response with a
clear error when remaining exceeds that configured capacity, while preserving
the existing finite non-negative validation and avoiding silent clamping.
In `@src/server/responses/core.ts`:
- Around line 1056-1068: Update settleComboReservation to release the economic
reservation when usage.estimated is true, while continuing to settle only
provider-reported usage and release when usage is absent. Add a lifecycle test
covering an estimated non-streaming response.
In `@tests/cli-help-allowance.test.ts`:
- Around line 9-13: Update the allowance help test around printSubcommandUsage
to verify the allowance help entry is registered before invoking the printer, or
stub process.exit during the call. Ensure a missing entry produces assertion
failures while always restoring console.log and any process.exit stub.
In `@tests/economic-management-api.test.ts`:
- Line 95: Update the config() fixture in the economic-management API test to
include distinct apiKey values for both included and payg providers, matching
the credential-bearing setup in the equivalent manual snapshot test. Keep the
existing JSON privacy assertion so it can detect explainEconomicCombo leaking
resolved provider credentials.
- Line 7: In the tests around handleManagementAPI, remove the unused NOW
constant and make the response assertion null-safe by either guarding the
nullable result before accessing status or using the non-null assertion where
the test guarantees a response. Keep the existing Date.now() timestamp usage
unchanged.
In `@tests/economic-manual-snapshot-api.test.ts`:
- Around line 391-398: Add a live reservation setup to the GET
/api/economic-allowances collection test, reusing the existing reservation
helper pattern from this file, then assert the returned allowance’s
activeReservations value alongside id, state, and snapshot.remaining. Ensure the
test exercises a held reservation so the listing count reflects the same
behavior used by the conflict response.
In `@tests/economic-ordering-stability.test.ts`:
- Around line 48-66: Correct the test description and assertion rationale around
selectEconomicTarget to reflect that safe() normalizes Infinity and NaN pricing
to 0, so the stable tie comes from equal costs and configIndex. Add a separate
mixed-pricing test covering one finite marginalUsd and one null marginalUsd, and
assert the expected candidate ordering/selection for that branch.
In `@tests/economic-reservation-race.test.ts`:
- Line 13: Update the config helper’s target type in function config to account
for optional OcxConfig["combos"] before indexing by string, using the existing
type definitions to derive the valid target shape without directly indexing an
optional property.
In `@tests/economic-reservation-settlement.test.ts`:
- Around line 232-247: Fix the SSE payloads in both streaming cancellation tests
around “releases streamed reservation when cancelled before usage” and “does not
settle even when usage would return tokens” by using real newline characters so
the parser receives complete frames. In the latter test, enqueue a usage frame
with prompt_tokens 1, completion_tokens 4, and total_tokens 5 before
cancellation, preserving the intended assertion that cancellation releases the
reservation instead of settling it.
In `@tests/economic-reservation-terminal.test.ts`:
- Around line 99-105: Update customFetchResponse to read req.body asynchronously
using the Request body API before parsing it, while preserving the existing
model check and context_length_exceeded 400 response. Ensure the fixture reaches
the intended terminal-error branch instead of throwing while parsing the request
body.
In `@tests/economic-snapshot-refresh.test.ts`:
- Around line 107-115: Update the test “skips recomputation for an unchanged
usage-log revision” to assert observable snapshot state instead of relying on
the setEconomicQuotaSnapshot spy. After the second refresh, verify the stored
snapshot’s updatedAt remains equal to the initial refresh time, using the
snapshot retrieval path already exposed by the economy module; remove the
ineffective negative spy assertion and related setup.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 5a792f2c-f499-4fc0-b3f4-4701aabcad6a
📒 Files selected for processing (37)
docs-site/src/content/docs/guides/combos.mddocs-site/src/content/docs/reference/configuration/routing.mdgui/src/combo-workspace-data.tssrc/cli/allowance.tssrc/cli/combo.tssrc/cli/help.tssrc/cli/index.tssrc/combos/economy-refresh.tssrc/combos/economy.tssrc/combos/index.tssrc/combos/resolve.tssrc/combos/types.tssrc/config.tssrc/lib/state-store-registrations.tssrc/lib/state-store-sweeper.tssrc/router.tssrc/server/management-api.tssrc/server/management/combo-routes.tssrc/server/management/economic-snapshot-routes.tssrc/server/responses/core.tssrc/types.tstests/cli-allowance.test.tstests/cli-combo.test.tstests/cli-help-allowance.test.tstests/combo-workspace-data.test.tstests/combos.test.tstests/economic-allowances-validation.test.tstests/economic-management-api.test.tstests/economic-manual-snapshot-api.test.tstests/economic-ordering-stability.test.tstests/economic-reservation-race.test.tstests/economic-reservation-settlement.test.tstests/economic-reservation-terminal.test.tstests/economic-review-blindspots.test.tstests/economic-routing.test.tstests/economic-snapshot-refresh.test.tstests/economic-window-boundaries.test.ts
| Run `ocx combo explain bulk-code --input-tokens 2000 --output-tokens 500 --json` (or | ||
| `GET /api/combos/bulk-code/explain`) to see eligibility, soft signals, every bucket's remaining and | ||
| reserved headroom, reserve threshold, expiry pressure, stale state, marginal/cash cost, and the | ||
| selected target. Selections reserve predicted consumption locally before dispatch; races never return | ||
| an allowance-backed target without a reservation. Completion settles actual usage (including stream | ||
| EOF); cancellation releases without burn. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document incomplete-stream settlement behavior.
Line 408 states that completion settles actual usage, but it does not describe transport failures or incomplete streams. Operators cannot determine whether the reservation is released or partially settled when a terminal usage event is absent. State the behavior for clean completion, cancellation, transport failure, and incomplete streams. Distinguish adapter-reported usage from estimated usage.
As per path instructions, “Streaming usage may arrive in terminal events, so reservation settlement must account for clean completion versus cancellation, transport failure, and incomplete streams.”
🤖 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-site/src/content/docs/guides/combos.md` around lines 404 - 409, Update
the combo explanation documentation near the settlement statement to explicitly
describe reservation outcomes for clean completion, cancellation, transport
failure, and incomplete streams when no terminal usage event arrives.
Distinguish adapter-reported usage from estimated usage, and state whether each
case settles actual or estimated consumption, releases the reservation, or
applies another defined outcome.
Source: Path instructions
| > **Windows users:** POSIX single quotes do not protect JSON in `cmd.exe` or PowerShell. On | ||
| > Windows, wrap the JSON argument in double quotes and escape inner double quotes, for example | ||
| > `ocx combo set bulk-code --combo-json "{\"strategy\":\"economy\",\"targets\":[...]}"`, or pass the | ||
| > JSON through a file with `--combo-json (Get-Content combo.json -Raw)` (PowerShell) / | ||
| > `--combo-json "<combo.json"` (`cmd.exe`). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the invalid cmd.exe file-input command.
Line 437 does not pass combo.json contents as the --combo-json argument. Input redirection supplies stdin, but --combo-json requires a command-line value. Remove the cmd.exe example or replace it with a command that constructs an escaped JSON argument.
As per path instructions, user-facing docs must stay in sync with actual CLI behavior.
🤖 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-site/src/content/docs/guides/combos.md` around lines 433 - 437, Remove
the invalid cmd.exe file-input example from the Windows guidance in the combo
documentation, or replace it with a command that passes the file’s JSON contents
as the --combo-json argument using syntax supported by the CLI. Keep the
PowerShell example and valid escaped JSON usage unchanged.
Source: Path instructions
| `strategy: "economy"` is an **experimental**, additive combo strategy. Shared static allowance | ||
| buckets live under `economicAllowances` and are referenced by target `allowances` arrays; remaining | ||
| values are cached **process-local** runtime snapshots (lost on restart; not shared across instances). | ||
| Selection is deterministic: hard eligibility first, then soft reserve / unknown-quota pressure, | ||
| expiration pressure, marginal cost, and configured target order. It does not classify requests or | ||
| call provider quota APIs on the request path. Ledger: settle debits actual usage only; cancel | ||
| releases holds without burn. Optional `usageMatch.providers` / `usageMatch.models` scopes | ||
| `source: "usage-log"` refresh; unscoped usage-log summation is experimental. Use | ||
| `ocx combo explain <id> --json` and `ocx allowance …` for operator surfaces. The GUI preserves | ||
| economy fields only — no full editor. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document stale quota snapshots.
Lines 242-251 describe missing snapshots but not stale or incomplete snapshots. A cached snapshot can still be unusable for economy selection. State that stale or incomplete snapshots are treated as quota-unknown and follow the unknownQuota policy.
As per path instructions, “Document incomplete or stale quota coverage explicitly.”
🤖 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-site/src/content/docs/reference/configuration/routing.md` around lines
242 - 251, Update the economy strategy documentation near the runtime snapshot
description to state that stale or incomplete quota snapshots are treated as
quota-unknown. Clarify that these snapshots follow the configured unknownQuota
policy during selection, while preserving the existing process-local caching
behavior.
Source: Path instructions
| export async function handleAllowanceCommand(argv: string[], deps: RuntimeApiDeps = {}): Promise<number> { | ||
| return runCliAction(async () => { | ||
| const [sub = "list", ...rest] = argv; | ||
| if (sub === "list") await list(rest, deps); | ||
| else if (sub === "snapshot") await snapshot(rest, deps); | ||
| else throw new CliUsageError(`unknown allowance command ${sub}`, USAGE); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Support the documented ocx allowance --json form.
ALLOWANCE_USAGE documents ocx allowance [list] [--json]. handleAllowanceCommand(["--json"]) sets sub to "--json" and exits with an unknown-command error before list() can consume the flag. Treat a leading --json as list arguments. Add a regression test for this invocation.
Proposed fix
export async function handleAllowanceCommand(argv: string[], deps: RuntimeApiDeps = {}): Promise<number> {
return runCliAction(async () => {
- const [sub = "list", ...rest] = argv;
- if (sub === "list") await list(rest, deps);
+ const [sub, ...rest] = argv;
+ if (sub === undefined || sub === "list") await list(rest, deps);
+ else if (sub === "--json") await list(argv, deps);
else if (sub === "snapshot") await snapshot(rest, deps);
else throw new CliUsageError(`unknown allowance command ${sub}`, USAGE);
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function handleAllowanceCommand(argv: string[], deps: RuntimeApiDeps = {}): Promise<number> { | |
| return runCliAction(async () => { | |
| const [sub = "list", ...rest] = argv; | |
| if (sub === "list") await list(rest, deps); | |
| else if (sub === "snapshot") await snapshot(rest, deps); | |
| else throw new CliUsageError(`unknown allowance command ${sub}`, USAGE); | |
| }); | |
| export async function handleAllowanceCommand(argv: string[], deps: RuntimeApiDeps = {}): Promise<number> { | |
| return runCliAction(async () => { | |
| const [sub, ...rest] = argv; | |
| if (sub === undefined || sub === "list") await list(rest, deps); | |
| else if (sub === "--json") await list(argv, deps); | |
| else if (sub === "snapshot") await snapshot(rest, deps); | |
| else throw new CliUsageError(`unknown allowance command ${sub}`, USAGE); | |
| }); |
🤖 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/cli/allowance.ts` around lines 96 - 102, Update handleAllowanceCommand so
a leading "--json" is treated as the default list command’s argument, allowing
list() to consume the flag instead of reporting an unknown subcommand. Preserve
existing list, snapshot, and unknown-command behavior, and add a regression test
covering handleAllowanceCommand(["--json"]).
| async function performRefresh(config: OcxConfig, now: number): Promise<void> { | ||
| let revisionKey: string; | ||
| try { | ||
| revisionKey = `${usageLogRevisionKey(currentUsageLogRevision())}\0${allowanceConfigKey(config)}`; | ||
| if (revisionKey === lastRevisionKey) return; | ||
| const entries = readRecentUsageEntries(2000); | ||
| const prepared = new Map<string, OcxEconomicSnapshot>(); | ||
| for (const [id, allowance] of Object.entries(config.economicAllowances ?? {})) { | ||
| if (allowance.source !== "usage-log") continue; | ||
| prepared.set(id, snapshotFor(allowance, entries, now)); | ||
| } | ||
| for (const [id, snapshot] of prepared) setEconomicQuotaSnapshot(id, snapshot); | ||
| lastRevisionKey = revisionKey; | ||
| } catch (error) { | ||
| markRefreshFailure(config, error); | ||
| lastRevisionKey = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Flag truncated usage-log reads instead of silently overstating headroom.
Line 89 reads at most 2000 entries. If the rolling window contains more than 2000 entries, snapshotFor sums only the newest 2000 rows. used is then too low, and remaining = capacity - used is too high. The snapshot looks like normal headroom, so usableHeadroom admits requests that the real quota cannot cover.
The window filter runs after the cap, so a long durationMs with high traffic hits this first.
Record the truncation in the snapshot so operators and the ranking path can see that the estimate is incomplete.
🐛 Proposed fix to surface truncation
- const entries = readRecentUsageEntries(2000);
+ const USAGE_READ_LIMIT = 2000;
+ const entries = readRecentUsageEntries(USAGE_READ_LIMIT);
+ const truncated = entries.length >= USAGE_READ_LIMIT;
const prepared = new Map<string, OcxEconomicSnapshot>();
for (const [id, allowance] of Object.entries(config.economicAllowances ?? {})) {
if (allowance.source !== "usage-log") continue;
- prepared.set(id, snapshotFor(allowance, entries, now));
+ const snapshot = snapshotFor(allowance, entries, now);
+ prepared.set(id, truncated
+ ? { ...snapshot, error: "usage-log read truncated; remaining may be overstated" }
+ : snapshot);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function performRefresh(config: OcxConfig, now: number): Promise<void> { | |
| let revisionKey: string; | |
| try { | |
| revisionKey = `${usageLogRevisionKey(currentUsageLogRevision())}\0${allowanceConfigKey(config)}`; | |
| if (revisionKey === lastRevisionKey) return; | |
| const entries = readRecentUsageEntries(2000); | |
| const prepared = new Map<string, OcxEconomicSnapshot>(); | |
| for (const [id, allowance] of Object.entries(config.economicAllowances ?? {})) { | |
| if (allowance.source !== "usage-log") continue; | |
| prepared.set(id, snapshotFor(allowance, entries, now)); | |
| } | |
| for (const [id, snapshot] of prepared) setEconomicQuotaSnapshot(id, snapshot); | |
| lastRevisionKey = revisionKey; | |
| } catch (error) { | |
| markRefreshFailure(config, error); | |
| lastRevisionKey = null; | |
| } | |
| } | |
| async function performRefresh(config: OcxConfig, now: number): Promise<void> { | |
| let revisionKey: string; | |
| try { | |
| revisionKey = `${usageLogRevisionKey(currentUsageLogRevision())}\0${allowanceConfigKey(config)}`; | |
| if (revisionKey === lastRevisionKey) return; | |
| const USAGE_READ_LIMIT = 2000; | |
| const entries = readRecentUsageEntries(USAGE_READ_LIMIT); | |
| const truncated = entries.length >= USAGE_READ_LIMIT; | |
| const prepared = new Map<string, OcxEconomicSnapshot>(); | |
| for (const [id, allowance] of Object.entries(config.economicAllowances ?? {})) { | |
| if (allowance.source !== "usage-log") continue; | |
| const snapshot = snapshotFor(allowance, entries, now); | |
| prepared.set(id, truncated | |
| ? { ...snapshot, error: "usage-log read truncated; remaining may be overstated" } | |
| : snapshot); | |
| } | |
| for (const [id, snapshot] of prepared) setEconomicQuotaSnapshot(id, snapshot); | |
| lastRevisionKey = revisionKey; | |
| } catch (error) { | |
| markRefreshFailure(config, error); | |
| lastRevisionKey = null; | |
| } | |
| } |
🤖 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/combos/economy-refresh.ts` around lines 84 - 101, Update performRefresh
and the snapshot data flow around readRecentUsageEntries and snapshotFor to
detect when the 2000-entry read is truncated, preserve that state in each
affected OcxEconomicSnapshot, and expose it to operators and usableHeadroom
ranking. Ensure incomplete estimates are identifiable rather than treated as
normal quota headroom, while leaving fully read windows unchanged.
| test("Infinity and NaN pricing values are treated as unknown and do not break ordering", () => { | ||
| const cfg: OcxConfig = { | ||
| ...config(), | ||
| combos: { | ||
| bulk: { | ||
| strategy: "economy", | ||
| targets: [ | ||
| { provider: "a", model: "m", pricing: { inputUsdPerMillion: Number.POSITIVE_INFINITY } }, | ||
| { provider: "b", model: "m", pricing: { inputUsdPerMillion: Number.NaN } }, | ||
| ], | ||
| }, | ||
| }, | ||
| }; | ||
| const estimate = { inputTokens: 1_000_000, outputTokens: 0, kind: "configured" as const }; | ||
| const result = selectEconomicTarget(cfg, "bulk", estimate, NOW); | ||
| // Both costs are non-finite => should be treated equal and stable (first wins) | ||
| expect(result.target?.provider).toBe("a"); | ||
| expect(result.candidates.every(c => c.marginalUsd === null || Number.isFinite(c.marginalUsd!))).toBe(true); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The comment does not match what this test exercises, and the mixed finite/non-finite branch stays uncovered.
marginalUsd in src/combos/economy.ts:275-282 passes every pricing field through safe(). safe(Infinity) and safe(NaN) both return 0, because finiteNonNegative rejects non-finite values.
So for both targets the computed cost is the finite number 0, not a non-finite value. The comment on Line 63 states the opposite. The tie at Line 64 comes from 0 === 0 and the configIndex fallback, not from the non-finite handling in compareCandidates.
The mixed branch — one target with a finite marginalUsd and one with null — is the branch that can reorder targets, and no test covers it. Add that case.
♻️ Proposed comment fix and added coverage
- // Both costs are non-finite => should be treated equal and stable (first wins)
+ // safe() coerces Infinity and NaN to 0, so both costs become the finite value 0.
+ // The tie therefore resolves through configIndex and the first target wins.
expect(result.target?.provider).toBe("a");
expect(result.candidates.every(c => c.marginalUsd === null || Number.isFinite(c.marginalUsd!))).toBe(true);
});
+
+ test("a priced target outranks an unpriced target with unknown marginal cost", () => {
+ const cfg: OcxConfig = {
+ ...config(),
+ combos: {
+ bulk: {
+ strategy: "economy",
+ targets: [
+ { provider: "a", model: "m" },
+ { provider: "b", model: "m", pricing: { inputUsdPerMillion: 1 } },
+ ],
+ },
+ },
+ };
+ const estimate = { inputTokens: 1_000_000, outputTokens: 0, kind: "configured" as const };
+ const result = selectEconomicTarget(cfg, "bulk", estimate, NOW);
+ // "a" has no pricing and no allowances => marginalUsd null => ranked after "b".
+ expect(result.target?.provider).toBe("b");
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("Infinity and NaN pricing values are treated as unknown and do not break ordering", () => { | |
| const cfg: OcxConfig = { | |
| ...config(), | |
| combos: { | |
| bulk: { | |
| strategy: "economy", | |
| targets: [ | |
| { provider: "a", model: "m", pricing: { inputUsdPerMillion: Number.POSITIVE_INFINITY } }, | |
| { provider: "b", model: "m", pricing: { inputUsdPerMillion: Number.NaN } }, | |
| ], | |
| }, | |
| }, | |
| }; | |
| const estimate = { inputTokens: 1_000_000, outputTokens: 0, kind: "configured" as const }; | |
| const result = selectEconomicTarget(cfg, "bulk", estimate, NOW); | |
| // Both costs are non-finite => should be treated equal and stable (first wins) | |
| expect(result.target?.provider).toBe("a"); | |
| expect(result.candidates.every(c => c.marginalUsd === null || Number.isFinite(c.marginalUsd!))).toBe(true); | |
| }); | |
| test("Infinity and NaN pricing values are treated as unknown and do not break ordering", () => { | |
| const cfg: OcxConfig = { | |
| ...config(), | |
| combos: { | |
| bulk: { | |
| strategy: "economy", | |
| targets: [ | |
| { provider: "a", model: "m", pricing: { inputUsdPerMillion: Number.POSITIVE_INFINITY } }, | |
| { provider: "b", model: "m", pricing: { inputUsdPerMillion: Number.NaN } }, | |
| ], | |
| }, | |
| }, | |
| }; | |
| const estimate = { inputTokens: 1_000_000, outputTokens: 0, kind: "configured" as const }; | |
| const result = selectEconomicTarget(cfg, "bulk", estimate, NOW); | |
| // safe() coerces Infinity and NaN to 0, so both costs become the finite value 0. | |
| // The tie therefore resolves through configIndex and the first target wins. | |
| expect(result.target?.provider).toBe("a"); | |
| expect(result.candidates.every(c => c.marginalUsd === null || Number.isFinite(c.marginalUsd!))).toBe(true); | |
| }); | |
| test("a priced target outranks an unpriced target with unknown marginal cost", () => { | |
| const cfg: OcxConfig = { | |
| ...config(), | |
| combos: { | |
| bulk: { | |
| strategy: "economy", | |
| targets: [ | |
| { provider: "a", model: "m" }, | |
| { provider: "b", model: "m", pricing: { inputUsdPerMillion: 1 } }, | |
| ], | |
| }, | |
| }, | |
| }; | |
| const estimate = { inputTokens: 1_000_000, outputTokens: 0, kind: "configured" as const }; | |
| const result = selectEconomicTarget(cfg, "bulk", estimate, NOW); | |
| // "a" has no pricing and no allowances => marginalUsd null => ranked after "b". | |
| expect(result.target?.provider).toBe("b"); | |
| }); |
🤖 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 `@tests/economic-ordering-stability.test.ts` around lines 48 - 66, Correct the
test description and assertion rationale around selectEconomicTarget to reflect
that safe() normalizes Infinity and NaN pricing to 0, so the stable tie comes
from equal costs and configIndex. Add a separate mixed-pricing test covering one
finite marginalUsd and one null marginalUsd, and assert the expected candidate
ordering/selection for that branch.
| const NOW = Date.parse("2026-08-10T12:00:00.000Z"); | ||
| const estimate = { inputTokens: 0, outputTokens: 0, fixedRequests: 1, kind: "configured" as const }; | ||
|
|
||
| function config(targets: OcxConfig["combos"][string]["targets"]): OcxConfig { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm combos optionality on OcxConfig and whether tests are typechecked.
rg -n -C 3 'combos\??:' src/types.ts
fd -t f 'tsconfig*.json' --exec cat -n {}
rg -n '"typecheck"|tsc --noEmit' package.jsonRepository: lidge-jun/opencodex
Length of output: 1448
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target test ---'
cat -n tests/economic-reservation-race.test.ts | sed -n '1,45p'
printf '%s\n' '--- OcxConfig declarations and relevant definitions ---'
rg -n -C 8 '^(export )?(interface|type) OcxConfig|combos\?:|type OcxComboConfig|interface OcxComboConfig' src tests
printf '%s\n' '--- TypeScript configuration files ---'
fd -t f 'tsconfig*.json' -o -t f 'bunfig.toml' -o -t f 'package.json' | sort | while read -r f; do
printf '\n--- %s ---\n' "$f"
cat -n "$f" | sed -n '1,180p'
done
printf '%s\n' '--- test typecheck references ---'
rg -n -C 4 'tests|typecheck|tsc|bun test|check' package.json tsconfig*.json bunfig.toml 2>/dev/null || true
printf '%s\n' '--- relevant target usages ---'
rg -n -C 4 'config\(|OcxConfig\["combos"\]|NonNullable<OcxConfig\["combos"\]>' tests srcRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target test lines ---'
sed -n '1,32p' tests/economic-reservation-race.test.ts
printf '%s\n' '--- OcxConfig declaration ---'
rg -n -C 12 'interface OcxConfig|type OcxConfig|combos\?:' src/types.ts src/combos/types.ts
printf '%s\n' '--- package scripts and configs ---'
sed -n '1,110p' package.json
for f in tsconfig.json tsconfig.app.json tsconfig.node.json bunfig.toml; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
cat -n "$f"
fi
done
printf '%s\n' '--- exact related test patterns ---'
rg -n -C 3 'OcxConfig\["combos"\]|NonNullable<OcxConfig\["combos"\]>' tests/economic-reservation-race.test.ts tests/server-combo-failover-e2e.test.ts tests/economic-routing.test.ts
printf '%s\n' '--- compiler availability ---'
if command -v tsc >/dev/null 2>&1; then tsc --version; else echo 'tsc unavailable'; fi
if command -v bun >/dev/null 2>&1; then bun --version; else echo 'bun unavailable'; fi
printf '%s\n' '--- standalone indexed-access probe ---'
cat >/tmp/ocx-indexed-access-probe.ts <<'TS'
type OcxComboConfig = { targets: Array<{ provider: string; model: string }> };
interface OcxConfig {
combos?: Record<string, OcxComboConfig>;
}
type Original = OcxConfig["combos"][string]["targets"];
type Fixed = NonNullable<OcxConfig["combos"]>[string]["targets"];
TS
if command -v tsc >/dev/null 2>&1; then
tsc --strict --noEmit --skipLibCheck /tmp/ocx-indexed-access-probe.ts 2>&1 || true
fiRepository: lidge-jun/opencodex
Length of output: 16840
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- standalone indexed-access probe with repository config ignored ---'
if command -v tsc >/dev/null 2>&1; then
tsc --ignoreConfig --strict --noEmit --skipLibCheck /tmp/ocx-indexed-access-probe.ts 2>&1 || true
fi
printf '%s\n' '--- test runner ---'
if [ -f scripts/test.ts ]; then
cat -n scripts/test.ts
else
echo 'scripts/test.ts not found'
fi
printf '%s\n' '--- test-related typecheck commands ---'
rg -n -C 3 'tsc|typecheck|bun test|test\(' scripts package.json .github 2>/dev/null | head -n 240 || trueRepository: lidge-jun/opencodex
Length of output: 22841
Fix the invalid indexed access at tests/economic-reservation-race.test.ts:13. OcxConfig["combos"] is optional (src/types.ts:894), so OcxConfig["combos"][string] fails with TS2537 under strict TypeScript.
Proposed fix
-function config(targets: OcxConfig["combos"][string]["targets"]): OcxConfig {
+function config(targets: NonNullable<OcxConfig["combos"]>[string]["targets"]): OcxConfig {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function config(targets: OcxConfig["combos"][string]["targets"]): OcxConfig { | |
| function config(targets: NonNullable<OcxConfig["combos"]>[string]["targets"]): OcxConfig { |
🤖 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 `@tests/economic-reservation-race.test.ts` at line 13, Update the config
helper’s target type in function config to account for optional
OcxConfig["combos"] before indexing by string, using the existing type
definitions to derive the valid target shape without directly indexing an
optional property.
| test("releases streamed reservation when cancelled before usage", async () => { | ||
| setEconomicQuotaSnapshot("allowance", { remaining: 50, updatedAt: Date.now(), source: "manual", confidence: "authoritative" }); | ||
| customFetchResponse = async () => new Response(new ReadableStream({ | ||
| start(controller) { | ||
| controller.enqueue(new TextEncoder().encode("data: {}\\n\\n")); | ||
| }, | ||
| }), { headers: { "content-type": "text/event-stream" } }); | ||
| const request = new Request("http://localhost/v1/responses", { | ||
| method: "POST", | ||
| headers: { "content-type": "application/json" }, | ||
| body: JSON.stringify({ model: "combo/c", input: "hello", max_output_tokens: 10, stream: true }), | ||
| }); | ||
| const response = await handleComboResponses(request, { model: "combo/c", input: "hello", max_output_tokens: 10, stream: true }, "c", lifecycleConfig("credits"), { model: "", provider: "" }, {}); | ||
| await response.body?.cancel(); | ||
| expect(getEconomicQuotaSnapshot("allowance")?.remaining).toBe(50); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The SSE payload on Line 236 contains literal \n characters, so these cancellation tests assert almost nothing.
Line 236 is a normal double-quoted string, not a raw string. The sequence \\n in source produces one backslash plus the letter n. The bytes written to the stream are therefore data: {}\n\n as six literal characters with no line break.
An SSE frame terminates on a blank line. Without real newlines the chunk is never parsed as an event, so the stream yields no usage regardless of the settlement logic. The assertion at Line 246 passes trivially.
The same defect is on Line 283, and it matters more there: the test at Line 278 is named "does not settle even when usage would return tokens", but no usage ever reaches the parser, so the "would return tokens" half of the scenario is never set up.
Lines 211-217 and 253-259 get this right with template literals and real line breaks. Use the same form.
🐛 Proposed fix for both occurrences
- controller.enqueue(new TextEncoder().encode("data: {}\\n\\n"));
+ controller.enqueue(new TextEncoder().encode('data: {"choices":[{"delta":{"content":"partial"},"finish_reason":null}]}\n\n'));Apply the identical change on Line 283. For the Line 278 test, emit a usage frame before the cancel so the assertion proves that a cancelled stream releases rather than settles:
controller.enqueue(new TextEncoder().encode(
'data: {"choices":[],"usage":{"prompt_tokens":1,"completion_tokens":4,"total_tokens":5}}\n\n',
));🤖 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 `@tests/economic-reservation-settlement.test.ts` around lines 232 - 247, Fix
the SSE payloads in both streaming cancellation tests around “releases streamed
reservation when cancelled before usage” and “does not settle even when usage
would return tokens” by using real newline characters so the parser receives
complete frames. In the latter test, enqueue a usage frame with prompt_tokens 1,
completion_tokens 4, and total_tokens 5 before cancellation, preserving the
intended assertion that cancellation releases the reservation instead of
settling it.
| customFetchResponse = async (req) => { | ||
| const body = JSON.parse(String(req.body)) as { model?: string }; | ||
| if (body.model === "m") { | ||
| return Response.json({ error: { code: "context_length_exceeded", message: "too long" } }, { status: 400 }); | ||
| } | ||
| return Response.json({ id: "x", object: "response", status: "completed", model: "m", output: [] }); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
JSON.parse(String(req.body)) throws, so this test never reaches the 400 branch it is named for.
req.body on a Request is a ReadableStream | null, not a string. String(...) on a ReadableStream produces "[object ReadableStream]", and JSON.parse on that value throws a SyntaxError.
The throw happens inside customFetchResponse, which the mocked adapter calls from fetchResponse (Lines 23-26). Control never reaches Line 101, so the context_length_exceeded 400 response at Line 102 is never returned.
The consequence is that this test does not verify what its name claims. It verifies reservation release after an adapter exception, which is exactly what the second test at Line 122 already covers. If expect(res.status).toBe(400) at Line 113 currently passes, it passes because the dispatch layer maps the adapter throw to 400, not because the fixture produced the terminal error.
Read the body asynchronously.
🐛 Proposed fix
customFetchResponse = async (req) => {
- const body = JSON.parse(String(req.body)) as { model?: string };
+ const body = await req.json() as { model?: string };
if (body.model === "m") {
return Response.json({ error: { code: "context_length_exceeded", message: "too long" } }, { status: 400 });
}
return Response.json({ id: "x", object: "response", status: "completed", model: "m", output: [] });
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| customFetchResponse = async (req) => { | |
| const body = JSON.parse(String(req.body)) as { model?: string }; | |
| if (body.model === "m") { | |
| return Response.json({ error: { code: "context_length_exceeded", message: "too long" } }, { status: 400 }); | |
| } | |
| return Response.json({ id: "x", object: "response", status: "completed", model: "m", output: [] }); | |
| }; | |
| customFetchResponse = async (req) => { | |
| const body = await req.json() as { model?: string }; | |
| if (body.model === "m") { | |
| return Response.json({ error: { code: "context_length_exceeded", message: "too long" } }, { status: 400 }); | |
| } | |
| return Response.json({ id: "x", object: "response", status: "completed", model: "m", output: [] }); | |
| }; |
🤖 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 `@tests/economic-reservation-terminal.test.ts` around lines 99 - 105, Update
customFetchResponse to read req.body asynchronously using the Request body API
before parsing it, while preserving the existing model check and
context_length_exceeded 400 response. Ensure the fixture reaches the intended
terminal-error branch instead of throwing while parsing the request body.
| test("skips recomputation for an unchanged usage-log revision", async () => { | ||
| appendUsageEntry(usage("one")); | ||
| await refreshEconomicSnapshots(config(), NOW); | ||
| const economy = await import("../src/combos/economy"); | ||
| const setter = spyOn(economy, "setEconomicQuotaSnapshot"); | ||
| await refreshEconomicSnapshots(config(), NOW + 1_000); | ||
| expect(setter).not.toHaveBeenCalled(); | ||
| setter.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
This test passes even when the spy is not wired to the module under test, so it cannot detect a regression.
The only assertion is expect(setter).not.toHaveBeenCalled() at Line 113. A negative assertion on a spy is satisfied in two different situations:
performRefreshcorrectly short-circuits on the unchanged revision key.- The spy never intercepts the call at all.
src/combos/economy-refresh.ts:8-11 binds setEconomicQuotaSnapshot through a static ESM import. If that binding does not route through the patched namespace property, case 2 applies and the revision-cache optimization is never actually verified.
Assert on observable state instead. The snapshot's updatedAt is written from the now argument in snapshotFor at src/combos/economy-refresh.ts:55, so an unchanged updatedAt proves that no recomputation occurred.
♻️ Proposed rewrite that fails on regression
test("skips recomputation for an unchanged usage-log revision", async () => {
appendUsageEntry(usage("one"));
await refreshEconomicSnapshots(config(), NOW);
- const economy = await import("../src/combos/economy");
- const setter = spyOn(economy, "setEconomicQuotaSnapshot");
+ const before = getEconomicQuotaSnapshot("promo")!;
+ expect(before.updatedAt).toBe(NOW);
await refreshEconomicSnapshots(config(), NOW + 1_000);
- expect(setter).not.toHaveBeenCalled();
- setter.mockRestore();
+ // A recomputation would stamp updatedAt with the new `now` argument.
+ expect(getEconomicQuotaSnapshot("promo")).toEqual(before);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("skips recomputation for an unchanged usage-log revision", async () => { | |
| appendUsageEntry(usage("one")); | |
| await refreshEconomicSnapshots(config(), NOW); | |
| const economy = await import("../src/combos/economy"); | |
| const setter = spyOn(economy, "setEconomicQuotaSnapshot"); | |
| await refreshEconomicSnapshots(config(), NOW + 1_000); | |
| expect(setter).not.toHaveBeenCalled(); | |
| setter.mockRestore(); | |
| }); | |
| test("skips recomputation for an unchanged usage-log revision", async () => { | |
| appendUsageEntry(usage("one")); | |
| await refreshEconomicSnapshots(config(), NOW); | |
| const before = getEconomicQuotaSnapshot("promo")!; | |
| expect(before.updatedAt).toBe(NOW); | |
| await refreshEconomicSnapshots(config(), NOW + 1_000); | |
| // A recomputation would stamp updatedAt with the new `now` argument. | |
| expect(getEconomicQuotaSnapshot("promo")).toEqual(before); | |
| }); |
🤖 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 `@tests/economic-snapshot-refresh.test.ts` around lines 107 - 115, Update the
test “skips recomputation for an unchanged usage-log revision” to assert
observable snapshot state instead of relying on the setEconomicQuotaSnapshot
spy. After the second refresh, verify the stored snapshot’s updatedAt remains
equal to the initial refresh time, using the snapshot retrieval path already
exposed by the economy module; remove the ineffective negative spy assertion and
related setup.
2 participants
Summary
"economy"that ranks user-declared interchangeable targets by quota opportunity cost; legacyfailoverandround-robinbehavior is unchanged (selection code paths untouched).economicAllowances(rolling / calendar / fixed-expiry / balance windows); remaining values are process-local runtime snapshots — never persisted, never fetched from provider quota APIs on the request path.remaining - actual); cancel/stream-error releases without burn; stream EOF settles. Multi-allowance reservations are atomic; PAYG picks carry no reservation id.usageMatch.providers/usageMatch.models(unscoped summation documented as experimental).maxMarginalUsdfail-closed on unknown cash cost) → soft reserve/unknown-quota pressure → expiration pressure → marginal cost → stable config order. Explain output splits hardexclusions/hardExclusionsfromsoftSignalswith honestreason.GET /api/combos/<id>/explain,GET /api/economic-allowances, snapshotGET/PUT/DELETEwith 409-until-clearReservationssemantics, and CLIocx combo set/explain,ocx allowance list/snapshot get|set|clear. GUI preserves economy fields on round-trip (no full editor).Verification
bun test tests/economic-*.test.ts tests/cli-allowance.test.ts tests/cli-combo.test.ts tests/cli-help-allowance.test.ts tests/combos.test.ts tests/combo-workspace-data.test.ts→ 196 pass, 0 fail.bun run typecheck→ clean.bun run privacy:scan→ passed (one run this session timed out under Windows I/O contention; a prior run on the same tree passed).cd docs-site && bun run build→ Complete (221 pages).bun run teston this Windows host is environment-noisy (ACL/temp-dir locks, dangling test proxies); focused economy/CLI/GUI suites are green. CI on Linux/macOS will exercise the full suite.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
Documentation