diff --git a/docs/branch-review-records/09f7a6223d2cc9cf801d28f42c84347fb6e8fe8167098620126ddf8a61a1994e.record.md b/docs/branch-review-records/09f7a6223d2cc9cf801d28f42c84347fb6e8fe8167098620126ddf8a61a1994e.record.md new file mode 100644 index 0000000000..98b0809764 --- /dev/null +++ b/docs/branch-review-records/09f7a6223d2cc9cf801d28f42c84347fb6e8fe8167098620126ddf8a61a1994e.record.md @@ -0,0 +1 @@ +| 2026-08-17 | 2018 | a4338471f29c12c4f98b5abf4910aaf6f461d992 | merge-conflict resolution + review fixes for PR #2009 (docs/filter-contract.md, scripts/check-outstanding-issues.mjs, scripts/ledger-inbox.mjs, src/components/clinical-dashboard/account-setup-dialog.tsx, tests/ui-smoke.spec.ts, tests/ui-tools.spec.ts) | reviewed and fixed: resolved 6-file merge conflict against main, fixed 2 CodeRabbit findings (fingerprint case-sensitivity, URL regex boundary), left 2 findings unaddressed (fingerprint-mandatory migration risk, design-token nitpick) | check-outstanding-issues self-test, ledger-inbox self-test, check:outstanding-issues, check-ledger-write-discipline, focused vitest (outstanding-issues-writer, repo-hygiene, ledger-inbox-cancellation, favourites-auth-gate), prettier --check, typecheck, eslint | diff --git a/scripts/check-outstanding-issues.mjs b/scripts/check-outstanding-issues.mjs index 2b6e9bc232..56352940d8 100644 --- a/scripts/check-outstanding-issues.mjs +++ b/scripts/check-outstanding-issues.mjs @@ -31,6 +31,7 @@ import { execFileSync } from "node:child_process"; import { readFileSync } from "node:fs"; +import { createHash } from "node:crypto"; import { canonicalLegacyIssueId, issueIdCitations, parseIssueIdCell } from "./issue-id.mjs"; @@ -41,6 +42,7 @@ const ARCHIVE_HEADING = "## Resolved / archive"; const QUEUE_HEADING = "## Recommended execution queue"; const MARKER = //; const PRETTIER_IGNORE = ""; +const ISSUE_ROW_FINGERPRINT = /^[0-9a-f]{64}$/i; /** * A table's separator row, e.g. `| ---- | --- |`, which declares its width. * The inner pipes must be in the class: without them this only ever matched a @@ -248,6 +250,26 @@ export function parseIssues(markdown) { }; } +export function issueRowFingerprint(markdown, issueId) { + const match = String(issueId) + .trim() + .match(/^#(\d+)$/); + if (!match) return null; + const number = Number(match[1]); + if (!Number.isFinite(number)) return null; + + const row = parseIssues(markdown).rows.find( + (entry) => entry.number === number && entry.table === "open" && entry.valid && entry.raw, + ); + if (!row) return null; + const normalized = `| ${cells(row.raw).join(" | ")} |`; + return createHash("sha256").update(normalized).digest("hex"); +} + +export function isValidIssueRowFingerprint(value) { + return ISSUE_ROW_FINGERPRINT.test(String(value ?? "")); +} + export function checkIssues(markdown, { prettierIgnored = false } = {}) { const problems = []; const { openStart, archiveStart, markerCount, rows, orphans, bodyCount, queueCitations } = parseIssues(markdown); diff --git a/scripts/ledger-inbox.mjs b/scripts/ledger-inbox.mjs index 952f718bd3..4376f0a6cc 100644 --- a/scripts/ledger-inbox.mjs +++ b/scripts/ledger-inbox.mjs @@ -13,7 +13,12 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import { addIssue, resolveIssue, updateIssue } from "./outstanding-issues.mjs"; -import { ISSUES_PATH, checkIssues } from "./check-outstanding-issues.mjs"; +import { + ISSUES_PATH, + checkIssues, + issueRowFingerprint, + isValidIssueRowFingerprint, +} from "./check-outstanding-issues.mjs"; import { isIssueDisplayId, isIssueUlid, issueUlid, issueUlidFromRequest } from "./issue-id.mjs"; const ROOT = path.join(path.dirname(fileURLToPath(import.meta.url)), ".."); @@ -39,6 +44,10 @@ function requestPath(id) { return path.posix.join(INBOX_DIR, `${id}.json`); } +function readOutstandingIssues() { + return readFileSync(path.join(ROOT, ISSUES_PATH), "utf8"); +} + export function validateRequest(request) { const problems = []; if (!request || typeof request !== "object") return ["request must be an object"]; @@ -60,6 +69,11 @@ export function validateRequest(request) { if (request.action === "done") { if (!isIssueDisplayId(request.payload?.id)) problems.push("done requires a canonical issue display id"); if (!request.payload?.outcome) problems.push("done requires outcome"); + if ( + request.payload?.baseRowFingerprint !== undefined && + !isValidIssueRowFingerprint(request.payload.baseRowFingerprint) + ) + problems.push("done requires a valid baseRowFingerprint"); } if (request.action === "update") { if (!isIssueDisplayId(request.payload?.id)) problems.push("update requires a canonical issue display id"); @@ -69,6 +83,11 @@ export function validateRequest(request) { if (!["pri", "summary", "detail", "source"].some((field) => request.payload?.[field] !== undefined)) { problems.push("update requires pri, summary, detail, or source"); } + if ( + request.payload?.baseRowFingerprint !== undefined && + !isValidIssueRowFingerprint(request.payload.baseRowFingerprint) + ) + problems.push("update requires a valid baseRowFingerprint"); if (request.payload?.pri !== undefined && !["P1", "P2", "P3"].includes(String(request.payload.pri))) { problems.push("update pri must be P1, P2, or P3"); } @@ -89,6 +108,18 @@ export function applyRequest(markdown, request) { throw new Error("cancel requests must be applied through batch reconciliation"); } const options = { date: request.createdOn }; + if ((request.action === "done" || request.action === "update") && request.payload?.baseRowFingerprint) { + const id = request.payload.id; + const fingerprint = issueRowFingerprint(markdown, id); + if (!fingerprint) { + throw new Error(`${id} is no longer open; reread and reissue this request from the latest ledger`); + } + if (fingerprint !== String(request.payload.baseRowFingerprint).toLowerCase()) { + throw new Error( + `${id} is stale: the ledger row changed after this request was queued; reread and reissue from the latest ledger`, + ); + } + } if (request.action === "add") { const durableId = request.payload.issueUlid ?? issueUlidFromRequest(request.createdOn, request.id); return addIssue(markdown, request.payload, { ...options, issueUlid: durableId }); @@ -403,6 +434,13 @@ function createRequest(action, argv) { detail: argValue(argv, "detail"), source: argValue(argv, "source"), }; + if (["done", "update"].includes(action) && typeof payload.id === "string") { + const currentFingerprint = issueRowFingerprint(readOutstandingIssues(), payload.id); + if (currentFingerprint === null) { + throw new Error(`ledger request rejected: ${payload.id} is not in Open items`); + } + payload.baseRowFingerprint = currentFingerprint; + } const request = { version: 2, id: randomUUID(), createdOn: date(), action, payload }; const problems = validateRequest(request); if (problems.length > 0) throw new Error(problems.join("; ")); @@ -557,14 +595,14 @@ function selfTest() { id: "22222222-2222-4222-8222-222222222222", createdOn: "2026-08-13", action: "done", - payload: { id: "#001", outcome: "done" }, + payload: { id: "#001", outcome: "done", baseRowFingerprint: issueRowFingerprint(base, "#001") }, }; const update = { version: 1, id: "33333333-3333-4333-8333-333333333333", createdOn: "2026-08-13", action: "update", - payload: { id: "#001", summary: "updated" }, + payload: { id: "#001", summary: "updated", baseRowFingerprint: issueRowFingerprint(base, "#001") }, }; const cancel = { version: 1, @@ -581,7 +619,7 @@ function selfTest() { id: "55555555-5555-4555-8555-555555555555", createdOn: "2026-08-13", action: "update", - payload: { id: "#001", pri: "P3" }, + payload: { id: "#001", pri: "P3", baseRowFingerprint: issueRowFingerprint(base, "#001") }, }; if (validateRequest(reprioritise).length > 0) { throw new Error("self-test failed: a pri-only update request must validate"); @@ -617,6 +655,22 @@ function selfTest() { if (validateRequest({ ...add, payload: {} }).length === 0) throw new Error("self-test failed: invalid request accepted"); + const staleBase = base.replace("one", "stale"); + let staleRejected = false; + try { + applyRequest(staleBase, done); + } catch (error) { + staleRejected = /stale/.test(String(error)); + } + if (!staleRejected) throw new Error("self-test failed: stale done request was not rejected"); + let staleUpdateRejected = false; + try { + applyRequest(staleBase, update); + } catch (error) { + staleUpdateRejected = /stale/.test(String(error)); + } + if (!staleUpdateRejected) throw new Error("self-test failed: stale update request was not rejected"); + let conflictRejected = false; try { applyRequestBatch(base, [done, update]); diff --git a/tests/repo-hygiene.test.ts b/tests/repo-hygiene.test.ts index 4caf4cf47a..8b417f5070 100644 --- a/tests/repo-hygiene.test.ts +++ b/tests/repo-hygiene.test.ts @@ -36,7 +36,7 @@ import { sanitizeCell, } from "../scripts/branch-review-ledger.mjs"; import { validateLedger } from "../scripts/check-branch-review-ledger.mjs"; -import { mergeAttributeProblem } from "../scripts/check-outstanding-issues.mjs"; +import { issueRowFingerprint, mergeAttributeProblem } from "../scripts/check-outstanding-issues.mjs"; import { applyRequest, validateRequest } from "../scripts/ledger-inbox.mjs"; describe("check-env-parity name parsing", () => { @@ -683,4 +683,57 @@ describe("outstanding-issues inbox", () => { expect(applied).toContain("| #ABCDEF | P2 | issue | queued |"); expect(applied).toContain(""); }); + + it("rejects stale done/update requests when the row hash changed after queueing", () => { + const ledger = [ + "", + "", + "## Recommended execution queue", + "", + "", + "", + "| Order | ID(s) |", + "| --- | --- |", + "| 1 | `#001` |", + "", + "## Open items", + "", + "", + "", + "| ID | Pri | Type | Summary | Detail / next action | Source | Added |", + "| --- | --- | --- | --- | --- | --- | --- |", + "| #001 | P2 | issue | original summary | original detail | source | 2026-01-01 |", + "", + "## Resolved / archive", + "", + "", + "", + "| ID | Type | Summary | Outcome | Resolved |", + "| ---- | ---- | ---- | ---- | ---- |", + "| #000 | issue | old | done | 2026-01-01 |", + "", + ].join("\n"); + const baseFingerprint = issueRowFingerprint(ledger, "#001"); + expect(baseFingerprint).not.toBeNull(); + + const done = { + version: 1, + id: "77777777-7777-4777-8777-777777777777", + createdOn: "2026-08-13", + action: "done", + payload: { id: "#001", outcome: "done", baseRowFingerprint: baseFingerprint }, + }; + const update = { + version: 1, + id: "88888888-8888-4888-8888-888888888888", + createdOn: "2026-08-13", + action: "update", + payload: { id: "#001", summary: "new summary", baseRowFingerprint: baseFingerprint }, + }; + + const stale = ledger.replace("original summary", "mutated summary"); + expect(() => applyRequest(ledger, done)).not.toThrow(); + expect(() => applyRequest(stale, done)).toThrow(/stale|no longer open/); + expect(() => applyRequest(stale, update)).toThrow(/stale|no longer open/); + }); }); diff --git a/tests/ui-tools.spec.ts b/tests/ui-tools.spec.ts index 86ce537561..1259874506 100644 --- a/tests/ui-tools.spec.ts +++ b/tests/ui-tools.spec.ts @@ -1454,7 +1454,7 @@ test.describe("Clinical KB tools directory and legacy launcher", () => { await expect(page).toHaveURL(/group=urgent/); await expect(page.getByRole("button", { name: "Remove Crisis & urgent filter" })).toBeVisible(); await filterPanel.getByTestId("service-filter-panel-clear").click(); - await expect(page).toHaveURL(/q=13YARN/); + await expect(page).toHaveURL(/[?&]q=13YARN(?:&|#|$)/); await expect(page).not.toHaveURL(/group=/); await expect(page.getByTestId("service-search-result-13yarn")).toBeVisible(); await filterPanel.getByRole("button", { name: "Close", exact: true }).click();