Shrink token accounting: static UTF-8-byte estimate, complete-usage-only bookkeeping, tool exports, stale tests - #12
Merged
Conversation
Four follow-ups from the round-4 review of the merged fixes: - Calibration: measure() is now messageEstimate x ratio + fixedEstimate (and observe derives the ratio from real minus fixed). The blended formula multiplied the English fixed overhead by a CJK conversation's rate, overstating a compressed history by up to 2x the overhead (simulated: 565 vs a true ~232). Adds the mixed-rate regression test the old suite lacked. - Usage honesty: usageSeen only flips on actual numbers (a usage object with null fields is not data), and a new optional usageMissed flags responses that carried none, so /tokens now has three states: unknown / reported-but-incomplete / reported. The label says 'Reported tokens' — no more claiming a complete total. - Tool exports: drop ALL_TOOLS and getTool (zero internal or test usage); the shared SubAgentTool instance they exposed reintroduced the cross-agent parent-stealing bug for library consumers. Only the makeAllTools factory remains. - npm test: clean runs first (portable node -e rm, no more rm -rf), so compiled remnants of deleted tests can no longer execute. Verified by planting a failing ghost test in dist/ — it no longer runs.
Replace the ratio-calibration subsystem with a stateless heuristic and a strict two-lane rule — the estimate drives pre-call decisions and is never calibrated; provider usage is post-hoc accounting and never feeds the estimate. - approxTokens = ceil(utf8Bytes / 3): ASCII lands at ~3 chars/token, CJK at ~1 char/token (3 bytes/char), and byte counting sidesteps UTF-16 surrogate pitfalls — no Unicode tables, no state. - delete ratio and observe() (and the agent call site); measure() is the plain estimate plus the fixed prompt/tool-schema overhead. - usage accumulates only when a chunk carries BOTH prompt and completion numbers; the incomplete mark now lands in a finally, so an aborted or torn-down stream flags the totals too (post-loop code never runs on teardown). - /tokens: 'Reported tokens: P / C (incomplete)' — three states, no total claim. - README: the three calibration passages now describe the byte estimate and the two-lane rule. - tests: calibration cases replaced by exact deterministic byte-math assertions; new partial-usage and aborted-stream-incomplete cases.
nullcache
changed the title
Owner
Author
|
Reworked per the final adjudication — this round deletes the problem source instead of patching it:
54/54 pass. Net vs min: the whole PR now removes more than it adds. |
A stream could deliver the complete usage chunk and then die before [DONE]: the finally saw complete numbers (so no incomplete mark) while the accumulation after the loop was skipped by the teardown — a ledger claiming completeness with the numbers missing. Accumulation now lives in the finally: book the numbers when complete, mark incomplete when not, one lifecycle event either way. Also: agent.ts comment no longer mentions calibration, and the test file is renamed context-estimate.test.ts to match what it tests.
Owner
Author
|
Blocker fixed exactly as specified: accumulation moved into the same |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All four round-4 findings confirmed and fixed in one themed PR (they are all follow-ups to the previously merged review fixes):
measure = messageEstimate × ratio + fixedEstimate, withobservederiving the ratio fromreal − fixed. The old blended form scaled the English fixed overhead by the conversation's CJK rate (~565 vs a true ~232 after compression). The existing four calibration tests pass unchanged — proof they only covered ratio≈1 — and a mixed-rate regression test is added.usageSeenonly flips on actual numbers (null-field usage objects are not data), new optionalusageMissedmarks responses without usage, and/tokensbecomes three-state: unknown / Reported tokens (incomplete…) / Reported tokens. No more "total" claim.ALL_TOOLS/getTooldeleted (zero internal/test usage) — they re-exposed the shared-SubAgentTool parent-stealing bug to library consumers. Factory only.npm testnow cleans first;cleanis a portablenode -erm. Verified with a planted ghost test indist/— it no longer executes.56/56 tests pass.