fix(dictation): redact API keys from streaming transcriber errors - #852
Conversation
Streaming dictation (Deepgram, OpenAI Realtime) injected API keys into websocket URLs/headers and echoed unredacted transport errors and server-side error payloads. Wrap those error endpoints with providerio.Redact and switch the connect-error format specifier from %w to %s so the raw error cannot leak via errors.Unwrap(). Adds regression tests asserting the API key is not present in the returned stream error for both transcribers. Co-authored-by: euxaristia <25621994+euxaristia@users.noreply.github.com>
…edaction tests - Redact API key in OpenAI Realtime session configuration error. - Return redacted OpenAI Realtime write error from writeErrCh instead of discarding. - Move TestOpenAIRealtimeStreamTranscribeErrorRedaction and TestDeepgramStreamTranscribeErrorRedaction next to their source files. Refs Gitlawb#710
…close Address CodeRabbit nitpick: the server now drains the client's audio frames and waits for CloseStream before rejecting the connection with an API-key-bearing policy-violation close reason. This ensures StreamTranscribe observes the close reason (and redacts it) instead of failing earlier on a write error. Refs Gitlawb#710
…action Formatting the read error with %s dropped the unwrap chain, so an Esc abort of a Deepgram or OpenAI Realtime dictation no longer matched errors.Is(msg.err, context.Canceled) in the TUI and produced a spurious error toast on top of the cancellation notice. Return the cancellation sentinel itself before redacting — it carries no key — and keep the redacted flat string for genuine failures. Adds regression tests that cancel a live stream and assert the sentinel survives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Key cancellation detection on ctx.Err() instead of errors.Is(err, context.Canceled), which was racing against writer-goroutine transport errors that could overwrite the error value right before the check. Applied consistently across both transcriber implementations. Fix goroutine leaks and a possible hang in the cancellation regression tests.
Add regression tests for Esc-abort during WebSocket dial and right after accept (session update / first write) so those redaction sites keep returning context.Canceled for the TUI errors.Is check.
Esc can cancel after conn.Read already has an error frame. Check ctx.Err() after partial callbacks and on the realtimeError path so the stream still returns context.Canceled instead of a redacted failure notice. Refs Gitlawb#710
handleDictationTranscribed suppressed context.Canceled but then fell through to the msg.submit branch. A delta -> Esc -> already-buffered realtime error race returns a nonempty transcript alongside context.Canceled, so with stt.autoSubmit on, Esc could submit the composer's restored pre-existing text instead of doing nothing. Treat a canceled result as terminal before the auto-submit path is reached, and add a TUI-level test covering the race.
…fter Esc and synchronize write cancellation test
…on 0 messages Ensure the first dictation session in a process gets stale-message protection by initializing sessionID to 1 in newDictationController and removing msg.sessionID != 0 exemptions in handleDictationTranscribed and handleDictationPartial. Clean up unreachable redaction guards in providerio. Refs Gitlawb#710
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDeepgram and OpenAI Realtime transcription now preserve ChangesDictation reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DictationController
participant StreamingTranscriber
participant Context
participant Composer
DictationController->>StreamingTranscriber: start stream with sessionID
StreamingTranscriber->>DictationController: return partial or completed result with sessionID
DictationController->>Context: check cancellation
DictationController->>DictationController: validate sessionID
DictationController->>Composer: apply active transcript
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Streaming dictation (Deepgram, OpenAI Realtime) injected API keys into websocket URLs/headers and echoed unredacted transport errors and server-side error payloads. Wrap those error endpoints with providerio.Redact and switch the connect-error format specifier from %w to %s so the raw error cannot leak via errors.Unwrap(). Adds regression tests asserting the API key is not present in the returned stream error for both transcribers. Co-authored-by: euxaristia <25621994+euxaristia@users.noreply.github.com>
…edaction tests - Redact API key in OpenAI Realtime session configuration error. - Return redacted OpenAI Realtime write error from writeErrCh instead of discarding. - Move TestOpenAIRealtimeStreamTranscribeErrorRedaction and TestDeepgramStreamTranscribeErrorRedaction next to their source files. Refs Gitlawb#710
…close Address CodeRabbit nitpick: the server now drains the client's audio frames and waits for CloseStream before rejecting the connection with an API-key-bearing policy-violation close reason. This ensures StreamTranscribe observes the close reason (and redacts it) instead of failing earlier on a write error. Refs Gitlawb#710
…action Formatting the read error with %s dropped the unwrap chain, so an Esc abort of a Deepgram or OpenAI Realtime dictation no longer matched errors.Is(msg.err, context.Canceled) in the TUI and produced a spurious error toast on top of the cancellation notice. Return the cancellation sentinel itself before redacting — it carries no key — and keep the redacted flat string for genuine failures. Adds regression tests that cancel a live stream and assert the sentinel survives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Key cancellation detection on ctx.Err() instead of errors.Is(err, context.Canceled), which was racing against writer-goroutine transport errors that could overwrite the error value right before the check. Applied consistently across both transcriber implementations. Fix goroutine leaks and a possible hang in the cancellation regression tests.
Add regression tests for Esc-abort during WebSocket dial and right after accept (session update / first write) so those redaction sites keep returning context.Canceled for the TUI errors.Is check.
Esc can cancel after conn.Read already has an error frame. Check ctx.Err() after partial callbacks and on the realtimeError path so the stream still returns context.Canceled instead of a redacted failure notice. Refs Gitlawb#710
handleDictationTranscribed suppressed context.Canceled but then fell through to the msg.submit branch. A delta -> Esc -> already-buffered realtime error race returns a nonempty transcript alongside context.Canceled, so with stt.autoSubmit on, Esc could submit the composer's restored pre-existing text instead of doing nothing. Treat a canceled result as terminal before the auto-submit path is reached, and add a TUI-level test covering the race.
…fter Esc and synchronize write cancellation test
…on 0 messages Ensure the first dictation session in a process gets stale-message protection by initializing sessionID to 1 in newDictationController and removing msg.sessionID != 0 exemptions in handleDictationTranscribed and handleDictationPartial. Clean up unreachable redaction guards in providerio. Refs Gitlawb#710
2814b35 to
fee2e2e
Compare
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for the rework — the cancellation half of this is in good shape now. I mutated each of the eight ctx.Err() != nil guards in the two transcribers one at a time (dial, read, session-update write, delta, completed, error-event, writeErrCh, and the Deepgram pair) and every one of them turned a test red, so the %w → %s flattening no longer costs you errors.Is(err, context.Canceled). That was the thing that broke last time and it's properly pinned.
The best part of the PR isn't in the title, though: the session-ID gate. I set up a model with sessionID=2, phase=dictRecording and live streamStop/cancel hooks, then fed it dictationTranscribedMsg{sessionID: 1}. With the gate, phase stays dictRecording and neither hook fires. With the gate disabled, phase drops to dictIdle and both streamStop() and cancel() are called — a stale completion from the previous session kills the mic on the session the user just started. That's a genuine bug on main and it's worth saying so in the description.
Some things to fix before this goes in.
The providerio.Redact change doesn't belong in this PR. I reverted internal/providers/providerio/providerio.go to main and reran all twelve new dictation tests — every one still passes, including transcriber_deepgram_test.go:154 TestDeepgramCustomHeaderErrorRedaction, which is named for the new X-Api-Key branch. It passes because the key is passed to Redact as an explicit secret, so the literal strings.ReplaceAll scrubs it and the header-word scavenging never runs. Meanwhile redact_classify_test.go wasn't touched, so Token / X-Api-Key / api-key have no coverage anywhere, and Redact sits on every provider error path in the repo. It also picks up new false positives — Redact("invalid Token gpt-4o-transcribe-2026-01-01") returns invalid Token [REDACTED]. Please drop it here; if you want the extra header names, they're a separate PR with tests in providerio's own package.
Three tests stay green when the thing they're named for is deleted.
-
transcriber_deepgram_test.go:211—TestDeepgramSinglePassRedactionasserts the error doesn't contain"[REDACTED:[REDACTED", a stringRedactnever produces. I replaced line 122 withfmt.Errorf("Deepgram stream error: %s", err.Error())(redaction gone entirely):TestDeepgramStreamTranscribeErrorRedactionFAIL,TestDeepgramCustomHeaderErrorRedactionFAIL,TestDeepgramSinglePassRedactionPASS. Either assert what you mean (exactly one[REDACTED]for one occurrence of the key) or delete it. -
internal/tui/dictation_test.go:139—TestDictationCanceledStreamRaceDoesNotAutoSubmitpasses withdictation.go:402disabled (if false && errors.Is(msg.err, context.Canceled)). The test buildsmodel{}sosessionIDis 0,cancelDictationbumps it to 1, and the message carries 0 — the session-ID gate atdictation.go:382catches it before thecontext.Canceledbranch is reached. To actually test that branch, give the message the samesessionIDthe controller currently holds. -
internal/tui/dictation_stream.go:82— the partial gate passes withif false && msg.sessionID != .... Both new stale-partial assertions are already satisfied by the pre-existing phase guard, since phase isdictIdleaftercancelDictation. The gate is real, it just needs the live-session case:sessionID=2,phase=dictRecording, composer"user typed this", thenhandleDictationPartial(sttPartialMsg{sessionID: 1, text: "ghost text from session 1"})— with the gate you get"user typed this", without it"user typed this ghost text from session 1".
transcriber_deepgram.go:122 dropped the //nolint:staticcheck that main carried on that line. make lint-static now reports ST1005: error strings should not be capitalized. It's advisory in CI so nothing goes red, but it was a deliberate suppression for the user-facing "Deepgram stream error:" text — please put it back.
Two smaller things, not blocking:
transcriber_openai_realtime.go:199-203 changes behaviour relative to main. Main silently swallowed a non-nil werr in the bottom writeErrCh select; you now abort the stream with a redacted error. I think that's the right call, but it isn't mentioned in the description and nothing pins it — replacing the final else with _ = werr leaves ./internal/dictation green.
And on the framing: I don't think the connect path was leaking. I stood up a real httptest server that 401s and echoes back the Authorization header it received — the server does see Token sk-probe-… and Bearer sk-probe-…, but coder/websocket v1.8.15 reports only failed to WebSocket dial: expected handshake response status code 101 but got 401, and a refused dial surfaces just the query-string URL, which carries no key. The realistic surface is server-echoed text (the Deepgram close reason, the OpenAI realtime error event), which is worth redacting as defence in depth. Could you reword the summary to say that rather than "could leak the raw API key"? It matters for anyone reading the changelog later.
Happy to re-review as soon as the providerio change is out and the three tests pin what they claim.
Everything in one pass, nothing queued behind it. Worth restating the shape of this one: the code changes here are good — the cancellation fix is properly pinned and the session-ID gate closes a real bug on main. What I'm asking for is mostly on the test side, plus dropping the providerio widening into its own PR where it can be covered properly. That's a much smaller ask than the count suggests.
…b.com/euxaristia/zero into fix/streaming-dictation-key-redaction
Revert the Token/X-Api-Key providerio.Redact expansion (belongs in its own PR with coverage). Make Deepgram single-pass, cancel-race, and live-session partial tests pin the branches they name, and restore the ST1005 nolint on the user-facing Deepgram error string.
|
Addressed review:
Framing note: defence-in-depth redaction of server-echoed close/error text (not the websocket dial path itself). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/dictation/transcriber_deepgram_test.go (1)
154-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename this test to match what it verifies.
The name
TestDeepgramCustomHeaderErrorRedactionimplies a custom-header feature. The transcriber sends onlyAuthorization: Token <key>, and this test exercises the same stream-error redaction path asTestDeepgramStreamTranscribeErrorRedaction. The only difference is the shape of the close reason. Rename the test so the intent is clear.♻️ Proposed rename
-func TestDeepgramCustomHeaderErrorRedaction(t *testing.T) { +func TestDeepgramStreamErrorRedactsHeaderShapedKey(t *testing.T) {🤖 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 `@internal/dictation/transcriber_deepgram_test.go` around lines 154 - 183, Rename TestDeepgramCustomHeaderErrorRedaction to clearly describe stream-error redaction for a close reason containing an API key, matching the intent of TestDeepgramStreamTranscribeErrorRedaction. Do not alter the test behavior or implementation.internal/dictation/transcriber_openai_realtime_test.go (2)
193-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the review provenance from this test comment.
The comment starts with "jatmn (review, PR
#710)". A reviewer handle and a pull request number age badly and add no information about the behavior under test. Keep the technical rationale and remove the attribution.♻️ Proposed edit
-// jatmn (review, PR `#710`): Esc can race an incoming OpenAI error event. -// conn.Read may already have the error frame in hand by the time the -// cancellation lands. The server fires a delta immediately followed by +// Esc can race an incoming OpenAI error event. conn.Read may already have +// the error frame in hand by the time the cancellation lands. The server +// fires a delta immediately followed by🤖 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 `@internal/dictation/transcriber_openai_realtime_test.go` around lines 193 - 199, Remove the “jatmn (review, PR `#710`):” attribution from the comment in the affected test, while preserving the technical explanation about cancellation racing the incoming OpenAI error event and the expected context.Canceled result.
147-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test does not reliably exercise the
writeErrChcancel branch.After the writer sends the single buffered chunk, it blocks on
range chunks. The cancel then unblocksconn.Read(ctx)in the main loop first. So the returned error normally comes from the read-error path intranscriber_openai_realtime.goLines 130-140, not from the writer-error branch at Lines 199-201.The assertion still passes, because both branches return
context.Canceled. Correct the comment so it does not claim coverage the test does not provide. If you want to pin the writer branch, keep audio flowing after the cancel so awriteJSONcall fails while the loop is still processing events.♻️ Proposed comment correction
-// After a server event the client selects writeErrCh. Cancel while the writer -// is blocked on more chunks so that path observes a cancelled write and must -// still return context.Canceled rather than a redacted flat string. +// Cancel while the writer is blocked on more chunks. Whichever path observes +// the cancellation first (the blocked conn.Read or the writeErrCh drain) must +// return context.Canceled rather than a redacted flat string.🤖 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 `@internal/dictation/transcriber_openai_realtime_test.go` around lines 147 - 191, Correct the comment above TestOpenAIRealtimeStreamTranscribeWriteCancelKeepsSentinel to describe the actual read-error cancellation path rather than claiming it exercises writeErrCh. Do not change the test assertion or behavior; only remove the inaccurate writer-branch coverage claim and state that cancellation must preserve context.Canceled.internal/dictation/transcriber_openai_realtime.go (1)
183-202: 🩺 Stability & Availability | 🔵 TrivialRun the affected OpenAI Realtime concurrency tests with the race detector.
Go test -racerequiresCGO_ENABLED=1; include those tests in the PR check run, for example:CGO_ENABLED=1 go test -race ./internal/dictation \ -run 'TestOpenAIRealtimeStreamTranscribe.*(Cancel|Race|Redaction|WriteCancel|DialCancel|StartupCancel)$'Add the result to the PR description per the concurrent-code guideline.
🤖 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 `@internal/dictation/transcriber_openai_realtime.go` around lines 183 - 202, Run the specified OpenAI Realtime concurrency test subset in internal/dictation with CGO_ENABLED=1 and the race detector enabled, then add the command and its result to the pull request description. Do not alter the implementation unless the race run exposes a failure.Source: Coding guidelines
🤖 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 `@internal/tui/dictation.go`:
- Line 317: Add sessionID propagation to startBatchRecordingCmd and
dictationStartedMsg, then update handleDictationStarted to ignore messages whose
session ID no longer matches the active dictation session before changing state
or calling reset. Add regression tests covering stale successful and failed
startup messages after cancellation, ensuring the current recording remains
unaffected.
---
Nitpick comments:
In `@internal/dictation/transcriber_deepgram_test.go`:
- Around line 154-183: Rename TestDeepgramCustomHeaderErrorRedaction to clearly
describe stream-error redaction for a close reason containing an API key,
matching the intent of TestDeepgramStreamTranscribeErrorRedaction. Do not alter
the test behavior or implementation.
In `@internal/dictation/transcriber_openai_realtime_test.go`:
- Around line 193-199: Remove the “jatmn (review, PR `#710`):” attribution from
the comment in the affected test, while preserving the technical explanation
about cancellation racing the incoming OpenAI error event and the expected
context.Canceled result.
- Around line 147-191: Correct the comment above
TestOpenAIRealtimeStreamTranscribeWriteCancelKeepsSentinel to describe the
actual read-error cancellation path rather than claiming it exercises
writeErrCh. Do not change the test assertion or behavior; only remove the
inaccurate writer-branch coverage claim and state that cancellation must
preserve context.Canceled.
In `@internal/dictation/transcriber_openai_realtime.go`:
- Around line 183-202: Run the specified OpenAI Realtime concurrency test subset
in internal/dictation with CGO_ENABLED=1 and the race detector enabled, then add
the command and its result to the pull request description. Do not alter the
implementation unless the race run exposes a failure.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c60c841f-6d6a-47e3-8f2f-2d819886213b
📒 Files selected for processing (7)
internal/dictation/transcriber_deepgram.gointernal/dictation/transcriber_deepgram_test.gointernal/dictation/transcriber_openai_realtime.gointernal/dictation/transcriber_openai_realtime_test.gointernal/tui/dictation.gointernal/tui/dictation_stream.gointernal/tui/dictation_test.go
Gate dictationStartedMsg on session ID so a late Start result after cancel cannot reset a newer recording. Rename and comment cleanups from review; race suite recorded on the PR body.
|
Addressed CodeRabbit follow-up:
|
|
Checked this against Vasanthdev2004's last review — the providerio revert, the restored nolint comment, and all three test fixes (redaction count assertion, matching sessionID in the cancel-race test, and the live-session partial gate) are already in the pushed commits, along with the coderabbit nitpicks (test rename, dropped review-provenance comment). Build and tests pass. Ready for re-review. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/dictation/transcriber_deepgram_test.go`:
- Around line 13-46: Extend internal/dictation/transcriber_deepgram_test.go at
lines 13-46 with a controllable non-cancellation WebSocket dial failure
containing the configured API key, then assert StreamTranscribe’s returned error
omits it. In internal/dictation/transcriber_openai_realtime_test.go at lines
13-43, add equivalent redaction tests for dial, session-configuration, and
asynchronous writer failures, ensuring each injected failure includes the API
key and each returned error excludes it; anchor the tests to Deepgram’s
StreamTranscribe and the OpenAI realtime transcription entry points.
In `@internal/dictation/transcriber_deepgram.go`:
- Around line 119-123: Update the audio writer loops in
internal/dictation/transcriber_deepgram.go:119-123 and
internal/dictation/transcriber_openai_realtime.go:137-140 to select between
receiving the next chunk and ctx.Done() while waiting, so cancellation exits the
writer even when chunks remains open and idle; preserve the existing chunk
processing and error behavior for non-cancelled sessions.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a396d4bd-584c-4de4-9264-0d25aa6578b5
📒 Files selected for processing (7)
internal/dictation/transcriber_deepgram.gointernal/dictation/transcriber_deepgram_test.gointernal/dictation/transcriber_openai_realtime.gointernal/dictation/transcriber_openai_realtime_test.gointernal/tui/dictation.gointernal/tui/dictation_stream.gointernal/tui/dictation_test.go
…aths Select on ctx.Done while waiting for audio chunks so cancelled streaming sessions do not leave writer goroutines blocked on an open idle channel. Add non-cancellation redaction tests for Deepgram dial failure and OpenAI Realtime dial, session-configuration, and async writer failures. Refs Gitlawb#852
|
Addressed the two open CodeRabbit majors on
Tests Both pass. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 13 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/dictation/transcriber_openai_realtime.go`:
- Around line 108-119: Update the audio writer failure paths in the
transcription flow to send the write error to writeErrCh and then close the
WebSocket connection, ensuring conn.Read(ctx) unblocks. Apply this ordering to
both the commit write after channel closure and the append write failure. Add a
regression test covering a silent server after session setup with an injected
audio write failure, verifying the main goroutine observes the error.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f9c408f1-61fc-4fbd-ba7a-b4899b478139
📒 Files selected for processing (4)
internal/dictation/transcriber_deepgram.gointernal/dictation/transcriber_deepgram_test.gointernal/dictation/transcriber_openai_realtime.gointernal/dictation/transcriber_openai_realtime_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/dictation/transcriber_deepgram.go
|
Checked head
Non-blocking: the bottom Verification at head: |
…nt server cannot strand the read loop CodeRabbit review 4885680006 flagged that the OpenAI Realtime writer goroutine pushed write errors to writeErrCh and returned without closing the connection, so conn.Read stays parked forever when the peer goes silent after session setup and the error is never drained. Close the socket on both the append and commit write failure paths (CloseNow, since the close handshake itself would block on a silent peer), and add a silent-server regression test. Refs Gitlawb#852 Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com>
|
Addressed CodeRabbit's actionable item from review 4885680006 (the one outstanding finding at head f34ad75): the audio writer goroutine now closes the WebSocket on write failure in both the append and commit paths, so a silent peer cannot leave conn.Read parked forever with the write error undrained in writeErrCh. CloseNow is used rather than a graceful close because the close handshake itself would block against a silent peer. Regression test TestOpenAIRealtimeWriterFailureUnblocksSilentServer covers the silent-server case: it fails without the close (5s timeout) and passes with it. Full internal/dictation suite passes, go vet and gofmt are clean. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/dictation/transcriber_openai_realtime_test.go`:
- Around line 186-212: Update the test around StreamTranscribe to create a
cancellable context, pass it to StreamTranscribe, and defer its cancellation so
teardown unblocks even if the connection remains stuck in conn.Read. Keep the
existing writer-failure assertions and timeout behavior unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fa960052-30ae-463f-aa62-86940c256b4c
📒 Files selected for processing (2)
internal/dictation/transcriber_openai_realtime.gointernal/dictation/transcriber_openai_realtime_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/dictation/transcriber_openai_realtime.go
…teardown CodeRabbit review 4909044011 identified that the silent-server regression test used context.Background(), allowing its goroutine to remain blocked in conn.Read after a timeout. Give the test a cancellable context and defer cancellation so teardown always releases the blocked read. Refs Gitlawb#852 Co-Authored-By: cairn-code <282421612+cairn-code@users.noreply.github.com>
|
Addressed CodeRabbit review 4909044011 in 2f14ac2. TestOpenAIRealtimeWriterFailureUnblocksSilentServer now creates a cancellable context, passes it to StreamTranscribe, and defers cancellation. The existing writer-failure assertions and timeout remain unchanged, while teardown can now release conn.Read if the regression fails. |
|
The reviewed teardown finding was already addressed at the current head |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving at 2f14ac2c. Checks green.
First, sorry for the wait. You answered on 2026-08-07 and my changes-requested sat on top of it until now, which is on me and not on you.
Re-verified rather than taking the summary at its word, since a stale verdict is exactly the thing I would not want held against this PR:
- The
providerio.Redactwidening is genuinely gone.provideriodoes not appear in this PR's changed files at all, so the false positive I was worried about,Redact("invalid Token gpt-4o-transcribe-2026-01-01")eating a model name, cannot happen from here. TestDeepgramSinglePassRedactionassertsstrings.Count(got, "[REDACTED]") != 1, which is the strong form. A substring check would have passed against a double wrap; a count cannot.
The rest of my list reads as addressed too, and the writer-failure work that came out of the CodeRabbit round is better than what I asked for: TestOpenAIRealtimeWriterFailureUnblocksSilentServer fails without the close and passes with it, which is the property worth pinning rather than just exercising the path. Using CloseNow because the graceful handshake would itself block against a silent peer is the right call and worth the comment it has.
Nothing further from me.
4 participants
Summary
Defence-in-depth redaction for server-echoed streaming dictation errors (Deepgram close reasons, OpenAI Realtime error events) so an API key that appears in those messages does not reach the UI. The websocket dial path itself does not surface the raw key in library error text.
Also fixes a real main-line bug: a stale completion from a cancelled dictation session could stop the mic on a newer session. Dictation sessions now carry a non-zero session ID; startup, partials, and finals ignore mismatches.
Changes
providerio.Redactwith the explicit key;%sflatten;ctx.Err()preservescontext.CanceleddictationStartedMsg), partials, and finals; stale started/partial/final messages are ignoredTest plan
go test ./internal/dictation/ ./internal/tui/ -count=1 -run "Dictation|Stale|Deepgram|OpenAIRealtime|Voice"CGO_ENABLED=1 go test -race ./internal/dictation -count=1 -run "TestOpenAIRealtimeStreamTranscribe.*(Cancel|Race|Redaction|WriteCancel|DialCancel|StartupCancel)$"(pass)Refs: session-ID gate closes stale-completion mic kill on main.
Summary by CodeRabbit
Bug Fixes
Tests