fix(overseer): tolerate trailing text after a health-review decision's JSON

docs/concepts/overseer-agentic-health-review.md

@@ -11,7 +11,7 @@ description: >
   plain-English operator notification on both channels). Deliberately WITHOUT
   record_step_failure plumbing or an N-identical-failure threshold counter: the
   journal already contains every failure, and an agent reading it sees them all.
-last_updated: 2026-07-21
+last_updated: 2026-07-28
 review_schedule: as-needed
 owner: simard
 doc_type: concept
@@ -126,9 +126,15 @@ caret, or surrounding backticks *before* matching a marker — otherwise a
 well-formed `LAUNCH_RECIPE=` line dressed as `- LAUNCH_RECIPE=…` would be
 silently dropped and a real crash-loop would go un-remediated with no signal
 (the terminal marker still parses, so the degraded-pass ladder never fires).
-Stripping decoration never *invents* a decision: only the three distinctive
-markers are ever acted on, a bulleted line of prose normalises to prose and
-matches nothing, and a malformed-JSON or empty-field decision is still skipped.
+Symmetrically, agents also append a short justification *after* a decision's JSON
+(`LAUNCH_RECIPE={…} — fixes the crash-loop`); because `serde_json` rejects a
+value with trailing non-whitespace, the rail first extracts the leading
+balanced JSON object (respecting braces inside string values) so that trailing
+clause is ignored rather than dropping the whole decision. Neither rail ever
+*invents* a decision: only the three distinctive markers are ever acted on, a
+bulleted line of prose normalises to prose and matches nothing, an
+unbalanced/truncated object is still skipped fail-closed, and a malformed-JSON or
+empty-field decision is still dropped.
 
 ### The verdict is observable, never a silent pass
 

src/overseer/health_review.rs

