Skip to content

fix(supervise): make the token charge additive over aggregated spend - #837

Merged
drewstone merged 3 commits into
mainfrom
fix/charged-tokens-additive-20260813
Aug 14, 2026
Merged

fix(supervise): make the token charge additive over aggregated spend#837
drewstone merged 3 commits into
mainfrom
fix/charged-tokens-additive-20260813

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Follow-up to #835, from an independent adversarial pass over that merged diff. Refs #831.

#835 shipped the right unit and then computed it in a form that is not additive. Two defects, both reproduced before fixing.

1. One unclassified turn re-charged every cached prefix beside it

chargedTokens read freshInput + cacheWrite under a complete split and fell back to input + output otherwise. addTokenUsage legitimately folds a classified turn together with an unclassified one, producing partial class totals plus cacheBreakdownKnown: false — and the fallback then applied to the WHOLE aggregate:

record A  {input: 1000, freshInput: 100, cacheRead: 900, cacheWrite: 0}   charges  100
record B  {input:   10}                                                   charges   10
aggregate {input: 1010, freshInput: 100, cacheRead: 900, cacheWrite: 0}   charges 1010   ← measured

The charge is now input - cacheRead + output. Identical under a complete split (input - cacheRead === freshInput + cacheWrite), and both terms accumulate through addTokenUsage, so the charge on an aggregate equals the sum of the charges on the records that built it. The unclassified remainder is charged in full; only reported cache reads are credited.

Two guards keep the subtraction from ever buying free tokens:

  • a cache class that exceeds the prompt total it partitions credits nothing, so bad telemetry can over-charge but never under-charge;
  • a zero prompt total charges no prompt tokens ({input: 0, output: 1, freshInput: 5} charged 6 before, charges 1 now).

2. A resumed pool died at construction on real telemetry

assertValidSpend demanded an exact partition from every spend carrying all three classes. The aggregate above carries all three and sums to 1000 against input: 1010, so:

createBudgetPool({...}, now, { committed: aggregate })
  → Error: budget restore committed.tokens cache classes must sum to input   ← measured

Every resumed supervisor whose journal mixes a classified and an unclassified turn aborted before admitting any work. Same throw on reconcile, from assertValidSpend before the ticket closes, which additionally leaks the reservation into the join barrier.

An incomplete split — the shape cacheBreakdownKnown: false exists to describe — now only has to FIT inside input. A spend that CLAIMS a complete split must still partition input exactly, and classes that exceed input are still refused with a distinct message. The invariant that lets the charge credit a cache read is unchanged; only the claim-to-be-complete case enforces equality.

3. equalKOnCost follows

A rolled-up arm containing one unclassified node reported 1010 against a pool that charged 110. It now reports what the pool charged.

Not fixed here, and why

  • Sandbox paths drop readable cache telemetry (src/runtime/sandbox-events.ts:106, src/runtime/supervise/sandbox-session.ts:217, src/runtime/run-loop.ts:750): an llm_call carrying promptCache.readTokens still yields {input, output, cacheBreakdownKnown: false}, so those runs charge the conservative upper bound. That is Token budget charges cached prompt reads at full weight; 98% of counted spend is cache #831's item 3 — populate the split on the bridge path — and it is the load-bearing next step, not a regression from this change.
  • assertValidSpend throws before the ticket closes (src/runtime/supervise/budget.ts reconcile, pre-existing). The file's own header says a throw placed before settlement is how a ticket escapes the pool. This change removes the common trigger; deciding what to CHARGE for a spend the pool cannot trust is a separate fail-loud design question and gets its own PR.
  • Restored uncertain reservations do not set cacheBreakdownKnown: false. Deliberate, and the field doc now states it: a declared ceiling has no cache composition to misread, and tokensKnown: false already reports that the balance is not a measurement.

Proof

New tests, each red on #835's code:

  • an aggregate charges exactly what its records charged, mixing classified and unclassified turns (folded pool balance === per-turn pool balance === 1_000_000 − 110)
  • a cache class exceeding the prompt total credits nothing
  • a zero prompt total charges no prompt tokens
  • an incomplete split covering part of input restores; one exceeding input still throws
  • equalKOnCost rates a mixed rolled-up arm at 110, not 1010

pnpm run typecheck, pnpm run lint, pnpm run docs:freshness pass. pnpm test: 211 files passed, 2 skipped; 2632 tests passed, 6 skipped, 0 failed.

`chargedTokens` read `freshInput + cacheWrite` under a complete split and fell
back to `input + output` otherwise. An aggregate that folded one classified turn
with one unclassified turn took the fallback for the WHOLE aggregate, so a single
unreported turn re-charged every cached prefix beside it: two records charging 100
and 10 charged 1010 together.

Charge `input - cacheRead + output` instead. It is the same number under a
complete split, and both terms accumulate through `addTokenUsage`, so the charge
on an aggregate equals the sum of the charges on the records that built it. An
unclassified remainder is charged in full and only reported cache reads are
credited. A cache class that exceeds the prompt total it partitions credits
nothing, so bad telemetry can over-charge but never buy free tokens, and a
zero prompt total charges no prompt tokens.

`assertValidSpend` demanded an exact partition from every spend that carried all
three classes. Aggregation legitimately produces partial class totals marked
`cacheBreakdownKnown: false`, which is the shape the flag exists to describe; a
pool restored from such a record threw at construction and killed the resumed run.
An incomplete split now only has to fit inside `input`. A spend claiming a
complete split must still partition `input` exactly.

