fix(riva): avoid stale interim carryover in segment assembly#3
Conversation
Do not pre-commit divergent interim hypotheses before final results. Riva can reset interim boundaries between updates; pre-committing the prior interim can prepend stale text or duplicate leading phrases in the final transcript. Tests: - go test ./apps/sotto/internal/riva - just ci-check - nix build 'path:.#sotto'
📝 WalkthroughWalkthroughStream interim handling now tracks interim stability and uses a stability-plus-punctuation and suffix/prefix similarity rule to decide when to commit a prior interim on divergence; stream state and processing were updated and tests added/renamed to validate replacement and commit behaviors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38d2798505
ℹ️ 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".
Address PR review feedback by preserving prior interim text only when divergence indicates a likely utterance boundary. - track interim stability in stream state - commit prior interim on divergence only when high-confidence or sentence-complete - keep low-confidence divergence replacement to avoid stale/duplicated leading text - add regression coverage for stable partial recovery and stale-prepend avoidance Tests: - go test ./apps/sotto/internal/riva - just ci-check - nix build 'path:.#sotto'
Only preserve divergent interim text when it looks like a completed sentence and has high stability. This avoids prepending corrected-but-stale leading words while still keeping partial-recovery support for likely utterance boundaries in no-final EOF paths. Tests: - go test ./apps/sotto/internal/riva - just ci-check - nix build 'path:.#sotto'
Relax divergent interim boundary commits to preserve stable partial speech in long dictation while treating suffix-based corrections as continuation updates. - commit divergent interim when prior stability is high or sentence punctuation is present - detect suffix-overlap corrections and avoid pre-committing stale leading words - add regression tests for long-stream partial recovery and suffix correction behavior 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/riva/transcript_segments.go (2)
72-75: Consider clarifying the minimum word count rationale.The
shorter >= 3condition prevents false positives on very short transcripts, but this intent isn't immediately obvious. A brief inline comment would help.📝 Suggested comment
+ // Require at least 3 words to use suffix similarity; shorter transcripts + // risk coincidental trailing-word matches. commonSuffix := commonSuffixWords(prevWords, currWords) if shorter >= 3 && commonSuffix*2 >= shorter { return true }🤖 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 72 - 75, Add a brief inline comment above the conditional that uses commonSuffixWords to explain the rationale for the minimum-word cutoff: clarify that the check (shorter >= 3 && commonSuffix*2 >= shorter) is intended to avoid false positives on very short transcripts by requiring at least 3 words before applying the "majority suffix" heuristic; reference the variables commonSuffix, shorter and the helper commonSuffixWords so reviewers can quickly understand why the threshold exists.
5-6: Consider documenting the threshold rationale.The
0.85threshold is a magic number that determines when interim results are stable enough to commit on divergence. A brief comment explaining the empirical basis or design rationale would help future maintainers understand when this value might need adjustment.📝 Suggested documentation
-const stableInterimBoundaryThreshold = 0.85 +// stableInterimBoundaryThreshold is the minimum stability score at which a prior +// interim hypothesis is considered reliable enough to commit when the next +// hypothesis diverges (i.e., is not a continuation). +const stableInterimBoundaryThreshold = 0.85🤖 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 5 - 6, The constant stableInterimBoundaryThreshold = 0.85 is a magic number; add a short inline comment above the stableInterimBoundaryThreshold declaration that documents its purpose and rationale (e.g., how 0.85 was chosen—empirically tuned for X dataset/latency vs stability tradeoff—or a pointer to tests/experiments that justify it), note the units/meaning (probability/confidence), and describe when to adjust it or where to find related tests/benchmarks that should be updated if it changes.
🤖 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/transcript_segments.go`:
- Around line 72-75: Add a brief inline comment above the conditional that uses
commonSuffixWords to explain the rationale for the minimum-word cutoff: clarify
that the check (shorter >= 3 && commonSuffix*2 >= shorter) is intended to avoid
false positives on very short transcripts by requiring at least 3 words before
applying the "majority suffix" heuristic; reference the variables commonSuffix,
shorter and the helper commonSuffixWords so reviewers can quickly understand why
the threshold exists.
- Around line 5-6: The constant stableInterimBoundaryThreshold = 0.85 is a magic
number; add a short inline comment above the stableInterimBoundaryThreshold
declaration that documents its purpose and rationale (e.g., how 0.85 was
chosen—empirically tuned for X dataset/latency vs stability tradeoff—or a
pointer to tests/experiments that justify it), note the units/meaning
(probability/confidence), and describe when to adjust it or where to find
related tests/benchmarks that should be updated if it changes.
Summary
Why
Riva interim hypotheses can reset boundaries across updates. Pre-committing previous interim text could prepend stale text or duplicate leading phrases in final output.
Testing
Summary by CodeRabbit
Bug Fixes
Tests