From 549ca46daef79b61f2369d966671e14507f14480 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Mon, 9 Nov 2026 21:42:46 -0800 Subject: [PATCH] fix(runner): retain Claude GitHub broker settings through the Rust sidecar (#20463) ## Thinking Path > - Paperclip lets people manage AI agents for work. > - Shared agents use the instructing person's GitHub identity for managed commands. > - Native Claude starts through the Rust daemon and a TypeScript ACPX sidecar. > - The Rust launch filter kept the command launchers but removed their broker transport. > - Shell commands then reported `GitHub capability_missing`, although connector tools worked. > - This pull request retains that transport for managed Claude launches. > - Each command can acquire its current identity through the existing broker checks. ## Linked Issues or Issue Description **What happened?** Native Claude could use GitHub connector tools but could not authenticate `git` or `gh`. The Rust daemon removed the broker URL and session/bridge capabilities before starting the ACPX sidecar. Reconnecting the account did not repair this process environment. **Expected behavior** Managed shell commands use the current authorized GitHub identity. The provider receives broker capabilities, while raw GitHub tokens remain confined to individual commands. **Steps to reproduce** 1. Start native Claude through the Rust Runner with managed GitHub launchers. 2. Run `gh api user` and a private repository ref lookup through Bash. 3. Observe `capability_missing` and no request to the broker. **Paperclip version or commit** Reproduced from source at `2c2186706ef4bb352b662f9d8faebefe47fb52ac`. The repair is based on `a5e01634f69f18dfe10a0bf709df02d9e6a2135b`. **Deployment mode** Native Claude through ACPX. Local acceptance used macOS arm64 and Node 25.6.1. The reported failure also affected a Linux sandbox. Related: #18212 preserved launcher paths. #18850 repaired the TypeScript filter. This change repairs the preceding Rust filter. The duplicate search found no open repair for this boundary. ROADMAP.md already lists managed personal GitHub identities. ## What Changed - Retain the broker URL and both broker capabilities when Claude has a nonempty managed launcher directory. - Keep raw GitHub tokens, unrelated secrets, empty launcher configurations, and sibling ACPX providers outside that exception. - Exercise the regression through the public sidecar initialize protocol in the existing Rust integration suite. Document the restart requirement. The Rust sidecar transport owns this filter. Credential resolution, company access, approval, and run identity policies keep their existing owners. No schema or API changes are needed. ## Verification - Red/green: the subprocess regression failed on the missing `PAPERCLIP_GITHUB_BROKER_URL` before the fix, then passed with all three broker settings and the negative controls. - Passed before moving the regression: 615 Rust core tests, with 3 existing ignored tests. After moving it into the integration suite, all 20 sidecar integration tests and the 9 remaining sidecar unit controls pass. The moved regression was also run against the unfixed source and failed on the missing broker URL. - Passed: 85 TypeScript environment, launcher, sandbox, and configured-environment controls. These include operation identity changes, revocation, recovery, and exclusion of capabilities from persisted environment data. - Passed: Runner Rust typecheck and format check, plus the Runner build through `node scripts/agent-preflight.mjs --skip-rust-debug`. - Live before/after: real `claude-opus-5-5` through the actual Rust daemon and built ACPX sidecar reproduced the failure with the old binary. Neither shell command reached the broker. With the fixed binary, `gh` identified the connected account and `git` read private repository refs. A second turn retained a workspace file and authenticated again. After the controlled broker denied access, the next command failed without reusing the prior token. No GitHub writes were made. - Live release cutover: saved a session with the unfixed daemon, suspended it, and restored it with the repaired daemon on `aa9f5b30614c011ebc3fe4509e11173b766c1fea`. The same session and workspace marker survived. `gh` authenticated after restoration, then failed when the controlled broker revoked access. - Live acceptance used an Anthropic API credential and a loopback broker with an authorized operator GitHub credential. This isolates the repaired transport; it does not qualify the hosted subscription credential or production grant resolver. - The first PR review gate did not recognize the in-source Rust test. The regression now lives in the existing integration test file, and the same test-coverage gate passes locally. - `pnpm verify` returned its documented exit 2 and required broad CI coverage. The final head passed the full CI typecheck, build, and test lanes. - Greptile reviewed `64cca69b76073cc3185b28334465dee48fbadb29` at 5/5 with no actionable findings or open review threads. Its check passed on that head. The same head is mergeable with no conflicts. - Final head `64cca69b76073cc3185b28334465dee48fbadb29` passed [full PR CI](https://github.com/paperclipai/paperclip/actions/runs/42761401396), [Windows Rust](https://github.com/paperclipai/paperclip/actions/runs/42761401117), and [isolated Runner compilation](https://github.com/paperclipai/paperclip/actions/runs/42761400973). Typecheck, build, all test shards, Runner checks, browser checks, and aggregate verification passed. No checks are pending or failed; five conditional jobs were skipped. ## Risks - The Claude sidecar now receives the intended broker capabilities. The existing broker must continue to enforce active-run identity and current connection access. Raw GitHub tokens remain excluded from the provider launch environment. - Previous-release checkpoint and profile formats carry forward unchanged. A running provider must retire and restart normally to inherit the repaired environment. No profile reset or data migration is required. - Hosted recovery requires delivery of the updated Runner binary. This PR does not deploy or retry existing tasks. Live acceptance has not exercised Linux sandbox confinement or a GitHub write. ## Model Used OpenAI `gpt-6.1-sol` through Codex, with xhigh reasoning. Used repository inspection, code editing, and shell execution. The context window size is not exposed. Anthropic `claude-opus-5-5` was exercised as the acceptance-test provider. ## 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 --- doc/execution-github-identity.md | 9 +- .../runner-core/src/acpx_sidecar_transport.rs | 49 ++-------- .../tests/acpx_sidecar_transport.rs | 92 +++++++++++++++++++ 3 files changed, 109 insertions(+), 41 deletions(-) diff --git a/doc/execution-github-identity.md b/doc/execution-github-identity.md index 90ef3e30874..86b5a3c3a1a 100644 --- a/doc/execution-github-identity.md +++ b/doc/execution-github-identity.md @@ -64,13 +64,20 @@ the active run through that broker. They do not require a persistent GitHub CLI login or inherit an ambient host broker capability. Claude through native ACPX retains the controller-projected broker URL and -session/bridge capabilities alongside its managed launcher paths. Both isolated +session/bridge capabilities alongside its managed launcher paths through both +the Rust daemon-to-sidecar and TypeScript provider launch boundaries. Both isolated and preconfigured homes use this transport for shell `git` and `gh` operations. These capabilities stay out of persisted session environment data. Raw GitHub tokens remain confined to each managed command; ambient host capabilities and task-supplied broker overrides are excluded. A working connector tool alone does not prove that the terminal received its broker transport. +The Rust sidecar launch accepts this transport only for Claude with a nonempty +managed launcher directory. Existing saved sessions retain their profile and +checkpoint format. A running provider needs its normal retirement and restart +to receive the repaired launch environment; reconnecting the GitHub account +does not repair a process that already started without the transport. + Without a managed identity, local commits default to `Paperclip `. The launcher writes this token-free default to a private per-operation global Git configuration file, removed on exit. diff --git a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_sidecar_transport.rs b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_sidecar_transport.rs index 631a61b3c9b..b84526d397c 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/src/acpx_sidecar_transport.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/src/acpx_sidecar_transport.rs @@ -271,6 +271,15 @@ impl AcpxSidecarTransport { keys.push("PAPERCLIP_ACPX_CREDENTIAL_BINDING"); } if agent == "claude" { + if std::env::var("PAPERCLIP_GITHUB_LAUNCHER_DIR") + .is_ok_and(|directory| !directory.trim().is_empty()) + { + keys.extend_from_slice(&[ + "PAPERCLIP_GITHUB_BROKER_URL", + "PAPERCLIP_GITHUB_BROKER_TOKEN", + "PAPERCLIP_GITHUB_BRIDGE_TOKEN", + ]); + } keys.extend_from_slice(&[ // Launcher/controller-owned helper paths; credentials stay behind the broker. "BASH_ENV", @@ -1290,46 +1299,6 @@ mod tests { use super::*; - #[test] - #[cfg(unix)] - fn managed_claude_helpers_reach_the_sidecar_without_host_credentials() { - const CHILD: &str = "PAPERCLIP_TEST_MANAGED_HELPER_CHILD"; - if std::env::var_os(CHILD).is_none() { - let output = std::process::Command::new(std::env::current_exe().unwrap()) - .args(["--exact", "acpx_sidecar_transport::tests::managed_claude_helpers_reach_the_sidecar_without_host_credentials"]) - .env_clear() - .env(CHILD, "1") - .env("PATH", "/usr/bin:/bin") - .env("BASH_ENV", "/managed/.bashrc") - .env("ZDOTDIR", "/managed") - .env("GH_CONFIG_DIR", "/managed") - .env("PAPERCLIP_GITHUB_LAUNCHER_DIR", "/managed") - .env("GH_TOKEN", "unbound-secret") - .env("PAPERCLIP_GITHUB_BROKER_TOKEN", "unbound-capability") - .output() - .unwrap(); - assert!( - output.status.success(), - "{}", - String::from_utf8_lossy(&output.stdout) - ); - return; - } - let config = AcpxSidecarTransportConfig { - command: PathBuf::from("/bin/sh"), - args: vec!["-c".into(), "printf '%s|%s|%s|%s|%s|%s\\n' \"$BASH_ENV\" \"$ZDOTDIR\" \"$GH_CONFIG_DIR\" \"$PAPERCLIP_GITHUB_LAUNCHER_DIR\" \"$GH_TOKEN\" \"$PAPERCLIP_GITHUB_BROKER_TOKEN\"".into()], - verified_launch: None, - request_timeout: Duration::from_secs(1), - shutdown_grace: Duration::from_millis(100), - }; - let mut transport = AcpxSidecarTransport::start_for_agent(&config, "claude").unwrap(); - let line = transport - .receive_stdout_line(Duration::from_secs(2), "managed-helper test") - .unwrap() - .expect("sidecar did not report its environment"); - assert_eq!(line, "/managed/.bashrc|/managed|/managed|/managed||"); - } - #[test] fn parse_frame_does_not_echo_untrusted_deserialization_details() { let cases = [ diff --git a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_sidecar_transport.rs b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_sidecar_transport.rs index f6264141dd1..1b04751a440 100644 --- a/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_sidecar_transport.rs +++ b/packages/paperclip-runner/runner/crates/runner-core/tests/acpx_sidecar_transport.rs @@ -4,6 +4,8 @@ use std::time::{Duration, Instant}; use paperclip_runner_core::acpx_sidecar_transport::{ AcpxSidecarTransport, AcpxSidecarTransportConfig, }; +#[cfg(unix)] +use paperclip_runner_core::generated_acpx_sidecar_contract::GENERATED_ACPX_SIDECAR_PROTOCOL_VERSION; use paperclip_runner_core::generated_acpx_sidecar_contract::{ GeneratedAcpxSidecarCommand, GeneratedAcpxSidecarEventType, }; @@ -521,3 +523,93 @@ fn pi_ordinary_request_timeout_still_retires_the_sidecar() { .shutdown() .expect("timeout cleanup remains idempotent"); } + +#[test] +#[cfg(unix)] +fn managed_claude_broker_transport_reaches_the_sidecar_without_host_credentials() { + const CHILD: &str = "PAPERCLIP_TEST_MANAGED_BROKER_CHILD"; + if std::env::var_os(CHILD).is_none() { + for (agent, launcher) in [ + ("claude", "/managed"), + ("claude", ""), + ("claude", " "), + ("codex", "/managed"), + ("cursor", "/managed"), + ] { + let output = std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "managed_claude_broker_transport_reaches_the_sidecar_without_host_credentials", + ]) + .env_clear() + .env(CHILD, agent) + .env("PATH", "/usr/bin:/bin") + .env("BASH_ENV", "/managed/.bashrc") + .env("ZDOTDIR", "/managed") + .env("GH_CONFIG_DIR", "/managed") + .env("PAPERCLIP_GITHUB_LAUNCHER_DIR", launcher) + .env("PAPERCLIP_GITHUB_BROKER_URL", "http://127.0.0.1:3210") + .env("PAPERCLIP_GITHUB_BROKER_TOKEN", "session-capability") + .env("PAPERCLIP_GITHUB_BRIDGE_TOKEN", "bridge-capability") + .env("GH_TOKEN", "unbound-secret") + .env("GITHUB_TOKEN", "unbound-secret") + .env("UNRELATED_SECRET", "unbound-secret") + .output() + .unwrap(); + assert!( + output.status.success(), + "agent={agent}, launcher={launcher:?}: {} {}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + } + return; + } + let agent = std::env::var(CHILD).unwrap(); + let names = [ + "BASH_ENV", + "ZDOTDIR", + "GH_CONFIG_DIR", + "PAPERCLIP_GITHUB_LAUNCHER_DIR", + "PAPERCLIP_GITHUB_BROKER_URL", + "PAPERCLIP_GITHUB_BROKER_TOKEN", + "PAPERCLIP_GITHUB_BRIDGE_TOKEN", + "GH_TOKEN", + "GITHUB_TOKEN", + "UNRELATED_SECRET", + ]; + let arguments = names.map(|name| format!("\"${name}\"")).join(" "); + let response = json!({ + "protocolVersion": GENERATED_ACPX_SIDECAR_PROTOCOL_VERSION, + "id": 1, "ok": true, "result": { "environment": vec!["%s"; names.len()] }, + }); + let config = AcpxSidecarTransportConfig { + command: PathBuf::from("/bin/sh"), + args: vec![ + "-c".into(), + format!("IFS= read -r request; printf '{response}\\n' {arguments}; IFS= read -r next"), + ], + verified_launch: None, + request_timeout: Duration::from_secs(1), + shutdown_grace: Duration::from_millis(100), + }; + let mut transport = AcpxSidecarTransport::start_for_agent(&config, &agent).unwrap(); + let managed = agent == "claude" + && !std::env::var("PAPERCLIP_GITHUB_LAUNCHER_DIR") + .unwrap() + .trim() + .is_empty(); + let response = transport + .request(GeneratedAcpxSidecarCommand::Initialize, json!({})) + .expect("sidecar should report its launch environment"); + for (index, name) in names.iter().enumerate() { + let value = response["environment"][index].as_str().unwrap(); + let expected = if (index < 4 && agent == "claude") || ((4..7).contains(&index) && managed) { + std::env::var(name).unwrap() + } else { + String::new() + }; + assert_eq!(value, expected, "sidecar environment: {name}"); + } + transport.shutdown().unwrap(); +}