fix(flows): trail-off backstop no longer clobbers real questions (#4956)

src/openhuman/flows/ops.rs

@@ -4367,15 +4367,17 @@ pub async fn flows_build(
     let trail_off = !capped && proposal.is_none() && run_error.is_none();
     let assistant_text = if trail_off && !text_looks_like_question(&assistant_text) {
         let fallback = build_trail_off_fallback(agent.history());
+        let combined = combine_trail_off_fallback(&fallback, &assistant_text);
         tracing::warn!(
             target: "flows",
             flow_id = req.flow_id.as_deref().unwrap_or("<none>"),
             original_len = assistant_text.len(),
             fallback_len = fallback.len(),
+            combined_len = combined.len(),
             "[flows] flows_build: trail-off detected (no proposal, no cap, no question) — \
-             guaranteeing a fallback question instead of silence"
+             guaranteeing a fallback question while preserving the model's original text"
         );
-        fallback
+        combined
     } else {
         assistant_text
     };
@@ -4414,15 +4416,31 @@ pub async fn flows_build(
     ))
 }
 
-/// Heuristic: does `text` already end with a clear, answerable question?
-/// Conservative by design (issue: builder convergence) — a false negative (an
-/// actual question this misses) just wraps it in the trail-off fallback,
-/// which still includes the blocker context, so the safe failure mode is
-/// "over-wrap", never "under-detect and stay silent".
+/// Heuristic: does `text` already contain a clear, answerable question in its
+/// final paragraph? Conservative by design (issue: builder convergence) — a
+/// false negative (an actual question this misses) no longer discards the
+/// model's text (see `combine_trail_off_fallback`), so the safe failure mode
+/// stays "add a guaranteed question on top", never "under-detect and stay
+/// silent".
+///
+/// Regression (#4887 follow-up): the original version only checked for a `?`
+/// at the very end of the text / last line, which false-negatived on the
+/// extremely common LLM pattern "What's X? You can find it at Y." — a real
+/// question immediately followed by a trailing instructional sentence. The
+/// backstop then clobbered a specific, answerable question with a generic
+/// fallback. To catch that shape, this now also scans the LAST non-empty
+/// paragraph for a `?` that isn't inside inline code or a fenced code block
+/// (so a literal `?` in a code sample, e.g. `WHERE id = ?`, doesn't count).
+///
+/// Note: the trailing-noise strip below deliberately does NOT include the
+/// backtick. Stripping a trailing backtick would peel off the CLOSING
+/// delimiter of a code span whose last character is `?` (e.g. `` `id = ?` ``
+/// at the very end of the text), exposing that `?` as if it were a bare
+/// trailing question mark and defeating the code guard entirely.
 fn text_looks_like_question(text: &str) -> bool {
     let trimmed = text
         .trim()
-        .trim_end_matches(['"', '\'', ')', ']', '*', '_', '`', '.'])
+        .trim_end_matches(['"', '\'', ')', ']', '*', '_', '.'])
         .trim_end();
     if trimmed.is_empty() {
         return false;
@@ -4432,15 +4450,132 @@ fn text_looks_like_question(text: &str) -> bool {
     }
     // The question may not be the literal last character (trailing markdown
     // like a closing code fence or list marker on its own line) — fall back
-    // to the last non-blank line. This does NOT catch a question followed by
-    // a further trailing sentence ("...channel?\n\nLet me know!") — that's
-    // an accepted false negative: the turn still ends in a real (if
-    // over-eagerly replaced) question, never in silence, which is the
-    // invariant this function exists to protect.
-    trimmed
+    // to the last non-blank line.
+    if trimmed
         .lines()
         .rfind(|line| !line.trim().is_empty())
         .is_some_and(|last_line| last_line.trim_end().ends_with('?'))
+    {
+        return true;
+    }
+    // Final-paragraph scan: a question can sit mid-paragraph, followed by a
+    // further trailing sentence on the SAME line/paragraph ("...ID? You can
+    // find it under Profile > Copy member ID."). Take the last non-blank
+    // paragraph and accept it if it contains a `?` that isn't inside inline
+    // code / a code fence.
+    last_paragraph(trimmed)
+        .as_deref()
+        .is_some_and(question_mark_outside_code)
+}
+
+/// Returns the last non-blank paragraph of `text` — a maximal run of
+/// consecutive non-blank lines, working backward from the end and skipping
+/// any trailing blank lines first. `None` if `text` has no non-blank lines.
+///
+/// CodeRabbit review follow-up: this used to split on the literal `"\n\n"`
+/// byte sequence, which mishandles two real shapes:
+/// - **CRLF input** (`"question?\r\n\r\nstatus"`): the separator is
+///   `"\r\n\r\n"`, not `"\n\n"`, so the whole text was treated as ONE
+///   paragraph — an earlier question could then suppress the fallback for a
+///   trailing non-question status paragraph.
+/// - **Whitespace-only separator lines** (`"question?\n \nstatus"` — a blank
+///   line that isn't perfectly empty): same failure, same reason.
+///
+/// Working line-by-line via [`str::lines`] (which normalizes CRLF) and
+/// treating any all-whitespace line as blank fixes both.
+fn last_paragraph(text: &str) -> Option<String> {
+    let mut collected: Vec<&str> = Vec::new();
+    for line in text.lines().rev() {
+        if line.trim().is_empty() {
+            if collected.is_empty() {
+                continue; // still skipping trailing blank lines
+            }
+            break; // blank line marks the start of the paragraph above
+        }
+        collected.push(line);
+    }
+    if collected.is_empty() {
+        return None;
+    }
+    collected.reverse();
+    Some(collected.join("\n"))
+}
+
+/// Does `text` contain at least one *sentence-terminal* `?` that isn't
+/// inside a backtick-delimited code span (inline code like `` `U...` `` or a
+/// fenced block like `` ``` ``)? Follows the CommonMark code-span rule: a
+/// *run* of one or more consecutive backticks opens a span, and that span is
+/// closed only by the next run of the SAME length — a shorter or longer run
+/// of backticks encountered while inside a span is just literal backtick
+/// characters, not a delimiter.
+///
+/// CodeRabbit review follow-up: an earlier version tracked a running
+/// per-character backtick COUNT and used its parity (even = outside code).
+/// That misclassifies any multi-backtick span whose delimiter is more than
+/// one backtick — e.g. ``` ``SELECT ? FROM t`` ``` opens with a 2-backtick
+/// run (count 0→2, even → looks "outside" again immediately), so the `?`
+/// inside a valid double-backtick span was wrongly treated as outside code.
+/// Tracking delimiter run LENGTH (not raw backtick count) fixes this while
+/// still handling the common single-backtick and triple-backtick-fence
+/// cases, since those are just the run-length-1 and run-length-3 instances
+/// of the same rule.
+///
+/// Codex review follow-up: a bare `?` outside code isn't necessarily a real
+/// question — a status line like "Checked https://api.example/search?q=foo
+/// and got 403." has one mid-token, in a URL query string. Counting that
+/// would flip `text_looks_like_question` to `true` and skip
+/// `combine_trail_off_fallback` entirely, leaving the user with an
+/// unanswerable status note — exactly the failure mode this backstop exists
+/// to prevent. So each candidate `?` is additionally required to be
+/// sentence-terminal via [`is_sentence_terminal_question_mark`].
+fn question_mark_outside_code(text: &str) -> bool {
+    let chars: Vec<char> = text.chars().collect();
+    // `Some(n)` while scanning is inside a code span opened by a run of `n`
+    // backticks; that span closes only on the next run of exactly `n`.
+    let mut open_run_len: Option<usize> = None;
+    let mut i = 0;
+    while i < chars.len() {
+        if chars[i] == '`' {
+            let start = i;
+            while i < chars.len() && chars[i] == '`' {
+                i += 1;
+            }
+            let run_len = i - start;
+            open_run_len = match open_run_len {
+                None => Some(run_len),
+                Some(n) if n == run_len => None,
+                Some(n) => Some(n), // mismatched run length: still inside the span
+            };
+            continue;
+        }
+        if chars[i] == '?'
+            && open_run_len.is_none()
+            && is_sentence_terminal_question_mark(&chars, i)
+        {
+            return true;
+        }
+        i += 1;
+    }
+    false
+}
+
+/// Is the `?` at `chars[index]` sentence-terminal — i.e. does it read as an
+/// actual question mark rather than a character that merely happens to be a
+/// `?` mid-token (a URL query string like `search?q=foo`, a shell glob,
+/// etc.)? Skips over any immediately-following closing quote/bracket
+/// punctuation (`"`, `'`, right single/double quotes, `)`, `]`) and requires
+/// what remains to be whitespace or the end of the text — the shape a `?`
+/// takes at the end of a real sentence or clause.
+fn is_sentence_terminal_question_mark(chars: &[char], index: usize) -> bool {
+    let mut i = index + 1;
+    while let Some(&c) = chars.get(i) {
+        if matches!(c, '"' | '\'' | '\u{2019}' | '\u{201D}' | ')' | ']') {
+            i += 1;
+            continue;
+        }
+        return c.is_whitespace();
+    }
+    true // '?' was the last character in the paragraph.
 }
 
 /// Builder-authoring tools whose result body can explain a trail-off — the
@@ -4480,6 +4615,24 @@ fn build_trail_off_fallback(
     }
 }
 
