From f7cde5d3d21901d0946b81d2a1c8f3e2e9e725ab Mon Sep 17 00:00:00 2001 From: Atkins Chang Date: Wed, 26 Aug 2026 17:52:14 +0800 Subject: [PATCH 1/4] fix(mcp): bucket explore's already-sent record per calling context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `codegraph_explore`'s cross-call dedup keyed its record per MCP connection, but one connection carries several agent contexts — harnesses route a subagent's calls over its parent's — so a fresh subagent was told source was "already sent" to a context that never received it, and it Read the file. The record is now bucketed by an optional caller-supplied `sessionId` a host hook injects. It is Map-key-only client data: trimmed, never logged, echoed, or used in a path; anything unusable collapses to the default bucket, which is byte-for-byte today's behaviour for hosts that inject nothing. Past 128 chars an id is hashed rather than cut — truncation would collapse two ids sharing a prefix into one shared record, reintroducing the bug this fixes. The LRU bound spans buckets, so a wide fan-out evicts the least recently active caller — a re-serve, the safe direction. The schema does NOT advertise the parameter. A host hook's replacement input reaches the server whether or not the property is declared, so declaring it buys the hook path nothing while exposing a knob an agent could set by hand. `CODEGRAPH_MCP_EXPLORE_SESSION_PARAM=1` declares it where a host validates arguments against the schema, or a harness passes it explicitly; the handler accepts it either way. --- CHANGELOG.md | 4 + __tests__/explore-cross-call-dedup.test.ts | 45 +++++++ __tests__/explore-session-state.test.ts | 107 ++++++++++++++++ __tests__/mcp-tool-allowlist.test.ts | 52 +++++++- src/mcp/explore-dedup.ts | 21 +++ src/mcp/explore-session-state.ts | 141 +++++++++++++++++---- src/mcp/tools.ts | 55 ++++++-- 7 files changed, 393 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ca5b9ff1..6a4725593 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Fixes + +- `codegraph_explore` no longer tells a subagent that source was "already sent" when it went to a different agent. Harnesses like Claude Code route a subagent's tool calls over the same connection to CodeGraph as the main agent, so a fresh subagent could be handed a pointer to code its own context never received — and it would go read the file instead. Already-sent tracking is now kept per calling context. + ## [1.6.0] - 2026-08-26 diff --git a/__tests__/explore-cross-call-dedup.test.ts b/__tests__/explore-cross-call-dedup.test.ts index 608d85ce8..8b775b399 100644 --- a/__tests__/explore-cross-call-dedup.test.ts +++ b/__tests__/explore-cross-call-dedup.test.ts @@ -313,6 +313,51 @@ describe('a second call against a real index', () => { expect(await explore(QUERY, b)).toBe(firstForA); }, 180_000); + /** + * Per-caller bucketing (`sessionId`). + * + * One MCP connection is not one agent context: Claude Code subagents dispatch + * over the parent's connection, so an unbucketed record hands a subagent a + * pointer to source only the parent was ever sent — the exact "codegraph + * doesn't have it" shape that costs a Read. A host hook injects a distinct id + * per context; calls that carry none share the default bucket and behave + * exactly as they did before it existed. + */ + it('serves a second context in full — a subagent never got the first call', async () => { + const session = new ExploreSessionState(); + const firstForA = await explore(QUERY, session, { sessionId: 'A' }); + const firstForB = await explore(QUERY, session, { sessionId: 'B' }); + expect(firstForB).toBe(firstForA); + expect(firstForB).not.toContain(POINTER); + }, 180_000); + + it('still dedups the second call of ONE context', async () => { + const session = new ExploreSessionState(); + await explore(QUERY, session, { sessionId: 'A' }); + expect(await explore(QUERY, session, { sessionId: 'A' })).toContain(POINTER); + }, 180_000); + + it('does not dedup an identified call against the unidentified bucket', async () => { + const session = new ExploreSessionState(); + const unidentified = await explore(QUERY, session); + const identified = await explore(QUERY, session, { sessionId: 'A' }); + expect(identified).toBe(unidentified); + expect(identified).not.toContain(POINTER); + }, 180_000); + + it('clearSessionRecord drops one context\'s history and leaves its siblings', async () => { + const session = new ExploreSessionState(); + const firstForA = await explore(QUERY, session, { sessionId: 'A' }); + await explore(QUERY, session, { sessionId: 'B' }); + + expect(session.clearSessionRecord('A')).toBe(1); + + // A was reset: its next call is a first call again, byte for byte. + expect(await explore(QUERY, session, { sessionId: 'A' })).toBe(firstForA); + // B never lost anything. + expect(await explore(QUERY, session, { sessionId: 'B' })).toContain(POINTER); + }, 240_000); + it('is off entirely under CODEGRAPH_EXPLORE_DEDUP=0', async () => { const session = new ExploreSessionState(); const previous = process.env.CODEGRAPH_EXPLORE_DEDUP; diff --git a/__tests__/explore-session-state.test.ts b/__tests__/explore-session-state.test.ts index c0b71d9a6..348be65a4 100644 --- a/__tests__/explore-session-state.test.ts +++ b/__tests__/explore-session-state.test.ts @@ -30,6 +30,7 @@ import { ExploreSessionState, coalesceRanges, exploreProjectKey, + normalizeExploreSessionId, rangesCover, readExploreSessionView, viewForProject, @@ -158,6 +159,112 @@ describe('ExploreSessionState — the container', () => { }); }); +describe('per-caller buckets', () => { + /** + * The record is per MCP connection, but a connection can carry several agent + * contexts (Claude Code subagents dispatch over the parent's). A caller id + * buckets the history so no context is ever told it already holds source that + * went to a sibling — the failure that costs a Read. + */ + it('keeps two callers on one session from seeing each other\'s calls', () => { + const state = new ExploreSessionState(); + state.record(emission('/repo/a'), 'A'); + state.record(emission('/repo/a'), 'A'); + + expect(state.view('A').projects).toHaveLength(1); + expect(state.view('A').projects[0]!.callCount).toBe(2); + expect(state.callCount('/repo/a', 'A')).toBe(2); + // B and the unidentified bucket have been served nothing. + expect(state.view('B').projects).toEqual([]); + expect(state.view().projects).toEqual([]); + expect(state.callCount('/repo/a')).toBe(0); + }); + + it('files a call with no caller id under the default bucket, as before', () => { + const state = new ExploreSessionState(); + state.record(emission('/repo/a')); + expect(state.view().projects[0]?.callCount).toBe(1); + expect(state.forProject('/repo/a')?.callCount).toBe(1); + expect(state.view('A').projects).toEqual([]); + }); + + it('treats an unusable caller id as no id rather than as its own bucket', () => { + const state = new ExploreSessionState(); + state.record(emission('/repo/a'), 42 as unknown as string); + state.record(emission('/repo/a'), ' '); + expect(state.view().projects[0]?.callCount).toBe(2); + expect(state.snapshot()).toHaveLength(1); + }); + + it('normalizes a caller id to an opaque, bounded label', () => { + expect(normalizeExploreSessionId(' main:abc ')).toBe('main:abc'); + expect(normalizeExploreSessionId('')).toBeUndefined(); + expect(normalizeExploreSessionId(' ')).toBeUndefined(); + expect(normalizeExploreSessionId(undefined)).toBeUndefined(); + expect(normalizeExploreSessionId(42)).toBeUndefined(); + expect(normalizeExploreSessionId({ id: 'A' })).toBeUndefined(); + // Past the cap the id is hashed, not cut: a fixed-size label that is still + // the same one every time. + const long = normalizeExploreSessionId('x'.repeat(200)); + expect(long).toMatch(/^[0-9a-f]{64}$/); + expect(normalizeExploreSessionId('x'.repeat(200))).toBe(long); + }); + + it('keeps two over-long ids apart when they differ only past the cap', () => { + // Truncating would collapse these into ONE bucket — a record shared between + // two contexts, which is the very bug bucketing exists to fix. + expect(normalizeExploreSessionId('x'.repeat(200))) + .not.toBe(normalizeExploreSessionId('x'.repeat(129))); + expect(normalizeExploreSessionId(`${'x'.repeat(128)}A`)) + .not.toBe(normalizeExploreSessionId(`${'x'.repeat(128)}B`)); + }); + + it('serves an over-long id from its own bucket, never its 128-char prefix', () => { + const state = new ExploreSessionState(); + state.record(emission('/repo/a'), 'x'.repeat(200)); + expect(state.view('x'.repeat(200)).projects[0]?.callCount).toBe(1); + // A caller whose id IS the truncation must not be handed the other's record. + expect(state.view('x'.repeat(128)).projects).toEqual([]); + expect(state.snapshot()).toHaveLength(1); + }); + + it('clears one caller\'s record without touching another\'s', () => { + const state = new ExploreSessionState(); + state.record(emission('/repo/a'), 'A'); + state.record(emission('/repo/b'), 'A'); + state.record(emission('/repo/a'), 'B'); + + expect(state.clearSessionRecord('A')).toBe(2); + expect(state.view('A').projects).toEqual([]); + expect(state.view('B').projects[0]?.callCount).toBe(1); + expect(state.clearSessionRecord('A')).toBe(0); + }); + + it('never lets an unusable id clear the bucket everyone else shares', () => { + const state = new ExploreSessionState(); + state.record(emission('/repo/a')); + expect(state.clearSessionRecord('')).toBe(0); + expect(state.clearSessionRecord(' ')).toBe(0); + expect(state.clearSessionRecord(undefined as unknown as string)).toBe(0); + expect(state.view().projects[0]?.callCount).toBe(1); + }); + + it('bounds (caller, project) entries together — eviction spans buckets', () => { + // Accepted cost of one shared bound: a wide fan-out evicts the least + // recently active caller's record, which re-serves (safe) rather than + // pointing anyone at source they never received. + const state = new ExploreSessionState(); + for (let i = 0; i < EXPLORE_SESSION_LIMITS.MAX_PROJECTS; i++) { + state.record(emission('/repo/a'), `caller-${i}`); + } + expect(state.snapshot()).toHaveLength(EXPLORE_SESSION_LIMITS.MAX_PROJECTS); + state.record(emission('/repo/a'), 'caller-last'); + expect(state.snapshot()).toHaveLength(EXPLORE_SESSION_LIMITS.MAX_PROJECTS); + expect(state.view('caller-0').projects).toEqual([]); + expect(state.view('caller-last').projects[0]?.callCount).toBe(1); + }); +}); + describe('range bookkeeping', () => { it('merges overlapping and adjacent spans into one', () => { const { ranges, truncated } = coalesceRanges([ diff --git a/__tests__/mcp-tool-allowlist.test.ts b/__tests__/mcp-tool-allowlist.test.ts index 8d342134e..4c1a1c051 100644 --- a/__tests__/mcp-tool-allowlist.test.ts +++ b/__tests__/mcp-tool-allowlist.test.ts @@ -4,7 +4,7 @@ * Filtering happens in ListTools (getTools) and is enforced again on execute(). */ import { describe, it, expect, afterEach } from 'vitest'; -import { ToolHandler } from '../src/mcp/tools'; +import { ToolHandler, getStaticTools } from '../src/mcp/tools'; const ENV = 'CODEGRAPH_MCP_TOOLS'; @@ -61,3 +61,53 @@ describe('CODEGRAPH_MCP_TOOLS allowlist', () => { expect(res.content[0].text).not.toMatch(/disabled via CODEGRAPH_MCP_TOOLS/); }); }); + +/** + * CODEGRAPH_MCP_EXPLORE_SESSION_PARAM — whether explore's `sessionId` is DECLARED. + * + * A host hook's `updatedInput` reaches the server whether or not the property is + * in the schema, so the declaration is off by default: it would buy the hook + * path nothing while exposing a knob an agent could set by hand. It exists for a + * host that validates arguments against the schema, or a harness passing it as + * an explicit tool argument. The HANDLER is unaffected either way — the bucketing + * tests in explore-session-state / explore-cross-call-dedup drive it directly and + * never touch this flag. + */ +describe('CODEGRAPH_MCP_EXPLORE_SESSION_PARAM', () => { + const PARAM = 'CODEGRAPH_MCP_EXPLORE_SESSION_PARAM'; + const original = process.env[PARAM]; + afterEach(() => { + if (original === undefined) delete process.env[PARAM]; + else process.env[PARAM] = original; + }); + + const exploreProps = (defs: { name: string; inputSchema: { properties: Record } }[]) => + defs.find((t) => t.name === 'codegraph_explore')!.inputSchema.properties; + + it('does not declare sessionId by default', () => { + delete process.env[PARAM]; + expect(exploreProps(new ToolHandler(null).getTools())).not.toHaveProperty('sessionId'); + expect(exploreProps(getStaticTools())).not.toHaveProperty('sessionId'); + }); + + it('declares it when the flag is set, on both served surfaces', () => { + process.env[PARAM] = '1'; + for (const props of [exploreProps(new ToolHandler(null).getTools()), exploreProps(getStaticTools())]) { + expect(props.sessionId).toEqual({ + type: 'string', + description: 'Caller-context id, injected by the host\'s hooks — not set manually.', + }); + } + // The rest of the schema is untouched. + expect(exploreProps(getStaticTools())).toHaveProperty('query'); + expect(exploreProps(getStaticTools())).toHaveProperty('maxFiles'); + }); + + it('treats the OFF values and an empty setting as unset', () => { + for (const value of ['0', 'false', 'off', 'no', 'OFF', ' ', '']) { + process.env[PARAM] = value; + expect(exploreProps(getStaticTools()), `value: ${JSON.stringify(value)}`).not.toHaveProperty('sessionId'); + } + }); + +}); diff --git a/src/mcp/explore-dedup.ts b/src/mcp/explore-dedup.ts index 541945c7d..1fd9b60d8 100644 --- a/src/mcp/explore-dedup.ts +++ b/src/mcp/explore-dedup.ts @@ -80,6 +80,27 @@ export function exploreDedupEnabled(): boolean { return !OFF.has(raw.trim().toLowerCase()); } +/** + * Whether the explore schema ADVERTISES its `sessionId` parameter. Off by + * default: a host hook's `updatedInput` reaches the server whether or not the + * property is declared, so declaring it buys the hook path nothing and only + * exposes a knob an agent could set by hand. + * `CODEGRAPH_MCP_EXPLORE_SESSION_PARAM=1` declares it for a host that validates + * arguments against the schema, or a harness that passes it as an explicit tool + * argument. `MCP_`-prefixed like `CODEGRAPH_MCP_TOOLS`, because it shapes the + * MCP surface only — unlike the `CODEGRAPH_EXPLORE_*` family, which changes + * behaviour everywhere. + * + * The HANDLER accepts `sessionId` either way — this gates the advertisement + * alone, never the bucketing. Read per call (not memoized) so a test can toggle it. + */ +export function exploreSessionParamAdvertised(): boolean { + const raw = process.env.CODEGRAPH_MCP_EXPLORE_SESSION_PARAM; + if (raw === undefined) return false; + const value = raw.trim().toLowerCase(); + return value !== '' && !OFF.has(value); +} + /** * Identity of the bytes a call served for one file. * diff --git a/src/mcp/explore-session-state.ts b/src/mcp/explore-session-state.ts index 26439c03e..90d1c0137 100644 --- a/src/mcp/explore-session-state.ts +++ b/src/mcp/explore-session-state.ts @@ -16,7 +16,12 @@ * 1. **Per session, never persisted.** One instance is owned by an * {@link ../mcp/session.MCPSession} and dies with the socket. A new agent * session starts clean — dedup across sessions would suppress source the - * new agent has never seen. + * new agent has never seen. One MCP connection is not always one agent + * CONTEXT, though: Claude Code subagents dispatch over the parent's + * connection, so the record is additionally bucketed by an optional + * caller-supplied `sessionId` (see {@link normalizeExploreSessionId}) — + * without it a subagent is told source was "already sent" to a context + * that never received it, and it Reads the file. * 2. **Per project inside the session.** A session can query several projects * by `projectPath`, so state is keyed by the RESOLVED project root * (`cg.getProjectRoot()`), not by whatever path the agent typed. @@ -39,6 +44,7 @@ */ import * as path from 'path'; +import { createHash } from 'crypto'; /** * Property on a {@link ../mcp/tools.ToolResult} carrying what an explore call @@ -155,6 +161,42 @@ export function exploreProjectKey(projectRoot: string): string { : resolved; } +/** + * Longest caller-supplied session id used verbatim as a bucket label. The cap + * exists to bound what a client can make the server hold as a key; beyond it + * the id is HASHED, never truncated. + * + * Truncating would be a correctness hole, not a size trade: two distinct ids + * sharing a 128-char prefix would collapse into ONE bucket — a record shared + * between two contexts, which is exactly the bug this bucketing exists to fix, + * and the pointers it would hand the second context name source only the first + * received. A digest keeps distinct ids distinct at a fixed size. + */ +const MAX_SESSION_ID_CHARS = 128; + +/** Bucket shared by every caller that supplies no `sessionId`. */ +const DEFAULT_BUCKET = ''; + +/** + * Normalize a caller-supplied `sessionId` into a bucket label. + * + * CLIENT-CONTROLLED DATA: this value is a Map key and nothing else. It must + * never reach a response, a log line, or a path. Anything that is not a + * non-empty string collapses to `undefined` — the default bucket, which is + * byte-for-byte the behaviour of a host that injects no id at all. + * + * Deterministic in both branches: one id always lands in one bucket, and two + * ids that differ anywhere — including only past {@link MAX_SESSION_ID_CHARS} — + * land in different ones. + */ +export function normalizeExploreSessionId(raw: unknown): string | undefined { + if (typeof raw !== 'string') return undefined; + const trimmed = raw.trim(); + if (!trimmed) return undefined; + if (trimmed.length <= MAX_SESSION_ID_CHARS) return trimmed; + return createHash('sha256').update(trimmed).digest('hex'); +} + /** * Merge overlapping / adjacent spans into the smallest equivalent set, then cap * it. Adjacency (`next.start <= cur.end + 1`) counts as overlap: two ranges that @@ -195,12 +237,24 @@ export function rangesCover(ranges: ReadonlyArray, line: numbe } interface MutableProjectState { + /** Caller bucket this project's history belongs to; `''` = no `sessionId`. */ + bucket: string; projectRoot: string; callCount: number; responseBytes: number; calls: ExploreCallRecord[]; } +/** + * Map key for one (bucket, project) pair. A resolved project root can never + * contain NUL, so the separator keeps the pair unambiguous whatever a client + * spells as its session id. Bucket membership is still tested against the + * stored {@link MutableProjectState.bucket}, never by prefix-matching this key. + */ +function bucketedKey(bucket: string, projectKey: string): string { + return `${bucket}\u0000${projectKey}`; +} + /** * One MCP session's explore history. Created per session, thrown away with it. * @@ -210,17 +264,20 @@ interface MutableProjectState { * the tool-call path and a bookkeeping bug must never fail an explore. */ export class ExploreSessionState { - /** Insertion-ordered; a touched project is re-inserted, so the head is the LRU. */ + /** + * Keyed by (caller bucket, project). Insertion-ordered; a touched entry is + * re-inserted, so the head is the LRU. + */ private readonly projects = new Map(); /** - * File an emission. Returns the record as stored (with its session call - * index), or `null` if the emission was unusable. + * File an emission under the caller's bucket. Returns the record as stored + * (with its session call index), or `null` if the emission was unusable. */ - record(emission: ExploreEmission): ExploreCallRecord | null { + record(emission: ExploreEmission, sessionId?: string): ExploreCallRecord | null { if (!emission || typeof emission.projectRoot !== 'string' || !emission.projectRoot) return null; - const key = exploreProjectKey(emission.projectRoot); - const state = this.touch(key, emission.projectRoot); + const bucket = normalizeExploreSessionId(sessionId) ?? DEFAULT_BUCKET; + const state = this.touch(bucket, exploreProjectKey(emission.projectRoot), emission.projectRoot); state.callCount += 1; state.responseBytes += Math.max(0, emission.responseBytes || 0); @@ -240,18 +297,18 @@ export class ExploreSessionState { return record; } - /** Full state for one project, or `null` if it was never queried this session. */ - forProject(projectRoot: string): ExploreProjectState | null { - const state = this.projects.get(exploreProjectKey(projectRoot)); + /** Full state for one project in one bucket, or `null` if never queried. */ + forProject(projectRoot: string, sessionId?: string): ExploreProjectState | null { + const state = this.projects.get(this.keyFor(sessionId, projectRoot)); return state ? cloneProject(state) : null; } - /** Explore calls made this session against a project (including evicted ones). */ - callCount(projectRoot: string): number { - return this.projects.get(exploreProjectKey(projectRoot))?.callCount ?? 0; + /** Explore calls one bucket made against a project (including evicted ones). */ + callCount(projectRoot: string, sessionId?: string): number { + return this.projects.get(this.keyFor(sessionId, projectRoot))?.callCount ?? 0; } - /** Every project this session has queried, least-recently-used first. */ + /** Every project this session has queried, across all buckets, LRU first. */ snapshot(): ExploreProjectState[] { return [...this.projects.values()].map(cloneProject); } @@ -261,10 +318,15 @@ export class ExploreSessionState { * {@link EXPLORE_SESSION_LIMITS.MAX_VIEW_CALLS} calls per project: it crosses a * worker boundary on every explore, so it carries what a dedup/decay decision * needs and not the whole history. + * + * Scoped to ONE bucket: a caller may only be told it already holds source its + * own context was served. Everything a different `sessionId` (or none) was + * sent is invisible here. */ - view(): ExploreSessionView { + view(sessionId?: string): ExploreSessionView { + const bucket = normalizeExploreSessionId(sessionId) ?? DEFAULT_BUCKET; return { - projects: [...this.projects.values()].map((state) => ({ + projects: [...this.projects.values()].filter((state) => state.bucket === bucket).map((state) => ({ projectRoot: state.projectRoot, callCount: state.callCount, responseBytes: state.responseBytes, @@ -275,25 +337,58 @@ export class ExploreSessionState { }; } - /** Drop everything. Used by tests; a real session just goes away instead. */ + /** Drop everything, every bucket. Used by tests; a real session just goes away. */ clear(): void { this.projects.clear(); } /** - * Fetch a project's state, creating it if new, and mark it most-recently-used. - * Evicts the LRU project past the bound — dropping a project entirely (rather - * than its detail) is right here: a session that has moved on to four other - * repos is not about to re-ask the first one. + * Forget one caller's history and nothing else — the reset a host hook fires + * when that context is discarded (a compact, a finished subagent), after + * which its next call is served as a first call again. + * + * Returns how many project entries were dropped. An id that normalizes away + * clears NOTHING: the default bucket is shared by every caller that sends no + * `sessionId`, so a malformed id must never be able to wipe it. + */ + clearSessionRecord(sessionId: string): number { + const bucket = normalizeExploreSessionId(sessionId); + if (bucket === undefined) return 0; + let removed = 0; + for (const [key, state] of [...this.projects]) { + if (state.bucket !== bucket) continue; + this.projects.delete(key); + removed += 1; + } + return removed; + } + + /** Where one bucket's copy of a project is filed. */ + private keyFor(sessionId: string | undefined, projectRoot: string): string { + return bucketedKey(normalizeExploreSessionId(sessionId) ?? DEFAULT_BUCKET, exploreProjectKey(projectRoot)); + } + + /** + * Fetch a bucket's project state, creating it if new, and mark it + * most-recently-used. Evicts the LRU project past the bound — dropping a + * project entirely (rather than its detail) is right here: a session that has + * moved on to four other repos is not about to re-ask the first one. + * + * The bound spans buckets: {@link EXPLORE_SESSION_LIMITS.MAX_PROJECTS} caps + * (bucket, project) entries, not projects per bucket, so a wide subagent + * fan-out over one repo evicts the least recently active caller's record. That + * costs a re-serve for whoever was evicted — the safe direction (see the + * module header); it can never point a caller at source it did not receive. */ - private touch(key: string, projectRoot: string): MutableProjectState { + private touch(bucket: string, projectKey: string, projectRoot: string): MutableProjectState { + const key = bucketedKey(bucket, projectKey); const existing = this.projects.get(key); if (existing) { this.projects.delete(key); this.projects.set(key, existing); return existing; } - const created: MutableProjectState = { projectRoot, callCount: 0, responseBytes: 0, calls: [] }; + const created: MutableProjectState = { bucket, projectRoot, callCount: 0, responseBytes: 0, calls: [] }; this.projects.set(key, created); while (this.projects.size > EXPLORE_SESSION_LIMITS.MAX_PROJECTS) { const lru = this.projects.keys().next().value as string | undefined; diff --git a/src/mcp/tools.ts b/src/mcp/tools.ts index 5c23f675d..b23f2f1e5 100644 --- a/src/mcp/tools.ts +++ b/src/mcp/tools.ts @@ -47,6 +47,7 @@ import { EXPLORE_EMISSION_KEY, EXPLORE_SESSION_VIEW_ARG, ExploreSessionState, + normalizeExploreSessionId, readExploreSessionView, viewForProject, type ExploreEmission, @@ -57,6 +58,7 @@ import { EXPLORE_DEDUP, dedupeRange, exploreDedupEnabled, + exploreSessionParamAdvertised, fileFingerprint, formatBackReference, mergeRanges, @@ -1279,6 +1281,32 @@ function withRequiredProjectPath(defs: ToolDefinition[]): ToolDefinition[] { }); } +/** + * Declare explore's `sessionId` parameter, when the host needs it declared to + * let it through (see {@link exploreSessionParamAdvertised}). Applied where tool + * definitions are SERVED, not at module load, so the env var can be toggled for + * one server without rebuilding the static list. + */ +function withSessionIdParam(defs: ToolDefinition[]): ToolDefinition[] { + if (!exploreSessionParamAdvertised()) return defs; + return defs.map((tool) => { + if (tool.name !== 'codegraph_explore') return tool; + return { + ...tool, + inputSchema: { + ...tool.inputSchema, + properties: { + ...tool.inputSchema.properties, + sessionId: { + type: 'string', + description: 'Caller-context id, injected by the host\'s hooks — not set manually.', + }, + }, + }, + }; + }); +} + /** * Allowlist-filtered tool definitions WITHOUT an engine — the static surface the * proxy answers `tools/list` with before any project is open. Mirrors @@ -1288,10 +1316,10 @@ function withRequiredProjectPath(defs: ToolDefinition[]): ToolDefinition[] { export function getStaticTools(): ToolDefinition[] { const raw = process.env.CODEGRAPH_MCP_TOOLS; if (!raw || !raw.trim()) { - return tools.filter(t => DEFAULT_MCP_TOOLS.has(t.name.replace(/^codegraph_/, ''))); + return withSessionIdParam(tools.filter(t => DEFAULT_MCP_TOOLS.has(t.name.replace(/^codegraph_/, '')))); } const allow = new Set(raw.split(',').map(s => s.trim().replace(/^codegraph_/, '')).filter(Boolean)); - return allow.size ? tools.filter(t => allow.has(t.name.replace(/^codegraph_/, ''))) : tools; + return withSessionIdParam(allow.size ? tools.filter(t => allow.has(t.name.replace(/^codegraph_/, ''))) : tools); } /** @@ -1484,9 +1512,9 @@ export class ToolHandler { // No explicit allowlist → the default 4-tool surface (see // DEFAULT_MCP_TOOLS for the evidence). An allowlist replaces the // default entirely, so any defined tool can be re-enabled. - let visible = allow + let visible = withSessionIdParam(allow ? tools.filter(t => allow.has(t.name.replace(/^codegraph_/, ''))) - : tools.filter(t => DEFAULT_MCP_TOOLS.has(t.name.replace(/^codegraph_/, ''))); + : tools.filter(t => DEFAULT_MCP_TOOLS.has(t.name.replace(/^codegraph_/, '')))); // No default project loaded → no-root-index case (#993): a gateway server // started outside any repo, or a monorepo root whose indexes live in // sub-projects. With nothing to fall back to, EVERY call needs an explicit @@ -2023,14 +2051,20 @@ export class ToolHandler { // object down, on the ToolResult up — because either leg may cross a // structured-clone boundary into a worker, where a closure or a handler // field could not follow. - const dispatchArgs = this.withSessionView(toolName, args, sessionState); + // One MCP connection can carry several agent contexts (Claude Code + // subagents dispatch over the parent's), so the session record is bucketed + // by the caller id a host hook injects. Derived ONCE and threaded to both + // the read and the write side: a call must record into the same bucket it + // was answered from. CLIENT-CONTROLLED — an opaque bucket label only. + const exploreSessionId = normalizeExploreSessionId(args.sessionId); + const dispatchArgs = this.withSessionView(toolName, args, sessionState, exploreSessionId); const raw = (this.queryPool && this.queryPool.healthy && this.queryPool.ready) ? await this.queryPool.run(toolName, dispatchArgs) : await this.executeReadTool(toolName, dispatchArgs); // Record + STRIP before anything else touches the result: the emission is // internal bookkeeping and must never reach the client, whether or not a // caller passed session state. - const result = this.takeExploreEmission(raw, sessionState); + const result = this.takeExploreEmission(raw, sessionState, exploreSessionId); const withWorktree = this.withWorktreeNotice(result, args.projectPath as string | undefined); return this.withStalenessNotice(withWorktree, args.projectPath as string | undefined); } catch (err) { @@ -2066,6 +2100,7 @@ export class ToolHandler { toolName: string, args: Record, sessionState: ExploreSessionState | undefined, + sessionId?: string, ): Record { if (!(EXPLORE_SESSION_VIEW_ARG in args) && (!sessionState || toolName !== 'codegraph_explore')) { return args; @@ -2073,7 +2108,10 @@ export class ToolHandler { const copy = { ...args }; delete copy[EXPLORE_SESSION_VIEW_ARG]; if (sessionState && toolName === 'codegraph_explore') { - copy[EXPLORE_SESSION_VIEW_ARG] = sessionState.view(); + // Only this caller's bucket: what a sibling context was served is not + // this one's to be told it already holds. `sessionId` itself stays on the + // args — declared or not, dispatch ignores it. + copy[EXPLORE_SESSION_VIEW_ARG] = sessionState.view(sessionId); } return copy; } @@ -2091,13 +2129,14 @@ export class ToolHandler { private takeExploreEmission( result: ToolResult, sessionState: ExploreSessionState | undefined, + sessionId?: string, ): ToolResult { const emission = result?.[EXPLORE_EMISSION_KEY]; if (emission === undefined) return result; delete result[EXPLORE_EMISSION_KEY]; if (sessionState) { try { - sessionState.record(emission); + sessionState.record(emission, sessionId); } catch { /* bookkeeping only — never fail a served call */ } } return result; From b99d06d4b0c7f2f454aa53654eb80fdc912f2b9a Mon Sep 17 00:00:00 2001 From: Atkins Chang Date: Wed, 26 Aug 2026 17:53:28 +0800 Subject: [PATCH 2/4] feat(mcp): reset one caller's already-sent record from outside the session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bucket has to be forgettable: after a compact the agent no longer holds the source the server believes it was sent, so its next explore comes back as pointers to text that is gone — the shape that costs a Read. Only the host knows when that happens, so the reset arrives from outside the MCP conversation. The daemon now answers a one-shot control line: a connection whose first line carries the `codegraph_control` marker is a command, never a session, so it is never registered as a client and never holds the idle timer. It sweeps every live session for the named bucket and replies with how much it dropped; an unknown op or a non-string id is refused rather than thrown, since a daemon that dies on a bad control line takes every connected session with it. A malformed line or a client hello still reaches the transport verbatim. A hidden `codegraph hooks` namespace carries the two entry points a host invokes: `pre-tool-use` stamps the calling context id onto a `codegraph_explore` call, `post-compact` relays the reset to the project's daemon. Both follow `prompt-hook`'s contract — always exit 0, stdout carries the hook protocol and nothing else. The context id is the host session plus the agent id when a subagent is calling. A bare agent id would assume the host numbers its agents uniquely across all sessions; scoping each to its session drops that assumption, and both hooks derive through the same function so a reset always names the bucket the stamp filled. `pre-tool-use --agent codex` additionally pairs the rewrite with `permissionDecision`, which codex requires or it silently discards the rewrite, and which is inert for its approvals. No other agent gets the field: Claude Code acts on it, so "allow" would override a user who chose not to allowlist codegraph. The agent is named by the wiring that installed the hook, never inferred from the payload, and an unrecognized name omits the field. --- CHANGELOG.md | 4 + __tests__/explore-session-reset.test.ts | 391 ++++++++++++++++++++++++ src/bin/codegraph.ts | 225 ++++++++++++++ src/mcp/daemon.ts | 94 +++++- src/mcp/session.ts | 9 + 5 files changed, 714 insertions(+), 9 deletions(-) create mode 100644 __tests__/explore-session-reset.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a4725593..82840c4e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### New Features + +- Two new hook entry points let a host agent tell CodeGraph which context is calling: `codegraph hooks pre-tool-use` tags each `codegraph_explore` call with the agent that made it, and `codegraph hooks post-compact` clears that context's already-sent record after a compaction. Wire them into your agent's hook settings and already-sent tracking follows each subagent and each compacted session instead of the connection they share. Nothing changes for hosts that don't run them. + ### Fixes - `codegraph_explore` no longer tells a subagent that source was "already sent" when it went to a different agent. Harnesses like Claude Code route a subagent's tool calls over the same connection to CodeGraph as the main agent, so a fresh subagent could be handed a pointer to code its own context never received — and it would go read the file instead. Already-sent tracking is now kept per calling context. diff --git a/__tests__/explore-session-reset.test.ts b/__tests__/explore-session-reset.test.ts new file mode 100644 index 000000000..303c3ce1f --- /dev/null +++ b/__tests__/explore-session-reset.test.ts @@ -0,0 +1,391 @@ +/** + * Resetting one caller's already-sent record. + * + * Bucketing the record per caller context (see explore-cross-call-dedup) fixes + * the subagent misfire, but a bucket still has to be forgettable: after a + * compact the agent's context no longer holds the source the server believes it + * was sent, so the next call gets pointers to text that is gone — the "codegraph + * doesn't have it" shape that costs a Read. The host knows when that happens and + * nothing else does, so the reset arrives from OUTSIDE the MCP conversation: + * + * 1. the daemon's control line — one JSON line on a fresh connection, one + * reply, no MCP session, and never a crash whatever the line says; + * 2. the `codegraph hooks` CLI entry points the host actually invokes, which + * must ALWAYS exit 0 and keep stdout clean, since stdout IS the hook + * protocol and a non-zero exit disrupts the agent. + */ +import { afterEach, describe, expect, it } from 'vitest'; +import { spawn } from 'child_process'; +import * as fs from 'fs'; +import * as net from 'net'; +import * as os from 'os'; +import * as path from 'path'; +import { Daemon, parseDaemonControlLine } from '../src/mcp/daemon'; +import { MCPSession } from '../src/mcp/session'; +import { getDaemonSocketCandidates } from '../src/mcp/daemon-paths'; +import type { MCPEngine } from '../src/mcp/engine'; +import type { JsonRpcTransport } from '../src/mcp/transport'; +import type { ExploreEmission } from '../src/mcp/explore-session-state'; + +const BIN = path.resolve(__dirname, '../dist/bin/codegraph.js'); + +const servers: net.Server[] = []; +const tmpDirs: string[] = []; +afterEach(async () => { + while (servers.length) { + const server = servers.pop()!; + await new Promise((resolve) => server.close(() => resolve())); + } + while (tmpDirs.length) { + try { fs.rmSync(tmpDirs.pop()!, { recursive: true, force: true }); } catch { /* best-effort */ } + } +}); + +function tmpProject(prefix: string): string { + const dir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), prefix))); + tmpDirs.push(dir); + // `.codegraph/codegraph.db` is what makes a directory look indexed, which is + // what the hook needs before it goes looking for a daemon. + fs.mkdirSync(path.join(dir, '.codegraph'), { recursive: true }); + fs.writeFileSync(path.join(dir, '.codegraph', 'codegraph.db'), ''); + return dir; +} + +function emission(projectRoot: string): ExploreEmission { + return { + projectRoot, + query: 'q', + files: [{ path: 'a.ts', ranges: [{ start: 1, end: 10 }], bytes: 100 }], + sourceBytes: 100, + responseBytes: 400, + }; +} + +/** A transport that is never driven — these tests reach the session directly. */ +const idleTransport = (): JsonRpcTransport => ({ + start() { /* no messages in this test */ }, + stop() { /* nothing to tear down */ }, + send() { /* unused */ }, + notify() { /* unused */ }, + async request() { return {}; }, + sendResult() { /* unused */ }, + sendError() { /* unused */ }, +}); + +/** Send one line to a socket and resolve the first reply line carrying `ok`. */ +function control(socketPath: string, line: string): Promise | null> { + return new Promise((resolve) => { + const socket = net.createConnection(socketPath); + let buffered = ''; + const done = (value: Record | null): void => { + try { socket.destroy(); } catch { /* already gone */ } + resolve(value); + }; + socket.setEncoding('utf8'); + socket.on('connect', () => socket.write(`${line}\n`)); + socket.on('data', (chunk: string) => { + buffered += chunk; + for (const reply of buffered.split('\n')) { + try { + const parsed = JSON.parse(reply); + if (parsed && typeof parsed === 'object' && 'ok' in parsed) { done(parsed); return; } + } catch { /* the daemon hello, or a partial line */ } + } + }); + socket.on('error', () => done(null)); + socket.on('close', () => done(null)); + }); +} + +describe('parseDaemonControlLine', () => { + it('parses a control line, passing its fields through unvalidated', () => { + expect(parseDaemonControlLine('{"codegraph_control":1,"op":"clear-session-record","sessionId":"A"}')) + .toEqual({ op: 'clear-session-record', sessionId: 'A' }); + expect(parseDaemonControlLine('{"codegraph_control":1,"op":42}')) + .toEqual({ op: 42, sessionId: undefined }); + }); + + it('leaves a JSON-RPC first line alone — a client must not be hijacked', () => { + expect(parseDaemonControlLine('{"jsonrpc":"2.0","id":1,"method":"initialize"}')).toBeNull(); + expect(parseDaemonControlLine('{"codegraph_client":1,"pid":7}')).toBeNull(); + }); + + it('requires the exact marker', () => { + expect(parseDaemonControlLine('{"codegraph_control":true,"op":"x"}')).toBeNull(); + expect(parseDaemonControlLine('{"codegraph_control":2,"op":"x"}')).toBeNull(); + }); + + it('returns null for invalid / empty / non-object JSON', () => { + expect(parseDaemonControlLine('not json')).toBeNull(); + expect(parseDaemonControlLine('')).toBeNull(); + expect(parseDaemonControlLine('42')).toBeNull(); + expect(parseDaemonControlLine('null')).toBeNull(); + }); +}); + +describe('the daemon control line', () => { + /** + * A daemon serving two real sessions, reachable over a real socket. Only the + * engine is absent — the control path never touches it (that is half the + * point: a reset must not depend on an initialized index). + */ + async function serveDaemon(): Promise<{ + socketPath: string; + sessions: MCPSession[]; + clients: Set; + }> { + const root = tmpProject('cg-control-'); + // idleTimeoutMs 0 = never arm a real idle timer in a unit test. + const daemon = new Daemon(root, { idleTimeoutMs: 0 }) as unknown as { + handleConnection: (socket: net.Socket) => void; + clients: Set; + engine: { stop: () => void }; + }; + const engine = {} as MCPEngine; + const sessions = [new MCPSession(idleTransport(), engine), new MCPSession(idleTransport(), engine)]; + for (const session of sessions) daemon.clients.add(session); + // The engine the constructor built is never initialized here; stop it so no + // worker pool outlives the test. + try { daemon.engine.stop(); } catch { /* nothing started */ } + + const socketPath = getDaemonSocketCandidates(root)[0]!; + const server = net.createServer((socket) => daemon.handleConnection(socket)); + servers.push(server); + await new Promise((resolve) => server.listen(socketPath, () => resolve())); + return { socketPath, sessions, clients: daemon.clients }; + } + + it('clears the named caller everywhere and reports how much it dropped', async () => { + const { socketPath, sessions, clients } = await serveDaemon(); + // The caller knows its context id, not which connection carries it: bucket + // "A" exists on both sessions, "B" only on the second. + sessions[0]!.getExploreSessionState().record(emission('/repo/one'), 'A'); + sessions[1]!.getExploreSessionState().record(emission('/repo/two'), 'A'); + sessions[1]!.getExploreSessionState().record(emission('/repo/two'), 'B'); + + expect(await control(socketPath, '{"codegraph_control":1,"op":"clear-session-record","sessionId":"A"}')) + .toEqual({ ok: true, cleared: 2 }); + + expect(sessions[0]!.getExploreSessionState().view('A').projects).toEqual([]); + expect(sessions[1]!.getExploreSessionState().view('A').projects).toEqual([]); + // B was never named, so B still holds everything it was served. + expect(sessions[1]!.getExploreSessionState().view('B').projects[0]?.callCount).toBe(1); + // A control connection is a command, not a client — it must not register as + // one (that would make it count against the daemon's idle lifecycle). + expect(clients.size).toBe(2); + }); + + it('refuses an unknown op and a non-string id, and keeps serving after both', async () => { + const { socketPath, sessions } = await serveDaemon(); + sessions[0]!.getExploreSessionState().record(emission('/repo/one'), 'A'); + + expect(await control(socketPath, '{"codegraph_control":1,"op":"drop-everything","sessionId":"A"}')) + .toEqual({ ok: false }); + expect(await control(socketPath, '{"codegraph_control":1,"op":"clear-session-record","sessionId":{"nope":1}}')) + .toEqual({ ok: false }); + // Nothing was cleared by either, and the daemon still answers a real one. + expect(sessions[0]!.getExploreSessionState().view('A').projects[0]?.callCount).toBe(1); + expect(await control(socketPath, '{"codegraph_control":1,"op":"clear-session-record","sessionId":"A"}')) + .toEqual({ ok: true, cleared: 1 }); + }); + + it('reports zero for a caller nobody has a record for', async () => { + const { socketPath } = await serveDaemon(); + expect(await control(socketPath, '{"codegraph_control":1,"op":"clear-session-record","sessionId":"ghost"}')) + .toEqual({ ok: true, cleared: 0 }); + }); +}); + +/** + * The CLI hook entry points. Run against the BUILT binary, because the contract + * under test is a process contract — exit status and stdout bytes — not a + * function's return value. + */ +describe('codegraph hooks', () => { + /** + * Spawned ASYNC on purpose: the fake daemon below serves from this very event + * loop, and a synchronous spawn would block it — the hook would then time out + * against a server that never got a turn to answer. + */ + function runHook(args: string[], stdin: string, cwd?: string): Promise<{ status: number | null; stdout: string }> { + return new Promise((resolve) => { + const child = spawn(process.execPath, [BIN, 'hooks', ...args], { + cwd, + // Skip the daemon spawn and the wasm re-exec: a hook must resolve in one + // fast process, and the test asserts what that process alone printed. + env: { ...process.env, CODEGRAPH_NO_DAEMON: '1', CODEGRAPH_WASM_RELAUNCHED: '1' }, + }); + let stdout = ''; + child.stdout.setEncoding('utf-8'); + child.stdout.on('data', (chunk: string) => { stdout += chunk; }); + child.on('close', (status) => resolve({ status, stdout })); + child.stdin.on('error', () => { /* a hook may never read stdin */ }); + child.stdin.end(stdin); + }); + } + + describe('pre-tool-use', () => { + const payload = (over: Record): string => JSON.stringify({ + session_id: 'sess-1', + hook_event_name: 'PreToolUse', + tool_name: 'mcp__codegraph__codegraph_explore', + tool_input: { query: 'AuthService login', maxFiles: 8 }, + ...over, + }); + + it('stamps the main agent with the bare session id, keeping the original input', async () => { + const { status, stdout } = await runHook(['pre-tool-use'], payload({})); + expect(status).toBe(0); + expect(JSON.parse(stdout)).toEqual({ + hookSpecificOutput: { + hookEventName: 'PreToolUse', + updatedInput: { query: 'AuthService login', maxFiles: 8, sessionId: 'sess-1' }, + }, + }); + }); + + it('scopes a subagent to its session — the whole point of the hook', async () => { + const { status, stdout } = await runHook(['pre-tool-use'], payload({ agent_id: 'agent-42' })); + expect(status).toBe(0); + expect(JSON.parse(stdout)).toEqual({ + hookSpecificOutput: { + hookEventName: 'PreToolUse', + // Composite, not the bare agent id: a host that numbers agents only + // per-session would otherwise cross-bucket two sessions' subagents. + updatedInput: { query: 'AuthService login', maxFiles: 8, sessionId: 'sess-1:agent-42' }, + }, + }); + }); + + /** + * `permissionDecision` is opt-in per agent, declared by the wiring rather + * than sniffed from the payload. Codex requires it paired with the rewrite + * or discards the rewrite; Claude Code ACTS on it, so "allow" there would + * override a user who chose not to allowlist codegraph. The default has to + * be omission, and an unrecognized agent has to fall back to the default — + * a wrong guess in this direction is a privilege escalation. + */ + it('pairs permissionDecision with the rewrite for --agent codex', async () => { + const { status, stdout } = await runHook(['pre-tool-use', '--agent', 'codex'], payload({})); + expect(status).toBe(0); + expect(JSON.parse(stdout)).toEqual({ + hookSpecificOutput: { + hookEventName: 'PreToolUse', + permissionDecision: 'allow', + updatedInput: { query: 'AuthService login', maxFiles: 8, sessionId: 'sess-1' }, + }, + }); + }); + + it('omits permissionDecision by default and for any other agent', async () => { + for (const args of [ + ['pre-tool-use'], + ['pre-tool-use', '--agent', 'claude'], + ['pre-tool-use', '--agent', 'not-an-agent'], + ['pre-tool-use', '--agent', 'Codex'], + ]) { + const { status, stdout } = await runHook(args, payload({})); + expect(status, args.join(' ')).toBe(0); + expect(JSON.parse(stdout).hookSpecificOutput, args.join(' ')) + .not.toHaveProperty('permissionDecision'); + // Only that one field differs — the stamp is identical either way. + expect(JSON.parse(stdout).hookSpecificOutput.updatedInput, args.join(' ')) + .toEqual({ query: 'AuthService login', maxFiles: 8, sessionId: 'sess-1' }); + } + }); + + it('keeps the main context and its own subagent in different buckets', async () => { + const idFor = async (over: Record): Promise => + JSON.parse((await runHook(['pre-tool-use'], payload(over))).stdout) + .hookSpecificOutput.updatedInput.sessionId; + expect(await idFor({})).not.toBe(await idFor({ agent_id: 'agent-42' })); + // Same agent id under a different session is a different bucket too. + expect(await idFor({ agent_id: 'agent-42' })) + .not.toBe(await idFor({ session_id: 'sess-2', agent_id: 'agent-42' })); + }); + + it('says nothing about a tool that is not explore', async () => { + const { status, stdout } = await runHook(['pre-tool-use'], payload({ tool_name: 'Read' })); + expect(status).toBe(0); + expect(stdout).toBe(''); + }); + + it('exits 0 with empty stdout on junk, empty, or id-less input', async () => { + for (const input of ['not json', '', '{}', payload({ session_id: '', agent_id: '' })]) { + const { status, stdout } = await runHook(['pre-tool-use'], input); + expect(status, `input: ${input}`).toBe(0); + expect(stdout, `input: ${input}`).toBe(''); + } + }); + }); + + describe('post-compact', () => { + /** Stand in for the daemon: greet, capture one line, answer it. */ + async function fakeDaemon(root: string): Promise<{ received: string[] }> { + const received: string[] = []; + const server = net.createServer((socket) => { + socket.write('{"codegraph":"0.0.0","pid":1,"socketPath":"x","protocol":1}\n'); + let buffered = ''; + socket.setEncoding('utf8'); + socket.on('data', (chunk: string) => { + buffered += chunk; + const nl = buffered.indexOf('\n'); + if (nl === -1) return; + received.push(buffered.slice(0, nl)); + buffered = ''; + socket.end('{"ok":true,"cleared":1}\n'); + }); + socket.on('error', () => { /* client may vanish */ }); + }); + servers.push(server); + await new Promise((resolve) => server.listen(getDaemonSocketCandidates(root)[0]!, () => resolve())); + return { received }; + } + + it('relays the compacted context id to the project daemon', async () => { + const root = tmpProject('cg-postcompact-'); + const daemon = await fakeDaemon(root); + const { status } = await runHook(['post-compact'], JSON.stringify({ + session_id: 'sess-9', cwd: root, hook_event_name: 'PostCompact', trigger: 'auto', + })); + expect(status).toBe(0); + expect(JSON.parse(daemon.received[0]!)).toEqual({ + codegraph_control: 1, op: 'clear-session-record', sessionId: 'sess-9', + }); + }); + + it('relays a subagent id, and honours an explicit --session-id with no payload', async () => { + const root = tmpProject('cg-postcompact-agent-'); + const daemon = await fakeDaemon(root); + + expect((await runHook(['post-compact'], JSON.stringify({ + session_id: 'sess-9', agent_id: 'agent-42', cwd: root, + }))).status).toBe(0); + // The reset must spell the id exactly as the stamping hook did. + expect(JSON.parse(daemon.received[0]!).sessionId).toBe('sess-9:agent-42'); + + // No hook payload at all — the relay path (LLMMesh), which knows the id + // but has nothing on stdin to derive it from. + expect((await runHook(['post-compact', '--path', root, '--session-id', 'relayed-7'], '')).status).toBe(0); + expect(JSON.parse(daemon.received[1]!).sessionId).toBe('relayed-7'); + }); + + it('exits 0 when no daemon is listening, on an unindexed cwd, and on junk input', async () => { + const indexed = tmpProject('cg-postcompact-nodaemon-'); + const bare = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'cg-postcompact-bare-'))); + tmpDirs.push(bare); + const payload = (cwd: string): string => JSON.stringify({ session_id: 'sess-9', cwd }); + + // Indexed but nothing is serving; nothing indexed at all; unparseable. + for (const [label, input] of [ + ['no daemon', payload(indexed)], + ['unindexed', payload(bare)], + ['junk', 'not json'], + ] as const) { + const { status, stdout } = await runHook(['post-compact'], input, bare); + expect(status, label).toBe(0); + expect(stdout, label).toBe(''); + } + }); + }); +}); diff --git a/src/bin/codegraph.ts b/src/bin/codegraph.ts index 19038df1b..7dd8eb445 100644 --- a/src/bin/codegraph.ts +++ b/src/bin/codegraph.ts @@ -40,6 +40,7 @@ try { import { Command } from 'commander'; import * as path from 'path'; import * as fs from 'fs'; +import * as net from 'net'; import { getCodeGraphDir, isInitialized, unsafeIndexRootReason, findNearestCodeGraphRoot, planFrontload, hasStructuralKeyword, extractCodeTokens } from '../directory'; import { extractProseCandidates } from '../search/identifier-segments'; import { detectWorktreeIndexMismatch, worktreeMismatchWarning } from '../sync/worktree'; @@ -1473,6 +1474,230 @@ program } }); +/** + * codegraph hooks … (hidden) + * + * Host-agent hook entry points. A host (Claude Code today) invokes these with + * its hook payload on stdin; their whole job is to tell the server WHICH agent + * context a call belongs to, because one MCP connection carries several — a + * subagent dispatches over its parent's — and cross-call dedup must not tell a + * fresh context that source was "already sent" to a sibling. + * + * LOAD-BEARING, same contract as `prompt-hook`: a non-zero exit or a stray byte + * on stdout disrupts the host. Every failure path here exits 0 and prints + * nothing; stdout carries the hook protocol and nothing else, notes go to + * stderr. + */ + +/** The hook payload on stdin, or `{}` when there isn't one. Never rejects. */ +async function readHookPayload(): Promise> { + if (process.stdin.isTTY) return {}; // invoked by hand — nothing is coming + const raw = await new Promise((resolve) => { + let data = ''; + process.stdin.setEncoding('utf8'); + process.stdin.on('data', (c) => { data += c; }); + process.stdin.on('end', () => resolve(data)); + process.stdin.on('error', () => resolve(data)); + }); + try { + const parsed: unknown = JSON.parse(raw); + return parsed && typeof parsed === 'object' ? parsed as Record : {}; + } catch { + return {}; + } +} + +/** + * The caller-context id for a hook payload: the host session, plus the agent id + * when a subagent is calling. + * + * Composite rather than the agent id alone, because a bare `agent_id` assumes + * the host numbers its agents uniquely across ALL sessions — a host whose ids + * are only unique within a session would put two sessions' subagents in one + * bucket on a shared connection, which is the bug this whole mechanism exists + * to prevent. Scoping every agent id to its session removes the assumption, and + * a main context is never confused with a subagent of the same session: one is + * the session id alone, the other always carries the separator and a suffix. + * + * Both hooks derive through here — the injecting hook and the resetting hook + * MUST agree on the spelling or a reset clears a bucket nobody is filling. + */ +function hookSessionId(payload: Record): string | null { + const agentId = typeof payload.agent_id === 'string' ? payload.agent_id.trim() : ''; + const sessionId = typeof payload.session_id === 'string' ? payload.session_id.trim() : ''; + if (sessionId && agentId) return `${sessionId}:${agentId}`; + // A payload carrying only one of the two still identifies a context; only a + // payload with neither is unusable. + return sessionId || agentId || null; +} + +/** Cap on what a control reply may stream before we stop reading it. */ +const MAX_CONTROL_REPLY_BYTES = 64 * 1024; + +/** + * One-shot control round-trip against a daemon socket. The daemon greets every + * connection with its own hello, so the reply is the first line carrying an + * `ok` field. Resolves `null` on ANY failure — no listener, wrong socket, + * timeout, malformed reply — because the only callers are hooks that must + * degrade silently. + */ +function sendDaemonControl( + socketPath: string, + command: Record, + timeoutMs = 2_000, +): Promise<{ ok?: boolean; cleared?: number } | null> { + return new Promise((resolve) => { + let settled = false; + const done = (value: { ok?: boolean; cleared?: number } | null): void => { + if (settled) return; + settled = true; + clearTimeout(timer); + try { socket.destroy(); } catch { /* already gone */ } + resolve(value); + }; + const socket = net.createConnection(socketPath); + const timer = setTimeout(() => done(null), timeoutMs); + timer.unref?.(); + let buffered = ''; + socket.setEncoding('utf8'); + socket.on('connect', () => { socket.write(`${JSON.stringify(command)}\n`); }); + socket.on('data', (chunk: string) => { + buffered += chunk; + if (buffered.length > MAX_CONTROL_REPLY_BYTES) { done(null); return; } + let nl: number; + while ((nl = buffered.indexOf('\n')) !== -1) { + const line = buffered.slice(0, nl); + buffered = buffered.slice(nl + 1); + try { + const parsed: unknown = JSON.parse(line); + if (parsed && typeof parsed === 'object' && 'ok' in parsed) { + done(parsed as { ok?: boolean; cleared?: number }); + return; + } + } catch { /* the daemon hello, or noise before the reply */ } + } + }); + socket.on('error', () => done(null)); + socket.on('close', () => done(null)); + }); +} + +const hooks = program + .command('hooks', { hidden: true }) + .description('Host-agent hook entry points (payload on stdin)'); + +/** + * codegraph hooks post-compact (hidden) + * + * PostCompact hook: the context this caller had is gone, so the server's record + * of what it was already sent has to go with it — otherwise the next call is + * answered with pointers to source the compacted context no longer holds, and + * the agent Reads the file. + * + * Both Claude Code and codex wire this to their PostCompact event, which fires + * only once the compacted history has been committed. That is the one moment a + * reset is correct: a compaction that failed returns before rewriting anything, + * leaving the agent still holding the source the record describes, and a reset + * there would throw away a ledger that is still valid. Only `session_id` / + * `agent_id` and the payload cwd are read, so either host's payload shape works + * unchanged; `hook_event_name` and the rest are ignored. + */ +hooks + .command('post-compact') + .description('Drop this caller context\'s already-sent record after a compact') + .option('--path ', 'Project whose daemon to signal (default: the payload cwd)') + .option('--session-id ', 'Caller context id, when there is no hook payload to derive it from') + .action(async (options: { path?: string; sessionId?: string }) => { + try { + // An explicit --session-id means there is no hook payload to wait for — + // a relay caller's stdin may be a pipe that never closes. + const payload = options.sessionId?.trim() ? {} : await readHookPayload(); + const sessionId = options.sessionId?.trim() || hookSessionId(payload); + if (!sessionId) return; + + const cwd = options.path || (typeof payload.cwd === 'string' ? payload.cwd : '') || process.cwd(); + const found = findNearestCodeGraphRoot(cwd); + if (!found) return; // nothing indexed here — no daemon can be holding a record + // Realpath'd to match how the daemon keys its socket and lockfile: a + // symlinked cwd would otherwise resolve to a socket nobody is serving. + let root = found; + try { root = fs.realpathSync(found); } catch { /* keep the un-resolved path */ } + + const { getDaemonPidPath, getDaemonSocketCandidates, decodeLockInfo } = await import('../mcp/daemon-paths'); + // The lockfile is authoritative — a daemon that relocated past an + // unusable in-project filesystem is bound somewhere the candidate list + // only guesses at — but it may be stale or missing, so the candidates + // still follow it. + const fromLock = (() => { + try { return decodeLockInfo(fs.readFileSync(getDaemonPidPath(root), 'utf8'))?.socketPath ?? null; } + catch { return null; } + })(); + const candidates = [...new Set([fromLock, ...getDaemonSocketCandidates(root)].filter((c): c is string => !!c))]; + + for (const candidate of candidates) { + const reply = await sendDaemonControl(candidate, { + codegraph_control: 1, + op: 'clear-session-record', + sessionId, + }); + if (reply?.ok) return; + } + // No daemon answered. Nothing to reset — a fresh one starts with no record. + } catch { + // Degradable by contract: never surface an error to the host's hook pipeline. + } + }); + +/** + * codegraph hooks pre-tool-use (hidden) + * + * PreToolUse hook: stamps the calling context's id onto a codegraph_explore + * call so the server buckets its already-sent record per context instead of per + * connection. Only explore is touched; every other tool passes through + * untouched (no stdout at all, which the host reads as "no change"). + */ +hooks + .command('pre-tool-use') + .description('Stamp the calling context id onto a codegraph_explore call') + .option('--agent ', 'Agent whose hook protocol to emit for (an installer target name, e.g. codex)') + .action(async (options: { agent?: string }) => { + try { + const payload = await readHookPayload(); + const toolName = typeof payload.tool_name === 'string' ? payload.tool_name : ''; + if (!toolName.includes('codegraph_explore')) return; + const sessionId = hookSessionId(payload); + if (!sessionId) return; + const toolInput = payload.tool_input && typeof payload.tool_input === 'object' + ? payload.tool_input as Record + : {}; + // `permissionDecision` is emitted only for an agent that REQUIRES it. + // Codex treats it and `updatedInput` as mutually required and silently + // discards an unpaired rewrite, and its decision is inert for approvals + // — it gates the rewrite inside the hooks crate and never reaches core + // dispatch. Claude Code needs no pairing and ACTS on the field: "allow" + // auto-approves, overriding a user who deliberately did not allowlist + // codegraph (and explore's `projectPath` reaches any indexed project), + // while "ask" would force a prompt on every call even for users who did. + // Emitting nothing leaves Claude's own permission flow in charge, which + // is the correct output there. + // + // The agent is declared by the wiring that installed the hook, never + // sniffed from the payload: a guess that lands on "allow" is a privilege + // escalation, so any other --agent omits the field. + const pairsPermissionDecision = options.agent === 'codex'; + process.stdout.write(`${JSON.stringify({ + hookSpecificOutput: { + hookEventName: 'PreToolUse', + ...(pairsPermissionDecision ? { permissionDecision: 'allow' } : {}), + updatedInput: { ...toolInput, sessionId }, + }, + })}\n`); + } catch { + // Degradable by contract: an un-stamped call still works, it just dedups + // against the connection-wide bucket. + } + }); + /** * codegraph node [name] * diff --git a/src/mcp/daemon.ts b/src/mcp/daemon.ts index 500c48a8c..250be427c 100644 --- a/src/mcp/daemon.ts +++ b/src/mcp/daemon.ts @@ -367,6 +367,13 @@ export class Daemon { // timeout, a non-hello first line, an early close — yields null pids and we // fall back to the socket-close lifecycle exactly as before (#692). void readClientHello(socket).then((peers) => { + // A control command claims the connection: one line in, one line out, no + // session. It is deliberately never added to `clients`, so a control + // caller neither counts as a client nor holds the idle timer off. + if (peers.control) { + this.handleControl(socket, peers.control); + return; + } const transport = new SocketTransport(socket); const session = new MCPSession(transport, this.engine, { explicitProjectPath: this.projectRoot, @@ -383,6 +390,31 @@ export class Daemon { }); } + /** + * Answer a one-shot control connection. + * + * Total by design: an unknown op or a malformed field replies `{"ok":false}` + * instead of throwing, because a daemon that dies on a bad control line takes + * every connected session down with it. + * + * `clear-session-record` sweeps EVERY live session: the caller knows its own + * context id, never which connection carries it (subagents dispatch over the + * parent's). The id is an opaque bucket label — never logged, never echoed. + */ + private handleControl(socket: net.Socket, command: DaemonControlCommand): void { + let reply: { ok: boolean; cleared?: number } = { ok: false }; + if (command.op === 'clear-session-record' && typeof command.sessionId === 'string') { + let cleared = 0; + for (const session of [...this.clients]) { + try { + cleared += session.clearSessionRecord(command.sessionId); + } catch { /* one wedged session must not sink the sweep */ } + } + reply = { ok: true, cleared }; + } + try { socket.end(JSON.stringify(reply) + '\n'); } catch { /* peer already gone */ } + } + private dropClient(session: MCPSession): void { if (!this.clients.delete(session)) return; this.clientPeers.delete(session); @@ -772,6 +804,35 @@ export function parseClientHelloLine( return { pid: o.pid, hostPid: typeof o.hostPid === 'number' ? o.hostPid : null }; } +/** + * A one-shot control command, as it arrived on the wire. Fields stay `unknown` + * because they are unvalidated client data — {@link Daemon.handleControl} is + * the single place that decides whether a command is answerable. + */ +export interface DaemonControlCommand { + op: unknown; + sessionId: unknown; +} + +/** + * Read the first line of a connection as a control command, or `null` when it + * is not one. + * + * The `codegraph_control` marker is what CLAIMS the connection: without it the + * line belongs to a proxy hello or a direct MCP client and must reach the + * transport verbatim, so a junk first line stays MCP's problem exactly as it + * was. With it, the connection is a control caller and is answered as one even + * if the rest of the command is nonsense. + */ +export function parseDaemonControlLine(line: string): DaemonControlCommand | null { + let parsed: unknown; + try { parsed = JSON.parse(line); } catch { return null; } + if (!parsed || typeof parsed !== 'object') return null; + const o = parsed as Record; + if (o.codegraph_control !== 1) return null; + return { op: o.op, sessionId: o.sessionId }; +} + /** * A client's peer is dead when its proxy process is gone, or when its known * host process is gone. Unknown pid (no client-hello) is never "dead" on this @@ -787,6 +848,14 @@ export function peerIsDead( return false; } +/** Outcome of reading a connection's first line. */ +interface ClientHelloResult { + pid: number | null; + hostPid: number | null; + /** Set when that line claimed the connection as a one-shot control command. */ + control?: DaemonControlCommand; +} + /** * Read the optional client-hello line a proxy sends after the daemon hello. * Always resolves (never rejects) — fail-safe by design, since every connection @@ -796,15 +865,13 @@ export function peerIsDead( * Accumulates as Buffers and splits on the newline byte so a UTF-8 sequence * straddling a chunk boundary in the unshifted tail is never corrupted. */ -function readClientHello( - socket: net.Socket, -): Promise<{ pid: number | null; hostPid: number | null }> { +function readClientHello(socket: net.Socket): Promise { return new Promise((resolve) => { let chunks: Buffer[] = []; let total = 0; let settled = false; const finish = ( - peers: { pid: number | null; hostPid: number | null }, + peers: ClientHelloResult, putBack?: Buffer, ) => { if (settled) return; @@ -845,15 +912,24 @@ function readClientHello( else chunks = [all]; return; } - const peers = parseClientHelloLine(all.subarray(0, nl).toString('utf8')); + const line = all.subarray(0, nl).toString('utf8'); + const peers = parseClientHelloLine(line); if (peers) { const tail = all.subarray(nl + 1); finish(peers, tail.length > 0 ? tail : undefined); - } else { - // First line is not a client-hello (legacy/direct client) — hand the - // whole buffer back so the transport sees the message verbatim. - finish({ pid: null, hostPid: null }, all); + return; + } + const control = parseDaemonControlLine(line); + if (control) { + // One line in, one line out: nothing after it is application data, so + // the tail is dropped rather than handed to a transport that will + // never exist for this connection. + finish({ pid: null, hostPid: null, control }); + return; } + // First line is not a client-hello (legacy/direct client) — hand the + // whole buffer back so the transport sees the message verbatim. + finish({ pid: null, hostPid: null }, all); }; const onEnd = () => finish({ pid: null, hostPid: null }); // On timeout, hand back whatever partial bytes accumulated — discarding diff --git a/src/mcp/session.ts b/src/mcp/session.ts index 1d5bd79c3..33d9e7288 100644 --- a/src/mcp/session.ts +++ b/src/mcp/session.ts @@ -159,6 +159,15 @@ export class MCPSession { return this.exploreSession; } + /** + * Forget one caller context's explore history (the daemon's control line + * relays a host hook's compact/subagent-teardown reset). Narrow on purpose: + * the daemon selects buckets, it never handles the state itself. + */ + clearSessionRecord(sessionId: string): number { + return this.exploreSession.clearSessionRecord(sessionId); + } + private async handleMessage(message: JsonRpcRequest | JsonRpcNotification): Promise { const isRequest = 'id' in message; switch (message.method) { From 763cc5dc0ae1c7dbf176989388d8dd9f52eda9b9 Mon Sep 17 00:00:00 2001 From: Atkins Chang Date: Wed, 26 Aug 2026 17:59:23 +0800 Subject: [PATCH 3/4] feat(installer): wire the per-context session hooks into Claude Code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-caller bucketing only engages if something injects the caller id, so without the hooks installed no real user gets the fix — the record stays keyed per MCP connection and a subagent is still told source was "already sent". The Claude target now merges two entries into the settings file for the install's scope: PreToolUse, matched on the tool suffix `mcp__.*__codegraph_explore` because the MCP server name is the user's to rename, and PostCompact, which fires once compaction has completed and before the resumed session's first turn — the only moment a reset is correct, since PreCompact would run while the agent still holds the source. PostCompact covers manual and automatic compaction alike and supports no matcher, so that key is omitted rather than written empty. The PreToolUse command names its agent, so the hook emits Claude's protocol rather than another's — which here means no `permissionDecision`: Claude acts on that field, and "allow" would auto-approve a call the user's own permission flow should decide. The command prefers the absolute launcher, resolved through the existing install-method detection, rather than trusting PATH: hooks run under whatever environment the host hands them, and Claude executes them through Git Bash on Windows. Entries are recognized by their `hooks ` substring with flags excluded, so an entry whose path or flags have drifted is rewritten in place instead of joined by a second copy of itself. The structure surgery — launcher resolution, the merge, and the prune — lives in the shared target helpers rather than in this target, since it is the host's event names and file that vary, not the shape of the edit. Each target keeps its own read/write policy, which is where they genuinely differ. Surgical throughout: our entries go in as their own groups, appended so a host that identifies a hook by position never renumbers the user's; uninstall drops exactly those two commands and prunes only structures left empty; a matcher-less sibling group survives both directions; a non-array a user put under one of these events is never overwritten; and a re-run on an unchanged file leaves the bytes alone. Claude only — Codex is immune by construction and Cursor/opencode have no equivalent hook system. --- CHANGELOG.md | 2 +- __tests__/installer-targets.test.ts | 198 +++++++++++++++++++++++++++- src/installer/targets/claude.ts | 106 +++++++++++---- src/installer/targets/shared.ts | 145 ++++++++++++++++++++ 4 files changed, 422 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82840c4e5..651d47596 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### New Features -- Two new hook entry points let a host agent tell CodeGraph which context is calling: `codegraph hooks pre-tool-use` tags each `codegraph_explore` call with the agent that made it, and `codegraph hooks post-compact` clears that context's already-sent record after a compaction. Wire them into your agent's hook settings and already-sent tracking follows each subagent and each compacted session instead of the connection they share. Nothing changes for hosts that don't run them. +- Two new hook entry points let a host agent tell CodeGraph which context is calling: `codegraph hooks pre-tool-use` tags each `codegraph_explore` call with the agent that made it, and `codegraph hooks post-compact` clears that context's already-sent record after a compaction. `codegraph install` wires them into Claude Code automatically, so already-sent tracking follows each subagent and each compacted session instead of the connection they share; `codegraph uninstall` removes them, and hooks you added yourself are left alone either way. Other agents are unaffected — nothing changes for hosts that don't run them. ### Fixes diff --git a/__tests__/installer-targets.test.ts b/__tests__/installer-targets.test.ts index 4ec3e5903..e2f0b7251 100644 --- a/__tests__/installer-targets.test.ts +++ b/__tests__/installer-targets.test.ts @@ -22,7 +22,13 @@ import { parse as parseJsonc } from 'jsonc-parser'; import { ALL_TARGETS, getTarget, resolveTargetFlag } from '../src/installer/targets/registry'; import { uninstallTargets, refreshTargets } from '../src/installer'; import { upsertTomlTable, removeTomlTable, buildTomlTable } from '../src/installer/targets/toml'; -import { cleanupLegacyHooks, writePromptHookEntry, removePromptHookEntry } from '../src/installer/targets/claude'; +import { + cleanupLegacyHooks, + writePromptHookEntry, + removePromptHookEntry, + writeSessionHookEntries, + removeSessionHookEntries, +} from '../src/installer/targets/claude'; function mkTmpDir(label: string): string { return fs.mkdtempSync(path.join(os.tmpdir(), `cg-targets-${label}-`)); @@ -1305,6 +1311,196 @@ describe('Installer targets — partial-state idempotency', () => { const stopCmds = (s.hooks?.Stop ?? []).flatMap((g: any) => (g.hooks ?? []).map((h: any) => h.command)); expect(stopCmds).toContain('codegraph sync-if-dirty'); }); + + // ---- Per-context session hooks (PreToolUse / PostCompact) ---- + // These are the delivery mechanism for per-context dedup: without them the + // already-sent record stays keyed per MCP connection and a subagent — + // dispatching over its parent's — is told source was "already sent" to a + // context that never received it. Written unconditionally by install (no + // opt-in flag), so the surgery has to be exact: the user's own hooks under + // the same events survive install AND uninstall. + const commandsFor = (s: any, event: string): string[] => + (s.hooks?.[event] ?? []).flatMap((g: any) => (g.hooks ?? []).map((h: any) => h.command)); + const readSettings = (loc: 'global' | 'local'): any => + JSON.parse(fs.readFileSync(path.join(loc === 'global' ? tmpHome : tmpCwd, '.claude', 'settings.json'), 'utf-8')); + + it('claude: install wires both session hooks under the right events', () => { + getTarget('claude')!.install('global', { autoAllow: true }); + const s = readSettings('global'); + + const preToolUse = s.hooks.PreToolUse.find((g: any) => + (g.hooks ?? []).some((h: any) => h.command.includes('hooks pre-tool-use'))); + // The MCP server name is the user's to rename, so the matcher keys on the + // tool SUFFIX rather than a fixed mcp__codegraph__ prefix. + expect(preToolUse.matcher).toBe('mcp__.*__codegraph_explore'); + expect(new RegExp(preToolUse.matcher).test('mcp__my-renamed-server__codegraph_explore')).toBe(true); + + expect(commandsFor(s, 'PostCompact').some((c: string) => c.includes('hooks post-compact'))).toBe(true); + // PostCompact fires once compaction has completed — PreCompact would run + // while the agent still holds the source, so it is deliberately not used, + // and neither is SessionStart (this event covers auto-compact explicitly). + expect(s.hooks.PreCompact).toBeUndefined(); + expect(s.hooks.SessionStart).toBeUndefined(); + + for (const event of ['PreToolUse', 'PostCompact']) { + expect(commandsFor(s, event).every((c: string) => c.includes('codegraph'))).toBe(true); + } + }); + + it('claude: session hooks install is idempotent (no duplicate, byte-identical re-run)', () => { + const claude = getTarget('claude')!; + const file = path.join(tmpHome, '.claude', 'settings.json'); + claude.install('global', { autoAllow: true }); + const first = fs.readFileSync(file, 'utf-8'); + expect(writeSessionHookEntries('global').action).toBe('unchanged'); + claude.install('global', { autoAllow: true }); + expect(fs.readFileSync(file, 'utf-8')).toBe(first); + + const s = JSON.parse(first); + expect(commandsFor(s, 'PreToolUse').filter((c: string) => c.includes('hooks pre-tool-use'))).toHaveLength(1); + expect(commandsFor(s, 'PostCompact').filter((c: string) => c.includes('hooks post-compact'))).toHaveLength(1); + }); + + // The user's PostCompact hook carries NO matcher — the shape that event uses — + // so this doubles as coverage that a matcher-less group survives our surgery. + const userHooks = (): Record => ({ + hooks: { + PreToolUse: [{ matcher: 'Bash', hooks: [{ type: 'command', command: 'my-bash-guard' }] }], + PostCompact: [{ hooks: [{ type: 'command', command: 'my-compact-note' }] }], + }, + }); + + it('claude: install preserves the user\'s own hooks under the same events', () => { + seedSettings('global', userHooks()); + getTarget('claude')!.install('global', { autoAllow: true }); + const s = readSettings('global'); + expect(commandsFor(s, 'PreToolUse')).toContain('my-bash-guard'); + expect(commandsFor(s, 'PostCompact')).toContain('my-compact-note'); + // The user's groups are untouched — ours is appended as its own. + expect(s.hooks.PreToolUse[0]).toEqual({ matcher: 'Bash', hooks: [{ type: 'command', command: 'my-bash-guard' }] }); + expect(s.hooks.PostCompact[0]).toEqual({ hooks: [{ type: 'command', command: 'my-compact-note' }] }); + }); + + it('claude: uninstall removes exactly our two entries and leaves the user\'s', () => { + seedSettings('global', userHooks()); + getTarget('claude')!.install('global', { autoAllow: true }); + getTarget('claude')!.uninstall('global'); + const s = readSettings('global'); + expect(commandsFor(s, 'PreToolUse')).toEqual(['my-bash-guard']); + // A matcher-less sibling group survives verbatim: the removal keys on the + // command, never on the group's matcher. + expect(s.hooks.PostCompact).toEqual([{ hooks: [{ type: 'command', command: 'my-compact-note' }] }]); + }); + + it('claude: uninstall reverses install, leaving no hook structures behind', () => { + const claude = getTarget('claude')!; + claude.install('global', { autoAllow: true }); + claude.uninstall('global'); + const s = readSettings('global'); + // Nothing else lived under these events, so the events — and `hooks` + // itself, which we created — are pruned rather than left as husks. + expect(s.hooks).toBeUndefined(); + }); + + it('claude: re-install re-points a session hook whose binary path went stale', () => { + seedSettings('global', { + hooks: { + PreToolUse: [{ + matcher: 'mcp__.*__codegraph_explore', + hooks: [{ type: 'command', command: '/gone/versions/v0.0.1/bin/codegraph hooks pre-tool-use' }], + }], + }, + }); + expect(writeSessionHookEntries('global').action).toBe('updated'); + const s = readSettings('global'); + // Rewritten in place, never duplicated: recognition is by the subcommand + // WITHOUT its flags, so both a path from an older install and a flag set + // that has since changed are ours to heal — the seed above carries neither + // the current path nor `--agent`, and comes back with both. + const pre = commandsFor(s, 'PreToolUse'); + expect(pre).toHaveLength(1); + expect(pre[0]).not.toContain('/gone/'); + expect(pre[0]).toContain('hooks pre-tool-use --agent claude'); + }); + + it('claude: session-hook removal leaves the prompt hook and legacy hooks alone', () => { + seedSettings('global', { + hooks: { + UserPromptSubmit: [{ hooks: [{ type: 'command', command: HOOK_CMD }] }], + Stop: [{ hooks: [{ type: 'command', command: 'codegraph sync-if-dirty' }] }], + }, + }); + writeSessionHookEntries('global'); + expect(removeSessionHookEntries('global').action).toBe('removed'); + const s = readSettings('global'); + expect(commandsFor(s, 'UserPromptSubmit')).toEqual([HOOK_CMD]); + expect(commandsFor(s, 'Stop')).toEqual(['codegraph sync-if-dirty']); + expect(s.hooks.PreToolUse).toBeUndefined(); + expect(s.hooks.PostCompact).toBeUndefined(); + }); + + it('claude: install never overwrites a non-array under a hook event', () => { + seedSettings('global', { hooks: { PreToolUse: { unexpected: 'user shape' } } }); + getTarget('claude')!.install('global', { autoAllow: true }); + const s = readSettings('global'); + expect(s.hooks.PreToolUse).toEqual({ unexpected: 'user shape' }); + // The other event still gets wired — one odd key doesn't block the install. + expect(commandsFor(s, 'PostCompact').some((c: string) => c.includes('hooks post-compact'))).toBe(true); + }); + + /** + * Fabricate the bundle layout `detectInstallMethod` recognizes — a vendored + * node and a launcher as siblings of `lib/` — and point argv[1] at the JS + * entry inside it, exactly as the real launcher does. + */ + function asBundledBinary(root: string, run: () => T): T { + const launcherName = process.platform === 'win32' ? 'codegraph.cmd' : 'codegraph'; + fs.mkdirSync(path.join(root, 'bin'), { recursive: true }); + fs.mkdirSync(path.join(root, 'lib', 'dist', 'bin'), { recursive: true }); + fs.writeFileSync(path.join(root, process.platform === 'win32' ? 'node.exe' : 'node'), ''); + fs.writeFileSync(path.join(root, 'bin', launcherName), ''); + const prev = process.argv[1]; + process.argv[1] = path.join(root, 'lib', 'dist', 'bin', 'codegraph.js'); + try { return run(); } finally { process.argv[1] = prev; } + } + + it('claude: prefers the absolute launcher over PATH when running from a bundle', () => { + const root = path.join(tmpCwd, 'bundle', 'versions', 'v1.2.3'); + const launcher = path.join(root, 'bin', process.platform === 'win32' ? 'codegraph.cmd' : 'codegraph'); + asBundledBinary(root, () => getTarget('claude')!.install('global', { autoAllow: true })); + const s = readSettings('global'); + expect(commandsFor(s, 'PreToolUse')).toEqual([`${launcher} hooks pre-tool-use --agent claude`]); + expect(commandsFor(s, 'PostCompact')).toEqual([`${launcher} hooks post-compact`]); + }); + + it('claude: quotes a launcher path containing spaces — hook commands are shell strings', () => { + const root = path.join(tmpCwd, 'My Tools', 'versions', 'v1.2.3'); + const launcher = path.join(root, 'bin', process.platform === 'win32' ? 'codegraph.cmd' : 'codegraph'); + asBundledBinary(root, () => writeSessionHookEntries('global')); + expect(commandsFor(readSettings('global'), 'PreToolUse')).toEqual([`"${launcher}" hooks pre-tool-use --agent claude`]); + }); + + it('claude: falls back to the PATH spelling when there is no installed bundle', () => { + // vitest's argv[1] is not a bundle layout — the source/npm/npx case. + getTarget('claude')!.install('global', { autoAllow: true }); + const expected = process.platform === 'win32' ? 'codegraph.cmd' : 'codegraph'; + expect(commandsFor(readSettings('global'), 'PreToolUse')).toEqual([`${expected} hooks pre-tool-use --agent claude`]); + }); + + it('claude: session hooks are Claude-only — no other target writes them', () => { + for (const target of ALL_TARGETS) { + if (target.id === 'claude') continue; + target.install('global', { autoAllow: true }); + } + // Codex is immune by construction (per-thread connections); Cursor and + // opencode have no equivalent hook system. None of them may grow one here. + const settings = path.join(tmpHome, '.claude', 'settings.json'); + if (fs.existsSync(settings)) { + const s = JSON.parse(fs.readFileSync(settings, 'utf-8')); + expect(commandsFor(s, 'PreToolUse')).toEqual([]); + expect(commandsFor(s, 'PostCompact')).toEqual([]); + } + }); }); describe('Installer targets — registry', () => { diff --git a/src/installer/targets/claude.ts b/src/installer/targets/claude.ts index e95b0a35d..966221f89 100644 --- a/src/installer/targets/claude.ts +++ b/src/installer/targets/claude.ts @@ -28,13 +28,18 @@ import { WriteResult, } from './types'; import { + codegraphBinary, getCodeGraphPermissions, getMcpServerConfig, + isSessionHookCommand, jsonDeepEqual, + mergeHookEntries, + pruneHookCommands, readJsonFile, removeMarkedSection, writeJsonFile, upsertInstructionsEntry, + type HookEntry, } from './shared'; import { CODEGRAPH_SECTION_END, @@ -133,6 +138,14 @@ class ClaudeCodeTarget implements AgentTarget { if (removed.action === 'removed') files.push(removed); } + // 2d. Per-context session hooks. Unconditional: they are what tells the + // server WHICH agent context a call belongs to, and without them the + // cross-call dedup record stays keyed per MCP connection — so a subagent, + // which dispatches over its parent's, is told source was "already sent" to + // a context that never received it. Both hooks are inert to a user who + // never triggers them. + files.push(writeSessionHookEntries(loc)); + // 3. CLAUDE.md instructions — the short marker-fenced CodeGraph // block (#704). The MCP initialize instructions reach only the main // agent; CLAUDE.md is what Task-tool subagents (and non-MCP @@ -203,6 +216,10 @@ class ClaudeCodeTarget implements AgentTarget { const promptHookCleanup = removePromptHookEntry(loc); if (promptHookCleanup.action === 'removed') files.push(promptHookCleanup); + // 2d. Remove the per-context session hooks. + const sessionHookCleanup = removeSessionHookEntries(loc); + if (sessionHookCleanup.action === 'removed') files.push(sessionHookCleanup); + // 3. Instructions — strip the legacy CodeGraph block if present. files.push(removeInstructionsEntry(loc)); @@ -344,33 +361,7 @@ function removeHookCommandsMatching( return { path: file, action: 'unchanged' }; } - // Pass 1: drop matching command(s) from inside every matcher group. - let removedAny = false; - for (const event of Object.keys(hooks)) { - const groups = hooks[event]; - if (!Array.isArray(groups)) continue; - for (const group of groups) { - if (!group || !Array.isArray(group.hooks)) continue; - const before = group.hooks.length; - group.hooks = group.hooks.filter((h: any) => !match(h?.command)); - if (group.hooks.length !== before) removedAny = true; - } - } - - if (!removedAny) return { path: file, action: 'unchanged' }; - - // Pass 2: prune empty matcher groups, then events with no groups left, - // then an empty top-level `hooks`. Guarded by `removedAny` so we never - // restructure a settings.json that had no matching hooks. Sibling hooks - // (a different command in the group, or a different event) survive. - for (const event of Object.keys(hooks)) { - const groups = hooks[event]; - if (!Array.isArray(groups)) continue; - hooks[event] = groups.filter( - (g: any) => !(g && Array.isArray(g.hooks) && g.hooks.length === 0), - ); - if (hooks[event].length === 0) delete hooks[event]; - } + if (!pruneHookCommands(hooks, match)) return { path: file, action: 'unchanged' }; if (Object.keys(hooks).length === 0) delete settings.hooks; writeJsonFile(file, settings); @@ -395,6 +386,67 @@ export function removePromptHookEntry(loc: Location): WriteResult['files'][numbe return removeHookCommandsMatching(loc, isPromptHookCommand); } +/** + * The per-context session hooks, as Claude Code spells them. + * + * `PreToolUse` stamps the calling agent's id onto a `codegraph_explore` call so + * the server's already-sent record is bucketed per agent context rather than + * per MCP connection — without it a subagent, which dispatches over its + * parent's connection, is told source was "already sent" to a context that + * never received it. The matcher is a regex over the tool name, and the MCP + * server name is the user's to rename, so it matches the tool SUFFIX rather + * than a fixed `mcp__codegraph__` prefix. The command names its agent so the + * hook emits Claude's protocol and not another's — here that means NO + * `permissionDecision`, since Claude acts on the field and would auto-approve + * a call the user's own permission flow should decide. + * + * `PostCompact` fires once compaction has completed and before the resumed + * session's first turn, which is the only moment a reset is correct — + * `PreCompact` runs while the agent still holds the source. It covers manual + * `/compact` and auto-compact alike, and takes no matcher: events without + * matcher support omit the key rather than carrying an empty one. + */ +const SESSION_HOOKS: HookEntry[] = [ + { event: 'PreToolUse', matcher: 'mcp__.*__codegraph_explore', subcommand: 'hooks pre-tool-use --agent claude' }, + { event: 'PostCompact', subcommand: 'hooks post-compact' }, +]; + +/** + * Wire the per-context session hooks into Claude `settings.json`. + * + * Surgical: each entry is appended as its own matcher group, so the user's own + * hooks — including other groups under the same event — are untouched. An + * entry we already wrote is rewritten in place when the binary spelling has + * drifted (a moved bundle, a settings.json synced from another platform) and + * otherwise left alone, so a re-run on an unchanged file reports `unchanged` + * and the bytes never move. + */ +export function writeSessionHookEntries(loc: Location): WriteResult['files'][number] { + const file = settingsJsonPath(loc); + const created = !fs.existsSync(file); + const settings = readJsonFile(file); + + if (!settings.hooks || typeof settings.hooks !== 'object' || Array.isArray(settings.hooks)) { + settings.hooks = {}; + } + + const changed = mergeHookEntries(settings.hooks, SESSION_HOOKS, codegraphBinary()); + + if (!changed && !created) return { path: file, action: 'unchanged' }; + writeJsonFile(file, settings); + return { path: file, action: created ? 'created' : 'updated' }; +} + +/** + * Remove the per-context session hooks this installer writes. Reuses the + * command-level surgery `removeHookCommandsMatching` already does: a sibling + * hook sharing our event survives, and the structures we created are pruned + * only once they are empty. + */ +export function removeSessionHookEntries(loc: Location): WriteResult['files'][number] { + return removeHookCommandsMatching(loc, isSessionHookCommand); +} + export function writePermissionsEntry(loc: Location): WriteResult['files'][number] { const file = settingsJsonPath(loc); const settings = readJsonFile(file); diff --git a/src/installer/targets/shared.ts b/src/installer/targets/shared.ts index 364f40427..95ccccc4a 100644 --- a/src/installer/targets/shared.ts +++ b/src/installer/targets/shared.ts @@ -15,6 +15,7 @@ import { CODEGRAPH_SECTION_START, CODEGRAPH_SECTION_END, } from '../instructions-template'; +import { detectInstallMethod } from '../../upgrade'; /** * The MCP-server config block codegraph injects. Same shape across @@ -48,6 +49,150 @@ export function getCodeGraphPermissions(): string[] { return ['mcp__codegraph__*']; } +/** + * The `codegraph` spelling to write into a hook command. + * + * Prefer the ABSOLUTE launcher — same discipline as Cursor's `--path`: resolve + * at install time what the agent would otherwise have to find at run time. A + * hook runs under whatever environment the host hands it, and Claude Code + * executes hooks through Git Bash on Windows, so a PATH lookup is the fragile + * half of the contract (#1466). + * + * Only a bundle install HAS an absolute launcher to point at; npm/npx put a + * shim on PATH and a source checkout has no installed binary at all, so those + * keep the PATH spelling. A path that has since moved is not a trap: hooks are + * recognized by their `hooks ` substring, so a re-install rewrites + * a stale one in place (see {@link mergeHookEntries}). + */ +export function codegraphBinary(): string { + const fallback = process.platform === 'win32' ? 'codegraph.cmd' : 'codegraph'; + try { + const method = detectInstallMethod({ + filename: process.argv[1] ?? '', + platform: process.platform, + cwd: process.cwd(), + }); + if (method.kind !== 'bundle' || !method.bundleRoot) return fallback; + const launcher = path.join(method.bundleRoot, 'bin', fallback); + if (!fs.existsSync(launcher)) return fallback; + // Hook commands are shell strings, so a launcher under "C:\Users\A B\…" + // has to survive word-splitting. + return /\s/.test(launcher) ? `"${launcher}"` : launcher; + } catch { + return fallback; + } +} + +/** One hook to wire: which event, an optional matcher, and our subcommand. */ +export interface HookEntry { + event: string; + matcher?: string; + subcommand: string; +} + +/** + * The `codegraph hooks` subcommands the installer wires into a host. Shared by + * every target: recognition has to be spelling-independent, so it keys on the + * subcommand rather than on the binary path or the host's event names. + */ +const SESSION_HOOK_SUBCOMMANDS = ['hooks pre-tool-use', 'hooks post-compact']; + +/** + * Recognizes a session hook this installer wrote, whatever binary spelling it + * was written with. The `codegraph`-scoped subcommand is the stable part — + * matching on it keeps an absolute path, a `.cmd`, or an `npx …` form all + * recognizable, and cannot collide with an unrelated user hook. + */ +export function isSessionHookCommand(command: unknown): boolean { + if (typeof command !== 'string' || !command.includes('codegraph')) return false; + return SESSION_HOOK_SUBCOMMANDS.some((s) => command.includes(s)); +} + +/** + * Merge hook entries into a host's event→matcher-groups map, in place. Returns + * whether anything actually changed, so the caller can leave a byte-identical + * file alone. + * + * Ours is always APPENDED as its own group rather than folded into a user's: + * hosts identify a hook by its position (codex keys its trust record on the + * group and handler index), so inserting ahead of existing groups renumbers + * them. An entry we already wrote is re-pointed at the binary we resolve now + * instead of being duplicated. + * + * Structure-only: every target reads and writes its own file, because their + * policies for a malformed one differ. + */ +export function mergeHookEntries( + hooks: Record, + entries: HookEntry[], + binary: string, +): boolean { + let changed = false; + for (const { event, matcher, subcommand } of entries) { + // A non-array under an event is not a shape any host reads, but it is the + // user's — overwriting it would destroy config we don't understand. + if (event in hooks && !Array.isArray(hooks[event])) continue; + const command = `${binary} ${subcommand}`; + // Recognize on the subcommand WITHOUT its flags: an entry whose flags have + // since changed is still the same entry, and must be re-pointed rather than + // joined by a second copy of itself. + const stable = subcommand.split(' --')[0]; + const groups: any[] = hooks[event] ?? []; + const ours = groups + .flatMap((g: any) => (g && Array.isArray(g.hooks) ? g.hooks : [])) + .filter((h: any) => isSessionHookCommand(h?.command) && h.command.includes(stable)); + if (ours.length > 0) { + for (const h of ours) { + if (h.command !== command) { h.command = command; changed = true; } + } + continue; + } + groups.push({ ...(matcher ? { matcher } : {}), hooks: [{ type: 'command', command }] }); + hooks[event] = groups; + changed = true; + } + return changed; +} + +/** + * Drop every hook command matching `match` from a host's event→groups map, in + * place, then prune what that emptied. Returns whether anything was removed. + * + * Surgical at the individual-command level: a sibling hook sharing a group (or + * an event) with ours survives. A group is pruned only once its `hooks` array + * is empty and an event only once it has no groups left — and none of that runs + * unless a command was actually removed, so a file with none of ours is left + * byte-for-byte untouched. The caller owns the now-possibly-empty `hooks` key + * itself, since where it hangs differs by host. + */ +export function pruneHookCommands( + hooks: Record, + match: (command: unknown) => boolean, +): boolean { + let removedAny = false; + for (const event of Object.keys(hooks)) { + const groups = hooks[event]; + if (!Array.isArray(groups)) continue; + for (const group of groups) { + if (!group || !Array.isArray(group.hooks)) continue; + const before = group.hooks.length; + group.hooks = group.hooks.filter((h: any) => !match(h?.command)); + if (group.hooks.length !== before) removedAny = true; + } + } + if (!removedAny) return false; + + for (const event of Object.keys(hooks)) { + const groups = hooks[event]; + if (!Array.isArray(groups)) continue; + hooks[event] = groups.filter( + (g: any) => !(g && Array.isArray(g.hooks) && g.hooks.length === 0), + ); + if (hooks[event].length === 0) delete hooks[event]; + } + return true; +} + /** * Read a JSON file, returning `{}` when missing or unparseable. * From e4be07c83fa2b3d21b708960d9c85ffbbfd355b9 Mon Sep 17 00:00:00 2001 From: Atkins Chang Date: Wed, 26 Aug 2026 19:58:35 +0800 Subject: [PATCH 4/4] feat(installer): wire the per-context session hooks into Codex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex carries the same problem the Claude wiring solved: without a hook to stamp the calling context, the server's already-sent record stays keyed per MCP connection. Its per-thread connections mean it never needs the stamp for isolation, but the post-compact reset still has to name a bucket. Codex discovers hooks from a `hooks.json` in each config layer's `.codex/` folder, which maps onto the global/local split the TOML config already uses. Two entries go in: PreToolUse, whose matcher is a regex over the tool name, and PostCompact, which codex gates behind compaction SUCCESS in three of its four compaction paths. That gate is what we want: in all four paths every failure returns before the history rewrite commits, so a failed compaction leaves the agent's context — and the record describing it — intact, and skipping the reset is exactly right. PreCompact would instead clear a still-valid record on every failed attempt. This matches how the Claude target wires the same reset. The PreToolUse command declares `--agent codex`, which is what makes the hook pair `permissionDecision` with its rewrite. Codex requires that pair or it silently drops the rewrite; no other agent receives the field. Both entries are recorded as trusted in `[hooks.state]` as they are written, so codex does not stop to ask about them. Its review flow guards hooks that arrive from somewhere the user didn't choose; an installer they just invoked, writing its own two entries pointing at its own binary, is not that threat, and a local install's state still sits behind codex's project-trust gate. Trust is held per ENTRY — a hash of each handler's normalized identity — so this can only ever record the two entries the installer wrote; the user's hooks are unreachable from there and their records are never read or rewritten. The hash reimplements a codex internal and is pinned by a fixture test against real entries codex wrote; if it ever stops agreeing, the entry simply reads as needing review, which is what happens today. An unparseable hooks.json is left exactly as it is rather than backed up and replaced: codex would otherwise run our file in place of config we failed to read. The contract suite's "sibling MCP server" case keyed off the first .json in describePaths as a proxy for "this target's MCP config". hooks.json is written but holds no MCP entry, so that proxy now asks detect() for the config path directly. Its "Claude-only" hooks case becomes "Claude and Codex only", and now also checks the remaining targets' own config files rather than only Claude's. The array-of-tables case no longer requires the user's `[[history]]` tables to END the file, since the trust records append after them — it requires them to survive byte-for-byte, which is the actual guarantee. --- CHANGELOG.md | 2 +- __tests__/installer-targets.test.ts | 282 ++++++++++++++++++++++++- src/installer/targets/codex.ts | 308 +++++++++++++++++++++++++++- src/installer/targets/toml.ts | 20 ++ 4 files changed, 598 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 651d47596..be585cb68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### New Features -- Two new hook entry points let a host agent tell CodeGraph which context is calling: `codegraph hooks pre-tool-use` tags each `codegraph_explore` call with the agent that made it, and `codegraph hooks post-compact` clears that context's already-sent record after a compaction. `codegraph install` wires them into Claude Code automatically, so already-sent tracking follows each subagent and each compacted session instead of the connection they share; `codegraph uninstall` removes them, and hooks you added yourself are left alone either way. Other agents are unaffected — nothing changes for hosts that don't run them. +- Two new hook entry points let a host agent tell CodeGraph which context is calling: `codegraph hooks pre-tool-use` tags each `codegraph_explore` call with the agent that made it, and `codegraph hooks post-compact` clears that context's already-sent record after a compaction. `codegraph install` wires them into Claude Code and Codex automatically — on Codex they are recorded as trusted as they are written, so it doesn't stop to ask you to review them — and already-sent tracking then follows each subagent and each compacted session instead of the connection they share. `codegraph uninstall` removes them, and hooks you added yourself are left alone either way. Other agents are unaffected — nothing changes for hosts that don't run them. ### Fixes diff --git a/__tests__/installer-targets.test.ts b/__tests__/installer-targets.test.ts index e2f0b7251..812a2bc15 100644 --- a/__tests__/installer-targets.test.ts +++ b/__tests__/installer-targets.test.ts @@ -29,6 +29,7 @@ import { writeSessionHookEntries, removeSessionHookEntries, } from '../src/installer/targets/claude'; +import { codexHookTrustHash } from '../src/installer/targets/codex'; function mkTmpDir(label: string): string { return fs.mkdtempSync(path.join(os.tmpdir(), `cg-targets-${label}-`)); @@ -133,9 +134,14 @@ describe('Installer targets — contract', () => { // and verify the sibling survives. Skip for Codex (TOML) // and any target with no JSON config — they get covered // by their own dedicated tests below. - const paths = target.describePaths(location); + // + // Keyed on detect()'s configPath — the target's own answer for + // "where does my MCP entry live" — rather than scanning + // describePaths for a .json, which also turns up files that are + // written but hold no MCP config (codex's hooks.json). + const mcpConfig = target.detect(location).configPath; // Match .json or .jsonc — opencode prefers .jsonc. - const jsonPath = paths.find((p) => /\.jsonc?$/.test(p)); + const jsonPath = /\.jsonc?$/.test(mcpConfig) ? mcpConfig : undefined; if (!jsonPath) return; // Seed pre-existing config. @@ -963,7 +969,11 @@ describe('Installer targets — partial-state idempotency', () => { const afterInstall = fs.readFileSync(tomlPath, 'utf-8'); expect(afterInstall).toContain('command = "codegraph"'); expect(afterInstall).not.toContain('[[not-a-table]]'); - expect(afterInstall.endsWith(historyTables)).toBe(true); + // The array-of-tables survives verbatim. It no longer ends the file — the + // hooks' trust records append after it, which is what closes the last + // `[[history]]` element — but not a byte of it is rewritten or reordered. + expect(afterInstall).toContain(historyTables.trimEnd()); + expect(afterInstall.match(/\[\[history\]\]/g)).toHaveLength(2); const second = codex.install('global', { autoAllow: false }); expect(second.files.find((f) => f.path === tomlPath)?.action).toBe('unchanged'); @@ -973,6 +983,246 @@ describe('Installer targets — partial-state idempotency', () => { expect(fs.readFileSync(tomlPath, 'utf-8')).toBe(historyTables); }); + // ---- Codex per-context session hooks (hooks.json) ---- + // Codex reads hooks from a hooks.json in each config layer's .codex/ folder. + // Same two entries as the Claude target, spelled codex's way: PreToolUse with + // a regex over the tool name, and PostCompact with no matcher at all. Trust + // is held per ENTRY in config.toml's + // [hooks.state], which the installer must never write — adding entries leaves + // the user's already-trusted hooks trusted, and ours await their review. + const hooksPathFor = (loc: 'global' | 'local'): string => + path.join(loc === 'global' ? tmpHome : tmpCwd, '.codex', 'hooks.json'); + const hookCommandsFor = (file: string, event: string): string[] => { + const root = JSON.parse(fs.readFileSync(file, 'utf-8')); + return (root.hooks?.[event] ?? []).flatMap((g: any) => (g.hooks ?? []).map((h: any) => h.command)); + }; + + for (const location of ['global', 'local'] as const) { + it(`codex: install writes both session hooks to ${location} hooks.json`, () => { + const result = getTarget('codex')!.install(location, { autoAllow: false }); + const file = hooksPathFor(location); + expect(fs.existsSync(file)).toBe(true); + expect(result.files.find((f) => f.path === file)?.action).toBe('created'); + + const root = JSON.parse(fs.readFileSync(file, 'utf-8')); + const preToolUse = root.hooks.PreToolUse.find((g: any) => + (g.hooks ?? []).some((h: any) => h.command.includes('hooks pre-tool-use'))); + expect(preToolUse.matcher).toBe('codegraph_explore'); + expect(preToolUse.hooks[0].type).toBe('command'); + // --agent codex is what makes the hook pair permissionDecision with the + // rewrite; codex drops an unpaired one, and no other agent gets the field. + expect(preToolUse.hooks[0].command).toMatch(/hooks pre-tool-use --agent codex$/); + // PostCompact: codex fires it only when compaction succeeded, and every + // failure path returns before the history rewrite commits — so the skip + // on failure is correct, and PreCompact would clear a still-valid record. + expect(hookCommandsFor(file, 'PostCompact').some((c) => c.includes('hooks post-compact'))).toBe(true); + expect(root.hooks.PreCompact).toBeUndefined(); + + // The hooks.json write surfaces ONLY as the Created/Updated file line + // asserted above — no prose about it. Codex's own TUI is where untrusted + // hooks surface, so the install output stays a list of file actions. + expect((result.notes ?? []).some((n) => n.includes('hooks.json'))).toBe(false); + }); + } + + /** + * The trust hash reimplements a codex internal, so it is pinned against a + * REAL trusted pair — a hooks.json group and the `trusted_hash` codex itself + * wrote for it. If codex changes its normalization this test fails loudly + * here rather than silently leaving users with entries codex reports as + * needing review. + */ + it('codex: reproduces the trust hash codex itself writes', () => { + // Verbatim from a real ~/.codex/hooks.json + config.toml pair: a PreToolUse + // group matching `spawn_agent$` with one command handler and no timeout. + expect(codexHookTrustHash( + 'PreToolUse', + 'spawn_agent$', + '/home/atkins/.codex/hooks/subagent.py', + 'Validating subagent runtime profile', + )).toBe('sha256:dd9550baea8600a2d1a58f765a532ac503baf8c786cd4c0d3b76742874961733'); + expect(codexHookTrustHash( + 'PostToolUse', + 'spawn_agent$', + '/home/atkins/.codex/hooks/subagent.py', + 'Rendering subagent runtime profile', + )).toBe('sha256:e5c757d1ea8dbffed4b43dd9f8a994bcb60e927b30a60d8d488972e8eef62d0d'); + // A matcher-less group omits the key entirely rather than sending an empty + // string — TOML has no null, so codex's own serialization drops it. + expect(codexHookTrustHash('PostCompact', undefined, 'x')) + .not.toBe(codexHookTrustHash('PostCompact', '', 'x')); + }); + + it('codex: reports the hooks file as a plain file action, nothing more', () => { + const result = getTarget('codex')!.install('global', { autoAllow: false }); + // Pre-trusting is not narrated anywhere in the output — the hooks file + // reports exactly like every other file the installer touches. + expect(result.files.find((f) => f.path === hooksPathFor('global'))) + .toEqual({ path: hooksPathFor('global'), action: 'created' }); + expect((result.notes ?? []).some((n) => /trust/i.test(n))).toBe(false); + }); + + it('codex: records ONLY our two entries, with codex-shaped keys', () => { + getTarget('codex')!.install('global', { autoAllow: false }); + const tomlPath = path.join(tmpHome, '.codex', 'config.toml'); + const toml = fs.readFileSync(tomlPath, 'utf-8'); + const hooksFile = hooksPathFor('global'); + + // Key is :::. + for (const event of ['pre_tool_use', 'post_compact']) { + expect(toml).toContain(`[hooks.state."${hooksFile}:${event}:0:0"]`); + } + // Exactly two records, and each hash matches what our hooks.json says. + expect(toml.match(/trusted_hash/g)).toHaveLength(2); + const root = JSON.parse(fs.readFileSync(hooksFile, 'utf-8')); + const preCmd = root.hooks.PreToolUse[0].hooks[0].command; + expect(toml).toContain(codexHookTrustHash('PreToolUse', 'codegraph_explore', preCmd)); + expect(toml).toContain(codexHookTrustHash('PostCompact', undefined, root.hooks.PostCompact[0].hooks[0].command)); + }); + + it('codex: auto-trust never rewrites the user\'s own trust records', () => { + const tomlPath = path.join(tmpHome, '.codex', 'config.toml'); + fs.mkdirSync(path.dirname(tomlPath), { recursive: true }); + fs.writeFileSync(tomlPath, [ + '[hooks.state."/home/u/.codex/hooks.json:pre_tool_use:0:0"]', + 'trusted_hash = "sha256:theirs"', + 'enabled = true', + '', + ].join('\n')); + + getTarget('codex')!.install('global', { autoAllow: false }); + const toml = fs.readFileSync(tomlPath, 'utf-8'); + // Trust is per entry, so theirs is unreachable from ours — byte-for-byte. + expect(toml).toContain('[hooks.state."/home/u/.codex/hooks.json:pre_tool_use:0:0"]'); + expect(toml).toContain('trusted_hash = "sha256:theirs"'); + expect(toml).toContain('enabled = true'); + }); + + it('codex: re-trusting an upgraded launcher keeps the user\'s enabled flag', () => { + const codex = getTarget('codex')!; + codex.install('global', { autoAllow: false }); + const tomlPath = path.join(tmpHome, '.codex', 'config.toml'); + const key = `${hooksPathFor('global')}:pre_tool_use:0:0`; + + // The user disabled our hook in the Codex TUI, then upgraded — the launcher + // path is part of the hashed identity, so the hash must be refreshed + // WITHOUT re-enabling a hook they turned off. + let toml = fs.readFileSync(tomlPath, 'utf-8'); + toml = toml.replace(`[hooks.state."${key}"]`, `[hooks.state."${key}"]\nenabled = false`); + fs.writeFileSync(tomlPath, toml); + // Only the binary moves; the subcommand must survive or this stops being an + // upgrade and becomes an unrecognized hook. + const stale = fs.readFileSync(hooksPathFor('global'), 'utf-8') + .replace('"command": "codegraph hooks pre-tool-use', '"command": "/old/bin/codegraph hooks pre-tool-use'); + fs.writeFileSync(hooksPathFor('global'), stale); + const staleHash = codexHookTrustHash('PreToolUse', 'codegraph_explore', '/old/bin/codegraph hooks pre-tool-use --agent codex'); + + codex.install('global', { autoAllow: false }); + const after = fs.readFileSync(tomlPath, 'utf-8'); + expect(after).toContain('enabled = false'); + expect(after.match(/trusted_hash/g)).toHaveLength(2); + // Refreshed to the new launcher, in place — not left on the old one. + expect(after).not.toContain(staleHash); + expect(after).toContain(codexHookTrustHash('PreToolUse', 'codegraph_explore', 'codegraph hooks pre-tool-use --agent codex')); + }); + + it('codex: auto-trust is idempotent and uninstall removes only our records', () => { + const codex = getTarget('codex')!; + const tomlPath = path.join(tmpHome, '.codex', 'config.toml'); + fs.mkdirSync(path.dirname(tomlPath), { recursive: true }); + fs.writeFileSync(tomlPath, '[hooks.state."other:pre_tool_use:0:0"]\ntrusted_hash = "sha256:theirs"\n'); + + codex.install('global', { autoAllow: false }); + const first = fs.readFileSync(tomlPath, 'utf-8'); + codex.install('global', { autoAllow: false }); + expect(fs.readFileSync(tomlPath, 'utf-8')).toBe(first); + + codex.uninstall('global'); + const after = fs.readFileSync(tomlPath, 'utf-8'); + expect(after).toContain('other:pre_tool_use:0:0'); + expect(after).toContain('sha256:theirs'); + expect(after).not.toContain(hooksPathFor('global')); + }); + + it('codex: hooks install is idempotent (no duplicate, byte-identical re-run)', () => { + const codex = getTarget('codex')!; + const file = hooksPathFor('global'); + codex.install('global', { autoAllow: false }); + const first = fs.readFileSync(file, 'utf-8'); + + const second = codex.install('global', { autoAllow: false }); + expect(second.files.find((f) => f.path === file)?.action).toBe('unchanged'); + expect(fs.readFileSync(file, 'utf-8')).toBe(first); + expect(hookCommandsFor(file, 'PreToolUse')).toHaveLength(1); + expect(hookCommandsFor(file, 'PostCompact')).toHaveLength(1); + }); + + it('codex: install preserves the user\'s own hooks and unrelated top-level keys', () => { + const file = hooksPathFor('global'); + fs.mkdirSync(path.dirname(file), { recursive: true }); + fs.writeFileSync(file, JSON.stringify({ + description: 'my hooks', + hooks: { + PreToolUse: [{ matcher: '^Bash$', hooks: [{ type: 'command', command: 'my-guard' }] }], + SessionEnd: [{ hooks: [{ type: 'command', command: 'my-cleanup' }] }], + }, + }, null, 2) + '\n'); + + getTarget('codex')!.install('global', { autoAllow: false }); + const root = JSON.parse(fs.readFileSync(file, 'utf-8')); + expect(root.description).toBe('my hooks'); + expect(hookCommandsFor(file, 'SessionEnd')).toEqual(['my-cleanup']); + // Ours is APPENDED — codex keys a hook's trust record on its group index, + // so inserting ahead of the user's groups would renumber (and untrust) them. + expect(root.hooks.PreToolUse[0]).toEqual({ + matcher: '^Bash$', hooks: [{ type: 'command', command: 'my-guard' }], + }); + expect(root.hooks.PreToolUse).toHaveLength(2); + }); + + it('codex: uninstall removes exactly our entries and leaves the user\'s', () => { + const file = hooksPathFor('global'); + fs.mkdirSync(path.dirname(file), { recursive: true }); + fs.writeFileSync(file, JSON.stringify({ + description: 'my hooks', + hooks: { PreToolUse: [{ matcher: '^Bash$', hooks: [{ type: 'command', command: 'my-guard' }] }] }, + }, null, 2) + '\n'); + + const codex = getTarget('codex')!; + codex.install('global', { autoAllow: false }); + codex.uninstall('global'); + + const root = JSON.parse(fs.readFileSync(file, 'utf-8')); + expect(root.description).toBe('my hooks'); + expect(hookCommandsFor(file, 'PreToolUse')).toEqual(['my-guard']); + expect(root.hooks.PostCompact).toBeUndefined(); + }); + + it('codex: uninstall deletes a hooks.json that held only our entries', () => { + const codex = getTarget('codex')!; + const file = hooksPathFor('global'); + codex.install('global', { autoAllow: false }); + expect(fs.existsSync(file)).toBe(true); + + codex.uninstall('global'); + // We created it and nothing else ever lived there — no empty husk left. + expect(fs.existsSync(file)).toBe(false); + }); + + it('codex: leaves an unparseable hooks.json exactly as it is', () => { + const file = hooksPathFor('global'); + fs.mkdirSync(path.dirname(file), { recursive: true }); + const broken = '{ "hooks": { "PreToolUse": [ }}} not json'; + fs.writeFileSync(file, broken); + + const result = getTarget('codex')!.install('global', { autoAllow: false }); + // Never clobbered, never backed up and replaced: codex would then run our + // file instead of the config we failed to read. + expect(fs.readFileSync(file, 'utf-8')).toBe(broken); + expect(result.files.find((f) => f.path === file)?.action).toBe('unchanged'); + expect(fs.existsSync(`${file}.backup`)).toBe(false); + }); + it('claude: local install writes ./.mcp.json (project scope), not ./.claude.json', () => { const claude = getTarget('claude')!; const result = claude.install('local', { autoAllow: false }); @@ -1487,19 +1737,31 @@ describe('Installer targets — partial-state idempotency', () => { expect(commandsFor(readSettings('global'), 'PreToolUse')).toEqual([`${expected} hooks pre-tool-use --agent claude`]); }); - it('claude: session hooks are Claude-only — no other target writes them', () => { - for (const target of ALL_TARGETS) { - if (target.id === 'claude') continue; - target.install('global', { autoAllow: true }); - } - // Codex is immune by construction (per-thread connections); Cursor and - // opencode have no equivalent hook system. None of them may grow one here. + it('session hooks reach Claude and Codex only — no other target writes them', () => { + // Codex's per-thread connections mean it never needs the stamp for + // ISOLATION, but the post-compact reset still has to name a bucket, so it + // wires hooks of its own (see the codex cases above). Every remaining + // target has no equivalent hook system, and none may quietly grow one. + const others = ALL_TARGETS.filter((t) => t.id !== 'claude' && t.id !== 'codex'); + for (const target of others) target.install('global', { autoAllow: true }); + + // Claude's own settings.json must not have been touched by any of them. const settings = path.join(tmpHome, '.claude', 'settings.json'); if (fs.existsSync(settings)) { const s = JSON.parse(fs.readFileSync(settings, 'utf-8')); expect(commandsFor(s, 'PreToolUse')).toEqual([]); expect(commandsFor(s, 'PostCompact')).toEqual([]); } + + // Nor may any of their OWN config files carry a codegraph hook command. + for (const target of others) { + for (const file of target.describePaths('global')) { + if (!fs.existsSync(file) || !/\.jsonc?$/.test(file)) continue; + const body = fs.readFileSync(file, 'utf-8'); + expect(body, `${target.id} → ${file}`).not.toContain('hooks pre-tool-use'); + expect(body, `${target.id} → ${file}`).not.toContain('hooks post-compact'); + } + } }); }); diff --git a/src/installer/targets/codex.ts b/src/installer/targets/codex.ts index d5e361ce3..e29e27770 100644 --- a/src/installer/targets/codex.ts +++ b/src/installer/targets/codex.ts @@ -40,15 +40,22 @@ import { } from './types'; import { atomicWriteFileSync, + codegraphBinary, getMcpServerConfig, + isSessionHookCommand, + mergeHookEntries, + pruneHookCommands, removeMarkedSection, upsertInstructionsEntry, + writeJsonFile, + type HookEntry, } from './shared'; import { CODEGRAPH_SECTION_END, CODEGRAPH_SECTION_START, } from '../instructions-template'; -import { buildTomlTable, removeTomlTable, upsertTomlTable } from './toml'; +import { buildTomlTable, readTomlTableBody, removeTomlTable, upsertTomlTable } from './toml'; +import { createHash } from 'crypto'; const TOML_HEADER = 'mcp_servers.codegraph'; @@ -70,6 +77,16 @@ function instructionsPath(loc: Location): string { : path.join(process.cwd(), 'AGENTS.md'); } +/** + * Codex discovers hooks from a `hooks.json` in each config layer's `.codex/` + * folder — `~/.codex/hooks.json` for the user layer, `/.codex/hooks.json` + * for the project layer (`hooks_config_folder` in codex's `config/src/state.rs`) + * — which is the same global/local split the TOML config already uses. + */ +function hooksJsonPath(loc: Location): string { + return path.join(configDir(loc), 'hooks.json'); +} + /** * Project layers are "loaded but disabled when untrusted" (openai/codex * `loader/mod.rs`), so a local install can be written correctly and @@ -79,6 +96,272 @@ function trustNote(): string { return `Codex applies ${tomlConfigPath('local')} only in a project marked trusted — otherwise the layer is loaded but disabled. Trust this project in Codex to activate it.`; } +/** + * The per-context session hooks, as codex spells them. + * + * `PreToolUse`'s matcher is a regex tested against the tool name, so the bare + * name matches however the server ends up namespaced. It declares `--agent + * codex` because codex requires `permissionDecision` paired with the rewrite + * and drops an unpaired one — the hook emits that field for no other agent, + * and learns which one it serves from this flag rather than from the payload. + * + * `PostCompact` — codex gates it behind compaction SUCCESS in three of its + * four compaction paths, and in all four every failure path returns before + * the history rewrite commits, so a failed compaction leaves the agent's + * context intact. Skipping the reset there is exactly right; PreCompact would + * clear a still-valid record on every failed attempt. It carries no matcher, + * which codex reads as every trigger — manual and auto alike. + */ +const SESSION_HOOKS: HookEntry[] = [ + { event: 'PreToolUse', matcher: 'codegraph_explore', subcommand: 'hooks pre-tool-use --agent codex' }, + { event: 'PostCompact', subcommand: 'hooks post-compact' }, +]; + +/** + * Parse a `hooks.json`, or `null` when it is unusable. + * + * Deliberately NOT `readJsonFile`: that one backs a broken file up and returns + * `{}`, which here would mean writing our hooks over config we failed to read. + * Codex would then run our file instead of the user's, so an unparseable + * hooks.json is left exactly as it is. + */ +function readHooksJson(file: string): Record | null { + if (!fs.existsSync(file)) return {}; + try { + const parsed = JSON.parse(fs.readFileSync(file, 'utf-8')); + return parsed && typeof parsed === 'object' && !Array.isArray(parsed) ? parsed : null; + } catch { + return null; + } +} + +/** + * Wire the session hooks into this location's `hooks.json`, creating the file + * when it doesn't exist. Surgical: existing events, groups and unrelated + * top-level keys are preserved, ours is appended, and a re-run that finds + * everything already in place leaves the bytes untouched. + * + * Codex holds hook trust per ENTRY, in `[hooks.state]` inside config.toml: each + * handler carries a hash of its own normalized identity. Writing that state + * ourselves would stamp our hooks as reviewed and skip the review the user is + * entitled to, so the installer never touches it — and adding entries leaves + * every hook the user already trusted trusted. Nothing here announces that: + * codex's own TUI surfaces untrusted hooks on its next run, so the install + * reports this file the way it reports every other one, as a file action. + */ +export function writeSessionHookEntries(loc: Location): WriteResult['files'][number] { + const file = hooksJsonPath(loc); + const created = !fs.existsSync(file); + const root = readHooksJson(file); + if (!root) { + console.warn(` Warning: ${file} is not valid JSON — leaving it untouched.`); + console.warn(' Fix the file and re-run "codegraph install" to wire the CodeGraph hooks.'); + return { path: file, action: 'unchanged' }; + } + + if (!root.hooks || typeof root.hooks !== 'object' || Array.isArray(root.hooks)) { + root.hooks = {}; + } + const changed = mergeHookEntries(root.hooks, SESSION_HOOKS, codegraphBinary()); + if (!changed && !created) return { path: file, action: 'unchanged' }; + + writeJsonFile(file, root); + return { path: file, action: created ? 'created' : 'updated' }; +} + +/** + * Codex's snake_case label for each event we wire. It spells the event both in + * a hook's trust identity and in its `[hooks.state]` key. + */ +const HOOK_EVENT_KEY_LABEL: Record = { + PreToolUse: 'pre_tool_use', + PostToolUse: 'post_tool_use', + PostCompact: 'post_compact', +}; + +/** What codex normalizes an unset command-hook `timeout` to, in seconds. */ +const CODEX_DEFAULT_HOOK_TIMEOUT_SEC = 600; + +/** Recursively key-sorted copy — codex canonicalizes before hashing. */ +function canonicalJson(value: unknown): unknown { + if (Array.isArray(value)) return value.map(canonicalJson); + if (value && typeof value === 'object') { + const source = value as Record; + return Object.fromEntries(Object.keys(source).sort().map((k) => [k, canonicalJson(source[k])])); + } + return value; +} + +/** + * Reproduce the trust hash codex computes for one handler. + * + * Codex hashes a NORMALIZED identity rather than the source text, so that the + * same hook expressed in config.toml and in hooks.json converges: the event's + * snake_case label, the group's matcher, and the one handler with its defaults + * filled in — an unset `timeout` becoming {@link CODEX_DEFAULT_HOOK_TIMEOUT_SEC} + * and `async` defaulting to false. That is serialized to TOML (which drops unset + * optionals), converted to JSON, key-sorted at every level, and SHA-256'd. + * + * Pinned against real trusted entries in the fixture test — this reimplements + * another project's internal, so the test is what says it still agrees. If codex + * changes its normalization the hash simply stops matching, and codex reports + * the entry as needing review and asks in its own TUI, exactly as it does for a + * hook nobody has trusted. A stale hash can never read as trusted. + */ +export function codexHookTrustHash( + event: string, + matcher: string | undefined, + command: string, + statusMessage?: string, +): string { + const handler: Record = { + type: 'command', + command, + async: false, + timeout: CODEX_DEFAULT_HOOK_TIMEOUT_SEC, + }; + // Unset optionals are absent, not empty — see the matcher note below. Ours + // never carry a statusMessage; the parameter exists so the fixture test can + // hash a real-world handler that does. + if (statusMessage !== undefined) handler.statusMessage = statusMessage; + const identity: Record = { + event_name: HOOK_EVENT_KEY_LABEL[event] ?? event, + hooks: [handler], + }; + // An absent matcher is absent from the identity, not an empty string: TOML + // has no null, so codex's own serialization drops the key entirely. + if (matcher !== undefined) identity.matcher = matcher; + const canonical = JSON.stringify(canonicalJson(identity)); + return `sha256:${createHash('sha256').update(canonical).digest('hex')}`; +} + +/** + * Locate our handlers in a written hooks.json and describe each the way codex + * will: its `[hooks.state]` key and its current trust hash. + * + * The key carries the handler's POSITION (`:::`), + * so the indices are read back off the file we just wrote rather than assumed — + * ours may sit after any number of the user's groups. + */ +function ourHookTrustState(file: string, root: Record): Array<{ key: string; hash: string }> { + const found: Array<{ key: string; hash: string }> = []; + for (const { event, subcommand } of SESSION_HOOKS) { + const groups: any[] = Array.isArray(root.hooks?.[event]) ? root.hooks[event] : []; + const stable = subcommand.split(' --')[0] ?? subcommand; + groups.forEach((group, groupIndex) => { + const handlers: any[] = group && Array.isArray(group.hooks) ? group.hooks : []; + handlers.forEach((handler, handlerIndex) => { + const command = handler?.command; + if (typeof command !== 'string') return; + if (!isSessionHookCommand(command) || !command.includes(stable)) return; + found.push({ + key: `${file}:${HOOK_EVENT_KEY_LABEL[event] ?? event}:${groupIndex}:${handlerIndex}`, + // Hash the matcher AS WRITTEN, which is what codex will read back. + hash: codexHookTrustHash(event, typeof group.matcher === 'string' ? group.matcher : undefined, command), + }); + }); + }); + } + return found; +} + +/** + * Record our two hooks as trusted in this layer's config.toml. + * + * This is the one place the installer writes `[hooks.state]`, and it is not + * gated on a question. Codex's TUI review guards hooks that ARRIVE from + * somewhere the user didn't choose — a cloned repo's project hooks.json, say — + * and an installer the user just invoked, writing its own two entries pointing + * at its own binary, is not that threat; the Claude target installs its hooks + * without asking for the same reason. A local install's state also still sits + * behind codex's project-trust gate, so nothing runs in a project the user + * hasn't trusted anyway. + * + * Trust is held PER ENTRY, so what is written can only ever be the entries we + * wrote ourselves, keyed by their own position — the user's hooks are + * unreachable from here, and their trust records are never read, rewritten, or + * invalidated by ours. + * + * The launcher path is part of the hashed identity and changes with every + * version, so an existing record is UPDATED rather than skipped; only its + * `trusted_hash` line is replaced, keeping any `enabled` the user set in the + * codex TUI (re-enabling a hook they turned off is not ours to do). + * + * ponytail: a record is keyed by POSITION, so if our group's index ever shifts + * — a user inserting a group ahead of ours in the same file — the record for + * the old position is left behind. Harmless (its hash matches nothing at the + * new position, so codex asks rather than trusting) but untidy; prune it here + * if that ever stops being rare, taking care not to delete a record the user + * made for a hook of their own in the same file. + */ +function writeHookTrustState(loc: Location, entries: Array<{ key: string; hash: string }>): boolean { + const file = tomlConfigPath(loc); + let content = fs.existsSync(file) ? fs.readFileSync(file, 'utf-8') : ''; + let changed = false; + + for (const { key, hash } of entries) { + const header = `hooks.state.${JSON.stringify(key)}`; + const existing = readTomlTableBody(content, header) ?? []; + const body = [ + `trusted_hash = ${JSON.stringify(hash)}`, + ...existing.filter((line) => !/^\s*trusted_hash\s*=/.test(line)), + ].join('\n'); + const { content: next, action } = upsertTomlTable(content, header, `[${header}]\n${body}`); + if (action !== 'unchanged') changed = true; + content = next; + } + + if (changed) atomicWriteFileSync(file, content); + return changed; +} + +/** Drop the trust records for our own hooks, and nothing else. */ +function removeHookTrustState(loc: Location, entries: Array<{ key: string; hash: string }>): boolean { + const file = tomlConfigPath(loc); + if (!fs.existsSync(file)) return false; + let content = fs.readFileSync(file, 'utf-8'); + let changed = false; + for (const { key } of entries) { + const { content: next, action } = removeTomlTable(content, `hooks.state.${JSON.stringify(key)}`); + if (action === 'removed') changed = true; + content = next; + } + if (changed) atomicWriteFileSync(file, content.trimEnd() + '\n'); + return changed; +} + +/** + * Remove the session hooks this installer wrote. Leaves the user's own hooks, + * and any other top-level key, alone; deletes the file only when removing ours + * empties it completely — i.e. only when we were the ones who created it. + */ +export function removeSessionHookEntries(loc: Location): WriteResult['files'][number] { + const file = hooksJsonPath(loc); + if (!fs.existsSync(file)) return { path: file, action: 'not-found' }; + const root = readHooksJson(file); + if (!root || !root.hooks || typeof root.hooks !== 'object' || Array.isArray(root.hooks)) { + return { path: file, action: 'unchanged' }; + } + if (!pruneHookCommands(root.hooks, isSessionHookCommand)) { + return { path: file, action: 'unchanged' }; + } + if (Object.keys(root.hooks).length === 0) delete root.hooks; + + if (Object.keys(root).length === 0) { + try { fs.unlinkSync(file); } catch { /* ignore */ } + } else { + writeJsonFile(file, root); + } + return { path: file, action: 'removed' }; +} + +/** Our hooks as they now sit on disk, keyed and hashed the way codex will. */ +function hookTrustEntries(loc: Location): Array<{ key: string; hash: string }> { + const file = hooksJsonPath(loc); + const root = readHooksJson(file); + return root ? ourHookTrustState(file, root) : []; +} + class CodexTarget implements AgentTarget { readonly id = 'codex' as const; readonly displayName = 'Codex CLI'; @@ -107,7 +390,19 @@ class CodexTarget implements AgentTarget { install(loc: Location, _opts: InstallOptions): WriteResult { const files: WriteResult['files'] = []; - files.push(writeMcpEntry(loc)); + const mcp = writeMcpEntry(loc); + + // Per-context session hooks. They are what tells the server WHICH agent + // context a call belongs to; without them its already-sent record stays + // keyed per MCP connection. Their trust records go in unconditionally — + // see writeHookTrustState — and the write no-ops once they are right. + const hooks = writeSessionHookEntries(loc); + const trustChanged = writeHookTrustState(loc, hookTrustEntries(loc)); + + // The MCP entry and the trust records share config.toml, so it is reported + // once, as whichever write actually touched it. + files.push(trustChanged && mcp.action === 'unchanged' ? { path: mcp.path, action: 'updated' } : mcp); + files.push(hooks); // AGENTS.md gets the short marker-fenced CodeGraph block (#704): // subagents and non-MCP harnesses read AGENTS.md but never the MCP @@ -120,6 +415,10 @@ class CodexTarget implements AgentTarget { uninstall(loc: Location): WriteResult { const files: WriteResult['files'] = []; + // Read the keys off hooks.json BEFORE it is stripped — they are derived + // from the handlers' positions in it. + removeHookTrustState(loc, hookTrustEntries(loc)); + const tomlPath = tomlConfigPath(loc); if (fs.existsSync(tomlPath)) { const content = fs.readFileSync(tomlPath, 'utf-8'); @@ -138,6 +437,9 @@ class CodexTarget implements AgentTarget { files.push({ path: tomlPath, action: 'not-found' }); } + const hooks = removeSessionHookEntries(loc); + if (hooks.action === 'removed') files.push(hooks); + files.push(removeInstructionsEntry(loc)); return { files }; @@ -149,7 +451,7 @@ class CodexTarget implements AgentTarget { } describePaths(loc: Location): string[] { - return [tomlConfigPath(loc), instructionsPath(loc)]; + return [tomlConfigPath(loc), hooksJsonPath(loc), instructionsPath(loc)]; } } diff --git a/src/installer/targets/toml.ts b/src/installer/targets/toml.ts index 1dc086bf3..eb6245d02 100644 --- a/src/installer/targets/toml.ts +++ b/src/installer/targets/toml.ts @@ -102,6 +102,26 @@ export function upsertTomlTable( }; } +/** + * The body lines of an existing dotted-key table, or `null` when the table + * isn't there. Uses the same lexer-backed block scan as upsert/remove, so a + * bracket inside a string value can't be mistaken for the next table. + * + * Lets a caller rewrite ONE key of a table it does not own outright, keeping + * every sibling key the user or another tool put there. + */ +export function readTomlTableBody(fileContent: string, header: string): string[] | null { + const headerLine = `[${header}]`; + const headerIdx = findHeaderIndex(fileContent, headerLine); + if (headerIdx === -1) return null; + const blockEnd = findNextTableHeader(fileContent, headerIdx + headerLine.length); + return fileContent + .substring(headerIdx + headerLine.length, blockEnd) + .split('\n') + .map((line) => line.trimEnd()) + .filter((line) => line.length > 0); +} + /** * Remove a top-level dotted-key TOML table block. Returns the * possibly-empty new content + an action flag.