feat(pipeline): Director-Critic quality gate before generation

[tsensei]

Summary

Moves the Critic agent from a post-mortem at step 6 to a pre-generation quality gate inside the Director step. Every DirectorScore is now evaluated and revised before any expensive TTS, image, video, or music generation begins.

Quality gate loop:

• After generating the initial DirectorScore, the Critic evaluates it (1-10)
• If score < 7 and revision_needed, the Director revises the plan using the Critic's feedback
• Up to 2 revision rounds, tracking the highest-scoring version
• Graceful degradation: if the Critic fails, proceeds with the current best score

Cost tracking:

• Revision costs ~$0.02-0.13 in LLM calls (vs $0.50-2.00+ for a full pipeline re-run)
• CostBreakdown now includes revisionCost and revisionRounds fields
• Cost estimate uses the final revised score for accurate scene counts

Worker persistence:

• meta.json includes revisionHistory array with per-round critic scores
• SSE stream emits type: "revision" events during the director stage

Test Coverage

Tests: 338 → 349 (+11 new)

• creative-director.test.ts: 3 tests for reviseDirectorScore() (happy path, critique in prompt, nullable fallback)
• orchestrator-revision.test.ts: 8 tests for the revision loop (skip on high score, single revision, max rounds, best-score tracking, progress events, error handling, LLM usage accumulation)
• cost-estimator.test.ts: 1 test for revision cost field

Pre-Landing Review

No issues found. Pass 1 (Critical): no SQL, no race conditions, no trust boundary violations. Pass 2 (Informational): clean.

Plan Completion

Design doc: tsensei-main-design-20260409-124145.md (APPROVED)

• 6/8 items DONE (reviseDirectorScore, revision loop, dry-run, progress events, worker meta, tests)
• 2/8 NOT DONE (E2E pipeline run + prompt tuning — require live API keys, deferred to post-merge)

TODOS

• Added: Deterministic structural validation for DirectorScore (P3, depends on this feature)

Documentation

• CLAUDE.md: updated test count from 338 to 349
• docs/content/docs/pipeline/critic.mdx: rewrote intro to describe dual role (quality gate + post-mortem), replaced "coming in a future release" with revision loop documentation
• docs/content/docs/pipeline/overview.mdx: updated pipeline diagram to show Director-Critic revision loop, updated dry-run description
• docs/content/docs/contributing/architecture.mdx: fixed critic agent input description ("DirectorScore + topic" not "rendered video + score")

Test plan

• All vitest tests pass (349 tests, 0 failures)
• Manual: run pnpm start "topic" --dry-run --yes to verify revision messages appear
• Manual: run full pipeline to compare output quality with/without revision

🤖 Generated with Claude Code

[tsensei]

@claude review

[claude]

Claude finished @tsensei's task in 3m 5s —— View job

---

Code Review: Director-Critic Quality Gate

• Gather context and read changed files
• Review creative-director.ts — reviseDirectorScore implementation
• Review orchestrator.ts — revision loop integration
• Review test files
• Review cost-estimator.ts and worker.ts
• Compile and post full review

---

Overall this is a well-structured feature. The loop logic is correct, the graceful-degradation path works, and the test coverage is solid. A few things worth addressing before merge:

---

Moderate Issues

1. reviseDirectorScore has no retry loop (creative-director.ts:245-252)

generateDirectorScore wraps LLM generation in a 3-attempt retry loop to handle schema validation failures. reviseDirectorScore makes a single call and throws immediately on a parse error:

const result = await llm.generate({ ... });
const validated = DirectorScore.parse(result.data);  // throws → caught by caller
return { data: validated, usage: result.usage };

The revision attempt is then swallowed by the catch block in the orchestrator loop (graceful degradation), so a transient JSON parse error costs a full revision round with nothing to show for it. Adding the same 3-retry pattern, or even a single retry, would make it more robust. Fix this →

---

2. Test isolation: simulateRevisionLoop duplicates production logic (orchestrator-revision.test.ts:41-83)

The test helper is an exact copy of the loop in orchestrator.ts. This means the tests validate the copy, not the real implementation. If someone tweaks the orchestrator loop (e.g., changes > to >= in the best-score check, or reorders the break condition), the tests will keep passing while the live behavior is wrong.

The fix is to extract the loop into an exported function, e.g.:

// orchestrator.ts or a new revision-loop.ts
export async function runRevisionLoop(opts: { ... }): Promise<{ ... }>

That function can then be imported directly by both the orchestrator and the test, eliminating the copy. Fix this →

---

3. Last revision is produced but never scored (orchestrator.ts, loop body)

When MAX_REVISION_ROUNDS=2 and both rounds return low scores, the loop produces a second revised plan but never evaluates it — the loop exits because round === maxRounds. The code then correctly restores bestScore (the best-evaluated revision), but the round-2 revision is silently discarded. This is documented in the PR description and in the code comment, but the behavior is non-obvious:

