Repository navigation
Read MCP Apps tool results from notification params - #3679
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. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
📚 Documentation preview
|
![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: bf951daadd
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding (which is pre-existing in the example, not introduced here), I also checked that params.content[0].text matches the ext-apps ui/notifications/tool-result notification shape, that these two embedded listeners are the only event.data readers in the repo, and that the prose in docs/advanced/apps.md never names the old result path, so no doc text needs updating.
Extended reasoning...
The diff is a one-line change in two embedded HTML strings (docs_src/apps/tutorial001.py and examples/stories/apps/server.py) switching the clock example's postMessage listener from event.data.result to event.data.params, which is the correct payload location for a JSON-RPC notification. No Python, SDK, or test code changes and line counts are preserved so the hl_lines references in docs/advanced/apps.md still align. The only security-relevant surface is the example iframe's unchecked message origin, which predates this PR and is covered by the inline finding.
There was a problem hiding this comment.
LGTM — the latest push adds the ui/initialize handshake and the parent-source/method guard, which closes the one substantive point from the earlier review; the remaining inline notes are optional nits.
What was reviewed:
- The handshake in both clock scripts (request id 1 with
appInfo/appCapabilities/protocolVersion,initializednotification on the response, tool-result gated behindinitialized) matches the ext-apps flow, and the two files are byte-identical in their script bodies. - The shifted
hl_linesin docs/advanced/apps.md (36 39 47 49 and 40-44) line up withApps(),@ apps.tool,add_html_resource,MCPServer(...)and theget_timebody in the new docs_src/apps/tutorial001.py. - All twelve i18n pages carry the same two new section hashes (sections 1 and 2, the only ones whose English text changed) with the rest unchanged, consistent with translate-tool output rather than hand edits.
- The new CI step runs in the pre-commit job after
uv sync --frozen --all-extras, so theuv run --frozen --no-syncsubprocess in the Node test has a synced environment there; README does not embed this snippet, so no snippet regeneration is needed.
Extended reasoning...
The change rewrites the inline iframe JavaScript in the docs snippet docs_src/apps/tutorial001.py and the example examples/stories/apps/server.py to perform the MCP Apps UI handshake and read tool results from notification params, shifts hl_lines in docs/advanced/apps.md and the twelve generated i18n copies, adds a Node vm-based test in tests/docs_src/test_apps.test.js, and wires that test into .github/workflows/shared.yml. It touches no SDK runtime code and no auth, crypto or permission surface; the only security-relevant aspect is the iframe listener, which now checks event.source against window.parent and filters on method, addressing the earlier inline finding. The diff is small and self-contained, the handshake is exercised deterministically by the new test, the hl_lines and i18n hashes check out against the new English content, there is no CODEOWNERS file, and no third-party objection is outstanding, so approval is warranted; the two open inline nits from the prior run are optional and do not block.
Still open from earlier reviews (2):
- Unresolved: 2 minor or pre-existing.
There was a problem hiding this comment.
LGTM — the follow-up commits address the points from my earlier passes (source check on the listener, self-contained Node test, spec provenance in the test name), and the remaining change is docs/example-only.
What was reviewed:
- Clock HTML in docs_src/apps/tutorial001.py and examples/stories/apps/server.py: the ui/initialize -> ui/notifications/initialized -> ui/notifications/tool-result sequence, with tool-result read from params and gated on event.source === parent and the initialized flag; both copies are identical.
- hl_lines shifts in docs/advanced/apps.md (36 39 47 49 and 40-44) traced against the new 50-line snippet — they land on Apps(), @ apps.tool, add_html_resource, MCPServer and the get_time body as before; the surrounding prose still describes the listener accurately.
- The twelve i18n page edits are limited to the same hl_lines values plus identical section-hash updates across every language, consistent with translate-tool output rather than hand edits.
- tests/docs_src/test_apps.test.js: walked the vm sandbox flow by hand — the ignore-before-init, foreign-source and other-method assertions each run against a still-unchanged textContent, so they are not hollow.
Extended reasoning...
The diff rewrites the inline JavaScript of the MCP Apps clock example in two Python-embedded HTML strings so the iframe performs the ui/initialize handshake and reads the tool result from the notification's params, shifts hl_lines in the English docs page and twelve generated translations, adds a Node vm test for the script, and wires that test into shared.yml. No SDK source code changes; the only security-relevant surface is the example postMessage listener, which now checks event.source against window.parent and gates on the initialized flag. Approving because the change is small and self-contained, my three prior inline findings are each visibly addressed by later commits (3950806 and 67cb81a), the only non-bot review in the timeline is dismissed, there is no CODEOWNERS file, and the hunt ran dry rather than hitting a budget bound.
2 participants
Summary
Read MCP Apps tool-result notifications from
paramsin the clock tutorial and story server. Initialize the UI bridge before consuming notifications, and accept tool results only from the parent window.Exercise both embedded scripts with self-contained Node tests. Update the documentation line highlights and translations to match the expanded examples.
Fixes #3662
Checks
node --test tests/docs_src/test_apps.test.js- 2 passeduv run --frozen pytest tests/docs_src/test_apps.py -q- 7 passeduv run --frozen ruff check docs_src/apps/tutorial001.py examples/stories/apps/server.py tests/docs_src/test_apps.pyuv run --frozen pyright- no errorsAI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.