fix(bitbucket): better handling of bot-initiated events

pkg/bitbucket/pull_requests.go

@@ -30,7 +30,7 @@ func InitPRData(ctx workflow.Context, event PullRequestEvent, prChannelID, slack
 			slog.Any("error", err), slog.String("pr_url", prURL))
 	}
 
-	email := users.BitbucketIDToEmail(ctx, event.Actor.AccountID)
+	email := users.BitbucketActorToEmail(ctx, event.Actor)
 	if email == "" {
 		logger.From(ctx).Error("initializing Bitbucket PR data without author's email",
 			slog.String("pr_url", prURL), slog.String("account_id", event.Actor.AccountID))
@@ -42,8 +42,8 @@ func InitPRData(ctx workflow.Context, event PullRequestEvent, prChannelID, slack
 	data.InitTurns(ctx, prURL, email)
 }
 
-// accountIDs extracts the IDs from a slice of [Account]s.
-// The output is guaranteed to be sorted, without repetitions, and not contain teams/apps.
+// accountIDs extracts the IDs from a slice of [Account]s. The output is guaranteed
+// to be sorted, without repetitions, and not contain apps/bots or teams.
 func accountIDs(accounts []Account) []string {
 	if len(accounts) == 0 {
 		return nil

pkg/bitbucket/pull_requests_test.go

@@ -45,7 +45,7 @@ func TestAccountIDs(t *testing.T) {
 			accounts: []Account{
 				{AccountID: "aaa", Type: "user"},
 				{AccountID: "team1", Type: "team"},
-				{AccountID: "app1", Type: "app"},
+				{AccountID: "app1", Type: "app_user"},
 				{AccountID: "bbb", Type: ""},
 			},
 			want: []string{"aaa", "bbb"},

pkg/bitbucket/workflows/comment.go

@@ -36,8 +36,7 @@ func (c Config) CommentCreatedWorkflow(ctx workflow.Context, event bitbucket.Pul
 	defer bitbucket.UpdateChannelBookmarks(ctx, event.PullRequest, prURL, channelID)
 
 	// Don't abort if this fails - it's more important to post the comment.
-	email := users.BitbucketIDToEmail(ctx, event.Actor.AccountID)
-	_ = data.SwitchTurn(ctx, prURL, email, false)
+	_ = data.SwitchTurn(ctx, prURL, users.BitbucketActorToEmail(ctx, event.Actor), false)
 
 	// If the comment was created by RevChat, i.e. mirrored from Slack, don't repost it.
 	// Also, don't poll Bitbucket for updates because we expect them to come from Slack.
@@ -168,7 +167,7 @@ func CommentResolvedWorkflow(ctx workflow.Context, event bitbucket.PullRequestEv
 		return nil
 	}
 
-	data.UpdateActivityTime(ctx, prURL, data.BitbucketIDToEmail(ctx, event.Actor.AccountID))
+	data.UpdateActivityTime(ctx, prURL, users.BitbucketActorToEmail(ctx, event.Actor))
 	defer bitbucket.UpdateChannelBookmarks(ctx, event.PullRequest, prURL, channelID)
 
 	url := bitbucket.HTMLURL(event.Comment.Links)
@@ -187,7 +186,7 @@ func CommentReopenedWorkflow(ctx workflow.Context, event bitbucket.PullRequestEv
 		return nil
 	}
 
-	data.UpdateActivityTime(ctx, prURL, data.BitbucketIDToEmail(ctx, event.Actor.AccountID))
+	data.UpdateActivityTime(ctx, prURL, users.BitbucketActorToEmail(ctx, event.Actor))
 	defer bitbucket.UpdateChannelBookmarks(ctx, event.PullRequest, prURL, channelID)
 
 	url := bitbucket.HTMLURL(event.Comment.Links)

pkg/bitbucket/workflows/pullrequest.go

@@ -129,13 +129,13 @@ func (c Config) PullRequestUpdatedWorkflow(ctx workflow.Context, event bitbucket
 
 	defer bitbucket.UpdateChannelBookmarks(ctx, pr, prURL, channelID)
 
-	email := data.BitbucketIDToEmail(ctx, event.Actor.AccountID)
+	email := users.BitbucketActorToEmail(ctx, event.Actor)
 	var errs []error
 
 	// Announce transitions between draft and ready-to-review modes.
 	if !snapshot.Draft && pr.Draft {
 		bitbucket.MentionUserInMsg(ctx, channelID, event.Actor, "%s marked this PR as a draft. :construction:")
-		_, _, _ = data.Nudge(ctx, prURL, data.BitbucketIDToEmail(ctx, event.PullRequest.Author.AccountID))
+		_, _, _ = data.Nudge(ctx, prURL, users.BitbucketActorToEmail(ctx, event.PullRequest.Author))
 	} else if snapshot.Draft && !pr.Draft {
 		bitbucket.MentionUserInMsg(ctx, channelID, event.Actor, "%s marked this PR as ready for review. :eyes:")
 		_ = data.SwitchTurn(ctx, prURL, email, true)
@@ -240,7 +240,7 @@ func PullRequestReviewedWorkflow(ctx workflow.Context, event bitbucket.PullReque
 
 	defer bitbucket.UpdateChannelBookmarks(ctx, event.PullRequest, prURL, channelID)
 
-	email := users.BitbucketIDToEmail(ctx, event.Actor.AccountID)
+	email := users.BitbucketActorToEmail(ctx, event.Actor)
 	msg := "%s "
 	var err error
 

pkg/data/turns.go

@@ -427,20 +427,20 @@ func readTurnsFile(ctx workflow.Context, url string) (*PRTurns, error) {
 	f, err := os.Open(path) //gosec:disable G304 // URL received from signature-verified 3rd-party, suffix is hardcoded.
 	if err != nil {
 		if errors.Is(err, os.ErrNotExist) {
-			return resetTurns(ctx, url)
+			return resetTurns(ctx, url, nil)
 		}
 		return nil, err
 	}
 	defer f.Close()
 
 	t := new(PRTurns)
 	if err := json.NewDecoder(f).Decode(&t); err != nil {
-		return resetTurns(ctx, url)
+		return resetTurns(ctx, url, nil)
 	}
 
 	// Data sanity checks.
 	if t.Author == "" {
-		return resetTurns(ctx, url)
+		return resetTurns(ctx, url, t)
 	}
 	normalizeEmailAddresses(t)
 	if t.Reviewers == nil {
@@ -513,51 +513,67 @@ func writeTurnsFileActivity(_ context.Context, url string, t *PRTurns) error {
 
 // resetTurns recreates the attention state file for a specific PR, based on the current
 // PR snapshot. This is a fallback for when the turn file is missing or corrupted.
-func resetTurns(ctx workflow.Context, url string) (*PRTurns, error) {
+func resetTurns(ctx workflow.Context, url string, t *PRTurns) (*PRTurns, error) {
 	logger.From(ctx).Warn("resetting PR attention state file", slog.String("pr_url", url))
 
 	pr, err := LoadPRSnapshot(ctx, url)
 	if err != nil {
 		return nil, err
 	}
 
-	author := userEmail(ctx, pr["author"])
+	author, accountType := userEmailAndType(ctx, pr["author"])
+	if author == "" && accountType == "app_user" {
+		author = "bot"
+	}
+
+	// If the only thing that needs fixing is the author's email, do just that.
+	if t != nil && len(t.Reviewers)+len(t.Activity)+len(t.Approvers) > 0 {
+		t.Author = author
+		return t, writeTurnsFile(ctx, url, t)
+	}
 
+	// Otherwise, recreate the entire struct and file.
 	reviewers := map[string]bool{}
 	jsonList, ok := pr["reviewers"].([]any)
 	if !ok {
 		jsonList = []any{}
 	}
 	for _, r := range jsonList {
-		if email := userEmail(ctx, r); email != "" {
+		if email, _ := userEmailAndType(ctx, r); email != "" {
 			reviewers[email] = true
 		}
 	}
 
-	t := &PRTurns{Author: author, Reviewers: reviewers, Activity: map[string]time.Time{}, Approvers: map[string]time.Time{}}
+	approvers := map[string]time.Time{}
+
+	t = &PRTurns{Author: author, Reviewers: reviewers, Activity: map[string]time.Time{}, Approvers: approvers}
 	return t, writeTurnsFile(ctx, url, t)
 }
 
-// userEmail extracts the Bitbucket account ID from user details map, and converts
-// it into the user's email address, based on RevChat's own user database.
-func userEmail(ctx workflow.Context, detailsMap any) string {
+// userEmailAndType extracts the Bitbucket account ID from user details map, and converts
+// it into the user's email address and account type, based on RevChat's own user database.
+func userEmailAndType(ctx workflow.Context, detailsMap any) (email, accountType string) {
 	user, ok := detailsMap.(map[string]any)
 	if !ok {
-		return ""
+		return "", ""
 	}
 
 	accountID, ok := user["account_id"].(string)
 	if !ok {
-		return ""
+		return "", ""
+	}
+	accountType, ok = user["type"].(string)
+	if !ok {
+		accountType = ""
 	}
 
-	return BitbucketIDToEmail(ctx, accountID)
+	return BitbucketIDToEmail(ctx, accountID, accountType), accountType
 }
 
 // BitbucketIDToEmail converts a Bitbucket account ID into an email address. This function returns an empty
 // string if the account ID is not found. It uses persistent data storage, or API calls as a fallback.
 // This function is also wrapped in the "users" package, and reused by other packages from there.
-func BitbucketIDToEmail(ctx workflow.Context, accountID string) string {
+func BitbucketIDToEmail(ctx workflow.Context, accountID, accountType string) string {
 	if accountID == "" {
 		return ""
 	}
@@ -566,6 +582,10 @@ func BitbucketIDToEmail(ctx workflow.Context, accountID string) string {
 		return user.Email
 	}
 
+	if accountType == "app_user" {
+		return "" // Not a real user, so no point to check in Jira.
+	}
+
 	// We use the Jira API as a fallback because the Bitbucket API doesn't expose email addresses.
 	jiraUser, err := jira.UsersGet(ctx, accountID)
 	if err != nil {

pkg/data/users.go

@@ -153,7 +153,7 @@ func SelectUserByBitbucketID(ctx workflow.Context, accountID string) User {
 }
 
 func SelectUserByEmail(ctx workflow.Context, email string) User {
-	if email == "" {
+	if email == "" || email == "bot" {
 		return User{}
 	}
 

pkg/slack/commands/turns.go

@@ -168,7 +168,16 @@ func WhoseTurn(ctx workflow.Context, event SlashCommandEvent) error {
 }
 
 // whoseTurnText builds a "whose turn is it" summary message, reused by multiple slash commands.
+// The emails slice must be deduped, but may contain invalid/bot email addresses (which are removed).
 func whoseTurnText(ctx workflow.Context, emails []string, user data.User, tweak string) string {
+	// Ignore invalid/bot email addresses.
+	if i := slices.Index(emails, ""); i > -1 {
+		emails = slices.Delete(emails, i, i+1)
+	}
+	if i := slices.Index(emails, "bot"); i > -1 {
+		emails = slices.Delete(emails, i, i+1)
+	}
+
 	// If the user who ran the command is in the list, highlight that to them.
 	var msg strings.Builder
 	i := slices.Index(emails, user.Email)
@@ -180,11 +189,14 @@ func whoseTurnText(ctx workflow.Context, emails []string, user data.User, tweak
 	msg.WriteString(tweak)
 
 	withOthers := false
-	if i == -1 {
+	switch {
+	case i == -1 && len(emails) == 0:
+		msg.WriteString(" no one's turn")
+	case i == -1 && len(emails) > 0:
 		msg.WriteString(" the turn of ")
-	} else {
-		emails = slices.Delete(emails, i, i+1)
+	default:
 		msg.WriteString(" *your* turn")
+		emails = slices.Delete(emails, i, i+1)
 		if len(emails) > 0 {
 			msg.WriteString(" - along with ")
 			withOthers = true

pkg/slack/commands/turns_test.go

@@ -17,6 +17,12 @@ func TestWhoseTurnText(t *testing.T) {
 		tweak  string
 		want   string
 	}{
+		{
+			name:   "no_one",
+			emails: []string{"", "bot"},
+			user:   data.User{Email: "author@example.com"},
+			want:   "I think it's no one's turn to review this PR.",
+		},
 		{
 			name:   "author_only_with_tweak",
 			emails: []string{"author@example.com"},

pkg/slack/pr_data.go

@@ -51,6 +51,9 @@ func LoadPRTurns(ctx workflow.Context, onlyCurrentTurn, authors, reviewers bool)
 		}
 
 		for _, email := range emails {
+			if email == "" || email == "bot" {
+				continue // Nothing to check/remove.
+			}
 			if id := users.EmailToSlackID(ctx, email); id != "" {
 				usersToPRs[id] = append(usersToPRs[id], prURL)
 				continue

pkg/users/bitbucket.go

@@ -45,9 +45,20 @@ func EmailToBitbucketID(ctx workflow.Context, email string) string {
 	return users[0].AccountID
 }
 
+// BitbucketActorToEmail is a trivial wrapper around [BitbucketIDToEmail]. Merely syntactic sugar.
+func BitbucketActorToEmail(ctx workflow.Context, actor bitbucket.User) string {
+	return BitbucketIDToEmail(ctx, actor.AccountID, actor.Type)
+}
+
 // BitbucketIDToEmail is a trivial wrapper around [data.BitbucketIDToEmail].
-func BitbucketIDToEmail(ctx workflow.Context, accountID string) string {
-	return data.BitbucketIDToEmail(ctx, accountID)
+// It exists only for architectural clarity (all "XxxToYyy" functions in this package).
+// However, outside the "data" package, we return "bot" instead of "" for non-user accounts.
+func BitbucketIDToEmail(ctx workflow.Context, accountID, accountType string) string {
+	email := data.BitbucketIDToEmail(ctx, accountID, accountType)
+	if email == "" && accountType == "app_user" {
+		return "bot"
+	}
+	return email
 }
 
 // BitbucketIDToSlackID converts a Bitbucket account ID into a Slack user ID. This function returns an empty
@@ -56,7 +67,7 @@ func BitbucketIDToSlackID(ctx workflow.Context, accountID string, checkOptIn boo
 	user := data.SelectUserByBitbucketID(ctx, accountID)
 	if user.SlackID == "" {
 		// Workaround in case only the user's Bitbucket account ID isn't stored yet, but the rest is.
-		user = data.SelectUserByEmail(ctx, BitbucketIDToEmail(ctx, accountID))
+		user = data.SelectUserByEmail(ctx, BitbucketIDToEmail(ctx, accountID, "user"))
 	}
 
 	if checkOptIn && !user.IsOptedIn() {
@@ -72,7 +83,7 @@ func BitbucketIDToSlackRef(ctx workflow.Context, accountID, displayName string)
 	user := data.SelectUserByBitbucketID(ctx, accountID)
 	if user.SlackID == "" {
 		// Workaround in case only the user's Bitbucket account ID isn't stored yet, but the rest is.
-		user = data.SelectUserByEmail(ctx, BitbucketIDToEmail(ctx, accountID))
+		user = data.SelectUserByEmail(ctx, BitbucketIDToEmail(ctx, accountID, "user"))
 	}
 
 	if user.SlackID != "" {

pkg/users/bitbucket_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/tzrikka/revchat/pkg/users"
+	"github.com/tzrikka/timpani-api/pkg/bitbucket"
 )
 
 func TestEmailToBitbucketID(t *testing.T) {
@@ -13,3 +14,40 @@ func TestEmailToBitbucketID(t *testing.T) {
 		t.Errorf("EmailToBitbucketID() = %q, want %q", got, want)
 	}
 }
+
+func TestBitbucketActorToEmail(t *testing.T) {
+	tests := []struct {
+		name  string
+		actor bitbucket.User
+		want  string
+	}{
+		{
+			name:  "empty_actor",
+			actor: bitbucket.User{},
+			want:  "",
+		},
+		{
+			name: "app_with_account_id",
+			actor: bitbucket.User{
+				Type:      "app_user",
+				AccountID: "account-id-123",
+			},
+			want: "bot",
+		},
+		{
+			name: "app_without_account_id",
+			actor: bitbucket.User{
+				Type: "app_user",
+			},
+			want: "bot",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := users.BitbucketActorToEmail(nil, tt.actor); got != tt.want {
+				t.Errorf("BitbucketActorToEmail() = %q, want %q", got, tt.want)
+			}
+		})
+	}
+}