Repository navigation
fix(memory): count tokens at decision boundaries through one shared helper (#13694) - #13797
Conversation
…elper (#13694) Context-fitting decisions ran on len(text.split()) * 1.3, whose own docstring conceded it held 'for English prose' — which is exactly where large prompts do not come from. Measured divergence against the character estimate: case words*1.3 chars/4 ratio English prose 19 20 1.1x Code 11 35 3.2x JSON 1 19 19.0x CJK 1 26 26.0x text.split() returns ONE element for a non-space-delimited script, so CJK under-counted ~26x. That number gates whether compression runs at all, so the threshold was reached only after the real window was blown. Investigation first, and it changed the fix. The issue proposed a local exact counter; tiktoken is importable in some dev environments but is in no requirements file, so building on it would be green locally and broken in CI. Meanwhile the 80/90% overflow trigger ALREADY consumes authoritative provider usage — api/chat.py:797 -> handle_message_completion -> check_and_protect (usage=llm_response.usage) — so that AC was already satisfied and did not need a tokenizer at all. What was actually wrong: - services/memory/compression.py used words*1.3 for the compression gate. - context_overflow.py re-added post-summary tokens with an inline len//4 — a fourth estimator, and the numbers the trigger runs on until the next provider response supplies a real count. autobot_shared/token_count.py now holds the fast/exact distinction, delegating the fast path to the canonical doc_chunking estimator rather than deriving a new ratio. Both call sites use it. Why no local exact tokenizer exists is documented there so the next reader does not re-litigate it. Also restores _CONFIG_PATH, which my first edit to compression.py deleted — caught by flake8 F821 before it left the branch.
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
CI diagnosis — one stale test, and it asserts the formula this PR is replacing
The test is not testing a behaviour, it is restating the old arithmetic — its own comment says so: def test_multiple_words(self) -> None:
text = " ".join(["word"] * 10)
# 10 * 1.3 = 13
assert _estimate_tokens(text) == 13This PR routes
Worth pointing at your own table while updating it, since it is the argument for the change: A test that hardcodes Also BEHIND — needs a branch update before it can merge. Not touching the branch: it is another session's. Flagging so whoever owns it does not have to |
|
Blocked — do not merge. Review found the core remedy does not survive measurement.
It additionally:
Full analysis and the re-scoping recommendation are on #13694. The real blocker is a dependency decision about a tokenizer, which belongs on the issue rather than being smuggled in behind a char heuristic. |
…13950) (#13962) * chore: claim worktree for #13950 * ci(code-quality): scope the hardcoded-value guard to changed lines (#13950) The guard reads each changed FILE whole, so editing one line of a legacy module makes a PR answerable for that file's entire backlog. Four PRs in one session failed on violations git blame attributes to other commits: #13924 (7), #13797 (3), #13934 (3), #13945 (1). None touched an offending line. The rule is good and all 14 were worth fixing — they are fixed. The cost is that each occurrence looks like a genuine regression until a per-line blame plus a check of the branch's own commit list proves otherwise, and it recurs on every touch of a large legacy module. It also makes avoiding old files the cheapest way to stay green. Worse, in two of the four the constant the message SUGGESTED was the wrong concept: #13934's 'user_role' is an RBAC input, not the chat-vocabulary CategoryDefaults.ROLE_USER, and #13945's le=100 is a ceiling, not KNOWLEDGE_DEFAULT_LIMIT. Both hold the same value today, so the mechanical fix the message invites goes green while encoding a different meaning. Advice that is unsafe to follow is a reason to only give it where the author is in the code. Opt-in via --changed-lines-only rather than a default, so every other hook that runs through this wrapper keeps its behaviour byte for byte. Only code-quality's hardcoded-values step passes it; pre-commit and a direct run still scan whole files, so the backlog stays measurable rather than invisible. Fails closed. If the hook exits non-zero and the filter parses no 'file:line' violations, the output is not one it understands, so it prints everything and exits with the hook's status — suppressing an unparsed failure would turn a real regression green, which is #13880's defect reached from the other direction. Suppressed violations are still printed under their own heading; the hook's own 'COMMIT BLOCKED' trailer is dropped in scoped mode because it counts what the hook found across whole files, not what this wrapper is about to decide. 3 mutations killed: filter suppressing everything (guard becomes an off switch), filter keeping everything (no scoping), and the default silently gaining scoping. A first version also counted the hook's banner and trailer as kept and reported '6 violations on lines this PR added' for a PR that added one clean line — the summary contradicting the correct suppression directly above it.
…sweep touching operator-chosen roots (#13933) (#13934) * chore: claim worktree for #13933 * fix(agent): bound classification by serialised size, and stop the sweep touching operator-chosen roots (#13933) * fix(agent): measure the replacement instead of estimating it, and release the resume binding (#13933) * fix(agent): replace hardcoded role literals with the right SSOT constant per concept (#13933) Same inheritance as #13924/#13797: the hardcoded-value guard scans changed files rather than changed lines, so touching loop.py and tool_registry.py made this PR responsible for three pre-existing violations. All three blame to adbf666, which is none of this branch's three commits. The guard's suggested fix is right for two of the three and wrong for the third. tool_registry.py's 'user_role' is an RBAC role — worker_node reads it in _validate_user_role and _check_task_permission — not a chat message role. It uses Role.USER from autobot_shared.auth.permissions rather than CategoryDefaults.ROLE_USER, which is the chat vocabulary and merely happens to hold the same string. Taking the suggestion literally would tie an authorization decision to a presentation constant, and the two are free to diverge. Behaviour-preserving: ROLE_ASSISTANT == 'assistant' and Role.USER.value == 'user' were checked against the values they replace. tool_registry.py now imports autobot_shared.auth.permissions; that module's own docstring warns about an import cycle through auth_middleware, so both modules were import-smoked to confirm none is introduced. tests/agent_loop: 230 passed.
…13694) flake8 F821: essential_story.py:208 calls estimate_fast with no import, so the module would raise NameError the first time _estimate_tokens ran. Self-inflicted. Earlier in this branch's history I started a rebase, it stopped part-way, and 'git rebase --abort' discarded a working-tree change that was adding exactly this import. The call site survived in an earlier commit; the import did not. Caught by flake8 rather than by any test, which is the point worth noting: the branch's own suite never exercises _estimate_tokens, so nothing here would have failed until the code ran in production. Verified by mutation, not by re-reading: removing the import reproduces 'F821 undefined name estimate_fast' at 208:16, restoring it returns exit 0, and the module imports with estimate_fast resolving to autobot_shared.token_count.
|
All four failures were one cause: Self-inflicted, and worth recording how. Earlier I started a rebase on this branch, it stopped part-way, and Fixed and verified by mutation rather than re-reading: removing the import reproduces The earlier |
|
Status, and a correction to my own method. Nothing had run on this PR's head SHA My watcher had reported this PR as "settled" because it polled for zero pending checks — which is equally true before anything is scheduled. Same shape as the defects I filed today (#14004/#14005/#14006) and as 23 runs approved (scoped to this SHA only, not a bulk approval — the parked backlog is months deep). The four earlier failures all had one cause, now fixed on the branch: Waiting on the real run before merging. |
… ratio (#13694) python-suite shard 9/12: test_multiple_words asserted 13, the shared helper returns 12. The test pinned the behaviour this PR deliberately removes — '10 * 1.3 = 13', the word-based formula whose own docstring conceded it held 'for English prose'. Checked before changing the number, because updating an expected value is exactly how a real regression gets waved through: old: len(text.split()) * 1.3 -> 10 words = 13 new: character-based canonical -> 49 chars = 12 They are different estimators, and 12 is the canonical one. The change is the point of the PR — the word formula under-counted JSON by ~19x and CJK by ~26x, and that number gates whether compression runs at all, so under-counting disabled compression on exactly the payloads that needed it. Rather than swapping 13 for 12, the test now asserts _estimate_tokens(text) == estimate_fast(text): agreement with the canonical estimator, not a second copy of its arithmetic that would drift the moment the ratio is tuned. Added test_a_non_space_delimited_script_is_not_under_counted, which pins the DEFECT rather than the arithmetic: text.split() returns one element for CJK, so the old estimator scored a 200-character Chinese string as 1 token. A value-only test goes green again on any ratio tweak; this one fails for the whole class of input the previous formula could not see. Verified by mutation: reverting to int(len(text.split()) * 1.3) fails both.
|
This PR cannot converge while the queue keeps merging — worth recording, because it is #13801 rather than anything wrong with the change. A Every merge is a new head SHA, which resets a ~60-check run that takes ~35 minutes on a single self-hosted runner. Merges are arriving roughly every 3–20 minutes. The run can therefore never finish, and the rollup collapses to 1 check each time — which is also indistinguishable from "nothing scheduled", the trap my earlier watcher fell into. Now watching the 10 required contexts instead of all ~60. Those are what branch protection actually gates on, they complete well before the full set, and they are the honest bar — waiting for checks that are not required to be green is waiting for something the repo does not require. The substantive work here is done and verified:
|
…tio (#13694) python-suite shard 8/12: test_essential_story.py::test_ten_words asserted int(10 * 1.3), the same defect as the shard 9 failure I fixed one commit ago in test_context_compression.py. My mistake was fixing the instance CI reported instead of the class. The estimator change touches three test files; I found the third only because a second shard failed. Swept properly this time: tests/memory/test_essential_story.py test_ten_words -> agreement assert + CJK under-count test tests/memory/test_essential_story.py:221 stale '10 words * 1.3 = 13' comment tests/services/test_context_compression.py:137 stale '6 words * 1.3' comment tests/test_token_count_boundaries.py keeps len(text.split()) * 1.3 on purpose — it is the baseline the new estimator is compared against there, not a stale assertion. Left alone. Both new assertions pin agreement with the canonical helper rather than a literal, so they cannot drift when the ratio is tuned, and the CJK test fails for the whole class of input the old formula could not see rather than for one value. Verified by mutation: reverting to int(len(text.split()) * 1.3) fails both. 59 passed across the three affected files.
|
Shard 9 now passes; shard 8 exposed the same defect in a third file. My mistake was fixing the instance CI reported rather than the class. The estimator change touches three test files and I only found the third because a second shard happened to fail — a sharding boundary, not a method. Swept properly now:
Verified by mutation: reverting the estimator fails both new assertions. 59 passed across the three affected files. Required contexts were all green on |
1 participant
Thinking Path
The issue proposed adding a local exact token counter. Investigating first changed the fix, in two ways.
tiktokenis not a declared dependency. It imports in this dev venv and appears in norequirements*.txt, so building the exact path on it would be green locally and broken in CI and any deployment installing from requirements — failing precisely in the environment that matters.The expensive decision already uses authoritative counts. The 80%/90% overflow trigger consumes
llm_response.usageend to end:So AC 3 ("prefer the provider's number") was already satisfied on the main path and needed no tokenizer at all. Worth noting I nearly concluded the opposite: grepping for
check_and_handle_overflowreturned nothing, which looked like "the trigger has no callers" — the method is actuallycheck_and_protect. A name I invented, not a finding.What was actually wrong
Two places, both real:
services/memory/compression.py—len(text.split()) * 1.3gating whether compression runs at all.chat_history/context_overflow.py— after summarising, remaining messages were re-added with an inlinelen(text) // 4: a fourth estimator, and the numbers the trigger runs on until the next provider response supplies a real count.Why it matters, measured
text.split()returns one element for a non-space-delimited script. AutoBot is i18n across 11 locales, so a 26× under-count is not hypothetical — and under-counting means the threshold is reached only after the window is already blown.What Changed
autobot_shared/token_count.py(new) holds the distinction the issue asked for:estimate_fast(text)— delegates to the canonicaldoc_chunking.estimate_tokensrather than deriving a ratio, so this is a distinction, not a fourth estimator.exact_from_usage(usage)— the provider's authoritative count, orNone.resolve_tokens(text, usage)— exact when available, else fast, with the precedence explicit at the call site rather than each caller re-deciding it. That re-deciding is how the estimators diverged in the first place.Why no local exact tokenizer exists is documented in the module, so the next reader doesn't re-litigate it.
Verification
Acceptance criteria
test_the_fast_path_delegates_to_the_canonical_estimatorasserts equality withdoc_chunking.estimate_tokensacross all four payload classes.test_resolve_prefers_the_provider_over_the_estimate.TestTheGateActuallyMoves, which shows a payload that stayed under the 8192 threshold on the old estimate now trips it.test_english_prose_is_substantially_unchangedis the guard against overcorrecting: the old heuristic was genuinely fine there, and a fix that broke the case it handled well would be a poor trade.One thing worth flagging
exact_from_usagereturnsNone, not0, for absent usage. Conflating them would silently pin a session at 0% fill and disable the overflow trigger entirely — a test pins that distinction.Caught before it left the branch
My first edit to
compression.pyspliced out_CONFIG_PATHalong with the old estimator.flake8F821 caught it; restored, andservices/memory/passes.Model Used
Claude Opus 5 (1M context)
Closes #13694
Part of #13685