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. */