Repository navigation
fix: address Codex review feedback (#6, #7, #13–#17) - #18
Conversation
A PR with no checks or statuses at all was reported as "pending" indefinitely, so `prw wait` and the MCP wait_for_ci tool blocked until their timeout on repositories without CI or on stacked PRs whose base branch runs none (14 of 18 real PRs in testing). Zero checks is also what every PR looks like right after a push, so a PR is only reported as no_ci once it has had no checks or statuses and hasn't been updated (PR updated_at, which every push bumps) for 3 minutes; a zero/unknown updated_at keeps it pending. - github: PullRequest.UpdatedAt, StateNoCI, ApplyNoCI, applied by run, wait and broadcast. - prw wait exits 4 for no_ci (not 0: `prw wait && deploy` must not deploy unverified PRs); MCP summaries and tool descriptions cover it. - prw run doesn't notify on a transition into no_ci, and notifies normally if CI shows up later. Verified live: 11 real PRs moved from pending to no_ci; `prw wait` on one returns in 2s with exit 4 instead of waiting 30 minutes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When a workflow runs more than once for the same commit (e.g. an older run cancelled by `concurrency: cancel-in-progress`), the check-runs API returns one run per check suite, so the cancelled one kept the PR "failed" forever even after the newer run passed. Only the newest run per app and check name counts now, like GitHub's merge box and required checks. Verified live on zephyrproject-rtos/zephyr#105183: previously reported as failure ("Prevent Merging" cancelled), now success. Addresses Codex review on #6. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GovSlack serves incoming webhooks from its own slack-gov.com domain, so auto mode sent them the generic payload, which Slack rejects. Addresses Codex review on #7. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A PR auto-watched with --review-requested that was merged or closed while prw was stopped no longer matched the is:open search, so the startup sync removed it as a withdrawn request before it was checked, and its final merged/closed notification was lost. Before removing review-request PRs that stopped matching, the sync now looks them up and keeps the closed ones; the regular check then notifies and drops them. If the lookup fails, the PR is removed as before. Addresses Codex review on #16. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Any comment or label change bumps a PR's updated_at, which restarted the no-CI grace period: `prw run` stored `pending` and, with the default `change` filter, notified "no_ci → pending" although no CI had appeared, then silently went back to no_ci three minutes later. A PR that was no_ci now stays no_ci until an actual check or status shows up. Addresses Codex review on #17. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… before returning Context: the GitHub client methods now take a context.Context, passed down to each HTTP request. Previously `prw wait --timeout`, MCP `wait_for_ci` `timeout_seconds` and client cancellation, and Ctrl-C were only noticed between polls: a stalled network could keep a poll (PR + status + check-run pages, each with a 15s HTTP timeout) running well past the deadline, and Ctrl-C did nothing until it finished. A poll cancelled this way now ends as timed out (exit 2) or interrupted (exit 130) instead of being retried as a transient error. `prw run` also stops immediately on Ctrl-C, leaving the PRs it hadn't checked unchanged. Head re-check: `prw wait` / `wait_for_ci` look up the PR and then its CI, so a commit pushed in between made them return the final result of the previous commit (e.g. `prw wait && deploy` deploying unchecked code). Before returning a final result, the PR is read again (a free 304 when unchanged); if the head moved, waiting continues on the new commit. Addresses Codex reviews on #13 and #15. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the 512-entry cache was full, every new 200 response wiped it. With more paths polled in rotation than fit (~170 PRs, fewer with paginated check runs) it was cleared over and over and nearly every request became unconditional, losing the free 304s. A new path now evicts a single random entry (refreshing a cached path evicts nothing). Random rather than LRU on purpose: cyclic polling is LRU's worst case. Simulated with 640 paths: 64% hits, was 0%. Addresses Codex review on #14. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2614d38b17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pr, err := client.GetPullRequest(ctx, result.Owner, result.Repo, result.Number) | ||
| return err == nil && pr.Head.SHA != result.SHA |
There was a problem hiding this comment.
When the final GetPullRequest fails, this expression treats the failure as “head unchanged,” so waitForCI immediately returns the terminal result from the previously fetched SHA. If a push lands during the poll and this confirmation request has a transient error, prw wait can report success for the old commit; cancellation or timeout during this request is likewise reported as success/failure instead of interrupted/timed out. Return the error from headMoved so the existing retry and context handling can process it.
Useful? React with 👍 / 👎.
| if ghPR, err := client.GetPullRequest(ctx, pr.Owner, pr.Repo, pr.Number); err == nil && ghPR.IsClosed() { | ||
| closed[key] = true | ||
| } |
There was a problem hiding this comment.
If Ctrl-C arrives after the review-request search succeeds but while one of these PR lookups is in flight, the canceled lookup is treated as “not closed.” SyncAutoWatch then removes that PR as a withdrawn review request and persists the removal, so merely interrupting prw run can silently discard review-request PRs and their eventual merged/closed notifications. A canceled context should abort the sync before the config update rather than classifying failed lookups as removable.
Useful? React with 👍 / 👎.
If the request confirming the PR head failed, `prw wait` returned the final result of the commit it had polled, even if a push had just landed, and a timeout or Ctrl-C during that request ended as success/failure instead of timed out / interrupted. Its errors now go through the same path as poll errors: retried when transient, reported as timeout or interruption when the context is done. Addresses Codex review on #18. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A review-request PR that stopped matching its search was dropped whenever the lookup meant to tell "merged/closed" apart from "request withdrawn" failed. Interrupting `prw run` (Ctrl-C) during that lookup, a network error or a rate limit therefore silently removed PRs together with their final merged/closed notification. Now a PR is only dropped when it's confirmed open (the request was withdrawn or answered) or GitHub answers 404; otherwise it's kept until the next sync. A cancelled sync returns before touching the config. Addresses Codex review on #18. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes for the Codex review comments on the earlier PRs of this stack. Each was checked against the code (and, where possible, the real API) before changing anything; every fix has a test that fails without it.
latestCheckRuns)cancelledand asuccessrun of "Prevent Merging" in two suites; wasfailure, nowsuccesshooks.slack-gov.comdetected as SlackwaitForCIre-reads the PR (304, free); if the head moved, it keeps waiting on the new commitcontext.Context, passed to each HTTP request; cancelled polls end as timed out (2) / interrupted (130).prw runstops at once on Ctrl-C and leaves unchecked PRs untouchedRunOncetest: merged-while-stopped PR now notifiesmergedno_ciflapping on metadata updatesno_ciPR staysno_ciuntil a real check/status appearsTest plan
go test ./... -racego vet ./...andgo vet -tags integration ./...-tags integration)prw waitlive on fix: detect GitHub Actions results (aggregate check runs with commit statuses) #6 (merged → exit 0) and Add SARIF and GitHub annotation output, and a GitHub Action docker-lint#5 (success)🤖 Generated with Claude Code