Skip to content

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

Closed
kojiwakayama wants to merge 2 commits into
mainfrom
fix/tool-result-parent-message-id
Closed

kojiwakayama wants to merge 2 commits into
mainfrom
fix/tool-result-parent-message-id

Conversation

@kojiwakayama

@kojiwakayama kojiwakayama commented Sep 14, 2026 •

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 (#4485)

The conversation run event encoder named the assistant message on
TOOL_CALL_START but not on TOOL_CALL_RESULT, so the API recovered a
result's turn from the synthetic `<assistantMessageId>:tool:<toolCallId>`
message id at ingest. 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 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
@chatgpt-codex-connector

Copy link
Copy Markdown

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

@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.

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@coderabbitai

coderabbitai Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 43 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: ec13b25f-e351-4041-9829-6b07fc006b4e

📥 Commits

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

📒 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.

@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.

@gitar-bot

gitar-bot Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Gitar is working

Gitar

Copy link
Copy Markdown
Contributor

Review: 92/100 — solid, minimal, well-tested fix

Summary: Small, focused change that mirrors an existing pattern (TOOL_CALL_START's parentMessageId) onto TOOL_CALL_RESULT, with matching schema and test coverage on both sides of the encoder/payload boundary.

Strengths

  • Correctness: ...(this.activeMessageId ? { parentMessageId: this.activeMessageId } : {}) on all four terminal branches (tool-input-error, tool-output-available, tool-output-error, tool-output-denied) is copy-identical to the existing tool-input-start handling (run-events.ts:302), so behavior is consistent across call-start and call-result rather than reinventing the rule.
  • Schema: parentMessageId added as optionalString on getToolCallResultPayloadSchema, matching getToolCallStartPayloadSchema's field exactly (required non-empty string when present) — good contract symmetry.
  • Tests are genuinely useful, not just coverage padding: parametrized over all four branches, checking both the "open message" and "no active message" cases, and pinning the untouched synthetic messageId shape alongside the new field. payload.test.ts also checks the reject cases (number, empty string).
  • Scope discipline: PR description explicitly calls out lifecycle-run-event-adapter.ts as intentionally untouched and explains why the contract fixture doesn't need a hash bump — the right amount of "why not" documentation for a change like this.
  • Backward compatible: field is optional and additive; passthrough schemas mean older consumers ignore it, newer ones (API) already accept it per the PR description.

Minor / non-blocking

  • No changelog entry (.changeset/*) included in the diff — worth double-checking whether this repo requires one for src/ behavior changes, even for an additive field.
  • The claim that the API's TOOL_CALL_RESULT schema on veryfront-api main already accepts parentMessageId isn't independently verifiable from this diff — if that's wrong, the field would be silently dropped there rather than erroring, so worth a quick cross-check before relying on it downstream.

Given the change is a narrow, additive, well-tested consistency fix with no behavioral risk to existing consumers, this is close to a model small PR. Nothing here blocks merge; the two notes above are just things to confirm, not required changes.


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: feb3ebbd46

ℹ️ 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".

Comment thread src/run-events/payload.ts
… reference

getToolCallResultPayloadSchema is public, so its JSDoc and the generated
API reference now say what `parentMessageId` means, matching the tool call
start entry (codex review on #4492).

Claude-Session: https://claude.ai/code/session_01CqY82FYm9aUkyawWmh4T9n
@chatgpt-codex-connector

Copy link
Copy Markdown

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

@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.

@kojiwakayama

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: a68defeca6

ℹ️ 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".

@kojiwakayama kojiwakayama changed the title fix(run-events): tool results carry parentMessageId like tool call starts (#4485) fix(run-events): tool results carry parentMessageId like tool call starts Sep 14, 2026
@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
@kojiwakayama
kojiwakayama removed this pull request from the merge queue due to a manual request Sep 14, 2026
@kojiwakayama
kojiwakayama added this pull request to the merge queue Sep 14, 2026
@kojiwakayama

Copy link
Copy Markdown
Contributor Author

Replaced by #4494, same change as a single commit.

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a manual request Sep 14, 2026
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