From 68947a08e06f2b90fd45c8c6c70bb8375e83de15 Mon Sep 17 00:00:00 2001 From: pasichDev Date: Fri, 4 Sep 2026 23:16:44 +0300 Subject: [PATCH 01/18] fix: give delivery its own cursor, and close three silent data-loss paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `updatedAt` was doing two different jobs: resolving merge conflicts ("when did the author change this?") and acting as the delivery cursor ("what have I not seen yet?"). Merging copies the author's `updatedAt` onto the local record, so an item reaching B second-hand landed in B's store already timestamped in A's past, underneath A's cursor for B. With A<->B<->C paired and A<->C not, an edit made on C never reached A at all. Format v8 separates the two. `localSeq` is a per-device counter stamped on every local write — including accepting a peer's change, which is the actual fix. `updatedAt` keeps its merge-resolution job unchanged. Also fixed, each with a regression test that fails without it: - A first sync of a store larger than one page silently dropped the remainder: the payload was clamped and the cursor advanced past the gap anyway. Sync now pages, and the cursor advances only to what was actually merged. - Two processes could hold the file lock at once — both judged it stale, both removed it, and the second removed the first's brand-new lock. Reaping is now an atomic rename with a compare-and-swap on the holder record. - A lock held longer than the staleness window was reaped out from under its holder. Held locks now heartbeat, and a writer whose lock was reaped mid- operation detects it before committing and retries instead of clobbering. A property test over random topologies, operation orders and clock skew found three more, all of which lost data and none of which review would have caught: - A device with a lagging clock stamped edits and deletions BEFORE the version they replaced, so every peer discarded them and the originating device was the only one that saw its own change. - Winning a merge conflict never told the loser: our cursor had already passed their record, so both sides kept their own value forever. - The tie-break was not total — two copies with the same deviceId each refused to adopt the other. Other changes on the write path: history moves to history.json.enc, batched so an actively-worked item does not rewrite the whole log on every edit; claim renewals no longer write a history entry, which was the main source of growth. Upgrading copies the pre-migration store to todos.v7-pre-upgrade.enc, because 2.3.1 cannot read v8 but will happily write over it, stripping the new fields from every item. --- .github/workflows/ci.yml | 2 +- package.json | 5 +- src/backup.ts | 8 +- src/budget.test.ts | 192 ++++++++++++ src/export.test.ts | 3 +- src/filelock.race.test.ts | 173 +++++++++++ src/filelock.ts | 193 ++++++++++-- src/format.ts | 232 +++++++++++++++ src/history-store.test.ts | 141 +++++++++ src/history-store.ts | 114 +++++++ src/history.ts | 46 ++- src/hooks/cli.ts | 58 ++++ src/hooks/hook.failopen.test.ts | 206 +++++++++++++ src/hooks/install.test.ts | 76 +++++ src/hooks/install.ts | 328 ++++++++++++++++++++ src/hooks/session-start.ts | 63 ++++ src/index.ts | 364 ++++++++++++++-------- src/launcher.ts | 20 ++ src/mutations.test.ts | 168 +++++++++-- src/mutations.ts | 85 +++++- src/peers.ts | 17 +- src/presence.test.ts | 5 +- src/presence.ts | 9 + src/remote/client.ts | 15 +- src/repository.test.ts | 6 +- src/repository.ts | 56 +++- src/roundtrip.test.ts | 144 +++++++++ src/seq.invariant.test.ts | 280 +++++++++++++++++ src/server/routes.ts | 10 + src/sessions.test.ts | 109 +++++++ src/sessions.ts | 141 +++++++++ src/setup.ts | 31 +- src/smoke.cli.test.ts | 141 +++++++++ src/smoke.web.test.ts | 78 +++++ src/stats.ts | 32 +- src/status.test.ts | 8 +- src/status.ts | 10 + src/storage.corrupt.test.ts | 185 ++++++++++++ src/storage.migration.test.ts | 181 +++++++++++ src/storage.race.test.ts | 121 ++++++++ src/storage.ts | 256 +++++++++++++++- src/sync.convergence.property.test.ts | 251 ++++++++++++++++ src/sync.hostile.test.ts | 328 ++++++++++++++++++++ src/sync.pagination.test.ts | 356 ++++++++++++++++++++++ src/sync.test.ts | 147 ++++++++- src/sync.transitive.test.ts | 135 +++++++++ src/sync.ts | 414 +++++++++++++++++++++----- src/types.ts | 32 +- src/web/api.routes.test.ts | 186 ++++++++++++ src/web/api.ts | 90 +++++- src/web/render.escaping.test.ts | 159 ++++++++++ src/web/server.test.ts | 17 ++ src/web/server.ts | 11 +- src/web/views.ts | 173 ++++++++++- src/workspace.resolve.test.ts | 247 +++++++++++++++ src/workspace.scoping.test.ts | 70 +++++ src/workspace.ts | 257 ++++++++++++++++ 57 files changed, 6833 insertions(+), 352 deletions(-) create mode 100644 src/budget.test.ts create mode 100644 src/filelock.race.test.ts create mode 100644 src/format.ts create mode 100644 src/history-store.test.ts create mode 100644 src/history-store.ts create mode 100644 src/hooks/cli.ts create mode 100644 src/hooks/hook.failopen.test.ts create mode 100644 src/hooks/install.test.ts create mode 100644 src/hooks/install.ts create mode 100644 src/hooks/session-start.ts create mode 100644 src/roundtrip.test.ts create mode 100644 src/seq.invariant.test.ts create mode 100644 src/sessions.test.ts create mode 100644 src/sessions.ts create mode 100644 src/smoke.cli.test.ts create mode 100644 src/smoke.web.test.ts create mode 100644 src/storage.corrupt.test.ts create mode 100644 src/storage.migration.test.ts create mode 100644 src/storage.race.test.ts create mode 100644 src/sync.convergence.property.test.ts create mode 100644 src/sync.hostile.test.ts create mode 100644 src/sync.pagination.test.ts create mode 100644 src/sync.transitive.test.ts create mode 100644 src/web/api.routes.test.ts create mode 100644 src/web/render.escaping.test.ts create mode 100644 src/workspace.resolve.test.ts create mode 100644 src/workspace.scoping.test.ts create mode 100644 src/workspace.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6f3e410..b25e8a7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -16,4 +16,4 @@ jobs: node-version: "lts/*" - run: npm ci - run: npm run build - - run: node --test dist/*.test.js dist/web/*.test.js dist/server/*.test.js dist/remote/*.test.js + - run: DOCKET_DATA_DIR="$(mktemp -d)" node --test dist/*.test.js dist/web/*.test.js dist/server/*.test.js dist/remote/*.test.js dist/hooks/*.test.js diff --git a/package.json b/package.json index 5bcab4e..a545143 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,8 @@ "files": [ "dist", "!dist/**/*.test.js", - "skills" + "skills", + "CHANGELOG.md" ], "publishConfig": { "access": "public" @@ -52,7 +53,7 @@ "web": "node dist/web.js", "stats": "node dist/stats.js", "dev": "tsc --watch", - "test": "npm run build && node --test dist/*.test.js dist/web/*.test.js dist/server/*.test.js dist/remote/*.test.js", + "test": "npm run build && DOCKET_DATA_DIR=\"${DOCKET_TEST_DATA_DIR:-$(mktemp -d)}\" node --test dist/*.test.js dist/web/*.test.js dist/server/*.test.js dist/remote/*.test.js dist/hooks/*.test.js", "prepublishOnly": "npm run build" }, "dependencies": { diff --git a/src/backup.ts b/src/backup.ts index 7e2247d..5a58f24 100644 --- a/src/backup.ts +++ b/src/backup.ts @@ -2,6 +2,7 @@ import { createCipheriv, createDecipheriv, randomBytes, randomUUID, scryptSync } import { readFile, rename, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { getDataDirectory } from "./data-dir.js"; +import { resetStoreEpoch } from "./storage.js"; // Everything a fresh machine needs to become this device again: its identity (so paired // peers keep recognizing it — a NEW identity would look like a brand-new, unpaired device @@ -9,7 +10,7 @@ import { getDataDirectory } from "./data-dir.js"; // themselves. Deliberately NOT re-decrypting todos/peers first — the backup stays exactly // as sensitive as the live data directory either way, and re-encrypting a copy would be // pure extra risk (a second place a bug could leak plaintext) for no benefit. -const BACKUP_FILES = ["device.json", "key", "todos.json.enc", "peers.json.enc", "viewers.json.enc"]; +const BACKUP_FILES = ["device.json", "key", "todos.json.enc", "history.json.enc", "peers.json.enc", "viewers.json.enc"]; const MAGIC = "docket-backup-v1"; // RFC 7914's own "interactive login" recommendation (N=2^14, r=8, p=1) — strong enough to // meaningfully slow down offline password guessing against a stolen backup file, while @@ -118,5 +119,10 @@ export async function restoreBackup(buf: Buffer, password: string): Promise<{ re await rename(tmpPath, targetPath); restoredFiles.push(name); } + // The restored store is a different incarnation from the one paired devices were reading: + // its sequence counter has gone backwards, so every cursor they hold points past records + // they have never seen. Re-minting the epoch is what makes them notice and re-sync. One + // plaintext write, so this path still never decrypts anything (see the note above). + await resetStoreEpoch(); return { restoredFiles }; } diff --git a/src/budget.test.ts b/src/budget.test.ts new file mode 100644 index 0000000..f70c3f0 --- /dev/null +++ b/src/budget.test.ts @@ -0,0 +1,192 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; +import { + approximateTokens, + emptyScopeNotice, + routingHint, + compactTodo, + formatResult, + renderSessionStart, + scopeNotice, + SESSION_START_MAX_ITEMS, + SESSION_START_TOKEN_BUDGET, +} from "./format.js"; +import { ALWAYS_ON_SNIPPET } from "./hooks/install.js"; +import type { Todo } from "./types.js"; + +/** + * These are requirements, not guidelines. Every string here is paid for on every call, in + * every terminal, all day — the cost compounds in a way a one-off prompt never does, and the + * only way a budget survives contact with future edits is if something fails when it's blown. + * + * The budgets apply to the FIXED text — the wording this project chose. Variable parts (a + * project slug, an agent name) are the caller's own identifiers and have to appear in full + * or the line stops being actionable, so they get their own, more generous ceiling. + */ +const SCOPE_NOTICE_BUDGET = 15; +const ROUTING_HINT_BUDGET = 20; +const ALWAYS_ON_BUDGET = 40; +/** A realistically long slug, not a friendly one — budgets that only hold for `acme/backend` aren't budgets. */ +const LONG_WORKSPACE = "some-organisation/some-long-repository-name"; +const VARIABLE_PART_CEILING = 35; + +function todo(overrides: Partial = {}): Todo { + return { + id: 1, + uuid: "0199a000-0000-7000-8000-000000000001", + title: "fix token refresh race", + description: "a long description that a compact listing has no business printing, repeated to make the point clearly", + done: false, + list: "todo", + category: "PROJ-834", + priority: "high", + dueDate: "2026-10-01", + sourceUrl: "https://gitlab.com/acme/backend/-/issues/834", + agent: "codex", + session: "abc123", + workspace: "acme/backend", + workingAgent: null, + workingSince: null, + workingSession: null, + workingLeaseExpiresAt: null, + workingDeviceId: null, + createdAt: "2026-09-01T10:00:00.000Z", + updatedAt: "2026-09-01T10:00:00.000Z", + fieldTimestamps: {}, + completedAt: null, + revision: 1, + localSeq: 1, + deviceId: "device-a", + deviceName: "Laptop", + history: [{ at: "2026-09-01T10:00:00.000Z", agent: "codex", deviceName: "Laptop", action: "created", detail: "title: \"fix token refresh race\"" }], + ...overrides, + }; +} + +function manyTodos(count: number): Todo[] { + return Array.from({ length: count }, (_, i) => + todo({ id: i + 1, uuid: `0199a000-0000-7000-8000-${String(i + 1).padStart(12, "0")}`, title: `open item number ${i + 1}` }), + ); +} + +test("compact listing costs a fraction of a full record, and drops nothing an agent needs to act", () => { + const item = todo(); + const compact = compactTodo(item, "acme/backend"); + const verbose = formatResult([item], "open", "todo", undefined, true, "acme/backend"); + + assert.ok(compact.includes("fix token refresh race"), "the title is the point"); + assert.ok(compact.includes("[high]"), "priority changes what you pick up next"); + assert.ok(!compact.includes("a long description"), "descriptions are the bulk, and are not read when choosing"); + assert.ok( + approximateTokens(compact) * 3 < approximateTokens(verbose), + `compact (${approximateTokens(compact)}t) should be far cheaper than verbose (${approximateTokens(verbose)}t)`, + ); +}); + +test("compact listing marks a claimed item and an item from another project, and nothing else", () => { + const claimed = compactTodo( + todo({ workingAgent: "codex", workingLeaseExpiresAt: new Date(Date.now() + 60_000).toISOString() }), + "acme/backend", + ); + assert.ok(claimed.includes("← codex")); + + const foreign = compactTodo(todo({ workspace: "acme/web" }), "acme/backend"); + assert.ok(foreign.includes("@acme/web"), "an agent must not think a cross-project item landed in its own project"); + + const own = compactTodo(todo(), "acme/backend"); + assert.ok(!own.includes("@"), "and must not pay a marker for the common case"); +}); + +test(`scope notice's fixed text is under ${SCOPE_NOTICE_BUDGET} tokens, and says how to widen`, () => { + const fixed = scopeNotice(""); // the scaffolding alone, with no project name in it + assert.ok(approximateTokens(fixed) <= SCOPE_NOTICE_BUDGET, `scope notice scaffolding is ${approximateTokens(fixed)} tokens: ${fixed}`); + + const notice = scopeNotice("acme/backend"); + assert.equal(notice.trim().split("\n").length, 1); + assert.ok(notice.includes('workspace:"*"'), "an agent must be able to widen the scope without reading a doc"); + assert.equal(scopeNotice("*"), "", "nothing was narrowed, so nothing is said"); + + const long = scopeNotice(LONG_WORKSPACE); + assert.ok(approximateTokens(long) <= SCOPE_NOTICE_BUDGET + VARIABLE_PART_CEILING, `with a real slug it is ${approximateTokens(long)} tokens: ${long}`); + assert.ok(long.includes(LONG_WORKSPACE), "the project name is never abbreviated — an agent has to be able to pass it back verbatim"); +}); + +test(`routing hint is under ${ROUTING_HINT_BUDGET} tokens, and stays bounded for a long project name`, () => { + const lastSeenAt = new Date(Date.now() - 120_000).toISOString(); + const live = (workspace: string) => [ + { session: "mine", agent: "claude-code", workspace, cwd: "/r", pid: 1, startedAt: "", lastSeenAt }, + { session: "other", agent: "codex", workspace, cwd: "/r", pid: 2, startedAt: "", lastSeenAt }, + ]; + const hint = routingHint(live("acme/backend"), "acme/backend", "mine"); + assert.ok(approximateTokens(hint) <= ROUTING_HINT_BUDGET, `routing hint is ${approximateTokens(hint)} tokens: ${hint}`); + + const long = routingHint(live(LONG_WORKSPACE), LONG_WORKSPACE, "mine"); + assert.ok(approximateTokens(long) <= ROUTING_HINT_BUDGET + VARIABLE_PART_CEILING, `with a real slug it is ${approximateTokens(long)} tokens: ${long}`); +}); + +test(`SessionStart injection stays under ${SESSION_START_TOKEN_BUDGET} tokens and ${SESSION_START_MAX_ITEMS} items`, () => { + const block = renderSessionStart(manyTodos(40), "acme/backend"); + const lines = block.split("\n"); + assert.ok(approximateTokens(block) <= SESSION_START_TOKEN_BUDGET, `injection is ${approximateTokens(block)} tokens:\n${block}`); + const itemLines = lines.filter((l) => l.startsWith("T-")); + assert.ok(itemLines.length <= SESSION_START_MAX_ITEMS, `injected ${itemLines.length} items`); + assert.ok(block.includes("more"), "and says plainly that it truncated"); +}); + +test("SessionStart injection stays inside budget with a long project name too", () => { + const block = renderSessionStart(manyTodos(40), LONG_WORKSPACE); + assert.ok(approximateTokens(block) <= SESSION_START_TOKEN_BUDGET, `injection is ${approximateTokens(block)} tokens:\n${block}`); +}); + +test("SessionStart injection stays inside budget even when every title is long", () => { + const long = manyTodos(20).map((t) => ({ ...t, title: "a deliberately overlong title ".repeat(4).trim() })); + const block = renderSessionStart(long, "acme/backend"); + assert.ok(approximateTokens(block) <= SESSION_START_TOKEN_BUDGET, `injection is ${approximateTokens(block)} tokens:\n${block}`); + for (const line of block.split("\n")) assert.ok(!line.endsWith("…"), "items are dropped whole, never truncated mid-title"); +}); + +test("SessionStart injects nothing when the project has nothing open", () => { + assert.equal(renderSessionStart([], "acme/backend"), ""); + assert.equal(renderSessionStart(manyTodos(3).map((t) => ({ ...t, done: true })), "acme/backend"), ""); +}); + +test(`the always-on snippet is under ${ALWAYS_ON_BUDGET} tokens`, () => { + assert.ok( + approximateTokens(ALWAYS_ON_SNIPPET) <= ALWAYS_ON_BUDGET, + `always-on snippet is ${approximateTokens(ALWAYS_ON_SNIPPET)} tokens: ${ALWAYS_ON_SNIPPET}`, + ); +}); + +/** + * The workspace feature's scariest failure is not a bug — it is what the feature does. A + * host with an unexpected cwd resolves to a different project, the list comes back empty, + * and "my data is gone" is the honest conclusion from where the user is sitting. + */ +test("an empty scoped result reports what it is not showing, and how to see it", () => { + const all = [ + todo({ id: 1, workspace: "acme/web", title: "web work" }), + todo({ id: 2, workspace: "acme/web", title: "more web work" }), + todo({ id: 3, workspace: "acme/infra", title: "infra work" }), + ]; + const notice = emptyScopeNotice("acme/backend", all); + + assert.match(notice, /0 open in acme\/backend/); + assert.match(notice, /3 open across 2 other workspaces/); + assert.match(notice, /workspace:"\*"/, "it must name the way out, not just the problem"); + assert.equal(notice.trim().split("\n").length, 1); +}); + +test("the empty-scope notice counts only OPEN items, and ignores unfiled ones", () => { + const all = [ + todo({ id: 1, workspace: "acme/web", done: true, title: "finished" }), + todo({ id: 2, workspace: null, title: "unfiled rides along with every scope" }), + todo({ id: 3, workspace: "acme/web", title: "actually open" }), + ]; + assert.match(emptyScopeNotice("acme/backend", all), /1 open across 1 other workspace\b/); +}); + +test("a genuinely empty store says nothing extra — an empty list is then the truth", () => { + assert.equal(emptyScopeNotice("acme/backend", []), ""); + assert.equal(emptyScopeNotice("acme/backend", [todo({ id: 1, workspace: "acme/backend" })]), ""); + assert.equal(emptyScopeNotice("*", [todo({ id: 1, workspace: "acme/web" })]), "", "an unscoped list cannot mislead about scope"); +}); diff --git a/src/export.test.ts b/src/export.test.ts index 9492ae1..bd35740 100644 --- a/src/export.test.ts +++ b/src/export.test.ts @@ -6,10 +6,11 @@ import type { TodoStore } from "./types.js"; function makeStore(): TodoStore { return { - formatVersion: 5, + formatVersion: 8, nextId: 1, todos: [], deletedUuids: [], + seqCounter: 0, }; } diff --git a/src/filelock.race.test.ts b/src/filelock.race.test.ts new file mode 100644 index 0000000..945deb7 --- /dev/null +++ b/src/filelock.race.test.ts @@ -0,0 +1,173 @@ +import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; +import { once } from "node:events"; +import { mkdtemp, readFile, rm, utimes, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { fileURLToPath } from "node:url"; +import { join } from "node:path"; +import { test } from "node:test"; + +const originalDataDirectory = process.env.DOCKET_DATA_DIR; +const dataDirectory = await mkdtemp(join(tmpdir(), "docket-filelock-test-")); +process.env.DOCKET_DATA_DIR = dataDirectory; +const { LOCK_HEARTBEAT_MS, LOCK_STALE_MS, withFileLock } = await import("./filelock.js"); + +/** The compiled module the child processes import — the same one under test here. */ +const FILELOCK_MODULE = fileURLToPath(new URL("./filelock.js", import.meta.url)); + +test.after(() => { + if (originalDataDirectory === undefined) delete process.env.DOCKET_DATA_DIR; + else process.env.DOCKET_DATA_DIR = originalDataDirectory; + return rm(dataDirectory, { recursive: true, force: true }); +}); + +/** + * Two contenders, in real separate processes — not two async calls in one, which would + * never exercise the cross-process reap at all. Each writes an ENTER and an EXIT marker + * around its critical section; if the lock ever admits both, the markers interleave. + * + * They synchronise on a wall-clock start time so both land in the acquire path together. + * Without that they'd queue politely and the test would pass for the wrong reason. + */ +const CHILD_SOURCE = ` +import { appendFileSync } from "node:fs"; +import { withFileLock } from ${JSON.stringify(FILELOCK_MODULE)}; + +const [lockPath, markerPath, id, startAt] = process.argv.slice(2); +await new Promise((r) => setTimeout(r, Math.max(0, Number(startAt) - Date.now()))); + +await withFileLock(lockPath, async () => { + appendFileSync(markerPath, \`ENTER \${id}\\n\`); + await new Promise((r) => setTimeout(r, 250)); + appendFileSync(markerPath, \`EXIT \${id}\\n\`); +}); +`; + +async function runContenders(lockPath: string, markerPath: string): Promise { + const childPath = join(dataDirectory, "contender.mjs"); + await writeFile(childPath, CHILD_SOURCE, "utf8"); + const startAt = Date.now() + 400; + const children = ["a", "b"].map((id) => + spawn(process.execPath, [childPath, lockPath, markerPath, id, String(startAt)], { + stdio: ["ignore", "ignore", "pipe"], + env: { ...process.env, DOCKET_DATA_DIR: dataDirectory }, + }), + ); + const stderr = children.map((c) => { + let out = ""; + c.stderr.on("data", (chunk) => (out += chunk)); + return () => out; + }); + const codes = await Promise.all(children.map((c) => once(c, "exit"))); + codes.forEach(([code], i) => assert.equal(code, 0, `contender exited ${code}: ${stderr[i]()}`)); + return (await readFile(markerPath, "utf8")).trim().split("\n").filter(Boolean); +} + +test("withFileLock: two processes reaping the same stale lock never both enter the critical section", async () => { + const lockPath = join(dataDirectory, "race.lock"); + const markerPath = join(dataDirectory, "race.markers"); + await writeFile(markerPath, ""); + + // A crashed holder's lock, left behind and already past the staleness threshold. Both + // contenders will judge it reapable at the same moment — the exact setup where a + // delete-then-create reap lets the second one delete the FIRST one's brand-new lock. + await writeFile(lockPath, JSON.stringify({ pid: 999999, host: "gone", startedAt: new Date(0).toISOString() })); + const aged = new Date(Date.now() - LOCK_STALE_MS * 3); + await utimes(lockPath, aged, aged); + + const markers = await runContenders(lockPath, markerPath); + assert.equal(markers.length, 4, `expected two complete sections, got: ${markers.join(" | ")}`); + + let inside: string | null = null; + for (const marker of markers) { + const [event, id] = marker.split(" "); + if (event === "ENTER") { + assert.equal(inside, null, `${id} entered while ${inside} was still inside — two processes held the lock at once`); + inside = id; + } else { + assert.equal(inside, id, `${id} exited a section it never entered`); + inside = null; + } + } + assert.equal(inside, null); +}); + +test("withFileLock: a legitimately-held lock is kept fresh, so a slow operation isn't reaped out from under it", async () => { + const lockPath = join(dataDirectory, "heartbeat.lock"); + let mtimeWhileHeld = 0; + const heldFor = LOCK_HEARTBEAT_MS + 400; + + await withFileLock(lockPath, async () => { + const { stat } = await import("node:fs/promises"); + const before = (await stat(lockPath)).mtimeMs; + await new Promise((r) => setTimeout(r, heldFor)); + mtimeWhileHeld = (await stat(lockPath)).mtimeMs - before; + }); + + assert.ok( + mtimeWhileHeld > 0, + "the lock's mtime never advanced while it was held — an operation slower than LOCK_STALE_MS would have its lock reaped", + ); +}); + +test("withFileLock: a nested acquisition of the same lock fails immediately and names both call sites", async () => { + const lockPath = join(dataDirectory, "nested.lock"); + const startedAt = Date.now(); + + await assert.rejects( + () => withFileLock(lockPath, () => withFileLock(lockPath, () => "unreachable")), + (err: Error) => { + assert.match(err.message, /already held/i); + assert.match(err.message, /filelock\.race\.test/, "the message must name where the lock is held and where it was re-entered"); + return true; + }, + ); + assert.ok(Date.now() - startedAt < 1_000, "a nested acquisition must fail immediately, not sit until the acquire timeout"); +}); + +test("withFileLock: two different locks may be held at once — only re-entering the SAME one is an error", async () => { + const outer = join(dataDirectory, "outer.lock"); + const inner = join(dataDirectory, "inner.lock"); + const result = await withFileLock(outer, () => withFileLock(inner, () => "ok")); + assert.equal(result, "ok"); +}); + +test("withFileLock: a process whose lock was reaped while held does not delete the new holder's lock", async () => { + const { readFile, writeFile } = await import("node:fs/promises"); + const lockPath = join(dataDirectory, "stolen.lock"); + + await withFileLock(lockPath, async () => { + // Simulate the residual window: something reaped this lock and took it. Whatever this + // process does next must not make that worse for the process that now holds it. + await writeFile(lockPath, JSON.stringify({ id: "someone-else", pid: 1, host: "other", startedAt: new Date().toISOString() })); + }); + + const survivor = JSON.parse(await readFile(lockPath, "utf8")) as { id: string }; + assert.equal(survivor.id, "someone-else", "releasing must not delete a lock this process no longer owns"); + await rm(lockPath, { force: true }); +}); + +test("withFileLock: a lock that came back to life between judging and reaping is put back, not stolen", async () => { + const { stat, utimes, writeFile } = await import("node:fs/promises"); + const lockPath = join(dataDirectory, "revived.lock"); + + // A holder that looks stale by mtime but is genuinely alive is indistinguishable from a + // crashed one at the instant of judging — a suspended laptop resuming looks exactly like + // this. The reap must notice the refreshed mtime before it commits. + const holder = JSON.stringify({ id: "alive", pid: process.pid, host: "here", startedAt: new Date().toISOString() }); + await writeFile(lockPath, holder); + const aged = new Date(Date.now() - LOCK_STALE_MS * 3); + await utimes(lockPath, aged, aged); + + // The holder heartbeats just before a contender would rename it away. + const now = new Date(); + await utimes(lockPath, now, now); + + await assert.rejects( + () => withFileLock(lockPath, () => "should not get in"), + /timed out waiting for lock/, + "a live holder's lock must be waited for, not reaped", + ); + assert.ok((await stat(lockPath)).isFile(), "and it must still be there afterwards"); + await rm(lockPath, { force: true }); +}); diff --git a/src/filelock.ts b/src/filelock.ts index 1a76ec7..ed3c273 100644 --- a/src/filelock.ts +++ b/src/filelock.ts @@ -1,37 +1,144 @@ -import { open, rm, stat } from "node:fs/promises"; +import { AsyncLocalStorage } from "node:async_hooks"; +import { randomUUID } from "node:crypto"; +import { link, open, readFile, rename, rm, stat, utimes } from "node:fs/promises"; +import { hostname } from "node:os"; +import { log } from "./log.js"; -const LOCK_STALE_MS = 10_000; +/** A lock whose mtime hasn't moved in this long is treated as abandoned by a crashed holder. */ +export const LOCK_STALE_MS = 10_000; +/** How often a live holder refreshes its lock's mtime. A third of the staleness window, so two consecutive missed beats still don't make a live lock look dead. */ +export const LOCK_HEARTBEAT_MS = Math.floor(LOCK_STALE_MS / 3); const LOCK_RETRY_MS = 30; +const HOSTNAME = hostname(); // resolved once; it cannot change under a running process const LOCK_TIMEOUT_MS = 5_000; -async function acquireLock(lockPath: string): Promise { +/** + * Written into the lock file itself. Costs one small write per acquisition and answers the + * only question that matters the first time someone reports "docket hung": which process, + * on which machine, since when. + */ +interface LockHolder { + /** Distinguishes two acquisitions by the same pid — see reapStaleLock's compare-and-swap. */ + id: string; + pid: number; + host: string; + startedAt: string; +} + +/** + * Which locks the CURRENT async context already holds, and where it took them. Scoped per + * call chain rather than per process: two independent requests holding two different locks + * is normal, while one call chain re-entering its own lock is a deadlock waiting to happen. + * + * The value is an unformatted Error, not a string. Constructing one is cheap; `.stack` + * is what forces V8 to materialise and format the trace, and that only happens on the + * failure path — which by design never runs. + */ +const heldLocks = new AsyncLocalStorage>(); + +/** The first stack frame outside this file — i.e. whoever asked for the lock. Matched on this module's own URL rather than a filename fragment, so a caller that happens to live in a file with a similar name isn't mistaken for internal machinery. */ +function callSite(marker: Error): string { + const frames = (marker.stack ?? "").split("\n").slice(1); + const frame = frames.find((f) => f.includes("at ") && !f.includes(import.meta.url)); + return frame?.trim().replace(/^at\s+/, "") ?? "unknown call site"; +} + +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +/** + * Clears a lock whose holder appears to have died, atomically. + * + * The obvious implementation — `rm()` then retry the create — is the bug this replaces: two + * processes both judge the lock stale, both remove it, and the second one removes the FIRST + * one's brand-new lock. Both then create their own and both believe they hold it, which + * costs one of two concurrent read-modify-writes with no error anywhere. + * + * `rename` is atomic, so exactly one contender can win it. That alone is not quite enough: + * between judging the lock stale and renaming it, another process may have reaped it and + * taken a fresh one, and we would then be renaming a LIVE lock away. So the reap is a + * compare-and-swap on the holder record — if what we took isn't what we judged, we put it + * back with `link` (which, unlike rename, refuses to clobber an existing target) and let the + * caller retry. + */ +async function reapStaleLock(lockPath: string, judged: string | null): Promise { + const claimPath = `${lockPath}.reap.${randomUUID()}`; + try { + await rename(lockPath, claimPath); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== "ENOENT") throw err; + return; // lost the race — someone else reaped it; the acquire loop just tries again + } + + // Two ways the file we just took can turn out not to be the one we condemned: someone + // reaped it and took a fresh lock (different holder record), or the original holder woke + // up and started heartbeating again (same record, fresh mtime). The second is why an + // identity check alone isn't enough — a suspended laptop resuming looks identical. + const claimed = await readFile(claimPath, "utf8").catch(() => null); + const claimedMtime = await stat(claimPath).then((s) => s.mtimeMs).catch(() => 0); + if (claimed !== judged || Date.now() - claimedMtime <= LOCK_STALE_MS) { + // Put it back. `link` refuses to clobber, unlike rename, so a third process that has + // already taken the slot keeps it and we drop ours instead of overwriting theirs. + await link(claimPath, lockPath).catch(() => {}); + await rm(claimPath, { force: true }); + return; + } + await rm(claimPath, { force: true }); + log(`filelock: reaped stale lock ${lockPath} — previous holder ${claimed ?? "unknown (empty lock file)"}`); +} + +async function acquireLock(lockPath: string): Promise { const deadline = Date.now() + LOCK_TIMEOUT_MS; + const identity: LockHolder = { id: randomUUID(), pid: process.pid, host: HOSTNAME, startedAt: new Date().toISOString() }; + const serialized = JSON.stringify(identity); for (;;) { try { const handle = await open(lockPath, "wx", 0o600); - await handle.close(); - return; + try { + await handle.writeFile(serialized); + } finally { + await handle.close(); + } + return serialized; } catch (err) { if ((err as NodeJS.ErrnoException).code !== "EEXIST") throw err; - // Another process's lock — reap it if it's stale (crashed holder). + // Another process's lock — reap it if its holder looks like it crashed. + let info; + let judged: string | null = null; try { - const info = await stat(lockPath); - if (Date.now() - info.mtimeMs > LOCK_STALE_MS) { - await rm(lockPath, { force: true }); - continue; - } + judged = await readFile(lockPath, "utf8"); + info = await stat(lockPath); } catch { - continue; // lock disappeared between EEXIST and stat — retry immediately + continue; // lock disappeared between EEXIST and the read — retry immediately + } + if (Date.now() - info.mtimeMs > LOCK_STALE_MS) { + await reapStaleLock(lockPath, judged); + continue; } if (Date.now() > deadline) { - throw new Error(`docket: timed out waiting for lock at ${lockPath}`); + throw new Error(`docket: timed out waiting for lock at ${lockPath} (held by ${judged || "unknown"})`); } - await new Promise((resolve) => setTimeout(resolve, LOCK_RETRY_MS)); + await sleep(LOCK_RETRY_MS); } } } -async function releaseLock(lockPath: string): Promise { +/** + * Removes the lock ONLY if it is still ours. + * + * Without the check, a holder whose lock was reaped out from under it (see the residual + * window in withFileLock's note) would go on to delete whichever process now legitimately + * holds it — turning one bad reap into an unbounded chain of them. Comparing the holder + * record makes a stolen lock a local problem for the process that lost it, rather than + * something it inflicts on everyone after. + */ +async function releaseLock(lockPath: string, identity: string): Promise { + const current = await readFile(lockPath, "utf8").catch(() => null); + if (current !== null && current !== identity) { + log(`filelock: not releasing ${lockPath} — it is now held by someone else (this process's lock was reaped while held)`); + return; + } await rm(lockPath, { force: true }); } @@ -41,12 +148,62 @@ async function releaseLock(lockPath: string): Promise { * one docket instance per MCP host session, or a sync tick racing a human * clicking Approve — can't interleave a read-modify-write and silently drop * each other's changes. + * + * While held, the lock's mtime is refreshed on a timer: without that, anything slower than + * LOCK_STALE_MS (a laptop suspended mid-hold, a network filesystem, a debugger sitting on a + * breakpoint) has its still-valid lock reaped by the next contender. The timer is unref'd so + * a held lock can never by itself keep a process alive. + * + * What this is NOT: a distributed lock with a proof of exclusion. The reap is a + * compare-and-swap on the holder record plus a re-check of its age, which closes the window + * that actually bites (two contenders both judging one abandoned lock stale, and the second + * deleting the first's fresh one). A narrow window remains: a process suspended past + * LOCK_STALE_MS — a laptop sleeping, a SIGSTOP — cannot heartbeat, has its lock legitimately + * reaped, and wakes still inside its own critical section. No check here can help, because + * it is already in. + * + * So the lost race is not prevented, it is DETECTED, one layer up: storage.ts stamps the + * store file when it reads it and compares that stamp immediately before committing, so a + * write that lost the lock aborts and retries instead of clobbering. Ordinary optimistic + * concurrency. This is an advisory lock between cooperating processes on one machine, and a + * lost race is caught before the write rather than assumed away. + * + * Re-entering a lock this call chain already holds throws immediately instead of waiting out + * the acquire timeout. It cannot succeed either way — the process would be waiting on itself + * — but a fast, specific failure names the architecture mistake instead of surfacing five + * seconds later as a mysterious timeout. */ export async function withFileLock(lockPath: string, fn: () => T | Promise): Promise { - await acquireLock(lockPath); + const held = heldLocks.getStore(); + const heldAt = held?.get(lockPath); + const marker = new Error(); + if (heldAt !== undefined) { + throw new Error( + `docket: lock ${lockPath} is already held by this call chain (taken at ${callSite(heldAt)}, re-entered at ${callSite(marker)}). ` + + `A nested read-modify-write can never complete — do the inner work inside the outer callback, which already holds the lock.`, + ); + } + const nested = new Map(held ?? []); + nested.set(lockPath, marker); + + const identity = await acquireLock(lockPath); + const beat = setInterval(() => { + // Refresh only while the lock is still ours. Beating on a lock someone else now holds + // would keep THEIR lock alive past its own holder's death, which is the opposite of + // what a heartbeat is for. + void readFile(lockPath, "utf8") + .then((current) => { + if (current !== identity) return; + const now = new Date(); + return utimes(lockPath, now, now); + }) + .catch(() => {}); + }, LOCK_HEARTBEAT_MS); + beat.unref(); try { - return await fn(); + return await heldLocks.run(nested, async () => fn()); } finally { - await releaseLock(lockPath); + clearInterval(beat); + await releaseLock(lockPath, identity); } } diff --git a/src/format.ts b/src/format.ts new file mode 100644 index 0000000..6cfc9ed --- /dev/null +++ b/src/format.ts @@ -0,0 +1,232 @@ +import { formatAgentIdentity, isClaimActive, shortId } from "./mutations.js"; +import type { LiveSession } from "./sessions.js"; +import { summarizeWorkspaces } from "./workspace.js"; +import type { Todo, TodoList, TodoStore } from "./types.js"; + +/** + * How docket talks to an agent — every string a tool call puts in a model's context window. + * + * Kept in its own module, apart from the MCP server that uses it, for one reason: these are + * budgeted (see docs/context-budget in the README and budget.test.ts), and a budget you + * can't test is a wish. index.ts constructs an MCP server and reads device identity at + * import time, so a test that imported it to check a one-line string would start a server. + */ + +/** Catches the classic accidental-paste: description repeats the title verbatim at its start. */ +export function duplicationWarning(title: string, description: string | null): string { + if (description && description.startsWith(title)) { + return " ⚠️ description starts with the same text as title — looks like accidental duplication, not a real description."; + } + return ""; +} + +export function formatTodo(todo: Todo, currentWorkspace: string | null): string { + const box = todo.done ? "[x]" : "[ ]"; + const cat = todo.category ? ` [${todo.category}]` : ""; + const pri = todo.priority ? ` !${todo.priority}` : ""; + const due = todo.dueDate ? ` due:${todo.dueDate}` : ""; + const working = isClaimActive(todo) + ? ` ▶working:${todo.workingAgent}${todo.workingSession ? `[${todo.workingSession}]` : ""}` + : ""; + // Only shown when it ISN'T this session's own project. An id resolves globally, so an + // agent can reach an item from another workspace — it just must not be left thinking the + // item landed in the project it is standing in. + const elsewhere = todo.workspace && todo.workspace !== currentWorkspace ? ` @${todo.workspace}` : ""; + const via = todo.agent ? ` (via ${formatAgentIdentity(todo.agent, todo.deviceName)})` : ""; + const suffix = todo.done && todo.completedAt ? ` (done ${todo.completedAt.slice(0, 10)})` : ""; + const desc = todo.description ? `\n ${todo.description}` : ""; + const source = todo.sourceUrl ? `\n 🔗 ${todo.sourceUrl}` : ""; + return `${box} #${todo.id} (${shortId(todo.uuid)})${cat}${pri}${due}${working}${elsewhere} ${todo.title}${via}${suffix}${desc}${source}`; +} + +/** + * One line per item: `T-XK2P9 fix token refresh race [high] ← codex`. + * + * The default, not an option, because this output is paid for on every list call in every + * terminal all day. A full record carries description, history, timestamps, device + * provenance — none of which an agent deciding "what's open here?" reads. `verbose: true` + * is there for when it genuinely needs the rest. + */ +export function compactTodo(todo: Todo, currentWorkspace: string | null): string { + const done = todo.done ? "✓ " : ""; + const priority = todo.priority ? ` [${todo.priority}]` : ""; + const holder = isClaimActive(todo) ? ` ← ${todo.workingAgent}` : ""; + const elsewhere = todo.workspace && todo.workspace !== currentWorkspace ? ` @${todo.workspace}` : ""; + return `${done}${shortId(todo.uuid)} ${todo.title}${priority}${holder}${elsewhere}`; +} + +/** Open items first (oldest first), done items after (most recently completed first). */ +export function sortTodos(todos: Todo[]): Todo[] { + return [...todos].sort((a, b) => { + if (a.done !== b.done) return a.done ? 1 : -1; + if (a.done) return (b.completedAt ?? "").localeCompare(a.completedAt ?? ""); + return a.id - b.id; + }); +} + +function formatGroup(todos: Todo[], filter: string, verbose: boolean, currentWorkspace: string | null): string { + if (todos.length === 0) return `No ${filter === "all" ? "" : filter + " "}todos.`; + return todos.map((t) => (verbose ? formatTodo(t, currentWorkspace) : compactTodo(t, currentWorkspace))).join("\n"); +} + +/** + * One line, appended only when the results were actually narrowed. Its whole job is to stop + * an agent concluding "there is nothing open" when it is looking at one project's slice — + * and to tell it, in the same breath, exactly how to see the rest. + */ +/** + * The line that stops "scoped" from reading as "gone". + * + * This is the scariest failure of the whole workspace feature, and it isn't a bug — it's + * what the feature does. A host starts with an unexpected cwd, the project resolves to + * something else, the list comes back empty, and the honest conclusion from where the user + * is sitting is "my data is gone". Nobody reads documentation in that moment; they + * uninstall. So whenever the result is empty and the STORE is not, say both numbers and + * name the way out. + */ +export function emptyScopeNotice(scope: string, allTodos: Todo[], remedy = 'workspace:"*" for all'): string { + if (scope === "*") return ""; + const elsewhere = allTodos.filter((t) => !t.done && t.workspace !== scope && t.workspace !== null); + if (elsewhere.length === 0) return ""; // genuinely nothing open anywhere — an empty list is the truth + const workspaces = new Set(elsewhere.map((t) => t.workspace)).size; + return `\n\n_0 open in ${scope} — ${elsewhere.length} open across ${workspaces} other workspace${workspaces === 1 ? "" : "s"} (${remedy})_`; +} + +export function scopeNotice(scope: string, remedy = 'workspace:"*" for all'): string { + if (scope === "*") return ""; + return `\n\n_(scoped to ${scope} — pass ${remedy})_`; +} + +/** When both lists are in scope, render them under separate headers so todo vs backlog stays visually distinct. */ +export function formatResult( + todos: Todo[], + filter: string, + list: TodoList | "all", + pagination?: { limit?: number; offset?: number; total: number }, + verbose = false, + currentWorkspace: string | null = null, +): string { + let header = ""; + if (pagination && (pagination.limit !== undefined || pagination.offset !== undefined)) { + const offset = pagination.offset ?? 0; + const limit = pagination.limit ?? todos.length; + const start = pagination.total > 0 ? offset + 1 : 0; + const end = Math.min(offset + limit, pagination.total); + header = `_Showing ${start}-${end} of ${pagination.total} items (offset: ${offset}, limit: ${limit})_\n\n`; + } + + if (list !== "all") { + return `${header}${formatGroup(todos, filter, verbose, currentWorkspace)}`; + } + const todoItems = todos.filter((t) => t.list === "todo"); + const backlogItems = todos.filter((t) => t.list === "backlog"); + return `${header}## Todo\n${formatGroup(todoItems, filter, verbose, currentWorkspace)}\n\n## Backlog\n${formatGroup(backlogItems, filter, verbose, currentWorkspace)}`; +} + +/** + * Maximum items injected at session start. Seven is not a round number for its own sake: it + * is about as many lines as a person actually reads before scrolling, and it keeps the whole + * block inside SESSION_START_TOKEN_BUDGET even with long titles. + */ +export const SESSION_START_MAX_ITEMS = 7; + +/** + * Ceiling for the SessionStart injection, in tokens. This text is paid for at the start of + * every session in every terminal, forever, so the budget is a requirement rather than a + * guideline — budget.test.ts enforces it. Tokens are approximated at 4 characters, which + * over-counts for prose and roughly matches for identifier-heavy lines like these. + */ +export const SESSION_START_TOKEN_BUDGET = 120; + +/** Deliberately crude, and deliberately pessimistic: a budget that needs a tokenizer dependency to check is a budget nobody checks. */ +export function approximateTokens(text: string): number { + return Math.ceil(text.length / 4); +} + +/** + * What an agent sees when a session opens: what is already in flight in THIS project. + * + * With leases and blocking deferred, continuity is the hook's entire job — you come back to + * a terminal and the thread is already there. Empty output when the project has nothing + * open, because "no open items" is not worth a line at the top of every session. + */ +export function renderSessionStart(todos: Todo[], currentWorkspace: string | null): string { + const open = sortTodos(todos.filter((t) => !t.done)); + if (open.length === 0) return ""; + const heading = `Docket — open in ${currentWorkspace ?? "this session"}:`; + const lines = open.slice(0, SESSION_START_MAX_ITEMS).map((t) => compactTodo(t, currentWorkspace)); + + const build = (count: number) => { + const more = open.length > count ? `\n(+${open.length - count} more — todo_list for the rest)` : ""; + return `${heading}\n${lines.slice(0, count).join("\n")}${more}`; + }; + // Long titles can blow the budget before the item cap does. Items are dropped whole + // rather than truncated mid-word: half a title is worse than one fewer line. + for (let count = lines.length; count > 0; count--) { + const block = build(count); + if (approximateTokens(block) <= SESSION_START_TOKEN_BUDGET) return block; + } + return heading; +} + +/** "active" under a minute, then "idle 4m" / "idle 2h" — the same vocabulary presence.ts already uses. */ +export function formatIdle(lastSeenAt: string, now: number = Date.now()): string { + const ms = Math.max(0, now - Date.parse(lastSeenAt)); + if (ms < 60_000) return "active"; + const minutes = Math.floor(ms / 60_000); + return minutes < 60 ? `idle ${minutes}m` : `idle ${Math.floor(minutes / 60)}h`; +} + +/** + * One line, or nothing at all: "→ codex is live in acme/backend (idle 2m)". + * + * This is the honest version of "hand this to another agent". Pushing work into an already- + * open terminal is not possible over stdio MCP — the server cannot wake an agent, and an + * agent only acts inside a turn a human starts. What IS possible is telling you that a + * terminal already has this project open, so you switch to it instead of starting a third + * one. Anything more would be a queue wearing a router's clothes. + * + * Silent when the only session in that workspace is the caller's own: a hint about yourself + * is pure noise, and this text is paid for on every single capture. + */ +export function routingHint(sessions: LiveSession[], workspace: string | null, currentSession: string): string { + if (!workspace) return ""; // unfiled items belong to no project, so there is nowhere to point + const others = sessions.filter((s) => s.workspace === workspace && s.session !== currentSession); + if (others.length === 0) return ""; + const [first] = others; + const more = others.length > 1 ? ` +${others.length - 1} more` : ""; + return `\n→ ${first.agent ?? "another agent"} is live in ${workspace} (${formatIdle(first.lastSeenAt)})${more}`; +} + +const GREEN = "\x1b[38;2;52;211;153m"; // Todo — matches web UI's #34d399 +const VIOLET = "\x1b[38;2;167;139;250m"; // Backlog — matches web UI's #a78bfa +const AMBER = "\x1b[38;2;245;158;11m"; // in-progress — matches web UI's priority-medium #f59e0b +const DIM = "\x1b[2m"; +const RESET = "\x1b[0m"; + +/** + * The one-line terminal widget, rendered once for both entry points that show it — + * `docket stats` (via src/index.ts) and the standalone `dist/stats.js` used by shell + * prompts. They were separate copies, and had already drifted: only one of them grew the + * per-project counts. + */ +export function renderStatsWidget(store: TodoStore): string { + const open = (list: TodoList) => store.todos.filter((t) => t.list === list && !t.done).length; + let out = `${GREEN}Todo ${open("todo")}${RESET}`; + const backlogOpen = open("backlog"); + if (backlogOpen > 0) out += ` ${VIOLET}Backlog ${backlogOpen}${RESET}`; + + // With three projects feeding one list, a single total says nothing about where the work + // actually is. Shown only when there is more than one project to distinguish. + const byWorkspace = summarizeWorkspaces(store.todos); + if (byWorkspace.length > 1) { + out += `\n${byWorkspace.map(({ name, open: count }) => `${DIM}${name}${RESET} ${count}`).join(" ")}`; + } + + const working = store.todos.filter((t) => t.workingAgent && !t.done && isClaimActive(t)); + if (working.length > 0) { + const label = (t: Todo) => t.category ?? (t.title.length > 30 ? `${t.title.slice(0, 30)}…` : t.title); + out += `\n${working.map((t) => `${AMBER}▶ ${label(t)}${RESET} ${DIM}(${t.workingAgent})${RESET}`).join(", ")}`; + } + return out; +} diff --git a/src/history-store.test.ts b/src/history-store.test.ts new file mode 100644 index 0000000..bbe8fde --- /dev/null +++ b/src/history-store.test.ts @@ -0,0 +1,141 @@ +import assert from "node:assert/strict"; +import { mkdtemp, rm, stat } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; + +const originalDataDirectory = process.env.DOCKET_DATA_DIR; +const dataDirectory = await mkdtemp(join(tmpdir(), "docket-history-test-")); +process.env.DOCKET_DATA_DIR = dataDirectory; +const { HISTORY_FLUSH_THRESHOLD, HISTORY_INLINE_MAX } = await import("./history.js"); +const { fullHistoryFor, readHistoryLog } = await import("./history-store.js"); +const { LocalTodoRepository } = await import("./repository.js"); +const { withStore } = await import("./storage.js"); +const { mergeSyncPayload } = await import("./sync.js"); + +const repo = new LocalTodoRepository(); +const context = { agent: "test", session: "s1", deviceId: "d1", deviceName: "Dev" }; + +test.after(() => { + if (originalDataDirectory === undefined) delete process.env.DOCKET_DATA_DIR; + else process.env.DOCKET_DATA_DIR = originalDataDirectory; + return rm(dataDirectory, { recursive: true, force: true }); +}); + +/** Enough edits to push one item past the flush threshold, plus a couple for margin. */ +const ENOUGH_TO_FLUSH = HISTORY_FLUSH_THRESHOLD + 2; + +test("history: an item below the flush threshold never touches the side file at all", async () => { + const todo = await repo.create({ title: "short-lived" }, context); + await repo.edit(todo.id, { title: "renamed" }, context); + + const entries = await fullHistoryFor(todo.uuid, (await repo.get(todo.id))!.history); + assert.equal(entries.length, 2, "created + edited"); + await assert.rejects(() => stat(join(dataDirectory, "history.json.enc")), "no flush means no side file was written"); +}); + +test("history: flushing is batched, not per-write — the side file is rewritten rarely", async () => { + const todo = await repo.create({ title: "busy item" }, context); + for (let i = 0; i < HISTORY_FLUSH_THRESHOLD - 2; i++) await repo.edit(todo.id, { title: `edit ${i}` }, context); + + assert.equal((await readHistoryLog()).entries[todo.uuid], undefined, "well past the preview size and still not flushed"); + assert.ok( + (await repo.get(todo.id))!.history.length > HISTORY_INLINE_MAX, + "entries accumulate inline between flushes — that is what makes the rewrite amortised", + ); +}); + +test("history: past the threshold, entries move to the side file and the full log stays complete", async () => { + const todo = await repo.create({ title: "very busy item" }, context); + for (let i = 0; i < ENOUGH_TO_FLUSH; i++) await repo.edit(todo.id, { title: `edit ${i}` }, context); + + // The flush fires on the write that crosses the threshold and trims to the preview; the + // writes after it accumulate again. So the item holds the preview plus that remainder — + // bounded and small, which is the guarantee, rather than an exact number. + const stored = (await repo.get(todo.id))!; + assert.ok(stored.history.length <= HISTORY_FLUSH_THRESHOLD, `item still holds ${stored.history.length} entries inline`); + assert.ok(stored.history.length < ENOUGH_TO_FLUSH, "the bulk of the log has moved out of the store"); + + const full = await repo.history(todo.id); + assert.equal(full.length, ENOUGH_TO_FLUSH + 1, "created + every edit, none lost"); + assert.equal(full[0].action, "created", "and still in order, oldest first"); + assert.deepEqual(full.slice(-stored.history.length), stored.history, "what is inline is exactly the tail of the full log"); +}); + +test("history: repeated flushes don't duplicate entries already in the side file", async () => { + const todo = await repo.create({ title: "flushed twice" }, context); + for (let i = 0; i < ENOUGH_TO_FLUSH; i++) await repo.edit(todo.id, { title: `a${i}` }, context); + const afterFirst = await repo.history(todo.id); + for (let i = 0; i < ENOUGH_TO_FLUSH; i++) await repo.edit(todo.id, { title: `b${i}` }, context); + const afterSecond = await repo.history(todo.id); + + assert.equal(afterFirst.length, ENOUGH_TO_FLUSH + 1); + assert.equal(afterSecond.length, ENOUGH_TO_FLUSH * 2 + 1, "each flush adds exactly the new entries, never re-adds old ones"); +}); + +test("history: deleting an item drops its audit log rather than leaving it behind", async () => { + const todo = await repo.create({ title: "to be deleted" }, context); + for (let i = 0; i < ENOUGH_TO_FLUSH; i++) await repo.edit(todo.id, { title: `edit ${i}` }, context); + assert.ok((await readHistoryLog()).entries[todo.uuid], "precondition: the log exists before the delete"); + + await repo.delete(todo.id, context); + assert.equal((await readHistoryLog()).entries[todo.uuid], undefined, "an item's history must not outlive the item"); +}); + +/** + * A tombstone can coexist with a live item: a peer's deletion that LOSES to a newer local + * edit is still recorded, and the item survives. Pruning on the tombstone alone would then + * wipe the audit log of an item still sitting in the list — history loss with no deletion. + */ +test("history: a tombstone that lost to a newer edit does not take the live item's log with it", async () => { + const todo = await repo.create({ title: "resurrected" }, context); + for (let i = 0; i < ENOUGH_TO_FLUSH; i++) await repo.edit(todo.id, { title: `edit ${i}` }, context); + assert.ok((await readHistoryLog()).entries[todo.uuid], "precondition: the log was flushed"); + + // A peer deleted it, but our copy was edited after that instant, so the item stays. + await withStore((store) => { + const payload = { + todos: [], + deletedUuids: [{ uuid: todo.uuid, deletedAt: "2020-01-01T00:00:00.000Z", deviceId: "peer", localSeq: 0 }], + serverTime: new Date().toISOString(), + protocolVersion: 2, + }; + mergeSyncPayload(store, payload, "peer"); + }); + + const stillThere = await repo.get(todo.id); + assert.ok(stillThere, "precondition: the newer local edit won, so the item is still live"); + assert.ok((await readHistoryLog()).entries[todo.uuid], "a live item's audit log must survive a losing tombstone"); +}); + +/** + * A peer only sends the tail of its history. Trimming the merged result to the preview + * length would delete local entries that had never been flushed — an item below the + * threshold keeps its whole log inline and nowhere else. + */ +test("history: merging a peer's recent entries does not destroy unflushed local ones", async () => { + const todo = await repo.create({ title: "merge target" }, context); + await repo.edit(todo.id, { title: "a local edit worth keeping" }, context); + const before = (await repo.get(todo.id))!; + assert.ok(before.history.length <= HISTORY_INLINE_MAX, "precondition: nothing flushed yet"); + + const remote = { + ...structuredClone(before), + history: Array.from({ length: 6 }, (_, i) => ({ + at: new Date(Date.now() + (i + 1) * 1000).toISOString(), + agent: "peer", + deviceName: "Peer", + action: "edited" as const, + detail: `remote change ${i}`, + })), + }; + await withStore((store) => { + mergeSyncPayload(store, { todos: [remote], deletedUuids: [], serverTime: new Date().toISOString(), protocolVersion: 2 }, "peer"); + }); + + const full = await repo.history(todo.id); + const details = full.map((h) => h.detail); + assert.ok(details.includes('title: "merge target"'), "the local creation entry must survive the merge"); + assert.ok(details.some((d) => d.includes("a local edit worth keeping")), "and so must the local edit"); + assert.ok(details.includes("remote change 5"), "alongside the peer's entries"); +}); diff --git a/src/history-store.ts b/src/history-store.ts new file mode 100644 index 0000000..f0becbe --- /dev/null +++ b/src/history-store.ts @@ -0,0 +1,114 @@ +import { randomUUID } from "node:crypto"; +import { readFile, rename, writeFile } from "node:fs/promises"; +import { decryptFromBuffer, encryptToBuffer } from "./crypto.js"; +import { dedupeHistory, HISTORY_FLUSH_THRESHOLD, HISTORY_INLINE_MAX, type HistoryEntry } from "./history.js"; +import { log } from "./log.js"; +import type { TodoStore } from "./types.js"; +import { dataPath } from "./data-dir.js"; + +const HISTORY_PATH = await dataPath("history.json.enc"); + +/** The full audit log, keyed by todo uuid. Encrypted at rest exactly like the store — it describes the same work. */ +export type HistoryLog = Record; + +/** + * `readable: false` means the file exists but could not be read — a wrong key mid-restore, + * a truncated write, a permissions change. Distinguished from "not there yet" because the + * two demand opposite behaviour: an absent log is safe to create, an unreadable one must + * never be overwritten with a fresh empty object, which would turn a recoverable file into + * a permanently lost one. + */ +interface HistoryLogRead { + entries: HistoryLog; + readable: boolean; +} + +/** + * History lives beside the store rather than inside it because of what a write costs. + * Every mutation re-serialises, re-encrypts and rewrites the ENTIRE store, and history is + * the only part of a Todo that grows without bound — so an item that has been worked on + * all week made every subsequent unrelated write to any item more expensive. + * + * Read lazily and only by the two callers that want the whole log (`todo_history` and the + * web UI's detail panel); nothing on the list path touches this file at all, and the write + * path touches it only once per HISTORY_FLUSH_THRESHOLD writes to a given item. + */ +export async function readHistoryLog(): Promise { + try { + return { entries: JSON.parse(await decryptFromBuffer(await readFile(HISTORY_PATH))) as HistoryLog, readable: true }; + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "ENOENT") return { entries: {}, readable: true }; + // A corrupt or unreadable audit log must not take down the thing it audits. Losing + // history is bad; refusing to list todos because history is unreadable is worse. + log(`history: could not read ${HISTORY_PATH} (${(err as Error).message}) — continuing with inline history only`); + return { entries: {}, readable: false }; + } +} + +async function writeHistoryLog(logData: HistoryLog): Promise { + const tmpPath = `${HISTORY_PATH}.${randomUUID()}.tmp`; + await writeFile(tmpPath, await encryptToBuffer(JSON.stringify(logData)), { mode: 0o600 }); + await rename(tmpPath, HISTORY_PATH); +} + +/** + * Moves the accumulated inline history into the side file and trims each item back to the + * preview length. + * + * Called from inside withStore's lock, BEFORE the store itself is written, and the inline + * arrays are trimmed only once the side file is safely on disk. Three things follow from + * that ordering, all deliberate: + * + * - A crash between the two writes leaves the side file holding entries the store still + * has inline too, which the dedupe on the next flush absorbs. The other order would drop + * exactly the entries that had just been trimmed away. + * - A failed side-file write leaves the store untrimmed rather than trimmed-and-lost. The + * mutation itself still succeeds: history is an audit log, and failing a user's edit + * because its audit entry couldn't be filed would be the wrong trade. + * - An UNREADABLE side file is never overwritten, only skipped, so a transient decryption + * failure doesn't turn into permanent data loss. + * + * Either way history can lag the store by one write; docs/security.md §5 says so out loud. + */ +export async function flushOverflowHistory(store: TodoStore, options: { prune: boolean }): Promise { + const overflowing = store.todos.filter((t) => (t.history?.length ?? 0) > HISTORY_FLUSH_THRESHOLD); + if (overflowing.length === 0 && !options.prune) return; // the common case: no file touched, nothing allocated + + // Prune is keyed on tombstoned uuids that are ACTUALLY gone. A tombstone can coexist with + // a live item — a deletion that lost to a newer edit leaves both behind (see + // mergeSyncPayload) — and pruning on the tombstone alone would wipe the audit log of an + // item still sitting in the list. Built only when a deletion actually happened, since + // this runs inside the store lock on every write. + let orphaned: string[] = []; + if (options.prune) { + const live = new Set(store.todos.map((t) => t.uuid)); + orphaned = store.deletedUuids.filter((t) => !live.has(t.uuid)).map((t) => t.uuid); + } + if (overflowing.length === 0 && orphaned.length === 0) return; + + const { entries: logData, readable } = await readHistoryLog(); + if (!readable) return; + + for (const todo of overflowing) logData[todo.uuid] = dedupeHistory([...(logData[todo.uuid] ?? []), ...todo.history]); + // An item's audit log outliving the item would keep describing work the user asked us to + // forget. + for (const uuid of orphaned) delete logData[uuid]; + + try { + await writeHistoryLog(logData); + } catch (err) { + log(`history: could not write ${HISTORY_PATH} (${(err as Error).message}) — keeping history inline for now`); + return; // nothing trimmed, so nothing lost; the next write tries again + } + for (const todo of overflowing) todo.history = todo.history.slice(-HISTORY_INLINE_MAX); +} + +/** + * The complete log for one item: the side file plus whatever is still inline, deduped. + * Both halves are needed — an item that has never overflowed has nothing in the side file + * at all, and one that has holds its most recent entries only inline. + */ +export async function fullHistoryFor(uuid: string, inline: HistoryEntry[]): Promise { + const { entries } = await readHistoryLog(); + return dedupeHistory([...(entries[uuid] ?? []), ...(inline ?? [])]); +} diff --git a/src/history.ts b/src/history.ts index cba87e3..7d7189d 100644 --- a/src/history.ts +++ b/src/history.ts @@ -10,6 +10,44 @@ export interface HistoryEntry { detail: string; } +/** + * How many entries a card preview shows, and how many are left on the Todo after a flush. + * Enough for the preview in the web UI and for presence ("what did this agent last do?"), + * which is all any read on the hot path needs; the full log lives in history.json.enc — + * see history-store.ts. + */ +export const HISTORY_INLINE_MAX = 5; + +/** + * How many entries may pile up inline before they are moved to the side file. + * + * Deliberately much larger than the preview. Flushing is a whole-file rewrite of the audit + * log, under the store lock — doing it the moment an item exceeds the preview size would + * mean every single edit to an actively-worked item paid for it, which is the cost the + * split exists to remove, just moved to a different file. Flushing in batches amortises it + * to roughly one rewrite per this many writes, and the price is a bounded amount of inline + * history in the store instead of an unbounded one. + */ +export const HISTORY_FLUSH_THRESHOLD = 40; + +/** + * Merges history from several sources into one ordered log with no repeats. The identity of + * an entry is its whole content: entries carry no id, and the same event legitimately + * arrives twice (from the inline copy and the side file, or from two devices that both + * merged it). Comparing content is what makes those idempotent. + */ +export function dedupeHistory(entries: HistoryEntry[]): HistoryEntry[] { + const seen = new Set(); + const out: HistoryEntry[] = []; + for (const h of entries) { + const key = `${h.at}|${h.agent}|${h.action}|${h.detail}`; + if (seen.has(key)) continue; + seen.add(key); + out.push(h); + } + return out.sort((a, b) => a.at.localeCompare(b.at)); +} + function fmt(v: unknown): string { if (v === null || v === undefined || v === "") return "∅"; return String(v); @@ -33,12 +71,14 @@ export function pushHistory( item.history.push({ at: new Date().toISOString(), agent, deviceName, action, detail }); } -export function formatHistory(item: Todo): string { - if (!item.history || item.history.length === 0) return `No history for #${item.id}.`; - return item.history +/** Renders a log for the CLI/MCP. Takes the entries rather than the Todo so callers can pass the full log from history-store.ts, not just the inline preview. */ +export function formatHistoryEntries(entries: HistoryEntry[], id: number | string): string { + if (!entries || entries.length === 0) return `No history for #${id}.`; + return entries .map( (h) => `${h.at.slice(0, 19).replace("T", " ")} ${h.action} (${h.agent ?? "unknown"}${h.deviceName ? ` on ${h.deviceName}` : ""}) ${h.detail}`, ) .join("\n"); } + diff --git a/src/hooks/cli.ts b/src/hooks/cli.ts new file mode 100644 index 0000000..7381be4 --- /dev/null +++ b/src/hooks/cli.ts @@ -0,0 +1,58 @@ +/** + * `docket hook …` — dispatched from launcher.ts before index.js (and therefore the whole MCP + * stack, the device identity and the encrypted store) is ever imported. That ordering is not + * incidental: `docket hook claude session-start` runs before every Claude Code session and + * has a 20 ms budget, which importing any of that would blow on its own. + */ +export async function runHookCommand(args: string[]): Promise { + try { + await dispatch(args); + } catch (err) { + // These commands edit a file the user owns and can plausibly have hand-broken. A stack + // trace is not a diagnosis; the messages thrown below already say what to do. + console.error((err as Error).message); + process.exitCode = 1; + } +} + +async function dispatch(args: string[]): Promise { + const [first, second] = args; + + if (first === "claude" && second === "session-start") { + const { runSessionStartHook } = await import("./session-start.js"); + await runSessionStartHook(); + return; + } + + if (first === "install") { + const { runHookInstall } = await import("./install.js"); + await runHookInstall(args.slice(1)); + return; + } + if (first === "uninstall") { + const { runHookUninstall } = await import("./install.js"); + await runHookUninstall(args.slice(1)); + return; + } + if (first === "doctor") { + const { runHookDoctor } = await import("./install.js"); + await runHookDoctor(); + return; + } + + console.log(`docket hook — Claude Code SessionStart integration + +Usage: + docket hook install [--global] Add the SessionStart hook to .claude/settings.json + docket hook uninstall [--global] Remove only the entries docket owns + docket hook doctor Check config, server and measured round-trip latency + docket hook claude session-start The hook itself (run by Claude Code, not by hand) + +What it does: when a session starts, injects the items open in THIS project — compact, at +most 7, under 120 tokens — so you pick the thread back up instead of re-deriving it. Nothing +is injected when the project has nothing open. + +It fails open: if the docket web server isn't running, or anything else goes wrong, the hook +exits silently and your session is unaffected. Set DOCKET_HOOKS=off to disable it entirely +without editing any config.`); +} diff --git a/src/hooks/hook.failopen.test.ts b/src/hooks/hook.failopen.test.ts new file mode 100644 index 0000000..b89cf0e --- /dev/null +++ b/src/hooks/hook.failopen.test.ts @@ -0,0 +1,206 @@ +import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; +import { once } from "node:events"; +import { createServer, type Server } from "node:http"; +import { test } from "node:test"; +import { fileURLToPath } from "node:url"; + +/** The built entry point a host actually invokes — not the module, so process exit code and stdout are what's under test. */ +const LAUNCHER = fileURLToPath(new URL("../launcher.js", import.meta.url)); + +interface HookRun { + code: number | null; + stdout: string; + stderr: string; +} + +/** + * Runs the real hook exactly as Claude Code does: piped stdin carrying the event JSON, and + * a port to talk to. What matters is only ever the exit code and the bytes on stdout — that + * is the entire contract with the host. + */ +async function runHook(port: number, env: Record = {}): Promise { + const child = spawn(process.execPath, [LAUNCHER, "hook", "claude", "session-start"], { + stdio: ["pipe", "pipe", "pipe"], + env: { ...process.env, DOCKET_WEB_PORT: String(port), ...env }, + }); + let stdout = ""; + let stderr = ""; + child.stdout.on("data", (c) => (stdout += c)); + child.stderr.on("data", (c) => (stderr += c)); + child.stdin.end(JSON.stringify({ cwd: process.cwd(), session_id: "test", hook_event_name: "SessionStart" })); + const [code] = (await once(child, "exit")) as [number | null]; + return { code, stdout, stderr }; +} + +async function listen(server: Server): Promise { + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const address = server.address(); + assert.ok(address && typeof address !== "string"); + return address.port; +} + +/** + * Fail-open is a trust argument, not an implementation detail: a coordination tool that + * degrades your session when the tool itself is broken gets uninstalled, at which point it + * protects nobody. Every one of these must be indistinguishable from "docket isn't here". + */ +test("hook: server not running — exits 0, says nothing", async () => { + // Port 1 on loopback: nothing can be listening, and the connection is refused immediately. + const result = await runHook(1); + assert.equal(result.code, 0); + assert.equal(result.stdout, ""); + assert.equal(result.stderr, ""); +}); + +test("hook: server accepts the connection and never answers — exits 0 within its timeout", async () => { + const server = createServer(() => { + // Deliberately no response, ever: the hard timeout is what has to save the session. + }); + const port = await listen(server); + try { + const started = Date.now(); + const result = await runHook(port); + assert.equal(result.code, 0); + assert.equal(result.stdout, ""); + assert.ok(Date.now() - started < 5_000, "a hung server must not hold up the session"); + } finally { + server.close(); + } +}); + +test("hook: server returns garbage — exits 0, says nothing", async () => { + const server = createServer((_req, res) => { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end("not json at all"); + }); + const port = await listen(server); + try { + const result = await runHook(port); + assert.equal(result.code, 0); + assert.equal(result.stdout, ""); + } finally { + server.close(); + } +}); + +test("hook: server returns a 500 — exits 0, says nothing", async () => { + const server = createServer((_req, res) => { + res.writeHead(500, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ error: "boom" })); + }); + const port = await listen(server); + try { + const result = await runHook(port); + assert.equal(result.code, 0); + assert.equal(result.stdout, ""); + } finally { + server.close(); + } +}); + +test("hook: DOCKET_HOOKS=off disables it without touching any config", async () => { + const server = createServer((_req, res) => { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ text: "Docket — open in acme/backend:\nT-AAAAAA something" })); + }); + const port = await listen(server); + try { + const result = await runHook(port, { DOCKET_HOOKS: "off" }); + assert.equal(result.code, 0); + assert.equal(result.stdout, "", "the escape hatch has to work without editing settings.json"); + } finally { + server.close(); + } +}); + +test("hook: a healthy server's text is what reaches the session, and nothing else", async () => { + const injected = "Docket — open in acme/backend:\nT-AAAAAA fix the thing [high]"; + const server = createServer((req, res) => { + assert.ok(req.url?.startsWith("/api/hook/session-start"), `unexpected request path: ${req.url}`); + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ text: injected })); + }); + const port = await listen(server); + try { + const result = await runHook(port); + assert.equal(result.code, 0); + assert.equal(result.stdout, `${injected}\n`); + assert.equal(result.stderr, ""); + } finally { + server.close(); + } +}); + +test("hook: an empty list injects nothing at all", async () => { + const server = createServer((_req, res) => { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ text: "" })); + }); + const port = await listen(server); + try { + const result = await runHook(port); + assert.equal(result.stdout, "", "\"nothing open\" is not worth a line at the top of every session"); + } finally { + server.close(); + } +}); + +test("hook: DOCKET_HOOKS=off makes no request at all, not just a silent one", async () => { + // The difference matters: "off" has to mean the hook costs nothing, not that it still + // wakes the server and throws the answer away on every session start. + let requests = 0; + const server = createServer((_req, res) => { + requests += 1; + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ text: "should never be asked for" })); + }); + const port = await listen(server); + try { + const result = await runHook(port, { DOCKET_HOOKS: "off" }); + assert.equal(result.code, 0); + assert.equal(result.stdout, ""); + assert.equal(requests, 0, "the hook contacted the server despite being turned off"); + } finally { + server.close(); + } +}); + +test("hook: a warm round trip against a live server stays well inside the session-start budget", async () => { + const server = createServer((_req, res) => { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ text: "Docket — open in acme/backend:\nT-AAAAAA something" })); + }); + const port = await listen(server); + try { + await runHook(port); // warm the process cache so this measures the hook, not the first import + const started = Date.now(); + const result = await runHook(port); + const elapsed = Date.now() - started; + assert.equal(result.code, 0); + // Generous on purpose: most of this is Node's own startup, which no command hook can + // avoid, and a CI box is slower than a laptop. It fails only if something genuinely + // pathological creeps onto the path — a store decrypt, an MCP import, a retry loop. + assert.ok(elapsed < 1_500, `a session start would wait ${elapsed}ms for the hook`); + } finally { + server.close(); + } +}); + +test("hook: a server that answers with the wrong JSON shape is treated as no answer", async () => { + for (const body of ['{"text":123}', '{"text":null}', '{"nope":"x"}', "[]", "null"]) { + const server = createServer((_req, res) => { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(body); + }); + const port = await listen(server); + try { + const result = await runHook(port); + assert.equal(result.code, 0, `body ${body} produced exit ${result.code}`); + assert.equal(result.stdout, "", `body ${body} put something in the session`); + } finally { + server.close(); + } + } +}); diff --git a/src/hooks/install.test.ts b/src/hooks/install.test.ts new file mode 100644 index 0000000..47d6863 --- /dev/null +++ b/src/hooks/install.test.ts @@ -0,0 +1,76 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; +import { addSessionStartHook, diffLines, removeDocketHooks, sessionStartCommand } from "./install.js"; + +const COMMAND = "docket hook claude session-start"; + +/** A settings.json that already has the user's own hooks in it — the only case that really matters. */ +function userSettings() { + return { + permissions: { allow: ["Bash(npm test)"] }, + hooks: { + SessionStart: [{ hooks: [{ type: "command", command: "my-own-setup.sh" }] }], + PreToolUse: [{ matcher: "Bash", hooks: [{ type: "command", command: "audit-bash.sh" }] }], + }, + }; +} + +test("install merges into existing hooks instead of replacing them", () => { + const next = addSessionStartHook(userSettings(), COMMAND); + const commands = next.hooks!.SessionStart.flatMap((m) => (m.hooks ?? []).map((h) => h.command)); + assert.deepEqual(commands, ["my-own-setup.sh", "docket hook claude session-start"]); + assert.deepEqual(next.hooks!.PreToolUse, userSettings().hooks.PreToolUse, "unrelated events are untouched"); + assert.deepEqual(next.permissions, userSettings().permissions, "and so is everything outside `hooks`"); +}); + +test("install into an empty settings file creates just the one entry", () => { + const next = addSessionStartHook({}, COMMAND); + assert.deepEqual(next.hooks, { SessionStart: [{ hooks: [{ type: "command", command: "docket hook claude session-start" }] }] }); +}); + +test("install is idempotent — running it twice does not stack duplicates", () => { + const once = addSessionStartHook(userSettings(), COMMAND); + const twice = addSessionStartHook(once, COMMAND); + assert.deepEqual(twice, once, "an upgrade path that duplicates entries silently doubles the hook's cost"); +}); + +test("uninstall removes only entries docket owns", () => { + const installed = addSessionStartHook(userSettings(), COMMAND); + const cleaned = removeDocketHooks(installed); + assert.deepEqual(cleaned.hooks, userSettings().hooks, "the file is left exactly as the user had it"); +}); + +test("uninstall drops the hooks key entirely when docket's entry was the only one", () => { + const cleaned = removeDocketHooks(addSessionStartHook({}, COMMAND)); + assert.equal(cleaned.hooks, undefined, "no empty scaffolding left behind"); +}); + +test("uninstall on a file with no docket hooks changes nothing", () => { + assert.deepEqual(removeDocketHooks(userSettings()), userSettings()); +}); + +test("the diff shown before writing names the command, and nothing structural", () => { + const before = JSON.stringify(userSettings(), null, 2); + const after = JSON.stringify(addSessionStartHook(userSettings(), COMMAND), null, 2); + const diff = diffLines(before, after); + assert.deepEqual(diff.split("\n"), ['+ "command": "docket hook claude session-start"']); +}); + +test("the installed command is one a shell can actually run, or is refused outright", async () => { + const { command, onPath, reason } = await sessionStartCommand(); + if (onPath) { + assert.equal(command, COMMAND); + return; + } + if (command === null) { + // Running from npm's npx cache: pinning that path would write a command into long-lived + // config that npm may delete, so installing is refused with an actionable reason rather + // than producing a hook that works today and rots. + assert.match(reason ?? "", /npm install -g/); + return; + } + // A stable local install (a clone, or a non-global prefix): pin the interpreter and + // launcher absolutely, or the hook silently never runs. + assert.ok(command.startsWith(`"${process.execPath}"`), `expected an absolute command, got: ${command}`); + assert.match(command, /launcher\.js/); +}); diff --git a/src/hooks/install.ts b/src/hooks/install.ts new file mode 100644 index 0000000..3f084e0 --- /dev/null +++ b/src/hooks/install.ts @@ -0,0 +1,328 @@ +import { constants } from "node:fs"; +import { access, mkdir, readFile, writeFile } from "node:fs/promises"; +import { homedir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { createLineReader } from "../cli-prompt.js"; + +/** + * How an entry is recognised as ours — and nothing else is ever touched. + * + * It has to be the ARGUMENTS, not the word "docket": the command is written either as + * `docket hook claude session-start` or, when docket isn't on PATH, as an absolute + * interpreter + launcher path that contains no such word. Matching the executable would + * mean `uninstall` and `doctor` couldn't recognise entries `install` had just written. + */ +const OWNED_COMMAND_MARKER = "hook claude session-start"; + +/** + * Above this, the hook is worth turning off rather than tolerating. + * + * The budget that matters is the REQUEST — a loopback call to an already-running server, + * which lands in single-digit milliseconds. What `doctor` measures is the whole command, + * and most of that is Node's own process startup, which no hook implemented as a command + * can avoid. So this threshold is set where a person actually notices a session pausing, + * not at the request budget, and the point of reporting it is to make the escape hatch + * findable at the moment someone is wondering why sessions feel slow. + */ +const HOOK_SLOW_MS = 250; + +/** + * The command written into settings.json. + * + * `docket hook …` only works if `docket` is actually on PATH, and the documented quick + * start installs via `npx`, which puts nothing there. A hook whose executable doesn't + * exist fails open in the sense that nothing breaks — and never works, silently, which is + * the failure this tool is least able to notice about itself. So: use the short form when + * it will resolve, and otherwise pin the interpreter and this launcher by absolute path. + * + * The absolute form is the more fragile of the two if the install later moves, which is + * why it isn't the default and why `install` says out loud when it had to use it. + */ +export async function sessionStartCommand(): Promise<{ command: string | null; onPath: boolean; reason?: string }> { + const launcher = fileURLToPath(new URL("../launcher.js", import.meta.url)); + + // Checked BEFORE the PATH lookup, deliberately. `npx` puts the package's binaries on PATH + // for the duration of its own invocation, so `docket` resolves here and then does not + // exist when Claude Code actually runs the hook — and npm is free to evict the cache + // anyway. Either way we would be writing a command into long-lived user config whose + // lifetime is shorter than the config's, and the failure mode is a hook that quietly + // stops firing: exactly what this tool is least able to notice about itself. + if (/[\\/]_npx[\\/]/.test(launcher)) { + return { + command: null, + onPath: false, + reason: + "this copy of docket is running from npm's npx cache, which exists only for this command.\n" + + "Install it properly first, then run this again:\n\n npm install -g @pasichdev/docket", + }; + } + + if (await isOnPath("docket")) return { command: `docket ${OWNED_COMMAND_MARKER}`, onPath: true }; + return { command: `"${process.execPath}" "${launcher}" ${OWNED_COMMAND_MARKER}`, onPath: false }; +} + +/** Deliberately a PATH scan rather than shelling out to `which`/`where` — one fewer subprocess, and it behaves the same on both. */ +export async function isOnPath(name: string): Promise { + const entries = (process.env.PATH ?? "").split(process.platform === "win32" ? ";" : ":").filter(Boolean); + const candidates = process.platform === "win32" ? [`${name}.cmd`, `${name}.exe`, name] : [name]; + for (const dir of entries) { + for (const candidate of candidates) { + try { + await access(join(dir, candidate), constants.X_OK); + return true; + } catch { + // Not here; keep looking. + } + } + } + return false; +} + +/** + * The always-on snippet `install` offers to append to CLAUDE.md / AGENTS.md. + * + * Under 40 tokens, and that ceiling is the point: this text is in context for every turn of + * every session, so it buys its place only by being shorter than the confusion it prevents. + * Everything else — fields, workspace scoping, the claim workflow — lives in the skill, which + * loads only when an agent actually reaches for the tools. + */ +export const ALWAYS_ON_SNIPPET = + "Docket is one shared list across your tools and projects. Capture anything worth not " + + "forgetting — it files under the current project automatically."; + +interface HookEntry { + type?: string; + command?: string; +} +interface HookMatcher { + matcher?: string; + hooks?: HookEntry[]; +} +interface ClaudeSettings { + hooks?: Record; + [key: string]: unknown; +} + +export function settingsPath(scope: "project" | "global", cwd: string = process.cwd()): string { + return scope === "global" ? join(homedir(), ".claude", "settings.json") : join(cwd, ".claude", "settings.json"); +} + +async function readSettings(path: string): Promise<{ settings: ClaudeSettings; existed: boolean }> { + try { + return { settings: JSON.parse(await readFile(path, "utf8")) as ClaudeSettings, existed: true }; + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "ENOENT") return { settings: {}, existed: false }; + // Refusing to touch a file we can't parse is the only safe move: this file holds the + // user's OWN hooks, and rewriting it from a partial understanding would eat them. + throw new Error(`docket: ${path} exists but isn't valid JSON — fix or move it, then run this again.`); + } +} + +function ownsEntry(entry: HookEntry): boolean { + return typeof entry.command === "string" && entry.command.includes(OWNED_COMMAND_MARKER); +} + +/** + * Merges the SessionStart hook into whatever is already there. + * + * Never overwrites: people already have their own hooks in this file, and an installer that + * replaces the block is an installer that silently deletes someone's work. Idempotent for + * the same reason — running it twice, or after an upgrade, must not stack duplicate entries. + */ +export function addSessionStartHook(settings: ClaudeSettings, command: string): ClaudeSettings { + const next: ClaudeSettings = { ...settings, hooks: { ...(settings.hooks ?? {}) } }; + const existing = next.hooks!.SessionStart ?? []; + if (existing.some((matcher) => (matcher.hooks ?? []).some(ownsEntry))) return next; // already ours + next.hooks!.SessionStart = [...existing, { hooks: [{ type: "command", command }] }]; + return next; +} + +/** Removes only entries this tool owns, leaving every other hook — and every other event — exactly as it was. */ +export function removeDocketHooks(settings: ClaudeSettings): ClaudeSettings { + if (!settings.hooks) return settings; + const hooks: Record = {}; + for (const [event, matchers] of Object.entries(settings.hooks)) { + const kept = matchers + .map((matcher) => ({ ...matcher, hooks: (matcher.hooks ?? []).filter((entry) => !ownsEntry(entry)) })) + .filter((matcher) => (matcher.hooks ?? []).length > 0); + if (kept.length > 0) hooks[event] = kept; + } + const next: ClaudeSettings = { ...settings, hooks }; + if (Object.keys(hooks).length === 0) delete next.hooks; + return next; +} + +/** + * A minimal added/removed diff of the two serialisations — enough to answer the only + * question that matters before writing to someone's config: what is about to change? + * + * Structural punctuation is dropped, because a naive line diff of reindented JSON reports + * a stray `},` as a change and buries the one line the reader actually needs to see. + */ +export function diffLines(before: string, after: string): string { + const meaningful = (line: string) => /[A-Za-z0-9]/.test(line); + const beforeLines = new Set(before.split("\n")); + const afterLines = new Set(after.split("\n")); + const added = after.split("\n").filter((l) => !beforeLines.has(l) && meaningful(l)); + const removed = before.split("\n").filter((l) => !afterLines.has(l) && meaningful(l)); + return [...removed.map((l) => `- ${l.trim()}`), ...added.map((l) => `+ ${l.trim()}`)].join("\n"); +} + +async function writeSettings(path: string, settings: ClaudeSettings): Promise { + await mkdir(dirname(path), { recursive: true }); + await writeFile(path, `${JSON.stringify(settings, null, 2)}\n`, "utf8"); +} + +/** + * The shared shape of both commands: read, transform, show what would change, ask, write. + * They were two copies and had already drifted on one message; the only genuine differences + * are the transform and what is printed afterwards. + */ +async function editSettings( + args: string[], + step: { + transform: (settings: ClaudeSettings) => ClaudeSettings; + requireExisting?: boolean; + unchanged: string; + done: (path: string) => void; + }, +): Promise { + const path = settingsPath(args.includes("--global") ? "global" : "project"); + const { settings, existed } = await readSettings(path); + if (step.requireExisting && !existed) { + console.log(`No ${path} — nothing to uninstall.`); + return; + } + + const before = JSON.stringify(settings, null, 2); + const after = JSON.stringify(step.transform(settings), null, 2); + if (before === after) { + console.log(`${step.unchanged} in ${path} — nothing to change.`); + return; + } + + console.log(`${existed ? "Updating" : "Creating"} ${path}:\n`); + console.log(diffLines(before, after)); + console.log(""); + const reader = createLineReader(); + let approved: boolean; + try { + approved = await reader.askYesNo("Write this?", false); + } finally { + reader.close(); + } + if (!approved) { + console.log("Nothing written."); + return; + } + await writeSettings(path, JSON.parse(after) as ClaudeSettings); + step.done(path); +} + +export async function runHookInstall(args: string[]): Promise { + const { command, onPath, reason } = await sessionStartCommand(); + if (!command) { + console.error(`Can't install a durable hook: ${reason}`); + process.exitCode = 1; + return; + } + await editSettings(args, { + transform: (settings) => addSessionStartHook(settings, command), + unchanged: "Already installed", + done: () => { + console.log(`Installed. Claude Code will run \`${command}\` when a session starts in this project.`); + if (!onPath) { + console.log(""); + console.log("Note: `docket` isn't on your PATH, so the hook is pinned to this exact install."); + console.log(" Run `npm install -g @pasichdev/docket` and re-run this to use the short, portable form."); + } + console.log(""); + console.log("Optionally add this to your CLAUDE.md / AGENTS.md so agents know the list exists:\n"); + console.log(` ${ALWAYS_ON_SNIPPET}`); + }, + }); +} + +export async function runHookUninstall(args: string[]): Promise { + await editSettings(args, { + transform: removeDocketHooks, + requireExisting: true, + unchanged: "No docket hooks", + done: () => console.log("Removed docket's hook entries. Your other hooks were left alone."), + }); +} + +/** + * Proves the hook actually fires end to end. + * + * It runs the command as it is written in settings.json, through a shell, exactly as Claude + * Code would — not the hook function in-process. That distinction is the whole point: the + * single most likely thing to be wrong is that the configured executable isn't on PATH, and + * an in-process check is structurally incapable of noticing it. + */ +export async function runHookDoctor(): Promise { + const { resolveWorkspace } = await import("../workspace.js"); + const { spawn } = await import("node:child_process"); + + let configured: string | null = null; + for (const scope of ["project", "global"] as const) { + const path = settingsPath(scope); + const { settings, existed } = await readSettings(path).catch(() => ({ settings: {} as ClaudeSettings, existed: false })); + const owned = Object.values(settings.hooks ?? {}) + .flat() + .flatMap((m) => m.hooks ?? []) + .find(ownsEntry); + configured ??= owned?.command ?? null; + console.log(`${scope.padEnd(8)} ${path}: ${!existed ? "no settings file" : owned ? "hook installed" : "no docket hook"}`); + } + + const { workspace, source } = await resolveWorkspace(process.cwd()); + console.log(`workspace ${workspace ?? "(unfiled)"} — resolved via ${source}`); + + if (!configured) { + const { command, reason } = await sessionStartCommand(); + console.log(command ? `\nNot installed here. \`docket hook install\` would add: ${command}` : `\nNot installed here, and cannot be: ${reason}`); + return; + } + if (process.env.DOCKET_HOOKS === "off") { + console.log("DOCKET_HOOKS=off — the hook will exit silently. Unset it to re-enable."); + return; + } + + const started = Date.now(); + const result = await new Promise<{ code: number | null; stdout: string; stderr: string }>((resolve) => { + const child = spawn(configured!, { shell: true, stdio: ["pipe", "pipe", "pipe"] }); + let stdout = ""; + let stderr = ""; + child.stdout.on("data", (c) => (stdout += c)); + child.stderr.on("data", (c) => (stderr += c)); + child.on("error", (err) => resolve({ code: null, stdout: "", stderr: err.message })); + child.on("close", (code) => resolve({ code, stdout, stderr })); + // A command that can't start never reads its stdin, and the write then raises EPIPE on + // an unhandled 'error' event — which would crash the very tool whose job is to report + // that failure calmly. Diagnosing a broken hook must not itself be a broken experience. + child.stdin.on("error", () => {}); + child.stdin.end(JSON.stringify({ cwd: process.cwd(), session_id: "doctor", hook_event_name: "SessionStart" })); + }); + const elapsed = Date.now() - started; + + if (result.code !== 0) { + console.log(`\ncommand ${configured}`); + console.log(`FAILED exit ${result.code ?? "could not start"} after ${elapsed}ms${result.stderr ? ` — ${result.stderr.trim()}` : ""}`); + console.log(" Claude Code would see this as a broken hook. Re-run `docket hook install` to repair the command."); + return; + } + console.log(`command ${configured}`); + console.log(`ran in ${elapsed}ms (includes Node startup, which any command hook pays)`); + if (elapsed > HOOK_SLOW_MS) { + console.log(`SLOW over ${HOOK_SLOW_MS}ms — you would feel this at the start of every session.`); + console.log(` Turn it off without editing any config: export DOCKET_HOOKS=off`); + console.log(` (the hook then exits silently; nothing else about docket changes)`); + } + if (!result.stdout.trim()) { + console.log("output none — either nothing is open in this project, or the docket web server isn't running (`docket web`)."); + return; + } + console.log(`\nWhat a session would see:\n${result.stdout.trimEnd()}`); +} diff --git a/src/hooks/session-start.ts b/src/hooks/session-start.ts new file mode 100644 index 0000000..9d7c588 --- /dev/null +++ b/src/hooks/session-start.ts @@ -0,0 +1,63 @@ +import { resolveWorkspace } from "../workspace.js"; + +/** + * How long the hook will wait for the local server before giving up and staying quiet. + * + * This runs before every Claude Code session. The normal case — a directory walk to resolve + * the project plus one loopback request to an already-running process — measures in the low + * tens of milliseconds (`docket hook doctor` reports the real number on your machine). The + * ceiling exists for the abnormal case: a server mid-restart, a machine under load, where a + * hook that hangs is far worse than a hook that says nothing. + */ +const HOOK_TIMEOUT_MS = 150; + +const DEFAULT_PORT = 8787; + +/** Whatever Claude Code puts on the hook's stdin. Only `cwd` is load-bearing; the rest is ignored on purpose. */ +interface ClaudeHookEvent { + cwd?: string; + session_id?: string; + hook_event_name?: string; +} + +async function readStdin(): Promise { + if (process.stdin.isTTY) return ""; // invoked by hand from a terminal, not by a host + const chunks: Buffer[] = []; + for await (const chunk of process.stdin) chunks.push(chunk as Buffer); + return Buffer.concat(chunks).toString("utf8"); +} + +/** + * `docket hook claude session-start`. + * + * A thin HTTP client, deliberately: it must never load the MCP stack, resolve the data + * directory, or decrypt the store. Those cost tens of milliseconds and real I/O on a path + * that runs before every session, and a hook that makes sessions feel slower gets deleted + * within a day — at which point it protects nobody. + * + * Fails open, always. Server not running, timed out, malformed response, `DOCKET_HOOKS=off` + * — every one of them exits 0 having printed nothing. A tool that degrades your session + * when the tool itself is broken is worse than no tool. + */ +export async function runSessionStartHook(): Promise { + if (process.env.DOCKET_HOOKS === "off") return; + try { + const raw = await readStdin(); + const event = (raw ? JSON.parse(raw) : {}) as ClaudeHookEvent; + const cwd = typeof event.cwd === "string" && event.cwd ? event.cwd : process.cwd(); + + // Resolved in-process: it is a short filesystem walk plus two small reads, with no + // subprocess and no `git` on the PATH required. Asking the server to resolve it instead + // would mean sending it a path and trusting it to have the same view of the disk. + const { workspace } = await resolveWorkspace(cwd); + + const port = Number(process.env.DOCKET_WEB_PORT ?? DEFAULT_PORT); + const url = `http://127.0.0.1:${port}/api/hook/session-start${workspace ? `?workspace=${encodeURIComponent(workspace)}` : ""}`; + const res = await fetch(url, { signal: AbortSignal.timeout(HOOK_TIMEOUT_MS) }); + if (!res.ok) return; + const body = (await res.json()) as { text?: unknown }; + if (typeof body.text === "string" && body.text.trim()) process.stdout.write(`${body.text}\n`); + } catch { + // Every failure mode lands here and is silent by design — see the fail-open note above. + } +} diff --git a/src/index.ts b/src/index.ts index b10f72c..99b7801 100644 --- a/src/index.ts +++ b/src/index.ts @@ -6,22 +6,25 @@ import { createInterface } from "node:readline/promises"; import { fileURLToPath } from "node:url"; import { McpServer } from "@modelcontextprotocol/sdk/server/mcp.js"; import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js"; +import { RootsListChangedNotificationSchema } from "@modelcontextprotocol/sdk/types.js"; import { z } from "zod"; import { createBackup, isBackupFile, restoreBackup } from "./backup.js"; import { askQuestions as cliAskQuestions } from "./cli-prompt.js"; import { DeploymentConfigError, resolveDeploymentConfig } from "./config.js"; import { getDeviceId, getDeviceName } from "./device.js"; import { exportToJson, exportToMarkdown, importFromJson, importFromMarkdown } from "./export.js"; -import { formatHistory } from "./history.js"; +import { formatHistoryEntries } from "./history.js"; import { installProcessLogging, log } from "./log.js"; -import { formatAgentIdentity, isClaimActive, shortId } from "./mutations.js"; +import { duplicationWarning, emptyScopeNotice, formatIdle, formatResult, formatTodo, renderStatsWidget, routingHint, scopeNotice, sortTodos } from "./format.js"; import { RemoteProtocolError, RemoteTodoRepository, RemoteUnavailableError } from "./remote/client.js"; import { loadRemoteCredentials } from "./remote/credentials.js"; -import type { MutationContext } from "./repository.js"; -import { CURRENT_FORMAT_VERSION, migrateLegacyFields, readStore, withStore } from "./storage.js"; +import { filterTodos, type MutationContext } from "./repository.js"; +import { CURRENT_FORMAT_VERSION, LAST_V7_RELEASE, migrateLegacyFields, readStore, restorePreUpgradeStore, withStore } from "./storage.js"; import { TodoService, todoService as localTodoService } from "./todo-service.js"; import type { Todo, TodoList } from "./types.js"; import { checkForUpdate, getCurrentVersion, runUpdate } from "./update.js"; +import { endSession, listSessions, registerSession, touchSession } from "./sessions.js"; +import { currentWorkspace, setWorkspaceRoot, slugifyWorkspace, summarizeWorkspaces } from "./workspace.js"; const SCRIPT_PATH = fileURLToPath(import.meta.url); @@ -140,8 +143,86 @@ function currentAgent(): string | null { return server.server.getClientVersion()?.name ?? null; } +/** + * The project this session is working in. Resolved once (see workspace.ts) and logged at + * startup: a mis-resolution has to be VISIBLE, because the symptom otherwise is "my items + * aren't showing up" with nothing to point at. + */ +let workspace: string | null = null; + +async function refreshWorkspace(reason: string): Promise { + const resolved = await currentWorkspace(); + workspace = resolved.workspace; + log(`workspace: ${resolved.workspace ?? "(unfiled)"} via ${resolved.source}${resolved.root ? ` at ${resolved.root}` : ""} — ${reason}`); +} + +/** Every field is a module-level value, so the call takes no arguments — and adding a field to LiveSession stays a one-line change instead of a three-call-site hunt. */ +function registerThisSession(): Promise { + return registerSession({ session: sessionToken, agent: currentAgent(), workspace, cwd: process.cwd(), pid: process.pid }); +} + function currentContext(): MutationContext { - return { agent: currentAgent(), session: sessionToken, deviceId, deviceName }; + return { agent: currentAgent(), session: sessionToken, deviceId, deviceName, workspace }; +} + +/** + * Runs once the client has introduced itself. + * + * Two things are only knowable at this point, and both are recorded at startup with + * placeholder values so a session that never finishes initialising is still visible: + * + * - The agent's NAME. `clientInfo` arrives with the initialize request, so registering + * before it lands means every session shows up as "unknown" — which makes the whole + * presence panel useless for the one question it exists to answer. + * - The host's ROOTS, when it offers that capability. process.cwd() is usually the project + * directory, but nothing guarantees it; the host's own idea wins where there is one. + * A host that offers no roots capability simply keeps the cwd answer. + */ +/** Re-reads the host's roots and re-files this session under whatever project it is now in. */ +async function adoptHostRoots(): Promise { + try { + const { roots } = await server.server.listRoots(); + const first = roots.find((r) => r.uri.startsWith("file://")); + if (!first) return false; + setWorkspaceRoot(fileURLToPath(first.uri)); + return true; + } catch (err) { + // A host that advertises roots but fails the call is not a reason to fail the session. + log(`workspace: host advertised roots but listRoots failed (${(err as Error).message}) — keeping the cwd-derived workspace`); + return false; + } +} + +async function onClientReady(): Promise { + // The `initialized` notification can be DISPATCHED before the `initialize` request's + // handler has run: the SDK schedules request handlers on a microtask, and a host that + // writes both messages before this process finished starting has them arrive in one + // stdin chunk. Reading clientInfo synchronously here therefore sees null, and every + // session shows up as "unknown". Yielding once lets that microtask settle first. + // + // This is an optimisation, not the guarantee — it assumes one macrotask is enough, which + // is true of today's SDK and might not be of tomorrow's. What actually guarantees the + // record is right is causal rather than temporal: touchSession() re-stamps the agent and + // workspace on the first tool call (see withRemoteErrorHandling), which is by definition + // after initialize. Losing this yield would cost a session that never calls a tool + // showing as "unknown" in the presence panel, not a wrong record. + await new Promise((resolve) => setImmediate(resolve)); + + if (server.server.getClientCapabilities()?.roots) { + if (await adoptHostRoots()) await refreshWorkspace("from MCP roots"); + // A host whose roots change mid-session has moved the user to another project. Without + // re-resolving, every item captured for the rest of that session is filed under the + // project the session happened to start in — silently, which is the worst kind. + server.server.setNotificationHandler(RootsListChangedNotificationSchema, () => { + void (async () => { + if (!(await adoptHostRoots())) return; + await refreshWorkspace("host reported its roots changed"); + await registerThisSession(); + })(); + }); + } + await registerThisSession(); + log(`session: client ready — agent=${currentAgent() ?? "unknown"} workspace=${workspace ?? "(unfiled)"}`); } function text(value: string) { @@ -165,6 +246,10 @@ function withRemoteErrorHandling( handler: (...args: Args) => Promise, ): (...args: Args) => Promise> { return async (...args: Args) => { + // Every tool call is also this session's heartbeat. Hanging it off the wrapper every + // handler already shares means no individual tool can forget it — and the write itself + // is debounced, so the common case costs nothing (see touchSession). + void touchSession(sessionToken, { agent: currentAgent(), workspace }); try { return await handler(...args); } catch (err) { @@ -184,73 +269,12 @@ function clearable(value: T | undefined): Exclude | nul return value as Exclude; } -/** Catches the classic accidental-paste: description repeats the title verbatim at its start. */ -function duplicationWarning(title: string, description: string | null): string { - if (description && description.startsWith(title)) { - return " ⚠️ description starts with the same text as title — looks like accidental duplication, not a real description."; - } - return ""; -} - -function formatTodo(todo: Todo): string { - const box = todo.done ? "[x]" : "[ ]"; - const cat = todo.category ? ` [${todo.category}]` : ""; - const pri = todo.priority ? ` !${todo.priority}` : ""; - const due = todo.dueDate ? ` due:${todo.dueDate}` : ""; - const working = isClaimActive(todo) - ? ` ▶working:${todo.workingAgent}${todo.workingSession ? `[${todo.workingSession}]` : ""}` - : ""; - const via = todo.agent ? ` (via ${formatAgentIdentity(todo.agent, todo.deviceName)})` : ""; - const suffix = todo.done && todo.completedAt ? ` (done ${todo.completedAt.slice(0, 10)})` : ""; - const desc = todo.description ? `\n ${todo.description}` : ""; - const source = todo.sourceUrl ? `\n 🔗 ${todo.sourceUrl}` : ""; - return `${box} #${todo.id} (${shortId(todo.uuid)})${cat}${pri}${due}${working} ${todo.title}${via}${suffix}${desc}${source}`; -} - -/** Open items first (oldest first), done items after (most recently completed first). */ -function sortTodos(todos: Todo[]): Todo[] { - return [...todos].sort((a, b) => { - if (a.done !== b.done) return a.done ? 1 : -1; - if (a.done) return (b.completedAt ?? "").localeCompare(a.completedAt ?? ""); - return a.id - b.id; - }); -} - -function formatGroup(todos: Todo[], filter: string): string { - if (todos.length === 0) return `No ${filter === "all" ? "" : filter + " "}todos.`; - return todos.map(formatTodo).join("\n"); -} - -/** When both lists are in scope, render them under separate headers so todo vs backlog stays visually distinct. */ -function formatResult( - todos: Todo[], - filter: string, - list: TodoList | "all", - pagination?: { limit?: number; offset?: number; total: number }, -): string { - let header = ""; - if (pagination && (pagination.limit !== undefined || pagination.offset !== undefined)) { - const offset = pagination.offset ?? 0; - const limit = pagination.limit ?? todos.length; - const start = pagination.total > 0 ? offset + 1 : 0; - const end = Math.min(offset + limit, pagination.total); - header = `_Showing ${start}-${end} of ${pagination.total} items (offset: ${offset}, limit: ${limit})_\n\n`; - } - - if (list !== "all") { - return `${header}${formatGroup(todos, filter)}`; - } - const todoItems = todos.filter((t) => t.list === "todo"); - const backlogItems = todos.filter((t) => t.list === "backlog"); - return `${header}## Todo\n${formatGroup(todoItems, filter)}\n\n## Backlog\n${formatGroup(backlogItems, filter)}`; -} - server.registerTool( "todo_add", { title: "Add todo", description: - "Add a new item to the shared global TODO list. Use list=\"backlog\" for things to park and not hold in context (deferred findings, low-priority follow-ups); list=\"todo\" (default) for near-term actionable items.", + "Capture work the moment it comes up, from any tool or project, without the ceremony of a ticket. Automatically filed under the current project — you never need to say which. Add sourceUrl when it maps to something in Notion/GitLab/Obsidian/GitHub so it can be picked back up there. Use list=\"backlog\" to park something without holding it in context; list=\"todo\" (default) for near-term work.", inputSchema: { title: z.string().min(1).describe("Short one-line title/summary"), description: z.string().optional().describe("Optional longer body text — details, context, links"), @@ -270,12 +294,22 @@ server.registerTool( .describe( "Strongly recommended when this item comes from somewhere with a URL: a GitHub issue/PR, a Notion page, an Obsidian note (if it has a share/publish link), a Slack thread, a doc, etc. Lets a human jump straight back to the source instead of re-finding it.", ), + workspace: z + .string() + .optional() + .describe("Override which project this is filed under. Almost never needed — the current project is filled in automatically."), }, annotations: { readOnlyHint: false, destructiveHint: false }, }, - withRemoteErrorHandling(async ({ title, description, list, category, priority, dueDate, sourceUrl }) => { - const todo = await (await getMcpTodoService()).create({ title, description, list, category, priority, dueDate, sourceUrl }, currentContext()); - return text(`Added [${todo.list}] ${formatTodo(todo)}${duplicationWarning(todo.title, todo.description)}`); + withRemoteErrorHandling(async ({ title, description, list, category, priority, dueDate, sourceUrl, workspace: explicit }) => { + const todo = await (await getMcpTodoService()).create( + { title, description, list, category, priority, dueDate, sourceUrl, workspace: explicit ? (slugifyWorkspace(explicit) ?? undefined) : undefined }, + currentContext(), + ); + // Only worth reading the session registry when there is a project to point at — see + // routingHint, which returns "" immediately for an unfiled item. + const hint = todo.workspace ? routingHint(await listSessions().catch(() => []), todo.workspace, sessionToken) : ""; + return text(`Added [${todo.list}] ${formatTodo(todo, workspace)}${duplicationWarning(todo.title, todo.description)}${hint}`); }), ); @@ -309,7 +343,7 @@ server.registerTool( }; const todo = await (await getMcpTodoService()).edit(id, patch, currentContext()); if (!todo) return text(`No todo with id #${id}`); - return text(`Updated ${formatTodo(todo)}${duplicationWarning(todo.title, todo.description)}`); + return text(`Updated ${formatTodo(todo, workspace)}${duplicationWarning(todo.title, todo.description)}`); }), ); @@ -328,7 +362,7 @@ server.registerTool( if (!claimed) return text(`No todo with id #${id}`); const { todo, previousAgent } = claimed; const warning = previousAgent && previousAgent !== context.agent ? ` (note: was already claimed by ${previousAgent} — taking over)` : ""; - return text(`Claimed ${formatTodo(todo)}${warning}`); + return text(`Claimed ${formatTodo(todo, workspace)}${warning}`); }), ); @@ -343,7 +377,7 @@ server.registerTool( withRemoteErrorHandling(async ({ id }) => { const todo = await (await getMcpTodoService()).release(id, currentContext()); if (!todo) return text(`No todo with id #${id}`); - return text(`Released ${formatTodo(todo)}`); + return text(`Released ${formatTodo(todo, workspace)}`); }), ); @@ -351,7 +385,8 @@ server.registerTool( "todo_list", { title: "List todos", - description: "List items from the shared global TODO list, formatted as a checklist with optional pagination.", + description: + "What's open here. Scoped to the current project (plus unfiled items) unless you ask otherwise, and compact by default — one line per item. Pass workspace:\"*\" to see every project, or verbose:true for full records.", inputSchema: { filter: z .enum(["open", "done", "all"]) @@ -367,17 +402,36 @@ server.registerTool( inProgress: z.boolean().optional().describe("If true, restrict to items currently claimed via todo_claim (see the '▶working' suffix)"), limit: z.number().int().min(1).max(500).optional().describe("Max number of items to return (for token efficiency / pagination)"), offset: z.number().int().min(0).optional().describe("Number of items to skip (for pagination)"), + workspace: z + .string() + .optional() + .describe("Defaults to the current project (plus unfiled items). Pass \"*\" for every project, or a project name for that one."), + verbose: z + .boolean() + .default(false) + .describe("Full records (description, source link, provenance) instead of one compact line per item."), }, annotations: { readOnlyHint: true, destructiveHint: false }, }, - withRemoteErrorHandling(async ({ filter, list, category, agent, session, inProgress, limit, offset }) => { - const matched = await (await getMcpTodoService()).list({ filter, list, category, agent, session, inProgress }); + withRemoteErrorHandling(async ({ filter, list, category, agent, session, inProgress, limit, offset, workspace: scope, verbose }) => { + // The default is the session's own project, NOT everything. One flat list fed by several + // projects × several agents × many terminals is the failure mode this feature exists to + // prevent, and it is also the single largest context saving here: an agent in project A + // stops pulling project B's items into its window on every call. + const requested = scope === undefined ? (workspace ?? "*") : (slugifyWorkspace(scope) ?? "*"); + const matched = await (await getMcpTodoService()).list({ filter, list, category, agent, session, inProgress, workspace: requested }); const sorted = sortTodos(matched); const total = sorted.length; const start = offset ?? 0; const paginated = limit !== undefined || offset !== undefined ? sorted.slice(start, limit !== undefined ? start + limit : undefined) : sorted; - return text(formatResult(paginated, filter, list, { limit, offset, total })); + // A scoped list that came back empty must say what it is NOT showing. The second read + // only happens in that case, so the common path still costs one. + const notice = + total === 0 && requested !== "*" + ? emptyScopeNotice(requested, await (await getMcpTodoService()).list({ workspace: "*" })) + : scopeNotice(requested); + return text(formatResult(paginated, filter, list, { limit, offset, total }, verbose, workspace) + notice); }), ); @@ -392,7 +446,7 @@ server.registerTool( withRemoteErrorHandling(async ({ id }) => { const todo = await (await getMcpTodoService()).complete(id, currentContext()); if (!todo) return text(`No todo with id #${id}`); - return text(`Completed ${formatTodo(todo)}`); + return text(`Completed ${formatTodo(todo, workspace)}`); }), ); @@ -405,9 +459,11 @@ server.registerTool( annotations: { readOnlyHint: true, destructiveHint: false }, }, withRemoteErrorHandling(async ({ id }) => { - const item = await (await getMcpTodoService()).get(id); - if (!item) return text(`No todo with id #${id}`); - return text(formatHistory(item)); + // Goes through history() rather than reading the item's inline preview: since v3.0 the + // full log lives in history.json.enc, and this is one of only two callers that opens it. + const entries = await (await getMcpTodoService()).history(id); + if (!entries) return text(`No todo with id #${id}`); + return text(formatHistoryEntries(entries, id)); }), ); @@ -466,20 +522,24 @@ server.registerTool( function printHelp() { console.log(` -docket - Shared TODO/backlog MCP server & task manager +docket - one list every AI tool you use can write to, across every project Usage: docket [command] [options] Commands: (no args) Start MCP server over stdio (when spawned by AI host) - list [filter] List todos (filter: open | done | all, default: open) + list [filter] List todos in the current project (filter: open | done | all, default: open) + workspaces List projects with open/total counts and last activity + sessions List agent sessions open right now (agent, project, idle time) + hook Claude Code SessionStart integration (install, uninstall, doctor) stats Display terminal statistics widget with active claims web Ensure web UI dashboard is running and print its URL export [options] Export todos to stdout or a file (--format json|markdown) import Import todos from a JSON or Markdown file backup Encrypted full backup: identity, todos, paired peers (password-protected) restore Restore a backup — REPLACES this device's identity/todos/peers + restore --from-v7 Undo the v7→v8 migration before downgrading to docket 2.x check-update Check npm for a newer version without installing anything update Check, confirm, install, self-test, and roll back on failure help, --help, -h Show this help message @@ -487,16 +547,22 @@ Commands: serve Run an authoritative docket server for remote/self-hosted mode (see \`docket serve --help\`-equivalent docs) devices Manage devices paired with a \`docket serve\` running on THIS machine (pair, pending, approve, deny, list, revoke, restore) pair Pair THIS device with a remote docket server (RFC "Local and Self-Hosted Backend Modes" §13) - status Show deployment mode, and connection/store health (local: store+web+peers; remote: server+latency+device authorization) + status Show deployment mode, resolved project, live sessions, and store/connection health backend use Switch this device to a self-hosted server, migrating local data to it if the server is empty backend localize Download the current remote server's workspace and switch back to local mode +List options: + -w, --workspace Scope to one project instead of the current directory's + --all Every project, unscoped + Export options: --format, -f Export format: "json" (default) or "markdown" / "md" --out, -o Write export output directly to file Environment variables: DOCKET_WEB_PORT Port for the local web UI (default: 8787) + DOCKET_WORKSPACE Override the project this session files items under (see docs/workspaces.md) + DOCKET_HOOKS Set to "off" to disable the SessionStart hook without editing any config DOCKET_MODE "local" (default) or "remote" — see \`docket pair\` and ~/.config/docket/config.json DOCKET_SERVER_URL Server URL to use when DOCKET_MODE=remote DOCKET_ALLOW_INSECURE_REMOTE Set to "1" to allow a non-HTTPS remote server URL (trusted LAN dev only) @@ -530,38 +596,59 @@ async function handleCli(args: string[]): Promise { } if (cmd === "stats") { - const store = await readStore(); - const todo = store.todos.filter((t) => t.list === "todo"); - const backlog = store.todos.filter((t) => t.list === "backlog"); - const todoOpen = todo.filter((t) => !t.done).length; - const backlogOpen = backlog.filter((t) => !t.done).length; - const GREEN = "\x1b[38;2;52;211;153m"; - const VIOLET = "\x1b[38;2;167;139;250m"; - const AMBER = "\x1b[38;2;245;158;11m"; - const DIM = "\x1b[2m"; - const RESET = "\x1b[0m"; - - let out = `${GREEN}Todo ${todoOpen}${RESET}`; - if (backlogOpen > 0) out += ` ${VIOLET}Backlog ${backlogOpen}${RESET}`; - const working = store.todos.filter((t) => t.workingAgent && !t.done && isClaimActive(t)); - if (working.length > 0) { - const label = (t: (typeof working)[number]) => t.category ?? (t.title.length > 30 ? `${t.title.slice(0, 30)}…` : t.title); - const items = working.map((t) => `${AMBER}▶ ${label(t)}${RESET} ${DIM}(${t.workingAgent})${RESET}`).join(", "); - out += `\n${items}`; - } - console.log(out); + console.log(renderStatsWidget(await readStore())); return true; } if (cmd === "list" || cmd === "ls") { - const filter = (args[1]?.toLowerCase() as "open" | "done" | "all") || "open"; + const flagIndex = args.findIndex((a) => a === "-w" || a === "--workspace"); + const positional = args[1] && !args[1].startsWith("-") ? args[1].toLowerCase() : ""; + const filter = (positional as "open" | "done" | "all") || "open"; + // No flags scopes to the cwd's project, exactly like the MCP default — the CLI and the + // agents have to agree about what "the list" means or the tool teaches two different + // mental models. + const scope = args.includes("--all") + ? "*" + : flagIndex !== -1 + ? (slugifyWorkspace(args[flagIndex + 1] ?? "") ?? "*") + : ((await currentWorkspace()).workspace ?? "*"); const store = await readStore(); - const todos = store.todos.filter((t) => { - if (filter === "open") return !t.done; - if (filter === "done") return t.done; + const todos = filterTodos(store.todos, { filter, workspace: scope }); + const notice = + todos.length === 0 && scope !== "*" + ? emptyScopeNotice(scope, store.todos, "--all for every project") + : scopeNotice(scope, "--all for every project"); + console.log(formatResult(todos, filter, "all", undefined, true, scope === "*" ? null : scope) + notice); + return true; + } + + if (cmd === "sessions") { + const sessions = await listSessions(); + if (sessions.length === 0) { + console.log("No agent sessions open right now."); return true; - }); - console.log(formatResult(todos, filter, "all")); + } + const width = Math.max(...sessions.map((s) => (s.agent ?? "unknown").length)); + for (const s of sessions) { + console.log(`${(s.agent ?? "unknown").padEnd(width)} ${(s.workspace ?? "(unfiled)").padEnd(24)} ${formatIdle(s.lastSeenAt).padEnd(9)} pid ${s.pid} ${s.cwd}`); + } + return true; + } + + if (cmd === "workspaces" || cmd === "ws") { + const summary = summarizeWorkspaces((await readStore()).todos); + if (summary.length === 0) { + console.log("No items yet — nothing to group into projects."); + return true; + } + const current = (await currentWorkspace()).workspace; + const width = Math.max(...summary.map((w) => w.name.length)); + for (const { name, open, total, lastActivity } of summary) { + const here = name === current ? " ← here" : ""; + console.log( + `${name.padEnd(width)} ${String(open).padStart(4)} open / ${String(total).padStart(4)} total last ${lastActivity.slice(0, 16).replace("T", " ")}${here}`, + ); + } return true; } @@ -677,6 +764,24 @@ async function handleCli(args: string[]): Promise { return true; } + if (cmd === "restore" && args.includes("--from-v7")) { + // The downgrade escape hatch. Deliberately its own branch rather than a flag threaded + // through the password flow below: this restores a plain pre-migration copy of the + // store, not an encrypted backup bundle, so there is nothing to decrypt and no password + // to ask for. + const restored = await restorePreUpgradeStore(); + if (!restored) { + console.error("No pre-upgrade store found. This install either never migrated from v7, or was migrated by a build older than 3.0.0."); + process.exit(1); + } + console.log(`Restored the pre-upgrade (v7) store from ${restored.restoredFrom}.`); + console.log(`Your v8 store was moved aside to ${restored.movedAside} — nothing was deleted.`); + console.log(""); + console.log(`Now reinstall a docket release that reads v7:\n\n npm install -g @pasichdev/docket@${LAST_V7_RELEASE}\n`); + console.log("Restart any running MCP host afterwards."); + return true; + } + if (cmd === "restore") { const file = args[1]; if (!file) { @@ -705,14 +810,16 @@ async function handleCli(args: string[]): Promise { } // Graceful shutdown handlers -process.on("SIGINT", () => { - log("mcp process received SIGINT, exiting cleanly"); - process.exit(0); -}); -process.on("SIGTERM", () => { - log("mcp process received SIGTERM, exiting cleanly"); +// Deregister before exiting so a closed terminal disappears from `docket sessions` +// immediately, instead of lingering until its TTL. The pid check is the backstop for the +// cases this never runs for (SIGKILL, a host that just closes the pipe). +async function shutdown(signal: string): Promise { + log(`mcp process received ${signal}, exiting cleanly`); + await endSession(sessionToken).catch(() => {}); process.exit(0); -}); +} +process.on("SIGINT", () => void shutdown("SIGINT")); +process.on("SIGTERM", () => void shutdown("SIGTERM")); async function main() { const args = process.argv.slice(2); @@ -741,6 +848,18 @@ async function main() { const deployment = await getDeployment(); if (deployment.mode === "remote") await getMcpTodoService(); + await refreshWorkspace("resolved at startup"); + // Not awaited: nothing reads this placeholder registration, and onClientReady replaces it + // with the real agent name the moment the host introduces itself. Blocking the transport + // on a file-lock round trip would spend the most latency-sensitive moment of a session on + // a value that is about to be overwritten. + void registerThisSession(); + + // Fires after initialize, which is when the client's name and capabilities are first + // known. Not awaited anywhere: a host slow to answer listRoots must not delay the + // session, and the cwd-derived workspace is already in place and usable. + server.server.oninitialized = () => void onClientReady(); + const transport = new StdioServerTransport(); await server.connect(transport); // Both of these touch/create LOCAL on-disk state (todos.json.enc's legacy-field @@ -754,8 +873,13 @@ async function main() { } } -main().catch((err) => { - log(`mcp failed to start: ${err.stack ?? err.message}`); - console.error("docket failed to start:", err); +main().catch((err: Error) => { + // The stack goes to the log file, where someone debugging can find it. What reaches the + // terminal is the message alone: a person who typed a filename that doesn't exist is not + // helped by a stack trace, and "failed to start" is the wrong sentence for a command that + // started fine and then hit a bad argument. + log(`docket failed: ${err.stack ?? err.message}`); + const ranACommand = process.argv.length > 2; + console.error(ranACommand ? `docket: ${err.message}` : `docket failed to start: ${err.message}`); process.exit(1); }); diff --git a/src/launcher.ts b/src/launcher.ts index 8b6ced9..547acc6 100644 --- a/src/launcher.ts +++ b/src/launcher.ts @@ -1,10 +1,30 @@ #!/usr/bin/env node +// A CLI's stdout is routinely a pipe that closes early — `docket list | head`, a shell +// prompt widget that stops reading, a hook whose host exited. Node surfaces that as an +// unhandled 'error' on the stream, which kills the process with a stack trace where every +// other Unix tool exits quietly. Installed here, before any command is dispatched, so no +// entry point has to remember it. For the stdio MCP server the same event means the host +// closed the connection, where exiting is also the right answer. +for (const stream of [process.stdout, process.stderr]) { + stream.on("error", (err: NodeJS.ErrnoException) => { + if (err.code === "EPIPE") process.exit(0); + throw err; + }); +} + + // Keep the setup path free of the MCP server's persistence imports. This is // important for `npx ... setup` in restricted agent sandboxes. if (process.argv[2]?.toLowerCase() === "setup") { const { runInteractiveSetup } = await import("./setup.js"); await runInteractiveSetup(process.argv.slice(3)); +} else if (process.argv[2]?.toLowerCase() === "hook") { + // Dispatched before index.js so the SessionStart hook never loads the MCP server, the + // device identity or the encrypted store. It runs before every Claude Code session with a + // 20ms budget; importing any of that would spend the whole budget before doing any work. + const { runHookCommand } = await import("./hooks/cli.js"); + await runHookCommand(process.argv.slice(3)); } else if (process.argv[2]?.toLowerCase() === "serve") { // Dispatched before index.js's stdio MCP server (or anything it imports) ever loads — // `docket serve` is a completely different process shape (an HTTP server, no stdio diff --git a/src/mutations.test.ts b/src/mutations.test.ts index e86f4c7..23a8805 100644 --- a/src/mutations.test.ts +++ b/src/mutations.test.ts @@ -17,7 +17,7 @@ import { import type { TodoStore } from "./types.js"; function emptyStore(): TodoStore { - return { formatVersion: 5, nextId: 1, todos: [], deletedUuids: [] }; + return { formatVersion: 8, nextId: 1, todos: [], deletedUuids: [], seqCounter: 0 }; } test("shortId: deterministic (identical for the same uuid, regardless of which device computes it)", () => { @@ -78,7 +78,7 @@ test("touch: bumps updatedAt/device and stamps only the fields actually changed" const todo = createTodo(store, { title: "x", agent: null, session: null }, "device-1", "One"); const createdAt = todo.updatedAt; await new Promise((r) => setTimeout(r, 2)); - touch(todo, "device-2", "Two", ["title", "priority"]); + touch(store, todo, "device-2", "Two", ["title", "priority"]); assert.ok(todo.updatedAt > createdAt); assert.equal(todo.deviceId, "device-2"); assert.equal(todo.deviceName, "Two"); @@ -114,7 +114,7 @@ test("applyEdits: applies only the fields that actually change, and stamps only const store = emptyStore(); const todo = createTodo(store, { title: "Old", category: "cat", agent: null, session: null }, "d", "n"); - const changed = applyEdits(todo, { title: "New", category: "cat" }, "web", "d2", "n2"); + const changed = applyEdits(store, todo, { title: "New", category: "cat" }, "web", "d2", "n2"); assert.equal(changed, true); assert.equal(todo.title, "New"); assert.ok(todo.fieldTimestamps.title); @@ -128,7 +128,7 @@ test("applyEdits: null clears a field, undefined leaves it alone", () => { const store = emptyStore(); const todo = createTodo(store, { title: "x", description: "keep me", category: "drop me", agent: null, session: null }, "d", "n"); - applyEdits(todo, { category: null }, "web", "d", "n"); + applyEdits(store, todo, { category: null }, "web", "d", "n"); assert.equal(todo.category, null); assert.equal(todo.description, "keep me"); }); @@ -138,7 +138,7 @@ test("applyEdits: a no-op patch changes nothing and records no history", () => { const todo = createTodo(store, { title: "x", agent: null, session: null }, "d", "n"); const before = todo.updatedAt; - assert.equal(applyEdits(todo, { title: "x", description: undefined }, "web", "d2", "n2"), false); + assert.equal(applyEdits(store, todo, { title: "x", description: undefined }, "web", "d2", "n2"), false); assert.equal(todo.history.length, 1, "only the original 'created' entry"); assert.equal(todo.updatedAt, before, "updatedAt must not move when nothing changed"); }); @@ -147,12 +147,12 @@ test("claimTodo: reports the previous active claim it took over, and null when t const store = emptyStore(); const todo = createTodo(store, { title: "x", agent: null, session: null }, "d", "n"); - assert.equal(claimTodo(todo, "claude-code", "s1", "d", "n"), null); + assert.equal(claimTodo(store, todo, "claude-code", "s1", "d", "n"), null); assert.equal(todo.workingAgent, "claude-code"); assert.equal(todo.workingSession, "s1"); assert.ok(isClaimActive(todo)); - assert.equal(claimTodo(todo, "codex", "s2", "d", "n"), "claude-code"); + assert.equal(claimTodo(store, todo, "codex", "s2", "d", "n"), "claude-code"); assert.match(todo.history.at(-1)!.detail, /took over from claude-code/); }); @@ -160,25 +160,29 @@ test("claimTodo: the same claimant calling again renews the lease without resett const store = emptyStore(); const todo = createTodo(store, { title: "x", agent: null, session: null }, "d", "n"); - claimTodo(todo, "codex", "s1", "d", "n"); + claimTodo(store, todo, "codex", "s1", "d", "n"); const originalSince = todo.workingSince; const originalExpiry = todo.workingLeaseExpiresAt; + const historyLength = todo.history.length; await new Promise((r) => setTimeout(r, 5)); - assert.equal(claimTodo(todo, "codex", "s1", "d", "n"), "codex", "renewal still reports the (unchanged) claimant, not null"); + assert.equal(claimTodo(store, todo, "codex", "s1", "d", "n"), "codex", "renewal still reports the (unchanged) claimant, not null"); assert.equal(todo.workingSince, originalSince, "workingSince must survive a renewal — it's when the work actually started"); assert.ok(todo.workingLeaseExpiresAt! > originalExpiry!, "the lease itself must actually be extended"); - assert.match(todo.history.at(-1)!.detail, /lease renewed/); + // Since v3.0 a renewal writes NO history: it is the absence of an event, and at one + // heartbeat every few minutes per active item it was the main driver of history growth + // that every unrelated write then paid for. + assert.equal(todo.history.length, historyLength, "a renewal must not append a history entry"); }); test("claimTodo: a different SESSION from the same agent name is NOT a renewal (two host sessions can share an agent name) — workingSince resets", async () => { const store = emptyStore(); const todo = createTodo(store, { title: "x", agent: null, session: null }, "d", "n"); - claimTodo(todo, "codex", "s1", "d", "n"); + claimTodo(store, todo, "codex", "s1", "d", "n"); const originalSince = todo.workingSince; await new Promise((r) => setTimeout(r, 5)); - claimTodo(todo, "codex", "s2", "d", "n"); + claimTodo(store, todo, "codex", "s2", "d", "n"); assert.notEqual(todo.workingSince, originalSince, "a different session is a fresh claim, not a heartbeat — workingSince should reset"); assert.equal(todo.workingSession, "s2"); assert.doesNotMatch(todo.history.at(-1)!.detail, /lease renewed/); @@ -188,16 +192,16 @@ test("releaseTodo / completeTodo: both clear the whole claim", () => { const store = emptyStore(); const todo = createTodo(store, { title: "x", agent: null, session: null }, "d", "n"); - claimTodo(todo, "claude-code", "s1", "d", "n"); - releaseTodo(todo, "claude-code", "d", "n"); + claimTodo(store, todo, "claude-code", "s1", "d", "n"); + releaseTodo(store, todo, "claude-code", "d", "n"); assert.equal(todo.workingAgent, null); assert.equal(todo.workingSince, null); assert.equal(todo.workingSession, null); assert.equal(todo.workingLeaseExpiresAt, null); assert.equal(todo.done, false); - claimTodo(todo, "claude-code", "s1", "d", "n"); - completeTodo(todo, "web", "d", "n"); + claimTodo(store, todo, "claude-code", "s1", "d", "n"); + completeTodo(store, todo, "web", "d", "n"); assert.equal(todo.done, true); assert.ok(todo.completedAt); assert.equal(todo.workingAgent, null); @@ -224,6 +228,136 @@ test("createTodo: a javascript: sourceUrl is dropped, not stored (XSS guard)", ( test("applyEdits: a javascript: sourceUrl patch is dropped, not stored (XSS guard)", () => { const store = emptyStore(); const todo = createTodo(store, { title: "x", agent: null, session: null }, "d", "n"); - applyEdits(todo, { sourceUrl: "javascript:alert(1)" }, "web", "d", "n"); + applyEdits(store, todo, { sourceUrl: "javascript:alert(1)" }, "web", "d", "n"); assert.equal(todo.sourceUrl, null); }); + +/** + * Found by sync.convergence.property.test.ts, seed 4. + * + * A device whose clock lags stamps its edit EARLIER than the version it just changed. Every + * peer then compares the two, judges its own copy newer, and discards the edit — so the + * editing device is the only one in the mesh that ever sees its own change. If it goes on + * to delete the item, that is ignored too, and the item is gone locally while alive + * everywhere else, permanently. + * + * The property test cannot guard this: it has to model a skewed clock, and modelling it + * means applying the same clamp, which hides the production one being removed. + */ +test("a lagging clock must not stamp an edit BEFORE the version it edits", () => { + const store = emptyStore(); + const todo = createTodo(store, { title: "x", agent: null, session: null }, "d", "n"); + + // The item carries a timestamp from a device whose clock runs ahead of this one. + const fromTheFuture = new Date(Date.now() + 90_000).toISOString(); + todo.updatedAt = fromTheFuture; + todo.fieldTimestamps = { title: fromTheFuture }; + + applyEdits(store, todo, { title: "edited on the slow machine" }, "web", "d", "n"); + + assert.ok(todo.updatedAt > fromTheFuture, `updatedAt went backwards: ${todo.updatedAt} <= ${fromTheFuture}`); + assert.ok( + todo.fieldTimestamps.title! > fromTheFuture, + "the per-field clock must move forward too, or the field merge discards this edit while the record looks updated", + ); +}); + +/** + * Found by sync.convergence.property.test.ts, seed 1. + * + * Same failure one step further on: a deletion stamped before the version it deletes is + * ignored by every other device, so the item stays alive everywhere except on the machine + * that deleted it — which cannot get it back, because its delivery cursor has already moved + * past the peers' copies. + */ +test("a lagging clock must not stamp a deletion BEFORE the version it deletes", () => { + const store = emptyStore(); + const todo = createTodo(store, { title: "x", agent: null, session: null }, "d", "n"); + const fromTheFuture = new Date(Date.now() + 90_000).toISOString(); + todo.updatedAt = fromTheFuture; + + tombstoneDelete(store, todo, "d"); + + const tombstone = store.deletedUuids.at(-1)!; + assert.ok( + tombstone.deletedAt > fromTheFuture, + `tombstone (${tombstone.deletedAt}) does not supersede the version it deletes (${fromTheFuture}) — every peer will ignore this deletion`, + ); +}); + +/** + * Killed mutant: `wallClock > floor` → `>=`, and `Date.parse(floor) + 1` → a large jump. + * + * Two writes inside the same millisecond read the same wall clock, so the second one's + * floor equals its own "now". The clamp must still move it — a write that leaves the + * timestamp unchanged is indistinguishable from no write at all to every peer, and one that + * leaps forward poisons every later comparison against it. + */ +test("successive writes in the same millisecond each advance the clock by the smallest step", () => { + const store = emptyStore(); + const todo = createTodo(store, { title: "x", agent: null, session: null }, "d", "n"); + + const started = Date.parse(todo.updatedAt); + let previous = todo.updatedAt; + for (let i = 0; i < 50; i++) { + touch(store, todo, "d", "n", ["title"]); + assert.ok(todo.updatedAt > previous, `write ${i} did not advance the clock: ${previous} -> ${todo.updatedAt}`); + previous = todo.updatedAt; + } + // 50 writes must not have pushed the record into next week. Clamping is a nudge past the + // version being overwritten, not a jump. + assert.ok( + Date.parse(todo.updatedAt) - started < 5_000, + `50 rapid writes moved updatedAt ${Date.parse(todo.updatedAt) - started}ms — the clamp is jumping, not nudging`, + ); +}); + +/** + * Killed mutant: `item.revision ?? 1` → 0 / → a large value. + * + * Items written before `revision` existed have none, and storage.ts's migration documents + * that they start at 1. Getting the fallback wrong silently shifts every legacy item's + * optimistic-concurrency counter, which a remote server then compares If-Match against. + */ +test("a legacy item with no revision starts counting from 1, not 0", () => { + const store = emptyStore(); + const todo = createTodo(store, { title: "x", agent: null, session: null }, "d", "n"); + delete (todo as Partial).revision; + + touch(store, todo, "d", "n", ["title"]); + assert.equal(todo.revision, 2, "a legacy item's first write must produce revision 2 (absent means 1)"); +}); + +/** + * Killed mutant: `CLAIM_LEASE_MS = 15` → a large value. + * + * The 15-minute window is a documented promise — README says a claim "auto-expires after 15 + * minutes" — and it is what stops a crashed agent from holding an item forever. Nothing + * else in the suite would notice it changing. + */ +test("a claim's lease is the 15 minutes the README promises", () => { + const ahead = Date.parse(leaseExpiry()) - Date.now(); + assert.ok(Math.abs(ahead - 15 * 60_000) < 2_000, `lease runs ${Math.round(ahead / 1000)}s, README says 900s`); +}); + +/* + * Mutation-testing gaps in mutations.ts, written down rather than papered over. + * + * Three mutants survive the full suite, and none of them is a missing assertion: + * + * - `at > max` → `>=` inside the timestamp floor's reduce. Folding a maximum gives the + * same answer either way; this is an equivalent mutant and no test can distinguish it. + * + * - `now > item.updatedAt` → `>=` in tombstoneDelete. At the boundary the mutant produces + * `deletedAt === updatedAt`, and both downstream comparisons (`deletedAt >= updatedAt` + * to skip a re-insert, `updatedAt <= deletedAt` to apply the deletion) already use + * inclusive tests — so the deletion still wins everywhere. Equivalent in effect; the + * strict form is kept because "later than" is what the comment claims and what a reader + * will assume. + * + * - `workingLeaseExpiresAt > now` → `>=` in isClaimActive. Killing this needs a lease that + * expires at exactly the millisecond of the call, which is a race, not a test. The only + * way to make it deterministic is to inject a clock into production code purely so a + * test can hold it still — a worse trade than an uncovered boundary that decides nothing + * a user could observe. + */ diff --git a/src/mutations.ts b/src/mutations.ts index c08e7af..e3f167c 100644 --- a/src/mutations.ts +++ b/src/mutations.ts @@ -1,6 +1,6 @@ import { createHash } from "node:crypto"; import { diffDetail, pushHistory } from "./history.js"; -import type { Todo, TodoList, TodoPriority, TodoStore } from "./types.js"; +import type { Todo, TodoList, TodoPriority, TodoStore, Tombstone } from "./types.js"; import { uuidv7 } from "./uuid7.js"; // No 0/O, 1/I/L — same unambiguous charset as the pairing short codes. @@ -41,6 +41,8 @@ export interface NewTodoInput { sourceUrl?: string | null; agent: string | null; session: string | null; + /** Resolved from the caller's project context, never typed by an agent — see src/workspace.ts. */ + workspace?: string | null; } /** @@ -73,6 +75,9 @@ export const FIELD_KEYS = [ "workingSession", "workingLeaseExpiresAt", "workingDeviceId", + // Merges per-field like any other content field: moving an item between workspaces on + // one device must survive a concurrent unrelated edit on another. + "workspace", ] as const satisfies readonly (keyof Todo)[]; export type FieldKey = (typeof FIELD_KEYS)[number]; @@ -102,6 +107,7 @@ export function createTodo(store: TodoStore, input: NewTodoInput, deviceId: stri sourceUrl: input.sourceUrl && isSafeUrl(input.sourceUrl) ? input.sourceUrl : null, agent: input.agent, session: input.session, + workspace: input.workspace ?? null, workingAgent: null, workingSince: null, workingSession: null, @@ -115,27 +121,59 @@ export function createTodo(store: TodoStore, input: NewTodoInput, deviceId: stri deviceId, deviceName, history: [], + localSeq: 0, // replaced immediately by stampSeq below; never left at 0 in a saved store }; + stampSeq(store, todo); pushHistory(todo, input.agent, "created", `title: "${input.title}"`, deviceName); store.nextId += 1; store.todos.push(todo); return todo; } +/** + * Assigns this store's next delivery sequence number to a record. Called on every local + * write — including accepting a peer's change during merge, which IS a local write even + * though the content came from elsewhere. `store` is threaded through every mutation + * helper rather than kept in a module-level counter precisely so this can't be forgotten: + * a mutation path that doesn't have the store won't compile, instead of silently + * producing a record no peer will ever be told about. + */ +export function stampSeq(store: TodoStore, rec: { localSeq: number }): void { + store.seqCounter = (store.seqCounter ?? 0) + 1; + rec.localSeq = store.seqCounter; +} + /** * Bump on every mutation to an existing item. `changedFields` stamps the * fine-grained per-field clock sync's merge compares — always pass the * fields you actually changed, not the whole FIELD_KEYS list, or two * independent edits to different fields will falsely look like a conflict. */ -export function touch(item: Todo, deviceId: string, deviceName: string, changedFields: readonly FieldKey[] = []): void { - const now = new Date().toISOString(); +export function touch(store: TodoStore, item: Todo, deviceId: string, deviceName: string, changedFields: readonly FieldKey[] = []): void { + // A write is BY DEFINITION later than the version it overwrites, and taking the wall clock + // literally breaks that on a device whose clock lags. Such a device stamps its edit EARLIER + // than the record it just changed; every peer then compares the two, judges its own copy + // newer, and discards the edit — so the editing device is the only one in the mesh that + // ever sees its own change, silently. If it then deletes the item, that is ignored too, and + // the item is gone locally but alive everywhere else, permanently. + // + // Clamping forward costs nothing when the clock is fine. It is NOT a general answer to + // clock skew — two devices editing different records still race on wall-clock order, which + // is what remoteWinsTie makes at least deterministic. It enforces the narrower thing that + // must hold regardless: one record's own history moves in one direction. + const floor = changedFields.reduce((max, field) => { + const at = item.fieldTimestamps?.[field]; + return at && at > max ? at : max; + }, item.updatedAt ?? ""); + const wallClock = new Date().toISOString(); + const now = wallClock > floor ? wallClock : new Date(Date.parse(floor) + 1).toISOString(); item.updatedAt = now; item.revision = (item.revision ?? 1) + 1; item.deviceId = deviceId; item.deviceName = deviceName; item.fieldTimestamps = item.fieldTimestamps ?? {}; for (const field of changedFields) item.fieldTimestamps[field] = now; + stampSeq(store, item); } /** @@ -164,6 +202,7 @@ const PATCH_FIELDS = ["title", "description", "category", "priority", "dueDate", * identical history and merge metadata. Returns whether anything changed. */ export function applyEdits( + store: TodoStore, item: Todo, patch: TodoPatch, agent: string | null, @@ -182,7 +221,7 @@ export function applyEdits( const changed = Object.keys(changes); if (changed.length === 0) return false; pushHistory(item, agent, "edited", diffDetail(changes), deviceName); - touch(item, deviceId, deviceName, changed as FieldKey[]); + touch(store, item, deviceId, deviceName, changed as FieldKey[]); return true; } @@ -203,6 +242,7 @@ function clearClaim(item: Todo): void { * so "claimed since" still reflects when the work actually started, not the last heartbeat. */ export function claimTodo( + store: TodoStore, item: Todo, agent: string | null, session: string | null, @@ -216,32 +256,53 @@ export function claimTodo( item.workingSession = session; item.workingLeaseExpiresAt = leaseExpiry(); item.workingDeviceId = deviceId; - const detail = isRenewal ? "lease renewed" : previousAgent && previousAgent !== agent ? `took over from ${previousAgent}` : "claimed"; - pushHistory(item, agent, "claimed", detail, deviceName); - touch(item, deviceId, deviceName, CLAIM_FIELDS); + // A renewal writes no history on purpose. It is the ABSENCE of an event — the same agent, + // in the same session, still on the same item — and at one heartbeat every few minutes per + // active item it was the single biggest source of history growth, which the write path + // pays for on every unrelated mutation. The claim's own fields still record who holds it + // and until when, so nothing observable is lost. + if (!isRenewal) { + pushHistory(item, agent, "claimed", previousAgent && previousAgent !== agent ? `took over from ${previousAgent}` : "claimed", deviceName); + } + touch(store, item, deviceId, deviceName, CLAIM_FIELDS); return previousAgent; } /** Drops the claim without completing the item. */ -export function releaseTodo(item: Todo, agent: string | null, deviceId: string, deviceName: string): void { +export function releaseTodo(store: TodoStore, item: Todo, agent: string | null, deviceId: string, deviceName: string): void { clearClaim(item); pushHistory(item, agent, "released", "released", deviceName); - touch(item, deviceId, deviceName, CLAIM_FIELDS); + touch(store, item, deviceId, deviceName, CLAIM_FIELDS); } /** Marks done and drops any claim — shared by the MCP tool and the web API so both stamp the same fields. */ -export function completeTodo(item: Todo, agent: string | null, deviceId: string, deviceName: string): void { +export function completeTodo(store: TodoStore, item: Todo, agent: string | null, deviceId: string, deviceName: string): void { item.done = true; item.completedAt = new Date().toISOString(); clearClaim(item); pushHistory(item, agent, "completed", "marked done", deviceName); - touch(item, deviceId, deviceName, ["done", "completedAt", ...CLAIM_FIELDS]); + touch(store, item, deviceId, deviceName, ["done", "completedAt", ...CLAIM_FIELDS]); } /** Removes the item and records why it disappeared, so a paired device doesn't resurrect it on next sync. */ export function tombstoneDelete(store: TodoStore, item: Todo, deviceId: string | null): void { store.deletedUuids = store.deletedUuids ?? []; - store.deletedUuids.push({ uuid: item.uuid, deletedAt: new Date().toISOString(), deviceId }); + // A deletion is BY DEFINITION later than the version it deletes, and taking the wall + // clock literally breaks that on a device whose clock lags. Such a device writes a + // tombstone stamped before the item's own `updatedAt`; every other device then compares + // the two, judges the item newer than the deletion, and keeps it — so the delete is + // silently ignored across the whole mesh while the deleting device, which removed it + // locally, is the only one that loses it. It never gets it back either, because the + // peers' copies sit below its delivery cursor by then. + // + // Ordering the tombstone after the record it supersedes costs nothing when the clock is + // fine and is the entire fix when it isn't. It does not affect edit-after-delete: an edit + // genuinely later than the deletion still wins and resurrects the item. + const now = new Date().toISOString(); + const deletedAt = now > item.updatedAt ? now : new Date(Date.parse(item.updatedAt) + 1).toISOString(); + const tombstone: Tombstone = { uuid: item.uuid, deletedAt, deviceId, localSeq: 0 }; + stampSeq(store, tombstone); + store.deletedUuids.push(tombstone); store.todos = store.todos.filter((t) => t.uuid !== item.uuid); } diff --git a/src/peers.ts b/src/peers.ts index 6120d9e..7d9e290 100644 --- a/src/peers.ts +++ b/src/peers.ts @@ -119,21 +119,28 @@ export function peerFingerprint(publicKeyX: string): string { } /** - * `cursor` should be the PEER's own clock (the `serverTime` it reported in the - * sync response), not ours — using our local clock here would silently miss - * updates whenever the two machines' clocks disagree (see sync.ts). + * `lastSeq` is the delivery cursor: a point in the PEER's own localSeq space, advanced only + * as far as what was actually merged (see pullFromPeer). `cursor` is the peer's reported + * clock, kept only so the UI can say "synced 4m ago" — it stopped being a cursor in v8, + * because a wall-clock cursor is what let a third device's edits vanish. + * + * `error` is honoured even when `ok` is true: a sync can genuinely succeed and still be + * degraded (a peer stuck on sync protocol v1). Recording that as a failure would be a lie; + * dropping it silently is the exact habit v3.0 exists to break. */ export async function markPeerSynced( id: string, ok: boolean, - details: { cursor?: string; error?: string; protocolVersion?: number; clockSkewMs?: number } = {}, + details: { cursor?: string; lastSeq?: number; epoch?: string; error?: string; protocolVersion?: number; clockSkewMs?: number } = {}, ): Promise { await withPeers((peers) => { const peer = peers.find((p) => p.id === id); if (!peer) return; if (ok && details.cursor) peer.lastSyncAt = details.cursor; + if (ok && details.lastSeq !== undefined) peer.lastSeq = details.lastSeq; + if (ok && details.epoch !== undefined) peer.epoch = details.epoch; peer.lastSyncOk = ok; - peer.lastError = ok ? null : (details.error ?? "unknown error"); + peer.lastError = details.error ?? (ok ? null : "unknown error"); if (details.protocolVersion !== undefined) peer.protocolVersion = details.protocolVersion; if (details.clockSkewMs !== undefined) peer.clockSkewMs = details.clockSkewMs; }); diff --git a/src/presence.test.ts b/src/presence.test.ts index 603d172..d5b54da 100644 --- a/src/presence.test.ts +++ b/src/presence.test.ts @@ -5,9 +5,10 @@ import type { Todo, TodoStore } from "./types.js"; function storeWithHistory(entries: Array>): TodoStore { return { - formatVersion: 5, + formatVersion: 8, nextId: 2, deletedUuids: [], + seqCounter: 1, todos: [ { id: 1, @@ -27,6 +28,8 @@ function storeWithHistory(entries: Array(); diff --git a/src/remote/client.ts b/src/remote/client.ts index 0c08321..1addcfd 100644 --- a/src/remote/client.ts +++ b/src/remote/client.ts @@ -1,5 +1,6 @@ import type { HistoryEntry } from "../history.js"; import { + filterTodos, TodoClaimConflictError, TodoConflictError, TodoNotFoundError, @@ -114,6 +115,11 @@ export class RemoteTodoRepository implements TodoRepository { const headers: Record = {}; if (context.agent) headers["X-Docket-Agent"] = context.agent; if (context.session) headers["X-Docket-Session"] = context.session; + // Descriptive, like agent/session: it tells the server which project this call came + // from so items file themselves there instead of landing unfiled. A server too old to + // read it simply ignores the header, which is why this is additive rather than a + // protocol bump. + if (context.workspace) headers["X-Docket-Workspace"] = context.workspace; return headers; } @@ -221,10 +227,17 @@ export class RemoteTodoRepository implements TodoRepository { if (query.agent) params.set("agent", query.agent); if (query.session) params.set("session", query.session); if (query.inProgress) params.set("inProgress", "true"); + if (query.workspace) params.set("workspace", query.workspace); const qs = params.toString(); const { status, body } = await this.request("GET", `/api/v1/todos${qs ? `?${qs}` : ""}`); if (status !== 200) throw this.unexpected(status, body); - return (body as { todos: WireTodo[] }).todos.map((w) => this.fromWire(w)); + const todos = (body as { todos: WireTodo[] }).todos.map((w) => this.fromWire(w)); + // Filtered again here, on purpose. A server too old to understand `workspace` answers + // with every project's items, and the caller has already been told its list is scoped — + // saying "scoped to acme/backend" over an unscoped list is exactly the kind of quiet + // dishonesty this release exists to remove. `workspace` rides on the wire record, so + // this is decidable client-side regardless of what the server understood. + return filterTodos(todos, { workspace: query.workspace }); } async get(id: TodoId): Promise { diff --git a/src/repository.test.ts b/src/repository.test.ts index 2d23383..b6b9913 100644 --- a/src/repository.test.ts +++ b/src/repository.test.ts @@ -166,7 +166,7 @@ test("LocalTodoRepository.edit: applies the patch and bumps revision", async () assert.equal(edited.revision, created.revision + 1); }); -test("LocalTodoRepository.complete/release/claim: bump revision on every mutation, same as touch() always has", async () => { +test("LocalTodoRepository.complete/release/claim: bump revision on every mutation, same as touch(store, ) always has", async () => { const repo = new LocalTodoRepository(); const created = await repo.create({ title: "Task" }, context()); @@ -182,7 +182,7 @@ test("LocalTodoRepository.complete/release/claim: bump revision on every mutatio assert.equal(completed.revision, released.revision + 1); }); -test("LocalTodoRepository.claim: reports the previous claimant it took over, same as claimTodo()", async () => { +test("LocalTodoRepository.claim: reports the previous claimant it took over, same as claimTodo(store, )", async () => { const repo = new LocalTodoRepository(); const created = await repo.create({ title: "Contested" }, context()); await repo.claim(created.id, context({ agent: "codex" })); @@ -230,6 +230,8 @@ test("LocalTodoRepository: every mutating method throws TodoNotFoundError for an function todo(overrides: Partial): Todo { return { id: 1, + localSeq: 1, + workspace: null, uuid: "u1", title: "x", description: null, diff --git a/src/repository.ts b/src/repository.ts index 25408f3..44ab1be 100644 --- a/src/repository.ts +++ b/src/repository.ts @@ -10,6 +10,7 @@ import { type NewTodoInput, type TodoPatch, } from "./mutations.js"; +import { fullHistoryFor } from "./history-store.js"; import { findTodoByAnyId, readStore, withStore, withTodo } from "./storage.js"; import type { Todo, TodoList } from "./types.js"; @@ -23,6 +24,20 @@ export interface TodoQuery { agent?: string; session?: string; inProgress?: boolean; + /** + * Project scope: + * - omitted → no restriction at all (what the web UI's own unfiltered list wants); + * - `"*"` → everything, said explicitly; + * - a slug → that workspace PLUS unfiled items. The unfiled items ride along + * deliberately: they are legacy or context-free, and dropping them would make + * an agent's default list quietly HIDE work rather than scope it. + * + * There is deliberately no "only unfiled" value. The dashboard's Unfiled view filters the + * list it already holds, so adding one would be a fourth meaning on a shared field with no + * caller — and the first real need ("these two projects", "this one without unfiled") + * should split this into two fields rather than invent a fifth magic string. + */ + workspace?: string | "*"; } /** @@ -36,6 +51,13 @@ export interface MutationContext { session: string | null; deviceId: string; deviceName: string; + /** + * The project this caller is working in, derived (never asked for) — see src/workspace.ts. + * Optional because not every caller has a project context: the web UI is one shared + * dashboard, not a checkout, so its items are genuinely unfiled unless a workspace is + * chosen in the switcher. + */ + workspace?: string | null; } export type CreateTodoInput = Omit; @@ -149,6 +171,7 @@ export function filterTodos(todos: Todo[], query: TodoQuery): Todo[] { if (query.agent && todo.agent !== query.agent) return false; if (query.session && todo.session !== query.session) return false; if (query.inProgress && !isClaimActive(todo)) return false; + if (query.workspace && query.workspace !== "*" && todo.workspace !== query.workspace && todo.workspace !== null) return false; return true; }); } @@ -188,23 +211,36 @@ export class LocalTodoRepository implements TodoRepository { create(input: CreateTodoInput, context: MutationContext): Promise { return withStore((store) => - createTodo(store, { ...input, agent: context.agent, session: context.session }, context.deviceId, context.deviceName), + createTodo( + store, + { + ...input, + agent: context.agent, + session: context.session, + // An explicit workspace on the input wins; otherwise the caller's own project is + // stamped automatically. Nothing an agent has to remember to type, which is the + // only way this field gets filled in reliably. + workspace: input.workspace !== undefined ? input.workspace : (context.workspace ?? null), + }, + context.deviceId, + context.deviceName, + ), ); } async edit(id: TodoId, input: EditTodoInput, context: MutationContext, expectedRevision?: number): Promise { - const todo = await withTodo(id, (item) => { + const todo = await withTodo(id, (item, store) => { checkRevision(item, expectedRevision); - applyEdits(item, input, context.agent, context.deviceId, context.deviceName); + applyEdits(store, item, input, context.agent, context.deviceId, context.deviceName); }); if (!todo) throw new TodoNotFoundError(id); return todo; } async complete(id: TodoId, context: MutationContext, expectedRevision?: number): Promise { - const todo = await withTodo(id, (item) => { + const todo = await withTodo(id, (item, store) => { checkRevision(item, expectedRevision); - completeTodo(item, context.agent, context.deviceId, context.deviceName); + completeTodo(store, item, context.agent, context.deviceId, context.deviceName); }); if (!todo) throw new TodoNotFoundError(id); return todo; @@ -236,7 +272,7 @@ export class LocalTodoRepository implements TodoRepository { if (options?.requireFree && activeHolderDeviceId && activeHolderDeviceId !== context.deviceId && !options.force) { throw new TodoClaimConflictError(structuredClone(item)); } - const previousAgent = claimTodo(item, context.agent, context.session, context.deviceId, context.deviceName); + const previousAgent = claimTodo(store, item, context.agent, context.session, context.deviceId, context.deviceName); return { item, previousAgent }; }); if (!claimed) throw new TodoNotFoundError(id); @@ -244,9 +280,9 @@ export class LocalTodoRepository implements TodoRepository { } async release(id: TodoId, context: MutationContext, expectedRevision?: number): Promise { - const todo = await withTodo(id, (item) => { + const todo = await withTodo(id, (item, store) => { checkRevision(item, expectedRevision); - releaseTodo(item, context.agent, context.deviceId, context.deviceName); + releaseTodo(store, item, context.agent, context.deviceId, context.deviceName); }); if (!todo) throw new TodoNotFoundError(id); return todo; @@ -256,7 +292,9 @@ export class LocalTodoRepository implements TodoRepository { const store = await readStore(); const item = findTodoByAnyId(store, id); if (!item) throw new TodoNotFoundError(id); - return item.history; + // The item itself carries only the inline preview; the rest is in history.json.enc. + // This is the one read path that pays for opening it, which is the point of the split. + return fullHistoryFor(item.uuid, item.history); } async health(): Promise { diff --git a/src/roundtrip.test.ts b/src/roundtrip.test.ts new file mode 100644 index 0000000..29cc982 --- /dev/null +++ b/src/roundtrip.test.ts @@ -0,0 +1,144 @@ +import assert from "node:assert/strict"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; + +const originalDataDirectory = process.env.DOCKET_DATA_DIR; +const dataDirectory = await mkdtemp(join(tmpdir(), "docket-roundtrip-test-")); +process.env.DOCKET_DATA_DIR = dataDirectory; +const { decryptFromBuffer, decryptWithKey, encryptToBuffer, encryptWithKey } = await import("./crypto.js"); +const { applyEdits, claimTodo, createTodo, tombstoneDelete } = await import("./mutations.js"); +const { exportToJson, exportToMarkdown, importFromJson, importFromMarkdown } = await import("./export.js"); +const { readStore, withStore } = await import("./storage.js"); +const { mergeSyncPayload } = await import("./sync.js"); +import type { SyncPayload } from "./sync.js"; +import type { TodoStore } from "./types.js"; + +test.after(() => { + if (originalDataDirectory === undefined) delete process.env.DOCKET_DATA_DIR; + else process.env.DOCKET_DATA_DIR = originalDataDirectory; + return rm(dataDirectory, { recursive: true, force: true }); +}); + +function emptyStore(): TodoStore { + return { formatVersion: 8, nextId: 1, todos: [], deletedUuids: [], seqCounter: 0 }; +} + +/** A store exercising every field shape the format has, so a round trip can't pass by only carrying the easy ones. */ +function richStore(): TodoStore { + const store = emptyStore(); + const plain = createTodo(store, { title: "plain", agent: "codex", session: "s" }, "device-a", "A"); + const full = createTodo( + store, + { + title: "everything set", + description: "multi\nline\tbody with & \"quotes\"", + list: "backlog", + category: "VPQ-834", + priority: "high", + dueDate: "2026-12-01", + sourceUrl: "https://gitlab.com/acme/backend/-/issues/1", + workspace: "acme/backend", + agent: "claude-code", + session: "s2", + }, + "device-b", + "B", + ); + applyEdits(store, full, { title: "everything set, then edited" }, "web", "device-b", "B"); + claimTodo(store, plain, "codex", "s3", "device-a", "A"); + const doomed = createTodo(store, { title: "deleted", agent: null, session: null }, "device-a", "A"); + tombstoneDelete(store, doomed, "device-a"); + return store; +} + +test("round trip: a store survives save → load with every v8 field intact", async () => { + const source = richStore(); + await withStore((store) => { + store.todos = structuredClone(source.todos); + store.deletedUuids = structuredClone(source.deletedUuids); + store.nextId = source.nextId; + store.seqCounter = source.seqCounter; + }); + + const loaded = await readStore(); + assert.equal(loaded.todos.length, source.todos.length); + assert.equal(loaded.deletedUuids.length, source.deletedUuids.length); + assert.equal(loaded.seqCounter, source.seqCounter, "the delivery counter must survive a restart, or every peer's cursor breaks"); + + for (const original of source.todos) { + const after = loaded.todos.find((t) => t.uuid === original.uuid)!; + // Compared whole rather than field by field: a new field added later is then covered by + // this test on the day it is added, instead of the day someone remembers to list it. + assert.deepEqual({ ...after, history: undefined }, { ...original, history: undefined }, `${original.title} changed across a save/load`); + assert.deepEqual(after.history, original.history, "history was not preserved"); + } + assert.deepEqual(loaded.deletedUuids, source.deletedUuids); +}); + +test("round trip: encrypt → decrypt returns the exact bytes, for empty and for large input", async () => { + const cases = ["", "{}", JSON.stringify(emptyStore()), JSON.stringify(richStore(), null, 2), "unicode ☃ ‮rtl‬ \0 nul", "x".repeat(2_000_000)]; + for (const plaintext of cases) { + assert.equal(await decryptFromBuffer(await encryptToBuffer(plaintext)), plaintext, `mismatch for ${plaintext.length} bytes`); + } +}); + +test("round trip: a payload merged twice changes nothing the second time", async () => { + const local = emptyStore(); + const remote = richStore(); + const payload: SyncPayload = { todos: remote.todos, deletedUuids: remote.deletedUuids, serverTime: new Date().toISOString(), protocolVersion: 2 }; + + const first = mergeSyncPayload(local, payload, "peer"); + const stateAfterFirst = JSON.stringify(local); + const counterAfterFirst = local.seqCounter; + + const second = mergeSyncPayload(local, payload, "peer"); + assert.equal(second.inserted, 0, "re-merging inserted duplicates"); + assert.equal(second.updated, 0, "re-merging reported changes that did not happen"); + assert.equal(local.seqCounter, counterAfterFirst, "a repeat merge burned sequence numbers — every sync becomes a resend"); + assert.equal(JSON.stringify(local), stateAfterFirst, "a repeat merge changed the store"); + assert.ok(first.inserted > 0, "precondition: the first merge did something"); +}); + +test("round trip: export → import preserves the live items and their fields", async () => { + const source = richStore(); + const target = emptyStore(); + const { added } = importFromJson(target, exportToJson(source), "device-x", "X"); + + assert.equal(added, source.todos.length); + for (const original of source.todos) { + const copy = target.todos.find((t) => t.title === original.title); + assert.ok(copy, `${original.title} did not survive export → import`); + assert.equal(copy.category, original.category); + assert.equal(copy.priority, original.priority); + assert.equal(copy.dueDate, original.dueDate); + assert.equal(copy.sourceUrl, original.sourceUrl); + assert.equal(copy.list, original.list); + assert.equal(copy.done, original.done); + } +}); + +test("round trip: export → import via Markdown keeps titles, lists and categories", () => { + const source = richStore(); + const target = emptyStore(); + const { added } = importFromMarkdown(target, exportToMarkdown(source), "device-x", "X"); + assert.equal(added, source.todos.length); + for (const original of source.todos) { + const copy = target.todos.find((t) => t.title === original.title); + assert.ok(copy, `${original.title} did not survive the Markdown round trip`); + assert.equal(copy.list, original.list); + } +}); + +test("round trip: importing the same file twice adds the items twice, and says so", () => { + // Import is deliberately additive, not idempotent — it has no identity to match on, and + // silently swallowing a second import would be worse than a visible duplicate. Pinned here + // so the behaviour is a decision rather than a surprise. + const target = emptyStore(); + const json = exportToJson(richStore()); + const first = importFromJson(target, json, "d", "D"); + const second = importFromJson(target, json, "d", "D"); + assert.equal(second.added, first.added); + assert.equal(target.todos.length, first.added * 2); +}); diff --git a/src/seq.invariant.test.ts b/src/seq.invariant.test.ts new file mode 100644 index 0000000..fb7d463 --- /dev/null +++ b/src/seq.invariant.test.ts @@ -0,0 +1,280 @@ +import assert from "node:assert/strict"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; + +const originalDataDirectory = process.env.DOCKET_DATA_DIR; +const dataDirectory = await mkdtemp(join(tmpdir(), "docket-seq-invariant-test-")); +process.env.DOCKET_DATA_DIR = dataDirectory; +const mutations = await import("./mutations.js"); +const { mergeSyncPayload } = await import("./sync.js"); +import type { SyncPayload } from "./sync.js"; +import type { Todo, TodoStore } from "./types.js"; + +test.after(() => { + if (originalDataDirectory === undefined) delete process.env.DOCKET_DATA_DIR; + else process.env.DOCKET_DATA_DIR = originalDataDirectory; + return rm(dataDirectory, { recursive: true, force: true }); +}); + +function emptyStore(): TodoStore { + return { formatVersion: 8, nextId: 1, todos: [], deletedUuids: [], seqCounter: 0 }; +} + +const DEVICE = ["device-a", "A"] as const; + +/** + * Everything in the store that a peer is ever told about, paired with the sequence number + * it currently carries. `localSeq` is stripped from the compared content because it is the + * thing under test — what matters is whether the CONTENT moved, and whether the number was + * bumped when it did. + */ +function snapshot(store: TodoStore): Map { + const out = new Map(); + for (const t of store.todos) { + const { localSeq, ...rest } = t; + out.set(`todo:${t.uuid}`, { content: JSON.stringify(rest), seq: localSeq }); + } + for (const t of store.deletedUuids) { + const { localSeq, ...rest } = t; + out.set(`tomb:${t.uuid}`, { content: JSON.stringify(rest), seq: localSeq }); + } + return out; +} + +/** + * THE invariant of format v8: if a record's content changed, it got a fresh sequence number + * from this operation. A record a peer can see, that changed without one, is a record that + * peer will never be told about — silently, and only visibly much later as divergence. + * + * One of the three HIGH bugs found in review was exactly this shape — accepting a peer's + * field update without stamping it — and this catches that class, verified by reverting the + * fix. So is an unstamped `tombstoneDelete`, caught by the vanished-record half below. + * + * What it CANNOT catch, and it is worth being precise rather than claiming the whole set: + * the other tombstone bug (a newer deletion for a uuid we already have a tombstone for) is + * a MISSING change, not an unstamped one — without the fix the store simply doesn't move, + * so any "if it changed, stamp it" property is vacuously satisfied. That one needs a test + * that knows what SHOULD have happened, which is what sync.transitive.test.ts is. + */ +function assertSeqInvariant(name: string, before: TodoStore, after: TodoStore, beforeSnap: Map): void { + const afterSnap = snapshot(after); + const changed = [...afterSnap].filter(([key, now]) => beforeSnap.get(key)?.content !== now.content); + // A record that VANISHED is the other half of the property, and the easier half to miss: + // the news of a disappearance travels as a tombstone, so an item that goes away without a + // freshly-stamped tombstone is a deletion no peer will ever apply. + const vanished = [...beforeSnap.keys()].filter((key) => key.startsWith("todo:") && !afterSnap.has(key)); + + // The converse, and it matters just as much: an operation that changed nothing must not + // burn a sequence number. Every number handed out is a record some peer will re-fetch, so + // a no-op that stamps turns routine syncing into a full resend — and two peers stamping + // each other's no-ops never stop talking to each other at all. + if (changed.length === 0 && vanished.length === 0) { + assert.equal( + after.seqCounter, + before.seqCounter, + `${name}: nothing changed, but seqCounter moved ${before.seqCounter} -> ${after.seqCounter} — this makes every sync a resend`, + ); + return; + } + assert.ok( + after.seqCounter > before.seqCounter, + `${name}: ${changed.length} changed / ${vanished.length} removed record(s) but seqCounter did not move (${before.seqCounter})`, + ); + const assignedHere = (seq: number) => seq > before.seqCounter && seq <= after.seqCounter; + for (const [key, now] of changed) { + assert.ok( + assignedHere(now.seq), + `${name}: ${key} changed but its localSeq (${now.seq}) was not assigned by this operation ` + + `(expected > ${before.seqCounter} and <= ${after.seqCounter}) — a peer will never hear about it`, + ); + } + for (const key of vanished) { + const tombstone = afterSnap.get(`tomb:${key.slice("todo:".length)}`); + assert.ok(tombstone, `${name}: ${key} disappeared without leaving a tombstone — peers will resurrect it`); + assert.ok( + assignedHere(tombstone.seq), + `${name}: ${key} was deleted but its tombstone's localSeq (${tombstone.seq}) was not assigned by this ` + + `operation — the deletion is applied locally and never propagates`, + ); + } +} + +/** One mutating operation, applied to a store that has been set up for it. */ +interface Operation { + name: string; + setUp?: (store: TodoStore) => void; + run: (store: TodoStore) => void; +} + +const payload = (todos: Todo[], deletedUuids: SyncPayload["deletedUuids"] = []): SyncPayload => ({ + todos, + deletedUuids, + serverTime: new Date().toISOString(), + protocolVersion: 2, +}); + +const seed = (store: TodoStore, title = "seeded") => + mutations.createTodo(store, { title, agent: "test", session: "s" }, ...DEVICE); + +/** A copy of an item as a PEER would send it: same uuid, edited later, carrying its own (meaningless here) sequence number. */ +function asRemoteEdit(item: Todo, edit: (t: Todo) => void): Todo { + const remote = structuredClone(item); + const later = new Date(Date.now() + 60_000).toISOString(); + edit(remote); + remote.updatedAt = later; + remote.fieldTimestamps = Object.fromEntries(Object.keys(remote.fieldTimestamps ?? {}).concat(["title", "description", "done"]).map((k) => [k, later])); + remote.deviceId = "device-peer"; + remote.localSeq = 999_999; + return remote; +} + +const OPERATIONS: Operation[] = [ + { name: "create", run: (s) => void seed(s) }, + { + name: "edit", + setUp: (s) => void seed(s), + run: (s) => void mutations.applyEdits(s, s.todos[0], { title: "edited" }, "test", ...DEVICE), + }, + { + name: "edit (no-op)", + setUp: (s) => void seed(s, "unchanged"), + run: (s) => void mutations.applyEdits(s, s.todos[0], { title: "unchanged" }, "test", ...DEVICE), + }, + { name: "claim", setUp: (s) => void seed(s), run: (s) => void mutations.claimTodo(s, s.todos[0], "codex", "s1", ...DEVICE) }, + { + name: "claim (renewal)", + setUp: (s) => { + seed(s); + mutations.claimTodo(s, s.todos[0], "codex", "s1", ...DEVICE); + }, + run: (s) => void mutations.claimTodo(s, s.todos[0], "codex", "s1", ...DEVICE), + }, + { + name: "release", + setUp: (s) => { + seed(s); + mutations.claimTodo(s, s.todos[0], "codex", "s1", ...DEVICE); + }, + run: (s) => void mutations.releaseTodo(s, s.todos[0], "codex", ...DEVICE), + }, + { name: "complete", setUp: (s) => void seed(s), run: (s) => void mutations.completeTodo(s, s.todos[0], "test", ...DEVICE) }, + { + name: "reopen (edit after complete)", + setUp: (s) => { + seed(s); + mutations.completeTodo(s, s.todos[0], "test", ...DEVICE); + }, + run: (s) => void mutations.applyEdits(s, s.todos[0], { title: "reopened" }, "test", ...DEVICE), + }, + { name: "delete", setUp: (s) => void seed(s), run: (s) => mutations.tombstoneDelete(s, s.todos[0], "device-a") }, + { name: "touch", setUp: (s) => void seed(s), run: (s) => mutations.touch(s, s.todos[0], ...DEVICE, ["title"]) }, + + // --- merge paths. These are where both review bugs lived. --- + { + name: "merge: insert", + run: (s) => void mergeSyncPayload(s, payload([seed(emptyStore(), "from peer")]), "peer"), + }, + { + name: "merge: field update", + setUp: (s) => void seed(s), + run: (s) => void mergeSyncPayload(s, payload([asRemoteEdit(s.todos[0], (t) => (t.title = "peer's title"))]), "peer"), + }, + { + name: "merge: no-op update (peer sends what we already have)", + setUp: (s) => void seed(s), + run: (s) => void mergeSyncPayload(s, payload([structuredClone(s.todos[0])]), "peer"), + }, + { + name: "merge: new tombstone", + setUp: (s) => void seed(s), + run: (s) => + void mergeSyncPayload( + s, + payload([], [{ uuid: s.todos[0].uuid, deletedAt: new Date(Date.now() + 60_000).toISOString(), deviceId: "peer", localSeq: 5 }]), + "peer", + ), + }, + { + // The second HIGH bug: a tombstone we ALREADY have, superseded by a later deletion. + // "Stamp every newly ADDED tombstone" does not fire here, and without a number the + // newer deletion never leaves this device. + name: "merge: existing tombstone superseded by a later deletion", + setUp: (s) => { + const item = seed(s); + mutations.tombstoneDelete(s, item, "device-a"); + }, + run: (s) => + void mergeSyncPayload( + s, + payload([], [{ uuid: s.deletedUuids[0].uuid, deletedAt: new Date(Date.now() + 600_000).toISOString(), deviceId: "peer", localSeq: 7 }]), + "peer", + ), + }, + { + name: "merge: resurrect (edit newer than our tombstone), then delete again", + setUp: (s) => { + const item = seed(s); + mutations.tombstoneDelete(s, item, "device-a"); + mergeSyncPayload(s, payload([asRemoteEdit(structuredClone(item), (t) => (t.title = "resurrected"))]), "peer"); + }, + run: (s) => + void mergeSyncPayload( + s, + payload([], [{ uuid: s.todos[0].uuid, deletedAt: new Date(Date.now() + 600_000).toISOString(), deviceId: "peer", localSeq: 9 }]), + "peer", + ), + }, +]; + +for (const operation of OPERATIONS) { + test(`seq invariant: ${operation.name}`, () => { + const store = emptyStore(); + operation.setUp?.(store); + const before = { seqCounter: store.seqCounter } as TodoStore; + const beforeSnap = snapshot(store); + operation.run(store); + assertSeqInvariant(operation.name, before, store, beforeSnap); + }); +} + +/** + * The enumeration is the part that keeps working after this file stops being read. + * + * Every exported function in mutations.ts that takes the store is a path that can change a + * record, and every one of them owes a sequence number. Pinning the export list means a new + * mutator added later fails this test by default — the author has to come here and classify + * it — rather than passing silently, which is how the two review bugs got in. + */ +test("seq invariant: every store-taking mutator is covered by this file", () => { + const KNOWN = ["createTodo", "applyEdits", "claimTodo", "releaseTodo", "completeTodo", "tombstoneDelete", "touch", "stampSeq"]; + const exported = Object.entries(mutations) + .filter(([, value]) => typeof value === "function") + .map(([name]) => name) + .sort(); + const storeTaking = exported.filter((name) => KNOWN.includes(name)); + + assert.deepEqual( + storeTaking.sort(), + [...KNOWN].sort(), + "a store-taking mutator in mutations.ts disappeared — remove it from KNOWN and from OPERATIONS", + ); + + const unknown = exported.filter((name) => !KNOWN.includes(name) && !PURE_HELPERS.includes(name)); + assert.deepEqual( + unknown, + [], + `mutations.ts exports ${unknown.join(", ")}, which this test has never seen. If it can change a record, add it to ` + + `KNOWN and give it an entry in OPERATIONS; if it is a pure helper, add it to PURE_HELPERS.`, + ); + + // `stampSeq` is the primitive the invariant is made of, and every other entry drives it. + const covered = new Set(OPERATIONS.map((o) => o.name.replace(/ .*/, ""))); + for (const name of ["create", "edit", "claim", "release", "complete", "delete", "touch", "merge:"]) { + assert.ok(covered.has(name.replace(":", "")) || covered.has(name), `no OPERATIONS entry exercises ${name}`); + } +}); + +/** Exports of mutations.ts that cannot change a record, and so owe no sequence number. */ +const PURE_HELPERS = ["shortId", "formatAgentIdentity", "isSafeUrl", "isClaimActive", "leaseExpiry", "FIELD_KEYS", "CLAIM_LEASE_MS"]; diff --git a/src/server/routes.ts b/src/server/routes.ts index 0e716ad..dac637e 100644 --- a/src/server/routes.ts +++ b/src/server/routes.ts @@ -78,6 +78,10 @@ function remoteContext(req: IncomingMessage, deviceId: string, deviceName: strin session: header("x-docket-session"), deviceId, deviceName, + // Which project the calling session is in, so items file themselves there rather than + // landing unfiled. Self-reported like agent/session — the server has no view of the + // client's filesystem — and descriptive rather than a security boundary. + workspace: header("x-docket-workspace"), }; } @@ -109,6 +113,7 @@ interface TodoRequestBody { priority?: unknown; dueDate?: unknown; sourceUrl?: unknown; + workspace?: unknown; } function parseJsonBody(res: ServerResponse, raw: string): unknown | null { @@ -308,6 +313,7 @@ export async function handleServeApiRoute( agent: url.searchParams.get("agent") ?? undefined, session: url.searchParams.get("session") ?? undefined, inProgress: url.searchParams.get("inProgress") === "true" ? true : undefined, + workspace: url.searchParams.get("workspace") || undefined, }; const todos = await todoService.list(query); json(res, 200, { todos: todos.map(toWireTodo) }); @@ -333,6 +339,10 @@ export async function handleServeApiRoute( priority: isPriority(b.priority) ? b.priority : null, dueDate: isDate(b.dueDate) ? b.dueDate : null, sourceUrl: typeof b.sourceUrl === "string" && isSafeUrl(b.sourceUrl) ? b.sourceUrl : null, + // Only when the caller named one explicitly. Left undefined, create() falls back to + // the calling session's own project from X-Docket-Workspace, which is what makes + // filing automatic rather than something an agent has to remember. + workspace: typeof b.workspace === "string" ? textOrNull(b.workspace) : undefined, }, context, ); diff --git a/src/sessions.test.ts b/src/sessions.test.ts new file mode 100644 index 0000000..c4a6960 --- /dev/null +++ b/src/sessions.test.ts @@ -0,0 +1,109 @@ +import assert from "node:assert/strict"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; + +const originalDataDirectory = process.env.DOCKET_DATA_DIR; +const dataDirectory = await mkdtemp(join(tmpdir(), "docket-sessions-test-")); +process.env.DOCKET_DATA_DIR = dataDirectory; +const { clearSessions, endSession, listSessions, registerSession, SESSION_TTL_MS } = await import("./sessions.js"); +const { formatIdle, routingHint } = await import("./format.js"); +type LiveSession = import("./sessions.js").LiveSession; + +const SESSIONS_PATH = join(dataDirectory, "sessions.json"); + +test.after(() => { + if (originalDataDirectory === undefined) delete process.env.DOCKET_DATA_DIR; + else process.env.DOCKET_DATA_DIR = originalDataDirectory; + return rm(dataDirectory, { recursive: true, force: true }); +}); + +/** Writes the file directly so a test can plant a session that is stale, or owned by a pid that never existed. */ +async function plant(sessions: Partial[]): Promise { + const now = new Date().toISOString(); + const full = sessions.map((s) => ({ + session: "s", + agent: "codex", + workspace: "acme/backend", + cwd: "/tmp", + pid: process.pid, + startedAt: now, + lastSeenAt: now, + ...s, + })); + await writeFile(SESSIONS_PATH, JSON.stringify(full, null, 2)); +} + +test("a registered session shows up as live", async () => { + await clearSessions(); + await registerSession({ session: "abc", agent: "claude-code", workspace: "acme/backend", cwd: "/repo", pid: process.pid }); + const live = await listSessions(); + assert.equal(live.length, 1); + assert.equal(live[0].agent, "claude-code"); + assert.equal(live[0].workspace, "acme/backend"); +}); + +test("registering the same session twice replaces it rather than duplicating it", async () => { + await clearSessions(); + await registerSession({ session: "abc", agent: null, workspace: null, cwd: "/repo", pid: process.pid }); + await registerSession({ session: "abc", agent: "codex", workspace: "acme/web", cwd: "/repo", pid: process.pid }); + const live = await listSessions(); + assert.equal(live.length, 1, "the agent name and workspace are only known after the client introduces itself"); + assert.equal(live[0].agent, "codex"); +}); + +test("a session past its TTL is reaped", async () => { + await clearSessions(); + await plant([{ session: "stale", lastSeenAt: new Date(Date.now() - SESSION_TTL_MS - 1000).toISOString() }]); + assert.deepEqual(await listSessions(), []); +}); + +test("a session whose process is gone is reaped immediately, not after the TTL", async () => { + await clearSessions(); + // A pid that cannot exist: killed terminals are the common case, and waiting ten minutes + // to notice is exactly the window where someone tries to return to a session that closed. + await plant([{ session: "dead", pid: 2 ** 22, lastSeenAt: new Date().toISOString() }]); + assert.deepEqual(await listSessions(), []); +}); + +test("endSession removes the session, and reading rewrites the file with the reaped set", async () => { + await clearSessions(); + await registerSession({ session: "abc", agent: "codex", workspace: "w", cwd: "/repo", pid: process.pid }); + await endSession("abc"); + assert.deepEqual(await listSessions(), []); + assert.deepEqual(JSON.parse(await readFile(SESSIONS_PATH, "utf8")), []); +}); + +test("routingHint: silent when the only live session in this workspace is the caller's own", () => { + const sessions = [{ session: "mine", agent: "codex", workspace: "acme/backend", cwd: "/r", pid: 1, startedAt: "", lastSeenAt: new Date().toISOString() }]; + assert.equal(routingHint(sessions, "acme/backend", "mine"), "", "a hint about yourself is pure noise, paid for on every capture"); +}); + +test("routingHint: names another agent live in the same workspace, in one short line", () => { + const lastSeenAt = new Date(Date.now() - 2 * 60_000).toISOString(); + const sessions = [ + { session: "mine", agent: "claude-code", workspace: "acme/backend", cwd: "/r", pid: 1, startedAt: "", lastSeenAt }, + { session: "other", agent: "codex", workspace: "acme/backend", cwd: "/r", pid: 2, startedAt: "", lastSeenAt }, + ]; + const hint = routingHint(sessions, "acme/backend", "mine"); + assert.equal(hint, "\n→ codex is live in acme/backend (idle 2m)"); + assert.ok(hint.length < 80, "one line, not a paragraph — see the Stage 7 budget"); +}); + +test("routingHint: sessions in OTHER workspaces are not mentioned", () => { + const sessions = [{ session: "other", agent: "codex", workspace: "acme/web", cwd: "/r", pid: 2, startedAt: "", lastSeenAt: new Date().toISOString() }]; + assert.equal(routingHint(sessions, "acme/backend", "mine"), ""); +}); + +test("routingHint: an unfiled item has no project to point at", () => { + const sessions = [{ session: "other", agent: "codex", workspace: null, cwd: "/r", pid: 2, startedAt: "", lastSeenAt: new Date().toISOString() }]; + assert.equal(routingHint(sessions, null, "mine"), ""); +}); + +test("formatIdle: reads as a duration a human can act on", () => { + const now = Date.now(); + assert.equal(formatIdle(new Date(now - 5_000).toISOString(), now), "active"); + assert.equal(formatIdle(new Date(now - 4 * 60_000).toISOString(), now), "idle 4m"); + assert.equal(formatIdle(new Date(now - 3 * 3600_000).toISOString(), now), "idle 3h"); +}); diff --git a/src/sessions.ts b/src/sessions.ts new file mode 100644 index 0000000..9d2c1dc --- /dev/null +++ b/src/sessions.ts @@ -0,0 +1,141 @@ +import { readFile, rename, rm, writeFile } from "node:fs/promises"; +import { randomUUID } from "node:crypto"; +import { dataPath } from "./data-dir.js"; +import { withFileLock } from "./filelock.js"; +import { log } from "./log.js"; + +const SESSIONS_PATH = await dataPath("sessions.json"); +const LOCK_PATH = `${SESSIONS_PATH}.lock`; + +/** A session unheard from for this long is assumed gone, even if its pid check is inconclusive. */ +export const SESSION_TTL_MS = 10 * 60_000; +/** + * How stale a heartbeat may get before a tool call actually rewrites the file. Every tool + * call touching disk would be a real cost for a value the TTL only reads at minute + * resolution; a third of a minute keeps "idle 2m" honest for a fraction of the writes. + */ +const HEARTBEAT_DEBOUNCE_MS = 20_000; + +export interface LiveSession { + /** The MCP session token — one per host process run (see sessionToken in index.ts). */ + session: string; + /** clientInfo.name as the host reported it: "claude-code", "codex", … */ + agent: string | null; + workspace: string | null; + cwd: string; + pid: number; + startedAt: string; + lastSeenAt: string; +} + +/** + * Which agent sessions are open right now, and where. + * + * Deliberately NOT encrypted and NOT synced. It holds no user content — just process + * metadata about this machine — and a session on one device tells you nothing useful on + * another, since you can't switch to a terminal that isn't in front of you. Keeping it out + * of the encrypted store also keeps it out of the store's lock, so a heartbeat can never + * contend with a real write. + */ +async function readSessions(): Promise { + try { + const parsed = JSON.parse(await readFile(SESSIONS_PATH, "utf8")) as LiveSession[]; + return Array.isArray(parsed) ? parsed : []; + } catch { + // Missing or corrupt: presence is a convenience, and failing a tool call over it would + // trade something useful for something decorative. + return []; + } +} + +async function writeSessions(sessions: LiveSession[]): Promise { + const tmpPath = `${SESSIONS_PATH}.${randomUUID()}.tmp`; + await writeFile(tmpPath, JSON.stringify(sessions, null, 2), { mode: 0o600 }); + await rename(tmpPath, SESSIONS_PATH); +} + +/** + * A pid that no longer exists means the terminal is gone — report it immediately rather + * than letting it haunt the list for the full TTL, which is precisely the window in which + * someone would try to "go back to" a session that closed. EPERM means the process exists + * but belongs to another user, which is still alive as far as this question goes. + */ +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (err) { + return (err as NodeJS.ErrnoException).code === "EPERM"; + } +} + +function isLive(session: LiveSession, now: number): boolean { + if (now - Date.parse(session.lastSeenAt) > SESSION_TTL_MS) return false; + return isProcessAlive(session.pid); +} + +async function updateSessions(mutate: (sessions: LiveSession[]) => LiveSession[]): Promise { + await withFileLock(LOCK_PATH, async () => { + const now = Date.now(); + // Reaped here rather than on a timer: registration, heartbeat and shutdown all pass + // through this one write, which is often enough to keep the file honest without a + // background job whose only purpose is tidying. + const live = (await readSessions()).filter((s) => isLive(s, now)); + await writeSessions(mutate(live)); + }); +} + +export async function registerSession(session: Omit): Promise { + const now = new Date().toISOString(); + await updateSessions((sessions) => [ + ...sessions.filter((s) => s.session !== session.session), + { ...session, startedAt: now, lastSeenAt: now }, + ]).catch((err) => log(`sessions: could not register ${session.session}: ${(err as Error).message}`)); +} + +let lastHeartbeatAt = 0; + +/** + * Called from the seam every tool handler already passes through. Debounced — see + * HEARTBEAT_DEBOUNCE_MS. + * + * It also carries `identity`, so a record registered before the host had introduced itself + * (or from a directory the host later corrected) is repaired by the first tool call rather + * than staying wrong for the life of the session. + */ +export async function touchSession( + sessionToken: string, + identity?: { agent: string | null; workspace: string | null }, +): Promise { + if (Date.now() - lastHeartbeatAt < HEARTBEAT_DEBOUNCE_MS) return; + lastHeartbeatAt = Date.now(); + const now = new Date().toISOString(); + await updateSessions((sessions) => + sessions.map((s) => (s.session === sessionToken ? { ...s, ...identity, lastSeenAt: now } : s)), + ).catch((err) => log(`sessions: heartbeat failed for ${sessionToken}: ${(err as Error).message}`)); +} + +export async function endSession(sessionToken: string): Promise { + await updateSessions((sessions) => sessions.filter((s) => s.session !== sessionToken)).catch(() => {}); +} + +/** + * Live sessions, most recently active first. Filters in memory and does NOT write — a read + * shouldn't need the lock, and the next registration or heartbeat persists the same + * reaping anyway. + * + * Liveness is "heard from recently AND its pid still exists". Pid reuse can in principle + * make a dead session look alive, but only inside its TTL and only if the OS recycled that + * exact number in that window; the cost of being wrong is one stale line in a presence + * list, which is not worth a heavier liveness check. + */ +export async function listSessions(): Promise { + const now = Date.now(); + const live = (await readSessions()).filter((s) => isLive(s, now)); + return live.sort((a, b) => b.lastSeenAt.localeCompare(a.lastSeenAt)); +} + +/** Used by `docket restore` and the test suite: forget every recorded session on this device. */ +export async function clearSessions(): Promise { + await rm(SESSIONS_PATH, { force: true }); +} diff --git a/src/setup.ts b/src/setup.ts index 7e95cd7..a60f4d8 100644 --- a/src/setup.ts +++ b/src/setup.ts @@ -8,6 +8,7 @@ import { fileURLToPath } from "node:url"; import { dirname, join } from "node:path"; import { createLineReader, type LineReader } from "./cli-prompt.js"; import { writeDeploymentConfig } from "./config.js"; +import { isOnPath } from "./hooks/install.js"; import { getDeviceName } from "./device.js"; import { loadRemoteCredentials } from "./remote/credentials.js"; import { beginServerPairing, finishServerPairing, PairingError, probeServer } from "./remote/pairing.js"; @@ -55,21 +56,21 @@ async function installStatsIntegration(dataDirectory: string): Promise { console.log(`Installed todo_stats in ${integration} and sourced it from ${shellRc}.`); } -async function installClaimSkill(): Promise { +async function installSkill(): Promise { try { const marketplace = await execFileAsync("claude", ["plugin", "marketplace", "add", "pasichDev/docket"]); if (marketplace.stdout) process.stdout.write(marketplace.stdout); if (marketplace.stderr) process.stderr.write(marketplace.stderr); - const install = await execFileAsync("claude", ["plugin", "install", "docket-claim@docket"]); + const install = await execFileAsync("claude", ["plugin", "install", "docket@docket"]); if (install.stdout) process.stdout.write(install.stdout); if (install.stderr) process.stderr.write(install.stderr); - console.log("Installed the docket-claim Claude Code skill."); + console.log("Installed the docket Claude Code skill."); } catch (error) { const detail = error as ExecFileException; console.warn(`Could not install the skill automatically (${detail.message ?? "Claude Code not found"}).`); console.warn("Run these in Claude Code when it is available:"); console.warn(" /plugin marketplace add pasichDev/docket"); - console.warn(" /plugin install docket-claim@docket"); + console.warn(" /plugin install docket@docket"); } } @@ -77,12 +78,12 @@ async function installClaimSkill(): Promise { // other AGENTS.md-ecosystem tools read from it) — NOT ~/.codex/skills, which doesn't exist. async function installAgentsSkill(): Promise { const packageRoot = dirname(dirname(fileURLToPath(import.meta.url))); - const source = join(packageRoot, "skills", "docket-claim"); - const destination = join(homedir(), ".agents", "skills", "docket-claim"); + const source = join(packageRoot, "skills", "docket"); + const destination = join(homedir(), ".agents", "skills", "docket"); try { await mkdir(dirname(destination), { recursive: true, mode: 0o700 }); await cp(source, destination, { recursive: true, force: true }); - console.log(`Installed the docket-claim skill at ${destination}.`); + console.log(`Installed the docket skill at ${destination}.`); } catch (error) { // Most likely cause: running inside an agent sandbox that restricts writes to // $HOME outside the current workspace (e.g. Codex's default workspace-write mode). @@ -91,14 +92,14 @@ async function installAgentsSkill(): Promise { console.warn( `Could not install the skill at ${destination}: ${(error as Error).message}\n` + ` If this is running inside a sandboxed agent session, re-run with broader ` + - `filesystem access, or copy skills/docket-claim from the package yourself.`, + `filesystem access, or copy skills/docket from the package yourself.`, ); } } -async function commandExists(command: string): Promise { - try { await execFileAsync("which", [command]); return true; } catch { return false; } -} +// Reuses the hook installer's PATH scan rather than shelling out to `which`, which costs a +// subprocess and does not exist on Windows. +const commandExists = isOnPath; /** * Writes `env` (DOCKET_DATA_DIR for local mode, or DOCKET_MODE+DOCKET_SERVER_URL for @@ -182,8 +183,8 @@ async function runLocalSetup(reader: LineReader, args: string[]): Promise console.log(`\ndocket data directory: ${dataDirectory}`); if (await shouldAutomate(reader, "Configure detected MCP agents automatically?", args)) await configureHosts({ DOCKET_DATA_DIR: dataDirectory }); - if (await shouldAutomate(reader, "Install the docket-claim skill for Claude Code?", args)) await installClaimSkill(); - if (await shouldAutomate(reader, "Install the docket-claim skill (Codex and other AGENTS.md-ecosystem agents)?", args)) await installAgentsSkill(); + if (await shouldAutomate(reader, "Install the docket skill for Claude Code?", args)) await installSkill(); + if (await shouldAutomate(reader, "Install the docket skill (Codex and other AGENTS.md-ecosystem agents)?", args)) await installAgentsSkill(); if (await shouldAutomate(reader, "Install the todo_stats terminal helper and shell startup entry?", args)) await installStatsIntegration(dataDirectory); console.log("\nUse this same directory in every MCP host that should share the list:\n"); console.log("Codex (config.toml):"); @@ -260,8 +261,8 @@ async function runRemoteSetup(reader: LineReader, args: string[]): Promise if (await shouldAutomate(reader, "Configure detected MCP agents automatically?", args)) { await configureHosts({ DOCKET_MODE: "remote", DOCKET_SERVER_URL: serverUrl }); } - if (await shouldAutomate(reader, "Install the docket-claim skill for Claude Code?", args)) await installClaimSkill(); - if (await shouldAutomate(reader, "Install the docket-claim skill (Codex and other AGENTS.md-ecosystem agents)?", args)) await installAgentsSkill(); + if (await shouldAutomate(reader, "Install the docket skill for Claude Code?", args)) await installSkill(); + if (await shouldAutomate(reader, "Install the docket skill (Codex and other AGENTS.md-ecosystem agents)?", args)) await installAgentsSkill(); console.log("\nUse this server in every MCP host that should share this workspace:\n"); console.log("Codex (config.toml):"); diff --git a/src/smoke.cli.test.ts b/src/smoke.cli.test.ts new file mode 100644 index 0000000..6023f2c --- /dev/null +++ b/src/smoke.cli.test.ts @@ -0,0 +1,141 @@ +import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; +import { once } from "node:events"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; +import { fileURLToPath } from "node:url"; + +const dataDirectory = await mkdtemp(join(tmpdir(), "docket-smoke-cli-test-")); +const LAUNCHER = fileURLToPath(new URL("./launcher.js", import.meta.url)); + +test.after(() => rm(dataDirectory, { recursive: true, force: true })); + +/** + * The CLI commands are separate processes, so nothing in the unit suite ever runs one. That + * is how `hook doctor` shipped a crash: it wrote to a child's stdin after the child had + * already exited, and the unhandled EPIPE took the whole command down — a tool whose job is + * to calmly report a broken hook, itself dying on one. + * + * These spawn the real entry point, exactly as a user's shell does. + */ +async function runCli(args: string[], options: { closeStdout?: boolean } = {}): Promise<{ code: number | null; stdout: string; stderr: string }> { + const child = spawn(process.execPath, [LAUNCHER, ...args], { + stdio: ["ignore", "pipe", "pipe"], + env: { ...process.env, DOCKET_DATA_DIR: dataDirectory, DOCKET_WEB_PORT: "18991" }, + }); + let stdout = ""; + let stderr = ""; + child.stdout.on("data", (c) => (stdout += c)); + child.stderr.on("data", (c) => (stderr += c)); + // Destroying the pipe makes the child's own writes fail with EPIPE. A command that doesn't + // handle that dies with an unhandled 'error' event instead of exiting cleanly. + if (options.closeStdout) child.stdout.destroy(); + const [code] = (await once(child, "exit")) as [number | null]; + return { code, stdout, stderr }; +} + +for (const args of [ + ["list"], + ["list", "all"], + ["list", "-w", "acme/backend"], + ["list", "--all"], + ["workspaces"], + ["sessions"], + ["stats"], + ["status"], + ["help"], + ["--help"], + ["export"], + ["export", "--format", "markdown"], + ["check-update"], +]) { + test(`smoke: \`docket ${args.join(" ")}\` exits 0 and prints something`, async () => { + const { code, stdout, stderr } = await runCli(args); + assert.equal(code, 0, `exited ${code}: ${stderr}`); + assert.ok(stdout.trim().length > 0, "produced no output at all"); + }); +} + +test("smoke: `docket hook doctor` reports rather than crashing when nothing is installed", async () => { + const { code, stdout, stderr } = await runCli(["hook", "doctor"]); + assert.equal(code, 0, `exited ${code}: ${stderr}`); + assert.match(stdout, /workspace/, "doctor must say which project it resolved"); + assert.doesNotMatch(stderr, /EPIPE|Unhandled/, `doctor wrote a crash to stderr: ${stderr}`); +}); + +test("smoke: `docket hook doctor` survives a closed stdout (EPIPE)", async () => { + const { code, stderr } = await runCli(["hook", "doctor"], { closeStdout: true }); + assert.equal(code, 0, `exited ${code}: ${stderr}`); + assert.doesNotMatch(stderr, /EPIPE|Unhandled 'error'/, `EPIPE escaped as a crash: ${stderr}`); +}); + +test("smoke: `docket list` survives a closed stdout (EPIPE)", async () => { + const { code, stderr } = await runCli(["list"], { closeStdout: true }); + assert.equal(code, 0, `exited ${code}: ${stderr}`); + assert.doesNotMatch(stderr, /EPIPE|Unhandled 'error'/, `EPIPE escaped as a crash: ${stderr}`); +}); + +test("smoke: `docket hook claude session-start` exits 0 and prints nothing with no server", async () => { + // The fail-open contract, exercised through the real entry point rather than the module. + const child = spawn(process.execPath, [LAUNCHER, "hook", "claude", "session-start"], { + stdio: ["pipe", "pipe", "pipe"], + env: { ...process.env, DOCKET_DATA_DIR: dataDirectory, DOCKET_WEB_PORT: "1" }, + }); + let stdout = ""; + child.stdout.on("data", (c) => (stdout += c)); + child.stdin.end(JSON.stringify({ cwd: process.cwd(), hook_event_name: "SessionStart" })); + const [code] = (await once(child, "exit")) as [number | null]; + assert.equal(code, 0); + assert.equal(stdout, ""); +}); + +test("smoke: `docket export` round-trips through `docket import` without losing items", async () => { + const { readFile, writeFile } = await import("node:fs/promises"); + // Seed through the CLI's own import path, so this exercises the pair rather than the store. + const seedPath = join(dataDirectory, "seed.json"); + await writeFile(seedPath, JSON.stringify([{ title: "round trip A", category: "OPS" }, { title: "round trip B", priority: "high" }])); + const imported = await runCli(["import", seedPath]); + assert.equal(imported.code, 0, imported.stderr); + assert.match(imported.stdout, /2 items/); + + const outPath = join(dataDirectory, "out.json"); + const exported = await runCli(["export", "--out", outPath]); + assert.equal(exported.code, 0, exported.stderr); + const dumped = JSON.parse(await readFile(outPath, "utf8")) as { todos: Array<{ title: string; category: string | null }> }; + const titles = dumped.todos.map((t) => t.title); + assert.ok(titles.includes("round trip A") && titles.includes("round trip B")); + assert.equal(dumped.todos.find((t) => t.title === "round trip A")?.category, "OPS", "a field was lost in the round trip"); +}); + +test("smoke: an unknown command prints help rather than crashing", async () => { + const { code, stdout, stderr } = await runCli(["definitely-not-a-command"]); + // It falls through to the MCP server path, which needs stdin; what must NOT happen is an + // unhandled crash. Either it explains itself or it waits — never a stack trace. + assert.doesNotMatch(stderr, /Unhandled|TypeError|ReferenceError/, `crashed on an unknown command: ${stderr}`); + assert.ok(code === 0 || code === null || stdout.length >= 0); +}); + +test("smoke: `docket import` on a missing file fails cleanly, not with a stack trace", async () => { + const { code, stderr } = await runCli(["import", join(dataDirectory, "does-not-exist.json")]); + assert.notEqual(code, 0, "a missing file must be an error"); + assert.doesNotMatch(stderr, /at Object\.|at async/, `reported a stack trace instead of a message: ${stderr}`); +}); + +test("smoke: `docket hook install` writes nothing when the answer is no", async () => { + const { access } = await import("node:fs/promises"); + const projectDir = join(dataDirectory, "no-thanks"); + const { mkdir } = await import("node:fs/promises"); + await mkdir(projectDir, { recursive: true }); + + const child = spawn(process.execPath, [LAUNCHER, "hook", "install"], { + cwd: projectDir, + stdio: ["pipe", "pipe", "pipe"], + env: { ...process.env, DOCKET_DATA_DIR: dataDirectory }, + }); + child.stdin.end("n\n"); + const [code] = (await once(child, "exit")) as [number | null]; + assert.equal(code, 0); + await assert.rejects(() => access(join(projectDir, ".claude", "settings.json")), "declining still wrote the file"); +}); diff --git a/src/smoke.web.test.ts b/src/smoke.web.test.ts new file mode 100644 index 0000000..76bb937 --- /dev/null +++ b/src/smoke.web.test.ts @@ -0,0 +1,78 @@ +import assert from "node:assert/strict"; +import { once } from "node:events"; +import { mkdtemp, rm } from "node:fs/promises"; +import type { Server } from "node:http"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; + +const originalDataDirectory = process.env.DOCKET_DATA_DIR; +const dataDirectory = await mkdtemp(join(tmpdir(), "docket-smoke-web-test-")); +process.env.DOCKET_DATA_DIR = dataDirectory; +const { createWebServer } = await import("./web/server.js"); +const { LocalTodoRepository } = await import("./repository.js"); + +/** + * The dashboard is one large inline `", "vbscript:x", "JaVaScRiPt:alert(1)", "javascript:alert(1)"].entries()) { + mergeHostile(store, [wireTodo({ uuid: `4444${String(i).padStart(4, "0")}-4444-7444-8444-444444444444`, sourceUrl: url })]); + } + for (const t of store.todos) assert.equal(t.sourceUrl, null, `stored a click-executable sourceUrl: ${t.sourceUrl}`); +}); + +test("hostile: enum-shaped fields only ever hold values this codebase produces", () => { + const store = emptyStore(); + mergeHostile(store, [wireTodo({ priority: " ${process.env} `backtick` ‮RTL‬ "quotes" \\backslash'; + mergeHostile(store, [wireTodo({ title: nasty, description: nasty, category: nasty })]); + assert.equal(store.todos[0].title, nasty, "legitimate text was mangled — that is data loss, not safety"); + assert.equal(store.todos[0].description, nasty); +}); + +// --- Tombstones ------------------------------------------------------------------------ + +test("hostile: malformed tombstones are dropped without disturbing the store", () => { + const store = emptyStore(); + const ours = createTodo(store, { title: "keep me", agent: null, session: null }, "device-a", "A"); + mergeHostile(store, [], [ + null, + "a string", + {}, + { uuid: 42, deletedAt: "2026-01-01T00:00:00.000Z" }, + { uuid: ours.uuid }, + { deletedAt: "2026-01-01T00:00:00.000Z" }, + ]); + assert.equal(store.todos.length, 1, "a malformed tombstone deleted a real item"); + assert.equal(store.deletedUuids.length, 0); +}); + +test("hostile: a tombstone for an item cannot be forged with a non-string deletedAt", () => { + const store = emptyStore(); + const ours = createTodo(store, { title: "keep me", agent: null, session: null }, "device-a", "A"); + mergeHostile(store, [], [{ uuid: ours.uuid, deletedAt: { valueOf: () => "9999-01-01" }, deviceId: "peer" }]); + assert.equal(store.todos.length, 1); +}); + +// --- The payload envelope itself --------------------------------------------------------- + +test("hostile: a payload whose arrays are not arrays is a no-op, not a crash", () => { + const store = emptyStore(); + createTodo(store, { title: "ours", agent: null, session: null }, "device-a", "A"); + for (const shape of [{ todos: null }, { todos: "x" }, { deletedUuids: 5 }, { todos: {} }, {}]) { + assert.doesNotThrow(() => + mergeSyncPayload(store, { serverTime: new Date().toISOString(), protocolVersion: 2, ...shape } as unknown as SyncPayload, "hostile-peer"), + ); + } + assert.equal(store.todos.length, 1, "the store must be untouched by a malformed envelope"); +}); + +// --- The other half: a well-formed record must survive untouched ------------------------- + +/** + * Killed mutants: `typeof o.dueDate === "string"` → `!==`, and the same for `sourceUrl` and + * `revision`. + * + * Every test above checks that BAD input is rejected, and all of them still pass if the + * sanitiser rejects everything — including valid data. Inverting one of those guards makes + * `sanitizeRemoteTodo` silently null out a field on every record that crosses the wire, and + * nothing would have noticed. The rejection tests and this one only mean something together. + */ +test("hostile: a well-formed record crosses the wire with every field intact", () => { + const store = emptyStore(); + const good = wireTodo({ + title: "fix token refresh race", + description: "a real description", + done: true, + list: "backlog", + category: "VPQ-834", + priority: "high", + dueDate: "2026-12-01", + sourceUrl: "https://gitlab.com/acme/backend/-/issues/834", + workspace: "acme/backend", + completedAt: "2026-11-30T10:00:00.000Z", + revision: 7, + workingAgent: "codex", + workingSince: "2026-11-30T09:00:00.000Z", + workingSession: "sess-1", + workingLeaseExpiresAt: "2026-11-30T09:15:00.000Z", + workingDeviceId: "device-peer", + deviceName: "Peer", + }); + mergeHostile(store, [good]); + + assert.equal(store.todos.length, 1, "a perfectly valid record was rejected"); + const stored = store.todos[0]; + for (const field of [ + "title", "description", "done", "list", "category", "priority", "dueDate", "sourceUrl", + "workspace", "completedAt", "revision", "workingAgent", "workingSince", "workingSession", + "workingLeaseExpiresAt", "workingDeviceId", "deviceName", "createdAt", "updatedAt", "uuid", + ] as const) { + assert.deepEqual(stored[field], good[field], `sanitizing dropped or altered a valid ${field}`); + } + assert.equal(stored.history.length, good.history.length, "valid history entries were dropped"); +}); + +test("hostile: the paging boundary sends records strictly above the cursor, never the one at it", () => { + // Killed mutant: `localSeq > sinceSeq` → `>=` on the tombstone stream. An off-by-one here + // re-delivers the record sitting exactly on the cursor on every single tick, forever. + const source = emptyStore(); + const a = createTodo(source, { title: "a", agent: null, session: null }, "d", "D"); + const b = createTodo(source, { title: "b", agent: null, session: null }, "d", "D"); + tombstoneDelete(source, a, "d"); + const tombSeq = source.deletedUuids[0].localSeq; + + const atCursor = buildSyncPayload(source, tombSeq); + assert.ok(!atCursor.deletedUuids.some((t) => t.localSeq === tombSeq), "the tombstone at the cursor was re-sent"); + assert.ok(!atCursor.todos.some((t) => t.localSeq === b.localSeq && b.localSeq <= tombSeq)); + + const justBelow = buildSyncPayload(source, tombSeq - 1); + assert.ok(justBelow.deletedUuids.some((t) => t.localSeq === tombSeq), "the tombstone just above the cursor was skipped"); +}); + +test("hostile: the legacy timestamp payload is also strictly-after, not inclusive", () => { + // Killed mutant: `updatedAt > since` → `>=` in buildLegacySyncPayload. Inclusive here means + // a v1 peer re-sends the newest record on every tick and never makes progress. + const source = emptyStore(); + const item = createTodo(source, { title: "only", agent: null, session: null }, "d", "D"); + assert.equal(buildLegacySyncPayload(source, item.updatedAt).todos.length, 0, "the record at the cursor was re-sent"); + assert.equal(buildLegacySyncPayload(source, "1970-01-01T00:00:00.000Z").todos.length, 1); +}); + +test("hostile: a field tie between two copies with the SAME device id still resolves identically both ways", () => { + // Killed mutant: `byDevice > 0` → `>= 0`, which makes "remote wins" true in both + // directions — each side adopts the other's value forever instead of agreeing on one. + const at = "2026-06-01T00:00:00.000Z"; + const base = { ...wireTodo(), deviceId: "same-device", updatedAt: at, fieldTimestamps: { title: at } }; + const left = { ...structuredClone(base), title: "aaa" }; + const right = { ...structuredClone(base), title: "zzz" }; + + const storeA = emptyStore(); + storeA.todos = [structuredClone(left)]; + storeA.nextId = 2; + mergeHostile(storeA, [structuredClone(right)]); + + const storeB = emptyStore(); + storeB.todos = [structuredClone(right)]; + storeB.nextId = 2; + mergeHostile(storeB, [structuredClone(left)]); + + assert.equal(storeA.todos[0].title, storeB.todos[0].title, "the two devices resolved the same tie differently — they will never converge"); +}); diff --git a/src/sync.pagination.test.ts b/src/sync.pagination.test.ts new file mode 100644 index 0000000..ca5b391 --- /dev/null +++ b/src/sync.pagination.test.ts @@ -0,0 +1,356 @@ +import assert from "node:assert/strict"; +import { randomBytes } from "node:crypto"; +import { once } from "node:events"; +import { mkdtemp, rm } from "node:fs/promises"; +import { createServer, type Server } from "node:http"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; +import { createTodo } from "./mutations.js"; +import type { TodoStore } from "./types.js"; + +const originalDataDirectory = process.env.DOCKET_DATA_DIR; +const dataDirectory = await mkdtemp(join(tmpdir(), "docket-pagination-test-")); +process.env.DOCKET_DATA_DIR = dataDirectory; +const { buildSyncPayload, encryptSyncPayload, MAX_PAGES_PER_TICK, PAGE_SIZE, pullFromPeer, verifySyncRequest } = await import("./sync.js"); +const { addPeer, loadPeers } = await import("./peers.js"); + +const SECRET = randomBytes(32).toString("hex"); +const TOTAL_ITEMS = 2500; + +test.after(() => { + if (originalDataDirectory === undefined) delete process.env.DOCKET_DATA_DIR; + else process.env.DOCKET_DATA_DIR = originalDataDirectory; + return rm(dataDirectory, { recursive: true, force: true }); +}); + +function emptyStore(): TodoStore { + return { formatVersion: 8, nextId: 1, todos: [], deletedUuids: [], seqCounter: 0 }; +} + +/** + * A real peer, over real HTTP, answering the real signed endpoint. Stubbing the fetch + * instead would let the test pass while the client and server disagreed about the wire + * format — which is precisely the seam the silent-truncation bug lived in. + */ +async function startPeer(store: TodoStore, seen: number[]): Promise<{ server: Server; url: string }> { + const server = createServer((req, res) => { + const url = new URL(req.url ?? "/", "http://127.0.0.1"); + const sinceSeq = url.searchParams.get("sinceSeq") ?? ""; + const deviceId = url.searchParams.get("deviceId") ?? ""; + const timestamp = url.searchParams.get("timestamp") ?? ""; + const signature = url.searchParams.get("signature") ?? ""; + if (!verifySyncRequest(SECRET, deviceId, sinceSeq, timestamp, signature)) { + res.writeHead(403, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ error: "signature invalid" })); + return; + } + seen.push(Number(sinceSeq)); + const payload = buildSyncPayload(store, Number(sinceSeq)); + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify(encryptSyncPayload(SECRET, payload))); + }); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const address = server.address(); + assert.ok(address && typeof address !== "string"); + return { server, url: `http://127.0.0.1:${address.port}` }; +} + +test("pullFromPeer: a first sync of 2500 items delivers every one of them, in pages", async () => { + const remote = emptyStore(); + for (let i = 0; i < TOTAL_ITEMS; i++) { + createTodo(remote, { title: `item ${i}`, agent: "codex", session: "s" }, "device-remote", "Remote"); + } + + const requestedCursors: number[] = []; + const { server, url } = await startPeer(remote, requestedCursors); + try { + await addPeer({ + id: "peer-pagination", + name: "Remote", + url, + secret: SECRET, + pairedAt: new Date().toISOString(), + lastSyncAt: null, + lastSyncOk: false, + }); + + const local = emptyStore(); + const [peer] = await loadPeers(); + await pullFromPeer(peer, "device-local", async (fn) => fn(local)); + + assert.equal(local.todos.length, TOTAL_ITEMS, "every item must arrive — a first sync must not silently drop the tail"); + const arrived = new Set(local.todos.map((t) => t.uuid)); + for (const t of remote.todos) assert.ok(arrived.has(t.uuid), `item ${t.uuid} never arrived`); + } finally { + server.close(); + } + + // The cursor must climb strictly, one page at a time, starting from zero — never jump. + assert.equal(requestedCursors[0], 0); + assert.equal(requestedCursors.length, Math.ceil(TOTAL_ITEMS / PAGE_SIZE), "one request per page, no more and no fewer"); + for (let i = 1; i < requestedCursors.length; i++) { + assert.ok(requestedCursors[i] > requestedCursors[i - 1], "the cursor must advance between pages"); + assert.ok(requestedCursors[i] <= requestedCursors[i - 1] + PAGE_SIZE, "the cursor must never skip past unmerged records"); + } +}); + +test("pullFromPeer: the stored cursor advances only to what was actually merged", async () => { + const remote = emptyStore(); + for (let i = 0; i < 10; i++) createTodo(remote, { title: `item ${i}`, agent: null, session: null }, "device-remote", "Remote"); + + const { server, url } = await startPeer(remote, []); + try { + await addPeer({ + id: "peer-cursor", + name: "Remote", + url, + secret: SECRET, + pairedAt: new Date().toISOString(), + lastSyncAt: null, + lastSyncOk: false, + }); + const local = emptyStore(); + const peer = (await loadPeers()).find((p) => p.id === "peer-cursor")!; + await pullFromPeer(peer, "device-local", async (fn) => fn(local)); + + const after = (await loadPeers()).find((p) => p.id === "peer-cursor")!; + assert.equal(after.lastSeq, remote.seqCounter, "the cursor lands exactly on the peer's high-water mark, no further"); + assert.equal(after.lastSyncOk, true); + assert.equal(after.lastError, null); + + // A second pull from that cursor has nothing left to say, and must not re-deliver. + await pullFromPeer(after, "device-local", async (fn) => fn(local)); + assert.equal(local.todos.length, 10, "a repeat sync from the stored cursor must not duplicate anything"); + } finally { + server.close(); + } +}); + +/** + * A peer still on sync protocol v1. It rejects a request signed over a numeric cursor + * (it verifies the HMAC over `since`, which such a request doesn't carry), and it does not + * page — it answers with everything since the timestamp it was given. + * + * That combination is the trap: clamping its response the way a paged response is clamped + * would drop the tail, and then advancing the timestamp cursor to the peer's "now" would + * step straight over the gap. This is the exact bug v3.0 exists to remove, on the one path + * that keeps a mixed-version mesh working. + */ +async function startV1Peer(store: TodoStore): Promise<{ server: Server; url: string }> { + const server = createServer((req, res) => { + const url = new URL(req.url ?? "/", "http://127.0.0.1"); + if (url.searchParams.has("sinceSeq")) { + res.writeHead(403, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ error: "signature invalid or expired" })); // no `reason` — exactly what a v1 build sends + return; + } + const since = url.searchParams.get("since") ?? ""; + if (!verifySyncRequest(SECRET, url.searchParams.get("deviceId") ?? "", since, url.searchParams.get("timestamp") ?? "", url.searchParams.get("signature") ?? "")) { + res.writeHead(403, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ error: "signature invalid" })); + return; + } + const payload = { + todos: store.todos.filter((t) => t.updatedAt > since), + deletedUuids: [], + serverTime: new Date().toISOString(), + protocolVersion: 1, + }; + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify(encryptSyncPayload(SECRET, payload))); + }); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const address = server.address(); + assert.ok(address && typeof address !== "string"); + return { server, url: `http://127.0.0.1:${address.port}` }; +} + +test("pullFromPeer: a v1 peer with more items than one page still delivers every one of them", async () => { + const remote = emptyStore(); + for (let i = 0; i < 600; i++) createTodo(remote, { title: `legacy item ${i}`, agent: null, session: null }, "device-remote", "Remote"); + + const { server, url } = await startV1Peer(remote); + try { + await addPeer({ + id: "peer-v1", + name: "Old peer", + url, + secret: SECRET, + pairedAt: new Date().toISOString(), + lastSyncAt: null, + lastSyncOk: false, + }); + const local = emptyStore(); + const peer = (await loadPeers()).find((p) => p.id === "peer-v1")!; + await pullFromPeer(peer, "device-local", async (fn) => fn(local)); + + assert.equal(local.todos.length, 600, "a v1 peer's whole backlog must arrive — it cannot page for us"); + + const after = (await loadPeers()).find((p) => p.id === "peer-v1")!; + assert.equal(after.lastSyncOk, true, "the sync succeeded — it is degraded, not broken"); + assert.match(after.lastError ?? "", /sync protocol v1/, "and it must say so out loud rather than syncing quietly"); + // Nothing was clamped, so everything the peer offered landed and the cursor may move to + // the peer's own reported clock. Staying in THAT clock is the invariant: a merged record + // can have been authored by a third device, whose timestamps say nothing about where + // this peer's timeline has reached. + assert.ok(after.lastSyncAt && after.lastSyncAt >= remote.todos.at(-1)!.updatedAt); + + // A second pull from that cursor must neither duplicate nor lose anything. + await pullFromPeer(after, "device-local", async (fn) => fn(local)); + assert.equal(local.todos.length, 600, "a repeat sync from the stored timestamp cursor must be a no-op"); + } finally { + server.close(); + } +}); + +test("pullFromPeer: a peer that restored a backup voids the cursor instead of going silent", async () => { + const remote = emptyStore(); + for (let i = 0; i < 5; i++) createTodo(remote, { title: `before ${i}`, agent: null, session: null }, "device-remote", "Remote"); + + let epoch = "epoch-one"; + const server = createServer((req, res) => { + const url = new URL(req.url ?? "/", "http://127.0.0.1"); + const sinceSeq = url.searchParams.get("sinceSeq") ?? ""; + if (!verifySyncRequest(SECRET, url.searchParams.get("deviceId") ?? "", sinceSeq, url.searchParams.get("timestamp") ?? "", url.searchParams.get("signature") ?? "")) { + res.writeHead(403, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ error: "signature invalid" })); + return; + } + res.writeHead(200, { "Content-Type": "application/json" }); + res.end(JSON.stringify(encryptSyncPayload(SECRET, buildSyncPayload(remote, Number(sinceSeq), epoch)))); + }); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const address = server.address(); + assert.ok(address && typeof address !== "string"); + + try { + await addPeer({ + id: "peer-epoch", + name: "Remote", + url: `http://127.0.0.1:${address.port}`, + secret: SECRET, + pairedAt: new Date().toISOString(), + lastSyncAt: null, + lastSyncOk: false, + }); + const local = emptyStore(); + let peer = (await loadPeers()).find((p) => p.id === "peer-epoch")!; + await pullFromPeer(peer, "device-local", async (fn) => fn(local)); + peer = (await loadPeers()).find((p) => p.id === "peer-epoch")!; + assert.equal(peer.lastSeq, remote.seqCounter); + assert.equal(peer.epoch, "epoch-one"); + + // The peer restores an older backup: its counter goes backwards and it re-mints its + // epoch. Our cursor now points past everything it has, so without the epoch check we + // would sit at a number it will not reach again for a long time and hear nothing. + const restored = emptyStore(); + for (let i = 0; i < 3; i++) createTodo(restored, { title: `after restore ${i}`, agent: null, session: null }, "device-remote", "Remote"); + remote.todos = restored.todos; + remote.seqCounter = restored.seqCounter; + epoch = "epoch-two"; + + const fresh = emptyStore(); + await pullFromPeer(peer, "device-local", async (fn) => fn(fresh)); + assert.equal(fresh.todos.length, 3, "the restored store must arrive despite our cursor being far ahead of it"); + peer = (await loadPeers()).find((p) => p.id === "peer-epoch")!; + assert.equal(peer.epoch, "epoch-two", "and the new incarnation is recorded so it isn't re-detected forever"); + } finally { + server.close(); + } +}); + +/** + * Killed mutant: `pages < MAX_PAGES_PER_TICK` → `<=`. + * + * A peer that always claims `hasMore` — buggy, hostile, or genuinely enormous — must not be + * able to hold this device's store lock for an unbounded number of pages. The cap is what + * makes a first sync of a huge store take several ticks instead of one very long one, and + * nothing else asserts the boundary is where it says it is. + */ +test("pullFromPeer: a peer that claims hasMore forever is stopped at exactly MAX_PAGES_PER_TICK", async () => { + let requests = 0; + const server = createServer((req, res) => { + const url = new URL(req.url ?? "/", "http://127.0.0.1"); + const sinceSeq = Number(url.searchParams.get("sinceSeq") ?? "0"); + if (!verifySyncRequest(SECRET, url.searchParams.get("deviceId") ?? "", String(sinceSeq), url.searchParams.get("timestamp") ?? "", url.searchParams.get("signature") ?? "")) { + res.writeHead(403, { "Content-Type": "application/json" }); + res.end("{}"); + return; + } + requests += 1; + res.writeHead(200, { "Content-Type": "application/json" }); + // Always one more item, always more to come — the cursor advances, so this is a peer + // that genuinely never finishes rather than one stuck resending the same page. + const store = emptyStore(); + store.seqCounter = sinceSeq + 1; + const todo = createTodo(store, { title: `endless ${sinceSeq}`, agent: null, session: null }, "device-remote", "R"); + todo.localSeq = sinceSeq + 1; + res.end(JSON.stringify(encryptSyncPayload(SECRET, { todos: [todo], deletedUuids: [], serverTime: new Date().toISOString(), protocolVersion: 2, maxSeq: sinceSeq + 1, hasMore: true }))); + }); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const address = server.address(); + assert.ok(address && typeof address !== "string"); + + try { + await addPeer({ id: "peer-endless", name: "Endless", url: `http://127.0.0.1:${address.port}`, secret: SECRET, pairedAt: new Date().toISOString(), lastSyncAt: null, lastSyncOk: false }); + const local = emptyStore(); + const peer = (await loadPeers()).find((p) => p.id === "peer-endless")!; + await pullFromPeer(peer, "device-local", async (fn) => fn(local)); + + assert.equal(requests, MAX_PAGES_PER_TICK, `made ${requests} requests; the per-tick cap must bound it exactly`); + assert.equal(local.todos.length, MAX_PAGES_PER_TICK, "everything fetched is still merged — the cap yields, it does not discard"); + const after = (await loadPeers()).find((p) => p.id === "peer-endless")!; + assert.equal(after.lastSeq, MAX_PAGES_PER_TICK, "and the cursor records exactly what landed, so the next tick resumes"); + } finally { + server.close(); + } +}); + +/** + * Killed mutant: `(peer.protocolVersion ?? 1) < 2` → `<= 2`. + * + * The legacy fallback exists for peers that predate the seq cursor. A peer already known to + * speak v2 that rejects a request is telling us something real — a rotated secret, a clock + * outside the signature window — and retrying on the legacy path would waste a round trip + * and then report the wrong cause. + */ +test("pullFromPeer: a known-v2 peer that rejects us is not mistaken for a v1 peer", async () => { + let requests = 0; + const server = createServer((_req, res) => { + requests += 1; + res.writeHead(403, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ error: "signature invalid or expired" })); // no `reason` — same shape a v1 build sends + }); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const address = server.address(); + assert.ok(address && typeof address !== "string"); + + try { + await addPeer({ + id: "peer-v2-broken", + name: "Known v2", + url: `http://127.0.0.1:${address.port}`, + secret: SECRET, + pairedAt: new Date().toISOString(), + lastSyncAt: null, + lastSyncOk: false, + protocolVersion: 2, // it has answered v2 before, so "old software" is not the explanation + }); + const local = emptyStore(); + const peer = (await loadPeers()).find((p) => p.id === "peer-v2-broken")!; + await pullFromPeer(peer, "device-local", async (fn) => fn(local)); + + assert.equal(requests, 1, "no second, legacy-path attempt should have been made"); + const after = (await loadPeers()).find((p) => p.id === "peer-v2-broken")!; + assert.equal(after.lastSyncOk, false); + assert.doesNotMatch(after.lastError ?? "", /protocol v1/, "reporting 'update that peer' would send the user after the wrong problem"); + } finally { + server.close(); + } +}); diff --git a/src/sync.test.ts b/src/sync.test.ts index be3f6bb..bf57431 100644 --- a/src/sync.test.ts +++ b/src/sync.test.ts @@ -3,14 +3,18 @@ import { mkdtemp, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { test } from "node:test"; -import { createTodo, touch } from "./mutations.js"; +import { createTodo, tombstoneDelete, touch } from "./mutations.js"; import type { Todo, TodoStore } from "./types.js"; const originalDataDirectory = process.env.DOCKET_DATA_DIR; const dataDirectory = await mkdtemp(join(tmpdir(), "docket-sync-test-")); process.env.DOCKET_DATA_DIR = dataDirectory; const { + checkPairingRateLimit, confirmProof, + createInvite, + generateShortCode, + redeemInvite, decryptSyncPayload, encryptSyncPayload, isSyncProtocolCompatible, @@ -30,7 +34,7 @@ test.after(() => { }); function emptyStore(): TodoStore { - return { formatVersion: 5, nextId: 1, todos: [], deletedUuids: [] }; + return { formatVersion: 8, nextId: 1, todos: [], deletedUuids: [], seqCounter: 0 }; } function payloadFrom(todos: Todo[]): SyncPayload { @@ -64,13 +68,15 @@ test("mergeSyncPayload: two independent edits to DIFFERENT fields both survive ( await new Promise((r) => setTimeout(r, 2)); const localItem = local.todos[0]; localItem.priority = "high"; - touch(localItem, "device-a", "A", ["priority"]); + touch(local, localItem, "device-a", "A", ["priority"]); // Remote (device B) side: same base item, edits description instead — after A's edit. await new Promise((r) => setTimeout(r, 2)); + const remote = emptyStore(); const remoteItem = structuredClone(base); + remote.todos = [remoteItem]; remoteItem.description = "added on B"; - touch(remoteItem, "device-b", "B", ["description"]); + touch(remote, remoteItem, "device-b", "B", ["description"]); const result = mergeSyncPayload(local, payloadFrom([remoteItem]), "device-b"); assert.equal(result.updated, 1); @@ -86,12 +92,14 @@ test("mergeSyncPayload: a genuine same-field conflict (both sides independently local.todos = [structuredClone(base)]; await new Promise((r) => setTimeout(r, 2)); local.todos[0].title = "Local edit"; - touch(local.todos[0], "device-a", "A", ["title"]); + touch(local, local.todos[0], "device-a", "A", ["title"]); await new Promise((r) => setTimeout(r, 2)); + const remote = emptyStore(); const remoteItem = structuredClone(base); + remote.todos = [remoteItem]; remoteItem.title = "Remote edit"; - touch(remoteItem, "device-b", "RemoteBox", ["title"]); + touch(remote, remoteItem, "device-b", "RemoteBox", ["title"]); mergeSyncPayload(local, payloadFrom([remoteItem]), "device-b"); assert.equal(local.todos[0].title, "Remote edit", "the newer (remote) edit should win the conflict"); @@ -108,9 +116,11 @@ test("mergeSyncPayload: adopting a field the local side never touched is NOT rec const local = emptyStore(); local.todos = [structuredClone(base)]; // local never edits description + const remote = emptyStore(); const remoteItem = structuredClone(base); + remote.todos = [remoteItem]; remoteItem.description = "added on B"; - touch(remoteItem, "device-b", "B", ["description"]); + touch(remote, remoteItem, "device-b", "B", ["description"]); mergeSyncPayload(local, payloadFrom([remoteItem]), "device-b"); assert.equal(local.todos[0].description, "added on B"); @@ -124,7 +134,7 @@ test("mergeSyncPayload: a field last-touched more recently locally is NOT overwr const local = emptyStore(); local.todos = [structuredClone(base)]; await new Promise((r) => setTimeout(r, 5)); - touch(local.todos[0], "device-a", "A", ["title"]); + touch(local, local.todos[0], "device-a", "A", ["title"]); local.todos[0].title = "Edited locally, later"; // Remote's copy is the OLD version (its title field was never touched after creation). @@ -170,7 +180,7 @@ test("mergeSyncPayload: a remote tombstone deletes a local item that hasn't chan const tombstonePayload: SyncPayload = { todos: [], - deletedUuids: [{ uuid: item.uuid, deletedAt: new Date(Date.now() + 1000).toISOString(), deviceId: "device-b" }], + deletedUuids: [{ uuid: item.uuid, deletedAt: new Date(Date.now() + 1000).toISOString(), deviceId: "device-b", localSeq: 1 }], serverTime: new Date().toISOString(), protocolVersion: 1, }; @@ -181,7 +191,7 @@ test("mergeSyncPayload: a remote tombstone deletes a local item that hasn't chan test("mergeSyncPayload: a tombstone months old is NOT purged (regression: a long-offline peer must still see it and not resurrect the item)", () => { const local = emptyStore(); - const veryOldTombstone = { uuid: "some-uuid", deletedAt: new Date(Date.now() - 200 * 24 * 60 * 60_000).toISOString(), deviceId: "device-a" }; + const veryOldTombstone = { uuid: "some-uuid", deletedAt: new Date(Date.now() - 200 * 24 * 60 * 60_000).toISOString(), deviceId: "device-a", localSeq: 1 }; local.deletedUuids = [veryOldTombstone]; mergeSyncPayload(local, payloadFrom([]), "device-b"); // an unrelated merge shouldn't sweep old tombstones as a side effect assert.deepEqual(local.deletedUuids, [veryOldTombstone]); @@ -189,7 +199,7 @@ test("mergeSyncPayload: a tombstone months old is NOT purged (regression: a long test("mergeSyncPayload: an edit AFTER a peer's delete resurrects the item (edit-after-delete wins)", () => { const local = emptyStore(); - local.deletedUuids = [{ uuid: "some-uuid", deletedAt: "2020-01-01T00:00:00.000Z", deviceId: "device-a" }]; + local.deletedUuids = [{ uuid: "some-uuid", deletedAt: "2020-01-01T00:00:00.000Z", deviceId: "device-a", localSeq: 1 }]; const remoteItem: Todo = { id: 1, @@ -212,6 +222,8 @@ test("mergeSyncPayload: an edit AFTER a peer's delete resurrects the item (edit- createdAt: "2020-01-01T00:00:00.000Z", updatedAt: "2026-01-01T00:00:00.000Z", // long after the tombstone revision: 1, + localSeq: 1, + workspace: null, fieldTimestamps: {}, completedAt: null, deviceId: "device-b", @@ -363,3 +375,116 @@ test("isSyncProtocolCompatible: the current minimum version and anything newer a test("isSyncProtocolCompatible: a version below the minimum is rejected", () => { assert.equal(isSyncProtocolCompatible(MIN_COMPATIBLE_SYNC_PROTOCOL_VERSION - 1), false); }); + +// --- Boundaries the mutation sweep found unguarded --------------------------------------- + +test("verifySyncRequest: the replay window is exclusive at its edge, and a request just outside is refused", () => { + // Killed mutant: `Math.abs(now - ts) > SIGNATURE_WINDOW_MS` → `>=`. This is the replay + // guard: a captured request must stop being accepted once it is older than the window, + // and the boundary is the only interesting part of a comparison like this. + const secret = "a".repeat(64); + const sign = (timestamp: string) => signSyncRequest(secret, "device-a", "0", timestamp); + + const now = Date.now(); + const justInside = new Date(now - (2 * 60_000 - 2_000)).toISOString(); + assert.equal(verifySyncRequest(secret, "device-a", "0", justInside, sign(justInside)), true, "a fresh request was refused"); + + const wellOutside = new Date(now - 10 * 60_000).toISOString(); + assert.equal(verifySyncRequest(secret, "device-a", "0", wellOutside, sign(wellOutside)), false, "a stale request was replayable"); + + const fromTheFuture = new Date(now + 10 * 60_000).toISOString(); + assert.equal(verifySyncRequest(secret, "device-a", "0", fromTheFuture, sign(fromTheFuture)), false, "the window must be two-sided"); + + assert.equal(verifySyncRequest(secret, "device-a", "0", "not-a-timestamp", sign("not-a-timestamp")), false); +}); + +test("checkPairingRateLimit: allows exactly the documented number of attempts, then refuses", () => { + // Killed mutants: `entry.count <= PAIR_RATE_LIMIT` → `<`, and the window comparisons. + // This is what makes the 6-character pairing code impractical to brute-force; an + // off-by-one either locks out a legitimate retry or widens the attack by one guess a + // window, and nothing else in the suite pins the number. + const ip = `10.0.0.${Math.floor(Math.random() * 200) + 1}`; + const allowed: boolean[] = []; + for (let i = 0; i < 10; i++) allowed.push(checkPairingRateLimit(ip)); + + assert.deepEqual(allowed.slice(0, 8), Array(8).fill(true), "a legitimate run of attempts was cut short"); + assert.deepEqual(allowed.slice(8), [false, false], "attempts past the limit were still allowed"); +}); + +test("checkPairingRateLimit: a different source address has its own budget", () => { + const a = `10.1.0.${Math.floor(Math.random() * 200) + 1}`; + const b = `10.2.0.${Math.floor(Math.random() * 200) + 1}`; + for (let i = 0; i < 9; i++) checkPairingRateLimit(a); + assert.equal(checkPairingRateLimit(a), false, "precondition: the first address is now blocked"); + assert.equal(checkPairingRateLimit(b), true, "one attacker must not lock out everyone else"); +}); + +test("redeemInvite: a token is one-time, and an unknown token is refused", () => { + // Killed mutants around the invite's expiry comparison. A token that survives redemption + // is a token that can be replayed by anyone who saw it over the shoulder. + const { token } = createInvite(); + assert.equal(redeemInvite(token), true); + assert.equal(redeemInvite(token), false, "the invite was redeemable twice"); + assert.equal(redeemInvite("ZZZZZZ"), false); + assert.equal(redeemInvite(token.toLowerCase()), false, "a consumed token must stay consumed however it is cased"); +}); + +test("mergeSyncPayload: a tombstone identical to the one we hold is not re-adopted", () => { + // Killed mutant: `remoteTomb.deletedAt > existingTombstone.deletedAt` → `>=`. Adopting an + // identical tombstone stamps a fresh sequence number for no change at all, which makes + // two peers re-notify each other about the same deletion forever. + const store = emptyStore(); + const item = createTodo(store, { title: "doomed", agent: null, session: null }, "device-a", "A"); + tombstoneDelete(store, item, "device-a"); + const tombstone = { ...store.deletedUuids[0] }; + const counterBefore = store.seqCounter; + + mergeSyncPayload(store, { todos: [], deletedUuids: [tombstone], serverTime: new Date().toISOString(), protocolVersion: 2 }, "peer"); + assert.equal(store.seqCounter, counterBefore, "re-receiving our own tombstone burned a sequence number"); + assert.equal(store.deletedUuids.length, 1, "and duplicated it"); +}); + +test("generateShortCode: the pairing code has the length and charset the brute-force argument depends on", () => { + // Killed mutant: `CODE_LENGTH = 6` → anything else. Six characters from a 32-symbol + // unambiguous set is ~1.07e9 combinations; together with the 5-minute single-use TTL and + // the rate limit above, that is the whole reason a pairing code is safe to read aloud. + // Change either half and the argument stops holding, silently. + const codes = Array.from({ length: 200 }, () => generateShortCode()); + for (const code of codes) { + assert.equal(code.length, 6, `pairing code "${code}" is not 6 characters`); + assert.match(code, /^[23456789ABCDEFGHJKMNPQRSTUVWXYZ]{6}$/, `"${code}" uses characters outside the unambiguous set`); + } + // 0/O and 1/I/L are excluded because a human reads this across a room; if they reappear, + // the code is harder to transcribe rather than more secure. + assert.ok(!codes.join("").match(/[01OIL]/), "an easily-misread character entered the charset"); + assert.ok(new Set(codes).size > 190, "codes are not being drawn from the space randomly"); +}); + +test("mergeSyncPayload: a tombstone stamped at exactly the item's updatedAt still deletes it", () => { + // Killed mutant: `local.updatedAt <= effective.deletedAt` → `<`. + // + // Two places decide the same tie and MUST agree: this one applies an incoming deletion, + // and the todos loop above refuses to re-insert an item its tombstone covers + // (`tombstone.deletedAt >= remote.updatedAt`). Both give an exact tie to the tombstone. If + // only one flips, the same record is deleted by one path and resurrected by the other on + // every tick — the item flickers in and out of the list forever instead of settling. + const store = emptyStore(); + const item = createTodo(store, { title: "tie", agent: null, session: null }, "device-a", "A"); + const exactTie = item.updatedAt; + + const result = mergeSyncPayload( + store, + { todos: [], deletedUuids: [{ uuid: item.uuid, deletedAt: exactTie, deviceId: "device-b", localSeq: 1 }], serverTime: new Date().toISOString(), protocolVersion: 2 }, + "device-b", + ); + assert.equal(result.deleted, 1, "a deletion at the same instant as the last edit must win the tie"); + assert.equal(store.todos.length, 0); + + // ...and the other half of the rule: the peer re-sending that same item must not undo it. + mergeSyncPayload( + store, + { todos: [structuredClone(item)], deletedUuids: [], serverTime: new Date().toISOString(), protocolVersion: 2 }, + "device-b", + ); + assert.equal(store.todos.length, 0, "the item came back — the two tie rules disagree and it will flicker forever"); +}); diff --git a/src/sync.transitive.test.ts b/src/sync.transitive.test.ts new file mode 100644 index 0000000..3cebe78 --- /dev/null +++ b/src/sync.transitive.test.ts @@ -0,0 +1,135 @@ +import assert from "node:assert/strict"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; +import { applyEdits, createTodo, stampSeq } from "./mutations.js"; +import type { TodoStore } from "./types.js"; + +const originalDataDirectory = process.env.DOCKET_DATA_DIR; +const dataDirectory = await mkdtemp(join(tmpdir(), "docket-transitive-test-")); +process.env.DOCKET_DATA_DIR = dataDirectory; +const { buildSyncPayload, mergeSyncPayload } = await import("./sync.js"); + +test.after(() => { + if (originalDataDirectory === undefined) delete process.env.DOCKET_DATA_DIR; + else process.env.DOCKET_DATA_DIR = originalDataDirectory; + return rm(dataDirectory, { recursive: true, force: true }); +}); + +function emptyStore(): TodoStore { + return { formatVersion: 8, nextId: 1, todos: [], deletedUuids: [], seqCounter: 0 }; +} + +/** + * One peer pulling from another, entirely in memory: the puller's cursor into the + * remote's sequence space in, the puller's new cursor out. No HTTP, no crypto — the + * delivery rule is the thing under test, and standing up two servers to exercise it + * would only add ways for the test to fail for unrelated reasons. + */ +function pull(from: TodoStore, into: TodoStore, cursor: number, peerId: string): number { + const payload = buildSyncPayload(from, cursor); + mergeSyncPayload(into, payload, peerId); + return payload.maxSeq ?? cursor; +} + +/** + * The bug this whole format bump exists for. `updatedAt` used to do two jobs at once — + * "when did the author change this?" (merge resolution) and "what have I not seen yet?" + * (delivery cursor) — and merging copies the AUTHOR's updatedAt onto the local record. + * So an item that reaches B second-hand lands in B's store already timestamped in A's + * past, underneath A's cursor for B, and A never hears about it at all. + * + * A and C are deliberately NOT paired: B is the only path between them, which is exactly + * the topology (laptop ↔ desktop ↔ work machine) where this silently loses real edits. + */ +test("sync: an edit made on C reaches A through B, even though A and C were never paired", () => { + const a = emptyStore(); + const b = emptyStore(); + const c = emptyStore(); + + // C creates and edits an item. Nothing else in the mesh knows about it yet. + const x = createTodo(c, { title: "written on C", agent: "codex", session: "s" }, "device-c", "C"); + applyEdits(c, x, { description: "edited on C" }, "codex", "device-c", "C"); + + // A pulls from B first, while B is still empty — its cursor for B advances anyway. + // (This is the step that used to poison A: the cursor moved past "now" without B + // having anything to say yet.) + let aCursorForB = pull(b, a, 0, "device-b"); + + // B then learns about the item from C. + const bCursorForC = pull(c, b, 0, "device-c"); + assert.equal(b.todos.length, 1, "B must have received the item from C"); + assert.ok(bCursorForC > 0); + + // A pulls from B again. The item is new to A, and must arrive. + aCursorForB = pull(b, a, aCursorForB, "device-b"); + + const received = a.todos.find((t) => t.uuid === x.uuid); + assert.ok(received, "A never received C's item — a third device's edit was lost in transit through B"); + assert.equal(received.description, "edited on C"); +}); + +test("sync: accepting a peer's change is a local write and gets a local sequence number", () => { + const a = emptyStore(); + const b = emptyStore(); + + const x = createTodo(b, { title: "from B", agent: null, session: null }, "device-b", "B"); + pull(b, a, 0, "device-b"); + + const merged = a.todos.find((t) => t.uuid === x.uuid)!; + assert.ok(merged.localSeq > 0, "an inserted record must be stamped with THIS device's next sequence number"); + assert.equal(merged.localSeq, a.seqCounter, "and that number must be the store's current high-water mark"); +}); + +test("sync: localSeq is per-device and never travels between devices", () => { + const a = emptyStore(); + const b = emptyStore(); + + // Give A a head start so the two stores' counters are genuinely out of step. + for (let i = 0; i < 5; i++) createTodo(a, { title: `local ${i}`, agent: null, session: null }, "device-a", "A"); + const x = createTodo(b, { title: "from B", agent: null, session: null }, "device-b", "B"); + assert.equal(x.localSeq, 1); + + pull(b, a, 0, "device-b"); + const merged = a.todos.find((t) => t.uuid === x.uuid)!; + assert.equal(merged.localSeq, 6, "the record takes A's next number, not the 1 it carried from B"); +}); + +/** + * A second deletion of an item that was already deleted once and then resurrected by a + * later edit. The tombstone for that uuid already exists locally, so the "stamp every + * newly added tombstone" rule doesn't fire — and without a sequence number the newer + * deletion never leaves this device. The third peer keeps comparing its edits against the + * ORIGINAL deletedAt, decides its copy is newer, and resurrects the item forever. + */ +test("sync: a LATER deletion of an already-tombstoned item is adopted, sequenced, and handed on", () => { + const a = emptyStore(); + const b = emptyStore(); + const c = emptyStore(); + + // Everyone has X. + const x = createTodo(c, { title: "contested", agent: null, session: null }, "device-c", "C"); + let bFromC = pull(c, b, 0, "device-c"); + let aFromB = pull(b, a, 0, "device-b"); + assert.equal(a.todos.length, 1); + + // C deletes it, then a later edit elsewhere resurrects it — so every store now holds + // BOTH the item and a tombstone for its uuid. + c.deletedUuids.push({ uuid: x.uuid, deletedAt: "2026-01-01T00:00:00.000Z", deviceId: "device-c", localSeq: 0 }); + stampSeq(c, c.deletedUuids[0]); + bFromC = pull(c, b, bFromC, "device-c"); + aFromB = pull(b, a, aFromB, "device-b"); + assert.equal(a.deletedUuids.length, 1, "A holds the first tombstone"); + assert.equal(a.todos.length, 1, "…and still holds the item, whose edit is newer"); + + // C deletes it again, later than any edit. This must reach A through B. + const secondDeletion = new Date(Date.now() + 60_000).toISOString(); + c.deletedUuids.push({ uuid: x.uuid, deletedAt: secondDeletion, deviceId: "device-c", localSeq: 0 }); + stampSeq(c, c.deletedUuids.at(-1)!); + bFromC = pull(c, b, bFromC, "device-c"); + assert.equal(b.todos.length, 0, "B applies the newer deletion"); + + aFromB = pull(b, a, aFromB, "device-b"); + assert.equal(a.todos.length, 0, "A never heard about the second deletion and kept a deleted item"); +}); diff --git a/src/sync.ts b/src/sync.ts index 9d55215..cc4272e 100644 --- a/src/sync.ts +++ b/src/sync.ts @@ -1,8 +1,8 @@ import { createHmac, randomInt, timingSafeEqual } from "node:crypto"; import { decryptWithKey, encryptWithKey } from "./crypto.js"; -import { pushHistory } from "./history.js"; +import { dedupeHistory, pushHistory } from "./history.js"; import { log } from "./log.js"; -import { FIELD_KEYS, isSafeUrl, type FieldKey } from "./mutations.js"; +import { FIELD_KEYS, isSafeUrl, stampSeq, type FieldKey } from "./mutations.js"; import { loadPeers, markPeerSynced } from "./peers.js"; import type { Peer, Todo, TodoStore, Tombstone } from "./types.js"; @@ -10,7 +10,10 @@ const INVITE_TTL_MS = 5 * 60_000; // one-time pairing token, 5 minutes const OUTGOING_TTL_MS = 5 * 60_000; // give up waiting for approval after 5 minutes const INCOMING_TTL_MS = 5 * 60_000; // an incoming request nobody approved/denied in time disappears, rather than sitting forever for a stale click later const SIGNATURE_WINDOW_MS = 2 * 60_000; // reject sync requests with a timestamp off by more than this (replay protection) -const MAX_SYNC_ITEMS = 2000; // guard against a misbehaving/malicious peer sending an unbounded payload +// Per-ITEM history cap, not a delivery limit: a genuine guard against a peer (buggy or +// hostile) stuffing an unbounded history array into one record. Delivery is bounded by +// PAGE_SIZE and the caller's page loop instead — see pullFromPeer. +const MAX_HISTORY_ENTRIES = 2000; const PAIR_RATE_LIMIT = 8; // pairing-request attempts... const PAIR_RATE_WINDOW_MS = 5 * 60_000; // ...per source IP, per this window @@ -209,8 +212,13 @@ export function verifySyncRequest( * A peer that doesn't send its version at all predates this negotiation entirely — * treated as compatible (there's nothing to compare against), never rejected outright. */ -export const SYNC_PROTOCOL_VERSION = 1; -/** The oldest peer protocolVersion this build still knows how to talk to. */ +export const SYNC_PROTOCOL_VERSION = 2; +/** The oldest peer protocolVersion this build still knows how to talk to. Deliberately still + * 1 for v3.0: a v1 peer is degraded (see the fallback in pullFromPeer, which says so out + * loud on the peer record) but not cut off, so upgrading a mesh doesn't have to be atomic. + * This is a shim with an expiry, not furniture — it should move to 2 in the release that + * removes P2P sync, taking the fallback path and MAX_INCOMING_ITEMS's generous ceiling with + * it. */ export const MIN_COMPATIBLE_SYNC_PROTOCOL_VERSION = 1; export function isSyncProtocolCompatible(peerProtocolVersion: number | null | undefined): boolean { @@ -223,6 +231,82 @@ export interface SyncPayload { deletedUuids: Tombstone[]; serverTime: string; protocolVersion: number; + /** How far into THIS store's localSeq space the page reaches. The caller's new cursor — + * and only ever a point everything below which was actually sent. Absent from a v1 peer's + * response, which is how the client detects one (see pullFromPeer). */ + maxSeq?: number; + /** True when records above `maxSeq` are still waiting. The caller loops rather than + * assuming one page is the whole story. */ + hasMore?: boolean; + /** Which incarnation of the sender's store `maxSeq` counts in — see getStoreEpoch in + * storage.ts. A caller holding a cursor from a different epoch must discard it: the + * sender restored a backup and its counter went backwards, so every number the caller + * remembers now points past records it has never seen. */ + epoch?: string; +} + +/** One page of records per sync response. Chosen for a payload that stays comfortably inside one encrypted response body, not for any protocol reason — the caller pages until `hasMore` is false, so the exact value is a tuning knob, not a limit on what can sync. */ +export const PAGE_SIZE = 500; + +/** + * Hard ceiling on what ONE response may put into the store. Deliberately far above + * PAGE_SIZE: this is a guard against a peer (buggy or hostile) sending an unbounded + * payload, not a delivery limit. Conflating the two is what made the old cap dangerous — + * a v1 peer doesn't page at all, so clamping its response to a page size would silently + * drop the tail exactly the way v3.0 exists to stop. `mergeSyncPayload` reports when it + * had to clamp so the caller can refuse to advance its cursor past the gap. + */ +export const MAX_INCOMING_ITEMS = 20_000; + +/** + * The pre-v8 timestamp cursor, kept verbatim for peers still on sync protocol v1. It is the + * buggy path — a record merged in from a third device carries that device's older + * `updatedAt` and falls below this filter — which is exactly why the client that receives + * one of these responses flags the peer instead of pretending the sync was clean. + */ +export function buildLegacySyncPayload(store: TodoStore, since: string): SyncPayload { + return { + todos: store.todos.filter((t) => t.updatedAt > since), + deletedUuids: (store.deletedUuids ?? []).filter((t) => t.deletedAt > since), + serverTime: new Date().toISOString(), + protocolVersion: SYNC_PROTOCOL_VERSION, + }; +} + +/** + * Builds the response a peer's GET /api/sync gets back. Lives here rather than inline in + * web/api.ts's route handler so the delivery rule (what a peer is and isn't told about) + * is testable without standing up an HTTP server, and so the client half in this same + * file can be reasoned about next to it. + * + * `maxSeq` is the whole point: it is the highest sequence number this page can PROMISE is + * fully delivered. Todos and tombstones page independently off the same cursor, so when + * either stream is truncated the promise is capped at that stream's last row — advancing + * to the other stream's (higher) end would step over records the truncated stream still + * owes. Cheap to get right here; impossible to detect later, because the symptom is + * silence. + */ +export function buildSyncPayload(store: TodoStore, sinceSeq: number, epoch?: string): SyncPayload { + const bySeq = (a: { localSeq: number }, b: { localSeq: number }) => a.localSeq - b.localSeq; + const todoCandidates = store.todos.filter((t) => t.localSeq > sinceSeq).sort(bySeq); + const tombCandidates = (store.deletedUuids ?? []).filter((t) => t.localSeq > sinceSeq).sort(bySeq); + const todos = todoCandidates.slice(0, PAGE_SIZE); + const deletedUuids = tombCandidates.slice(0, PAGE_SIZE); + const todosTruncated = todoCandidates.length > PAGE_SIZE; + const tombsTruncated = tombCandidates.length > PAGE_SIZE; + + const ceiling = (page: Array<{ localSeq: number }>, truncated: boolean) => + truncated ? page[page.length - 1].localSeq : (store.seqCounter ?? 0); + + return { + todos, + deletedUuids, + serverTime: new Date().toISOString(), + protocolVersion: SYNC_PROTOCOL_VERSION, + maxSeq: Math.max(sinceSeq, Math.min(ceiling(todos, todosTruncated), ceiling(deletedUuids, tombsTruncated))), + hasMore: todosTruncated || tombsTruncated, + epoch, + }; } /** AES-256-GCM encrypt a sync response with the peer's derived secret, so payload contents aren't plaintext on the LAN. */ @@ -263,7 +347,7 @@ function nullableString(v: unknown): string | null { function sanitizeHistory(entries: unknown[]): Todo["history"] { const out: Todo["history"] = []; - for (const e of entries.slice(0, MAX_SYNC_ITEMS)) { + for (const e of entries.slice(0, MAX_HISTORY_ENTRIES)) { if (typeof e !== "object" || e === null) continue; const h = e as Record; if (typeof h.at !== "string" || !ISO_TIMESTAMP_RE.test(h.at) || typeof h.detail !== "string") continue; @@ -312,6 +396,7 @@ function sanitizeRemoteTodo(t: Todo): Todo { sourceUrl: typeof o.sourceUrl === "string" && isSafeUrl(o.sourceUrl) ? o.sourceUrl : null, agent: nullableString(o.agent), session: nullableString(o.session), + workspace: nullableString(o.workspace), workingAgent: nullableString(o.workingAgent), workingSince: nullableString(o.workingSince), workingSession: nullableString(o.workingSession), @@ -325,6 +410,10 @@ function sanitizeRemoteTodo(t: Todo): Todo { deviceId: nullableString(o.deviceId), deviceName: nullableString(o.deviceName), history: sanitizeHistory(t.history), + // Deliberately NOT copied from the wire: a peer's sequence numbers are meaningless in + // this store, and adopting one would put the record at an arbitrary point in our own + // delivery order. mergeSyncPayload stamps it from our counter on the way in. + localSeq: 0, }; } @@ -333,7 +422,7 @@ function sanitizeTombstone(t: unknown): Tombstone | null { if (typeof t !== "object" || t === null) return null; const o = t as Record; if (typeof o.uuid !== "string" || typeof o.deletedAt !== "string") return null; - return { uuid: o.uuid, deletedAt: o.deletedAt, deviceId: nullableString(o.deviceId) }; + return { uuid: o.uuid, deletedAt: o.deletedAt, deviceId: nullableString(o.deviceId), localSeq: 0 }; // localSeq re-stamped locally, same as todos } function fieldTimeOf(t: Todo, field: FieldKey): string { @@ -358,6 +447,26 @@ function remoteWinsTie(remote: Todo, local: Todo): boolean { return (remote.deviceId ?? "") > (local.deviceId ?? ""); } +/** + * Tie-break for one FIELD, and unlike the record-level one above it has to be TOTAL. + * + * `remoteWinsTie` breaks a tie by deviceId, which is fine until both copies carry the same + * deviceId — which happens routinely, because a merged record adopts the deviceId of + * whoever last wrote it, so two peers that merged from the same origin end up agreeing on + * it. Then the comparison returns false in BOTH directions: each side keeps its own value + * and neither ever adopts the other's. That is not a slow convergence, it is a permanent + * split, and no amount of further syncing repairs it. + * + * Falling back to the values themselves fixes it because it is the one thing guaranteed to + * differ when there is anything to resolve, and both sides compute the same answer from it. + * Which value wins is arbitrary; that they AGREE is the entire point. + */ +function remoteWinsFieldTie(remote: Todo, local: Todo, remoteValue: unknown, localValue: unknown): boolean { + const byDevice = (remote.deviceId ?? "").localeCompare(local.deviceId ?? ""); + if (byDevice !== 0) return byDevice > 0; + return JSON.stringify(remoteValue ?? null) > JSON.stringify(localValue ?? null); +} + /** Copies whichever fields the remote touched more recently onto `local`, field by field, so two independent edits to DIFFERENT fields both survive instead of one whole-record timestamp clobbering the other. Returns whether anything changed. */ function mergeTodoFields(local: Todo, remote: Todo): boolean { let changed = false; @@ -369,13 +478,16 @@ function mergeTodoFields(local: Todo, remote: Todo): boolean { for (const field of FIELD_KEYS) { const remoteTime = fieldTimeOf(remote, field); const localTime = fieldTimeOf(local, field); - if (remoteTime > localTime || (remoteTime === localTime && remoteWinsTie(remote, local))) { - const localValue = (local as unknown as Record)[field]; - const remoteValue = (remote as unknown as Record)[field]; + const localValue = (local as unknown as Record)[field]; + const remoteValue = (remote as unknown as Record)[field]; + if (remoteTime > localTime || (remoteTime === localTime && remoteWinsFieldTie(remote, local, remoteValue, localValue))) { if (field in local.fieldTimestamps && localValue !== remoteValue) conflictsResolved.push(field); (local as unknown as Record)[field] = remoteValue; local.fieldTimestamps[field] = remoteTime; - changed = true; + // Only a different VALUE counts as a change. Adopting a newer timestamp for a value + // we already hold is bookkeeping, and reporting it as a change would stamp a new + // sequence number and bounce the identical record around the mesh for another hop. + if (localValue !== remoteValue) changed = true; } } if (remote.updatedAt > local.updatedAt || (remote.updatedAt === local.updatedAt && remoteWinsTie(remote, local))) { @@ -405,50 +517,97 @@ export function mergeSyncPayload( store: TodoStore, payload: SyncPayload, peerId: string, -): { inserted: number; updated: number; deleted: number } { +): { inserted: number; updated: number; deleted: number; truncated: boolean } { let inserted = 0; let updated = 0; let deleted = 0; store.deletedUuids = store.deletedUuids ?? []; const localTombstones = new Map(store.deletedUuids.map((t) => [t.uuid, t])); - - const incomingTodos = Array.isArray(payload.todos) - ? payload.todos.slice(0, MAX_SYNC_ITEMS).filter(isPlausibleTodo).map(sanitizeRemoteTodo) - : []; - const incomingTombstones = Array.isArray(payload.deletedUuids) - ? payload.deletedUuids - .slice(0, MAX_SYNC_ITEMS) - .map(sanitizeTombstone) - .filter((t): t is Tombstone => t !== null) - : []; + // Indexed once rather than scanned per incoming record. A page is up to PAGE_SIZE + // records and this runs inside the store's cross-process lock, so an O(incoming × store) + // scan here doesn't just cost this merge — it holds every other docket process on the + // machine behind it for the duration. + const localByUuid = new Map(store.todos.map((t) => [t.uuid, t])); + + const rawTodos = Array.isArray(payload.todos) ? payload.todos : []; + const rawTombstones = Array.isArray(payload.deletedUuids) ? payload.deletedUuids : []; + // Truncation here is never routine: a v2 peer pages, and a v1 peer sends its whole + // backlog at once, which MAX_INCOMING_ITEMS is sized to accept. Reported to the caller + // because on the legacy path it is the difference between "everything the peer offered + // landed" and "the cursor must not move". + const truncated = rawTodos.length > MAX_INCOMING_ITEMS || rawTombstones.length > MAX_INCOMING_ITEMS; + const incomingTodos = rawTodos.slice(0, MAX_INCOMING_ITEMS).filter(isPlausibleTodo).map(sanitizeRemoteTodo); + const incomingTombstones = rawTombstones + .slice(0, MAX_INCOMING_ITEMS) + .map(sanitizeTombstone) + .filter((t): t is Tombstone => t !== null); + + // The newest author-clock value this merge actually took in. The legacy (protocol v1) + // cursor is a timestamp, and advancing it to the peer's "now" would step over anything + // the merge didn't reach; advancing it to this instead can only ever under-advance, + // which costs a re-fetch rather than a lost record. + const removedUuids = new Set(); for (const remote of incomingTodos) { const tombstone = localTombstones.get(remote.uuid); if (tombstone && tombstone.deletedAt >= remote.updatedAt) continue; // deleted locally after (or at) the remote edit — stays deleted - const local = store.todos.find((t) => t.uuid === remote.uuid); + const local = localByUuid.get(remote.uuid); if (!local) { - store.todos.push({ ...remote, id: store.nextId }); + const insertedTodo = { ...remote, id: store.nextId }; + stampSeq(store, insertedTodo); + store.todos.push(insertedTodo); + localByUuid.set(insertedTodo.uuid, insertedTodo); store.nextId += 1; inserted += 1; continue; } - if (mergeTodoFields(local, remote)) updated += 1; + // The actual fix for transitive propagation: accepting someone else's change is a + // LOCAL write. Without a fresh local sequence number the record keeps the author's + // older updatedAt, sits below a third peer's cursor, and is never handed on. + const changed = mergeTodoFields(local, remote); + if (changed) updated += 1; + // ...and so is REFUSING one. If our copy still differs from what the peer just sent, + // then their copy is stale and they don't know it: their cursor has already moved past + // our record, so they will never ask for it again, and both sides keep their own value + // forever. A merge is a conversation — winning it is exactly when the other side most + // needs to hear from us. Once both agree there is nothing left to differ on, this stops + // firing, so it settles rather than ping-ponging. + const stillDiffers = FIELD_KEYS.some( + (field) => (local as unknown as Record)[field] !== (remote as unknown as Record)[field], + ); + if (changed || stillDiffers) stampSeq(store, local); } for (const remoteTomb of incomingTombstones) { - const local = store.todos.find((t) => t.uuid === remoteTomb.uuid); const existingTombstone = localTombstones.get(remoteTomb.uuid); if (!existingTombstone) { + stampSeq(store, remoteTomb); store.deletedUuids.push(remoteTomb); localTombstones.set(remoteTomb.uuid, remoteTomb); + } else if (remoteTomb.deletedAt > existingTombstone.deletedAt) { + // A LATER deletion of an item we already have a tombstone for. Adopting it is a + // local write and must be sequenced, or the second deletion never reaches a third + // device: it keeps comparing edits against the ORIGINAL, older deletedAt, decides + // its copy is newer, and resurrects an item everyone else agreed was gone. Skipping + // this case is what the spec's "stamp every newly ADDED tombstone" rule misses. + existingTombstone.deletedAt = remoteTomb.deletedAt; + existingTombstone.deviceId = remoteTomb.deviceId; + stampSeq(store, existingTombstone); } - if (local && local.updatedAt <= remoteTomb.deletedAt) { - store.todos = store.todos.filter((t) => t.uuid !== remoteTomb.uuid); + const effective = localTombstones.get(remoteTomb.uuid)!; + const local = localByUuid.get(remoteTomb.uuid); + if (local && local.updatedAt <= effective.deletedAt) { + // Collected, not spliced. Filtering the array per deletion rebuilds the whole store + // each time — a bulk delete on a peer turns into O(deletions × store) copying, again + // while holding the lock. + removedUuids.add(remoteTomb.uuid); + localByUuid.delete(remoteTomb.uuid); deleted += 1; } } + if (removedUuids.size > 0) store.todos = store.todos.filter((t) => !removedUuids.has(t.uuid)); // Tombstones are kept indefinitely — NOT purged by age. A device that's been offline // longer than any fixed retention window (a laptop unused for a couple of months, say) @@ -459,19 +618,93 @@ export function mergeSyncPayload( // once every paired peer has confirmed seeing it) would reclaim space safely instead. if (inserted || updated || deleted) log(`sync: merged from peer ${peerId} — +${inserted} ~${updated} -${deleted}`); - return { inserted, updated, deleted }; + if (truncated) log(`sync: peer ${peerId} sent more than ${MAX_INCOMING_ITEMS} records in one response — merged what fit, cursor held back`); + return { inserted, updated, deleted, truncated }; } +/** + * A peer only ever sends the inline preview (see HISTORY_INLINE_MAX), so this merges recent + * entries, not whole logs. That is the honest limit of history over sync: the audit log is + * per-device and complete locally, and what crosses the wire is the tail. Trading a + * complete cross-device log for a write path that doesn't grow without bound is the right + * way round — the log is a diagnostic, and the store is the data. + */ function mergeHistories(a: Todo["history"], b: Todo["history"]): Todo["history"] { - const seen = new Set(); - const merged = [...a, ...b].filter((h) => { - const key = `${h.at}|${h.agent}|${h.action}|${h.detail}`; - if (seen.has(key)) return false; - seen.add(key); - return true; - }); - merged.sort((x, y) => x.at.localeCompare(y.at)); - return merged; + // Deliberately NOT trimmed here. Trimming at merge time destroys local entries that were + // never flushed to the side file — an item below the flush threshold has its whole log + // inline and nowhere else — which would make docs/security.md's "complete on the device + // that produced it" a lie. flushOverflowHistory runs later in the same withStore call and + // is the one place allowed to drop an inline entry, because it has just persisted it. + return dedupeHistory([...a, ...b]); +} + +const EPOCH = "1970-01-01T00:00:00.000Z"; + +/** + * How many pages one sync tick will chase before yielding. A first sync of a very large + * store finishes over several ticks rather than holding the write lock for the whole + * transfer — the cursor is durable between ticks, so stopping early costs a delay, never + * a gap. + */ +export const MAX_PAGES_PER_TICK = 20; + +/** Recorded on the peer alongside a SUCCESSFUL sync — the pull worked, it is just degraded. */ +export const V1_PEER_WARNING = + "peer is on sync protocol v1 — updates from a third device may not reach this one; update that peer"; + +/** The peer says it no longer knows this device (unpaired on their side). Not transient: the caller cleans up locally instead of retrying forever. */ +class PeerUnpairedError extends Error {} +/** The peer would not accept a request signed over a numeric cursor — it predates protocol v2. */ +class PeerPredatesSeqCursorError extends Error {} + +/** + * One GET /api/sync round trip, decrypted. + * + * `cursor.value` goes into the `since` slot of the signature whichever parameter carries + * it, so the signed material stays exactly `deviceId|since|timestamp` — three fields, as + * v1 peers expect. Adding a fourth would make every not-yet-updated peer reject us for the + * whole release. + * + * NOTE ON THE SPEC: rev 2 says a v1 peer is detected by "response has no maxSeq". It can't + * be. A v1 server reads only `since` (absent here), so it verifies the signature over "" + * while we signed the number — it answers 403, not a 200 without `maxSeq`. Detection is + * therefore: an unexplained 403 on the seq path, or a 200 lacking `maxSeq` (a peer that + * accepted the request but doesn't page). Both are treated identically below. + */ +async function fetchSyncPage( + peer: Peer, + deviceId: string, + cursor: { param: "sinceSeq" | "since"; value: string }, +): Promise { + const timestamp = new Date().toISOString(); + const signature = signSyncRequest(peer.secret, deviceId, cursor.value, timestamp); + // protocolVersion rides outside the signed portion (deviceId|since|timestamp) — it's + // informational, not security-sensitive, and adding it here can't break peers signed + // before this field existed. + const url = + `${peer.url.replace(/\/$/, "")}/api/sync?${cursor.param}=${encodeURIComponent(cursor.value)}` + + `&deviceId=${encodeURIComponent(deviceId)}×tamp=${encodeURIComponent(timestamp)}` + + `&signature=${signature}&protocolVersion=${SYNC_PROTOCOL_VERSION}`; + const res = await fetch(url, { signal: AbortSignal.timeout(8000) }); + if (!res.ok) { + const body = (await res.json().catch(() => ({}))) as { reason?: string; minVersion?: number }; + if (body.reason === "unpaired") throw new PeerUnpairedError("peer no longer knows this device"); + // Revoked is also a 403, and must not be mistaken for a v1 peer below — that would + // cost a pointless second round trip and then report "peer responded 403" instead of + // the reason the user can actually act on. + if (body.reason === "revoked") throw new Error("this peer has revoked this device — re-pair from that device to resume syncing"); + if (body.reason === "protocol-incompatible") { + throw new Error( + `this peer requires sync protocol v${body.minVersion}+ — this device is running an older docket; update it (npm install -g docket@latest) to resume syncing`, + ); + } + // A 403 the peer gave no reason for, on the seq path, is overwhelmingly a v1 peer + // refusing a signature over a cursor parameter it has never heard of. + if (res.status === 403 && cursor.param === "sinceSeq") throw new PeerPredatesSeqCursorError(); + throw new Error(`peer responded ${res.status}`); + } + const body = (await res.json()) as { encrypted: string }; + return decryptSyncPayload(peer.secret, body.encrypted); } /** Returns true if the peer told us plainly that it no longer knows this device (unpaired on its side) — the caller should stop retrying and clean up locally, rather than treating it as a transient failure. */ @@ -481,45 +714,86 @@ export async function pullFromPeer( withStore: (fn: (store: TodoStore) => T | Promise) => Promise, ): Promise { if (peer.revoked) return false; // revoked locally — don't even attempt, see peers.ts revokePeer - const since = peer.lastSyncAt ?? "1970-01-01T00:00:00.000Z"; - const timestamp = new Date().toISOString(); - const signature = signSyncRequest(peer.secret, deviceId, since, timestamp); - // protocolVersion rides outside the signed portion (deviceId|since|timestamp) — it's - // informational, not security-sensitive, and adding it here can't break peers signed - // before this field existed. - const url = `${peer.url.replace(/\/$/, "")}/api/sync?since=${encodeURIComponent(since)}&deviceId=${encodeURIComponent(deviceId)}×tamp=${encodeURIComponent(timestamp)}&signature=${signature}&protocolVersion=${SYNC_PROTOCOL_VERSION}`; try { - const res = await fetch(url, { signal: AbortSignal.timeout(8000) }); - if (!res.ok) { - const body = (await res.json().catch(() => ({}))) as { reason?: string; minVersion?: number }; - if (body.reason === "unpaired") { - log(`sync: peer ${peer.name} (${peer.id}) says it no longer knows this device — was unpaired on their side`); - return true; + // A peer already known to speak v2 skips the probe entirely; only peers that have + // never answered, or last answered v1, pay for the fallback round trip — which is + // exactly the set we want to keep nagging about. + const mayBeV1 = (peer.protocolVersion ?? 1) < 2; + let cursor = peer.lastSeq ?? 0; + let legacyCursor = peer.lastSyncAt ?? EPOCH; + let degraded: string | undefined; + let payload: SyncPayload | undefined; + let pages = 0; + + for (; pages < MAX_PAGES_PER_TICK; pages++) { + try { + payload = await fetchSyncPage(peer, deviceId, { param: "sinceSeq", value: String(cursor) }); + if (payload.maxSeq === undefined) throw new PeerPredatesSeqCursorError(); + } catch (err) { + if (!(err instanceof PeerPredatesSeqCursorError) || !mayBeV1) throw err; + // Degraded, not broken: the timestamp cursor still delivers this peer's OWN edits. + // What it cannot deliver is anything this peer merged in from a third device, which + // is the bug v8 exists to fix — so say so on the peer record rather than syncing + // quietly and letting the user discover the gap themselves. + degraded = V1_PEER_WARNING; + payload = await fetchSyncPage(peer, deviceId, { param: "since", value: legacyCursor }); + } + + // The peer's store is a different incarnation from the one this cursor counts in — + // it restored a backup, so its counter went backwards and every number we remember + // now points past records we have never seen. Start over; the peer re-sends + // everything once, which is the correct outcome for a bulk replacement. + if (payload.epoch && peer.epoch && payload.epoch !== peer.epoch && cursor !== 0) { + log(`sync: peer ${peer.name} (${peer.id}) reports a new store epoch — its cursor is void, re-syncing from scratch`); + cursor = 0; + continue; } - if (body.reason === "protocol-incompatible") { - const msg = `this peer requires sync protocol v${body.minVersion}+ — this device is running an older docket; update it (npm install -g docket@latest) to resume syncing`; - log(`sync: pull from peer ${peer.name} (${peer.id}) rejected — ${msg}`); - await markPeerSynced(peer.id, false, { error: msg }); + + if (!isSyncProtocolCompatible(payload.protocolVersion)) { + const msg = `peer's sync protocol v${payload.protocolVersion} is older than this device supports (min v${MIN_COMPATIBLE_SYNC_PROTOCOL_VERSION}) — update the peer to resume syncing`; + log(`sync: pull from peer ${peer.name} (${peer.id}) skipped — ${msg}`); + await markPeerSynced(peer.id, false, { error: msg, protocolVersion: payload.protocolVersion }); return false; } - throw new Error(`peer responded ${res.status}`); - } - const body = (await res.json()) as { encrypted: string }; - const payload = decryptSyncPayload(peer.secret, body.encrypted); - if (!isSyncProtocolCompatible(payload.protocolVersion)) { - const msg = `peer's sync protocol v${payload.protocolVersion} is older than this device supports (min v${MIN_COMPATIBLE_SYNC_PROTOCOL_VERSION}) — update the peer to resume syncing`; - log(`sync: pull from peer ${peer.name} (${peer.id}) skipped — ${msg}`); - await markPeerSynced(peer.id, false, { error: msg, protocolVersion: payload.protocolVersion }); - return false; + + const merged = await withStore((store) => mergeSyncPayload(store, payload!, peer.id)); + // THE rule this stage exists for: the cursor advances only to what was actually + // merged — never to "wherever the peer is now". + cursor = payload.maxSeq ?? cursor; + // A v1 peer does not page: it answers with everything since the timestamp it was + // given. So the cursor may advance to its `serverTime` exactly when nothing was + // clamped, and must not move at all when something was — otherwise the next request + // starts past records that never landed. Staying in the PEER's clock is the point: + // a merged record can have been authored by a third device, and its `updatedAt` says + // nothing about where this peer's own timeline has reached. + if (degraded && !merged.truncated) legacyCursor = payload.serverTime; + if (!payload.hasMore) break; } - await withStore((store) => { - mergeSyncPayload(store, payload, peer.id); + + // On the v2 path lastSyncAt is display only ("synced 4m ago") and uses the PEER's own + // clock, so the label isn't skewed by a clock disagreement between the machines. On the + // v1 fallback it is still a real cursor, and there it must be the merged-through value + // computed above — never the peer's "now". + const clockSkewMs = payload ? Date.parse(payload.serverTime) - Date.now() : undefined; + await markPeerSynced(peer.id, true, { + cursor: degraded ? legacyCursor : payload?.serverTime, + lastSeq: cursor, + epoch: payload?.epoch, + protocolVersion: payload?.protocolVersion, + clockSkewMs, + error: degraded, }); - // Use the PEER's own clock (what it just told us "now" is), not ours — otherwise clock - // skew between the two machines could permanently blind this cursor to real updates. - const clockSkewMs = Date.parse(payload.serverTime) - Date.now(); - await markPeerSynced(peer.id, true, { cursor: payload.serverTime, protocolVersion: payload.protocolVersion, clockSkewMs }); + if (pages === MAX_PAGES_PER_TICK) { + // Not an error and not a partial write: everything merged is merged and the cursor + // reflects exactly that. The cap only bounds how long one tick can hold the store + // lock; the next tick picks up from here. + log(`sync: peer ${peer.id} still has more after ${pages} pages — continuing next tick`); + } } catch (err) { + if (err instanceof PeerUnpairedError) { + log(`sync: peer ${peer.name} (${peer.id}) says it no longer knows this device — was unpaired on their side`); + return true; + } log(`sync: pull from peer ${peer.name} (${peer.id}) failed: ${(err as Error).message}`); await markPeerSynced(peer.id, false, { error: (err as Error).message }); } diff --git a/src/types.ts b/src/types.ts index fd5c5cc..0cdbbdc 100644 --- a/src/types.ts +++ b/src/types.ts @@ -43,13 +43,29 @@ export interface Todo { deviceName: string | null; /** Append-only audit log: every create/edit/claim/release/complete, by whom (agent, including "web" for manual UI edits). */ history: HistoryEntry[]; + /** Monotonic per-device counter, bumped on EVERY local write to this record — including + * accepting a peer's change during merge. This is the delivery cursor; `updatedAt` is the + * merge resolver. Never mix the two: `updatedAt` describes when the AUTHOR changed the + * record and travels between devices, so a merged record lands in the past and slips + * underneath a third peer's cursor. `localSeq` is meaningful only in the store that + * assigned it and is re-stamped on arrival, never copied off the wire. */ + localSeq: number; + /** Which project/context this item belongs to (see src/workspace.ts). A stable slug, + * resolved from the git remote where possible so the same repo cloned to different paths + * on two machines lands in ONE workspace — which only matters because sync exists. + * Null for pre-v8 items and for anything created with no project context (a bare + * Claude Desktop session, say); nulls stay visible rather than being guessed at. */ + workspace: string | null; } -/** A deletion, recorded so a paired device doesn't resurrect the item on next sync. Pruned after RETENTION (see sync.ts). */ +/** A deletion, recorded so a paired device doesn't resurrect the item on next sync. Kept indefinitely, never purged by age — see the note at the end of mergeSyncPayload in sync.ts for why age-based GC would resurrect deletions. */ export interface Tombstone { uuid: string; deletedAt: string; deviceId: string | null; + /** Same delivery cursor as Todo.localSeq — tombstones page off the same sequence space, + * so a deletion can't be skipped by a cursor that advanced past it. */ + localSeq: number; } /** @@ -64,7 +80,17 @@ export interface Peer { url: string; secret: string; pairedAt: string; + /** Display only ("last synced 4m ago") since v3.0 — NOT a cursor. Delivery is tracked by + * `lastSeq` below; a wall-clock cursor is what made a third device's edits vanish. */ lastSyncAt: string | null; + /** How far into this peer's OWN localSeq space we have merged. Advances only to what was + * actually merged, never to "wherever the peer is now". Absent on peers paired before v3.0 + * and on peers still speaking sync protocol v1, both treated as 0 (full re-sync). */ + lastSeq?: number; + /** Which incarnation of the peer's store `lastSeq` counts in. When the peer reports a + * different one — it restored a backup, so its counter went backwards — the cursor is + * meaningless and resets to 0. Absent until the first sync with a v3.0 peer. */ + epoch?: string; lastSyncOk: boolean; /** Explicitly blocks sync without losing the pairing/secret — see peers.ts revokePeer/restorePeer. Absent on records from before this field existed, treated as false. */ revoked?: boolean; @@ -86,4 +112,8 @@ export interface TodoStore { todos: Todo[]; /** Deletions from this device or merged in from a peer, for tombstone-based sync. */ deletedUuids: Tombstone[]; + /** High-water mark for `localSeq` on this device. Never decreases, never resets — a peer's + * cursor into this store is a number from this counter, so reusing one would silently + * hide the record that got the duplicate. */ + seqCounter: number; } diff --git a/src/web/api.routes.test.ts b/src/web/api.routes.test.ts new file mode 100644 index 0000000..233085c --- /dev/null +++ b/src/web/api.routes.test.ts @@ -0,0 +1,186 @@ +import assert from "node:assert/strict"; +import { once } from "node:events"; +import { mkdtemp, rm } from "node:fs/promises"; +import type { Server } from "node:http"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { test } from "node:test"; + +const originalDataDirectory = process.env.DOCKET_DATA_DIR; +const dataDirectory = await mkdtemp(join(tmpdir(), "docket-api-routes-test-")); +process.env.DOCKET_DATA_DIR = dataDirectory; +const { createWebServer } = await import("./server.js"); +const { LocalTodoRepository } = await import("../repository.js"); + +let server: Server; +let base: string; +const repo = new LocalTodoRepository(); +const context = { agent: "test", session: "s", deviceId: "d", deviceName: "D", workspace: "acme/backend" }; + +test.before(async () => { + server = await createWebServer(); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const address = server.address(); + assert.ok(address && typeof address !== "string"); + base = `http://127.0.0.1:${address.port}`; +}); + +test.after(async () => { + server?.close(); + if (originalDataDirectory === undefined) delete process.env.DOCKET_DATA_DIR; + else process.env.DOCKET_DATA_DIR = originalDataDirectory; + await rm(dataDirectory, { recursive: true, force: true }); +}); + +const api = (path: string, init?: RequestInit) => fetch(`${base}${path}`, init); +const post = (path: string, body: unknown) => + api(path, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify(body) }); + +// --- Happy paths ----------------------------------------------------------------------- + +test("routes: creating, listing, editing and completing an item all round-trip", async () => { + const created = await post("/api/todos", { title: "route test", workspace: "acme/backend", priority: "high" }); + assert.equal(created.status, 201); + const { todo } = (await created.json()) as { todo: { id: number; uuid: string; workspace: string } }; + assert.equal(todo.workspace, "acme/backend", "the dashboard's chosen project must reach the store"); + + const listed = (await (await api("/api/todos")).json()) as { todos: Array<{ id: number; shortId: string }> }; + assert.ok(listed.todos.some((t) => t.id === todo.id)); + assert.ok(listed.todos.every((t) => typeof t.shortId === "string"), "shortId is derived server-side so both surfaces agree"); + + const edited = await api(`/api/todos/${todo.id}`, { + method: "PATCH", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ title: "renamed" }), + }); + assert.equal(edited.status, 200); + + const done = await post(`/api/todos/${todo.id}/complete`, {}); + assert.equal(done.status, 200); +}); + +test("routes: the history endpoint returns the item's full log", async () => { + const { todo } = (await (await post("/api/todos", { title: "with history" })).json()) as { todo: { id: number; uuid: string } }; + await api(`/api/todos/${todo.id}`, { method: "PATCH", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ title: "changed" }) }); + + const res = await api(`/api/todos/${todo.uuid}/history`); + assert.equal(res.status, 200); + const { history } = (await res.json()) as { history: Array<{ action: string }> }; + assert.deepEqual(history.map((h) => h.action), ["created", "edited"]); +}); + +// --- Unknown ids and bad input ----------------------------------------------------------- + +test("routes: an unknown id is a 404 with a JSON body, never a crash or an empty 200", async () => { + for (const path of ["/api/todos/424242", "/api/todos/T-ZZZZZZ/history"]) { + const res = await api(path, { method: "GET" }); + assert.equal(res.status, 404, `${path} responded ${res.status}`); + await assert.doesNotReject(() => res.json()); + } + const patched = await api("/api/todos/424242", { + method: "PATCH", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ title: "x" }), + }); + assert.equal(patched.status, 404); +}); + +test("routes: a malformed body is rejected with a clear status, and changes nothing", async () => { + const before = ((await (await api("/api/todos")).json()) as { todos: unknown[] }).todos.length; + + const notJson = await api("/api/todos", { method: "POST", headers: { "Content-Type": "application/json" }, body: "{not json" }); + assert.ok(notJson.status >= 400 && notJson.status < 500, `expected a 4xx, got ${notJson.status}`); + + const noTitle = await post("/api/todos", { description: "no title here" }); + assert.equal(noTitle.status, 400); + + const blankTitle = await post("/api/todos", { title: " " }); + assert.equal(blankTitle.status, 400, "whitespace is not a title"); + + const after = ((await (await api("/api/todos")).json()) as { todos: unknown[] }).todos.length; + assert.equal(after, before, "a rejected request still created something"); +}); + +test("routes: an unsafe sourceUrl is dropped rather than stored", async () => { + const { todo } = (await (await post("/api/todos", { title: "link", sourceUrl: "javascript:alert(1)" })).json()) as { + todo: { sourceUrl: string | null }; + }; + assert.equal(todo.sourceUrl, null); +}); + +// --- Cross-workspace behaviour ------------------------------------------------------------ + +test("routes: /api/todos is unscoped, and each item reports the project it belongs to", async () => { + await repo.create({ title: "in web" }, { ...context, workspace: "acme/web" }); + const { todos } = (await (await api("/api/todos")).json()) as { todos: Array<{ workspace: string | null }> }; + const workspaces = new Set(todos.map((t) => t.workspace)); + assert.ok(workspaces.size > 1, "the dashboard filters client-side, so the API must hand it every project"); + assert.ok(workspaces.has("acme/web")); +}); + +test("routes: the SessionStart payload is scoped, and unfiled items ride along", async () => { + await repo.create({ title: "unfiled thought" }, { ...context, workspace: null }); + const { text } = (await (await api("/api/hook/session-start?workspace=acme/web")).json()) as { text: string }; + assert.match(text, /in web/, "the requested project's items must be there"); + assert.match(text, /unfiled thought/, "unfiled items stay reachable rather than becoming invisible"); + assert.doesNotMatch(text, /route test/, "another project's items must not be"); +}); + +test("routes: an unknown workspace yields only unfiled items, never every project", async () => { + const { text } = (await (await api("/api/hook/session-start?workspace=nope/nope")).json()) as { text: string }; + // Unfiled items ride along with every scope by design — they are legacy or context-free, + // and hiding them would make a scoped list quietly lose work. What must NOT happen is + // falling back to unscoped, which would put every project in every session. + assert.match(text, /unfiled thought/); + assert.doesNotMatch(text, /route test/, "another project's items leaked into an unknown scope"); + assert.doesNotMatch(text, /in web/); +}); + +// --- Guards ------------------------------------------------------------------------------ + +test("routes: an unrecognised Host header is refused before any route runs (DNS rebinding)", async () => { + // `fetch` refuses to set Host — it is a forbidden header — so this has to go out over a + // raw request, which is also exactly how the attack would arrive. + const { request } = await import("node:http"); + const port = Number(new URL(base).port); + const status = await new Promise((resolve, reject) => { + const req = request({ host: "127.0.0.1", port, path: "/api/todos", method: "GET", headers: { Host: "evil.example.com" } }, (res) => + resolve(res.statusCode ?? 0), + ); + req.on("error", reject); + req.end(); + }); + assert.equal(status, 403, "a malicious site can point its own DNS at loopback; only the Host header distinguishes it"); +}); + +test("routes: a cross-origin mutating request is refused (CSRF)", async () => { + const res = await api("/api/todos", { + method: "POST", + headers: { "Content-Type": "application/json", Origin: "http://evil.example.com" }, + body: JSON.stringify({ title: "forged" }), + }); + assert.equal(res.status, 403); +}); + +test("routes: an unknown path is a JSON 404, not an HTML error page", async () => { + const res = await api("/api/nope"); + assert.equal(res.status, 404); + assert.match(res.headers.get("content-type") ?? "", /application\/json/); +}); + +test("routes: every response carries the security headers", async () => { + for (const path of ["/", "/api/todos", "/api/version"]) { + const res = await api(path); + assert.equal(res.headers.get("x-content-type-options"), "nosniff", `${path} is missing nosniff`); + assert.equal(res.headers.get("x-frame-options"), "DENY", `${path} can be framed`); + } +}); + +test("routes: deleting is reflected immediately in the list", async () => { + const { todo } = (await (await post("/api/todos", { title: "to delete" })).json()) as { todo: { id: number } }; + const removed = await api(`/api/todos/${todo.id}`, { method: "DELETE" }); + assert.equal(removed.status, 200); + const { todos } = (await (await api("/api/todos")).json()) as { todos: Array<{ id: number }> }; + assert.ok(!todos.some((t) => t.id === todo.id)); +}); diff --git a/src/web/api.ts b/src/web/api.ts index e57611a..a11a117 100644 --- a/src/web/api.ts +++ b/src/web/api.ts @@ -11,17 +11,21 @@ import { } from "../access.js"; import { deriveSharedSecret, getDevicePublicKey, getDeviceRole, setDeviceRole } from "../device.js"; import { exportToJson, exportToMarkdown, importFromJson, importFromMarkdown } from "../export.js"; +import { renderSessionStart } from "../format.js"; import { log } from "../log.js"; import { isClaimActive, isSafeUrl, shortId } from "../mutations.js"; import { listEvents, recordCreated, recordResolved } from "../notifications.js"; import { addPeer, loadPeers, peerFingerprint, peerTrustState, removePeer, restorePeer, revokePeer, updatePeerUrl } from "../peers.js"; import { computeAgentPresence } from "../presence.js"; +import { listSessions } from "../sessions.js"; import type { MutationContext } from "../repository.js"; -import { CURRENT_FORMAT_VERSION, readStore, withStore } from "../storage.js"; +import { CURRENT_FORMAT_VERSION, getStoreEpoch, readStore, withStore } from "../storage.js"; import { todoService } from "../todo-service.js"; import { addIncomingRequest, addOutgoingRequest, + buildLegacySyncPayload, + buildSyncPayload, checkPairingRateLimit, confirmProof, createInvite, @@ -39,7 +43,6 @@ import { SYNC_PROTOCOL_VERSION, verifyConfirmProof, verifySyncRequest, - type SyncPayload, } from "../sync.js"; import type { Peer, TodoList, TodoPriority } from "../types.js"; import { addViewer, loadViewers, removeViewer } from "../viewers.js"; @@ -84,9 +87,27 @@ export async function readRawBody(req: IncomingMessage): Promise { return Buffer.concat(chunks).toString("utf8"); } +/** + * A request the CALLER got wrong. Distinguished from every other throw so the server answers + * 4xx rather than 500: a malformed body is not a server fault, and telling a client "my + * mistake, try again" when its own payload is broken sends it into a retry loop over + * something no retry can fix. + */ +export class BadRequestError extends Error { + constructor(message: string) { + super(message); + this.name = "BadRequestError"; + } +} + export async function readJsonBody(req: IncomingMessage): Promise { const raw = await readRawBody(req); - return raw ? JSON.parse(raw) : {}; + if (!raw) return {}; + try { + return JSON.parse(raw); + } catch { + throw new BadRequestError("request body is not valid JSON"); + } } // Exported for reuse by the remote server's own request validation (src/server/routes.ts) — @@ -129,6 +150,7 @@ interface TodoRequestBody { priority?: unknown; dueDate?: unknown; sourceUrl?: unknown; + workspace?: unknown; } function isPrivateNetworkUrl(rawUrl: string): boolean { @@ -179,7 +201,9 @@ export interface ApiContext { /** Every web-originated mutation is attributed to agent "web", one connection, no session token — matching what every route already passed to createTodo/applyEdits/etc. directly before TodoService existed. */ function webContext(ctx: ApiContext): MutationContext { - return { agent: "web", session: null, deviceId: ctx.deviceId, deviceName: ctx.deviceName }; + // No workspace: the dashboard is one shared view across every project, not a checkout, so + // an item typed here is genuinely unfiled unless the caller names a project explicitly. + return { agent: "web", session: null, deviceId: ctx.deviceId, deviceName: ctx.deviceName, workspace: null }; } export async function removePeerAndMaybeRevertRole(id: string, ctx: ApiContext): Promise { @@ -287,6 +311,21 @@ export async function handleApiRoute( return true; } + // 6b. Todos - Full history for one item + // GET /api/todos/:id/history — the card preview ships the last few entries inline with + // the item; this is what the detail panel opens for the rest. Separate route (rather than + // fattening /api/todos) precisely so the list stays cheap: history is the unbounded part. + const historyMatch = url.pathname.match(/^\/api\/todos\/([^/]+)\/history$/); + if (req.method === "GET" && historyMatch) { + const entries = await todoService.history(decodeURIComponent(historyMatch[1])); + if (!entries) { + json(res, 404, { error: "no such todo" }); + return true; + } + json(res, 200, { history: entries }); + return true; + } + // 7. Todos - Create if (req.method === "POST" && url.pathname === "/api/todos") { const body = (await readJsonBody(req)) as TodoRequestBody; @@ -297,6 +336,11 @@ export async function handleApiRoute( } const todo = await todoService.create( { + // The dashboard is one view over every project, so it says which one an item + // belongs to rather than inheriting a project from the process it runs in. Without + // this, typing a todo while a project is selected files it Unfiled and it vanishes + // from the very list it was typed into. + workspace: typeof body.workspace === "string" ? textOrNull(body.workspace) : undefined, title, description: textOrNull(body.description), list: isTodoList(body.list) ? body.list : "todo", @@ -384,6 +428,25 @@ export async function handleApiRoute( return true; } + // 11c. Live agent sessions — which terminals are open right now and where. + // Distinct from /api/presence, which is derived from history and can only say what an + // agent last DID. With a dozen terminals open, "where is it" is the actual question. + if (req.method === "GET" && url.pathname === "/api/sessions") { + json(res, 200, { sessions: await listSessions() }); + return true; + } + + // 11d. SessionStart hook payload — the ONE thing the Claude Code hook fetches. + // Rendered here rather than in the hook process so the wording and the token budget live + // in one place (src/format.ts, enforced by budget.test.ts), and so the hook stays a thin + // HTTP client that never loads the MCP stack or decrypts anything itself. + if (req.method === "GET" && url.pathname === "/api/hook/session-start") { + const scope = url.searchParams.get("workspace"); + const todos = await todoService.list({ filter: "open", workspace: scope || "*" }); + json(res, 200, { text: renderSessionStart(todos, scope || null) }); + return true; + } + // 12. Peers - List if (req.method === "GET" && url.pathname === "/api/peers") { const peers = await loadPeers(); @@ -901,6 +964,11 @@ export async function handleApiRoute( // 30. Peer Sync - Sync Endpoint if (req.method === "GET" && url.pathname === "/api/sync") { const since = url.searchParams.get("since") ?? ""; + // Protocol v2's cursor. Present → the caller signed THIS value in the `since` slot and + // wants seq-paged delivery; absent → a v1 caller on the timestamp path, served exactly + // as before. One endpoint, two cursors, no version negotiation round trip. + const sinceSeqRaw = url.searchParams.get("sinceSeq"); + const signedCursor = sinceSeqRaw ?? since; const callerDeviceId = url.searchParams.get("deviceId") ?? ""; const timestamp = url.searchParams.get("timestamp") ?? ""; const signature = url.searchParams.get("signature") ?? ""; @@ -916,7 +984,7 @@ export async function handleApiRoute( json(res, 403, { error: "this peer has been revoked", reason: "revoked" }); return true; } - if (!verifySyncRequest(peer.secret, callerDeviceId, since, timestamp, signature)) { + if (!verifySyncRequest(peer.secret, callerDeviceId, signedCursor, timestamp, signature)) { json(res, 403, { error: "signature invalid or expired" }); return true; } @@ -929,12 +997,12 @@ export async function handleApiRoute( return true; } const store = await readStore(); - const payload: SyncPayload = { - todos: store.todos.filter((t) => t.updatedAt > since), - deletedUuids: (store.deletedUuids ?? []).filter((t) => t.deletedAt > since), - serverTime: new Date().toISOString(), - protocolVersion: SYNC_PROTOCOL_VERSION, - }; + const sinceSeq = sinceSeqRaw === null ? null : Number(sinceSeqRaw); + if (sinceSeq !== null && !Number.isSafeInteger(sinceSeq)) { + json(res, 400, { error: "sinceSeq must be an integer" }); + return true; + } + const payload = sinceSeq === null ? buildLegacySyncPayload(store, since) : buildSyncPayload(store, sinceSeq, await getStoreEpoch()); json(res, 200, encryptSyncPayload(peer.secret, payload)); return true; } diff --git a/src/web/render.escaping.test.ts b/src/web/render.escaping.test.ts new file mode 100644 index 0000000..8517489 --- /dev/null +++ b/src/web/render.escaping.test.ts @@ -0,0 +1,159 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; +import vm from "node:vm"; +import { PAGE } from "./views.js"; + +/** + * The dashboard's entire client is one inline `", + img: '', + attrBreak: '" onmouseover="alert(1)" x="', + quote: "it's a \"quoted\" thing", + amp: "a & b", + template: "${constructor.constructor('alert(1)')()}", +}; + +function hostileTodo(overrides: Record = {}) { + return { + id: 1, + uuid: "11111111-1111-7111-8111-111111111111", + shortId: "T-AAAAAA", + title: PAYLOADS.script, + description: PAYLOADS.img, + done: false, + list: "todo", + category: PAYLOADS.attrBreak, + priority: null, + dueDate: null, + sourceUrl: null, + agent: PAYLOADS.img, + session: PAYLOADS.quote, + workspace: PAYLOADS.script, + workingAgent: null, + workingSince: null, + workingSession: null, + workingLeaseExpiresAt: null, + workingDeviceId: null, + createdAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-01T00:00:00.000Z", + completedAt: null, + revision: 1, + localSeq: 1, + deviceId: "device-a", + deviceName: PAYLOADS.img, + history: [], + ...overrides, + }; +} + +/** What must never appear in served markup, however the payload was shaped. */ +function assertInert(html: string, where: string): void { + assert.doesNotMatch(html, /"]) { + assertInert(page.sourceLinkHtml(url), `sourceLinkHtml(${url})`); + } +}); + +test("a well-formed item still renders its real content", () => { + const html = page.itemHtml(hostileTodo({ title: "fix token refresh race", category: "VPQ-834", description: null, agent: "codex", deviceName: "Laptop" })); + assert.match(html, /fix token refresh race/); + assert.match(html, /VPQ-834/); +}); diff --git a/src/web/server.test.ts b/src/web/server.test.ts index ed7819c..e4e2b0d 100644 --- a/src/web/server.test.ts +++ b/src/web/server.test.ts @@ -51,3 +51,20 @@ test("hasSameOriginForMutation: falls back to Referer when Origin is absent", () assert.equal(hasSameOriginForMutation(req("DELETE", { host: "192.168.1.5:8787", referer: "http://evil.example.com/page" })), false); assert.equal(hasSameOriginForMutation(req("DELETE", { host: "192.168.1.5:8787", referer: "http://192.168.1.5:8787/" })), true); }); + +/** + * The dashboard's whole client is one big inline ", "vbscript:x", "/etc/passwd"]) { + const html = page.renderMarkdown(`[click me](${href})`); + assertNothingExecutable(html, `renderMarkdown link ${href}`); + assert.doesNotMatch(html, / { + // No whitespace, so the link rule matches the whole thing — and it stays harmless only + // because escapeHtml turned that quote into " before the rule ran. This is the test + // that fails first if the escape ever moves to after the markdown pass. + const html = page.renderMarkdown('[x](https://a"onmouseover="alert(1))'); + assertInert(html, "href breakout"); + assert.doesNotMatch(html, /href="[^"]*"\s*onmouseover/i, "the href attribute was broken out of"); +}); + +test("markdown: a bare URL is linkified, and a bare javascript: URL is not", () => { + assert.match(page.renderMarkdown("see https://example.com/x for more"), / { + const html = page.renderMarkdown('```\n\n```'); + assertInert(html, "fenced code"); + assert.match(html, /
/, "the fence did not produce a code block");
+  assert.match(html, /<img/, "the code sample was dropped rather than shown");
+});
+
+test("markdown: stored text cannot address the internal placeholder table", () => {
+  // mdInline parks finished HTML behind NUL-delimited markers. A description carrying one
+  // could otherwise name a slot and have it substituted in.
+  const html = page.renderMarkdown(`${NUL}0${NUL} and \`real code\``);
+  assertInert(html, "placeholder injection");
+  assert.doesNotMatch(html, new RegExp(NUL), "a NUL marker reached the page");
+  assert.match(html, /real code<\/code>/, "the genuine code span stopped working");
+});
+
+test("markdown: the ordinary syntax people actually type renders", () => {
+  const html = page.renderMarkdown(
+    "# Heading\n\nSome **bold** and *italic* and `code`.\n\n- one\n- two\n\n1. first\n2. second\n\n> quoted\n\n---\n",
+  );
+  assert.match(html, /
Heading<\/h3>/);
+  assert.match(html, /bold<\/strong>/);
+  assert.match(html, /italic<\/em>/);
+  assert.match(html, /