diff --git a/docs/superpowers/plans/2026-07-10-spoon-implementation-plan-index.md b/docs/superpowers/plans/2026-07-10-spoon-implementation-plan-index.md new file mode 100644 index 0000000..d4f724e --- /dev/null +++ b/docs/superpowers/plans/2026-07-10-spoon-implementation-plan-index.md @@ -0,0 +1,95 @@ +# Spoon: Stabilize → Unify → Build — Implementation Plan Index + +> **For agentic workers:** Execute the phase plans **in order**. Each phase is a +> standalone plan that ships working, tested software on its own. Use +> `superpowers:subagent-driven-development` (fresh subagent per task, review +> between tasks) or `superpowers:executing-plans` (batched with checkpoints). + +**Spec:** [`../specs/2026-07-10-spoon-stabilize-unify-build-design.md`](../specs/2026-07-10-spoon-stabilize-unify-build-design.md) +**Design review:** Gib, 2026-07-10 (tenancy = owner + trusted friends; runtimes = +Codex + OpenCode + Claude Code, all in the per-user box; all four feature tracks; +ordering = stabilize → unify → build). + +## Phases + +| # | Plan | Tasks | Scope | +| - | ---- | ----- | ----- | +| 1 | [phase1-stabilize](2026-07-10-spoon-phase1-stabilize.md) | 13 | Critical/High bug fixes on the current architecture: home-deletion data loss, box lifecycle races/leaks, symlink escape, job-lifecycle recovery (heartbeat/timeout/cancel), auto-sync gating, terminal sizing, workspace shell keying, not-found handling. | +| 2 | [phase2-runtime-unification](2026-07-10-spoon-phase2-runtime-unification.md) | 12 | `AgentRuntime` adapter interface; Codex/OpenCode/Claude Code all exec into the box; delete the legacy per-job OpenCode container; runtime picker + queue-time validation; Fedora image + smoke; secrets hygiene. | +| 3 | [phase3-dev-box-surface](2026-07-10-spoon-phase3-dev-box-surface.md) | 11 | `/machine` page (box status/lifecycle + full-page terminal + home file browser); user-scoped `/box/*` worker routes + HMAC tokens; persistent workspace terminal (survives tab switch); terminal QoL. | +| 4 | [phase4-sync-correctness](2026-07-10-spoon-phase4-sync-correctness.md) | 12 | Correct head-SHA (branch ref, not truncated compare); stable maintenance-thread dedup; GitHub webhooks (push + installation lifecycle); Octokit retry/throttle + `rate_limited` status; bound unbounded queries; surface `needs_reauth`. | +| 5 | [phase5-notifications-polish](2026-07-10-spoon-phase5-notifications-polish.md) | 15 | `notifications` + `notificationPreferences` tables, in-app bell, preference-gated UseSend email; polish sweep (skeletons, error handling, form re-sync, auth copy, workspace UX, a11y, dead-code removal); docs rewrite. | + +**Total: 63 tasks**, each ending in an independently testable, committed deliverable. + +## Cross-phase dependencies + +- **Phase 1 → 2, 3.** Phases 2 and 3 consume the Phase-1 box-lifecycle API: + `export type BoxHandle = { boxName: string; release: () => void }` and + `acquireUserBox(args: { username; workdir; containerHome }) => Promise` + (idempotent `release`, per-username mutex, `--init`, startup reconcile of + `spoon-box-*`). Both later plans carry a fallback note for the pre-Phase-1 + `Promise` + `releaseUserBox(username)` signature — **build Phase 1 + first and the `BoxHandle` form is what lands.** +- **Phase 1 → 5.** Phase 5's `agent_turn_finished` / `agent_needs_input` + notifications emit at the job-status transitions that Phase 1 hardens + (`updateStatus`, `applyMaintenanceDecision`). +- **Phase 4 → 5.** Phase 5's `connection_needs_reauth` email emits where the + Phase-4 webhook sets `gitConnections.status = needs_reauth`; Phase 5 Task 3 + marks that emit point defensively if Phase 4 hasn't landed. +- **Phase 4 depends on Phase 1's auto-sync gating** (`autoSyncEnabled` etc. + land in Phase 1 Task 8) and layers head-SHA/dedup/webhooks on top. + +Phases 3 and 4 are otherwise independent and could be reordered or parallelized +after Phases 1–2. The recommended sequential order matches the spec. + +## Deliberate deferrals (documented by the plan authors) + +These are called out inline in the plans, not gaps: + +- **Phase 1:** `updateStatus` implements the terminal-state guard (the + cancel-revert fix) rather than a full forward-only transition matrix — no + illegal transition is otherwise exercised. Stale-job recovery **times out** + rather than requeues (requeue-with-attempt-limit needs a new `claimAttempts` + field — deferred). `requireAiLowRiskForSync` is scoped to Phase 4 (the + AI-review→merge path it guards doesn't exist yet). +- **Phase 2:** exact `opencode run` / `claude -p` flags and the + `@anthropic-ai/claude-code` version pin are reconciled at implementation time + via the extended `scripts/smoke-agent-container` (`--help` probes + `npm view`), + since the live CLIs aren't available while planning. +- **Phase 3:** dotfiles re-materialization for the `/box` surface is out of + scope (the persistent home already carries dotfiles from prior job runs); + box start only ensures a login-shell `.bash_profile`. Docker-touching code is + verified by documented manual steps; only pure logic is unit-tested. +- **Phase 4:** the webhook route test cannot assert the scheduled `'use node'` + refresh ran in-process (convex-test VM limitation) — covered by the manual + checklist. Octokit plugin versions may need a bump on `bun install`. +- **Phase 5:** `(app)/error.tsx` + not-found handling lives in **Phase 1** + (finding #14), not here. + +## Testing + +Every task is test-first where a test is meaningful. Test surfaces: + +- **Worker** (`apps/agent-worker/tests/unit/`, vitest): box-registry + mutex/handle semantics, path containment incl. symlinks, terminal + resize-message parsing, runtime-adapter fixture-stream parsing. +- **Backend** (`packages/backend/tests/unit/`, `convex-test`): claim + validation/rollback, heartbeat-timeout cron, transition guard incl. + cancel-vs-worker race, auto-sync gating matrix, maintenance-thread dedup, + webhook signature verification, notification emit/preferences. +- **Next** (`apps/next/tests/component`, jsdom + testing-library): terminal + fit-on-fonts-ready + resize, workspace shell remount on job switch, form + re-sync, not-found states, notification bell. +- **Docker-touching worker code** (box `--init`, dockerode TTY exec, `/box` + lifecycle) uses documented manual verification — needs a live daemon. + +## How to run each phase + +``` +Subagent-driven (recommended): one fresh subagent per task, two-stage review between tasks. +Inline: superpowers:executing-plans, batched with checkpoints. +``` + +Start with Phase 1, Task 1 (`cleanupOrphanedWorkspaces` layout-awareness) — it +closes the highest-severity bug (a cleanup call deletes every user's home). diff --git a/docs/superpowers/plans/2026-07-10-spoon-phase1-stabilize.md b/docs/superpowers/plans/2026-07-10-spoon-phase1-stabilize.md new file mode 100644 index 0000000..7764bf1 --- /dev/null +++ b/docs/superpowers/plans/2026-07-10-spoon-phase1-stabilize.md @@ -0,0 +1,1704 @@ +# Spoon Phase 1 — Stabilize: Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Eliminate the Critical/High bugs from the 2026-07-10 audit without changing the current architecture: stop the worker cleanup from deleting user homes, make the job state machine crash-safe and cancel-safe, honor upstream-sync settings, close the symlink escape, fix the terminal (client sizing + real PTY resize), key the workspace shell by job, make the per-user box registry concurrency-correct, and add friendly not-found handling. Each task is an independent, test-first commit. + +**Architecture:** A Convex backend (`packages/backend/convex`) owns the authoritative job/thread state machine and is polled by a single long-running Bun worker (`apps/agent-worker`) that execs into one persistent per-user Docker/Podman "box" (`spoon-box-{username}`) mounting a persistent home at `/home/{username}`; each thread is a checkout at `~/Code/{spoon}/{branch}`. A Next.js 16 app (`apps/next`) renders the workspace UI (xterm terminal, file tree, diff) and proxies worker access after Convex ownership checks. The Convex state machine is authoritative; the worker conforms and never resurrects terminal jobs. + +**Tech Stack:** TypeScript, Convex, Next.js 16 (React 19), agent-worker (Bun, dockerode, ws), vitest. + +## Global Constraints +- Tests: vitest projects. Worker unit tests: `apps/agent-worker/tests/unit/*.test.ts` run with `cd apps/agent-worker && bun run test:unit` (or `vitest run --project unit`). Backend tests: `packages/backend/tests/unit/*.test.ts` using `convex-test`, run with `cd packages/backend && bun run test:unit`. Next component tests: jsdom + @testing-library/react via `cd apps/next && bun run test:component`; Next unit tests (node env) via `cd apps/next && bun run test:unit`. +- Commit style: conventional commits (feat:/fix:/refactor:/test:/docs:). Frequent commits, one per task. +- Do NOT use `git commit -n` unless a hook blocks; precommit runs lint-staged. +- Convex functions live in `packages/backend/convex`; codegen via `bun codegen:convex` from repo root before typecheck/test. New Convex functions are auto-discovered by the convex-test harness (`import.meta.glob('../../convex/**/*.*s')`), but you must run `bun codegen:convex` from the repo root so `_generated/api` and `_generated/dataModel` typecheck. +- Worker unit tests that touch modules importing `./env` MUST set the required env vars **before** the dynamic import and use `vi.resetModules()` (see `apps/agent-worker/tests/unit/docker-runtime.test.ts` for the exact pattern). `env.ts` throws at import time if `SPOON_WORKER_TOKEN`, `GITHUB_APP_ID`, or `GITHUB_APP_PRIVATE_KEY` are missing. +- Worker in-memory concurrency (box registry, cleanup) is tested by `vi.mock`-ing `../../src/runtime/docker` and `../../src/env`, then dynamically importing the module under test. Match the style of `tests/unit/docker-runtime.test.ts` and `tests/unit/terminal-token.test.ts`. + +--- + +## Task 1: Layout-aware `cleanupOrphanedWorkspaces` (stop deleting user homes) + +**Why first:** This is the only data-loss bug. `POST /cleanup` (Settings → Worker) currently `rm -rf`s every top-level entry under `env.workdir` that is not an active workdir. In the Phase-2 layout every user's persistent home lives under `homes/{username}`, and an active workspace's `workdir` is `homes/{username}` (the home root) — but `activeWorkdirs` only contains homes of *currently running* jobs, so an idle user's entire home is deleted. It also only enumerates `spoon-agent-job-*` containers, ignoring the new `spoon-box-*` boxes. + +**Files:** +- Modify `apps/agent-worker/src/worker.ts` (`cleanupOrphanedWorkspaces` ~1911-1954; `getWorkerHealth` ~1880-1909; add pure helpers near top). +- Modify `apps/agent-worker/src/user-container.ts` (export `runningBoxUsernames()` used by cleanup). +- Create `apps/agent-worker/tests/unit/cleanup-layout.test.ts`. + +**Interfaces:** +- Produces (worker.ts): `export const planWorkdirCleanup = (args: { rootEntries: { name: string; isDirectory: boolean }[]; homesEntries: Record; codeLeaves: Record; activeWorkdirs: Set; runningBoxUsernames: Set; root: string }) => { removeDirs: string[] }` +- Produces (user-container.ts): `export const runningBoxUsernames = (): Set` — usernames whose box currently has refs > 0 OR is registered (adopted). For Task 1 return the set of keys in the `boxes` map (a registered box means the user is/was active). Task 2 refines this. + +**Steps:** + +1. - [ ] Write failing test `apps/agent-worker/tests/unit/cleanup-layout.test.ts`. Set env, `vi.resetModules()`, dynamic-import `../../src/worker` (it imports `./env`, so set `SPOON_WORKER_TOKEN`, `GITHUB_APP_ID`, `GITHUB_APP_PRIVATE_KEY` first, matching `docker-runtime.test.ts`). Assert on `planWorkdirCleanup`: + +```ts +import { afterEach, describe, expect, test, vi } from 'vitest'; + +const load = async () => { + vi.resetModules(); + process.env.SPOON_WORKER_TOKEN = 'test-worker-token'; + process.env.GITHUB_APP_ID = '123'; + process.env.GITHUB_APP_PRIVATE_KEY = + '-----BEGIN PRIVATE KEY-----\\ntest\\n-----END PRIVATE KEY-----'; + return await import('../../src/worker'); +}; + +describe('planWorkdirCleanup', () => { + afterEach(() => vi.resetModules()); + + test('never removes the homes/ root or any home directory', async () => { + const { planWorkdirCleanup } = await load(); + const plan = planWorkdirCleanup({ + root: '/work', + rootEntries: [{ name: 'homes', isDirectory: true }], + homesEntries: { alice: [{ name: 'Code', isDirectory: true }] }, + codeLeaves: {}, + activeWorkdirs: new Set(), + runningBoxUsernames: new Set(), + }); + expect(plan.removeDirs).not.toContain('/work/homes'); + expect(plan.removeDirs).not.toContain('/work/homes/alice'); + }); + + test('removes legacy top-level job dirs not in the active set', async () => { + const { planWorkdirCleanup } = await load(); + const plan = planWorkdirCleanup({ + root: '/work', + rootEntries: [ + { name: 'homes', isDirectory: true }, + { name: 'legacy-job-123', isDirectory: true }, + { name: 'dev', isDirectory: true }, + ], + homesEntries: {}, + codeLeaves: {}, + activeWorkdirs: new Set(['/work/dev']), + runningBoxUsernames: new Set(), + }); + expect(plan.removeDirs).toContain('/work/legacy-job-123'); + expect(plan.removeDirs).not.toContain('/work/dev'); + expect(plan.removeDirs).not.toContain('/work/homes'); + }); + + test('removes per-thread checkouts only for users with no running box', async () => { + const { planWorkdirCleanup } = await load(); + const plan = planWorkdirCleanup({ + root: '/work', + rootEntries: [{ name: 'homes', isDirectory: true }], + homesEntries: { + alice: [{ name: 'Code', isDirectory: true }], + bob: [{ name: 'Code', isDirectory: true }], + }, + codeLeaves: { + alice: ['/work/homes/alice/Code/spoon-a/branch-x'], + bob: ['/work/homes/bob/Code/spoon-b/branch-y'], + }, + activeWorkdirs: new Set(), + runningBoxUsernames: new Set(['bob']), + }); + expect(plan.removeDirs).toContain('/work/homes/alice/Code/spoon-a/branch-x'); + expect(plan.removeDirs).not.toContain('/work/homes/bob/Code/spoon-b/branch-y'); + // never the home root, never Code + expect(plan.removeDirs).not.toContain('/work/homes/alice'); + expect(plan.removeDirs).not.toContain('/work/homes/alice/Code'); + }); +}); +``` + +2. - [ ] Run `cd apps/agent-worker && bun run test:unit -- cleanup-layout` — expect FAIL (`planWorkdirCleanup` is not exported). + +3. - [ ] Implement the pure helper. Add near the top of `apps/agent-worker/src/worker.ts` (after the `slugify` helper, before `runClaim`): + +```ts +// Pure cleanup planner: decides which host directories under the worker workdir +// are safe to remove. NEVER returns `homes/`, a home root, or a home's `Code/` +// dir. Removes (a) legacy pre-Phase-2 top-level job dirs and (b) per-thread +// checkouts (homes/{user}/Code/{spoon}/{branch}) for users with no running box. +export const planWorkdirCleanup = (args: { + root: string; + rootEntries: { name: string; isDirectory: boolean }[]; + homesEntries: Record; + codeLeaves: Record; + activeWorkdirs: Set; + runningBoxUsernames: Set; +}): { removeDirs: string[] } => { + const removeDirs: string[] = []; + for (const entry of args.rootEntries) { + if (!entry.isDirectory || entry.name.startsWith('.')) continue; + if (entry.name === 'homes') continue; // never touch the homes tree here + const abs = path.resolve(args.root, entry.name); + if (args.activeWorkdirs.has(abs)) continue; + removeDirs.push(abs); // legacy top-level job dir + } + for (const [username, leaves] of Object.entries(args.codeLeaves)) { + if (args.runningBoxUsernames.has(username)) continue; // box is live: keep + for (const leaf of leaves) removeDirs.push(leaf); + } + return { removeDirs }; +}; +``` + +4. - [ ] Run `cd apps/agent-worker && bun run test:unit -- cleanup-layout` — expect PASS. + +5. - [ ] Wire the planner into `cleanupOrphanedWorkspaces`. Replace the body (~1911-1954) so it: enumerates BOTH container prefixes, builds the tree inputs from the filesystem, calls `planWorkdirCleanup`, and removes only the planned dirs. Real code: + +```ts +export const cleanupOrphanedWorkspaces = async () => { + const activeContainers = new Set( + [...activeWorkspaces.values()] + .map((workspace) => workspace.containerName) + .filter((value): value is string => Boolean(value)), + ); + const activeWorkdirs = new Set( + [...activeWorkspaces.values()].map((workspace) => + path.resolve(workspace.workdir), + ), + ); + const removedContainers: string[] = []; + // Only reap legacy per-job containers; boxes are owned by the registry. + for (const containerName of await listWorkspaceContainerNames( + 'spoon-agent-job-', + )) { + if (activeContainers.has(containerName)) continue; + await stopWorkspaceContainer(containerName); + removedContainers.push(containerName); + } + + const root = path.resolve(env.workdir); + const removedWorkdirs: string[] = []; + try { + const rootDirents = await readdir(root, { withFileTypes: true }); + const rootEntries = rootDirents.map((d) => ({ + name: d.name, + isDirectory: d.isDirectory(), + })); + const homesRoot = path.join(root, 'homes'); + const homesEntries: Record< + string, + { name: string; isDirectory: boolean }[] + > = {}; + const codeLeaves: Record = {}; + const homesDirents = await readdir(homesRoot, { + withFileTypes: true, + }).catch(() => []); + for (const userDir of homesDirents) { + if (!userDir.isDirectory()) continue; + const username = userDir.name; + const codeRoot = path.join(homesRoot, username, 'Code'); + homesEntries[username] = []; + const leaves: string[] = []; + const spoons = await readdir(codeRoot, { withFileTypes: true }).catch( + () => [], + ); + for (const spoonDir of spoons) { + if (!spoonDir.isDirectory()) continue; + const spoonRoot = path.join(codeRoot, spoonDir.name); + const branches = await readdir(spoonRoot, { + withFileTypes: true, + }).catch(() => []); + for (const branchDir of branches) { + if (branchDir.isDirectory()) { + leaves.push(path.join(spoonRoot, branchDir.name)); + } + } + } + codeLeaves[username] = leaves; + } + const { removeDirs } = planWorkdirCleanup({ + root, + rootEntries, + homesEntries, + codeLeaves, + activeWorkdirs, + runningBoxUsernames: runningBoxUsernames(), + }); + for (const target of removeDirs) { + await rm(target, { recursive: true, force: true }); + removedWorkdirs.push(target); + } + } catch (error) { + const code = error && typeof error === 'object' ? 'code' in error : false; + if (!code || (error as { code?: string }).code !== 'ENOENT') throw error; + } + + const boxContainers = await listWorkspaceContainerNames('spoon-box-'); + return { success: true, removedContainers, removedWorkdirs, boxContainers }; +}; +``` + +6. - [ ] Add `runningBoxUsernames` import to worker.ts: change the import from `./user-container` (line ~44) to `import { acquireUserBox, releaseUserBox, runningBoxUsernames } from './user-container';`. + +7. - [ ] Export `runningBoxUsernames` from `apps/agent-worker/src/user-container.ts` (append): + +```ts +// Usernames whose box is currently registered (held or adopted). Cleanup uses +// this to avoid deleting a live user's per-thread checkouts. +export const runningBoxUsernames = (): Set => new Set(boxes.keys()); +``` + +8. - [ ] Update `getWorkerHealth` (~1888) so `workspaceContainers` enumerates boxes too. Replace the single `listWorkspaceContainerNames('spoon-agent-job-')` call with: + +```ts + const jobContainers = await listWorkspaceContainerNames('spoon-agent-job-'); + const boxContainers = await listWorkspaceContainerNames('spoon-box-'); +``` +and in the returned object replace `workspaceContainers: containerNames,` with `workspaceContainers: jobContainers,` and add `boxContainers,`. + +9. - [ ] Run `cd apps/agent-worker && bun run test:unit` and `bun run typecheck` — expect PASS. + +10. - [ ] Commit: `git add -A && git commit -m "fix(worker): make workspace cleanup layout-aware so it never deletes user homes"` + +--- + +## Task 2: Box lifecycle correctness (mutex, idempotent handle release, startup reconcile, `--init`) + +**Why:** `acquireUserBox`/`releaseUserBox` use a clamped shared counter (`Math.max(0, refs-1)`), so a double-release reaps a box that another thread/terminal still uses. Concurrent acquires race `docker run`. A terminal disconnect *during* acquire leaks a ref forever (`terminal.ts` sets `acquired=false` until the await resolves, so `cleanup` never releases). Worker restarts orphan all `spoon-box-*`. + +**Files:** +- Rewrite `apps/agent-worker/src/user-container.ts`. +- Modify `apps/agent-worker/src/runtime/docker.ts` (`ensureUserContainer` ~341-383: add `--init`). +- Modify `apps/agent-worker/src/worker.ts` (store a `boxHandle` on `ActiveWorkspace`; release via handle in `runClaim` catch, `openWorkspacePullRequest`, `stopWorkspace`). +- Modify `apps/agent-worker/src/terminal.ts` (use the handle; release on disconnect-during-acquire). +- Modify `apps/agent-worker/src/index.ts` (call `reconcileExistingBoxes()` at startup). +- Create `apps/agent-worker/tests/unit/user-container.test.ts`. + +**Interfaces:** +- `export type BoxHandle = { boxName: string; release: () => void };` +- `export const acquireUserBox = (args: { username: string; workdir: string; containerHome: string }) => Promise` (was `Promise`). +- `export const runningBoxUsernames = () => Set` (kept; now = usernames with refs.size > 0 OR adopted). +- `export const reconcileExistingBoxes = () => Promise` — adopt existing `spoon-box-*` with zero refs so the idle reaper applies. +- `export const _resetBoxRegistryForTests = () => void` — clears the map + timers (test-only). + +**Steps:** + +1. - [ ] Write failing test `apps/agent-worker/tests/unit/user-container.test.ts`. `vi.mock('../../src/runtime/docker')` and `vi.mock('../../src/env')`; use fake timers for the idle reaper. Example: + +```ts +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; + +vi.mock('../../src/env', () => ({ env: { boxIdleMs: 1000 } })); +vi.mock('../../src/runtime/docker', () => ({ + ensureUserContainer: vi.fn(async (a: { username: string }) => `spoon-box-${a.username}`), + stopWorkspaceContainer: vi.fn(async () => {}), + userContainerName: (u: string) => `spoon-box-${u}`, + listWorkspaceContainerNames: vi.fn(async () => []), +})); + +const load = async () => await import('../../src/user-container'); + +describe('box registry', () => { + beforeEach(() => vi.useFakeTimers()); + afterEach(async () => { + const m = await load(); + m._resetBoxRegistryForTests(); + vi.useRealTimers(); + vi.resetModules(); + vi.clearAllMocks(); + }); + + test('serializes concurrent acquire into a single docker run', async () => { + const m = await load(); + const docker = await import('../../src/runtime/docker'); + const [a, b] = await Promise.all([ + m.acquireUserBox({ username: 'alice', workdir: '/w', containerHome: '/home/alice' }), + m.acquireUserBox({ username: 'alice', workdir: '/w', containerHome: '/home/alice' }), + ]); + expect(docker.ensureUserContainer).toHaveBeenCalledTimes(1); + expect(a.boxName).toBe('spoon-box-alice'); + // two refs held -> release one -> not reaped + a.release(); + vi.advanceTimersByTime(2000); + expect(docker.stopWorkspaceContainer).not.toHaveBeenCalled(); + b.release(); + vi.advanceTimersByTime(2000); + expect(docker.stopWorkspaceContainer).toHaveBeenCalledTimes(1); + }); + + test('double-release of one handle does not reap a box held elsewhere', async () => { + const m = await load(); + const docker = await import('../../src/runtime/docker'); + const a = await m.acquireUserBox({ username: 'bob', workdir: '/w', containerHome: '/home/bob' }); + const b = await m.acquireUserBox({ username: 'bob', workdir: '/w', containerHome: '/home/bob' }); + a.release(); + a.release(); // idempotent: must NOT drop b's ref + vi.advanceTimersByTime(2000); + expect(docker.stopWorkspaceContainer).not.toHaveBeenCalled(); + b.release(); + vi.advanceTimersByTime(2000); + expect(docker.stopWorkspaceContainer).toHaveBeenCalledTimes(1); + }); + + test('reconcileExistingBoxes adopts orphans with zero refs and reaps them', async () => { + const m = await load(); + const docker = await import('../../src/runtime/docker'); + (docker.listWorkspaceContainerNames as unknown as ReturnType).mockResolvedValueOnce(['spoon-box-carol']); + await m.reconcileExistingBoxes(); + expect(m.runningBoxUsernames().has('carol')).toBe(true); + vi.advanceTimersByTime(2000); + expect(docker.stopWorkspaceContainer).toHaveBeenCalledWith('spoon-box-carol'); + }); +}); +``` + +2. - [ ] Run `cd apps/agent-worker && bun run test:unit -- user-container` — expect FAIL. + +3. - [ ] Rewrite `apps/agent-worker/src/user-container.ts` in full: + +```ts +import { env } from './env'; +import { + ensureUserContainer, + listWorkspaceContainerNames, + stopWorkspaceContainer, + userContainerName, +} from './runtime/docker'; + +// Phase 2: one persistent "box" container per user. Reference-counted by opaque +// handles (not a shared integer), serialized per-username with an async mutex, +// and idle-reaped once no handle is held. +export type BoxHandle = { boxName: string; release: () => void }; + +type Box = { name: string; refs: Set; idleTimer?: ReturnType }; +const boxes = new Map(); +const locks = new Map>(); + +// Per-username async mutex: chain each operation after the previous one. +const withLock = (username: string, fn: () => Promise): Promise => { + const prev = locks.get(username) ?? Promise.resolve(); + const next = prev.then(fn, fn); + locks.set( + username, + next.then( + () => undefined, + () => undefined, + ), + ); + return next; +}; + +const scheduleReapIfIdle = (username: string) => { + const box = boxes.get(username); + if (!box || box.refs.size > 0) return; + if (box.idleTimer) clearTimeout(box.idleTimer); + box.idleTimer = setTimeout(() => { + void stopWorkspaceContainer(userContainerName(username)); + boxes.delete(username); + }, env.boxIdleMs); +}; + +const makeHandle = (username: string, box: Box): BoxHandle => { + const token = Symbol('box-ref'); + box.refs.add(token); + let released = false; + return { + boxName: box.name, + release: () => { + if (released) return; // idempotent + released = true; + box.refs.delete(token); + scheduleReapIfIdle(username); + }, + }; +}; + +export const acquireUserBox = (args: { + username: string; + workdir: string; + containerHome: string; +}): Promise => + withLock(args.username, async () => { + let box = boxes.get(args.username); + if (box?.idleTimer) { + clearTimeout(box.idleTimer); + box.idleTimer = undefined; + } + if (!box) { + box = { name: userContainerName(args.username), refs: new Set() }; + boxes.set(args.username, box); + } + // Register the ref BEFORE the (slow) docker call so a disconnect during + // acquire has a handle to release and never leaks. + const handle = makeHandle(args.username, box); + try { + box.name = await ensureUserContainer(args); + return handle; + } catch (error) { + handle.release(); + throw error; + } + }); + +// Adopt any pre-existing spoon-box-* containers (from a prior worker process) +// into the registry with zero refs so the idle reaper cleans them up. +export const reconcileExistingBoxes = async (): Promise => { + const names = await listWorkspaceContainerNames('spoon-box-'); + for (const name of names) { + const username = name.replace(/^spoon-box-/, ''); + if (boxes.has(username)) continue; + const box: Box = { name, refs: new Set() }; + boxes.set(username, box); + scheduleReapIfIdle(username); + } +}; + +export const runningBoxUsernames = (): Set => new Set(boxes.keys()); + +export const _resetBoxRegistryForTests = () => { + for (const box of boxes.values()) { + if (box.idleTimer) clearTimeout(box.idleTimer); + } + boxes.clear(); + locks.clear(); +}; +``` + +4. - [ ] Run `cd apps/agent-worker && bun run test:unit -- user-container` — expect PASS. + +5. - [ ] Add `--init` to the box in `apps/agent-worker/src/runtime/docker.ts` `ensureUserContainer` (~360). In the `run` args array, insert `'--init',` immediately after `'-d',`: + +```ts + [ + 'run', + '-d', + '--init', + '--name', + name, +``` +This is pure-infra (needs a real container to observe zombie reaping). Manual verification: `docker inspect --format '{{.HostConfig.Init}}' spoon-box-` prints `true` after a job runs; documented, no unit test. + +6. - [ ] Update `apps/agent-worker/src/worker.ts` to store and release the handle instead of a username string. + - Add to the `ActiveWorkspace` type (after `boxName: string;`, ~line 110): `boxHandle: BoxHandle;` + - Import the type: change the `./user-container` import to `import { acquireUserBox, runningBoxUsernames } from './user-container';` and `import type { BoxHandle } from './user-container';` (drop `releaseUserBox`). + - In `runClaim` (~1330): change `let acquiredBoxUser: string | undefined;` to `let acquiredBoxHandle: BoxHandle | undefined;`. Replace `const boxName = await acquireUserBox({...}); acquiredBoxUser = username;` with: + +```ts + const boxHandle = await acquireUserBox({ + username, + workdir: homeDir, + containerHome, + }); + acquiredBoxHandle = boxHandle; + const boxName = boxHandle.boxName; +``` + - In the workspace object literal (~1348) add `boxHandle,`. + - In the `catch` (~1436) replace `if (acquiredBoxUser) releaseUserBox(acquiredBoxUser);` with `acquiredBoxHandle?.release();`. + - In `openWorkspacePullRequest` (~1859) replace `releaseUserBox(workspace.username);` with `workspace.boxHandle.release();`. + - In `stopWorkspace` (~1876) replace `releaseUserBox(workspace.username);` with `workspace.boxHandle.release();`. + +7. - [ ] Update `apps/agent-worker/src/terminal.ts` to use the handle and release on disconnect-during-acquire. + - Change import (line 9) to `import { acquireUserBox } from './user-container';` and `import type { BoxHandle } from './user-container';`. + - Replace the acquire/cleanup block (~66-98). Real code: + +```ts + const handleHolder: { current?: BoxHandle } = {}; + let released = false; + const isReleased = () => released; + const cleanup = () => { + if (released) return; + released = true; + procHolder.current?.kill(); + handleHolder.current?.release(); + }; + ws.on('close', cleanup); + ws.on('error', cleanup); + + let boxName: string; + try { + const handle = await acquireUserBox({ + username: workspace.username, + workdir: workspace.workdir, + containerHome: workspace.containerHome, + }); + handleHolder.current = handle; + boxName = handle.boxName; + } catch (error) { + ws.close( + 1011, + `Failed to start terminal: ${error instanceof Error ? error.message : 'unknown error'}`, + ); + return; + } + + // If the socket already closed during acquire, release the box now: cleanup + // ran before the handle existed, so nothing has released it. + if (isReleased()) { + handleHolder.current.release(); + return; + } +``` + (Delete the old `let acquired`/`acquired = true` logic.) + +8. - [ ] Call reconcile at startup. In `apps/agent-worker/src/index.ts`, change the import line `import { startWorker } from './worker';` region to also import reconcile and call it before `startWorker`: + +```ts +import { reconcileExistingBoxes } from './user-container'; +``` +and just before `await startWorker();` at the bottom add: +```ts +await reconcileExistingBoxes(); +``` + +9. - [ ] Run `cd apps/agent-worker && bun run test:unit && bun run typecheck` — expect PASS. (If `releaseUserBox` is referenced anywhere else, `grep -rn releaseUserBox apps/agent-worker/src` returns nothing.) + +10. - [ ] Commit: `git add -A && git commit -m "fix(worker): make per-user box registry concurrency-safe with idempotent handles and startup reconcile"` + +--- + +## Task 3: Symlink containment for all worker file access + +**Why (High):** `safeWorkspacePath` (`worker.ts:1059`) and `safeHomeJoin` (`user-environment.ts:35`) validate the path *lexically* then `readFile`/`writeFile` follow symlinks. A repo containing a symlink (e.g. `link -> /home/otheruser` or `-> /proc/self/environ`) lets the `file` GET/PUT and dotfile-overlay routes read/write outside the checkout — other users' homes and worker secrets. + +**Files:** +- Create `apps/agent-worker/src/path-containment.ts` (shared helper). +- Modify `apps/agent-worker/src/worker.ts` (`safeWorkspacePath` → async containment for reads/writes at `readWorkspaceFile`, `writeWorkspaceFile`, `materializeEnvFile`). +- Modify `apps/agent-worker/src/user-environment.ts` (`safeHomeJoin` write targets). +- Create `apps/agent-worker/tests/unit/path-containment.test.ts`. + +**Interfaces:** +- `export const assertContainedRealPath = async (root: string, requested: string, opts?: { forWrite?: boolean }) => Promise` — resolves the lexical target, then `fs.realpath`s the deepest existing ancestor; throws if the real path escapes `realpath(root)`. For writes, if the final target exists and is a symlink, throw. + +**Steps:** + +1. - [ ] Write failing test `apps/agent-worker/tests/unit/path-containment.test.ts` using real fs in an OS temp dir (no env import needed, so no module mocking): + +```ts +import { mkdtemp, mkdir, symlink, writeFile, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, describe, expect, test } from 'vitest'; +import { assertContainedRealPath } from '../../src/path-containment'; + +let dir: string; +afterEach(async () => { if (dir) await rm(dir, { recursive: true, force: true }); }); + +describe('assertContainedRealPath', () => { + test('allows a normal file inside the root', async () => { + dir = await mkdtemp(path.join(tmpdir(), 'spoon-')); + const root = path.join(dir, 'repo'); + await mkdir(root, { recursive: true }); + await writeFile(path.join(root, 'a.txt'), 'hi'); + await expect(assertContainedRealPath(root, 'a.txt')).resolves.toBe( + path.join(root, 'a.txt'), + ); + }); + + test('rejects a lexical .. escape', async () => { + dir = await mkdtemp(path.join(tmpdir(), 'spoon-')); + const root = path.join(dir, 'repo'); + await mkdir(root, { recursive: true }); + await expect(assertContainedRealPath(root, '../secret')).rejects.toThrow(); + }); + + test('rejects reading through a symlink that escapes the root', async () => { + dir = await mkdtemp(path.join(tmpdir(), 'spoon-')); + const root = path.join(dir, 'repo'); + const outside = path.join(dir, 'outside'); + await mkdir(root, { recursive: true }); + await mkdir(outside, { recursive: true }); + await writeFile(path.join(outside, 'secret'), 'S'); + await symlink(outside, path.join(root, 'link')); + await expect(assertContainedRealPath(root, 'link/secret')).rejects.toThrow(); + }); + + test('rejects a write whose final target is a symlink', async () => { + dir = await mkdtemp(path.join(tmpdir(), 'spoon-')); + const root = path.join(dir, 'repo'); + await mkdir(root, { recursive: true }); + await symlink(path.join(dir, 'evil'), path.join(root, 'out')); + await expect( + assertContainedRealPath(root, 'out', { forWrite: true }), + ).rejects.toThrow(); + }); +}); +``` + +2. - [ ] Run `cd apps/agent-worker && bun run test:unit -- path-containment` — expect FAIL (module missing). + +3. - [ ] Create `apps/agent-worker/src/path-containment.ts`: + +```ts +import { lstat, realpath } from 'node:fs/promises'; +import path from 'node:path'; + +const realpathOrDeepestExisting = async (target: string): Promise => { + let current = target; + // Walk up to the deepest ancestor that exists, realpath it, then re-append + // the not-yet-created tail. This lets writes create new files while still + // resolving any symlinked directory in the existing prefix. + const tail: string[] = []; + for (;;) { + try { + const real = await realpath(current); + return path.join(real, ...tail.reverse()); + } catch (error) { + if ((error as { code?: string }).code !== 'ENOENT') throw error; + const parent = path.dirname(current); + if (parent === current) throw error; + tail.push(path.basename(current)); + current = parent; + } + } +}; + +// Resolve `requested` under `root`, following symlinks, and assert the real +// path stays inside the real root. For writes, also reject when the final +// target itself is a symlink. +export const assertContainedRealPath = async ( + root: string, + requested: string, + opts: { forWrite?: boolean } = {}, +): Promise => { + const lexical = path.resolve(root, requested); + const realRoot = await realpath(root); + const resolved = await realpathOrDeepestExisting(lexical); + if (resolved !== realRoot && !resolved.startsWith(`${realRoot}${path.sep}`)) { + throw new Error(`Refusing to access path outside root: ${requested}`); + } + if (opts.forWrite) { + const info = await lstat(lexical).catch(() => null); + if (info?.isSymbolicLink()) { + throw new Error(`Refusing to write through a symlink: ${requested}`); + } + } + return resolved; +}; +``` + +4. - [ ] Run `cd apps/agent-worker && bun run test:unit -- path-containment` — expect PASS. + +5. - [ ] Use it in `apps/agent-worker/src/worker.ts`. Add `import { assertContainedRealPath } from './path-containment';` near the top imports. + - `readWorkspaceFile` (~1477): replace `const target = safeWorkspacePath(workspace.repoDir, filePath);` with `const target = await assertContainedRealPath(workspace.repoDir, filePath);`. + - `writeWorkspaceFile` (~1483): replace `const target = safeWorkspacePath(workspace.repoDir, filePath);` with `const target = await assertContainedRealPath(workspace.repoDir, filePath, { forWrite: true });`. + - `materializeEnvFile` (~1170): replace `const envPath = safeWorkspacePath(repoDir, claim.job.envFilePath);` with `const envPath = await assertContainedRealPath(repoDir, claim.job.envFilePath, { forWrite: true });`. + - Leave the synchronous `safeWorkspacePath` in place only if still referenced; `grep -rn safeWorkspacePath apps/agent-worker/src` — if no remaining references, delete the function (~1059-1066). + +6. - [ ] Use it in `apps/agent-worker/src/user-environment.ts` overlay writes. Add `import { assertContainedRealPath } from './path-containment';`. In the overlay loop (~113): replace `const target = safeHomeJoin(homeDir, file.path);` with `const target = await assertContainedRealPath(homeDir, file.path, { forWrite: true });`. Remove the now-unused `safeHomeJoin` (~35-42) if `grep -rn safeHomeJoin apps/agent-worker/src` shows no other users. + +7. - [ ] Run `cd apps/agent-worker && bun run test:unit && bun run typecheck` — expect PASS. + +8. - [ ] Commit: `git add -A && git commit -m "fix(worker): resolve realpath and reject symlinked targets on all workspace file access"` + +--- + +## Task 4: `updateStatus` rejects writes to terminal jobs (fixes cancel-revert) + +**Why (Critical):** `cancel` sets `status: 'cancelled'`, but the worker's next `updateStatus` (e.g. `running`) flips the job back out of `cancelled` because `updateStatus` accepts any transition as long as `claimedBy` matches. The container keeps running. Fix: `updateStatus` no-ops when the job is already in a terminal status. + +**Files:** +- Modify `packages/backend/convex/agentJobs.ts` (add `isTerminalStatus`; guard in `updateStatus` ~1108). +- Create `packages/backend/tests/unit/update-status.test.ts`. + +**Interfaces:** +- Add `const TERMINAL_STATUSES = new Set(['failed', 'cancelled', 'timed_out', 'draft_pr_opened']);` and `const isTerminalStatus = (s: string) => TERMINAL_STATUSES.has(s);` in `agentJobs.ts`. +- `updateStatus` returns `{ success: true }` or `{ success: true, ignored: true }`. + +**Steps:** + +1. - [ ] Write failing test `packages/backend/tests/unit/update-status.test.ts` modeled on `harness.test.ts` (convexTest, `import.meta.glob`, `authed`/`createUser`). Seed a job with `status: 'cancelled'` and `claimedBy: 'worker-1'`, call `api.agentJobs.updateStatus` with `status: 'running'`, assert the job stays `cancelled` and the result is `{ success: true, ignored: true }`. Then a second test: a `running` job accepts `checks_running`. Set `process.env.SPOON_WORKER_TOKEN = 'test-worker-token'` in the test and pass `workerToken: 'test-worker-token'`. Example core: + +```ts +import { convexTest } from 'convex-test'; +import { describe, expect, test, beforeAll } from 'vitest'; +import { api } from '../../convex/_generated/api.js'; +import schema from '../../convex/schema'; + +const modules = import.meta.glob('../../convex/**/*.*s'); +beforeAll(() => { process.env.SPOON_WORKER_TOKEN = 'test-worker-token'; }); + +const seedJob = async (t, status) => + await t.mutation(async (ctx) => { + const now = Date.now(); + const ownerId = await ctx.db.insert('users', { email: 'a@b.c', name: 'A' }); + const spoonId = await ctx.db.insert('spoons', { /* minimal required spoon fields — copy from harness.test.ts spoonInput + ownerId + status:'active' + createdAt/updatedAt */ }); + const requestId = await ctx.db.insert('agentRequests', { spoonId, ownerId, prompt: 'x', status: 'running', createdAt: now, updatedAt: now }); + return await ctx.db.insert('agentJobs', { + spoonId, ownerId, agentRequestId: requestId, status, prompt: 'x', runtime: 'opencode', + baseBranch: 'main', workBranch: 'spoon/x', forkOwner: 'team', forkRepo: 'r', forkUrl: 'u', + upstreamOwner: 'up', upstreamRepo: 'r', selectedSecretIds: [], model: '', reasoningEffort: 'medium', + claimedBy: 'worker-1', createdAt: now, updatedAt: now, + }); + }); + +describe('updateStatus terminal guard', () => { + test('ignores writes to a cancelled job', async () => { + const t = convexTest(schema, modules); + const jobId = await seedJob(t, 'cancelled'); + const res = await t.mutation(api.agentJobs.updateStatus, { + workerToken: 'test-worker-token', workerId: 'worker-1', jobId, status: 'running', + }); + expect(res).toEqual({ success: true, ignored: true }); + const job = await t.run(async (ctx) => ctx.db.get(jobId)); + expect(job?.status).toBe('cancelled'); + }); +}); +``` +(Use the exact `spoonInput` shape from `packages/backend/tests/unit/harness.test.ts` for the spoon insert.) + +2. - [ ] Run `bun codegen:convex` (repo root) then `cd packages/backend && bun run test:unit -- update-status` — expect FAIL (result lacks `ignored`, status flips to running). + +3. - [ ] Implement. In `packages/backend/convex/agentJobs.ts`, add near `isTerminalJob` (~243): + +```ts +const TERMINAL_STATUSES = new Set([ + 'failed', + 'cancelled', + 'timed_out', + 'draft_pr_opened', +]); +const isTerminalStatus = (status: string) => TERMINAL_STATUSES.has(status); +``` +In `updateStatus.handler` (~1117) after the `claimedBy` check add: + +```ts + if (isTerminalStatus(job.status)) { + // The Convex state machine is authoritative: never resurrect a terminal + // job (fixes the cancel-then-worker-write revert). + return { success: true, ignored: true as const }; + } +``` + +4. - [ ] Run `cd packages/backend && bun run test:unit -- update-status` — expect PASS. + +5. - [ ] Commit: `git add -A && git commit -m "fix(convex): reject status writes to terminal agent jobs so cancel sticks"` + +--- + +## Task 5: Validate everything inside `claimNextInternal` before patching to `claimed` + +**Why (Critical):** `claimNextInternal` patches the job to `claimed` unconditionally, then the *action* `claimNextForWorker` validates spoon/profile/secrets and throws — stranding the job in `claimed` forever (no worker owns it, no recovery). Move validation into the mutation so a bad job goes `failed` with a reason and the poller receives `null`. + +**Files:** +- Modify `packages/backend/convex/agentJobs.ts` (`claimNextInternal` ~1046). +- Modify `packages/backend/convex/agentJobsNode.ts` (`claimNextForWorker` ~50 — drop the now-redundant throws; keep decryption). +- Create `packages/backend/tests/unit/claim-validation.test.ts`. + +**Interfaces:** +- `claimNextInternal` still returns the same `ClaimedJob | null` shape on success; returns `null` after marking a job `failed` when validation fails (so the poller simply gets no work and does not throw). + +**Steps:** + +1. - [ ] Write failing test `packages/backend/tests/unit/claim-validation.test.ts`. Seed a queued job whose `aiProviderProfileId` is undefined and the owner has no configured profile. Call `internal.agentJobs.claimNextInternal` via `t.mutation(internal.agentJobs.claimNextInternal, { workerId: 'w1' })`. Assert it returns `null` and the job is now `status: 'failed'` with a non-empty `error`. Second test: a fully valid queued job returns a non-null claim and status `claimed`. + +```ts +import { internal } from '../../convex/_generated/api'; +// ... seed a queued job with aiProviderProfileId undefined ... +const res = await t.mutation(internal.agentJobs.claimNextInternal, { workerId: 'w1' }); +expect(res).toBeNull(); +const job = await t.run((ctx) => ctx.db.get(jobId)); +expect(job?.status).toBe('failed'); +expect(job?.error).toBeTruthy(); +``` + +2. - [ ] Run `bun codegen:convex && cd packages/backend && bun run test:unit -- claim-validation` — expect FAIL. + +3. - [ ] Implement in `claimNextInternal` (~1046). After loading `spoon`, `aiProviderProfile`, and `secrets` but BEFORE the `ctx.db.patch(job._id, { status: 'claimed', ... })`, add a validation block that, on failure, patches the job to `failed` and returns `null`: + +```ts + const ownedProfile = + aiProviderProfile?.ownerId === job.ownerId ? aiProviderProfile : null; + const failClaim = async (reason: string) => { + const failedAt = Date.now(); + await ctx.db.patch(job._id, { + status: 'failed', + error: reason, + completedAt: failedAt, + updatedAt: failedAt, + }); + await ctx.db.patch(job.agentRequestId, { + status: 'failed', + updatedAt: failedAt, + }); + if (job.threadId) { + await ctx.db.patch(job.threadId, { + status: 'failed', + updatedAt: failedAt, + resolvedAt: failedAt, + }); + } + await ctx.db.insert('agentJobEvents', { + jobId: job._id, + spoonId: job.spoonId, + ownerId: job.ownerId, + level: 'error', + phase: 'queued', + message: `Job could not be claimed: ${reason}`, + createdAt: failedAt, + }); + }; + if (!spoon) { + await failClaim('The Spoon for this job no longer exists.'); + return null; + } + if (!ownedProfile) { + await failClaim( + 'AI is not configured for this user. Add an AI provider in settings.', + ); + return null; + } + if (ownedProfile.authType !== 'none' && !ownedProfile.encryptedSecret) { + await failClaim('Selected AI provider is missing credentials.'); + return null; + } +``` +Then the existing `patch(job._id, { status: 'claimed', ... })` and `return { ..., aiProviderProfile: ownedProfile, ... }` proceed (change the returned `aiProviderProfile` to `ownedProfile`). + +4. - [ ] Simplify `claimNextForWorker` in `agentJobsNode.ts`: the mutation now guarantees a valid `spoon` and `aiProviderProfile`, but keep defensive non-throwing handling — replace the three `throw new ConvexError(...)` guards (~64-77) with an early `if (!claimed.spoon || !claimed.aiProviderProfile) return null;` (the job is already `failed`, so returning null just moves on). Keep the decryption/return mapping. + +5. - [ ] Run `bun codegen:convex && cd packages/backend && bun run test:unit -- claim-validation` — expect PASS. Also re-run `bun run test:unit` (full) to ensure no regression. + +6. - [ ] Commit: `git add -A && git commit -m "fix(convex): validate spoon/profile/secrets inside claim so bad jobs fail instead of wedging"` + +--- + +## Task 6: Worker heartbeats + recovery cron + cancel propagation + +**Why (Critical):** `lastHeartbeatAt` is never written by the worker and never read by any recovery path, so a worker crash strands non-terminal jobs forever. And the worker is never told about a user `cancel`. Fix: (a) worker heartbeats every ~30s per active workspace; (b) `heartbeatWorkspace` returns `cancelRequested` so the worker tears down; (c) a recovery cron times out stale non-terminal jobs. + +**Files:** +- Modify `packages/backend/convex/agentJobs.ts` (`heartbeatWorkspace` ~1358 returns `cancelRequested`; add `recoverStaleJobs` internalMutation). +- Modify `packages/backend/convex/crons.ts` (register the recovery cron). +- Modify `apps/agent-worker/src/worker.ts` (heartbeat loop per active workspace; teardown on cancel). +- Create `packages/backend/tests/unit/recover-stale-jobs.test.ts`. + +**Interfaces:** +- `heartbeatWorkspace` returns `{ success: true, cancelRequested: boolean }` where `cancelRequested = job.status === 'cancelled'`. +- `export const recoverStaleJobs = internalMutation({ args: { staleMs: v.optional(v.number()) }, ... })` — for jobs with `status ∈ {claimed, preparing, running, checks_running}` whose `lastHeartbeatAt` (or `claimedAt` if no heartbeat) is older than `staleMs` (default 180000), set `status: 'timed_out'`, thread `status: 'failed'`. +- Worker: `startHeartbeat(jobId)` / `stopHeartbeat(jobId)` managing `setInterval` handles in a `Map`. + +**Steps:** + +1. - [ ] Write failing test `packages/backend/tests/unit/recover-stale-jobs.test.ts`: seed a `running` job with `lastHeartbeatAt = Date.now() - 10*60*1000` and `claimedBy: 'w1'` (+ its thread). Call `t.mutation(internal.agentJobs.recoverStaleJobs, {})`. Assert job `status === 'timed_out'` and thread `status === 'failed'`. Second test: a `running` job with a fresh `lastHeartbeatAt = Date.now()` is left untouched. + +2. - [ ] Run `bun codegen:convex && cd packages/backend && bun run test:unit -- recover-stale-jobs` — expect FAIL. + +3. - [ ] Implement `recoverStaleJobs` in `agentJobs.ts` (add near the other internal mutations; ensure `internalMutation` is imported — it already is): + +```ts +export const recoverStaleJobs = internalMutation({ + args: { staleMs: v.optional(v.number()) }, + handler: async (ctx, { staleMs }) => { + const threshold = Date.now() - (staleMs ?? 3 * 60 * 1000); + const activeStatuses = [ + 'claimed', + 'preparing', + 'running', + 'checks_running', + ] as const; + let recovered = 0; + for (const status of activeStatuses) { + const jobs = await ctx.db + .query('agentJobs') + .withIndex('by_status', (q) => q.eq('status', status)) + .collect(); + for (const job of jobs) { + const last = job.lastHeartbeatAt ?? job.claimedAt ?? job.createdAt; + if (last > threshold) continue; + const now = Date.now(); + await ctx.db.patch(job._id, { + status: 'timed_out', + error: + job.error ?? + 'The worker stopped reporting progress; the job timed out.', + completedAt: now, + updatedAt: now, + }); + await ctx.db.patch(job.agentRequestId, { + status: 'failed', + updatedAt: now, + }); + if (job.threadId) { + await ctx.db.patch(job.threadId, { + status: 'failed', + updatedAt: now, + resolvedAt: now, + }); + } + recovered += 1; + } + } + return { recovered }; + }, +}); +``` + +4. - [ ] Update `heartbeatWorkspace` (~1358) to return `cancelRequested`. After the `patch`, replace `return { success: true };` with: + +```ts + return { success: true, cancelRequested: job.status === 'cancelled' }; +``` +(Note: when `job.status === 'cancelled'`, the heartbeat still patches `workspaceStatus`/`lastHeartbeatAt` but never changes `status`, so the cancel stands.) + +5. - [ ] Register the cron in `packages/backend/convex/crons.ts` (after the existing refresh cron): + +```ts +crons.interval( + 'Recover stale agent jobs', + { minutes: 1 }, + internal.agentJobs.recoverStaleJobs, + {}, +); +``` + +6. - [ ] Run `bun codegen:convex && cd packages/backend && bun run test:unit -- recover-stale-jobs` — expect PASS. + +7. - [ ] Add the worker heartbeat loop + cancel teardown in `apps/agent-worker/src/worker.ts`. + - Add a wrapper mutation helper near `markWorkspaceStopped` (~244): + +```ts +const heartbeatWorkspaceMutation = async (jobId: Id<'agentJobs'>) => + await client.mutation(api.agentJobs.heartbeatWorkspace, { + workerToken: env.workerToken, + workerId: env.workerId, + jobId, + }); + +const heartbeatTimers = new Map>(); + +const stopHeartbeat = (jobId: string) => { + const timer = heartbeatTimers.get(jobId); + if (timer) clearInterval(timer); + heartbeatTimers.delete(jobId); +}; + +const startHeartbeat = (jobId: Id<'agentJobs'>) => { + stopHeartbeat(jobId); + const timer = setInterval(() => { + void (async () => { + try { + const result = await heartbeatWorkspaceMutation(jobId); + if (result?.cancelRequested && activeWorkspaces.has(jobId)) { + await appendEvent(jobId, 'warn', 'cleanup', 'Cancellation requested; stopping workspace.'); + await abortWorkspaceAgent(jobId).catch(() => {}); + await stopWorkspace(jobId).catch(() => {}); + stopHeartbeat(jobId); + } + } catch (error) { + console.error(error); + } + })(); + }, 30_000); + timer.unref?.(); + heartbeatTimers.set(jobId, timer); +}; +``` + - In `runClaim`, right after `await markWorkspaceActive({ jobId });` (~1388) add `startHeartbeat(jobId);`. + - In `stopWorkspace` (~1866) and `openWorkspacePullRequest` (~1851), add `stopHeartbeat(jobId);` right before `activeWorkspaces.delete(jobId);`. + - In `runClaim`'s `catch` (~1436), add `stopHeartbeat(jobId);` alongside `acquiredBoxHandle?.release();`. + +8. - [ ] Run `cd apps/agent-worker && bun run typecheck` — expect PASS (no new worker unit test; the heartbeat interval is integration-level. The backend behavior is covered by Task 6 step 1-3 and Task 4's terminal guard). Document the manual check: queue a job, `POST /cleanup` off; call `api.agentJobs.cancel`; within ~30s the worker logs "Cancellation requested" and the box is released. + +9. - [ ] Commit: `git add -A && git commit -m "feat(worker,convex): heartbeat active workspaces, recover stale jobs via cron, and tear down on cancel"` + +--- + +## Task 7: `applyMaintenanceDecision` closes the job and stops the workspace + +**Why (Critical):** After a parsed maintenance review, the thread is resolved but the job stays `running` and its box keeps running. Fix: `applyMaintenanceDecision` marks the job terminal (`workspaceStatus: 'stopped'` + `completedAt`), and the worker tears the workspace down after applying the decision. + +**Files:** +- Modify `packages/backend/convex/agentJobs.ts` (`applyMaintenanceDecision` ~1423: patch the job too). +- Modify `apps/agent-worker/src/worker.ts` (`sendWorkspaceMessage` maintenance branch ~1742: stop the workspace after applying). +- Create `packages/backend/tests/unit/maintenance-decision.test.ts`. + +**Interfaces:** unchanged signatures; `applyMaintenanceDecision` now also patches the job. Note: `isTerminalJob` already treats `workspaceStatus ∈ {stopped, expired, failed}` as terminal, so setting `workspaceStatus: 'stopped'` closes the job for `createForThread`'s active-job guard and marks it recovered-safe. + +**Steps:** + +1. - [ ] Write failing test `packages/backend/tests/unit/maintenance-decision.test.ts`: seed a `running` maintenance job with a `threadId`, `claimedBy: 'w1'`. Call `api.agentJobs.applyMaintenanceDecision` with a `sync`/low-risk decision (`requiresUserApproval: false`). Assert the thread `status === 'resolved'` AND the job `workspaceStatus === 'stopped'` with a `completedAt` set. + +2. - [ ] Run `bun codegen:convex && cd packages/backend && bun run test:unit -- maintenance-decision` — expect FAIL. + +3. - [ ] Implement in `applyMaintenanceDecision` (~1436). After the guard `if (!job.threadId) return { success: true };` and before/after the thread patch, add a job patch. Insert right after `const now = Date.now();` (~1443): + +```ts + await ctx.db.patch(args.jobId, { + workspaceStatus: 'stopped', + summary: args.summary, + completedAt: job.completedAt ?? now, + updatedAt: now, + }); +``` +(Keep the rest of the handler as-is.) + +4. - [ ] In the worker, after applying the decision, stop the workspace. In `apps/agent-worker/src/worker.ts` `sendWorkspaceMessage`, in the `maintenance_review` branch (~1742), after `await applyMaintenanceDecision(claim.job._id, decision);` add: + +```ts + await stopWorkspace(claim.job._id).catch((error: unknown) => { + console.error(error); + }); + return; +``` +(The early `return` skips the trailing diff/artifact recording, which is unnecessary once the workspace is torn down. `stopWorkspace` is defined later in the module — it is a hoisted `const` exported function, so reference is fine at call time.) + +5. - [ ] Run `bun codegen:convex && cd packages/backend && bun run test:unit -- maintenance-decision` and `cd apps/agent-worker && bun run typecheck` — expect PASS. + +6. - [ ] Commit: `git add -A && git commit -m "fix(convex,worker): close the job and stop the box after a maintenance decision"` + +--- + +## Task 8: Auto-sync honors settings + migration + +**Why (High):** `refreshOwnedSpoon` fast-forwards the fork whenever `status === 'behind' && forkAheadBy === 0`, and creates maintenance-review threads on `diverged`, ignoring `autoSyncEnabled` (default false), `requireCleanCompareForSync`, and `autoReviewEnabled`. Gate both on `spoonSettings`. Migration flips existing/new GitHub spoons to `autoSyncEnabled=true` to preserve today's observed behavior. + +**Files:** +- Create `packages/backend/convex/syncGating.ts` (pure gating helpers, no `'use node'`). +- Modify `packages/backend/convex/githubSync.ts` (`refreshOwnedSpoon` ~48: load settings, gate auto-sync and thread creation). +- Modify `packages/backend/convex/github.ts` (`createForkSpoonRecord` ~193: default `autoSyncEnabled: true`). +- Create `packages/backend/convex/migrations.ts` (internalMutation to backfill existing spoonSettings). +- Create `packages/backend/tests/unit/sync-gating.test.ts`. + +**Interfaces:** +- `export const shouldAutoSync = (settings: { autoSyncEnabled: boolean; requireCleanCompareForSync: boolean }, ctx: { status: string; forkAheadBy: number }): boolean` — true only when `autoSyncEnabled` and `status === 'behind'` and `forkAheadBy === 0` (a clean compare). +- `export const shouldCreateReviewThread = (settings: { autoReviewEnabled: boolean }, ctx: { status: string }): boolean` — true only when `autoReviewEnabled` and `status === 'diverged'`. +- `export const backfillAutoSyncDefaults = internalMutation(...)` — for every `spoonSettings` row whose spoon is a GitHub spoon, set `autoSyncEnabled: true` if not already. + +**Steps:** + +1. - [ ] Write failing test `packages/backend/tests/unit/sync-gating.test.ts` (pure unit test, node env — put it under `tests/unit/`): + +```ts +import { describe, expect, test } from 'vitest'; +import { shouldAutoSync, shouldCreateReviewThread } from '../../convex/syncGating'; + +describe('syncGating', () => { + test('auto-sync requires the flag and a clean behind compare', () => { + expect(shouldAutoSync({ autoSyncEnabled: false, requireCleanCompareForSync: true }, { status: 'behind', forkAheadBy: 0 })).toBe(false); + expect(shouldAutoSync({ autoSyncEnabled: true, requireCleanCompareForSync: true }, { status: 'behind', forkAheadBy: 0 })).toBe(true); + expect(shouldAutoSync({ autoSyncEnabled: true, requireCleanCompareForSync: true }, { status: 'behind', forkAheadBy: 2 })).toBe(false); + expect(shouldAutoSync({ autoSyncEnabled: true, requireCleanCompareForSync: true }, { status: 'diverged', forkAheadBy: 0 })).toBe(false); + }); + test('review thread requires autoReviewEnabled and diverged', () => { + expect(shouldCreateReviewThread({ autoReviewEnabled: false }, { status: 'diverged' })).toBe(false); + expect(shouldCreateReviewThread({ autoReviewEnabled: true }, { status: 'diverged' })).toBe(true); + expect(shouldCreateReviewThread({ autoReviewEnabled: true }, { status: 'behind' })).toBe(false); + }); +}); +``` + +2. - [ ] Run `cd packages/backend && bun run test:unit -- sync-gating` — expect FAIL. + +3. - [ ] Create `packages/backend/convex/syncGating.ts` (NO `'use node'`; keep it importable by both actions and tests): + +```ts +export const shouldAutoSync = ( + settings: { autoSyncEnabled: boolean; requireCleanCompareForSync: boolean }, + ctx: { status: string; forkAheadBy: number }, +): boolean => { + if (!settings.autoSyncEnabled) return false; + if (ctx.status !== 'behind') return false; + if (settings.requireCleanCompareForSync && ctx.forkAheadBy !== 0) return false; + return ctx.forkAheadBy === 0; +}; + +export const shouldCreateReviewThread = ( + settings: { autoReviewEnabled: boolean }, + ctx: { status: string }, +): boolean => settings.autoReviewEnabled && ctx.status === 'diverged'; +``` + +4. - [ ] Run `cd packages/backend && bun run test:unit -- sync-gating` — expect PASS. + +5. - [ ] Wire gating into `refreshOwnedSpoon` (`githubSync.ts`). Add imports: `import { shouldAutoSync, shouldCreateReviewThread } from './syncGating';`. Immediately after resolving `spoon` (~66), load settings: + +```ts + const settings = await ctx.runQuery(internal.spoonSettings.getInternal, { + spoonId, + ownerId, + }); + const autoSyncEnabled = settings?.autoSyncEnabled ?? false; + const autoReviewEnabled = settings?.autoReviewEnabled ?? true; + const requireCleanCompareForSync = settings?.requireCleanCompareForSync ?? true; +``` + - Change the auto-sync condition (~205) from `if (status === 'behind' && forkCompare.aheadBy === 0 && allowAutoSync) {` to: + +```ts + if ( + allowAutoSync && + shouldAutoSync( + { autoSyncEnabled, requireCleanCompareForSync }, + { status, forkAheadBy: forkCompare.aheadBy }, + ) + ) { +``` + - Change the diverged review-thread condition (~261) from `if (status === 'diverged') {` to: + +```ts + if (shouldCreateReviewThread({ autoReviewEnabled }, { status })) { +``` + (The `requireAiLowRiskForSync` flag gates the *AI review → auto-merge* path, which does not exist in Phase 1; leave it for Phase 4. Note this in a code comment.) + +6. - [ ] Flip the default for new GitHub spoons: in `packages/backend/convex/github.ts` `createForkSpoonRecord` (~193) change `autoSyncEnabled: false,` to `autoSyncEnabled: true,`. + +7. - [ ] Create `packages/backend/convex/migrations.ts` with the backfill: + +```ts +import { internalMutation } from './_generated/server'; + +// One-shot backfill: preserve today's observed behavior (auto fast-forward of +// clean GitHub forks) by enabling autoSyncEnabled on existing GitHub spoons, +// now that refreshOwnedSpoon gates on it. Idempotent. +export const backfillAutoSyncDefaults = internalMutation({ + args: {}, + handler: async (ctx) => { + const settings = await ctx.db.query('spoonSettings').collect(); + let updated = 0; + for (const row of settings) { + if (row.autoSyncEnabled) continue; + const spoon = await ctx.db.get(row.spoonId); + if (spoon?.provider !== 'github') continue; + await ctx.db.patch(row._id, { + autoSyncEnabled: true, + updatedAt: Date.now(), + }); + updated += 1; + } + return { updated }; + }, +}); +``` +Add a test in `sync-gating.test.ts` (or a new `packages/backend/tests/unit/migrations.test.ts`) using convex-test: seed two GitHub spoons (one with `autoSyncEnabled: false`, one already `true`) and a non-GitHub spoon with `false`; run `internal.migrations.backfillAutoSyncDefaults`; assert `{ updated: 1 }` and that only the GitHub `false` row flipped to `true`. + +8. - [ ] Run `bun codegen:convex && cd packages/backend && bun run test:unit` — expect PASS. Document the deploy step: after deploying, run the migration once via `bunx convex run migrations:backfillAutoSyncDefaults` (self-hosted: use the deployment's `convex run`). + +9. - [ ] Commit: `git add -A && git commit -m "feat(convex): gate auto-sync and review threads on spoon settings, backfill GitHub defaults"` + +--- + +## Task 9: Unified trimmed worker-token compare + `getEnvironmentForJob` asserts `claimedBy` + +**Why (Medium, security hygiene):** `userDotfilesNode.requireWorkerToken` (`userDotfilesNode.ts:22`) compares against an untrimmed `process.env.SPOON_WORKER_TOKEN`, diverging from the trimmed comparisons elsewhere — a trailing newline in the env var silently rejects a valid worker. And `getEnvironmentForJob` returns a user's decrypted dotfiles for any job id without checking the job is actually claimed by a worker. + +**Files:** +- Create `packages/backend/convex/workerAuth.ts` (shared trimming compare). +- Modify `packages/backend/convex/agentJobs.ts`, `agentJobsNode.ts`, `userDotfilesNode.ts` to use it. +- Modify `packages/backend/convex/userEnvironment.ts` (`getRawEnvironmentForJobInternal` ~68: return null when the job is not claimed). +- Create `packages/backend/tests/unit/worker-auth.test.ts`. + +**Interfaces:** +- `export const assertWorkerToken = (provided: string): void` — trims both sides; throws `ConvexError('SPOON_WORKER_TOKEN is not configured.')` / `ConvexError('Invalid worker token.')`. + +**Steps:** + +1. - [ ] Write failing test `packages/backend/tests/unit/worker-auth.test.ts`: + +```ts +import { ConvexError } from 'convex/values'; +import { afterEach, describe, expect, test } from 'vitest'; +import { assertWorkerToken } from '../../convex/workerAuth'; + +afterEach(() => { delete process.env.SPOON_WORKER_TOKEN; }); + +describe('assertWorkerToken', () => { + test('accepts a token that matches after trimming', () => { + process.env.SPOON_WORKER_TOKEN = 'secret\n'; + expect(() => assertWorkerToken('secret')).not.toThrow(); + expect(() => assertWorkerToken(' secret ')).not.toThrow(); + }); + test('rejects a mismatched token', () => { + process.env.SPOON_WORKER_TOKEN = 'secret'; + expect(() => assertWorkerToken('nope')).toThrow(ConvexError); + }); + test('throws when not configured', () => { + expect(() => assertWorkerToken('x')).toThrow(ConvexError); + }); +}); +``` + +2. - [ ] Run `cd packages/backend && bun run test:unit -- worker-auth` — expect FAIL. + +3. - [ ] Create `packages/backend/convex/workerAuth.ts`: + +```ts +import { ConvexError } from 'convex/values'; + +// Single source of truth for verifying the shared worker token. Trims both the +// configured secret and the provided token so trailing newlines never cause a +// spurious mismatch. +export const assertWorkerToken = (provided: string): void => { + const expected = process.env.SPOON_WORKER_TOKEN?.trim(); + if (!expected) throw new ConvexError('SPOON_WORKER_TOKEN is not configured.'); + if (provided.trim() !== expected) { + throw new ConvexError('Invalid worker token.'); + } +}; +``` + +4. - [ ] Run `cd packages/backend && bun run test:unit -- worker-auth` — expect PASS. + +5. - [ ] Replace the local `requireWorkerToken` implementations with the shared helper: + - `agentJobs.ts` (~155-161): delete the local `getWorkerToken`/`requireWorkerToken`; add `import { assertWorkerToken } from './workerAuth';`; replace every `requireWorkerToken(args.workerToken)` call with `assertWorkerToken(args.workerToken)`. + - `agentJobsNode.ts` (~44-48): delete local `requireWorkerToken`; import + use `assertWorkerToken`. + - `userDotfilesNode.ts` (~21-25): delete local `requireWorkerToken`; import + use `assertWorkerToken`. (This is the untrimmed one — the actual bug fix.) + +6. - [ ] Assert `claimedBy` in `userEnvironment.getRawEnvironmentForJobInternal` (~68). After `const job = await ctx.db.get(jobId); if (!job) return null;` add: + +```ts + if (!job.claimedBy) return null; // only a claimed job's env is exposed +``` +Add a convex-test to `worker-auth.test.ts` (or a `getEnvironmentForJob` test) verifying an unclaimed job yields `null` — seed a queued job with no `claimedBy`, run `internal.userEnvironment.getRawEnvironmentForJobInternal`, assert `null`; then set `claimedBy: 'w1'` and assert non-null. + +7. - [ ] Run `bun codegen:convex && cd packages/backend && bun run test:unit` — expect PASS. + +8. - [ ] Commit: `git add -A && git commit -m "fix(convex): unify trimmed worker-token compare and require a claimed job for env access"` + +--- + +## Task 10: Terminal worker — real TTY exec via dockerode so resize works + +**Why (High):** The worker drives ` exec -i … script -qfc …` and only sizes the PTY once via `stty` at launch; resize messages from the client just mutate local `cols`/`rows` and never reach the container, so the terminal stays the initial size. Replace the `script` pipe hack with a dockerode exec that allocates a real TTY (`exec.start({ Tty: true })`) and honors `exec.resize({ h, w })`. `dockerode` is already a dependency; it talks to the Docker socket (this deployment uses Docker for the terminal path — `attachTerminalServer` already early-returns unless `env.runtime === 'docker'`). + +**Files:** +- Modify `apps/agent-worker/src/terminal.ts` (`bridge` ~21-150: dockerode exec + resize). +- Modify `apps/agent-worker/src/runtime/docker.ts` (export a shared `Docker` instance / helper). +- Create `apps/agent-worker/tests/unit/terminal-resize.test.ts` (unit test the pure resize-dimension extraction; the dockerode exec itself is verified manually). + +**Interfaces:** +- Extract a pure helper in `terminal.ts`: `export const parseResizeMessage = (data: Buffer, isBinary: boolean): { cols: number; rows: number } | null` — returns clamped dims for a `{type:'resize'}` text frame, else null. Unit-testable without docker. +- `runtime/docker.ts`: `export const getDockerClient = () => Docker` (a singleton `new Docker()` using the default socket). + +**Steps:** + +1. - [ ] Write failing test `apps/agent-worker/tests/unit/terminal-resize.test.ts`. `terminal.ts` imports `./env`, `./worker`, `./user-container`, `ws` — to unit-test only the pure parser, `vi.mock` those heavy imports so importing `terminal.ts` doesn't pull docker/ws: + +```ts +import { afterEach, describe, expect, test, vi } from 'vitest'; + +const load = async () => { + vi.resetModules(); + process.env.SPOON_WORKER_TOKEN = 'test-worker-token'; + process.env.GITHUB_APP_ID = '123'; + process.env.GITHUB_APP_PRIVATE_KEY = + '-----BEGIN PRIVATE KEY-----\\ntest\\n-----END PRIVATE KEY-----'; + vi.doMock('../../src/worker', () => ({ getTerminalWorkspace: () => null })); + vi.doMock('../../src/user-container', () => ({ + acquireUserBox: vi.fn(), + })); + return await import('../../src/terminal'); +}; + +describe('parseResizeMessage', () => { + afterEach(() => vi.resetModules()); + test('parses and clamps a resize control frame', async () => { + const { parseResizeMessage } = await load(); + const msg = Buffer.from(JSON.stringify({ type: 'resize', cols: 120, rows: 40 })); + expect(parseResizeMessage(msg, false)).toEqual({ cols: 120, rows: 40 }); + }); + test('returns null for binary input (raw keystrokes)', async () => { + const { parseResizeMessage } = await load(); + expect(parseResizeMessage(Buffer.from([1, 2, 3]), true)).toBeNull(); + }); + test('returns null for non-resize JSON', async () => { + const { parseResizeMessage } = await load(); + expect(parseResizeMessage(Buffer.from('{"type":"other"}'), false)).toBeNull(); + }); + test('clamps out-of-range dimensions into [1,1000]', async () => { + const { parseResizeMessage } = await load(); + const msg = Buffer.from(JSON.stringify({ type: 'resize', cols: 99999, rows: 0 })); + expect(parseResizeMessage(msg, false)).toEqual({ cols: 1000, rows: 1 }); + }); +}); +``` + +2. - [ ] Run `cd apps/agent-worker && bun run test:unit -- terminal-resize` — expect FAIL. + +3. - [ ] In `apps/agent-worker/src/terminal.ts`, extract and export the parser (reuse the existing `clampDimension`): + +```ts +export const parseResizeMessage = ( + data: Buffer, + isBinary: boolean, +): { cols: number; rows: number } | null => { + if (isBinary) return null; + try { + const message = JSON.parse(data.toString('utf8')) as { + type?: string; + cols?: number; + rows?: number; + }; + if (message.type !== 'resize') return null; + const cols = clampDimension(message.cols); + const rows = clampDimension(message.rows); + if (!cols || !rows) return null; + return { cols, rows }; + } catch { + return null; + } +}; +``` + +4. - [ ] Run `cd apps/agent-worker && bun run test:unit -- terminal-resize` — expect PASS. + +5. - [ ] Add a dockerode singleton to `apps/agent-worker/src/runtime/docker.ts`: + +```ts +import Docker from 'dockerode'; + +let dockerClient: Docker | undefined; +export const getDockerClient = () => { + dockerClient ??= new Docker(); + return dockerClient; +}; +``` +(Add `import Docker from 'dockerode';` at the top.) + +6. - [ ] Rewrite `bridge` in `terminal.ts` to use dockerode. Declare the holders and rewrite the message handler (replacing ~35-64): + +```ts + const execHolder: { + current?: { exec: import('dockerode').Exec; stream: NodeJS.ReadWriteStream }; + } = {}; + const pendingInput: Buffer[] = []; + let cols = 80; + let rows = 24; + + ws.on('message', (data: Buffer, isBinary: boolean) => { + const resize = parseResizeMessage(data, isBinary); + if (resize) { + cols = resize.cols; + rows = resize.rows; + void execHolder.current?.exec.resize({ h: rows, w: cols }).catch(() => {}); + return; + } + if (execHolder.current) execHolder.current.stream.write(data); + else pendingInput.push(data); + }); +``` + - after acquiring the box, create the exec with a TTY and pipe both directions: + +```ts + const docker = getDockerClient(); + const container = docker.getContainer(boxName); + const exec = await container.exec({ + AttachStdin: true, + AttachStdout: true, + AttachStderr: true, + Tty: true, + Cmd: [ + '/bin/bash', + '-lc', + 'if command -v tmux >/dev/null 2>&1; then exec tmux new-session -A -s spoon; else exec bash -il; fi', + ], + Env: [ + 'TERM=xterm-256color', + `HOME=${workspace.containerHome}`, + ...workspace.secrets.map((s) => `${s.name}=${s.value}`), + ], + WorkingDir: workspace.containerRepo, + }); + const stream = await exec.start({ hijack: true, stdin: true, Tty: true }); + execHolder.current = { exec, stream }; + await exec.resize({ h: rows, w: cols }).catch(() => {}); + // replay buffered input, then bridge + for (const buffered of pendingInput) stream.write(buffered); + pendingInput.length = 0; + stream.on('data', (chunk: Buffer) => { + if (ws.readyState === ws.OPEN) ws.send(chunk, { binary: true }); + }); + stream.on('end', () => { if (ws.readyState === ws.OPEN) ws.close(); }); +``` + - update `cleanup` to `stream.end()`/destroy instead of `procHolder.current?.kill()`; keep `handleHolder.current?.release()` from Task 2. + - in the resize branch of `ws.on('message')`, after updating `cols`/`rows`, add: `void execHolder.current?.exec.resize({ h: rows, w: cols }).catch(() => {});` + - remove the now-unused `spawn`, `shellQuote`, `script -qfc`, `stty` launcher, and `ChildProcessWithoutNullStreams` imports. + +7. - [ ] Run `cd apps/agent-worker && bun run test:unit && bun run typecheck` — expect PASS. + +8. - [ ] Manual verification (pure-infra; cannot unit-test dockerode against a real socket). With a running box: open the workspace terminal, run `tput cols; tput lines`, resize the browser pane, run again — the values track the pane. `printf '\ue0b0'` renders the powerline glyph. tmux reattach still works across reconnects. Documented as manual because the exec path needs a live Docker daemon. + +9. - [ ] Commit: `git add -A && git commit -m "fix(worker): drive the terminal through a dockerode TTY exec so resize actually resizes the PTY"` + +--- + +## Task 11: Terminal client — re-fit on fonts.ready + Nerd-Font load + rAF first fit + +**Why (High):** xterm measures cell size in `fit()` immediately after `term.open()`, before the Victor Mono webfont loads, so the grid is computed against a fallback metric and renders quarter-size; nothing re-fits when the real font arrives (`workspace-terminal.tsx:145`). Monaco already fixes this via `document.fonts.ready`. + +**Files:** +- Create `apps/next/src/components/agent-workspace/terminal-fit.ts` (pure fit-scheduling helper). +- Modify `apps/next/src/components/agent-workspace/workspace-terminal.tsx` (use the helper; first fit in rAF; re-fit + resize on fonts). +- Create `apps/next/tests/unit/terminal-fit.test.ts`. + +**Interfaces:** +- `export const scheduleTerminalFits = (deps: { fit: () => void; sendResize: () => void; fontsReady: Promise; loadNerdFont: () => Promise; raf: (cb: () => void) => void; isAborted: () => boolean }) => void` — runs `fit()`+`sendResize()` in the next animation frame, then again after `fontsReady`, then again after `loadNerdFont`, skipping when `isAborted()`. + +**Steps:** + +1. - [ ] Write failing test `apps/next/tests/unit/terminal-fit.test.ts` (node env — pure logic, no DOM): + +```ts +import { describe, expect, test, vi } from 'vitest'; +import { scheduleTerminalFits } from '@/components/agent-workspace/terminal-fit'; + +const flush = async () => { await Promise.resolve(); await Promise.resolve(); }; + +describe('scheduleTerminalFits', () => { + test('fits in rAF and again after fonts.ready and nerd-font load', async () => { + const fit = vi.fn(); + const sendResize = vi.fn(); + let rafCb: (() => void) | undefined; + scheduleTerminalFits({ + fit, + sendResize, + fontsReady: Promise.resolve(), + loadNerdFont: () => Promise.resolve(), + raf: (cb) => { rafCb = cb; }, + isAborted: () => false, + }); + rafCb?.(); + await flush(); + expect(fit).toHaveBeenCalledTimes(3); // rAF + fonts.ready + nerd font + expect(sendResize).toHaveBeenCalledTimes(3); + }); + test('does nothing once aborted', async () => { + const fit = vi.fn(); + scheduleTerminalFits({ + fit, sendResize: vi.fn(), + fontsReady: Promise.resolve(), + loadNerdFont: () => Promise.resolve(), + raf: (cb) => cb(), + isAborted: () => true, + }); + await flush(); + expect(fit).not.toHaveBeenCalled(); + }); +}); +``` + +2. - [ ] Run `cd apps/next && bun run test:unit -- terminal-fit` — expect FAIL. + +3. - [ ] Create `apps/next/src/components/agent-workspace/terminal-fit.ts`: + +```ts +export const scheduleTerminalFits = (deps: { + fit: () => void; + sendResize: () => void; + fontsReady: Promise; + loadNerdFont: () => Promise; + raf: (cb: () => void) => void; + isAborted: () => boolean; +}): void => { + const refit = () => { + if (deps.isAborted()) return; + deps.fit(); + deps.sendResize(); + }; + deps.raf(refit); + void deps.fontsReady.then(refit).catch(() => undefined); + void deps.loadNerdFont().then(refit).catch(() => undefined); +}; +``` + +4. - [ ] Run `cd apps/next && bun run test:unit -- terminal-fit` — expect PASS. + +5. - [ ] Use it in `workspace-terminal.tsx`. Replace the block after `term.open(container); fit.fit(); termRef.current = term;` and the standalone `document.fonts.load(...)` (~145-156). New code (keep `sendResize` defined before this call — move its declaration up if needed): + +```ts + term.open(container); + termRef.current = term; + + const sendResize = () => { + if (ws?.readyState !== WebSocket.OPEN) return; + ws.send( + JSON.stringify({ type: 'resize', cols: term.cols, rows: term.rows }), + ); + }; + + scheduleTerminalFits({ + fit: () => { + try { + fit.fit(); + } catch { + // ignore transient layout errors + } + }, + sendResize, + fontsReady: document.fonts.ready, + loadNerdFont: () => + document.fonts + .load("16px 'Symbols Nerd Font Mono'", '\ue0b0') + .then(() => { + if (!isAborted()) term.refresh(0, term.rows - 1); + }), + raf: (cb) => requestAnimationFrame(cb), + isAborted, + }); +``` + - Delete the later duplicate `const sendResize = () => {...}` (~158-163) now that it is defined above. + - Add the import at the top of the file: `import { scheduleTerminalFits } from './terminal-fit';` + +6. - [ ] Run `cd apps/next && bun run test:unit -- terminal-fit && bun run typecheck` — expect PASS. + +7. - [ ] Commit: `git add -A && git commit -m "fix(next): re-fit the workspace terminal on fonts.ready and Nerd-Font load"` + +--- + +## Task 12: Key `AgentWorkspaceShell` by job on both routes + +**Why (High):** Navigating thread A → B keeps the same `AgentWorkspaceShell` React instance, so it shows A's files and overwrites B's persisted UI state with A's. Adding `key={jobId}` forces a fresh mount per job. + +**Files:** +- Modify `apps/next/src/app/(app)/threads/[threadId]/page.tsx` (~56). +- Modify `apps/next/src/app/(app)/spoons/[spoonId]/agent/[jobId]/page.tsx` (~40). +- Create `apps/next/tests/component/agent-workspace-shell-key.test.tsx`. + +**Interfaces:** none changed; add `key`. + +**Steps:** + +1. - [ ] Write failing component test `apps/next/tests/component/agent-workspace-shell-key.test.tsx`. Because the real routes pull in Convex/xterm, test the *keying contract* directly: a tiny wrapper that renders a mount-counting child with `key={jobId}` and asserts a jobId change remounts it (mount counter increments, state resets). Match the render style of `apps/next/tests/component/render.test.tsx`: + +```ts +import { render } from '@testing-library/react'; +import { describe, expect, test } from 'vitest'; +import { useEffect } from 'react'; + +let mounts = 0; +const Child = ({ jobId }: { jobId: string }) => { + useEffect(() => { mounts += 1; }, []); + return {jobId}; +}; +const Shell = ({ jobId }: { jobId: string }) => ; + +describe('workspace shell keyed by job', () => { + test('remounts when jobId changes', () => { + mounts = 0; + const { rerender } = render(); + expect(mounts).toBe(1); + rerender(); + expect(mounts).toBe(2); + }); +}); +``` +This encodes the expected pattern (a keyed child remounts on id change). It passes once the pattern is present; it is a regression guard for the pattern the routes must use. + +2. - [ ] Run `cd apps/next && bun run test:component -- agent-workspace-shell-key` — expect PASS (it validates the keying contract). If your harness requires the failing-first cycle, first assert `mounts).toBe(1)` after rerender (wrong) to see it FAIL, then correct to `2`. + +3. - [ ] Apply the key in both routes: + - `threads/[threadId]/page.tsx` (~56): ``. + - `spoons/[spoonId]/agent/[jobId]/page.tsx` (~40): ``. + +4. - [ ] Run `cd apps/next && bun run typecheck && bun run test:component -- agent-workspace-shell-key` — expect PASS. + +5. - [ ] Commit: `git add -A && git commit -m "fix(next): remount AgentWorkspaceShell per job so thread switches don't cross state"` + +--- + +## Task 13: Not-found handling — `(app)/error.tsx` + `not-found`, queries return null + +**Why (Medium):** `threads.get` and `agentJobs.get` throw `ConvexError` for a missing/foreign id, which bubbles to `global-error.tsx` (a full-page crash) instead of a friendly in-app not-found. Change these read queries to return `null` for missing/unowned, add an `(app)/error.tsx` + `not-found.tsx`, and make the two detail pages distinguish loading (`undefined`) from missing (`null`). + +**Files:** +- Modify `packages/backend/convex/agentJobs.ts` (`get` ~707: return `null` instead of throw). +- Modify `packages/backend/convex/threads.ts` (`get` ~209: return `null` instead of throw). +- Create `apps/next/src/app/(app)/error.tsx` and `apps/next/src/app/(app)/not-found.tsx`. +- Modify `apps/next/src/app/(app)/spoons/[spoonId]/agent/[jobId]/page.tsx` and `.../threads/[threadId]/page.tsx` to handle `null`. +- Create `packages/backend/tests/unit/detail-null.test.ts`. + +**Interfaces:** +- `agentJobs.get`: returns `Doc<'agentJobs'> | null`. +- `threads.get`: returns `{ thread, spoon, latestJob } | null`. + +**Steps:** + +1. - [ ] Write failing test `packages/backend/tests/unit/detail-null.test.ts`: authed user A creates a thread + a job; authed user B calls `api.threads.get`/`api.agentJobs.get` with A's ids and gets `null` (not a throw). Also a genuinely nonexistent id returns `null`. Use the `authed()`/`createUser()` helpers from `harness.test.ts`. Assert `await expect(...).resolves.toBeNull()`. + +2. - [ ] Run `bun codegen:convex && cd packages/backend && bun run test:unit -- detail-null` — expect FAIL (currently throws). + +3. - [ ] Implement: + - `agentJobs.get` (~707): replace `if (job?.ownerId !== ownerId) throw new ConvexError('Agent job not found.');` with `if (job?.ownerId !== ownerId) return null;`. + - `threads.get` (~209): replace `if (thread?.ownerId !== ownerId) throw new ConvexError('Thread not found.');` with `if (thread?.ownerId !== ownerId) return null;`. + (Leave the other owner-mutating functions throwing; only the read-detail `get`s change.) + +4. - [ ] Run `bun codegen:convex && cd packages/backend && bun run test:unit -- detail-null` — expect PASS. + +5. - [ ] Create `apps/next/src/app/(app)/error.tsx` (client component; friendly, matches app shell — the `(app)/layout.tsx` wraps it in `AppShell`): + +```tsx +'use client'; + +import { useEffect } from 'react'; +import * as Sentry from '@sentry/nextjs'; +import { Button } from '@spoon/ui'; + +const AppError = ({ + error, + reset, +}: { + error: Error & { digest?: string }; + reset: () => void; +}) => { + useEffect(() => { + Sentry.captureException(error); + }, [error]); + return ( +
+

Something went wrong

+

+ This page hit an unexpected error. It has been reported. +

+ +
+ ); +}; + +export default AppError; +``` + +6. - [ ] Create `apps/next/src/app/(app)/not-found.tsx`: + +```tsx +import Link from 'next/link'; +import { Button } from '@spoon/ui'; + +const NotFound = () => ( +
+

Not found

+

+ This item does not exist, or you do not have access to it. +

+ +
+); + +export default NotFound; +``` + +7. - [ ] Handle `null` in the detail pages (distinguish loading `undefined` from missing `null`): + - `spoons/[spoonId]/agent/[jobId]/page.tsx`: after `const job = useQuery(api.agentJobs.get, { jobId });` add, before the `useEffect`: + +```tsx + if (job === undefined) { + return
Loading workspace...
; + } + if (job === null) { + return ( +
+

This workspace was not found.

+ +
+ ); + } +``` + (Keep the existing `useEffect`/`threadId` redirect below; note React hooks must run unconditionally — since `useQuery` and `useEffect` are already declared before these returns, move the early returns to AFTER the `useEffect` declaration to preserve hook order. Concretely: keep `const job = useQuery(...)` and the `useEffect(...)` at the top, then the `undefined`/`null` guards, then the render.) + - `threads/[threadId]/page.tsx`: `details` is already guarded for `undefined` (~42). Add a `null` guard right after: `if (details === null) { return (
This thread was not found.
); }` and destructure only after. (Note `threads.get` now returns `null`; TypeScript will require this guard before `const { thread, spoon, latestJob } = details;`.) + +8. - [ ] Run `bun codegen:convex && cd apps/next && bun run typecheck` and `cd packages/backend && bun run test:unit` — expect PASS. + +9. - [ ] Commit: `git add -A && git commit -m "feat(next,convex): return null for missing detail queries and add friendly app error/not-found states"` + +--- + +## Final verification + +- [ ] From repo root: `bun codegen:convex`. +- [ ] `cd apps/agent-worker && bun run test:unit && bun run typecheck`. +- [ ] `cd packages/backend && bun run test:unit && bun run typecheck`. +- [ ] `cd apps/next && bun run test:unit && bun run test:component && bun run typecheck`. +- [ ] Confirm `grep -rn "releaseUserBox\|safeWorkspacePath\|safeHomeJoin" apps/agent-worker/src` returns no stale references (all replaced). +- [ ] Post-deploy one-shot: `bunx convex run migrations:backfillAutoSyncDefaults` against the target deployment. diff --git a/docs/superpowers/plans/2026-07-10-spoon-phase2-runtime-unification.md b/docs/superpowers/plans/2026-07-10-spoon-phase2-runtime-unification.md new file mode 100644 index 0000000..d7f58b3 --- /dev/null +++ b/docs/superpowers/plans/2026-07-10-spoon-phase2-runtime-unification.md @@ -0,0 +1,545 @@ +# Spoon Phase 2 — Runtime Unification: Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Collapse the three agent runtimes (Codex, OpenCode, Claude Code) behind one `AgentRuntime` adapter interface in the worker, run all three *inside* the per-user `spoon-box-{username}` container (no per-job containers), and let users pick the runtime per thread / per spoon with queue-time validation against their AI provider profile. + +**Architecture:** The worker (`apps/agent-worker`) polls Convex, claims a job, acquires the user's persistent box (Phase 1), clones `~/Code/{spoon}/{branch}` into the mounted home, then runs agent turns by `docker exec`-ing a CLI into the box. Today dispatch is implicit (Codex if the profile is a ChatGPT-login profile, otherwise a *separate* `opencode serve` container). Phase 2 makes `job.runtime` (`codex|opencode|claude`) authoritative: a registry maps it to an adapter; every adapter execs into the box via `streamExecInContainer`, tags its process group with a unique marker, and kills that group in-box on timeout/abort. The OpenCode server-in-a-side-container path is deleted. The Codex `auth.json` encrypted-snapshot pattern is generalized to Claude OAuth credentials. + +**Tech Stack:** TypeScript, Bun, execa (docker/podman CLI), Convex (`packages/backend/convex`), Vitest (worker unit tests + convex-test backend tests), Next.js 16 (`apps/next`), Fedora 41 job image (`docker/agent-job.Dockerfile`) shipping `opencode`, `@openai/codex`, and (new) `@anthropic-ai/claude-code`. + +**Depends on:** Phase 1 (box lifecycle: per-username mutex, idempotent acquire/release handles, `--init` on the box, heartbeat + `cancelRequested` teardown, startup reconcile of `spoon-box-*`). This plan assumes `acquireUserBox`/`releaseUserBox` are already concurrency-safe and the box runs with `--init` so orphaned in-box children are reaped. Where Phase-1 handle semantics are referenced, use whatever `acquireUserBox` returns after Phase 1; if Phase 1 kept the `string` box-name return used in `worker.ts:1330`, keep using it. + +## Global Constraints + +- **Worker unit tests** live in `apps/agent-worker/tests/unit/*.test.ts`; run `cd apps/agent-worker && bun run test:unit` (`vitest run --project unit`). **Worker typecheck:** `cd apps/agent-worker && bun run typecheck`. +- **Backend tests** live in `packages/backend/tests/unit/*.test.ts` using `convex-test` (see `packages/backend/tests/unit/harness.test.ts` for the exact fixture style: `convexTest(schema, modules)`, `authed(t, userId)`, `spoonInput`/`githubSpoonInput`). Run `cd packages/backend && bun run test:unit`. **Always `cd packages/backend && bun run codegen` before `bun run typecheck`** (codegen regenerates `_generated/` after schema/function changes). +- Match the existing fixture/test style: `expect(normalizeX(...)).toContainEqual({ kind: ... })` for event normalizers (see `apps/agent-worker/tests/unit/agent-events.test.ts`); `mkdtemp` + `afterEach` cleanup for filesystem tests (see `apps/agent-worker/tests/unit/codex-runtime.test.ts`). +- **Conventional commits; one commit per task.** Run the relevant `test:unit` + `typecheck` (and `codegen` for backend) before committing. Do not commit with a failing suite. +- **Three runtimes: `codex`, `opencode`, `claude` — all `docker exec` into `spoon-box-{username}`. No per-job containers.** Never leave a `spoon-agent-job-*` code path behind. +- A real turn against a live CLI cannot run in unit tests. Adapters therefore take an **injectable exec function** so `runTurn` can be driven with fixture stream lines; live behavior is covered by the extended `scripts/smoke-agent-container` manual step. Event parsing is covered by pure normalizer fixtures. + +--- + +## Task 1: Define the `AgentRuntime` adapter interface, shared types, and registry + +**Files:** +- Create `apps/agent-worker/src/runtime/agent-runtime.ts` (new). +- Create `apps/agent-worker/src/runtime/provider.ts` (new — pure helpers moved out of `worker.ts` so adapters don't import `worker.ts` at runtime). +- Edit `apps/agent-worker/src/worker.ts` (extract the `Claim` type and pure helpers; make `ActiveWorkspace` extend the shared workspace shape; add `runtime`/`turnMarker` fields). +- Create `apps/agent-worker/tests/unit/agent-runtime.test.ts` (new). + +**Interfaces:** + +*Produces* (`runtime/agent-runtime.ts`): +```ts +import type { NormalizedAgentEvent } from '../agent-events'; +import type { streamExecInContainer } from './docker'; +import type { AdapterWorkspace } from './provider'; + +export type AgentRuntimeName = 'codex' | 'opencode' | 'claude'; + +export type ExecStreamFn = typeof streamExecInContainer; + +export type TurnResult = { + // Assistant text the adapter captured out-of-band (Codex --output-last-message + // file, Claude `result` event). Empty/undefined when everything streamed via onEvent. + finalMessage?: string; + // Runtime session id to persist for continuity (resume/--session/--resume). + sessionId?: string; + // Non-empty when the runtime reported a hard failure the caller must surface. + error?: string; +}; + +export type AgentRuntime = { + readonly name: AgentRuntimeName; + prepareAuth(workspace: AdapterWorkspace): Promise; + runTurn( + workspace: AdapterWorkspace, + prompt: string, + onEvent: (event: NormalizedAgentEvent) => Promise, + ): Promise; + abort(workspace: AdapterWorkspace): Promise; +}; + +export type AdapterFactory = (deps?: { execStream?: ExecStreamFn }) => AgentRuntime; +``` + +*Produces* (`runtime/provider.ts`) — `AdapterWorkspace` is the subset of `ActiveWorkspace` adapters read, plus the pure helpers currently in `worker.ts:47-95, 350-446`: +```ts +export type Claim = { /* moved verbatim from worker.ts:47-95, but runtime widened */ }; +// job.runtime union becomes: runtime?: 'codex' | 'opencode' | 'claude'; + +export type AdapterWorkspace = { + claim: Claim; + workdir: string; + homeDir: string; + username: string; + containerHome: string; // e.g. /home/{username} + containerRepo: string; // e.g. /home/{username}/Code/{spoon}/{branch} + repoDir: string; // host path to the checkout + boxName: string; // spoon-box-{username} + redact: (value: string) => string; + runtime: AgentRuntimeName; + codexSessionId?: string; + opencodeSessionId?: string; + claudeSessionId?: string; + turnMarker?: string; // set per turn; used for in-box process-group kill +}; + +export const isCodexLoginProfile: (claim: Claim) => boolean; // moved from worker.ts:350 +export const collectJsonStringValues: (value?: string) => string[]; // moved from worker.ts:354 +export const providerEnvironment: (claim: Claim, workspaceRoot?: string) => Record; // moved from worker.ts:379 +export const opencodeModel: (claim: Claim) => string; // moved from worker.ts:425 +export const codexModel: (claim: Claim) => string; // moved from worker.ts:440 +export const codexModelArgs: (claim: Claim) => string[]; // moved from worker.ts:445 +``` + +*Consumes:* `NormalizedAgentEvent` from `agent-events.ts`; `streamExecInContainer` from `runtime/docker.ts`. + +**Registry** (`runtime/agent-runtime.ts`): a lazy map. Adapters register in later tasks; until then `getAdapter` throws for unknown names. +```ts +const factories = new Map(); +export const registerAdapter = (name: AgentRuntimeName, factory: AdapterFactory): void => { factories.set(name, factory); }; +export const getAdapter = (name: AgentRuntimeName): AgentRuntime => { + const factory = factories.get(name); + if (!factory) throw new Error(`No agent runtime adapter registered for "${name}".`); + return factory(); +}; +``` + +**Steps:** +- [ ] Write a failing test `apps/agent-worker/tests/unit/agent-runtime.test.ts`: `registerAdapter('codex', () => fake)` then `expect(getAdapter('codex').name).toBe('codex')`, and `expect(() => getAdapter('opencode')).toThrow(/No agent runtime adapter/)`. Also assert `isCodexLoginProfile`/`opencodeModel` re-exported from `runtime/provider.ts` behave as before (copy one existing expectation, e.g. `opencodeModel({ aiProviderProfile: { provider: 'anthropic', model: 'claude-x', ... } } as Claim)` → `'anthropic/claude-x'`). +- [ ] Run `cd apps/agent-worker && bun run test:unit` → FAIL (module/exports missing). +- [ ] Create `runtime/provider.ts`: move `Claim` (worker.ts:47-95), `isCodexLoginProfile`, `collectJsonStringValues`, `providerEnvironment`, `opencodeModel`, `codexModel`, `codexModelArgs` (worker.ts:350-446) verbatim; widen `Claim['job']['runtime']` to `'codex' | 'opencode' | 'claude'`; add and export `AdapterWorkspace`. +- [ ] Create `runtime/agent-runtime.ts` with the interface, `TurnResult`, `ExecStreamFn`, `AdapterFactory`, and the registry above. +- [ ] Edit `worker.ts`: import `Claim`, the helpers, and `AdapterWorkspace` from `runtime/provider.ts` (delete the now-moved definitions); change `ActiveWorkspace` (worker.ts:97) to `type ActiveWorkspace = AdapterWorkspace & { githubToken: string; runtimeMode?: ...; agentTurnActive?: boolean; resolveTurn?: () => void; lastRecordedDiffSignature?: string; codexTurnError?: string; ... }`; set `runtime` when the workspace is built in `runClaim` (worker.ts:1348 — temporarily `runtime: 'codex'` placeholder; real resolution lands in Task 3). +- [ ] Run `cd apps/agent-worker && bun run test:unit` → PASS. Run `bun run typecheck` → PASS. +- [ ] Commit: `refactor(worker): extract AgentRuntime interface, shared provider helpers, and adapter registry`. + +--- + +## Task 2: In-box process-group kill + marked-command helpers + +**Files:** +- Edit `apps/agent-worker/src/runtime/docker.ts` (add two exported helpers near `streamExecInContainer` at line 385). +- Create `apps/agent-worker/tests/unit/box-process-kill.test.ts` (new). + +**Interfaces:** + +*Produces* (`runtime/docker.ts`): +```ts +// Wraps a CLI argv so the process runs as a new session/process-group leader whose +// bash parent carries the marker in its argv (matchable by `pgrep -f`). We do NOT +// `exec` (bash must survive so the marker stays visible); stdout/stderr fds are +// inherited by the CLI so streaming still works. +export const buildMarkedCommand = (marker: string, command: string[]): string[] => { + const script = `# ${marker}\nexec 0 => { + const script = [ + `pids=$(pgrep -f ${shellQuote(args.marker)} || true)`, + `for pid in $pids; do kill -TERM -"$pid" 2>/dev/null || true; done`, + `sleep 2`, + `for pid in $pids; do kill -KILL -"$pid" 2>/dev/null || true; done`, + ].join('\n'); + await execa(containerRuntime(), ['exec', args.containerName, 'bash', '-lc', script], { + reject: false, stdin: 'ignore', + }); +}; +``` +Add a local `const shellQuote = (v: string) => `'${v.replaceAll("'", "'\\''")}'`;` (mirror `worker.ts:1049`). + +*Consumes:* `execa`, `containerRuntime()` (already in file). + +**Steps:** +- [ ] Write failing `apps/agent-worker/tests/unit/box-process-kill.test.ts`: import `buildMarkedCommand`. Assert `buildMarkedCommand('spoon-turn-abc', ['codex', 'exec', '--json', 'hi'])` returns `['setsid', 'bash', '-lc', expect.stringContaining('# spoon-turn-abc')]` and the script's last line contains `codex exec --json 'hi'`. (Do not unit-test `killBoxProcessesByMarker` end-to-end — it shells out; assert only the argv via a spy if you refactor it to a pure `buildKillScript(marker)` helper. **Recommended:** extract `export const buildKillScript = (marker: string): string` and test it contains `pgrep -f 'spoon-turn-abc'`, `kill -TERM -"$pid"`, and `kill -KILL -"$pid"`.) +- [ ] Run `bun run test:unit` → FAIL. +- [ ] Implement `buildMarkedCommand`, `buildKillScript`, and `killBoxProcessesByMarker` in `docker.ts`. +- [ ] Run `bun run test:unit` → PASS. `bun run typecheck` → PASS. +- [ ] Commit: `feat(worker): in-box process-group marker + kill helpers for agent turns`. + +--- + +## Task 3: CodexAdapter — refactor `runCodexTurn` behind the interface, dispatch codex through the registry + +**Files:** +- Create `apps/agent-worker/src/runtime/codex-adapter.ts` (new). +- Edit `apps/agent-worker/src/worker.ts` (replace `runCodexTurn` call path in `sendWorkspaceMessage`; resolve `workspace.runtime`; route abort). +- Create `apps/agent-worker/tests/unit/codex-adapter.test.ts` (new). + +**Interfaces:** + +*Produces* (`runtime/codex-adapter.ts`): +```ts +export const createCodexAdapter: AdapterFactory = (deps) => { + const execStream = deps?.execStream ?? streamExecInContainer; + return { + name: 'codex', + prepareAuth, // moves prepareCodexAuth (worker.ts:459) + prepareCodexWorkspaceFiles + writeJsonFile here + runTurn, // moves runCodexTurn body (worker.ts:712-853), using buildMarkedCommand + execStream + abort, // killBoxProcessesByMarker({ containerName: ws.boxName, marker: ws.turnMarker }) + }; +}; +``` +`runTurn(ws, prompt, onEvent)`: +1. `ws.runtime = 'codex'`; `ws.turnMarker = 'spoon-turn-' + randomUUID()`; `ws.codexSessionId` continuity preserved. +2. Build the same codex argv as `worker.ts:737-761` (`codex exec [resume ] --json ...codexModelArgs --dangerously-bypass-approvals-and-sandbox --output-last-message [--cd ] `), then `const command = buildMarkedCommand(ws.turnMarker, codexArgv)`. +3. `await execStream({ containerName: ws.boxName, containerCwd: ws.containerRepo, command, environment: { ...providerEnvironment(ws.claim, ws.containerHome), ...secretEnv }, redact: ws.redact, timeoutMs: env.jobTimeoutMs, onStdoutLine: (line) => forEach normalizeCodexJsonLine(line) → onEvent(event), onStderrLine: (line) => onEvent({ kind: 'status', status: line }) if not noise })`. +4. On timeout: register `setTimeout(() => void killBoxProcessesByMarker(...), env.jobTimeoutMs)` and clear it when execStream resolves (execa's own `timeout` only kills the local client — the marker kill is what stops the in-box process). +5. Read `--output-last-message` file (worker.ts:811-843) → `finalMessage`. Capture `error` events into a local var → `TurnResult.error`. Return `{ finalMessage, sessionId: capturedSessionId, error }`. + +*Consumes:* `normalizeCodexJsonLine` (`agent-events.ts`), `providerEnvironment`/`codexModelArgs`/`isCodexLoginProfile` (`runtime/provider.ts`), `buildMarkedCommand`/`killBoxProcessesByMarker`/`streamExecInContainer` (`runtime/docker.ts`), `prepareCodexWorkspaceFiles` (`codex-runtime.ts`), `env`. + +*Worker dispatch change* (`sendWorkspaceMessage`, worker.ts:1624): replace the `isCodexLoginProfile(claim) ? runCodexTurn : env.runtime==='docker' ? runOpenCodeTurn : localRun` branching with: +```ts +const adapter = getAdapter(workspace.runtime); +const onEvent = (event: NormalizedAgentEvent) => handleAgentEvent({ workspace, event, assistantMessageId, assistantContent }); +const result = await adapter.runTurn(workspace, prompt, onEvent); +if (result.sessionId) await persistRuntimeSession(workspace, result.sessionId); // generic setter, replaces setCodexSessionId call site +if (!assistantContent.value.trim() && result.finalMessage) { assistantContent.value = truncate(workspace.redact(result.finalMessage), 40_000); await updateMessage(...); } +if (!assistantContent.value.trim() && result.error) throw new Error(`${workspace.runtime} failed:\n${result.error}`); +``` +Resolve `workspace.runtime` in `runClaim` (worker.ts:1348): for now set `runtime: isCodexLoginProfile(claim) ? 'codex' : 'opencode'` so behavior is byte-identical to today (job.runtime becomes authoritative in Task 8). Route `abortWorkspaceAgent` (worker.ts:1562) through `getAdapter(workspace.runtime).abort(workspace)`. + +**Steps:** +- [ ] Write failing `apps/agent-worker/tests/unit/codex-adapter.test.ts`: build the adapter with a fake `execStream` that calls `onStdoutLine` with fixture lines (reuse the exact JSON shapes from `agent-events.test.ts:110-172`, e.g. `{"type":"item.completed","item":{"id":"item-1","type":"agent_message","text":"done"}}` and `{"type":"turn.completed"}`) then resolves `{ exitCode: 0, output: '' }`. Assert `onEvent` received `{ kind: 'assistant_delta', content: 'done\n\n', externalMessageId: 'item-1' }` and `{ kind: 'assistant_completed' }`, and that `runTurn` returns a `TurnResult`. Add a second case: fake execStream that emits `{"type":"turn.failed","error":{"message":"boom"}}` and returns exitCode 0 → `result.error` contains `boom`. Assert `buildMarkedCommand` was used (spy the fake to capture `args.command[0] === 'setsid'`). +- [ ] Run `bun run test:unit` → FAIL. +- [ ] Create `runtime/codex-adapter.ts`; move Codex logic out of `worker.ts` (`prepareCodexAuth`, `runCodexTurn`, `writeJsonFile`, `readCodexTurnError`). Register it: add `registerAdapter('codex', createCodexAdapter)` in a new `runtime/register.ts` imported once from `worker.ts` top-level. +- [ ] Rewire `sendWorkspaceMessage` and `abortWorkspaceAgent` per above; add `persistRuntimeSession` (writes `setCodexSessionId`/`opencodeSessionId`/`claudeSessionId` based on `workspace.runtime`). Keep the maintenance-decision parse, diff artifact, `recordChangedFiles` tail (worker.ts:1731-1758) unchanged. +- [ ] Run `bun run test:unit` (all existing + new) → PASS. `bun run typecheck` → PASS. +- [ ] Commit: `refactor(worker): move Codex turn logic into CodexAdapter behind AgentRuntime`. + +--- + +## Task 4: OpenCodeAdapter — run `opencode run --format json` inside the box + +**Files:** +- Create `apps/agent-worker/src/runtime/opencode-adapter.ts` (new). +- Edit `apps/agent-worker/src/agent-events.ts` (add `normalizeOpenCodeRunLine` for `opencode run --format json` line output; may delegate to `normalizeOpenCodeEvent`). +- Edit `apps/agent-worker/src/runtime/register.ts` (register `opencode`). +- Create `apps/agent-worker/tests/unit/opencode-adapter.test.ts` (new). +- Edit `apps/agent-worker/tests/unit/agent-events.test.ts` (add `normalizeOpenCodeRunLine` cases). + +**Interfaces:** + +*Produces* (`runtime/opencode-adapter.ts`): +```ts +export const createOpenCodeAdapter: AdapterFactory = (deps) => { + const execStream = deps?.execStream ?? streamExecInContainer; + return { + name: 'opencode', + prepareAuth: async (ws) => { /* opencode reads ANTHROPIC_API_KEY/OPENAI_API_KEY from env; for opencode_auth_json profiles, write ~/.local/share/opencode/auth.json 0600 (reuse logic from worker.ts:472-480) */ }, + runTurn, + abort: (ws) => killBoxProcessesByMarker({ containerName: ws.boxName, marker: ws.turnMarker ?? '' }), + }; +}; +``` +`runTurn(ws, prompt, onEvent)`: +1. `ws.runtime = 'opencode'`; `ws.turnMarker = 'spoon-turn-' + randomUUID()`. +2. argv: `['opencode', 'run', '--format', 'json', '--model', opencodeModel(ws.claim), ...(ws.opencodeSessionId ? ['--session', ws.opencodeSessionId] : []), '--', prompt]`. **Verify the continuity flag** against `opencode run --help` during the smoke step (Task 11); if `--session` is unsupported, drop it and note that OpenCode turns are stateless (acceptable — Codex/Claude carry continuity). +3. `command = buildMarkedCommand(ws.turnMarker, argv)`; `execStream({ containerName: ws.boxName, containerCwd: ws.containerRepo, command, environment: { ...providerEnvironment(ws.claim), ...secretEnv }, redact, timeoutMs, onStdoutLine: (line) => normalizeOpenCodeRunLine(line).forEach(onEvent), onStderrLine })`. +4. Return `{ finalMessage, sessionId: capturedSessionId, error }` (session id parsed from the run output if present). + +*Produces* (`agent-events.ts`): `export const normalizeOpenCodeRunLine = (line: string): NormalizedAgentEvent[]` — `JSON.parse` each line; if it matches the SSE-style `{ type, properties }` shape, delegate to `normalizeOpenCodeEvent(parsed)`; otherwise map `opencode run`'s result shape (capture `sessionID`, final text). Unparseable lines → `[{ kind: 'status', status: line }]`. + +*Consumes:* `opencodeModel`/`providerEnvironment` (`runtime/provider.ts`), `buildMarkedCommand`/`killBoxProcessesByMarker`/`streamExecInContainer` (`runtime/docker.ts`). + +**Steps:** +- [ ] Write failing `agent-events.test.ts` case: `normalizeOpenCodeRunLine(JSON.stringify({ type: 'message.part.delta', properties: { part: { text: 'hi' }, messageID: 'm1' } }))` → `toContainEqual({ kind: 'assistant_delta', content: 'hi', externalMessageId: 'm1' })`; and a non-JSON line → `{ kind: 'status', status: '' }`. +- [ ] Write failing `opencode-adapter.test.ts`: fake `execStream` emits the delta line + a final line, returns `{ exitCode: 0, output: '' }`; assert `onEvent` got the delta, `runTurn` resolves, and the captured `args.command[0] === 'setsid'` and `args.command[3]` (script) contains `opencode run --format json`. +- [ ] Run `bun run test:unit` → FAIL. +- [ ] Implement `normalizeOpenCodeRunLine`, `createOpenCodeAdapter`, and register it. +- [ ] Run `bun run test:unit` → PASS. `bun run typecheck` → PASS. +- [ ] Commit: `feat(worker): OpenCodeAdapter runs opencode in the per-user box (no side container)`. + +--- + +## Task 5: Delete the per-job OpenCode container path and dead docker helpers + +**Files:** +- Edit `apps/agent-worker/src/worker.ts` (remove `ensureOpenCodeSession`, `runOpenCodeTurn`, `workspaceContainerName`, `startWorkspaceContainer` import + usage, `stopWorkspaceContainer` calls, `containerName`/`containerId`/`opencodePassword`/`opencodeSession` workspace fields, the OpenCode-server branches of `abortWorkspaceAgent`/`replyToInteraction`/`getWorkspaceAgentStatus`; retarget health/cleanup to `spoon-box-`). +- Delete `apps/agent-worker/src/opencode-session.ts`. +- Edit `apps/agent-worker/src/runtime/docker.ts` (delete `runInJobContainer`, `streamInJobContainer`, `execInWorkspaceContainer`, `inspectWorkspaceContainer`, `startWorkspaceContainer`, `getPublishedPort`; keep `ensureUserContainer`, `streamExecInContainer`, `runExecInContainer`, `stopWorkspaceContainer`, `listWorkspaceContainerNames`, `jobWorkspaceVolumeSpec`, `normalizeRunResult`, `ensureJobImagePulled`). +- Edit `apps/agent-worker/src/env.ts` (remove `containerAccess`; remove now-dead `terminalImage`/`terminalIdleMs`/`maxConcurrentJobs` only if not read anywhere else — grep first). +- Edit `packages/backend/convex/*` — none here. +- Edit `apps/agent-worker/tests/unit/*` as needed (no test imports the deleted symbols; if any do, update). + +**Interfaces:** *Consumes:* now only box-scoped docker helpers. `getWorkerHealth` (worker.ts:1880) enumerates `listWorkspaceContainerNames('spoon-box-')`; `cleanupOrphanedWorkspaces` (worker.ts:1911) enumerates `spoon-box-` for the container list (do **not** touch the `homes/` workdir removal here — that is Phase 1's layout-aware rewrite; leave whatever Phase 1 produced). + +**Steps:** +- [ ] `grep -rn "spoon-agent-job\|startWorkspaceContainer\|ensureOpenCodeSession\|opencode-session\|containerAccess\|host_port\|runInJobContainer\|streamInJobContainer\|execInWorkspaceContainer\|inspectWorkspaceContainer" apps/agent-worker/src` → build the deletion checklist from the hits. +- [ ] `grep -rn "env.terminalImage\|env.terminalIdleMs\|env.maxConcurrentJobs" apps/agent-worker/src` → only remove the env var if zero hits remain after Phase 1 (`maxConcurrentJobs` is surfaced in `getWorkerHealth` at worker.ts:1903 — drop that line too, or keep the field; note it in the commit). +- [ ] Remove the symbols above. In `abortWorkspaceAgent`/`replyToInteraction`/`getWorkspaceAgentStatus`, drop the `opencodeSession` branches: `abortWorkspaceAgent` → `await getAdapter(workspace.runtime).abort(workspace)`; `replyToInteraction` → `throw new Error('Interactive replies are not supported by exec-based runtimes.')` (no runtime generates interaction requests after this refactor); `getWorkspaceAgentStatus` → return `{ runtimeMode: workspace.runtime, codexSessionId, opencodeSessionId, claudeSessionId, active }`. +- [ ] In `openWorkspacePullRequest` (worker.ts:1852-1855) and `stopWorkspace` (worker.ts:1869-1872), delete the `workspace.opencodeSession?.close()` and `stopWorkspaceContainer(workspace.containerName)` lines (box lifecycle is now purely `releaseUserBox`). +- [ ] Delete `apps/agent-worker/src/opencode-session.ts` and its imports (worker.ts:31-35). +- [ ] Run `cd apps/agent-worker && bun run typecheck` → PASS (fix any dangling refs). Run `bun run test:unit` → PASS. +- [ ] Commit: `refactor(worker): delete per-job OpenCode container path, host_port split, and dead docker helpers`. + +--- + +## Task 6: ClaudeCodeAdapter — `claude -p --output-format stream-json` in the box + +**Files:** +- Create `apps/agent-worker/src/runtime/claude-adapter.ts` (new). +- Edit `apps/agent-worker/src/agent-events.ts` (add `normalizeClaudeJsonLine`). +- Edit `apps/agent-worker/src/runtime/register.ts` (register `claude`). +- Create `apps/agent-worker/tests/unit/claude-adapter.test.ts` (new). +- Edit `apps/agent-worker/tests/unit/agent-events.test.ts` (add Claude normalizer cases). + +**Interfaces:** + +*Produces* (`agent-events.ts`): `export const normalizeClaudeJsonLine = (line: string): NormalizedAgentEvent[]`. Claude Code `stream-json` emits one JSON object per line: +- `{"type":"system","subtype":"init","session_id":"abc",...}` → `{ kind: 'session', sessionId: 'abc' }`. +- `{"type":"assistant","message":{"content":[{"type":"text","text":"..."}]}}` → `{ kind: 'assistant_delta', content: '...' }` (concatenate text blocks); `{"type":"content_block_delta",...}` if present → `assistant_delta`. +- `{"type":"assistant","message":{"content":[{"type":"tool_use","name":"Bash","input":{...}}]}}` → `{ kind: 'tool_started', name, input: JSON.stringify(input) }`. +- `{"type":"user","message":{"content":[{"type":"tool_result",...}]}}` → `{ kind: 'tool_completed', name: 'tool', output }`. +- `{"type":"result","subtype":"success","result":"final text","session_id":"abc"}` → `{ kind: 'assistant_completed', content: 'final text' }` (+ a `session` event if the id is new). +- `{"type":"result","subtype":"error_max_turns"|"error_during_execution",...}` → `{ kind: 'error', message }`. +- Unparseable → `[{ kind: 'status', status: line }]`. + +*Produces* (`runtime/claude-adapter.ts`): +```ts +export const createClaudeAdapter: AdapterFactory = (deps) => { + const execStream = deps?.execStream ?? streamExecInContainer; + return { + name: 'claude', + prepareAuth, // API-key kind: nothing on disk (ANTHROPIC_API_KEY comes from providerEnvironment). OAuth-json kind: write /.claude/.credentials.json 0600 from claim.aiProviderProfile.secret (reuse writeJsonFile). + runTurn, + abort: (ws) => killBoxProcessesByMarker({ containerName: ws.boxName, marker: ws.turnMarker ?? '' }), + }; +}; +``` +`runTurn(ws, prompt, onEvent)`: +1. `ws.runtime = 'claude'`; `ws.turnMarker = 'spoon-turn-' + randomUUID()`. +2. argv: `['claude', '-p', prompt, '--output-format', 'stream-json', '--verbose', '--dangerously-skip-permissions', '--model', claudeModel(ws.claim), ...(ws.claudeSessionId ? ['--resume', ws.claudeSessionId] : [])]` (cwd = `containerRepo`). `--verbose` is required for `stream-json`. **Confirm exact flags** in the smoke step against the pinned CLI (`claude --help`). +3. `command = buildMarkedCommand(ws.turnMarker, argv)`; `execStream({ containerName: ws.boxName, containerCwd: ws.containerRepo, command, environment: { ...claudeEnv(ws.claim, ws.containerHome), ...secretEnv }, redact, timeoutMs, onStdoutLine: (line) => normalizeClaudeJsonLine(line).forEach(onEvent), onStderrLine })`. `claudeEnv`: for API-key kind `{ ANTHROPIC_API_KEY: secret, HOME: ws.containerHome }`; for OAuth-json kind `{ HOME: ws.containerHome }` (CLI reads `~/.claude/.credentials.json`). +4. Return `{ finalMessage, sessionId, error }` (`finalMessage` from the `result` event; `sessionId` from `system:init`/`result`). + +*Consumes:* `normalizeClaudeJsonLine` (`agent-events.ts`), `providerEnvironment` (`runtime/provider.ts`), box helpers (`runtime/docker.ts`), `writeJsonFile` (move to `runtime/provider.ts` or a small `runtime/auth.ts` shared by Codex + Claude adapters). + +**Steps:** +- [ ] Write failing `agent-events.test.ts` Claude cases: `system:init` → session; `assistant` text block → `assistant_delta`; `result:success` → `assistant_completed` with content; `result:error_max_turns` → `error`. +- [ ] Write failing `claude-adapter.test.ts`: fake `execStream` emits `system:init` + `assistant` text + `result:success` lines, returns `{ exitCode: 0, output: '' }`; assert `onEvent` received the mapped events, `runTurn` returns `{ finalMessage: 'final text', sessionId: 'abc' }`, and `args.command[0] === 'setsid'` with the script containing `claude -p`. +- [ ] Run `bun run test:unit` → FAIL. +- [ ] Implement `normalizeClaudeJsonLine`, `createClaudeAdapter`, register it. Move `writeJsonFile` to a shared `runtime/auth.ts`. +- [ ] Run `bun run test:unit` → PASS. `bun run typecheck` → PASS. +- [ ] Commit: `feat(worker): ClaudeCodeAdapter runs claude -p stream-json in the per-user box`. + +--- + +## Task 7: Backend schema — add `claude` runtime, Anthropic OAuth auth kind, and profile→runtimes derivation + +**Files:** +- Edit `packages/backend/convex/schema.ts` (runtime unions + authType union). +- Create `packages/backend/convex/runtimeSupport.ts` (new — pure derivation helper, no `'use node'`). +- Edit `packages/backend/convex/agentJobs.ts` and `packages/backend/convex/spoonAgentSettings.ts` and `packages/backend/convex/aiProviderProfiles.ts`/`aiProviderProfilesNode.ts` (union constants). +- Create `packages/backend/tests/unit/runtime-support.test.ts` (new). + +**Interfaces:** + +*Schema changes:* +- `agentJobs.runtime` (schema.ts:536): `v.optional(v.union(v.literal('openai_direct'), v.literal('opencode'), v.literal('codex'), v.literal('claude')))` — keep `openai_direct` for legacy reads (deprecated, never written). +- `spoonAgentSettings.runtime` (schema.ts:472): same widened union. +- `aiProviderProfiles.authType` (schema.ts:389): add `v.literal('anthropic_oauth_json')`. + +*Produces* (`runtimeSupport.ts`): +```ts +import type { Doc } from './_generated/dataModel'; +export type AgentRuntimeName = 'codex' | 'opencode' | 'claude'; + +// Which runtimes a provider profile can drive. +export const runtimesForProfile = ( + profile: Pick, 'provider' | 'authType'>, +): AgentRuntimeName[] => { + if (profile.provider === 'opencode_openai_login' || profile.authType === 'opencode_auth_json') { + return ['codex']; // ChatGPT-login snapshot → Codex CLI only + } + if (profile.provider === 'anthropic') { + return profile.authType === 'anthropic_oauth_json' ? ['claude'] : ['claude', 'opencode']; + } + if (profile.provider === 'openai') return ['opencode', 'codex']; + // Every remaining API-key provider is OpenAI-compatible → OpenCode. + return ['opencode']; +}; +``` + +**Steps:** +- [ ] Write failing `packages/backend/tests/unit/runtime-support.test.ts`: `runtimesForProfile({ provider: 'anthropic', authType: 'api_key' })` → `['claude','opencode']`; `{ provider: 'opencode_openai_login', authType: 'opencode_auth_json' }` → `['codex']`; `{ provider: 'openrouter', authType: 'api_key' }` → `['opencode']`; `{ provider: 'openai', authType: 'api_key' }` → `['opencode','codex']`. +- [ ] Run `cd packages/backend && bun run test:unit` → FAIL. +- [ ] Edit `schema.ts` unions; create `runtimeSupport.ts`; widen the `authType` union constant in `aiProviderProfiles.ts:24`, `aiProviderProfilesNode.ts:24`. +- [ ] Run `cd packages/backend && bun run codegen` then `bun run typecheck` → PASS. `bun run test:unit` → PASS. +- [ ] Commit: `feat(backend): add claude runtime, anthropic OAuth auth kind, and profile→runtime derivation`. + +--- + +## Task 8: Backend queue-time runtime validation + claim payload carries runtime + +**Files:** +- Edit `packages/backend/convex/agentJobs.ts` (`insertJob` at line 379, the three `runtime` union constants at line 21, `createFromRequest`/`createForThread`/`createForThreadInternal` args, `claimNextInternal` return). +- Edit `packages/backend/convex/agentJobsNode.ts` (map `job.runtime` into the `WorkerClaim`). +- Edit `packages/backend/convex/threads.ts` (`createUserThread` passes `runtime`). +- Edit `packages/backend/convex/aiProviderProfiles.ts` (`publicProfile` exposes `supportedRuntimes`). +- Edit `apps/agent-worker/src/runtime/provider.ts` (`Claim.job.runtime` already widened in Task 1 — confirm) and `worker.ts` `runClaim` (use `claim.job.runtime` as authoritative). +- Edit `packages/backend/tests/unit/harness.test.ts` or add `packages/backend/tests/unit/runtime-validation.test.ts` (new). + +**Interfaces:** + +- `const runtime = v.union(v.literal('codex'), v.literal('opencode'), v.literal('claude'))` (agentJobs.ts:21). The three create mutations gain `runtime: v.optional(runtime)`. +- `insertJob` resolves and validates: +```ts +const profile = await getJobProfile(ctx, ownerId, aiProviderProfileId); +const supported = runtimesForProfile(profile); +const resolvedRuntime = + requestedRuntime ?? (settings.runtime as AgentRuntimeName | undefined) ?? supported[0]; +if (!supported.includes(resolvedRuntime)) { + throw new ConvexError( + `Provider "${profile.name}" cannot run the "${resolvedRuntime}" runtime. Supported: ${supported.join(', ')}.`, + ); +} +// insert agentJobs with runtime: resolvedRuntime +``` +- `claimNextInternal` already returns the full `job` doc (schema.ts row includes `runtime`), so no change there; `agentJobsNode.claimNextForWorker` must forward `job.runtime` — it already returns `claimed.job` verbatim in `WorkerClaim.job`, so `job.runtime` flows through. Confirm the worker's `Claim.job.runtime` type (widened in Task 1) matches. +- `publicProfile` (aiProviderProfiles.ts:45): add `supportedRuntimes: runtimesForProfile(profile)` so the UI can filter. + +*Worker change:* `runClaim` (worker.ts:1301) currently throws for non-`opencode`. Replace with: resolve `const runtime = (claim.job.runtime ?? 'opencode') as AgentRuntimeName;` set `workspace.runtime = runtime`; delete the legacy `openai_direct` throw (queue-time guard now prevents bad runtimes). `prepareAuth` for the resolved adapter runs in place of the `isCodexLoginProfile` special-case (worker.ts:1375): `await getAdapter(runtime).prepareAuth(workspace)`. + +**Steps:** +- [ ] Write failing `packages/backend/tests/unit/runtime-validation.test.ts` (convex-test style from harness.test.ts): create a user + GitHub spoon + an `anthropic`/`api_key` profile set default; call `createForThread`/`createUserThread` equivalent with `runtime: 'codex'` → expect throw `/cannot run the "codex" runtime/`; with `runtime: 'claude'` → resolves; with no runtime and an `opencode_openai_login` profile → job.runtime === 'codex'. +- [ ] Run `cd packages/backend && bun run test:unit` → FAIL. +- [ ] Implement the union widening, `insertJob` validation, thread/request arg plumbing, and `publicProfile.supportedRuntimes`. +- [ ] Update `worker.ts` `runClaim` to set `workspace.runtime` from `claim.job.runtime` and call `getAdapter(runtime).prepareAuth(workspace)`; delete the legacy throw. +- [ ] Run `cd packages/backend && bun run codegen && bun run typecheck && bun run test:unit` → PASS. Run `cd apps/agent-worker && bun run typecheck && bun run test:unit` → PASS. +- [ ] Commit: `feat(backend): queue-time runtime/profile validation; job.runtime authoritative in worker`. + +--- + +## Task 9: Migrate/deprecate `openai_direct` + +**Files:** +- Create `packages/backend/convex/migrations.ts` (new internalMutation) OR add to an existing maintenance module — grep for an existing pattern first (`grep -rn "internalMutation" packages/backend/convex | grep -i migrat`; there is none, so create `migrations.ts`). +- Edit `packages/backend/tests/unit/` (new test or extend). + +**Interfaces:** +```ts +export const migrateOpenAiDirectRuntime = internalMutation({ + args: {}, + handler: async (ctx) => { + let patched = 0; + for (const job of await ctx.db.query('agentJobs').collect()) { + if (job.runtime === 'openai_direct') { await ctx.db.patch(job._id, { runtime: 'opencode', updatedAt: Date.now() }); patched++; } + } + for (const s of await ctx.db.query('spoonAgentSettings').collect()) { + if (s.runtime === 'openai_direct') { await ctx.db.patch(s._id, { runtime: 'opencode', updatedAt: Date.now() }); patched++; } + } + return { patched }; + }, +}); +``` +Run once post-deploy via `npx convex run migrations:migrateOpenAiDirectRuntime` (document in the smoke/deploy notes). `openai_direct` stays in the schema unions as a legacy-read literal; nothing writes it. + +**Steps:** +- [ ] Write failing test: insert a `spoonAgentSettings` row with `runtime: 'openai_direct'` (via `t.run`/raw ctx), run the migration, assert it becomes `'opencode'` and `patched >= 1`. +- [ ] Run `cd packages/backend && bun run test:unit` → FAIL. +- [ ] Implement `migrations.ts`. +- [ ] Run `cd packages/backend && bun run codegen && bun run typecheck && bun run test:unit` → PASS. +- [ ] Commit: `feat(backend): migration to retire openai_direct runtime`. + +--- + +## Task 10: Next UI — runtime picker (thread create + spoon agent settings) and Anthropic OAuth profile kind + +**Files:** +- Edit `apps/next/src/components/threads/thread-workspace-form.tsx` (replace the disabled `Input value='OpenCode workspace'` at line 142-144 with a runtime `Select`; pass `runtime` to `createUserThread`). +- Edit `apps/next/src/components/spoons/spoon-agent-settings-form.tsx` (replace the disabled Runtime input at line 187-190 with a `Select`; pass `runtime` to `spoonAgentSettings.update`). +- Edit `apps/next/src/components/integrations/ai-provider-profiles-panel.tsx` (add an Anthropic OAuth JSON option to `providerOptions` / auth-type handling at line 71-97). +- Edit `packages/backend/convex/threads.ts` `createUserThread` args + `spoonAgentSettings.ts` `update` args + `agentJobs.ts` create mutations to accept `runtime` (done in Task 8 for agentJobs; add to `createUserThread`/`update`). + +**Interfaces:** +- `createUserThread` gains `runtime: v.optional(v.union(v.literal('codex'), v.literal('opencode'), v.literal('claude')))`, forwarded to `createForThreadInternal` → `insertJob`. +- `spoonAgentSettings.update` `runtime` arg union widens to `codex|opencode|claude` and `patch.runtime = args.runtime` (drop the `= 'opencode'` hardcode at spoonAgentSettings.ts:126). +- UI: both forms read `selectedProfile.supportedRuntimes` (new field from Task 8) and render only those `SelectItem`s; default the value to the settings runtime or `supportedRuntimes[0]`; disable submit if the chosen runtime ∉ supportedRuntimes. +- Profiles panel: add `{ value: 'anthropic', label: 'Anthropic OAuth (paste credentials)', authType: 'anthropic_oauth_json' }` alongside the existing `anthropic` API-key option; when `anthropic_oauth_json`, render a `Textarea` for the pasted `~/.claude/.credentials.json` (same UX as the Codex `opencode_auth_json` textarea). + +**Steps:** +- [ ] Add `runtime` to `createUserThread` and `spoonAgentSettings.update` args + handlers; `cd packages/backend && bun run codegen && bun run typecheck`. +- [ ] Edit the three Next components: runtime `Select`s wired to `supportedRuntimes`; Anthropic OAuth option in the profiles panel. +- [ ] Typecheck Next: `cd apps/next && bun run typecheck` (or the repo's Next typecheck script — confirm via `apps/next/package.json`). If component tests exist for these forms, extend them; otherwise cover behavior in the Task 11 manual checklist (note: no jsdom test is required by the spec for the picker, but add one if a sibling form test already exists). +- [ ] Commit: `feat(web): per-thread/per-spoon runtime picker and Anthropic OAuth provider profile`. + +--- + +## Task 11: Add Claude Code CLI to the job image + extend the smoke test + +**Files:** +- Edit `docker/agent-job.Dockerfile` (line 50 npm install line). +- Edit `scripts/smoke-agent-container` (version checks + one turn per runtime). +- Edit `README.md` (the "Production agent runtime images" `
` at line 189-260: mention Claude Code CLI + the `anthropic_oauth_json` credential snapshot pattern, mirroring the Codex `auth.json` paragraph at line 255-258). + +**Interfaces / concrete commands:** +- Dockerfile line 50 becomes: +```dockerfile +RUN npm install -g pnpm yarn bun@1.3.10 opencode-ai@1.17.9 @openai/codex@0.142.0 @anthropic-ai/claude-code@ \ + && npm cache clean --force +``` +Pin `` to the latest published `@anthropic-ai/claude-code` version at implementation time (`npm view @anthropic-ai/claude-code version`); record the exact version in the commit message. +- `scripts/smoke-agent-container`: add to the in-container check block (after `codex --version` at line 30): `claude --version`. Then add a documented manual per-runtime turn section (commented, runnable by hand) — build the image, start a throwaway box, exec one turn per runtime: +```sh +# Manual per-runtime smoke (requires creds in your shell env): +# docker build -f docker/agent-job.Dockerfile -t spoon-agent-job:latest . +# docker run -d --name spoon-box-smoke --init spoon-agent-job:latest sleep infinity +# docker exec -e OPENAI_API_KEY -w /workspace spoon-box-smoke \ +# opencode run --format json --model openai/gpt-5.5 -- 'print hello' +# docker exec -e ANTHROPIC_API_KEY -w /workspace spoon-box-smoke \ +# claude -p 'print hello' --output-format stream-json --verbose --dangerously-skip-permissions +# docker exec -e CODEX_HOME=/root/.codex -w /workspace spoon-box-smoke \ +# codex exec --json --dangerously-bypass-approvals-and-sandbox 'print hello' +# docker rm -f spoon-box-smoke +``` +Use these commands to also **confirm the real flag names** assumed in Tasks 4/6 (`opencode run --help`, `claude --help`) and fix the adapters if they differ. + +**Steps:** +- [ ] `npm view @anthropic-ai/claude-code version` → choose the pin. Edit the Dockerfile. +- [ ] Build the image and run `SPOON_AGENT_JOB_IMAGE=spoon-agent-job:latest ./scripts/smoke-agent-container` → all `--version` checks pass (`claude --version` included). +- [ ] Run the manual per-runtime turns; reconcile adapter flags with reality (patch Task 4/6 code if needed, keeping unit tests green). +- [ ] Update `README.md`. +- [ ] Commit: `build(agent-job): pin Claude Code CLI; smoke one turn per runtime`. + +--- + +## Task 12: Secrets hygiene — 0600 env files, deleted on stop, re-materialized per run + +**Files:** +- Edit `apps/agent-worker/src/worker.ts` (`materializeEnvFile` at line 1167; `stopWorkspace`/`openWorkspacePullRequest` teardown; call `materializeEnvFile` per run). +- Create `apps/agent-worker/tests/unit/materialize-env.test.ts` (new). + +**Interfaces:** +- `materializeEnvFile` (worker.ts:1167): write with `{ mode: 0o600 }` (currently no mode). Extract the file path resolution + write into a pure-ish helper `writeMaterializedEnv(repoDir, envFilePath, secrets): Promise` returning the absolute path, so it is unit-testable with `mkdtemp`. +- Track the materialized path on the workspace (`workspace.materializedEnvPath?: string`). +- On `stopWorkspace` (worker.ts:1866) and `openWorkspacePullRequest` (worker.ts:1851) and the `runClaim` catch (worker.ts:1410) teardown: if `workspace.materializedEnvPath` exists, `await rm(workspace.materializedEnvPath, { force: true })`. +- Re-materialize per run: call `materializeEnvFile(workspace)` at the top of `sendWorkspaceMessage` (worker.ts:1624) when `claim.job.materializeEnvFile` is set, before the adapter turn — so the env file is fresh each turn and can be deleted between turns. Keep the existing `ensureNoEnvFilesStaged` staging guard (worker.ts:1269) unchanged. + +**Steps:** +- [ ] Write failing `materialize-env.test.ts`: `mkdtemp` a repo dir, call `writeMaterializedEnv(dir, '.env.local', [{ name: 'A', value: 'x' }])`, assert the file exists, `(await stat(path)).mode & 0o777 === 0o600`, and content is `A="x"\n` (matches worker.ts:1172-1174 formatting). Mirror the `codex-runtime.test.ts` `mode()` helper + `afterEach` cleanup. +- [ ] Run `cd apps/agent-worker && bun run test:unit` → FAIL. +- [ ] Implement `writeMaterializedEnv` (0600), wire deletion on stop/PR/failure, re-materialize per run. +- [ ] Run `bun run test:unit` → PASS. `bun run typecheck` → PASS. +- [ ] Commit: `fix(worker): materialize env files 0600, re-materialize per run, delete on stop`. + +--- + +## Self-review against the Phase 2 spec + +- **Adapter interface, all exec into the box, process-group kill** → Tasks 1–2 (`buildMarkedCommand` = `setsid` + marker; `killBoxProcessesByMarker` = in-box `pgrep`+`kill -TERM/-KILL` of the group; never the local execa client). ✅ +- **Codex refactored behind the interface** → Task 3. ✅ +- **OpenCode inside the box, per-job path deleted** → Tasks 4–5 (`opencode run --format json` in the box; `startWorkspaceContainer`/`spoon-agent-job-*`/`containerAccess host_port|network`/dead helpers all removed). ✅ +- **Claude Code adapter + new provider kind + pinned CLI + prepareAuth** → Tasks 6, 7, 10, 11 (`anthropic_oauth_json` snapshot mirrors Codex `auth.json`; API-key path uses `ANTHROPIC_API_KEY`). ✅ +- **Runtime picker + profiles declare supported runtimes + queue-time validation replacing run-time `openai_direct` discovery + migrate/deprecate** → Tasks 7–10. ✅ +- **Secrets hygiene (0600, delete on stop, re-materialize; staging guard stays)** → Task 12. ✅ +- **Testing approach** → adapter-level fixture stream tests (injectable `execStream`), normalizer fixtures matching the existing style, convex-test backend tests, extended manual smoke. ✅ diff --git a/docs/superpowers/plans/2026-07-10-spoon-phase3-dev-box-surface.md b/docs/superpowers/plans/2026-07-10-spoon-phase3-dev-box-surface.md new file mode 100644 index 0000000..2365696 --- /dev/null +++ b/docs/superpowers/plans/2026-07-10-spoon-phase3-dev-box-surface.md @@ -0,0 +1,409 @@ +# Spoon Phase 3 — Dev Box as a First-Class Surface: Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Give every user a standalone "My Machine" page at `/machine` that exposes their persistent per-user Fedora box directly: live status + start/stop/restart controls, a full-page terminal rooted at `~`, and a home-scoped file browser (reuse `FileTree`/`CodeEditor`). Make the workspace terminal survive tab switches, and add terminal QoL (web-links, copy-on-select). The box is USER-scoped (there is no `jobId`): the browser reaches it only through Next routes that mint short-lived HMAC tokens **after** Convex auth resolves the caller's own box username server-side — the browser never sees a worker secret and can only ever reach its OWN box. + +**Architecture:** Same ownership-proxy pattern already used for `/jobs/:id/*`, replicated for a user-scoped `/box/*` surface: +- **Worker (Bun/dockerode/ws):** new user-scoped HMAC terminal token (`terminal-token.ts`), a box helper module (`box.ts`) for home-path derivation + home-scoped file access + status, HTTP routes `GET /box/status`, `POST /box/lifecycle`, `GET /box/tree`, `GET|PUT /box/file`, and a WS route `GET /box/terminal`. All authed by the internal bearer token (HTTP) or the signed user token (WS), which embeds the username. +- **Next (App Router):** `agent-worker-proxy.ts` gains `mintBoxTerminalToken(username)`, `resolveBoxUsername()`, and `proxyBox(...)`; `/api/box/*` route handlers derive the caller's username from Convex auth and proxy to the worker; a client `/machine` page reuses an extracted `XtermSession` component plus `FileTree` + `CodeEditor`. + +**Tech Stack:** Next.js 16 (React 19), Convex, agent-worker (Bun, dockerode, ws, xterm). + +**Depends on:** Phase 1 (terminal sizing fix + box handle/mutex lifecycle). Phase 3 assumes the Phase-1 `acquireUserBox` handle API (see "Phase-1 assumptions" below) and the Phase-1 realpath-based path containment. It benefits from Phase 2 (one-box-three-agents) but does **not** require it: the `/box` surface only needs the persistent box to exist, not any particular agent runtime. + +## Phase-1 assumptions (read before starting) + +Phase 1 reworks `apps/agent-worker/src/user-container.ts` to a per-username mutex + handle API. This plan is written against: + +```ts +// apps/agent-worker/src/user-container.ts (Phase-1 shape) +export type BoxHandle = { boxName: string; release: () => void }; // release is idempotent +export const acquireUserBox: (args: { + username: string; workdir: string; containerHome: string; +}) => Promise; +``` + +**If Phase 1 kept the pre-Phase-1 signature** (`acquireUserBox(...) => Promise` + `releaseUserBox(username)`, as in commit `b092955`), adapt every `handle.release()` in this plan to `releaseUserBox(username)` and treat the returned string as `boxName`. Check the real signature in `user-container.ts` before writing Task 5, and follow it. + +## Global Constraints + +- **Next tests:** jsdom + @testing-library/react. Component tests live in `apps/next/tests/component/*.test.tsx`, unit tests in `apps/next/tests/unit/*.test.ts`. Run `cd apps/next && bun run test:component` and `bun run test:unit`. Mock `convex/react`, `next/navigation`, and `sonner` exactly as `apps/next/tests/component/render.test.tsx:10-38` does. Monaco (`CodeEditor`) and xterm do not render under jsdom — mock those child components in page tests (see Task 9). +- **Worker tests:** `apps/agent-worker/tests/unit/*.test.ts` (vitest). Run `cd apps/agent-worker && bun run test:unit`. Match `apps/agent-worker/tests/unit/terminal-token.test.ts` for token tests. Docker-dependent code (status/lifecycle/exec) is **not** unit-testable without a daemon — document manual verification instead; unit-test only the pure logic (path containment, token parsing, tree building against a real tmpdir). +- **Conventional commits, one per task** (e.g. `feat(worker): user-scoped box terminal token`). Commit only after the task's tests pass. +- **Browser NEVER receives a worker token.** Tokens are minted server-side in Next route handlers after Convex auth. The username is derived server-side from the authed user (`api.userEnvironment.getMine`), never taken from the request body/query — so a user can only ever reach their own box. +- **Reuse, don't duplicate.** Extract the xterm wiring from `workspace-terminal.tsx` into a shared `XtermSession` (Task 8) before building the box terminal; reuse `FileTree` and `CodeEditor` verbatim on `/machine`. Reuse `safeHomeJoin`-style realpath containment for `/box/file`. + +## Reference facts (verified against the codebase) + +- **Job terminal token** (`apps/agent-worker/src/terminal-token.ts`): format `${expiresAtMs}.${jobId}.${hmacHex}`, HMAC over `${expiresAtMs}.${jobId}`. Minted in `apps/next/src/lib/agent-worker-proxy.ts:20` (`mintTerminalToken`). Secret precedence (both sides): `SPOON_AGENT_TERMINAL_SECRET` → `SPOON_AGENT_WORKER_INTERNAL_TOKEN` → `SPOON_WORKER_TOKEN`. Worker reads it as `env.terminalSecret` (`apps/agent-worker/src/env.ts:43-47`); Next builds it in `terminalSecret()` (`agent-worker-proxy.ts:12-15`). +- **Worker HTTP auth**: `requireAuth` (`apps/agent-worker/src/server.ts:45-51`) compares the `Authorization: Bearer` header to `env.internalToken`. Routes are dispatched in `startWorkerServer` (`server.ts:59-186`); only `/health`, `/cleanup`, and `/jobs/:id/*` exist today. The `jobRoute` regex (`server.ts:53-57`) is the pattern to mirror for `/box/*`. +- **Worker WS**: `attachTerminalServer` (`terminal.ts:157-181`) handles `server.on('upgrade')`, matches `/^\/jobs\/([^/]+)\/terminal$/`, verifies the token, then calls `bridge(ws, jobId)`. Only enabled when `env.runtime === 'docker'`. +- **`bridge()`** (`terminal.ts:21-150`): resolves the workspace via `getTerminalWorkspace(jobId)`, acquires the box via `acquireUserBox`, then spawns ` exec -i -e TERM=... -e HOME=... -w /bin/bash -lc 'exec script -qfc /dev/null'`. The launcher does `stty rows.. cols..` then `exec tmux new-session -A -s spoon` (or `bash -il`). Input/resize buffered until the exec is ready; `cleanup()` kills the proc and releases the box. +- **Box home layout** (`apps/agent-worker/src/worker.ts:1315-1334`): `username = userEnv?.username ?? 'user'`; `homeDir = path.resolve(env.workdir, 'homes', username)`; `containerHome = path.posix.join('/home', username)`. The box is acquired with `{ username, workdir: homeDir, containerHome }`. +- **Box container** (`apps/agent-worker/src/runtime/docker.ts`): `userContainerName(username)` = `spoon-box-` (`docker.ts:338-339`); `ensureUserContainer({username,workdir,containerHome})` starts `spoon-box-*` with `--memory 4g --cpus 2` mounting the home (`docker.ts:341-383`); `stopWorkspaceContainer(name)` = `rm -f` (`docker.ts:438-442`); `inspectWorkspaceContainer(name)` runs `inspect` (`docker.ts:444-453`). Container runtime binary = `containerRuntime()`. +- **Username resolution (server)**: `api.userEnvironment.getMine` (`packages/backend/convex/userEnvironment.ts:15-34`) returns `{ enabled, username, firstName, dotfilesRepoUrl, dotfilesRepoRef, setupCommand }` where `username = settings?.homeUsername ?? deriveHomeUsername(user?.name)` — the exact value the worker uses. This is the authoritative source for the caller's box username on the Next side. +- **Existing file/tree worker fns** (`worker.ts`): `listWorkspaceTree` (1440), `readWorkspaceFile` (1477), `writeWorkspaceFile` (1483) — model the box variants on these but root at `homeDir` and use realpath containment. +- **Reusable UI**: `FileTree` (`apps/next/src/components/agent-workspace/file-tree.tsx`) props `{ tree: FileTreeNode|null; selectedPath?; expandedPaths: string[]; onSelect; onToggleDirectory }`. `CodeEditor` (`code-editor.tsx:33-51`) props `{ path?; content; savedContent; readOnly; vimEnabled; onSave; onChange; onVimEnabledChange }`. `FileTreeNode`, `FileResponse`, `DiffResponse` live in `agent-workspace/types.ts`. Nav items are declared in `apps/next/src/components/layout/header/index.tsx:23-45` (`NavItem` = `{ href; icon; label; external? }`). + +--- + +## Task 1: Worker — user-scoped box terminal token (mint helper is Next-side; verify is worker-side) + +Add a user-scoped token alongside the existing job-scoped one. It must be unambiguous from the 3-part job token, so it embeds a literal `box` segment. + +**Files:** +- `apps/agent-worker/src/terminal-token.ts` — add `verifyBoxTerminalToken`; keep `verifyTerminalToken` unchanged. +- `apps/agent-worker/tests/unit/box-terminal-token.test.ts` — new test. + +**Interfaces:** +- **Token format (Produces/Consumes):** `${expiresAtMs}.box.${username}.${hmacSha256Hex}`, HMAC over the string `${expiresAtMs}.box.${username}`. Four dot-separated parts (job token has three), so the two can never collide. Username is `[a-z0-9_-]+` (already sanitized by `deriveHomeUsername`) and therefore never contains `.`. +- **Produces:** + ```ts + export const verifyBoxTerminalToken = ( + token: string, + username: string, + secret: string, + ): boolean; + ``` + Returns `true` only when: token has exactly 4 parts, part[1] === `'box'`, part[2] === `username`, expiry not passed, and the HMAC (over `${parts[0]}.box.${parts[2]}`) matches via `timingSafeEqual` on equal-length buffers. Empty token or empty secret → `false`. + +**Steps:** +- [ ] Write `apps/agent-worker/tests/unit/box-terminal-token.test.ts` mirroring `terminal-token.test.ts`: a local `mintBox(username, expiresAt, secret)` helper producing the 4-part token; cases: accepts valid unexpired username-matched token; rejects expired; rejects token minted for another username; rejects wrong secret; rejects malformed/empty; **rejects a 3-part job token passed to `verifyBoxTerminalToken`** (cross-scheme confusion guard). +- [ ] Run `cd apps/agent-worker && bun run test:unit` → FAIL (function missing). +- [ ] Implement `verifyBoxTerminalToken` in `terminal-token.ts` reusing the existing `signature()` helper. Split on `.`, require `parts.length === 4 && parts[1] === 'box'`, check `parts[2] === username`, parse+check expiry, `timingSafeEqual`. +- [ ] Run tests → PASS. +- [ ] Commit: `feat(worker): user-scoped box terminal token verification`. + +--- + +## Task 2: Worker — box helper module (home paths, status, lifecycle by username) + +Centralize per-user box path derivation, status inspection, and lifecycle so both HTTP routes and the WS bridge share one source of truth. + +**Files:** +- `apps/agent-worker/src/box.ts` — new module. +- `apps/agent-worker/src/runtime/docker.ts` — add `inspectUserBoxStatus`. +- `apps/agent-worker/tests/unit/box-paths.test.ts` — new test (pure path logic only). + +**Interfaces:** +- **Produces (`box.ts`):** + ```ts + export const boxHomePaths = (username: string) => ({ + homeDir: string, // path.resolve(env.workdir, 'homes', username) + containerHome: string, // path.posix.join('/home', username) + }); + + export type BoxStatus = { + running: boolean; + image: string | null; + startedAt: string | null; // ISO from container State.StartedAt, null if stopped/absent + memoryLimitBytes: number | null; + containerName: string; // userContainerName(username) + }; + export const getBoxStatus = (username: string) => Promise; + + export const startBox = (username: string) => Promise; // ensureUserContainer(boxHomePaths) + export const stopBox = (username: string) => Promise; // stop container + drop registry entry + export const restartBox = (username: string) => Promise; // stopBox then startBox + ``` +- **Produces (`docker.ts`):** + ```ts + export const inspectUserBoxStatus = (username: string) => Promise<{ + running: boolean; image: string | null; startedAt: string | null; memoryLimitBytes: number | null; + }>; + ``` + Implementation: `execa(containerRuntime(), ['inspect','--format','{{.State.Running}}|{{.Config.Image}}|{{.State.StartedAt}}|{{.HostConfig.Memory}}', userContainerName(username)], { reject:false, stdin:'ignore' })`. On non-zero exit (no such container) return `{ running:false, image:null, startedAt:null, memoryLimitBytes:null }`. Parse the pipe-split line; `running = field==='true'`; `memoryLimitBytes = Number(field) || null` (0 means unlimited → null); `startedAt` null when not running. +- **Consumes:** `ensureUserContainer`, `stopWorkspaceContainer`, `userContainerName` (docker.ts); `env.workdir` (env.ts). `stopBox` must also clear the in-memory registry so a stopped box isn't considered "held" — add `export const resetBox = (username: string) => void` to `user-container.ts` (clears the idle timer and deletes the map entry) and call it from `stopBox`. + +**Steps:** +- [ ] Add `resetBox(username)` to `user-container.ts` (clear `box.idleTimer`, `boxes.delete(username)`). +- [ ] Write `apps/agent-worker/tests/unit/box-paths.test.ts`: assert `boxHomePaths('gib')` returns `homeDir` ending in `homes/gib` and `containerHome === '/home/gib'`; assert two usernames produce distinct homeDirs. (Do NOT test docker-touching fns here.) +- [ ] Run `cd apps/agent-worker && bun run test:unit` → FAIL. +- [ ] Implement `inspectUserBoxStatus` in `docker.ts`, then `box.ts` (`boxHomePaths`, `getBoxStatus` wrapping `inspectUserBoxStatus` + `userContainerName`, `startBox`/`stopBox`/`restartBox`). +- [ ] Run tests → PASS. +- [ ] **Manual verification (docker required), record in the commit body:** with the worker running and a box up, `curl -H "Authorization: Bearer $TOKEN" localhost:3921/box/status?user=` (after Task 4) shows `running:true` + image + startedAt; after `POST /box/lifecycle {"action":"stop"}` status flips to `running:false`; `start` brings it back. +- [ ] Commit: `feat(worker): per-user box status and lifecycle helpers`. + +--- + +## Task 3: Worker — home-scoped file access (tree/read/write with realpath containment) + +Home-scoped equivalents of `listWorkspaceTree`/`readWorkspaceFile`/`writeWorkspaceFile`, rooted at the user's `homeDir`, hardened against symlink escape (reuse the Phase-1 containment approach: resolve `fs.realpath` of the parent and re-check containment; reject symlinked write targets). + +**Files:** +- `apps/agent-worker/src/box.ts` — add tree/read/write. +- `apps/agent-worker/tests/unit/box-files.test.ts` — new test against a real tmpdir. + +**Interfaces:** +- **Produces:** + ```ts + export const listBoxTree = (username: string) => Promise; + export const readBoxFile = (username: string, relPath: string) => Promise; + export const writeBoxFile = (username: string, relPath: string, content: string) => Promise<{ success: true }>; + // internal: safeHomePath(homeDir, relPath) => Promise (realpath-checked absolute path) + ``` + - `listBoxTree`: walk `homeDir` (root node `name: '~'`, `path: ''`), skipping a small ignore set: `.git`, `node_modules`, `.cache`, `.local/share/Trash`, `.npm`, `.bun`, `.cargo`, `dist`, `build`, `.next`. Cap traversal (e.g. skip directories once total node count exceeds ~5000) so a huge home can't hang the request. Node shape matches `FileTreeNode` from `agent-workspace/types.ts` (`{ name; path; type: 'file'|'directory'; children? }`). + - `readBoxFile`/`writeBoxFile`: resolve via `safeHomePath`. `writeBoxFile` `mkdir -p` the parent, then write. **No job/diff/event recording** (there is no job) — this is the key difference from `writeWorkspaceFile`. + - `safeHomePath`: lexical resolve, then `realpath` the existing ancestor and re-check `startsWith(homeDir + sep)`; throw `Refusing to access path outside home: ` on escape. For writes, if the target itself exists and is a symlink, reject. +- **Consumes:** `boxHomePaths`, `node:fs/promises` (`stat`, `readdir`, `readFile`, `writeFile`, `mkdir`, `realpath`), `FileTreeNode` type (import from where the worker declares it — check `worker.ts` for the local `FileTreeNode` type; if not exported, define a matching local type in `box.ts`). + +**Steps:** +- [ ] Write `apps/agent-worker/tests/unit/box-files.test.ts`: create a tmp home dir (`os.tmpdir()` + `mkdtemp`), stub `env.workdir` so `boxHomePaths('t').homeDir` points at it (either set `SPOON_AGENT_WORKDIR` before importing, or test `safeHomePath` directly by exporting it). Cases: `writeBoxFile` then `readBoxFile` round-trips; `readBoxFile('../../etc/passwd')` rejects; a symlink inside home pointing outside is rejected on read AND write; `listBoxTree` includes a created file and excludes `node_modules`. +- [ ] Run `cd apps/agent-worker && bun run test:unit` → FAIL. +- [ ] Implement `safeHomePath`, `listBoxTree`, `readBoxFile`, `writeBoxFile` in `box.ts`. +- [ ] Run tests → PASS. +- [ ] Commit: `feat(worker): home-scoped box file tree/read/write with realpath containment`. + +--- + +## Task 4: Worker — `/box/*` HTTP routes + +Wire the box helpers into the HTTP server, authed by the internal bearer token like every other route. + +**Files:** +- `apps/agent-worker/src/server.ts` — add box route dispatch (import from `./box`). + +**Interfaces (route → response shape):** +- `GET /box/status?user=` → `200 { status: BoxStatus }`. +- `POST /box/lifecycle?user=` body `{ action: 'start' | 'stop' | 'restart' }` → `200 { status: BoxStatus }` (return the post-action status; unknown action → `400 { error }`). +- `GET /box/tree?user=` → `200 { tree: FileTreeNode }`. +- `GET /box/file?user=&path=` → `200 { path, content }`. +- `PUT /box/file?user=` body `{ path, content }` → `200 { success: true }`. +- Missing/empty `user` → `400 { error: 'Missing user' }`. + +**Notes:** the `user` query param is trusted here because this route is only reachable with the internal bearer token, and the only caller (Next, Task 7) derives it from Convex auth. Add a `boxRoute(pathname)` matcher (mirror `jobRoute`, `server.ts:53-57`) matching `/^\/box\/(status|lifecycle|tree|file)$/`, and dispatch inside the existing try/catch in `startWorkerServer` **before** the `jobRoute` block. Read `user` via `url.searchParams.get('user')`. + +**Steps:** +- [ ] (No new unit test — this is HTTP glue over already-tested helpers; verify manually.) Add the `boxRoute` matcher and the five handlers in `server.ts`, importing `getBoxStatus, startBox, stopBox, restartBox, listBoxTree, readBoxFile, writeBoxFile` from `./box`. Reuse `sendJson`/`parseJson`. +- [ ] `cd apps/agent-worker && bun run build` (or `bun run typecheck`) → passes. +- [ ] **Manual verification (record in commit body):** with a box for your username, `curl -H "Authorization: Bearer $SPOON_WORKER_TOKEN" 'localhost:3921/box/status?user='` returns status JSON; `curl -X PUT ... 'localhost:3921/box/file?user=' -d '{"path":"spoon-test.txt","content":"hi"}'` then `GET .../box/file?...&path=spoon-test.txt` round-trips; `GET .../box/tree?user=` lists the home. +- [ ] Commit: `feat(worker): /box status, lifecycle, tree, and file HTTP routes`. + +--- + +## Task 5: Worker — `/box/terminal` WebSocket bridge + +Add a user-scoped terminal WS that acquires the box and opens a login shell rooted at `~`. Extract the shared PTY-bridging logic so the job and box bridges don't duplicate the exec/buffer/cleanup dance. + +**Files:** +- `apps/agent-worker/src/terminal.ts` — add `bridgeBox`, refactor shared exec bridging into a helper, add the `/box/terminal` upgrade match. + +**Interfaces:** +- **WS route:** `GET /box/terminal?token=`. Verify with `verifyBoxTerminalToken(token, username, env.terminalSecret)` — but the **username comes from the token itself** (parse part[2] after a successful format check), since the token is what authorizes the connection. Concretely: parse the token, extract the candidate username, then `if (!verifyBoxTerminalToken(token, candidateUsername, env.terminalSecret)) reject`. On success call `bridgeBox(ws, candidateUsername)`. +- **`bridgeBox(ws, username)`:** derive `{ homeDir, containerHome } = boxHomePaths(username)`; `const handle = await acquireUserBox({ username, workdir: homeDir, containerHome })`; ensure a login shell works even for a brand-new home by writing a minimal `.bash_profile` if absent (reuse the snippet from `user-environment.ts:62-68`, or extract that into a shared `ensureBashProfile(homeDir)` and call it here); spawn the same `exec -i` shell as `bridge()` but with `-w ` (root at `~`, not a repo) and env `TERM` + `HOME=` only (**no per-job secrets** — the box terminal is not job-scoped). Reuse the buffered-input + resize + cleanup logic; `cleanup()` calls `handle.release()` (or `releaseUserBox(username)` per the Phase-1 assumption). + +**Refactor detail:** extract from the current `bridge()` (`terminal.ts:21-150`) a helper like: +```ts +const runShellBridge = (ws, boxName, opts: { cwd: string; envFlags: string[]; getSize: () => {cols:number;rows:number} }) => { /* buffered input, spawn, forward, exit */ }; +``` +so both `bridge(ws, jobId)` and `bridgeBox(ws, username)` call it. Keep behavior identical for the job path (regression-guard by leaving its manual smoke test intact). + +**Steps:** +- [ ] In `attachTerminalServer` (`terminal.ts:157-181`), add a second upgrade match `/^\/box\/terminal$/`: read `token`, parse username, verify with `verifyBoxTerminalToken`, on failure `401` + destroy, on success `wss.handleUpgrade(... => bridgeBox(ws, username))`. Keep the existing `/jobs/:id/terminal` branch working. +- [ ] Implement `bridgeBox` and the shared `runShellBridge` (refactor `bridge` to use it). Add `ensureBashProfile` (shared with `user-environment.ts` or duplicated minimal). +- [ ] `cd apps/agent-worker && bun run test:unit` → existing tests still PASS; `bun run build`/typecheck passes. +- [ ] **Manual verification (record in commit body):** after Task 8/9 the browser flow covers this; for now, mint a box token by hand and connect a `websocat`/wscat client to `ws://localhost:3921/box/terminal?token=...`, confirm you land in `~` (`pwd` shows `/home/`), typing works, resize works (`stty size` reflects the client), and disconnect releases the box (status idle-reaps after `boxIdleMs`). +- [ ] Commit: `feat(worker): user-scoped /box/terminal websocket bridge`. + +--- + +## Task 6: Next — box proxy helpers (mint token, resolve username, proxy) + +Server-only helpers that mint the box token and proxy `/box/*` to the worker, deriving the username from Convex auth so the caller can only reach their own box. + +**Files:** +- `apps/next/src/lib/agent-worker-proxy.ts` — add `mintBoxTerminalToken`, `resolveBoxUsername`, `proxyBox`, `withBox`. +- `apps/next/tests/unit/box-terminal-token.test.ts` — new unit test for the mint format. + +**Interfaces:** +- **Produces:** + ```ts + // Mirrors mintTerminalToken (agent-worker-proxy.ts:20). Returns null if secret/WS base unset. + export const mintBoxTerminalToken = (username: string): { url: string; expiresAt: number } | null; + // payload = `${expiresAt}.box.${username}`; token = `${payload}.${hmacHex}`; + // url = `${wsBase}/box/terminal?token=${encodeURIComponent(token)}` (NOTE: /box/terminal, no jobId) + + // Resolves the authed caller's own box username, or an unauthorized/again response. + export const resolveBoxUsername = (): Promise< + | { ok: true; username: string } + | { ok: false; response: NextResponse } + >; + // uses convexAuthNextjsToken() then fetchQuery(api.userEnvironment.getMine, {}, { token }) -> .username + + // Proxies to the worker with the internal token, always appending user=. + export const proxyBox = ( + username: string, action: 'status'|'lifecycle'|'tree'|'file', + init?: RequestInit, search?: URLSearchParams, + ): Promise; + + // Convenience wrapper: resolve username -> run handler, 401/500 on failure (mirror withOwnedJob). + export const withBox = ( + handler: (username: string) => Promise, + ): Promise; + ``` +- **Consumes:** `terminalSecret()` and `workerToken()` (existing, `agent-worker-proxy.ts:12-15,41-42`), `env.NEXT_PUBLIC_SPOON_AGENT_WORKER_WS_URL`, `env.SPOON_AGENT_WORKER_URL`, `convexAuthNextjsToken`, `fetchQuery`, `api.userEnvironment.getMine`. + +**Steps:** +- [ ] Write `apps/next/tests/unit/box-terminal-token.test.ts`: import `createHmac`, replicate the expected token string for a fixed secret+username+expiry, and assert `mintBoxTerminalToken`'s `url` contains `/box/terminal?token=` and the token has 4 dot-parts with `parts[1]==='box'`. To make the secret deterministic, set `process.env.SPOON_AGENT_TERMINAL_SECRET` and `NEXT_PUBLIC_SPOON_AGENT_WORKER_WS_URL` before import (match how `apps/next/tests/unit/environment.test.ts` handles env). If importing `agent-worker-proxy.ts` pulls in `server-only`/Convex and fails under vitest, extract the pure token-building into a tiny `buildBoxToken(username, secret, wsBase, now)` and unit-test that instead (keep `mintBoxTerminalToken` a thin wrapper). Prefer the extraction — it keeps the test hermetic. +- [ ] Run `cd apps/next && bun run test:unit` → FAIL. +- [ ] Implement the four helpers (+ `buildBoxToken` if extracted). `resolveBoxUsername` returns `{ ok:false, response: 401 }` when no auth token; `proxyBox` mirrors `proxyWorker` (`agent-worker-proxy.ts:95-131`) but targets `/box/${action}` and force-sets `user` in the search params from the derived username. +- [ ] Run tests → PASS. +- [ ] Commit: `feat(web): box terminal token mint + ownership proxy helpers`. + +--- + +## Task 7: Next — `/api/box/*` route handlers + +App-Router handlers that derive the caller's username and proxy to the worker. + +**Files (all new):** +- `apps/next/src/app/api/box/status/route.ts` — `GET`. +- `apps/next/src/app/api/box/lifecycle/route.ts` — `POST`. +- `apps/next/src/app/api/box/tree/route.ts` — `GET`. +- `apps/next/src/app/api/box/file/route.ts` — `GET` + `PUT`. +- `apps/next/src/app/api/box/terminal-token/route.ts` — `GET`. + +**Interfaces (client contract):** +- `GET /api/box/status` → `{ status: BoxStatus }`. +- `POST /api/box/lifecycle` body `{ action:'start'|'stop'|'restart' }` → `{ status: BoxStatus }`. +- `GET /api/box/tree` → `{ tree: FileTreeNode }`. +- `GET /api/box/file?path=` → `{ path, content }`; `PUT /api/box/file` body `{ path, content }` → `{ success:true }`. +- `GET /api/box/terminal-token` → `{ url, expiresAt }` or `503 { error }` when unconfigured (mirror `agent-jobs/[jobId]/terminal-token/route.ts`). +- All return `401` when unauthenticated (via `resolveBoxUsername`/`withBox`). + +**Implementation:** each handler uses `withBox`/`resolveBoxUsername`. Example (`tree`): +```ts +export const GET = async () => await withBox(async (username) => await proxyBox(username, 'tree', { method: 'GET' })); +``` +`terminal-token` uses `resolveBoxUsername` then `mintBoxTerminalToken(username)` (503 if null). `file` forwards `path` and request body like `agent-jobs/[jobId]/file/route.ts:1-28`. `lifecycle` forwards the JSON body. + +**Steps:** +- [ ] Create the five route files following the `agent-jobs/[jobId]/*` handlers as templates but with `withBox`/`resolveBoxUsername` instead of `withOwnedJob`. +- [ ] `cd apps/next && bun run typecheck` (or the repo's lint/build) → passes. +- [ ] **Manual verification (record in commit body):** signed in, hit `/api/box/status` in the browser/devtools → `200` with your box status; signed out → `401`. `PUT`+`GET /api/box/file` round-trips a file into your home. +- [ ] Commit: `feat(web): /api/box status, lifecycle, tree, file, terminal-token routes`. + +--- + +## Task 8: Next — extract a reusable `XtermSession` component + +Factor the xterm wiring out of `workspace-terminal.tsx` so `/machine` and the workspace both use one implementation (DRY the WS/fit/fonts/theme logic). This also sets up Task 10's persistence work. + +**Files:** +- `apps/next/src/components/agent-workspace/xterm-session.tsx` — new shared component (move the themes + effect from `workspace-terminal.tsx:16-209`). +- `apps/next/src/components/agent-workspace/workspace-terminal.tsx` — becomes a thin wrapper over `XtermSession`. +- `apps/next/tests/component/xterm-session.test.tsx` — new (behavioral, jsdom-limited). + +**Interfaces:** +- **Produces:** + ```ts + type XtermStatus = 'idle' | 'connecting' | 'connected' | 'closed' | 'error' | 'unconfigured'; + export const XtermSession = (props: { + active: boolean; // mount/connect gate + tokenUrl: string; // e.g. `/api/box/terminal-token` or `/api/agent-jobs/${jobId}/terminal-token` + sessionKey: string; // identity for the connection effect deps (jobId or username) + waitingLabel?: string; // shown when active is false-but-expected (see Task 10) + copyOnSelect?: boolean; // Task 11 + }) => JSX.Element; + ``` + Preserve current behavior: lazy-import `@xterm/xterm`, `@xterm/addon-fit`, `@xterm/addon-web-links`; fetch `tokenUrl` for `{ url }`; open WS; `fit()` after `term.open()`; refit on `document.fonts.ready` / Nerd-Font load (Phase-1 sizing fix — keep it); theme swap without teardown; reconnect button. Move the two `ITheme` objects into this file. +- **`WorkspaceTerminal`** keeps its `{ jobId, active }` signature and renders ``. + +**Steps:** +- [ ] Write `apps/next/tests/component/xterm-session.test.tsx`. Since xterm can't render in jsdom, mock the three `@xterm/*` dynamic imports (`vi.mock('@xterm/xterm', ...)` returning a fake `Terminal` class with `open`/`loadAddon`/`onData`/`onResize`/`dispose`/`refresh` spies, etc.) and stub `global.WebSocket`. Assert: with `active:false` it renders the waiting label and does NOT construct a WebSocket; with `active:true` it fetches `tokenUrl` and opens a WebSocket to the returned `url`. (Mock `fetch` to return `{ url: 'ws://x' }`.) Keep assertions coarse — this is a smoke/behavior guard, not pixel-level. +- [ ] Run `cd apps/next && bun run test:component` → FAIL. +- [ ] Extract `XtermSession`, rewrite `WorkspaceTerminal` as the wrapper. Ensure the existing terminal tab still works (`agent-workspace-shell.tsx:664-667` passes `active`). +- [ ] Run tests → PASS. +- [ ] Commit: `refactor(web): extract reusable XtermSession from WorkspaceTerminal`. + +--- + +## Task 9: Next — `/machine` page + nav entry ("My Machine") + +The standalone dev-box surface: status card + lifecycle controls, home file browser (`FileTree` + `CodeEditor`), and a full-page `~`-rooted terminal. + +**Files (new unless noted):** +- `apps/next/src/app/(app)/machine/page.tsx` — the page. +- `apps/next/src/components/machine/machine-shell.tsx` — client shell (status card, actions, split file-browser/editor + terminal). Mirrors the load/save/tree logic from `agent-workspace-shell.tsx` but against `/api/box/*` and with no job concepts. +- `apps/next/src/components/machine/box-status-card.tsx` — status + start/stop/restart buttons. +- `apps/next/src/components/layout/header/index.tsx` — add the `/machine` nav item (edit, not new). +- `apps/next/tests/component/machine-shell.test.tsx` — new component test. + +**Interfaces / behavior:** +- **Nav:** add `{ href: '/machine', icon: , label: 'Machine' }` to the authenticated `navItems` array (`index.tsx:23-45`), placed after Threads. +- **Status card:** fetch `GET /api/box/status` on mount + after each lifecycle action; poll every ~10s. Show running/stopped badge, image, uptime (derive from `startedAt`), memory cap (format `memoryLimitBytes` → GiB, or "unlimited" when null). Buttons call `POST /api/box/lifecycle` with `start`/`stop`/`restart`; disable while a request is in flight; `toast.error` on failure and only `toast.success` after the awaited response resolves (per the repo's error-handling principle). Refresh status from the response. +- **File browser:** reuse `FileTree` (props exactly as `agent-workspace-shell.tsx:574-580`) fed by `GET /api/box/tree`; open files via `GET /api/box/file?path=`; edit in `CodeEditor`; save via `PUT /api/box/file`. Reuse the `OpenFileState`/`openFile`/`loadFile`/`writeFileContent` shapes from `agent-workspace-shell.tsx` (copy the minimal subset — no diff/UI-state persistence needed). `readOnly={false}`, `vimEnabled` local state. +- **Terminal:** ``, laid out full-page (own tab or a resizable pane — a Radix `Tabs` with "Files" and "Terminal", or a two-pane layout). The page is `~`-rooted by construction (worker `bridgeBox` uses `containerHome`). +- **Username:** the page reads `api.userEnvironment.getMine` via `useQuery` for display (firstName/username), but all box access is server-derived — the client never sends the username. + +**Steps:** +- [ ] Write `apps/next/tests/component/machine-shell.test.tsx`: mock `convex/react` (`useQuery` → `{ username:'gib', firstName:'Gib', enabled:true }`), `next/navigation`, `sonner`, and `@/components/agent-workspace/xterm-session` (render `
terminal
`) and `@/components/agent-workspace/code-editor` (Monaco won't run in jsdom). Mock `global.fetch` so `/api/box/status` returns a stopped box and `/api/box/tree` returns a small tree. Assert: the status card renders "Stopped" and a "Start" button; clicking "Start" POSTs `/api/box/lifecycle` with `{action:'start'}` (assert on the `fetch` mock); the file tree renders a known file; the terminal placeholder renders. +- [ ] Run `cd apps/next && bun run test:component` → FAIL. +- [ ] Build `box-status-card.tsx`, `machine-shell.tsx`, `machine/page.tsx`, and add the nav item. The page is a server component that renders the client `MachineShell` (like other `(app)` pages); auth-gating follows the existing `(app)` layout — no special guard needed beyond what the layout provides. +- [ ] Run tests → PASS. +- [ ] **Manual verification (record in commit body):** sign in, open `/machine`. (1) Status card shows correct running/stopped + image + uptime + mem cap; Start/Stop/Restart each change the status within a few seconds. (2) Terminal drops you into `~` (`pwd` = `/home/`), typing + resize work, output survives idle. (3) File browser lists your home; open+edit+save a file, confirm it persists (re-open shows the change; `cat` in the terminal confirms). +- [ ] Commit: `feat(web): /machine dev-box page with status, terminal, and home file browser`. + +--- + +## Task 10: Next — persistent workspace terminal (survive tab switches) + +Make the workspace Terminal tab keep its PTY/scrollback when switching Radix tabs, refit + resize-send when it becomes visible, and distinguish "waiting for workspace" from "connecting". + +**Files:** +- `apps/next/src/components/agent-workspace/agent-workspace-shell.tsx` — the Terminal `TabsContent` (currently `660-668`). +- `apps/next/src/components/agent-workspace/xterm-session.tsx` — add refit-on-visible + `waiting` handling. +- `apps/next/tests/component/xterm-session.test.tsx` — extend from Task 8. + +**Behavior / interfaces:** +- **Persistence:** give the Terminal `TabsContent` `forceMount` and hide it with CSS when inactive instead of unmounting (Radix `TabsContent` unmounts by default — that destroys the PTY, audit item #15). Pattern: ``. The other tabs stay as-is. Keep `` **mounted regardless of tab** so the WS/PTY persists; use a separate `visible` prop for fit behavior. +- **Refit on visible:** add a `visible: boolean` prop to `XtermSession`. When it transitions to `true`, call `fitAddon.fit()` inside `requestAnimationFrame` and send a resize frame (the Phase-1 pattern). Keep `active` as the connect gate (`workspaceReady`), `visible` as the CSS-visibility gate (`activeWorkspaceTab === 'terminal'`). In the shell, pass `active={workspaceReady}` and `visible={activeWorkspaceTab === 'terminal'}`. +- **Waiting vs connecting:** `XtermSession` status starts `'idle'`; when `active` is false but the workspace is expected (job pending), show `waitingLabel` ("Waiting for workspace…") — distinct from `'connecting'` (token fetched, WS opening). The shell already computes `workspacePending`/`workspaceReady` (`agent-workspace-shell.tsx:109-122`); pass an appropriate `waitingLabel`. + +**Steps:** +- [ ] Extend `xterm-session.test.tsx`: assert that toggling `visible` false→true triggers a `fit()` call on the mocked fit addon and sends a resize frame over the mocked WebSocket; assert that with `active:false` + a `waitingLabel`, the waiting label renders and no WebSocket is constructed; assert the component stays mounted (WS not closed) when `visible` goes true→false while `active` stays true. +- [ ] Run `cd apps/next && bun run test:component` → FAIL. +- [ ] Add the `visible` prop + refit effect to `XtermSession`; update `WorkspaceTerminal` to forward `visible`; change the Terminal `TabsContent` in `agent-workspace-shell.tsx` to `forceMount` + CSS-hide, and mount `` (update `WorkspaceTerminal` props to accept `visible`). +- [ ] Run tests → PASS. +- [ ] **Manual verification (record in commit body):** open a running thread workspace, go to Terminal, run `top` (or type a long-running output), switch to Editor then back — the session and scrollback are intact (not a fresh shell), and the terminal is correctly sized (not quarter-size) on return. Before a worker claims the job, the Terminal tab shows "Waiting for workspace…", not "Connecting…". +- [ ] Commit: `feat(web): persistent workspace terminal across tab switches`. + +--- + +## Task 11: Next — terminal QoL (copy-on-select; confirm web-links) + +Web-links is already loaded (`workspace-terminal.tsx:144` / carried into `XtermSession`). Add a copy-on-select toggle available in both terminals. + +**Files:** +- `apps/next/src/components/agent-workspace/xterm-session.tsx` — copy-on-select via `term.onSelectionChange`. +- `apps/next/tests/component/xterm-session.test.tsx` — extend. + +**Interfaces / behavior:** +- Add a `copyOnSelect?: boolean` prop (already declared in Task 8) with a small toggle control in the terminal header (a button/checkbox). When enabled, on `term.onSelectionChange` copy `term.getSelection()` to the clipboard (`navigator.clipboard.writeText`, guarded — clipboard may be unavailable/denied; swallow errors). Persist the toggle in `localStorage` (`spoon.terminal.copyOnSelect`) so it sticks across sessions. Confirm `WebLinksAddon` is loaded in `XtermSession` (it is — keep it; it makes URLs clickable). + +**Steps:** +- [ ] Extend `xterm-session.test.tsx`: mock `navigator.clipboard.writeText`; with `copyOnSelect` on, simulate the mocked terminal's selection-change callback with a non-empty selection and assert `writeText` was called with it; with it off, assert not called. Assert the header toggle renders and flipping it updates behavior. +- [ ] Run `cd apps/next && bun run test:component` → FAIL. +- [ ] Implement the toggle + `onSelectionChange` handler + `localStorage` persistence in `XtermSession`; ensure `WorkspaceTerminal` and the `/machine` terminal both expose it. +- [ ] Run tests → PASS. +- [ ] **Manual verification (record in commit body):** in `/machine` and a workspace terminal, enable copy-on-select, select text → it's on the clipboard; a printed URL is clickable (web-links) and opens in a new tab. +- [ ] Commit: `feat(web): terminal copy-on-select toggle and web-links polish`. + +--- + +## Final verification (run before declaring the phase done) + +- [ ] `cd apps/agent-worker && bun run test:unit` — all green (token, box-paths, box-files). +- [ ] `cd apps/next && bun run test:unit && bun run test:component` — all green. +- [ ] Typecheck/lint per repo scripts for both apps. +- [ ] End-to-end manual smoke on a docker-enabled deployment: `/machine` status+lifecycle+terminal+file-edit all work; workspace terminal persists across tab switches and is correctly sized; a user signed in as A cannot reach user B's box (all `/api/box/*` derive the username server-side — confirm by inspecting that no request carries a username). + +## Self-review against the Phase 3 spec + +- ✅ `/machine` page: status (running/stopped, image, uptime, mem cap), start/stop/restart, full-page `~`-rooted terminal, home file browser reusing FileTree/CodeEditor — Tasks 9 (+2,3,4 backing). +- ✅ Worker user-scoped HMAC tokens + `/box/tree|file|terminal` + status/lifecycle endpoints — Tasks 1–5. +- ✅ Same ownership-proxy pattern (Next mints after Convex auth, browser never sees worker secret) — Tasks 6–7; username derived server-side (a user can only reach their own box). +- ✅ Persistent workspace terminal (forceMount + CSS hide, refit-on-visible, waiting-vs-connecting) — Task 10. +- ✅ Terminal QoL (web-links, copy-on-select) — Task 11. +- ✅ Secret precedence + payload around username+expiry — Task 1/6. diff --git a/docs/superpowers/plans/2026-07-10-spoon-phase4-sync-correctness.md b/docs/superpowers/plans/2026-07-10-spoon-phase4-sync-correctness.md new file mode 100644 index 0000000..049f521 --- /dev/null +++ b/docs/superpowers/plans/2026-07-10-spoon-phase4-sync-correctness.md @@ -0,0 +1,621 @@ +# Spoon Phase 4 — Upstream-Sync Correctness: Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make Spoon's GitHub upstream-sync correct and event-driven: resolve head SHAs from the branch ref (never inferred from a truncated compare list), paginate compares where counts matter, give maintenance threads a stable dedup key (merge-base + head digest, never `Date.now()`), add a signature-verified GitHub webhook that does targeted refreshes on `push` and flips `gitConnections.status` on installation lifecycle events, add Octokit retry/throttling with a distinct "rate-limited" sync status, and fix unbounded `.collect()`s on hot paths. Surface `needs_reauth`/`revoked` in Settings → Integrations. + +**Architecture:** All work is in `packages/backend/convex` (Convex functions, one `httpAction`, one internalAction, schema indexes) plus a small `apps/next` settings UI change. The webhook `httpAction` runs in Convex's default (non-node) runtime and verifies the HMAC with Web Crypto (`crypto.subtle`), then `ctx.scheduler.runAfter(0, …)` schedules the node-runtime refresh. All Octokit calls stay in the existing `'use node'` modules (`githubClient.ts`, `githubSync.ts`). + +**Tech Stack:** Convex (functions, httpActions, crons), Octokit (`@octokit/rest` + `@octokit/plugin-retry` + `@octokit/plugin-throttling`), Next.js settings UI. + +**Depends on:** Phase 1 (auto-sync gating on `autoSyncEnabled` and the `require*` flags lands there; this phase does NOT touch the auto-sync decision at `githubSync.ts:205`). This phase adds head-SHA correctness, dedup, webhooks, and rate-limit handling on top. It is otherwise independent of Phases 1–3. + +## Global Constraints + +- **Backend tests:** live in `packages/backend/tests/unit/*.test.ts`, use `convex-test`. Run from `packages/backend`: `bun run test:unit` (this is `vitest run --project unit`). The test harness pattern is in `packages/backend/tests/unit/harness.test.ts` — copy its `convexTest(schema)`, `import.meta.glob('../../convex/**/*.*s')`, `createUser`, and `authed` helpers. +- **convex-test HTTP:** `const t = convexTest(schema, modules); const res = await t.fetch('/webhooks/github', { method: 'POST', headers, body })` returns a `Response`. Use this to test the webhook `httpAction`. +- **convex-test with node actions:** functions in `'use node'` files (`githubSync.ts`, `githubClient.ts`, `githubNode.ts`) cannot be unit-tested with convex-test directly (no node runtime in the test VM). For those, extract pure logic into a **non-`'use node'`** helper module and unit-test that; mock Octokit as a plain object literal shaped like the calls used (see Task 1/2 for the mock shape). Do NOT try to `t.action(...)` a node action in unit tests. +- Run `bun codegen:convex` from the repo root before `typecheck` whenever you add/rename a Convex function (regenerates `convex/_generated/api.d.ts`). Typecheck with `cd packages/backend && bun run typecheck`. +- Octokit plugin deps are NOT yet installed. Task 9 adds `@octokit/plugin-retry` and `@octokit/plugin-throttling` to `packages/backend/package.json` and runs `bun install` from the repo root. +- Conventional commits, one commit per task (e.g. `fix(sync): resolve head SHA from branch ref`). +- **Webhook signature:** HMAC-SHA256 over the **raw request body** (never a re-serialized JSON), header `X-Hub-Signature-256` formatted `sha256=`, compared with a constant-time comparison. Secret is `process.env.GITHUB_APP_WEBHOOK_SECRET` (already declared in `convex/globals.d.ts:14`). +- Ownership/security: the webhook is unauthenticated (GitHub calls it) — it MUST reject on bad/missing signature with HTTP 401 and never trust any user id from the payload; it only maps `installation.id` / repo full-name to existing owned records. + +--- + +## Task 1: `resolveBranchHead` — real head SHA from the branch ref + +**Problem:** `compareAcrossForkNetwork` sets `headSha = commits[commits.length - 1]?.sha` (`githubClient.ts:132`). GitHub's compare endpoint truncates `commits` to the last page (max ~250 across pages, 100/page here with no pagination), so for a divergence >100 commits the "head" is a middle commit, not the branch tip. Fetch the tip from the branch ref instead. + +**Files:** +- `packages/backend/convex/githubClient.ts` (add `resolveBranchHead` near `getRepository` ~line 82–89). +- `packages/backend/tests/unit/github-head.test.ts` (new). + +**Interfaces:** +- Produces: `export const resolveBranchHead = async (octokit: Octokit, owner: string, repo: string, branch: string): Promise<{ sha: string }>` + - Implementation: `const result = await octokit.rest.repos.getBranch({ owner, repo, branch }); return { sha: result.data.commit.sha };` +- Consumes: an `Octokit` whose `rest.repos.getBranch` resolves `{ data: { commit: { sha } } }`. + +**Note on testing node modules:** `githubClient.ts` is NOT a `'use node'` file (it has no `'use node'` pragma — verify: it starts with imports, no pragma). It can therefore be imported directly in a vitest unit test and called with a hand-rolled mock Octokit. The mock is a plain object: `{ rest: { repos: { getBranch: async () => ({ data: { commit: { sha: 'abc123' } } }) } } } as unknown as Octokit`. + +**Steps:** +- [ ] Write failing test `tests/unit/github-head.test.ts`: import `resolveBranchHead` from `../../convex/githubClient`; call it with a mock Octokit whose `getBranch` returns `{ data: { commit: { sha: 'deadbeef' } } }` and asserts the returned `sha === 'deadbeef'` and that `getBranch` was called with `{ owner: 'o', repo: 'r', branch: 'main' }` (use a `vi.fn()`). +- [ ] Run `cd packages/backend && bun run test:unit` — confirm it FAILS (export does not exist). +- [ ] Add `resolveBranchHead` to `githubClient.ts` exactly as in Interfaces. +- [ ] Run `bun run test:unit` — confirm PASS. +- [ ] `bun codegen:convex` (repo root) + `cd packages/backend && bun run typecheck` — confirm clean. +- [ ] Commit: `feat(sync): add resolveBranchHead to fetch branch tip SHA`. + +--- + +## Task 2: Paginate compare + use resolved head; fix `getLastCommitAt` + +**Problem:** `compareAcrossForkNetwork` (`githubClient.ts:110-137`) requests `per_page: 100` with no pagination and derives `headSha` from the truncated list; `getLastCommitAt` (`githubSync.ts:34-35`) has the same last-element bug. Paginate the commit list (bounded), take `ahead_by`/`merge_base_commit`/`base_commit` from the first page, and set `headSha` from `resolveBranchHead`. + +**Files:** +- `packages/backend/convex/githubClient.ts` (rewrite `compareAcrossForkNetwork` ~110-137; add `headRepo` to its args). +- `packages/backend/convex/githubSync.ts` (fix `getLastCommitAt` ~34-35; pass `headRepo` at both `compareAcrossForkNetwork` call sites ~107-122). +- `packages/backend/tests/unit/github-head.test.ts` (extend). + +**Interfaces:** +- Changed: `compareAcrossForkNetwork(octokit, args: { owner; repo; baseOwner; baseBranch; headOwner; headRepo; headBranch })` — **new required `headRepo: string`** (the repo the head branch lives in; upstream repo for the upstream compare, fork repo for the fork compare). +- The function now internally calls `resolveBranchHead(octokit, args.headOwner, args.headRepo, args.headBranch)` and sets `headSha` from it. +- `GitHubCompareSummary` shape (`githubClient.ts:37-44`) is unchanged. + +**Implementation for `compareAcrossForkNetwork`:** +```ts +const COMPARE_PER_PAGE = 100; +const COMPARE_MAX_PAGES = 30; // bound: up to 3000 commits collected + +export const compareAcrossForkNetwork = async ( + octokit: Octokit, + args: { + owner: string; + repo: string; + baseOwner: string; + baseBranch: string; + headOwner: string; + headRepo: string; + headBranch: string; + }, +): Promise => { + const basehead = `${args.baseOwner}:${args.baseBranch}...${args.headOwner}:${args.headBranch}`; + const first = await octokit.rest.repos.compareCommitsWithBasehead({ + owner: args.owner, + repo: args.repo, + basehead, + per_page: COMPARE_PER_PAGE, + page: 1, + }); + const rawCommits = [...first.data.commits]; + const totalCommits = first.data.total_commits; + let page = 2; + while (rawCommits.length < totalCommits && page <= COMPARE_MAX_PAGES) { + const next = await octokit.rest.repos.compareCommitsWithBasehead({ + owner: args.owner, + repo: args.repo, + basehead, + per_page: COMPARE_PER_PAGE, + page, + }); + if (next.data.commits.length === 0) break; + rawCommits.push(...next.data.commits); + page += 1; + } + const head = await resolveBranchHead( + octokit, + args.headOwner, + args.headRepo, + args.headBranch, + ); + return { + aheadBy: first.data.ahead_by, + mergeBaseSha: first.data.merge_base_commit.sha, + headSha: head.sha, + baseSha: first.data.base_commit.sha, + htmlUrl: first.data.html_url, + commits: rawCommits.map(normalizeCompareCommit), + }; +}; +``` + +**Implementation for `getLastCommitAt` (`githubSync.ts`):** +```ts +const getLastCommitAt = (compare: GitHubCompareSummary) => { + const head = compare.commits.find((c) => c.sha === compare.headSha); + return ( + head?.committedAt ?? + compare.commits[compare.commits.length - 1]?.committedAt + ); +}; +``` + +**Call-site edits in `githubSync.ts` (~107-122):** add `headRepo: spoon.upstreamRepo` to the upstream compare call and `headRepo: forkRepo` to the fork compare call. + +**Steps:** +- [ ] Extend `tests/unit/github-head.test.ts` with a test for `compareAcrossForkNetwork`: mock Octokit with `compareCommitsWithBasehead` returning page 1 of 100 commits with `total_commits: 150`, page 2 of 50 commits, and `getBranch` returning `{ data: { commit: { sha: 'TIP' } } }`. Assert `result.headSha === 'TIP'` (NOT the last commit in the list), `result.commits.length === 150`, `result.aheadBy` equals the mocked `ahead_by`. Use `merge_base_commit`/`base_commit` objects with `.sha` in the mock so mapping doesn't throw. +- [ ] Run `bun run test:unit` — confirm FAIL. +- [ ] Rewrite `compareAcrossForkNetwork` and `getLastCommitAt`; add `headRepo` to both call sites. +- [ ] Run `bun run test:unit` — confirm PASS. +- [ ] `bun codegen:convex` + `bun run typecheck` — confirm clean (the new required `headRepo` arg must be satisfied at both call sites). +- [ ] Commit: `fix(sync): paginate compare and resolve head SHA from branch ref`. + +--- + +## Task 3: Stable maintenance-thread dedup key (schema + pure helper) + +**Problem:** Threads are deduped by `upstreamTo` string with a `Date.now()` fallback (`githubSync.ts:232,271`, `threads.ts:426-440`). When head is unresolved the fallback makes a unique key every run → a brand-new maintenance thread + agent job every scheduled check. Replace with a deterministic key of `mergeBase:head`, and treat "no head" as "cannot dedup → skip". + +**Files:** +- `packages/backend/convex/maintenanceDedup.ts` (new, NOT `'use node'` — pure, unit-testable). +- `packages/backend/convex/schema.ts` (add `dedupKey` field + index to `threads` table). +- `packages/backend/tests/unit/maintenance-dedup.test.ts` (new). + +**Interfaces:** +- Produces: `export const maintenanceDedupKey = (input: { mergeBaseSha?: string; headSha?: string }): string | null` — returns `null` when `headSha` is missing/empty; otherwise `` `${input.mergeBaseSha ?? 'nobase'}:${input.headSha}` ``. + +**Schema change (`threads` table):** add `dedupKey: v.optional(v.string())` to the field list and a new index at the bottom of the table def: `.index('by_spoon_dedup', ['spoonId', 'dedupKey'])`. (Optional so existing rows validate; new maintenance threads always set it.) + +**Implementation (`maintenanceDedup.ts`):** +```ts +export const maintenanceDedupKey = (input: { + mergeBaseSha?: string; + headSha?: string; +}): string | null => { + const head = input.headSha?.trim(); + if (!head) return null; + const base = input.mergeBaseSha?.trim() || 'nobase'; + return `${base}:${head}`; +}; +``` + +**Steps:** +- [ ] Write failing `tests/unit/maintenance-dedup.test.ts`: `maintenanceDedupKey({ mergeBaseSha: 'a', headSha: 'b' }) === 'a:b'`; `maintenanceDedupKey({ headSha: 'b' }) === 'nobase:b'`; `maintenanceDedupKey({ mergeBaseSha: 'a' }) === null`; `maintenanceDedupKey({ mergeBaseSha: 'a', headSha: '' }) === null`. +- [ ] Run `bun run test:unit` — confirm FAIL (module missing). +- [ ] Create `maintenanceDedup.ts`; add `dedupKey` field + `by_spoon_dedup` index to `threads` in `schema.ts`. +- [ ] Run `bun run test:unit` — PASS. `bun codegen:convex` + `bun run typecheck` — clean. +- [ ] Commit: `feat(sync): add deterministic maintenance-thread dedup key`. + +--- + +## Task 4: `createMaintenanceThread` / `findOpenMaintenanceThread` dedup by key (indexed) + +**Problem:** Both dedup helpers `.collect()` every thread for the spoon then `.find(...)` in JS (`threads.ts:389-393` and `426-440`) — unbounded, and keyed on `upstreamTo`. Switch to `dedupKey` via the `by_spoon_dedup` index and stop collecting. + +**Files:** +- `packages/backend/convex/threads.ts` (`findOpenMaintenanceThread` ~382-405; `createMaintenanceThread` ~407-497). +- `packages/backend/tests/unit/maintenance-thread.test.ts` (new). + +**Interfaces:** +- Changed `createMaintenanceThread` args: **replace `upstreamTo: v.string()` with `dedupKey: v.string()`** (required; caller guarantees non-null — see Task 5). Keep `upstreamTo` but make it `v.optional(v.string())` (still stored for display). Persist `dedupKey` on the inserted thread row. +- Changed `findOpenMaintenanceThread` args: `{ spoonId, ownerId, dedupKey: v.string() }`. +- Both now query `.withIndex('by_spoon_dedup', (q) => q.eq('spoonId', args.spoonId).eq('dedupKey', args.dedupKey))`, then filter by `ownerId` and non-terminal status in JS over the (now tiny) result set. + +**Non-terminal status set (unchanged):** exclude `['resolved', 'ignored', 'failed', 'cancelled']`. + +**Existing-thread lookup in `createMaintenanceThread`:** +```ts +const existing = await ctx.db + .query('threads') + .withIndex('by_spoon_dedup', (q) => + q.eq('spoonId', args.spoonId).eq('dedupKey', args.dedupKey), + ) + .order('desc') + .collect() + .then((threads) => + threads.find( + (thread) => + thread.ownerId === args.ownerId && + !['resolved', 'ignored', 'failed', 'cancelled'].includes( + thread.status, + ), + ), + ); +``` +Insert path sets `dedupKey: args.dedupKey` and `upstreamTo: args.upstreamTo` on the new `threads` row. + +**Steps:** +- [ ] Write failing `tests/unit/maintenance-thread.test.ts` using convex-test: create a user + github spoon (copy `githubSpoonInput` + insert pattern from `harness.test.ts`; insert the spoon row directly via `t.mutation(async ctx => ctx.db.insert('spoons', {...}))`). Call `internal.threads.createMaintenanceThread` twice with the SAME `dedupKey` → assert the second returns the same `threadId` and that only one `threads` row exists for that spoon; then call with a DIFFERENT `dedupKey` → asserts a new thread id. (Invoke internal fns via `t.mutation(internal.threads.createMaintenanceThread, args)`.) +- [ ] Run `bun run test:unit` — FAIL (arg `dedupKey` not accepted yet). +- [ ] Update `createMaintenanceThread` + `findOpenMaintenanceThread` signatures and bodies; ensure the `threads.insert` writes `dedupKey`. +- [ ] Run `bun run test:unit` — PASS. `bun codegen:convex` + `bun run typecheck` — clean (call sites in `githubSync.ts` will still be broken until Task 5 — that's expected; if typecheck must be green per-commit, do Task 5 in the same commit; otherwise note it). +- [ ] Commit: `refactor(threads): dedup maintenance threads by stable key via index`. + +> **Sequencing note:** Tasks 4 and 5 both touch the `createMaintenanceThread` contract. If your workflow requires each commit to typecheck green, combine Tasks 4 and 5 into one commit. Otherwise keep them separate and run typecheck at the end of Task 5. + +--- + +## Task 5: `githubSync` call sites — pass `dedupKey`, never `Date.now()`, skip when unresolvable + +**Problem:** `githubSync.ts` builds `upstreamTo: upstreamCompare.headSha ?? \`${Date.now()}\`` at lines 232 and 271 (and a similar `${Date.now()}` fallback at 398 in `syncForkWithUpstream`). Replace with the deterministic key; when `maintenanceDedupKey(...)` is `null` (no head resolvable), **skip thread creation entirely** and record the skip on the sync run instead. + +**Files:** +- `packages/backend/convex/githubSync.ts` (merge-conflict branch ~223-258; diverged branch ~261-284; `syncForkWithUpstream` conflict branch ~387-411). + +**Interfaces:** +- Consumes: `maintenanceDedupKey` from `./maintenanceDedup`, `internal.threads.createMaintenanceThread` (now takes `dedupKey`). +- Import at top of `githubSync.ts`: `import { maintenanceDedupKey } from './maintenanceDedup';` + +**Pattern (apply at each of the 3 thread-creation sites):** +```ts +const dedupKey = maintenanceDedupKey({ + mergeBaseSha: upstreamCompare.mergeBaseSha ?? forkCompare.mergeBaseSha, + headSha: upstreamCompare.headSha, +}); +if (!dedupKey) { + await ctx.runMutation(internal.syncRuns.patchInternal, { + syncRunId, + status: 'failed', + error: 'Could not resolve upstream head SHA; skipping maintenance thread.', + }); + // return the appropriate summary object for this branch (do NOT create a thread) +} else { + const threadId = await ctx.runMutation( + internal.threads.createMaintenanceThread, + { + /* existing args, but: */ + dedupKey, + upstreamTo: upstreamCompare.headSha, // now optional, display only + upstreamFrom: upstreamCompare.mergeBaseSha, + // ...title, summary, forkHeadAtCreation, mergeBaseAtCreation, relatedSyncRunId, jobType + }, + ); + // existing syncRuns/spoons patches +} +``` +For the `syncForkWithUpstream` conflict branch (~387), build the key from `state.mergeBaseSha ?? spoon.lastMergeBaseCommit` and `state.upstreamHeadSha ?? spoon.lastUpstreamCommit`; if `null`, skip thread creation and just patch the sync run/ spoon error. + +**Steps:** +- [ ] Grep-confirm there are no remaining `Date.now()}` fallbacks feeding `upstreamTo` in `githubSync.ts` after edits: `grep -n "Date.now()}" packages/backend/convex/githubSync.ts` returns nothing. +- [ ] Apply the pattern to all 3 sites; remove the old `upstreamTo: … ?? \`${Date.now()}\`` fallbacks. +- [ ] `bun codegen:convex` + `bun run typecheck` — clean. Re-run `bun run test:unit` (Task 4 test still passes). +- [ ] Commit: `fix(sync): use stable dedup key and skip threads when head unresolved`. + +--- + +## Task 6: Webhook signature verifier (pure, Web Crypto) + known-vector unit test + +**Problem:** No webhook verification exists. Implement HMAC-SHA256 over the raw body, constant-time compared against `X-Hub-Signature-256`. Must run in Convex's default (non-node) httpAction runtime, so use Web Crypto (`crypto.subtle`), not `node:crypto`. + +**Files:** +- `packages/backend/convex/githubWebhookVerify.ts` (new, NOT `'use node'`). +- `packages/backend/tests/unit/github-webhook-verify.test.ts` (new). + +**Interfaces:** +- Produces: `export const verifyGithubSignature = async (secret: string, rawBody: string, signatureHeader: string | null): Promise` +- Produces (helper, exported for reuse/testing): `export const timingSafeEqualHex = (a: string, b: string): boolean` — length-checked, constant-time char compare. + +**Implementation:** +```ts +const toHex = (buffer: ArrayBuffer): string => + Array.from(new Uint8Array(buffer)) + .map((b) => b.toString(16).padStart(2, '0')) + .join(''); + +export const timingSafeEqualHex = (a: string, b: string): boolean => { + if (a.length !== b.length) return false; + let mismatch = 0; + for (let i = 0; i < a.length; i += 1) { + mismatch |= a.charCodeAt(i) ^ b.charCodeAt(i); + } + return mismatch === 0; +}; + +export const verifyGithubSignature = async ( + secret: string, + rawBody: string, + signatureHeader: string | null, +): Promise => { + if (!signatureHeader || !signatureHeader.startsWith('sha256=')) return false; + const provided = signatureHeader.slice('sha256='.length); + const key = await crypto.subtle.importKey( + 'raw', + new TextEncoder().encode(secret), + { name: 'HMAC', hash: 'SHA-256' }, + false, + ['sign'], + ); + const mac = await crypto.subtle.sign( + 'HMAC', + key, + new TextEncoder().encode(rawBody), + ); + return timingSafeEqualHex(toHex(mac), provided); +}; +``` + +**Known test vector (from GitHub's webhook docs):** secret `It's a Secret to Everybody`, body `Hello, World!` → `sha256=757107ea0eb2509fc211221cce984b8a37570b6d7586c22c46f4379c8b043e17`. + +**Steps:** +- [ ] Write failing `tests/unit/github-webhook-verify.test.ts`: + - `await verifyGithubSignature("It's a Secret to Everybody", 'Hello, World!', 'sha256=757107ea0eb2509fc211221cce984b8a37570b6d7586c22c46f4379c8b043e17')` → `true`. + - Wrong signature → `false`; missing `sha256=` prefix → `false`; `null` header → `false`; correct hex but wrong secret → `false`. + - `timingSafeEqualHex('abcd','abcd') === true`, `timingSafeEqualHex('abcd','abce') === false`, different lengths → `false`. +- [ ] Run `bun run test:unit` — FAIL (module missing). +- [ ] Create `githubWebhookVerify.ts`. +- [ ] Run `bun run test:unit` — PASS (the known vector proves the HMAC is correct). `bun run typecheck` — clean. +- [ ] Commit: `feat(webhooks): add GitHub webhook signature verifier`. + +--- + +## Task 7: `gitConnections` status-by-installation index + mutation + +**Problem:** `gitConnections.status` is only ever written `'active'` (`github.ts:61,123`); nothing sets `needs_reauth`/`revoked`. The webhook needs to flip status by GitHub `installation.id`, but there is no index on `installationId`. Add one and an internal mutation. + +**Files:** +- `packages/backend/convex/schema.ts` (`gitConnections` table ~32-56: add `.index('by_installation', ['installationId'])`). +- `packages/backend/convex/github.ts` (add internal mutation). +- `packages/backend/tests/unit/git-connection-status.test.ts` (new). + +**Interfaces:** +- Produces: `export const setConnectionStatusByInstallation = internalMutation({ args: { installationId: v.string(), status: v.union(v.literal('active'), v.literal('needs_reauth'), v.literal('revoked')) }, handler })` + - Body: query `by_installation` for the given `installationId`, patch every matching row with `{ status, updatedAt: Date.now() }`. Return `{ updated: }`. (Use `.collect()` here — installations map to at most a handful of connection rows; this is not a hot path.) + +**Steps:** +- [ ] Write failing `tests/unit/git-connection-status.test.ts` (convex-test): create a user, insert a `gitConnections` row with `installationId: '42'`, `status: 'active'`; call `internal.github.setConnectionStatusByInstallation` with `{ installationId: '42', status: 'needs_reauth' }`; re-read the row and assert `status === 'needs_reauth'`. Add a case where `installationId` matches nothing → `{ updated: 0 }`. +- [ ] Run `bun run test:unit` — FAIL. +- [ ] Add `by_installation` index + `setConnectionStatusByInstallation` mutation. +- [ ] `bun codegen:convex` + `bun run test:unit` PASS + `bun run typecheck` clean. +- [ ] Commit: `feat(github): index connections by installation + status mutation`. + +--- + +## Task 8: Find owned spoons for a pushed repo (index + internal query) + +**Problem:** A `push` webhook carries a repo full-name (`owner/repo`) and `installation.id`. To do a targeted refresh we must map that to owned `spoons`. Pushes to the **fork** change `forkHeadSha`; pushes to the **upstream** (if the app is installed there) change `upstreamHeadSha`. The `spoons` table already has `by_upstream` (`['provider','upstreamOwner','upstreamRepo']`) but no fork index. + +**Files:** +- `packages/backend/convex/schema.ts` (`spoons` table: add `.index('by_fork', ['provider', 'forkOwner', 'forkRepo'])`). +- `packages/backend/convex/spoons.ts` (add internal query). +- `packages/backend/tests/unit/spoons-for-repo.test.ts` (new). + +**Interfaces:** +- Produces: `export const findForRepo = internalQuery({ args: { owner: v.string(), repo: v.string() }, handler }): Promise<{ spoonId: Id<'spoons'>; ownerId: Id<'users'> }[]>` + - Query `by_upstream` with `provider:'github', upstreamOwner: owner, upstreamRepo: repo` → collect. Query `by_fork` with `provider:'github', forkOwner: owner, forkRepo: repo` → collect. Union, dedup by `spoonId`, exclude `status === 'archived'`, return `{ spoonId, ownerId }[]`. + +**Steps:** +- [ ] Write failing `tests/unit/spoons-for-repo.test.ts` (convex-test): insert two github spoons — one where `upstreamOwner/upstreamRepo = 'up/x'`, another where `forkOwner/forkRepo = 'me/x-fork'`. Call `internal.spoons.findForRepo` with `{ owner: 'up', repo: 'x' }` → returns the first spoon. Call with `{ owner: 'me', repo: 'x-fork' }` → returns the second. Call with an unknown repo → `[]`. Add a case where the same spoon matches both (upstream == fork owner/repo edge) → returned once. +- [ ] Run `bun run test:unit` — FAIL. +- [ ] Add `by_fork` index + `findForRepo` internal query. +- [ ] `bun codegen:convex` + `bun run test:unit` PASS + `bun run typecheck` clean. +- [ ] Commit: `feat(spoons): find owned spoons by pushed repo (indexed)`. + +--- + +## Task 9: Targeted-refresh internalAction + webhook httpAction route + +**Problem:** No webhook endpoint. Add a Convex `httpAction` at `/webhooks/github` that verifies the signature, then: on `push` → schedule a targeted refresh for each matching spoon; on `installation`/`installation_repositories` → flip `gitConnections.status`. The hourly cron (`crons.ts`) stays as fallback (unchanged). + +**Files:** +- `packages/backend/convex/githubSync.ts` (add `refreshSpoonById` internalAction wrapping the existing module-local `refreshOwnedSpoon`). +- `packages/backend/convex/githubWebhooks.ts` (new — the `httpAction` handler; NOT `'use node'`, uses Web Crypto + scheduler + runQuery/runMutation only). +- `packages/backend/convex/http.ts` (register the route). +- `packages/backend/tests/unit/github-webhook-route.test.ts` (new). + +**Interfaces:** +- Produces: `export const refreshSpoonById = internalAction({ args: { spoonId: v.id('spoons'), ownerId: v.id('users') }, handler })` — calls `await refreshOwnedSpoon(ctx, ownerId, spoonId, 'scheduled_check')`; catches and swallows errors into a returned `{ success: boolean; error?: string }` so webhook fan-out never throws unhandled. +- Produces: `export const handleGithubWebhook = httpAction(async (ctx, request) => Response)` in `githubWebhooks.ts`. +- Route (in `http.ts`): `http.route({ path: '/webhooks/github', method: 'POST', handler: handleGithubWebhook });` +- **Webhook route path:** `POST /webhooks/github` (full URL is `/webhooks/github` — this is the URL to configure in the GitHub App settings; note it in the final report for deploy docs). + +**`handleGithubWebhook` behavior:** +```ts +export const handleGithubWebhook = httpAction(async (ctx, request) => { + const secret = process.env.GITHUB_APP_WEBHOOK_SECRET; + if (!secret) return new Response('Webhook not configured', { status: 503 }); + const rawBody = await request.text(); + const signature = request.headers.get('x-hub-signature-256'); + const ok = await verifyGithubSignature(secret, rawBody, signature); + if (!ok) return new Response('Invalid signature', { status: 401 }); + + const event = request.headers.get('x-github-event'); + const payload = JSON.parse(rawBody) as GithubWebhookPayload; // narrow type below + + if (event === 'push') { + const fullName: string | undefined = payload.repository?.full_name; + if (fullName) { + const [owner, repo] = fullName.split('/'); + const targets = await ctx.runQuery(internal.spoons.findForRepo, { + owner, + repo, + }); + for (const t of targets) { + await ctx.scheduler.runAfter(0, internal.githubSync.refreshSpoonById, { + spoonId: t.spoonId, + ownerId: t.ownerId, + }); + } + } + } else if (event === 'installation' || event === 'installation_repositories') { + const installationId = String(payload.installation?.id ?? ''); + const action = payload.action; // 'deleted' | 'suspend' | 'unsuspend' | 'created' | ... + if (installationId) { + const status = + action === 'deleted' || action === 'suspend' + ? ('revoked' as const) + : action === 'unsuspend' || action === 'created' + ? ('active' as const) + : ('needs_reauth' as const); // 'new_permissions_accepted', 'removed', etc. + await ctx.runMutation( + internal.github.setConnectionStatusByInstallation, + { installationId, status }, + ); + } + } + // Always 200 for verified events we don't act on, so GitHub doesn't retry. + return new Response('ok', { status: 200 }); +}); +``` +Define a minimal payload type (`repository?: { full_name?: string }; installation?: { id?: number }; action?: string`) rather than importing Octokit webhook types. + +**Runtime note:** `httpAction` cannot call node actions synchronously, but `ctx.scheduler.runAfter(0, internal.githubSync.refreshSpoonById, …)` schedules the node internalAction fine. `refreshSpoonById` lives in the `'use node'` `githubSync.ts` — that is allowed as a scheduled target. + +**Steps:** +- [ ] Add `refreshSpoonById` internalAction to `githubSync.ts` (wrap `refreshOwnedSpoon` in try/catch, return `{ success, error? }`). +- [ ] Create `githubWebhooks.ts` with `handleGithubWebhook`; register the route in `http.ts` (keep `auth.addHttpRoutes(http)`). +- [ ] Write `tests/unit/github-webhook-route.test.ts` (convex-test `t.fetch`): + - Set the secret for the test: `t.mutation` can't set env; instead the verifier reads `process.env.GITHUB_APP_WEBHOOK_SECRET`. In the test, set `process.env.GITHUB_APP_WEBHOOK_SECRET = 'testsecret'` in a `beforeAll` (vitest allows mutating `process.env`), and compute the expected signature with the same Web Crypto HMAC (import `verifyGithubSignature`'s sibling or recompute via `node:crypto` `createHmac('sha256', secret).update(body).digest('hex')` in the test file — node crypto is available in the vitest process even though the function under test uses Web Crypto). + - **Bad signature:** `t.fetch('/webhooks/github', { method:'POST', headers:{'x-github-event':'push','x-hub-signature-256':'sha256=deadbeef'}, body })` → assert `res.status === 401`. + - **installation deleted:** seed a `gitConnections` row (installationId `'99'`, status `active`); POST an `installation` event `{ action:'deleted', installation:{ id:99 } }` with a VALID signature → assert `res.status === 200` and the connection row is now `revoked`. + - **push:** seed a github spoon whose fork is `me/x-fork`; POST a `push` with `{ repository:{ full_name:'me/x-fork' } }` and valid signature → assert `res.status === 200`. (You cannot assert the node action ran under convex-test; assert the 200 + that no throw occurred. Optionally assert `findForRepo` is exercised by checking a scheduled function was enqueued via `await t.finishInProgressScheduledFunctions()` doesn't throw — but since `refreshSpoonById` is a node action it will no-op/throw in the test VM; prefer NOT to finish scheduled functions here, just assert the 200.) +- [ ] Run `bun run test:unit` — iterate to PASS. +- [ ] `bun codegen:convex` + `bun run typecheck` — clean. +- [ ] Commit: `feat(webhooks): add signature-verified GitHub webhook route`. + +--- + +## Task 10: Octokit retry + throttling plugins; distinguish rate-limited from hard error + +**Problem:** `getInstallationOctokit` (`githubClient.ts:54-68`) uses a bare `Octokit` with no retry/backoff. On rate limits, refreshes hard-fail and the spoon shows `syncStatus: 'error'` with no distinction. Add the retry + throttling plugins and a distinct `rate_limited` status. + +**Files:** +- `packages/backend/package.json` (add deps). +- `packages/backend/convex/githubClient.ts` (compose plugins; add `isRateLimitError` helper). +- `packages/backend/convex/schema.ts` (`spoons.syncStatus` union: add `v.literal('rate_limited')`). +- `packages/backend/convex/githubSync.ts` (catch block ~291-308: branch on `isRateLimitError`). +- `packages/backend/tests/unit/rate-limit.test.ts` (new). + +**Interfaces:** +- Deps: `"@octokit/plugin-retry": "^7.1.0"`, `"@octokit/plugin-throttling": "^9.3.0"` (match the `@octokit/rest@^22` core version line; verify with `bun install` that peer ranges resolve — bump if bun errors). +- `getInstallationOctokit` composition: +```ts +import { retry } from '@octokit/plugin-retry'; +import { throttling } from '@octokit/plugin-throttling'; +const SpoonOctokit = Octokit.plugin(retry, throttling); +export const getInstallationOctokit = (installationId: string) => + new SpoonOctokit({ + authStrategy: createAppAuth, + auth: { appId: getEnv('GITHUB_APP_ID'), privateKey: normalizePrivateKey(getEnv('GITHUB_APP_PRIVATE_KEY')), installationId }, + userAgent: 'Spoon', + request: { headers: { 'X-GitHub-Api-Version': '2022-11-28' } }, + throttle: { + onRateLimit: (retryAfter, options, _octokit, retryCount) => retryCount < 2, + onSecondaryRateLimit: (retryAfter, options, _octokit, retryCount) => retryCount < 2, + }, + }); +``` +- Produces (pure, exported): `export const isRateLimitError = (error: unknown): boolean` — returns true when `error` looks like an Octokit `RequestError` with `status === 403 || status === 429` and a rate-limit signal (message includes `rate limit`, or `response.headers['x-ratelimit-remaining'] === '0'`, or `x-ratelimit-reset` present). Guard with `typeof error === 'object' && error !== null`. + +**Catch-block change in `refreshOwnedSpoon` (`githubSync.ts:291`):** +```ts +} catch (error) { + const message = error instanceof Error ? error.message : String(error); + const rateLimited = isRateLimitError(error); + await Promise.all([ + ctx.runMutation(internal.spoons.patchSyncFields, { + spoonId, + syncStatus: rateLimited ? 'rate_limited' : 'error', + lastGithubRefreshAt: Date.now(), + lastCheckedAt: Date.now(), + lastError: rateLimited ? `Rate limited by GitHub; will retry. ${message}` : message, + }), + ctx.runMutation(internal.syncRuns.patchInternal, { + syncRunId, + status: 'failed', + error: message, + }), + ]); + throw new ConvexError(message); +} +``` +(The hourly cron / webhook already re-drive the refresh, so `rate_limited` is the "will retry" state; no new scheduling needed. `patchSyncFields` already accepts `syncStatus` — extend its arg validator if it enumerates the union; check `spoons.patchSyncFields` ~316 and add `'rate_limited'` there too if the union is inlined.) + +**Steps:** +- [ ] Add the two deps to `packages/backend/package.json`; run `bun install` from repo root; confirm it resolves (bump versions if bun reports peer conflicts against `@octokit/core` used by `@octokit/rest@22`). +- [ ] Write failing `tests/unit/rate-limit.test.ts`: `isRateLimitError({ status: 403, response: { headers: { 'x-ratelimit-remaining': '0' } } })` → true; `isRateLimitError({ status: 429 })` → true; `isRateLimitError({ status: 404 })` → false; `isRateLimitError(new Error('boom'))` → false; `isRateLimitError({ status: 403, message: 'secondary rate limit' })` → true. +- [ ] Run `bun run test:unit` — FAIL. +- [ ] Add `isRateLimitError` + plugin composition to `githubClient.ts`; add `'rate_limited'` to `spoons.syncStatus` in `schema.ts` (and to `patchSyncFields`'s inline union if present); update the catch block. +- [ ] `bun codegen:convex` + `bun run test:unit` PASS + `bun run typecheck` clean. +- [ ] Commit: `feat(sync): add Octokit retry/throttle and rate-limited status`. + +--- + +## Task 11: Fix unbounded `.collect()`s on hot paths + +**Problem:** Several hot queries `.collect()` owner-wide then filter/slice in JS: +- `spoonCommits.listForSpoon` no-side branch (`spoonCommits.ts:26-33`) collects ALL of an owner's commits. +- `agentJobs.countOldWorkspaces` (`agentJobs.ts:1009-1012`) and `deleteOldWorkspaces` (`agentJobs.ts:1031-1034`) collect ALL of an owner's jobs. + +**Files:** +- `packages/backend/convex/spoonCommits.ts` (~26-33). +- `packages/backend/convex/agentJobs.ts` (~1001-1044). +- `packages/backend/tests/unit/hot-paths.test.ts` (new). + +**Fix — `spoonCommits.listForSpoon` no-side branch:** the table already has `by_spoon_side`. Replace the owner-wide collect with two indexed `.take()`s (one per side) merged and sorted: +```ts +const [upstream, fork] = await Promise.all([ + ctx.db.query('spoonCommits').withIndex('by_spoon_side', (q) => q.eq('spoonId', spoonId).eq('side', 'upstream')).order('desc').take(limit ?? 100), + ctx.db.query('spoonCommits').withIndex('by_spoon_side', (q) => q.eq('spoonId', spoonId).eq('side', 'fork')).order('desc').take(limit ?? 100), +]); +return [...upstream, ...fork].sort((a, b) => (b.committedAt ?? 0) - (a.committedAt ?? 0)).slice(0, limit ?? 100); +``` + +**Fix — `agentJobs.countOldWorkspaces` / `deleteOldWorkspaces`:** cap the scan with `.take()` on the existing `by_owner` index instead of unbounded `.collect()`. For count, take a bounded window (e.g. `.take(500)`) — document that count is "up to N"; for delete, `deleteOldWorkspaces` already slices to `max` (≤100), so change its collect to `.take(500)` before filtering, keeping the `max` slice. (These are maintenance/settings actions, not per-request hot loops, but the owner-wide `.collect()` is still the audited unbounded read; bounding it removes the growth risk.) + +**Steps:** +- [ ] Write failing `tests/unit/hot-paths.test.ts` (convex-test): seed a spoon with, say, 3 upstream + 3 fork `spoonCommits` rows (insert directly); call `api.spoonCommits.listForSpoon` (authed) with no `side`, `limit: 4` → assert exactly 4 rows returned, newest `committedAt` first, and both sides represented. (This asserts behavior; the index change must keep the same observable ordering.) +- [ ] Run `bun run test:unit` — should PASS against current code IF ordering matches; adjust the test to pin ordering so the refactor is covered (make committedAt values interleave upstream/fork so a per-side take + merge is required to get the right top-4). Confirm the test FAILS if you naively `.take(4)` one side — i.e. it genuinely exercises the merge. +- [ ] Apply the `spoonCommits.listForSpoon` fix; apply the `.take(500)` bound to both agentJobs functions. +- [ ] Run `bun run test:unit` — PASS. `bun run typecheck` — clean. +- [ ] Commit: `perf(convex): bound unbounded owner-wide collects on hot paths`. + +--- + +## Task 12: Surface `needs_reauth`/`revoked` in Settings → Integrations + +**Problem:** `GithubIntegrationPanel` (`apps/next/src/components/integrations/github-integration-panel.tsx`) shows the connection but never surfaces `connection.status`. Users can't see when GitHub access is broken. `api.github.getConnection` already returns the full row (with `status`). + +**Files:** +- `apps/next/src/components/integrations/github-integration-panel.tsx`. + +**Interfaces:** +- Consumes: `connection.status: 'active' | 'needs_reauth' | 'revoked'` (already on the query result). +- Add a status badge/alert block in the connected branch (~line 51-61): when `status !== 'active'`, render a prominent warning with a "Reconnect GitHub App" CTA linking to `installUrl` (the existing `installUrl` query). Copy: + - `needs_reauth`: "GitHub access needs re-authorization. Reconnect the app to resume syncing." (amber) + - `revoked`: "The GitHub App installation was removed. Reinstall to resume syncing." (red) + +**Implementation sketch:** +```tsx +{connection && connection.status !== 'active' ? ( +
+