+/// Combines the guaranteed trail-off `fallback` question with the model's own
+/// `original` text instead of discarding it (#4887 follow-up, Change 2). Even
+/// after loosening `text_looks_like_question`, a future false negative must
+/// never destroy the model's words — it should only ever ADD the guaranteed
+/// question on top. The `fallback` is prepended (so the user sees the
+/// actionable question first) and the original is kept below a divider for
+/// context. When `original` is empty/whitespace-only (a genuine silent
+/// turn — there's nothing to preserve), returns the fallback alone rather
+/// than prepending an empty divider.
+fn combine_trail_off_fallback(fallback: &str, original: &str) -> String {
+    let trimmed_original = original.trim();
+    if trimmed_original.is_empty() {
+        fallback.to_string()
+    } else {
+        format!("{fallback}\n\n---\n\n{trimmed_original}")
+    }
+}
+
 /// Scans `history` in reverse for the last result from a
 /// [`TRAIL_OFF_BLOCKER_TOOLS`] call that reads as a failure — a plain-text
 /// error message (gate rejection), or a JSON body with `"ok": false` — and

src/openhuman/flows/ops_tests.rs

@@ -4751,18 +4751,120 @@ fn text_looks_like_question_detects_trailing_question_mark() {
     ));
 }
 
-/// A question followed by a further trailing sentence on its own line
-/// ("...channel?\n\nLet me know!") is an accepted false negative — the
-/// heuristic is deliberately conservative (see the function doc). Pin that
-/// this case is NOT detected so a future "improvement" doesn't silently
-/// change the accepted trade-off without a matching design review.
+/// Regression (#4887 follow-up): a question immediately followed by a
+/// trailing pleasantry/instruction in the SAME paragraph ("...to? Let me
+/// know!") used to be an accepted false negative. That false negative let the
+/// trail-off backstop clobber real, specific questions with a generic
+/// fallback — this is now DETECTED via the final-paragraph scan in
+/// `text_looks_like_question`.
+///
+/// Note: a question mark separated from the trailing sentence by a full
+/// blank-line paragraph break (`"...to?\n\nLet me know!"`) is a DIFFERENT
+/// shape — the `?` there sits in an earlier paragraph, not the last one — and
+/// remains an intentional false negative: the final-paragraph scan only
+/// looks at the LAST non-blank paragraph, by design (see the function doc
+/// and `text_looks_like_question_ignores_question_mark_in_earlier_paragraph`
+/// below, which pins that scope decision).
 #[test]
