From ca5912c8b6cbe61197749d49cd8827a0a646da9f Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 15 Sep 2026 21:49:49 +0200 Subject: [PATCH] fix: hold a waiting row across a read, guard learn, keep a kept draft masked Co-Authored-By: Claude Opus 5 (1M context) --- .../components/copilot/chat/global/core.ts | 6 +- frontend/src/lib/itemStore.svelte.ts | 17 ++++- frontend/src/lib/itemStore.test.ts | 67 ++++++++++++++++++- 3 files changed, 83 insertions(+), 7 deletions(-) diff --git a/frontend/src/lib/components/copilot/chat/global/core.ts b/frontend/src/lib/components/copilot/chat/global/core.ts index 2b87b08ac9..a90aa59d66 100644 --- a/frontend/src/lib/components/copilot/chat/global/core.ts +++ b/frontend/src/lib/components/copilot/chat/global/core.ts @@ -6608,8 +6608,10 @@ async function discardLocalDraft( const discarded = await deleteGlobalDraft(workspace, type, path, triggerKind) // The chat's touch on the item is undone — drop it from the mask so a - // pre-existing deployed item doesn't keep reading as this chat's edit. - const discardedKind = itemKindFor(type, triggerKind) + // pre-existing deployed item doesn't keep reading as this chat's edit. Only when the draft + // actually went: one an editor kept is still this chat's edit, and dropping it would leave it + // out of Compare & Deploy. + const discardedKind = discarded.removed ? itemKindFor(type, triggerKind) : undefined if (discardedKind) { toolCallbacks.onItemDiscarded?.( discardedKind, diff --git a/frontend/src/lib/itemStore.svelte.ts b/frontend/src/lib/itemStore.svelte.ts index 69038c3c7e..f0bc0e4b35 100644 --- a/frontend/src/lib/itemStore.svelte.ts +++ b/frontend/src/lib/itemStore.svelte.ts @@ -412,6 +412,7 @@ class Entry { let rowsAtRead = 0 let valuesAtRead = 0 let unbasedPending = false + let waitingBefore: unknown | undefined let res: ItemLoad try { if (this.retired) return superseded @@ -431,6 +432,11 @@ class Entry { // above already has its row on this side of the count, and crediting it again // would pay for a row that arrives later and belongs to nobody here. valuesAtRead = this.values + // Taken before the read, not after: a row waiting here may land while the read is + // out, and then the response predates it while `pending` has gone quiet. Read + // afterwards, this entry would answer with the older value and reseed over the + // baseline that row's own save had just set. + waitingBefore = this.ports.pending(key) res = await adapter.load(key) } catch (e) { this.error = errorMessage(e) @@ -466,7 +472,7 @@ class Entry { // A row the syncer took but could not send is newer than this response and is the only // copy of that edit, so the item opens on it. One the server *refused* is the other // way round — the response is what won — and `adoptRow` drops it below. - const unsent = this.ports.conflicted(key) ? undefined : this.ports.pending(key) + const unsent = this.ports.conflicted(key) ? undefined : waitingBefore // A parked delete is what this tab wants gone, not what is there: the row is still the // server's, so the reconcile at the end re-issues the delete instead of believing it // landed and leaving the draft behind with nothing to remove it. @@ -837,7 +843,7 @@ class Entry { /** Fetch something about the deployed item the load did not carry (a secret's plaintext), * and write it into both sides. */ learn(fetch: (key: ItemKey) => Promise<(side: V) => void>): Promise { - return this.run(async () => { + return this.run(async (at) => { let apply: (side: V) => void try { apply = await fetch(this.key) @@ -849,7 +855,12 @@ class Entry { apply(d) this.serverDeployed = d } - if (this.value !== undefined) { + // What it learned is about the deployed item, so that side takes it either way. The + // value is the user's, and anything written there while the fetch was out is newer + // than this: folding a decrypted secret from before into it would put the old one + // back and autosave it. + const moved = this.edits !== at.edits || this.externals !== at.externals + if (this.value !== undefined && !moved) { const v = snapshot(this.value) apply(v) this.replaceValue(v) diff --git a/frontend/src/lib/itemStore.test.ts b/frontend/src/lib/itemStore.test.ts index 43f708d8cd..9a7b763c67 100644 --- a/frontend/src/lib/itemStore.test.ts +++ b/frontend/src/lib/itemStore.test.ts @@ -90,8 +90,16 @@ function fakeRows() { handed(key.path) }, rowMark: (key) => sends.get(key.path) ?? 0, - pending: (key) => (parked.has(key.path) ? parked.get(key.path) : undefined), - unbased: (key) => parked.has(key.path) && !baselines.has(key.path), + // `pendingSaveOpts` holds the latest payload from the moment it is handed over, so a row + // still in the debouncer counts as waiting just as much as one the server refused. + pending: (key) => + queued.has(key.path) + ? queued.get(key.path) + : parked.has(key.path) + ? parked.get(key.path) + : undefined, + unbased: (key) => + (queued.has(key.path) || parked.has(key.path)) && !baselines.has(key.path), markConflict: (key) => void conflicts.add(key.path), seedSync: (key, at, since) => { // The syncer refuses a baseline from a read a row handed over since has passed. @@ -109,7 +117,10 @@ function fakeRows() { failure: (key) => failures.get(key.path), dropPending: (key) => { dropped.push(key.path) + // Cancels the debounce as well as forgetting the refused payload: both live in + // `pendingSaveOpts`, and leaving the queued one would resurrect what was dropped. parked.delete(key.path) + queued.delete(key.path) }, hint: (key, on) => void hints.set(key.path, on) } @@ -1829,6 +1840,58 @@ describe('item store: conflicts', () => { expect(open.deployed).toEqual({ ...b, args: { a: 2 } }) }) + it('does not fold a secret it fetched over a value written since', async () => { + type Var = { path: string; value: string; is_secret: boolean } + const rows = fakeRows() + const store = createItemStore(rows.port) + const decrypting = deferred<(side: Var) => void>() + const { handle: item } = store.acquire( + { workspace: 'w', kind: 'variable', path: 'u/me/v' }, + { workspace: 'w', path: 'u/me/v' }, + { + load: async () => ({ + deployed: { path: 'u/me/v', value: '$encrypted:xx', is_secret: true } + }) + } as ItemAdapter + ) + await settle() + + const learning = item.learn(() => decrypting.promise) + await settle() + // The chat writes a newer secret while the decryption request is out. + item.applyExternal({ path: 'u/me/v', value: 'the new secret', is_secret: true }) + decrypting.resolve((side) => void (side.value = 'the old secret')) + + expect(await learning).toEqual({ ok: true }) + // What it decrypted is the deployed secret from before that write. Folding it into the + // value would put the old one back on screen and autosave it over the new one. + expect(item.value?.value).toBe('the new secret') + expect(item.deployed?.value).toBe('the old secret') + }) + + it('keeps a row that landed while its reopening read was in flight', async () => { + const rows = fakeRows() + const read = deferred>() + const store = createItemStore(rows.port) + const key: ItemKey = { workspace: 'w', kind: 'resource', path: 'u/me/r' } + const mine = { ...deployedRes, description: 'queued, not yet sent' } + const a = adapter(() => read.promise) + // A first autosave is queued and unsent, so the key has no baseline yet. + rows.port.write(key, mine) + const acq = store.acquire(key, { workspace: 'w', path: 'u/me/r' }, a) + await settle() + // Its debounce fires and lands while the reopening GET is still out. + await rows.port.flush(key) + // The GET predates that, so it reports no draft at all. + read.resolve({ deployed: deployedRes }) + await settle() + + // Answering with the deployed value would hide the draft that just landed and reseed the + // baseline its own save had set. What was waiting when the read went out is what stands. + expect(acq.handle.value).toEqual(mine) + expect(rows.seeds).toEqual([]) + }) + it('does not replay a first failed autosave over a draft created since', async () => { const rows = fakeRows() const store = createItemStore(rows.port)