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.
622 lines
41 KiB
Markdown
622 lines
41 KiB
Markdown
# 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=<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 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<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.
|