fix(dictation): redact API keys from streaming transcriber errors - #852
fix(dictation): redact API keys from streaming transcriber errors#852euxaristia wants to merge 27 commits into
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
|
Warning Review limit reached
Next review available in: 17 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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
DictationController->>Context: check cancellation
DictationController->>DictationController: validate sessionID
DictationController->>Composer: apply active transcript
Possibly related PRs
🚥 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:
|
2 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.