Skip to content

feat(flows): guided creation flows, P1–P9 - #5612

Merged
georgi merged 21 commits into
mainfrom
claude/tasks-creation-flow-subagents-kodwc2
Sep 6, 2026
Merged

feat(flows): guided creation flows, P1–P9#5612
georgi merged 21 commits into
mainfrom
claude/tasks-creation-flow-subagents-kodwc2

Conversation

@georgi

@georgi georgi commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

What changed

All nine phases of docs/creation-flows/tasks.md — the guided creation flows for all five document kinds.

Storyboard (P1–P5). Contracts first: a persisted setupStage and genre, scenes on the screenplay, scene_id and camera equipment on a shot, and a per-version RenderInputs record so a still or clip reads stale against the inputs its shot would use now. Then one shared prompt module both the editor and the headless render tools compose through, the store's eight ordering operations behind a contiguous shot.index, the SetupFlow shell and its steps, the board (scene headers, drag across scenes, staleness, batch reattachment), the full-screen Edit Shot dialog, and the FDX/CSV/DOCX/PDF imports with a custom-style path.

Video, Script, Image, Workflow (P6–P9). Each follows the storyboard's shape: a persisted setup on its document, format/use-case/category cards, a step-2 plan that spends nothing (D4), a look-or-setup step, and a generate that lands in the editor — with headless mirrors and eval cases.

Every schema stays passthrough and every new field defaults, so documents written before this change load unchanged (PRD § 7.7.7).

Notable calls, each argued in its own commit message:

  • / step versions in the Edit dialog, per PRD § 7.5. An earlier pass had them stepping shots, which swapped the shot out from under an open draft.
  • Genre stills and style thumbnails live under assets/nodetool-base/, which bundle-backend.mjs copies wholesale — not PACKAGE_RUNTIME_ASSETS, which is for files beside a package's compiled dist/. tasks.md said the latter; it is corrected here, since registering a path with no file behind it fails backend:smoke.
  • storyboards.stylePresets is classified gap, not elsewhere: reading the presets is reachable through list_entities, but seedStylePresets runs from that mutation and nowhere else, so a headless run on a fresh account finds none.

Verification

  • npm run test:affected
  • npm run typecheck — web and electron clean
  • npm run lint — exit 0; lint:anti-slop:enforced at zero
  • npm run dev:nodetool -- harness gate --base origin/main — 12/12 selfchecks, no capability-mapping violations
  • npm run capabilities:check — current at 275 capabilities
  • Full web suite — 1,354 suites, 14,942 tests

Three defects CI or the harness caught that local runs did not, all fixed here:

Line ids collided within a millisecond. The prefix was w${Date.now().toString(36)}, so two writes in the same millisecond minted the same ids and a rewrite's new line inherited the takes — generated audio — of the line it replaced. A monotonic counter joins the timestamp, as this repo's other id helpers already do. Both call sites had it. The regression test freezes the clock; against the old prefix it fails with Received array: ["wmjuohs00_line_1", "wmjuohs00_line_2"].

Incomplete multi-character sanitization (CodeQL) in the subtitle parser: one strip pass spliced a nested tag's remains into a new one, so <scr<b>ipt>alert(1)</scr</b>ipt> came out as ipt>alert(1)ipt>. The tag class now excludes < and the replace repeats to a fixed point. A bare < with no > is still left alone — this parser's contract is the spoken words verbatim, and "if x < y" is words, not markup.

edit_timeline advertised four ops no test reached through that surface. The harness said so. The first fix would have added the flow's suite to the attribution pool — but that suite drives the eval bridge's ui_timeline_* tools, a different entry point, so it would have made the check pass without making the coverage real. The actual fix invokes edit_timeline with set_setup, plan_beats and generate_from_beats.

Agent capabilities

Six new capabilities, all in the table with named suites: write_script, refine_image_brief, set_workflow_setup, plan_workflow, update_workflow_plan_step, build_workflow_from_plan. edit_storyboard, edit_script, edit_sketch and edit_timeline re-declare their contracts and carry updated coverage. capabilities:check passes.

The capability-suites selfcheck's command and its filter list were two copies of one string, so timeline-video-flow.test.ts was written, passing, and never run by the gate — they are one constant now, as the script↔storyboard entry already is.

New checks

  • Each new check was inverted once and observed failing; the failing output is in the commit that adds it
  • The R6 app-build case grades the run's output, not only validation — a plan that validates and produces nothing scores 0.67 and fails, pinned by its own test so the check cannot stop examining anything

