[hugocasa]

Summary

Stacked on #11738. That PR serves the AI session's GLOBAL toolset over MCP by reimplementing the tools in Rust, which left two implementations of every tool: the chat's TypeScript and the server's Rust. This PR makes the chat call the Rust handlers too, so a tool's behaviour is written once and the chat and MCP clients get the same tools.

The chat calls POST /w/{workspace}/ai_session_tools/{name} with the tool's arguments and the context only the browser knows (open editors, the session, the AI provider catalog, the run phase). The server answers with the model-facing text plus effects the browser applies: draft writes and deletes (saved through the browser's own draft store, so open editors, autosave and conflict handling behave as before), card statuses, code diffs. The chat's drafts never touch the server before the browser saves them.

Changes

• Backend
  • mcp/session moved to backend/windmill-api/src/ai_session_tools, compiled without the mcp feature; mcp/session.rs is now a thin MCP adapter.
  • New route POST /w/{w}/ai_session_tools/{name} (chat.rs, in openapi.yaml as callAiSessionTool). Job tokens and scoped tokens are refused.
  • Tools take a Surface (chat or MCP): MCP-only links (Run page, preview_url, draft url) and the rebase size guard stay MCP-only.
  • AI agent provider validation against a catalog (ai_providers.rs): the chat sends the catalog it builds for its prompt; MCP gets one built from resources, without model listings, so it never rules out a model id it cannot see.
  • Runs: the server resolves what a run tool runs (draft or deployed, step, app runnable, SQL) and starts it; the browser keeps the argument form in between.
  • Ported from the chat: npm search (ATA's file tree, npm proxy, @types fallback, bounded fetch), schema-less script writes (the browser infers schemas with its parsers), check-only dry runs before confirmation cards (settings, delete).
  • Datatable migration calls get their own wait budget instead of the 30 s per-request timeout.
• Frontend
  • global/backendSessionTools.ts calls the route and applies effects; global/serverTools.ts and the converted datatable, migration, DuckLake, workspace-settings, hub and skill tools are thin wrappers.
  • About 3,500 lines of tool logic removed from global/core.ts and its helpers (flowRunTree.ts and friends).
  • Flow drafts the server returns are checked against the openflow schema before saving, as the chat did before.
• Kept in TypeScript on purpose (their Rust copies serve MCP only)
  • deploy_workspace_item and diff: they share the UI's deploy and diff code with Compare & Deploy and the session deploy panel.
  • The run tools' form side: coercion, secrets, plan mode, streaming, the outcome text.
  • Editor-only tools, get_instructions, the trigger and schedule schemas.
• AI evals: global mode now runs against a real workspace per attempt, seeded through the API (realWorkspace.ts), instead of a mock backend; about 2,200 lines of mock code that only global mode used are gone.
  • A fixture user who is not the admin is written into the backend's database (WMILL_AI_EVAL_DATABASE_URL), since CE cannot create users through the API; their token is revoked afterwards.
  • A fixture that sets no resource types gets the hub's, and the case waits until the type search indexes them.
  • The scheduled workflow builds with embedding and http_trigger, starts a native worker for the postgresql tag, and sets a short resource type index interval.
  • Fixed harness bugs the mock hid: the run form answered in the wrong shape (every test run started with no arguments), and standalone cases were given a session, so pipeline nodes were refused.
• AGENTS.md: the MCP paragraph now says what is written once and what is still written twice.

Intended differences

• write_flow honours override.
• Error texts carry the server's message rather than a bare HTTP status text (cancel_job, create_folder).
• A refused write keeps its in-progress label next to the error on the card.
• get_db_schema waits up to 120 s, then says the query did not finish; the chat used to wait on the job.
• search_app may order matched files slightly differently when its output is cut.
• npm search (here and in script mode) keeps the registry's best match, and the runner-up only when it scores 60% of it, instead of dropping everything under a score of 1000. The registry's scores have changed scale: a descriptive query's right answer scores a few hundred (rss parser gives rss-parser at 815), so the fixed cut-off returned nothing for anything but exact package names.

Test plan

• Chat tests: global/core.test.ts reworked to the server seam (tests of logic that moved are deleted); new tests for draft-write conflicts and the openflow check.
• Rust unit tests for ai_session_tools (48), with and without the mcp feature.
• Parity harness (not committed): every moved tool run through the old TypeScript and the new path on twin workspaces, outputs diffed; plus an audit of conditional fields, which found and fixed migrations_enabled, the hub display-name fallback and two smaller gaps.
• Browser: write, test run, cancel, live editor update, deploy, delete, flow write and test.
• MCP with a real client (claude -p): write, run, list drafts.
• Route guard: a scoped token and a job token are refused.
• A 35 s migration applies through the chat route.
• Full global eval suite with the judge (107 cases, Sonnet), against the base branch's mock backend: 92 pass here, 89 on base. Over the 37 judged cases the mean judge score is 82.8 here, 80.2 on base (91.2 and 91.9 on the cases both pass). Of the four cases failing only here, test3-flow-create passes 3/3 on rerun and the other three fail at base's own rate over repeated runs (docs and email-trigger cases are model variance on both). The pipeline cases now pass here; they failed on base because of the session bug in the harness.
• A linked agent's unsaved edit is settled before its step runs; a conflict is refused.

Screenshots

Write, test run and result, with the draft open in the editor:

An edit made on the server reaching the open editor:

Deploy (TypeScript path) then delete (server, after its check-only pass):

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	4df7824
Status:	✅  Deploy successful!
Preview URL:	https://805e36c1.windmill.pages.dev
Branch Preview URL:	https://ai-session-backend-mcp-tools.windmill.pages.dev

View logs

[hugocasa]

/review

[claude]

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

---

Reviewing PR

• Read AGENTS.md and the diff against origin/windmill-mcp-server
• Review the new route (chat.rs): auth guard, route mounting
• Review the moved tool modules (runs, npm search, check-only) and the frontend effect-applying seam
• Post findings

[github-actions]

Pi Review

Good to merge

This is a large but well-executed refactor: the AI session's GLOBAL toolset now runs on the backend (the Rust handlers previously served only over MCP), with the chat calling POST /w/{workspace}/ai_session_tools/{name} and applying returned effects through its own draft store. I read the full backend (ai_session_tools/*, mcp/session.rs, route mounting in lib.rs, the openapi.yaml route) and the frontend seam (backendSessionTools.ts, core.ts, the tool wrappers, userDraftAdapter.ts). The port is careful, extensively unit-tested (48 Rust tests; reworked core.test.ts), and the auth/authorization story is sound.

Considered and dismissed

• Auth guard on the new route (chat.rs:45-52): the route refuses job tokens and scope-restricted tokens via job_id.is_some() || !is_effectively_unscoped(...). is_effectively_unscoped mirrors check_scopes/check_route_access (tag-filter scopes still count as unscoped), so an unscoped user token is required. Correct.
• Path traversal / route escape in guarded_url (mod.rs): pre-existing from feat: serve the ai session toolset over mcp at /api/mcp #11738, only moved. It refuses #, non-plain workspace ids, non-workspace routes, and verifies the parsed (dot-segment-resolved) path still ends with the written route. Covered by a_call_stays_on_the_route_its_handler_wrote.
• Minted JWT carrying full caller rights (mint_caller_jwt): for the chat the source token is already unscoped; for MCP, ReadAndRun/ReadOnly access is held to the rule by check_read_only_for_route in guarded_url, and Full requires mcp:all. The job_id provenance (GHSA-hfh4-cx4h-3fcr) is preserved. No escalation.
• set_workspace_settings / get_workspace_settings admin enforcement (settings.rs): no explicit is_admin check in the tool, but both workspaces/settings_document GET/POST handlers call require_admin (workspace_settings_document.rs:454,1650), so a non-admin is refused by the API itself.
• SSRF via read_docs_page url (misc.rs): the model-controlled URL is passed to /docs/page, a pre-existing backend route the old chat also called (docs/core.ts is a thin wrapper over /api/docs/*). Not introduced here.
• SSRF via npm search (misc.rs::npm_search / NpmSources): package names come from the npm registry (npm-validated), file names from the jsdelivr file tree, and all fetches target fixed hosts (registry.npmjs.org, unpkg.com, cdn.jsdelivr.net, data.jsdelivr.com) or the workspace's own configured npm proxy. No attacker-controllable host.
• Regex ReDoS in matches_glob (mod.rs): the Rust regex crate is linear-time (no catastrophic backtracking), and the pattern is built from escaped literals plus */**/?.
• Unbounded npm readme fetch (misc.rs::npm_search, response.text() before js_head truncates): the PR claims "bounded fetch", and the commit bounded the types fetch but not the readme. Dismissed as non-actionable: the host is fixed, the search is capped at size=2, and the model already has arbitrary code execution in the workspace (it can write and run scripts), so a large readme is not a meaningful additional risk.
• Locks across await: Ctx.effects (std::sync::Mutex) is only ever locked for a synchronous push; IN_PROCESS_ROUTERS (RwLock) is read-locked only to .clone(); jwt is a tokio::OnceCell. No lock is held across an await point.
• read_skill unencoded path (guidance.rs:125): the path is validated by exact match against list_skills, and the route is resources/get_value/{*path} (wildcard), so multi-segment paths work and traversal is impossible. Only exotic characters in a skill path would break encoding, and item paths use a restricted charset.
• list_workspace_items limit applied to deployed rows but not the draft overlay: harmless — the draft overlay iterates the user's own (bounded) draft list; no correctness impact.

Test coverage

• Backend: strong unit coverage — every ai_session_tools module carries #[cfg(test)] tests, including the guarded_url route-escape table, provider-catalog validate, run-tree shaping/truncation, secret-arg redaction, migration role/scope annotation, YAML-diff emitter parity, and parse (the hand-rolled zod-schema parser). The one gap worth naming: the route's own auth guard (job token / scoped token refusal) has no committed integration test; it was verified manually per the test plan. Given it's a three-line guard reusing a well-tested predicate, I don't consider this blocking.
• Frontend: core.test.ts was reworked to the new server seam (draft-write conflicts, the openflow schema check on server-returned flow drafts). Deleted tests correspond to logic that moved to Rust and is now covered there. The codebase does not test Svelte components, and no new pure-logic utility needs a sibling test.
• CI / workflows / docs: config-only changes (ai-evals-scheduled.yml builds with embedding,http_trigger and starts a native worker); no automated tests expected.

Manual verification

The one outstanding item is the full global eval suite run against the base branch, which the PR's own test plan marks as "results to follow" — that comparison is the last confirmation that the parity of the moved tool logic holds end-to-end. Beyond that, the author's test plan already covers the paths that matter: browser drive-through of write / test-run / cancel / deploy / delete, MCP with a real claude -p client, the route-guard refusals, and a 35 s migration through the chat route. No additional manual scenario is needed from me.

[github-actions]

Codex Review

cc @hugocasa

Should address issues before merging: linked-agent tests ignore browser drafts

• P1 — backend/windmill-api/src/ai_session_tools/runs.rs:733: Browser test_run_step calls now use with_agent_drafts, which reads only resources/get?...get_draft=true. It ignores the resource draft cells supplied in the chat context. If a linked agent has pending local edits, a failed save, or a draft conflict, the test can silently execute the older server configuration. The previous browser path awaited fetchAgentWithDraft, checked save failures/conflicts, and preferred the local draft. The form does not reliably perform that synchronization: its optional enabled-tools lookup catches failures. Preserve local draft precedence and the failure/conflict guard before queueing the preview.

Test coverage

Rust unit tests and frontend core tests cover argument handling, forms, effects, and run formatting. Add an integration regression where the linked agent’s browser cell differs from its stored draft, including failed-save/conflict cases.

Manually edit a linked agent, then invoke test_run_step from global chat without configuring enabled_tools. Confirm the preview uses the visible agent configuration and refuses unresolved save failures/conflicts.

I inspected the target diff and call paths and ran git diff --check, which passed. I did not run backend/frontend suites or real-workspace evals; this checkout lacks build artifacts and installed frontend/eval dependencies. CI changes need no separate unit tests, but the global smoke/suite should run with the configured worker.

[hugocasa]

/review

[claude]

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

---

cc @hugocasa

Mergeable, but should ideally address nits: the new linked-agent draft settling in core.ts refuses deleted or unreadable agents, unlike the server; a new test sits under another test's comment in core.test.ts

• Read AGENTS.md and the diff against origin/windmill-mcp-server
• Check the latest commit (67763b0, linked-agent draft settling, which answers the Codex P1)
• Review the route guard (chat.rs) and the frontend effect seam (backendSessionTools.ts)
• Post findings

Since the last reviews

67763b0 fixes the Codex P1. test_run_step now calls fetchAgentWithDraft for each linked agent before startPreparedRun. That replays the parked save, and it refuses when a save failed or is in conflict. After that, the server's with_agent_drafts reads a row that matches what the browser holds. The new test covers the conflict case. I did not re-flag anything the Pi review already went through and dismissed.

Findings

• P2: deleted or unreadable linked agents now fail in the browser (frontend/src/lib/components/copilot/chat/global/core.ts:5521). fetchAgentWithDraft lets the getResource 401/403/404 propagate. The server treats those as ordinary states of a link and runs the step against what is deployed (runs.rs:1361-1363). loadLinkedAgentDrafts already handles all of this in one helper: it flushes, refuses on a failed save or a conflict, and tolerates the expected link failures (linkedAgentDrafts.ts:163-169). Using it here keeps the chat and the server consistent. In practice the run would probably fail at the worker anyway, so the main effect is a raw ApiError where the server would give a clearer message.
• P2: test placed under the wrong comment (core.test.ts:2384). The new it went in between the comment block about an agent step's own rows and enabled_tools and the test that comment describes.

Checked and fine

• Route guard (chat.rs:45): job tokens and scoped tokens are refused before the full-rights JWT is minted. Tool refusals come back as BadRequest, and effects are only collected after the call finishes, with no lock held across an await.
• Effects (backendSessionTools.ts): draft writes go through persistDraftAt with the server's last_sync and force. A failed delete stops the sequence, the same way the old throw did.

AGENTS.md compliance

No violations found. The AGENTS.md MCP paragraph matches the new layout: written once in Rust, with deploy_workspace_item, diff and the run form still mirrored. The EDITOR_ONLY and gen:mcp-session rules are still enforced by the existing tests.

Test coverage

Backend: the Rust unit coverage of ai_session_tools is extensive. The route's auth guard still has no committed integration test; it was verified by hand per the test plan. It is a short guard over a well-tested predicate, so I don't consider that blocking.

Frontend: core.test.ts was reworked to the server seam, and the new conflict test pins the regression 67763b0 fixes. A test for the failed-save branch (failureMessage) would be cheap to add next to it.

CI and eval workflows: config only, so no automated tests are expected.

Manual verification

Edit a linked agent's prompt in the agent editor, then from global chat call test_run_step on the flow step that links it, within 1.5 s and without saving. The preview job should use the edited prompt. Then delete that agent, or move it to a folder the user cannot read, and run test_run_step again. You should get the same outcome as the server path, not a raw 404 from the browser. The full global eval suite comparison is still open in the PR's own test plan.

I did not run the backend or frontend test suites in this checkout.

[claude]

P2: fetchAgentWithDraft lets the getResource 401/403/404 propagate. Before this call, a step linked to a deleted agent or one in an unreadable folder still reached the server, and with_agent_drafts (runs.rs:1361-1363) deliberately treats that as "run against what is deployed". Now the browser throws a raw ApiError before startPreparedRun is ever called. loadLinkedAgentDrafts already does flush, refuse on failure or conflict, and tolerate expected link failures, so using it here would keep the two sides consistent:

await loadLinkedAgentDrafts(linkedAgentPaths({ modules: [module] } as FlowValue), workspace)

[claude]

P2: the new test went in between the comment block above (about an agent step opening on the editor's own rows and enabled_tools) and the test that comment describes (test_run_step opens an aiagent step on the step's own configuration). The comment now sits on the wrong test. Move this it above the comment.

[github-actions]

Codex Review

Good to merge

No new issues found. The latest commit resolves the earlier linked-agent draft finding by checking pending saves, failures, and conflicts before starting the step test.

Test coverage

The backend retains 48 unit tests. Frontend regressions cover draft-effect conflicts, flow validation, run forms, and the linked-agent conflict guard. The new HTTP/browser round trip has no committed integration regression. CI and documentation changes need no separate unit tests.

I inspected the target diff and call paths and ran git diff --check, which passed. I did not run backend/frontend suites, real-workspace evals, or browser checks; this checkout has no installed frontend dependencies, backend build, or running instance.

Manually edit a linked agent, then invoke test_run_step from global chat without configuring enabled_tools. Confirm the preview uses the visible edits and refuses unresolved save failures/conflicts. Run the global eval smoke/suite against the real workspace and worker configuration before merge.

[github-actions]

Pi Review

Good to merge

This is a large, careful refactor: the AI session's GLOBAL toolset now runs through POST /w/{workspace}/ai_session_tools/{name} (the same Rust handlers the MCP server already served), with the chat sending browser-only context (open editors, draft cells, the AI-provider catalog, run phase) and applying the returned effects through its own draft store. I read the route and its auth guard (chat.rs), the router/dispatch/URL-guard core (mod.rs), the moved tool groups (runs.rs, workspace.rs, flows.rs, items.rs, misc.rs, data.rs, settings.rs, guidance.rs, apps.rs, deploy.rs, ai_providers.rs), the MCP adapter (mcp/session.rs, mcp/core.rs), and the frontend seam (backendSessionTools.ts, userDraftAdapter.ts, serverTools.ts, runDraftWrite in core.ts, and the linked-agent fix). I also confirmed the Codex P1 from the prior round is addressed by the latest commit.

Considered and dismissed

• Auth on the new route (chat.rs:45): the guard refuses job_id and any scope-restricted token via is_effectively_unscoped. read_only tokens are refused one layer earlier by the auth middleware (auth.rs check_read_only_for_route, which maps POST to a write on this non-run route), so they never reach the handler and never get re-minted through Authed::from, which drops read_only. Workspace membership is the standard /w/{w} auth resolution. Correct.
• mint_caller_jwt drops credential_expiry (mod.rs): the comment on ApiAuthed.credential_expiry says a minted token is "capped at this". The minted JWT is cached in a OnceCell and used only for in-process calls within the single tool request; it is never returned to the client, so a guest's expiry cannot be extended beyond the request. The behavior is also pre-existing (moved from mcp/utils.rs), not introduced here.
• mint_caller_jwt drops read_only / job provenance: job provenance is carried (api_authed.job_id is passed through), so require_super_admin stays job-token-aware (GHSA-hfh4-cx4h-3fcr); read_only cannot reach the minting point as above. No escalation.
• Path traversal / route escape in guarded_url (mod.rs): refuses #, non-plain workspace ids, non-workspace/non-instance routes, and verifies the dot-segment-resolved path still ends with the route the handler wrote. Covered by a_call_stays_on_the_route_its_handler_wrote. Pre-existing (moved from feat: serve the ai session toolset over mcp at /api/mcp #11738).
• Read-and-run token write surface: guarded_url restricts ReadAndRun to jobs/run/p/, jobs/run/f/ and variables/create (secret minting), refusing jobs/run/preview and draft writes; ReadOnly is fully read-only. Correct, and tested.
• SSRF via read_docs_page / npm search (misc.rs): docs go through the pre-existing /docs/* routes the old chat also called; npm targets fixed hosts (registry.npmjs.org, unpkg.com, cdn.jsdelivr.net, data.jsdelivr.com) or the workspace's own npm proxy, with size=2. Not introduced here.
• Unbounded npm readme fetch (misc.rs::npm_search): the documentation body is fetched in full before js_head truncates, and the CDN fetch has no timeout (only the proxy path does). This is a two-package search against a fixed host for a caller that already has arbitrary code execution in the workspace; a negligible resource nit, not a correctness or security finding.
• Regex ReDoS (matches_glob, YAML_* regexes): the Rust regex crate is linear-time; patterns are built from escaped literals plus */**/?.
• Locks across await: Ctx.effects (std::sync::Mutex) is only locked for a synchronous push; IN_PROCESS_ROUTERS is read-locked only to .clone(); jwt is a tokio::OnceCell. No lock spans an await point.
• save_draft returns Ok(true) unconditionally for the chat (workspace.rs): by design — the chat applies the DraftWrite/DraftDelete effect through its own draft store (persistDraftAt/deleteDraftAt), which records the sync baseline and reports conflict/failure itself; the PR documents the "in-progress label next to the error" difference.
• set_workspace_settings / get_workspace_settings admin enforcement (settings.rs): no explicit is_admin check in the tool, but both workspaces/settings_document GET and POST call require_admin (workspace_settings_document.rs:454,1650), so a non-admin is refused by the API and the refusal surfaces through the tool. Verified.
• read_skill unencoded path (guidance.rs:139): the path must exactly match a list_skills entry, and the route is resources/get_value/{*path} (wildcard); item paths use a restricted charset, so traversal is impossible and only a skill path with exotic characters would break encoding. Dismissed.
• call_router unbounded response body (mod.rs, to_bytes(…, usize::MAX)): internal API responses are bounded by the server's own response-size limits; no additional exposure.
• run_tool Start phase trusts the client-supplied RunTarget (runs.rs): the caller is the workspace user themselves (the chat), who can already run arbitrary previews; the run phase only changes when the target is resolved, not who may run it. No escalation.
• Linked-agent draft regression (Codex P1) (runs.rs with_agent_drafts, core.ts testRunFlowStepByPath): the latest commit settles each linked agent's draft via fetchAgentWithDraft before startPreparedRun, which flushes the parked autosave, throws on a failed save, and throws on a conflict; a new core.test.ts case asserts the conflict message and that no run starts. The server-side with_agent_drafts then reads the (now-flushed) stored draft, and correctly treats 401/403/404 links as ordinary states while surfacing every other failure. Fixed.
• Migration timeout budget (data.rs::post_migrations): the datatable migration routes get their own MIGRATION_RUN_TIMEOUT via request_within, separate from the 30 s per-request default; call_router wraps the in-process call in tokio::time::timeout. A long browser-held HTTP request is an ops concern, not a code bug (and was exercised per the test plan).

Test coverage

• Backend: strong unit coverage — 48+ Rust tests across ai_session_tools covering the URL guard, arg parsing/conformance, secret redaction, agent-step forms, the run-tree shaping, flow validation, migration annotations, YAML diffing, and more. The two invariants the dispatch surface needs (every_session_tool_is_implemented_or_editor_only, the guarded-URL tests) are present. The moved API interactions (deploy/delete/settings/runs) are thin calls into routes that already have their own integration tests; I don't see a new handler or DB path here that needs a new integration test.
• Frontend: component tests aren't expected, but the pure-logic seams are covered — core.test.ts was reworked to the server seam and adds the draft-write-conflict and openflow-check tests, and datatableTools.test.ts/ducklakeTools.test.ts/shared.test.ts were updated. The linked-agent conflict regression test pins the new fix.
• CI / evals: config-only (ai-evals-*.yml, README); no automated tests expected, and the PR notes the full global eval suite still needs to run.

Manual verification — the PR's test plan already exercises the in-app paths (write/test/cancel/live-editor update/deploy/delete/flow write, MCP via a real client, route-guard refusals, a 35 s migration). The one thing left open is the full global eval suite against the base branch (listed as pending in the PR), which I'd want green before relying on the eval changes.

[hugocasa]

/review

[claude]

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

---

Good to merge

• Read the commits since the last review (67763b0..HEAD: 67861c8, eb785f2, c908c8f)
• Check the linked-agent fix, npm search scoring, eval harness changes
• Post findings

Since the last review

Both P2 nits from my previous round are fixed:

• Deleted or unreadable linked agents (core.ts:5520): test_run_step now calls loadLinkedAgentDrafts. It still sends the parked save first, and it still refuses a failed save or a conflict (it rethrows AgentDraftUnavailable). It now lets 401, 403 and 404 through, the same way with_agent_drafts does on the server, so the chat and MCP give the same result for a link that cannot be resolved.
• Test placement (core.test.ts): the comment about the aiagent step is back above the test it describes.

New changes, checked and fine

• npm search scoring (misc.rs::confident_npm_matches, script/core.ts): the registry's best match is kept, plus the runner-up when it scores at least 60% of the best. Rust and TypeScript use the same rule. When every score is missing or zero, the cut-off is also zero, so both results are kept; with size=2 that is harmless. One consequence: a query with no relevant package now always returns the registry's top hit. Before, it returned nothing. The PR description states this change, so I'm not flagging it. The unit test covers both the case where the runner-up is dropped and the case where it is kept.
• Eval fixture users written through psql (realWorkspace.ts):
  • The token insert writes token_hash, token_prefix and the plaintext token, the same shape as backend/tests/fixtures/*.sql.
  • Values go through literal(), which escapes ', and execFile runs without a shell.
  • Each token's label carries its workspace id, so dispose revokes exactly that attempt's token, including when seeding fails.
  • The rows go into the eval backend's own database only, and only when WMILL_AI_EVAL_DATABASE_URL is set. Otherwise the harness stops with a clear error and does not quietly run the case as the admin.
• Standalone eval chats (globalEvalRunner.ts): sessionId is only passed for sessionChat cases, which fixes the harness bug that made pipeline nodes get refused.

AGENTS.md compliance

No violations. The new comments explain constraints and say nothing about how the code was drafted.

Test coverage

Backend: the scoring rule has a unit test.

Frontend: the linked-agent conflict test still pins the guard. A failed-save case next to it would be cheap to add but isn't needed.

Eval harness and CI: config and test-infra changes only, so no automated tests are expected.

Manual verification

Link a flow step to an agent, then delete the agent (or move it to a folder the user cannot read). From global chat, call test_run_step on that step. The run should start against the deployed configuration, the same as through MCP, and not stop on a raw 404. I did not run the backend or frontend suites in this checkout.

[github-actions]

Pi Review

Good to merge

I read the route and its auth guard (ai_session_tools/chat.rs), the dispatch/URL-guard core (mod.rs), the moved tool groups (runs.rs, workspace.rs, flows.rs, items.rs, misc.rs, data.rs, settings.rs, guidance.rs, apps.rs, deploy.rs, ai_providers.rs), the MCP adapter (mcp/session.rs), and the frontend seam (backendSessionTools.ts, serverTools.ts, userDraftAdapter.ts, the linked-agent fix in core.ts). This is a careful, extensively unit-tested port: the chat now calls the same Rust handlers the MCP server already served, and the auth/authorization story is layered and sound.

Considered and dismissed

• Auth guard on the new route (chat.rs:45): refuses job_id and any genuinely scope-restricted token via is_effectively_unscoped, which treats empty/if_jobs:filter_tags:-only scope lists as unscoped, mirroring check_scopes/check_route_access. Correct.
• read_only tokens reaching the handler: the auth middleware (auth.rs:1236) applies check_read_only_for_route to every non-MCP-transport route, and map_http_method_to_action maps POST to Write (a run/cancel path aside), so a read-only token is refused before call_tool runs and before it could be re-minted through mint_caller_jwt.
• mint_caller_jwt dropping read_only / job provenance (mod.rs): api_authed.job_id is passed through, so require_super_admin stays job-token-aware (GHSA-hfh4-cx4h-3fcr); read_only cannot reach the minting point for the chat, and the MCP path is held to its rule by allowed() plus guarded_url's check_read_only_for_route for ReadOnly/ReadAndRun.
• Path traversal / route escape (guarded_url): refuses #, non-plain workspace ids, non-workspace/non-instance routes, and re-parses the URL so dot-segment resolution (.., %2e%2e) can't land a call off its route. Covered by a_call_stays_on_the_route_its_handler_wrote.
• Read-and-run write surface: guarded_url restricts ReadAndRun to jobs/run/p/, jobs/run/f/ and variables/create (secret minting for runs); ReadOnly is fully read-only. Tested.
• SSRF via read_docs_page / npm search (misc.rs): docs go through the pre-existing /docs/* routes the old chat also called; npm targets fixed hosts (registry.npmjs.org, unpkg.com, cdn.jsdelivr.net, data.jsdelivr.com) or the workspace's own npm proxy, with size=2.
• Unbounded npm readme fetch / missing CDN timeout (misc.rs::npm_search, fetch): a two-package search against a fixed host for a caller that already has arbitrary code execution in the workspace (it can write and run scripts). A resource nit, not a correctness or security finding.
• Regex ReDoS (matches_glob, the YAML and control-char regexes in deploy.rs): the Rust regex crate is linear-time; patterns are built from escaped literals plus */**/?.
• Locks across await: Ctx.effects (std::sync::Mutex) is only locked for a synchronous push; IN_PROCESS_ROUTERS is read-locked to .clone(); jwt is a tokio::OnceCell. No lock spans an await point.
• Unbounded to_bytes(usize::MAX) in call_router: mirrors the HTTP path's response.text() buffering; no meaningful additional risk.
• sessionToolContext sending all draft cells (including secret variable/resource values) to the server on every call: by design — the chat's drafts now run on the server, which is the user's own authenticated backend that already stores those secrets; the cells are saved back through the browser's own draft store. Not a confidentiality break, only a minor per-call payload cost.
• set_workspace_settings/get_workspace_settings admin enforcement (settings.rs): no explicit is_admin check in the tool, but both workspaces/settings_document handlers call require_admin, so the API itself refuses a non-admin.

The prior-round findings are all resolved in the latest commits: the Codex P1 (linked-agent browser drafts) is fixed by loadLinkedAgentDrafts before startPreparedRun in test_run_step (core.ts:5520), and the two P2 nits (deleted/unreadable linked agents, misplaced test) are addressed.

Test coverage

• Backend: strong unit coverage across ai_session_tools (the mod.rs tests pin the URL guard, runs.rs pins argument conformance/redaction/tree shaping, flows.rs pins inline-script placeholders and lock dropping, misc.rs pins npm scoring, apps.rs pins runnable/policy shaping, data.rs pins schema narrowing and migration annotations). The route's auth guard has no committed integration test; it was verified by hand per the PR test plan, and it is a short guard over the well-tested is_effectively_unscoped predicate, so I don't consider that blocking.
• Frontend: core.test.ts was reworked to the server seam and pins the draft-write conflict and linked-agent guard; the pure-logic utilities that kept logic (script core.ts, aiAgentProviderCatalog) have sibling tests. Component tests are not expected in this codebase.
• CI / eval harness / docs: config and test-infra only, so no automated tests are expected.

Manual verification: The full global eval suite and the MCP client run are listed in the PR's own test plan and were reported as passing. The one remaining in-app scenario worth a human check before merge is the linked-agent draft settlement: edit a linked agent's prompt in the agent editor, then from global chat call test_run_step on the flow step that links it without saving — the preview job should use the edited prompt; then delete that agent (or move it to an unreadable folder) and run test_run_step again, which should run against the deployed configuration rather than a raw 404, matching the server/MCP path.

[github-actions]

Codex Review

Good to merge

No new issues found in the three commits since the previous Codex review. I checked the linked-agent follow-up, eval identity/session changes, and matching npm score filters in Rust and script chat. The earlier linked-agent draft finding remains resolved.

Test coverage

The npm filtering change adds a Rust regression test. I ran git diff --check and four source-level script-chat npm search cases with mocked dependencies; all passed. CI and documentation changes need no separate unit tests.

I did not run the committed backend/frontend suites, browser checks, or real-backend evals; this checkout lacks installed dependencies, a backend build, and a running instance. The PR reports successful browser/MCP checks and the full global eval comparison.

Manual coverage should confirm that descriptive npm queries return useful results in global and script chat. It should also exercise deleted/unreadable linked agents while retaining failed-save/conflict refusal, plus non-admin and standalone-pipeline eval cases against the real backend. Those paths were not independently exercised in this review.

[windmill-internal-app]

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

[hugocasa]

/review

[claude]

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

---

Good to merge

• Identify changes since the last review: the only new PR-side commit is the merge 4df7824 (feat: serve the ai session toolset over mcp at /api/mcp #11738 landed on main; workspace roles ported to the session tools). The rest of c908c8f..HEAD is main's own history.
• Review the workspace-roles port into ai_session_tools and the chat/eval side
• Post findings

Since the last review

The merge commit is the only change that touches this PR's own surface. It does three things.

1. builds_apps now reads the role (ai_session_tools/apps.rs:218-232). It used to read operator_settings.builder_apps; it now reads the capabilities.apps == "compose" entry for this workspace from /workspaces/users. Checked against the role model:

• /workspaces/users returns the role's capabilities per workspace (workspaces.rs:6750-6782). AuthorLevel serializes snake_case, so "compose" matches.
• The port is more correct than the old check. The compatibility builder_apps flag is derived as apps != None (workspaces.rs:6764), so a role with apps: Full would have read as a builder before this change.
• The early return on !ctx.auth.is_operator matches the server: caller_capabilities answers FULL for any non-operator caller (roles.rs:715). A compose role is always operator-flagged, since set_workspace_role stores operator = !caps.code. The Rust side therefore forces the builder policy and the static template exactly when apps.rs/drafts.rs would refuse inline runnables.

2. The chat side (global/core.ts). The prompt now keys buildsFlows/buildsApps on write_inline_flow_code/write_inline_app_code. set_flow_module_code requires write_inline_flow_code. get_instructions' compose-only guidance follows the subject's own inline-code capability, and the open-page gating goes through capabilitiesIn/roleOpenPages. This is consistent with the Rust builds_apps check and with main's role model. Where tool gating is wrong, the server (drafts.rs:475-503) still refuses the inline code.

3. Evals (globalEvalRunner.ts). Operator profiles now pass legacyOperatorCapabilities(...) and seed role/capabilities on the workspace row, so eval fixtures exercise the same capability path as production.

The regenerated gen/tools.json (rebase_draft description) matches the TypeScript definition. The rest of the merge diff in apps.rs is rustfmt reflow only.

Findings

None.

AGENTS.md compliance

No violations. The new comments state the constraint ("a composing role… writes the server refuses") and say nothing about earlier drafts of the code.

Test coverage

Backend: builds_apps makes a live API call, and the merge adds no unit test for it. The behaviour it gates (the builder template and the sandbox policy) is enforced again by the API itself. A test would need an operator fixture with a custom role, so I don't consider it necessary.

Frontend: sessionAccess.test.ts and sessionToolset.test.ts were updated by main's roles PRs and carried through the merge.

Evals/CI: test-infra only.

Manual verification

In a workspace where a member's role has apps: compose (operator, no code), have that member ask the global chat to create an app. init_app should start from the static builder template with no starter runnables, and the saved draft's policy should be sandboxed. Repeat with a role that has apps: full: the demo tour with starter runnables should appear.

I did not run the backend or frontend suites in this checkout.

[github-actions]

Codex Review

Good to merge

No new issues found since the previous Codex review at c908c8f. I checked the workspace-role adaptations, backend/browser run contract, draft conflict handling, and eval routing. The earlier linked-agent draft finding remains resolved.

Test coverage

Backend unit tests and frontend regressions cover tool logic, draft conflicts, flow validation, run forms, and linked-agent guards. The HTTP/browser round trip and latest role adaptations have no committed integration regression. No Svelte component tests or separate automated tests for CI/documentation changes are expected.

I ran git diff --check, which passed. I inspected tests and call paths but did not run backend/frontend suites, real-workspace evals, or browser checks.

In an AI Session, use a member whose role grants app composition. Create an app, add a deployed runnable by path, and preview it. Confirm the draft starts without inline runnables, preserves sandbox isolation, and saves successfully. Also check independently granted script, flow, and app rights expose the corresponding tools.

Run the global eval smoke suite against the merged backend, including non-admin profiles and standalone pipeline cases. Confirm attempts use their fixture identity, complete successfully, and clean up their workspaces and tokens.

[github-actions]

Pi Review

Good to merge

The delta since the prior reviews is the workspace-roles port in the merge commit 4df7824dd: ai_session_tools/apps.rs builds_apps now reads the role's capabilities.apps == "compose" instead of the synthesized operator_settings.builder_apps, the global eval harness passes role capabilities through the new capabilitiesForRole signature, and the MCP tool definitions are regenerated. I read apps.rs, roles.rs, the drafts API (update_draft / require_can_write_path / operator_can_draft), the frontend mirrors (roleCapabilities.ts, sessionAccess.ts, builderAppPolicy.ts), and the eval harness (globalEvalRunner.ts, realWorkspace.ts). The mapping is correct and the enforcement layering holds.

Considered and dismissed

• builds_apps narrowed from builder_apps == true to apps == "compose" (apps.rs:229-231) — correct, not a regression. The synthesized builder_apps is apps != None (compose or full); the new check is compose-only. Per docs/operator-builder-rights.md, the composition checks (forced sandbox, viewer→publisher, builder run identity) must run at compose and are skipped only at full. A full app role requires code: true (RoleCapabilities::validate), so it authors in full and correctly no longer has its sandbox forced. none is refused outright by operator_can_draft (RawApp => caps.apps != None), and validate_operator_composed_app in Draft mode returns PermissionDenied("…can only save sandboxed apps") for a non-sandboxed draft — so conform_builder_policy is a convenience on top of a hard server-side refusal, not the only gate.
• Fail-open when ctx.get("/workspaces/users") errors (builds_apps returns false) — safe: update_draft reads capabilities from the DB directly and refuses a compose role's non-sandboxed draft, so a read failure at worst surfaces a clearer error, never a silently unsandboxed draft.
• Per-write GET /workspaces/users — unchanged from before the merge (same call, only the field read changed); app writes are not a hot path.
• apps = compose + code = true edge case — a valid role config. builds_apps and the frontend's write_inline_app_code (apps === 'full') agree that such a role composes but writes no inline app runnables, and validate_operator_composed_app refuses inline scripts on the draft regardless of code.
• test_run_app_runnable body change (apps.rs:1149-1184) — pure rustfmt reflow of the existing .map_err/finish_run logic; no functional change.
• tools.json rebase_draft description — regenerated from the session source (npm run gen:mcp-session); cosmetic, and mcpSessionGen.test.ts pins freshness.
• Generated instructions.json still documents the legacy operator_settings key — correct: the legacy key still applies as an override of the operator role (docs/workspace-roles.md).
• Eval harness capabilities (globalEvalRunner.ts:162-178) — capabilitiesForRole now takes capabilities; the runner derives legacyOperatorCapabilities(operatorSettings) for operators and FULL_CAPABILITIES otherwise, matching production resolveSessionAccess. seedUser seeds via operator + operator_settings (the legacy path the server mirrors into role_overrides.operator), so the browser-side access and the server-side /workspaces/users capabilities agree.

Test coverage

• Backend: the 48 ai_session_tools unit tests remain, and this delta touches a private helper (builds_apps), not a public surface. There is no test pinning the compose-vs-full mapping in builds_apps specifically; I don't consider that blocking because the behavior is redundantly enforced by the drafts API (validate_operator_composed_app), but a small test asserting "compose → sandbox forced, full/none → not" would guard against a future reversion to the builder_apps == true reading.
• Eval harness / generated files: test infrastructure and generated output, so no additional automated tests are expected; the harness is exercised by the full eval suite the PR already ran (107 cases).

Manual verification

The PR's test plan already covers the browser, MCP, and full eval suite. The remaining scenario worth an end-to-end check is the compose-role path itself: log in as an operator whose role has apps = compose, and from global chat ask to create an app with an inline runnable — the draft should be saved sandboxed (or refused for the inline runnable), and an app composed purely of path runnables should save with policy.sandbox = true; then repeat as a full-app role and confirm the sandbox is not forced. I did not run the backend or frontend suites in this checkout.