🐛 A run whose only model call the provider rejected is a failure, and says why - #232
Conversation
… says why
goose renders a rejected model call as assistant prose ("Ran into this
error: … The security token included in the request is invalid.") and
ends the turn with end_turn. The harness already recognised that text
(konveyor#170) but only logged a WARN and told viewers to "see the pod log";
the stage exited 0, so the run showed Succeeded in green. Three people
read a dead Bedrock credential as a broken plan ladder today (konveyor#231).
Harness: classifyOutcome takes providerError; with TurnsUsed == 0 the
stage is outcomeFailed (nothing happened). The termination log's
stopReason carries "provider error: <first line of the provider's
message>", already redacted, and the viewer notice quotes the same line
instead of pointing at a log the console user cannot read. A provider
error after real work keeps the turn's outcome (konveyor#129's contract) but
still gets the notice.
Controller: setTerminalOutcome pasted the whole termination message
into the Succeeded=False condition — since konveyor#189 that message is the
ADR 0011 JSON blob, so the console showed JSON. humanFailureMessage
extracts stopReason from the blob, leaves plain text (konveyor#143) as-is, and
yields "" for a blob with no stopReason so the generic exit-code text
stands.
Closes konveyor#231
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: ibolton336 <ibolton@redhat.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
djzager
left a comment
djzager
left a comment
There was a problem hiding this comment.
Please preserve the detected provider error in the summary and keep harness-specific termination JSON interpretation outside the controller, consistent with the documented contract.
| providerRejected := providerErr && primaryResult.TurnsUsed == 0 | ||
| var providerSummary string | ||
| if providerErr { | ||
| providerSummary = providerErrorSummary(red.redact(primaryResult.FinalMessage())) |
There was a problem hiding this comment.
Could we select the provider-error message or line before shortening it? FinalMessage() can include narration preceding the error—TestLooksLikeProviderError already covers “Here is the plan:” followed by a ThrottlingException. The summary currently returns the narration, so the viewer notice and failure condition lose the error that explains what went wrong. Please cover that scenario in the summary tests.
There was a problem hiding this comment.
Fixed in aa8b6aa. acp.ProviderErrorText now picks out the failure text before shortening, skipping narration goose streamed ahead of it, and LooksLikeProviderError is built on it so detection and selection can't drift. TestProviderErrorSummary covers the "Here is the plan:" + ThrottlingException case, narration in an earlier message, and the trailer form.
Testing against #231's real Bedrock line also showed the 200-rune cut landing mid-reason (goose's Debug rendering of the AWS SDK error puts "The security token included in the request is invalid." past rune 160), so the cap is now 1000 runes, still well inside the termination log.
| return raw | ||
| } | ||
| var blob struct { | ||
| StopReason string `json:"stopReason"` |
There was a problem hiding this comment.
Could we keep this extraction in Hub/UI, where the harness’s termination schema is understood? CONTEXT.md and ADRs 0011/0018 explicitly keep termination JSON opaque to the controller. This helper assumes every JSON object uses stopReason, so another harness’s {"error":"clone failed"} becomes a generic exit-code message. If we want a shared failure-message field instead, we should define that contract and amend the docs explicitly.
There was a problem hiding this comment.
Agreed, dropped in 52dff65. humanFailureMessage and its tests are gone and setTerminalOutcome is back to its base behaviour, so the termination JSON stays opaque per ADR 0011/0018. The harness still puts the provider error in stopReason; rendering it from terminationData will be a separate tackle2-ui change rather than a new shared field here.
providerErrorSummary took the first line of the closing message, but
goose can stream text ahead of its failure in the same entry ("Here is
the plan:" then "Ran into this error: ThrottlingException"), and
FinalMessage joins earlier messages in front of it, so the notice and
stopReason quoted the narration. acp.ProviderErrorText returns the
failure text itself, and LooksLikeProviderError is built on it so
detection and selection cannot drift.
The 200-rune cut also lost the reason on konveyor#231's own line: goose renders
the AWS SDK error with Debug formatting, ~500 runes with "The security
token included in the request is invalid." past rune 160. The cap is
now 1000 runes, still well inside the 4096-byte termination log.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: ibolton336 <ibolton@redhat.com>
ADR 0011, ADR 0018 and CONTEXT.md keep the harness's termination JSON uninterpreted by the controller, and humanFailureMessage assumed every blob carries stopReason, so another harness's JSON would lose its message. setTerminalOutcome copies the termination message as before; the harness's stopReason reaches the UI through terminationData. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
djzager
left a comment
djzager
left a comment
There was a problem hiding this comment.
Re-reviewed the updates. Both requested changes are addressed: provider summaries select the actual error instead of preceding narration, and termination JSON remains opaque to the controller. The original reproduction and full harness test suite pass. No further actionable findings.
## What
When an AgentRun fails, the controller copies the agent container's
whole termination message into the `Succeeded=False` condition. Since
the harness began writing that message as the ADR 0011 JSON blob
(agentic-controller#189), the console printed raw JSON beside the Failed
phase:
```
Failed {"exitCode":1,"outcome":"failed","stopReason":"provider error: Ran into this error: Failed to call Bedrock: ...","usage":{"turnsUsed":0}}
```
This reads the harness's `stopReason` — the only free-text field in its
schema — and shows that instead:
```
Failed provider error: Ran into this error: Failed to call Bedrock: The security token included in the request is invalid.
```
## Why in the UI
ADR 0011/0018 keep `status.terminationData` opaque to the controller: "a
different harness writes different data. The Konveyor UI knows how to
read the Konveyor harness's schema." The review on
konveyor/agentic-controller#232 asked for that split explicitly, so the
extraction lives here rather than in the controller.
## Changes
- `client/src/app/api/agentic/contract.ts` — `HarnessTerminationData`
(every field optional, since another harness may write a different
object) and `AgentRunStatus.terminationData`.
- `client/src/app/pages/agent-runs/components/RunConditionSummary.tsx` —
new `explanatoryMessage(condition, terminationData)`, used by the
summary beside the phase and by the run table's Reason tooltip.
- The agent run detail page and the agent runs page pass
`status.terminationData`.
- 10 unit tests for the message selection.
## Scope
The substitution applies only to `Succeeded=False` with reason `Failed`,
the one branch that carries the termination message. `LimitReached`
keeps the controller's own "Execution limit reached; the agent committed
a handoff" — its blob's `stopReason` is the ACP stop reason
(`end_turn`), which would otherwise replace that sentence. Plain-text
messages (agentic-controller#143) and JSON from a harness with another
schema are shown exactly as today.
Not covered here: a failed **workflow** run still shows `Stage "x" did
not succeed: {…}`. That condition is `Ready=False`/`StageFailed` and
`AgentWorkflowRunStatus` carries no `terminationData`, so surfacing the
stage's reason needs the failed stage's AgentRun fetched — a separate
change.
No Hub change is needed: Hub pins `agentic-controller/api` at
`22fee493`, which already has `TerminationData`, and returns the object
as-is.
## Verification
- `npm run tsc -w client` clean
- `npm run test -w client` — 48 suites / 557 tests pass (10 new)
- `npm run lint` — 0 errors, warnings unchanged at the repository's 20
- `npm run format:check -w client` clean
- Browser check against a mock hub serving four failed runs: the
provider-error run shows the sentence instead of the blob, the
limit-reached run keeps the handoff text, plain text and another
harness's `{"error":"clone failed"}` are unchanged, and the run table's
Reason tooltip shows the sentence. No console errors at desktop or
768px.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
- **Improvements**
- Failed agent runs now display a clear stop reason instead of raw
termination data.
- Agent run lists use consistent explanatory failure messages.
- Condition messages remain available for successful runs, limit-reached
runs, and cases without a specific stop reason.
- Run status details now include termination information such as
outcome, usage, and stop reason.
- **Documentation**
- Clarified gateway selection rules for agent runs, including behavior
when gateways are declared, omitted, or limited to one option.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Signed-off-by: ibolton336 <ibolton@redhat.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2 participants
Closes #231.
Problem
goose renders a rejected model call as assistant prose and ends the turn normally:
The harness recognised that text since #170 but only logged a WARN and told viewers to "see the pod log". Exit 0,
stopReason=end_turn, run Succeeded in green, Gateway Ready (Bedrock cannot be probed, #222). On the demo cluster today three runs died this way on a rotated AWS key and were read as a broken plan ladder, because nothing anyone could see said otherwise.Change
Harness
classifyOutcometakesproviderError(the existingPromptResult.ClosingProviderError()). WithTurnsUsed == 0the stage isoutcomeFailed: the model never worked, so nothing succeeded. A provider error after real work keeps the turn's outcome — that is Run outcomes: refusal and no-op must not report bare Succeeded #129's broader contract — but still gets the notice below.stopReasonbecomesprovider error: <the provider's error line>(redacted, cut at 1000 runes). The line is the failure itself:acp.ProviderErrorTextskips narration goose streamed ahead of it ("Here is the plan:" … "Ran into this error: …"), andLooksLikeProviderErroris built on it, so detection and selection cannot drift. The cap is sized for Harness: a run whose only model call is rejected by the provider ends Succeeded #231's real line: goose renders the AWS SDK error with Debug formatting, ~500 runes with "The security token included in the request is invalid." past rune 160, which a 200-rune cut lost.Controller
Unchanged. The termination blob stays opaque to the controller (ADR 0011, ADR 0018,
CONTEXT.md): it is stored verbatim onstatus.terminationData, andsetTerminalOutcomecopies the termination message onto theSucceeded=Falsecondition as before. ShowingterminationData.stopReasonbeside the phase instead of the raw blob belongs in the UI, which knows the harness schema — a tackle2-ui follow-up.Net effect: the run is Failed instead of Succeeded, and the provider's error line — the sentence that tells the operator to rotate the Gateway credential — is in the viewer notice and in the termination blob's
stopReason, which the console shows raw beside the phase until the UI renders it.Exit-code contract (ADR 0011) is unchanged: this is an exit-1 failure, not a new code. #170's "exit status is unchanged on purpose … belongs with the Succeeded condition work in #119" deferral is what this resolves; #119 landed in #169.
Verification
gofmt,go vet,go test ./...green inharness/;go vetplusTestSetTerminalOutcome,TestPodTerminationMessage,TestTerminationDataFromPodgreen ininternal/controller/.TestClassifyOutcome— provider error with no turns → failed; after 7 turns → succeeded; at the native limit → limitReached still wins.TestProviderErrorText— the failure without the narration ahead of it, in both of goose's forms; a quoted phrase is not a failure.TestProviderErrorSummary— narration in the same or an earlier message, the trailer form, Harness: a run whose only model call is rejected by the provider ends Succeeded #231's Bedrock line kept whole, the 1000-rune cut.🤖 Generated with Claude Code