From 6d094a2f5933b145bbb7541fe892757f78ba66bc Mon Sep 17 00:00:00 2001 From: Sayt-0 Date: Mon, 6 Jul 2026 17:21:34 +0200 Subject: [PATCH] fix(review-pr): enforce a coherent time budget across agent retries The review agent ran with timeout 2700s, max-retries 1 and retry-on-timeout 1 inside a 50-minute job: a first attempt timing out at 45 min launched a retry that GitHub always killed at the job ceiling, skipping the composite's always() steps (lock release, PR summary, reactions) and sometimes posting a duplicate review. - add total-timeout input: wall-clock budget across all attempts and retry delays, enforced by the runner (last attempt capped to the remaining budget, retry needs >= 60s left), so the agent step always ends before the job-level timeout and cleanup steps run - add no-retry-pattern input: skip remaining retries once the verbose log proves a side effect landed (pullrequestreview-N), preventing duplicate reviews on retry - review-pr: cap review at 2700s total, feedback processing at 300s, drop the doomed retry-on-timeout, raise job ceiling to 60 min as a safety net that should never fire - fix stale comments (1800s, 35-min budget) in action.yml and AGENTS.md --- .github/workflows/review-pr.yml | 8 ++- AGENTS.md | 2 +- README.md | 2 + action.yml | 8 +++ review-pr/action.yml | 18 +++-- src/main/__tests__/exec.test.ts | 120 +++++++++++++++++++++++++++++++- src/main/exec.ts | 81 ++++++++++++++++++--- src/main/index.ts | 4 ++ 8 files changed, 227 insertions(+), 16 deletions(-) diff --git a/.github/workflows/review-pr.yml b/.github/workflows/review-pr.yml index 50d5623..b86027a 100644 --- a/.github/workflows/review-pr.yml +++ b/.github/workflows/review-pr.yml @@ -188,7 +188,13 @@ jobs: (needs.resolve-context.result == 'success' && needs.resolve-context.outputs.trigger-event == 'pull_request') ) 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 diff --git a/AGENTS.md b/AGENTS.md index 402cbd9..ad9897a 100644 --- a/AGENTS.md +++ b/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---*` 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---*` 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---` 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: `` on review comments, `` on agent replies (including mention-reply responses). **Don't change these strings** — workflows in consumer repos grep for them. diff --git a/README.md b/README.md index 66ff8e8..660d898 100644 --- a/README.md +++ b/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` | diff --git a/action.yml b/action.yml index 35f9f0c..655f8a9 100644 --- a/action.yml +++ b/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 diff --git a/review-pr/action.yml b/review-pr/action.yml index 8d365af..caf7130 100644 --- a/review-pr/action.yml +++ b/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 }} diff --git a/src/main/__tests__/exec.test.ts b/src/main/__tests__/exec.test.ts index e8c2845..50a23ee 100644 --- a/src/main/__tests__/exec.test.ts +++ b/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[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'), + ); + }); +}); diff --git a/src/main/exec.ts b/src/main/exec.ts index 61e1d19..576afed 100644 --- a/src/main/exec.ts +++ b/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 { 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 { } 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 { 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 { env, stdinData, verboseLogFd, - timeoutSeconds: opts.timeout, + timeoutSeconds: attemptTimeout, }); } finally { fs.closeSync(verboseLogFd); @@ -328,20 +374,39 @@ export async function runAgent(opts: RunAgentOptions): Promise { } 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...`); } diff --git a/src/main/index.ts b/src/main/index.ts index c942fc7..3a5dca6 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -218,6 +218,8 @@ async function run(): Promise { const maxRetries = parseInt(core.getInput('max-retries') || '2', 10); const retryDelay = parseInt(core.getInput('retry-delay') || '5', 10); const retryOnTimeout = parseInt(core.getInput('retry-on-timeout') || '0', 10); + const totalTimeout = parseInt(core.getInput('total-timeout') || '0', 10); + const noRetryPattern = core.getInput('no-retry-pattern'); const yolo = core.getBooleanInput('yolo'); const workingDirectory = core.getInput('working-directory') || '.'; const extraArgs = core.getInput('extra-args'); @@ -344,6 +346,8 @@ async function run(): Promise { maxRetries, retryDelay, retryOnTimeout, + totalTimeout, + noRetryPattern, debug, anthropicApiKey, openaiApiKey,