Refactor claude-notify instructions into module scope#7
Conversation
…ests - add deterministic activation coverage across unit/integration/e2e (U017-U020, I129, E012-E013) - stabilize flaky sender/fallback cases with controlled delivery and osascript stubs - introduce shared shell test helpers and deduplicate shell test scaffolding - simplify notify argument/process handling and tmux notify path internals - update required-case/docs for new test layout and debug observability - move AGENTS/CLAUDE guidance into hooks/claude-notify and scope it to module-only usage
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR removes the repository-wide AGENTS.md, adds a hook-specific AGENTS.md, refactors Swift subprocess and argument handling, standardizes shell test helpers and runners under Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
participant User
participant notify.sh as ShellNotify
participant NotifyDaemon as SwiftDaemon
participant OSAScript as osascript
participant TMUX as tmux
end
User->>ShellNotify: trigger notification (env, subtitle, execute_cmd)
ShellNotify->>SwiftDaemon: invoke notify binary (with args / markers)
alt inside tmux
ShellNotify->>TMUX: construct tmux display payload
TMUX-->>ShellNotify: display / forward execute payload
end
SwiftDaemon->>OSAScript: probe frontmost app (when needed)
SwiftDaemon->>SwiftDaemon: runClickTask(executable, args) -> spawn subprocess
SwiftDaemon-->>ShellNotify: write logs / exit code / payload
ShellNotify->>User: deliver or emit tmux payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (8)
hooks/claude-notify/README.md (2)
116-119: "Shell helper layer" is not a gate — the closing sentence is misleading.The two entries above it ("Required-case gate", "Docstring coverage gate") each name a specific check script. "Shell helper layer" only documents file locations and has no corresponding check command. The trailing bullet at line 119 ("These gates run from Makefile targets…") then sweeps the helper-layer entry into the "gates" set, which is inaccurate.
Consider either:
- Giving it a separate, non-gate heading (e.g., "Shared test helpers"), or
- Scoping the closing sentence to the two actual gates explicitly.
✏️ Suggested rewording
## Maintenance notes - Required-case gate: - `scripts/check-required-cases.sh` validates IDs listed in `Tests/required-cases.txt`. - Docstring coverage gate: - `scripts/check-docstring-coverage.sh --min 80 Sources Tests`. -- Shell helper layer: - - `tests/shell/lib/testlib.sh` contains shared assertions and wait helpers. - - `tests/shell/lib/notify-test-helpers.sh` contains shared fake writers, temp-dir helpers, and execute payload extraction. -- These gates run from Makefile targets and are part of the expected CI quality checks. +- These gates run from Makefile targets and are part of the expected CI quality checks. +- Shared shell test helpers (not gates): + - `tests/shell/lib/testlib.sh`: shared assertions and wait helpers. + - `tests/shell/lib/notify-test-helpers.sh`: shared fake writers, temp-dir helpers, and execute payload extraction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/README.md` around lines 116 - 119, The README currently mislabels "Shell helper layer" as a gate and then lumps it into the trailing sentence that says "These gates run from Makefile targets"; update the README so the helper files are not described as gates: either move the "Shell helper layer" entry under a new heading like "Shared test helpers" (mentioning tests/shell/lib/testlib.sh and tests/shell/lib/notify-test-helpers.sh) or edit the final bullet to explicitly scope "These gates run from Makefile targets" to only the two actual gates ("Required-case gate" and "Docstring coverage gate"), ensuring the helper layer is clearly documented as helpers, not a gate.
40-41: Clarify whichmake test-unitinvocation the description applies to.Line 40 sits between the root-level (lines 32–38) and module-level (lines 42–50) command blocks. A reader could reasonably infer it applies only to the root invocation. Since the same behaviour applies to both, consider moving the note inside both blocks or adding it as a shared paragraph that clearly references both:
✏️ Suggested rewording
+> **Note:** `make test-unit` runs both Swift unit tests and shell unit tests +> (`tests/shell/unit/run.sh`) regardless of invocation location. + From repository root: ```bash make build make test-unit ...-
make test-unitruns both Swift unit tests and shell unit tests (tests/shell/unit/run.sh).From module directory (
hooks/claude-notify):</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@hooks/claude-notify/README.mdaround lines 40 - 41, The note "make test-unitruns both Swift unit tests and shell unit tests
(tests/shell/unit/run.sh)." is ambiguous between the root-level and
module-level command blocks; update README.md so the statement explicitly
applies to both invocations by either placing the sentence inside both command
blocks or adding a short shared paragraph above/between the blocks that
references "root-levelmake test-unit" and "module-levelmake test-unit
(from hooks/claude-notify)" so readers know the behavior applies to both
contexts.</details> </blockquote></details> <details> <summary>hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh (2)</summary><blockquote> `55-112`: **`write_fake_osascript_success` and `write_fake_frontmost_osascript` are identical except for the log-variable name.** The only behavioral difference is `OSASCRIPT_ARGS_LOG` vs `ACTIVATE_OSASCRIPT_ARGS_LOG`. Consider a single parameterised helper, or at minimum a comment calling out the duplication so future changes are applied to both. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sharound lines 55 -
112, The two helpers write_fake_osascript_success and
write_fake_frontmost_osascript are duplicated except for the log env var name;
refactor by creating a single parameterized helper (e.g., write_fake_osascript
<script_path> <log_env_var>) that writes the same script but uses the provided
log variable (OSASCRIPT_ARGS_LOG vs ACTIVATE_OSASCRIPT_ARGS_LOG), update callers
to pass the appropriate env var name, and remove the duplicated function;
alternatively add a clear comment above both functions calling out the
duplication and the need to update both if one changes.</details> --- `146-153`: **`drain_pid_file_if_present` has an undeclared dependency on `testlib.sh`.** It calls `wait_for_pid_file` and `wait_for_pid_removed`, which are only available after `testlib.sh` is sourced. If this helpers file is ever sourced standalone (or before `testlib.sh`), the function will fail at runtime with an unresolved symbol. Adding a comment stating the required sourcing order would prevent future confusion. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sharound lines 146
- 153, The drain_pid_file_if_present helper implicitly depends on
wait_for_pid_file and wait_for_pid_removed from testlib.sh, which can break if
this file is sourced standalone. Add a clear comment at the top of this helpers
file stating it must be sourced after testlib.sh, explicitly naming
wait_for_pid_file and wait_for_pid_removed. Optionally include a brief note near
the drain_pid_file_if_present definition reiterating this dependency to aid
local discovery.</details> </blockquote></details> <details> <summary>hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh (1)</summary><blockquote> `90-93`: **U020 has an implicit hard dependency on U019's state.** U020 reads `$ARGS_LOG` (and the `wait_for_file` gate) established by U019's setup, with no independent setup of its own. If U019 fails or the background notify process is slow, U020's failure will be attributed to a "tmux execute payload" assertion rather than the root cause. A brief comment (`# Uses ARGS_LOG from U019`) or promoting the wait to U020's own setup step would make the dependency explicit. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@hooks/claude-notify/Tests/shell/unit/test-notify-activate.sharound lines 90
- 93, The U020 test implicitly depends on U019's setup because it reads
$ARGS_LOG and relies on wait_for_file created by U019; update the U020 case
(around case_start "U020" / assert_file_contains "U020") to make this explicit
by either (A) adding a brief comment like "# Uses ARGS_LOG from U019"
immediately above the case_start, or (B) promoting the readiness check into U020
by calling wait_for_file "$ARGS_LOG" (or recreating the minimal ARGS_LOG setup)
before assert_file_contains so U020 has its own gating rather than relying on
U019.</details> </blockquote></details> <details> <summary>hooks/claude-notify/Tests/shell/unit/run.sh (1)</summary><blockquote> `7-7`: **Hardcoded `tests/` path would break on a case-sensitive filesystem.** `run.sh` lives under `Tests/shell/unit/` (uppercase) but unconditionally invokes `$PROJECT_DIR/tests/shell/unit/test-notify-activate.sh` (lowercase). On macOS this is fine, but using a self-relative path makes the script portable across case-sensitive environments and consistent with the Makefile's `UNIT_SHELL_TEST_DIR` fallback logic. <details> <summary>♻️ Proposed fix</summary> ```diff -"$PROJECT_DIR/tests/shell/unit/test-notify-activate.sh" +"$(CDPATH= cd -- "$(dirname "$0")" && pwd)/test-notify-activate.sh"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/Tests/shell/unit/run.sh` at line 7, The script run.sh currently calls "$PROJECT_DIR/tests/shell/unit/test-notify-activate.sh" with a hardcoded lowercase path; update the invocation to use the correct case and the Makefile fallback variable by referencing UNIT_SHELL_TEST_DIR (e.g. invoke "$PROJECT_DIR/${UNIT_SHELL_TEST_DIR:-Tests/shell/unit}/test-notify-activate.sh") so the path is self-relative, honors the UNIT_SHELL_TEST_DIR fallback, and works on case-sensitive filesystems while still executing test-notify-activate.sh.hooks/claude-notify/tests/shell/e2e/test-runtime.sh (1)
140-146: Minor nit:OSASCRIPT_LOGis defined but only asserted in E007; in E012 it's passed but not validated.In E007 (Line 142 / Line 153-154), the
OSASCRIPT_LOGis properly asserted. In E012 (Line 241), it's defined and passed to the environment but never checked in assertions (Lines 249-259). This is harmless (useful for debugging), but worth noting for consistency if you want tighter test scoping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/tests/shell/e2e/test-runtime.sh` around lines 140 - 146, Test E012 defines OSASCRIPT_LOG and passes it via CLAUDE_NOTIFY_OSASCRIPT_BIN but never asserts its contents; add the same log assertion used in E007 to E012 to keep tests consistent. In the E012 test block (the section that exports OSASCRIPT_LOG and invokes "$NOTIFY"), after the command completes assert the OSASCRIPT_LOG file exists and contains the expected osascript arguments (mirror the checks used in E007 that read OSASCRIPT_LOG and validate the script arguments), using the same helper or grep/assert pattern so OSASCRIPT_LOG, CLAUDE_NOTIFY_OSASCRIPT_BIN and "$NOTIFY" behavior are verified. Ensure failure messages match existing style.hooks/claude-notify/tests/shell/integration/test-deterministic.sh (1)
225-235: Inconsistent manual cleanup aftermake_case_tmp_dir.Some test cases (e.g., I117 Line 390, I118 Line 424, I119 Line 445) manually
rm -rf "$TMP_DIR"at the end, while others (I115, I116) rely solely on the EXIT trap viacleanup_registered_tmp_dirs. Both approaches work — the EXIT trap acts as a safety net — but the inconsistency is a minor style nit. Consider either always doing manual cleanup for early space reclamation or always relying on the trap for uniformity.Also applies to: 265-275, 316-317, 393-393, 427-427, 448-448, 469-469, 491-491, 544-544, 565-565, 595-595, 678-678, 762-762, 825-825
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/tests/shell/integration/test-deterministic.sh` around lines 225 - 235, The test creates TMP_DIR via make_case_tmp_dir ("I115") and currently relies on the global EXIT trap (cleanup_registered_tmp_dirs) for removal; make this consistent with the other tests by either adding an explicit manual removal (rm -rf "$TMP_DIR") at the end of this test block (after drain_notify_pid_file) or removing manual rm -rf calls from the other tests and relying solely on the trap—update this file so all tests use the same pattern (refer to the TMP_DIR variable, make_case_tmp_dir call, and any existing rm -rf "$TMP_DIR" usage to implement the chosen consistent cleanup approach).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
AGENTS.mdhooks/claude-notify/AGENTS.mdhooks/claude-notify/CLAUDE.mdhooks/claude-notify/Makefilehooks/claude-notify/README.mdhooks/claude-notify/Sources/ClaudeNotify/main.swifthooks/claude-notify/Sources/NotifyCore/NotifyCore.swifthooks/claude-notify/Tests/required-cases.txthooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Tests/shell/unit/run.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/notify.shhooks/claude-notify/tests/shell/e2e/test-runtime.shhooks/claude-notify/tests/shell/integration/test-deterministic.shhooks/claude-notify/tests/shell/lib/testlib.sh
💤 Files with no reviewable changes (1)
- AGENTS.md
🧰 Additional context used
📓 Path-based instructions (3)
hooks/claude-notify/Tests/**/*.{swift,sh}
📄 CodeRabbit inference engine (AGENTS.md)
All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U016 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I128, e2e shell tests use E001–E011
Files:
hooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/Tests/shell/unit/run.sh
hooks/*/+(*.sh|notify)
📄 CodeRabbit inference engine (AGENTS.md)
Hook entrypoints must be shell scripts (e.g., notify.sh) that accept JSON on stdin per the Claude hook payload format, or a message as $1
Files:
hooks/claude-notify/notify.sh
hooks/claude-notify/{Sources,Tests}/**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
Swift docstring coverage threshold: ≥80% on Swift sources in Sources/ and Tests/. Each enum, struct, class, protocol, func, init declaration must have a preceding /// comment
Files:
hooks/claude-notify/Sources/NotifyCore/NotifyCore.swifthooks/claude-notify/Sources/ClaudeNotify/main.swift
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Package.swift : Swift package for hooks must have two targets: NotifyCore (library with all core logic) and ClaudeNotify (executable with thin main.swift entry). Tests target NotifyCore
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/{Sources,Tests}/**/*.swift : Swift docstring coverage threshold: ≥80% on Swift sources in Sources/ and Tests/. Each enum, struct, class, protocol, func, init declaration must have a preceding /// comment
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: When hooks or skills are added, removed, or significantly changed, update README.md: add component section entry with name, description, and 2–3 key feature bullets; update Quick Start if new dependencies are introduced
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: The claude-notify.app/ bundle skeleton must be committed. Binary (Contents/MacOS/) and code-sign (Contents/_CodeSignature/) directories must be gitignored and produced by make build
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Tests/**/*.{swift,sh} : All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U016 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I128, e2e shell tests use E001–E011
Applied to files:
hooks/claude-notify/tests/shell/lib/testlib.shhooks/claude-notify/README.mdhooks/claude-notify/AGENTS.mdhooks/claude-notify/tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Makefilehooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/required-cases.txthooks/claude-notify/Tests/shell/unit/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Package.swift : Swift package for hooks must have two targets: NotifyCore (library with all core logic) and ClaudeNotify (executable with thin main.swift entry). Tests target NotifyCore
Applied to files:
hooks/claude-notify/README.mdhooks/claude-notify/AGENTS.mdhooks/claude-notify/tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Makefilehooks/claude-notify/notify.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/Sources/NotifyCore/NotifyCore.swifthooks/claude-notify/tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/required-cases.txthooks/claude-notify/Sources/ClaudeNotify/main.swifthooks/claude-notify/Tests/shell/unit/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/{Sources,Tests}/**/*.swift : Swift docstring coverage threshold: ≥80% on Swift sources in Sources/ and Tests/. Each enum, struct, class, protocol, func, init declaration must have a preceding /// comment
Applied to files:
hooks/claude-notify/README.mdhooks/claude-notify/AGENTS.mdhooks/claude-notify/Sources/NotifyCore/NotifyCore.swifthooks/claude-notify/Tests/required-cases.txthooks/claude-notify/Sources/ClaudeNotify/main.swift
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Hook behavior is configured via NOTIFY_* environment variables. See hooks/claude-notify/README.md for the full list
Applied to files:
hooks/claude-notify/README.mdhooks/claude-notify/AGENTS.mdhooks/claude-notify/tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Makefilehooks/claude-notify/notify.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/unit/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: The claude-notify.app/ bundle skeleton must be committed. Binary (Contents/MacOS/) and code-sign (Contents/_CodeSignature/) directories must be gitignored and produced by make build
Applied to files:
hooks/claude-notify/README.mdhooks/claude-notify/AGENTS.mdhooks/claude-notify/Makefilehooks/claude-notify/tests/shell/integration/test-deterministic.shhooks/claude-notify/Sources/ClaudeNotify/main.swift
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: When hooks or skills are added, removed, or significantly changed, update README.md: add component section entry with name, description, and 2–3 key feature bullets; update Quick Start if new dependencies are introduced
Applied to files:
hooks/claude-notify/README.md
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/*/+(*.sh|notify) : Hook entrypoints must be shell scripts (e.g., notify.sh) that accept JSON on stdin per the Claude hook payload format, or a message as $1
Applied to files:
hooks/claude-notify/README.mdhooks/claude-notify/AGENTS.mdhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/notify.shhooks/claude-notify/tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/unit/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Tests are tiered (unit → integration → e2e) with sequential execution in CI. Each tier gates the next. Integration tier includes both Swift XCTest (I001–I005) and shell tests (I101+), both run by make test-integration
Applied to files:
hooks/claude-notify/README.mdhooks/claude-notify/Makefile
📚 Learning: 2026-02-17T03:00:33.154Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T03:00:33.154Z
Learning: Applies to AGENTS.md : Document agent implementations and their capabilities in AGENTS.md
Applied to files:
hooks/claude-notify/AGENTS.md
📚 Learning: 2026-02-17T03:00:33.154Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T03:00:33.154Z
Learning: Applies to **/*agent*.{ts,tsx,js,jsx} : Maintain clear agent interface definitions for consistency across agent implementations
Applied to files:
hooks/claude-notify/AGENTS.md
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Root Makefile delegates to component Makefiles via SUB_MAKE. Add new hooks by extending the HOOKS variable
Applied to files:
hooks/claude-notify/Makefile
🧬 Code graph analysis (3)
hooks/claude-notify/tests/shell/e2e/test-runtime.sh (2)
hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh (4)
kill_pid_from_file(127-144)make_case_tmp_dir(8-14)write_fake_osascript_success(55-74)drain_pid_file_if_present(146-154)hooks/claude-notify/tests/shell/lib/testlib.sh (4)
pass(6-9)fail(11-14)wait_for_file(58-68)case_start(16-18)
hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh (1)
hooks/claude-notify/tests/shell/lib/testlib.sh (2)
wait_for_pid_file(20-30)wait_for_pid_removed(32-42)
hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh (1)
hooks/claude-notify/tests/shell/lib/testlib.sh (8)
case_start(16-18)wait_for_file(58-68)assert_rc_eq(75-86)assert_file_contains(88-99)pass(6-9)fail(11-14)assert_file_not_contains(101-112)finish(70-73)
🪛 markdownlint-cli2 (0.21.0)
hooks/claude-notify/AGENTS.md
[warning] 12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
hooks/claude-notify/tests/shell/e2e/test-runtime.sh
[warning] 12-12: TEST_TMP_DIRS appears unused. Verify use (or export if used externally).
(SC2034)
hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh
[warning] 3-3: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 10-10: TEST_TMP_DIRS appears unused. Verify use (or export if used externally).
(SC2034)
hooks/claude-notify/Tests/shell/unit/run.sh
[warning] 5-5: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration
🔇 Additional comments (20)
hooks/claude-notify/AGENTS.md (1)
1-85: LGTM — content is accurate and well-scoped.The updated case-ID ranges (U001–U020, I101–I129, E001–E013) correctly reflect the new tests introduced in this PR's commit messages. All conventions (entrypoint contract, NotifyCore/thin-main split, app-bundle skeleton, docstring threshold, README sync rule) align with the established module guidelines. Based on learnings from prior reviews, these invariants are all properly captured here.
hooks/claude-notify/Sources/ClaudeNotify/main.swift (1)
319-368: LGTM — clean deduplication with no behavior change.The extraction of
runClickTaskis well-structured:
- Non-escaping closures for message construction are called strictly within the synchronous body — no capture/retain hazard.
- Control flow is semantically identical to the previous inline code: a non-zero exit from the
executetask emits a warning but does not short-circuit the subsequentactivatecall, which matches the intended "best-effort" semantics.- The
///doc comment with explicit- Parameters:entries satisfies the ≥80% docstring coverage requirement for this path.hooks/claude-notify/Makefile (1)
21-53: LGTM —UNIT_SHELL_TEST_DIRfallback and hook-up totest-unitlook correct.The wildcard-based fallback (preferring
tests/shell/unitoverTests/shell/unit) is a clean migration bridge, and bothcheck-cases-unitandtest-unitare consistently updated to use the variable.hooks/claude-notify/tests/shell/lib/testlib.sh (1)
75-112: LGTM — new assertion helpers are correct and consistent with the existing harness.
assert_rc_equses arithmetic comparison,assert_file_contains/assert_file_not_containsusegrep -q --which correctly handles pattern injection, and the BRE patterns in the callers (\]=-activate$,\]=-execute$) are compatible.hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh (1)
10-10: ShellCheck SC2034 onTEST_TMP_DIRSis a false positive.The variable is initialised here and consumed by
register_tmp_dir(and subsequentlycleanup_registered_tmp_dirs) in the sourcednotify-test-helpers.sh. ShellCheck cannot see cross-source usage, so the warning can be safely ignored or suppressed with# shellcheck disable=SC2034.hooks/claude-notify/notify.sh (2)
60-77: LGTM —tmux_notifycorrectly centralises the tmux notification path.
NOTIFY_ACTIVATE_OVERRIDE=""as a command prefix correctly suppresses-activatefor tmux deliveries (where activation is delegated to the execute payload), and theset --accumulation pattern is safe because POSIX shells scope positional-parameter modifications to the function's own frame.
152-162: LGTM — outside-tmux activation inference correctly mirrors the tmux path.Inferring
ACTIVATE_BUNDLE_IDonly when unset (rather than unconditionally) preserves the explicit-override semantics ofNOTIFY_ACTIVATE_BUNDLE_ID, and running the inference synchronously before the backgroundednotify_runensures the inferred bundle is available before the notification fires.hooks/claude-notify/tests/shell/e2e/test-runtime.sh (6)
6-7: New helper sourcing and shared state initialization look correct.The
notify-test-helpers.shsource andEXPECTED_NOTIFY_NAMEextraction align well with the centralized helper approach.Regarding ShellCheck SC2034 on
TEST_TMP_DIRS(Line 12): this is a false positive — the variable is initialized here and mutated/consumed byregister_tmp_dir/cleanup_registered_tmp_dirsin the sourced helper library, which ShellCheck cannot trace across files.Also applies to: 12-13
16-21: LGTM!The cleanup function properly guards process termination with the expected binary name and delegates temp-dir cleanup to the centralized helper.
138-163: LGTM!E007's rewrite properly isolates the test with a per-case tmp dir, uses a deterministic fake osascript, and validates the full fallback chain (exit code, payload content, and stderr diagnostics). The registered tmp dir is cleaned up by the EXIT trap.
213-236: LGTM!E011 correctly validates the distinction between AppleScript delivery fallback (expected for
requiredsender mode after post error) and non-spoof re-launch fallback (which should not happen inrequiredmode). The assertions align with the test description.
261-292: LGTM!E013 correctly simulates the tmux environment with a fake tmux binary that returns deterministic metadata, then validates that the debug log captures the full inference chain: bundle ID inference →
notify_runparameters → execute payload preparation. Thegrep -qwith.*on Line 288 works correctly as BRE.
238-259: No action required. E012 and E013 are properly registered inhooks/claude-notify/Tests/required-cases.txtand comply with the stable-ID invariant for e2e shell tests.Likely an incorrect or invalid review comment.
hooks/claude-notify/tests/shell/integration/test-deterministic.sh (7)
17-20: LGTM — Clean local wrapper.
drain_notify_pid_fileis a well-scoped convenience wrapper that captures the repeated pattern of draining with the file-scoped PID_FILE and expected name. Reduces repetition across the test cases.
564-592: LGTM — I124 rewrite properly validates outside-tmux activation inference.The test correctly sets
NOTIFY_ACTIVATE_BUNDLE_ID=""to force inference, provides a fake frontmost-app osascript, and validates both the inferred bundle ID forwarding and the osascript probe invocation.
761-822: LGTM!I128's rewrite cleanly replaces inline scripting with the shared
write_fake_frontmost_osascripthelper andextract_execute_payload. The assertions correctly validate the full tmux activate chain: no native-activate, inferred bundle in redirect payload, and argv-based osascript activation.
824-851: I129 is well-designed — verify test ID registration alongside E012/E013.Good edge-case coverage: intentionally pointing
NOTIFY_ACTIVATE_OSASCRIPT_BINto a non-existent path validates graceful degradation without activate inference, while still forwarding the notification payload. The samerequired-cases.txtregistration concern from E012/E013 applies here.
358-390: LGTM —extract_execute_payloadhelper centralizes a common extraction pattern.Good refactor from presumably manual grep/sed extraction to a shared helper. The payload execution test chain (extract → execute → validate args) is clean and deterministic.
490-541: LGTM!I122's two-phase validation (successful AppleScript fallback + failing AppleScript fallback) is thorough and well-organized with the centralized helpers.
22-27: LGTM!The cleanup function mirrors the e2e test pattern: guarded PID kill, explicit PID file removal, and registered tmp dir cleanup via EXIT trap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/claude-notify/AGENTS.md`:
- Around line 12-34: The fenced directory-tree block in AGENTS.md for the
hooks/claude-notify section is missing a language tag which triggers MD040;
update the opening triple-backticks to include a language identifier like text
(e.g., change ``` to ```text) so the tree block is fenced as plain text; locate
the directory tree block under the hooks/claude-notify listing in AGENTS.md (the
block showing hooks/claude-notify/ ├── Makefile ... claude-notify.app/) and add
the language tag to the opening fence.
In `@hooks/claude-notify/Sources/NotifyCore/NotifyCore.swift`:
- Around line 233-241: The DocC comment for rewriteArguments is broken by a
plain // line between doc-comment lines; replace that lone // with a /// (an
empty doc-comment continuation) or remove it so the doc comment lines for
rewriteArguments remain contiguous and use /// throughout, ensuring the -
Parameters: and - Returns: sections are included in generated DocC/Xcode Quick
Help for the rewriteArguments function.
In `@hooks/claude-notify/Tests/required-cases.txt`:
- Around line 21-24: AGENTS.md and the range-check logic in
check-required-cases.sh must be updated to reflect the new test ID ceilings
introduced by the PR: expand the unit ID upper bound to U020, the integration
Swift/XCTest upper bound to I129, and the e2e shell tests upper bound to E013;
update any descriptive text that lists previous ceilings (e.g., "U001–U016",
"I001–I005", "I101–I128", "E001–E011") and ensure the generator/validator that
reads required-cases.txt (which now contains U017–U020, I129, E012–E013) will
accept those IDs. Address both AGENTS.md and the check-required-cases.sh logic
so documentation and the enforcement script remain consistent with the new IDs.
---
Nitpick comments:
In `@hooks/claude-notify/README.md`:
- Around line 116-119: The README currently mislabels "Shell helper layer" as a
gate and then lumps it into the trailing sentence that says "These gates run
from Makefile targets"; update the README so the helper files are not described
as gates: either move the "Shell helper layer" entry under a new heading like
"Shared test helpers" (mentioning tests/shell/lib/testlib.sh and
tests/shell/lib/notify-test-helpers.sh) or edit the final bullet to explicitly
scope "These gates run from Makefile targets" to only the two actual gates
("Required-case gate" and "Docstring coverage gate"), ensuring the helper layer
is clearly documented as helpers, not a gate.
- Around line 40-41: The note "`make test-unit` runs both Swift unit tests and
shell unit tests (`tests/shell/unit/run.sh`)." is ambiguous between the
root-level and module-level command blocks; update README.md so the statement
explicitly applies to both invocations by either placing the sentence inside
both command blocks or adding a short shared paragraph above/between the blocks
that references "root-level `make test-unit`" and "module-level `make test-unit`
(from hooks/claude-notify)" so readers know the behavior applies to both
contexts.
In `@hooks/claude-notify/tests/shell/e2e/test-runtime.sh`:
- Around line 140-146: Test E012 defines OSASCRIPT_LOG and passes it via
CLAUDE_NOTIFY_OSASCRIPT_BIN but never asserts its contents; add the same log
assertion used in E007 to E012 to keep tests consistent. In the E012 test block
(the section that exports OSASCRIPT_LOG and invokes "$NOTIFY"), after the
command completes assert the OSASCRIPT_LOG file exists and contains the expected
osascript arguments (mirror the checks used in E007 that read OSASCRIPT_LOG and
validate the script arguments), using the same helper or grep/assert pattern so
OSASCRIPT_LOG, CLAUDE_NOTIFY_OSASCRIPT_BIN and "$NOTIFY" behavior are verified.
Ensure failure messages match existing style.
In `@hooks/claude-notify/tests/shell/integration/test-deterministic.sh`:
- Around line 225-235: The test creates TMP_DIR via make_case_tmp_dir ("I115")
and currently relies on the global EXIT trap (cleanup_registered_tmp_dirs) for
removal; make this consistent with the other tests by either adding an explicit
manual removal (rm -rf "$TMP_DIR") at the end of this test block (after
drain_notify_pid_file) or removing manual rm -rf calls from the other tests and
relying solely on the trap—update this file so all tests use the same pattern
(refer to the TMP_DIR variable, make_case_tmp_dir call, and any existing rm -rf
"$TMP_DIR" usage to implement the chosen consistent cleanup approach).
In `@hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh`:
- Around line 55-112: The two helpers write_fake_osascript_success and
write_fake_frontmost_osascript are duplicated except for the log env var name;
refactor by creating a single parameterized helper (e.g., write_fake_osascript
<script_path> <log_env_var>) that writes the same script but uses the provided
log variable (OSASCRIPT_ARGS_LOG vs ACTIVATE_OSASCRIPT_ARGS_LOG), update callers
to pass the appropriate env var name, and remove the duplicated function;
alternatively add a clear comment above both functions calling out the
duplication and the need to update both if one changes.
- Around line 146-153: The drain_pid_file_if_present helper implicitly depends
on wait_for_pid_file and wait_for_pid_removed from testlib.sh, which can break
if this file is sourced standalone. Add a clear comment at the top of this
helpers file stating it must be sourced after testlib.sh, explicitly naming
wait_for_pid_file and wait_for_pid_removed. Optionally include a brief note near
the drain_pid_file_if_present definition reiterating this dependency to aid
local discovery.
In `@hooks/claude-notify/Tests/shell/unit/run.sh`:
- Line 7: The script run.sh currently calls
"$PROJECT_DIR/tests/shell/unit/test-notify-activate.sh" with a hardcoded
lowercase path; update the invocation to use the correct case and the Makefile
fallback variable by referencing UNIT_SHELL_TEST_DIR (e.g. invoke
"$PROJECT_DIR/${UNIT_SHELL_TEST_DIR:-Tests/shell/unit}/test-notify-activate.sh")
so the path is self-relative, honors the UNIT_SHELL_TEST_DIR fallback, and works
on case-sensitive filesystems while still executing test-notify-activate.sh.
In `@hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh`:
- Around line 90-93: The U020 test implicitly depends on U019's setup because it
reads $ARGS_LOG and relies on wait_for_file created by U019; update the U020
case (around case_start "U020" / assert_file_contains "U020") to make this
explicit by either (A) adding a brief comment like "# Uses ARGS_LOG from U019"
immediately above the case_start, or (B) promoting the readiness check into U020
by calling wait_for_file "$ARGS_LOG" (or recreating the minimal ARGS_LOG setup)
before assert_file_contains so U020 has its own gating rather than relying on
U019.
| /// Rewrites CLI arguments while dropping selected internal flags. | ||
| /// | ||
| /// - Parameter argv: Original process arguments without executable name. | ||
| /// - Returns: Arguments with spoof-only options removed and fallback markers applied. | ||
| public func buildFallbackArguments(from argv: [String]) -> [String] { | ||
| /// - Parameters: | ||
| /// - argv: Original process arguments without executable name. | ||
| /// - dropStandaloneFlags: Flags removed without consuming trailing values. | ||
| /// - dropValueFlags: Value flags removed with their value when present. | ||
| /// - preserveDanglingValueFlags: Whether value flags missing a value should be preserved. | ||
| /// - Returns: Rewritten argument array. | ||
| private func rewriteArguments( |
There was a problem hiding this comment.
Plain // on line 234 breaks the DocC block for rewriteArguments.
Swift DocC requires contiguous /// lines. The bare // between lines 233 and 235 terminates the doc comment, leaving the - Parameters: and - Returns: sections orphaned and invisible in Xcode Quick Help / generated docs. Line 234 should be /// (an empty doc-comment continuation) or removed.
🔧 Proposed fix
/// Rewrites CLI arguments while dropping selected internal flags.
-//
+///
/// - Parameters:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Rewrites CLI arguments while dropping selected internal flags. | |
| /// | |
| /// - Parameter argv: Original process arguments without executable name. | |
| /// - Returns: Arguments with spoof-only options removed and fallback markers applied. | |
| public func buildFallbackArguments(from argv: [String]) -> [String] { | |
| /// - Parameters: | |
| /// - argv: Original process arguments without executable name. | |
| /// - dropStandaloneFlags: Flags removed without consuming trailing values. | |
| /// - dropValueFlags: Value flags removed with their value when present. | |
| /// - preserveDanglingValueFlags: Whether value flags missing a value should be preserved. | |
| /// - Returns: Rewritten argument array. | |
| private func rewriteArguments( | |
| /// Rewrites CLI arguments while dropping selected internal flags. | |
| /// | |
| /// - Parameters: | |
| /// - argv: Original process arguments without executable name. | |
| /// - dropStandaloneFlags: Flags removed without consuming trailing values. | |
| /// - dropValueFlags: Value flags removed with their value when present. | |
| /// - preserveDanglingValueFlags: Whether value flags missing a value should be preserved. | |
| /// - Returns: Rewritten argument array. | |
| private func rewriteArguments( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/claude-notify/Sources/NotifyCore/NotifyCore.swift` around lines 233 -
241, The DocC comment for rewriteArguments is broken by a plain // line between
doc-comment lines; replace that lone // with a /// (an empty doc-comment
continuation) or remove it so the doc comment lines for rewriteArguments remain
contiguous and use /// throughout, ensuring the - Parameters: and - Returns:
sections are included in generated DocC/Xcode Quick Help for the
rewriteArguments function.
| U017 | ||
| U018 | ||
| U019 | ||
| U020 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Update the stated ID ranges in AGENTS.md to cover the new ceilings.
The new entries U017–U020, I129, and E012–E013 exceed the ranges codified in the project guidelines. Since AGENTS.md is being rewritten in this PR, it should also update the stated upper bounds to at least U020, I129, and E013 so the check-required-cases.sh script and documentation stay consistent.
Based on learnings: "Unit tests use U001–U016 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I128, e2e shell tests use E001–E011" — all three upper bounds are exceeded by this PR.
Also applies to: 62-62, 76-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/claude-notify/Tests/required-cases.txt` around lines 21 - 24, AGENTS.md
and the range-check logic in check-required-cases.sh must be updated to reflect
the new test ID ceilings introduced by the PR: expand the unit ID upper bound to
U020, the integration Swift/XCTest upper bound to I129, and the e2e shell tests
upper bound to E013; update any descriptive text that lists previous ceilings
(e.g., "U001–U016", "I001–I005", "I101–I128", "E001–E011") and ensure the
generator/validator that reads required-cases.txt (which now contains U017–U020,
I129, E012–E013) will accept those IDs. Address both AGENTS.md and the
check-required-cases.sh logic so documentation and the enforcement script remain
consistent with the new IDs.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- standardize shell test paths to canonical `Tests/shell/...` across runners, docs, and Makefile - add `scripts/check-shell-path-casing.sh` and wire it into `make test-unit` prechecks - remove unit shell path fallback indirection from Makefile - update `AGENTS.md` and `README.md` path references and markdown fence language tag - align shell test entrypoints with case-sensitive filesystem compatibility
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh (1)
90-93: U020 implicitly depends on U019's$ARGS_LOGwithout its own setup.U020 reuses the
$ARGS_LOGvariable set during U019's execution. If U019 fails early or the file is missing, U020'sassert_file_containswill fail with a confusinggreperror (file not found) rather than a descriptive test failure. Consider adding a guard or at minimum documenting this coupling.Optional: guard against missing file
case_start "U020" "notify.sh tmux execute payload carries inferred activate bundle" +if [ ! -f "$ARGS_LOG" ]; then + fail "U020" "ARGS_LOG from U019 not available" +else assert_file_contains "U020" "$ARGS_LOG" "tmux-redirect.sh'.*'com.jetbrains.intellij'" \ "notify.sh passes inferred activate bundle to tmux redirect execute payload" \ "notify.sh did not carry inferred activate bundle in tmux execute payload" +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh` around lines 90 - 93, The U020 test implicitly relies on the ARGS_LOG file created by U019; to fix this, make U020 self-contained by ensuring ARGS_LOG exists before calling assert_file_contains: either invoke the same setup that creates ARGS_LOG (or create a blank ARGS_LOG file) after case_start "U020" or add an explicit guard that checks for ARGS_LOG and fails with a clear message (e.g., "U020 requires ARGS_LOG but file not found") so assert_file_contains (and grep) never errors with a cryptic file-not-found; reference the test identifiers case_start "U020", the ARGS_LOG variable, and the assert_file_contains invocation when applying the change.hooks/claude-notify/scripts/check-shell-path-casing.sh (1)
9-25: Fragile if any listed path doesn't exist.If any of the explicitly listed files (e.g.,
AGENTS.md,test-claude-notify.sh) is missing, bothrgandgrepwill emit warnings to stderr. Consider redirecting stderr or adding existence checks for the target paths, especially since this runs as a CI precheck.Optional: suppress missing-path noise
matches=$(rg --line-number --fixed-strings "$PATTERN" \ "$PROJECT_DIR/Makefile" \ "$PROJECT_DIR/README.md" \ "$PROJECT_DIR/AGENTS.md" \ "$PROJECT_DIR/test-claude-notify.sh" \ "$PROJECT_DIR/Tests/shell" \ - "$PROJECT_DIR/scripts" | grep -v '/scripts/check-shell-path-casing.sh:' || true) + "$PROJECT_DIR/scripts" 2>/dev/null | grep -v '/scripts/check-shell-path-casing.sh:' || true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/scripts/check-shell-path-casing.sh` around lines 9 - 25, The search command is noisy when any of the hard-coded targets (e.g., "$PROJECT_DIR/AGENTS.md", "$PROJECT_DIR/test-claude-notify.sh", "$PROJECT_DIR/scripts") are missing; update the logic in check-shell-path-casing.sh around the matches assignment so you either (a) build a list/array of candidate paths by checking existence (e.g., test -e) and pass only existing files/dirs to rg/grep, or (b) suppress missing-file warnings by redirecting stderr (append 2>/dev/null) to the rg and grep invocations; keep the use of PATTERN and PROJECT_DIR and preserve the grep -v exclusion for scripts/check-shell-path-casing.sh.hooks/claude-notify/Tests/shell/unit/run.sh (1)
5-5:CDPATH=''silences the false-positive SC1007 warning without changing semantics.ShellCheck SC1007 fires because it misreads
CDPATH= cdas an incomplete variable assignment. The idiom is valid POSIX (temporary env-var prefix), but writingCDPATH=''is identical in effect and explicitly communicates "empty string."🔧 Proposed fix
-PROJECT_DIR="$(CDPATH= cd -- "$(dirname "$0")/../../.." && pwd)" +PROJECT_DIR="$(CDPATH='' cd -- "$(dirname "$0")/../../.." && pwd)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/Tests/shell/unit/run.sh` at line 5, Replace the temporary environment prefix in the PROJECT_DIR assignment to silence ShellCheck SC1007 by using an explicit empty CDPATH value: change the invocation that sets CDPATH before cd (the PROJECT_DIR="$(CDPATH= cd -- "$(dirname "$0")/../../.." && pwd)") to use CDPATH='' so the semantics remain identical but the linter no longer flags it; update the PROJECT_DIR assignment accordingly, leaving the rest of the command (cd -- "$(dirname "$0")/../../.." && pwd) unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
hooks/claude-notify/AGENTS.mdhooks/claude-notify/Makefilehooks/claude-notify/README.mdhooks/claude-notify/Tests/shell/e2e/run.shhooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/integration/run.shhooks/claude-notify/Tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/lib/testlib.shhooks/claude-notify/Tests/shell/run.shhooks/claude-notify/Tests/shell/unit/run.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/scripts/check-shell-path-casing.shhooks/claude-notify/test-claude-notify.shhooks/claude-notify/tests/shell/run.sh
💤 Files with no reviewable changes (1)
- hooks/claude-notify/tests/shell/run.sh
✅ Files skipped from review due to trivial changes (1)
- hooks/claude-notify/Tests/shell/integration/run.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- hooks/claude-notify/Makefile
- hooks/claude-notify/README.md
- hooks/claude-notify/AGENTS.md
🧰 Additional context used
📓 Path-based instructions (2)
hooks/*/+(*.sh|notify)
📄 CodeRabbit inference engine (AGENTS.md)
Hook entrypoints must be shell scripts (e.g., notify.sh) that accept JSON on stdin per the Claude hook payload format, or a message as $1
Files:
hooks/claude-notify/test-claude-notify.sh
hooks/claude-notify/Tests/**/*.{swift,sh}
📄 CodeRabbit inference engine (AGENTS.md)
All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U016 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I128, e2e shell tests use E001–E011
Files:
hooks/claude-notify/Tests/shell/run.shhooks/claude-notify/Tests/shell/lib/testlib.shhooks/claude-notify/Tests/shell/e2e/run.shhooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/run.shhooks/claude-notify/Tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.sh
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Package.swift : Swift package for hooks must have two targets: NotifyCore (library with all core logic) and ClaudeNotify (executable with thin main.swift entry). Tests target NotifyCore
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Tests/**/*.{swift,sh} : All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U016 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I128, e2e shell tests use E001–E011
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/{Sources,Tests}/**/*.swift : Swift docstring coverage threshold: ≥80% on Swift sources in Sources/ and Tests/. Each enum, struct, class, protocol, func, init declaration must have a preceding /// comment
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Hook behavior is configured via NOTIFY_* environment variables. See hooks/claude-notify/README.md for the full list
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: The claude-notify.app/ bundle skeleton must be committed. Binary (Contents/MacOS/) and code-sign (Contents/_CodeSignature/) directories must be gitignored and produced by make build
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Tests are tiered (unit → integration → e2e) with sequential execution in CI. Each tier gates the next. Integration tier includes both Swift XCTest (I001–I005) and shell tests (I101+), both run by make test-integration
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/*/+(*.sh|notify) : Hook entrypoints must be shell scripts (e.g., notify.sh) that accept JSON on stdin per the Claude hook payload format, or a message as $1
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Package.swift : Swift package for hooks must have two targets: NotifyCore (library with all core logic) and ClaudeNotify (executable with thin main.swift entry). Tests target NotifyCore
Applied to files:
hooks/claude-notify/test-claude-notify.shhooks/claude-notify/Tests/shell/run.shhooks/claude-notify/Tests/shell/e2e/run.shhooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/run.shhooks/claude-notify/Tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/*/+(*.sh|notify) : Hook entrypoints must be shell scripts (e.g., notify.sh) that accept JSON on stdin per the Claude hook payload format, or a message as $1
Applied to files:
hooks/claude-notify/test-claude-notify.shhooks/claude-notify/scripts/check-shell-path-casing.shhooks/claude-notify/Tests/shell/run.shhooks/claude-notify/Tests/shell/e2e/run.shhooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/run.shhooks/claude-notify/Tests/shell/integration/test-deterministic.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Tests/**/*.{swift,sh} : All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U016 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I128, e2e shell tests use E001–E011
Applied to files:
hooks/claude-notify/test-claude-notify.shhooks/claude-notify/Tests/shell/run.shhooks/claude-notify/Tests/shell/lib/testlib.shhooks/claude-notify/Tests/shell/e2e/run.shhooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/run.shhooks/claude-notify/Tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Hook behavior is configured via NOTIFY_* environment variables. See hooks/claude-notify/README.md for the full list
Applied to files:
hooks/claude-notify/test-claude-notify.shhooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/run.shhooks/claude-notify/Tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Tests are tiered (unit → integration → e2e) with sequential execution in CI. Each tier gates the next. Integration tier includes both Swift XCTest (I001–I005) and shell tests (I101+), both run by make test-integration
Applied to files:
hooks/claude-notify/Tests/shell/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: The claude-notify.app/ bundle skeleton must be committed. Binary (Contents/MacOS/) and code-sign (Contents/_CodeSignature/) directories must be gitignored and produced by make build
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/run.shhooks/claude-notify/Tests/shell/integration/test-deterministic.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/{Sources,Tests}/**/*.swift : Swift docstring coverage threshold: ≥80% on Swift sources in Sources/ and Tests/. Each enum, struct, class, protocol, func, init declaration must have a preceding /// comment
Applied to files:
hooks/claude-notify/Tests/shell/unit/run.sh
🧬 Code graph analysis (3)
hooks/claude-notify/Tests/shell/lib/testlib.sh (1)
hooks/claude-notify/tests/shell/lib/testlib.sh (4)
pass(6-9)finish(70-73)case_start(16-18)fail(11-14)
hooks/claude-notify/Tests/shell/e2e/test-runtime.sh (2)
hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh (5)
kill_pid_from_file(127-144)cleanup_registered_tmp_dirs(16-20)make_case_tmp_dir(8-14)write_fake_osascript_success(55-74)drain_pid_file_if_present(146-154)hooks/claude-notify/Tests/shell/lib/testlib.sh (4)
pass(6-9)fail(11-14)wait_for_file(58-68)case_start(16-18)
hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh (2)
hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh (4)
cleanup_registered_tmp_dirs(16-20)make_case_tmp_dir(8-14)write_fake_notify(22-37)write_fake_frontmost_osascript(93-112)hooks/claude-notify/Tests/shell/lib/testlib.sh (8)
case_start(16-18)wait_for_file(58-68)assert_rc_eq(75-86)assert_file_contains(88-99)pass(6-9)fail(11-14)assert_file_not_contains(101-112)finish(70-73)
🪛 Shellcheck (0.11.0)
hooks/claude-notify/scripts/check-shell-path-casing.sh
[warning] 5-5: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 6-6: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
hooks/claude-notify/Tests/shell/run.sh
[warning] 5-5: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
hooks/claude-notify/Tests/shell/e2e/test-runtime.sh
[warning] 12-12: TEST_TMP_DIRS appears unused. Verify use (or export if used externally).
(SC2034)
hooks/claude-notify/Tests/shell/unit/run.sh
[warning] 5-5: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh
[warning] 3-3: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 10-10: TEST_TMP_DIRS appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration
🔇 Additional comments (10)
hooks/claude-notify/Tests/shell/lib/testlib.sh (1)
75-112: Clean assertion helpers — LGTM.The three new helpers (
assert_rc_eq,assert_file_contains,assert_file_not_contains) reduce boilerplate across test scripts with a consistent interface and proper use of--to guard against patterns being interpreted as flags.hooks/claude-notify/Tests/shell/e2e/test-runtime.sh (3)
4-7: Updated source paths and cleanup logic — LGTM.Source paths correctly point to
Tests/shell/lib/, and cleanup now useskill_pid_from_filewith an expected-name guard andcleanup_registered_tmp_dirsfor temp directory cleanup. This is a solid improvement over inline cleanup.Also applies to: 12-19
138-163: E007 expanded with AppleScript fallback verification — LGTM.The test now validates the deterministic AppleScript fallback path by verifying the osascript args log contains the expected payload (
[3]=post-test) and checking stderr for the fallback success message. Good coverage of the fallback delivery path.
238-292: No action needed. Test IDs E012–E013 are already registered in Tests/required-cases.txt and the documented range in AGENTS.md is already updated to E001–E013. The e2e test suite documentation and registration are current.Likely an incorrect or invalid review comment.
hooks/claude-notify/Tests/shell/integration/test-deterministic.sh (3)
17-26: Centralized PID drain and cleanup — LGTM.
drain_notify_pid_fileneatly wraps the common pattern of draining with the expected notify name, and the cleanup function properly useskill_pid_from_filewith a name guard pluscleanup_registered_tmp_dirs. Good consolidation.
358-358: Consistent use ofextract_execute_payload— LGTM.Replacing inline
awkblocks with the sharedextract_execute_payloadhelper improves readability and maintainability across I117, I125, I127, and I128.Also applies to: 621-621, 730-730, 801-801
824-851: No action required — I129 is already registered in Tests/required-cases.txt.I129 appears at line 62 of
hooks/claude-notify/Tests/required-cases.txt, so the test is properly registered per coding guidelines. The test code is compliant.Likely an incorrect or invalid review comment.
hooks/claude-notify/test-claude-notify.sh (1)
6-6: Path casing update toTests/shell/run.sh— LGTM.Consistent with the PR-wide standardization to canonical
Tests/shell/paths.hooks/claude-notify/Tests/shell/e2e/run.sh (1)
7-7: Path casing update — LGTM.hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh (1)
17-35: > Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/claude-notify/scripts/check-shell-path-casing.sh`:
- Line 33: Add a POSIX-compliant trailing newline to the end of the script by
ensuring the final line containing echo "Shell path casing check passed." ends
with a newline character (i.e., add an empty line after that echo statement) so
the file ends with a newline.
In `@hooks/claude-notify/Tests/shell/run.sh`:
- Around line 7-8: The top-level shell orchestrator currently invokes
"$PROJECT_DIR/Tests/shell/integration/run.sh" and then
"$PROJECT_DIR/Tests/shell/e2e/run.sh", skipping unit tests; update the script to
invoke the unit tier first by inserting or moving a call to
"$PROJECT_DIR/Tests/shell/unit/run.sh" before the integration and e2e
invocations so tests run in the required order (unit → integration → e2e).
---
Nitpick comments:
In `@hooks/claude-notify/scripts/check-shell-path-casing.sh`:
- Around line 9-25: The search command is noisy when any of the hard-coded
targets (e.g., "$PROJECT_DIR/AGENTS.md", "$PROJECT_DIR/test-claude-notify.sh",
"$PROJECT_DIR/scripts") are missing; update the logic in
check-shell-path-casing.sh around the matches assignment so you either (a) build
a list/array of candidate paths by checking existence (e.g., test -e) and pass
only existing files/dirs to rg/grep, or (b) suppress missing-file warnings by
redirecting stderr (append 2>/dev/null) to the rg and grep invocations; keep the
use of PATTERN and PROJECT_DIR and preserve the grep -v exclusion for
scripts/check-shell-path-casing.sh.
In `@hooks/claude-notify/Tests/shell/unit/run.sh`:
- Line 5: Replace the temporary environment prefix in the PROJECT_DIR assignment
to silence ShellCheck SC1007 by using an explicit empty CDPATH value: change the
invocation that sets CDPATH before cd (the PROJECT_DIR="$(CDPATH= cd --
"$(dirname "$0")/../../.." && pwd)") to use CDPATH='' so the semantics remain
identical but the linter no longer flags it; update the PROJECT_DIR assignment
accordingly, leaving the rest of the command (cd -- "$(dirname "$0")/../../.."
&& pwd) unchanged.
In `@hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh`:
- Around line 90-93: The U020 test implicitly relies on the ARGS_LOG file
created by U019; to fix this, make U020 self-contained by ensuring ARGS_LOG
exists before calling assert_file_contains: either invoke the same setup that
creates ARGS_LOG (or create a blank ARGS_LOG file) after case_start "U020" or
add an explicit guard that checks for ARGS_LOG and fails with a clear message
(e.g., "U020 requires ARGS_LOG but file not found") so assert_file_contains (and
grep) never errors with a cryptic file-not-found; reference the test identifiers
case_start "U020", the ARGS_LOG variable, and the assert_file_contains
invocation when applying the change.
| exit 1 | ||
| fi | ||
|
|
||
| echo "Shell path casing check passed." |
There was a problem hiding this comment.
Missing trailing newline.
The file lacks a trailing newline after the final echo statement. POSIX text files should end with a newline.
Suggested fix
echo "Shell path casing check passed."
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Shell path casing check passed." | |
| echo "Shell path casing check passed." | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/claude-notify/scripts/check-shell-path-casing.sh` at line 33, Add a
POSIX-compliant trailing newline to the end of the script by ensuring the final
line containing echo "Shell path casing check passed." ends with a newline
character (i.e., add an empty line after that echo statement) so the file ends
with a newline.
There was a problem hiding this comment.
Verified on current head: /hooks/claude-notify/scripts/check-shell-path-casing.sh already ends with a trailing newline (POSIX-compliant). No additional code change was required for this nitpick.
There was a problem hiding this comment.
@giwankim, thank you for verifying! I appreciate you checking the current state of the file and confirming that it already has the POSIX-compliant trailing newline. No further action needed on this one.
🧠 Learnings used
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/*/+(*.sh|notify) : Hook entrypoints must be shell scripts (e.g., notify.sh) that accept JSON on stdin per the Claude hook payload format, or a message as $1
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: The claude-notify.app/ bundle skeleton must be committed. Binary (Contents/MacOS/) and code-sign (Contents/_CodeSignature/) directories must be gitignored and produced by make build
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Package.swift : Swift package for hooks must have two targets: NotifyCore (library with all core logic) and ClaudeNotify (executable with thin main.swift entry). Tests target NotifyCore
- clarify README maintenance notes by separating gates from shared shell helpers - clarify test-unit note as applying to both root and module invocations - refactor duplicated osascript fake writers into one parameterized helper - document notify-test-helpers dependency on testlib wait helpers - make U020 dependency on U019 explicit and assert E012 osascript invocation - normalize integration tmp-dir cleanup strategy to rely on trap
|
Addressed the nitpick items from review #3838982755 in commit fb47379 and pushed to this branch.\n\nImplemented:\n- README: clarified that |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
hooks/claude-notify/Tests/shell/integration/test-deterministic.sh (1)
13-13:TEST_TMP_DIRSis dead code after the cleanup refactor.The variable is assigned but never read —
cleanup()now delegates tocleanup_registered_tmp_dirsand per-case dirs are allocated viamake_case_tmp_dir, so this declaration serves no purpose.♻️ Proposed removal
EXPECTED_NOTIFY_NAME=$(basename "$NOTIFY") export CLAUDE_NOTIFY_PID_FILE="$PID_FILE" -TEST_TMP_DIRS=""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/Tests/shell/integration/test-deterministic.sh` at line 13, The TEST_TMP_DIRS variable assignment is dead code after the cleanup refactor; remove the unused declaration "TEST_TMP_DIRS=\"\"" from the script so the file no longer defines an unused variable. Locate the line containing TEST_TMP_DIRS and delete it; ensure no other code references TEST_TMP_DIRS and that cleanup() now uses cleanup_registered_tmp_dirs and per-case tmp dirs are created via make_case_tmp_dir.hooks/claude-notify/Tests/shell/e2e/test-runtime.sh (2)
213-236: E011: osascript payload content is not validated, only file existence.In E007 you verify the payload content with
grep -q '\[3\]=post-test', but in E011 (line 227) you only check whether$OSASCRIPT_LOGexists. If the fallback invoked osascript for an unrelated reason (or an empty invocation), the test would still pass. Consider adding a content assertion (e.g., checking for the expected message substring) for stronger verification.💡 Proposed stronger assertion
-if wait_for_file "$OSASCRIPT_LOG" 20 >/dev/null 2>&1; then +if wait_for_file "$OSASCRIPT_LOG" 20 >/dev/null 2>&1 \ + && grep -q '\[3\]=sender-required-forced-failure' "$OSASCRIPT_LOG"; then pass "E011" "required spoofed post failure uses deterministic AppleScript fallback" else fail "E011" "required spoofed post failure did not invoke deterministic AppleScript fallback"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/Tests/shell/e2e/test-runtime.sh` around lines 213 - 236, The test E011 currently only verifies OSASCRIPT_LOG exists—update the assertion to validate the AppleScript payload content by grepping OSASCRIPT_LOG for the expected message string (the -message "sender-required-forced-failure") instead of just checking file existence; modify the block that calls wait_for_file "$OSASCRIPT_LOG" and the subsequent conditional to assert the log contains the expected substring (use the same pattern approach as E007 which uses grep -q '\[3\]=post-test') referencing variables OSASCRIPT_LOG, FAKE_OSASCRIPT and the test marker "E011".
267-298: E013: fake-tmux output format should be documented or derived from a shared constant.The heredoc on lines 273–280 returns a pipe-delimited string (
sess|6|win|3|%61|client-e2e|/dev/ttys061) that must match the exact formatnotify.shexpects fromtmux display-message. If that format ever changes, this fake and the real tmux parsing will silently diverge, and the test will fail for non-obvious reasons.Consider extracting the format string into a shared variable or at minimum adding a comment referencing the expected
tmux display-message -pformat template used bynotify.sh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/Tests/shell/e2e/test-runtime.sh` around lines 267 - 298, The fake-tmux heredoc that emits sess|6|win|3|%61|client-e2e|/dev/ttys061 must match the format parsed by notify.sh's tmux display-message handling; extract that format into a shared constant or reference so both test and implementation use the same template (e.g., add a TMUX_DISPLAY_MSG_FORMAT constant or export a helper in the test helpers and use it when writing FAKE_TMUX), or at minimum add a clear comment above the heredoc referencing the exact display-message -p template string used by notify.sh (and update the fake to be generated from that constant/helper instead of a hardcoded string) so changes cannot drift.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
hooks/claude-notify/README.mdhooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Tests/shell/unit/run.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh
🧰 Additional context used
📓 Path-based instructions (1)
hooks/claude-notify/Tests/**/*.{swift,sh}
📄 CodeRabbit inference engine (AGENTS.md)
All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U016 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I128, e2e shell tests use E001–E011
Files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/Tests/shell/unit/run.sh
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Package.swift : Swift package for hooks must have two targets: NotifyCore (library with all core logic) and ClaudeNotify (executable with thin main.swift entry). Tests target NotifyCore
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T03:00:33.154Z
Learning: Applies to AGENTS.md : Document agent implementations and their capabilities in AGENTS.md
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T03:00:33.154Z
Learning: Applies to **/*agent*.{ts,tsx,js,jsx} : Maintain clear agent interface definitions for consistency across agent implementations
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Tests/**/*.{swift,sh} : All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U016 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I128, e2e shell tests use E001–E011
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/README.mdhooks/claude-notify/Tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/Tests/shell/unit/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Package.swift : Swift package for hooks must have two targets: NotifyCore (library with all core logic) and ClaudeNotify (executable with thin main.swift entry). Tests target NotifyCore
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/README.mdhooks/claude-notify/Tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/Tests/shell/unit/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/*/+(*.sh|notify) : Hook entrypoints must be shell scripts (e.g., notify.sh) that accept JSON on stdin per the Claude hook payload format, or a message as $1
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/README.mdhooks/claude-notify/Tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/unit/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Hook behavior is configured via NOTIFY_* environment variables. See hooks/claude-notify/README.md for the full list
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/README.mdhooks/claude-notify/Tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/Tests/shell/unit/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: The claude-notify.app/ bundle skeleton must be committed. Binary (Contents/MacOS/) and code-sign (Contents/_CodeSignature/) directories must be gitignored and produced by make build
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/README.mdhooks/claude-notify/Tests/shell/integration/test-deterministic.shhooks/claude-notify/Tests/shell/unit/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/{Sources,Tests}/**/*.swift : Swift docstring coverage threshold: ≥80% on Swift sources in Sources/ and Tests/. Each enum, struct, class, protocol, func, init declaration must have a preceding /// comment
Applied to files:
hooks/claude-notify/README.mdhooks/claude-notify/Tests/shell/unit/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: When hooks or skills are added, removed, or significantly changed, update README.md: add component section entry with name, description, and 2–3 key feature bullets; update Quick Start if new dependencies are introduced
Applied to files:
hooks/claude-notify/README.md
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Tests are tiered (unit → integration → e2e) with sequential execution in CI. Each tier gates the next. Integration tier includes both Swift XCTest (I001–I005) and shell tests (I101+), both run by make test-integration
Applied to files:
hooks/claude-notify/README.md
🧬 Code graph analysis (4)
hooks/claude-notify/Tests/shell/e2e/test-runtime.sh (2)
hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh (5)
kill_pid_from_file(121-138)cleanup_registered_tmp_dirs(19-23)make_case_tmp_dir(11-17)write_fake_osascript_success(81-84)drain_pid_file_if_present(140-149)hooks/claude-notify/Tests/shell/lib/testlib.sh (4)
pass(6-9)fail(11-14)wait_for_file(58-68)case_start(16-18)
hooks/claude-notify/README.md (1)
hooks/claude-notify/Sources/ClaudeNotify/main.swift (1)
args(311-370)
hooks/claude-notify/Tests/shell/integration/test-deterministic.sh (1)
hooks/claude-notify/Tests/shell/lib/testlib.sh (2)
case_start(16-18)wait_for_file(58-68)
hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh (4)
hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh (4)
cleanup_registered_tmp_dirs(19-23)make_case_tmp_dir(11-17)write_fake_notify(25-40)write_fake_frontmost_osascript(86-89)hooks/claude-notify/Tests/shell/integration/test-deterministic.sh (1)
cleanup(22-26)hooks/claude-notify/Tests/shell/lib/testlib.sh (4)
case_start(16-18)wait_for_file(58-68)fail(11-14)finish(70-73)hooks/claude-notify/notify.sh (1)
infer_tmux_activate_bundle_id(30-33)
🪛 Shellcheck (0.11.0)
hooks/claude-notify/Tests/shell/e2e/test-runtime.sh
[warning] 12-12: TEST_TMP_DIRS appears unused. Verify use (or export if used externally).
(SC2034)
hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh
[warning] 3-3: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 10-10: TEST_TMP_DIRS appears unused. Verify use (or export if used externally).
(SC2034)
hooks/claude-notify/Tests/shell/unit/run.sh
[warning] 5-5: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-shell
🔇 Additional comments (13)
hooks/claude-notify/Tests/shell/integration/test-deterministic.sh (3)
4-7: LGTM — path updates are consistent.Both
shellcheck source=directives correctly reflect the capitalizedTests/prefix introduced in this PR.
489-489: No action needed. All three helper functions (write_fake_osascript_success,write_failing_osascript,write_fake_frontmost_osascript) are defined inhooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh(lines 81, 91, 86 respectively) and test case IDs I122, I124, I128 are properly registered inTests/required-cases.txt.
813-838: I129 is already registered in Tests/required-cases.txt — no action required.The test case I129 is present in line 62 of Tests/required-cases.txt under the [integration] section and does not need to be registered. No changes to AGENTS.md or the manifest are necessary.
Likely an incorrect or invalid review comment.
hooks/claude-notify/Tests/shell/e2e/test-runtime.sh (3)
4-7: LGTM on the updated source paths.The shellcheck directives now correctly reference the canonical
Tests/shell/lib/paths, and the sourcing uses$PROJECT_DIRwhich resolves correctly (three directories up from this script's location).
12-13: Good: centralized cleanup via helpers.
TEST_TMP_DIRSis initialized here and consumed byregister_tmp_dir/cleanup_registered_tmp_dirsfrom the sourced helper, so the SC2034 shellcheck warning is a false positive.EXPECTED_NOTIFY_NAMEgateskill_pid_from_fileto avoid killing unrelated processes — solid safety measure.Also applies to: 16-20
138-163: E007: nicely structured deterministic fallback verification.The test now validates three things: exit code, AppleScript payload content (
[3]=post-test), and the stderr fallback success message. Thewait_for_fileguard before thegrepis appropriately defensive even though$NOTIFYruns synchronously.hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh (3)
1-15: LGTM — test setup and harness sourcing look correct.The
CDPATH=idiom on line 3 is a standard POSIX pattern to suppresscdCDPATH lookups (ShellCheck SC1007 is a false positive here).TEST_TMP_DIRSon line 10 is consumed by the sourcedcleanup_registered_tmp_dirshelper, so SC2034 is likewise a false positive.
17-35: U017: frontmost-app inference outside tmux — looks good.Clear setup with isolated tmp dir, fake scripts, and proper env-var passing for both
NOTIFY_ARGS_LOGandACTIVATE_OSASCRIPT_ARGS_LOG. Assertions verify both the exit code and that the osascript probe was called with the expected AppleScript expression.
90-97: U020 depends on U019'sARGS_LOG— dependency is explicit and guarded.The comment on line 91 and the
wait_for_fileguard on lines 92-94 make the dependency clear and provide a reasonable failure message. This is acceptable for sequential shell tests.One subtle point:
ARGS_LOGstill holds U019's tmp-dir path (set on line 64), and that directory is still alive because cleanup runs onEXIT. This works correctly in the current sequential flow but would break if tests were parallelized or reordered.hooks/claude-notify/Tests/shell/unit/run.sh (1)
1-7: LGTM — minimal and correct test runner.
set -eensures the runner propagates any test failure. TheCDPATH=idiom on line 5 is intentional (ShellCheck SC1007 is a false positive).hooks/claude-notify/README.md (3)
7-7: LGTM — documentation accurately reflects the new shell test infrastructure.The updated descriptions for test coverage scope (
Swift + shell), directory layout, andmake test-unitbehavior are clear and consistent with the PR changes.Also applies to: 14-14, 29-30
75-76: LGTM — new environment variable documentation is clear.
NOTIFY_ACTIVATE_BUNDLE_IDauto-inference default,NOTIFY_ACTIVATE_OSASCRIPT_BIN, and the debug log variables are well-described and consistent with the code. Based on learnings: "Hook behavior is configured via NOTIFY_* environment variables. See hooks/claude-notify/README.md for the full list."Also applies to: 79-80
94-96: LGTM — troubleshooting and maintenance sections updated appropriately.The activation inference behavior (tmux vs. non-tmux) is clearly explained, and the shared shell test helper documentation provides useful pointers for contributors.
Also applies to: 116-118
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/claude-notify/Tests/shell/e2e/test-runtime.sh`:
- Around line 238-298: Update the coding-guidelines text that currently lists
e2e shell test IDs as "E001–E011" to match Tests/required-cases.txt by changing
it to "E001–E013"; likewise correct the unit tests range "U001–U016" to
"U001–U020" and the integration shell tests range "I101–I128" to "I101–I129" so
the documented ranges align with the registered IDs (search for the guideline
headings that mention E001–E011, U001–U016, and I101–I128 and update those
numeric ranges accordingly).
- Around line 238-265: The test case ID "E012" is outside the allowed e2e shell
test range (E001–E011); rename the case to an unused ID within that range (e.g.,
change "E012" to "E011" or another free E00x) wherever it appears in this test
(the case_start call, any pass/fail messages, rc checks, and variables that
include the ID) and update Tests/required-cases.txt to list the new ID instead
of E012; ensure the new ID is unique and update any other test helpers or
references that expect the original ID.
In `@hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh`:
- Around line 17-97: The test IDs U017–U020 in test-notify-activate.sh violate
the documented U001–U016 guideline; either update the guideline ceiling or
rename these tests to valid IDs and update references: change every occurrence
of "U017"/"U018"/"U019"/"U020" in the file (the case_start calls and
TMP_DIR/make_case_tmp_dir usages) and in Tests/required-cases.txt (and any other
references) to unused IDs within U001–U016, and ensure dependent
waits/asserts/log file names remain consistent after renaming.
- Around line 59-88: U019's test invocation omits ACTIVATE_OSASCRIPT_ARGS_LOG
which prevents the fake osascript (created by write_fake_frontmost_osascript /
FAKE_ACTIVATE_OSASCRIPT) from logging its args; add
ACTIVATE_OSASCRIPT_ARGS_LOG="$ARGS_LOG" to the inline environment variables
passed to "$SCRIPT" in the U019 case (the same way U017/U018 do) so the fake
activation script receives and logs its arguments for future assertions.
---
Nitpick comments:
In `@hooks/claude-notify/Tests/shell/e2e/test-runtime.sh`:
- Around line 213-236: The test E011 currently only verifies OSASCRIPT_LOG
exists—update the assertion to validate the AppleScript payload content by
grepping OSASCRIPT_LOG for the expected message string (the -message
"sender-required-forced-failure") instead of just checking file existence;
modify the block that calls wait_for_file "$OSASCRIPT_LOG" and the subsequent
conditional to assert the log contains the expected substring (use the same
pattern approach as E007 which uses grep -q '\[3\]=post-test') referencing
variables OSASCRIPT_LOG, FAKE_OSASCRIPT and the test marker "E011".
- Around line 267-298: The fake-tmux heredoc that emits
sess|6|win|3|%61|client-e2e|/dev/ttys061 must match the format parsed by
notify.sh's tmux display-message handling; extract that format into a shared
constant or reference so both test and implementation use the same template
(e.g., add a TMUX_DISPLAY_MSG_FORMAT constant or export a helper in the test
helpers and use it when writing FAKE_TMUX), or at minimum add a clear comment
above the heredoc referencing the exact display-message -p template string used
by notify.sh (and update the fake to be generated from that constant/helper
instead of a hardcoded string) so changes cannot drift.
In `@hooks/claude-notify/Tests/shell/integration/test-deterministic.sh`:
- Line 13: The TEST_TMP_DIRS variable assignment is dead code after the cleanup
refactor; remove the unused declaration "TEST_TMP_DIRS=\"\"" from the script so
the file no longer defines an unused variable. Locate the line containing
TEST_TMP_DIRS and delete it; ensure no other code references TEST_TMP_DIRS and
that cleanup() now uses cleanup_registered_tmp_dirs and per-case tmp dirs are
created via make_case_tmp_dir.
| case_start "E012" "notify.sh outside tmux logs activate inference path" | ||
| TMP_DIR=$(make_case_tmp_dir "E012") | ||
| FAKE_OSASCRIPT="$TMP_DIR/fake-osascript.sh" | ||
| OSASCRIPT_LOG="$TMP_DIR/osascript-args.log" | ||
| DEBUG_LOG="$TMP_DIR/notify-debug.log" | ||
| write_fake_osascript_success "$FAKE_OSASCRIPT" | ||
| TMUX="" NOTIFY_DEBUG_LOG="$DEBUG_LOG" NOTIFY_ACTIVATE_BUNDLE_ID="" \ | ||
| NOTIFY_ACTIVATE_OSASCRIPT_BIN="$FAKE_OSASCRIPT" OSASCRIPT_ARGS_LOG="$OSASCRIPT_LOG" CLAUDE_NOTIFY_TEST_SKIP_DELIVERY=1 NOTIFY_TIMEOUT=1 \ | ||
| "$SCRIPT" "e2e outside tmux inference test" 2>/dev/null | ||
| rc=$? | ||
| drain_pid_file_if_present "$PID_FILE" 30 "$EXPECTED_NOTIFY_NAME" | ||
| if [ "$rc" -eq 0 ]; then | ||
| pass "E012" "notify.sh outside-tmux inference probe exits 0" | ||
| else | ||
| fail "E012" "notify.sh outside-tmux inference probe exited $rc" | ||
| fi | ||
| if wait_for_file "$OSASCRIPT_LOG" 20 >/dev/null 2>&1 \ | ||
| && grep -q 'id of app (path to frontmost application as text)' "$OSASCRIPT_LOG"; then | ||
| pass "E012" "notify.sh outside-tmux inference probe invoked deterministic frontmost-app osascript" | ||
| else | ||
| fail "E012" "notify.sh outside-tmux inference probe missing deterministic frontmost-app osascript invocation" | ||
| fi | ||
| if grep -q "inferred activate bundle id outside tmux: com.jetbrains.intellij" "$DEBUG_LOG" \ | ||
| && grep -q "notify_run activate_override=com.jetbrains.intellij activate_bundle=com.jetbrains.intellij" "$DEBUG_LOG"; then | ||
| pass "E012" "notify.sh outside-tmux debug log records inferred native activate path" | ||
| else | ||
| fail "E012" "notify.sh outside-tmux debug log missing inferred native activate path" | ||
| fi | ||
|
|
||
| case_start "E013" "notify.sh tmux mode logs execute payload inference path" | ||
| TMP_DIR=$(make_case_tmp_dir "E013") | ||
| FAKE_OSASCRIPT="$TMP_DIR/fake-osascript.sh" | ||
| FAKE_TMUX="$TMP_DIR/fake-tmux.sh" | ||
| DEBUG_LOG="$TMP_DIR/notify-debug.log" | ||
| write_fake_osascript_success "$FAKE_OSASCRIPT" | ||
| cat > "$FAKE_TMUX" <<'FAKE_TMUX_E013' | ||
| #!/bin/sh | ||
| if [ "$1" = "display-message" ]; then | ||
| printf '%s\n' "sess|6|win|3|%61|client-e2e|/dev/ttys061" | ||
| exit 0 | ||
| fi | ||
| exit 0 | ||
| FAKE_TMUX_E013 | ||
| chmod +x "$FAKE_TMUX" | ||
| TMUX="/tmp/fake-socket,661,0" TMUX_PANE="%61" NOTIFY_DEBUG_LOG="$DEBUG_LOG" NOTIFY_ACTIVATE_BUNDLE_ID="" \ | ||
| NOTIFY_ACTIVATE_OSASCRIPT_BIN="$FAKE_OSASCRIPT" NOTIFY_TMUX_BIN="$FAKE_TMUX" CLAUDE_NOTIFY_TEST_SKIP_DELIVERY=1 NOTIFY_TIMEOUT=1 \ | ||
| "$SCRIPT" "e2e tmux inference test" 2>/dev/null | ||
| rc=$? | ||
| drain_pid_file_if_present "$PID_FILE" 30 "$EXPECTED_NOTIFY_NAME" | ||
| if [ "$rc" -eq 0 ]; then | ||
| pass "E013" "notify.sh tmux inference probe exits 0" | ||
| else | ||
| fail "E013" "notify.sh tmux inference probe exited $rc" | ||
| fi | ||
| if grep -q "inferred activate bundle id in tmux mode: com.jetbrains.intellij" "$DEBUG_LOG" \ | ||
| && grep -q "notify_run activate_override=<empty> activate_bundle=com.jetbrains.intellij" "$DEBUG_LOG" \ | ||
| && grep -q "execute payload prepared .*activate_bundle=com.jetbrains.intellij" "$DEBUG_LOG"; then | ||
| pass "E013" "notify.sh tmux debug log records execute payload inference path" | ||
| else | ||
| fail "E013" "notify.sh tmux debug log missing execute payload inference path" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and display the contents of required-cases.txt
echo "--- Searching for required-cases.txt ---"
fd -t f 'required-cases.txt' --full-path hooks/claude-notify
# If found, display its contents
echo ""
echo "--- Contents of required-cases.txt ---"
fd -t f 'required-cases.txt' --full-path hooks/claude-notify --exec cat {} \;
# Check specifically for E012 and E013
echo ""
echo "--- Entries for E012 and E013 ---"
fd -t f 'required-cases.txt' --full-path hooks/claude-notify --exec grep 'E01[23]' {} \;Repository: giwankim/my-claude-code
Length of output: 837
Update the coding guidelines to reflect the actual e2e test ID range.
The coding guidelines state that e2e shell tests use E001–E011, but Tests/required-cases.txt already registers E012 and E013 in the [e2e] section. Update the guidelines to reflect the current range E001–E013 (or adjust further if additional test IDs are planned beyond E013).
Note: The unit tests guideline also shows a mismatch (states U001–U016 but required-cases.txt has U001–U020), and integration shell tests have a similar issue (states I101–I128 but required-cases.txt has I101–I129).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/claude-notify/Tests/shell/e2e/test-runtime.sh` around lines 238 - 298,
Update the coding-guidelines text that currently lists e2e shell test IDs as
"E001–E011" to match Tests/required-cases.txt by changing it to "E001–E013";
likewise correct the unit tests range "U001–U016" to "U001–U020" and the
integration shell tests range "I101–I128" to "I101–I129" so the documented
ranges align with the registered IDs (search for the guideline headings that
mention E001–E011, U001–U016, and I101–I128 and update those numeric ranges
accordingly).
| case_start "E012" "notify.sh outside tmux logs activate inference path" | ||
| TMP_DIR=$(make_case_tmp_dir "E012") | ||
| FAKE_OSASCRIPT="$TMP_DIR/fake-osascript.sh" | ||
| OSASCRIPT_LOG="$TMP_DIR/osascript-args.log" | ||
| DEBUG_LOG="$TMP_DIR/notify-debug.log" | ||
| write_fake_osascript_success "$FAKE_OSASCRIPT" | ||
| TMUX="" NOTIFY_DEBUG_LOG="$DEBUG_LOG" NOTIFY_ACTIVATE_BUNDLE_ID="" \ | ||
| NOTIFY_ACTIVATE_OSASCRIPT_BIN="$FAKE_OSASCRIPT" OSASCRIPT_ARGS_LOG="$OSASCRIPT_LOG" CLAUDE_NOTIFY_TEST_SKIP_DELIVERY=1 NOTIFY_TIMEOUT=1 \ | ||
| "$SCRIPT" "e2e outside tmux inference test" 2>/dev/null | ||
| rc=$? | ||
| drain_pid_file_if_present "$PID_FILE" 30 "$EXPECTED_NOTIFY_NAME" | ||
| if [ "$rc" -eq 0 ]; then | ||
| pass "E012" "notify.sh outside-tmux inference probe exits 0" | ||
| else | ||
| fail "E012" "notify.sh outside-tmux inference probe exited $rc" | ||
| fi | ||
| if wait_for_file "$OSASCRIPT_LOG" 20 >/dev/null 2>&1 \ | ||
| && grep -q 'id of app (path to frontmost application as text)' "$OSASCRIPT_LOG"; then | ||
| pass "E012" "notify.sh outside-tmux inference probe invoked deterministic frontmost-app osascript" | ||
| else | ||
| fail "E012" "notify.sh outside-tmux inference probe missing deterministic frontmost-app osascript invocation" | ||
| fi | ||
| if grep -q "inferred activate bundle id outside tmux: com.jetbrains.intellij" "$DEBUG_LOG" \ | ||
| && grep -q "notify_run activate_override=com.jetbrains.intellij activate_bundle=com.jetbrains.intellij" "$DEBUG_LOG"; then | ||
| pass "E012" "notify.sh outside-tmux debug log records inferred native activate path" | ||
| else | ||
| fail "E012" "notify.sh outside-tmux debug log missing inferred native activate path" | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if E012 is registered in Tests/required-cases.txt
cat hooks/claude-notify/Tests/required-cases.txt 2>/dev/null || echo "File not found"Repository: giwankim/my-claude-code
Length of output: 666
🏁 Script executed:
# Search for how notify.sh infers the IntelliJ bundle ID
fd -t f 'notify.sh' hooks/claude-notify --exec cat {} \; | grep -n -i 'intellij\|infer\|bundle' | head -50Repository: giwankim/my-claude-code
Length of output: 1698
🏁 Script executed:
# Check for documentation of E012 and environment prerequisites
fd -t f 'README\|AGENTS\|required-cases' hooks/claude-notify --exec cat {} \;Repository: giwankim/my-claude-code
Length of output: 49
🏁 Script executed:
# Extract the infer_frontmost_activate_bundle_id function from notify.sh
fd -t f 'notify.sh' hooks/claude-notify --exec sed -n '30,50p' {} \;Repository: giwankim/my-claude-code
Length of output: 933
🏁 Script executed:
# Get more context around the inference function to understand how it determines the bundle
fd -t f 'notify.sh' hooks/claude-notify --exec sed -n '1,100p' {} \;Repository: giwankim/my-claude-code
Length of output: 4055
E012 test case ID violates documented range for e2e shell tests.
According to the coding guidelines, e2e shell tests must use the ID range E001–E011, but E012 is currently registered in Tests/required-cases.txt. Either this test should be renumbered to fall within the valid range, or the guidelines should be updated to reflect the expanded range.
The inference logic itself is sound: infer_frontmost_activate_bundle_id() calls osascript to query the frontmost application's bundle ID. The test correctly mocks this with FAKE_OSASCRIPT, so the test setup properly isolates the behavior and should not produce spurious failures in different terminal environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/claude-notify/Tests/shell/e2e/test-runtime.sh` around lines 238 - 265,
The test case ID "E012" is outside the allowed e2e shell test range (E001–E011);
rename the case to an unused ID within that range (e.g., change "E012" to "E011"
or another free E00x) wherever it appears in this test (the case_start call, any
pass/fail messages, rc checks, and variables that include the ID) and update
Tests/required-cases.txt to list the new ID instead of E012; ensure the new ID
is unique and update any other test helpers or references that expect the
original ID.
There was a problem hiding this comment.
Re-verified: E012/E013 are intentionally valid in this PR. Current documented e2e range is E001–E013 and Tests/required-cases.txt registers E012 and E013. No ID rename is required.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of Tests/required-cases.txt to verify E012 and E013
cat hooks/claude-notify/Tests/required-cases.txt
echo "---"
echo "Checking for documentation of e2e test ranges:"
echo "---"
# Check AGENTS.md for e2e range documentation
fd -t f 'AGENTS.md' hooks/claude-notify --exec grep -n -i 'e001\|e2e\|range' {} \; 2>/dev/null || echo "No AGENTS.md found or no range mentions"
Length of output: 944
@giwankim, you're absolutely right—I apologize for the incorrect comment. The verification confirms that:
- Tests/required-cases.txt registers both E012 and E013 in the
[e2e]section - hooks/claude-notify/AGENTS.md documents the e2e range as E001–E013 (line 68)
My comment was based on an outdated learning that stated the range was E001–E011. The test IDs E012 and E013 are valid and intentional additions in this PR.
Would you like me to remove the outdated learning (ID: 6b000e8a-83c9-4931-9092-697a868c0f07) and replace it with the updated range E001–E013?
🧬 Code Graph Analysis Results
File: hooks/claude-notify/Tests/shell/lib/testlib.sh (relevant snippet)
At line range 16-18:
case_start() {
printf 'Case %s: %s\n' "$1" "$2"
}
At line range 20-30:
wait_for_pid_file() {
pid_file="$1"
max_tries="${2:-50}"
i=0
while [ "$i" -lt "$max_tries" ]; do
[ -s "$pid_file" ] && return 0
sleep 0.1
i=$((i + 1))
done
return 1
}
At line range 44-56:
wait_for_process_exit() {
pid="$1"
max_tries="${2:-50}"
i=0
while [ "$i" -lt "$max_tries" ]; do
if ! kill -0 "$pid" 2>/dev/null; then
return 0
fi
sleep 0.1
i=$((i + 1))
done
return 1
}
At line range 32-42:
wait_for_pid_removed() {
pid_file="$1"
max_tries="${2:-50}"
i=0
while [ "$i" -lt "$max_tries" ]; do
[ ! -e "$pid_file" ] && return 0
sleep 0.1
i=$((i + 1))
done
return 1
}
At line range 58-68:
wait_for_file() {
file="$1"
max_tries="${2:-30}"
i=0
while [ "$i" -lt "$max_tries" ]; do
[ -f "$file" ] && return 0
sleep 0.1
i=$((i + 1))
done
return 1
}
At line range 70-73:
finish() {
printf '\nResults: %d passed, %d failed\n' "$PASS" "$FAIL"
[ "$FAIL" -eq 0 ]
}
File: hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh (relevant snippet)
At line range 121-138:
kill_pid_from_file() {
pid_file="$1"
expected_name="${2:-}"
[ -f "$pid_file" ] || return 1
pid=$(cat "$pid_file" 2>/dev/null)
case "$pid" in
''|*[!0-9]*)
return 1
;;
esac
[ "$pid" -gt 0 ] 2>/dev/null || return 1
if [ -n "$expected_name" ]; then
kill -0 "$pid" 2>/dev/null || return 1
proc=$(ps -p "$pid" -o comm= 2>/dev/null | awk 'NR==1 {print $1}')
[ "$proc" = "$expected_name" ] || return 1
fi
kill "$pid" 2>/dev/null
}
At line range 140-149:
drain_pid_file_if_present() {
pid_file="$1"
max_tries="${2:-20}"
expected_name="${3:-}"
# Depends on wait_for_pid_file and wait_for_pid_removed from testlib.sh.
if wait_for_pid_file "$pid_file" "$max_tries"; then
kill_pid_from_file "$pid_file" "$expected_name" >/dev/null 2>&1 || true
wait_for_pid_removed "$pid_file" "$max_tries" >/dev/null 2>&1 || true
fi
}
File: hooks/claude-notify/Tests/shell/lib/testlib.sh (additional relevant helper)
At line range 11-17:
make_case_tmp_dir() {
case_id="$1"
dir="/tmp/claude-notify-test-${case_id}.$$"
register_tmp_dir "$dir"
mkdir -p "$dir"
printf '%s\n' "$dir"
}
At line range 19-23:
cleanup_registered_tmp_dirs() {
for dir in $TEST_TMP_DIRS; do
rm -rf "$dir"
done
}
File: hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh (relevant snippet)
At line range 12-14:
cleanup() {
cleanup_registered_tmp_dirs
}
File: hooks/claude-notify/Tests/shell/integration/test-deterministic.sh (relevant snippet)
At line range 22-26:
cleanup() {
kill_pid_from_file "$PID_FILE" "$EXPECTED_NOTIFY_NAME" >/dev/null 2>&1 || true
rm -f "$PID_FILE" "$RELAUNCH_MARKER"
cleanup_registered_tmp_dirs
}
🧠 Learnings used
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Tests/**/*.{swift,sh} : All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U016 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I128, e2e shell tests use E001–E011
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/*/+(*.sh|notify) : Hook entrypoints must be shell scripts (e.g., notify.sh) that accept JSON on stdin per the Claude hook payload format, or a message as $1
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Package.swift : Swift package for hooks must have two targets: NotifyCore (library with all core logic) and ClaudeNotify (executable with thin main.swift entry). Tests target NotifyCore
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Hook behavior is configured via NOTIFY_* environment variables. See hooks/claude-notify/README.md for the full list
There was a problem hiding this comment.
🧩 Analysis chain
Let me search for the actual documentation instead:
🏁 Script executed:
#!/bin/bash
# Check the AGENTS.md documentation for the e2e test range
cat hooks/claude-notify/AGENTS.md | grep -i -A 5 -B 5 'e2e\|E001\|E011\|E012\|E013'
Length of output: 1361
🏁 Script executed:
#!/bin/bash
# Check Tests/required-cases.txt to see which e2e cases are registered
cat hooks/claude-notify/Tests/required-cases.txt
Length of output: 599
🏁 Script executed:
#!/bin/bash
# Check if the outdated learning range E001-E011 appears in any documentation
rg -i 'E001.*E011' hooks/claude-notify/
Length of output: 0
@giwankim, you're absolutely right—I apologize for the incorrect comment. The documentation in hooks/claude-notify/AGENTS.md clearly states the e2e shell test range is E001–E013, and both E012 and E013 are properly registered in Tests/required-cases.txt. Thank you for the clarification!
🧬 Code Graph Analysis Results
-
File: hooks/claude-notify/Tests/shell/lib/testlib.sh
-
Lines 16-18
-
Snippet:
case_start() { printf 'Case %s: %s\n' "$1" "$2" } -
Lines 20-30
-
Snippet:
wait_for_pid_file() { pid_file="$1" max_tries="${2:-50}" i=0 while [ "$i" -lt "$max_tries" ]; do [ -s "$pid_file" ] && return 0 sleep 0.1 i=$((i + 1)) done return 1 } -
Lines 44-56
-
Snippet:
wait_for_process_exit() { pid="$1" max_tries="${2:-50}" i=0 while [ "$i" -lt "$max_tries" ]; do if ! kill -0 "$pid" 2>/dev/null; then return 0 fi sleep 0.1 i=$((i + 1)) done return 1 } -
Lines 32-42
-
Snippet:
wait_for_pid_removed() { pid_file="$1" max_tries="${2:-50}" i=0 while [ "$i" -lt "$max_tries" ]; do [ ! -e "$pid_file" ] && return 0 sleep 0.1 i=$((i + 1)) done return 1 } -
Lines 58-68
-
Snippet:
wait_for_file() { file="$1" max_tries="${2:-30}" i=0 while [ "$i" -lt "$max_tries" ]; do [ -f "$file" ] && return 0 sleep 0.1 i=$((i + 1)) done return 1 } -
Lines 70-73
-
Snippet:
finish() { printf '\nResults: %d passed, %d failed\n' "$PASS" "$FAIL" [ "$FAIL" -eq 0 ] }
-
-
File: hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh
-
Lines 121-138
-
Snippet:
kill_pid_from_file() { pid_file="$1" expected_name="${2:-}" [ -f "$pid_file" ] || return 1 pid=$(cat "$pid_file" 2>/dev/null) case "$pid" in ''|*[!0-9]*) return 1 ;; esac [ "$pid" -gt 0 ] 2>/dev/null || return 1 if [ -n "$expected_name" ]; then kill -0 "$pid" 2>/dev/null || return 1 proc=$(ps -p "$pid" -o comm= 2>/dev/null | awk 'NR==1 {print $1}') [ "$proc" = "$expected_name" ] || return 1 fi kill "$pid" 2>/dev/null } -
Lines 140-149
-
Snippet:
drain_pid_file_if_present() { pid_file="$1" max_tries="${2:-20}" expected_name="${3:-}" # Depends on wait_for_pid_file and wait_for_pid_removed from testlib.sh. if wait_for_pid_file "$pid_file" "$max_tries"; then kill_pid_from_file "$pid_file" "$expected_name" >/dev/null 2>&1 || true wait_for_pid_removed "$pid_file" "$max_tries" >/dev/null 2>&1 || true fi }
-
-
File: hooks/claude-notify/Tests/shell/lib/testlib.sh
- Lines 11-17
- Snippet:
make_case_tmp_dir() { case_id="$1" dir="/tmp/claude-notify-test-${case_id}.$$" register_tmp_dir "$dir" mkdir -p "$dir" printf '%s\n' "$dir" }
-
File: hooks/claude-notify/Tests/shell/runtime/test-runtime.sh (embedded references)
- Lines 81-84
- Snippet:
write_fake_osascript_success "$FAKE_OSASCRIPT" - (Note: This appears in the E007/E012/E013 contexts as part of the setup for deterministic AppleScript fallback behavior.)
-
File: hooks/claude-notify/Tests/shell/integration/test-deterministic.sh (contextual relevance)
- Lines 22-26
- Snippet:
cleanup() { kill_pid_from_file "$PID_FILE" "$EXPECTED_NOTIFY_NAME" >/dev/null 2>&1 || true rm -f "$PID_FILE" "$RELAUNCH_MARKER" cleanup_registered_tmp_dirs }
✏️ Learnings added
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 7
File: hooks/claude-notify/Tests/shell/e2e/test-runtime.sh:238-265
Timestamp: 2026-02-23T07:28:09.415Z
Learning: Applies to hooks/claude-notify/Tests/**/*.{swift,sh}: All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U020 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I129, e2e shell tests use E001–E013.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
⛔ Learnings removed
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Tests/**/*.{swift,sh} : All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U016 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I128, e2e shell tests use E001–E011
🧠 Learnings used
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Tests/**/*.{swift,sh} : All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U016 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I128, e2e shell tests use E001–E011
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/*/+(*.sh|notify) : Hook entrypoints must be shell scripts (e.g., notify.sh) that accept JSON on stdin per the Claude hook payload format, or a message as $1
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Package.swift : Swift package for hooks must have two targets: NotifyCore (library with all core logic) and ClaudeNotify (executable with thin main.swift entry). Tests target NotifyCore
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Hook behavior is configured via NOTIFY_* environment variables. See hooks/claude-notify/README.md for the full list
…uce drift risk - run shell orchestrator tiers in order: unit -> integration -> e2e - pass ACTIVATE_OSASCRIPT_ARGS_LOG in U019 for consistent activation test wiring - strengthen E011 to assert AppleScript fallback payload content - add shared tmux display-message format/payload helpers for shell tests - extract notify.sh tmux display-message parser format into a named constant and use it in E013 drift checks
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
hooks/claude-notify/Tests/shell/run.sh (1)
7-9: Tier ordering is correct — LGTM.Unit → integration → e2e sequencing is now enforced, matching the project's gating model. This resolves the previously flagged gap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/Tests/shell/run.sh` around lines 7 - 9, The sequencing of unit → integration → e2e scripts is correct, but to enforce failure propagation add a strict-fail guard (e.g., set -euo pipefail) at the top of the wrapper run.sh and check each child script's exit status; ensure the wrapper invokes unit/run.sh, integration/run.sh, and e2e/run.sh in that order and exits immediately if any returns non-zero so downstream stages are not executed after a failure.
🧹 Nitpick comments (4)
hooks/claude-notify/Tests/shell/run.sh (1)
1-3: Consider enablingset -ualongsideset -e.The script currently only exits on command failure. Adding
-uwill also abort on any unset-variable expansion, which is cheap insurance for future edits.♻️ Proposed change
-set -e +set -eu🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/Tests/shell/run.sh` around lines 1 - 3, The script currently uses "set -e" which only exits on command failure; update the shell options in the top-level script (the shebang/run script) to enable unset-variable detection as well by adding the -u flag (e.g., change the existing set -e invocation in run.sh to use -u too, such as "set -eu" or equivalent) so the script aborts on unset variable expansions as well.hooks/claude-notify/Tests/shell/e2e/test-runtime.sh (2)
241-244: Consider usingwrite_fake_frontmost_osascript+ACTIVATE_OSASCRIPT_ARGS_LOGfor semantic consistency with E011/U017/U018.E012 passes this fake as
NOTIFY_ACTIVATE_OSASCRIPT_BIN(the activation inference path), yet useswrite_fake_osascript_success(which readsOSASCRIPT_ARGS_LOG) rather thanwrite_fake_frontmost_osascript(which readsACTIVATE_OSASCRIPT_ARGS_LOG). The test is functionally correct —OSASCRIPT_ARGS_LOG="$OSASCRIPT_LOG"is explicitly threaded through — but the log-variable naming diverges from every other call site that exercises the same path.♻️ Proposed fix
-FAKE_OSASCRIPT="$TMP_DIR/fake-osascript.sh" -OSASCRIPT_LOG="$TMP_DIR/osascript-args.log" -DEBUG_LOG="$TMP_DIR/notify-debug.log" -write_fake_osascript_success "$FAKE_OSASCRIPT" -TMUX="" NOTIFY_DEBUG_LOG="$DEBUG_LOG" NOTIFY_ACTIVATE_BUNDLE_ID="" \ - NOTIFY_ACTIVATE_OSASCRIPT_BIN="$FAKE_OSASCRIPT" OSASCRIPT_ARGS_LOG="$OSASCRIPT_LOG" CLAUDE_NOTIFY_TEST_SKIP_DELIVERY=1 NOTIFY_TIMEOUT=1 \ - "$SCRIPT" "e2e outside tmux inference test" 2>/dev/null +FAKE_ACTIVATE_OSASCRIPT="$TMP_DIR/fake-activate-osascript.sh" +ACTIVATE_OSASCRIPT_LOG="$TMP_DIR/activate-osascript-args.log" +DEBUG_LOG="$TMP_DIR/notify-debug.log" +write_fake_frontmost_osascript "$FAKE_ACTIVATE_OSASCRIPT" +TMUX="" NOTIFY_DEBUG_LOG="$DEBUG_LOG" NOTIFY_ACTIVATE_BUNDLE_ID="" \ + NOTIFY_ACTIVATE_OSASCRIPT_BIN="$FAKE_ACTIVATE_OSASCRIPT" ACTIVATE_OSASCRIPT_ARGS_LOG="$ACTIVATE_OSASCRIPT_LOG" CLAUDE_NOTIFY_TEST_SKIP_DELIVERY=1 NOTIFY_TIMEOUT=1 \ + "$SCRIPT" "e2e outside tmux inference test" 2>/dev/nullUpdate the downstream assertions to reference
$ACTIVATE_OSASCRIPT_LOGin place of$OSASCRIPT_LOG.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/Tests/shell/e2e/test-runtime.sh` around lines 241 - 244, The test uses write_fake_osascript_success and OSASCRIPT_ARGS_LOG while exercising the activation-inference path via NOTIFY_ACTIVATE_OSASCRIPT_BIN; change to write_fake_frontmost_osascript and swap usages of OSASCRIPT_ARGS_LOG/OSASCRIPT_LOG to the activation-specific names ACTIVATE_OSASCRIPT_ARGS_LOG/ACTIVATE_OSASCRIPT_LOG so the fake script and its log variable match the semantic path (update the call to write_fake_frontmost_osascript "$FAKE_OSASCRIPT" and any downstream assertions that currently reference $OSASCRIPT_LOG to reference $ACTIVATE_OSASCRIPT_LOG instead).
12-12: Silence the spurious SC2034 warning onTEST_TMP_DIRS.ShellCheck can't trace the variable through the sourced helper, so it fires SC2034. A suppression directive avoids CI noise without hiding real issues.
♻️ Proposed fix
-TEST_TMP_DIRS="" +# shellcheck disable=SC2034 # used by register_tmp_dir / cleanup_registered_tmp_dirs in notify-test-helpers.sh +TEST_TMP_DIRS=""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/Tests/shell/e2e/test-runtime.sh` at line 12, Add a ShellCheck suppression for SC2034 immediately above the TEST_TMP_DIRS assignment to silence the false positive; insert a comment line "# shellcheck disable=SC2034" just before the "TEST_TMP_DIRS=\"\"" declaration so the linter ignores the unused-variable warning for TEST_TMP_DIRS while leaving the assignment intact.hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh (1)
10-10: Silence the spurious SC2034 warning onTEST_TMP_DIRS.Same root cause as the identical warning in
test-runtime.sh: ShellCheck can't see thatregister_tmp_dir(in the sourced helper) appends to this variable.♻️ Proposed fix
-TEST_TMP_DIRS="" +# shellcheck disable=SC2034 # used by register_tmp_dir / cleanup_registered_tmp_dirs in notify-test-helpers.sh +TEST_TMP_DIRS=""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh` at line 10, Silence the ShellCheck SC2034 warning for TEST_TMP_DIRS by marking the assignment as intentionally unused: add a ShellCheck directive above the TEST_TMP_DIRS declaration (e.g. a comment like "# shellcheck disable=SC2034") so ShellCheck knows the variable is populated externally by the sourced helper (register_tmp_dir); keep the variable name TEST_TMP_DIRS and the empty assignment intact.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Tests/shell/run.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/notify.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/claude-notify/notify.sh
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-shell
🧰 Additional context used
📓 Path-based instructions (1)
hooks/claude-notify/Tests/**/*.{swift,sh}
📄 CodeRabbit inference engine (AGENTS.md)
All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U016 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I128, e2e shell tests use E001–E011
Files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Tests/shell/run.sh
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Package.swift : Swift package for hooks must have two targets: NotifyCore (library with all core logic) and ClaudeNotify (executable with thin main.swift entry). Tests target NotifyCore
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T03:00:33.154Z
Learning: Applies to AGENTS.md : Document agent implementations and their capabilities in AGENTS.md
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T03:00:33.154Z
Learning: Applies to **/*agent*.{ts,tsx,js,jsx} : Maintain clear agent interface definitions for consistency across agent implementations
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 7
File: hooks/claude-notify/Tests/shell/e2e/test-runtime.sh:238-265
Timestamp: 2026-02-23T07:28:09.415Z
Learning: Applies to hooks/claude-notify/Tests/**/*.{swift,sh}: All test scenarios must have a stable ID registered in Tests/required-cases.txt. Unit tests use U001–U020 prefix, integration Swift XCTest use I001–I005, integration shell tests use I101–I129, e2e shell tests use E001–E013.
📚 Learning: 2026-02-23T07:28:09.415Z
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 7
File: hooks/claude-notify/Tests/shell/e2e/test-runtime.sh:238-265
Timestamp: 2026-02-23T07:28:09.415Z
Learning: In hooks/claude-notify/Tests, ensure every test scenario has a stable ID registered in Tests/required-cases.txt. Use the following ID prefixes and ranges: Unit tests U001–U020, Integration Swift XCTest I001–I005, Integration shell tests I101–I129, and End-to-End shell tests E001–E013. For any test under this path, require a corresponding ID from these ranges to ensure traceability and reproducibility.
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Tests/shell/run.sh
📚 Learning: 2026-02-23T07:27:26.956Z
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 7
File: hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh:17-97
Timestamp: 2026-02-23T07:27:26.956Z
Learning: Enforce test case IDs for all tests under hooks/claude-notify/Tests. Each test scenario must have a stable ID registered in Tests/required-cases.txt. Active ID ranges: Unit tests U001–U020, Integration Swift XCTest I001–I005, Integration shell tests I101–I129, E2E shell tests E001–E013. Reserve I006–I100. During code reviews, verify that every test file under Tests contains a corresponding ID in the required-cases.txt with the correct prefix/range, and that IDs are not reused or out of range.
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Tests/shell/run.sh
📚 Learning: 2026-02-23T07:27:36.086Z
Learnt from: giwankim
Repo: giwankim/my-claude-code PR: 7
File: hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh:17-97
Timestamp: 2026-02-23T07:27:36.086Z
Learning: In hooks/claude-notify/Tests/shell/**/*.sh, ensure every test scenario has a stable ID registered in Tests/required-cases.txt. Use one of the documented ID ranges:
- Unit tests: U001–U020
- Integration Swift XCTest: I001–I005
- Integration shell: I101–I129
- E2E shell tests: E001–E013
Note: I006–I100 are reserved. Use a valid ID from the appropriate range and verify its presence in Tests/required-cases.txt to ensure consistency across test scenarios.
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Tests/shell/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/Package.swift : Swift package for hooks must have two targets: NotifyCore (library with all core logic) and ClaudeNotify (executable with thin main.swift entry). Tests target NotifyCore
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.shhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Tests/shell/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/*/+(*.sh|notify) : Hook entrypoints must be shell scripts (e.g., notify.sh) that accept JSON on stdin per the Claude hook payload format, or a message as $1
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Tests/shell/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Hook behavior is configured via NOTIFY_* environment variables. See hooks/claude-notify/README.md for the full list
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/lib/notify-test-helpers.shhooks/claude-notify/Tests/shell/run.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Applies to hooks/claude-notify/{Sources,Tests}/**/*.swift : Swift docstring coverage threshold: ≥80% on Swift sources in Sources/ and Tests/. Each enum, struct, class, protocol, func, init declaration must have a preceding /// comment
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: The claude-notify.app/ bundle skeleton must be committed. Binary (Contents/MacOS/) and code-sign (Contents/_CodeSignature/) directories must be gitignored and produced by make build
Applied to files:
hooks/claude-notify/Tests/shell/e2e/test-runtime.shhooks/claude-notify/Tests/shell/unit/test-notify-activate.sh
📚 Learning: 2026-02-17T03:00:43.276Z
Learnt from: CR
Repo: giwankim/my-claude-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T03:00:43.276Z
Learning: Tests are tiered (unit → integration → e2e) with sequential execution in CI. Each tier gates the next. Integration tier includes both Swift XCTest (I001–I005) and shell tests (I101+), both run by make test-integration
Applied to files:
hooks/claude-notify/Tests/shell/run.sh
🧬 Code graph analysis (2)
hooks/claude-notify/Tests/shell/e2e/test-runtime.sh (2)
hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh (7)
kill_pid_from_file(145-162)cleanup_registered_tmp_dirs(21-25)make_case_tmp_dir(13-19)write_fake_osascript_success(83-86)drain_pid_file_if_present(164-173)notify_tmux_display_message_format(110-118)build_tmux_display_message_payload(120-130)hooks/claude-notify/Tests/shell/lib/testlib.sh (4)
pass(6-9)fail(11-14)wait_for_file(58-68)case_start(16-18)
hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh (1)
hooks/claude-notify/Tests/shell/lib/testlib.sh (2)
wait_for_pid_file(20-30)wait_for_pid_removed(32-42)
🪛 Shellcheck (0.11.0)
hooks/claude-notify/Tests/shell/e2e/test-runtime.sh
[warning] 12-12: TEST_TMP_DIRS appears unused. Verify use (or export if used externally).
(SC2034)
hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh
[warning] 3-3: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
[warning] 10-10: TEST_TMP_DIRS appears unused. Verify use (or export if used externally).
(SC2034)
hooks/claude-notify/Tests/shell/run.sh
[warning] 5-5: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
🔇 Additional comments (3)
hooks/claude-notify/Tests/shell/run.sh (1)
5-5: SC1007 warning is a false positive — no change needed.
CDPATH= cdis a standard POSIX prefix-assignment idiom that clearsCDPATHonly for the duration of thecdinvocation, guarding againstcdresolving to an unintended directory. ShellCheck's SC1007 does not recognise this pattern, but the construct is correct as written.hooks/claude-notify/Tests/shell/lib/notify-test-helpers.sh (1)
1-173: LGTM — well-structured helper library.The heredoc escaping in
write_fake_osascript(unquoted delimiter to expand$log_env_varat write time,\$for runtime-deferred references) is correct and deliberate.extract_execute_payload'sgetline > 0guard, the numeric PID validation inkill_pid_from_file, and the explicit sourcing-order note fordrain_pid_file_if_presentare all good defensive choices.hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh (1)
30-35: Resolved: All assertion helpers exist and test IDs are valid.The assertion helpers
assert_rc_eq,assert_file_contains, andassert_file_not_containsare all defined intestlib.sh(lines 75, 88, and 101 respectively). Additionally, test IDs U017–U020 are properly registered inTests/required-cases.txt, so there is no command-not-found risk or ID validation issue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@hooks/claude-notify/Tests/shell/run.sh`:
- Around line 7-9: The sequencing of unit → integration → e2e scripts is
correct, but to enforce failure propagation add a strict-fail guard (e.g., set
-euo pipefail) at the top of the wrapper run.sh and check each child script's
exit status; ensure the wrapper invokes unit/run.sh, integration/run.sh, and
e2e/run.sh in that order and exits immediately if any returns non-zero so
downstream stages are not executed after a failure.
---
Nitpick comments:
In `@hooks/claude-notify/Tests/shell/e2e/test-runtime.sh`:
- Around line 241-244: The test uses write_fake_osascript_success and
OSASCRIPT_ARGS_LOG while exercising the activation-inference path via
NOTIFY_ACTIVATE_OSASCRIPT_BIN; change to write_fake_frontmost_osascript and swap
usages of OSASCRIPT_ARGS_LOG/OSASCRIPT_LOG to the activation-specific names
ACTIVATE_OSASCRIPT_ARGS_LOG/ACTIVATE_OSASCRIPT_LOG so the fake script and its
log variable match the semantic path (update the call to
write_fake_frontmost_osascript "$FAKE_OSASCRIPT" and any downstream assertions
that currently reference $OSASCRIPT_LOG to reference $ACTIVATE_OSASCRIPT_LOG
instead).
- Line 12: Add a ShellCheck suppression for SC2034 immediately above the
TEST_TMP_DIRS assignment to silence the false positive; insert a comment line "#
shellcheck disable=SC2034" just before the "TEST_TMP_DIRS=\"\"" declaration so
the linter ignores the unused-variable warning for TEST_TMP_DIRS while leaving
the assignment intact.
In `@hooks/claude-notify/Tests/shell/run.sh`:
- Around line 1-3: The script currently uses "set -e" which only exits on
command failure; update the shell options in the top-level script (the
shebang/run script) to enable unset-variable detection as well by adding the -u
flag (e.g., change the existing set -e invocation in run.sh to use -u too, such
as "set -eu" or equivalent) so the script aborts on unset variable expansions as
well.
In `@hooks/claude-notify/Tests/shell/unit/test-notify-activate.sh`:
- Line 10: Silence the ShellCheck SC2034 warning for TEST_TMP_DIRS by marking
the assignment as intentionally unused: add a ShellCheck directive above the
TEST_TMP_DIRS declaration (e.g. a comment like "# shellcheck disable=SC2034") so
ShellCheck knows the variable is populated externally by the sourced helper
(register_tmp_dir); keep the variable name TEST_TMP_DIRS and the empty
assignment intact.
1 participant
Summary
hooks/claude-notifywith the new symlink to keep the module-specific entrypoint.hooks/claude-notifypaths, commands, docstring/case requirements, and maintenance expectations.Testing