feat(ndjson): emit video_url/duration_ms/retryable on terminal events (0.6.3)

CHANGELOG.md

@@ -1,5 +1,69 @@
 # Changelog
 
+## 0.6.3 — 2026-05-15
+
+### Fixed (NDJSON terminal events were missing result data)
+
+- `task.succeeded` events emitted by `vk create --output ndjson` and
+  `vk video wait --output ndjson` now include `video_url` (and, on the
+  pipeline engine, `duration_ms`). Previously the CLI dropped these
+  fields from the backend `aim_result` SSE payload and forwarded only
+  `session_id`, so any NDJSON consumer (agent or shell script) had no
+  way to retrieve the rendered video URL from the terminal event
+  without making a separate `vk video status` follow-up call.
+- `task.failed` events now include a `retryable` boolean derived on
+  the CLI side from the error code (transient codes — `rate_limited`,
+  `internal_error`, `concurrent_work_limit` — map to `retryable=true`;
+  permanent codes like `insufficient_credits` and `script_invalid`
+  map to `retryable=false`). The backend's terminal `error` event
+  carries no retryable flag of its own, so the CLI is the source of
+  truth for this signal.
+- `vk create` exit codes now honor the `retryable` flag: transient
+  task failures exit **4** (previously always 5). `vk video wait`
+  gained the same exit-code mapping plus `script_invalid → exit 2`
+  parity with `vk create`. Shell scripts that previously hard-coded
+  `[ $? = 5 ]` to detect any task failure should switch to
+  `[ $? -ge 4 ] && [ $? -le 5 ]` or branch on the NDJSON `retryable`
+  field directly.
+- HTTP `/v1/tasks/init` errors now share the same retryable exit-code
+  mapping as in-stream `task.failed` events. Previously a backend
+  envelope code of `concurrent_work_limit` / `rate_limited` /
+  `internal_error` returned by InitTask exited **1** (cobra default),
+  while the same code surfaced mid-stream exited **4** — same
+  condition, different exit code is precisely the agent-confusing
+  inconsistency the `retryable` flag exists to prevent. After this
+  patch both paths exit 4 and emit identical `task.failed` NDJSON
+  events when `--output ndjson` is set. Verified end-to-end against
+  the beta backend: a real `concurrent_work_limit` at InitTask now
+  produces exit 4 + a structured terminal event with
+  `retryable: true`.
+- `vk create --output ndjson` now synthesizes a terminal
+  `task.failed` event on stdout for **pre-stream** failures (InitTask
+  errors). Previously a pre-stream failure left stdout empty,
+  forcing NDJSON consumers to special-case "no terminal event implies
+  it failed before the stream started". Every CLI exit ≠ 0 in NDJSON
+  mode now ships exactly one terminal `task.failed` line on stdout.
+
+### Docs
+
+- `skills/vibeknow-create/references/events.md` rewritten against
+  the actual emitted event taxonomy. Previously documented but
+  never-emitted events (`task.submitted`, `task.queued`, `stage.*`,
+  `task.cancelled`) removed. Engine-difference section added to make
+  the pipeline-vs-agent split explicit. Schema version stays `"1"`;
+  a `"2"` bump is reserved for renaming `node.*` → `stage.*` or
+  `code` → `error_code`, neither of which ships in this release.
+
+### Internal
+
+- New `figlens.StreamEvent.NDJSONFields()` helper produces the wire
+  shape both `vk create` and `vk video wait` emit. The two commands
+  previously diverged in subtle ways (wait emitted `stage` / `node`
+  on every event regardless of type and never emitted `code`,
+  `retryable`, `video_url`); they now share one canonical projection.
+- New `httpclient.IsRetryableCode(code)` centralizes the retryable
+  inference so the SSE path no longer needs an ad-hoc switch.
+
 ## 0.6.2 — 2026-05-14
 
 ### Fixed

client/figlens/event_ndjson.go

