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

[somanshreddy]

Scope

Surfaces: CLI (external v3 API) | Module: Error handling / telemetry

Final PR of the error-handling stack (#205, #206, #207 merged); now based on main.

Summary

Three things: (1) finishes reclassifying the opaque error bucket, the last CLI-side parse/read failures get specific codes; (2) introduces a reserved cli_ prefix for CLI-originated error codes, with a registry and an enforcement test, so a CLI code can never collide with a current or future API code; (3) adds an internal origin signal (source + http_status) to telemetry so the error dashboards are collision-safe and can separate app failures from edge/CLI ones.

How it works

Reclassification. The remaining generic error sites in the client get specific codes: a mid-response body-read failure becomes network_error (a dropped connection, not a parse problem); JSON-parse and poll field-extraction failures become cli_response_parse_error.

Reserved cli_ prefix (the collision guarantee). Error codes share one flat namespace between the v3 API and the CLI, and origin is deliberately not in the user-facing envelope, so the API is authoritative. To prevent a reverse collision (the CLI ships a unique code, the API later mints the same name), CLI-originated codes now carry the reserved cli_ prefix. A registry in internal/errors/codes.go partitions every CLI-minted code into three closed sets: cli_-prefixed (new), grandfathered-bare (codes already released bare, e.g. error/usage_error/network_error/timeout), and bare API-semantic (mirrors of API codes + the unclassified_* fallbacks). An enforcement test scans the source and fails on any CLI-minted code that is unregistered or bare-but-not-allowlisted, and pins the bare allowlists so they cannot silently grow. Because the API never emits a cli_* code, every future CLI code is collision-proof by construction.

Telemetry origin (source + http_status). COMMAND_RUN_COMPLETE gains two properties on error: source (api when the code came from an API response envelope, else cli; defaults to cli) and http_status (the upstream status when known, omitted otherwise). Both are telemetry-only, not in the user-facing envelope. Dashboards group by (source, error_code), which is a second, independent guard against code-name collision in our metrics.

request_id. Reads only X-Request-Id. The x-amzn-* ALB fallback was intentionally dropped: api_service does not log those ids, so there is no working server-side correlation path, and surfacing an untraceable id is worse than an empty one. The request_id envelope field is kept (omit-when-empty), so it populates for free once the backend sets a real, traceable id.

Design decisions

cli_ for CLI-originated codes; released codes grandfathered bare. Prefixing is the only thing that prevents the reverse collision (a reserved namespace the API never uses) rather than merely detecting it. Released CLI codes stay bare (a frozen allowlist) to avoid a pure BC break, and unclassified_* stays bare because it labels an API/server fault, not a CLI condition, and the API will not mint an "unclassified" code. Zero BC: only previously-unreleased codes are prefixed.

source/http_status live in telemetry, not the envelope. The agent acts on code/message/hint/doc_url; whether an error originated in our CLI vs our API doesn't change remediation and would leak internal architecture. Dashboards are the consumer that needs origin, so it goes there.

source defaults to cli. Only FromAPIError sets api (and only for a preserved, specific API code); every other construction path is CLI-origin.

Testing

Unit tests: the reclassified codes (network_error, cli_response_parse_error); the registry partition, the source-scan that fails on any unregistered/wrongly-bare CLI code, and the frozen-allowlist check; FromAPIError sets source/http_status (api for a preserved code, cli for status-derived) and they are absent from the envelope; the analytics event carries source/http_status and omits them on success. make lint clean.

Follow-up (not in this PR)

• Dashboards group by (source, error_code) and add the new cli_* codes to the server/client code-lists, a release-time PostHog change.
• Documenting the cli_ codes (and wiring their doc_url) is tracked in PRINFRA-247.

[james-russo-rames-d-jusso]

Reviewed at 6c4f744.

Solid finish to the error-handling stack. The reserved cli_ prefix + registry-with-source-scan is a clean way to prevent reverse collisions, the source/http_status telemetry split gives dashboards a clean origin axis, and the reclassifications (network_error for mid-body drops, cli_response_parse_error for parse/extract failures) fit the taxonomy well. Bare/grandfathered set spot-checks all clean, all six registered cli_* codes are used, and the added regex-form test locks the scan contract. LGTM from my side — leaving as a comment with a few observations inline.

Concerns

• 🟡 http_status is populated for API-origin errors via FromAPIError but not on the CLI-transport paths that already know the status (mid-body read at internal/client/executor.go:226-231, presigned-URL failures at cmd/heygen/video_download.go:220-231). Dashboards can't distinguish an expired-URL 403 from a 404, or an asset-host 500 from a 502. Trivially fixable — see inline.
• 🟡 The enforcement-test regex in internal/errors/codes_test.go couples to the literal identifier code; a future author writing errCode := "cli_foo" would evade the scan. See inline for a suggestion.

Questions

• 🟡 Is the cli_ prefix reservation 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. See inline.

Nits

• Small hardcode in cmd/heygen/main.go:50 could move into classifyError. See inline.

What I did not verify

• API-side enforcement of cli_* prefix reservation (no api_service repo access from this workspace).
• The PostHog dashboard (source, error_code) grouping change referenced in the PR body — deferred by design, tracked separately.
• Pre-existing govulncheck failure (golang.org/x/crypto/openpgp via selfupdate + crypto/tls@go1.25.11) is on main already and unrelated to this diff.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟡 The mid-body-read failure now emits network_error (good), but resp.StatusCode is already known at this point (from lines 220-222) — yet HTTPStatus isn't populated on the CLIError. Dashboards using the (source, http_status) axis will see this as source=cli, http_status=0, indistinguishable from the pre-response transport failure four lines up (line 214-219) where the status genuinely isn't known.

Same shape in cmd/heygen/video_download.go:220-231: cli_download_url_expired (403/404) and cli_download_failed (other 4xx/5xx from the asset host) both have resp.StatusCode in scope but don't set it. The PR body says the design captures "the upstream status when known" — and it IS known at all three sites.

Suggestion: add HTTPStatus: resp.StatusCode on the three sites that have a resp in scope (this file 226-231, and video_download.go 220-231 x2). Zero-cost, completes the origin-axis telemetry design.

[james-russo-rames-d-jusso]

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

[james-russo-rames-d-jusso]

🟡 The regex catches Code: (struct field) and the local variable name code, which matches the current codebase convention. But if a future author writes errCode := "cli_foo" / myCode = "cli_bar" — or embeds a code literal in a slice/map/switch initializer — the enforcement test silently misses.

Two ways to harden:

1. Invert the invariant: scan any non-_test.go file for string literals matching ^cli_[a-z0-9_]*$ and fail if not in cliPrefixedCodes. Catches every cli_* literal regardless of syntactic context. False-positive risk on comments/log strings mentioning a code by name — addressable via a small ignore-set or by requiring backtick-quoted code names in comments.

2. Keep the current regex but widen the variable name to [a-zA-Z_][a-zA-Z0-9_]*Code\s*:?= so errCode / apiCode etc. also match.

If the team is comfortable with the current "only Code: struct field or code local" convention, worth a docstring line near the regex spelling that out so future authors don't accidentally evade it. Not a blocker either way.

[james-russo-rames-d-jusso]

Nit: this hardcode duplicates the "empty-source-defaults-to-cli" logic four lines up. Could be moved into classifyError itself (set wrapped.Source = "cli" before returning) so the origin concern lives with the constructor, and the else-branch loses the special case entirely. Very minor.

[somanshreddy]

Thanks — folded the three code items, filed the fourth:

🟡 http_status on the CLI sites that know the status — fixed. Set HTTPStatus: resp.StatusCode on the mid-body-read network_error (executor.go) and both cli_download_url_expired / cli_download_failed sites (video_download.go). Dashboards on the (source, http_status) axis can now distinguish a 403 from a 404 and a 500 from a 502; the genuine pre-response transport failure still reports http_status=0. Completes the origin-axis design.

🟡 enforcement-test identifier coupling — hardened. Added an inverted scan (TestAllCliLiteralsRegistered): any "cli_..." literal in production source, regardless of assignment form (Code:, code :=, errCode =, slice/map/switch), must be a registered cliPrefixedCode, or an explicit non-code literal. That surfaced cli_version (a telemetry key), now in a small documented cliNonCodeLiterals allowlist. (Also broadened the code-form regex to catch := per the earlier round.)

Nit: main.go:50 source hardcode — moved into classifyError (sets wrapped.Source = "cli"); the wrapped-error branch now just reads wrapped.Source, no special case.

🟡 Question: is cli_ reserved API-side? — Honest answer: convention-only today, not yet enforced server-side. Filed PRINFRA-251 to add the symmetric check in EF (assert no _STANDARD_ERROR_DEFINITIONS key starts with cli_), which makes the reservation airtight end-to-end. Out of scope for this CLI-half PR, but tracked.

Pushed as 4a41a5b. (govulncheck remains the pre-existing x/crypto/crypto/tls failure on main, unrelated.)

[james-russo-rames-d-jusso]

Reviewed at 4a41a5b. R1 baseline was 6c4f744.

R2: all three code items landed cleanly. Enforcement-regex fix chose the stronger design — new TestAllCliLiteralsRegistered inverts the scan on the cli_ side, closing the identifier-coupling gap AND catching slice/map/switch initializer forms the old code-identifier scan couldn't reach. Verified all six registered cli_* codes still emit, and the sole non-code cli_ literal (cli_version at internal/analytics/analytics.go:126) is correctly allowlisted. http_status on the three transport sites populated with correct nil-safety (all inside resp != nil paths). source = "cli" folded into classifyError — sole prod caller is the Cobra-wrap path, which is always CLI-origin, so no misdirection risk. LGTM from my side — leaving as a comment with one small doc-tightening nit inline on codes.go.

Verified

• ✅ http_status populated: internal/client/executor.go:233 (mid-body read network_error), cmd/heygen/video_download.go:224 (cli_download_url_expired), cmd/heygen/video_download.go:232 (cli_download_failed). All three sites are inside successful resp != nil blocks, so resp.StatusCode is always available; the resp == nil transport-fail case at executor.go:212 correctly still reports http_status=0, and the new doc-comment on the mid-body read explains that distinction for future readers.
• ✅ TestAllCliLiteralsRegistered (internal/errors/codes_test.go:186-217) — regex "(cli_[a-z0-9_]+)" requires whole-token match (opening + closing "), so mid-string substrings can't slip through. Skips _test.go. Walks cmd/ + internal/. Confirmed the whitelist is minimal + correct: only cli_version (PostHog property key). No fragile identifier coupling anywhere.
• ✅ source = "cli" moved into classifyError (cmd/heygen/main.go:79). Only prod caller of classifyError is main.go:46, invoked in the Cobra-wrap branch (always CLI-origin) — no risk of a new caller inheriting a hardcoded default that shouldn't apply. CLIError branch retains the if source == "" { source = "cli" } fallback (main.go:39-41), so both paths agree on the default.
• 🟡 API-side cli_ prefix enforcement — acknowledged as convention-only today; PRINFRA-251 tracks the EF-side symmetric check. See inline on the codes.go docstring — small doc-vs-reality nit.

CI

• All test jobs pass on ubuntu/macos/windows. govulncheck remains the pre-existing x/crypto/openpgp (via selfupdate) + crypto/tls@go1.25.11 failure noted in R1 — unrelated to this diff.
• Mergeable state is behind (base has advanced); trivial rebase/merge. behind_by=0 from R1 → HEAD means no rebase happened between rounds, so all prior verification still holds.

What I did not verify

• API-side enforcement of cli_* prefix reservation still requires server-side work (PRINFRA-251).
• The PostHog dashboard (source, error_code) grouping change referenced in the PR body — deferred by design.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

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

[jrusso1020]

Approving at head 4a41a5bf (matches RDJ's R2 head). Verified the R1 fixes at source:

• http_status gap — HTTPStatus: resp.StatusCode now populated on the transport sites (executor.go:224, video_download.go:224/232).
• Enforcement-regex fragility — resolved with TestAllCliLiteralsRegistered + the whole-token "(cli_[a-z0-9_]+)" scan (codes_test.go:173/189), so any assignment shape is caught, not just code := ....
• source nit — folded into classifyError per RDJ's R2.
  Required checks green (test ×3 / lint / secrets / goreleaser); govulncheck red is non-required + pre-existing (x/crypto via selfupdate + crypto/tls), same as #206/#207. mergeable_state=behind is a trivial rebase. Remaining item — the codes.go docstring reading "enforced API-side" while it's convention-only (PRINFRA-251 pending EF-side enforcement) — is RDJ's non-blocking nit; fine to soften or defer. Clean finish to the stack. LGTM.