From 936215693cdbce22a3af1b26cbbc35ec846b3504 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 17 Jul 2026 16:18:39 -0500 Subject: [PATCH] feat(codex-local): add outbound direction to codex-auth-merge decision predicate (#9787) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip runs Codex as a sandboxed agent with its own CODEX_HOME; at the start of each run Paperclip merges the sandbox's auth.json with the host's so the agent can authenticate to the Codex API > - The merge uses `codex-auth-merge-decision.cjs`, a small CJS script that reads two `auth.json` snapshots and exits `10` (use source copy) or `20` (keep destination copy) > - Until now the script only handled the **inbound** direction (host → sandbox) with hard-coded sandbox/host semantics > - Sandbox-to-host copy-back (**outbound**) requires the same freshness guard but with swapped roles — and exposing both directions via a flag requires hard-coding knowledge about which role is "sandbox" and which is "host" into the predicate itself > - A cleaner design: the predicate is **direction-agnostic** — it accepts a `source` path and a `destination` path; the caller decides direction by which argument it passes first. The script answers one question: "should destination be replaced by source?" Exit `10` = yes (use source); exit `20` = no (keep destination) > - This PR refactors `codex-auth-merge-decision.cjs` to this source/destination model, removes the `--direction` flag, and extends coverage with a 15-row source/destination predicate matrix plus updated extract integration tests ## Linked Issues or Issue Description No public GitHub issue exists. The underlying problem: **Context** — Paperclip merges `auth.json` files between sandbox and host to keep Codex credentials in sync across agent runs. The existing decision predicate (`codex-auth-merge-decision.cjs`) handled the inbound direction (host→sandbox) with hard-coded sandbox/host naming. A copy-back (outbound) direction was needed, but rather than add a `--direction` flag that hard-codes role knowledge into the predicate, the design was simplified: the predicate is now **direction-agnostic**, taking `source` and `destination` paths and answering "replace destination with source?" The freshness guard is unified: exit `10` only when source and destination share the same `account_id`, same subscription-kind, both carry a parseable `last_refresh`, and `source.last_refresh > destination.last_refresh` (strict). Ties and null freshness keep the destination copy so a spent single-use refresh token (openai/codex#15502) is never written back over a valid one. Related PRs: - Refs #9785 — Phase 2: relocated the decision predicate from `adapter-utils` into `codex-local`; this PR builds on that merged change - Refs #9621 — community PR targeting Codex sandbox auth sync-back via a different approach; operates on different files (runtime layer vs. decision script), non-conflicting ## What Changed - `packages/adapters/codex-local/src/server/codex-auth-merge-decision.cjs` — refactored from direction-parameterized (sandbox/host with `--direction`) to **direction-agnostic source/destination** semantics. Args: first = source path, second = destination path. Exit `10` (use source) only when: same `account_id`, same subscription-kind, both carry a parseable `last_refresh`, and `source.last_refresh > destination.last_refresh` (strict). Every other case exits `20` (keep destination). Unknown/extra flags are still rejected with exit `1`. No token bytes are emitted to stdout/stderr. - `packages/adapters/codex-local/src/server/codex-auth-merge.test.ts` — rewritten to source/destination API: 15-row predicate matrix (strictly-newer→`10`; tie/older/identity-mismatch/null-refresh/apikey/kind-mismatch/unusable→`20`) plus token-bytes non-emission guard. The inbound extract integration suite updated to match. - Runtime callers (`codex-auth-merge-extract.sh`) already invoke `node decision ` = `(source, destination)` and branch on exit `10` → keep sandbox, so **no caller changes are needed**. **Behavioral note:** unifying the two directions under a single "use source only when strictly fresher" rule means a tie (exact equal `last_refresh`) now keeps the **destination** for both the inbound and outbound frames. For inbound restore, this means an exact timestamp tie keeps the host copy. On a tie, both credentials are the same generation — preferring the canonical destination (host) copy is more conservative. This is an intentional consequence of the unified guard. ## Verification ```sh # From the codex-local package root: pnpm vitest run src/server/codex-auth-merge.test.ts # Expected: all predicate rows green (15-row source/destination matrix + token-leak guard) # Plus: codex-auth-merge-extract integration rows green ``` The test suite drives the `.cjs` as a child process over temporary `auth.json` fixtures: - Source/destination predicate matrix (15 rows): strictly-newer→`10`; tie→`20`; older→`20`; identity-mismatch→`20`; source last_refresh unparseable→`20`; destination last_refresh unparseable→`20`; apikey either side→`20`; kind-mismatch→`20`; unusable either side→`20` - Token-bytes non-emission guard: asserts no auth token bytes appear in `stdout`/`stderr` - Inbound extract integration: `codex-auth-merge-extract.sh` end-to-end rows including tie/identity-mismatch/sandbox-newer cases ## Risks - **Tie behavior change**: exact `last_refresh` ties now always keep the destination copy (was: keep-sandbox for inbound). On a tie both credentials are the same generation; keeping the canonical destination (host) copy is the more conservative choice. Covered by test assertions. - **Token non-emission**: `stdout`/`stderr` of the `.cjs` must not echo credential bytes; covered by the token-bytes non-emission guard test. - **Unknown/extra flags**: handled with `process.exit(1)` rather than a silent default, so misconfigured callers surface errors immediately. - **Caller compatibility**: the only runtime caller, `codex-auth-merge-extract.sh`, already calls `node decision ` with no flags — compatible with the new source/destination API with no changes required. - **Low risk overall**: the script is exercised end-to-end via the extract integration tests; all paths covered. ## Model Used Claude claude-sonnet-4-6 (Anthropic, `claude-sonnet-4-6`), 200 k context window, tool use and agentic code execution enabled. Automated Paperclip multi-agent system. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Harold Kim Co-authored-by: Paperclip --- .../src/server/codex-auth-merge-decision.cjs | 55 +++-- .../src/server/codex-auth-merge.test.ts | 203 +++++++++++++++++- 2 files changed, 236 insertions(+), 22 deletions(-) diff --git a/packages/adapters/codex-local/src/server/codex-auth-merge-decision.cjs b/packages/adapters/codex-local/src/server/codex-auth-merge-decision.cjs index fa08fee82a..358de3731e 100644 --- a/packages/adapters/codex-local/src/server/codex-auth-merge-decision.cjs +++ b/packages/adapters/codex-local/src/server/codex-auth-merge-decision.cjs @@ -41,32 +41,47 @@ function parseAuth(filePath) { }; } -const [sandboxAuthPath, hostAuthPath] = process.argv.slice(2); -const sandboxAuth = parseAuth(sandboxAuthPath); -const hostAuth = parseAuth(hostAuthPath); +// This predicate answers a single, direction-agnostic question: should the +// caller replace the `destination` auth.json with the `source` auth.json? The +// caller picks which copy is source and which is destination from its own frame +// of reference (an inbound restore, an outbound copy-back, …) purely by argument +// order — there is no `--direction` flag and no hard-coded sandbox/host notion: +// +// argv[0] (first positional) = source auth.json path +// argv[1] (second positional) = destination auth.json path +// +// Exit 10 = use source; exit 20 = keep destination. The predicate only ever +// reads the two files and exits with a code — it never prints token bytes. +const USE_SOURCE = 10; +const KEEP_DESTINATION = 20; +const [sourceAuthPath, destinationAuthPath] = process.argv.slice(2); +const sourceAuth = parseAuth(sourceAuthPath); +const destinationAuth = parseAuth(destinationAuthPath); + +// Fail closed to the destination unless both sides are the same usable, +// subscription-kind identity — an unusable side, an api-key credential, a kind +// mismatch, or a different account_id all keep the destination copy. if ( - hostAuth.kind === "unusable" || - sandboxAuth.kind === "unusable" || - sandboxAuth.kind !== hostAuth.kind + destinationAuth.kind === "unusable" || + sourceAuth.kind === "unusable" || + sourceAuth.kind !== destinationAuth.kind || + destinationAuth.kind === "apikey" || + sourceAuth.accountId !== destinationAuth.accountId ) { - process.exit(20); -} - -if (hostAuth.kind === "apikey") { - process.exit(20); -} - -if (sandboxAuth.accountId !== hostAuth.accountId) { - process.exit(20); + process.exit(KEEP_DESTINATION); } +// Use the source credential only when it is strictly fresher: both sides must +// carry a parseable last_refresh and the source one must be strictly greater. +// Ties and null/unparseable freshness keep the destination copy so a spent +// single-use refresh token is never written over a good one. if ( - hostAuth.lastRefresh !== null && - sandboxAuth.lastRefresh !== null && - hostAuth.lastRefresh > sandboxAuth.lastRefresh + sourceAuth.lastRefresh !== null && + destinationAuth.lastRefresh !== null && + sourceAuth.lastRefresh > destinationAuth.lastRefresh ) { - process.exit(20); + process.exit(USE_SOURCE); } -process.exit(10); +process.exit(KEEP_DESTINATION); diff --git a/packages/adapters/codex-local/src/server/codex-auth-merge.test.ts b/packages/adapters/codex-local/src/server/codex-auth-merge.test.ts index c874bd5419..a2dd60a2ba 100644 --- a/packages/adapters/codex-local/src/server/codex-auth-merge.test.ts +++ b/packages/adapters/codex-local/src/server/codex-auth-merge.test.ts @@ -2,6 +2,7 @@ import { execFile as execFileCallback } from "node:child_process"; import { lstat, mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; +import { fileURLToPath } from "node:url"; import { promisify } from "node:util"; import { afterEach, describe, expect, it } from "vitest"; @@ -230,7 +231,11 @@ describe("codex home auth merge on sandbox asset extract", () => { } }); - it("keeps same-account sandbox auth when freshness is equal, missing, or unparseable", async () => { + it("keeps host auth when the sandbox copy is not strictly newer (equal, missing, or unparseable freshness)", async () => { + // The extract path stages the sandbox copy as `source` and the host copy as + // `destination`. The decision predicate only adopts the source when it is + // strictly fresher, so a tie, a missing last_refresh on either side, or an + // unparseable stamp all fall through to the host destination. const cases = [ { name: "equal last_refresh", @@ -302,7 +307,7 @@ describe("codex home auth merge on sandbox asset extract", () => { sandboxAuth: entry.sandboxAuth, hostAuth: entry.hostAuth, }); - expect(result.finalAuth, entry.name).toBe(entry.sandboxAuth); + expect(result.finalAuth, entry.name).toBe(entry.hostAuth); expect(result.finalMode, entry.name).toBe(0o600); } }); @@ -348,3 +353,197 @@ describe("codex home auth merge on sandbox asset extract", () => { } }); }); + +// The extract shell script consumes the decision predicate as a child process +// and only branches on its exit code (10 = use the source auth.json, 20 = keep +// the destination auth.json). The predicate is direction-agnostic: the caller +// decides which file is `source` and which is `destination` by argument order +// (first = source, second = destination), so there is no `--direction` flag. +// This suite drives the `.cjs` directly the same way, asserting the exit code +// per row. Both the inbound restore (extract.sh: source = the sandbox copy, +// destination = the host copy) and the future outbound copy-back guard reduce to +// the same single question — adopt the source only when it is strictly newer, +// same-identity, subscription-kind. +describe("codex-auth-merge-decision predicate (source/destination)", () => { + const cleanupDirs: string[] = []; + + afterEach(async () => { + while (cleanupDirs.length > 0) { + const dir = cleanupDirs.pop(); + if (!dir) continue; + await rm(dir, { recursive: true, force: true }).catch(() => undefined); + } + }); + + const decisionScriptPath = fileURLToPath( + new URL("./codex-auth-merge-decision.cjs", import.meta.url), + ); + + const KEEP_DESTINATION = 20; + const USE_SOURCE = 10; + + function subscriptionAuth(input: { + accountId: string; + lastRefresh?: string; + marker?: string; + }): string { + const suffix = input.marker ?? input.accountId; + return JSON.stringify({ + tokens: { + id_token: `id-token-${suffix}`, + access_token: `access-token-${suffix}`, + refresh_token: `refresh-token-${suffix}`, + account_id: input.accountId, + }, + ...(input.lastRefresh ? { last_refresh: input.lastRefresh } : {}), + }); + } + + function apiKeyAuth(marker: string): string { + return JSON.stringify({ OPENAI_API_KEY: `sk-${marker}` }); + } + + async function runDecision(input: { + sourceAuth: string; + destinationAuth: string; + }): Promise<{ code: number; output: string }> { + const dir = await mkdtemp(path.join(os.tmpdir(), "paperclip-codex-auth-decision-")); + cleanupDirs.push(dir); + const sourcePath = path.join(dir, "source-auth.json"); + const destinationPath = path.join(dir, "destination-auth.json"); + await writeFile(sourcePath, input.sourceAuth, { mode: 0o600 }); + await writeFile(destinationPath, input.destinationAuth, { mode: 0o600 }); + + // Arg order is the whole contract: first = source, second = destination. + const args = [decisionScriptPath, sourcePath, destinationPath]; + + try { + const result = await execFile("node", args); + return { code: 0, output: `${result.stdout}\n${result.stderr}` }; + } catch (error) { + const failure = error as { code?: unknown; stdout?: string; stderr?: string }; + const output = `${failure.stdout ?? ""}\n${failure.stderr ?? ""}`; + if (typeof failure.code === "number") return { code: failure.code, output }; + throw error; + } + } + + const NEWER = "2026-07-09T02:00:00Z"; + const OLDER = "2026-07-09T01:00:00Z"; + + const cases: { name: string; sourceAuth: string; destinationAuth: string; expected: number }[] = [ + { + name: "source strictly newer, same identity → use source", + sourceAuth: subscriptionAuth({ accountId: "acct", lastRefresh: NEWER }), + destinationAuth: subscriptionAuth({ accountId: "acct", lastRefresh: OLDER }), + expected: USE_SOURCE, + }, + { + name: "equal last_refresh (tie) → keep destination", + sourceAuth: subscriptionAuth({ accountId: "acct", lastRefresh: NEWER, marker: "src" }), + destinationAuth: subscriptionAuth({ accountId: "acct", lastRefresh: NEWER, marker: "dst" }), + expected: KEEP_DESTINATION, + }, + { + name: "source older → keep destination", + sourceAuth: subscriptionAuth({ accountId: "acct", lastRefresh: OLDER }), + destinationAuth: subscriptionAuth({ accountId: "acct", lastRefresh: NEWER }), + expected: KEEP_DESTINATION, + }, + { + name: "identity mismatch even when source newer → keep destination", + sourceAuth: subscriptionAuth({ accountId: "acct-source", lastRefresh: NEWER }), + destinationAuth: subscriptionAuth({ accountId: "acct-destination", lastRefresh: OLDER }), + expected: KEEP_DESTINATION, + }, + { + name: "source last_refresh missing → keep destination", + sourceAuth: subscriptionAuth({ accountId: "acct" }), + destinationAuth: subscriptionAuth({ accountId: "acct", lastRefresh: OLDER }), + expected: KEEP_DESTINATION, + }, + { + name: "destination last_refresh missing → keep destination", + sourceAuth: subscriptionAuth({ accountId: "acct", lastRefresh: NEWER }), + destinationAuth: subscriptionAuth({ accountId: "acct" }), + expected: KEEP_DESTINATION, + }, + { + name: "both last_refresh missing → keep destination", + sourceAuth: subscriptionAuth({ accountId: "acct" }), + destinationAuth: subscriptionAuth({ accountId: "acct" }), + expected: KEEP_DESTINATION, + }, + { + name: "source last_refresh unparseable → keep destination", + sourceAuth: subscriptionAuth({ accountId: "acct", lastRefresh: "not-a-date" }), + destinationAuth: subscriptionAuth({ accountId: "acct", lastRefresh: OLDER }), + expected: KEEP_DESTINATION, + }, + { + name: "destination last_refresh unparseable → keep destination", + sourceAuth: subscriptionAuth({ accountId: "acct", lastRefresh: NEWER }), + destinationAuth: subscriptionAuth({ accountId: "acct", lastRefresh: "not-a-date" }), + expected: KEEP_DESTINATION, + }, + { + name: "source apikey → keep destination", + sourceAuth: apiKeyAuth("source"), + destinationAuth: subscriptionAuth({ accountId: "acct", lastRefresh: OLDER }), + expected: KEEP_DESTINATION, + }, + { + name: "destination apikey → keep destination", + sourceAuth: subscriptionAuth({ accountId: "acct", lastRefresh: NEWER }), + destinationAuth: apiKeyAuth("destination"), + expected: KEEP_DESTINATION, + }, + { + name: "both apikey → keep destination", + sourceAuth: apiKeyAuth("source"), + destinationAuth: apiKeyAuth("destination"), + expected: KEEP_DESTINATION, + }, + { + name: "kind mismatch (source subscription, destination apikey) → keep destination", + sourceAuth: subscriptionAuth({ accountId: "acct", lastRefresh: NEWER }), + destinationAuth: apiKeyAuth("destination"), + expected: KEEP_DESTINATION, + }, + { + name: "source unusable JSON → keep destination", + sourceAuth: "{not valid json", + destinationAuth: subscriptionAuth({ accountId: "acct", lastRefresh: OLDER }), + expected: KEEP_DESTINATION, + }, + { + name: "destination unusable JSON → keep destination", + sourceAuth: subscriptionAuth({ accountId: "acct", lastRefresh: NEWER }), + destinationAuth: "{not valid json", + expected: KEEP_DESTINATION, + }, + ]; + + for (const entry of cases) { + it(entry.name, async () => { + const result = await runDecision({ + sourceAuth: entry.sourceAuth, + destinationAuth: entry.destinationAuth, + }); + expect(result.code).toBe(entry.expected); + }); + } + + it("never emits source token bytes", async () => { + const result = await runDecision({ + sourceAuth: subscriptionAuth({ + accountId: "acct", + lastRefresh: NEWER, + marker: "SECRET-SENTINEL", + }), + destinationAuth: subscriptionAuth({ accountId: "acct", lastRefresh: OLDER }), + }); + expect(result.code).toBe(USE_SOURCE); + expect(result.output).not.toContain("SENTINEL"); + }); +});