MUL-5154: fix(agent): surface kimi provider.api_error 400 as task failure instead of silent completion#5785
Conversation
…ad of silent completion (Fixes multica-ai#5760) When a kimi ACP session is resumed and the upstream LLM rejects the rebuilt history with a 400 ("the message at position N with role 'assistant' must not be empty"), kimi logs the error to stderr and reports session/prompt as stopReason=end_turn. The acpProviderErrorSniffer did not match this line format — no emoji prefix, lowercase "error:", no known keyword — so promoteACPResultOnProviderError found no provider error and left the task status as "completed" with empty output and no error visible anywhere (MUL-5152, multica-ai#5760). Three regex changes in hermes.go (shared by kimi/hermes/grok/kiro): 1. acpErrorHeaderRe: add `provider.api_error` as an additional match alternative so the kimi-style line is captured into the sniffer buffer. 2. acpErrorDetailRe: add `provider.api_error: NNN` as a detail-extraction prefix so the human-readable message after the status code is surfaced (e.g. "the message at position 43 with role 'assistant' must not be empty") rather than the raw header line. 3. acpTerminalErrorRe: add `provider.api_error: 4` so any 4xx kimi API error is classified as terminal. 4xx responses from the LLM API are client errors and cannot be resolved by retrying the same request. 5xx responses are captured but not terminal, so they only promote to failed when output is also empty (existing fallback behaviour). Two new tests: - TestACPProviderErrorSnifferKimiApiError: the exact multica-ai#5760 stderr line must be classified as terminal and prompt failed with the error detail. - TestACPProviderErrorSnifferKimiApiError5xx: a 5xx line is captured but not terminal.
|
@kakiuwang-ui is attempting to deploy a commit to the IndexLabs Team on Vercel. A member of the Team first needs to authorize it. |
multica-eve
left a comment
There was a problem hiding this comment.
这个改动能让原本“看似成功却没有回复”的任务显示真实失败原因,方向是对的;但已经失效的助手仍不会恢复,用户下一次操作还会重复失败。
✅ 产品上应该接受这个修复方向,但以下两个问题需要在合入前解决:
-
acpTerminalErrorRe使用provider\.api_error: 4会把 400、401、408、429 等所有 4xx 都标成 sticky terminal。尤其一次可恢复的 429 即使随后重试成功并产生正常输出,也可能被翻成 failed。这个 sniffer 被 Hermes、Kimi、Kiro、Qoder、Trae、Grok 共用,回归范围不是 Kimi 局部。请解析完整状态码,并补一个“429 后成功仍 completed”的测试。 -
当前改动只在
promoteACPResultOnProviderError中把状态改成 failed,没有为精确的坏历史错误设置ResumeRejected,也不会命中现有api_invalid_requestpoison 分类。结果仍保留原 session,下一条任务会继续 resume 并重复同一个 400。请只对 resume 路径中同时满足400、role 'assistant'、must not be empty的信号标记 resume unsafe,复用 daemon 的tools == 0安全门最多 fresh retry 一次;若仍失败,确保 issue/chat 后续查询不再选择该 session。
另外,acpErrorDetailRe 把原有分支的 \s* 一并收紧为 \s+,会丢失 detail:value 这种格式;建议保留旧分支,只单独增加 provider.api_error 解析。
请补 Kimi ACP 端到端恢复测试、fresh retry 仍失败不三次执行、有工具时不重跑,以及 poisoned session 查询排除测试。
…ejected for poisoned history Three fixes addressing multica-eve review on multica-ai#5785: 1. acpErrorDetailRe: restore \s* (was \s+ in the PR, breaking "detail:value" no-space format in pre-existing branches) 2. acpTerminalErrorRe: replace the prefix match "provider.api_error: 4" with an exact set "(?:400|401|403)". 429 (rate-limit) and 408 (timeout) are now excluded — the kimi adapter retries those internally, and a run that ultimately succeeds must stay status=completed. 3. isPoisonedHistory + ResumeRejected: add isPoisonedHistory() to acpProviderErrorSniffer to detect the kimi "400 + role 'assistant' + must not be empty" pattern. After promoteACPResultOnProviderError, set resumeRejected=true when the history is poisoned so the daemon drops the broken session and retries fresh via the tools==0 gate. Tests added: - TestACPProviderErrorSnifferKimiRateLimitNotTerminal: 429 not terminal, completed status preserved when output is non-empty ("429 after success still completed" case requested in review) - TestACPProviderErrorSnifferPoisonedHistory: isPoisonedHistory() true for the exact pattern, false for plain 400 with different message - TestHermesBackendPoisonedHistorySetsResumeRejected: end-to-end backend test with fake kimi ACP script; asserts Status=failed + ResumeRejected=true on poisoned-history resume
|
感谢详细的 review,三个问题都已修复,新增了 commit 410d6a8。 1. 已将 理由:429(限速)和 408(超时)由 kimi adapter 内部重试处理,最终成功的任务不应被翻为 failed;400/401/403 属于客户端错误,重试无意义,应标为 terminal。 新增测试
2. 新增 在 新增两个测试:
3. 已将合并时引入的 |
multica-eve
left a comment
There was a problem hiding this comment.
Thanks for addressing the overly broad 4xx match and restoring the existing detail:value behavior. Two merge-blocking issues remain on 410d6a8e6e:
-
The recovery signal was added to the wrong backend.
The new
ResumeRejectedassignment is inhermesBackend.Execute(server/pkg/agent/hermes.go), but this bug runs through the separatekimiBackend.Execute. AfterpromoteACPResultOnProviderErroratserver/pkg/agent/kimi.go:379, the Kimi path returns without checkingisPoisonedHistory, so the daemon still does not perform the guarded fresh-session retry.The new end-to-end-looking test does not catch this because
server/pkg/agent/hermes_test.go:2377constructsNew("hermes", ...), notNew("kimi", ...). Please exercise the real Kimi backend and only setResumeRejectedwhen a resume was actually requested. The fingerprint should also requirerole 'assistant'; the current matcher only checks that the line contains the wordrole. -
The failed session is still eligible for future issue/chat resume.
messageLockedremoves the400status code from the surfaced error, leaving onlykimi provider error: ... must not be empty. The daemon poison classifier only recognizes400 + invalid_request_error, andGetLastTaskSession/GetLastChatTaskSessionexclude only the existing poison reasons or that same raw marker. This failure therefore lands as an ordinary unknown agent error with a session ID, and the next issue mention or chat can select the same poisoned session again.Please persist a resume-unsafe failure reason for this exact Kimi fingerprint (reusing
api_invalid_requestis reasonable), and add issue/chat query coverage. The fresh retry failure path must receive the same classification so it cannot become the next resumable session.
Please add coverage for the real flow: Kimi resume → one fresh retry, fresh retry failure → no third execution, tool activity → no retry, and poisoned-session exclusion for both issue and chat lookups.
The poisoned-history 400 error (role 'assistant' must not be empty) is emitted by the kimi adapter, so kimi.go must also set resumeRejected=true after promoteACPResultOnProviderError when isPoisonedHistory() fires. The previous commit only wired this in hermes.go.
|
补充:发现 kimi.go 的 |
multica-eve
left a comment
There was a problem hiding this comment.
这次更新让 Kimi 在恢复坏会话时能触发一次有副作用保护的新会话重试,修复了上一版接错执行路径的问题;但如果新会话也失败,系统仍可能保存并再次使用不可恢复的会话,用户后续操作会继续遇到同样失败。产品方向应该接受,但下面的问题需要在合入前解决。
-
[P1] fresh retry 失败后的 poisoned session 仍未进入排除链路。
server/pkg/agent/hermes.go:2283的messageLocked从最终错误中剥掉了provider.api_error: 400,只留下kimi provider error: ... must not be empty。server/internal/daemon/poisoned.go:104仍只识别同时包含400和invalid_request_error的错误,因此该结果不会落成api_invalid_request;GetLastTaskSession/GetLastChatTaskSession也就不会排除它。fresh retry 若再次失败,下一次 issue mention 或 chat 仍能恢复这个坏 session。请保留结构化的状态码,或在 agent Result 中显式传递 resume-unsafe 原因;对精确 Kimi 指纹写入已有的
api_invalid_request(或等价的不可恢复分类),并补 issue/chat 两条查询的排除测试。fresh retry 失败的最终结果必须经过同一分类。 -
ResumeRejected 的条件仍需收紧,并补真实 Kimi 链路覆盖。
server/pkg/agent/kimi.go:380没有要求opts.ResumeSessionID != "";server/pkg/agent/hermes.go:2272-2274的isPoisonedHistory只检查任意role,没有精确要求role 'assistant'。请限定为“本次确实 resume + 精确 400 + assistant + must not be empty”,避免把 fresh run 或其它错误误报成 resume rejection。当前新增的端到端样式测试仍构造
New("hermes"),没有覆盖真实 Kimi backend。请补:Kimi resume → 最多一次 fresh retry、fresh retry 再失败不执行第三次、已有工具活动时不重跑,以及上述 poisoned-session 持久化排除。
本地针对性 agent/daemon 测试和 git diff --check 均通过,GitHub 六项 CI 也全绿;现有测试未覆盖上述最终持久化与真实 Kimi 恢复边界。
结论:Request changes。
…sonedError to Kimi format Three issues addressed from review: 1. isPoisonedHistory: tighten "role" → "role 'assistant'" to avoid matching role 'user' or other role values that cannot cause this specific error. 2. ResumeRejected guard: add opts.ResumeSessionID != "" in both kimi.go and hermes.go so a fresh run (no resume) that encounters the same error fingerprint is not incorrectly flagged. 3. classifyPoisonedError: add Kimi-specific pattern. messageLocked strips the "provider.api_error: 400" prefix, leaving "kimi provider error: … role 'assistant' must not be empty". Without this classifier the task was saved with a non-api_invalid_request failure_reason, so GetLastTaskSession did not exclude it and subsequent tasks could still resume the poisoned session. Tests added: - TestKimiBackendPoisonedHistorySetsResumeRejected: true kimi backend path - TestKimiBackendPoisonedHistoryFreshRunNoResumeRejected: fresh run stays false - TestClassifyPoisonedError: Kimi stripped format and false-positive guards - TestACPProviderErrorSnifferPoisonedHistory: role 'user' must not match
|
Thanks for the detailed review. This push addresses all three blockers: 1.
|
…tity contamination (MUL-5736) When an agent is renamed (e.g. "Reviewer" → "Backend Builder"), the workdir from its last run still holds a CLAUDE.md whose ## Agent Identity section names the old role. Resuming that session means the conversation history has already established the old identity, causing the agent to misidentify itself and refuse tasks — the behaviour reported in MUL-5736. Fix: before overwriting CLAUDE.md with the new identity, read the name already recorded in the managed marker block. If it differs from the current agent name, drop PriorSessionID and start a fresh conversation. The check runs after gateCodexResumeToRolloutPresence while the old file is still on disk. The drop does not set PriorSessionResumeUnavailable because a rename is intentional: the user changed the agent's identity on purpose, so a fresh session is expected behaviour rather than an unexpected loss to surface. ReadPriorAgentName is added to execenv so the parsing logic is testable in isolation; gateResumeToSameAgentName wraps the daemon-level gate.
…lassifyPoisonedError Kimi's two-line stderr format emits "provider.api_error: 400" on the error header line and "detail: messages[N].content: content must not be empty" on the next line. Three bugs combined to let poisoned sessions slip through the exclusion chain: 1. messageLocked(): acpErrorDetailRe's `provider\.api_error: [0-9]+` branch was backtracking one digit into `(.+)` when the status code was the last token on the line, capturing "0" as the detail and returning "kimi provider error: provider.api_error: 400 0" before ever reaching the real detail line. Fixed by tracking apiErrLineIdx and skipping pure-digit captures on that line (backtrack artefact). 2. isPoisonedHistory(): required all three markers (provider.api_error: 400, role 'assistant', must not be empty) on the same stderr line. Fixed to scan across all captured lines with separate hasBadRequest and hasEmptyContent flags. 3. classifyPoisonedError(): only had a "role 'assistant' + must not be empty" pattern (Kimi single-line format). Added a "provider.api_error: 400 + must not be empty" pattern so the status prefix forwarded by the fixed messageLocked is recognised and the task row gets failure_reason=api_invalid_request, excluding it from GetLastTaskSession resume lookups. Adds: - TestACPProviderErrorSnifferMessageLockedPreservesAPIErrorStatus - Two-line case in TestACPProviderErrorSnifferPoisonedHistory - TestKimiBackendPoisonedHistoryTwoLineFormat (end-to-end integration) - Three new classifyPoisonedError table cases in poisoned_test.go
…fresh retry (MUL-5785) Three-part fix for the remaining P1: when a Kimi fresh retry fails without establishing a new session, the prior completed task's session is still returned by GetLastTaskSession, causing the next run to resume the same poisoned history indefinitely. 1. daemon.go: when shouldRetryWithFreshSession is true and the fresh retry fails without establishing a new session (SessionID == ""), restore the original poisoned error message and PriorSessionID on the result. This ensures classifyPoisonedError labels the task api_invalid_request and the session_id column points at the poisoned session. 2. agent.sql / chat.sql: add NOT EXISTS subquery to both GetLastTaskSession and GetLastChatTaskSession. The subquery excludes any session that a subsequent task for the same (agent, issue) / chat_session has flagged as api_invalid_request — catching the completed-Task-A / poisoned-Task-B pattern that the direct failure_reason filter cannot reach. 3. rerun_session_test.go: add two integration tests covering both query paths: TestGetLastTaskSessionExcludesPoisonedCompletedViaSubsequentTask and TestGetLastChatTaskSessionExcludesPoisonedViaSubsequentTask.
Fixes #5760
Root cause
When a resumed Kimi session's LLM request returns HTTP 400 ("the message at position N with role 'assistant' must not be empty"), kimi-code logs the error to stderr and still reports
session/promptasstopReason=end_turn. The ACP provider-error sniffer (acpProviderErrorSniffer) did not recognise this line format because:⚠️/❌) and no[ERROR]tagerror:instead ofError:(not matched byacpErrorDetailRe)BadRequestError,HTTP NNN, etc.) in the right positionSo
promoteACPResultOnProviderErrorsaw no provider error, left the status as"completed"with empty output, and posted no error anywhere. From the user's perspective: agent stops responding, every task "succeeds" immediately withtools=0and no output, no log entry.Changes
Three regex changes in
hermes.go(shared by kimi/hermes/grok/kiro/qoder):acpErrorHeaderRe|provider\.api_erroralternativeacpErrorDetailReprovider\.api_error: [0-9]+prefixacpTerminalErrorReprovider\.api_error: 4Result for the #5760 stderr line:
acpErrorHeaderRe→ captured ✓acpTerminalErrorRe→ terminal (4xx) ✓acpErrorDetailRe→ extractsthe message at position 43…✓promoteACPResultOnProviderError→status=failed,error="kimi provider error: the message at position 43 with role 'assistant' must not be empty"✓5xx provider errors (
provider.api_error: 503) are captured but not terminal, so they only promote to failed when output is also empty — consistent with the existing transient-error behaviour.Tests
TestACPProviderErrorSnifferKimiApiError: the exact Kimi (ACP) runtime: tasks silently complete with no output when resumed session's LLM request fails (400 empty assistant message) #5760 stderr line must classify as terminal and causepromoteACPResultOnProviderErrorto flip status tofailed.TestACPProviderErrorSnifferKimiApiError5xx: a 5xx line is captured but not terminal.TestHermesProviderError*tests continue to pass.