From 83ef6ce56dc7379bc0269d99ebb24b87947a925c Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Thu, 1 Oct 2026 10:33:42 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20review=20round=20=E2=80=94=20failed=20bu?= =?UTF-8?q?ndle=20save=20fails=20the=20deploy,=20removing=20a=20draft=20re?= =?UTF-8?q?cords=20no=20change?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 (1M context) --- .../assets/AssetGraph/PipelineGraphEditor.svelte | 4 ++-- .../copilot/chat/pipeline/core.test.ts | 16 ++++++++++++++++ .../lib/components/copilot/chat/pipeline/core.ts | 3 ++- .../sessions/PipelineEditorView.svelte | 14 ++++++++++++-- .../lib/components/sessions/modifiedItemsMask.ts | 5 +++-- .../(logged)/pipeline/[folder]/+page.svelte | 4 +++- 6 files changed, 38 insertions(+), 8 deletions(-) diff --git a/frontend/src/lib/components/assets/AssetGraph/PipelineGraphEditor.svelte b/frontend/src/lib/components/assets/AssetGraph/PipelineGraphEditor.svelte index 8072e25ec1..b3e43e75ea 100644 --- a/frontend/src/lib/components/assets/AssetGraph/PipelineGraphEditor.svelte +++ b/frontend/src/lib/components/assets/AssetGraph/PipelineGraphEditor.svelte @@ -451,8 +451,8 @@ fromServer = true } } - // A folder retarget during the await (the route-page header switcher, or a - // session retarget) changed the target and reset hydratedFromDb. Don't + // A folder switch during the await (the route page's header switcher) + // changed the target and reset hydratedFromDb. Don't // apply this now-stale folder's bundle or mark the new folder hydrated — // otherwise the stale result blocks the new folder's hydrate and its // drafts bleed across folders. diff --git a/frontend/src/lib/components/copilot/chat/pipeline/core.test.ts b/frontend/src/lib/components/copilot/chat/pipeline/core.test.ts index dd5579eb45..1c80b4aea8 100644 --- a/frontend/src/lib/components/copilot/chat/pipeline/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/pipeline/core.test.ts @@ -198,6 +198,22 @@ describe('pipeline tools', () => { ).rejects.toThrow(/No pipeline editor is open/) }) + it('remove_pipeline_node records no change: undoing a draft leaves nothing to deploy', async () => { + const { helpers } = makeHelpers() + const modified: string[] = [] + await toolByName('remove_pipeline_node').fn({ + args: { path: 'f/analytics/clean' }, + workspace: 'w', + helpers, + toolCallbacks: { + ...noopCallbacks(), + onItemModified: (kind, path) => modified.push(`${kind}:${path}`) + }, + toolId: 't' + }) + expect(modified).toEqual([]) + }) + it('test_pipeline_node requires confirmation', () => { expect(toolByName('test_pipeline_node').requiresConfirmation).toBe(true) }) diff --git a/frontend/src/lib/components/copilot/chat/pipeline/core.ts b/frontend/src/lib/components/copilot/chat/pipeline/core.ts index efc734373f..8c29fbcbeb 100644 --- a/frontend/src/lib/components/copilot/chat/pipeline/core.ts +++ b/frontend/src/lib/components/copilot/chat/pipeline/core.ts @@ -366,8 +366,9 @@ export const pipelineTools: SessionTool[] = [ const { path } = removePipelineNodeSchema.parse(args) const pipeline = pipelineForPath(helpers, path) toolCallbacks.setToolStatus(toolId, { content: `Discarding draft '${path}'...` }) + // Not recorded: undoing a draft changes nothing to deploy, and the last + // one gone would leave the pipeline listed as this chat's change. await pipeline.removeProposedNode(path) - recordPipelineModified(toolCallbacks, pipeline) toolCallbacks.setToolStatus(toolId, { content: `Discarded draft '${path}'`, result: 'Success' diff --git a/frontend/src/lib/components/sessions/PipelineEditorView.svelte b/frontend/src/lib/components/sessions/PipelineEditorView.svelte index 9b9ed8446b..71ed7db905 100644 --- a/frontend/src/lib/components/sessions/PipelineEditorView.svelte +++ b/frontend/src/lib/components/sessions/PipelineEditorView.svelte @@ -354,12 +354,22 @@ // The session's changes list re-reads the draft once this resolves, so // what is left of it has to be saved by then. await tick() - await UserDraftDbSyncer.flush({ + const draft = { workspace: workspaceId, itemKind: PIPELINE_DRAFT_KIND, path: pipelineBundlePath(path) - }) + } + await UserDraftDbSyncer.flush(draft) deployErrors = errors + // A flush resolves even when the save failed: the server would keep the + // deployed drafts, and deploying them again would recreate their triggers. + const sync = UserDraftDbSyncer.getState(draft) + if (UserDraftDbSyncer.getConflict(draft).conflict || sync.state === 'failed') { + return { + success: false, + error: `Deployed, but the remaining drafts could not be saved: ${sync.failureMessage ?? 'they conflict with a newer version'}.` + } + } if (errors.size > 0) { deployErrorsOpen = true return { diff --git a/frontend/src/lib/components/sessions/modifiedItemsMask.ts b/frontend/src/lib/components/sessions/modifiedItemsMask.ts index f2f7494379..d597c46ec9 100644 --- a/frontend/src/lib/components/sessions/modifiedItemsMask.ts +++ b/frontend/src/lib/components/sessions/modifiedItemsMask.ts @@ -48,8 +48,9 @@ export function forkDiffKindToUserDraftKind(kind: ForkDiffKind): UserDraftItemKi // A pipeline the chat edited is masked as its folder's draft bundle, and each // script that bundle deploys is masked too, so a fork review selects exactly -// those. The session's changes list shows them as the pipeline's one row, not -// as rows of their own. +// those. The session's changes list shows deployed scripts of a masked +// pipeline's folder as that pipeline's one row — any of them, since a script's +// mask entry does not say whether a pipeline deploy or the chat put it there. export function foldedIntoPipelineRow( mask: ReadonlySet, kind: UserDraftItemKind, diff --git a/frontend/src/routes/(root)/(logged)/pipeline/[folder]/+page.svelte b/frontend/src/routes/(root)/(logged)/pipeline/[folder]/+page.svelte index da6ab6b84e..b546f0412f 100644 --- a/frontend/src/routes/(root)/(logged)/pipeline/[folder]/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/pipeline/[folder]/+page.svelte @@ -540,7 +540,9 @@ } const sync = UserDraftDbSyncer.getState(draft) if (sync.state === 'failed') { - throw new Error(`The pipeline drafts could not be saved: ${sync.failureMessage ?? 'unknown error'}`) + throw new Error( + `The pipeline drafts could not be saved: ${sync.failureMessage ?? 'unknown error'}` + ) } // The drafts go with the pipeline into the session, so leaving for it is // not leaving them behind.