From 30a98a4e7ff41440cc96746cf5f593df14c41a13 Mon Sep 17 00:00:00 2001 From: John Brecht Date: Mon, 15 Jun 2026 22:37:56 -0700 Subject: [PATCH 1/2] fix(cli): fail loudly on render job path collisions (#65) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildRenderJobs now throws a clear error when two jobs resolve to the same work dir / output — i.e. a tutorial id that coincides with another tutorial's id+language suffix (e.g. `setup.es` source vs `setup` in es). Pre-existing edge (a benign sequential overwrite before); the new --render-concurrency makes it simultaneous corruption, so detect and reject it up front rather than ship a latent corruption path with the concurrency feature. Tests cover the collision + the legitimate case. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 2 +- packages/cli/src/render.ts | 32 ++++++++++++++++++++++++++++++-- packages/cli/test/render.test.ts | 13 +++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8afe6fb..3b86267 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ Everything below is opt-in. Notes for existing consumers: - **`render --phase record` runs without a prior `tts` phase (#50).** The TTS-free framing check — `--phase record --contact-sheet`, used to verify selectors and framing across a whole tutorial before paying for narration — no longer throws `run the tts phase first` on a fresh work dir. When `tts.json` is absent it falls back to silent placeholder timings (steps pace as silent), so you get a contact sheet at zero TTS cost. `--phase post` still requires real timings. New exports: `silentTTSResult`, `loadTTSResultIfPresent`. - **Broader recap-framing lint (#49).** The strict-mode lint that nudges the last step to close with a recap (#36) no longer false-warns on valid past-tense / accomplishment recaps (e.g. "you created an event, set up ticketing… from here you can…"). `RECAP_CUE_RE` now also matches accomplishment phrasing and the "from here you…" hand-off, bringing it to parity with the looser objective cue. Advisory-only — it never fails a render. -- **Parallel batch rendering (#62).** Render multiple tutorials × languages concurrently with `--render-concurrency ` (or `renderConcurrency` in config); default **1** (serial, unchanged). The record phase is mostly real-time waiting, so a batch (e.g. regenerating a whole tutorial set) runs much faster in parallel — near-linear up to CPU/RAM. **Opt-in because it requires a parallel-safe adapter** (concurrent renders each run their own `setup`/`teardown`, so a shared seed DB must isolate per render — see [adapters.md → Parallel rendering](docs/adapters.md#parallel-rendering)). New export: `mapLimit`. +- **Parallel batch rendering (#62).** Render multiple tutorials × languages concurrently with `--render-concurrency ` (or `renderConcurrency` in config); default **1** (serial, unchanged). The record phase is mostly real-time waiting, so a batch (e.g. regenerating a whole tutorial set) runs much faster in parallel — near-linear up to CPU/RAM. **Opt-in because it requires a parallel-safe adapter** (concurrent renders each run their own `setup`/`teardown`, so a shared seed DB must isolate per render — see [adapters.md → Parallel rendering](docs/adapters.md#parallel-rendering)). New export: `mapLimit`. Renders that would write the same work dir / output (a tutorial id colliding with another's id+language suffix) now fail with a clear error instead of corrupting each other under concurrency (#65). ## 0.12.0 — chapters that work on YouTube & Vimeo diff --git a/packages/cli/src/render.ts b/packages/cli/src/render.ts index 46b65f2..5cd87b6 100644 --- a/packages/cli/src/render.ts +++ b/packages/cli/src/render.ts @@ -65,19 +65,47 @@ export function resolveRenderConcurrency( return Number.isFinite(raw) && (raw as number) >= 1 ? Math.floor(raw as number) : 1; } +/** Work dir / output key for a job: `` or `.`. Two jobs that share it write the same paths. */ +function jobPathKey(id: string, lang: string | null): string { + return lang ? `${id}.${lang}` : id; +} + /** * Flatten discovered tutorials × languages into a flat render-job list, in * tutorial-major order. Languages are de-duplicated first: two jobs with the * same `(id, lang)` would target the same `.forge/` work dir and * `.mp4` output, which under `--render-concurrency > 1` means two * renders writing the same paths at once (e.g. `--lang "es,es"`). + * + * Throws on the rarer cross-tutorial collision (#65): distinct ids whose + * id+language suffixes coincide (e.g. a tutorial named `setup.es` and a tutorial + * `setup` rendered in `es` both resolve to `setup.es`). Serially that was a + * benign sequential overwrite; under concurrency it's simultaneous corruption, + * so fail loudly with a clear message instead. */ -export function buildRenderJobs( +export function buildRenderJobs( discovered: Array<{ tutorial: T }>, langs: Array, ): Array<{ tutorial: T; lang: string | null }> { const uniqueLangs = [...new Set(langs)]; - return discovered.flatMap(({ tutorial }) => uniqueLangs.map((lang) => ({ tutorial, lang }))); + const jobs = discovered.flatMap(({ tutorial }) => uniqueLangs.map((lang) => ({ tutorial, lang }))); + + const byKey = new Map(); + for (const job of jobs) { + const key = jobPathKey(job.tutorial.id, job.lang); + const prior = byKey.get(key); + if (prior) { + const desc = (j: { tutorial: T; lang: string | null }) => + `"${j.tutorial.id}" (${j.lang ? `lang ${j.lang}` : 'source language'})`; + throw new Error( + `Render job collision: ${desc(prior)} and ${desc(job)} both resolve to "${key}" — ` + + `same work dir (.forge/${key}) and output (${key}.mp4). Rename one tutorial so its ` + + `id and language suffix don't coincide with another's.`, + ); + } + byKey.set(key, job); + } + return jobs; } export async function renderCommand(globs: string[], opts: RenderCmdOptions): Promise { diff --git a/packages/cli/test/render.test.ts b/packages/cli/test/render.test.ts index da8f43c..9ead652 100644 --- a/packages/cli/test/render.test.ts +++ b/packages/cli/test/render.test.ts @@ -51,4 +51,17 @@ describe('buildRenderJobs (#62)', () => { it('produces exactly one job for a single tutorial rendered in the source language', () => { expect(buildRenderJobs([{ tutorial: { id: 'a' } }], [null])).toHaveLength(1); }); + + it('throws on a cross-tutorial id+suffix collision instead of letting jobs share a path (#65)', () => { + // `setup` rendered in `es` → "setup.es"; `setup.es` rendered source → also "setup.es". + expect(() => + buildRenderJobs([{ tutorial: { id: 'setup' } }, { tutorial: { id: 'setup.es' } }], [null, 'es']), + ).toThrow(/collision.*setup\.es/i); + }); + + it('does not flag legitimately distinct id+suffix combinations', () => { + expect(() => + buildRenderJobs([{ tutorial: { id: 'setup' } }, { tutorial: { id: 'teardown' } }], ['es', 'fr']), + ).not.toThrow(); + }); }); From 33cd51b943110b4fda88b524d744646239a56d47 Mon Sep 17 00:00:00 2001 From: John Brecht Date: Mon, 15 Jun 2026 22:43:21 -0700 Subject: [PATCH 2/2] docs/test: reframe #65 guard as a backstop; pin the real slug protection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review on PR #68: the cross-tutorial id+suffix collision can't occur through the CLI — validateTutorial's SLUG_RE forbids dots in ids and discoverTutorials rejects duplicate ids, so the real protection is the slug rule, not this guard. - Reframe the guard's comment + CHANGELOG: it's defense-in-depth (guards a future relaxed slug rule / a caller that skips validation), not a live corruption fix. - Add a spec test pinning that a dotted id (`setup.es`) is rejected at validation — the actual first line of defense for #65. - Note in the guard's own test that its literals deliberately bypass validation to exercise the backstop directly. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 2 +- packages/cli/src/render.ts | 11 +++++++---- packages/cli/test/render.test.ts | 5 ++++- packages/core/test/spec.test.ts | 6 ++++++ 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b86267..2ddcd19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ Everything below is opt-in. Notes for existing consumers: - **`render --phase record` runs without a prior `tts` phase (#50).** The TTS-free framing check — `--phase record --contact-sheet`, used to verify selectors and framing across a whole tutorial before paying for narration — no longer throws `run the tts phase first` on a fresh work dir. When `tts.json` is absent it falls back to silent placeholder timings (steps pace as silent), so you get a contact sheet at zero TTS cost. `--phase post` still requires real timings. New exports: `silentTTSResult`, `loadTTSResultIfPresent`. - **Broader recap-framing lint (#49).** The strict-mode lint that nudges the last step to close with a recap (#36) no longer false-warns on valid past-tense / accomplishment recaps (e.g. "you created an event, set up ticketing… from here you can…"). `RECAP_CUE_RE` now also matches accomplishment phrasing and the "from here you…" hand-off, bringing it to parity with the looser objective cue. Advisory-only — it never fails a render. -- **Parallel batch rendering (#62).** Render multiple tutorials × languages concurrently with `--render-concurrency ` (or `renderConcurrency` in config); default **1** (serial, unchanged). The record phase is mostly real-time waiting, so a batch (e.g. regenerating a whole tutorial set) runs much faster in parallel — near-linear up to CPU/RAM. **Opt-in because it requires a parallel-safe adapter** (concurrent renders each run their own `setup`/`teardown`, so a shared seed DB must isolate per render — see [adapters.md → Parallel rendering](docs/adapters.md#parallel-rendering)). New export: `mapLimit`. Renders that would write the same work dir / output (a tutorial id colliding with another's id+language suffix) now fail with a clear error instead of corrupting each other under concurrency (#65). +- **Parallel batch rendering (#62).** Render multiple tutorials × languages concurrently with `--render-concurrency ` (or `renderConcurrency` in config); default **1** (serial, unchanged). The record phase is mostly real-time waiting, so a batch (e.g. regenerating a whole tutorial set) runs much faster in parallel — near-linear up to CPU/RAM. **Opt-in because it requires a parallel-safe adapter** (concurrent renders each run their own `setup`/`teardown`, so a shared seed DB must isolate per render — see [adapters.md → Parallel rendering](docs/adapters.md#parallel-rendering)). New export: `mapLimit`. As a backstop, `buildRenderJobs` rejects any two jobs that would resolve to the same work dir / output with a clear error (#65) — though tutorial ids are already slug-validated (no dots), which prevents the cross-tutorial case through the normal CLI path. ## 0.12.0 — chapters that work on YouTube & Vimeo diff --git a/packages/cli/src/render.ts b/packages/cli/src/render.ts index 5cd87b6..372a5ae 100644 --- a/packages/cli/src/render.ts +++ b/packages/cli/src/render.ts @@ -77,11 +77,14 @@ function jobPathKey(id: string, lang: string | null): string { * `.mp4` output, which under `--render-concurrency > 1` means two * renders writing the same paths at once (e.g. `--lang "es,es"`). * - * Throws on the rarer cross-tutorial collision (#65): distinct ids whose + * Also throws on the rarer cross-tutorial collision (#65): distinct ids whose * id+language suffixes coincide (e.g. a tutorial named `setup.es` and a tutorial - * `setup` rendered in `es` both resolve to `setup.es`). Serially that was a - * benign sequential overwrite; under concurrency it's simultaneous corruption, - * so fail loudly with a clear message instead. + * `setup` rendered in `es` both resolve to `setup.es`). This is a **backstop** — + * `validateTutorial` already forbids dots in ids (`SLUG_RE`) and `discoverTutorials` + * rejects duplicate ids, so it can't occur through the normal CLI path today. It + * guards against a future relaxed slug rule or a caller that skips validation, + * where the collision would mean simultaneous corruption under concurrency rather + * than a benign sequential overwrite. */ export function buildRenderJobs( discovered: Array<{ tutorial: T }>, diff --git a/packages/cli/test/render.test.ts b/packages/cli/test/render.test.ts index 9ead652..6df019d 100644 --- a/packages/cli/test/render.test.ts +++ b/packages/cli/test/render.test.ts @@ -52,8 +52,11 @@ describe('buildRenderJobs (#62)', () => { expect(buildRenderJobs([{ tutorial: { id: 'a' } }], [null])).toHaveLength(1); }); - it('throws on a cross-tutorial id+suffix collision instead of letting jobs share a path (#65)', () => { + it('backstop: throws if two jobs would resolve to the same path (#65)', () => { // `setup` rendered in `es` → "setup.es"; `setup.es` rendered source → also "setup.es". + // Note: a dotted id can't reach here via the CLI (validateTutorial/SLUG_RE + // forbids it — see spec.test.ts); these literals bypass that to exercise the + // backstop directly. expect(() => buildRenderJobs([{ tutorial: { id: 'setup' } }, { tutorial: { id: 'setup.es' } }], [null, 'es']), ).toThrow(/collision.*setup\.es/i); diff --git a/packages/core/test/spec.test.ts b/packages/core/test/spec.test.ts index c9f4391..463e61c 100644 --- a/packages/core/test/spec.test.ts +++ b/packages/core/test/spec.test.ts @@ -31,6 +31,12 @@ describe('tutorial()', () => { expect(() => tutorial('x', [step('a', noop)], { id: 'Has Spaces' })).toThrow(/lowercase slug/); }); + it('rejects ids with dots — the first line of defense against render path collisions (#65)', () => { + // A dotted id like `setup.es` would otherwise collide with `setup` rendered + // in `es` (both → .forge/setup.es); the slug rule forbids it up front. + expect(() => tutorial('x', [step('a', noop)], { id: 'setup.es' })).toThrow(/lowercase slug/); + }); + it('rejects a non-function focus with the index in the message', () => { expect(() => tutorial('x', [{ narration: 'a', run: noop, focus: 'nope' as unknown as () => never }]),