fix(supervise): make the token charge additive over aggregated spend - #837
Conversation
`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
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
🟢 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 toinput + outputwhen the cache split is incomplete) toinput − 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
chargedfield 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.
✅ No Blockers —
|
| 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
cacheReadwhenever 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 + outputand '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
left a comment
There was a problem hiding this comment.
✅ 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
|
Addressed the audit on Fixed — the fold rescued a credit the record refuses. A delta whose classes do not fit inside its own 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 Not changing — crediting a partial 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.
|
tangletools
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
🟢 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 + outputunder a complete split (falling back to non-additiveinput + outputotherwise) toinput − 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.
✅ No Blockers —
|
| 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 onlycacheReadpresent (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
violationthrows below. The newcache classes must not exceed inputarm (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
cacheReadbut omitsfreshInput/cacheWrite,hasCompleteCacheBreakdownreturns false (tainting the flag), yetchargedTokensstill credits the reportedcacheRead, so the debited amount isinput - 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,classifiedrequiressum === input, which already impliesclassifiedTotal(delta, delta.cacheRead ?? 0) === input <= input, sofitsis necessarily true wheneverclassifiedis true. Thefits &&guard is dead weight, not a bug. Cosmetic only; suggest dropping it to avoid a reader wondering whether there is a case wherefitsis false yetclassifiedwould 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
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
🟢 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 + outputonly under a complete split, elseinput + output) to an unconditionalinput − 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 toaddTokenUsage(util.ts:229-234) so a delta w - Goals it achieves: (1) Additivity — the charge on an aggregate built by
addTokenUsageequals 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.
chargedTokensis 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:inputandcacheReadboth already ac - Better / existing approach: none — this is the right approach. Searched for a parallel token-charge computation (grep'd
chargedTokens,freshInput.*cacheWrite,input.*-.*cacheReadacross src): there is none;chargedTokensis the single source. Considered the alternative of havingaddTokenUsageaccumulate a precomputed charge field onLoopTokenUsage: 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.
✅ No Blockers —
|
| 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 frominput + outputtoinput − cacheRead + outputfor 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 acacheReadthat dwarfsinput— 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 wheneverclassifiedTotal > 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
addTokenUsagecomputesfits = classifiedTotal(delta, delta.cacheRead ?? 0) <= inputand silently discards the classes of a turn whose classes exceed its own input (line 229-234). The same shape arriving as a directSpend(all three classes present,cacheBreakdownKnown: false, classified > input) is refused byassertValidSpendin budget.ts:184 ('must not exceed input') inobserve/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
left a comment
There was a problem hiding this comment.
✅ 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
|
On the MEDIUM for 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 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 The remaining findings are doc-precision notes on wording I reworked in Merging at |
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
chargedTokensreadfreshInput + cacheWriteunder a complete split and fell back toinput + outputotherwise.addTokenUsagelegitimately folds a classified turn together with an unclassified one, producing partial class totals pluscacheBreakdownKnown: false— and the fallback then applied to the WHOLE aggregate:The charge is now
input - cacheRead + output. Identical under a complete split (input - cacheRead === freshInput + cacheWrite), and both terms accumulate throughaddTokenUsage, 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:
{input: 0, output: 1, freshInput: 5}charged 6 before, charges 1 now).2. A resumed pool died at construction on real telemetry
assertValidSpenddemanded an exact partition from every spend carrying all three classes. The aggregate above carries all three and sums to 1000 againstinput: 1010, so:Every resumed supervisor whose journal mixes a classified and an unclassified turn aborted before admitting any work. Same throw on
reconcile, fromassertValidSpendbefore the ticket closes, which additionally leaks the reservation into the join barrier.An incomplete split — the shape
cacheBreakdownKnown: falseexists to describe — now only has to FIT insideinput. A spend that CLAIMS a complete split must still partitioninputexactly, and classes that exceedinputare 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.
equalKOnCostfollowsA 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
src/runtime/sandbox-events.ts:106,src/runtime/supervise/sandbox-session.ts:217,src/runtime/run-loop.ts:750): anllm_callcarryingpromptCache.readTokensstill 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.assertValidSpendthrows before the ticket closes (src/runtime/supervise/budget.tsreconcile, 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.cacheBreakdownKnown: false. Deliberate, and the field doc now states it: a declared ceiling has no cache composition to misread, andtokensKnown: falsealready reports that the balance is not a measurement.Proof
New tests, each red on #835's code:
inputrestores; one exceedinginputstill throwsequalKOnCostrates a mixed rolled-up arm at 110, not 1010pnpm run typecheck,pnpm run lint,pnpm run docs:freshnesspass.pnpm test: 211 files passed, 2 skipped; 2632 tests passed, 6 skipped, 0 failed.