✅ Local verification — fix(core): read BMP height as signed int32 for top-down bitmaps

Verdict: LGTM — safe to merge. The fix is correct, minimal, and properly guarded by the two new tests. One non-blocking note about the PR description's symptom wording (§4 below).

Verified as maintainer on 🐧 Linux (the OS the PR marked not tested), Node v22.22.2, against PR head ade8f2d. Real testing in an isolated tmux socket: unit tests both directions → harness over the real built packages/core/dist with real on-disk BMP files → confirmation the fix shipped into the user-facing dist/cli.js bundle → a live end-to-end CLI turn.

1. Unit tests & static checks

• PR branch: imageTokenizer.test.ts → 13/13 pass; full request-tokenizer/ suite → 55/55 pass; tsc --noEmit on packages/core → clean.
• Regression proof — reverting only the one-line change (readInt32LE → readUInt32LE) fails exactly the top-down case while the bottom-up case keeps passing, so the test genuinely guards the bug:

× ImageTokenizer > BMP dimension extraction > reads a top-down (negative height) BMP as its absolute height
  → expected 4294967246 to be 50
 Tests  1 failed | 12 passed (13)

2. Real built code + real BMP files

Generated two genuine BMPs with PIL (15054 bytes each, real pixel data): a bottom-up (height +50) and a top-down (height -50; header bytes at offset 22 = ce ff ff ff). Fed both through the built ImageTokenizer / RequestTokenEstimator (not a re-implementation):

=== top-down (height -50) | topdown.bmp (15054 bytes) ===
  raw header @22  readUInt32LE=4294967246  readInt32LE=-50
  extractImageMetadata -> width=100 height=50            ✅ recovered
  calculateTokens (fixed dim 100x50)         = 10
  calculateTokens (buggy dim 100x4294967246) = 2         (garbage)
  RequestTokenEstimator(request): totalTokens=10 imageTokens=10
=== bottom-up (height +50) ===
  10 tokens both ways → common case untouched             ✅

3. Live tmux end-to-end

(a) The exact computation OpenAIContentGenerator.countTokens() performs (new RequestTokenEstimator().calculateTokens(request)), run live on the real top-down BMP:

LIVE countTokens() on real top-down BMP (100x-50):
  raw @22 uint32 : 4294967246 (pre-fix)   int32 : -50 (post-fix)
  => totalTokens : 10   breakdown {"imageTokens":10,...}
  RESULT: PASS (sane 10-token count, not 2/131074 garbage)

(b) A full real CLI turn — qwen -p '… @topdown.bmp …' — ingested the real top-down file end-to-end; the vision model read it back as 100x50, exit 0, no crash on the would-be 4-billion dimension.

4. 📌 Non-blocking note on the PR description

The motivation text says the bug yields "a large, silent over-count … pinned to the per-image maximum." Running the real calculateTokens across many top-down dimensions shows the opposite for typical images: after scaling, the (real-sized) width floors to 0, so the estimate collapses to just the 2 special tokens — a silent under-count. The over-count direction only shows up for pathological near-1px-wide images:

width height  preFix(uint32)  postFix(abs)
100   50      2               10       <- under-count
1920  1080    2               2693     <- under-count
4000  3000    2               15303    <- under-count
1     1       131074          6        <- over-count (degenerate only)

Either way the pre-fix estimate is garbage by orders of magnitude and the fix is unambiguously correct — only the description's wording could be tweaked. Does not affect the merge decision.

Out of scope (FYI — pre-existing, not introduced here)

• width stays unsigned (readUInt32LE) — fine; BMP width is positive by spec.
• The TIFF type === 3 ? value : value no-op ternaries (≈ lines 429/432) are unrelated to this PR.

Recommendation: merge. ✅

× ... reads a top-down (negative height) BMP as its absolute height
  → expected 4294967246 to be 50
 Tests  1 failed | 12 passed (13)

=== top-down (height -50) | topdown.bmp (15054 字节) ===
  原始头 @22  readUInt32LE=4294967246  readInt32LE=-50
  extractImageMetadata -> width=100 height=50            ✅ 已还原
  calculateTokens (修复后维度 100x50)        = 10
  calculateTokens (错误维度 100x4294967246)  = 2          (垃圾值)
  RequestTokenEstimator(request): totalTokens=10 imageTokens=10
