fix(runtime): preserve provider diagnostics on failed turns - #4528
fix(runtime): preserve provider diagnostics on failed turns#4528testikun wants to merge 5 commits into
Conversation
f1a15fd to
5147940
Compare
Generated-by: Codex
Generated-by: Codex
5147940 to
19ff1ad
Compare
Generated-by: OpenAI Codex
| } | ||
|
|
||
| function providerFailureSummaryFromRuntimeEvent( | ||
| event: import('@maka/core/runtime-event').RuntimeEvent, |
There was a problem hiding this comment.
nit: This inline type query is valid and does not emit a runtime import, but the surrounding runtime-host server code consistently uses top-level type-only imports, including every other RuntimeEvent reference. Could we add import type { RuntimeEvent } from '@maka/core/runtime-event' and use event: RuntimeEvent here for consistency and readability?
| } | ||
| const summary = content.details.providerSummary; | ||
| return typeof summary === 'string' && summary.length > 0 | ||
| ? truncateUtf8(redactSecrets(summary), TURN_FAILURE_MESSAGE_MAX_BYTES, '…') |
There was a problem hiding this comment.
[P2] Preserve structured metadata when applying the durable bound
providerFailureSummary() reserves space for (code=..., status=..., requestId=...) at the end of a 2 KiB string, but this projection truncates that complete string to 256 bytes from the front. With a sufficiently long provider message, all of the structured metadata is therefore lost—the exact diagnostic information this change is intended to preserve. Could we apply the final bound while reserving space for the metadata suffix (or persist these fields separately), and add a regression test with a long message plus code/status/requestId?
| recoverable: false, | ||
| reason, | ||
| message: `Turn failed: ${reason}`, | ||
| message: terminal.failureMessage ?? `Turn failed: ${reason}`, |
There was a problem hiding this comment.
[P2] Preserve provider-summary provenance and stable error semantics
TurnSnapshot.failureMessage is not necessarily provider-specific: readCanonicalTurnSnapshot() still falls back to the terminal event's generic content.message. Treating every value here as details.providerSummary means replayed internal/runtime failures can be presented as “Provider response details.” It also replaces the stable ErrorEvent.message with the diagnostic, whereas live ModelAdapter events keep the classified message in message and carry provider text only in details. Could we preserve provider-summary provenance in a separate field, keep the replayed message stable, and only populate providerSummary when it actually originated from the provider?
Astro-Han
left a comment
There was a problem hiding this comment.
The problem is real and #4502 states it correctly: normalizeProviderFailure only keeps providerFailureSummary's text when failure.kind === 'unknown', and the adapter never populates ErrorEvent.details, so a 429 and a 404 collapse into the same sentence. Carrying a bounded, redacted summary through the event, the durable Turn, the wire and the UI is the right shape, and the collapsed <details> below the classified banner is the right presentation.
Two things before the code.
The branch is CONFLICTING, and gh pr checks reports no checks at all on this head: no checks reported on the 'codex/issue-4502-provider-diagnostics' branch. Everything in the Verification section is a local claim with nothing to check it against. Please rebase and let CI run before the next round.
P1: the compatibility epoch. main is at 109, and 107 is already taken (token_usage anchors, #4559). This lowers the constant to 107 and attaches a second meaning to it, which would leave two different 107s and make the handshake gate meaningless. Inline.
On the main question I had, which is whether the diagnostic ends up with one owner: mostly it does, but three places take a copy, and two of those replace a value that already had an owner.
P1: markRunFailed (agent-run.ts:947-951) now stores the provider summary instead of ev.message. That field is not a display slot; it reaches AgentRunHeader (:1348, :1404, :1524), becomes the terminal event message in terminal-run-commit.ts:212-213, and is read by runtime-resource-projection.ts:164-168 and conversation-copy.ts:641. None of those consumers are looked at here, and #4502 asks for the provider text in addition to the classified message, not instead of it. The summary already travels independently via details.providerSummary and the appendTurnState call two lines above, so this second copy is not needed. Inline.
P2: the extraction rule is written three times, identically (read details.providerSummary, redact, truncate to 256): session-projection-helpers.ts:188-197, runtime-event-read-model.ts:1210-1218, canonical-turn-snapshot.ts:116-122. The 256-byte bound is declared four times: protocol/turn.ts:51 (already on main), the new SESSION_TURN_FAILURE_MESSAGE_MAX_BYTES, the new local TURN_FAILURE_MESSAGE_MAX_BYTES in runtime-event-read-model.ts:51, and a bare 256 literal. One rule should have one place. Inline on two of them.
P2: I agree with both of @Yx01-me's findings and will not open duplicate threads.
On session-projector.ts:293: worth adding that this PR changes an existing assertion to accept the new behavior. canonical-session-projection.test.ts:423 went from 'canonical provider failure api_key=[redacted]' to the provider summary. When an existing assertion is rewritten to match a change, the PR body should say why the old behavior stopped being correct. Live keeps the classified sentence in message and the provider text in details (model-adapter.ts:565-566); after this, replay does not, so the same failed turn reads differently before and after a reload.
On the truncation: agreed, and I would fix it structurally rather than by moving the cut. code, status and requestId are the fields that separate a 429 from a 404, and appending them to a string that three downstream layers each truncate from the front will keep losing them.
P3, small ones: ErrorEvent.details is string[] | Record<string, unknown>, so every reader needs an Array.isArray plus a typeof guard; a typed providerSummary?: string on ErrorEvent would delete all three guards along with the three copies. agent-run.ts:943 and :949 each call providerFailureMessageFromEvent(ev) separately. And the body says "live and reloaded", but the live toast path #4502 names (app-shell-session-events.ts) is untouched; ChatView covers the transcript, the toast diagnostic is still the old text. Worth stating rather than changing.
Evidence boundary: read at c4d2da79 against origin/main (ab7b739260). No build, no tests run, and no CI exists at this head to read.
AI-assisted review: drafted with Maka.
| // Increment when the same protocol version no longer guarantees safe Client-Host | ||
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 105 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 107 as const; |
There was a problem hiding this comment.
P1: main is at 109, and 107 already means "token_usage anchors record the model and connection that produced them" (#4559). This lowers the constant and gives 107 a second meaning. The comment block here jumping straight from 107 to 105 shows the branch base predates 106 through 109, which is also most of why the PR is CONFLICTING. After the rebase this should be 110, with its note added above the existing 109 entry.
| this.markRunFailed(ev.reason ?? ev.code ?? 'unknown', ev.message, ev.ts); | ||
| this.markRunFailed( | ||
| ev.reason ?? ev.code ?? 'unknown', | ||
| (ev.type === 'error' ? providerFailureMessageFromEvent(ev) : undefined) ?? ev.message, |
There was a problem hiding this comment.
P1: this replaces the run's failure message rather than adding to it, and AgentRun.failureMessage is not a display-only field. It is written to AgentRunHeader (:1348, :1404, :1524), becomes the terminal event's message in terminal-run-commit.ts:212-213, and is read by runtime-resource-projection.ts:164-168 and conversation-copy.ts:641. Swapping the classified sentence for provider text changes what all of those report, and none of them are covered by this PR's tests.
The provider summary already reaches the durable Turn through details.providerSummary and the appendTurnState call on line 943, so nothing is lost by leaving this one alone: keep ev.message here.
| ? { | ||
| content: { | ||
| kind: 'error' as const, | ||
| message: turnState.failureMessage, |
There was a problem hiding this comment.
P2: two problems in one block. message here is a string that never existed as a message: it is turnState.failureMessage, already truncated to 256 bytes by the projection that wrote it. And it is the same value as details.providerSummary on the next line, while the read model only reads the latter, so message is pure duplication.
Beyond that, recovered terminal events for failed turns previously carried no content at all. Giving them content.kind === 'error' changes what content-driven readers see (failureClassFromRuntimeEvent, projectTerminalTurnState), and there is no backfill test in this PR. Suggest populating only details.providerSummary, leaving message unset, and adding a regression test for a legacy failed turn.
| export const SESSION_TURN_QUERY_MAX_CONTRIBUTIONS = 128; | ||
| export const SESSION_TURN_QUERY_RESULT_MAX_BYTES = 192 * 1024; | ||
| export const SESSION_TURN_DIAGNOSTIC_MAX_BYTES = 128; | ||
| export const SESSION_TURN_FAILURE_MESSAGE_MAX_BYTES = 256; |
There was a problem hiding this comment.
P2: TURN_FAILURE_MESSAGE_MAX_BYTES = 256 already exists in this package at protocol/turn.ts:51, same value, same meaning, same bound on the same string. Import it instead of declaring a second name for it.
| if (!event.details || Array.isArray(event.details)) return undefined; | ||
| const summary = event.details.providerSummary; | ||
| return typeof summary === 'string' && summary.length > 0 | ||
| ? truncateUtf8(redactSecrets(summary), 256, '…') |
There was a problem hiding this comment.
P2: the bound is a bare literal here, a new module-local TURN_FAILURE_MESSAGE_MAX_BYTES in runtime-event-read-model.ts:51, and a third constant in the Host protocol. This whole function is also duplicated twice: runtime-event-read-model.ts:1210-1218 and runtime-host/src/server/canonical-turn-snapshot.ts:116-122 do exactly the same read, redact and truncate.
One extraction, one bound. If the Host side cannot import from @maka/runtime, this belongs in @maka/core next to the ErrorEvent definition. A typed providerSummary?: string on ErrorEvent would go further and remove the Array.isArray plus typeof guards from all three call sites.
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
|
已处理并重新提交。\n\n- 已合并当前 apache/main,compatibility epoch 对齐到 109。\n- 保留分类错误消息,同时单独持久化 bounded、redacted provider summary。\n- 统一 provider summary 的提取、脱敏和 256-byte 截断逻辑。\n- 补齐 live diagnostic 与 reload 后的 provider response details。\n- Runtime/Core/Runtime Host/UI 定向构建和 129 个相关测试已通过。\n- 未运行本机完整 Host lifecycle 套件。 |
|
已处理并重新提交。\n\n- 已合并最新 apache/main,解决合并冲突。\n- 保留所有 failure class 的 bounded provider diagnostics 透传、持久化、reload 展示和 secret redaction/size bounds。\n- Runtime Host compatibility epoch 更新为 107,避免与其他开放协议变更冲突。\n- 本地 Core、Storage、Runtime、Runtime Host、UI 构建通过;相关 diagnostics/projection 测试通过。\n- 按约束未运行本机完整 Host 生命周期测试。 |
Summary
Preserve the bounded, redacted provider response summary for every classified provider failure while keeping the existing error category as the primary presentation. The summary now flows through RuntimeEvent, durable Turn, Runtime Host projections, and Desktop, where it appears collapsed and expandable beneath failed-turn banners for live and reloaded turns. Retry policy and unrelated error semantics remain unchanged.
Fixes #4502
Verification
apache/main(epoch 99 → 100).apache/main.<details>below the classified banner and reload materialization coverage.Existing secret redaction and byte bounds are preserved at the provider, durable Turn, Host, and event boundaries.
AI use
Tool(s) and scope: Codex authored the implementation and regression tests.
Checklist
Does this PR entail a change in behavior?