Refs #831

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — f10ff8ed

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.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-14T00:07:46Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

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
Coverage 2 of 2 lenses (value, usefulness)
Concerns 1 (1 low)
Heuristic 0.0s
Duplication 0.2s
Interrogation 96.6s (2 bridge agents)
Total 96.8s

💰 Value — sound

Rewrites the token-charge unit as input − cacheRead + output so it is additive over aggregated spend, and relaxes assertValidSpend to accept the partial-class aggregate shape that addTokenUsage legitimately produces — a correct, in-grain fix with no better alternative.

  • What it does: Two coupled fixes. (1) chargedTokens (src/runtime/util.ts:196-205) changes from freshInput + cacheWrite + output (with a whole-aggregate fallback to input + output when the cache split is incomplete) to input − creditedCacheRead + output, where creditedCacheRead returns the reported cacheRead only when the classified classes do not exceed the prompt total. Because both input and cacheRead ac
  • Goals it achieves: Make the conserved token charge additive so that a child's settlement, a driver observation, a restored committed spend, and a rolled-up trajectory arm all charge the same total — eliminating the 10x over-charge where one unclassified turn re-charged every cached prefix beside it, and unblocking pool construction on the real telemetry shape addTokenUsage produces. Secondary goal: keep the fail-saf
  • Assessment: Sound and in the grain of the codebase. The math is correct: input and cacheRead both distribute through addTokenUsage, so input − cacheRead + output is additive in the valid domain (classified ≤ input, which assertValidSpend now enforces), and in the degenerate over-report domain creditedCacheRead floors the credit at 0 so the charge can only go up, never below the true cost. The fix reuses the s
  • Better / existing approach: none — this is the right approach. Searched for alternatives: (a) precomputing and storing a charged field on Spend/LoopTokenUsage so each consumer avoids recomputing — this would duplicate the canonical computation across the accumulate and charge sites and risk silent divergence, strictly worse than the current single-sourced chargedTokens; (b) pushing the charge into addTokenUsage as a runnin
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

