Skip to content

feat(agent-core): carry prompt-cache tokens through readTokenUsage - #117

Merged
drewstone merged 2 commits into
mainfrom
feat/token-usage-prompt-cache
Aug 4, 2026
Merged

feat(agent-core): carry prompt-cache tokens through readTokenUsage#117
drewstone merged 2 commits into
mainfrom
feat/token-usage-prompt-cache

Conversation

@drewstone

@drewstone drewstone commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

readTokenUsage is the one hop every sandbox-path token count passes through, and it returned {inputTokens, outputTokens} — dropping every prompt-cache counter the provider reported. The number that survived to a cost formula was therefore neither the tokens billed at the input rate nor the size of the prompt.

The two conventions

Both reach a caller through the SAME OpenAI-compatible endpoint, so the transport cannot tell them apart — only the key that carried the count can. Measured on router.tangle.tools on 2026-08-03, one append-only 4-turn conversation per provider:

provider payload reading
zai / glm-5.2 prompt_tokens=8656, prompt_tokens_details.cached_tokens=8576, completion=42, total=8698 8656 + 42 == 8698 → cached tokens are inside prompt_tokens
anthropic prompt_tokens=39, cache_read_input_tokens=9924, completion=10, total=49 39 + 10 == 49 → the 9,924 are outside prompt_tokens

A reader that ignores the split is wrong in one direction or the other on every call.

What changed

TokenUsageCounts gains cacheReadTokens / cacheWriteTokens, and the three prompt-side fields are now disjoint and additive, which is the shape a cost formula needs (input × p_in + cacheRead × p_read + cacheWrite × p_write):

  • inputTokens — freshly billed tail only
  • promptTokens(usage) — the whole prompt the model saw
  • cacheHitRate(usage) — cached share, undefined when the producer reported nothing, so an unmetered path is never read as a 0% hit rate

Also handled: nested prompt_tokens_details, and bare-named cache bags (opencode's tokens.cache.{read,write}, the router's prompt_cache.{read_tokens,write_tokens}). A provider-native key always outranks the router's normalized echo of the same tokens, so the prompt is never double-counted.

Behavior change

On OpenAI-compatible payloads inputTokens used to return the reported prompt_tokens. The glm-5.2 turn above used to read as 8,656 input tokens; it now reads as 80 input + 8,576 cache-read. Callers pricing inputTokens at the full input rate were overcharging warm calls by the cached share, and now undercharge until they price the cache terms too. Rates measured off the router's per-request x-tangle-price-* headers on 2026-08-03: $0.912/Mtok input, $0.168/Mtok cache read (an 81.6% discount). On a real 5-step agent run that makes whole-prompt-at-full-rate a 2.77x overstatement and tail-only a 40% understatement. Minor-bumped via changeset; the semantic change is spelled out there.

Verification

  • vitest run in packages/agent-core: 434 passed (18 files), including the pre-existing genai-attributes.test.ts vocabulary pins, unchanged.
  • tsc --noEmit: clean.
  • Calibrated: reverting only token-usage.ts and re-running the new test fails 11 assertions, reporting inputTokens: 8656 where the fix reports 80 + cacheReadTokens: 8576. The test cannot pass on the old reader.
  • Test payloads are verbatim captures, not paraphrases.

readTokenUsage returned {inputTokens, outputTokens} and dropped every cache
counter the provider reported, so the one number that survived to a cost
formula was neither the billed input nor the prompt size.

TokenUsageCounts gains cacheReadTokens/cacheWriteTokens, and the three
prompt-side fields are now disjoint: inputTokens is the freshly billed tail and
promptTokens() sums the whole prompt. Two conventions reach callers through the
same OpenAI-compatible endpoint and are normalized here — a cached count
reported INSIDE prompt_tokens is subtracted out, one reported beside it is not
— and a router echo of a provider-native counter is not counted twice.

