Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The new tests pass and type-check cleanly, but I'd like changes before this goes in: the cost model leaves out cache writes, and the compaction test checks almost nothing about compaction.
What I ran: bun test test/llm-meter.test.ts test/llm-meter-step.test.ts passed 12 of 12, the new session-runner case passes, and bun run typecheck in packages/core is clean. Public API: unchanged (test files only).
Concerns:
- Cache writes are free in the meter (see the inline comment). Real Anthropic billing charges extra for writes, and every tail marker writes on every step. So the policy table, the "default is cheapest" test and the "+88% with no caching" figure all favour placing more markers. Write cost is also what #48542 and #52761 are about.
- The meter never sees a request from the runner. Both meter test files build requests by calling
applyCachePolicydirectly on a fixed prompt, and the oneSessionRunnercase doesn't use the meter. So it measures the policy function on synthetic input, not what an agent step costs. The numbers in the description (68,400 → 3,819 characters, the 20-step table) aren't checked by any test. - The compaction case mostly checks its own inputs (see the inline comment).
shows the default policy is the cheapest…locks today's"auto"choice in under this simplified cost model, so a later policy change that is cheaper on real bills could fail it.
I'd suggest cutting this down to one meter test file that models cache writes, plus either a case that runs the meter through the runner or no runner case at all. The two new files largely repeat each other and packages/ai/test/cache-policy.test.ts.
| inputTokens: total, | ||
| nonCachedInputTokens: uncached, | ||
| cacheReadInputTokens: cached, | ||
| cacheWriteInputTokens: 0, |
There was a problem hiding this comment.
Cache writes are always reported as 0, and relativeCost has no write price. Anthropic charges more for a cache write than for normal input, and every tail marker writes on every step. Without that cost, the meter favours placing more markers: "none" looks much more expensive than it is, and placing more markers looks free. If the meter is meant to judge cache policies by their cost, it needs to count the tokens each marker writes and price them.
| const compaction = context.find((message) => message.type === "compaction") | ||
| expect(compaction?.type === "compaction" && compaction.status === "completed").toBe(true) | ||
|
|
||
| const original = "Earlier question ".repeat(180).length + "Recent exact request ".repeat(180).length |
There was a problem hiding this comment.
This checks very little about compaction:
originalis a constant built from the test's own prompts.- The summary is the scripted
"## Objective\n- measured summary", soreplacement < originalholds whatever compaction does. billablecounts only text, not tools, and the scripted summary is tiny, so the last check passes for any reasonable runner too.
It wouldn't catch a growing recent (#51818) or a summary request that costs more than it saves. Either run it through the meter with a realistic summary size, or drop the case.
| expect(withPolicy.uncached()).toBeLessThan(without.uncached() / 4) | ||
| }) | ||
|
|
||
| it("shows the default policy is the cheapest of the placements that work", () => { |
There was a problem hiding this comment.
This freezes today's "auto" placement as the cheapest under this cost model, which leaves out cache writes. A later policy change that is cheaper on real bills could fail this test. I'd keep only the "none" vs "auto" vs no-tail comparison, or drop the ranking.
26213d5 to
bf6a2c2
Compare
The meter prices cache reads at a tenth of uncached input and writes at 1.25, so a policy that marks more prefixes is not treated as free. bill() applies the same accounting to a sequence of real requests, and the compaction case uses it on SessionRunner's own summary and post-compaction requests instead of summing the test's prompt lengths.
bf6a2c2 to
d6b9ebd
Compare
|
@opencode-agent All four points are addressed in the new head (
For reference, the measured 20-step numbers with writes included are in the PR description: |
Issue for this PR
Closes #56422
Type of change
What does this PR do?
Adds a way to measure what an agent step costs, so changes to context assembly, the cache policy, or compaction can be judged by their effect on the bill instead of by whether a marker still landed where a test expected one.
test/fixture/llm-meter.tsis a provider stand-in that counts the tokens of the request it is handed and reports a cache read only for a prefix an earlier request already wrote, read off the sameCacheHintmarkerscache-policy.tsplaces. It prices both sides of the cache: a read costs 0.1 uncached input tokens and a write 1.25, Anthropic's five-minute multipliers. Writes matter for the questions this is meant to answer: every moving tail marker writes the new turn, so a meter that treats writes as free makes placing more markers look free.bill(requests)prices a sequence of real requests with the same accountingmeter()uses, so a test can meter what the runner actually sent instead of rebuilding the request by hand.The two meter test files are now one (
test/llm-meter.test.ts), and the policy table is reduced to the comparisons that survive a cost-model change:autobeats no tail, no tail beats none, and none reads nothing back. Rankingautoagainsttail: 2andtail: 3was removed because it would fail a future policy that is cheaper on real bills.Measured over 20 steps with the shipped policy and a caching provider (input cost in uncached-token units; output is identical across policies and excluded):
auto(tools + system + one tail)The compaction case now meters
SessionRunner's own requests. The summary request re-sends the old history; the step after it carries the checkpoint. In a 4k-window session the summary request sent 2,838 input tokens; the step after it sent 1,605, of which 388 were read from the prefix the summary request had just written — 43% fewer input tokens. A 785-character summary stands in for the model's output, so a summary that grew past what it replaced would fail the input comparison.How did you verify your code works?
bun run script/test.ts test/llm-meter.test.ts test/session-runner.test.ts— 261 pass, 0 fail.packages/coresuite: 7,463 pass, 0 fail, 28 skipped across 323 files.bun run typecheck:tscinpackages/core— clean.Prettier is clean on the fixture and the meter test.
session-runner.test.tshas one pre-existing formatting difference at line 2091, unrelated to this change, which I left untouched.No production code is touched. The meter lives in
test/fixture/and only reads requests the runner already produced.Screenshots / recordings
None. No user-visible change.
Checklist