Write down testing rules and prune the suite against them

.changeset/prune-tests-against-rules.md

@@ -0,0 +1,2 @@
+---
+---

.changeset/seven-pots-shout.md

@@ -0,0 +1,2 @@
+---
+---

AGENTS.md

@@ -51,7 +51,7 @@ Plugin, seam, token, or kit UI: read [`docs/plugins.md`](./docs/plugins.md).
 
 ## Testing
 
-Testing or coverage: read [`CONTRIBUTING.md`](./CONTRIBUTING.md). **A perf guard asserts a countable invariant.**
+Writing, reviewing, or pruning a test: read [`CONTRIBUTING.md#testing`](./CONTRIBUTING.md#testing). **A perf guard asserts a countable invariant.**
 
 ## Effect
 

CONTRIBUTING.md

@@ -162,6 +162,96 @@ row. An operator-comped user is a hand-inserted active row with no Stripe ids.
 Keep the founding seat cap in `getCheckoutSessionParams` at checkout-creation
 time. Webhooks and `subscription.list()` resolve plans from that same list.
 
+## Testing
+
+A test earns its place by catching a regression nothing cheaper would catch.
+More green checks are not more confidence: every test costs run time and edit
+time. Write fewer, longer tests. While you build, write whatever tests check
+your work; a later pruning pass decides which ones stay. These rules adapt Kent C. Dodds'
+[testing principles](https://github.com/kentcdodds/kody/blob/main/docs/contributing/testing-principles.md).
+
+### Pick the lightest flavor that can falsify the behavior
+
+| Flavor                              | Use it for                                                                                                                 | Command            |
+| ----------------------------------- | -------------------------------------------------------------------------------------------------------------------------- | ------------------ |
+| Unit, `*.test.ts` beside its module | Pure logic: `tree.ts`, parsers, tokens, the Worker planners and schemas.                                                   | `bun run test`     |
+| CLI, `cli/test/`                    | The built executable against loopback fixtures.                                                                            | `bun run test:cli` |
+| Playwright, `e2e/*.spec.ts`         | A few user-critical journeys, and behavior only a real browser shows: caret, contentEditable, layout, the collection path. | `bun run test:e2e` |
+
+Unit tests reach pure functions; the DOM path belongs to Playwright, because a
+mocked DOM proves only the mock. When an e2e case is really exercising a parser
+or planner, test that function in `bun test` and keep only the journey in the
+browser.
+
+### Write each test as one workflow
+
+- **One test is one workflow**: setup, actions, then every assertion that
+  proves it. Read it like a manual tester's script.
+- Write flat top-level `test(...)` calls with the setup inline, so each test
+  reads top to bottom on its own. For many input-output pairs of one function,
+  use one table test (`test.each`).
+- **Every assertion needs an independent oracle**: a literal, or a value you
+  derive by hand. An expected value from the code under test, an exported
+  constant, or a copy of the production helper agrees with the bug. Write
+  `NOW - 30 * DAY_MS`, not `NOW - RESTORE_WINDOW_MS`.
+- **One behavior, one home.** Test it beside the module that owns it. A
+  re-export inherits its source's tests.
+- Assert on what callers and users rely on. Copy, error message wording,
+  styling, animation timing, and facts the type checker enforces stay unpinned.
+- Test paths a caller can reach. An edge case or fallback that no input can
+  trigger stays untested.
+- An absence assertion runs on a path that can still show the thing: empty
+  versus populated, admin versus user.
+- **A perf guard asserts a countable invariant**, such as a render or
+  registration count, never a wall clock.
+
+### Hold Playwright to a high bar
+
+- Grow an existing journey before you add a spec. Add a spec only when the flow
+  is user-critical and no faster test can cover it.
+- A bug fix gets its regression test at the lowest layer that reproduces it.
+- Locate by role or label (`getByRole`, `getByLabel`); reach for a CSS locator
+  only when no accessible one exists. Press `ControlOrMeta+…`, never a bare
+  `Meta+`.
+- Wait on state (`expect(...)` retries, `expect.poll`), never on a fixed
+  `waitForTimeout`.
+- **e2e does not run in CI.** It is a local pre-PR gate.
+  `bun run test:e2e:app --workers=2` is the clean-signal app-only run;
+  `bun run test:e2e` also runs the isolated real-Worker suites. Use
+  `bun run test:e2e:serial` when chasing a flake. A
+  parallel-contention flake isn't a real failure. The app suite runs its own Vite
+  server on port 3210; kill a zombie or set `E2E_PORT`. For a caret, set the
+  Selection range directly. `toHaveText` normalizes whitespace.
+
+### Keep test seams out of production
+
+Ask: would production keep this code if every test were deleted? If not, it
+belongs in the test file or `e2e/fixtures.ts`. Reach behavior through public
+interfaces, the Effect `TestClock`, and fixture route mocks. Known debt: the
+quick-add resolve gate (`src/components/quick-add.tsx`) and `setClock` in
+`worker/mcp-tools.ts`.
+
+### Prune
+
+A test that was valuable when written is not valuable forever. When asked to
+prune or tighten tests:
+
+1. Read each test in scope against every rule in this section.
+2. Rewrite or move a test that breaks a rule while preserving distinct reachable
+   setups. Delete it only when another test provides equivalent coverage or the
+   path is no longer reachable. Move an e2e case that only exercises logic down
+   to `bun test`. Delete a test that only checked work in progress, such as one
+   proving a removed feature is gone.
+3. Run `bun run test`, plus each e2e spec you changed.
+
+The pass is done when every remaining test satisfies every rule, the PR
+description lists what you deleted, consolidated, and kept (each deletion names
+the rule it broke; each kept test that looks borderline says why it stays), and
+the gates are green. Line reduction is not a completion criterion. Lunora
+retirement tests leave with their modules
+([ADR 0061](./docs/adr/0061-retire-lunora-through-per-user-cutover.md)), not in
+a pruning pass.
+
 ## Before you open a PR
 
 Run the full gate. These mirror CI — except `bun run test:e2e`, which is
@@ -174,7 +264,7 @@ bun run typecheck       # tsc over the app (DOM libs)
 bun run typecheck:worker # tsc over worker/ (workers-types)
 bun run typecheck:test  # tsc over the unit tests (bun types)
 bun run test            # bun test — pure-logic unit tests (src + worker/)
-bun run test:e2e        # playwright (chromium) — behavior/integration
+bun run test:e2e        # playwright (chromium) — app plus isolated real-Worker suites
 bunx changeset          # describe your change for the changelog (see below)
 bun run check:changeset # verify the committed branch carries that decision
 ```
@@ -196,7 +286,7 @@ lint through `bun run lint`; `lint:cli` is the focused local command.
 
 | Surface | Additional checks                                                                                                       | Runtime or fixtures                                                                                                                                                                                                 |
 | ------- | ----------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| Editor  | `bun run test:e2e e2e/<name>.spec.ts --workers=1`                                                                       | Playwright starts its own Vite development server on `E2E_PORT` (default 3210). [`e2e/fixtures.ts`](./e2e/fixtures.ts) mocks the data API.                                                                          |
+| Editor  | `bun run test:e2e:app e2e/<name>.spec.ts --workers=1`                                                                   | Playwright starts its own Vite development server on `E2E_PORT` (default 3210). [`e2e/fixtures.ts`](./e2e/fixtures.ts) mocks the data API.                                                                          |
 | CLI     | `bun run lint:cli`, `bun run build:cli`, `bun run typecheck:cli`, `bun run test:cli`, `bun run --cwd cli check:package` | Install `cli/` dependencies first with `bun install --cwd cli --frozen-lockfile`. Tests run the Node executable against loopback fixtures. The [live test](./cli/README.md#development-and-verification) is opt-in. |
 | Landing | `bun run --cwd landing typecheck`, `bun run --cwd landing build`                                                        | Install `landing/` dependencies first. Inspect the rendered change at desktop and mobile widths.                                                                                                                    |
 
@@ -230,18 +320,6 @@ Rules of thumb:
   maintains the release PR; merging that PR approves publication. See
   [ADR 0062](./docs/adr/0062-automated-cli-releases.md).
 
-- **Unit tests (`bun test`) cover pure logic only** — `tree.ts`, `tags.ts`,
-  `links.ts`, the Worker planners/schemas. Editor behavior (caret, contentEditable,
-  the collection/DO path) stays in **Playwright** (`e2e/`). Don't unit-test the
-  DOM path; you'd just be mocking the world.
-- **Chasing a flake? `bun run test:e2e:serial`** (`--workers=1`) is the
-  maximum-determinism local run; `--workers=2` is the clean-signal full run
-  before a PR. **e2e does not run in CI** — Playwright is a local pre-PR gate,
-  so running it here is what stands in for a CI check. A parallel-contention
-  flake isn't a real failure. e2e runs on its own Vite server on port 3210;
-  kill a zombie or set `E2E_PORT`. For a caret, set the Selection range
-  directly. `toHaveText` normalizes whitespace. **A perf guard asserts a
-  countable invariant**, never a wall clock.
 - **react-doctor is an occasional manual audit, not a gate** — its accepted
   editor false-positives (the deliberately kept manual memos) are known noise
   on every run, so it stays out of the recurring validation set.

cli/test/auth.test.ts

@@ -102,7 +102,7 @@ test("environment credentials never touch the credential directory", async () =>
   expect(await readdir(dir)).toEqual([]);
   await expect(
     Effect.runPromise(accessToken("https://one.test", dir, "")),
-  ).rejects.toThrow("malformed");
+  ).rejects.toThrow();
 });
 
 test.skipIf(process.platform === "win32")(
@@ -115,14 +115,10 @@ test.skipIf(process.platform === "win32")(
     if (!name) throw new Error("No credential file");
     const file = join(dir, name);
     await chmod(file, 0o644);
-    await expect(Effect.runPromise(store.load())).rejects.toThrow(
-      "Cannot read",
-    );
+    await expect(Effect.runPromise(store.load())).rejects.toThrow();
     await rm(file);
     await symlink("/etc/passwd", file);
-    await expect(Effect.runPromise(store.load())).rejects.toThrow(
-      "Cannot read",
-    );
+    await expect(Effect.runPromise(store.load())).rejects.toThrow();
   },
 );
 
@@ -233,7 +229,7 @@ test("OAuth discovery refuses cross-origin endpoints before registration", async
   try {
     await expect(
       Effect.runPromise(login(server.url.origin, dir, true, () => {}, false)),
-    ).rejects.toThrow("selected server");
+    ).rejects.toThrow();
     expect(requests).toBe(1);
     expect(await readdir(dir)).toEqual([]);
   } finally {

cli/test/commands.test.ts

@@ -3,7 +3,7 @@ import { Effect } from "effect";
 
 import manifest from "../package.json" with { type: "json" };
 import { callbackCode, trustedEndpoint } from "../src/auth.js";
-import { commands, mergeInput, parse } from "../src/commands.js";
+import { mergeInput, parse } from "../src/commands.js";
 import { io, serverUrl, terminalText, VERSION } from "../src/core.js";
 
 const parsed = async (args: string[]) => {
@@ -52,29 +52,6 @@ test("I/O errors expose safe operation codes without native messages", async ()
   }
 });
 
-test("covers all twelve current MCP tools", () => {
-  expect(
-    Object.values(commands)
-      .map((c) => c.tool)
-      .sort(),
-  ).toEqual(
-    [
-      "get_outline",
-      "search_nodes",
-      "add_node",
-      "add_subtree",
-      "update_node",
-      "delete_node",
-      "move_nodes",
-      "add_to_today",
-      "mirror_node",
-      "mirror_to_today",
-      "import_opml",
-      "export_opml",
-    ].sort(),
-  );
-});
-
 test("flags preserve false, empty text, numbers, and global options", async () => {
   const update = await parsed([
     "--json",
@@ -133,9 +110,7 @@ test("friendly search maps pagination flags and keeps --all CLI-only", async ()
   const all = await parsed(["search", "project", "--all"]);
   expect(all.fields).toEqual({ query: "project" });
   expect(all.all).toBe(true);
-  expect(() => mergeInput(all, { cursor: "from-json" }, "UTC")).toThrow(
-    "first page",
-  );
+  expect(() => mergeInput(all, { cursor: "from-json" }, "UTC")).toThrow();
 });
 
 test("moves keep input order and all tool arguments can arrive as JSON", async () => {
@@ -183,27 +158,12 @@ test("friendly today uses local time; raw call preserves UTC defaults", async ()
   ).toEqual({ text: "hello" });
 });
 
-test("deletion requires --yes for both entry points", async () => {
-  for (const argv of [
-    ["delete", "id"],
-    ["call", "delete_node"],
-  ]) {
-    const command = await parsed(argv);
-    expect(() => mergeInput(command, {}, "UTC")).toThrow("--yes");
-    expect(() =>
-      mergeInput({ ...command, yes: true }, {}, "UTC"),
-    ).not.toThrow();
-  }
-});
-
 test("rejects duplicate input sources and ambiguous field merging", async () => {
   const command = await parsed(["add", "text"]);
-  expect(() => mergeInput(command, { text: "other" }, "UTC")).toThrow(
-    "only once",
-  );
+  expect(() => mergeInput(command, { text: "other" }, "UTC")).toThrow();
   expect(() =>
     mergeInput({ ...command, input: "a", args: "{}" }, {}, "UTC"),
-  ).toThrow("not both");
+  ).toThrow();
 });
 
 test("rejects unknown options, extra arguments, malformed flags", async () => {
@@ -253,7 +213,7 @@ test("OAuth callback validates path, state, unique code, and denial", () => {
   }
   expect(() =>
     callbackCode("/callback?state=expected&error=denied", "expected"),
-  ).toThrow("denied");
+  ).toThrow();
 });
 
 test("human output strips terminal control characters", () => {

cli/test/credential-races.test.ts

@@ -155,7 +155,7 @@ test("failed keyring metadata finalization preserves file credentials for cleanu
       Effect.runPromise(
         store.save({ ...credential, accessToken: "replacement" }, false),
       ),
-    ).rejects.toThrow("Cannot save credential configuration");
+    ).rejects.toThrow();
     afterKeyringSet = undefined;
     await rm(path, { recursive: true });
     await rename(intent, path);
@@ -272,9 +272,7 @@ test.skipIf(process.platform !== "linux")(
         },
       );
       expect(await child.exited).toBe(0);
-      expect(await new Response(child.stderr).text()).toContain(
-        "Cleanup is pending",
-      );
+      expect(await new Response(child.stderr).text()).not.toBe("");
       const record = JSON.parse(await readFile(path, "utf8"));
       expect(record.storage).toBe("file");
       expect(record.pendingKeyringCleanup).toBe(true);

cli/test/process.test.ts

@@ -152,27 +152,9 @@ test("Node executable supports JSON/stdin, future tools, and distinct refusal/pl
     );
     expect(numericRefusal.code).toBe(4);
     expect(numericRefusal.stdout).not.toContain('"123"');
-    expect(JSON.parse(numericRefusal.stdout)).toEqual({
-      content: [
-        {
-          type: "text",
-          text: 'Rejected bearer [redacted] and "[redacted]"',
-        },
-      ],
-      structuredContent: {
-        authorization: "Bearer [redacted]",
-        "[redacted]": "must redact keys too",
-        expiresAt: 1234,
-        nested: [
-          null,
-          false,
-          17,
-          { text: "secret [redacted]" },
-          ["Bearer [redacted]"],
-        ],
-      },
-      isError: true,
-    });
+    expect(JSON.parse(numericRefusal.stdout)).toEqual(
+      JSON.parse(refused.stdout),
+    );
     const plan = await cli(server.url.origin, ["call", "plan", "--json"]);
     expect(plan.code).toBe(5);
     expect(plan.stdout).toBe("");
@@ -396,11 +378,12 @@ test("search --all validates and buffers real pages while raw search stays singl
     expect(empty.code).toBe(0);
     expect(empty.stdout.trim()).toBe('No nodes match "project".');
 
+    // Only the server's own message is asserted; CLI-authored wording is free to change.
     for (const [failureMode, message] of [
       ["stale", "Restart without the cursor."],
-      ["malformed", "Invalid search page"],
-      ["repeat", "repeated a search cursor"],
-      ["duplicate", "duplicate search node"],
+      ["malformed", undefined],
+      ["repeat", undefined],
+      ["duplicate", undefined],
     ] as const) {
       mode = failureMode;
       searchCalls.length = 0;
@@ -411,7 +394,8 @@ test("search --all validates and buffers real pages while raw search stays singl
       ]);
       expect(failed.code).toBe(1);
       expect(failed.stdout).toBe("");
-      expect(failed.stderr).toContain(message);
+      if (message) expect(failed.stderr).toContain(message);
+      else expect(failed.stderr).not.toBe("");
       expect(failed.stderr).not.toContain("test-token");
       expect(searchCalls).toHaveLength(2);
     }

docs/runbooks/retire-lunora.md

@@ -6,7 +6,7 @@ This is the operator runbook for [ADR 0061](../adr/0061-retire-lunora-through-pe
 
 Run the isolated checks with `bun run test:e2e:retirement`. They exercise Workerd, SQLite Durable Objects, D1, and R2 without production bindings. They cover sequential cutover, recovery, overlapping request rejection, retained operation claims, CLI batch stopping, Classic-only MCP routing, and rejection of the retired public namespace without changing retained shard data.
 
-Run `bun run test:e2e e2e/lunora-retirement-browser.spec.ts` to confirm that stale enabled preferences, localStorage flags, and URL overrides cannot bypass Classic sync. Settings must not expose the upgraded-sync toggle, edits must survive reload, and the browser must make no `/_lunora` requests.
+Run `bun run test:e2e:app e2e/lunora-retirement-browser.spec.ts` to confirm that stale enabled preferences, localStorage flags, and URL overrides cannot bypass Classic sync. Settings must not expose the upgraded-sync toggle, edits must survive reload, and the browser must make no `/_lunora` requests.
 
 ## Before running
 

e2e/capture-real.spec.ts

@@ -219,8 +219,8 @@ test("free-account HTTP capture honors explicit date and has decisive idempotenc
     "2024",
     "2024-02",
     "2024-02-29",
-    "2024-W09",
     "container",
+    "week:2024-02-26",
   ]);
 });