diff --git a/src/openhuman/flows/ops.rs b/src/openhuman/flows/ops.rs index 4ee914ab64..2075d20722 100644 --- a/src/openhuman/flows/ops.rs +++ b/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(""), 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 { + 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 = 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 = 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 diff --git a/src/openhuman/flows/ops_tests.rs b/src/openhuman/flows/ops_tests.rs index 5744046515..09a9e9db5d 100644 --- a/src/openhuman/flows/ops_tests.rs +++ b/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); +}