fix(cli): preserve per-input transcripts - #2493
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
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:
- Include the extension in the stem:
${basename(inputPath)}.transcript.json→foo.wav.transcript.jsonandfoo.mp3.transcript.jsonare distinguishable. Filename gets slightly longer; the disambiguation is worth it. - If (1) reads too noisy, at least add a warning when the resolved
transcriptPathalready exists to a different input — the CLI can surface "existing transcript fromX.wavwill 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.
|
Addressed at |
vanceingalls
left a comment
There was a problem hiding this comment.
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/studio — ECONNREFUSED 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):
- Narrowed collision surface still silent.
basename(inputPath, extname(inputPath))collapses on same-stem inputs —first/audio.wav+second/audio.wavboth deriveaudio.transcript.json, andsong.mp3+song.wavboth derivesong.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. - 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.
- Import mode when input already lives at
dir/transcript.json. stemtranscript→ writesdir/transcript.transcript.json(per-input) ANDdir/transcript.json(legacy alias, same content as the source). Functionally safe but cognitively surprising; a one-line comment neartranscriptPathForInputacknowledging 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
left a comment
There was a problem hiding this comment.
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.json → sample.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.json → first.srt.transcript.json / first.wav.transcript.json in both Parakeet + Whisper suites — no drift left behind.
Verdict framing
Nit closed. LGTM from my side.
vanceingalls
left a comment
There was a problem hiding this comment.
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→ bothfoo.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:
- Same-stem-different-directory collision.
first/audio.wav+second/audio.wavboth deriveaudio.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. - 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.
- Dispatch-chain verified clean. Global grep at
d24975a8fortranscript.jsonreads across all packages: onlypackages/cli/src/commands/init.ts:881,1100(template scaffold, unaffected) — no composition-builder / caption-renderer / media-check consumer reads a hardcoded transcript path elsewhere. - 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
left a comment
There was a problem hiding this comment.
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 todir— same as before, expected.writeLegacyTranscriptAlias(dir, words)continues to writedir/transcript.json— legacy consumers unaffected.- Parakeet + Whisper both consume
options.transcriptPathand fall back tojoin(dir, "transcript.json")for non-CLI callers — unchanged. - Dispatch chain re-checked — no consumer reads a hardcoded stem path anywhere in the repo (
init.tsstill readstranscript.jsonalias; 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→ bothbasename=audio.wav→ both deriveaudio.wav.transcript.jsonwhen 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
left a comment
There was a problem hiding this comment.
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
2e3361b to
e33f9d5
Compare
Addressed at Verification:
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
left a comment
There was a problem hiding this comment.
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.wav → foo.wav.transcript.json; foo.mp3 → foo.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.srt→second.srt): pins thatfirst.srt.transcript.jsonsurvives aftersecond.srtwrites, and the legacy alias reflectsSECOND. - Distinct ASR paths (mocked
transcribeMockinspectingopts.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 tobasename(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 outputDir — dirname = 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()outsidepackages/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 totranscript.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
transcribeMockin test 2 (passes distinct per-input paths to the ASR engine) is set up as a top-levelvi.mockon../../whisper/transcribe.js. Trusting Miguel's30 passedin 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 andwriteFileSynchandles 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 transcribeinvocations against the same project directory could race on the sharedtranscript.jsonlegacy 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.
vanceingalls
left a comment
There was a problem hiding this comment.
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.ts74/0,packages/cli/src/commands/transcribe.ts16/3,packages/cli/src/whisper/parakeet.ts2/1,packages/cli/src/whisper/transcribe.ts11/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
Summary
<input-basename>.transcript.jsonfilefoo.wavandfoo.mp3remain distincttranscript.jsonas a backward-compatible alias to the latest resultRoot cause
Every
transcribeinvocation wrote to the samemedia/transcript.jsonpath. Consecutive runs for different inputs in one project silently replaced the earlier transcript.Verification
foo.wavandfoo.mp3collidingbunx vitest run src/commands/transcribe.test.ts src/whisper/transcribe.test.ts src/whisper/parakeet.test.ts— 30 passedoxfmt --check— passedoxlint— 0 warnings / 0 errorsgit diff --check— passed