cli: reclassify parse/read failures, add source/http_status telemetry, reserve cli_ code prefix (#208)

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* cli: address #208 review — http_status on known-status CLI sites, invert cli_ scan, tidy classifyError

- 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) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: somanshreddy

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 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. |

cmd/heygen/main.go

@@ -26,25 +26,33 @@ func main() {
 
 	exitCode := 0
 	errorCode := ""
+	source := ""
+	httpStatus := 0
 	if err != nil {
 		var cliErr *clierrors.CLIError
 		if errors.As(err, &cliErr) {
 			enrichAuthHint(cliErr, credSourceFromCmd(executedCmd))
 			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).
 			wrapped := classifyError(err)
 			formatter.Error(wrapped)
 			exitCode = wrapped.ExitCode
 			errorCode = wrapped.Code
+			source = wrapped.Source
 		}
 	}
 
 	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)
@@ -55,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 {

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 <video_id>",
-				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 <video_id>",
+				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,
 		}
 	}
 

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,
 	})
 }
 

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, "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)
+		}
+		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")
+		}
+	})
+}

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: "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.",
+			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: "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.",
+				ExitCode: clierrors.ExitGeneral,
+			}
 		}
 
 		if slices.Contains(spec.PollConfig.TerminalOK, status) {
@@ -201,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),
@@ -212,7 +222,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.
+		// 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.",
+			HTTPStatus: resp.StatusCode,
+			ExitCode:   clierrors.ExitGeneral,
+		}
 	}
 
 	if resp.StatusCode >= 400 {
@@ -394,7 +414,12 @@ func extractJSONPath(raw json.RawMessage, path string) (string, error) {
 
 	var current any
 	if err := json.Unmarshal(raw, &current); err != nil {
-		return "", clierrors.New(fmt.Sprintf("failed to parse JSON response: %v", err))
+		return "", &clierrors.CLIError{
+			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,
+		}
 	}
 
 	for _, part := range strings.Split(path, ".") {

internal/client/executor_test.go

@@ -997,3 +997,32 @@ func TestParseErrorResponse_SurfacesParamAndDocURL(t *testing.T) {
 		t.Errorf("DocURL = %q, want the doc URL", err.DocURL)
 	}
 }
+
+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 != "cli_response_parse_error" {
+		t.Fatalf("err = %v, want a cli_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)
+	}
+}

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",
+}