Repair the UI e2e suite and run it in CI (gate PRs + auto-fix on main)

.github/workflows/e2e.yml

@@ -0,0 +1,86 @@
+name: E2E
+
+# Runs the Playwright UI e2e suite (`bun run e2e:ui`) on every PR and on every
+# merge to main. The suite drives the REAL built daemon, so it can't be a bun
+# unit test — and until now it had no automated gate, which is how it silently
+# rotted across several UI refactors (see DECISIONS.md "UI e2e caps workers…").
+#
+# On a POST-MERGE failure (a regression landed on main), the `autofix` job hands
+# the failure to Claude Code, which reproduces it, fixes the right side (app or
+# stale test), and opens a PR. On a PR, the red check is the gate — you fix it on
+# the branch, so the bot does not open a PR there.
+#
+# Requirements (one-time): the `ANTHROPIC_API_KEY` repo secret, and
+# Settings → Actions → General → "Allow GitHub Actions to create and approve
+# pull requests" (so the autofix job can open the PR).
+
+on:
+  push:
+    branches: [main]
+  pull_request:
+  workflow_dispatch:
+
+# Cancel superseded PR runs; let every merge to main run to completion.
+concurrency:
+  group: e2e-${{ github.ref }}
+  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
+
+jobs:
+  e2e:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: oven-sh/setup-bun@v2
+      - name: Install dependencies
+        run: bun install --frozen-lockfile
+      - name: Install Playwright browser
+        run: bunx playwright install --with-deps chromium
+      - name: Run UI e2e suite
+        run: bun run e2e:ui
+      - name: Upload Playwright artifacts on failure
+        if: failure()
+        uses: actions/upload-artifact@v4
+        with:
+          name: playwright-results
+          path: test-results/
+          retention-days: 7
+          if-no-files-found: ignore
+
+  # Post-merge self-heal. Runs only when the suite fails on a push to main; it
+  # sets up bun + the Playwright browser too, so Claude can actually re-run
+  # `bun run e2e:ui` to reproduce and verify its fix before opening the PR.
+  autofix:
+    needs: [e2e]
+    if: ${{ failure() && github.event_name == 'push' }}
+    runs-on: ubuntu-latest
+    permissions:
+      contents: write
+      pull-requests: write
+      id-token: write
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+      - uses: oven-sh/setup-bun@v2
+      - name: Install dependencies
+        run: bun install --frozen-lockfile
+      - name: Install Playwright browser
+        run: bunx playwright install --with-deps chromium
+      - name: Fix the failing e2e suite and open a PR
+        uses: anthropics/claude-code-action@v1
+        with:
+          anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}
+          claude_args: "--max-turns 30"
+          prompt: |
+            The Playwright UI e2e suite (`bun run e2e:ui`) is failing on `main` after a merge.
+            Investigate, fix it, and open a pull request with the fix.
+
+            - Reproduce with `bun run e2e:ui` — it builds, boots the real daemon on a throwaway
+              OTACON_HOME, and runs `test/ui/*.e2e.ts`.
+            - For each failure, decide whether the bug is in the app/daemon (`src/`) or a stale
+              expectation in `test/ui/*.e2e.ts`, and fix the correct side. Read DESIGN.md and
+              DECISIONS.md before changing any product behavior — the suite drives the real
+              built daemon, and DESIGN.md is the source of truth for intended behavior.
+            - Re-run `bun run e2e:ui` until the whole suite is green, and keep `bun test` and
+              `bun run typecheck` passing too.
+            - Open a PR whose description states the root cause and the fix.

DECISIONS.md

@@ -650,6 +650,23 @@ Revisit when**. Every tradeoff made in a change gets its entry here in the same
 - **Revisit when:** the suites need shared fixtures, or bun can run Playwright specs
   natively.
 
