Skip to content

fix: skip resolve-context gracefully when trigger artifact is absent - #43

Merged
Sayt-0 merged 4 commits into
mainfrom
fix/resolve-context-missing-trigger-artifact
Jul 6, 2026
Merged

fix: skip resolve-context gracefully when trigger artifact is absent#43
Sayt-0 merged 4 commits into
mainfrom
fix/resolve-context-missing-trigger-artifact

Conversation

@Sayt-0

@Sayt-0 Sayt-0 commented Jul 6, 2026

Copy link
Copy Markdown
Member

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"

Sayt-0 and others added 2 commits July 6, 2026 16:27
The trigger workflow concludes success even when save-context is skipped
by the anti-loop filter (e.g. the bot's own inline replies), so review-pr
was invoked with a trigger-run-id pointing at a run with no
pr-review-context artifact. The download step then failed and every bot
inline reply produced a red run.

Check artifact existence before downloading and turn the job into a no-op
when it is legitimately absent; empty outputs make all downstream jobs
skip. Listing failures fall through to the download attempt so real
infrastructure errors stay loud.
@Sayt-0
Sayt-0 requested a review from derekmisler July 6, 2026 15:06

@docker-agent docker-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

One security hardening finding (forced inline — security floor). The core fix logic is sound.

Comment thread .github/workflows/review-pr.yml
Co-authored-by: Docker Agent <svc-github-docker-agent@docker.com>
Signed-off-by: Sayt0 <138035894+Sayt-0@users.noreply.github.com>

@derekmisler derekmisler left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 APPROVE

nice fix. the artifact-existence guard is the right layer to catch this (reusable workflow rather than every caller), the fail-open behavior on API errors is well-reasoned, and the ^[0-9]+$ validation of trigger-run-id is a good defensive touch. all CI green.

@Sayt-0
Sayt-0 merged commit c05e043 into main Jul 6, 2026
8 checks passed
@Sayt-0
Sayt-0 deleted the fix/resolve-context-missing-trigger-artifact branch July 6, 2026 16:16
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants