fix(review-pr): enforce a coherent time budget across agent retries

.github/workflows/review-pr.yml

@@ -235,7 +235,13 @@ jobs:
          (needs.resolve-context.outputs.requested-reviewer == 'docker-agent' || needs.resolve-context.outputs.requested-reviewer == ''))
       )
     runs-on: ubuntu-latest
-    timeout-minutes: 50
+    # Budget: review agent hard-capped at 2700 s total + feedback processing at
+    # 300 s (both runner-enforced via total-timeout in review-pr/action.yml),
+    # leaving ~10 min for preamble and cleanup. The agent can never outlive its
+    # own budget, so this ceiling is a safety net that should never fire — which
+    # keeps the composite's always() steps (lock release, summary, reactions)
+    # running on every path.
+    timeout-minutes: 60
     permissions:
       contents: read
       pull-requests: write

AGENTS.md

@@ -172,7 +172,7 @@ The action runs untrusted input (PR titles, bodies, comments, diffs) through an
 
 ### `review-pr` action specifics
 
-- Uses a **best-effort cache lock** (`pr-review-lock-<repo>-<pr>-*` cache key) to avoid concurrent reviews on the same PR. Lock TTL is 600s; the agent execution timeout is 1800s (30 min) — these are intentionally decoupled. Reviews are idempotent so the small race window is acceptable.
+- Uses a **best-effort cache lock** (`pr-review-lock-<repo>-<pr>-*` cache key) to avoid concurrent reviews on the same PR. Lock TTL is 600s; the review agent's wall-clock budget is 2700s (45 min, enforced by the root action's `total-timeout` across all attempts) — these are intentionally decoupled. Reviews are idempotent so the small race window is acceptable.
 - **Memory persistence** uses `actions/cache` keyed by `pr-review-memory-<repo>-<job>-<run_id>` with prefix-based restore. The DB lives at `${{ github.workspace }}/.cache/pr-review-memory.db`.
 - **Feedback loop**: the `reply-to-feedback` job in `.github/workflows/review-pr.yml` (which runs the `pr-review-reply.yaml` agent) uploads a `pr-review-feedback` artifact on every reply via its "Upload feedback artifact" step. The next review run downloads all such artifacts, runs `pr-review-feedback.yaml` to call `add_memory(...)` for each, then deletes the artifacts.
 - **Bot reply detection** uses HTML markers: `<!-- docker-agent-review -->` on review comments, `<!-- docker-agent-review-reply -->` on agent replies (including mention-reply responses). **Don't change these strings** — workflows in consumer repos grep for them.

README.md

@@ -114,6 +114,8 @@ To report a vulnerability, see our [Security Policy](SECURITY.md).
 | `yolo`                     | Auto-approve all prompts (`true`/`false`)                                            | No       | `true`         |
 | `max-retries`              | Maximum number of retries on failure (0 = no retries)                                | No       | `2`            |
 | `retry-delay`              | Base delay in seconds between retries (doubles each attempt)                         | No       | `5`            |
+| `total-timeout`            | Total wall-clock budget in seconds across all attempts and retry delays (0 = unlimited) | No       | `0`            |
+| `no-retry-pattern`         | Regex tested against the agent log after a failed attempt; on match, retries are skipped | No       | -              |
 | `extra-args`               | Additional arguments to pass to `docker agent run`                                   | No       | -              |
 | `add-prompt-files`         | Comma-separated list of files to append to the prompt (e.g., `AGENTS.md,CLAUDE.md`)  | No       | -              |
 | `skip-summary`             | Skip writing agent output to the job summary (useful when callers write their own)  | No       | `false`        |

action.yml

@@ -75,6 +75,14 @@ inputs:
     description: "Number of additional retry attempts when the agent times out (exit code 124). Independent of max-retries — both budgets can be consumed in the same run. Default 0 = no timeout retries."
     required: false
     default: "0"