+ {connection.status === 'revoked' + ? 'GitHub App installation removed' + : 'GitHub access needs re-authorization'} +

+

+ {connection.status === 'revoked' + ? 'Reinstall the app to resume syncing.' + : 'Reconnect the app to resume syncing.'} +

+ {installUrl ? ( + + Reconnect GitHub App + + ) : null} +
+) : null} +``` + +**Steps:** +- [ ] Add the status warning block to `github-integration-panel.tsx` (guard on `connection` being present; place above or within the connected grid). +- [ ] Verify types: `bun run typecheck` in `apps/next` (or repo-root typecheck task) — the `status` field is already typed via the generated api. +- [ ] Manual check (no automated Next component test required for this phase): run the app, temporarily set a connection row's `status` to `needs_reauth` (via Convex dashboard or a test mutation) and confirm the banner + reconnect CTA render on Settings → Integrations. +- [ ] Commit: `feat(settings): surface GitHub connection needs_reauth/revoked`. + +--- + +## Manual verification checklist (end of phase) + +- [ ] `cd packages/backend && bun run test:unit` — all green. +- [ ] `bun codegen:convex` (root) then `cd packages/backend && bun run typecheck` — clean. +- [ ] Deploy notes for the operator: configure the GitHub App webhook URL to `/webhooks/github`, subscribe to `push`, `installation`, and `installation_repositories` events, and set `GITHUB_APP_WEBHOOK_SECRET` in the Convex deployment env (matching the App's webhook secret). +- [ ] Trigger a real `push` to a watched fork → confirm a targeted refresh runs (spoon `lastGithubRefreshAt` updates within seconds, not the hourly cron window). +- [ ] Uninstall/reinstall the App → confirm `gitConnections.status` flips to `revoked`/`active` and the Settings → Integrations banner reflects it. +- [ ] Force a divergence >100 commits (or a fork far behind) → confirm `upstreamHeadSha` equals the real branch tip and only ONE maintenance thread is created across repeated scheduled checks. diff --git a/docs/superpowers/plans/2026-07-10-spoon-phase5-notifications-polish.md b/docs/superpowers/plans/2026-07-10-spoon-phase5-notifications-polish.md new file mode 100644 index 0000000..f93d350 --- /dev/null +++ b/docs/superpowers/plans/2026-07-10-spoon-phase5-notifications-polish.md @@ -0,0 +1,405 @@ +# Spoon Phase 5 — Notifications & Polish: Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Ship the one net-new subsystem for Phase 5 — a `notifications` table with an in-app header bell and preference-gated email via the existing UseSend integration — then sweep the audit's polish findings (#14–#20: loading skeletons, fire-and-forget error handling, form re-sync, auth copy, workspace UX, accessibility, dead code) and rewrite the docs for the box-first model. + +**Architecture:** Notifications are written by a single shared TS helper `emitNotification(ctx, {...})` invoked at the existing event points (maintenance-thread creation, job status transitions, sync failure, connection re-auth). The helper inserts an in-app row and, when the user's per-kind email preference allows, schedules a `'use node'` internal action that sends through UseSend (`usesend-js`, already a dependency). The header bell subscribes to a reactive inbox query; mark-read/mark-all-read are mutations. Preferences live in a per-user `notificationPreferences` row edited from a new Settings → Notifications page. Everything else in the phase is independent, grouped, small fixes to `apps/next` UI and `apps/agent-worker` cleanup. + +**Tech Stack:** Convex (notifications table + queries/mutations, UseSend email), Next.js 16 (React 19) UI, agent-worker minor cleanup. + +**Depends on:** Phases 1–4 (notification emit points reference their statuses/events — job status transitions from Phase 1, connection `needs_reauth` from the Phase 4 webhook); polish tasks are independent and can run in any order after the notifications subsystem. + +## Global Constraints + +- **Backend tests:** `packages/backend/tests/unit` with `convex-test`. Run `cd packages/backend && bun run test:unit`. The harness pattern is in `packages/backend/tests/unit/harness.test.ts`: `convexTest(schema, import.meta.glob('../../convex/**/*.*s'))`, `createUser`/`authed(t, userId)` helpers, `t.withIdentity({ subject: '|session', issuer: 'https://convex.test' })`. Add new tests as `packages/backend/tests/unit/.test.ts`. +- **Next component tests:** jsdom + `@testing-library` in `apps/next/tests/component` (see `render.test.tsx`). Run `cd apps/next && bun run test:component`. +- **Codegen before typecheck:** after any `convex/schema.ts` or function-signature change run `cd packages/backend && bunx convex codegen` so `_generated` is current, then typecheck. +- **Conventional commits, one per task** (e.g. `feat(notifications): add notifications + notificationPreferences tables`). +- **Theme:** use existing semantic tokens only (`text-foreground`, `bg-card`, `text-muted-foreground`, `bg-primary/10`, etc.). No hard-coded hex colors. The codebase is already clean here. +- **Auth in Convex functions:** queries/mutations get the caller via `getRequiredUserId(ctx)` from `convex/model.ts`. Actions use `getAuthUserId(ctx)` (see `githubSync.ts`). + +--- + +## Task 1: `notifications` + `notificationPreferences` schema + +**Files:** +- `packages/backend/convex/schema.ts` (add two tables to `applicationTables`, ~after `threadMessages` / before `ignoredUpstreamChanges`, line ~820) + +**Interfaces:** +- **Produces** table `notifications`: + ```ts + notifications: defineTable({ + userId: v.id('users'), + kind: v.union( + v.literal('maintenance_thread'), + v.literal('agent_turn_finished'), + v.literal('agent_needs_input'), + v.literal('sync_failed'), + v.literal('connection_needs_reauth'), + ), + title: v.string(), + body: v.string(), + link: v.optional(v.string()), // in-app relative path, e.g. `/threads/` + spoonId: v.optional(v.id('spoons')), + threadId: v.optional(v.id('threads')), + readAt: v.optional(v.number()), + emailedAt: v.optional(v.number()), + createdAt: v.number(), + }) + .index('by_user', ['userId']) + .index('by_user_unread', ['userId', 'readAt']) + .index('by_user_created', ['userId', 'createdAt']), + ``` +- **Produces** table `notificationPreferences` (one row per user; unset field = default enabled): + ```ts + notificationPreferences: defineTable({ + userId: v.id('users'), + emailEnabled: v.optional(v.boolean()), // master email switch, default true + emailMaintenance: v.optional(v.boolean()), + emailAgentTurnFinished: v.optional(v.boolean()), + emailAgentNeedsInput: v.optional(v.boolean()), + emailSyncFailed: v.optional(v.boolean()), + emailConnectionReauth: v.optional(v.boolean()), + updatedAt: v.number(), + }).index('by_user', ['userId']), + ``` + +**Steps:** +- [ ] Write failing test `packages/backend/tests/unit/notifications.test.ts` that inserts a `notifications` row via `t.run(async (ctx) => ctx.db.insert('notifications', {...}))` and reads it back, plus one `notificationPreferences` row. FAIL: tables don't exist (schema validation error). +- [ ] Add both `defineTable` blocks to `applicationTables` in `schema.ts`. +- [ ] `cd packages/backend && bunx convex codegen`. +- [ ] Run `bun run test:unit` — PASS. +- [ ] Commit: `feat(notifications): add notifications + notificationPreferences tables`. + +--- + +## Task 2: emit helper, `emit` mutation wrapper, and UseSend email dispatch + +The single choke point all emit points call. A plain TS helper is callable directly inside mutations; an `internalMutation` wrapper lets Convex **actions** (e.g. `githubSync`) call it via `ctx.runMutation`. + +**Files:** +- `packages/backend/convex/notifications.ts` (new — helper + emit wrapper + preference read) +- `packages/backend/convex/notificationsNode.ts` (new, `'use node'` — email send action reusing `usesend-js`) +- Reference for UseSend usage: `packages/backend/convex/custom/auth/providers/usesend.ts` (env `USESEND_API_KEY`, `USESEND_URL`, `USESEND_FROM_EMAIL`; `new UseSend(apiKey, useSendUrl)`, `useSend.emails.send({from,to,subject,text,html})`). + +**Interfaces:** +- **Produces** exported type + helper (import `MutationCtx` from `./_generated/server`, `Id` from `./_generated/dataModel`): + ```ts + export type NotificationKind = + | 'maintenance_thread' | 'agent_turn_finished' | 'agent_needs_input' + | 'sync_failed' | 'connection_needs_reauth'; + + // Maps a kind to the notificationPreferences field that gates its email. + const EMAIL_PREF_FIELD: Record> = { + maintenance_thread: 'emailMaintenance', + agent_turn_finished: 'emailAgentTurnFinished', + agent_needs_input: 'emailAgentNeedsInput', + sync_failed: 'emailSyncFailed', + connection_needs_reauth: 'emailConnectionReauth', + }; + + export async function emitNotification( + ctx: MutationCtx, + args: { userId: Id<'users'>; kind: NotificationKind; title: string; body: string; + link?: string; spoonId?: Id<'spoons'>; threadId?: Id<'threads'> }, + ): Promise>; + ``` + Behavior: insert the `notifications` row (`readAt` unset, `createdAt: Date.now()`). Then load the user's `notificationPreferences` (index `by_user`); email is allowed when `emailEnabled !== false` **and** `prefs[EMAIL_PREF_FIELD[kind]] !== false` (unset = true). If allowed and the user has an `email`, `ctx.scheduler.runAfter(0, internal.notificationsNode.sendNotificationEmail, { notificationId })`. Return the id. +- **Produces** `internalMutation emit` (thin wrapper for actions) with the same args as the helper, body `return await emitNotification(ctx, args);`. +- **Produces** `internalMutation markEmailed({ notificationId })` → patches `emailedAt: Date.now()` (called by the email action). +- **Produces** `internalQuery getForEmail({ notificationId })` → returns `{ to: string | null; title; body; link }` by loading the notification + its user's email. +- **Produces** in `notificationsNode.ts`: `internalAction sendNotificationEmail({ notificationId })` — runs `getForEmail`; if no `to`, return; else send via UseSend (subject = title, text = body + optional link, minimal HTML mirroring `usesend.ts`); on success `ctx.runMutation(internal.notifications.markEmailed, { notificationId })`. Missing `USESEND_*` env → log and return (do **not** throw; email is best-effort). + +**Steps:** +- [ ] In `notifications.test.ts` add failing tests: (a) `emitNotification` inserts an unread row for the user; (b) with `emailEnabled: false` no email is scheduled (assert via a `t.run` count of scheduled functions is impractical — instead assert `emailedAt` stays undefined after `t.finishAllScheduledFunctions()` with UseSend env unset, i.e. row exists and is not emailed); (c) with prefs default (no row) and `USESEND_*` unset, `sendNotificationEmail` runs without throwing and leaves `emailedAt` undefined. FAIL: module doesn't exist. +- [ ] Implement `notifications.ts` (helper + `emit` + `markEmailed` + `getForEmail`) and `notificationsNode.ts` (`sendNotificationEmail`). +- [ ] `cd packages/backend && bunx convex codegen`. +- [ ] `bun run test:unit` — PASS. +- [ ] Commit: `feat(notifications): add emit helper + UseSend email dispatch`. + +--- + +## Task 3: wire emit points + +Call the helper/mutation at each event the spec lists. All are existing functions. + +**Files & exact emit points:** +- `packages/backend/convex/threads.ts` — `createMaintenanceThread` (internalMutation, ~line 407). After the **new-thread** insert (~line 475, not the dedup/existing branch), call `await emitNotification(ctx, { userId: args.ownerId, kind: 'maintenance_thread', title: args.title, body: args.summary, link: \`/threads/${threadId}\`, spoonId: args.spoonId, threadId })`. (Import `emitNotification` from `./notifications`.) +- `packages/backend/convex/agentJobs.ts` — `updateStatus` (mutation, ~line 1108). Inside the `if (job.threadId)` block, after the thread patch: when `args.status === 'changes_ready'` **or** `'draft_pr_opened'`, emit `kind: 'agent_turn_finished'` (title `Agent turn finished`, body = `args.summary ?? job.summary ?? ''`, link `/threads/${job.threadId}`, threadId, spoonId). Emit only on the transition into that status (guard `job.status !== args.status`). +- `packages/backend/convex/agentJobs.ts` — `applyMaintenanceDecision` (mutation, ~line 1423). When the computed `status === 'waiting_for_user'`, emit `kind: 'agent_needs_input'` (title `Agent needs your input`, body `args.summary`, link `/threads/${job.threadId}`). +- `packages/backend/convex/githubSync.ts` — this is a `'use node'` action, so use `await ctx.runMutation(internal.notifications.emit, {...})`. Two sites: + - Terminal `catch` of `refreshOwnedSpoon` (~line 291, the outer catch that sets `syncStatus: 'error'`): emit `kind: 'sync_failed'` (title `Sync failed for ${spoon.name}`, body `message`, link `/spoons/${spoonId}`, spoonId). `spoon.name` is in scope. + - The maintenance-thread branches already call `createMaintenanceThread` (which now emits) — do **not** double-emit there. +- **Connection needs re-auth** — the Phase 4 webhook sets `gitConnections.status = 'needs_reauth' | 'revoked'`. Emit from that mutation (created in Phase 4). If that mutation does not yet exist in this branch, add a `// TODO(phase4): emit connection_needs_reauth here` marker at `packages/backend/convex/github.ts` near the connection status writes (~line 181) and emit from the first mutation that patches a connection to `needs_reauth`. Kind `connection_needs_reauth`, title `GitHub connection needs re-authorization`, body `Reconnect your GitHub account to keep syncing.`, link `/settings/integrations`. + +**Interfaces:** +- **Consumes** `emitNotification` (mutations) / `internal.notifications.emit` (actions) from Task 2. + +**Steps:** +- [ ] Add failing tests in `notifications.test.ts`: (a) calling `createMaintenanceThread` (via `t.run`/internal) for a fresh upstream produces one `maintenance_thread` notification for the owner; a second dedup call does **not** add another. (b) `updateStatus` to `changes_ready` on a claimed job with a thread produces one `agent_turn_finished` notification; a repeat call at the same status does not add a second. Use the harness helpers to seed a spoon/thread/job like existing agentJobs tests. FAIL: no notifications produced. +- [ ] Add the emit calls at the five points above (imports as noted). +- [ ] `cd packages/backend && bunx convex codegen`; `bun run test:unit` — PASS. +- [ ] Commit: `feat(notifications): emit at maintenance/turn/sync/reauth events`. + +--- + +## Task 4: inbox query + mark-read mutations + +**Files:** +- `packages/backend/convex/notifications.ts` (add public query/mutations) + +**Interfaces:** +- **Produces** `query listMine({ limit?: number })` → `Doc<'notifications'>[]` for `getRequiredUserId(ctx)`, index `by_user_created` desc, `take(args.limit ?? 30)`. +- **Produces** `query unreadCount({})` → `number`. Use index `by_user_unread` filtered to `readAt === undefined` (`.withIndex('by_user_unread', q => q.eq('userId', uid).eq('readAt', undefined))`), `.take(100)` then `.length` capped display (return raw count via `.collect().length` bounded by take(100)). +- **Produces** `mutation markRead({ notificationId })` → ensures the row's `userId === getRequiredUserId(ctx)` (else `ConvexError('Notification not found.')`), patch `readAt: Date.now()` if unset. +- **Produces** `mutation markAllRead({})` → iterate `by_user_unread` unread rows for the caller, patch each `readAt`. + +**Steps:** +- [ ] Failing tests in `notifications.test.ts`: seed 3 notifications for user A + 1 for user B via `t.run`; `authed(t, A).query(api.notifications.listMine, {})` returns exactly A's 3 newest-first; `unreadCount` returns 3; `markRead` on one drops `unreadCount` to 2 and rejects B's row for A; `markAllRead` zeroes it. FAIL: functions missing. +- [ ] Implement the query + mutations. +- [ ] `bunx convex codegen`; `bun run test:unit` — PASS. +- [ ] Commit: `feat(notifications): inbox query + mark-read mutations`. + +--- + +## Task 5: header notification bell + +**Files:** +- `apps/next/src/components/layout/header/controls/notification-bell.tsx` (new) +- `apps/next/src/components/layout/header/controls/index.tsx` (or wherever `Controls` is composed — confirm by reading `apps/next/src/components/layout/header/controls/`) — mount `` before ``, only when authenticated (use `useConvexAuth().isAuthenticated`, matching `header/index.tsx`). + +**Interfaces:** +- **Consumes** `api.notifications.unreadCount`, `api.notifications.listMine`, `api.notifications.markRead`, `api.notifications.markAllRead`. +- Client component (`'use client'`). Uses `useQuery`/`useMutation` from `convex/react`. Bell icon from `lucide-react` (`Bell`). Wrap in the existing `@spoon/ui` `DropdownMenu` primitives (same set `AvatarDropdown.tsx` imports). Unread badge: small `bg-primary text-primary-foreground` count pill when `unreadCount > 0` (show `9+` above 9). Each item is a `Link` to `notification.link ?? '#'` that calls `markRead({ notificationId })` on click; a "Mark all read" action calls `markAllRead`. Empty state: `No notifications yet.` Relative time via existing date util if present (grep `formatDistanceToNow`/`date-fns` under `apps/next/src`); otherwise a minimal inline formatter. + +**Steps:** +- [ ] Failing component test `apps/next/tests/component/notification-bell.test.tsx`: render `` with a mocked Convex client (follow `render.test.tsx` for the provider/mock pattern) returning `unreadCount: 2` and two notifications; assert the badge shows `2` and both titles render; clicking one invokes the `markRead` mock. FAIL: component missing. +- [ ] Implement `notification-bell.tsx`; mount it in the header controls. +- [ ] `cd apps/next && bun run test:component` — PASS. +- [ ] Manual verify: `bun run dev`, sign in, confirm the bell renders and the badge updates reactively after seeding a notification. Document the check. +- [ ] Commit: `feat(notifications): header notification bell`. + +--- + +## Task 6: Settings → Notifications preferences + +**Files:** +- `packages/backend/convex/notifications.ts` (add `getPreferences` query + `updatePreferences` mutation) +- `apps/next/src/app/(app)/settings/notifications/page.tsx` (new) +- `apps/next/src/components/settings/notification-preferences-panel.tsx` (new) +- `apps/next/src/app/(app)/settings/layout.tsx` (add nav item `{ href: '/settings/notifications', label: 'Notifications', icon: Bell }` — import `Bell` from `lucide-react`; insert after Dotfiles) + +**Interfaces:** +- **Produces** `query getPreferences({})` → the caller's `notificationPreferences` row or a defaults object `{ emailEnabled: true, emailMaintenance: true, emailAgentTurnFinished: true, emailAgentNeedsInput: true, emailSyncFailed: true, emailConnectionReauth: true }` when no row (unset = enabled). +- **Produces** `mutation updatePreferences({ emailEnabled?, emailMaintenance?, emailAgentTurnFinished?, emailAgentNeedsInput?, emailSyncFailed?, emailConnectionReauth? })` → upsert the caller's row (patch existing or insert), set `updatedAt`. +- Panel: `'use client'`, `useQuery(api.notifications.getPreferences)` + `useMutation(api.notifications.updatePreferences)`. **Seed local state from the query using the hydrated-flag pattern** (Task 9 reference) so late-resolving prefs don't clobber. Each toggle is a `Switch`/`Checkbox` from `@spoon/ui`; on change call the mutation in a `try/catch` with `toast.success('Preferences saved.')` on resolve and `toast.error(...)` on failure (see Task 8 pattern). Master `emailEnabled` disables the per-kind toggles when off. + +**Steps:** +- [ ] Failing backend test in `notifications.test.ts`: `getPreferences` returns all-true defaults with no row; `updatePreferences({ emailSyncFailed: false })` persists and a subsequent `getPreferences` reflects it; the gating in Task 2 respects it (emit `sync_failed` with pref false → no email scheduled / `emailedAt` stays unset). FAIL. +- [ ] Implement `getPreferences` + `updatePreferences`; `bunx convex codegen`; `bun run test:unit` — PASS. +- [ ] Build the page + panel; add the settings nav item. +- [ ] Manual verify: toggle a preference, reload, confirm it persists. Document the check. +- [ ] Commit: `feat(notifications): settings notification preferences`. + +--- + +## Task 7: loading skeletons for dashboard / spoons / threads + +Kills the zero-state flash where `useQuery(...) ?? []` renders an empty state before data resolves. + +**Files (read each first for exact `useQuery` lines):** +- `apps/next/src/app/(app)/dashboard/page.tsx` (~lines 14–16: `?? []` on `listMineWithState`, `syncRuns.listRecent`, `threads.listMine`) +- `apps/next/src/app/(app)/spoons/page.tsx` (~line 35 `?? []`) +- `apps/next/src/app/(app)/threads/page.tsx` (`?? []` on list) +- New: `apps/next/src/components/ui/list-skeleton.tsx` (or reuse an existing `Skeleton` from `@spoon/ui` if present — grep `Skeleton` under `packages/ui`/`apps/next` first). + +**Interfaces:** +- **Consumes** existing Convex list queries. Change pattern: keep the raw query result **without** `?? []`, so `undefined` (loading) is distinguishable from `[]` (empty). Render a skeleton block while `data === undefined`; render the real empty-state only when `data.length === 0`. + +**Steps:** +- [ ] Component test `apps/next/tests/component/dashboard-loading.test.tsx` (or extend `render.test.tsx`): with the query mock returning `undefined`, assert a skeleton (`data-testid="list-skeleton"` / `role="status"`) is shown and the empty-state copy is **not**; with `[]`, assert the empty state shows and no skeleton. FAIL. +- [ ] Add the skeleton component; update the three pages to branch on `undefined` vs `[]`. Give the skeleton `role="status"` + `aria-label="Loading"`. +- [ ] `bun run test:component` — PASS. +- [ ] Commit: `fix(ui): loading skeletons for dashboard/spoons/threads`. + +--- + +## Task 8: fire-and-forget mutation error handling across panels + +Wrap unhandled mutation calls in `try/catch`; only `toast.success` after resolve; `toast.error` on failure. Model on the already-correct `importAll`/`newFile` in `dotfiles-manager.tsx` (lines 172–181, 221–230). + +**Files & exact sites:** +- `apps/next/src/components/settings/dotfiles/dotfiles-manager.tsx` — `saveSelected` (lines 161–170: wrap `await putFile(...)` in try/catch; `toast.success('Saved.')` inside try after resolve, `toast.error(...)` in catch) and `deleteSelected` (lines 232–239: same around `await removeFile(...)`). +- `apps/next/src/components/integrations/ai-provider-profiles-panel.tsx` (~lines 235–240 and 282–285: the save + delete mutations). +- `apps/next/src/components/spoons/spoon-clone-panel.tsx` (~lines 225–228). +- `apps/next/src/components/spoons/spoon-secrets-form.tsx` (~lines 312–315; **and** the sequential import at ~134–141 — wrap each iteration so one failure surfaces via `toast.error` and doesn't silently swallow the rest; report count of successes). + +**Interfaces:** no signature changes; behavior only. Error message: `error instanceof Error ? error.message : ' failed.'`. + +**Steps:** +- [ ] Read each file at the cited lines to capture exact `await (...)` calls. +- [ ] Component test `apps/next/tests/component/dotfiles-error.test.tsx`: render the dotfiles manager with a `putFile` mock that rejects; trigger save; assert `toast.error` called and `toast.success` **not** called. (Mock `sonner`'s `toast`.) FAIL (currently unconditional success). +- [ ] Apply try/catch + resolve-gated success to all sites above. +- [ ] `bun run test:component` — PASS. Manual grep to confirm no remaining unconditional `toast.success(` sits before its `await` in these files. +- [ ] Commit: `fix(ui): error-handle fire-and-forget mutations in settings panels`. + +--- + +## Task 9: settings/thread forms re-sync on loaded record id + +`useState` seeded once from a late-resolving Convex prop can save defaults over real config. Fix with the **hydrated-flag** pattern already in `dotfiles-manager.tsx` RepoPanel (lines 383–394): a `hydrated` boolean, an effect that seeds from the loaded record then sets `hydrated`, keyed on the record's `_id`. + +**Files:** +- `apps/next/src/components/spoons/spoon-agent-settings-form.tsx` (~lines 61–119: `useState` seeded from the settings query) +- `apps/next/src/components/threads/thread-workspace-form.tsx` (~lines 57–74) + +**Interfaces:** +- Pattern (per form): `const [hydrated, setHydrated] = useState(false);` + `useEffect(() => { if (!record || hydrated) return; /* seed each useState from record fields */ setHydrated(true); }, [record, hydrated]);`. If the mounting parent can swap records, prefer remounting via `key={record._id}` on the form in the parent **or** reset `hydrated` when `record._id` changes (`useEffect(() => setHydrated(false), [record?._id])`). Choose keyed remount if the parent already has the id; otherwise the id-change effect. + +**Steps:** +- [ ] Component test `apps/next/tests/component/form-resync.test.tsx`: render `spoon-agent-settings-form` first with the settings prop `undefined`, then rerender with a resolved record having non-default values; assert the inputs reflect the loaded values (not the initial defaults). FAIL (state seeded once from undefined). +- [ ] Apply the hydrated-flag (or keyed-remount) pattern to both forms. +- [ ] `bun run test:component` — PASS. +- [ ] Commit: `fix(ui): re-sync settings/thread forms on loaded record`. + +--- + +## Task 10: sign-in + forgot-password copy & input-retention fixes + +**Files & exact fixes:** +- `apps/next/src/app/(auth)/sign-in/page.tsx` + - Remove `signInForm.reset()` from the `finally` on failed sign-in (line 140) — keep the email/password on error. Only reset on success (move a `.reset()` into the success branch if desired, or drop it). Same for `signUpForm.reset()` (line 163) and `verifyEmailForm.reset()` (line 182): do not wipe input in `finally`; reset only after a successful transition. + - `pendingText='Signing Up...'` on the **Verify Email** button (line 235) → `pendingText='Verifying...'`. Leave the actual sign-up button (line 443) as `Signing Up...`. + - Typo `Confirm Passsword` (line 426) → `Confirm Password`. +- `apps/next/src/app/(auth)/forgot-password/page.tsx` + - `Please enter the one-time password sent to your phone.` (~line 232–233) → `...sent to your email.` + - `Confirm Passsword` typo (~line 268) → `Confirm Password`. + +**Steps:** +- [ ] Component test `apps/next/tests/component/sign-in-retention.test.tsx`: render the sign-in form, type an email + password, make the `signIn` mock reject, submit; assert the email/password inputs still hold the typed values after the rejection settles. FAIL (reset in finally wipes them). +- [ ] Apply the input-retention + copy fixes above. +- [ ] `bun run test:component` — PASS. Manual grep: `grep -rn "Passsword\|sent to your phone\|Signing Up" apps/next/src/app/(auth)` returns only the intentional sign-up button. +- [ ] Commit: `fix(auth): retain input on failure + copy fixes`. + +--- + +## Task 11: profile page `'use server'` removal + AvatarDropdown fixes + +**Files:** +- `apps/next/src/app/(app)/settings/profile/page.tsx` (line 1: remove the stray `'use server';` — this is an async server component using `preloadQuery`; `'use server'` marks it as a Server Actions module and is wrong here). +- `apps/next/src/components/layout/header/controls/AvatarDropdown.tsx` + - Line 73: `Link href='/profile'` → `href='/settings/profile'` (the `/profile` route is being deleted in Task 14). + - Lines 64 & 67: empty-string name falls through `??`. Change `user?.name ?? user?.email` and `user.name?.trim() ?? user.email?.trim()` to use `||` so an empty/whitespace name falls back to email: `(user?.name?.trim() || user?.email)` and label `{user.name?.trim() || user.email?.trim()}`. + +**Steps:** +- [ ] Manual verification (test impractical — server component + dropdown wiring): + - Before: `head -1 apps/next/src/app/(app)/settings/profile/page.tsx` shows `'use server';`. After: first line is `import ...`; `grep -n "use server" apps/next/src/app/(app)/settings/profile/page.tsx` returns nothing. + - Before: `grep -n "href='/profile'" .../AvatarDropdown.tsx` matches. After: it matches `href='/settings/profile'` and no bare `/profile`. + - Before: `grep -n "?? user" .../AvatarDropdown.tsx` matches lines 64/67. After: those use `|| user`. +- [ ] Apply the three edits. +- [ ] `cd apps/next && bun run typecheck` (or `bun run build` if no dedicated typecheck script) — passes. +- [ ] Commit: `fix(ui): profile page directive + avatar dropdown links/fallbacks`. + +--- + +## Task 12: accessibility — remove `role='link'` with nested interactive elements + +A `role='link'` container wrapping real ``/`` children is an invalid nested-interactive pattern. + +**Files:** +- `apps/next/src/app/(app)/spoons/page.tsx` (~lines 112–124: `TableRow role='link'` containing links) +- `apps/next/src/app/(app)/threads/page.tsx` (~lines 317–329: `Card role='link'` containing links) + +**Interfaces:** +- Pattern: drop the `role='link'` (and any `tabIndex`/`onKeyDown` that simulated link behavior on the container). Make the row/card navigable via a single primary `Link` — either a full-row `Link` wrapping the content, or a "stretched link" overlay (`` on a `relative` container) so nested action links remain individually clickable. Preserve existing hover styles. + +**Steps:** +- [ ] Read both cited blocks to capture current structure. +- [ ] Component test `apps/next/tests/component/list-a11y.test.tsx`: render the spoons table (mock data) and assert no element has `role="link"` and the row exposes an accessible link to the spoon detail. FAIL (role='link' present). +- [ ] Refactor both to the stretched-link (or single wrapping Link) pattern; remove `role='link'` and simulated key handlers. +- [ ] `bun run test:component` — PASS. Manual grep: `grep -rn "role='link'" apps/next/src/app` returns nothing. +- [ ] Commit: `fix(a11y): remove nested-interactive role='link' on spoon/thread lists`. + +--- + +## Task 13: agent workspace UX polish + +Group the Phase-5 workspace items. Read each cited range first. + +**Files & fixes:** +- `apps/next/src/components/agent-workspace/agent-workspace-shell.tsx` + - **Diff/tree load race** (~124–138): guard out-of-order responses. Track a per-request sequence number (or `AbortController`); ignore a response whose request id is stale so a slower earlier fetch can't overwrite newer data. + - **Editor buffer not refreshed when agent edits the open file** (~252–265): when a workspace change (`agentWorkspaceChanges`) arrives for the currently open file: if the buffer is **not dirty**, reload the file content into the editor; if dirty, set a conflict flag (badge/toast "This file changed on disk") rather than silently overwriting. + - **Recovery panel over-triggers** (~503–557): require **N consecutive** status/health failures (e.g. `const RECOVERY_THRESHOLD = 3`) before showing the recovery panel; reset the counter on any success. One transient error must not trip it. + - **Mobile layout** (~476 `min-h-[720px]`, ~562 `grid-cols-1`): replace the fixed `min-h-[720px]` with a viewport-relative min-height (e.g. `min-h-[70vh]`/`min-h-[calc(100dvh-…)]`) and ensure the single-column mobile layout collapses the file tree (collapsible/`Sheet` or a toggle) instead of stacking full-height panels. +- `apps/next/src/components/agent-workspace/agent-thread.tsx` + - **Auto-scroll fights the user** (~114–132): only auto-scroll to newest when the user is already near the bottom (compute `scrollHeight - scrollTop - clientHeight < threshold`, e.g. 80px). If scrolled up, don't yank; optionally show a "jump to latest" affordance. +- `apps/next/src/components/agent-workspace/code-editor.tsx` + - **editorRef not cleared on unmount** (~53, ~157): in the mount effect's cleanup, dispose the editor and set `editorRef.current = null` to avoid a stale ref / leak across remounts. + +**Steps:** +- [ ] Read each cited range. +- [ ] Component test `apps/next/tests/component/agent-thread-scroll.test.tsx`: simulate the messages list scrolled up (mock `scrollTop`/`scrollHeight`/`clientHeight`), append a new message, assert `scrollTo`/`scrollIntoView` is **not** called; then with near-bottom, assert it **is**. FAIL. +- [ ] Add a unit-style test for the recovery threshold: extract the consecutive-failure decision into a small pure helper (`shouldShowRecovery(consecutiveFailures, threshold)`) and test it, or assert via a component test that 1–2 failures don't render the recovery panel and the 3rd does. FAIL. +- [ ] Implement all fixes above (sequencing guard, dirty-aware buffer refresh, consecutive-failure threshold, mobile min-height + collapsible tree, near-bottom auto-scroll, editorRef cleanup). +- [ ] `bun run test:component` — PASS. +- [ ] Manual verify in `bun run dev`: open a workspace, scroll the thread up while the agent streams (no yank), trigger an agent edit of an open clean file (buffer refreshes), resize to mobile width (tree collapses). Document the checks. +- [ ] Commit: `fix(workspace): scroll/diff-race/recovery/editor/mobile polish`. + +--- + +## Task 14: dead code removal + worker `redact.ts` / `env.ts` cleanup + +Group all removals + the two worker cleanups. **Note:** `redact.ts` and `env.ts` items are Phase-5-scoped per the audit; some `env.ts` vars may already be gone after Phase 2 — only remove ones that are still present **and** unreferenced. + +**Files & actions:** +- Delete route dirs: `apps/next/src/app/(app)/updates/`, `apps/next/src/app/(app)/agents/`, `apps/next/src/app/(app)/settings/ai/`, `apps/next/src/app/(auth)/profile/`. +- Delete component: `apps/next/src/components/landing/tech-stack.tsx` (grep first for imports; remove any import/usage — likely in a landing page). +- Delete empty dirs: `apps/next/src/components/agents/`, `apps/next/src/components/updates/` (only if empty after the above). +- `apps/next/src/proxy.ts` (lines 11–12): remove `'/updates(.*)'`, `'/agents(.*)'`, and `'/profile(.*)'` from `isProtectedRoute` (those routes no longer exist). +- `apps/agent-worker/src/redact.ts` (lines 1–17): the first three patterns (`ghs_…`, `github_pat_…`, `sk-…`) have **no** capture group, so the shared `'$1=[redacted]'` replacement emits a literal `$1`. Fix: give each pattern its own replacement — group-less patterns replace with `'[redacted]'`; the `(client_secret|…)=(…)` pattern keeps `'$1=[redacted]'`. Restructure `secretPatterns` to `{ pattern, replacement }` objects and map accordingly. +- `apps/agent-worker/src/env.ts`: remove `terminalImage` and `terminalIdleMs` (and any other audit-listed unused vars — `containerAccess`, `maxConcurrentJobs`) **only if** a repo-wide grep shows no remaining references. Grep before deleting each. + +**Steps:** +- [ ] Grep for references to each route/component before deleting: `grep -rn "tech-stack\|/updates\|/agents\|/settings/ai\|(auth)/profile\|href='/profile'" apps/next/src`. Confirm nothing (except the AvatarDropdown link already fixed in Task 11) still points at them. +- [ ] Failing worker test `apps/agent-worker/src/redact.test.ts` (vitest): `createRedactor([])('token ghs_ABC123 sk-xyz')` must contain `[redacted]` and **not** contain the literal `$1`; the `api_key=secret` case still yields `api_key=[redacted]`. FAIL (current output has `$1`). +- [ ] Fix `redact.ts` per-pattern replacement; run the worker test (`cd apps/agent-worker && bunx vitest run src/redact.test.ts` or the repo's worker test command) — PASS. +- [ ] Delete the routes/components/empty dirs; trim `proxy.ts`; remove verified-unused `env.ts` vars (grep each first). +- [ ] `cd apps/next && bun run build` (or typecheck) passes with no missing-import errors; `cd apps/agent-worker && ` passes. +- [ ] Manual grep confirms removals: `find apps/next/src/app -type d \( -name updates -o -name agents -o -name ai -o -name profile \)` returns nothing under the deleted paths. +- [ ] Commit: `chore: remove dead routes/components + fix redact/env cleanup`. + +--- + +## Task 15: docs rewrite for the box-first model + +**Files:** +- `README.md` +- `docs/*.md` (enumerate: `ls docs/*.md` and any relevant subdirs; the spec calls out server-deploy notes) + +**Interfaces:** documentation only. Rewrite the per-job-container narrative to the Phase-2 **one long-running per-user box** model: +- Each user owns a persistent Fedora container (`spoon-box-*`) with a persistent home (`homes//`, `~/Code`), dotfiles, terminal, and agent CLIs; the legacy per-job OpenCode container path is gone. +- Three agent runtimes exec into the box: Codex, OpenCode, **and Claude Code** (note the new Anthropic provider-profile kind). +- **GitHub webhooks:** document the webhook URL (Convex `httpAction`) and `GITHUB_APP_WEBHOOK_SECRET`; hourly cron remains a fallback. +- **New env vars:** list the box/terminal/notification/webhook env (`SPOON_AGENT_BOX_IDLE_MS`, `USESEND_API_KEY`/`USESEND_URL`/`USESEND_FROM_EMAIL`, `GITHUB_APP_WEBHOOK_SECRET`, Claude Code auth) and drop removed ones (`SPOON_AGENT_TERMINAL_IMAGE`, `SPOON_AGENT_TERMINAL_IDLE_MS`, and other Task-14-removed vars). +- Add a short **Notifications** section (in-app bell + email via UseSend, per-user preferences, web push out of scope). + +**Steps:** +- [ ] `ls docs/*.md docs/**/*.md README.md`; read each to find per-job-container language (`grep -rn "per-job\|spoon-agent-job\|container per\|host_port" README.md docs`). +- [ ] Rewrite the affected sections per the bullets above. No stale references to deleted routes/env. +- [ ] Manual verification: `grep -rn "spoon-agent-job\|per-job container" README.md docs` returns nothing; the deploy doc lists the webhook URL + `GITHUB_APP_WEBHOOK_SECRET` + `USESEND_*` + Claude Code; a Notifications section exists. +- [ ] Commit: `docs: rewrite for box-first model + webhooks + notifications`. + +--- + +## Manual verification checklist (end of phase) + +- [ ] Trigger a maintenance thread (diverged spoon) → in-app bell increments and (with email pref on + `USESEND_*` set) an email arrives. +- [ ] Finish an agent turn (`changes_ready`) → `agent_turn_finished` notification; a maintenance decision needing approval → `agent_needs_input`. +- [ ] Force a sync failure → `sync_failed` notification. +- [ ] Toggle each preference off → the corresponding email stops while the in-app row still appears. +- [ ] Mark-read / mark-all-read update the badge reactively. +- [ ] Dashboard/spoons/threads show skeletons (no empty-state flash) on cold load. +- [ ] Sign-in keeps input on failure; auth copy fixed; forms re-sync from loaded records. +- [ ] Deleted routes 404; app builds; worker `redact.ts` no longer emits `$1`.