Repository navigation
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.
| const cases = JSON.parse( | ||
| execFileSync("uv", ["run", "--frozen", "--no-sync", "python", "-c", python], { | ||
| cwd: root, | ||
| encoding: "utf8", | ||
| env: { | ||
| ...process.env, | ||
| PYTHONPATH: [root, path.join(root, "examples"), process.env.PYTHONPATH] | ||
| .filter(Boolean) | ||
| .join(path.delimiter), | ||
| }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
🟡 nit (optional): contributors running node --test get a Python subprocess spawned at module load, which the repo's test rules reserve for a last resort. tests/docs_src/test_apps.test.js:40 shells out to uv run --frozen --no-sync python -c before any test is registered, so a missing uv or an unsynced .venv fails the whole file with a spawn error instead of a test assertion. Fix: keep the JS test self-contained by reading CLOCK_HTML out of docs_src/apps/tutorial001.py and examples/stories/apps/server.py as text and pinning the expected tool-result text, leaving the server-side result shape to the in-memory tests in tests/docs_src/test_apps.py.
Why this was flagged
A developer runs node --test tests/docs_src/test_apps.test.js locally without having run uv sync, or without uv on PATH. At tests/docs_src/test_apps.test.js:40-51 the module-level execFileSync("uv", ["run", "--frozen", "--no-sync", "python", "-c", python], ...) runs before any test() is registered, so the failure surfaces as an uncaught spawn or ImportError for the file rather than a named test result. On the base branch there is no such file; tests/docs_src/test_apps.py already drives both tutorials in-memory. AGENTS.md says to prefer in-memory execution and use subprocesses only as a last resort, and the delegated test-quality guide says subprocesses are for when the process boundary is itself under test; here the boundary is incidental, since the only Python-sourced inputs (the HTML string and the expected text) are static literals the Node side could read from the source files. In CI the prior uv sync --frozen --all-extras --python 3.10 step at .github/workflows/shared.yml:27 makes the subprocess succeed, so nothing breaks there.
Verification: Anyone runs node --test tests/docs_src/test_apps.test.js where uv is not on PATH or the .venv has not been synced. tests/docs_src/test_apps.test.js:39-51 executes execFileSync on uv at module top level, so a missing uv throws out of module evaluation and the whole file errors rather than reporting a named failing test. The added code violates AGENTS.md:75-76 as written.
| }, | ||
| }), | ||
| ); | ||
|
|
There was a problem hiding this comment.
🟡 nit (optional): maintainers get a test that pins the params wire shape without saying where that shape comes from. tests/docs_src/test_apps.test.js:52 names the behaviour but neither the test nor the file says the payload under params is the ext-apps spec's ui/notifications/tool-result notification (spec-mandated), which is exactly the params vs result confusion behind #3662. Fix: add a one-line comment or test description stating the provenance, e.g. that ui/notifications/tool-result carries a CallToolResult as its JSON-RPC params per the MCP Apps spec, so a future reader knows a failure here is a regression and not an SDK choice.
Why this was flagged
The condition is a later reader or agent editing the clock HTML or this test and asking whether the host posts the tool result under params or result; they arrive via tests/docs_src/test_apps.test.js:51-75. The test at tests/docs_src/test_apps.test.js:72 dispatches { jsonrpc, method: "ui/notifications/tool-result", params: result } and asserts at :73, but no docstring or comment states that this shape is mandated by the ext-apps spec rather than invented for the test. On the base branch there is no JS test at all, so nothing is lost at runtime; the gap is that the new test does not carry the triage key the repository's test-quality bar asks for (AGENTS.md:68 incorporates .claude/skills/test-quality/SKILL.md, whose bullet at SKILL.md:16-19 asks docstrings to be honest about provenance: spec-mandated, SDK-defined, or pinned gap). No safeguard covers this: pyright, ruff and pytest do not read this file, and the Python test tests/docs_src/test_apps.py does not exercise the notification shape.
Verification: nit. A later reader of tests/docs_src/test_apps.test.js asks where the params-carrying ui/notifications/tool-result shape comes from. The file has no header or per-test comment; the only statements of the wire shape are the test name at :52 and the dispatch at :72. Nothing in the file says this shape is the ext-apps spec's notification rather than a test-invented payload. SKILL.md:16-19 requires stating provenance.
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.
This branch has not been deployed
1 participant
Summary
Read the MCP Apps tool-result notification from
paramsin the clock tutorial and story server. JSON-RPC notifications do not carry aresultfield.Fixes #3662
Checks
uv run --frozen ruff check docs_src/apps/tutorial001.py examples/stories/apps/server.pyuv run --frozen ruff format --check docs_src/apps/tutorial001.py examples/stories/apps/server.pyuv run --frozen python -m py_compile docs_src/apps/tutorial001.py examples/stories/apps/server.pyAI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.