fix(flows): trail-off backstop no longer clobbers real questions#4956
Conversation
…al-paragraph detection + non-destructive fallback) PR tinyhumansai#4887's flows_build terminal-state backstop replaced the agent's final message with a generic fallback whenever text_looks_like_question() false-negatived. That detector only checked for a trailing `?` at the very end of the text / last line, so it missed the extremely common LLM pattern "What's X? You can find it at Y." — a real question immediately followed by a trailing instructional sentence. A live tester's agent asked a specific, answerable question in exactly this shape and had it clobbered by the generic fallback three times in a row. Two changes: - text_looks_like_question now also scans the last non-blank paragraph for a `?` outside inline code / a fenced code block, catching the mid-sentence + trailing-instruction shape without false-positiving on code samples. - The backstop's fallback is no longer destructive: when it fires, the guaranteed fallback question is prepended to the model's original text (divided by a `---`) instead of discarding it, so even a future false-negative can never destroy the model's words — only add a guaranteed question on top. A genuinely empty/whitespace-only original still yields the bare fallback (no empty divider).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2dc2cc9e0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for ch in text.chars() { | ||
| match ch { | ||
| '`' => backtick_count += 1, | ||
| '?' if backtick_count % 2 == 0 => return true, |
There was a problem hiding this comment.
When the builder's final paragraph is a status/update containing a non-question ? outside backticks, such as a URL with a query string (Checked https://api.example/search?q=foo and got 403.), this branch returns true solely because the character is outside code. flows_build then skips combine_trail_off_fallback, so the terminal-state backstop can still leave the user with a non-answerable status note and no proposal/question.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in c09ec76: each candidate ? outside code now also has to be sentence-terminal (whitespace/EOS after it, skipping closing quote/bracket punctuation) via a new is_sentence_terminal_question_mark helper, so a mid-token ? like a URL query string no longer flips text_looks_like_question to true. Added a regression test (text_looks_like_question_ignores_question_mark_in_url_query_string) covering your exact example plus a bare-? query-param case.
…on (addresses @chatgpt-codex-connector on src/openhuman/flows/ops.rs:4486) The final-paragraph scan added for the mid-sentence question shape counted ANY `?` outside code as a question, including one embedded mid-token (a URL query string like `search?q=foo`, or similar non-prose use). That made text_looks_like_question() false-positive on status dumps like "Checked https://api.example/search?q=foo and got 403.", which skips combine_trail_off_fallback() entirely and leaves the user with an unanswerable status note — the exact failure mode this backstop exists to prevent. Require each candidate `?` to be sentence-terminal (followed by whitespace/EOS, skipping over closing quote/bracket punctuation) before it counts. Also fixes the CI clippy failure on this line (manual_is_multiple_of) that landed in the same match arm.
…scope - Rename text_looks_like_question_accepts_false_negative_on_trailing_pleasantry to text_looks_like_question_detects_same_paragraph_trailing_pleasantry — the old name described the pre-fix behavior (an accepted false negative); post-fix this shape is correctly detected. - Add a companion test pinning the still-intentional cross-paragraph false negative (a `?` in an earlier paragraph than the last one is not detected, by design — the non-destructive fallback makes this safe).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTrail-off handling now preserves non-question assistant text alongside generated fallback questions. Question detection scans terminal paragraphs while ignoring code and URL query marks, with expanded regression coverage. ChangesTrail-off fallback handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/flows/ops_tests.rs`:
- Around line 4960-4975: The test
combine_trail_off_fallback_preserves_original_text_on_genuine_non_question
currently verifies presence but not ordering or separation. Replace the two
contains assertions with an exact equality assertion for the documented
fallback-first combined format, including the required divider and both fallback
and original texts; retain the question-invariant assertion.
In `@src/openhuman/flows/ops.rs`:
- Around line 4466-4470: Update the paragraph detection around
question_mark_outside_code to split text using lines() and blank-line
boundaries, treating whitespace-only separators and CRLF input as separate
paragraphs. Preserve the reverse search for the final non-empty paragraph so
only a terminal question satisfies the guarantee, and add regressions covering
whitespace-only and CRLF paragraph separators.
- Around line 4491-4504: Update question_mark_outside_code to track Markdown
backtick delimiter runs and their lengths, rather than treating any even
backtick count as outside code. Match closing runs to the active opening fence
length so embedded backticks and double-backtick spans remain inside code; add
tests covering double-backtick fences and embedded backticks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38c97147-7dd3-4900-9df0-46d5179ef5a3
📒 Files selected for processing (2)
src/openhuman/flows/ops.rssrc/openhuman/flows/ops_tests.rs
…esses @coderabbitai on ops.rs:4470, ops.rs:4504) Two correctness fixes to the trail-off question detector from the last commit, both flagged by CodeRabbit: - Final-paragraph splitting used the literal "\n\n" byte sequence, which mishandles CRLF text ("question?\r\n\r\nstatus") and whitespace-only blank-line separators ("question?\n \nstatus") — both get treated as ONE paragraph, so an earlier question can leak into and suppress the fallback for a later, non-question status paragraph. Replaced with a new `last_paragraph` helper that walks `str::lines()` (which normalizes CRLF) and treats any all-whitespace line as a paragraph break. - `question_mark_outside_code` tracked a running per-character backtick COUNT and used its parity to decide "inside/outside a span." That misclassifies any code span whose delimiter is more than one backtick (e.g. ``SELECT ? FROM t`` opens with a 2-backtick run, count 0->2 is even, so it looks "outside" again immediately) — the `?` inside a valid double-backtick span was wrongly counted as a real question mark. Rewrote to track backtick delimiter RUN LENGTH per CommonMark's code-span rule: a run of N backticks opens a span that only the next run of exactly N backticks closes; mismatched runs are literal characters within the span.
… assertion - Assert the exact combined string in combine_trail_off_fallback_preserves_original_text_on_genuine_non_question instead of two `contains` checks, so the test actually pins the documented fallback-first ordering and `---` divider (addresses @coderabbitai on ops_tests.rs:4975). - Add text_looks_like_question_treats_crlf_and_whitespace_lines_as_paragraph_breaks covering both CRLF and whitespace-only paragraph separators (addresses @coderabbitai on ops.rs:4470). - Add text_looks_like_question_ignores_question_mark_inside_double_backtick_span covering a double-backtick span and one with an embedded single backtick (addresses @coderabbitai on ops.rs:4504).
1 participant
Summary
flows_buildterminal-state backstop from fix(flows): guarantee a terminal state on every flows_build turn (no silent trail-off) #4887 was clobbering the agent's real, specific question with a generic fallback whenevertext_looks_like_questionfalse-negatived — which it did on an extremely common LLM output shape.Problem
text_looks_like_question(insrc/openhuman/flows/ops.rs) only detected a?at the very end of the text, or on the last non-blank line. A real tester's agent asked:Here the
?sits mid-sentence, immediately followed by a trailing instructional sentence on the same paragraph — a shape LLMs produce constantly ("What's X? You can find it at Y."). The detector missed it, so the trail-off backstop atflows_build(~ops.rs:4368) fired and replaced the agent's specific 538-char question with a generic 146-char fallback ("I wasn't able to finish building this workflow... Could you tell me how you'd like me to resolve that...").Solution
Two changes:
1. Loosen
text_looks_like_question— after the existing terminal-?/ last-line checks fail, scan the LAST non-empty paragraph (split on a blank line) for a?that is NOT inside inline code or a fenced code block. Added a smallquestion_mark_outside_codehelper that tracks a running backtick count and treats a?as "outside code" when the count so far is even (this correctly handles both single-backtick inline spans and triple-backtick fences, since either toggles the cumulative parity by an odd amount at open and close).While fixing this I found a related, subtler bug in the existing trailing-noise strip:
trim_end_matches([...,'',...])was stripping a trailing backtick unconditionally, which could peel the closing delimiter off a code span whose last character was?(e.g. ``id = ?`` at the very end of the text), exposing that?` as if it were a bare trailing question mark and defeating the new code guard entirely. Fixed by excluding the backtick from that noise-trim set — the function doc now explains why.2. Make the fallback non-destructive. Even with the loosened detector, a future false negative is always possible — the fix should make that failure mode harmless, not just rarer. When the backstop fires, it now prepends the fallback question to the model's original text (
format!("{fallback}\n\n---\n\n{original}")) via a newcombine_trail_off_fallbackhelper, instead of discarding the original withassistant_text = fallback.I chose PREPEND over a REPLACE-with-append because the fallback is the actionable, guaranteed-to-be-a-question part the UI/user needs first; the model's own words are still fully preserved below a
---divider (renders as a horizontal rule in the chat markdown) for context, rather than being silently dropped. If the original text is empty/whitespace-only (a genuine silent turn — nothing to preserve),combine_trail_off_fallbackreturns the bare fallback rather than prepending an empty divider.Together: a real question is (almost) never misdetected as non-question (Change 1), and even if a future edge case still misfires, the model's message is never destroyed (Change 2) — only added to.
Testing
GGML_NATIVE=OFF cargo check— clean.cargo test --lib openhuman::flows::— 391 passed, 0 failed.cargo fmt --check— clean.New/updated tests in
src/openhuman/flows/ops_tests.rs:text_looks_like_question_accepts_false_negative_on_trailing_pleasantry— updated to assert the trailing-pleasantry/instruction pattern (same paragraph, e.g."...to? Let me know!") is now DETECTED (true). Documents that a pleasantry in a separate paragraph ("...to?\n\nLet me know!") remains an intentional, documented false negative — the scan only looks at the last non-blank paragraph by design.text_looks_like_question_detects_mid_sentence_question_with_trailing_instruction(new) — the tester's exact shape, assertstrue.text_looks_like_question_ignores_question_mark_inside_code(new) — a?only inside inline code or a fenced code block is NOT treated as a question; covers both the inline-code and fenced-code-block form of the trailing-backtick edge case found while fixing this.text_looks_like_question_rejects_status_dumps_and_silence(existing, unchanged) — a genuine no-question status dump still returnsfalse, so the backstop still fires for real silence.combine_trail_off_fallback_preserves_original_text_on_genuine_non_question(new) — asserts the model's original text survives in the combined output when the backstop fires.combine_trail_off_fallback_returns_fallback_alone_for_genuine_silence(new) — no empty---divider prepended when there's nothing to preserve.build_trail_off_fallbacktests remain green, unmodified.Acceptance
text_looks_like_questiondetects the tester's exact regression shape (mid-sentence?+ trailing instruction, same paragraph)?inside inline code / a fenced code block is still correctly ignored?still returnsfalse(backstop still fires for real silence)cargo check,cargo test --lib openhuman::flows::, andcargo fmt --checkall cleanSummary by CodeRabbit
Bug Fixes
Tests