Payloads in the test are verbatim captures from router.tangle.tools.

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 2aff5b68

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-08-04T02:01:05Z

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 4 (1 medium-concern, 3 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 166.2s (2 bridge agents)
Total 166.2s

💰 Value — sound-with-nits

Correctly fixes readTokenUsage to split prompt-cache counters (OpenAI-inclusive vs Anthropic-exclusive), but adds a 4th ad-hoc cache parser and a 3rd field name to a stack that's already fragmented.

  • What it does: Extends readTokenUsage (packages/agent-core/src/telemetry/token-usage.ts:314) — the shared normalizer that every sandbox-path token count flows through — to recognize prompt-cache counters and split them by the two provider conventions: OpenAI-compatible where cached tokens are INSIDE prompt_tokens (subtracted from the billed tail), vs Anthropic-native where they sit BESIDE it (added). TokenUsageC
  • Goals it achieves: Stop silently discarding every prompt-cache counter the provider reports; make the three prompt-side fields disjoint and additive so a cost formula can bill each at its own rate (input × p_in + cacheRead × p_read + cacheWrite × p_write); stop callers reading inputTokens as 'context size' (it understates a warm loop by the cached share, which is most of it); keep 'no producer reported cache' (undef
  • Assessment: Technically excellent within its scope. The inclusive/exclusive split is real and the PR nails it: ground-truth payloads are pinned verbatim in tests (tests/token-usage-prompt-cache.test.ts:21-67), the native-vs-echo precedence prevents the 2x double-count the router's prompt_cache echo would otherwise cause (token-usage.ts:269-294), the Math.max(0, ...) clamp at token-usage.ts:334 refuses to emit
  • Better / existing approach: Searched the codebase for every other cache-token parser. Found three pre-existing ones this PR does not touch or consolidate: (1) StreamUsageExtractor in packages/agent-core/src/sse/index.ts:502-534 already has cacheReadTokens/cacheWriteTokens on StreamTokenUsage (lines 314-315) and parses cache_read_input_tokens / cache_creation_input_tokens directly from events, treating them as purely additive
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound-with-nits

A correct, well-tested cache-token split on the TokenUsageCounts reader, but the split lands on a library shape whose documented cost path (OTel span attributes) this PR does not extend — so a human should confirm the external cost consumer actually reads TokenUsageCounts and not the un-split sp

  • Integration: @tangle-network/agent-core is a published library: zero non-test packages in this repo import it (grep from "@tangle-network/agent-core" → only README/doc mentions), so absence of in-repo callers is the EXPECTED shape, not dead surface. The new exports (promptTokens, cacheHitRate, the TOKEN_USAGE_CACHE_* vocabularies) are re-exported through src/index.ts:188,209,225-233 and `telemetry/
  • Fit with existing patterns: Mostly fits, with one real architecture gap. The inclusive/exclusive field-name-keyed split is the right design given the documented constraint (both conventions arrive on one OpenAI-compatible endpoint, so the transport can't disambiguate — token-usage.ts:42-63). It is consistent with the codebase's established 'frozen candidate key vocabulary' pattern (TOKEN_USAGE_INPUT_KEYS, `GEN_AI_INPUT_T
  • Real-world viability: Holds up well. Pure functions, no shared state, no concurrency concerns. The Math.max(0, reportedInput - cache.inclusive) clamp (token-usage.ts:334) silently masks a provider reporting cached_tokens > prompt_tokens (a genuine inconsistency) rather than emitting a negative — defensible (a negative corrupts every downstream sum) though it hides a real producer bug. addOptional (`token-usage.
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🎯 Usefulness Audit

🟠 Cache split lands on TokenUsageCounts; the documented cost path reads OTel span attributes, which this PR does not extend [problem-fit] ``

The PR body's stated motivation is that 'the number that survived to a cost formula was neither the tokens billed at the input rate nor the size of the prompt.' But this package documents (genai-attributes.ts:3-5, token-usage.ts:13-16) that the cost path runs over OTel span attributes lowered by genAiUsageAttributes, and agent-trace-contract's cost-reasoning engine keys exclusively off gen_ai.usage.input_tokens / output_tokens (validate.ts:799-844). genAiUsageAttributes (`genai-a

💰 Value Audit

🟡 3 other ad-hoc cache parsers remain unconsolidated; the SSE one has the same bug this PR fixes [better-architecture] ``

StreamUsageExtractor at packages/agent-core/src/sse/index.ts:502-510 and 529-534 parses cache_read_input_tokens / cache_creation_input_tokens directly into its own StreamTokenUsage shape, purely additively — it never asks whether the reported input_tokens already includes the cached share. That is precisely the inclusive-vs-exclusive disagreement this PR exists to fix, so a glm-5.2 stream event will still be mis-counted on the SSE path. packages/agent-provider-cli-bridge/src/index.ts:708-717 is

🟡 Field-name drift: cacheReadTokens/cacheWriteTokens vs the existing cacheReadInputTokens/cacheCreationInputTokens [against-grain] ``

The canonical cross-package TokenUsage type (packages/agent-interface/src/index.ts:368-369) and its zod schema (packages/agent-interface/src/environment-provider.ts:323-324) already standardize on cacheReadInputTokens / cacheCreationInputTokens — the Anthropic-native vocabulary. This PR introduces cacheReadTokens / cacheWriteTokens on TokenUsageCounts (token-usage.ts:147-149), and the SSE layer uses the same new names (sse/index.ts:314-315). Three names now exist for two concepts. Pick one vocab

🟡 Writer side genAiUsageAttributes still drops cache fields on the OTel path [better-architecture] ``

genAiUsageAttributes (packages/agent-core/src/telemetry/genai-attributes.ts:78-92) and its GenAiUsage type (line 58) lower only gen_ai.usage.input_tokens / output_tokens to span attributes; no cache attribute is emitted. If a downstream cost formula reads the OTel attribute bag rather than TokenUsageCounts directly, the cache split this PR captures is lost at the next hop. Either extend GenAiUsage + genAiUsageAttributes to emit the cache counters (OTel GenAI conventions do define cache-read/writ


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260804T020602Z

@tangletools

Copy link
Copy Markdown

✅ No Blockers — 2aff5b68

Review health 100/100 · Reviewer score 61/100 · Confidence 75/100 · 15 findings (1 medium, 14 low)

glm deepseek deepseek-flash aggregate
Readiness 83 89 61 61
Confidence 75 75 75 75
Correctness 83 89 61 61
Security 83 89 61 61
Testing 83 89 61 61
Architecture 83 89 61 61

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 3/3 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 5 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM Behavior-breaking change to public inputTokens field shipped as minor; naive cost callers flip from overcharge to undercharge — packages/agent-core/src/telemetry/token-usage.ts

inputTokens is now max(0, reportedInput - cache.inclusive), so for OpenAI-compatible payloads it drops from the full prompt to the uncached tail (measured zai: 8656 -> 80). A consumer that prices inputTokens at the full input rate previously OVERcharged warm calls (8656fullRate vs the true 80fullRate + 8576cacheRate) and now UNDERcharges them (80fullRate, missing the 8576*cacheRate entirely) until it also reads cacheReadTokens/cacheWriteTokens. The changeset documents the change but never flags this flip, and no consumer in this repo is updated. This is also a semantic change to the OTel gen_ai.usage.input_tokens lowering (genai-attributes.ts:86) which the spec defines as total input tokens, so warm-call input volume is under-reported on that attribute for any consumer feeding readTo

🟡 LOW Additive prompt-size contract is stated as universal but is only evidenced for the cache-read path — .changeset/prompt-cache-token-usage.md

Line 9-10 asserts inputTokens + cacheReadTokens + cacheWriteTokens is always the prompt size, and line 24-28 implies both provider conventions are fully normalized. The PR's measured payloads and tests cover cache-READ only (ZAI cached_tokens, Anthropic cache_read_input_tokens; write is 0 in every fixture). For Anthropic-native calls that report cache_creation_input_tokens (classified in the code as EXCLUSIVE, token-usage.ts:84-91) while also including those creation tokens inside input_tokens, `promptTokens()

🟡 LOW Behavior change on public field documented, but bump is minor — .changeset/prompt-cache-token-usage.md

The changeset flags a 'Behavior change on an existing field' (lines 13-22): public TokenUsageCounts.inputTokens semantics change for OpenAI-compatible payloads (now excludes the cached share). At current 0.4.33 a minor bump is the conventional semver-for-0.x way to make a breaking change, so this is acceptable - but callers pricing inputTokens or reading it as context size will silently change numbers on upgrade. Consider stating the version decision explicitly or noting migration for callers that relied on the old meaning.

🟡 LOW Borderline version bump: minor vs major — .changeset/prompt-cache-token-usage.md

The inputTokens field changes from 'total prompt tokens including cache' to 'uncached tail only'. Callers that used inputTokens for pricing (overcharging) or context sizing (understating) were operating on wrong numbers, so this is arguably a bugfix (patch). But any caller that independently worked around the old semantics now needs to update. The changeset documents this with a concrete measured example (prompt_tokens=8656, cached_tokens=8576 → old: 8656 input, new: 80 input + 8576 cache-read), which is the right mitigation for a minor bump. Consider whether this should be major given the field semantics change.

🟡 LOW Semver bump could be argued as major — .changeset/prompt-cache-token-usage.md

The changeset ships a semantic behavior change on an existing public field (inputTokens now excludes cached share for OpenAI-compatible payloads). For a 1.x+ package this would warrant a major bump; for 0.x (currently 0.4.33) minor is the conventional choice and the change is documented in bold, so this is a nit only. No action required unless the repo enforces strict major-on-behavior-change.

🟡 LOW Bag write fallback can double-count a write echoed beside an inclusive read payload — packages/agent-core/src/telemetry/token-usage.ts

When read resolves from an INCLUSIVE native key, write still falls back to the bag (exclusive, inclusive=0). Probe: {prompt_tokens:10000, cached_tokens:9000, prompt_cache:{write_tokens:9924}} -> inputTokens 1000, cacheReadTokens 9000, cacheWriteTokens 9924, promptTokens() 19924 > the 10000 the provider reported. If prompt_tokens already included the write (inclusive convention), this double-counts. Measured zai payloads carry write_tokens:0 so the observed cases are safe, but the echo-double-count class the comment warns about for reads is only defended on the read side, not the write side.

🟡 LOW Exclusive-first precedence misclassifies a payload carrying both an exclusive and an inclusive native read counter — packages/agent-core/src/telemetry/token-usage.ts

resolve() checks nativeExclusive before nativeInclusive. Probe: {prompt_tokens:8656, prompt_tokens_details:{cached_tokens:8576}, cache_read_input_tokens:8576} -> exclusive wins, inputTokens stays 8656 (cached share NOT subtracted) while cacheReadTokens=8576, so promptTokens() reports 17232 — a 2x double-count of the cached share. The PR's own comment claims a router can carry two spellings of the same tokens, but names the BAG as the second spelling (handled by native-over-bag precedence) rather than a second native key. Not observed in the measured payloads, so this is a robustness gap, not a live bug. Fix: pick convention consistently across all counters (if any native exclusive counter is present, treat the whole response as exclusive) and add a regression test for the dual-native sha

🟡 LOW Inclusive-vs-exclusive ambiguity silently drops one value if both native conventions appear together — packages/agent-core/src/telemetry/token-usage.ts

resolve() returns immediately on the first matching convention (exclusiveKeys, then inclusiveKeys, then bag). If a payload ever carried BOTH an inclusive native key (e.g. cached_tokens) AND an exclusive native key (e.g. cache_read_input_tokens) naming DIFFERENT token sets, the exclusive value wins and the inclusive value is silently dropped — and because inclusive=0 in that branch, inputTokens would not be reduced, producing an inconsistent promptTokens() sum. The doc asserts 'a provider reports ONE convention per counter' and this is corroborated by the two measured router payloads, so there is no evidence this happens today. Flagging only because the failure mode (silent partial drop) is the same shape the rest of the file is at pains to prevent. Mitigation is one assertion-style log if

🟡 LOW Precedence matrix is untested; tests only pin the measured payloads, not the adversarial branches — packages/agent-core/src/telemetry/token-usage.ts

token-usage-prompt-cache.test.ts pins the four measured shapes and the negative clamp, but never exercises (a) a payload carrying BOTH an exclusive and an inclusive native counter (the branch my probe shows double-counts), (b) an inclusive write (cache_write_tokens), (c) cacheHitRate on a write-only or mixed read+write turn, or (d) a read+write turn where write resolution comes from the bag while read comes from a native key. For a reader whose whole correctness argument rests on a precedence order, the untested branch is exactly the one that misclassifies. Fix: add a precedence-matrix table test (exclusive only / inclusive only / bag only / exclusive+inclusive / native+bag for both read and write).

🟡 LOW cacheHitRate denominator includes cacheWriteTokens; write-heavy turns report 0% and writes dilute the read share — packages/agent-core/src/telemetry/token-usage.ts

hitRate = cacheReadTokens / promptTokens(), where promptTokens() includes cacheWriteTokens. A cache-write turn (read 0, write 9924, input 15) reports a 0.0% hit rate, and on a mixed read+write turn writes lower the share. This is internally consistent with the docstring ('share of the prompt served from cache' = read / whole prompt), but readers expecting a conventional hit rate (read / (read + fresh input)) will find write-heavy warm turns surprisingly at/near 0%. Consider documenting that writes are in the denominator, or using read/(read+input) if the metric is meant to be 'hits over cacheable prompt'.

🟡 LOW cacheHitRate guard line is 89 chars; no enforced formatter in repo to catch it — packages/agent-core/src/telemetry/token-usage.ts

The if (usage.cacheReadTokens === undefined && usage.cacheWriteTokens === undefined) { line is the longest in the file at 89 columns. The repo has no .prettierrc/.biome/.eslintrc at root or in agent-core, so nothing enforces a width. The rest of the file consistently wraps at <80 cols (e.g. the addOptional return statement, the readCacheSplit return). Either keep the convention and wrap the condition, or accept the one-off; non-blocking. Cosmetic only.

🟡 LOW readTokenUsage synthesizes inputTokens: 0 for cache-only payloads, contradicting the absent-stays-absent convention — packages/agent-core/src/telemetry/token-usage.ts

For a payload reporting only cache counters (test: {cache_read_input_tokens:1234}), the function returns inputTokens:0 and outputTokens:0, a fabricated zero rather than an absent value. genai-attributes.ts:71-77 explicitly documents that synthesized zeros corrupt downstream aggregates ('absent stays absent'). Lowering this through genAiUsageAttributes would emit gen_ai.usage.input_tokens:0 for a call whose input was never reported. The test pins this behavior deliberately, so it is a convention inconsistency worth deciding, not a crash.

🟡 LOW Freeze test misses 3 new bag-related key arrays — packages/agent-core/tests/token-usage-prompt-cache.test.ts

The freeze test at line 213 iterates over 5 key arrays (TOKEN_USAGE_CACHE_READ_INCLUSIVE_KEYS, TOKEN_USAGE_CACHE_READ_EXCLUSIVE_KEYS, TOKEN_USAGE_CACHE_WRITE_INCLUSIVE_KEYS, TOKEN_USAGE_CACHE_WRITE_EXCLUSIVE_KEYS, TOKEN_USAGE_DETAIL_KEYS) but does not verify the 3 bag-related frozen arrays added in this PR: TOKEN_USAGE_CACHE_BAG_KEYS, TOKEN_USAGE_CACHE_BAG_READ_KEYS, TOKEN_USAGE_CACHE_BAG_WRITE_KEYS. All 3 are Object.freeze'd in the source and would still pass, but the drift guard is incomplete. Add them to the loop.

🟡 LOW Nested Anthropic write bag cache_creation.ephemeral_5m_input_tokens is unpinned and unreadable — packages/agent-core/tests/token-usage-prompt-cache.test.ts

ANTHROPIC_TURN_2 includes cache_creation: { ephemeral_5m_input_tokens: 0 }, but no reader in token-usage.ts can see it: TOKEN_USAGE_DETAIL_KEYS (src/telemetry/token-usage.ts:96-101) lists only prompt/input detail bags and usageBags/cacheCount (lines 219-242) never descend into cache_creation. The test's cacheWriteTokens: 0 (line 91) passes solely because the prompt_cache.write_tokens echo supplies the counter. A real Anthropic turn that reports a cache WRITE only as `ca

🟡 LOW cacheHitRate total===0 and write-only branches untested — packages/agent-core/tests/token-usage-prompt-cache.test.ts

The cacheHitRate tests cover the normal inclusive case (8576/8656) and the undefined case (no cache fields). Two branches in token-usage.ts:402-403 are not exercised: (1) total===0 -> returns 0 (e.g. {inputTokens:0, cacheReadTokens:0}); (2) a write-only turn like ANTHROPIC_TURN_1 yields cacheHitRate 0/(15+0+9924)=0, which is arguably correct but unasserted. Add 1-2 expects to lock the contract. Low impact — neither branch is on a billing-critical path.


tangletools · 2026-08-04T02:14:20Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved — 15 non-blocking findings — 2aff5b68

Full multi-shot audit completed 3/3 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 5 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 5 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-08-04T02:14:20Z · immutable trace

…tributes

Review caught that narrowing inputTokens to the billed tail silently narrows
gen_ai.usage.input_tokens with it, so a warm call's prompt volume would vanish
from the one attribute cost aggregates key off.

genAiUsageAttributes now emits gen_ai.usage.cache_read_input_tokens and
gen_ai.usage.cache_creation_input_tokens beside it — omitted entirely when the
producer reported no cache information, since a synthesized 0 reads as
'measured, no cache'. The three sum back to the whole prompt.

The changeset now also spells out that the pricing error FLIPS direction: a
caller pricing inputTokens at the full rate used to overcharge warm calls and
will now undercharge until it prices the cache terms too.

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 76ba0ba0

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-08-04T02:28:49Z

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Value Audit — sound

Verdict sound
Concerns 0 (none)
Heuristic 0.0s
Duplication 0.0s
Interrogation 140.7s (2 bridge agents)
Total 140.7s

💰 Value — error

value agent produced no parseable value-audit JSON.

  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge error: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

Fixes a real, measured correctness bug — readTokenUsage was collapsing cached and uncached prompt tokens into one number that was neither the billed input nor the prompt size — and does it in the grain of the codebase, consistently with the existing StreamTokenUsage/GenAiUsage shapes.

  • Integration: readTokenUsage/addTokenUsage/TokenUsageCounts/genAiUsageAttributes are exported from @tangle-network/agent-core (v0.4.33, published SDK; packages/agent-core/package.json). No in-repo production caller exists, but that is expected: the token-usage.ts:1-17 header explicitly names the consumers ('the trace sink's root-span aggregation, the signal extractor') which live downstream, and t
  • Fit with existing patterns: Excellent. (1) The chosen field names cacheReadTokens/cacheWriteTokens match the pre-existing StreamTokenUsage in the same package (packages/agent-core/src/sse/index.ts:314-315) and the writer-side GenAiUsage this PR also extends — so the package now speaks one cache-token vocabulary across reader, accumulator, and attribute writer. (2) The reader correctly bridges the distinct wire sche
  • Real-world viability: Robust across the three real provider shapes it was measured against (OpenAI-inclusive prompt_tokens, Anthropic-exclusive cache_read_input_tokens, opencode bare cache.{read,write} bag). The precedence rule in readCacheSplit (token-usage.ts:277-294) — provider-native key outranks the router's prompt_cache echo — prevents the one genuinely nasty failure mode (double-counting when the route
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

No concerns — sound change, no better or existing approach found. ✅


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260804T035141Z

@tangletools

Copy link
Copy Markdown

✅ No Blockers — 76ba0ba0

Review health 100/100 · Reviewer score 61/100 · Confidence 75/100 · 20 findings (1 medium, 19 low)

glm deepseek deepseek-flash aggregate
Readiness 77 80 61 61
Confidence 75 75 75 75
Correctness 77 80 61 61
Security 77 80 61 61
Testing 77 80 61 61
Architecture 77 80 61 61

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 3/3 planned shots over 6 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 6 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 6 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM inputTokens semantic change silently flips the pricing error direction for every external consumer — packages/agent-core/src/telemetry/token-usage.ts

For OpenAI-compatible payloads, inputTokens used to return the full reported prompt_tokens and now returns it minus the cached share. The changeset documents this and admits the pricing error flips from overcharge to undercharge. But agent-core has ZERO in-repo consumers of readTokenUsage (verified by rg across packages/ — only agent-core src + tests match), so every real consumer (trace sink, opencode telemetry, cost aggregators) lives outside this repo and must be updated in lockstep, or a warm call's inputTokens silently under-reports and any inputTokens-only cost formula undercharges. This is a release-coordination hazard on a published SDK (@tangle-network/agent-core 0.4.33, minor bump): verify downstream repos are migrated before publish, not after.

🟡 LOW Additive invariant is stated unconditionally but breaks on clamped malformed payloads — .changeset/prompt-cache-token-usage.md

Lines 7-11 state inputTokens + cacheReadTokens + cacheWriteTokens 'is the size of the prompt the model saw', presented as guaranteed. readTokenUsage clamps a negative billed tail via Math.max(0, reportedInput - cache.inclusive) (token-usage.ts:334), so a malformed payload like the test's {prompt_tokens:10, cached_tokens:999} yields inputTokens=0 + cacheReadTokens=999, and the sum (999) overstates the 10-token prompt. The invariant holds for well-formed provider data only. Impact: a downstream consumer trusting the additive identity verbatim on a broken payload gets an overstated context size (safe for cost since cache is cheap, wrong for prompt-volume a

🟡 LOW Semver bump is 'minor' but inputTokens behavior change is breaking — .changeset/prompt-cache-token-usage.md

The changeset declares minor for @tangle-network/agent-core, but inputTokens changes meaning: it previously included the cached share (the full prompt_tokens), and now excludes it, returning only the freshly-billed tail. The changeset itself documents this as a behavioral change that flips the billing direction (lines 21-24: 'callers must be updated, not just re-read'). A consumer relying on semver to detect compatibility breaks would not expect this from a minor bump. Consider major.

🟡 LOW minor bump carries a documented behavior change on a public field — .changeset/prompt-cache-token-usage.md

The changeset explicitly states 'callers must be updated, not just re-read' because inputTokens semantics flip for OpenAI-compatible payloads (now excludes cached share). By strict semver this is a breaking change on a public field and could warrant 'major'. For a 0.4.x pre-1.0 package this is defensible (semver §4: 0.y.z is unstable), and the changeset is admirably explicit about the migration (cost formula + 'use promptTokens() never inputTokens for context size'). No action required unless the repo has a stricter semver policy; flagged only so the global verifier can weigh it.

🟡 LOW minor bump for a semantic change to an existing public field that requires caller updates — .changeset/prompt-cache-token-usage.md

The changeset itself says 'callers must be updated, not just re-read' and documents that inputTokens changes meaning for OpenAI-compatible payloads, silently flipping existing cost formulas from overcharge to undercharge. A minor bump is defensible (the old behavior was a bug and the migration path is spelled out), but a strict semver reading would call the meaning change to a public field breaking and use major. Flagging so the author consciously chose minor; no change required if the bug-fix framing is accepted.

🟡 LOW New GenAI cache attributes have no reader-side candidate list; drift guard only covers legacy keys — packages/agent-core/src/telemetry/genai-attributes.ts

GEN_AI_USAGE_CACHE_READ_INPUT_TOKENS / GEN_AI_USAGE_CACHE_CREATION_INPUT_TOKENS are exported writer keys, but the co-located reader vocabulary (GEN_AI_*_TOKEN_KEYS) has no cache entries and the drift-guard test (genai-attributes.test.ts:54-58) only asserts the legacy first-candidate pairing. Any external ingest that wants to accept foreign spellings of these attributes (e.g. llm.token_count variants) must hardcode the literal keys. Consider adding reader candidate lists and a drift-guard assertion, and confirming the ingest consumes the exact literals.

🟡 LOW Export list missing TOKEN_USAGE_CACHE_BAG_READ_KEYS and TOKEN_USAGE_CACHE_BAG_WRITE_KEYS from telemetry/index.ts — packages/agent-core/src/telemetry/index.ts

The telemetry/index.ts barrel re-exports TOKEN_USAGE_CACHE_BAG_KEYS (line 80), TOKEN_USAGE_CACHE_READ_EXCLUSIVE_KEYS (line 83), and TOKEN_USAGE_CACHE_WRITE_* keys (lines 85-86), but does NOT export TOKEN_USAGE_CACHE_BAG_READ_KEYS or TOKEN_USAGE_CACHE_BAG_WRITE_KEYS. However, TOKEN_USAGE_CACHE_BAG_READ_KEYS IS exported from the top-level index.ts ([line 229](https://github.c

🟡 LOW Anthropic nested cache_creation bag (ephemeral_5m_input_tokens) is dropped — cache write silently lost — packages/agent-core/src/telemetry/token-usage.ts

The repo's own pinned ANTHROPIC_TURN_2 payload (token-usage-prompt-cache.test.ts:52) carries cache_creation: { ephemeral_5m_input_tokens: 0 }, but usageBags/readCacheSplit only ever traverse TOKEN_USAGE_DETAIL_KEYS (prompt/input_tokens_details) and TOKEN_USAGE_CACHE_BAG_KEYS (cache/prompt_cache/promptCache) — cache_creation is never inspected. Reproduced: {prompt_tokens:15, completion_tokens:14, cache_creation:{ephemeral_5m_input_tokens:9924}} returns {inputTokens:15, outputTokens:14} with no cacheWriteTokens; a 9,924-token write vanishes. The measured router path captures writes via the flat cache_creation_input_tokens key (TURN_1), so real traffic is covered today, but this is a reachable Anthropic spelling that the reader claims to normalize and currently drops. Add cache_creation to th

🟡 LOW Inconsistent convention for the flat cache_*_tokens family: read classified EXCLUSIVE, write classified INCLUSIVE — packages/agent-core/src/telemetry/token-usage.ts

cache_read_tokens / cacheReadTokens sit in TOKEN_USAGE_CACHE_READ_EXCLUSIVE_KEYS (lines 72-79) while cache_write_tokens / cacheWriteTokens sit in TOKEN_USAGE_CACHE_WRITE_INCLUSIVE_KEYS (line 81-82) — the identical <verb>_tokens naming pattern assigned opposite conventions. If a producer emits flat cache_read_tokens + cache_write_tokens (a consistent family), the write share is subtracted from the billed tail and the read share is not. Reproduced: {prompt_tokens:1000, cache_read_tokens:900, cac

🟡 LOW No test covers native-exclusive vs native-inclusive key conflict on the same payload — packages/agent-core/src/telemetry/token-usage.ts

The resolve() function checks nativeExclusive before nativeInclusive and returns on first match. The existing tests cover native-vs-bag precedence (ZAI has cached_tokens inclusive + prompt_cache bag; ANTHROPIC has cache_read_input_tokens exclusive + prompt_cache bag) but no test asserts the outcome when BOTH a native exclusive key (e.g. cache_read_input_tokens) AND a native inclusive key (e.g. cached_tokens) appear in the same payload. The measured real-world data shows one convention per provider, so this is theoretical, but the precedence is load-bearing for cost correctness and a pinned test would lock it against a future reorder. Fix: add a test case like { prompt_tokens: 100, cached_tokens: 50, cache_read_input_tokens: 200 } asserting exclusive wins (cacheReadTokens=200, inputTokens=1

🟡 LOW cacheHitRate condition exceeds typical line width — packages/agent-core/src/telemetry/token-usage.ts

The guard if (usage.cacheReadTokens === undefined && usage.cacheWriteTokens === undefined) { is a single 89-char line. The rest of the file wraps multi-condition ifs. Cosmetic consistency nit only; no behavior impact.

🟡 LOW Anthropic nested cache_creation.ephemeral_5m_input_tokens write counter is unpinned and unreadable — packages/agent-core/tests/token-usage-prompt-cache.test.ts

ANTHROPIC_TURN_2 carries the real Anthropic API write shape cache_creation:{ephemeral_5m_input_tokens:0}, but TOKEN_USAGE_CACHE_WRITE_*_KEYS only contain cache_creation_input_tokens/cache_creation_tokens/cache_write etc. — the nested ephemeral_5m key is in no vocabulary. The pinned payload writes 0, so the tests can neither prove a nonzero write in this shape is captured (it would be silently dropped: write resolves to undefined unless prompt_cache.write_tokens exists) nor pin a regression reading it. Turn 1 papers over this by using the router-normalized top-level cache_creation_input_tokens:9924. The file claims these payloads are 'ground truth' — worth either a comment that the router normalizes this shape (hence the top-level key) or a fixture that pins behavior for the nested-only cas

🟡 LOW Freeze-vocabulary test omits 3 of the 8 frozen key arrays — packages/agent-core/tests/token-usage-prompt-cache.test.ts

The test asserts Object.isFrozen on 5 arrays (CACHE_READ/WRITE_INCLUSIVE/EXCLUSIVE + DETAIL_KEYS) but the source also Object.freeze's TOKEN_USAGE_CACHE_BAG_KEYS, TOKEN_USAGE_CACHE_BAG_READ_KEYS, and TOKEN_USAGE_CACHE_BAG_WRITE_KEYS (token-usage.ts:117-128). These bag-key arrays are load-bearing for the opencode-shape fallback tested at line ~117, so an accidental unfreeze + push would not be caught. Fix: import and add the three to the loop. Cosmetic; no runtime impact today.

🟡 LOW Native-vs-echo precedence is never actually distinguished by the fixtures — packages/agent-core/tests/token-usage-prompt-cache.test.ts

Test 'lets a provider-native inclusive key outrank the router's echo' (and test 5 'does not double-count') assert against ZAI_TURN_2 and ANTHROPIC_TURN_2, where prompt_cache.read_tokens (8576 / 9924) EXACTLY equals the provider-native counter (cached_tokens / cache_read_input_tokens). The precedence rule readCacheSplit implements (native exclusive > native inclusive > bag) cannot fail these tests: a reader that returned the echo's value instead of the native one would still yield cacheReadTokens=8576 and, via the same inclusive subtraction, inputTokens=80. Tests 1 and 7 do catch mis-classifying the echo as exclusive (inputTokens would be 8656) and test 5 catches summing both (17152), but the 'native wins' half of the rule is unobservable. Fix: add a fixture where prompt_cache.read_tokens d

🟡 LOW No explicit cacheHitRate=0 edge case for a measured-zero prompt — packages/agent-core/tests/token-usage-prompt-cache.test.ts

cacheHitRate (token-usage.ts:398) has a total === 0 ? 0 : ... guard for the degenerate case where promptTokens sums to 0 with defined cache counters (e.g. {inputTokens:0, cacheReadTokens:0}). That branch is not exercised; the only hit-rate assertions are the ZAI ratio and the undefined-unmetered case. Low risk since the guard is a one-liner, but it is an uncovered branch.

🟡 LOW Test 'never emits a negative billed tail' expects cacheWriteTokens: undefined in an object that never has that key — packages/agent-core/tests/token-usage-prompt-cache.test.ts

The toEqual expected object includes cacheWriteTokens: undefined, but readTokenUsage omits the key entirely when cache.write is undefined (spread is conditional). vitest toEqual treats absent-key as equal to undefined-key, so the test passes, but the notation is misleading — a reader may think the function emits an explicit undefined. Fix: remove cacheWriteTokens: undefined from the expected object for clarity, matching the style of the 'stays undefined' test at line 187.

🟡 LOW cacheWriteTokens: undefined in expected object is a no-op under toEqual — packages/agent-core/tests/token-usage-prompt-cache.test.ts

Test 'never emits a negative billed tail' asserts toEqual({ ..., cacheWriteTokens: undefined }). Vitest's toEqual ignores undefined-valued keys, so the assertion passes whether the key is absent or present-as-undefined — it pins only the three defined fields, not the write counter's absence. The absent-vs-zero semantics the suite otherwise cares about are genuinely pinned only in test 13 via expect(addTokenUsage(plain, plain).cacheReadTokens).toBeUndefined(). No behavior bug; consider asserting Object.prototype.hasOwnProperty or dropping the key from the expected literal for honesty.

🟡 LOW imprecise toEqual with explicit undefined — packages/agent-core/tests/token-usage-prompt-cache.test.ts

The expected object includes cacheWriteTokens: undefined, but readTokenUsage omits absent keys entirely. This relies on vitest's toEqual treating absent and undefined as equivalent. Not a bug, but could mask a future regression if toEqual behavior changes.

🟡 LOW missing cacheHitRate coverage for write-only cache — packages/agent-core/tests/token-usage-prompt-cache.test.ts

cacheHitRate returns a defined number when cacheWriteTokens is present even if cacheReadTokens is undefined (hit rate = 0/promptTokens). No test exercises this path.

🟡 LOW missing cacheHitRate zero-total coverage — packages/agent-core/tests/token-usage-prompt-cache.test.ts

When promptTokens returns 0 and cache was measured, cacheHitRate returns 0 rather than undefined. No test covers this denominator-zero edge.


tangletools · 2026-08-04T04:00:24Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved — 20 non-blocking findings — 76ba0ba0

Full multi-shot audit completed 3/3 planned shots over 6 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 6 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 6 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-08-04T04:00:24Z · immutable trace

@drewstone
drewstone merged commit b449679 into main Aug 4, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants