From a6448cd060936e29656f45f374eca6d968216dcd Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Wed, 23 Sep 2026 15:43:46 -0700 Subject: [PATCH] fix(runner): tolerate Codex account notifications during active turns (#13902) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The Rust runner connects task runs to Codex app-server sessions. > - Codex sends account notifications on the connection during authentication updates. > - These notifications have no task, thread, or turn identity. > - The runner treated a valid account update as an invalid task event and stopped the run. > - This pull request classifies account updates as connection information while preserving task identity checks. > - Sandbox images can also contain an older runner with compatible metadata. The server must compare its bytes before reuse. > - Slack conversations use the deployed runner and can complete when Codex refreshes account state. ## Linked Issues or Issue Description Related: #13853. That change fixes the supported Codex version range. This fixes a separate notification failure after the version check succeeds. **What happened?** An active Codex task stopped with `thread_binding_mismatch` when the provider emitted `account/updated` without a thread ID. Slack showed that the agent stopped before completing its turn. **Expected behavior** Connection-level account updates must not stop a task or acquire task authority. Account details must not appear in task output. **Steps to reproduce** 1. Start a task with the native Codex runner. 2. Emit `account/updated` during the turn, with `authMode` and `planType` but no thread ID. 3. Observe that the old runner rejects the notification and fails the turn. **Paperclip version or commit** Reproduced after #13853. The fix is based on `b648d8cdd`. **Deployment mode** Cloud staging with a remote Codex runner and a Slack chat connection. ## What Changed - Classify `account/updated` and `account/login/completed` as connection information. - Reject account notifications that contain execution identity fields. - Reuse the existing bounded diagnostic path without publishing account payloads. - Test both notification types during two consecutive turns. Preserve existing identity rejection tests. - Compare preinstalled sandbox runner bytes with the controller artifact. Stage the deployed binary after a mismatch, failed checksum, or timeout. Keep exact retained artifact reuse. ## Verification - `cargo test --release --locked -p paperclip-runner-core`: passed, 582 test executions; two existing ignored cases. - `cargo test --release --locked -p paperclip-runner-core --test codex_provider`: 89 passed; two existing ignored cases. - `cargo fmt --all` and `git diff --check`: passed. - Repository-wide `pnpm -r typecheck`: passed. Server typecheck also passed after the artifact-selection change. - Server executor suite: 456 tests passed, including preinstalled digest match, mismatch, checksum failure, and timeout. - Repository-wide Vitest and build are running. - Deployed `5648d90d5dd762c5a8b697face269f6eea9cd59d` to one staging stack and verified its serving SHA. - Retried the failed conversation through the Slack UI. The agent replied and the task completed. - Sent a fresh Slack mention asking which bots belong to the channel. Verified successful `slack_members` and `slack_user` calls, a successful run, and the delivered Slack answer. - Sent a follow-up in the same Slack thread without another mention. The agent returned the requested names from context, with one final reply. ## Risks - Only two known connection-level notification methods change behavior. Unknown authoritative methods and malformed or conflicting identities still fail closed. - An older sandbox runner now requires one upload when its bytes differ from the controller artifact. Remote OS and architecture checks still apply. - No schema, credential, permission, Codex version-range, or UI changes. The Codex minimum remains 0.149.0. ## Model Used OpenAI GPT-6 (`gpt-6-astra`) through Codex, with reasoning, code editing, terminal tools, and browser verification. The context-window size is not exposed in this session. ## 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: Paperclip --- .../src/bin/fake-codex-app-server.rs | 8 +++ .../crates/runner-core/src/codex_provider.rs | 46 +++++++++++++- .../runner-core/tests/codex_provider.rs | 48 +++++++++++++++ .../native-session-executor.test.ts | 12 ++-- .../native-runtime/native-session-executor.ts | 61 ++++++++++--------- 5 files changed, 138 insertions(+), 37 deletions(-) diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-codex-app-server.rs b/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-codex-app-server.rs index aeb637022e1..bac103d0ff0 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-codex-app-server.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/bin/fake-codex-app-server.rs @@ -1471,6 +1471,14 @@ fn run() -> Result<(), Box> { "method": "turn/started", "params": {"turn": {"id": provider_turn_id}} }))?; + if args.iter().any(|value| value == "--account-notifications") { + send(json!({"method":"account/updated", "params":{ + "authMode":"chatgpt", "planType":"pro" + }}))?; + send(json!({"method":"account/login/completed", "params":{ + "loginId":"fixture-login", "success":true, "error":null + }}))?; + } if args.iter().any(|value| value == "--helper-tool-requests") { if !args.iter().any(|value| value == "--foreign-helper-tool") { send(json!({"method":"item/completed", "params":{ diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/codex_provider.rs b/packages/paperclip-runner/runner/crates/runner-core/src/codex_provider.rs index bfa72a01c71..549b6d88908 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/codex_provider.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/codex_provider.rs @@ -3383,8 +3383,18 @@ fn classify_notification_thread( "Codex notification has malformed turn identity", )); } - // This connection-level notification carries no task authority. Codex can - // emit it while loading skills during the first turn. + // Account and skill updates describe the provider connection, not a task. + // Codex can emit them during startup or credential refresh without thread + // or turn IDs. Never let them acquire execution authority or expose their + // account payload as task output. + if matches!(method, "account/updated" | "account/login/completed") { + if contains_provider_work_binding(params) { + return Err(LocalRunnerError::invalid( + "Codex account notification contains execution identity", + )); + } + return Ok(NotificationThread::UnrelatedInformation); + } if method == "skills/changed" && !contains_provider_work_binding(params) { return Ok(NotificationThread::UnrelatedInformation); } @@ -4986,6 +4996,38 @@ mod notification_identity_tests { } } + #[test] + fn account_notifications_are_connection_information_without_execution_authority() { + for (method, params) in [ + ( + "account/updated", + json!({"authMode":"chatgpt", "planType":"pro"}), + ), + ( + "account/login/completed", + json!({"loginId":null, "success":true, "error":null}), + ), + ] { + assert_eq!( + classify_notification_thread(method, "root", &BTreeSet::new(), ¶ms).unwrap(), + NotificationThread::UnrelatedInformation + ); + for invalid in [ + json!({"threadId":"other"}), + json!({"turnId":"unbound"}), + json!({"itemId":"unbound"}), + json!({"nested":{"request":{"id":"unbound"}}}), + json!({"threadId":7}), + json!({"threadId":"root", "thread":{"id":"other"}}), + ] { + assert!( + classify_notification_thread(method, "root", &BTreeSet::new(), &invalid) + .is_err() + ); + } + } + } + #[test] fn child_mcp_startup_before_lineage_has_no_execution_authority() { let params = diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs index b32714c0cb7..ff4edc2e33b 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/codex_provider.rs @@ -588,6 +588,54 @@ fn codex_transport_buffers_notifications_while_waiting_for_responses() { fs::remove_dir_all(directory).expect("remove Codex integration-test directory"); } +#[test] +fn codex_account_updates_do_not_interrupt_turns_or_publish_account_details() { + let directory = temporary_directory("account-notifications"); + let config = provider_config(&directory, &["--account-notifications"]); + let mut provider = CodexProvider::start(&config, None).expect("start fake Codex provider"); + for _ in 0..2 { + provider + .start_turn("Are you there?", &config.cwd) + .expect("start provider turn"); + let mut completed = false; + let mut account_notices = 0; + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while std::time::Instant::now() < deadline { + match provider.poll().expect("poll provider event") { + Some(CodexProviderEvent::ProtocolFailure { diagnostic }) => { + panic!("account notification interrupted the turn: {diagnostic}"); + } + Some(CodexProviderEvent::Notification { method, params }) => { + if params["providerMethod"] + .as_str() + .is_some_and(|method| method.starts_with("account/")) + { + account_notices += 1; + assert_eq!(method, "warning"); + assert_eq!(params["classification"], "unrelated_information"); + assert!(!params.to_string().contains("fixture-login")); + assert!(params.get("authMode").is_none()); + assert!(params.get("planType").is_none()); + } + if method == "turn/completed" { + completed = true; + break; + } + } + _ => {} + } + std::thread::sleep(std::time::Duration::from_millis(1)); + } + assert!( + completed, + "the real provider boundary must deliver the terminal" + ); + assert_eq!(account_notices, 2); + } + provider.shutdown().expect("stop provider"); + fs::remove_dir_all(directory).expect("remove account notification test directory"); +} + #[test] fn codex_goal_autostart_binds_the_provider_turn_authority() { let directory = temporary_directory("goal-autostart"); diff --git a/server/src/services/native-runtime/native-session-executor.test.ts b/server/src/services/native-runtime/native-session-executor.test.ts index 3be76308f40..4bd73dce899 100644 --- a/server/src/services/native-runtime/native-session-executor.test.ts +++ b/server/src/services/native-runtime/native-session-executor.test.ts @@ -10396,7 +10396,7 @@ describe("runnerd provider runtime wiring", () => { }); it.each([ - ...["current", "stale", "missing", "retained", "retained-mismatch", "retained-error", "retained-timeout", "retained-explicit"] + ...["current", "preinstalled-exact", "preinstalled-mismatch", "preinstalled-error", "preinstalled-timeout", "stale", "missing", "retained", "retained-mismatch", "retained-error", "retained-timeout", "retained-explicit"] .map((image) => ({ image, version: "0.156.0", compatible: true })), ...["0.149.0", "0.149.1", "0.153.4", "0.156.1"] .map((version) => ({ image: "current", version, compatible: true })), @@ -10405,11 +10405,11 @@ describe("runnerd provider runtime wiring", () => { ])("uses shared Codex and the server-owned replacement artifact (image=$image, Codex=$version)", async ({ image, version, compatible }) => { const retained = image.startsWith("retained"); const exactRetained = image === "retained" || image === "retained-explicit"; - const needsReplacement = image !== "current" && !exactRetained; + const needsReplacement = image !== "current" && image !== "preinstalled-exact" && !exactRetained; // The mocked remote executes metadata probes; artifact staging only needs bytes. // Keep this regression independent of a locally compiled Rust runner binary. const controllerArtifact = join(isolatedStateDirectory, "paperclip-runnerd"); - if (needsReplacement || retained) { + if (needsReplacement || retained || image === "preinstalled-exact") { await writeFile(controllerArtifact, "fixture runner artifact"); state.resolveRunnerBinary.mockReturnValueOnce(controllerArtifact); } @@ -10420,10 +10420,10 @@ describe("runnerd provider runtime wiring", () => { let stdout = ""; const script = command.args?.[1] ?? ""; if (script.includes('sha256sum "$1"')) { - if (image === "retained-error") throw new Error("checksum unavailable"); + if (image.endsWith("-error")) throw new Error("checksum unavailable"); return { - exitCode: 0, signal: null, timedOut: image === "retained-timeout", stderr: "", - stdout: `${createHash("sha256").update(image === "retained-mismatch" ? "stale artifact" : "fixture runner artifact").digest("hex")} ${command.args?.[3]}\n`, + exitCode: 0, signal: null, timedOut: image.endsWith("-timeout"), stderr: "", + stdout: `${createHash("sha256").update(image.endsWith("-mismatch") ? "stale artifact" : "fixture runner artifact").digest("hex")} ${command.args?.[3]}\n`, }; } else if (command.args?.[0] === "--build-metadata") { stdout = JSON.stringify({ diff --git a/server/src/services/native-runtime/native-session-executor.ts b/server/src/services/native-runtime/native-session-executor.ts index 48de155f837..e6ec8839124 100644 --- a/server/src/services/native-runtime/native-session-executor.ts +++ b/server/src/services/native-runtime/native-session-executor.ts @@ -10852,9 +10852,33 @@ async function createRunnerdBackendWithinSessionClaim( remotePrepared = true; return; } - // A resumed sandbox may already contain the exact controller-owned binary. - // Do not upload it every turn, but never treat compatible metadata alone as - // proof of artifact identity (especially for an explicit operator override). + // Compatibility metadata does not prove artifact identity. Reuse only the + // exact controller-owned bytes when the controller artifact is available. + const matchesControllerRunnerArtifact = async (executable: string): Promise => { + const expected = createHash("sha256") + .update(readFileSync(controllerRunnerBinary)) + .digest("hex"); + try { + const probe = await remoteCommandRunner.execute({ + command: "sh", + args: [ + "-c", + 'test -x "$1" || exit 1; if command -v sha256sum >/dev/null 2>&1; then sha256sum "$1"; else shasum -a 256 "$1"; fi', + "paperclip-runner-artifact", + executable, + ], + cwd: remoteTarget.remoteCwd, + bypassSession: true, + timeoutMs: 10_000, + }); + return probe.exitCode === 0 && !probe.timedOut && + /^[a-f0-9]{64}\s/.test(probe.stdout) && + probe.stdout.trim().split(/\s+/)[0] === expected; + } catch { + // An unavailable checksum uses the verified staging path. + return false; + } + }; let runnerArtifactPrepared = false; if ( sandboxLeaseAcquisition?.outcome === "resumed" && @@ -10863,32 +10887,7 @@ async function createRunnerdBackendWithinSessionClaim( runnerArtifactPrepared = await measureNativeRunnerSpan( input.trace, "runner.artifact.verify_retained", - async () => { - const expected = createHash("sha256") - .update(readFileSync(controllerRunnerBinary)) - .digest("hex"); - try { - const probe = await remoteCommandRunner.execute({ - command: "sh", - args: [ - "-c", - 'test -x "$1" || exit 1; if command -v sha256sum >/dev/null 2>&1; then sha256sum "$1"; else shasum -a 256 "$1"; fi', - "paperclip-runner-artifact", - remoteBinary, - ], - cwd: remoteTarget.remoteCwd, - bypassSession: true, - timeoutMs: 10_000, - }); - return probe.exitCode === 0 && !probe.timedOut && - /^[a-f0-9]{64}\s/.test(probe.stdout) && - probe.stdout.trim().split(/\s+/)[0] === expected; - } catch { - // Missing binaries, checksum tools, or a failed probe use the - // ordinary verified staging path; none authorizes cached execution. - return false; - } - }, + () => matchesControllerRunnerArtifact(remoteBinary), ); } const explicitRemoteBinary = input.runnerRemoteBinaryPath?.trim() || null; @@ -10905,6 +10904,10 @@ async function createRunnerdBackendWithinSessionClaim( "runner.artifact.verify_preinstalled", () => verifyRemoteRunner(requiredMode, preinstalledRunner), ); + if (existsSync(controllerRunnerBinary) && + !await matchesControllerRunnerArtifact(preinstalledRunner)) { + throw new Error("runner_remote_preinstalled_artifact_mismatch"); + } await measureNativeRunnerSpan( input.trace, "runner.artifact.link",