fix(mosaico): route on the latest user turn of a forwarded conversation#2561
Conversation
The MOSAICO reference agent forwards the WHOLE conversation to a solution agent,
one A2A text part per turn shaped "{role}: {content}", which the A2A SDK joins
with newlines. dispatch.py treated that blob as a single fresh request, which
caused four routing defects with one shared root cause: "latest user intent"
was never modelled.
- _extract_diff took the first fenced diff, so a conversation containing a
corrected re-paste was answered against the superseded patch. This is the
worst of the four: the reply is confident and well formed, just about the
wrong code, and nothing signals it.
- The verb was picked by _VALID_VERBS tuple order, so the first entry present
anywhere in the text won. Even single-turn, "now describe it, do not review"
resolved to review. Forwarded history made it near-certain: pr-agent's own
output carries "Estimated effort to review:", which matches the verb pattern
(the "PR Reviewer Guide" heading does not -- review\b does not match
"reviewer" -- so that heading lands on review via the default instead).
- _diff_prose kept only the text before the first raw diff marker, discarding
everything after it, so a request written after a pasted unfenced patch never
reached the matcher at all.
- A PR URL was looked for across the whole blob before any diff, so a URL that
cannot be fetched -- a private repo as much as a dead link -- failed the task
outright and silently discarded a good diff supplied in the current turn.
Turns are now split on role-label lines (conservatively: the text must open with
a label and carry at least two, so single-turn requests are untouched, and a
turn's content stays multi-line). The verb is resolved from the latest user turn
and, within it, by position in the text rather than tuple order, skipping negated
occurrences; agent turns are never consulted for intent. _diff_prose now excises
the patch body and keeps the prose on both sides. The diff/PR to act on is taken
from the newest turn that supplies one -- agent turns included, since a coding
agent may be the one that produced the patch -- so a fresh inline diff beats an
unfetchable URL from history. Within a turn, the last diff and the existing
URL-over-diff precedence both hold.
Splitting turns also un-corrupts a raw diff that opens a forwarded turn: its
"diff --git" header used to arrive prefixed with "user: ", so parse_unified_diff
found no files and the run returned the empty fallback.
Code Review by Qodo
1.
|
PR Summary by QodoFix MOSAICO routing to use latest user turn in forwarded conversations
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Flagged by CodeQL on this PR. The import was already unused before this change; reflowing the import block moved it to a new line, which surfaced it as a new alert.
Code Review by Qodo
1. Diff prose stripping incomplete
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
_DIFF_BODY_LINE_RE did not cover the extended header git writes between 'diff --git' and the first hunk (new/deleted file mode, rename/copy from/to, similarity index, binary). The first such line ended body mode, so the index/---/+++ lines after it leaked into the prose that picks the verb, letting the PATCH override what the user asked for: a patch renaming to improve.py read as an explicit /improve and beat the user's own 'review this' on position, and a '?' in a leaked path routed to /ask. Only the whitelist is widened. Body mode still ends at the first line that is not part of the patch, because that is what preserves a request written AFTER the pasted patch. _ROLE_LINE_RE now consumes all whitespace after the colon rather than a single space, so a turn whose own content starts with a space no longer leaves 'diff --git' indented, which made parse_unified_diff return [].
|
Thanks Qodo. Both findings had a real core, so both are fixed in 1e18059. One piece of the suggested remediation is deliberately not taken, and one part of finding 2's reasoning does not hold up. Details below. Finding 1, "Diff prose stripping incomplete" — ACCEPTEDConfirmed, and it is worse than "potentially flipping routing". It flips routing two ways, reproduced against the branch:
The first is the nastier one: Not taken: "consider keeping Finding 2, "Indented role diff missed" — ACCEPTED, with a correctionThe conclusion is right and it is fixed (
Detection is not prevented. Verification
|
Thanks for the detailed validation. The corrections are clear: finding 1’s termination behavior should remain unchanged to preserve trailing prose, and finding 2 affects parsing rather than diff detection. The fixes and regression coverage in commit |
|
Code review by qodo was updated up to the latest commit 1e18059 |
|
Findings 1 and 4 are confirmed resolved in 2. "Negated questions misrouted" — REJECTEDThe premise is that
The suggested direction is to drop
So the proposed fix regresses One real imprecision this does leave: 3. "False blob splitting" — REJECTEDI could not make this change any outcome for the input class cited. Config shaped text has no diff and no PR URL, so it always lands on path (c), whose guidance is verb independent: → splits as claimed, and returns The only shape where it does diverge additionally requires a pasted diff and a verb placed after the → verb Against that, tightening detection risks failing to split a real forwarded blob, which is the production path and the entire reason this PR exists: every multi-turn conversation would go back to the five defects in the description. There is also no robust discriminator available. A real blob is exactly The cost asymmetry decides it: a false split costs a verb on contrived input, a missed split costs correctness on every real conversation. Detection stays conservative. No code change for either, so CI remains green on |
Drops development archaeology (defect numbers, 'used to', how the behaviour came to be), narration of what the code plainly does, and test docstrings that only restate the test name. Keeps, and tightens, the parts whose absence would invite a wrong change: the forwarded-conversation format, why blob detection is conservative, that the production role label is 'agent' rather than 'assistant', that only a turn's first line carries the label, why verbs resolve by position, why intent comes from user turns but context may come from agent turns, why negation is adjacent-only, and why the interrogative opener is guarded against a following 'not'. Comments only; the executable code is unchanged.
|
Code review by qodo was updated up to the latest commit c2a7a95 |
Collapses every surviving multi-line comment to one line and drops the test docstrings, counterfactual reasoning and fixture narration. What remains: the forwarded-conversation format, the live 'agent' label, conservative blob detection, positional verb selection, the user-turn intent / agent-turn context split, adjacent-only negation, the 'not' guard, and the AGENT_REVIEW_OUTPUT fixture trap. Comments only; the executable code is unchanged.
|
Code review by qodo was updated up to the latest commit 86d90a8 |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
|
Code review by qodo was updated up to the latest commit be64e6f |
The bundle pinned 0.40.0, an image built before every MOSAICO fix merged since: pydantic-settings (#2560), the forwarded-conversation router fix (#2561), and failure fidelity (#2563). Version tags are immutable, so the pin has to move for the consortium to get them. Verified against the published image before opening: digest sha256:6cf24cd58c0c78bfcaa458e576eb9fb4a1fecb79cadfbfe92d9307486b89015f, dispatch.py carries _split_turns and propagate_tool_errors, pydantic_settings imports at 2.14.2, the langfuse_otel callback constructs as LangfuseOtelLogger where 0.40.0 returned None, and get_version() reports 0.41.0. The README upgrade example moves to 0.41.0 -> 0.42.0; left alone it would tell a reader to upgrade to the version they are already running. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
The MOSAICO reference agent forwards the whole conversation as one text part per turn, shaped
{role}: {content}, which the A2A SDK joins with newlines. Dispatch treated that blob as a single fresh request, which produced five distinct defects. All five were reproduced against the deployed agent.Worst first: the agent answers about the wrong patch, silently
_extract_diffused a non-greedyre.search, so the oldest fenced diff won. Live, before:A confident, well-formed answer about the wrong code, with nothing signalling the error. After:
### PR Type+ "Fixed invoice total calculation to include quantity."Verb selection ignored what the user asked
_detect_verblooped_VALID_VERBS = ("review", "improve", "describe", "ask")and returned the first entry found anywhere, so selection was by tuple position. This was never multi-turn-specific — single-turn requests broke too:describe this PRNow describe it instead, do not reviewcan you improve this? do not reviewdo not review, describe thisSince pr-agent's own output contains
Estimated effort to review:, one review made every later turn sticky — leaving 3 of 4 advertised skills unreachable.Three more
TASK_STATE_FAILED "could not fetch a public diff for .../pull/999999999"with a perfectly good diff in the current turn. Fires on any private repo URL too, not just a 404._diff_prosereturnedtext[:m.start()], discarding everything after the first raw diff marker — so the user's latest turn never reached the matcher.user: diff --git ..., soparse_unified_diffreturned[]and the run empty-fell-back to "no output produced". Turn-splitting fixes this for free._DIFF_BODY_LINE_REdid not recognise git's extended header (rename from/to,similarity index,new/deleted file mode, binary), so the first such line ended body mode and theindex/---/+++lines after it leaked into the prose the verb matcher reads. A patch containingrename to improve.pyoverrode a user's explicit "review this" and routed to improve; a?in a leaked path routed to ask.Approach
Blob detection is deliberately conservative: the text must both open with a role label and carry at least two. Otherwise it is treated as one segment and behaves exactly as before. Verb resolves from the latest user turn by position in the text; agent turns are never consulted for intent. Context (diff or URL) comes from the newest turn supplying one, agent turns included — a coding agent may be the party that produced the patch.
Two details were taken from the running reference agent rather than assumed: turn content is multi-line, so only the first line of a turn carries a label; and labels are
user/agent, notuser/assistant. Matching onlyassistantwould have been a no-op in production.Negation is handled adjacently (negator within two words of the verb). Wider handling is intent classification and does not belong in a regex router. Guarded against false positives:
there is no bug, review thisandi do not have time, review thisboth still route to review.Verification
1485 → 1533 passed, 1 skipped, 1 xfailed. 48 tests added, no existing test modified.
Live A/B ran against the deployed stack with only
dispatch.pybind-mounted, after confirming the image's copy was byte-identical tomain.Deliberately not fixed
Multiple fenced diffs in a single turn (last wins); prose immediately after a raw patch that begins with a markdown bullet, which is indistinguishable from a deletion line; non-adjacent negation. The
/improveHTML-table anddiff_providercapability issues are separate and out of scope here.