=== bottom-up (height +50) ===
  两种读法都是 10 tokens → 常见路径不受影响               ✅

真实 top-down BMP (100x-50) 上的 countTokens()：
  raw @22 uint32 : 4294967246 (修复前)   int32 : -50 (修复后)
  => totalTokens : 10   breakdown {"imageTokens":10,...}
  结果：PASS（合理的 10 tokens，而非 2 / 131074 垃圾值）

width height  修复前(uint32)  修复后(abs)
100   50      2               10       <- 少算
1920  1080    2               2693     <- 少算
4000  3000    2               15303    <- 少算
1     1       131074          6        <- 超额（仅退化情形）

_{Verification artifacts: real BMP fixtures (md5 topdown.bmp = 79c3d11f…), dist harness, and live tmux captures retained locally.}

@qwen-code /triage

Thanks for the PR!

Template looks good ✓

On direction: this fixes a real, concrete bug — top-down BMPs (negative height) produce a ~4-billion-pixel dimension that corrupts token estimation. The code already had the Math.abs() intent but the unsigned read defeated it. Claude Code's CHANGELOG has a related BMP fix ("Fixed image pasting not working on WSL2 systems where Windows copies images as BMP format"), confirming the BMP path matters in practice. Clearly aligned with the project.

On approach: this is about as minimal as a fix gets — one line changed (readUInt32LE → readInt32LE), one comment added, two focused tests. Positive heights read identically signed or unsigned, so the common bottom-up case is untouched. Width stays unsigned, which is correct. Nothing to cut.

Moving on to code review and testing. 🔍

— Qwen Code · qwen3.7-max

Code Review

My independent proposal: change readUInt32LE(22) → readInt32LE(22) in extractBmpDimensions. The existing Math.abs(height) on the return line already handles the negative case — the bug is purely that the unsigned read converts -50 to 4294967246 before Math.abs ever sees it. Add two tests (positive and negative height).

The PR's approach matches exactly. One-line production change, two focused tests, width correctly left as unsigned. Nothing missed, nothing extra. No blockers.

Testing

Unit tests (PR branch): 13/13 passed, including both new BMP cases.

$ cd packages/core && npx vitest run src/utils/request-tokenizer/imageTokenizer.test.ts

 RUN  v3.2.4 packages/core
      Coverage enabled with v8

 ✓ src/utils/request-tokenizer/imageTokenizer.test.ts (13 tests) 9ms

 Test Files  1 passed (1)
      Tests  13 passed (13)
   Duration  3.92s

TypeScript: tsc --noEmit on packages/core — clean, no errors.

Regression proof — reverting only readInt32LE → readUInt32LE fails exactly the top-down case while bottom-up still passes:

$ sed -i 's/buffer.readInt32LE(22)/buffer.readUInt32LE(22)/' .../imageTokenizer.ts
$ npx vitest run src/utils/request-tokenizer/imageTokenizer.test.ts

 ❯ src/utils/request-tokenizer/imageTokenizer.test.ts (13 tests | 1 failed) 16ms
   ✓ ... reads a bottom-up (positive height) BMP 0ms
   × ... reads a top-down (negative height) BMP as its absolute height 8ms
     → expected 4294967246 to be 50 // Object.is equality

- Expected
+ Received

- 50
+ 4294967246

 Test Files  1 failed (1)
      Tests  1 failed | 12 passed (13)

The test genuinely guards the bug — restoring readInt32LE makes it pass again.

— Qwen Code · qwen3.7-max

This is a textbook bugfix — the kind of PR you wish every contribution looked like.

The bug is real and well-diagnosed: BMP height is a signed int32, the code read it unsigned, Math.abs on a huge positive number does nothing. The fix is one line (readUInt32LE → readInt32LE), the existing Math.abs does the rest, and the comment explains why. Width stays unsigned because BMP width is always positive — correct asymmetry.

The two tests are exactly the right shape: one bottom-up (positive height, common case, should keep working), one top-down (negative height, the bug). The regression proof — reverting the one-line change causes exactly the top-down test to fail while bottom-up passes — confirms the tests genuinely guard the fix rather than just happening to pass.

The existing maintainer verification comment on this PR already covers real BMP files through the built ImageTokenizer and a live CLI turn. My testing confirms the unit tests and regression guard. Nothing left to verify.

Verdict: Approve. ✅

— Qwen Code · qwen3.7-max