fix: autocommit CAS + prune tie-break, bwrap --new-session, cost estimates, tool schema contract

src/autocommit.rs

@@ -26,6 +26,10 @@ pub enum SnapshotOutcome {
     NoChanges,
     /// Auto-commit is disabled (config, non-git dir, etc). Human-readable reason.
     Disabled { reason: String },
+    /// Another writer moved the session's shadow ref while this turn was being
+    /// snapshotted. The turn was NOT recorded, but nothing was destroyed — the
+    /// working tree is untouched and the other instance's history is intact.
+    Conflict { reason: String },
 }
 
 /// Report returned by `restore_to`.
@@ -143,9 +147,19 @@ fn subject_from_prompt(prompt: &str) -> String {
     }
 }
 
-/// Assumes a single writer per `session_id`; concurrent snapshots with the same session id race on the shadow ref and are not supported.
-///
 /// Take a full-tree snapshot of `cwd` as a commit on the session's shadow ref.
+///
+/// **Concurrency.** `parent` comes from this process's in-memory `auto_commits`,
+/// so two instances sharing a `session_id` (a second pane, a resumed session)
+/// each build their own chain and both write the same ref — the later
+/// `update-ref` silently orphans the other's history, which is exactly the
+/// history `/undo` exists to reach.
+///
+/// The ref value is therefore read before the snapshot is built and passed to
+/// `update-ref` as an expected-old value, making the write a compare-and-swap.
+/// Git enforces it atomically under its own ref lock, across processes and
+/// without a lockfile of ours to leak. A concurrent write now fails loudly
+/// ([`SnapshotOutcome::Conflict`]) instead of destroying data quietly.
 pub fn snapshot_turn(
     cwd: &Path,
     config: &AutoCommitConfig,
@@ -166,6 +180,26 @@ pub fn snapshot_turn(
         });
     }
 
+    // 0. Work out where *this process* believes the ref should be, so the final
+    //    update-ref can compare-and-swap against it.
+    //
+    //    Reading the ref here instead would be useless: a competing instance
+    //    that wrote between our turns has already landed, and we would happily
+    //    CAS against its value and clobber it. The meaningful expectation is our
+    //    own chain head — anything else means someone moved the ref since we
+    //    last wrote.
+    //
+    //    The expectation is `auto_commits.last()`, NOT the undo position:
+    //    `restore_to` rewrites the working tree but deliberately leaves the ref
+    //    alone, so after an /undo the ref still points at the newest commit we
+    //    wrote while `undo_position` has moved back. Using the undo position
+    //    here manufactures a conflict on the first turn after any undo.
+    //
+    //    An empty expectation (no commits yet) means the ref must not exist,
+    //    which is exactly right for a fresh session.
+    let ref_name = shadow_ref(session_id);
+    let expected_ref = auto_commits.last().cloned();
+
     // 1. Temp index file, isolated via GIT_INDEX_FILE.
     let td = tempfile::TempDir::new()?;
     let temp_index = td.path().join("turn.index");
@@ -239,13 +273,24 @@ pub fn snapshot_turn(
     }
     let commit_sha = git_output(&mut commit_cmd)?;
 
-    // 7. Update the shadow ref.
-    let ref_name = shadow_ref(session_id);
+    // 7. Update the shadow ref, compare-and-swap against the value we started
+    //    from. An empty expected-old tells git the ref must not exist yet.
+    let expected_old = expected_ref.as_deref().unwrap_or("");
     let update_status = git_cmd(cwd)
