feat: add optional sound notifications on run lifecycle events - #96

internal/core/workspace/config_test.go (1)

649-710: Please fold the new sound cases into table-driven subtests.

These assertions are fine, but this file already uses table-driven t.Run(...) coverage for config variants. Grouping the sound scenarios the same way will make it easier to extend coverage for both presets and validation branches without adding more one-off top-level tests. As per coding guidelines, **/*_test.go: Table-driven tests with subtests (t.Run) as the default patternandMUST use t.Run("Should...") pattern for ALL test cases`.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/workspace/config_test.go` around lines 649 - 710, Replace the
three separate top-level tests (TestLoadConfigParsesSoundSection,
TestLoadConfigLeavesSoundDisabledByDefault,
TestLoadConfigRejectsEmptySoundPreset) with a single table-driven test (e.g.,
TestLoadConfig_SoundCases) that defines a slice of cases containing a
descriptive name, the workspace config string, expected error flag, and expected
Sound fields; iterate cases and call t.Run("Should ...", func(t *testing.T) {
... }) for each, create temp dir, writeWorkspaceConfig, call LoadConfig, and
assert expectations (cfg.Sound.Enabled, OnCompleted, OnFailed or error contents
like "sound.on_completed") inside each subtest so all scenarios are covered
uniformly and follow the required t.Run("Should...") pattern.

internal/core/sound/player.go (1)

61-64: Simplify redundant error check.

The error check wrapping runner.Run can be simplified since you're just returning the error directly.

♻️ Simplified return

-	if err := p.runner.Run(ctx, name, args...); err != nil {
-		return err
-	}
-	return nil
+	return p.runner.Run(ctx, name, args...)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/sound/player.go` around lines 61 - 64, The current pattern
checks the error from p.runner.Run then returns it, which is redundant; replace
the if-block with a direct return of the call — i.e., return p.runner.Run(ctx,
name, args...) — to simplify the code in the method that calls p.runner.Run.

internal/core/sound/presets.go (2)

18-18: Consider using full name for consistency: PresetSubmarine.

The constant name PresetSubmar appears truncated compared to other presets (PresetGlass, PresetBasso, etc.). While the value is correctly "submarine", the constant name inconsistency could cause confusion when developers reference it in code.

♻️ Suggested rename

-	PresetSubmar = "submarine"
+	PresetSubmarine = "submarine"

Then update references in KnownPresets() and all preset maps.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/sound/presets.go` at line 18, Rename the truncated constant
PresetSubmar to PresetSubmarine for consistency; update its declaration and
every reference (including in KnownPresets() and all preset maps/variables that
use PresetSubmar) to the new identifier PresetSubmarine, ensuring the string
value remains "submarine" and run/rebuild to catch any missed usages.

---

115-125: Consider resolving Windows presets using the WINDIR environment variable at runtime.

The hardcoded C:\Windows path will fail on non-standard Windows installations where WINDIR points to a different drive. While the comment documents this limitation and users can provide absolute paths as a workaround, using os.Getenv("WINDIR") would make the feature more robust for non-standard setups. This is a low-priority enhancement given the documented workaround exists.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/sound/presets.go` around lines 115 - 125, The windowsPresets
map currently uses hardcoded "C:\Windows\Media\..." paths which can break on
machines where WINDIR is different; update the code so windowsPresets is built
at runtime using os.Getenv("WINDIR") (fallback to "C:\\Windows" if empty) to
join with "Media" and the filenames (e.g., build the map in an init function or
a helper like newWindowsPresets) and keep the same keys (PresetGlass,
PresetBasso, PresetPing, PresetHero, PresetFunk, PresetTink, PresetSubmar) so
callers are unchanged. Ensure path construction uses filepath.Join on Windows.

internal/core/sound/player_test.go (2)

37-45: Consider using table-driven subtests for consistency.

This test covers two distinct cases (empty string and preset name) inline. While the test is simple, using t.Run subtests would align with coding guidelines and improve failure diagnostics.

♻️ Optional refactor to use t.Run

 func TestNoop_Play_AlwaysNil(t *testing.T) {
 	t.Parallel()
-	if err := (Noop{}).Play(context.Background(), ""); err != nil {
-		t.Fatalf("expected nil error from Noop, got %v", err)
-	}
-	if err := (Noop{}).Play(context.Background(), "glass"); err != nil {
-		t.Fatalf("expected nil error from Noop with preset, got %v", err)
-	}
+	t.Run("Should return nil for empty sound", func(t *testing.T) {
+		t.Parallel()
+		if err := (Noop{}).Play(context.Background(), ""); err != nil {
+			t.Fatalf("expected nil error from Noop, got %v", err)
+		}
+	})
+	t.Run("Should return nil for preset name", func(t *testing.T) {
+		t.Parallel()
+		if err := (Noop{}).Play(context.Background(), "glass"); err != nil {
+			t.Fatalf("expected nil error from Noop with preset, got %v", err)
+		}
+	})
 }

As per coding guidelines: "MUST use t.Run("Should...") pattern for ALL test cases".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/sound/player_test.go` around lines 37 - 45, Update
TestNoop_Play_AlwaysNil to use table-driven subtests: create a slice of test
cases (with Name like "Should return nil for empty" and "Should return nil for
preset" or similar) containing the preset string values, call t.Run for each
case and run the (Noop{}).Play(context.Background(), preset) assertion inside
the subtest (optionally calling t.Parallel() inside each subtest), and keep the
original error checks but replace the inline checks with t.Fatalf inside each
t.Run; reference the existing test function TestNoop_Play_AlwaysNil and the
Noop.Play method when locating the change.

---

116-121: Consider removing or expanding this trivial constructor test.

Per coding guidelines, "Constructor tests that only check != nil" is listed as an anti-pattern to reject. This test doesn't verify any meaningful business logic.

If you want to keep coverage for New(), consider testing that the returned player actually behaves correctly (e.g., verify it's an osPlayer with proper wiring by calling Play with a known preset and checking the command resolution).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/sound/player_test.go` around lines 116 - 121, The
TestNew_ReturnsNonNilPlayer test is trivial and should be removed or replaced:
either delete the TestNew_ReturnsNonNilPlayer test entirely, or expand it to
assert behavior by calling New() and verifying the concrete type and wiring
(e.g., assert the returned value from New() is an *osPlayer or implements
expected methods) and exercise a method such as Play with a deterministic preset
to confirm command resolution and dependencies are set up correctly; update the
test to call New(), cast/inspect the returned player, and invoke Play with a
known input to assert expected outputs or mock interactions instead of only
checking for nil.

internal/core/sound/subscriber_test.go (1)

55-56: Remove unnecessary loop variable capture in table-driven tests.

The tc := tc pattern at lines 56 and 90 is no longer needed. Go 1.26.1 (the project's target version) captures loop variables per-iteration by default since Go 1.22. These lines can be safely removed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/sound/subscriber_test.go` around lines 55 - 56, Remove the
redundant per-iteration variable capture statement "tc := tc" inside the
table-driven test loops; locate the "for _, tc := range cases {" loops in the
test and delete the inner "tc := tc" lines (they are unnecessary as Go 1.26.1
already captures loop variables per iteration), leaving the loop body to
reference tc directly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

internal/core/sound/presets_test.go (1)

130-134: Strengthen Windows preset assertion to exact-path validation.

The Windows case currently passes on any path containing "windows", which can mask regressions. Set wantPath explicitly (C:\Windows\Media\ding.wav) and assert exact equality, same as the Darwin/Linux cases.

Also applies to: 163-165

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/sound/presets_test.go` around lines 130 - 134, The Windows
preset test case for PresetPing should assert the exact resolved path instead of
checking for any "windows" substring: add a wantPath field with the literal
"C:\\Windows\\Media\\ding.wav" to the test case (the one using preset PresetPing
and goos "windows") and change the test assertion to compare equality against
wantPath (same exact equality behavior used by the Darwin/Linux cases); apply
the same change to the second Windows case referenced around lines 163-165 so
both Windows entries validate exact path equality rather than a substring.

internal/core/sound/subscriber_test.go (1)

67-68: Remove redundant loop variable captures.

Since Go 1.22+, loop variables are scoped per iteration, making tc := tc unnecessary. The project uses Go 1.26 per go.mod.

♻️ Suggested cleanup

 	for _, tc := range cases {
-		tc := tc
 		t.Run(tc.name, func(t *testing.T) {

Apply similarly at line 102.

Also applies to: 101-102

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/sound/subscriber_test.go` around lines 67 - 68, The test
contains redundant loop-variable shadowing like "tc := tc" inside the for-range
over "cases" (seen in the test loops in subscriber_test.go); remove those
unnecessary reassignments (both occurrences, the one around the first for _, tc
:= range cases and the similar second loop noted) so the code relies on Go 1.22+
per-iteration scoping and no longer creates shadowed "tc" variables.

internal/core/sound/player.go (1)

61-64: Simplify error return.

The runner error handling can be simplified to a single return statement.

♻️ Suggested simplification

-	if err := p.runner.Run(ctx, name, args...); err != nil {
-		return err
-	}
-	return nil
+	return p.runner.Run(ctx, name, args...)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/sound/player.go` around lines 61 - 64, The error handling in
the method can be simplified by returning the runner call directly instead of
using an if-check; replace the current if err := p.runner.Run(ctx, name,
args...); err != nil { return err } return nil with a single return
p.runner.Run(ctx, name, args...) to simplify code in the method where
p.runner.Run is invoked.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes