From 5ce14326ae3da23855ef2cf73be02f19458c268f Mon Sep 17 00:00:00 2001 From: lawrencecchen <54008264+lawrencecchen@users.noreply.github.com> Date: Sat, 31 Oct 2026 04:09:46 +0000 Subject: [PATCH 1/2] fix(chief toolpacks): the MCP launchers read secrets from the store main passes (toolpack_mcp red on macOS) enabling_a_pack_adds_its_skills_and_servers_and_disabling_removes_them has been red on every macOS run since 2026-10-24: the test wrote the Acme token to a FileStore and ran the optchat-chief binary, whose `pack mcp` took the store from toolpacks::store(), the login keychain on macOS ("secret:acme: this Chief has no Acme token"; tests/toolpack_mcp.rs:219). serve_mcp and mcp_headers now take the store (StoreOf) and serve_mcp its input and output as parameters; main, the composition root, passes the Chief home's toolpacks::store and the process's stdin and stdout, so the binary behaves as before. The test calls serve_mcp in-process with the FileStore it wrote. No settable or static hook. Red before: fleet optchat-chief runs at feat 038bf00cc91 (step 32985ab9d258616a8e4bcaaf, aws-m4pro-4) and at 3c94857e859 (step 52751e3eef0eaaade0c87d33). --- Native/OptChat/optchat-chief/src/main.rs | 14 ++++- Native/OptChat/optchat-chief/src/toolpacks.rs | 35 ++++++++--- .../optchat-chief/tests/toolpack_mcp.rs | 58 ++++++++++++++----- 3 files changed, 81 insertions(+), 26 deletions(-) diff --git a/Native/OptChat/optchat-chief/src/main.rs b/Native/OptChat/optchat-chief/src/main.rs index c961ead77ec2..484dc7c59702 100644 --- a/Native/OptChat/optchat-chief/src/main.rs +++ b/Native/OptChat/optchat-chief/src/main.rs @@ -84,10 +84,20 @@ fn main() -> std::process::ExitCode { flags.words.get(3).cloned().unwrap_or_default(), ); let home = home(&flags); + // The composition root of the launchers: this Chief home's store, the process's stdio. + let store_of = + |p: &optchat_chief::toolpacks::Pack| optchat_chief::toolpacks::store(&home, p); let out = if args[1] == "mcp" { - optchat_chief::toolpacks::serve_mcp(&home, &pack, &server) + optchat_chief::toolpacks::serve_mcp( + &home, + &pack, + &server, + &store_of, + Box::new(std::io::BufReader::new(std::io::stdin())), + Box::new(std::io::stdout()), + ) } else { - optchat_chief::toolpacks::mcp_headers(&home, &pack, &server).map(|h| { + optchat_chief::toolpacks::mcp_headers(&home, &pack, &server, &store_of).map(|h| { println!("{h}"); 0 }) diff --git a/Native/OptChat/optchat-chief/src/toolpacks.rs b/Native/OptChat/optchat-chief/src/toolpacks.rs index a7139fa8c49a..dfb0060ecc4e 100644 --- a/Native/OptChat/optchat-chief/src/toolpacks.rs +++ b/Native/OptChat/optchat-chief/src/toolpacks.rs @@ -591,12 +591,17 @@ pub fn codex_servers(packs: &[Pack], bin: &Path) -> toml::Table { out } -fn secret_of(mux_home: &Path, packs: &[Pack], name: &str) -> Result { +/// Where the launchers read a pack's secret. The composition root (`main`) passes +/// [`store`] of the Chief home (the macOS login keychain, else a 0600 file); a +/// test passes the store it wrote the secret into. +pub type StoreOf<'a> = &'a dyn Fn(&Pack) -> Box; + +fn secret_of(store_of: StoreOf<'_>, packs: &[Pack], name: &str) -> Result { let p = packs .iter() .find(|p| p.name == name) .ok_or_else(|| format!("secret:{name}: no such pack is on"))?; - store(mux_home, p).get().ok_or_else(|| { + store_of(p).get().ok_or_else(|| { format!( "secret:{name}: this Chief has no {} token (chief pack status {name})", p.title @@ -613,7 +618,12 @@ fn find_server<'a>(packs: &'a [Pack], pack: &str, server: &str) -> Result<&'a Mc } /// `pack mcp-headers PACK SERVER`: Claude Code's headersHelper answer. -pub fn mcp_headers(mux_home: &Path, pack: &str, server: &str) -> Result { +pub fn mcp_headers( + mux_home: &Path, + pack: &str, + server: &str, + store_of: StoreOf<'_>, +) -> Result { let packs = load(mux_home); let m = find_server(&packs, pack, server)?; let r = m @@ -621,13 +631,22 @@ pub fn mcp_headers(mux_home: &Path, pack: &str, server: &str) -> Result Result { +/// It speaks over `input` and `output` (`main` passes the process's stdin and +/// stdout) and reads secrets from `store_of`. +pub fn serve_mcp( + mux_home: &Path, + pack: &str, + server: &str, + store_of: StoreOf<'_>, + input: Box, + output: Box, +) -> Result { use std::io::BufRead; let packs = load(mux_home); let m = find_server(&packs, pack, server)?.clone(); @@ -635,7 +654,7 @@ pub fn serve_mcp(mux_home: &Path, pack: &str, server: &str) -> Result cmd.env(k, secret_of(mux_home, &packs, r)?), + Some(r) => cmd.env(k, secret_of(store_of, &packs, r)?), None => cmd.env(k, v), }; } @@ -648,14 +667,14 @@ pub fn serve_mcp(mux_home: &Path, pack: &str, server: &str) -> Result::new())); let mut to_child = child.stdin.take().ok_or("no stdin")?; let from_child = child.stdout.take().ok_or("no stdout")?; let allowed = m.tools.clone(); let (out2, lists2) = (out.clone(), lists.clone()); let reader = std::thread::spawn(move || { - for line in std::io::stdin().lock().lines() { + for line in input.lines() { let Ok(line) = line else { break }; let msg: Value = serde_json::from_str(&line).unwrap_or(Value::Null); match msg["method"].as_str() { diff --git a/Native/OptChat/optchat-chief/tests/toolpack_mcp.rs b/Native/OptChat/optchat-chief/tests/toolpack_mcp.rs index 4d57fa57fdba..3d84e6028af4 100644 --- a/Native/OptChat/optchat-chief/tests/toolpack_mcp.rs +++ b/Native/OptChat/optchat-chief/tests/toolpack_mcp.rs @@ -8,12 +8,32 @@ use std::io::Write; use std::os::unix::fs::PermissionsExt; use std::path::Path; -use std::process::{Command, Stdio}; +use std::sync::{Arc, Mutex}; use optchat_chief::prompt::Tools; use optchat_chief::session_dir::{SessionSetup, write}; use optchat_chief::toolpacks::{self, FileStore, Store}; +/// What the launcher wrote, shared with the test. +#[derive(Clone, Default)] +struct Captured(Arc>>); + +impl Write for Captured { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0.lock().unwrap().extend_from_slice(buf); + Ok(buf.len()) + } + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +impl Captured { + fn text(&self) -> String { + String::from_utf8(self.0.lock().unwrap().clone()).unwrap() + } +} + const SECRET: &str = "acme_0123456789abcdefABCDEF0123"; fn acme(enabled: bool, server: &Path) -> String { @@ -197,21 +217,27 @@ fn enabling_a_pack_adds_its_skills_and_servers_and_disabling_removes_them() { ); } - // The launcher hands the secret to the server and keeps only the allowed tools. - let mut child = Command::new(env!("CARGO_BIN_EXE_optchat-chief")) - .args(["pack", "mcp", "acme", "acme", "--mux-home"]) - .arg(home) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .spawn() - .unwrap(); - child - .stdin - .take() - .unwrap() - .write_all(b"{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"tools/list\"}\n{\"jsonrpc\":\"2.0\",\"id\":3,\"method\":\"tools/call\",\"params\":{\"name\":\"acme_delete\"}}\n{\"jsonrpc\":\"2.0\",\"id\":2,\"method\":\"tools/call\",\"params\":{\"name\":\"acme_read\"}}\n") - .unwrap(); - let out = String::from_utf8(child.wait_with_output().unwrap().stdout).unwrap(); + // The launcher hands the secret to the server and keeps only the allowed tools. It reads + // the secret from the store it is given: this test's file store (the binary's `main` passes + // the Chief home's own, the login keychain on macOS). + let output = Captured::default(); + let input = b"{\"jsonrpc\":\"2.0\",\"id\":1,\"method\":\"tools/list\"}\n{\"jsonrpc\":\"2.0\",\"id\":3,\"method\":\"tools/call\",\"params\":{\"name\":\"acme_delete\"}}\n{\"jsonrpc\":\"2.0\",\"id\":2,\"method\":\"tools/call\",\"params\":{\"name\":\"acme_read\"}}\n"; + let file_store = |p: &toolpacks::Pack| -> Box { + Box::new(FileStore { + path: toolpacks::secret_file(home, &p.name), + pack: p.clone(), + }) + }; + toolpacks::serve_mcp( + home, + "acme", + "acme", + &file_store, + Box::new(std::io::Cursor::new(input.to_vec())), + Box::new(output.clone()), + ) + .unwrap(); + let out = output.text(); let lines: Vec = out .lines() .map(|l| serde_json::from_str(l).unwrap()) From e854cfd0861c9ee75a1fb6e780a24f788e23ea5d Mon Sep 17 00:00:00 2001 From: lawrencecchen <54008264+lawrencecchen@users.noreply.github.com> Date: Sat, 31 Oct 2026 15:35:56 +0000 Subject: [PATCH 2/2] fix(chief toolpacks): remove the debug-build file-store seam; the test passes its store to serve_mcp 043454696ac added OPTCHAT_CHIEF_TEST_FILE_SECRETS so the spawned launcher read the file store on macOS. After the merge of feat, the test calls serve_mcp in-process with its own FileStore, so nothing sets the variable. The seam was the only env-read path to a file store in toolpacks::store. --- Native/OptChat/optchat-chief/src/toolpacks.rs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/Native/OptChat/optchat-chief/src/toolpacks.rs b/Native/OptChat/optchat-chief/src/toolpacks.rs index 57335f67bb50..dfb0060ecc4e 100644 --- a/Native/OptChat/optchat-chief/src/toolpacks.rs +++ b/Native/OptChat/optchat-chief/src/toolpacks.rs @@ -877,16 +877,6 @@ impl Store for FileStore { /// The store of a pack in this Chief home on this machine. pub fn store(home: &Path, pack: &Pack) -> Box { - // Debug builds only: with `OPTCHAT_CHIEF_TEST_FILE_SECRETS` set, the file store, so a test - // on macOS reads the secret it wrote and never the host's login keychain (cx-71g32y, - // tests/toolpack_mcp.rs). A release build has no seam: on macOS it reads the keychain. - #[cfg(debug_assertions)] - if std::env::var_os("OPTCHAT_CHIEF_TEST_FILE_SECRETS").is_some() { - return Box::new(FileStore { - path: secret_file(home, &pack.name), - pack: pack.clone(), - }); - } if cfg!(target_os = "macos") && Path::new("/usr/bin/security").exists() { Box::new(Keychain { account: crate::paths::home_id(home),