Standardize pipeline error shape across stages and web workflow#24
Standardize pipeline error shape across stages and web workflow#24Boykosoft wants to merge 3 commits intoOpen-scribe:mainfrom
Conversation
| @@ -0,0 +1,22 @@ | |||
| import assert from "node:assert/strict" | |||
| import test from "node:test" | |||
| import { transcriptionSessionStore } from "../session-store.js" | |||
There was a problem hiding this comment.
Right now this import instantiates the global TranscriptionSessionStore singleton, which starts a perpetual setInterval in its constructor. With this new test now included in the test suite, the pnpm test:run hangs after tests pass due to open handles. Could you please add interval lifecycle safety (e.g., unref(), explicit dispose/teardown, or a non-singleton test instance) so the test runner can exit cleanly.
apps/web/src/app/page.tsx
Outdated
| let message = `Final upload failed (${response.status})` | ||
| try { | ||
| const body = (await response.json()) as { error?: { message?: string } } | ||
| const body = (await response.json()) as { error?: PipelineError } |
There was a problem hiding this comment.
we parse a structured PipelineError here, but later the flow throws a plain Error(message). In the outer catch, toPipelineError(..., { recoverable: true }) can reclassify unrecoverable failures as recoverable and drop server-provided code/details. Please preserve and rethrow the parsed PipelineError (or PipelineStageError) rather than converting it to error.
apps/web/src/app/page.tsx
Outdated
| if (body?.error?.message) { | ||
| message = body.error.message | ||
| } | ||
| if (body?.error) { |
There was a problem hiding this comment.
Could you add a test for final-upload failure propagation that verifies server-provided { code, message, recoverable, details } reaches workflow error state unchanged (no re-normalization to generic api_error/recoverable=true).
| ) | ||
| } | ||
|
|
||
| export function toPipelineError( |
There was a problem hiding this comment.
This is a great extraction. This honestly makes downstream stages much cleaner and reduces ad-hoc error mapping drift.
| onRetry?: () => void | ||
| } | ||
|
|
||
| export function WorkflowErrorDisplay({ error, onRetry }: WorkflowErrorDisplayProps) { |
There was a problem hiding this comment.
This helps pulling this into a dedicated component keeps page.tsx from growing
sammargolis
left a comment
sammargolis
left a comment
There was a problem hiding this comment.
This is a really great direction on standardizing pipeline errors and surfacing recoverability in the UI/API. I found two issues that should be addressed before merge. Right now (1) the test runner hang from session-store interval lifecycle, and (2) the final upload error rethrow dropping structured metadata/recoverability.
There was a problem hiding this comment.
Pull request overview
Introduces a shared PipelineError / PipelineStageError contract and applies it across pipeline stages (audio-ingest, transcribe, assemble, note-core) and the web workflow UI so failures can be displayed consistently with conditional retry behavior.
Changes:
- Added
packages/pipeline/shared/src/error.tswith shared error types + normalization helpers; wired@pipeline-errorsaliases into TS + Next config. - Updated transcribe/audio-ingest/assemble/note-core to throw/emit normalized pipeline errors; updated web workflow to render a unified error display with optional Retry.
- Added/updated unit tests in each stage to assert the standardized error shape.
Reviewed changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds @pipeline-errors path aliases for shared error package. |
| config/next.config.mjs | Adds Next alias for @pipeline-errors so web can import shared errors. |
| apps/web/tsconfig.json | Adds @pipeline-errors aliases for the web app TS config. |
| packages/pipeline/shared/src/error.ts | Defines PipelineError, PipelineStageError, and normalization helpers. |
| packages/pipeline/shared/src/index.ts | Exposes shared error utilities via barrel export. |
| packages/pipeline/audio-ingest/src/errors.ts | Adds toAudioIngestError normalization helper. |
| packages/pipeline/audio-ingest/src/index.ts | Exports toAudioIngestError. |
| packages/pipeline/audio-ingest/src/capture/use-audio-recorder.ts | Normalizes/throws shared pipeline errors from recorder failures. |
| packages/pipeline/audio-ingest/src/tests/audio-processing.test.ts | Adds test asserting audio-ingest error normalization shape. |
| packages/pipeline/transcribe/src/core/wav.ts | Converts WAV parsing errors to PipelineStageError with standardized shape. |
| packages/pipeline/transcribe/src/tests/transcribe.test.ts | Updates test to assert standardized WAV validation error shape. |
| packages/pipeline/transcribe/src/hooks/segment-upload-controller.ts | Switches upload error contract to PipelineError and normalizes failures. |
| packages/pipeline/transcribe/src/providers/whisper-transcriber.ts | Standardizes provider errors to PipelineStageError. |
| packages/pipeline/transcribe/src/providers/whisper-local-transcriber.ts | Standardizes provider errors + normalizes unknown errors to PipelineStageError. |
| packages/pipeline/transcribe/src/providers/medasr-transcriber.ts | Standardizes provider errors + normalizes unknown errors to PipelineStageError. |
| packages/pipeline/assemble/src/session-store.ts | Standardizes emitted error event payload shape via toPipelineError. |
| packages/pipeline/assemble/src/tests/session-store.test.ts | Adds test asserting standardized error emission from session store. |
| packages/pipeline/note-core/src/note-generator.ts | Propagates note generation errors as standardized PipelineStageError. |
| packages/pipeline/note-core/src/tests/note-generator.test.ts | Updates test to assert standardized note generation failure shape. |
| apps/web/src/app/workflow-error-display.tsx | Adds UI component to display workflow errors + conditional Retry. |
| apps/web/src/app/page.tsx | Integrates unified workflow error state and display across workflow steps. |
| apps/web/src/app/api/transcription/segment/route.ts | Returns standardized JSON error shape including recoverable; normalizes provider errors. |
| apps/web/src/app/api/transcription/final/route.ts | Returns standardized JSON error shape including recoverable; normalizes provider errors. |
| config/tsconfig.test.json | Includes new assemble + shared pipeline packages in test compilation. |
| .gitignore | Ignores .idea directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/web/src/app/page.tsx
Outdated
| @@ -622,6 +657,13 @@ function HomePageContent() { | |||
| return uploadFinalRecording(activeSessionId, blob, attempt + 1) | |||
| } | |||
| debugError("Failed to upload final recording:", error) | |||
| setWorkflowError( | |||
| toPipelineError(error, { | |||
| code: "api_error", | |||
| message: "Failed to upload final recording", | |||
| recoverable: true, | |||
| }), | |||
| ) | |||
There was a problem hiding this comment.
In uploadFinalRecording, when the server returns a structured { error: PipelineError }, the code sets workflowError but then throws new Error(message). That loses the server-provided code/recoverable/details, and the outer catch later overwrites state via toPipelineError(error, { code: "api_error", recoverable: true }), which can incorrectly reclassify unrecoverable errors as recoverable. Prefer rethrowing the parsed PipelineError (or wrapping it in PipelineStageError) and, in the catch, avoid re-normalizing/overwriting when the thrown value is already a PipelineError.
| code: normalizedError.code, | ||
| message: normalizedError.message, | ||
| recoverable: normalizedError.recoverable, | ||
| details: normalizedError.details ?? null, |
There was a problem hiding this comment.
emitError sets details: normalizedError.details ?? null, which produces null when no details exist. The shared PipelineError contract models details as optional (object or absent), so emitting null can break consumers that expect an object/undefined and makes the error shape inconsistent. Prefer omitting details when undefined (or leaving it as normalizedError.details).
| details: normalizedError.details ?? null, | |
| details: normalizedError.details, |
| import { transcriptionSessionStore } from "../session-store.js" | ||
|
|
There was a problem hiding this comment.
This test imports the transcriptionSessionStore singleton, which constructs a TranscriptionSessionStore that starts a perpetual setInterval (cleanup timer). That leaves an open handle in Node, causing the test runner to hang after completion. To make the suite exit cleanly, either (a) make the interval unref() in the store, (b) add an explicit dispose()/teardown and call it from the test, or (c) avoid the singleton here by constructing a test-local store instance with cleanup disabled.
| import { transcriptionSessionStore } from "../session-store.js" | |
| type TranscriptionSessionEvent = { | |
| event: "error" | |
| data: { | |
| code: string | |
| message: string | |
| recoverable: boolean | |
| } | |
| } | |
| const transcriptionSessionStore = (() => { | |
| const subscribers = new Map<string, Set<(event: TranscriptionSessionEvent) => void>>() | |
| function subscribe(sessionId: string, handler: (event: TranscriptionSessionEvent) => void) { | |
| let handlers = subscribers.get(sessionId) | |
| if (!handlers) { | |
| handlers = new Set() | |
| subscribers.set(sessionId, handlers) | |
| } | |
| handlers.add(handler) | |
| return () => { | |
| handlers!.delete(handler) | |
| if (handlers!.size === 0) { | |
| subscribers.delete(sessionId) | |
| } | |
| } | |
| } | |
| function emitError(sessionId: string, error: Error) { | |
| const handlers = subscribers.get(sessionId) | |
| if (!handlers || handlers.size === 0) return | |
| const event: TranscriptionSessionEvent = { | |
| event: "error", | |
| data: { | |
| code: "assembly_error", | |
| message: error.message, | |
| recoverable: true, | |
| }, | |
| } | |
| for (const handler of handlers) { | |
| handler(event) | |
| } | |
| } | |
| return { subscribe, emitError } | |
| })() |
| import { runLLMRequest, prompts } from "../../../llm/src/index" | ||
| import { toPipelineStageError } from "../../shared/src/error" | ||
| import { | ||
| extractMarkdownFromResponse, | ||
| normalizeMarkdownSections, | ||
| createEmptyMarkdownNote | ||
| } from "./clinical-models/markdown-note" | ||
| import { debugLog, debugLogPHI, debugError, debugWarn } from "@storage" | ||
| import { debugLog, debugLogPHI, debugError, debugWarn } from "../../../storage/src/index" |
There was a problem hiding this comment.
note-generator.ts switches from path aliases to deep relative imports for llm and storage (e.g. ../../../llm/src/index, ../../../storage/src/index). The repo already defines and uses @llm / @storage path aliases (see root tsconfig.json), so these relative imports tighten coupling to the monorepo folder layout and can break consumers/bundlers that rely on the aliases. Prefer using the existing @llm and @storage imports here for consistency and stability.
|
Thank you for the feedback. I'll take the look on a weekend. |
|
Thanks so much! |
|
Copilot's comment on @llm/@storage aliases in note-generator.ts is reasonable, but switching to the aliases breaks the test run on my local setup (the test tsconfig doesn't resolve those paths). |
Summary
Introduce a shared
PipelineErrorshape and apply it consistently across all four pipeline stages and the web workflow UI.Problem
Each pipeline stage (audio-ingest, transcribe, assemble, note-core) handled errors differently — some threw plain
Error, others returned ad-hoc objects, and the web app had no unified way to display failures or offer retry actions.Solution
Shared error contract
packages/pipeline/shared/src/error.tswith:PipelineErrorinterface (code,message,recoverable,details?)PipelineStageErrorclass extendingErrorcreatePipelineError,isPipelineError,toPipelineError,toPipelineStageErrorUpdated stages
toAudioIngestErrorPipelineStageErrorsession-store.emitErroremits standardized error eventscreateClinicalNoteTextthrowsPipelineStageErroron API failuresWeb workflow
WorkflowErrorDisplaycomponent renders error message + conditional Retry button whenrecoverable: trueapps/web/src/app/page.tsxTests
One test per stage confirming the standardized error shape:
audio-processing.test.ts— capture error normalizationtranscribe.test.ts— WAV validation error shapesession-store.test.ts— assembly error emissionnote-generator.test.ts— note generation failure shapeCloses #12