From adfbd72e6c0d7ef2375be143b694b058519e1ca4 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 9 Oct 2026 22:48:38 -0400 Subject: [PATCH 1/5] Include per-session model attribution in Run notifications (#606) --- README.md | 2 + src/harness/interpretation.rs | 11 +- src/harness/interpretation/models.rs | 59 ++++++++++ src/notification.rs | 16 ++- src/session.rs | 23 +++- src/session/models.rs | 87 +++++++++++++++ tests/run_notification.rs | 157 +++++++++++++++++++++++++++ 7 files changed, 345 insertions(+), 10 deletions(-) create mode 100644 src/harness/interpretation/models.rs create mode 100644 src/session/models.rs diff --git a/README.md b/README.md index de6b2e603..90d7a55a1 100644 --- a/README.md +++ b/README.md @@ -405,6 +405,8 @@ A Run that asks for a notification, by the flag or by `email.always`, makes the - **Subject**: `[thirdshift] /# : `, where the outcome is `ready for review`, `merged`, `failed` or `interrupted`. The title is left out if it can't be read from GitHub. - **Body**, plain text: a `Result:` line with the same outcome as the subject, so it is readable even when a long issue title hides the end of the subject; then the pull request URL (if any), the failure cause (if failed), after an Inherited failure with no Base fix taken its [`Base check:`, `Retry with:` and `Or set:` lines](#an-inherited-failure-links-the-base-branchs-checks), the session log path (if any), the [Command log](#logs) path as a `Command log:` line, the hostname and how long the Run took. +The `Built with` line names the command's configured Harness, Model and Effort. A `Session models` section names each session's Harness and Models reported by its output or retained records, including model switches and delegated Claude answers. It covers failed sessions, Resumes and child Runs too. When the Harness supplies no model evidence, the requested Model is labelled `(requested)`, or its default is labelled `(not reported)`. Effort is the session's configured Effort; it does not describe a delegated answer's Effort. Attribution records (`*.models.json`) are kept beside the Session logs; a failure to write or read them is a warning and never changes the Run's outcome. + A [Spec run](#spec-runs) sends at most one notification for the whole Spec, under the same rules, with its checks made once before any Ticket starts. Its subject names the Spec, its outcome is the Spec run's, and its body, after what a Run's holds (the Spec PR, if any), lists each Ticket's outcome, one line per Ticket as in the summary on stderr, such as `#21 landed with https://github.com/acme/widgets/pull/1` or `#22 blocked by #21`. The Ticket Runs inside it never send a notification of their own, whatever the User config says. An [Architect run](#architect-runs) takes the same flags and the same `email.always`, and sends [one notification](#one-run-notification) covering its review and the run it dispatched. diff --git a/src/harness/interpretation.rs b/src/harness/interpretation.rs index 1e25933a5..474e9d9e6 100644 --- a/src/harness/interpretation.rs +++ b/src/harness/interpretation.rs @@ -10,6 +10,7 @@ use crate::interrupt; mod stream; pub(super) use stream::Stream; +mod models; mod security; pub use security::SafeguardRefusal; use security::Security; @@ -21,6 +22,7 @@ pub struct Interpretation { decoder: Box, retained: Retained, security: Option, + models: models::Models, } /// Reporting survives ordinary failures; only success supplies Resume facts. @@ -33,7 +35,7 @@ pub struct Completion { pub struct Report { pub warnings: Vec, pub summary: Option, - /// Answering Models recovered from retained records, in response order. + /// Models reported by session output or retained records. pub models: Vec, } @@ -170,6 +172,7 @@ impl Interpretation { decoder, retained, security: None, + models: models::Models::new(cli), } } @@ -182,6 +185,7 @@ impl Interpretation { /// Unknown or malformed lines produce no progress, never an error. pub fn condense(&mut self, raw: &str) -> Vec { let mut lines = self.decoder.condense(raw); + self.models.observe(raw); if let Some(security) = &mut self.security && let Some(line) = security.observe(raw) { @@ -218,16 +222,13 @@ impl Interpretation { if let Err(error) = interrupt::check() { return (None, Err(Failure::Execution(error))); } - let security_session = self.security.is_some(); let mut facts = self.decoder.complete(); + facts.report.models = self.models.into_names(); let refusal = self.security.and_then(|security| security.refusal); if refusal.is_some() { facts.outcome = TurnOutcome::Failed; } let recovered = self.retained.reconcile(&mut facts); - if !security_session { - facts.report.models.clear(); - } if let Err(error) = interrupt::check() { return (None, Err(Failure::Execution(error))); } diff --git a/src/harness/interpretation/models.rs b/src/harness/interpretation/models.rs new file mode 100644 index 000000000..c27aeab99 --- /dev/null +++ b/src/harness/interpretation/models.rs @@ -0,0 +1,59 @@ +//! Models named by session evidence, including delegated Claude answers. +//! Requested defaults are reported by the session owner, never as observations. + +use serde_json::Value; + +use crate::harness::Harness; + +pub(super) struct Models { + harness: Option, + names: Vec, +} + +impl Models { + pub fn new(cli: &str) -> Self { + Self { + harness: Harness::named(cli), + names: Vec::new(), + } + } + + pub fn observe(&mut self, raw: &str) { + let Ok(event) = serde_json::from_str::(raw) else { + return; + }; + let model = match self.harness { + Some(Harness::Claude | Harness::Grok) + if event["type"] == "assistant" && event["is_api_error_message"] != true => + { + event["message"]["model"].as_str() + } + Some(Harness::Agy) if event["event"] == "init" => event["init"]["model"].as_str(), + _ => None, + }; + if let Some(model) = model { + self.include(model); + } + if self.harness == Some(Harness::Claude) + && event["type"] == "result" + && let Some(usage) = event["modelUsage"].as_object() + { + for model in usage.keys() { + self.include(model); + } + } + } + + fn include(&mut self, model: &str) { + if !model.is_empty() + && model != "" + && !self.names.iter().any(|name| name == model) + { + self.names.push(model.to_string()); + } + } + + pub fn into_names(self) -> Vec { + self.names + } +} diff --git a/src/notification.rs b/src/notification.rs index 9f05d9937..b81953554 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -143,13 +143,27 @@ impl RunNotification { }; let host = host::name(); let command_log = logs::command_log_path(); - let text = body( + let mut text = body( account, self.built_with.as_deref(), command_log.as_deref(), host.as_deref().unwrap_or("unknown host"), self.checked.started.elapsed(), ); + if let Some(log) = &command_log { + match crate::session::models::notification_lines(log) { + Ok(lines) if !lines.is_empty() => { + text += "\nSession models:\n"; + for line in lines { + text += &format!("{line}\n"); + } + } + Ok(_) => {} + Err(error) => progress::step(format_args!( + "warning: could not read session models for the Run notification: {error:#}" + )), + } + } if let Err(error) = self.checked.resend.send(&subject, &text) { progress::step(format_args!( "warning: could not send the Run notification: {error:#}" diff --git a/src/session.rs b/src/session.rs index 91cdd8be9..91c5c4fa7 100644 --- a/src/session.rs +++ b/src/session.rs @@ -23,6 +23,7 @@ use crate::progress; use crate::prompt; use crate::skills; +pub mod models; mod review_reports; use review_reports::ReviewReports; @@ -318,7 +319,7 @@ impl Outside for OnMachine { }; run( kind, - self.adapter, + &self.harness, &self.worktree, invocation, log, @@ -352,12 +353,13 @@ fn ending_with(killed: &[&str]) -> String { /// and fails with `interrupted`. fn run( kind: &str, - adapter: &'static dyn Adapter, + choice: &Choice, worktree: &Path, invocation: Invocation, log: &Path, stream: Interpretation, ) -> Result { + let adapter = choice.harness.adapter(); let cli = adapter.name(); if let Some(dir) = log.parent() { fs::create_dir_all(dir).with_context(|| format!("could not create {}", dir.display()))?; @@ -377,9 +379,22 @@ fn run( )?; let completion = executed.state.finish(executed.execution); + let report_models = completion + .report + .as_ref() + .map(|report| report.models.clone()) + .unwrap_or_default(); + models::keep(kind, log, choice, report_models); if let Some(report) = completion.report { - for model in report.models { - progress::step(format_args!("{kind}: Model: {model}")); + // Stream models already have security progress lines. Retained records + // need theirs here, as before. + if matches!( + choice.harness, + crate::harness::Harness::Muse | crate::harness::Harness::OpenCode + ) { + for model in report.models { + progress::step(format_args!("{kind}: Model: {model}")); + } } for warning in report.warnings { progress::step(format_args!("{kind}: {warning}")); diff --git a/src/session/models.rs b/src/session/models.rs new file mode 100644 index 000000000..d499b4f34 --- /dev/null +++ b/src/session/models.rs @@ -0,0 +1,87 @@ +//! Per-session model evidence beside Session logs. Child Runs write their own +//! files so the owning command can include their models in its one notification. + +use std::fs; +use std::path::Path; + +use anyhow::Result; +use serde::{Deserialize, Serialize}; + +use crate::harness::Choice; +use crate::{logs, progress}; + +#[derive(Serialize, Deserialize)] +struct SessionModels { + kind: String, + harness: String, + requested: Option, + effort: Option, + observed: Vec, +} + +/// Record only model metadata; failure costs attribution, never the session. +pub(super) fn keep(kind: &str, log: &Path, choice: &Choice, observed: Vec) { + let record = SessionModels { + kind: kind.to_string(), + harness: choice.harness.name().to_string(), + requested: choice.model.clone(), + effort: choice.effort.clone(), + observed, + }; + let path = log.with_extension("models.json"); + let kept = serde_json::to_vec(&record) + .map_err(anyhow::Error::from) + .and_then(|json| fs::write(&path, json).map_err(anyhow::Error::from)); + if let Err(error) = kept { + progress::step(format_args!( + "warning: could not record session models: {error:#}" + )); + } +} + +/// Read this command's evidence, including child Runs sharing its start stamp. +/// Other commands in the same repository and folder are excluded. +pub fn notification_lines(command_log: &Path) -> Result> { + let Some(dir) = command_log.parent() else { + return Ok(Vec::new()); + }; + let marker = format!("-{}-", logs::stamp()); + let mut paths = Vec::new(); + for entry in fs::read_dir(dir)? { + let path = entry?.path(); + if path + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.contains(&marker) && name.ends_with(".models.json")) + { + paths.push(path); + } + } + paths.sort(); + paths + .into_iter() + .map(|path| { + let record: SessionModels = serde_json::from_slice(&fs::read(path)?)?; + let models = if record.observed.is_empty() { + record.requested.map_or_else( + || "Harness default (not reported)".to_string(), + |model| format!("{model} (requested)"), + ) + } else { + let mut names = Vec::new(); + for model in record.observed { + if !names.contains(&model) { + names.push(model); + } + } + names.join(", ") + }; + Ok(format!( + "- {}: {} · {models} · session effort: {}", + record.kind, + record.harness, + record.effort.as_deref().unwrap_or("default effort") + )) + }) + .collect() +} diff --git a/tests/run_notification.rs b/tests/run_notification.rs index 924758a78..74eeb4fc3 100644 --- a/tests/run_notification.rs +++ b/tests/run_notification.rs @@ -74,6 +74,163 @@ fn assert_contains(text: &str, part: &str) { assert!(text.contains(part), "expected {part:?} in: {text}"); } +#[test] +fn a_spec_notification_includes_child_models_and_excludes_an_earlier_commands_models() { + let scenario = Scenario::new(); + scenario.agent_does_for(7, &format!( + "printf '%s\\n' '{{\"type\":\"assistant\",\"message\":{{\"model\":\"earlier-model\",\"content\":[]}}}}'\n{AGENT_OPENS_PR}" + )); + let earlier = scenario.run(&[&scenario.issue_url(7)]); + assert_eq!(earlier.code, Some(0), "{}", earlier.stderr); + scenario.spec_has_tickets(20, &[(21, &[]), (22, &[])]); + for (ticket, model) in [(21, "claude-sonnet-5"), (22, "claude-opus-4-8")] { + scenario.agent_does_for(ticket, &format!(r#" +printf '%s\n' '{{"type":"assistant","message":{{"model":"{model}","content":[]}}}}' +echo {ticket} > feature-{ticket}.txt +git add feature-{ticket}.txt +git commit -q -m 'Ticket {ticket}' +gh pr create --base issue-20 --head issue-{ticket} --title 'Ticket {ticket}' --body 'Closes #{ticket}' +"#)); + } + scenario.agent_does_for(20, "true\n"); + let resend = ResendStandIn::replying(200, ACCEPTED); + let result = run( + &scenario, + &resend, + &[ + &scenario.issue_url(20), + "parallel", + "2", + "email", + "me@example.com", + ], + Some(KEY), + ); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let request = the_one_request(&resend); + let text = text(&request); + assert_contains(text, "- implement: claude · claude-sonnet-5"); + assert_contains(text, "- implement: claude · claude-opus-4-8"); + assert_contains( + text, + "- spec-review: claude · Harness default (not reported)", + ); + assert!(!text.contains("earlier-model"), "{text}"); +} + +#[test] +fn a_codex_notification_labels_requested_models_for_the_review_and_its_resume() { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, AGENT_OPENS_PR); + scenario.agent_does_in_session(2, r#" +printf '%s\n' '{"type":"item.started","item":{"id":"test","type":"command_execution","command":"cargo test","status":"in_progress"}}' +"#); + scenario.agent_does_in_session(3, "exit 3\n"); + let resend = ResendStandIn::replying(200, ACCEPTED); + let result = run( + &scenario, + &resend, + &[ + &scenario.issue_url(7), + "harness", + "codex", + "model", + "gpt-6.1-sol", + "effort", + "high", + "security-review", + "merge", + "email", + "me@example.com", + ], + Some(KEY), + ); + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(scenario.codex_calls().len(), 3); + let request = the_one_request(&resend); + let text = text(&request); + for kind in ["implement", "security-review", "security-review-resume"] { + assert_contains( + text, + &format!("- {kind}: codex · gpt-6.1-sol (requested) · session effort: high"), + ); + } +} + +#[test] +fn a_notification_includes_delegated_models_and_result_usage_without_synthetic_answers() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + r#" +printf '%s\n' '{{"type":"assistant","message":{{"model":"claude-opus-5-5","content":[]}}}}' +printf '%s\n' '{{"type":"assistant","parent_tool_use_id":"child","message":{{"model":"claude-sonnet-5","content":[]}}}}' +printf '%s\n' '{{"type":"assistant","message":{{"model":"","content":[]}}}}' +printf '%s\n' '{{"type":"assistant","is_api_error_message":true,"message":{{"model":"refused-model","content":[]}}}}' +printf '%s\n' '{{"type":"result","subtype":"success","modelUsage":{{"claude-haiku-4-5":{{"inputTokens":100}}}}}}' +{AGENT_OPENS_PR} +"# + )); + let resend = ResendStandIn::replying(200, ACCEPTED); + let result = run( + &scenario, + &resend, + &[&scenario.issue_url(7), "email", "me@example.com"], + Some(KEY), + ); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let request = the_one_request(&resend); + let text = text(&request); + assert_contains(text, "claude-opus-5-5, claude-sonnet-5, claude-haiku-4-5"); + assert!(!text.contains(""), "{text}"); + assert!(!text.contains("refused-model"), "{text}"); +} + +#[test] +fn a_failed_security_review_notification_names_each_sessions_models() { + let scenario = Scenario::new(); + scenario.agent_does_in_session( + 1, + &format!( + "printf '%s\\n' '{{\"type\":\"assistant\",\"message\":{{\"model\":\"claude-opus-5-5\",\"content\":[]}}}}'\n{AGENT_OPENS_PR}" + ), + ); + scenario.agent_does_in_session( + 2, + r#" +printf '%s\n' '{"type":"assistant","message":{"model":"claude-opus-4-8","content":[]}}' +printf '%s\n' '{"type":"assistant","message":{"model":"claude-opus-4-8","content":[]}}' +exit 3 +"#, + ); + let resend = ResendStandIn::replying(200, ACCEPTED); + let result = run( + &scenario, + &resend, + &[ + &scenario.issue_url(7), + "security-review", + "merge", + "email", + "me@example.com", + ], + Some(KEY), + ); + assert_eq!(result.code, Some(1), "{}", result.stderr); + let request = the_one_request(&resend); + let text = text(&request); + assert_contains(text, "Security review incomplete"); + assert_contains(text, "Built with claude · default model · default effort"); + assert_contains( + text, + "- implement: claude · claude-opus-5-5 · session effort: default effort", + ); + assert_contains( + text, + "- security-review: claude · claude-opus-4-8 · session effort: default effort", + ); + assert_eq!(text.matches("claude-opus-4-8").count(), 1, "{text}"); +} + #[test] fn a_run_ready_for_review_sends_one_notification_to_the_address_given() { let scenario = Scenario::new(); From 0147b571e555d7d9682fd2174b5a9a34332a3bf4 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 9 Oct 2026 23:02:27 -0400 Subject: [PATCH 2/5] Preserve interrupted model evidence and isolate Command attribution --- README.md | 2 +- src/child_run.rs | 1 + src/harness/interpretation.rs | 6 ++ src/harness/interpretation/models.rs | 29 +++++--- src/harness/interpretation/security.rs | 31 +++----- src/session.rs | 14 +++- src/session/models.rs | 73 ++++++++++++------- tests/run_notification.rs | 97 ++++++++++++++++++++++++++ 8 files changed, 193 insertions(+), 60 deletions(-) diff --git a/README.md b/README.md index 90d7a55a1..f60d72c33 100644 --- a/README.md +++ b/README.md @@ -405,7 +405,7 @@ A Run that asks for a notification, by the flag or by `email.always`, makes the - **Subject**: `[thirdshift] /# : `, where the outcome is `ready for review`, `merged`, `failed` or `interrupted`. The title is left out if it can't be read from GitHub. - **Body**, plain text: a `Result:` line with the same outcome as the subject, so it is readable even when a long issue title hides the end of the subject; then the pull request URL (if any), the failure cause (if failed), after an Inherited failure with no Base fix taken its [`Base check:`, `Retry with:` and `Or set:` lines](#an-inherited-failure-links-the-base-branchs-checks), the session log path (if any), the [Command log](#logs) path as a `Command log:` line, the hostname and how long the Run took. -The `Built with` line names the command's configured Harness, Model and Effort. A `Session models` section names each session's Harness and Models reported by its output or retained records, including model switches and delegated Claude answers. It covers failed sessions, Resumes and child Runs too. When the Harness supplies no model evidence, the requested Model is labelled `(requested)`, or its default is labelled `(not reported)`. Effort is the session's configured Effort; it does not describe a delegated answer's Effort. Attribution records (`*.models.json`) are kept beside the Session logs; a failure to write or read them is a warning and never changes the Run's outcome. +The `Built with` line names the command's configured Harness, Model and Effort. A `Session models` section names each session's Harness and Models reported by its output or retained records, including model switches and delegated Claude answers. It covers failed or interrupted sessions, Resumes and child Runs belonging to that Command. When the Harness supplies no model evidence, the requested Model is labelled `(requested)`, or its default is labelled `(not reported)`. Effort is the session's configured Effort; it does not describe a delegated answer's Effort. Attribution records (`*.models.json`) are kept beside the Session logs; a failure to write or read them is a warning and never changes the Run's outcome. A [Spec run](#spec-runs) sends at most one notification for the whole Spec, under the same rules, with its checks made once before any Ticket starts. Its subject names the Spec, its outcome is the Spec run's, and its body, after what a Run's holds (the Spec PR, if any), lists each Ticket's outcome, one line per Ticket as in the summary on stderr, such as `#21 landed with https://github.com/acme/widgets/pull/1` or `#22 blocked by #21`. The Ticket Runs inside it never send a notification of their own, whatever the User config says. diff --git a/src/child_run.rs b/src/child_run.rs index 6d528b636..153154785 100644 --- a/src/child_run.rs +++ b/src/child_run.rs @@ -387,6 +387,7 @@ fn start_using( startup: impl FnOnce(&mut Handle) -> Result<()>, ) -> Result { let mut command = Command::new(executable); + crate::session::models::inherit_command(&mut command); // On Linux `executable` is a link, and the child goes by this process's // command instead. if cfg!(target_os = "linux") diff --git a/src/harness/interpretation.rs b/src/harness/interpretation.rs index 474e9d9e6..e22a9a395 100644 --- a/src/harness/interpretation.rs +++ b/src/harness/interpretation.rs @@ -182,6 +182,12 @@ impl Interpretation { self } + /// Stream evidence already read, even when interruption suppresses + /// completion reporting and retained-record reads. + pub fn observed_models(&self) -> Vec { + self.models.names().to_vec() + } + /// Unknown or malformed lines produce no progress, never an error. pub fn condense(&mut self, raw: &str) -> Vec { let mut lines = self.decoder.condense(raw); diff --git a/src/harness/interpretation/models.rs b/src/harness/interpretation/models.rs index c27aeab99..eaf6bd3e2 100644 --- a/src/harness/interpretation/models.rs +++ b/src/harness/interpretation/models.rs @@ -22,16 +22,7 @@ impl Models { let Ok(event) = serde_json::from_str::(raw) else { return; }; - let model = match self.harness { - Some(Harness::Claude | Harness::Grok) - if event["type"] == "assistant" && event["is_api_error_message"] != true => - { - event["message"]["model"].as_str() - } - Some(Harness::Agy) if event["event"] == "init" => event["init"]["model"].as_str(), - _ => None, - }; - if let Some(model) = model { + if let Some(model) = reported_model(self.harness, &event) { self.include(model); } if self.harness == Some(Harness::Claude) @@ -56,4 +47,22 @@ impl Models { pub fn into_names(self) -> Vec { self.names } + + pub fn names(&self) -> &[String] { + &self.names + } +} + +/// Shared protocol evidence. Callers choose whether delegated answers count. +pub(super) fn reported_model(harness: Option, event: &Value) -> Option<&str> { + let model = match harness { + Some(Harness::Claude | Harness::Grok) + if event["type"] == "assistant" && event["is_api_error_message"] != true => + { + event["message"]["model"].as_str() + } + Some(Harness::Agy) if event["event"] == "init" => event["init"]["model"].as_str(), + _ => None, + }; + model.filter(|model| !model.is_empty() && *model != "") } diff --git a/src/harness/interpretation/security.rs b/src/harness/interpretation/security.rs index 2ed61ea9b..6f2b17dad 100644 --- a/src/harness/interpretation/security.rs +++ b/src/harness/interpretation/security.rs @@ -49,7 +49,7 @@ impl Security { pub fn observe(&mut self, raw: &str) -> Option { let event = serde_json::from_str::(raw).ok()?; - let model = match self.harness { + match self.harness { Some(Harness::Claude) => { let refused = match event["type"].as_str() { Some("system") => { @@ -66,9 +66,7 @@ impl Security { if refused { self.refusal = Some(SafeguardRefusal::ClaudeCyber); } - Self::answer_model(&event) } - Some(Harness::Grok) => Self::answer_model(&event), Some(Harness::Codex) => { if event["type"] == "turn.failed" && event["error"]["message"] @@ -88,29 +86,18 @@ impl Security { self.last_model = Some(model.to_string()); return Some(line); } - None } - Some(Harness::Agy) if event["event"] == "init" => event["init"]["model"].as_str(), - _ => None, - }?; - if model.is_empty() || self.last_model.as_deref() == Some(model) { + _ => {} + } + // Init names the requested Model; result.modelUsage includes sub-agents. + // Security progress names only main-loop answers, while notification + // attribution also counts delegated answers. + let model = super::models::reported_model(self.harness, &event) + .filter(|_| event["parent_tool_use_id"].is_null())?; + if self.last_model.as_deref() == Some(model) { return None; } self.last_model = Some(model.to_string()); Some(format!("Model: {model}")) } - - // Init names the requested Model; result.modelUsage includes sub-agents. - // Only main-loop assistant messages identify the Model that answered. - fn answer_model(event: &Value) -> Option<&str> { - if event["type"] != "assistant" - || !event["parent_tool_use_id"].is_null() - || event["is_api_error_message"] == true - { - return None; - } - event["message"]["model"] - .as_str() - .filter(|model| *model != "") - } } diff --git a/src/session.rs b/src/session.rs index 91c5c4fa7..a75f4ec21 100644 --- a/src/session.rs +++ b/src/session.rs @@ -368,6 +368,7 @@ fn run( File::create(log).with_context(|| format!("could not create {}", log.display()))?; let started = Instant::now(); let (kind_owned, log_owned) = (kind.to_string(), log.to_path_buf()); + let choice_owned = choice.clone(); let executed = process::streaming( adapter, Command::new(cli) @@ -375,7 +376,18 @@ fn run( .current_dir(worktree), invocation.stdin.as_deref(), stream, - move |output, stream| follow(&kind_owned, output, &mut log_file, &log_owned, stream), + move |output, stream| { + let followed = follow(&kind_owned, output, &mut log_file, &log_owned, stream); + // The process owner joins this reader before returning, including + // on interruption, when it deliberately suppresses state recovery. + models::keep( + &kind_owned, + &log_owned, + &choice_owned, + stream.observed_models(), + ); + followed + }, )?; let completion = executed.state.finish(executed.execution); diff --git a/src/session/models.rs b/src/session/models.rs index d499b4f34..a0ebded72 100644 --- a/src/session/models.rs +++ b/src/session/models.rs @@ -2,7 +2,8 @@ //! files so the owning command can include their models in its one notification. use std::fs; -use std::path::Path; +use std::path::{Path, PathBuf}; +use std::process::Command; use anyhow::Result; use serde::{Deserialize, Serialize}; @@ -12,6 +13,7 @@ use crate::{logs, progress}; #[derive(Serialize, Deserialize)] struct SessionModels { + command_log: Option, kind: String, harness: String, requested: Option, @@ -22,6 +24,7 @@ struct SessionModels { /// Record only model metadata; failure costs attribution, never the session. pub(super) fn keep(kind: &str, log: &Path, choice: &Choice, observed: Vec) { let record = SessionModels { + command_log: owning_command_log(), kind: kind.to_string(), harness: choice.harness.name().to_string(), requested: choice.model.clone(), @@ -39,6 +42,22 @@ pub(super) fn keep(kind: &str, log: &Path, choice: &Choice, observed: Vec Option { + logs::command_log_path().or_else(|| std::env::var_os(PARENT_COMMAND_LOG).map(PathBuf::from)) +} + +/// Give child Runs the exact owning Command, independently of timestamp +/// collisions. Nested child Runs pass on that same ownership. +pub fn inherit_command(command: &mut Command) { + if let Some(log) = owning_command_log() { + command.env(PARENT_COMMAND_LOG, log); + } else { + command.env_remove(PARENT_COMMAND_LOG); + } +} + /// Read this command's evidence, including child Runs sharing its start stamp. /// Other commands in the same repository and folder are excluded. pub fn notification_lines(command_log: &Path) -> Result> { @@ -58,30 +77,32 @@ pub fn notification_lines(command_log: &Path) -> Result> { } } paths.sort(); - paths - .into_iter() - .map(|path| { - let record: SessionModels = serde_json::from_slice(&fs::read(path)?)?; - let models = if record.observed.is_empty() { - record.requested.map_or_else( - || "Harness default (not reported)".to_string(), - |model| format!("{model} (requested)"), - ) - } else { - let mut names = Vec::new(); - for model in record.observed { - if !names.contains(&model) { - names.push(model); - } + let mut lines = Vec::new(); + for path in paths { + let record: SessionModels = serde_json::from_slice(&fs::read(path)?)?; + if record.command_log.as_deref() != Some(command_log) { + continue; + } + let models = if record.observed.is_empty() { + record.requested.map_or_else( + || "Harness default (not reported)".to_string(), + |model| format!("{model} (requested)"), + ) + } else { + let mut names = Vec::new(); + for model in record.observed { + if !names.contains(&model) { + names.push(model); } - names.join(", ") - }; - Ok(format!( - "- {}: {} · {models} · session effort: {}", - record.kind, - record.harness, - record.effort.as_deref().unwrap_or("default effort") - )) - }) - .collect() + } + names.join(", ") + }; + lines.push(format!( + "- {}: {} · {models} · session effort: {}", + record.kind, + record.harness, + record.effort.as_deref().unwrap_or("default effort") + )); + } + Ok(lines) } diff --git a/tests/run_notification.rs b/tests/run_notification.rs index 74eeb4fc3..121b56e30 100644 --- a/tests/run_notification.rs +++ b/tests/run_notification.rs @@ -74,6 +74,103 @@ fn assert_contains(text: &str, part: &str) { assert!(text.contains(part), "expected {part:?} in: {text}"); } +#[test] +fn a_notification_excludes_an_unrelated_commands_models_with_the_same_stamp() { + let scenario = Scenario::new(); + let started = scenario.path("agent-started"); + let release = scenario.path("agent-release"); + scenario.agent_does(&format!( + "printf '%s\\n' '{}'\ntouch '{}'\n\ + while [ ! -e '{}' ]; do sleep 0.01; done\n{AGENT_OPENS_PR}", + r#"{"type":"assistant","message":{"model":"current-model","content":[]}}"#, + started.display(), + release.display(), + )); + let resend = ResendStandIn::replying(200, ACCEPTED); + let held = scenario.run_until( + &[&scenario.issue_url(7), "email", "me@example.com"], + &env(&resend, Some(KEY)), + "agent-started", + ); + + let folder = "home/.thirdshift/logs/acme/widgets/commands/issue"; + let command_logs = scenario.log_files(folder, "log"); + assert_eq!(command_logs.len(), 1, "{command_logs:?}"); + let stamp = command_logs[0] + .strip_prefix("7-") + .unwrap() + .strip_suffix(".log") + .unwrap(); + let directory = scenario.path(folder); + + // Normal files belonging to an independent issue #8 Command that + // started in the same second; #8 is not a child of this Run. + std::fs::write(directory.join(format!("8-{stamp}.log")), "").unwrap(); + std::fs::write( + directory.join(format!("8-{stamp}-implement.models.json")), + r#"{"kind":"implement","harness":"claude","requested":null,"effort":null,"observed":["unrelated-model"]}"#, + ).unwrap(); + // A record with explicit ownership must also be excluded when its + // owning Command differs, even though its timestamp matches. + std::fs::write( + directory.join(format!("9-{stamp}-implement.models.json")), + serde_json::json!({ + "command_log": directory.join(format!("9-{stamp}.log")), + "kind": "implement", "harness": "claude", "requested": null, + "effort": null, "observed": ["other-command-model"] + }) + .to_string(), + ) + .unwrap(); + std::fs::write(&release, "").unwrap(); + + let result = held.finish(); + assert_eq!(result.code, Some(0), "{}", result.stderr); + let request = the_one_request(&resend); + assert_contains(text(&request), "- implement: claude · current-model"); + assert!( + !text(&request).contains("other-command-model"), + "{}", + text(&request) + ); + assert!( + !text(&request).contains("unrelated-model"), + "{}", + text(&request), + ); +} + +#[test] +fn an_interrupted_notification_keeps_models_observed_before_the_signal() { + let scenario = Scenario::new(); + let started = scenario.path("agent-started"); + scenario.agent_does(&format!( + "printf '%s\\n' '{}'\nprintf '%s\\n' '{}'\ntouch '{}'\nsleep 30\n", + r#"{"type":"assistant","message":{"model":"claude-opus-5-5","content":[]}}"#, + r#"{"type":"assistant","parent_tool_use_id":"child","message":{"model":"claude-sonnet-5","content":[]}}"#, + started.display(), + )); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = scenario.run_and_signal_with_env( + &[&scenario.issue_url(7), "--email", "me@example.com"], + &env(&resend, Some(KEY)), + "agent-started", + "TERM", + ); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + let transcript = std::fs::read_to_string(the_one_log(&scenario)).unwrap(); + assert_contains(&transcript, "claude-opus-5-5"); + assert_contains(&transcript, "claude-sonnet-5"); + let request = the_one_request(&resend); + assert!(subject(&request).ends_with(": interrupted")); + assert_contains( + text(&request), + "- implement: claude · claude-opus-5-5, claude-sonnet-5", + ); +} + #[test] fn a_spec_notification_includes_child_models_and_excludes_an_earlier_commands_models() { let scenario = Scenario::new(); From 018a82db5daeb16b2378fe674189f618d271ccf5 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 9 Oct 2026 23:20:52 -0400 Subject: [PATCH 3/5] thirdshift: failed run (interrupted) 2026-10-10T03:20:52Z, host thirdshift. Uncommitted work at the time of failure is included in this commit. From ce870c379cb0fb81faff10feeb781cd7695a7772 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Sat, 10 Oct 2026 04:00:05 +0000 Subject: [PATCH 4/5] Preserve valid model attribution when another record is corrupt --- src/session/models.rs | 18 +++++++++++++-- tests/run_notification.rs | 48 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/src/session/models.rs b/src/session/models.rs index a0ebded72..15468979f 100644 --- a/src/session/models.rs +++ b/src/session/models.rs @@ -5,7 +5,7 @@ use std::fs; use std::path::{Path, PathBuf}; use std::process::Command; -use anyhow::Result; +use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; use crate::harness::Choice; @@ -79,7 +79,21 @@ pub fn notification_lines(command_log: &Path) -> Result> { paths.sort(); let mut lines = Vec::new(); for path in paths { - let record: SessionModels = serde_json::from_slice(&fs::read(path)?)?; + let read = fs::read(&path) + .map_err(anyhow::Error::from) + .and_then(|json| { + serde_json::from_slice::(&json).map_err(anyhow::Error::from) + }) + .with_context(|| format!("could not read {}", path.display())); + let record = match read { + Ok(record) => record, + Err(error) => { + progress::step(format_args!( + "warning: could not read session models for the Run notification: {error:#}" + )); + continue; + } + }; if record.command_log.as_deref() != Some(command_log) { continue; } diff --git a/tests/run_notification.rs b/tests/run_notification.rs index 121b56e30..aab8a5a2e 100644 --- a/tests/run_notification.rs +++ b/tests/run_notification.rs @@ -74,6 +74,54 @@ fn assert_contains(text: &str, part: &str) { assert!(text.contains(part), "expected {part:?} in: {text}"); } +#[test] +fn a_notification_keeps_current_models_when_an_unrelated_record_is_corrupt() { + let scenario = Scenario::new(); + let started = scenario.path("agent-started"); + let release = scenario.path("agent-release"); + scenario.agent_does(&format!( + "printf '%s\\n' '{}'\nprintf '%s\\n' '{}'\ntouch '{}'\n\ + while [ ! -e '{}' ]; do sleep 0.01; done\n{AGENT_OPENS_PR}", + r#"{"type":"assistant","message":{"model":"current-model","content":[]}}"#, + r#"{"type":"assistant","message":{"model":"second-model","content":[]}}"#, + started.display(), + release.display(), + )); + let resend = ResendStandIn::replying(200, ACCEPTED); + let held = scenario.run_until( + &[&scenario.issue_url(7), "email", "me@example.com"], + &env(&resend, Some(KEY)), + "agent-started", + ); + let folder = "home/.thirdshift/logs/acme/widgets/commands/issue"; + let command_logs = scenario.log_files(folder, "log"); + assert_eq!(command_logs.len(), 1, "{command_logs:?}"); + let stamp = command_logs[0] + .strip_prefix("7-") + .unwrap() + .strip_suffix(".log") + .unwrap(); + std::fs::write( + scenario + .path(folder) + .join(format!("8-{stamp}-implement.models.json")), + "{ truncated", + ) + .unwrap(); + std::fs::write(&release, "").unwrap(); + let result = held.finish(); + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_contains( + &result.stderr, + "warning: could not read session models for the Run notification:", + ); + let request = the_one_request(&resend); + assert_contains( + text(&request), + "- implement: claude · current-model, second-model", + ); +} + #[test] fn a_notification_excludes_an_unrelated_commands_models_with_the_same_stamp() { let scenario = Scenario::new(); From ddaeaca9d4cde5c3ae0de9fb2c96fb5b808606be Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Sat, 10 Oct 2026 04:18:00 +0000 Subject: [PATCH 5/5] Include model records in Command log directory expectations --- tests/command_logs.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/command_logs.rs b/tests/command_logs.rs index e5eb79b40..1be20610d 100644 --- a/tests/command_logs.rs +++ b/tests/command_logs.rs @@ -136,6 +136,7 @@ fn a_run_keeps_a_command_log_of_everything_it_printed_and_its_session_logs_share logs_in(&scenario, "commands/issue"), [ format!("7-{stamp}-implement.jsonl"), + format!("7-{stamp}-implement.models.json"), format!("7-{stamp}.log") ] ); @@ -201,6 +202,7 @@ fn a_pickup_run_that_takes_an_issue_keeps_a_command_log_from_its_first_line() { logs_in(&scenario, "commands/pickup"), [ format!("7-{stamp}-implement.jsonl"), + format!("7-{stamp}-implement.models.json"), format!("7-{stamp}.log") ] ); @@ -222,9 +224,12 @@ fn a_pickup_spec_keeps_its_tickets_session_logs_beside_its_command_log() { logs_in(&scenario, "commands/pickup"), [ format!("20-{stamp}-spec-review.jsonl"), + format!("20-{stamp}-spec-review.models.json"), format!("20-{stamp}.log"), format!("21-{stamp}-implement.jsonl"), + format!("21-{stamp}-implement.models.json"), format!("22-{stamp}-implement.jsonl"), + format!("22-{stamp}-implement.models.json"), ] ); assert!(!scenario.path(LOGS).join("sessions").exists());