fix(runtime): dedup only a trailing identical user turn on session recovery (closes #378) - #379
Conversation
The delegated-consent auth gate (#330) was consulted only for resolveClient (connection-establish) errors. But per-request-auth transports (transport_http attaches the bearer on every Send, including tools/call) 403 at CALL time, not at initialize — so a type=user server whose user has no grant surfaced ErrNoToken from client.CallTool, which the gate never inspected. The call failed hard with reason=no_token, no mcp_auth_required event fired, and the platform consent flow had nothing to trigger on (field: Atlassian agent, forge#376). Execute now wraps the whole resolve→call sequence in the gate: an ErrNoToken from either half parks via authGate.Await and, on a granted resume, retries resolve→call. Same bounded, one-shot semantics as before; the diagnostic phase prefix is irrelevant now since both paths are covered. Tests: call-time no-token parks + retries to success; gate give-up still fails as no_token; a non-auth CallTool error never parks; nil gate still surfaces ErrNoToken.
…covery (closes #378) The recovered-session dedup skipped the incoming user message when it matched the last user message ANYWHERE in history. A verbatim re-run of an already-answered request (the session ends in an assistant turn) therefore matched the earlier user turn and was dropped — the loop replayed the poisoned transcript and repeated its last reply without re-attempting tools. Field: a delegated MCP (Jira) call that failed "not connected" never recovered when the user connected the account and re-sent the identical request. Dedup is now trailing-only: skip only when the recovered session's LAST message is an identical user turn (a genuine premature-loop-exit duplicate — persisted but never answered). If it ends in an assistant turn, an identical incoming message is a legitimate re-run and is appended so the loop re-enters and re-attempts tools. Tests: a re-run after an answered session appends the turn (regression); a trailing-user duplicate is still skipped (no regression).
initializ-mk
left a comment
There was a problem hiding this comment.
Reviewed against the branch source — correct, minimal, and the reasoning is airtight. LGTM.
Scope note — stacked on #377
The diff carries two commits: 8fafa7013 is #377 verbatim (I diffed mcp_tool.go across both branches — byte-identical, already reviewed and approved on #377), and 593fa9c46 is the net-new loop.go fix reviewed here. Merge ordering: landing #379 subsumes #377 — either merge #377 first, or merge #379 and close #377 as included.
The fix is right
The old dedup found the last user message anywhere in history and dropped the incoming message on a content match. Wrong for an answered session ([user, assistant]): a verbatim re-run matched the earlier user turn, got dropped, and the loop re-ran on a transcript ending in the assistant's own refusal — so the model continued from its last answer and never re-attempted tools. Trailing-only scoping is the correct fix, and it matches the original comment's stated intent ("already ends with an identical user message").
What makes trailing-only safe in both directions:
- Premature-exit (
[…, user], never answered):trailingDuptrue → skip append. Still correct, because the recovered transcript already ends in that user turn, so the re-entered loop drives a fresh attempt anyway — skipping just avoids a literal duplicate. - Answered (
[…, assistant]):trailingDupfalse → append →[…, assistant, user]→ model sees a fresh trailing user turn → re-attempts tools. The #378 fix.
No over-append risk: the only situation warranting a dedup is a trailing unanswered user turn, which is exactly what the guard catches; everything else should be appended.
Tests are precise
VerbatimReRunIsAppended— answered session (ends assistant), asserts the model's captured trailing message is the re-run user turn. Fails on the old code (which left the assistant refusal trailing) — a true regression test.TrailingUserDupSkipped— session ending in an identical user turn, asserts exactly one copy survives. Guards the case the dedup was originally written for.
Non-blocking observation (pre-existing)
The comparison is .Content string-equality only — two messages with identical text but differing attachments/parts are treated as duplicates. True of the old code too; out of scope for this fix, noted for completeness.
CI fully green (Test, Integration, all builds, Lint).
| } | ||
| if lastUserIdx < 0 || msgs[lastUserIdx].Content != newMsg.Content { | ||
| n := len(msgs) | ||
| trailingDup := n > 0 && msgs[n-1].Role == llm.RoleUser && msgs[n-1].Content == newMsg.Content |
There was a problem hiding this comment.
This is the crux and it's correct. Scoping the dedup to the trailing message (rather than the last user message anywhere) is what distinguishes a premature-exit retry ([…, user] → skip; the trailing user turn already re-drives the loop) from a legitimate re-run of an answered request ([…, assistant] → append → model sees a fresh user turn and re-attempts tools).
Worth calling out that the skip branch is still functionally correct, not just dedup-safe: in the premature-exit case the recovered transcript already ends in the user turn, so re-entering the loop attempts the tool regardless of whether we append. So neither branch can strand the request.
Closes #378.
Problem
Re-running the same request in an already-answered execution does nothing — the agent replays its previous answer and never re-attempts tools. Field case: a delegated MCP (Atlassian) call failed "not connected"; the user connected the account and re-sent the identical request; the execution stayed broken (no tool call), while a fresh execution worked.
Log proof (
next-SNAPSHOT-50cd5e3):session recovered messages=7, then onellm_callwhose prompt ends with the assistant's prior refusal — no new user turn appended — and nomcp_tool_call/tool_exec. The re-send's message never reached the model.Root cause —
forge-core/runtime/loop.goOn a recovered session, the dedup compared the incoming message against the last user message anywhere in history:
Its own comment scopes the intent to "already ends with an identical user message" (a premature-loop-exit duplicate). But when a prior identical request was already answered — the session ends in an assistant turn — a verbatim re-run matched that earlier user turn and was dropped. The model was re-run on the poisoned transcript with no new input.
Fix
Dedup trailing-only:
A premature-exit duplicate leaves the session ending in a user turn → still skipped. A re-run of an answered request (session ends in an assistant turn) → appended, loop re-enters, tools re-attempted.
Tests
VerbatimReRunIsAppended— answered session (ends assistant) + identical incoming message → the message is the trailing turn the model sees (regression; fails on old code, which left the assistant refusal trailing).TrailingUserDupSkipped— session ending in an identical user turn → still deduped (no regression).go test ./runtime/...green, gofmt + golangci-lint clean.Related