From 056ebdb03543a93094c80ca354c117236cd8d6c8 Mon Sep 17 00:00:00 2001 From: Guilhem Date: Mon, 6 Jul 2026 11:34:08 +0200 Subject: [PATCH] fix: read chat drafts via own-draft route so drawer-kind drafts deploy (#9913) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: read chat drafts via own-draft route so drawer-kind drafts deploy Co-Authored-By: Claude Fable 5 * test: cover trigger and resource chat-draft read/deploy regressions Co-Authored-By: Claude Fable 5 * 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 * 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 --------- Co-authored-by: Claude Fable 5 Co-authored-by: Ruben Fiszel --- ai_evals/adapters/frontend/mockBackend.ts | 19 +- .../frontend/mockBackendDrafts.test.ts | 22 ++ .../adapters/frontend/vitestAdapter.test.ts | 5 + .../copilot/chat/global/core.test.ts | 199 +++++++++++++++++- .../copilot/chat/global/userDraftAdapter.ts | 35 ++- 5 files changed, 249 insertions(+), 31 deletions(-) diff --git a/ai_evals/adapters/frontend/mockBackend.ts b/ai_evals/adapters/frontend/mockBackend.ts index 379eb6d447..f9023449a2 100644 --- a/ai_evals/adapters/frontend/mockBackend.ts +++ b/ai_evals/adapters/frontend/mockBackend.ts @@ -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()] diff --git a/ai_evals/adapters/frontend/mockBackendDrafts.test.ts b/ai_evals/adapters/frontend/mockBackendDrafts.test.ts index a720de5e43..0ab79d216e 100644 --- a/ai_evals/adapters/frontend/mockBackendDrafts.test.ts +++ b/ai_evals/adapters/frontend/mockBackendDrafts.test.ts @@ -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' }) diff --git a/ai_evals/adapters/frontend/vitestAdapter.test.ts b/ai_evals/adapters/frontend/vitestAdapter.test.ts index dcaa1d2ca4..92e33414df 100644 --- a/ai_evals/adapters/frontend/vitestAdapter.test.ts +++ b/ai_evals/adapters/frontend/vitestAdapter.test.ts @@ -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) diff --git a/frontend/src/lib/components/copilot/chat/global/core.test.ts b/frontend/src/lib/components/copilot/chat/global/core.test.ts index a3b9c22499..0f30076bda 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -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(), - // 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(), failingReads: new Set() @@ -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) diff --git a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts index 2717d41608..342717f26e 100644 --- a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts +++ b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts @@ -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 { - 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