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

[Sayt-0]

Summary

The review agent could consume up to 2 x 2700 s (plus up to ~9 min of feedback-processing retries) inside a job capped at 50 minutes. A first attempt timing out at 45 min launched a retry that GitHub always killed at the job ceiling: wasted retry, cancelled run, and the composite's always() steps (lock release, summary, reactions) never ran, leaving a residual lock and no feedback on the PR. A retry after a partial failure could also post a duplicate review.

Changes

Runner: two new root-action inputs (src/main/exec.ts, action.yml)

Input	Behavior
total-timeout (seconds, 0 = unlimited)	Wall-clock budget across all attempts and retry delays. Each attempt is capped to the remaining budget; a retry only starts with at least 60 s left. The runner ends the agent step itself, so a job-level timeout-minutes kill never fires and cleanup steps stay alive.
no-retry-pattern (regex)	Tested against the verbose log after a failed attempt. On match, remaining retries are skipped without consuming a retry budget. Fail-open: invalid regex or unreadable log just disables the guard.

Budget rewiring (review-pr/action.yml, review-pr.yml)

Step	Worst case before	Worst case after
Feedback processing	180 s x 3 attempts + delays (~9.4 min)	total-timeout: 300 (5 min hard)
Run PR Review	2700 s x 2 via max-retries + retry-on-timeout (90+ min)	total-timeout: 2700 (45 min hard)
Job ceiling	50 min, killed mid-composite	60 min safety net that should never fire

timeout-minutes: 60 (safety net)
└─ review step, total-timeout 2700 s (runner-enforced)
   ├─ attempt 1: capped to min(2700, remaining budget)
   └─ retry: only if >= 60 s remain AND log does not match no-retry-pattern

• retry-on-timeout: 1 removed: a second 45 min pass can never fit the budget; timeouts are already surfaced on the PR for a manual re-request.
• no-retry-pattern: pullrequestreview-[0-9]+ (the same marker the "Post clean summary" step greps to detect a posted review): no duplicate review on retry.
• Stale comments fixed: "now 1800 s" (review-pr/action.yml), "35-min job budget" (review-pr/action.yml), "1800s (30 min)" (AGENTS.md).

Issue expectations mapping

Issue point	Handled by
2 x 2700 s agent in a 50 min job	total-timeout: 2700 on the review step, retry-on-timeout removed
Retry always killed by GitHub at 50 min	Runner-enforced budget ends before the job ceiling; ceiling raised to 60 min as safety net
Composite always() steps skipped (residual lock, no PR feedback)	Job-level kill can no longer interrupt the composite
Stale comments (1800 s, 35-min budget)	Updated in review-pr/action.yml, AGENTS.md
Bonus: duplicate review posted by retry	no-retry-pattern guard
Must not hinder other open PRs	See below

Impact on other open PRs and consumers

• New inputs default to inert values (0 / empty): every other root-action consumer keeps identical behavior.
• review-pr/action.yml references the root action by pinned SHA (v2.0.2): the new inputs stay inactive until the next release re-pins the chain (release.yml 3-pass), so no mixed state is possible.
• Per-PR concurrency group and cancel-in-progress: false untouched: one PR's budget never affects another PR's review.
• Merge order against open PRs review-pr: Avoid OIDC for direct API key reviews #40, fix(review-pr): gate trigger-path reviews on the requested reviewer #42, fix: skip resolve-context gracefully when trigger artifact is absent #43, chore: update Docker Agent to v1.99.0 #47 verified by local merge simulation: only review-pr: Avoid OIDC for direct API key reviews #40 produces a single adjacency conflict in review-pr.yml (its env: block replaces the permissions: block right below the timeout-minutes line); resolution keeps both sides verbatim, whichever lands second.

Validation

Check	Result
pnpm build	pass
pnpm test (753 tests, 8 new for total-timeout / no-retry-pattern)	pass
pnpm test:integration	pass
pnpm lint (Biome + tsc + actionlint)	pass
tests/test-job-summary.sh, tests/test-output-extraction.sh	pass

[docker-agent]

Assessment: 🟢 APPROVE

The time-budget enforcement logic is sound. The two medium-severity hypotheses raised during analysis were both dismissed after careful code tracing:

• "Budget guard subtracts already-doubled delay" — DISMISSED. At line 395, currentDelay holds the post-doubled value, which is exactly the duration of the upcoming sleep on the next iteration. The guard correctly asks: "after sleeping currentDelay seconds, will at least 60 s remain?" — this is the intended conservative check and there is no off-by-one.

• "Negative remaining budget spawns agent with 1-second timeout" — DISMISSED. The guard at line 395 (checked after each failed attempt) guarantees remainingAfterDelay >= 60 s before allowing a retry. After sleeping currentDelay, roughly 60 s remain when line 352 is reached, so Math.max((deadline - Date.now()) / 1000, 1) never clamps to 1 s in practice.

The two low-severity configuration-clarity observations (feedback step max-retries vs total-timeout=300; review step identical timeout and total-timeout) were noted in YAML wiring not changed by this PR's diff and are therefore out of scope.

[derekmisler]

🟢 APPROVE

solid fix, the core logic in exec.ts is clean and the budget math holds up. one nitpick below about parseInt vs parseFloat (super low-risk in practice, nobody passes 0.5 seconds as a real budget, but worth fixing for consistency with the test layer). also, with timeout: "2700" and total-timeout: "2700" both set to the same value, only a quick-fail retry can actually start; a timed-out attempt exhausts the budget completely, so max-retries: "1" only kicks in for the != 124 path. that's the intended behavior per the PR description, just worth a quick comment confirming it's deliberate.

Lower-confidence findings (not posted inline):

• max-retries: "1" is inert for timeout-failures (both budgets are identical, so remainingAfterDelay < 60 always fires after a full 2700s run). this is intentional and correct per the description, but a short comment in the workflow would help future readers understand why the retry is only for transient API failures, not timeouts.

[derekmisler]

suggestion (non-blocking): parseInt silently truncates non-integer inputs to 0, which disables the budget. all other numeric inputs in this file use parseInt and that's fine for timeout, max-retries, etc. (nobody passes 0.5 seconds there). but the tests exercise totalTimeout: 0.05 directly, and the docs don't explicitly say "integer only", so there's a subtle mismatch: an action caller who copies a decimal (e.g. from some calculator that produces 300.0) would silently get an unlimited budget instead of a 5-min cap.

Suggested change

	const totalTimeout = parseInt(core.getInput('total-timeout') || '0', 10);
	const totalTimeout = parseFloat(core.getInput('total-timeout') || '0');

Confidence	🟡 moderate	62/100