From a8bd22bdea9cdf1734ac2b955efc313a6a1b3ad4 Mon Sep 17 00:00:00 2001 From: Somansh Reddy Satish Date: Tue, 7 Jul 2026 07:01:21 +0000 Subject: [PATCH 1/5] cli: reclassify parse/read failures + add source/http_status telemetry Reclassify the remaining opaque `error` sites: a mid-response read failure -> network_error; JSON-parse and poll field-extraction failures -> response_parse_error. Capture the request id from x-amzn-request-id / x-amzn-trace-id (the app does not set X-Request-Id; the ALB does), falling back to X-Request-Id. Add telemetry-only origin signals to COMMAND_RUN_COMPLETE: source (api = from the API envelope, cli = CLI-generated; defaults to cli) and http_status (upstream status when known). Both are omitted on success and are NOT in the user-facing error envelope. Dashboards can then group by (source, error_code) collision-safely and separate an app 5xx from a no-envelope 5xx via source. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/heygen/main.go | 10 ++++- internal/analytics/analytics.go | 22 ++++++++--- internal/analytics/analytics_test.go | 40 ++++++++++++++++++- internal/client/executor.go | 51 ++++++++++++++++++------ internal/client/executor_test.go | 58 ++++++++++++++++++++++++++++ internal/errors/errors.go | 27 +++++++++---- internal/errors/errors_test.go | 21 ++++++++++ 7 files changed, 202 insertions(+), 27 deletions(-) diff --git a/cmd/heygen/main.go b/cmd/heygen/main.go index fe4dc73a..b8ced414 100644 --- a/cmd/heygen/main.go +++ b/cmd/heygen/main.go @@ -26,6 +26,8 @@ func main() { exitCode := 0 errorCode := "" + source := "" + httpStatus := 0 if err != nil { var cliErr *clierrors.CLIError if errors.As(err, &cliErr) { @@ -33,6 +35,11 @@ func main() { formatter.Error(cliErr) exitCode = cliErr.ExitCode errorCode = cliErr.Code + source = cliErr.Source + if source == "" { + source = "cli" // any error the CLI raised without an explicit origin + } + httpStatus = cliErr.HTTPStatus } else { // Cobra returns plain errors for unknown commands and arg validation. // Detect these and wrap as usage errors (exit 2). @@ -40,11 +47,12 @@ func main() { formatter.Error(wrapped) exitCode = wrapped.ExitCode errorCode = wrapped.Code + source = "cli" } } if analyticsClient.Started() && executedCmd != nil { - analyticsClient.CommandRunComplete(executedCmd.CommandPath(), exitCode, time.Since(start), errorCode) + analyticsClient.CommandRunComplete(executedCmd.CommandPath(), exitCode, time.Since(start), errorCode, source, httpStatus) } analyticsClient.Close() os.Exit(exitCode) diff --git a/internal/analytics/analytics.go b/internal/analytics/analytics.go index b2f18a9b..f35e06f1 100644 --- a/internal/analytics/analytics.go +++ b/internal/analytics/analytics.go @@ -70,18 +70,28 @@ func (c *Client) CommandRun(command string) { }) } -func (c *Client) CommandRunComplete(command string, exitCode int, duration time.Duration, errorCode string) { +// CommandRunComplete records a completed command. On error, source is "api" (the +// error came from an API response envelope) or "cli" (CLI-generated), and httpStatus +// is the upstream status when known (0 otherwise); both are omitted on success. +func (c *Client) CommandRunComplete(command string, exitCode int, duration time.Duration, errorCode, source string, httpStatus int) { if !c.enabled || c.ph == nil { return } + props := c.baseProperties(command). + Set("exit_code", exitCode). + Set("duration_ms", duration.Milliseconds()). + Set("success", exitCode == 0). + Set("error_code", errorCode) + if source != "" { + props = props.Set("source", source) + } + if httpStatus > 0 { + props = props.Set("http_status", httpStatus) + } _ = c.ph.Enqueue(posthog.Capture{ DistinctId: c.distinctID, Event: "COMMAND_RUN_COMPLETE", - Properties: c.baseProperties(command). - Set("exit_code", exitCode). - Set("duration_ms", duration.Milliseconds()). - Set("success", exitCode == 0). - Set("error_code", errorCode), + Properties: props, }) } diff --git a/internal/analytics/analytics_test.go b/internal/analytics/analytics_test.go index 4d8bcc33..dc41b63d 100644 --- a/internal/analytics/analytics_test.go +++ b/internal/analytics/analytics_test.go @@ -59,7 +59,7 @@ func TestCommandRunComplete_Properties(t *testing.T) { client := newWithCapture("v1.2.3", stub) client.distinctID = "anon-id" - client.CommandRunComplete("heygen video create", 4, 1500*time.Millisecond, "timeout") + client.CommandRunComplete("heygen video create", 4, 1500*time.Millisecond, "timeout", "cli", 0) if len(stub.messages) != 1 { t.Fatalf("messages = %d, want 1", len(stub.messages)) @@ -107,7 +107,7 @@ func TestCommandRunComplete_IncludesClientOrigin(t *testing.T) { client := newWithCapture("v1.2.3", stub) client.clientOrigin = "claude_code" - client.CommandRunComplete("heygen video create", 0, time.Second, "") + client.CommandRunComplete("heygen video create", 0, time.Second, "", "", 0) msg := stub.messages[0].(posthog.Capture) if got := msg.Properties["client_origin"]; got != "claude_code" { @@ -225,3 +225,39 @@ func TestDistinctID_Persists(t *testing.T) { t.Fatalf("second distinct ID = %q, want %q", second, first) } } + +func TestCommandRunComplete_SourceAndHTTPStatus(t *testing.T) { + t.Run("api source carries http_status", func(t *testing.T) { + stub := &stubCaptureClient{} + client := newWithCapture("v1.2.3", stub) + client.CommandRunComplete("heygen video create", 1, time.Second, "insufficient_credit", "api", 402) + msg := stub.messages[0].(posthog.Capture) + if got := msg.Properties["source"]; got != "api" { + t.Fatalf("source = %v, want api", got) + } + if got := msg.Properties["http_status"]; got != 402 { + t.Fatalf("http_status = %v, want 402", got) + } + }) + t.Run("cli source omits http_status when 0", func(t *testing.T) { + stub := &stubCaptureClient{} + client := newWithCapture("v1.2.3", stub) + client.CommandRunComplete("heygen video create", 1, time.Second, "file_io_error", "cli", 0) + msg := stub.messages[0].(posthog.Capture) + if got := msg.Properties["source"]; got != "cli" { + t.Fatalf("source = %v, want cli", got) + } + if _, ok := msg.Properties["http_status"]; ok { + t.Fatalf("http_status should be omitted when 0") + } + }) + t.Run("success omits source", func(t *testing.T) { + stub := &stubCaptureClient{} + client := newWithCapture("v1.2.3", stub) + client.CommandRunComplete("heygen video list", 0, time.Second, "", "", 0) + msg := stub.messages[0].(posthog.Capture) + if _, ok := msg.Properties["source"]; ok { + t.Fatalf("source should be omitted on success") + } + }) +} diff --git a/internal/client/executor.go b/internal/client/executor.go index 6d3857e3..d00a1831 100644 --- a/internal/client/executor.go +++ b/internal/client/executor.go @@ -81,10 +81,13 @@ func (c *Client) ExecuteAndPoll( resourceID, err := extractJSONPath(createResp, spec.PollConfig.IDField) if err != nil { - return nil, clierrors.New(fmt.Sprintf( - "failed to extract resource ID from %q: %v. This command may require manual polling for batch responses", - spec.PollConfig.IDField, err, - )) + return nil, &clierrors.CLIError{ + Code: "response_parse_error", + Message: fmt.Sprintf( + "failed to extract resource ID from %q: %v", spec.PollConfig.IDField, err), + Hint: "The create response did not have the expected shape. This command may require manual polling for batch responses.", + ExitCode: clierrors.ExitGeneral, + } } // If IDField uses an array index (e.g., "data.ids.0"), reject batch @@ -131,10 +134,13 @@ func (c *Client) ExecuteAndPoll( status, err := extractJSONPath(statusResp, spec.PollConfig.StatusField) if err != nil { - return nil, clierrors.New(fmt.Sprintf( - "failed to extract status from %q: %v", - spec.PollConfig.StatusField, err, - )) + return nil, &clierrors.CLIError{ + Code: "response_parse_error", + Message: fmt.Sprintf( + "failed to extract status from %q: %v", spec.PollConfig.StatusField, err), + Hint: "The status response did not have the expected shape. Retry; if it persists, report it.", + ExitCode: clierrors.ExitGeneral, + } } if slices.Contains(spec.PollConfig.TerminalOK, status) { @@ -212,11 +218,17 @@ func (c *Client) executeWithContext(ctx context.Context, spec *command.Spec, inv respBody, err := io.ReadAll(resp.Body) if err != nil { - return nil, clierrors.New(fmt.Sprintf("failed to read response body: %v", err)) + // Mid-response read failure is a dropped connection, not a parse issue. + return nil, &clierrors.CLIError{ + Code: "network_error", + Message: fmt.Sprintf("failed to read response body: %v", err), + Hint: "This is usually a temporary network issue. Check your connection and retry.", + ExitCode: clierrors.ExitGeneral, + } } if resp.StatusCode >= 400 { - return nil, parseErrorResponse(resp.StatusCode, respBody, resp.Header.Get("X-Request-Id")) + return nil, parseErrorResponse(resp.StatusCode, respBody, requestIDFromHeaders(resp.Header)) } return json.RawMessage(respBody), nil @@ -394,7 +406,12 @@ func extractJSONPath(raw json.RawMessage, path string) (string, error) { var current any if err := json.Unmarshal(raw, ¤t); err != nil { - return "", clierrors.New(fmt.Sprintf("failed to parse JSON response: %v", err)) + return "", &clierrors.CLIError{ + Code: "response_parse_error", + Message: fmt.Sprintf("failed to parse JSON response: %v", err), + Hint: "The API response could not be parsed. Retry; if it persists, report it (possible CLI/API mismatch).", + ExitCode: clierrors.ExitGeneral, + } } for _, part := range strings.Split(path, ".") { @@ -515,3 +532,15 @@ func parseErrorResponse(statusCode int, body []byte, requestID string) *clierror return clierrors.FromAPIError(statusCode, &clierrors.APIError{}, requestID) } + +// requestIDFromHeaders returns the best available request/trace id for support +// correlation. The v3 app does not set X-Request-Id; the AWS ALB sets x-amzn-*, so +// prefer the app header when present, then fall back to the ALB's. +func requestIDFromHeaders(h http.Header) string { + for _, k := range []string{"X-Request-Id", "X-Amzn-Request-Id", "X-Amzn-Trace-Id"} { + if v := h.Get(k); v != "" { + return v + } + } + return "" +} diff --git a/internal/client/executor_test.go b/internal/client/executor_test.go index 966d80e8..27905e64 100644 --- a/internal/client/executor_test.go +++ b/internal/client/executor_test.go @@ -997,3 +997,61 @@ func TestParseErrorResponse_SurfacesParamAndDocURL(t *testing.T) { t.Errorf("DocURL = %q, want the doc URL", err.DocURL) } } + +func TestRequestIDFromHeaders(t *testing.T) { + tests := []struct { + name string + h http.Header + want string + }{ + {"app X-Request-Id preferred", http.Header{"X-Request-Id": {"app-1"}, "X-Amzn-Request-Id": {"amzn-1"}}, "app-1"}, + {"falls back to x-amzn-request-id", http.Header{"X-Amzn-Request-Id": {"amzn-1"}}, "amzn-1"}, + {"falls back to x-amzn-trace-id", http.Header{"X-Amzn-Trace-Id": {"trace-1"}}, "trace-1"}, + {"none present", http.Header{}, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := requestIDFromHeaders(tt.h); got != tt.want { + t.Errorf("requestIDFromHeaders = %q, want %q", got, tt.want) + } + }) + } +} + +func TestExtractJSONPath_ParseError(t *testing.T) { + _, err := extractJSONPath([]byte("{not valid json"), "data.id") + var cliErr *clierrors.CLIError + if !errors.As(err, &cliErr) || cliErr.Code != "response_parse_error" { + t.Fatalf("err = %v, want a response_parse_error CLIError", err) + } +} + +// errReadCloser fails on Read, simulating a connection dropped mid-response. +type errReadCloser struct{} + +func (errReadCloser) Read([]byte) (int, error) { return 0, errors.New("connection reset mid-body") } +func (errReadCloser) Close() error { return nil } + +type errBodyTransport struct{} + +func (errBodyTransport) RoundTrip(*http.Request) (*http.Response, error) { + return &http.Response{StatusCode: 200, Body: errReadCloser{}, Header: make(http.Header)}, nil +} + +func TestExecute_ResponseBodyReadError_NetworkError(t *testing.T) { + c := New("key", WithBaseURL("http://example.invalid"), WithHTTPClient(&http.Client{Transport: errBodyTransport{}})) + _, err := c.Execute(&command.Spec{Endpoint: "/v3/videos", Method: "GET"}, &command.Invocation{PathParams: map[string]string{}}) + var cliErr *clierrors.CLIError + if !errors.As(err, &cliErr) || cliErr.Code != "network_error" { + t.Fatalf("err = %v, want a network_error CLIError", err) + } +} + +func TestRequestIDFromHeaders_CaseInsensitive(t *testing.T) { + // A lowercase ALB header (as parsed/canonicalized by net/http) is still found. + h := http.Header{} + h.Set("x-amzn-request-id", "amzn-lower") + if got := requestIDFromHeaders(h); got != "amzn-lower" { + t.Errorf("requestIDFromHeaders = %q, want amzn-lower", got) + } +} diff --git a/internal/errors/errors.go b/internal/errors/errors.go index f76168b0..2152fedc 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -23,6 +23,10 @@ type CLIError struct { DocURL string // API: link to this error code's documentation RequestID string // from API X-Request-Id header (if applicable) ExitCode int // process exit code (0/1/2/3/4) + + // Telemetry-only (NOT serialized in ToErrorEnvelope — internal origin signal): + Source string // "api" (from the API envelope) or "cli" (CLI-generated). Empty defaults to "cli". + HTTPStatus int // upstream HTTP status when the error came from an API response (0 otherwise) } // Error implements the error interface. @@ -184,14 +188,23 @@ func FromAPIError(statusCode int, apiErr *APIError, requestID string) *CLIError docURL = *apiErr.DocURL } + // Origin for telemetry: a preserved (specific) API code came from the API + // envelope; a status-derived code was generated by the CLI (non-envelope / edge). + source := "cli" + if !isNonSpecific(apiErr.Code) { + source = "api" + } + return &CLIError{ - Code: code, - Message: message, - Hint: hint, - Param: param, - DocURL: docURL, - RequestID: requestID, - ExitCode: exitCode, + Code: code, + Message: message, + Hint: hint, + Param: param, + DocURL: docURL, + RequestID: requestID, + ExitCode: exitCode, + Source: source, + HTTPStatus: statusCode, } } diff --git a/internal/errors/errors_test.go b/internal/errors/errors_test.go index ca03fb4d..17479baa 100644 --- a/internal/errors/errors_test.go +++ b/internal/errors/errors_test.go @@ -362,3 +362,24 @@ func TestToErrorEnvelope_OmitsDocURLAndParamWhenEmpty(t *testing.T) { t.Error("doc_url should be omitted when empty") } } + +func TestFromAPIError_SourceAndHTTPStatus(t *testing.T) { + // A preserved (specific) API code originates from the API envelope. + got := FromAPIError(402, &APIError{Code: "insufficient_credit", Message: "x"}, "") + if got.Source != "api" || got.HTTPStatus != 402 { + t.Errorf("Source/HTTPStatus = %q/%d, want api/402", got.Source, got.HTTPStatus) + } + // A status-derived (non-specific) code is CLI-generated. + derived := FromAPIError(500, &APIError{Message: "x"}, "") + if derived.Source != "cli" || derived.HTTPStatus != 500 { + t.Errorf("Source/HTTPStatus = %q/%d, want cli/500", derived.Source, derived.HTTPStatus) + } + // Source/HTTPStatus are telemetry-only — never serialized in the envelope. + inner := got.ToErrorEnvelope()["error"].(map[string]any) + if _, ok := inner["source"]; ok { + t.Error("source must not appear in the error envelope") + } + if _, ok := inner["http_status"]; ok { + t.Error("http_status must not appear in the error envelope") + } +} From 8f105aea5d39c271b7a58c8ebb981f10218b96a9 Mon Sep 17 00:00:00 2001 From: Somansh Reddy Satish Date: Thu, 9 Jul 2026 07:35:15 +0000 Subject: [PATCH 2/5] cli: add reserved cli_ prefix + code registry with enforcement test Namespace the CLI-originated response-parse code as cli_response_parse_error, add a central registry of all CLI-minted codes, and a test that scans source and fails on any unregistered or bare-but-not-allowlisted CLI code, forcing the cli_ prefix on future codes so they can never collide with an API code. Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 2 +- internal/analytics/analytics_test.go | 2 +- internal/client/executor.go | 10 +- internal/client/executor_test.go | 4 +- internal/errors/codes.go | 50 ++++++++++ internal/errors/codes_test.go | 141 +++++++++++++++++++++++++++ 6 files changed, 202 insertions(+), 7 deletions(-) create mode 100644 internal/errors/codes.go create mode 100644 internal/errors/codes_test.go diff --git a/README.md b/README.md index 9a2601dc..9d635e3b 100644 --- a/README.md +++ b/README.md @@ -164,7 +164,7 @@ Every command supports `--help`. | Aspect | Behavior | |--------|----------| | **stdout** | Always JSON. Even `video download` — binary writes to disk; stdout emits `{"asset", "message", "path"}` so you can chain on `.path`. | -| **stderr** | Structured envelope on error: `{"error": {"code", "message", "hint", "param", "doc_url", "request_id"}}`. `code`/`message` are always present; `hint`/`param`/`doc_url`/`request_id` appear when applicable (`param`/`doc_url` are surfaced from the API for validation and documented errors). Stable `code` values for programmatic branching. | +| **stderr** | Structured envelope on error: `{"error": {"code", "message", "hint", "param", "doc_url", "request_id"}}`. `code`/`message` are always present; `hint`/`param`/`doc_url`/`request_id` appear when applicable (`param`/`doc_url` are surfaced from the API for validation and documented errors). Stable `code` values for programmatic branching. A code prefixed `cli_` is originated by the CLI itself (client/transport/local conditions, e.g. `cli_download_url_expired`); a bare code carries API semantics. The `cli_` prefix is reserved for the CLI, so the two never collide. | | **Exit codes** | `0` ok · `1` API or network · `2` usage · `3` auth / not permitted · `4` timeout under `--wait` (stdout contains partial resource for resume) | | **Request bodies** | Flags for simple inputs; `-d` for nested JSON (inline, file path, or `-` for stdin). Flags override matching fields. | | **Async jobs** | `--wait` blocks with exponential backoff; `--timeout` sets max (default 20m). 429s and 5xx retry automatically. | diff --git a/internal/analytics/analytics_test.go b/internal/analytics/analytics_test.go index dc41b63d..36c8bd2a 100644 --- a/internal/analytics/analytics_test.go +++ b/internal/analytics/analytics_test.go @@ -242,7 +242,7 @@ func TestCommandRunComplete_SourceAndHTTPStatus(t *testing.T) { t.Run("cli source omits http_status when 0", func(t *testing.T) { stub := &stubCaptureClient{} client := newWithCapture("v1.2.3", stub) - client.CommandRunComplete("heygen video create", 1, time.Second, "file_io_error", "cli", 0) + client.CommandRunComplete("heygen video create", 1, time.Second, "cli_file_io_error", "cli", 0) msg := stub.messages[0].(posthog.Capture) if got := msg.Properties["source"]; got != "cli" { t.Fatalf("source = %v, want cli", got) diff --git a/internal/client/executor.go b/internal/client/executor.go index d00a1831..1791c2b6 100644 --- a/internal/client/executor.go +++ b/internal/client/executor.go @@ -82,7 +82,7 @@ func (c *Client) ExecuteAndPoll( resourceID, err := extractJSONPath(createResp, spec.PollConfig.IDField) if err != nil { return nil, &clierrors.CLIError{ - Code: "response_parse_error", + Code: "cli_response_parse_error", Message: fmt.Sprintf( "failed to extract resource ID from %q: %v", spec.PollConfig.IDField, err), Hint: "The create response did not have the expected shape. This command may require manual polling for batch responses.", @@ -135,7 +135,7 @@ func (c *Client) ExecuteAndPoll( status, err := extractJSONPath(statusResp, spec.PollConfig.StatusField) if err != nil { return nil, &clierrors.CLIError{ - Code: "response_parse_error", + Code: "cli_response_parse_error", Message: fmt.Sprintf( "failed to extract status from %q: %v", spec.PollConfig.StatusField, err), Hint: "The status response did not have the expected shape. Retry; if it persists, report it.", @@ -207,6 +207,10 @@ func (c *Client) executeWithContext(ctx context.Context, spec *command.Spec, inv ExitCode: clierrors.ExitAuth, } } + // network_error is a grandfathered bare code (not cli_-prefixed): it is + // shared with the download-command transport path (cmd/heygen/video_download.go) + // and predates the cli_ convention. See the grandfathered set in + // internal/errors/codes.go; keep both emission sites bare and in sync. return nil, &clierrors.CLIError{ Code: "network_error", Message: fmt.Sprintf("request failed: %v", err), @@ -407,7 +411,7 @@ func extractJSONPath(raw json.RawMessage, path string) (string, error) { var current any if err := json.Unmarshal(raw, ¤t); err != nil { return "", &clierrors.CLIError{ - Code: "response_parse_error", + Code: "cli_response_parse_error", Message: fmt.Sprintf("failed to parse JSON response: %v", err), Hint: "The API response could not be parsed. Retry; if it persists, report it (possible CLI/API mismatch).", ExitCode: clierrors.ExitGeneral, diff --git a/internal/client/executor_test.go b/internal/client/executor_test.go index 27905e64..08e5d34f 100644 --- a/internal/client/executor_test.go +++ b/internal/client/executor_test.go @@ -1021,8 +1021,8 @@ func TestRequestIDFromHeaders(t *testing.T) { func TestExtractJSONPath_ParseError(t *testing.T) { _, err := extractJSONPath([]byte("{not valid json"), "data.id") var cliErr *clierrors.CLIError - if !errors.As(err, &cliErr) || cliErr.Code != "response_parse_error" { - t.Fatalf("err = %v, want a response_parse_error CLIError", err) + if !errors.As(err, &cliErr) || cliErr.Code != "cli_response_parse_error" { + t.Fatalf("err = %v, want a cli_response_parse_error CLIError", err) } } diff --git a/internal/errors/codes.go b/internal/errors/codes.go new file mode 100644 index 00000000..0d82c391 --- /dev/null +++ b/internal/errors/codes.go @@ -0,0 +1,50 @@ +package errors + +// CLI error-code namespace governance. +// +// A code is "CLI-originated" when the CLI synthesizes it itself, as opposed to +// relaying a code from the API response envelope. Every CLI-originated code must +// be registered in exactly one of the three sets below. New CLI-originated codes +// MUST carry the reserved cli_ prefix (cliPrefixedCodes); the two bare sets are +// CLOSED allowlists (grandfathered legacy + API-semantic) that must not grow. +// +// The cli_ prefix is reserved so a CLI code can never collide with a future API +// code, provided the API never mints a cli_-prefixed code (enforced API-side). +// codes_test.go scans the source and fails on any CLI-minted code that is either +// unregistered or bare-but-not-allowlisted, which forces the prefix on new codes. + +// CLICodePrefix is reserved for CLI-originated error codes. The API must never +// emit a code beginning with this prefix. +const CLICodePrefix = "cli_" + +// cliPrefixedCodes are CLI-originated codes carrying the reserved prefix. +var cliPrefixedCodes = []string{ + "cli_response_encode_error", + "cli_response_parse_error", + "cli_download_url_expired", + "cli_download_failed", + "cli_download_interrupted", + "cli_file_io_error", +} + +// grandfatheredBareCodes are CLI-originated codes that shipped bare in a stable +// release before the cli_ convention. FROZEN: do not add. Their contract is the +// exit code (buckets) or they are CLI-only names the API has no reason to mint. +var grandfatheredBareCodes = []string{ + "error", "usage_error", "auth_error", "timeout", "canceled", + "network_error", "confirmation_required", "file_exists", + "batch_not_supported", "wrong_install_method", + "video_failed", "video_not_ready", +} + +// bareAPISemanticCodes are bare codes that carry API semantics: CLI mirrors of +// API codes (same name, same meaning, so a same-name overlap is correct, not a +// clash) and CLI fallback labels for API faults. Not reverse-collision risk; +// unclassified_* is an accepted risk (the API will not mint an "unclassified" +// code). asset_not_available is dual-use: an API code the CLI also mints. +var bareAPISemanticCodes = []string{ + "not_found", "insufficient_credit", "forbidden", "unauthorized", + "conflict", "rate_limit_exceeded", "validation_error", "payload_too_large", + "asset_not_available", + "unclassified_server_error", "unclassified_client_error", +} diff --git a/internal/errors/codes_test.go b/internal/errors/codes_test.go new file mode 100644 index 00000000..05b980e1 --- /dev/null +++ b/internal/errors/codes_test.go @@ -0,0 +1,141 @@ +package errors + +import ( + "os" + "path/filepath" + "regexp" + "runtime" + "strings" + "testing" +) + +// codeLiteral matches a CLI-minted error-code string literal: either a `Code:` +// struct field or a `code =` assignment (the two forms the CLI uses to +// synthesize a code). API codes relayed from a response come from a variable +// (apiErr.Code), not a literal, so they do not match. +var codeLiteral = regexp.MustCompile(`(?:\bCode:\s*|\bcode\s*=\s*)"([a-z][a-z0-9_]*)"`) + +func registrySet(t *testing.T) map[string]string { + t.Helper() + m := map[string]string{} + add := func(codes []string, bucket string) { + for _, c := range codes { + if prev, dup := m[c]; dup { + t.Fatalf("code %q registered in both %q and %q", c, prev, bucket) + } + m[c] = bucket + } + } + add(cliPrefixedCodes, "cliPrefixed") + add(grandfatheredBareCodes, "grandfatheredBare") + add(bareAPISemanticCodes, "bareAPISemantic") + return m +} + +// TestCLICodePartition enforces the namespace invariants on the registry. +func TestCLICodePartition(t *testing.T) { + for _, c := range cliPrefixedCodes { + if !strings.HasPrefix(c, CLICodePrefix) { + t.Errorf("cliPrefixedCodes: %q must start with %q", c, CLICodePrefix) + } + } + for _, c := range append(append([]string{}, grandfatheredBareCodes...), bareAPISemanticCodes...) { + if strings.HasPrefix(c, CLICodePrefix) { + t.Errorf("bare code %q must not start with %q (only cliPrefixedCodes may)", c, CLICodePrefix) + } + } +} + +// TestBareAllowlistsAreFrozen pins the exact contents of the two bare sets. +// The reserved-prefix guarantee relies on these being CLOSED: without this, +// a new bare CLI code could bypass the cli_ rule by simply being appended to +// grandfatheredBareCodes. Changing either set now requires editing this +// expected list too, which surfaces the change in review. +func TestBareAllowlistsAreFrozen(t *testing.T) { + wantGrandfathered := []string{ + "auth_error", "batch_not_supported", "canceled", "confirmation_required", + "error", "file_exists", "network_error", "timeout", "usage_error", + "video_failed", "video_not_ready", "wrong_install_method", + } + wantAPISemantic := []string{ + "asset_not_available", "conflict", "forbidden", "insufficient_credit", + "not_found", "payload_too_large", "rate_limit_exceeded", "unauthorized", + "unclassified_client_error", "unclassified_server_error", "validation_error", + } + assertSetEqual(t, "grandfatheredBareCodes", grandfatheredBareCodes, wantGrandfathered) + assertSetEqual(t, "bareAPISemanticCodes", bareAPISemanticCodes, wantAPISemantic) +} + +func assertSetEqual(t *testing.T, name string, got, want []string) { + t.Helper() + gm := map[string]bool{} + for _, c := range got { + gm[c] = true + } + wm := map[string]bool{} + for _, c := range want { + wm[c] = true + } + for c := range gm { + if !wm[c] { + t.Errorf("%s: unexpected code %q — if this is a new bare CLI code, it likely must be cli_-prefixed instead; only add to the frozen set with explicit justification", name, c) + } + } + for c := range wm { + if !gm[c] { + t.Errorf("%s: missing expected code %q", name, c) + } + } +} + +// TestNoUnregisteredCLICodes scans production source for CLI-minted code +// literals and fails on any that is not registered. A new CLI code therefore +// must be added to the registry, and (unless grandfathered/API-semantic) must +// carry the cli_ prefix, which is what keeps future codes collision-proof. +func TestNoUnregisteredCLICodes(t *testing.T) { + reg := registrySet(t) + root := repoRoot(t) + + // Codes the CLI never originates: relayed from tests as API-response bodies, + // or test-only fixtures. These are not CLI-minted, so exclude them. + ignore := map[string]bool{} + + for _, dir := range []string{"cmd", "internal"} { + err := filepath.WalkDir(filepath.Join(root, dir), func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + src, err := os.ReadFile(path) + if err != nil { + return err + } + for _, m := range codeLiteral.FindAllStringSubmatch(string(src), -1) { + code := m[1] + if ignore[code] { + continue + } + if _, ok := reg[code]; !ok { + rel, _ := filepath.Rel(root, path) + t.Errorf("unregistered CLI-minted code %q in %s: prefix it with %q and add it to cliPrefixedCodes in codes.go (or, if it mirrors an API code / is grandfathered, add it to the matching bare set)", code, rel, CLICodePrefix) + } + } + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", dir, err) + } + } +} + +func repoRoot(t *testing.T) string { + t.Helper() + _, file, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("runtime.Caller failed") + } + // this file lives at /internal/errors/codes_test.go + return filepath.Clean(filepath.Join(filepath.Dir(file), "..", "..")) +} From a9cf06d5f57a771c83dae5ffdbc1e68f3ea41d89 Mon Sep 17 00:00:00 2001 From: Somansh Reddy Satish Date: Thu, 9 Jul 2026 23:18:20 +0000 Subject: [PATCH 3/5] cli: drop x-amzn request-id fallback from the error envelope The x-amzn-request-id/trace-id fallback populated request_id, but api_service does not log those ids, so there is no working server-side correlation path for a caller to use. Revert to reading only X-Request-Id (which the app doesn't set today, so request_id stays omitted) rather than surfacing an id nobody can trace. The request_id envelope field is kept so it populates for free once the backend sets/logs a real request id. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/client/executor.go | 14 +------------- internal/client/executor_test.go | 29 ----------------------------- 2 files changed, 1 insertion(+), 42 deletions(-) diff --git a/internal/client/executor.go b/internal/client/executor.go index 1791c2b6..ad365076 100644 --- a/internal/client/executor.go +++ b/internal/client/executor.go @@ -232,7 +232,7 @@ func (c *Client) executeWithContext(ctx context.Context, spec *command.Spec, inv } if resp.StatusCode >= 400 { - return nil, parseErrorResponse(resp.StatusCode, respBody, requestIDFromHeaders(resp.Header)) + return nil, parseErrorResponse(resp.StatusCode, respBody, resp.Header.Get("X-Request-Id")) } return json.RawMessage(respBody), nil @@ -536,15 +536,3 @@ func parseErrorResponse(statusCode int, body []byte, requestID string) *clierror return clierrors.FromAPIError(statusCode, &clierrors.APIError{}, requestID) } - -// requestIDFromHeaders returns the best available request/trace id for support -// correlation. The v3 app does not set X-Request-Id; the AWS ALB sets x-amzn-*, so -// prefer the app header when present, then fall back to the ALB's. -func requestIDFromHeaders(h http.Header) string { - for _, k := range []string{"X-Request-Id", "X-Amzn-Request-Id", "X-Amzn-Trace-Id"} { - if v := h.Get(k); v != "" { - return v - } - } - return "" -} diff --git a/internal/client/executor_test.go b/internal/client/executor_test.go index 08e5d34f..9761ddd6 100644 --- a/internal/client/executor_test.go +++ b/internal/client/executor_test.go @@ -998,26 +998,6 @@ func TestParseErrorResponse_SurfacesParamAndDocURL(t *testing.T) { } } -func TestRequestIDFromHeaders(t *testing.T) { - tests := []struct { - name string - h http.Header - want string - }{ - {"app X-Request-Id preferred", http.Header{"X-Request-Id": {"app-1"}, "X-Amzn-Request-Id": {"amzn-1"}}, "app-1"}, - {"falls back to x-amzn-request-id", http.Header{"X-Amzn-Request-Id": {"amzn-1"}}, "amzn-1"}, - {"falls back to x-amzn-trace-id", http.Header{"X-Amzn-Trace-Id": {"trace-1"}}, "trace-1"}, - {"none present", http.Header{}, ""}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := requestIDFromHeaders(tt.h); got != tt.want { - t.Errorf("requestIDFromHeaders = %q, want %q", got, tt.want) - } - }) - } -} - func TestExtractJSONPath_ParseError(t *testing.T) { _, err := extractJSONPath([]byte("{not valid json"), "data.id") var cliErr *clierrors.CLIError @@ -1046,12 +1026,3 @@ func TestExecute_ResponseBodyReadError_NetworkError(t *testing.T) { t.Fatalf("err = %v, want a network_error CLIError", err) } } - -func TestRequestIDFromHeaders_CaseInsensitive(t *testing.T) { - // A lowercase ALB header (as parsed/canonicalized by net/http) is still found. - h := http.Header{} - h.Set("x-amzn-request-id", "amzn-lower") - if got := requestIDFromHeaders(h); got != "amzn-lower" { - t.Errorf("requestIDFromHeaders = %q, want amzn-lower", got) - } -} From 6c4f744fa8fdd50ffce205b62eab673c31cbb2d3 Mon Sep 17 00:00:00 2001 From: Somansh Reddy Satish Date: Fri, 10 Jul 2026 09:10:42 +0000 Subject: [PATCH 4/5] cli: catch code:= in the registry scan; correct the envelope-doc bare-code note Codex nits on the cli_ namespacing: the source-scan regex missed the 'code :=' short-declaration form (a bare CLI code declared that way would evade the enforcement test), and the README said every bare code carries API semantics, omitting the grandfathered-legacy-CLI exception. Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 2 +- internal/errors/codes_test.go | 36 ++++++++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 9d635e3b..28f82a9f 100644 --- a/README.md +++ b/README.md @@ -164,7 +164,7 @@ Every command supports `--help`. | Aspect | Behavior | |--------|----------| | **stdout** | Always JSON. Even `video download` — binary writes to disk; stdout emits `{"asset", "message", "path"}` so you can chain on `.path`. | -| **stderr** | Structured envelope on error: `{"error": {"code", "message", "hint", "param", "doc_url", "request_id"}}`. `code`/`message` are always present; `hint`/`param`/`doc_url`/`request_id` appear when applicable (`param`/`doc_url` are surfaced from the API for validation and documented errors). Stable `code` values for programmatic branching. A code prefixed `cli_` is originated by the CLI itself (client/transport/local conditions, e.g. `cli_download_url_expired`); a bare code carries API semantics. The `cli_` prefix is reserved for the CLI, so the two never collide. | +| **stderr** | Structured envelope on error: `{"error": {"code", "message", "hint", "param", "doc_url", "request_id"}}`. `code`/`message` are always present; `hint`/`param`/`doc_url`/`request_id` appear when applicable (`param`/`doc_url` are surfaced from the API for validation and documented errors). Stable `code` values for programmatic branching. A code prefixed `cli_` is originated by the CLI itself (client/transport/local conditions, e.g. `cli_download_url_expired`). A bare code is either an API code (or a CLI mirror of one) or one of a small frozen set of legacy CLI codes that predate the prefix. The `cli_` prefix is reserved for the CLI, so a new CLI code can never collide with an API code. | | **Exit codes** | `0` ok · `1` API or network · `2` usage · `3` auth / not permitted · `4` timeout under `--wait` (stdout contains partial resource for resume) | | **Request bodies** | Flags for simple inputs; `-d` for nested JSON (inline, file path, or `-` for stdin). Flags override matching fields. | | **Async jobs** | `--wait` blocks with exponential backoff; `--timeout` sets max (default 20m). 429s and 5xx retry automatically. | diff --git a/internal/errors/codes_test.go b/internal/errors/codes_test.go index 05b980e1..dd2e3fe4 100644 --- a/internal/errors/codes_test.go +++ b/internal/errors/codes_test.go @@ -9,11 +9,12 @@ import ( "testing" ) -// codeLiteral matches a CLI-minted error-code string literal: either a `Code:` -// struct field or a `code =` assignment (the two forms the CLI uses to -// synthesize a code). API codes relayed from a response come from a variable -// (apiErr.Code), not a literal, so they do not match. -var codeLiteral = regexp.MustCompile(`(?:\bCode:\s*|\bcode\s*=\s*)"([a-z][a-z0-9_]*)"`) +// codeLiteral matches a CLI-minted error-code string literal: a `Code:` struct +// field, or a `code =` / `code :=` assignment (the forms the CLI uses to +// synthesize a code). The `:?=` covers both `=` and `:=` without matching the +// `==` comparison in isNonSpecific. API codes relayed from a response come from +// a variable (apiErr.Code), not a literal, so they do not match. +var codeLiteral = regexp.MustCompile(`(?:\bCode:\s*|\bcode\s*:?=\s*)"([a-z][a-z0-9_]*)"`) func registrySet(t *testing.T) map[string]string { t.Helper() @@ -139,3 +140,28 @@ func repoRoot(t *testing.T) string { // this file lives at /internal/errors/codes_test.go return filepath.Clean(filepath.Join(filepath.Dir(file), "..", "..")) } + +// TestCodeLiteral_MatchesDeclarationForms locks the source-scan regex to the +// three ways the CLI declares a code literal (Code: / code = / code :=) and +// confirms it does not match the `code == "..."` comparison in isNonSpecific. +func TestCodeLiteral_MatchesDeclarationForms(t *testing.T) { + src := ` + Code: "cli_alpha", + code = "cli_beta" + code := "cli_gamma" + if code == "error" { + } + ` + got := map[string]bool{} + for _, m := range codeLiteral.FindAllStringSubmatch(src, -1) { + got[m[1]] = true + } + for _, want := range []string{"cli_alpha", "cli_beta", "cli_gamma"} { + if !got[want] { + t.Errorf("codeLiteral missed declaration form for %q", want) + } + } + if got["error"] { + t.Error(`codeLiteral matched the code == "error" comparison; it must not`) + } +} From 4a41a5bf611398b1e5ea27ab1b33bb308d2c921a Mon Sep 17 00:00:00 2001 From: Somansh Reddy Satish Date: Fri, 10 Jul 2026 09:39:52 +0000 Subject: [PATCH 5/5] =?UTF-8?q?cli:=20address=20#208=20review=20=E2=80=94?= =?UTF-8?q?=20http=5Fstatus=20on=20known-status=20CLI=20sites,=20invert=20?= =?UTF-8?q?cli=5F=20scan,=20tidy=20classifyError?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Set HTTPStatus on the CLI-synthesized errors whose status is known: the mid-body-read network_error and the download url-expired/failed cases, so the telemetry (source, http_status) axis can tell a 403 from a 404 and a 500 from a 502 (previously http_status=0 there). Completes the origin-axis design. - Add an inverted registry scan: any cli_-prefixed string literal in production source must be a registered code (or an explicit non-code literal like the cli_version analytics key), closing the identifier-coupling gap where a code assigned via a non-'code' variable name would evade the Code:/code= scan. - Move the CLI-origin default into classifyError so main.go's wrapped-error branch no longer special-cases source. - Fix an em dash in the cli_download_failed hint. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/heygen/main.go | 10 +++++-- cmd/heygen/video_download.go | 18 ++++++------ internal/client/executor.go | 12 +++++--- internal/errors/codes_test.go | 52 +++++++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 15 deletions(-) diff --git a/cmd/heygen/main.go b/cmd/heygen/main.go index b8ced414..aea55f29 100644 --- a/cmd/heygen/main.go +++ b/cmd/heygen/main.go @@ -47,7 +47,7 @@ func main() { formatter.Error(wrapped) exitCode = wrapped.ExitCode errorCode = wrapped.Code - source = "cli" + source = wrapped.Source } } @@ -63,15 +63,19 @@ func main() { // everything else gets exit 1. func classifyError(err error) *clierrors.CLIError { msg := err.Error() + var wrapped *clierrors.CLIError if strings.HasPrefix(msg, "unknown command") || strings.HasPrefix(msg, "unknown flag") || strings.HasPrefix(msg, "unknown shorthand flag") || strings.Contains(msg, "accepts ") || strings.HasPrefix(msg, "required flag") || strings.HasPrefix(msg, "invalid argument") { - return clierrors.NewUsage(msg) + wrapped = clierrors.NewUsage(msg) + } else { + wrapped = clierrors.New(msg) } - return clierrors.New(msg) + wrapped.Source = "cli" // Cobra-wrapped errors are always CLI-origin. + return wrapped } func analyticsEnabled() bool { diff --git a/cmd/heygen/video_download.go b/cmd/heygen/video_download.go index aa6fe30d..7f8f8829 100644 --- a/cmd/heygen/video_download.go +++ b/cmd/heygen/video_download.go @@ -218,17 +218,19 @@ func downloadFile(ctx context.Context, videoURL, dest string) error { // re-fetch); any other status is the asset host (our storage/CDN) failing. if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusNotFound { return &clierrors.CLIError{ - Code: "cli_download_url_expired", - Message: fmt.Sprintf("download link expired or unavailable (HTTP %d)", resp.StatusCode), - Hint: "This download link has expired. Re-fetch a fresh URL: heygen video get ", - ExitCode: clierrors.ExitGeneral, + Code: "cli_download_url_expired", + Message: fmt.Sprintf("download link expired or unavailable (HTTP %d)", resp.StatusCode), + Hint: "This download link has expired. Re-fetch a fresh URL: heygen video get ", + HTTPStatus: resp.StatusCode, + ExitCode: clierrors.ExitGeneral, } } return &clierrors.CLIError{ - Code: "cli_download_failed", - Message: fmt.Sprintf("download failed with HTTP %d", resp.StatusCode), - Hint: "The asset host returned an error fetching the file. This is usually transient — retry shortly.", - ExitCode: clierrors.ExitGeneral, + Code: "cli_download_failed", + Message: fmt.Sprintf("download failed with HTTP %d", resp.StatusCode), + Hint: "The asset host returned an error fetching the file. This is usually transient. Retry shortly.", + HTTPStatus: resp.StatusCode, + ExitCode: clierrors.ExitGeneral, } } diff --git a/internal/client/executor.go b/internal/client/executor.go index ad365076..fe957320 100644 --- a/internal/client/executor.go +++ b/internal/client/executor.go @@ -223,11 +223,15 @@ func (c *Client) executeWithContext(ctx context.Context, spec *command.Spec, inv respBody, err := io.ReadAll(resp.Body) if err != nil { // Mid-response read failure is a dropped connection, not a parse issue. + // The response arrived (status known) before the body dropped, so record + // it: dashboards can then tell this from the pre-response transport failure + // above (where resp is nil and the status is genuinely 0). return nil, &clierrors.CLIError{ - Code: "network_error", - Message: fmt.Sprintf("failed to read response body: %v", err), - Hint: "This is usually a temporary network issue. Check your connection and retry.", - ExitCode: clierrors.ExitGeneral, + Code: "network_error", + Message: fmt.Sprintf("failed to read response body: %v", err), + Hint: "This is usually a temporary network issue. Check your connection and retry.", + HTTPStatus: resp.StatusCode, + ExitCode: clierrors.ExitGeneral, } } diff --git a/internal/errors/codes_test.go b/internal/errors/codes_test.go index dd2e3fe4..5d4c3b1f 100644 --- a/internal/errors/codes_test.go +++ b/internal/errors/codes_test.go @@ -165,3 +165,55 @@ func TestCodeLiteral_MatchesDeclarationForms(t *testing.T) { t.Error(`codeLiteral matched the code == "error" comparison; it must not`) } } + +// cliLiteral matches a whole double-quoted cli_-prefixed token (at least one +// char after the prefix, so the "cli_" prefix constant itself doesn't match). A +// literal that merely contains "cli_" mid-string (e.g. a log message) does not +// match, since the closing quote must immediately follow the token. +var cliLiteral = regexp.MustCompile(`"(cli_[a-z0-9_]+)"`) + +// cliNonCodeLiterals are cli_-prefixed string literals that are deliberately NOT +// error codes (e.g. telemetry property keys). A new cli_ literal that surfaces +// must be triaged: if it's a code, register it in cliPrefixedCodes; if not, add +// it here with a reason. +var cliNonCodeLiterals = map[string]bool{ + "cli_version": true, // PostHog analytics property key, not an error code +} + +// TestAllCliLiteralsRegistered inverts the scan: any cli_-prefixed string +// literal in production source, regardless of how it is assigned (Code:, +// code :=, errCode =, a slice/map/switch initializer, ...), must be a +// registered cliPrefixedCode or an explicit non-code literal. This closes the +// identifier-coupling gap in the Code:/code= scan for the cli_ side of the +// reservation guarantee. +func TestAllCliLiteralsRegistered(t *testing.T) { + reg := map[string]bool{} + for _, c := range cliPrefixedCodes { + reg[c] = true + } + root := repoRoot(t) + for _, dir := range []string{"cmd", "internal"} { + err := filepath.WalkDir(filepath.Join(root, dir), func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + src, err := os.ReadFile(path) + if err != nil { + return err + } + for _, m := range cliLiteral.FindAllStringSubmatch(string(src), -1) { + if !reg[m[1]] && !cliNonCodeLiterals[m[1]] { + rel, _ := filepath.Rel(root, path) + t.Errorf("cli_-prefixed literal %q in %s is not registered in cliPrefixedCodes (codes.go); if it is a code, register it, else add it to cliNonCodeLiterals", m[1], rel) + } + } + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", dir, err) + } + } +}