🧰 refactor: Pure Step Reducer for Tool Call Events - #16290
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. |
2b0c602 to
ec1170c
Compare
8ffd5f8 to
92d2c28
Compare
![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: 92d2c289e0
ℹ️ 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".
| expect(completedTwice.content).toEqual(message.content); | ||
| expect(toolCallAt(reopened, 1)).toMatchObject({ id: 'call-1', name: 'search' }); | ||
| expect(reopened.content).toHaveLength(2); |
There was a problem hiding this comment.
When ON_RUN_STEP is replayed after completion, applyToolCallsStep performs a non-final updateContent, which rebuilds the tool call without its output, progress, or terminal status. These assertions check only the ID, name, and array length, so the test passes even though the supposedly idempotent replay erases the result and regresses the card from completed; compare the reopened part with the completed part or explicitly assert the terminal fields.
AGENTS.md reference: AGENTS.md:L31-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ecb9642. The test overclaimed: it now asserts only what holds, a duplicate completion leaves the content equal and re-announcing a still-open step keeps its streamed args. A run step replayed after completion does drop the output, but that is canary's behavior, which this refactor keeps unchanged; tracked at berry-13#115.
| const toolCard = (page: Page) => | ||
| messagesView(page).getByRole('button', { name: 'Finished running', exact: true }); |
There was a problem hiding this comment.
When the mocked tool step takes at least one second, as can happen on a loaded CI runner, ProgressText appends the localized screen-reader duration to the button's accessible name, so it becomes something like Finished running took 1 second. This exact-name locator then finds no card and the scenario times out despite correct rendering; match the stable name prefix or locate the tool row without removing the accessible duration.
AGENTS.md reference: AGENTS.md:L31-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ecb9642. The card is matched by the /^Finished running/ name prefix, so a duration suffix no longer breaks the locator; the scenario passes in desktop light, desktop dark and mobile.
| it('fills the slot when a completion arrives before its run step opened it', () => { | ||
| const completed = applyToolCallCompleted( | ||
| createResponse(), | ||
| search, | ||
| toolEnd(search.id, { id: 'call-1', name: 'search', args: { q: 'cats' }, output: '3' }), | ||
| 0, | ||
| ); |
There was a problem hiding this comment.
This test manually supplies the RunStep, so it does not cover the event ordering named by the test. When ON_RUN_STEP_COMPLETED actually arrives before ON_RUN_STEP, useStepHandler cannot find the step in stepMap and returns without buffering the completion, meaning the tool output is still lost; drive this case through the handler or add completion buffering before claiming this ordering is supported.
AGENTS.md reference: AGENTS.md:L31-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ecb9642. The test is renamed to what it covers: the reducer writes a completion into its step slot when the opening part is missing. The handler dropping a completion that arrives before its run step is canary's behavior and unchanged here; buffering it is tracked at berry-13#116.
Add pure reducers for tool_calls run steps, argument deltas, completions and run-step closures, and move the skill-authoring detection with them. useStepHandler keeps the step-to-tool-call-id map and records the id each reducer returns.
Drop the claim that a replayed run step is idempotent after completion and that a completion may arrive before its run step; cover re-announcing an open step instead. Match the code card by its name prefix, since a slow run appends its duration.
ecb9642 to
667d59d
Compare
1 participant
Summary
Completes the pure step reducer behind
useStepHandlerby lifting the tool-call transitions intoclient/src/hooks/SSE/steps/tools.ts: opening atool_callsrun step, argument (and OAuth prompt) deltas, completion, and run-step closure, along with the skill-authoring detection. Stacked on #16289.Each reducer takes the response message, the run step and one event and returns the next message.
applyToolCallsStepalso returns the tool-call id to record, so the hook keeps owning the step-to-tool-call-id map in its ref. Hook signature and behavior are unchanged.Type of change
Testing
Tested environments/configuration:
lcdev servers on this branch, agents endpoint via Anthropicclaude-sonnet-4-6with Run Code enabled. A reasoning turn with four tool calls rendered its parts in order and matched the persisted message after a hard reload; a turn stopped mid-stream matched after reload. Successful code execution was not exercised: the sandbox in my environment was unavailable, so the calls completed with an error output.Automated tests:
steps.spec.tswith the tool-call family: open, stream args, complete, duplicate run step and duplicate completion, completion before open, OAuth prompt on a delta and its displacement, closure only stamping tool calls.cd client && npx jest hooks/SSE hooks/Chat Providers: 28 suites, 778 tests pass;useStepHandler.spec.tsis unchanged.cd client && npm run typecheck: passes.npm run static-checks -- --against origin/canary: passes.Screenshots / recordings
No user-facing change.
Risk / compatibility
Behavior-preserving refactor of the tool-call streaming path.
Checklist