chore(write): address #168 nice-to-haves (code subset)

CHANGELOG.md

@@ -9,6 +9,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- Address the post-write-phase nice-to-have bundle from #168:
+  - `kaswrite.Call` now prefixes the module label onto the wrapped
+    `ErrUnexpectedReturnString` message, so a regression in e.g.
+    `mailforward.Client.Add` reads `kaswrite: unexpected ReturnString
+    (want TRUE): mailforward add_mailforward got "…"` instead of
+    losing the module hint to the canonical sentinel prefix. The
+    sentinel identity is unchanged — `errors.Is(err,
+    mailforward.ErrUnexpectedReturnString)` keeps working.
+  - `kasapi-cli mail forwards add --target` help no longer claims to
+    "replace the full target list" (which is true for `update`, not
+    for `add`); the add-side help now reads "repeatable; at least
+    one required".
+  - `kasapi-cli mail lists update --active` now takes an explicit
+    `Y|N` argument (string flag), matching the `mail accounts
+    update --active` surface and removing the slightly non-obvious
+    `--active=false` form that previously meant "deactivate".
+  - `runWriteE` resolves the credentials exactly once per write
+    invocation: the post-gate dispatch now reuses the creds
+    `runWriteE` already resolved for the audit login, via the new
+    unexported `buildAPIClientFromCreds` helper. Previously the
+    config + env + flag resolution ran twice — once for the audit,
+    once inside `BuildAPIClient` — for every real write call.
+    `BuildAPIClient` (the read-side seam) is unchanged.
+
 - Cross-module convention alignment, post-#122-review parity sweep:
   - `account.AccountList` / `mailaccount.MailAccountList` now render
     the `used_*_space` column with a `" MB"` suffix as part of the

docs/cli/kasapi-cli_mail_forwards_add.md

