fix(tokens): dedup defaultCountTokens, correct stale comments - #113
Open
ranxianglei wants to merge 1 commit into
Open
fix(tokens): dedup defaultCountTokens, correct stale comments#113ranxianglei wants to merge 1 commit into
ranxianglei wants to merge 1 commit into
Conversation
search-index.ts duplicated the kernel's defaultCountTokens regex locally - identical today, but a drift bomb if the kernel heuristic ever changes. Delegate to the imported defaultCountTokens instead. commands.ts comments called the counter "chars/4" but it is CJK-aware (1:1 for CJK, chars/4 for non-CJK). Correct the wording so readers don't assume Chinese text is underestimated 4:1.
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
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.
What
src/search-index.ts: replace the locally-duplicateddefaultCountTokensregex with a delegation to the kernel's importeddefaultCountTokens.src/commands.ts: correct two stale comments that called the counter "chars/4" — it is actually CJK-aware (1:1 for CJK chars, chars/4 for non-CJK).Why
search-index.ts had a verbatim copy of the kernel's
defaultCountTokensregex ([\u4e00-\u9fff\u3040-\u30ff\uac00-\ud7af]counted 1:1, remainder chars/4). Identical to the kernel today, but a drift bomb: if the kernel heuristic ever changes, this copy diverges silently and the search index's token estimates fall out of sync with the rest of the pipeline. Delegating to the imported function removes the sync burden.commands.ts comments described the counter as "chars/4" and "chars/4-vs-real-tokenizer drift". Misleading —
defaultCountTokenscounts CJK characters 1:1 (not 4:1), so Chinese/Japanese/Korean text is NOT underestimated 4×. Corrected to namedefaultCountTokensand its real behavior, so readers don't assume CJK content is severely undercounted.Context
Found during the acp-kernel tokenizer-consistency review (kernel PR #56
fix(recommend): use ctx.countTokens). billion-context-pi's per-message counting is already consistent with the kernel (all paths usedefaultCountTokens); this PR closes a drift risk and fixes misleading docs.Out of scope
acp-kernel@0.0.17): deferred until kernel PR fix(delegate): replace status tool with blocking wait #56 merges and publishes, then a separate release branch bumps it (pi CInpm cirequires the published version).tokenCountfrom real provider usage vs per-message heuristic): intentional — the footer/nudge anchor on real usage. Not touched.Verification
tsc --noEmit: cleannode --import tsx --test tests/*.test.ts: 153/153 passscripts/ci/check-pr.sh: passed (branch name + version)