cli: classify video-download failures into specific codes

[somanshreddy]

Scope

Surfaces: CLI (video download) | Module: Error handling

Stacked on #206.

Summary

The video-download path collapsed every failure — expired link, CDN error, network drop, local disk error, unparseable response — into the generic error code. This splits each into a specific, actionable code so an agent or user knows what actually failed and what to do. CLI-originated codes carry the reserved cli_ prefix.

How it works

downloadFile fetches the finished asset directly from a presigned CDN URL (the v3 API is not in that path), so these are all CLI-side failures classified by where they occur:

Failure	Code	Meaning / next step
Presigned URL 403/404	cli_download_url_expired	link expired — re-fetch with heygen video get
Asset host 5xx/other	cli_download_failed	storage/CDN error — usually transient, retry
Transport failure	network_error	connectivity — retry (bare: grandfathered, shared with the executor transport path)
Mid-stream cutoff (io.Copy)	cli_download_interrupted	retry (partial file cleaned up)
Local temp/finalize/rename	cli_file_io_error	check destination path / disk space
Unparseable video-get response / unusable URL	cli_response_parse_error	retry; report if persistent
Encoding our own output	cli_response_encode_error	CLI bug

Each carries a matching hint. No exit-code changes — all were exit 1 and stay exit 1.

Design decisions

CLI-originated codes are cli_-prefixed. These are all CLI-side failures (the CLI fetches the asset from a presigned CDN URL; the API is not in the path), so their codes carry the reserved cli_ prefix to keep them out of the API's namespace and collision-proof against any current or future API code. network_error is the one deliberate exception: it is a grandfathered, released code shared with the API-executor transport path (internal/client/executor.go), kept bare so both sites agree (documented at the emission site).

cli_download_failed vs the API's download_failed. The API's download_failed is a 400 for a user-provided input URL the API failed to fetch (a client error) — the opposite of the CLI failing to fetch the finished asset from the CDN (server-side). These near-opposite conditions must not share a code; the cli_ prefix keeps them unambiguous. (An earlier asset_-qualified name dodged the same collision by hand; the prefix now handles it structurally.)

Split by origin, not lumped. cli_download_url_expired (client can fix by re-fetching) is separated from cli_download_failed (our storage failing) because the remedies differ.

Testing

httptest-backed download tests: 403/404 → cli_download_url_expired (+ re-fetch hint), 500 → cli_download_failed, malformed JSON + unusable URL → cli_response_parse_error, transport failure → network_error, mid-stream cutoff → cli_download_interrupted, missing-parent-dir → cli_file_io_error. Existing download success/not-ready tests still pass. make lint clean. (cli_response_encode_error is a json.Marshal of a fixed map — effectively unreachable, not tested.)

Follow-up (not in this PR) — tracked in PRINFRA-247

Dashboard server-list at release: add cli_download_failed + cli_response_parse_error; move the API's client-side download_failed (400) out of the server-list. Documenting the cli_ codes and wiring their doc_url is tracked in the same issue. source origin tag is in telemetry (#208).

[james-russo-rames-d-jusso]

Reviewed at c42c4de.

Traced every error-emission site in downloadFile + extractAssetURL + RunE, cross-checked the test file, and re-read errors.go for how these codes flow into the envelope. The classification is sound, exit-code preservation holds (every new path is ExitGeneral / 1), the presigned-URL 403/404 split is a real usability win, and cli_download_failed vs the API's download_failed is a well-motivated distinction. Two things I'd like to see addressed before this lands, plus a coverage note.

Concerns

• 🟠 PR body ↔ code drift. The "How it works" table in the PR description lists the codes as download_url_expired, asset_download_failed, download_interrupted, file_io_error, response_parse_error, internal_error. The actual codes in the diff are:

  PR body	Actual code
  download_url_expired	cli_download_url_expired
  asset_download_failed	cli_download_failed
  network_error	network_error (no prefix)
  download_interrupted	cli_download_interrupted
  file_io_error	cli_file_io_error
  response_parse_error	cli_response_parse_error
  internal_error	cli_response_encode_error (different name)

  The last commit (c42c4de — "namespace CLI-originated download/encode error codes with cli_ prefix") clearly happened after the body was written. Two consumer-facing consequences: (1) the "Follow-up (not in this PR)" note about the dashboard server-list references the old names, and someone updating the server-list from the PR description will register the wrong strings; (2) the design-decision paragraph ("asset_download_failed is intentionally CLI-specific, not the API's download_failed") still argues for asset_download_failed even though the ship name is cli_download_failed. The rationale still applies, just under a different name. Please refresh the PR body table + follow-up section so it matches the codes users will actually see.

