test: add shellcheck disable and spec for rtk-rewrite.sh - #891
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
📝 WalkthroughWalkthroughThis PR adds test coverage for a shell script rewriting utility ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
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 enhances the robustness and testability of the "rtk-rewrite.sh" script. It addresses a specific "shellcheck" warning by disabling it where necessary due to "sed" pattern usage, and significantly improves test coverage by introducing a dedicated spec file with a wide array of tests for the script's rewriting logic. Highlights
Changelog
Activity
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;DRAllow sed-based rewrite patterns in What changed?
Description generated by Mesa. Update settings |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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 pull request is a great addition, adding comprehensive tests for the rtk-rewrite.sh script and ensuring it's tracked for test coverage. The new tests in spec/rtk_rewrite_spec.sh cover a wide range of scenarios. My review includes several suggestions to make these new tests even more robust by using exact assertions (eq) instead of partial ones (include), which will help prevent future regressions. Overall, this is a solid contribution to improving the project's test quality.
| Data '{"tool_input": {"command": "find . -name \"*.ts\""}}' | ||
| When run bash -c "bash '$SCRIPT' | jq -r '.hookSpecificOutput.updatedInput.command'" | ||
| The status should be success | ||
| The output should include 'rtk find' |
There was a problem hiding this comment.
For a more robust test, it's better to assert the exact output using eq instead of include. This ensures the entire command is rewritten as expected and prevents regressions where arguments might be unintentionally modified.
| The output should include 'rtk find' | |
| The output should eq 'rtk find . -name "*.ts"' |
| Data '{"tool_input": {"command": "curl https://example.com"}}' | ||
| When run bash -c "bash '$SCRIPT' | jq -r '.hookSpecificOutput.updatedInput.command'" | ||
| The status should be success | ||
| The output should include 'rtk curl' |
There was a problem hiding this comment.
For a more robust test, it's better to assert the exact output using eq instead of include. This ensures the entire command is rewritten as expected and prevents regressions.
| The output should include 'rtk curl' | |
| The output should eq 'rtk curl https://example.com' |
| Data '{"tool_input": {"command": "pytest tests/"}}' | ||
| When run bash -c "bash '$SCRIPT' | jq -r '.hookSpecificOutput.updatedInput.command'" | ||
| The status should be success | ||
| The output should include 'rtk pytest' |
There was a problem hiding this comment.
For a more robust test, it's better to assert the exact output using eq instead of include. This ensures the entire command is rewritten as expected and prevents regressions.
| The output should include 'rtk pytest' | |
| The output should eq 'rtk pytest tests/' |
| Data '{"tool_input": {"command": "go test ./..."}}' | ||
| When run bash -c "bash '$SCRIPT' | jq -r '.hookSpecificOutput.updatedInput.command'" | ||
| The status should be success | ||
| The output should include 'rtk go test' |
There was a problem hiding this comment.
For a more robust test, it's better to assert the exact output using eq instead of include. This ensures the entire command is rewritten as expected and prevents regressions.
| The output should include 'rtk go test' | |
| The output should eq 'rtk go test ./...' |
| Data '{"tool_input": {"command": "FOO=bar git status"}}' | ||
| When run bash -c "bash '$SCRIPT' | jq -r '.hookSpecificOutput.updatedInput.command'" | ||
| The status should be success | ||
| The output should include 'FOO=bar rtk git status' |
There was a problem hiding this comment.
For a more robust test, it's better to assert the exact output using eq instead of include. This ensures the environment variable prefix is preserved correctly and no other part of the command is unintentionally modified.
| The output should include 'FOO=bar rtk git status' | |
| The output should eq 'FOO=bar rtk git status' |
| Data '{"tool_input": {"command": "git status"}}' | ||
| When run bash -c "bash '$SCRIPT' | jq -e '.hookSpecificOutput.permissionDecision'" | ||
| The status should be success | ||
| The output should include 'allow' |
There was a problem hiding this comment.
The output of this jq command is deterministic. Using eq for an exact match is more precise and robust than include.
| The output should include 'allow' | |
| The output should eq 'allow' |
| Data '{"tool_input": {"command": "git status", "timeout": 5000}}' | ||
| When run bash -c "bash '$SCRIPT' | jq -e '.hookSpecificOutput.updatedInput.timeout'" | ||
| The status should be success | ||
| The output should include '5000' |
There was a problem hiding this comment.
The output here will be exactly '5000'. Using eq provides a stricter and more reliable test than include.
| The output should include '5000' | |
| The output should eq '5000' |
There was a problem hiding this comment.
Pull request overview
Adds ShellSpec coverage for the Claude Code rtk-rewrite.sh pre-tool hook and registers it in the repo’s shell-script test coverage list, while silencing a ShellCheck warning related to sed usage.
Changes:
- Added
spec/rtk_rewrite_spec.shwith coverage for guard/skip behavior and a variety of command rewrite cases. - Registered
config/claude/rtk-rewrite.shinspec/coverage_spec.shso it’s enforced by the “all scripts covered” test. - Added
# shellcheck disable=SC2001toconfig/claude/rtk-rewrite.sh.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| spec/rtk_rewrite_spec.sh | New ShellSpec suite validating rewrite/skip behavior and JSON output contract. |
| spec/coverage_spec.sh | Ensures the new script has a spec and is included in the “covered scripts” list. |
| config/claude/rtk-rewrite.sh | Adds a targeted ShellCheck suppression for sed-based rewrites. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,206 @@ | |||
| #!/usr/bin/env bash | |||
| # shellcheck disable=SC2329 | |||
|
|
|||
There was a problem hiding this comment.
These specs rely on config/claude/rtk-rewrite.sh passing its dependency guard (command -v rtk), but the spec doesn’t set up a stub rtk binary in PATH. In environments where rtk isn’t installed (e.g., fresh CI runners), the script will exit 0 with empty output and the jq assertions will fail. Consider adding a setup()/cleanup() with mock_bin_setup rtk / mock_bin_cleanup (as used in other specs) so the rewrite tests are hermetic and don’t depend on system-installed rtk.
| setup() { | |
| mock_bin_setup rtk | |
| } | |
| cleanup() { | |
| mock_bin_cleanup | |
| } |
CI doesn't have rtk installed, so the guard exits silently. Use helper functions with MOCK_BIN in PATH to ensure tests pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
spec/rtk_rewrite_spec.sh (2)
141-141: Prefereqoverincludewhere the full rewritten command is deterministic.
should includeis appropriate for partial matches, but forfind,curl,pytest,go test, and the env-prefix test the full expected string is known.should eqwould catch accidental double-prefixing or trailing garbage, whereasincludewould silently acceptrtk rtk find .orFOO=bar rtk FOO=bar rtk git status.♻️ Suggested assertion strengthening
# find (line 141) -The output should include 'rtk find' +The output should eq 'rtk find . -name "*.ts"' # curl (line 173) -The output should include 'rtk curl' +The output should eq 'rtk curl https://example.com' # pytest (line 182) -The output should include 'rtk pytest' +The output should eq 'rtk pytest tests/' # go test (line 191) -The output should include 'rtk go test' +The output should eq 'rtk go test ./...' # env prefix (line 200) -The output should include 'FOO=bar rtk git status' +The output should eq 'FOO=bar rtk git status'Also applies to: 173-174, 182-183, 191-192, 200-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/rtk_rewrite_spec.sh` at line 141, Tests using "should include" for fully-deterministic rewritten commands (the "rtk find", "rtk curl", "rtk pytest", "rtk go test" and the env-prefix case) should be tightened to "should eq": locate the assertions in the spec that expect the rewritten command output (the tests referencing "rtk find", "rtk curl", "rtk pytest", "rtk go test" and the env-prefix test) and replace the include-based matchers with exact equality matchers so the expected full string is compared exactly (this prevents double-prefixing or trailing garbage being accepted).
89-100: Consider adding tests forheadrewrites — the most complex transformation in the script has no coverage.The
headbranch performs argument reordering (head -N file→rtk read file --max-lines Nandhead --lines=N file→rtk read file --max-lines N), which is structurally different from all other rewrites (which are pure prefix substitutions). Both sed extraction chains are untested, including theLINES/FILEcapture and their interaction with multi-word filenames.💡 Suggested test cases to add inside the existing spec
+Describe 'head rewrites' + It 'rewrites head -N file to rtk read with --max-lines' + Data '{"tool_input": {"command": "head -20 README.md"}}' + When run run_hook_jq + The status should be success + The output should eq 'rtk read README.md --max-lines 20' + End + + It 'rewrites head --lines=N file to rtk read with --max-lines' + Data '{"tool_input": {"command": "head --lines=10 src/main.ts"}}' + When run run_hook_jq + The status should be success + The output should eq 'rtk read src/main.ts --max-lines 10' + End + + It 'does not rewrite head without -N flag' + Data '{"tool_input": {"command": "head file.txt"}}' + When run run_hook + The status should be success + The output should eq '' + End +End🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/rtk_rewrite_spec.sh` around lines 89 - 100, Add tests covering the "head" rewrite transformation in spec/rtk_rewrite_spec.sh: create a new Describe block (e.g., Describe 'head rewrites') that uses Data with inputs for both forms "head -5 file.txt" and "head --lines=5 file.txt" and for at least one case a multi-word filename (e.g., "my file.txt"); run run_hook and assert status is success and The output should eq the expected rewritten command "rtk read file.txt --max-lines 5" (and for the multi-word file ensure quoting/spacing is preserved in the expected output); include cases that verify the sed capture behavior for LINES and FILE so both the "-N" and "--lines=N" parsing branches are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/rtk_rewrite_spec.sh`:
- Line 141: Tests using "should include" for fully-deterministic rewritten
commands (the "rtk find", "rtk curl", "rtk pytest", "rtk go test" and the
env-prefix case) should be tightened to "should eq": locate the assertions in
the spec that expect the rewritten command output (the tests referencing "rtk
find", "rtk curl", "rtk pytest", "rtk go test" and the env-prefix test) and
replace the include-based matchers with exact equality matchers so the expected
full string is compared exactly (this prevents double-prefixing or trailing
garbage being accepted).
- Around line 89-100: Add tests covering the "head" rewrite transformation in
spec/rtk_rewrite_spec.sh: create a new Describe block (e.g., Describe 'head
rewrites') that uses Data with inputs for both forms "head -5 file.txt" and
"head --lines=5 file.txt" and for at least one case a multi-word filename (e.g.,
"my file.txt"); run run_hook and assert status is success and The output should
eq the expected rewritten command "rtk read file.txt --max-lines 5" (and for the
multi-word file ensure quoting/spacing is preserved in the expected output);
include cases that verify the sed capture behavior for LINES and FILE so both
the "-N" and "--lines=N" parsing branches are exercised.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
# shellcheck disable=SC2001toconfig/claude/rtk-rewrite.sh— the sed patterns use^(start-of-string anchor) which bash parameter expansion cannot replicatespec/rtk_rewrite_spec.shwith 26 tests covering guards, skip conditions, git/gh/cargo/file/JS/Python/Go rewrites, env prefix preservation, and output formatrtk-rewrite.shinspec/coverage_spec.shcoverage listTest plan
make shell-lintpasses (0 warnings)make shell-testpasses (523 examples, 0 failures)🤖 Generated with Claude Code
Summary by cubic
Allow sed-based rewrite patterns in rtk-rewrite.sh and add a comprehensive spec to lock down behavior. Suppresses a false shellcheck warning, expands coverage, keeps CI passing, and formats the new spec.
Bug Fixes
New Features
Written for commit 339ba3e. Summary will update on new commits.