diff --git a/docs/adr/47108-add-console-print-stderr-wrappers.md b/docs/adr/47108-add-console-print-stderr-wrappers.md new file mode 100644 index 00000000000..5d6d4caa9df --- /dev/null +++ b/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.* diff --git a/pkg/cli/engine_secrets.go b/pkg/cli/engine_secrets.go index 660c1683267..6b12c577a70 100644 --- a/pkg/cli/engine_secrets.go +++ b/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,13 +239,13 @@ 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 } @@ -253,13 +253,13 @@ func ensureSecretAvailable(req SecretRequirement, config EngineSecretConfig) err 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,11 +281,11 @@ 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) @@ -293,7 +293,7 @@ func ensureSecretAvailable(req SecretRequirement, config EngineSecretConfig) err 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,7 +532,7 @@ 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 } @@ -540,7 +540,7 @@ func checkOptionalSecret(req SecretRequirement, config EngineSecretConfig) error // 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,18 +557,18 @@ 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) @@ -576,7 +576,7 @@ func uploadSecretToRepo(secretName, secretValue, repoSlug string, verbose bool, 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 diff --git a/pkg/cli/mcp_inspect_mcp.go b/pkg/cli/mcp_inspect_mcp.go index 730b8bf8603..e88fff95bb5 100644 --- a/pkg/cli/mcp_inspect_mcp.go +++ b/pkg/cli/mcp_inspect_mcp.go @@ -85,7 +85,7 @@ func inspectMCPServer(config parser.RegistryMCPServerConfig, toolFilter string, mcpInspectServerLog.Printf("Successfully connected to MCP server: %s", config.Name) if verbose { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("✅ Successfully connected to MCP server")) + console.PrintSuccessMessage("✅ Successfully connected to MCP server") } // Display server capabilities @@ -135,7 +135,7 @@ func connectToMCPServer(config parser.RegistryMCPServerConfig, verbose bool) (*p func connectStdioMCPServer(ctx context.Context, config parser.RegistryMCPServerConfig, verbose bool) (*parser.MCPServerInfo, error) { mcpInspectServerLog.Printf("Connecting to stdio MCP server: command=%s, args=%d", config.Command, len(config.Args)) if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Starting stdio MCP server: %s %s", config.Command, strings.Join(config.Args, " ")))) + console.PrintInfoMessage(fmt.Sprintf("Starting stdio MCP server: %s %s", config.Command, strings.Join(config.Args, " "))) } // Validate the command exists @@ -183,7 +183,7 @@ func connectStdioMCPServer(ctx context.Context, config parser.RegistryMCPServerC defer session.Close() if verbose { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Successfully connected to MCP server")) + console.PrintSuccessMessage("Successfully connected to MCP server") } // Query server capabilities @@ -202,7 +202,7 @@ func connectStdioMCPServer(ctx context.Context, config parser.RegistryMCPServerC cancel() if err != nil { if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to list tools: %v", err))) + console.PrintWarningMessage(fmt.Sprintf("Failed to list tools: %v", err)) } } else { info.Tools = append(info.Tools, toolsResult.Tools...) @@ -215,7 +215,7 @@ func connectStdioMCPServer(ctx context.Context, config parser.RegistryMCPServerC cancel() if err != nil { if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to list resources: %v", err))) + console.PrintWarningMessage(fmt.Sprintf("Failed to list resources: %v", err)) } } else { info.Resources = append(info.Resources, resourcesResult.Resources...) @@ -231,7 +231,7 @@ func connectStdioMCPServer(ctx context.Context, config parser.RegistryMCPServerC // connectHTTPMCPServer connects to an HTTP-based MCP server using the Go SDK func connectHTTPMCPServer(ctx context.Context, config parser.RegistryMCPServerConfig, verbose bool) (*parser.MCPServerInfo, error) { if verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Connecting to HTTP MCP server: "+config.URL)) + console.PrintInfoMessage("Connecting to HTTP MCP server: " + config.URL) } // Create MCP client with logger for better debugging @@ -274,7 +274,7 @@ func connectHTTPMCPServer(ctx context.Context, config parser.RegistryMCPServerCo defer session.Close() if verbose { - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Successfully connected to HTTP MCP server")) + console.PrintSuccessMessage("Successfully connected to HTTP MCP server") } // Query server capabilities @@ -293,7 +293,7 @@ func connectHTTPMCPServer(ctx context.Context, config parser.RegistryMCPServerCo cancel() if err != nil { if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to list tools: %v", err))) + console.PrintWarningMessage(fmt.Sprintf("Failed to list tools: %v", err)) } } else { info.Tools = append(info.Tools, toolsResult.Tools...) @@ -306,7 +306,7 @@ func connectHTTPMCPServer(ctx context.Context, config parser.RegistryMCPServerCo cancel() if err != nil { if verbose { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to list resources: %v", err))) + console.PrintWarningMessage(fmt.Sprintf("Failed to list resources: %v", err)) } } else { info.Resources = append(info.Resources, resourcesResult.Resources...) @@ -363,7 +363,8 @@ func displayServerCapabilities(info *parser.MCPServerInfo, toolFilter string) { if toolFilter != "" { displayDetailedToolInfo(info, toolFilter) } else { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatSectionHeader("🛠️ Tool Access Status")) + fmt.Fprintln(os.Stderr) + console.PrintSectionHeader("🛠️ Tool Access Status") // Configure options for inspect command // Use a slightly shorter truncation length than list-tools for better fit @@ -382,15 +383,18 @@ func displayServerCapabilities(info *parser.MCPServerInfo, toolFilter string) { } else { if toolFilter != "" { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatWarningMessage(fmt.Sprintf("Tool '%s' not found", toolFilter))) + fmt.Fprintln(os.Stderr) + console.PrintWarningMessage(fmt.Sprintf("Tool '%s' not found", toolFilter)) } else { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatWarningMessage("No tools available")) + fmt.Fprintln(os.Stderr) + console.PrintWarningMessage("No tools available") } } // Display resources (skip if showing specific tool details) if toolFilter == "" && len(info.Resources) > 0 { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatSectionHeader("📚 Available Resources")) + fmt.Fprintln(os.Stderr) + console.PrintSectionHeader("📚 Available Resources") headers := []string{"URI", "Name", "Description", "MIME Type"} rows := make([][]string, 0, len(info.Resources)) @@ -411,12 +415,14 @@ func displayServerCapabilities(info *parser.MCPServerInfo, toolFilter string) { Rows: rows, })) } else if toolFilter == "" { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatWarningMessage("No resources available")) + fmt.Fprintln(os.Stderr) + console.PrintWarningMessage("No resources available") } // Display roots (skip if showing specific tool details) if toolFilter == "" && len(info.Roots) > 0 { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatSectionHeader("🌳 Available Roots")) + fmt.Fprintln(os.Stderr) + console.PrintSectionHeader("🌳 Available Roots") headers := []string{"URI", "Name"} rows := make([][]string, 0, len(info.Roots)) @@ -430,7 +436,8 @@ func displayServerCapabilities(info *parser.MCPServerInfo, toolFilter string) { Rows: rows, })) } else if toolFilter == "" { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatWarningMessage("No roots available")) + fmt.Fprintln(os.Stderr) + console.PrintWarningMessage("No roots available") } fmt.Fprintln(os.Stderr) @@ -448,7 +455,8 @@ func displayDetailedToolInfo(info *parser.MCPServerInfo, toolName string) { } if foundTool == nil { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatWarningMessage(fmt.Sprintf("Tool '%s' not found", toolName))) + fmt.Fprintln(os.Stderr) + console.PrintWarningMessage(fmt.Sprintf("Tool '%s' not found", toolName)) fmt.Fprintf(os.Stderr, "Available tools: ") toolNames := make([]string, len(info.Tools)) for i, tool := range info.Tools { @@ -464,7 +472,8 @@ func displayDetailedToolInfo(info *parser.MCPServerInfo, toolName string) { isAllowed = true } - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatSectionHeader("🛠️ Tool Details: "+foundTool.Name)) + fmt.Fprintln(os.Stderr) + console.PrintSectionHeader("🛠️ Tool Details: " + foundTool.Name) // Display basic information fmt.Fprintf(os.Stderr, "📋 **Name:** %s\n", foundTool.Name) @@ -488,7 +497,8 @@ func displayDetailedToolInfo(info *parser.MCPServerInfo, toolName string) { // Display annotations if available if foundTool.Annotations != nil { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatSectionHeader("⚙️ Tool Attributes")) + fmt.Fprintln(os.Stderr) + console.PrintSectionHeader("⚙️ Tool Attributes") if foundTool.Annotations.ReadOnlyHint { fmt.Fprintf(os.Stderr, "🔒 **Read-only:** This tool does not modify its environment\n") @@ -519,26 +529,30 @@ func displayDetailedToolInfo(info *parser.MCPServerInfo, toolName string) { // Display input schema if foundTool.InputSchema != nil { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatSectionHeader("📥 Input Schema")) + fmt.Fprintln(os.Stderr) + console.PrintSectionHeader("📥 Input Schema") if schemaJSON, err := json.MarshalIndent(foundTool.InputSchema, "", " "); err == nil { fmt.Fprintf(os.Stderr, "```json\n%s\n```\n", string(schemaJSON)) } else { fmt.Fprintf(os.Stderr, "Error displaying input schema: %v\n", err) } } else { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatInfoMessage("📥 No input schema defined")) + fmt.Fprintln(os.Stderr) + console.PrintInfoMessage("📥 No input schema defined") } // Display output schema if foundTool.OutputSchema != nil { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatSectionHeader("📤 Output Schema")) + fmt.Fprintln(os.Stderr) + console.PrintSectionHeader("📤 Output Schema") if schemaJSON, err := json.MarshalIndent(foundTool.OutputSchema, "", " "); err == nil { fmt.Fprintf(os.Stderr, "```json\n%s\n```\n", string(schemaJSON)) } else { fmt.Fprintf(os.Stderr, "Error displaying output schema: %v\n", err) } } else { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatInfoMessage("📤 No output schema defined")) + fmt.Fprintln(os.Stderr) + console.PrintInfoMessage("📤 No output schema defined") } fmt.Fprintln(os.Stderr) @@ -563,7 +577,8 @@ func displayToolAllowanceHint(info *parser.MCPServerInfo) { } if len(blockedTools) > 0 { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatInfoMessage("💡 To allow blocked tools, add them to your workflow frontmatter:")) + fmt.Fprintln(os.Stderr) + console.PrintInfoMessage("💡 To allow blocked tools, add them to your workflow frontmatter:") // Show the frontmatter syntax example fmt.Fprintf(os.Stderr, "\n") @@ -591,13 +606,16 @@ func displayToolAllowanceHint(info *parser.MCPServerInfo) { fmt.Fprintf(os.Stderr, "```\n") if len(blockedTools) > 3 { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatInfoMessage("📋 All blocked tools: "+strings.Join(blockedTools, ", "))) + fmt.Fprintln(os.Stderr) + console.PrintInfoMessage("📋 All blocked tools: " + strings.Join(blockedTools, ", ")) } } else if len(info.Config.Allowed) == 0 { // No explicit allowed list - all tools are allowed by default - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatInfoMessage("💡 All tools are currently allowed (no 'allowed' list specified)")) + fmt.Fprintln(os.Stderr) + console.PrintInfoMessage("💡 All tools are currently allowed (no 'allowed' list specified)") if len(info.Tools) > 0 { - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatInfoMessage("To restrict tools, add an 'allowed' list to your workflow frontmatter:")) + fmt.Fprintln(os.Stderr) + console.PrintInfoMessage("To restrict tools, add an 'allowed' list to your workflow frontmatter:") fmt.Fprintf(os.Stderr, "\n") fmt.Fprintf(os.Stderr, "```yaml\n") fmt.Fprintf(os.Stderr, "tools:\n") @@ -611,8 +629,10 @@ func displayToolAllowanceHint(info *parser.MCPServerInfo) { } } else { // All tools are explicitly allowed - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatSuccessMessage("✅ All available tools are explicitly allowed in your workflow")) + fmt.Fprintln(os.Stderr) + console.PrintSuccessMessage("✅ All available tools are explicitly allowed in your workflow") } - fmt.Fprintf(os.Stderr, "\n%s\n", console.FormatInfoMessage("📖 For more information, see: https://github.com/github/gh-aw/blob/main/docs/tools.md")) + fmt.Fprintln(os.Stderr) + console.PrintInfoMessage("📖 For more information, see: https://github.com/github/gh-aw/blob/main/docs/tools.md") } diff --git a/pkg/console/README.md b/pkg/console/README.md index 3f150bfb647..349609b3a2e 100644 --- a/pkg/console/README.md +++ b/pkg/console/README.md @@ -396,18 +396,16 @@ fmt.Fprintln(w, console.FormatListItemStderr("Detail item")) ```go import ( - "fmt" - "os" "github.com/github/gh-aw/pkg/console" ) -// Always write formatted messages to stderr -fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Workflow compiled successfully")) -fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Processing 3 files...")) -fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Network access is unrestricted")) -fmt.Fprintln(os.Stderr, console.FormatErrorMessage("File not found: workflow.md")) -fmt.Fprintln(os.Stderr, console.FormatCommandMessage("gh aw compile workflow.md")) -fmt.Fprintln(os.Stderr, console.FormatProgressMessage("Downloading release...")) +// Prefer direct stderr print helpers +console.PrintSuccessMessage("Workflow compiled successfully") +console.PrintInfoMessage("Processing 3 files...") +console.PrintWarningMessage("Network access is unrestricted") +console.PrintErrorMessage("File not found: workflow.md") +console.PrintCommandMessage("gh aw compile workflow.md") +console.PrintProgressMessage("Downloading release...") ``` ## Error Formatting diff --git a/pkg/console/console.go b/pkg/console/console.go index 06b5ee1df48..7ec24e04c78 100644 --- a/pkg/console/console.go +++ b/pkg/console/console.go @@ -221,6 +221,11 @@ func FormatWarningMessage(message string) string { return applyStyle(styles.Warning, "⚠ ") + message } +// FormatWarningMessageStderr formats a warning message for stderr output. +func FormatWarningMessageStderr(message string) string { + return applyStyleWithTTY(styles.Warning, "⚠ ", isStderrTTY) + message +} + // RenderTable renders a formatted table using lipgloss/table package func RenderTable(config TableConfig) string { if len(config.Headers) == 0 { @@ -316,21 +321,41 @@ func FormatCommandMessage(command string) string { return applyStyle(styles.Command, "$ ") + command } +// FormatCommandMessageStderr formats a command execution message for stderr output. +func FormatCommandMessageStderr(command string) string { + return applyStyleWithTTY(styles.Command, "$ ", isStderrTTY) + command +} + // FormatProgressMessage formats a progress/activity message func FormatProgressMessage(message string) string { return applyStyle(styles.Progress, "▸ ") + message } +// FormatProgressMessageStderr formats a progress/activity message for stderr output. +func FormatProgressMessageStderr(message string) string { + return applyStyleWithTTY(styles.Progress, "▸ ", isStderrTTY) + message +} + // FormatPromptMessage formats a user prompt message func FormatPromptMessage(message string) string { return applyStyle(styles.Prompt, "? ") + message } +// FormatPromptMessageStderr formats a user prompt message for stderr output. +func FormatPromptMessageStderr(message string) string { + return applyStyleWithTTY(styles.Prompt, "? ", isStderrTTY) + message +} + // FormatVerboseMessage formats verbose debugging output func FormatVerboseMessage(message string) string { return applyStyle(styles.Verbose, "» ") + message } +// FormatVerboseMessageStderr formats verbose debugging output for stderr output. +func FormatVerboseMessageStderr(message string) string { + return applyStyleWithTTY(styles.Verbose, "» ", isStderrTTY) + message +} + // FormatListItem formats an item in a list func FormatListItem(item string) string { return formatListItemWithTTY(item, isTTY, stdoutEnviron()) diff --git a/pkg/console/console_wasm.go b/pkg/console/console_wasm.go index 21684c45775..856c487884a 100644 --- a/pkg/console/console_wasm.go +++ b/pkg/console/console_wasm.go @@ -65,21 +65,41 @@ func FormatError(err CompilerError) string { return output.String() } -func FormatSuccessMessage(message string) string { return "✓ " + message } -func FormatInfoMessage(message string) string { return "i " + message } -func FormatTableHeaderStderr(text string) string { return text } -func FormatWarningMessage(message string) string { return "⚠ " + message } -func FormatErrorMessage(message string) string { return "✗ " + message } -func FormatErrorTextStderr(text string) string { return text } -func FormatLocationMessage(message string) string { return "~ " + message } -func FormatCommandMessage(command string) string { return "$ " + command } -func FormatProgressMessage(message string) string { return "▸ " + message } -func FormatPromptMessage(message string) string { return "? " + message } -func FormatCountMessage(message string) string { return "# " + message } -func FormatVerboseMessage(message string) string { return "» " + message } -func FormatListHeader(header string) string { return header } -func FormatListItem(item string) string { return " • " + item } -func FormatSectionHeader(header string) string { return header } +func FormatSuccessMessage(message string) string { return "✓ " + message } +func FormatSuccessMessageStderr(message string) string { return "✓ " + message } +func FormatInfoMessage(message string) string { return "i " + message } +func FormatInfoMessageStderr(message string) string { return "i " + message } +func FormatTableHeaderStderr(text string) string { return text } +func FormatWarningMessage(message string) string { return "⚠ " + message } +func FormatWarningMessageStderr(message string) string { return "⚠ " + message } +func FormatErrorMessage(message string) string { return "✗ " + message } +func FormatErrorTextStderr(text string) string { return text } +func FormatLocationMessage(message string) string { return "~ " + message } +func FormatCommandMessage(command string) string { return "$ " + command } +func FormatCommandMessageStderr(command string) string { return "$ " + command } +func FormatProgressMessage(message string) string { return "▸ " + message } +func FormatProgressMessageStderr(message string) string { return "▸ " + message } +func FormatPromptMessage(message string) string { return "? " + message } +func FormatPromptMessageStderr(message string) string { return "? " + message } +func FormatCountMessage(message string) string { return "# " + message } +func FormatVerboseMessage(message string) string { return "» " + message } +func FormatVerboseMessageStderr(message string) string { return "» " + message } +func FormatListHeader(header string) string { return header } +func FormatListItem(item string) string { return " • " + item } +func FormatListItemStderr(item string) string { return " • " + item } +func FormatSectionHeader(header string) string { return header } +func FormatSectionHeaderStderr(header string) string { return header } + +// FormatErrorChain formats an error and its full unwrapped chain. +// In the WASM build there is no rich terminal styling; the top-level error +// message is sufficient for the JSON-based diagnostics that WASM consumers +// produce. Full chain formatting is provided by the non-WASM implementation. +func FormatErrorChain(err error) string { + if err == nil { + return "" + } + return "✗ " + err.Error() +} func RenderTable(config TableConfig) string { if len(config.Headers) == 0 { diff --git a/pkg/console/doc.go b/pkg/console/doc.go index 45a5101151c..9152ae63fc2 100644 --- a/pkg/console/doc.go +++ b/pkg/console/doc.go @@ -21,5 +21,5 @@ // // All diagnostic output (messages, warnings, errors) should be written to stderr. // Structured data output (JSON, hashes, graphs) should be written to stdout. -// Use fmt.Fprintln(os.Stderr, ...) with the Format* helpers for diagnostic output. +// Prefer Print* helpers (or fmt.Fprintln(os.Stderr, ...) with Format* helpers) for diagnostic output. package console diff --git a/pkg/console/print.go b/pkg/console/print.go new file mode 100644 index 00000000000..c5420127eeb --- /dev/null +++ b/pkg/console/print.go @@ -0,0 +1,84 @@ +package console + +import ( + "fmt" + "io" + "os" +) + +// stderr is the writer used by all Print* helpers. Tests may replace it with a +// bytes.Buffer to capture output without touching OS file descriptors. +// Tests must not call t.Parallel() as this variable is not concurrency-safe. +var stderr io.Writer = os.Stderr + +// PrintError formats and prints a compiler error to stderr. +// FormatError already includes a trailing newline, so Fprint is used to avoid +// emitting a spurious blank line. +func PrintError(err CompilerError) { + fmt.Fprint(stderr, FormatError(err)) +} + +// PrintSuccessMessage formats and prints a success message to stderr. +func PrintSuccessMessage(message string) { + fmt.Fprintln(stderr, FormatSuccessMessageStderr(message)) +} + +// PrintInfoMessage formats and prints an info message to stderr. +func PrintInfoMessage(message string) { + fmt.Fprintln(stderr, FormatInfoMessageStderr(message)) +} + +// PrintTableHeaderStderr formats and prints a table header to stderr. +func PrintTableHeaderStderr(text string) { + fmt.Fprintln(stderr, FormatTableHeaderStderr(text)) +} + +// PrintWarningMessage formats and prints a warning message to stderr. +func PrintWarningMessage(message string) { + fmt.Fprintln(stderr, FormatWarningMessageStderr(message)) +} + +// PrintErrorMessage formats and prints a simple error message to stderr. +func PrintErrorMessage(message string) { + fmt.Fprintln(stderr, FormatErrorMessage(message)) +} + +// PrintErrorTextStderr formats and prints error-styled text to stderr. +func PrintErrorTextStderr(text string) { + fmt.Fprintln(stderr, FormatErrorTextStderr(text)) +} + +// PrintCommandMessage formats and prints a command message to stderr. +func PrintCommandMessage(command string) { + fmt.Fprintln(stderr, FormatCommandMessageStderr(command)) +} + +// PrintProgressMessage formats and prints a progress message to stderr. +func PrintProgressMessage(message string) { + fmt.Fprintln(stderr, FormatProgressMessageStderr(message)) +} + +// PrintPromptMessage formats and prints a prompt message to stderr. +func PrintPromptMessage(message string) { + fmt.Fprintln(stderr, FormatPromptMessageStderr(message)) +} + +// PrintVerboseMessage formats and prints a verbose message to stderr. +func PrintVerboseMessage(message string) { + fmt.Fprintln(stderr, FormatVerboseMessageStderr(message)) +} + +// PrintListItem formats and prints a list item to stderr. +func PrintListItem(item string) { + fmt.Fprintln(stderr, FormatListItemStderr(item)) +} + +// PrintSectionHeader formats and prints a section header to stderr. +func PrintSectionHeader(header string) { + fmt.Fprintln(stderr, FormatSectionHeaderStderr(header)) +} + +// PrintErrorChain formats and prints an error chain to stderr. +func PrintErrorChain(err error) { + fmt.Fprintln(stderr, FormatErrorChain(err)) +} diff --git a/pkg/console/print_test.go b/pkg/console/print_test.go new file mode 100644 index 00000000000..ab8fe50060d --- /dev/null +++ b/pkg/console/print_test.go @@ -0,0 +1,144 @@ +//go:build !integration + +package console + +import ( + "bytes" + "errors" + "fmt" + "testing" +) + +// captureStderr redirects Print* output to an in-memory buffer by swapping the +// package-level stderr variable. Tests using this helper must not call +// t.Parallel() because the variable is not concurrency-safe. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + var buf bytes.Buffer + old := stderr + stderr = &buf + defer func() { stderr = old }() + fn() + return buf.String() +} + +// TestPrintErrorNewline verifies that PrintError does not emit a spurious blank +// line: FormatError already terminates with \n, so Fprint (not Fprintln) must +// be used. +func TestPrintErrorNewline(t *testing.T) { + ce := CompilerError{Type: "error", Message: "something went wrong"} + got := captureStderr(t, func() { PrintError(ce) }) + + want := FormatError(ce) + if got != want { + t.Fatalf("PrintError output mismatch\nwant: %q\ngot: %q", want, got) + } +} + +func TestPrintSuccessMessage(t *testing.T) { + got := captureStderr(t, func() { PrintSuccessMessage("ok") }) + want := FormatSuccessMessageStderr("ok") + "\n" + if got != want { + t.Fatalf("want %q, got %q", want, got) + } +} + +func TestPrintInfoMessage(t *testing.T) { + got := captureStderr(t, func() { PrintInfoMessage("info") }) + want := FormatInfoMessageStderr("info") + "\n" + if got != want { + t.Fatalf("want %q, got %q", want, got) + } +} + +func TestPrintTableHeaderStderr(t *testing.T) { + got := captureStderr(t, func() { PrintTableHeaderStderr("Name") }) + want := FormatTableHeaderStderr("Name") + "\n" + if got != want { + t.Fatalf("want %q, got %q", want, got) + } +} + +func TestPrintWarningMessage(t *testing.T) { + got := captureStderr(t, func() { PrintWarningMessage("warn") }) + want := FormatWarningMessageStderr("warn") + "\n" + if got != want { + t.Fatalf("want %q, got %q", want, got) + } +} + +func TestPrintErrorMessage(t *testing.T) { + got := captureStderr(t, func() { PrintErrorMessage("boom") }) + want := FormatErrorMessage("boom") + "\n" + if got != want { + t.Fatalf("want %q, got %q", want, got) + } +} + +func TestPrintErrorTextStderr(t *testing.T) { + got := captureStderr(t, func() { PrintErrorTextStderr("error text") }) + want := FormatErrorTextStderr("error text") + "\n" + if got != want { + t.Fatalf("want %q, got %q", want, got) + } +} + +func TestPrintCommandMessage(t *testing.T) { + got := captureStderr(t, func() { PrintCommandMessage("gh aw status") }) + want := FormatCommandMessageStderr("gh aw status") + "\n" + if got != want { + t.Fatalf("want %q, got %q", want, got) + } +} + +func TestPrintProgressMessage(t *testing.T) { + got := captureStderr(t, func() { PrintProgressMessage("loading") }) + want := FormatProgressMessageStderr("loading") + "\n" + if got != want { + t.Fatalf("want %q, got %q", want, got) + } +} + +func TestPrintPromptMessage(t *testing.T) { + got := captureStderr(t, func() { PrintPromptMessage("continue?") }) + want := FormatPromptMessageStderr("continue?") + "\n" + if got != want { + t.Fatalf("want %q, got %q", want, got) + } +} + +func TestPrintVerboseMessage(t *testing.T) { + got := captureStderr(t, func() { PrintVerboseMessage("debug info") }) + want := FormatVerboseMessageStderr("debug info") + "\n" + if got != want { + t.Fatalf("want %q, got %q", want, got) + } +} + +func TestPrintListItem(t *testing.T) { + got := captureStderr(t, func() { PrintListItem("item one") }) + want := FormatListItemStderr("item one") + "\n" + if got != want { + t.Fatalf("want %q, got %q", want, got) + } +} + +func TestPrintSectionHeader(t *testing.T) { + got := captureStderr(t, func() { PrintSectionHeader("Section") }) + want := FormatSectionHeaderStderr("Section") + "\n" + if got != want { + t.Fatalf("want %q, got %q", want, got) + } +} + +func TestPrintErrorChain(t *testing.T) { + err := errors.New("inner") + wrapped := fmt.Errorf("outer: %w", err) + + got := captureStderr(t, func() { PrintErrorChain(wrapped) }) + + want := FormatErrorChain(wrapped) + "\n" + if got != want { + t.Fatalf("expected %q, got %q", want, got) + } +}