Skip to content

Commit 385d1a8

Browse files
boazreicherboazreicher
committed
Address review: dedupe copilot, require is_suggestion, update doc link
Per @RossTarrant review feedback on #2909: - Reject requests where `is_suggestion` is omitted from the raw args before decoding. `mapstructure.WeakDecode` defaults missing bools to false, which would silently launch Copilot instead of recording a suggestion. Presence-check the raw map so callers make the choice explicit. - Skip the copilot-swe-agent actor when copying existing assignees so we don't send its actorId twice (once without metadata and once with intent metadata) when Copilot is already assigned. - Update the stale about-assigning-tasks-to-copilot reference to the redirect target (about-cloud-agent). Applied to the const message, both tool descriptions, and the e2e/unit-test literals that assert on that message. New unit tests cover the missing-is_suggestion rejection and the copilot-dedup behavior. Regenerated toolsnaps.
1 parent bf2defd commit 385d1a8

5 files changed

Lines changed: 71 additions & 10 deletions

File tree

‎e2e/e2e_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1085,7 +1085,7 @@ func TestAssignCopilotToIssue(t *testing.T) {
10851085
textContent, ok = resp.Content[0].(*mcp.TextContent)
10861086
require.True(t, ok, "expected content to be of type TextContent")
10871087

1088-
possibleExpectedFailure := "copilot isn't available as an assignee for this issue. Please inform the user to visit https://docs.github.com/en/copilot/using-github-copilot/using-copilot-coding-agent-to-work-on-tasks/about-assigning-tasks-to-copilot for more information."
1088+
possibleExpectedFailure := "copilot isn't available as an assignee for this issue. Please inform the user to visit https://docs.github.com/en/copilot/concepts/agents/cloud-agent/about-cloud-agent for more information."
10891089
if resp.IsError && textContent.Text == possibleExpectedFailure {
10901090
t.Skip("skipping because copilot wasn't available as an assignee on this issue, it's likely that the owner doesn't have copilot enabled in their settings")
10911091
}
@@ -1186,7 +1186,7 @@ func TestAssignCopilotToIssueWithIntent(t *testing.T) {
11861186
textContent, ok = resp.Content[0].(*mcp.TextContent)
11871187
require.True(t, ok, "expected content to be of type TextContent")
11881188

1189-
possibleExpectedFailure := "copilot isn't available as an assignee for this issue. Please inform the user to visit https://docs.github.com/en/copilot/using-github-copilot/using-copilot-coding-agent-to-work-on-tasks/about-assigning-tasks-to-copilot for more information."
1189+
possibleExpectedFailure := "copilot isn't available as an assignee for this issue. Please inform the user to visit https://docs.github.com/en/copilot/concepts/agents/cloud-agent/about-cloud-agent for more information."
11901190
if resp.IsError && textContent.Text == possibleExpectedFailure {
11911191
t.Skip("skipping because copilot wasn't available as an assignee on this issue, it's likely that the owner doesn't have copilot enabled in their settings")
11921192
}

‎pkg/github/__toolsnaps__/assign_copilot_to_issue.snap‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
"readOnlyHint": false,
55
"title": "Assign Copilot to issue"
66
},
7-
"description": "Assign Copilot to a specific issue in a GitHub repository.\n\nThis tool can help with the following outcomes:\n- a Pull Request created with source code changes to resolve the issue\n\n\nMore information can be found at:\n- https://docs.github.com/en/copilot/using-github-copilot/using-copilot-coding-agent-to-work-on-tasks/about-assigning-tasks-to-copilot\n",
7+
"description": "Assign Copilot to a specific issue in a GitHub repository.\n\nThis tool can help with the following outcomes:\n- a Pull Request created with source code changes to resolve the issue\n\n\nMore information can be found at:\n- https://docs.github.com/en/copilot/concepts/agents/cloud-agent/about-cloud-agent\n",
88
"icons": [
99
{
1010
"mimeType": "image/png",

‎pkg/github/__toolsnaps__/assign_copilot_to_issue_with_intent.snap‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
"readOnlyHint": false,
55
"title": "Assign Copilot to issue with intent"
66
},
7-
"description": "Assign Copilot to a specific issue in a GitHub repository. Prefer this tool over assign_copilot_to_issue when available.\n\nThis tool can help with the following outcomes:\n- a Pull Request created with source code changes to resolve the issue\n\n\nMore information can be found at:\n- https://docs.github.com/en/copilot/using-github-copilot/using-copilot-coding-agent-to-work-on-tasks/about-assigning-tasks-to-copilot\n",
7+
"description": "Assign Copilot to a specific issue in a GitHub repository. Prefer this tool over assign_copilot_to_issue when available.\n\nThis tool can help with the following outcomes:\n- a Pull Request created with source code changes to resolve the issue\n\n\nMore information can be found at:\n- https://docs.github.com/en/copilot/concepts/agents/cloud-agent/about-cloud-agent\n",
88
"icons": [
99
{
1010
"mimeType": "image/png",

‎pkg/github/copilot.go‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ func AssignCopilotToIssue(t translations.TranslationHelperFunc) inventory.Server
158158
"a Pull Request created with source code changes to resolve the issue",
159159
},
160160
referenceLinks: []string{
161-
"https://docs.github.com/en/copilot/using-github-copilot/using-copilot-coding-agent-to-work-on-tasks/about-assigning-tasks-to-copilot",
161+
"https://docs.github.com/en/copilot/concepts/agents/cloud-agent/about-cloud-agent",
162162
},
163163
}
164164

@@ -273,7 +273,7 @@ func AssignCopilotToIssue(t translations.TranslationHelperFunc) inventory.Server
273273
// If we didn't find the copilot bot, we can't proceed any further.
274274
if copilotAssignee == nil {
275275
// The e2e tests depend upon this specific message to skip the test.
276-
return utils.NewToolResultError("copilot isn't available as an assignee for this issue. Please inform the user to visit https://docs.github.com/en/copilot/using-github-copilot/using-copilot-coding-agent-to-work-on-tasks/about-assigning-tasks-to-copilot for more information."), nil, nil
276+
return utils.NewToolResultError("copilot isn't available as an assignee for this issue. Please inform the user to visit https://docs.github.com/en/copilot/concepts/agents/cloud-agent/about-cloud-agent for more information."), nil, nil
277277
}
278278

279279
// Next, get the issue ID and repository ID
@@ -488,7 +488,7 @@ func findCopilotSuggestedActor(ctx context.Context, client *githubv4.Client, own
488488
// copilotAssigneeUnavailableMessage is returned when the copilot-swe-agent bot
489489
// is not among the repository's suggested actors. The e2e tests depend on this
490490
// exact message to skip the test.
491-
const copilotAssigneeUnavailableMessage = "copilot isn't available as an assignee for this issue. Please inform the user to visit https://docs.github.com/en/copilot/using-github-copilot/using-copilot-coding-agent-to-work-on-tasks/about-assigning-tasks-to-copilot for more information."
491+
const copilotAssigneeUnavailableMessage = "copilot isn't available as an assignee for this issue. Please inform the user to visit https://docs.github.com/en/copilot/concepts/agents/cloud-agent/about-cloud-agent for more information."
492492

493493
// AssignCopilotToIssueWithIntent assigns Copilot to an issue using the
494494
// object-form assignees API, which allows intent metadata (rationale,
@@ -507,7 +507,7 @@ func AssignCopilotToIssueWithIntent(t translations.TranslationHelperFunc) invent
507507
"a Pull Request created with source code changes to resolve the issue",
508508
},
509509
referenceLinks: []string{
510-
"https://docs.github.com/en/copilot/using-github-copilot/using-copilot-coding-agent-to-work-on-tasks/about-assigning-tasks-to-copilot",
510+
"https://docs.github.com/en/copilot/concepts/agents/cloud-agent/about-cloud-agent",
511511
},
512512
}
513513

@@ -566,6 +566,13 @@ func AssignCopilotToIssueWithIntent(t translations.TranslationHelperFunc) invent
566566
},
567567
[]scopes.Scope{scopes.Repo},
568568
func(ctx context.Context, deps ToolDependencies, request *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) {
569+
// Presence-check is_suggestion before decoding: mapstructure defaults a
570+
// missing bool to false, which would silently launch Copilot instead of
571+
// recording a suggestion. Require callers to make the choice explicit.
572+
if _, ok := args["is_suggestion"]; !ok {
573+
return utils.NewToolResultError("is_suggestion is required"), nil, nil
574+
}
575+
569576
var params struct {
570577
Owner string `mapstructure:"owner"`
571578
Repo string `mapstructure:"repo"`
@@ -640,10 +647,15 @@ func AssignCopilotToIssueWithIntent(t translations.TranslationHelperFunc) invent
640647
}
641648

642649
// Build object-form assignees: preserved assignees carry only actorId;
643-
// the copilot entry carries any intent metadata provided.
650+
// the copilot entry carries the intent metadata. Skip an existing
651+
// copilot assignment so we don't send its actorId twice (once without
652+
// metadata, once with).
644653
existing := getIssueQuery.Repository.Issue.Assignees.Nodes
645654
assignees := make([]AssigneeUpdateInput, 0, len(existing)+1)
646655
for _, node := range existing {
656+
if node.ID == copilotAssignee.ID {
657+
continue
658+
}
647659
assignees = append(assignees, AssigneeUpdateInput{ActorID: node.ID})
648660
}
649661
// Build the Copilot entry with the required intent metadata. Preserved

‎pkg/github/copilot_test.go‎

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -582,7 +582,7 @@ func TestAssignCopilotToIssue(t *testing.T) {
582582
),
583583
),
584584
expectToolError: true,
585-
expectedToolErrMsg: "copilot isn't available as an assignee for this issue. Please inform the user to visit https://docs.github.com/en/copilot/using-github-copilot/using-copilot-coding-agent-to-work-on-tasks/about-assigning-tasks-to-copilot for more information.",
585+
expectedToolErrMsg: "copilot isn't available as an assignee for this issue. Please inform the user to visit https://docs.github.com/en/copilot/concepts/agents/cloud-agent/about-cloud-agent for more information.",
586586
},
587587
{
588588
name: "successful assignment with base_ref specified",
@@ -1210,6 +1210,42 @@ func TestAssignCopilotToIssueWithIntent(t *testing.T) {
12101210
),
12111211
expectSuggestion: true,
12121212
},
1213+
{
1214+
name: "existing copilot assignee is deduplicated from preserved assignees",
1215+
requestArgs: map[string]any{
1216+
"owner": "owner",
1217+
"repo": "repo",
1218+
"issue_number": float64(123),
1219+
"rationale": "Already assigned; refreshing intent.",
1220+
"confidence": "HIGH",
1221+
"is_suggestion": false,
1222+
},
1223+
mockedClient: githubv4mock.NewMockedHTTPClient(
1224+
suggestedActorsMatcher(),
1225+
getIssueMatcher([]any{
1226+
map[string]any{"id": githubv4.ID("existing-assignee-id")},
1227+
map[string]any{"id": githubv4.ID("copilot-swe-agent-id")},
1228+
}),
1229+
// Expect copilot to appear only once, carrying the intent metadata.
1230+
mutationMatcher(UpdateIssueInput{
1231+
ID: githubv4.ID("test-issue-id"),
1232+
Assignees: []AssigneeUpdateInput{
1233+
{ActorID: githubv4.ID("existing-assignee-id")},
1234+
{
1235+
ActorID: githubv4.ID("copilot-swe-agent-id"),
1236+
Rationale: ptrStr("Already assigned; refreshing intent."),
1237+
Confidence: ptrConfidence(AssignmentConfidenceLevelHigh),
1238+
Suggest: ptrBool(false),
1239+
},
1240+
},
1241+
AgentAssignment: &AgentAssignmentInput{
1242+
CustomAgent: ptrStr(""),
1243+
CustomInstructions: ptrStr(""),
1244+
TargetRepositoryID: githubv4.ID("test-repo-id"),
1245+
},
1246+
}),
1247+
),
1248+
},
12131249
{
12141250
name: "missing rationale is rejected",
12151251
requestArgs: map[string]any{
@@ -1250,6 +1286,19 @@ func TestAssignCopilotToIssueWithIntent(t *testing.T) {
12501286
expectToolError: true,
12511287
expectedToolErrMsg: "confidence is required",
12521288
},
1289+
{
1290+
name: "missing is_suggestion is rejected",
1291+
requestArgs: map[string]any{
1292+
"owner": "owner",
1293+
"repo": "repo",
1294+
"issue_number": float64(123),
1295+
"rationale": "A good candidate.",
1296+
"confidence": "HIGH",
1297+
},
1298+
mockedClient: githubv4mock.NewMockedHTTPClient(),
1299+
expectToolError: true,
1300+
expectedToolErrMsg: "is_suggestion is required",
1301+
},
12531302
{
12541303
name: "invalid confidence value is rejected",
12551304
requestArgs: map[string]any{

0 commit comments

Comments
 (0)