-        .args(["update-ref", &ref_name, &commit_sha])
+        .args(["update-ref", &ref_name, &commit_sha, expected_old])
         .status()?;
     if !update_status.success() {
-        anyhow::bail!("git update-ref {ref_name} failed");
+        // The commit object is already written and reachable by sha, so nothing
+        // the user did is lost — we simply refuse to move the ref over someone
+        // else's work.
+        return Ok(SnapshotOutcome::Conflict {
+            reason: format!(
+                "another rustyclaw instance wrote to this session's history \
+                 while this turn was being snapshotted (session '{session_id}'). \
+                 This turn was not recorded; the working tree is untouched. \
+                 Use a distinct session per instance — /undo history is per-session."
+            ),
+        });
     }
 
     // 8. Discard redo tail if user was in an undone state, then append.
@@ -409,11 +454,20 @@ pub fn restore_to(
 /// and the integration test cannot catch it, because `%(committerdate:unix)`
 /// has one-second granularity and sessions created in a loop all tie.
 ///
-/// Ties are resolved by whatever order git returned (refname-alphabetical),
-/// which is *not* recency. See the review tracker: prune ordering is unreliable
-/// for sessions created within the same second.
+/// `%(committerdate:unix)` has one-second granularity, so sessions created in
+/// quick succession tie. Ties previously fell through to git's output order
+/// (refname-alphabetical), which is unrelated to recency — so prune could keep
+/// an older session and delete a newer one purely because of how the ref was
+/// named.
+///
+/// Ties are now broken by refname *descending*. Shadow ref names embed the
+/// session id, and session ids are monotonic within a run, so this is a strictly
+/// better proxy for recency than ascending-alphabetical and is at minimum
+/// deterministic. It is a heuristic, not a guarantee: two sessions genuinely
+/// created in the same second with unordered ids are still arbitrary — but the
+/// arbitrariness is now stable rather than accidental.
 fn select_refs_to_delete(mut rows: Vec<(i64, String)>, keep: usize) -> Vec<String> {
-    rows.sort_by_key(|e| std::cmp::Reverse(e.0));
+    rows.sort_by(|a, b| b.0.cmp(&a.0).then_with(|| b.1.cmp(&a.1)));
     rows.into_iter().skip(keep).map(|(_, r)| r).collect()
 }
 
@@ -524,13 +578,23 @@ mod prune_selection_tests {
         assert_eq!(deleted.len(), 2);
     }
 
-    /// Documents the known weakness rather than asserting a guarantee we do not
-    /// have: with equal timestamps the survivors are decided by git's output
-    /// order, not by recency.
+    /// With one-second timestamp granularity ties are the norm, not the
+    /// exception. They must resolve deterministically rather than by git's
+    /// output order, which is unrelated to recency.
+    #[test]
+    fn tied_timestamps_break_deterministically_by_refname_desc() {
+        // Input order deliberately shuffled — the result must not depend on it.
+        let a = select_refs_to_delete(rows(&[(7, "a"), (7, "c"), (7, "b")]), 1);
+        let b = select_refs_to_delete(rows(&[(7, "c"), (7, "b"), (7, "a")]), 1);
+        assert_eq!(a, b, "tie-break must be independent of input order");
+        assert_eq!(a, vec!["b".to_string(), "a".to_string()]);
+    }
+
+    /// Timestamp still dominates — the tie-break only applies within a second.
     #[test]
-    fn tied_timestamps_fall_back_to_input_order() {
-        let deleted = select_refs_to_delete(rows(&[(7, "a"), (7, "b"), (7, "c")]), 1);
-        assert_eq!(deleted, vec!["b".to_string(), "c".to_string()]);
+    fn timestamp_beats_refname() {
+        let deleted = select_refs_to_delete(rows(&[(1, "zzz"), (9, "aaa")]), 1);
+        assert_eq!(deleted, vec!["zzz".to_string()], "older loses regardless of name");
     }
 }
 

src/cost.rs

@@ -9,6 +9,10 @@ use std::collections::HashMap;
 struct ModelPrice {
     input: f64,
     output: f64,
+    /// True when this is a guess for an unrecognised model rather than a known
+    /// published rate. Surfaced in `/cost` so a wrong number is never presented
+    /// as an authoritative one.
+    estimated: bool,
 }
 
 /// Get pricing for a model.  Returns (input_price, output_price) per million tokens.
@@ -17,50 +21,62 @@ fn model_price(model: &str) -> ModelPrice {
         ModelPrice {
             input: 15.0,
             output: 75.0,
+            estimated: false,
         }
     } else if model.contains("haiku") {
         ModelPrice {
             input: 0.25,
             output: 1.25,
+            estimated: false,
         }
     } else if model.contains("sonnet") {
         ModelPrice {
             input: 3.0,
             output: 15.0,
+            estimated: false,
         }
     } else if model.starts_with("ollama:") {
         // Local models are free
         ModelPrice {
             input: 0.0,
             output: 0.0,
+            estimated: false,
         }
     } else if model.contains("groq:") || model.contains("together:") {
         // Rough estimate for hosted open-source models
         ModelPrice {
             input: 0.5,
             output: 1.0,
+            estimated: false,
         }
     } else if model.contains("deepseek:") {
         ModelPrice {
             input: 0.27,
             output: 1.10,
+            estimated: false,
         }
     } else if model.contains("mistral:") {
         ModelPrice {
             input: 2.0,
             output: 6.0,
+            estimated: false,
         }
     } else if model.contains("oai:") || model.contains("openai:") {
         // GPT-4o class pricing
         ModelPrice {
             input: 2.5,
             output: 10.0,
+            estimated: false,
         }
     } else {
-        // Unknown — assume Sonnet-tier pricing
+        // Unknown model — fall back to Sonnet-tier rates so a budget still
+        // functions, but flag it: an unrecognised model may be an order of
+        // magnitude cheaper or dearer, and silently reporting a guess as fact
+        // is how a /budget cap gets trusted when it should not be.
         ModelPrice {
             input: 3.0,
             output: 15.0,
+            estimated: true,
         }
     }
 }