• 🟠 Prefix inconsistency inside downloadFile. Six of the new codes get the cli_ prefix; network_error (video_download.go:204) does not. The reason is presumably that network_error is a pre-existing shared code (already emitted by internal/client/executor.go:205 for API transport failures) and you didn't want to fork the name. That's a reasonable call, but it isn't spelled out anywhere — a future contributor adding another CLI-side code will have no rule to follow. Consider one of:

  • Add a short comment at the top of downloadFile (or above the first cli_* emission) documenting: "CLI-originated failure codes here are prefixed cli_ to disambiguate from API codes; network_error is intentionally unprefixed because it's shared with the API-executor transport-failure path."
  • Or bite the bullet and make network_error → cli_network_error and rev the executor too (larger blast radius, probably not worth it in this PR).

  Either is fine; leaving the rule undocumented isn't.

• 🟡 Failure-path test coverage. Nice coverage on cli_download_failed (500), cli_download_url_expired (403/404), and both cli_response_parse_error paths (malformed JSON + unusable URL). Three of the seven new codes are still untested:

  • network_error (transport error): downloadClient.Do failure — testable by pointing downloadClient at a listener that refuses connect, or by cancelling context mid-request.
  • cli_download_interrupted (mid-stream io.Copy cutoff): testable with a handler that writes Content-Length: N then closes after < N bytes, or that hijacks the connection and drops.
  • cli_file_io_error (temp-create / rename / close): the tempfile-create branch is testable by pointing --output-path at a directory whose parent isn't writable (or non-existent).

  Per feedback_observability_pr_failure_path_coverage — the untested dispatch branches are exactly the ones that regress silently on refactors. At least covering network_error and cli_download_interrupted would lock the classification in. cli_response_encode_error (json.Marshal of map[string]string) is effectively unreachable and not worth testing.

Nits

• cli_response_parse_error doing double duty (video_download.go:131 and :194). The first is a JSON.Unmarshal failure on the API response body; the second is http.NewRequestWithContext failing on an unparseable URL. Both are "the CLI got something it can't use from the API," so the shared code is defensible, but they're conceptually distinct — a URL parse failure isn't really a "response parse" thing. If you split, cli_bad_download_url for the URL case would be clearer. Not blocking. (nit)

• cli_download_interrupted hint — "The transfer was cut off. Retry; the partial file was cleaned up." Small phrasing thing: since retry advice is baked into the hint, worth ending with a period-then-sentence rather than semicolons, matching the other hints' style. (nit)

Questions

• Is the follow-up TODO ("Dashboard server-list at release: add asset_download_failed + response_parse_error; move the API's client-side download_failed (400) out of the server-list") tracked anywhere — Linear, TODO comment, checklist? Not blocking for this PR (the CLI is self-contained), but the API's download_failed continuing to live in a "server-side" server-list is exactly the kind of thing that gets forgotten across a release.

What I didn't verify

