fix(riva): rebuild segment assembly for long dictation#4
Merged
Conversation
Remove interim divergence heuristics and keep session behavior strict: - commit only final ASR segments during an active toggle session - keep a single latest interim snapshot only as end-of-session fallback - no interim pre-commit or boundary inference across updates This aligns runtime behavior with toggle boundaries and avoids stale-prefix commit logic. Tests: - go test ./apps/sotto/internal/riva - just ci-check - nix build 'path:.#sotto'
Restore segment-building behavior inside a single toggle session by sealing mature interim chains on divergence while still treating rewrite/correction updates as continuation. This preserves earlier speech when Riva drops or rewrites interim hypotheses during long utterances and avoids stale-prefix prepends. Tests: - go test ./apps/sotto/internal/riva - just ci-check - nix build 'path:.#sotto'
Introduce capture/stream interfaces and dependency hooks in pipeline.Transcriber so lifecycle logic can be tested without live Pulse/Riva runtime dependencies. Also adds broad transcriber unit coverage for start/stop/cancel/send-loop success and error paths. Tests: - go test ./apps/sotto/internal/pipeline - just ci-check - nix build 'path:.#sotto'
Add focused unit tests across remaining lower-coverage internal modules: - app: status/socket helpers, toggle owner startup failure cleanup, session log fields - audio: pulse-unavailable selection/listing paths and capture close/device wrappers - config: JSONC normalization/decoder/field-validation edge cases - doctor: report + runtime check routing for paste override vs hyprctl defaults Tests: - go test ./apps/sotto/internal/... -cover - just ci-check - nix build 'path:.#sotto'
📝 WalkthroughWalkthroughAdds numerous tests across app, audio, config, doctor, and riva packages; refactors Transcriber for dependency injection; and changes Riva interim handling from stability-based commits to an age/continuation and boundary-based approach. Changes
Sequence Diagram(s)sequenceDiagram
participant T as Transcriber
participant A as Audio (select/start)
participant C as Capture
participant R as Riva (dial/stream)
T->>A: selectDevice(ctx, selector)
A-->>T: Device
T->>R: dialStream(ctx, config)
R-->>T: StreamClient
T->>A: startCapture(ctx, device)
A-->>T: Capture
loop capture loop
C-->>T: audio chunk
T->>R: SendAudio(chunk)
R-->>T: interim/final transcripts
end
T->>R: CloseAndCollect()
R-->>T: results / latency
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6b51096ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/sotto/internal/config/parser_jsonc_test.go (1)
114-123: Tighten the expected error to avoid masking regressions.
Accepting “unknown field” here could let multi-value detection regress without failing the test. Consider asserting the specific “multiple JSON values” error once behavior is stable.💡 Suggested tweak
- require.True( - t, - strings.Contains(err.Error(), "multiple JSON values") || strings.Contains(err.Error(), "unknown field"), - "unexpected error: %v", - err, - ) + require.Contains(t, err.Error(), "multiple JSON values")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/config/parser_jsonc_test.go` around lines 114 - 123, The test TestParseJSONCRejectsMultipleTopLevelValues currently allows either "multiple JSON values" or "unknown field", which can mask regressions; update the assertion to require the specific "multiple JSON values" error from parseJSONC (referencing parseJSONC and TestParseJSONCRejectsMultipleTopLevelValues) by replacing the dual-condition check with a single strict check (e.g., use require.Contains or require.ErrorContains(err, "multiple JSON values") or assert strings.Contains(err.Error(), "multiple JSON values")) so the test only passes when the parser reports the multi-value error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/sotto/internal/config/parser_jsonc_test.go`:
- Around line 114-123: The test TestParseJSONCRejectsMultipleTopLevelValues
currently allows either "multiple JSON values" or "unknown field", which can
mask regressions; update the assertion to require the specific "multiple JSON
values" error from parseJSONC (referencing parseJSONC and
TestParseJSONCRejectsMultipleTopLevelValues) by replacing the dual-condition
check with a single strict check (e.g., use require.Contains or
require.ErrorContains(err, "multiple JSON values") or assert
strings.Contains(err.Error(), "multiple JSON values")) so the test only passes
when the parser reports the multi-value error.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/sotto/internal/app/app_test.goapps/sotto/internal/audio/pulse_test.goapps/sotto/internal/config/parser_jsonc_test.goapps/sotto/internal/doctor/doctor_test.goapps/sotto/internal/pipeline/transcriber.goapps/sotto/internal/pipeline/transcriber_test.goapps/sotto/internal/riva/client.goapps/sotto/internal/riva/client_test.goapps/sotto/internal/riva/stream_receive.goapps/sotto/internal/riva/transcript_segments.go
Make multi-value JSON payloads fail with a stable "multiple JSON values" error and tighten parser regression coverage accordingly. Tests: - go test ./apps/sotto/internal/config - just ci-check - nix build 'path:.#sotto'
Address PR feedback on divergence handling by preserving single-update interim segments when they appear reliable (high stability or sentence punctuation), while retaining age-based chain commits and continuation matching. Tests: - go test ./apps/sotto/internal/riva - just ci-check - nix build 'path:.#sotto'
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/sotto/internal/config/parser_jsonc_test.go (2)
120-127: Optional: add a syntax error test to completewrapJSONDecodeErrorcoverage.
TestParseJSONCTypeErrorIncludesLocationcovers the*json.UnmarshalTypeErrorbranch. The*json.SyntaxErrorbranch is untested.🧪 Suggested additional test
+func TestParseJSONCTypeErrorIncludesLocationForSyntaxError(t *testing.T) { + _, _, err := parseJSONC("{\n \"riva\": {\"grpc\": \"val\"\n}", Default()) + // Intentionally malformed — missing closing brace to trigger SyntaxError. + _, _, err = parseJSONC("{\n \"riva\": {\n", Default()) + require.Error(t, err) + require.Contains(t, err.Error(), "line") + require.Contains(t, err.Error(), "column") +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/config/parser_jsonc_test.go` around lines 120 - 127, Add a new unit test that triggers and asserts the *json.SyntaxError branch in wrapJSONDecodeError: call parseJSONC with malformed JSONC input (e.g. a truncated/invalid token) so parsing returns an error, then require.Error and check err.Error() contains "line" and "column" (similar to TestParseJSONCTypeErrorIncludesLocation). Reference parseJSONC and wrapJSONDecodeError in the test name/description (e.g., TestParseJSONCSyntaxErrorIncludesLocation) so the SyntaxError path is exercised and covered.
84-118: Consider adding a non-object second value case toTestParseJSONCRejectsMultipleTopLevelValues.The existing test uses
{...}{...}(object + object). The core motivation for switching tojson.RawMessageis that a concrete struct target returns*json.UnmarshalTypeErrorwhen the extra value is a non-object type (number, string, array), silently bypassing the"multiple JSON values"sentinel. A companion case like{"paste":{"enable":false}} 42would directly document and protect that behavior.🧪 Suggested additional sub-case
func TestParseJSONCRejectsMultipleTopLevelValues(t *testing.T) { _, _, err := parseJSONC(`{"paste":{"enable":false}}{"paste":{"enable":true}}`, Default()) require.Error(t, err) require.Contains(t, err.Error(), "multiple JSON values") + + // Non-object second value: previously returned UnmarshalTypeError with a struct target. + _, _, err = parseJSONC(`{"paste":{"enable":false}} 42`, Default()) + require.Error(t, err) + require.Contains(t, err.Error(), "multiple JSON values") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/config/parser_jsonc_test.go` around lines 84 - 118, Add a sub-case to TestParseJSONCRejectsMultipleTopLevelValues that feeds parseJSONC a JSON string where the second top-level value is a non-object (e.g., `{"paste":{"enable":false}} 42`), then require an error and require that the error message contains "multiple JSON values"; locate the test function TestParseJSONCRejectsMultipleTopLevelValues and the parseJSONC call to add this assertion alongside the existing object+object case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/sotto/internal/config/parser_jsonc_test.go`:
- Around line 120-127: Add a new unit test that triggers and asserts the
*json.SyntaxError branch in wrapJSONDecodeError: call parseJSONC with malformed
JSONC input (e.g. a truncated/invalid token) so parsing returns an error, then
require.Error and check err.Error() contains "line" and "column" (similar to
TestParseJSONCTypeErrorIncludesLocation). Reference parseJSONC and
wrapJSONDecodeError in the test name/description (e.g.,
TestParseJSONCSyntaxErrorIncludesLocation) so the SyntaxError path is exercised
and covered.
- Around line 84-118: Add a sub-case to
TestParseJSONCRejectsMultipleTopLevelValues that feeds parseJSONC a JSON string
where the second top-level value is a non-object (e.g.,
`{"paste":{"enable":false}} 42`), then require an error and require that the
error message contains "multiple JSON values"; locate the test function
TestParseJSONCRejectsMultipleTopLevelValues and the parseJSONC call to add this
assertion alongside the existing object+object case.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/sotto/internal/riva/transcript_segments.go (1)
67-69:shorter == 0guard is unreachable dead code.At this point both
previousandcurrenthave already passed the non-empty check on lines 48–49, andcleanSegmentguarantees that a non-empty result produces at least onestrings.Fieldstoken, soshorteris always≥ 1when execution reaches line 67.🧹 Proposed cleanup
- if shorter == 0 { - return true - } - if commonPrefixWords(prevWords, currWords)*2 >= shorter {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/riva/transcript_segments.go` around lines 67 - 69, The if-block checking "shorter == 0" is unreachable because both previous and current are already non-empty (lines that call cleanSegment ensure strings.Fields produces at least one token), so remove the unreachable guard; delete the "if shorter == 0 { return true }" check referencing the shorter variable and rely on the existing comparison logic between previous and current (and functions/variables cleanSegment, previous, current, shorter) to determine equality.apps/sotto/internal/riva/client_test.go (1)
140-156: Consider adding intermediate-state assertions to the long-stream test.Unlike
TestRecordResponseCommitsInterimChainOnDivergence(which assertss.segments,s.lastInterim, ands.lastInterimAgeindividually), this test only asserts the finalcollectSegmentsoutput. A mid-streams.segmentsassertion (e.g., after "beta" triggers the first boundary commit) would pin down exactly where each segment is sealed and make regressions easier to diagnose.♻️ Suggested intermediate assertion
for _, resp := range responses { s.recordResponse(resp) } + // After "alpha one" and "beta two" chains are sealed by "gamma" + require.Equal(t, []string{"alpha one", "beta two"}, s.segments) + require.Equal(t, "gamma", s.lastInterim) + require.Equal(t, 1, s.lastInterimAge) + segments := collectSegments(s.segments, s.lastInterim) require.Equal(t, []string{"alpha one", "beta two", "gamma"}, segments)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/riva/client_test.go` around lines 140 - 156, Add an intermediate assertion in TestRecordResponseBuildsMultipleSegmentsAcrossLongInterimStream to verify stream state after the "beta" interim triggers a boundary: after calling s.recordResponse for the response with Transcript "beta", assert s.segments contains the expected sealed segment(s) (e.g., ["alpha one"]), check s.lastInterim equals the current interim ("beta") and assert s.lastInterimAge has been updated appropriately; use the existing helpers/variables (TestRecordResponseBuildsMultipleSegmentsAcrossLongInterimStream, s.recordResponse, s.segments, s.lastInterim, s.lastInterimAge, collectSegments) so the test pins the mid-stream commit point before continuing to the remaining responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/sotto/internal/riva/client_test.go`:
- Around line 140-156: Add an intermediate assertion in
TestRecordResponseBuildsMultipleSegmentsAcrossLongInterimStream to verify stream
state after the "beta" interim triggers a boundary: after calling
s.recordResponse for the response with Transcript "beta", assert s.segments
contains the expected sealed segment(s) (e.g., ["alpha one"]), check
s.lastInterim equals the current interim ("beta") and assert s.lastInterimAge
has been updated appropriately; use the existing helpers/variables
(TestRecordResponseBuildsMultipleSegmentsAcrossLongInterimStream,
s.recordResponse, s.segments, s.lastInterim, s.lastInterimAge, collectSegments)
so the test pins the mid-stream commit point before continuing to the remaining
responses.
In `@apps/sotto/internal/riva/transcript_segments.go`:
- Around line 67-69: The if-block checking "shorter == 0" is unreachable because
both previous and current are already non-empty (lines that call cleanSegment
ensure strings.Fields produces at least one token), so remove the unreachable
guard; delete the "if shorter == 0 { return true }" check referencing the
shorter variable and rely on the existing comparison logic between previous and
current (and functions/variables cleanSegment, previous, current, shorter) to
determine equality.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/sotto/internal/riva/client.goapps/sotto/internal/riva/client_test.goapps/sotto/internal/riva/stream_receive.goapps/sotto/internal/riva/transcript_segments.go
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.
Summary
Testing
Summary by CodeRabbit
Tests
Bug Fixes
Refactor