Not done

  • The 14 genre stills and 12 style-preset thumbnails need artwork. Data, paths, seeding and a directory-walking test are in place; cards render a typographic fallback. Adding a JPG plus its id to SHIPPED_GENRE_STILLS turns each one on. No path is registered without a file behind it.
  • The Edit Shot dialog's scene-header save writes up to three undo checkpoints where § 7.5 asks for one. The twelve table fields — what criterion 14 measures — are one entry; the header row needs a batched store action.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4

P1 contracts from docs/creation-flows/tasks.md: `Scene`, `Screenplay.genre`
and `scenes`, `Shot.scene_id`, `camera.equipment`, the `RenderInputs` render
record, and `setupStage`/`genre` on the storyboard document. A sync sha-256
lands with them because the staleness comparison hashes the composed prompt
and has to run in the browser bundle, where `crypto.subtle` is async.

Checkpoint: the contracts compile and the protocol suite passes; the tests
covering the new shapes land with the rest of P1.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
The enforced anti-slop rule `no-conditional-empty-object-spread` rejects it.
A plain assignment keeps the same behaviour: a screenplay that carried no
scenes stays without the key rather than gaining an empty one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
The rest of phase P1 from docs/creation-flows/tasks.md.

`packages/protocol/src/shot-prompt.ts` is now the one place a shot becomes a
prompt: the § 7.7.5 matrix as three pure functions, with the web hook and the
headless render tools calling into it instead of each keeping a private copy.
Camera angle, lens, movement and equipment and the scene's lighting reach a
prompt for the first time — D8 says those fields ship with their prompt effect
or not at all. A test in each caller asserts the composed prompt equals the
module's, so the two cannot drift again.

The Director now asks for scenes: `genre` goes in, sluglines and per-scene
lighting come back, and every shot names one of them. JSON Schema cannot
express that cross-reference between sibling arrays, so `parseScreenplay`
enforces it — a shot naming an unreturned scene inherits its predecessor's,
which is the repair that keeps a scene's shots contiguous.

The store gets the eight ordering operations, each one undo entry ending in a
full reindex, plus `setStylePreset`. `reorderShots` now refuses an order that
splits a scene. Scene grouping is derived, never stored: `sceneOrder` and
`displayNumber` in `web/src/lib/storyboard/sceneOrder.ts` compute it from
`shot.index`. The existing directional `moveShot` becomes `nudgeShot` so the
name can carry the scene-aware signature the PRD specifies.

A timeline regression test pins the point of all this: eight shots grouped
three ways assemble to the same cut, field for field, because assembly reads
`shot.index` and nothing else (D5). Its inverse guard reverses the indexes and
asserts the output changes, so it cannot pass against a builder that ignores
its input.

`POST /api/documents/extract-text` lands early from P5: PDF through pdfium,
DOCX through mammoth, 25 MB cap enforced as the route's body limit so an
oversize file is refused while it is still arriving. FDX never reaches it —
that is XML the browser parses itself.

`SetupFlow` and the four shared setup components are the P2 shell, not yet
wired into any surface.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
`isVersionStale` answers whether a still or clip was rendered from inputs the
shot no longer has, so the board can mark what is out of date without spending
anything to find out (D12: a style change never renders).

Nothing is persisted as a flag. What is persisted is the `RenderInputs` record
taken when the job was enqueued, so a render that finishes after a style change
carries its enqueue-time inputs and reads stale on arrival. A version with no
record — an upload, a flip, an image-editor edit — is never stale, because it
was never a render to begin with.

The prompt is hashed rather than stored: the record rides on every version, and
it is only ever compared, never read back as text. A keyframe-mode clip also
records the still it animated, so re-picking a take makes the clip stale
without touching the still.

`BoardRenderContext` is structural rather than the web store's board type —
protocol cannot import from web, and the headless capabilities hold the same
values in a different shape.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
…rface

`harness gate` now runs the prompt-composition, scene-ordering, staleness and
Director-scene suites for a diff under `web/src/lib/storyboard/`,
`web/src/components/setup/`, or the three protocol modules they live in.

The command and its selfcheck become one constant. They were two copies of the
same string, and a selfcheck that runs less than the command it stands for
reports green on code it never executed.