+  total-timeout:
+    description: "Total wall-clock budget in seconds across all attempts, retry delays included (0 = unlimited). The last attempt is capped to the remaining budget and a retry only starts with at least 60 s left, so the action always finishes within this budget. Set it below the job's timeout-minutes to keep post-run cleanup steps alive."
+    required: false
+    default: "0"
+  no-retry-pattern:
+    description: "Regex tested against the verbose agent log after a failed attempt. On match, remaining retries are skipped — use it when a retry would repeat a side effect the failed attempt already committed (e.g. 'pullrequestreview-[0-9]+' once a PR review was posted). Empty = disabled."
+    required: false
+    default: ""
   extra-args:
     description: "Additional arguments to pass to docker agent run"
     required: false

review-pr/action.yml

@@ -173,7 +173,7 @@ runs:
           LOCK_TIME=$(cat .cache/review-lock/timestamp 2>/dev/null || echo "0")
           NOW=$(date +%s)
           AGE=$(( NOW - LOCK_TIME ))
-          if [ "$AGE" -lt 600 ]; then  # 600 s lock TTL — intentionally decoupled from the review timeout (now 1800 s)
+          if [ "$AGE" -lt 600 ]; then  # 600 s lock TTL — intentionally decoupled from the review agent's 2700 s total budget
             echo "⏭️ Review already in progress (started ${AGE}s ago) — skipping"
             echo "skip=true" >> $GITHUB_OUTPUT
             echo "skip-reason=concurrent" >> $GITHUB_OUTPUT
@@ -694,7 +694,8 @@ runs:
           3. If they're agreeing and adding context, store the additional insight
 
           Use add_memory to record what you learned from each feedback item.
-        timeout: "180"  # 3 min — haiku call; prevents hang from eating the 35-min job budget
+        timeout: "180"  # 3 min per attempt — haiku call
+        total-timeout: "300"  # 5 min hard cap across attempts so feedback processing can't eat the review budget
         anthropic-api-key: ${{ inputs.anthropic-api-key }}
         openai-api-key: ${{ inputs.openai-api-key }}
         google-api-key: ${{ inputs.google-api-key }}
@@ -867,7 +868,14 @@ runs:
       with:
         agent: ${{ env.ACTION_PATH }}/agents/pr-review.yaml
         prompt: ${{ steps.context.outputs.review_prompt }}
-        timeout: "2700"  # 45 min — allows complex reviews to complete; lock TTL (600 s) is intentionally shorter
+        # Budget: total-timeout (2700 s) is the hard wall-clock cap for this step,
+        # attempts and retry delays included, so it always fits the caller's job
+        # timeout with room for the cleanup steps below (lock release, summary,
+        # reactions). No retry-on-timeout: a second 45-min pass can never fit the
+        # same budget, so a timed-out review is surfaced to the PR for a manual
+        # re-request instead of silently burning a doomed retry.
+        timeout: "2700"  # 45 min per attempt — allows complex reviews to complete; lock TTL (600 s) is intentionally shorter
+        total-timeout: "2700"
         anthropic-api-key: ${{ inputs.anthropic-api-key }}
         openai-api-key: ${{ inputs.openai-api-key }}
         google-api-key: ${{ inputs.google-api-key }}
@@ -878,8 +886,8 @@ runs:
         github-token: ${{ steps.resolve-token.outputs.token }}
         extra-args: ${{ inputs.model && format('--model={0}', inputs.model) || '' }}
         add-prompt-files: ${{ inputs.add-prompt-files }}
-        max-retries: "1"  # One retry handles transient API failures (e.g. Anthropic 400s) without risking duplicate reviews from full pipeline restarts
-        retry-on-timeout: "1"  # Retry once on timeout — agent may succeed on a second pass after a transient infra hiccup
+        max-retries: "1"  # One retry handles transient API failures (e.g. Anthropic 400s); total-timeout caps the sum of both attempts
+        no-retry-pattern: "pullrequestreview-[0-9]+"  # Never restart the pipeline once a review was posted — a retry would post a duplicate
         skip-summary: "true"
         org-membership-token: ${{ inputs.org-membership-token }}
         auth-org: ${{ inputs.auth-org }}

src/main/__tests__/exec.test.ts

@@ -29,7 +29,7 @@ vi.mock('node:child_process', () => ({
   spawn: mockSpawn,
 }));
 
-import { buildArgs, runAgent, TIMEOUT_EXIT_CODE } from '../exec.js';
+import { buildArgs, MIN_RETRY_BUDGET_SECONDS, runAgent, TIMEOUT_EXIT_CODE } from '../exec.js';
 
 // ── Helpers ─────────────────────────────────────────────────────────────────
 
