fix(drafts): propagate failed headless draft writes instead of swallowing

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

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<WorkspaceItem> {
-	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
 	)

frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts

@@ -367,10 +367,62 @@ export async function saveGlobalDraftValue<V>(
 		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<V>(
+	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<V>(
+	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<void> {
 			path: draft.path,
 			value: null,
 			immediate: true,
-			force: true
+			force: true,
+			throwOnError: true
 		})
 	}
 	clearEphemeralSecretVariableDraftValues(workspace)

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<string, UserDraftDbSyncerSaveOpts>()
  */
 const conflicts = new SvelteMap<string, DraftConflictInfo>()
 
-async function postSave(opts: UserDraftDbSyncerSaveOpts): Promise<void> {
+/**
+ * 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<void> {
 	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<void> {
+	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, () => {