From e33aa8a46aa2c05a0faa4ce35380b0a14f7fcdf8 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 6 Nov 2026 18:07:26 -0600 Subject: [PATCH] fix(runner): let saved Codex threads follow the current feature policy (#19659) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The Paperclip runner keeps a saved Codex thread so the next run of a task continues the same conversation > - On each new run, the controller sends `run.attach` with the current Codex launch arguments, and the runner refuses any change to "protected" arguments > - #19516 removed `--disable image_generation` from the Codex launch arguments, but every thread saved before that deploy still records the flag > - The runner treated the feature toggle as protected, so every pre-deploy Codex thread failed with `run.attach cannot change protected launch arguments` > - Same-run recovery then tried to replace the thread, the replacement failed, and the tasks became blocked for an operator > - This pull request treats Codex feature toggles as controller launch policy, so a saved thread adopts the current toggles and restarts on the same thread > - The benefit is that a release that turns a Codex feature on or off no longer strands every saved Codex thread ## Linked Issues or Issue Description Refs #19516 **What happened?** After a staging deploy that included #19516, Codex agent runs on tasks with a saved thread failed at the start. The run log showed `native_session_recovery_failed: PRP command run.attach failed: run.attach cannot change protected launch arguments` (error code `native_session_interrupted`). Same-run retries then failed with `runner_continuity_break_archive_failed` and `runner_remote_state_unavailable`, and the tasks were blocked with "Workspace repair required". Four tasks on one Codex agent showed this first failure. Each one last ran successfully before the deploy. **Steps to reproduce** 1. Run a Codex task so that its thread is saved with the launch arguments from before #19516 (`--disable image_generation`). 2. Deploy a revision that includes #19516. 3. Wake the same task again. `run.attach` sends the current arguments without the toggle, and the runner rejects them. **Expected behavior** The saved thread continues under the current launch policy. **Root cause** `CodexCommandExecutor::stable_launch_args` drops only the per-run filesystem and shell-environment entries before it compares saved and current arguments. All other arguments, including Codex `--enable`/`--disable` feature toggles, must be identical. The controller computes the toggles from Paperclip policy for every launch, so any change to that policy made every saved thread impossible to attach. ## What Changed - `provider_backend.rs`: `stable_launch_args` also skips `--enable ` and `--disable ` pairs and the `--enable=`/`--disable=` form. The function contract comment now lists the rebindable arguments. `run.attach` then adopts the controller's current toggles. Because the arguments changed, the existing path stops the provider and restarts it on the same thread. - New test `saved_thread_adopts_the_controllers_current_codex_feature_policy`: - A thread saved with `--disable image_generation` attaches with the current arguments, keeps its thread ID, and records the new arguments. - A new `--disable` toggle (a tighter policy) is also adopted. - A new `--enable` toggle is adopted in both flag forms, and the thread ID stays the same. - A sandbox bypass flag, a different `default_permissions` profile, and a different subcommand are still rejected with "protected launch arguments", and the saved state does not change. ## Verification - Red on `master` (8621bcf53e) with only the new test added: ``` test provider_backend::tests::saved_thread_adopts_the_controllers_current_codex_feature_policy ... FAILED a saved thread must follow the current feature policy: DurableRunnerError("run.attach cannot change protected launch arguments", None, None) ``` - Integration regression `tests/codex_provider.rs::saved_codex_thread_attaches_after_a_release_changes_its_feature_toggles` drives the real durable executor with the fake Codex app-server. It saves a thread with `--disable image_generation`, attaches with the current arguments, runs a second turn, and checks that the provider restarted once. A sandbox-bypass change is still rejected. Red on unfixed source: ``` the saved thread adopts the current feature toggles: DurableRunnerError("run.attach cannot change protected launch arguments", None, None) ``` Green with the fix. The full `codex_provider` binary passes (116 passed). - Green after the fix (unit test): ``` test provider_backend::tests::saved_thread_adopts_the_controllers_current_codex_feature_policy ... ok ``` - `cargo test -p paperclip-runner-core --lib`: 560 passed, 0 failed. - `cargo test -p paperclip-runner-core --no-fail-fast`: all Codex, native-provider-backend, and durable-recovery integration binaries pass. Some tests in `acpx_park_readiness` and `stdio_mcp` failed. Different tests in these same binaries also fail on unmodified `master` on this machine. They are timing tests under load and do not use this code. - Not run: a live staging attach with a pre-#19516 thread. The runner binary in a sandbox must match the controller's binary digest, so this fix reaches sandboxes with the server deploy. ## Risks - A saved thread now accepts any `--enable`/`--disable` toggle that the controller sends. The controller is the only source of `run.attach`, and a new thread gets the same toggles. A tightened policy now also applies to saved threads, where before the saved thread was stranded. - The sandbox, permission profile, approval, model, and subcommand checks do not change. - When the toggles change, the warm provider process restarts on the same thread once. This is the existing path for changed runtime arguments. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used through Claude Code with tool use (shell, file edit, browser reads of staging). ## 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- .../runner-core/src/provider_backend.rs | 150 +++++++++++++++++- .../runner-core/tests/codex_provider.rs | 114 +++++++++++++ 2 files changed, 262 insertions(+), 2 deletions(-) diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/provider_backend.rs b/packages/paperclip-runner/runner/crates/runner-core/src/provider_backend.rs index 6ac3fe4dbbe..310ddb5eb76 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/provider_backend.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/provider_backend.rs @@ -3007,12 +3007,25 @@ impl CodexCommandExecutor { } // Only the authenticated controller can rebind these run-scoped launch - // settings. Executable, model, instructions, approval mode, and all other - // flags remain part of the immutable durable provider profile. + // settings: per-run filesystem and shell-environment entries, and Codex + // feature toggles (`--enable`/`--disable`). The controller recomputes the + // toggles from Paperclip policy for every launch, so a saved thread must + // follow a release that turns a feature on or off rather than become + // impossible to resume. Executable, model, instructions, approval mode, + // sandbox, permission profile, subcommand, and all other flags remain part + // of the immutable durable provider profile. fn stable_launch_args(args: &[String]) -> Vec { let mut stable = Vec::new(); let mut index = 0; while index < args.len() { + if (args[index] == "--enable" || args[index] == "--disable") && index + 1 < args.len() { + index += 2; + continue; + } + if args[index].starts_with("--enable=") || args[index].starts_with("--disable=") { + index += 1; + continue; + } if args[index] == "-c" && index + 1 < args.len() { let key = args[index + 1].split('=').next().unwrap_or(""); if matches!( @@ -6468,6 +6481,139 @@ done fs::remove_dir_all(directory).unwrap(); } + #[test] + fn saved_thread_adopts_the_controllers_current_codex_feature_policy() { + // A thread saved before a Paperclip upgrade carries that release's + // feature toggles. The controller recomputes them for every launch, so a + // changed toggle must not strand the saved thread. Sandbox, permission + // profile, and command changes stay protected. + let saved_args: Vec = vec![ + "-c", + "default_permissions=\"paperclip-runner-workspace-only\"", + "-c", + "shell_environment_policy.set={PATH=\"/run/a\"}", + "--disable", + "image_generation", + "app-server", + ] + .into_iter() + .map(str::to_owned) + .collect(); + let current_args: Vec = vec![ + "-c", + "default_permissions=\"paperclip-runner-workspace-only\"", + "-c", + "shell_environment_policy.set={PATH=\"/run/b\"}", + "app-server", + ] + .into_iter() + .map(str::to_owned) + .collect(); + let mut settled = opencode_result_state(); + settled.config.provider = "codex".to_owned(); + settled.config.driver = "codex_app_server".to_owned(); + settled.config.command = PathBuf::from("must-not-start-during-attachment"); + settled.config.model = None; + settled.config.args = saved_args.clone(); + settled.active_provider_turn_id = None; + settled.provider_process_generation = 1; + settled.lifecycle = "prepared".to_owned(); + + let directory = std::env::temp_dir().join(format!( + "paperclip-provider-feature-policy-{}", + uuid::Uuid::new_v4() + )); + fs::create_dir_all(&directory).unwrap(); + let mut executor = CodexCommandExecutor::new(&directory); + executor.state = Some(settled.clone()); + executor + .attach_run(&json!({ + "provider": settled.config.clone(), + "runtimeLaunchArgs": current_args.clone(), + })) + .expect("a saved thread must follow the current feature policy"); + let rebound = executor.state.as_ref().unwrap(); + assert_eq!(rebound.config.args, current_args); + assert_eq!(rebound.thread_id.as_deref(), Some("thread-1")); + assert_eq!(rebound.lifecycle, "prepared"); + assert!(executor.provider.is_none()); + fs::remove_dir_all(directory).unwrap(); + + // Tightening the policy follows the controller as well. + let mut tightened = settled.clone(); + tightened.config.args = current_args.clone(); + let mut disabled = current_args.clone(); + disabled.splice(4..4, ["--disable".to_owned(), "memories".to_owned()]); + let directory = std::env::temp_dir().join(format!( + "paperclip-provider-feature-tightening-{}", + uuid::Uuid::new_v4() + )); + fs::create_dir_all(&directory).unwrap(); + let mut executor = CodexCommandExecutor::new(&directory); + executor.state = Some(tightened.clone()); + executor + .attach_run(&json!({ + "provider": tightened.config.clone(), + "runtimeLaunchArgs": disabled.clone(), + })) + .unwrap(); + assert_eq!(executor.state.as_ref().unwrap().config.args, disabled); + fs::remove_dir_all(directory).unwrap(); + + // A release that turns a feature on is adopted too, in either flag form. + for enabled_form in [ + vec!["--enable".to_owned(), "goals".to_owned()], + vec!["--enable=goals".to_owned()], + ] { + let mut enabled = current_args.clone(); + enabled.splice(4..4, enabled_form); + let directory = std::env::temp_dir().join(format!( + "paperclip-provider-feature-enable-{}", + uuid::Uuid::new_v4() + )); + fs::create_dir_all(&directory).unwrap(); + let mut executor = CodexCommandExecutor::new(&directory); + executor.state = Some(tightened.clone()); + executor + .attach_run(&json!({ + "provider": tightened.config.clone(), + "runtimeLaunchArgs": enabled.clone(), + })) + .unwrap(); + let rebound = executor.state.as_ref().unwrap(); + assert_eq!(rebound.config.args, enabled); + assert_eq!(rebound.thread_id.as_deref(), Some("thread-1")); + fs::remove_dir_all(directory).unwrap(); + } + + let mut sandbox_bypass = current_args.clone(); + sandbox_bypass.insert(4, "--dangerously-bypass-approvals-and-sandbox".to_owned()); + let mut permission_profile = current_args.clone(); + permission_profile[1] = + "default_permissions=\"paperclip-runner-external-sandbox\"".to_owned(); + let mut subcommand = current_args.clone(); + subcommand[4] = "exec".to_owned(); + for changed in [sandbox_bypass, permission_profile, subcommand] { + let before = serde_json::to_value(&settled).unwrap(); + let mut executor = CodexCommandExecutor::new("unused-protected-launch-change"); + executor.state = Some(settled.clone()); + let error = executor + .attach_run(&json!({ + "provider": settled.config.clone(), + "runtimeLaunchArgs": changed, + })) + .expect_err("protected launch arguments must stay immutable"); + assert!( + error.to_string().contains("protected launch arguments"), + "{error}" + ); + assert_eq!( + serde_json::to_value(executor.state.as_ref().unwrap()).unwrap(), + before + ); + } + } + #[test] fn stopped_checkpoint_rebinding_retains_undelivered_facts_for_the_prior_authority() { let directory = std::env::temp_dir().join(format!( 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 b23adf70231..9a83c92d94f 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 @@ -4468,6 +4468,120 @@ fn durable_backend_settles_pending_tools_when_recovery_finds_the_turn_ended() { fs::remove_dir_all(directory).expect("remove Codex integration-test directory"); } +fn drain_completed_turn(executor: &mut CodexCommandExecutor, label: &str) { + let mut turn_completed = false; + let deadline = event_deadline(); + while std::time::Instant::now() < deadline { + let events = poll_and_ack(executor).expect(label); + turn_completed |= events + .iter() + .any(|event| event.event_type == "turn.completed"); + if turn_completed && events.is_empty() { + return; + } + if events.is_empty() { + std::thread::sleep(std::time::Duration::from_millis(1)); + } + } + panic!("{label}: the turn did not complete before the deadline"); +} + +#[test] +fn saved_codex_thread_attaches_after_a_release_changes_its_feature_toggles() { + // A thread saved by an earlier release keeps that release's feature + // toggles. The next run's controller sends the current policy, which no + // longer disables image generation. Attachment must adopt the current + // toggles and restart the provider on the same thread. + let directory = temporary_directory("feature-policy-attach"); + let mut config = provider_config( + &directory, + &["--durable-turn-ids", "--record-process-start"], + ); + let current_args = config.args.clone(); + config + .args + .extend(["--disable".to_owned(), "image_generation".to_owned()]); + let runner_config = durable_config(&directory); + let mut executor = CodexCommandExecutor::with_runner_config(&directory, &runner_config); + executor + .execute(&command( + "prepare", + 1, + "run.prepare", + json!({"provider": config, "authorizedTools": task_context_tool_set()}), + )) + .expect("prepare the thread with the earlier release's toggles"); + executor + .execute(&command("open", 2, "session.open", json!({}))) + .expect("open the thread"); + executor + .execute(&command( + "first-turn", + 3, + "turn.start", + json!({"text": "Work under the earlier release."}), + )) + .expect("start the first turn"); + drain_completed_turn(&mut executor, "drain the first turn"); + + let mut protected = current_args.clone(); + protected.push("--dangerously-bypass-approvals-and-sandbox".to_owned()); + let error = executor + .execute(&command( + "protected-attach", + 4, + "run.attach", + json!({ + "authorizedTools": task_context_tool_set(), + "runtimeLaunchArgs": protected, + }), + )) + .expect_err("a sandbox change stays protected"); + assert!( + error.to_string().contains("protected launch arguments"), + "unexpected attach error: {error}" + ); + + let attached = executor + .execute(&command( + "attach", + 5, + "run.attach", + json!({ + "authorizedTools": task_context_tool_set(), + "runtimeLaunchArgs": current_args, + }), + )) + .expect("the saved thread adopts the current feature toggles"); + assert!(attached + .events + .iter() + .any(|(event_type, _, _)| event_type == "run.attached")); + + executor + .execute(&command( + "second-turn", + 6, + "turn.start", + json!({"text": "Continue under the current release."}), + )) + .expect("start a turn on the same thread after attachment"); + drain_completed_turn(&mut executor, "drain the second turn"); + + let calls = fs::read_to_string(directory.join("calls.log")).expect("read the call log"); + assert_eq!( + calls + .lines() + .filter(|line| *line == "process-start") + .count(), + 2, + "the provider restarts once to apply the current toggles: {calls}" + ); + + executor.shutdown().expect("stop the provider"); + fs::remove_dir_all(directory).expect("remove Codex integration-test directory"); +} + #[test] fn durable_backend_rotates_tool_authority_for_fresh_run_attach() { let directory = temporary_directory("durable-tool-attach-rotation");