[hugocasa]

Summary

Makes /api/mcp the one URL for Windmill's MCP server, and gives it the AI session's toolset: an external client (Claude, ChatGPT, Cursor) can explore the workspace, run scripts and flows, write per-user drafts, test-run them, diff and deploy, as the in-app chat does.

What a client gets depends on the token, chosen when it signs in or when the token is created:

Access (UI label)	Scopes	What the client gets on /api/mcp
Build and run (default)	mcp:all	the whole toolset (60 tools): drafts, test runs, diff, deploy, runs
Read and run	mcp:all mcp:read_run	the read tools plus running deployed scripts and flows (28 tools)
Read only	mcp:all mcp:read_only	the read tools (26 tools)
Scripts and flows as tools	mcp:favorites, folders, mcp:scripts:…, mcp:flows:…, mcp:hub:…; "All scripts/flows" is stored as mcp:scripts:* mcp:flows:*	each selected script or flow as its own typed tool, and nothing else

The workspaces a token reaches are chosen the same way: one (the token is bound to it, as before), several (mcp:workspaces:a,b), or all of the user's, including ones joined later. Every session tool takes a workspace_id.

The existing URLs (/api/mcp/w/{workspace}/mcp, /sse, /api/mcp/gateway) keep serving existing tokens exactly as before, endpoint scopes included. They are no longer shown in the UI.

How it is built

• One URL, two servers behind it. /api/mcp routes on the token's scopes: mcp:all goes to the session toolset (backend/windmill-mcp/src/server/session.rs), a selection goes to the existing tools server with its endpoint tools turned off (NoEndpointTools). The existing URLs never set that flag. Both servers keep their own code.
• What the model reads is generated from the session. Tool names, descriptions, input schemas, the instructions, the get_instructions texts, the trigger/schedule schemas and the raw-app templates are emitted from the GLOBAL chat's TypeScript into backend/windmill-api/src/mcp/session/gen/ by npm run gen:mcp-session (mcpSessionGen.test.ts is also the freshness check). Sentences that promise the chat's argument form or a user confirmation are rewritten for MCP, and generation fails if a new one appears. The folder guidance is cut out of the prompt as its own pieces, so a single-workspace token gets the prompt as before byte for byte, and a multi-workspace token gets the rest plus get_workspace_context.
• What a tool does is written in Rust, in backend/windmill-api/src/mcp/session/, one file per group, mirroring chat/global/core.ts. Each tool acts through the API as the caller, so permission checks are the API's own. AGENTS.md says a behaviour change on one side is mirrored on the other, and a cargo test fails when a session tool is neither implemented nor listed as editor-only. Changes to the session merged on main while this was open (stale step locks, AI agent identity on deploy, folder and group reads, create_resource_type, workspace settings tools, app sandboxing and viewer data access, chat-enabled flows, datatable migration schemas) are mirrored.
• Tool calls reach the API in-process. Ctx::request calls the server's fully layered axum router with oneshot and the same minted JWT, instead of a loopback HTTP request to base_internal_url: same middleware, auth, handlers, permission checks and 30s timeout, no socket. The router is registered per base_internal_url, so the backend tests' one-server-per-test processes stay on their own databases; with no router registered it falls back to HTTP. The per-runnable tools server still uses HTTP.
• The core rules come first in the MCP instructions. Claude Code keeps only the first 4096 characters of a server's instructions, which cut the prompt (about 14.8k) before the get_instructions rule and the default-run rule. For an MCP client the prompt builder now puts the core rules (drafts only, find and read first, deploy on request, get_instructions before writing, test after editing, test-run versus run) right after the opening, ahead of the path conventions, and the rest follows under "More rules". The text is the same, only reordered; the chat's own prompt is byte-identical.
• 60 of the session's 65 tools are served. Left out because they need the Windmill UI: open_page, askUserQuestion, update_user_instructions, search_files, read_file.
• Links instead of a preview panel. Run tools and get_run end with the run's page, init_app/app writes return the draft's editor (preview_url), and a deploy returns the deployed item's page. Links use BASE_URL, or the request's origin while BASE_URL is unset. The MCP prompt, near its top, and init_app's result ask the model to open the link in a browser the user can see from the conversation when it has one, and otherwise to give the user the link.
• Writes go to drafts. deploy_workspace_item, delete_workspace_item and the other tools that reach the deployed workspace are annotated destructiveHint, standing in for the session's confirmation card.
• rebase_draft keeps a draft it cannot return whole. It discards the stale draft and returns the diff as the only record of the edits; when that result would exceed the server's tool-result cap (87,500 bytes), the draft is kept and the user is sent to the editor instead.
• The chat prompt builder gained an external option that drops mentions of editor-only tools and the run form. With it off, the chat's prompt is byte-identical to before.
• One chat fix found while mirroring: the chat's own resource deploy reset an AI agent's identity to the deployer; it now keeps it, as the MCP side does.

Auth and security (please look closely)