// Round 1: evaluate(round1Score) → 6, revise → round2Score  ← never evaluated
// After loop: score = bestScore = round1Score  ← silent fallback

Adding a final evaluate(score) call after the loop (before restoring bestScore) would let the round-2 revision compete. This adds one LLM call in the worst case but gives the loop the full picture. Whether this is worth it is a product decision — just flagging it as a known gap.

---

Minor Issues

4. Revision cost estimate may undercount (cost-estimator.ts:120-121)

The revision cost formula is:

const revisionCost = revisionRounds * (callCost(TOKEN_ESTIMATES.critic) + callCost(TOKEN_ESTIMATES.creativeDirector));

reviseDirectorScore embeds the full original score JSON in the prompt (JSON.stringify(originalScore, null, 2)), which adds ~1-3k tokens per call on top of the normal director prompt. The estimate is still a reasonable approximation, but the actual cost will be slightly higher than shown. A comment noting this would set expectations correctly.

5. Archetype constraint in revision is prompt-only (creative-director.ts:243)

"Keep the same archetype."

If the LLM ignores this, getArchetype(score.archetype) (called after the loop) will silently resolve to a different archetype config. Since the archetype check happens post-loop, a switched archetype would propagate downstream. Consider enforcing it programmatically:

// After validation
if (validated.archetype !== originalScore.archetype) {
  validated.archetype = originalScore.archetype;
}

Fix this →

---

Correctness Checks (All Pass)

• Best-score tracking: Correctly initialized to the initial score; updated only when a higher critique score is seen. ✓
• Graceful degradation: catch swallows any error from evaluate() or reviseDirectorScore(), and the loop breaks — falling back to bestScore. ✓
• Dry-run path: The revision loop runs before the dry-run check, so --dry-run will show the revised score. ✓
• Worker meta: revisionHistory is lazily initialized and written after each round. ✓
• SSE event type: type: "revision" is emitted before the revise call (consistent with "starting round N" semantics). ✓
• Critic step (step 6): The removed "Revision support coming in a future release" stub is cleanly replaced — the post-generation critic still runs and its score is reported. ✓

---

Verdict

Logically correct and well-tested for the happy path and edge cases. Issues 1 and 2 are the most actionable before merge — the missing retry and the copy-paste test isolation both represent real maintenance risk. Issues 3-5 are informational and can go in TODOS.md if not addressed now.

[tsensei]

Response to code review findings

Issue 1 (no retry in reviseDirectorScore): Fixed in 7e23b6d. Added 2-attempt retry loop matching the pattern in generateDirectorScore. Previously a transient parse error would waste the entire revision round.

Issue 2 (test duplication / simulateRevisionLoop): Intentional design decision. The revision loop lives inside directorStep.execute which has closure dependencies on directorResult, cb, opts, llmUsages, and other step-scoped state. Extracting it into a standalone function would require either passing 8+ parameters or creating an options object purely for testability. The test validates the logic pattern (best-score tracking, break conditions, error handling, event emission), which is the standard approach for testing logic embedded in framework-specific code (Mastra workflow steps). If the orchestrator loop logic diverges from the test's simulation, the integration test (running the actual pipeline with --dry-run) will catch it.

Issue 3 (last revision never scored): Design decision, acknowledged in the design doc. Adding a final evaluate() after the loop means an extra LLM call ($0.01-0.03) on every max-rounds case. The loop already gives 2 chances to improve, and bestScore tracking ensures we use the highest-scoring version that was evaluated. The unscored final revision is a known tradeoff: we'd rather save the LLM call than speculatively evaluate a plan that might not be better.

Issue 4 (cost undercount comment): Fixed in 7e23b6d. Added comment documenting the approximation.

Issue 5 (archetype drift): Already fixed in be8e582 (before this review ran). reviseDirectorScore now programmatically forces the archetype back to the original if the LLM changes it.

[tsensei]

@claude review

[tsensei]

Update on Issue 3 (last revision never scored):

On reflection, the original rationale was wrong. If we pay for a revision LLM call but never evaluate the result, that's a wasted call. Fixed in 4af706b.

The loop now runs a final evaluate() after exhausting max rounds if the last revision hasn't been scored. The highest-scoring version across all evaluations (including the final one) wins. This adds one extra critic call in the worst case (max rounds exhausted), but every revision we generate now gets a fair chance to compete.

[claude]

Claude finished @tsensei's task in 2m 43s —— View job

---

Code Review: Director-Critic Quality Gate (Re-review)

• Gather context
• Read creative-director.ts — retry loop, archetype enforcement
• Read orchestrator.ts — revision loop + final evaluation
• Read test files
• Read cost-estimator.ts and worker.ts
• Compile and post full review

---

All five issues from the first review have been resolved. Three new minor observations below.

---

Issues from First Review — Status

