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 })