From 2c53a948654b207140323366d4404f57f9f5c4fa Mon Sep 17 00:00:00 2001 From: Daniel Perez Date: Tue, 25 Aug 2026 09:03:22 -0700 Subject: [PATCH 1/5] Support reclaiming mannequins to customer-owned bot accounts Adds bot-target support to `gh elm target mannequin reclaim`, so an org admin can reattribute migrated content from a mannequin to a customer-owned GitHub App / bot account (previously only human users were supported). - BotID resolves a [bot] login to its node ID via the REST users endpoint. - ReattributeMannequinToBot calls the reattributeMannequinToBot mutation. - The reclaim service routes [bot]-suffixed targets (case-insensitively) through the bot path; bot reclaims auto-accept and are fail-fast. - Confirmation prompt and advisory warning before irreversible bot reclaims, skippable with --no-prompt. - Adds mannequin_claiming_bot to the GraphQL-Features header. --- README.md | 4 + internal/cmd/target/mannequins.go | 56 +++++++++- internal/cmd/target/mannequins_test.go | 140 ++++++++++++++++++++++++- internal/ghapi/ghapi.go | 9 +- internal/ghapi/ghapi_test.go | 35 ++----- internal/ghapi/reclaim.go | 35 ++++--- internal/ghapi/reclaim_test.go | 32 ++++-- 7 files changed, 252 insertions(+), 59 deletions(-) diff --git a/README.md b/README.md index 8685c10..0321ada 100644 --- a/README.md +++ b/README.md @@ -206,6 +206,10 @@ gh elm target mannequin reclaim octo-org --csv mannequins.csv # Immediate reattribution (EMU orgs only); prompts unless --no-prompt gh elm target mannequin reclaim octo-org --csv mannequins.csv --skip-invitation + +# Reclaim to a customer-owned GitHub App / bot account (target login ends in [bot]); +# irreversible, so it prompts unless --no-prompt +gh elm target mannequin reclaim octo-org legacy-ci[bot] example-ci[bot] ``` ## Configuration diff --git a/internal/cmd/target/mannequins.go b/internal/cmd/target/mannequins.go index 2a1f63a..f2b0702 100644 --- a/internal/cmd/target/mannequins.go +++ b/internal/cmd/target/mannequins.go @@ -237,13 +237,18 @@ func newMannequinReclaimCmd() *cobra.Command { if err != nil { return err } + if err := confirmBotReclaims(cmd, log, records, noPrompt); err != nil { + return err + } if err := svc.ReclaimMannequins(cmd.Context(), records, githubOrg, force, skipInvitation); err != nil { return annotateMannequinAuthError(err, targetURLResolved) } return nil } - log.Infof("Reclaiming mannequin...") + if err := confirmBotReclaims(cmd, log, []ghapi.MannequinRecord{{MannequinUser: mannequinUser, TargetUser: targetUser}}, noPrompt); err != nil { + return err + } if err := svc.ReclaimMannequin(cmd.Context(), mannequinUser, mannequinID, targetUser, githubOrg, force, skipInvitation); err != nil { return annotateMannequinAuthError(err, targetURLResolved) } @@ -259,7 +264,7 @@ func newMannequinReclaimCmd() *cobra.Command { cmd.Flags().StringVar(&targetUser, "target-user", "", "Target user login (alternative to the positional argument).") cmd.Flags().BoolVar(&force, "force", false, "Reclaim even if the mannequin is already mapped to a user.") cmd.Flags().BoolVar(&skipInvitation, "skip-invitation", false, "Reattribute immediately without the invitation email (EMU orgs only).") - cmd.Flags().BoolVar(&noPrompt, "no-prompt", false, "Do not prompt for confirmation when using --skip-invitation.") + cmd.Flags().BoolVar(&noPrompt, "no-prompt", false, "Do not prompt for confirmation (--skip-invitation or bot reclaims).") cmd.Flags().StringVar(&targetURL, "target-url", "", "Override the target API base URL.") cmd.Flags().StringVar(&targetToken, "target-token", "", "Override the target API token.") _ = cmd.Flags().MarkHidden("github-org") @@ -312,6 +317,53 @@ func confirm(in io.Reader, out io.Writer, prompt string) bool { } } +// confirmBotReclaims warns about likely mis-targets and, unless noPrompt is set, +// asks for confirmation before an irreversible bot reattribution. Reattributing +// to a bot auto-accepts and cannot be undone. The source mannequin's login is +// our only hint that it represents a bot; a non-"[bot]" source is very likely a +// mis-target (a human's content going to a bot), but the convention is +// GitHub-specific, so we warn and let the admin proceed rather than blocking. +func confirmBotReclaims(cmd *cobra.Command, log mannequinLogger, records []ghapi.MannequinRecord, noPrompt bool) error { + var botCount, humanCount int + warned := make(map[string]bool) + var firstBot ghapi.MannequinRecord + for _, r := range records { + if !ghapi.IsBotLogin(r.TargetUser) { + humanCount++ + continue + } + if botCount == 0 { + firstBot = r + } + botCount++ + if !ghapi.IsBotLogin(r.MannequinUser) && !warned[r.MannequinUser] { + warned[r.MannequinUser] = true + log.Warnf("%q does not look like a bot mannequin (its login does not end in %q). Are you sure you want to do this?", r.MannequinUser, "[bot]") + } + } + + if botCount == 0 || noPrompt { + return nil + } + + var summary string + if len(records) > 1 { + summary = fmt.Sprintf("You are about to reattribute %d mannequin(s) to GitHub App / bot account(s)", botCount) + if humanCount > 0 { + summary += fmt.Sprintf(" and %d mannequin(s) to user(s)", humanCount) + } + summary += "." + } else { + summary = fmt.Sprintf("You are about to reattribute mannequin %q to the GitHub App / bot account %q.", firstBot.MannequinUser, firstBot.TargetUser) + } + + if !confirm(cmd.InOrStdin(), cmd.ErrOrStderr(), + summary+" Reattributing content to a bot is immediate and cannot be undone. Continue? [y/N]") { + return errors.New("aborted") + } + return nil +} + // mannequinClient resolves the target endpoint (flag > env > stored config) and // returns a ready GitHub API client plus the resolved base URL for error // messages. It mirrors targetClient but returns a *ghapi.Client for the diff --git a/internal/cmd/target/mannequins_test.go b/internal/cmd/target/mannequins_test.go index f1ee275..7a081f0 100644 --- a/internal/cmd/target/mannequins_test.go +++ b/internal/cmd/target/mannequins_test.go @@ -3,6 +3,7 @@ package target import ( "bytes" "encoding/json" + "fmt" "io" "net/http" "net/http/httptest" @@ -185,7 +186,6 @@ func TestMannequinReclaim(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "aborted") }) - t.Run("accepts the legacy claim command and flags", func(t *testing.T) { _, err := runMannequin(t, newMannequinClaimCmd, "", "--github-org", "octo", "--target-url", "https://x", "--target-token", "tok") @@ -214,4 +214,142 @@ func TestMannequinReclaim(t *testing.T) { assert.Contains(t, err.Error(), "no target URL configured") assert.NotContains(t, err.Error(), "duplicates") }) + + t.Run("reclaims to a bot after confirmation", func(t *testing.T) { + srv, called := botClaimServer(t, "legacy-ci[bot]") + defer srv.Close() + + out, err := runMannequin(t, newMannequinReclaimCmd, "y\n", + "octo", "legacy-ci[bot]", "example-ci[bot]", + "--target-url", srv.URL, "--target-token", "tok") + require.NoErrorf(t, err, "claim output:\n%s", out) + assert.True(t, *called, "expected reattributeMannequinToBot to be called") + }) + + t.Run("aborts a bot reclaim when the admin declines the prompt", func(t *testing.T) { + srv, called := botClaimServer(t, "legacy-ci[bot]") + defer srv.Close() + + _, err := runMannequin(t, newMannequinReclaimCmd, "n\n", + "octo", "legacy-ci[bot]", "example-ci[bot]", + "--target-url", srv.URL, "--target-token", "tok") + require.Error(t, err) + assert.Contains(t, err.Error(), "aborted") + assert.False(t, *called, "no reclaim should occur after declining") + }) + + t.Run("bot reclaim with --no-prompt proceeds without confirmation", func(t *testing.T) { + srv, called := botClaimServer(t, "legacy-ci[bot]") + defer srv.Close() + + out, err := runMannequin(t, newMannequinReclaimCmd, "", + "octo", "legacy-ci[bot]", "example-ci[bot]", + "--no-prompt", "--target-url", srv.URL, "--target-token", "tok") + require.NoErrorf(t, err, "claim output:\n%s", out) + assert.True(t, *called, "expected reattributeMannequinToBot to be called") + }) + + t.Run("warns when the source mannequin does not look like a bot", func(t *testing.T) { + srv, called := botClaimServer(t, "alice") + defer srv.Close() + + out, err := runMannequin(t, newMannequinReclaimCmd, "y\n", + "octo", "alice", "example-ci[bot]", + "--target-url", srv.URL, "--target-token", "tok") + require.NoErrorf(t, err, "claim output:\n%s", out) + assert.True(t, *called, "expected reattributeMannequinToBot to be called") + assert.Contains(t, out, "does not look like a bot mannequin", "expected soft-signal advisory warning") + }) + + t.Run("reclaims a bot row from a CSV with mixed targets", func(t *testing.T) { + srv, botCalled, invited := botCSVServer(t) + defer srv.Close() + + path := filepath.Join(t.TempDir(), "mannequins.csv") + csv := "mannequin-user,mannequin-id,target-user\n" + + "legacy-ci[bot],m1,example-ci[bot]\n" + + "alice,m2,alice-t\n" + require.NoError(t, os.WriteFile(path, []byte(csv), 0o600)) + + out, err := runMannequin(t, newMannequinReclaimCmd, "y\n", + "octo", "--csv", path, + "--target-url", srv.URL, "--target-token", "tok") + require.NoErrorf(t, err, "claim output:\n%s", out) + assert.True(t, *botCalled, "expected reattributeMannequinToBot to be called for the bot row") + assert.True(t, *invited, "expected createAttributionInvitation to be called for the human row") + }) +} + +// botCSVServer answers the org id, all-mannequins listing, REST bot lookup, user +// lookup, and both the reattributeMannequinToBot and createAttributionInvitation +// calls made by a mixed CSV reclaim (bot "example-ci[bot]" plus human "alice-t"). +// The returned bools are set when the bot mutation and the invitation are +// invoked, respectively. +func botCSVServer(t *testing.T) (srv *httptest.Server, botCalled, invited *bool) { + botCalled = new(bool) + invited = new(bool) + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasPrefix(r.URL.Path, "/users/") { + _, _ = io.WriteString(w, `{"type":"Bot","node_id":"BOT1"}`) + return + } + body, _ := io.ReadAll(r.Body) + var req struct { + Query string `json:"query"` + } + _ = json.Unmarshal(body, &req) + switch { + case strings.Contains(req.Query, "organization(login"): + _, _ = io.WriteString(w, `{"data":{"organization":{"id":"ORG"}}}`) + case strings.Contains(req.Query, "mannequins"): + _, _ = io.WriteString(w, `{"data":{"node":{"mannequins":{"pageInfo":{"endCursor":"","hasNextPage":false},"nodes":[{"id":"m1","login":"legacy-ci[bot]","claimant":null},{"id":"m2","login":"alice","claimant":null}]}}}}`) + case strings.Contains(req.Query, "user(login"): + _, _ = io.WriteString(w, `{"data":{"user":{"id":"u2"}}}`) + case strings.Contains(req.Query, "reattributeMannequinToBot"): + *botCalled = true + _, _ = io.WriteString(w, `{"data":{"reattributeMannequinToBot":{"source":{"id":"m1","login":"legacy-ci[bot]"},"target":{"id":"BOT1","login":"example-ci[bot]"}}}}`) + case strings.Contains(req.Query, "createAttributionInvitation"): + *invited = true + _, _ = io.WriteString(w, `{"data":{"createAttributionInvitation":{"source":{"id":"m2","login":"alice"},"target":{"id":"u2","login":"alice-t"}}}}`) + default: + assert.Failf(t, "unexpected query", "%s", req.Query) + } + })) + return srv, botCalled, invited +} + +// botClaimServer answers the org id, mannequins-by-login, REST bot lookup, and +// reattributeMannequinToBot calls made by a bot reclaim to target +// "example-ci[bot]". The returned bool is set to true when the bot mutation is +// invoked. +func botClaimServer(t *testing.T, mannequinLogin string) (*httptest.Server, *bool) { + const ( + mannequinID = "m1" + botLogin = "example-ci[bot]" + botNodeID = "BOT1" + ) + called := new(bool) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasPrefix(r.URL.Path, "/users/") { + _, _ = fmt.Fprintf(w, `{"type":"Bot","node_id":%q}`, botNodeID) + return + } + body, _ := io.ReadAll(r.Body) + var req struct { + Query string `json:"query"` + } + _ = json.Unmarshal(body, &req) + switch { + case strings.Contains(req.Query, "organization(login"): + _, _ = io.WriteString(w, `{"data":{"organization":{"id":"ORG"}}}`) + case strings.Contains(req.Query, "mannequins"): + _, _ = fmt.Fprintf(w, `{"data":{"node":{"mannequins":{"pageInfo":{"endCursor":"","hasNextPage":false},"nodes":[{"id":%q,"login":%q,"claimant":null}]}}}}`, mannequinID, mannequinLogin) + case strings.Contains(req.Query, "reattributeMannequinToBot"): + *called = true + _, _ = fmt.Fprintf(w, `{"data":{"reattributeMannequinToBot":{"source":{"id":%q,"login":%q},"target":{"id":%q,"login":%q}}}}`, mannequinID, mannequinLogin, botNodeID, botLogin) + default: + assert.Failf(t, "unexpected query", "%s", req.Query) + } + })) + return srv, called } diff --git a/internal/ghapi/ghapi.go b/internal/ghapi/ghapi.go index dd937f6..a0dba5e 100644 --- a/internal/ghapi/ghapi.go +++ b/internal/ghapi/ghapi.go @@ -24,11 +24,10 @@ const ( apiVersionHeader = "X-GitHub-Api-Version" apiVersion = "2022-11-28" - // graphQLFeaturesHeader opts into preview GraphQL schema features. The - // mannequin_claiming_emu feature exposes the reattributeMannequinToUser - // mutation used by `mannequin reclaim --skip-invitation`; without it the - // mutation; mannequin_claiming_bot - // exposes reattributeMannequinToBot used when reclaiming to a GitHub App / bot + // graphQLFeaturesHeader opts into preview GraphQL schema features. + // mannequin_claiming_emu exposes the reattributeMannequinToUser mutation used + // by `mannequin reclaim --skip-invitation`; mannequin_claiming_bot exposes the + // reattributeMannequinToBot mutation used when reclaiming to a GitHub App / bot // account. Without them the mutations are absent from the schema and the call // fails with "doesn't exist on type 'Mutation'". gh-gei sends this header on // every request (it is ignored by REST), so we do the same. diff --git a/internal/ghapi/ghapi_test.go b/internal/ghapi/ghapi_test.go index ba8654e..76761b9 100644 --- a/internal/ghapi/ghapi_test.go +++ b/internal/ghapi/ghapi_test.go @@ -2,7 +2,6 @@ package ghapi import ( "encoding/json" - "errors" "io" "net/http" "net/http/httptest" @@ -141,20 +140,14 @@ func TestReattributeMannequinToUser(t *testing.T) { func TestBotID(t *testing.T) { t.Run("returns the node id for a bot", func(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if !strings.HasPrefix(r.URL.Path, "/users/") { - t.Errorf("path = %q", r.URL.Path) - } + assert.True(t, strings.HasPrefix(r.URL.Path, "/users/"), "path = %q", r.URL.Path) _, _ = io.WriteString(w, `{"type":"Bot","node_id":"BOT_kgDNAbc"}`) })) defer srv.Close() id, err := NewClient(srv.URL, "tok").BotID(t.Context(), "example-ci[bot]") - if err != nil { - t.Fatalf("BotID: %v", err) - } - if id != "BOT_kgDNAbc" { - t.Errorf("id = %q", id) - } + require.NoError(t, err, "BotID") + assert.Equal(t, "BOT_kgDNAbc", id) }) t.Run("errors when the account is not a bot", func(t *testing.T) { @@ -164,9 +157,8 @@ func TestBotID(t *testing.T) { defer srv.Close() _, err := NewClient(srv.URL, "tok").BotID(t.Context(), "mona") - if err == nil || !strings.Contains(err.Error(), "not a GitHub App / bot account") { - t.Fatalf("expected not-a-bot error, got %v", err) - } + require.Error(t, err) + assert.Contains(t, err.Error(), "not a GitHub App / bot account") }) t.Run("returns ErrUserNotFound on 404", func(t *testing.T) { @@ -176,28 +168,21 @@ func TestBotID(t *testing.T) { defer srv.Close() _, err := NewClient(srv.URL, "tok").BotID(t.Context(), "ghost[bot]") - if !errors.Is(err, ErrUserNotFound) { - t.Fatalf("expected ErrUserNotFound, got %v", err) - } + require.ErrorIs(t, err, ErrUserNotFound) }) } func TestReattributeMannequinToBot(t *testing.T) { t.Run("returns the source/target pair", func(t *testing.T) { srv := graphQLServer(t, func(q string, _ map[string]any) string { - if !strings.Contains(q, "reattributeMannequinToBot") { - t.Errorf("unexpected query: %s", q) - } + assert.Contains(t, q, "reattributeMannequinToBot", "unexpected query") return `{"data":{"reattributeMannequinToBot":{"source":{"id":"m1","login":"alice"},"target":{"id":"b1","login":"example-ci[bot]"}}}}` }) defer srv.Close() res, err := NewClient(srv.URL, "tok").ReattributeMannequinToBot(t.Context(), "ORG", "m1", "b1") - if err != nil { - t.Fatalf("ReattributeMannequinToBot: %v", err) - } - if res.SourceID != "m1" || res.TargetID != "b1" { - t.Errorf("result = %+v", res) - } + require.NoError(t, err, "ReattributeMannequinToBot") + assert.Equal(t, "m1", res.SourceID) + assert.Equal(t, "b1", res.TargetID) }) } diff --git a/internal/ghapi/reclaim.go b/internal/ghapi/reclaim.go index dd678bd..661d13e 100644 --- a/internal/ghapi/reclaim.go +++ b/internal/ghapi/reclaim.go @@ -60,7 +60,7 @@ func (s *ReclaimService) ReclaimMannequin(ctx context.Context, mannequinUser, ma return fmt.Errorf("user %s is already mapped to a user; use --force to reclaim again", mannequinUser) } - isBot := isBotLogin(targetUser) + isBot := IsBotLogin(targetUser) targetUserID, err := s.resolveTargetID(ctx, targetUser, isBot) if err != nil { return err @@ -119,7 +119,7 @@ func (s *ReclaimService) ReclaimMannequins(ctx context.Context, records []Manneq continue } - isBot := isBotLogin(r.TargetUser) + isBot := IsBotLogin(r.TargetUser) claimantID, err := s.resolveTargetID(ctx, r.TargetUser, isBot) if err != nil { if errors.Is(err, ErrUserNotFound) { @@ -132,8 +132,10 @@ func (s *ReclaimService) ReclaimMannequins(ctx context.Context, records []Manneq m := Mannequin{ID: r.MannequinID, Login: r.MannequinUser} if !s.reclaimOne(ctx, orgID, m, r.TargetUser, claimantID, skipInvitation, isBot) && (skipInvitation || isBot) { - // Fail-fast for skip-invitation and bot reclaims, matching gh-gei. - return nil + // Fail-fast for skip-invitation and bot reclaims: the operation is + // irreversible, so surface an error rather than exiting 0 so automation + // can detect that the reclaim did not complete. + return errors.New("failed to reclaim mannequin") } } return nil @@ -145,14 +147,14 @@ func (s *ReclaimService) reclaimOne(ctx context.Context, orgID string, m Mannequ if isBot { result, err := s.client.ReattributeMannequinToBot(ctx, orgID, m.ID, targetUserID) if err != nil && isBotReclaimUnavailable(err) { - s.log.Warnf("Reclaiming mannequins to a GitHub App / bot account is not enabled for your GitHub organization. For more details, contact GitHub Support.") + s.log.Warnf("Reclaiming mannequins to a GitHub App / bot account is not enabled for your GitHub organization or enterprise. For more details, contact GitHub Support.") return false } - return s.handleReattribution(m, targetUser, targetUserID, result, err) + return s.handleReattribution(m, targetUser, targetUserID, result, err, true) } if skipInvitation { result, err := s.client.ReattributeMannequinToUser(ctx, orgID, m.ID, targetUserID) - return s.handleReattribution(m, targetUser, targetUserID, result, err) + return s.handleReattribution(m, targetUser, targetUserID, result, err, false) } result, err := s.client.CreateAttributionInvitation(ctx, orgID, m.ID, targetUserID) return s.handleInvitation(m, targetUser, targetUserID, result, err) @@ -167,10 +169,11 @@ func (s *ReclaimService) resolveTargetID(ctx context.Context, targetUser string, return s.client.UserID(ctx, targetUser) } -// isBotLogin reports whether a target login is a GitHub App / bot account, which -// GitHub renders with a trailing "[bot]" suffix. -func isBotLogin(login string) bool { - return strings.HasSuffix(login, "[bot]") +// IsBotLogin reports whether a target login is a GitHub App / bot account, which +// GitHub renders with a trailing "[bot]" suffix. GitHub logins are +// case-insensitive, so the suffix is matched case-insensitively. +func IsBotLogin(login string) bool { + return strings.HasSuffix(strings.ToLower(login), "[bot]") } func (s *ReclaimService) handleInvitation(m Mannequin, targetUser, targetUserID string, result *AttributionResult, err error) bool { @@ -186,19 +189,21 @@ func (s *ReclaimService) handleInvitation(m Mannequin, targetUser, targetUserID return true } -func (s *ReclaimService) handleReattribution(m Mannequin, targetUser, targetUserID string, result *AttributionResult, err error) bool { +func (s *ReclaimService) handleReattribution(m Mannequin, targetUser, targetUserID string, result *AttributionResult, err error, bot bool) bool { if err != nil { if isSkipInvitationUnavailable(err) { s.log.Warnf("Reclaiming mannequins with --skip-invitation is not enabled for your GitHub organization. For more details, contact GitHub Support.") return false } - // "Target must be a member" and similar are per-mannequin soft failures. + // "Target must be a member" and similar are per-mannequin soft failures for + // user reattribution; a bot reattribution is irreversible, so any failure is + // hard so the caller can fail-fast rather than report a false success. s.log.Warnf("Failed to reattribute content belonging to mannequin %s (%s) to %s: %v", m.Login, m.ID, targetUser, err) - return true + return !bot } if result == nil || result.SourceID != m.ID || result.TargetID != targetUserID { s.log.Warnf("Failed to reattribute content belonging to mannequin %s (%s) to %s", m.Login, m.ID, targetUser) - return true + return !bot } s.log.Successf("Successfully reclaimed content belonging to mannequin %s (%s) to %s", m.Login, m.ID, targetUser) return true diff --git a/internal/ghapi/reclaim_test.go b/internal/ghapi/reclaim_test.go index 280db15..acc1584 100644 --- a/internal/ghapi/reclaim_test.go +++ b/internal/ghapi/reclaim_test.go @@ -165,15 +165,24 @@ func TestReclaimMannequin(t *testing.T) { svc, _ := newService(f) err := svc.ReclaimMannequin(t.Context(), "alice", "", "example-ci[bot]", "octo", false, false) - if err != nil { - t.Fatalf("ReclaimMannequin: %v", err) - } - if len(f.botReattributions) != 1 || f.botReattributions[0] != "m1->b1" { - t.Errorf("botReattributions = %v", f.botReattributions) - } - if len(f.invitations) != 0 || f.reattributeAttempts != 0 { - t.Errorf("should not have used the user path: invitations=%v reattributeAttempts=%d", f.invitations, f.reattributeAttempts) + require.NoError(t, err, "ReclaimMannequin") + assert.Equal(t, []string{"m1->b1"}, f.botReattributions) + assert.Empty(t, f.invitations, "should not have used the invitation path") + assert.Zero(t, f.reattributeAttempts, "should not have used the user reattribution path") + }) + + t.Run("returns an error when the bot mutation fails", func(t *testing.T) { + f := &fakeClient{ + orgID: "ORG", + byLogin: map[string][]Mannequin{"alice": {{ID: "m1", Login: "alice"}}}, + botIDs: map[string]string{"example-ci[bot]": "b1"}, + botReattributeErr: &GraphQLError{Messages: []string{"Target must be an admin of the organization"}}, } + svc, _ := newService(f) + + err := svc.ReclaimMannequin(t.Context(), "alice", "", "example-ci[bot]", "octo", false, false) + require.Error(t, err) + assert.Empty(t, f.botReattributions, "no successful reattribution should be recorded") }) } @@ -281,9 +290,10 @@ func TestReclaimMannequins(t *testing.T) { } svc, log := newService(f) - require.NoError(t, svc.ReclaimMannequins(t.Context(), recs("alice,m1,alice-t", "bob,m2,bob-t"), "octo", false, true), "ReclaimMannequins") - // Must stop after the first row rather than treating it as a soft skip - // and continuing through the batch. + err := svc.ReclaimMannequins(t.Context(), recs("alice,m1,alice-t", "bob,m2,bob-t"), "octo", false, true) + // Must stop after the first row rather than treating it as a soft skip and + // continuing through the batch, and surface the failure to the caller. + require.Error(t, err) assert.Equal(t, 1, f.reattributeAttempts, "reattributeAttempts (fail-fast)") assert.True(t, log.contains("not enabled"), "missing unavailability warning: %v", log.lines) }) From 27b55130a025b764e27551b0f812f87a0ecf221c Mon Sep 17 00:00:00 2001 From: Daniel Perez Date: Tue, 25 Aug 2026 13:24:27 -0700 Subject: [PATCH 2/5] Address PR review comments - Quote [bot] logins in the README example so shells don't glob them - Reword the single-target reclaim prompt to reflect that it reattributes every mannequin identity matching the login, not just one - Use ghapi.IsBotLogin in the TUI so an uppercase [BOT] target is treated as the irreversible bot flow, and cover the casing in the TUI confirmation test --- README.md | 2 +- internal/cmd/target/mannequins.go | 2 +- internal/tui/model.go | 4 ++-- internal/tui/model_test.go | 22 ++++++++++++++++++++++ 4 files changed, 26 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 0321ada..de9476d 100644 --- a/README.md +++ b/README.md @@ -209,7 +209,7 @@ gh elm target mannequin reclaim octo-org --csv mannequins.csv --skip-invitation # Reclaim to a customer-owned GitHub App / bot account (target login ends in [bot]); # irreversible, so it prompts unless --no-prompt -gh elm target mannequin reclaim octo-org legacy-ci[bot] example-ci[bot] +gh elm target mannequin reclaim octo-org 'legacy-ci[bot]' 'example-ci[bot]' ``` ## Configuration diff --git a/internal/cmd/target/mannequins.go b/internal/cmd/target/mannequins.go index f2b0702..36baed1 100644 --- a/internal/cmd/target/mannequins.go +++ b/internal/cmd/target/mannequins.go @@ -354,7 +354,7 @@ func confirmBotReclaims(cmd *cobra.Command, log mannequinLogger, records []ghapi } summary += "." } else { - summary = fmt.Sprintf("You are about to reattribute mannequin %q to the GitHub App / bot account %q.", firstBot.MannequinUser, firstBot.TargetUser) + summary = fmt.Sprintf("You are about to reattribute every mannequin identity matching %q to the GitHub App / bot account %q.", firstBot.MannequinUser, firstBot.TargetUser) } if !confirm(cmd.InOrStdin(), cmd.ErrOrStderr(), diff --git a/internal/tui/model.go b/internal/tui/model.go index 197f0cc..1347f41 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -1522,7 +1522,7 @@ func (m *Model) openMannequinReclaimForm(csvMode bool) (tea.Model, tea.Cmd) { Force: values["force"] == "true", SkipInvitation: values["skip"] == "true", } - if strings.HasSuffix(input.TargetUser, "[bot]") { + if ghapi.IsBotLogin(input.TargetUser) { input.SkipInvitation = true } action := func() tea.Msg { @@ -1538,7 +1538,7 @@ func (m *Model) openMannequinReclaimForm(csvMode bool) (tea.Model, tea.Cmd) { if csvMode { confirmation = "Reclaim mannequins from this CSV? Rows targeting app[bot] accounts are reattributed immediately and cannot be undone." } - if input.SkipInvitation || strings.HasSuffix(input.TargetUser, "[bot]") { + if input.SkipInvitation || ghapi.IsBotLogin(input.TargetUser) { confirmation = "This immediately reattributes mannequin content and cannot be undone." } return func() tea.Msg { diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index edf9190..796d8aa 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -786,6 +786,28 @@ func TestModel(t *testing.T) { _, _ = model.Update(cmd()) assert.Equal(t, 1, svc.reclaimCalls) }) + + t.Run("uppercase [BOT] target still requires the irreversible confirmation", func(t *testing.T) { + svc := &fakeService{} + model := New(t.Context(), svc) + model.screen = screenMannequins + + updated, _ := model.openMannequinReclaimForm(false) + model = updated.(*Model) + model.form.fields[0].value = "octo-org" + model.form.fields[1].value = "mannequin" + model.form.fields[3].value = "app[BOT]" + model.form.cursor = len(model.form.fields) - 1 + + updated, cmd := model.Update(tea.KeyMsg{Type: tea.KeyEnter}) + model = updated.(*Model) + require.NotNil(t, cmd) + + updated, _ = model.Update(cmd()) + model = updated.(*Model) + require.Equal(t, screenConfirm, model.screen) + assert.Contains(t, model.View(), "cannot be undone") + }) } func setSourceStatus(model *Model, status string) { From e7278f616c42cb2187539051f87c3c0e29930ad7 Mon Sep 17 00:00:00 2001 From: Daniel Perez Date: Tue, 25 Aug 2026 13:55:16 -0700 Subject: [PATCH 3/5] Address PR review comments - Only enforce --skip-invitation org-admin eligibility when a non-bot row will use user reattribution, so all-bot batches aren't wrongly rejected or prompted twice (bot reclaims ignore --skip-invitation) - TUI: trim the target before choosing the reclaim path so a bot login with stray whitespace still selects the irreversible confirmation - TUI: classify the actual records (reading CSV rows) so bot reclaims surface the irreversible warning and flag likely mis-targets, matching confirmBotReclaims - Add ghapi.BotReclaimAdvisory shared by the CLI and TUI, with unit and TUI tests --- internal/cmd/target/mannequins.go | 53 ++++++++++++++----------------- internal/ghapi/reclaim.go | 21 ++++++++++++ internal/ghapi/reclaim_test.go | 30 +++++++++++++++++ internal/tui/model.go | 37 ++++++++++++++++++++- internal/tui/model_test.go | 23 ++++++++++++++ 5 files changed, 133 insertions(+), 31 deletions(-) diff --git a/internal/cmd/target/mannequins.go b/internal/cmd/target/mannequins.go index 36baed1..c61bfc9 100644 --- a/internal/cmd/target/mannequins.go +++ b/internal/cmd/target/mannequins.go @@ -220,12 +220,7 @@ func newMannequinReclaimCmd() *cobra.Command { log := mannequinLogger{w: cmd.ErrOrStderr()} svc := ghapi.NewReclaimService(client, log) - if skipInvitation { - if err := ensureSkipInvitationAllowed(cmd, client, githubOrg, noPrompt); err != nil { - return annotateMannequinAuthError(err, targetURLResolved) - } - } - + var records []ghapi.MannequinRecord if csvPath != "" { log.Infof("Reclaiming mannequins from CSV...") f, err := os.Open(csvPath) @@ -233,22 +228,33 @@ func newMannequinReclaimCmd() *cobra.Command { return fmt.Errorf("opening %s: %w", csvPath, err) } defer func() { _ = f.Close() }() - records, err := ghapi.ReadMannequinCSV(f) + records, err = ghapi.ReadMannequinCSV(f) if err != nil { return err } - if err := confirmBotReclaims(cmd, log, records, noPrompt); err != nil { - return err + } else { + log.Infof("Reclaiming mannequin...") + records = []ghapi.MannequinRecord{{MannequinUser: mannequinUser, TargetUser: targetUser}} + } + + if skipInvitation { + if _, userCount, _ := ghapi.BotReclaimAdvisory(records); userCount > 0 { + if err := ensureSkipInvitationAllowed(cmd, client, githubOrg, noPrompt); err != nil { + return annotateMannequinAuthError(err, targetURLResolved) + } } + } + + if err := confirmBotReclaims(cmd, log, records, noPrompt); err != nil { + return err + } + + if csvPath != "" { if err := svc.ReclaimMannequins(cmd.Context(), records, githubOrg, force, skipInvitation); err != nil { return annotateMannequinAuthError(err, targetURLResolved) } return nil } - log.Infof("Reclaiming mannequin...") - if err := confirmBotReclaims(cmd, log, []ghapi.MannequinRecord{{MannequinUser: mannequinUser, TargetUser: targetUser}}, noPrompt); err != nil { - return err - } if err := svc.ReclaimMannequin(cmd.Context(), mannequinUser, mannequinID, targetUser, githubOrg, force, skipInvitation); err != nil { return annotateMannequinAuthError(err, targetURLResolved) } @@ -324,22 +330,9 @@ func confirm(in io.Reader, out io.Writer, prompt string) bool { // mis-target (a human's content going to a bot), but the convention is // GitHub-specific, so we warn and let the admin proceed rather than blocking. func confirmBotReclaims(cmd *cobra.Command, log mannequinLogger, records []ghapi.MannequinRecord, noPrompt bool) error { - var botCount, humanCount int - warned := make(map[string]bool) - var firstBot ghapi.MannequinRecord - for _, r := range records { - if !ghapi.IsBotLogin(r.TargetUser) { - humanCount++ - continue - } - if botCount == 0 { - firstBot = r - } - botCount++ - if !ghapi.IsBotLogin(r.MannequinUser) && !warned[r.MannequinUser] { - warned[r.MannequinUser] = true - log.Warnf("%q does not look like a bot mannequin (its login does not end in %q). Are you sure you want to do this?", r.MannequinUser, "[bot]") - } + botCount, humanCount, mistargets := ghapi.BotReclaimAdvisory(records) + for _, src := range mistargets { + log.Warnf("%q does not look like a bot mannequin (its login does not end in %q). Are you sure you want to do this?", src, "[bot]") } if botCount == 0 || noPrompt { @@ -354,7 +347,7 @@ func confirmBotReclaims(cmd *cobra.Command, log mannequinLogger, records []ghapi } summary += "." } else { - summary = fmt.Sprintf("You are about to reattribute every mannequin identity matching %q to the GitHub App / bot account %q.", firstBot.MannequinUser, firstBot.TargetUser) + summary = fmt.Sprintf("You are about to reattribute every mannequin identity matching %q to the GitHub App / bot account %q.", records[0].MannequinUser, records[0].TargetUser) } if !confirm(cmd.InOrStdin(), cmd.ErrOrStderr(), diff --git a/internal/ghapi/reclaim.go b/internal/ghapi/reclaim.go index 661d13e..b361656 100644 --- a/internal/ghapi/reclaim.go +++ b/internal/ghapi/reclaim.go @@ -176,6 +176,27 @@ func IsBotLogin(login string) bool { return strings.HasSuffix(strings.ToLower(login), "[bot]") } +// BotReclaimAdvisory classifies reclaim records by target type. It returns the +// number of records targeting a bot, the number targeting a user, and the +// distinct source logins that target a bot but do not themselves look like a +// bot (likely mis-targets worth warning about). Target logins are trimmed +// before classification so surrounding whitespace does not change the result. +func BotReclaimAdvisory(records []MannequinRecord) (botCount, userCount int, mistargetSources []string) { + seen := make(map[string]bool) + for _, r := range records { + if !IsBotLogin(strings.TrimSpace(r.TargetUser)) { + userCount++ + continue + } + botCount++ + if !IsBotLogin(r.MannequinUser) && !seen[r.MannequinUser] { + seen[r.MannequinUser] = true + mistargetSources = append(mistargetSources, r.MannequinUser) + } + } + return botCount, userCount, mistargetSources +} + func (s *ReclaimService) handleInvitation(m Mannequin, targetUser, targetUserID string, result *AttributionResult, err error) bool { if err != nil { s.log.Warnf("Failed to send reclaim invitation email to %s for mannequin %s (%s): %v", targetUser, m.Login, m.ID, err) diff --git a/internal/ghapi/reclaim_test.go b/internal/ghapi/reclaim_test.go index acc1584..393e0b3 100644 --- a/internal/ghapi/reclaim_test.go +++ b/internal/ghapi/reclaim_test.go @@ -299,6 +299,36 @@ func TestReclaimMannequins(t *testing.T) { }) } +func TestBotReclaimAdvisory(t *testing.T) { + t.Run("classifies bot and user targets and flags mis-targets", func(t *testing.T) { + botCount, userCount, mistargets := BotReclaimAdvisory([]MannequinRecord{ + {MannequinUser: "legacy-ci[bot]", TargetUser: "example-ci[bot]"}, + {MannequinUser: "human", TargetUser: "app[bot]"}, + {MannequinUser: "alice", TargetUser: "alice-t"}, + }) + assert.Equal(t, 2, botCount) + assert.Equal(t, 1, userCount) + assert.Equal(t, []string{"human"}, mistargets) + }) + + t.Run("trims the target and matches casing", func(t *testing.T) { + botCount, userCount, mistargets := BotReclaimAdvisory([]MannequinRecord{ + {MannequinUser: "human", TargetUser: "app[BOT] "}, + }) + assert.Equal(t, 1, botCount) + assert.Zero(t, userCount) + assert.Equal(t, []string{"human"}, mistargets) + }) + + t.Run("deduplicates mis-target source logins", func(t *testing.T) { + _, _, mistargets := BotReclaimAdvisory([]MannequinRecord{ + {MannequinUser: "human", TargetUser: "a[bot]"}, + {MannequinUser: "human", TargetUser: "b[bot]"}, + }) + assert.Equal(t, []string{"human"}, mistargets) + }) +} + func TestIsSkipInvitationUnavailable(t *testing.T) { t.Run("missing mutation", func(t *testing.T) { err := &GraphQLError{Messages: []string{"Field 'reattributeMannequinToUser' doesn't exist on type 'Mutation'"}} diff --git a/internal/tui/model.go b/internal/tui/model.go index 1347f41..10d4603 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "os" "strconv" "strings" "time" @@ -1522,6 +1523,10 @@ func (m *Model) openMannequinReclaimForm(csvMode bool) (tea.Model, tea.Cmd) { Force: values["force"] == "true", SkipInvitation: values["skip"] == "true", } + // Match workflow.Service, which trims the target before deciding the + // reclaim path, so a bot login with stray whitespace still selects the + // irreversible confirmation. + input.TargetUser = strings.TrimSpace(input.TargetUser) if ghapi.IsBotLogin(input.TargetUser) { input.SkipInvitation = true } @@ -1534,13 +1539,27 @@ func (m *Model) openMannequinReclaimForm(csvMode bool) (tea.Model, tea.Cmd) { } return actionMsg{title: "Mannequin reclaim", body: body, parent: screenMannequins, err: err} } + // Mirror the CLI's confirmBotReclaims: classify the actual records so + // bot reclaims surface the irreversible warning and flag likely + // mis-targets (a bot target whose source login is not itself a bot). + var records []ghapi.MannequinRecord + if csvMode { + records, _ = readReclaimCSV(input.CSVPath) + } else { + records = []ghapi.MannequinRecord{{MannequinUser: strings.TrimSpace(input.Mannequin), TargetUser: input.TargetUser}} + } + botCount, _, mistargets := ghapi.BotReclaimAdvisory(records) + confirmation := "Send reclaim invitations for the selected mannequin data?" if csvMode { confirmation = "Reclaim mannequins from this CSV? Rows targeting app[bot] accounts are reattributed immediately and cannot be undone." } - if input.SkipInvitation || ghapi.IsBotLogin(input.TargetUser) { + if input.SkipInvitation || botCount > 0 { confirmation = "This immediately reattributes mannequin content and cannot be undone." } + for _, src := range mistargets { + confirmation += fmt.Sprintf("\nWarning: %q does not look like a bot mannequin (its login does not end in \"[bot]\"). Are you sure you want to do this?", src) + } return func() tea.Msg { return confirmRequestMsg{ title: "Confirm mannequin reclaim", @@ -1553,6 +1572,22 @@ func (m *Model) openMannequinReclaimForm(csvMode bool) (tea.Model, tea.Cmd) { }) } +// readReclaimCSV reads mannequin reclaim records from the CSV at path so the TUI +// can classify bot targets before confirming. An empty path or read error +// yields no records, letting the confirmation fall back to its generic copy. +func readReclaimCSV(path string) ([]ghapi.MannequinRecord, error) { + trimmed := strings.TrimSpace(path) + if trimmed == "" { + return nil, nil + } + file, err := os.Open(trimmed) + if err != nil { + return nil, err + } + defer func() { _ = file.Close() }() + return ghapi.ReadMannequinCSV(file) +} + func (m *Model) openConfigurationForm() (tea.Model, tea.Cmd) { sourceURL, targetURL := "", "" if m.configuration != nil { diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index 796d8aa..cdf27dd 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -808,6 +808,29 @@ func TestModel(t *testing.T) { require.Equal(t, screenConfirm, model.screen) assert.Contains(t, model.View(), "cannot be undone") }) + + t.Run("bot target with whitespace and a non-bot source surfaces the advisory", func(t *testing.T) { + svc := &fakeService{} + model := New(t.Context(), svc) + model.screen = screenMannequins + + updated, _ := model.openMannequinReclaimForm(false) + model = updated.(*Model) + model.form.fields[0].value = "octo-org" + model.form.fields[1].value = "human-mannequin" + model.form.fields[3].value = "app[bot] " + model.form.cursor = len(model.form.fields) - 1 + + updated, cmd := model.Update(tea.KeyMsg{Type: tea.KeyEnter}) + model = updated.(*Model) + require.NotNil(t, cmd) + + updated, _ = model.Update(cmd()) + model = updated.(*Model) + require.Equal(t, screenConfirm, model.screen) + assert.Contains(t, model.confirm.body, "cannot be undone") + assert.Contains(t, model.confirm.body, "does not look like a bot mannequin") + }) } func setSourceStatus(model *Model, status string) { From 70270cab39a68794f73741271d497fd25f86937b Mon Sep 17 00:00:00 2001 From: Daniel Perez Date: Wed, 26 Aug 2026 06:05:39 -0700 Subject: [PATCH 4/5] Ignore empty targets when classifying bot reclaims BotReclaimAdvisory counted rows with a blank target as user reclaims, but ReclaimMannequins skips those rows (e.g. unedited rows exported by `mannequin list`). That mis-stated the confirmation summary and, with --skip-invitation, ran the org-admin-only preflight for an otherwise bot-only batch. Skip records whose trimmed target is empty before classifying. --- internal/ghapi/reclaim.go | 10 ++++++++-- internal/ghapi/reclaim_test.go | 11 +++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/internal/ghapi/reclaim.go b/internal/ghapi/reclaim.go index b361656..f793174 100644 --- a/internal/ghapi/reclaim.go +++ b/internal/ghapi/reclaim.go @@ -180,11 +180,17 @@ func IsBotLogin(login string) bool { // number of records targeting a bot, the number targeting a user, and the // distinct source logins that target a bot but do not themselves look like a // bot (likely mis-targets worth warning about). Target logins are trimmed -// before classification so surrounding whitespace does not change the result. +// before classification so surrounding whitespace does not change the result; +// records with an empty target are ignored, mirroring ReclaimMannequins, which +// skips them (e.g. unedited rows exported by `mannequin list`). func BotReclaimAdvisory(records []MannequinRecord) (botCount, userCount int, mistargetSources []string) { seen := make(map[string]bool) for _, r := range records { - if !IsBotLogin(strings.TrimSpace(r.TargetUser)) { + target := strings.TrimSpace(r.TargetUser) + if target == "" { + continue + } + if !IsBotLogin(target) { userCount++ continue } diff --git a/internal/ghapi/reclaim_test.go b/internal/ghapi/reclaim_test.go index 393e0b3..3199b27 100644 --- a/internal/ghapi/reclaim_test.go +++ b/internal/ghapi/reclaim_test.go @@ -327,6 +327,17 @@ func TestBotReclaimAdvisory(t *testing.T) { }) assert.Equal(t, []string{"human"}, mistargets) }) + + t.Run("ignores rows with an empty target", func(t *testing.T) { + botCount, userCount, mistargets := BotReclaimAdvisory([]MannequinRecord{ + {MannequinUser: "alice", TargetUser: ""}, + {MannequinUser: "bob", TargetUser: " "}, + {MannequinUser: "legacy-ci[bot]", TargetUser: "example-ci[bot]"}, + }) + assert.Equal(t, 1, botCount) + assert.Zero(t, userCount) + assert.Empty(t, mistargets) + }) } func TestIsSkipInvitationUnavailable(t *testing.T) { From dbea1796d0d04fbdb89718953dd597026ca2d42a Mon Sep 17 00:00:00 2001 From: Daniel Perez Date: Mon, 31 Aug 2026 09:51:16 -0700 Subject: [PATCH 5/5] Dispatch GraphQL test mocks on operationName Name each GraphQL query/mutation and send operationName so test mocks can switch on the operation instead of matching substrings of the query text, making them resilient to query formatting changes and explicit about which operations they support. Addresses review feedback from @iomekam. --- internal/cmd/target/mannequins_test.go | 34 ++++++++++++-------------- internal/ghapi/ghapi.go | 21 +++++++++++++--- internal/ghapi/ghapi_test.go | 14 +++++++++++ internal/ghapi/mannequins.go | 16 ++++++------ 4 files changed, 56 insertions(+), 29 deletions(-) diff --git a/internal/cmd/target/mannequins_test.go b/internal/cmd/target/mannequins_test.go index 7a081f0..5a19fbb 100644 --- a/internal/cmd/target/mannequins_test.go +++ b/internal/cmd/target/mannequins_test.go @@ -293,26 +293,25 @@ func botCSVServer(t *testing.T) (srv *httptest.Server, botCalled, invited *bool) _, _ = io.WriteString(w, `{"type":"Bot","node_id":"BOT1"}`) return } - body, _ := io.ReadAll(r.Body) var req struct { - Query string `json:"query"` + OperationName string `json:"operationName"` } - _ = json.Unmarshal(body, &req) - switch { - case strings.Contains(req.Query, "organization(login"): + require.NoError(t, json.NewDecoder(r.Body).Decode(&req)) + switch req.OperationName { + case "GetOrganization": _, _ = io.WriteString(w, `{"data":{"organization":{"id":"ORG"}}}`) - case strings.Contains(req.Query, "mannequins"): + case "ListMannequins": _, _ = io.WriteString(w, `{"data":{"node":{"mannequins":{"pageInfo":{"endCursor":"","hasNextPage":false},"nodes":[{"id":"m1","login":"legacy-ci[bot]","claimant":null},{"id":"m2","login":"alice","claimant":null}]}}}}`) - case strings.Contains(req.Query, "user(login"): + case "GetUser": _, _ = io.WriteString(w, `{"data":{"user":{"id":"u2"}}}`) - case strings.Contains(req.Query, "reattributeMannequinToBot"): + case "ReattributeMannequinToBot": *botCalled = true _, _ = io.WriteString(w, `{"data":{"reattributeMannequinToBot":{"source":{"id":"m1","login":"legacy-ci[bot]"},"target":{"id":"BOT1","login":"example-ci[bot]"}}}}`) - case strings.Contains(req.Query, "createAttributionInvitation"): + case "CreateAttributionInvitation": *invited = true _, _ = io.WriteString(w, `{"data":{"createAttributionInvitation":{"source":{"id":"m2","login":"alice"},"target":{"id":"u2","login":"alice-t"}}}}`) default: - assert.Failf(t, "unexpected query", "%s", req.Query) + require.Failf(t, "unexpected GraphQL operation", "operation: %q", req.OperationName) } })) return srv, botCalled, invited @@ -334,21 +333,20 @@ func botClaimServer(t *testing.T, mannequinLogin string) (*httptest.Server, *boo _, _ = fmt.Fprintf(w, `{"type":"Bot","node_id":%q}`, botNodeID) return } - body, _ := io.ReadAll(r.Body) var req struct { - Query string `json:"query"` + OperationName string `json:"operationName"` } - _ = json.Unmarshal(body, &req) - switch { - case strings.Contains(req.Query, "organization(login"): + require.NoError(t, json.NewDecoder(r.Body).Decode(&req)) + switch req.OperationName { + case "GetOrganization": _, _ = io.WriteString(w, `{"data":{"organization":{"id":"ORG"}}}`) - case strings.Contains(req.Query, "mannequins"): + case "ListMannequinsByLogin": _, _ = fmt.Fprintf(w, `{"data":{"node":{"mannequins":{"pageInfo":{"endCursor":"","hasNextPage":false},"nodes":[{"id":%q,"login":%q,"claimant":null}]}}}}`, mannequinID, mannequinLogin) - case strings.Contains(req.Query, "reattributeMannequinToBot"): + case "ReattributeMannequinToBot": *called = true _, _ = fmt.Fprintf(w, `{"data":{"reattributeMannequinToBot":{"source":{"id":%q,"login":%q},"target":{"id":%q,"login":%q}}}}`, mannequinID, mannequinLogin, botNodeID, botLogin) default: - assert.Failf(t, "unexpected query", "%s", req.Query) + require.Failf(t, "unexpected GraphQL operation", "operation: %q", req.OperationName) } })) return srv, called diff --git a/internal/ghapi/ghapi.go b/internal/ghapi/ghapi.go index a0dba5e..67ce87f 100644 --- a/internal/ghapi/ghapi.go +++ b/internal/ghapi/ghapi.go @@ -12,6 +12,7 @@ import ( "fmt" "io" "net/http" + "regexp" "slices" "strings" "time" @@ -101,8 +102,22 @@ func (e *GraphQLError) Error() string { // graphQLRequest is the JSON body of a GraphQL POST. type graphQLRequest struct { - Query string `json:"query"` - Variables any `json:"variables,omitempty"` + Query string `json:"query"` + OperationName string `json:"operationName,omitempty"` + Variables any `json:"variables,omitempty"` +} + +// graphQLOperationNameRE captures the name of a named query or mutation. +var graphQLOperationNameRE = regexp.MustCompile(`(?m)^\s*(?:query|mutation)\s+([A-Za-z_]\w*)`) + +// graphQLOperationName returns the operation name of a named query or mutation, +// or "" for an anonymous operation. Sending operationName lets servers (and test +// mocks) dispatch on the operation rather than the query text. +func graphQLOperationName(query string) string { + if m := graphQLOperationNameRE.FindStringSubmatch(query); m != nil { + return m[1] + } + return "" } // graphQLResponse captures the parts of a GraphQL response gh-elm inspects. @@ -116,7 +131,7 @@ type graphQLResponse struct { // graphQL issues a GraphQL query/mutation and decodes response.data into out. // A non-empty errors array is returned as a *GraphQLError. func (c *Client) graphQL(ctx context.Context, query string, variables, out any) error { - body, err := json.Marshal(graphQLRequest{Query: query, Variables: variables}) + body, err := json.Marshal(graphQLRequest{Query: query, OperationName: graphQLOperationName(query), Variables: variables}) if err != nil { return fmt.Errorf("encoding graphql request: %w", err) } diff --git a/internal/ghapi/ghapi_test.go b/internal/ghapi/ghapi_test.go index 76761b9..47b6977 100644 --- a/internal/ghapi/ghapi_test.go +++ b/internal/ghapi/ghapi_test.go @@ -137,6 +137,20 @@ func TestReattributeMannequinToUser(t *testing.T) { }) } +func TestGraphQLOperationName(t *testing.T) { + t.Run("named query", func(t *testing.T) { + assert.Equal(t, "GetUser", graphQLOperationName("query GetUser($login: String!) { user(login: $login) { id } }")) + }) + + t.Run("named mutation with leading whitespace", func(t *testing.T) { + assert.Equal(t, "ReattributeMannequinToBot", graphQLOperationName("\n mutation ReattributeMannequinToBot($orgId: ID!) { x }")) + }) + + t.Run("anonymous operation", func(t *testing.T) { + assert.Empty(t, graphQLOperationName("query { viewer { login } }")) + }) +} + func TestBotID(t *testing.T) { t.Run("returns the node id for a bot", func(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/ghapi/mannequins.go b/internal/ghapi/mannequins.go index c75fb0b..4c55a60 100644 --- a/internal/ghapi/mannequins.go +++ b/internal/ghapi/mannequins.go @@ -58,7 +58,7 @@ type mannequinsData struct { } `json:"node"` } -const mannequinsQuery = `query($id: ID!, $first: Int, $after: String) { +const mannequinsQuery = `query ListMannequins($id: ID!, $first: Int, $after: String) { node(id: $id) { ... on Organization { mannequins(first: $first, after: $after) { @@ -69,7 +69,7 @@ const mannequinsQuery = `query($id: ID!, $first: Int, $after: String) { } }` -const mannequinsByLoginQuery = `query($id: ID!, $first: Int, $after: String, $login: String) { +const mannequinsByLoginQuery = `query ListMannequinsByLogin($id: ID!, $first: Int, $after: String, $login: String) { node(id: $id) { ... on Organization { mannequins(first: $first, after: $after, login: $login) { @@ -88,7 +88,7 @@ func (c *Client) OrganizationID(ctx context.Context, org string) (string, error) } `json:"organization"` } vars := map[string]any{"login": org} - if err := c.graphQL(ctx, `query($login: String!) { organization(login: $login) { login id name } }`, vars, &data); err != nil { + if err := c.graphQL(ctx, `query GetOrganization($login: String!) { organization(login: $login) { login id name } }`, vars, &data); err != nil { return "", fmt.Errorf("looking up organization ID for %q: %w", org, err) } if data.Organization.ID == "" { @@ -105,7 +105,7 @@ func (c *Client) UserID(ctx context.Context, login string) (string, error) { } `json:"user"` } vars := map[string]any{"login": login} - if err := c.graphQL(ctx, `query($login: String!) { user(login: $login) { id name } }`, vars, &data); err != nil { + if err := c.graphQL(ctx, `query GetUser($login: String!) { user(login: $login) { id name } }`, vars, &data); err != nil { if isUserNotFound(err) { return "", fmt.Errorf("user %q not found: %w", login, ErrUserNotFound) } @@ -163,7 +163,7 @@ func (c *Client) LoginName(ctx context.Context) (string, error) { Login string `json:"login"` } `json:"viewer"` } - if err := c.graphQL(ctx, `query { viewer { login } }`, nil, &data); err != nil { + if err := c.graphQL(ctx, `query GetViewer { viewer { login } }`, nil, &data); err != nil { return "", fmt.Errorf("looking up the current user's login: %w", err) } return data.Viewer.Login, nil @@ -226,21 +226,21 @@ func (c *Client) fetchMannequins(ctx context.Context, query string, vars map[str } } -const createAttributionInvitationMutation = `mutation($orgId: ID!, $sourceId: ID!, $targetId: ID!) { +const createAttributionInvitationMutation = `mutation CreateAttributionInvitation($orgId: ID!, $sourceId: ID!, $targetId: ID!) { createAttributionInvitation(input: { ownerId: $orgId, sourceId: $sourceId, targetId: $targetId }) { source { ... on Mannequin { id login } } target { ... on User { id login } } } }` -const reattributeMannequinToUserMutation = `mutation($orgId: ID!, $sourceId: ID!, $targetId: ID!) { +const reattributeMannequinToUserMutation = `mutation ReattributeMannequinToUser($orgId: ID!, $sourceId: ID!, $targetId: ID!) { reattributeMannequinToUser(input: { ownerId: $orgId, sourceId: $sourceId, targetId: $targetId }) { source { ... on Mannequin { id login } } target { ... on User { id login } } } }` -const reattributeMannequinToBotMutation = `mutation($orgId: ID!, $sourceId: ID!, $targetId: ID!) { +const reattributeMannequinToBotMutation = `mutation ReattributeMannequinToBot($orgId: ID!, $sourceId: ID!, $targetId: ID!) { reattributeMannequinToBot(input: { ownerId: $orgId, sourceId: $sourceId, targetId: $targetId }) { source { ... on Mannequin { id login } } target { ... on Bot { id login } }