From e2ab35af3cb2f0e3cc9126e12bee6247183dce27 Mon Sep 17 00:00:00 2001 From: Yeti Paw <22755327+ForkedInTime@users.noreply.github.com> Date: Sun, 2 Aug 2026 18:12:43 -0700 Subject: [PATCH] fix: autocommit CAS + prune tie-break, bwrap --new-session, cost estimates, tool schema contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four tracker items, each with a regression test verified to fail against the reintroduced bug. ## autocommit: concurrent writes silently destroyed history `parent` comes from this process's in-memory `auto_commits`, so two instances sharing a session id (second pane, resumed session) each built their own commit chain and both wrote the same shadow ref. The later `update-ref` orphaned the other's history — exactly the history /undo exists to reach, gone with no error. `update-ref` now carries an expected-old value, making the write a compare-and-swap that git enforces atomically under its own ref lock. No lockfile of ours to leak, and it works across processes and machines. A conflict returns the new `SnapshotOutcome::Conflict` and is surfaced in the TUI rather than only logged — the turn is missing from /undo history, so the user has to know. Two corrections while building this, both from checking assumptions rather than trusting them: - Reading the ref at the start of the call is useless: a competing writer that landed between turns has already committed, and we would cheerfully CAS against its value. The meaningful expectation is our own chain head. - That chain head 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. Using the undo position manufactured a conflict on the first turn after any undo — caught by the existing redo test. ## autocommit: prune could delete the newer of two sessions `%(committerdate:unix)` has one-second granularity, so sessions created in quick succession tie, and ties fell through to git's output order (refname-alphabetical) — unrelated to recency. Ties now break on refname descending: session ids are monotonic within a run, so it is a better proxy and is at minimum deterministic. Documented as a heuristic, not a guarantee. ## bwrap: --new-session (P3) Without it the sandboxed process shares our controlling terminal and can push characters back into it with TIOCSTI, which the parent shell then executes as if typed — 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. ## cost: unknown models were priced as fact (P4) An unrecognised model silently fell back to Sonnet-tier rates, so /cost and any /budget cap reported a guess as a published number — and an unknown model may be an order of magnitude cheaper or dearer. The fallback stays (a budget should still function) but is now flagged: the model is warned about once, per-model rows are marked `~`, and the summary carries a footnote. ## tools: standing schema contract (P5) Swept all 43 registered tools for schema/implementation drift. The only static hit was a false positive — `Grep`'s ripgrep-style flags (`-A`, `-i`, …) connect through `#[serde(rename)]`, which the scanner did not model. Rather than leave that as a one-off, the sweep is now a test over the **full** registry (`all_tools`, 44 tools — `default_tools` is a subset and would have hidden this): names unique and non-empty, descriptions present, schemas type=object, every property typed and described, everything in `required` actually declared. It immediately found 5 real defects that the static pass missed — `TaskUpdate.status`, `TaskUpdate.task_id`, `TaskStop.task_id`, `browse_done.achieved`, `browse_done.summary` all shipped with no description, leaving the model to guess their meaning. Fixed. QA: 575 tests pass, 0 failures. Clippy clean under the CI gate. Release builds at 19.05 MB. Each of the four fixes verified by reintroducing its bug: dropping the CAS, removing the tie-break, un-flagging the estimate, and deleting a param description each fail their test. Co-Authored-By: Arch Linux --- src/autocommit.rs | 96 ++++++++++++++++++++++++++------ src/cost.rs | 77 +++++++++++++++++++++++++- src/sandbox.rs | 7 +++ src/tools/browser_tools.rs | 4 +- src/tools/mod.rs | 97 +++++++++++++++++++++++++++++++++ src/tools/tasks.rs | 6 +- src/tui/run.rs | 9 +++ tests/autocommit_integration.rs | 94 ++++++++++++++++++++++++++++++++ 8 files changed, 367 insertions(+), 23 deletions(-) diff --git a/src/autocommit.rs b/src/autocommit.rs index 9ac14ae..e647a20 100644 --- a/src/autocommit.rs +++ b/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 { - 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"); } } diff --git a/src/cost.rs b/src/cost.rs index 58a5458..2fce771 100644 --- a/src/cost.rs +++ b/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(); diff --git a/src/sandbox.rs b/src/sandbox.rs index 37f5711..7539e37 100644 --- a/src/sandbox.rs +++ b/src/sandbox.rs @@ -152,6 +152,12 @@ pub fn strict_check(cmd: &str) -> Option { /// - 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, diff --git a/src/tools/browser_tools.rs b/src/tools/browser_tools.rs index 424a867..74028eb 100644 --- a/src/tools/browser_tools.rs +++ b/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"] }) diff --git a/src/tools/mod.rs b/src/tools/mod.rs index a230766..9cd898d 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -807,3 +807,100 @@ mod sensitive_path_tests { ); } } + +#[cfg(test)] +mod schema_contract_tests { + use super::*; + + /// Every tool's advertised schema must be internally consistent. + /// + /// This is the standing form of a one-off sweep: a tool that advertises a + /// parameter it does not implement (or requires one it never declares) + /// silently misleads the model, which then sends inputs that are ignored. + /// One such bug — `run_in_background` on the Agent tool — was found and + /// removed in a previous audit; this stops the class returning rather than + /// catching them one at a time. + #[test] + fn every_tool_schema_is_self_consistent() { + // The full registry, not `default_tools()` — that is a subset, and a + // schema bug in a tool only reachable via the full set is exactly the + // kind this is meant to catch. + let cfg = crate::config::Config::default(); + let tools = all_tools(&cfg); + assert!( + tools.len() > 30, + "expected the full registry, got {} tools", + tools.len() + ); + + let mut problems: Vec = Vec::new(); + let mut seen: Vec = Vec::new(); + + for t in &tools { + let name = t.name().to_string(); + + if name.trim().is_empty() { + problems.push("a tool has an empty name".into()); + } + if seen.contains(&name) { + problems.push(format!("{name}: duplicate tool name in the registry")); + } + seen.push(name.clone()); + + if t.description().trim().is_empty() { + problems.push(format!("{name}: empty description — the model selects on this")); + } + + let schema = t.input_schema(); + if schema.get("type").and_then(|v| v.as_str()) != Some("object") { + problems.push(format!("{name}: input_schema must be type=object")); + continue; + } + + let props = match schema.get("properties").and_then(|v| v.as_object()) { + Some(p) => p, + None => { + // A tool taking no input is legitimate, but then it must not + // declare anything required either. + if schema.get("required").is_some() { + problems.push(format!("{name}: has `required` but no `properties`")); + } + continue; + } + }; + + for (prop, def) in props { + if def.get("type").is_none() && def.get("enum").is_none() { + problems.push(format!("{name}.{prop}: property has neither `type` nor `enum`")); + } + if def.get("description").and_then(|d| d.as_str()).is_none_or(str::is_empty) { + problems.push(format!( + "{name}.{prop}: no description — the model has to guess what it means" + )); + } + } + + // Anything required must actually be advertised. + if let Some(req) = schema.get("required").and_then(|v| v.as_array()) { + for r in req { + let Some(r) = r.as_str() else { + problems.push(format!("{name}: non-string entry in `required`")); + continue; + }; + if !props.contains_key(r) { + problems.push(format!( + "{name}: `{r}` is required but never declared in properties" + )); + } + } + } + } + + assert!( + problems.is_empty(), + "tool schema contract violations ({}):\n {}", + problems.len(), + problems.join("\n ") + ); + } +} diff --git a/src/tools/tasks.rs b/src/tools/tasks.rs index 89f1110..2a3d2f8 100644 --- a/src/tools/tasks.rs +++ b/src/tools/tasks.rs @@ -197,8 +197,8 @@ impl Tool for TaskUpdateTool { json!({ "type": "object", "properties": { - "task_id": { "type": "string" }, - "status": { "type": "string", "enum": ["pending","in_progress","completed","failed","stopped"] }, + "task_id": { "type": "string", "description": "The task id returned by TaskCreate" }, + "status": { "type": "string", "enum": ["pending","in_progress","completed","failed","stopped"], "description": "New status for the task" }, "output": { "type": "string", "description": "Result or progress message" } }, "required": ["task_id"] @@ -254,7 +254,7 @@ impl Tool for TaskStopTool { json!({ "type": "object", "properties": { - "task_id": { "type": "string" } + "task_id": { "type": "string", "description": "The task id returned by TaskCreate" } }, "required": ["task_id"] }) diff --git a/src/tui/run.rs b/src/tui/run.rs index 9b069e6..176ed30 100644 --- a/src/tui/run.rs +++ b/src/tui/run.rs @@ -1097,6 +1097,15 @@ async fn run_loop(mut config: Config, resume_id: Option) -> Result<()> { Ok(rustyclaw::autocommit::SnapshotOutcome::Disabled { reason }) => { tracing::debug!("autoCommit: disabled ({reason})"); } + Ok(rustyclaw::autocommit::SnapshotOutcome::Conflict { reason }) => { + // Must be visible, not just logged: this turn is absent + // from the undo history, so /undo will silently skip it + // if the user is never told. + tracing::warn!("autoCommit: {reason}"); + app.entries.push(crate::tui::app::ChatEntry::error( + format!("⚠ Auto-commit conflict — this turn was not added to /undo history.\n{reason}"), + )); + } Err(e) => { tracing::warn!("autoCommit: snapshot failed: {e}"); } diff --git a/tests/autocommit_integration.rs b/tests/autocommit_integration.rs index 1d38ca7..c46edb6 100644 --- a/tests/autocommit_integration.rs +++ b/tests/autocommit_integration.rs @@ -213,3 +213,97 @@ fn prune_integration_15_refs_keeps_10() { let remaining = String::from_utf8(out.stdout).unwrap(); assert_eq!(remaining.lines().count(), 10); } + +/// Two rustyclaw instances sharing a session id each build their own commit +/// chain in memory and both write the same shadow ref. Before compare-and-swap +/// the later `update-ref` silently orphaned the other's history — losing exactly +/// the turns `/undo` exists to reach. +/// +/// Simulated by moving the ref out from under an in-flight session, which is +/// what a concurrent writer looks like from this process's point of view. +#[test] +fn concurrent_ref_write_is_detected_not_clobbered() { + let td = TempDir::new().unwrap(); + git_init(td.path()); + commit_initial(td.path()); + + let cfg = AutoCommitConfig::default(); + let mut commits: Vec = Vec::new(); + let mut pos = 0usize; + + // Our instance records one turn. + write(td.path(), "a.txt", "one\n"); + snapshot_turn(td.path(), &cfg, "shared", "t1", 1, &mut commits, &mut pos).unwrap(); + let ours = commits.last().cloned().expect("first turn committed"); + + // Another instance writes the same ref behind our back. + let refname = format!("{SHADOW_REF_PREFIX}shared"); + let head = Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(td.path()) + .output() + .unwrap(); + let other = String::from_utf8(head.stdout).unwrap().trim().to_string(); + assert!( + Command::new("git") + .args(["update-ref", &refname, &other]) + .current_dir(td.path()) + .status() + .unwrap() + .success() + ); + + // Our next turn must refuse rather than overwrite the other writer. + write(td.path(), "a.txt", "two\n"); + let outcome = + snapshot_turn(td.path(), &cfg, "shared", "t2", 2, &mut commits, &mut pos).unwrap(); + assert!( + matches!(outcome, SnapshotOutcome::Conflict { .. }), + "expected Conflict, got {outcome:?}" + ); + + // The other writer's value survived — we did not clobber it. + let now = Command::new("git") + .args(["rev-parse", &refname]) + .current_dir(td.path()) + .output() + .unwrap(); + let now = String::from_utf8(now.stdout).unwrap().trim().to_string(); + assert_eq!(now, other, "the concurrent writer's ref must be intact"); + assert_ne!(now, ours); +} + +/// A conflict must not damage the working tree — the user's files are theirs, +/// and a bookkeeping failure is not a reason to touch them. +#[test] +fn conflict_leaves_the_working_tree_untouched() { + let td = TempDir::new().unwrap(); + git_init(td.path()); + commit_initial(td.path()); + + let cfg = AutoCommitConfig::default(); + let mut commits: Vec = Vec::new(); + let mut pos = 0usize; + + write(td.path(), "w.txt", "v1\n"); + snapshot_turn(td.path(), &cfg, "s", "t1", 1, &mut commits, &mut pos).unwrap(); + + let refname = format!("{SHADOW_REF_PREFIX}s"); + let head = Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(td.path()) + .output() + .unwrap(); + let other = String::from_utf8(head.stdout).unwrap().trim().to_string(); + Command::new("git") + .args(["update-ref", &refname, &other]) + .current_dir(td.path()) + .status() + .unwrap(); + + write(td.path(), "w.txt", "v2-user-edit\n"); + let _ = snapshot_turn(td.path(), &cfg, "s", "t2", 2, &mut commits, &mut pos).unwrap(); + + let on_disk = std::fs::read_to_string(td.path().join("w.txt")).unwrap(); + assert_eq!(on_disk, "v2-user-edit\n", "working tree must be untouched"); +}