@@ -0,0 +1,49 @@
+package figlens
+
+// NDJSONFields renders the event into the canonical wire-shape map that
+// `vk create` and `vk video wait` both emit on `--output ndjson`. The two
+// commands deliberately use the same projection so a downstream agent
+// can consume either stream interchangeably.
+//
+// Optional fields are omitted (not zero-emitted) so consumers can rely
+// on presence implying a real value — useful in particular for
+// `duration_ms`, which the agent engine simply does not produce.
+func (e StreamEvent) NDJSONFields() map[string]any {
+	switch e.Type {
+	case "node.started", "node.succeeded", "node.failed":
+		return map[string]any{
+			"type":    e.Type,
+			"stage":   e.Stage,
+			"node":    e.Node,
+			"message": e.Message,
+		}
+	case "node.progress":
+		return map[string]any{
+			"type":    e.Type,
+			"status":  e.Status,
+			"message": e.Message,
+		}
+	case "task.succeeded":
+		out := map[string]any{
+			"type":       e.Type,
+			"session_id": e.SessionID,
+		}
+		if e.VideoURL != "" {
+			out["video_url"] = e.VideoURL
+		}
+		if e.DurationMs > 0 {
+			out["duration_ms"] = e.DurationMs
+		}
+		return out
+	case "task.failed":
+		return map[string]any{
+			"type":      e.Type,
+			"code":      e.Code,
+			"message":   e.Message,
+			"retryable": e.Retryable,
+		}
+	}
+	// Unknown event types are passed through with just the type so
+	// future backend additions don't crash existing consumers.
+	return map[string]any{"type": e.Type}
+}

client/figlens/event_ndjson_test.go

@@ -0,0 +1,66 @@
+package figlens_test
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/vibeknow/cli/client/figlens"
+)
+
+func TestNDJSONFields_TaskSucceededOmitsAbsentFields(t *testing.T) {
+	// Agent engine has no duration_ms — must be omitted, not zero-emitted,
+	// so a consumer doing `if "duration_ms" in event` works.
+	ev := figlens.StreamEvent{Type: "task.succeeded", SessionID: "s", VideoURL: "https://x"}
+	got := ev.NDJSONFields()
+	if _, has := got["duration_ms"]; has {
+		t.Fatalf("duration_ms must be omitted when zero, got %v", got["duration_ms"])
+	}
+	if got["video_url"] != "https://x" {
+		t.Fatalf("video_url = %v, want https://x", got["video_url"])
+	}
+}
+
+func TestNDJSONFields_TaskSucceededIncludesPresentFields(t *testing.T) {
+	ev := figlens.StreamEvent{
+		Type: "task.succeeded", SessionID: "s",
+		VideoURL: "https://x", DurationMs: 12345,
+	}
+	got := ev.NDJSONFields()
+	want := map[string]any{
+		"type":        "task.succeeded",
+		"session_id":  "s",
+		"video_url":   "https://x",
+		"duration_ms": int64(12345),
+	}
+	if !reflect.DeepEqual(got, want) {
+		t.Fatalf("got %v, want %v", got, want)
+	}
+}
+
+func TestNDJSONFields_TaskFailedAlwaysCarriesRetryable(t *testing.T) {
+	// retryable must be present on every task.failed event — consumers
+	// branch exit-code 4 vs 5 on it, so omission would silently change
+	// behavior.
+	ev := figlens.StreamEvent{Type: "task.failed", Code: "rate_limited", Message: "slow down", Retryable: true}
+	got := ev.NDJSONFields()
+	if got["retryable"] != true {
+		t.Fatalf("retryable = %v, want true", got["retryable"])
+	}
+	if got["code"] != "rate_limited" {
+		t.Fatalf("code = %v", got["code"])
+	}
+}
+
+func TestNDJSONFields_NodeProgressUsesStatusNotStage(t *testing.T) {
+	// node.progress is the agent-engine free-form event; it carries
+	// `status` (start/success/error), not stage/node which are pipeline
+	// concepts.
+	ev := figlens.StreamEvent{Type: "node.progress", Status: "start", Message: "calling KB"}
+	got := ev.NDJSONFields()
+	if _, has := got["stage"]; has {
+		t.Fatalf("node.progress must not carry stage/node, got %v", got)
+	}
+	if got["status"] != "start" {
+		t.Fatalf("status = %v, want start", got["status"])
+	}
+}

client/figlens/stream.go