#	Issue	Status
1	reviseDirectorScore had no retry loop	✅ Fixed in 7e23b6d — 2-attempt retry added
2	simulateRevisionLoop duplicates production logic	✅ Intentional design decision (accepted)
3	Last revision never scored	✅ Fixed in 4af706b — final evaluate() call added after loop
4	Revision cost comment missing	✅ Fixed in 7e23b6d
5	Archetype drift not enforced programmatically	✅ Fixed in be8e582

---

New Findings (Minor)

1. Retry count inconsistency: reviseDirectorScore uses 2 attempts, generateDirectorScore uses 3 (creative-director.ts:102 vs :245)

The PR response to issue 1 says "matching the pattern in generateDirectorScore", but generateDirectorScore uses maxRetries = 3 while reviseDirectorScore uses maxRetries = 2. Both patterns are correct; 2 retries is still reasonable and the difference is low-impact since the orchestrator's catch provides graceful degradation on total failure. Just worth noting the descriptions don't quite match.

---

2. evaluationsCompleted is semantically mismatched with the revisionRounds parameter (orchestrator.ts:514, cost-estimator.ts:120)

// orchestrator.ts
const costBreakdown = estimateCost(..., evaluationsCompleted);   // passed here

// cost-estimator.ts
const revisionCost = revisionRounds * (callCost(critic) + callCost(director)); // named this

evaluationsCompleted counts gate evaluate() calls (always ≥ 1 when the critic runs), while the cost formula charges for (critic + director) per unit. This means when the initial score passes (≥7) on the first try:

• evaluationsCompleted = 1 → bills for 1 × (critic + director)
• Actual cost: 1 × critic (no revision ran)
• Over-charges by 1 director-sized call ($0.003 on Anthropic)

Using revisionRoundsCompleted instead would charge only for rounds where a director call actually happened — still an approximation (undercounts by 1 critic per path), but semantically consistent with the parameter name revisionRounds. Given the "approximation" comment already in the code, this is cosmetic; just flag it in a follow-up if cost display accuracy is a concern.

---

3. Type cast mutation for archetype enforcement is slightly awkward (creative-director.ts:264-266)

if (validated.archetype !== originalScore.archetype) {
  (validated as { archetype: string }).archetype = originalScore.archetype;
}

This mutates validated through a cast. A cleaner alternative that avoids the cast entirely:

const data = validated.archetype !== originalScore.archetype
  ? { ...validated, archetype: originalScore.archetype }
  : validated;
return { data, usage: totalUsage };

No behavioral difference — purely a style preference. Not a blocker.

---

Correctness Checks (All Pass)

• Retry loop in reviseDirectorScore: Accumulates usage across attempts; throws on full exhaustion (caught by orchestrator). ✓
• Final evaluation (4af706b): revisionRoundsCompleted > 0 && score !== bestScore correctly guards the extra call — won't fire if no revisions happened or if the best score was already the last revised score. ✓
• Best-score tracking: Initialized to initialScore; updated only when critique.score > bestCritiqueScore. Score-degradation test in orchestrator-revision.test.ts validates this. ✓
• Archetype enforcement: Programmatic override after DirectorScore.parse — runs even on retry attempts. ✓
• evaluationsCompleted counter: Incremented in both the loop body and the final-evaluation block; both paths guarded by try-catch. ✓
• Worker meta.json: revisionHistory lazily initialized, written after each SSE "revision" event. ✓
• Test sync (4af706b + simulateRevisionLoop): Both orchestrator.ts and simulateRevisionLoop were updated in parallel in this commit, demonstrating the maintenance concern from issue 2 is real — though manageable given the small loop size. ✓

---

Verdict

This PR is in excellent shape. The logic is correct, the edge cases are covered, and the graceful-degradation path works at every level (evaluate failure, revise failure, parse error). The three new findings are all minor/cosmetic and don't affect correctness. Ready to merge.

[tsensei]

Response to re-review findings

Finding 1 (retry count 2 vs 3): Intentional. reviseDirectorScore uses 2 retries, generateDirectorScore uses 3. The difference: reviseDirectorScore runs inside a revision loop that already has graceful degradation (the orchestrator's try/catch skips the round on failure). 3 retries inside a round that can itself be skipped is excessive. 2 is deliberate.

Finding 2 (evaluationsCompleted semantic mismatch): Valid bug, fixed in c478868. estimateCost now takes separate gateEvaluations and revisionRounds params. Cost formula is evaluations * critic_cost + revisions * director_cost. When score >= 7 first try (1 eval, 0 revisions), we now correctly charge for 1 critic call only, not critic + director.

Finding 3 (type cast mutation): Style preference, keeping as-is. The cast only fires when archetype differs (rare path). The spread alternative allocates a new object on every call even on the happy path (the conditional would need to wrap the return, not just the assignment). Neither is better; current code is more explicit about the mutation being intentional.