-fn text_looks_like_question_accepts_false_negative_on_trailing_pleasantry() {
+fn text_looks_like_question_detects_same_paragraph_trailing_pleasantry() {
+    assert!(text_looks_like_question(
+        "Which channel should I post to? Let me know!"
+    ));
+}
+
+/// Pins the intentional cross-paragraph false negative documented above: a
+/// `?` that sits in an EARLIER paragraph than the last one is deliberately
+/// NOT detected — the final-paragraph scan only looks at the last non-blank
+/// paragraph, by design. This is harmless because the trail-off backstop's
+/// fallback is non-destructive (PREPEND, not REPLACE): even when this false
+/// negative fires, the model's original question is preserved below the
+/// fallback rather than discarded.
+#[test]
+fn text_looks_like_question_ignores_question_mark_in_earlier_paragraph() {
     assert!(!text_looks_like_question(
         "Which channel should I post to?\n\nLet me know!"
     ));
 }
 
+/// The exact shape a live tester hit (#4887 regression): a clear, specific
+/// question mid-sentence, immediately followed by a trailing instructional
+/// sentence on the SAME paragraph/line. The old last-line-only check missed
+/// this entirely; the final-paragraph scan must catch it.
+#[test]
+fn text_looks_like_question_detects_mid_sentence_question_with_trailing_instruction() {
+    assert!(text_looks_like_question(
+        "Alan — what's your **Slack user ID** (the `U...` code) so I can DM you the daily \
+         update? You can find it in Slack under Profile > Copy member ID."
+    ));
+}
+
+/// A `?` that only appears inside inline code or a fenced code block must
+/// NOT be treated as a question — the guard on `question_mark_outside_code`
+/// has to hold, or a code sample like `WHERE id = ?` would false-positive.
+#[test]
+fn text_looks_like_question_ignores_question_mark_inside_code() {
+    assert!(!text_looks_like_question(
+        "Run the query below to check the row.\n\n`SELECT * FROM t WHERE id = ?`"
+    ));
+    assert!(!text_looks_like_question(
+        "Here's the query:\n\n```sql\nSELECT * FROM t WHERE id = ?\n```"
+    ));
+}
+
+/// Codex review follow-up: a `?` mid-token that isn't a real question mark —
+/// e.g. a URL query string in a status update — must NOT flip
+/// `text_looks_like_question` to `true`. Counting it would make `flows_build`
+/// skip `combine_trail_off_fallback` entirely, leaving the user with an
+/// unanswerable status note and no guaranteed question — exactly the failure
+/// mode this backstop exists to prevent.
+#[test]
+fn text_looks_like_question_ignores_question_mark_in_url_query_string() {
+    assert!(!text_looks_like_question(
+        "Checked https://api.example/search?q=foo and got 403."
+    ));
+    assert!(!text_looks_like_question(
+        "Ran the search with filter?status=open but the API rejected it."
+    ));
+}
+
+/// CodeRabbit review follow-up: paragraph boundaries must be recognized for
+/// CRLF line endings and whitespace-only blank lines, not just a literal
+/// `"\n\n"` byte sequence — otherwise an earlier question survives into what
+/// should be treated as a separate, later, non-question status paragraph,
+/// and the fallback gets wrongly suppressed for that trailing paragraph.
+#[test]
+fn text_looks_like_question_treats_crlf_and_whitespace_lines_as_paragraph_breaks() {
+    // CRLF paragraph break: the earlier "?" must not leak into the final
+    // paragraph, which is a plain status line with no question of its own.
+    assert!(!text_looks_like_question(
+        "Which channel should I post to?\r\n\r\nPosted the update just now."
+    ));
+    // Whitespace-only blank line (not perfectly empty) must also count as a
+    // paragraph break.
+    assert!(!text_looks_like_question(
+        "Which channel should I post to?\n   \nPosted the update just now."
+    ));
+}
+
+/// CodeRabbit review follow-up: a multi-backtick Markdown code span (e.g.
+/// double backtick, used so the span can itself contain a literal single
+/// backtick) must still be recognized as code — a naive backtick-count
+/// parity check misclassifies it because two backticks flip parity back to
+/// "even" immediately. The span must only close on a run of the SAME length
+/// that opened it.
+#[test]
+fn text_looks_like_question_ignores_question_mark_inside_double_backtick_span() {
+    assert!(!text_looks_like_question(
+        "Run the query below to check the row.\n\n``SELECT * FROM t WHERE id = ?``"
+    ));
+    // A single backtick embedded inside a double-backtick span (the classic
+    // reason to use a longer delimiter) must not be mistaken for the span's
+    // closing delimiter.
+    assert!(!text_looks_like_question(
+        "Use ``SELECT `id` FROM t WHERE id = ?`` before retrying."
+    ));
+}
+
 #[test]
 fn text_looks_like_question_rejects_status_dumps_and_silence() {
     assert!(!text_looks_like_question(
@@ -4887,3 +4989,33 @@ fn build_trail_off_fallback_does_not_resurface_a_resolved_blocker() {
     );
     assert!(text_looks_like_question(&fallback));
 }
+
+/// Change 2 of the #4887 regression fix: when the trail-off backstop fires on
+/// a genuine non-question (a status dump), the model's original words must
+/// still be present in the combined output — the fallback question is added
+/// on top, never a replacement.
+#[test]
+fn combine_trail_off_fallback_preserves_original_text_on_genuine_non_question() {
+    let original = "## Done so far\n- Checked connections\n- Verified contracts";
+    let fallback = build_trail_off_fallback(&[]);
+    let combined = combine_trail_off_fallback(&fallback, original);
+    // Assert the exact combined string, not just that both pieces appear
+    // somewhere — this pins the documented fallback-first ordering and the
+    // `---` divider, which a looser `contains`-based check wouldn't catch a
+    // regression in (e.g. original-first ordering, or a missing divider).
+    assert_eq!(combined, format!("{fallback}\n\n---\n\n{original}"));
+    // The combined text still ends in the model's original (non-question)
+    // words, so the "is this a question" invariant applies to the
+    // fallback alone, not the full combined string.
+    assert!(text_looks_like_question(&fallback));
+}
+
+/// Guards against prepending an empty divider when the original text is a
+/// genuine silent turn (empty/whitespace-only) — there is nothing to
+/// preserve, so the combined output should just be the fallback.
+#[test]
+fn combine_trail_off_fallback_returns_fallback_alone_for_genuine_silence() {
+    let fallback = build_trail_off_fallback(&[]);
+    assert_eq!(combine_trail_off_fallback(&fallback, ""), fallback);
+    assert_eq!(combine_trail_off_fallback(&fallback, "   \n\n  "), fallback);
+}