From b3d16caa5746959803d870ec96a17f401b8c6207 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 26 Oct 2026 19:47:43 -0400 Subject: [PATCH 1/3] Preserve completion lines across late background replies (#1405) --- site/how-it-works/index.html | 1 + src/harness/interpretation/stream.rs | 39 ++++++++- src/harness/interpretation_tests.rs | 119 +++++++++++++++++++++++++++ tests/bug_run.rs | 80 +++++++++++++++++- 4 files changed, 236 insertions(+), 3 deletions(-) diff --git a/site/how-it-works/index.html b/site/how-it-works/index.html index 2c41808e..3dd83d60 100644 --- a/site/how-it-works/index.html +++ b/site/how-it-works/index.html @@ -100,6 +100,7 @@

How thirdshift works

03 / What the agent does

Let the agent implement and review

thirdshift starts a headless session in the selected Harness, in the Worktree. The Session prompt supplies the issue, Base branch, Issue branch and review instructions. Factory skills are linked where that Harness discovers them. The opening session carries the change from implementation through review to a committed branch and pull request.

+

On Claude and Grok, a Session's completion line survives later background-task notifications. A later reply carrying a completion line takes precedence.

Inside the opening agent session
    diff --git a/src/harness/interpretation/stream.rs b/src/harness/interpretation/stream.rs index 3291b02e..193e6afa 100644 --- a/src/harness/interpretation/stream.rs +++ b/src/harness/interpretation/stream.rs @@ -2,7 +2,8 @@ //! //! A result ends a turn, not necessarily the session: background sub-agents //! can resume it. Only the first init sets shared session fields, and only -//! completion supplies the last result and any work killed since that result. +//! completion supplies the last result holding a completion line (otherwise +//! the last result) and any work killed since the latest result. use std::collections::{BTreeMap, HashMap}; @@ -13,6 +14,7 @@ use super::{ TurnOutcome, }; use crate::progress::{bash, shorten}; +use crate::prompt; mod events; pub struct Stream { @@ -118,6 +120,31 @@ fn message_text(event: &Value) -> Option { (!text.is_empty()).then(|| text.join("\n")) } +/// Recognize handoffs here, leaving their payload validation to each reader. +/// Keep the whole reply: Architecture reviews also carry a strength line. +fn has_completion_line(message: &str) -> bool { + let line = message.trim_end().lines().last().unwrap_or("").trim(); + matches!( + line, + prompt::BUG_REPRODUCED_LINE + | prompt::AUDIT_COMPLETE_LINE + | prompt::AUDIT_INCOMPLETE_LINE + | prompt::CLOSE_CHECK_PASSES_LINE + ) || [ + prompt::BUG_NOT_REPRODUCED_LINE, + prompt::PLAN_LINE, + prompt::IDEA_LINE, + prompt::ALREADY_FILED_LINE, + prompt::CLOSE_CHECK_FAILS_PREFIX, + prompt::SECURITY_FIX_LINE, + prompt::SECURITY_FIX_SPEC_LINE, + "Security review: ", + "Security reproduction: ", + ] + .iter() + .any(|prefix| line.starts_with(prefix)) +} + impl Dialect { /// Extract live facts before root-result and initialization guards. Grok /// shares assistant framing without Claude's usage or safeguard rules. @@ -289,7 +316,15 @@ impl Stream { { return Vec::new(); } - self.final_message = event["result"].as_str().map(String::from); + let message = event["result"].as_str(); + if message.is_some_and(has_completion_line) + || !self + .final_message + .as_deref() + .is_some_and(has_completion_line) + { + self.final_message = message.map(String::from); + } self.completed = true; if let (Some(turns), Some(cost)) = ( event["num_turns"].as_u64(), diff --git a/src/harness/interpretation_tests.rs b/src/harness/interpretation_tests.rs index 89cc9bcb..a730416c 100644 --- a/src/harness/interpretation_tests.rs +++ b/src/harness/interpretation_tests.rs @@ -239,6 +239,125 @@ fn claude_completion_keeps_the_last_result_and_the_last_complete_usage() { ); } +#[test] +fn completion_line_survives_background_notification() { + let messages: Vec<_> = [Harness::Grok, Harness::Claude] + .into_iter() + .map(|harness| { + let completion = after_background_notification( + harness, + "PR ready.\nBug reproduced: fixed", + "That background failure was already handled. The PR is unchanged.", + ); + completion.outcome.unwrap().final_message + }) + .collect(); + assert_eq!( + messages, + vec![Some("PR ready.\nBug reproduced: fixed".to_string()); 2] + ); +} + +#[test] +fn completion_handoffs_survive_late_replies_and_later_handoffs_win() { + for harness in [Harness::Grok, Harness::Claude] { + for message in [ + "Bug not reproduced: https://github.com/acme/widgets/issues/7#issuecomment-42", + "Recommendation strength: Strong\nArchitecture review plan: https://github.com/acme/widgets/issues/8", + "Architecture review idea: https://github.com/acme/widgets/issues/9", + "Architecture review already filed: https://github.com/acme/widgets/issues/10", + "Security audit: complete", + "Security audit: incomplete", + "Security close check: passes", + "Security close check: fails: test still fails", + "Security fix Ticket: https://github.com/acme/widgets/issues/11", + "Security fix Spec: https://github.com/acme/widgets/issues/12", + "Security reproduction: reproduced high single", + "Security reproduction: not reproduced", + ] { + for later in ["Background task handled.", ""] { + let ended = after_background_notification(harness, message, later) + .outcome + .unwrap(); + assert_eq!( + ended.require_final_message("fixture").unwrap(), + message, + "{harness:?}" + ); + } + let ended = after_background_notification(harness, "Bug reproduced: fixed", message) + .outcome + .unwrap(); + assert_eq!(ended.final_message.as_deref(), Some(message), "{harness:?}"); + } + } +} + +#[test] +fn security_review_reads_its_completion_line_after_a_background_notification() { + let message = concat!( + "Reviewed.\n", + r#"Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"],"pre_existing_count":0}"# + ); + for harness in [Harness::Grok, Harness::Claude] { + let ended = after_background_notification(harness, message, "Background task handled.") + .outcome + .unwrap(); + let review = + crate::security::review::Review::from_final_message(ended.final_message.as_deref()) + .unwrap(); + assert_eq!(review.findings, ["Cross-tenant read"]); + } +} + +#[test] +fn only_root_result_handoffs_count_and_later_failures_still_fail() { + let mut interpretation = stream(Harness::Claude, ""); + for event in [ + r#"{"type":"result","subtype":"success","result":"Bug reproduced: fixed"}"#, + r#"{"type":"result","subtype":"success","parent_tool_use_id":"child","result":"Security audit: incomplete"}"#, + ] { + interpretation.decode(event); + } + assert_eq!( + finish(interpretation) + .outcome + .unwrap() + .final_message + .as_deref(), + Some("Bug reproduced: fixed") + ); + for harness in [Harness::Grok, Harness::Claude] { + let mut interpretation = stream(harness, ""); + for event in [ + r#"{"type":"result","subtype":"success","result":"Bug reproduced: fixed"}"#, + r#"{"type":"result","subtype":"error","is_error":true,"result":"Late turn failed"}"#, + ] { + interpretation.decode(event); + } + assert!(finish(interpretation).outcome.is_err(), "{harness:?}"); + } +} + +fn after_background_notification( + harness: Harness, + earlier: &str, + later: &str, +) -> interpretation::Completion { + let mut interpretation = stream(harness, ""); + for event in [ + serde_json::json!({"type": "result", "subtype": "success", "result": earlier}), + serde_json::json!({ + "type": "system", "subtype": "task_notification", "task_id": "test", + "status": "failed", "summary": "The earlier background test failed." + }), + serde_json::json!({"type": "result", "subtype": "success", "result": later}), + ] { + interpretation.condense(&event.to_string()); + } + finish(interpretation) +} + pub(crate) fn stream(harness: Harness, prompt: &str) -> interpretation::Interpretation { // A missing working directory makes optional OpenCode acquisition fail // locally, so in-process fallback tests never invoke a machine's CLI. diff --git a/tests/bug_run.rs b/tests/bug_run.rs index 57c6f842..1f355517 100644 --- a/tests/bug_run.rs +++ b/tests/bug_run.rs @@ -6,7 +6,7 @@ use support::Scenario; use support::resend::saved_notification; const IMPLEMENT: &str = r#" -test -f .claude/skills/thirdshift-diagnosing-bugs/SKILL.md || exit 1 +test -f .claude/skills/thirdshift-diagnosing-bugs/SKILL.md || test -f .agents/skills/thirdshift-diagnosing-bugs/SKILL.md || exit 1 echo feature > feature.txt git add feature.txt git commit -q -m 'Fix bug' @@ -14,6 +14,84 @@ gh pr create --base main --head issue-7 --title 'Fix bug' --body 'Closes #7' echo 'Bug reproduced: fixed' > "$FAKE_CLAUDE_FINAL_MESSAGE" "#; +fn late_background_reply(earlier: &str) -> String { + let earlier = serde_json::json!({"type": "result", "subtype": "success", "result": earlier}); + format!( + r#" +if [ -n "${{FAKE_GROK_RESULT:-}}" ]; then + cat > "$FAKE_GROK_RESULT" <<'JSON' +{earlier} +JSON +else + : > "$FAKE_CLAUDE_AFTER_RESULT" +fi +cat >> "${{FAKE_GROK_RESULT:-$FAKE_CLAUDE_AFTER_RESULT}}" <<'JSON' +{{"type":"system","subtype":"task_notification","task_id":"test","status":"failed","summary":"Earlier test failed"}} +{{"type":"result","subtype":"success","result":"The earlier background failure was handled. The PR is unchanged."}} +JSON +"# + ) +} + +#[test] +fn bug_diagnosis_survives_a_late_background_reply() { + for harness in ["claude", "grok"] { + for reproduced in [true, false] { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["bug", "ready-for-agent"]); + let script = if reproduced { + IMPLEMENT + } else { + r#" +gh fake labels 7 '["bug", "needs-info", "in-progress"]' +echo 'Bug not reproduced: https://github.com/acme/widgets/issues/7#issuecomment-42' > "$FAKE_CLAUDE_FINAL_MESSAGE" +"# + }; + let earlier = if reproduced { + "Bug reproduced: fixed" + } else { + "Bug not reproduced: https://github.com/acme/widgets/issues/7#issuecomment-42" + }; + scenario.agent_does(&format!("{script}\n{}", late_background_reply(earlier))); + + let result = scenario.run(&["harness", harness, &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!( + result.stdout, + if reproduced { + "https://github.com/acme/widgets/pull/1\n" + } else { + "https://github.com/acme/widgets/issues/7#issuecomment-42\n" + } + ); + } + } +} + +#[test] +fn bug_diagnosis_without_any_completion_line_keeps_its_error_after_a_late_reply() { + for harness in ["claude", "grok"] { + let scenario = Scenario::new(); + scenario.issue_labelled(7, &["bug"]); + scenario.agent_does(&format!( + "echo 'Done.' > \"$FAKE_CLAUDE_FINAL_MESSAGE\"\n{}", + late_background_reply("Done.") + )); + + let result = scenario.run(&["harness", harness, &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert!( + result.stderr.contains( + "bug diagnosis ended without a reproduced or not reproduced completion line" + ), + "{}", + result.stderr + ); + } +} + /// Probes the checkout the way #1362 did, unless the prompt names a merge base. /// An already-fixed branch then goes on to review and the pull request. const DIAGNOSE_AT_NAMED_MERGE_BASE: &str = r#" From 6ff11678ca79af9f28b181c010a0362853b7aa96 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 26 Oct 2026 19:54:20 -0400 Subject: [PATCH 2/3] Retain assistant completion replies before the sole result --- src/harness/interpretation/stream.rs | 34 +++++++++---- src/harness/interpretation_tests.rs | 74 ++++++++++++++++++++++++++++ tests/bug_run.rs | 18 +++---- 3 files changed, 107 insertions(+), 19 deletions(-) diff --git a/src/harness/interpretation/stream.rs b/src/harness/interpretation/stream.rs index 193e6afa..d1eb2aa4 100644 --- a/src/harness/interpretation/stream.rs +++ b/src/harness/interpretation/stream.rs @@ -2,7 +2,7 @@ //! //! A result ends a turn, not necessarily the session: background sub-agents //! can resume it. Only the first init sets shared session fields, and only -//! completion supplies the last result holding a completion line (otherwise +//! completion supplies the last reply holding a completion line (otherwise //! the last result) and any work killed since the latest result. use std::collections::{BTreeMap, HashMap}; @@ -23,6 +23,7 @@ pub struct Stream { /// Cumulative totals survive later results without a complete pair. turns_and_cost: Option<(u64, f64)>, final_message: Option, + completion_message: Option, completed: bool, dialect: Dialect, } @@ -214,6 +215,7 @@ impl Stream { init: None, turns_and_cost: None, final_message: None, + completion_message: None, completed: false, dialect, } @@ -237,6 +239,12 @@ impl Stream { } } + fn remember_completion(&mut self, message: &str) { + if has_completion_line(message) { + self.completion_message = Some(message.to_string()); + } + } + fn tool_use(&self, name: &str, input: &Value) -> String { let grok = matches!(self.dialect, Dialect::Grok(_)); let name = if grok && matches!(name, "bash" | "run_terminal_command") { @@ -290,6 +298,11 @@ impl Stream { Some("assistant") => { if event["parent_tool_use_id"].is_null() { self.completed = false; + if !assistant_error(event) + && let Some(message) = message_text(event) + { + self.remember_completion(&message); + } } if let Dialect::Claude(claude) = &mut self.dialect { claude.assistant(event); @@ -316,15 +329,10 @@ impl Stream { { return Vec::new(); } - let message = event["result"].as_str(); - if message.is_some_and(has_completion_line) - || !self - .final_message - .as_deref() - .is_some_and(has_completion_line) - { - self.final_message = message.map(String::from); + if let Some(message) = event["result"].as_str() { + self.remember_completion(message); } + self.final_message = event["result"].as_str().map(String::from); self.completed = true; if let (Some(turns), Some(cost)) = ( event["num_turns"].as_u64(), @@ -383,7 +391,13 @@ impl Decoder for Stream { let mut ended = Ended { session_id: self.init.and_then(|init| init.session_id), killed: Vec::new(), - final_message: self.final_message, + // A handoff can precede the sole result, but cannot supply a + // missing terminal result itself. + final_message: if self.completed { + self.completion_message.or(self.final_message) + } else { + self.final_message + }, completed: self.completed, }; let (failed, diagnostic) = match self.dialect { diff --git a/src/harness/interpretation_tests.rs b/src/harness/interpretation_tests.rs index a730416c..f304c0c8 100644 --- a/src/harness/interpretation_tests.rs +++ b/src/harness/interpretation_tests.rs @@ -239,6 +239,80 @@ fn claude_completion_keeps_the_last_result_and_the_last_complete_usage() { ); } +#[test] +fn assistant_completion_line_survives_a_late_result() { + for harness in [Harness::Grok, Harness::Claude] { + for earlier in [ + "PR ready.\nBug reproduced: fixed", + "Bug not reproduced: https://github.com/acme/widgets/issues/7#issuecomment-42", + "Security audit: complete", + ] { + let later = "The earlier background failure was already handled."; + let mut interpretation = stream(harness, ""); + for event in [ + serde_json::json!({ + "type": "assistant", + "message": { + "id": "earlier", "role": "assistant", + "content": [{"type": "text", "text": earlier}] + } + }), + serde_json::json!({ + "type": "system", "subtype": "task_notification", + "task_id": "test", "status": "failed", + "summary": "The earlier background test failed." + }), + serde_json::json!({ + "type": "assistant", + "message": { + "id": "later", "role": "assistant", + "content": [{"type": "text", "text": later}] + } + }), + serde_json::json!({ + "type": "result", "subtype": "success", "result": later + }), + ] { + interpretation.condense(&event.to_string()); + } + let ended = finish(interpretation).outcome.unwrap(); + assert_eq!( + ended.require_final_message("fixture").unwrap(), + earlier, + "{harness:?}" + ); + } + } +} + +#[test] +fn assistant_handoffs_keep_reply_order_and_require_a_root_result() { + for harness in [Harness::Grok, Harness::Claude] { + let mut interpretation = stream(harness, ""); + interpretation.condense(r#"{"type":"assistant","message":{"content":[{"type":"text","text":"Bug reproduced: fixed"}]}}"#); + assert_eq!(finish(interpretation).outcome.unwrap().final_message, None); + + let mut interpretation = stream(harness, ""); + for event in [ + r#"{"type":"result","subtype":"success","result":"Bug reproduced: fixed"}"#, + r#"{"type":"assistant","message":{"content":[{"type":"text","text":"Security audit: incomplete"}]}}"#, + r#"{"type":"assistant","parent_tool_use_id":"child","message":{"content":[{"type":"text","text":"Security audit: complete"}]}}"#, + r#"{"type":"result","subtype":"success","result":"Background task handled."}"#, + ] { + interpretation.decode(event); + } + assert_eq!( + finish(interpretation) + .outcome + .unwrap() + .final_message + .as_deref(), + Some("Security audit: incomplete"), + "{harness:?}" + ); + } +} + #[test] fn completion_line_survives_background_notification() { let messages: Vec<_> = [Harness::Grok, Harness::Claude] diff --git a/tests/bug_run.rs b/tests/bug_run.rs index 1f355517..016d26b2 100644 --- a/tests/bug_run.rs +++ b/tests/bug_run.rs @@ -15,20 +15,20 @@ echo 'Bug reproduced: fixed' > "$FAKE_CLAUDE_FINAL_MESSAGE" "#; fn late_background_reply(earlier: &str) -> String { - let earlier = serde_json::json!({"type": "result", "subtype": "success", "result": earlier}); + let earlier = serde_json::json!({ + "type": "assistant", "message": { + "id": "earlier", "role": "assistant", + "content": [{"type": "text", "text": earlier}] + } + }); format!( r#" -if [ -n "${{FAKE_GROK_RESULT:-}}" ]; then - cat > "$FAKE_GROK_RESULT" <<'JSON' +cat <<'JSON' {earlier} -JSON -else - : > "$FAKE_CLAUDE_AFTER_RESULT" -fi -cat >> "${{FAKE_GROK_RESULT:-$FAKE_CLAUDE_AFTER_RESULT}}" <<'JSON' {{"type":"system","subtype":"task_notification","task_id":"test","status":"failed","summary":"Earlier test failed"}} -{{"type":"result","subtype":"success","result":"The earlier background failure was handled. The PR is unchanged."}} +{{"type":"assistant","message":{{"id":"later","role":"assistant","content":[{{"type":"text","text":"The earlier background failure was handled. The PR is unchanged."}}]}}}} JSON +echo 'The earlier background failure was handled. The PR is unchanged.' > "$FAKE_CLAUDE_FINAL_MESSAGE" "# ) } From b1aedb2066da1f48224ee87f74b0be4934cf0648 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 26 Oct 2026 20:37:53 -0400 Subject: [PATCH 3/3] Do not retain a completion line from a Grok child result A child result that ends on a completion line was kept when the root session's later reply had none, so that delegated text could replace the root security review. Root results still supply the handoff. --- src/harness/interpretation/stream.rs | 7 ++++- src/harness/interpretation_tests.rs | 44 ++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/harness/interpretation/stream.rs b/src/harness/interpretation/stream.rs index d1eb2aa4..464e13c4 100644 --- a/src/harness/interpretation/stream.rs +++ b/src/harness/interpretation/stream.rs @@ -329,7 +329,12 @@ impl Stream { { return Vec::new(); } - if let Some(message) = event["result"].as_str() { + // A child result may replace the latest text, but it is not a + // root handoff. Remembering it would let that text outlive a + // later root reply that carries no completion line. + if event["parent_tool_use_id"].is_null() + && let Some(message) = event["result"].as_str() + { self.remember_completion(message); } self.final_message = event["result"].as_str().map(String::from); diff --git a/src/harness/interpretation_tests.rs b/src/harness/interpretation_tests.rs index f304c0c8..691c47f4 100644 --- a/src/harness/interpretation_tests.rs +++ b/src/harness/interpretation_tests.rs @@ -413,6 +413,50 @@ fn only_root_result_handoffs_count_and_later_failures_still_fail() { } } +/// A delegated result is not a root handoff. Its completion line must not +/// replace the root session's security review when a later root reply has none. +#[test] +fn grok_child_result_cannot_clear_a_root_security_review() { + let root = concat!( + "Reviewed.\n", + r#"Security review: {"unaddressed_count":1,"findings":["Cross-tenant read"],"pre_existing_count":0}"# + ); + let child = r#"Security review: {"unaddressed_count":0,"findings":[],"pre_existing_count":0}"#; + for harness in [Harness::Grok, Harness::Claude] { + let mut interpretation = stream(harness, ""); + for event in [ + serde_json::json!({"type": "result", "subtype": "success", "result": root}), + serde_json::json!({ + "type": "result", + "subtype": "success", + "parent_tool_use_id": "child", + "result": child + }), + serde_json::json!({ + "type": "result", + "subtype": "success", + "result": "Background task handled." + }), + ] { + interpretation.condense(&event.to_string()); + } + let message = finish(interpretation) + .outcome + .unwrap() + .final_message + .unwrap_or_default(); + assert_ne!(message, child, "{harness:?}"); + let review = crate::security::review::Review::from_final_message(Some(&message)); + assert!( + review + .as_ref() + .ok() + .is_none_or(|review| !review.findings.is_empty()), + "{harness:?} accepted a cleared review from {message:?}" + ); + } +} + fn after_background_notification( harness: Harness, earlier: &str,