examples(mcp-app): convention-first routes own the status server, resource, and App (#726) - #735
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. |
113c246 to
81e0b43
Compare
There was a problem hiding this comment.
💡 Codex Review
agent-bundle/website/docs/en/examples/mcp-app.mdx
Lines 92 to 94 in 113c246
After this example becomes a conventional App route, selecting the App does not immediately render a live preview: AppRouteWorkspace initially shows “Call the bound tool to render the App with its result,” and the user must choose a service and click Call tool and preview. This English walkthrough therefore prevents readers from reproducing the populated state; update it to describe the invocation step, as the corresponding Chinese walkthrough does.
AGENTS.md reference: AGENTS.md:L139-L142
ℹ️ 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".
…ow the conventional mcp-app
7990a50 to
7793244
Compare
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Current-head example review — 77932441aacc9f92f241e83cf13495894a7d8f7a
No additional concrete code blocker found in the inspected migration. Complete/record the actual final-head gate and self-review before merge. Reviewed the changed-file inventory, conventional tool/resource/App modules, shared domain records, client typing, package/lockfile changes, browser and source-deleted web tests, and the updated walkthrough. Repository/browser tests were not run by this reviewer.
#726's requested ownership change is real
The custom MCP server factory/registration and manual browser contract augmentation are deleted. The framework now registers the real show-status operation, readiness-policy resource and bound App; AppRouteInput/AppRouteResult derive from generated routes, not a second handwritten map. The browser keeps createAppClient rather than recreating transport. Moving the real read-policy URI out of ui:// is appropriate for the new non-App resource; the old URI was not a working registered operation.
The tests now distinguish mock bridge coverage from an actual generated-server journey: web-command.e2e.test.ts deletes source before launch, reads the real policy, and waits for the real refresh response rather than treating unavailable as successful example functionality. Preserve that level of proof.
Previous review findings are fixed in this head
onToolInputstorescurrentService, allowing a failed opening call to be retried without relying on a successful result first.- The English walkthrough explicitly says choose a service and Call tool and preview, matching the conventional App workspace.
Reply to those existing threads with this evidence; do not add the same fixes again.
Gate / integration details
The PR body still has _pending_ local gate and self-review sections at the time checked. Those are missing evidence, not reproduced failing tests. Record the accepted local build/typecheck/lint/unit gate plus the changed browser, examples-contract, workbench-surface, examples-real and web-command tests, and the English/Chinese docsite on the final head. This review does not add a requirement to wait for hosted CI instead of the repository's local-gate policy.
#734 also edits examples-contract/workbench-surface/examples-real expectations. Whichever lands second must retain both changes and rerun the shared tests rather than resolve route counts mechanically. Keep dedicated custom-factory interoperability fixtures: a convention-first primary example is not removal of the supported low-level escape hatch.
No changeset is demanded solely for private examples/tests/docs. No approval, code change, merge, host execution, or publication was performed.
Closes #726.
examples/mcp-appis convention-first. The handwrittensrc/mcp/status.ts(McpServer,registerTool,registerResource, the@modelcontextprotocol/serverdependency), the config-declaredmcp.servers.status.appsblock,src/compiler-status-contract.ts, and the App's hand-typedStatusPanelRouteContracts/AppRegisteraugmentation are gone. Route modules own registration and the browser client's types:tool:status/show-statussrc/mcp/status/tools/show-status.tsx—_meta.ui.resourceUri: appResourceUri('status')resource:status/readiness-policysrc/mcp/status/resources/readiness-policy.tsx—policy://mcp-app-example/readinessapp:status/statussrc/mcp/status/apps/status.ts+status.html(moved fromviews/)src/service-status.tsholds the schemas and the two records;src/readiness-policy.tsholds the policy text and URI (browser-safe, no zod in the App bundle). The App callstool:status/show-statustyped from the generated.agent-bundle/routes.d.ts.The two advertised controls are real routes, not deleted: the App's Refresh status re-calls
show-statusfor the service on screen (there is norefresh-statustool — the old view called one the handwritten server never registered), and Read policy reads thereadiness-policyresource that now exists. The App is built for every target thestatusserver ships to (AB4828), notportableonly.Deletion totals: 23 files, +282/−277 → net example source −136 lines (
src/mcp/status.ts−57,compiler-status-contract.ts−37, config −14, App view −49 of hand-typed contract/bridge code).Tests
examples/mcp-app/tests/browser-app/status-panel.browser.test.ts— real compiled App over the product bridge; refresh now waits for the opening result and asserts theshow-statusre-call.packages/workbench/tests/web-command.e2e.test.ts— realwebcommand, real generated server, source directory deleted before launch; asserts the policy text and “Status refreshed.” from the real routes.packages/workbench/tests/examples-real.e2e.test.ts— the App leaf now exists, so the Workbench App workspace is driven for the first time: selectpayments-api, Call tool and preview, assert#service/#statusinside the sandboxed App frame.packages/agent-bundle/tests/examples-contract.test.ts— manifest route graph[app, resource, tool], listing carries_meta.ui, invoke returns the degraded record.packages/agent-bundle/tests/workbench-surface.test.ts— three-route catalog and Tools/Resources/Apps subgroups.Docs
examples/mcp-app/README.md,website/docs/{en,zh}/examples/mcp-app.mdx,website/docs/{en,zh}/guide/authoring/mcp.mdx(the example no longer hand-declares anAppRegistermap),docs/entry-conventions.md.No changeset: only
examples/*(private), tests, and docs change —skip-changeset.Local gate (branch contains
origin/main@ a2eb7f4, i.e. after #729 and #734)Deslop
Deslop: Claude, 3 edits (duplicate
currentServiceassignment, consent comment, EOF blank line).Self-review
Reviewer: GPT-5.6 Sol Max (generalPurpose;
change-risk-reviewerhad no shell/TraceDecay in this session). Two passes.currentServiceonly set on success, so Refresh was a no-op after a failed opening call (Codex P2 raised the same)onToolInput; browser-app error test now retries via Refresh and asserts theshow-statuscall + recovery (7793244).zoddep, App element ids, en/zh parityPass 2: all resolved, no new risks.