From baaa7c10d26ac359df36532f41e32aa35aeacc03 Mon Sep 17 00:00:00 2001 From: AlexRV12 <71396855+AlexRV12@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:57:01 +0200 Subject: [PATCH] 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. --- .../copilot/chat/AIChatDisplay.svelte | 2 +- .../copilot/chat/AIChatManager.svelte.ts | 12 ++++--- .../copilot/chat/global/apiCatalogTools.ts | 5 +-- .../components/copilot/chat/global/core.ts | 19 +++++++++-- .../copilot/chat/global/sessionAccess.test.ts | 28 +++++++++++++-- .../copilot/chat/global/sessionAccess.ts | 33 +++++++++++++----- .../copilot/chat/global/sessionAssembly.ts | 4 +-- .../chat/global/sessionToolset.test.ts | 34 +++++++++++++++---- .../copilot/chat/global/sessionToolset.ts | 25 ++++++++------ 9 files changed, 124 insertions(+), 38 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte b/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte index 699098c116..81b9589ca0 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte +++ b/frontend/src/lib/components/copilot/chat/AIChatDisplay.svelte @@ -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, diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index cafd8f78cc..9747e57a4e 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -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(undefined) private sessionAccessGeneration = 0 autoAcceptEditsAvailable = $derived(supportsAutoAcceptEdits(this.mode)) autoAcceptEditsActive = $derived( @@ -654,6 +656,10 @@ export class AIChatManager { content: '' }) tools = $state[]>([]) + /** 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(undefined) scriptEditorOptions = $state(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 diff --git a/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.ts b/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.ts index 5d1324ebe3..a26e218e74 100644 --- a/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/apiCatalogTools.ts @@ -45,8 +45,9 @@ const COVERED_ENDPOINTS: Record = { 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', diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index e5bae1a0b3..573a506ef7 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -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//\` — a shared folder scope; the must already exist (a bare \`f/\` with no folder segment is INVALID and will fail). - If the user supplies a fully qualified \`f//...\` 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}/\`. Never invent an \`f//...\` 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="") 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="") 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') } diff --git a/frontend/src/lib/components/copilot/chat/global/sessionAccess.test.ts b/frontend/src/lib/components/copilot/chat/global/sessionAccess.test.ts index c292e5a259..bc3bbbaf41 100644 --- a/frontend/src/lib/components/copilot/chat/global/sessionAccess.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/sessionAccess.test.ts @@ -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 () => { diff --git a/frontend/src/lib/components/copilot/chat/global/sessionAccess.ts b/frontend/src/lib/components/copilot/chat/global/sessionAccess.ts index 3b1bebdbd8..eb3ed32ccc 100644 --- a/frontend/src/lib/components/copilot/chat/global/sessionAccess.ts +++ b/frontend/src/lib/components/copilot/chat/global/sessionAccess.ts @@ -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 } -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 { // 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) }) diff --git a/frontend/src/lib/components/copilot/chat/global/sessionToolset.ts b/frontend/src/lib/components/copilot/chat/global/sessionToolset.ts index 83adc8b654..9d4ece04bd 100644 --- a/frontend/src/lib/components/copilot/chat/global/sessionToolset.ts +++ b/frontend/src/lib/components/copilot/chat/global/sessionToolset.ts @@ -59,9 +59,11 @@ export const SESSION_TOOL_POLICIES: Record = { 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 = { 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 = { 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,