From 953f524c2a6ecbe529a640466c9c77ec769746e8 Mon Sep 17 00:00:00 2001 From: yishuiliunian Date: Sun, 16 Aug 2026 05:50:59 -0400 Subject: [PATCH] fix(ci): restore cross-platform post-merge coverage --- .../provider/host-provider-mcp-rich.spec.ts | 3 +- build_defs/rust/desktop_test.bzl | 3 +- build_defs/rust/root_tests.bzl | 18 ++- crates/loopal-agent-client/src/lib.rs | 2 + crates/loopal-agent-client/src/process.rs | 15 +- .../loopal-agent-client/src/process/tests.rs | 32 ++-- .../src/process/tests/branch_close.rs | 2 +- crates/loopal-agent-client/src/runfiles.rs | 146 ++++++++++++++++++ .../loopal-agent-client/src/runfiles/tests.rs | 113 ++++++++++++++ crates/loopal-agent-hub/BUILD.bazel | 2 +- .../src/dispatch/authorization_policy.rs | 14 +- .../src/dispatch/authorization_tests.rs | 30 ++++ crates/loopal-agent-hub/src/uplink_tests.rs | 26 ++++ .../tests/suite/e2e_bootstrap_test.rs | 6 +- crates/loopal-agent/BUILD.bazel | 2 +- crates/loopal-backend/BUILD.bazel | 5 +- .../src/private_log_file_tests.rs | 5 +- crates/loopal-backend/tests/suite.rs | 2 - crates/loopal-ipc/BUILD.bazel | 6 +- crates/loopal-mcp/src/local_provider.rs | 18 ++- .../src/local_provider_reconnect.rs | 27 +++- crates/loopal-mcp/src/manager_reconnect.rs | 14 +- .../loopal-mcp/src/manager_reconnect_tests.rs | 49 +++++- crates/loopal-meta-hub/BUILD.bazel | 2 +- .../tests/e2e/cluster_harness.rs | 6 +- crates/loopal-runtime/src/agent_loop/llm.rs | 7 +- .../loopal-runtime/src/agent_loop/params.rs | 1 + .../src/agent_loop/params_builder.rs | 7 + .../src/agent_loop/tool_result_sink.rs | 6 +- .../tests/agent_loop/llm_coverage_test.rs | 2 + .../tests/agent_loop/params_builder_test.rs | 8 + .../agent_loop/tool_action_runner_test.rs | 5 +- .../agent_loop/tool_result_image_sink_test.rs | 26 ++-- .../suite/post_hook_secret_input_test.rs | 18 ++- .../suite/unresolved_secret_effect_test.rs | 29 ++-- crates/loopal-secret-runtime/src/hooks.rs | 95 ++++++++---- .../loopal-secret-runtime/src/hooks/audit.rs | 61 ++++++++ crates/loopal-secret-runtime/src/lib.rs | 5 +- .../tests/suite/hooks_test.rs | 45 +++--- crates/loopal-storage/src/resources.rs | 4 +- .../loopal-storage/src/resources/file_io.rs | 67 +++++++- .../src/resources/file_io_tests.rs | 108 ++++++++++++- .../src/workflow_journal/fs/windows.rs | 3 +- .../src/workflow_journal/fs/windows/tests.rs | 19 ++- .../tests/suite/resources_test.rs | 6 + src/bootstrap/mod.rs | 10 +- src/bootstrap/modes/acp_lifecycle_tests.rs | 2 +- .../modes/hub_only_lifecycle_tests.rs | 8 +- .../modes/hub_only_resume_lifecycle_tests.rs | 2 +- src/bootstrap/modes/lifecycle_test_support.rs | 17 +- .../modes/server_mode/lifecycle_tests.rs | 5 +- tests/e2e/bootstrap_typestate.rs | 4 +- tests/e2e/cli_llm/support_process.rs | 11 +- tests/e2e/desktop/support.rs | 11 +- tests/e2e/hub_lifecycle.rs | 4 +- tests/e2e/hub_llm/support_hub.rs | 17 +- .../hub_llm/workflow_stale_completion_test.rs | 5 +- tests/e2e/join_hub.rs | 4 +- tests/e2e/system_ipc.rs | 9 +- tests/regressions/hub_only_mcp_deadlock.rs | 4 +- 60 files changed, 961 insertions(+), 222 deletions(-) create mode 100644 crates/loopal-agent-client/src/runfiles.rs create mode 100644 crates/loopal-agent-client/src/runfiles/tests.rs create mode 100644 crates/loopal-secret-runtime/src/hooks/audit.rs diff --git a/apps/desktop/e2e/real/provider/host-provider-mcp-rich.spec.ts b/apps/desktop/e2e/real/provider/host-provider-mcp-rich.spec.ts index 5ef0eb93..4828b799 100644 --- a/apps/desktop/e2e/real/provider/host-provider-mcp-rich.spec.ts +++ b/apps/desktop/e2e/real/provider/host-provider-mcp-rich.spec.ts @@ -50,7 +50,8 @@ test('preserves rich MCP, error, reconnect, and cancellation semantics', async ( await rich.locator(':scope > summary').click() const output = rich.locator('.tool-output') await expect(output).toContainText('fixture rich text') - await expect(output).toContainText('data:image/png;base64,iVBORw0KGgo=') + await expect(output).toContainText('[MCP binary content denied]') + await expect(output).not.toContainText('data:image/png;base64,iVBORw0KGgo=') await expect(output).toContainText('[resource fixture://embedded]') await expect(output).toContainText('[resource: fixture://linked]') await ready(page) diff --git a/build_defs/rust/desktop_test.bzl b/build_defs/rust/desktop_test.bzl index 47d08d81..85e016cb 100644 --- a/build_defs/rust/desktop_test.bzl +++ b/build_defs/rust/desktop_test.bzl @@ -14,12 +14,13 @@ def desktop_serve_test(): crate_root = "tests/e2e/desktop/serve.rs", edition = "2024", data = [":loopal"], - env = {"LOOPAL_BINARY": "$(rootpath :loopal)"}, + env = {"LOOPAL_BINARY": "$(rlocationpath :loopal)"}, local = True, # The tests launch real Desktop/Hub processes and enforce startup # deadlines, so their timing must not include unrelated //... load. tags = ["exclusive"], deps = [ + "//crates/loopal-agent-client", "//crates/loopal-ipc", "@crates//:serde_json", "@crates//:tempfile", diff --git a/build_defs/rust/root_tests.bzl b/build_defs/rust/root_tests.bzl index 45a856bc..2c6d548c 100644 --- a/build_defs/rust/root_tests.bzl +++ b/build_defs/rust/root_tests.bzl @@ -12,10 +12,10 @@ def _binary_e2e(name, src, deps, extra_srcs = [], tags = []): crate_root = src, data = [":loopal"], edition = "2024", - env = {"LOOPAL_BINARY": "$(rootpath :loopal)"}, + env = {"LOOPAL_BINARY": "$(rlocationpath :loopal)"}, local = True, tags = ["exclusive"] + tags, - deps = deps, + deps = ["//crates/loopal-agent-client"] + deps, ) def loopal_root_tests(): @@ -76,7 +76,7 @@ def loopal_root_tests(): crate_root = "tests/e2e/cli_llm/suite.rs", data = [":loopal"], edition = "2024", - env = {"LOOPAL_BINARY": "$(rootpath :loopal)"}, + env = {"LOOPAL_BINARY": "$(rlocationpath :loopal)"}, local = True, # reason: excluded from `//...` wildcards (three-OS CI matrix) and run # by the dedicated Agent E2E gate job, mirroring the desktop e2e setup. @@ -86,6 +86,7 @@ def loopal_root_tests(): "manual", ], deps = [ + "//crates/loopal-agent-client", "//crates/loopal-ipc", "//crates/loopal-protocol", "//crates/loopal-test-support:mock-llm-server", @@ -124,9 +125,9 @@ def loopal_root_tests(): ], edition = "2024", env = { - "LOOPAL_BINARY": "$(rootpath :loopal)", - "LOOPAL_MOCK_MCP_BINARY": "$(rootpath :mock_mcp_server)", - "LOOPAL_MOCK_WORKFLOW_WORKER_BINARY": "$(rootpath :mock_workflow_worker)", + "LOOPAL_BINARY": "$(rlocationpath :loopal)", + "LOOPAL_MOCK_MCP_BINARY": "$(rlocationpath :mock_mcp_server)", + "LOOPAL_MOCK_WORKFLOW_WORKER_BINARY": "$(rlocationpath :mock_workflow_worker)", }, local = True, tags = [ @@ -135,6 +136,7 @@ def loopal_root_tests(): "manual", ], deps = [ + "//crates/loopal-agent-client", "//crates/loopal-ipc", "//crates/loopal-protocol", "//crates/loopal-storage", @@ -211,9 +213,9 @@ def loopal_root_tests(): ], edition = "2024", env = { - "LOOPAL_BINARY": "$(rootpath :loopal)", + "LOOPAL_BINARY": "$(rlocationpath :loopal)", "LOOPAL_OTEL_ENABLED": "0", - "LOOPAL_TEST_PROVIDER": "$(rootpath tests/fixtures/bootstrap_mock_provider.json)", + "LOOPAL_TEST_PROVIDER": "$(rlocationpath tests/fixtures/bootstrap_mock_provider.json)", }, local = True, tags = ["exclusive"], diff --git a/crates/loopal-agent-client/src/lib.rs b/crates/loopal-agent-client/src/lib.rs index 93e9fba2..8be7108c 100644 --- a/crates/loopal-agent-client/src/lib.rs +++ b/crates/loopal-agent-client/src/lib.rs @@ -7,6 +7,7 @@ pub mod bridge; mod client; mod process; mod process_command; +mod runfiles; mod start_params; pub(crate) mod stderr_drain; @@ -18,4 +19,5 @@ pub mod test_support { pub use bridge::{BridgeHandles, start_bridge}; pub use client::{AgentClient, AgentClientEvent}; pub use process::AgentProcess; +pub use runfiles::{require_runfile_env, resolve_runfile_env}; pub use start_params::{StartAgentParams, encode}; diff --git a/crates/loopal-agent-client/src/process.rs b/crates/loopal-agent-client/src/process.rs index 969d53e6..4a922a6c 100644 --- a/crates/loopal-agent-client/src/process.rs +++ b/crates/loopal-agent-client/src/process.rs @@ -168,9 +168,11 @@ impl AgentProcess { } fn resolve_executable(name: &str) -> anyhow::Result { - let override_path = std::env::var("LOOPAL_BINARY").ok().map(PathBuf::from); + if let Some(path) = crate::resolve_runfile_env("LOOPAL_BINARY")? { + return Ok(path); + } let current = std::env::current_exe().ok(); - let selected = Self::select_executable(name, override_path, current); + let selected = Self::select_executable(name, current); if selected.exists() { return std::fs::canonicalize(&selected).map_err(|error| { anyhow::anyhow!( @@ -182,14 +184,7 @@ impl AgentProcess { Ok(selected) } - fn select_executable( - name: &str, - override_path: Option, - current: Option, - ) -> PathBuf { - if let Some(path) = override_path.filter(|path| path.exists()) { - return path; - } + fn select_executable(name: &str, current: Option) -> PathBuf { let explicit = PathBuf::from(name); if explicit.is_absolute() && explicit.exists() { return explicit; diff --git a/crates/loopal-agent-client/src/process/tests.rs b/crates/loopal-agent-client/src/process/tests.rs index 2824df21..4abb13dd 100644 --- a/crates/loopal-agent-client/src/process/tests.rs +++ b/crates/loopal-agent-client/src/process/tests.rs @@ -11,31 +11,39 @@ mod support; #[test] fn executable_resolver_obeys_precedence() { - let override_path = script("exit 0"); let explicit = script("exit 0"); let current = script("exit 0"); assert_eq!( - AgentProcess::select_executable( - explicit.to_str().unwrap(), - Some(override_path.clone()), - Some(current.clone()), - ), - override_path - ); - assert_eq!( - AgentProcess::select_executable(explicit.to_str().unwrap(), None, Some(current.clone())), + AgentProcess::select_executable(explicit.to_str().unwrap(), Some(current.clone())), explicit ); assert_eq!( - AgentProcess::select_executable("loopal", Some(missing_path()), Some(current.clone())), + AgentProcess::select_executable("loopal", Some(current.clone())), current ); assert_eq!( - AgentProcess::select_executable("loopal", Some(missing_path()), None), + AgentProcess::select_executable("loopal", None), Path::new("loopal") ); } +#[tokio::test] +async fn invalid_explicit_override_is_an_error() { + let _lock = env_lock().await; + let old = std::env::var_os("LOOPAL_BINARY"); + let missing = missing_path(); + unsafe { std::env::set_var("LOOPAL_BINARY", &missing) }; + + let error = AgentProcess::spawn_now(None).err().unwrap().to_string(); + + assert!(error.contains("LOOPAL_BINARY"), "{error}"); + assert!(error.contains(&missing.display().to_string()), "{error}"); + match old { + Some(value) => unsafe { std::env::set_var("LOOPAL_BINARY", value) }, + None => unsafe { std::env::remove_var("LOOPAL_BINARY") }, + } +} + #[tokio::test] async fn spawn_now_some_succeeds_without_test_binary() { let _lock = env_lock().await; diff --git a/crates/loopal-agent-client/src/process/tests/branch_close.rs b/crates/loopal-agent-client/src/process/tests/branch_close.rs index 055fdaff..8c84dacd 100644 --- a/crates/loopal-agent-client/src/process/tests/branch_close.rs +++ b/crates/loopal-agent-client/src/process/tests/branch_close.rs @@ -5,7 +5,7 @@ fn missing_absolute_executable_falls_through_without_path_rewrite() { let missing = support::missing_path(); assert!(missing.is_absolute()); assert_eq!( - AgentProcess::select_executable(missing.to_str().unwrap(), None, None), + AgentProcess::select_executable(missing.to_str().unwrap(), None), missing ); } diff --git a/crates/loopal-agent-client/src/runfiles.rs b/crates/loopal-agent-client/src/runfiles.rs new file mode 100644 index 00000000..458070a5 --- /dev/null +++ b/crates/loopal-agent-client/src/runfiles.rs @@ -0,0 +1,146 @@ +use std::ffi::OsStr; +use std::path::{Path, PathBuf}; + +/// Resolve a file-valued environment variable from Bazel runfiles. +/// +/// A configured value is authoritative: callers receive an error when it +/// cannot be resolved. `Ok(None)` is returned only when the variable is absent. +pub fn resolve_runfile_env(variable: &str) -> anyhow::Result> { + let Some(configured) = std::env::var_os(variable) else { + return Ok(None); + }; + resolve_configured_file(variable, &configured, &RunfilesLocations::from_env()).map(Some) +} + +/// Resolve a required file-valued environment variable from Bazel runfiles. +pub fn require_runfile_env(variable: &str) -> anyhow::Result { + resolve_runfile_env(variable)? + .ok_or_else(|| anyhow::anyhow!("{variable} must be set to a file path")) +} + +#[derive(Default)] +struct RunfilesLocations { + runfiles_dir: Option, + test_srcdir: Option, + manifest: Option, +} + +impl RunfilesLocations { + fn from_env() -> Self { + Self { + runfiles_dir: std::env::var_os("RUNFILES_DIR").map(PathBuf::from), + test_srcdir: std::env::var_os("TEST_SRCDIR").map(PathBuf::from), + manifest: std::env::var_os("RUNFILES_MANIFEST_FILE").map(PathBuf::from), + } + } +} + +fn resolve_configured_file( + variable: &str, + configured: &OsStr, + locations: &RunfilesLocations, +) -> anyhow::Result { + let logical = PathBuf::from(configured); + if logical.as_os_str().is_empty() { + return Err(unresolved(variable, &logical)); + } + if logical.is_absolute() { + return existing_file(variable, &logical, &logical); + } + if logical.is_file() { + return canonicalize(variable, &logical, &logical); + } + for root in [&locations.runfiles_dir, &locations.test_srcdir] + .into_iter() + .flatten() + { + let candidate = root.join(&logical); + if candidate.is_file() { + return canonicalize(variable, &logical, &candidate); + } + } + if let (Some(manifest), Some(key)) = (&locations.manifest, logical.to_str()) + && let Some(candidate) = manifest_entry(manifest, key).map_err(|error| { + anyhow::anyhow!( + "failed to resolve {variable} runfile '{}': could not read manifest '{}': {error}", + logical.display(), + manifest.display() + ) + })? + { + return existing_file(variable, &logical, &candidate); + } + Err(unresolved(variable, &logical)) +} + +fn manifest_entry(manifest: &Path, key: &str) -> std::io::Result> { + let contents = std::fs::read_to_string(manifest)?; + Ok(contents.lines().find_map(|line| { + let (logical, physical) = parse_manifest_entry(line.trim_end_matches('\r'))?; + (logical == key).then(|| PathBuf::from(physical)) + })) +} + +fn parse_manifest_entry(line: &str) -> Option<(String, String)> { + let (escaped, entry) = match line.strip_prefix(' ') { + Some(entry) => (true, entry), + None => (false, line), + }; + let (logical, physical) = entry.split_once(' ')?; + if !escaped { + return Some((logical.to_owned(), physical.to_owned())); + } + Some(( + decode_manifest_field(logical), + decode_manifest_field(physical), + )) +} + +fn decode_manifest_field(value: &str) -> String { + let mut decoded = String::with_capacity(value.len()); + let mut chars = value.chars(); + while let Some(character) = chars.next() { + if character != '\\' { + decoded.push(character); + continue; + } + match chars.next() { + Some('s') => decoded.push(' '), + Some('n') => decoded.push('\n'), + Some('b') => decoded.push('\\'), + Some(other) => { + decoded.push('\\'); + decoded.push(other); + } + None => decoded.push('\\'), + } + } + decoded +} + +fn existing_file(variable: &str, logical: &Path, candidate: &Path) -> anyhow::Result { + if !candidate.is_file() { + return Err(unresolved(variable, logical)); + } + canonicalize(variable, logical, candidate) +} + +fn canonicalize(variable: &str, logical: &Path, candidate: &Path) -> anyhow::Result { + std::fs::canonicalize(candidate).map_err(|error| { + anyhow::anyhow!( + "failed to resolve {variable} runfile '{}': {error}", + logical.display() + ) + }) +} + +fn unresolved(variable: &str, logical: &Path) -> anyhow::Error { + anyhow::anyhow!( + "failed to resolve {variable} runfile '{}': file not found", + logical.display() + ) +} + +#[cfg(test)] +#[path = "runfiles/tests.rs"] +mod tests; diff --git a/crates/loopal-agent-client/src/runfiles/tests.rs b/crates/loopal-agent-client/src/runfiles/tests.rs new file mode 100644 index 00000000..3a60e9bd --- /dev/null +++ b/crates/loopal-agent-client/src/runfiles/tests.rs @@ -0,0 +1,113 @@ +use std::ffi::OsStr; +use std::path::PathBuf; +use std::sync::atomic::{AtomicU64, Ordering}; + +use super::*; + +fn temp_dir() -> PathBuf { + static NEXT: AtomicU64 = AtomicU64::new(1); + let path = std::env::temp_dir().join(format!( + "loopal-runfiles-{}-{}", + std::process::id(), + NEXT.fetch_add(1, Ordering::Relaxed) + )); + std::fs::create_dir_all(&path).unwrap(); + path +} + +#[test] +fn resolves_escaped_manifest_entry() { + let root = temp_dir(); + let physical = root.join("physical dir").join("loopal binary"); + std::fs::create_dir_all(physical.parent().unwrap()).unwrap(); + std::fs::write(&physical, "fixture").unwrap(); + let manifest = root.join("MANIFEST"); + let logical = "_main/loopal binary"; + std::fs::write( + &manifest, + format!( + " {} {}\n_main/other ignored\n", + escape_manifest_field(logical), + escape_manifest_field(&physical.to_string_lossy()), + ), + ) + .unwrap(); + let locations = RunfilesLocations { + manifest: Some(manifest), + ..Default::default() + }; + + let resolved = + resolve_configured_file("LOOPAL_BINARY", OsStr::new(logical), &locations).unwrap(); + + assert_eq!(resolved, std::fs::canonicalize(&physical).unwrap()); + std::fs::remove_dir_all(root).unwrap(); +} + +fn escape_manifest_field(value: &str) -> String { + value + .replace('\\', "\\b") + .replace(' ', "\\s") + .replace('\n', "\\n") +} + +#[test] +fn decodes_all_bazel_manifest_escape_sequences() { + let (logical, physical) = parse_manifest_entry(r" _main/a\sb\nc\bd /tmp/x\sy\nz\bw").unwrap(); + + assert_eq!(logical, "_main/a b\nc\\d"); + assert_eq!(physical, "/tmp/x y\nz\\w"); +} + +#[test] +fn runfiles_dir_precedes_test_srcdir() { + let root = temp_dir(); + let logical = PathBuf::from("_main/loopal"); + let runfiles_file = root.join("runfiles").join(&logical); + let test_srcdir_file = root.join("test-srcdir").join(&logical); + for file in [&runfiles_file, &test_srcdir_file] { + std::fs::create_dir_all(file.parent().unwrap()).unwrap(); + std::fs::write(file, "fixture").unwrap(); + } + let locations = RunfilesLocations { + runfiles_dir: Some(root.join("runfiles")), + test_srcdir: Some(root.join("test-srcdir")), + manifest: None, + }; + + let resolved = + resolve_configured_file("LOOPAL_BINARY", logical.as_os_str(), &locations).unwrap(); + + assert_eq!(resolved, std::fs::canonicalize(&runfiles_file).unwrap()); + std::fs::remove_file(runfiles_file).unwrap(); + let resolved = + resolve_configured_file("LOOPAL_BINARY", logical.as_os_str(), &locations).unwrap(); + assert_eq!(resolved, std::fs::canonicalize(test_srcdir_file).unwrap()); + std::fs::remove_dir_all(root).unwrap(); +} + +#[test] +fn invalid_absolute_path_does_not_fall_back_to_manifest() { + let root = temp_dir(); + let physical = root.join("loopal"); + std::fs::write(&physical, "fixture").unwrap(); + let missing = root.join("missing-loopal"); + let manifest = root.join("MANIFEST"); + std::fs::write( + &manifest, + format!("{} {}\n", missing.display(), physical.display()), + ) + .unwrap(); + let locations = RunfilesLocations { + manifest: Some(manifest), + ..Default::default() + }; + + let error = resolve_configured_file("LOOPAL_BINARY", missing.as_os_str(), &locations) + .unwrap_err() + .to_string(); + + assert!(error.contains("LOOPAL_BINARY"), "{error}"); + assert!(error.contains(&missing.display().to_string()), "{error}"); + std::fs::remove_dir_all(root).unwrap(); +} diff --git a/crates/loopal-agent-hub/BUILD.bazel b/crates/loopal-agent-hub/BUILD.bazel index 4f601176..6e2b3280 100644 --- a/crates/loopal-agent-hub/BUILD.bazel +++ b/crates/loopal-agent-hub/BUILD.bazel @@ -56,7 +56,7 @@ rust_test( crate_root = "tests/suite.rs", data = ["//:loopal"], edition = "2024", - env = {"LOOPAL_BINARY": "$(rootpath //:loopal)"}, + env = {"LOOPAL_BINARY": "$(rlocationpath //:loopal)"}, local = True, deps = [ ":loopal-agent-hub", diff --git a/crates/loopal-agent-hub/src/dispatch/authorization_policy.rs b/crates/loopal-agent-hub/src/dispatch/authorization_policy.rs index b287d503..d78598b2 100644 --- a/crates/loopal-agent-hub/src/dispatch/authorization_policy.rs +++ b/crates/loopal-agent-hub/src/dispatch/authorization_policy.rs @@ -56,7 +56,10 @@ pub(super) fn managed_meta_method(method: &str) -> bool { } pub(super) fn trusted_meta_method(method: &str) -> bool { - matches!(method, "hub/spawn_remote_agent" | "hub/remote_relay") + matches!( + method, + "hub/spawn_remote_agent" | "hub/remote_relay" | "hub/topology" + ) } #[cfg(test)] @@ -111,4 +114,13 @@ mod tests { assert!(!managed_meta_method(methods::META_SPAWN.name)); assert!(!managed_meta_method("meta/future_admin")); } + + #[test] + fn trusted_metahub_acl_is_closed_set() { + assert!(trusted_meta_method(methods::HUB_SPAWN_REMOTE_AGENT.name)); + assert!(trusted_meta_method(methods::HUB_REMOTE_RELAY.name)); + assert!(trusted_meta_method(methods::HUB_TOPOLOGY.name)); + assert!(!trusted_meta_method(methods::HUB_STATUS.name)); + assert!(!trusted_meta_method("hub/future_metahub_method")); + } } diff --git a/crates/loopal-agent-hub/src/dispatch/authorization_tests.rs b/crates/loopal-agent-hub/src/dispatch/authorization_tests.rs index 054d6f25..aaba7cbd 100644 --- a/crates/loopal-agent-hub/src/dispatch/authorization_tests.rs +++ b/crates/loopal-agent-hub/src/dispatch/authorization_tests.rs @@ -160,6 +160,36 @@ async fn trusted_metahub_principal_requires_active_uplink_connection() { ); } +#[tokio::test] +async fn trusted_metahub_topology_requires_active_uplink_connection() { + let hub = hub(); + let active = connection(); + let stale = connection(); + hub.lock().await.uplink = Some(Arc::new(HubUplink::new(active.clone(), "local".into()))); + + let stale_principal = Arc::new(HubRequestPrincipal::TrustedMetaHub( + TrustedMetaHubPrincipal::new(stale), + )); + let error = + match authorization::authorize(&hub, methods::HUB_TOPOLOGY.name, stale_principal).await { + Ok(_) => panic!("stale MetaHub topology principal was authorized"), + Err(error) => error, + }; + assert!(error.to_string().contains("not authorized")); + + let active_principal = Arc::new(HubRequestPrincipal::TrustedMetaHub( + TrustedMetaHubPrincipal::new(active.clone()), + )); + let context = authorization::authorize(&hub, methods::HUB_TOPOLOGY.name, active_principal) + .await + .unwrap(); + assert!( + authorization::trusted_meta(&context) + .unwrap() + .matches_connection(&active) + ); +} + #[test] fn principal_extractors_fail_closed_for_missing_or_wrong_type() { let empty = loopal_ipc::HandlerCtx::new("none"); diff --git a/crates/loopal-agent-hub/src/uplink_tests.rs b/crates/loopal-agent-hub/src/uplink_tests.rs index e905a607..57b66c5e 100644 --- a/crates/loopal-agent-hub/src/uplink_tests.rs +++ b/crates/loopal-agent-hub/src/uplink_tests.rs @@ -99,6 +99,32 @@ async fn reverse_request_and_notification_deliver_to_local_agent() { responder.await.unwrap(); } +#[tokio::test] +async fn active_reverse_metahub_can_query_topology() { + let hub = hub(); + let ((_agent_peer, _agent_peer_rx), (agent_hub, _agent_hub_rx)) = pair(); + hub.lock() + .await + .registry + .register_connection("main", agent_hub) + .unwrap(); + let ((reverse, reverse_rx), (meta, _meta_rx)) = pair(); + hub.lock().await.uplink = Some(Arc::new(HubUplink::new(reverse.clone(), "hub-a".into()))); + tokio::spawn(handle_reverse_requests( + hub, + reverse, + reverse_rx, + "hub-a".into(), + )); + + let topology = meta + .send_request(methods::HUB_TOPOLOGY.name, serde_json::json!({})) + .await + .unwrap(); + assert_eq!(topology["agents"].as_array().unwrap().len(), 1); + assert_eq!(topology["agents"][0]["name"], "main"); +} + #[tokio::test] async fn reverse_requests_fail_closed_for_invalid_message_and_hub_method() { let hub = hub(); diff --git a/crates/loopal-agent-hub/tests/suite/e2e_bootstrap_test.rs b/crates/loopal-agent-hub/tests/suite/e2e_bootstrap_test.rs index 33eddf3a..8f54656e 100644 --- a/crates/loopal-agent-hub/tests/suite/e2e_bootstrap_test.rs +++ b/crates/loopal-agent-hub/tests/suite/e2e_bootstrap_test.rs @@ -391,10 +391,10 @@ fn parse_missing_plan_path(result: &str) -> std::path::PathBuf { /// Find the loopal binary. Checks LOOPAL_BINARY env var first (set by Bazel), /// then falls back to Cargo target directory layout. fn resolve_loopal_binary() -> String { - if let Ok(path) = std::env::var("LOOPAL_BINARY") - && std::path::Path::new(&path).exists() + if let Some(path) = + loopal_agent_client::resolve_runfile_env("LOOPAL_BINARY").expect("resolve LOOPAL_BINARY") { - return path; + return path.to_string_lossy().into_owned(); } let test_exe = std::env::current_exe().expect("current_exe"); let target_dir = test_exe diff --git a/crates/loopal-agent/BUILD.bazel b/crates/loopal-agent/BUILD.bazel index c6cb06c7..318b7d13 100644 --- a/crates/loopal-agent/BUILD.bazel +++ b/crates/loopal-agent/BUILD.bazel @@ -49,7 +49,7 @@ rust_test( crate_root = "tests/suite.rs", data = ["//:loopal"], edition = "2024", - env = {"LOOPAL_BINARY": "$(rootpath //:loopal)"}, + env = {"LOOPAL_BINARY": "$(rlocationpath //:loopal)"}, local = True, proc_macro_deps = ["@crates//:async-trait"], deps = [ diff --git a/crates/loopal-backend/BUILD.bazel b/crates/loopal-backend/BUILD.bazel index 5781149d..14d92bb3 100644 --- a/crates/loopal-backend/BUILD.bazel +++ b/crates/loopal-backend/BUILD.bazel @@ -44,7 +44,10 @@ rust_test( rust_test( name = "loopal-backend_test", - srcs = glob(["tests/**/*.rs"]), + srcs = glob( + ["tests/**/*.rs"], + exclude = ["tests/suite/process_group_windows_test.rs"], + ), crate_root = "tests/suite.rs", edition = "2024", proc_macro_deps = ["@crates//:async-trait"], diff --git a/crates/loopal-backend/src/private_log_file_tests.rs b/crates/loopal-backend/src/private_log_file_tests.rs index deaaffff..2ae770b8 100644 --- a/crates/loopal-backend/src/private_log_file_tests.rs +++ b/crates/loopal-backend/src/private_log_file_tests.rs @@ -1,4 +1,6 @@ -use super::{create, ensure_private_directory, ensure_private_file}; +use super::create; +#[cfg(unix)] +use super::{ensure_private_directory, ensure_private_file}; fn unique_path(label: &str) -> std::path::PathBuf { std::env::temp_dir().join(format!("loopal-{label}-{}", uuid::Uuid::new_v4())) @@ -14,6 +16,7 @@ async fn create_rejects_an_existing_path() { std::fs::remove_file(path).unwrap(); } +#[cfg(unix)] #[tokio::test] async fn private_directory_inspection_fails_for_missing_path() { let path = unique_path("missing-log-dir"); diff --git a/crates/loopal-backend/tests/suite.rs b/crates/loopal-backend/tests/suite.rs index b6337e6b..f42900f9 100644 --- a/crates/loopal-backend/tests/suite.rs +++ b/crates/loopal-backend/tests/suite.rs @@ -31,8 +31,6 @@ mod log_permissions_test; mod path_approval_test; #[path = "suite/process_group_test.rs"] mod process_group_test; -#[path = "suite/process_group_windows_test.rs"] -mod process_group_windows_test; #[path = "suite/process_test_support.rs"] mod process_test_support; #[path = "suite/resolve_checked_test.rs"] diff --git a/crates/loopal-ipc/BUILD.bazel b/crates/loopal-ipc/BUILD.bazel index 0b93229a..0f91371d 100644 --- a/crates/loopal-ipc/BUILD.bazel +++ b/crates/loopal-ipc/BUILD.bazel @@ -4,6 +4,7 @@ rust_library( name = "loopal-ipc", srcs = glob(["src/**/*.rs"]), edition = "2024", + proc_macro_deps = ["@crates//:async-trait"], visibility = ["//visibility:public"], deps = [ "//crates/loopal-error", @@ -13,7 +14,6 @@ rust_library( "@crates//:tracing", "@crates//:uuid", ], - proc_macro_deps = ["@crates//:async-trait"], ) rust_test( @@ -26,10 +26,10 @@ rust_test( name = "loopal-ipc_test", srcs = glob(["tests/**/*.rs"]), crate_root = "tests/suite.rs", + data = ["//:loopal"], edition = "2024", + env = {"LOOPAL_BINARY": "$(rlocationpath //:loopal)"}, local = True, - data = ["//:loopal"], - env = {"LOOPAL_BINARY": "$(rootpath //:loopal)"}, deps = [ ":loopal-ipc", "//crates/loopal-agent-client", diff --git a/crates/loopal-mcp/src/local_provider.rs b/crates/loopal-mcp/src/local_provider.rs index a1486434..4d674c94 100644 --- a/crates/loopal-mcp/src/local_provider.rs +++ b/crates/loopal-mcp/src/local_provider.rs @@ -125,12 +125,12 @@ impl McpProvider for LocalMcpProvider { args: &Value, _budget: loopal_ipc::IpcBudget, ) -> Result { - let first = self - .manager - .read() - .await - .call_tool(server, tool, args) - .await; + let (failed_generation, first) = { + let manager = self.manager.read().await; + let generation = manager.connection_generation(server); + let result = manager.call_tool(server, tool, args).await; + (generation, result) + }; let transport_closed = matches!(&first, Err(McpError::TransportClosed(_))) || self .manager @@ -142,7 +142,11 @@ impl McpProvider for LocalMcpProvider { .is_none_or(|client| client.is_closed()); if first.is_err() && transport_closed { tracing::warn!(server, tool, "MCP transport closed, attempting reconnect"); - if self.try_reconnect(server).await { + let reconnected = match failed_generation { + Some(generation) => self.try_reconnect_after_failure(server, generation).await, + None => self.try_reconnect(server).await, + }; + if reconnected { return self .manager .read() diff --git a/crates/loopal-mcp/src/local_provider_reconnect.rs b/crates/loopal-mcp/src/local_provider_reconnect.rs index 4917c348..3a57acb3 100644 --- a/crates/loopal-mcp/src/local_provider_reconnect.rs +++ b/crates/loopal-mcp/src/local_provider_reconnect.rs @@ -1,17 +1,40 @@ use super::LocalMcpProvider; +use crate::connection_generation::ConnectionGeneration; impl LocalMcpProvider { pub async fn try_reconnect(&self, server: &str) -> bool { - self.try_reconnect_guarded(server, |commit| commit()).await + self.try_reconnect_guarded_inner(server, None, |commit| commit()) + .await } pub async fn try_reconnect_guarded(&self, server: &str, guard: F) -> bool + where + F: FnOnce(&mut dyn FnMut()), + { + self.try_reconnect_guarded_inner(server, None, guard).await + } + + pub(super) async fn try_reconnect_after_failure( + &self, + server: &str, + failed_generation: ConnectionGeneration, + ) -> bool { + self.try_reconnect_guarded_inner(server, Some(failed_generation), |commit| commit()) + .await + } + + async fn try_reconnect_guarded_inner( + &self, + server: &str, + failed_generation: Option, + guard: F, + ) -> bool where F: FnOnce(&mut dyn FnMut()), { let plan = { let manager = self.manager.read().await; - match manager.plan_reconnect(server) { + match manager.plan_reconnect(server, failed_generation.as_ref()) { Ok(Some(plan)) => plan, Ok(None) => return true, Err(_) => return false, diff --git a/crates/loopal-mcp/src/manager_reconnect.rs b/crates/loopal-mcp/src/manager_reconnect.rs index d4a62715..8ec6321a 100644 --- a/crates/loopal-mcp/src/manager_reconnect.rs +++ b/crates/loopal-mcp/src/manager_reconnect.rs @@ -27,12 +27,22 @@ impl ReconnectPlan { } impl McpManager { - pub(crate) fn plan_reconnect(&self, server: &str) -> Result, McpError> { + pub(crate) fn connection_generation(&self, server: &str) -> Option { + self.connections.get(server).map(McpConnection::generation) + } + + pub(crate) fn plan_reconnect( + &self, + server: &str, + failed_generation: Option<&ConnectionGeneration>, + ) -> Result, McpError> { let current = self .connections .get(server) .ok_or_else(|| McpError::ServerNotFound(server.to_string()))?; - if connection_is_open(current) { + let current_request_failed = + failed_generation.is_some_and(|generation| current.owns_generation(generation)); + if !current_request_failed && connection_is_open(current) { return Ok(None); } let candidate = McpConnection::new( diff --git a/crates/loopal-mcp/src/manager_reconnect_tests.rs b/crates/loopal-mcp/src/manager_reconnect_tests.rs index 4da7d335..5857f6b9 100644 --- a/crates/loopal-mcp/src/manager_reconnect_tests.rs +++ b/crates/loopal-mcp/src/manager_reconnect_tests.rs @@ -41,8 +41,8 @@ async fn connected(tool: &str) -> McpConnection { async fn stale_failed_candidate_cannot_remove_successful_generation() { let mut manager = McpManager::new(); let _ = manager.absorb_connections(vec![failed()]); - let mut winner = manager.plan_reconnect("server").unwrap().unwrap(); - let loser = manager.plan_reconnect("server").unwrap().unwrap(); + let mut winner = manager.plan_reconnect("server", None).unwrap().unwrap(); + let loser = manager.plan_reconnect("server", None).unwrap().unwrap(); winner.candidate = connected("winner_tool").await; let committed = manager.commit_reconnect(winner); @@ -60,7 +60,7 @@ async fn stale_failed_candidate_cannot_remove_successful_generation() { async fn disconnect_generation_rejects_in_flight_successful_candidate() { let mut manager = McpManager::new(); let _ = manager.absorb_connections(vec![failed()]); - let mut plan = manager.plan_reconnect("server").unwrap().unwrap(); + let mut plan = manager.plan_reconnect("server", None).unwrap().unwrap(); plan.candidate = connected("stale_tool").await; manager.connections["server"].disconnect().await; @@ -79,7 +79,7 @@ async fn disconnect_generation_rejects_in_flight_successful_candidate() { async fn removed_server_rejects_in_flight_candidate() { let mut manager = McpManager::new(); let _ = manager.absorb_connections(vec![failed()]); - let mut plan = manager.plan_reconnect("server").unwrap().unwrap(); + let mut plan = manager.plan_reconnect("server", None).unwrap().unwrap(); plan.candidate = connected("orphaned_tool").await; manager.connections.shift_remove("server"); @@ -91,12 +91,47 @@ async fn removed_server_rejects_in_flight_candidate() { } #[tokio::test] -async fn open_current_generation_skips_new_plan() { +async fn open_current_generation_skips_unforced_plan() { let mut manager = McpManager::new(); manager .absorb_connections(vec![connected("existing").await]) .unwrap(); - assert!(manager.plan_reconnect("server").unwrap().is_none()); - assert!(manager.plan_reconnect("missing").is_err()); + assert!(manager.plan_reconnect("server", None).unwrap().is_none()); + assert!(manager.plan_reconnect("missing", None).is_err()); +} + +#[tokio::test] +async fn current_generation_transport_failure_forces_replacement_before_closed_flag() { + let mut manager = McpManager::new(); + manager + .absorb_connections(vec![connected("existing").await]) + .unwrap(); + let failed_generation = manager.connection_generation("server").unwrap(); + + assert!( + manager + .plan_reconnect("server", Some(&failed_generation)) + .unwrap() + .is_some() + ); +} + +#[tokio::test] +async fn stale_transport_failure_reuses_open_replacement_generation() { + let mut manager = McpManager::new(); + manager + .absorb_connections(vec![connected("original").await]) + .unwrap(); + let stale_generation = manager.connection_generation("server").unwrap(); + manager + .absorb_connections(vec![connected("replacement").await]) + .unwrap(); + + assert!( + manager + .plan_reconnect("server", Some(&stale_generation)) + .unwrap() + .is_none() + ); } diff --git a/crates/loopal-meta-hub/BUILD.bazel b/crates/loopal-meta-hub/BUILD.bazel index 58d1664a..8c5a2d6b 100644 --- a/crates/loopal-meta-hub/BUILD.bazel +++ b/crates/loopal-meta-hub/BUILD.bazel @@ -49,7 +49,7 @@ rust_test( data = ["//:loopal"], edition = "2024", env = { - "LOOPAL_BINARY": "$(rootpath //:loopal)", + "LOOPAL_BINARY": "$(rlocationpath //:loopal)", "RUST_TEST_THREADS": "1", }, local = True, diff --git a/crates/loopal-meta-hub/tests/e2e/cluster_harness.rs b/crates/loopal-meta-hub/tests/e2e/cluster_harness.rs index ed16ef69..bb2592c7 100644 --- a/crates/loopal-meta-hub/tests/e2e/cluster_harness.rs +++ b/crates/loopal-meta-hub/tests/e2e/cluster_harness.rs @@ -177,10 +177,10 @@ fn create_mock_fixture(name: &str) -> std::path::PathBuf { } fn resolve_binary() -> String { - if let Ok(path) = std::env::var("LOOPAL_BINARY") - && std::path::Path::new(&path).exists() + if let Some(path) = + loopal_agent_client::resolve_runfile_env("LOOPAL_BINARY").expect("resolve LOOPAL_BINARY") { - return path; + return path.to_string_lossy().into_owned(); } let test_exe = std::env::current_exe().expect("current_exe"); let target_dir = test_exe diff --git a/crates/loopal-runtime/src/agent_loop/llm.rs b/crates/loopal-runtime/src/agent_loop/llm.rs index 0104b10b..37010c1c 100644 --- a/crates/loopal-runtime/src/agent_loop/llm.rs +++ b/crates/loopal-runtime/src/agent_loop/llm.rs @@ -24,7 +24,12 @@ impl AgentLoopRunner { } let mut chat_params = self.prepare_chat_params(intent)?; - if let Some(store) = crate::hydrate::resource_store() { + if let Some(store) = self + .params + .resource_store + .clone() + .or_else(crate::hydrate::resource_store) + { crate::hydrate::hydrate_turn_images( &mut chat_params.turns, store.as_ref(), diff --git a/crates/loopal-runtime/src/agent_loop/params.rs b/crates/loopal-runtime/src/agent_loop/params.rs index 988e65dd..799dff2a 100644 --- a/crates/loopal-runtime/src/agent_loop/params.rs +++ b/crates/loopal-runtime/src/agent_loop/params.rs @@ -140,6 +140,7 @@ pub struct AgentLoopParams { pub budget: ContextBudget, pub initial_turns: Vec, pub hydrate_initial_history: bool, + pub resource_store: Option>, pub interrupt: InterruptHandle, pub shared: Option>, pub memory_channel: Option>, diff --git a/crates/loopal-runtime/src/agent_loop/params_builder.rs b/crates/loopal-runtime/src/agent_loop/params_builder.rs index 09fe4303..9b883492 100644 --- a/crates/loopal-runtime/src/agent_loop/params_builder.rs +++ b/crates/loopal-runtime/src/agent_loop/params_builder.rs @@ -21,6 +21,7 @@ pub struct AgentLoopParamsBuilder { interrupt: InterruptHandle, initial_turns: Vec, hydrate_initial_history: bool, + resource_store: Option>, shared: Option>, memory_channel: Option>, one_shot_chat: Option>, @@ -55,6 +56,7 @@ impl AgentLoopParamsBuilder { interrupt, initial_turns: Vec::new(), hydrate_initial_history: false, + resource_store: None, shared: None, memory_channel: None, one_shot_chat: None, @@ -84,6 +86,10 @@ impl AgentLoopParamsBuilder { self.hydrate_initial_history = hydrate; self } + pub fn resource_store(mut self, store: Arc) -> Self { + self.resource_store = Some(store); + self + } pub fn shared(mut self, s: Arc) -> Self { self.shared = Some(s); @@ -178,6 +184,7 @@ impl AgentLoopParamsBuilder { budget: self.budget, initial_turns: self.initial_turns, hydrate_initial_history: self.hydrate_initial_history, + resource_store: self.resource_store, interrupt: self.interrupt, shared: self.shared, memory_channel: self.memory_channel, diff --git a/crates/loopal-runtime/src/agent_loop/tool_result_sink.rs b/crates/loopal-runtime/src/agent_loop/tool_result_sink.rs index 6c8507b7..df4b5149 100644 --- a/crates/loopal-runtime/src/agent_loop/tool_result_sink.rs +++ b/crates/loopal-runtime/src/agent_loop/tool_result_sink.rs @@ -100,7 +100,11 @@ impl PendingToolResult { }; let mut images = std::mem::take(&mut self.result.images); if !images.is_empty() - && let Some(store) = crate::hydrate::resource_store() + && let Some(store) = runner + .params + .resource_store + .clone() + .or_else(crate::hydrate::resource_store) && crate::hydrate::maybe_persist_inline_images( store.as_ref(), &runner.tool_ctx.session_id, diff --git a/crates/loopal-runtime/tests/agent_loop/llm_coverage_test.rs b/crates/loopal-runtime/tests/agent_loop/llm_coverage_test.rs index 5ad7fb49..726a4966 100644 --- a/crates/loopal-runtime/tests/agent_loop/llm_coverage_test.rs +++ b/crates/loopal-runtime/tests/agent_loop/llm_coverage_test.rs @@ -146,6 +146,7 @@ async fn run_child() { super::mock_provider::make_runner_with_dyn_provider(Arc::new(HydrationProvider)); let home = std::path::PathBuf::from(std::env::var_os("HOME").unwrap()); let store = FileResourceStore::with_base_dir(home.join(".loopal")); + runner.params.resource_store = Some(store.clone()); let bytes = b"\x89PNG\r\n\x1a\n"; let id = store .write(&runner.params.session.id, "image/png", bytes) @@ -163,6 +164,7 @@ async fn run_child() { let (mut rejected, _event_rx) = super::mock_provider::make_runner_with_dyn_provider(Arc::new(HydrationProvider)); + rejected.params.resource_store = Some(store); append_resource_result(&mut rejected, "invalid".into(), 8); in_turn(rejected.stream_llm_with(None, &make_cancel())) .await diff --git a/crates/loopal-runtime/tests/agent_loop/params_builder_test.rs b/crates/loopal-runtime/tests/agent_loop/params_builder_test.rs index 0ce77d56..46e6eb3b 100644 --- a/crates/loopal-runtime/tests/agent_loop/params_builder_test.rs +++ b/crates/loopal-runtime/tests/agent_loop/params_builder_test.rs @@ -79,6 +79,7 @@ async fn builder_default_optionals_yield_none_or_empty() { assert!(params.message_snapshot.is_none()); assert!(params.resume_hooks.is_empty()); assert!(!params.hydrate_initial_history); + assert!(params.resource_store.is_none()); } #[tokio::test] @@ -95,6 +96,8 @@ async fn builder_chained_setters_override_defaults() { max_output_tokens: 64, }; let hook: Arc = Arc::new(NoopHook); + let resource_store: Arc = + loopal_storage::FileResourceStore::with_base_dir(fixture.path().join("resources")); let params = AgentLoopParamsBuilder::new( AgentConfig::default(), deps_for(&fixture), @@ -104,9 +107,14 @@ async fn builder_chained_setters_override_defaults() { ) .resume_hooks(vec![hook.clone()]) .hydrate_initial_history(true) + .resource_store(resource_store.clone()) .build(); assert_eq!(params.resume_hooks.len(), 1); assert!(params.hydrate_initial_history); + assert!(Arc::ptr_eq( + params.resource_store.as_ref().unwrap(), + &resource_store + )); } #[tokio::test] diff --git a/crates/loopal-runtime/tests/agent_loop/tool_action_runner_test.rs b/crates/loopal-runtime/tests/agent_loop/tool_action_runner_test.rs index 7233d592..9fb6e1ee 100644 --- a/crates/loopal-runtime/tests/agent_loop/tool_action_runner_test.rs +++ b/crates/loopal-runtime/tests/agent_loop/tool_action_runner_test.rs @@ -43,8 +43,9 @@ fn runner_with_hook( async fn rewritten_action_is_the_action_permission_classifies() { let target = std::env::temp_dir().join(format!("loopal-rewritten-{}.txt", std::process::id())); let _ = std::fs::remove_file(&target); + let hook_target = target.to_string_lossy().replace('\\', "/"); let rewrite = json!({ - "updated_input": {"file_path": target, "content": "rewritten"} + "updated_input": {"file_path": hook_target.clone(), "content": "rewritten"} }); let (mut runner, mut events, permission_tx) = runner_with_hook(&rewrite.to_string()); runner.params.config.permission_mode = PermissionMode::AskAnyWrite; @@ -72,7 +73,7 @@ async fn rewritten_action_is_the_action_permission_classifies() { input = input_rx.recv() => input.unwrap(), result = &mut execution => panic!("execution ended before approval: {result:?}"), }; - assert_eq!(approved_input["file_path"], json!(target)); + assert_eq!(approved_input["file_path"], json!(hook_target)); permission_tx.send(true).await.unwrap(); let _ = execution.await.unwrap(); event_task.await.unwrap(); diff --git a/crates/loopal-runtime/tests/agent_loop/tool_result_image_sink_test.rs b/crates/loopal-runtime/tests/agent_loop/tool_result_image_sink_test.rs index 3498c2a9..9337e8dc 100644 --- a/crates/loopal-runtime/tests/agent_loop/tool_result_image_sink_test.rs +++ b/crates/loopal-runtime/tests/agent_loop/tool_result_image_sink_test.rs @@ -1,6 +1,6 @@ -use base64::Engine; use loopal_protocol::AgentEventPayload; use loopal_provider_api::ContentBlock; +use loopal_storage::FileResourceStore; use loopal_tool_api::PermissionMode; use loopal_tool_invocation::ToolImageBlock; use serde_json::json; @@ -20,8 +20,13 @@ async fn validated_image_flows_through_final_event_and_block_sink() { let (mut runner, mut events, _, _, _) = make_runner_with_channels(); runner.params.config.permission_mode = PermissionMode::Bypass; let temp = tempfile::tempdir().unwrap(); + runner.params.resource_store = Some(FileResourceStore::with_base_dir( + temp.path().join("resources"), + )); let path = temp.path().join("sink.png"); - std::fs::write(&path, minimal_png()).unwrap(); + let mut image = minimal_png(); + image.resize(300 * 1024, 0); + std::fs::write(&path, &image).unwrap(); runner.tool_ctx.backend = loopal_backend::LocalBackend::new( temp.path().to_path_buf(), None, @@ -57,17 +62,8 @@ async fn validated_image_flows_through_final_event_and_block_sink() { panic!("expected ToolResult"); }; assert_eq!(images.len(), 1); - match &images[0] { - ToolImageBlock::Inline { data, .. } => { - assert_eq!( - base64::engine::general_purpose::STANDARD - .decode(data) - .unwrap(), - minimal_png() - ); - } - ToolImageBlock::SessionResource { byte_size, .. } => { - assert_eq!(*byte_size, minimal_png().len()); - } - } + let ToolImageBlock::SessionResource { byte_size, .. } = &images[0] else { + panic!("injected resource store must persist the inline image"); + }; + assert_eq!(*byte_size, image.len()); } diff --git a/crates/loopal-runtime/tests/suite/post_hook_secret_input_test.rs b/crates/loopal-runtime/tests/suite/post_hook_secret_input_test.rs index 42e70427..0f0484d7 100644 --- a/crates/loopal-runtime/tests/suite/post_hook_secret_input_test.rs +++ b/crates/loopal-runtime/tests/suite/post_hook_secret_input_test.rs @@ -13,6 +13,20 @@ use serde_json::json; struct OneSecret; +fn shell_path(path: &std::path::Path) -> String { + path.to_string_lossy() + .replace('\\', "/") + .replace('\'', "'\\''") +} + +fn print_token_command() -> &'static str { + if cfg!(windows) { + "echo %TOKEN%" + } else { + "printf '%s' \"$TOKEN\"" + } +} + #[async_trait] impl SecretClient for OneSecret { async fn get(&self, _name: &str, _budget: IpcBudget) -> SecretResult { @@ -44,7 +58,7 @@ async fn post_hook_receives_placeholder_input_not_plaintext() { event: HookEvent::PostToolUse, command: format!( "cat > '{}'; printf '%s\\n' '{{\"additional_context\":\"post-hook-plaintext-canary\"}}'", - capture.display() + shell_path(&capture) ), tool_filter: Some(vec!["Bash".into()]), timeout_ms: 5000, @@ -75,7 +89,7 @@ async fn post_hook_receives_placeholder_input_not_plaintext() { "id", "Bash", json!({ - "command": "printf '%s' \"$TOKEN\"", + "command": print_token_command(), "env": {"TOKEN": ""} }), ) diff --git a/crates/loopal-runtime/tests/suite/unresolved_secret_effect_test.rs b/crates/loopal-runtime/tests/suite/unresolved_secret_effect_test.rs index 77b28634..427787b9 100644 --- a/crates/loopal-runtime/tests/suite/unresolved_secret_effect_test.rs +++ b/crates/loopal-runtime/tests/suite/unresolved_secret_effect_test.rs @@ -74,14 +74,20 @@ fn context() -> ToolContext { async fn bash_action_at( kernel: &Kernel, - marker: &str, + marker: &std::path::Path, ) -> loopal_runtime::tool_action::PreparedToolAction { + let command = if cfg!(windows) { + format!("echo effect>\"{}\"", marker.display()) + } else { + let marker = marker.to_string_lossy().replace('\'', "'\\''"); + format!("printf effect > '{marker}'") + }; prepare_tool_action( kernel, "id", "Bash", json!({ - "command": format!("printf effect > {marker}"), + "command": command, "env": {"TOKEN": ""} }), ) @@ -91,38 +97,33 @@ async fn bash_action_at( .unwrap() } -async fn bash_action(kernel: &Kernel) -> loopal_runtime::tool_action::PreparedToolAction { - bash_action_at(kernel, "/tmp/loopal-unresolved-secret-effect").await -} - #[tokio::test] async fn unresolved_wire_ref_fails_closed_but_missing_marker_can_execute() { - let path = std::path::Path::new("/tmp/loopal-unresolved-secret-effect"); - let _ = std::fs::remove_file(path); + let root = tempfile::tempdir().unwrap(); + let path = root.path().join("unresolved-secret-effect"); let kernel = Kernel::new(Settings::default()).unwrap(); - let action = bash_action(&kernel).await; + let action = bash_action_at(&kernel, &path).await; let error = execute_tool(&kernel, action, &context(), &AgentMode::Act) .await .expect_err("missing secret client must fail closed"); assert!(error.to_string().contains("secret resolution failed")); assert!(!path.exists()); - let action = bash_action(&kernel).await; + let action = bash_action_at(&kernel, &path).await; let ctx = context().with_secret_client(Arc::new(MissingSecret)); let result = execute_tool(&kernel, action, &ctx, &AgentMode::Act) .await .expect("resolved missing-secret marker is safe literal input"); assert!(!result.is_error); assert!(path.exists()); - let _ = std::fs::remove_file(path); } #[tokio::test] async fn denied_secret_resolution_cannot_execute_the_effect() { - let path = std::path::Path::new("/tmp/loopal-denied-secret-effect"); - let _ = std::fs::remove_file(path); + let root = tempfile::tempdir().unwrap(); + let path = root.path().join("denied-secret-effect"); let kernel = Kernel::new(Settings::default()).unwrap(); - let action = bash_action_at(&kernel, path.to_str().unwrap()).await; + let action = bash_action_at(&kernel, &path).await; let ctx = context().with_secret_client(Arc::new(DeniedSecret)); let error = execute_tool(&kernel, action, &ctx, &AgentMode::Act) diff --git a/crates/loopal-secret-runtime/src/hooks.rs b/crates/loopal-secret-runtime/src/hooks.rs index edee3a84..b54dca27 100644 --- a/crates/loopal-secret-runtime/src/hooks.rs +++ b/crates/loopal-secret-runtime/src/hooks.rs @@ -6,16 +6,59 @@ use secrecy::{ExposeSecret, SecretString}; use serde_json::Value; use tracing::warn; -use crate::audit::{JsonlAuditSink, RuntimeOp, default_telemetry_dir}; +use crate::audit::{JsonlAuditSink, RuntimeOp}; use crate::redactor::Redactor; use crate::resolver::{collect_wire_refs, resolve_in_value}; +mod audit; + +use audit::{record_audit, record_redaction_hits_inner}; +pub use audit::{record_redaction_hits, record_redaction_hits_with_audit}; + pub async fn apply_resolver( tool_name: &str, effective_input: &mut Value, whitelist: &[&str], client: Option<&Arc>, session_id: &str, +) -> Vec<(String, SecretString)> { + apply_resolver_inner( + tool_name, + effective_input, + whitelist, + client, + session_id, + None, + ) + .await +} + +pub async fn apply_resolver_with_audit( + tool_name: &str, + effective_input: &mut Value, + whitelist: &[&str], + client: Option<&Arc>, + session_id: &str, + audit: &JsonlAuditSink, +) -> Vec<(String, SecretString)> { + apply_resolver_inner( + tool_name, + effective_input, + whitelist, + client, + session_id, + Some(audit), + ) + .await +} + +async fn apply_resolver_inner( + tool_name: &str, + effective_input: &mut Value, + whitelist: &[&str], + client: Option<&Arc>, + session_id: &str, + audit: Option<&JsonlAuditSink>, ) -> Vec<(String, SecretString)> { let Some(client) = client else { return Vec::new(); @@ -64,6 +107,7 @@ pub async fn apply_resolver( session_id: Some(session_id), ..AuditMetadata::default() }, + audit, ); let leaked = detect_argv_exposure(effective_input, &seed); @@ -81,6 +125,7 @@ pub async fn apply_resolver( session_id: Some(session_id), ..AuditMetadata::default() }, + audit, ); } seed @@ -123,38 +168,32 @@ pub fn apply_redactor( content: String, seed: &[(String, SecretString)], session_id: &str, +) -> String { + apply_redactor_inner(tool_name, content, seed, session_id, None) +} + +pub fn apply_redactor_with_audit( + tool_name: &str, + content: String, + seed: &[(String, SecretString)], + session_id: &str, + audit: &JsonlAuditSink, +) -> String { + apply_redactor_inner(tool_name, content, seed, session_id, Some(audit)) +} + +fn apply_redactor_inner( + tool_name: &str, + content: String, + seed: &[(String, SecretString)], + session_id: &str, + audit: Option<&JsonlAuditSink>, ) -> String { if seed.is_empty() { return content; } let redactor = Redactor::from_pairs(seed); let (redacted, hit_names) = redactor.scan_and_redact(&content); - record_redaction_hits(tool_name, &hit_names, session_id); + record_redaction_hits_inner(tool_name, &hit_names, session_id, audit); redacted } - -pub fn record_redaction_hits(tool_name: &str, hit_names: &[String], session_id: &str) { - if hit_names.is_empty() { - return; - } - warn!(tool = tool_name, hit = ?hit_names, "redacted plaintext from tool output"); - record_audit( - RuntimeOp::Redacted, - hit_names, - &AuditMetadata { - session_id: Some(session_id), - ..AuditMetadata::default() - }, - ); -} - -fn record_audit(op: RuntimeOp, names: &[String], metadata: &AuditMetadata<'_>) { - let Some(dir) = default_telemetry_dir() else { - warn!("runtime audit directory unavailable"); - return; - }; - let sink = JsonlAuditSink::new(dir); - if let Err(error) = sink.record_runtime(op, names, metadata) { - warn!(%error, "runtime protected audit failed"); - } -} diff --git a/crates/loopal-secret-runtime/src/hooks/audit.rs b/crates/loopal-secret-runtime/src/hooks/audit.rs new file mode 100644 index 00000000..34bdae18 --- /dev/null +++ b/crates/loopal-secret-runtime/src/hooks/audit.rs @@ -0,0 +1,61 @@ +use loopal_vault_api::AuditMetadata; +use tracing::warn; + +use crate::audit::{JsonlAuditSink, RuntimeOp, default_telemetry_dir}; + +pub fn record_redaction_hits(tool_name: &str, hit_names: &[String], session_id: &str) { + record_redaction_hits_inner(tool_name, hit_names, session_id, None); +} + +pub fn record_redaction_hits_with_audit( + tool_name: &str, + hit_names: &[String], + session_id: &str, + audit: &JsonlAuditSink, +) { + record_redaction_hits_inner(tool_name, hit_names, session_id, Some(audit)); +} + +pub(super) fn record_redaction_hits_inner( + tool_name: &str, + hit_names: &[String], + session_id: &str, + audit: Option<&JsonlAuditSink>, +) { + if hit_names.is_empty() { + return; + } + warn!(tool = tool_name, hit = ?hit_names, "redacted plaintext from tool output"); + record_audit( + RuntimeOp::Redacted, + hit_names, + &AuditMetadata { + session_id: Some(session_id), + ..AuditMetadata::default() + }, + audit, + ); +} + +pub(super) fn record_audit( + op: RuntimeOp, + names: &[String], + metadata: &AuditMetadata<'_>, + audit: Option<&JsonlAuditSink>, +) { + let default_sink; + let sink = match audit { + Some(sink) => sink, + None => { + let Some(dir) = default_telemetry_dir() else { + warn!("runtime audit directory unavailable"); + return; + }; + default_sink = JsonlAuditSink::new(dir); + &default_sink + } + }; + if let Err(error) = sink.record_runtime(op, names, metadata) { + warn!(%error, "runtime protected audit failed"); + } +} diff --git a/crates/loopal-secret-runtime/src/lib.rs b/crates/loopal-secret-runtime/src/lib.rs index 4ef254b5..8896d106 100644 --- a/crates/loopal-secret-runtime/src/lib.rs +++ b/crates/loopal-secret-runtime/src/lib.rs @@ -8,7 +8,10 @@ pub mod template; pub use audit::{JsonlAuditSink, RuntimeOp, default_telemetry_dir}; pub use guard::{SECRET_REJECTION_MESSAGE, WIRE_REF_MARKER, input_contains_secret_ref}; -pub use hooks::{apply_redactor, apply_resolver, detect_argv_exposure, record_redaction_hits}; +pub use hooks::{ + apply_redactor, apply_redactor_with_audit, apply_resolver, apply_resolver_with_audit, + detect_argv_exposure, record_redaction_hits, record_redaction_hits_with_audit, +}; pub use merged::MergedVault; pub use redactor::Redactor; pub use resolver::{ResolverReport, collect_wire_refs, resolve_in_value}; diff --git a/crates/loopal-secret-runtime/tests/suite/hooks_test.rs b/crates/loopal-secret-runtime/tests/suite/hooks_test.rs index fc51fe8f..8fa1c717 100644 --- a/crates/loopal-secret-runtime/tests/suite/hooks_test.rs +++ b/crates/loopal-secret-runtime/tests/suite/hooks_test.rs @@ -4,7 +4,9 @@ use std::sync::atomic::{AtomicUsize, Ordering}; use async_trait::async_trait; use loopal_secret_client::{IpcBudget, SecretClient, SecretError, SecretResult}; use loopal_secret_runtime::{ - apply_redactor, apply_resolver, detect_argv_exposure, record_redaction_hits, + JsonlAuditSink, apply_redactor, apply_redactor_with_audit, apply_resolver, + apply_resolver_with_audit, detect_argv_exposure, record_redaction_hits, + record_redaction_hits_with_audit, }; use secrecy::{ExposeSecret, SecretString}; use serde_json::json; @@ -48,25 +50,6 @@ fn client() -> Arc { }) } -struct HomeGuard(Option); - -impl HomeGuard { - fn set(path: &std::path::Path) -> Self { - let previous = std::env::var_os("HOME"); - unsafe { std::env::set_var("HOME", path) }; - Self(previous) - } -} - -impl Drop for HomeGuard { - fn drop(&mut self) { - match self.0.take() { - Some(value) => unsafe { std::env::set_var("HOME", value) }, - None => unsafe { std::env::remove_var("HOME") }, - } - } -} - #[tokio::test] async fn resolver_early_returns_preserve_placeholders() { let mut input = json!({"command": ""}); @@ -106,7 +89,8 @@ async fn resolver_early_returns_preserve_placeholders() { #[tokio::test] async fn resolver_and_redactor_keep_plaintext_consumer_scoped() { let home = tempfile::tempdir().unwrap(); - let _home = HomeGuard::set(home.path()); + let telemetry_dir = home.path().join(".loopal/telemetry"); + let audit_sink = JsonlAuditSink::new(telemetry_dir.clone()); let client = client(); let secret_client: Arc = client; let mut input = json!({ @@ -115,12 +99,13 @@ async fn resolver_and_redactor_keep_plaintext_consumer_scoped() { "description": "" }); - let seed = apply_resolver( + let seed = apply_resolver_with_audit( "Bash", &mut input, &["command", "env"], Some(&secret_client), "session-1", + &audit_sink, ) .await; @@ -135,7 +120,13 @@ async fn resolver_and_redactor_keep_plaintext_consumer_scoped() { ); assert_eq!( - apply_redactor("Bash", "leaked sk-present".into(), &seed, "session-1"), + apply_redactor_with_audit( + "Bash", + "leaked sk-present".into(), + &seed, + "session-1", + &audit_sink, + ), "leaked " ); assert_eq!( @@ -152,10 +143,14 @@ async fn resolver_and_redactor_keep_plaintext_consumer_scoped() { assert!(!audit.contains("sk-present")); assert!(!audit.contains("env-secret")); - let telemetry_dir = home.path().join(".loopal/telemetry"); std::fs::remove_dir_all(&telemetry_dir).unwrap(); std::fs::write(&telemetry_dir, "not a directory").unwrap(); - record_redaction_hits("Bash", &[String::from("present")], "session-audit-error"); + record_redaction_hits_with_audit( + "Bash", + &[String::from("present")], + "session-audit-error", + &audit_sink, + ); } #[tokio::test] diff --git a/crates/loopal-storage/src/resources.rs b/crates/loopal-storage/src/resources.rs index bb202795..a0660e0a 100644 --- a/crates/loopal-storage/src/resources.rs +++ b/crates/loopal-storage/src/resources.rs @@ -117,12 +117,12 @@ impl ResourceStore for FileResourceStore { file.write_all(bytes).await?; file.sync_all().await?; drop(file); - file_io::replace_file(&tmp, &path).await + file_io::replace_file(&tmp, &path, bytes).await } .await; if let Err(error) = prepared { let _ = fs::remove_file(&tmp).await; - return Err(error.into()); + return Err(error); } Ok(id) } diff --git a/crates/loopal-storage/src/resources/file_io.rs b/crates/loopal-storage/src/resources/file_io.rs index 34e13453..9695e657 100644 --- a/crates/loopal-storage/src/resources/file_io.rs +++ b/crates/loopal-storage/src/resources/file_io.rs @@ -4,6 +4,8 @@ use loopal_error::StorageError; use tokio::fs; use tokio::io::AsyncReadExt; +const REPLACE_RETRY_LIMIT: usize = 8; + pub(super) async fn existing_matches(path: &Path, expected: &[u8]) -> Result { match open_regular_bounded(path, expected.len()).await { Ok((file, existing)) if existing == expected => { @@ -37,8 +39,69 @@ pub(super) async fn enforce_private_permissions(_file: &fs::File) -> std::io::Re Ok(()) } -pub(super) async fn replace_file(temp: &Path, target: &Path) -> std::io::Result<()> { - replace_file_inner(temp, target).await +pub(super) async fn replace_file( + temp: &Path, + target: &Path, + expected: &[u8], +) -> Result<(), StorageError> { + let mut retries = 0usize; + loop { + let replace_error = match replace_file_inner(temp, target).await { + Ok(()) => return Ok(()), + Err(error) => error, + }; + + // A content-addressed writer may have installed the same bytes while + // this replace was racing with it, especially on Windows. + match existing_matches(target, expected).await { + Ok(true) => { + if let Err(error) = fs::remove_file(temp).await + && error.kind() != std::io::ErrorKind::NotFound + { + return Err(error.into()); + } + return Ok(()); + } + Ok(false) => {} + Err(error) + if retryable_replace_error(&replace_error) + && retryable_verification_error(&error) + && retries < REPLACE_RETRY_LIMIT => {} + Err(error) => return Err(error), + } + + if !retryable_replace_error(&replace_error) || retries >= REPLACE_RETRY_LIMIT { + return Err(replace_error.into()); + } + retries += 1; + tokio::time::sleep(replace_retry_delay(retries)).await; + } +} + +#[cfg(windows)] +fn retryable_replace_error(error: &std::io::Error) -> bool { + use windows_sys::Win32::Foundation::{ + ERROR_ACCESS_DENIED, ERROR_LOCK_VIOLATION, ERROR_SHARING_VIOLATION, + }; + + matches!( + error.raw_os_error().map(|value| value as u32), + Some(ERROR_ACCESS_DENIED | ERROR_LOCK_VIOLATION | ERROR_SHARING_VIOLATION) + ) +} + +#[cfg(not(windows))] +fn retryable_replace_error(_error: &std::io::Error) -> bool { + false +} + +fn retryable_verification_error(error: &StorageError) -> bool { + matches!(error, StorageError::Io(error) if retryable_replace_error(error)) +} + +fn replace_retry_delay(retry: usize) -> std::time::Duration { + let shift = u32::try_from(retry.min(5)).unwrap_or(5); + std::time::Duration::from_millis(1u64 << shift) } pub(super) async fn read_regular_bounded( diff --git a/crates/loopal-storage/src/resources/file_io_tests.rs b/crates/loopal-storage/src/resources/file_io_tests.rs index 2b2db6c1..22a6d5b9 100644 --- a/crates/loopal-storage/src/resources/file_io_tests.rs +++ b/crates/loopal-storage/src/resources/file_io_tests.rs @@ -1,6 +1,6 @@ use loopal_error::StorageError; -use super::file_io::read_regular_bounded; +use super::file_io::{read_regular_bounded, replace_file}; #[cfg(unix)] use super::file_io::existing_matches; @@ -14,6 +14,112 @@ async fn bounded_read_rejects_a_directory() { )); } +#[tokio::test] +async fn failed_replace_accepts_an_existing_matching_winner() { + let temp = tempfile::tempdir().unwrap(); + let missing = temp.path().join("missing-temp"); + let target = temp.path().join("resource"); + std::fs::write(&target, b"expected").unwrap(); + + replace_file(&missing, &target, b"expected").await.unwrap(); + + assert_eq!(std::fs::read(target).unwrap(), b"expected"); +} + +#[tokio::test] +async fn matching_winner_reports_loser_cleanup_failure() { + let root = tempfile::tempdir().unwrap(); + let temp = root.path().join("invalid-temp-directory"); + let target = root.path().join("resource"); + std::fs::create_dir(&temp).unwrap(); + std::fs::write(&target, b"expected").unwrap(); + + assert!(matches!( + replace_file(&temp, &target, b"expected").await, + Err(StorageError::Io(_)) + )); + assert!(temp.is_dir()); + assert_eq!(std::fs::read(target).unwrap(), b"expected"); +} + +#[tokio::test] +async fn failed_replace_does_not_accept_mismatched_content() { + let temp = tempfile::tempdir().unwrap(); + let missing = temp.path().join("missing-temp"); + let target = temp.path().join("resource"); + std::fs::write(&target, b"tampered").unwrap(); + + let error = replace_file(&missing, &target, b"expected") + .await + .unwrap_err(); + + assert!(matches!(error, StorageError::Io(_))); + assert_eq!(std::fs::read(target).unwrap(), b"tampered"); +} + +#[tokio::test] +async fn failed_replace_rejects_a_non_regular_winner() { + let temp = tempfile::tempdir().unwrap(); + let missing = temp.path().join("missing-temp"); + let target = temp.path().join("resource"); + std::fs::create_dir(&target).unwrap(); + + assert!(matches!( + replace_file(&missing, &target, b"expected").await, + Err(StorageError::ResourceIntegrity) + )); +} + +#[cfg(windows)] +#[tokio::test] +async fn matching_locked_winner_resolves_replace_competition_and_cleans_temp() { + use std::os::windows::fs::OpenOptionsExt; + use windows_sys::Win32::Storage::FileSystem::{FILE_SHARE_READ, FILE_SHARE_WRITE}; + + let root = tempfile::tempdir().unwrap(); + let temp = root.path().join("prepared-temp"); + let target = root.path().join("resource"); + std::fs::write(&temp, b"expected").unwrap(); + std::fs::write(&target, b"expected").unwrap(); + let locked_target = std::fs::OpenOptions::new() + .read(true) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE) + .open(&target) + .unwrap(); + + replace_file(&temp, &target, b"expected").await.unwrap(); + + assert!(!temp.exists()); + assert_eq!(std::fs::read(&target).unwrap(), b"expected"); + drop(locked_target); +} + +#[cfg(windows)] +#[tokio::test] +async fn mismatched_locked_winner_fails_closed_after_bounded_retries() { + use std::os::windows::fs::OpenOptionsExt; + use windows_sys::Win32::Storage::FileSystem::{FILE_SHARE_READ, FILE_SHARE_WRITE}; + + let root = tempfile::tempdir().unwrap(); + let temp = root.path().join("prepared-temp"); + let target = root.path().join("resource"); + std::fs::write(&temp, b"expected").unwrap(); + std::fs::write(&target, b"tampered").unwrap(); + let locked_target = std::fs::OpenOptions::new() + .read(true) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE) + .open(&target) + .unwrap(); + + assert!(matches!( + replace_file(&temp, &target, b"expected").await, + Err(StorageError::Io(_)) + )); + assert!(temp.exists()); + assert_eq!(std::fs::read(&target).unwrap(), b"tampered"); + drop(locked_target); +} + #[cfg(unix)] #[tokio::test] async fn existing_match_propagates_nonmissing_open_errors() { diff --git a/crates/loopal-storage/src/workflow_journal/fs/windows.rs b/crates/loopal-storage/src/workflow_journal/fs/windows.rs index 8c6d6ea1..1a76cba2 100644 --- a/crates/loopal-storage/src/workflow_journal/fs/windows.rs +++ b/crates/loopal-storage/src/workflow_journal/fs/windows.rs @@ -65,7 +65,8 @@ pub(super) fn discover(base: &Path, session: &str) -> Result, FsError let name = entry.file_name(); let mut options = OpenOptions::new(); options.read(true); - secure_options(&mut options, false, true); + // Open directories too, then reject them from the opened handle. + secure_options(&mut options, true, true); let file = options.open(directory.join(&name)).map_err(classify_open)?; let opened = validate_file(&file)?; let bytes = file.metadata().map_err(FsError::Io)?.len(); diff --git a/crates/loopal-storage/src/workflow_journal/fs/windows/tests.rs b/crates/loopal-storage/src/workflow_journal/fs/windows/tests.rs index a4ad0e43..45c29f2b 100644 --- a/crates/loopal-storage/src/workflow_journal/fs/windows/tests.rs +++ b/crates/loopal-storage/src/workflow_journal/fs/windows/tests.rs @@ -1,5 +1,5 @@ -use super::{OpenMode, open, workflows_directory}; -use crate::workflow_journal::fs::JournalLocation; +use super::{OpenMode, discover, open, workflows_directory}; +use crate::workflow_journal::fs::{FsError, JournalLocation}; #[test] fn open_parent_handles_block_directory_replacement() { @@ -32,3 +32,18 @@ fn append_handle_is_exclusive_until_released() { .unwrap_or_else(|_| panic!("journal did not reopen after writer release")); drop(reopened); } + +#[test] +fn discovery_rejects_a_directory_from_its_opened_handle() { + let temp = tempfile::tempdir().unwrap(); + let (_guards, directory) = workflows_directory(temp.path(), "session-one", true) + .unwrap_or_else(|_| panic!("workflow directory creation failed")); + std::fs::create_dir(directory.join("wrun_directory.jsonl")).unwrap(); + + assert!(matches!( + discover(temp.path(), "session-one"), + Err(FsError::Integrity( + "workflow journal is not a private regular file" + )) + )); +} diff --git a/crates/loopal-storage/tests/suite/resources_test.rs b/crates/loopal-storage/tests/suite/resources_test.rs index dcaf2b22..37cd04a8 100644 --- a/crates/loopal-storage/tests/suite/resources_test.rs +++ b/crates/loopal-storage/tests/suite/resources_test.rs @@ -133,4 +133,10 @@ async fn concurrent_writes_of_same_content_yield_one_file() { .await .unwrap(); assert_eq!(read, payload); + let resources = dir.path().join("sessions/sess-c/resources"); + let entries = std::fs::read_dir(resources) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect::>(); + assert_eq!(entries, vec![std::ffi::OsString::from(first)]); } diff --git a/src/bootstrap/mod.rs b/src/bootstrap/mod.rs index f43c1116..1d628a26 100644 --- a/src/bootstrap/mod.rs +++ b/src/bootstrap/mod.rs @@ -151,11 +151,11 @@ pub async fn run() -> anyhow::Result<()> { } if cli.parent_only.serve { - let test_provider = cli - .parent_only - .test_provider - .clone() - .or_else(|| std::env::var("LOOPAL_TEST_PROVIDER").ok()); + let test_provider = match cli.parent_only.test_provider.clone() { + Some(path) => Some(path), + None => loopal_agent_client::resolve_runfile_env("LOOPAL_TEST_PROVIDER")? + .map(|path| path.to_string_lossy().into_owned()), + }; if let Some(path) = test_provider { return loopal_agent_server::run_agent_server_with_mock(&path).await; } diff --git a/src/bootstrap/modes/acp_lifecycle_tests.rs b/src/bootstrap/modes/acp_lifecycle_tests.rs index b74ce510..7cb9c03e 100644 --- a/src/bootstrap/modes/acp_lifecycle_tests.rs +++ b/src/bootstrap/modes/acp_lifecycle_tests.rs @@ -8,7 +8,7 @@ use crate::bootstrap::lifecycle_test_support::{EnvGuard, assert_runtime_fixture, #[tokio::test] #[ignore = "real-process Bazel coverage producer"] async fn acp_entrypoint_owns_a_real_hub_and_agent_lifecycle() { - assert_runtime_fixture(); + let _fixtures = assert_runtime_fixture(); let home = tempfile::tempdir().expect("create ACP coverage home"); let _home = EnvGuard::set("HOME", home.path()); let project = tempfile::tempdir().expect("create ACP coverage project"); diff --git a/src/bootstrap/modes/hub_only_lifecycle_tests.rs b/src/bootstrap/modes/hub_only_lifecycle_tests.rs index 0c883b97..eefde944 100644 --- a/src/bootstrap/modes/hub_only_lifecycle_tests.rs +++ b/src/bootstrap/modes/hub_only_lifecycle_tests.rs @@ -14,7 +14,7 @@ use super::{StartupProtocol, run, run_desktop, run_with_protocol_observed}; #[tokio::test] #[ignore = "real-process Bazel coverage producer"] async fn hub_only_runs_enabled_workflow_runtime_until_remote_shutdown() { - assert_runtime_fixture(); + let _fixtures = assert_runtime_fixture(); let home = tempfile::tempdir().expect("create Hub-only coverage home"); let _home = EnvGuard::set("HOME", home.path()); let project = tempfile::tempdir().expect("create Hub-only coverage project"); @@ -42,7 +42,7 @@ async fn hub_only_runs_enabled_workflow_runtime_until_remote_shutdown() { #[tokio::test] #[ignore = "real-process Bazel coverage producer"] async fn missing_resume_reports_startup_failure_and_rolls_back() { - assert_runtime_fixture(); + let _fixtures = assert_runtime_fixture(); let home = tempfile::tempdir().expect("create resume coverage home"); let _home = EnvGuard::set("HOME", home.path()); let project = tempfile::tempdir().expect("create resume coverage project"); @@ -70,7 +70,7 @@ async fn missing_resume_reports_startup_failure_and_rolls_back() { #[ignore = "real-process Bazel coverage producer"] #[cfg(unix)] async fn desktop_entrypoint_rejects_a_parent_that_exited_during_startup() { - assert_runtime_fixture(); + let _fixtures = assert_runtime_fixture(); let home = tempfile::tempdir().expect("create Desktop failure coverage home"); let _home = EnvGuard::set("HOME", home.path()); let project = tempfile::tempdir().expect("create Desktop failure coverage project"); @@ -105,7 +105,7 @@ async fn desktop_entrypoint_rejects_a_parent_that_exited_during_startup() { #[ignore = "real-process Bazel coverage producer"] #[cfg(unix)] async fn desktop_covers_parent_exit_and_hub_shutdown_paths() { - assert_runtime_fixture(); + let _fixtures = assert_runtime_fixture(); let home = tempfile::tempdir().expect("create Desktop coverage home"); let _home = EnvGuard::set("HOME", home.path()); for parent_exits in [true, false] { diff --git a/src/bootstrap/modes/hub_only_resume_lifecycle_tests.rs b/src/bootstrap/modes/hub_only_resume_lifecycle_tests.rs index f2454df2..9f2bead0 100644 --- a/src/bootstrap/modes/hub_only_resume_lifecycle_tests.rs +++ b/src/bootstrap/modes/hub_only_resume_lifecycle_tests.rs @@ -11,7 +11,7 @@ use super::run; #[tokio::test] #[ignore = "real-process Bazel coverage producer"] async fn hub_only_can_resume_the_session_created_by_an_earlier_run() { - assert_runtime_fixture(); + let _fixtures = assert_runtime_fixture(); let home = tempfile::tempdir().expect("create Hub-only resume home"); let _home = EnvGuard::set("HOME", home.path()); let project = tempfile::tempdir().expect("create Hub-only resume project"); diff --git a/src/bootstrap/modes/lifecycle_test_support.rs b/src/bootstrap/modes/lifecycle_test_support.rs index 33e14a92..1820aadc 100644 --- a/src/bootstrap/modes/lifecycle_test_support.rs +++ b/src/bootstrap/modes/lifecycle_test_support.rs @@ -33,10 +33,19 @@ impl Drop for EnvGuard { } } -pub fn assert_runtime_fixture() { - for variable in ["LOOPAL_BINARY", "LOOPAL_TEST_PROVIDER"] { - let path = std::env::var(variable).unwrap_or_else(|_| panic!("{variable} must be set")); - assert!(std::path::Path::new(&path).is_file(), "missing {variable}"); +pub struct RuntimeFixtureGuard { + _binary: EnvGuard, + _provider: EnvGuard, +} + +pub fn assert_runtime_fixture() -> RuntimeFixtureGuard { + let binary = loopal_agent_client::require_runfile_env("LOOPAL_BINARY") + .expect("resolve LOOPAL_BINARY fixture"); + let provider = loopal_agent_client::require_runfile_env("LOOPAL_TEST_PROVIDER") + .expect("resolve LOOPAL_TEST_PROVIDER fixture"); + RuntimeFixtureGuard { + _binary: EnvGuard::set("LOOPAL_BINARY", binary), + _provider: EnvGuard::set("LOOPAL_TEST_PROVIDER", provider), } } diff --git a/src/bootstrap/modes/server_mode/lifecycle_tests.rs b/src/bootstrap/modes/server_mode/lifecycle_tests.rs index 5892f39c..cb3d778a 100644 --- a/src/bootstrap/modes/server_mode/lifecycle_tests.rs +++ b/src/bootstrap/modes/server_mode/lifecycle_tests.rs @@ -30,10 +30,7 @@ fn config(root: &std::path::Path) -> loopal_config::ResolvedConfig { #[tokio::test] #[ignore = "real-process Bazel coverage producer"] async fn ephemeral_server_runs_real_agent_and_shuts_down() { - for variable in ["LOOPAL_BINARY", "LOOPAL_TEST_PROVIDER"] { - let path = std::env::var(variable).unwrap_or_else(|_| panic!("{variable} must be set")); - assert!(std::path::Path::new(&path).is_file(), "missing {variable}"); - } + let _fixtures = crate::bootstrap::lifecycle_test_support::assert_runtime_fixture(); let project = tempfile::tempdir().expect("create isolated server project"); let cli = Cli { diff --git a/tests/e2e/bootstrap_typestate.rs b/tests/e2e/bootstrap_typestate.rs index 4ccb4787..e3a7b2a3 100644 --- a/tests/e2e/bootstrap_typestate.rs +++ b/tests/e2e/bootstrap_typestate.rs @@ -22,8 +22,8 @@ use tokio::time::timeout; const ALIVE_BUDGET: Duration = Duration::from_secs(3); const READY_BUDGET: Duration = Duration::from_secs(8); -fn binary_path() -> String { - std::env::var("LOOPAL_BINARY").expect("LOOPAL_BINARY env required") +fn binary_path() -> std::path::PathBuf { + loopal_agent_client::require_runfile_env("LOOPAL_BINARY").expect("resolve LOOPAL_BINARY") } #[tokio::test] diff --git a/tests/e2e/cli_llm/support_process.rs b/tests/e2e/cli_llm/support_process.rs index 60e8dbb5..d9d7ee7b 100644 --- a/tests/e2e/cli_llm/support_process.rs +++ b/tests/e2e/cli_llm/support_process.rs @@ -65,9 +65,14 @@ impl Provider { } } -fn binary_path() -> String { - std::env::var("LOOPAL_BINARY") - .or_else(|_| std::env::var("CARGO_BIN_EXE_loopal")) +fn binary_path() -> std::path::PathBuf { + if let Some(path) = + loopal_agent_client::resolve_runfile_env("LOOPAL_BINARY").expect("resolve LOOPAL_BINARY") + { + return path; + } + std::env::var_os("CARGO_BIN_EXE_loopal") + .map(std::path::PathBuf::from) .expect("set LOOPAL_BINARY or CARGO_BIN_EXE_loopal to the loopal binary") } diff --git a/tests/e2e/desktop/support.rs b/tests/e2e/desktop/support.rs index c8a6c107..f49c0f99 100644 --- a/tests/e2e/desktop/support.rs +++ b/tests/e2e/desktop/support.rs @@ -16,16 +16,7 @@ pub const STARTUP_DEADLINE: Duration = Duration::from_secs(20); pub const EXIT_DEADLINE: Duration = Duration::from_secs(8); fn binary_path() -> std::path::PathBuf { - let configured = std::path::PathBuf::from( - std::env::var("LOOPAL_BINARY").expect("LOOPAL_BINARY env required"), - ); - if configured.is_absolute() { - configured - } else { - std::env::current_dir() - .expect("test current directory") - .join(configured) - } + loopal_agent_client::require_runfile_env("LOOPAL_BINARY").expect("resolve LOOPAL_BINARY") } pub fn write_mock_fixture() -> tempfile::NamedTempFile { diff --git a/tests/e2e/hub_lifecycle.rs b/tests/e2e/hub_lifecycle.rs index 0f5b9e3f..a1abb597 100644 --- a/tests/e2e/hub_lifecycle.rs +++ b/tests/e2e/hub_lifecycle.rs @@ -17,8 +17,8 @@ use tokio::time::timeout; const SPAWN_DEADLINE: Duration = Duration::from_secs(20); -fn binary_path() -> String { - std::env::var("LOOPAL_BINARY").expect("LOOPAL_BINARY env required") +fn binary_path() -> std::path::PathBuf { + loopal_agent_client::require_runfile_env("LOOPAL_BINARY").expect("resolve LOOPAL_BINARY") } fn write_mock_fixture() -> tempfile::NamedTempFile { diff --git a/tests/e2e/hub_llm/support_hub.rs b/tests/e2e/hub_llm/support_hub.rs index d9c773b4..faf84c9f 100644 --- a/tests/e2e/hub_llm/support_hub.rs +++ b/tests/e2e/hub_llm/support_hub.rs @@ -166,20 +166,15 @@ impl HubHarness { } } -fn binary_path() -> String { - let path = std::env::var("LOOPAL_BINARY").expect("LOOPAL_BINARY env required"); - // reason: the child uses a different cwd than Bazel's rootpath. - std::fs::canonicalize(&path) - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or(path) +fn binary_path() -> std::path::PathBuf { + loopal_agent_client::require_runfile_env("LOOPAL_BINARY").expect("resolve LOOPAL_BINARY") } fn write_hub_settings(home: &std::path::Path) { - let command = std::env::var("LOOPAL_MOCK_MCP_BINARY") - .expect("LOOPAL_MOCK_MCP_BINARY env required (bazel data dep)"); - let command = std::fs::canonicalize(&command) - .map(|p| p.to_string_lossy().into_owned()) - .unwrap_or(command); + let command = loopal_agent_client::require_runfile_env("LOOPAL_MOCK_MCP_BINARY") + .expect("resolve LOOPAL_MOCK_MCP_BINARY") + .to_string_lossy() + .into_owned(); let dir = home.join(".loopal"); std::fs::create_dir_all(&dir).unwrap(); let settings = json!({ diff --git a/tests/e2e/hub_llm/workflow_stale_completion_test.rs b/tests/e2e/hub_llm/workflow_stale_completion_test.rs index e8b0feba..7f25db8d 100644 --- a/tests/e2e/hub_llm/workflow_stale_completion_test.rs +++ b/tests/e2e/hub_llm/workflow_stale_completion_test.rs @@ -118,9 +118,8 @@ async fn success_reported_after_timeout_interrupt_cannot_win() { } fn install_late_completion_worker(env: &mut HubEnv) { - let fixture = std::env::var("LOOPAL_MOCK_WORKFLOW_WORKER_BINARY") - .expect("LOOPAL_MOCK_WORKFLOW_WORKER_BINARY env required"); - let fixture = std::fs::canonicalize(fixture).expect("resolve mock workflow worker"); + let fixture = loopal_agent_client::require_runfile_env("LOOPAL_MOCK_WORKFLOW_WORKER_BINARY") + .expect("resolve LOOPAL_MOCK_WORKFLOW_WORKER_BINARY"); let path = env.home.path().join("workflow-late-completion-shim"); let script = format!( r#"#!/bin/sh diff --git a/tests/e2e/join_hub.rs b/tests/e2e/join_hub.rs index e5d28fe6..60ef7c77 100644 --- a/tests/e2e/join_hub.rs +++ b/tests/e2e/join_hub.rs @@ -22,8 +22,8 @@ const SPAWN_DEADLINE: Duration = Duration::from_secs(20); const REGISTER_DEADLINE: Duration = Duration::from_secs(10); const TOKEN: &str = "test-token-deadbeef"; -fn binary_path() -> String { - std::env::var("LOOPAL_BINARY").expect("LOOPAL_BINARY env required") +fn binary_path() -> std::path::PathBuf { + loopal_agent_client::require_runfile_env("LOOPAL_BINARY").expect("resolve LOOPAL_BINARY") } fn write_mock_provider() -> tempfile::NamedTempFile { diff --git a/tests/e2e/system_ipc.rs b/tests/e2e/system_ipc.rs index 1764e50b..31431949 100644 --- a/tests/e2e/system_ipc.rs +++ b/tests/e2e/system_ipc.rs @@ -14,11 +14,14 @@ use loopal_ipc::protocol::methods; /// Path to the built binary. Checks LOOPAL_BINARY env var (Bazel), then /// CARGO_BIN_EXE_loopal (Cargo). -fn binary_path() -> String { - if let Ok(path) = std::env::var("LOOPAL_BINARY") { +fn binary_path() -> std::path::PathBuf { + if let Some(path) = + loopal_agent_client::resolve_runfile_env("LOOPAL_BINARY").expect("resolve LOOPAL_BINARY") + { return path; } - std::env::var("CARGO_BIN_EXE_loopal") + std::env::var_os("CARGO_BIN_EXE_loopal") + .map(std::path::PathBuf::from) .expect("Set LOOPAL_BINARY or CARGO_BIN_EXE_loopal to the loopal binary path") } diff --git a/tests/regressions/hub_only_mcp_deadlock.rs b/tests/regressions/hub_only_mcp_deadlock.rs index bd4e55a9..f4e1b456 100644 --- a/tests/regressions/hub_only_mcp_deadlock.rs +++ b/tests/regressions/hub_only_mcp_deadlock.rs @@ -13,8 +13,8 @@ use tokio::time::timeout; const HANDSHAKE_BUDGET: Duration = Duration::from_secs(5); -fn binary_path() -> String { - std::env::var("LOOPAL_BINARY").expect("LOOPAL_BINARY env required") +fn binary_path() -> std::path::PathBuf { + loopal_agent_client::require_runfile_env("LOOPAL_BINARY").expect("resolve LOOPAL_BINARY") } fn write_settings_with_unresponsive_mcp(home_dir: &std::path::Path) -> std::io::Result<()> {