Repository navigation
Conversation
[Medium impact] The change reaches outside-agent tool catalogs across six app workflows. The approval changes appear sound, but the required app changelog entries must be added before merging. Findings
SummaryExposes seven existing inline actions through curated MCP catalogs and adds catalog snapshots and approval checks.
Reviews (2) · Last reviewed commit: "Scope inline app approval to outside act..." · Reviewed by Greptile |
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 |
b869f80 to
4d03699
Compare
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🔴
Review Details
Incremental review summary
The latest update adds a shared isOutsideAgentCall helper and applies it to Calendar bulk deletion, Content sharing, and Design handoff approval policies. I verified the five prior findings are fixed and resolved their review threads. Two new bypasses remain: the HTTP route can still pass caller: "frontend" based on an untrusted header even when provenUiRequest is false, causing the helper to skip approval; and the CLI action dispatcher invokes handlers directly without evaluating needsApproval, despite the helper classifying CLI calls as outside. Both affect the newly approval-gated data disclosure/destructive actions. The shared abstraction is useful, but its trust decision needs to be enforced by every execution surface. Risk remains high. Browser testing is not applicable because the diff has no UI implementation changes.
🧪 Browser testing: Skipped — PR changes approval and catalog behavior, not user-facing UI implementation.
| return ( | ||
| ctx?.networkProtocol !== undefined || | ||
| ctx?.automation !== undefined || | ||
| (ctx?.caller !== "frontend" && ctx?.caller !== "tool") |
There was a problem hiding this comment.
🔴 Unproven frontend claims bypass approval
The HTTP route can set caller: "frontend" from the client-controlled frontend header even when provenUiRequest is false; it then passes that caller into the approval predicate, which treats it as trusted and skips approval. Only classify frontend calls as trusted after the route verifies the UI capability, and add a regression test for the header without that proof.
There was a problem hiding this comment.
Thanks for the update. The route can still treat an arbitrary caller-supplied x-agent-native-frontend: 1 header as trusted even when the UI capability proof fails. Please make frontend trusted only after the route verifies that proof and add a regression for the forged header; otherwise outside callers can skip the approval gate.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for the update. This still depends on #8071 and the follow-up regression, so I’m keeping approval held until both land and are reviewed at the resulting head.
| return ( | ||
| ctx?.networkProtocol !== undefined || | ||
| ctx?.automation !== undefined || | ||
| (ctx?.caller !== "frontend" && ctx?.caller !== "tool") |
There was a problem hiding this comment.
🟡 CLI actions bypass the new approval policy
This helper classifies caller: "cli" as outside, but the CLI dispatcher invokes handler.run(...) directly without evaluating needsApproval. Calls to the newly gated sharing, handoff, or bulk-delete actions through the CLI therefore bypass the intended approval; enforce the policy before dispatch or block approval-gated actions on that path.
There was a problem hiding this comment.
Thanks for the update. The CLI dispatcher still calls handlers directly without evaluating needsApproval, so the new gate is bypassed on CLI requests too. Please enforce the approval policy before dispatch or fail closed for approval-gated actions, with a CLI regression.
There was a problem hiding this comment.
Confirmed. scripts/runner.ts and server/dev-action-bridge.ts run handlers without calling preparedCallNeedsApproval, and that's true on main for every action with a gate. The CLI should evaluate the gate before it dispatches and refuse when it can't get approval. That's a core change that affects every gated action, so it's being scoped separately; this PR won't merge without it. I'm leaving this thread open until it's pushed.
- Claude Code AI
There was a problem hiding this comment.
Thanks for confirming the CLI gap. I’m holding approval until CLI dispatch applies preparedCallNeedsApproval and refuses gated calls when it cannot obtain approval. Please send the current head once that fix is ready.
Problem
Outside assistants could not call seven actions that already open real inline editors. These apps curate their external catalogs, so
compactCatalog: truealone did not advertise the tools. Design's MCP instructions also recommended exports absent from its default catalog.Changes and safety
Expose the existing actions through their curated catalogs, with reviewed mutation annotations and inline UI resource snapshots:
manage-event-draft: manages the caller's unsent invite drafts and never sends invitations; outside callers must approvedelete-allbefore any drafts are removed.share-local-file-document: requires outside-caller approval before creating or refreshing an owned local-file copy. New copies remain private, but refreshing an already shared copy can disclose updated contents.generate-screens: plans up to eight screen placements after editor access checks; does not generate content, invoke a model, publish, or change sharing. Marked destructive because it replaces generation-session state.export-coding-handoff: requires outside-caller approval because it mints raw/ZIP bearer URLs valid for seven days, after existing design access checks.clone-context-slide: appends an editable copy to an authorized deck, preserving source provenance; does not publish or widen sharing.clone-context-slide: retains existing ownership, provenance, Lab checks, and mutation annotations when adding a copied slide.open-traffic-dashboard: opens the read-only first-party dashboard; does not query warehouse rows or mutate a dashboard.Approval boundary: Content sharing, Design coding handoffs, and Calendar bulk draft deletion use Core's shared
isOutsideAgentCall. Frontend and ordinary in-apptoolcalls count as inside only when both network and automation provenance are absent. MCP, WebMCP, A2A, HTTP, CLI, card callers, automations, and delegatedask_appcalls require approval. A2A provenance matters because its handoff runs ascaller: "tool"withnetworkProtocol: "a2a";ask_appsimilarly retains MCP provenance. Hosts/routes unable to complete approval fail closed, including the reported Claude-in-T3 behavior. Other Calendar draft operations remain ungated.The helper and its spec are copied byte for byte from #8005 at
fe40a38e, with the identical root export. This keeps the shared addition identical whichever PR merges first. A separate Corenonechangeset satisfies package coverage without duplicating #8005's patch bump or release note, even if a Version Packages release lands between the two PRs. Six app changelog entries describe the new capabilities. Four reviewed actions leave the legacy destructive-hint inventory; Mail's sibling hunk remains untouched.Design's MCP instructions recommend only advertised
export-pngandexport-coding-handoff. Correcting the instructions avoids exposing the separate HTML/ZIP/Figma export contracts in this sweep. The in-app guide retains those three exports,generate-screens, and the outside-caller approval note. Its guide stays below the 6,000-character cap; canonical workspace guidance is preserved.History and exclusions
Checked each candidate with
git log -S, blame, introducing/catalog diffs, and PR descriptions. No action-specific deliberate exclusion was recorded for the seven additions. Preserve #7833's curated catalogs rather than changing Core's catalog gate.Skipped
submit-content-database-form: #2866 explicitly made submission an authenticated, consequential, message-only capability so Content owns schema interpretation and mutation. #3674 selected direct document operations without changing that boundary.The unproven-frontend HTTP route gap is addressed separately in #8071 at the approval boundary; this PR does not modify
action-routes.ts.No migrations or changes to resource access checks. Reverting the catalog declarations removes exposure; it does not delete created records or revoke minted handoff URLs.
Verification
Fresh verification on
4b89e095using CI's Node 22.23.3:git diff --checkpass.The new Calendar transport tests failed before the fix: deletion succeeded without host approval, and an approval-capable host received no prompt. Content and Design's new caller-policy tests each reproduced the WebMCP bypass. Those policy assertions now live in focused app specs rather than the six-app catalog fixture.
Historical local Calendar/Content HTTP
tools/listchecks showed the new tools with_meta.ui.resourceUri. MCPJam connected and called Calendar's draft action, but its Claude simulation rejected the nested app withcspFrameDomains is unsupported: frame-src 'self'; full real-host rendering remains unverified.Earlier rebase repair preserved #8003's Design guide intent. Slides-2's clone row now lives outside its exact first-turn action table. Standalone Chat's dependency-age install failure cleared without changing dependency declarations or the 24-hour policy. These are historical checks, not a claim about CI after this push; CI is not being watched.
Follow-ups
manage-draft: sibling Expose Mail draft review to connected agents #8005 onmail/expose-manage-draft.compose-dashboard,mutate-dashboard,update-dashboard,save-analysis,ensure-*): product/safety decision needed.reorder-dashboard-panelshasagentTool: false; Designshow-design-questionshasmcpTool: false. Report only.