Skip to content

Mark pull_request_review_write and sub_issue_write as destructive#2838

Open
JuggerGrimrod88 wants to merge 1 commit into
github:mainfrom
JuggerGrimrod88:fix/destructive-hint-write-tools-sweep
Open

Mark pull_request_review_write and sub_issue_write as destructive#2838
JuggerGrimrod88 wants to merge 1 commit into
github:mainfrom
JuggerGrimrod88:fix/destructive-hint-write-tools-sweep

Conversation

@JuggerGrimrod88

Copy link
Copy Markdown

What

Sets DestructiveHint: true on the two remaining method-dispatched write tools whose delete/remove method discards data:

  • pull_request_review_writemethod: "delete_pending" deletes a pending review and discards its body.
  • sub_issue_writemethod: "remove" deletes a sub-issue link.

Why

This completes the convention established by #2836 (issue #2723) for label_write, and already followed by discussion_comment_write and projects_write: a method-dispatched write tool capable of destruction sets DestructiveHint: true so MCP clients that gate on the hint surface a confirmation prompt before running it.

Before this PR, a client gating on DestructiveHint would warn before label_write delete but not before pull_request_review_write method: "delete_pending" or sub_issue_write method: "remove", even though both discard data. The granular equivalents already flag these as destructive:

  • delete_pending_pull_request_review (pkg/github/pullrequests_granular.go) sets DestructiveHint: true.
  • remove_sub_issue (pkg/github/issues_granular.go) sets DestructiveHint: true.

So this brings the umbrella method-dispatched tools in line with both their granular equivalents and the sibling umbrella tools (discussion_comment_write, projects_write, label_write). No new judgment call — just consistency.

This was surfaced by a self-review of #2836/#2837: the #2836 rationale ("method-dispatched tools capable of destruction should set DestructiveHint") is satisfied by these two tools as well, so a maintainer would reasonably ask "why only label_write?" — this answers it.

Changes

  • pkg/github/pullrequests.go — add DestructiveHint: jsonschema.Ptr(true) to pull_request_review_write (gofmt re-aligned the struct fields).
  • pkg/github/issues.go — add DestructiveHint: jsonschema.Ptr(true) to sub_issue_write.
  • pkg/github/pullrequests_test.go, pkg/github/issues_test.go — add require.NotNil + assert.True assertions mirroring labels_test.go.
  • Regenerated golden snapshots: pull_request_review_write.snap, sub_issue_write.snap (only delta is + "destructiveHint": true).

No behavior, schema, or API change.

Overlap with #2837 (heads-up)

#2837 also edits pull_request_review_write's annotation block (its Title string) and regenerates the same pull_request_review_write.snap. This PR and #2837 touch different fields of the same Annotations struct, so the .go merge is clean. The .snap file will conflict if both merge, but it's a one-line regen — whichever merges second just re-runs UPDATE_TOOLSNAPS=true go test ./pkg/github/ -run TestCreateAndSubmitPullRequestReview. Happy to rebase onto whichever lands first if helpful.

Verification

  • gofmt -l clean on all touched files.
  • go vet ./pkg/github/
  • go build ./...
  • go test ./pkg/github/ -run 'TestCreateAndSubmitPullRequestReview|TestCreatePendingPullRequestReview|TestSubmitPendingPullRequestReview|TestDeletePendingPullRequestReview|Test_AddSubIssue|Test_RemoveSubIssue|Test_ReprioritizeSubIssue' — green (snapshots regenerated with UPDATE_TOOLSNAPS=true, then re-run without the flag).

Set DestructiveHint: true on the two remaining method-dispatched write
tools whose delete/remove method discards data, completing the convention
established for label_write (github#2836, issue github#2723) and already followed by
discussion_comment_write and projects_write.

- pull_request_review_write: delete_pending discards a pending review's
  body. Its granular equivalent delete_pending_pull_request_review
  already sets DestructiveHint.
- sub_issue_write: remove deletes a sub-issue link. Its granular
  equivalent remove_sub_issue already sets DestructiveHint.

Adds require.NotNil/assert.True assertions mirroring labels_test.go and
regenerates both golden snapshots. No behavior, schema, or API change.

Follow-up to github#2836; surfaced by self-review of github#2836/github#2837.

Co-Authored-By: Claude <noreply@anthropic.com>
@JuggerGrimrod88 JuggerGrimrod88 requested a review from a team as a code owner July 8, 2026 02:18
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.

1 participant