feat(ndjson): emit video_url/duration_ms/retryable on terminal events (0.6.3)#9
Merged
Conversation
… (0.6.3) Reconcile the long-standing gap between events.md (aspirational) and the real NDJSON wire shape (drift). The events.md draft has documented these fields since the schema was first written, but the CLI was dropping them from backend `aim_result` and `error` SSE payloads — agent consumers had no way to retrieve the rendered video URL from the terminal event. Code - StreamEvent gains VideoURL, DurationMs, Retryable. stream.go pulls the URL from html_path (v=3 pipeline) or text (v=2 agent) and reads duration_ms from the v=3 metadata bag. Retryable is inferred from the code label via httpclient.IsRetryableCode since the backend emits no retryable flag of its own. - New StreamEvent.NDJSONFields() helper produces the canonical wire shape. `vk create` and `vk video wait` both delegate to it so the two stream-consuming entry points stay in sync (they previously diverged: wait emitted stage/node on every event regardless of type and never emitted code/retryable/video_url). - Exit codes: retryable task failures now exit 4 (previously always 5). `vk video wait` gained the same script_invalid→2 / retryable→4 parity already in `vk create`. Docs - events.md rewritten against the actual taxonomy. Removed five documented-but-never-emitted events. Added engine-difference section. Schema version stays "1"; node.*→stage.* rename and code→error_code rename are reserved for a future "2" bump. Tests - httpclient.IsRetryableCode unit table (transient vs permanent vs unknown). - stream_test: aim_result v=3 carries video_url+duration_ms; aim_result v=2 falls back to text and omits duration_ms; plain `error` SSE marks retryable=false; concurrent_work_limit marks retryable=true. - event_ndjson_test: helper omits absent optional fields and always emits retryable on task.failed. - New integration test runs the real binary with mock figlens and asserts the on-the-wire NDJSON shape end-to-end, plus the exit-4 path for concurrent_work_limit. Real-backend smoke deliberately skipped this round to avoid burning credits on an already-validated payload-shape change; the integration test exercises the same wire path end-to-end through the binary.
…le errors
Closes the second half of the NDJSON/exit-code consistency gap surfaced
by the 0.6.3 smoke. The SSE-side `task.failed` path already exits 4 on
retryable codes after the prior commit, but `/v1/tasks/init` errors
(InitTask) returning the same envelope code (`concurrent_work_limit`,
`rate_limited`, `internal_error`) still exited 1 via cobra's default
handler — same condition, different exit code is precisely the
agent-confusing inconsistency the `retryable` flag exists to prevent.
Also closes an NDJSON gap: pre-stream failures left stdout empty,
forcing consumers to special-case "no terminal event implies it failed
before the stream started". `vk create --output ndjson` now synthesizes
one `task.failed` event on stdout for InitTask-time errors, sharing the
wire shape with in-stream task.failed (via the same `code`/`message`/
`retryable` fields). The events.md note makes the synthesis explicit.
Verified end-to-end against the beta backend: a real
`concurrent_work_limit` returned by `/v1/tasks/init` now produces:
- exit code 4
- stdout: `{"type":"task.failed","code":"concurrent_work_limit",
"retryable":true,"message":"...","schema_version":"1","ts":"..."}`
- stderr: clerr-rendered Chinese error line
Integration test pins the contract with a httptest mux returning 100003
so the regression can never silently revert.
Refs PR #9, follows the same retryable-inference taxonomy from
httpclient.IsRetryableCode used by the SSE path.
`vk video wait` shares the in-stream task.failed → exit code mapping with `vk create` (script_invalid → 2, retryable → 4, otherwise 5), but that mapping was previously untested for wait specifically — only the NDJSONFields helper had coverage. A regression that hard-coded wait back to "always 5" would have slipped through the suite. Two integration tests, mirroring the existing create-side coverage: - retryable code (100003 concurrent_work_limit) → exit 4 + ndjson task.failed with retryable=true - script_invalid (100004) → exit 2 Both run against an httptest figlens server that emits the SSE envelope the real backend produces.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reconciles the long-standing gap between `events.md` (which has always documented `video_url` / `duration_ms` / `retryable` on terminal NDJSON events) and the real wire shape (which dropped them). Closes the agent-consumer hole where `vk create --output ndjson` could not surface the rendered video URL — consumers were forced into a follow-up `vk video status` call.
What changes
Wire shape additions — additive within schema_version `"1"`:
Exit codes — retryable failures now exit `4` (was always `5`). `vk video wait` gains the same `script_invalid → 2` / `retryable → 4` parity as `vk create`.
Doc reconcile — `events.md` rewritten against actual taxonomy. Removed five aspirational events that were never emitted (`task.submitted`, `task.queued`, `stage.`, `task.cancelled`). Engine-difference section added (pipeline vs agent). The breaking renames `node.` → `stage.*` and `code` → `error_code` are explicitly deferred to a future schema `"2"` bump.
Internal — new `StreamEvent.NDJSONFields()` canonicalizes the wire shape so `vk create` and `vk video wait` cannot drift again. New `httpclient.IsRetryableCode` centralizes the retryable inference.
Why this is a fix, not a feature
The CLI was dropping fields from terminal events that `events.md` has documented since the schema was first written. Calling this "new feature" would understate the bug — `vk create --output ndjson` is the agent-facing entry point and `task.succeeded` without `video_url` is unusable for that audience.
Possible-breaking surface
Shell scripts that hard-coded `[ $? = 5 ]` to detect task failure will now sometimes see exit `4` for transient failures (`rate_limited`, `internal_error`, `concurrent_work_limit`). Documented in CHANGELOG with recommended replacement.
Test plan
🤖 Generated with Claude Code