Confirmed with `--dry-run`: `packages/protocol/src/shot-prompt.ts` and
`web/src/lib/storyboard/sceneOrder.ts` each select the storyboard surface,
while a sibling `web/src/lib/` path outside it selects no storyboard harness
and zero selfchecks — so the mapping matches these paths rather than everything.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
Twelve new tools and four extended ones, from PRD § 7.10, each one store
action and one undo entry: setup, direct, move, duplicate, remove, the four
scene operations, style, the three version operations, and `staleOnly` on the
two render tools. The headless `edit_storyboard` ops and the new
`direct_storyboard` capability mirror them op for op, and the two suites assert
the same document effects — a divergence between the surfaces is what they
exist to catch.

`ui_storyboard_direct` routes through `useDirectScreenplay`, the hook the
button already uses, rather than adding a second Director caller.
`ui_storyboard_set_style` resolves an entity id in the bridge, which already
holds the resolved library; the store never learns about entity kinds.

The headless render path now stamps the render record it reads. It did not
before, which left `stale_only` permanently empty on an agent-rendered board —
every existing case hand-wrote a record, so nothing caught it. The new
round-trip test renders, asserts the version carries a record, and drives
`stale_only` through it in both directions.

`setupStage` and `genre` reach the web store and round-trip through the server
sync, so P2 can resume a board by stage without further store work.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
The case builds a budget with a 10ms deadline and then asserts the cost
ceiling is the reason reported. On a loaded runner the whole 10ms can be spent
reaching the first `reserve`, so the deadline wins the race and the case fails
on the reason it exists to pin — `expected 'deadline' to be 'cost'`.

Faking the clock removes the race rather than widening the margin: the deadline
is measured against `Date.now()` from the moment the budget is built, and a
frozen clock cannot run out underneath the assertions.

Not this branch's code — the diff touches nothing in `packages/runtime` — but
it is what failed the Quality Gate here, it is flaky by construction, and the
fix is local and preserves every assertion.

Reproduced first: a 15ms stall between the two lines produces CI's exact
message locally. With the clock faked the same stall cannot expire the
deadline at all.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
The genre-still and style-thumbnail tasks said to register the files in
`PACKAGE_RUNTIME_ASSETS`. That registry is for files shipped beside a
package's compiled `dist/`, and registering a path with no file behind it
fails `backend:smoke` — so the instruction as written would have produced
either a broken bundle check or a fake asset.

Example-board stills are the precedent and none of them is registered:
`scripts/bundle-backend.mjs` copies `assets/nodetool-base/` wholesale, so the
genre art needs no per-file entry at all. Point both tasks at that tree and
say plainly not to register what has not been shipped.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
The guided storyboard flow end to end: the five entry cards on the New
Project surface and three on Studio home, resume from the document's
`setupStage` in both storyboard hosts, and the four step bodies — Idea,
Genre, Direct, Review, Look.

Contracts the phase turns on:
- Nothing renders before the last step. The review is editable text, and
  `useLookStep` writes stage `done` before it enqueues the first still, so a
  closed tab resumes on the board rather than back in setup (PRD § 7.3, D3).
- A refused Director call leaves the stage at `genre`. The hook never
  rejects, because the board's own Direct button fires it from a click
  handler, so the flow config throws on its `false` return instead.
- Twelve style presets seed as read-only system entities. The refusal is one
  static on the model and guards all four write paths; the `update_entity`
  retarget branch bypassed `saveEntityAsset` and would have moved a preset's
  marker onto another asset.

Not done, deliberately: the fourteen genre stills and twelve style
thumbnails need artwork. Both grids render a typographic tile instead, and
`genres.test.ts` walks the asset directory in both directions so a
registered id with no file — or a file with no id — fails rather than
drifting. Nothing is registered that has not shipped.

`ASPECT_OPTIONS` moves out of `StoryboardBoard.tsx` so the board and the look
step read one list.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
Studio extracted its script during the prompt start, from the Director's
first draft. The words a creator edits in the review step never reached it.
The call moves to the review step's own handler, so the script comes from
the screenplay they actually reviewed (PRD D9, criterion 6).

The flow config gains `onReviewed`; a host with no linked script passes
nothing and the step just advances.

`useStudioPromptStart` goes with it. The setup flow replaced its `start`,
leaving no production caller, and its `projectNameFromPrompt` was a second
copy of the one in `projectStarters.ts` that the New Project surface
actually uses — only its own test imported it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
…hment

Scene headers from `sceneOrder` with `Scene N | Shot N` captions, `+` insert
points, drag across headers through `moveShot`, a hover toolbar (download,
duplicate, delete), entity chips and the dialogue icon, the footer, the genre
chip and `Change Style`, the stale banner and `Retry N failed`, measured
remaining time, and the next-steps strip.

Three things the split across agents left disconnected, each silent:

