fix(review-pr): gate trigger-path reviews on the requested reviewer - #42
Merged
Conversation
A review request for any reviewer fanned out to a full docker-agent review: the trigger workflow never filtered on requested_reviewer.login == 'docker-agent', the artifact did not store the requested reviewer, and review-pr.yml accepted any trigger-event == 'pull_request' context (membership then passed via the PR author). - skip save-context at the source unless docker-agent is the requested reviewer, and key the trigger concurrency group by reviewer so a skipped request cannot cancel an in-flight docker-agent capture - store requested_reviewer.txt in the trigger artifact and require docker-agent (or empty, for legacy artifacts and auto-review events) on the review job's trigger path - apply the same gate to the consumer trigger template
derekmisler
approved these changes
Jul 6, 2026
derekmisler
left a comment
derekmisler
left a comment
Collaborator
There was a problem hiding this comment.
🟢 APPROVE
nice fix. the layered approach (source gate in the trigger + downstream gate in the review job + updated readme template) is the right way to handle this. the '' fallback for legacy artifacts is well-reasoned and the concurrency key change prevents the cancellation race cleanly. ci is green, approving.
Sayt-0
added a commit
that referenced
this pull request
Jul 6, 2026
…43) ## Summary Every inline reply posted by docker-agent itself produced a red "Self PR Review" run. The trigger workflow skips `save-context` for the bot's own comments (anti-loop filter) but still concludes `success`, so `self-review-pr.yml` invoked `review-pr.yml` with a `trigger-run-id` pointing at a run that never uploaded the `pr-review-context` artifact. The `resolve-context` download step then failed and the whole run went red, even though every downstream job already handles `resolve-context.result == 'failure'` by skipping. The failure was anticipated but never neutralized. ## Failure flow ``` bot posts inline reply -> pull_request_review_comment fires "Self PR Review - Trigger" -> save-context skipped (anti-loop filter), run concludes success, no artifact -> workflow_run: conclusion == success, review-pr.yml called with trigger-run-id -> resolve-context: download pr-review-context -> not found -> job failed -> run red ``` ## Fix `resolve-context` now checks artifact existence up front (list artifacts of the trigger run filtered by name, using `github.token`; `actions: read` is already required from all callers) and gates the credentials/verify/download/read steps on the result. A legitimately absent artifact turns the job into a no-op: empty outputs make `review`, `reply-to-feedback` and `reply-to-mention` skip through their existing conditions, and the run stays green. | Scenario | Before | After | | --- | --- | --- | | Bot inline reply (filtered event, no artifact) | download fails, red run | no-op job, downstream skips, green run | | Human reply via trigger (artifact present) | download and reply | unchanged | | Direct `issue_comment` (`trigger-run-id` empty) | resolve-context skipped | unchanged | | Expired or deleted artifact | red run | clean skip (`.expired == false` filter) | | Artifact listing call fails | n/a | fail-open: warning, then download attempt, so real infrastructure errors (403, missing token) stay loud | | Missing `GITHUB_APP_TOKEN` with artifact present | verify step fails, red run | unchanged (real misconfiguration) | Fixing this in the reusable workflow (rather than a caller-side gate in `self-review-pr.yml`) propagates the fix to every consumer of the 2-workflow pattern on their next version bump, and skips the OIDC round-trip of `setup-credentials` on the no-op path. ## Interaction with #42 #42 widens the set of filtered trigger events: a review request for a non docker-agent reviewer also skips `save-context`, producing another success-run-without-artifact case with the same red-run mechanism. This change neutralizes that path as well. The two branches merge cleanly (verified with `git merge-tree`; the merged file passes actionlint) and with both applied the empty `trigger-event` output short-circuits the review gate before the new `requested-reviewer` clause is evaluated. ## Validation - actionlint passes on the modified workflow and its callers (`self-review-pr.yml`, `self-review-pr-trigger.yml`) - Downstream job conditions verified branch by branch: with empty outputs, all three dependent jobs skip - The cross-run `workflow_run` path is not reproducible locally; final proof is the first bot inline reply after merge producing a green run with `resolve-context` logging "nothing to do" --------- Signed-off-by: Sayt0 <138035894+Sayt-0@users.noreply.github.com> Co-authored-by: Docker Agent <svc-github-docker-agent@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.
Summary
Requesting a PR review from any reviewer (human or team) fanned out to a full docker-agent review on the trigger (
workflow_run) path. Therequested_reviewer.login == 'docker-agent'gate described in SECURITY.md existed only on the directpull_requestpath, so this is an implementation vs design divergence, not a behavior choice.Cause
review_requestedevent, no reviewer filterself-review-pr-trigger.ymlself-review-pr-trigger.ymltrigger-event == 'pull_request'; membership then passes via the PR author (path 1 ofcheck-org-membership)review-pr.ymlreview jobif:user_requested=true, bypassing the draft checkreview-pr.ymlclassify stepFlow before:
Fan-out: requesting N reviewers produced N events in one
cancel-in-progress: trueconcurrency group, yielding 1 to N reviews depending on timing.Fix (layered)
save-contextskips unlessrequested_reviewer.login == 'docker-agent'(scoped topull_request/review_requested; comment events untouched). Team requests (null login) are filtered toorequested_reviewer.txt;resolve-contextexposes it asrequested-reviewerreviewjob requiresdocker-agentor''(legacy artifact or auto-review event)review-pr/README.md(action-scoped, since the template also subscribes toopened/ready_for_review)Behavior
review-pr.ymlpin''branch, no regression; fixed once the trigger copy updatesreview-pr.ymlignores the extra artifact fileSecurity model unchanged
The artifact stays untrusted for authorization: a fork can forge
requested_reviewer.txt, but that only reproduces today's baseline and still goes throughcheck-org-membership(PR author membership, or requester re-derived from the PR timeline). The new gate reduces when reviews run; it cannot authorize anything new.Why the lenient
''branchupdate-consumers.ymlonly bumps thereview-pr.yml@shapin and never rewrites consumers' trigger-workflow copies. A strict gate would break sidebar review requests (the documented/reviewreplacement) andopened/ready_for_reviewtrigger events for every consumer with a stale copy.Validation
actionlint -shellcheck=(the CI invocation) passes; no TypeScript touched, so unit/integration suites are unaffected by construction.pull_requesttrigger workflows run from the PR merge ref. Requesting a review from a human on this PR should produce a skippedsave-contextand no review; requesting docker-agent should still produce a review (the downstreamreview-pr.ymlruns from main until merge).Follow-ups (separate issues, not addressed here)
user_requested=true, so consumer-templateopened/ready_for_reviewevents bypass the draft check; storing the event action in the artifact would fix classification.update-consumers.ymlcould also refresh consumers' trigger-workflow copies so gate/schema fixes propagate without manual edits.