Add console.Print* stderr wrappers and migrate high-volume MCP/secrets callsites

docs/adr/47108-add-console-print-stderr-wrappers.md

@@ -0,0 +1,46 @@
+# ADR-47108: Add `Print*` Stderr Convenience Wrappers to the `console` Package
+
+**Date**: 2026-07-22
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+The `pkg/console` package exposed `Format*` helpers that return formatted strings suitable for diagnostic stderr output, but provided no direct write API. Every call site was required to compose `fmt.Fprintln(os.Stderr, console.Format*(...))` — a two-step pattern that was widespread (38+ call sites in `engine_secrets.go`, 29+ in `mcp_inspect_mcp.go` alone) and inconsistently expressed: some callers used `fmt.Fprintln`, others used `fmt.Fprintf(os.Stderr, "\n%s\n", ...)` with an explicit newline wrapper, creating divergent output-routing ergonomics within the same package boundary. Callers also required `fmt` and `os` imports solely to route console output to stderr.
+
+### Decision
+
+We will add a thin `Print*` layer in `pkg/console/print.go` that wraps each `Format*` helper with `fmt.Fprintln(os.Stderr, ...)`, providing a single-call API for every diagnostic output variant (success, info, warning, error, command, progress, prompt, verbose, section header, list item, error chain). We will migrate the two highest-volume call sites (`pkg/cli/engine_secrets.go` and `pkg/cli/mcp_inspect_mcp.go`) to use the new wrappers immediately, and update package docs and the README to prefer `Print*` for diagnostic stderr output.
+
+### Alternatives Considered
+
+#### Alternative 1: Enforce the existing two-step pattern via a linter rule
+
+The current `fmt.Fprintln(os.Stderr, console.Format*(...))` pattern could be mandated through a custom lint rule that rejects bare `fmt.Fprintln(os.Stderr, ...)` in favor of the canonical form. This avoids introducing a new API surface and keeps the format and write concerns separate. It was rejected because it adds lint infrastructure without eliminating boilerplate or import overhead, and it cannot address the divergent `Fprintf` + newline-wrapper variant already present in the codebase.
+
+#### Alternative 2: Change `Format*` functions to write directly to stderr
+
+The existing `Format*` functions could be converted to print side-effectfully rather than returning strings. This would achieve a single-call API without a new parallel set of names. It was rejected because it would destroy the composability of the format layer: callers that redirect output to a non-stderr writer (e.g., in-memory buffers, test writers, or `io.Writer` parameters) would lose that flexibility. Keeping `Format*` as pure string formatters and introducing separate `Print*` writers preserves both use cases.
+
+### Consequences
+
+#### Positive
+- Call sites that only write to stderr can drop `fmt` and `os` imports, reducing import churn across CLI code.
+- Stderr routing is centralized in `pkg/console` rather than distributed across every call site, making future output-redirection changes (e.g., pluggable writers) a one-place edit.
+- Inconsistent newline-wrapping patterns (`Fprintln` vs. `Fprintf("\n%s\n", ...)`) are resolved; `Print*` always adds exactly one trailing newline.
+- Test coverage for the new layer is provided via `pkg/console/print_test.go`.
+
+#### Negative
+- Two parallel APIs now exist for the same underlying concepts: `Format*` (string) and `Print*` (stderr side-effect). Callers must decide which to use and may mix them inconsistently during the partial migration period.
+- `Print*` functions hardcode `os.Stderr` as the destination, which limits testability without OS-level pipe tricks (as demonstrated in `print_test.go`). A future `io.Writer`-parameterised API may be needed for thorough unit testing.
+- The migration is intentionally incomplete: only two files are updated in this PR. The remaining call sites continue using the old pattern, so the codebase is temporarily in a mixed state.
+
+#### Neutral
+- The `Print*` functions propagate `(int, error)` return values from `fmt.Fprintln`, consistent with Go conventions, but callers universally discard both values (`_, _ = console.Print*(...)`) since stderr-write errors are not actionable at runtime.
+- Package documentation (`doc.go`, `README.md`) is updated to recommend `Print*` as the preferred pattern for diagnostic output.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/engine_secrets.go