- Cards were never handed a `BoardRenderContext`, so no still ever showed a
  stale marker. Nothing failed when the prop was removed, so the board's stub
  now surfaces it and a test pins it.
- `useStoryboardGenerationSubscriptions` was called with no board id in both
  hosts, so batch reattachment never fired in the product at all.
- The context was derived in three places and two disagreed on which style
  entity wins — first vs. last. `setStylePreset` keeps only one, so they
  matched in practice, but a board carrying two would have compared records
  against a style they were never stamped with and read stale forever. One
  derivation now, in `lib/storyboard/boardRenderContext.ts`, with the
  two-style case tested.

`dropShotOrder` goes: drag moves through `moveShot` now, and only its own test
still called it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
P4: a full-screen `ShotEditDialog` replacing the docked inspector — the twelve
§ 7.7.2 columns over a draft, scene slugline and lighting in the header, the
pan/zoom/flip viewer with its version pager, the takes gallery, the script
panel, and linked-board rules for dialogue and ERT. `ShotInspector` shrinks
from 993 lines to the selection footer.

P5: `parseFdx`, `verifyImportedText` and `parseShotlistCsv` as pure modules,
the upload and CSV cards, and `Add your own style`.

Two corrections to my own brief, which the PRD settles:

- `←`/`→` step versions (§ 7.5), not shots. The brief said shots, so that is
  what got built — stepping the shot out from under an open draft. The keys now
  live in the viewer, which owns the pager index and the still/clip toggle;
  the shell keeping a copy would be a second source of truth. Both halves are
  pinned: the viewer steps, and the shell provably does not.
- The harness entry ran the `web/src/lib/storyboard` suites but not
  `web/src/components/setup`, which the storyboard surface maps to it — so an
  FDX import that stopped reading back verbatim would not have failed the gate.

`generate_text` accepts content blocks so reference images can reach a model.
The schema is a union that keeps `string`, and an existing string payload still
parses — checked, not assumed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
The PR had been conflicted since the P3 push. GitHub builds PR-event
workflows against `refs/pull/<n>/merge`, which cannot be computed while the
merge is dirty, so the Quality Gate silently stopped running: three pushes
landed with only CodeQL reporting, and the green checks I had been reading
belonged to an older head.

Two textual conflicts, both resolved toward main:

- `creative.ts`: main deleted `isShot` as unused in a codebase audit. The
  `isScene` I had added beside it has no caller either, so it goes too rather
  than reintroducing the dead weight that audit just removed.
- `capabilities/storyboards.ts`: main removed the `*_SCHEMA` re-export block.
  Every schema is defined and used in `storyboards.specs.ts`, and nothing
  imports one through the capability module, so the block was pass-through.

And one conflict git did not mark: main replaced `isPlainObject` with
`isRecord` in `api-schemas/storyboards.ts` while `normalizeStoryboardScene`
kept calling the old name. The merge was textually clean and the build was
not — the kind of break only compiling the result finds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
…ndary

The style-preset procedure landed unclassified, and
`tests/sandbox-api-coverage.test.ts` refuses that by design: an omission next
to a deliberate exclusion reads the same from outside, which is how the
boundary rots.

Classified `gap`, not `elsewhere`. Reading the presets back is already
reachable — they are `style` entities and list_entities returns them — but
`seedStylePresets` is called from this mutation and nowhere else, so on an
account whose editor has never opened the look step a headless run finds no
presets to apply. That is work, and the entry says so.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
All four remaining creation flows, each following the storyboard pattern: a
persisted `setup` on its document, format/use-case/category cards, a plan step
that spends nothing, a look or setup step, and a generate that lands in the
editor. Their headless mirrors and eval cases come with them.

The four flows were built in parallel and could not touch the files they all
needed, so the entry wiring is applied here: all five cards live, one
discriminated `SetupTarget` per document kind on the New Project surface, and
Video and Script on Studio home (D24 keeps Image and Workflow out).

Three things the integration turned up, none visible from inside one phase:

- `edit_timeline`'s description advertised the four flow ops, but nothing
  invoked the capability with them — the flow's own suite drives the eval
  bridge's `ui_timeline_*` tools, a different entry point. The harness said so
  ("changed its declared contract, but its coverage mapping is unchanged").
  My first fix was to add the flow suite to the attribution pool, which would
  have made the check pass without making the coverage real; the actual fix is
  a case that invokes `edit_timeline` with `set_setup`, `plan_beats` and
  `generate_from_beats` and asserts the plan creates no clip.
