From eece4cf005873149f64d53d99a2b29c280b8b6ec Mon Sep 17 00:00:00 2001 From: ozymandiashh <234437643+ozymandiashh@users.noreply.github.com> Date: Mon, 3 Aug 2026 23:23:12 +0300 Subject: [PATCH] fix(codex): validate rollouts structurally instead of by originator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex session discovery required `payload.originator` to start with "codex" (case-insensitive). `originator` is a free-form client identity string, not a format marker: any tool driving `codex app-server` writes structurally identical rollouts under ~/.codex/sessions with its own value ("t3code_desktop", "JetBrains.IntelliJ IDEA", ...). Those sessions were silently dropped from every report, and each past fix only admitted one more spelling. Gate on structure instead: a first line that parses as JSON, has type === "session_meta", and carries a plain-object payload. Foreign and malformed files are still rejected. Directory ownership decides the provider — codex.ts is the only provider that reads ~/.codex, and the walk only visits rollout-*.jsonl under the strict YYYY/MM/DD path or archived_sessions/ — so no double counting is possible. `originator` is still parsed onto the meta entry; nothing downstream reads it. Bump the daily cache to v16. Historical days are served from that cache (usage-aggregator only recomputes today) and retention is ten years, so without a bump an upgrading user with a warm cache keeps the pre-fix rollups forever: discovery reruns, so the session COUNT moves, while cost and calls stay frozen — a self-contradicting report that reads as "fixed". Measured on a fixture with two same-day rollouts, one codex-cli and one t3code_desktop: pristine main, fresh cache cost 4.55 calls 1 sessions 1 this branch, main's warm cache cost 4.55 calls 1 sessions 2 (was) this branch, main's warm cache cost 18.2 calls 2 sessions 2 (now) this branch, fresh cache cost 18.2 calls 2 sessions 2 (truth) CODEX_CACHE_VERSION and PROVIDER_PARSE_VERSIONS.codex deliberately stay put: both caches are keyed per file path and are written only after a successful parse, so a file rejected at discovery has no entry to invalidate. Verified on the fixture above — main's codex-results.json and session-cache.v7.json hold only the first-party rollout, and reusing them unchanged still yields the correct total. Harden `payload.cwd` while admitting unverified clients. It is declared `string` but comes straight off JSON.parse, and a number/object/array threw "cwd.replace is not a function" out of sanitizeProject; the throw escaped discoverSessions into safeDiscoverSessions, which returns [] for the WHOLE provider, so one malformed file made every Codex report read zero. Guarded in discovery (falls back to the `unknown` project) and on the parse side, where a non-string cwd would otherwise ride into projectPath/workingDirectory and reach the parser's path helpers. Closes #873, closes #626. --- docs/providers/codex.md | 8 +- src/daily-cache.ts | 16 ++- src/providers/codex.ts | 38 +++++++- tests/daily-cache.test.ts | 76 +++++++++++++++ tests/providers/codex.test.ts | 177 ++++++++++++++++++++++++++++++++++ 5 files changed, 306 insertions(+), 9 deletions(-) diff --git a/docs/providers/codex.md b/docs/providers/codex.md index b68847d..1ba46fd 100644 --- a/docs/providers/codex.md +++ b/docs/providers/codex.md @@ -4,7 +4,7 @@ OpenAI Codex CLI. - **Source:** `src/providers/codex.ts` - **Loading:** eager (`src/providers/index.ts:2`) -- **Test:** `tests/providers/codex.test.ts` (374 lines) +- **Test:** `tests/providers/codex.test.ts` (1075 lines) ## Where it reads from @@ -24,7 +24,11 @@ The active-session discovery walk uses strict regex (`^\d{4}$`, `^\d{2}$`) on ea ## Storage format -JSONL. The first line must be a `session_meta` entry with `payload.originator` starting with `codex` (case-insensitive). Files that fail this check are silently skipped. +JSONL. Validation of the first line is **structural**: it must parse as JSON, have `type === "session_meta"`, and carry a `payload` that is a plain object (not missing, not a scalar, not an array). Files that fail this check are silently skipped. + +`payload.originator` is deliberately **not** part of the check. It is a free-form client identity string, not a format marker: Codex CLI writes `codex-tui` / `codex_exec` / `codex_cli_rs`, Codex Desktop writes `Codex Desktop`, and third-party frontends driving `codex app-server` write their own values (`t3code_desktop`, `JetBrains.IntelliJ IDEA`, ...) into structurally identical rollouts. Gating discovery on the spelling silently dropped those sessions from every report and required a new allowlist entry per client (issues #626, #873). Directory ownership decides the provider instead: `codex.ts` is the only provider that reads `~/.codex`, and the walk only visits `rollout-*.jsonl` under the strict `YYYY/MM/DD` path or `archived_sessions/`. `originator` is still parsed into the session meta entry, but nothing downstream reads it. + +Because admission no longer implies a known client, every payload field is treated as untrusted JSON. `payload.cwd` in particular is type-guarded before it reaches `sanitizeProject` (discovery) or `projectPath`/`workingDirectory` (parse): a non-string `cwd` falls back to the `unknown` project instead of throwing out of `discoverSessions`, which `safeDiscoverSessions` would have turned into an empty session list for the *entire* provider. The first line read is capped at 1 MB (`FIRST_LINE_READ_CAP`). Codex CLI 0.128+ embeds the full system prompt in `session_meta`, which can run 20-27 KB; the cap leaves headroom while bounding memory if a corrupt file has no newline. diff --git a/src/daily-cache.ts b/src/daily-cache.ts index 7a7486c..1c6bf57 100644 --- a/src/daily-cache.ts +++ b/src/daily-cache.ts @@ -5,7 +5,17 @@ import { homedir } from 'os' import { join } from 'path' import type { DateRange, ProjectSummary } from './types.js' -// Bumped to 15: per-project daily rollups. Days and provider slices now carry +// Bumped to 16: Codex discovery is structural instead of originator-gated +// (#873/#626), so rollouts written by third-party frontends driving +// `codex app-server` ("t3code_desktop", "JetBrains.IntelliJ IDEA", ...) now +// contribute usage that v15 rollups never contained. Those files were rejected +// before they were ever parsed, so nothing downstream can notice on its own: +// `usage-aggregator` serves every day before today from this cache, and +// retention is ten years, so an upgrading user with a warm cache would keep the +// pre-fix history forever while today's numbers silently disagreed with it. +// Raising MIN_SUPPORTED_VERSION forces the one-time re-derivation. +// +// v15: per-project daily rollups. Days and provider slices now carry // a `projects` breakdown (cost/calls/savings/sessions per project) so project // history outlives the session files, like models and categories already do. // This bump is the first to ride the v14 carry-forward: the old cache is @@ -57,8 +67,8 @@ import type { DateRange, ProjectSummary } from './types.js' // that older binaries skipped. v8 added local-model savings to the daily // rollup; the `savingsConfigHash` field is invalidated separately when the // user changes their `localModelSavings` mapping. -export const DAILY_CACHE_VERSION = 15 -const MIN_SUPPORTED_VERSION = 15 +export const DAILY_CACHE_VERSION = 16 +const MIN_SUPPORTED_VERSION = 16 // Version-suffixed so different binaries each own a distinct file and never // clobber an incompatible schema. Bumping the version mints a fresh filename; // adoptOlderDailyCaches then unions days out of every previous file (including diff --git a/src/providers/codex.ts b/src/providers/codex.ts index faa8fcf..8b7b46d 100644 --- a/src/providers/codex.ts +++ b/src/providers/codex.ts @@ -185,12 +185,30 @@ async function readFirstLine(filePath: string): Promise { } } +// Validation is STRUCTURAL, never string-matching on `payload.originator`. +// `originator` is a free-form CLIENT IDENTITY string, not a format marker: any +// tool driving `codex app-server` writes structurally identical rollouts under +// ~/.codex/sessions with its own value ("codex-tui", "Codex Desktop", +// "t3code_desktop", "JetBrains.IntelliJ IDEA", ...). Gating on the spelling +// silently dropped every third-party frontend and needed a new allowlist entry +// per client (issues #626, #873). +// +// A `session_meta` first line with a well-formed payload object is signal +// enough: the walker only visits `rollout-*.jsonl` under the strict +// YYYY/MM/DD path or `archived_sessions/`, and codex.ts is the only provider +// that reads ~/.codex, so directory ownership — not originator content — +// decides the provider. Genuinely foreign files (wrong entry type, missing or +// non-object payload, malformed JSON) are still rejected. async function isValidCodexSession(filePath: string): Promise<{ valid: boolean; meta?: CodexEntry }> { const entry = await readFirstLine(filePath) if (!entry) return { valid: false } + // `entry` comes from an unchecked JSON.parse cast, so re-check the payload + // shape at runtime instead of trusting the declared type. + const payload: unknown = entry.payload const valid = entry.type === 'session_meta' && - typeof entry.payload?.originator === 'string' && - entry.payload.originator.toLowerCase().startsWith('codex') + typeof payload === 'object' && + payload !== null && + !Array.isArray(payload) return { valid, meta: valid ? entry : undefined } } @@ -471,7 +489,13 @@ async function discoverSessionFile(filePath: string): Promise): SessionPars if (entry.type === 'session_meta') { sessionId = entry.payload?.session_id ?? basename(source.path, '.jsonl') - sessionCwd = entry.payload?.cwd ?? sessionCwd + // Guarded for the same reason as discoverSessionFile: parseCodexLine + // hands back an unchecked JSON.parse cast, and a non-string cwd would + // ride into projectPath/workingDirectory where the parser's path + // helpers (normalizeProjectPathKey, resolveCanonicalProjectPath) call + // string methods on it. + const rawSessionCwd: unknown = entry.payload?.cwd + if (typeof rawSessionCwd === 'string' && rawSessionCwd) sessionCwd = rawSessionCwd forkedFromId = entry.payload?.forked_from_id ?? '' if (forkedFromId && entry.timestamp) { forkCutoff = new Date(new Date(entry.timestamp).getTime() + 5000).toISOString() diff --git a/tests/daily-cache.test.ts b/tests/daily-cache.test.ts index d887d25..1c377ef 100644 --- a/tests/daily-cache.test.ts +++ b/tests/daily-cache.test.ts @@ -366,6 +366,82 @@ describe('ensureCacheHydrated', () => { }) }) +// Codex discovery went structural in v16 (#873/#626), admitting rollouts from +// third-party frontends that v15 rollups never counted. Every historical day is +// served from this cache (usage-aggregator only recomputes today) and retention +// is ten years, so without a schema bump an upgrading user keeps the pre-fix +// numbers forever: the session COUNT moves because discovery reruns, while +// cost/calls stay frozen — a self-contradicting report that reads as "fixed". +describe('ensureCacheHydrated: schema version invalidation (#873)', () => { + it('re-derives a warm v15 cache instead of serving its pre-fix rollups', async () => { + vi.useFakeTimers() + vi.setSystemTime(new Date('2026-06-12T12:00:00.000Z')) + + const { writeFile, mkdir } = await import('fs/promises') + await mkdir(TMP_CACHE_ROOT, { recursive: true }) + // A cache exactly as a pre-fix release left it: current schema at the time, + // finalized off a complete parse, watermark at yesterday, matching tz. + // Nothing but the version bump can invalidate it. + const v15 = { + version: 15, + savingsConfigHash: '', + tzKey: currentTzKey(), + lastComputedDate: '2026-06-11', + days: [emptyDay('2026-06-11', 4.55, 1)], + complete: true, + watermarkTrusted: true, + } + await writeFile(join(TMP_CACHE_ROOT, 'daily-cache.v15.json'), JSON.stringify(v15), 'utf-8') + + let parseCalls = 0 + const hydrated = await ensureCacheHydrated( + async () => { + parseCalls += 1 + return [] + }, + () => [emptyDay('2026-06-11', 18.2, 2)], + ) + + // The whole point: the window is re-parsed rather than served frozen. + expect(parseCalls).toBe(1) + // ...and the fresh derivation wins over the stale v15 day. + expect(hydrated.days.find(d => d.date === '2026-06-11')?.cost).toBe(18.2) + expect(hydrated.days.find(d => d.date === '2026-06-11')?.calls).toBe(2) + expect(hydrated.version).toBe(DAILY_CACHE_VERSION) + // The v15 file is never rewritten or deleted — old binaries still own it. + expect(JSON.parse(await readFile(join(TMP_CACHE_ROOT, 'daily-cache.v15.json'), 'utf-8')).version).toBe(15) + }) + + it('carries a v15 day forward when its sources can no longer re-derive it', async () => { + vi.useFakeTimers() + vi.setSystemTime(new Date('2026-06-12T12:00:00.000Z')) + + const { writeFile, mkdir } = await import('fs/promises') + await mkdir(TMP_CACHE_ROOT, { recursive: true }) + const v15 = { + version: 15, + savingsConfigHash: '', + tzKey: currentTzKey(), + lastComputedDate: '2026-06-11', + days: [emptyDay('2026-04-02', 7, 3), emptyDay('2026-06-11', 4.55, 1)], + complete: true, + watermarkTrusted: true, + } + await writeFile(join(TMP_CACHE_ROOT, 'daily-cache.v15.json'), JSON.stringify(v15), 'utf-8') + + // The parse can only still see the recent day; April's sources are gone. + const hydrated = await ensureCacheHydrated( + async () => [], + () => [emptyDay('2026-06-11', 18.2, 2)], + ) + + // NEVER-LOSE (v14) still holds across this bump: the sourceless day keeps + // its old accounting rather than being dropped or zeroed. + expect(hydrated.days.find(d => d.date === '2026-04-02')?.cost).toBe(7) + expect(hydrated.days.find(d => d.date === '2026-06-11')?.cost).toBe(18.2) + }) +}) + describe('withDailyCacheLock', () => { it('serializes concurrent operations', async () => { const sequence: string[] = [] diff --git a/tests/providers/codex.test.ts b/tests/providers/codex.test.ts index dba279d..1307d4f 100644 --- a/tests/providers/codex.test.ts +++ b/tests/providers/codex.test.ts @@ -227,6 +227,183 @@ describe('codex provider - session discovery', () => { expect(sessions).toHaveLength(1) }) + it('accepts a third-party frontend originator (t3code_desktop)', async () => { + // Any client driving `codex app-server` writes structurally identical + // rollouts under ~/.codex/sessions with its own originator string. + // Discovery must be structural, not a per-client allowlist (issue #873). + await writeSession(tmpDir, '2026-04-14', 'rollout-t3code.jsonl', [ + sessionMeta({ originator: 't3code_desktop', session_id: 'sess-t3code', cwd: '/Users/test/t3code' }), + tokenCount({ last: { input: 100, output: 50 }, total: { total: 150 } }), + ]) + + const provider = createCodexProvider(tmpDir) + const sessions = await provider.discoverSessions() + expect(sessions).toHaveLength(1) + expect(sessions[0]!.path).toContain('rollout-t3code.jsonl') + expect(sessions[0]!.project).toBe('Users-test-t3code') + }) + + it('accepts the JetBrains plugin originator (issue #626)', async () => { + await writeSession(tmpDir, '2026-04-14', 'rollout-jetbrains.jsonl', [ + sessionMeta({ originator: 'JetBrains.IntelliJ IDEA', session_id: 'sess-jb', cwd: '/Users/test/jb' }), + tokenCount({ last: { input: 100, output: 50 }, total: { total: 150 } }), + ]) + + const provider = createCodexProvider(tmpDir) + const sessions = await provider.discoverSessions() + expect(sessions).toHaveLength(1) + expect(sessions[0]!.path).toContain('rollout-jetbrains.jsonl') + expect(sessions[0]!.project).toBe('Users-test-jb') + }) + + it('accepts a rollout with no originator field at all', async () => { + // Proves the gate is structural rather than string-matching: a rollout that + // omits `originator` entirely is still a valid Codex session. + const [year, month, day] = '2026-04-14'.split('-') + const sessionDir = join(tmpDir, 'sessions', year!, month!, day!) + await mkdir(sessionDir, { recursive: true }) + await writeFile( + join(sessionDir, 'rollout-no-originator.jsonl'), + JSON.stringify({ + type: 'session_meta', + timestamp: '2026-04-14T10:00:00Z', + payload: { + cwd: '/Users/test/anon', + session_id: 'sess-anon', + model: 'gpt-5.5', + }, + }) + '\n' + + tokenCount({ last: { input: 100, output: 50 }, total: { total: 150 } }) + '\n', + ) + + const provider = createCodexProvider(tmpDir) + const sessions = await provider.discoverSessions() + expect(sessions).toHaveLength(1) + expect(sessions[0]!.project).toBe('Users-test-anon') + }) + + it('accepts an archived rollout from a third-party frontend', async () => { + await writeArchivedSession(tmpDir, 'rollout-archived-t3code.jsonl', [ + sessionMeta({ originator: 't3code_desktop', session_id: 'sess-arch-t3', cwd: '/Users/test/arch' }), + tokenCount({ last: { input: 100, output: 50 }, total: { total: 150 } }), + ]) + + const provider = createCodexProvider(tmpDir) + const sessions = await provider.discoverSessions() + expect(sessions).toHaveLength(1) + expect(sessions[0]!.project).toBe('Users-test-arch') + }) + + it('still rejects foreign and malformed first lines regardless of originator', async () => { + const [year, month, day] = '2026-04-14'.split('-') + const sessionDir = join(tmpDir, 'sessions', year!, month!, day!) + await mkdir(sessionDir, { recursive: true }) + // Wrong entry type, even with a codex-looking originator. + await writeFile( + join(sessionDir, 'rollout-wrong-type.jsonl'), + JSON.stringify({ type: 'other', payload: { originator: 'codex-cli', cwd: '/x' } }) + '\n', + ) + // session_meta with no payload at all. + await writeFile( + join(sessionDir, 'rollout-no-payload.jsonl'), + JSON.stringify({ type: 'session_meta', timestamp: '2026-04-14T10:00:00Z' }) + '\n', + ) + // session_meta with a non-object payload. + await writeFile( + join(sessionDir, 'rollout-scalar-payload.jsonl'), + JSON.stringify({ type: 'session_meta', payload: 'codex-cli' }) + '\n', + ) + // session_meta with an array payload. + await writeFile( + join(sessionDir, 'rollout-array-payload.jsonl'), + JSON.stringify({ type: 'session_meta', payload: [] }) + '\n', + ) + // Not JSON at all. + await writeFile(join(sessionDir, 'rollout-not-json.jsonl'), 'not json at all\n') + + const provider = createCodexProvider(tmpDir) + const sessions = await provider.discoverSessions() + expect(sessions).toEqual([]) + }) + + it('survives a non-string cwd instead of zeroing out the whole provider', async () => { + // Structural discovery admits rollouts from clients whose schema conformance + // is unverified, so a payload field can hold anything JSON can express. + // `cwd` is declared `string` but reaches sanitizeProject straight off + // JSON.parse: a number/object/array/bool used to throw + // "cwd.replace is not a function", escape discoverSessions, and get caught + // by safeDiscoverSessions — which returns [] for the ENTIRE codex provider, + // so one malformed file made every Codex report read zero. + const [year, month, day] = '2026-04-14'.split('-') + const sessionDir = join(tmpDir, 'sessions', year!, month!, day!) + await mkdir(sessionDir, { recursive: true }) + const badCwds: Array<[string, unknown]> = [ + ['number', 123], + ['object', { path: '/Users/test/obj' }], + ['array', ['/Users/test/arr']], + ['bool', true], + ['null', null], + ['empty', ''], + ] + for (const [label, cwd] of badCwds) { + await writeFile( + join(sessionDir, `rollout-badcwd-${label}.jsonl`), + JSON.stringify({ + type: 'session_meta', + timestamp: '2026-04-14T10:00:00Z', + payload: { cwd, session_id: `sess-${label}`, originator: 'codex-cli' }, + }) + '\n' + + tokenCount({ last: { input: 100, output: 50 }, total: { total: 150 } }) + '\n', + ) + } + // A healthy sibling: proves the provider is not zeroed out by the bad ones. + await writeSession(tmpDir, '2026-04-14', 'rollout-good.jsonl', [ + sessionMeta({ cwd: '/Users/test/good', session_id: 'sess-good' }), + tokenCount({ last: { input: 100, output: 50 }, total: { total: 150 } }), + ]) + + const provider = createCodexProvider(tmpDir) + const sessions = await provider.discoverSessions() + + expect(sessions).toHaveLength(badCwds.length + 1) + for (const s of sessions) expect(typeof s.project).toBe('string') + const byName = new Map(sessions.map(s => [s.path.split('/').pop()!, s.project])) + for (const [label] of badCwds) { + expect(byName.get(`rollout-badcwd-${label}.jsonl`)).toBe('unknown') + } + expect(byName.get('rollout-good.jsonl')).toBe('Users-test-good') + }) + + it('does not leak a non-string cwd into projectPath/workingDirectory', async () => { + // Same unchecked cast on the parse side: sessionCwd feeds projectPath and + // workingDirectory, which the parser's path helpers call string methods on. + const [year, month, day] = '2026-04-14'.split('-') + const sessionDir = join(tmpDir, 'sessions', year!, month!, day!) + await mkdir(sessionDir, { recursive: true }) + await writeFile( + join(sessionDir, 'rollout-badcwd-parse.jsonl'), + JSON.stringify({ + type: 'session_meta', + timestamp: '2026-04-14T10:00:00Z', + payload: { cwd: 123, session_id: 'sess-badcwd', model: 'gpt-5.5', originator: 'codex-cli' }, + }) + '\n' + + tokenCount({ last: { input: 100, output: 50 }, total: { total: 150 } }) + '\n', + ) + + const provider = createCodexProvider(tmpDir) + const sessions = await provider.discoverSessions() + expect(sessions).toHaveLength(1) + + const calls: ParsedProviderCall[] = [] + for await (const call of provider.createSessionParser(sessions[0]!, new Set()).parse()) calls.push(call) + + expect(calls.length).toBeGreaterThan(0) + for (const call of calls) { + expect(call.projectPath === undefined || typeof call.projectPath === 'string').toBe(true) + expect(call.workingDirectory === undefined || typeof call.workingDirectory === 'string').toBe(true) + } + }) + it('accepts session_meta lines larger than 16 KB (Codex CLI 0.128+)', async () => { // Codex CLI 0.128+ embeds the full base_instructions / system prompt in the // first session_meta line, often pushing it past 20 KB. Regression guard