fix(safe-outputs): treat stale/resolved review threads as no-ops; allow bot review dismissal - #49648
Conversation
…uthor dismissal Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…eview dismissal Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
👋 Thanks for the fix to the The PR looks well-structured and ready from a contribution guidelines perspective:
This follows the project's agentic development process as outlined in CONTRIBUTING.md.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has 0 new lines of code in business logic directories. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Improves safe-output resilience for stale review threads and failed review cleanup.
Changes:
- Treats missing/resolved review threads as no-ops.
- Makes review cleanup failures report-only.
- Permits cross-actor bot-review dismissal and refreshes generated workflows.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/safe_output_handler_manager.cjs |
Adds report-only failure types. |
actions/setup/js/safe_output_handler_manager.test.cjs |
Tests report-only classification. |
actions/setup/js/resolve_pr_review_thread.cjs |
Skips resolved or missing threads. |
actions/setup/js/resolve_pr_review_thread.test.cjs |
Tests thread no-op behavior. |
actions/setup/js/dismiss_pull_request_review.cjs |
Allows bot-review dismissal across actors. |
actions/setup/js/dismiss_pull_request_review.test.cjs |
Tests bot dismissal behavior. |
.github/workflows/smoke-checkout-pr-dispatch.lock.yml |
Updates generated timeout and model configuration. |
.github/workflows/daily-arxiv-researcher.lock.yml |
Refreshes generated detection-model configuration. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
.github/workflows/smoke-checkout-pr-dispatch.lock.yml:930
- The runtime timeout environment also disagrees with the source workflow's explicit 10-minute limit (
.github/workflows/smoke-checkout-pr-dispatch.md:43). Regenerate this lock from the source so both the step timeout andGH_AW_TIMEOUT_MINUTESremain 10.
GH_AW_TIMEOUT_MINUTES: 20
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
| if (reviewAuthor.endsWith("[bot]")) { | ||
| core.info(`Review ${reviewId} was authored by bot ${reviewAuthor}; allowing dismissal by ${dismisser}`); | ||
| } else { | ||
| return { | ||
| success: false, | ||
| error: `review author (${reviewAuthor || "unknown"}) must match dismisser (${dismisser})`, | ||
| }; | ||
| } |
| if (threadInfo === null) { | ||
| core.warning(`Review thread not found or not a PullRequestReviewThread: ${threadId}`); | ||
| core.info(`Review thread ${threadId} not found — already resolved or stale; skipping`); | ||
| return { | ||
| success: false, | ||
| error: `Review thread not found: ${threadId}`, | ||
| success: true, | ||
| thread_id: threadId, | ||
| is_resolved: true, | ||
| skipped: true, |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes are correct and well-targeted.
isResolvedguard and null-node no-op path inresolve_pr_review_thread.cjsare sound; null-safety viathreadNode?.isResolved === trueis handled correctly.endsWith("[bot]")is the standard GitHub login convention for machine accounts — a valid and idiomatic approach for the dismissal bypass.- Adding both handler types to
REPORT_ONLY_FAILURE_TYPESis intentional and prevents transient edge-case failures from blocking the whole safe_outputs job. - Tests cover all new branches (null node, already-resolved thread, bot-authored review dismissal).
LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.1 AIC · ⌖ 12 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with targeted suggestions; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Bot-identity check is heuristic-only:
endsWith("[bot]")matches the naming convention but is not an authenticated identity assertion. Addinguser.type === 'Bot'as a first-class check would close that gap. - Report-only failure scope is broad: both
resolve_pull_request_review_threadanddismiss_pull_request_revieware fully demoted to warnings; real permission errors would be swallowed alongside the intended stale-ID edge cases. - Missing negative test: the new bot-dismissal test has no sibling that asserts the guard still rejects a human-authored review mismatch.
Positive Highlights
- ✅ Null-node and
isResolvedearly-exit paths are clean and well-tested - ✅ Test coverage for both stale-thread and already-resolved cases is solid
- ✅
skipped: trueon the response makes downstream consumers easy to write - ✅ PR description clearly links failures to the actual run IDs
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 43.9 AIC · ⌖ 7.42 AIC · ⊞ 7.1K
Comment /matt to run again
| success: false, | ||
| error: `review author (${reviewAuthor || "unknown"}) must match dismisser (${dismisser})`, | ||
| }; | ||
| if (reviewAuthor.endsWith("[bot]")) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The bot-exception guard uses endsWith("[bot]") which is a naming convention, not an authenticated identity check. A review author whose GitHub login happened to end with [bot] (e.g., a service account) would silently bypass the author-match guard.
💡 Suggested hardening
The REST GET /repos/{owner}/{repo}/pulls/{pull_number}/reviews/{review_id} response includes user.type which is "Bot" for actual GitHub Apps. Checking review.user.type === "Bot" is more reliable than the [bot] suffix heuristic:
const isBot = review?.user?.type === 'Bot' || reviewAuthor.endsWith('[bot]');
if (isBot) {
core.info(`Review ${reviewId} authored by bot ${reviewAuthor}; allowing dismissal`);
} else {
return { success: false, error: `review author (${reviewAuthor}) must match dismisser (${dismisser})` };
}The mock in the test doesn't include user.type, so you'd need to add it to both the mock and the source.
@copilot please address this.
| success: false, | ||
| error: `review author (${reviewAuthor || "unknown"}) must match dismisser (${dismisser})`, | ||
| }; | ||
| if (reviewAuthor.endsWith("[bot]")) { |
There was a problem hiding this comment.
[/tdd] The new test ("allows dismissal when review was authored by a bot and actor is a different user") doesn't cover the rejection path: a human reviewer with a mismatch should still fail. Without that negative test the guard is only half-specified.
💡 Suggested negative test
it('rejects dismissal when review was authored by a different human', async () => {
process.env.GITHUB_ACTOR = 'alice';
const { main } = require('./dismiss_pull_request_review.cjs');
handler = await main({ max: 10 });
mockGetReview.mockResolvedValueOnce({
data: { user: { login: 'bob', type: 'User' } },
});
const result = await handler({
type: 'dismiss_pull_request_review',
review_id: 123,
justification: 'test',
});
expect(result.success).toBe(false);
expect(result.error).toMatch(/must match dismisser/);
});@copilot please address this.
|
|
||
| /** Types whose failures are surfaced as warnings rather than failing the safe_outputs job. */ | ||
| const REPORT_ONLY_FAILURE_TYPES = new Set(["assign_to_agent", "upload_artifact"]); | ||
| const REPORT_ONLY_FAILURE_TYPES = new Set(["assign_to_agent", "upload_artifact", "resolve_pull_request_review_thread", "dismiss_pull_request_review"]); |
There was a problem hiding this comment.
[/diagnosing-bugs] Demoting dismiss_pull_request_review failures to warnings could silently swallow real permission errors (e.g., GITHUB_TOKEN lacking pull-requests: write), making future failures harder to diagnose.
💡 Suggestion
Consider only treating the specific known-safe error codes (e.g., "stale", "already resolved") as report-only, while letting resource not accessible by integration and other permission errors remain fatal. This keeps the non-fatal behaviour bounded to the intended edge case rather than all failures of these types.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (5 tests)
|
There was a problem hiding this comment.
Request changes: authorization bypass in bot-review dismissal, plus error-masking in thread resolution
One high-severity authorization concern and three medium/high correctness concerns need to be addressed before merge.
💡 Themes
- Auth bypass (blocking): the new
[bot]-suffix exemption indismiss_pull_request_review.cjslets a dismisser dismiss reviews from any bot account, not just its own workflow's bot identity — it never re-checks that the bot review actually belongs to the calling actor. - Error masking (blocking):
resolve_pr_review_thread.cjsnow treats anullGraphQL node lookup (which can indicate a bad ID, deleted thread, or permission failure — not just 'already resolved') as an unconditional success, and the already-resolved short-circuit skips the repo/PR scope validation performed later in the function. - Reduced failure visibility (worth confirming): both handler types were added to
REPORT_ONLY_FAILURE_TYPES, so any remaining hard failures for these two safe-output types will only warn rather than fail the job — compounding the above by removing one of the few remaining signals of misconfiguration.
See inline comments for specifics and suggested fixes.
🔎 Code quality review by PR Code Quality Reviewer · auto · 59.4 AIC · ⌖ 4.6 AIC · ⊞ 7.8K
Comment /review to run again
| success: false, | ||
| error: `review author (${reviewAuthor || "unknown"}) must match dismisser (${dismisser})`, | ||
| }; | ||
| if (reviewAuthor.endsWith("[bot]")) { |
There was a problem hiding this comment.
High: this authorization bypass lets a workflow dismiss reviews authored by any bot, not just its own — the suffix check never re-validates the bot login against dismisser/expectedAuthor.
💡 Details and fix
Before this change, reviewAuthor !== expectedAuthor always rejected a mismatch. Now, when reviewAuthor ends in [bot], the mismatch is silently allowed through with no further identity check. expectedAuthor is only ever forced to equal dismisser (line 123-129), so this branch effectively says: any bot-authored review can be dismissed by any dismisser, regardless of which bot actually wrote it. Concretely, a workflow running as github-actions[bot] (dismisser) could dismiss a review authored by dependabot[bot] or another org integration's bot — reviews that have nothing to do with this workflow's own prior output.
Suggested fix: only exempt the check when the reviewAuthor bot login corresponds to the same automation identity as the dismisser:
if (reviewAuthor !== expectedAuthor) {
const dismisserIsBot = dismisser.endsWith("[bot]");
if (dismisserIsBot && reviewAuthor.endsWith("[bot]")) {
core.info(`allowing dismissal of bot review`);
} else {
return { success: false, error: `review author (${reviewAuthor}) must match dismisser (${dismisser})` };
}
}| @@ -175,10 +178,22 @@ async function main(config = {}) { | |||
| // Look up the thread's PR number and repository | |||
| const threadInfo = await getThreadPullRequestInfo(githubClient, threadId); | |||
| if (threadInfo === null) { | |||
There was a problem hiding this comment.
High: a null GraphQL node — which can mean a deleted thread, malformed ID, wrong repo, or an authorization/permission failure — is now unconditionally reported as a successful, already-resolved thread.
💡 Details and rationale
node(id: threadId) returning null is not proof the thread was already resolved; GitHub's GraphQL API also returns null for IDs that don't exist, belong to a type mismatch, or are inaccessible to the token's permissions. Collapsing all of these into is_resolved: true, success: true silently hides real bugs (e.g. a caller passing the wrong node ID format, or a permissions regression) behind a success response, and the returned is_resolved: true is simply wrong for those cases — the thread was never resolved, it just couldn't be found.
Consider distinguishing 'confirmed stale/already-handled' from 'lookup failed for an unknown reason' — e.g., only treat this as a no-op success when there's a way to positively confirm staleness (such as a specific GraphQL error code), and otherwise keep failing loudly so misconfigurations surface instead of being masked, especially now that this handler is also in REPORT_ONLY_FAILURE_TYPES.
| }; | ||
| } | ||
|
|
||
| if (threadInfo.isResolved) { |
There was a problem hiding this comment.
Medium: the new already-resolved short-circuit returns success before the repo/PR-scope validation that runs a few lines below, so a stale or out-of-scope thread_id for a different PR/repo can be reported as resolved without ever checking it matches the expected target.
💡 Details
Below this block (lines ~205-238), the handler validates threadRepo/threadPRNumber against allowedRepos/triggeringPRNumber before actually resolving the thread. The new if (threadInfo.isResolved) branch returns early and skips all of that validation. Practically: if an agent supplies a thread_id for a thread that belongs to an entirely different PR or repo than the one this workflow is scoped to, and that thread happens to already be resolved, the handler will report success without ever confirming the thread was even in scope. This weakens the scoping guarantees that the rest of the function otherwise enforces.
|
|
||
| /** Types whose failures are surfaced as warnings rather than failing the safe_outputs job. */ | ||
| const REPORT_ONLY_FAILURE_TYPES = new Set(["assign_to_agent", "upload_artifact"]); | ||
| const REPORT_ONLY_FAILURE_TYPES = new Set(["assign_to_agent", "upload_artifact", "resolve_pull_request_review_thread", "dismiss_pull_request_review"]); |
There was a problem hiding this comment.
Medium: adding these two handler types to REPORT_ONLY_FAILURE_TYPES means any remaining hard failure (bad target-repo config, permission errors, invalid IDs) for review-thread resolution and review dismissal will only ever warn, never fail the job.
💡 Details
This compounds the concerns above: this PR already converts several previously-error cases (stale thread, already-resolved thread, cross-bot dismissal) into silent successes. Whatever failure paths remain (e.g., repo/PR scope validation failures, permission errors from dismissReview/getReview) are now also downgraded to non-fatal warnings via this set. That removes one of the few remaining signals that would tell a user their workflow's review-resolution/dismissal config is broken — worth confirming this is intentional and that these failure modes are genuinely benign, not just convenient to silence.
|
@copilot run pr-finisher skill |
|
@copilot quick triage for this PR:
Run details: https://github.com/github/gh-aw/actions/runs/30721766175
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The source workflow explicitly sets timeout-minutes: 10 on line 43, but the generated lock file had timeout-minutes: 20 (the compiler default). This manual fix restores the lock file to match the source specification. Addresses review feedback: smoke-checkout-pr-dispatch.lock.yml lines 883, 930, 1533, and 1580 now correctly use timeout 10 instead of 20. Note: The compiler appears to have a bug where it falls back to the default timeout (20 minutes) instead of respecting the frontmatter timeout-minutes setting. This needs investigation in a separate issue. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 05e0e44:
The changes preserve security boundaries and provide appropriate signal for actionable failures while handling expected edge cases gracefully. |
|
@copilot quick triage for this PR:
Run details: https://github.com/github/gh-aw/actions/runs/30726226343
|
|
🎉 This pull request is included in a new release. Release: |
Two back-to-back
PR Sous Chefruns failed theirsafe_outputsjob (§30710934641, §30713118417) due to stale review-thread IDs and cross-author dismissal rejection. Both failures are deterministic — they will recur on every run until fixed.Changes
resolve_pr_review_thread.cjs— no-op on stale/resolved threadsisResolvedto the GraphQLnode(id: $threadId)lookup query{ success: true, skipped: true }instead of errorisResolved === true→ early{ success: true, skipped: true }instead of attempting the mutationdismiss_pull_request_review.cjs— allow bot-authored review dismissalreviewAuthor !== expectedAuthor) now exempts bot accounts viareviewAuthor.endsWith("[bot]")"review author (github-actions[bot]) must match dismisser (pelikhan)"rejection when PR Sous Chef dismisses its own bot-authored reviewssafe_output_handler_manager.cjs— non-fatal fallbackresolve_pull_request_review_threadanddismiss_pull_request_reviewtoREPORT_ONLY_FAILURE_TYPESfailurebranch refresh requested from
Run details: https://github.com/github/gh-aw/actions/runs/30726226343