- Five capabilities were missing from the registry's category snapshot: P7 and
  P9 added them without running that suite.
- `timeline-video-flow.test.ts` matched none of the `capability-suites`
  selfcheck's path filters, so the gate never ran it. That command and its
  selfcheck were two copies of one string; they are one constant now, as the
  script↔storyboard entry already is.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
Comment thread web/src/lib/script/parseSrt.ts Fixed
CodeQL, incomplete multi-character sanitization. `stripMarkup` removed tags in
one pass, so removing a nested tag spliced its remains into a new one the pass
had already gone past: `<scr<b>ipt>alert(1)</scr</b>ipt>` came out as
`ipt>alert(1)ipt>`, and a differently nested cue yields a live `<script>`.

Two changes. The tag class excludes `<`, so a match is the innermost tag rather
than one run from an opening bracket to a later close. And the replace repeats
until the text stops changing; each pass deletes at least two characters, so it
ends.

A bare `<` with no `>` is still left alone. This parser's contract is the
spoken words verbatim, and a subtitle reading "if x < y" is words, not markup —
pinned by its own case.

The nesting test fails against the old one-pass strip with
`Received: "ipt>alert(1)ipt>"`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
CI's test-app leg failed where an isolated run passed: a rewrite's new line
arrived holding the takes of the line it replaced.

The cause is not the retention, which is keyed by line id and correct. It is
the id: the prefix was `w${Date.now().toString(36)}`, so two writes inside one
millisecond mint the same ids, and `applyWrittenScript` hands the new
`w…_line_2` everything the dropped `w…_line_2` had. Takes are generated audio,
so the script would then voice words nobody wrote — and the faster the machine,
the likelier it is.

A monotonic counter joins the timestamp, which is what this repo's other id
helpers (`ScriptStore`, `extractScript`) already do. Both call sites had the
bug: the browser hook and the headless `write_script`.

The regression test freezes the clock, making the fast runner's accident
deterministic. Against the old prefix it fails with
`Received array: ["wmjuohs00_line_1", "wmjuohs00_line_2"]` — the rewrite
reusing both ids.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
@georgi georgi changed the title feat(storyboard): P1 storyboard contracts for the guided creation flows feat(flows): guided creation flows, P1–P9 Sep 6, 2026
… settings bag, price a text block with no text

Three findings from the correctness review of this branch, each with a test
written red first.

BLOCKER — useTimelineDirectGenJob settled a direct-gen reply against whichever
sequence was open when the reply landed, not the one the request was sent for.
A creator who switched sequences while a render was out left the original
sequence's pending entry in the list forever; reattachment then restored it, put
the clip back to `generating` and subscribed to a request that had already
answered — a clip stuck rendering over a paid render that was thrown away.
`sequenceId` is now captured at send time and passed into `subscribeDirectGen`.
The settle also moved above the `clips.find` lookup: the pending entry belongs to
the request, not to whether its clip is still on screen, and the early return was
the second half of the same leak.

writeWorkflowSetup dropped every sibling setting when the stored `setup` failed
to parse. A `setup` a newer client or an agent wrote — a stage outside this
build's enum — sent the whole bag through the `{}` branch, so `hide_ui` and the
rest were lost on the next write. Unparseable `setup` now keeps the caller's own
keys and replaces only `setup`.

messageChars read `.length` off a text block's `text` without checking it was a
string. The wire schema validates a block as `{ type: string }` with passthrough,
so `{ type: "text" }` is a valid frame; it threw on the spend-admission path
before anything could refuse the call.

Also fixes a TS2554 in NewProjectSurface.test.tsx: the `startImageFlow` mock had
no parameter type, so the call through it did not typecheck under web's pinned
@types/jest.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
The spec review found the PRD had been relaxed on a misreading. R5 does not
forbid model samples — it says they may be fetched on first use instead of
shipped, and E2's clip tiles already do exactly that. E4 dropped the sample and
the PRD was rewritten to match; `tasks.md` P8 still asked for it. The code is
what was wrong.

`modelSamples.ts` moves up a level and takes the sample kind: a `<video>`
element and `loadedmetadata` for a clip, an `<img>` and `load` for a still, and
the extension that goes with each. Both look steps go through it. A model with
no published sample is unchanged — the tile keeps its typographic fallback.

The test settles a stubbed element per kind. Against the video-only probe it
fails on both image cases: the URL comes back `.mp4` and the probe never asks
for a still.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4
…-flow-subagents-kodwc2

