Repository navigation
Conversation
[Medium impact] This PR appears safe to merge. SummaryAdds
No actionable issues found. Acknowledged limits from
|
|
Here's a visual recap of what changed: Open the full interactive recap |
e06f366 to
63a8297
Compare
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🔴
Review Details
Incremental Review Summary
This update adds a shared outside-agent provenance helper, uses it to gate draft deletion, and hardens draft creation against ID collisions with a pre-check plus atomic compare-and-set. It also adds tests and a core changeset. The previous A2A approval-bypass and draft-ID overwrite findings are fixed; both review threads were resolved. Risk remains high because the approval policy protects destructive Gmail operations.
Findings
- 🔴 HIGH — An untrusted frontend header bypasses the new approval policy. The action route maps
x-agent-native-frontend: 1to callerfrontend, while explicitly treating that header as only a client claim.isOutsideAgentCalltreats that caller as trusted without checking proven UI capability; an authenticated HTTP request can therefore avoid approval for destructive actions. - LOW — If the compensating Gmail delete fails after a compare-and-set collision, the cleanup error masks the intended 409 and leaves the losing Gmail draft orphaned.
The updated tests cover the collision and A2A provenance cases, but not the spoofed-header approval path. Browser verification was skipped because the browser-test-planner agent is unavailable in this environment.

