diff --git a/crates/trodden-capture/src/builder.rs b/crates/trodden-capture/src/builder.rs index e361d50..5e46188 100644 --- a/crates/trodden-capture/src/builder.rs +++ b/crates/trodden-capture/src/builder.rs @@ -250,6 +250,7 @@ impl<'a> TraceBuilder<'a> { changes: Vec::new(), duration_ms: None, error: None, + confirmed_by_output: false, })); if let Some(id) = id { self.pending.insert(id.to_owned(), index); @@ -298,6 +299,7 @@ impl<'a> TraceBuilder<'a> { (Finish::Succeeded, Some(false)) => ToolOutcome::Failed { exit_code: None }, (Finish::Succeeded, _) => ToolOutcome::Succeeded, }; + call.confirmed_by_output = verdict.is_some() && call.outcome != ToolOutcome::Interrupted; if matches!(call.outcome, ToolOutcome::Failed { .. }) { call.error = ErrorSignature::of(output, redactor); } diff --git a/crates/trodden-capture/src/journal.rs b/crates/trodden-capture/src/journal.rs index 6623776..a0b1e41 100644 --- a/crates/trodden-capture/src/journal.rs +++ b/crates/trodden-capture/src/journal.rs @@ -47,6 +47,8 @@ pub enum Observed { duration_ms: Option, #[serde(default, skip_serializing_if = "Vec::is_empty")] changes: Vec, + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + confirmed_by_output: bool, }, Compaction { automatic: bool, @@ -99,6 +101,7 @@ impl Observer<'_> { error: None, duration_ms: None, changes: Vec::new(), + confirmed_by_output: false, } } @@ -136,6 +139,7 @@ impl Observer<'_> { error, duration_ms: ran.duration_ms, changes: Vec::new(), + confirmed_by_output: verdict.is_some() && outcome != ToolOutcome::Interrupted, }) } @@ -173,6 +177,7 @@ impl Observer<'_> { error: None, duration_ms: None, changes, + confirmed_by_output: false, }) } @@ -251,6 +256,7 @@ impl Journal { error, duration_ms, changes, + confirmed_by_output, } => { let call = Self::relative( &builder, @@ -262,6 +268,7 @@ impl Journal { changes, duration_ms, error, + confirmed_by_output, }, ); builder.emit(EventKind::ToolCall(call)); @@ -420,6 +427,44 @@ mod tests { assert!(call.error.is_some()); } + #[test] + fn masked_checks_record_when_their_output_decided() { + let cases = [ + ( + "cargo test 2>&1 | tail -3", + "test result: ok. 3 passed", + true, + ), + ( + "cargo test 2>&1 | tail -3", + "test result: FAILED. 1 failed", + true, + ), + ("cargo test 2>&1 | tail -3", "running 3 tests", false), + ("cargo test", "test result: ok. 3 passed", false), + ]; + for (command, output, confirmed) in cases { + let mut session = Session::new(); + session.observe("/home/dev/shop", "2026-10-09T10:00:09Z", |o| { + o.command( + "Shell", + command, + Ran { + output, + exit_code: Some(0), + failed: false, + interrupted: false, + duration_ms: None, + }, + ) + }); + + let call = session.calls().remove(0); + + assert_eq!(call.confirmed_by_output, confirmed, "{command}: {output}"); + } + } + #[test] fn commands_from_a_subdirectory_replay_from_the_first_one() { let mut session = Session::new(); diff --git a/crates/trodden-core/src/trace.rs b/crates/trodden-core/src/trace.rs index d96339e..c3340f1 100644 --- a/crates/trodden-core/src/trace.rs +++ b/crates/trodden-core/src/trace.rs @@ -58,6 +58,8 @@ pub struct ToolCall { pub duration_ms: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub error: Option, + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub confirmed_by_output: bool, } #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, JsonSchema)] diff --git a/crates/trodden-extract/src/lib.rs b/crates/trodden-extract/src/lib.rs index 816ec3f..9fe4e6b 100644 --- a/crates/trodden-extract/src/lib.rs +++ b/crates/trodden-extract/src/lib.rs @@ -90,6 +90,7 @@ pub enum Rejection { NoEdits, NotVerified, VerificationFailed { command: String }, + StatusHidden { command: String }, TooManySteps { steps: usize }, ContainsSecret, Dangerous { rule: &'static str, command: String }, @@ -102,6 +103,9 @@ impl fmt::Display for Rejection { Self::NoEdits => f.write_str("no files were changed"), Self::NotVerified => f.write_str("no check ran after the last edit"), Self::VerificationFailed { command } => write!(f, "the last check failed: `{command}`"), + Self::StatusHidden { command } => { + write!(f, "the last check hid its exit status: `{command}`") + } Self::TooManySteps { steps } => write!(f, "{steps} steps is more than {MAX_STEPS}"), Self::ContainsSecret => f.write_str("a step depends on a redacted secret"), Self::Dangerous { rule, command } => write!(f, "{rule}: `{command}`"), @@ -165,6 +169,12 @@ impl<'a> Call<'a> { self.call.outcome == ToolOutcome::Succeeded } + fn hides_status(&self) -> bool { + self.succeeded() + && !self.call.confirmed_by_output + && self.command().is_some_and(Verification::hides_status) + } + fn is_edit(&self) -> bool { self.call.action == ToolAction::Edit && self.succeeded() && !self.call.changes.is_empty() } @@ -215,10 +225,13 @@ impl<'a> Builder<'a> { } fn outcome(&self) -> TaskOutcome { - match self.calls.iter().rposition(Call::is_edit) { - None => TaskOutcome::Unjudged, - Some(last_edit) if self.final_verification(last_edit).is_ok() => TaskOutcome::Succeeded, - Some(_) => TaskOutcome::Failed, + let Some(last_edit) = self.calls.iter().rposition(Call::is_edit) else { + return TaskOutcome::Unjudged; + }; + match self.final_verification(last_edit) { + Ok(_) => TaskOutcome::Succeeded, + Err(Rejection::StatusHidden { .. }) => TaskOutcome::Unjudged, + Err(_) => TaskOutcome::Failed, } } @@ -268,20 +281,26 @@ impl<'a> Builder<'a> { fn final_verification(&self, last_edit: usize) -> Result { let learned = self.learned_checks(last_edit); - let is_check = |call: &Call<'_>| { + let runs_check = |call: &Call<'_>| { call.command().is_some_and(|command| { Verification::is_verify(command) || learned.contains(&command) || self.checks.find(command).is_some() }) }; + let is_check = |call: &Call<'_>| runs_check(call) && !call.hides_status(); let failure = |call: &Call<'_>| Rejection::VerificationFailed { command: Verification::clean(call.command().unwrap_or_default()), }; let after_edit = || self.calls.iter().enumerate().skip(last_edit + 1); - let (index, call) = after_edit() - .rfind(|(_, call)| is_check(call)) - .ok_or(Rejection::NotVerified)?; + let Some((index, call)) = after_edit().rfind(|(_, call)| is_check(call)) else { + return Err(after_edit().rfind(|(_, call)| runs_check(call)).map_or( + Rejection::NotVerified, + |(_, call)| Rejection::StatusHidden { + command: call.command().unwrap_or_default().to_owned(), + }, + )); + }; if !call.succeeded() { return Err(failure(call)); } @@ -488,6 +507,7 @@ impl<'a> Builder<'a> { .take_while(|call| not_found || !call.is_edit()) .find(|call| { call.succeeded() + && !call.hides_status() && call.command().is_some_and(|other| { other != command && Self::lint(other).is_ok() @@ -953,6 +973,7 @@ mod tests { changes: Vec::new(), duration_ms: None, error: None, + confirmed_by_output: false, })) } @@ -974,6 +995,7 @@ mod tests { }], duration_ms: None, error: None, + confirmed_by_output: false, })) } @@ -989,6 +1011,7 @@ mod tests { changes: Vec::new(), duration_ms: None, error: None, + confirmed_by_output: false, })) } @@ -1004,6 +1027,18 @@ mod tests { self } + fn confirmed(mut self, command: &str, exit_code: i32) -> Self { + self = self.run(command, exit_code); + if let Some(Event { + kind: EventKind::ToolCall(call), + .. + }) = self.0.events.last_mut() + { + call.confirmed_by_output = true; + } + self + } + fn extract(&self) -> Vec> { Extractor::default() .extract(&self.0, &RepoId::new("5f0c3a1e"), true) @@ -1231,16 +1266,83 @@ mod tests { } #[test] - fn stores_checks_without_tails_that_hide_their_result() { + fn a_check_whose_status_is_hidden_does_not_pass() { for command in [ "npm test 2>&1 | tail -5", + "cargo test 2>&1 | head -40", "npm test || true", "npm test; echo ok", + "cd web && npm test | tee test.log", ] { + let edited = || { + Sketch::new() + .prompt("Fix the pagination bug in the catalog") + .edit("src/paginate.js", &[]) + }; + + assert_eq!( + edited().run(command, 0).extract_one(), + Err(Rejection::StatusHidden { + command: command.to_owned() + }), + "{command}" + ); + assert_eq!( + edited().run("npm test", 1).run(command, 0).extract_one(), + Err(Rejection::VerificationFailed { + command: "npm test".to_owned() + }), + "{command}" + ); + let procedure = edited() + .run("npm test", 0) + .run(command, 0) + .extract_one() + .expect("the earlier check passed"); + assert_eq!( + procedure.verify.map(|verify| verify.command).as_deref(), + Some("npm test"), + "{command}" + ); + } + } + + #[test] + fn a_hidden_status_confirmed_by_output_counts() { + let edited = || { + Sketch::new() + .prompt("Fix the pagination bug in the catalog") + .edit("src/paginate.js", &[]) + }; + + let procedure = edited() + .run("npm test", 1) + .confirmed("npm test 2>&1 | tail -20", 0) + .extract_one() + .expect("the output showed the check passing"); + assert_eq!( + procedure.verify.map(|verify| verify.command).as_deref(), + Some("npm test") + ); + assert_eq!( + edited() + .run("npm test", 0) + .confirmed("npm test 2>&1 | tail -20", 1) + .extract_one(), + Err(Rejection::VerificationFailed { + command: "npm test".to_owned() + }) + ); + } + + #[test] + fn stores_checks_without_redirects_and_tails() { + for command in ["npm test 2>&1", "npm test 2>&1 | tail -5"] { let procedure = Sketch::new() .prompt("Fix the pagination bug in the catalog") .edit("src/paginate.js", &[]) - .run(command, 0) + .run(command, 1) + .run("npm test 2>&1", 0) .extract_one() .expect("the check passed"); @@ -1252,6 +1354,20 @@ mod tests { } } + #[test] + fn a_hidden_status_does_not_teach_a_replacement() { + let procedure = Sketch::new() + .prompt("Fix the pagination bug in the catalog") + .run("cargo test --lib", 101) + .run("cargo test 2>&1 | tail -20", 0) + .edit("src/paginate.rs", &[]) + .run("cargo test", 0) + .extract_one() + .expect("cargo test passed after the edit"); + + assert!(procedure.avoid.is_empty(), "{:?}", procedure.avoid); + } + #[test] fn a_failing_check_needs_a_later_pass_of_the_same_kind() { let checked = |runs: &[(&str, i32)]| { @@ -1890,6 +2006,29 @@ mod tests { assert_eq!(outcomes, [TaskOutcome::Unjudged, TaskOutcome::Failed]); } + #[test] + fn a_hidden_exit_status_leaves_the_task_unjudged() { + let outcomes: Vec = Extractor::default() + .extract( + &Sketch::new() + .prompt("Fix the off-by-one in src/paginate.js") + .edit("src/paginate.js", &[]) + .run("npm test 2>&1 | tail -20", 0) + .prompt("Fix the rounding in src/totals.js") + .edit("src/totals.js", &[]) + .run("npm test", 1) + .run("npm test || true", 0) + .0, + &RepoId::new("5f0c3a1e"), + true, + ) + .into_iter() + .map(|extraction| extraction.outcome) + .collect(); + + assert_eq!(outcomes, [TaskOutcome::Unjudged, TaskOutcome::Failed]); + } + #[test] fn titles_are_first_sentences() { assert_eq!(Builder::title("Fix it. Then run the tests."), "Fix it"); diff --git a/crates/trodden-extract/src/verify.rs b/crates/trodden-extract/src/verify.rs index c870aee..44cf7da 100644 --- a/crates/trodden-extract/src/verify.rs +++ b/crates/trodden-extract/src/verify.rs @@ -1,4 +1,4 @@ -use std::sync::LazyLock; +use std::{mem, sync::LazyLock}; use regex::Regex; use trodden_capture::Command; @@ -141,6 +141,73 @@ impl Verification { program(passed) == program(failed) } + pub(crate) fn hides_status(command: &str) -> bool { + let segments = Self::segments(command); + let Some(check) = segments.iter().position(|(segment, separator)| { + segment.split_whitespace().next() != Some("cd") + || !["&&", ";", "\n"].contains(separator) + }) else { + return false; + }; + segments[check..] + .windows(2) + .any(|pair| pair[0].1 != "&&" && !pair[1].0.trim().is_empty()) + } + + fn segments(command: &str) -> Vec<(String, &'static str)> { + let chars: Vec = command.chars().collect(); + let mut segments = Vec::new(); + let mut segment = String::new(); + let mut quote = None; + let mut index = 0; + while let Some(&c) = chars.get(index) { + let next = chars.get(index + 1).copied(); + let separator = match (quote, c) { + (Some(open), _) if c == open => { + quote = None; + None + } + (Some('\''), _) => None, + (_, '\\') => { + segment.push(c); + segment.extend(next); + index += 2; + continue; + } + (Some(_), _) => None, + (None, '\'' | '"') => { + quote = Some(c); + None + } + (None, '|') => Some(match next { + Some('|') => "||", + Some('&') => "|&", + _ => "|", + }), + (None, '&') if next == Some('&') => Some("&&"), + (None, '&') + if index.checked_sub(1).map(|before| chars[before]) == Some('>') + || next == Some('>') => + { + None + } + (None, '&') => Some("&"), + (None, ';') => Some(";"), + (None, '\n') => Some("\n"), + (None, _) => None, + }; + if let Some(separator) = separator { + segments.push((mem::take(&mut segment), separator)); + index += separator.len(); + } else { + segment.push(c); + index += 1; + } + } + segments.push((segment, "")); + segments + } + pub(crate) fn clean(command: &str) -> String { let mut text = command.trim().to_owned(); loop { @@ -285,6 +352,41 @@ mod tests { } } + #[test] + fn finds_what_hides_a_checks_exit_status() { + for command in [ + "cargo test 2>&1 | tail -20", + "cargo test |& tee test.log", + "cargo test | sort", + "npm test || true", + "npm test || exit 0", + "npm test; echo ok", + "npm test & wait", + "cd web && npm test | head", + "cargo test && echo ok; true", + "pytest -q\necho done", + ] { + assert!(Verification::hides_status(command), "{command}"); + } + for command in [ + "cargo test", + "cargo test 2>&1", + "npm test &> out.log", + "npm test >&2", + "cargo build && cargo test", + "cargo test && echo ok", + "cd web && npm test", + "cd web; npm test", + "cd web\nnpm test", + "npm test;", + "pytest -k 'paging | totals'", + "pytest -k \"paging; totals\"", + "grep -q a\\|b x && pytest", + ] { + assert!(!Verification::hides_status(command), "{command}"); + } + } + #[test] fn strips_output_filters() { assert_eq!(