feat(audio): the journalled serial transcription pipeline (4.8-4.11, 4.15, 4.17) - #11
Merged
Conversation
Plan 4.8 to 4.11, 4.15 and 4.17. `SttProvider` is one narrow boundary — a chunk in, its text and segments out — with two adapters that are not variations on one theme: Groq is an HTTP call that sends the audio to somebody else's machine, whisper.cpp is a subprocess that keeps it here. Each declares the container it wants a chunk in. Groq takes FLAC because fifteen minutes of 16 kHz mono PCM is ~28 MB against a 25 MB cap, so the one chunk length 4.7 is allowed to produce is exactly the one that would not fit; whisper.cpp takes WAV, which is what it reads natively. Everything the plan asks for around the boundary belongs to the pipeline rather than the adapters, because an adapter that also decided ordering and resuming would have to be written twice and would drift on the second. The journal is `adr:0012` made real: one chunk at a time, written to disk before the next one starts, so an application killed mid-run loses at most the chunk in flight. Both tracks are transcribed, so 4.12 can label `me` and `remote` by the track a passage came from. A failure records and carries on — 6.3 offers "redo only what failed", which needs the rest attempted — but three failures in a row stop the run, because that is the shape of a bad credential and finding it out otherwise costs twenty requests on a paid provider. A resume is refused when the journal no longer describes the same work. The dangerous case is not a different chunk count: it is the same count cut in different places, where every offset inside every chunk means something else and the result reads perfectly. Boundaries are compared one by one. Rebuilding absolute time takes two additions — the chunk's own start, and the time map. Doing only the first is the failure that looks right: every timestamp after the first chunk would be wrong by exactly the length of what came before it, and the transcript would still read. Segments are clamped into their chunk because Whisper over-runs, and an eleventh second that belongs to two chunks puts the same words in the timeline twice. The vocabulary lives in `@open-wiki/access`, because it reads pages. It is what stops the project's own name from coming out wrong — a model has never heard "Fenix" and writes "Phoenix", on every chunk. Bounded and ranked: Whisper's prompt window holds only its last 224 tokens. `transcriptionInputs` carries the language and the names and deliberately no credential. `config/secrets.ts` says the CLI, the hooks and the MCP process must not read the key because their stderr is consumed by an agent and travels to a model provider — which is why there is no `ow transcribe` verb, and why the orchestrator that reads the key lands with the desktop application. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
The sharpest one was backwards, not broken. Both the ranking and the prompt builder justified their bound with the same fact — Whisper's window holds only its *last* 224 tokens — and then emitted the names best-first, which puts "Fenix" exactly where truncation drops it. The feature would have degraded on precisely the projects with enough pages for it to matter, and no test could see it because every test used three names. The ranking stays best-first; the prompt is emitted reversed, and bounded in characters, which is what actually breaks: a page title from a cloned project became a megabyte of form field on the upload and an argv past Windows' limit locally. `absolute.ts` clamped the start against the chunk, the end against the recording, and resolved the wall time from a third value — so a passage could claim to start at 28 s, end at 25 s, and carry an instant belonging to neither. One ceiling now, applied once. And the clamp did not prevent the duplication its own comment described: a segment beginning past the chunk end was clipped to a zero-length passage on the boundary and emitted, so the next chunk returned the same words and both landed in the timeline. It is dropped. `sources/state.ts` had to move with the pipeline. `failed` meant "some chunk has an error", and the pipeline records an error and carries on — so one 429 twelve minutes into a healthy run made the source read as failed, with a progress count that kept climbing, for the remaining forty. `failed` now means nothing is left to try. Security: - `isJournal` validated the scheduling fields and not the content ones, while returning `value is Journal`. `text` and `segments` end up in the timeline and then in a wiki page carrying real provenance timestamps, and a journal marked complete makes 4.14 delete 690 MB of source audio without a provider having been called. Both are validated, and the chunk list is bounded. - The Groq request had no timeout and read an error body whole before truncating it. A stalled socket hung the run with no journal write and no progress; a streaming response exhausted memory. - `baseUrl` took any URL and the credential went with it. It requires https, and `redirect: "error"` makes "the key does not follow a redirect" a property of this code rather than of whichever fetch is underneath. - `basename` on the provider's filename, so `whispercpp.ts` does not depend on a numeric guard in another file to know it is not a path. Also: a network failure now retries. `adr:0012` names "the network drops between chunk four and chunk five" as motivating, and a rejecting `fetch` throws a TypeError rather than an SttError — so a 503 got three attempts and the most common transient failure got one. `createProvider` gained a real spawn default, so the local provider can be built from configuration alone instead of reporting a wiring bug as a settings error. And the language is checked on resume alongside the provider and the model: it was a field recorded and never read, and a `pt-BR` journal resumed in English produces one transcript in two languages. Four tests were replaced because they asserted the implementation rather than the requirement — the journal's atomic write, the over-run clamp, the map's refusal, and the durability of a *failure* before the next chunk is sent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
📝 WalkthroughWalkthroughThe PR adds journaled audio transcription with Groq and whisper.cpp providers, timestamp reconstruction, vocabulary injection, source-state updates, public exports, and comprehensive tests. ChangesTranscription stack
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant transcribeRecording
participant ffmpeg
participant SttProvider
participant Journal
transcribeRecording->>Journal: load or create recording journal
transcribeRecording->>ffmpeg: extract pending chunk audio
ffmpeg-->>transcribeRecording: return temporary audio file
transcribeRecording->>SttProvider: transcribe audio with language and vocabulary
SttProvider-->>transcribeRecording: return transcription result
transcribeRecording->>Journal: persist chunk success or failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
packages/audio/src/stt/provider.ts (1)
141-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider packing the budget instead of stopping at the first name that does not fit.
The loop uses
break. One long name near the front ends selection, so shorter and still useful names after it are dropped. Acontinuekeeps the best-first priority and fills the remaining budget.This is optional. The current behavior is bounded and correct.
♻️ Proposed change
for (const raw of vocabulary) { const name = raw.trim().slice(0, MAX_NAME_CHARS); if (!name) continue; const cost = name.length + (kept.length > 0 ? 2 : 0); - if (used + cost > maxChars) break; + if (used + cost > maxChars) continue; used += cost; kept.push(name); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/audio/src/stt/provider.ts` around lines 141 - 150, Update the vocabulary selection loop to skip names that exceed the remaining character budget rather than stopping at the first oversized name. In the loop processing raw vocabulary entries, replace the early termination behavior while preserving ordering, cost calculation, trimming, and the existing budget bounds.packages/audio/src/stt/groq.ts (1)
178-191: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCreate the
TextDecoderonce, outside the read loop.A new
TextDecoderis created for each chunk.{ stream: true }then carries no state between chunks. A multi-byte UTF-8 character split across two reads decodes as replacement characters in the error detail.♻️ Proposed change
const reader = body.getReader(); const parts: string[] = []; + const decoder = new TextDecoder(); let size = 0; try { while (size < MAX_ERROR_BODY_BYTES) { const { done, value } = await reader.read(); if (done || !value) break; size += value.byteLength; - parts.push(new TextDecoder().decode(value, { stream: true })); + parts.push(decoder.decode(value, { stream: true })); } } finally { await reader.cancel().catch(() => {}); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/audio/src/stt/groq.ts` around lines 178 - 191, Move the TextDecoder construction outside the read loop in the response-body reader, keeping one decoder instance for all chunks. Continue decoding each chunk with stream: true so split multi-byte UTF-8 characters are preserved, while retaining the existing size limit, cancellation, and truncation behavior.packages/audio/tests/stt.spec.ts (1)
184-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the timeout, not only the presence of a signal.
The test name states a bound on one attempt. The assertion only checks that
init.signalis anAbortSignal. It passes for any timeout value, and it would still pass if the bound were removed and some other signal were passed. Assert that a stalledfetchaborts, using a smalltimeoutMs.♻️ Proposed change
it("bounds how long one attempt may take", async () => { - let signal: AbortSignal | undefined; - const doFetch: FetchLike = async (_url, init) => { - signal = init.signal ?? undefined; - return new Response(JSON.stringify(body), { status: 200 }); - }; - await transcribeWith(createGroqProvider({ apiKey: "k", fetch: doFetch })); - expect(signal).toBeInstanceOf(AbortSignal); + // A stalled socket must not hang the run: the attempt's signal aborts it. + const doFetch: FetchLike = (_url, init) => + new Promise((_resolve, reject) => { + init.signal?.addEventListener("abort", () => reject(new Error("aborted"))); + }); + const provider = createGroqProvider({ + apiKey: "k", + fetch: doFetch, + timeoutMs: 5, + attempts: 1, + }); + await expect(transcribeWith(provider)).rejects.toThrow(/aborted/); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/audio/tests/stt.spec.ts` around lines 184 - 192, Update the “bounds how long one attempt may take” test to use a deliberately stalled doFetch and a small timeoutMs when creating the Groq provider, then assert that the fetch signal is aborted after the attempt times out. Replace the signal-type-only assertion while preserving the test’s existing transcription setup.packages/audio/src/stt/index.ts (1)
57-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the local run the same way the Groq request is bounded.
defaultSpawnwaits forclosewith no time limit and no kill path. A whisper.cpp process that stalls holds the serial pipeline forever, with no journal write and no progress.groq.tsalready argues this case for a stalled socket atDEFAULT_REQUEST_TIMEOUT_MS; the subprocess has the same failure mode.Add a timeout that kills the child and rejects.
♻️ Proposed change
function defaultSpawn(): WhisperCppOptions["run"] { return async (exe, args) => new Promise((resolvePromise, rejectPromise) => { const child = spawn(exe, [...args], { windowsHide: true }); let stderr = ""; + const timer = setTimeout(() => { + child.kill("SIGKILL"); + }, MAX_RUN_MS); child.stderr.on("data", (d: Buffer) => { // Bounded for the same reason `spawnFfmpeg` bounds its own: a run over // a damaged file emits a line per bad frame. stderr = (stderr + d.toString("utf8")).slice(-MAX_STDERR_CHARS); }); - child.on("error", rejectPromise); - child.on("close", (code) => resolvePromise({ code: code ?? -1, stderr })); + child.on("error", (e) => { + clearTimeout(timer); + rejectPromise(e); + }); + child.on("close", (code) => { + clearTimeout(timer); + resolvePromise({ code: code ?? -1, stderr }); + }); }); }Declare the bound beside
MAX_STDERR_CHARS:/** A local run of one chunk. Far beyond slow, and well short of forever. */ const MAX_RUN_MS = 30 * 60 * 1000;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/audio/src/stt/index.ts` around lines 57 - 70, Update defaultSpawn to enforce a MAX_RUN_MS timeout for each whisper.cpp child process, declared beside MAX_STDERR_CHARS. Start a timer after spawning; when it expires, kill the child and reject the promise, and clear the timer when the child closes or errors to avoid leaks.packages/audio/tests/transcribe.spec.ts (1)
343-418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a resume test for a changed content language.
transcribeRecordingis the only place that putslanguageinto the expectation passed tojournalMatches.journal.spec.tsproves the matcher refuses a language change, but no test proves the pipeline supplies that field. If the field is dropped from the expectation at Line 86 oftranscribe.ts, every test here still passes and apt-BRjournal resumes in English.💚 Proposed test
it("ignores a journal that will not parse and plans a fresh one", async () => {Add before that test:
it("refuses a journal written for another content language", async () => { writeJournal(dir, journalFor({ language: "pt-BR" })); const p = provider(said); await expect( transcribeRecording({ dir, provider: p.provider, language: "en", map, run: okFfmpeg }), ).rejects.toThrow(JournalMismatchError); expect(p.calls()).toBe(0); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/audio/tests/transcribe.spec.ts` around lines 343 - 418, Add a test in the “transcribeRecording resuming (4.17)” suite covering a journal with a different content language, using journalFor({ language: "pt-BR" }) while invoking transcribeRecording with language "en"; assert JournalMismatchError and zero provider calls, verifying the language field is included in the journalMatches expectation.packages/audio/src/journal.ts (1)
199-209: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFlush the temporary file before the rename.
renameSyncis atomic against concurrent readers, but it does not flush data. If the machine loses power after the rename and before the page cache reaches disk, the target can exist with truncated or empty content.readJournalthen returnsnull, which discards every chunk already transcribed — the outcome this function documents as the reason for the rename.Call
fsyncon the temporary file before renaming.♻️ Proposed refactor
-import { existsSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs"; +import { + closeSync, + existsSync, + fsyncSync, + openSync, + readFileSync, + renameSync, + rmSync, + writeFileSync, +} from "node:fs";export function writeJournal(dir: string, journal: Journal): void { const target = journalPath(dir); const temp = `${target}.tmp`; try { writeFileSync(temp, `${JSON.stringify(journal, null, 2)}\n`, "utf8"); + const handle = openSync(temp, "r+"); + try { + fsyncSync(handle); + } finally { + closeSync(handle); + } renameSync(temp, target); } catch (e) { rmSync(temp, { force: true }); throw e; } }A full guarantee also needs the containing directory synced after the rename. That is platform-dependent, so decide whether the file-level flush is enough for this product.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/audio/src/journal.ts` around lines 199 - 209, Update writeJournal to flush the temporary file with fsync before renameSync, using the temporary file descriptor and ensuring it is closed afterward while preserving cleanup and error propagation. Keep the existing atomic rename flow unchanged; do not add directory syncing unless required by the product decision.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/access/src/sources/state.ts`:
- Around line 109-115: Update the failed-state construction in the state
aggregation flow so journal.error takes precedence over the chunk-level failed
value when both are present; preserve chunk errors as the fallback when no
journal error exists. Add a regression case covering simultaneous journal and
chunk errors and assert that the returned terminal state exposes the journal
error.
In `@packages/audio/src/journal.ts`:
- Around line 131-141: Update the mismatch reason in the journal validation
branch to derive the reported current chunk count from the journal’s own
distinct boundaries, not from tracks.length or the expected track set. Remove
the tracks binding if it is unused after this change, while preserving the
existing expected chunk count and boundary-moved message.
In `@packages/audio/src/stt/whispercpp.ts`:
- Around line 76-92: Normalize the inputs before constructing paths and
arguments in the surrounding transcription flow: ensure request.filename yields
a non-directory fallback filename before writeFileSync receives input, and
ensure request.language produces a non-empty language value before adding the -l
argument. Preserve the existing basename and language-prefix behavior for valid
inputs.
- Around line 118-129: Update parseWhisperJson to validate and coerce
raw.offsets.from and raw.offsets.to before calculating startNs and endNs. Define
a reusable finiteMs helper outside parseWhisperJson that returns a finite
numeric millisecond value with the existing zero/from fallback behavior, then
use it for both offsets so generated segment timestamps are always finite.
---
Nitpick comments:
In `@packages/audio/src/journal.ts`:
- Around line 199-209: Update writeJournal to flush the temporary file with
fsync before renameSync, using the temporary file descriptor and ensuring it is
closed afterward while preserving cleanup and error propagation. Keep the
existing atomic rename flow unchanged; do not add directory syncing unless
required by the product decision.
In `@packages/audio/src/stt/groq.ts`:
- Around line 178-191: Move the TextDecoder construction outside the read loop
in the response-body reader, keeping one decoder instance for all chunks.
Continue decoding each chunk with stream: true so split multi-byte UTF-8
characters are preserved, while retaining the existing size limit, cancellation,
and truncation behavior.
In `@packages/audio/src/stt/index.ts`:
- Around line 57-70: Update defaultSpawn to enforce a MAX_RUN_MS timeout for
each whisper.cpp child process, declared beside MAX_STDERR_CHARS. Start a timer
after spawning; when it expires, kill the child and reject the promise, and
clear the timer when the child closes or errors to avoid leaks.
In `@packages/audio/src/stt/provider.ts`:
- Around line 141-150: Update the vocabulary selection loop to skip names that
exceed the remaining character budget rather than stopping at the first
oversized name. In the loop processing raw vocabulary entries, replace the early
termination behavior while preserving ordering, cost calculation, trimming, and
the existing budget bounds.
In `@packages/audio/tests/stt.spec.ts`:
- Around line 184-192: Update the “bounds how long one attempt may take” test to
use a deliberately stalled doFetch and a small timeoutMs when creating the Groq
provider, then assert that the fetch signal is aborted after the attempt times
out. Replace the signal-type-only assertion while preserving the test’s existing
transcription setup.
In `@packages/audio/tests/transcribe.spec.ts`:
- Around line 343-418: Add a test in the “transcribeRecording resuming (4.17)”
suite covering a journal with a different content language, using journalFor({
language: "pt-BR" }) while invoking transcribeRecording with language "en";
assert JournalMismatchError and zero provider calls, verifying the language
field is included in the journalMatches expectation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9ba65f3-6387-4040-ac6f-43824759c894
📒 Files selected for processing (25)
packages/access/src/gate/errors.tspackages/access/src/gate/guard.tspackages/access/src/index.tspackages/access/src/sources/state.tspackages/access/src/sources/transcription.tspackages/access/src/sources/vocabulary.tspackages/access/src/store/staleness.tspackages/access/tests/gate-guard.spec.tspackages/access/tests/sources-manifest.spec.tspackages/access/tests/sources-state.spec.tspackages/access/tests/sources-vocabulary.spec.tspackages/access/tests/store-staleness.spec.tspackages/audio/src/absolute.tspackages/audio/src/index.tspackages/audio/src/journal.tspackages/audio/src/stt/groq.tspackages/audio/src/stt/index.tspackages/audio/src/stt/provider.tspackages/audio/src/stt/whispercpp.tspackages/audio/src/transcribe.tspackages/audio/tests/absolute.spec.tspackages/audio/tests/journal.spec.tspackages/audio/tests/stt.spec.tspackages/audio/tests/transcribe.spec.tsplans/open-wiki.md
Comment on lines
+109
to
115
| const stopped = | ||
| journal?.error !== undefined || (chunks.length > 0 && !untried && done < chunks.length); | ||
| const error = failed !== undefined ? { error: failed } : {}; | ||
|
|
||
| if (stopped && !textReady) { | ||
| return { ...base, stage: "failed", ...error, ...progressOf(chunks.length, done) }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prefer the terminal journal error.
When journal.error and a chunk error both exist, failed selects the first chunk error at line 88. The terminal failed state then hides the journal-level stop reason. Give journal.error precedence and add a regression case with both error types.
Proposed fix
- const failed = chunks.find((c) => c.error)?.error ?? journal?.error;
+ const failed = journal?.error ?? chunks.find((c) => c.error)?.error;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/access/src/sources/state.ts` around lines 109 - 115, Update the
failed-state construction in the state aggregation flow so journal.error takes
precedence over the chunk-level failed value when both are present; preserve
chunk errors as the fallback when no journal error exists. Add a regression case
covering simultaneous journal and chunk errors and assert that the returned
terminal state exposes the journal error.
Comment on lines
+131
to
+141
| if (journal.chunks.length !== wanted.length) { | ||
| // Counted in cuts, not in units of work: the user recognises "the | ||
| // recording cuts into 3 chunks", not the 6 requests that makes over two | ||
| // tracks. | ||
| return { | ||
| ok: false, | ||
| reason: | ||
| `this journal covers ${journal.chunks.length / tracks.length} chunks and the ` + | ||
| `recording now cuts into ${expected.chunks.length} — the boundaries moved`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Derive the reported chunk count from the journal, not from the expected track count.
Line 138 divides journal.chunks.length by tracks.length, but tracks describes the current expectation. If the caller passes a different track set than the journal was planned with, the message reports the wrong number of cuts, and for three tracks it reports a fraction.
Count the journal's own distinct boundaries instead.
🐛 Proposed fix
const tracks = expected.tracks ?? TRACKS;
const wanted = planJournal(expected, journal.language).chunks;
if (journal.chunks.length !== wanted.length) {
+ const journalTracks = new Set(journal.chunks.map((c) => c.track)).size || 1;
// Counted in cuts, not in units of work: the user recognises "the
// recording cuts into 3 chunks", not the 6 requests that makes over two
// tracks.
return {
ok: false,
reason:
- `this journal covers ${journal.chunks.length / tracks.length} chunks and the ` +
+ `this journal covers ${journal.chunks.length / journalTracks} chunks and the ` +
`recording now cuts into ${expected.chunks.length} — the boundaries moved`,
};tracks then becomes unused in this branch; remove the binding if no other branch needs it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/audio/src/journal.ts` around lines 131 - 141, Update the mismatch
reason in the journal validation branch to derive the reported current chunk
count from the journal’s own distinct boundaries, not from tracks.length or the
expected track set. Remove the tracks binding if it is unused after this change,
while preserving the existing expected chunk count and boundary-moved message.
Comment on lines
+76
to
+92
| const input = join(dir, basename(request.filename)); | ||
| const outputStem = join(dir, "out"); | ||
| try { | ||
| writeFileSync(input, request.audio); | ||
| const args = [ | ||
| "-m", | ||
| options.modelPath, | ||
| "-f", | ||
| input, | ||
| "-l", | ||
| request.language.split("-")[0]!.toLowerCase(), | ||
| "-oj", | ||
| "-of", | ||
| outputStem, | ||
| ]; | ||
| const prompt = vocabularyPrompt(request.vocabulary); | ||
| if (prompt) args.push("--prompt", prompt); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Normalize a degenerate language or filename before building the args.
Line 86 passes request.language.split("-")[0] straight to -l. An empty language produces -l "", whisper.cpp fails, and the pipeline stops after three consecutive failures with only an exit code recorded. Line 76 has the same class of gap: basename of "", ".", or ".." resolves to a directory, so writeFileSync throws EISDIR.
Apply a fallback for both.
🛡️ Proposed fix
- const input = join(dir, basename(request.filename));
+ const safeName = basename(request.filename).replace(/^\.+$/, "") || "audio";
+ const input = join(dir, safeName);
const outputStem = join(dir, "out");
try {
writeFileSync(input, request.audio);
+ const language = request.language.split("-")[0]!.toLowerCase() || "auto";
const args = [
"-m",
options.modelPath,
"-f",
input,
"-l",
- request.language.split("-")[0]!.toLowerCase(),
+ language,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const input = join(dir, basename(request.filename)); | |
| const outputStem = join(dir, "out"); | |
| try { | |
| writeFileSync(input, request.audio); | |
| const args = [ | |
| "-m", | |
| options.modelPath, | |
| "-f", | |
| input, | |
| "-l", | |
| request.language.split("-")[0]!.toLowerCase(), | |
| "-oj", | |
| "-of", | |
| outputStem, | |
| ]; | |
| const prompt = vocabularyPrompt(request.vocabulary); | |
| if (prompt) args.push("--prompt", prompt); | |
| const safeName = basename(request.filename).replace(/^\.+$/, "") || "audio"; | |
| const input = join(dir, safeName); | |
| const outputStem = join(dir, "out"); | |
| try { | |
| writeFileSync(input, request.audio); | |
| const language = request.language.split("-")[0]!.toLowerCase() || "auto"; | |
| const args = [ | |
| "-m", | |
| options.modelPath, | |
| "-f", | |
| input, | |
| "-l", | |
| language, | |
| "-oj", | |
| "-of", | |
| outputStem, | |
| ]; | |
| const prompt = vocabularyPrompt(request.vocabulary); | |
| if (prompt) args.push("--prompt", prompt); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/audio/src/stt/whispercpp.ts` around lines 76 - 92, Normalize the
inputs before constructing paths and arguments in the surrounding transcription
flow: ensure request.filename yields a non-directory fallback filename before
writeFileSync receives input, and ensure request.language produces a non-empty
language value before adding the -l argument. Preserve the existing basename and
language-prefix behavior for valid inputs.
Comment on lines
+118
to
+129
| export function parseWhisperJson(body: WhisperJson): SttResult { | ||
| const segments: SttSegment[] = []; | ||
| for (const raw of body.transcription ?? []) { | ||
| const text = (raw.text ?? "").trim(); | ||
| if (!text) continue; | ||
| const from = raw.offsets?.from ?? 0; | ||
| segments.push({ | ||
| startNs: from * NS_PER_MS, | ||
| endNs: (raw.offsets?.to ?? from) * NS_PER_MS, | ||
| text, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the offsets before they reach the journal.
body comes from JSON.parse and is only cast to WhisperJson, so offsets.from and offsets.to are not guaranteed to be numbers. A non-numeric or non-finite offset makes startNs or endNs NaN.
That value then propagates across a module boundary. transcribe.ts assigns the segments to the chunk and calls writeJournal, and JSON.stringify writes NaN as null. On the next run, isSegment in journal.ts (Line 258) requires Number.isFinite, so isJournal rejects the file and readJournal returns null. Every chunk already paid for is discarded.
Coerce the offsets here.
🐛 Proposed fix
export function parseWhisperJson(body: WhisperJson): SttResult {
const segments: SttSegment[] = [];
for (const raw of body.transcription ?? []) {
const text = (raw.text ?? "").trim();
if (!text) continue;
- const from = raw.offsets?.from ?? 0;
+ const from = finiteMs(raw.offsets?.from, 0);
segments.push({
startNs: from * NS_PER_MS,
- endNs: (raw.offsets?.to ?? from) * NS_PER_MS,
+ endNs: finiteMs(raw.offsets?.to, from) * NS_PER_MS,
text,
});
}
+
+function finiteMs(value: unknown, fallback: number): number {
+ return typeof value === "number" && Number.isFinite(value) ? value : fallback;
+}Place finiteMs outside parseWhisperJson.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function parseWhisperJson(body: WhisperJson): SttResult { | |
| const segments: SttSegment[] = []; | |
| for (const raw of body.transcription ?? []) { | |
| const text = (raw.text ?? "").trim(); | |
| if (!text) continue; | |
| const from = raw.offsets?.from ?? 0; | |
| segments.push({ | |
| startNs: from * NS_PER_MS, | |
| endNs: (raw.offsets?.to ?? from) * NS_PER_MS, | |
| text, | |
| }); | |
| } | |
| export function parseWhisperJson(body: WhisperJson): SttResult { | |
| const segments: SttSegment[] = []; | |
| for (const raw of body.transcription ?? []) { | |
| const text = (raw.text ?? "").trim(); | |
| if (!text) continue; | |
| const from = finiteMs(raw.offsets?.from, 0); | |
| segments.push({ | |
| startNs: from * NS_PER_MS, | |
| endNs: finiteMs(raw.offsets?.to, from) * NS_PER_MS, | |
| text, | |
| }); | |
| } | |
| function finiteMs(value: unknown, fallback: number): number { | |
| return typeof value === "number" && Number.isFinite(value) ? value : fallback; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/audio/src/stt/whispercpp.ts` around lines 118 - 129, Update
parseWhisperJson to validate and coerce raw.offsets.from and raw.offsets.to
before calculating startNs and endNs. Define a reusable finiteMs helper outside
parseWhisperJson that returns a finite numeric millisecond value with the
existing zero/from fallback behavior, then use it for both offsets so generated
segment timestamps are always finite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes tasks 4.8, 4.9, 4.10, 4.11, 4.15 and 4.17 of
plans/open-wiki.md.What changed
SttProvideris one narrow boundary — a chunk in, its text and segments out — with two adapters that are not variations on one theme: Groq is an HTTP call that sends the audio to somebody else's machine, whisper.cpp is a subprocess that keeps it here. Each declares the container it wants a chunk in. Groq takes FLAC because fifteen minutes of 16 kHz mono PCM is ~28 MB against a 25 MB cap — the one chunk length 4.7 is allowed to produce is exactly the one that would not fit; whisper.cpp takes WAV, which is what it reads natively.The journal is
adr:0012-transcription-is-a-journalled-serial-pipelinemade real: one chunk at a time, on disk before the next one starts, so an application killed mid-run loses at most the chunk in flight. Both tracks are transcribed, so 4.12 can labelmeandremoteby the track a passage came from. A failure records and carries on — 6.3 offers "redo only what failed", which needs the rest attempted — but three failures in a row stop the run, because that is the shape of a bad credential and finding it out otherwise costs twenty requests on a paid provider.A resume is refused when the journal no longer describes the same work. The dangerous case is not a different chunk count: it is the same count cut in different places, where every offset inside every chunk means something else and the result reads perfectly.
Rebuilding absolute time takes two additions — the chunk's own start, and the time map. Doing only the first is the failure that looks right: every timestamp after the first chunk would be wrong by exactly the length of what came before it, and the transcript would still read.
Two things the reviews changed
The vocabulary ordering was backwards. Both modules justified their bound with the same fact — Whisper's window holds only its last 224 tokens — and then emitted the names best-first, which puts "Fenix" exactly where truncation drops it. It would have degraded on precisely the projects with enough pages to matter, and no test could see it because every test used three names.
sources/state.tshad to move with the pipeline.failedmeant "some chunk has an error", and the pipeline records an error and carries on — so one 429 twelve minutes into a healthy run made the source read as failed, with a progress count that kept climbing, for the remaining forty.failednow means nothing is left to try.Where the credential is, and is not
transcriptionInputscarries the language and the names and deliberately no credential.config/secrets.tsstates the rule: the CLI, the hooks and the MCP process must not read the key, because their stderr is consumed by an agent and travels to a model provider. That is why there is noow transcribeverb — the orchestrator that reads the key lands with the desktop application in group 8.The security review confirmed the rule holds transitively through the new exports, and that the key never reaches a URL, a form field, a log line, an error message, or the journal.
How it was verified
pnpm test— 739 passing (249 in@open-wiki/audio, 363 in@open-wiki/access)pnpm --filter @open-wiki/audio test:coverage— 99.1% lines, floor is 76%pnpm run typecheck,pnpm lint,prettier --check— cleanscc validate— no findingscode-reviewandsecurity-reviewsubagents run on the diff; every finding closed in f24335d, including four tests replaced because they asserted the implementation rather than the requirement4.9, 4.11 and 4.17 are
(TDD): red observed first — 28 assertion failures against signature-only stubs — then green.🤖 Generated with Claude Code
https://claude.ai/code/session_01D3VYWWTZtEE2NxPiksKsAK
Summary by CodeRabbit