From c2053bcbfc4bed59f2513a1c8817ec42474cc472 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Fri, 25 Sep 2026 17:24:43 +0200 Subject: [PATCH] perf: settle a deployed path without listing the workspace's drafts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-5 review findings. The deployed-item check now runs before the drafts listing again, but a failed probe is remembered rather than swallowed: it only matters once something is staged under the name, and there the call stops instead of guessing which draft it means. That keeps the cheap short-circuit for a path that is simply where something is deployed — one indexed row, no listing — without the exposure that made the first ordering unsafe for a delete or a deploy. A kind no editor stages a name for skips the lookup entirely: those reach neither the probe nor the listing, where before they always paid a workspace-wide read that checks write access per row. The trigger test that pinned name routing for such a kind now pins what the declaration is actually for — finding the path nested inside `config` — since the state it set up, an http trigger draft carrying `draft_path`, is one no editor or drawer produces. An empty path reached `draftTargetOf`, whose message blamed the tool definition for what is a malformed argument, and the model reads that string. It names the cause. Measured on the running instance: a read of a deployed script makes 0 listings and 1 exists; a variable write makes 0 listings; a script write to a fresh path still lists, since nothing is deployed there to settle it. Co-Authored-By: Claude Opus 5 (1M context) --- .../copilot/chat/global/core.test.ts | 20 +++++----- .../components/copilot/chat/global/core.ts | 7 ++-- .../copilot/chat/global/userDraftAdapter.ts | 39 ++++++++++++------- 3 files changed, 39 insertions(+), 27 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 ddc9ea2d21..cd71a547a4 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -3157,14 +3157,14 @@ describe('global AI tools', () => { }) // write_trigger is the one tool whose path is nested inside another argument, so its - // target declaration has to reach into `config` to find the name to resolve. - it('routes a trigger write nested in config to the draft staged under that name', async () => { + // target declaration has to reach into `config` for it. Without that the call resolves + // nothing and never reaches the draft it means to update. + it('resolves a trigger write through the path nested in config', async () => { seedBackendDraft( 'trigger_http', 'u/admin/draft_http_0001', { path: 'u/admin/draft_http_0001', - draft_path: 'u/admin/staged_route', script_path: 'f/scripts/handler', is_flow: false, route_path: 'api/staged', @@ -3178,7 +3178,7 @@ describe('global AI tools', () => { await callGlobalTool('write_trigger', { kind: 'http', config: { - path: 'u/admin/staged_route', + path: 'u/admin/draft_http_0001', script_path: 'f/scripts/handler', is_flow: false, route_path: 'api/renamed', @@ -3188,13 +3188,11 @@ describe('global AI tools', () => { } }) - expect( - getBackendDraft('trigger_http', 'u/admin/draft_http_0001', { workspace: WORKSPACE }) - ?.route_path - ).toBe('api/renamed') - expect( - getBackendDraft('trigger_http', 'u/admin/staged_route', { workspace: WORKSPACE }) - ).toBeUndefined() + const draft = getBackendDraft('trigger_http', 'u/admin/draft_http_0001', { + workspace: WORKSPACE + }) + expect(draft?.route_path).toBe('api/renamed') + expect(draft?.script_path).toBe('f/scripts/handler') }) // A script draft's chosen name IS `value.path`, and the deployer deploys at the path diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index 3047fde6e4..7b36df39fd 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -4750,11 +4750,12 @@ function draftTarget( } } -/** The draft this call addresses. Absent means the tool did not declare `draftTarget`; - * failing here beats writing to a path that was never resolved to a storage key. */ +/** The draft this call addresses. Failing here beats writing to a path that was never + * resolved to a storage key. Reachable when the call named an empty path, which the schemas + * accept — so the message names that rather than the declaration a reader cannot fix. */ function draftTargetOf(ctx: WriteDraftCtx): ResolvedDraftTarget { if (!ctx.target) { - throw new Error('Internal error: this tool did not declare the draft it addresses.') + throw new Error('This call named no item to act on. Pass the path of the item.') } return ctx.target } diff --git a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts index 40a93b735b..c52ec98b21 100644 --- a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts +++ b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts @@ -368,6 +368,10 @@ async function storagePathForChosenName( path: string, opts: { forWrite?: boolean } ): Promise { + // Nothing stages a name for this kind, so the path is already the key and there is no + // listing worth reading. + if (!NAMEABLE_DRAFT_KINDS.has(itemKind)) return path + const staged: StagedDraft[] = [] const add = (storagePath: string, summary: string | undefined) => { // A new script or flow editor stores under '', which no tool call can address. @@ -375,7 +379,19 @@ 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. Asked before the listing, which is + // workspace-wide and checks write access per row — one indexed lookup settles the common + // case, where the path is simply where something is deployed. const exists = deployedExists[itemKind] + let probeUnknown = false + try { + if (exists && (await exists(workspace, path))) return path + } catch { + // Unknown, not "nothing deployed": remembered, so a staged name below is never taken + // for this path's draft on the strength of a question that went unanswered. + probeUnknown = true + } let rows: Awaited> try { rows = await DraftService.listDrafts({ workspace }) @@ -383,14 +399,7 @@ async function storagePathForChosenName( // Nothing is stored at this path, so it may be a name. Without the listing that // cannot be told, and a write that guessed the path itself would create a second // draft; a read of whatever is deployed there is harmless. - // - // For a kind no editor stages a name for, the path cannot be one, so there is - // 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 + if (!opts.forWrite) 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.` @@ -406,11 +415,15 @@ 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 (probeUnknown) { + // Something is staged under this name, and whether the path is also a deployed item's + // own is what says which draft the call means. Answering it either way could delete or + // deploy an unrelated draft. + throw new Error( + `Could not tell whether something is already deployed at "${path}", so the draft staged ` + + `under that name cannot be identified. Try again.` + ) + } if (staged.length > 1) { // Refused rather than guessed: nothing checks a name for uniqueness before deploy. const listed = staged