# Conflicts:
#	packages/runtime/tests/run-budget.test.ts

georgi commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Review round

Three parallel review agents (correctness, standards, spec) ran over the whole branch. Four findings fixed in 343f1f92 and 5616938d, each with a test proven red against the old code first.

Direct-gen replies settled against the wrong sequence. subscribeDirectGen re-read timeline.getState().sequenceId when the rpc_response arrived, minutes after the send. Switch sequences while a clip is rendering and the original sequence's pending entry is never removed; reopen it inside the 30-minute TTL and reattachment restores it, sets the clip back to generating and subscribes to a request id that already replied — a clip stuck rendering over a paid render that was thrown away. The id is now captured at send time and passed in. The settle also moved above the clips.find lookup: the pending entry belongs to the request, not to whether its clip is still on screen, and the early return was the second half of the same leak. The first diagnosis stopped at the live read; writing the test showed both halves were needed.

writeWorkflowSetup dropped the settings bag on an unparseable setup. A stage outside this build's enum — one a newer client or an agent wrote — sent the whole bag through the {} branch, so hide_ui and every sibling setting were lost on the next write. Unparseable setup now keeps the caller's keys and replaces only setup.

messageChars threw on a schema-valid frame. The wire schema validates a block as { type: string } with passthrough, so { "type": "text" } with no text is legal; reading .length off it threw on the spend-admission path, before anything could refuse the call.

PRD § 10.3 had been relaxed on a misreading of R5. R5 does not forbid model samples — it says they may be fetched on first use instead of shipped, which is what E2's clip tiles already do. E4 dropped the sample and the PRD was rewritten to match, contradicting tasks.md P8. The code was what was wrong. modelSamples.ts moves up a level and takes the sample kind: <video> + loadedmetadata + .mp4 for a clip, <img> + load + .jpg for a still. Both look steps go through it, and the PRD text is restored. A model with no published sample keeps its typographic fallback.

Reported and not acted on, since they are refactors across files this PR only partly touches: five-way duplication of the brief-entry block across the IdeaStep.tsx files, the five start*Flow callbacks in NewProjectSurface, and StudioHome's third copy of the same starters.

origin/main merged in. One conflict, in run-budget.test.ts: both sides had fixed the same fake-timer flake with identical bodies and different comments; main's comment kept.

Verified on the merge result: build:packages 62/62, tsc -p web/tsconfig.json clean, packages/runtime 80/80, packages/protocol 9/9, packages/websocket direct-text-spend 7/7, and 397 web tests across the 48 suites this diff and the merge touch.

The two documented gaps in the PR description — genre and style artwork, and the scene-header save writing up to three undo checkpoints — still stand as written.


Generated by Claude Code

@georgi georgi left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Found three actionable bugs in the video creation flow at df0fd0f, detailed inline. F1 prevents recovery of paid renders after a browser reload; F2 ignores the selected output aspect ratio; F3 never submits the music bed.

Validation: executed the PR's generateFromBeats function in isolation with a minimal store and stubbed model/job boundaries. A portrait sequence with a landscape format produced aspectRatio="16:9", and music:true created a music clip without a provider/model while only the video clip was submitted. Traced F1 through the current-head WebSocket subscription, RPC response, and socket delivery paths. The existing reattachment test injects a response after subscribing and therefore does not exercise a closed socket or a completed request.

This was a focused correctness review of generation and recovery paths, not an exhaustive review of all 349 files. I did not run the full application test suite.

Comment on lines +195 to +196
timeline.getState().patchClip(job.clipId, { status: "generating" });
subscribeDirectGen(timeline, job.clipId, job.requestId, sequenceId);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

F1 [P1] Recover the server result instead of only registering a local listener

Start a video render, close/reload the browser tab, then reopen the sequence. This code sets the clip to generating and calls subscribeDirectGen, but GlobalWebSocketManager.subscribe only adds a handler to a local Map; it sends no recovery request. generate_media runs through CommandRouter.runRpc, whose rpc_response has neither job_id nor thread_id, so WebSocketClientSession.sendMessage does not put it in a replay session. It goes to the original socket and is dropped once that socket is disconnected. The reopened sequence therefore never receives the result, even if generation finishes after reopening. Persist a server-side request/job result and retrieve or resume it here before marking the clip generating. Add a test where the original socket closes and generation completes before the new client subscribes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Your trace is right, and I confirmed each hop: runRpc sends { type: "rpc_response", request_id, command, result } with no job_id and no thread_id, so WebSocketClientSession.sendMessage falls past both replay branches to sendToSocket, which returns early on a disconnected socket. Re-subscribing to that id can never produce the reply.

