diff --git a/src/agent_author.rs b/src/agent_author.rs index 46723435..5d54e131 100644 --- a/src/agent_author.rs +++ b/src/agent_author.rs @@ -1263,7 +1263,7 @@ mod tests { &declaration("worker", "h", None, "catalog"), ); - let added = add_resource( + let added = add_resource_with_selector( root, "h.worker", "h", @@ -1272,13 +1272,14 @@ mod tests { "github-issue://example/project/123", "release work item", None, + None, ) .unwrap(); assert_eq!(added.result, AuthorOutcome::Changed); assert_eq!(added.identity, "h.worker"); assert_eq!(added.inactive_reason, None); - add_resource( + add_resource_with_selector( root, "h.worker", "h", @@ -1287,13 +1288,14 @@ mod tests { "worktree://github.com/example/project/change", "primary checkout", None, + None, ) .unwrap(); let two_bindings = fs::read_to_string(&path).unwrap(); // An identical request proves the binding rather than rewriting the declaration. assert_eq!( - add_resource( + add_resource_with_selector( root, "h.worker", "h", @@ -1302,6 +1304,7 @@ mod tests { "github-issue://example/project/123", "release work item", None, + None, ) .unwrap() .result, @@ -1311,7 +1314,7 @@ mod tests { // Re-declaring an existing name updates it in place, keeping its position and siblings. assert_eq!( - add_resource( + add_resource_with_selector( root, "h.worker", "h", @@ -1320,6 +1323,7 @@ mod tests { "github-issue://example/project/456", "follow-up work item", Some("superseded by the follow-up"), + None, ) .unwrap() .result, @@ -1345,7 +1349,7 @@ mod tests { // The request declares the complete binding, so an omitted inactive-reason clears it. assert_eq!( - add_resource( + add_resource_with_selector( root, "h.worker", "h", @@ -1354,6 +1358,7 @@ mod tests { "github-issue://example/project/456", "follow-up work item", None, + None, ) .unwrap() .result, @@ -1379,7 +1384,7 @@ mod tests { // Hand-authored property order and spacing are proven, not re-rendered. assert_eq!( - add_resource( + add_resource_with_selector( root, "h.worker", "h", @@ -1388,6 +1393,7 @@ mod tests { "github-issue://example/project/123", "release work item", None, + None, ) .unwrap() .result, @@ -1405,7 +1411,7 @@ mod tests { "h/worker/agent.kdl", &declaration("worker", "h", None, "catalog"), ); - add_resource( + add_resource_with_selector( root, "h.worker", "h", @@ -1414,9 +1420,10 @@ mod tests { "github-issue://example/project/123", "release work item", None, + None, ) .unwrap(); - add_resource( + add_resource_with_selector( root, "h.worker", "h", @@ -1425,6 +1432,7 @@ mod tests { "worktree://github.com/example/project/change", "primary checkout", None, + None, ) .unwrap(); @@ -1461,7 +1469,7 @@ mod tests { "h/worker/agent.kdl", &declaration("worker", "h", None, "catalog"), ); - add_resource( + add_resource_with_selector( root, "h.worker", "h", @@ -1470,9 +1478,10 @@ mod tests { "github-issue://example/project/123", "release work item", Some("merged and retained for traceability"), + None, ) .unwrap(); - add_resource( + add_resource_with_selector( root, "h.worker", "h", @@ -1481,6 +1490,7 @@ mod tests { "worktree://github.com/example/project/change", "primary checkout", None, + None, ) .unwrap(); let before = fs::read_to_string(&path).unwrap(); @@ -1561,7 +1571,7 @@ mod tests { ); let untouched = fs::read_to_string(&nix_owned).unwrap(); - add_resource( + add_resource_with_selector( root, "h.child", "h", @@ -1570,11 +1580,12 @@ mod tests { "github-issue://example/project/1", "supervised work item", None, + None, ) .unwrap(); assert_eq!( - add_resource( + add_resource_with_selector( root, "h.sibling", "h", @@ -1583,6 +1594,7 @@ mod tests { "github-issue://example/project/1", "reaching across the fleet", None, + None, ) .unwrap_err() .code(), @@ -1595,7 +1607,7 @@ mod tests { "resource-not-authorized" ); assert_eq!( - add_resource( + add_resource_with_selector( root, "h.nix", "h", @@ -1604,6 +1616,7 @@ mod tests { "github-issue://example/project/1", "Nix owns this declaration", None, + None, ) .unwrap_err() .code(), @@ -1625,7 +1638,7 @@ mod tests { Some(""), ), ] { - let error = add_resource( + let error = add_resource_with_selector( root, "h.child", "h", @@ -1634,6 +1647,7 @@ mod tests { uri, reason, inactive_reason, + None, ) .unwrap_err(); assert_eq!(error.code(), "invalid-resource", "{name}: {error}"); @@ -1647,7 +1661,7 @@ mod tests { ); // #345 widened the envelope: a catalog-relative carrier path is a valid binding uri. - add_resource( + add_resource_with_selector( root, "h.child", "h", @@ -1656,6 +1670,7 @@ mod tests { "carriers/goal.md", "Catalog-relative carrier.", None, + None, ) .expect("a catalog-relative carrier path is admitted"); assert_eq!(fs::read_to_string(&nix_owned).unwrap(), untouched); @@ -1672,7 +1687,7 @@ mod tests { ); let exact = "vendor+Thing://Authority.Example/Exact%20Identity?Query=A%2Fb#Frag%20Ment"; - add_resource( + add_resource_with_selector( root, "h.worker", "h", @@ -1681,6 +1696,7 @@ mod tests { exact, "exact vendor identity", None, + None, ) .unwrap(); assert_eq!(bound(root, "worker", "subject").uri(), exact); @@ -1691,7 +1707,7 @@ mod tests { // A byte-identical re-declaration is a proven no-op, not a rewrite. assert_eq!( - add_resource( + add_resource_with_selector( root, "h.worker", "h", @@ -1700,6 +1716,7 @@ mod tests { exact, "exact vendor identity", None, + None, ) .unwrap() .result, diff --git a/src/agent_author/resource.rs b/src/agent_author/resource.rs index c644633c..8c948c76 100644 --- a/src/agent_author/resource.rs +++ b/src/agent_author/resource.rs @@ -39,30 +39,6 @@ pub struct ResourceRenameReceipt { /// /// st2 preserves the binding for readers; it resolves nothing and grants nothing. `uri` is the /// exact absolute identity and is stored byte for byte with no normalization. -#[allow(clippy::too_many_arguments)] -pub fn add_resource( - catalog_root: &Path, - selector: &str, - this_host: &str, - actor: Option<&str>, - name: &str, - uri: &str, - reason: &str, - inactive_reason: Option<&str>, -) -> Result { - add_resource_with_selector( - catalog_root, - selector, - this_host, - actor, - name, - uri, - reason, - inactive_reason, - None, - ) -} - #[allow(clippy::too_many_arguments)] pub fn add_resource_with_selector( catalog_root: &Path, diff --git a/src/codex_app_server.rs b/src/codex_app_server.rs index 955fe5f5..c7e444b8 100644 --- a/src/codex_app_server.rs +++ b/src/codex_app_server.rs @@ -234,7 +234,7 @@ pub enum CodexObservedState { #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] -pub enum CodexHoldReason { +pub(crate) enum CodexHoldReason { ActiveWithoutTurn, ConflictingTurn, Review, @@ -249,7 +249,7 @@ pub enum CodexHoldReason { #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] -pub enum CodexTerminalError { +pub(crate) enum CodexTerminalError { SystemError, ProviderAuthRejected, } @@ -3002,7 +3002,8 @@ fn atomic_json(path: &Path, value: &impl Serialize) -> Result<()> { result } -pub fn load_current_binding( +#[cfg(test)] +fn load_current_binding( path: &Path, runtime: &CodexRuntime, ) -> Result> { @@ -3025,7 +3026,8 @@ pub fn load_current_binding( Ok(Some(binding)) } -pub fn load_current_control_state( +#[cfg(test)] +fn load_current_control_state( path: &Path, runtime: &CodexRuntime, binding: &CodexThreadBinding, diff --git a/src/context.rs b/src/context.rs index 761a6185..87d95264 100644 --- a/src/context.rs +++ b/src/context.rs @@ -122,13 +122,6 @@ fn lock_now(context_dir: &Path) -> anyhow::Result { .context("acquire now.md writer lock") } -/// Append one decision to the log. `decision` and `why` must be single non-empty lines (the log is a -/// scannable list; multi-line reasoning belongs in a doc). Renders `- . why: .` -/// into a fresh `decisions/-.md`. Returns the entry's filename. -pub fn append_decision(context_dir: &Path, decision: &str, why: &str) -> anyhow::Result { - append_decision_to_dir(&decisions_dir(context_dir), decision, why) -} - pub fn append_decision_to_dir(dir: &Path, decision: &str, why: &str) -> anyhow::Result { let decision = decision.trim(); let why = why.trim(); @@ -285,14 +278,14 @@ mod tests { fn append_decisions_are_ordered_bullets() { let tmp = tempfile::tempdir().unwrap(); let dir = context_dir(tmp.path()); - append_decision( - &dir, + append_decision_to_dir( + &decisions_dir(&dir), "use hook-enforced perms", "never prompts an autonomous pty", ) .unwrap(); std::thread::sleep(std::time::Duration::from_millis(2)); - append_decision(&dir, "defer shims", "scope enforcement is follow-on").unwrap(); + append_decision_to_dir(&decisions_dir(&dir), "defer shims", "scope enforcement is follow-on").unwrap(); let dec = read(&dir, View::Decisions); let lines: Vec<&str> = dec.lines().collect(); @@ -315,8 +308,8 @@ mod tests { fn append_rejects_empty_or_multiline() { let tmp = tempfile::tempdir().unwrap(); let dir = context_dir(tmp.path()); - assert!(append_decision(&dir, "", "why").is_err()); - assert!(append_decision(&dir, "d", "").is_err()); - assert!(append_decision(&dir, "line1\nline2", "why").is_err()); + assert!(append_decision_to_dir(&decisions_dir(&dir), "", "why").is_err()); + assert!(append_decision_to_dir(&decisions_dir(&dir), "d", "").is_err()); + assert!(append_decision_to_dir(&decisions_dir(&dir), "line1\nline2", "why").is_err()); } } diff --git a/src/ding/mod.rs b/src/ding/mod.rs index ef7403a6..eb6f668d 100644 --- a/src/ding/mod.rs +++ b/src/ding/mod.rs @@ -606,26 +606,6 @@ fn submit_retained_after_final_observation( observe_receipt_with_window(text, peek, poll, observation_window) } -#[allow(dead_code)] -fn observed_poke( - text: &str, - peek: &mut dyn FnMut() -> anyhow::Result, - stage: &mut dyn FnMut() -> anyhow::Result<()>, - submit: &mut dyn FnMut() -> anyhow::Result<()>, - poll: &mut dyn FnMut(), - before_submit: &mut dyn FnMut() -> anyhow::Result<()>, -) -> anyhow::Result { - observed_poke_with_window( - text, - peek, - stage, - submit, - poll, - before_submit, - COMPOSER_OBSERVATION_WINDOW, - ) -} - /// Two-phase DING delivery with injected operations for deterministic regression tests. fn observed_poke_with_window( text: &str, diff --git a/src/driver_diagnostic.rs b/src/driver_diagnostic.rs index d55df72b..47a237e7 100644 --- a/src/driver_diagnostic.rs +++ b/src/driver_diagnostic.rs @@ -624,7 +624,33 @@ fn emit( ); } +/// Create the staging sibling exclusively at `0600`. +/// +/// An existing regular file, a directory, or a symlink an agent planted at this path is refused +/// with `AlreadyExists` rather than followed or truncated. This directory is agent-writable, so +/// that refusal is the whole security property. +fn create_staging(path: &Path) -> std::io::Result { + use std::os::unix::fs::OpenOptionsExt as _; + + fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path) +} + +/// Durable replacement: the record's bytes reach disk before the rename and the directory entry is +/// synced after it. +/// +/// The staging file is created exclusively at `0600` under a name unique to this process *and* +/// write, so a stale or adversarial path cannot be followed or truncated and two writes to the +/// same agent directory cannot collide. Its sibling in `delivery_ledger` documents why that +/// matters; this helper used to be the one that did not do it. fn atomic_json(path: &Path, value: &impl Serialize) -> std::io::Result<()> { + use std::sync::atomic::{AtomicU64, Ordering}; + + static WRITE: AtomicU64 = AtomicU64::new(0); + let Some(parent) = path.parent() else { return Err(std::io::Error::new( std::io::ErrorKind::InvalidInput, @@ -632,15 +658,27 @@ fn atomic_json(path: &Path, value: &impl Serialize) -> std::io::Result<()> { )); }; fs::create_dir_all(parent)?; - let tmp = parent.join(format!(".driver-diagnostic.{}.tmp", std::process::id())); - let mut file = fs::File::create(&tmp)?; - serde_json::to_writer(&mut file, value).map_err(std::io::Error::other)?; - file.write_all(b"\n")?; - file.sync_all()?; - if let Err(error) = fs::rename(&tmp, path) { + let tmp = parent.join(format!( + ".driver-diagnostic.tmp-{}-{}", + std::process::id(), + WRITE.fetch_add(1, Ordering::Relaxed) + )); + let mut file = create_staging(&tmp)?; + let staged = (|| -> std::io::Result<()> { + serde_json::to_writer(&mut file, value).map_err(std::io::Error::other)?; + file.write_all(b"\n")?; + file.sync_all() + })(); + drop(file); + if let Err(error) = staged.and_then(|()| fs::rename(&tmp, path)) { let _ = fs::remove_file(&tmp); return Err(error); } + // Best-effort, exactly like `delivery_ledger`: the record is already durable, and a directory + // that cannot be synced must not turn a published diagnostic into a reported failure. + if let Ok(dir) = fs::File::open(parent) { + let _ = dir.sync_all(); + } Ok(()) } @@ -865,4 +903,62 @@ mod tests { out } } + + /// The publication path writes into an agent-writable directory, so its staging file is the + /// one place an agent could aim st2's own privilege at a file it does not own. Refusing an + /// existing path is what stops that, and `0600` is what stops the diagnostic being readable + /// by anyone who can reach the directory. + #[test] + fn a_planted_staging_symlink_is_refused_and_the_record_is_owner_only() { + use std::os::unix::fs::{PermissionsExt as _, symlink}; + + let tmp = tempfile::tempdir().unwrap(); + let agent = tmp.path().join("agents/h/worker"); + fs::create_dir_all(&agent).unwrap(); + + let victim = tmp.path().join("authored"); + fs::write(&victim, b"authored bytes").unwrap(); + let planted = agent.join(".driver-diagnostic.tmp-planted"); + symlink(&victim, &planted).unwrap(); + + let refused = create_staging(&planted).unwrap_err(); + assert_eq!( + refused.kind(), + std::io::ErrorKind::AlreadyExists, + "a planted symlink at the staging path must be refused, not followed" + ); + assert_eq!( + fs::read(&victim).unwrap(), + b"authored bytes", + "the planted symlink was followed and its target was truncated" + ); + + let record = Record { + schema: SCHEMA.to_owned(), + driver: Driver::OpenCode, + stage: Stage::Seed, + reason: Reason::UnknownStatus, + source: Source::StatusSnapshot, + producer_version: None, + support: Support::Supported, + observed_at: 100, + recovery: RECOVERY.to_owned(), + }; + let path = agent.join("driver-diagnostic"); + atomic_json(&path, &record).unwrap(); + + assert_eq!( + fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600, + "the diagnostic is readable by anyone who can reach the agent directory" + ); + let residue = fs::read_dir(&agent) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .filter(|name| { + name.starts_with(".driver-diagnostic.tmp-") && name != ".driver-diagnostic.tmp-planted" + }) + .collect::>(); + assert!(residue.is_empty(), "staging residue left behind: {residue:?}"); + } } diff --git a/src/hooks.rs b/src/hooks.rs index 65b0a2aa..d7bbe056 100644 --- a/src/hooks.rs +++ b/src/hooks.rs @@ -758,10 +758,6 @@ mod tests { assert_eq!(registered, claude_settings_registration()); } - /// The gate that holds a pi launch until the set is verified keys on this predicate, so a - /// fencepost here silently ungates every pi agent. The `driver pi-session` shape is the one - /// expansion actually emits. - #[test] #[test] fn omp_launch_classification_is_exact() { let root = Path::new("/catalog"); @@ -807,6 +803,9 @@ mod tests { assert!(!argv_invokes_omp(&["/opt/bin/omph".into()], root)); } + /// The gate that holds a pi launch until the set is verified keys on this predicate, so a + /// fencepost here silently ungates every pi agent. The `driver pi-session` shape is the one + /// expansion actually emits. #[test] fn pi_launch_classification_is_exact() { let root = Path::new("/catalog"); diff --git a/src/identity.rs b/src/identity.rs index 09b032e4..a9f31ada 100644 --- a/src/identity.rs +++ b/src/identity.rs @@ -72,10 +72,6 @@ impl AddressBookEntry { } } - /// The human-routable bus address `.
`. - pub fn bus_address(&self) -> String { - format!("{}.{}", self.host, self.address) - } } /// Why an ordinary reference did not name exactly one subject. diff --git a/src/pretrust.rs b/src/pretrust.rs index d54e718c..129fa477 100644 --- a/src/pretrust.rs +++ b/src/pretrust.rs @@ -50,13 +50,6 @@ pub fn pretrust_claude(dirs: &[PathBuf]) -> Result { pretrust_at(&config_path()?, dirs) } -/// Pre-trust workspaces for Codex only in the caller's ambient config. This remains available to -/// explicit tooling, but reconciliation does not call it: a provider command may select an -/// account-specific `CODEX_HOME` only after st2 launches it. -pub fn pretrust_codex(dirs: &[PathBuf]) -> Result { - pretrust_codex_at(&codex_config_path()?, dirs) -} - /// The codex config file: `$CODEX_HOME/config.toml` if set, else `~/.codex/config.toml`. fn codex_config_path() -> Result { if let Some(dir) = std::env::var_os("CODEX_HOME") { diff --git a/src/resource_profile.rs b/src/resource_profile.rs index 2831358b..d09f249e 100644 --- a/src/resource_profile.rs +++ b/src/resource_profile.rs @@ -189,10 +189,6 @@ impl SnapshotTarget { &self.root } - pub fn relative_path(&self) -> &Path { - &self.relative - } - pub fn path(&self) -> PathBuf { self.root.join(&self.relative) } @@ -843,6 +839,7 @@ impl CatchUpState { ) } + #[cfg(test)] pub fn last_commit(&self) -> Option { self.last_intent .as_ref()