fix(mail): address Codex PR review — lazy reply derivation + restored local guards

- deriveReplyDefaults now takes a replyNeeds struct and only parses source
  headers for slots the caller actually consumes. An explicit --to override
  no longer fails on a malformed source From; an explicit --cc override
  no longer fails on malformed source To/Cc.
- Restore the local 'len(toAddrs) == 0' guard in non-reply mode so empty
  --to fails before any Gmail API call (regression introduced by reply mode).
- Treat --reply-to "" as non-reply mode (don't call GetMessage with empty ID,
  don't skip --to/--subject requirements). --reply-all --reply-to "" still
  errors with the existing requires-reply-to message.
- New tests pin exact alias-filter expectations and cover the three new
  edge cases above.

Author: rianjs

internal/cmd/mail/draft.go

@@ -56,7 +56,10 @@ Examples:
   gro mail draft --reply-to <message-id> --body "thanks, will review"
   gro mail draft --reply-to <message-id> --reply-all --body "..."`,
 		RunE: func(cmd *cobra.Command, _ []string) error {
-			isReply := cmd.Flags().Changed("reply-to")
+			// Reply mode is opt-in via a non-empty --reply-to. Empty string
+			// (e.g. --reply-to "") is treated the same as omitting the flag,
+			// since an empty message ID can't be fetched.
+			isReply := strings.TrimSpace(replyTo) != ""
 
 			// 1. Required-flag checks. In reply mode, --to and --subject are
 			//    derived from the source message and only required if not derivable.
@@ -108,6 +111,12 @@ Examples:
 				fromAddr = parsed.Address
 			}
 
+			// 3b. Non-reply mode: require at least one --to address now (no I/O yet).
+			//     Reply mode defers this check until after source derivation.
+			if !isReply && len(toAddrs) == 0 {
+				return fmt.Errorf("--to must contain at least one address")
+			}
+
 			// 4. Body source: exactly one of --body, --stdin, --file.
 			bodySources := 0
 			if cmd.Flags().Changed("body") {
@@ -195,8 +204,13 @@ Examples:
 				if err != nil {
 					return fmt.Errorf("fetching reply source %s: %w", replyTo, err)
 				}
+				needs := replyNeeds{
+					To:      !cmd.Flags().Changed("to"),
+					Cc:      replyAll && !cmd.Flags().Changed("cc"),
+					Subject: !cmd.Flags().Changed("subject"),
+				}
 				selfSet := map[string]bool{}
-				if replyAll {
+				if needs.Cc {
 					profile, err := client.GetProfile(cmd.Context())
 					if err != nil {
 						return fmt.Errorf("fetching profile for --reply-all: %w", err)
@@ -208,20 +222,20 @@ Examples:
 						selfSet[strings.ToLower(fromAddr)] = true
 					}
 				}
-				derived, err := deriveReplyDefaults(src, replyAll, selfSet)
+				derived, err := deriveReplyDefaults(src, needs, selfSet)
 				if err != nil {
 					return err
 				}
 				threadID = derived.ThreadID
 				inReplyTo = derived.InReplyTo
 				references = derived.References
-				if !cmd.Flags().Changed("to") {
+				if needs.To {
 					toAddrs = derived.To
 				}
-				if !cmd.Flags().Changed("cc") {
+				if needs.Cc {
 					ccAddrs = derived.Cc
 				}
-				if !cmd.Flags().Changed("subject") {
+				if needs.Subject {
 					subject = derived.Subject
 				}
 			}
@@ -302,28 +316,44 @@ type replyDerivation struct {
 	Subject    string
 }
 
-func deriveReplyDefaults(src *gmailapi.Message, replyAll bool, selfSet map[string]bool) (replyDerivation, error) {
+// replyNeeds tells deriveReplyDefaults which slots the caller will actually
+// consume. Source headers are only parsed for needed slots, so an explicit
+// --to/--cc/--subject override is unaffected by a malformed/missing source
+// header for that slot.
+type replyNeeds struct {
+	To      bool
+	Cc      bool
+	Subject bool
+}
+
+func deriveReplyDefaults(src *gmailapi.Message, needs replyNeeds, selfSet map[string]bool) (replyDerivation, error) {
 	if src == nil {
 		return replyDerivation{}, fmt.Errorf("source message is nil")
 	}
-	if strings.TrimSpace(src.From) == "" {
-		return replyDerivation{}, fmt.Errorf("source message %s has no From header", src.ID)
-	}
+	// Message-Id is always required: it feeds In-Reply-To and References,
+	// which are emitted regardless of override flags.
 	if strings.TrimSpace(src.RFCMessageID) == "" {
 		return replyDerivation{}, fmt.Errorf("source message %s has no Message-Id header", src.ID)
 	}
-	fromAddr, err := mail.ParseAddress(src.From)
-	if err != nil {
-		return replyDerivation{}, fmt.Errorf("parsing source From header %q: %w", src.From, err)
-	}
 	out := replyDerivation{
 		ThreadID:   src.ThreadID,
 		InReplyTo:  src.RFCMessageID,
 		References: buildReferences(src.References, src.RFCMessageID),
-		To:         []string{fromAddr.Address},
-		Subject:    addRePrefix(src.Subject),
 	}
-	if replyAll {
+	if needs.To {
+		if strings.TrimSpace(src.From) == "" {
+			return replyDerivation{}, fmt.Errorf("source message %s has no From header", src.ID)
+		}
+		fromAddr, err := mail.ParseAddress(src.From)
+		if err != nil {
+			return replyDerivation{}, fmt.Errorf("parsing source From header %q: %w", src.From, err)
+		}
+		out.To = []string{fromAddr.Address}
+	}
+	if needs.Subject {
+		out.Subject = addRePrefix(src.Subject)
+	}
+	if needs.Cc {
 		ccAddrs, err := splitAddressHeaders(src.To, src.Cc)
 		if err != nil {
 			return replyDerivation{}, fmt.Errorf("parsing source To/Cc headers: %w", err)

internal/cmd/mail/draft_test.go

@@ -606,12 +606,87 @@ func TestDraftCommand_ReplyAll_FiltersFromAlias(t *testing.T) {
 	withMockClient(mock, func() {
 		err := cmd.Execute()
 		testutil.NoError(t, err)
-		// alias@example.com filtered (via --from), me@example.com would have been (not present here), bob+carol remain.
-		for _, a := range seen.Cc {
-			if a == "alias@example.com" {
-				t.Errorf("alias not filtered from Cc: %v", seen.Cc)
-			}
-		}
+		// Source To = alias + bob; source Cc = carol. alias filtered via --from.
+		// Pin the exact list so the test fails if filtering accidentally drops bob/carol too.
+		testutil.LenSlice(t, len(seen.Cc), 2)
+		testutil.Equal(t, seen.Cc[0], "bob@example.com")
+		testutil.Equal(t, seen.Cc[1], "carol@example.com")
+	})
+}
+
+func TestDraftCommand_ReplyTo_OverrideToWithMalformedSourceFrom(t *testing.T) {
+	// --to override means the source From is never parsed, so a malformed
+	// source From header must not fail the command.
+	var seen gmailapi.DraftMessage
+	src := srcReply()
+	src.From = "not-an-email"
+	mock := &MockGmailClient{
+		GetMessageFunc: func(_ context.Context, _ string, _ bool) (*gmailapi.Message, error) { return src, nil },
+		CreateDraftFunc: func(_ context.Context, msg gmailapi.DraftMessage) (*gmailapi.DraftResult, error) {
+			seen = msg
+			return &gmailapi.DraftResult{ID: "d1"}, nil
+		},
+	}
+	cmd := newDraftCommand()
+	cmd.SetArgs([]string{"--reply-to", "msg-src", "--to", "override@x.com", "--body", "x", "--plain"})
+	withMockClient(mock, func() {
+		err := cmd.Execute()
+		testutil.NoError(t, err)
+		testutil.LenSlice(t, len(seen.To), 1)
+		testutil.Equal(t, seen.To[0], "override@x.com")
+		// Threading headers still derive (Message-Id is unaffected).
+		testutil.Equal(t, seen.InReplyTo, "<orig@example.com>")
+	})
+}
+
+func TestDraftCommand_ReplyAll_OverrideCcWithMalformedSourceToCc(t *testing.T) {
+	// --cc override means source To/Cc are never parsed, so malformed values must not fail.
+	var seen gmailapi.DraftMessage
+	src := srcReply()
+	src.To = "garbage @@@ not parseable"
+	mock := &MockGmailClient{
+		GetMessageFunc: func(_ context.Context, _ string, _ bool) (*gmailapi.Message, error) { return src, nil },
+		CreateDraftFunc: func(_ context.Context, msg gmailapi.DraftMessage) (*gmailapi.DraftResult, error) {
+			seen = msg
+			return &gmailapi.DraftResult{ID: "d1"}, nil
+		},
+	}
+	cmd := newDraftCommand()
+	cmd.SetArgs([]string{"--reply-to", "msg-src", "--reply-all", "--cc", "explicit@x.com", "--body", "x", "--plain"})
+	withMockClient(mock, func() {
+		err := cmd.Execute()
+		testutil.NoError(t, err)
+		testutil.LenSlice(t, len(seen.Cc), 1)
+		testutil.Equal(t, seen.Cc[0], "explicit@x.com")
+	})
+}
+
+func TestDraftCommand_EmptyReplyToTreatedAsNonReply(t *testing.T) {
+	mock := &MockGmailClient{
+		GetMessageFunc: func(_ context.Context, _ string, _ bool) (*gmailapi.Message, error) {
+			t.Fatal("GetMessage must not be called with empty --reply-to")
+			return nil, nil
+		},
+	}
+	cmd := newDraftCommand()
+	cmd.SetArgs([]string{"--reply-to", "", "--body", "x", "--plain"})
+	withMockClient(mock, func() {
+		err := cmd.Execute()
+		testutil.Error(t, err)
+		// Empty --reply-to falls back to non-reply mode, so --to is required.
+		testutil.Contains(t, err.Error(), "--to")
+	})
+}
+
+func TestDraftCommand_EmptyReplyToWithReplyAll(t *testing.T) {
+	mock := &MockGmailClient{}
+	cmd := newDraftCommand()
+	cmd.SetArgs([]string{"--reply-to", "", "--reply-all", "--body", "x", "--plain"})
+	withMockClient(mock, func() {
+		err := cmd.Execute()
+		testutil.Error(t, err)
+		testutil.Contains(t, err.Error(), "--reply-all")
+		testutil.Contains(t, err.Error(), "--reply-to")
 	})
 }