From ebf8dd34a50eb2fccadbca89736991e397216be5 Mon Sep 17 00:00:00 2001 From: AgentSeal Date: Fri, 10 Jul 2026 12:11:59 +0200 Subject: [PATCH] fix(codex): attribute CLI-wrapped MCP calls (mcp-cli), and re-parse cached sessions (#478) Users who run MCP tools through a CLI wrapper (e.g. philschmid/mcp-cli) instead of registering servers natively don't produce Codex mcp_tool_call_end events; Codex logs a plain exec_command. So their MCP usage showed only as a shell command and was absent from the MCP breakdown, even after #513 fixed the native path. - Recognize `mcp-cli [options] call ` in the exec command and also attribute it as mcp____. The exec still counts as Bash since it genuinely is a shell exec. Flag-tolerant, quote/path/bash-lc tolerant, and scoped to the mcp-cli binary (not foo-mcp-cli); only the `call` subcommand (a real execution) matches, not info/grep/listing. - Register `codex` in PROVIDER_PARSE_VERSIONS. session-cache.json serves unchanged session files without invoking the provider parser, so the codex-cache version bump alone would never re-attribute already-cached sessions. This also retroactively repairs #513's native-MCP fix for users whose files were cached before it shipped. - Bump CODEX_CACHE_VERSION 4->5 for the provider-internal layer. Tested: parse cases (bash -lc wrapper, flags-before-call, argv array, plus info/grep/foo-mcp-cli negatives), and a mutation-verified cache regression test that fails without the PROVIDER_PARSE_VERSIONS entry. ReDoS-checked. Reviewed with a Fable 5 adversarial pass (Codex is down); its two must-fixes (the cache gap and the flags-before-call miss that would have missed the reporter's own sessions) are folded in. Fixes #478 --- src/codex-cache.ts | 5 +- src/providers/codex.ts | 41 ++++++++++ src/session-cache.ts | 5 ++ tests/codex-cache-invalidation.test.ts | 108 +++++++++++++++++++++++++ tests/providers/codex.test.ts | 45 +++++++++++ 5 files changed, 203 insertions(+), 1 deletion(-) create mode 100644 tests/codex-cache-invalidation.test.ts diff --git a/src/codex-cache.ts b/src/codex-cache.ts index 9cc61842..b2499c8c 100644 --- a/src/codex-cache.ts +++ b/src/codex-cache.ts @@ -8,7 +8,10 @@ import type { ParsedProviderCall } from './providers/types.js' // v4: attribute MCP calls emitted as event_msg/mcp_tool_call_end (issue #478). // Recent Codex sessions cached under v3 dropped these, so force a re-parse. -const CODEX_CACHE_VERSION = 4 +// v5: also attribute CLI-wrapped MCP calls (`mcp-cli call server tool`) that +// Codex logs as a plain exec_command (issue #478 follow-up). Force a re-parse +// so sessions cached under v4 pick up the CLI-MCP attribution. +const CODEX_CACHE_VERSION = 5 const CACHE_FILE = 'codex-results.json' type FileFingerprint = { mtimeMs: number; sizeBytes: number } diff --git a/src/providers/codex.ts b/src/providers/codex.ts index 8cadf0d8..0917f66f 100644 --- a/src/providers/codex.ts +++ b/src/providers/codex.ts @@ -41,6 +41,40 @@ const toolNameMap: Record = { read_dir: 'Glob', } +// CLI-based MCP wrappers (e.g. philschmid/mcp-cli) let Codex call an MCP tool +// through a shell command instead of registering the server natively. Codex +// then logs a plain exec_command with no `mcp_tool_call_end` event, so the MCP +// usage would only appear as a shell command and be absent from the MCP +// breakdown (issue #478). Recognize the `mcp-cli [options] call +// ` form and return the canonical mcp____ so the call is +// also attributed to MCP. Only the `call` subcommand (an actual tool execution) +// is matched; info / grep / bare listing are lookups. The exec_command still +// counts as Bash since it genuinely is a shell exec. Scoped to the mcp-cli +// binary; other wrappers would need their own pattern. +// +// The negative lookbehind keeps `mcp-cli` a standalone binary (a leading +// quote/space/slash from a `bash -lc "..."` wrapper or absolute path is fine, +// but `foo-mcp-cli` is not). `(?:\s+(?!call\b)[^\s;|&]+)*` skips any options and +// their values between the binary and the subcommand (e.g. +// `mcp-cli -c ./mcp.json call ...`) without crossing a shell separator, and +// stops at the `call` token. This is substring matching, so a command that +// merely mentions the phrase (a comment, an echo, a commit message) can +// false-positive, an accepted tradeoff for the common case. \s+ and the token +// class don't overlap, so there is no catastrophic backtracking. +const MCP_CLI_CALL = /(? typeof x === 'string').join(' ') : '' + if (!text) return null + const m = MCP_CLI_CALL.exec(text) + if (!m) return null + const server = m[1]!.replace(/['"]/g, '') + const tool = m[2]!.replace(/['"]/g, '') + if (!server || !tool) return null + return `mcp__${server}__${tool}` +} + type CodexEntry = { type: string timestamp?: string @@ -386,6 +420,13 @@ function createParser(source: SessionSource, seenKeys: Set): SessionPars if (typeof fp === 'string') call.file = fp const cmd = args['command'] ?? args['cmd'] if (typeof cmd === 'string') call.command = cmd + // Attribute a CLI-wrapped MCP call (e.g. `mcp-cli call server tool`) + // to the MCP breakdown too; the exec still counts as Bash above. + const mcpTool = mcpToolFromShellCommand(cmd) + if (mcpTool) { + pendingTools.push(mcpTool) + pendingToolSequence.push([{ tool: mcpTool }]) + } } pendingToolSequence.push([call]) continue diff --git a/src/session-cache.ts b/src/session-cache.ts index 0193b73e..7d02e9cd 100644 --- a/src/session-cache.ts +++ b/src/session-cache.ts @@ -111,6 +111,11 @@ export const DURABLE_PROVIDER_NAMES: ReadonlySet = new Set(['copilot']) const PROVIDER_PARSE_VERSIONS: Record = { claude: 'cowork-space-grouping-v1', cline: 'worktree-project-grouping-v1', + // Bump when the Codex parser changes attribution so unchanged, already-cached + // session files re-parse (session-cache.json serves them without invoking the + // provider parser otherwise). Covers native mcp_tool_call_end (#513) and + // CLI-wrapped `mcp-cli call` (#478) MCP attribution. + codex: 'mcp-attribution-v2', cursor: 'composer-anchored-crediting-v1', 'cursor-agent': 'workspaceless-transcript-v1', copilot: 'otel-durable-v1', diff --git a/tests/codex-cache-invalidation.test.ts b/tests/codex-cache-invalidation.test.ts new file mode 100644 index 00000000..cfba9c45 --- /dev/null +++ b/tests/codex-cache-invalidation.test.ts @@ -0,0 +1,108 @@ +// Regression for the codex stale-cache path (#478 follow-up, same class as +// the kiro bug #618/#619). session-cache.json serves unchanged session files +// without invoking the provider parser, so bumping CODEX_CACHE_VERSION alone +// does NOT re-attribute already-cached sessions. Registering `codex` in +// PROVIDER_PARSE_VERSIONS changes the provider envFingerprint, which discards +// the stale section and forces a re-parse. This exercises the full +// parseAllSessions pipeline against a cache seeded with the PRE-fix +// fingerprint and asserts the mcp-cli MCP attribution is recovered. + +import { describe, it, expect, beforeEach, afterAll, vi } from 'vitest' +import { mkdir, rm, readFile, writeFile } from 'fs/promises' +import { createHash } from 'crypto' +import { join } from 'path' + +import { clearSessionCache, parseAllSessions } from '../src/parser.js' + +const testRoot = vi.hoisted(() => { + const root = `${process.env['TMPDIR'] || '/tmp'}/codex-stale-repro-${process.pid}-${Date.now()}` + process.env['HOME'] = `${root}/home` + process.env['USERPROFILE'] = `${root}/home` + process.env['CODEX_HOME'] = `${root}/codex` + return root +}) + +const CODEX_HOME = join(testRoot, 'codex') +const CACHE_DIR = join(testRoot, 'cache') + +// computeEnvFingerprint('codex') as staged (no PROVIDER_PARSE_VERSIONS entry): +// hash of just CODEX_HOME. This is what sits in every existing user cache. +function preFixFingerprint(): string { + return createHash('sha256').update(`CODEX_HOME=${CODEX_HOME}`).digest('hex').slice(0, 16) +} + +beforeEach(() => { + process.env['HOME'] = join(testRoot, 'home') + process.env['USERPROFILE'] = join(testRoot, 'home') + process.env['CODEX_HOME'] = CODEX_HOME + process.env['CODEBURN_CACHE_DIR'] = CACHE_DIR +}) + +afterAll(async () => { + await rm(testRoot, { recursive: true, force: true }) +}) + +function allMcpServers(projects: Awaited>): string[] { + const servers: string[] = [] + for (const p of projects) { + for (const s of p.sessions) { + servers.push(...Object.keys(s.mcpBreakdown)) + } + } + return servers +} + +describe('codex parser change invalidates stale session-cache (#478/#513)', () => { + it('re-parses unchanged codex files after a parser attribution change', async () => { + const sessionDir = join(CODEX_HOME, 'sessions', '2026', '04', '14') + await mkdir(sessionDir, { recursive: true }) + await mkdir(CACHE_DIR, { recursive: true }) + const lines = [ + JSON.stringify({ type: 'session_meta', timestamp: '2026-04-14T10:00:00Z', payload: { session_id: 'sess-stale', model: 'gpt-5.5', cwd: '/Users/test/proj', originator: 'codex_cli_rs' } }), + JSON.stringify({ type: 'response_item', timestamp: '2026-04-14T10:00:10Z', payload: { type: 'message', role: 'user', content: [{ type: 'input_text', text: 'call mcp via cli' }] } }), + JSON.stringify({ type: 'response_item', timestamp: '2026-04-14T10:00:30Z', payload: { type: 'function_call', name: 'exec_command', arguments: JSON.stringify({ command: "bash -lc \"mcp-cli call github get_issue '{}'\"" }) } }), + JSON.stringify({ type: 'event_msg', timestamp: '2026-04-14T10:01:00Z', payload: { type: 'token_count', info: { last_token_usage: { input_tokens: 300, output_tokens: 100 }, total_token_usage: { total_tokens: 400 } } } }), + ] + await writeFile(join(sessionDir, 'rollout-stale.jsonl'), lines.join('\n') + '\n') + + // Run 1: cold cache, fixed parser. MCP attribution present (sanity). + clearSessionCache() + const fresh = await parseAllSessions(undefined, 'codex') + expect(allMcpServers(fresh)).toContain('github') + + // Simulate a user whose session-cache.json was written by the PRE-fix + // release: pre-fix envFingerprint, unchanged file fingerprint, cached + // turns lack the mcp__ tool. Also reset codex-results.json to v4 so the + // provider (if it runs at all) must genuinely re-parse. + const cachePath = join(CACHE_DIR, 'session-cache.json') + const cache = JSON.parse(await readFile(cachePath, 'utf8')) + cache.providers.codex.envFingerprint = preFixFingerprint() + for (const f of Object.values(cache.providers.codex.files) as any[]) { + for (const turn of f.turns) { + for (const call of turn.calls) { + call.tools = call.tools.filter((t: string) => !t.startsWith('mcp__')) + if (call.toolSequence) { + call.toolSequence = call.toolSequence.filter((step: any[]) => !step.some(c => c.tool.startsWith('mcp__'))) + } + } + } + } + await writeFile(cachePath, JSON.stringify(cache)) + const codexCachePath = join(CACHE_DIR, 'codex-results.json') + const codexCache = JSON.parse(await readFile(codexCachePath, 'utf8')) + codexCache.version = 4 + for (const f of Object.values(codexCache.files) as any[]) { + for (const call of f.calls ?? []) { + call.tools = (call.tools ?? []).filter((t: string) => !t.startsWith('mcp__')) + } + } + await writeFile(codexCachePath, JSON.stringify(codexCache)) + + clearSessionCache() + const second = await parseAllSessions(undefined, 'codex') + // FIXED: `codex` is now in PROVIDER_PARSE_VERSIONS, so the pre-fix + // envFingerprint no longer matches, the stale section is discarded, the + // unchanged file re-parses, and the mcp-cli attribution reappears. + expect(allMcpServers(second)).toContain('github') + }) +}) diff --git a/tests/providers/codex.test.ts b/tests/providers/codex.test.ts index 958139f8..86f1176a 100644 --- a/tests/providers/codex.test.ts +++ b/tests/providers/codex.test.ts @@ -332,6 +332,51 @@ describe('codex provider - JSONL parsing', () => { expect(calls[0]!.tools).toEqual(['mcp__github__get_issue']) }) + it('attributes CLI-wrapped MCP calls (mcp-cli call server tool) to MCP + Bash', async () => { + const execStr = (command: string) => JSON.stringify({ + type: 'response_item', + timestamp: '2026-04-14T10:00:30Z', + payload: { type: 'function_call', name: 'exec_command', arguments: JSON.stringify({ command }) }, + }) + // command as an array (Codex sometimes logs argv form). + const execArr = (command: string[]) => JSON.stringify({ + type: 'response_item', + timestamp: '2026-04-14T10:00:30Z', + payload: { type: 'function_call', name: 'exec_command', arguments: JSON.stringify({ command }) }, + }) + const filePath = await writeSession(tmpDir, '2026-04-14', 'rollout-mcpcli.jsonl', [ + sessionMeta({ session_id: 'sess-mcpcli', model: 'gpt-5.5' }), + userMessage('look up an issue via the MCP CLI'), + // Real invocation forms that MUST attribute to MCP: + execStr("bash -lc \"mcp-cli call github get_issue '{\\\"id\\\": 5}'\""), // bash -lc wrapper + execStr('mcp-cli -c ./mcp.json call linear list_issues'), // flags before subcommand + execArr(['mcp-cli', 'call', 'slack', 'post_message', '{}']), // argv array form + // Lookups and unrelated commands that must NOT attribute: + execStr('mcp-cli info github'), + execStr('mcp-cli grep "*issue*"'), + execStr('my-mcp-cli-wrapper call github get_issue'), // not the mcp-cli binary + execStr('ls -la'), + tokenCount({ timestamp: '2026-04-14T10:01:00Z', last: { input: 300, output: 100 }, total: { total: 400 } }), + ]) + + const provider = createCodexProvider(tmpDir) + const source = { path: filePath, project: 'test', provider: 'codex' } + const parser = provider.createSessionParser(source, new Set()) + const calls: ParsedProviderCall[] = [] + for await (const call of parser.parse()) calls.push(call) + + expect(calls).toHaveLength(1) + const tools = calls[0]!.tools + // Every exec still counts as Bash (7 exec_commands total). + expect(tools.filter(t => t === 'Bash')).toHaveLength(7) + // Exactly the three `call` invocations attribute to MCP; info/grep/wrapper/ls do not. + expect(tools.filter(t => t.startsWith('mcp__')).sort()).toEqual([ + 'mcp__github__get_issue', + 'mcp__linear__list_issues', + 'mcp__slack__post_message', + ]) + }) + it('normalizes Codex subagent tool calls to Agent', async () => { const filePath = await writeSession(tmpDir, '2026-04-14', 'rollout-agent.jsonl', [ sessionMeta({ session_id: 'sess-agent', model: 'gpt-5.5' }),