Files
spoon/docs/superpowers/plans/2026-07-10-spoon-phase4-sync-correctness.md
Gabriel Brown 8aa9140191 docs: multi-phase implementation plan (63 tasks across 5 phases)
Phase 1 Stabilize (13), Phase 2 Runtime unification (12),
Phase 3 Dev-box surface (11), Phase 4 Sync correctness (12),
Phase 5 Notifications & polish (15), plus index.
2026-07-10 15:18:51 -04:00

622 lines
41 KiB
Markdown
Raw Permalink Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# 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 13.
## 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=<hex>`, 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 8289).
- `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<GitHubCompareSummary> => {
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<boolean>`
- 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<boolean> => {
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: <count> }`. (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 `<CONVEX_SITE_URL>/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' ? (
<div
className={
connection.status === 'revoked'
? 'rounded-md border border-red-500/40 bg-red-500/10 p-3 text-sm text-red-600'
: 'rounded-md border border-amber-500/40 bg-amber-500/10 p-3 text-sm text-amber-600'
}
>
<p className='font-medium'>
{connection.status === 'revoked'
? 'GitHub App installation removed'
: 'GitHub access needs re-authorization'}
</p>
<p className='mt-1'>
{connection.status === 'revoked'
? 'Reinstall the app to resume syncing.'
: 'Reconnect the app to resume syncing.'}
</p>
{installUrl ? (
<a className='mt-2 inline-block underline' href={installUrl} target='_blank' rel='noreferrer'>
Reconnect GitHub App
</a>
) : null}
</div>
) : 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 `<CONVEX_SITE_URL>/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.