mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix: address review findings on the session capability filter
Six findings from the Claude and Codex review rounds. - `discard_local_draft` is ungated. The backend exempts discarding your OWN draft from `require_can_write_path` precisely so drafts stay cleanable after a role change; gating it stranded that cleanup. - `deploy` splits into `deploy` and `deploy_gated_kinds`, mirroring `deployPermissionForKind`. A direct-deployment lock stops only the kinds that reach `check_deploy_rules`, so schedules and triggers stay deployable and the two kind-taking deploy tools survive the lock; `create_folder` does not, folder being a gated kind. The prompt now names the lock and what it leaves deployable, instead of implying nothing can be deployed. - `COVERED_ENDPOINTS` keyed `createApp` / `updateApp`, which the MCP catalog does not expose; the app-authoring endpoints it does expose, `createAppRawSource` and `updateAppRawSource`, were uncovered and reachable through `call_api_endpoint`. - The YOLO tooltip listed tools a restricted session never ships. Both it and the token estimate now read one `shippedTools`, and `sessionAccess` is reactive so the UI follows the resolution.
This commit is contained in:
@@ -558,7 +558,7 @@
|
||||
})
|
||||
|
||||
const yoloBypassedTools = $derived.by(() => {
|
||||
return aiChatManager.tools
|
||||
return aiChatManager.shippedTools
|
||||
.filter((tool) => tool.requiresConfirmation === true)
|
||||
.map((tool) => ({
|
||||
name: tool.def.function.name,
|
||||
|
||||
@@ -630,7 +630,9 @@ export class AIChatManager {
|
||||
isSessionChat = $state(false)
|
||||
// What the user may do in this session's operating workspace. Undefined until the
|
||||
// first send resolves it — see `resolveSessionAccessForSend`.
|
||||
private sessionAccess: SessionAccess | undefined = undefined
|
||||
// Reactive: `shippedTools` derives from it, so the UI's view of the toolset
|
||||
// follows the resolution instead of a pre-resolution snapshot.
|
||||
private sessionAccess = $state<SessionAccess | undefined>(undefined)
|
||||
private sessionAccessGeneration = 0
|
||||
autoAcceptEditsAvailable = $derived(supportsAutoAcceptEdits(this.mode))
|
||||
autoAcceptEditsActive = $derived(
|
||||
@@ -654,6 +656,10 @@ export class AIChatManager {
|
||||
content: ''
|
||||
})
|
||||
tools = $state<Tool<any>[]>([])
|
||||
/** What the request actually carries: `tools` minus whatever this session's
|
||||
* capabilities withhold. Anything describing the toolset to the user or counting
|
||||
* its cost must read this, not `tools`. */
|
||||
shippedTools = $derived(filterSessionTools(this.tools, this.sessionAccess))
|
||||
helpers = $state<any | undefined>(undefined)
|
||||
|
||||
scriptEditorOptions = $state<ScriptOptions | undefined>(undefined)
|
||||
@@ -1282,9 +1288,7 @@ export class AIChatManager {
|
||||
typeof this.systemMessage.content === 'string'
|
||||
? this.systemMessage.content.length / tokenPerCharacter
|
||||
: 0
|
||||
// The filtered set, matching what the request actually carries: a restricted
|
||||
// session ships fewer definitions than `this.tools` holds.
|
||||
const tools = filterSessionTools(this.tools, this.sessionAccess)
|
||||
const tools = this.shippedTools
|
||||
const toolTokens =
|
||||
tools.length > 0 ? JSON.stringify(tools.map((t) => t.def)).length / tokenPerCharacter : 0
|
||||
return systemTokens + toolTokens
|
||||
|
||||
@@ -45,8 +45,9 @@ const COVERED_ENDPOINTS: Record<string, string> = {
|
||||
createScript: 'write_script',
|
||||
createFlow: 'write_flow',
|
||||
updateFlow: 'patch_flow_json or write_flow',
|
||||
createApp: 'init_app and the app draft tools',
|
||||
updateApp: 'write_app_file / write_app_runnable',
|
||||
// The catalog exposes the raw-source app endpoints, not `createApp`/`updateApp`.
|
||||
createAppRawSource: 'init_app and the app draft tools',
|
||||
updateAppRawSource: 'write_app_file / write_app_runnable',
|
||||
createVariable: 'write_variable',
|
||||
updateVariable: 'write_variable',
|
||||
createResource: 'write_resource',
|
||||
|
||||
@@ -1271,10 +1271,14 @@ const buildGlobalSystemPrompt = (
|
||||
const canWriteDraft = !access || access.capabilities.has('write_draft')
|
||||
const canDeploy = !access || access.capabilities.has('deploy')
|
||||
const canRunPreview = !access || access.capabilities.has('run_preview')
|
||||
// `create_folder` needs the stronger half: a folder is one of the kinds
|
||||
// `check_deploy_rules` gates, so a direct-deployment lock refuses it while the
|
||||
// deploy tools stay usable for schedules and triggers.
|
||||
const canCreateFolder = !access || access.capabilities.has('deploy_gated_kinds')
|
||||
// Each gated block carries its own leading newline, so dropping one leaves no blank
|
||||
// line behind and a full-access prompt is byte-for-byte the ungated text.
|
||||
const when = (cond: boolean, block: string) => (cond ? block : '')
|
||||
const folderGuidance = buildFolderGuidance(username, folderCtx, canDeploy)
|
||||
const folderGuidance = buildFolderGuidance(username, folderCtx, canCreateFolder)
|
||||
const folderGuidanceBlock = folderGuidance ? `\n${folderGuidance}` : ''
|
||||
// `previewTools` doubles as "this is a session chat" — sessions are the only
|
||||
// chats that get the preview tool set. The alpha heads-up only makes sense
|
||||
@@ -1322,7 +1326,7 @@ Path conventions:
|
||||
- \`f/<folder>/<name>\` — a shared folder scope; the <folder> must already exist (a bare \`f/<name>\` with no folder segment is INVALID and will fail).
|
||||
- If the user supplies a fully qualified \`f/<folder>/...\` path, use that exact path; they have already chosen the folder. Do not ask for folder confirmation or substitute a \`u/${username}/...\` path unless a tool rejects it.
|
||||
- Default a bare name with no namespace prefix (e.g. "create a flow called myflow") to \`u/${username}/<name>\`. Never invent an \`f/<folder>/...\` path for a folder that does not exist${when(
|
||||
canDeploy,
|
||||
canCreateFolder,
|
||||
'; create one with `create_folder` only when the user explicitly asks for a new folder'
|
||||
)}.${folderGuidanceBlock}`
|
||||
)}
|
||||
@@ -1391,7 +1395,7 @@ ${pipelineBullet}`
|
||||
canWriteDraft,
|
||||
`
|
||||
- After writing or substantially editing a script / flow / app draft, show it via open_preview(kind, path) so the user sees the editor and live preview right next to the chat. First check whether it is already shown: if unsure, call get_preview_status. Only call open_preview (or offer to) when no preview is open or it is showing a different item — don't re-open a preview already showing the item you just edited.
|
||||
- Building a data pipeline: call open_preview(kind="pipeline", path="<folder>") as the FIRST step, before creating any node — this opens the pipeline editor the user reviews in. path is the folder, not an item; an empty ${when(canDeploy, 'or not-yet-created ')}folder is fine${when(canDeploy, ' (create_folder first if needed, then open it)')}. Opening it registers build_pipeline_node / edit_pipeline_node — use ONLY those to add or change pipeline nodes, never write_script for a pipeline node — they apply directly as unsaved drafts on the canvas (no separate accept/reject step) that the user reviews and deploys. Do not write pipeline scripts without first opening the editor.`
|
||||
- Building a data pipeline: call open_preview(kind="pipeline", path="<folder>") as the FIRST step, before creating any node — this opens the pipeline editor the user reviews in. path is the folder, not an item; an empty ${when(canCreateFolder, 'or not-yet-created ')}folder is fine${when(canCreateFolder, ' (create_folder first if needed, then open it)')}. Opening it registers build_pipeline_node / edit_pipeline_node — use ONLY those to add or change pipeline nodes, never write_script for a pipeline node — they apply directly as unsaved drafts on the canvas (no separate accept/reject step) that the user reviews and deploys. Do not write pipeline scripts without first opening the editor.`
|
||||
)}
|
||||
- When debugging a running raw app, call get_app_runtime_logs to read the live preview's browser console output. It needs the raw app preview open (open_preview kind="raw_app").
|
||||
- To inspect what actually rendered in a running raw app (verify an edit landed on screen, diagnose a blank/empty or wrong view, answer "what's showing"), use search_dom (regex over the live HTML) and read_dom (a line-numbered window). Pass a \`selector\` to scope to an element — prefer the selector from a DOM element chip the user attached — or omit it for the whole page. When a chip lists an \`app_path\`, pass it too so the RIGHT app is read (several previews can be open; a query without \`app_path\` hits the visible one). The DOM is read live and is never in context; no match means the element isn't rendered. Both need the raw app preview open.
|
||||
@@ -2380,6 +2384,7 @@ export function getSessionContextPromptSection(
|
||||
// profile rather than assume the gating happened upstream. Each branch keeps its
|
||||
// "where work lands" fact either way.
|
||||
const canDeploy = !access || access.capabilities.has('deploy')
|
||||
const canDeployGatedKinds = !access || access.capabilities.has('deploy_gated_kinds')
|
||||
const canWriteDraft = !access || access.capabilities.has('write_draft')
|
||||
const canRunPreview = !access || access.capabilities.has('run_preview')
|
||||
const targets = [
|
||||
@@ -2421,6 +2426,14 @@ export function getSessionContextPromptSection(
|
||||
'- No operating workspace is set yet; the user picks one (or a new staged fork) before the first message is sent.'
|
||||
)
|
||||
}
|
||||
// Without this the model reads "deploys" among its targets and has no way to know
|
||||
// which kinds the workspace refuses, so it would keep proposing script and flow
|
||||
// deploys that come back 403.
|
||||
if (canDeploy && !canDeployGatedKinds) {
|
||||
lines.push(
|
||||
'- Direct deployment is disabled in this workspace: only schedules and triggers can be deployed with deploy_workspace_item. Scripts, flows, apps, resources, variables and folders must be promoted from the session\'s deploy panel (fork or pull request) — do not offer to deploy them.'
|
||||
)
|
||||
}
|
||||
return lines.join('\n')
|
||||
}
|
||||
|
||||
|
||||
@@ -43,7 +43,12 @@ describe('resolveSessionAccess', () => {
|
||||
|
||||
it('gives a developer every capability', async () => {
|
||||
const caps = await capabilitiesFor({})
|
||||
expect([...caps].sort()).toEqual(['deploy', 'run_preview', 'write_draft'])
|
||||
expect([...caps].sort()).toEqual([
|
||||
'deploy',
|
||||
'deploy_gated_kinds',
|
||||
'run_preview',
|
||||
'write_draft'
|
||||
])
|
||||
})
|
||||
|
||||
it('leaves an operator only what their token can still do', async () => {
|
||||
@@ -71,12 +76,31 @@ describe('resolveSessionAccess', () => {
|
||||
// wm_deployers can unblock a non-admin. The resolver must not second-guess the shared
|
||||
// preflight, and must not let a deploy refusal take drafting down with it.
|
||||
it('takes deploy from the shared permission check, not from the role', async () => {
|
||||
deployPermission.mockResolvedValue({ ok: false, reason: 'restricted to deployers' })
|
||||
deployPermission.mockResolvedValue({
|
||||
ok: false,
|
||||
reason: 'restricted to deployers',
|
||||
refusedBy: 'RestrictDeployToDeployers'
|
||||
})
|
||||
const caps = await capabilitiesFor({})
|
||||
expect(caps.has('deploy')).toBe(false)
|
||||
expect(caps.has('deploy_gated_kinds')).toBe(false)
|
||||
expect(caps.has('write_draft')).toBe(true)
|
||||
})
|
||||
|
||||
// A direct-deployment lock is the one refusal that does not cover every kind: the
|
||||
// server still accepts schedule and trigger deploys, so the resolver must keep the
|
||||
// weaker half rather than dropping deploy wholesale.
|
||||
it('keeps the ungated half of deploy under a direct-deployment lock', async () => {
|
||||
deployPermission.mockResolvedValue({
|
||||
ok: false,
|
||||
reason: 'direct deployment disabled',
|
||||
refusedBy: 'DisableDirectDeployment'
|
||||
})
|
||||
const caps = await capabilitiesFor({})
|
||||
expect(caps.has('deploy')).toBe(true)
|
||||
expect(caps.has('deploy_gated_kinds')).toBe(false)
|
||||
})
|
||||
|
||||
// Fail open, matching checkDeployPermission: a transient whoami failure must not
|
||||
// strip a session's toolset — the server is still the enforcement point.
|
||||
it('grants everything when the role cannot be resolved', async () => {
|
||||
|
||||
@@ -14,7 +14,14 @@ import { checkDeployPermission } from '$lib/utils_workspace_deploy'
|
||||
* and "operator" differs per capability. Collapsing them into one ordering would get
|
||||
* `write_draft` wrong for a superadmin whose workspace role is operator.
|
||||
*/
|
||||
export type SessionCapability = 'write_draft' | 'run_preview' | 'deploy'
|
||||
export type SessionCapability =
|
||||
| 'write_draft'
|
||||
| 'run_preview'
|
||||
/** May deploy at least something: no operator or deployers-only refusal. */
|
||||
| 'deploy'
|
||||
/** May also deploy the kinds `check_deploy_rules` gates — everything except
|
||||
* schedules and triggers, which no rule covers (`kindGatedByDeployRules`). */
|
||||
| 'deploy_gated_kinds'
|
||||
|
||||
export type SessionAccess = {
|
||||
/** The workspace these capabilities were resolved against — a session targets its
|
||||
@@ -23,7 +30,12 @@ export type SessionAccess = {
|
||||
capabilities: ReadonlySet<SessionCapability>
|
||||
}
|
||||
|
||||
const ALL_CAPABILITIES: SessionCapability[] = ['write_draft', 'run_preview', 'deploy']
|
||||
const ALL_CAPABILITIES: SessionCapability[] = [
|
||||
'write_draft',
|
||||
'run_preview',
|
||||
'deploy',
|
||||
'deploy_gated_kinds'
|
||||
]
|
||||
|
||||
/** Benefit of the doubt: an unresolvable role must not blank the toolset, since a
|
||||
* transient failure would otherwise tell a developer mid-session that they cannot
|
||||
@@ -65,12 +77,17 @@ export async function resolveSessionAccess(workspace: string): Promise<SessionAc
|
||||
capabilities.add('run_preview')
|
||||
}
|
||||
|
||||
// `checkDeployPermission` already carries every term of the backend's `check_deploy_rules`,
|
||||
// superadmin included, so it is the whole answer here. Its per-kind narrowing
|
||||
// (`deployPermissionForKind`) deliberately is not: this filter runs on tool NAMES, before
|
||||
// the model has named a kind, so a direct-deployment lock also withholds the deploy tools
|
||||
// for schedules and triggers — kinds the server would still accept.
|
||||
if ((await checkDeployPermission(workspace, me)).ok) {
|
||||
// `checkDeployPermission` carries every term of the backend's `check_deploy_rules`,
|
||||
// superadmin included, so it is the whole answer. Splitting its verdict in two mirrors
|
||||
// `deployPermissionForKind`: a direct-deployment lock stops the kinds that reach
|
||||
// `check_deploy_rules` and nothing else, so schedules and triggers stay deployable —
|
||||
// the same narrowing the Compare page applies. An operator or deployers-only refusal
|
||||
// covers every kind, so it takes both.
|
||||
const deploy = await checkDeployPermission(workspace, me)
|
||||
if (deploy.ok) {
|
||||
capabilities.add('deploy')
|
||||
capabilities.add('deploy_gated_kinds')
|
||||
} else if (deploy.refusedBy === 'DisableDirectDeployment') {
|
||||
capabilities.add('deploy')
|
||||
}
|
||||
|
||||
|
||||
@@ -15,8 +15,8 @@ import type { SessionAccess } from './sessionAccess'
|
||||
* Assembly of what a GLOBAL-mode chat ships: the system prompt's sections, and the
|
||||
* tool sources they document. Both live here so production and the capability tests
|
||||
* go through the same code — a section or a tool source added to one cannot be
|
||||
* missing from the other, which is how a prompt once kept naming tools the filter
|
||||
* had already withheld.
|
||||
* missing from the other, which is what keeps the prompt from naming a tool the
|
||||
* capability filter withheld.
|
||||
*/
|
||||
|
||||
export type GlobalAssemblyOptions = {
|
||||
|
||||
@@ -108,7 +108,7 @@ describe('session tool policies', () => {
|
||||
// Relevance is the second axis: these need no capability, so `requires` alone
|
||||
// would keep advertising them to a session that can never author anything.
|
||||
it('drops authoring aids when drafts cannot be written', () => {
|
||||
const readOnly = accessWith(['deploy'])
|
||||
const readOnly = accessWith(['deploy', 'deploy_gated_kinds'])
|
||||
expect(sessionToolAllowed('get_instructions', readOnly)).toBe(false)
|
||||
expect(sessionToolAllowed('search_npm_packages', readOnly)).toBe(false)
|
||||
expect(sessionToolAllowed('search_docs', readOnly)).toBe(true)
|
||||
@@ -120,8 +120,29 @@ describe('session tool policies', () => {
|
||||
expect(sessionToolAllowed('create_folder', accessWith(['write_draft', 'run_preview']))).toBe(
|
||||
false
|
||||
)
|
||||
expect(sessionToolAllowed('create_folder', accessWith(['write_draft', 'deploy']))).toBe(true)
|
||||
expect(sessionToolAllowed('create_folder', accessWith(['deploy']))).toBe(false)
|
||||
expect(
|
||||
sessionToolAllowed('create_folder', accessWith(['write_draft', 'deploy_gated_kinds']))
|
||||
).toBe(true)
|
||||
expect(sessionToolAllowed('create_folder', accessWith(['deploy_gated_kinds']))).toBe(false)
|
||||
})
|
||||
|
||||
// A direct-deployment lock stops the kinds check_deploy_rules gates and nothing else,
|
||||
// so the kind-taking deploy tools survive it while create_folder — a gated kind — does
|
||||
// not. Collapsing the two capabilities back into one silently blocks the schedule and
|
||||
// trigger deploys the server accepts.
|
||||
it('keeps the deploy tools but not create_folder under a direct-deployment lock', () => {
|
||||
const locked = accessWith(['write_draft', 'run_preview', 'deploy'])
|
||||
expect(sessionToolAllowed('deploy_workspace_item', locked)).toBe(true)
|
||||
expect(sessionToolAllowed('delete_workspace_item', locked)).toBe(true)
|
||||
expect(sessionToolAllowed('create_folder', locked)).toBe(false)
|
||||
})
|
||||
|
||||
// The backend exempts discarding your OWN draft from require_can_write_path precisely
|
||||
// so drafts stay cleanable after a role change; gating it here would strand them.
|
||||
it('keeps discard_local_draft without write_draft, but not rebase_draft', () => {
|
||||
const readOnly = accessWith([])
|
||||
expect(sessionToolAllowed('discard_local_draft', readOnly)).toBe(true)
|
||||
expect(sessionToolAllowed('rebase_draft', readOnly)).toBe(false)
|
||||
})
|
||||
|
||||
// The prompt is documentation OF the toolset, so it must never name a tool the same
|
||||
@@ -142,8 +163,9 @@ describe('session tool policies', () => {
|
||||
['read-only', [], true],
|
||||
['drafts, no deploy', ['write_draft', 'run_preview'], true],
|
||||
['drafts only', ['write_draft'], true],
|
||||
['drafts, no preview', ['write_draft', 'deploy'], false],
|
||||
['deploy, no drafts', ['deploy'], false]
|
||||
['direct-deployment lock', ['write_draft', 'run_preview', 'deploy'], true],
|
||||
['drafts, no preview', ['write_draft', 'deploy', 'deploy_gated_kinds'], false],
|
||||
['deploy, no drafts', ['deploy', 'deploy_gated_kinds'], false]
|
||||
] as [string, SessionCapability[], boolean][])(
|
||||
'never names a withheld tool in the assembled prompt (%s)',
|
||||
(_label, capabilities, reachable) => {
|
||||
@@ -198,7 +220,7 @@ describe('session tool policies', () => {
|
||||
expect(sessionToolAllowed('write_script', draftsOnly)).toBe(true)
|
||||
expect(sessionToolAllowed('deploy_workspace_item', draftsOnly)).toBe(false)
|
||||
|
||||
const deployOnly = accessWith(['deploy'])
|
||||
const deployOnly = accessWith(['deploy', 'deploy_gated_kinds'])
|
||||
expect(sessionToolAllowed('write_script', deployOnly)).toBe(false)
|
||||
expect(sessionToolAllowed('deploy_workspace_item', deployOnly)).toBe(true)
|
||||
})
|
||||
|
||||
@@ -59,9 +59,11 @@ export const SESSION_TOOL_POLICIES: Record<string, SessionToolPolicy> = {
|
||||
exec_datatable_sql: RUN_PREVIEW,
|
||||
|
||||
// ── API catalog and MCP ─────────────────────────────────────────────────
|
||||
// No capability needed: every endpoint these can reach is a read or a run-by-path.
|
||||
// The authoring and delete endpoints are refused for everyone by COVERED_ENDPOINTS
|
||||
// in apiCatalogTools, so there is no per-role cut left to make here.
|
||||
// No capability needed: COVERED_ENDPOINTS in apiCatalogTools refuses the authoring
|
||||
// and delete endpoints for everyone, leaving reads and run-by-path. That list is
|
||||
// keyed by operationId and the server serves the catalog unfiltered, so it holds
|
||||
// only as long as it tracks the catalog — the server, not this table, is what
|
||||
// actually refuses a call that slips through.
|
||||
search_api_endpoints: NONE,
|
||||
call_api_get: NONE,
|
||||
call_api_endpoint: NONE,
|
||||
@@ -93,10 +95,10 @@ export const SESSION_TOOL_POLICIES: Record<string, SessionToolPolicy> = {
|
||||
get_schedule_schema: AUTHORING_AID,
|
||||
get_db_schema: AUTHORING_AID,
|
||||
// Folder creation runs through the backend's `check_deploy_rules` (folders.rs
|
||||
// `create_folder`), so a workspace that blocks deploys refuses it too — hence
|
||||
// `deploy`, even though a folder is not a deployed item. It is also useless
|
||||
// `create_folder`), and `folder` is one of the gated kinds — so a direct-deployment
|
||||
// lock refuses it even though a folder is not a deployed item. It is also useless
|
||||
// without something to put in it, hence the authoring relevance.
|
||||
create_folder: { requires: ['deploy'], relevance: 'authoring' },
|
||||
create_folder: { requires: ['deploy_gated_kinds'], relevance: 'authoring' },
|
||||
// Ungated on purpose, for two reasons. Plan mode's deliverable is a plan artifact,
|
||||
// which is worth producing for someone else to execute even when this user can
|
||||
// change nothing themselves. And it is a posture the USER selects, so withholding
|
||||
@@ -125,13 +127,16 @@ export const SESSION_TOOL_POLICIES: Record<string, SessionToolPolicy> = {
|
||||
delete_app_file: WRITE_DRAFT,
|
||||
write_app_runnable: WRITE_DRAFT,
|
||||
delete_app_runnable: WRITE_DRAFT,
|
||||
// Relevance, not authorization: discarding your OWN draft deliberately skips
|
||||
// `require_can_write_path` (drafts.rs), so the backend would allow it — but a
|
||||
// session that cannot write a draft has none to discard or rebase.
|
||||
discard_local_draft: WRITE_DRAFT,
|
||||
// Ungated on purpose: discarding your OWN draft skips `require_can_write_path`
|
||||
// (drafts.rs), and the exemption exists precisely so a user who has LOST write
|
||||
// access can still clean up drafts they left behind. Gating it here would strand
|
||||
// that cleanup. Rebasing is not exempt — it writes a fresh draft.
|
||||
discard_local_draft: NONE,
|
||||
rebase_draft: WRITE_DRAFT,
|
||||
|
||||
// ── Deployed-object mutations ───────────────────────────────────────────
|
||||
// Both take the kind as an argument, so they stay available under a direct-deployment
|
||||
// lock: schedules and triggers are still deployable, and the prompt says which.
|
||||
deploy_workspace_item: DEPLOY,
|
||||
delete_workspace_item: DEPLOY,
|
||||
|
||||
|
||||
Reference in New Issue
Block a user