Skip to content

Commit 25b6452

Browse files
uaytenclaude
andauthored
fix(crux): match echoed target ids against the requested ids and name the miss (#492)
Some models (gpt-5.6-luna, deepseek, gemini, gpt-4o-mini) copy the whole target row into the returned id. enrich looks summaries up by node id, so every summary was dropped and the file failed as [empty-parsed, finish_reason=null]. - resolveTargetId maps a returned id onto a requested one: exact; requested id + " | "; the same after a leading "id="; a row ending in " | id=<id>". The longest candidate wins, and an unmatched id keeps its raw text. - classifyCruxMiss reports id-mismatch, with the first returned id, when a reply has entries but none was requested. The id is printed on its own line and kept out of the failure gate, whose quota/auth match would otherwise read source text such as a symbol named Quota. - Target rows end with "| id=<id>" so the id field is terminated (#377). Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent c07101c commit 25b6452

4 files changed

Lines changed: 230 additions & 15 deletions

File tree

‎src/ai/crux.ts‎

Lines changed: 99 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,19 @@ export interface CruxSummarizer {
4747
}
4848

4949
/** Why a crux call produced no usable summaries (#235). */
50-
export type CruxMissKind = "empty-toolCalls" | "unparseable" | "truncated" | "empty-parsed";
50+
export type CruxMissKind =
51+
| "empty-toolCalls"
52+
| "unparseable"
53+
| "truncated"
54+
| "empty-parsed"
55+
| "id-mismatch";
5156

5257
export interface CruxMiss {
5358
kind: CruxMissKind;
5459
finishReason: string | null;
60+
/** `id-mismatch` only: how many entries came back, and the first one's raw id. */
61+
returned?: number;
62+
firstId?: string;
5563
}
5664

5765
function isTruncatedStop(reason: string | null): boolean {
@@ -60,11 +68,24 @@ function isTruncatedStop(reason: string | null): boolean {
6068
return r === "length" || r === "max_tokens";
6169
}
6270

63-
/** Classify an empty/unusable crux reply. `null` means at least one usable summary. */
64-
export function classifyCruxMiss(res: ChatResponse, parsed: NodeCrux[]): CruxMiss | null {
71+
/**
72+
* Classify an empty/unusable crux reply. `null` means at least one usable summary.
73+
* With `requested`, a summary only counts when its id is one we asked for: enrich
74+
* looks results up by node id, so a summary under any other id is never applied.
75+
*/
76+
export function classifyCruxMiss(
77+
res: ChatResponse,
78+
parsed: NodeCrux[],
79+
requested?: readonly string[],
80+
): CruxMiss | null {
6581
const finishReason = res.stopReason;
66-
if (parsed.some((p) => p.summary.trim())) return null;
82+
const wanted = requested ? new Set(requested) : undefined;
83+
const matched = wanted ? parsed.filter((p) => wanted.has(p.id)) : parsed;
84+
if (matched.some((p) => p.summary.trim())) return null;
6785
if (isTruncatedStop(finishReason)) return { kind: "truncated", finishReason };
86+
if (parsed.length > 0 && matched.length === 0) {
87+
return { kind: "id-mismatch", finishReason, returned: parsed.length, firstId: parsed[0].id };
88+
}
6889
if (parsed.length > 0) return { kind: "empty-parsed", finishReason };
6990
const emptyTools = res.toolCalls.length === 0;
7091
const emptyText = !res.text?.trim();
@@ -75,9 +96,29 @@ export function classifyCruxMiss(res: ChatResponse, parsed: NodeCrux[]): CruxMis
7596
/** Per-file error text: miss class + the provider's finish_reason (#235). */
7697
export function formatCruxMiss(kind: CruxMissKind, finishReason: string | null): string {
7798
const fr = finishReason == null || finishReason === "" ? "null" : finishReason;
99+
if (kind === "id-mismatch") {
100+
return `model returned summaries but no id matched a requested target [${kind}, finish_reason=${fr}]`;
101+
}
78102
return `model returned no usable symbol summaries [${kind}, finish_reason=${fr}]`;
79103
}
80104

105+
/** Longest echoed id shown in an error; a whole echoed row can carry a long signature. */
106+
const MAX_SHOWN_ID_CHARS = 200;
107+
108+
/**
109+
* The model's text for an `id-mismatch`, kept apart from {@link formatCruxMiss}:
110+
* the failure gate matches words like "quota" in the error message, and a
111+
* returned id can carry any source text, so it must never reach the gate.
112+
*/
113+
export function formatCruxMissDetail(miss: CruxMiss | null | undefined): string {
114+
if (miss?.kind !== "id-mismatch" || miss.firstId === undefined) return "";
115+
const id =
116+
miss.firstId.length > MAX_SHOWN_ID_CHARS
117+
? `${miss.firstId.slice(0, MAX_SHOWN_ID_CHARS)}…`
118+
: miss.firstId;
119+
return `first of ${miss.returned ?? 1} returned id(s): ${JSON.stringify(id)}`;
120+
}
121+
81122
const SYSTEM_PROMPT = `You explain code definitions for a code graph that helps engineers navigate a codebase.
82123
83124
You are given ONE source file with 1-based line numbers, and a list of TARGET definitions in it. Describe EVERY target via the record_symbols tool.
@@ -122,27 +163,73 @@ function numberLines(source: string): string {
122163
.join("\n");
123164
}
124165

166+
/**
167+
* `id=` is the LAST field of a target line, so the id ends where the line does.
168+
* With the id first nothing terminated it, and models asked for the id
169+
* "verbatim" copied the whole row — `a.cs#Level | variable | lines L58-L58 | sig`
170+
* — into `id` (#259). {@link resolveTargetId} still repairs echoes of either layout.
171+
*/
125172
function userContent(input: FileCruxInput): string {
126173
const targets = input.nodes
127174
.map(
128175
(n) =>
129-
`- id=${n.id} | ${n.kind} | lines L${n.startLine}-L${n.endLine}` +
130-
(n.signature ? ` | ${n.signature}` : ""),
176+
`- ${n.kind} | lines L${n.startLine}-L${n.endLine}` +
177+
(n.signature ? ` | ${n.signature}` : "") +
178+
` | id=${n.id}`,
131179
)
132180
.join("\n");
133181
const n = input.nodes.length;
134182
return `FILE: ${input.path}\n\n${numberLines(input.source)}\n\nTARGETS (${n} — return all ${n}, one entry per id):\n${targets}`;
135183
}
136184

137-
/** Normalize the tool's parsed argument object into a {@link NodeCrux} list. */
138-
function parseResults(obj: { symbols?: unknown } | undefined): NodeCrux[] {
185+
/** The longest requested id `test` accepts, or null. */
186+
function longestMatch(requested: readonly string[], test: (id: string) => boolean): string | null {
187+
let best: string | null = null;
188+
for (const id of requested) if (test(id) && (best === null || id.length > best.length)) best = id;
189+
return best;
190+
}
191+
192+
/**
193+
* Map an id the model returned onto one of the ids we asked for, or null.
194+
*
195+
* Models echo decorated target rows instead of the bare id (#259, #298, #486).
196+
* Rather than cut the string at a guessed delimiter, every rule compares against
197+
* the requested ids, so the result is always an id we asked for — never a
198+
* fragment of a hallucinated one — and no rule assumes which characters an id
199+
* may contain. In order:
200+
* 1. exact match;
201+
* 2. a requested id followed by ` | ` (the row echoed from the id onwards);
202+
* 3. the same two with a leading `id=` removed;
203+
* 4. a row ending in ` | id=<requested id>` (the current layout echoed whole).
204+
* When several requested ids qualify, the longest wins.
205+
*/
206+
export function resolveTargetId(raw: string, requested: readonly string[]): string | null {
207+
const id = raw.trim();
208+
if (requested.includes(id)) return id;
209+
const echoed = longestMatch(requested, (r) => id.startsWith(`${r} | `));
210+
if (echoed) return echoed;
211+
if (id.startsWith("id=")) {
212+
const bare = id.slice(3).trim();
213+
if (requested.includes(bare)) return bare;
214+
const bareEchoed = longestMatch(requested, (r) => bare.startsWith(`${r} | `));
215+
if (bareEchoed) return bareEchoed;
216+
}
217+
return longestMatch(requested, (r) => id.endsWith(` | id=${r}`));
218+
}
219+
220+
/**
221+
* Normalize the tool's parsed argument object into a {@link NodeCrux} list. Ids
222+
* are resolved against `requested`; one that matches nothing keeps its raw text
223+
* so {@link classifyCruxMiss} can report what the model actually sent.
224+
*/
225+
function parseResults(obj: { symbols?: unknown } | undefined, requested: readonly string[]): NodeCrux[] {
139226
if (!obj || !Array.isArray(obj.symbols)) return [];
140227
const num = (v: unknown) => (typeof v === "number" && Number.isFinite(v) ? Math.trunc(v) : 0);
141228
return obj.symbols
142229
.map((s) => s as Record<string, unknown>)
143230
.filter((s) => typeof s.id === "string")
144231
.map((s) => ({
145-
id: s.id as string,
232+
id: resolveTargetId(s.id as string, requested) ?? (s.id as string),
146233
summary: typeof s.summary === "string" ? s.summary.trim() : "",
147234
crux_start: num(s.crux_start),
148235
crux_end: num(s.crux_end),
@@ -196,8 +283,9 @@ export class ChatCruxSummarizer implements CruxSummarizer {
196283
{ role: "user", content: userContent(input) },
197284
],
198285
});
199-
const parsed = parseResults(argsFromResponse(res));
200-
this.lastMiss = classifyCruxMiss(res, parsed);
286+
const requested = input.nodes.map((n) => n.id);
287+
const parsed = parseResults(argsFromResponse(res), requested);
288+
this.lastMiss = classifyCruxMiss(res, parsed, requested);
201289
return parsed;
202290
}
203291
}

‎src/graph/enrich.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
* once, to cut `crux.code` verbatim from the source. `crux.span` is a pointer
2020
* only and is never used to re-slice.
2121
*/
22-
import { formatCruxMiss, type CruxMissKind, type CruxSummarizer, type NodeCrux, type NodeRef } from "../ai/crux.js";
22+
import { formatCruxMiss, formatCruxMissDetail, type CruxMissKind, type CruxSummarizer, type NodeCrux, type NodeRef } from "../ai/crux.js";
2323
import { LlmFailureGate } from "../ai/failure.js";
2424
import type { Crux, NodeV1 } from "./types.js";
2525

@@ -171,7 +171,7 @@ export async function enrichGraph(
171171
const { results, error, quality } = await collectFileCrux(summarizer, path, source, refs);
172172
let fileError = error;
173173
if (fileError) {
174-
stats.errors.push(`${path}: ${fileError}`);
174+
stats.errors.push(`${path}: ${fileError}${cruxMissDetail(summarizer)}`);
175175
gate.record(fileError, { quality });
176176
}
177177

@@ -198,7 +198,7 @@ export async function enrichGraph(
198198
// collectFileCrux already labels a total miss; this catches "got entries
199199
// but every summary was blank" so the CLI's #127 degraded-exit path fires.
200200
fileError = cruxMissMessage(summarizer, "empty-parsed");
201-
stats.errors.push(`${path}: ${fileError}`);
201+
stats.errors.push(`${path}: ${fileError}${cruxMissDetail(summarizer)}`);
202202
gate.record(fileError, { quality: true });
203203
} else if (!fileError) {
204204
gate.succeeded();
@@ -245,6 +245,12 @@ function cruxMissMessage(summarizer: CruxSummarizer, fallback: CruxMissKind): st
245245
return formatCruxMiss(miss?.kind ?? fallback, miss?.finishReason ?? null);
246246
}
247247

248+
/** The echoed id behind an `id-mismatch`, as an indented second line; "" otherwise. */
249+
function cruxMissDetail(summarizer: CruxSummarizer): string {
250+
const detail = formatCruxMissDetail(summarizer.lastMiss);
251+
return detail ? `\n ${detail}` : "";
252+
}
253+
248254
/**
249255
* Describe every requested definition in a file, re-asking for any the model
250256
* omits (it sometimes drops entries from a batch). Returns whatever it collected

‎test/graph-enrich-quality.test.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,3 +184,29 @@ test("#235: blank parsed summaries name empty-parsed and stay pending", async ()
184184
assert.equal(nodes[0].summary_state, "pending");
185185
assert.equal(nodes[0].summary, null);
186186
});
187+
188+
test("#259: unmatched ids name id-mismatch with the returned id, and never reach the gate", async () => {
189+
// A returned id can hold any source text; "Quota" here must not read as a quota error.
190+
const invented = "f0.ts#Quota | function | lines L1-L3";
191+
const summarizer = new ChatCruxSummarizer(
192+
new CannedModel({
193+
toolCalls: [
194+
{
195+
id: "1",
196+
name: "record_symbols",
197+
args: { symbols: [{ id: invented, summary: "runs it", crux_start: 0, crux_end: 0 }] },
198+
},
199+
],
200+
stopReason: "tool_calls",
201+
}),
202+
);
203+
const { stats, nodes } = await oneFile(summarizer);
204+
assert.equal(stats.failedFiles, 1);
205+
assert.equal(stats.fatal, undefined);
206+
assert.equal(
207+
stats.errors[0],
208+
"f0.ts: model returned summaries but no id matched a requested target [id-mismatch, finish_reason=tool_calls]" +
209+
`\n first of 1 returned id(s): ${JSON.stringify(invented)}`,
210+
);
211+
assert.equal(nodes[0].summary_state, "pending");
212+
});

‎test/llm-ops.test.ts‎

Lines changed: 96 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,12 @@ import { test } from "node:test";
88
import assert from "node:assert/strict";
99
import { ChatSummarizer } from "../src/ai/summarize.js";
1010
import { ChatSynthesizer } from "../src/ai/synthesize.js";
11-
import { ChatCruxSummarizer } from "../src/ai/crux.js";
11+
import {
12+
ChatCruxSummarizer,
13+
formatCruxMiss,
14+
formatCruxMissDetail,
15+
resolveTargetId,
16+
} from "../src/ai/crux.js";
1217
import { recoverToolArgsFromContent } from "../src/ai/llm/recover-tool.js";
1318
import type { ChatModel, ChatRequest, ChatResponse, ToolCall } from "../src/ai/llm/types.js";
1419

@@ -68,6 +73,96 @@ test("ChatCruxSummarizer forces record_symbols and normalizes numbers", async ()
6873
assert.deepEqual(out, [{ id: "sym1", summary: "does x", crux_start: 3, crux_end: 5 }]);
6974
});
7075

76+
/** A crux reply carrying one entry per id in `ids`, in order. */
77+
function cruxReply(ids: string[]): FakeChatModel {
78+
const symbols = ids.map((id, i) => ({ id, summary: `summary ${i}`, crux_start: 0, crux_end: 0 }));
79+
return new FakeChatModel({ toolCalls: [{ id: "1", name: "record_symbols", args: { symbols } }] });
80+
}
81+
82+
const CRUX_INPUT = {
83+
path: "Runtime/Core/Ctx.cs",
84+
source: "l1\nl2\nl3\nl4\nl5\n",
85+
nodes: [
86+
{ id: "Runtime/Core/Ctx.cs", kind: "file" as const, signature: null, startLine: 1, endLine: 5 },
87+
{
88+
id: "Runtime/Core/Ctx.cs#Level",
89+
kind: "variable" as const,
90+
signature: "public int Level { get; }",
91+
startLine: 3,
92+
endLine: 3,
93+
},
94+
],
95+
};
96+
97+
test("crux target lines end with the id, so the id field is terminated (#259)", async () => {
98+
const m = cruxReply(["Runtime/Core/Ctx.cs", "Runtime/Core/Ctx.cs#Level"]);
99+
await new ChatCruxSummarizer(m).describeFile(CRUX_INPUT);
100+
const user = m.last?.messages.find((x) => x.role === "user")?.content ?? "";
101+
const rows = user.split("\n").filter((l) => l.startsWith("- "));
102+
assert.deepEqual(rows, [
103+
"- file | lines L1-L5 | id=Runtime/Core/Ctx.cs",
104+
"- variable | lines L3-L3 | public int Level { get; } | id=Runtime/Core/Ctx.cs#Level",
105+
]);
106+
});
107+
108+
test("ChatCruxSummarizer keeps an exact id and reports no miss", async () => {
109+
const s = new ChatCruxSummarizer(cruxReply(["Runtime/Core/Ctx.cs", "Runtime/Core/Ctx.cs#Level"]));
110+
const out = await s.describeFile(CRUX_INPUT);
111+
assert.deepEqual(out.map((r) => r.id), ["Runtime/Core/Ctx.cs", "Runtime/Core/Ctx.cs#Level"]);
112+
assert.equal(s.lastMiss, null);
113+
});
114+
115+
test("ChatCruxSummarizer maps a whole echoed target row back to the requested id (#259)", async () => {
116+
const s = new ChatCruxSummarizer(
117+
cruxReply([
118+
// gpt-5.6-luna on 0.20.0: the old `id=` first row, echoed from the id onwards.
119+
"Runtime/Core/Ctx.cs | file | lines L1-L5",
120+
"Runtime/Core/Ctx.cs#Level | variable | lines L3-L3 | public int Level { get; }",
121+
]),
122+
);
123+
const out = await s.describeFile(CRUX_INPUT);
124+
assert.deepEqual(out.map((r) => r.id), ["Runtime/Core/Ctx.cs", "Runtime/Core/Ctx.cs#Level"]);
125+
assert.equal(s.lastMiss, null);
126+
});
127+
128+
test("ChatCruxSummarizer strips a leading id= and repairs an echo of the current layout", async () => {
129+
const s = new ChatCruxSummarizer(
130+
cruxReply([
131+
"id=Runtime/Core/Ctx.cs",
132+
"- variable | lines L3-L3 | public int Level { get; } | id=Runtime/Core/Ctx.cs#Level",
133+
]),
134+
);
135+
const out = await s.describeFile(CRUX_INPUT);
136+
assert.deepEqual(out.map((r) => r.id), ["Runtime/Core/Ctx.cs", "Runtime/Core/Ctx.cs#Level"]);
137+
assert.equal(s.lastMiss, null);
138+
});
139+
140+
test("an invented id is not repaired and is reported as id-mismatch", async () => {
141+
const invented = "Runtime/Core/Other.cs#Level | variable | lines L3-L3";
142+
const s = new ChatCruxSummarizer(cruxReply([invented, "id=Runtime/Core/Nope.cs"]));
143+
const out = await s.describeFile(CRUX_INPUT);
144+
assert.deepEqual(out.map((r) => r.id), [invented, "id=Runtime/Core/Nope.cs"]);
145+
assert.deepEqual(s.lastMiss, {
146+
kind: "id-mismatch",
147+
finishReason: "stop",
148+
returned: 2,
149+
firstId: invented,
150+
});
151+
assert.equal(
152+
formatCruxMiss("id-mismatch", "stop"),
153+
"model returned summaries but no id matched a requested target [id-mismatch, finish_reason=stop]",
154+
);
155+
assert.equal(formatCruxMissDetail(s.lastMiss), `first of 2 returned id(s): ${JSON.stringify(invented)}`);
156+
});
157+
158+
test("resolveTargetId takes the longest requested id when several qualify", () => {
159+
const requested = ["a.ts#f", "a.ts#f | g"];
160+
assert.equal(resolveTargetId("a.ts#f | g | function | lines L1-L2", requested), "a.ts#f | g");
161+
assert.equal(resolveTargetId("a.ts#f | function | lines L1-L2", requested), "a.ts#f");
162+
assert.equal(resolveTargetId(" a.ts#f ", requested), "a.ts#f");
163+
assert.equal(resolveTargetId("a.ts#fg | function", requested), null);
164+
});
165+
71166
test("structured ops degrade gracefully when the model returns no tool call", async () => {
72167
const empty = new FakeChatModel({ toolCalls: [] });
73168
const { err } = await withCapturedError(async () => {

0 commit comments

Comments
 (0)