From 101563c0bd03a0f6b7049afad2ae257fab94492b Mon Sep 17 00:00:00 2001 From: Bryan Zwicker Date: Wed, 22 Jul 2026 12:54:16 -0400 Subject: [PATCH 1/7] Implement batch project write engine Resolve and validate shared field updates and item references before executing ordered, chunked GraphQL writes with explicit ambiguous outcomes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ae767ff-c1d0-46a9-b126-2e91403993a0 --- pkg/github/projects.go | 1 + pkg/github/projects_batch.go | 771 ++++++++++++++++++++++++++++++ pkg/github/projects_batch_test.go | 512 ++++++++++++++++++++ 3 files changed, 1284 insertions(+) create mode 100644 pkg/github/projects_batch.go create mode 100644 pkg/github/projects_batch_test.go diff --git a/pkg/github/projects.go b/pkg/github/projects.go index 308c2b87e8..8dd3df3ab2 100644 --- a/pkg/github/projects.go +++ b/pkg/github/projects.go @@ -32,6 +32,7 @@ const ( ProjectStatusUpdateCreateFailedError = "failed to create project status update" ProjectResolveIDFailedError = "failed to resolve project ID" MaxProjectsPerPage = 50 + maxProjectItemsPerBatch = 50 ) // Method constants for consolidated project tools diff --git a/pkg/github/projects_batch.go b/pkg/github/projects_batch.go new file mode 100644 index 0000000000..3c77d250c7 --- /dev/null +++ b/pkg/github/projects_batch.go @@ -0,0 +1,771 @@ +package github + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "math" + "sync" + "time" + + ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/utils" + "github.com/google/go-github/v89/github" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/shurcooL/githubv4" +) + +// Unknown outcomes cannot be attributed or retried safely because the pinned +// client drops errors[].path. +type batchItemStatus string + +const ( + batchItemSucceeded batchItemStatus = "succeeded" + batchItemFailed batchItemStatus = "failed" + batchItemUnknown batchItemStatus = "unknown" +) + +type batchItemResult struct { + Index int `json:"index"` + Status batchItemStatus `json:"status"` + Item *batchItemIdentity `json:"item,omitempty"` + Error *batchItemError `json:"error,omitempty"` + // Ref preserves the request identity when resolution fails. + Ref map[string]any `json:"ref,omitempty"` +} + +type batchItemIdentity struct { + NodeID string `json:"node_id,omitempty"` + FullDatabaseID string `json:"full_database_id,omitempty"` + ItemID int64 `json:"item_id,omitempty"` +} + +type batchItemError struct { + Code string `json:"code"` + Message string `json:"message"` + Candidates []any `json:"candidates,omitempty"` + Hint string `json:"hint,omitempty"` +} + +type resolvedBatchItem struct { + index int + ref map[string]any + nodeID string + fullDatabaseID int64 +} + +type batchWriteOperation struct { + gqlClient *githubv4.Client + kind batchMutationKind + projectID githubv4.ID + fieldID githubv4.ID + value githubv4.ProjectV2FieldValue +} + +func updateProjectItemsBatch(ctx context.Context, client *github.Client, gqlClient *githubv4.Client, owner, ownerType string, projectNumber int, args map[string]any) (*mcp.CallToolResult, any, error) { + rawItems, exists := args["items"] + if !exists { + return utils.NewToolResultError("missing required parameter: items"), nil, nil + } + itemsRaw, ok := rawItems.([]any) + if !ok { + return utils.NewToolResultError("items must be an array"), nil, nil + } + if len(itemsRaw) == 0 { + return utils.NewToolResultError("items must contain at least one entry"), nil, nil + } + if len(itemsRaw) > maxProjectItemsPerBatch { + return utils.NewToolResultError(fmt.Sprintf("items exceeds maximum of %d entries per call (got %d)", maxProjectItemsPerBatch, len(itemsRaw))), nil, nil + } + + rawField, hasField := args["updated_field"] + if !hasField { + return utils.NewToolResultError("missing required parameter: updated_field"), nil, nil + } + fieldSpec, fieldSpecErr := parseBatchFieldSpec(rawField) + if fieldSpecErr != nil { + return utils.NewToolResultError(fieldSpecErr.Error()), nil, nil + } + + if gqlClient == nil { + return utils.NewToolResultError("internal error: gqlClient is required for update_project_items"), nil, nil + } + + parsed := make([]parsedBatchItem, len(itemsRaw)) + for i, raw := range itemsRaw { + parsed[i] = parseBatchItemEntry(i, raw) + } + + results := make([]batchItemResult, len(itemsRaw)) + pending := 0 + for i, p := range parsed { + if p.err != nil { + results[i] = batchItemResult{Index: i, Status: batchItemFailed, Ref: p.ref, Error: p.err} + } else { + pending++ + } + } + if pending == 0 { + return newUpdateProjectItemsResult(results) + } + + projectID, err := resolveProjectNodeID(ctx, gqlClient, owner, ownerType, projectNumber) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + field, fieldErr := resolveBatchProjectField(ctx, gqlClient, owner, ownerType, projectNumber, fieldSpec) + if fieldErr != nil { + return batchTopLevelError(fieldErr), nil, nil + } + + kind := batchMutationUpdate + var value githubv4.ProjectV2FieldValue + if fieldSpec.value == nil { + kind = batchMutationClear + } else { + value, fieldErr = convertProjectFieldValue(field, fieldSpec.value) + if fieldErr != nil { + return batchTopLevelError(fieldErr), nil, nil + } + } + + var numericIDs []int64 + for _, p := range parsed { + if p.err == nil && p.refKind == batchRefItemID { + numericIDs = append(numericIDs, p.itemID) + } + } + itemIDLookups := resolveItemNodeIDsByNumericID(ctx, client, owner, ownerType, projectNumber, numericIDs) + + issueLookups := resolveIssueRefs(ctx, gqlClient, projectID, parsed) + + var work []resolvedBatchItem + seenTargets := make(map[string]int) + + for i, p := range parsed { + if p.err != nil { + continue + } + + nodeID, fullDatabaseID, lookupErr := resolveItemReference(p, itemIDLookups, issueLookups) + if lookupErr != nil { + results[i] = batchItemResult{Index: i, Status: batchItemFailed, Ref: p.ref, Error: batchErrorFromResolution(lookupErr)} + continue + } + + if firstIndex, dup := seenTargets[nodeID]; dup { + results[i] = batchItemResult{ + Index: i, Status: batchItemFailed, Ref: p.ref, + Error: &batchItemError{ + Code: "duplicate_target", + Message: fmt.Sprintf("items[%d] targets the same project item as items[%d]; each item may only be written once per call", i, firstIndex), + }, + } + continue + } + + seenTargets[nodeID] = i + work = append(work, resolvedBatchItem{index: i, ref: p.ref, nodeID: nodeID, fullDatabaseID: fullDatabaseID}) + } + + executeBatchWrites(ctx, batchWriteOperation{ + gqlClient: gqlClient, + kind: kind, + projectID: projectID, + fieldID: githubv4.ID(field.NodeID), + value: value, + }, work, results) + + return newUpdateProjectItemsResult(results) +} + +func batchTopLevelError(err error) *mcp.CallToolResult { + var structured *ghErrors.StructuredResolutionError + if errors.As(err, &structured) { + return ghErrors.NewStructuredResolutionErrorResponse(structured) + } + return utils.NewToolResultError(err.Error()) +} + +func newUpdateProjectItemsResult(results []batchItemResult) (*mcp.CallToolResult, any, error) { + succeeded, failed, unknown := 0, 0, 0 + for _, r := range results { + switch r.Status { + case batchItemSucceeded: + succeeded++ + case batchItemUnknown: + unknown++ + default: + failed++ + } + } + + response := map[string]any{ + "total": len(results), + "succeeded": succeeded, + "failed": failed, + "unknown": unknown, + "results": results, + } + r, err := json.Marshal(response) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal response: %w", err) + } + + result := utils.NewToolResultText(string(r)) + if succeeded == 0 { + result.IsError = true + } + return result, nil, nil +} + +func resolveItemReference(p parsedBatchItem, itemIDLookups map[int64]itemLookupResult, issueLookups map[issueRefKey]itemLookupResult) (nodeID string, fullDatabaseID int64, err error) { + switch p.refKind { + case batchRefNodeID: + return p.nodeID, 0, nil + case batchRefItemID: + lookup := itemIDLookups[p.itemID] + if lookup.err != nil { + return "", 0, lookup.err + } + return lookup.nodeID, p.itemID, nil + case batchRefIssue: + key := issueRefKey{owner: p.issueOwner, repo: p.issueRepo, number: p.issueNumber} + lookup := issueLookups[key] + if lookup.err != nil { + return "", 0, lookup.err + } + return lookup.nodeID, lookup.fullDatabaseID, nil + default: + return "", 0, fmt.Errorf("internal error: unrecognised item reference kind") + } +} + +// Transport, cancellation, or incomplete-data ambiguity stops later chunks; +// GraphQL response errors do not because populated aliases still confirm writes. +func executeBatchWrites(ctx context.Context, operation batchWriteOperation, items []resolvedBatchItem, results []batchItemResult) { + for start := 0; start < len(items); start += batchMutationWireChunkSize { + if ctx.Err() != nil { + markChunkUnknown(items[start:], results, ctx.Err()) + return + } + + end := min(start+batchMutationWireChunkSize, len(items)) + chunk := items[start:end] + + inputs := make([]githubv4.Input, len(chunk)) + for i, item := range chunk { + if operation.kind == batchMutationClear { + inputs[i] = githubv4.ClearProjectV2ItemFieldValueInput{ + ProjectID: operation.projectID, + ItemID: githubv4.ID(item.nodeID), + FieldID: operation.fieldID, + } + } else { + inputs[i] = githubv4.UpdateProjectV2ItemFieldValueInput{ + ProjectID: operation.projectID, + ItemID: githubv4.ID(item.nodeID), + FieldID: operation.fieldID, + Value: operation.value, + } + } + } + + outcomes, mutateErr := executeAliasedMutation(ctx, operation.gqlClient, operation.kind, inputs) + + populated := 0 + for i, oc := range outcomes { + if oc.Populated { + populated++ + results[chunk[i].index] = batchItemResult{ + Index: chunk[i].index, + Status: batchItemSucceeded, + Ref: chunk[i].ref, + Item: &batchItemIdentity{ + NodeID: oc.NodeID, + FullDatabaseID: oc.FullDatabaseID, + ItemID: chunk[i].fullDatabaseID, + }, + } + } + } + + if isGraphQLResponseError(mutateErr) { + markUnpopulatedUnknown(chunk, outcomes, results, mutateErr) + continue + } + + if mutateErr != nil { + markChunkUnknown(items[start:], results, mutateErr) + return + } + + if populated != len(chunk) { + markChunkUnknown(items[start:], results, fmt.Errorf("mutation response did not include every item")) + return + } + } +} + +func markUnpopulatedUnknown(chunk []resolvedBatchItem, outcomes []mutationAliasOutcome, results []batchItemResult, err error) { + for i, oc := range outcomes { + if oc.Populated { + continue + } + results[chunk[i].index] = batchItemResult{ + Index: chunk[i].index, + Status: batchItemUnknown, + Ref: chunk[i].ref, + Error: &batchItemError{Code: "mutation_unconfirmed", Message: err.Error()}, + } + } +} + +func markChunkUnknown(chunk []resolvedBatchItem, results []batchItemResult, err error) { + for _, item := range chunk { + if results[item.index].Status == batchItemSucceeded { + continue + } + results[item.index] = batchItemResult{ + Index: item.index, + Status: batchItemUnknown, + Ref: item.ref, + Error: &batchItemError{Code: "mutation_unconfirmed", Message: err.Error()}, + } + } +} + +const batchItemLookupConcurrency = 5 + +type batchItemRefKind int + +const ( + batchRefNodeID batchItemRefKind = iota + batchRefItemID + batchRefIssue +) + +type parsedBatchItem struct { + index int + ref map[string]any + refKind batchItemRefKind + + nodeID string + itemID int64 + + issueOwner string + issueRepo string + issueNumber int + + err *batchItemError +} + +func parseBatchItemEntry(index int, raw any) parsedBatchItem { + p := parsedBatchItem{index: index} + + entry, ok := raw.(map[string]any) + if !ok || entry == nil { + p.err = &batchItemError{Code: "invalid_item", Message: fmt.Sprintf("items[%d] must be an object", index)} + return p + } + p.ref = itemRefEcho(entry) + + if _, hasUpdatedField := entry["updated_field"]; hasUpdatedField { + p.err = &batchItemError{Code: "invalid_item", Message: fmt.Sprintf("items[%d].updated_field is not supported; use the top-level updated_field", index)} + return p + } + + if refErr := p.parseItemRef(entry); refErr != nil { + p.err = &batchItemError{Code: "invalid_item_ref", Message: refErr.Error()} + } + return p +} + +func (p *parsedBatchItem) parseItemRef(entry map[string]any) error { + _, hasNodeID := entry["node_id"] + _, hasItemID := entry["item_id"] + _, hasOwner := entry["item_owner"] + _, hasRepo := entry["item_repo"] + _, hasIssueNumber := entry["issue_number"] + hasIssueRef := hasOwner || hasRepo || hasIssueNumber + + formsPresent := 0 + if hasNodeID { + formsPresent++ + } + if hasItemID { + formsPresent++ + } + if hasIssueRef { + formsPresent++ + } + + switch { + case formsPresent == 0: + return fmt.Errorf("each item requires exactly one of node_id, item_id, or item_owner + item_repo + issue_number") + case formsPresent > 1: + return fmt.Errorf("each item must set exactly one of node_id, item_id, or item_owner + item_repo + issue_number, not more than one") + } + + switch { + case hasNodeID: + s, ok := entry["node_id"].(string) + if !ok || s == "" { + return fmt.Errorf("node_id must be a non-empty string") + } + p.refKind = batchRefNodeID + p.nodeID = s + case hasItemID: + id, err := validatePositiveInt64(entry["item_id"]) + if err != nil { + return fmt.Errorf("item_id: %w", err) + } + p.refKind = batchRefItemID + p.itemID = id + default: + issueOwner, ownerErr := stringFromEntry(entry, "item_owner") + issueRepo, repoErr := stringFromEntry(entry, "item_repo") + issueNumber, numErr := intFromEntry(entry, "issue_number") + for _, err := range []error{ownerErr, repoErr, numErr} { + if err != nil { + return fmt.Errorf("item_owner, item_repo, and issue_number must all be provided together: %w", err) + } + } + p.refKind = batchRefIssue + p.issueOwner = issueOwner + p.issueRepo = issueRepo + p.issueNumber = issueNumber + } + return nil +} + +func itemRefEcho(entry map[string]any) map[string]any { + ref := map[string]any{} + for _, key := range []string{"node_id", "item_id", "item_owner", "item_repo", "issue_number"} { + if v, ok := entry[key]; ok { + ref[key] = v + } + } + if len(ref) == 0 { + return nil + } + return ref +} + +func stringFromEntry(entry map[string]any, key string) (string, error) { + v, ok := entry[key] + if !ok { + return "", fmt.Errorf("missing %s", key) + } + s, ok := v.(string) + if !ok || s == "" { + return "", fmt.Errorf("%s must be a non-empty string", key) + } + return s, nil +} + +func intFromEntry(entry map[string]any, key string) (int, error) { + v, ok := entry[key] + if !ok { + return 0, fmt.Errorf("missing %s", key) + } + n, err := validatePositiveInt64(v) + if err != nil { + return 0, fmt.Errorf("%s must be a positive integer: %w", key, err) + } + if n > math.MaxInt32 { + return 0, fmt.Errorf("%s exceeds the GraphQL Int maximum of %d", key, int64(math.MaxInt32)) + } + return int(n), nil +} + +func validatePositiveInt64(value any) (int64, error) { + n, err := validateAndConvertToInt64(value) + if err != nil { + return 0, err + } + if n <= 0 { + return 0, fmt.Errorf("value must be greater than zero (got %d)", n) + } + return n, nil +} + +type batchFieldSpec struct { + id int64 + name string + value any +} + +func parseBatchFieldSpec(raw any) (batchFieldSpec, error) { + var spec batchFieldSpec + input, ok := raw.(map[string]any) + if !ok || input == nil { + return spec, fmt.Errorf("updated_field must be an object") + } + + value, hasValue := input["value"] + if !hasValue { + return spec, fmt.Errorf("updated_field.value is required") + } + spec.value = value + + idField, hasID := input["id"] + nameField, hasName := input["name"] + switch { + case hasID && hasName: + return spec, fmt.Errorf("updated_field must set either id or name, not both") + case !hasID && !hasName: + return spec, fmt.Errorf("updated_field requires either id or name") + case hasID: + id, err := validatePositiveInt64(idField) + if err != nil { + return spec, fmt.Errorf("updated_field.id: %w", err) + } + spec.id = id + default: + name, ok := nameField.(string) + if !ok || name == "" { + return spec, fmt.Errorf("updated_field.name must be a non-empty string") + } + spec.name = name + } + return spec, nil +} + +func resolveBatchProjectField(ctx context.Context, gqlClient *githubv4.Client, owner, ownerType string, projectNumber int, spec batchFieldSpec) (*ResolvedField, error) { + if spec.name != "" { + return resolveProjectFieldByName(ctx, gqlClient, owner, ownerType, projectNumber, spec.name, "") + } + + fields, err := listAllProjectFields(ctx, gqlClient, owner, ownerType, projectNumber) + if err != nil { + return nil, err + } + + id := fmt.Sprintf("%d", spec.id) + for _, field := range fields { + if field.ID == id { + return &field, nil + } + } + return nil, ghErrors.NewStructuredResolutionError( + "field_not_found", + id, + fmt.Sprintf("no project field with id %s on project %s#%d; see candidates for available fields", id, owner, projectNumber), + projectFieldCandidates(fields), + ) +} + +func projectFieldCandidates(fields []ResolvedField) []any { + candidates := make([]any, 0, len(fields)) + for _, field := range fields { + candidates = append(candidates, map[string]any{ + "id": field.ID, + "name": field.Name, + "data_type": field.DataType, + }) + } + return candidates +} + +func convertProjectFieldValue(field *ResolvedField, raw any) (githubv4.ProjectV2FieldValue, error) { + var zero githubv4.ProjectV2FieldValue + + switch field.DataType { + case "TEXT": + s, ok := raw.(string) + if !ok { + return zero, fmt.Errorf("field %q is TEXT; value must be a string", field.Name) + } + v := githubv4.String(s) + return githubv4.ProjectV2FieldValue{Text: &v}, nil + + case "NUMBER": + f, ok := toFloat64(raw) + if !ok { + return zero, fmt.Errorf("field %q is NUMBER; value must be a number", field.Name) + } + v := githubv4.Float(f) + return githubv4.ProjectV2FieldValue{Number: &v}, nil + + case "DATE": + s, ok := raw.(string) + if !ok { + return zero, fmt.Errorf("field %q is DATE; value must be a YYYY-MM-DD string", field.Name) + } + t, err := time.Parse("2006-01-02", s) + if err != nil { + return zero, fmt.Errorf("field %q is DATE; value %q is not in YYYY-MM-DD format: %w", field.Name, s, err) + } + return githubv4.ProjectV2FieldValue{Date: &githubv4.Date{Time: t}}, nil + + case "SINGLE_SELECT": + s, ok := raw.(string) + if !ok || s == "" { + return zero, fmt.Errorf("field %q is SINGLE_SELECT; value must be a non-empty string (option name or ID)", field.Name) + } + optID := s + if resolvedID, optErr := resolveSingleSelectOptionByName(field, s); optErr == nil { + optID = resolvedID + } else { + known := false + for _, opt := range field.Options { + if opt.ID == s { + known = true + break + } + } + if !known { + return zero, optErr + } + } + v := githubv4.String(optID) + return githubv4.ProjectV2FieldValue{SingleSelectOptionID: &v}, nil + + case "ITERATION": + s, ok := raw.(string) + if !ok || s == "" { + return zero, fmt.Errorf("field %q is ITERATION; value must be a non-empty iteration ID string", field.Name) + } + v := githubv4.String(s) + return githubv4.ProjectV2FieldValue{IterationID: &v}, nil + + default: + return zero, fmt.Errorf("field %q has unsupported data type %q for update_project_items; use update_project_item instead", field.Name, field.DataType) + } +} + +func toFloat64(raw any) (float64, bool) { + var number float64 + switch v := raw.(type) { + case float64: + number = v + case int: + number = float64(v) + case int64: + number = float64(v) + default: + return 0, false + } + if math.IsNaN(number) || math.IsInf(number, 0) { + return 0, false + } + return number, true +} + +type itemLookupResult struct { + nodeID string + fullDatabaseID int64 + err error +} + +// Numeric lookups are deduplicated and concurrency-bounded; individual failures +// remain isolated while cancellation stops pending work. +func resolveItemNodeIDsByNumericID(ctx context.Context, client *github.Client, owner, ownerType string, projectNumber int, ids []int64) map[int64]itemLookupResult { + seen := make(map[int64]struct{}, len(ids)) + var unique []int64 + for _, id := range ids { + if _, dup := seen[id]; dup { + continue + } + seen[id] = struct{}{} + unique = append(unique, id) + } + + out := make(map[int64]itemLookupResult, len(unique)) + if len(unique) == 0 { + return out + } + + var mu sync.Mutex + var wg sync.WaitGroup + sem := make(chan struct{}, batchItemLookupConcurrency) + + for _, id := range unique { + wg.Add(1) + go func(id int64) { + defer wg.Done() + + select { + case sem <- struct{}{}: + case <-ctx.Done(): + mu.Lock() + out[id] = itemLookupResult{err: ctx.Err()} + mu.Unlock() + return + } + defer func() { <-sem }() + + if ctx.Err() != nil { + mu.Lock() + out[id] = itemLookupResult{err: ctx.Err()} + mu.Unlock() + return + } + + var item *github.ProjectV2Item + var err error + if ownerType == "org" { + item, _, err = client.Projects.GetOrganizationProjectItem(ctx, owner, projectNumber, id, nil) + } else { + item, _, err = client.Projects.GetUserProjectItem(ctx, owner, projectNumber, id, nil) + } + + var res itemLookupResult + switch { + case err != nil: + res = itemLookupResult{err: fmt.Errorf("project item %d: %w", id, err)} + case item == nil || item.NodeID == nil || *item.NodeID == "": + res = itemLookupResult{err: fmt.Errorf("project item %d: response did not include a node id", id)} + default: + res = itemLookupResult{nodeID: *item.NodeID, fullDatabaseID: id} + } + + mu.Lock() + out[id] = res + mu.Unlock() + }(id) + } + wg.Wait() + return out +} + +type issueRefKey struct { + owner string + repo string + number int +} + +func resolveIssueRefs(ctx context.Context, gqlClient *githubv4.Client, projectID githubv4.ID, items []parsedBatchItem) map[issueRefKey]itemLookupResult { + out := make(map[issueRefKey]itemLookupResult) + for _, it := range items { + if it.err != nil || it.refKind != batchRefIssue { + continue + } + key := issueRefKey{owner: it.issueOwner, repo: it.issueRepo, number: it.issueNumber} + if _, done := out[key]; done { + continue + } + nodeID, itemID, err := resolveProjectItemByIssueNumberWithProjectID(ctx, gqlClient, projectID, it.issueOwner, it.issueRepo, it.issueNumber) + out[key] = itemLookupResult{nodeID: nodeID, fullDatabaseID: itemID, err: err} + } + return out +} + +func batchErrorFromResolution(err error) *batchItemError { + var structured *ghErrors.StructuredResolutionError + if errors.As(err, &structured) { + return &batchItemError{ + Code: structured.Kind, + Message: fmt.Sprintf("%s: %s", structured.Kind, structured.Name), + Hint: structured.Hint, + Candidates: structured.Candidates, + } + } + return &batchItemError{ + Code: "invalid_argument", + Message: err.Error(), + } +} diff --git a/pkg/github/projects_batch_test.go b/pkg/github/projects_batch_test.go new file mode 100644 index 0000000000..48e4db16aa --- /dev/null +++ b/pkg/github/projects_batch_test.go @@ -0,0 +1,512 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + "math" + "net/http" + "testing" + + "github.com/github/github-mcp-server/internal/githubv4mock" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_UpdateProjectItemsBatch_TopLevelGuards(t *testing.T) { + tooMany := make([]any, maxProjectItemsPerBatch+1) + validItem := map[string]any{"node_id": "PVTI_item1"} + validField := map[string]any{"name": "Notes", "value": "hello"} + tests := []struct { + name string + args map[string]any + wantErr string + }{ + {name: "missing items", args: map[string]any{}, wantErr: "missing required parameter: items"}, + {name: "non-array items", args: map[string]any{"items": "invalid"}, wantErr: "items must be an array"}, + {name: "empty items", args: map[string]any{"items": []any{}}, wantErr: "items must contain at least one entry"}, + {name: "too many items", args: map[string]any{"items": tooMany}, wantErr: "items exceeds maximum of 50 entries"}, + {name: "missing updated field", args: map[string]any{"items": []any{validItem}}, wantErr: "missing required parameter: updated_field"}, + {name: "malformed updated field", args: map[string]any{"items": []any{validItem}, "updated_field": "invalid"}, wantErr: "updated_field must be an object"}, + {name: "missing field value", args: map[string]any{"items": []any{validItem}, "updated_field": map[string]any{"name": "Notes"}}, wantErr: "updated_field.value is required"}, + {name: "missing field reference", args: map[string]any{"items": []any{validItem}, "updated_field": map[string]any{"value": "hello"}}, wantErr: "updated_field requires either id or name"}, + {name: "ambiguous field reference", args: map[string]any{"items": []any{validItem}, "updated_field": map[string]any{"id": float64(1), "name": "Notes", "value": "hello"}}, wantErr: "updated_field must set either id or name"}, + {name: "nil GraphQL client", args: map[string]any{"items": []any{validItem}, "updated_field": validField}, wantErr: "gqlClient is required"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, structured, err := updateProjectItemsBatch(t.Context(), nil, nil, "octo-org", "org", 1, tt.args) + require.NoError(t, err) + assert.Nil(t, structured) + assert.Contains(t, getErrorResult(t, result).Text, tt.wantErr) + }) + } +} + +func Test_ParseItemRef_ExactlyOneFormRequired(t *testing.T) { + tests := []struct { + name string + entry map[string]any + wantErr string + }{ + { + name: "none provided", + entry: map[string]any{}, + wantErr: "exactly one of", + }, + { + name: "node_id and item_id both provided", + entry: map[string]any{"node_id": "PVTI_x", "item_id": float64(1)}, + wantErr: "not more than one", + }, + { + name: "item_id and issue ref both provided", + entry: map[string]any{"item_id": float64(1), "item_owner": "o", "item_repo": "r", "issue_number": float64(1)}, + wantErr: "not more than one", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := parsedBatchItem{} + err := p.parseItemRef(tt.entry) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +} + +func Test_ParseItemRef_NodeIDBypassesLookup(t *testing.T) { + p := parsedBatchItem{} + err := p.parseItemRef(map[string]any{"node_id": "PVTI_abc123"}) + require.NoError(t, err) + assert.Equal(t, batchRefNodeID, p.refKind) + assert.Equal(t, "PVTI_abc123", p.nodeID) +} + +func Test_ParseItemRef_ItemID(t *testing.T) { + p := parsedBatchItem{} + err := p.parseItemRef(map[string]any{"item_id": float64(42)}) + require.NoError(t, err) + assert.Equal(t, batchRefItemID, p.refKind) + assert.Equal(t, int64(42), p.itemID) +} + +func Test_ParseItemRef_IssueRef(t *testing.T) { + p := parsedBatchItem{} + err := p.parseItemRef(map[string]any{"item_owner": "github", "item_repo": "planning-tracking", "issue_number": float64(123)}) + require.NoError(t, err) + assert.Equal(t, batchRefIssue, p.refKind) + assert.Equal(t, "github", p.issueOwner) + assert.Equal(t, "planning-tracking", p.issueRepo) + assert.Equal(t, 123, p.issueNumber) +} + +func Test_ParseItemRef_InvalidNumericReferences(t *testing.T) { + issueRef := func(value any) map[string]any { + return map[string]any{ + "item_owner": "github", + "item_repo": "planning-tracking", + "issue_number": value, + } + } + tests := []struct { + name string + entry map[string]any + }{ + {name: "zero item ID", entry: map[string]any{"item_id": float64(0)}}, + {name: "negative item ID", entry: map[string]any{"item_id": float64(-1)}}, + {name: "fractional item ID", entry: map[string]any{"item_id": float64(1.5)}}, + {name: "NaN item ID", entry: map[string]any{"item_id": math.NaN()}}, + {name: "infinite item ID", entry: map[string]any{"item_id": math.Inf(1)}}, + {name: "overflowing item ID", entry: map[string]any{"item_id": math.MaxFloat64}}, + {name: "zero issue number", entry: issueRef(float64(0))}, + {name: "negative issue number", entry: issueRef(float64(-1))}, + {name: "fractional issue number", entry: issueRef(float64(1.5))}, + {name: "overflowing issue number", entry: issueRef(float64(math.MaxInt32) + 1)}, + {name: "NaN issue number", entry: issueRef(math.NaN())}, + {name: "infinite issue number", entry: issueRef(math.Inf(1))}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := parsedBatchItem{} + err := p.parseItemRef(tt.entry) + require.Error(t, err) + }) + } +} + +func Test_ParseItemRef_PartialIssueRefIsError(t *testing.T) { + p := parsedBatchItem{} + err := p.parseItemRef(map[string]any{"item_owner": "github"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "must all be provided together") +} + +func Test_ParseBatchItemEntry_InvalidShape(t *testing.T) { + p := parseBatchItemEntry(0, "not-an-object") + require.NotNil(t, p.err) + assert.Equal(t, "invalid_item", p.err.Code) +} + +func Test_ParseBatchItemEntry_RejectsPerItemUpdatedField(t *testing.T) { + p := parseBatchItemEntry(0, map[string]any{ + "node_id": "PVTI_1", + "updated_field": map[string]any{"name": "Notes", "value": "x"}, + }) + require.NotNil(t, p.err) + assert.Contains(t, p.err.Message, "use the top-level updated_field") +} + +func Test_ConvertProjectFieldValue_Text(t *testing.T) { + field := &ResolvedField{Name: "Notes", DataType: "TEXT"} + v, err := convertProjectFieldValue(field, "hello") + require.NoError(t, err) + require.NotNil(t, v.Text) + assert.Equal(t, "hello", string(*v.Text)) +} + +func Test_ConvertProjectFieldValue_Text_WrongType(t *testing.T) { + field := &ResolvedField{Name: "Notes", DataType: "TEXT"} + _, err := convertProjectFieldValue(field, float64(1)) + require.Error(t, err) +} + +func Test_ConvertProjectFieldValue_Number(t *testing.T) { + field := &ResolvedField{Name: "Estimate", DataType: "NUMBER"} + v, err := convertProjectFieldValue(field, float64(8)) + require.NoError(t, err) + require.NotNil(t, v.Number) + assert.InDelta(t, 8.0, float64(*v.Number), 0.0001) +} + +func Test_ConvertProjectFieldValue_Number_NonFinite(t *testing.T) { + field := &ResolvedField{Name: "Estimate", DataType: "NUMBER"} + for _, value := range []float64{math.NaN(), math.Inf(-1), math.Inf(1)} { + _, err := convertProjectFieldValue(field, value) + require.Error(t, err) + } +} + +func Test_ConvertProjectFieldValue_Date(t *testing.T) { + field := &ResolvedField{Name: "Due", DataType: "DATE"} + v, err := convertProjectFieldValue(field, "2024-01-15") + require.NoError(t, err) + require.NotNil(t, v.Date) + assert.Equal(t, 2024, v.Date.Year()) + assert.Equal(t, 1, int(v.Date.Month())) + assert.Equal(t, 15, v.Date.Day()) +} + +func Test_ConvertProjectFieldValue_Date_BadFormat(t *testing.T) { + field := &ResolvedField{Name: "Due", DataType: "DATE"} + _, err := convertProjectFieldValue(field, "01/15/2024") + require.Error(t, err) +} + +func Test_ConvertProjectFieldValue_SingleSelect_ByName(t *testing.T) { + field := &ResolvedField{ + Name: "Status", + DataType: "SINGLE_SELECT", + Options: []ResolvedFieldOption{{ID: "OPT_1", Name: "In Progress"}}, + } + v, err := convertProjectFieldValue(field, "In Progress") + require.NoError(t, err) + require.NotNil(t, v.SingleSelectOptionID) + assert.Equal(t, "OPT_1", string(*v.SingleSelectOptionID)) +} + +func Test_ConvertProjectFieldValue_SingleSelect_ByOptionID(t *testing.T) { + field := &ResolvedField{ + Name: "Status", + DataType: "SINGLE_SELECT", + Options: []ResolvedFieldOption{{ID: "OPT_1", Name: "In Progress"}}, + } + v, err := convertProjectFieldValue(field, "OPT_1") + require.NoError(t, err) + require.NotNil(t, v.SingleSelectOptionID) + assert.Equal(t, "OPT_1", string(*v.SingleSelectOptionID)) +} + +func Test_ConvertProjectFieldValue_SingleSelect_Unknown(t *testing.T) { + field := &ResolvedField{ + Name: "Status", + DataType: "SINGLE_SELECT", + Options: []ResolvedFieldOption{{ID: "OPT_1", Name: "In Progress"}}, + } + _, err := convertProjectFieldValue(field, "Nonexistent") + require.Error(t, err) +} + +func Test_ConvertProjectFieldValue_Iteration(t *testing.T) { + field := &ResolvedField{Name: "Sprint", DataType: "ITERATION"} + v, err := convertProjectFieldValue(field, "abc123==") + require.NoError(t, err) + require.NotNil(t, v.IterationID) + assert.Equal(t, "abc123==", string(*v.IterationID)) +} + +func Test_ConvertProjectFieldValue_Iteration_EmptyIsError(t *testing.T) { + field := &ResolvedField{Name: "Sprint", DataType: "ITERATION"} + _, err := convertProjectFieldValue(field, "") + require.Error(t, err) +} + +func Test_ConvertProjectFieldValue_UnsupportedDataType(t *testing.T) { + field := &ResolvedField{Name: "Assignees", DataType: "ASSIGNEES"} + _, err := convertProjectFieldValue(field, "someone") + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported data type") + assert.Contains(t, err.Error(), "update_project_item") +} + +func Test_ResolveBatchProjectField_ByIDAndName(t *testing.T) { + tests := []struct { + name string + spec batchFieldSpec + wantID string + }{ + {name: "numeric ID", spec: batchFieldSpec{id: 101}, wantID: "PVTF_status"}, + {name: "case-insensitive name", spec: batchFieldSpec{name: "priority"}, wantID: "PVTF_priority"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mocked := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 7), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + statusFieldNode("PVTF_status", 101, "Status", nil), + statusFieldNode("PVTF_priority", 202, "Priority", nil), + })), + ), + ) + + field, err := resolveBatchProjectField(t.Context(), githubv4.NewClient(mocked), "octo-org", "org", 7, tt.spec) + require.NoError(t, err) + assert.Equal(t, tt.wantID, field.NodeID) + }) + } +} + +func Test_ResolveBatchProjectField_AmbiguousName(t *testing.T) { + mocked := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 7), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + statusFieldNode("PVTSSF_status1", 101, "Status", nil), + statusFieldNode("PVTSSF_status2", 202, "Status", nil), + })), + ), + ) + + _, err := resolveBatchProjectField( + t.Context(), + githubv4.NewClient(mocked), + "octo-org", + "org", + 7, + batchFieldSpec{name: "status"}, + ) + require.Error(t, err) + + var response struct { + Error string `json:"error"` + Candidates []map[string]any `json:"candidates"` + } + require.NoError(t, json.Unmarshal([]byte(err.Error()), &response)) + assert.Equal(t, "field_ambiguous", response.Error) + require.Len(t, response.Candidates, 2) + assert.ElementsMatch(t, []any{"101", "202"}, []any{response.Candidates[0]["id"], response.Candidates[1]["id"]}) +} + +func Test_ResolveItemNodeIDsByNumericID_DeduplicatesOrgAndUserLookups(t *testing.T) { + tests := []struct { + name string + ownerType string + endpoint string + }{ + {name: "organization", ownerType: "org", endpoint: GetOrgsProjectsV2ItemsByProjectByItemID}, + {name: "user", ownerType: "user", endpoint: GetUsersProjectsV2ItemsByUsernameByProjectByItemID}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + calls := 0 + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + tt.endpoint: func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":1001,"node_id":"PVTI_item1001"}`)) + }, + })) + + resolved := resolveItemNodeIDsByNumericID(t.Context(), client, "octocat", tt.ownerType, 1, []int64{1001, 1001}) + + require.NoError(t, resolved[1001].err) + assert.Equal(t, "PVTI_item1001", resolved[1001].nodeID) + assert.Equal(t, 1, calls) + }) + } +} + +func Test_ExecuteBatchWrites_AllAliasGraphQLErrorContinues(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(_ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationErrorResponse(t, map[string]any{}, "all aliases failed") + }, + func(_ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item20", FullDatabaseID: "1020"}, + }) + }, + }, + } + items, results := batchItemsOfSize(21) + + executeTestBatchWrites(t.Context(), newTestGQLClient(transport), items, results) + + assert.Len(t, transport.calls, 2) + for i := range 20 { + assert.Equal(t, batchItemUnknown, results[i].Status) + } + assert.Equal(t, batchItemSucceeded, results[20].Status) +} + +func Test_ExecuteBatchWrites_PartialGraphQLErrorPreservesSuccess(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(_ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationErrorResponse(t, map[string]any{ + "item0": map[string]any{ + "projectV2Item": map[string]any{"id": "PVTI_item0", "fullDatabaseId": "1000"}, + }, + "item1": nil, + }, "item1 failed") + }, + }, + } + items, results := batchItemsOfSize(2) + + executeTestBatchWrites(t.Context(), newTestGQLClient(transport), items, results) + + assert.Equal(t, batchItemSucceeded, results[0].Status) + assert.Equal(t, items[0].ref, results[0].Ref) + assert.Equal(t, batchItemUnknown, results[1].Status) + assert.Equal(t, items[1].ref, results[1].Ref) +} + +func Test_ExecuteBatchWrites_AmbiguousSuccessResponseAborts(t *testing.T) { + tests := []struct { + name string + body string + confirmedSuccesses int + }{ + { + name: "null data", + body: `{"data":null}`, + }, + { + name: "missing data", + body: `{}`, + }, + { + name: "partial data without errors", + body: mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item0", FullDatabaseID: "1000"}, + }), + confirmedSuccesses: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(_ capturedGraphQLRequest) (int, string) { + return http.StatusOK, tt.body + }, + func(_ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item20", FullDatabaseID: "1020"}, + }) + }, + }, + } + items, results := batchItemsOfSize(21) + + executeTestBatchWrites(t.Context(), newTestGQLClient(transport), items, results) + + assert.Len(t, transport.calls, 1) + for i, result := range results { + if i < tt.confirmedSuccesses { + assert.Equal(t, batchItemSucceeded, result.Status) + continue + } + assert.Equal(t, batchItemUnknown, result.Status) + } + }) + } +} + +func Test_ExecuteBatchWrites_TransportTimeoutAborts(t *testing.T) { + transport := &errorGraphQLTransport{err: context.DeadlineExceeded} + items, results := batchItemsOfSize(21) + + executeTestBatchWrites(t.Context(), newTestGQLClient(transport), items, results) + + assert.Equal(t, 1, transport.calls) + for _, result := range results { + assert.Equal(t, batchItemUnknown, result.Status) + } +} + +func Test_ExecuteBatchWrites_CanceledContextSkipsWrites(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + transport := &sequencedGraphQLTransport{t: t} + items, results := batchItemsOfSize(21) + + executeTestBatchWrites(ctx, newTestGQLClient(transport), items, results) + + assert.Empty(t, transport.calls) + for _, result := range results { + assert.Equal(t, batchItemUnknown, result.Status) + } +} + +func executeTestBatchWrites(ctx context.Context, gqlClient *githubv4.Client, items []resolvedBatchItem, results []batchItemResult) { + executeBatchWrites( + ctx, + batchWriteOperation{ + gqlClient: gqlClient, + kind: batchMutationUpdate, + projectID: githubv4.ID("PVT_project"), + fieldID: githubv4.ID("PVTF_field"), + value: githubv4.ProjectV2FieldValue{Text: githubv4.NewString("value")}, + }, + items, + results, + ) +} + +func batchItemsOfSize(n int) ([]resolvedBatchItem, []batchItemResult) { + items := make([]resolvedBatchItem, n) + for i := range n { + nodeID := fmt.Sprintf("PVTI_item%d", i) + items[i] = resolvedBatchItem{ + index: i, + ref: map[string]any{"node_id": nodeID}, + nodeID: nodeID, + } + } + return items, make([]batchItemResult, n) +} From 9928ec60b6b62acd90594f11ea0bab8d9fd44cbf Mon Sep 17 00:00:00 2001 From: Bryan Zwicker Date: Wed, 22 Jul 2026 12:55:40 -0400 Subject: [PATCH 2/7] Expose update_project_items Add the public projects_write contract, routing, handler coverage, and generated documentation for shared field updates across batches of up to 50 items. Co-authored-by: Lizeth Vera <47796851+veralizeth@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ae767ff-c1d0-46a9-b126-2e91403993a0 --- README.md | 3 +- pkg/github/__toolsnaps__/projects_write.snap | 99 ++- pkg/github/projects.go | 91 ++- pkg/github/projects_batch_test.go | 779 +++++++++++++++++++ pkg/github/projects_test.go | 61 +- 5 files changed, 1025 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 1a06c0697d..1f89dafd06 100644 --- a/README.md +++ b/README.md @@ -1121,6 +1121,7 @@ The following sets of tools are available: - `item_owner`: The owner (user or organization) of the repository containing the issue or pull request. Required for 'add_project_item' method. Also accepted by 'update_project_item' when resolving the item by issue number. (string, optional) - `item_repo`: The name of the repository containing the issue or pull request. Required for 'add_project_item' method. Also accepted by 'update_project_item' when resolving the item by issue number. (string, optional) - `item_type`: The item's type, either issue or pull_request. Required for 'add_project_item' method. (string, optional) + - `items`: The items to update with the top-level 'updated_field'. Required for 'update_project_items'; prefer it over calling 'update_project_item' in a loop. Each entry must match exactly one reference variant: 'node_id', numeric 'item_id', or 'item_owner' + 'item_repo' + 'issue_number'. Limit: 50 items per call. (object[], optional) - `iteration_duration`: Duration in days for iterations of the field (e.g. 7 for weekly, 14 for bi-weekly). Required for 'create_iteration_field' method. (number, optional) - `iterations`: Custom iterations for 'create_iteration_field' method. Only set this when you need iterations with varying durations, breaks between them, or specific titles. Otherwise omit it: GitHub auto-creates three iterations of 'iteration_duration' days starting on 'start_date', which is the right choice for most cases. (object[], optional) - `method`: The method to execute (string, required) @@ -1132,7 +1133,7 @@ The following sets of tools are available: - `status`: The status of the project. Used for 'create_project_status_update' method. (string, optional) - `target_date`: The target date of the status update in YYYY-MM-DD format. Used for 'create_project_status_update' method. (string, optional) - `title`: The project title. Required for 'create_project' method. (string, optional) - - `updated_field`: Object describing the field to update and its new value. Required for 'update_project_item'. Two shapes are accepted: (1) by ID — {"id": 123456, "value": "..."}; (2) by name — {"name": "Status", "value": "In Progress"}. For single-select fields, option-name resolution requires the by-name shape; on the by-ID shape, pass the option ID. Set value to null to clear the field. (object, optional) + - `updated_field`: The field/value to apply, using {"id": 123, "value": ...} or {"name": "Status", "value": ...}; null clears the field. Required for 'update_project_item' and 'update_project_items', where one top-level field/value applies to every item in a batch. For 'update_project_item' SINGLE_SELECT fields, the name form accepts option names; the ID form expects an option ID. (object, optional) diff --git a/pkg/github/__toolsnaps__/projects_write.snap b/pkg/github/__toolsnaps__/projects_write.snap index 762ee08c93..d7c5d25eab 100644 --- a/pkg/github/__toolsnaps__/projects_write.snap +++ b/pkg/github/__toolsnaps__/projects_write.snap @@ -5,7 +5,7 @@ "readOnlyHint": false, "title": "Manage GitHub Projects" }, - "description": "Create and manage GitHub Projects: create projects, add/update/delete items, create status updates, and add iteration fields.", + "description": "Create and manage GitHub Projects: create projects, add/update/delete items, bulk-update many items at once, create status updates, and add iteration fields.", "inputSchema": { "properties": { "body": { @@ -40,6 +40,64 @@ ], "type": "string" }, + "items": { + "description": "The items to update with the top-level 'updated_field'. Required for 'update_project_items'; prefer it over calling 'update_project_item' in a loop. Each entry must match exactly one reference variant: 'node_id', numeric 'item_id', or 'item_owner' + 'item_repo' + 'issue_number'. Limit: 50 items per call.", + "items": { + "oneOf": [ + { + "additionalProperties": false, + "properties": { + "node_id": { + "description": "The project item's GraphQL node ID, as returned by 'list_project_items' or 'add_project_item'.", + "type": "string" + } + }, + "required": [ + "node_id" + ], + "type": "object" + }, + { + "additionalProperties": false, + "properties": { + "item_id": { + "description": "The numeric project item ID.", + "type": "integer" + } + }, + "required": [ + "item_id" + ], + "type": "object" + }, + { + "additionalProperties": false, + "properties": { + "issue_number": { + "description": "Issue number used to resolve the project item.", + "type": "integer" + }, + "item_owner": { + "description": "Owner of the repository containing the issue.", + "type": "string" + }, + "item_repo": { + "description": "Repository containing the issue.", + "type": "string" + } + }, + "required": [ + "item_owner", + "item_repo", + "issue_number" + ], + "type": "object" + } + ], + "type": "object" + }, + "type": "array" + }, "iteration_duration": { "description": "Duration in days for iterations of the field (e.g. 7 for weekly, 14 for bi-weekly). Required for 'create_iteration_field' method.", "type": "number" @@ -76,6 +134,7 @@ "enum": [ "add_project_item", "update_project_item", + "update_project_items", "delete_project_item", "create_project_status_update", "create_project", @@ -127,7 +186,43 @@ "type": "string" }, "updated_field": { - "description": "Object describing the field to update and its new value. Required for 'update_project_item'. Two shapes are accepted: (1) by ID — {\"id\": 123456, \"value\": \"...\"}; (2) by name — {\"name\": \"Status\", \"value\": \"In Progress\"}. For single-select fields, option-name resolution requires the by-name shape; on the by-ID shape, pass the option ID. Set value to null to clear the field.", + "description": "The field/value to apply, using {\"id\": 123, \"value\": ...} or {\"name\": \"Status\", \"value\": ...}; null clears the field. Required for 'update_project_item' and 'update_project_items', where one top-level field/value applies to every item in a batch. For 'update_project_item' SINGLE_SELECT fields, the name form accepts option names; the ID form expects an option ID.", + "oneOf": [ + { + "additionalProperties": false, + "properties": { + "id": { + "description": "The numeric project field ID.", + "type": "integer" + }, + "value": { + "description": "The value to apply. Any JSON value is accepted; use null to clear the field." + } + }, + "required": [ + "id", + "value" + ], + "type": "object" + }, + { + "additionalProperties": false, + "properties": { + "name": { + "description": "The project field name. Matching is case-insensitive.", + "type": "string" + }, + "value": { + "description": "The value to apply. Any JSON value is accepted; use null to clear the field." + } + }, + "required": [ + "name", + "value" + ], + "type": "object" + } + ], "type": "object" } }, diff --git a/pkg/github/projects.go b/pkg/github/projects.go index 8dd3df3ab2..514964be93 100644 --- a/pkg/github/projects.go +++ b/pkg/github/projects.go @@ -45,6 +45,7 @@ const ( projectsMethodGetProjectItem = "get_project_item" projectsMethodAddProjectItem = "add_project_item" projectsMethodUpdateProjectItem = "update_project_item" + projectsMethodUpdateProjectItems = "update_project_items" projectsMethodDeleteProjectItem = "delete_project_item" projectsMethodListProjectStatusUpdates = "list_project_status_updates" projectsMethodGetProjectStatusUpdate = "get_project_status_update" @@ -491,13 +492,90 @@ Use this tool to get details about individual projects, project fields, and proj return tool } +func updateProjectItemsItemSchema() *jsonschema.Schema { + variant := func(required []string, properties map[string]*jsonschema.Schema) *jsonschema.Schema { + return &jsonschema.Schema{ + Type: "object", + AdditionalProperties: &jsonschema.Schema{Not: &jsonschema.Schema{}}, + Properties: properties, + Required: required, + } + } + + return &jsonschema.Schema{ + Type: "object", + OneOf: []*jsonschema.Schema{ + variant([]string{"node_id"}, map[string]*jsonschema.Schema{ + "node_id": { + Type: "string", + Description: "The project item's GraphQL node ID, as returned by 'list_project_items' or 'add_project_item'.", + }, + }), + variant([]string{"item_id"}, map[string]*jsonschema.Schema{ + "item_id": { + Type: "integer", + Description: "The numeric project item ID.", + }, + }), + variant([]string{"item_owner", "item_repo", "issue_number"}, map[string]*jsonschema.Schema{ + "item_owner": { + Type: "string", + Description: "Owner of the repository containing the issue.", + }, + "item_repo": { + Type: "string", + Description: "Repository containing the issue.", + }, + "issue_number": { + Type: "integer", + Description: "Issue number used to resolve the project item.", + }, + }), + }, + } +} + +func projectUpdatedFieldSchema() *jsonschema.Schema { + value := &jsonschema.Schema{ + Description: "The value to apply. Any JSON value is accepted; use null to clear the field.", + } + variant := func(required []string, properties map[string]*jsonschema.Schema) *jsonschema.Schema { + properties["value"] = value + return &jsonschema.Schema{ + Type: "object", + AdditionalProperties: &jsonschema.Schema{Not: &jsonschema.Schema{}}, + Properties: properties, + Required: required, + } + } + + return &jsonschema.Schema{ + Type: "object", + Description: "The field/value to apply, using {\"id\": 123, \"value\": ...} or {\"name\": \"Status\", \"value\": ...}; null clears the field. Required for 'update_project_item' and 'update_project_items', where one top-level field/value applies to every item in a batch. For 'update_project_item' SINGLE_SELECT fields, the name form accepts option names; the ID form expects an option ID.", + OneOf: []*jsonschema.Schema{ + variant([]string{"id", "value"}, map[string]*jsonschema.Schema{ + "id": { + Type: "integer", + Description: "The numeric project field ID.", + }, + }), + variant([]string{"name", "value"}, map[string]*jsonschema.Schema{ + "name": { + Type: "string", + Description: "The project field name. Matching is case-insensitive.", + }, + }), + }, + } +} + // ProjectsWrite returns the tool and handler for modifying GitHub Projects resources. func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool { tool := NewTool( ToolsetMetadataProjects, mcp.Tool{ Name: "projects_write", - Description: t("TOOL_PROJECTS_WRITE_DESCRIPTION", "Create and manage GitHub Projects: create projects, add/update/delete items, create status updates, and add iteration fields."), + Description: t("TOOL_PROJECTS_WRITE_DESCRIPTION", "Create and manage GitHub Projects: create projects, add/update/delete items, bulk-update many items at once, create status updates, and add iteration fields."), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_PROJECTS_WRITE_USER_TITLE", "Manage GitHub Projects"), ReadOnlyHint: false, @@ -512,6 +590,7 @@ func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool { Enum: []any{ projectsMethodAddProjectItem, projectsMethodUpdateProjectItem, + projectsMethodUpdateProjectItems, projectsMethodDeleteProjectItem, projectsMethodCreateProjectStatusUpdate, projectsMethodCreateProject, @@ -560,9 +639,11 @@ func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool { Type: "number", Description: "The pull request number (use when item_type is 'pull_request' for 'add_project_item' method). Provide either issue_number or pull_request_number.", }, - "updated_field": { - Type: "object", - Description: "Object describing the field to update and its new value. Required for 'update_project_item'. Two shapes are accepted: (1) by ID — {\"id\": 123456, \"value\": \"...\"}; (2) by name — {\"name\": \"Status\", \"value\": \"In Progress\"}. For single-select fields, option-name resolution requires the by-name shape; on the by-ID shape, pass the option ID. Set value to null to clear the field.", + "updated_field": projectUpdatedFieldSchema(), + "items": { + Type: "array", + Description: "The items to update with the top-level 'updated_field'. Required for 'update_project_items'; prefer it over calling 'update_project_item' in a loop. Each entry must match exactly one reference variant: 'node_id', numeric 'item_id', or 'item_owner' + 'item_repo' + 'issue_number'. Limit: " + strconv.Itoa(maxProjectItemsPerBatch) + " items per call.", + Items: updateProjectItemsItemSchema(), }, "body": { Type: "string", @@ -723,6 +804,8 @@ func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError("updated_field must be an object"), nil, nil } return updateProjectItem(ctx, client, gqlClient, owner, ownerType, projectNumber, itemID, fieldValue) + case projectsMethodUpdateProjectItems: + return updateProjectItemsBatch(ctx, client, gqlClient, owner, ownerType, projectNumber, args) case projectsMethodDeleteProjectItem: itemID, err := RequiredBigInt(args, "item_id") if err != nil { diff --git a/pkg/github/projects_batch_test.go b/pkg/github/projects_batch_test.go index 48e4db16aa..f2aef1734b 100644 --- a/pkg/github/projects_batch_test.go +++ b/pkg/github/projects_batch_test.go @@ -4,16 +4,109 @@ import ( "context" "encoding/json" "fmt" + "io" "math" "net/http" + "strings" + "sync/atomic" "testing" "github.com/github/github-mcp-server/internal/githubv4mock" + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/translations" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// fieldNode is a generic project field response node for use in mock data, +// covering data types beyond SINGLE_SELECT (statusFieldNode in +// projects_resolver_test.go is fixed to SINGLE_SELECT). See the comment on +// listAllProjectFields's inline-fragment decoding: the underlying jsonutil +// decoder populates id/databaseId/name/dataType identically across all three +// ProjectV2*Field fragments for a flat node object, so a single flat map +// (with "options" only where relevant) is sufficient regardless of dataType. +func fieldNode(nodeID string, databaseID int, name, dataType string) map[string]any { + return map[string]any{ + "id": nodeID, + "databaseId": databaseID, + "name": name, + "dataType": dataType, + } +} + +// projectIDMatcher returns the githubv4mock matcher for the org project-node-ID +// resolution query issued once per update_project_items call. +func projectIDMatcher(owner string, projectNumber int, projectNodeID string) githubv4mock.Matcher { + return githubv4mock.NewQueryMatcher( + struct { + Organization struct { + ProjectV2 struct { + ID githubv4.ID + } `graphql:"projectV2(number: $projectNumber)"` + } `graphql:"organization(login: $owner)"` + }{}, + map[string]any{ + "owner": githubv4.String(owner), + "projectNumber": githubv4.Int(int32(projectNumber)), //nolint:gosec + }, + githubv4mock.DataResponse(map[string]any{ + "organization": map[string]any{ + "projectV2": map[string]any{"id": projectNodeID}, + }, + }), + ) +} + +// mutationAwareTransport routes GraphQL requests to a fixed query-matcher +// transport (e.g. githubv4mock.NewMockedHTTPClient's Transport) for ordinary +// queries/lookups, and to a sequenced, call-counted responder for mutation +// requests, so end-to-end tests can assert on aliased-mutation call counts and +// per-call variables without needing to hand-construct the exact minified +// mutation query text that reflect.StructOf produces. +type mutationAwareTransport struct { + t *testing.T + queries http.RoundTripper + mutationRespond func(callIndex int, req capturedGraphQLRequest) (status int, body string) + queryCalls []capturedGraphQLRequest + mutationCalls []capturedGraphQLRequest +} + +func (m *mutationAwareTransport) RoundTrip(req *http.Request) (*http.Response, error) { + raw, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + _ = req.Body.Close() + + var parsed struct { + Query string `json:"query"` + Variables map[string]any `json:"variables"` + } + if err := json.Unmarshal(raw, &parsed); err != nil { + return nil, err + } + + if !strings.HasPrefix(strings.TrimSpace(parsed.Query), "mutation") { + m.queryCalls = append(m.queryCalls, capturedGraphQLRequest{Query: parsed.Query, Variables: parsed.Variables}) + req.Body = io.NopCloser(strings.NewReader(string(raw))) + return m.queries.RoundTrip(req) + } + + captured := capturedGraphQLRequest{Query: parsed.Query, Variables: parsed.Variables} + idx := len(m.mutationCalls) + m.mutationCalls = append(m.mutationCalls, captured) + if m.mutationRespond == nil { + m.t.Fatalf("unexpected mutation call #%d (query: %s)", idx, parsed.Query) + } + status, body := m.mutationRespond(idx, captured) + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(strings.NewReader(body)), + Header: make(http.Header), + }, nil +} + func Test_UpdateProjectItemsBatch_TopLevelGuards(t *testing.T) { tooMany := make([]any, maxProjectItemsPerBatch+1) validItem := map[string]any{"node_id": "PVTI_item1"} @@ -45,6 +138,692 @@ func Test_UpdateProjectItemsBatch_TopLevelGuards(t *testing.T) { } } +func Test_UpdateProjectItemsBatch_InvalidSharedValueIsTopLevelError(t *testing.T) { + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + statusFieldNode("PVTSSF_status", 101, "Status", []map[string]any{ + {"id": "OPT_todo", "name": "Todo"}, + }), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, _ capturedGraphQLRequest) (int, string) { + t.Fatal("invalid shared values must fail before writes") + return http.StatusInternalServerError, "" + }, + } + + result, structured, err := updateProjectItemsBatch( + t.Context(), + nil, + newTestGQLClient(transport), + "octo-org", + "org", + 1, + map[string]any{ + "updated_field": map[string]any{"name": "Status", "value": "Missing"}, + "items": []any{map[string]any{"node_id": "PVTI_item1"}}, + }, + ) + require.NoError(t, err) + assert.Nil(t, structured) + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getErrorResult(t, result).Text), &response)) + assert.Equal(t, "option_not_found", response["error"]) + assert.Equal(t, "Missing", response["name"]) + assert.Equal(t, []any{map[string]any{"name": "Todo"}}, response["candidates"]) + assert.Empty(t, transport.mutationCalls) +} + +func Test_ProjectsWrite_UpdateProjectItems_NodeIDBypassesRESTLookup(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, req capturedGraphQLRequest) (int, string) { + assert.Contains(t, req.Query, "updateProjectV2ItemFieldValue") + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item1", FullDatabaseID: "1001"}, + }) + }, + } + gqlClient := newTestGQLClient(transport) + + // No REST handlers registered at all: if the implementation ever fell back + // to a REST lookup for a node_id-addressed item, this would 404. + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "hello"}, + "items": []any{ + map[string]any{"node_id": "PVTI_item1"}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(1), response["succeeded"]) + assert.Equal(t, float64(0), response["failed"]) + assert.Equal(t, float64(0), response["unknown"]) +} + +func Test_ProjectsWrite_UpdateProjectItems_NumericItemIDDeduplicatesRESTLookup(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, req capturedGraphQLRequest) (int, string) { + require.Len(t, req.Variables, 1) + assert.Equal(t, "PVTF_notes", req.Variables["input"].(map[string]any)["fieldId"]) + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item1001", FullDatabaseID: "1001"}, + }) + }, + } + gqlClient := newTestGQLClient(transport) + + var restCalls int32 + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetOrgsProjectsV2ItemsByProjectByItemID: func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&restCalls, 1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"id":1001,"node_id":"PVTI_item1001"}`)) + }, + })) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "hello"}, + "items": []any{ + map[string]any{"item_id": float64(1001)}, + map[string]any{"item_id": float64(1001)}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(1), response["succeeded"]) + assert.Equal(t, float64(1), response["failed"]) + assert.Equal(t, int32(1), atomic.LoadInt32(&restCalls), "the same numeric item_id must only be resolved once") + results := response["results"].([]any) + assert.Equal(t, "duplicate_target", results[1].(map[string]any)["error"].(map[string]any)["code"]) +} + +func Test_ProjectsWrite_UpdateProjectItems_IssueRefPaginationIsDeduplicated(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + resolveItemByIssueQuery{}, + map[string]any{ + "issueOwner": githubv4.String("github"), + "issueRepo": githubv4.String("planning-tracking"), + "issueNumber": githubv4.Int(123), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "projectItems": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": "PVTI_other", + "fullDatabaseId": "9999", + "project": map[string]any{"id": "PVT_other"}, + }, + }, + "pageInfo": map[string]any{ + "hasNextPage": true, "hasPreviousPage": false, + "startCursor": "page-one", "endCursor": "page-one", + }, + }, + }, + }, + }), + ), + githubv4mock.NewQueryMatcher( + resolveItemByIssuePageQuery{}, + map[string]any{ + "issueOwner": githubv4.String("github"), + "issueRepo": githubv4.String("planning-tracking"), + "issueNumber": githubv4.Int(123), + "after": githubv4.String("page-one"), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "projectItems": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": "PVTI_item2002", + "fullDatabaseId": "2002", + "project": map[string]any{"id": "PVT_project1"}, + }, + }, + "pageInfo": map[string]any{ + "hasNextPage": false, "hasPreviousPage": true, + "startCursor": "page-two", "endCursor": "page-two", + }, + }, + }, + }, + }), + ), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, req capturedGraphQLRequest) (int, string) { + require.Len(t, req.Variables, 1) + assert.Equal(t, "PVTI_item2002", req.Variables["input"].(map[string]any)["itemId"]) + assert.Equal(t, "PVTF_notes", req.Variables["input"].(map[string]any)["fieldId"]) + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item2002", FullDatabaseID: "2002"}, + }) + }, + } + gqlClient := newTestGQLClient(transport) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "hello"}, + "items": []any{ + map[string]any{ + "item_owner": "github", "item_repo": "planning-tracking", "issue_number": float64(123), + }, + map[string]any{ + "item_owner": "github", "item_repo": "planning-tracking", "issue_number": float64(123), + }, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(1), response["succeeded"]) + assert.Equal(t, float64(1), response["failed"]) + results := response["results"].([]any) + item := results[0].(map[string]any)["item"].(map[string]any) + assert.Equal(t, "PVTI_item2002", item["node_id"]) + assert.Equal(t, "2002", item["full_database_id"]) + assert.Equal(t, "duplicate_target", results[1].(map[string]any)["error"].(map[string]any)["code"]) + issueResolutionCalls := 0 + for _, call := range transport.queryCalls { + if strings.Contains(call.Query, "projectItems") { + issueResolutionCalls++ + } + } + assert.Equal(t, 2, issueResolutionCalls, "duplicate issue refs should share one two-page resolution chain") + assert.Len(t, transport.queryCalls, 4, "expected project, fields, and two issue-page queries") +} + +func Test_ProjectsWrite_UpdateProjectItems_DuplicateTargetRejected(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, req capturedGraphQLRequest) (int, string) { + require.Len(t, req.Variables, 1) + assert.Equal(t, 1, strings.Count(req.Query, "updateProjectV2ItemFieldValue")) + assert.Equal(t, "PVTI_item1", req.Variables["input"].(map[string]any)["itemId"]) + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item1", FullDatabaseID: "1001"}, + }) + }, + } + gqlClient := newTestGQLClient(transport) + var restCalls int32 + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetOrgsProjectsV2ItemsByProjectByItemID: func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&restCalls, 1) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":1001,"node_id":"PVTI_item1"}`)) + }, + })) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "hello"}, + "items": []any{ + map[string]any{"node_id": "PVTI_item1"}, + map[string]any{"item_id": float64(1001)}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(1), response["succeeded"]) + assert.Equal(t, float64(1), response["failed"]) + + results := response["results"].([]any) + second := results[1].(map[string]any) + assert.Equal(t, "failed", second["status"]) + assert.Equal(t, "duplicate_target", second["error"].(map[string]any)["code"]) + assert.Equal(t, int32(1), atomic.LoadInt32(&restCalls)) + assert.Len(t, transport.mutationCalls, 1) +} + +func Test_ProjectsWrite_UpdateProjectItems_TwentyWritesIsOneMutationRequest(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + transport := chunkSizeTestRun(t, toolDef, 20) + assert.Len(t, transport.mutationCalls, 1) +} + +func Test_ProjectsWrite_UpdateProjectItems_TwentyOneWritesIsTwoMutationRequests(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + transport := chunkSizeTestRun(t, toolDef, 21) + assert.Len(t, transport.mutationCalls, 2) +} + +func Test_ProjectsWrite_UpdateProjectItems_MaximumWritesIsThreeMutationRequests(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + transport := chunkSizeTestRun(t, toolDef, maxProjectItemsPerBatch) + assert.Len(t, transport.mutationCalls, 3) +} + +// chunkSizeTestRun runs an update_project_items call with itemCount node_id +// items (all TEXT field updates), returning the mutationAwareTransport so the +// caller can assert on how many aliased-mutation HTTP requests were made. +func chunkSizeTestRun(t *testing.T, toolDef inventory.ServerTool, itemCount int) *mutationAwareTransport { + t.Helper() + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, req capturedGraphQLRequest) (int, string) { + // input (index 0) plus inputN for each additional alias in this chunk. + chunkSize := len(req.Variables) + ids := make(map[int]struct{ NodeID, FullDatabaseID string }, chunkSize) + for i := range chunkSize { + ids[i] = struct{ NodeID, FullDatabaseID string }{ + NodeID: "PVTI_chunk", + FullDatabaseID: "1", + } + } + return http.StatusOK, mutationDataResponse(t, ids) + }, + } + gqlClient := newTestGQLClient(transport) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + items := make([]any, itemCount) + for i := range itemCount { + items[i] = map[string]any{"node_id": fmt.Sprintf("PVTI_item%d", i)} + } + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "hello"}, + "items": items, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(itemCount), response["succeeded"]) + + return transport +} + +func Test_ProjectsWrite_UpdateProjectItems_SharedNullClearsAllItemsInOrder(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, req capturedGraphQLRequest) (int, string) { + assert.Contains(t, req.Query, "clearProjectV2ItemFieldValue") + assert.NotContains(t, req.Query, "updateProjectV2ItemFieldValue") + for _, input := range req.Variables { + assert.NotContains(t, input.(map[string]any), "value") + } + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item0", FullDatabaseID: "1000"}, + 1: {NodeID: "PVTI_item1", FullDatabaseID: "1001"}, + 2: {NodeID: "PVTI_item2", FullDatabaseID: "1002"}, + }) + }, + } + gqlClient := newTestGQLClient(transport) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": nil}, + "items": []any{ + map[string]any{"node_id": "PVTI_item0"}, + map[string]any{"node_id": "PVTI_item1"}, + map[string]any{"node_id": "PVTI_item2"}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(3), response["succeeded"]) + + results := response["results"].([]any) + require.Len(t, results, 3) + for i, r := range results { + entry := r.(map[string]any) + assert.Equal(t, float64(i), entry["index"]) + assert.Equal(t, "succeeded", entry["status"]) + assert.Equal(t, fmt.Sprintf("%d", 1000+i), entry["item"].(map[string]any)["full_database_id"]) + } + assert.Len(t, transport.mutationCalls, 1) +} + +func Test_ProjectsWrite_UpdateProjectItems_TransportFailureAbortsLaterChunks(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(callIndex int, _ capturedGraphQLRequest) (int, string) { + if callIndex == 0 { + // Systemic transport-level failure: no data at all. + return http.StatusInternalServerError, `{"message":"internal server error"}` + } + t.Fatalf("chunk #%d must not execute after an ambiguous chunk-level failure", callIndex) + return http.StatusInternalServerError, "" + }, + } + gqlClient := newTestGQLClient(transport) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + items := make([]any, 25) + for i := range 25 { + items[i] = map[string]any{"node_id": fmt.Sprintf("PVTI_item%d", i)} + } + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "x"}, + "items": items, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + // No item succeeded (all unknown after the abort), so IsError is set per + // the "no item succeeded" rule, even though nothing was deterministically + // rejected; the structured result (with unknown statuses) is still available. + assert.True(t, result.IsError) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(0), response["succeeded"]) + assert.Equal(t, float64(25), response["unknown"]) + assert.Len(t, transport.mutationCalls, 1, "only the first (failing) chunk should have been sent") + + results := response["results"].([]any) + for _, r := range results { + assert.Equal(t, "unknown", r.(map[string]any)["status"]) + } +} + +func Test_ProjectsWrite_UpdateProjectItems_AllFailedSetsIsError(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + mocked := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + ) + countingTransport := &requestCountingTransport{inner: mocked.Transport} + gqlClient := newTestGQLClient(countingTransport) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "x"}, + "items": []any{ + map[string]any{}, + map[string]any{"node_id": ""}, + map[string]any{"item_id": float64(0)}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.True(t, result.IsError, "IsError must be set when no item in the batch succeeds") + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(0), response["succeeded"]) + assert.Equal(t, float64(3), response["failed"]) + assert.Zero(t, countingTransport.count, "an all-invalid batch should not perform GraphQL resolution") +} + +func Test_ProjectsWrite_UpdateProjectItems_MixedOutcomeKeepsIsErrorFalse(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, _ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item0", FullDatabaseID: "1000"}, + }) + }, + } + gqlClient := newTestGQLClient(transport) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "x"}, + "items": []any{ + map[string]any{"node_id": "PVTI_item0"}, + map[string]any{}, // deterministic failure + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError, "mixed outcomes must keep IsError false so the structured result stays available") + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(1), response["succeeded"]) + assert.Equal(t, float64(1), response["failed"]) +} + +// Test_ProjectsWrite_UpdateProjectItems_EnterpriseClientWiring verifies the +// batch mutation path works unchanged when gqlClient was constructed via +// githubv4.NewEnterpriseClient (GHES), not just githubv4.NewClient: the +// reflection-based mutation logic never assumes a specific endpoint and only +// ever uses the injected client. +func Test_ProjectsWrite_UpdateProjectItems_EnterpriseClientWiring(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, _ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item0", FullDatabaseID: "1000"}, + }) + }, + } + gqlClient := githubv4.NewEnterpriseClient("https://ghe.example.com/graphql", &http.Client{Transport: transport}) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "x"}, + "items": []any{ + map[string]any{"node_id": "PVTI_item0"}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(1), response["succeeded"]) +} + func Test_ParseItemRef_ExactlyOneFormRequired(t *testing.T) { tests := []struct { name string diff --git a/pkg/github/projects_test.go b/pkg/github/projects_test.go index 553c2421a4..92de4a5d5f 100644 --- a/pkg/github/projects_test.go +++ b/pkg/github/projects_test.go @@ -866,7 +866,7 @@ func Test_ProjectsWrite(t *testing.T) { require.NoError(t, toolsnaps.Test(toolDef.Tool.Name, toolDef.Tool)) assert.Equal(t, "projects_write", toolDef.Tool.Name) - assert.NotEmpty(t, toolDef.Tool.Description) + assert.Contains(t, toolDef.Tool.Description, "bulk-update many items at once") inputSchema := toolDef.Tool.InputSchema.(*jsonschema.Schema) assert.Contains(t, inputSchema.Properties, "method") assert.Contains(t, inputSchema.Properties, "owner") @@ -879,6 +879,7 @@ func Test_ProjectsWrite(t *testing.T) { assert.Contains(t, inputSchema.Properties, "issue_number") assert.Contains(t, inputSchema.Properties, "pull_request_number") assert.Contains(t, inputSchema.Properties, "updated_field") + assert.Contains(t, inputSchema.Properties, "items") assert.ElementsMatch(t, inputSchema.Required, []string{"method", "owner"}) // Verify DestructiveHint is set @@ -887,6 +888,64 @@ func Test_ProjectsWrite(t *testing.T) { assert.True(t, *toolDef.Tool.Annotations.DestructiveHint) } +func Test_ProjectsWrite_UpdateProjectItemsSchema(t *testing.T) { + inputSchema := ProjectsWrite(translations.NullTranslationHelper).Tool.InputSchema.(*jsonschema.Schema) + assert.Contains(t, inputSchema.Properties["items"].Description, "prefer it over calling 'update_project_item' in a loop") + itemSchema := inputSchema.Properties["items"].Items + + assert.Equal(t, "object", itemSchema.Type) + assert.Empty(t, itemSchema.Properties, "item references should be modeled by oneOf, not flattened properties") + require.Len(t, itemSchema.OneOf, 3) + + expectedRequired := [][]string{ + {"node_id"}, + {"item_id"}, + {"item_owner", "item_repo", "issue_number"}, + } + expectedProperties := [][]string{ + {"node_id"}, + {"item_id"}, + {"item_owner", "item_repo", "issue_number"}, + } + for i, variant := range itemSchema.OneOf { + properties := make([]string, 0, len(variant.Properties)) + for name := range variant.Properties { + properties = append(properties, name) + } + assert.Equal(t, "object", variant.Type) + assert.ElementsMatch(t, expectedRequired[i], variant.Required) + assert.ElementsMatch(t, expectedProperties[i], properties) + for _, property := range variant.Properties { + assert.NotEmpty(t, property.Type) + assert.NotEmpty(t, property.Description) + } + require.NotNil(t, variant.AdditionalProperties) + assert.NotNil(t, variant.AdditionalProperties.Not, "variant must reject additional properties") + } + + fieldSchema := inputSchema.Properties["updated_field"] + assert.Equal(t, "object", fieldSchema.Type) + assert.Contains(t, fieldSchema.Description, "one top-level field/value applies to every item") + require.Len(t, fieldSchema.OneOf, 2) + for i, variant := range fieldSchema.OneOf { + reference := "id" + if i == 1 { + reference = "name" + } + properties := make([]string, 0, len(variant.Properties)) + for name := range variant.Properties { + properties = append(properties, name) + } + assert.ElementsMatch(t, []string{reference, "value"}, variant.Required) + assert.ElementsMatch(t, []string{reference, "value"}, properties) + require.NotNil(t, variant.AdditionalProperties) + assert.NotNil(t, variant.AdditionalProperties.Not) + assert.Empty(t, variant.Properties["value"].Type, "an unconstrained value schema accepts any JSON value, including null") + assert.Empty(t, variant.Properties["value"].Types) + assert.NotEmpty(t, variant.Properties["value"].Description) + } +} + func Test_ProjectsWrite_AddProjectItem(t *testing.T) { toolDef := ProjectsWrite(translations.NullTranslationHelper) From 868102927487f23cb3f13c5ae57f8c0781d18618 Mon Sep 17 00:00:00 2001 From: Bryan Zwicker Date: Wed, 22 Jul 2026 14:11:05 -0400 Subject: [PATCH 3/7] Classify batch resolution failures Use a neutral code for non-structured lookup failures while preserving structured resolution details. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ae767ff-c1d0-46a9-b126-2e91403993a0 --- pkg/github/projects_batch.go | 2 +- pkg/github/projects_batch_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/pkg/github/projects_batch.go b/pkg/github/projects_batch.go index 3c77d250c7..216c09af9a 100644 --- a/pkg/github/projects_batch.go +++ b/pkg/github/projects_batch.go @@ -765,7 +765,7 @@ func batchErrorFromResolution(err error) *batchItemError { } } return &batchItemError{ - Code: "invalid_argument", + Code: "resolution_failed", Message: err.Error(), } } diff --git a/pkg/github/projects_batch_test.go b/pkg/github/projects_batch_test.go index f2aef1734b..344fd74928 100644 --- a/pkg/github/projects_batch_test.go +++ b/pkg/github/projects_batch_test.go @@ -12,6 +12,7 @@ import ( "testing" "github.com/github/github-mcp-server/internal/githubv4mock" + ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/translations" "github.com/shurcooL/githubv4" @@ -1133,6 +1134,32 @@ func Test_ResolveItemNodeIDsByNumericID_DeduplicatesOrgAndUserLookups(t *testing } } +func Test_BatchErrorFromResolution(t *testing.T) { + t.Run("generic wrapped error", func(t *testing.T) { + err := batchErrorFromResolution(fmt.Errorf("item lookup failed: %w", context.DeadlineExceeded)) + + assert.Equal(t, "resolution_failed", err.Code) + assert.Equal(t, "item lookup failed: context deadline exceeded", err.Message) + }) + + t.Run("structured error", func(t *testing.T) { + candidates := []any{map[string]any{"id": "PVTI_1"}} + structured := ghErrors.NewStructuredResolutionError( + "item_not_found", + "octo/repo#42", + "Check that the item belongs to the project.", + candidates, + ) + + err := batchErrorFromResolution(fmt.Errorf("resolve item: %w", structured)) + + assert.Equal(t, structured.Kind, err.Code) + assert.Equal(t, "item_not_found: octo/repo#42", err.Message) + assert.Equal(t, structured.Hint, err.Hint) + assert.Equal(t, candidates, err.Candidates) + }) +} + func Test_ExecuteBatchWrites_AllAliasGraphQLErrorContinues(t *testing.T) { transport := &sequencedGraphQLTransport{ t: t, From a387c39a771db455638e006067ee0c115abb0157 Mon Sep 17 00:00:00 2001 From: Bryan Zwicker Date: Thu, 23 Jul 2026 10:54:10 -0400 Subject: [PATCH 4/7] Resolve issue references concurrently Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ae767ff-c1d0-46a9-b126-2e91403993a0 Copilot-Session: 5709a470-df75-43ec-9a9c-98868e6065d2 --- pkg/github/projects_batch.go | 49 ++++++- pkg/github/projects_batch_test.go | 225 ++++++++++++++++++++++++++++++ 2 files changed, 270 insertions(+), 4 deletions(-) diff --git a/pkg/github/projects_batch.go b/pkg/github/projects_batch.go index 216c09af9a..28493a4bf2 100644 --- a/pkg/github/projects_batch.go +++ b/pkg/github/projects_batch.go @@ -739,18 +739,59 @@ type issueRefKey struct { } func resolveIssueRefs(ctx context.Context, gqlClient *githubv4.Client, projectID githubv4.ID, items []parsedBatchItem) map[issueRefKey]itemLookupResult { - out := make(map[issueRefKey]itemLookupResult) + seen := make(map[issueRefKey]struct{}, len(items)) + var unique []issueRefKey for _, it := range items { if it.err != nil || it.refKind != batchRefIssue { continue } key := issueRefKey{owner: it.issueOwner, repo: it.issueRepo, number: it.issueNumber} - if _, done := out[key]; done { + if _, dup := seen[key]; dup { continue } - nodeID, itemID, err := resolveProjectItemByIssueNumberWithProjectID(ctx, gqlClient, projectID, it.issueOwner, it.issueRepo, it.issueNumber) - out[key] = itemLookupResult{nodeID: nodeID, fullDatabaseID: itemID, err: err} + seen[key] = struct{}{} + unique = append(unique, key) } + + out := make(map[issueRefKey]itemLookupResult, len(unique)) + if len(unique) == 0 { + return out + } + + var mu sync.Mutex + var wg sync.WaitGroup + sem := make(chan struct{}, batchItemLookupConcurrency) + + for _, key := range unique { + wg.Add(1) + go func(key issueRefKey) { + defer wg.Done() + + select { + case sem <- struct{}{}: + case <-ctx.Done(): + mu.Lock() + out[key] = itemLookupResult{err: ctx.Err()} + mu.Unlock() + return + } + defer func() { <-sem }() + + if ctx.Err() != nil { + mu.Lock() + out[key] = itemLookupResult{err: ctx.Err()} + mu.Unlock() + return + } + + nodeID, itemID, err := resolveProjectItemByIssueNumberWithProjectID(ctx, gqlClient, projectID, key.owner, key.repo, key.number) + + mu.Lock() + out[key] = itemLookupResult{nodeID: nodeID, fullDatabaseID: itemID, err: err} + mu.Unlock() + }(key) + } + wg.Wait() return out } diff --git a/pkg/github/projects_batch_test.go b/pkg/github/projects_batch_test.go index 344fd74928..985cd5bfc7 100644 --- a/pkg/github/projects_batch_test.go +++ b/pkg/github/projects_batch_test.go @@ -5,11 +5,14 @@ import ( "encoding/json" "fmt" "io" + "maps" "math" "net/http" "strings" + "sync" "sync/atomic" "testing" + "time" "github.com/github/github-mcp-server/internal/githubv4mock" ghErrors "github.com/github/github-mcp-server/pkg/errors" @@ -108,6 +111,137 @@ func (m *mutationAwareTransport) RoundTrip(req *http.Request) (*http.Response, e }, nil } +type gatedIssueLookupTransport struct { + gate <-chan struct{} + started chan int + projectID string + + mu sync.Mutex + active int + peak int + calls map[int]int +} + +func newGatedIssueLookupTransport(gate <-chan struct{}, projectID string) *gatedIssueLookupTransport { + return &gatedIssueLookupTransport{ + gate: gate, + started: make(chan int, maxProjectItemsPerBatch), + projectID: projectID, + calls: make(map[int]int), + } +} + +func (t *gatedIssueLookupTransport) RoundTrip(req *http.Request) (*http.Response, error) { + raw, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + _ = req.Body.Close() + + var parsed struct { + Variables map[string]any `json:"variables"` + } + if err := json.Unmarshal(raw, &parsed); err != nil { + return nil, err + } + rawIssueNumber, ok := parsed.Variables["issueNumber"].(float64) + if !ok { + return nil, fmt.Errorf("issueNumber variable is missing or invalid") + } + issueNumber := int(rawIssueNumber) + + t.mu.Lock() + t.calls[issueNumber]++ + t.active++ + t.peak = max(t.peak, t.active) + t.mu.Unlock() + defer func() { + t.mu.Lock() + t.active-- + t.mu.Unlock() + }() + + t.started <- issueNumber + select { + case <-t.gate: + case <-req.Context().Done(): + return nil, req.Context().Err() + } + + body, err := json.Marshal(map[string]any{ + "data": map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "projectItems": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": fmt.Sprintf("PVTI_item%d", issueNumber), + "fullDatabaseId": fmt.Sprintf("%d", 1000+issueNumber), + "project": map[string]any{"id": t.projectID}, + }, + }, + "pageInfo": map[string]any{ + "hasNextPage": false, "hasPreviousPage": false, + "startCursor": "page-one", "endCursor": "page-one", + }, + }, + }, + }, + }, + }) + if err != nil { + return nil, err + } + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(string(body))), + Header: http.Header{"Content-Type": []string{"application/json"}}, + }, nil +} + +func (t *gatedIssueLookupTransport) snapshot() (active int, peak int, calls map[int]int) { + t.mu.Lock() + defer t.mu.Unlock() + + return t.active, t.peak, maps.Clone(t.calls) +} + +func issueBatchItems(issueNumbers ...int) []parsedBatchItem { + items := make([]parsedBatchItem, 0, len(issueNumbers)) + for index, issueNumber := range issueNumbers { + items = append(items, parsedBatchItem{ + index: index, + refKind: batchRefIssue, + issueOwner: "octo-org", + issueRepo: "roadmap", + issueNumber: issueNumber, + }) + } + return items +} + +func waitForIssueLookups(ctx context.Context, t *testing.T, started <-chan int, count int) { + t.Helper() + for range count { + select { + case <-started: + case <-ctx.Done(): + t.Fatalf("timed out waiting for %d issue lookups to start: %v", count, ctx.Err()) + } + } +} + +func waitForIssueLookupResults(ctx context.Context, t *testing.T, results <-chan map[issueRefKey]itemLookupResult) map[issueRefKey]itemLookupResult { + t.Helper() + select { + case resolved := <-results: + return resolved + case <-ctx.Done(): + t.Fatalf("timed out waiting for issue lookups to finish: %v", ctx.Err()) + return nil + } +} + func Test_UpdateProjectItemsBatch_TopLevelGuards(t *testing.T) { tooMany := make([]any, maxProjectItemsPerBatch+1) validItem := map[string]any{"node_id": "PVTI_item1"} @@ -1134,6 +1268,97 @@ func Test_ResolveItemNodeIDsByNumericID_DeduplicatesOrgAndUserLookups(t *testing } } +func Test_ResolveIssueRefs_DeduplicatesAndBoundsConcurrency(t *testing.T) { + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + + gate := make(chan struct{}) + transport := newGatedIssueLookupTransport(gate, "PVT_project") + results := make(chan map[issueRefKey]itemLookupResult, 1) + go func() { + results <- resolveIssueRefs( + ctx, + newTestGQLClient(transport), + githubv4.ID("PVT_project"), + issueBatchItems(1, 2, 3, 4, 5, 6, 1), + ) + }() + + waitForIssueLookups(ctx, t, transport.started, batchItemLookupConcurrency) + active, peak, calls := transport.snapshot() + assert.Equal(t, batchItemLookupConcurrency, active) + assert.Equal(t, batchItemLookupConcurrency, peak) + assert.Len(t, calls, batchItemLookupConcurrency) + + close(gate) + resolved := waitForIssueLookupResults(ctx, t, results) + + require.Len(t, resolved, 6) + for issueNumber := 1; issueNumber <= 6; issueNumber++ { + key := issueRefKey{owner: "octo-org", repo: "roadmap", number: issueNumber} + result, ok := resolved[key] + require.True(t, ok) + require.NoError(t, result.err) + assert.Equal(t, fmt.Sprintf("PVTI_item%d", issueNumber), result.nodeID) + assert.Equal(t, int64(1000+issueNumber), result.fullDatabaseID) + } + + active, peak, calls = transport.snapshot() + assert.Zero(t, active) + assert.Equal(t, batchItemLookupConcurrency, peak) + require.Len(t, calls, 6) + for issueNumber := 1; issueNumber <= 6; issueNumber++ { + assert.Equal(t, 1, calls[issueNumber]) + } +} + +func Test_ResolveIssueRefs_CancellationPopulatesWaitingRefs(t *testing.T) { + testCtx, stop := context.WithTimeout(t.Context(), 5*time.Second) + defer stop() + ctx, cancel := context.WithCancel(testCtx) + defer cancel() + + gate := make(chan struct{}) + defer close(gate) + transport := newGatedIssueLookupTransport(gate, "PVT_project") + results := make(chan map[issueRefKey]itemLookupResult, 1) + go func() { + results <- resolveIssueRefs( + ctx, + newTestGQLClient(transport), + githubv4.ID("PVT_project"), + issueBatchItems(1, 2, 3, 4, 5, 6, 7), + ) + }() + + waitForIssueLookups(testCtx, t, transport.started, batchItemLookupConcurrency) + _, peak, startedCalls := transport.snapshot() + require.Equal(t, batchItemLookupConcurrency, peak) + require.Len(t, startedCalls, batchItemLookupConcurrency) + + cancel() + resolved := waitForIssueLookupResults(testCtx, t, results) + + require.Len(t, resolved, 7) + waiting := 0 + for issueNumber := 1; issueNumber <= 7; issueNumber++ { + key := issueRefKey{owner: "octo-org", repo: "roadmap", number: issueNumber} + result, ok := resolved[key] + require.True(t, ok) + require.ErrorIs(t, result.err, context.Canceled) + if _, started := startedCalls[issueNumber]; !started { + waiting++ + assert.Equal(t, context.Canceled, result.err) + } + } + assert.Equal(t, 2, waiting) + + active, peak, calls := transport.snapshot() + assert.Zero(t, active) + assert.Equal(t, batchItemLookupConcurrency, peak) + assert.Equal(t, startedCalls, calls) +} + func Test_BatchErrorFromResolution(t *testing.T) { t.Run("generic wrapped error", func(t *testing.T) { err := batchErrorFromResolution(fmt.Errorf("item lookup failed: %w", context.DeadlineExceeded)) From 282ac3ff9a2a28006148eb4ad2a5adeef1748a6e Mon Sep 17 00:00:00 2001 From: Bryan Zwicker Date: Mon, 27 Jul 2026 11:31:47 -0400 Subject: [PATCH 5/7] Add singular Issue Field project updates Support name-based attached Issue Field updates for singular Project items while preserving existing read and standard field behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4c94f3ce-c04a-482f-830b-ab85abc3f6e4 --- pkg/github/granular_tools_test.go | 195 +++++------------------ pkg/github/issues_granular.go | 54 +++---- pkg/github/projects.go | 225 +++++++++++++++++++++++---- pkg/github/projects_resolver.go | 158 +++++++++++++++++++ pkg/github/projects_resolver_test.go | 122 +++++++++++++++ pkg/github/projects_test.go | 163 +++++++++++++++++++ 6 files changed, 700 insertions(+), 217 deletions(-) diff --git a/pkg/github/granular_tools_test.go b/pkg/github/granular_tools_test.go index 58fd904e88..11dc0a055b 100644 --- a/pkg/github/granular_tools_test.go +++ b/pkg/github/granular_tools_test.go @@ -1746,6 +1746,27 @@ func TestGranularUnresolveReviewThread(t *testing.T) { } func TestGranularSetIssueFields(t *testing.T) { + t.Run("mutation selects only issue identity", func(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(req capturedGraphQLRequest) (int, string) { + assert.Contains(t, req.Query, "issue{id,url}") + assert.NotContains(t, req.Query, "issueFieldValues") + assert.NotContains(t, req.Query, "number") + return http.StatusOK, `{"data":{"setIssueFieldValue":{"issue":{"id":"ISSUE_123","url":"https://github.com/owner/repo/issues/5"}}}}` + }, + }, + } + _, err := SetIssueFieldValues(context.Background(), githubv4.NewClient(&http.Client{Transport: transport}), SetIssueFieldValueInput{ + IssueID: githubv4.ID("ISSUE_123"), + IssueFields: []IssueFieldCreateOrUpdateInput{{ + FieldID: githubv4.ID("FIELD_1"), TextValue: githubv4.NewString("hello"), + }}, + }) + require.NoError(t, err) + }) + t.Run("successful set with text value", func(t *testing.T) { matchers := []githubv4mock.Matcher{ // Mock the issue ID query @@ -1770,29 +1791,7 @@ func TestGranularSetIssueFields(t *testing.T) { ), // Mock the setIssueFieldValue mutation githubv4mock.NewMutationMatcher( - struct { - SetIssueFieldValue struct { - Issue struct { - ID githubv4.ID - Number githubv4.Int - URL githubv4.String - } - IssueFieldValues []struct { - TextValue struct { - Value string - } `graphql:"... on IssueFieldTextValue"` - SingleSelectValue struct { - Name string - } `graphql:"... on IssueFieldSingleSelectValue"` - DateValue struct { - Value string - } `graphql:"... on IssueFieldDateValue"` - NumberValue struct { - Value float64 - } `graphql:"... on IssueFieldNumberValue"` - } - } `graphql:"setIssueFieldValue(input: $input)"` - }{}, + setIssueFieldValueMutation{}, SetIssueFieldValueInput{ IssueID: githubv4.ID("ISSUE_123"), IssueFields: []IssueFieldCreateOrUpdateInput{ @@ -1806,9 +1805,8 @@ func TestGranularSetIssueFields(t *testing.T) { githubv4mock.DataResponse(map[string]any{ "setIssueFieldValue": map[string]any{ "issue": map[string]any{ - "id": "ISSUE_123", - "number": 5, - "url": "https://github.com/owner/repo/issues/5", + "id": "ISSUE_123", + "url": "https://github.com/owner/repo/issues/5", }, }, }), @@ -1945,29 +1943,7 @@ func TestGranularSetIssueFields(t *testing.T) { }), ), githubv4mock.NewMutationMatcher( - struct { - SetIssueFieldValue struct { - Issue struct { - ID githubv4.ID - Number githubv4.Int - URL githubv4.String - } - IssueFieldValues []struct { - TextValue struct { - Value string - } `graphql:"... on IssueFieldTextValue"` - SingleSelectValue struct { - Name string - } `graphql:"... on IssueFieldSingleSelectValue"` - DateValue struct { - Value string - } `graphql:"... on IssueFieldDateValue"` - NumberValue struct { - Value float64 - } `graphql:"... on IssueFieldNumberValue"` - } - } `graphql:"setIssueFieldValue(input: $input)"` - }{}, + setIssueFieldValueMutation{}, SetIssueFieldValueInput{ IssueID: githubv4.ID("ISSUE_123"), IssueFields: []IssueFieldCreateOrUpdateInput{ @@ -1982,9 +1958,8 @@ func TestGranularSetIssueFields(t *testing.T) { githubv4mock.DataResponse(map[string]any{ "setIssueFieldValue": map[string]any{ "issue": map[string]any{ - "id": "ISSUE_123", - "number": 5, - "url": "https://github.com/owner/repo/issues/5", + "id": "ISSUE_123", + "url": "https://github.com/owner/repo/issues/5", }, }, }), @@ -2059,29 +2034,7 @@ func TestGranularSetIssueFields(t *testing.T) { }), ), githubv4mock.NewMutationMatcher( - struct { - SetIssueFieldValue struct { - Issue struct { - ID githubv4.ID - Number githubv4.Int - URL githubv4.String - } - IssueFieldValues []struct { - TextValue struct { - Value string - } `graphql:"... on IssueFieldTextValue"` - SingleSelectValue struct { - Name string - } `graphql:"... on IssueFieldSingleSelectValue"` - DateValue struct { - Value string - } `graphql:"... on IssueFieldDateValue"` - NumberValue struct { - Value float64 - } `graphql:"... on IssueFieldNumberValue"` - } - } `graphql:"setIssueFieldValue(input: $input)"` - }{}, + setIssueFieldValueMutation{}, SetIssueFieldValueInput{ IssueID: githubv4.ID("ISSUE_123"), IssueFields: []IssueFieldCreateOrUpdateInput{ @@ -2096,9 +2049,8 @@ func TestGranularSetIssueFields(t *testing.T) { githubv4mock.DataResponse(map[string]any{ "setIssueFieldValue": map[string]any{ "issue": map[string]any{ - "id": "ISSUE_123", - "number": 5, - "url": "https://github.com/owner/repo/issues/5", + "id": "ISSUE_123", + "url": "https://github.com/owner/repo/issues/5", }, }, }), @@ -2173,29 +2125,7 @@ func TestGranularSetIssueFields(t *testing.T) { }), ), githubv4mock.NewMutationMatcher( - struct { - SetIssueFieldValue struct { - Issue struct { - ID githubv4.ID - Number githubv4.Int - URL githubv4.String - } - IssueFieldValues []struct { - TextValue struct { - Value string - } `graphql:"... on IssueFieldTextValue"` - SingleSelectValue struct { - Name string - } `graphql:"... on IssueFieldSingleSelectValue"` - DateValue struct { - Value string - } `graphql:"... on IssueFieldDateValue"` - NumberValue struct { - Value float64 - } `graphql:"... on IssueFieldNumberValue"` - } - } `graphql:"setIssueFieldValue(input: $input)"` - }{}, + setIssueFieldValueMutation{}, SetIssueFieldValueInput{ IssueID: githubv4.ID("ISSUE_123"), IssueFields: []IssueFieldCreateOrUpdateInput{ @@ -2210,9 +2140,8 @@ func TestGranularSetIssueFields(t *testing.T) { githubv4mock.DataResponse(map[string]any{ "setIssueFieldValue": map[string]any{ "issue": map[string]any{ - "id": "ISSUE_123", - "number": 5, - "url": "https://github.com/owner/repo/issues/5", + "id": "ISSUE_123", + "url": "https://github.com/owner/repo/issues/5", }, }, }), @@ -2264,29 +2193,7 @@ func TestGranularSetIssueFields(t *testing.T) { }), ), githubv4mock.NewMutationMatcher( - struct { - SetIssueFieldValue struct { - Issue struct { - ID githubv4.ID - Number githubv4.Int - URL githubv4.String - } - IssueFieldValues []struct { - TextValue struct { - Value string - } `graphql:"... on IssueFieldTextValue"` - SingleSelectValue struct { - Name string - } `graphql:"... on IssueFieldSingleSelectValue"` - DateValue struct { - Value string - } `graphql:"... on IssueFieldDateValue"` - NumberValue struct { - Value float64 - } `graphql:"... on IssueFieldNumberValue"` - } - } `graphql:"setIssueFieldValue(input: $input)"` - }{}, + setIssueFieldValueMutation{}, SetIssueFieldValueInput{ IssueID: githubv4.ID("ISSUE_123"), IssueFields: []IssueFieldCreateOrUpdateInput{ @@ -2302,9 +2209,8 @@ func TestGranularSetIssueFields(t *testing.T) { githubv4mock.DataResponse(map[string]any{ "setIssueFieldValue": map[string]any{ "issue": map[string]any{ - "id": "ISSUE_123", - "number": 5, - "url": "https://github.com/owner/repo/issues/5", + "id": "ISSUE_123", + "url": "https://github.com/owner/repo/issues/5", }, }, }), @@ -2356,29 +2262,7 @@ func TestGranularSetIssueFields(t *testing.T) { }), ), githubv4mock.NewMutationMatcher( - struct { - SetIssueFieldValue struct { - Issue struct { - ID githubv4.ID - Number githubv4.Int - URL githubv4.String - } - IssueFieldValues []struct { - TextValue struct { - Value string - } `graphql:"... on IssueFieldTextValue"` - SingleSelectValue struct { - Name string - } `graphql:"... on IssueFieldSingleSelectValue"` - DateValue struct { - Value string - } `graphql:"... on IssueFieldDateValue"` - NumberValue struct { - Value float64 - } `graphql:"... on IssueFieldNumberValue"` - } - } `graphql:"setIssueFieldValue(input: $input)"` - }{}, + setIssueFieldValueMutation{}, SetIssueFieldValueInput{ IssueID: githubv4.ID("ISSUE_123"), IssueFields: []IssueFieldCreateOrUpdateInput{ @@ -2392,9 +2276,8 @@ func TestGranularSetIssueFields(t *testing.T) { githubv4mock.DataResponse(map[string]any{ "setIssueFieldValue": map[string]any{ "issue": map[string]any{ - "id": "ISSUE_123", - "number": 5, - "url": "https://github.com/owner/repo/issues/5", + "id": "ISSUE_123", + "url": "https://github.com/owner/repo/issues/5", }, }, }), diff --git a/pkg/github/issues_granular.go b/pkg/github/issues_granular.go index c1eb556c9c..ab52cab59f 100644 --- a/pkg/github/issues_granular.go +++ b/pkg/github/issues_granular.go @@ -1263,6 +1263,27 @@ type IssueFieldCreateOrUpdateInput struct { Suggest *githubv4.Boolean `json:"suggest,omitempty"` } +type setIssueFieldValueMutation struct { + SetIssueFieldValue struct { + Issue struct { + ID githubv4.ID + URL githubv4.String + } + } `graphql:"setIssueFieldValue(input: $input)"` +} + +// SetIssueFieldValues updates Issue Field values and returns the updated issue. +func SetIssueFieldValues(ctx context.Context, gqlClient *githubv4.Client, input SetIssueFieldValueInput) (MinimalResponse, error) { + var mutation setIssueFieldValueMutation + if err := gqlClient.Mutate(ctx, &mutation, input, nil); err != nil { + return MinimalResponse{}, err + } + return MinimalResponse{ + ID: fmt.Sprintf("%v", mutation.SetIssueFieldValue.Issue.ID), + URL: string(mutation.SetIssueFieldValue.Issue.URL), + }, nil +} + // GranularSetIssueFields creates a tool to set issue field values on an issue using GraphQL. func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.ServerTool { st := NewTool( @@ -1486,31 +1507,6 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to get issue", err), nil, nil } - // Execute the setIssueFieldValue mutation - var mutation struct { - SetIssueFieldValue struct { - Issue struct { - ID githubv4.ID - Number githubv4.Int - URL githubv4.String - } - IssueFieldValues []struct { - TextValue struct { - Value string - } `graphql:"... on IssueFieldTextValue"` - SingleSelectValue struct { - Name string - } `graphql:"... on IssueFieldSingleSelectValue"` - DateValue struct { - Value string - } `graphql:"... on IssueFieldDateValue"` - NumberValue struct { - Value float64 - } `graphql:"... on IssueFieldNumberValue"` - } - } `graphql:"setIssueFieldValue(input: $input)"` - } - mutationInput := SetIssueFieldValueInput{ IssueID: issueID, IssueFields: issueFields, @@ -1519,14 +1515,12 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv // The rationale and suggest input fields on IssueFieldCreateOrUpdateInput // are gated behind the update_issue_suggestions GraphQL feature flag. ctxWithFeatures := ghcontext.WithGraphQLFeatures(ctx, "update_issue_suggestions") - if err := gqlClient.Mutate(ctxWithFeatures, &mutation, mutationInput, nil); err != nil { + response, err := SetIssueFieldValues(ctxWithFeatures, gqlClient, mutationInput) + if err != nil { return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to set issue field values", err), nil, nil } - r, err := json.Marshal(MinimalResponse{ - ID: fmt.Sprintf("%v", mutation.SetIssueFieldValue.Issue.ID), - URL: string(mutation.SetIssueFieldValue.Issue.URL), - }) + r, err := json.Marshal(response) if err != nil { return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil } diff --git a/pkg/github/projects.go b/pkg/github/projects.go index 514964be93..4ae131089f 100644 --- a/pkg/github/projects.go +++ b/pkg/github/projects.go @@ -1192,23 +1192,7 @@ func getProjectField(ctx context.Context, client *github.Client, owner, ownerTyp } func getProjectItem(ctx context.Context, client *github.Client, owner, ownerType string, projectNumber int, itemID int64, fields []int64) (*mcp.CallToolResult, any, error) { - var resp *github.Response - var projectItem *github.ProjectV2Item - var opts *github.GetProjectItemOptions - var err error - - if len(fields) > 0 { - opts = &github.GetProjectItemOptions{ - Fields: fields, - } - } - - if ownerType == "org" { - projectItem, resp, err = client.Projects.GetOrganizationProjectItem(ctx, owner, projectNumber, itemID, opts) - } else { - projectItem, resp, err = client.Projects.GetUserProjectItem(ctx, owner, projectNumber, itemID, opts) - } - + projectItem, resp, err := fetchProjectItem(ctx, client, owner, ownerType, projectNumber, itemID, fields) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get project item", @@ -1234,8 +1218,29 @@ func getProjectItem(ctx context.Context, client *github.Client, owner, ownerType return utils.NewToolResultText(string(r)), nil, nil } +func fetchProjectItem(ctx context.Context, client *github.Client, owner, ownerType string, projectNumber int, itemID int64, fields []int64) (*github.ProjectV2Item, *github.Response, error) { + var resp *github.Response + var projectItem *github.ProjectV2Item + var opts *github.GetProjectItemOptions + var err error + + if len(fields) > 0 { + opts = &github.GetProjectItemOptions{ + Fields: fields, + } + } + + if ownerType == "org" { + projectItem, resp, err = client.Projects.GetOrganizationProjectItem(ctx, owner, projectNumber, itemID, opts) + } else { + projectItem, resp, err = client.Projects.GetUserProjectItem(ctx, owner, projectNumber, itemID, opts) + } + + return projectItem, resp, err +} + func updateProjectItem(ctx context.Context, client *github.Client, gqlClient *githubv4.Client, owner, ownerType string, projectNumber int, itemID int64, fieldValue map[string]any) (*mcp.CallToolResult, any, error) { - updatePayload, err := buildUpdateProjectItem(ctx, gqlClient, owner, ownerType, projectNumber, fieldValue) + updatePayload, issueField, err := buildUpdateProjectItem(ctx, gqlClient, owner, ownerType, projectNumber, fieldValue) if err != nil { var structured *ghErrors.StructuredResolutionError if errors.As(err, &structured) { @@ -1244,6 +1249,44 @@ func updateProjectItem(ctx context.Context, client *github.Client, gqlClient *gi return utils.NewToolResultError(err.Error()), nil, nil } + if issueField != nil { + projectItem, resp, fetchErr := fetchProjectItem(ctx, client, owner, ownerType, projectNumber, itemID, nil) + if fetchErr != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get project item", resp, fetchErr), nil, nil + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + body, readErr := io.ReadAll(resp.Body) + if readErr != nil { + return nil, nil, fmt.Errorf("failed to read response body: %w", readErr) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get project item", resp, body), nil, nil + } + + issueID, resolveErr := projectItemIssueID(projectItem) + if resolveErr != nil { + var structured *ghErrors.StructuredResolutionError + if errors.As(resolveErr, &structured) { + return ghErrors.NewStructuredResolutionErrorResponse(structured), nil, nil + } + return utils.NewToolResultError(resolveErr.Error()), nil, nil + } + + response, mutationErr := SetIssueFieldValues(ctx, gqlClient, SetIssueFieldValueInput{ + IssueID: issueID, + IssueFields: []IssueFieldCreateOrUpdateInput{*issueField}, + }) + if mutationErr != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to set issue field value", mutationErr), nil, nil + } + + r, marshalErr := json.Marshal(response) + if marshalErr != nil { + return nil, nil, fmt.Errorf("failed to marshal response: %w", marshalErr) + } + return utils.NewToolResultText(string(r)), nil, nil + } + var resp *github.Response var updatedItem *github.ProjectV2Item @@ -1277,6 +1320,42 @@ func updateProjectItem(ctx context.Context, client *github.Client, gqlClient *gi return utils.NewToolResultText(string(r)), nil, nil } +func projectItemIssueID(item *github.ProjectV2Item) (githubv4.ID, error) { + if item == nil { + return nil, ghErrors.NewStructuredResolutionError( + "missing_metadata", + "", + "project item metadata is missing", + nil, + ) + } + + contentType := "" + if item.ContentType != nil { + contentType = string(*item.ContentType) + } + if contentType != string(github.ProjectV2ItemContentTypeIssue) { + return nil, ghErrors.NewStructuredResolutionError( + "unsupported_item_type", + contentType, + "attached Issue Fields can only be updated on Issue project items", + nil, + ) + } + + content := item.GetContent() + if content == nil || content.GetIssue() == nil || content.GetIssue().GetNodeID() == "" { + return nil, ghErrors.NewStructuredResolutionError( + "missing_metadata", + contentType, + "project Issue item is missing its Issue node ID", + nil, + ) + } + + return githubv4.ID(content.GetIssue().GetNodeID()), nil +} + func deleteProjectItem(ctx context.Context, client *github.Client, owner, ownerType string, projectNumber int, itemID int64) (*mcp.CallToolResult, any, error) { var resp *github.Response var err error @@ -1614,15 +1693,15 @@ func validateAndConvertToInt64(value any) (int64, error) { } } -// buildUpdateProjectItem builds UpdateProjectItemOptions, resolving field names and SINGLE_SELECT option names server-side. -func buildUpdateProjectItem(ctx context.Context, gqlClient *githubv4.Client, owner, ownerType string, projectNumber int, input map[string]any) (*github.UpdateProjectItemOptions, error) { +// buildUpdateProjectItem builds either a standard Project update or an attached Issue Field update. +func buildUpdateProjectItem(ctx context.Context, gqlClient *githubv4.Client, owner, ownerType string, projectNumber int, input map[string]any) (*github.UpdateProjectItemOptions, *IssueFieldCreateOrUpdateInput, error) { if input == nil { - return nil, fmt.Errorf("updated_field must be an object") + return nil, nil, fmt.Errorf("updated_field must be an object") } valueField, hasValue := input["value"] if !hasValue { - return nil, fmt.Errorf("updated_field.value is required") + return nil, nil, fmt.Errorf("updated_field.value is required") } idField, hasID := input["id"] @@ -1630,9 +1709,9 @@ func buildUpdateProjectItem(ctx context.Context, gqlClient *githubv4.Client, own switch { case hasID && hasName: - return nil, fmt.Errorf("updated_field must set either id or name, not both") + return nil, nil, fmt.Errorf("updated_field must set either id or name, not both") case !hasID && !hasName: - return nil, fmt.Errorf("updated_field requires either id or name") + return nil, nil, fmt.Errorf("updated_field requires either id or name") } var ( @@ -1644,24 +1723,35 @@ func buildUpdateProjectItem(ctx context.Context, gqlClient *githubv4.Client, own var err error fieldID, err = validateAndConvertToInt64(idField) if err != nil { - return nil, fmt.Errorf("updated_field.id: %w", err) + return nil, nil, fmt.Errorf("updated_field.id: %w", err) } } else { fieldName, ok := nameField.(string) if !ok || fieldName == "" { - return nil, fmt.Errorf("updated_field.name must be a non-empty string") + return nil, nil, fmt.Errorf("updated_field.name must be a non-empty string") } if gqlClient == nil { - return nil, fmt.Errorf("internal error: gqlClient is required to resolve updated_field.name") + return nil, nil, fmt.Errorf("internal error: gqlClient is required to resolve updated_field.name") } var err error resolved, err = resolveProjectFieldByName(ctx, gqlClient, owner, ownerType, projectNumber, fieldName, "") if err != nil { - return nil, err + return nil, nil, err + } + resolved, err = resolveIssueFieldForUpdate(ctx, gqlClient, owner, ownerType, projectNumber, resolved) + if err != nil { + return nil, nil, err + } + if resolved.IsIssueField { + issueField, buildErr := buildIssueFieldUpdate(resolved, valueField) + if buildErr != nil { + return nil, nil, buildErr + } + return nil, issueField, nil } parsedID, parseErr := parseInt64(resolved.ID) if parseErr != nil { - return nil, fmt.Errorf("resolved field %q has non-numeric ID %q; pass updated_field.id directly", resolved.Name, resolved.ID) + return nil, nil, fmt.Errorf("resolved field %q has non-numeric ID %q; pass updated_field.id directly", resolved.Name, resolved.ID) } fieldID = parsedID } @@ -1681,7 +1771,7 @@ func buildUpdateProjectItem(ctx context.Context, gqlClient *githubv4.Client, own } } if !known { - return nil, optErr + return nil, nil, optErr } } } @@ -1694,7 +1784,80 @@ func buildUpdateProjectItem(ctx context.Context, gqlClient *githubv4.Client, own }}, } - return payload, nil + return payload, nil, nil +} + +func buildIssueFieldUpdate(field *ResolvedField, value any) (*IssueFieldCreateOrUpdateInput, error) { + switch field.DataType { + case "TEXT", "NUMBER", "DATE", "SINGLE_SELECT": + default: + return nil, ghErrors.NewStructuredResolutionError( + "unsupported_field_type", + field.Name, + fmt.Sprintf("attached Issue Field %q has unsupported data type %q", field.Name, field.DataType), + nil, + ) + } + + if field.IssueFieldID == "" { + return nil, ghErrors.NewStructuredResolutionError( + "missing_field_metadata", + field.Name, + fmt.Sprintf("attached Issue Field %q is missing its Issue Field node ID", field.Name), + nil, + ) + } + + input := &IssueFieldCreateOrUpdateInput{FieldID: githubv4.ID(field.IssueFieldID)} + if value == nil { + input.Delete = githubv4.NewBoolean(githubv4.Boolean(true)) + return input, nil + } + + switch field.DataType { + case "TEXT": + text, ok := value.(string) + if !ok { + return nil, invalidIssueFieldValue(field, "value must be a string") + } + input.TextValue = githubv4.NewString(githubv4.String(text)) + case "NUMBER": + number, ok := toFloat64(value) + if !ok { + return nil, invalidIssueFieldValue(field, "value must be a number") + } + input.NumberValue = githubv4.NewFloat(githubv4.Float(number)) + case "DATE": + date, ok := value.(string) + if !ok { + return nil, invalidIssueFieldValue(field, "value must be a date string in YYYY-MM-DD format") + } + if _, err := time.Parse(time.DateOnly, date); err != nil { + return nil, invalidIssueFieldValue(field, "value must be a valid date in YYYY-MM-DD format") + } + input.DateValue = githubv4.NewString(githubv4.String(date)) + case "SINGLE_SELECT": + optionName, ok := value.(string) + if !ok || optionName == "" { + return nil, invalidIssueFieldValue(field, "value must be a non-empty option name") + } + optionID, err := resolveSingleSelectOptionByName(field, optionName) + if err != nil { + return nil, err + } + input.SingleSelectOptionID = githubv4.NewID(githubv4.ID(optionID)) + } + + return input, nil +} + +func invalidIssueFieldValue(field *ResolvedField, hint string) error { + return ghErrors.NewStructuredResolutionError( + "invalid_field_value", + field.Name, + fmt.Sprintf("invalid value for attached Issue Field %q: %s", field.Name, hint), + nil, + ) } func extractPaginationOptionsFromArgs(args map[string]any) (github.ListProjectsPaginationOptions, error) { diff --git a/pkg/github/projects_resolver.go b/pkg/github/projects_resolver.go index 3643d6eafa..a4b193f1dd 100644 --- a/pkg/github/projects_resolver.go +++ b/pkg/github/projects_resolver.go @@ -6,6 +6,7 @@ import ( "strconv" "strings" + ghcontext "github.com/github/github-mcp-server/pkg/context" ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/shurcooL/githubv4" ) @@ -28,6 +29,9 @@ type ResolvedField struct { Name string DataType string Options []ResolvedFieldOption + + IsIssueField bool + IssueFieldID string } // projectFieldsQueryOrg fetches all fields on an org-owned project (paginated). @@ -216,6 +220,160 @@ func resolveProjectFieldByName(ctx context.Context, gqlClient *githubv4.Client, return &field, nil } +type projectIssueFieldMetadata struct { + IssueFieldText struct{ ID githubv4.ID } `graphql:"... on IssueFieldText"` + IssueFieldNumber struct{ ID githubv4.ID } `graphql:"... on IssueFieldNumber"` + IssueFieldDate struct{ ID githubv4.ID } `graphql:"... on IssueFieldDate"` + IssueFieldSingleSelect struct { + ID githubv4.ID + Options []struct { + ID githubv4.ID + Name githubv4.String + } + } `graphql:"... on IssueFieldSingleSelect"` +} + +type projectIssueFieldMetadataConnection struct { + Nodes []struct { + TypeName githubv4.String `graphql:"__typename"` + ProjectV2Field struct { + DatabaseID githubv4.Int `graphql:"databaseId"` + IsIssueField githubv4.Boolean + IssueField projectIssueFieldMetadata + } `graphql:"... on ProjectV2Field"` + ProjectV2SingleSelectField struct { + DatabaseID githubv4.Int `graphql:"databaseId"` + IsIssueField githubv4.Boolean + IssueField projectIssueFieldMetadata + } `graphql:"... on ProjectV2SingleSelectField"` + } + PageInfo PageInfoFragment +} + +type projectIssueFieldMetadataQueryOrg struct { + Organization struct { + ProjectV2 struct { + Fields projectIssueFieldMetadataConnection `graphql:"fields(first: $first, after: $after)"` + } `graphql:"projectV2(number: $projectNumber)"` + } `graphql:"organization(login: $owner)"` +} + +type projectIssueFieldMetadataQueryUser struct { + User struct { + ProjectV2 struct { + Fields projectIssueFieldMetadataConnection `graphql:"fields(first: $first, after: $after)"` + } `graphql:"projectV2(number: $projectNumber)"` + } `graphql:"user(login: $owner)"` +} + +func resolveIssueFieldForUpdate(ctx context.Context, gqlClient *githubv4.Client, owner, ownerType string, projectNumber int, resolved *ResolvedField) (*ResolvedField, error) { + field := *resolved + var after *githubv4.String + + for { + vars := map[string]any{ + "owner": githubv4.String(owner), + "projectNumber": githubv4.Int(int32(projectNumber)), //nolint:gosec // Project numbers are small + "first": githubv4.Int(resolverFieldsPageSize), + "after": (*githubv4.String)(nil), + } + if after != nil { + vars["after"] = after + } + + var conn projectIssueFieldMetadataConnection + ctxWithFeatures := ghcontext.WithGraphQLFeatures(ctx, "issue_fields") + var queryErr error + if ownerType == "org" { + var q projectIssueFieldMetadataQueryOrg + queryErr = gqlClient.Query(ctxWithFeatures, &q, vars) + conn = q.Organization.ProjectV2.Fields + } else { + var q projectIssueFieldMetadataQueryUser + queryErr = gqlClient.Query(ctxWithFeatures, &q, vars) + conn = q.User.ProjectV2.Fields + } + if queryErr != nil { + if isMissingIssueFieldSchemaError(queryErr) { + return &field, nil + } + return nil, fmt.Errorf("failed to query project Issue Field metadata: %w", queryErr) + } + + for _, node := range conn.Nodes { + switch string(node.TypeName) { + case "ProjectV2Field": + if fmt.Sprintf("%d", node.ProjectV2Field.DatabaseID) == field.ID { + enrichIssueField(&field, bool(node.ProjectV2Field.IsIssueField), node.ProjectV2Field.IssueField) + return &field, nil + } + case "ProjectV2SingleSelectField": + if fmt.Sprintf("%d", node.ProjectV2SingleSelectField.DatabaseID) == field.ID { + enrichIssueField(&field, bool(node.ProjectV2SingleSelectField.IsIssueField), node.ProjectV2SingleSelectField.IssueField) + return &field, nil + } + } + } + + if !bool(conn.PageInfo.HasNextPage) { + break + } + end := conn.PageInfo.EndCursor + after = &end + } + + return nil, ghErrors.NewStructuredResolutionError( + "missing_field_metadata", + field.Name, + fmt.Sprintf("resolved field %q is missing update metadata", field.Name), + nil, + ) +} + +func enrichIssueField(field *ResolvedField, isIssueField bool, metadata projectIssueFieldMetadata) { + if !isIssueField { + return + } + field.IsIssueField = true + + switch field.DataType { + case "TEXT": + field.IssueFieldID = graphqlIDString(metadata.IssueFieldText.ID) + case "NUMBER": + field.IssueFieldID = graphqlIDString(metadata.IssueFieldNumber.ID) + case "DATE": + field.IssueFieldID = graphqlIDString(metadata.IssueFieldDate.ID) + case "SINGLE_SELECT": + field.IssueFieldID = graphqlIDString(metadata.IssueFieldSingleSelect.ID) + field.Options = make([]ResolvedFieldOption, 0, len(metadata.IssueFieldSingleSelect.Options)) + for _, option := range metadata.IssueFieldSingleSelect.Options { + field.Options = append(field.Options, ResolvedFieldOption{ + ID: graphqlIDString(option.ID), + Name: string(option.Name), + }) + } + } +} + +func graphqlIDString(id githubv4.ID) string { + if id == nil { + return "" + } + return fmt.Sprintf("%v", id) +} + +func isMissingIssueFieldSchemaError(err error) bool { + switch err.Error() { + case "Field 'isIssueField' doesn't exist on type 'ProjectV2Field'", + "Field 'issueField' doesn't exist on type 'ProjectV2Field'", + "Field 'isIssueField' doesn't exist on type 'ProjectV2SingleSelectField'", + "Field 'issueField' doesn't exist on type 'ProjectV2SingleSelectField'": + return true + default: + return false + } +} + // resolveSingleSelectOptionByName resolves an option name to its ID on a // SINGLE_SELECT field. Returns a structured error if not found or ambiguous. func resolveSingleSelectOptionByName(field *ResolvedField, optionName string) (string, error) { diff --git a/pkg/github/projects_resolver_test.go b/pkg/github/projects_resolver_test.go index b08e00cac6..7632a7002d 100644 --- a/pkg/github/projects_resolver_test.go +++ b/pkg/github/projects_resolver_test.go @@ -7,6 +7,8 @@ import ( "testing" "github.com/github/github-mcp-server/internal/githubv4mock" + "github.com/github/github-mcp-server/pkg/http/headers" + transportpkg "github.com/github/github-mcp-server/pkg/http/transport" "github.com/github/github-mcp-server/pkg/translations" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" @@ -139,6 +141,102 @@ func Test_ResolveProjectFieldByName_Success(t *testing.T) { assert.Equal(t, "OPT_b", optionID) } +func Test_ResolveIssueFieldForUpdate(t *testing.T) { + tests := []struct { + name string + resolved ResolvedField + databaseID int + typeName string + issueField map[string]any + wantID string + wantOption ResolvedFieldOption + }{ + {name: "text", resolved: ResolvedField{ID: "101", Name: "Customer", DataType: "TEXT"}, databaseID: 101, typeName: "ProjectV2Field", issueField: map[string]any{"id": "IF_TEXT"}, wantID: "IF_TEXT"}, + { + name: "single select", resolved: ResolvedField{ID: "102", Name: "Impact", DataType: "SINGLE_SELECT"}, + databaseID: 102, typeName: "ProjectV2SingleSelectField", + issueField: map[string]any{"id": "IF_SELECT", "options": []any{map[string]any{"id": "OPT_HIGH", "name": "High"}}}, + wantID: "IF_SELECT", + wantOption: ResolvedFieldOption{ID: "OPT_HIGH", Name: "High"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mocked := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher(projectIssueFieldMetadataQueryOrg{}, fieldsQueryVars("octo-org", 7), + githubv4mock.DataResponse(issueFieldMetadataResponse(tt.typeName, tt.databaseID, true, tt.issueField))), + ) + capture := &headerCaptureTransport{inner: mocked.Transport} + gql := githubv4.NewClient(&http.Client{Transport: &transportpkg.GraphQLFeaturesTransport{Transport: capture}}) + + field, err := resolveIssueFieldForUpdate(context.Background(), gql, "octo-org", "org", 7, &tt.resolved) + require.NoError(t, err) + assert.True(t, field.IsIssueField) + assert.Equal(t, tt.wantID, field.IssueFieldID) + if tt.wantOption.ID != "" { + assert.Equal(t, []ResolvedFieldOption{tt.wantOption}, field.Options) + } + assert.Equal(t, "issue_fields", capture.captured.Get(headers.GraphQLFeaturesHeader)) + }) + } +} + +func Test_ResolveIssueFieldForUpdate_ErrorHandling(t *testing.T) { + for _, tt := range []struct { + name, message string + fallback bool + }{ + {name: "missing schema falls back", message: "Field 'isIssueField' doesn't exist on type 'ProjectV2Field'", fallback: true}, + {name: "unrelated error propagates", message: "Resource not accessible by integration"}, + } { + t.Run(tt.name, func(t *testing.T) { + mocked := githubv4mock.NewMockedHTTPClient(githubv4mock.NewQueryMatcher( + projectIssueFieldMetadataQueryOrg{}, fieldsQueryVars("octo-org", 7), githubv4mock.ErrorResponse(tt.message), + )) + resolved := &ResolvedField{ID: "101", Name: "Status", DataType: "SINGLE_SELECT"} + field, err := resolveIssueFieldForUpdate(context.Background(), githubv4.NewClient(mocked), "octo-org", "org", 7, resolved) + if tt.fallback { + require.NoError(t, err) + assert.Equal(t, resolved, field) + } else { + require.ErrorContains(t, err, tt.message) + } + }) + } +} + +func Test_ResolveFieldNamesToIDs_QueryRemainsIssueFieldUngated(t *testing.T) { + mocked := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + genericFieldNode("PVTF_text", 101, "Customer", "TEXT"), + })), + ), + ) + capture := &headerCaptureTransport{inner: mocked.Transport} + gql := githubv4.NewClient(&http.Client{Transport: &transportpkg.GraphQLFeaturesTransport{Transport: capture}}) + + ids, err := resolveFieldNamesToIDs(context.Background(), gql, "octo-org", "org", 1, []string{"Customer"}) + require.NoError(t, err) + assert.Equal(t, []int64{101}, ids) + assert.Empty(t, capture.captured.Get(headers.GraphQLFeaturesHeader)) +} + +func issueFieldMetadataResponse(typeName string, databaseID any, isIssueField bool, issueField map[string]any) map[string]any { + node := map[string]any{ + "__typename": typeName, + "databaseId": databaseID, + "isIssueField": isIssueField, + } + if issueField != nil { + node["issueField"] = issueField + } + return fieldsResponse([]map[string]any{node}) +} + func Test_ResolveProjectFieldByName_NodeIDsForAllVariants(t *testing.T) { mocked := githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( @@ -740,6 +838,30 @@ func Test_ProjectsWrite_UpdateProjectItem_ByName(t *testing.T) { }), })), ), + // 4. supplemental update metadata confirms this is a standard Project field + githubv4mock.NewQueryMatcher( + projectIssueFieldMetadataQueryOrg{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(map[string]any{ + "organization": map[string]any{ + "projectV2": map[string]any{ + "fields": map[string]any{ + "nodes": []any{ + map[string]any{ + "__typename": "ProjectV2SingleSelectField", + "databaseId": 101, + "isIssueField": false, + }, + }, + "pageInfo": map[string]any{ + "hasNextPage": false, "hasPreviousPage": false, + "startCursor": "", "endCursor": "", + }, + }, + }, + }, + }), + ), ) gqlClient := githubv4.NewClient(mockedGQL) diff --git a/pkg/github/projects_test.go b/pkg/github/projects_test.go index 92de4a5d5f..4a3eacaeb3 100644 --- a/pkg/github/projects_test.go +++ b/pkg/github/projects_test.go @@ -8,7 +8,10 @@ import ( "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" + ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/translations" + gogithub "github.com/google/go-github/v89/github" "github.com/google/jsonschema-go/jsonschema" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" @@ -1283,6 +1286,166 @@ func Test_ProjectsWrite_UpdateProjectItem(t *testing.T) { }) } +func Test_ProjectItemReads_FieldNamesIncludeIssueFieldValues(t *testing.T) { + item := issueProjectItemFixture("Issue") + tests := []struct { + name string + tool inventory.ServerTool + method string + restPath string + response any + }{ + {name: "get project item", tool: ProjectsGet(translations.NullTranslationHelper), method: "get_project_item", restPath: GetOrgsProjectsV2ItemsByProjectByItemID, response: item}, + {name: "list project items", tool: ProjectsList(translations.NullTranslationHelper), method: "list_project_items", restPath: GetOrgsProjectsV2ItemsByProject, response: []any{item}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + tt.restPath: mockResponse(t, http.StatusOK, tt.response), + })) + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + genericFieldNode("PVTF_customer", 101, "Customer", "TEXT"), + })), + ), + )) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := tt.tool.Handler(deps) + args := map[string]any{"method": tt.method, "owner": "octo-org", "owner_type": "org", "project_number": float64(1), "field_names": []any{"Customer"}} + if tt.method == "get_project_item" { + args["item_id"] = float64(1001) + } + + request := createMCPRequest(args) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + if tt.method == "list_project_items" { + response = response["items"].([]any)[0].(map[string]any) + } + fields := response["fields"].([]any) + require.Len(t, fields, 1) + assert.Equal(t, "Customer", fields[0].(map[string]any)["name"]) + assert.Equal(t, "Acme", fields[0].(map[string]any)["value"]) + }) + } +} + +func Test_ProjectsWrite_UpdateProjectItem_AttachedIssueFieldDispatch(t *testing.T) { + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{genericFieldNode("PVTF_field", 101, "Customer", "TEXT")})), + ), + githubv4mock.NewQueryMatcher( + projectIssueFieldMetadataQueryOrg{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(issueFieldMetadataResponse( + "ProjectV2Field", 101, true, map[string]any{"id": "IF_TEXT"}, + )), + ), + githubv4mock.NewMutationMatcher( + setIssueFieldValueMutation{}, + SetIssueFieldValueInput{ + IssueID: githubv4.ID("ISSUE_1"), + IssueFields: []IssueFieldCreateOrUpdateInput{{ + FieldID: githubv4.ID("IF_TEXT"), + TextValue: githubv4.NewString("Acme"), + }}, + }, + nil, + githubv4mock.DataResponse(map[string]any{"setIssueFieldValue": map[string]any{ + "issue": map[string]any{"id": "ISSUE_1", "url": "https://github.com/octo-org/repo/issues/1"}, + }}), + ), + )) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetOrgsProjectsV2ItemsByProjectByItemID: mockResponse(t, http.StatusOK, issueProjectItemFixture("Issue")), + })) + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + tool := ProjectsWrite(translations.NullTranslationHelper) + handler := tool.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_item", "owner": "octo-org", "owner_type": "org", + "project_number": float64(1), "item_id": float64(1001), + "updated_field": map[string]any{"name": "Customer", "value": "Acme"}, + }) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + assert.JSONEq(t, `{"id":"ISSUE_1","url":"https://github.com/octo-org/repo/issues/1"}`, getTextResult(t, result).Text) +} + +func Test_BuildIssueFieldUpdate(t *testing.T) { + selectField := ResolvedField{ + Name: "Impact", DataType: "SINGLE_SELECT", IssueFieldID: "IF_SELECT", + Options: []ResolvedFieldOption{{ID: "OPT_HIGH", Name: "High"}}, + } + tests := []struct { + name string + field ResolvedField + value any + kind string + want *IssueFieldCreateOrUpdateInput + }{ + {name: "text", field: ResolvedField{Name: "Customer", DataType: "TEXT", IssueFieldID: "IF_TEXT"}, value: "Acme", want: &IssueFieldCreateOrUpdateInput{FieldID: githubv4.ID("IF_TEXT"), TextValue: githubv4.NewString("Acme")}}, + {name: "number", field: ResolvedField{Name: "Score", DataType: "NUMBER", IssueFieldID: "IF_NUMBER"}, value: float64(42.5), want: &IssueFieldCreateOrUpdateInput{FieldID: githubv4.ID("IF_NUMBER"), NumberValue: githubv4.NewFloat(42.5)}}, + {name: "date", field: ResolvedField{Name: "Target", DataType: "DATE", IssueFieldID: "IF_DATE"}, value: "2026-07-27", want: &IssueFieldCreateOrUpdateInput{FieldID: githubv4.ID("IF_DATE"), DateValue: githubv4.NewString("2026-07-27")}}, + {name: "single select name", field: selectField, value: "high", want: &IssueFieldCreateOrUpdateInput{FieldID: githubv4.ID("IF_SELECT"), SingleSelectOptionID: githubv4.NewID("OPT_HIGH")}}, + {name: "clear", field: ResolvedField{Name: "Customer", DataType: "TEXT", IssueFieldID: "IF_TEXT"}, value: nil, want: &IssueFieldCreateOrUpdateInput{FieldID: githubv4.ID("IF_TEXT"), Delete: githubv4.NewBoolean(true)}}, + {name: "invalid text", field: ResolvedField{Name: "Customer", DataType: "TEXT", IssueFieldID: "IF_TEXT"}, value: 42, kind: "invalid_field_value"}, + {name: "invalid number", field: ResolvedField{Name: "Score", DataType: "NUMBER", IssueFieldID: "IF_NUMBER"}, value: "42", kind: "invalid_field_value"}, + {name: "invalid date", field: ResolvedField{Name: "Target", DataType: "DATE", IssueFieldID: "IF_DATE"}, value: "2026-02-30", kind: "invalid_field_value"}, + {name: "option ID rejected", field: selectField, value: "OPT_HIGH", kind: "option_not_found"}, + {name: "missing metadata", field: ResolvedField{Name: "Customer", DataType: "TEXT"}, value: "Acme", kind: "missing_field_metadata"}, + {name: "unsupported type", field: ResolvedField{Name: "Related", DataType: "MULTI_SELECT", IssueFieldID: "IF_MULTI"}, value: "one", kind: "unsupported_field_type"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := buildIssueFieldUpdate(&tt.field, tt.value) + if tt.kind == "" { + require.NoError(t, err) + assert.Equal(t, tt.want, got) + return + } + var structured *ghErrors.StructuredResolutionError + require.ErrorAs(t, err, &structured) + assert.Equal(t, tt.kind, structured.Kind) + }) + } +} + +func Test_ProjectItemIssueID_RejectsNonIssueItems(t *testing.T) { + for _, contentType := range []string{"PullRequest", "DraftIssue"} { + t.Run(contentType, func(t *testing.T) { + item := &gogithub.ProjectV2Item{ContentType: gogithub.Ptr(gogithub.ProjectV2ItemContentType(contentType))} + _, err := projectItemIssueID(item) + var structured *ghErrors.StructuredResolutionError + require.ErrorAs(t, err, &structured) + assert.Equal(t, "unsupported_item_type", structured.Kind) + }) + } +} + +func issueProjectItemFixture(contentType string) map[string]any { + return map[string]any{ + "id": 1001, "node_id": "PVTI_1", "content_type": contentType, + "content": map[string]any{"node_id": "ISSUE_1"}, + "fields": []any{map[string]any{"id": 101, "name": "Customer", "data_type": "text", "value": "Acme"}}, + } +} + func Test_ProjectsWrite_DeleteProjectItem(t *testing.T) { toolDef := ProjectsWrite(translations.NullTranslationHelper) From cfa16b2cffc845148dd2e268c3a86af4aae5cc26 Mon Sep 17 00:00:00 2001 From: Bryan Zwicker Date: Mon, 27 Jul 2026 11:46:00 -0400 Subject: [PATCH 6/7] Preserve iteration project field updates Bypass Issue Field metadata resolution for standard field data types and recognize exact missing fragment-type schema errors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4c94f3ce-c04a-482f-830b-ab85abc3f6e4 --- pkg/github/projects.go | 31 +++++++----- pkg/github/projects_resolver.go | 6 ++- pkg/github/projects_resolver_test.go | 71 ++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 12 deletions(-) diff --git a/pkg/github/projects.go b/pkg/github/projects.go index 4ae131089f..857b4d7b37 100644 --- a/pkg/github/projects.go +++ b/pkg/github/projects.go @@ -1738,16 +1738,18 @@ func buildUpdateProjectItem(ctx context.Context, gqlClient *githubv4.Client, own if err != nil { return nil, nil, err } - resolved, err = resolveIssueFieldForUpdate(ctx, gqlClient, owner, ownerType, projectNumber, resolved) - if err != nil { - return nil, nil, err - } - if resolved.IsIssueField { - issueField, buildErr := buildIssueFieldUpdate(resolved, valueField) - if buildErr != nil { - return nil, nil, buildErr + if supportsIssueFieldUpdate(resolved.DataType) { + resolved, err = resolveIssueFieldForUpdate(ctx, gqlClient, owner, ownerType, projectNumber, resolved) + if err != nil { + return nil, nil, err + } + if resolved.IsIssueField { + issueField, buildErr := buildIssueFieldUpdate(resolved, valueField) + if buildErr != nil { + return nil, nil, buildErr + } + return nil, issueField, nil } - return nil, issueField, nil } parsedID, parseErr := parseInt64(resolved.ID) if parseErr != nil { @@ -1787,10 +1789,17 @@ func buildUpdateProjectItem(ctx context.Context, gqlClient *githubv4.Client, own return payload, nil, nil } -func buildIssueFieldUpdate(field *ResolvedField, value any) (*IssueFieldCreateOrUpdateInput, error) { - switch field.DataType { +func supportsIssueFieldUpdate(dataType string) bool { + switch dataType { case "TEXT", "NUMBER", "DATE", "SINGLE_SELECT": + return true default: + return false + } +} + +func buildIssueFieldUpdate(field *ResolvedField, value any) (*IssueFieldCreateOrUpdateInput, error) { + if !supportsIssueFieldUpdate(field.DataType) { return nil, ghErrors.NewStructuredResolutionError( "unsupported_field_type", field.Name, diff --git a/pkg/github/projects_resolver.go b/pkg/github/projects_resolver.go index a4b193f1dd..1c9ba9fdcb 100644 --- a/pkg/github/projects_resolver.go +++ b/pkg/github/projects_resolver.go @@ -367,7 +367,11 @@ func isMissingIssueFieldSchemaError(err error) bool { case "Field 'isIssueField' doesn't exist on type 'ProjectV2Field'", "Field 'issueField' doesn't exist on type 'ProjectV2Field'", "Field 'isIssueField' doesn't exist on type 'ProjectV2SingleSelectField'", - "Field 'issueField' doesn't exist on type 'ProjectV2SingleSelectField'": + "Field 'issueField' doesn't exist on type 'ProjectV2SingleSelectField'", + "No such type IssueFieldText, so it cannot be a fragment condition", + "No such type IssueFieldNumber, so it cannot be a fragment condition", + "No such type IssueFieldDate, so it cannot be a fragment condition", + "No such type IssueFieldSingleSelect, so it cannot be a fragment condition": return true default: return false diff --git a/pkg/github/projects_resolver_test.go b/pkg/github/projects_resolver_test.go index 7632a7002d..8b11690abe 100644 --- a/pkg/github/projects_resolver_test.go +++ b/pkg/github/projects_resolver_test.go @@ -188,6 +188,11 @@ func Test_ResolveIssueFieldForUpdate_ErrorHandling(t *testing.T) { fallback bool }{ {name: "missing schema falls back", message: "Field 'isIssueField' doesn't exist on type 'ProjectV2Field'", fallback: true}, + {name: "missing text fragment type falls back", message: "No such type IssueFieldText, so it cannot be a fragment condition", fallback: true}, + {name: "missing number fragment type falls back", message: "No such type IssueFieldNumber, so it cannot be a fragment condition", fallback: true}, + {name: "missing date fragment type falls back", message: "No such type IssueFieldDate, so it cannot be a fragment condition", fallback: true}, + {name: "missing single select fragment type falls back", message: "No such type IssueFieldSingleSelect, so it cannot be a fragment condition", fallback: true}, + {name: "unknown fragment type propagates", message: "No such type IssueFieldMultiSelect, so it cannot be a fragment condition"}, {name: "unrelated error propagates", message: "Resource not accessible by integration"}, } { t.Run(tt.name, func(t *testing.T) { @@ -204,6 +209,18 @@ func Test_ResolveIssueFieldForUpdate_ErrorHandling(t *testing.T) { } }) } + + t.Run("supported type missing metadata still fails", func(t *testing.T) { + mocked := githubv4mock.NewMockedHTTPClient(githubv4mock.NewQueryMatcher( + projectIssueFieldMetadataQueryOrg{}, + fieldsQueryVars("octo-org", 7), + githubv4mock.DataResponse(fieldsResponse(nil)), + )) + resolved := &ResolvedField{ID: "101", Name: "Customer", DataType: "TEXT"} + + _, err := resolveIssueFieldForUpdate(context.Background(), githubv4.NewClient(mocked), "octo-org", "org", 7, resolved) + require.ErrorContains(t, err, "missing_field_metadata") + }) } func Test_ResolveFieldNamesToIDs_QueryRemainsIssueFieldUngated(t *testing.T) { @@ -886,6 +903,60 @@ func Test_ProjectsWrite_UpdateProjectItem_ByName(t *testing.T) { require.False(t, result.IsError, getTextResult(t, result).Text) } +func Test_ProjectsWrite_UpdateProjectItem_ByNameIteration(t *testing.T) { + updatedItem := verbosePullRequestProjectItemFixture() + restCalled := false + mockedREST := MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchOrgsProjectsV2ItemsByProjectByItemID: func(w http.ResponseWriter, r *http.Request) { + restCalled = true + var update struct { + Fields []struct { + ID int64 `json:"id"` + Value any `json:"value"` + } `json:"fields"` + } + require.NoError(t, json.NewDecoder(r.Body).Decode(&update)) + require.Len(t, update.Fields, 1) + assert.Equal(t, int64(222), update.Fields[0].ID) + assert.Equal(t, "ITERATION_1", update.Fields[0].Value) + + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(updatedItem)) + }, + }) + mockedGQL := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + iterationFieldNode("PVTIF_iteration1", 222, "Sprint"), + })), + ), + ) + deps := BaseDeps{ + Client: mustNewGHClient(t, mockedREST), + GQLClient: githubv4.NewClient(mockedGQL), + } + toolDef := ProjectsWrite(translations.NullTranslationHelper) + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_item", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "item_id": float64(1001), + "updated_field": map[string]any{ + "name": "Sprint", + "value": "ITERATION_1", + }, + }) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + assert.True(t, restCalled) +} + func Test_ProjectsWrite_UpdateProjectItem_NameNotFound_StructuredError(t *testing.T) { toolDef := ProjectsWrite(translations.NullTranslationHelper) From cf9dad9064a930f6e98f83ab76ef50c93145f31a Mon Sep 17 00:00:00 2001 From: Lizeth Vera <47796851+veralizeth@users.noreply.github.com> Date: Tue, 28 Jul 2026 22:43:51 +0000 Subject: [PATCH 7/7] Adding GraphQL-Features: update_issue_suggestions --- pkg/github/projects.go | 6 +++++- pkg/github/projects_test.go | 14 ++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/pkg/github/projects.go b/pkg/github/projects.go index 857b4d7b37..4ceb432e69 100644 --- a/pkg/github/projects.go +++ b/pkg/github/projects.go @@ -10,6 +10,7 @@ import ( "strconv" "time" + ghcontext "github.com/github/github-mcp-server/pkg/context" ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/ifc" "github.com/github/github-mcp-server/pkg/inventory" @@ -1272,7 +1273,10 @@ func updateProjectItem(ctx context.Context, client *github.Client, gqlClient *gi return utils.NewToolResultError(resolveErr.Error()), nil, nil } - response, mutationErr := SetIssueFieldValues(ctx, gqlClient, SetIssueFieldValueInput{ + // The setIssueFieldValue mutation is gated behind the update_issue_suggestions + // GraphQL feature flag, matching the set_issue_fields tool. + ctxWithFeatures := ghcontext.WithGraphQLFeatures(ctx, "update_issue_suggestions") + response, mutationErr := SetIssueFieldValues(ctxWithFeatures, gqlClient, SetIssueFieldValueInput{ IssueID: issueID, IssueFields: []IssueFieldCreateOrUpdateInput{*issueField}, }) diff --git a/pkg/github/projects_test.go b/pkg/github/projects_test.go index 4a3eacaeb3..075bbb70ce 100644 --- a/pkg/github/projects_test.go +++ b/pkg/github/projects_test.go @@ -9,6 +9,8 @@ import ( "github.com/github/github-mcp-server/internal/githubv4mock" "github.com/github/github-mcp-server/internal/toolsnaps" ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/http/headers" + transportpkg "github.com/github/github-mcp-server/pkg/http/transport" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/translations" gogithub "github.com/google/go-github/v89/github" @@ -1340,7 +1342,7 @@ func Test_ProjectItemReads_FieldNamesIncludeIssueFieldValues(t *testing.T) { } func Test_ProjectsWrite_UpdateProjectItem_AttachedIssueFieldDispatch(t *testing.T) { - gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient( + mockClient := githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( projectFieldsTestQuery{}, fieldsQueryVars("octo-org", 1), @@ -1367,7 +1369,12 @@ func Test_ProjectsWrite_UpdateProjectItem_AttachedIssueFieldDispatch(t *testing. "issue": map[string]any{"id": "ISSUE_1", "url": "https://github.com/octo-org/repo/issues/1"}, }}), ), - )) + ) + + spy := &headerCaptureTransport{inner: mockClient.Transport} + gqlClient := githubv4.NewClient(&http.Client{ + Transport: &transportpkg.GraphQLFeaturesTransport{Transport: spy}, + }) restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ GetOrgsProjectsV2ItemsByProjectByItemID: mockResponse(t, http.StatusOK, issueProjectItemFixture("Issue")), })) @@ -1384,6 +1391,9 @@ func Test_ProjectsWrite_UpdateProjectItem_AttachedIssueFieldDispatch(t *testing. require.NoError(t, err) require.False(t, result.IsError, getTextResult(t, result).Text) assert.JSONEq(t, `{"id":"ISSUE_1","url":"https://github.com/octo-org/repo/issues/1"}`, getTextResult(t, result).Text) + // The last request captured is the mutation; the preceding field/metadata + // queries do not require the update_issue_suggestions feature flag. + assert.Equal(t, "update_issue_suggestions", spy.captured.Get(headers.GraphQLFeaturesHeader)) } func Test_BuildIssueFieldUpdate(t *testing.T) {