Skip to content

fix(crux): terminate the id field in target lines so a verbatim echo matches - #377

Open
Def3nder wants to merge 1 commit into
trailhq:mainfrom
Def3nder:fix/crux-target-id-delimiter
Open

Def3nder wants to merge 1 commit into
trailhq:mainfrom
Def3nder:fix/crux-target-id-delimiter

Conversation

@Def3nder

Copy link
Copy Markdown

What happens

Against a local model (qwen3.8:27b on Ollama, OpenAI-compatible endpoint), the
meaning pass reported four of sixteen files as total misses:

✗ scraper/scrape_all.js: model returned no usable symbol summaries [empty-parsed, finish_reason=null]
✗ tts/jobs.cjs:          model returned no usable symbol summaries [empty-parsed, finish_reason=null]
✗ tts/jobs.test.cjs:     model returned no usable symbol summaries [empty-parsed, finish_reason=null]
✗ tts/markdown_tts.test.js: model returned no usable symbol summaries [empty-parsed, finish_reason=null]

✗ the deep pass did not complete — the meaning tier is incomplete.
  meaning coverage: 211/292 symbols (72%).

The model was neither truncated nor confused: for tts/jobs.cjs it answered
with finish_reason: tool_calls after 113 completion tokens, with a perfectly
usable summary. The reply was discarded anyway.

Why

userContent() in src/ai/crux.ts builds each target line as:

`- id=${n.id} | ${n.kind} | lines L${n.startLine}-L${n.endLine}` + (n.signature ? ` | ${n.signature}` : "")

The id= field has no terminator — visually the id runs to the end of the line.
The system prompt asks for that id verbatim:

Return EXACTLY ONE entry for EVERY target id, using that id verbatim.

So the model returned the whole line as the id. Captured at the wire
(requested id → returned id):

"tts/jobs.cjs"             ->  "tts/jobs.cjs | file | lines L1-L168"
"tts/jobs.test.cjs#cancel" ->  "tts/jobs.test.cjs#cancel | method | lines L27-L27 | cancel()"

enrich.ts then looks up results.get(node.id), misses every node, and
applied stays 0, which is reported as a total miss:

if (!fileError && refs.length > 0 && applied === 0) {
  fileError = cruxMissMessage(summarizer, "empty-parsed");

The finish_reason=null in the message is the tell: classifyCruxMiss returned
null (at least one non-empty summary was parsed), so lastMiss was unset. The
model did its job; only the key did not match.

Fix

Both ends, so neither depends on the other:

  1. id= is the LAST field on a target line. "Everything after id=" is then
    exactly the id, with no quoting rule for the model to get wrong:

    - method | lines L27-L27 | cancel() | id=tts/jobs.test.cjs#cancel
    
  2. normalizeId() repairs a whole-line echo on the way in, for the new
    layout (… | id=<id>) and the previous one (<id> | kind | …). Ids are
    minted as `${ctx.rel}#${scope.join(".")}` in graph/extract.ts, so
    neither | nor id= can occur inside a real id and both cuts are safe.

A model that already returned bare ids is unaffected: normalizeId returns such
a string unchanged.

Scope

src/ai/crux.ts only. No behaviour change for a model that answers correctly
today; no new option, no config.

Test

test/crux-id-normalization.test.ts covers: the target line ends with the id;
a bare id passes through; a whole-line echo of either layout is repaired; and a
repaired reply is not classified as a miss.

Notes

Observed with qwen3.8:27b via Ollama, but nothing about it is provider-specific
— it is an ambiguous prompt format, and any model that follows "verbatim"
literally can hit it. Cost here was the whole meaning tier for the four affected
files.

@trailhq-graft

trailhq-graft Bot commented Sep 13, 2026

Copy link
Copy Markdown

🌱 graft blast radius

Nothing outside this diff depends on it. 1 area changed; no indexed dependents at depth 2.
Tests: no test reaches AI Result Parsing.
Tag: @Frankie-Xu — AI Result Parsing · @anirudhkumar-nanonets — AI Result Parsing

Who knows this code — 2 people across 1 area
Area Who knows it
AI Result Parsing · changed @Frankie-Xu — 3 commits, last 13d ago · @anirudhkumar-nanonets — 3 commits, last 2mo 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.

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.

  • AI Result Parsing — 0 of 3 reached · no test file reaches it
    • not reached: userContent, normalizeId, parseResults

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.

@Frankie-Xu Frankie-Xu 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.

Reviewing against #332 (parse-side peel, Fixes #327) and the closed #270, not as a third peel.

Prompt reorder. Putting id= last so a verbatim echo is unambiguous is a real improvement for new logs. It will conflict with #274's userContent (still - id=<id> | kind | lines …) and does not by itself fix the captured #259 / #327 samples, which used the old layout.

normalizeId vs #332. Keep the #332 peel as the compatibility layer. Two reasons:

  1. On an old-layout echo id=a.ts | file | lines L1-L5, lastIndexOf(id=) is 0, so this returns a.ts | file | lines L1-L5 instead of a.ts. The previous layout test uses a line without the id= prefix (tts/jobs.cjs | file | …), so it does not cover the #327 Grok sample. #332 cuts at the first | before stripping id=, which does.
  2. A second parseResults rewriter stacked on #332 / #274 is three-way conflict for little extra coverage once the cut-at-| peel exists.

The prompt tweak is additive if it lands after a peel, not a replacement. I would not have this close #259 / #327.

Not requesting a competing peel from here.

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.

2 participants