@@ -74,6 +90,8 @@ pub struct ModelUsage {
     pub output_tokens: u64,
     pub turns: u32,
     pub cost_usd: f64,
+    /// Cost for this model is based on fallback rates, not published ones.
+    pub estimated: bool,
 }
 
 /// Session-wide cost tracker.
@@ -122,6 +140,13 @@ impl CostTracker {
             + (output_tokens as f64 / 1_000_000.0) * price.output;
 
         let entry = self.by_model.entry(model.to_string()).or_default();
+        if price.estimated && !entry.estimated {
+            entry.estimated = true;
+            tracing::warn!(
+                "cost: '{model}' is not a recognised model — pricing it at Sonnet-tier \
+                 rates. Reported cost and any /budget cap are estimates for this model."
+            );
+        }
         entry.input_tokens += input_tokens;
         entry.output_tokens += output_tokens;
         entry.turns += 1;
@@ -178,16 +203,27 @@ impl CostTracker {
             // order costs nothing and removes the failure mode permanently.
             models.sort_by(|a, b| b.1.cost_usd.total_cmp(&a.1.cost_usd));
 
+            let mut any_estimated = false;
             for (model, usage) in models {
                 let short = short_model_name(model);
+                if usage.estimated {
+                    any_estimated = true;
+                }
                 lines.push(format!(
-                    "  {short}: {turns} turns, {in_tok} in / {out_tok} out, ${cost:.4}",
+                    "  {short}: {turns} turns, {in_tok} in / {out_tok} out, {approx}${cost:.4}",
                     turns = usage.turns,
                     in_tok = format_tokens(usage.input_tokens),
                     out_tok = format_tokens(usage.output_tokens),
+                    approx = if usage.estimated { "~" } else { "" },
                     cost = usage.cost_usd,
                 ));
             }
+            if any_estimated {
+                lines.push(String::new());
+                lines.push(
+                    "  ~ estimated — model not recognised, priced at Sonnet-tier rates.".into(),
+                );
+            }
         }
 
         let total_in: u64 = self.by_model.values().map(|u| u.input_tokens).sum();
@@ -306,6 +342,7 @@ mod tests {
                     output_tokens: 1,
                     turns: 1,
                     cost_usd: cost,
+                    estimated: false,
                 },
             );
         }
