diff --git a/src/app/harness.rs b/src/app/harness.rs index 9f341f3..b05a640 100644 --- a/src/app/harness.rs +++ b/src/app/harness.rs @@ -18,23 +18,31 @@ pub struct Harness; impl Harness { /// Decide whether a tool call is allowed to execute. /// - /// Flow: if the tool isn't flagged risky, allow immediately → otherwise - /// defer to `classify`. + /// Flow: if the tool isn't flagged risky, allow immediately → basic + /// content checks (path traversal) → defer to `classify`. /// - /// Why: `_args` and `_workspace_roots` are accepted for a future - /// content-aware classifier but currently unused — `classify` is a - /// stub that always allows. + /// Why: `classify` is currently a stub that always allows; the basic + /// checks here serve as defense-in-depth alongside the shell filters + /// and `resolve_path` in the tool modules. /// /// Return: `Verdict::Allow` or `Verdict::Block(reason)`. pub fn gate_tool_call( tool_name: &str, - _args: &serde_json::Value, + args: &serde_json::Value, _workspace_roots: &[&std::path::Path], ) -> Verdict { if !crate::tool::tool_is_risky(tool_name) { return Verdict::Allow; } + // Basic path traversal check for file-mutating tools. + if matches!(tool_name, "write" | "edit" | "delete") { + if let Some(path) = args.get("path").and_then(|v| v.as_str()) { + if path.contains("..") { + return Verdict::Block("path traversal detected in 'path' argument".to_string()); + } + } + } Self::classify(tool_name) } diff --git a/src/app/subagent/engine.rs b/src/app/subagent/engine.rs index c2c68b7..7bf2da5 100644 --- a/src/app/subagent/engine.rs +++ b/src/app/subagent/engine.rs @@ -101,9 +101,13 @@ pub fn run_subagent(ctx: SubagentContext, tx: mpsc::Sender) -> an let (tools, tdefs) = build_subagent_tools(&ctx.allowed_tools); let tdefs_opt: Option> = if tdefs.is_empty() { None } else { Some(tdefs) }; + // Cache provider config once before the loop instead of re-resolving + // from disk on every step (Settings::load + AppConfig::load each parse + // JSON files, and the config cannot change between steps). + let (api_key, model, base_url) = resolve_provider_config(); + let client = crate::service::provider::LlmClient::new(api_key, model, base_url); + for step in 0..ctx.max_steps { - let (api_key, model, base_url) = resolve_provider_config(); - let client = crate::service::provider::LlmClient::new(api_key, model, base_url); // Use the structured tool-calling API so the LLM can request tools with // proper arguments, exactly like the main agent does. diff --git a/src/model/agent_def/global.rs b/src/model/agent_def/global.rs index bf3ee5c..7696cac 100644 --- a/src/model/agent_def/global.rs +++ b/src/model/agent_def/global.rs @@ -36,13 +36,16 @@ pub fn load_global_agents() -> Vec { agents } -/// Persist a global agent definition as `/agents/.json`. +/// Persist a global agent definition as `/agents/.json`, +/// with fsync for crash safety. /// /// Flow: ensure the `agents/` directory exists → serialize `def` to -/// pretty JSON → write to a file named after `def.name`. +/// pretty JSON → write to a temp file → fsync → rename into place → +/// fsync parent directory. /// /// Why: writing by name overwrites any existing definition with the -/// same name, acting as an upsert. +/// same name, acting as an upsert; fsync prevents a torn write from +/// losing the definition on crash. /// /// Return: `Ok(())` on success, or an error if directory creation, /// serialization, or the write fails. @@ -51,8 +54,13 @@ pub fn save_global_agent(def: &AgentDefinition) -> anyhow::Result<()> { let agents_dir = store.base_dir.join("agents"); std::fs::create_dir_all(&agents_dir)?; let path = agents_dir.join(format!("{}.json", def.name)); + let tmp = agents_dir.join(format!("{}.json.tmp", def.name)); let content = serde_json::to_string_pretty(def)?; - std::fs::write(path, content)?; + std::fs::write(&tmp, content)?; + let f = std::fs::File::open(&tmp)?; + f.sync_all()?; + std::fs::rename(&tmp, path)?; + let _ = std::fs::File::open(&agents_dir).and_then(|d| d.sync_all()); Ok(()) } diff --git a/src/model/agent_def/session.rs b/src/model/agent_def/session.rs index 3b67800..62f03d5 100644 --- a/src/model/agent_def/session.rs +++ b/src/model/agent_def/session.rs @@ -31,17 +31,23 @@ pub fn load_session_agents(session_dir: &Path) -> Vec { } } -/// Overwrite `/agents.json` with the given agent list. +/// Overwrite `/agents.json` with the given agent list, +/// with fsync for crash safety. /// -/// Flow: serialize `agents` to pretty JSON → write to -/// `/agents.json`. +/// Flow: serialize `agents` to pretty JSON → write to a temp file → +/// fsync → rename over `agents.json` → fsync parent directory. /// /// Return: `Ok(())` on success, or an error if serialization or the /// write fails. pub fn save_session_agents(session_dir: &Path, agents: &[AgentDefinition]) -> anyhow::Result<()> { let agents_file = session_dir.join("agents.json"); + let tmp = session_dir.join("agents.json.tmp"); let content = serde_json::to_string_pretty(agents)?; - std::fs::write(agents_file, content)?; + std::fs::write(&tmp, content)?; + let f = std::fs::File::open(&tmp)?; + f.sync_all()?; + std::fs::rename(&tmp, agents_file)?; + let _ = std::fs::File::open(session_dir).and_then(|d| d.sync_all()); Ok(()) } diff --git a/src/model/editlog.rs b/src/model/editlog.rs index b743b99..a55dd4d 100644 --- a/src/model/editlog.rs +++ b/src/model/editlog.rs @@ -48,14 +48,16 @@ impl EditLog { .collect() } - /// Append one entry to `edits.jsonl` on disk and to the in-memory log. + /// Append one entry to `edits.jsonl` on disk and to the in-memory log, + /// with fsync for crash safety. /// /// Flow: serialize `entry` to a JSON line → ensure parent dir exists → - /// open the file in append mode → write the line → push into + /// open the file in append mode → write the line → fsync → push into /// `self.entries`. /// /// Why: appending (not rewriting) keeps the log durable and cheap even - /// as it grows across a long session. + /// as it grows across a long session; fsync ensures the entry survives + /// a crash rather than lingering in the page cache. /// /// Return: `Ok(())` on success; an `io::Error` if serialization or /// any filesystem operation fails. @@ -69,6 +71,7 @@ impl EditLog { .open(&self.path)?; use std::io::Write; file.write_all(line.as_bytes())?; + file.sync_all()?; self.entries.push(entry); Ok(()) } diff --git a/src/model/memory.rs b/src/model/memory.rs index 68b5ff1..c4e4641 100644 --- a/src/model/memory.rs +++ b/src/model/memory.rs @@ -219,7 +219,15 @@ pub fn export_lessons(memory_dir: &Path, output: &Path) -> std::io::Result<()> { .collect(); let data = serde_json::to_string_pretty(&lessons) .map_err(std::io::Error::other)?; - std::fs::write(output, data)?; + // Write to temp, fsync, then rename for crash-safe export + let tmp = output.with_extension("json.tmp"); + std::fs::write(&tmp, data)?; + let f = std::fs::File::open(&tmp)?; + f.sync_all()?; + std::fs::rename(&tmp, output)?; + if let Some(parent) = output.parent() { + let _ = std::fs::File::open(parent).and_then(|d| d.sync_all()); + } Ok(()) } /// Import memories from a JSON export file into `memory_dir`, skipping diff --git a/src/model/session.rs b/src/model/session.rs index ef44e4d..6132090 100644 --- a/src/model/session.rs +++ b/src/model/session.rs @@ -50,13 +50,16 @@ impl Session { self.session_dir(base_dir).join("conversation.json") } - /// Persist this session's metadata to `session.json`, atomically. + /// Persist this session's metadata to `session.json`, atomically + /// with fsync for crash safety. /// /// Flow: ensure the session directory exists → serialize to pretty - /// JSON → write to `session.json.tmp` → rename over `session.json`. + /// JSON → write to `session.json.tmp` → fsync → rename over + /// `session.json` → fsync parent directory. /// /// Why: write-then-rename avoids a torn/partial `session.json` if - /// interrupted mid-write. + /// interrupted mid-write; fsync before rename ensures the data is + /// on disk before the rename makes it visible. /// /// Return: `Ok(())` on success, or an `io::Error` from any step. pub fn save(&self, base_dir: &Path) -> std::io::Result<()> { @@ -66,7 +69,10 @@ impl Session { let data = serde_json::to_string_pretty(self)?; let tmp = dir.join("session.json.tmp"); std::fs::write(&tmp, data)?; + let f = std::fs::File::open(&tmp)?; + f.sync_all()?; std::fs::rename(&tmp, path)?; + let _ = std::fs::File::open(&dir).and_then(|d| d.sync_all()); Ok(()) } diff --git a/src/model/settings.rs b/src/model/settings.rs index ec3a602..61cb545 100644 --- a/src/model/settings.rs +++ b/src/model/settings.rs @@ -78,17 +78,24 @@ impl Settings { .unwrap_or_default() } - /// Serialize and write settings to `/settings.json`. + /// Serialize and write settings to `/settings.json`, + /// using write-then-rename with fsync for crash safety. /// - /// Flow: ensure base dir exists → pretty-print JSON → write to disk. + /// Flow: ensure base dir exists → pretty-print JSON → write to a temp + /// file → sync to disk → rename over the real path → sync the directory. /// /// Return: `Err` if the directory can't be created or the write fails. pub fn save(&self) -> std::io::Result<()> { let store = super::store::Store::new(); std::fs::create_dir_all(&store.base_dir)?; let path = store.base_dir.join("settings.json"); + let tmp = store.base_dir.join("settings.json.tmp"); let s = serde_json::to_string_pretty(self)?; - std::fs::write(path, s)?; + std::fs::write(&tmp, s)?; + let f = std::fs::File::open(&tmp)?; + f.sync_all()?; + std::fs::rename(&tmp, path)?; + let _ = std::fs::File::open(&store.base_dir).and_then(|d| d.sync_all()); Ok(()) } } diff --git a/src/tool/git_worktree.rs b/src/tool/git_worktree.rs index 23d3793..e469951 100644 --- a/src/tool/git_worktree.rs +++ b/src/tool/git_worktree.rs @@ -47,6 +47,9 @@ impl Tool for GitWorktree { .and_then(|v| v.as_str()) .ok_or_else(|| anyhow!("missing required argument: name"))? .to_string(); + if name.contains('/') || name.contains('\\') || name.contains("..") { + anyhow::bail!("worktree name must not contain path separators or '..'"); + } let base_ref = args.get("base_ref") .and_then(|v| v.as_str()) .ok_or_else(|| anyhow!("missing required argument: base_ref"))?