From ff2ae3d274f807c7e908213af8b281455d88c2d4 Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Sat, 29 Aug 2026 14:15:27 +0200 Subject: [PATCH 1/2] fix(ding): say why a delivery was deferred instead of breaking silently A deferral is the one outcome that delivers nothing and leaves nothing behind, and it had no log line at all: any pane the classifier could not positively type as empty-and-safe got zero delivery and zero diagnostic. PokeOutcome::Deferred now carries which cause produced it, flush_pending reports it to the watch loop, and the loop logs the edge only, so a pane stuck in one verdict costs one line rather than one per retry. agent-identity: dev3.direct.claude.paqjmjfq agent-persona: generalist agent-supervisor: unavailable agent-tool: Claude Code agent-tool-version: 2.1.250 agent-runtime: Claude Code 2.1.250 tooling-profile: dotfiles@a1a5f89 --- src/ding/composer.rs | 15 ++- src/ding/harness/claude.rs | 4 + src/ding/harness/codex.rs | 4 + src/ding/harness/mod.rs | 3 + src/ding/harness/opencode.rs | 4 + src/ding/mod.rs | 186 ++++++++++++++++++++++++++++++++--- 6 files changed, 203 insertions(+), 13 deletions(-) diff --git a/src/ding/composer.rs b/src/ding/composer.rs index 79488a1c..e7aebbc8 100644 --- a/src/ding/composer.rs +++ b/src/ding/composer.rs @@ -177,6 +177,17 @@ pub(super) fn strip_ansi(input: &str) -> String { /// it can type into, or submit, a human's live draft. Scrollback is above the live composer by /// construction, so picking the lowest needs no per-pair special case. pub(super) fn classify_composer(screen: &str, expected: &str) -> ComposerState { + classify_located_composer(screen, expected).0 +} + +/// The same routing, additionally naming which maintained harness owned the composer that was +/// classified. `None` means no maintained composer was locatable at all — which is the difference +/// between "a human is drafting in a harness we understand" and "we do not recognise this pane", +/// and the two want different operator responses. +pub(super) fn classify_located_composer( + screen: &str, + expected: &str, +) -> (ComposerState, Option<&'static str>) { let plain = strip_ansi(screen); let screen = Screen { raw: screen, @@ -190,9 +201,9 @@ pub(super) fn classify_composer(screen: &str, expected: &str) -> ComposerState { .map(|located| (located.row, harness)) }) .max_by_key(|(row, _)| *row) - .map(|(_, harness)| harness.classify(&screen, expected)) + .map(|(_, harness)| (harness.classify(&screen, expected), Some(harness.name()))) // No maintained composer is locatable, so nothing is proven either way. - .unwrap_or(ComposerState::Ambiguous) + .unwrap_or((ComposerState::Ambiguous, None)) } /// Route post-submit receipt classification through the lowest maintained live composer, using diff --git a/src/ding/harness/claude.rs b/src/ding/harness/claude.rs index 25f66acd..522b6aca 100644 --- a/src/ding/harness/claude.rs +++ b/src/ding/harness/claude.rs @@ -9,6 +9,10 @@ use crate::ding::composer::{ pub(super) struct Claude; impl Harness for Claude { + fn name(&self) -> &'static str { + "claude" + } + fn locate(&self, screen: &Screen<'_>) -> Option { located_bottom_claude_composer(screen.plain).map(|(row, _, _)| Located { row }) } diff --git a/src/ding/harness/codex.rs b/src/ding/harness/codex.rs index e7191806..b9253750 100644 --- a/src/ding/harness/codex.rs +++ b/src/ding/harness/codex.rs @@ -9,6 +9,10 @@ use crate::ding::composer::{ pub(super) struct Codex; impl Harness for Codex { + fn name(&self) -> &'static str { + "codex" + } + fn locate(&self, screen: &Screen<'_>) -> Option { // The markers are ANSI, so this locator works in raw byte offsets while the router compares // stripped rows. Every marker starts at an `\x1b[` boundary, so stripping the prefix is diff --git a/src/ding/harness/mod.rs b/src/ding/harness/mod.rs index ee9fca8b..7c504e18 100644 --- a/src/ding/harness/mod.rs +++ b/src/ding/harness/mod.rs @@ -50,6 +50,9 @@ pub(super) struct Located { } pub(super) trait Harness { + /// How this harness is named in operator-facing diagnostics. + fn name(&self) -> &'static str; + /// Locate this harness's composer, if this screen has one. fn locate(&self, screen: &Screen<'_>) -> Option; diff --git a/src/ding/harness/opencode.rs b/src/ding/harness/opencode.rs index f103b77f..56876338 100644 --- a/src/ding/harness/opencode.rs +++ b/src/ding/harness/opencode.rs @@ -6,6 +6,10 @@ use crate::ding::composer::ComposerState; pub(super) struct OpenCode; impl Harness for OpenCode { + fn name(&self) -> &'static str { + "opencode" + } + fn locate(&self, screen: &Screen<'_>) -> Option { locate_composer(screen.plain).map(|composer| Located { row: composer.start, diff --git a/src/ding/mod.rs b/src/ding/mod.rs index 7e8de03f..007594e8 100644 --- a/src/ding/mod.rs +++ b/src/ding/mod.rs @@ -30,7 +30,7 @@ use crate::run::{CAPTURE_CAP_BYTES, read_bounded_tail, reap_detached}; use crate::status; use crate::supervisor_chain::{SUPERVISOR_CHAIN_LIMIT, chain_bus_ids, resolve_spec}; -use composer::{ComposerState, classify_composer, classify_receipt}; +use composer::{ComposerState, classify_composer, classify_located_composer, classify_receipt}; use harness::ReceiptState; const BRACKETED_PASTE_START: &str = "\x1b[200~"; @@ -243,7 +243,66 @@ pub enum PokeOutcome { /// A maintained adapter positively proved that the exact staged notice is absent. Queue state /// decides whether an archive receipt makes that proof sufficient to relinquish ownership. NotRetained, - Deferred, + Deferred(DeferralReason), +} + +/// Why one attempt performed no input at all. +/// +/// A deferral is the one outcome that both delivers nothing and leaves nothing behind, so it is +/// the one that must say why. The two composer verdicts are deliberately distinct: a human drafting +/// in a harness we understand is a wait, while a pane no maintained harness can locate is a gap in +/// coverage that will never resolve on its own. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DeferralReason { + /// The lowest maintained composer holds text that is not the exact notice — typically a human + /// draft. Named by harness only: the text on that pane belongs to whoever is typing it. + ComposerChanged { harness: &'static str }, + /// No maintained harness could locate a composer on this pane, so nothing is proven either + /// way. An unrecognised, resized, or not-yet-drawn TUI lands here. + NoMaintainedComposer, + /// The poker performs no input of its own; delivery is somebody else's job. + NoInputPerformed, +} + +impl std::fmt::Display for DeferralReason { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::ComposerChanged { harness } => write!( + formatter, + "the {harness} composer holds other text (a draft or an unfinished turn); waiting rather than typing over it" + ), + Self::NoMaintainedComposer => formatter.write_str( + "no maintained harness could locate a composer on this pane; nothing will be delivered here until one can", + ), + Self::NoInputPerformed => formatter.write_str("this poker performs no input"), + } + } +} + +/// Whether a deferral is news, so a pane stuck in one verdict costs one log line rather than one +/// per retry. The first deferral after progress is news, a changed verdict is news, and the same +/// verdict repeating is the same fact. +#[derive(Debug, Default)] +pub struct DeferralJournal { + last: Option, +} + +impl DeferralJournal { + pub fn observe(&mut self, current: Option) -> bool { + if self.last == current { + return false; + } + self.last = current; + current.is_some() + } +} + +/// What one `flush_pending` chose not to do. Deliberately a plain struct rather than an +/// `Option`: callers that only want the flush keep compiling as statements. +#[derive(Debug, Default, Clone, Copy)] +pub struct FlushReport { + /// Set when the front notice performed no input this pass. + pub deferred: Option, } /// How DING delivers a poke and checks liveness, abstracted so the watch loop is testable without a @@ -251,7 +310,7 @@ pub enum PokeOutcome { pub trait Poker { fn poke(&self, text: &str) -> anyhow::Result; fn retry_staged(&self, _text: &str) -> anyhow::Result { - Ok(PokeOutcome::Deferred) + Ok(PokeOutcome::Deferred(DeferralReason::NoInputPerformed)) } fn adopt_staged(&self, _candidates: &[String]) -> anyhow::Result> { Ok(None) @@ -569,7 +628,8 @@ fn observed_poke_with_window( before_submit: &mut dyn FnMut() -> anyhow::Result<()>, observation_window: Duration, ) -> anyhow::Result { - match classify_composer(&peek()?, text) { + let (state, harness) = classify_located_composer(&peek()?, text); + match state { ComposerState::ExactSafe => { return submit_after_final_observation( text, @@ -583,7 +643,15 @@ fn observed_poke_with_window( ComposerState::ExactBlocked => return Ok(PokeOutcome::Staged), ComposerState::EmptySafe => {} ComposerState::Changed | ComposerState::Ambiguous => { - return Ok(PokeOutcome::Deferred); + // `Ambiguous` without a located harness is the unrecognised pane; with one it is a + // maintained harness that proved nothing about this screen, which reads the same to an + // operator as an unlocatable composer. + return Ok(PokeOutcome::Deferred(match (state, harness) { + (ComposerState::Changed, Some(harness)) => { + DeferralReason::ComposerChanged { harness } + } + _ => DeferralReason::NoMaintainedComposer, + })); } } @@ -969,6 +1037,7 @@ pub fn run_ding( let mut logged_waiting = false; let mut last_refresh: Option = None; let mut next_delivery_attempt: Option = None; + let mut deferrals = DeferralJournal::default(); loop { if stop.load(Ordering::SeqCst) { @@ -1022,7 +1091,15 @@ pub fn run_ding( } } } - flush_pending(context, status_path, &mut pending, poker); + let report = flush_pending(context, status_path, &mut pending, poker); + if deferrals.observe(report.deferred) + && let Some(reason) = report.deferred + { + tracing::warn!( + "st2 ding: delivery deferred for '{}', no input performed: {reason}", + context.recipient + ); + } next_delivery_attempt = (startup_candidates.is_some() || !pending.is_empty()) .then(|| Instant::now() + DELIVERY_RETRY_BACKOFF); } @@ -1084,9 +1161,10 @@ fn flush_pending( status_path: Option<&Path>, pending: &mut VecDeque, poker: &dyn Poker, -) { +) -> FlushReport { + let mut report = FlushReport::default(); if delivery_suppressed(status_path) { - return; + return report; } let mut resolver = None; @@ -1115,7 +1193,7 @@ fn flush_pending( notice.set_staged_text(Some(text)); break; } - Ok(PokeOutcome::Deferred) if was_staged => { + Ok(PokeOutcome::Deferred(_)) if was_staged => { // The exact owned payload disappeared or changed. Adopted startup text has the // generic recovery notice behind it, while unread ordinary work may make one later // fresh guarded attempt. Archived work is done. @@ -1125,13 +1203,20 @@ fn flush_pending( } pending.pop_front(); } - Ok(PokeOutcome::Deferred) => break, + // The one outcome that delivers nothing and leaves nothing behind. Report it out so + // the watch loop can say so; an unreported break here is how an eleven-day fleet-wide + // delivery failure stayed invisible. + Ok(PokeOutcome::Deferred(reason)) => { + report.deferred = Some(reason); + break; + } Err(error) => { tracing::warn!("st2 ding: {error}"); break; } } } + report } /// Set by SIGINT/SIGTERM so `st2 ding` exits cleanly when st2 tears the sidecar down. @@ -1294,7 +1379,7 @@ mod tests { fn poke(&self, text: &str) -> anyhow::Result { self.calls.lock().unwrap().push(text.to_string()); if self.defer.load(Ordering::SeqCst) { - return Ok(PokeOutcome::Deferred); + return Ok(PokeOutcome::Deferred(DeferralReason::NoMaintainedComposer)); } let mut failures = self.failures.lock().unwrap(); if *failures > 0 { @@ -3844,4 +3929,83 @@ Enter to select · ↑/↓ to navigate · Esc to cancel"; SessionLiveness::Dead ); } + + /// A deferral is the one outcome that both delivers nothing and leaves nothing behind, so it + /// has to carry why. The two causes want different responses: a human drafting in a harness we + /// understand clears on its own, a pane no harness can locate never will. + #[test] + fn a_deferral_names_the_cause_that_produced_it() { + let text = "[DING] ? cos: guarded [id:abc123]"; + for (screen, expected) in [ + ( + human_codex_screen(), + DeferralReason::ComposerChanged { harness: "codex" }, + ), + ( + "unrecognized renderer".to_string(), + DeferralReason::NoMaintainedComposer, + ), + ] { + let outcome = observed_poke_with_window( + text, + &mut || Ok(screen.clone()), + &mut || Ok(()), + &mut || Ok(()), + &mut || {}, + &mut || Ok(()), + Duration::ZERO, + ) + .unwrap(); + assert_eq!(outcome, PokeOutcome::Deferred(expected)); + } + } + + /// The deferral has to reach the watch loop, which is the only place that knows the recipient + /// and can decide whether it is worth saying. + #[test] + fn flush_reports_a_deferral_outward() { + let catalog = tempfile::tempdir().unwrap(); + let context = DingContext { + catalog_root: catalog.path(), + this_host: "h", + recipient: "h.recipient", + }; + let notice = || { + VecDeque::from([PendingNotice::message(msg( + "1785070000000-abc123.md", + "h.sender", + Some("hello"), + ))]) + }; + + let poker = RecordingPoker::live(); + poker.defer.store(true, Ordering::SeqCst); + assert_eq!( + flush_pending(context, None, &mut notice(), &poker).deferred, + Some(DeferralReason::NoMaintainedComposer) + ); + + // A delivery that lands has nothing to report. + let poker = RecordingPoker::live(); + assert_eq!( + flush_pending(context, None, &mut notice(), &poker).deferred, + None + ); + } + + /// A pane stuck in one verdict costs one line, not one per retry — but a changed verdict, and + /// the first deferral after delivery resumed, are both news. + #[test] + fn only_a_changed_deferral_verdict_is_worth_reporting() { + let changed = DeferralReason::ComposerChanged { harness: "codex" }; + let unknown = DeferralReason::NoMaintainedComposer; + let mut journal = DeferralJournal::default(); + + assert!(journal.observe(Some(changed))); + assert!(!journal.observe(Some(changed))); + assert!(journal.observe(Some(unknown))); + assert!(!journal.observe(Some(unknown))); + assert!(!journal.observe(None)); + assert!(journal.observe(Some(unknown))); + } } From 5f08eee92f825c8c0e6baca17952883dc394bab0 Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Sat, 29 Aug 2026 16:54:38 +0200 Subject: [PATCH 2/2] fix(ding): report a located-but-unproven composer as a wait, not a coverage gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A harness that locates its composer and still returns Ambiguous — an active turn, a modal, an unrecognised footer — was folded into NoMaintainedComposer, which says no harness could locate the pane and that it will not clear until one can. Both halves were false for a covered pane that may settle on its own. Per review on #375. agent-identity: dev3.direct.claude.paqjmjfq agent-persona: generalist agent-supervisor: unavailable agent-tool: Claude Code agent-tool-version: 2.1.250 agent-runtime: Claude Code 2.1.250 tooling-profile: dotfiles@a1a5f89 --- src/ding/mod.rs | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/ding/mod.rs b/src/ding/mod.rs index 007594e8..fa055d26 100644 --- a/src/ding/mod.rs +++ b/src/ding/mod.rs @@ -257,6 +257,10 @@ pub enum DeferralReason { /// The lowest maintained composer holds text that is not the exact notice — typically a human /// draft. Named by harness only: the text on that pane belongs to whoever is typing it. ComposerChanged { harness: &'static str }, + /// A maintained harness located its composer but proved nothing about this screen — an active + /// turn, a modal, or a footer it does not recognise. Distinct from an unlocatable pane: this + /// one is covered and can clear on its own, so it is a wait rather than a coverage gap. + ComposerUnproven { harness: &'static str }, /// No maintained harness could locate a composer on this pane, so nothing is proven either /// way. An unrecognised, resized, or not-yet-drawn TUI lands here. NoMaintainedComposer, @@ -271,6 +275,10 @@ impl std::fmt::Display for DeferralReason { formatter, "the {harness} composer holds other text (a draft or an unfinished turn); waiting rather than typing over it" ), + Self::ComposerUnproven { harness } => write!( + formatter, + "the {harness} composer was located but proved nothing about this screen (an active turn, a modal, or an unrecognised footer); waiting for it to settle" + ), Self::NoMaintainedComposer => formatter.write_str( "no maintained harness could locate a composer on this pane; nothing will be delivered here until one can", ), @@ -643,14 +651,17 @@ fn observed_poke_with_window( ComposerState::ExactBlocked => return Ok(PokeOutcome::Staged), ComposerState::EmptySafe => {} ComposerState::Changed | ComposerState::Ambiguous => { - // `Ambiguous` without a located harness is the unrecognised pane; with one it is a - // maintained harness that proved nothing about this screen, which reads the same to an - // operator as an unlocatable composer. + // Whether a harness was located is the difference between a wait and a coverage gap, + // so it decides the reason rather than being folded into one catch-all. return Ok(PokeOutcome::Deferred(match (state, harness) { (ComposerState::Changed, Some(harness)) => { DeferralReason::ComposerChanged { harness } } - _ => DeferralReason::NoMaintainedComposer, + // `Changed` cannot arise without a located harness — only a located composer can + // be read as holding other text — but the classifier owns that invariant, not + // this call site, so an unlocated pane is reported as exactly what was observed. + (_, Some(harness)) => DeferralReason::ComposerUnproven { harness }, + (_, None) => DeferralReason::NoMaintainedComposer, })); } } @@ -3941,6 +3952,12 @@ Enter to select · ↑/↓ to navigate · Esc to cancel"; human_codex_screen(), DeferralReason::ComposerChanged { harness: "codex" }, ), + // Located, but an unrecognised footer proves nothing about the screen. This is a wait, + // not a coverage gap, and must not be reported as an unlocatable pane. + ( + idle_codex_screen_with_footer("Esc to interrupt"), + DeferralReason::ComposerUnproven { harness: "codex" }, + ), ( "unrecognized renderer".to_string(), DeferralReason::NoMaintainedComposer,