@@ -217,7 +217,7 @@ func checkAndEnsureEngineSecretsForEngine(config EngineSecretConfig) error {
 		if req.Optional {
 			// For optional secrets, just check and report
 			if err := checkOptionalSecret(req, config); err != nil && config.Verbose {
-				fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Optional secret %s: %v", req.Name, err)))
+				console.PrintWarningMessage(fmt.Sprintf("Optional secret %s: %v", req.Name, err))
 			}
 			continue
 		}
@@ -239,27 +239,27 @@ func ensureSecretAvailable(req SecretRequirement, config EngineSecretConfig) err
 	// Check if secret already exists in the repository
 	if setutil.Contains(config.ExistingSecrets, req.Name) {
 		if mustValidateExistingSecretValue(req) {
-			fmt.Fprintln(os.Stderr, console.FormatWarningMessage(req.Name+" already exists, but GitHub does not expose stored secret values for validation."))
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Paste the current or replacement fine-grained PAT so gh aw can validate it and update the repository secret."))
+			console.PrintWarningMessage(req.Name + " already exists, but GitHub does not expose stored secret values for validation.")
+			console.PrintInfoMessage("Paste the current or replacement fine-grained PAT so gh aw can validate it and update the repository secret.")
 			revalidateConfig := config
 			revalidateConfig.OverwriteExistingSecret = true
 			return engineSecretsPromptFn(req, revalidateConfig)
 		}
-		fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Using existing %s secret in repository", req.Name)))
+		console.PrintSuccessMessage(fmt.Sprintf("Using existing %s secret in repository", req.Name))
 		return nil
 	}
 
 	// Check alternative secret names in repository
 	for _, alt := range req.AlternativeEnvVars {
 		if setutil.Contains(config.ExistingSecrets, alt) {
 			if mustValidateExistingSecretValue(req) {
-				fmt.Fprintln(os.Stderr, console.FormatWarningMessage(alt+" already exists in the repository, but GitHub does not expose stored secret values for validation."))
-				fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Paste the current or replacement fine-grained PAT so gh aw can validate it and store it as %s.", req.Name)))
+				console.PrintWarningMessage(alt + " already exists in the repository, but GitHub does not expose stored secret values for validation.")
+				console.PrintInfoMessage(fmt.Sprintf("Paste the current or replacement fine-grained PAT so gh aw can validate it and store it as %s.", req.Name))
 				revalidateConfig := config
 				revalidateConfig.OverwriteExistingSecret = true
 				return engineSecretsPromptFn(req, revalidateConfig)
 			}
-			fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Using existing %s secret in repository (alternative for %s)", alt, req.Name)))
+			console.PrintSuccessMessage(fmt.Sprintf("Using existing %s secret in repository (alternative for %s)", alt, req.Name))
 			return nil
 		}
 	}
@@ -281,19 +281,19 @@ func ensureSecretAvailable(req SecretRequirement, config EngineSecretConfig) err
 		// Validate if it's a Copilot token
 		if req.IsEngineSecret && req.EngineName == string(constants.CopilotEngine) {
 			if err := stringutil.ValidateCopilotPAT(envValue); err != nil {
-				fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("%s in environment is not a valid fine-grained PAT: %s", req.Name, stringutil.GetPATTypeDescription(envValue))))
-				fmt.Fprintln(os.Stderr, console.FormatErrorMessage(err.Error()))
+				console.PrintWarningMessage(fmt.Sprintf("%s in environment is not a valid fine-grained PAT: %s", req.Name, stringutil.GetPATTypeDescription(envValue)))
+				console.PrintErrorMessage(err.Error())
 				// Continue to prompt for a new token
 			} else {
-				fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Found valid %s in environment", req.Name)))
+				console.PrintSuccessMessage(fmt.Sprintf("Found valid %s in environment", req.Name))
 				// Upload to repository if we have a repo slug
 				if config.RepoSlug != "" {
 					return engineSecretsUploadFn(req.Name, envValue, config.RepoSlug, config.Verbose, config.OverwriteExistingSecret)
 				}
 				return nil
 			}
 		} else {
-			fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Found %s in environment", req.Name)))
+			console.PrintSuccessMessage(fmt.Sprintf("Found %s in environment", req.Name))
 			// Upload to repository if we have a repo slug
 			if config.RepoSlug != "" {
 				return engineSecretsUploadFn(req.Name, envValue, config.RepoSlug, config.Verbose, config.OverwriteExistingSecret)
@@ -335,7 +335,7 @@ func promptForCopilotPATUnified(req SecretRequirement, config EngineSecretConfig
 	fmt.Fprintln(os.Stderr, "Create a fine-grained Personal Access Token (PAT) from the preconfigured page below, then paste it back here.")
 	fmt.Fprintln(os.Stderr, "")
 	fmt.Fprintln(os.Stderr, "Preconfigured token creation page:")
-	fmt.Fprintln(os.Stderr, console.FormatCommandMessage("  "+preconfiguredPATURL))
+	console.PrintCommandMessage("  " + preconfiguredPATURL)
 	fmt.Fprintln(os.Stderr, "")
 
 	openBrowser := true
@@ -353,9 +353,9 @@ func promptForCopilotPATUnified(req SecretRequirement, config EngineSecretConfig
 		// Non-interactive: skip the consent gate and fall through to token input
 	} else if openBrowser {
 		if openBootstrapBrowser(preconfiguredPATURL) {
-			fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Opened the preconfigured Copilot PAT page in your browser."))
+			console.PrintSuccessMessage("Opened the preconfigured Copilot PAT page in your browser.")
 		} else {
-			fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Couldn't open your browser automatically (no supported opener found) — open the URL above manually."))
+			console.PrintWarningMessage("Couldn't open your browser automatically (no supported opener found) — open the URL above manually.")
 		}
 	}
 
@@ -387,7 +387,7 @@ func promptForCopilotPATUnified(req SecretRequirement, config EngineSecretConfig
 
 	token = strings.TrimSpace(token)
 
-	fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Valid fine-grained Copilot token received"))
+	console.PrintSuccessMessage("Valid fine-grained Copilot token received")
 
 	// Upload to repository if we have a repo slug
 	if config.RepoSlug != "" {
@@ -436,11 +436,11 @@ func promptForSystemTokenUnified(req SecretRequirement, config EngineSecretConfi
 	fmt.Fprintln(os.Stderr, "")
 	fmt.Fprintf(os.Stderr, "%s requires a GitHub Personal Access Token (PAT).\n", req.Name)
 	fmt.Fprintln(os.Stderr, "")
-	fmt.Fprintln(os.Stderr, console.FormatInfoMessage("When needed: "+req.WhenNeeded))
-	fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Recommended scopes: "+req.Description))
+	console.PrintInfoMessage("When needed: " + req.WhenNeeded)
+	console.PrintInfoMessage("Recommended scopes: " + req.Description)
 	fmt.Fprintln(os.Stderr, "")
 	fmt.Fprintln(os.Stderr, "Create a token at:")
-	fmt.Fprintln(os.Stderr, console.FormatCommandMessage("  "+patURL))
+	console.PrintCommandMessage("  " + patURL)
 	fmt.Fprintln(os.Stderr, "")
 
 	var token string
@@ -465,7 +465,7 @@ func promptForSystemTokenUnified(req SecretRequirement, config EngineSecretConfi
 		return fmt.Errorf("failed to get %s token: %w", req.Name, err)
 	}
 
-	fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(req.Name+" token received"))
+	console.PrintSuccessMessage(req.Name + " token received")
 
 	// Upload to repository if we have a repo slug
 	if config.RepoSlug != "" {
@@ -491,7 +491,7 @@ func promptForGenericAPIKeyUnified(req SecretRequirement, config EngineSecretCon
 	fmt.Fprintln(os.Stderr, "")
 	if req.KeyURL != "" {
 		fmt.Fprintln(os.Stderr, "Get your API key from:")
-		fmt.Fprintln(os.Stderr, console.FormatCommandMessage("  "+req.KeyURL))
+		console.PrintCommandMessage("  " + req.KeyURL)
 		fmt.Fprintln(os.Stderr, "")
 	}
 
@@ -517,7 +517,7 @@ func promptForGenericAPIKeyUnified(req SecretRequirement, config EngineSecretCon
 		return fmt.Errorf("failed to get %s API key: %w", label, err)
 	}
 
-	fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(label+" API key received"))
+	console.PrintSuccessMessage(label + " API key received")
 
 	// Upload to repository if we have a repo slug
 	if config.RepoSlug != "" {
@@ -532,15 +532,15 @@ func checkOptionalSecret(req SecretRequirement, config EngineSecretConfig) error
 	// Check repository
 	if setutil.Contains(config.ExistingSecrets, req.Name) {
 		if config.Verbose {
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Optional secret %s exists in repository", req.Name)))
+			console.PrintInfoMessage(fmt.Sprintf("Optional secret %s exists in repository", req.Name))
 		}
 		return nil
 	}
 
 	// Check environment
 	if os.Getenv(req.Name) != "" { //nolint:osgetenvlibrary
 		if config.Verbose {
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Optional secret %s found in environment", req.Name)))
+			console.PrintInfoMessage(fmt.Sprintf("Optional secret %s found in environment", req.Name))
 		}
 		return nil
 	}
@@ -557,26 +557,26 @@ func uploadSecretToRepo(secretName, secretValue, repoSlug string, verbose bool,
 	if err == nil && stringContainsSecretName(string(output), secretName) {
 		if !overwriteExisting {
 			if verbose {
-				fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Secret %s already exists, skipping upload", secretName)))
+				console.PrintInfoMessage(fmt.Sprintf("Secret %s already exists, skipping upload", secretName))
 			}
 			return nil
 		}
 		if verbose {
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Secret %s already exists, replacing it with the validated value", secretName)))
+			console.PrintInfoMessage(fmt.Sprintf("Secret %s already exists, replacing it with the validated value", secretName))
 		}
 	}
 
 	// Upload the secret
 	if verbose {
-		fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Uploading %s secret to repository", secretName)))
+		console.PrintInfoMessage(fmt.Sprintf("Uploading %s secret to repository", secretName))
 	}
 
 	output, err = workflow.RunGHCombined("Setting secret...", "secret", "set", secretName, "--repo", repoSlug, "--body", secretValue)
 	if err != nil {
 		return fmt.Errorf("failed to set %s secret: %w (output: %s)", secretName, err, string(output))
 	}
 
-	fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Uploaded %s secret to repository", secretName)))
+	console.PrintSuccessMessage(fmt.Sprintf("Uploaded %s secret to repository", secretName))
 	return nil
 }
 
@@ -697,25 +697,25 @@ func displayMissingSecrets(requirements []SecretRequirement, repoSlug string, ex
 	cmdRepo := parts[1]
 
 	if len(requiredMissing) > 0 {
-		fmt.Fprintln(os.Stderr, console.FormatErrorMessage("Required secrets are missing:"))
+		console.PrintErrorMessage("Required secrets are missing:")
 		for _, req := range requiredMissing {
 			fmt.Fprintln(os.Stderr, "")
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Secret: "+req.Name))
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage("When needed: "+req.WhenNeeded))
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Recommended scopes: "+req.Description))
-			fmt.Fprintln(os.Stderr, console.FormatCommandMessage(fmt.Sprintf("gh aw secrets set %s --owner %s --repo %s", req.Name, cmdOwner, cmdRepo)))
+			console.PrintInfoMessage("Secret: " + req.Name)
+			console.PrintInfoMessage("When needed: " + req.WhenNeeded)
+			console.PrintInfoMessage("Recommended scopes: " + req.Description)
+			console.PrintCommandMessage(fmt.Sprintf("gh aw secrets set %s --owner %s --repo %s", req.Name, cmdOwner, cmdRepo))
 		}
 	}
 
 	if len(optionalMissing) > 0 {
 		fmt.Fprintln(os.Stderr, "")
-		fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Optional secrets are missing:"))
+		console.PrintWarningMessage("Optional secrets are missing:")
 		for _, req := range optionalMissing {
 			fmt.Fprintln(os.Stderr, "")
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Secret: %s (optional)", req.Name)))
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage("When needed: "+req.WhenNeeded))
-			fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Recommended scopes: "+req.Description))
-			fmt.Fprintln(os.Stderr, console.FormatCommandMessage(fmt.Sprintf("gh aw secrets set %s --owner %s --repo %s", req.Name, cmdOwner, cmdRepo)))
+			console.PrintInfoMessage(fmt.Sprintf("Secret: %s (optional)", req.Name))
+			console.PrintInfoMessage("When needed: " + req.WhenNeeded)
+			console.PrintInfoMessage("Recommended scopes: " + req.Description)
+			console.PrintCommandMessage(fmt.Sprintf("gh aw secrets set %s --owner %s --repo %s", req.Name, cmdOwner, cmdRepo))
 		}
 	}
 
@@ -739,7 +739,7 @@ func displaySecretsSummaryTable(requirements []SecretRequirement, existingSecret
 	}
 
 	fmt.Fprintln(os.Stderr, "")
-	fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Required secrets summary:"))
+	console.PrintInfoMessage("Required secrets summary:")
 	fmt.Fprintln(os.Stderr, "")
 
 	// Calculate max width for alignment