measure(explore): the factory-closure envelope premise does not hold (CG-27)
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.
This commit is contained in:
Colby McHenry
@@ -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<number>;
|
||||
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);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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<string | null> | 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<boolean> {
|
||||
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<void> {
|
||||
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<string | null> {
|
||||
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<string | null> {
|
||||
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,
|
||||
};
|
||||
}
|
||||
Reference in New Issue
Block a user