diff --git a/docs/concepts/overseer-agentic-health-review.md b/docs/concepts/overseer-agentic-health-review.md index c655dc63e..1b7318677 100644 --- a/docs/concepts/overseer-agentic-health-review.md +++ b/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 diff --git a/src/overseer/health_review.rs b/src/overseer/health_review.rs index 90dc00fe3..f1ac9ae48 100644 --- a/src/overseer/health_review.rs +++ b/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 { let mut interventions: Vec = Vec::new(); let mut summary: Option = 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 { +/// 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 { + 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::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 { +/// 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 { + 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}` 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 { diff --git a/tests/gadugi/overseer-health-review.sh b/tests/gadugi/overseer-health-review.sh index 2d85e74e6..122cff975 100755 --- a/tests/gadugi/overseer-health-review.sh +++ b/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' diff --git a/tests/qa-scenarios/overseer-health-review-trailing-text.yaml b/tests/qa-scenarios/overseer-health-review-trailing-text.yaml new file mode 100644 index 000000000..c099df89e --- /dev/null +++ b/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