diff --git a/README.md b/README.md index 8685c10..de9476d 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..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,17 +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 } + } 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 := svc.ReclaimMannequin(cmd.Context(), mannequinUser, mannequinID, targetUser, githubOrg, force, skipInvitation); err != nil { return annotateMannequinAuthError(err, targetURLResolved) } @@ -259,7 +270,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 +323,40 @@ 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 { + 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 { + 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 every mannequin identity matching %q to the GitHub App / bot account %q.", records[0].MannequinUser, records[0].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..5a19fbb 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,140 @@ 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 + } + var req struct { + OperationName string `json:"operationName"` + } + require.NoError(t, json.NewDecoder(r.Body).Decode(&req)) + switch req.OperationName { + case "GetOrganization": + _, _ = io.WriteString(w, `{"data":{"organization":{"id":"ORG"}}}`) + 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 "GetUser": + _, _ = io.WriteString(w, `{"data":{"user":{"id":"u2"}}}`) + case "ReattributeMannequinToBot": + *botCalled = true + _, _ = io.WriteString(w, `{"data":{"reattributeMannequinToBot":{"source":{"id":"m1","login":"legacy-ci[bot]"},"target":{"id":"BOT1","login":"example-ci[bot]"}}}}`) + case "CreateAttributionInvitation": + *invited = true + _, _ = io.WriteString(w, `{"data":{"createAttributionInvitation":{"source":{"id":"m2","login":"alice"},"target":{"id":"u2","login":"alice-t"}}}}`) + default: + require.Failf(t, "unexpected GraphQL operation", "operation: %q", req.OperationName) + } + })) + 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 + } + var req struct { + OperationName string `json:"operationName"` + } + require.NoError(t, json.NewDecoder(r.Body).Decode(&req)) + switch req.OperationName { + case "GetOrganization": + _, _ = io.WriteString(w, `{"data":{"organization":{"id":"ORG"}}}`) + case "ListMannequinsByLogin": + _, _ = fmt.Fprintf(w, `{"data":{"node":{"mannequins":{"pageInfo":{"endCursor":"","hasNextPage":false},"nodes":[{"id":%q,"login":%q,"claimant":null}]}}}}`, mannequinID, mannequinLogin) + case "ReattributeMannequinToBot": + *called = true + _, _ = fmt.Fprintf(w, `{"data":{"reattributeMannequinToBot":{"source":{"id":%q,"login":%q},"target":{"id":%q,"login":%q}}}}`, mannequinID, mannequinLogin, botNodeID, botLogin) + default: + 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 dd937f6..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" @@ -24,11 +25,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. @@ -102,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. @@ -117,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 ba8654e..47b6977 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" @@ -138,23 +137,31 @@ 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) { - 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 +171,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 +182,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/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 } } diff --git a/internal/ghapi/reclaim.go b/internal/ghapi/reclaim.go index dd678bd..f793174 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,38 @@ 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]") +} + +// 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; +// 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 { + target := strings.TrimSpace(r.TargetUser) + if target == "" { + continue + } + if !IsBotLogin(target) { + 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 { @@ -186,19 +216,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..3199b27 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,14 +290,56 @@ 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) }) } +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) + }) + + 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) { 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 197f0cc..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,7 +1523,11 @@ func (m *Model) openMannequinReclaimForm(csvMode bool) (tea.Model, tea.Cmd) { Force: values["force"] == "true", SkipInvitation: values["skip"] == "true", } - if strings.HasSuffix(input.TargetUser, "[bot]") { + // 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 } action := func() tea.Msg { @@ -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 || strings.HasSuffix(input.TargetUser, "[bot]") { + 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 edf9190..cdf27dd 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -786,6 +786,51 @@ 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") + }) + + 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) {