Trim batch implementation commentary

Remove redundant implementation narration while retaining comments for non-obvious client and failure semantics.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d7dc302d-e6f2-41e9-a2c8-ed598de47067

Author: zwick

pkg/github/projects.go

@@ -32,10 +32,7 @@ const (
 	ProjectStatusUpdateCreateFailedError = "failed to create project status update"
 	ProjectResolveIDFailedError          = "failed to resolve project ID"
 	MaxProjectsPerPage                   = 50
-	// MaxProjectItemsPerBatch caps the number of items processed in a single
-	// update_project_items call. This keeps a batch bounded, prevents runaway
-	// resolution costs, and matches the practical scale of typical triage sweeps.
-	MaxProjectItemsPerBatch = 100
+	maxProjectItemsPerBatch              = 100
 )
 
 // Method constants for consolidated project tools
@@ -619,7 +616,7 @@ func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool {
 					},
 					"items": {
 						Type:        "array",
-						Description: "The items to update. Required for 'update_project_items'. Each entry must match exactly one reference variant: 'node_id', numeric 'item_id', or 'item_owner' + 'item_repo' + 'issue_number'. Each entry may include 'updated_field'; otherwise the top-level 'updated_field' applies. Limit: " + strconv.Itoa(MaxProjectItemsPerBatch) + " items per call.",
+						Description: "The items to update. Required for 'update_project_items'. Each entry must match exactly one reference variant: 'node_id', numeric 'item_id', or 'item_owner' + 'item_repo' + 'issue_number'. Each entry may include 'updated_field'; otherwise the top-level 'updated_field' applies. Limit: " + strconv.Itoa(maxProjectItemsPerBatch) + " items per call.",
 						Items:       updateProjectItemsItemSchema(),
 					},
 					"body": {
@@ -1596,9 +1593,6 @@ func buildUpdateProjectItem(ctx context.Context, gqlClient *githubv4.Client, own
 	return buildUpdateProjectItemWithCache(ctx, gqlClient, nil, owner, ownerType, projectNumber, input)
 }
 
-// buildUpdateProjectItemWithCache is the cache-aware variant of buildUpdateProjectItem.
-// When cache is non-nil, name-based field lookups reuse the cache's in-memory index; when
-// nil, each name lookup performs its own GraphQL round trip (single-item behaviour).
 func buildUpdateProjectItemWithCache(ctx context.Context, gqlClient *githubv4.Client, cache *projectFieldsCache, owner, ownerType string, projectNumber int, input map[string]any) (*github.UpdateProjectItemOptions, error) {
 	if input == nil {
 		return nil, fmt.Errorf("updated_field must be an object")

pkg/github/projects_batch.go

@@ -11,13 +11,8 @@ import (
 	"github.com/shurcooL/githubv4"
 )
 
-// batchItemStatus is the tri-state outcome of one item in update_project_items.
-//   - succeeded: the item's alias came back populated, confirming the write.
-//   - failed: a deterministic failure before any mutation was sent (invalid
-//     input, resolution failure, value-conversion failure, duplicate target).
-//   - unknown: a mutation touching this item was not confirmed, or was never
-//     sent after an earlier systemic failure. The pinned client drops
-//     errors[].path, so we don't retry or issue a per-item fallback mutation.
+// Unknown outcomes cannot be attributed or retried safely because the pinned
+// client drops errors[].path.
 type batchItemStatus string
 
 const (
@@ -26,21 +21,15 @@ const (
 	batchItemUnknown   batchItemStatus = "unknown"
 )
 
-// batchItemResult is the per-item outcome returned by update_project_items.
 type batchItemResult struct {
 	Index  int                `json:"index"`
 	Status batchItemStatus    `json:"status"`
 	Item   *batchItemIdentity `json:"item,omitempty"`
 	Error  *batchItemError    `json:"error,omitempty"`
-	// Ref echoes the identifying input for the item so agents can correlate results
-	// with the request even when resolution fails.
+	// Ref preserves the request identity when resolution fails.
 	Ref map[string]any `json:"ref,omitempty"`
 }
 
-// batchItemIdentity is the minimal item identity echoed back for a confirmed
-// write: the mutation response's `projectV2Item { id, fullDatabaseId }`, plus
-// the numeric item_id the request used (when known), so callers can correlate
-// without a follow-up read.
 type batchItemIdentity struct {
 	NodeID         string `json:"node_id,omitempty"`
 	FullDatabaseID string `json:"full_database_id,omitempty"`
@@ -54,21 +43,14 @@ type batchItemError struct {
 	Hint       string `json:"hint,omitempty"`
 }
 
-// resolvedBatchItem is a fully-resolved, validated write, ready to be included
-// in an aliased update or clear mutation.
 type resolvedBatchItem struct {
 	index          int
 	nodeID         string
 	fullDatabaseID int64
 	fieldNodeID    string
-	value          githubv4.ProjectV2FieldValue // unused when this is a clear
+	value          githubv4.ProjectV2FieldValue
 }
 
-// updateProjectItemsBatch handles the update_project_items method. It resolves
-// the project, every distinct field, and every item once up front, then writes
-// via chunked, aliased updateProjectV2ItemFieldValue / clearProjectV2ItemFieldValue
-// GraphQL mutations (pkg/github/projects_batch_mutation.go) instead of one REST
-// PATCH per item.
 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 {
@@ -81,8 +63,8 @@ func updateProjectItemsBatch(ctx context.Context, client *github.Client, gqlClie
 	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
+	if len(itemsRaw) > maxProjectItemsPerBatch {
+		return utils.NewToolResultError(fmt.Sprintf("items exceeds maximum of %d entries per call (got %d)", maxProjectItemsPerBatch, len(itemsRaw))), nil, nil
 	}
 
 	var sharedField map[string]any
@@ -116,17 +98,14 @@ func updateProjectItemsBatch(ctx context.Context, client *github.Client, gqlClie
 		return newUpdateProjectItemsResult(results)
 	}
 
-	// Resolve the project node ID once; every mutation input needs it.
 	projectID, err := resolveProjectNodeID(ctx, gqlClient, owner, ownerType, projectNumber)
 	if err != nil {
 		return utils.NewToolResultError(err.Error()), nil, nil
 	}
 
-	// Resolve every distinct effective field once.
 	cache := newProjectFieldsCache()
 	fieldResolutions := resolveDistinctFields(ctx, gqlClient, cache, owner, ownerType, projectNumber, parsed)
 
-	// Resolve numeric item_id -> node id, deduplicated with bounded concurrency.
 	var numericIDs []int64
 	for _, p := range parsed {
 		if p.err == nil && p.refKind == batchRefItemID {
@@ -135,16 +114,14 @@ func updateProjectItemsBatch(ctx context.Context, client *github.Client, gqlClie
 	}
 	itemIDLookups := resolveItemNodeIDsByNumericID(ctx, client, owner, ownerType, projectNumber, numericIDs)
 
-	// Resolve issue-ref items, deduplicated.
 	issueLookups := resolveIssueRefs(ctx, gqlClient, projectID, parsed)
 
-	// Build the final resolved plan, rejecting duplicate item+field targets.
 	var updates, clears []resolvedBatchItem
-	seenTargets := make(map[string]int) // "nodeID|fieldNodeID" -> first index that claimed it
+	seenTargets := make(map[string]int)
 
 	for i, p := range parsed {
 		if p.err != nil {
-			continue // deterministic parse failure already recorded above
+			continue
 		}
 
 		fr := fieldResolutions[p.fieldKey]
@@ -187,9 +164,7 @@ func updateProjectItemsBatch(ctx context.Context, client *github.Client, gqlClie
 		updates = append(updates, resolvedBatchItem{index: i, nodeID: nodeID, fullDatabaseID: fullDatabaseID, fieldNodeID: fr.field.NodeID, value: value})
 	}
 
-	// Execute update mutations, then clear mutations, in sequential chunks.
-	// Clears never run if the update phase aborted: we do not know whether an
-	// ambiguous failure landed, so we do not risk further writes this call.
+	// Skip clears after an ambiguous update failure because an earlier write may have landed.
 	aborted := executeBatchPartition(ctx, gqlClient, batchMutationUpdate, projectID, updates, results, parsed)
 	if !aborted {
 		executeBatchPartition(ctx, gqlClient, batchMutationClear, projectID, clears, results, parsed)
@@ -232,9 +207,6 @@ func newUpdateProjectItemsResult(results []batchItemResult) (*mcp.CallToolResult
 	return result, nil, nil
 }
 
-// resolveItemReference maps a parsed item's reference to its project item node
-// ID and (when known) numeric full database ID, using the pre-computed lookup
-// tables for the item_id and issue-ref cases.
 func resolveItemReference(p parsedBatchItem, itemIDLookups map[int64]itemLookupResult, issueLookups map[issueRefKey]itemLookupResult) (nodeID string, fullDatabaseID int64, err error) {
 	switch p.refKind {
 	case batchRefNodeID:
@@ -257,12 +229,8 @@ func resolveItemReference(p parsedBatchItem, itemIDLookups map[int64]itemLookupR
 	}
 }
 
-// executeBatchPartition executes a single partition (all updates, or all
-// clears) in sequential chunks of at most batchMutationWireChunkSize. It
-// writes outcomes directly into results (keyed by original index) and returns
-// true if execution was aborted before every item in the partition was
-// attempted, because an earlier chunk's failure looked systemic (transport
-// failure or context cancellation) rather than a per-mutation GraphQL error.
+// Transport, cancellation, or incomplete-data ambiguity stops later chunks;
+// GraphQL response errors do not because populated aliases still confirm writes.
 func executeBatchPartition(ctx context.Context, gqlClient *githubv4.Client, kind batchMutationKind, projectID githubv4.ID, items []resolvedBatchItem, results []batchItemResult, parsed []parsedBatchItem) bool {
 	for start := 0; start < len(items); start += batchMutationWireChunkSize {
 		if ctx.Err() != nil {

pkg/github/projects_batch_e2e_test.go

@@ -57,7 +57,7 @@ func projectIDMatcher(owner string, projectNumber int, projectNodeID string) git
 }
 
 func Test_UpdateProjectItemsBatch_TopLevelGuards(t *testing.T) {
-	tooMany := make([]any, MaxProjectItemsPerBatch+1)
+	tooMany := make([]any, maxProjectItemsPerBatch+1)
 	validItem := map[string]any{
 		"node_id":       "PVTI_item1",
 		"updated_field": map[string]any{"name": "Notes", "value": "hello"},

pkg/github/projects_batch_mutation.go

@@ -10,14 +10,8 @@ import (
 	"github.com/shurcooL/githubv4"
 )
 
-// batchMutationWireChunkSize bounds how many aliased mutations are sent per
-// GraphQL request. It is smaller than MaxProjectItemsPerBatch (100 items/call)
-// to leave headroom under typical ~10s gateway/client timeouts. This value is
-// provisional, not derived from a measured benchmark against the live API.
 const batchMutationWireChunkSize = 20
 
-// batchMutationKind selects which ProjectV2 item field-value mutation an
-// aliased request builds.
 type batchMutationKind int
 
 const (
@@ -32,33 +26,25 @@ func (k batchMutationKind) fieldName() string {
 	return "updateProjectV2ItemFieldValue"
 }
 
-// projectV2ItemMutationResult is the minimal response shape shared by both
-// updateProjectV2ItemFieldValue and clearProjectV2ItemFieldValue: both expose
-// a `projectV2Item { id, fullDatabaseId }` field.
 type projectV2ItemMutationResult struct {
 	ProjectV2Item struct {
 		ID             string
 		FullDatabaseID string `graphql:"fullDatabaseId"`
 	} `graphql:"projectV2Item"`
 }
 
-// reflectedMutationTypeKey identifies a cached reflected mutation struct type,
-// keyed only by operation kind and chunk size (never owner/project/field/value).
 type reflectedMutationTypeKey struct {
 	kind batchMutationKind
 	size int
 }
 
-var reflectedMutationTypeCache sync.Map // map[reflectedMutationTypeKey]reflect.Type
+var reflectedMutationTypeCache sync.Map
 
-// buildAliasedMutationType returns a reflect.Type for a struct with exactly
-// size fields, named Item0..ItemN-1, each tagged with a positional GraphQL
-// alias (item0, item1, ...) for kind and a positional input variable
-// reference. The pinned shurcooL/githubv4 Client.Mutate always sends its
-// third argument under the "input" variable name, so alias 0 must be the only
-// mutation referencing $input; aliases 1.. reference $input1, $input2, ...
-// supplied via the extra variables map. Struct tags are purely positional:
-// no owner, project, item, field, or value data is ever embedded in them.
+// Reflected types are cached only by operation and chunk size to bound
+// reflect.StructOf's runtime cache; positional names and tags keep request data
+// out of type identities. The pinned Client.Mutate binds its third argument to
+// $input, so item0 uses $input and later aliases use $input1, $input2, ...
+// supplied through the variables map.
 func buildAliasedMutationType(kind batchMutationKind, size int) reflect.Type {
 	key := reflectedMutationTypeKey{kind: kind, size: size}
 	if cached, ok := reflectedMutationTypeCache.Load(key); ok {
@@ -84,26 +70,17 @@ func buildAliasedMutationType(kind batchMutationKind, size int) reflect.Type {
 	return actual.(reflect.Type)
 }
 
-// mutationAliasOutcome is the per-alias outcome of one executeAliasedMutation call.
 type mutationAliasOutcome struct {
-	// Populated is true when the server returned a non-empty projectV2Item.id
-	// for this alias, confirming the write happened even if the overall
-	// request also returned a GraphQL error.
+	// Populated confirms this alias returned a project item, even when the
+	// response also contains GraphQL errors.
 	Populated      bool
 	NodeID         string
 	FullDatabaseID string
 }
 
-// executeAliasedMutation sends a single GraphQL request containing up to
-// batchMutationWireChunkSize aliased update/clear mutations (one per input),
-// using reflect.StructOf to build the request/response shape at runtime; it
-// still uses the existing authenticated *githubv4.Client.
-//
-// The pinned shurcooL/graphql client decodes any partial `data` payload
-// before returning the transport/GraphQL error, so outcomes[i].Populated
-// reports whether alias i's write is confirmed even when err is non-nil. The
-// client discards errors[].path, so a non-nil err cannot be attributed to a
-// specific alias; callers must rely on which outcomes are Populated instead.
+// The pinned client decodes partial data before returning GraphQL errors but
+// discards errors[].path. Populated aliases confirm writes; unpopulated aliases
+// remain unknown and must not be retried individually.
 func executeAliasedMutation(ctx context.Context, gqlClient *githubv4.Client, kind batchMutationKind, inputs []githubv4.Input) ([]mutationAliasOutcome, error) {
 	if len(inputs) == 0 {
 		return nil, nil
@@ -123,7 +100,6 @@ func executeAliasedMutation(ctx context.Context, gqlClient *githubv4.Client, kin
 		}
 	}
 
-	// Alias 0's input is bound positionally to $input; aliases 1.. use variables.
 	mutateErr := gqlClient.Mutate(ctx, mutationPtr.Interface(), inputs[0], variables)
 
 	outcomes := make([]mutationAliasOutcome, len(inputs))
@@ -142,8 +118,8 @@ func executeAliasedMutation(ctx context.Context, gqlClient *githubv4.Client, kin
 	return outcomes, mutateErr
 }
 
-// isGraphQLResponseError distinguishes the pinned client's unexported
-// graphql.errors response type from transport and decoding failures.
+// The pinned client's GraphQL response error type is unexported; transport and
+// decoding failures must remain distinguishable.
 func isGraphQLResponseError(err error) bool {
 	for err != nil {
 		errType := reflect.TypeOf(err)