Repository navigation
fix(infinity-agent-core): surface completion failures to clients - #159
akainth015 wants to merge 1 commit into
Conversation
Deploying infinity with
|
| Latest commit: |
ebb2852
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://afad70fc.infinity-dc7.pages.dev |
| Branch Preview URL: | https://pr-completion-failure-visibi.infinity-dc7.pages.dev |
| /// [`CompletionEvent::Error`]. | ||
| const ERROR_SUMMARY_MAX_CHARS: usize = 400; | ||
|
|
||
| /// Condense a provider error message for user display. Provider errors are |
There was a problem hiding this comment.
This should be the responsibility of the provider.
There was a problem hiding this comment.
Agreed — removed. Providers now author the user-facing text (CompletionError::with_user_message / user_message(), falling back to Display) and core forwards it verbatim. Bedrock writes concise messages itself (e.g. the mwinit hint) and owns the length; the field also crosses the remote-provider wire.
There was a problem hiding this comment.
What? Why an additional user_message? What I meant is that the provider should be responsible for making sure the error is sufficiently short.
There was a problem hiding this comment.
Got it. There's no new API now: user_message is gone. Bedrock's sdk_error_message makes the existing message short itself: the service's own message when there is one, otherwise the error kind plus its root cause, not the whole chain. For credential failures, that root cause is the credential source's own explanation (e.g. a credential_process's stderr). Core forwards the message unchanged. (d749373)
| // any still-unvalidated inputs so the commit phase | ||
| // persists them — they must not be lost with the | ||
| // driver (no overflow was reported, they are safe). | ||
| history.mark_inputs_model_validated(); |
There was a problem hiding this comment.
I'm not sure this is right. If we are getting provider errors, it might also be due to malformed user input, in which case we should not mark this as trusted.
There was a problem hiding this comment.
Good catch. An unexpected stream end gives the provider no chance to classify the failure, so those inputs are no longer trusted (not persisted), and the error no longer claims the message was saved.
| // (the next appended message is a user turn). Promote | ||
| // any still-unvalidated inputs so the commit phase | ||
| // persists them (no overflow — safe to keep). | ||
| history.mark_inputs_model_validated(); |
There was a problem hiding this comment.
Same fix, generalized: inputs are promoted on give-up only when the provider positively declares not_caused_by_input() (credentials, network, throttling, 5xx, access). Anything else — including ValidationException, unknown errors, and older providers — keeps main's semantics, and the error tells the user the message wasn't kept so they can resend.
There was a problem hiding this comment.
Nope, even if the issue is due to credentials or others, inputs cannot be promoted because we do not have evidence that they are safe. The best we can do is keep them around but in a separate list of pending untrusted inputs but this would have to be maintained separately.
There was a problem hiding this comment.
Agreed. As of abd3f19, nothing is promoted on failure, so persistence matches main. When the round had unanswered inputs, the give-up error just tells the user "Your message was not kept; please resend it." A durable list of pending untrusted inputs can come later as its own change; it fits with the "durable inbox" #178 lists as its next step.
c2e228b to
abd3f19
Compare
abd3f19 to
d749373
Compare
When a completion could not start or gave up (e.g. expired AWS credentials), users saw nothing: every failure path emitted only `Info`, which some clients drop, and Bedrock retried credential failures silently for about a minute. * New `AgentEvent::Error`, mapped to the existing `DaemonMessage::Error`. The first retry notice of a retry sequence and the final give-up are errors (later retries stay `Info`); driver/step failures too. Give-up still settles spinners via `CompletionFinished`. * Give-up keeps the existing input semantics (unvalidated inputs are not persisted); the error says the message was not kept and asks the user to resend it. * Bedrock: credential-loading failures are `Fatal` (fail fast), and transport errors carry a concise message (a short `mwinit` hint for credentials, otherwise the error kind and root cause) instead of the full SDK error chain. Core shows provider errors verbatim. * `MockModelController::fail_next_stream` to test initiation failures.
d749373 to
ebb2852
Compare
Incident: a user message arrived while AWS credentials were expired. The completion failed at stream initiation about 12 times over a minute, the thread went idle, and the user saw nothing.
Core reports failures as data
AgentEvents; core writes no user-facing wording for them:CompletionRetrying { error, attempt, delay }for each retry.CompletionFailed { error, retries, discarded_user_messages }on give-up. Core knows an input's kind (user text, tool result, …), not who sent it, so it reports a count and leaves the wording to the embedder.Error { error }for driver/step failures outside a completion. These wereInfo { text: "Error: …" }before.main: inputs are never promoted on failure, including after an unexpected stream end.discarded_user_messagescounts the user messages among the round's unvalidated inputs. They are neither persisted nor re-sent on the next round.Daemon writes the wording (
session/display.rs, mapped to the existingDaemonMessage::Error/Info, no protocol change)Errorso a failing completion is never a silent retry loop; later retries areInfo.Completion failed[ after N retries]: <error>.A discarded user message addsYour message was not kept; please resend it.User messages in a daemon session come from its clients, so the daemon can address the reader.CompletionFinished.Bedrock: concise, provider-owned messages
sdk_error_messagekeeps the service's own message when there is one. Otherwise it reports the error kind plus its root cause instead of the full chain. For credential-loading failures, the root cause carries the credential source's own explanation, e.g. acredential_process's stderr. Core forwards the message as-is and does no truncation.Fatal, so they fail fast instead of retrying silently for a minute.Tests
completion_start_failure_reports_discarded_user_message,first_transient_failure_is_surfaced_before_retry.MockModelController::fail_next_streammakes a stream fail at initiation.first_retry_is_an_error_later_retries_are_info,give_up_asks_for_resend_only_when_a_user_message_was_discarded.transport_errors_are_concise.The pinned
should_panicwedge testparent_message_after_fatal_error_during_active_tool_callis unchanged.