feat: dismiss pr reviews when all threads are resolved in pr-sous-chef#43457
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Remove overly strict actor guard: allow workflow_dispatch runs to call dismiss_pull_request_review (GITHUB_TOKEN is always github-actions[bot] regardless of trigger) - Only skip dismissal for slash-command runs (acknowledgment nudges should not perform automated cleanup) - Change dismissal condition from per-review related-threads check to global "all PR review threads resolved" check - Update sub-agent (pr-processor) to return dismiss_reviews list - Add dismissed_reviews counter to noop run summary - Update contract test assertions to match new behavior Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot determine why action pins changes.json was not properly serialized with 2 spaces per tab. Fix build script or api to avoid the current changes that use 4 spaces |
…ync-action-pins Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Root cause: commit Two fixes in commit
|
There was a problem hiding this comment.
Pull request overview
Updates the PR Sous Chef workflow guidance and contract test to dismiss stale github-actions[bot] CHANGES_REQUESTED reviews only when the PR has no unresolved review threads, and adjusts action-pins syncing to produce consistently formatted JSON.
Changes:
- Shift review-dismissal condition from per-review “related threads” to a global “all PR review threads resolved” gate and document the new slash-command skip behavior.
- Extend the
pr-processorsub-agent contract to returndismiss_reviews: []and adddismissed_reviewsto the noop summary expectations. - Reformat synced action pins JSON via
jq --indent 2instead of byte-for-byte copying.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/pr_sous_chef_workflow_contract_test.go | Updates contract assertions for the new dismissal condition + noop payload field. |
| Makefile | Changes sync-action-pins to pretty-print JSON when syncing action pins. |
| .github/workflows/pr-sous-chef.md | Updates workflow guidance/spec for dismissal logic, slash-command guard, and summary counters. |
| .github/workflows/pr-sous-chef.lock.yml | Regenerated lock metadata to reflect the updated workflow source content. |
| .github/aw/actions-lock.json | Reformats JSON output (indentation/pretty-printing) to match new sync behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/5 changed files
- Comments generated: 3
- Review effort level: Low
| - For `schedule` and `workflow_dispatch` runs, use the `dismiss_reviews` list returned by the `pr-processor` sub-agent. The sub-agent populates this list only when ALL review threads on the PR are resolved; leave reviews untouched if any thread remains unresolved. | ||
| - `dismiss_pull_request_review` uses the `GITHUB_TOKEN`, which is always authenticated as `github-actions[bot]` regardless of the workflow trigger. It can therefore dismiss `github-actions[bot]`-authored reviews on any non-slash-command run. |
| assert.Contains(t, text, "dismiss-pull-request-review", "Workflow should configure the native safe-output path for dismissing stale github-actions reviews") | ||
| assert.Contains(t, text, "safeoutputs dismiss_pull_request_review", "Workflow should call the native dismiss_pull_request_review safe-output tool") | ||
| assert.Contains(t, text, "dismiss_pull_request_review` is actor-bound", "Workflow must document that dismiss_pull_request_review is actor-bound") | ||
| assert.Contains(t, text, "human-triggered runs cannot dismiss bot-authored reviews", "Workflow must require human-triggered runs to skip dismissal") | ||
| assert.Contains(t, text, "all review threads on the PR are resolved", "Workflow must dismiss reviews only when all PR review threads are resolved") | ||
| assert.Contains(t, text, "slash-command runs are acknowledgment nudges and must not perform automated review cleanup", "Workflow must require slash-command runs to skip dismissal") | ||
| assert.Contains(t, text, "dismissed_reviews", "Noop summary must include dismissed_reviews counter") |
| jq --indent 2 . .github/aw/actions-lock.json > pkg/actionpins/data/action_pins.json; \ | ||
| jq --indent 2 . .github/aw/actions-lock.json > pkg/workflow/data/action_pins.json; \ |
|
🎉 This pull request is included in a new release. Release: |
The old dismiss logic had two problems: it was gated behind a scheduled-only actor guard (incorrectly assuming
workflow_dispatchcan't dismissgithub-actions[bot]reviews), and it checked per-review related threads rather than whether the PR is globally clear of unresolved feedback.Changes
Dismiss condition
github-actions[bot]CHANGES_REQUESTEDreviews only when all review threads on the PR are resolvedActor guard removed for
workflow_dispatchdismiss_pull_request_reviewalways usesGITHUB_TOKEN(authenticated asgithub-actions[bot]), so it can dismiss bot reviews on any trigger/souschef) runs still skip dismissal — they are acknowledgment nudges, not cleanup passesSub-agent (
pr-processor) extendeddismiss_reviews: []— list of review IDs to dismiss (empty unless all threads resolved), so the main agent doesn't re-inspect independentlyRun summary
dismissed_reviewscounter to the noop payloadContract test
dismissed_reviewscounter