• The session toolset admits mcp:all only, optionally narrowed by mcp:read_only, mcp:read_run or mcp:workspaces:. A scope from outside mcp: that confines routes or paths (variables:read:f/x/*) is refused; a job tag fence (if_jobs:filter_tags:*) is allowed and carried into every tool call.
• Tool API calls use a JWT with the caller's full rights, since several routes the session uses (hub, embeddings) map to no route scope. Every call goes through one guard (guarded_url) that refuses any path that does not stay on the route its handler wrote (dot segments, #, another workspace, an unlisted instance route). The same guard holds read-only tokens to the read-only route rule, and read-and-run tokens to it except POST jobs/run/p/ and jobs/run/f/ (the preview routes take code, so they stay refused). Unit tests cover both. The JWT carries the caller's job provenance, so a job's token stays capped.
• Every level below "Build and run" stays at its level on every URL. A token carrying mcp:read_only or mcp:read_run gets no API endpoint tools on /api/mcp/w/{w}/mcp and /api/mcp/gateway either; without that, these mcp:all-based tokens reached the full endpoint list there, getVariable on secrets included. A selection made as "Scripts and flows as tools" carries the marker mcp:runnables_as_tools for the same effect, since its other scopes are the ones older selections have. The picker adds the marker and hides it; approval containment treats it as narrowing only. Tokens minted before this PR carry none of these scopes and keep their tools.
• mcp:workspaces: is enforced on both servers, the existing tools server's per-workspace URLs and gateway included, otherwise a listed token would reach any workspace through /api/mcp/w/{other}/mcp.
• mcp:read_only and mcp:read_run are honoured by the existing tools server too, as read-only there (it has no read-and-run).
• OAuth approval containment (McpScopeConfig::contains) treats the workspace list, mcp:read_only and mcp:read_run as narrowing only: an approving token cannot drop them.
• Read-only is a scope for MCP tokens (mcp:read_only), because an OAuth grant carries scopes and no read-only flag. Tokens with the existing read-only column keep working.
• Variable values are never read back; secret arguments to runs are minted into expiring secret variables, as in the session.

UI

• Connect drawer and account settings: "Build and run from your AI client", the /api/mcp URL, and one collapsed "Use a token instead of signing in". In account settings MCP is its own section above Tokens instead of a toggle inside it.
• Sign-in from an MCP client skips the workspace picker. After login, a user with several workspaces was sent to "Select a workspace" before the consent page, which is where workspaces are chosen. The login redirect now goes straight to /oauth/mcp_authorize, as the root layout already did for a logged-in user.
• One access picker (McpAccessPicker.svelte) in the token form, the OAuth sign-in page and the token editor, so editing a token's access can no longer flip it to another server's model. Under it, a collapsed "Tools the client gets" lists the tools of the chosen level (GET /api/mcp/toolset?access=). "Scripts and flows as tools" opens the existing picker without endpoints, "All scripts/flows" included, and needs one workspace.
• The token editor only rewrites scopes once the access picker is used. Saving after a label change keeps the token's scopes exactly, including what the picker does not model (an old token's endpoint list).
• OAuth sign-in picks one, several or all workspaces plus the access.

Migration

workspace_id on mcp_oauth_server_code and mcp_oauth_refresh_token becomes nullable, so a sign-in granting several or all workspaces mints a token bound to none. The down migration also deletes every token, refresh token and code carrying mcp:read_only, mcp:read_run, mcp:runnables_as_tools or mcp:workspaces:: a server without these scopes ignores them, which would widen the token.

Fallback for endpoint-specific rights

Not in the UI any more. A token with mcp:endpoints:… can still be created through the API and used on the existing URLs, which keep serving endpoint tools. Telemetry in this PR (mcp/endpoint_tool_call, mcp/runnable_tool_call, mcp_workspace/tool_call, EE registry in the companion PR) shows whether anyone still relies on them before they are dropped.

Known differences from a session

• diff against="parent_workspace" (fork vs parent) returns "not available on this server".
• A script or flow draft carrying trigger edits staged in the editor is refused at deploy rather than deployed without them.
• get_instructions subject:"flow" has no live AI-provider catalogue section.
• Skills: all readable ai_skill resources are offered (the session's on/off toggles live in the browser). User instructions are not included for the same reason.
• No argument form or confirmation card: run tools run with the arguments sent.
• The tool list is not tailored to the role: a non-admin sees get_workspace_settings/set_workspace_settings and the API refuses them; an operator's restrictions are enforced by the API on each call.
• init_app always starts from the framework template, where the chat gives an operator builder a static page.
• YAML results are formatted slightly differently from the chat's.

Test plan

• Real client (claude -p against /api/mcp): read, write, deploy, run, a failing read and a delete; identical results through the in-process path and the loopback HTTP path. A scripted pass of 28 calls (reads, writes, deploys, runs, test runs, failures, deletes) was also identical, error texts byte for byte.
• Instruction order, A/B with claude -p (3 tasks × 2 runs per arm: write and check a script, build an app, run an existing script). Baseline (prompt cut at 4096 by Claude Code): never called get_instructions, and ran the deployed script for "run X". Core rules first: get_instructions before the script and the app, test_run_script for "run X" as the rules ask. A condensed 3.7k prompt with the rest on demand behaved the same and was dropped for this simpler shape. Links in every answer in all arms; cost per task is higher once the model reads the guides.
• Two API servers in one process on separate databases each reach their own router (scratch test, both ran concurrently).
• Earlier passes: Claude Code wrote a script draft, test-ran it, wrote and test-ran a two-step flow, read the diff, deployed the script and ran it; differential runs through the official MCP SDK client against the session tools.
• Access levels on the per-workspace URL: read only, read and run and marked selection tokens list no endpoint tools there; tokens made before this PR are unchanged (42 / 54 tools). Token editor in the browser: a label-only save kept an old folder token's mcp:endpoints:*; switching it to Read only wrote mcp:all mcp:read_only, and to a selection mcp:favorites mcp:runnables_as_tools. Down migration run in a rolled-back transaction: deleted the 12 tokens with the new scopes, kept the others. The approve route accepts the old workspace_id body.
• Routing on /api/mcp: a selection token gets its typed tools (named after their last path segment, as on main) and no endpoint tools; an mcp:all token gets the toolset; the same selection token keeps its endpoint tools on /api/mcp/w/{w}/mcp; no token gives 401 with resource /api/mcp.
• Workspaces: a bound token reaches its workspace only; a listed token exactly its list; an unbound token all of the user's; refusals for ungranted workspaces and a missing workspace_id; the list enforced on the tools server's per-workspace URL and gateway.
• Access: read only (26 tools), read and run (run_script and run_flow ran deployed items on a worker, including with a password argument, stored as an expiring $var: reference; writes and test runs refused), the same tokens on the tools server reading only.
• rebase_draft: a 146 KB stale draft is kept with an editor link; a small one is rebased and its diff returned.
• Links: the run page and the draft-only app editor (with its live preview) open from the returned URLs.
• Mirrored main changes exercised live on the dev instance: folder/group reads with extraPerms, create_resource_type, workspace settings get/set and the non-admin refusal, stale lock drops, AI agent identity kept on deploy.
• OAuth: approve for several / all / one workspace, code exchange and refresh, each through the sign-in page in the browser and by hand; empty pick and a non-member workspace refused.
• UI in the browser: drawer, account settings section, token creation for each access, the tool list per level, the token editor preselecting a token's access, the sign-in page and its one-workspace rule for a selection.
• cargo test: windmill-mcp (90), the MCP part of windmill-api (80), tests/mcp_*.rs, tests/ai_sessions.rs; check:fast (only the errors already on main); chat vitest (the failures are the same 43 as on main).
• Not exercised: OAuth sign-in from Claude.ai or ChatGPT, trigger drafts (no trigger feature in the dev build), applying a datatable migration (no worker for it in the dev build), AI agent step test runs (need an AI provider).

Screenshots

left in draft: adds a migration, touches auth and token paths and has an EE companion, wants a human look before ready

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	9348749
Status:	⚡️  Build in progress...

View logs

[hugocasa]

/review

[claude]

Claude finished @hugocasa's task in 1m 8s —— View job

---

Reviewing PR #11738

• Read AGENTS.md and diff
• Review auth/admission + route guard (mcp/core.rs, utils.rs, oauth_server.rs, session/mod.rs)
• Review tool handlers (session/*.rs)
• Review frontend + generator changes
• Post review

[github-actions]

Pi Review

cc @hugocasa

Should address issues before merging: if_jobs:filter_tags tag restriction is dropped by the workspace MCP server

P1 — if_jobs:filter_tags tag restriction is silently dropped (backend/windmill-mcp/src/server/session.rs:26, backend/windmill-api/src/mcp/session/mod.rs:166)

The server's admission predicate explicitly lets in tokens that carry mcp:all plus an if_jobs:filter_tags: scope — there is even a test asserting admitted(&["mcp:all", "if_jobs:filter_tags:gpu"]) — but every tool call then goes through Ctx::request → send_as_caller(..., scopes: None), which mints a fresh, unscoped JWT (create_jwt_token(..., scopes: None) in utils.rs). get_scope_tags returns None for an unscoped caller, so the tag fence is not applied on either the run or the read side:

• if_jobs:filter_tags is described in jobs.rs as "an orthogonal hard restriction on a scoped token" (check_tag_available_for_workspace at run time; get_completed_job, get_completed_job_result, and the list queries all pass get_scope_tags(authed) into a tag = ANY($n) filter). The existing tools server honors it: its run_script/run_flow handlers call run_wait_result_*_internal with the original ApiAuthed, so the restriction survives.
• The session server does not. A token scoped mcp:all + if_jobs:filter_tags:gpu can use run_script/run_flow/test_run_* to start jobs on any tag, and get_run/list_runs to read jobs outside its allowed tags — widening a deliberately restricted credential.

This contradicts the PR's security summary ("mcp:all combined with any other narrowing scope are refused") and the admits doc comment ("mcp:all and nothing that narrows it"). guarded_url confines the route, not the job tag, so the route guard cannot compensate. If the intent is to drop the fence (it isn't mcp:-shaped), the fix is to refuse such tokens in admits; otherwise the minted JWT must carry the if_jobs:filter_tags: scopes through (mirroring what jwt_scopes_for_proxied_route already does for the existing server).

P2 — admits doc/claim overstates the restriction (backend/windmill-mcp/src/server/session.rs:23-30)

Aside from if_jobs:filter_tags, the predicate also admits mcp:all combined with other mcp:* scopes (mcp:favorites, mcp:all:f/x/*, mcp:hub:*), despite "nothing that narrows it" and the PR summary's "any other narrowing scope are refused". These particular cases are harmless — every mcp:* scope is a subset of mcp:all, so no grant is widened — but the doc comment (and the PR summary) is inaccurate. Worth rewording to match the actual rule while fixing the P1 above.

Considered and dismissed

• guarded_url dot-segment / # / cross-workspace bypass — verified against the implementation and the regression test in session/mod.rs: reqwest::Url::parse resolves dot segments (raw and percent-encoded) and the ends_with("/api{route}") check plus path.contains('#') and the w_id charset check hold. All tool path/query construction goes through enc/path_enc, so literal ?/#/%2F can't reach the guard unencoded. No bypass found.
• Read-only confinement — read_only is enforced in three independent places (tool-list filter, call_session_tool's allowed_read_only, and guarded_url's check_read_only_for_route), so a read-only token neither lists write tools nor issues non-GET requests. The minted JWT not carrying the flag is handled, unlike the tag scope.
• mcp:all + other mcp:* scopes being admitted — safe (subsets), folded into the P2 note above rather than flagged separately.
• read_skill arbitrary-resource read — the path is checked for exact equality against list_skills output before the resources/get_value/{path} call, so a model-chosen path can't reach an arbitrary resource.
• Variable value read-back — get_route adds ?decrypt_secret=false for variables, and the PR's claim "values are never read back" holds in the code.
• misc.rs fetch (SSRF) — targets are fixed hosts (unpkg.com, registry.npmjs.org) with only package-name/version path fragments from the registry response; no caller-controlled host.
• Frontend newMcpTokenReachesWorkspaceServer = !!tokenScopes?.includes('mcp:all') — shows the workspace URL for tokens the server would refuse (mcp:all + non-mcp: scope), but the MCP scope selector is single-mode, so the combination is effectively unreachable from the UI, and the failure mode is a clear connect-time error. Not worth flagging.

Test coverage

• Backend — good unit coverage for the parts that need it: a_call_stays_on_the_route_its_handler_wrote (path guard), every_session_tool_is_implemented_or_editor_only (mirror freshness), and only_an_unnarrowed_mcp_all_token_is_admitted. However, that last test currently asserts the buggy behavior for if_jobs:filter_tags, and there is no test that a tag-scoped token's run/read is confined. No integration test exercises the new HTTP surface end-to-end (the session tools act through the internal HTTP API, so an integration test would need the full server; the PR body documents the manual differential testing instead).
• Frontend — mcpSessionGen.test.ts is the freshness check for the generated gen/ files; no Svelte component tests are expected in this codebase, and none are needed for the token-dialog text change.
• Manual verification still needed before merge — in addition to the PR's own "not exercised" list (trigger drafts, data table/DuckLake beyond "none configured", AI-agent step test runs, OAuth sign-in from Claude.ai/ChatGPT), the P1 above should be confirmed with a token scoped mcp:all + if_jobs:filter_tags:<tag>: connect it to /api/mcp/w/{workspace}/windmill and show that run_script on an item with a different tag, and get_run on an out-of-scope job, are refused rather than executed/returned.

[hugocasa]

Re Pi review: P1 fixed in 991925b. The if_jobs:filter_tags:* scopes are now carried into the JWT behind every tool call, so the fence applies as it does on a direct API call (checked: a gpu-fenced token gets an empty list_runs and cannot read a bun job). P2: the admits doc comment and the PR description now state the actual rule.

/review

[hugocasa]

/review

[claude]

Claude finished @hugocasa's task in 1m 41s —— View job

---

cc @hugocasa

Mergeable, but should ideally address nits: the error shown when a token is refused at connect is stale (windmill-mcp/src/server/session.rs)

What changed since the last review

The fix for the Pi review's P1 in 991925b is correct. The tag fence now holds on every tool call.

• Ctx::request (session/mod.rs:167-176) now passes the caller's if_jobs:filter_tags:* scopes into the JWT it mints.

• On the API side, a JWT that carries only tag scopes still counts as unscoped for route checks. The code that does this:

  • windmill-api-auth/src/scopes.rs:91,107
  • windmill-api-auth/src/lib.rs:448-461, 654-672
  • mcp/utils.rs:580

  So the fix does not lock tool calls out of the routes they need (hub, embeddings, drafts). It does turn on check_tag_available_for_workspace and the tag = ANY(...) read filters. That matches what you saw when testing: a gpu-fenced token got an empty list_runs.

• A tag-fenced token can no longer mint tokens (lib.rs:614-632). That doesn't matter here: as far as I can see, no session tool mints a token. Secret run arguments become secret variables.

Findings

P2: the refusal message no longer matches admits (backend/windmill-mcp/src/server/session.rs:55, inline comment posted). admits now accepts mcp:all together with other mcp:* scopes or an if_jobs:filter_tags:* fence. The error still says "a token whose only scope is mcp:all". The fix is a one-line wording change.

Checked, no issue found

• Admission (session.rs::admits and its test): an mcp:all token combined with a non-mcp: route or path scope is refused. mcp:all:f/x/* alone and mcp:favorites are refused too.

• guarded_url (session/mod.rs:228-261):

  • It refuses #, any other workspace, a workspace id that isn't made of plain characters, and routes that aren't listed.
  • Dot segments are resolved by Url::parse and then caught by the ends_with("/api{route}") check.
  • Every model-supplied id I sampled in runs.rs goes through enc or path_enc.

• Read-only: a read-only token is checked three times:

  • the tool list filter, which also hides get_db_schema;
  • a check at call time;
  • check_read_only_for_route inside the guard.

  Exempting /windmill from the read-only check in auth.rs:1198 only covers the transport, which matches how /mcp and /sse are treated.

• OAuth / WWW-Authenticate: the McpEndpoint extension and the new well-known route give /windmill its own resource URL. The resource URL for /mcp is unchanged, because the extension's default is "mcp".

• send_as_caller split (mcp/utils.rs): create_http_request still scopes its JWT per route, so the existing tools server behaves as before, and job_id provenance is still kept.

• The only direct outbound HTTP in the session tools is misc.rs::fetch. It always goes to unpkg.com or registry.npmjs.org, and a package name or entry path taken from a response cannot change that host.

• No tool handler goes around Ctx::request: I found no use of the DB pool, create_jwt or HTTP_CLIENT under session/ other than the npm fetch above.

I did not read all of the roughly 12k lines of tool handlers line by line for whether they behave the same as core.ts. My review covered the auth and security surface and sampled the handlers. The differential runs described in the PR body are the stronger evidence on behaviour.

AGENTS.md compliance

• AGENTS.md now has a rule for the workspace MCP server: run npm run gen:mcp-session, mirror behaviour in core.ts, and fail a test when a tool is neither implemented nor editor-only. every_session_tool_is_implemented_or_editor_only enforces it.
• I saw no comments that narrate history or address the reviewer in the fix commit.
• The new feature_usage counters depend on the EE companion registry (ee-repo-ref.txt is bumped). Per docs/feature-telemetry.md, a pair that isn't registered is dropped silently. The PR states the counters were verified on an enterprise,private build.

Test coverage

Backend: the unit tests cover what is new and security-relevant:

• a_call_stays_on_the_route_its_handler_wrote covers the path guard.
• only_an_unnarrowed_mcp_all_token_is_admitted covers admission.
• The mirror freshness test covers the tool list.

No automated test checks that a tag-fenced token stays fenced through a session tool call. That needs a running server, and the author checked it by hand (empty list_runs, a bun job could not be read). An integration test with a tag-only token exists for direct API calls (tests/jobs_read_auth.rs) but does not go through this server. I'm not blocking on it.

Frontend: mcpSessionGen.test.ts checks that the generated files are fresh. The token dialog change needs no component test.

Manual verification still needed:

• The PR already lists what was not exercised: trigger drafts, data table and DuckLake tools beyond the "none configured" case, AI agent step test runs, and OAuth sign-in from Claude.ai or ChatGPT. That list stands.
• One more check for the tag fence. With a token scoped mcp:all + if_jobs:filter_tags:gpu, call test_run_script (or run_script) on a script whose tag is bun. It should be refused with a tag error and no job should be queued. Only the read side has been confirmed so far.

[claude]

P2: the refusal message no longer matches admits. Since 991925b, admits also accepts mcp:all together with other mcp:* scopes or an if_jobs:filter_tags:* fence. "whose only scope is mcp:all" now describes a stricter rule than the one enforced. That misleads a user who is refused because of a variables:read:… scope, and one who reads the message as a reason to remove their tag fence.

Suggested change

	return Err(ErrorData::invalid_request(
	"This server needs an `mcp:all` token with no scope from outside `mcp:` other than a job tag filter (`if_jobs:filter_tags:*`)",

[hugocasa]

/review

[claude]

Claude finished @hugocasa's task in 19s —— View job

---

Good to merge

What changed since the last review

The only new commit is 3bb56e6. It fixes the P2 from the last review: the error shown when a token is refused at connect (backend/windmill-mcp/src/server/session.rs:56) now reads:

This server needs an mcp:all token with no other route or path scope

This now matches admits:

• It no longer claims mcp:all must be the only scope, so a tag-fenced token is not told to drop its fence.
• A token refused because of a non-mcp: route or path scope (e.g. variables:read:f/x/*) gets a reason that fits. Other mcp:* scopes that come with mcp:all are subsets of it, so the message doesn't need to list them.

The same rule now appears the same way in the admits doc comment, the refusal message and the PR description.

Findings

None. Everything else in the earlier reviews still holds and is not repeated here:

• Pi review's P1: the tag fence is fixed in 991925b.
• Previous review's "checked, no issue found" list: the admission rule, guarded_url, the read-only checks, the OAuth resource URL and the send_as_caller split are all unchanged by this commit.

AGENTS.md compliance

No new comments, and the commit message follows the repo's conventional format. Nothing to flag.

Test coverage

Backend: the change is a string in an error message. only_an_unnarrowed_mcp_all_token_is_admitted still pins the rule behind it, and no new test is needed.

Manual verification: two checks are still open from before.

• Run side of the tag fence: connect a token scoped mcp:all + if_jobs:filter_tags:gpu to /api/mcp/w/{workspace}/windmill and call test_run_script (or run_script) on a script tagged bun. It should fail with a tag error and queue no job. Only the read side has been confirmed so far.
• What the PR says it did not exercise: trigger drafts, data table and DuckLake tools beyond the "none configured" case, AI agent step test runs, and OAuth sign-in from Claude.ai or ChatGPT.

To see the new wording, connect a token scoped mcp:all + variables:read:f/x/*.

[github-actions]

Codex Review

cc @hugocasa

Should address issues before merging: rebase_draft loses large draft changes

[P1] Preserve oversized rebase changes before deleting the draft — backend/windmill-api/src/mcp/session/workspace.rs:1710

rebase_draft deletes the stored draft, then returns its edits in your_changes. The new MCP runner truncates every result to 87,500 bytes (backend/windmill-mcp/src/server/session.rs:152). When a stale script, flow, or app produces a larger diff, the omitted edits become unrecoverable: the draft is gone, and subsequent reads return the deployed version.

An isolated reproduction using the actual diff and truncation functions produced a 100,128-byte patch from a 100 KB change; the returned text lost its tail. The existing chat rebase implementation returns the full diff without this runner cap. Check the complete serialized response before deletion and preserve the draft when it exceeds the limit, or retain a recoverable snapshot with paginated reads.

Test coverage

Added Rust unit tests cover admission, route confinement, and handler helpers; the frontend test checks generated-definition freshness. No new integration tests cover MCP transport, API authorization, or worker execution. Documentation changes need no automated tests.

The PR author reports real-client, differential, raw-app, token, and token-dialog verification. I ran the isolated truncation reproduction; I did not rerun the project suites or exercise a running instance.

After fixing the finding, rebase a stale draft containing over 87,500 bytes of edits through /windmill and confirm every edit remains recoverable.

Verify the run-side tag fence with an mcp:all + if_jobs:filter_tags:gpu token against a bun runnable: it should reject the run and queue no job.

Trigger drafts remain unexercised. Create, edit, and deploy one through MCP, then confirm the stored trigger configuration matches the draft.

Configured data-table and DuckLake paths remain unexercised. Check schema reads, SQL and migrations with restricted roles, and an actual DuckLake pipeline run.

AI-agent step runs and OAuth sign-in from Claude.ai or ChatGPT remain unexercised. Confirm a step completes with its supplied inputs, and an OAuth-connected client discovers and calls the workspace tools successfully.

[github-actions]

Pi Review

cc @hugocasa

Should address issues before merging: rebase_draft silently loses large draft edits when the returned diff exceeds the runner's 87,500-byte result cap

P1 — rebase_draft deletes the draft, then the runner truncates the diff that is the only copy of the edits (backend/windmill-api/src/mcp/session/workspace.rs:1710, backend/windmill-mcp/src/server/session.rs:152, backend/windmill-mcp/src/server/runner.rs:74)

The new workspace MCP server applies truncate_tool_result (87,500 bytes) to every tool result, in SessionRunner::call_tool (session.rs:152). rebase_draft is a destructive-then-report tool: it computes your_changes — a unified diff, the only surviving record of the caller's edits — and then hard-deletes the draft (workspace.rs:1710, delete_draft → save_draft(value: None)), before returning the diff (workspace.rs:1738).

For a stale script, flow, or app draft whose diff is larger than ~87.5 KB (a big script, a flow/app with a large files/runnables payload), the tail of your_changes is cut off by the runner cap, and the draft is already gone, so those edits are unrecoverable. The model only sees the [truncated …] marker — after the fact. The in-UI chat rebase_draft has no such cap, so this is a regression introduced by routing the same operation through the new server.

The anthropic/maxResultSizeChars = 100_000 meta on the tools does not help here: it raises the client's limit above the runner cap, so the runner's own 87,500-byte truncation is the one that applies, and it truncates before the draft-preserving result can reach the client.

Fix: before delete_draft, serialize your_changes and check it against the cap; if it exceeds it, either refuse the rebase (leave the draft intact) or return the diff without deleting until the client has acknowledged it. At minimum, the destructive delete must not run when the surviving record cannot fit in the result.

P2 — token dialog over-advertises the workspace URL for mixed-scope tokens (frontend/src/lib/components/settings/CreateToken.svelte)

newMcpTokenReachesWorkspaceServer = !!tokenScopes?.includes('mcp:all') shows the workspace-server URL for any token carrying mcp:all. But admits (backend/windmill-mcp/src/server/session.rs:23) refuses a token that combines mcp:all with a scope from outside mcp: that confines routes/paths (e.g. variables:read:f/x/*). Such a token is told to connect to a URL that will reject it at connect. Minor, but the dialog's condition should match admits (exclude tokens with any non-mcp: scope other than if_jobs:filter_tags:*), or the message should note the restriction.

Considered and dismissed

• read_skill builds resources/get_value/{path} without path_enc (guidance.rs:111) — dismissed: the route is get_value/{*path} (catch-all, slashes fine), and the model-supplied path must exactly equal a path from list_skills before the fetch, so no arbitrary resource read. # is refused by guarded_url; ?/% in a real skill path is not a realistic attack surface and would only mis-address a skill the caller already listed.
• result_size_meta 100,000 chars vs 87,500-byte cap — dismissed as a standalone bug: the meta is above the runner cap, which is the stated intent ("so that cap is the one that applies"); its only consequence is folded into the P1 above.
• Read-only confinement gap — dismissed: enforced in three independent places (tool-list filter, call_session_tool's allowed_read_only, and guarded_url's check_read_only_for_route), and that last check delegates to the same map_http_method_to_action the API uses, so a read-only token cannot issue a non-read request.
• admits admitting mcp:all combined with other mcp:* scopes — dismissed: every mcp:* scope is a subset of mcp:all, so no grant is widened (already settled in prior review rounds).
• guarded_url dot-segment / # / cross-workspace / unlisted-route bypass — dismissed: the ends_with("/api{route}") check after Url::parse (which resolves encoded dot segments), the # and w_id charset checks, and the regression test in session/mod.rs all hold; tool path/query construction goes through enc/path_enc.
• if_jobs:filter_tags fence — already fixed in 991925b and confirmed by the prior reviews; not re-flagged.

Test coverage

• Backend (Rust): the diff adds unit tests for admission (only_an_unnarrowed_mcp_all_token_is_admitted), route confinement (a_call_stays_on_the_route_its_handler_wrote), and the implemented/editor-only invariant (every_session_tool_is_implemented_or_editor_only). Nothing covers the P1: there is no test that a rebase_draft whose diff exceeds 87,500 bytes either preserves the draft or surfaces the full diff. No integration test exercises the MCP transport end-to-end against the API authorization.
• Frontend (Svelte/TS): the new mcpSessionGen.test.ts pins generated-definition freshness; no component tests are expected for the token dialog (consistent with the codebase), so none is required there.
• Docs / generated JSON: no automated tests expected beyond the freshness check.

Manual verification

The P1 needs a concrete repro before and after the fix: connect to /api/mcp/w/{workspace}/windmill, create a script draft whose content is large enough that a full rewrite produces a unified diff over ~90 KB, edit the deployed script, then call rebase_draft. The draft is silently deleted and the returned your_changes ends in the truncation marker, with the tail of edits unrecoverable — after the fix, the rebase must either fail leaving the draft intact or return the complete diff.

The run-side tag fence remains unexercised: connect a token scoped mcp:all + if_jobs:filter_tags:gpu and call test_run_script on a script tagged differently; it should reject the run and queue no job (only the read side has been confirmed). Trigger drafts, configured data-table/DuckLake paths, AI-agent step runs, and OAuth sign-in from Claude.ai/ChatGPT also remain unexercised, per the PR's own test plan.

[claude]

Claude finished @hugocasa's task in 1m 42s —— View job

---

Good to merge

What changed since the last review

The last review was at 57d3c7e. The branch commits since then are fac4efa through a4a9ac3, plus a merge of main.

Core rules now come first in the MCP prompt (core.ts, regenerated gen/prompt.json). The rules are now named pieces (draftsOnlyRule, findItemsRule, deployRule, instructionsRule, testDraftRule, runRule).

• External prompt: they come right after the intro as Rules:, and the rest is under More rules:.
• Chat prompt: each piece is used back in its old place.
• Conditions: each rule keeps the condition it had before:
  • instructionsRule still depends on canWriteDraft.
  • testDraftRule still depends on canRunPreview && canWriteDraft.
  • deployRule still depends on canWriteAnyDraft && canDeployAnyKind.
  • runRule keeps its two branches.
• Chat prompt is unchanged: I traced the non-external template and found no change in order or newlines.
• Position in the generated prompt: the core rules end, and Path conventions starts, before character 4000 of base. The {{folder_guidance}} slot, whose text depends on the user, comes after the core rules, so a long folder list cannot push them past Claude Code's 4096-character cut.

Links to drafts and deployed items (workspace.rs::draft_page, deploy.rs::deployed_page):

• A script, flow or agent draft write now returns its editor url. The script editor already handles a path that was never deployed ("undefined for draft-only paths").
• An ai_agent resource links to /agents/get/… once deployed.
• is_agent reads resource_type from the draft value, the same check deploy.rs:226 already makes.

Raw app preview-only view:

• ?view=preview sets compact on RawAppEditor. compact collapses the sidebar with setWithoutPersist, so the user's saved choice is kept, and turns split view off.
• The build log now starts folded whenever the editor opens without split view. That also covers the session preview, which passes defaultSplitWithPreview={false}, as the commit intends.

Login:

• A redirect to /oauth/mcp_authorize now skips the workspace picker. It sits next to the existing /user/workspaces and /projects/import exceptions, and resolvedRd is already resolved, so it adds no new redirect target.
• The consent page no longer shows a toast on a 401 while logged out.

Findings

None.

Nothing in the new commits touches the auth surface: admission, guarded_url, the read-and-run variables/create exception, mcp:workspaces:, OAuth containment, the tag fence and the rebase_draft size guard are all as at the last review.

AGENTS.md compliance

• The new comments state constraints rather than history. Examples: "An MCP client may keep only the first 4096 characters…" in core.ts, and "a detail page shows only the deployed version" in workspace.rs.
• gen/prompt.json was regenerated together with the core.ts change, as the MCP server rule requires ("after changing … the global system prompt … run npm run gen:mcp-session"). mcpSessionGen.test.ts remains the check that it stays in sync.

Test coverage

Backend: the changes are link strings and a check of resource_type. No new test is needed, and the existing session tests still cover the tool list and the route guard.

Frontend:

• The prompt change is covered by the mcpSessionGen.test.ts freshness check.
• The editor, login and consent-page changes are in Svelte, where the codebase does not expect component tests.

Manual verification still needed:

• Draft links: write a script draft, a flow draft and an ai_agent resource draft through /api/mcp, never deploying them. Each returned url should open its editor on the draft, with no menu bar, rather than a not-found page. Then deploy the agent: the returned url should go to /agents/get/….
• Sign-in redirect: log out, start an MCP sign-in as a user with several workspaces, and log in. You should land on the consent page with no "Select a workspace" step and no "Failed to load workspaces" toast.
• Carried over from earlier rounds:
  • the run side of the tag fence;
  • running with a secret argument under read-and-run;
  • the PR's own "Not exercised" list: OAuth from Claude.ai or ChatGPT, trigger drafts, applying a datatable migration, AI agent step test runs.

[github-actions]

Codex Review

Good to merge

Reviewed the changes since the last completed review at 57d3c7e: instruction ordering, MCP login redirects, draft and agent links, and compact app previews. No new issues found. Earlier resolved findings remain fixed.

Test coverage

Backend unit tests cover admission, scope containment, route guards, and handler helpers. The latest link changes add no integration coverage. Frontend coverage includes generated-definition freshness; no component tests are expected. Documentation needs no automated tests.

I ran git diff --check, parsed all five generated MCP JSON files, and confirmed the six core rules begin within the first 4096 characters. These checks passed. I did not run project suites or exercise a running instance; frontend dependencies are absent.

Follow the returned script, flow, and agent draft links and confirm they show the saved drafts. Open an app preview link and confirm the preview, sidebar, and build logs start in the intended state without changing the saved sidebar preference.

For a logged-out user with several workspaces, complete MCP sign-in and confirm it reaches consent directly without a workspace error toast.

The author still lists trigger drafts, applying data-table migrations, AI-agent step runs, and OAuth sign-in from Claude.ai or ChatGPT as unexercised. Verify deployment, execution, and workspace/access restrictions on those paths.

[github-actions]

Pi Review

Good to merge

I reviewed the commits added since the last review (57d3c7e) — the instruction reorder/condense-and-revert pair (c749ef9ef, 288819ffe), the login redirect and 401-toast fixes (1d1ad7c57, 706f235ad), the raw-app preview-only view (6bb2f92a3, a4a9ac354), and the draft/deploy links to editors (4e1e1355e). I also re-verified that the earlier-reviewed findings (tag fence, rebase_draft size guard, guarded_url, read-only confinement, mcp:workspaces: enforcement) are untouched by these commits. No P0/P1/P2 findings.

What the new commits do, checked

• Prompt reorder (core.ts, instructions.rs, guidance.rs): the condensed/full_base split from c749ef9ef is fully reverted in 288819ffe. full_instructions, full_base, the "rules" subject, and the condensed parameter are gone with no dangling references (grep'd full_instructions|full_base|fullBase|condensed|"rules" — zero hits in session/ and chat/global/). The get_instructions tool description no longer lists "rules". For external=false the emitted prompt is byte-identical to 57d3c7e: I traced draftsOnlyRule/findItemsRule/deployRule/instructionsRule/testDraftRule/runRule through the template — the leading-newline placement in the testDraftRule block and the when(!external, …) guards preserve the exact prior output. For external=true, the core rules are emitted once, ahead of paths, and each is correctly omitted from the later "More rules" section so nothing is duplicated.
• Editor links (workspace.rs:1019-1060, deploy.rs:471-484): is_agent reads value.resource_type == "ai_agent" off the value synthesized by draft_item, which always carries resource_type for resources (confirmed in draft_item's "resource" arm), so the /agents/edit|get/{path} routing only fires for real agents and non-agent resources fall through to /resources#/resource/{path}. finish_draft_write's new url field is additive and only produced for script/flow/agent drafts.
• Preview-only view (RawAppEditor.svelte): compact has exactly one caller (apps_raw/edit page, keyed on view=preview) and defaultSplitWithPreview={false} has exactly one caller (RawAppEditorView). splitWithPreview and logsCollapsed are seeded once via untrack, so a later URL change can't re-trigger compaction; the user's toggle still sticks. view=preview has no collision with any existing query-param use.
• Login redirect (Login.svelte:350-360): resolvedRd is already same-origin-reduced via toSameOriginRelativePath before this branch, so adding /oauth/mcp_authorize to the skip-picker list introduces no open redirect. The 401 catch in mcp_authorize only suppresses the toast for the logged-out case.

Considered and dismissed

• page_url with unencoded {path} (draft/deploy links) — pre-existing: save_app already emitted page_url("/apps_raw/edit/{path}") before these commits; the new code only appends query params, and Windmill paths are validated on write. No new class of issue.
• Agent detection silently linking the wrong page — dismissed: draft_item's resource arm sets resource_type from the draft's own resource_type, so deployed_page/draft_page see the same value the deploy persists; a non-agent resource (including resource_type: "") correctly falls through to the resources page.
• logsCollapsed = !defaultSplitWithPreview || compact folding logs somewhere it shouldn't — dismissed: defaultSplitWithPreview=false means "open on the preview tab alone" (see the comment at RawAppEditor.svelte:2399), so folding the log in that mode is the stated intent, and the only other false caller is the session preview.
• view=preview trapping the user in compact mode — dismissed: splitWithPreview is seeded once and owned locally, so the split toggle still works after mount.
• Core rules still beyond the 4096-char cut when the operator intro is long — dismissed: this is a heuristic prompt-order optimization, not a regression; the order is strictly better than before, and the PR's A/B test plan covers it.
• get_instructions losing the "rules" subject mid-PR — dismissed: the "rules" subject only ever existed between c749ef9ef and 288819ffe, both inside this PR; no shipped client depended on it.

Test coverage

• Backend: the new is_agent/draft_page/deployed_page routing has no dedicated unit test. It's a trivial, low-risk branch (a string comparison on the same resource_type field the deploy itself persists), so I'm not treating its absence as a blocker, but a one-case assertion (agent resource → /agents/get/{path}, non-agent resource → /resources#/resource/{path}) would cheaply pin it against a future resource-type rename.
• Frontend: the prompt reorder is covered by the updated mcpSessionGen.test.ts (generated-definition freshness), which is the right layer for this change; Svelte component behavior (compact, the login redirect) is not unit-tested in this codebase, so I'm not asking for component tests.

Manual verification still outstanding (carried from prior rounds, unchanged by these commits): the run side of the if_jobs:filter_tags fence, OAuth sign-in from Claude.ai/ChatGPT, trigger drafts, applying a datatable migration, and AI-agent step test runs. Additionally, the new editor links should be spot-checked once in a real client: after a write_script/write_flow and an agent write_resource, the returned url should open the draft editor (not the deployed page), and a deploy_workspace_item on an agent resource should return /agents/get/<path> rather than the resources page.

[hugocasa]

/review

[claude]

Claude finished @hugocasa's task in 1m 19s —— View job

---

Good to merge

What changed since the last review

The last review was at a4a9ac3. Since then there is a merge of main (da99dec) and four commits: e7cbcdc, d0f3ce5, b7487dc and f9edfc8. I reviewed the four commits. The merge brings in main's own changes and adds nothing to the MCP surface.

mcp:no_endpoints (e7cbcdc).

• New scope: parse_mcp_scopes turns the scope into no_endpoints = true.
• Server flag: both Runner handlers now OR in the server's own flag (scope_config.no_endpoints |= no_endpoints) instead of overwriting it. So the scope holds on /api/mcp/w/{w}/mcp and on the gateway, and /api/mcp still forces it for a selection.
• Routing on /api/mcp: scoped_to_selection (core.rs:501) only checks for mcp:all, so the extra scope changes nothing there. Read-only and read-and-run tokens still reach the session server, and admits accepts them, since every mcp:* scope passes.
• Approval: contains refuses a request that drops no_endpoints when the approver has it. The scope is treated as narrowing only, as the PR says. The new test pins both directions, and mcp:all still covers a fenced selection.
• Picker: the read, read-and-run and selection levels now add the scope; "Build and run" emits mcp:all alone. mcpAccessOf and initialSelection remove it before they classify the token, so it is never counted twice and never turns a token into a "selection".
• Old selection tokens: once the picker is used on one, mcp:no_endpoints is added, so its mcp:endpoints:* scopes stop applying. This is the tightening the commit intends, and it fails closed.

Down migration (d0f3ce5).

• It deletes every token, refresh token and code carrying mcp:read_only, mcp:read_run, mcp:no_endpoints or mcp:workspaces:%, before it restores NOT NULL.
• A token with scopes = NULL gives unnest no rows, so it is kept, which is correct.
• Without this, an older server would ignore these scopes, and the comment says exactly why that widens a token.

Token editor keeps scopes until the picker is used (b7487dc).

• Until a pointer or key event inside the picker's group, scopesToSave is the token's original initialScopes. A save that only changes the label is therefore a no-op on scopes.
• Once the picker is used:
  • The workspace list is kept.
  • mcp:hub:* is carried over when the new level is not a selection. It is a subset of mcp:all, so it widens nothing.
• Expanding "Tools the client gets" also counts as using the picker. On a non-selection level the picker gives back what the token had, so this changes nothing.

One JWT per tool call (f9edfc8).

• Ctx is built fresh for each call_session_tool (core.rs:85-92, 490), so the OnceCell lasts one tool call.
• The minted JWT lives 3600s (utils.rs:722) and carries the same tag scopes on every call. Reusing it within a call changes nothing about what the call can reach.
• The HTTP fallback (send_as_caller) still mints one JWT per request. That only happens when no router is registered.

Old approve body and advertised scopes (f9edfc8).

• If workspace_ids is empty, gateway_oauth_approve takes the legacy workspace_id. The local workspace_ids then goes through the same empty, single and list checks as before. form.workspace_ids is moved into that local, so nothing further down can read the unchecked field.
• supported_scopes now lists the three new scopes.

Findings

None.

Nothing else on the auth surface changed since the last review: admission, guarded_url, the read-and-run variables/create exception, mcp:workspaces: enforcement, the tag fence and the rebase_draft size guard.

AGENTS.md compliance

• The new comments state constraints, not history. Examples:
  • in the down migration: "A server without these scopes ignores them, which would widen every token carrying one";
  • in the editor: "dropping the workspace list would widen the token to every workspace";
  • on the OnceCell: "a tool can make dozens of calls, which would evict other users' cached sessions".
• The commit messages use the conventional format.

Test coverage

Backend:

• test_no_endpoints_scope_only_narrows pins the scope's parsing and its containment.
• a_selection_without_endpoints_gets_none now gets its fence from the scope rather than from the server flag, which covers the |= change.
• The down migration and the reuse of the JWT have no automated test. The PR body reports that the migration was run in a rolled-back transaction, which is reasonable for SQL that only runs on revert.

Frontend: the picker and editor changes are in Svelte components, where the codebase does not expect component tests.

Manual verification still needed:

• Token editor: on an old folder token that carries mcp:endpoints:*, rename it, open "Tools the client gets", and save. The rename should keep mcp:endpoints:*. Opening the tool list on a non-selection token should leave its scopes as they were.
• Consent page: approving at a level below "Build and run" should give a token that lists no endpoint tools on /api/mcp/w/{w}/mcp.
• Carried over from earlier rounds:
  • the run side of the tag fence;
  • running a script with a secret argument under read-and-run;
  • the PR's own "Not exercised" list: OAuth from Claude.ai or ChatGPT, trigger drafts, applying a datatable migration and AI agent step test runs.

[github-actions]

Codex Review

cc @hugocasa

Mergeable, but should ideally address nits: reset MCP edit state for each token

1. [P2] Reset MCP edit state for each token — frontend/src/lib/components/settings/EditTokenScopesModal.svelte:51

   TokensTable keeps one modal mounted, but mcpTouched is never reset when it opens or changes tokens. After using the access picker on token A—even if that edit is cancelled—a label-only save on token B reconstructs B’s scopes. For a legacy folder token, this removes mcp:endpoints:* and adds mcp:no_endpoints, silently disabling its endpoint tools. Reset the flag for each edit alongside the existing label initialization.

Reviewed the four commits since a4a9ac354, including endpoint confinement, rollback revocation, token editing, OAuth body compatibility, and JWT reuse. No new backend issues found.

Test coverage

The updated Rust tests cover the new scope’s containment and endpoint-tool refusal. Frontend component tests are not expected, but the repeated-edit scenario above needs verification. Documentation needs no automated tests.

I ran git diff --check and reproduced the finding with the actual scope expressions in an isolated Node check. I did not run project suites or exercise a running instance. The author reports Rust unit/integration tests, browser checks, legacy OAuth-body acceptance, and transactional rollback verification.

After fixing the finding, use token A’s picker and cancel, then rename a legacy token B without touching its picker. Its stored scopes and callable endpoint tools should remain unchanged.

The author still reports trigger drafts as unexercised. Deploy one and confirm the stored trigger configuration takes effect.

Apply a data-table migration with the required worker and confirm the intended schema change completes. Run an AI-agent step with a real provider and confirm successful execution.

Complete OAuth sign-in from Claude.ai or ChatGPT and confirm tool discovery and calls respect the selected workspace and access restrictions.

[github-actions]

Pi Review

cc @hugocasa

Mergeable, but should ideally address nits: doc comment misattached to NO_ENDPOINTS_SCOPE in scope.rs

Reviewed the four commits added since the last completed review (e7cbcdc, d0f3ce5, b7487dc, f9edfc8): the mcp:no_endpoints scope, the down-migration revocation, the token-editor "only rewrite scopes once the picker is used" change, and the per-tool-call JWT caching. All four are correct; the SlackAgents changes in token.rs/scopes.rs are from the origin/main merge, not this PR.

Finding

P2 — the doc comment above NO_ENDPOINTS_SCOPE still opens with the parser's sentence (backend/windmill-mcp/src/common/scope.rs:160-162)

The insert placed pub const NO_ENDPOINTS_SCOPE between /// Parse MCP scopes from token scope strings and pub fn parse_mcp_scopes, so the whole three-line comment now documents the constant while the function carries no doc. The first line ("Parse MCP scopes from token scope strings") describes the function, not the constant — the constant only "withholds the endpoint tools on every URL". One-line fix: move that first line back down to the function (or drop it), leaving the const its own description.

Considered and dismissed

• Down migration over/under-deletion (20261015140602_...down.sql) — dismissed. The three new DELETE … WHERE EXISTS (SELECT 1 FROM unnest(scopes) …) statements cover every scope the reverted server ignores (mcp:read_only, mcp:read_run, mcp:no_endpoints, mcp:workspaces:%), so no token widens on rollback. scopes is text[] on all three tables (token nullable, the other two NOT NULL); unnest(NULL) yields no rows, so NULL/empty arrays are left alone. The token.workspace_id column was always nullable (first.up.sql), so the workspace_id IS NULL deletes only need to cover the two OAuth tables before their SET NOT NULL, and they do.
• contains + no_endpoints widening (scope.rs:97-99) — dismissed. The check if self.no_endpoints && !requested.no_endpoints { return false } sits before the self.all early return, so a full-access-but-fenced caller (mcp:all mcp:no_endpoints) cannot approve a grant that drops the fence; the added test test_no_endpoints_scope_only_narrows pins exactly this. A caller without the fence may still approve a grant with it (adding it is a narrowing), which is correct.
• gateway_oauth_approve back-compat workspace_id skipping ensure_member (oauth_server.rs:1026-1048) — dismissed. The single-workspace arm ([only] => Some(…)) passes into oauth_approve_inner, which calls ensure_member for the resolved workspace; the several-workspace arm validates each inline. No path to a workspace the approver doesn't belong to.
• no_endpoints enforcement gap on the tools server (runner.rs:321,348,597,627) — dismissed. Both the listing gate (endpoint_tool_in_scope) and the call gate (authorize_endpoint_call) check scope_config.no_endpoints first, so a mcp:no_endpoints token gets no endpoint tools and no endpoint call on the per-workspace URL or gateway. Per-script/flow tools are listed and dispatched on a separate path and are correctly not gated by no_endpoints. The |= OR of the scope and the /api/mcp server flag is the right composition (either source withholds).
• mcpAccessOf reassigning its argument (McpAccessPicker.svelte:19) — dismissed. scopes = scopes.filter(…) rebinds the local parameter; the caller's array is never mutated, and the only caller (keptAside in EditTokenScopesModal) reads mcpScopes reactively via $derived.
• Token editor dropping an old endpoint list on a label-only save (EditTokenScopesModal.svelte) — dismissed as the very thing this commit fixes. With mcpTouched false, scopesToSave is initialScopes verbatim, so scopesChanged is false and no scope rewrite is sent; once the picker is used, re-deriving from what the picker models (and dropping what it doesn't) is the documented intent. Workspace-list scopes are always carried aside; mcp:hub: is kept aside outside "selected" mode and handled by the selector's own hub section in "selected" mode, so neither is silently dropped or duplicated.
• JWT OnceCell caching a stale/under-scoped token (session/mod.rs:186-191,248-253) — dismissed. Ctx is minted fresh per call_session_tool (core.rs:490), the tag-scope set and w_id are constant for the life of one tool call, and get_or_try_init retries on a minting error. The HTTP fallback still mints per request, but that path is the no-router fallback (per-test processes), not the production in-process path the fix targets.

Test coverage

Backend: the new unit tests cover the mcp:no_endpoints scope (test_no_endpoints_scope_only_narrows) and the runner's fenced-selection listing/call refusal, alongside the existing route-guard and admission tests. The OnceCell JWT change is a per-tool-call caching optimization with unchanged observable behavior, so no new test is warranted; the workspace_id back-compat in gateway_oauth_approve is a small parse change without a dedicated test, which is acceptable given the manual passes in the PR body. The down migration is SQL (config-only) with no automated test expected.

Frontend: the access-picker and token-editor changes are Svelte component state logic; the codebase does not expect component tests here, and mcpAccessOf has no sibling *.test.ts to extend.

Manual verification still needed (carried over, not exercised in the PR): OAuth sign-in from a real external client (Claude.ai / ChatGPT), trigger drafts end-to-end (deploy and confirm stored trigger config), applying a datatable migration through the data tools, and AI-agent step test runs against a live provider. For the specific new commits: connect a token created through the access picker at "Read only" to /api/mcp/w/{w}/mcp and confirm the endpoint tool list is empty (no getVariable), then connect an old token minted before mcp:no_endpoints to the same URL and confirm its endpoint list is unchanged; and save a token-editor label change without touching the picker, confirming the stored scopes are byte-identical afterwards.

[hugocasa]

/review

[claude]

Claude finished @hugocasa's task in 35s —— View job

---

Good to merge

What changed since the last review

My last review was at f9edfc8. Since then there are two commits, 6060fc3 and f40271a. Between them they fix the two P2s that Codex and Pi raised on that round.

6060fc3 resets the token editor for each token (Codex P2).

• EditTokenScopesModal.svelte now sets mcpTouched = false in the same open effect that resets the label.
• Before this, using the picker on one token, even if that edit was cancelled, made a label-only save on the next token rebuild its scopes. That save could drop an old token's mcp:endpoints:*. Now a label-only save keeps the stored scopes exactly.

f40271a renames mcp:no_endpoints to mcp:runnables_as_tools and limits the marker to selections.

• Read levels imply the fence: parse_mcp_scopes now sets no_endpoints for mcp:read_only and mcp:read_run themselves. Both scopes are new in this PR, so no token minted before it changes. Read-only and read-and-run tokens still get no endpoint tools on /api/mcp/w/{w}/mcp or the gateway. They just no longer need a separate scope for it.
• Approval containment still narrows only. An approver carrying mcp:read_only or mcp:read_run already can't drop them in contains, and those scopes now carry the fence with them. A selection approver still can't drop mcp:runnables_as_tools. The updated test pins all three cases.
• Picker: read and read_run now emit mcp:all plus the narrowing scope only. A selection gets the marker, and mcpAccessOf and initialSelection strip it before they classify the token.
• Everything else follows the rename: supported_scopes, the down migration's revocation list and the runner test. The doc comment now sits on the constant, and parse_mcp_scopes has its own comment back, which fixes Pi's misattached-doc P2.
• No stale references: grepping for mcp:no_endpoints or NO_ENDPOINTS finds nothing in the code.
• A token minted with mcp:no_endpoints could only have come from this branch between e7cbcdc and f40271a, so no released token is affected.

Findings

None.

These commits don't change the rest of the auth surface: admission, guarded_url, the read-and-run variables/create exception, mcp:workspaces: enforcement, the tag fence and the rebase_draft size guard are as I reviewed them before.

AGENTS.md compliance

• The new comments state constraints rather than history. For example: "Marks a selection … which an older selection, carrying none of the newer scopes, still does."
• The commit messages use the conventional format.

Test coverage

Backend: test_runnables_as_tools_scope_only_narrows pins three things:

• the marker's parsing and containment;
• that mcp:read_only and mcp:read_run each imply no_endpoints;
• that mcp:all still covers a fenced selection.

a_selection_without_endpoints_gets_none covers the runner's refusal. The down migration is SQL that only runs on revert, so no automated test is expected for it.

Frontend: the picker and editor changes are in Svelte components, where the codebase doesn't expect component tests.

Manual verification still needed:

Token editor: open token A's editor, change its access level and cancel. Then open token B, an old folder token that carries mcp:endpoints:*, rename it without touching the picker, and save. B's stored scopes should be unchanged.

Per-workspace URL: create one token at "Read only" and one at "Read and run" from the picker. Their stored scopes should be mcp:all mcp:read_only and mcp:all mcp:read_run, with no marker. Both should list no endpoint tools on /api/mcp/w/{w}/mcp.

Selection: a token made as "Scripts and flows as tools" should carry mcp:runnables_as_tools and list only its typed tools there.

Carried over from earlier rounds:

• the run side of the tag fence;
• a secret argument run under read-and-run;
• what the PR says it did not exercise: OAuth from Claude.ai or ChatGPT, trigger drafts, applying a datatable migration and AI agent step test runs.

[github-actions]

Pi Review

Good to merge

I reviewed the full diff at the head (f40271a) — the MCP scope parser and contains containment (scope.rs), the session-server admission and route guard (session/mod.rs, server/session.rs), the /api/mcp routing and in-process dispatch (core.rs, lib.rs), the OAuth approval/exchange paths (oauth_server.rs), the JWT minting and path-param validation (utils.rs), and the two latest commits (6060fc3, f40271a). Every finding from the prior rounds is fixed at the head, and I found no new P0/P1/P2 issues.

The two latest commits are correct.

• 6060fc3 resets mcpTouched = false inside the same $effect that seeds the label on open/tokenPrefix change, so a label-only save on token B after picking on token A no longer rewrites B's scopes. scopesToSave falls back to initialScopes verbatim until the picker is used, so scopesEqual is false and no updateTokenScopes is sent.
• f40271a renames mcp:no_endpoints → mcp:runnables_as_tools and makes mcp:read_only/mcp:read_run set no_endpoints themselves in parse_mcp_scopes. Grep confirms no stale NO_ENDPOINTS/mcp:no_endpoints references; supported_scopes, the down-migration revocation list, the picker, mcpAccessOf, initialSelection, and both runner tests follow the rename. contains still narrows-only for all three flags (checked before the self.all early return), and the new test pins it.

No new findings. The mcp:read_run + variables/create carve-out (the last blocking item, fixed in 57d3c7e) remains safe: the match is exact-path, call_session_tool re-checks allowed() (so deploy_variable is unreachable by a read-and-run token), and process_secret_args builds the variable path itself (u/{username}/secret_arg/<random>, secret, 7-day expiry) — the model supplies only the value.

Considered and dismissed

• request_origin trusting x-forwarded-host/x-forwarded-proto (server/session.rs:73-83) — dismissed: the derived links are returned to the same client that sent the headers, which already holds the user's full-rights token; the header is never persisted or used for authorization.
• Session JWT minted with no route scope (only the tag fence) (session/mod.rs:167-176) — dismissed: guarded_url and the allowed() re-check in call_session_tool are the confinement, and every route the handlers write is pinned by a_call_stays_on_the_route_its_handler_wrote (dot segments, #, cross-workspace, non-listed routes).
• variables/create under ReadAndRun widening writes — dismissed for the reasons above (exact route, allowed() gate, caller-owned secret folder).
• admits accepting mcp:all alongside other mcp:* scopes — dismissed: every mcp:* scope is a subset of mcp:all, so no grant is widened; non-mcp: route/path scopes are refused (and if_jobs:filter_tags:* is carried).
• mcp:read_run without mcp:all — dismissed: scoped_to_selection sends it to the tools server, where read_run falls back to read_only; the session server's admits also refuses it (scope.all false). A degenerate token that widens nothing.
• gateway_oauth_approve with all_workspaces + a restricted approver — dismissed: contains returns false when a caller with a workspaces list approves a grant whose workspaces is None (_ => return false), so no workspace list can be widened to "all".
• Unbounded to_bytes(…, usize::MAX) in the in-process path / parse_response_body — dismissed: API responses are bounded by the server's own limits, and this matches the pre-existing HTTP-response parsing pattern.

Test coverage

Backend: unit tests cover scope parsing/containment (test_runnables_as_tools_scope_only_narrows, test_read_run_scope_only_narrows, test_contains_*, test_workspaces_scope), the route guard (a_call_stays_on_the_route_its_handler_wrote, including the new variables/create allow/refuse cases), tool-list completeness (every_session_tool_is_implemented_or_editor_only), admission (only_an_unnarrowed_mcp_all_token_is_admitted), selection routing, and the proxied-JWT scope logic. No new end-to-end transport/OAuth integration test is added, which is consistent with the layers changed; the PR author reports exercising the real client paths manually. The down migration is SQL that only runs on revert, so no automated test is expected.

Frontend: the generated-definition freshness check (mcpSessionGen.test.ts) covers the prompt/toolset generation; the picker/editor/consent-page changes are Svelte components, where this codebase does not expect component tests.

Docs/config: the AGENTS.md/system_prompts/README.md additions document the new server and its generation rule; no automated tests expected.

Manual verification still needed: (1) with an mcp:all mcp:read_run token, run_script/run_flow on a deployed runnable with a literal password field should complete and store $var:u/<user>/secret_arg/… with ~7-day expiry, while draft writes and preview runs stay refused; (2) the run side of the if_jobs:filter_tags:* fence should refuse a run of a differently-tagged runnable; (3) trigger drafts, applying a datatable migration, AI-agent step runs, and OAuth sign-in from Claude.ai/ChatGPT remain unexercised per the author and should be verified on a build with the relevant features.

[github-actions]

Codex Review

Good to merge

Reviewed the changes since the previous Codex review at f9edfc877. The token editor now resets its picker state for each edit. The renamed selection scope consistently restricts endpoint tools, and read-only/read-and-run scopes imply the same restriction. OAuth containment and rollback revocation preserve those limits. No new issues found; earlier findings are addressed.

Test coverage

Backend unit tests cover scope parsing, containment and endpoint refusal. I ran all 11 scope tests in an isolated rustc harness with logging stubbed; all passed. git diff --check passed. No new end-to-end integration coverage accompanies the latest changes.

Frontend component tests are not expected. Generated MCP definitions have freshness coverage. Documentation needs no automated tests.

I did not run the Cargo/frontend suites or exercise a running instance. The author reports broader backend, client and browser verification.

Use token A’s picker and cancel, then rename legacy token B without touching its picker. B’s stored scopes and callable endpoint tools should remain unchanged.

Verify read-only, read-and-run and selection tokens cannot list or directly call endpoint tools on the legacy URLs. Confirm legacy tokens retain their existing tools.

Exercise rollback with the renamed mcp:runnables_as_tools scope: narrowed tokens, refresh tokens and authorization codes should be revoked while unrelated credentials remain.

The author still lists trigger drafts, data-table migrations and AI-agent step runs as unexercised. Verify their deployment or execution with the required features, workers and provider.

Complete OAuth sign-in from Claude.ai or ChatGPT and confirm discovery and tool calls respect the selected workspace and access restrictions.

[hugocasa]

✅ Review round clean @ f40271a

[windmill-internal-app]

🤖 Updated ee-repo-ref.txt to b3fd7d49f2ba58f5080fabc3a5686c34a0b2684c after windmill-ee-private PR #877 was merged.