Skip to content

fix(llm): install the embedded BPE loader from package init - #1228

Merged
lizhengfeng101 merged 1 commit into
alibaba:mainfrom
basil-k-aji-dev:fix/tokenizer-embedded-loader-init
Sep 14, 2026
Merged

lizhengfeng101 merged 1 commit into
alibaba:mainfrom
basil-k-aji-dev:fix/tokenizer-embedded-loader-init

Conversation

@basil-k-aji-dev

Copy link
Copy Markdown
Contributor

Description

TestSelectFilesTooLarge_Boundary (the test #1012 reports under its former name TestFilterLargeDiffs_Boundary) fails on a clean checkout with fixture drift: llm.CountTokens(<80-token string>) = 39, want 80.

The issue attributes this to BPE merging in the exactNTokens fixture, but that is not the cause. The fixture is fine — strings.Repeat("a ", 80) really is 80 cl100k_base tokens. The failure comes from token counting silently degrading:

  • InitEmbeddedLoader() was only called from cmd/opencodereview/main.go.
  • Every other consumer of internal/llm — the whole test suite included — therefore left tiktoken on its default loader, which downloads the encoding file over the network.
  • When that download is unavailable, countTokensWithEncoding falls back to len(text)/4.

For the fixture, len("a a a ... a") / 4 = 159 / 4 = 39, which is exactly the number in the issue report.

So the test does not fail because of the tokenizer; it fails on any machine where the encoding download is blocked or unavailable, and passes everywhere else. That also explains why it is not failing for everyone.

This affects two tests, not just the one in the issue — internal/scan's TestSelectScanItems_LargeBoundary shares the same helper and fails identically.

Fix

Install the embedded BPE loader from the internal/llm package init() rather than relying on an explicit startup call. The embedded data already ships in the package; it was just not wired up unless main() asked for it.

InitEmbeddedLoader() stays exported and is now redundant in main(), so that call is removed.

This is more than a test fix: the silent len(text)/4 fallback affects real token budgeting for any consumer that does not go through the CLI entry point.

Why not adjust the fixture instead

#1012 suggests building the fixture from the tokenizer's own output. That would make the test pass under the degraded byte estimate too, hiding the real defect. Keeping the strict exactNTokens check leaves it as a canary, which is what caught this.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (described below)

Reproduced the failure deterministically by pointing TIKTOKEN_CACHE_DIR at an empty directory and blackholing the proxy env, which makes the encoding download fail:

$ TIKTOKEN_CACHE_DIR=<empty> HTTPS_PROXY=http://127.0.0.1:9 go test ./internal/agent ./internal/scan -run Boundary -count=1
--- FAIL: TestSelectFilesTooLarge_Boundary
    agent_test.go:650: fixture drift: llm.CountTokens(<80-token string>) = 39, want 80
--- FAIL: TestSelectScanItems_LargeBoundary
    agent_test.go:285: fixture drift: llm.CountTokens(<80-token string>) = 39, want 80

With this change, the full suite passes under those same offline conditions, and make check and make build pass.

Added TestCountTokensUsesEmbeddedBpeData, which never calls InitEmbeddedLoader and asserts the count is not the len(text)/4 estimate. I verified it fails (= 39) when the new init() is removed.

One environment note, for accuracy: make test uses -race, and the race detector cannot start on my arm64 box (ThreadSanitizer: unsupported VMA range, Found 47 - Supported 48) for any package, on main as well as on this branch. I ran the identical package list with LC_ALL=C go test -count=1 instead; all 23 packages pass. CI will exercise the -race path.

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA
  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

AI disclosure: this change was developed with AI assistance (Claude Opus 5, via Claude Code). I reviewed the diff and the reasoning myself, no commits are attributed to AI, and I will answer maintainer questions in my own words.

Related Issues

Closes #1012

Token counting fell back to a len(text)/4 byte estimate whenever the
tiktoken encoding could not be downloaded, because InitEmbeddedLoader was
only called by the CLI binary. Installing it from the package init keeps
every consumer, tests included, on the embedded data.

Fixes alibaba#1012
@CLAassistant

CLAassistant commented Sep 13, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 2 selected item(s).

@lizhengfeng101

Copy link
Copy Markdown
Contributor

@basil-k-aji-dev Great job! I actually ran into this exact problem locally yesterday!

@lizhengfeng101 lizhengfeng101 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.

LGTM

@lizhengfeng101
lizhengfeng101 merged commit 813d589 into alibaba:main Sep 14, 2026
13 checks passed
Githab-capibara added a commit to Githab-capibara/open-code-review that referenced this pull request Sep 17, 2026
…1228)

Token counting fell back to a len(text)/4 byte estimate whenever the
tiktoken encoding could not be downloaded, because InitEmbeddedLoader was
only called by the CLI binary. Installing it from the package init keeps
every consumer, tests included, on the embedded data.

Fixes alibaba#1012
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.

TestFilterLargeDiffs_Boundary fails because exactNTokens fixture is not token-exact

3 participants