Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. |
Expand Down
18 changes: 15 additions & 3 deletions cmd/heygen/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand Down
18 changes: 10 additions & 8 deletions cmd/heygen/video_download.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down
22 changes: 16 additions & 6 deletions internal/analytics/analytics.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
}

Expand Down
40 changes: 38 additions & 2 deletions internal/analytics/analytics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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" {
Expand Down Expand Up @@ -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")
}
})
}
45 changes: 35 additions & 10 deletions internal/client/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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),
Expand All @@ -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 {
Expand Down Expand Up @@ -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, ".") {
Expand Down
29 changes: 29 additions & 0 deletions internal/client/executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
50 changes: 50 additions & 0 deletions internal/errors/codes.go
Original file line number Diff line number Diff line change
@@ -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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Small doc-vs-reality tightening: the "(enforced API-side)" clause reads as if server-side enforcement is already in place, but per the PR comment it is convention-only today (PRINFRA-251 tracks the EF-side symmetric check). A future engineer reading this docstring alone might trust an end-to-end guarantee that doesn't yet exist. Consider:

// 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 — currently by
// convention; planned symmetric enforcement in EF tracked in PRINFRA-251.

Small; not blocking. Feel free to defer to the follow-up PR that lands the EF check and doc together.

// 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_"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Question: is the cli_ prefix reservation actually enforced on the API side? The design guarantee — "a new CLI code can never collide with an API code" — is airtight only if the API also refuses to mint cli_* codes. The PR body notes "provided the API never mints a cli_-prefixed code (enforced API-side)"; is there a symmetric check server-side (e.g. a test that scans the API's error registry for the reserved prefix), or is it convention-only right now?

Not blocking (this PR is the CLI half), but worth confirming the invariant is enforced end-to-end so the reservation is more than aspirational — otherwise an API engineer could accidentally ship cli_foo and produce exactly the reverse collision this PR is designed against.


// 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",
}
Loading
Loading