Repository navigation
🗳️ feat: Approval-Responded, Output-Denied and Ask-User Parts in the UI Message View - #16961
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0f9650711
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (approval?.approved === false && (ended || failure)) { | ||
| state = 'output-denied'; |
There was a problem hiding this comment.
When a completed respond decision carries the user's response in tool_call.output—the resume contract explicitly produces this shape in api/server/controllers/agents/__tests__/resume.spec.js:4265-4271—this branch classifies it as output-denied, and line 527 consequently omits the output. The resulting UIMessage loses the human-supplied tool result, leaving facade/useChat consumers unable to render or serialize it; distinguish respond from reject and preserve its output.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4f326dc (next push): an ended respond decision now reads output-available and keeps the reviewer's output, covered by the new respond case in parts.spec.ts.
| if (part.type === 'data-ask-user-question') { | ||
| return { type: ASK_USER_QUESTION, ...part.data } as unknown as TMessageContentParts; |
There was a problem hiding this comment.
For any TypeScript consumer that round-trips data-ask-user-question, this function returns an ask_user_question object while declaring it as TMessageContentParts, whose union does not contain that discriminator. Callers therefore cannot safely narrow or construct TMessage.content with the newly exported part type, and the client still needs unknown casts to use it. Move the shared part shape into the content types and include it in TMessageContentParts rather than hiding the mismatch behind this cast.
AGENTS.md reference: AGENTS.md:L174-L179
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to berry-13#320: the synthetic ask part already lives in TMessage.content outside the union on dev (client/src/utils/approval.ts), the round trip is lossless at runtime, and widening TMessageContentParts reaches every content switch in the client, so it gets its own change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f326dcec7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| const { actionId, description } = toolCall.approval; | ||
| return { id: actionId, ...(description && { requestReason: description }) }; | ||
| const { actionId, description, decision, reason } = toolCall.approval; |
There was a problem hiding this comment.
At reviewed head 4f326dcec76afa3831268d83327d06da51bafea2, production tool calls never populate this new decision field: the resume contract persists only the human-authored output (api/server/controllers/agents/__tests__/resume.spec.js:4265-4278), and the final SSE merge rebuilds the tool call without preserving approval (client/src/hooks/SSE/steps/content.ts:319-336). Thus decision is undefined here for real resumed calls, so a rejected completion is still classified as output-available and the new approval-responded/output-denied states are reachable only in the synthetic tests; carry the submitted decision into the resumed tool-call record before deriving these states.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct that no producer writes the decision yet; without it the mapping is unchanged from dev, so nothing regresses. Persisting it spans the client stamp, the SSE merge and the server resume, so it is tracked in berry-13#321.
| const responded = 'name' in toolCall && toolCall.approval?.decision === 'respond'; | ||
| if (approval?.approved === false && !responded && (ended || failure)) { | ||
| state = 'output-denied'; | ||
| } else if (failure) { |
There was a problem hiding this comment.
When a reviewer supplies text matching the legacy failure heuristic—such as Error: tool call failed: ... or a value beginning Error processing tool—resolveFailure marks the human response as failed before this branch, so a completed respond decision becomes output-error and its text is moved to errorText instead of remaining the successful substitute tool output. The agents SDK's respond path constructs a successful tool result from arbitrary reviewer text, so the respond exception must also bypass output-text/caller failure inference unless the call carries an explicit operational failure marker.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3f1e324: a respond decision now uses only the explicit failure markers (failed or cancelled step, background cancel, input validation) and skips the output heuristic and the caller resolver; parts.spec covers it and fails without the fix.
3f1e324 to
8d12994
Compare
Summary
The
UIMessageview inlibrechat-data-provider(the parts projection the chat facade hands touseChatconsumers) covered 5 of the AI SDK's 7 tool states and had no part for an ask-user pause: a reviewed tool call could only read asapproval-requested, and the client-writtenask_user_questioncontent part mapped tostep-start, sofromUIMessagedropped it and left a hole. That keeps the facade surface short ofuseChatparity for approvals.Tool parts now follow
ai@7.0.137: a call whose stored approval carries adecisionreads asapproval-responded(withapprovedandreason), a denied call that ended reads asoutput-denied, and an approved call keeps itsapprovalthroughoutput-availableoroutput-error. The decision is a new optionaldecision/reasonpair on the stored approval; nothing writes it yet, so current messages map exactly as before. The ask-user pause becomes adata-ask-user-questionpart that keeps its content slot and round-trips losslessly, and its type moves into data-provider so the client reuses it. Thepending_actiontransport comment now names both mappings.Type of change
Testing
Tested environments/configuration:
Unit level only; no runtime surface changes, since no producer writes
decisionyet and the ask part renders from the same content as before.Automated tests:
packages/data-provider:npx jest(68 suites, 2788 passed), including newparts.spec.tscases: each decision kind asapproval-responded,output-deniedfrom an output or a failed step, approved calls through output and failure, the unchanged undecided-approval case, hand-built parts rebuilt without a base, and the ask pause throughtoUIPart,toUIParts(slot kept) andtoUIMessage/fromUIMessagewith and without a base, alltoStrictEqual.client:facade.spec,transport.spec,approval.spec,LiveParity,ToolCallGroup,useStepHandler.spec(6 suites, 422 passed);tsc --noEmitonclientandpackages/data-providerclean.Screenshots / recordings
No user-facing change.
Risk / compatibility
Agents.ToolCall.approvalgains two optional fields; stored messages and existing producers are unaffected.Checklist