@@ -207,6 +207,12 @@ pub struct HealthReviewReport {
 ///   rather than silently dropped. This never invents a marker: only the three
 ///   distinctive markers are ever acted on, and a bulleted line of prose still
 ///   matches nothing.
+/// - TRAILING text an agent appends after a decision's JSON object on the same
+///   line (e.g. `LAUNCH_RECIPE={…} — fixes the crash-loop`) is tolerated: the
+///   leading balanced object is extracted ([`extract_leading_json_object`])
+///   before serde parsing. This is the trailing-side sibling of decoration
+///   stripping; it stays fail-closed (an unbalanced/truncated object is still
+///   skipped, never half-parsed).
 pub fn parse_health_review_output(stdout: &str) -> Result<HealthReviewReport, String> {
     let mut interventions: Vec<Intervention> = Vec::new();
     let mut summary: Option<String> = None;
@@ -310,9 +316,66 @@ fn strip_leading_bullet(s: &str) -> Option<&str> {
     None
 }
 
+/// Extract the leading balanced JSON object from a decision-marker payload,
+/// tolerating any TRAILING text an agent appends after it on the same line —
+/// e.g. `{"task_description":"…"} — fixes the crash-loop` or a stray closing
+/// backtick decoration-stripping did not reach. Returns the `{…}` substring, or
+/// `None` when the payload does not START with a `{` or the braces never balance.
+///
+/// It is the trailing-side sibling of [`strip_marker_decoration`] (which removes
+/// LEADING framing): `serde_json::from_str` rejects a value with trailing
+/// non-whitespace, so without this a well-formed decision dressed with a trailing
+/// clause parses to nothing and a real remediation is dropped with no signal —
+/// exactly the silent-drop the decoration fix closed on the leading side.
+///
+/// Fail-closed and mechanical: it never edits the JSON it returns (serde still
+/// validates it), and an UNBALANCED/truncated object yields `None` so the caller
+/// falls back to parsing the whole payload — which serde then rejects — rather
+/// than half-parsing a genuinely broken decision. JSON string contents (including
+/// escaped quotes) are respected so a `}` inside a string value never ends the
+/// object early. When several objects are concatenated it returns the FIRST,
+/// which the caller validates like any single decision.
+fn extract_leading_json_object(payload: &str) -> Option<&str> {
+    let s = payload.trim_start();
+    if !s.starts_with('{') {
+        return None;
+    }
+    let mut depth = 0usize;
+    let mut in_string = false;
+    let mut escaped = false;
+    for (i, c) in s.char_indices() {
+        if in_string {
+            if escaped {
+                escaped = false;
+            } else if c == '\\' {
+                escaped = true;
+            } else if c == '"' {
+                in_string = false;
+            }
+            continue;
+        }
+        match c {
+            '"' => in_string = true,
+            '{' => depth += 1,
+            '}' => {
+                depth -= 1;
+                if depth == 0 {
+                    return Some(&s[..i + c.len_utf8()]);
+                }
+            }
+            _ => {}
+        }
+    }
+    None
+}
+
 /// Parse one `LAUNCH_RECIPE=` JSON payload into an [`Intervention::LaunchRecipe`],
 /// or `None` (logged) when it is malformed or its `task_description` is empty.
-fn parse_launch_decision(json: &str) -> Option<Intervention> {
+/// Trailing text after the JSON object is tolerated via
+/// [`extract_leading_json_object`]; a payload with no balanced object falls
+/// through to the raw string, which serde then rejects (skipped fail-closed).
+fn parse_launch_decision(payload: &str) -> Option<Intervention> {
+    let json = extract_leading_json_object(payload).unwrap_or(payload);
     let decision: LaunchDecision = match serde_json::from_str(json) {
         Ok(d) => d,
         Err(e) => {
@@ -350,8 +413,10 @@ fn parse_launch_decision(json: &str) -> Option<Intervention> {
 /// [`Intervention::EscalateBlockedGoal`], or `None` (logged) when it is
 /// malformed or a required plain-English field is empty. An escalation with an
 /// empty `goal_id`/`problem`/`next_step` would surface a meaningless message to
-/// the operator, so it is dropped fail-closed.
-fn parse_escalate_decision(json: &str) -> Option<Intervention> {
+/// the operator, so it is dropped fail-closed. Trailing text after the JSON
+/// object is tolerated via [`extract_leading_json_object`].
+fn parse_escalate_decision(payload: &str) -> Option<Intervention> {
+    let json = extract_leading_json_object(payload).unwrap_or(payload);
     let decision: EscalateDecision = match serde_json::from_str(json) {
         Ok(d) => d,
         Err(e) => {
@@ -952,6 +1017,132 @@ mod tests {
         assert_eq!(strip_marker_decoration("-notabullet"), "-notabullet");
     }
 
+    // ── trailing-text tolerance (the trailing-side sibling of decoration) ──
+
+    #[test]
+    fn parse_tolerates_trailing_text_after_launch_json() {
+        // Agents routinely append a short justification clause after the JSON.
+        // serde rejects trailing non-whitespace, so without extraction this
+        // well-formed systemic launch would be DROPPED and the crash-loop would
+        // go un-remediated with no signal (the terminal marker still parses).
+        let out = concat!(
+            r#"LAUNCH_RECIPE={"task_description":"fix actor-binding crash-loop (286x)","target_repo":"rysweet/Simard","sequence_group":null} — this addresses the systemic root cause"#,
+            "\nHEALTH_REVIEW_COMPLETE=1 systemic launch\n"
+        );
+        let report = parse_health_review_output(out).expect("parses");
+        assert_eq!(report.interventions.len(), 1);
+        match &report.interventions[0] {
+            Intervention::LaunchRecipe { brief } => {
+                assert!(brief.task_description.contains("actor-binding"));
+                assert!(
+                    !brief.task_description.contains("addresses the systemic"),
+                    "the trailing clause must NOT leak into the parsed brief"
+                );
+            }
+            other => panic!("expected LaunchRecipe, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn parse_tolerates_trailing_text_after_escalate_json() {
+        let out = concat!(
+            r#"ESCALATE_GOAL={"goal_id":"g-42","problem":"The done-gate cannot be measured.","next_step":"Pick a measurable target.","why":"unmeasurable done-gate","reason":"health-review:per-goal","link":null}  (needs a human decision)"#,
+            "\nHEALTH_REVIEW_COMPLETE=1 goal escalated\n"
+        );
+        let report = parse_health_review_output(out).expect("parses");
+        assert_eq!(report.interventions.len(), 1);
+        match &report.interventions[0] {
+            Intervention::EscalateBlockedGoal {
+                goal_id, next_step, ..
+            } => {
+                assert_eq!(goal_id, "g-42");
+                assert_eq!(
+                    next_step, "Pick a measurable target.",
+                    "the trailing clause must NOT leak into the operator-facing next_step"
+                );
+            }
+            other => panic!("expected EscalateBlockedGoal, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn parse_tolerates_leading_decoration_and_trailing_text_together() {
+        // The two robustness rails compose: a bulleted decision line with a
+        // trailing justification clause is still dispatched.
+        let out = concat!(
+            r#"- LAUNCH_RECIPE={"task_description":"bound the parse-failure spike"} because it recurs across goals"#,
+            "\nHEALTH_REVIEW_COMPLETE=1 launch\n"
+        );
+        let report = parse_health_review_output(out).expect("parses");
+        assert_eq!(report.interventions.len(), 1);
+        assert!(matches!(
+            report.interventions[0],
+            Intervention::LaunchRecipe { .. }
+        ));
+    }
+
+    #[test]
+    fn parse_trailing_text_never_ends_json_early_inside_a_string() {
+        // A `}` inside a JSON string value must NOT be mistaken for the object's
+        // close, or the brief would be truncated mid-value.
+        let out = concat!(
+            r#"LAUNCH_RECIPE={"task_description":"fix the `foo() -> Result<T, E>}` mis-binding"} trailing note"#,
+            "\nHEALTH_REVIEW_COMPLETE=1 launch\n"
+        );
+        let report = parse_health_review_output(out).expect("parses");
+        assert_eq!(report.interventions.len(), 1);
+        match &report.interventions[0] {
+            Intervention::LaunchRecipe { brief } => assert!(
+                brief.task_description.ends_with("mis-binding"),
+                "the whole quoted value (incl. its inner braces) is preserved"
+            ),
+            other => panic!("expected LaunchRecipe, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn parse_still_skips_unbalanced_truncated_json_fail_closed() {
+        // An unbalanced object must NOT be half-parsed: extraction returns None,
+        // the raw payload is handed to serde, and serde rejects it → skipped.
+        let out = concat!(
+            r#"LAUNCH_RECIPE={"task_description":"truncated pass"#,
+            "\nHEALTH_REVIEW_COMPLETE=nothing actionable\n"
+        );
+        let report = parse_health_review_output(out).expect("parses");
+        assert!(
+            report.interventions.is_empty(),
+            "a truncated/unbalanced decision is dropped fail-closed, never half-parsed"
+        );
+    }
+
+    #[test]
+    fn extract_leading_json_object_basics() {
+        // Exact object, trailing text, brace-in-string, non-object, unbalanced.
+        assert_eq!(
+            extract_leading_json_object(r#"{"a":1}"#),
+            Some(r#"{"a":1}"#)
+        );
+        assert_eq!(
+            extract_leading_json_object(r#"{"a":1} tail"#),
+            Some(r#"{"a":1}"#)
+        );
+        assert_eq!(
+            extract_leading_json_object(r#"  {"a":{"b":2}} x"#),
+            Some(r#"{"a":{"b":2}}"#)
+        );
+        assert_eq!(
+            extract_leading_json_object(r#"{"a":"}"} x"#),
+            Some(r#"{"a":"}"}"#)
+        );
+        assert_eq!(extract_leading_json_object("not an object"), None);
+        assert_eq!(extract_leading_json_object(r#"{"a":1"#), None);
+        // A concatenation returns the FIRST balanced object; the caller validates.
+        assert_eq!(
+            extract_leading_json_object(r#"{"a":1}{"b":2}"#),
+            Some(r#"{"a":1}"#)
+        );
+    }
+
     // ── reviewer over the injectable seam ─────────────────────────────────
 
     enum Scripted {

tests/gadugi/overseer-health-review.sh

@@ -103,6 +103,17 @@ require_test 'parse_missing_terminal_marker_is_error'
 # Fail-closed: malformed / missing-field decisions are dropped, never fabricated.
 require_test 'parse_skips_malformed_json_but_keeps_valid_decisions'
 require_test 'parse_skips_escalation_missing_plain_english_fields'
+# Robustness: a well-formed decision is DISPATCHED, not silently dropped, when the
+# agent wraps it in ordinary markdown/prose framing — trailing justification text
+# after the JSON object (the trailing-side sibling of the leading-decoration
+# fix) — while an unbalanced/truncated object is still skipped fail-closed and the
+# trailing clause never leaks into the parsed brief / operator-facing text.
+require_test 'parse_tolerates_trailing_text_after_launch_json'
+require_test 'parse_tolerates_trailing_text_after_escalate_json'
+require_test 'parse_tolerates_leading_decoration_and_trailing_text_together'
+require_test 'parse_trailing_text_never_ends_json_early_inside_a_string'
+require_test 'parse_still_skips_unbalanced_truncated_json_fail_closed'
+require_test 'extract_leading_json_object_basics'
 # The rail degrades safely on a runner error / a missing terminal marker.
 require_test 'review_degrades_to_empty_on_runner_error'
 require_test 'review_degrades_to_empty_on_missing_terminal_marker'

tests/qa-scenarios/overseer-health-review-trailing-text.yaml

@@ -0,0 +1,68 @@
+name: overseer-health-review-trailing-text
+app_type: cli
+description: |
+  Outside-in proof that the Overseer's agentic HEALTH-REVIEW rail dispatches a
+  well-formed remediation decision even when the agent appends a short
+  justification clause AFTER the decision's JSON on the same line — instead of
+  silently dropping it.
+
+  Why this matters (the standing self-heal goal): the health-review recipe reads
+  the journal, reasons to a small set of typed DECISIONS
+  (`LAUNCH_RECIPE=`/`ESCALATE_GOAL=`/`HEALTH_REVIEW_COMPLETE=`), and the thin
+  Rust rail (`src/overseer/health_review.rs`) parses those markers onto the SAME
+  gated capability path every other fix uses. A prior hardening made the parser
+  tolerant of LEADING markdown decoration (a `- `/`>`/backtick wrapper), but the
+  JSON was still handed to `serde_json::from_str`, which REJECTS any TRAILING
+  non-whitespace after the object. So a decision an agent wrote as
+  `LAUNCH_RECIPE={...} — fixes the crash-loop` parsed to ZERO interventions while
+  the terminal marker still parsed "successfully" — the degraded-pass escalation
+  ladder never fired and a real crash-loop's remediation was lost with no signal.
+  That directly defeats "DRIVE remediation through EXISTING capabilities".
+
+  This scenario drives the hermetic, in-process unit tests that pin the
+  corrected, fail-closed contract:
+    - a LAUNCH_RECIPE / ESCALATE_GOAL line with trailing text after its JSON is
+      still parsed and dispatched, and the trailing clause never leaks into the
+      parsed brief / operator-facing text;
+    - leading decoration and trailing text compose (a bulleted decision line with
+      a trailing clause is dispatched);
+    - a `}` inside a JSON string value never ends the object early;
+    - an unbalanced / truncated object is still skipped fail-closed (never
+      half-parsed), and the balanced-object extractor's basics hold.
+  It also re-runs the full overseer suite to prove the existing health-review
+  parse / decoration / ladder / dispatch / gate contract is unregressed. No
+  network, no live host mutation. The CLI runner rejects any non-zero exit.
+agents:
+  - name: simard-cli
+    type: cli
+    command: cargo
+
+steps:
+  # The trailing-text-tolerant marker-parse contract in isolation.
+  - action: run
+    params:
+      command: "cargo test --locked --lib overseer::health_review::tests"
+    timeout: 600000
+  - action: wait_for_output
+    params:
+      value: "test result: ok"
+    timeout: 8000
+  - action: validate_exit_code
+    params:
+      value: "0"
+    timeout: 5000
+
+  # The full overseer suite proves the health-review rail (parse, decoration,
+  # escalation ladder, dispatch, gate) did not regress under the new extractor.
+  - action: run
+    params:
+      command: "cargo test --locked --lib overseer::"
+    timeout: 600000
+  - action: wait_for_output
+    params:
+      value: "test result: ok"
+    timeout: 8000
+  - action: validate_exit_code
+    params:
+      value: "0"
+    timeout: 5000