From 8437246aa1f4fb2033fa02eb1d28c41a9b6cafc8 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Tue, 21 Apr 2026 15:22:42 -0400 Subject: [PATCH 1/2] Surface request IDs in CLI errors --- internal/commands/api.go | 6 ++-- internal/commands/api_test.go | 23 ++++++++++++ internal/commands/projects.go | 1 + internal/output/envelope.go | 3 ++ internal/output/errors.go | 26 +++++++++++++- internal/output/output_test.go | 64 ++++++++++++++++++++++++++++++++++ internal/output/render.go | 19 ++++++++++ 7 files changed, 138 insertions(+), 4 deletions(-) diff --git a/internal/commands/api.go b/internal/commands/api.go index 40e1b48f..a2223e6a 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -98,7 +98,7 @@ func newAPIPostCmd() *cobra.Command { resp, err := app.Account().Post(cmd.Context(), path, body) if err != nil { - return err + return convertSDKError(err) } summary := fmt.Sprintf("POST %s: %s", path, apiSummary(resp.Data)) @@ -147,7 +147,7 @@ func newAPIPutCmd() *cobra.Command { resp, err := app.Account().Put(cmd.Context(), path, body) if err != nil { - return err + return convertSDKError(err) } summary := fmt.Sprintf("PUT %s: %s", path, apiSummary(resp.Data)) @@ -179,7 +179,7 @@ func newAPIDeleteCmd() *cobra.Command { path := parsePath(args[0]) resp, err := app.Account().Delete(cmd.Context(), path) if err != nil { - return err + return convertSDKError(err) } summary := fmt.Sprintf("DELETE %s", path) diff --git a/internal/commands/api_test.go b/internal/commands/api_test.go index 340da950..103ec1f0 100644 --- a/internal/commands/api_test.go +++ b/internal/commands/api_test.go @@ -1,11 +1,15 @@ package commands import ( + "errors" "testing" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/output" ) func TestParsePath(t *testing.T) { @@ -47,3 +51,22 @@ func TestAPIPathArgs(t *testing.T) { assert.Contains(t, err.Error(), "path required") }) } + +func TestConvertSDKErrorPreservesRequestID(t *testing.T) { + sdkErr := &basecamp.Error{ + Code: basecamp.CodeAPI, + Message: "server error", + HTTPStatus: 500, + RequestID: "req-cli-123", + } + + err := convertSDKError(sdkErr) + + var outErr *output.Error + require.True(t, errors.As(err, &outErr), "expected *output.Error, got %T", err) + assert.Equal(t, output.CodeAPI, outErr.Code) + + var gotSDK *basecamp.Error + require.True(t, errors.As(err, &gotSDK), "expected wrapped *basecamp.Error, got %T", err) + assert.Equal(t, "req-cli-123", gotSDK.RequestID) +} diff --git a/internal/commands/projects.go b/internal/commands/projects.go index 3518bf7f..33bb7825 100644 --- a/internal/commands/projects.go +++ b/internal/commands/projects.go @@ -445,6 +445,7 @@ func convertSDKError(err error) error { Hint: sdkErr.Hint, HTTPStatus: sdkErr.HTTPStatus, Retryable: sdkErr.Retryable, + Cause: sdkErr, } } return err diff --git a/internal/output/envelope.go b/internal/output/envelope.go index e70469f6..feb53b6a 100644 --- a/internal/output/envelope.go +++ b/internal/output/envelope.go @@ -157,6 +157,9 @@ func (w *Writer) Err(err error, opts ...ErrorResponseOption) error { Code: e.Code, Hint: e.Hint, } + if requestID := RequestID(err); requestID != "" { + resp.Meta = map[string]any{"request_id": requestID} + } for _, opt := range opts { opt(resp) } diff --git a/internal/output/errors.go b/internal/output/errors.go index d908c754..59e33c23 100644 --- a/internal/output/errors.go +++ b/internal/output/errors.go @@ -5,6 +5,7 @@ import ( "fmt" "strings" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" clioutput "github.com/basecamp/cli/output" ) @@ -29,7 +30,30 @@ func ErrAPI(status int, msg string) *Error { return clioutput.ErrAPI(status, msg func ErrAmbiguous(resource string, matches []string) *Error { return clioutput.ErrAmbiguous(resource, matches) } -func AsError(err error) *Error { return clioutput.AsError(err) } + +func AsError(err error) *Error { + var sdkErr *basecamp.Error + if errors.As(err, &sdkErr) { + return &Error{ + Code: sdkErr.Code, + Message: sdkErr.Message, + Hint: sdkErr.Hint, + HTTPStatus: sdkErr.HTTPStatus, + Retryable: sdkErr.Retryable, + Cause: sdkErr, + } + } + return clioutput.AsError(err) +} + +// RequestID returns the SDK request ID carried by err, if present. +func RequestID(err error) string { + var sdkErr *basecamp.Error + if errors.As(err, &sdkErr) { + return sdkErr.RequestID + } + return "" +} // App-specific error constructors with basecamp-cli hints. diff --git a/internal/output/output_test.go b/internal/output/output_test.go index e4c5adc1..9fad60f4 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -14,6 +14,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + "github.com/basecamp/basecamp-cli/internal/observability" ) @@ -422,6 +424,30 @@ func TestWriterErr(t *testing.T) { assert.Equal(t, CodeNotFound, resp.Code) } +func TestWriterErrIncludesRequestIDMeta(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatJSON, + Writer: &buf, + }) + + err := w.Err(&basecamp.Error{ + Code: basecamp.CodeAPI, + Message: "server error", + HTTPStatus: 500, + RequestID: "req-cli-123", + }) + require.NoError(t, err, "Err() failed") + + var resp ErrorResponse + require.NoError(t, json.Unmarshal(buf.Bytes(), &resp), "Failed to unmarshal output") + + assert.False(t, resp.OK) + assert.Equal(t, CodeAPI, resp.Code) + require.NotNil(t, resp.Meta) + assert.Equal(t, "req-cli-123", resp.Meta["request_id"]) +} + func TestWriterQuietFormat(t *testing.T) { var buf bytes.Buffer w := New(Options{ @@ -2213,6 +2239,44 @@ func TestWriterStyledErrorWithHint(t *testing.T) { assert.Contains(t, output, "\x1b[", "Expected ANSI escape codes in styled output") } +func TestWriterStyledErrorIncludesRequestID(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatStyled, + Writer: &buf, + }) + + writeErr := w.Err(&basecamp.Error{ + Code: basecamp.CodeAPI, + Message: "server error", + HTTPStatus: 500, + RequestID: "req-cli-123", + }) + require.NoError(t, writeErr, "Err() failed") + + output := buf.String() + assert.Contains(t, output, "Request ID: req-cli-123") +} + +func TestWriterMarkdownErrorIncludesRequestID(t *testing.T) { + var buf bytes.Buffer + w := New(Options{ + Format: FormatMarkdown, + Writer: &buf, + }) + + writeErr := w.Err(&basecamp.Error{ + Code: basecamp.CodeAPI, + Message: "server error", + HTTPStatus: 500, + RequestID: "req-cli-123", + }) + require.NoError(t, writeErr, "Err() failed") + + output := buf.String() + assert.Contains(t, output, "Request ID: req-cli-123") +} + // ============================================================================= // Terminal Escape Injection Defense Tests // ============================================================================= diff --git a/internal/output/render.go b/internal/output/render.go index d45a2d5b..2bf9a3f2 100644 --- a/internal/output/render.go +++ b/internal/output/render.go @@ -189,6 +189,10 @@ func (r *Renderer) RenderError(w io.Writer, resp *ErrorResponse) error { } } } + if requestID := errorRequestID(resp); requestID != "" { + contentLines = append(contentLines, "") + contentLines = append(contentLines, r.Hint.Render("Request ID: "+requestID)) + } // Create bordered box with error color border boxStyle := lipgloss.NewStyle(). @@ -208,12 +212,24 @@ func (r *Renderer) RenderError(w io.Writer, resp *ErrorResponse) error { b.WriteString("Hint: " + resp.Hint) b.WriteString("\n") } + if requestID := errorRequestID(resp); requestID != "" { + b.WriteString("Request ID: " + requestID) + b.WriteString("\n") + } } _, err := io.WriteString(w, b.String()) return err } +func errorRequestID(resp *ErrorResponse) string { + if resp == nil || resp.Meta == nil { + return "" + } + requestID, _ := resp.Meta["request_id"].(string) + return requestID +} + // wrapText wraps text to fit within maxWidth, preserving words and newlines. // Uses display-cell width for proper Unicode support. func wrapText(text string, maxWidth int) string { @@ -1164,6 +1180,9 @@ func (r *MarkdownRenderer) RenderError(w io.Writer, resp *ErrorResponse) error { if resp.Hint != "" { b.WriteString("\n*Hint: " + resp.Hint + "*\n") } + if requestID := errorRequestID(resp); requestID != "" { + b.WriteString("\n*Request ID: " + requestID + "*\n") + } _, err := io.WriteString(w, b.String()) return err From 5977027f5c61b656ec119bfdeb539aa543d381a1 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Tue, 21 Apr 2026 15:53:44 -0400 Subject: [PATCH 2/2] Address request ID review feedback --- internal/names/resolver.go | 1 + internal/output/envelope.go | 5 ++- internal/output/errors.go | 9 ++++- internal/output/output_test.go | 29 +++++++++++--- internal/output/render.go | 73 +++++++++++++++++++++++++++++++--- 5 files changed, 105 insertions(+), 12 deletions(-) diff --git a/internal/names/resolver.go b/internal/names/resolver.go index 6e07440a..b8957c42 100644 --- a/internal/names/resolver.go +++ b/internal/names/resolver.go @@ -724,6 +724,7 @@ func convertSDKError(err error) error { Hint: sdkErr.Hint, HTTPStatus: sdkErr.HTTPStatus, Retryable: sdkErr.Retryable, + Cause: sdkErr, } } return err diff --git a/internal/output/envelope.go b/internal/output/envelope.go index feb53b6a..1bf01cd8 100644 --- a/internal/output/envelope.go +++ b/internal/output/envelope.go @@ -158,7 +158,10 @@ func (w *Writer) Err(err error, opts ...ErrorResponseOption) error { Hint: e.Hint, } if requestID := RequestID(err); requestID != "" { - resp.Meta = map[string]any{"request_id": requestID} + if resp.Meta == nil { + resp.Meta = make(map[string]any) + } + resp.Meta["request_id"] = requestID } for _, opt := range opts { opt(resp) diff --git a/internal/output/errors.go b/internal/output/errors.go index 59e33c23..76c6676a 100644 --- a/internal/output/errors.go +++ b/internal/output/errors.go @@ -34,9 +34,16 @@ func ErrAmbiguous(resource string, matches []string) *Error { func AsError(err error) *Error { var sdkErr *basecamp.Error if errors.As(err, &sdkErr) { + message := err.Error() + if sdkErr.Hint != "" { + message = strings.TrimSuffix(message, ": "+sdkErr.Hint) + } + if message == "" { + message = sdkErr.Message + } return &Error{ Code: sdkErr.Code, - Message: sdkErr.Message, + Message: message, Hint: sdkErr.Hint, HTTPStatus: sdkErr.HTTPStatus, Retryable: sdkErr.Retryable, diff --git a/internal/output/output_test.go b/internal/output/output_test.go index 9fad60f4..91147db4 100644 --- a/internal/output/output_test.go +++ b/internal/output/output_test.go @@ -325,6 +325,23 @@ func TestAsErrorWithWrappedOutputError(t *testing.T) { assert.Equal(t, CodeAuth, result.Code) } +func TestAsErrorWithWrappedSDKErrorPreservesContext(t *testing.T) { + original := &basecamp.Error{ + Code: basecamp.CodeForbidden, + Message: "access denied", + Hint: "retry later", + HTTPStatus: 403, + RequestID: "req-cli-123", + } + wrapped := fmt.Errorf("GET /projects.json: %w", original) + + result := AsError(wrapped) + assert.Equal(t, CodeForbidden, result.Code) + assert.Equal(t, "GET /projects.json: access denied", result.Message) + assert.Equal(t, "retry later", result.Hint) + assert.Equal(t, original, result.Cause) +} + // Note: AsError(nil) panics because it calls err.Error() on nil. // This is expected behavior - callers should not pass nil to AsError. @@ -2250,12 +2267,13 @@ func TestWriterStyledErrorIncludesRequestID(t *testing.T) { Code: basecamp.CodeAPI, Message: "server error", HTTPStatus: 500, - RequestID: "req-cli-123", + RequestID: "req-cli-123\x1b[31m\nnext", }) require.NoError(t, writeErr, "Err() failed") - output := buf.String() - assert.Contains(t, output, "Request ID: req-cli-123") + output := ansi.Strip(buf.String()) + assert.Contains(t, output, "Request ID: req-cli-123 next") + assert.NotContains(t, output, "[31m") } func TestWriterMarkdownErrorIncludesRequestID(t *testing.T) { @@ -2269,12 +2287,13 @@ func TestWriterMarkdownErrorIncludesRequestID(t *testing.T) { Code: basecamp.CodeAPI, Message: "server error", HTTPStatus: 500, - RequestID: "req-cli-123", + RequestID: "req*cli`123\x1b[31m", }) require.NoError(t, writeErr, "Err() failed") output := buf.String() - assert.Contains(t, output, "Request ID: req-cli-123") + assert.Contains(t, output, "Request ID: req\\*cli\\`123") + assert.NotContains(t, output, "\x1b") } // ============================================================================= diff --git a/internal/output/render.go b/internal/output/render.go index 2bf9a3f2..7b973273 100644 --- a/internal/output/render.go +++ b/internal/output/render.go @@ -9,6 +9,7 @@ import ( "sort" "strings" "time" + "unicode" "charm.land/lipgloss/v2" "charm.land/lipgloss/v2/table" @@ -191,7 +192,9 @@ func (r *Renderer) RenderError(w io.Writer, resp *ErrorResponse) error { } if requestID := errorRequestID(resp); requestID != "" { contentLines = append(contentLines, "") - contentLines = append(contentLines, r.Hint.Render("Request ID: "+requestID)) + for _, line := range wrappedRequestIDLines(requestID, maxWidth) { + contentLines = append(contentLines, r.Hint.Render(line)) + } } // Create bordered box with error color border @@ -213,8 +216,10 @@ func (r *Renderer) RenderError(w io.Writer, resp *ErrorResponse) error { b.WriteString("\n") } if requestID := errorRequestID(resp); requestID != "" { - b.WriteString("Request ID: " + requestID) - b.WriteString("\n") + for _, line := range wrappedRequestIDLines(requestID, r.width) { + b.WriteString(line) + b.WriteString("\n") + } } } @@ -230,6 +235,64 @@ func errorRequestID(resp *ErrorResponse) string { return requestID } +func wrappedRequestIDLines(requestID string, width int) []string { + const prefix = "Request ID: " + sanitized := sanitizeRequestID(requestID) + if sanitized == "" { + return nil + } + wrapped := wrapText(sanitized, max(width-len(prefix), 1)) + lines := strings.Split(wrapped, "\n") + if len(lines) == 0 { + return nil + } + out := make([]string, 0, len(lines)) + for i, line := range lines { + if i == 0 { + out = append(out, prefix+line) + continue + } + out = append(out, strings.Repeat(" ", len(prefix))+line) + } + return out +} + +func sanitizeRequestID(requestID string) string { + requestID = ansi.Strip(requestID) + requestID = strings.Map(func(r rune) rune { + switch { + case r == '\n' || r == '\r' || r == '\t': + return ' ' + case unicode.IsControl(r): + return -1 + default: + return r + } + }, requestID) + return strings.Join(strings.Fields(requestID), " ") +} + +func escapeMarkdownText(s string) string { + replacer := strings.NewReplacer( + `\\`, `\\`, + "`", "\\`", + "*", "\\*", + "_", "\\_", + "{", "\\{", + "}", "\\}", + "[", "\\[", + "]", "\\]", + "(", "\\(", + ")", "\\)", + "#", "\\#", + "+", "\\+", + "-", "\\-", + "!", "\\!", + "|", "\\|", + ) + return replacer.Replace(s) +} + // wrapText wraps text to fit within maxWidth, preserving words and newlines. // Uses display-cell width for proper Unicode support. func wrapText(text string, maxWidth int) string { @@ -1180,8 +1243,8 @@ func (r *MarkdownRenderer) RenderError(w io.Writer, resp *ErrorResponse) error { if resp.Hint != "" { b.WriteString("\n*Hint: " + resp.Hint + "*\n") } - if requestID := errorRequestID(resp); requestID != "" { - b.WriteString("\n*Request ID: " + requestID + "*\n") + if requestID := sanitizeRequestID(errorRequestID(resp)); requestID != "" { + b.WriteString("\nRequest ID: " + escapeMarkdownText(requestID) + "\n") } _, err := io.WriteString(w, b.String())