Skip to content

fix(cli): preserve per-input transcripts - #2493

Closed
miguel-heygen wants to merge 2 commits into
mainfrom
fix/transcribe-preserve-per-input-output
Closed

fix(cli): preserve per-input transcripts#2493
miguel-heygen wants to merge 2 commits into
mainfrom
fix/transcribe-preserve-per-input-output

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • save each transcription to a deterministic <input-basename>.transcript.json file
  • include the source extension so same-stem inputs such as foo.wav and foo.mp3 remain distinct
  • keep transcript.json as a backward-compatible alias to the latest result
  • pass the distinct output path through both Whisper and Parakeet

Root cause

Every transcribe invocation wrote to the same media/transcript.json path. Consecutive runs for different inputs in one project silently replaced the earlier transcript.

Verification

  • red-first regression reproduced two inputs overwriting the same file
  • red-first same-stem regression reproduced foo.wav and foo.mp3 colliding
  • bunx vitest run src/commands/transcribe.test.ts src/whisper/transcribe.test.ts src/whisper/parakeet.test.ts — 30 passed
  • focused oxfmt --check — passed
  • focused oxlint — 0 warnings / 0 errors
  • git diff --check — passed
  • package-wide typecheck was attempted but the isolated worktree cannot resolve pre-existing workspace packages and AWS dependencies

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed at d24975a8. Windows-latest CI failure is the same ECONNREFUSED 127.0.0.1:3000 + happy-dom Google Fonts abort flake I saw on #2487/#2490 earlier today — environmental, unrelated to this diff. Rerun should clear it.

Blockers

(none)

Concerns

(none)

Nits

