diff --git a/client/src/lib/fullAppQaSurface.test.ts b/client/src/lib/fullAppQaSurface.test.ts index 2f00f95..dab0ded 100644 --- a/client/src/lib/fullAppQaSurface.test.ts +++ b/client/src/lib/fullAppQaSurface.test.ts @@ -443,7 +443,7 @@ test("issues page keeps the QA-tested issue monitor and work surface wired", asy assertHasExpression(sourceFile, "stale issue helper", /\bisStaleIssue\b/); assertHasExpression(sourceFile, "issue detail refresh", /\bselectedIssueDetail\b/); assertHasExpression(sourceFile, "issue UI polling uses tuning interval", /\bgetUiPollIntervalMs\(config\)/); - assertHasExpression(sourceFile, "issue coverage pauses during GitHub throttling", /enabled: config !== undefined && !globalDrainMode && !isGitHubThrottled/); + assertHasExpression(sourceFile, "issue coverage stays available during GitHub throttling", /queryKey: \["\/api\/issues\/coverage"\][\s\S]*?enabled: config !== undefined && !globalDrainMode,/); assertHasExpression(sourceFile, "issue PR mergeability", /\bworkPrMergeable\b/); assertHasExpression(sourceFile, "issue queue helper", /\bbuildQueueStatusIndex\b/); assertHasExpression(sourceFile, "issue queue badge", /\bQueueStatusBadge\b/); diff --git a/client/src/pages/issues.tsx b/client/src/pages/issues.tsx index 6906dac..1301bdd 100644 --- a/client/src/pages/issues.tsx +++ b/client/src/pages/issues.tsx @@ -759,7 +759,7 @@ function IssuesPage() { const isGitHubThrottled = githubRateLimit?.limited === true; const { data: issueCoverage = [] } = useQuery({ queryKey: ["/api/issues/coverage"], - enabled: config !== undefined && !globalDrainMode && !isGitHubThrottled, + enabled: config !== undefined && !globalDrainMode, refetchInterval: uiPollIntervalMs, }); const queueStatusById = useMemo(() => buildQueueStatusIndex(activities), [activities]); diff --git a/docs/plans/github-rate-limit-cache.md b/docs/plans/github-rate-limit-cache.md new file mode 100644 index 0000000..66f1c51 --- /dev/null +++ b/docs/plans/github-rate-limit-cache.md @@ -0,0 +1,253 @@ +# GitHub rate-limit: stop paying for the same data twice + +Cut GitHub API spend on the paths we already identified. Keep product behavior (coverage numbers, PR babysitting, feedback, CI, releases). Do not add a generic Octokit HTTP cache. + +## Goal + +Quiet watched repos and dashboard polls should consume **zero primary Search budget** and **near-zero extra REST/GraphQL** after the first successful fetch. Restarts must not re-paginate every open PR list. Feedback GraphQL and CI-by-SHA must not rerun when the underlying data cannot have changed. + +Success looks like: + +- `/api/issues/coverage` never calls `GET /search/issues` +- Issues page still shows `synced N / GitHub M` from last sweep +- After process restart, a quiet repo’s open-PR list is a 304, not a full pagination +- Changed issue lists cost one page-1 GET, not two +- Unchanged PR comment lists skip GraphQL entirely +- Settled `owner/repo@sha` CI is fetched once per process +- `listMergedPullsSince` stops paging once `updated_at` is older than the since bound + +## Non-goals + +- Octokit plugin-cache / generic HTTP cache (stale CI and mergeable) +- Token rotation or extra PATs +- Changing `pollIntervalMs` as the fix +- Caching writes, diffs, timelines, or file contents +- Persisting CI snapshots to SQLite (in-memory by SHA is enough) + +## Delivery + +Work from a **fresh worktree of origin/main**, not `main` itself. + +Five stacked PRs so each is reviewable and each has tests. One worktree, branch stack `fix/github-quota-coverage` → `…-pr-etags` → `…-feedback` → `…-ci-sha` → `…-merged-pulls` (or Graphite if available). + +Copy this plan to `docs/plans/github-rate-limit-cache.md` in PR 1 so it is in-repo. + +TDD on every behavior change (`test.md`). File headers match existing modules (no new copyright banners on files that do not have them). + +--- + +## PR 1 — Coverage: no Search on UI poll + +**Why first:** Search is 30 req/min. The Issues page refetches `/api/issues/coverage` on `getUiPollIntervalMs`, and each refetch hits Search once per watched repo. + +**Behavior** + +- `listIssueCoverage()` reads SQLite only: local `syncedOpenCount` + persisted `githubOpenCount` + `lastSyncedAt`. No Octokit. +- During the issue sweep (`syncStoredIssuesStep`), fetch open-issue count **once per repo that actually syncs** via GraphQL: + +```graphql +query($owner: String!, $name: String!) { + repository(owner: $owner, name: $name) { + issues(states: OPEN) { totalCount } + } +} +``` + +That is ~1 GraphQL point vs Search. Persist on `repo_sync_state` (`kind = 'issues'`). +- 304 probe path: **do not refetch** the count; previous value is still valid. +- Full sweep / successful page sync: write the new count. +- GraphQL count failure: leave previous count; coverage shows last known (`null` only if never observed). +- Issues page: keep polling `/api/issues/coverage` (now a local read). **Enable it while GitHub is throttled** so last-known counts still render. Update `fullAppQaSurface.test.ts` (today it asserts `!isGitHubThrottled`). + +**Schema** + +- `repo_sync_state.github_open_count INTEGER` via `ensureColumn` +- `RepoSyncState.githubOpenCount: number | null` +- `upsertRepoSyncState` accepts optional `githubOpenCount` +- Persist across reopen (`storage.test.ts` already has the etag reopen pattern) + +**Files** + +- `server/storage.ts`, `memoryStorage.ts`, `sqliteStorage.ts`, `storage.test.ts` +- `server/github.ts` — `fetchOpenIssueCount(octokit, repo)` +- `server/appRuntime.ts` — sweep writes count; `listIssueCoverage` does not call GitHub +- `server/appRuntime.test.ts` — coverage + 304 does not GraphQL; changed list does +- `client/src/pages/issues.tsx` — drop throttle gate on coverage query +- `client/src/lib/fullAppQaSurface.test.ts` + +**Tests (red first)** + +- `listIssueCoverage` with a stub octokit that throws if `request`/`graphql` is called — still returns persisted counts +- Sweep 200 path stores `githubOpenCount` from GraphQL `totalCount` +- Sweep 304 path does not call GraphQL and keeps the old count +- Sqlite reopen preserves `githubOpenCount` +- Issues page coverage query stays enabled when `githubRateLimit.limited === true` + +**Verify:** `npx tsx --test server/appRuntime.test.ts server/storage.test.ts server/github.test.ts client/src/lib/fullAppQaSurface.test.ts` and `npm run check` + +--- + +## PR 2 — Persist PR list cache + reuse issue page 1 + +**Why:** Issue etags already survive restart; PR etags do not. On restart the babysitter 304s then **falls back to a full `listOpenPullsForRepo`** because the in-memory list is gone (`babysitter.ts` ~2612–2615). Issue probe also downloads page 1 twice on a cache miss. + +### 2a. PR list: persist etag **and** summaries + +Persisting etag alone is not enough. 304 without a list still paginates. + +- Add `github_etags.payload TEXT` via `ensureColumn` +- `getGithubEtag` stays as-is; add `getGithubEtagRecord(url) -> { etag, payload } | undefined` and `setGithubEtag(url, etag, payload?)` +- Key remains `prs:open:${repoSlug}` (use the same style as `issues:open:${repoSlug}`) +- On 200: write etag + `JSON.stringify(pulls)` and keep the in-memory map +- On 304: if memory miss, parse payload and reuse; **never** call `listOpenPullsForRepo` for a 304 +- If 304 and payload missing (upgrade / first persist): treat as miss and do the conditional 200 path (no `If-None-Match`), then persist. Do not add a second uncached pagination helper call if the conditional already returned 304 with no payload — drop the etag and fetch without If-None-Match once. + +Babysitter `prListCache` remains the hot path. Storage is the restart path. + +### 2b. Issues: one page-1 GET + +Replace `probeRepoIssuesChanged` + first `listOpenIssuesForRepo` with a pulls-style helper: + +`listOpenIssuesForRepoConditional(octokit, repo, cachedEtag, { offset, limit })` + +- Page 1 sends `If-None-Match` +- 304 → `{ notModified: true }` (unchanged) +- 200 → `{ notModified: false, etag, items, hasMore }` using the **same** page already fetched +- Further pages (full sweep) stay uncached, as today + +`syncStoredIssuesStep` uses that result as offset 0. Existing 304 test (`listForRepoCalls === 1`) stays. Change the 200 test to assert **one** `listForRepo` call, not two. + +Keep `probeRepoIssuesChanged` as a thin wrapper **only if** something else calls it; otherwise delete it and update `github.test.ts`. + +**Files** + +- `server/github.ts`, `github.test.ts` +- `server/babysitter.ts`, `babysitter.test.ts` (restart: 304 + persisted payload, zero `listOpenPullsForRepo`) +- `server/appRuntime.ts`, `appRuntime.test.ts` +- `server/storage.ts`, `sqliteStorage.ts`, `memoryStorage.ts`, `storage.test.ts` + +**Tests** + +- Sqlite stores/reloads etag+payload +- Babysitter 304 with empty memory uses persisted pulls; `listOpenPullsForRepo` not called +- Conditional issue list 200 returns items+etag in one GET +- `syncRepos` on a changed issue list: `listForRepoCalls === 1` + +**Verify:** `npx tsx --test server/github.test.ts server/babysitter.test.ts server/appRuntime.test.ts server/storage.test.ts` and `npm run check` + +--- + +## PR 3 — Feedback: 304-skip GraphQL + smaller nested page + +**Why:** `fetchFeedbackItemsForPR` always paginates 3 REST lists **and** runs `reviewThreads(first: 100) { comments(first: 100) }`. That nested query is the GraphQL point bomb. On a quiet PR the comment lists are unchanged. + +**Behavior** + +- Manual page-1 `If-None-Match` on: + - `pulls.listReviewComments` + - `pulls.listReviews` + - `issues.listComments` +- Persist three etags: `pr:${repo}#${n}:review-comments|reviews|issue-comments` +- If **all three** 304: return `{ notModified: true }`. Babysitter `syncFeedbackForPR` skips merge and keeps stored `feedbackItems`. +- If **any** 200: full fetch as today (paginate remaining pages, GraphQL threads), then persist new etags. +- Shrink `REVIEW_THREADS_QUERY` nested `comments(first: 100)` to `comments(first: 20)`. Overflow still uses `REVIEW_THREAD_COMMENTS_QUERY`. Cost ≈ 2.1k points/query instead of ~10k (and under GitHub’s 5k/query ceiling). + +Do not try to persist comment bodies. All-304 means GitHub says nothing in those lists changed, so stored feedback is the source of truth. + +**Files** + +- `server/github.ts` — `fetchFeedbackItemsForPR` return type becomes `FeedbackItem[] | { notModified: true }` **or** keep the array and add `fetchFeedbackItemsForPRConditional`. Prefer a single function with an optional `{ etags }` input/output so babysitter owns persistence via existing `github_etags`. +- `server/babysitter.ts` — load/store the three etags around sync +- `server/github.test.ts` — 304 all three: graphql call count 0; mixed 304/200: GraphQL runs; pagination test updated for `first: 20` +- `server/babysitter.test.ts` — unchanged comments: no GraphQL, feedback items unmodified + +**Tests** + +- All-304 → no `graphql`, no extra REST pages +- One list 200 → GraphQL + that list’s body used +- Nested comments page size 20 still maps `databaseId` 21 via overflow query + +**Verify:** `npx tsx --test server/github.test.ts server/babysitter.test.ts` and `npm run check` + +--- + +## PR 4 — CI by SHA: one fetch, then reuse + +**Why:** `getCombinedStatusForRef` + `checks.listForRef` run from `fetchCiPollResult`, `listFailingStatuses`, `checkCISettled`, and `fetchCheckSnapshotsForRef`. The babysit path at ~3884 calls `listFailingStatuses` **then** `fetchCheckSnapshotsForRef` (same two endpoints twice). Settled SHA results do not change. + +**Behavior** + +- One internal `loadCommitCheckState(octokit, repo, sha)` that hits the two endpoints (plus Actions job enrichment when needed). +- `fetchCiPollResult`, `listFailingStatuses`, `checkCISettled`, `fetchCheckSnapshotsForRef` all read that result. +- Process-local cache keyed by `${owner}/${repo}@${sha}`: + - If last result is **settled** (no pending statuses/checks): return it + - If unsettled: fetch again +- Babysitter ~3884: drop the extra `listFailingStatuses` call; derive failures from snapshots +- `pollForCICompletion` keeps low-priority re-polls; cache still applies so a settled SHA is not refetched +- Cache is in-memory only. Restart refetches once. Fine. + +**Files** + +- `server/github.ts`, `github.test.ts` +- `server/babysitter.ts` (the double-fetch site and poll loop) + +**Tests** + +- Two `fetchCiPollResult` calls for a settled SHA: one HTTP pair +- Unsettled SHA: second call hits GitHub again +- `fetchCheckSnapshotsForRef` after `fetchCiPollResult` on same SHA: zero extra HTTP +- Babysitter healing/status path: statuses+checks fetched once + +**Verify:** `npx tsx --test server/github.test.ts server/babysitter.test.ts` and `npm run check` + +--- + +## PR 5 — Early-stop merged-PR pagination + +**Why:** `listMergedPullsSince` uses `octokit.paginate` over **all** closed PRs, then filters by `merged_at`. For a busy repo that is tens of REST pages per release preview. + +**Behavior** + +- Replace `paginate` with a manual `per_page: 100` loop (`sort: updated`, `direction: desc`), same as open-PR listing. +- After each page: + - Apply the existing `merged_at` / merge-sha filters + - **Stop** when every item on the page has `updated_at` older than `sinceMergedAt` (`merged_at <= updated_at`, so the since window cannot still be ahead) + - **Stop** when `sinceMergeCommitSha` is found +- No since bound (and no sha): keep paging (same as today). `listUnreleasedMergedPulls` almost always has a release-date bound. + +**Files** + +- `server/github.ts`, `github.test.ts` (existing tests mock `paginate`; switch them to paged `pulls.list`) + +**Tests** + +- Two pages, page 2 all `updated_at` before since → `pulls.list` called twice, not a third time +- Boundary sha on page 1 → no page 2 +- No since / no sha → pages until a short page (current unbounded behavior) +- Existing timestamp + sha filter assertions still pass + +**Verify:** `npx tsx --test server/github.test.ts server/releaseManager.test.ts` and `npm run check` + +--- + +## Execution order and gates + +1. Fetch `origin/main`, create worktree, branch for PR 1 +2. TDD → implement → `npm run check` + focused tests per PR +3. After PR 5: `npm run test:all` +4. UI: coverage is the only dashboard behavior change. After PR 1, with the app running, open Issues, confirm `synced N / GitHub M` still appears, Network tab shows `/api/issues/coverage` with **no** GitHub token use (server-side: no Search). If the app is not running, say so and rely on tests. +5. Open PRs as they complete; do not push to `main` + +## Risk notes + +- **Coverage freshness:** GitHub count updates on issue sweep (default 10 min), not every UI poll. That is the point. Manual full sweep still refreshes it. +- **PR payload size:** Open-PR JSON in `github_etags.payload` is small (summaries, not diffs). Hundreds of PRs is fine. +- **Feedback 304:** GitHub 304 on those three lists is the contract that comments/reviews did not change. Thread resolved-state that GitHub changes *without* touching comments is rare; accept that until someone files it. Do not add a fourth GraphQL poll to catch it. +- **CI cache:** Commit statuses can theoretically flip after “settled” if a check is rerun. Key stays SHA; a rerun on the same SHA would be stale until process restart **or** until we also invalidate when babysitter explicitly reruns Actions. Invalidate the SHA entry in `rerunFailedGitHubActionsRunsForSnapshots` (same PR 4). +- **Merged-PR sort:** Early-stop depends on `sort=updated`. Do not change sort. + +## Out of scope unless a PR still 429s after this + +- Secondary-limit pacing beyond the existing 67ms / search-concurrency=2 throttle +- GraphQL persisted queries +- Cross-process CI cache diff --git a/server/appRuntime.test.ts b/server/appRuntime.test.ts index 129f599..c0b91a4 100644 --- a/server/appRuntime.test.ts +++ b/server/appRuntime.test.ts @@ -1026,6 +1026,83 @@ test("syncRepos skips the issue sweep for a repo whose issue list responds 304", assert.equal(synced.items.length, 0, "a 304 must not sync any issues"); }); +test("listIssueCoverage reads persisted counts and does not call GitHub", async () => { + const storage = new MemStorage(); + await storage.updateConfig({ watchedRepos: ["owner/repo"] }); + await storage.upsertSyncedIssues("owner/repo", [{ + number: 7, + title: "Issue 7", + body: null, + bodyHtml: null, + url: "https://github.com/owner/repo/issues/7", + repoFullName: "owner/repo", + repoCloneUrl: "https://github.com/owner/repo.git", + author: "alice", + labels: [], + assignees: [], + comments: 0, + createdAt: "2026-05-03T17:00:00.000Z", + updatedAt: "2026-05-03T18:00:00.000Z", + }], "2026-05-03T18:00:00.000Z"); + await storage.upsertRepoSyncState("owner/repo", "issues", { + lastSyncedAt: "2026-05-03T18:00:00.000Z", + githubOpenCount: 12, + }); + + const runtime = createAppRuntime({ + storage, + startBackgroundServices: false, + startWatcher: false, + babysitter: { syncAndBabysitTrackedRepos: async () => {} } as never, + buildOctokitFn: async () => { + throw new Error("listIssueCoverage must not build an Octokit client"); + }, + }); + + const coverage = await runtime.listIssueCoverage(); + assert.deepEqual(coverage, [{ + repo: "owner/repo", + syncedOpenCount: 1, + githubOpenCount: 12, + lastSyncedAt: "2026-05-03T18:00:00.000Z", + }]); +}); + +test("syncRepos 304 probe does not fetch a GitHub open-issue count", async () => { + const storage = new MemStorage(); + await storage.updateConfig({ watchedRepos: ["owner/repo"] }); + await storage.upsertRepoSyncState("owner/repo", "issues", { githubOpenCount: 9 }); + + let graphqlCalls = 0; + const fakeOctokit = { + issues: { + listForRepo: async () => { + const error = new Error("Not modified") as Error & { status: number }; + error.status = 304; + throw error; + }, + }, + graphql: async () => { + graphqlCalls += 1; + return { repository: { issues: { totalCount: 99 } } }; + }, + }; + + const runtime = createAppRuntime({ + storage, + startBackgroundServices: false, + startWatcher: false, + babysitter: { syncAndBabysitTrackedRepos: async () => {} } as never, + buildOctokitFn: async () => fakeOctokit as never, + }); + + await runtime.syncRepos(); + + assert.equal(graphqlCalls, 0, "a 304 must not spend GraphQL on a count we already have"); + const state = (await storage.getRepoSyncStates("issues"))[0]; + assert.equal(state?.githubOpenCount, 9); +}); + test("syncRepos syncs issues and persists the new etag when the issue list changed", async () => { const storage = new MemStorage(); await storage.updateConfig({ watchedRepos: ["owner/repo"] }); @@ -1042,12 +1119,21 @@ test("syncRepos syncs issues and persists the new etag when the issue list chang created_at: "2026-05-03T17:00:00.000Z", updated_at: "2026-05-03T18:00:00.000Z", }; + let graphqlCalls = 0; + let listForRepoCalls = 0; const fakeOctokit = { issues: { - listForRepo: async () => ({ - data: [issuePayload], - headers: { etag: 'W/"issues-v1"' }, - }), + listForRepo: async () => { + listForRepoCalls += 1; + return { + data: [issuePayload], + headers: { etag: 'W/"issues-v1"' }, + }; + }, + }, + graphql: async () => { + graphqlCalls += 1; + return { repository: { issues: { totalCount: 4 } } }; }, }; @@ -1069,6 +1155,9 @@ test("syncRepos syncs issues and persists the new etag when the issue list chang 'W/"issues-v1"', "a successful sweep should persist the fresh etag for next tick", ); + assert.equal(graphqlCalls, 1); + assert.equal(listForRepoCalls, 1, "a changed issue list must reuse the probe page instead of fetching page 1 twice"); + assert.equal((await storage.getRepoSyncStates("issues"))[0]?.githubOpenCount, 4); }); test("listIssues stays cached-only when issue automation is off", async () => { diff --git a/server/appRuntime.ts b/server/appRuntime.ts index 1e7f31f..43350f5 100644 --- a/server/appRuntime.ts +++ b/server/appRuntime.ts @@ -66,14 +66,15 @@ import { getLatestSemverTagForRepo, GitHubIntegrationError, installCodeReviewWorkflow, + fetchOpenIssueCount, listOpenIssuesForRepo, + listOpenIssuesForRepoConditional, listOpenLinkedPullRequestsForIssue, listReleasesForRepo, listUnreleasedMergedPulls, type MergedPRSummary, parsePRUrl, parseRepoSlug, - probeRepoIssuesChanged, addLabelsToIssue, removeLabelsFromIssue, resolveNextSemverTag, @@ -1564,10 +1565,16 @@ export function createAppRuntime(dependencies: AppRuntimeDependencies = {}): App // unchanged repo we skip the whole sweep instead of paginating it. const etagKey = `issues:open:${repoSlug}`; let pendingEtag: string | null = null; + let didWork = false; if (nextOffset === 0) { const cachedEtag = (await storage.getGithubEtag(etagKey)) ?? null; - const probe = await probeRepoIssuesChanged(octokit, parsed, cachedEtag); - if (probe.notModified) { + const conditional = await listOpenIssuesForRepoConditional( + octokit, + parsed, + cachedEtag, + { limit, offset: 0 }, + ); + if (conditional.notModified) { // A 304 confirms the repo's issue list is current. Record the // freshness check and defer the next sweep — a quiet repo does not // need an every-tick slot. @@ -1579,26 +1586,47 @@ export function createAppRuntime(dependencies: AppRuntimeDependencies = {}): App issueRepoCursor = index === -1 ? issueRepoCursor : (index + 1) % repoCount; continue; } - pendingEtag = probe.etag; - } - - let didWork = false; - for (let i = 0; i < loops; i += 1) { - const page = await listOpenIssuesForRepo(octokit, parsed, { limit, offset: nextOffset }); + pendingEtag = conditional.etag; const seenAt = new Date().toISOString(); - if (nextOffset === 0) { - await storage.markRepoIssuesStale(repoSlug); - } - await storage.upsertSyncedIssues(repoSlug, page.items, seenAt); - nextOffset = page.hasMore ? nextOffset + limit : 0; + await storage.markRepoIssuesStale(repoSlug); + await storage.upsertSyncedIssues(repoSlug, conditional.items, seenAt); + nextOffset = conditional.hasMore ? limit : 0; issueRepoSyncOffsets.set(repoSlug, nextOffset); didWork = true; - if (!page.hasMore || !options?.fullSweep) break; + if (conditional.hasMore && options?.fullSweep) { + for (let i = 1; i < loops; i += 1) { + const page = await listOpenIssuesForRepo(octokit, parsed, { limit, offset: nextOffset }); + await storage.upsertSyncedIssues(repoSlug, page.items, new Date().toISOString()); + nextOffset = page.hasMore ? nextOffset + limit : 0; + issueRepoSyncOffsets.set(repoSlug, nextOffset); + if (!page.hasMore) break; + } + } + } else { + for (let i = 0; i < loops; i += 1) { + const page = await listOpenIssuesForRepo(octokit, parsed, { limit, offset: nextOffset }); + const seenAt = new Date().toISOString(); + await storage.upsertSyncedIssues(repoSlug, page.items, seenAt); + nextOffset = page.hasMore ? nextOffset + limit : 0; + issueRepoSyncOffsets.set(repoSlug, nextOffset); + didWork = true; + if (!page.hasMore || !options?.fullSweep) break; + } } if (didWork) { + let githubOpenCount: number | null | undefined; + try { + githubOpenCount = await fetchOpenIssueCount(octokit, parsed); + } catch (error) { + log.warn( + { err: error instanceof Error ? error.message : String(error), repo: repoSlug }, + "Open issue count lookup failed; keeping last known coverage count", + ); + } await storage.upsertRepoSyncState(repoSlug, "issues", { lastSyncedAt: new Date().toISOString(), nextEligibleAt: null, + ...(typeof githubOpenCount === "number" ? { githubOpenCount } : {}), }); // Persist the etag only after a successful sync so a failure mid-sweep // re-probes and re-syncs next tick instead of 304-skipping stale data. @@ -2509,26 +2537,16 @@ export function createAppRuntime(dependencies: AppRuntimeDependencies = {}): App storage.getRepoSyncStates("issues"), ]); const syncByRepo = new Map(syncStates.map((state) => [state.repo, state])); - const octokit = await buildOctokit(config); - const coverage = await Promise.all(config.watchedRepos.map(async (repo): Promise => { + return config.watchedRepos.map((repo): IssueCoverage => { const localCount = counts.repoTotals[repo] ?? 0; const syncState = syncByRepo.get(repo); - const parsed = parseRepoSlug(repo); - if (!parsed) { - return { repo, syncedOpenCount: localCount, githubOpenCount: null, lastSyncedAt: syncState?.lastSyncedAt ?? null }; - } - try { - const result = await octokit.request("GET /search/issues", { - q: `repo:${repo} is:issue is:open`, - per_page: 1, - }); - const githubOpenCount = typeof result.data?.total_count === "number" ? result.data.total_count : null; - return { repo, syncedOpenCount: localCount, githubOpenCount, lastSyncedAt: syncState?.lastSyncedAt ?? null }; - } catch { - return { repo, syncedOpenCount: localCount, githubOpenCount: null, lastSyncedAt: syncState?.lastSyncedAt ?? null }; - } - })); - return coverage; + return { + repo, + syncedOpenCount: localCount, + githubOpenCount: syncState?.githubOpenCount ?? null, + lastSyncedAt: syncState?.lastSyncedAt ?? null, + }; + }); }, async createManualRelease(repoInput) { diff --git a/server/babysitter.test.ts b/server/babysitter.test.ts index 2d2c0d0..5175c27 100644 --- a/server/babysitter.test.ts +++ b/server/babysitter.test.ts @@ -620,6 +620,84 @@ test("syncAndBabysitTrackedRepos reuses the cached PR list on a conditional 304" assert.equal(fullFetchCalls, 0, "a 304 with a warm cache must not trigger a full PR-list fetch"); }); +test("syncAndBabysitTrackedRepos reuses a persisted PR list after a process restart", async () => { + const storage = new MemStorage(); + await storage.updateConfig({ watchedRepos: ["octo/example"] }); + + const samplePull = { + number: 1, + title: "Tracked PR", + body: null, + bodyHtml: null, + branch: "feature/x", + author: "octocat", + url: "https://github.com/octo/example/pull/1", + repoFullName: "octo/example", + repoCloneUrl: "https://github.com/octo/example.git", + headSha: "head1", + headRef: "feature/x", + headRepoFullName: "octo/example", + headRepoCloneUrl: "https://github.com/octo/example.git", + baseRef: "main", + baseSha: "base1", + mergeable: null, + mergeableState: null, + merged: false, + mergedAt: null, + closedAt: null, + mergeCommitSha: null, + }; + + const runtime = { + resolveAgent: async () => "codex" as const, + ciPollIntervalMs: 0, + evaluateFixNecessityWithAgent: async () => ({ needsFix: false, reason: "unused" }), + applyFixesWithAgent: async () => ({ code: 0, stdout: "", stderr: "" }), + runCommand: async () => ({ code: 0, stdout: "", stderr: "" }), + }; + + const first = new PRBabysitter( + storage, + makeWatcherGitHubService({ + listOpenPullsForRepoConditional: async () => ({ + notModified: false as const, + etag: 'W/"pulls-v1"', + pulls: [samplePull], + }), + }), + runtime, + ); + await first.syncAndBabysitTrackedRepos(); + + let fullFetchCalls = 0; + let sentEtag: string | null | undefined; + const restarted = new PRBabysitter( + storage, + makeWatcherGitHubService({ + listOpenPullsForRepo: async () => { + fullFetchCalls += 1; + return []; + }, + listOpenPullsForRepoConditional: async ( + _octokit: unknown, + _repo: unknown, + cachedEtag: string | null, + ) => { + sentEtag = cachedEtag; + return { notModified: true as const }; + }, + }), + runtime, + ); + await restarted.syncAndBabysitTrackedRepos(); + + assert.equal(sentEtag, 'W/"pulls-v1"'); + assert.equal(fullFetchCalls, 0, "a 304 after restart must reuse the persisted list, not paginate again"); + const stored = await storage.getGithubEtagRecord("prs:open:octo/example"); + assert.equal(stored?.etag, 'W/"pulls-v1"'); + assert.ok(stored?.payload?.includes('"number":1')); +}); + test("syncAndBabysitTrackedRepos persists a backoff when listing open PRs fails", async () => { const storage = new MemStorage(); await storage.updateConfig({ watchedRepos: ["octo/example"] }); diff --git a/server/babysitter.ts b/server/babysitter.ts index cf1bef8..c67a9c6 100644 --- a/server/babysitter.ts +++ b/server/babysitter.ts @@ -26,6 +26,7 @@ import { fetchCheckSnapshotsForRef, fetchCiPollResult, fetchFeedbackItemsForPR, + fetchFeedbackItemsForPRIfChanged, fetchPullCloseState, fetchPullSummary, formatRepoSlug, @@ -44,6 +45,7 @@ import { APP_STATUS_COMMENT_PATTERN, type CiPollResult, type GitHubActionsRerun, + type FeedbackListEtags, type GitHubPullSummary, type GitHubStatusFailure, type ParsedPRUrl, @@ -115,6 +117,32 @@ const APP_REPOSITORY_URL = "https://github.com/jeremymcs/patchdeck"; export const APP_COMMENT_FOOTER = formatAppCommentFooter(APP_NAME, true); const AUDIT_TOKEN_PATTERN = /\bcodefactory-feedback:[^\s<>()[\]{}"']+/g; +function feedbackEtagKeys(parsed: ParsedPRUrl): { + reviewComments: string; + reviews: string; + issueComments: string; +} { + const prefix = `pr:${parsed.owner}/${parsed.repo}#${parsed.number}`; + return { + reviewComments: `${prefix}:review-comments`, + reviews: `${prefix}:reviews`, + issueComments: `${prefix}:issue-comments`, + }; +} + +function parseStoredPullList( + record: { etag: string; payload: string | null } | undefined, +): { etag: string | null; pulls: GitHubPullSummary[] } | null { + if (!record?.payload) return null; + try { + const parsed = JSON.parse(record.payload) as unknown; + if (!Array.isArray(parsed)) return null; + return { etag: record.etag, pulls: parsed as GitHubPullSummary[] }; + } catch { + return null; + } +} + export class TerminalBabysitterError extends Error { constructor(message: string) { super(message); @@ -127,6 +155,7 @@ type GitHubService = { buildOctokit: typeof buildOctokit; checkCISettled: typeof checkCISettled; fetchFeedbackItemsForPR: typeof fetchFeedbackItemsForPR; + fetchFeedbackItemsForPRIfChanged?: typeof fetchFeedbackItemsForPRIfChanged; fetchPullCloseState?: typeof fetchPullCloseState; fetchPullSummary: typeof fetchPullSummary; fetchCheckSnapshotsForRef?: typeof fetchCheckSnapshotsForRef; @@ -208,6 +237,7 @@ const defaultGitHubService: GitHubService = { fetchCheckSnapshotsForRef, fetchCiPollResult, fetchFeedbackItemsForPR, + fetchFeedbackItemsForPRIfChanged, fetchPullCloseState, fetchPullSummary, getAuthenticatedLogin: async (octokit) => { @@ -2411,7 +2441,32 @@ export class PRBabysitter { }); } - const incomingFeedback = await this.github.fetchFeedbackItemsForPR(octokit, parsed, config); + const etagKeys = feedbackEtagKeys(parsed); + const previousEtags: FeedbackListEtags = { + reviewComments: (await this.storage.getGithubEtag(etagKeys.reviewComments)) ?? null, + reviews: (await this.storage.getGithubEtag(etagKeys.reviews)) ?? null, + issueComments: (await this.storage.getGithubEtag(etagKeys.issueComments)) ?? null, + }; + const feedbackResult = this.github.fetchFeedbackItemsForPRIfChanged + ? await this.github.fetchFeedbackItemsForPRIfChanged(octokit, parsed, config, previousEtags) + : { + notModified: false as const, + items: await this.github.fetchFeedbackItemsForPR(octokit, parsed, config), + etags: previousEtags, + }; + if (feedbackResult.notModified) { + return pr; + } + const incomingFeedback = feedbackResult.items; + if (feedbackResult.etags.reviewComments) { + await this.storage.setGithubEtag(etagKeys.reviewComments, feedbackResult.etags.reviewComments); + } + if (feedbackResult.etags.reviews) { + await this.storage.setGithubEtag(etagKeys.reviews, feedbackResult.etags.reviews); + } + if (feedbackResult.etags.issueComments) { + await this.storage.setGithubEtag(etagKeys.issueComments, feedbackResult.etags.issueComments); + } const { merged, newCount } = mergeFeedbackItems(pr.feedbackItems, incomingFeedback); const counters = countDecisions(merged); @@ -2599,7 +2654,9 @@ export class PRBabysitter { // last sweep, so the cached list is reused and the fetch costs no // primary rate-limit budget. The etag and list are cached together in // memory so they cannot drift out of sync across a restart. - const cachedPrList = this.prListCache.get(repoSlug); + const etagKey = `prs:open:${repoSlug}`; + const storedPrList = parseStoredPullList(await this.storage.getGithubEtagRecord(etagKey)); + const cachedPrList = this.prListCache.get(repoSlug) ?? storedPrList; const conditional = await this.github.listOpenPullsForRepoConditional( octokit, repo, @@ -2609,13 +2666,27 @@ export class PRBabysitter { if (conditional.notModified && cachedPrList) { openPulls = cachedPrList.pulls; prListUnchanged = true; + this.prListCache.set(repoSlug, cachedPrList); } else if (conditional.notModified) { - // Etag hit without a cached list (should not happen, since both are - // stored together) — fall back to a full fetch. - openPulls = await this.github.listOpenPullsForRepo(octokit, repo); + // 304 without a stored list (upgrade path): refetch without + // If-None-Match once, then persist so the next restart is free. + const fresh = await this.github.listOpenPullsForRepoConditional(octokit, repo, null); + openPulls = fresh.notModified ? [] : fresh.pulls; + const freshEtag = fresh.notModified ? null : fresh.etag; + this.prListCache.set(repoSlug, { etag: freshEtag, pulls: openPulls }); + if (freshEtag) { + await this.storage.setGithubEtag(etagKey, freshEtag, JSON.stringify(openPulls)); + } } else { openPulls = conditional.pulls; this.prListCache.set(repoSlug, { etag: conditional.etag, pulls: conditional.pulls }); + if (conditional.etag) { + await this.storage.setGithubEtag( + etagKey, + conditional.etag, + JSON.stringify(conditional.pulls), + ); + } } // When the PR list is unchanged, defer the next sweep — a quiet repo // does not need an every-tick slot. Any change returns it to every-tick. diff --git a/server/github.test.ts b/server/github.test.ts index 9497400..755dd32 100644 --- a/server/github.test.ts +++ b/server/github.test.ts @@ -10,8 +10,10 @@ import { buildFeedbackAuditToken, checkOnboardingStatus, fetchCheckSnapshotsForRef, + fetchCiPollResult, createGitHubRelease, fetchFeedbackItemsForPR, + fetchFeedbackItemsForPRIfChanged, fetchIssueSummary, fetchPullCloseState, fetchPullSummary, @@ -23,6 +25,8 @@ import { listOpenPullsForRepo, listOpenPullsForRepoConditional, probeRepoIssuesChanged, + listOpenIssuesForRepoConditional, + fetchOpenIssueCount, listMergedPullsSince, listReleasesForRepo, listTagsForRepo, @@ -825,6 +829,75 @@ test("fetchCheckSnapshotsForRef bounds parallel GitHub Actions job detail reques ); }); +test("settled commit checks are fetched once per SHA and reused across helpers", async () => { + let statusCalls = 0; + let checkCalls = 0; + const octokit = { + repos: { + getCombinedStatusForRef: async () => { + statusCalls += 1; + return { + data: { + statuses: [{ + state: "success", + context: "ci", + description: "ok", + target_url: null, + }], + }, + }; + }, + }, + checks: { + listForRef: async () => { + checkCalls += 1; + return { data: { check_runs: [] } }; + }, + }, + }; + + const repo = { owner: "owner", repo: "cache-repo" }; + const first = await fetchCiPollResult(octokit as never, repo, "sha-settled"); + const second = await fetchCiPollResult(octokit as never, repo, "sha-settled"); + await fetchCheckSnapshotsForRef(octokit as never, repo, "pr-1", "sha-settled"); + + assert.equal(first.settled, true); + assert.equal(second.settled, true); + assert.equal(statusCalls, 1); + assert.equal(checkCalls, 1); +}); + +test("unsettled commit checks are refetched on the next call", async () => { + let statusCalls = 0; + const octokit = { + repos: { + getCombinedStatusForRef: async () => { + statusCalls += 1; + return { + data: { + statuses: [{ + state: statusCalls === 1 ? "pending" : "success", + context: "ci", + description: "ok", + target_url: null, + }], + }, + }; + }, + }, + checks: { + listForRef: async () => ({ data: { check_runs: [] } }), + }, + }; + + const repo = { owner: "owner", repo: "pending-repo" }; + const first = await fetchCiPollResult(octokit as never, repo, "sha-pending"); + const second = await fetchCiPollResult(octokit as never, repo, "sha-pending"); + assert.equal(first.settled, false); + assert.equal(second.settled, true); + assert.equal(statusCalls, 2); +}); + test("listOpenPullsForRepo retries transient GitHub connection resets", async () => { let attempts = 0; const octokit = { @@ -1069,6 +1142,69 @@ test("probeRepoIssuesChanged returns the response etag on a 200", async () => { } }); +test("listOpenIssuesForRepoConditional returns mapped issues and the page-1 etag on a 200", async () => { + let listCalls = 0; + const octokit = { + issues: { + listForRepo: async (params: { page: number; headers?: Record }) => { + listCalls += 1; + assert.equal(params.page, 1); + assert.equal(params.headers?.["if-none-match"], 'W/"cached"'); + return { + data: [{ + number: 7, + title: "Issue 7", + body: "body", + html_url: "https://github.com/owner/repo/issues/7", + user: { login: "alice" }, + labels: [], + assignees: [], + comments: 0, + created_at: "2026-05-03T17:00:00.000Z", + updated_at: "2026-05-03T18:00:00.000Z", + }], + headers: { etag: 'W/"issues-v1"' }, + }; + }, + }, + }; + + const result = await listOpenIssuesForRepoConditional( + octokit as never, + { owner: "owner", repo: "repo" }, + 'W/"cached"', + { offset: 0, limit: 100 }, + ); + + assert.equal(result.notModified, false); + if (!result.notModified) { + assert.equal(result.etag, 'W/"issues-v1"'); + assert.equal(result.items.length, 1); + assert.equal(result.items[0]?.number, 7); + assert.equal(result.hasMore, false); + } + assert.equal(listCalls, 1); +}); + +test("listOpenIssuesForRepoConditional reports notModified on a 304", async () => { + const octokit = { + issues: { + listForRepo: async () => { + const error = new Error("Not modified") as Error & { status: number }; + error.status = 304; + throw error; + }, + }, + }; + + const result = await listOpenIssuesForRepoConditional( + octokit as never, + { owner: "owner", repo: "repo" }, + 'W/"cached"', + ); + assert.equal(result.notModified, true); +}); + test("probeRepoIssuesChanged reports notModified on a 304 and forwards If-None-Match", async () => { let sentIfNoneMatch: string | undefined; const octokit = { @@ -1092,6 +1228,29 @@ test("probeRepoIssuesChanged reports notModified on a 304 and forwards If-None-M assert.equal(sentIfNoneMatch, 'W/"cached"'); }); +test("fetchOpenIssueCount reads repository.issues.totalCount from GraphQL", async () => { + let received: { query: string; owner?: string; repo?: string } | null = null; + const octokit = { + graphql: async (query: string, params: { owner: string; repo: string }) => { + received = { query, ...params }; + return { repository: { issues: { totalCount: 17 } } }; + }, + }; + + const count = await fetchOpenIssueCount(octokit as never, { owner: "owner", repo: "repo" }); + assert.equal(count, 17); + assert.equal(received?.owner, "owner"); + assert.equal(received?.repo, "repo"); + assert.match(received?.query || "", /issues\(states: OPEN\)/); +}); + +test("fetchOpenIssueCount returns null when GraphQL omits totalCount", async () => { + const octokit = { + graphql: async () => ({ repository: { issues: {} } }), + }; + assert.equal(await fetchOpenIssueCount(octokit as never, { owner: "owner", repo: "repo" }), null); +}); + test("probeRepoIssuesChanged rethrows non-304 errors instead of skipping the sweep", async () => { const octokit = { issues: { @@ -1562,12 +1721,12 @@ test("fetchFeedbackItemsForPR paginates review thread comments beyond the first id: "THREAD_node_999", isResolved: true, comments: { - nodes: Array.from({ length: 100 }, (_unused, index) => ({ + nodes: Array.from({ length: 20 }, (_unused, index) => ({ databaseId: index + 1, })), pageInfo: { hasNextPage: true, - endCursor: "cursor-100", + endCursor: "cursor-20", }, }, }, @@ -1583,7 +1742,7 @@ test("fetchFeedbackItemsForPR paginates review thread comments beyond the first } assert.equal(params.threadId, "THREAD_node_999"); - assert.equal(params.cursor, "cursor-100"); + assert.equal(params.cursor, "cursor-20"); return { node: { @@ -1630,13 +1789,88 @@ test("fetchFeedbackItemsForPR paginates review thread comments beyond the first }, { threadId: "THREAD_node_999", - cursor: "cursor-100", + cursor: "cursor-20", }, ]); assert.match(graphqlCalls[0]?.query || "", /CodeFactoryReviewThreads/); + assert.match(graphqlCalls[0]?.query || "", /comments\(first: 20\)/); assert.match(graphqlCalls[1]?.query || "", /CodeFactoryReviewThreadComments/); }); +test("fetchFeedbackItemsForPRIfChanged skips GraphQL when all comment lists return 304", async () => { + let graphqlCalls = 0; + const notModified = () => { + const error = new Error("Not modified") as Error & { status: number }; + error.status = 304; + throw error; + }; + const octokit = { + graphql: async () => { + graphqlCalls += 1; + return {}; + }, + paginate: async () => { + throw new Error("paginate should not run on an all-304 probe"); + }, + pulls: { + listReviewComments: async () => notModified(), + listReviews: async () => notModified(), + }, + issues: { + listComments: async () => notModified(), + }, + }; + + const result = await fetchFeedbackItemsForPRIfChanged( + octokit as never, + { owner: "owner", repo: "repo", number: 1 }, + config, + { reviewComments: 'W/"c"', reviews: 'W/"r"', issueComments: 'W/"i"' }, + ); + + assert.equal(result.notModified, true); + assert.equal(graphqlCalls, 0); +}); + +test("fetchFeedbackItemsForPRIfChanged fetches feedback when any list changed", async () => { + let graphqlCalls = 0; + const notModified = () => { + const error = new Error("Not modified") as Error & { status: number }; + error.status = 304; + throw error; + }; + const octokit = { + graphql: async () => { + graphqlCalls += 1; + return { repository: { pullRequest: { reviewThreads: { nodes: [], pageInfo: {} } } } }; + }, + paginate: async () => [], + pulls: { + listReviewComments: async () => ({ data: [], headers: { etag: 'W/"c2"' } }), + listReviews: async () => notModified(), + }, + issues: { + listComments: async () => notModified(), + }, + }; + + const result = await fetchFeedbackItemsForPRIfChanged( + octokit as never, + { owner: "owner", repo: "repo", number: 1 }, + config, + { reviewComments: 'W/"c"', reviews: 'W/"r"', issueComments: 'W/"i"' }, + ); + + assert.equal(result.notModified, false); + if (!result.notModified) { + assert.deepEqual(result.items, []); + assert.equal(result.etags.reviewComments, 'W/"c2"'); + assert.equal(result.etags.reviews, 'W/"r"'); + assert.equal(result.etags.issueComments, 'W/"i"'); + } + assert.equal(graphqlCalls, 1); +}); + test("postFollowUpForFeedbackItem replies to review threads and resolveReviewThread resolves them", async () => { const requests: Array<{ query: string; params: Record }> = []; @@ -2373,6 +2607,52 @@ test("listMergedPullsSince applies timestamp and merge-sha boundaries", async () assert.equal(merged[0]?.mergeCommitSha, "sha-30"); }); +test("listMergedPullsSince stops paging once updated_at is older than the since bound", async () => { + const pages: number[] = []; + const octokit = { + pulls: { + list: async (params: { page: number; per_page: number }) => { + pages.push(params.page); + if (params.page === 1) { + return { + data: Array.from({ length: params.per_page }, (_unused, index) => ({ + number: 200 - index, + title: `new ${index}`, + html_url: `https://github.com/octo/example/pull/${200 - index}`, + user: { login: "a" }, + merged_at: "2026-03-28T12:00:00Z", + updated_at: "2026-03-28T12:00:00Z", + merge_commit_sha: `sha-${200 - index}`, + })), + }; + } + return { + data: [{ + number: 1, + title: "ancient", + html_url: "https://github.com/octo/example/pull/1", + user: { login: "b" }, + merged_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + merge_commit_sha: "sha-1", + }], + }; + }, + }, + }; + + const merged = await listMergedPullsSince( + octokit as never, + { owner: "octo", repo: "example" }, + { baseRef: "main", sinceMergedAt: "2026-03-01T00:00:00Z" }, + ); + + assert.deepEqual(pages, [1, 2]); + assert.equal(merged.length, 100); + assert.equal(merged.some((pull) => pull.number === 1), false); + assert.equal(merged.some((pull) => pull.number === 200), true); +}); + test("listMergedPullsSince uses the repository default branch when none is provided", async () => { const listPulls = Symbol("listPulls"); let repoLookups = 0; diff --git a/server/github.ts b/server/github.ts index 3b9386c..edb6acf 100644 --- a/server/github.ts +++ b/server/github.ts @@ -204,7 +204,7 @@ const REVIEW_THREADS_QUERY = ` nodes { id isResolved - comments(first: 100) { + comments(first: 20) { nodes { databaseId } @@ -262,6 +262,15 @@ const RESOLVE_REVIEW_THREAD_MUTATION = ` } } `; +const OPEN_ISSUE_COUNT_QUERY = ` + query CodeFactoryOpenIssueCount($owner: String!, $repo: String!) { + repository(owner: $owner, name: $repo) { + issues(states: OPEN) { + totalCount + } + } + } +`; const statusReplyMutationSchema = z.object({ addPullRequestReviewThreadReply: z.object({ @@ -602,13 +611,33 @@ async function enrichCheckRunsWithGitHubActionsSteps( return enrichedRuns; } -export async function fetchCheckSnapshotsForRef( +type LoadedCommitChecks = { + statuses: GitHubCommitStatusResponse[]; + checkRuns: GitHubCheckRunResponse[]; + settled: boolean; + failures: GitHubStatusFailure[]; +}; + +const commitCheckCache = new WeakMap>(); + +function commitCheckCacheKey(repo: ParsedRepoSlug, sha: string): string { + return `${repo.owner}/${repo.repo}@${sha}`; +} + +function invalidateCommitCheckCache(octokit: object, repo: ParsedRepoSlug, sha: string): void { + commitCheckCache.get(octokit)?.delete(commitCheckCacheKey(repo, sha)); +} + +async function loadCommitCheckState( octokit: Octokit, repo: ParsedRepoSlug, - prId: string, headSha: string, -): Promise { - if (!headSha) return []; +): Promise { + const key = commitCheckCacheKey(repo, headSha); + const cached = commitCheckCache.get(octokit)?.get(key); + if (cached?.settled) { + return cached; + } const [statusResponse, checkRunsResponse] = await Promise.all([ withGitHubErrorHandling("commit statuses", repo, () => octokit.repos.getCombinedStatusForRef({ @@ -623,16 +652,61 @@ export async function fetchCheckSnapshotsForRef( })), ]); + const statuses = (statusResponse.data.statuses ?? []) as GitHubCommitStatusResponse[]; + const checkRuns = (checkRunsResponse.data.check_runs ?? []) as GitHubCheckRunResponse[]; + const failures: GitHubStatusFailure[] = [ + ...statuses + .filter((status) => status.state === "failure" || status.state === "error") + .map((status) => ({ + context: status.context || "status-check", + description: status.description || "Failed status check", + targetUrl: status.target_url || null, + })), + ...checkRuns + .filter((run) => run.conclusion === "failure" || run.conclusion === "timed_out" || run.conclusion === "cancelled") + .map((run) => ({ + context: run.name || "check-run", + description: run.output?.summary || run.output?.title || `Check run ${run.conclusion}`, + targetUrl: run.html_url || null, + })), + ]; + const hasAnyChecks = statuses.length > 0 || checkRuns.length > 0; + const hasPending = statuses.some((status) => status.state === "pending") + || checkRuns.some((run) => run.status !== "completed"); + const loaded: LoadedCommitChecks = { + statuses, + checkRuns, + settled: hasAnyChecks && !hasPending, + failures, + }; + let bucket = commitCheckCache.get(octokit); + if (!bucket) { + bucket = new Map(); + commitCheckCache.set(octokit, bucket); + } + bucket.set(key, loaded); + return loaded; +} + +export async function fetchCheckSnapshotsForRef( + octokit: Octokit, + repo: ParsedRepoSlug, + prId: string, + headSha: string, +): Promise { + if (!headSha) return []; + + const loaded = await loadCommitCheckState(octokit, repo, headSha); const checkRuns = await enrichCheckRunsWithGitHubActionsSteps( octokit, repo, - (checkRunsResponse.data.check_runs ?? []) as GitHubCheckRunResponse[], + loaded.checkRuns, ); return normalizeCheckSnapshotsFromRef({ prId, sha: headSha, - statuses: (statusResponse.data.statuses ?? []) as GitHubCommitStatusResponse[], + statuses: loaded.statuses, checkRuns, }); } @@ -686,6 +760,10 @@ export async function rerunFailedGitHubActionsRunsForSnapshots( } const reruns = Array.from(byRunId.values()); + const shas = Array.from(new Set(snapshots.map((snapshot) => snapshot.sha).filter((sha) => Boolean(sha)))); + for (const sha of shas) { + invalidateCommitCheckCache(octokit, repo, sha); + } for (const rerun of reruns) { await withGitHubErrorHandling("rerun failed workflow jobs", repo, () => octokit.rest.actions.reRunWorkflowFailedJobs({ @@ -2162,6 +2240,96 @@ export async function postFollowUpForFeedbackItem( await replyToIssueComment(octokit, parsed, body); } +export type FeedbackListEtags = { + reviewComments: string | null; + reviews: string | null; + issueComments: string | null; +}; + +async function probeListEtag( + context: string, + parsed: ParsedPRUrl, + cachedEtag: string | null, + request: (headers: Record) => Promise<{ headers?: { etag?: string } }>, +): Promise<{ notModified: true } | { notModified: false; etag: string | null }> { + return withGitHubErrorHandling(context, parsed, async () => { + try { + const response = await request(cachedEtag ? { "if-none-match": cachedEtag } : {}); + const etag = typeof response.headers?.etag === "string" ? response.headers.etag : null; + return { notModified: false, etag }; + } catch (error) { + if ((error as { status?: number } | undefined)?.status === 304) { + return { notModified: true }; + } + throw error; + } + }); +} + +export async function fetchFeedbackItemsForPRIfChanged( + octokit: Octokit, + parsed: ParsedPRUrl, + config: Config, + etags: FeedbackListEtags, +): Promise<{ notModified: true } | { notModified: false; items: FeedbackItem[]; etags: FeedbackListEtags }> { + const canProbe = typeof octokit.pulls?.listReviewComments === "function" + && typeof octokit.pulls?.listReviews === "function" + && typeof octokit.issues?.listComments === "function"; + + if (canProbe) { + const [reviewComments, reviews, issueComments] = await Promise.all([ + probeListEtag("review comments", parsed, etags.reviewComments, (headers) => + octokit.pulls.listReviewComments({ + owner: parsed.owner, + repo: parsed.repo, + pull_number: parsed.number, + per_page: 100, + page: 1, + headers, + }), + ), + probeListEtag("reviews", parsed, etags.reviews, (headers) => + octokit.pulls.listReviews({ + owner: parsed.owner, + repo: parsed.repo, + pull_number: parsed.number, + per_page: 100, + page: 1, + headers, + }), + ), + probeListEtag("issue comments", parsed, etags.issueComments, (headers) => + octokit.issues.listComments({ + owner: parsed.owner, + repo: parsed.repo, + issue_number: parsed.number, + per_page: 100, + page: 1, + headers, + }), + ), + ]); + + if (reviewComments.notModified && reviews.notModified && issueComments.notModified) { + return { notModified: true }; + } + + const items = await fetchFeedbackItemsForPR(octokit, parsed, config); + return { + notModified: false, + items, + etags: { + reviewComments: reviewComments.notModified ? etags.reviewComments : reviewComments.etag, + reviews: reviews.notModified ? etags.reviews : reviews.etag, + issueComments: issueComments.notModified ? etags.issueComments : issueComments.etag, + }, + }; + } + + const items = await fetchFeedbackItemsForPR(octokit, parsed, config); + return { notModified: false, items, etags }; +} + export async function fetchFeedbackItemsForPR( octokit: Octokit, parsed: ParsedPRUrl, @@ -2394,6 +2562,26 @@ export async function listOpenPullsForRepoConditional( }); } +export async function fetchOpenIssueCount( + octokit: Octokit, + repo: ParsedRepoSlug, +): Promise { + const response = await withGitHubErrorHandling("open issue count", repo, () => + octokit.graphql<{ + repository?: { + issues?: { + totalCount?: number | null; + } | null; + } | null; + }>(OPEN_ISSUE_COUNT_QUERY, { + owner: repo.owner, + repo: repo.repo, + }), + ); + const count = response.repository?.issues?.totalCount; + return typeof count === "number" && Number.isFinite(count) ? count : null; +} + export async function listOpenIssuesForRepo( octokit: Octokit, repo: ParsedRepoSlug, @@ -2499,6 +2687,105 @@ export type RepoIssuesProbeResult = | { notModified: true } | { notModified: false; etag: string | null }; +export type ConditionalIssueList = + | { notModified: true } + | { notModified: false; etag: string | null; items: GitHubIssueSummary[]; hasMore: boolean }; + +type GitHubIssueListItem = Awaited>["data"][number]; + +function mapIssueListItem(issue: GitHubIssueListItem, repo: ParsedRepoSlug): GitHubIssueSummary | null { + if (issue.pull_request) return null; + return { + number: issue.number, + title: issue.title || `Issue #${issue.number}`, + body: typeof issue.body === "string" ? issue.body : null, + bodyHtml: typeof issue.body === "string" ? renderGitHubMarkdown(issue.body) : null, + url: issue.html_url || `https://github.com/${repo.owner}/${repo.repo}/issues/${issue.number}`, + repoFullName: `${repo.owner}/${repo.repo}`, + repoCloneUrl: `https://github.com/${repo.owner}/${repo.repo}.git`, + author: issue.user?.login || "", + labels: Array.isArray(issue.labels) + ? issue.labels + .map((label) => (typeof label === "string" ? label : label?.name ?? "")) + .filter((label): label is string => Boolean(label)) + : [], + assignees: Array.isArray(issue.assignees) + ? issue.assignees + .map((assignee) => assignee?.login || "") + .filter((assignee): assignee is string => Boolean(assignee)) + : [], + comments: typeof issue.comments === "number" ? issue.comments : 0, + createdAt: issue.created_at || new Date().toISOString(), + updatedAt: issue.updated_at || issue.created_at || new Date().toISOString(), + }; +} + +/** + * Conditional GET of a repo's open issues. Page 1 carries `If-None-Match`; a + * 304 means the list is unchanged and costs no primary rate-limit budget. A + * 200 reuses that page instead of fetching page 1 twice. + */ +export async function listOpenIssuesForRepoConditional( + octokit: Octokit, + repo: ParsedRepoSlug, + cachedEtag: string | null, + options: { offset: number; limit: number } = { offset: 0, limit: 100 }, +): Promise { + if (typeof octokit.issues?.listForRepo !== "function" || options.offset > 0) { + const page = await listOpenIssuesForRepo(octokit, repo, options); + return { notModified: false, etag: null, ...page }; + } + + return withGitHubErrorHandling("open issues", repo, async () => { + const perPage = 100; + const desired = options.limit + 1; + const collected: GitHubIssueSummary[] = []; + let firstPageEtag: string | null = null; + + for (let page = 1; ; page += 1) { + let response; + try { + response = await octokit.issues.listForRepo({ + owner: repo.owner, + repo: repo.repo, + state: "open", + sort: "updated", + direction: "desc", + per_page: perPage, + page, + ...(page === 1 && cachedEtag ? { headers: { "if-none-match": cachedEtag } } : {}), + }); + } catch (error) { + if (page === 1 && (error as { status?: number } | undefined)?.status === 304) { + return { notModified: true }; + } + throw error; + } + + if (page === 1) { + const etag = response.headers?.etag; + firstPageEtag = typeof etag === "string" ? etag : null; + } + + for (const issue of response.data) { + const mapped = mapIssueListItem(issue, repo); + if (mapped) collected.push(mapped); + } + + if (response.data.length < perPage || collected.length >= desired) { + break; + } + } + + return { + notModified: false, + etag: firstPageEtag, + items: collected.slice(0, options.limit), + hasMore: collected.length > options.limit, + }; + }); +} + /** * Conditional GET of a repo's open-issue list (page 1, sorted by `updated`). * A 304 means no issue has changed since `cachedEtag` was stored, so the @@ -2731,40 +3018,8 @@ export async function listFailingStatuses( headSha: string, ): Promise { if (!headSha) return []; - - // Fetch both commit statuses AND check runs in parallel. - // GitHub Actions CI/CD reports results as check runs (Checks API), - // while some integrations use the older commit status API. - const [statusResponse, checkRunsResponse] = await Promise.all([ - withGitHubErrorHandling("commit statuses", repo, () => octokit.repos.getCombinedStatusForRef({ - owner: repo.owner, - repo: repo.repo, - ref: headSha, - })), - withGitHubErrorHandling("check runs", repo, () => octokit.checks.listForRef({ - owner: repo.owner, - repo: repo.repo, - ref: headSha, - })), - ]); - - const fromStatuses: GitHubStatusFailure[] = statusResponse.data.statuses - .filter((status) => status.state === "failure" || status.state === "error") - .map((status) => ({ - context: status.context || "status-check", - description: status.description || "Failed status check", - targetUrl: status.target_url || null, - })); - - const fromCheckRuns: GitHubStatusFailure[] = checkRunsResponse.data.check_runs - .filter((run) => run.conclusion === "failure" || run.conclusion === "timed_out" || run.conclusion === "cancelled") - .map((run) => ({ - context: run.name, - description: run.output?.summary || run.output?.title || `Check run ${run.conclusion}`, - targetUrl: run.html_url || null, - })); - - return [...fromStatuses, ...fromCheckRuns]; + const loaded = await loadCommitCheckState(octokit, repo, headSha); + return loaded.failures; } /** @@ -2778,26 +3033,9 @@ export async function checkCISettled( headSha: string, ): Promise { if (!headSha) return false; - try { - const [statusResp, checkResp] = await Promise.all([ - withGitHubErrorHandling("commit statuses (settled check)", repo, () => octokit.repos.getCombinedStatusForRef({ - owner: repo.owner, - repo: repo.repo, - ref: headSha, - })), - withGitHubErrorHandling("check runs (settled check)", repo, () => octokit.checks.listForRef({ - owner: repo.owner, - repo: repo.repo, - ref: headSha, - })), - ]); - - const hasPendingStatus = statusResp.data.statuses.some((s) => s.state === "pending"); - const hasPendingCheck = checkResp.data.check_runs.some((r) => r.status !== "completed"); - const hasAnyChecks = statusResp.data.statuses.length > 0 || checkResp.data.check_runs.length > 0; - - return hasAnyChecks && !hasPendingStatus && !hasPendingCheck; + const loaded = await loadCommitCheckState(octokit, repo, headSha); + return loaded.settled; } catch { return false; } @@ -2820,45 +3058,8 @@ export async function fetchCiPollResult( headSha: string, ): Promise { if (!headSha) return { settled: false, failures: [] }; - - const [statusResponse, checkRunsResponse] = await Promise.all([ - withGitHubErrorHandling("commit statuses", repo, () => octokit.repos.getCombinedStatusForRef({ - owner: repo.owner, - repo: repo.repo, - ref: headSha, - })), - withGitHubErrorHandling("check runs", repo, () => octokit.checks.listForRef({ - owner: repo.owner, - repo: repo.repo, - ref: headSha, - })), - ]); - - const statuses = statusResponse.data.statuses; - const checkRuns = checkRunsResponse.data.check_runs; - - const failures: GitHubStatusFailure[] = [ - ...statuses - .filter((status) => status.state === "failure" || status.state === "error") - .map((status) => ({ - context: status.context || "status-check", - description: status.description || "Failed status check", - targetUrl: status.target_url || null, - })), - ...checkRuns - .filter((run) => run.conclusion === "failure" || run.conclusion === "timed_out" || run.conclusion === "cancelled") - .map((run) => ({ - context: run.name, - description: run.output?.summary || run.output?.title || `Check run ${run.conclusion}`, - targetUrl: run.html_url || null, - })), - ]; - - const hasAnyChecks = statuses.length > 0 || checkRuns.length > 0; - const hasPending = statuses.some((s) => s.state === "pending") - || checkRuns.some((r) => r.status !== "completed"); - - return { settled: hasAnyChecks && !hasPending, failures }; + const loaded = await loadCommitCheckState(octokit, repo, headSha); + return { settled: loaded.settled, failures: loaded.failures }; } export type MergedPRSummary = { @@ -2908,35 +3109,80 @@ export async function listMergedPullsSince( const sinceMergedAtMs = parseDateMs(options?.sinceMergedAt); const sinceMergeCommitSha = options?.sinceMergeCommitSha?.trim() || null; - const pulls = await withGitHubErrorHandling("merged pull requests", repo, () => - octokit.paginate(octokit.pulls.list, { - owner: repo.owner, - repo: repo.repo, - state: "closed", - base: baseRef, - sort: "updated", - direction: "desc", - per_page: 100, - }), - ); + if (typeof octokit.pulls?.list !== "function") { + const pulls = await withGitHubErrorHandling("merged pull requests", repo, () => + octokit.paginate(octokit.pulls.list, { + owner: repo.owner, + repo: repo.repo, + state: "closed", + base: baseRef, + sort: "updated", + direction: "desc", + per_page: 100, + }), + ); - const merged = pulls - .map((pull) => normalizeMergedPullSummary(pull, repo)) - .filter((pull): pull is MergedPRSummary => Boolean(pull)) - .filter((pull) => { - if (sinceMergedAtMs === null) return true; - return Date.parse(pull.mergedAt) > sinceMergedAtMs; - }); + const merged = pulls + .map((pull) => normalizeMergedPullSummary(pull, repo)) + .filter((pull): pull is MergedPRSummary => Boolean(pull)) + .filter((pull) => { + if (sinceMergedAtMs === null) return true; + return Date.parse(pull.mergedAt) > sinceMergedAtMs; + }); - let boundedBySha = merged; - if (sinceMergeCommitSha) { - const boundaryIndex = merged.findIndex((pull) => pull.mergeCommitSha === sinceMergeCommitSha); - if (boundaryIndex >= 0) { - boundedBySha = merged.slice(0, boundaryIndex); + let boundedBySha = merged; + if (sinceMergeCommitSha) { + const boundaryIndex = merged.findIndex((pull) => pull.mergeCommitSha === sinceMergeCommitSha); + if (boundaryIndex >= 0) { + boundedBySha = merged.slice(0, boundaryIndex); + } } + + return boundedBySha.sort((a, b) => Date.parse(a.mergedAt) - Date.parse(b.mergedAt)); } - return boundedBySha.sort((a, b) => Date.parse(a.mergedAt) - Date.parse(b.mergedAt)); + return withGitHubErrorHandling("merged pull requests", repo, async () => { + const perPage = 100; + const collected: MergedPRSummary[] = []; + + for (let page = 1; ; page += 1) { + const response = await octokit.pulls.list({ + owner: repo.owner, + repo: repo.repo, + state: "closed", + base: baseRef, + sort: "updated", + direction: "desc", + per_page: perPage, + page, + }); + const pageItems = response.data; + let foundShaBoundary = false; + let pageAllOlderThanSince = sinceMergedAtMs !== null && pageItems.length > 0; + + for (const pull of pageItems) { + const updatedMs = Date.parse(pull.updated_at || pull.merged_at || ""); + if (sinceMergedAtMs === null || !Number.isFinite(updatedMs) || updatedMs > sinceMergedAtMs) { + pageAllOlderThanSince = false; + } + if (foundShaBoundary) continue; + if (sinceMergeCommitSha && pull.merge_commit_sha === sinceMergeCommitSha) { + foundShaBoundary = true; + continue; + } + const summary = normalizeMergedPullSummary(pull, repo); + if (!summary) continue; + if (sinceMergedAtMs !== null && Date.parse(summary.mergedAt) <= sinceMergedAtMs) continue; + collected.push(summary); + } + + if (foundShaBoundary || pageItems.length < perPage || pageAllOlderThanSince) { + break; + } + } + + return collected.sort((a, b) => Date.parse(a.mergedAt) - Date.parse(b.mergedAt)); + }); } export async function listUnreleasedMergedPulls( diff --git a/server/memoryStorage.ts b/server/memoryStorage.ts index a7d3314..e47f4da 100644 --- a/server/memoryStorage.ts +++ b/server/memoryStorage.ts @@ -80,7 +80,7 @@ export class MemStorage implements IStorage { private issueEvaluations: Map = new Map(); private issueSubtaskSets: Map = new Map(); private syncedIssues: Map = new Map(); - private githubEtags: Map = new Map(); + private githubEtags: Map = new Map(); private repoSyncStates: Map = new Map(); private cloneConfig(config: Config): Config { @@ -400,11 +400,20 @@ export class MemStorage implements IStorage { } async getGithubEtag(url: string): Promise { - return this.githubEtags.get(url); + return this.githubEtags.get(url)?.etag; } - async setGithubEtag(url: string, etag: string): Promise { - this.githubEtags.set(url, etag); + async getGithubEtagRecord(url: string): Promise<{ etag: string; payload: string | null } | undefined> { + const record = this.githubEtags.get(url); + return record ? { ...record } : undefined; + } + + async setGithubEtag(url: string, etag: string, payload?: string | null): Promise { + const existing = this.githubEtags.get(url); + this.githubEtags.set(url, { + etag, + payload: payload === undefined ? existing?.payload ?? null : payload, + }); } async clearGithubEtag(url: string): Promise { @@ -420,7 +429,11 @@ export class MemStorage implements IStorage { async upsertRepoSyncState( repo: string, kind: RepoSyncKind, - updates: { lastSyncedAt?: string | null; nextEligibleAt?: string | null }, + updates: { + lastSyncedAt?: string | null; + nextEligibleAt?: string | null; + githubOpenCount?: number | null; + }, ): Promise { const key = `${repo}#${kind}`; const existing = this.repoSyncStates.get(key); @@ -433,6 +446,9 @@ export class MemStorage implements IStorage { nextEligibleAt: "nextEligibleAt" in updates ? updates.nextEligibleAt ?? null : existing?.nextEligibleAt ?? null, + githubOpenCount: "githubOpenCount" in updates + ? updates.githubOpenCount ?? null + : existing?.githubOpenCount ?? null, }); } diff --git a/server/sqliteStorage.ts b/server/sqliteStorage.ts index f909544..d82548c 100644 --- a/server/sqliteStorage.ts +++ b/server/sqliteStorage.ts @@ -992,6 +992,8 @@ export class SqliteStorage implements IStorage { this.ensureColumn("prs", "pr_stage", "TEXT"); this.ensureColumn("prs", "mergeable_state", "TEXT"); this.ensureColumn("prs", "work_contract_json", "TEXT"); + this.ensureColumn("repo_sync_state", "github_open_count", "INTEGER"); + this.ensureColumn("github_etags", "payload", "TEXT"); const configExists = this.get<{ present: number }>("SELECT 1 AS present FROM config WHERE id = 1"); if (!configExists) { @@ -2338,13 +2340,28 @@ export class SqliteStorage implements IStorage { return row?.etag; } - async setGithubEtag(url: string, etag: string): Promise { + async getGithubEtagRecord(url: string): Promise<{ etag: string; payload: string | null } | undefined> { + const row = this.get<{ etag: string; payload: string | null }>( + "SELECT etag, payload FROM github_etags WHERE url = ?", + url, + ); + if (!row) return undefined; + return { etag: row.etag, payload: row.payload ?? null }; + } + + async setGithubEtag(url: string, etag: string, payload?: string | null): Promise { this.withWriteTransaction(() => { + const existing = this.get<{ payload: string | null }>( + "SELECT payload FROM github_etags WHERE url = ?", + url, + ); + const nextPayload = payload === undefined ? existing?.payload ?? null : payload; this.run( - `INSERT INTO github_etags (url, etag, updated_at) VALUES (?, ?, ?) - ON CONFLICT(url) DO UPDATE SET etag = excluded.etag, updated_at = excluded.updated_at`, + `INSERT INTO github_etags (url, etag, payload, updated_at) VALUES (?, ?, ?, ?) + ON CONFLICT(url) DO UPDATE SET etag = excluded.etag, payload = excluded.payload, updated_at = excluded.updated_at`, url, etag, + nextPayload, new Date().toISOString(), ); }); @@ -2362,23 +2379,33 @@ export class SqliteStorage implements IStorage { kind: string; last_synced_at: string | null; next_eligible_at: string | null; - }>("SELECT repo, kind, last_synced_at, next_eligible_at FROM repo_sync_state WHERE kind = ?", kind); + github_open_count: number | null; + }>("SELECT repo, kind, last_synced_at, next_eligible_at, github_open_count FROM repo_sync_state WHERE kind = ?", kind); return rows.map((row) => ({ repo: row.repo, kind: row.kind as RepoSyncKind, lastSyncedAt: row.last_synced_at, nextEligibleAt: row.next_eligible_at, + githubOpenCount: typeof row.github_open_count === "number" ? row.github_open_count : null, })); } async upsertRepoSyncState( repo: string, kind: RepoSyncKind, - updates: { lastSyncedAt?: string | null; nextEligibleAt?: string | null }, + updates: { + lastSyncedAt?: string | null; + nextEligibleAt?: string | null; + githubOpenCount?: number | null; + }, ): Promise { this.withWriteTransaction(() => { - const existing = this.get<{ last_synced_at: string | null; next_eligible_at: string | null }>( - "SELECT last_synced_at, next_eligible_at FROM repo_sync_state WHERE repo = ? AND kind = ?", + const existing = this.get<{ + last_synced_at: string | null; + next_eligible_at: string | null; + github_open_count: number | null; + }>( + "SELECT last_synced_at, next_eligible_at, github_open_count FROM repo_sync_state WHERE repo = ? AND kind = ?", repo, kind, ); @@ -2388,15 +2415,20 @@ export class SqliteStorage implements IStorage { const nextEligibleAt = "nextEligibleAt" in updates ? updates.nextEligibleAt ?? null : existing?.next_eligible_at ?? null; + const githubOpenCount = "githubOpenCount" in updates + ? updates.githubOpenCount ?? null + : existing?.github_open_count ?? null; this.run( - `INSERT INTO repo_sync_state (repo, kind, last_synced_at, next_eligible_at) VALUES (?, ?, ?, ?) + `INSERT INTO repo_sync_state (repo, kind, last_synced_at, next_eligible_at, github_open_count) VALUES (?, ?, ?, ?, ?) ON CONFLICT(repo, kind) DO UPDATE SET last_synced_at = excluded.last_synced_at, - next_eligible_at = excluded.next_eligible_at`, + next_eligible_at = excluded.next_eligible_at, + github_open_count = excluded.github_open_count`, repo, kind, lastSyncedAt, nextEligibleAt, + githubOpenCount, ); }); } diff --git a/server/storage.test.ts b/server/storage.test.ts index a1fa99a..d407c57 100644 --- a/server/storage.test.ts +++ b/server/storage.test.ts @@ -1311,6 +1311,29 @@ test("MemStorage GitHub etag round-trip matches the SqliteStorage contract", asy assert.equal(await storage.getGithubEtag("issues:open:owner/repo"), undefined); }); +test("SqliteStorage persists GitHub etag payloads across reopen without dropping them on etag-only writes", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "codefactory-storage-")); + const first = new SqliteStorage(root); + try { + await first.setGithubEtag("prs:open:owner/repo", 'W/"pulls-v1"', JSON.stringify([{ number: 5 }])); + await first.setGithubEtag("prs:open:owner/repo", 'W/"pulls-v2"'); + const record = await first.getGithubEtagRecord("prs:open:owner/repo"); + assert.equal(record?.etag, 'W/"pulls-v2"'); + assert.equal(record?.payload, JSON.stringify([{ number: 5 }])); + } finally { + first.close(); + } + + const second = new SqliteStorage(root); + try { + const record = await second.getGithubEtagRecord("prs:open:owner/repo"); + assert.equal(record?.etag, 'W/"pulls-v2"'); + assert.equal(record?.payload, JSON.stringify([{ number: 5 }])); + } finally { + second.close(); + } +}); + test("SqliteStorage persists repo sync state with partial updates across reopen", async () => { const root = await mkdtemp(path.join(os.tmpdir(), "codefactory-storage-")); const first = new SqliteStorage(root); @@ -1333,6 +1356,7 @@ test("SqliteStorage persists repo sync state with partial updates across reopen" kind: "prs", lastSyncedAt: "2026-05-15T10:00:00.000Z", nextEligibleAt: "2026-05-15T12:00:00.000Z", + githubOpenCount: null, }]); // `kind` partitions the rows. assert.equal((await first.getRepoSyncStates("issues")).length, 1); @@ -1356,10 +1380,38 @@ test("MemStorage repo sync state matches the SqliteStorage contract", async () = await storage.upsertRepoSyncState("o/r", "prs", { lastSyncedAt: "t1", nextEligibleAt: "t2" }); await storage.upsertRepoSyncState("o/r", "prs", { nextEligibleAt: null }); assert.deepEqual(await storage.getRepoSyncStates("prs"), [ - { repo: "o/r", kind: "prs", lastSyncedAt: "t1", nextEligibleAt: null }, + { repo: "o/r", kind: "prs", lastSyncedAt: "t1", nextEligibleAt: null, githubOpenCount: null }, ]); }); +test("SqliteStorage persists githubOpenCount across reopen and partial updates", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "codefactory-storage-")); + const first = new SqliteStorage(root); + try { + await first.upsertRepoSyncState("o/r", "issues", { + lastSyncedAt: "2026-05-15T10:00:00.000Z", + githubOpenCount: 42, + }); + await first.upsertRepoSyncState("o/r", "issues", { + nextEligibleAt: "2026-05-15T12:00:00.000Z", + }); + const rows = await first.getRepoSyncStates("issues"); + assert.equal(rows[0]?.githubOpenCount, 42); + assert.equal(rows[0]?.lastSyncedAt, "2026-05-15T10:00:00.000Z"); + } finally { + first.close(); + } + + const second = new SqliteStorage(root); + try { + const rows = await second.getRepoSyncStates("issues"); + assert.equal(rows[0]?.githubOpenCount, 42); + assert.equal(rows[0]?.nextEligibleAt, "2026-05-15T12:00:00.000Z"); + } finally { + second.close(); + } +}); + test("both storages requeue a parked background job with a fresh attempt budget", async () => { // Goal: parking must be reversible without a human. Reviving resets the attempt count so the // job gets a full recovery cycle rather than failing again on its first tick. diff --git a/server/storage.ts b/server/storage.ts index c085a89..798bc1c 100644 --- a/server/storage.ts +++ b/server/storage.ts @@ -60,6 +60,7 @@ export type RepoSyncState = { kind: RepoSyncKind; lastSyncedAt: string | null; nextEligibleAt: string | null; + githubOpenCount: number | null; }; export interface IStorage { @@ -123,7 +124,8 @@ export interface IStorage { // GitHub conditional-request etags (If-None-Match), keyed by a stable request URL. getGithubEtag(url: string): Promise; - setGithubEtag(url: string, etag: string): Promise; + getGithubEtagRecord(url: string): Promise<{ etag: string; payload: string | null } | undefined>; + setGithubEtag(url: string, etag: string, payload?: string | null): Promise; clearGithubEtag(url: string): Promise; // Per-repo sync state (backoff + last-synced), persisted so a restart does @@ -132,7 +134,11 @@ export interface IStorage { upsertRepoSyncState( repo: string, kind: RepoSyncKind, - updates: { lastSyncedAt?: string | null; nextEligibleAt?: string | null }, + updates: { + lastSyncedAt?: string | null; + nextEligibleAt?: string | null; + githubOpenCount?: number | null; + }, ): Promise; // CI healing