From cd1ea27ea8e8833ca612252e5b5a98523086b4b8 Mon Sep 17 00:00:00 2001 From: Colby McHenry Date: Thu, 6 Aug 2026 02:08:28 -0500 Subject: [PATCH] fix(explore): restore the CG-30 bound d652c14 reverted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit d652c14 was committed while an ab-new-vs-baseline run had the engine checked out at the BASELINE ref — that harness swaps src/ files mid-run and restores them on exit — so it captured main's tools.ts and explore-diagnostics.ts and silently undid 765c06a. Restored from 765c06a, with d652c14's doc tweak re-applied. The A/B runs started after that commit are void with it (their "changed:" line lists only explore-diagnostics.ts, i.e. both arms ran the same retrieval code) and are re-run rather than reported. Co-Authored-By: Claude Opus 5 --- src/mcp/explore-diagnostics.ts | 28 +++++- src/mcp/tools.ts | 159 +++++++++++++++++++++++++++++++-- 2 files changed, 177 insertions(+), 10 deletions(-) diff --git a/src/mcp/explore-diagnostics.ts b/src/mcp/explore-diagnostics.ts index 6dcdfef..f428f82 100644 --- a/src/mcp/explore-diagnostics.ts +++ b/src/mcp/explore-diagnostics.ts @@ -94,6 +94,15 @@ interface FileRecord extends ExploreCandidateMeta { * lifts a file above its reservation. */ allowance: number | null; + /** + * What the file could actually SPEND: its reservation plus the slack the + * files above it left on the table (bounded by MAX_SHARE). Every render bound + * reads this, not `allowance`, so it — not the reservation — is what an + * overshoot is measured against. `null` until the render loop reaches the + * file. Reporting only `allowance` makes an ordinary carry-forward look like + * a file spending over its reservation. + */ + spendable: number | null; render?: ExploreRenderMode; /** * Source chars this call did NOT re-send because an earlier call in the @@ -141,6 +150,8 @@ interface BudgetShape { export interface ExploreDiagnosticFile extends ExploreCandidateMeta { path: string; allowance: number | null; + /** Reservation + inherited slack — the bound the render paths actually use. */ + spendable: number | null; render: ExploreRenderMode | null; skipped: ExploreSkipReason | null; clipped: boolean; @@ -364,7 +375,7 @@ export class ExploreDiagnostics { /** Record one ranked candidate's scoring inputs, in final sort order. */ noteCandidate(path: string, meta: ExploreCandidateMeta): void { this.files.set(path, { - path, ...meta, allowance: null, + path, ...meta, allowance: null, spendable: null, dedupSavedChars: 0, dedupCovered: [], emittedChars: 0, finalChars: 0, share: 0, allocatedShare: 0, clipped: false, }); @@ -393,6 +404,15 @@ export class ExploreDiagnostics { } } + /** + * What the render loop will let this file spend — reservation plus inherited + * slack. Called once per file, before any of its render paths run. + */ + recordSpendable(path: string, chars: number): void { + const rec = this.files.get(path); + if (rec) rec.spendable = chars; + } + /** A candidate rendered source into the response. */ recordRender(path: string, render: ExploreRenderMode, sourceChars: number, clipped: boolean): void { const rec = this.files.get(path); @@ -540,6 +560,7 @@ export class ExploreDiagnostics { penalty: round6(r.penalty), kinds: r.kinds, allowance: r.allowance, + spendable: r.spendable, render: r.render ?? null, skipped: r.skipped ?? null, clipped: r.clipped, @@ -706,6 +727,11 @@ export function renderTable(report: ExploreDiagnosticReport): string { f.path, ); out.push(' kinds: ' + (f.kinds || '-')); + // Only when it differs: a file that spent over `reserved` but inside + // `spendable` took inherited slack, not a budget bug. + if (f.spendable !== null && f.allowance !== null && f.spendable !== f.allowance) { + out.push(` spendable: ${num(f.spendable)} (reservation + inherited slack)`); + } if (f.dedupSavedChars > 0) { const spans = f.dedupCovered.slice(0, 6).map(([a, b]) => (a === b ? `${a}` : `${a}-${b}`)).join(','); const more = f.dedupCovered.length > 6 ? `,+${f.dedupCovered.length - 6}` : ''; diff --git a/src/mcp/tools.ts b/src/mcp/tools.ts index 4a6e1a5..2655827 100644 --- a/src/mcp/tools.ts +++ b/src/mcp/tools.ts @@ -4093,6 +4093,7 @@ export class ToolHandler { Math.max(reserved, Math.round(budget.maxOutputChars * EXPLORE_ALLOCATION.MAX_SHARE)), ); reservedSoFar += reserved; + diag?.recordSpendable(filePath, allowance); const absPath = validatePathWithinRoot(projectRoot, filePath); if (!absPath || !existsSync(absPath)) { diag?.recordSkip(filePath, 'unreadable'); @@ -4731,7 +4732,9 @@ export class ToolHandler { for (const r of byImportance) { const sz = sizeOf(r) + GAP_MARKER.length; // Always keep the most important range, even if it alone is oversize — - // an empty section sends the agent to Read, which costs far more. + // an empty section sends the agent to Read, which costs far more. How + // far it may overshoot is bounded by the caller's ceiling (CG-30), which + // windows a runaway member instead of dropping it. if (keep.length > 0 && kept + sz > cap) continue; keep.push(r); kept += sz; @@ -4749,6 +4752,117 @@ export class ToolHandler { return merged.flatMap((m) => buildSection(m)); }; + /** + * Bounded overshoot for one cluster's render (CG-30). + * + * `shrinkCluster` keeps the highest-importance member whole even when that + * member alone is oversize — an empty file section sends the agent to Read, + * which is exactly what explore exists to prevent. But "never empty" is not + * "any size": with nothing bounding it, one 22K member rendered against a + * 9K reservation (2.4x), which collapses the headroom every file ranked + * below it draws from. Past the ceiling the member is WINDOWED rather than + * dropped — a leading window (signature + head of the body), plus a window + * on the spine's call site when the head misses it, since on a flow cluster + * the call path IS the answer. + */ + const MIN_WINDOW_LINES = 12; + /** Rendered cost of one source line, line numbering included. */ + const lineCost = (ln: number): number => + (fileLines[ln - 1] ?? '').length + 1 + (withLineNumbers ? String(ln).length + 1 : 0); + /** + * Longest prefix of `r` that fits `room`. `minLines` is the never-empty + * floor — it may overrun `room`, so it is only ever asked for when nothing + * else has been emitted and the alternative is an empty section. + */ + const headWindowOf = ( + r: ExploreLineRange, room: number, minLines = 0, + ): ExploreLineRange | null => { + let end = r.start - 1; + let chars = 0; + for (let ln = r.start; ln <= r.end; ln++) { + const cost = lineCost(ln); + if (chars + cost > room && end - r.start + 1 >= minLines) break; + chars += cost; + end = ln; + } + return end >= r.start ? { start: r.start, end } : null; + }; + /** Widest window around `line` inside [lo, hi] that fits `room`. */ + const centeredWindowOf = ( + line: number, lo: number, hi: number, room: number, + ): ExploreLineRange | null => { + if (line < lo || line > hi) return null; + let start = line, end = line, chars = lineCost(line); + for (let grown = true; grown;) { + grown = false; + if (end + 1 <= hi && chars + lineCost(end + 1) <= room) { end += 1; chars += lineCost(end); grown = true; } + if (start - 1 >= lo && chars + lineCost(start - 1) <= room) { start -= 1; chars += lineCost(start); grown = true; } + } + return { start, end }; + }; + /** + * Reduce rendered parts to fit `ceiling`, never to nothing. Whole parts are + * kept while they fit; the first part that overruns is cut to a leading + * window on whole lines (a body is never cut mid-line), and everything past + * it is dropped. The GAP_MARKER between surviving parts — and the line-number + * jump — is what tells the agent the cut happened. + * + * A partial window shorter than MIN_WINDOW_LINES is not worth emitting, and + * emitting one is actively harmful: the session record then claims a 4-line + * sliver, and the NEXT call's dedup has to either shred a whole block around + * it or re-send it. Below that floor the part is simply dropped — unless + * nothing has been emitted at all, where the floor wins over the ceiling + * because an empty section is the one outcome worse than an oversize one. + */ + const windowToCeiling = ( + parts: ReadonlyArray, + ceiling: number, + focusLine?: number, + ): SectionPart[] => { + const emit: ExploreLineRange[] = []; + const inParts = (line: number) => + parts.some((p) => line >= p.range.start && line <= p.range.end); + const needFocus = typeof focusLine === 'number' && focusLine > 0 && inParts(focusLine); + // Hold room back for the call site so the head window can't eat all of it. + const headRoom = needFocus ? Math.floor(ceiling * 0.6) : ceiling; + let used = 0; + for (const p of parts) { + const join = emit.length > 0 ? GAP_MARKER.length : 0; + if (used + join + p.text.length <= headRoom) { + emit.push(p.range); + used += join + p.text.length; + continue; + } + const first = emit.length === 0; + const win = headWindowOf( + p.range, Math.max(0, headRoom - used - join), first ? MIN_WINDOW_LINES : 0); + if (win && (first || win.end - win.start + 1 >= MIN_WINDOW_LINES)) { + emit.push(win); + used += join + renderSpan(win).length; + } + break; + } + const last = emit[emit.length - 1]; + if (needFocus && (!last || focusLine! > last.end)) { + const host = parts.find((p) => focusLine! >= p.range.start && focusLine! <= p.range.end)!; + const lo = Math.max(host.range.start, focusLine! - SPINE_WINDOW, last ? last.end + 1 : 0); + const hi = Math.min(host.range.end, focusLine! + SPINE_WINDOW); + const win = centeredWindowOf( + focusLine!, lo, hi, Math.max(0, ceiling - used - GAP_MARKER.length)); + // Same sliver floor as the head window — a two-line peek at the call + // site teaches the next call's dedup to shred the block around it. + if (win && win.end - win.start + 1 >= MIN_WINDOW_LINES) emit.push(win); + } + // Never empty: a section with no source sends the agent to Read. + if (emit.length === 0 && parts.length > 0) { + const first = headWindowOf(parts[0]!.range, ceiling, MIN_WINDOW_LINES); + if (first) emit.push(first); + } + return emit + .sort((a, b) => a.start - b.start) + .map((r) => ({ range: r, text: renderSpan(r) })); + }; + /** * One cluster's final parts: built, shrunk if it overruns `cap`, then * passed through the session history (CG-18). @@ -4761,15 +4875,33 @@ export class ToolHandler { const renderCluster = ( c: ExploreCluster, cap: number, + /** + * Hard bound on the rendered result (CG-30). `cap` is what selection asks + * for; this is how far a single oversize member is allowed to overshoot it + * before being windowed. Always >= `cap`, so a cluster that already fits is + * never touched. + */ + ceiling: number = Infinity, ): { parts: SectionPart[]; covered: ExploreLineRange[]; shrunk: boolean } => { const base = dedupeSpans(buildSection(c)); + const bound = ( + r: { parts: SectionPart[]; covered: ExploreLineRange[]; shrunk: boolean }, + ) => { + if (!Number.isFinite(ceiling) || sectionText(r.parts).length <= ceiling) return r; + // Windows are subsets of spans dedupeSpans already cleared, so the record + // still only ever claims source that was actually sent. + const parts = windowToCeiling(r.parts, ceiling, c.spineCallLine); + return { parts, covered: r.covered, shrunk: true }; + }; if (sectionText(base.parts).length <= cap) { return { parts: base.parts, covered: base.covered, shrunk: false }; } const shrunk = shrinkCluster(c, cap); - if (shrunk === null) return { parts: base.parts, covered: base.covered, shrunk: false }; + if (shrunk === null) { + return bound({ parts: base.parts, covered: base.covered, shrunk: false }); + } const dd = dedupeSpans(shrunk); - return { parts: dd.parts, covered: dd.covered, shrunk: true }; + return bound({ parts: dd.parts, covered: dd.covered, shrunk: true }); }; // Rank clusters for inclusion under the per-file cap. Entry-point @@ -4830,7 +4962,13 @@ export class ToolHandler { // clusters are never shrunk — they either fit or wait for another call. const first = chosenIndices.size === 0; const cap = rc.c.hasSpine ? SPINE_CEILING : fileBudget; - const section = renderCluster(rc.c, first ? cap : Infinity); + // CG-30: shrinking keeps the top member whole however big it is, so bound + // how far that member may overshoot — the same 1.5x-of-reservation bound + // SPINE_CEILING already draws, never below `cap` (a cluster that fits its + // cap is never windowed). A spine cluster's cap already IS that bound, so + // this holds it to it rather than letting the member rule walk past it. + const ceiling = Math.max(cap, SPINE_CEILING); + const section = renderCluster(rc.c, first ? cap : Infinity, first ? ceiling : Infinity); const text = sectionText(section.parts); const sectionLen = text.length + (!first && text.length > 0 ? GAP_MARKER.length : 0); if (first) { @@ -4872,10 +5010,11 @@ export class ToolHandler { // A chosen cluster is a COMPLETE method-range — we never cut through a body, // and a shrunk cluster drops WHOLE members for the same reason. An oversize - // single MEMBER (one long monolithic function) still renders in full: half a - // method is useless (the agent just Reads the rest for the other half), which - // is the very fallback explore exists to prevent. A pathological file is - // bounded by the cluster SELECTION above + the total hard ceiling. + // single MEMBER (one long monolithic function) is kept whole for as long as + // it fits the bounded overshoot (half a method is useless — the agent just + // Reads the rest, the fallback explore exists to prevent); past that bound it + // is WINDOWED on whole lines rather than dropped (CG-30), so a god-method + // can neither be silently lost nor spend the response's whole envelope. if (chosenIndices.size < clusters.length || anyClusterShrunk) { anyFileTrimmed = true; } @@ -4928,7 +5067,9 @@ export class ToolHandler { covered: mergeRanges(coveredRanges), overhead: 200, mode: 'clusters', - clipped: chosenIndices.size < clusters.length, + // Windowing an oversize member elides source too — reporting it as + // unclipped would hide exactly the cut the diagnostic exists to show. + clipped: chosenIndices.size < clusters.length || anyClusterShrunk, fullBody: sectionText(fullClusterParts), fullRanges: fullClusterParts.map((p) => p.range), });