From e7c8a3051935938da42e6d170c621e04d4cdd86a Mon Sep 17 00:00:00 2001 From: Alex Lavaee Date: Wed, 4 Nov 2026 11:11:03 -0800 Subject: [PATCH] Refuse an evicted-read decision replay whose anchor was recovered by digest Without the Planner's retained read, a replay of an `open_decisions` key was accepted whenever the card stood right after a block carrying the replayed digest. When one edit rewrote the paragraph the card was opened on and left an identical copy of its old text between the rewritten paragraph and the card, the card's anchor was recovered by digest onto that copy, which is also the block before the card's widget, so the replay was reported as captured. With the read retained the same replay was refused. Every edit rebases the cards' anchors, and a rebase re-expresses an anchor it recovered by digest as a fresh position on the block it found. Asking afterwards whether the anchor still resolves by position therefore cannot tell a copy from the paragraph. The rebase of a card's relationship anchors now marks an anchor it found again by digest (`byDigest`, an optional field of the anchor in the card's stored record), keeps the mark through later rebases and a restart, and drops it only when the Planner anchors the card again. Comment, prose and follow-up anchors are rebased as before. Without the read, a replay is refused for a card whose anchor carries the mark, or whose stored anchor no longer resolves, with the cause that the card's paragraph can only be found again by its text. A card beside a paragraph that kept its place through insertions, deletions, edits and typing elsewhere is still accepted, with the read retained, evicted or lost to a restart. An edit that takes a card's position for any other reason (replacing the whole document, moving another block across the paragraph, a rebuilt epoch) marks it too, so such a replay is refused until the Planner reads again; a record stored before the mark existed has none. The decisions guide says so. Fixes #776 Assistant-model: Claude Sonnet 5.5 Assistant-workflow: goal (run 9570775f-38b7-4817-859f-1fd90b98cd5f) Assistant-duration: 40m converged, estimated 1h Assistant-verification: red before fix passed: with the source change stashed, the new replay tests failed (a rewrite plus copy through applyBlockEdit, through a browser plan:update, and after two restarts were all accepted as captured; one more lacked the new cause), and all 44 capture-decisions tests pass with it Assistant-verification: mutation check passed: dropping the mark's carry in the positional branch of the rebase failed 4 tests, not marking in the question rebase failed 4, and not marking at replay time failed 5 Assistant-verification: bun test apps/server passed: 4140 pass, 0 fail, 6 skipped without a database Assistant-verification: bun test packages apps/web apps/connector scripts passed: 2813 pass, 0 fail Assistant-verification: bun run fix passed: no changes, 72 existing lint warnings Assistant-verification: bun run types passed: whole workspace and e2e Assistant-verification: bun run ci passed: dprint, oxlint, tokens and design checks Assistant-verification: bun run e2e planner-decisions harness-decisions-deck mcp-update passed: 11 tests Assistant-verification: bun test apps/server/src/storage/postgres passed: 124 pass on a disposable PostgreSQL 17 at port 55432, because the 5432 that bun run test:postgres uses was taken; the marker's restart is covered with the memory adapter only Co-authored-by: Alex Lavaee --- apps/server/src/plan/room.ts | 22 +- .../src/questions/capture-decisions.test.ts | 234 +++++++++++++++++- apps/server/src/questions/service-publish.ts | 63 +++-- .../src/questions/service-relationships.ts | 2 +- docs/decisions.md | 18 +- packages/protocol/plan.d.ts | 6 + 6 files changed, 313 insertions(+), 32 deletions(-) diff --git a/apps/server/src/plan/room.ts b/apps/server/src/plan/room.ts index d9b3a295f..0fc197594 100644 --- a/apps/server/src/plan/room.ts +++ b/apps/server/src/plan/room.ts @@ -1114,8 +1114,16 @@ export function matchesAnchor(target: Document, anchor: Anchor, index: number): * identical paragraphs give no way to tell which was meant. The rest are kept * and marked orphaned rather than guessed at: a relationship pointing at the * wrong passage is worse than one admitting it is lost. + * + * A match by digest alone may be an identical copy of the block the anchor was + * made for, and re-expressing it as a position hides that. With `marking`, such + * an anchor says so with `byDigest`, and an anchor already saying so keeps it. */ -export function rebase(target: Document, anchors: Anchor[]): Anchor[] { +export function rebase( + target: Document, + anchors: Anchor[], + options: { marking?: boolean } = {}, +): Anchor[] { let hashes = digests(target); let keys: string[] = []; target.editor.getEditorState().read(() => { @@ -1136,12 +1144,20 @@ export function rebase(target: Document, anchors: Anchor[]): Anchor[] { if (key) { let index = keys.indexOf(key); let hash = hashes[index]; - if (hash) return anchorForKey(target, key, hash); + if (hash) { + return { + ...anchorForKey(target, key, hash), + ...(options.marking && anchor.byDigest ? { byDigest: true as const } : {}), + }; + } } let matches = byDigest.get(anchor.digest) ?? []; return matches.length === 1 - ? anchorForKey(target, matches[0], anchor.digest) + ? { + ...anchorForKey(target, matches[0], anchor.digest), + ...(options.marking ? { byDigest: true as const } : {}), + } : { ...anchor, epoch: target.epoch, orphaned: true as const }; }); } diff --git a/apps/server/src/questions/capture-decisions.test.ts b/apps/server/src/questions/capture-decisions.test.ts index 38cc9dbf8..7e5763486 100644 --- a/apps/server/src/questions/capture-decisions.test.ts +++ b/apps/server/src/questions/capture-decisions.test.ts @@ -1,6 +1,9 @@ import { afterEach, expect, spyOn, test } from "bun:test"; +import { $createParagraphNode, $createTextNode, $getRoot } from "lexical"; +import * as Y from "yjs"; import { ULID } from "@chopin/dialect"; import * as Question from "@chopin/question"; +import { applyBlockEdit } from "../agent/document-ops"; import * as Edit from "../plan/edit"; import * as Room from "../plan/room"; import * as Plan from "../plan/service"; @@ -10,6 +13,8 @@ import * as Questions from "./service"; import * as Store from "./store"; import { member } from "./text-response.test-fixtures"; +import type { DocumentRoom } from "../agent/tools"; + let contexts: Awaited>[] = []; afterEach(async () => { for (let context of contexts) await Plan.close(context.plan); @@ -525,10 +530,11 @@ async function expectNothingCreated( let frames = context.broadcasts.length; let refusal = await Capture.captureDecisions(context.plan, context.server, context.plan.id, input) .then(() => undefined, (error: unknown) => error as Error); - expect(refusal?.message).toContain(REFUSED); - if (cause) expect(refusal?.message).toContain(cause); - expect(refusal?.message).not.toContain("different decision"); - expect(refusal?.message).not.toContain("new key"); + if (!refusal) throw new Error("the batch was accepted as captured, not refused"); + expect(refusal.message).toContain(REFUSED); + if (cause) expect(refusal.message).toContain(cause); + expect(refusal.message).not.toContain("different decision"); + expect(refusal.message).not.toContain("new key"); expect(context.plan.records.size).toBe(cards); expect(context.plan.questions.open.size).toBe(cards); expect(Room.project(context.plan.document)).toBe(source); @@ -677,3 +683,223 @@ test("a repeated key whose read was evicted is refused when a later passage was expect(Edit.read(context.plan, 0)).toBeUndefined(); await expectNothingCreated(context, input, 1, "the server no longer has that read"); }); + +/** + * The Planner's `edit_plan` path: the structural batch, the rebase of every decision's anchors + * that follows it, then the commit. `edited` above commits without that rebase. + */ +async function planned(context: Awaited>, operations: Edit.Operation[]) { + let target = { + id: context.plan.id, + plan: context.plan, + server: context.server, + publish: (mutation: { update: Uint8Array; source: string }) => + Plan.publish(context.plan, context.server, context.plan.id, mutation, { agent: true }), + persist: () => Plan.persist(context.plan), + exclusive: (action: () => Promise) => Plan.exclusive(context.plan, action), + anchors() {}, + changes() {}, + visualAdmission: async () => [], + } as unknown as DocumentRoom; + let outcome = await target.exclusive(() => + applyBlockEdit(target, { revision: context.plan.revision, operations }) + ); + if (!outcome.ok) throw new Error("fixture edit failed"); +} + +/** A person's browser edit: a Yjs update over the socket, then the rebase that follows it. */ +async function typed(context: Awaited>, change: () => void) { + let peer = await Room.restore( + context.plan.document.epoch, + Y.encodeStateAsUpdate(context.plan.document.doc), + Room.project(context.plan.document), + [], + ); + try { + let vector = Y.encodeStateVector(peer.doc); + peer.editor.update(change, { discrete: true }); + await Room.settle(); + let replies: Array> = []; + let socket = { + data: { canEdit: true, room: context.plan.id, handle: "ana", client: "client-ana" }, + send: (raw: string) => replies.push(JSON.parse(raw) as Record), + publish() {}, + close() {}, + } as unknown as Parameters[1]; + Plan.submit(context.plan, socket, { + kind: "plan:update", + ts: 0, + rid: "typed", + id: "typed", + epoch: context.plan.document.epoch, + update: Buffer.from(Y.encodeStateAsUpdate(peer.doc, vector)).toString("base64"), + }); + await Plan.drain(context.plan); + expect(replies.map(reply => reply.kind)).toContain("plan:ack"); + } finally { + peer.doc.destroy(); + } +} + +/** The edit of the issue: the paragraph rewritten, an identical copy of its old text left after it. */ +function rewriteLeavingCopy() { + let original = $getRoot().getChildren()[1]!; + let rewritten = $createParagraphNode().append( + $createTextNode("Provisional onboarding, rewritten."), + ); + original.replace(rewritten); + rewritten.insertAfter($createParagraphNode().append($createTextNode("Provisional onboarding."))); +} + +async function evictRead(context: Awaited>) { + for (let note = 0; note < 9; note++) { + await planned(context, [{ op: "insert_root", source: `Note ${note}.` }]); + } + expect(Edit.read(context.plan, 0)).toBeUndefined(); +} + +async function reopen(context: Awaited>) { + await Plan.close(context.plan); + context.plan = await Plan.open(context.plan.id, context.backend, context.server); + expect(Edit.read(context.plan, 0)).toBeUndefined(); +} + +/** Where the card's widget stands: right after the copy, which is the block before it. */ +function expectCardAfterCopy(context: Awaited>) { + expect(Room.questionnaireIndex(context.plan.document, [...context.plan.records.keys()][0]!)) + .toBe(3); +} + +async function opened() { + let context = await setup(); + let input = { revision: 0, questions: [question(context.plan, "decision-one")] }; + await Capture.captureDecisions(context.plan, context.server, context.plan.id, input); + return { context, input }; +} + +const NO_READ = "the server no longer has that read"; +const COPY = `${NO_READ}, and the card's paragraph can only be found again by its text`; + +test("a replay is refused while its read is retained when the Planner rewrote the paragraph and left a copy before the card", async () => { + let { context, input } = await opened(); + await planned(context, [{ + op: "replace", + index: 1, + source: "Provisional onboarding, rewritten.\n\nProvisional onboarding.", + }]); + expectCardAfterCopy(context); + expect(Edit.read(context.plan, 0)).toBeDefined(); + await expectNothingCreated(context, input, 1, "has been rewritten, removed or moved since"); +}); + +test("a replay without its read is refused when the Planner rewrote the paragraph and left a copy before the card", async () => { + let { context, input } = await opened(); + await planned(context, [{ + op: "replace", + index: 1, + source: "Provisional onboarding, rewritten.\n\nProvisional onboarding.", + }]); + expectCardAfterCopy(context); + await evictRead(context); + await expectNothingCreated(context, input, 1, COPY); +}); + +test("a replay without its read is refused when a person's edit rewrote the paragraph and left a copy before the card", async () => { + let { context, input } = await opened(); + await typed(context, rewriteLeavingCopy); + expectCardAfterCopy(context); + await evictRead(context); + await expectNothingCreated(context, input, 1, COPY); +}); + +test("a replay is refused after a restart that lost its read when the paragraph was rewritten with a copy left before the card", async () => { + let { context, input } = await opened(); + await planned(context, [{ + op: "replace", + index: 1, + source: "Provisional onboarding, rewritten.\n\nProvisional onboarding.", + }]); + expectCardAfterCopy(context); + await reopen(context); + expectCardAfterCopy(context); + await expectNothingCreated(context, input, 1, COPY); +}); + +test("a replay is still refused after a restart that follows a person's rewrite, later edits and another restart", async () => { + let { context, input } = await opened(); + await typed(context, rewriteLeavingCopy); + await planned(context, [{ op: "insert", index: 0, source: "A new opening." }]); + await reopen(context); + await planned(context, [{ op: "insert_root", source: "A closing note." }]); + await reopen(context); + await expectNothingCreated(context, input, 1, COPY); +}); + +test("a replay of a card beside an unchanged paragraph is accepted once, with its read retained, evicted or lost to a restart", async () => { + let { context, input } = await opened(); + let id = [...context.plan.records.keys()][0]!; + await planned(context, [{ op: "insert", index: 0, source: "A new opening." }]); + expect(Room.questionnaireIndex(context.plan.document, id)).toBe(3); + let replay = async () => { + let again = await Capture.captureDecisions( + context.plan, + context.server, + context.plan.id, + input, + ); + expect(again.decisions).toEqual([{ key: "decision-one", id, status: "open" }]); + expect(context.plan.records.size).toBe(1); + expect(context.plan.questions.open.size).toBe(1); + }; + expect(Edit.read(context.plan, 0)).toBeDefined(); + await replay(); + await evictRead(context); + await replay(); + await reopen(context); + await replay(); +}); + +test("a replay of a card whose paragraph keeps its place is accepted without its read after other paragraphs were inserted, edited or deleted around it", async () => { + let { context, input } = await opened(); + let id = [...context.plan.records.keys()][0]!; + await planned(context, [{ op: "insert", index: 0, source: "A new opening." }]); + await planned(context, [{ + op: "replace", + index: 4, + source: "Provisional collaboration, rewritten.", + }]); + await typed(context, () => { + let tail = $getRoot().getLastChild()!; + tail.insertAfter($createParagraphNode().append($createTextNode("A browser note."))); + }); + await planned(context, [{ op: "delete", index: 0 }]); + expect(Room.questionnaireIndex(context.plan.document, id)).toBe(2); + await evictRead(context); + let again = await Capture.captureDecisions(context.plan, context.server, context.plan.id, input); + expect(again.decisions).toEqual([{ key: "decision-one", id, status: "open" }]); + expect(context.plan.records.size).toBe(1); + await reopen(context); + let restarted = await Capture.captureDecisions( + context.plan, + context.server, + context.plan.id, + input, + ); + expect(restarted.decisions).toEqual([{ key: "decision-one", id, status: "open" }]); +}); + +test("a replay is refused without its read when no rebase has yet recorded that the anchor lost its position", async () => { + let { context, input } = await opened(); + await edited(context, [{ + op: "replace", + index: 1, + source: "Provisional onboarding, rewritten.\n\nProvisional onboarding.", + }]); + expectCardAfterCopy(context); + for (let note = 0; note < 9; note++) { + await edited(context, [{ op: "insert_root", source: `Note ${note}.` }]); + Edit.outline(context.plan); + } + expect(Edit.read(context.plan, 0)).toBeUndefined(); + await expectNothingCreated(context, input, 1, COPY); +}); diff --git a/apps/server/src/questions/service-publish.ts b/apps/server/src/questions/service-publish.ts index b26d07ca4..e2cbe038d 100644 --- a/apps/server/src/questions/service-publish.ts +++ b/apps/server/src/questions/service-publish.ts @@ -104,6 +104,8 @@ function unmatched(key: string, cause: string): string { + `document as you read it: ${cause}.`; } +const NO_READ = "the server no longer has that read"; + type Located = { index: number } | { cause: string }; /** @@ -118,7 +120,7 @@ function locate( block: { index: number; digest: string }, ): Located { let read = edit.read(plan, revision); - if (!read) return { cause: "the server no longer has that read" }; + if (!read) return { cause: NO_READ }; if (read.blocks[block.index]?.digest !== block.digest) { return { cause: "its address does not match the block you read at that position" }; } @@ -154,13 +156,25 @@ function readBlock( : { cause: "an identical paragraph now stands beside it" }; } -/** The block each passage of a card still names, or nothing where it can no longer be told. */ -function cardBlocks(plan: Plan, record: Record): Array { +/** Where each passage of a card now is, brought forward onto the document as it stands. */ +function cardAnchors(plan: Plan, record: Record) { let id = record.definition.questions[0]!.id; let anchors = Anchors.read(record).questions[id]?.anchors ?? []; - return room.rebase(plan.document, anchors).map(anchor => - anchor.orphaned ? undefined : room.blockNamedBy(plan.document, anchor) - ); + return room.rebase(plan.document, anchors, { marking: true }); +} + +/** + * The block each passage of a card still names, or nothing where it can no longer be told, and + * whether the card's anchor found it by digest alone, now or on an earlier rebase. + */ +function cardBlocks( + plan: Plan, + record: Record, +): Array<{ index: number; byDigest: boolean } | undefined> { + return cardAnchors(plan, record).map(anchor => { + let index = anchor.orphaned ? undefined : room.blockNamedBy(plan.document, anchor); + return index === undefined ? undefined : { index, byDigest: anchor.byDigest === true }; + }); } /** @@ -193,7 +207,7 @@ function namesAnotherPassage( return related.some((block, position) => { let located = locate(plan, readAt, block); return !("cause" in located) && hashes[located.index] === block.digest - && located.index !== blocks[position]; + && located.index !== blocks[position]?.index; }); } @@ -232,7 +246,9 @@ function sameDefinition(record: Record, wanted: Question.Item): boolean { * compared on its definition, as its passages no longer mean anything. Nothing is placed, so the * card's own anchor says which block it sits beside; the input's digest must still match that block, * and its index too unless the document has moved on since the Planner read it, in which case the - * block the Planner read, where the server still has that read, must be the card's block. + * block the Planner read, where the server still has that read, must be the card's block. Where it + * does not, the card must stand right after the one passage it was opened on and its anchor must + * never have been found again by digest alone: such an anchor may sit on an identical copy. */ function repeats( plan: Plan, @@ -248,17 +264,19 @@ function repeats( let hashes = room.digests(plan.document); let retained = readAt !== undefined && edit.read(plan, readAt) !== undefined; let standing = readAt !== undefined && !retained ? blockBeforeCard(plan, record) : undefined; - return blocks.every((index, position) => { + return blocks.every((found, position) => { let block = related[position]!; - if (index === undefined || hashes[index] !== block.digest) return false; - if (readAt === undefined) return index === block.index; + if (!found || hashes[found.index] !== block.digest) return false; + if (readAt === undefined) return found.index === block.index; if (retained) { let located = locate(plan, readAt, block); - return !("cause" in located) && located.index === index; + return !("cause" in located) && located.index === found.index; } // Without the read, only where the card stands says which paragraph it was opened beside, // and it stands beside the first passage alone; a later one cannot be told from a twin. - return position === 0 && standing === index; + // An anchor found again by digest alone says nothing more: it may sit on an identical copy + // of a paragraph rewritten since, which is also the block before the card. + return position === 0 && standing === found.index && !found.byDigest; }); } @@ -326,15 +344,18 @@ export async function publishQuestions( readAt !== undefined && sameDefinition(record, wanted) && !namesAnotherPassage(plan, readAt, record, related) ) { - let cause = related.map(block => readBlock(plan, readAt, block)).find(found => - "cause" in found + let found = related.map(block => readBlock(plan, readAt, block)).find(block => + "cause" in block ); - Question.reject(unmatched( - key, - cause && "cause" in cause - ? cause.cause - : "the card for this key no longer stands beside the block you read", - )); + let cause = found && "cause" in found + ? found.cause + : "the card for this key no longer stands beside the block you read"; + // Without that read, a card found again by digest alone may be on a copy. + if (cause === NO_READ && cardBlocks(plan, record).some(block => block?.byDigest)) { + cause += ", and the card's paragraph can only be found again by its text, which an " + + "identical copy would match too"; + } + Question.reject(unmatched(key, cause)); } Question.reject( `Decision key "${key}" already holds a different decision, so this one was not opened. ` diff --git a/apps/server/src/questions/service-relationships.ts b/apps/server/src/questions/service-relationships.ts index 8534293cd..1342a4018 100644 --- a/apps/server/src/questions/service-relationships.ts +++ b/apps/server/src/questions/service-relationships.ts @@ -60,7 +60,7 @@ export function rebase(plan: Plan, previousSource = plan.persistence?.committedS } export function carry(plan: Plan, set: Wired.AnchorSet): Wired.AnchorSet { - let anchors = room.rebase(plan.document, set.anchors); + let anchors = room.rebase(plan.document, set.anchors, { marking: true }); let lost = anchors.some(anchor => anchor.orphaned); return { anchors, diff --git a/docs/decisions.md b/docs/decisions.md index b342f38cd..affd8f328 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -41,9 +41,21 @@ retains each refuse the batch with a message saying the document changed and to rather than anchoring a card beside a twin. Nothing is created for a refused batch. Replaying a key whose card exists is judged against the same read. Where the server no longer holds it (after a restart, or once more than eight revisions have been read since), a replay is -accepted only for a card standing beside the single passage it was opened on; a card with several -passages is refused until the Planner reads again, since a later passage cannot then be told from -an identical copy. +accepted only for a card standing beside the single passage it was opened on, and only if that +card's anchor has never had to be found again by its digest. A card with several passages is +refused until the Planner reads again, since a later passage cannot then be told from an +identical copy. So is a card whose position in its paragraph was lost: after every edit the +server re-expresses an anchor as a position, and when an edit removes it (the paragraph was +rewritten or replaced, another block was moved across it, or the document was rebuilt under a +new epoch) the anchor is matched to the one block carrying its digest. That block may be an +identical copy of the paragraph, for instance one an edit left between the rewritten paragraph +and the card, so the match is recorded on the anchor in the stored decision, survives later +edits and restarts, and ends only when the Planner anchors the card again. The refusal says +the card's paragraph can only be found again by its text; reading the document again gives the +replay a read to be judged against. A card whose paragraph kept its place through insertions, +deletions, edits and typing elsewhere is still accepted, with or without the read. A card +recorded before the mark existed has none, and is judged by where it stands unless an edit +since has taken its position. Stop Planner does not withdraw committed cards. Each Save queues an update to the related prose. Decisions sharing a passage share its write diff --git a/packages/protocol/plan.d.ts b/packages/protocol/plan.d.ts index fcd3e5dcf..484cb9e6b 100644 --- a/packages/protocol/plan.d.ts +++ b/packages/protocol/plan.d.ts @@ -49,6 +49,12 @@ export declare namespace Plan { orphaned?: true; /** A unique disappeared prose block may recover on the next document edit only. */ recoverOnNextEdit?: true; + /** + * A decision's anchor that was found again by its digest after its position was lost, and + * since re-expressed as a position on what it found, which may be an identical copy of the + * block it was made for. It stays until the anchor is replaced. + */ + byDigest?: true; }; /** The editable paragraph a decided card became, kept even when its block is lost. */