Skip to content

fix(deep): chunk crux calls for symbol-dense files (#260) - #274

Open
Frankie-Xu wants to merge 2 commits into
trailhq:mainfrom
Frankie-Xu:fix/260-crux-chunk-large-files
Open

Frankie-Xu wants to merge 2 commits into
trailhq:mainfrom
Frankie-Xu:fix/260-crux-chunk-large-files

Conversation

@Frankie-Xu

Copy link
Copy Markdown
Contributor

Summary

  • One crux call per file hits maxTokens: 8192 around ~125 symbols. The OpenAI adapter's JSON.parse of the truncated tool arguments fails, parseResults returns [], and the whole file is dropped. Re-running --deep hits the same ceiling, so those files stay failed forever.
  • A (this PR): chunk targets at CRUX_CHUNK_SIZE = 80 (pinned by test; ~90 completion tokens/entry stays under 8192). Dense files take several sequential calls; results are merged. Small files still cost one call. finish_reason=length is logged instead of swallowed. Empty chunks stay pending and are not cached as ready (fix(deep): retry pending nodes — don't cache empty meaning summaries (#172) #177).
  • B (not this PR): bump maxTokens and retry once on finish_reason=length. Rejected: public/js/app.js has 698 targets (~65k completion tokens); there is no ceiling that covers that in one call.

Orthogonal to the #254 circuit breaker (quality misses vs quota) and Draft #270 (echoed TARGET-line ids). Does not touch --only-dir (#262), recover-tool (#269), or the echo peel.

Closes #260

Rebase vs Draft #270

Both PRs edit src/ai/crux.ts. This one only changes packing (describeFile splits + merges; parseResults untouched). #270 peels echoed ids inside parseResults. They compose: chunk, then peel per chunk.

Suggested order: land this, rebase #270 onto it (conflict should be describeFile vs parseResults, not overlapping hunks). If #270 lands first, rebase this onto it — the describeFile rewrite is the larger hunk.

Test plan

  • Mock ceiling at 125 symbols (the observed cliff, test-only — not a production constant). 200 targets in one call → 0 summaries; after chunking at 80 → 3 calls (80/80/40), 200 summaries. No DeepSeek key, no live API.
  • Small file (5 targets) is still one model.create
  • finish_reason=length with empty {} tool args logs truncated / finish_reason=length (the silent-drop shape)
  • Empty chunks: 200 nodes stay pending, not cached ready (fix(deep): retry pending nodes — don't cache empty meaning summaries (#172) #177)
  • A later empty chunk keeps the first chunk's summaries; missed ids stay pending
  • node --import tsx --test test/crux-chunk.test.ts test/llm-ops.test.ts test/graph-enrich-pending.test.ts test/graph-enrich-quality.test.ts test/graph-enrich-failures.test.ts
  • npm run build
  • No DeepSeek/OpenAI key in this window — did not replay a live dense PHP/JS file

Made with Cursor

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

🌱 graft blast radius

1 area changed → 2 areas can be affected. 4 dependent symbols, depth 2.
Tests: 1 area updated its tests.
Tag: @anirudhkumar-nanonets — 3 of 3 areas · @shhdwi — Crux Processing, Crux Summarization

flowchart TB
  A0(("Graph Enrichment<br/>3 symbols"))
  A1(("Crux Summarization<br/>1 symbol"))
  classDef reached fill:#D9EDF3,stroke:#3AA7C9,stroke-width:1.5px,color:#0E313C;
  class A0,A1 reached;
Loading
Can be affected Symbols Nearest hop Reached from
Graph Enrichment 3 src/graph/enrich.ts:L243-L246 cruxMissMessage — calls, depth 1 Crux Processing
Crux Summarization 1 src/engine.ts:L146-L148 cruxSummarizer — references, depth 2 Crux Processing
Who knows this code — 2 people across 3 areas
Area Who knows it
Crux Processing · changed @anirudhkumar-nanonets — 3 commits, last 2mo ago · @shhdwi — 1 commit, last 28d ago
Graph Enrichment · affected @anirudhkumar-nanonets — 4 commits, last 26d ago
Crux Summarization · affected @anirudhkumar-nanonets — 16 commits, last 2mo ago · @shhdwi — 3 commits, last 1mo ago

Ownership is git history over each area's own files, weighted towards recent work (120-day half-life). Merge commits and bots are dropped, and you are dropped from your own PR. A name with no @ has no GitHub handle in its commit email — tag them by hand, or add a .mailmap entry. A suggestion from history, not a CODEOWNERS rule.

All 4 dependent symbols, grouped by area

Graph Enrichment — 3 symbols in 1 file

  • src/graph/enrich.ts:L243-L246 — cruxMissMessage (calls, depth 1)
    243: function cruxMissMessage(summarizer: CruxSummarizer, fallback: CruxMissKind): string {
  • src/graph/enrich.ts:L253-L280 — collectFileCrux (calls, depth 2)
    254: summarizer: CruxSummarizer,
  • src/graph/enrich.ts:L74-L223 — enrichGraph (calls, depth 2)
    166: const refs: NodeRef[] = fileNodes.map((n) => {

Crux Summarization — 1 symbol in 1 file

  • src/engine.ts:L146-L148 — cruxSummarizer (references, depth 2)
    146: private cruxSummarizer(): CruxSummarizer {
Test signal per changed area — 1 ✓

Reached = a node under a test path has a resolved edge into the changed symbol. It undercounts anything called indirectly — through a CLI, a spawned process or a dynamic import — so read a low ratio as “look here”, never as a coverage gate.

  • Crux Processing — 1 of 13 reached · 1 test file changed here: test/crux-chunk.test.ts
    • not reached: argsFromResponse, constructor, describeChunk, describeFile, classifyCruxMiss, formatCruxMiss, isTruncatedStop, numberLines, …4 more
5 test suites also reference this code

10 symbols, kept out of the diagram and the table so they cannot crowd out the areas a reviewer has to look at.

  • test/graph-enrich-checkpoint.test.ts
  • test/graph-enrich-failures.test.ts
  • test/graph-enrich-pending.test.ts
  • test/graph-enrich-quality.test.ts
  • test/llm-ops.test.ts

graft blast · origin/main...HEAD · depth 2 · 2 changed files

Open the interactive graph → — click an area to see its dependent symbols at file:line.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Frankie-Xu
Frankie-Xu marked this pull request as ready for review September 11, 2026 12:35
@Frankie-Xu

Copy link
Copy Markdown
Contributor Author

Rebased onto current main. CI is green. Ready for review.

@SeatownSin

Copy link
Copy Markdown

Chunking is the right call over raising maxTokens — I reached the same conclusion independently
on a 42 MB repo, for the same reason you give in the "B (not this PR)" note. One thing I think
this misses, though, and I left the measurement on #260 before noticing this PR was open.

The chunk loop spreads the file's source unchanged into every chunk:

for (const nodes of chunkCruxTargets(input.nodes)) {
  const { parsed, res } = await this.describeChunk({ ...input, nodes });

describeChunk builds its prompt with userContent(), which still calls
numberLines(input.source) — and numberLines clips at MAX_CODE_CHARS (18k) from the start
of the file
. So chunk 5 of a 7,838-line file is shown the same first ~450 lines that chunk 1
was, and is asked about symbols at line 7,000 that appear nowhere in its prompt.

The output-budget problem is genuinely fixed by this PR. What's left is that the binding
constraint moves: those later chunks now return successfully, describing symbols from their
signature alone. That is worse than the truncation it replaces, because truncation was at least
loud — finish_reason=length, which this PR now logs properly. A confident summary for a
function the model never saw looks exactly like a good one.

The fix composes with what's already here — window the source alongside the targets. Split
on the chunk's real source span rather than a fixed target count, send that slice, and keep the
line numbers file-absolute (buildCrux slices the file by whatever the model reports back, and
#260 flags the same constraint).

Measured on a synthetic 500 KB / 8,000-line / 200-symbol file with a stub model:

requests made    : 29
symbols returned : 200 / 200
targets whose start line was NOT in the shown source : 0

That last line is the assertion worth adding to test/crux-chunk.test.ts. The current dense-file
test uses source: "export const x = 1;\n" for every node, so the source window is never
exercised — a chunker that shows the wrong 18k passes it. Something like: build the fixture with
real distinct line ranges, then assert every target's crux_start falls inside the source that
chunk's prompt actually contained.

On a real 14-file / 1,447-symbol directory, source-span windowing took coverage 6% → 96%, and
the remaining 4% was the decorated-id bug (#327 / #332). Happy to share the windowing patch if
it's useful here — it's about 60 lines and sits in the same describeFile you've already
rewritten, so it would be a rebase on this rather than a competing PR.

@Frankie-Xu

Copy link
Copy Markdown
Contributor Author

Thanks @SeatownSin — you are right. Chunking fixed the output-budget miss, but each chunk still sent the whole input.source through numberLines, which always clips 18k chars from the start of the file. Later TARGETS then sat at L7000 while the prompt showed the first ~450 lines, so the model could write a confident summary of a function it never saw.

Pushed 03655c2 on this branch (not a competing PR):

  • userContent now slices source to that chunk's real startLineendLine span.
  • Numbered lines in the prompt stay file-absolute, so buildCrux can still cut by the model's returned range.
  • test/crux-chunk.test.ts adds a fixture the one-line dense-file test could not: 80 short functions, then padding past 18k, then a late symbol. Each chunk's prompt must contain its targets' absolute lines.

Did not take a pasted patch. Did not touch parseResults / id peel (#332 / #377) or maxTokens.

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.

--deep: one crux call per file cannot describe a file with more than ~125 symbols — the truncated tool response is silently dropped

2 participants