review-pr: Avoid OIDC for direct API key reviews#40
Open
vvoland wants to merge 3 commits into
Open
Conversation
Direct API-key callers do not need AWS-backed credential setup, but the reusable workflow unconditionally requested id-token: write and ran setup-credentials. Organizations that disable OIDC for reusable workflows were blocked at the permission-validation step. Drop the top-level and per-job permissions blocks so the workflow inherits from the caller. When a model key secret is supplied, skip the OIDC credential fetch and authorize using the caller's GITHUB_TOKEN by checking repository write permission instead of Docker org membership. setup-credentials/action.yml gains a fetch-credentials input (default true). When false, DOCKER_AGENT_ACTION_ROOT is exported and Node.js is set up, but the OIDC exchange is skipped. The review job always runs setup-credentials (for DOCKER_AGENT_ACTION_ROOT and Node) and passes fetch-credentials dynamically, so direct-key callers pay no OIDC cost. The reply-to-feedback and reply-to-mention jobs skip setup-credentials entirely when a direct key is present — they use bash gh-api calls and uses: actions, not dist/ scripts, so neither DOCKER_AGENT_ACTION_ROOT nor an explicit Node install is needed. src/check-org-membership adds checkRepositoryWritePermission (write/maintain/admin collaborator check) and an isAuthorizedUser helper that dispatches to org membership (when ORG_MEMBERSHIP_TOKEN is set) or the repo permission check (when it is not). The CLI entry point now accepts GITHUB_TOKEN as a fallback for GITHUB_APP_TOKEN. The mention-reply handler and the reply-to-feedback auth step receive the same treatment: org-membership-token is now optional; when absent, authorization falls back to checkRepositoryWritePermission. The HAS_DIRECT_API_KEY guard that silently dis Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Sayt-0
added a commit
that referenced
this pull request
Jul 27, 2026
) ## 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 #40, #42, #43, #47 verified by local merge simulation: only #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 | --------- Signed-off-by: Sayt0 <138035894+Sayt-0@users.noreply.github.com> Co-authored-by: Derek Misler <derek.misler@docker.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issues
Summary
Direct API-key callers do not need AWS-backed credential setup, but the
reusable workflow unconditionally requested id-token: write and ran
setup-credentials. Organizations that disable OIDC for reusable
workflows were blocked at the permission-validation step.
Drop the top-level and per-job permissions blocks so the workflow
inherits from the caller. When a model key secret is supplied, skip the
OIDC credential fetch and authorize using the caller's GITHUB_TOKEN by
checking repository write permission instead of Docker org membership.
setup-credentials/action.yml gains a fetch-credentials input (default
true). When false, DOCKER_AGENT_ACTION_ROOT is exported and Node.js is
set up, but the OIDC exchange is skipped. The review job always runs
setup-credentials (for DOCKER_AGENT_ACTION_ROOT and Node) and passes
fetch-credentials dynamically, so direct-key callers pay no OIDC cost.
The reply-to-feedback and reply-to-mention jobs skip setup-credentials
entirely when a direct key is present — they use bash gh-api calls and
uses: actions, not dist/ scripts, so neither DOCKER_AGENT_ACTION_ROOT
nor an explicit Node install is needed.
src/check-org-membership adds checkRepositoryWritePermission
(write/maintain/admin collaborator check) and an isAuthorizedUser helper
that dispatches to org membership (when ORG_MEMBERSHIP_TOKEN is set) or
the repo permission check (when it is not). The CLI entry point now
accepts GITHUB_TOKEN as a fallback for GITHUB_APP_TOKEN.
The mention-reply handler and the reply-to-feedback auth step receive
the same treatment: org-membership-token is now optional; when absent,
authorization falls back to checkRepositoryWritePermission. The
HAS_DIRECT_API_KEY guard that silently dis