Problem
External agents connected directly to Mail cannot discover
manage-draft, even though it already has an MCP Apps compose UI. The explicit connector catalog excludes it, including from tool search, so agents fall back toask_appwithout a draft card.Change and safety
Add only
manage-draftto Mail's connector catalog. Create and update remain directly callable.delete,delete-saved, anddelete-allrequire fresh human approval whenever an outside agent initiated the call, including requests delegated through A2A orask_app. Persistent approval is disabled. Hosts unable to approve fail closed.delete-allcan delete saved Gmail drafts as well as compose state.The shared
isOutsideAgentCallhelper (packages/core/src/action-caller.ts, exported from@agent-native/core) checks network provenance before exempting ordinaryfrontendandtoolcalls. Direct MCP, A2A, WebMCP, card, HTTP, CLI, automation, and missing/unknown caller contexts are conservative; any network protocol or automation context also makes an app-agent run outside-origin. Mail's proven UI Delete button and its own non-delegated agent retain their behavior. This deliberately changes the earlier policy: outside-agent requests throughask_appnow require approval for deletion too.Create rejects an ID already in use with
draft_already_exists(409), before calling Gmail. Atomic insert-if-absent prevents a concurrent create from replacing the winning draft. A loser cleans up only the Gmail draft newly created by that losing call.The tool declares
destructiveHint: trueandopenWorldHint: truebecause it can change a connected Gmail mailbox, and is removed from the legacy unreviewed-hint baseline. Other Mail writes remain outside this catalog; the embed target is unchanged.The helper uses only
caller,networkProtocol, andautomation. CLI defaults outside because coding agents drive it, pending Alice's decision. Automation defaults outside because its run context has no trusted origin; #8070'smcpManagedlives on job frontmatter rather than that context.HTTP/embed boundary coordination belongs to #8071, so this PR leaves
action-routes.tsand its spec unchanged. At inspected head80f8d860, that PR treats embedded card calls as MCP and strengthens browser-cookie/UI proof. One case remains open for the approvals thread: a non-embedded authenticated HTTP request withx-agent-native-frontend: 1still reaches the conditional policy asfrontendwhen UI proof fails, so this helper exempts it. This PR does not claim that HTTP header boundary is fixed.The shared catalog logic is unchanged: Mail does not configure
externalAgents.writes: "ask_app_only". That framework policy still suppresses writes wherever enabled. Mail's explicit allowlist intentionally preventscompactCatalog: truealone from exposing an action.CI repair
The original failing job failed all three
production-agent.approved-tool-gone.spec.tstests withaborted_execution-lease-claim-failed. An isolated local database with a migration ledger but no execution-lease table reproduces all three failures. These agent files were unchanged by this PR.Rebased onto
af23ad48bf59, which includes the independently merged #8011 fixture fix. That spec now initializes its run-store migrations before exercising approval. #8011 documents the same failure and has all eight targeted CI lanes passing. No shared agent implementation change was added to this PR.The follow-up also addresses both review findings: disable persistent draft approval and mark Gmail effects as open-world. The new persistent-approval regression test failed before the fix and passes afterward.
Verification
pnpm --filter mail test— all 181 files and 1,927 tests passed before the final helper path/export alignment; the 34 affected draft/catalog tests passed again after that alignment. Before the fixes, the three delegated-delete cases and existing-ID collision regression failed as expected.pnpm --filter @agent-native/core exec vitest --run src/action-caller.spec.ts src/action.spec.ts— 51 helper and 168 action tests passed after the final path/signature/export alignment (219 total). The helper covers all 10 caller values crossed with all three network protocols, plus missing and unknown context; every caller is also tested with automation set.pnpm --filter @agent-native/core exec vitest --run src/agent/production-agent.spec.ts -t approvalwith the prepared isolated PGlite database — 21 passed; 493 excluded by the name filter.pnpm --filter @agent-native/core exec vitest --run src/mcp/server.spec.ts --maxWorkers 1— 195 passed; the unchanged ChatGPT directory-profile scan exceeded its existing 60-second budget. The focused retry of that same scan also timed out at 60 seconds. The scan loads Slides, Design, and Content profiles rather than Mail; the test and those profiles are unchanged by this PR. This remains a local verification failure, not a passing check.git diff --checkpassed.The first combined core run hit three MCP timing overruns and two subsequent mock-count failures; the standalone rerun reduced that to the one directory-profile scan timeout. No assertions or timeout settings were changed. Earlier Windows schema-setup limitations and the original three-test lease fixture repair remain historical evidence, not newly rerun integration checks.
Original integration evidence at
e06f366e: the live local HTTP MCP client listedmanage-draftwith_meta.ui.resourceUri = ui://mail/manage-draft/shell-v65; create/update returned draft data and embed metadata;resources/readreturned MCP Apps HTML. All three static-token delete calls were refused for lacking verified human approval. MCPJam Inspector 3.16.2 connected through OAuth and executed create/update. This evidence is retained for the unchanged catalog/embed path; it does not establish the new approval flag or current production rendering.Full compose rendering and delivery remain unverified: strict MCPJam Claude simulation blocked the local frame, permissive simulation remained blank, and direct local Mail required a Google connection. No email was sent. The original repository-wide annotation inventory spec stalled and was interrupted, so it is not counted as passed. Screenshots and protocol logs remain in the worktree's ignored
.tmp/directory. The two disposable credential config files have been deleted.Send and Dispatch findings
The embedded compose uses
ComposeModal.handleSend→useSendEmail→POST /api/emails/send, not the MCPsend-emailaction. The route's guard and handler callgetSession, which accepts ordinary signed embed sessions; the embed-auth client attaches the session bearer/target to same-origin POSTs. A user click therefore uses Mail's browser/embed session without an MCP approval exchange. This is code-supported; an actual embedded Send click has not been verified.Dispatch does not proxy Mail's named tool or action-specific card. Its own catalog exposes local tools plus
list_apps,ask_app, andopen_app;ask_appdelegates through A2A and returns response/task data.open_app({app:"mail",path:...,embed:true})can separately embed granted Mail UI. No Dispatch changes here.Inline resources still require the existing deployment rollout flag
AGENT_NATIVE_MCP_APPS_INLINEor an allowed caller email. This PR does not change that rollout policy.Independent static security review found the unproven frontend-header issue described above. A local repair was tested, then removed to keep HTTP approval policy in its owning thread and avoid conflicting with #8071. No additional findings were identified in the collision handling. These reviews were static and did not establish live host behavior.