+## UI e2e caps workers at 2 with 2 retries: one daemon, not a worker fleet
+
+- **Decision:** `playwright.config.ts` pins `workers: 2` and `retries: 2` (keeping
+  `fullyParallel`). The whole suite shares ONE `node dist/daemon/main.js`; each
+  worker holds SSE streams and spawns real `otacon wait` long-polls, so high
+  concurrency saturates that single process and its keep-alive connections reset
+  mid-flight (`ECONNRESET` on the API client). The reset count scales with workers
+  (≈1 → 8 → 24 at 2 → 4 → 9); two workers keeps it rare and the retry budget
+  absorbs the residual, so the run is deterministically green.
+- **Why:** The contention is a test-harness artifact, not a daemon bug — production
+  otacond serves one human plus their agent, never a fleet of concurrent clients, so
+  raising the server's keep-alive to chase e2e throughput would change product code
+  for a test-only reason. Capping the harness is the scoped fix; retries cover the
+  inherently timing-dependent reuse race a worker cap alone can't fully eliminate.
+- **Revisit when:** the suite is sharded across daemons (a port/home per worker), or
+  the daemon gains real multi-client load worth hardening keep-alive for.
+
 ## M1 scope: CLI surface is `start`/`submit`/`wait`/`status` only
 
 - **Decision:** M1 ships sessions, registry, submit + linter (L1/L2/L6), event queues,

playwright.config.ts

