From 5d1b20fb62a42b2f15d011adc8194fa4cc2771e7 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Wed, 7 Oct 2026 02:50:25 -0700 Subject: [PATCH] refactor: let a resolved target carry only the address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third CI round, third P1 from the same source. `ResolvedDraftTarget` carried the draft's value alongside its key, so that a body reading the draft it just resolved would not fetch twice. Every reader that merges onto that value can be stale by the time it runs: a tool awaits other work before writing, and the user can edit, save and close the editor in that window. The first two rounds patched `readGlobalDraftValue` — once for the open editor, once for the editor that closed. Codex then found the same bug in the next reader along: `write_flow` restores `inline_script.` placeholders through `getGlobalDraft`, so a body saved during the wait came back as the old code. Worse than before the patches, since the metadata around it was now preserved: the user would see their settings survive and their code silently revert. So the value is gone from the type. A resolved target is a path and a storage key, nothing else, and the two readers below it fetch: the cell if an editor holds one, else the backend. The cost is one request on calls that used to answer from the carried value; three P1s came out of saving it. `addressedDisplayPath` named the item from that value; it now asks the open editor, which answers synchronously and is the only place a name can be fresher than the stored draft anyway. Tests: the placeholder race Codex described, which fails against the previous commit's code. Also drops a comment line the `fresh` removal left restating the sentence above it. Co-Authored-By: Claude Opus 5 (1M context) --- .../copilot/chat/global/core.test.ts | 61 +++++++++++++++++++ .../components/copilot/chat/global/core.ts | 24 ++++---- .../copilot/chat/global/userDraftAdapter.ts | 40 ++++++------ .../src/lib/components/copilot/chat/shared.ts | 4 -- 4 files changed, 94 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 31fb8e954b..67165e28e2 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.test.ts @@ -5569,6 +5569,67 @@ describe('global AI tools', () => { ).resolves.toBe(code) }) + // A tool awaits other work between the dispatch resolving its target and its write — + // `write_flow` waits on the provider catalog. If the user edits an inline script and saves + // in that window, restoring an `inline_script.` placeholder from anything the dispatch + // read would put the old body back. + it('restores an inline script from the current draft, not the one resolution saw', async () => { + const path = 'u/admin/draft_inline_race' + const flowWith = (code: string) => ({ + path, + draft_path: 'f/team/raced_flow', + summary: 'Raced', + value: { + modules: [ + { + id: 'call_api', + value: { type: 'rawscript', language: 'bun', content: code, input_transforms: {} } + } + ] + } + }) + seedBackendDraft('flow', path, flowWith('export async function main() { return 1 }'), { + workspace: WORKSPACE + }) + + const args = { + path, + summary: 'Reordered', + modules: JSON.stringify([ + { + id: 'call_api', + value: { + type: 'rawscript', + language: 'bun', + content: 'inline_script.call_api', + input_transforms: {} + } + } + ]) + } + const tool = getGlobalTool('write_flow') + const target = await tool.draftTarget?.({ args, workspace: WORKSPACE }) + + // The user saves a new body while the tool is still awaiting. + seedBackendDraft('flow', path, flowWith('export async function main() { return 2 }'), { + workspace: WORKSPACE + }) + + await tool.fn({ + args, + workspace: WORKSPACE, + helpers: {}, + toolCallbacks, + toolId: 'test-race', + target + }) + + const saved = getBackendDraft('flow', path, { workspace: WORKSPACE }) + expect(saved?.value?.modules?.[0]?.value?.content).toBe( + 'export async function main() { return 2 }' + ) + }) + it('writes flows with flow-mode arguments and reads compact flow value', async () => { const writeResult = JSON.parse( await callGlobalTool('write_flow', { diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index ac97aa80b4..f3c8306429 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -248,6 +248,7 @@ import { itemKindFor, chosenDraftName, listGlobalDrafts, + liveGlobalDraftDisplayPath, liveGlobalDraftStoragePath, type DraftTarget, readGlobalDraftValue, @@ -4808,14 +4809,16 @@ function draftTarget( /** The name the addressed draft deploys under, known before the write from the value the * dispatch already read. Falls back to the addressed path when no draft was found there. */ function addressedDisplayPath( + workspace: string, target: ResolvedDraftTarget, type: WorkspaceItemType, triggerKind?: TriggerKind ): string { - const itemKind = itemKindFor(type, triggerKind) - const name = - itemKind && target.value !== undefined ? chosenDraftName(itemKind, target.value) : undefined - return name ?? target.path + // From the open editor, which holds the freshest name and answers without a request. A + // draft no editor holds falls back to the path the call named; the write's own result + // names it properly once the value has been read. + const live = liveGlobalDraftDisplayPath(workspace, type, target.storagePath, triggerKind) + return live === target.storagePath ? target.path : live } /** The draft this call addresses. Failing here beats writing to a path that was never @@ -5521,7 +5524,7 @@ async function writeDraft( ): Promise { const { workspace } = ctx const target = draftTargetOf(ctx) - startDraftWrite(ctx, type, addressedDisplayPath(target, type, opts.triggerKind)) + startDraftWrite(ctx, type, addressedDisplayPath(workspace, target, type, opts.triggerKind)) const existingDraft = await readGlobalDraftValue(workspace, type, target, { triggerKind: opts.triggerKind @@ -7006,7 +7009,7 @@ async function writeAppFile( assertNotGeneratedAppFile(target.filePath) const draft = draftTargetOf(ctx) - const appName = addressedDisplayPath(draft, 'app') + const appName = addressedDisplayPath(workspace, draft, 'app') toolCallbacks.setToolStatus(toolId, { content: `Writing ${target.filePath} to app "${appName}"...` }) @@ -7040,7 +7043,7 @@ async function deleteAppFile( assertNotGeneratedAppFile(target.filePath) const draft = draftTargetOf(ctx) - const appName = addressedDisplayPath(draft, 'app') + const appName = addressedDisplayPath(workspace, draft, 'app') toolCallbacks.setToolStatus(toolId, { content: `Deleting ${target.filePath} from app "${appName}"...` }) @@ -7087,7 +7090,7 @@ async function patchAppFile( } const draft = draftTargetOf(ctx) - const appName = addressedDisplayPath(draft, 'app') + const appName = addressedDisplayPath(workspace, draft, 'app') toolCallbacks.setToolStatus(toolId, { content: `Patching ${target.filePath} in app "${appName}"...` }) @@ -7162,7 +7165,7 @@ async function writeAppRunnable( const { workspace, toolId, toolCallbacks } = ctx const { key, runnable: input } = args const target = draftTargetOf(ctx) - const appName = addressedDisplayPath(target, 'app') + const appName = addressedDisplayPath(workspace, target, 'app') toolCallbacks.setToolStatus(toolId, { content: `Writing runnable "${key}" to app "${appName}"...` }) @@ -7308,7 +7311,7 @@ async function deleteAppRunnable( const { workspace, toolId, toolCallbacks } = ctx const { key } = args const target = draftTargetOf(ctx) - const appName = addressedDisplayPath(target, 'app') + const appName = addressedDisplayPath(workspace, target, 'app') toolCallbacks.setToolStatus(toolId, { content: `Removing runnable "${key}" from app "${appName}"...` }) @@ -8598,7 +8601,6 @@ async function deployDraft( // The shared deployer deploys at the path inside the draft and doesn't report it // 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. - // Read after the flush above, which may have persisted a rename parked in the editor. deployedPath = chosenDraftName(type, await readGlobalDraftValue(workspace, type, target)) ?? storagePath const result = await deployDraftToWorkspace(type, storagePath, workspace, { diff --git a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts index 9fadc8ab58..445f576feb 100644 --- a/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts +++ b/frontend/src/lib/components/copilot/chat/global/userDraftAdapter.ts @@ -468,6 +468,20 @@ function liveEditorStoragePath( return path } +/** The name to show for a draft stored at `storagePath`, from the open editor alone — the + * staged rename it holds, else the key itself. Synchronous, so a status line can name the + * item without a request; a caller that needs the name of a draft no editor holds reads it + * from the value it already has. */ +export function liveGlobalDraftDisplayPath( + workspace: string, + type: WorkspaceItemType, + storagePath: string, + triggerKind?: TriggerKind +): string { + const itemKind = itemKindFor(type, triggerKind) + return itemKind ? liveDisplayPath(workspace, itemKind, storagePath).displayPath : storagePath +} + /** Follows the open editor's staged rename only, for callers that already hold a storage * path. A path that may be a draft's chosen name needs `resolveDraftTarget`. */ export function liveGlobalDraftStoragePath( @@ -493,10 +507,7 @@ export function liveGlobalDraftTarget( path: string, triggerKind?: TriggerKind ): ResolvedDraftTarget { - const storagePath = liveGlobalDraftStoragePath(workspace, type, path, triggerKind) - const itemKind = itemKindFor(type, triggerKind) - const cell = itemKind ? UserDraft.get(itemKind, storagePath, { workspace }) : undefined - return { path, storagePath, value: cell, fetched: cell !== undefined } + return { path, storagePath: liveGlobalDraftStoragePath(workspace, type, path, triggerKind) } } /** The draft a call addresses: the path it named, and whether the call goes on to write. */ @@ -523,16 +534,9 @@ export async function resolveDraftTarget( ): Promise { const { type, path, triggerKind, forWrite } = target const itemKind = itemKindFor(type, triggerKind) - if (!itemKind) return { path, storagePath: path, fetched: false } + if (!itemKind) return { path, storagePath: path } const resolved = await resolveDraft(workspace, itemKind, path, { forWrite }) - const cell = UserDraft.get(itemKind, resolved.storagePath, { workspace }) - return { - path, - 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 - } + return { path, storagePath: resolved.storagePath } } // Current user's persisted draft value (+ records the sync baseline so a later @@ -669,22 +673,18 @@ export async function getGlobalDraft( ): Promise { const itemKind = itemKindFor(type, triggerKind) if (!itemKind) return undefined - return draftItemAt(workspace, itemKind, target.storagePath, target) + return draftItemAt(workspace, itemKind, target.storagePath) } async function draftItemAt( workspace: string, itemKind: UserDraftItemKind, - storagePath: string, - found: { value?: unknown; fetched?: boolean } + storagePath: string ): Promise { // A draft can live only as a local cell — a second session tab, or an editor with // autosave off — with no backend row behind it. const cell = UserDraft.get(itemKind, storagePath, { workspace }) - const value = - cell ?? - found.value ?? - (found.fetched ? undefined : await fetchBackendDraftValue(workspace, itemKind, storagePath)) + const value = cell ?? (await fetchBackendDraftValue(workspace, itemKind, storagePath)) if (value === undefined || value === null) return undefined const { displayPath, isLiveDraft } = liveDisplayPath(workspace, itemKind, storagePath) return userDraftEntryToWorkspaceItem( diff --git a/frontend/src/lib/components/copilot/chat/shared.ts b/frontend/src/lib/components/copilot/chat/shared.ts index 79085304aa..aed0510464 100644 --- a/frontend/src/lib/components/copilot/chat/shared.ts +++ b/frontend/src/lib/components/copilot/chat/shared.ts @@ -1227,10 +1227,6 @@ export type ResolvedDraftTarget = { /** The path the call named. What the model and the user see, so messages quote this. */ path: string storagePath: string - /** The draft's value when resolving it already read one, so the body does not refetch. */ - value?: unknown - /** Whether `value` reflects a read: `undefined` then means no draft, not "not looked". */ - fetched: boolean } export interface Tool {