• Whether Graphite will cleanly rebase #207 onto main after #206 lands (base is 07-07-cli_error_docurl_param). No conflicting files but there's always drift risk — see feedback_graphite_restack_silent_revert if you notice anything odd post-merge.
• End-to-end: didn't drive heygen video download against a real broken CDN to confirm the codes emit as expected.
• Whether the CI govulncheck failure is pre-existing (I checked briefly — it's golang.org/x/crypto@v0.52.0 reachable via selfupdate.init, unrelated to this diff).

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

R2 — reviewed at 08254f1.

The restack picked up the executor-test fixture fixes from #206, but the PR-207-specific R1 items are unaddressed. Full diff since R1 (c42c4de..08254f1) is a 4-line propagation from #206 — no changes to cmd/heygen/video_download.go, no new tests, no doc updates.

Resolution

• 🟠 #3 PR body ↔ code drift — ❌ Not addressed, and worse than R1. Body table still shows pre-rename codes for all seven rows. Two are not just missing the cli_ prefix — they were renamed:
  • Body: asset_download_failed → code: cli_download_failed (cmd/heygen/video_download.go:224). The clarifying "asset" is gone; cli_download_failed is now one prefix step from the API's own download_failed (400) — the exact collision the body's Design-decisions paragraph explicitly warns against.
  • Body: internal_error → code: cli_response_encode_error (cmd/heygen/video_download.go:108). Different name entirely.
  • The other four (download_url_expired, download_interrupted, file_io_error, response_parse_error) are missing the cli_ prefix.
  • Both prose sections — "Design decisions" and the "Follow-up (dashboard server-list)" — still say asset_download_failed. If someone reads the body and registers that string in the dashboard, they'll register a name the CLI no longer emits.
• 🟠 #4 network_error unprefixed rationale — ❌ Not addressed. Grep-verified the R1 hypothesis: name is shared across internal/client/executor.go:205 and cmd/heygen/video_download.go:204. The asymmetry is deliberate but undocumented; still needs a code comment at one of the two emission sites so the next person doesn't "fix" the inconsistency and break executor.
• 🟡 #5 Coverage gap — ❌ Not addressed. No new tests since R1. network_error, cli_download_interrupted, cli_file_io_error remain untested. httptest handles two of the three (transport failure by hanging up the connection; chunked-response abort mid-stream); ~15–20 lines total.
• #6 Clean items — remain clean. os.Remove(tmpPath) fires on all three write-path failures (video_download.go:248,257,270); every emission still clierrors.ExitGeneral; no callers of the old un-prefixed names remain in non-test code.

CI

Same pre-existing govulncheck failure as #206 — golang.org/x/crypto@v0.52.0 via selfupdate.init, not a regression.

Holding on the same #3 + #4 items from R1. The PR-body update is the load-bearing one — the "Follow-up" section explicitly names strings a downstream consumer would use to register codes.

— Review by Rames D Jusso

[somanshreddy]

R2 items addressed (branch amended + restacked; SHAs updated, CI re-running):

#3 PR body ↔ code drift — fixed. Body table now shows the actual shipped codes (cli_download_url_expired, cli_download_failed, cli_download_interrupted, cli_file_io_error, cli_response_parse_error, cli_response_encode_error, and network_error bare). The Design-decisions paragraph now argues under cli_download_failed and notes the cli_ prefix is what structurally handles the API-download_failed (400) collision the old asset_-qualifier was dodging. The Follow-up section uses the new names and points at PRINFRA-247 (which also tracks moving the API's download_failed out of the server-list).

#4 network_error unprefixed rationale — documented. Added a comment at the emission site (video_download.go) stating it's a grandfathered shared code, also emitted by internal/client/executor.go, kept bare so both sites agree, with a pointer to the registry in internal/errors/codes.go. So the next contributor has an explicit rule and won't "fix" it and break the executor.

#5 Coverage gap — closed. Added three httptest-backed tests: network_error (dead listener → transport failure), cli_download_interrupted (hijack + Content-Length overshoot then drop → mid-stream EOF), and cli_file_io_error (dest under a missing parent dir → temp-create fails). All pass. Skipped cli_response_encode_error (a json.Marshal of a fixed map, effectively unreachable), matching your note.

Nits:

• cli_download_interrupted hint — reworded to period-separated sentences ("… Retry the download. The partial file was cleaned up.") to match the other hints.
• cli_response_parse_error double-duty (JSON-parse vs unusable-URL) — left as the shared code (you noted it's defensible). Happy to split the URL case out to a distinct cli_bad_download_url if you'd prefer; it's a one-code add + registry entry.

Follow-up question — yes, the dashboard server-list work (and the download_failed-in-server-list cleanup) is tracked in PRINFRA-247, now referenced in the PR body.

govulncheck remains the pre-existing x/crypto@v0.52.0-via-selfupdate.init failure, not a regression.

[james-russo-rames-d-jusso]

R3 — reviewed at 87b9f59.

Force-push landed the R2 fixes. Verifying each prior item independently against HEAD:

R2 finding	Status	Evidence
🟠 PR body ↔ code drift (7 pre-rename codes, asset_download_failed in follow-up, Design-decisions warning under old name)	✅ Addressed	Body table now enumerates the actually-shipped codes; Design-decisions paragraph argues under cli_download_failed and pins the API-download_failed (400) collision to the cli_ prefix; Follow-up section uses new names and references PRINFRA-247.
🟠 network_error unprefix rationale undocumented	✅ Mostly addressed (see below)	4-line comment at cmd/heygen/video_download.go:203-206 explains why it stays bare and points at the sibling emission in internal/client/executor.go.
🟡 Coverage gap — network_error, cli_download_interrupted, cli_file_io_error untested	✅ Addressed	Three new httptest-backed tests in cmd/heygen/video_download_test.go: TestVideoDownload_NetworkError (dead listener → transport failure at 523-549), TestVideoDownload_Interrupted (hijack + Content-Length overshoot then drop → mid-stream EOF at 552-591), TestVideoDownload_FileIOError (dest under missing parent → temp-create fails at 594-620). All three assert the specific code in the stderr envelope. Ubuntu / macOS / Windows test jobs all green on this run.
Nit: cli_download_interrupted hint sentence style	✅ Addressed	Reworded to period-separated ("Retry the download. The partial file was cleaned up.") at video_download.go:255.

New concern from R2 fix — 🟡 network_error documentation is one-sided and points at a file that doesn't exist

The R2 fix landed at the video-download emission site, but two smaller gaps remain around it:

1. Broken cross-reference. The new comment at cmd/heygen/video_download.go:206 says "See the grandfathered set in internal/errors/codes.go before 'fixing' this." — but internal/errors/codes.go does not exist (find internal/errors -name '*.go' yields only api_error.go, errors.go, errors_test.go). The Design-decisions paragraph in the PR body makes the same claim ("documented at the emission site and in internal/errors/codes.go"). A future contributor who follows the pointer will hit a dead end. Either drop the codes.go clause from both the comment and the PR body, or actually add a codes.go (or a // Grandfathered: marker in errors.go) that enumerates the bare-code set.

2. Sibling site has no matching comment. The R2 concern was that a future contributor would drift the two sites — and while the video-download site now warns them, the executor site (internal/client/executor.go:205) is untouched:

   return nil, &clierrors.CLIError{
       Code:     "network_error",
       Message:  fmt.Sprintf("request failed: %v", err),
       ...

   A contributor doing "rename bare codes to client_ prefix" in executor.go sees no signal at the site they're actually editing. A short mirror comment there ("see cmd/heygen/video_download.go for the shared-code rationale") closes the loop.

Non-blocking — the core R2 ask (someone reading the video-download emission site understands why it's bare) is satisfied. But the "grandfathered set" mechanism the comment promises isn't real yet, and the sibling site is still exposed.

What I verified

• Somansh's summary comment claims independently verified against the diff — all three R2 items land as described (gh api repos/heygen-com/heygen-cli/pulls/207/commits + full pr diff).
• The three new tests structurally exercise the code paths they claim (dead-listener transport error, Content-Length overshoot with hijack + close, os.CreateTemp in a missing parent dir).
• cli_response_encode_error remains untested — acknowledged in the PR body as a json.Marshal of a fixed map (effectively unreachable), which matches my R2 note.
• No competing peer reviews to layer against — I'm the only reviewer on this PR.
• CI: tests / lint / secrets / goreleaser / pr-template all green. govulncheck red is pre-existing x/crypto@v0.52.0-via-selfupdate.init plus a fresh Go-stdlib TLS advisory (GO-2026-5856, crypto/tls@go1.25.11) on files this PR doesn't touch — environmental, not a regression.
• Mergeable: MERGEABLE.

What I didn't verify

• Whether cli_download_url_expired fires for a 401 or other 4xx on a presigned URL — the code only branches on 403/404 and lumps 401 into cli_download_failed. If AWS/GCP presigned URLs ever return 401 for signature-expiry, that error would misclassify as "asset host failing." Non-blocker unless the storage provider's semantics say otherwise; worth a follow-up once you see production data.

LGTM from my side pending your call on the 🟡 above. Leaving as a comment (approval routing lives elsewhere).

— Review by Rames D Jusso

[somanshreddy]

R3 🟡 fixed. The codes.go reference was a forward-reference — that file lands in #208, so citing it from #207 was dangling. Resolved both ways you suggested, split across the right branches:

• cli: classify video-download failures into specific codes #207: the network_error comment (video_download.go) and the Design-decisions paragraph no longer mention codes.go. They're now self-contained and cite only internal/client/executor.go, which is present on this PR.
• cli: reclassify parse/read failures, add source/http_status telemetry, reserve cli_ code prefix #208 (where codes.go actually exists): added the missing mirror comment at the sibling emission site (internal/client/executor.go, the transport-failure network_error), pointing at the grandfathered set in codes.go and noting both sites must stay bare and in sync.

Net: both network_error emission sites are documented, and no PR references a file it doesn't contain. Branch amended + restacked (#207 2291775, #208 cc750c0); govulncheck unchanged (pre-existing).

[jrusso1020]

Approving at 2291775f. Download-failure classification is sound: 403/404 → cli_download_url_expired (presigned URL expired, re-fetchable) vs other HTTP → cli_download_failed (asset-host failure), plus cli_file_io_error / cli_download_interrupted / cli_response_parse/encode_error. Confirmed the R3 nit is resolved — the comment no longer references a nonexistent internal/errors/codes.go; it now reads "grandfathered shared transport code, intentionally unprefixed," and network_error stays bare with a clear rationale (shared with executor.go transport path, don't prefix one site without the other). New httptest-backed tests cover the gap codes. All required checks green; govulncheck red is the same non-required pre-existing x/crypto finding. LGTM.