@@ -315,6 +352,42 @@ mod tests {
         assert!(summary.contains("model-nan"), "every model must still render");
     }
 
+    /// An unrecognised model is priced at Sonnet-tier rates so a budget still
+    /// functions — but reporting that guess as fact is how a /budget cap gets
+    /// trusted when it should not be.
+    #[test]
+    fn unknown_model_cost_is_marked_as_estimated() {
+        let mut t = CostTracker::new();
+        t.record("some-new-provider:mystery-model", 1_000_000, 0);
+        assert!(
+            t.by_model["some-new-provider:mystery-model"].estimated,
+            "unrecognised model must be flagged"
+        );
+        let s = t.summary();
+        assert!(s.contains('~'), "estimate must be marked in the report: {s}");
+        assert!(s.contains("not recognised"), "and explained: {s}");
+    }
+
+    #[test]
+    fn known_models_are_not_marked_as_estimated() {
+        let mut t = CostTracker::new();
+        for m in ["claude-opus-5", "claude-sonnet-4-6", "claude-haiku-4-5", "ollama:llama3"] {
+            t.record(m, 1000, 100);
+            assert!(!t.by_model[m].estimated, "{m} has published rates");
+        }
+        let s = t.summary();
+        assert!(!s.contains("not recognised"), "no footnote expected: {s}");
+    }
+
+    /// Local models are free — pricing them at Sonnet rates would invent spend.
+    #[test]
+    fn ollama_models_are_free_and_not_estimated() {
+        let mut t = CostTracker::new();
+        t.record("ollama:llama3", 5_000_000, 5_000_000);
+        assert_eq!(t.total_cost_usd, 0.0);
+        assert!(!t.by_model["ollama:llama3"].estimated);
+    }
+
     #[test]
     fn test_cost_tracking() {
         let mut tracker = CostTracker::new();

src/sandbox.rs

@@ -152,6 +152,12 @@ pub fn strict_check(cmd: &str) -> Option<String> {
 ///   - Binds /tmp as read-write (tmpfs)
 ///   - Uses --unshare-net to block network (configurable)
 ///   - Uses --unshare-pid for process isolation
+///   - Uses --new-session to detach from the controlling terminal. Without it
+///     the sandboxed process shares our tty and can push characters back into
+///     it with TIOCSTI, which the parent shell then executes as if the user had
+///     typed them — an escape straight out of the sandbox. Modern kernels
+///     default `dev.tty.legacy_tiocsti=0`, but that is a host setting we do not
+///     control, so bwrap's own guard is the right place to rely on.
 ///   - Uses --die-with-parent so cleanup is automatic
 pub fn bwrap_wrap(command: &str, cwd: &std::path::Path, allow_network: bool) -> String {
     let cwd_quoted = shell_quote(&cwd.display().to_string());
@@ -175,6 +181,7 @@ pub fn bwrap_wrap(command: &str, cwd: &std::path::Path, allow_network: bool) ->
          --chdir {cwd} \
          {net_flag}\
          --unshare-pid \
+         --new-session \
          --die-with-parent \
          -- /bin/sh -c {shell_quoted}",
         cwd = cwd_quoted,

src/tools/browser_tools.rs

@@ -514,8 +514,8 @@ impl Tool for BrowseDoneTool {
         json!({
             "type": "object",
             "properties": {
-                "summary":  { "type": "string" },
-                "achieved": { "type": "boolean" }
+                "summary":  { "type": "string", "description": "Short summary of what was found or done during browsing" },
+                "achieved": { "type": "boolean", "description": "True if the browsing goal was accomplished, false if it could not be" }
             },
             "required": ["summary", "achieved"]
         })