mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-10 16:05:58 +00:00
fix: stop the chat deleting drafts as deployed workspace items (#10476)
* fix: stop the chat deleting drafts as deployed workspace items Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: keep archived scripts deletable and defer malformed args to the schema Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: correct the delete/draft prompt claim and narrow the script probe catch to 404 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Guilhem
Claude Opus 5
@@ -1773,6 +1773,27 @@
|
||||
judgeChecklist:
|
||||
- deletes the deployed script via delete_workspace_item rather than a raw API endpoint
|
||||
|
||||
- id: global-undo-created-draft
|
||||
prompt: |-
|
||||
Create a draft Postgres resource at `u/admin/scratch_db` for host db.example.com port 5432, database `orders`, user `app`, and tell me what fields it ended up with.
|
||||
Once you've shown me that, delete it from the workspace again — I only wanted to see the shape of it.
|
||||
initial: ai_evals/fixtures/frontend/global/initial/user_admin_empty.json
|
||||
runtime:
|
||||
maxTurns: 10
|
||||
validate:
|
||||
draftCountExactly: 0
|
||||
toolExpect:
|
||||
requiredToolsUsed:
|
||||
- write_resource
|
||||
- discard_local_draft
|
||||
forbiddenToolsUsed:
|
||||
- delete_workspace_item
|
||||
- deploy_workspace_item
|
||||
# Undoing a draft leaves no draft behind; validate via tool use.
|
||||
skipJudge: true
|
||||
judgeChecklist:
|
||||
- undoes its own never-deployed resource with discard_local_draft rather than delete_workspace_item
|
||||
|
||||
- id: global-draft-diff-report
|
||||
prompt: |-
|
||||
Update the existing workspace script at `f/evals/global/format_greeting` so the returned message ends with an exclamation mark, keeping everything else the same.
|
||||
|
||||
@@ -204,7 +204,8 @@ vi.mock('$lib/gen', async () => {
|
||||
throw new Error('getResource mock not configured')
|
||||
}),
|
||||
createResource: vi.fn(async () => 'created'),
|
||||
updateResource: vi.fn(async () => 'updated')
|
||||
updateResource: vi.fn(async () => 'updated'),
|
||||
deleteResource: vi.fn(async () => 'deleted')
|
||||
}),
|
||||
VariableService: wrapService(actual.VariableService, {
|
||||
existsVariable: vi.fn(async () => false),
|
||||
@@ -1253,6 +1254,63 @@ describe('global AI tools', () => {
|
||||
).toBeUndefined()
|
||||
})
|
||||
|
||||
// "Create a resource, then never mind": delete_workspace_item must reject a path
|
||||
// that was never deployed, before the confirmation card — otherwise the user
|
||||
// confirms a workspace mutation that 404s past the draft cleanup, leaving the
|
||||
// draft they asked to be rid of.
|
||||
it('rejects deleting a draft-only item and names discard_local_draft', async () => {
|
||||
await callGlobalTool('write_resource', {
|
||||
path: 'u/admin/never_mind_db',
|
||||
value: { host: 'db.example.com', port: 5432 },
|
||||
resource_type: 'postgresql'
|
||||
})
|
||||
|
||||
const error = await getGlobalTool('delete_workspace_item').validateBeforeConfirmation?.({
|
||||
args: { type: 'resource', path: 'u/admin/never_mind_db' },
|
||||
workspace: WORKSPACE,
|
||||
helpers: {}
|
||||
})
|
||||
|
||||
expect(error).toMatch(/only exists as a draft/)
|
||||
expect(error).toMatch(/discard_local_draft/)
|
||||
expect(ResourceService.deleteResource).not.toHaveBeenCalled()
|
||||
expect(
|
||||
getBackendDraft('resource', 'u/admin/never_mind_db', { workspace: WORKSPACE })
|
||||
).toBeDefined()
|
||||
})
|
||||
|
||||
it('lets delete_workspace_item through when the item is deployed', async () => {
|
||||
vi.mocked(ResourceService.existsResource).mockResolvedValueOnce(true)
|
||||
|
||||
await expect(
|
||||
getGlobalTool('delete_workspace_item').validateBeforeConfirmation?.({
|
||||
args: { type: 'resource', path: 'u/admin/deployed_db' },
|
||||
workspace: WORKSPACE,
|
||||
helpers: {}
|
||||
})
|
||||
).resolves.toBeUndefined()
|
||||
})
|
||||
|
||||
// existsScriptByPath filters archived=false but deleteScriptByPath does not, so
|
||||
// probing with it alone would make an archived script undeletable via the chat.
|
||||
it('lets delete_workspace_item through for an archived script', async () => {
|
||||
vi.mocked(ScriptService.existsScriptByPath).mockResolvedValueOnce(false)
|
||||
vi.mocked(ScriptService.getScriptByPath).mockResolvedValueOnce({
|
||||
path: 'f/scripts/archived_one',
|
||||
content: 'export async function main() {}',
|
||||
language: 'bun',
|
||||
archived: true
|
||||
} as any)
|
||||
|
||||
await expect(
|
||||
getGlobalTool('delete_workspace_item').validateBeforeConfirmation?.({
|
||||
args: { type: 'script', path: 'f/scripts/archived_one' },
|
||||
workspace: WORKSPACE,
|
||||
helpers: {}
|
||||
})
|
||||
).resolves.toBeUndefined()
|
||||
})
|
||||
|
||||
// Covers the conflict-on-save / override branch of `persistGlobalDraft`
|
||||
// directly: a non-force save whose recorded baseline is older than the
|
||||
// server row is rejected with `status:'conflict'`, and `override` (force)
|
||||
@@ -4033,7 +4091,7 @@ describe('prepareGlobalSystemMessage', () => {
|
||||
|
||||
expect(content).toContain('Draft tools create or update drafts only')
|
||||
expect(content).toContain(
|
||||
'Use discard_local_draft to remove a draft, including the matching open editor draft'
|
||||
'To undo something you created or changed in this chat, use discard_local_draft'
|
||||
)
|
||||
expect(content).toContain(
|
||||
'After creating or editing a script or flow draft, run test_run_script, test_run_flow, or test_run_step'
|
||||
@@ -4148,10 +4206,10 @@ describe('prepareGlobalSystemMessage', () => {
|
||||
const deleteItem = getGlobalTool('delete_workspace_item')
|
||||
|
||||
expect(discard.def.function.description).toBe(
|
||||
'Discard a draft only. Does not mutate deployed workspace items, but clears the matching open editor draft if one is mounted.'
|
||||
'Discard a draft only — the tool to undo an item you created or edited in this chat and have not deployed. Does not mutate deployed workspace items, but clears the matching open editor draft if one is mounted.'
|
||||
)
|
||||
expect(deleteItem.def.function.description).toBe(
|
||||
'Delete a deployed workspace item. Mutates the workspace.'
|
||||
'Delete an item that is already deployed in the workspace. Mutates the workspace. FAILS if the path has no deployed item, so never call it to undo something you created in this chat — that is a draft; use discard_local_draft instead.'
|
||||
)
|
||||
expect(discard.requiresConfirmation).toBe(true)
|
||||
expect(deleteItem.requiresConfirmation).toBe(true)
|
||||
|
||||
@@ -1184,7 +1184,7 @@ Rules:
|
||||
- Use list_workspace_items to find items and read_workspace_item before changing an existing item. For triggers, pass trigger_kind.
|
||||
- If the user message includes an ACTIVE EDITOR section, treat it as the currently open item and use it for references like "this", "current", or "open editor".
|
||||
- Use deploy_workspace_item only after the user explicitly asks to deploy. It persists a draft to the workspace.
|
||||
- Use discard_local_draft to remove a draft, including the matching open editor draft. Use delete_workspace_item only to delete a deployed workspace item.
|
||||
- To undo something you created or changed in this chat, use discard_local_draft: everything you write is a draft until it is explicitly deployed, so "delete it" / "never mind" / "remove that" about your own work means discarding the draft (it also clears the matching open editor draft). Use delete_workspace_item only to remove an item that is already deployed in the workspace; it mutates the workspace and fails if nothing is deployed at that path.
|
||||
- Use diff to review changes — before deploying, or when the user asks what changed. It is read-only: without arguments it lists every draft in the workspace with its change status; with type+path it returns that item's unified diff (for multi-file apps, pass file to read one file's diff). In a fork, pass against="parent_workspace" to compare the deployed fork with its parent workspace instead. Pass search to grep changed lines across all diffs.
|
||||
- Variable values are never readable. For secrets, create a secret variable and reference it from resources as "$var:path/to/variable".
|
||||
- Use search_resource_types before write_resource, and get_trigger_schema before write_trigger: the trigger config fields differ per kind and are not listed in the write_trigger definition.
|
||||
@@ -3208,12 +3208,13 @@ export const globalTools: Tool<{}>[] = [
|
||||
def: createToolDef(
|
||||
deleteWorkspaceItemSchema,
|
||||
'delete_workspace_item',
|
||||
'Delete a deployed workspace item. Mutates the workspace.'
|
||||
'Delete an item that is already deployed in the workspace. Mutates the workspace. FAILS if the path has no deployed item, so never call it to undo something you created in this chat — that is a draft; use discard_local_draft instead.'
|
||||
),
|
||||
showDetails: true,
|
||||
showFade: true,
|
||||
requiresConfirmation: true,
|
||||
confirmationMessage: 'Delete workspace item',
|
||||
validateBeforeConfirmation: validateDeleteWorkspaceItemTarget,
|
||||
fn: async (ctx) => {
|
||||
const parsed = deleteWorkspaceItemSchema.parse(ctx.args)
|
||||
return deleteWorkspaceItem(parsed, ctx)
|
||||
@@ -3223,7 +3224,7 @@ export const globalTools: Tool<{}>[] = [
|
||||
def: createToolDef(
|
||||
discardLocalDraftSchema,
|
||||
'discard_local_draft',
|
||||
'Discard a draft only. Does not mutate deployed workspace items, but clears the matching open editor draft if one is mounted.'
|
||||
'Discard a draft only — the tool to undo an item you created or edited in this chat and have not deployed. Does not mutate deployed workspace items, but clears the matching open editor draft if one is mounted.'
|
||||
),
|
||||
showDetails: true,
|
||||
showFade: true,
|
||||
@@ -6748,6 +6749,67 @@ async function deployDraft(
|
||||
)
|
||||
}
|
||||
|
||||
// Undoing something created in this chat means discarding a draft, not deleting a
|
||||
// deploy. Must stay in validateBeforeConfirmation, not the tool body: otherwise the
|
||||
// user is asked to confirm a workspace mutation that cannot apply, and the delete
|
||||
// API 404s before the draft cleanup runs, leaving the draft they wanted gone.
|
||||
async function validateDeleteWorkspaceItemTarget(args: {
|
||||
args: unknown
|
||||
workspace: string
|
||||
}): Promise<string | undefined> {
|
||||
// These are the raw tool arguments; the schema parse runs later in the tool body.
|
||||
// Wave a malformed call through so it still fails with the canonical schema error.
|
||||
const parsed = deleteWorkspaceItemSchema.safeParse(args.args)
|
||||
if (!parsed.success) return undefined
|
||||
const { type, path, trigger_kind: triggerKind } = parsed.data
|
||||
if (type === 'trigger' && !triggerKind) return undefined
|
||||
|
||||
const { workspace } = args
|
||||
if (await deployedItemExists(workspace, type, path, triggerKind)) return undefined
|
||||
|
||||
const draft = await getGlobalDraft(workspace, type, path, triggerKind)
|
||||
return draft
|
||||
? `No deployed ${type} at "${path}" — it only exists as a draft, so there is nothing to delete ` +
|
||||
`from the workspace. Call discard_local_draft with the same arguments to remove the draft.`
|
||||
: `No ${type} at "${path}": neither a deployed item nor a draft. Nothing to delete.`
|
||||
}
|
||||
|
||||
async function deployedItemExists(
|
||||
workspace: string,
|
||||
type: WorkspaceItemType,
|
||||
path: string,
|
||||
triggerKind: TriggerKind | undefined
|
||||
): Promise<boolean> {
|
||||
switch (type) {
|
||||
case 'script':
|
||||
// existsScriptByPath is the only probe that filters archived=false, while
|
||||
// deleteScriptByPath removes every row at the path. Fall back to the
|
||||
// archived-inclusive read so an archived script stays deletable.
|
||||
if (await ScriptService.existsScriptByPath({ workspace, path })) return true
|
||||
try {
|
||||
await ScriptService.getScriptByPath({ workspace, path })
|
||||
return true
|
||||
} catch (e) {
|
||||
// Only a 404 means "no script here". Let any other failure propagate rather
|
||||
// than reporting a transient error as a missing item.
|
||||
if ((e as { status?: number } | null | undefined)?.status === 404) return false
|
||||
throw e
|
||||
}
|
||||
case 'flow':
|
||||
return FlowService.existsFlowByPath({ workspace, path })
|
||||
case 'schedule':
|
||||
return ScheduleService.existsSchedule({ workspace, path })
|
||||
case 'trigger':
|
||||
return triggerServices[triggerKind!].exists({ workspace, path })
|
||||
case 'resource':
|
||||
return ResourceService.existsResource({ workspace, path })
|
||||
case 'variable':
|
||||
return VariableService.existsVariable({ workspace, path })
|
||||
case 'app':
|
||||
return AppService.existsApp({ workspace, path })
|
||||
}
|
||||
}
|
||||
|
||||
async function deleteWorkspaceItem(
|
||||
args: { type: WorkspaceItemType; path: string; trigger_kind?: TriggerKind },
|
||||
ctx: WriteDraftCtx
|
||||
|
||||
Reference in New Issue
Block a user