From c0c29dacdeda62c3d41c05b144093181a7f7ebc1 Mon Sep 17 00:00:00 2001 From: centdix Date: Mon, 8 Jun 2026 14:56:47 +0200 Subject: [PATCH] fix(drafts): propagate failed headless draft writes instead of swallowing --- .../chat/global/MIGRATION_DB_DRAFTS.md | 34 +++++ .../components/copilot/chat/global/core.ts | 94 +++++++------- .../copilot/chat/global/userDraftAdapter.ts | 85 +++++++++--- frontend/src/lib/userDraftDbSyncer.svelte.ts | 122 ++++++++++++------ 4 files changed, 229 insertions(+), 106 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/global/MIGRATION_DB_DRAFTS.md b/frontend/src/lib/components/copilot/chat/global/MIGRATION_DB_DRAFTS.md index 825d2859ee..45da2d7c25 100644 --- a/frontend/src/lib/components/copilot/chat/global/MIGRATION_DB_DRAFTS.md +++ b/frontend/src/lib/components/copilot/chat/global/MIGRATION_DB_DRAFTS.md @@ -266,6 +266,40 @@ distinct operation (drop a draft, keep the deployed item) — not redundant with `value:null` → `get_draft` 404. Confirms the exact request/response shapes the seam uses against the endpoints the unit tests mock. +### Hardening (post-review) + +The original seam did **save → read-back**: `saveGlobalDraftValue` pushed to +`UserDraftDbSyncer.save`, then the writer re-read via `getRequiredGlobalDraft` +to shape its return value. Two problems with that: + +1. **A failed write was reported as success.** `UserDraftDbSyncer.postSave` + swallows every error (`catch → console.error`) — correct for fire-and-forget + autosave, wrong for an awaited write whose result is reported to the model. + On a new draft a failed save surfaced as a misleading *"Could not read written + draft"*; on an **overwrite** the read-back returned the **stale** server copy + and the tool reported success with old content (silent write loss). +2. **Every write cost a POST + a GET** to reconstruct a value already in hand. + +Both are closed: + +- **Errors propagate.** `postSave` is split into a throwing `performSave` core + and the swallow-and-log `postSave` wrapper. A new `throwOnError` opt (honored + **only** on the awaited `immediate` path) routes to `performSave`, so a failed + POST rejects the `save()` promise. The headless `saveGlobalDraftValue`, + `deleteGlobalDraft`, and `clearGlobalDrafts` pass `throwOnError: true`; the + debounced autosave path and `overwrite(...)` keep the swallow (default + `false`) so they can't reject unhandled. +- **No read-back.** New `shapeGlobalDraftItem(workspace, type, path, value, + triggerKind?)` shapes the `WorkspaceItem` from the value just saved (same + `liveDisplayPath`/`isLiveDraft` logic as `getGlobalDraftSlot`, factored into a + shared `buildGlobalDraftItem`). `getRequiredGlobalDraft` is replaced by the + sync `requireWrittenDraftItem`; the six write handlers hoist their built draft + value and shape locally. `saveGlobalAppDraft` shapes locally too. Secret + variables are unaffected — the shaped value carries `value: ''` (the real + secret stays in the ephemeral map), same as the read-back produced. + +Unit suite stays green (`core.test.ts` 58/58). + ### Remaining - **E2E with the live LLM-driven chat** (§8 step 5) is still unrun — it needs a diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index d3d92c9d65..1ebcf795c6 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -109,6 +109,7 @@ import { saveGlobalAppDraft, saveGlobalDraftValue, setEphemeralSecretVariableDraftValue, + shapeGlobalDraftItem, triggerKindToUserDraftKind } from './userDraftAdapter' @@ -2173,17 +2174,26 @@ function startDraftWrite(ctx: WriteDraftCtx, type: WorkspaceItemType, path: stri }) } -async function getRequiredGlobalDraft( +/** + * Build the `WorkspaceItem` to return from a write tool from the value we + * just saved, rather than reading it back from the server. The preceding + * `saveGlobalDraftValue` rejects on a failed write, so by the time we get + * here the value is durably persisted (or the headless live cell holds + * it) — a read-back would only add a round trip and could return a stale + * copy if another writer raced in between. + */ +function requireWrittenDraftItem( workspace: string, type: WorkspaceItemType, path: string, + value: unknown, triggerKind?: TriggerKind -): Promise { - const draft = await getGlobalDraft(workspace, type, path, triggerKind) - if (!draft) { - throw new Error(`Could not read written draft ${type} "${path}".`) +): WorkspaceItem { + const item = shapeGlobalDraftItem(workspace, type, path, value, triggerKind) + if (!item) { + throw new Error(`Could not shape written draft ${type} "${path}".`) } - return draft + return item } function finishDraftWrite(stored: WorkspaceItem, existed: boolean, ctx: WriteDraftCtx): string { @@ -2221,8 +2231,9 @@ async function writeScriptDraft( ? false : await ScriptService.existsScriptByPath({ workspace, path: args.path }) + let draft: NewScript if (existingDraft) { - const draft: NewScript = { + draft = { ...structuredClone(existingDraft), path: args.path, summary: args.summary ?? existingDraft.summary, @@ -2236,7 +2247,7 @@ async function writeScriptDraft( path: args.path }) const base = existing as unknown as NewScript - const draft: NewScript = { + draft = { ...structuredClone(base), parent_hash: existing.hash, path: args.path, @@ -2248,7 +2259,7 @@ async function writeScriptDraft( remoteRev: existing.hash }) } else { - const draft: NewScript = { + draft = { path: args.path, summary: args.summary ?? '', description: '', @@ -2262,7 +2273,7 @@ async function writeScriptDraft( } return finishDraftWrite( - await getRequiredGlobalDraft(workspace, 'script', args.path), + requireWrittenDraftItem(workspace, 'script', args.path, draft), existingDraft !== undefined || backendExists, ctx ) @@ -2287,8 +2298,9 @@ async function writeFlowDraft( ? false : await FlowService.existsFlowByPath({ workspace, path: args.path }) + let draft: Flow if (existingDraft) { - const draft: Flow = { + draft = { ...structuredClone(existingDraft), path: args.path, summary: args.summary ?? existingDraft.summary, @@ -2301,7 +2313,7 @@ async function writeFlowDraft( FlowService.getFlowByPath({ workspace, path: args.path }), FlowService.getFlowLatestVersion({ workspace, path: args.path }) ]) - const draft: Flow = { + draft = { ...structuredClone(existing), path: args.path, summary: args.summary ?? existing.summary, @@ -2312,7 +2324,7 @@ async function writeFlowDraft( remoteRev: latestVersion.id }) } else { - const draft: Flow = { + draft = { path: args.path, summary: args.summary ?? '', value, @@ -2326,7 +2338,7 @@ async function writeFlowDraft( } return finishDraftWrite( - await getRequiredGlobalDraft(workspace, 'flow', args.path), + requireWrittenDraftItem(workspace, 'flow', args.path, draft), existingDraft !== undefined || backendExists, ctx ) @@ -2358,7 +2370,7 @@ async function writeScheduleDraft(args: NewSchedule, ctx: WriteDraftCtx): Promis await saveGlobalDraftValue(workspace, 'trigger_schedule', args.path, draft) return finishDraftWrite( - await getRequiredGlobalDraft(workspace, 'schedule', args.path), + requireWrittenDraftItem(workspace, 'schedule', args.path, draft), existingDraft !== undefined || backendExists, ctx ) @@ -2389,7 +2401,7 @@ async function writeTriggerDraft( await saveGlobalDraftValue(workspace, itemKind, path, draft) return finishDraftWrite( - await getRequiredGlobalDraft(workspace, 'trigger', path, args.kind), + requireWrittenDraftItem(workspace, 'trigger', path, draft, args.kind), existingDraft !== undefined || backendExists, ctx ) @@ -2408,28 +2420,23 @@ async function writeResourceDraft(args: CreateResource, ctx: WriteDraftCtx): Pro ? false : await ResourceService.existsResource({ workspace, path: args.path }) + let draftState: ResourceDraftState if (existingDraft) { - await saveGlobalDraftValue( - workspace, - 'resource', - args.path, - createResourceToDraftState(args, existingDraft) - ) + draftState = createResourceToDraftState(args, existingDraft) + await saveGlobalDraftValue(workspace, 'resource', args.path, draftState) } else if (backendExists) { const existing = await ResourceService.getResource({ workspace, path: args.path }) - await saveGlobalDraftValue( - workspace, - 'resource', - args.path, - createResourceToDraftState(args, resourceToDraftState(existing)), - { remoteRev: existing.edited_at } - ) + draftState = createResourceToDraftState(args, resourceToDraftState(existing)) + await saveGlobalDraftValue(workspace, 'resource', args.path, draftState, { + remoteRev: existing.edited_at + }) } else { - await saveGlobalDraftValue(workspace, 'resource', args.path, createResourceToDraftState(args)) + draftState = createResourceToDraftState(args) + await saveGlobalDraftValue(workspace, 'resource', args.path, draftState) } return finishDraftWrite( - await getRequiredGlobalDraft(workspace, 'resource', args.path), + requireWrittenDraftItem(workspace, 'resource', args.path, draftState), existingDraft !== undefined || backendExists, ctx ) @@ -2448,33 +2455,28 @@ async function writeVariableDraft(args: CreateVariable, ctx: WriteDraftCtx): Pro ? false : await VariableService.existsVariable({ workspace, path: args.path }) + let draftState: VariableDraftState if (existingDraft) { - await saveGlobalDraftValue( - workspace, - 'variable', - args.path, - createVariableToDraftState(args, existingDraft) - ) + draftState = createVariableToDraftState(args, existingDraft) + await saveGlobalDraftValue(workspace, 'variable', args.path, draftState) } else if (backendExists) { const existing = await VariableService.getVariable({ workspace, path: args.path, decryptSecret: false }) - await saveGlobalDraftValue( - workspace, - 'variable', - args.path, - createVariableToDraftState(args, variableToDraftState(existing)), - { remoteRev: existing.edited_at } - ) + draftState = createVariableToDraftState(args, variableToDraftState(existing)) + await saveGlobalDraftValue(workspace, 'variable', args.path, draftState, { + remoteRev: existing.edited_at + }) } else { - await saveGlobalDraftValue(workspace, 'variable', args.path, createVariableToDraftState(args)) + draftState = createVariableToDraftState(args) + await saveGlobalDraftValue(workspace, 'variable', args.path, draftState) } syncEphemeralSecretVariableDraftValue(workspace, args) return finishDraftWrite( - await getRequiredGlobalDraft(workspace, 'variable', args.path), + requireWrittenDraftItem(workspace, 'variable', args.path, draftState), existingDraft !== undefined || backendExists, ctx ) diff --git a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts index a1a941ac88..1863df254a 100644 --- a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts +++ b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts @@ -367,10 +367,62 @@ export async function saveGlobalDraftValue( path: storagePath, value, immediate: true, - force: true + force: true, + // Headless write: a failed POST must reject so the calling tool + // reports the failure, instead of being silently swallowed and + // then read back as a stale value (or 404) and mis-reported as a + // successful write. + throwOnError: true }) } +/** + * Shape a draft `value` into a `WorkspaceItem` at its live-editor display + * path. Shared by the DB read path (`getGlobalDraftSlot`) and the + * write-then-shape path (`shapeGlobalDraftItem`) so both yield an + * identical item for the same value. + */ +function buildGlobalDraftItem( + workspace: string, + itemKind: UserDraftItemKind, + storagePath: string, + value: V +): { displayPath: string; item: WorkspaceItem } | undefined { + const { displayPath, isLiveDraft } = liveDisplayPath(workspace, itemKind, storagePath) + const entry: UserDraftEntry = { + workspace, + itemKind, + path: storagePath, + value, + meta: {} + } + const item = userDraftEntryToWorkspaceItem(entry, displayPath, isLiveDraft) + return item ? { displayPath, item } : undefined +} + +/** + * Shape a freshly-written draft `value` into a `WorkspaceItem` WITHOUT a + * server read-back. The value is exactly what `saveGlobalDraftValue` just + * persisted (and that save now rejects on failure), so reading it back + * would be a redundant round trip — and, before the syncer learned to + * reject failed writes, the read-back could mask a failed save by + * returning the stale server copy. Mirrors `getGlobalDraftSlot`'s shaping + * (same live-editor display-path / `isLiveDraft` resolution), sourced + * from the in-hand value. + */ +export function shapeGlobalDraftItem( + workspace: string, + type: WorkspaceItemType, + path: string, + value: V, + triggerKind?: TriggerKind +): WorkspaceItem | undefined { + const itemKind = itemKindFor(type, triggerKind) + if (!itemKind) return undefined + const storagePath = resolveDraftStoragePath(workspace, itemKind, path) + return buildGlobalDraftItem(workspace, itemKind, storagePath, value)?.item +} + async function getGlobalDraftSlot( workspace: string, type: WorkspaceItemType, @@ -383,19 +435,9 @@ async function getGlobalDraftSlot( const draft = await readGlobalDraftValue(workspace, itemKind, storagePath) if (draft === undefined) return undefined - const { displayPath, isLiveDraft } = liveDisplayPath(workspace, itemKind, storagePath) - const entry = { - workspace, - itemKind, - path: storagePath, - value: draft, - meta: {}, - persisted: false, - live: false - } - const item = userDraftEntryToWorkspaceItem(entry, displayPath, isLiveDraft) - if (!item) return undefined - return { itemKind, storagePath, displayPath, item } + const shaped = buildGlobalDraftItem(workspace, itemKind, storagePath, draft) + if (!shaped) return undefined + return { itemKind, storagePath, displayPath: shaped.displayPath, item: shaped.item } } export async function getGlobalDraft( @@ -467,8 +509,11 @@ export async function saveGlobalAppDraft( const storagePath = resolveDraftStoragePath(workspace, 'raw_app', path) const normalized = normalizeAppDraftValue(value) await saveGlobalDraftValue(workspace, 'raw_app', storagePath, normalized, meta) - const stored = await getGlobalDraft(workspace, 'app', path) - if (!stored) throw new Error(`Could not read written app draft "${path}".`) + // Shape from the value we just persisted instead of reading it back — + // the save above rejects on failure, so a read-back would only add a + // round trip (and risk returning a stale copy). + const stored = shapeGlobalDraftItem(workspace, 'app', path, normalized) + if (!stored) throw new Error(`Could not shape written app draft "${path}".`) return stored } @@ -510,7 +555,10 @@ export async function deleteGlobalDraft( path: storagePath, value: null, immediate: true, - force: true + force: true, + // A failed delete must reject so callers (discard / deploy / + // delete tools) don't report "discarded" while the row survives. + throwOnError: true }) if (type === 'variable') clearEphemeralSecretVariableDraftValue(workspace, storagePath) } @@ -529,7 +577,8 @@ export async function clearGlobalDrafts(workspace: string): Promise { path: draft.path, value: null, immediate: true, - force: true + force: true, + throwOnError: true }) } clearEphemeralSecretVariableDraftValues(workspace) diff --git a/frontend/src/lib/userDraftDbSyncer.svelte.ts b/frontend/src/lib/userDraftDbSyncer.svelte.ts index 30a47d4215..b7ec1e724c 100644 --- a/frontend/src/lib/userDraftDbSyncer.svelte.ts +++ b/frontend/src/lib/userDraftDbSyncer.svelte.ts @@ -74,6 +74,17 @@ export type UserDraftDbSyncerSaveOpts = { * the conflict locally. Default `false`: regular autosaves attach * `last_sync` and respect the server's reject response. */ force?: boolean + /** Propagate a failed POST (network / HTTP error, or a rejected + * optimistic-concurrency check) by rejecting the returned promise + * instead of swallowing it. ONLY honored on the `immediate` path — + * the debounced autosave path is fire-and-forget and must never + * reject into an unhandled rejection. Use when the caller awaits the + * save and reports its success/failure to a user or model (the + * headless AI-chat draft writes): a swallowed failure would otherwise + * be read back as a stale value and reported as a successful write. + * Default `false` (preserves the swallow-and-log behaviour for + * `overwrite(...)` and other immediate callers). */ + throwOnError?: boolean } /** @@ -155,51 +166,72 @@ const pendingSaveOpts = new Map() */ const conflicts = new SvelteMap() -async function postSave(opts: UserDraftDbSyncerSaveOpts): Promise { +/** + * The network-and-state half of a save. THROWS on a failed POST (network + * / HTTP error) or — when `opts.throwOnError` is set — on a rejected + * optimistic-concurrency check. `postSave` wraps this with the + * swallow-and-log behaviour the debounced autosave path relies on; the + * `immediate` + `throwOnError` path calls it directly so the rejection + * reaches the awaiting caller. + */ +async function performSave(opts: UserDraftDbSyncerSaveOpts): Promise { const key = draftKey(opts.workspace, opts.itemKind, opts.path) const lastSync = getLastSyncEntry(key)?.lastSync - try { - const resp = await DraftService.saveDraft({ - workspace: opts.workspace, - kind: opts.itemKind, - path: opts.path, - requestBody: { - value: opts.value as any, - // Force-saves skip the conflict check (callers that - // explicitly opted in via `overwrite(...)` already showed - // the user the conflict). Regular autosaves attach - // `last_sync` (when we have one) so the server can reject - // stale writes — defining behaviour for a first-ever save - // is "no last_sync field at all", matching the backend's - // "treat as fresh" branch. - last_sync: opts.force ? undefined : lastSync, - force: opts.force ?? false - } + const resp = await DraftService.saveDraft({ + workspace: opts.workspace, + kind: opts.itemKind, + path: opts.path, + requestBody: { + value: opts.value as any, + // Force-saves skip the conflict check (callers that + // explicitly opted in via `overwrite(...)` already showed + // the user the conflict). Regular autosaves attach + // `last_sync` (when we have one) so the server can reject + // stale writes — defining behaviour for a first-ever save + // is "no last_sync field at all", matching the backend's + // "treat as fresh" branch. + last_sync: opts.force ? undefined : lastSync, + force: opts.force ?? false + } + }) + if (resp.status === 'conflict') { + // Server rejected the write because someone (another tab / + // browser / user) advanced the row past our `last_sync`. Park + // the snapshot in the conflict map for the UI to pick up; do + // NOT touch the local `lastSync` (the next save retries from + // the same baseline so the conflict persists until resolved). + conflicts.set(key, { + serverTimestamp: resp.current_timestamp, + localLastSync: lastSync ?? null }) - if (resp.status === 'conflict') { - // Server rejected the write because someone (another tab / - // browser / user) advanced the row past our `last_sync`. Park - // the snapshot in the conflict map for the UI to pick up; do - // NOT touch the local `lastSync` (the next save retries from - // the same baseline so the conflict persists until resolved). - conflicts.set(key, { - serverTimestamp: resp.current_timestamp, - localLastSync: lastSync ?? null - }) - return + // A caller awaiting this save needs to know the write did NOT + // land — surface it rather than returning as if it had. (Forced + // saves never hit this branch, so the headless AI-chat writes, + // which always force, won't trip it.) + if (opts.throwOnError) { + throw new Error( + `UserDraftDbSyncer: save for ${key} was rejected by the optimistic-concurrency check` + ) } - // resp.status === 'saved' — clear any prior conflict and bring - // the local lastSync forward (or drop it entirely on a delete). - if (opts.value === null) { - clearLastSync(opts.workspace, opts.itemKind, opts.path) - } else { - setLastSync(opts.workspace, opts.itemKind, opts.path, resp.current_timestamp) - } - conflicts.delete(key) - // Clear pending only if it's still the opts we just saved — a - // newer `save()` that arrived during the POST replaces the entry - // and must survive for the next flush / debouncer round. - if (pendingSaveOpts.get(key) === opts) pendingSaveOpts.delete(key) + return + } + // resp.status === 'saved' — clear any prior conflict and bring + // the local lastSync forward (or drop it entirely on a delete). + if (opts.value === null) { + clearLastSync(opts.workspace, opts.itemKind, opts.path) + } else { + setLastSync(opts.workspace, opts.itemKind, opts.path, resp.current_timestamp) + } + conflicts.delete(key) + // Clear pending only if it's still the opts we just saved — a + // newer `save()` that arrived during the POST replaces the entry + // and must survive for the next flush / debouncer round. + if (pendingSaveOpts.get(key) === opts) pendingSaveOpts.delete(key) +} + +async function postSave(opts: UserDraftDbSyncerSaveOpts): Promise { + try { + await performSave(opts) } catch (e) { console.error('UserDraftDbSyncer.save failed', e) } @@ -337,7 +369,13 @@ export const UserDraftDbSyncer = { // observable in the runner's internal state for debugging. debouncer.cancel(key) runner.cancel(key) - await runner.submitAndWait(key, () => postSave(opts)) + // `throwOnError` callers await the result and report it — run + // the throwing `performSave` so `submitAndWait` rejects on a + // failed POST. Everyone else keeps the swallow-and-log + // `postSave` so an immediate autosave can't reject unhandled. + await runner.submitAndWait(key, () => + opts.throwOnError ? performSave(opts) : postSave(opts) + ) return } debouncer.schedule(key, () => {