@@ -25,6 +25,17 @@ export default defineConfig({
   testDir: "test/ui",
   testMatch: "**/*.e2e.ts",
   fullyParallel: true,
+  // The whole suite drives ONE real daemon (a single Node HTTP server). Each
+  // worker opens SSE streams and spawns real `otacon wait` long-polls, so a high
+  // worker count saturates that one process — its keep-alive connections get
+  // reset mid-flight (`ECONNRESET` on the API client), and the failure count
+  // scales with concurrency (≈1 → 8 → 24 at 2 → 4 → 9 workers). Cap the workers
+  // so the shared daemon is never overwhelmed, and keep a small retry budget to
+  // absorb the rare residual reset; the suite is then deterministically green.
+  // (The production daemon serves one human + their agent, not a worker fleet, so
+  // this is a test-harness concern, not a daemon bug to fix in product code.)
+  workers: 2,
+  retries: 2,
   reporter: "list",
   use: { baseURL },
   projects: [{ name: "chromium", use: { ...devices["Desktop Chrome"] } }],

test/ui/acceptance.e2e.ts

@@ -2,14 +2,14 @@
 // in a real browser against the real built daemon AND the real built CLI. One
 // session is driven through the entire agent/reviewer loop — grill → draft → review →
 // revise → approve — and every screen a real reviewer touches is asserted:
-//   • the index card surfaces the session and its live status chip;
+//   • the sidebar surfaces the session and its live status glyph;
 //   • a grill question card is answerable, waking a parked `otacon wait`;
 //   • the submitted plan renders as schema'd sections, a real mermaid diagram,
 //     and a collapsed Details block with its size badge;
 //   • a comment composed in the UI reaches a parked CLI wait;
 //   • a new revision raises the changelog banner + diff + gutter markers;
-//   • a resolved thread collapses, and a thread whose quote a later revision
-//     deletes lands in the orphan tray;
+//   • an agent reply lands inline on the open thread, and a thread whose quote a
+//     later revision deletes detaches inline (muted quote, no orphan tray);
 //   • Approve from the UI flips the screen to its approved read-only state and
 //     surfaces the saved project copy under .otacon/plans (plus the home copy).
 // Three screenshots (desktop review, phone review, index) are captured to a
@@ -18,7 +18,7 @@
 // Reuses the shared helpers/CLI harness; runs headless under the same webServer
 // (node dist/daemon/main.js, temp OTACON_HOME) as every other *.e2e.ts spec.
 
-import { mkdtempSync, readFileSync, writeFileSync } from "node:fs";
+import { existsSync, mkdtempSync, readdirSync, readFileSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { dirname, join } from "node:path";
 import { fileURLToPath } from "node:url";
@@ -63,11 +63,15 @@ test("UI acceptance: the whole agent/reviewer loop renders and round-trips in th
     session.id,
   );
 
-  // ── 1. Index surfaces the new session, drafting ──────────────────────────
+  // ── 1. Index surfaces the new session ────────────────────────────────────
+  // The app-shell sidebar (≥960px) is the live index: the session shows as a
+  // row the moment it registers. Freshly created over the API with no agent on
+  // the line yet (no `otacon wait`/`ask` has parked it), its status derivation
+  // reads stalled — the brighter "drafting" spinner lights once contact lands.
   await page.goto("/");
-  const card = page.locator(".card", { hasText: session.title });
+  const card = page.locator(".sl-row", { hasText: session.title });
   await expect(card).toBeVisible();
-  await expect(card.locator(".chip")).toHaveText("agent working");
+  await expect(card.locator(".sl-glyph")).toHaveAttribute("aria-label", "stalled");
 
   // ── 2. Grill: an `otacon ask` card is answerable; it wakes a parked wait ──
   await page.goto(`/s/${session.id}`);
@@ -109,8 +113,11 @@ test("UI acceptance: the whole agent/reviewer loop renders and round-trips in th
     dir,
     richBase.replace("- D1: RS256 over HS256 [assumed]", `- D1: RS256 over HS256 ← ${qid}`),
   );
-  // The fresh new-revision banner carries the landed revision (the r0 header pill is gone).
-  await expect(page.locator(".rev-fresh .rev-label")).toHaveText("r1 received");
+  // r1 is a first review, not a re-review: it raises no new-revision banner (the
+  // banner renders only from r2 — DECISIONS "banner from r2"). The plan itself
+  // lands live over SSE, replacing the grill placeholder — the schema assertions
+  // below are the proof it rendered without a reload.
+  await expect(page.locator(".rev-fresh")).toHaveCount(0);
   // Schema-aware structure, not markdown soup.
   await expect(page.locator("#summary .section-title")).toHaveText("Summary");
   await expect(page.locator("#phase-1 .phase-name")).toHaveText("Token issuance");
@@ -153,7 +160,7 @@ test("UI acceptance: the whole agent/reviewer loop renders and round-trips in th
   expect(cev.items[0]!.anchor.exact).toBe("RS256 over HS256");
   const threadId = cev.items[0]!.thread;
 
-  // ── 5. Revise: r2 raises the banner + gutter marker; the thread resolves ──
+  // ── 5. Revise: r2 raises the banner; the agent replies on the open thread ──
   await plantMarker(page);
   await cliSubmit(
     session.id,
@@ -172,81 +179,115 @@ test("UI acceptance: the whole agent/reviewer loop renders and round-trips in th
   await expect(banner.locator(".rev-label")).toHaveText("r2 received");
   await expect(banner.locator(".rev-changelog")).toContainText("Reworded the summary");
   expect(await readMarker(page)).toBe(true); // banner arrived over SSE
-  // Gutter marker on the changed section, clean view.
+  // The comment batch in step 4 implicitly marked r1 reviewed (a comment moves
+  // the diff baseline — app.ts), so r2's gutter markers + diff baseline off r1:
+  // the reworded summary is flagged changed in the clean view.
   await expect(page.locator("#summary.unit-changed")).toHaveCount(1);
-  // The resolved thread collapses to its ✓ line; expanding shows the reply.
-  const resolved = page.locator(".thread-resolved");
-  await expect(resolved).toHaveCount(1);
-  await expect(resolved.locator(".resolved-rev")).toHaveText("r2");
-  await resolved.locator(".resolved-summary").click();
-  await expect(resolved.locator(".thread-answer-body")).toContainText("verifiers hold only");
-  // The banner's diff shows hunks only where the plan changed.
+  // A revision reply is a *response*, not a close (DECISIONS "a landed reply is a
+  // response, not a close" — the reviewer resolves separately): the agent's reply
+  // lands inline on the still-open comment card, never collapsing it to a ✓.
+  const thread = page.locator(".thread-comment");
+  await expect(thread.locator(".thread-answer-body")).toContainText("verifiers hold only");
+  await expect(thread.locator(".thread-answer-label")).toContainText("r2");
+  await expect(page.locator(".thread-resolved")).toHaveCount(0);
+  // The banner's diff shows hunks only where the plan changed (r1 → r2).
   await banner.locator(".btn-ghost", { hasText: "view diff" }).click();
   await expect(page.locator(".plan-diff")).toBeVisible();
   await expect(page.locator(".diff-legend")).toContainText("r1 → r2");
   await expect(page.locator("#summary .dline-add")).toContainText("Replace legacy session-cookie");
   await page.locator(".seg-btn", { hasText: "clean" }).click();
+  // Dismiss marks r2 reviewed (lastReviewedRevision → 2), so the NEXT revision's
+  // diff and gutter markers baseline off r2.
   await banner.locator(".btn-primary", { hasText: "dismiss" }).click();
   await expect(banner).toHaveCount(0);
 
-  // ── 6. Orphan tray: r3 deletes the quoted text → the thread orphans live ──
+  // ── 6. Detach: r3 deletes the quoted text → the thread detaches inline ──────
   await plantMarker(page);
   await cliSubmit(
     session.id,
     dir,
     richBase
       .replace("revision: 1", "revision: 3")
       .replace("- D1: RS256 over HS256 [assumed]\n", "") // deletes the anchored quote
-      .replace("Replace session-cookie auth", "Replace legacy session-cookie auth"),
-    { changelog: "Dropped the RS256 decision line." },
+      .replace("Replace session-cookie auth", "Replace the legacy session-cookie auth flow"),
+    { changelog: "Dropped the RS256 decision line; retitled the summary." },
   );
-  const tray = page.locator(".orphan-toggle");
-  await expect(tray).toBeVisible();
-  await expect(tray.locator(".orphan-count")).toHaveText("1");
-  await tray.click();
-  const orphan = page.locator(".orphan");
-  await expect(orphan).toHaveCount(1);
-  await expect(orphan.locator(".thread-quote")).toContainText("RS256 over HS256");
-  await expect(orphan.locator(".orphan-where")).toHaveText("#decisions");
+  const banner3 = page.locator(".rev-banner");
+  await expect(banner3.locator(".rev-label")).toHaveText("r3 received");
+  await expect(banner3.locator(".rev-changelog")).toContainText("Dropped the RS256 decision line");
+  expect(await readMarker(page)).toBe(true); // banner arrived over SSE
+  // Gutter marker on the reworded summary, clean view — the baseline is r2 now.
+  await expect(page.locator("#summary.unit-changed")).toHaveCount(1);
+  // No orphan tray (DECISIONS "no UI tray, inline & muted"): the detached
+  // thread stays inline in the rail with its quote muted (no live text to jump
+  // to) beside the ⌀ glyph; the word "orphan" appears nowhere.
+  await expect(thread.locator(".thread-quote-muted")).toContainText("RS256 over HS256");
+  await expect(thread.locator(".thread-quote-detached")).toBeVisible();
+  await expect(page.locator(".rail")).not.toContainText("orphan");
+  // The banner's diff baselines off r2 now.
+  await banner3.locator(".btn-ghost", { hasText: "view diff" }).click();
+  await expect(page.locator(".diff-legend")).toContainText("r2 → r3");
+  await page.locator(".seg-btn", { hasText: "clean" }).click();
+  await banner3.locator(".btn-primary", { hasText: "dismiss" }).click();
 
   // ── 7. Approve from the UI → approved read-only state + project-copy path ──
   await page.locator(".ctrl-approve").click();
   const sheet = page.locator(".approve-sheet");
   await expect(sheet).toContainText(".otacon/plans");
   await sheet.locator(".btn-approve").click();
-  // The lone orphaned (resolved) thread is not an open thread, but the orphan
-  // tray keeps it; if approve warns, force through it — the human's path.
-  if (await sheet.locator(".btn-force").isVisible().catch(() => false)) {
-    await sheet.locator(".btn-force").click();
-  }
-  const note = page.locator(".approved-note");
-  await expect(note).toBeVisible();
-  await expect(page.locator(".chip")).toHaveText("approved");
-  const noted = await note.locator(".approved-path").textContent();
-  const relPath = /\.otacon\/plans\/\S+\.md/.exec(noted ?? "")?.[0];
-  expect(relPath, "approved note surfaces the project-copy path").toBeTruthy();
-  // The project copy is on disk in the session's repo with the rewritten frontmatter.
-  const artifact = readFileSync(join(session.repo, relPath!), "utf8");
+  // The replied-but-unresolved comment still counts as unresolved at approve (a
+  // reply is a response, not a close — app.ts), so the sheet flips to its amber
+  // warning. Wait for that stage, then force through it ("Save anyway") — the
+  // human's path. (No "Send to agent": the thread already has a reply, so there
+  // are no open comments to fold in.)
+  await expect(sheet).toHaveClass(/approve-warning/);
+  await sheet.locator(".btn-force").click();
+  // A Save-approve of the viewed session redirects it home on the live
+  // approved crossing (DECISIONS "Approving the viewed session redirects home"):
+  // the screen leaves /s/:id for the index rather than lingering on a session
+  // whose switcher chip is gone.
+  await expect(page).toHaveURL(/\/$/);
+  // otacon never commits the plan, but it writes a project copy under the repo's
+  // .otacon/plans. Poll for it (the write completes before the redirect frame),
+  // then assert the rewritten frontmatter + the folded-in interview.
+  const plansDir = join(session.repo, ".otacon", "plans");
+  let planFiles: string[] = [];
+  await expect
+    .poll(() => {
+      planFiles = existsSync(plansDir)
+        ? readdirSync(plansDir).filter((f) => f.endsWith(".md"))
+        : [];
+      return planFiles.length;
+    })
+    .toBeGreaterThan(0);
+  const artifact = readFileSync(join(plansDir, planFiles[0]!), "utf8");
   expect(artifact).toContain("status: approved");
   expect(artifact).toContain("## Interview");
   expect(artifact).toContain(`### ${qid}`);
-  // Read-only: the mutation surfaces are gone, and the daemon refuses mutations.
-  await expect(page.locator(".ctrl-approve")).toHaveCount(0);
+  // The daemon refuses mutations on the over (approved) session.
   const refused = await request.post(`/api/sessions/${session.id}/comments`, {
     data: { items: [{ anchor: null, body: "too late" }] },
   });
   expect(refused.status()).toBe(409);
 
   // 📸 the index after the loop, and the phone review screen of the same plan.
+  // Approved (over) sessions fall to the sidebar's collapsed `approved (n)`
+  // disclosure; expand it and confirm the session now reads approved.
   await page.goto("/");
-  await expect(page.locator(".card", { hasText: session.title }).locator(".chip")).toHaveText(
-    "approved",
-  );
+  await page.locator(".sl-approved-toggle").click();
+  const approvedRow = page.locator(".sl-approved-rows .sl-row", { hasText: session.title });
+  await expect(approvedRow).toBeVisible();
+  await expect(approvedRow.locator(".sl-glyph")).toHaveAttribute("aria-label", "approved");
   await page.screenshot({ path: join(shotsDir, "03-index.png"), fullPage: true });
 
   await page.setViewportSize({ width: 375, height: 720 });
+  // Opening an already-approved session does NOT redirect (the crossing guard is
+  // live-transition only): it renders read-only behind the approved notice, with
+  // the mutation surfaces (Approve) gone.
   await page.goto(`/s/${session.id}`);
   await expect(page.locator("#summary .md")).toBeVisible();
+  await expect(page.locator(".approved-note")).toBeVisible();
+  await expect(page.locator(".ctrl-approve")).toHaveCount(0);
   await page.screenshot({ path: join(shotsDir, "02-phone-review.png"), fullPage: true });
 
   // Print the screenshot dir so the orchestrator can surface the images.