@@ -33,6 +33,16 @@ type StreamEvent struct {
 	Node      string
 	Message   string
 	SessionID string
+	// Set on task.succeeded. VideoURL is the playable HTML URL (v=3
+	// `html_path`, v=2 `text`). DurationMs is the rendered video length
+	// in milliseconds; backend only emits it on v=3 (in `data.duration_ms`),
+	// so v=2 task.succeeded events carry DurationMs=0.
+	VideoURL   string
+	DurationMs int64
+	// Set on task.failed. Derived from Code via httpclient.IsRetryableCode
+	// because the backend's terminal SSE error event carries no retryable
+	// flag of its own.
+	Retryable bool
 }
 
 type ssePayload struct {
@@ -45,6 +55,18 @@ type sseData struct {
 	Log       json.RawMessage `json:"log,omitempty"`
 	SessionID string          `json:"session_id,omitempty"`
 	Message   string          `json:"message,omitempty"`
+	// aim_result terminal-event fields. The two engines disagree on
+	// where the playable URL lives — see resolveVideoURL.
+	HtmlPath string          `json:"html_path,omitempty"`  // v=3 pipeline
+	Text     string          `json:"text,omitempty"`       // v=2 agent (also free text on v=3, ignored there)
+	DataMap  json.RawMessage `json:"data,omitempty"`       // v=3 metadata bag, contains duration_ms etc.
+}
+
+// aimResultData captures the v=3 metadata bag. Other fields exist
+// (themeId, fps, coverUrl, scenes, …) but the CLI only forwards
+// duration_ms today — additions are cheap when consumers need them.
+type aimResultData struct {
+	DurationMs int64 `json:"duration_ms"`
 }
 
 type processLog struct {
@@ -62,6 +84,22 @@ func mapSSECode(code int) string {
 	return "business_error"
 }
 
+// resolveVideoURL extracts the playable HTML URL from an aim_result payload.
+// The two engines disagree on which field holds it:
+//   - v=3 pipeline: `html_path` (preferred — engine emits a structured URL).
+//   - v=2 agent:    `text` (the agent only has one free-form result field;
+//     it puts the HTML URL there directly).
+//
+// Picking HtmlPath first means v=3 wins cleanly when both happen to be set,
+// which they shouldn't be — but we defend against the cross-engine drift
+// case rather than assume the backend stays in its lane.
+func resolveVideoURL(d sseData) string {
+	if d.HtmlPath != "" {
+		return d.HtmlPath
+	}
+	return d.Text
+}
+
 func (c *Client) StreamChat(ctx context.Context, params StreamParams, onEvent func(StreamEvent)) error {
 	resp, err := c.http.DoRaw(ctx, "POST", params.Engine.StreamPath(), params)
 	if err != nil {
@@ -101,10 +139,12 @@ func (c *Client) StreamChat(ctx context.Context, params StreamParams, onEvent fu
 					msg = d.Message
 				}
 			}
+			code := mapSSECode(payload.Code)
 			onEvent(StreamEvent{
-				Type:    "task.failed",
-				Code:    mapSSECode(payload.Code),
-				Message: msg,
+				Type:      "task.failed",
+				Code:      code,
+				Message:   msg,
+				Retryable: httpclient.IsRetryableCode(code),
 			})
 			return nil
 		}
@@ -154,13 +194,23 @@ func (c *Client) StreamChat(ctx context.Context, params StreamParams, onEvent fu
 			}
 
 		case "aim_result":
-			onEvent(StreamEvent{Type: "task.succeeded", SessionID: d.SessionID})
+			ev := StreamEvent{Type: "task.succeeded", SessionID: d.SessionID}
+			ev.VideoURL = resolveVideoURL(d)
+			if len(d.DataMap) > 0 {
+				var ar aimResultData
+				if json.Unmarshal(d.DataMap, &ar) == nil {
+					ev.DurationMs = ar.DurationMs
+				}
+			}
+			onEvent(ev)
 
 		case "error", "ERROR":
 			msg := d.Message
 			if msg == "" {
 				msg = string(payload.Data)
 			}
+			// Plain `error` SSE never carries a code; Retryable defaults
+			// to false so consumers do not silently re-run unbounded.
 			onEvent(StreamEvent{Type: "task.failed", Message: msg})
 		}
 	}