I have not built the server-side resume, and I want to be explicit that this is a deliberate stand-down rather than an oversight.

Why not here. The same limitation is E1's, in this same PR — StoryboardGenerationStore rides the identical generate_media RPC and documents it at PENDING_JOB_TTL_MS: "A reply that landed while the tab was shut is gone, so its entry can never resolve." PRD § 8.4 asks E2 to reattach "through the same job reconciliation as E1", and it does. What neither meets is § 7.4's premise that "batch jobs are server jobs" reconciled by id. Fixing that means giving generate_media a persisted result and a resume path, then moving both flows onto it — a backend feature with its own surface, not something to land inside a 349-file PR as a review fix. I have added it to the PR's Not-done list rather than quietly narrowing the spec to match the code.

What I did fix, because it is the part that actually costs the user something: the clip no longer spins forever. reattachSequenceJobs passes the remainder of the entry's own 30-minute window to subscribeDirectGen, and a subscription that reaches it settles the pending entry and fails the clip. A reply that has not arrived by the end of its window never will, so the clip lands on Retry — the affordance § 8.4 already gives a failed clip — instead of generating over a dead socket. Before this the entry was pruned by TTL while the clip it had already patched stayed generating with nothing behind it.

Two tests, in reattachSequenceJobs.test.ts on a fake clock: one reattaches an entry started 25 minutes ago, asserts generating, advances past the window and asserts failed with the entry gone; the other lets the reply arrive, advances an hour, and asserts the clip kept its asset and was not failed. The first fails without the deadline.

I also corrected the header comment in directGenPending.ts — it said the subscription "dies with the tab", which overstated what reattachment recovers. The boundary is the socket, and it now says so, along with what closing that gap would take.


Generated by Claude Code

prompt: beat.prompt,
provider,
model,
aspectRatio: format?.aspectRatio,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

F2 [P2] Generate clips with the aspect ratio selected in the Look step

The Look step's aspect picker saves the sequence width/height, and its cost estimate uses those dimensions. This function instead stamps every generated clip with the original format's aspectRatio. For example, choose a 16:9 format, switch to 9:16 in Look, then Generate: the timeline is portrait but every paid video request still asks for 16:9. The isolated function reproduction confirms this. Derive the generation aspect from the current sequence dimensions, or pass the selected aspect explicitly, so preview, estimate and provider request agree.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed and fixed. The generated clips now read aspectOf(store.width, store.height) — the same derivation the estimate uses.

aspectOf moved out of LookStep.tsx into components/storyboard/aspectOptions.ts, beside the ASPECT_OPTIONS it scans, so the picker, the estimate and the request are one function rather than three readings. A hook importing a helper out of a component was the shape that let them drift.

Test: a "ad-15" (16:9) plan on a sequence switched to 1080×1920 asserts every video clip stamps 9:16. Against aspectRatio: format?.aspectRatio it fails with 16:9. A second case pins the unchanged path, so the fix cannot be "always portrait".


Generated by Claude Code

Comment on lines +224 to +225
const queued = [...videoClipIds, ...voiceoverClipIds];
if (musicClipId && options.musicModel) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

F3 [P2] Supply a music model and enqueue the enabled music bed

The actual Look-step caller passes only { voiceover, voice, music: musicOn }; it never supplies musicProvider/musicModel, and there is no music-model picker or fallback. With Music enabled, this function creates a bed with undefined provider/model, then this condition excludes it from the generation queue. The flow finishes with a silent draft placeholder instead of music. Reproduced with the caller's options: musicClipId is returned but startedClipIds contains only the video clip. Resolve a supported music model/provider and enqueue the bed when Music is enabled (or make the unavailable option explicit until that path exists).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed. Took the second option — made the absence explicit — because the first has nothing to resolve against.

NODETOOL_MODELS carries language, image, video and tts kinds and no music one, and the last-model store's audio bucket holds the TTS voice, so there is no curated music model for the step to pick. useMusicModelsByProvider() would find one only where the user has connected a provider that serves text_to_music, and it pulls a TanStack query into a step that is otherwise static — it broke useVideoSetupFlow.test.tsx with No QueryClient set, which is a fair signal about where that dependency belongs. Which music model NodeTool curates and bills for is a product call, not one to make inside a review fix.