@@ -70,6 +70,8 @@ function baseOpts(overrides: Partial<Parameters<typeof runAgent>[0]> = {}) {
     maxRetries: 0,
     retryDelay: 0,
     retryOnTimeout: 0,
+    totalTimeout: 0,
+    noRetryPattern: '',
     debug: false,
     anthropicApiKey: 'sk-ant-test',
     telemetryTags: 'source=test',
@@ -390,3 +392,119 @@ describe('runAgent', () => {
     expect(envPassed.TELEMETRY_TAGS).toBe('source=ci');
   });
 });
+
+// ── total-timeout budget ──────────────────────────────────────────────────────────────────
+
+describe('runAgent total-timeout', () => {
+  it('enforces the total budget as a kill timer even when per-attempt timeout is 0', async () => {
+    // Child would exit naturally in 5 s; the 50 ms total budget must kill it first.
+    const child = makeMockChild(0, 5000);
+    mockSpawn.mockReturnValue(child);
+
+    const result = await runAgent(baseOpts({ timeout: 0, totalTimeout: 0.05, maxRetries: 0 }));
+
+    expect(result.exitCode).toBe(TIMEOUT_EXIT_CODE);
+    expect(child.kill).toHaveBeenCalledWith('SIGTERM');
+  }, 3000);
+
+  it('skips the retry when the remaining budget is under the minimum floor', async () => {
+    mockSpawn.mockImplementation(() => makeMockChild(1));
+
+    // Budget (10 s) minus elapsed leaves < MIN_RETRY_BUDGET_SECONDS (60) → no retry.
+    const result = await runAgent(baseOpts({ maxRetries: 2, retryDelay: 0, totalTimeout: 10 }));
+
+    expect(result.exitCode).toBe(1);
+    expect(mockSpawn).toHaveBeenCalledOnce();
+    expect(MIN_RETRY_BUDGET_SECONDS).toBeGreaterThan(10);
+  });
+
+  it('allows retries while the remaining budget stays above the floor', async () => {
+    mockSpawn
+      .mockImplementationOnce(() => makeMockChild(1))
+      .mockImplementation(() => makeMockChild(0));
+
+    const result = await runAgent(baseOpts({ maxRetries: 1, retryDelay: 0, totalTimeout: 300 }));
+
+    expect(result.exitCode).toBe(0);
+    expect(mockSpawn).toHaveBeenCalledTimes(2);
+  });
+
+  it('caps the per-attempt timeout to the remaining budget', async () => {
+    // totalTimeout (0.05 s) < timeout (600 s): the budget, not the attempt cap,
+    // must kill the child.
+    const child = makeMockChild(0, 5000);
+    mockSpawn.mockReturnValue(child);
+
+    const result = await runAgent(baseOpts({ timeout: 600, totalTimeout: 0.05, maxRetries: 0 }));
+
+    expect(result.exitCode).toBe(TIMEOUT_EXIT_CODE);
+    expect(child.kill).toHaveBeenCalledWith('SIGTERM');
+  }, 3000);
+});
+
+// ── no-retry-pattern ──────────────────────────────────────────────────────────────────────
+
+describe('runAgent no-retry-pattern', () => {
+  it('skips retries when the verbose log matches the pattern', async () => {
+    mockSpawn.mockImplementation(() => {
+      // Simulate the agent posting a review before failing.
+      fsSync.appendFileSync(verboseLogFile, 'posted pullrequestreview-123456\n', 'utf-8');
+      return makeMockChild(1);
+    });
+
+    const result = await runAgent(
+      baseOpts({ maxRetries: 2, retryDelay: 0, noRetryPattern: 'pullrequestreview-[0-9]+' }),
+    );
+
+    expect(result.exitCode).toBe(1);
+    expect(mockSpawn).toHaveBeenCalledOnce();
+  });
+
+  it('skips timeout retries when the verbose log matches the pattern', async () => {
+    mockSpawn.mockImplementation(() => {
+      fsSync.appendFileSync(verboseLogFile, 'posted pullrequestreview-123456\n', 'utf-8');
+      return makeMockChild(TIMEOUT_EXIT_CODE);
+    });
+
+    const result = await runAgent(
+      baseOpts({
+        retryOnTimeout: 1,
+        retryDelay: 0,
+        noRetryPattern: 'pullrequestreview-[0-9]+',
+      }),
+    );
+
+    expect(result.exitCode).toBe(TIMEOUT_EXIT_CODE);
+    expect(mockSpawn).toHaveBeenCalledOnce();
+  });
+
+  it('retries normally when the log does not match', async () => {
+    mockSpawn
+      .mockImplementationOnce(() => makeMockChild(1))
+      .mockImplementation(() => makeMockChild(0));
+
+    const result = await runAgent(
+      baseOpts({ maxRetries: 1, retryDelay: 0, noRetryPattern: 'pullrequestreview-[0-9]+' }),
+    );
+
+    expect(result.exitCode).toBe(0);
+    expect(mockSpawn).toHaveBeenCalledTimes(2);
+  });
+
+  it('ignores an invalid regex and keeps retrying', async () => {
+    const { warning } = await import('@actions/core');
+    mockSpawn
+      .mockImplementationOnce(() => makeMockChild(1))
+      .mockImplementation(() => makeMockChild(0));
+
+    const result = await runAgent(
+      baseOpts({ maxRetries: 1, retryDelay: 0, noRetryPattern: '([unclosed' }),
+    );
+
+    expect(result.exitCode).toBe(0);
+    expect(mockSpawn).toHaveBeenCalledTimes(2);
+    expect(vi.mocked(warning)).toHaveBeenCalledWith(
+      expect.stringContaining('Invalid no-retry-pattern'),
+    );
+  });
+});

src/main/exec.ts

@@ -11,8 +11,13 @@
  *   - Keys are registered with core.setSecret() BEFORE any exec call
  *   - Prompt is passed via stdin (from sanitized file or raw string)
  *   - stdout + stderr go to verbose log file (keeps runner console clean)
- *   - Exit code 124 = timeout (no retry)
+ *   - Exit code 124 = timeout (retried only within the retry-on-timeout budget)
  *   - Retry loop with exponential backoff
+ *   - Optional total-timeout budget spanning all attempts: the last attempt is
+ *     capped to the remaining budget and a retry needs >= 60 s left to start,
+ *     so the loop always ends before a job-level timeout-minutes kill
+ *   - Optional no-retry-pattern: retries are skipped once the verbose log
+ *     proves a side effect already happened (e.g. a PR review was posted)
  *   - On retry: truncate clean output file, append separator to verbose log
  *   - SIGTERM on timeout, exit code reported as 124
  */
@@ -24,6 +29,9 @@ import * as core from '@actions/core';
 
 export const TIMEOUT_EXIT_CODE = 124;
 
+/** Minimum remaining total-timeout budget (seconds) for a retry to be worth starting. */
+export const MIN_RETRY_BUDGET_SECONDS = 60;
+
 export interface RunAgentOptions {
   /** Absolute path to the docker-agent binary. */
   dockerAgentPath: string;
@@ -49,6 +57,10 @@ export interface RunAgentOptions {
   retryDelay: number;
   /** Number of additional retry attempts allowed when the agent times out (exit 124). */
   retryOnTimeout: number;
+  /** Total wall-clock budget in seconds across all attempts, retries and delays (0 = unlimited). */
+  totalTimeout: number;
+  /** Regex source tested against the verbose log after a failed attempt; on match, retries are skipped. */
+  noRetryPattern: string;
   /** Whether debug mode is enabled. */
   debug: boolean;
 
@@ -131,6 +143,18 @@ function sleep(seconds: number): Promise<void> {
   return new Promise((resolve) => setTimeout(resolve, seconds * 1000));
 }
 
+/**
+ * Test the verbose log against the no-retry pattern. Fails open on read errors:
+ * an unreadable log cannot prove a side effect happened, so retrying stays allowed.
+ */
+function verboseLogMatches(logFile: string, pattern: RegExp): boolean {
+  try {
+    return pattern.test(fs.readFileSync(logFile, 'utf-8'));
+  } catch {
+    return false;
+  }
+}
+
 /**
  * Spawn docker-agent as a child process, piping stdin from the prompt
  * and stdout+stderr to the verbose log file.
@@ -277,6 +301,19 @@ export async function runAgent(opts: RunAgentOptions): Promise<RunAgentResult> {
   }
 
   const startTime = Date.now();
+
+  let noRetryRegex: RegExp | null = null;
+  if (opts.noRetryPattern) {
+    try {
+      noRetryRegex = new RegExp(opts.noRetryPattern);
+    } catch {
+      core.warning(`Invalid no-retry-pattern /${opts.noRetryPattern}/ - ignoring it`);
+    }
+  }
+
+  const deadline =
+    opts.totalTimeout > 0 ? startTime + opts.totalTimeout * 1000 : Number.POSITIVE_INFINITY;
+
   let exitCode = 1;
   let totalAttempt = 0;
   // Track the two retry budgets independently so mixed-failure sequences
@@ -307,6 +344,15 @@ export async function runAgent(opts: RunAgentOptions): Promise<RunAgentResult> {
       fs.appendFileSync(opts.verboseLogFile, separator, 'utf-8');
     }
 
+    // Cap the attempt to whatever remains of the total budget so the loop can
+    // never outlive it (a runner-enforced kill keeps job-level cleanup alive).
+    // Clamped to >= 1 s: a zero/negative value would disable spawnAgent's timer.
+    let attemptTimeout = opts.timeout;
+    if (deadline !== Number.POSITIVE_INFINITY) {
+      const remaining = Math.max((deadline - Date.now()) / 1000, 1);
+      attemptTimeout = opts.timeout > 0 ? Math.min(opts.timeout, remaining) : remaining;
+    }
+
     // Open verbose log fd for appending
     const verboseLogFd = fs.openSync(opts.verboseLogFile, 'a');
 
@@ -317,7 +363,7 @@ export async function runAgent(opts: RunAgentOptions): Promise<RunAgentResult> {
         env,
         stdinData,
         verboseLogFd,
-        timeoutSeconds: opts.timeout,
+        timeoutSeconds: attemptTimeout,
       });
     } finally {
       fs.closeSync(verboseLogFd);
@@ -328,20 +374,39 @@ export async function runAgent(opts: RunAgentOptions): Promise<RunAgentResult> {
     }
 
     if (exitCode === TIMEOUT_EXIT_CODE) {
-      core.error(`Agent execution timed out after ${opts.timeout} seconds`);
+      core.error(`Agent execution timed out after ${Math.round(attemptTimeout)} seconds`);
       if (timeoutRetryCount >= opts.retryOnTimeout) {
         break; // Timeout retry budget exhausted
       }
+    } else if (failureRetryCount >= opts.maxRetries) {
+      core.warning(`Agent failed after ${opts.maxRetries} retries (exit code: ${exitCode})`);
+      break;
+    }
+
+    // A retry is available. Cross-cutting guards run before a retry budget is
+    // consumed so a skipped retry never counts against either budget.
+    if (noRetryRegex && verboseLogMatches(opts.verboseLogFile, noRetryRegex)) {
+      core.warning(
+        'Skipping retry: verbose log matches no-retry-pattern (a side effect of the failed attempt may already be committed)',
+      );
+      break;
+    }
+    if (deadline !== Number.POSITIVE_INFINITY) {
+      const remainingAfterDelay = (deadline - Date.now()) / 1000 - currentDelay;
+      if (remainingAfterDelay < MIN_RETRY_BUDGET_SECONDS) {
+        core.warning(
+          `Skipping retry: less than ${MIN_RETRY_BUDGET_SECONDS}s of the ${opts.totalTimeout}s total-timeout budget remains`,
+        );
+        break;
+      }
+    }
+
+    if (exitCode === TIMEOUT_EXIT_CODE) {
       timeoutRetryCount++;
       core.warning(
         `Timeout — will retry (${timeoutRetryCount}/${opts.retryOnTimeout} timeout retries used)`,
       );
-      // fall through to retry
     } else {
-      if (failureRetryCount >= opts.maxRetries) {
-        core.warning(`Agent failed after ${opts.maxRetries} retries (exit code: ${exitCode})`);
-        break;
-      }
       failureRetryCount++;
       core.warning(`Agent failed (exit code: ${exitCode}), will retry...`);
     }