mirror of
https://github.com/multica-ai/multica.git
synced 2026-08-05 09:30:05 +02:00
30318b79bc
* fix(daemon): retire sessions whose history the provider refuses to replay A run killed mid-reply (machine shutdown, force-quit, SIGKILL) can leave an empty assistant message in the agent CLI's transcript. Every later resume replays it, the provider rejects the request, and the (agent, issue) pair is bricked with no self-healing and no user-facing recovery. Multica already has the mechanism for this — poisoned-session classification — but its detector paired "400" with "invalid_request_error", which is the Anthropic wire shape. The same defect reported by any other provider carried neither token, so it classified as agent_error.unknown: resume-safe by omission. GetLastTaskSession kept handing back the dead session on every follow-up, manual Rerun resolved it through the same predicate, and the in-turn fresh-session retry never fired because ResumeRejected is false here (nothing rejected the resume — the transcript loaded and the provider refused to replay it). Add taskfailure.UnresumableHistory, which recognises the defect by what the provider says is wrong — some content is empty, and here is which message in the history — rather than by status code or provider name. Both signals are required, so a tool reporting "field must not be empty" does not match. Wire it into the four places that decide whether a session survives: - classifyPoisonedError, so the task is written as api_invalid_request - shouldRetryWithFreshSession, so the turn recovers on all 17 backends instead of the subset whose adapter learned to detect it; the tools == 0 gate is unchanged, so a run that already used a tool is never re-run - ResumeUnsafeFailure, covering the manual-Rerun path - both resume queries, as defense-in-depth for hosts whose daemon predates this (self-host daemons upgrade on their own cadence) Fixes #6066. Also covers the daemon half of #5760. Co-authored-by: multica-agent <github@multica.ai> * fix(session): close the Chat and fresh-retry paths that resurrect a poisoned session Review found the previous commit stopped short in two places, both of which put the dead transcript back in play. Chat never consulted the guarded query. The claim handler reads chat_session.session_id first and only falls back to GetLastChatTaskSession when it is empty, so a poisoned pointer there bypasses every filter that query applies. The fail path merely declined to OVERWRITE the pointer, leaving it in place. It now clears it in the same transaction, matched on session and runtime so a concurrent turn's newer pointer survives. The promote guard moves to ResumeUnsafeFailure as well — the reason-only check passed an un-upgraded daemon's agent_error.unknown row and re-pinned what the clear had just removed. GetLastChatTaskSession also kept the row-level filter the issue query dropped in GH #5975: it discarded the newest poisoned row and fell back to an older completed row carrying the same dead session. It now judges each session by its latest terminal state, matching GetLastTaskSession. A recovered turn could not retire anything. A terminal report carried one session_id, and an empty one meant both "nothing to report" and "forget the old session", so a fresh-session retry that SUCCEEDED left the id it retried away from selectable — through an older completed row on the issue, or through the chat pointer. agent_task_queue.retired_session_id records the abandonment itself, reported on every terminal path including completed, and both resume lookups exclude it. This is the contract gap the previous PR deferred; the fresh-retry path now runs on all backends, so deferring it is not safe. Also narrows what the cross-backend test claims: it pins the shared decision, not that all 17 adapters surface the error into Result.Error (#5760 is the counter-example), and says so. Co-authored-by: multica-agent <github@multica.ai> * test(session): require pgx.ErrNoRows in the resume-exclusion assertions The `if err == nil && prior.SessionID.Valid` form these tests shared is false-green: any real fault — undefined column, syntax error, dead connection — makes err non-nil, so the condition is false and the test passes. Run against a database missing this branch's new column, the exclusion tests reported PASS on a SQLSTATE 42703, meaning they could not have caught a broken query. requireSessionExcluded demands pgx.ErrNoRows specifically and fails loudly on anything else, so a green run now means the filter worked rather than the query never ran. Applied to all nine sites, not just the four this branch added: the other five guard the same GetLastTaskSession exclusion behaviour that this branch changes, so leaving them false-green would leave the change under-tested. All nine pass on a correctly migrated database. Co-authored-by: multica-agent <github@multica.ai> --------- Co-authored-by: Bohan-J <bohan@devv.ai> Co-authored-by: multica-agent <github@multica.ai>
56 lines
3.0 KiB
Go
56 lines
3.0 KiB
Go
package taskfailure
|
|
|
|
import "regexp"
|
|
|
|
// UnresumableHistory reports whether an agent error means the conversation
|
|
// history itself can no longer be sent to the provider: a message already
|
|
// baked into the transcript carries empty content, so every resume of that
|
|
// session replays the same body and reproduces the same rejection.
|
|
//
|
|
// This predicate is deliberately provider-agnostic, and that is the whole
|
|
// point. The original detector paired "400" with "invalid_request_error"
|
|
// (see classifyPoisonedError in internal/daemon), which is the Anthropic wire
|
|
// shape. The identical defect is reported by other providers with neither
|
|
// token present:
|
|
//
|
|
// Invalid request: the message at position 37 with role 'assistant' must not be empty (GH #6066)
|
|
// provider.api_error: 400 the message at position 43 with role 'assistant' must not be empty (GH #5760)
|
|
// messages.37: all messages must have non-empty content ... (Anthropic)
|
|
// messages[43].content: content must not be empty
|
|
//
|
|
// Keying on a status code or a provider name means missing the next backend
|
|
// that words it differently — Multica supports 17 of them and holds only an
|
|
// opaque session id, so it cannot know which CLIs write a truncated
|
|
// transcript. What all of these DO state is the two things that define the
|
|
// defect: that some content is empty, and which message in the history it
|
|
// belongs to.
|
|
//
|
|
// Both signals are required, and that is what keeps the predicate narrow. A
|
|
// tool reporting "commit message must not be empty" has no message locator
|
|
// and does not match; a diff mentioning "messages[3]" without an emptiness
|
|
// complaint does not match either. Erring toward NOT matching is the safe
|
|
// direction: a miss leaves today's behaviour (the task fails and the user
|
|
// retries), while a false positive would discard a healthy session pointer
|
|
// and lose conversation context.
|
|
func UnresumableHistory(errText string) bool {
|
|
if errText == "" {
|
|
return false
|
|
}
|
|
return emptyContentRe.MatchString(errText) && historyMessageLocatorRe.MatchString(errText)
|
|
}
|
|
|
|
// emptyContentRe matches the provider's complaint that a content field is
|
|
// empty, in the wordings observed across providers.
|
|
var emptyContentRe = regexp.MustCompile(`(?i)must not be empty|must be non-?empty|must have non-?empty|non-?empty content|cannot be empty|should not be empty`)
|
|
|
|
// historyMessageLocatorRe matches the part of the error that points at a
|
|
// message inside the conversation history — a role name, an index, or a
|
|
// position. Without one of these an emptiness complaint is about some other
|
|
// field entirely and says nothing about the transcript.
|
|
//
|
|
// Keep in sync with the equivalent regex in the GetLastTaskSession /
|
|
// GetLastChatTaskSession resume queries (pkg/db/queries), which apply the same
|
|
// text guard server-side for rows an older daemon classified as
|
|
// agent_error.unknown.
|
|
var historyMessageLocatorRe = regexp.MustCompile(`(?i)role[^a-z0-9]{0,2}assistant|assistant message|message at position|messages\.[0-9]|messages\[[0-9]`)
|