Add ShellSpec harness and coverage - #414
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughSecurity matching now conditionally converts legacy trailing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testing infrastructure by introducing a reusable ShellSpec harness, making it easier to write and maintain tests for the Claude hook scripts. Concurrently, it expands the test suite to cover various notification scenarios and critical security checks. Furthermore, the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DRAdds a reusable ShellSpec mock harness and expands tests for Claude hook scripts to improve reliability and coverage. Fixes security command parsing for legacy patterns and command chaining on macOS. What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a great pull request that significantly improves the test infrastructure and coverage for the hook scripts. The introduction of the reusable mock_bin_setup harness is an excellent addition that cleans up the test files nicely. The expanded test suites for notify.sh, pushover.sh, and security.sh add a lot of value and confidence in the scripts' behavior. The fixes for legacy security patterns and command chaining on macOS are also well-implemented and now properly covered by tests. I have one minor suggestion for improving robustness.
| # This handles: cmd1 ; cmd2, cmd1 && cmd2, cmd1 || cmd2, cmd1 | cmd2 | ||
| # shellcheck disable=SC2001 | ||
| IFS=$'\n' read -r -d '' -a segments < <(echo "$command" | sed 's/[;&|]\+/\n/g' && printf '\0') || true | ||
| IFS=$'\n' read -r -d '' -a segments < <(echo "$command" | sed -E 's/[;&|]+/\n/g' && printf '\0') || true |
There was a problem hiding this comment.
For better robustness and portability, it's recommended to use printf instead of echo when piping variable content to other commands. echo can have surprising behavior if the variable's content starts with a hyphen (-) or contains backslash escape sequences, and its implementation varies between shells. printf '%s' "$command" is a safer alternative that will print the string exactly as is.
| IFS=$'\n' read -r -d '' -a segments < <(echo "$command" | sed -E 's/[;&|]+/\n/g' && printf '\0') || true | |
| IFS=$'\n' read -r -d '' -a segments < <(printf '%s' "$command" | sed -E 's/[;&|]+ /\n/g' && printf '\0') || true |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds a reusable mock harness for ShellSpec testing and significantly expands test coverage for Claude hook scripts. It also includes two important fixes to the security hook: proper handling of legacy deny patterns (e.g., sudo:* → sudo*) and improved command chaining detection on macOS through explicit extended regex in sed.
Key Changes
- Introduced
mock_bin_setupandmock_bin_cleanuphelper functions to standardize mock binary creation across tests - Fixed security.sh to properly convert legacy
pattern:*deny patterns to glob formatpattern* - Enhanced command splitting to use explicit extended regex (
sed -E) for better macOS compatibility - Added comprehensive test coverage for security blocking, notification handling, and Pushover integration
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/support/custom_matcher.sh | New reusable mock harness providing mock_bin_setup and mock_bin_cleanup functions for creating temporary mock binaries that log invocations |
| spec/security_spec.sh | Added tests for legacy sudo:* pattern blocking and semicolon command chaining detection |
| spec/pushover_spec.sh | Refactored to use new mock harness and added tests for Stop hook scenarios (plan approval vs work completed) |
| spec/notify_spec.sh | Refactored to use new mock harness and expanded coverage for permission requests, PreCompact, SubagentStop, and risky command warnings |
| config/claude/security.sh | Fixed legacy pattern conversion logic (sudo:* → sudo*) and switched to explicit extended regex (sed -E) for better portability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| : "${MOCK_LOG:?MOCK_LOG must be set}" | ||
| printf '%s\n' "$0 $*" >>"$MOCK_LOG" |
There was a problem hiding this comment.
The mock script uses $* which may not properly handle arguments with spaces or special characters. Consider using $@ instead, which preserves the original argument structure and is more suitable for logging invocations. While this may not cause issues in the current tests, it could lead to incorrect logging if mocked commands are called with arguments containing spaces.
| printf '%s\n' "$0 $*" >>"$MOCK_LOG" | |
| printf '%s' "$0" >>"$MOCK_LOG" | |
| for arg in "$@"; do printf ' %s' "$arg" >>"$MOCK_LOG"; done | |
| printf '\n' >>"$MOCK_LOG" |
| mock_bin_setup() { | ||
| MOCK_BIN="$(mktemp -d)" | ||
| MOCK_LOG="$MOCK_BIN/mock.log" | ||
| : >"$MOCK_LOG" | ||
|
|
||
| MOCK_ORIGINAL_PATH="${PATH:-}" | ||
| export MOCK_BIN MOCK_LOG MOCK_ORIGINAL_PATH | ||
| export PATH="$MOCK_BIN:$MOCK_ORIGINAL_PATH" | ||
|
|
||
| local cmd | ||
| for cmd in "$@"; do | ||
| cat >"$MOCK_BIN/$cmd" <<'EOF' | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| : "${MOCK_LOG:?MOCK_LOG must be set}" | ||
| printf '%s\n' "$0 $*" >>"$MOCK_LOG" | ||
| exit 0 | ||
| EOF | ||
| chmod +x "$MOCK_BIN/$cmd" | ||
| done | ||
| } | ||
|
|
||
| mock_bin_cleanup() { | ||
| if [[ -n "${MOCK_ORIGINAL_PATH:-}" ]]; then | ||
| export PATH="$MOCK_ORIGINAL_PATH" | ||
| fi | ||
| if [[ -n "${MOCK_BIN:-}" ]]; then | ||
| rm -rf "$MOCK_BIN" | ||
| fi | ||
| unset MOCK_BIN MOCK_LOG MOCK_ORIGINAL_PATH | ||
| } |
There was a problem hiding this comment.
The mock harness functions lack documentation comments. Consider adding comments to explain the purpose and usage of mock_bin_setup and mock_bin_cleanup, including the environment variables they set (MOCK_BIN, MOCK_LOG, MOCK_ORIGINAL_PATH) and how they should be used in tests. This would improve maintainability and help other developers understand how to use these testing utilities.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
spec/security_spec.sh (1)
80-114: Nice coverage expansion, but add||and|chaining cases to match stated parsing fixes.These new blocked-command scenarios are well-targeted and should catch regressions in legacy
:*handling and command segmentation. Since the PR summary calls out splitting on||and|too, I’d add at least one case for each (e.g.,echo ok || rm -rf /*andecho ok | rm -rf /*) to ensure the macOSsed -Epath is fully covered.spec/pushover_spec.sh (1)
29-64: Avoid coupling assertions tocat "$MOCK_LOG"output ordering.Right now, the test output is
script stdout + mock log stdoutin one stream, which can get brittle. If feasible in ShellSpec here, prefer checking the log file separately (or add a tiny helper matcher likeThe file "$MOCK_LOG" should include ...) so the SUT output and mock-call assertions don’t interfere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
spec/notify_spec.sh(3 hunks)spec/pushover_spec.sh(2 hunks)spec/security_spec.sh(1 hunks)spec/shell_files_spec.sh(1 hunks)spec/support/custom_matcher.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/notify_spec.sh
- spec/support/custom_matcher.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{sh,bash}
📄 CodeRabbit inference engine (CLAUDE.md)
Use shfmt with 2-space indentation for shell scripts
**/*.{sh,bash}: Use 2 spaces for indentation in shell scripts
Add proper shebang lines to shell scripts
Follow shellcheck recommendations in shell scripts
Document complex commands in shell scripts
Use consistent variable naming in shell scripts
Files:
spec/pushover_spec.shspec/security_spec.shspec/shell_files_spec.sh
🧠 Learnings (5)
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Follow shellcheck recommendations in shell scripts
Applied to files:
spec/shell_files_spec.sh
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Add proper shebang lines to shell scripts
Applied to files:
spec/shell_files_spec.sh
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Use consistent variable naming in shell scripts
Applied to files:
spec/shell_files_spec.sh
📚 Learning: 2025-11-25T09:34:23.224Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:34:23.224Z
Learning: Applies to **/*.{sh,bash} : Use shfmt with 2-space indentation for shell scripts
Applied to files:
spec/shell_files_spec.sh
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Document complex commands in shell scripts
Applied to files:
spec/shell_files_spec.sh
🧬 Code graph analysis (1)
spec/pushover_spec.sh (1)
spec/support/custom_matcher.sh (2)
mock_bin_setup(5-25)mock_bin_cleanup(27-35)
⏰ 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). (14)
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: nix-flake
- GitHub Check: nix-darwin
- GitHub Check: nix-linux
- GitHub Check: nix-nixos
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: shellcheck
- GitHub Check: shellspec
- GitHub Check: lua-neovim
- GitHub Check: lua-hammerspoon
- GitHub Check: lua-neovim-test
🔇 Additional comments (4)
spec/shell_files_spec.sh (2)
1-3: Verify necessity of SC2329 disable.Line 2 disables SC2329 (unreachable code after return), but I don't see any return/exit statements that would trigger this warning in the code. This disable may be unnecessary or precautionary without clear justification.
Please clarify: Is this disable needed for ShellSpec's dynamic parameter generation, or can it be removed?
19-38: Good use of ShellSpec Parameters:dynamic for dual-phase collection.The test setup cleverly collects both .sh-extension files (lines 20–22) and executable scripts with bash/sh shebangs (lines 24–37), ensuring comprehensive coverage. Defensive file checks and read patterns are sound.
Consider adding a comment above line 19 to explain the collection strategy for future reviewers:
+# Collect tracked shell files in two phases: +# 1. All files with .sh extension +# 2. Non-.sh files with bash or sh shebang (executable scripts) Parameters:dynamicThis minor clarification aligns with the coding guideline to document complex commands.
spec/pushover_spec.sh (2)
7-17: Good move centralizing curl mocking viamock_bin_setup/mock_bin_cleanup.
161-201: Stop-hook coverage looks solid; transcript-driven cases are well chosen.
| It 'prefers environment variables over HOME/dotfiles/.env' | ||
| cat >"$TRANSCRIPT" <<'JSON' | ||
| {"type":"user","message":{"content":[{"type":"text","text":"Do not source"}]}} | ||
| JSON | ||
| TEMP_HOME=$(mktemp -d) | ||
| mkdir -p "$TEMP_HOME/dotfiles" | ||
| cat >"$TEMP_HOME/dotfiles/.env" <<'ENV' | ||
| PUSHOVER_API_TOKEN=bad_token | ||
| PUSHOVER_USER_KEY=bad_user | ||
| ENV | ||
| When run bash -c 'echo "{\"message\": \"Claude is waiting for your input\", \"transcript_path\": \"'"$TRANSCRIPT"'\"}" | env HOME="'"$TEMP_HOME"'" PUSHOVER_API_TOKEN="test_token" PUSHOVER_USER_KEY="test_user" bash '"$SCRIPT"'; rm -rf "'"$TEMP_HOME"'"; cat "$MOCK_LOG"' | ||
| The status should be success |
There was a problem hiding this comment.
Don’t rm -rf temp dirs inside the When run command.
Line 154 deletes TEMP_HOME inline; if the SUT exits early/fails before the rm -rf, the temp dir will leak. Prefer moving that cleanup into the surrounding cleanup() (or track TEMP_HOME at the Describe scope and always remove it in After).
🤖 Prompt for AI Agents
In spec/pushover_spec.sh around lines 144 to 155, the test currently performs rm
-rf "$TEMP_HOME" inside the When run command which may never execute if the SUT
exits early; remove the inline rm -rf invocation and instead ensure TEMP_HOME is
cleaned up unconditionally by registering it with the existing cleanup()
mechanism or by creating/setting TEMP_HOME at the Describe scope and adding an
After/tearDown that always removes the directory (or call cleanup "$TEMP_HOME"
in the surrounding test harness) so temporary directories are never leaked even
on failures.
| syntax_check() { | ||
| local file="$1" | ||
| local first_line="" | ||
|
|
||
| IFS= read -r first_line <"$file" || first_line="" | ||
|
|
||
| case "$first_line" in | ||
| '#!'*bash*) bash -n "$file" ;; | ||
| '#!'*sh*) sh -n "$file" ;; | ||
| *) bash -n "$file" ;; | ||
| esac | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add documentation to the syntax_check function.
The function lacks a docstring explaining its purpose and behavior. Given that it conditionally selects a linter based on shebang detection, adding comments would improve clarity for future maintainers.
Apply this diff to document the function:
syntax_check() {
+ # Validate shell file syntax using the appropriate linter based on shebang.
+ # Falls back to bash -n if no shebang is detected.
local file="$1"
local first_line=""
IFS= read -r first_line <"$file" || first_line=""
case "$first_line" in
'#!'*bash*) bash -n "$file" ;;
'#!'*sh*) sh -n "$file" ;;
*) bash -n "$file" ;;
esac
}🤖 Prompt for AI Agents
In spec/shell_files_spec.sh around lines 6 to 17, the syntax_check function
lacks documentation; add a concise docstring comment above the function that
states its purpose (validate shell script syntax), explains parameters (file
path), describes behavior (reads first line to detect shebang and chooses bash
-n for bash or default, sh -n for sh, falls back to bash -n), and notes return
behavior (exit status mirrors linter); keep the comment short, use single-line
or block comment style consistent with the file, and ensure it precedes the
function definition.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
spec/pushover_spec.sh (1)
29-64: Move inlinerm -rf "$TEMP_HOME"out ofWhen runand intocleanup().The
prefers environment variables over HOME/dotfiles/.envtest still deletesTEMP_HOMEinline (Line 154). If the SUT/execution fails before that segment runs, the temp dir can leak and make the spec flaky. Prefer creatingTEMP_HOMEinsetup()(or at Describe scope) and always removing it incleanup().- TEMP_HOME=$(mktemp -d) - mkdir -p "$TEMP_HOME/dotfiles" + TEMP_HOME=$(mktemp -d) + mkdir -p "$TEMP_HOME/dotfiles" cat >"$TEMP_HOME/dotfiles/.env" <<'ENV' PUSHOVER_API_TOKEN=bad_token PUSHOVER_USER_KEY=bad_user ENV - When run bash -c '...'; rm -rf "'"$TEMP_HOME"'"; cat "$MOCK_LOG"' + When run bash -c '...; cat "$MOCK_LOG"'…and ensure
cleanup()doesrm -rf "$TEMP_HOME"for that scope (like the earlier.envDescribe already does).Also applies to: 66-92, 94-159, 161-201
🧹 Nitpick comments (1)
spec/notify_spec.sh (1)
21-45: Consider quoting$SCRIPTinside thebash -cone-liners (and add brief comments for the complex ones).Most
When run bash -c '... bash '"$SCRIPT"' ...'invocations expand$SCRIPTunquoted, which can break if$PWDcontains spaces; also these are complex pipelines/chained commands and would benefit from a short explanatory comment per shell-script guidelines/learnings.Example pattern:
- When run bash -c 'echo "{\"message\": \"Test message\"}" | bash '"$SCRIPT"'; cat "$MOCK_LOG"' + # Run SUT with stdin JSON; then print mock log for assertions. + When run bash -c 'echo "{\"message\": \"Test message\"}" | bash "$1"; cat "$MOCK_LOG"' -- "$SCRIPT"(Apply similarly across the other
When runcases.)
As per coding guidelines / retrieved learnings.Also applies to: 48-56, 72-78, 80-105, 107-148, 172-217
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
spec/notify_spec.sh(4 hunks)spec/pushover_spec.sh(2 hunks)spec/shell_files_spec.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/shell_files_spec.sh
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{sh,bash}
📄 CodeRabbit inference engine (CLAUDE.md)
Use shfmt with 2-space indentation for shell scripts
**/*.{sh,bash}: Use 2 spaces for indentation in shell scripts
Add proper shebang lines to shell scripts
Follow shellcheck recommendations in shell scripts
Document complex commands in shell scripts
Use consistent variable naming in shell scripts
Files:
spec/notify_spec.shspec/pushover_spec.sh
🧠 Learnings (3)
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Document complex commands in shell scripts
Applied to files:
spec/notify_spec.sh
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Use consistent variable naming in shell scripts
Applied to files:
spec/notify_spec.sh
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Follow shellcheck recommendations in shell scripts
Applied to files:
spec/notify_spec.sh
🧬 Code graph analysis (2)
spec/notify_spec.sh (1)
spec/support/custom_matcher.sh (2)
mock_bin_setup(5-25)mock_bin_cleanup(27-35)
spec/pushover_spec.sh (1)
spec/support/custom_matcher.sh (2)
mock_bin_setup(5-25)mock_bin_cleanup(27-35)
⏰ 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). (14)
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: nix-darwin
- GitHub Check: nix-nixos
- GitHub Check: nix-flake
- GitHub Check: nix-linux
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: lua-neovim
- GitHub Check: lua-hammerspoon
- GitHub Check: lua-neovim-test
- GitHub Check: shellspec
- GitHub Check: shellcheck
Adds a reusable mock harness for ShellSpec and expands test coverage for the Claude hook scripts. Also fixes security hook parsing for legacy deny patterns and command chaining on macOS.
Summary by cubic
Adds a reusable ShellSpec mock harness and expands tests for Claude hook scripts to improve reliability and coverage. Fixes security command parsing for legacy patterns and command chaining on macOS.
New Features
Bug Fixes
Written for commit 9d8b256. Summary will update automatically on new commits.