fix: read chat drafts via own-draft route so drawer-kind drafts deploy (#9913)

* fix: read chat drafts via own-draft route so drawer-kind drafts deploy Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: cover trigger and resource chat-draft read/deploy regressions Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: cover non-secret variable chat-draft read/deploy regression Completes the drawer-kind matrix from the review notes on #9913: schedule, trigger, and resource already had full write→read→deploy regressions; this adds the variable one (non-secret — the secret flow deploys through the ephemeral in-memory value and is pinned by the existing ephemeral tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(ai_evals): mock getOwnDraft so eval draft hydration stays in-memory The frontend eval adapter intercepts DraftService for benchmark workspaces, but only updateDraft/getDraftForUser/listDrafts. Global eval output collection hydrates draft values through getGlobalDraft, which reads via getOwnDraft — so draft-producing global cases fell through to the real generated client instead of the in-memory benchmark store. Adds a getBenchmarkOwnDraft helper (null on miss, mirroring the 200/null route semantics), wires it into the adapter mock, and pins it in mockBackendDrafts.test.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Ruben Fiszel <ruben@windmill.dev>
2026-08-19 16:02:14 +00:00 · 2026-07-06 11:34:08 +02:00
parent 98013483c8
commit 056ebdb035
5 changed files with 249 additions and 31 deletions
@@ -11,6 +11,7 @@ import type {
 	DataTableTables,
 	DataTableTableSchema,
 	GetDraftForUserResponse,
+	GetOwnDraftResponse,
 	ListDraftsResponse,
 	ScriptLang,
 	UpdateDraftResponse,
@@ -294,8 +295,8 @@ export function getBenchmarkJobLogs(workspace: string, jobId: string): string {
 /**
 * In-memory stand-in for the per-user draft backend (`DraftService`). The global
 * AI chat now persists and reads drafts through the backend DB instead of an
- * in-tab `UserDraft` cell, so the eval mocks the three draft endpoints it
- * exercises (`updateDraft` / `getDraftForUser` / `listDrafts`) and keeps the
+ * in-tab `UserDraft` cell, so the eval mocks the draft endpoints it exercises
+ * (`updateDraft` / `getOwnDraft` / `getDraftForUser` / `listDrafts`) and keeps the
 * saved values here, keyed by workspace + draft kind + storage path. Mirrors the
 * semantics of the production unit test's mock in
 * `frontend/src/lib/components/copilot/chat/global/core.test.ts`.
@@ -379,6 +380,20 @@ export function getBenchmarkDraftForUser(input: {
 	return { value: entry.value, created_at: BENCHMARK_DRAFT_TIMESTAMP }
 }

+/** Mirror `DraftService.getOwnDraft`: `null` (200) when absent — unlike
+ * `getDraftForUser`, absence is not an error on this route. */
+export function getBenchmarkOwnDraft(input: {
+	workspace: string
+	kind: UserDraftItemKind
+	path: string
+}): GetOwnDraftResponse {
+	const entry = benchmarkDrafts.get(benchmarkDraftKey(input.workspace, input.kind, input.path))
+	if (!entry) {
+		return null
+	}
+	return { value: entry.value, created_at: BENCHMARK_DRAFT_TIMESTAMP }
+}
+
 /** Mirror `DraftService.listDrafts`: metadata rows (no value) for a workspace. */
 export function listBenchmarkDrafts(workspace: string): ListDraftsResponse {
 	return [...benchmarkDrafts.values()]
@@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
 import {
 	clearBenchmarkDrafts,
 	getBenchmarkDraftForUser,
+	getBenchmarkOwnDraft,
 	listBenchmarkDrafts,
 	resetBenchmarkMockBackend,
 	seedBenchmarkDraft,
@@ -55,6 +56,27 @@ describe('mockBackend drafts', () => {
 		expect(() => getBenchmarkDraftForUser({ workspace: WORKSPACE, kind: 'variable', path: 'f/evals/token' })).toThrow()
 	})

+	it('returns null from getOwnDraft when no draft exists', () => {
+		expect(
+			getBenchmarkOwnDraft({ workspace: WORKSPACE, kind: 'trigger_schedule', path: 'u/evals/missing' })
+		).toBeNull()
+	})
+
+	// The global chat hydrates drawer-kind drafts (schedule/trigger/resource/variable)
+	// through getOwnDraft — getDraftForUser rejects those kinds as private.
+	it('hydrates a saved drawer-kind draft through getOwnDraft', () => {
+		const value = { path: 'u/evals/nightly', schedule: '0 0 9 * * *' }
+		updateBenchmarkDraft({
+			workspace: WORKSPACE,
+			kind: 'trigger_schedule',
+			path: 'u/evals/nightly',
+			requestBody: { value }
+		})
+		expect(
+			getBenchmarkOwnDraft({ workspace: WORKSPACE, kind: 'trigger_schedule', path: 'u/evals/nightly' })?.value
+		).toEqual(value)
+	})
+
 	it('throws a 404-shaped error when no draft exists', () => {
 		try {
 			getBenchmarkDraftForUser({ workspace: WORKSPACE, kind: 'script', path: 'f/evals/missing' })
@@ -40,6 +40,7 @@ vi.mock('$lib/gen', async () => {
 		getBenchmarkDraftForUser,
 		getBenchmarkFlowByPath,
 		getBenchmarkJobLogs,
+		getBenchmarkOwnDraft,
 		getBenchmarkScriptByHash,
 		getBenchmarkScriptByPath,
 		hasBenchmarkWorkspace,
@@ -86,6 +87,10 @@ vi.mock('$lib/gen', async () => {
 				hasBenchmarkWorkspace(data.workspace)
 					? getBenchmarkDraftForUser(data)
 					: actual.DraftService.getDraftForUser(data),
+			getOwnDraft: async (data: { workspace: string; kind: any; path: string }) =>
+				hasBenchmarkWorkspace(data.workspace)
+					? getBenchmarkOwnDraft(data)
+					: actual.DraftService.getOwnDraft(data),
 			listDrafts: async (data: { workspace: string }) =>
 				hasBenchmarkWorkspace(data.workspace)
 					? listBenchmarkDrafts(data.workspace)
@@ -36,7 +36,7 @@ const { backendDrafts, serverTimestamps, failingWrites, failingReads } = vi.hois
 	// concurrent writer advancing the row; otherwise empty, so the conflict
 	// branch in `updateDraft` stays inert for every pre-existing test.
 	serverTimestamps: new Map<string, string>(),
-	// Keys whose `updateDraft` / `getDraftForUser` throw a non-404 (network/5xx);
+	// Keys whose `updateDraft` / draft reads throw a non-404 (network/5xx);
 	// only set by the error-handling tests, empty otherwise.
 	failingWrites: new Set<string>(),
 	failingReads: new Set<string>()
@@ -132,13 +132,17 @@ vi.mock('$lib/gen', async () => {
 			existsSchedule: vi.fn(async () => false),
 			getSchedule: vi.fn(async () => {
 				throw new Error('getSchedule mock not configured')
-			})
+			}),
+			createSchedule: vi.fn(async () => 'created'),
+			updateSchedule: vi.fn(async () => 'updated')
 		}),
 		HttpTriggerService: wrapService(actual.HttpTriggerService, {
 			existsHttpTrigger: vi.fn(async () => false),
 			getHttpTrigger: vi.fn(async () => {
 				throw new Error('getHttpTrigger mock not configured')
-			})
+			}),
+			createHttpTrigger: vi.fn(async () => 'created'),
+			updateHttpTrigger: vi.fn(async () => 'updated')
 		}),
 		AppService: wrapService(actual.AppService, {
 			existsApp: vi.fn(async () => false),
@@ -156,7 +160,9 @@ vi.mock('$lib/gen', async () => {
 			existsResource: vi.fn(async () => false),
 			getResource: vi.fn(async () => {
 				throw new Error('getResource mock not configured')
-			})
+			}),
+			createResource: vi.fn(async () => 'created'),
+			updateResource: vi.fn(async () => 'updated')
 		}),
 		VariableService: wrapService(actual.VariableService, {
 			existsVariable: vi.fn(async () => false),
@@ -190,14 +196,27 @@ vi.mock('$lib/gen', async () => {
 				return { status: 'saved', current_timestamp: '2026-06-15T00:00:00Z' }
 			}),
 			getDraftForUser: vi.fn(async ({ kind, path }: any) => {
+				// The real endpoint rejects drawer kinds up front (drafts for
+				// schedule/trigger/resource/variable are private to their owner) —
+				// mirror it so a caller regressing to this route for those kinds
+				// fails in tests the same way it does against the backend.
+				if (!['script', 'flow', 'app', 'raw_app'].includes(kind))
+					throw Object.assign(new Error('drafts for this item kind are private to their owner'), {
+						status: 404
+					})
 				const key = `${kind}:${path}`
 				if (failingReads.has(key)) throw Object.assign(new Error('server error'), { status: 500 })
-				// 404-shaped (status) like the real ApiError, so the adapter's
-				// narrowed catch treats it as "no draft" rather than re-throwing.
 				if (!backendDrafts.has(key))
 					throw Object.assign(new Error('no draft for that owner at that path'), { status: 404 })
 				return { value: backendDrafts.get(key), created_at: '2026-06-15T00:00:00Z' }
 			}),
+			getOwnDraft: vi.fn(async ({ kind, path }: any) => {
+				const key = `${kind}:${path}`
+				if (failingReads.has(key)) throw Object.assign(new Error('server error'), { status: 500 })
+				// The real endpoint returns 200 with null when the user has no draft.
+				if (!backendDrafts.has(key)) return null
+				return { value: backendDrafts.get(key), created_at: '2026-06-15T00:00:00Z' }
+			}),
 			listDrafts: vi.fn(async () =>
 				Array.from(backendDrafts.entries()).map(([key, value]) => {
 					const idx = key.indexOf(':')
@@ -1163,6 +1182,170 @@ describe('global AI tools', () => {
 		expect(draft).not.toHaveProperty('override')
 	})

+	// Schedule drafts (like all drawer kinds) are private to their owner, so the
+	// cross-user draft route 404s on them. Reading them back must go through the
+	// own-draft route, else a freshly written schedule draft is listed but can
+	// never be read or deployed.
+	it('reads and deploys a schedule draft written by the chat', async () => {
+		await callGlobalTool('write_schedule', {
+			path: 'u/admin/test_schedule_greet',
+			schedule: '0 0 9 * * *',
+			timezone: 'UTC',
+			script_path: 'f/scripts/greet',
+			is_flow: false,
+			args: {}
+		})
+
+		const readRaw = await callGlobalTool('read_workspace_item', {
+			type: 'schedule',
+			path: 'u/admin/test_schedule_greet'
+		})
+		expect(JSON.parse(readRaw)).toMatchObject({
+			type: 'schedule',
+			path: 'u/admin/test_schedule_greet',
+			isDraft: true
+		})
+
+		await callGlobalTool('deploy_workspace_item', {
+			type: 'schedule',
+			path: 'u/admin/test_schedule_greet'
+		})
+		expect(ScheduleService.createSchedule).toHaveBeenCalledWith({
+			workspace: WORKSPACE,
+			requestBody: expect.objectContaining({
+				path: 'u/admin/test_schedule_greet',
+				schedule: '0 0 9 * * *',
+				script_path: 'f/scripts/greet'
+			})
+		})
+		// The draft is consumed by the deploy.
+		expect(
+			getBackendDraft('trigger_schedule', 'u/admin/test_schedule_greet', {
+				workspace: WORKSPACE
+			})
+		).toBeUndefined()
+	})
+
+	// Same private-owner read path as schedules, for the trigger drawer kinds.
+	it('reads and deploys a trigger draft written by the chat', async () => {
+		await callGlobalTool('write_trigger', {
+			kind: 'http',
+			config: {
+				path: 'u/admin/fresh_route',
+				script_path: 'f/scripts/handler',
+				is_flow: false,
+				route_path: 'api/fresh',
+				http_method: 'get',
+				authentication_method: 'none',
+				is_static_website: false
+			}
+		})
+
+		const readRaw = await callGlobalTool('read_workspace_item', {
+			type: 'trigger',
+			trigger_kind: 'http',
+			path: 'u/admin/fresh_route'
+		})
+		expect(JSON.parse(readRaw)).toMatchObject({
+			type: 'trigger',
+			triggerKind: 'http',
+			path: 'u/admin/fresh_route',
+			isDraft: true
+		})
+
+		await callGlobalTool('deploy_workspace_item', {
+			type: 'trigger',
+			trigger_kind: 'http',
+			path: 'u/admin/fresh_route'
+		})
+		expect(HttpTriggerService.createHttpTrigger).toHaveBeenCalledWith({
+			workspace: WORKSPACE,
+			requestBody: expect.objectContaining({
+				path: 'u/admin/fresh_route',
+				route_path: 'api/fresh',
+				script_path: 'f/scripts/handler'
+			})
+		})
+		expect(
+			getBackendDraft('trigger_http', 'u/admin/fresh_route', { workspace: WORKSPACE })
+		).toBeUndefined()
+	})
+
+	// Same private-owner read path as schedules, for the resource drawer kind.
+	it('reads and deploys a resource draft written by the chat', async () => {
+		await callGlobalTool('write_resource', {
+			path: 'u/admin/fresh_db',
+			value: { host: 'db.example.com', port: 5432 },
+			resource_type: 'postgresql',
+			description: 'fresh database'
+		})
+
+		const readRaw = await callGlobalTool('read_workspace_item', {
+			type: 'resource',
+			path: 'u/admin/fresh_db'
+		})
+		expect(JSON.parse(readRaw)).toMatchObject({
+			type: 'resource',
+			path: 'u/admin/fresh_db',
+			isDraft: true
+		})
+
+		await callGlobalTool('deploy_workspace_item', {
+			type: 'resource',
+			path: 'u/admin/fresh_db'
+		})
+		expect(ResourceService.createResource).toHaveBeenCalledWith({
+			workspace: WORKSPACE,
+			requestBody: expect.objectContaining({
+				path: 'u/admin/fresh_db',
+				resource_type: 'postgresql',
+				value: { host: 'db.example.com', port: 5432 }
+			})
+		})
+		expect(
+			getBackendDraft('resource', 'u/admin/fresh_db', { workspace: WORKSPACE })
+		).toBeUndefined()
+	})
+
+	// Same private-owner read path as schedules, for the variable drawer kind.
+	// Secret variables deploy through the ephemeral in-memory value instead
+	// (see the ephemeral-value tests above); this pins the plain-value cycle.
+	it('reads and deploys a non-secret variable draft written by the chat', async () => {
+		await callGlobalTool('write_variable', {
+			path: 'u/admin/fresh_config',
+			value: 'plain-value',
+			is_secret: false,
+			description: 'fresh config'
+		})
+
+		const readRaw = await callGlobalTool('read_workspace_item', {
+			type: 'variable',
+			path: 'u/admin/fresh_config'
+		})
+		expect(JSON.parse(readRaw)).toMatchObject({
+			type: 'variable',
+			path: 'u/admin/fresh_config',
+			isDraft: true
+		})
+
+		await callGlobalTool('deploy_workspace_item', {
+			type: 'variable',
+			path: 'u/admin/fresh_config'
+		})
+		expect(VariableService.createVariable).toHaveBeenCalledWith({
+			workspace: WORKSPACE,
+			requestBody: expect.objectContaining({
+				path: 'u/admin/fresh_config',
+				value: 'plain-value',
+				is_secret: false,
+				description: 'fresh config'
+			})
+		})
+		expect(
+			getBackendDraft('variable', 'u/admin/fresh_config', { workspace: WORKSPACE })
+		).toBeUndefined()
+	})
+
 	it('requires trigger_kind when discarding a trigger draft', async () => {
 		await expect(
 			callGlobalTool('discard_local_draft', {
@@ -3086,9 +3269,7 @@ describe('folder tools', () => {
 	})

 	it('create_folder surfaces a backend error (e.g. name conflict)', async () => {
-		vi.mocked(FolderService.createFolder).mockRejectedValueOnce(
-			new Error('Folder already exists')
-		)
+		vi.mocked(FolderService.createFolder).mockRejectedValueOnce(new Error('Folder already exists'))
 		const raw = await callGlobalTool('create_folder', { name: 'taken' })
 		const parsed = JSON.parse(raw)
 		expect(parsed.success).toBe(false)
@@ -1,7 +1,5 @@
 import type { Flow, NewSchedule, NewScript } from '$lib/gen/types.gen'
 import { DraftService } from '$lib/gen'
-import { get } from 'svelte/store'
-import { userStore } from '$lib/stores'
 import { UserDraftDbSyncer } from '$lib/userDraftDbSyncer.svelte'
 import { DEFAULT_DATA as DEFAULT_RAW_APP_DATA } from '$lib/components/raw_apps/dataTableRefUtils'
 import { UserDraft, type UserDraftEntry, type UserDraftItemKind } from '$lib/userDraft.svelte'
@@ -336,29 +334,26 @@ function getGlobalDraftSlot(
 }

 // Current user's persisted draft value (+ records the sync baseline so a later
-// save detects external conflicts). undefined on 404 (no draft at that path).
+// save detects external conflicts). undefined when no draft exists at that path.
+// Uses `getOwnDraft` (not `getDraftForUser`): the latter rejects drawer kinds
+// (schedule/trigger/resource/variable drafts are private to their owner), which
+// would make those drafts write-only here — listed but never readable/deployable.
+// Errors (403/500/network) MUST propagate: swallowing one would make the write
+// merge fall through to the deployed item instead of the user's in-progress
+// draft, silently overwriting their draft-only changes.
 async function fetchBackendDraftValue(
 	workspace: string,
 	itemKind: UserDraftItemKind,
 	storagePath: string
 ): Promise<unknown | undefined> {
-	try {
-		const resp = await DraftService.getDraftForUser({
-			workspace,
-			kind: itemKind as any,
-			path: storagePath,
-			username: get(userStore)?.username
-		})
-		UserDraftDbSyncer.recordRemoteSync({ workspace, itemKind, path: storagePath }, resp.created_at)
-		return resp.value ?? undefined
-	} catch (e) {
-		// 404 = no draft for this owner at that path (the intended empty case).
-		// Anything else (403/500/network) MUST propagate: swallowing it would make
-		// the write merge fall through to the deployed item instead of the user's
-		// in-progress draft, silently overwriting their draft-only changes.
-		if ((e as { status?: number } | null | undefined)?.status === 404) return undefined
-		throw e
-	}
+	const resp = await DraftService.getOwnDraft({
+		workspace,
+		kind: itemKind,
+		path: storagePath
+	})
+	if (!resp) return undefined
+	UserDraftDbSyncer.recordRemoteSync({ workspace, itemKind, path: storagePath }, resp.created_at)
+	return resp.value ?? undefined
 }

 // Draft VALUE for a write merge: cell-if-present (the user's freshest in-tab