From e0e8a059853ab268de0b5aca81e1f874b6755c3b Mon Sep 17 00:00:00 2001 From: JuggerGrimrod88 Date: Tue, 7 Jul 2026 22:17:51 -0400 Subject: [PATCH] Mark pull_request_review_write and sub_issue_write as destructive Set DestructiveHint: true on the two remaining method-dispatched write tools whose delete/remove method discards data, completing the convention established for label_write (#2836, issue #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 #2836; surfaced by self-review of #2836/#2837. Co-Authored-By: Claude --- pkg/github/__toolsnaps__/pull_request_review_write.snap | 1 + pkg/github/__toolsnaps__/sub_issue_write.snap | 1 + pkg/github/issues.go | 5 +++-- pkg/github/issues_test.go | 4 ++++ pkg/github/pullrequests.go | 5 +++-- pkg/github/pullrequests_test.go | 4 ++++ 6 files changed, 16 insertions(+), 4 deletions(-) diff --git a/pkg/github/__toolsnaps__/pull_request_review_write.snap b/pkg/github/__toolsnaps__/pull_request_review_write.snap index 74ef808559..5f9ee4a8b0 100644 --- a/pkg/github/__toolsnaps__/pull_request_review_write.snap +++ b/pkg/github/__toolsnaps__/pull_request_review_write.snap @@ -1,5 +1,6 @@ { "annotations": { + "destructiveHint": true, "idempotentHint": false, "readOnlyHint": false, "title": "Write operations (create, submit, delete) on pull request reviews" diff --git a/pkg/github/__toolsnaps__/sub_issue_write.snap b/pkg/github/__toolsnaps__/sub_issue_write.snap index 9dc2776c32..983ab32ffb 100644 --- a/pkg/github/__toolsnaps__/sub_issue_write.snap +++ b/pkg/github/__toolsnaps__/sub_issue_write.snap @@ -1,5 +1,6 @@ { "annotations": { + "destructiveHint": true, "idempotentHint": false, "readOnlyHint": false, "title": "Change sub-issue" diff --git a/pkg/github/issues.go b/pkg/github/issues.go index a40afa172e..7b49f395e7 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1384,8 +1384,9 @@ func SubIssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { Name: "sub_issue_write", Description: t("TOOL_SUB_ISSUE_WRITE_DESCRIPTION", "Add a sub-issue to a parent issue in a GitHub repository."), Annotations: &mcp.ToolAnnotations{ - Title: t("TOOL_SUB_ISSUE_WRITE_USER_TITLE", "Change sub-issue"), - ReadOnlyHint: false, + Title: t("TOOL_SUB_ISSUE_WRITE_USER_TITLE", "Change sub-issue"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(true), }, InputSchema: &jsonschema.Schema{ Type: "object", diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 191b6aa710..5066ae7b54 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -3941,6 +3941,10 @@ func Test_AddSubIssue(t *testing.T) { assert.Equal(t, "sub_issue_write", tool.Name) assert.NotEmpty(t, tool.Description) + // remove deletes a sub-issue link, so clients must treat the tool as + // destructive and confirm before running it. + require.NotNil(t, tool.Annotations.DestructiveHint, "sub_issue_write should set DestructiveHint") + assert.True(t, *tool.Annotations.DestructiveHint, "sub_issue_write should be destructive") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "method") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "owner") assert.Contains(t, tool.InputSchema.(*jsonschema.Schema).Properties, "repo") diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 9fc8455988..077efa38c4 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1784,8 +1784,9 @@ Available methods: - unresolve_thread: Unresolve a previously resolved review thread. Requires only "threadId" parameter. The owner, repo, and pullNumber parameters are not used for this method. Unresolving an already-unresolved thread is a no-op. `), Annotations: &mcp.ToolAnnotations{ - Title: t("TOOL_PULL_REQUEST_REVIEW_WRITE_USER_TITLE", "Write operations (create, submit, delete) on pull request reviews"), - ReadOnlyHint: false, + Title: t("TOOL_PULL_REQUEST_REVIEW_WRITE_USER_TITLE", "Write operations (create, submit, delete) on pull request reviews"), + ReadOnlyHint: false, + DestructiveHint: jsonschema.Ptr(true), }, InputSchema: schema, }, diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index bcae87c94c..66ddc7e7af 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -2843,6 +2843,10 @@ func TestCreateAndSubmitPullRequestReview(t *testing.T) { assert.Equal(t, "pull_request_review_write", tool.Name) assert.NotEmpty(t, tool.Description) + // delete_pending discards a pending review's body, so clients must treat + // the tool as destructive and confirm before running it. + require.NotNil(t, tool.Annotations.DestructiveHint, "pull_request_review_write should set DestructiveHint") + assert.True(t, *tool.Annotations.DestructiveHint, "pull_request_review_write should be destructive") schema := tool.InputSchema.(*jsonschema.Schema) assert.Contains(t, schema.Properties, "method") assert.Contains(t, schema.Properties, "owner")