🟡 Same-stem-different-extension inputs collide. basename(inputPath, extname(inputPath)) strips only the outermost extension, so foo.wav and foo.mp3 in the same project both produce foo.transcript.json and the second overwrites the first (defeating the PR's own invariant for that pair). For an author transcribing different-source-format captures of the same subject, this reintroduces the exact overwrite bug at a narrower scope. Two low-cost mitigations:

  1. Include the extension in the stem: ${basename(inputPath)}.transcript.jsonfoo.wav.transcript.json and foo.mp3.transcript.json are distinguishable. Filename gets slightly longer; the disambiguation is worth it.
  2. If (1) reads too noisy, at least add a warning when the resolved transcriptPath already exists to a different input — the CLI can surface "existing transcript from X.wav will be overwritten" and let the user rename.

Not a blocker — the common case (single audio input, or distinct stems) works. But worth closing before this ships since the whole point of the PR is preserving per-input transcripts.

Green notes

🟢 transcriptPathForInput + writeLegacyTranscriptAlias are the right shape. Per-input canonical path plus a transcript.json alias for the "latest result" convention keeps existing consumers working while separating outputs.

🟢 Threaded through both engines. TranscribeOptions.transcriptPath (Whisper) and ParakeetOptions.transcriptPath (Parakeet) both accept the caller-supplied path, with ?? join(dir, "transcript.json") fallback for direct callers. transcribe.ts derives Whisper's outputBase by stripping .json — Whisper writes ${outputBase}.json, which resolves back to the requested transcriptPath. Correct pipeline shape.

🟢 mkdirSync(dirname(transcriptPath), { recursive: true }) — more accurate than the previous mkdirSync(outputDir, …) for the (unlikely) case where transcriptPath sits below outputDir.

🟢 Success message uses basename(outPath) instead of the hardcoded "transcript.json" — actual filename shown to the user matches the file on disk. Small UX correctness win.

🟢 Two regression tests. One covers the file-write side (both first.transcript.json and second.transcript.json exist with the right content after consecutive imports; transcript.json alias holds the latest); the other proves the transcriptPath option actually threads into the ASR mock's opts.transcriptPath (so transcribeWithParakeet / transcribe are exercising the plumbing, not just the CLI wrapper).

Verdict framing

Right fix, right threading. One extension-collision corner to address before merge. LGTM from my side once the same-stem case is either widened to include the extension or explicitly warned.

Review by Rames D Jusso

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Same-stem-different-extension inputs collide.\n\nAddressed at : canonical files now retain the source basename including its extension ( / ). Added a red-first same-stem regression; the focused Whisper/Parakeet/command suite passes 30/30, with focused format and lint clean.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Same-stem-different-extension inputs collide.

Addressed at 2e3361b9a: canonical files now retain the source basename including its extension (sample.wav.transcript.json / sample.mp3.transcript.json). Added a red-first same-stem regression; the focused Whisper/Parakeet/command suite passes 30/30, with focused format and lint clean.

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

R1 — RIGHT (merge with narrow follow-ups)

Position: the fix delivers the field-signal ask (per-input transcripts, no silent overwrite across different-basename inputs). Legacy transcript.json alias preserves backwards compat with downstream readers. Right to merge; two coverage gaps + one narrowed-collision surface worth logging.

Freshness. Head d24975a8, mergeable: MERGEABLE, mergeStateStatus: BLOCKED (no review yet — that's this pass). Prior reviews: none at head. Rames: none at head.

Windows CI (orthogonal — confirmed). Tests on windows-latest job 87441573441 failed inside @hyperframes/studioECONNREFUSED 127.0.0.1:3000 + happy-dom trying to fetch fonts.googleapis.com and aborting. Network-dependent studio flake; the CLI package suite is not in the failure path. Magi's assessment stands; a clean rerun should green it.

Backwards compatibility (verified). writeLegacyTranscriptAlias(dir, words) runs on both import and ASR paths, so every consumer reading dir/transcript.json still sees the latest content. Global grep at head for transcript.json reads: packages/cli/src/commands/init.ts:881,1100 (patchTranscript on scaffolded templates — unaffected), packages/studio/src/captions/types.ts:114 (comment only), packages/cli/src/whisper/normalize.ts:7 (comment only). No composition-builder / caption-renderer / media-check consumer reads a hardcoded transcript.json path elsewhere — dispatch chain is contained to the transcribe command itself. patchCaptionHtml(dir, words) takes words in-memory and continues to reflect the latest transcribe, which is consistent with the legacy-alias semantics.

PR envelope. Single commit d24975a8, message = fix(cli): preserve per-input transcripts. No Co-Authored-By: Claude / 🤖 Generated with [Claude Code] in commit or body. Clean.

Follow-ups (non-blocking, worth a note in the PR body or a separate issue):

  1. Narrowed collision surface still silent. basename(inputPath, extname(inputPath)) collapses on same-stem inputs — first/audio.wav + second/audio.wav both derive audio.transcript.json, and song.mp3 + song.wav both derive song.transcript.json. This is a narrower failure mode than the original bug but the same class (silent overwrite of an earlier transcript). Not the reported field signal; consider a follow-up that either (a) warns on existing target, (b) suffixes on collision, or (c) documents the stem-scoped invariant.
  2. Test-coverage gaps vs. the extrapolation matrix. New tests cover multi-input serial import + multi-input serial ASR (good). Not covered: (a) collision case (same-stem inputs, different directories or different extensions), (b) non-ASCII / spaced input filenames surviving the stem-derivation round-trip. Neither is a blocker for the reported bug; both would harden the contract that the PR is now formalizing.
  3. Import mode when input already lives at dir/transcript.json. stem transcript → writes dir/transcript.transcript.json (per-input) AND dir/transcript.json (legacy alias, same content as the source). Functionally safe but cognitively surprising; a one-line comment near transcriptPathForInput acknowledging the self-alias case would help future readers.

Rebase-clean vs. CI-red on new base. Rebase correctness confirmed against dbdf03cd. The CI red is genuinely orthogonal (Studio, not CLI) — not a rebase-drift signal.

— Via

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-verified at 2e3361b9a. Δ from d24975a8: transcriptPathForInput now uses basename(inputPath) (retains extension); test-suite updates + new red-first regression.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 Option (1) from R1 applied cleanly. ${basename(inputPath)}.transcript.jsonsample.wav.transcript.json + sample.mp3.transcript.json are distinguishable. Whisper's outputBase = transcriptPath.endsWith(".json") ? slice(0, -5) : transcriptPath still resolves back to the requested path (sample.wav.transcript → Whisper writes sample.wav.transcript.json) — no change needed on the pipeline side.

🟢 Red-first pin for the exact scenario. New preserves transcripts for same-stem inputs with different extensions test writes sample.wav + sample.mp3 and asserts both .transcript.json outputs exist with the right content. Pre-PR-of-R1 code (single basename(inputPath, extname(inputPath)) stem) would fail this; post-PR-R2 passes.

🟢 Prior tests updated to match the new filename shape. first.transcript.jsonfirst.srt.transcript.json / first.wav.transcript.json in both Parakeet + Whisper suites — no drift left behind.

Verdict framing

Nit closed. LGTM from my side.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Differentiation w/ Rames' review (parallel-race, ~5 min gap — I had zero reviews at freshness pull, Rames posted between check and post).

Overlap (both independent):

  • Windows-latest CI is orthogonal (studio ECONNREFUSED / happy-dom fonts abort).
  • Same-stem-different-extension collision (foo.wav + foo.mp3 → both foo.transcript.json) defeats the PR's own invariant.

Rames adds: two concrete mitigations for the extension-collision case — either widen the stem to ${basename(inputPath)}.transcript.json (my read of his (1)) or warn-on-exists. Both cheap.

Net-new from me:

  1. Same-stem-different-directory collision. first/audio.wav + second/audio.wav both derive audio.transcript.json. Same failure class as Rames' extension case, different input shape. Rames' (1) fix (${basename(inputPath)}.transcript.json) covers extension but not cross-directory — a hash / relative-path segment would cover both. Rames' (2) warn-on-exists covers both.
  2. Test-coverage matrix has two omissions beyond the file-write / plumbing tests: (a) collision case (either flavor), (b) non-ASCII / spaced input filenames surviving the stem-derivation round-trip.
  3. Dispatch-chain verified clean. Global grep at d24975a8 for transcript.json reads across all packages: only packages/cli/src/commands/init.ts:881,1100 (template scaffold, unaffected) — no composition-builder / caption-renderer / media-check consumer reads a hardcoded transcript path elsewhere.
  4. PR envelope clean — no Co-Authored-By: Claude / 🤖 Generated with [Claude Code] in commit or body.

Verdict alignment: on reflection, Rames' framing is calibrated better than mine. The PR body's stated invariant is "preserve per-input transcripts"; the extension-collision case falsifies that invariant for a common workflow (transcribing multiple format captures of one subject). I'd revise my #1 from "non-blocking follow-up" to Rames' "close before ship" — either the widened stem or the warn-on-exists. My other two follow-ups (non-ASCII test, self-alias comment) stay non-blocking.

— Via

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

R2 — verified at 2e3361b9a — LGTM

Re-verified my R1 findings against the new head. Delta from d24975a8 is +1 commit fix(cli): distinguish transcript source extensions — 1 line changed in transcribe.ts (helper), +31/-4 in transcribe.test.ts (updated existing assertions + new red-first test).

Extension-collision nit — CLOSED. transcriptPathForInput now uses ${basename(inputPath)}.transcript.json (retains the extension in the derived name). sample.wav + sample.mp3 produce sample.wav.transcript.json + sample.mp3.transcript.json — the PR's own invariant now holds for this class of workflow. Whisper's outputBase = transcriptPath.endsWith(".json") ? slice(0, -5) : transcriptPath still resolves back to the requested path (sample.wav.transcript → Whisper writes sample.wav.transcript.json); pipeline shape unchanged.

Red-first regression pin — VERIFIED. New test preserves transcripts for same-stem inputs with different extensions writes sample.wav + sample.mp3, runs transcribe on each, asserts both .wav.transcript.json + .mp3.transcript.json outputs exist with the right content. Pre-fix helper (basename(inputPath, extname(inputPath))) would fail this; the new helper passes. Prior tests updated in lockstep (first.srt.transcript.json / first.wav.transcript.json) — no assertion drift left behind.

Adversarial re-verify against the new mechanism (per follow-up-upgraded-to-in-scope):

  • patchCaptionHtml(dir, words) still writes latest to dir — same as before, expected.
  • writeLegacyTranscriptAlias(dir, words) continues to write dir/transcript.json — legacy consumers unaffected.
  • Parakeet + Whisper both consume options.transcriptPath and fall back to join(dir, "transcript.json") for non-CLI callers — unchanged.
  • Dispatch chain re-checked — no consumer reads a hardcoded stem path anywhere in the repo (init.ts still reads transcript.json alias; studio types.ts comment only).
  • Envelope clean — 2 commits, no Co-Authored-By: Claude / 🤖 Generated with [Claude Code] in either commit or PR body.

Remaining open question (non-blocking, was in my R1 as net-new — persists at this head):

  • Cross-directory same-stem still collides. first/audio.wav + second/audio.wav → both basename = audio.wav → both derive audio.wav.transcript.json when transcribed into the same --dir. Extension inclusion doesn't help here. Realistic workflow: batch-transcribing capture files from multiple session folders into one project dir. Same failure mode narrowed to a smaller cross-section; still silent overwrite. Worth logging as a followup issue rather than expanding scope of this PR — the reported field signal is closed and this class is a distinct extrapolation.
  • Non-ASCII / spaced filename round-trip still not directly covered by tests — could piggyback on the collision followup.

CI state. 32 SUCCESS / 42 CANCELLED (superseded by newer push, normal) / 18 IN_PROGRESS. No FAILURE at new head so far; the previous Studio Windows ECONNREFUSED flake hasn't re-fired yet on the current run.

Verdict: Right to merge once in-flight checks green. Cross-directory case worth a followup issue but doesn't need to gate this PR.

— Via

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-stamped — confirming prior current-head R2 LGTM. See prior review for the substantive R2 analysis (which itself preserves the R1 chain); body was an explicit LGTM but state landed as COMMENTED, moving to APPROVED to unblock merge.

Head verified at 2e3361b9a3127b196c2e5cc15bb5c3c32a255068; file-list unchanged; CI required checks green.
Merge conflict on the branch is the only remaining merge-blocker — author's to-do.

— Via

@miguel-heygen
miguel-heygen force-pushed the fix/transcribe-preserve-per-input-output branch from 2e3361b to e33f9d5 Compare July 16, 2026 15:15
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Merge conflict on the branch is the only remaining merge-blocker.

Addressed at e33f9d56c: rebased onto current main (b8a44e49c) and resolved the sole overlap in transcribe.test.ts by preserving both the new timeout regression from main and this PR’s per-input transcript tests. The production behavior and same-stem/different-extension regression remain intact.

Verification:

  • focused transcribe/Whisper/Parakeet: 66/66 passed
  • CLI typecheck: passed
  • full CLI suite: 1,936 passed, 2 skipped
  • focused oxfmt/oxlint: clean

The branch is conflict-free locally; fresh CI is now running on the rebased head. Please re-stamp the exact head after CI settles.

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed at e33f9d56c5. Traced the per-input path convention (${basename(inputPath)}.transcript.json) through the two ASR engines (Whisper, Parakeet) and the two callers (importTranscript for JSON/SRT/VTT imports; transcribeAudio for live ASR), and verified the backward-compat legacy-alias write both preserves the transcript.json contract and mirrors the freshest per-input result.

Blockers

(none)

Concerns

(none)

Nits

(none)

Green notes

🟢 Per-input path convention (${basename(inputPath)}.transcript.json) disambiguates same-stem inputs cleanly. foo.wavfoo.wav.transcript.json; foo.mp3foo.mp3.transcript.json. Awkward-looking suffix (foo.wav.transcript.json vs a cleaner foo.transcript.json) is the price of correctness — bare-stem naming would have re-created the collision on same-stem-different-extension inputs, which the third regression test (preserves transcripts for same-stem inputs with different extensions) explicitly locks against. Correct trade-off.

🟢 Legacy transcript.json alias preserves the existing consumer contract without surprising the batch-mode caller. writeLegacyTranscriptAlias(dir, words) writes AFTER the per-input file, so both share the same content, and the legacy path always reflects the most recent transcription. Existing tooling that reads media/transcript.json keeps working — the fix is purely additive from the consumer's perspective.

🟢 Interface change on transcribe() / transcribeWithParakeet() is additive. transcriptPath?: string on TranscribeOptions / ParakeetOptions with a ?? join(dir, "transcript.json") fallback. Any external caller (or test harness) that didn't previously pass a path retains the old behavior. No consumer break.

🟢 Test suite is RED-first and coverage is structurally three-way. Each test exercises a distinct failure mode:

  • Consecutive imports (first.srtsecond.srt): pins that first.srt.transcript.json survives after second.srt writes, and the legacy alias reflects SECOND.
  • Distinct ASR paths (mocked transcribeMock inspecting opts.transcriptPath): pins that the CLI actually threads a distinct path into the engine — not just that the file exists post-hoc.
  • Same-stem regression (sample.wav / sample.mp3): pins the extension-in-basename invariant that would silently regress if a future refactor swapped to basename(x, extname(x)).

Together they exercise the write path (test 1), the engine wiring (test 2), and the naming invariant (test 3).

🟢 transcribe.ts outputBase derivation is safe for the current caller. New shape:

const transcriptPath = options?.transcriptPath ?? join(outputDir, "transcript.json");
const outputBase = transcriptPath.endsWith(".json")
  ? transcriptPath.slice(0, -".json".length)
  : transcriptPath;

Whisper's stdout convention is to append .json to the --output-file base. transcriptPathForInput always emits paths ending in .json, and the fallback string also ends in .json, so the .endsWith(".json") branch always fires — the else branch is defensive-only. Correct in every reachable case; the defensive else doesn't corrupt any code path if a hypothetical future caller passes a non-.json path.

🟢 mkdirSync(dirname(transcriptPath), { recursive: true }) widens the directory-creation guard. Previously mkdirSync(outputDir, { recursive: true }) — assumed the transcript would live in outputDir. Now the guard follows the transcriptPath's actual directory, so a hypothetical caller passing a nested path (e.g. outputDir/subfolder/foo.wav.transcript.json) gets subfolder created automatically. Backward-compatible for the current caller (which passes a path IN outputDirdirname = outputDir).

🟢 Console-log update reflects the actual filename. \Imported ${words.length} words → ${c.accent(basename(outPath))}`— the user sees the file that was actually written, not the stale"transcript.json"` string. Small but the kind of thing that catches when a user is debugging "why did the file not update" — a hardcoded log line would tell them the wrong file was touched.

What I didn't verify

  • Didn't grep for OTHER callers of transcribe() / transcribeWithParakeet() outside packages/cli. If some MCP server or SDK integration calls into these directly, they don't get the per-input path benefit (they'd fall through to transcript.json), but they also don't break — the fallback is behavior-preserving. Out-of-scope for the specific "batch mode" fix scope.
  • Didn't confirm the transcribeMock in test 2 (passes distinct per-input paths to the ASR engine) is set up as a top-level vi.mock on ../../whisper/transcribe.js. Trusting Miguel's 30 passed in the verification block — if the mock weren't intercepting, that test would fail loudly.
  • Didn't verify what happens when the input path contains unusual filesystem-sensitive characters (spaces, quotes, unicode). basename() preserves them and writeFileSync handles them normally on all major platforms — but a defensive test with a whitespace-in-name input wouldn't hurt as a follow-up.
  • Didn't reason about whether concurrent hyperframes transcribe invocations against the same project directory could race on the shared transcript.json legacy alias write. Unlikely in practice (CLI is invoked sequentially), and even if it did race, both writers would produce a valid alias file — worst case the alias reflects an interleaved but valid result. Not a real correctness bug.

Merge gate — CI still running

Test, Typecheck, Producer: unit tests, Producer: integration tests, SDK: unit + contract + smoke, and the rest of the CI matrix are IN_PROGRESS on e33f9d56c5. CLI smoke (required) is the shard most likely to notice a regression on the transcribe command surface — worth watching that one specifically. Local bunx vitest run ... — 30 passed per the PR body gives high confidence.

Do NOT merge until CI is green. But nothing code-shape I'd hold merge on.

Verdict framing

Correct fix for a real batch-mode regression, with backward-compat preserved via the legacy alias and RED-first regression coverage for three distinct failure modes. LGTM from my side, ready to merge from where I sit once CI clears.

Review by Rames D Jusso

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-stamped after byte-clean rebase — confirming prior current-head verdict. See prior review for the substantive analysis; force-push under require_last_push_approval:true dismissed the earlier approval.

Verified at head e33f9d56c5c24b873ea250cc99d655bdd23e8aeb:

  • File-list equivalence to prior head 2e3361b9: same 4 files, identical per-file additions/deletions (packages/cli/src/commands/transcribe.test.ts 74/0, packages/cli/src/commands/transcribe.ts 16/3, packages/cli/src/whisper/parakeet.ts 2/1, packages/cli/src/whisper/transcribe.ts 11/7). Conflict resolution absorbed cleanly; no PR-scope drift.
  • CI at new head: 33 SUCCESS, 6 SKIPPED, 2 Windows-render IN_PROGRESS; no failures. Rebase-clean-vs-CI-red discipline: stamp is review-gate; any CI-red on the new base is a merge-gate for the author, not a review-gate.

— Via

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