Repository navigation
Show MCP approval reviews inside the app, in its own theme - #8084
Conversation
Visual recap — generation failedThe visual recap could not be generated for this pull request. This is informational only and does not block the PR. Diagnostic: No plan URL: Repair changed too much of targeted file plan.mdx; expected a localized parser fix. Agent output: Repaired recap-source.json: escaped the Mermaid and diff code strings that contained parser-breaking literal newlines. JSON and required block IDs remain valid. --- ⠀ 🟢 Visual recap source repaired and ready for the hosted parser retry. stderr: Reading additional input from stdin... |
bff38d5 to
619f58e
Compare
619f58e to
07645e1
Compare
c3d55ab to
b5dda3d
Compare
b5dda3d to
5a97854
Compare
5a97854 to
b412c80
Compare
b412c80 to
d537563
Compare
|
On the low finding in the 133efc4 review (a 401 on Approve or Deny leaves no in-page way back): leaving this as is. The review link the agent gave is where sign-in happens. Opening that link again signs the reader in and brings them back to the same review. That works because the decision was never saved and the grant is still pending until its deadline. A session that ends inside the ten-minute review window, such as a sign-out in another tab, is rare. Adding a sign-in control to the review would be a new visual element, and this PR is closed to further visual changes. If it's wanted, it can be a follow-up shared with #8016's plain page, which says the same thing. |
… ask A needsApproval action called over MCP now returns a normal tool result with a review-page link when the client can't show an approval prompt, including every stateless 2025-era client that used to get -32021, or when it declines or cancels the prompt. The page needs the same signed-in user and org, renders only the stored redacted arguments, and takes Approve or Deny as a same-origin JSON POST with a per-user token. A page approval lets the identical call run once within ten minutes; Deny is final. The page and the in-client form share one server-side summary: times in the reader's zone with labels, the requested zone when it differs, zoneless times labelled (Calendar's create-event declares zonelessTimes: "calendar"), the authorized OAuth connection, and a warning when the same call already ran unless the action is retrySafe. Page approvals are audited as mcp-review. The grant store gains additive columns, joins the sensitive framework tables, and is registered for identity rekey and offboarding.
…and clear stored arguments The review page now finds the reviewer only from the browser's sign-in cookie (Better Auth's or the framework's own), never from getSession, so bearer, embed, query and custom-resolver credentials can't decide a grant. Grants carry the app's canonical id in the caller key and every read, decision and consume matches it, so apps sharing a database can't use each other's approvals. Stored arguments are cleared when a grant is used, denied or declined, hidden once it expires, and pruned by a recurring sweep. The page's JSON view and POST require Sec-Fetch-Site: same-origin as CSRF protection, and a reader's own grant from another organization says to switch organizations.
… app's own origin The review page now lists every recipient and argument whole and says plainly when storage had to cut the stored arguments. The in-client form shows at most 20 recipients, 12 fields and 280 characters a value, says what it left out, and links the page when the page shows more. Approval links use the request's origin only when the app declares it (or declares none), as sign-in redirects do. Calendar's zonelessTimes survives both action loaders. Impossible offsets and dates are shown as written instead of converted. A page approval still runs the call when the prompt that asked for it has expired; that prompt's own answers no longer count.
…e text, and read an end in its start's zone Sweep handlers only run from the platform sweep route, which nothing calls on a long-lived server, so the approval route also prunes on its own ten-minute timer there. The shared argument redactor now removes a credential inside ordinary text (Authorization: Bearer ..., password=...), as its text summaries already did. The approval summary reads an end time with no zone of its own in its start's zone, as Calendar does.
…n, and leave a skipped DST time as written - mcpApprovalReviewUrl reads the approval route from the request's MCP mount, so mountMCP with a custom routePrefix links to the page it mounted. - declaredLinkOrigin keeps the request's origin when the app declares none; Netlify's URL and DEPLOY_URL don't count, as for Builder connect. - A wall time its zone skips (spring-forward gap) reads as no instant and is shown as written, not as a time with no zone.
- A signed-out reader in an app with no sign-in page gets a short page in their language instead of an English JSON error; JSON callers get the same text. - A stale decision token answers in the reader's language, and the page says to reload instead of to try again.
- mountMcpApprovalRoute registers <route>/:id with registerAuthPublicPaths, so a custom routePrefix under /_agent-native reaches the page's own sign-in instead of the guard's bare 401. - Exact registered public paths accept a :name segment for one path segment, and the guard no longer hard-codes the default review path.
…lback # Conflicts: # packages/core/src/mcp/server.ts
…trict compiler check passes
… deny rather than decline there
…acle guard asks for
…lback # Conflicts: # packages/core/src/audit/types.ts # packages/toolkit/src/app/i18n/catalogs/ar-SA.ts # packages/toolkit/src/app/i18n/catalogs/de-DE.ts # packages/toolkit/src/app/i18n/catalogs/en-US.ts # packages/toolkit/src/app/i18n/catalogs/es-ES.ts # packages/toolkit/src/app/i18n/catalogs/fr-FR.ts # packages/toolkit/src/app/i18n/catalogs/hi-IN.ts # packages/toolkit/src/app/i18n/catalogs/ja-JP.ts # packages/toolkit/src/app/i18n/catalogs/ko-KR.ts # packages/toolkit/src/app/i18n/catalogs/pt-BR.ts # packages/toolkit/src/app/i18n/catalogs/zh-CN.ts # packages/toolkit/src/app/i18n/catalogs/zh-TW.ts # packages/toolkit/src/app/settings/shell/pages/audit.tsx
|
On the 🟡 LOW from the
|
…ike clock skew to the page
… read a missing window
There was a problem hiding this comment.
Builder reviewed your changes and has a few items to flag 🟡
Review Details
Incremental Review Summary
This update keeps the core MCP approval flow and themed in-app review surface intact, with the latest addition protecting the review boundary from server rendering through ClientOnly. I verified that the previous SSR browser-global issue is fixed: ProvidersInner (and thus McpApprovalReviewBoundary) is now rendered only after the client mounts, and a server-render test asserts the review stays out of markup when window is absent. Its prior thread has been resolved. The approval and grant-state paths otherwise appear sound in the current review.
Finding
- 🟡 Low — Schema labels can hide the actual argument value:
describeFieldValuesubstitutes a schema-authored title/description for the literal value in the primary summary. A reviewer could see an inaccurate label while approving the actual argument; show the literal alongside the label or keep the label secondary. This may be filtered by review policy.
🧪 Browser testing: Skipped — dev server setup fails during Toolkit postinstall with 93 TS2883 React type errors; restart reproduced the failure.
|
On the 🟡 about schema labels hiding the literal value: the literal is always on the page. Both the in-app review and the plain page show the call's stored arguments, unchanged, under Details. A label replaces a value only when that value exactly matches a |
This PR adds the themed app review surface on top of #8016's approval flow, which has landed on main.
What changes
When an MCP client cannot show a person's approval prompt, #8016 returns a link to a plain review page. This PR opens the same review inside an opted-in app, using its name, logo, theme, and components. The person can inspect the call, approve or deny it, and return to the app.
#mcpApproval=<id>. The fragment survives home-route redirects, andAppProvidersshows the review in place of the app. Sign-in continues at the original review link.appApprovalPageis off by default. Apps opt in through the chat plugin ormountMCPonly when their shell rendersAppProviders. The 16 chat templates, Dispatch, and Builder Code starter opt in. Plan opts in on its separate MCP mount. The default scaffold remains off.to,cc, andbccfields. Schema choice titles, including titled booleans, supply readable values. Identifier-style enums are split into words, and time zones use localized names. The plain page and client form share these improvements.@. Recipient text shrinks when needed on narrow screens. Approve uses the theme's foreground and background colors.servedAtvalue, so a fast or slow device clock does not change the decision window.The browser reads and decides through the new
@agent-native/core/client/mcp-approvalhelpers. Decisions retain #8016's session, ownership, token, and same-origin checks. The app surface requires the standard framework route prefix; other MCP prefixes keep the plain page. New copy is translated into all 11 locales, and affected apps receive changelog entries.Verification
8254996c: merged main up to the commit before the squash, recorded the squash with-s ourssince this branch already had its changes, and checked that the diff against main covers the same 56 files as before. The Toolkit review andAppProvidersspecs (62 tests) and the Core approval route, summary and review-helper specs (81 tests) pass on04333606.AppProviderskeeps it insideClientOnly, and a new test server-rendersAppProviderswithwindowstubbed out. If the gate moves in front ofClientOnly, the test fails withCannot read properties of undefined (reading 'top').97a75888, which now includes main through Ask a person before CLI calls run actions that need approval #8117, without conflicts. The earlierAppProviders.tsxconflict kept both main's lazy agent-run awareness component and this PR's review boundary, and main's strict lint flag on a dead?? ""was fixed by finding the time placeholder by position.undefinedabort signal wherefetchtakesnull; it now passesnull. On the merged tree Core typechecks, and the oracle-loosening, strict-lint and escape-hatch guards pass. The release test that pins Toolkit's Core floor passes.ccorbccnow keeps its own row on the review page, so a copy doesn't read as sent to; a lonetoorattendeeslist of addresses is still left to Reaches. The summary, route, Toolkit review and Calendar MCP write specs pass (77, 30 and 33 tests).shared/mcp-approval-review.tsnow have their own spec beside them, which the changed-line mutation job needs to test that file.servedAttime and state after its async reads (requester, prior run, decision token), so read latency no longer looks like clock skew to the page; a grant that expires during those reads comes backexpired. A new route test covers it and fails on the previous code.674b15fd), 385 of 386 Core approval, redaction and mount tests and 61 Toolkit review andAppProviderstests pass. The other Core test, the ChatGPT scan replay inserver.spec.ts, hit its 60 s timeout with this machine's CPU at 99% from other work; it passed alone in 30 s on the previous head. strict-lint passes, and escape hatches show no growth across the 59 changed files.AppProviders, Dispatch, and CRM catalog options.spawn EINVALfailure in aggregatepnpm lint.Before merge
Toolkit's optional Core peer is now
>=0.221.0. Both changesets bump Core asminor, so 0.221.0 is the first release that will carryclient/mcp-approval; no published Core exports it yet. Main's Core is still 0.220.0. If a Core release goes out before this PR lands, the floor moves to the release after it.Remaining scope
Calendar's
sendUpdatesschema can separately add option titles such as “All guests” to improve the generic labels.🤖 Generated with Claude Code