feat(asr): launch local-first cli runtime with tests and docs#1
Merged
feat(asr): launch local-first cli runtime with tests and docs#1
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the complete sotto CLI: CLI entrypoint and parsing, configuration parsing/validation, PulseAudio capture, Riva gRPC streaming client, session FSM and controller, IPC socket protocol, clipboard/paste outputs, Hyprland indicators, extensive tests, CI/CD, Nix packaging, codegen manifests, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant App
participant IPC
participant Session
participant Riva
participant Clipboard
User->>CLI: "sotto toggle"
CLI->>App: Execute(args)
App->>IPC: Acquire/Probe socket
alt no running session
App->>Session: Start controller (Run)
Session->>Riva: DialStream / SendAudio
Session->>Clipboard: Commit(transcript)
Session-->>IPC: Serve responses to commands
else existing session
App->>IPC: forward command (stop/cancel/status)
IPC-->>App: Response
end
Session->>App: Result
App-->>CLI: exit code
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
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: 2c7950c74f
ℹ️ 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".
rbright
force-pushed
the
feat/asr-cli-foundation
branch
from
2c7950c to
d030b2e
Compare
Owner
Author
|
Addressed review feedback in latest force-push ():\n\n- removed from repo and pinned \n- fixed IPC forward path to avoid unlinking socket on transient errors\n- fixed stale-socket acquisition logic to avoid unlinking when probe is inconclusive\n- fixed leading behavior and added regression coverage\n- fixed single-quoted config parsing in \n- replaced hard-coded app/terminal paste heuristics with configurable \n- rebalanced hooks: light checks at pre-commit, heavy checks at pre-push (and validated with trim trailing whitespace.................................................Passed |
Owner
Author
|
Follow-up update pushed to feat/asr-cli-foundation:
Commit: 0a0fcdb |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (21)
apps/sotto/internal/config/argv_test.go (1)
9-38: Consider adding a test case for backslash behavior inside single quotes.The current tests cover many edge cases but don't explicitly document whether backslash escaping works inside single quotes. Adding a test case would clarify the intended behavior:
📝 Optional: Add test case for backslash in single quotes
{name: "escaped space", input: `mycmd hello\ world`, want: []string{"mycmd", "hello world"}}, + {name: "backslash in single quote", input: `mycmd 'hello\nworld'`, want: []string{"mycmd", "hellonworld"}}, // or {"mycmd", "hello\\nworld"} if literal {name: "leading comment", input: `# wl-copy --trim-newline`, want: nil},This would serve as documentation for the expected behavior regardless of which semantic you choose.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/config/argv_test.go` around lines 9 - 38, Add a unit test to TestParseArgv covering backslash behavior inside single quotes for parseArgv: add a test case (name like "backslash in single quotes") with input where an argument value is enclosed in single quotes and contains a backslash (e.g. --name 'hello\ world') and assert the parser returns the backslash as a literal character inside the token (i.e. "hello\ world"); place this alongside the other cases in the tests slice so the expected behavior for single-quoted backslashes is documented and verified.apps/sotto/internal/config/argv.go (1)
33-45: Single-quoted strings treat backslash as escape character, diverging from shell semantics.In standard shell behavior, single quotes preserve all characters literally (including backslashes). The current implementation treats
\as an escape character even inside single quotes. If you intend to mimic shell behavior, consider:♻️ Optional: Preserve backslash literally inside single quotes
for _, r := range input { switch { case escape: current.WriteRune(r) escape = false case r == '\\': + if quote == '\'' { + current.WriteRune(r) + continue + } escape = true case quote != 0:If the current behavior is intentional for this config format, consider adding a brief comment noting the deviation from POSIX shell semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/config/argv.go` around lines 33 - 45, The parser currently treats backslash as an escape inside single-quoted strings; update the parsing logic in the loop "for _, r := range input" (the switch using variables escape, quote, current) so that backslash '\' only sets escape when not inside single quotes — e.g., only set escape when quote == 0 or quote == '"' (double-quoted context). Modify the case r == '\\' branch accordingly and add a short comment near that switch explaining the POSIX deviation if this behavior is intentional.apps/sotto/internal/hypr/query.go (1)
90-96: SimplifyrunHyprctlJSONreturn.The function stores output only to return it immediately. Consider returning directly.
♻️ Proposed simplification
func runHyprctlJSON(ctx context.Context, target string) ([]byte, error) { - output, err := runHyprctlOutput(ctx, "-j", target) - if err != nil { - return nil, err - } - return output, nil + return runHyprctlOutput(ctx, "-j", target) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/hypr/query.go` around lines 90 - 96, The function runHyprctlJSON currently assigns the result of runHyprctlOutput to a local variable only to return it; simplify by returning the call directly. Replace the body of runHyprctlJSON so it calls and returns runHyprctlOutput(ctx, "-j", target) (preserving the error return) and remove the unnecessary local variable; reference runHyprctlJSON and runHyprctlOutput to locate the change.apps/sotto/internal/output/paste.go (1)
18-18: Minor: redundant trim.
window.Addressis already trimmed byhypr.QueryActiveWindow(seequery.goline 32). The trim here is harmless but unnecessary.♻️ Proposed simplification
- payload, err := buildPasteShortcut(shortcut, strings.TrimSpace(window.Address)) + payload, err := buildPasteShortcut(shortcut, window.Address)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/output/paste.go` at line 18, The call to strings.TrimSpace on window.Address in the buildPasteShortcut invocation is redundant because hypr.QueryActiveWindow already returns a trimmed Address; remove the extra trimming in the paste.go call (i.e., change payload, err := buildPasteShortcut(shortcut, strings.TrimSpace(window.Address)) to pass window.Address directly) so buildPasteShortcut receives the already-trimmed value and avoid unnecessary string operations..github/workflows/release.yml (1)
27-37: Consider adding architecture suffix to release artifact.The release bundles
dist/sottowithout an architecture identifier. For clarity and future multi-platform support, consider naming itsotto-linux-amd64(or detecting the arch dynamically).💡 Proposed improvement
- name: Package binary run: | mkdir -p dist - cp result/bin/sotto dist/sotto - chmod +x dist/sotto + cp result/bin/sotto dist/sotto-linux-amd64 + chmod +x dist/sotto-linux-amd64 - name: Create release uses: softprops/action-gh-release@v2 with: - files: dist/sotto + files: dist/sotto-linux-amd64 generate_release_notes: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 27 - 37, The release artifact "dist/sotto" created in the "Package binary" step lacks an architecture suffix; update the packaging step to copy/rename the binary into dist with an arch-aware name (e.g., dist/sotto-linux-amd64 or dist/sotto-$(uname -m)) and update the "Create release" step (softprops/action-gh-release@v2) to upload that renamed file instead of dist/sotto so artifacts clearly indicate platform/architecture.apps/sotto/internal/output/paste_test.go (1)
17-33: Consider addingt.Parallel()to subtests.The parent test calls
t.Parallel(), but the subtests don't. For consistent parallel execution, subtests should also be marked parallel.♻️ Proposed improvement
t.Run("builds payload", func(t *testing.T) { + t.Parallel() got, err := buildPasteShortcut("SUPER,V", "0xabc") require.NoError(t, err) require.Equal(t, "SUPER,V,address:0xabc", got) }) t.Run("rejects empty shortcut", func(t *testing.T) { + t.Parallel() _, err := buildPasteShortcut("", "0xabc") require.Error(t, err) require.Contains(t, err.Error(), "shortcut") }) t.Run("rejects empty address", func(t *testing.T) { + t.Parallel() _, err := buildPasteShortcut("CTRL,V", "") require.Error(t, err) require.Contains(t, err.Error(), "address") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/output/paste_test.go` around lines 17 - 33, Add t.Parallel() to each subtest body so they run concurrently with the parent test; in the t.Run closures that call buildPasteShortcut (the three subtests "builds payload", "rejects empty shortcut", and "rejects empty address"), insert t.Parallel() as the first statement inside each func(t *testing.T) to enable proper parallel execution alongside the parent.apps/sotto/internal/audio/pulse.go (1)
302-345: Consider memory implications of unbounded rawPCM accumulation.The
onPCMmethod appends all received audio data toc.rawPCM(line 324). For long recording sessions, this could consume significant memory (16kHz mono 16-bit = ~1.9 MB/minute). IfRawPCM()is only needed for debugging, consider making it conditional onDebugConfig.EnableAudioDump.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/audio/pulse.go` around lines 302 - 345, The onPCM method unconditionally appends incoming audio to Capture.rawPCM causing unbounded memory growth; guard or disable that behavior when not needed by checking a debug flag (e.g., DebugConfig.EnableAudioDump) before appending to rawPCM inside Capture.onPCM (and ensure RawPCM() reads respect the same flag), or only accumulate a bounded window (capping length) if continuous capture is required; update the logic around rawPCM append and any related access in Capture/RawPCM to use that guard or cap to prevent excessive memory use.apps/sotto/internal/indicator/sound.go (1)
52-65: Silent fallback on file playback error may hide configuration issues.When
playCueFilefails,emitCuesilently falls back to synthesized cues without logging. If a user configures a custom sound file path that's invalid, they'll get the fallback sound with no indication of the misconfiguration.Consider logging a warning when the configured file path exists in config but playback fails, so users can diagnose configuration issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/indicator/sound.go` around lines 52 - 65, emitCue currently swallows errors from playCueFile and falls back silently; update emitCue so that when cuePath(kind, cfg) returns a non-empty path and playCueFile(path) returns an error, you log a warning that includes the configured path, the error details, and that you're falling back to synthesized cues (then continue to call cueSamples/playSynthCue as before). Locate the logic in emitCue and add a single warning-level log call in the branch where playCueFile fails (referencing cuePath, playCueFile, and playSynthCue) using the project’s logger so users can diagnose invalid configured file paths.apps/sotto/internal/audio/pulse_test.go (1)
144-158: Consider adding a comment explaining the reflection usage.The
setSourcePortshelper uses reflection to populate internal proto fields. A brief comment explaining why reflection is necessary (e.g., proto fields not exported or difficult to construct directly) would help future maintainers understand this approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/audio/pulse_test.go` around lines 144 - 158, Add a short comment above setSourcePorts explaining why reflection is used: note that the pulseproto.GetSourceInfoReply.Ports slice or its element fields ("Ports", "Name", "Available") are unexported/internal (or generated by proto and not straightforward to construct directly), so the helper uses reflect to allocate and set those fields; mention the intent (test-only helper to populate proto internals) and caution that changing generated proto names will break this helper.apps/sotto/internal/config/types.go (1)
63-67: Type mismatch warrants clarification:VocabSet.Boost(float64) vsSpeechPhrase.Boost(float32).The inconsistency is intentional:
VocabSet.Boostusesfloat64for configuration parsing precision, whileSpeechPhrase.Boostusesfloat32to match the Riva API's expectations. The explicit conversion occurs at a single boundary (config/validate.go line 97):float32(c.boost). For typical boost values (small integers like 10, 20), precision loss is negligible. However, this design difference should be documented in code comments to clarify why the types differ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/config/types.go` around lines 63 - 67, Add a brief code comment explaining the intentional type mismatch between VocabSet.Boost (float64) and SpeechPhrase.Boost (float32): VocabSet uses float64 for configuration/parsing precision while SpeechPhrase uses float32 to match the Riva API, and the explicit conversion is performed at the config-to-domain boundary (the float32(c.boost) conversion). Place the comment near the VocabSet type definition and/or next to the conversion site to make the design decision discoverable for future maintainers.apps/sotto/internal/audio/pulse_integration_test.go (1)
13-19: Context parameter inListDevicesis unused and won't protect against hangs.The test creates a 3-second timeout context, but
ListDevicesdiscards it entirely (line 35 inpulse.godeclares the parameter as_ context.Context). The implementation never checks or respects context cancellation, so if PulseAudio hangs, the timeout won't cancel the operation. The same issue affectsSelectDevice(line 73), which passes the context toListDevicesbut it's discarded.Consider updating
ListDevicesto accept and respect context cancellation, or remove the parameter if it cannot be used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/audio/pulse_integration_test.go` around lines 13 - 19, ListDevices currently discards its context parameter (declared as _ context.Context) so cancellations don't work; update ListDevices in pulse.go to accept and use the provided ctx (remove the underscore), and make the implementation context-aware (e.g., use exec.CommandContext or otherwise check ctx.Done and return early on cancellation) so the 3s timeout in TestListDevicesIntegration actually cancels a hanging call; also propagate the real ctx through SelectDevice (which currently passes a context but ends up calling ListDevices that ignores it) so both ListDevices and SelectDevice consistently respect context cancellation.docs/configuration.md (1)
44-44: Clarify "required" wording forclipboard_cmd.The documentation states
clipboard_cmdis a "required command," but a default value (wl-copy --trim-newline) is provided byconfig.Default(). Consider rephrasing to "required for output; defaults towl-copy --trim-newline" to avoid implying the user must explicitly set it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/configuration.md` at line 44, The table entry for `clipboard_cmd` currently says "required command" which conflicts with the presence of a default in config.Default(); update the description to clarify that the command is required for output but has a default. Edit the `clipboard_cmd` row so the description reads something like "required for output; defaults to `wl-copy --trim-newline`" (or equivalent wording) to reference the default provided by config.Default().apps/sotto/internal/ipc/socket_test.go (1)
24-36: Consider using testify assertions for consistency.Lines 29-36 use manual
if/t.Fatalfpatterns while other tests in this file userequire.*assertions. For consistency:- if err != nil { - t.Fatalf("Acquire() error = %v", err) - } + require.NoError(t, err) defer listener.Close() - if rescueCalls == 0 { - t.Fatalf("expected stale-socket rescue to run") - } + require.NotZero(t, rescueCalls, "expected stale-socket rescue to run")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/ipc/socket_test.go` around lines 24 - 36, Replace the manual if/t.Fatalf checks in the Acquire test with testify/require assertions for consistency: assert that err is nil using require.NoError(t, err) after calling Acquire, and assert that rescueCalls is greater than 0 using require.Greater(t, rescueCalls, 0) (or require.NotZero) instead of the if block; keep the defer listener.Close() and add the testify/require import if it’s not already present.apps/sotto/internal/pipeline/transcriber_test.go (1)
120-126: Consider test isolation forPULSE_SERVERmanipulation.Setting
PULSE_SERVERglobally viat.Setenvcould interfere with other parallel tests that depend on PulseAudio. Whilet.Setenvis scoped to the test, if this test runs in parallel with others that spawn processes using PulseAudio, the env var propagates to child processes.If parallel execution is intended later, consider wrapping this in a
t.Runsubtest or ensuring it doesn't conflict:func TestStartFailsWhenAudioSelectionUnavailable(t *testing.T) { if testing.Short() { t.Skip("skipping in short mode") } // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/pipeline/transcriber_test.go` around lines 120 - 126, The test TestStartFailsWhenAudioSelectionUnavailable mutates PULSE_SERVER via t.Setenv which can leak to concurrent tests; wrap the logic in a dedicated subtest (use t.Run("isolated", func(t *testing.T) { t.Setenv(...); transcriber := NewTranscriber(...); err := transcriber.Start(...); require.Error(t, err) })) so the env change is scoped to the subtest, and optionally add a guard to skip in short mode (if testing.Short()) to avoid running this integration-like case in fast test runs.apps/sotto/internal/ipc/socket.go (1)
74-79: Consider usingsyscall.EADDRINUSEfor more robust detection.String-based error matching is fragile and may break with localized error messages or future Go changes. Using
errors.Iswithsyscall.EADDRINUSEis more reliable.♻️ Suggested refactor
+import "syscall" + func isAddrInUse(err error) bool { - if err == nil { - return false - } - return strings.Contains(err.Error(), "address already in use") + var errno syscall.Errno + if errors.As(err, &errno) { + return errno == syscall.EADDRINUSE + } + return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/ipc/socket.go` around lines 74 - 79, The isAddrInUse function currently matches the error string; replace this fragile string check by using errors.Is to compare the error to syscall.EADDRINUSE (importing "errors" and "syscall") so the function returns true when errors.Is(err, syscall.EADDRINUSE) is true, while preserving the nil check; update the isAddrInUse function to use errors.Is(err, syscall.EADDRINUSE) for robust detection.apps/sotto/internal/pipeline/transcriber.go (1)
83-88: Simplify debug sink assignment.The IIFE is unnecessary; a simple conditional assignment is clearer.
♻️ Proposed simplification
- DebugResponseSinkJSON: func() *os.File { - if t.debugGRPCFile == nil { - return nil - } - return t.debugGRPCFile - }(), + DebugResponseSinkJSON: t.debugGRPCFile,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/pipeline/transcriber.go` around lines 83 - 88, The inline anonymous function used to set DebugResponseSinkJSON is unnecessary; replace the IIFE with a direct assignment of the file variable (use DebugResponseSinkJSON: t.debugGRPCFile) so the field gets the *os.File (or nil) directly, removing the anonymous func wrapper and its immediate invocation.apps/sotto/internal/config/validate.go (1)
100-105: Dead code in sort comparator.The Boost comparison at line 102 is unreachable because
selectedis a map keyed by phrase, ensuring each phrase appears exactly once inphrases. The conditionphrases[i].Phrase == phrases[j].Phrasewill never be true.♻️ Simplified sort
sort.Slice(phrases, func(i, j int) bool { - if phrases[i].Phrase == phrases[j].Phrase { - return phrases[i].Boost < phrases[j].Boost - } return phrases[i].Phrase < phrases[j].Phrase })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/config/validate.go` around lines 100 - 105, The comparator passed to sort.Slice contains a dead branch comparing Boost when phrases[i].Phrase == phrases[j].Phrase (which never happens because the input slice `phrases` is derived from the `selected` map keyed by Phrase), so simplify the comparator in the sort.Slice call to only compare phrases by `Phrase` (e.g., return phrases[i].Phrase < phrases[j].Phrase) and remove the unreachable Boost comparison; update any comments to reflect that sorting is by Phrase only.apps/sotto/internal/doctor/doctor.go (2)
104-114: Consider adding timeout to audio selection check.
audio.SelectDeviceis called withcontext.Background(), which could hang indefinitely if PulseAudio is unresponsive. Consider using a context with timeout for the doctor check.♻️ Proposed fix
func checkAudioSelection(cfg config.Config) Check { - selection, err := audio.SelectDevice(context.Background(), cfg.Audio.Input, cfg.Audio.Fallback) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + selection, err := audio.SelectDevice(ctx, cfg.Audio.Input, cfg.Audio.Fallback) if err != nil { return Check{Name: "audio.device", Pass: false, Message: err.Error()} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/doctor/doctor.go` around lines 104 - 114, checkAudioSelection uses context.Background() for audio.SelectDevice which can hang; wrap the call in a context.WithTimeout (e.g., 5s or a configurable value) and defer cancel so the check fails fast on unresponsive PulseAudio. Update the call to use the new ctx (ctx, cancel := context.WithTimeout(context.Background(), timeout); defer cancel) and ensure the returned error is propagated the same way from checkAudioSelection; reference the checkAudioSelection function and the audio.SelectDevice call when making this change.
138-143: Clarify readiness logic for edge case.The condition at line 139 returns
Pass: truewith a generic "HTTP 200" message when the body is non-empty but doesn't contain "ready". This is valid but may confuse operators expecting explicit readiness confirmation.Consider adding a comment explaining the intent: any 2xx is considered passing, but the message distinguishes whether the body explicitly confirms readiness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/doctor/doctor.go` around lines 138 - 143, The current readiness branch in doctor.go (variables bodyText, resp.StatusCode, url and returning Check{Name: "riva.ready", Pass: true, ...}) treats any non-empty non-"ready" body as a passing 2xx response but can confuse operators; update this branch to (1) add a short comment above the if explaining the intent: any 2xx is considered a pass even if the body does not explicitly contain "ready", and (2) make the Check.Message more explicit (e.g., include that the body did not contain "ready" such as "HTTP %d from %s (no 'ready' in body)") so operators can distinguish explicit readiness vs. implicit 2xx success while keeping Pass: true.apps/sotto/internal/cli/cli.go (1)
39-77: Consider supporting--config=PATHsyntax.The current implementation only supports
--config PATH(space-separated). Many CLI tools also accept--config=PATHsyntax. This is a minor usability consideration.♻️ Optional: Add support for --config=PATH syntax
for i := 0; i < len(args); i++ { arg := args[i] switch arg { case "-h", "--help": parsed.ShowHelp = true parsed.Command = CommandHelp case "--version": parsed.ShowHelp = false parsed.Command = CommandVersion - case "--config": - i++ - if i >= len(args) { - return Parsed{}, errors.New("--config requires a path") - } - parsed.ConfigPath = args[i] default: + if arg == "--config" { + i++ + if i >= len(args) { + return Parsed{}, errors.New("--config requires a path") + } + parsed.ConfigPath = args[i] + continue + } + if strings.HasPrefix(arg, "--config=") { + parsed.ConfigPath = strings.TrimPrefix(arg, "--config=") + if parsed.ConfigPath == "" { + return Parsed{}, errors.New("--config requires a path") + } + continue + } if strings.HasPrefix(arg, "-") { return Parsed{}, fmt.Errorf("unknown flag: %s", arg) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/cli/cli.go` around lines 39 - 77, The CLI Parse function only handles "--config PATH" and rejects the "--config=PATH" form; update Parse to also accept and parse "--config=PATH" by checking for a prefix like "--config=" (or splitting on '=') inside the args loop before incrementing i, extract the value, validate it is non-empty, set parsed.ConfigPath accordingly, and keep the existing error path when no value is provided; make this change in the block that currently matches case "--config" so both "--config" (space-separated) and "--config=PATH" are supported without breaking other flags or command validation.apps/sotto/internal/session/session.go (1)
114-248: Consider extracting result population into a helper.The
Runmethod is comprehensive but has repetitive result field assignments across multiple exit paths (e.g., lines 118-121, 128-133, 148-152, etc.). A helper likepopulateResult(result *Result, stopResult, err)could reduce boilerplate and ensure consistency.This is optional given the method is readable and each path has clear intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/session/session.go` around lines 114 - 248, The Run method repeats setting many Result fields across exit paths; extract a helper (e.g., populateResult) to centralize assignments and reduce boilerplate: add a function populateResult(res *Result, state fsm.State, finishedAt time.Time, opts ...func(*Result)) that sets common fields (State, FinishedAt, FocusedMonitor via c.indicator.FocusedMonitor()) and apply optional mutators to set Err, Cancelled, Transcript, AudioDevice, BytesCaptured, GRPCLatency, StartedAt, etc.; replace the repeated blocks in Controller.Run (including branches handling ctx.Done, actionCancel, actionStop error/success paths, and default) to call populateResult with appropriate mutators and keep existing calls to c.toErrorAndReset, c.indicator cues, and transitions intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/sotto/go.mod`:
- Around line 1-20: go.mod and go.work specify Go 1.25.5 but flake.nix doesn't
pin a Go version; update flake.nix to explicitly pin and use Go 1.25.5 (the same
patch version) in its inputs/buildInputs (or nixpkgs overlay) so the Nix
development/CI environment matches go.mod/go.work; ensure the symbol you change
references the Go package entry in flake.nix (the inputs or buildInputs list)
and keep the pinned version 1.25.5.
In `@apps/sotto/internal/doctor/doctor_test.go`:
- Around line 102-116: The test TestCheckRivaReadyPassesOnHTTP200NonReadyBody
sets cfg.RivaHTTP to server.URL including the "http://" prefix which can cause
duplicate schemes when checkRivaReady prepends one; update the test to strip the
scheme before assigning RivaHTTP (e.g., use strings.TrimPrefix on server.URL for
"http://" and "https://") so cfg.RivaHTTP contains only the host/host:port
expected by checkRivaReady.
- Around line 87-100: The failure test TestCheckRivaReadyFailureStatusCode uses
cfg.RivaHTTP = server.URL while the success test trims the "http://" prefix;
make them consistent by setting cfg.RivaHTTP the same way in both tests (either
always use server.URL or always use strings.TrimPrefix(server.URL, "http://")).
Update TestCheckRivaReadyFailureStatusCode to match TestCheckRivaReadySuccess’s
assignment style so both tests exercise checkRivaReady with the same input
format for RivaHTTP.
In `@apps/sotto/internal/ipc/socket.go`:
- Around line 50-52: The probe error is being incorrectly wrapped with
ErrAlreadyRunning; update the error return in the Probe failure branch so it
does not classify all probe failures as "already running". Replace the current
return that wraps probeErr with ErrAlreadyRunning and instead return the probe
error with context (e.g., include the socket path and wrap probeErr), leaving
ErrAlreadyRunning reserved for the specific success/indicator path that proves a
session is already running; refer to Probe, probeErr, ErrAlreadyRunning and path
to locate and fix the code.
In `@apps/sotto/internal/pipeline/transcriber.go`:
- Around line 111-139: The sendLoop early-return path can leave sendErrCh
unreadable by StopAndTranscribe; update sendLoop to guarantee a single send to
sendErrCh (buffered size 1) on all exits—e.g., add an unconditional deferred
send (defer func() { if sendErrCh != nil { sendErrCh <- nil } }()) at the top of
sendLoop so that StopAndTranscribe's receive will never block, and keep the
existing error sends to override the nil when an actual error occurs; reference
sendLoop, Start, and sendErrCh when making this change.
In `@docs/verification.md`:
- Around line 41-50: The checklist repeats the word “Verify” across items 3–8
which hurts readability; update the phrasing to vary verbs and tighten sentences
for flow—e.g., use "Confirm" for transcript commit, "Ensure" for clipboard
retention and paste behavior, "Check" for cancel path behavior with sotto
cancel, and "Validate" for Riva-down and stale socket recovery; keep references
to the commands shown (sotto doctor, sotto toggle, sotto cancel) and make each
line a short imperative sentence without duplicating "Verify".
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 27-37: The release artifact "dist/sotto" created in the "Package
binary" step lacks an architecture suffix; update the packaging step to
copy/rename the binary into dist with an arch-aware name (e.g.,
dist/sotto-linux-amd64 or dist/sotto-$(uname -m)) and update the "Create
release" step (softprops/action-gh-release@v2) to upload that renamed file
instead of dist/sotto so artifacts clearly indicate platform/architecture.
In `@apps/sotto/internal/audio/pulse_integration_test.go`:
- Around line 13-19: ListDevices currently discards its context parameter
(declared as _ context.Context) so cancellations don't work; update ListDevices
in pulse.go to accept and use the provided ctx (remove the underscore), and make
the implementation context-aware (e.g., use exec.CommandContext or otherwise
check ctx.Done and return early on cancellation) so the 3s timeout in
TestListDevicesIntegration actually cancels a hanging call; also propagate the
real ctx through SelectDevice (which currently passes a context but ends up
calling ListDevices that ignores it) so both ListDevices and SelectDevice
consistently respect context cancellation.
In `@apps/sotto/internal/audio/pulse_test.go`:
- Around line 144-158: Add a short comment above setSourcePorts explaining why
reflection is used: note that the pulseproto.GetSourceInfoReply.Ports slice or
its element fields ("Ports", "Name", "Available") are unexported/internal (or
generated by proto and not straightforward to construct directly), so the helper
uses reflect to allocate and set those fields; mention the intent (test-only
helper to populate proto internals) and caution that changing generated proto
names will break this helper.
In `@apps/sotto/internal/audio/pulse.go`:
- Around line 302-345: The onPCM method unconditionally appends incoming audio
to Capture.rawPCM causing unbounded memory growth; guard or disable that
behavior when not needed by checking a debug flag (e.g.,
DebugConfig.EnableAudioDump) before appending to rawPCM inside Capture.onPCM
(and ensure RawPCM() reads respect the same flag), or only accumulate a bounded
window (capping length) if continuous capture is required; update the logic
around rawPCM append and any related access in Capture/RawPCM to use that guard
or cap to prevent excessive memory use.
In `@apps/sotto/internal/cli/cli.go`:
- Around line 39-77: The CLI Parse function only handles "--config PATH" and
rejects the "--config=PATH" form; update Parse to also accept and parse
"--config=PATH" by checking for a prefix like "--config=" (or splitting on '=')
inside the args loop before incrementing i, extract the value, validate it is
non-empty, set parsed.ConfigPath accordingly, and keep the existing error path
when no value is provided; make this change in the block that currently matches
case "--config" so both "--config" (space-separated) and "--config=PATH" are
supported without breaking other flags or command validation.
In `@apps/sotto/internal/config/argv_test.go`:
- Around line 9-38: Add a unit test to TestParseArgv covering backslash behavior
inside single quotes for parseArgv: add a test case (name like "backslash in
single quotes") with input where an argument value is enclosed in single quotes
and contains a backslash (e.g. --name 'hello\ world') and assert the parser
returns the backslash as a literal character inside the token (i.e. "hello\
world"); place this alongside the other cases in the tests slice so the expected
behavior for single-quoted backslashes is documented and verified.
In `@apps/sotto/internal/config/argv.go`:
- Around line 33-45: The parser currently treats backslash as an escape inside
single-quoted strings; update the parsing logic in the loop "for _, r := range
input" (the switch using variables escape, quote, current) so that backslash '\'
only sets escape when not inside single quotes — e.g., only set escape when
quote == 0 or quote == '"' (double-quoted context). Modify the case r == '\\'
branch accordingly and add a short comment near that switch explaining the POSIX
deviation if this behavior is intentional.
In `@apps/sotto/internal/config/types.go`:
- Around line 63-67: Add a brief code comment explaining the intentional type
mismatch between VocabSet.Boost (float64) and SpeechPhrase.Boost (float32):
VocabSet uses float64 for configuration/parsing precision while SpeechPhrase
uses float32 to match the Riva API, and the explicit conversion is performed at
the config-to-domain boundary (the float32(c.boost) conversion). Place the
comment near the VocabSet type definition and/or next to the conversion site to
make the design decision discoverable for future maintainers.
In `@apps/sotto/internal/config/validate.go`:
- Around line 100-105: The comparator passed to sort.Slice contains a dead
branch comparing Boost when phrases[i].Phrase == phrases[j].Phrase (which never
happens because the input slice `phrases` is derived from the `selected` map
keyed by Phrase), so simplify the comparator in the sort.Slice call to only
compare phrases by `Phrase` (e.g., return phrases[i].Phrase < phrases[j].Phrase)
and remove the unreachable Boost comparison; update any comments to reflect that
sorting is by Phrase only.
In `@apps/sotto/internal/doctor/doctor.go`:
- Around line 104-114: checkAudioSelection uses context.Background() for
audio.SelectDevice which can hang; wrap the call in a context.WithTimeout (e.g.,
5s or a configurable value) and defer cancel so the check fails fast on
unresponsive PulseAudio. Update the call to use the new ctx (ctx, cancel :=
context.WithTimeout(context.Background(), timeout); defer cancel) and ensure the
returned error is propagated the same way from checkAudioSelection; reference
the checkAudioSelection function and the audio.SelectDevice call when making
this change.
- Around line 138-143: The current readiness branch in doctor.go (variables
bodyText, resp.StatusCode, url and returning Check{Name: "riva.ready", Pass:
true, ...}) treats any non-empty non-"ready" body as a passing 2xx response but
can confuse operators; update this branch to (1) add a short comment above the
if explaining the intent: any 2xx is considered a pass even if the body does not
explicitly contain "ready", and (2) make the Check.Message more explicit (e.g.,
include that the body did not contain "ready" such as "HTTP %d from %s (no
'ready' in body)") so operators can distinguish explicit readiness vs. implicit
2xx success while keeping Pass: true.
In `@apps/sotto/internal/hypr/query.go`:
- Around line 90-96: The function runHyprctlJSON currently assigns the result of
runHyprctlOutput to a local variable only to return it; simplify by returning
the call directly. Replace the body of runHyprctlJSON so it calls and returns
runHyprctlOutput(ctx, "-j", target) (preserving the error return) and remove the
unnecessary local variable; reference runHyprctlJSON and runHyprctlOutput to
locate the change.
In `@apps/sotto/internal/indicator/sound.go`:
- Around line 52-65: emitCue currently swallows errors from playCueFile and
falls back silently; update emitCue so that when cuePath(kind, cfg) returns a
non-empty path and playCueFile(path) returns an error, you log a warning that
includes the configured path, the error details, and that you're falling back to
synthesized cues (then continue to call cueSamples/playSynthCue as before).
Locate the logic in emitCue and add a single warning-level log call in the
branch where playCueFile fails (referencing cuePath, playCueFile, and
playSynthCue) using the project’s logger so users can diagnose invalid
configured file paths.
In `@apps/sotto/internal/ipc/socket_test.go`:
- Around line 24-36: Replace the manual if/t.Fatalf checks in the Acquire test
with testify/require assertions for consistency: assert that err is nil using
require.NoError(t, err) after calling Acquire, and assert that rescueCalls is
greater than 0 using require.Greater(t, rescueCalls, 0) (or require.NotZero)
instead of the if block; keep the defer listener.Close() and add the
testify/require import if it’s not already present.
In `@apps/sotto/internal/ipc/socket.go`:
- Around line 74-79: The isAddrInUse function currently matches the error
string; replace this fragile string check by using errors.Is to compare the
error to syscall.EADDRINUSE (importing "errors" and "syscall") so the function
returns true when errors.Is(err, syscall.EADDRINUSE) is true, while preserving
the nil check; update the isAddrInUse function to use errors.Is(err,
syscall.EADDRINUSE) for robust detection.
In `@apps/sotto/internal/output/paste_test.go`:
- Around line 17-33: Add t.Parallel() to each subtest body so they run
concurrently with the parent test; in the t.Run closures that call
buildPasteShortcut (the three subtests "builds payload", "rejects empty
shortcut", and "rejects empty address"), insert t.Parallel() as the first
statement inside each func(t *testing.T) to enable proper parallel execution
alongside the parent.
In `@apps/sotto/internal/output/paste.go`:
- Line 18: The call to strings.TrimSpace on window.Address in the
buildPasteShortcut invocation is redundant because hypr.QueryActiveWindow
already returns a trimmed Address; remove the extra trimming in the paste.go
call (i.e., change payload, err := buildPasteShortcut(shortcut,
strings.TrimSpace(window.Address)) to pass window.Address directly) so
buildPasteShortcut receives the already-trimmed value and avoid unnecessary
string operations.
In `@apps/sotto/internal/pipeline/transcriber_test.go`:
- Around line 120-126: The test TestStartFailsWhenAudioSelectionUnavailable
mutates PULSE_SERVER via t.Setenv which can leak to concurrent tests; wrap the
logic in a dedicated subtest (use t.Run("isolated", func(t *testing.T) {
t.Setenv(...); transcriber := NewTranscriber(...); err :=
transcriber.Start(...); require.Error(t, err) })) so the env change is scoped to
the subtest, and optionally add a guard to skip in short mode (if
testing.Short()) to avoid running this integration-like case in fast test runs.
In `@apps/sotto/internal/pipeline/transcriber.go`:
- Around line 83-88: The inline anonymous function used to set
DebugResponseSinkJSON is unnecessary; replace the IIFE with a direct assignment
of the file variable (use DebugResponseSinkJSON: t.debugGRPCFile) so the field
gets the *os.File (or nil) directly, removing the anonymous func wrapper and its
immediate invocation.
In `@apps/sotto/internal/session/session.go`:
- Around line 114-248: The Run method repeats setting many Result fields across
exit paths; extract a helper (e.g., populateResult) to centralize assignments
and reduce boilerplate: add a function populateResult(res *Result, state
fsm.State, finishedAt time.Time, opts ...func(*Result)) that sets common fields
(State, FinishedAt, FocusedMonitor via c.indicator.FocusedMonitor()) and apply
optional mutators to set Err, Cancelled, Transcript, AudioDevice, BytesCaptured,
GRPCLatency, StartedAt, etc.; replace the repeated blocks in Controller.Run
(including branches handling ctx.Done, actionCancel, actionStop error/success
paths, and default) to call populateResult with appropriate mutators and keep
existing calls to c.toErrorAndReset, c.indicator cues, and transitions intact.
In `@docs/configuration.md`:
- Line 44: The table entry for `clipboard_cmd` currently says "required command"
which conflicts with the presence of a default in config.Default(); update the
description to clarify that the command is required for output but has a
default. Edit the `clipboard_cmd` row so the description reads something like
"required for output; defaults to `wl-copy --trim-newline`" (or equivalent
wording) to reference the default provided by config.Default().
Owner
Author
|
Addressed the latest CI/comment set:
Note on Go pinning thread: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/sotto/internal/ipc/socket.go (2)
58-60: Consider logging or propagating rescue callback errors.The rescue callback's error is silently ignored. If rescue is intended as a best-effort recovery mechanism, this is acceptable—but a log statement would help surface failures during diagnostics. If rescue failures should abort acquisition, the error should be returned.
📝 Optional: Log rescue errors for diagnostics
if rescue != nil { - _ = rescue(ctx) + if rescueErr := rescue(ctx); rescueErr != nil { + // Log or handle rescue error if observability is desired + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/ipc/socket.go` around lines 58 - 60, The rescue callback error is being ignored (rescue(ctx) result discarded); either log it for diagnostics or propagate it up: if rescue is best-effort, call rescue(ctx), check the returned error and emit a diagnostic log (use the package/logger instance in this module or log.Printf) including context and the error; if rescue failures should abort acquisition, change the surrounding function to return error and propagate the rescue(ctx) error instead of discarding it. Ensure you update callers if you alter the function signature.
74-79: String matching forEADDRINUSEis portable but fragile.Using
strings.Contains(err.Error(), "address already in use")is a common portable approach, but it relies on stable error message text. Consider usingerrors.Iswithsyscall.EADDRINUSEfor more robust detection on Unix platforms (matching the pattern already established inclient.go).📝 Optional: More robust EADDRINUSE detection
+import "syscall" + func isAddrInUse(err error) bool { if err == nil { return false } + if errors.Is(err, syscall.EADDRINUSE) { + return true + } return strings.Contains(err.Error(), "address already in use") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/ipc/socket.go` around lines 74 - 79, Replace the fragile string-match in isAddrInUse by using errors.Is(err, syscall.EADDRINUSE) to detect EADDRINUSE (as done in client.go), and keep the current strings.Contains fallback only for platforms/err types where syscall matching may not apply; update isAddrInUse to first check errors.Is(err, syscall.EADDRINUSE) and then fallback to the existing "address already in use" substring check so detection is robust across environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/sotto/internal/config/parser.go`:
- Around line 28-34: The code assigns to cfg.Vocab.Sets when closing an in-vocab
set (inside the state.inVocabSet != nil branch) which will panic if
cfg.Vocab.Sets is nil; modify the block that handles trimmed == "}" to
defensively ensure cfg.Vocab.Sets is initialized (e.g., if cfg.Vocab.Sets == nil
{ init map }), then perform cfg.Vocab.Sets[state.inVocabSet.Name] =
*state.inVocabSet and clear state.inVocabSet/vocabSetStartLine; note that
Default() normally initializes this map but add the nil-check here to make
parsing robust against uninitialized Config inputs.
---
Nitpick comments:
In `@apps/sotto/internal/ipc/socket.go`:
- Around line 58-60: The rescue callback error is being ignored (rescue(ctx)
result discarded); either log it for diagnostics or propagate it up: if rescue
is best-effort, call rescue(ctx), check the returned error and emit a diagnostic
log (use the package/logger instance in this module or log.Printf) including
context and the error; if rescue failures should abort acquisition, change the
surrounding function to return error and propagate the rescue(ctx) error instead
of discarding it. Ensure you update callers if you alter the function signature.
- Around line 74-79: Replace the fragile string-match in isAddrInUse by using
errors.Is(err, syscall.EADDRINUSE) to detect EADDRINUSE (as done in client.go),
and keep the current strings.Contains fallback only for platforms/err types
where syscall matching may not apply; update isAddrInUse to first check
errors.Is(err, syscall.EADDRINUSE) and then fallback to the existing "address
already in use" substring check so detection is robust across environments.
Owner
Author
|
Pushed follow-up updates for indicator visibility + latest review thread:
Also resolved the remaining review thread after this push. If you want top-center behavior with mako, set: and add to mako config: |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/sotto/internal/config/types.go (1)
65-69: Minor type inconsistency betweenVocabSet.BoostandSpeechPhrase.Boost.
VocabSet.Boostisfloat64whileSpeechPhrase.Boostisfloat32. This requires a narrowing conversion inBuildSpeechPhrases(validate.go line 107). For typical boost values this is fine, but consider aligning the types for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/config/types.go` around lines 65 - 69, VocabSet.Boost is defined as float64 while SpeechPhrase.Boost is float32 causing a narrowing conversion in BuildSpeechPhrases; fix by choosing one consistent type and updating the definitions and usages accordingly — either change VocabSet.Boost to float32 or SpeechPhrase.Boost to float64, then update BuildSpeechPhrases to remove the cast and run tests to ensure no other callsites rely on the previous type.apps/sotto/internal/indicator/indicator.go (1)
174-185: Consider non-blocking cue playback to avoid stale cues.The current implementation spawns a goroutine that blocks on
soundMu. If cues are triggered rapidly (e.g., quick start-then-cancel), goroutines could queue up, causing stale cues to play after the session state has already changed.A non-blocking approach (e.g.,
TryLockor a single-slot channel) could skip stale cues. However, for typical usage patterns where cues are infrequent, the current behavior is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/indicator/indicator.go` around lines 174 - 185, playCue currently spawns a goroutine that blocks on h.soundMu, allowing queued goroutines to play stale cues; change to a non-blocking single-slot queue: add a buffered channel field on HyprNotify (e.g., cueCh chan cueKind with capacity 1), have playCue return early if !h.cfg.SoundEnable, then non-blockingly attempt to send the cue into cueCh (use select with default to drop if full) instead of spinning a goroutine and locking; run a single dedicated goroutine (started once when HyprNotify is created) that reads from cueCh and calls emitCue(kind, h.cfg) (and use h.soundMu only inside that single worker if needed) so rapid cues are dropped rather than queued and stale audio is avoided.
🤖 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/types.go`:
- Around line 65-69: VocabSet.Boost is defined as float64 while
SpeechPhrase.Boost is float32 causing a narrowing conversion in
BuildSpeechPhrases; fix by choosing one consistent type and updating the
definitions and usages accordingly — either change VocabSet.Boost to float32 or
SpeechPhrase.Boost to float64, then update BuildSpeechPhrases to remove the cast
and run tests to ensure no other callsites rely on the previous type.
In `@apps/sotto/internal/indicator/indicator.go`:
- Around line 174-185: playCue currently spawns a goroutine that blocks on
h.soundMu, allowing queued goroutines to play stale cues; change to a
non-blocking single-slot queue: add a buffered channel field on HyprNotify
(e.g., cueCh chan cueKind with capacity 1), have playCue return early if
!h.cfg.SoundEnable, then non-blockingly attempt to send the cue into cueCh (use
select with default to drop if full) instead of spinning a goroutine and
locking; run a single dedicated goroutine (started once when HyprNotify is
created) that reads from cueCh and calls emitCue(kind, h.cfg) (and use h.soundMu
only inside that single worker if needed) so rapid cues are dropped rather than
queued and stale audio is avoided.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
apps/sotto/internal/config/validate.go (1)
112-117: Dead code: boost comparison in sort is unreachable.Since
selectedis a map keyed by phrase, each phrase inphrasesis unique. The conditionphrases[i].Phrase == phrases[j].Phrase(Line 113) will never be true, making the boost comparison (Line 114) unreachable.♻️ Proposed simplification
sort.Slice(phrases, func(i, j int) bool { - if phrases[i].Phrase == phrases[j].Phrase { - return phrases[i].Boost < phrases[j].Boost - } return phrases[i].Phrase < phrases[j].Phrase })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/config/validate.go` around lines 112 - 117, The comparator passed to sort.Slice contains an unreachable branch comparing Boost because phrases are unique (built from the selected map), so remove the equality branch and simplify the comparator to only compare Phrase; update the call to sort.Slice(phrases, func(i, j int) bool { return phrases[i].Phrase < phrases[j].Phrase }) (or use sort.SliceStable with the same comparator if stability is desired) to eliminate dead code and make intent clear.apps/sotto/internal/indicator/indicator.go (1)
112-130: Benign TOCTOU in ensureFocusedMonitor.The check-then-act pattern (Lines 114-119) releases the lock between checking
focusedMonitorand setting it. Two concurrent calls could both pass the empty check and query the monitor. This is harmless (last write wins, values are identical), but you could eliminate the extra query by holding the lock across the operation or using a sync.Once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/indicator/indicator.go` around lines 112 - 130, The ensureFocusedMonitor method uses a check-then-act pattern on HyprNotify.focusedMonitor with mu unlocked between check and set, allowing redundant hypr.QueryFocusedMonitor calls under concurrency; fix by either holding h.mu for the whole function so you re-check and set under the same lock (lock, if empty call hypr.QueryFocusedMonitor while still holding mu or re-check after obtaining lock), or refactor to use a sync.Once field to perform the query exactly once; update ensureFocusedMonitor (and associated mu or add a once field) so only one goroutine performs hypr.QueryFocusedMonitor and sets h.focusedMonitor.apps/sotto/internal/doctor/doctor.go (2)
112-123: Missing timeout in audio device selection could hang indefinitely.
checkAudioSelectionusescontext.Background()with no timeout. If PulseAudio is unresponsive, this could block the doctor command indefinitely.🛡️ Proposed fix to add timeout
func checkAudioSelection(cfg config.Config) Check { - selection, err := audio.SelectDevice(context.Background(), cfg.Audio.Input, cfg.Audio.Fallback) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + selection, err := audio.SelectDevice(ctx, cfg.Audio.Input, cfg.Audio.Fallback) if err != nil { return Check{Name: "audio.device", Pass: false, Message: err.Error()} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/doctor/doctor.go` around lines 112 - 123, checkAudioSelection currently calls audio.SelectDevice with context.Background(), which can block indefinitely; modify it to use a cancellable context with a timeout (e.g., context.WithTimeout) and defer cancel so SelectDevice is bounded. Create the timeout duration from a sensible constant or a config field (e.g., cfg.Audio.Timeout if available, otherwise a short default like 5s), pass the new ctx into audio.SelectDevice, and ensure the returned error is handled the same way (it will surface context.DeadlineExceeded if timed out).
148-154: Confusing readiness logic: empty body implies "ready".The condition on Line 149 causes an empty response body to report "ready at URL" (Line 153), while a non-empty body without "ready" reports "HTTP %d" (Line 150). This seems inverted—an empty body is less indicative of readiness than a 2xx status alone.
Consider clarifying the intent or adjusting the logic:
♻️ Suggested clarification
bodyText := strings.ToLower(strings.TrimSpace(string(body))) - if bodyText != "" && !strings.Contains(bodyText, "ready") { - return Check{Name: "riva.ready", Pass: true, Message: fmt.Sprintf("HTTP %d from %s", resp.StatusCode, url)} + if strings.Contains(bodyText, "ready") { + return Check{Name: "riva.ready", Pass: true, Message: fmt.Sprintf("ready at %s", url)} } - - return Check{Name: "riva.ready", Pass: true, Message: fmt.Sprintf("ready at %s", url)} + // 2xx without explicit "ready" in body - still considered OK + return Check{Name: "riva.ready", Pass: true, Message: fmt.Sprintf("HTTP %d from %s (no ready indicator in body)", resp.StatusCode, url)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/doctor/doctor.go` around lines 148 - 154, The readiness check in doctor.go is inverted: currently an empty response body results in reporting ready while a non-empty body that does not contain "ready" returns an HTTP message; update the logic in the function that builds the Check{Name: "riva.ready", ...} so that only a non-empty body containing "ready" yields Pass: true and message "ready at <url>", otherwise return Pass: false with a message including the HTTP status and body (e.g., fmt.Sprintf("HTTP %d from %s: %q", resp.StatusCode, url, bodyText)) to make failures explicit; ensure you update the branches that construct the Check to use Pass:false for the non-ready case and keep the Check name "riva.ready".apps/sotto/internal/ipc/server.go (1)
44-64: Consider adding read/write deadlines to prevent resource exhaustion.Per-connection handling lacks timeouts. A misbehaving or slow client could hold a connection indefinitely, exhausting server resources (goroutines, file descriptors). Since this is a local Unix socket IPC, the risk is lower, but adding deadlines would improve robustness.
🛡️ Proposed fix to add connection deadlines
wg.Add(1) go func(c net.Conn) { defer wg.Done() defer c.Close() + + // Set a reasonable deadline for the entire request/response cycle. + _ = c.SetDeadline(time.Now().Add(5 * time.Second)) reader := bufio.NewReader(c) line, err := reader.ReadBytes('\n')Note: This requires importing
"time".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/ipc/server.go` around lines 44 - 64, Add per-connection read/write deadlines to the anonymous goroutine that handles each net.Conn to avoid hung clients; before calling reader.ReadBytes('\n') call c.SetReadDeadline(time.Now().Add(readTimeout)) and before writing responses via json.NewEncoder(c).Encode(...) call c.SetWriteDeadline(time.Now().Add(writeTimeout)); define sensible timeouts (e.g., readTimeout and writeTimeout variables or constants) and import "time", and ensure deadlines are set/renewed appropriately around blocking I/O (reads and writes) so handler.Handle(ctx, req) cannot be used to indefinitely hold the connection.apps/sotto/internal/indicator/sound.go (1)
54-68: File playback errors silently swallowed in fallback path.When
playCueFilefails (Line 57-58), the error is discarded and synthesis is attempted. This is appropriate graceful degradation, but consider logging the file error at debug level so users can diagnose missing/invalid cue files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/indicator/sound.go` around lines 54 - 68, emitCue currently swallows the error returned by playCueFile and falls back to synthesis; update emitCue to log the playCueFile error at debug level before proceeding to cueSamples/playSynthCue so file playback failures are recorded for diagnostics. Specifically, in emitCue (which calls cuePath, playCueFile, cueSamples, playSynthCue) capture the error from playCueFile and send it to your debug logger (use the existing package/app logger facility — e.g., a debug or Tracef call) with context about the cue kind and path, then continue to fallback behavior.apps/sotto/internal/config/parser.go (1)
319-328: Clarify:vocab.globalclears previous values before appending.Line 321 truncates
GlobalSetsto zero length before appending new values. This means multiplevocab.globalassignments will not accumulate; the last one wins. If this is intentional (consistent with "last definition wins" semantics elsewhere), consider adding a brief comment for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/config/parser.go` around lines 319 - 328, The code in the case "vocab.global" currently resets cfg.Vocab.GlobalSets with cfg.Vocab.GlobalSets = cfg.Vocab.GlobalSets[:0] which makes multiple vocab.global entries non-accumulative (last definition wins); make this explicit by adding a brief clarifying comment above the block (referencing case "vocab.global" and cfg.Vocab.GlobalSets) that states the intentional "last-definition-wins" behavior, or if you intended accumulation, remove the truncation line so append preserves previous values—choose one and implement the corresponding change.apps/sotto/internal/config/types.go (1)
73-96: Minor type inconsistency: VocabSet.Boost is float64 but SpeechPhrase.Boost is float32.
VocabSet.Boost(Line 76) isfloat64whileSpeechPhrase.Boost(Line 95) isfloat32. This requires a narrowing conversion inBuildSpeechPhrases(validate.go Line 109). While this is intentional (matching Riva's expected type), documenting the rationale would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/sotto/internal/config/types.go` around lines 73 - 96, VocabSet.Boost and SpeechPhrase.Boost use different float widths which forces a narrowing conversion in BuildSpeechPhrases; update the code to make that explicit and document the reason: add a short comment near the type declarations (VocabSet.Boost or SpeechPhrase.Boost) noting that SpeechPhrase uses float32 to match Riva’s API, and in BuildSpeechPhrases ensure you perform an explicit cast like float32(vs.Boost) with a brief inline comment explaining the intentional conversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/sotto/internal/audio/pulse.go`:
- Around line 272-360: onPCM currently calls c.inflight.Add(1) before checking
c.stopped, which can race with Stop()'s inflight.Wait(); change onPCM so it
checks the stopCh and then acquires c.mu to verify c.stopped, and only if not
stopped call c.inflight.Add(1) while still under the mutex (then release mutex
and continue normal processing); ensure corresponding defer c.inflight.Done()
only runs if Add succeeded and preserve existing checks of stopCh/c.stopped and
chunk emission logic so no Adds occur after Stop() sets c.stopped=true.
In `@apps/sotto/internal/pipeline/transcriber.go`:
- Around line 114-188: After successful StopAndTranscribe or Cancel, reset the
Transcriber state under the mutex so the instance can be reused and to avoid
blocking on a drained sendErrCh; specifically, inside StopAndTranscribe (after
handling sendErr and/or after CloseAndCollect) and inside Cancel (after
stopping/cancelling), acquire t.mu and set t.started = false, t.capture = nil,
t.stream = nil, and t.sendErrCh = nil (do this while holding the mutex to avoid
races). Ensure you perform this cleanup in both the error paths (where you
currently return after writing debug artifacts) and the success path so repeated
StopAndTranscribe/Cancel calls won’t deadlock or prevent Start() from running
again.
---
Nitpick comments:
In `@apps/sotto/internal/config/parser.go`:
- Around line 319-328: The code in the case "vocab.global" currently resets
cfg.Vocab.GlobalSets with cfg.Vocab.GlobalSets = cfg.Vocab.GlobalSets[:0] which
makes multiple vocab.global entries non-accumulative (last definition wins);
make this explicit by adding a brief clarifying comment above the block
(referencing case "vocab.global" and cfg.Vocab.GlobalSets) that states the
intentional "last-definition-wins" behavior, or if you intended accumulation,
remove the truncation line so append preserves previous values—choose one and
implement the corresponding change.
In `@apps/sotto/internal/config/types.go`:
- Around line 73-96: VocabSet.Boost and SpeechPhrase.Boost use different float
widths which forces a narrowing conversion in BuildSpeechPhrases; update the
code to make that explicit and document the reason: add a short comment near the
type declarations (VocabSet.Boost or SpeechPhrase.Boost) noting that
SpeechPhrase uses float32 to match Riva’s API, and in BuildSpeechPhrases ensure
you perform an explicit cast like float32(vs.Boost) with a brief inline comment
explaining the intentional conversion.
In `@apps/sotto/internal/config/validate.go`:
- Around line 112-117: The comparator passed to sort.Slice contains an
unreachable branch comparing Boost because phrases are unique (built from the
selected map), so remove the equality branch and simplify the comparator to only
compare Phrase; update the call to sort.Slice(phrases, func(i, j int) bool {
return phrases[i].Phrase < phrases[j].Phrase }) (or use sort.SliceStable with
the same comparator if stability is desired) to eliminate dead code and make
intent clear.
In `@apps/sotto/internal/doctor/doctor.go`:
- Around line 112-123: checkAudioSelection currently calls audio.SelectDevice
with context.Background(), which can block indefinitely; modify it to use a
cancellable context with a timeout (e.g., context.WithTimeout) and defer cancel
so SelectDevice is bounded. Create the timeout duration from a sensible constant
or a config field (e.g., cfg.Audio.Timeout if available, otherwise a short
default like 5s), pass the new ctx into audio.SelectDevice, and ensure the
returned error is handled the same way (it will surface context.DeadlineExceeded
if timed out).
- Around line 148-154: The readiness check in doctor.go is inverted: currently
an empty response body results in reporting ready while a non-empty body that
does not contain "ready" returns an HTTP message; update the logic in the
function that builds the Check{Name: "riva.ready", ...} so that only a non-empty
body containing "ready" yields Pass: true and message "ready at <url>",
otherwise return Pass: false with a message including the HTTP status and body
(e.g., fmt.Sprintf("HTTP %d from %s: %q", resp.StatusCode, url, bodyText)) to
make failures explicit; ensure you update the branches that construct the Check
to use Pass:false for the non-ready case and keep the Check name "riva.ready".
In `@apps/sotto/internal/indicator/indicator.go`:
- Around line 112-130: The ensureFocusedMonitor method uses a check-then-act
pattern on HyprNotify.focusedMonitor with mu unlocked between check and set,
allowing redundant hypr.QueryFocusedMonitor calls under concurrency; fix by
either holding h.mu for the whole function so you re-check and set under the
same lock (lock, if empty call hypr.QueryFocusedMonitor while still holding mu
or re-check after obtaining lock), or refactor to use a sync.Once field to
perform the query exactly once; update ensureFocusedMonitor (and associated mu
or add a once field) so only one goroutine performs hypr.QueryFocusedMonitor and
sets h.focusedMonitor.
In `@apps/sotto/internal/indicator/sound.go`:
- Around line 54-68: emitCue currently swallows the error returned by
playCueFile and falls back to synthesis; update emitCue to log the playCueFile
error at debug level before proceeding to cueSamples/playSynthCue so file
playback failures are recorded for diagnostics. Specifically, in emitCue (which
calls cuePath, playCueFile, cueSamples, playSynthCue) capture the error from
playCueFile and send it to your debug logger (use the existing package/app
logger facility — e.g., a debug or Tracef call) with context about the cue kind
and path, then continue to fallback behavior.
In `@apps/sotto/internal/ipc/server.go`:
- Around line 44-64: Add per-connection read/write deadlines to the anonymous
goroutine that handles each net.Conn to avoid hung clients; before calling
reader.ReadBytes('\n') call c.SetReadDeadline(time.Now().Add(readTimeout)) and
before writing responses via json.NewEncoder(c).Encode(...) call
c.SetWriteDeadline(time.Now().Add(writeTimeout)); define sensible timeouts
(e.g., readTimeout and writeTimeout variables or constants) and import "time",
and ensure deadlines are set/renewed appropriately around blocking I/O (reads
and writes) so handler.Handle(ctx, req) cannot be used to indefinitely hold the
connection.
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
docs/) and reframe README for generic local-first ASR usagevendorHashfor Nix module fetchingpaste.shortcutCommit sequence
chore(repo): bootstrap tooling, ci, and packagingfeat(asr): implement local-first cli runtime and asr pipelinetest(asr): add broad unit and integration coveragedocs(readme): add progressive-disclosure docs setdocs(config): document configurable paste shortcutVerification
just precommit-runjust prepush-runjust ci-checknix build 'path:.#sotto'Summary by CodeRabbit
New Features
Documentation
Infrastructure