Skip to content

fix(review): newlineChunks can fail to make forward progress despite chunkOverlap < chunkChars #7447

Description

@JSONbored

Context

src/review/rag.ts's chunkFile (line 188) clamps chunkOverlap to chunkChars - 1 with a
comment claiming this prevents newlineChunks's while-loop from stalling ("#rag-verify
infinite-loop guard", line 198-200). This is incomplete: newlineChunks (line 280) can shrink
end well below start + chunkChars via its newline-snap heuristic (if (nl > start + chunkChars / 2) end = nl + 1), and when chunkOverlap is close to chunkChars (a value the existing clamp
explicitly permits), start = Math.max(0, end - chunkOverlap) can land at or below the previous
iteration's start — the loop then repeats the identical start/end pair forever. Reproduced
directly: chunkFile("f.py", "x".repeat(60) + "\n" + "y".repeat(5000), "", { chunkChars: 100, chunkOverlap: 99 }) never returns (both values pass the existing clamp).

Both production call sites (src/review/rag-index.ts:333,404) use the hardcoded defaults
(CHUNK_CHARS/CHUNK_OVERLAP, ~9% overlap ratio) and never trip this today, but chunkFile and
its ChunkOpts parameter are exported and public — any future caller that exposes
chunkChars/chunkOverlap as configuration (e.g. a per-repo RAG tuning surface) can trigger an
unbounded synchronous CPU burn (the loop is synchronous, so it blocks the isolate — not even a
same-process timer fires while it spins) with a single high-overlap value, which is exactly the
misconfig-DOS class the existing comment already guards against for the chunkChars <= 0 case, but
does not actually close here.

Requirements

  • newlineChunks (or the clamp that feeds it) must guarantee that every loop iteration strictly
    increases start, regardless of the newline-snap interaction — not just that chunkOverlap < chunkChars. Fix at the clamp OR at the loop; either is acceptable as long as forward progress is
    provably guaranteed for every valid chunkChars >= 1 and every chunkOverlap the clamp can ever
    produce.
  • The fix must not change today's actual chunk boundaries for the two production call sites (which
    never pass opts and so use CHUNK_CHARS/CHUNK_OVERLAP — well outside the failure region) —
    this is a defensive-bound fix for the general/configurable case, not a behavior change to the
    current default chunking.
  • Must not regress chunkJsTs's own fallback path (line 268), which also calls newlineChunks with
    caller-supplied chunkChars/chunkOverlap for an oversized single logical unit.

Deliverables

  • A provable forward-progress guarantee added to newlineChunks and/or its callers' clamp (e.g.
    an explicit Math.max(start + 1, end - chunkOverlap) floor, or tightening the chunkOverlap
    clamp to a value that can never combine with the newline-snap to stall — document whichever
    approach is chosen and why it's sufficient)
  • Regression test reproducing the exact failure above (chunkChars: 100, chunkOverlap: 99 over
    an oversized input with an early qualifying newline) that asserts chunkFile returns in
    bounded time/iterations instead of looping
  • A property-style or table-driven test sweeping a range of chunkChars/chunkOverlap
    combinations (including the previous stalling pair and boundary values like chunkOverlap = chunkChars - 1) asserting the returned chunk count is always finite and start strictly
    increases every iteration

Test Coverage Requirements

This repo's Codecov patch gate is 99%+ hard (branch-counted) on every changed line/branch in
src/**. The new test(s) must actually exercise chunkFile/newlineChunks with a real
would-have-stalled input, not just assert on the clamp's numeric output.

Expected Outcome

chunkFile/newlineChunks can no longer be driven into an unbounded loop by any combination of
chunkChars >= 1 and a chunkOverlap the function's own clamp allows — the "#rag-verify
infinite-loop guard" comment's claim becomes actually true for every input, not just the
chunkChars <= 0 case it currently covers.

Links & Resources

src/review/rag.ts:188 (chunkFile), :198-202 (the incomplete clamp + its comment), :280-297
(newlineChunks), :233-277 (chunkJsTs, the other caller of newlineChunks with
caller-supplied values). src/review/rag-index.ts:333,404 (the two production call sites, for
confirming they're unaffected by the fix).

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions