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 482c9be441..ddc9ea2d21 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -521,6 +521,11 @@ describe('global AI tools', () => { failingReads.clear() clearGlobalDrafts(WORKSPACE) vi.clearAllMocks() + // `clearAllMocks` keeps implementations, so a test that says "this item is deployed" + // would answer for every later test too, making them order-dependent. + vi.mocked(ScriptService.existsScriptByPath).mockImplementation(async () => false) + vi.mocked(FlowService.existsFlowByPath).mockImplementation(async () => false) + vi.mocked(AppService.existsApp).mockImplementation(async () => false) }) it('defaults the datatable instruction subject to the TypeScript SQL SDK', async () => { @@ -3245,7 +3250,9 @@ describe('global AI tools', () => { const draft = getBackendDraft('flow', 'u/admin/draft_f1', { workspace: WORKSPACE }) expect(draft?.draft_path).toBe('f/team/new_flow') - expect(draft?.path).toBe('f/team/new_flow') + // `path` stays the storage key: the flow editor compares a typed rename against it + // and drops `draft_path` when they agree, which would lose the name entirely. + expect(draft?.path).toBe('u/admin/draft_f1') }) // Same private-owner read path as schedules, for the trigger drawer kinds. diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index afe8051a1d..3047fde6e4 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -5066,12 +5066,12 @@ function stripBackendMetadata(value: T): T { function mergeDraftConfig( base: T | undefined, overrides: DraftConfig, - chosenName: string + valuePath: string ): T { return { ...(base ? stripBackendMetadata(base) : {}), ...structuredClone(overrides), - path: chosenName + path: valuePath } as unknown as T } @@ -5366,9 +5366,9 @@ function finishDraftWrite( type WriteSpec = { probe: (workspace: string, path: string) => Promise fetchDeployed: (workspace: string, path: string) => Promise - /** `chosenName` is the name the draft deploys under, which is the only path a value - * may carry — never the key it is stored at. */ - buildDraft: (base: T | undefined, args: A, chosenName: string) => T | Promise + /** `valuePath` is what belongs in the value's own `path` for this kind — a script's + * name, every other kind's storage key. Never the path the call named. */ + buildDraft: (base: T | undefined, args: A, valuePath: string) => T | Promise } async function writeDraft( @@ -5396,15 +5396,15 @@ async function writeDraft( existed = true } - // The name a base already carries wins over the path the call named: a draft is - // addressable by its storage key, and writing that key back into the value would - // rename the item to it — the deployer deploys at the path inside the value. Only a - // create (no base) takes its name from the call. - const itemKind = itemKindFor(type, opts.triggerKind) - const chosenName = - (base !== undefined && itemKind ? chosenDraftName(itemKind, base) : undefined) ?? target.path + // The path the value already carries, whatever it means for this kind: a script's is its + // name, a flow's is the storage key its typed rename is compared against. Either way the + // path the call named is not it — a draft is addressable by its storage key, and writing + // that key over a script's `path` renames the script to it, while writing it over a + // flow's makes the rename look applied and the editor then drops `draft_path`. Only a + // create, with no base to preserve, takes its path from the call. + const valuePath = (base as { path?: string } | undefined)?.path || target.path - const draft = await spec.buildDraft(base, args, chosenName) + const draft = await spec.buildDraft(base, args, valuePath) const result = await persistGlobalDraft(workspace, type, target, draft, { triggerKind: opts.triggerKind, @@ -5428,17 +5428,17 @@ const SCRIPT_SPEC: WriteSpec = { const existing = await ScriptService.getScriptByPath({ workspace, path }) return { ...(existing as unknown as NewScript), parent_hash: existing.hash } }, - buildDraft: async (base, args, chosenName) => { + buildDraft: async (base, args, valuePath) => { const draft: NewScript = base ? { ...structuredClone(base), - path: chosenName, + path: valuePath, summary: args.summary ?? base.summary, content: args.content, language: args.language } : { - path: chosenName, + path: valuePath, summary: args.summary ?? '', description: '', content: args.content, @@ -5498,7 +5498,7 @@ const FLOW_STRUCTURAL_VALUE_KEYS = new Set(EDITABLE_FLOW_STRUCTURAL_KEYS const FLOW_SPEC: WriteSpec = { probe: (workspace, path) => FlowService.existsFlowByPath({ workspace, path }), fetchDeployed: (workspace, path) => FlowService.getFlowByPath({ workspace, path }), - buildDraft: (base, args, chosenName) => { + buildDraft: (base, args, valuePath) => { const value = structuredClone(args.flow.value) if (args.flow.groups !== undefined && args.flow.groups !== null) { value.groups = structuredClone(args.flow.groups) @@ -5516,14 +5516,14 @@ const FLOW_SPEC: WriteSpec = { return base ? { ...structuredClone(base), - path: chosenName, + path: valuePath, summary: args.summary ?? base.summary, description: args.description ?? base.description, value, schema: args.flow.schema ?? base.schema } : { - path: chosenName, + path: valuePath, summary: args.summary ?? '', description: args.description ?? '', value, @@ -5544,11 +5544,11 @@ const SCHEDULE_SPEC: WriteSpec ScheduleService.existsSchedule({ workspace, path }), fetchDeployed: async (workspace, path) => (await ScheduleService.getSchedule({ workspace, path })) as ScheduleDraftConfig, - buildDraft: (base, args, chosenName) => { + buildDraft: (base, args, valuePath) => { // `override` is a tool-only conflict-resolution flag, not schedule config — // strip it so mergeDraftConfig doesn't clone it into the persisted draft. const { override: _override, ...config } = args - return mergeDraftConfig(base, config as DraftConfig, chosenName) + return mergeDraftConfig(base, config as DraftConfig, valuePath) } } @@ -5565,8 +5565,8 @@ function triggerWriteSpec(kind: TriggerKind): WriteSpec service.exists({ workspace, path }), fetchDeployed: async (workspace, path) => (await service.get({ workspace, path })) as TriggerDraftConfig, - buildDraft: (base, config, chosenName) => { - const draft = mergeDraftConfig(base, config, chosenName) + buildDraft: (base, config, valuePath) => { + const draft = mergeDraftConfig(base, config, valuePath) if (kind === 'email') { // workspaced_local_part maps to a NOT NULL column but is optional in the // tool schema. Default on the merged draft (not the incoming config) so an diff --git a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts index 18eae5d546..40a93b735b 100644 --- a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts +++ b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts @@ -375,21 +375,7 @@ async function storagePathForChosenName( if (staged.some((s) => s.storagePath === storagePath)) return staged.push({ storagePath, summary }) } - // A deployed item keeps its own path: drafts staged under that name belong to other - // items and are reached by their storage path. Settled before the listing, which is - // workspace-wide — an indexed single-row check answers the common case, where the path - // is simply where something is deployed, and a path that is unambiguous by this rule is - // then never refused because the listing failed. - // - // A failed probe falls through to the listing rather than out of the call: the listing - // below degrades on the same class of transient failure, and a read that addressed a - // deployed path made no such request at all before. const exists = deployedExists[itemKind] - try { - if (exists && (await exists(workspace, path))) return path - } catch { - // fall through to the listing - } let rows: Awaited> try { rows = await DraftService.listDrafts({ workspace }) @@ -402,6 +388,9 @@ async function storagePathForChosenName( // nothing the listing would have told a write either — refusing it would fail a // write that has no ambiguity to resolve. if (!opts.forWrite || !NAMEABLE_DRAFT_KINDS.has(itemKind)) return path + // A deployed item keeps its own path whatever is staged, so the listing was not + // needed after all. Only refuse when that cannot be established either. + if (exists && (await exists(workspace, path).catch(() => false))) return path throw new Error( `Could not load this workspace's drafts, so "${path}" cannot be matched to the draft ` + `it may name: ${e instanceof Error ? e.message : String(e)}. Try again.` @@ -417,6 +406,11 @@ async function storagePathForChosenName( } } if (staged.length === 0) return path + // A deployed item keeps its own path: drafts staged under that name belong to other + // items and are reached by their storage path. Consulted only once something is staged, + // so the common path costs no request and a failure here — which would leave a delete or + // a deploy pointing at a namesake draft — stops the call rather than being guessed past. + if (exists && (await exists(workspace, path))) return path if (staged.length > 1) { // Refused rather than guessed: nothing checks a name for uniqueness before deploy. const listed = staged