From 91cb5b43176ebe1ab2745082dfae1f7a5275c16e Mon Sep 17 00:00:00 2001 From: Colby McHenry Date: Thu, 6 Aug 2026 14:07:22 -0500 Subject: [PATCH] measure(explore): the factory-closure envelope premise does not hold (CG-27) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CG-27 asked whether the >50%-of-file envelope drop should cover `function` / `method`, so a `createFoo()` factory returning an object of closures stops merging every closure inside it into one cluster. Measured on a hermetic fixture, it should not, and the issue is closed as obsolete with CG-30 credited. Two mechanisms already absorb the shape. shrinkCluster orders members by (importance desc, size ASC) and refuses any member that overruns the cap once something is kept, so a file-spanning member is only selected when it is the sole member of the top importance tier — eight of nine query shapes never selected it at all. When it IS selected, CG-30 windows it on whole lines, so the file still delivers bounded, readable source (6 of 9 closure definitions in that configuration). Dropping the range instead SPLITS the file, and only the first-chosen cluster may be shrunk: a trivial 7-line cluster won the density tiebreak and the answer-bearing cluster was dropped whole — rank-#1 file 7,539 chars and 7 of 11 closures to 397 and none. Reaching the same intent more carefully (defer the envelope MEMBER inside shrinkCluster, leaving clustering untouched) is noise: 69 vs 68 closure definitions across nine query shapes. Nothing shipped. Adds the fixture, the probe, a standing gate on the outcome, and the record — including a real defect the measurement exposed on the epic tip: django's query.py leaves 8,212 of 10,135 unspent and drops a score-290 cluster to keep a score-14 one. Filed separately. No behaviour change, so no CHANGELOG entry. --- __tests__/explore-factory-closure.test.ts | 157 ++++++++++++++++++ .../src/stores/session-store.ts | 148 +++++++++++++++++ .../explore-factory-closure-cg27.md | 133 +++++++++++++++ scripts/agent-eval/probe-factory-closure.mjs | 9 +- 4 files changed, 444 insertions(+), 3 deletions(-) create mode 100644 __tests__/explore-factory-closure.test.ts create mode 100644 __tests__/fixtures/factory-closure-ts/src/stores/session-store.ts create mode 100644 docs/benchmarks/explore-factory-closure-cg27.md diff --git a/__tests__/explore-factory-closure.test.ts b/__tests__/explore-factory-closure.test.ts new file mode 100644 index 0000000..d0b0838 --- /dev/null +++ b/__tests__/explore-factory-closure.test.ts @@ -0,0 +1,157 @@ +/** + * Regression gate for the FACTORY-CLOSURE file shape (task CG-27). + * + * A `createFoo()` that returns an object of closures spans almost all of its + * file, so its indexed range is an ENVELOPE around every symbol the query + * actually wants. Svelte 5 rune stores, React custom-hook modules, IIFE + * module-pattern JS and Zustand's `create((set, get) => ({ … }))` are all + * written this way, so it is a shape rather than a one-repo quirk. + * + * CG-27 asked whether the >50%-of-file envelope drop — which fires for `class`, + * `struct`, `interface` and friends but not for `function`/`method` — should be + * extended to cover it. **Measured, it should not**, and the issue was closed as + * obsolete: `docs/benchmarks/explore-factory-closure-cg27.md` has the numbers. + * Two independent mechanisms already absorb the shape: + * + * - `shrinkCluster` orders members by (importance desc, SIZE ASC) and refuses + * any member that overruns the cap once something is kept, so a file-spanning + * member is only ever selected when it is the sole member of the top + * importance tier; + * - when it IS selected, CG-30 windows it on whole lines rather than emitting + * it whole, so the file still delivers bounded, readable source. + * + * Dropping the range instead SPLITS the file into several clusters, and only the + * first-chosen cluster may be shrunk — measured, a trivial 7-line cluster won the + * density tiebreak and the answer-bearing cluster was dropped whole, taking the + * rank-#1 file from 7,539 chars and 7 of 11 inner definitions to 397 and none. + * + * So this file pins the OUTCOME, not the mechanism: whatever future work does to + * clustering, a factory-closure file must keep delivering the closures inside it + * — that is what stops the agent Reading the file back. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; +import CodeGraph from '../src/index'; +import { ToolHandler } from '../src/mcp/tools'; +import type { ExploreDiagnosticReport } from '../src/mcp/explore-diagnostics'; + +const FIXTURE_SRC = path.join(__dirname, 'fixtures', 'factory-closure-ts'); + +/** The factory file, and the closure factory whose body is nearly all of it. */ +const TARGET = 'src/stores/dashboard-store.ts'; +const FACTORY = 'createDashboardStore'; +/** Prose the way a newcomer asks it, naming two of the closures inside. */ +const QUERY = 'how does the dashboard store refresh its metrics and apply a filter'; + +describe('CG-27 — a factory-closure file delivers the closures inside it', () => { + let testDir: string; + let cg: CodeGraph; + let response: string; + let report: ExploreDiagnosticReport; + /** Source lines of TARGET the response actually carried. */ + let delivered: Set; + let sourceLines: string[]; + + beforeAll(async () => { + testDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-cg27-')); + fs.cpSync(FIXTURE_SRC, testDir, { recursive: true }); + fs.rmSync(path.join(testDir, '.codegraph'), { recursive: true, force: true }); + + cg = CodeGraph.initSync(testDir); + await cg.indexAll(); + + const sidecar = path.join(testDir, 'explore-diag.jsonl'); + const previous = process.env.CODEGRAPH_EXPLORE_DEBUG; + process.env.CODEGRAPH_EXPLORE_DEBUG = sidecar; + try { + response = (await new ToolHandler(cg).execute('codegraph_explore', { query: QUERY })) + .content?.[0]?.text ?? ''; + } finally { + if (previous === undefined) delete process.env.CODEGRAPH_EXPLORE_DEBUG; + else process.env.CODEGRAPH_EXPLORE_DEBUG = previous; + } + const written = fs.readFileSync(sidecar, 'utf-8').trim().split('\n').filter(Boolean); + report = JSON.parse(written[written.length - 1]!) as ExploreDiagnosticReport; + + // A line counts as delivered only when the response numbers it AND the text + // matches that source line — a line number quoted in prose must not count. + sourceLines = fs.readFileSync(path.join(testDir, TARGET), 'utf-8').split('\n'); + delivered = new Set(); + for (const line of response.split('\n')) { + const m = /^(\d+)\t(.*)$/.exec(line); + if (!m) continue; + const n = Number(m[1]); + if (n >= 1 && n <= sourceLines.length && sourceLines[n - 1] === m[2]) delivered.add(n); + } + }, 120_000); + + afterAll(() => { + if (cg) cg.destroy(); + if (testDir && fs.existsSync(testDir)) fs.rmSync(testDir, { recursive: true, force: true }); + }); + + /** The closures defined inside the factory, straight from the index. */ + const innerClosures = () => { + const nodes = cg.getNodesInFile(TARGET); + const factory = nodes.find((n) => n.name === FACTORY)!; + return nodes.filter((n) => (n.kind === 'function' || n.kind === 'method') + && n.name !== FACTORY + && n.startLine > factory.startLine && n.endLine <= factory.endLine); + }; + + describe('fixture shape — if this rots, the gate below means nothing', () => { + it('holds one symbol spanning most of the file, with closures inside it', () => { + const factory = cg.getNodesInFile(TARGET).find((n) => n.name === FACTORY); + expect(factory, `${TARGET} has no ${FACTORY} node`).toBeDefined(); + // The envelope condition the >50% drop tests for — and `function`, the kind + // that drop does not cover. + expect(factory!.kind).toBe('function'); + expect(factory!.endLine - factory!.startLine + 1) + .toBeGreaterThan(sourceLines.length * 0.5); + expect(innerClosures().length).toBeGreaterThanOrEqual(8); + }); + + it('is too long to ship whole, so it renders through the cluster path', () => { + // Past WHOLE_FILE_MAX_LINES (220 for a non-central file): the whole-file + // grace and buy arms cannot claim it, so the envelope actually matters. + expect(sourceLines.length).toBeGreaterThan(220); + expect(report.files.find((f) => f.path === TARGET)?.render).toBe('clusters'); + }); + }); + + describe('the gate', () => { + it('delivers the closures the query named, not just the factory head', () => { + const inner = innerClosures(); + for (const name of ['refreshMetrics', 'applyFilter']) { + const node = inner.find((n) => n.name === name)!; + expect(node, `${name} is not an inner closure any more`).toBeDefined(); + expect(delivered.has(node.startLine), `${name} definition line not delivered`).toBe(true); + } + }); + + it('delivers most of the closures, spread across the file', () => { + const inner = innerClosures(); + const hit = inner.filter((n) => delivered.has(n.startLine)); + // Measured on the `feature/CG-24` tip: 7 of 11. The bar is half, so ordinary + // budget movement does not fail the suite, but losing the closures does. + expect(hit.length).toBeGreaterThanOrEqual(Math.ceil(inner.length / 2)); + // Not one contiguous head window off the top of the factory: the whole + // point is that selection reaches symbols deep in the body. + const last = inner[inner.length - 1]!; + const deepest = Math.max(...hit.map((n) => n.startLine)); + expect(deepest).toBeGreaterThan((last.startLine + inner[0]!.startLine) / 2); + }); + + it('never renders an empty section for the file', () => { + const rec = report.files.find((f) => f.path === TARGET)!; + expect(rec.emittedChars).toBeGreaterThan(0); + expect(delivered.size).toBeGreaterThan(20); + }); + + it('keeps the response inside the hard ceiling', () => { + expect(report.envelope.chars).toBeLessThanOrEqual(report.budget.hardCeiling); + }); + }); +}); diff --git a/__tests__/fixtures/factory-closure-ts/src/stores/session-store.ts b/__tests__/fixtures/factory-closure-ts/src/stores/session-store.ts new file mode 100644 index 0000000..031d2ab --- /dev/null +++ b/__tests__/fixtures/factory-closure-ts/src/stores/session-store.ts @@ -0,0 +1,148 @@ +import type { StoreDeps } from './types'; +import { joinPath, toQueryString } from '../lib/http'; + +/** + * The session store — a factory closure and NOTHING else at file scope. No + * companion type alias, no tail helper, no exported constants: every other + * symbol in this file lives inside the closure. That shape matters, because it + * is the one where the enclosing range is the only top-importance symbol the + * file can offer a query. + */ +export function createSessionStore(deps: StoreDeps, baseUrl: string) { + const SESSION_ENDPOINT = '/api/session'; + const REFRESH_SKEW_MS = 30_000; + + let token: string | null = null; + let expiresAt = 0; + let profile: { id: string; email: string; roles: string[] } | null = null; + let refreshing: Promise | null = null; + const auditLog: Array<{ at: number; event: string }> = []; + + function record(event: string): void { + auditLog.push({ at: deps.now(), event }); + if (auditLog.length > 200) auditLog.splice(0, auditLog.length - 200); + } + + /** Exchange credentials for a session token and cache the profile. */ + async function signIn(email: string, password: string): Promise { + const url = joinPath(baseUrl, SESSION_ENDPOINT) + toQueryString({ email }); + let payload: unknown; + try { + payload = await deps.fetchJson(url); + } catch (error) { + record(`signIn failed: ${error instanceof Error ? error.message : String(error)}`); + return false; + } + if (typeof payload !== 'object' || payload === null) { + record('signIn got a non-object payload'); + return false; + } + const body = payload as { token?: string; expiresAt?: number; profile?: typeof profile }; + if (typeof body.token !== 'string' || body.token.length === 0) { + record('signIn payload carried no token'); + return false; + } + void password; + token = body.token; + expiresAt = typeof body.expiresAt === 'number' ? body.expiresAt : deps.now() + 3_600_000; + profile = body.profile ?? null; + record(`signIn ok for ${email}`); + return true; + } + + /** Drop every trace of the session, locally and on the server. */ + async function signOut(): Promise { + if (token === null) return; + const url = joinPath(baseUrl, SESSION_ENDPOINT) + toQueryString({ action: 'revoke' }); + try { + await deps.fetchJson(url); + } catch (error) { + record(`signOut revoke failed: ${error instanceof Error ? error.message : String(error)}`); + } + token = null; + expiresAt = 0; + profile = null; + refreshing = null; + record('signOut complete'); + } + + /** + * Renew the token before it expires. Concurrent callers share one in-flight + * request so a burst of requests cannot start a refresh storm. + */ + async function refreshToken(): Promise { + if (token === null) return null; + if (refreshing !== null) return refreshing; + + refreshing = (async () => { + const url = joinPath(baseUrl, SESSION_ENDPOINT) + toQueryString({ action: 'refresh' }); + try { + const payload = await deps.fetchJson(url); + const body = payload as { token?: string; expiresAt?: number }; + if (typeof body?.token === 'string' && body.token.length > 0) { + token = body.token; + expiresAt = typeof body.expiresAt === 'number' ? body.expiresAt : deps.now() + 3_600_000; + record('refreshToken renewed the session'); + return token; + } + record('refreshToken payload carried no token'); + return null; + } catch (error) { + record(`refreshToken failed: ${error instanceof Error ? error.message : String(error)}`); + return null; + } finally { + refreshing = null; + } + })(); + + return refreshing; + } + + /** The token to send with a request, renewing it first when it is close to expiry. */ + async function authorize(): Promise { + if (token === null) return null; + if (deps.now() + REFRESH_SKEW_MS < expiresAt) return token; + return refreshToken(); + } + + /** Does the signed-in user hold every one of these roles? */ + function hasRoles(...required: string[]): boolean { + if (profile === null) return false; + const held = new Set(profile.roles); + for (const role of required) { + if (!held.has(role)) return false; + } + return true; + } + + /** Seconds left on the session, floored at zero. */ + function secondsRemaining(): number { + if (token === null) return 0; + return Math.max(0, Math.floor((expiresAt - deps.now()) / 1000)); + } + + /** The last N audit entries, newest first — what the account page renders. */ + function recentActivity(limit = 20): Array<{ at: number; event: string }> { + return auditLog.slice(-limit).reverse(); + } + + function snapshot() { + return { + signedIn: token !== null, + email: profile?.email ?? null, + roles: profile?.roles ?? [], + secondsRemaining: secondsRemaining(), + }; + } + + return { + signIn, + signOut, + refreshToken, + authorize, + hasRoles, + secondsRemaining, + recentActivity, + snapshot, + }; +} diff --git a/docs/benchmarks/explore-factory-closure-cg27.md b/docs/benchmarks/explore-factory-closure-cg27.md new file mode 100644 index 0000000..ef39aef --- /dev/null +++ b/docs/benchmarks/explore-factory-closure-cg27.md @@ -0,0 +1,133 @@ +# Deterministic measurement — the factory-closure envelope (task CG-27) + +**Date:** 2026-08-06 · **Baseline:** `feature/CG-24` @ `dc4fd75` · +**Harness:** `scripts/agent-eval/probe-factory-closure.mjs` against a hermetic fixture +(`__tests__/fixtures/factory-closure-ts/`, copied to a temp dir and indexed per run, so two runs +on one build give identical numbers). No agent A/B: the claim under test is which SYMBOLS get +selected inside one file, and the agent runs are far too noisy to see that. + +**Verdict: the premise does not survive measurement. CG-27 is closed as obsolete, CG-30 credited.** +The literal change the issue proposes is a large REGRESSION, and a more careful mechanism reaching +the same intent is noise (69 vs 68 inner definitions delivered across nine query shapes). + +--- + +## The claim + +`ENVELOPE_KINDS` in `src/mcp/tools.ts` drops a node covering >50% of its file from the cluster +ranges, so the granular symbols inside form their own clusters instead of merging into one blob. +It lists container kinds — `class`, `struct`, `interface`, `enum`, … — and **not `function` or +`method`**. A factory closure (`createFoo()` returning an object of closures) therefore survives +as a file-spanning range. That shape is common, not a one-repo quirk: Svelte 5 `.svelte.ts` rune +stores, React custom-hook modules, IIFE/module-pattern JS, and Zustand's +`create((set, get) => ({ … }))`. + +CG-30 already bounds the BYTES such a member may spend, so what remained was a ranking claim: +a file-spanning range merges every inner symbol into one cluster, so selection cannot rank and +pick the relevant closures independently. The issue required that claim be measured before any fix. + +## The fixture + +`__tests__/fixtures/factory-closure-ts/` — a dashboard app with three stores written as factory +closures, two stateless services and a UI consumer competing for one envelope. + +| file | lines | shape | +|---|---|---| +| `src/stores/dashboard-store.ts` | 385 | `createDashboardStore` spans 15–376 (**94%**), 11 closures inside; a tail type alias + helper at file scope | +| `src/stores/alerts-store.ts` | 141 | `createAlertsStore` spans 19–138 (**85%**), 9 closures inside | +| `src/stores/session-store.ts` | 148 | a factory and NOTHING else at file scope — no companion type, no tail helper | +| `src/services/metric-service.ts`, `src/services/filter-parser.ts` | 105, 62 | ordinary top-level functions — the control | + +Both factory files are past `WHOLE_FILE_MAX_LINES` where it matters, so they render through the +cluster path and the envelope actually bites. + +## Result 1 — the envelope is almost never selected in the first place + +`shrinkCluster` orders a cluster's members by **(importance desc, size ASC)** and refuses any +member that overruns the cap once something is kept. A file-spanning member is therefore only ever +selected when it is the FIRST candidate — which requires it to be the *sole* member of the top +importance tier. In eight of the nine query shapes measured, some smaller member shared that tier +(a one-line type alias, a tail helper, another closure), so the factory sorted last and was never +kept. The envelope was inert. + +## Result 2 — the proposed change is a large regression + +Making the >50% drop kind-independent, measured on the primary query +(*"how does the dashboard store refresh its metrics and apply a filter"*): + +| | baseline | drop the range | +|---|---|---| +| `dashboard-store.ts` (rank #1) delivered | 7,539 chars | **397** | +| inner closure definitions delivered | 7 of 11 | **0 of 11** | +| its own reservation left unspent | 0 | ~5,200 of 5,601 | + +The mechanism, from the cluster dump: dropping the range **splits** the file into two clusters — +`378-384` (a one-line type alias plus a four-line helper, score 15, span 7) and `4-362` (every +closure, score 116, span 359). Cluster ranking breaks the `maxImportance` tie on **density**, so +the trivial cluster wins, is taken first, and is the only one that may be shrunk. The +answer-bearing cluster then does not fit the remainder and is **dropped whole** — later clusters +are never shrunk, by design. + +The enclosing range is what was holding the file together as one cluster, inside which +`shrinkCluster` was already doing exactly the per-symbol ranking the issue asked for. + +## Result 3 — the careful version of the same intent is noise + +Deferring the envelope MEMBER inside `shrinkCluster` (leaving clustering granularity untouched, so +Result 2's split never happens) reaches the issue's intent by a better mechanism. Nine query +shapes, same fixture, same indexes — inner closure definitions delivered: + +| query | target | baseline | deferred | +|---|---|---|---| +| how does the dashboard store refresh its metrics and apply a filter | dashboard | 7/11 | **8/11** | +| createDashboardStore | dashboard | 8/11 | 8/11 | +| how is the dashboard store created and wired up | dashboard | **9/11** | 8/11 | +| createDashboardStore exportCsv summarize | dashboard | 9/11 | 9/11 | +| where is the dashboard store constructed | dashboard | 7/11 | 7/11 | +| how are widgets loaded and the layout reconciled | dashboard | 4/11 | 4/11 | +| createSessionStore (adverse: the factory IS the sole top-tier member) | alerts | 6/9 | **7/9** | +| how are alerts refreshed and acknowledged | alerts | 9/9 | 9/9 | +| createAlertsStore | alerts | 9/9 | 9/9 | +| **total** | | **68** | **69** | + +One better, one worse, seven unchanged — on a fixture built specifically to make this pattern +maximally visible. That is not a measurable selection improvement, so nothing shipped. + +## Where the envelope DOES get selected, and why CG-30 already covers it + +The adverse row above is the one configuration the ordering cannot neutralise: `createAlertsStore` +was the sole importance-10 member, so it was kept first at 3,939 chars against a 2,468 cap and +every closure was skipped. CG-30 then **windowed it on whole lines** rather than emitting it whole +or dropping the file — the response carried lines 16–108, a contiguous, readable head of the +factory carrying 6 of its 9 closure definitions. Bounded, sufficient, never empty. That is the +symptom this issue was filed against, already absorbed. + +--- + +## Byproduct — a real defect this measurement exposed (filed separately) + +Result 2's mechanism is not confined to the hypothetical change. Instrumenting the **epic tip** +across the deterministic 6-repo suite for files that drop a cluster while leaving most of their +reservation unspent: + +| file | budget | spent | unspent | kept cluster | dropped cluster | +|---|---|---|---|---|---| +| `django/db/models/sql/query.py` | 10,135 | 1,923 | **8,212 (81%)** | 1379–1400, score 14 | 306–929, **score 290** | +| `okhttp .../RealInterceptorChain.kt` | 6,058 | 1,474 | **4,584 (76%)** | 16–44, score 44 | 113–373, score 171 | +| `okhttp .../Interceptor.kt` | 4,697 | 2,027 | 2,670 (57%) | 85–138, score 21 | 154–257, score 10 | +| `gin/routergroup.go` | 5,782 | 3,273 | 2,509 (43%) | 33–91, score 116 | 103–188, score 128 | + +A file whose top cluster by density is trivial keeps that one, drops the cluster carrying 20x the +score, and leaves most of its own reservation unspent — because only the first-chosen cluster may +be shrunk. `query.py` is the file CLAUDE.md already names as the `_fetch_all` case. + +## Reproducing + +```bash +npm run build +node scripts/agent-eval/probe-factory-closure.mjs # primary query +node scripts/agent-eval/probe-factory-closure.mjs \ + --target src/stores/alerts-store.ts --factory createAlertsStore \ + --query "createSessionStore" # the adverse configuration +npx vitest run __tests__/explore-factory-closure.test.ts # the standing gate +``` diff --git a/scripts/agent-eval/probe-factory-closure.mjs b/scripts/agent-eval/probe-factory-closure.mjs index cf6301f..0b5a6e9 100644 --- a/scripts/agent-eval/probe-factory-closure.mjs +++ b/scripts/agent-eval/probe-factory-closure.mjs @@ -26,7 +26,10 @@ import { fileURLToPath, pathToFileURL } from 'node:url'; const HERE = dirname(fileURLToPath(import.meta.url)); const REPO_ROOT = resolve(HERE, '../..'); const FIXTURE = join(REPO_ROOT, '__tests__/fixtures/factory-closure-ts'); -const TARGET = 'src/stores/dashboard-store.ts'; +const targetAt = process.argv.indexOf('--target'); +const TARGET = targetAt >= 0 ? process.argv[targetAt + 1] : 'src/stores/dashboard-store.ts'; +const factoryAt = process.argv.indexOf('--factory'); +const FACTORY = factoryAt >= 0 ? process.argv[factoryAt + 1] : 'createDashboardStore'; const argv = process.argv.slice(2); const asJson = argv.includes('--json'); @@ -60,10 +63,10 @@ try { // Inner function definitions, straight from the index — the symbols the file's // enclosing factory range would otherwise swallow. const nodes = cg.getNodesInFile(TARGET); - const factory = nodes.find((n) => n.name === 'createDashboardStore'); + const factory = nodes.find((n) => n.name === FACTORY); const inner = nodes .filter((n) => (n.kind === 'function' || n.kind === 'method') - && n.name !== 'createDashboardStore' + && n.name !== FACTORY && factory && n.startLine > factory.startLine && n.endLine <= factory.endLine) .sort((a, b) => a.startLine - b.startLine); cg.close?.();