Fix major correctness, security, and data-loss issues in core + agentic crates

crates/hi-agent/src/agent/compaction_turn.rs

@@ -87,30 +87,35 @@ impl crate::Agent {
         safety_window: Option<u32>,
         ui: &mut dyn Ui,
     ) -> Result<ContextPreflight> {
-        let Some(window) = self.effective_context_window(safety_window) else {
-            return Ok(ContextPreflight {
-                max_tokens: requested_max_tokens,
-                dropped_prior_context: false,
-            });
-        };
-        if window == 0 {
+        // The *soft* window is the min of the real model window and any
+        // read-only safety window (12k). It only ever triggers non-destructive
+        // tool-output elision — keeping a read-only review turn lean — and must
+        // NOT gate the destructive steps below. The *hard* window is the real
+        // model window alone; only exceeding it may drop prior history or
+        // hard-fail. Otherwise an ordinary read-only question on a 200k-window
+        // model would durably discard the whole session the instant its context
+        // crossed the 12k safety preference.
+        let soft_window = self.effective_context_window(safety_window);
+        let hard_window = self.effective_context_window(None);
+
+        // 1. Already within the soft preference → nothing to do.
+        if let Some(soft) = soft_window
+            && soft > 0
+            && self.request_estimated_tokens(requested_max_tokens, request_overhead_tokens)
+                <= u64::from(soft)
+        {
             return Ok(ContextPreflight {
                 max_tokens: requested_max_tokens,
                 dropped_prior_context: false,
             });
         }
 
-        let mut dropped_prior_context = false;
-        if self.request_estimated_tokens(requested_max_tokens, request_overhead_tokens)
-            <= u64::from(window)
+        // 2. Over the soft preference: try non-destructive elision (no model
+        //    call — see note below), and if that brings us under, we're done.
+        if self.config.auto_compact
+            && let Some(soft) = soft_window
+            && soft > 0
         {
-            return Ok(ContextPreflight {
-                max_tokens: requested_max_tokens,
-                dropped_prior_context,
-            });
-        }
-
-        if self.config.auto_compact {
             let freed = compaction::elide_tool_outputs_except_recent(
                 self.messages.mutate_slice(),
                 self.config.in_turn_keep_tool_results,
@@ -123,11 +128,11 @@ impl crate::Agent {
                 ));
             }
             if self.request_estimated_tokens(requested_max_tokens, request_overhead_tokens)
-                <= u64::from(window)
+                <= u64::from(soft)
             {
                 return Ok(ContextPreflight {
                     max_tokens: requested_max_tokens,
-                    dropped_prior_context,
+                    dropped_prior_context: false,
                 });
             }
 
@@ -139,6 +144,25 @@ impl crate::Agent {
             // bulk or drop prior context while preserving the latest prompt.
         }
 
+        // 3. Destructive recovery is gated on the REAL model window only. With
+        //    no configured window we can't tell — so proceed rather than drop.
+        let Some(window) = hard_window else {
+            return Ok(ContextPreflight {
+                max_tokens: requested_max_tokens,
+                dropped_prior_context: false,
+            });
+        };
+        if window == 0
+            || self.request_estimated_tokens(requested_max_tokens, request_overhead_tokens)
+                <= u64::from(window)
+        {
+            return Ok(ContextPreflight {
+                max_tokens: requested_max_tokens,
+                dropped_prior_context: false,
+            });
+        }
+
+        let mut dropped_prior_context = false;
         if turn_start > 1 {
             self.replace_history_with_compaction(vec![self.system_message()])?;
             self.messages.push_user(format!(
@@ -428,7 +452,9 @@ impl crate::Agent {
         let completion = match self.provider.stream(request, &mut sink).await {
             Ok(completion) => completion,
             Err(err) => {
-                self.add_error_usage(&err);
+                // Summarize is a side call — don't let its request size clobber
+                // the main conversation's `context_used` gauge.
+                self.add_side_error_usage(&err);
                 self.emit_usage(ui);
                 // Flush any partially-streamed summary text before returning.
                 ui.assistant_end();

crates/hi-agent/src/agent/curate_turn.rs

@@ -91,7 +91,7 @@ impl crate::Agent {
         let completion = match self.provider.stream(request, &mut sink).await {
             Ok(completion) => completion,
             Err(err) => {
-                self.add_error_usage(&err);
+                self.add_side_error_usage(&err);
                 ui.status(&format!("(couldn't curate skill: {err})"));
                 return;
             }

crates/hi-agent/src/agent/lifecycle.rs

@@ -485,6 +485,18 @@ impl crate::Agent {
         self.add_usage(provider_error_usage(err));
     }
 
+    /// Like [`add_error_usage`] but for a *side* model call (skeptic, curate,
+    /// memory, goal planning, finalize, summarize). Books the error's usage
+    /// toward totals/turn spend without touching `context_used` — routing a
+    /// small side request's input size through `add_usage` would reset the main
+    /// conversation's occupancy gauge and silently disable the next
+    /// auto-compaction (see [`add_side_usage`]). Providers do attach nonzero
+    /// input usage to some errors (e.g. EmptyCompletion/MalformedStream), so
+    /// this matters in practice, not just in theory.
+    pub(crate) fn add_side_error_usage(&mut self, err: &anyhow::Error) {
+        self.add_side_usage(provider_error_usage(err));
+    }
+
     pub(crate) fn emit_usage(&self, ui: &mut dyn Ui) {
         ui.usage(
             self.last_user_prompt_tokens,
@@ -847,7 +859,14 @@ impl crate::Agent {
 
     pub(crate) fn persist(&mut self) -> Result<()> {
         if let Some(session) = self.session.as_mut() {
-            session.record(&self.messages.as_slice()[self.persisted..], self.totals)?;
+            // Clamp the cursor: transcript-shrinking ops (`strip_trailing_nudges`,
+            // `strip_finalize_pair`) pop messages without adjusting `persisted`,
+            // so after a mid-turn persist that already recorded up to a
+            // now-popped message, `persisted` can exceed the current length.
+            // Slicing `[persisted..]` would then panic; clamp so we simply record
+            // nothing new instead of crashing the session.
+            let start = self.persisted.min(self.messages.len());
+            session.record(&self.messages.as_slice()[start..], self.totals)?;
             self.persisted = self.messages.len();
         }
         Ok(())

crates/hi-agent/src/agent/memory_turn.rs

@@ -107,7 +107,7 @@ impl crate::Agent {
         let completion = match self.provider.stream(request, &mut sink).await {
             Ok(completion) => completion,
             Err(err) => {
-                self.add_error_usage(&err);
+                self.add_side_error_usage(&err);
                 // Flush any partially-streamed memory text before the status.
                 ui.assistant_end();
                 let _ = self.persist();

crates/hi-agent/src/agent/plan_goal.rs

@@ -61,7 +61,7 @@ impl crate::Agent {
         let completion = match self.provider.stream(request, &mut sink).await {
             Ok(completion) => completion,
             Err(err) => {
-                self.add_error_usage(&err);
+                self.add_side_error_usage(&err);
                 return Err(err);
             }
         };

crates/hi-agent/src/agent/skeptic.rs

@@ -153,7 +153,7 @@ impl crate::Agent {
         let completion = match self.provider.stream(request, &mut sink).await {
             Ok(completion) => completion,
             Err(err) => {
-                self.add_error_usage(&err);
+                self.add_side_error_usage(&err);
                 return SkepticVerdict::Approve; // fail-open on a provider error
             }
         };

crates/hi-agent/src/agent/turn.rs

@@ -17,9 +17,9 @@ use crate::command;
 use crate::compaction;
 use crate::heuristics::{
     RECOVERY_SAMPLING, StallMode, emit_tool_output, humanize_count, looks_like_continue,
-    looks_like_unfinished_step, looks_mutating, parse_text_tool_calls, plan_has_pending_steps,
-    recovery_sampling, recovery_telemetry, respects_deps, textcall_id_offset, tool_deps,
-    tool_mode_label,
+    looks_like_unfinished_step, looks_mutating, mode_blocks_tool, parse_text_tool_calls,
+    plan_has_pending_steps, recovery_sampling, recovery_telemetry, respects_deps,
+    textcall_id_offset, tool_deps, tool_mode_label,
 };
 use crate::snapshot::changed_files_between;
 use crate::steering::{
@@ -613,7 +613,18 @@ impl crate::Agent {
         let input = turn_input.as_str();
         self.reset_last_turn_usage(user_prompt_tokens);
 
-        if read_only_intent.is_none() && self.tools_unavailable_for(input) {
+        // A top-level session the user restricted to ChatOnly/ReadOnly gets a
+        // clear early "your mode blocks edits" error when the prompt clearly asks
+        // for mutation. This must NOT fire for a subagent: an `explore` child
+        // runs ReadOnly as internal capability-scoping (not a user restriction),
+        // and its task text naturally contains verbs like "find where X creates
+        // Y" — pattern-matching that as a mutating request would abort the child
+        // before its first model call and return "(no answer)". The child simply
+        // isn't advertised mutating tools, so it's safe to let it run and answer.
+        if read_only_intent.is_none()
+            && !self.config.is_subagent
+            && self.tools_unavailable_for(input)
+        {
             self.last_verify = None;
             self.last_changed_files.clear();
             self.last_compat_fallbacks.clear();
@@ -2393,9 +2404,20 @@ If the task is already complete, stop and give your final recap."
                 // tree — so they now dispatch inside the dep-aware scheduler
                 // loop below.)
                 for (i, (id, name, arguments)) in calls.iter().enumerate() {
-                    if read_only_blocks_tool(read_only_intent, name) {
+                    // Block calls forbidden by the review intent (read-only
+                    // prompt) OR the session tool_mode. The tool_mode check is
+                    // essential for the text-promoted tool-call path above: a
+                    // local model can emit `{"name":"write",…}` as prose, which
+                    // bypasses tool *advertisement*, so without an execution-time
+                    // guard a ChatOnly/ReadOnly session — every `explore` subagent
+                    // included — could still run a mutating `write`/`bash`.
+                    let blocked = if read_only_blocks_tool(read_only_intent, name) {
+                        Some(read_only_blocked_tool_result(name))
+                    } else {
+                        mode_blocks_tool(self.config.tool_mode, name)
+                    };
+                    if let Some(content) = blocked {
                         ui.tool_call(name, arguments);
-                        let content = read_only_blocked_tool_result(name);
                         emit_tool_output(
                             &mut *ui,
                             name,
@@ -3215,7 +3237,9 @@ If the task is already complete, stop and give your final recap."
         let completion = match self.provider.stream(request, &mut sink).await {
             Ok(completion) => completion,
             Err(err) => {
-                self.add_error_usage(&err);
+                // Finalize is a side call — book its error usage without resetting
+                // the main conversation's `context_used` gauge.
+                self.add_side_error_usage(&err);
                 self.emit_usage(ui);
                 // Flush any partially-streamed recap text before the status
                 // line, so it isn't left dangling in the UI's pending buffer.

crates/hi-agent/src/heuristics.rs

@@ -312,7 +312,11 @@ pub(crate) fn emit_tool_output(ui: &mut dyn Ui, name: &str, output: &ToolOutput)
 /// - A mutating call (`write`/`edit`/`multi_edit`/`bash`/`apply_patch`) depends
 ///   on every earlier mutating call, so side effects apply in emission order.
 ///   (Two independent writes still serialize — file edits aren't commutative
-///   and a later write may depend on an earlier write's content.)
+///   and a later write may depend on an earlier write's content.) It also
+///   depends on any earlier *read* of the same path (write-after-read): "read
+///   a.rs, then write a.rs" must let the read observe the pre-write content, not
+///   a file being truncated/rewritten under it. A mutation with an unknown write
+///   path (`bash`) conservatively waits for every earlier read.
 /// - A read-only call depends on any earlier mutating call whose inferred
 ///   target path matches the read's target path — so "write a.rs, then read
 ///   a.rs" reads the post-write state even if a scheduler reorders independent
@@ -333,8 +337,12 @@ pub(crate) fn tool_deps(calls: &[(String, String, String)]) -> Vec<Vec<usize>> {
         let my_path = hi_tools::target_path(name, arguments);
         for (j, (was_mut, their_path)) in prior.iter().enumerate() {
             let must_wait = if mutating {
-                // Mutating calls serialize after all earlier mutations.
-                *was_mut
+                // Serialize after all earlier mutations (was_mut), and after an
+                // earlier read of the same path (write-after-read: the read must
+                // see the pre-write file). A mutation with an unknown write path
+                // (bash) conservatively waits for every earlier read, since
+                // paths_overlap treats an unknown path as overlapping.
+                *was_mut || paths_overlap(their_path.as_deref(), my_path.as_deref())
             } else {
                 // Reads wait for an earlier mutation on the same path. If
                 // either side has no parseable path, be safe and serialize
@@ -418,6 +426,31 @@ pub(crate) fn tool_mode_label(mode: ToolMode) -> &'static str {
     }
 }
 
+/// Whether the session `tool_mode` forbids *executing* `name`, returning the
+/// synthetic blocked-tool result to feed back to the model if so.
+///
+/// This enforces the mode at execution time, not just via tool advertisement —
+/// which is what closes the text-promoted tool-call hole: a local model can emit
+/// a tool call as prose (`{"name":"write",…}`) that never went through the
+/// advertised tool list, so a ChatOnly/ReadOnly session (including every
+/// `explore` subagent) would otherwise run it. `explore` launches only a
+/// read-only child, so it's allowed under ReadOnly (mirroring the advertisement
+/// rules and [`crate::steering::nudges::read_only_blocks_tool`]).
+pub(crate) fn mode_blocks_tool(mode: ToolMode, name: &str) -> Option<String> {
+    match mode {
+        ToolMode::Auto | ToolMode::Required => None,
+        ToolMode::ChatOnly => Some(format!(
+            "Tool `{name}` blocked: this is a discuss-only turn (tool mode chat-only). \
+             Answer in text without calling tools."
+        )),
+        ToolMode::ReadOnly if !hi_tools::is_read_only(name) && name != "explore" => Some(format!(
+            "Tool `{name}` blocked: this session is read-only (tool mode read-only). \
+             Use read-only inspection tools and do not modify files."
+        )),
+        ToolMode::ReadOnly => None,
+    }
+}
+
 pub(crate) fn looks_mutating(input: &str) -> bool {
     let s = input.to_ascii_lowercase();
     [
@@ -695,6 +728,55 @@ pub(crate) fn recovery_telemetry(
 mod tests {
     use super::*;
 
+    #[test]
+    fn tool_deps_serializes_write_after_read_on_same_path() {
+        // read a.rs then write a.rs: the write must wait for the read so the read
+        // observes the pre-write file (previously they ran concurrently, so the
+        // read could see a torn/post-write file).
+        let calls = vec![
+            ("r".into(), "read".into(), r#"{"path":"a.rs"}"#.into()),
+            (
+                "w".into(),
+                "write".into(),
+                r#"{"path":"a.rs","content":"x"}"#.into(),
+            ),
+        ];
+        let deps = tool_deps(&calls);
+        assert!(deps[0].is_empty(), "the read has no deps");
+        assert_eq!(deps[1], vec![0], "the write waits for the same-path read");
+
+        // read a.rs then write b.rs: different files, still independent.
+        let calls = vec![
+            ("r".into(), "read".into(), r#"{"path":"a.rs"}"#.into()),
+            (
+                "w".into(),
+                "write".into(),
+                r#"{"path":"b.rs","content":"x"}"#.into(),
+            ),
+        ];
+        let deps = tool_deps(&calls);
+        assert!(
+            deps[1].is_empty(),
+            "a write to a different file is independent of the read"
+        );
+    }
+
+    #[test]
+    fn mode_blocks_tool_enforces_session_mode() {
+        // ChatOnly blocks every tool (nothing runs, not even reads).
+        assert!(mode_blocks_tool(ToolMode::ChatOnly, "read").is_some());
+        assert!(mode_blocks_tool(ToolMode::ChatOnly, "write").is_some());
+        // ReadOnly blocks mutating tools but allows inspection + `explore`.
+        assert!(mode_blocks_tool(ToolMode::ReadOnly, "write").is_some());
+        assert!(mode_blocks_tool(ToolMode::ReadOnly, "bash").is_some());
+        assert!(mode_blocks_tool(ToolMode::ReadOnly, "read").is_none());
+        assert!(mode_blocks_tool(ToolMode::ReadOnly, "grep").is_none());
+        assert!(mode_blocks_tool(ToolMode::ReadOnly, "explore").is_none());
+        // Auto/Required never block by mode.
+        assert!(mode_blocks_tool(ToolMode::Auto, "write").is_none());
+        assert!(mode_blocks_tool(ToolMode::Required, "bash").is_none());
+    }
+
     #[test]
     fn humanize_count_abbreviates_consistently() {
         assert_eq!(humanize_count(0), "0");

crates/hi-agent/src/memory.rs

@@ -554,13 +554,17 @@ pub(crate) fn write_memory(path: &Path, body: &str) -> Result<usize, String> {
 
     // Atomic publish: temp file + rename. fs::rename is atomic on POSIX when
     // source and destination are on the same filesystem (they are — both in
-    // the memory file's parent dir).
+    // the memory file's parent dir). The temp name is PID-scoped (mirroring
+    // `write_skill`): even if two processes ever hold the lock at once (the
+    // stale-lock break is racy), distinct temp files mean each rename installs a
+    // *complete* file — last-writer-wins, never the torn/empty result a shared
+    // `memory.md.tmp` produced when one writer truncated it mid-rename.
     let mut tmp = path.to_path_buf();
     let mut name = tmp
         .file_name()
         .map(|n| n.to_os_string())
         .unwrap_or_else(|| std::ffi::OsString::from("memory.md"));
-    name.push(".tmp");
+    name.push(format!(".{}.tmp", std::process::id()));
     tmp.set_file_name(name);
 
     let content = format!("{MEMORY_HEADER}\n{body}\n");