So: MUSIC_AVAILABLE = false in LookStep.tsx, the toggle rendered disabled with "Not available yet — no music model is curated", and generateFromBeats no longer creates a bed it has no model for (if (musicTrack && options.musicModel)). The queue filter you pointed at is gone — a bed that exists is now always queued, which is the invariant that was missing. musicProvider/musicModel stay on the options, so the agent bridge's generate_from_beats can still supply them, and the constant flips to a real lookup when a music model is curated.

Three tests changed count from 6 to 7 as a result: they were pinning the bed's exclusion. The queue test now reads "enqueues every clip it cut, bed included".


Generated by Claude Code

… a reattached request

Three findings from review of the video creation flow, each with a test written
red first.

F2 — generated clips carried the format's original aspect ratio, not the one
the sequence is cut at. The look step's picker writes the sequence dimensions
and its cost estimate reads them back, so a creator who chose a 16:9 format and
switched to 9:16 got a portrait timeline whose every paid request still asked
for 16:9. `aspectOf` moves out of `LookStep.tsx` into `aspectOptions.ts`, beside
the options it scans, and the picker, the estimate and the request are now one
derivation. A hook reaching into a component for a helper was the shape that let
them drift.

F3 — the music bed was created and then filtered out of the queue. The look step
passes no music model and nothing resolves one: `NODETOOL_MODELS` has language,
image, video and tts kinds and no music one, and the last-model store's audio
bucket holds the TTS voice. So the flow finished carrying a silent placeholder
nothing would fill. The bed is now only cut when there is a model to render it
with, and a bed that exists is always queued. The toggle is offered disabled,
naming the gap, rather than promising music the flow cannot make.
`musicProvider`/`musicModel` stay on the options for the agent bridge.

F1 — a reattached request whose reply can never arrive left its clip at
`generating` forever. An `rpc_response` carries no `job_id` and no `thread_id`,
so the server writes it to the socket that asked and drops it if that socket has
gone: reattachment recovers a request whose socket outlived the sequence, never
one whose reply landed while the browser was shut. The TTL pruned the entry while
the clip it had already patched kept spinning. A reattached subscription now
carries the remainder of its own window and, on reaching it, settles the entry
and fails the clip, which is what puts Retry in front of the user.

Recovering those replies needs `generate_media` to become a resumable server job.
That is the storyboard flow's gap too — it rides the same RPC — so it is a
backend change for both, not a fix inside this PR. The header comment in
directGenPending.ts claimed the subscription "dies with the tab"; the boundary is
the socket, and it now says so.

Three queue-count assertions moved from 6 to 7: they had been pinning the bed's
exclusion.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TL39a2ajm5iVkRrnjbVER4

georgi commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Review round 2 — @georgi's F1–F3

All three confirmed by reading the code, and fixed in b4443342. Each fix has a test proven red against the reported behaviour first; details are on the three review threads.

  • F2 — generated clips now stamp aspectOf(width, height), the same derivation the cost estimate uses. aspectOf moved beside ASPECT_OPTIONS so the picker, the estimate and the provider request are one function.
  • F3 — took the "make the unavailable option explicit" branch: no music model is curated (NODETOOL_MODELS has no music kind), so the bed is only cut when a model is supplied, a bed that exists is always queued, and the toggle is offered disabled naming the gap.
  • F1 — the server-side resume is not built; see the thread for the full reasoning. The clip no longer spins forever: a reattached subscription carries the remainder of its own 30-minute window and, on reaching it, fails the clip so it offers Retry.

Not done — added by this round

  • A generate_media reply is lost when its socket is. It is an rpc_response with no job_id and no thread_id, so WebSocketClientSession.sendMessage writes it to the asking socket and drops it if that socket has gone. Reattachment therefore recovers a request whose socket outlived the sequence, not one whose reply landed while the browser was shut. This is the storyboard flow's gap tooStoryboardGenerationStore rides the same RPC and documents it at PENDING_JOB_TTL_MS — so PRD § 7.4's "batch jobs are server jobs, reconciled by id" is unmet by both. Closing it means giving generate_media a persisted result and a resume path and moving both flows onto it: a backend change with its own surface, not a review fix inside this PR. Recorded here rather than narrowing the spec to match the code.

Verified: web typecheck exit 0; 894 tests across the 98 suites under hooks/timeline, components/setup, components/storyboard and lib/tools; oxlint and the enforced anti-slop config exit 0 on every changed file.


Generated by Claude Code

@georgi
georgi merged commit ba61f94 into main Sep 6, 2026
25 checks passed
@georgi
georgi deleted the claude/tasks-creation-flow-subagents-kodwc2 branch September 6, 2026 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants