From a0a3b9583d5e374295cb87262fdfe0934115b3a4 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Wed, 23 Sep 2026 14:46:09 +0200 Subject: [PATCH] fix: resolve a draft's key once per operation and pass it down Review round 7: - Deleting a deployed item removes the reason its path outranks a draft staged under that name, so a cleanup that resolved afterwards deleted that unrelated draft. The key is resolved while the deployed item is still there, and both cleanups now pass it rather than let the delete resolve again. - rebase_draft probed the deployed item at the name it was addressed by, so a renamed draft rebased against a path where nothing is deployed and the tool reported no deployed version. All three kinds read the storage path. - A write resolved its merge base and its destination separately, and only the destination failed closed, so a listing that failed in between based the merge on the deployed item and wrote that over the draft. Writes resolve once and thread that key through base, merge and persist. `readGlobalDraftValue` has no callers left and is replaced by `readGlobalDraftValueAt`, which takes the key its caller resolved. Co-Authored-By: Claude Opus 5 (1M context) --- .../copilot/chat/global/core.test.ts | 37 +++++++++++- .../components/copilot/chat/global/core.ts | 54 ++++++++++++------ .../copilot/chat/global/userDraftAdapter.ts | 56 ++++++++++++++----- 3 files changed, 112 insertions(+), 35 deletions(-) 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 aafede147f..7d1d493262 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -395,7 +395,7 @@ import { deleteGlobalDraft, listGlobalDrafts, persistGlobalDraft, - readGlobalDraftValue, + readGlobalDraftValueAt, saveGlobalAppDraft } from './userDraftAdapter' import { bundleRawAppDraft } from './rawAppBundlerBridge' @@ -2590,6 +2590,37 @@ describe('global AI tools', () => { } }) + // Deleting the deployed item removes the reason its path outranks a draft staged under + // that name, so a cleanup resolved afterwards would delete that unrelated draft. + it('leaves alone a draft staged under a deleted item\'s path', async () => { + seedBackendDraft( + 'raw_app', + 'u/admin/draft_namesake', + { summary: 'Namesake', draft_path: 'f/sales/deleted_app', files: {}, runnables: {} }, + { workspace: WORKSPACE } + ) + const deletedApps = new Set() + vi.mocked(AppService.existsApp).mockImplementation( + async ({ path }) => path === 'f/sales/deleted_app' && !deletedApps.has(path) + ) + const deleteApp = vi + .spyOn(AppService, 'deleteApp') + .mockImplementation(async ({ path }: any) => { + deletedApps.add(path) + return 'deleted' as any + }) + + try { + await callGlobalTool('delete_workspace_item', { type: 'app', path: 'f/sales/deleted_app' }) + expect( + getBackendDraft('raw_app', 'u/admin/draft_namesake', { workspace: WORKSPACE }) + ).toBeDefined() + } finally { + vi.mocked(AppService.existsApp).mockImplementation(async () => false) + deleteApp.mockRestore() + } + }) + it('refuses a draft_path that two drafts are staged under', async () => { for (const storage of ['u/admin/draft_a', 'u/admin/draft_b']) { seedBackendDraft( @@ -2897,7 +2928,7 @@ describe('global AI tools', () => { // directly: a non-force save whose recorded baseline is older than the // server row is rejected with `status:'conflict'`, and `override` (force) // pushes our version through. NB: this targets persistGlobalDraft, not the - // write_* tools — those re-read the backend first (readGlobalDraftValue -> + // write_* tools — those re-read the backend first (readGlobalDraftValueAt -> // recordRemoteSync), which re-seeds the baseline and so can only surface a // conflict when a live editor cell is mounted (not the case in unit tests). it('persistGlobalDraft surfaces a conflict on a stale baseline and override forces it', async () => { @@ -2962,7 +2993,7 @@ describe('global AI tools', () => { it('a non-404 backend read failure propagates instead of returning undefined', async () => { const path = 'f/scripts/readfail' failingReads.add(`script:${path}`) - await expect(readGlobalDraftValue(WORKSPACE, 'script', path)).rejects.toThrow() + await expect(readGlobalDraftValueAt(WORKSPACE, 'script', path)).rejects.toThrow() }) // Raw-app writes go through saveGlobalAppDraft, which must carry the conflict diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index 3265351e4f..89cef6cdb1 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -228,9 +228,10 @@ import { chosenDraftName, listGlobalDrafts, liveGlobalDraftStoragePath, + readGlobalDraftValueAt, + resolveGlobalDraft, resolveGlobalDraftStoragePath, persistGlobalDraft, - readGlobalDraftValue, readLocalDraftCellByKind, resolveGlobalDraftStoragePathByKind, saveGlobalAppDraft, @@ -5130,7 +5131,16 @@ async function writeDraft( const { workspace } = ctx startDraftWrite(ctx, type, path) - const existingDraft = await readGlobalDraftValue(workspace, type, path, opts.triggerKind) + // One resolution for the merge base and the write: resolving twice lets a listing that + // fails in between base the merge on the deployed item and write that over the draft. + const resolved = await resolveGlobalDraft(workspace, type, path, opts.triggerKind, { + forWrite: true + }) + const existingDraft = ( + resolved.fetched + ? resolved.value + : await readGlobalDraftValueAt(workspace, type, resolved.storagePath, opts.triggerKind) + ) as T | undefined let base = existingDraft let existed = existingDraft !== undefined if (base === undefined && (await spec.probe(workspace, path))) { @@ -5142,7 +5152,8 @@ async function writeDraft( const result = await persistGlobalDraft(workspace, type, path, draft, { triggerKind: opts.triggerKind, - force: opts.override + force: opts.override, + storagePath: resolved.storagePath }) return finishDraftWrite(result, existed, ctx) } @@ -6911,11 +6922,14 @@ async function rebaseScriptDraft(path: string, ctx: WriteDraftCtx): Promise if (!draft || !draft.value || typeof draft.value === 'string' || !('files' in draft.value)) { throw new Error(`No app draft found for "${path}".`) } - if (!(await AppService.existsApp({ workspace, path }))) { + const basePath = await resolveGlobalDraftStoragePath(workspace, 'app', path) + if (!(await AppService.existsApp({ workspace, path: basePath }))) { throw new Error(`App "${path}" is not deployed; there is no newer version to rebase onto.`) } - const deployed = await AppService.getAppByPath({ workspace, path }) + const deployed = await AppService.getAppByPath({ workspace, path: basePath }) const headVersion = deployed.versions?.[deployed.versions.length - 1] const baseVersion = draft.parentVersionId if (baseVersion != null && baseVersion === headVersion) { @@ -8014,7 +8030,8 @@ async function deployDraft( // back, so read it here — post-flush, since a rename may have been parked. This is // where a draft-only or renamed item lands, and what the callbacks below must name. deployedPath = - chosenDraftName(type, await readGlobalDraftValue(workspace, type, storagePath)) ?? storagePath + chosenDraftName(type, await readGlobalDraftValueAt(workspace, type, storagePath)) ?? + storagePath const result = await deployDraftToWorkspace(type, storagePath, workspace, { draftOnly, deploymentMessage @@ -8276,8 +8293,9 @@ async function deployDraft( // By the path resolved before the deploy: the app now deployed at the draft's chosen // name would otherwise win the name back, and the draft would be left behind. - await deleteGlobalDraft(workspace, type, draftStoragePath, triggerKind, { - preserveLiveDraft: true + await deleteGlobalDraft(workspace, type, path, triggerKind, { + preserveLiveDraft: true, + storagePath: draftStoragePath }) // Move the chat's mask entry to the deployed path: a draft-only item's @@ -8427,6 +8445,11 @@ async function deleteWorkspaceItem( throw new Error('trigger_kind is required when deleting a trigger.') } + // Resolved while the deployed item is still there: once it is gone, this path could + // resolve to another item's draft staged under its name, and the cleanup below would + // delete that instead. + const draftStoragePath = await resolveGlobalDraftStoragePath(workspace, type, path, triggerKind) + toolCallbacks.setToolStatus(toolId, { content: `Deleting ${type} "${path}"...` }) @@ -8459,17 +8482,14 @@ async function deleteWorkspaceItem( // are no longer trustworthy (same rule as deploy success). Before the // draft cleanup: a cleanup failure must not leave stale comparisons. invalidateWorkspaceComparison(workspace) - await deleteGlobalDraft(workspace, type, path, triggerKind) + await deleteGlobalDraft(workspace, type, path, triggerKind, { storagePath: draftStoragePath }) // Record the deletion in the chat's modified-items mask. In a fork this leaves a // reviewable "removed" diff vs the parent that stays scoped to this chat. Keyed // by the same (itemKind, storagePath) as writes so it joins the draft/fork lists. const deletedKind = itemKindFor(type, triggerKind) if (deletedKind) { - toolCallbacks.onItemModified?.( - deletedKind, - liveGlobalDraftStoragePath(workspace, type, path, triggerKind) - ) + toolCallbacks.onItemModified?.(deletedKind, draftStoragePath) } toolCallbacks.setToolStatus(toolId, { diff --git a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts index 992c4965a2..ca2e15e5ee 100644 --- a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts +++ b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts @@ -428,6 +428,29 @@ export function liveGlobalDraftStoragePath( return itemKind ? liveEditorStoragePath(workspace, itemKind, path) : path } +/** One resolution for a whole operation, so a tool reads its merge base, writes and + * cleans up at the same key. Resolving twice lets a listing that fails in between send + * the two halves to different drafts. `value` is the draft found while resolving, if any; + * `fetched` says the backend was already asked, so no caller asks again. */ +export async function resolveGlobalDraft( + workspace: string, + type: WorkspaceItemType, + path: string, + triggerKind?: TriggerKind, + opts: { forWrite?: boolean } = {} +): Promise<{ storagePath: string; value?: unknown; fetched: boolean }> { + const itemKind = itemKindFor(type, triggerKind) + if (!itemKind) return { storagePath: path, fetched: false } + const resolved = await resolveDraft(workspace, itemKind, path, opts) + const cell = UserDraft.get(itemKind, resolved.storagePath, { workspace }) + return { + storagePath: resolved.storagePath, + value: cell ?? resolved.value, + // Only the path it probed was fetched; a name resolved elsewhere was not. + fetched: cell !== undefined || resolved.storagePath === path + } +} + /** Where the draft a model-supplied path names is stored. */ export async function resolveGlobalDraftStoragePath( workspace: string, @@ -490,23 +513,19 @@ async function fetchBackendDraftValue( return resp.value ?? undefined } -// Draft VALUE for a write merge: cell-if-present (the user's freshest in-tab -// edits) else the current user's backend draft. -export async function readGlobalDraftValue( +/** Draft VALUE at a key already resolved: cell-if-present (the user's freshest in-tab + * edits) else the current user's backend draft. */ +export async function readGlobalDraftValueAt( workspace: string, type: WorkspaceItemType, - path: string, + storagePath: string, triggerKind?: TriggerKind ): Promise { const itemKind = itemKindFor(type, triggerKind) if (!itemKind) return undefined - const resolved = await resolveDraft(workspace, itemKind, path) - // The cell at the RESOLVED path: it is the freshest state when a save is parked, failed - // or conflicted, and a draft that exists only as a cell has no backend row behind it. - const cell = UserDraft.get(itemKind, resolved.storagePath, { workspace }) + const cell = UserDraft.get(itemKind, storagePath, { workspace }) if (cell !== undefined) return cell - if (resolved.value !== undefined) return resolved.value as V - return (await fetchBackendDraftValue(workspace, itemKind, resolved.storagePath)) as V | undefined + return (await fetchBackendDraftValue(workspace, itemKind, storagePath)) as V | undefined } // `itemKind` + `storagePath` are the canonical identity of the persisted draft @@ -538,14 +557,16 @@ export async function persistGlobalDraft( type: WorkspaceItemType, path: string, value: unknown, - opts: { triggerKind?: TriggerKind; force?: boolean } = {} + opts: { triggerKind?: TriggerKind; force?: boolean; storagePath?: string } = {} ): Promise { const itemKind = itemKindFor(type, opts.triggerKind) if (!itemKind) throw new Error(`Unsupported draft type "${type}".`) // Resolved rather than taken as given: a write under a chosen name that did not resolve - // would create a second draft beside the one it meant to edit. - const storagePath = (await resolveDraft(workspace, itemKind, path, { forWrite: true })) - .storagePath + // would create a second draft beside the one it meant to edit. A caller that resolved + // for the whole operation passes its key so both halves land on one draft. + const storagePath = + opts.storagePath ?? + (await resolveDraft(workspace, itemKind, path, { forWrite: true })).storagePath UserDraft.seed(itemKind, storagePath, value, { workspace }) await UserDraftDbSyncer.save({ workspace, @@ -706,6 +727,10 @@ export async function saveGlobalAppDraft( type DeleteGlobalDraftOptions = { preserveLiveDraft?: boolean + /** Key resolved by the caller, for a cleanup whose own resolution would now answer + * differently — after a deploy created the item at the draft's chosen name, or after + * the deployed item that outranked that name was deleted. */ + storagePath?: string } export async function deleteGlobalDraft( @@ -717,7 +742,8 @@ export async function deleteGlobalDraft( ): Promise { const itemKind = itemKindFor(type, triggerKind) if (!itemKind) return - const storagePath = (await resolveDraft(workspace, itemKind, path)).storagePath + const storagePath = + options.storagePath ?? (await resolveDraft(workspace, itemKind, path)).storagePath const liveDraft = UserDraft.getLiveEditorDraft(itemKind, { workspace }) if (options.preserveLiveDraft && liveDraft?.storagePath === storagePath) { UserDraft.remove(itemKind, storagePath, { workspace })