Add dismiss-review safe output with actor-bound PR review dismissal guards#43125
dismiss-review safe output with actor-bound PR review dismissal guards#43125Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
dismiss-review safe output with actor-bound PR review dismissal guards
There was a problem hiding this comment.
Pull request overview
This PR adds a new safe output (dismiss-pull-request-review / alias dismiss-review) that allows workflows to dismiss an existing PR review, with guardrails intended to ensure the dismissal is actor-bound and justified.
Changes:
- Added
dismiss_pull_request_reviewto the agent-output schema, workflow/frontmatter schema, and safe-output validation config (including min justification length). - Wired the new safe output through workflow compilation/tool catalogs and safe-output registry/enablement logic.
- Implemented MCP-side pre-validation plus a runtime JS handler (with tests) that fetches the review and dismisses it only after identity checks.
Show a summary per file
| File | Description |
|---|---|
| schemas/agent-output.json | Adds DismissPullRequestReviewOutput to the agent output union schema. |
| pkg/workflow/unified_prompt_step.go | Enables dismiss_pull_request_review tool exposure when configured. |
| pkg/workflow/tool_description_enhancer.go | Adds config-derived constraints for the new tool description. |
| pkg/workflow/safe_outputs_validation.go | Includes new safe-output key in target validation pass. |
| pkg/workflow/safe_outputs_validation_config.go | Adds per-field validation rules for dismiss_pull_request_review. |
| pkg/workflow/safe_outputs_tools_repo_params.go | Enables repo parameter behavior (allowed-repos/target-repo) for the tool. |
| pkg/workflow/safe_outputs_tools_computation.go | Includes tool in enabled-tools computation. |
| pkg/workflow/safe_outputs_state.go | Counts new config as “safe outputs enabled” for state/guards. |
| pkg/workflow/safe_outputs_max_validation.go | Validates max for the new config type. |
| pkg/workflow/safe_outputs_handler_registry.go | Registers handler config emission for runtime processing. |
| pkg/workflow/safe_outputs_config.go | Adds DismissPullRequestReview config + frontmatter parsing (incl alias). |
| pkg/workflow/safe_output_handlers.go | Registers safe-output descriptor + permissions for dismiss-review. |
| pkg/workflow/js/safe_outputs_tools.json | Adds the tool definition to the workflow-side JS tool catalog. |
| pkg/workflow/dismiss_pull_request_review.go | New Go config parser/type for dismissing PR reviews. |
| pkg/parser/schemas/main_workflow_schema.json | Adds workflow schema support for dismiss-pull-request-review and alias. |
| actions/setup/js/types/safe-outputs.d.ts | Adds TS item type for dismiss_pull_request_review. |
| actions/setup/js/safe_outputs_tools.json | Adds the tool definition to the action runtime tool catalog. |
| actions/setup/js/safe_outputs_tools_loader.cjs | Wires tool name to the new MCP pre-validation handler. |
| actions/setup/js/safe_outputs_handlers.test.cjs | Adds tests for MCP-side justification + author/actor checks. |
| actions/setup/js/safe_outputs_handlers.cjs | Adds MCP-side pre-validation handler for justification/author checks. |
| actions/setup/js/safe_output_handler_manager.cjs | Registers runtime handler path + threat classification. |
| actions/setup/js/dismiss_pull_request_review.test.cjs | New runtime handler tests for author/actor and fetched-author guards. |
| actions/setup/js/dismiss_pull_request_review.cjs | New runtime handler that resolves target/repo, validates author, fetches review, and dismisses it. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 22/23 changed files
- Comments generated: 2
- Review effort level: Low
| "name": "dismiss_pull_request_review", | ||
| "description": "Dismiss a pull request review. The review must be authored by the same workflow actor who is dismissing it. By default, the actor is inferred from the current GitHub Actions context (typically github-actions[bot]). You must provide a justification of at least 20 characters.", |
| "name": "dismiss_pull_request_review", | ||
| "description": "Dismiss a pull request review. The review must be authored by the same workflow actor who is dismissing it. By default, the actor is inferred from the current GitHub Actions context (typically github-actions[bot]). You must provide a justification of at least 20 characters.", |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (263 new lines in Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (6 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Review SummaryApplied
@copilot please address the review comments above.
|
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 96.9 AIC · ⌖ 9.43 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/dismiss_pull_request_review.cjs:85
[/codebase-design] The comment supportsPR=false means PR-only handlers inverts the obvious reading — a false value named supportsPR would normally imply the opposite. This will confuse future maintainers unfamiliar with resolveTarget conventions.
<details>
<summary>💡 Suggestion</summary>
Consider renaming to requiresPR: true and updating resolveTarget to accept that semantics, or at minimum change the comment to be explicit: `// false = handler targets PR only (no issue fallb…
actions/setup/js/dismiss_pull_request_review.cjs:107
[/tdd] In staged mode, the identity check against the fetched review author is skipped entirely — the staged path calls processedCount++ and returns success without fetching the review. This means staged mode can produce misleading results: it would succeed even when the review's author isn't the current actor, masking a runtime failure.
<details>
<summary>💡 Suggestion</summary>
Either document explicitly that staged mode skips review-author verification, or move the staged branch belo…
actions/setup/js/dismiss_pull_request_review.cjs:27
[/codebase-design] getEffectiveActor() reads from process.env.GITHUB_ACTOR first, then context?.actor — but in the runtime handler (inside the factory closure), context is the global GitHub Actions context object. However, process.env.GITHUB_ACTOR might differ from context.actor in certain matrix or reusable-workflow scenarios. Since this value is used as the security boundary for dismissal, prefer context?.actor as the authoritative source and keep env as a fallback only.
…
actions/setup/js/dismiss_pull_request_review.test.cjs:289
[/tdd] The test file only covers three scenarios. Missing edge cases that this security-sensitive handler should cover: (1) max-count enforcement — call the handler max+1 times and assert the last call fails; (2) review_id validation — zero, negative, non-numeric strings; (3) justification boundary — exactly 19 characters (fail) and exactly 20 characters (pass); (4) staged mode — verify correct return shape and that no GitHub API calls are made.
<details>
<summary>💡 Suggested addition…
actions/setup/js/safe_outputs_handlers.cjs:329
[/codebase-design] The MCP handler duplicates the actor-resolution logic from dismiss_pull_request_review.cjs (getEffectiveActor). Both inline process.env.GITHUB_ACTOR || context?.actor || "github-actions[bot]". If the resolution logic ever changes (e.g., priority reordering), it must be updated in two places.
<details>
<summary>💡 Suggestion</summary>
Extract actor resolution into a shared helper — e.g., a getWorkflowActor() function in safe_output_helpers.cjs — and import it i…
actions/setup/js/dismiss_pull_request_review.cjs:139
[/tdd] The error message uses reviewAuthor || "unknown" but reviewAuthor at this point has already passed the empty-string guard on line 129–134 (it is always a non-empty string here). The || "unknown" fallback is dead code and creates an impression that reviewAuthor might still be empty, which could confuse readers.
<details>
<summary>💡 Fix</summary>
// reviewAuthor is guaranteed non-empty by the guard above — remove the || 'unknown' fallback
error: `review author (${revie…
</details>
<details><summary>actions/setup/js/dismiss_pull_request_review.test.cjs:176</summary>
**[/tdd]** The test file uses `import` syntax on line 176 (`import { describe, it, expect, beforeEach, vi } from "vitest"`) but the module it tests is a CJS module (`.cjs`). The `require` call on line 233 will work in CJS interop but mixing ES module imports with CJS `require` in Vitest can lead to subtle module-isolation issues. Other test files in this codebase appear to use a consistent pattern — ensure this matches.
<details>
<summary>💡 Suggestion</summary>
Check whether peer test files …
</details>
<details><summary>pkg/workflow/safe_outputs_validation_config.go:895</summary>
**[/codebase-design]** The justification `MaxLength` is set to `MaxBodyLength` (same cap as comment bodies). A dismissal justification that approaches a comment-body length would be nonsensical and signals the wrong semantics to callers. This may also produce a misleading error message that references a body length instead of a justification length.
<details>
<summary>💡 Suggestion</summary>
Use a smaller, purpose-appropriate cap — e.g., 500 or 1000 characters — with a clear constant name lik…
</details>
<details><summary>actions/setup/js/dismiss_pull_request_review.cjs:151</summary>
**[/tdd]** The success response does not increment `processedCount` until after `dismissReview` succeeds, which is correct. However, the success response returns `author: reviewAuthor` (the fetched review author) rather than `author: expectedAuthor` (which was validated against the actor). If they differ due to case normalisation the return value will be misleading. Both are guaranteed equal at this point, so use the more semantically meaningful one, or return both for diagnostic purposes.
<de…
</details>There was a problem hiding this comment.
REQUEST_CHANGES — 3 correctness/security issues + 1 test gap
Three issues in the new dismiss_pull_request_review handler need fixing before merge.
Blocking issues summary
1. Stale actor snapshot (medium) — dismisser is captured once at main() factory time while the MCP pre-validation phase re-reads GITHUB_ACTOR fresh each call. The two layers enforce actor-identity against potentially different values. Fix: call getEffectiveActor() inside the returned handler, not at factory init. (Line 37)
2. parseInt silent truncation on review_id (medium) — Number.parseInt("123abc", 10) returns 123, silently dismissing the wrong review. The Go-layer IsPositiveInteger guard is strict and protects production paths, but the runtime handler should defend itself rather than relying on upstream pipeline integrity. Fix: use a round-trip check (String(Number(rawId)) !== rawId) to reject non-pure-integer strings. (Line 50)
3. Staged mode returns unverified author (medium) — staged responses include author: expectedAuthor without verifying it against the GitHub API. This makes staged output structurally misleading for callers who rely on it to preview what would happen in real mode. Fix: omit author from staged responses or annotate it as unverified. (Line 111)
4. Thin test coverage on a security-gating handler (medium) — only 3 test cases exist; max-count budget, invalid review_id, staged mode, and short justification paths all lack coverage. (dismiss_pull_request_review.test.cjs)
🔎 Code quality review by PR Code Quality Reviewer · 216.4 AIC · ⌖ 8.3 AIC · ⊞ 5.4K
Comment /review to run again
| const githubClient = await createAuthenticatedGitHubClient(config); | ||
| const requiredLabels = Array.isArray(config.required_labels) ? config.required_labels : []; | ||
| const requiredTitlePrefix = config.required_title_prefix || ""; | ||
| const dismisser = getEffectiveActor(); |
There was a problem hiding this comment.
Stale actor snapshot breaks actor-identity coherence between MCP and runtime phases: dismisser is captured once at main() factory time, but the MCP pre-validation handler re-reads GITHUB_ACTOR fresh on every call. If the environment changes between factory creation and invocation, the two layers enforce identity against different values.
💡 Suggested fix
getEffectiveActor() is a pure, side-effect-free function — there is no reason to cache its result. Moving the call inside the returned handler makes both layers consistent:
// Remove the factory-level capture:
// const dismisser = getEffectiveActor(); // <-- DELETE
return async function handleDismissPullRequestReview(message) {
const dismisser = getEffectiveActor(); // read per invocation
// ... rest unchanged ...
};Note: the test suite sets process.env.GITHUB_ACTOR in beforeEach after the module is required — because main() is called inside beforeEach too, the tests would catch a per-call read correctly, but they do not surface the drift caused by a factory-time snapshot.
| } | ||
|
|
||
| const reviewId = Number.parseInt(String(message.review_id || ""), 10); | ||
| if (!Number.isInteger(reviewId) || reviewId <= 0) { |
There was a problem hiding this comment.
parseInt silently truncates malformed review IDs: Number.parseInt("123abc", 10) returns 123 — the trailing non-numeric characters are silently dropped, so the handler proceeds against review 123 instead of rejecting the input.
💡 Details and suggested fix
The Go-layer IsPositiveInteger check (which uses strconv.ParseInt) runs before runtime execution and would reject "123abc". However the runtime handler should not rely on upstream validation as its only guard — a defensive strict check here prevents silent data corruption if the validation pipeline changes or is bypassed.
// Current (fragile — silently truncates "123abc" -> 123):
const reviewId = Number.parseInt(String(message.review_id || ""), 10);
if (!Number.isInteger(reviewId) || reviewId <= 0) { ... }
// Safer — reject if the full string is not a pure integer:
const rawId = String(message.review_id ?? "").trim();
const reviewId = Number(rawId);
if (!Number.isInteger(reviewId) || reviewId <= 0 || String(reviewId) !== rawId) {
return { success: false, error: "review_id must be a positive integer" };
}The same truncation bug is present in safe_output_type_validator.cjs's validatePositiveInteger, but that is pre-existing and outside this diff.
| pull_request_number: pullRequestNumber, | ||
| repo: `${owner}/${repo}`, | ||
| author: expectedAuthor, | ||
| }; |
There was a problem hiding this comment.
Staged mode returns an unverified author field: in staged mode the handler skips the getReview API call (correct — staged mode avoids all API writes), but the success response includes author: expectedAuthor which is the caller-supplied or actor-derived value, not the actual review author on GitHub. A caller relying on the staged response to validate author identity before promoting to real mode gets a false positive.
💡 Details
The real mode fetches the review, validates review.user.login === expectedAuthor, then returns author: reviewAuthor (the API-verified value). The staged mode returns author: expectedAuthor (the local assumption) without any API verification.
This inconsistency makes staged output structurally misleading. Consider either:
- Omitting the
authorfield from staged responses (consumers can't trust it anyway), or - Adding a comment like
// author unverified in staged modeto make the gap explicit.
There are no tests for staged mode, so this behavioral gap has no coverage.
| handler = await main({ max: 10 }); | ||
| }); | ||
|
|
||
| it("dismisses a review when author matches current actor", async () => { |
There was a problem hiding this comment.
Test suite covers only the happy path and two author-mismatch cases — critical paths are untested: the security contract of this handler rests on actor-identity enforcement and the max-count budget, yet neither has any test coverage.
💡 Missing test cases
Minimum additional tests needed for a security-gating handler:
- Max count exhaustion — create handler with
max: 1, call it twice; second call must return{ success: false, error: /Max count/ }. - Invalid
review_id— missing, zero, negative, and string"0"should all fail withreview_id must be a positive integer. - Staged mode — handler with
staged: truemust not callgetReviewordismissReview, must returnsuccess: true, staged: true. - Short justification —
justification: "short"(< 20 chars) must fail before any API call. - Review author unavailable —
getReviewreturns{ data: { user: null } }; handler must return the unavailability error.
The first two are correctness/security; the rest are basic robustness coverage expected for any safe-output handler.
There was a problem hiding this comment.
Review: Add dismiss-review safe output with actor-bound PR review dismissal guards
Overall the implementation is well-structured and follows established safe-output patterns throughout the codebase. The defense-in-depth design (MCP pre-validation + runtime identity fetch) is correct, and the wiring across Go config, registry, tool catalog, and JS runtime is complete.
3 issues found — all in the runtime JS handler:
| # | Severity | File | Description |
|---|---|---|---|
| 1 | Medium | dismiss_pull_request_review.cjs:50 |
Float review_id inputs (e.g. 123.5) pass validation after parseInt silently truncates them |
| 2 | Low | dismiss_pull_request_review.cjs:37 |
Actor is captured once at factory time; move getEffectiveActor() inside the inner function for per-message evaluation |
| 3 | Low | dismiss_pull_request_review.cjs:58 |
Runtime handler's own justification < 20 guard (line 58) is not exercised in dismiss_pull_request_review.test.cjs |
Minor nit (non-blocking): The inline comment at line 79 misstates the supportsPR: false semantics — logic is correct but the wording is inverted.
What looks good
- Permission (
NewPermissionsContentsReadPRWrite) matches thepulls.dismissReviewAPI scope. THREAT_WARNING_ABORT_TYPEScorrectly includesdismiss_pull_request_review.- The
processedCountis incremented only on success paths (staged and live), preventing limit bypass via retries. - Go struct embeds
BaseSafeOutputConfig,SafeOutputTargetConfig, andSafeOutputFilterConfigcorrectly —isSafeOutputHandlerEnabledAndUnstagedwill findStagedvia the standard reflection path. agent-output.jsondefinition correctly omitssecrecy/integrity(those belong in the tool input schema, not the agent output schema — consistent with all other output types).- The
dismiss-reviewalias is handled in both the frontmatter parser and the JSON schema$ref, which is the correct dual-registration pattern. - Test coverage for the MCP-side handler is thorough (valid case, short justification, author mismatch).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 170.8 AIC · ⌖ 6.73 AIC · ⊞ 4.9K
| } | ||
|
|
||
| const reviewId = Number.parseInt(String(message.review_id || ""), 10); | ||
| if (!Number.isInteger(reviewId) || reviewId <= 0) { |
There was a problem hiding this comment.
review_id float inputs are silently truncated.
parseInt("123.5", 10) returns 123, which then satisfies Number.isInteger(123). A caller passing a float review_id would silently target a different review. Consider rejecting non-integer string inputs that contain a decimal:
const rawStr = String(message.review_id || "");
if (!/^\d+$/.test(rawStr)) {
return { success: false, error: "review_id must be a positive integer" };
}
const reviewId = Number.parseInt(rawStr, 10);@copilot please address this.
| const githubClient = await createAuthenticatedGitHubClient(config); | ||
| const requiredLabels = Array.isArray(config.required_labels) ? config.required_labels : []; | ||
| const requiredTitlePrefix = config.required_title_prefix || ""; | ||
| const dismisser = getEffectiveActor(); |
There was a problem hiding this comment.
Actor is captured once at factory construction time, not per-call.
dismisser is resolved when main() is called (once per handler lifecycle), not when each message is handled. In most deployments this is fine, but if GITHUB_ACTOR changes after the factory is constructed (e.g., in tests that mutate the environment between calls without re-instantiating the handler), the guard silently uses a stale actor value.
The test currently works around this by setting process.env.GITHUB_ACTOR before calling main() in beforeEach, but calling vi.resetModules() doesn't help if the same handler instance is reused. Consider documenting this contract explicitly, or moving the getEffectiveActor() call inside the inner handler function so it's evaluated per-message:
return async function handleDismissPullRequestReview(message) {
const dismisser = getEffectiveActor(); // evaluate per call
...
};@copilot please address this.
| } | ||
|
|
||
| const justification = typeof message.justification === "string" ? message.justification.trim() : ""; | ||
| if (justification.length < 20) { |
There was a problem hiding this comment.
Runtime handler lacks tests for its own justification < 20 guard.
The MCP handler test (safe_outputs_handlers.test.cjs) validates the MCP-layer rejection of short justifications, but dismiss_pull_request_review.test.cjs (the runtime handler) has no test exercising line 58's rejection path. Since the runtime handler runs in the GitHub Actions step (not the MCP phase), this code path is untested end-to-end.
Add a test case like:
it("rejects when justification is shorter than 20 characters", async () => {
const result = await handler({
type: "dismiss_pull_request_review",
review_id: 123,
justification: "too short",
});
expect(result.success).toBe(false);
expect(result.error).toContain("at least 20 characters");
expect(mockDismissReview).not.toHaveBeenCalled();
});@copilot please address this.
| context, | ||
| itemType: "pull request review dismissal", | ||
| // In resolveTarget conventions, supportsPR=false means PR-only handlers. | ||
| supportsPR: false, |
There was a problem hiding this comment.
Misleading inline comment on supportsPR: false.
The comment reads: "In resolveTarget conventions, supportsPR=false means PR-only handlers." This is backwards: supportsPR=false does not mean PR-only by positive assertion. It means the handler does not support issues. PR-only is the implied default when both supportsPR and supportsIssue are false. The JSDoc for resolveTarget says: "When false, handler supports PRs ONLY." The logic is correct; the comment is just confusing and could lead to future misreads.
Suggested replacement:
// supportsPR: false + supportsIssue: false (defaults) → PR-only handler per resolveTarget contract\nsupportsPR: false,\n```
@copilot please address this.|
@copilot please run the
Run: https://github.com/github/gh-aw/actions/runs/28645166299
|
🤖 PR Triage
Score Breakdown: Impact 40 + Urgency 18 + Quality 14 Rationale: Adds new
Recommended Action: Author must address review issues; then fast-track for human approval.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved review feedback in commit CI on this new head is stale after agent push and needs a maintainer re-trigger. |
This introduces a new safe output for dismissing pull request reviews from the workflow actor (defaulting to
github-actions[bot]from action context). It enforces three invariants: justification length (>= 20), caller-suppliedauthormust equal the inferred actor, and the fetched review author must also equal that actor before dismissal.Safe-output surface area (config + schemas)
dismiss-pull-request-reviewwith aliasdismiss-reviewto safe-output config parsing and handler descriptors.dismiss_pull_request_reviewin both runtime and workflow-side tool catalogs.MCP-side validation
dismiss_pull_request_reviewto reject shortjustificationand actor/author mismatches before recording output.Runtime execution guardrails
dismiss_pull_request_review.cjshandler to:pulls.dismissReviewonly after identity + justification checks pass.Registry/types wiring
dismiss_pull_request_review.{ "type": "dismiss_pull_request_review", "review_id": 12345, "justification": "This review is stale after significant follow-up changes.", "author": "github-actions[bot]" }