Follow up the engine fetch's review: a fetch time limit, a testable skip decision, sturdier tests

README.md

@@ -60,10 +60,11 @@ jobs:
 ```
 
 `pull_request_target` and `issue_comment` both run this file from the default branch, so a pull request can't change
-its own review, and the pull request's code is only read, never built or run. The workflow doesn't check the head out: the
-engine fetches the exact commit itself, after it has decided to review, with git started without configuration, hooks or
-a credential helper (a workflow that checks the head out into `pr` itself is also accepted, if it is at that commit). Pushes start nothing; comment `/review`
-to review the latest commit. Pull requests from forks aren't reviewed, by either trigger.
+its own review, and the pull request's code is only read, never built or run. The workflow doesn't check the head
+out: the engine fetches the exact commit itself, after it has decided to review, with git started without
+configuration, hooks or a credential helper, and gives up on a fetch that takes longer than three minutes. A workflow
+that checks the head out into `pr` itself is also accepted, if it is at that commit. Pushes start nothing; comment
+`/review` to review the latest commit. Pull requests from forks aren't reviewed, by either trigger.
 
 ## Versions
 

src/command.ts

@@ -28,3 +28,28 @@
 export function isFromFork(pullRequest: { head?: { repo?: { full_name?: string } | null } } | undefined, repository: string): boolean {
   return pullRequest?.head?.repo?.full_name !== repository;
 }
+
+// Why a review is skipped, if it is: the pull request comes from a fork, or its head was already reviewed.
+// `reviewedHead` looks up the head the bot last reviewed; it is called only for a pull request that isn't from a fork
+// and has a head, so a fork costs no API call. /review passes none, because it reviews a commit again on purpose.
+export async function skipReason(
+  pullRequest: { number?: number; head?: { sha?: string; repo?: { full_name?: string } | null } } | undefined,
+  repository: string,
+  reviewedHead?: () => Promise<string | undefined>,
+): Promise<string | undefined> {
+  if (isFromFork(pullRequest, repository)) return `#${pullRequest?.number} comes from a fork, and forks aren't reviewed.`;
+  const head = pullRequest?.head?.sha;
+  if (head && reviewedHead && (await reviewedHead()) === head) return `${head.slice(0, 7)} was already reviewed; comment /review to review it again.`;
+  return undefined;
+}
+
+// The skip rules for one run. A /review comment reviews a commit again on purpose, so only the fork rule applies and the
+// reviewed head is never looked up; an automatic run also skips a head already reviewed.
+export function skipReasonFor(
+  trigger: 'comment' | 'automatic',
+  pullRequest: Parameters<typeof skipReason>[0],
+  repository: string,
+  reviewedHead: () => Promise<string | undefined>,
+): Promise<string | undefined> {
+  return skipReason(pullRequest, repository, trigger === 'automatic' ? reviewedHead : undefined);
+}

src/fetch-head.ts

@@ -13,6 +13,19 @@
 import os from 'node:os';
 import path from 'node:path';
 
+// How long a fetch may take before the run reports it instead of waiting for the job's own limit.
+export const fetchTimeoutMs = 180_000;
+
+// A failed fetch's folder is removed with these retries, about two seconds in all: a git killed at its time limit can leave
+// a child holding the folder for a moment.
+const cleanupRetries = 10;
+const cleanupRetryDelayMs = 200;
+
+// Removes the folder a failed fetch made. A test passes its own to make the removal fail.
+export function removeFolder(folder: string): void {
+  rmSync(folder, { recursive: true, force: true, maxRetries: cleanupRetries, retryDelay: cleanupRetryDelayMs });
+}
+
 export interface FetchHead {
   // The folder to fill. A folder that is already a repository at the named commit is left alone, as a workflow that
   // checks the head out itself expects.
@@ -23,6 +36,10 @@
   // The commit to check out.
   sha: string;
   token?: string;
+  // Replaces the default limit on each fetch, in milliseconds. It must be positive: there is no way to turn the limit off.
+  timeoutMs?: number;
+  // Replaces `removeFolder`, the cleanup after a failed fetch.
+  remove?: (folder: string) => void;
 }
 
 // The fetch's inputs from the parsed pull request: the commit is the one the event or the API named.
@@ -51,10 +68,13 @@
 
 // An empty folder stands in for the home directory, the hooks folder and the global configuration, so nothing a runner
 // or the checked-out code could hold there is read.
-function git(where: { cwd: string; empty: string }, args: string[], extra: Record<string, string> = {}): string {
+// `run.env` adds to git's environment (the token's header), and `run.timeout` is a limit in milliseconds.
+function git(where: { cwd: string; empty: string }, args: string[], run: { env?: Record<string, string>; timeout?: number } = {}): string {
   const { cwd, empty } = where;
+  const { env: extra = {}, timeout } = run;
   return execFileSync('git', ['-C', cwd, '-c', `core.hooksPath=${empty}`, '-c', 'core.fsmonitor=false', '-c', 'advice.detachedHead=false', ...args], {
     encoding: 'utf8',
+    ...(timeout === undefined ? {} : { timeout, killSignal: 'SIGKILL' as const }),
     stdio: ['ignore', 'pipe', 'inherit'],
     env: {
       PATH: process.env['PATH'] ?? '',
@@ -83,9 +103,12 @@
 export function fetchPullRequestHead(options: FetchHead): void {
   if (!/^[0-9a-f]{40}$/i.test(options.sha)) throw new Error(`"${options.sha}" isn't a full commit SHA.`);
   if (!Number.isInteger(options.number) || options.number < 1) throw new Error(`"${options.number}" isn't a pull request number.`);
+  const limit = options.timeoutMs ?? fetchTimeoutMs;
+  // Node takes a timeout only as a whole number of milliseconds that fits a 32-bit signed integer.
+  if (!Number.isInteger(limit) || limit < 1 || limit > 2 ** 31 - 1) throw new Error(`The fetch's time limit must be a positive whole number of milliseconds, not ${limit}.`);
   const empty = mkdtempSync(path.join(os.tmpdir(), 'code-review-git-'));
   const where = { cwd: options.path, empty };
   try {
     if (existsSync(path.join(options.path, '.git'))) {
       let head: string;
       try {
@@ -106,13 +129,20 @@
       git(where, ['remote', 'add', 'origin', options.remoteUrl]);
       // The branches give the merge base its history; the pull request's own ref carries the head, which a pull request
       // may not have on any branch yet.
-      git(where, ['fetch', '-q', '--no-tags', '--prune', 'origin', '+refs/heads/*:refs/remotes/origin/*', `+refs/pull/${options.number}/head:refs/remotes/pull/${options.number}`], auth);
+      git(where, ['fetch', '-q', '--no-tags', '--prune', 'origin', '+refs/heads/*:refs/remotes/origin/*', `+refs/pull/${options.number}/head:refs/remotes/pull/${options.number}`], { env: auth, timeout: limit });
       // A push after the event leaves the event's commit off both, so ask for it by name.
-      if (!has(where, options.sha)) git(where, ['fetch', '-q', '--no-tags', 'origin', options.sha], auth);
+      if (!has(where, options.sha)) git(where, ['fetch', '-q', '--no-tags', 'origin', options.sha], { env: auth, timeout: limit });
       git(where, ['checkout', '-q', '--force', '--detach', options.sha]);
     } catch (error) {
-      // A half-made repository would be taken for a checkout by the next attempt.
-      rmSync(options.path, { recursive: true, force: true });
+      // A half-made repository would be taken for a checkout by the next attempt, which refuses one with no commit. A
+      // git killed at its time limit can leave a child holding the folder for a moment, so retry, and never let a failed
+      // cleanup hide the fetch's own error.
+      try {
+        (options.remove ?? removeFolder)(options.path);
+      } catch (cleanupError) {
+        // The fetch's error below stays the one thrown; this says why the next attempt will find the folder.
+        console.error(`Couldn't remove ${options.path} after the failed fetch: ${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}`);
+      }
       const detail = error instanceof Error ? error.message : String(error);
       throw new Error(`Couldn't fetch commit ${options.sha.slice(0, 7)} of pull request #${options.number}; if it was pushed over since, comment /review to review the latest one. (${detail})`);
     }

src/main.ts

@@ -4,7 +4,7 @@
 import { appendFileSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs';
 import path from 'node:path';
 import { fileURLToPath } from 'node:url';
-import { commandFromEvent, isFromFork } from './command.ts';
+import { commandFromEvent, skipReasonFor } from './command.ts';
 import { AnthropicTokenSource, secretValues, type Credentials } from './credentials.ts';
 import { fetchHeadFor, fetchPullRequestHead } from './fetch-head.ts';
 import { closedIssues, gather, maxIssueChars, readPullRequest } from './gather.ts';
@@ -85,22 +85,20 @@
       headers: { accept: 'application/vnd.github+json', ...(githubToken ? { authorization: `Bearer ${githubToken}` } : {}) },
     });
     if (!response.ok) throw new Error(`Couldn't read pull request #${request.number}: HTTP ${response.status}`);
-    const pullRequest = (await response.json()) as { head?: { repo?: { full_name?: string } } };
-    if (isFromFork(pullRequest, repository)) {
-      console.log(`#${request.number} comes from a fork, and forks aren't reviewed.`);
+    const pullRequest = (await response.json()) as { number?: number; head?: { sha?: string; repo?: { full_name?: string } | null } };
+    const reason = await skipReasonFor('comment', pullRequest, repository, () => lastReviewedHead(repository, request.number, githubToken));
+    if (reason) {
+      console.log(reason);
       return;
     }
     event = { pull_request: pullRequest };
   } else {
-    if (isFromFork(event.pull_request, repository)) {
-      console.log(`#${event.pull_request?.number} comes from a fork, and forks aren't reviewed.`);
-      return;
-    }
     // An automatic run on a commit already reviewed, such as marking a draft ready without pushing, would pay again for
     // the same review. /review still reviews it.
-    const head = event.pull_request?.head?.sha;
-    if (head && (await lastReviewedHead(repository, event.pull_request.number, githubToken)) === head) {
-      console.log(`${String(head).slice(0, 7)} was already reviewed; comment /review to review it again.`);
+    const pullRequest = event.pull_request;
+    const reason = await skipReasonFor('automatic', pullRequest, repository, () => lastReviewedHead(repository, pullRequest.number, githubToken));
+    if (reason) {
+      console.log(reason);
       return;
     }
   }

test/command.test.ts

@@ -0,0 +1,77 @@
+import assert from 'node:assert/strict';
+import { test } from 'node:test';
+import { commandFromEvent, isFromFork, skipReason, skipReasonFor } from '../src/command.ts';
+
+test('commandFromEvent accepts only an exact /review from a writer on a pull request', () => {
+  const event = (body: string, association = 'MEMBER', pullRequest: unknown = {}) => ({
+    issue: { number: 7, pull_request: pullRequest },
+    comment: { body, author_association: association },
+  });
+  assert.deepEqual(commandFromEvent(event('/review')), { command: { kind: 'review' }, number: 7 });
+  assert.deepEqual(commandFromEvent(event('  /review\n')), { command: { kind: 'review' }, number: 7 });
+  assert.equal(commandFromEvent(event('/review please')), undefined);
+  assert.equal(commandFromEvent(event('Run /review again')), undefined);
+  assert.equal(commandFromEvent(event('/review', 'CONTRIBUTOR')), undefined);
+  assert.equal(commandFromEvent(event('/review', 'NONE')), undefined);
+  assert.equal(commandFromEvent(event('/review', 'MEMBER', null)), undefined);
+});
+
+test('isFromFork treats a head in another repository, or in none, as a fork', () => {
+  assert.equal(isFromFork({ head: { repo: { full_name: 'NCodeGroup/code-review' } } }, 'NCodeGroup/code-review'), false);
+  assert.equal(isFromFork({ head: { repo: { full_name: 'someone/code-review' } } }, 'NCodeGroup/code-review'), true);
+  assert.equal(isFromFork({ head: { repo: null } }, 'NCodeGroup/code-review'), true);
+  assert.equal(isFromFork({ head: {} }, 'NCodeGroup/code-review'), true);
+  assert.equal(isFromFork(undefined, 'NCodeGroup/code-review'), true);
+});
+
+const own = { number: 7, head: { sha: 'a'.repeat(40), repo: { full_name: 'NCodeGroup/code-review' } } };
+
+// A lookup that records whether it was called, and answers with the given head.
+function lookup(head: string | undefined): { calls: number; fn: () => Promise<string | undefined> } {
+  const result = { calls: 0, fn: async () => { result.calls++; return head; } };
+  return result;
+}
+
+test('skipReason skips a pull request from a fork or a deleted fork, without looking up the reviewed head', async () => {
+  const looked = lookup('a'.repeat(40));
+  assert.match((await skipReason({ ...own, head: { ...own.head, repo: { full_name: 'someone/code-review' } } }, 'NCodeGroup/code-review', looked.fn)) ?? '', /#7 comes from a fork/);
+  assert.match((await skipReason({ ...own, head: { sha: own.head.sha, repo: null } }, 'NCodeGroup/code-review', looked.fn)) ?? '', /comes from a fork/);
+  assert.equal(looked.calls, 0);
+});
+
+test('skipReason skips an automatic review of a head already reviewed', async () => {
+  assert.match((await skipReason(own, 'NCodeGroup/code-review', lookup('a'.repeat(40)).fn)) ?? '', /aaaaaaa was already reviewed; comment \/review/);
+});
+
+test('skipReason reviews a head not yet reviewed', async () => {
+  assert.equal(await skipReason(own, 'NCodeGroup/code-review', lookup('b'.repeat(40)).fn), undefined);
+});
+
+test('skipReason reviews when no lookup is passed', async () => {
+  assert.equal(await skipReason(own, 'NCodeGroup/code-review'), undefined);
+});
+
+test('skipReason reviews a pull request with no head commit, without looking up the reviewed head', async () => {
+  const looked = lookup('a'.repeat(40));
+  assert.equal(await skipReason({ number: 7, head: { repo: { full_name: 'NCodeGroup/code-review' } } }, 'NCodeGroup/code-review', looked.fn), undefined);
+  assert.equal(looked.calls, 0);
+});
+
+test('skipReasonFor reviews a head already reviewed again for a /review comment, without looking it up', async () => {
+  const looked = lookup('a'.repeat(40));
+  assert.equal(await skipReasonFor('comment', own, 'NCodeGroup/code-review', looked.fn), undefined);
+  assert.equal(looked.calls, 0);
+});
+
+test('skipReasonFor skips a head already reviewed for an automatic run', async () => {
+  const looked = lookup('a'.repeat(40));
+  assert.match((await skipReasonFor('automatic', own, 'NCodeGroup/code-review', looked.fn)) ?? '', /already reviewed/);
+  assert.equal(looked.calls, 1);
+});
+
+test('skipReasonFor skips a fork for either trigger', async () => {
+  const fork = { ...own, head: { ...own.head, repo: { full_name: 'someone/code-review' } } };
+  for (const trigger of ['comment', 'automatic'] as const) {
+    assert.match((await skipReasonFor(trigger, fork, 'NCodeGroup/code-review', lookup(undefined).fn)) ?? '', /comes from a fork/);
+  }
+});

test/engine.test.ts

@@ -214,21 +214,6 @@ test('checkTitle counts findings by severity', () => {
   assert.equal(checkTitle(results), '1 blocker, 0 should-fix, 1 nit (shadow mode)');
 });
 
-test('commandFromEvent accepts only an exact /review from a writer on a pull request', async () => {
-  const { commandFromEvent } = await import('../src/command.ts');
-  const event = (body: string, association = 'MEMBER', pullRequest: unknown = {}) => ({
-    issue: { number: 7, pull_request: pullRequest },
-    comment: { body, author_association: association },
-  });
-  assert.deepEqual(commandFromEvent(event('/review')), { command: { kind: 'review' }, number: 7 });
-  assert.deepEqual(commandFromEvent(event('  /review\n')), { command: { kind: 'review' }, number: 7 });
-  assert.equal(commandFromEvent(event('/review please')), undefined);
-  assert.equal(commandFromEvent(event('Run /review again')), undefined);
-  assert.equal(commandFromEvent(event('/review', 'CONTRIBUTOR')), undefined);
-  assert.equal(commandFromEvent(event('/review', 'NONE')), undefined);
-  assert.equal(commandFromEvent(event('/review', 'MEMBER', null)), undefined);
-});
-
 test('summaryComment shows reviewers, models, counts and the worst severity', async () => {
   const { summaryComment, summaryMarker, shortModel, topSeverity } = await import('../src/results.ts');
   assert.equal(shortModel('global.anthropic.claude-haiku-5-5'), 'haiku-5-5');