Sound / completion notifier: terminal bell + OSC-9 on turn completion & awaiting-input - #149
Conversation
…ate focus-reporting on enabled
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a dep-free terminal notification package (BEL/OSC-9) with focus gating, wires notify settings through config/resolver/types, exposes per-run CLI flags ChangesNotification system implementation and integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/cli/exec_test.go (1)
1776-1813: ⚡ Quick winExpand notify tests to cover invalid values and stderr-only emission.
These tests cover precedence and conflict, but they don’t guard the two brittle paths: rejecting invalid
--notifyvalues and ensuring notification bytes never leak into stdout (especially stream-json mode).✅ Suggested additional cases
+func TestParseExecNotifyRejectsInvalidValue(t *testing.T) { + _, _, err := parseExecArgs([]string{"--notify", "loud", "hello"}) + if err == nil || !strings.Contains(err.Error(), "invalid --notify") { + t.Fatalf("expected invalid --notify error, got %v", err) + } +} + +func TestRunExecNotifyBellWritesOnlyToStderr(t *testing.T) { + exitCode, stdout, stderr := runExecWithEcho(t, []string{"exec", "--notify", "bell", "--output-format", "stream-json", "hello"}) + if exitCode != exitSuccess { + t.Fatalf("exitCode = %d stdout=%s stderr=%s", exitCode, stdout, stderr) + } + if strings.Contains(stdout, "\a") { + t.Fatalf("stdout must not contain BEL, got %q", stdout) + } + if !strings.Contains(stderr, "\a") { + t.Fatalf("expected BEL on stderr, got %q", stderr) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/exec_test.go` around lines 1776 - 1813, Add two tests to exec_test.go: one that calls parseExecArgs with an invalid --notify value (e.g., "--notify", "invalid", "cmd") and asserts an error is returned to exercise validation in parseExecArgs, and another that runs the exec path in stream-json mode (using execOptions/execCommand helper or invoking the same code path used by execOptions) to assert any notification output is written to stderr and nothing is written to stdout (capture both streams and check stderr contains notification bytes while stdout is empty) so notification bytes never leak into stdout; reference parseExecArgs and execNotifyMode to locate parsing/resolve logic and the exec code path that emits notifications.internal/tui/model_test.go (1)
1300-1327: ⚡ Quick winAdd notifier regression coverage for zero-question
ask_user.This test covers focus-gated completion well. Please also add a case proving no
AwaitingInputemit whenask_userhas zero questions, so the no-input fast-path stays quiet.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/model_test.go` around lines 1300 - 1327, Add a test case that calls the model's ask_user helper with an empty question list and asserts the notifier does not emit an AwaitingInput event: using the existing test's model instance (model, m.notifier) after any desired focus state, call ask_user with zero questions (e.g., ask_user(nil) or ask_user([]Question{})) and verify the notify buffer stays empty (buf.Len()==0) and that no notify.AwaitingInput string was written; reference the ask_user helper, the model type, and notify.AwaitingInput to locate and implement the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/exec_parse.go`:
- Around line 31-44: The notify flag parsing currently assigns any string to
options.notifyMode in both the separate-flag branch (using nextFlagValue) and
the inline branch (using requiredInlineFlagValue); add validation after
obtaining value to accept only "off", "bell", "notify", or "both" and return an
error for any other value so invalid CLI overrides are rejected early; update
the branches that set options.notifyMode (the case handling arg == "--notify"
and the case handling strings.HasPrefix(arg, "--notify=")) to validate the
trimmed value and return the parse error, and make the same change at the other
parsing location that sets options.notifyMode (the code around the other
occurrence noted).
In `@internal/config/resolver_test.go`:
- Around line 1289-1290: Tests calling Resolve are flaky because they rely on
ambient process environment; update each failing test to provide a deterministic
Env by setting ResolveOptions.Env to an explicit empty map (or a controlled map
of expected keys) instead of leaving it nil so Resolve does not call os.Getenv.
Locate calls like Resolve(ResolveOptions{UserConfigPath: path}) in
resolver_test.go and change them to include Env: map[string]string{} (or the
specific env entries needed) for each occurrence (including the other test calls
noted) so the resolver behavior is deterministic.
In `@internal/tui/model.go`:
- Around line 1134-1136: The AwaitingInput notification is sent unconditionally
via m.notifier.Notify(notify.AwaitingInput,
notify.DefaultMessage(notify.AwaitingInput)) even when the ask_user path
auto-resolves empty questions; change the code so you only call
m.notifier.Notify(...) when input is actually required (e.g., check the
questions slice or the ask_user decision that would block — ensure the check
covers the empty-questions auto-resolve path), by moving the notification after
that check (or gating it with a condition like len(questions)>0 or
!autoResolved) so spurious AwaitingInput notifications are not emitted.
---
Nitpick comments:
In `@internal/cli/exec_test.go`:
- Around line 1776-1813: Add two tests to exec_test.go: one that calls
parseExecArgs with an invalid --notify value (e.g., "--notify", "invalid",
"cmd") and asserts an error is returned to exercise validation in parseExecArgs,
and another that runs the exec path in stream-json mode (using
execOptions/execCommand helper or invoking the same code path used by
execOptions) to assert any notification output is written to stderr and nothing
is written to stdout (capture both streams and check stderr contains
notification bytes while stdout is empty) so notification bytes never leak into
stdout; reference parseExecArgs and execNotifyMode to locate parsing/resolve
logic and the exec code path that emits notifications.
In `@internal/tui/model_test.go`:
- Around line 1300-1327: Add a test case that calls the model's ask_user helper
with an empty question list and asserts the notifier does not emit an
AwaitingInput event: using the existing test's model instance (model,
m.notifier) after any desired focus state, call ask_user with zero questions
(e.g., ask_user(nil) or ask_user([]Question{})) and verify the notify buffer
stays empty (buf.Len()==0) and that no notify.AwaitingInput string was written;
reference the ask_user helper, the model type, and notify.AwaitingInput to
locate and implement the assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 89ae58ff-acf5-4a44-9afc-7101e4b70158
📒 Files selected for processing (14)
internal/cli/app.gointernal/cli/exec.gointernal/cli/exec_parse.gointernal/cli/exec_spec.gointernal/cli/exec_test.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/config/types.gointernal/notify/notify.gointernal/notify/notify_test.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/options.gointernal/tui/run.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Findings:
-
P2
internal/cli/exec_parse.go:31:--notifyaccepts any string. Config validation rejects invalid modes, but CLI overrides like--notify buzsilently run with no notification. Please validate the CLI override during parse and accept onlyoff,bell,notify, orboth. -
P2
internal/tui/model.go:1134:AwaitingInputnotification fires before checking whetherask_useractually has questions. Empty question prompts auto-resolve, so this can send a false bell/desktop notification. Gate the notification onlen(request.Questions) > 0or move it after the non-empty check. -
P3
internal/config/resolver_test.go:1289: notify resolver tests should passEnv: map[string]string{}. The resolver falls back toos.Getenvwhen Env is nil, so these tests can become ambient-env dependent.
Validation I ran locally on the PR worktree:
git diff --check origin/main...HEADgo test ./internal/notify ./internal/cli ./internal/tui ./internal/configgo test ./...
All validation passed, but the issues above should be fixed before merge.
…ministic resolver tests Addresses #149 review (CodeRabbit + @Vasanthdev2004): - parseExecArgs rejects an invalid --notify value (only off/bell/notify/both), so a CLI typo errors early instead of silently running with no notification. - TUI OnAskUser fires AwaitingInput only when len(request.Questions) > 0 — a request with no questions auto-resolves without prompting, so the bell/desktop notification was a false positive. - notify resolver tests pass Env: map[string]string{} so Resolve does not fall back to os.Getenv (deterministic, not ambient-env dependent).
|
@Vasanthdev2004 all three addressed in 4019303:
@coderabbitai review |
|
✅ Action performedReview finished.
|
# Conflicts: # internal/config/resolver.go # internal/config/types.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/exec.go`:
- Around line 246-249: The current logic only disables deferral when tool_search
is explicitly in options.disabledTools, but misses the case where an allow-list
is active and omits tool_search; update the guard around effectiveDeferThreshold
so it also treats tool_search as "disabled" when options.enabledTools is
non-empty and does not contain tools.ToolSearchToolName. Concretely, modify the
condition that sets effectiveDeferThreshold = 0 (near
registerToolSearchIfEligible/effectiveDeferThreshold) to check: if
toolListContains(options.disabledTools, tools.ToolSearchToolName) OR
(len(options.enabledTools) > 0 && !toolListContains(options.enabledTools,
tools.ToolSearchToolName)) then set effectiveDeferThreshold = 0 before calling
registerToolSearchIfEligible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b302a5d4-3413-45a4-91fc-7195e0f8f27f
📒 Files selected for processing (5)
internal/cli/app.gointernal/cli/exec.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/config/types.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/config/resolver_test.go
- internal/config/types.go
- internal/cli/app.go
- internal/config/resolver.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/exec.go`:
- Around line 246-249: The current logic only disables deferral when tool_search
is explicitly in options.disabledTools, but misses the case where an allow-list
is active and omits tool_search; update the guard around effectiveDeferThreshold
so it also treats tool_search as "disabled" when options.enabledTools is
non-empty and does not contain tools.ToolSearchToolName. Concretely, modify the
condition that sets effectiveDeferThreshold = 0 (near
registerToolSearchIfEligible/effectiveDeferThreshold) to check: if
toolListContains(options.disabledTools, tools.ToolSearchToolName) OR
(len(options.enabledTools) > 0 && !toolListContains(options.enabledTools,
tools.ToolSearchToolName)) then set effectiveDeferThreshold = 0 before calling
registerToolSearchIfEligible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b302a5d4-3413-45a4-91fc-7195e0f8f27f
📒 Files selected for processing (5)
internal/cli/app.gointernal/cli/exec.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/config/types.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/config/resolver_test.go
- internal/config/types.go
- internal/cli/app.go
- internal/config/resolver.go
🛑 Comments failed to post (1)
internal/cli/exec.go (1)
246-249:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
enabledToolsallow-list dead-end fortool_search.Deferral is disabled when
tool_searchis indisabledTools, but not when an allow-list is active and omitstool_search. That can still activate deferral and hide deferred tools behind an unreachable loader.Suggested fix
effectiveDeferThreshold := resolved.Tools.DeferThreshold if toolListContains(options.disabledTools, tools.ToolSearchToolName) { effectiveDeferThreshold = 0 } +if len(options.enabledTools) > 0 && !toolListContains(options.enabledTools, tools.ToolSearchToolName) { + effectiveDeferThreshold = 0 +} registerToolSearchIfEligible(registry, effectiveDeferThreshold, permissionMode, options.enabledTools, options.disabledTools)Also applies to: 567-575
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/exec.go` around lines 246 - 249, The current logic only disables deferral when tool_search is explicitly in options.disabledTools, but misses the case where an allow-list is active and omits tool_search; update the guard around effectiveDeferThreshold so it also treats tool_search as "disabled" when options.enabledTools is non-empty and does not contain tools.ToolSearchToolName. Concretely, modify the condition that sets effectiveDeferThreshold = 0 (near registerToolSearchIfEligible/effectiveDeferThreshold) to check: if toolListContains(options.disabledTools, tools.ToolSearchToolName) OR (len(options.enabledTools) > 0 && !toolListContains(options.enabledTools, tools.ToolSearchToolName)) then set effectiveDeferThreshold = 0 before calling registerToolSearchIfEligible.
|
@coderabbitai Re: the
So zeroing |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List CodeRabbit review comments on PR `#149` to find the effectiveDeferThreshold comment
gh api repos/Gitlawb/zero/pulls/149/comments --paginate \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: (.body | .[0:200]), url: .html_url}' 2>/dev/null || \
gh api repos/Gitlawb/zero/pulls/149/comments --paginate \
--jq '.[] | select(.user.login | startswith("coderabbit")) | {id: .id, body: (.body | .[0:200]), url: .html_url}'Length of output: 2345 🏁 Script executed: #!/bin/bash
# Check PR reviews for the effectiveDeferThreshold comment
gh api repos/Gitlawb/zero/pulls/149/reviews --paginate \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: (.body | .[0:500]), state: .state}'
# Also search all review comments (including from reviews) for effectiveDeferThreshold
gh api repos/Gitlawb/zero/pulls/149/comments --paginate \
--jq '.[] | select(.body | test("effectiveDeferThreshold"; "i")) | {id: .id, body: (.body | .[0:300]), url: .html_url}'Length of output: 2620
One thing to note: it looks like that inline comment never successfully posted — GitHub returned an internal server error when the review tried to submit it (review ✏️ Learnings added
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: approve.
Rereviewed latest head e62c6d5.
The earlier blockers are resolved:
--notifynow validates values and rejects invalid modes.ask_userwith zero questions no longer emitsAwaitingInputbefore the auto-resolve path.- notify config tests use explicit empty env maps.
I also rechecked the latest CodeRabbit enabledTools/tool_search concern. That does not apply after the merged deferred-tool behavior: tool_search is deliberately exposed and dispatch-allowed on the active path even when an allowlist omits it, while explicit deny still wins.
Validation run locally on the PR worktree:
git diff --check 2ea36cf138e559af947b58e360d8f0d8fc8bdf80...HEADgo test ./internal/notify ./internal/cli ./internal/config ./internal/tuigo test ./...
All passed.
|
@coderabbitai review — the lone finding from the 08:26 run was confirmed a non-issue (tool_search is allowlist-exempt by design; you agreed above), and per your note its inline comment never actually posted. No code changes outstanding. Please re-review so the stale CHANGES_REQUESTED clears. @Vasanthdev2004 — all three of your findings were addressed in commit 4019303: (1) |
|
✅ Action performedReview finished.
|
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@CodeRabbit approve |
✅ Action performedComments resolved and changes approved. |
2 participants
Summary
Adds a dep-free completion / awaiting-input notifier: a terminal bell and/or OSC-9 desktop notification when Zero finishes a turn or needs your input. Opt-in and byte-identical when unconfigured (default
off). No new module dependencies.notify.mode):off(default) |bell(BEL) |notify(OSC-9 desktop notificationESC ]9; <msg> BEL) |both.ask_userquestion).notify.focusMode, TUI only):unfocused(default — only when the terminal is NOT focused) |always|focused. Uses Bubble Tea focus reporting.zero exec --notify <mode>/--no-notify. A TUI/notifypicker is a deliberate follow-up.Design
internal/notify(new, pure, stdlib-only):Mode/FocusMode/Event,Notifier{New, SetFocused, Notify},DefaultMessage,Enabled. Two pure helpers (shouldEmit,sequence) carry the policy and are fully unit-tested without a terminal. Messages are generic ("Zero: ready" / "Zero: needs input") — no prompt/response content (privacy) — and sanitized (ESC/BEL/control stripped, clamped to 120 runes) so they can't break the escape.internal/config):notify.mode/notify.focusMode, validated atResolve(invalid → error), mirroring theSandboxConfigpattern (incl.Overrides/merge threading, for parity with the existing forward-plumbing pattern).internal/tui): focus reporting enabled only when notify is on (sooffstays byte-identical);FocusMsg/BlurMsgtrack focus; completion fires once per finished foreground run (excluded from cancel/quit/background-drain paths); permission/ask_userwrappers fire awaiting-input. Notifier writes to os.Stderr (out-of-band; never the stdout render target).internal/cli): notifier on stderr (never stdout — may carry stream-json); fires completion afteragent.Runon both the normal and--use-specpaths, on success and error. Headless has no focus signal, soexecignoresfocusModeand always emits when a mode is set.Test Plan
go build ./.../GOOS=windows GOARCH=amd64 go build ./...cleango vet ./...clean;gofmtclean (touched files)go test ./...green (new tests acrossnotify,config,cli,tui)go test -race ./internal/{notify,config,cli,tui}/...greenexec --use-specnever notified) and hardened headless focus handling + off-state byte-identityReviewer focus
modeunset/off, no notifier output and focus reporting is not enabled.Known follow-ups (not blockers)
--notify <value>is silently no-op (config values are validated and error). A smallnotify.Valid(mode)guard for the flag is a clean follow-up./notifyinteractive picker.Summary by CodeRabbit
New Features
Config
Tests