diff --git a/apps/server/src/plan/service.ts b/apps/server/src/plan/service.ts index 3f0e8ad18..b72b0765b 100644 --- a/apps/server/src/plan/service.ts +++ b/apps/server/src/plan/service.ts @@ -1988,6 +1988,8 @@ export async function rewrite( try { Questions.rebase(candidate); Comments.rebase(candidate); + // A reviewed link whose blocks the rewrite left alone stays reviewed. + let reviewed = Questions.snapshot(candidate); let outcome = edit.replace(candidate, plan.revision, nextSource); if (outcome.ok && plan.persistence.documentTitles) room.trackTitle(document); if (!outcome.ok) return outcome; @@ -1995,7 +1997,7 @@ export async function rewrite( let changed = source !== before; if (changed) { Questions.rebase(candidate); - Questions.invalidate(candidate, "plan_changed"); + Questions.invalidate(candidate, "plan_changed", reviewed); Comments.rebase(candidate); Comments.invalidate(candidate, "plan_changed"); } else { @@ -2039,7 +2041,7 @@ export async function rewrite( plan.outlines = candidate.outlines; if (changed) { Questions.rebase(plan, before); - Questions.invalidate(plan, "plan_changed"); + Questions.invalidate(plan, "plan_changed", reviewed); Comments.rebase(plan); Comments.invalidate(plan, "plan_changed"); } diff --git a/apps/server/src/questions/anchors.ts b/apps/server/src/questions/anchors.ts index 115b1d246..5a4788775 100644 --- a/apps/server/src/questions/anchors.ts +++ b/apps/server/src/questions/anchors.ts @@ -62,17 +62,34 @@ export function read(record: Question): Plan.WidgetAnchors { * Called when the plan changes or an answer does. The prose a decision lives in * is the thing most likely to have been rewritten, and a link to where it used * to be is worse than an admission that nobody has checked. + * + * Given the relationships from before a whole-document rewrite, one that was + * reviewed and whose every block survived with the same digest stays reviewed: + * nothing it points at changed, and only a Planner turn could review it again. */ -export function invalidate(record: Question, reason: Plan.AnchorReason): Plan.WidgetAnchors { +export function invalidate( + record: Question, + reason: Plan.AnchorReason, + before?: Plan.WidgetAnchors, +): Plan.WidgetAnchors { let value = read(record); if (record.status !== "answered") return value; for (let [id, set] of Object.entries(value.questions)) { + if (unchanged(before?.questions[id], set)) continue; value.questions[id] = { ...set, pending: true, reason }; } return value; } +function unchanged(before: Plan.AnchorSet | undefined, after: Plan.AnchorSet): boolean { + if (!before || before.pending || before.anchors.length === 0) return false; + if (after.pending || after.anchors.length !== before.anchors.length) return false; + return after.anchors.every((anchor, index) => + !anchor.orphaned && anchor.digest === before.anchors[index]!.digest + ); +} + /** * A decision's relationships once it is answered. * diff --git a/apps/server/src/questions/rewrite-links.test.ts b/apps/server/src/questions/rewrite-links.test.ts new file mode 100644 index 000000000..147217832 --- /dev/null +++ b/apps/server/src/questions/rewrite-links.test.ts @@ -0,0 +1,129 @@ +import { afterEach, expect, test } from "bun:test"; + +import * as room from "../plan/room"; +import * as Service from "../plan/service"; +import { openPlan } from "../testing/plan"; + +import * as Questions from "./service"; + +const WIDGET = "01K0N4TR8K7JGM4R1J7PW4R8YJ"; +const QUESTION = "01K0N4V4E7Y6P4MJ5WD8XZF3B2"; +const OPTION = "01K0N4W3B7P27CBAEC7A8C8WEA"; +const SOURCE = `Storage section. + +Deployment section. + +Retention section. + + + + + +`; + +let plans: Service.Plan[] = []; +let reopen = new Map Promise>(); + +afterEach(async () => { + for (let plan of plans) await Service.close(plan); + plans = []; + reopen.clear(); +}); + +/** An answered decision the Planner linked to the given blocks, reviewed. */ +async function linked(indices: number[]) { + let context = await openPlan(SOURCE, { + revision: 1, + questions: [{ + id: WIDGET, + status: "answered", + resolver: "ana", + definition: { + questions: [{ + id: QUESTION, + header: "Cache", + question: "How long do we cache?", + multiple: false, + options: [{ id: OPTION, label: "60 seconds", description: "" }], + }], + }, + answers: { [QUESTION]: "60 seconds" }, + }], + }); + plans.push(context.plan); + reopen.set(context.plan, () => Service.open(context.channel.id, context.backend, context.server)); + let digests = room.digests(context.plan.document); + let blocks = indices.map(index => ({ index, digest: digests[index]! })); + expect(Questions.relate(context.plan, WIDGET, QUESTION, blocks)).toBeUndefined(); + await Service.persist(context.plan); + expect(Questions.outstanding(context.plan)).toEqual([]); + return context.plan; +} + +function rewrite(plan: Service.Plan, from: string, to: string) { + let source = Service.source(plan); + let next = source.replace(from, to); + expect(next).not.toBe(source); + return Service.rewrite(plan, next, undefined); +} + +function link(plan: Service.Plan) { + return Questions.anchors(plan).find(item => item.widget === WIDGET)!.questions[QUESTION]!; +} + +test("a rewrite that leaves a decision's blocks alone keeps its link reviewed", async () => { + let plan = await linked([0]); + + expect((await rewrite(plan, "Deployment section.", "Deployment section, revised.")).ok) + .toBe(true); + + let set = link(plan); + expect(set.pending).toBe(false); + expect(set.reason).toBeUndefined(); + expect(set.anchors).toHaveLength(1); + expect(set.anchors[0]!.orphaned).toBeUndefined(); + expect(room.digests(plan.document).indexOf(set.anchors[0]!.digest)).toBe(0); + expect(Questions.outstanding(plan)).toEqual([]); + + await Service.close(plans.pop()!); + let reopened = await reopen.get(plan)!(); + plans.push(reopened); + expect(link(reopened).pending).toBe(false); + expect(Questions.outstanding(reopened)).toEqual([]); +}); + +test("a rewrite of the linked block owes a review", async () => { + let plan = await linked([0]); + + expect((await rewrite(plan, "Storage section.", "Storage section, revised.")).ok).toBe(true); + + expect(link(plan).pending).toBe(true); + expect(Questions.outstanding(plan)).toHaveLength(1); +}); + +test("a rewrite flags a decision when any one of its blocks changed or went away", async () => { + let edited = await linked([0, 1]); + expect((await rewrite(edited, "Deployment section.", "Deployment section, revised.")).ok) + .toBe(true); + expect(link(edited).pending).toBe(true); + + let removed = await linked([0, 2]); + expect((await rewrite(removed, "Retention section.\n\n", "")).ok).toBe(true); + expect(link(removed).pending).toBe(true); + expect(link(removed).anchors.map(anchor => !!anchor.orphaned)).toEqual([false, true]); +}); + +test("a rewrite leaves pending and deliberately unlinked decisions owing a review", async () => { + let pending = await linked([0]); + Questions.invalidate(pending, "plan_changed"); + expect((await rewrite(pending, "Deployment section.", "Deployment section, revised.")).ok) + .toBe(true); + expect(link(pending)).toMatchObject({ pending: true, reason: "plan_changed" }); + + let unlinked = await linked([]); + expect((await rewrite(unlinked, "Deployment section.", "Deployment section, revised.")).ok) + .toBe(true); + expect(link(unlinked)).toMatchObject({ anchors: [], pending: true, reason: "plan_changed" }); +}); diff --git a/apps/server/src/questions/service-relationships.ts b/apps/server/src/questions/service-relationships.ts index 925129176..fba484ebd 100644 --- a/apps/server/src/questions/service-relationships.ts +++ b/apps/server/src/questions/service-relationships.ts @@ -58,12 +58,25 @@ export function carry(plan: Plan, set: Wired.AnchorSet): Wired.AnchorSet { }; } -export function invalidate(plan: Plan, reason: Wired.AnchorReason): void { +export function invalidate( + plan: Plan, + reason: Wired.AnchorReason, + /** From before a rewrite: a reviewed link whose blocks all kept their digest stays reviewed. */ + before?: Map, +): void { for (let [id, record] of plan.records) { - plan.records.set(id, { ...record, anchors: Anchors.invalidate(record, reason) }); + plan.records.set(id, { + ...record, + anchors: Anchors.invalidate(record, reason, before?.get(id)), + }); } } +/** Every decision's relationships as they stand, to compare with after an edit. */ +export function snapshot(plan: Plan): Map { + return new Map([...plan.records].map(([id, record]) => [id, Anchors.read(record)])); +} + export function outstanding(plan: Plan): Anchors.Pending[] { return [...plan.records.values()].flatMap(record => Anchors.pending(record)); } diff --git a/apps/server/src/questions/service.ts b/apps/server/src/questions/service.ts index a11102554..246ca6e6d 100644 --- a/apps/server/src/questions/service.ts +++ b/apps/server/src/questions/service.ts @@ -46,6 +46,7 @@ export { rebase, relate, setProse, + snapshot, } from "./service-relationships"; export type { Placement } from "./service-relationships"; export { reopen } from "./service-reopen"; diff --git a/docs/local-agent-mcp.md b/docs/local-agent-mcp.md index f506f2452..6d421510c 100644 --- a/docs/local-agent-mcp.md +++ b/docs/local-agent-mcp.md @@ -239,6 +239,11 @@ document delta commits. Their change list identifies the MCP client without displaying a Planner cursor. Calls without initialization are attributed to `unknown`. +An answered decision stays linked to its text when the update leaves every +block it is linked to unchanged. Any other answered decision, including one whose +linked text changed or was removed, waits for a Planner turn to link it again +and shows as not linked, or as having its related text removed, until then. + ## Upload an image Document images are either absolute `https:` URLs or images Chopin hosts itself. diff --git a/e2e/mcp-update.e2e.ts b/e2e/mcp-update.e2e.ts index ab646b4bf..80b0f41bc 100644 --- a/e2e/mcp-update.e2e.ts +++ b/e2e/mcp-update.e2e.ts @@ -1,3 +1,5 @@ +import { createHash } from "node:crypto"; + import { content, expect, test } from "./room"; async function mcp(baseURL: string, body: unknown, session?: string): Promise { @@ -113,3 +115,77 @@ test("MCP changes identify the client and revision transition in the change list `Review bot 1.2.3, revisions ${revision} to ${revision + 1}`, ); }); + +test("an MCP rewrite of other prose keeps a linked decision's way to the document", async ({ baseURL, join, room, seed }) => { + let widget = "01K0N4TR8K7JGM4R1J7PW4R8YJ"; + let question = "01K0N4V4E7Y6P4MJ5WD8XZF3B2"; + let option = "01K0N4W3B7P27CBAEC7A8C8WEA"; + let linked = "We roll out with a canary."; + let source = `# Title\n\n${linked}\n\nOther paragraph.\n\n` + + `\n` + + `\n` + + `\n\n`; + let digest = `sha256:${createHash("sha256").update(`${linked}\n`).digest("hex")}`; + await seed(source, { + revision: 1, + questions: [{ + id: widget, + status: "answered", + definition: { + questions: [{ + id: question, + header: "Rollout", + question: "How should we deploy?", + multiple: false, + options: [{ id: option, label: "Canary", description: "" }], + }], + }, + answers: { [question]: "Canary" }, + resolver: "ana", + anchors: { + widget, + questions: { + [question]: { anchors: [{ epoch: "stale", position: "", digest }], pending: false }, + }, + }, + }], + }); + let page = await join("ana"); + await page.getByRole("button", { name: /^Decisions/ }).click(); + let card = page.locator( + `[data-document-view="decisions"] [data-plan-sidecar-questionnaire="${widget}"]`, + ); + await expect(card.getByRole("button", { name: "Show in document" })).toBeVisible(); + + let read = await (await mcp(baseURL!, { + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { name: "read_document", arguments: { id: room } }, + })).json(); + let current = read.result.structuredContent; + let updated = await (await mcp(baseURL!, { + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { + name: "update_document", + arguments: { + id: room, + revision: current.revision, + plan: current.source.replace("Other paragraph.", "Other paragraph, revised."), + idempotencyKey: "e2e-keep-link", + }, + }, + })).json(); + expect(updated.result.isError).toBeUndefined(); + await page.getByRole("button", { name: /^Document(?:,|$)/ }).click(); + await expect(content(page)).toContainText("Other paragraph, revised."); + await page.getByRole("button", { name: /^Decisions/ }).click(); + await expect(card.getByRole("button", { name: "Show in document" })).toBeVisible(); + await expect(card.locator("[data-relation]")).toHaveAttribute("data-relation", "linked"); + await page.reload(); + await page.getByRole("button", { name: /^Decisions/ }).click(); + await expect(card.getByRole("button", { name: "Show in document" })).toBeVisible(); +});