-
Notifications
You must be signed in to change notification settings - Fork 445
fix(runtime): preserve provider diagnostics on failed turns #4528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c4c90c2
0f67e93
c27131f
ee36860
2f3ad6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| import { expect, test } from './fixtures.js'; | ||
|
|
||
| test('provider failure detail is collapsed until the user expands it', async ({ | ||
| providerFailureWindow: page, | ||
| }) => { | ||
| const diagnostic = page.locator('.maka-turn-failed-diagnostic'); | ||
| await expect(diagnostic).not.toHaveAttribute('open', ''); | ||
| await expect(diagnostic.getByText('Provider 响应详情', { exact: true })).toBeVisible(); | ||
| await expect(diagnostic.locator('pre')).not.toBeVisible(); | ||
|
|
||
| await diagnostic.locator('summary').click(); | ||
|
|
||
| await expect(diagnostic).toHaveAttribute('open', ''); | ||
| await expect(diagnostic.locator('pre')).toHaveText( | ||
| 'Provider returned 429: request rate limit reached. Please retry after 30 seconds.', | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,7 +71,10 @@ export async function readCanonicalTurnSnapshot( | |
| const failureMessage = | ||
| fact.terminalEvent.content?.kind === 'error' | ||
| ? truncateUtf8( | ||
| redactSecrets(fact.terminalEvent.content.message), | ||
| redactSecrets( | ||
| providerFailureSummaryFromRuntimeEvent(fact.terminalEvent) ?? | ||
| fact.terminalEvent.content.message, | ||
| ), | ||
| TURN_FAILURE_MESSAGE_MAX_BYTES, | ||
| '…', | ||
| ) | ||
|
|
@@ -121,6 +124,14 @@ async function hasPendingInteraction( | |
| ); | ||
| } | ||
|
|
||
| function providerFailureSummaryFromRuntimeEvent( | ||
| event: import('@maka/core/runtime-event').RuntimeEvent, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| ): string | undefined { | ||
| const details = event.content?.kind === 'error' ? event.content.details : undefined; | ||
| if (!details || Array.isArray(details)) return undefined; | ||
| const summary = details.providerSummary; | ||
| return typeof summary === 'string' && summary.length > 0 ? summary : undefined; | ||
| } | ||
| function readContextCompactionOutcome(value: unknown): ContextCompactionOutcome | undefined { | ||
| if (!value || typeof value !== 'object') return undefined; | ||
| const outcome = value as Record<string, unknown>; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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?