Makes the conserved token charge additive so the within-run pool and the cross-run roll-up agree, and relaxes the restored-spend partition check so a resumed supervisor with mixed classified/unclassified telemetry stops dying at construction.

  • Integration: Fully wired onto hot paths. chargedTokens is the single unit shared by budget.ts (reconcile/observe/restore) and trajectory.ts equalKOnCost (the cross-run check whose doc at trajectory.ts:163-166 requires agreement with the pool). The assertValidSpend relaxation unblocks the real resume path at supervisor.ts:482-483, which feeds createBudgetPool an aggregate built by addTokenUsage over many settle
  • Fit with existing patterns: Squarely in the grain. The architecture already rests on chargedTokens being the one unit both ends use; #835 shipped the unit but in a non-additive form, so the cross-run roll-up (chargedTokens over an addTokenUsage aggregate) could disagree with the per-turn pool. This change makes the unit actually satisfy the contract the surrounding code already assumes. No competing pattern exists.
  • Real-world viability: Holds under realistic and edge inputs. Bad telemetry with cacheRead exceeding input credits nothing (util.ts:204), so it can only over-charge, never mint capacity; zero-prompt observations charge no prompt tokens; mixed classified/unclassified aggregates charge the unclassified remainder in full and credit reported cache reads. The single additivity-breaking case (one record with corrupt cacheRead
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: magic number added tests/kernel/supervise.test.ts

  • // One classified turn (100 new of 1000 prompt) and one turn the provider never classified.

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 · 20260814T002306Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — f10ff8ed

Review health 100/100 · Reviewer score 73/100 · Confidence 85/100 · 8 findings (2 medium, 6 low)

opencode GLM 5.2 opencode DeepSeek v4 Pro opencode DeepSeek v4 Flash aggregate
Readiness 92 82 73 73
Confidence 85 85 85 85
Correctness 92 82 73 73
Security 92 82 73 73
Testing 92 82 73 73
Architecture 92 82 73 73

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

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

🟠 MEDIUM Partial cacheRead is credited, weakening the fail-closed upper bound and overstating the invariant — src/runtime/util.ts

creditedCacheRead (util.ts:200-205) returns cacheRead whenever the classified sum does not exceed input, even when freshInput/cacheWrite are absent. The doc at util.ts:190-191 claims 'an over-reported cacheRead credits nothing, so bad cache telemetry can only over-charge, never buy free tokens' — that guarantee holds only for a COMPLETE split (where classified>input is detectable). For a partial split it is false: a provider reporting input=1000, cacheRead=900 (true read 300) with no freshInput/cacheWrite yields classified=900<=1000, so charge=input-cacheRead+output=100 instead of the true ~700, under-charging the token budget by 600. This partial shape is reachable: the bridge executor emits promptCache with independently-optional fields (runtime.ts:3830-3872, mapped to cacheRead/freshI

🟠 MEDIUM Additivity claim untested and false when an over-reported cacheRead is folded with an unclassified turn — tests/kernel/supervise.test.ts

The test locks in folded==perTurn for the under-partition aggregate (classified < input), but the same equality fails for an over-report record. Repro against head (chargedTokens + addTokenUsage): turnA {input:10, output:0, freshInput:5, cacheRead:10, cacheWrite:0} + turnB {input:100, output:0} → per-turn charges 10 + 100 = 110; the folded aggregate {input:110, cacheRead:10, freshInput:5, cacheBreakdownKnown:false} charges 100. The over-reported cacheRead (classified 15 > turnA input 10, credit denied per-turn) is credited at the aggregate because classified(15) <= aggregate input(110). Worse, the per-turn path REJECTS turnA ('cache classes must not exceed input') while the folded path silently accepts and credits it — folded accounting is less strict than per-turn. This contradicts the te

🟡 LOW Changelog invariant phrased absolutely but enforcement requires all three cache classes present — CHANGELOG.md

Bullet 2 states 'Bad cache telemetry can over-charge; it can never buy free tokens.' The enforcement is narrower than the phrasing: assertValidSpend (src/runtime/supervise/budget.ts:169) only validates the class sum when freshInput, cacheRead AND cacheWrite are all defined, and creditedCacheRead (src/runtime/util.ts:203-204) credits any cacheRead where classified <= input. A spend carrying only cacheRead <= input (no freshInput/cacheWrite) passes validation and is still credited — that shape could under-charge if the reported cacheRead were fabricated. Not exploitable today: both runtime producers (routerPromptCacheUsage src/runtime/supervise/runtime.ts:834-861 and driverPromptCacheUsage src/runtime/supervise/coordination-driver.ts:911-933) emit all-or-none classes, so the lone-cacheRead s

🟡 LOW Doc omits the over-report clamp edge of the charge formula — docs/architecture.md

The doc states the charge is input - cacheRead + output and 'charges each token ONCE'. The implementation credits cacheRead only when the classified classes do not exceed input (src/runtime/util.ts:200-205); an over-reported cacheRead credits nothing, so those tokens are charged in full, not once. This is an intentional fail-safe (bad telemetry can only over-charge), documented in the code comment (util.ts:190-195), and the doc's following sentence already covers the unclassified/full-charge case. Suggest a parenthetical for precision: e.g. 'a cache class exceeding the prompt total it partitions credits nothing.' Purely cosmetic; no behavior or correctness impact.

🟡 LOW assertValidSpend skips partition validation for partial splits, so a lone cacheRead is credited without any sum check — src/runtime/supervise/budget.ts

assertValidSpend (budget.ts:168-185) only runs the partition check when all three of freshInput/cacheRead/cacheWrite are present. A Spend carrying only cacheRead (freshInput/cacheWrite undefined) bypasses both branches of the check entirely, yet chargedTokens still credits that cacheRead. Combined with the primary finding, an over-reported lone cacheRead <= input passes validation and under-charges. The new tests cover complete-split-exceeds and incomplete-split-covers-only-part, but none cover a lone cacheRead over-report within the total. Low because it is the same root trust gap as the primary finding and the token channel is not a hard security boundary (a lying provider can already under-report input/output), but the validation asymmetry is worth closing or documenting.

🟡 LOW Additivity/over-charge-only guarantee assumes per-delta classes never exceed their own input; not enforced in the fold — src/runtime/util.ts

chargedTokens now credits the aggregate cacheRead whenever classified <= input (util.ts:200-205). If a single delta over-reports classes relative to its own input (e.g. {input:1000, cacheRead:1500}), folding it with an under-classified turn (e.g. {input:2000, cacheRead:100}) yields aggregate classified=1600 <= input=3000, so the aggregate credits 1600 while the per-record charges would be 1000 and 1900 — the aggregate charge (1400) is below the sum of its records (2900) and below true newly-presented work. This contradicts the docstrings in util.ts:190-194 and budget.ts:23-25 claiming 'bad cache telemetry can only over-charge, never buy free tokens', and assertValidSpend (budget.ts:182) cannot catch it because it checks the aggregate sum, not per-delta consistency. Impact: theoretical unde

🟡 LOW Partial cacheRead crediting weakens the documented 'upper bound' guarantee for the token charge — src/runtime/util.ts

OLD chargedTokens returned usage.input + usage.output for any non-complete split — a guaranteed over-estimate of newly-presented work. NEW creditedCacheRead subtracts cacheRead whenever classified <= input, even when freshInput/cacheWrite are undefined (partial split). Concrete case: { input: 100, cacheRead: 80 } (freshInput/cacheWrite missing) now charges 100 - 80 + output = 20 + output, where the old code charged 100 + output. If a provider over-reports cacheRead within input bounds (e.g. true cache hits were 0, not 80), the charge drops BELOW the true newly-presented count, so the BudgetReadout.cacheBreakdownKnown=false flag no longer marks an upper bound — it only marks 'not a measurement'. This contradicts the util.ts:190-194 doc claim 'bad cache telemetry can only over-charge, neve

🟡 LOW New tests do not assert the credit cap on a negative-charge-shaped input — tests/kernel/supervise.test.ts

Test 3 covers the single-spend over-report (cacheRead 4000 > input 10) and correctly asserts tokensLeft 990. It does not assert the invariant that matters for the over-report family: chargedTokens never goes negative for any input (e.g. {input:0, cacheRead:1} charges 0, not -1). creditedCacheRead's classified > input guard does bound this (credit <= classified <= input), but with the aggregate gap above unresolved, an assertion on the aggregate's floor (charge >= output) would pin the real safety property. Minor: strengthen the test to assert the aggregate charge never drops below output when a folded record over-reports.


tangletools · 2026-08-14T00:35:57Z · trace

tangletools
tangletools previously approved these changes Aug 14, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 8 non-blocking findings — f10ff8ed

Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 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-14T00:35:57Z · immutable trace

`chargedTokens` denies a credit when the reported classes overflow the prompt
total they partition. The fold did not: `addTokenUsage` accumulated the classes
of an overflowing turn, and the accumulator's larger `input` then absorbed the
overflow, so the aggregate credited a cache read the record refuses.

    turn A {input: 10,  freshInput: 5, cacheRead: 10}  charges  10 (credit denied)
    turn B {input: 100}                                charges 100
    folded {input: 110, freshInput: 5, cacheRead: 10}  charged 100  ← under-charge

A delta whose classes do not fit inside its own `input` now contributes its
prompt total and no classes, so the aggregate charge equals the sum of the
charges on the records that built it, and the charge never falls below the
output tokens.

The docstrings claimed bad cache telemetry could only ever over-charge. That
overstated it: a provider reporting a cache read it never served is trusted
exactly as far as its reported `input`, and the token channel is an accounting
unit, not a trust boundary. The claim now states the arithmetic guarantee it
actually holds.

Refs #831
@drewstone

Copy link
Copy Markdown
Contributor Author

Addressed the audit on f10ff8ed. Both MEDIUM findings were the same real defect and are fixed in 7ec02735.

Fixed — the fold rescued a credit the record refuses. chargedTokens denies a credit when the reported classes overflow the prompt total they partition, but addTokenUsage still accumulated those classes, and the accumulator's larger input then absorbed the overflow:

turn A {input: 10,  freshInput: 5, cacheRead: 10}  charges  10  (credit denied)
turn B {input: 100}                                charges 100
folded {input: 110, freshInput: 5, cacheRead: 10}  charged 100  ← under-charge

A delta whose classes do not fit inside its own input now contributes its prompt total and no classes. The aggregate charge equals the sum of the charges on its records again, and the folded path is no longer laxer than the per-turn path. Your exact repro is now a test (does not let a fold rescue a cache read that overflowed its own turn), asserting folded balance === per-turn balance === 1000 − 110. Your LOW finding about a per-delta over-report folded with an under-classified turn is the same root and is closed by the same change.

Fixed — the docstring overclaimed. Four findings correctly attacked "bad cache telemetry can only over-charge, never buy free tokens". That is false for a lone cacheRead inside input: a provider reporting a cache read it never served under-charges. The guarantee the code actually holds is arithmetic — classes that do not FIT inside the total they partition credit nothing, so the charge never drops below output. util.ts, the budget.ts header, docs/architecture.md, docs/glossary.md and the changelog now say that, and say plainly that the token channel is an accounting unit, not a trust boundary against a provider that misreports its own usage.

Not changing — crediting a partial cacheRead is the fix, not a hole in it. A spend carrying cacheRead without freshInput/cacheWrite is credited on purpose. Refusing it would restore the exact defect #831 reports for every harness that reports a read but not a write. As the review notes, a provider that fabricates cacheRead can already under-report input or output; the token cap never defended against that, and assertValidSpend still refuses any class total that exceeds input. Both runtime producers (routerPromptCacheUsage, driverPromptCacheUsage) emit all-or-none classes today, so the lone-cacheRead shape is not reachable from our own code.

Added the floor test you asked for: the charge never falls below the output tokens, including a zero prompt total with a positive reported read.

pnpm run typecheck, pnpm run lint, pnpm run docs:freshness pass. pnpm test: 211 files passed, 2 skipped; 2634 passed, 6 skipped, 0 failed.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 7ec02735

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.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-14T00:59:15Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

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
Coverage 2 of 2 lenses (value, usefulness)
Concerns 1 (1 low)
Heuristic 0.0s
Duplication 0.0s
Interrogation 94.9s (2 bridge agents)
Total 94.9s

💰 Value — sound

Rewrites the supervised-budget token charge to an additive form (input − cacheRead + output) so a rolled-up aggregate charges exactly the sum of its records, and relaxes assertValidSpend to accept the incomplete-split shape that fold legitimately produces.

  • What it does: Two coordinated changes to the token-accounting core. (1) chargedTokens (src/runtime/util.ts:199) switches from freshInput + cacheWrite + output under a complete split (falling back to non-additive input + output otherwise) to input − creditedCacheRead(usage) + output, where creditedCacheRead returns cacheRead only when the reported classes fit inside input. Because input and cacheRead both
  • Goals it achieves: Make the conserved token unit additive across aggregation, so (a) a resumed pool built from a real rolled-up committed spend no longer dies at construction, (b) a child's settlement charge equals the sum of its per-turn charges, and (c) the within-run pool and the cross-run equalKOnCost check (src/runtime/personify/trajectory.ts:179) cannot disagree about what an arm cost — the contract documented
  • Assessment: Sound and squarely in the grain. The cross-run check explicitly requires the same unit as the pool; the non-additive #835 form broke that (one unclassified turn in a fold dropped the whole aggregate to input+output, charging 1010 against a pool that charged 110 — reproduced in tests/kernel/supervise.test.ts:865 and tests/kernel/rsi-wave.test.ts:712). The subtraction form is the minimal additive re
  • Better / existing approach: none — this is the right approach. Searched src/**.ts for chargedTokens/cacheRead/freshInput/cacheWrite consumers: chargedTokens is the one charge function (budget.ts reconcile/observe/restore, trajectory.ts equalKOnCost, coordination-driver.ts logging) and addTokenUsage is the one accumulator (spendFromUsageEvents, foldUsage, trajectory rollup). No existing equivalent to reuse. The only alternati
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

Fixes two real defects in the conserved-budget token accounting unit: a non-additive charge that over-charged aggregates mixing classified and unclassified turns, and a resume-crash where assertValidSpend rejected the aggregate shape that addTokenUsage legitimately produces — both squarely on the li

  • Integration: chargedTokens and addTokenUsage are the core accounting primitives consumed by the budget pool (budget.ts:402,493,540), the cross-run equal-K cost comparator (trajectory.ts:179), and the supervisor resume path (supervisor.ts:483 via addSpend→addTokenUsage). The fix lands directly on the path that was broken: supervisor.ts:483 passes an addTokenUsage-folded aggregate as restore.committed, which hit
  • Fit with existing patterns: The subtraction form input − cacheRead + output is exactly equivalent to the old freshInput + cacheWrite + output under a complete split (since input = freshInput + cacheRead + cacheWrite), so it preserves the established unit and the cacheBreakdownKnown incompleteness-flag convention. It does not compete with any existing pattern — it fixes the arithmetic of the one the codebase already built.
  • Real-world viability: Arithmetic verified for the key scenarios: aggregate additivity with mixed classified/unclassified turns (1010−900=110 = 100+10), overflow guard preventing under-charge (a turn whose classes exceed its own input credits nothing at record and fold), zero-input edge ({input:0,output:1,freshInput:5} charges 1 not 6). Both chargedTokens and addTokenUsage are pure synchronous functions with no shared m
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: magic number added tests/kernel/supervise.test.ts

  • // One classified turn (100 new of 1000 prompt) and one turn the provider never classified.

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 · 20260814T010329Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 7ec02735

Review health 100/100 · Reviewer score 77/100 · Confidence 85/100 · 11 findings (11 low)

opencode GLM 5.2 opencode DeepSeek v4 Pro opencode DeepSeek v4 Flash aggregate
Readiness 86 89 77 77
Confidence 85 85 85 85
Correctness 86 89 77 77
Security 86 89 77 77
Testing 86 89 77 77
Architecture 86 89 77 77

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

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

🟡 LOW Two sentences share one physical line, against repo markdown convention — docs/architecture.md

Line 384 carries two full sentences on one physical line ('...credits nothing. Prompt tokens...'). AGENTS.md and the file's own style put each full sentence on its own physical line for long markdown. Not a correctness issue; sentence-per-line would match convention and ease later diffs.

🟡 LOW Overflow handling is inconsistent by class-field shape — src/runtime/supervise/budget.ts

A spend with all three classes and classified > input throws cache classes must not exceed input (fail-loud, and at reconcile pre-settlement). The identical overflow expressed with only cacheRead present (no freshInput/cacheWrite) passes assertValidSpend silently; creditedCacheRead (util.ts:207) then refuses the credit and the charge is the same. Both end at charge = input + output, so no billing difference — but the validation contract depends on whether the offending class is one of a complete-looking triple or a partial set. A single guard in chargedTokens/addTokenUsage already covers both; the assertValidSpend throw is redundant for the partial shape only.

🟡 LOW Partial-class spends make cacheBreakdownKnown:false mean trusted-exact, not strictly upper bound — src/runtime/supervise/budget.ts

New chargedTokens credits cacheRead on ANY fitting class set, including {input:100, cacheRead:90} with freshInput/cacheWrite absent — base charged 100, new charges 10. The readout docstring still promises the debited amount is 'an upper bound on newly-presented work'. That holds for the fully-unclassified case and for the unclassified remainder, but for a partial split it is exact only if the provider's cacheRead is accurate; an under-reported cacheRead turns the flagged balance into a potential under-count (lower bound), not an upper bound. The trust-boundary caveat is documented in util.ts:192-194 and CHANGELOG, so this is a doc-precision nit on budget.ts:94-101, not a code defect.

🟡 LOW Pre-settlement assertValidSpend can still strand a reservation — src/runtime/supervise/budget.ts

reconcile calls assertValidSpend before open.delete; the module doc at lines 410-415 promises no throw can skip settlement, but that promise covers only the violation throws below. The new cache classes must not exceed input arm (budget.ts:184) is another pre-settlement throw. It is not NEW exposure — base threw on any all-three-class non-partitioning spend at the same spot — and folded spends can never trigger it (addTokenUsage's fits gate prevents classes > input), so it is reachable only by hand-built or legacy-persisted spends. Worth noting as a latent fragility while this function is open.

🟡 LOW cacheBreakdownKnown=false can now describe an exact measurement, not only an upper bound — src/runtime/supervise/budget.ts

When a spend reports cacheRead but omits freshInput/cacheWrite, hasCompleteCacheBreakdown returns false (tainting the flag), yet chargedTokens still credits the reported cacheRead, so the debited amount is input - cacheRead + output — an exact measurement if the provider's cacheRead is complete and accurate, not an upper bound. The flag therefore reads as more uncertain than the charge actually is. This is the conservative/safe direction (it never under-states uncertainty), and the doc was reworded to 'whose prompt-cache split it could not read', so this is a wording nuance, not a data-integrity issue. No change required; noted for awareness.

🟡 LOW Incomplete split now credits reported cacheRead, reducing the charged total vs. the old full-input fallback — src/runtime/util.ts

Before this PR, chargedTokens returned input+output for any non-complete split (hasCompleteCacheBreakdown false). Now it returns input - cacheRead + output whenever the reported classes 'fit' inside input, even when freshInput/cacheWrite are absent (partial split). A provider (or a malformed aggregated spend) reporting a large cacheRead with no other classes reduces its charge from input+output down toward output. The 'fits' guard in creditedCacheRead (util.ts:204-208) caps the credit so it can never exceed input, so the charge never drops below output and no tokens are minted, and cacheBreakdownKnown is set false so the readout reports a bound. The module header (budget.ts:25-27) and chargedTokens JSDoc (util.ts:190-194) explicitly accept this: 'the token channel is an accounting unit, no

🟡 LOW Redundant fits && in the classified computation — src/runtime/util.ts

In addTokenUsage, classified requires sum === input, which already implies classifiedTotal(delta, delta.cacheRead ?? 0) === input <= input, so fits is necessarily true whenever classified is true. The fits && guard is dead weight, not a bug. Cosmetic only; suggest dropping it to avoid a reader wondering whether there is a case where fits is false yet classified would otherwise be true.

🟡 LOW classifiedTotal parameter typed Partial but always receives a full usage object from chargedTokens path — src/runtime/util.ts

classifiedTotal(usage: Partial, cacheRead: number) is shared between creditedCacheRead (which passes a full LoopTokenUsage) and addTokenUsage (which passes a Partial delta). The Partial typing is correct for the delta call site but slightly imprecise for the usage call site; functionally harmless since both use only ?? coalescing on freshInput/cacheWrite. Non-blocking style nit.

🟡 LOW Non-discriminating test: 'credits nothing when a reported cache class exceeds' passes on base and head — tests/kernel/supervise.test.ts

Evidence: with the base commit's util.ts/budget.ts checked out, this test still passes (only 5 of 6 new supervise tests failed; this one did not). For the shape used ({input:10, output:0, cacheRead:4000}, no freshInput/cacheWrite), base's hasCompleteCacheBreakdown path returns input+output=10 and head's creditedCacheRead returns 0 credit (4000>10), so input+output=10 — identical charge and identical cacheBreakdownKnown:false. The new creditedCacheRead guard only changes behavior for shapes where all three classes are present but the classes overflow input while cacheRead alone is ≤ input (e.g. input 10, fresh 5, cacheRead 10, cacheWrite 5) — that discriminating shape is not exercised here. Impact: the test is a valid behavior pin but never executes the code path this PR added; the fold-ove

🟡 LOW Zero-prompt test leaves cacheBreakdownKnown unpinned for a stray class field — tests/kernel/supervise.test.ts

Evidence: for {input:0, output:1, freshInput:5}, hasCompleteCacheBreakdown (util.ts:156-159) short-circuits to true when input===0 (util.ts:162), so cacheBreakdownTainted stays false and the readout reports cacheBreakdownKnown:true even though the stray freshInput=5 contradicts a zero prompt total. The test asserts only tokensLeft (99), so this coexistence of a nonzero class field with cacheBreakdownKnown:true is left unpinned. Impact: if a future change makes the zero-input case taint the breakdown flag, this test will not notice; the charge itself (output-only) is correct under both old and new chargedTokens, so the test does not mis-guard the fix. Fix: add expect(pool.readout().cacheBreakdownKnown).toBe(true) (or deliberately pin the intended value) so the flag contract for this shape i

🟡 LOW coverage overlap: 'fold rescue' and 'never charges less' exercise the same overflow-drop path — tests/kernel/supervise.test.ts

Both 'does not let a fold rescue a cache read that overflowed its own turn' (L891) and 'never charges less than the output tokens' (L912) depend on addTokenUsage dropping classes when classifiedTotal>input. In L912's case event1 {input:0,cacheRead:1} has classifiedTotal 1>0 so its class is dropped — the exact behavior L891 already locks. Not a defect (both pass and assert distinct surface outcomes: L891 checks additivity equality, L912 checks the output floor), but the two share their load-bearing mechanism, so a regression in addTokenUsage's fits-guard would fail both identically. No action required; noted for future test maintenance.


tangletools · 2026-08-14T01:16:46Z · trace

tangletools
tangletools previously approved these changes Aug 14, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 11 non-blocking findings — 7ec02735

Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 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-14T01:16:46Z · immutable trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 451ecff0

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.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-14T01:28:49Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

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
Coverage 2 of 2 lenses (value, usefulness)
Concerns 1 (1 low)
Heuristic 0.0s
Duplication 0.0s
Interrogation 188.7s (2 bridge agents)
Total 188.7s

💰 Value — sound

Rewrites the supervised token charge into an additive input − cacheRead + output form with a per-record fit guard so an aggregate charges exactly what its records charged, and relaxes spend validation so a resumed pool survives real incomplete-split telemetry.

  • What it does: Changes chargedTokens() (src/runtime/util.ts:199) from a conditional form (freshInput + cacheWrite + output only under a complete split, else input + output) to an unconditional input − creditedCacheRead + output, where the cache-read credit is denied when the reported classes overflow the prompt total (util.ts:204-208). Adds a 'fits' guard to addTokenUsage (util.ts:229-234) so a delta w
  • Goals it achieves: (1) Additivity — the charge on an aggregate built by addTokenUsage equals the sum of charges on its records, so a child's settlement agrees with the pool roll-up and the equal-k cost comparison (trajectory.ts:179) agrees with the within-run pool. (2) Let a resumed supervisor reconstruct its pool from real telemetry that carries a partial cache split instead of throwing at construction. (3) Guara
  • Assessment: A clean, correct fix in the grain of the codebase. chargedTokens is the single-sourced unit — grep confirms it is the ONLY charge formula, consumed identically by budget.ts (reconcile/observe/restore:402,493,540), coordination-driver.ts:794-795 (logging), and trajectory.ts:179 (equal-k cost). Rewriting it in subtraction form is minimal and the right shape: input and cacheRead both already ac
  • Better / existing approach: none — this is the right approach. Searched for a parallel token-charge computation (grep'd chargedTokens, freshInput.*cacheWrite, input.*-.*cacheRead across src): there is none; chargedTokens is the single source. Considered the alternative of having addTokenUsage accumulate a precomputed charge field on LoopTokenUsage: that would be more invasive (new type field, every call site must
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error

🎯 Usefulness — sound

Makes the budget token charge arithmetically additive over the codebase's single-sourced spend fold and un-breaks resumed pools, by modifying two heavily-consumed existing functions in place — verified by tests across every consumer.

  • Integration: Fully reachable, no new surface. chargedTokens (src/runtime/util.ts:199) is consumed by the conserved pool at budget.ts:402/493/540, driver summaries at coordination-driver.ts:794-795, and cross-run arm rating at trajectory.ts:179; addTokenUsage (util.ts:223) is the single spend fold called from 8 runtime files. The assertValidSpend relaxation (budget.ts:170-187) sits on every reconcile/observe/re
  • Fit with existing patterns: In the grain. The codebase single-sources spend arithmetic on addTokenUsage (comment at trajectory.ts:291) and documents that the cross-run equal-k check must use 'the same unit the conserved pool spends' (trajectory.ts:163-166). The prior freshInput+cacheWrite form with whole-record fallback broke that agreement; input − cacheRead + output restores charge(aggregate) === Σ charge(records) by const
  • Real-world viability: Holds beyond the happy path. Overflow guard (util.ts:204-208) yields credit 0 when classes exceed input, so charge never drops below output; addTokenUsage drops non-fitting class sets (util.ts:228-234) so an overflowing record cannot be rescued by a larger accumulator. Both live provider paths keep input as the rolled-up total including cache (runtime.ts:848-855 router, runtime.ts:3843-3849 bridge
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: magic number added tests/kernel/supervise.test.ts

  • // One classified turn (100 new of 1000 prompt) and one turn the provider never classified.

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 · 20260814T014703Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 451ecff0

Review health 100/100 · Reviewer score 67/100 · Confidence 85/100 · 8 findings (1 medium, 7 low)

opencode GLM 5.2 opencode DeepSeek v4 Pro opencode DeepSeek v4 Flash aggregate
Readiness 92 95 67 67
Confidence 85 85 85 85
Correctness 92 95 67 67
Security 92 95 67 67
Testing 92 95 67 67
Architecture 92 95 67 67

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

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

🟠 MEDIUM Restore re-charges old-version journals with the new smaller formula, minting capacity across a version boundary — src/runtime/supervise/budget.ts

restore debits chargedTokens(committed.tokens) (line 540). The formula changed this PR from input + output to input − cacheRead + output for incomplete splits. A journal written by a prior-version process that ran an incomplete aggregate (e.g. input=1010, freshInput=100/cacheRead=900/cacheWrite=0, cacheBreakdownKnown:false) was charged 1010 live by that process; restoring that same committed Spend under this version debits 110, silently handing ~900 tokens back to freeTokens. The module doc asserts 'restart never mints capacity' ([line 46](https://github.com/tangle-network/agent-runtime/blob/451ecff02c4ac0b77f57d2fc60188de83a1cc70b/src/runtime/supervis

🟡 LOW 'cache class that does not fit' phrasing is mildly imprecise — docs/architecture.md

The sentence 'A cache class that does not fit inside the prompt total it partitions credits nothing' attributes the fit check to a single class. The code (creditedCacheRead in src/runtime/util.ts) zeroes the cacheRead credit when the classified TOTAL (freshInput + cacheRead + cacheWrite) exceeds input, not when any one class fails to fit. Only cacheRead is ever credited; freshInput and cacheWrite are never subtracted. The intent is conveyed by context but the wording could read as 'each class is individually checked against input'. Consider 'A cache split whose classes do not fit inside the prompt total they partition credits nothing.' No functional impact — documentation only.

🟡 LOW Upper-bound claim omits the provider-trust qualifier — docs/architecture.md

The sentence 'cacheBreakdownKnown: false then marks tokensLeft an upper bound on newly-presented work' is exact only when the provider does not over-report cacheRead: charge - trueNew = trueCacheRead - reportedCacheRead, so an over-report turns the bound into a lower bound. This matches the code's own docs (budget.ts:25-27, util.ts:192-194 'the pool trusts a reported cache read the same way it trusts a reported input'), so doc and code agree; the architecture summary simply omits that qualifier. Optional one-clause addition; not blocking. Evidence: chargedTokens() util.ts:199-201 only credits the REPORTED cacheRead that fits.

🟡 LOW Overflow guard in assertValidSpend only fires when all three cache classes are present — src/runtime/supervise/budget.ts

The new partition check (lines 170-187) is gated on freshInput !== undefined && cacheRead !== undefined && cacheWrite !== undefined. A Spend with only a cacheRead that dwarfs input — e.g. { input: 10, cacheRead: 4000, cacheBreakdownKnown: false } — skips the guard entirely and is accepted, while the same overflow with all three classes present throws. It is not exploitable: chargedTokens/creditedCacheRead (util.ts:207) refuses the credit whenever classifiedTotal > input, so no free tokens are granted — the gap is purely in validation strictness and shape-dependent error behavior. Confirm the intent is to validate only the fully-declared sh

🟡 LOW Fold and validate diverge on the same overflowing telemetry: silent drop vs hard throw — src/runtime/util.ts

addTokenUsage computes fits = classifiedTotal(delta, delta.cacheRead ?? 0) <= input and silently discards the classes of a turn whose classes exceed its own input (line 229-234). The same shape arriving as a direct Spend (all three classes present, cacheBreakdownKnown: false, classified > input) is refused by assertValidSpend in budget.ts:184 ('must not exceed input') in observe/reconcile/restore. Same underlying telemetry yields a silent fold and a fail-loud ValidationError depending on which path built the Spend. Both outcomes are safe — creditedCacheRead (util.ts:207) independently refuses the credit, so no under-charge is possible either way —

🟡 LOW rsi-wave test does not independently assert the within-run pool agreement it cites — tests/kernel/rsi-wave.test.ts

The comment says the arm reading 110 vs 1010 matters because 'the cross-run check would contradict the within-run pool' — but the test asserts verdict.arms[0].tokens === 110 without asserting that a budget pool fed the same two spends charges 110. That fact is pinned in supervise.test.ts (lines 865-892), so cross-file coverage exists; the nit is that this test's stated justification is not locally verified. Optional: fold a createBudgetPool() check into the same test so the pool↔equalKOnCost agreement is asserted where it is claimed.

🟡 LOW 'credits nothing when a reported cache class exceeds the prompt total' is vacuous against base — tests/kernel/supervise.test.ts

Verified by running against base: with {input:10, cacheRead:4000} (no freshInput/cacheWrite), base's assertValidSpend skips the class check and base's chargedTokens charges input+output = 10, so base also reads tokensLeft 990. The test only guards the NEW credit-refusal invariant (it fails if creditedCacheRead were regressed to credit unconditionally). That is fine, but it does not, on its own, distinguish fixed from unfixed behavior — a reviewer reading the comment 'Bad telemetry may over-charge; it may never buy free tokens' should not count it as a regression test. Consider extending it to assert the overflow-credit case that only new code could produce (e.g., a fitting cacheRead-only credit followed by an overflow that must not be absorbed by a later larger input).

🟡 LOW reconcile() path untested for the incomplete partial-class split — tests/kernel/supervise.test.ts

The bug that motivated this PR — a resumed pool dying at construction on a partial-class split with cacheBreakdownKnown:false — is covered via restore (line 825) and observe (lines 870-886, 908-912), but never via reconcile(ticket, spent). reconcile shares assertValidSpend + chargedTokens, so behavior should be identical, but the settlement+refund arithmetic (freeTokens += rTokens - spentTokens with a partial-class spent) is untested. Fix: add a reconcile case using the {input:1010, freshInput:100, cacheRead:900, cacheWrite


tangletools · 2026-08-14T01:59:20Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 8 non-blocking findings — 451ecff0

Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 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-14T01:59:20Z · immutable trace

@drewstone

Copy link
Copy Markdown
Contributor Author

On the MEDIUM for 451ecff0 — restore re-charging an old-version journal with the new formula: correct observation, intended behavior, not fixing it.

The reading of "restart never mints capacity" is that a restart must not let a run re-spend budget it already consumed. It is not a promise that the ledger is byte-identical across a change of unit. Charging pre-restart work at input + output while charging post-restart work at input - cacheRead + output would put two different units in one pool, and the pre-restart half would carry exactly the defect #831 reports — a resumed cache-heavy run would still die at ~1.8% of its declared budget, and only the resumed portion would be measured honestly.

So the restored balance is the corrected one, by the same one-time step every running config takes at 0.134.0. That step is already stated at the top of the changelog: existing numbers are unchanged, the unit they measure is not, and a caller who tuned maxTokens down to compensate must re-derive it. A journal crossing the version boundary is that same migration, applied to work already recorded.

The remaining findings are doc-precision notes on wording I reworked in 7ec02735, and the pre-settlement assertValidSpend throw — a pre-existing lifecycle wart I have filed on #831 rather than smuggle a fail-loud redesign into a token-unit change.

Merging at 451ecff0: 5/5 audit shots completed, no blockers, all four CI checks green.

@drewstone
drewstone merged commit 162769b into main Aug 14, 2026
4 checks 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