Skip to content

fix(run-events): tool results carry parentMessageId like tool call starts - #4494

Merged
kojiwakayama merged 1 commit into
mainfrom
fix/tool-result-parent-message-id-v2
Sep 14, 2026
Merged

kojiwakayama merged 1 commit into
mainfrom
fix/tool-result-parent-message-id-v2

Conversation

@kojiwakayama

Copy link
Copy Markdown
Contributor

Why

The conversation run event encoder already names the assistant message on TOOL_CALL_START (parentMessageId). It did not do the same on TOOL_CALL_RESULT, so the API had to work out a tool result's turn from the made-up message id <assistantMessageId>:tool:<toolCallId> when it ingests the event. Recovering it that way is a stopgap. The encoder should state the parent directly, the same way it does for starts.

What changed

  • getToolCallResultPayloadSchema (src/run-events/payload.ts) now has an optional parentMessageId field. It must be a non-empty string, the same rule TOOL_CALL_START uses.
  • ConversationRunEventEncoder (src/agent/conversation/run-events.ts) sets parentMessageId to the active assistant message id on all four places that emit a tool result: tool-input-error, tool-output-available, tool-output-error and tool-output-denied. When no message is open, the key is left out, which matches TOOL_CALL_START.
  • The public JSDoc for getToolCallResultPayloadSchema and the generated docs/api-reference/veryfront/run-events.md now describe parentMessageId.
  • The made-up messageId is unchanged, because consumers and the API's ingest-time recovery depend on its format.

Contract fixture

No change. tests/fixtures/contracts/native-run-events.json only covers the native frame types (for tool calls, that is TOOL_CALL_STATUS_CHANGED) and has no TOOL_CALL_RESULT sample. The fixture and its SHA-256 hash stay as they are, so veryfront-api needs no companion fixture PR. The API's typed TOOL_CALL_RESULT schema on its main branch already accepts an optional parentMessageId.

Tests

  • src/agent/conversation/run-events.test.ts: for each of the four branches, one test checks that parentMessageId equals the active message id inside an open message, and another checks that the key is missing when no message is open. Both tests also pin the made-up messageId. These failed before the change and pass after it. The existing exact-shape test now includes the new key.
  • src/run-events/payload.test.ts: the schema accepts a string parentMessageId and rejects a number or an empty string. This failed before the change, because the passthrough schema let a number through.
  • Every other test that uses the encoder or the fixture passes: native-run-events, hosted-lifecycle, legacy-run-read-adapter, run-chunk-mirror, run-stream-mirror, durable-run-event-sink, request-protocol, vocabulary, chat/conversation, durable, run-event-normalization, lifecycle-run-event-adapter, sse-parser, api-contract-fixture, and the two fixture contract tests.

Out of scope

lifecycle-run-event-adapter.ts builds its own tool records and uses a different shape: messageId on starts and no parent on results. This PR leaves it unchanged; any change there is a separate piece of work.

https://claude.ai/code/session_01CqY82FYm9aUkyawWmh4T9n

…arts

The conversation run event encoder named the assistant message on
TOOL_CALL_START but not on TOOL_CALL_RESULT, so a consumer had to recover
a result's turn from the synthetic `<assistantMessageId>:tool:<toolCallId>`
message id. State it explicitly instead: the tool-result payload schema
gains an optional non-empty `parentMessageId`, and all four result
branches (tool-input-error, tool-output-available, tool-output-error,
tool-output-denied) set it to the active message id when one is open.
The synthetic messageId is unchanged.

The schema's JSDoc and the generated API reference describe the field.
The native run events contract fixture has no TOOL_CALL_RESULT sample,
so it and its pinned hash are unchanged.

Claude-Session: https://claude.ai/code/session_01CqY82FYm9aUkyawWmh4T9n
@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for security reviews. Please try again later.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 22 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 35407559-30fb-49a1-8f1f-b078eceab1c0

📥 Commits

Reviewing files that changed from the base of the PR and between f105ec1 and db59571.

📒 Files selected for processing (5)
  • docs/api-reference/veryfront/run-events.md
  • src/agent/conversation/run-events.test.ts
  • src/agent/conversation/run-events.ts
  • src/run-events/payload.test.ts
  • src/run-events/payload.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your trial has ended. Reactivate Greptile to resume code reviews.

@gitar-bot

gitar-bot Bot commented Sep 14, 2026

Copy link
Copy Markdown

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

Gitar

@github-actions

Copy link
Copy Markdown

📦 Client bundle boundary

Entrypoint Modules Source size Server leaks
src/index.client.ts 289 2307 KiB ✅ 0

A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in scripts/lint/client-bundle-baseline.json to burn down.

Copy link
Copy Markdown
Contributor

Review: 92/100 — excellent

Small, well-scoped bugfix that does exactly what it says: TOOL_CALL_RESULT now carries parentMessageId the same way TOOL_CALL_START already did, removing the API's need to reverse-engineer a result's turn from the synthetic <assistantMessageId>:tool:<toolCallId> message id.

Strengths

  • Correctness verified by reading: all four result branches (tool-input-error, tool-output-available, tool-output-error, tool-output-denied) use ...(this.activeMessageId ? { parentMessageId: this.activeMessageId } : {}), an exact mirror of the existing TOOL_CALL_START pattern a few lines up — no drift between the two.
  • Test coverage is thorough: new tests cover both the "inside an open message" and "no message open" cases for all four branches, plus schema-level tests in payload.test.ts asserting the field accepts a string and rejects a number/empty string. The existing exact-shape test was updated rather than left stale.
  • Backward-compatible: the field is optional and omitted (not null/undefined-valued) when absent, matching how TOOL_CALL_START behaves — no shape change for existing consumers.
  • Good scope discipline: PR description explicitly calls out why the contract fixture doesn't need touching (no TOOL_CALL_RESULT sample in it) and explicitly excludes lifecycle-run-event-adapter.ts's different shape as out of scope rather than silently leaving it inconsistent.
  • Docs (JSDoc + generated run-events.md) updated alongside the schema change.

Minor nit (non-blocking)

  • The ...(this.activeMessageId ? { parentMessageId: this.activeMessageId } : {}) spread is now duplicated 5 times (1 start + 4 result branches) in run-events.ts. Pre-existing style, not introduced by this PR, but now would be a reasonable time to extract a small parentMessageIdField() helper if you want to avoid a 6th copy-paste next time this pattern is touched. Not asking for a change here.

No security, design, or test-coverage concerns. CI was still running several jobs at review time (coverage shards, lint, typecheck, integration/e2e) with everything completed so far green — worth a final glance before merge, but nothing in the diff itself suggests risk there.


Generated by Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. You're on a roll.

Reviewed commit: db59571336

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@sonarqubecloud

Copy link
Copy Markdown

@kojiwakayama
kojiwakayama added this pull request to the merge queue Sep 14, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 14, 2026
@kojiwakayama
kojiwakayama added this pull request to the merge queue Sep 14, 2026
Merged via the queue into main with commit 6874835 Sep 14, 2026
60 checks passed
@kojiwakayama
kojiwakayama deleted the fix/tool-result-parent-message-id-v2 branch September 14, 2026 07:27
@kojiwakayama kojiwakayama mentioned this pull request Sep 16, 2026
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants