diff --git a/README.md b/README.md index 9a6ef3677..3cae52090 100644 --- a/README.md +++ b/README.md @@ -1119,6 +1119,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: 100 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) @@ -1130,7 +1131,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 762ee08c9..756b75e75 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: 100 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 308c2b87e..15c1c3023 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 = 100 ) // Method constants for consolidated project tools @@ -44,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" @@ -490,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, @@ -511,6 +590,7 @@ func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool { Enum: []any{ projectsMethodAddProjectItem, projectsMethodUpdateProjectItem, + projectsMethodUpdateProjectItems, projectsMethodDeleteProjectItem, projectsMethodCreateProjectStatusUpdate, projectsMethodCreateProject, @@ -559,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", @@ -722,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.go b/pkg/github/projects_batch.go new file mode 100644 index 000000000..3c77d250c --- /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 000000000..f17b4871f --- /dev/null +++ b/pkg/github/projects_batch_test.go @@ -0,0 +1,1285 @@ +package github + +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"} + 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 100 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_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) +} + +// 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 + 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) +} diff --git a/pkg/github/projects_test.go b/pkg/github/projects_test.go index 553c2421a..92de4a5d5 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)