@@ -10,7 +10,7 @@ kasapi-cli mail forwards add <address> --target <addr> [--target <addr>...] [fla
 
 ```
   -h, --help                 help for add
-      --target stringArray   forward target address (repeatable; replaces the full target list)
+      --target stringArray   forward target address (repeatable; at least one required)
 ```
 
 ### Options inherited from parent commands

docs/cli/kasapi-cli_mail_lists_update.md

@@ -3,13 +3,13 @@
 Replace mutable fields of a mailing list (update_mailinglist)
 
 ```
-kasapi-cli mail lists update <name> [--subscriber <addr>...] [--restrict-post <addr>...] [--config-file <path>] [--active] [flags]
+kasapi-cli mail lists update <name> [--subscriber <addr>...] [--restrict-post <addr>...] [--config-file <path>] [--active Y|N] [flags]
 ```
 
 ### Options
 
 ```
-      --active                      activate the list; pass --active=false to deactivate it
+      --active string               list status (Y|N)
       --config-file string          path to the complete list configuration file (replaces the config wholesale)
   -h, --help                        help for update
       --restrict-post stringArray   restrict-post address (repeatable; replaces the full restrict-post list)

internal/cli/mail.go

@@ -84,10 +84,9 @@ func newMailListsAddCmd(opts *RootOptions) *cobra.Command {
 
 func newMailListsUpdateCmd(opts *RootOptions) *cobra.Command {
 	var subscriber, restrictPost []string
-	var configFile string
-	var active bool
+	var configFile, active string
 	cmd := &cobra.Command{
-		Use:   "update <name> [--subscriber <addr>...] [--restrict-post <addr>...] [--config-file <path>] [--active]",
+		Use:   "update <name> [--subscriber <addr>...] [--restrict-post <addr>...] [--config-file <path>] [--active Y|N]",
 		Short: "Replace mutable fields of a mailing list (update_mailinglist)",
 		Args:  cobra.ExactArgs(1),
 	}
@@ -113,11 +112,7 @@ func newMailListsUpdateCmd(opts *RootOptions) *cobra.Command {
 			fields[mailinglist.FieldConfig] = string(b)
 		}
 		if cmd.Flags().Changed("active") {
-			if active {
-				fields[mailinglist.FieldIsActive] = "Y"
-			} else {
-				fields[mailinglist.FieldIsActive] = "N"
-			}
+			fields[mailinglist.FieldIsActive] = active
 		}
 		if len(fields) == 0 {
 			return writeSpec{}, fmt.Errorf("at least one of --subscriber/--restrict-post/--config-file/--active is required")
@@ -138,7 +133,7 @@ func newMailListsUpdateCmd(opts *RootOptions) *cobra.Command {
 	cmd.Flags().StringArrayVar(&subscriber, "subscriber", nil, "list subscriber address (repeatable; replaces the full subscriber list)")
 	cmd.Flags().StringArrayVar(&restrictPost, "restrict-post", nil, "restrict-post address (repeatable; replaces the full restrict-post list)")
 	cmd.Flags().StringVar(&configFile, "config-file", "", "path to the complete list configuration file (replaces the config wholesale)")
-	cmd.Flags().BoolVar(&active, "active", false, "activate the list; pass --active=false to deactivate it")
+	cmd.Flags().StringVar(&active, "active", "", "list status (Y|N)")
 	return cmd
 }
 
@@ -639,7 +634,7 @@ func newMailForwardsAddCmd(opts *RootOptions) *cobra.Command {
 			}, nil
 		}),
 	}
-	cmd.Flags().StringArrayVar(&targets, "target", nil, "forward target address (repeatable; replaces the full target list)")
+	cmd.Flags().StringArrayVar(&targets, "target", nil, "forward target address (repeatable; at least one required)")
 	return cmd
 }
 

internal/cli/mail_test.go

@@ -542,7 +542,7 @@ func TestMailListsDestructiveRefuseNonTTY(t *testing.T) {
 	t.Parallel()
 	for _, args := range [][]string{
 		{"mail", "lists", "delete", "announce-example-com"},
-		{"mail", "lists", "update", "announce-example-com", "--active"},
+		{"mail", "lists", "update", "announce-example-com", "--active", "Y"},
 	} {
 		t.Run(args[2], func(t *testing.T) {
 			t.Parallel()
@@ -580,14 +580,14 @@ func TestMailListsUpdateDryRunFieldAssembly(t *testing.T) {
 		absent []string
 	}{
 		{
-			"active true",
-			[]string{"mail", "lists", "update", "L", "--active"},
+			"active Y",
+			[]string{"mail", "lists", "update", "L", "--active", "Y"},
 			map[string]string{"mailinglist_name": "L", "is_active": "Y"},
 			[]string{"subscriber", "restrict_post", "config"},
 		},
 		{
-			"active false",
-			[]string{"mail", "lists", "update", "L", "--active=false"},
+			"active N",
+			[]string{"mail", "lists", "update", "L", "--active", "N"},
 			map[string]string{"mailinglist_name": "L", "is_active": "N"},
 			nil,
 		},

internal/cli/run.go

@@ -97,6 +97,12 @@ type writeSpec struct {
 // and the dry-run audit (#131) — and only then builds the API client
 // and dispatches. After dispatch it always emits the success/failure
 // audit record via OutcomeFor, then prints the result line on success.
+//
+// Credentials are resolved exactly once per invocation: the resolved
+// login feeds the dry-run / destructive-gate path directly, and the
+// post-gate dispatch reuses the same creds via the unexported
+// buildAPIClientFromCreds helper instead of re-running the
+// config+env+flag resolution that BuildAPIClient would do internally.
 func runWriteE(opts *RootOptions, build func(args []string) (writeSpec, error)) func(*cobra.Command, []string) error {
 	return func(cmd *cobra.Command, args []string) error {
 		spec, err := build(args)
@@ -133,7 +139,7 @@ func runWriteE(opts *RootOptions, build func(args []string) (writeSpec, error))
 			return err
 		}
 
-		c, err := BuildAPIClient(opts)
+		c, err := buildAPIClientFromCreds(opts, creds)
 		if err != nil {
 			return err
 		}

internal/cli/wire.go

@@ -36,13 +36,20 @@ func BuildAPIClient(opts *RootOptions) (*api.Client, error) {
 	if opts == nil {
 		return nil, UserError(errors.New("nil RootOptions"), "cli")
 	}
-
-	logger := buildLogger(opts.Verbose)
-
 	creds, err := resolveCreds(opts)
 	if err != nil {
 		return nil, err
 	}
+	return buildAPIClientFromCreds(opts, creds)
+}
+
+// buildAPIClientFromCreds wires the transport + token source on top of
+// already-resolved credentials. Callers that need the login both for an
+// audit trace and the actual KAS call (runWriteE / sessions delete)
+// resolve once and call this helper to avoid re-running the
+// config+env+flag resolution.
+func buildAPIClientFromCreds(opts *RootOptions, creds config.Credentials) (*api.Client, error) {
+	logger := buildLogger(opts.Verbose)
 	logger.Info("cli: credentials resolved",
 		"login", creds.Login, "auth_type", creds.AuthType, "auth_data", "<redacted>")
 

internal/kaswrite/kaswrite.go

@@ -38,7 +38,10 @@ var ErrUnexpectedReturnString = errors.New("kaswrite: unexpected ReturnString (w
 // with label and action; a non-fault response whose ReturnString is
 // not "TRUE" wraps ErrUnexpectedReturnString so a future API drift
 // fails the mapping test instead of silently passing. label is the
-// domain module name used only for the nil-response diagnostic.
+// domain module name (e.g. "mailforward") prefixed onto both the
+// nil-response diagnostic and the ReturnString-wrap message so log
+// readers see which module surfaced the failure even after the
+// canonical "kaswrite:" sentinel.
 func Call(
 	ctx context.Context, caller Caller, label, action string, params map[string]any,
 ) (*soap.Response, error) {
@@ -50,7 +53,7 @@ func Call(
 		return nil, fmt.Errorf("%s: %s: nil response without error from Caller", label, action)
 	}
 	if got := resp.Body.ReturnString; got != "TRUE" {
-		return nil, fmt.Errorf("%w: %s got %q", ErrUnexpectedReturnString, action, got)
+		return nil, fmt.Errorf("%w: %s %s got %q", ErrUnexpectedReturnString, label, action, got)
 	}
 	return resp, nil
 }

internal/kaswrite/kaswrite_test.go

@@ -53,6 +53,9 @@ func TestCallRejectsNonTrueReturnString(t *testing.T) {
 	if !strings.Contains(err.Error(), "update_mailinglist") || !strings.Contains(err.Error(), "FALSE") {
 		t.Errorf("err = %q, want it to contain the action and the observed value", err)
 	}
+	if !strings.Contains(err.Error(), "mailinglist") {
+		t.Errorf("err = %q, want it to carry the module label prefix", err)
+	}
 }
 
 func TestCallPropagatesCallerError(t *testing.T) {