chore: upgrade#1789
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughTwo RTK auto-rewrite hook scripts ( ChangesRTK Hook Unified Input/Output Refactoring
🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels:
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 refactors the rtk-rewrite.sh hooks for Claude and Codex to focus on Claude Code PreToolUse:Bash. The changes significantly simplify the command extraction and input processing logic by removing support for multiple input formats (such as .toolArgs and .tool.input) and eliminating GitHub Copilot-specific output structures. Feedback highlights that these changes introduce regressions, including data loss for metadata in certain input structures and broken compatibility with GitHub Copilot integrations.
| // .command | ||
| // empty | ||
| ') | ||
| CMD=$(echo "$INPUT" | jq -r '.tool_input.command // empty') |
There was a problem hiding this comment.
This simplified command extraction logic removes support for multiple input formats used by GitHub Copilot and other integrations (e.g., .toolArgs, .tool.input, and stringified JSON in toolArgs). This regression will cause the hook to skip rewrites for these platforms and will break existing tests in spec/rtk_rewrite_spec.sh (lines 113-128). To maintain robustness and consistency, ensure the logic follows established patterns for Nix-extracted scripts and uses unique delimiters with read and IFS for any command output parsing.
References
- To robustly parse command output in shell scripts, use a unique delimiter (e.g., tab) in the format string and read with a matching IFS. This is safer than splitting by spaces with cut, especially when data fields might contain spaces.
- Maintain consistency with established patterns for writing scripts that are extracted from Nix expressions, even if it involves suppressing linter warnings like ShellCheck SC2024.
| ) | if type == "string" then (fromjson? // {}) else . end | ||
| ') | ||
| # Build the updated tool_input with all original fields preserved, only command changed. | ||
| ORIGINAL_INPUT=$(echo "$INPUT" | jq -c '.tool_input') |
There was a problem hiding this comment.
Restricting ORIGINAL_INPUT to only the .tool_input object leads to data loss for other supported input structures. Previously, the script ensured that all original fields (such as timeout or other metadata) were preserved regardless of the input format.
| }' | ||
| fi | ||
| elif [ "$EXIT_CODE" -eq 3 ]; then | ||
| if [ "$EXIT_CODE" -eq 3 ]; then |
There was a problem hiding this comment.
The removal of the IS_COPILOT_INPUT check and the specific modifiedArgs output format breaks compatibility with GitHub Copilot. Copilot requires this specific JSON structure to accept and execute the rewritten command; without it, the rewrite functionality will be ignored by the Copilot client.
| // .command | ||
| // empty | ||
| ') | ||
| CMD=$(echo "$INPUT" | jq -r '.tool_input.command // empty') |
There was a problem hiding this comment.
This simplified command extraction logic removes support for multiple input formats used by GitHub Copilot and other integrations (e.g., .toolArgs, .tool.input, and stringified JSON in toolArgs). This regression will cause the hook to skip rewrites for these platforms and will break existing tests in spec/codex_rtk_rewrite_spec.sh (lines 67-81). To maintain robustness and consistency, ensure the logic follows established patterns for Nix-extracted scripts and uses unique delimiters with read and IFS for any command output parsing.
References
- To robustly parse command output in shell scripts, use a unique delimiter (e.g., tab) in the format string and read with a matching IFS. This is safer than splitting by spaces with cut, especially when data fields might contain spaces.
- Maintain consistency with established patterns for writing scripts that are extracted from Nix expressions, even if it involves suppressing linter warnings like ShellCheck SC2024.
| ) | if type == "string" then (fromjson? // {}) else . end | ||
| ') | ||
| # Build the updated tool_input with all original fields preserved, only command changed. | ||
| ORIGINAL_INPUT=$(echo "$INPUT" | jq -c '.tool_input') |
There was a problem hiding this comment.
Restricting ORIGINAL_INPUT to only the .tool_input object leads to data loss for other supported input structures. Previously, the script ensured that all original fields (such as timeout or other metadata) were preserved regardless of the input format.
| }' | ||
| fi | ||
| elif [ "$EXIT_CODE" -eq 3 ]; then | ||
| if [ "$EXIT_CODE" -eq 3 ]; then |
There was a problem hiding this comment.
The removal of the IS_COPILOT_INPUT check and the specific modifiedArgs output format breaks compatibility with GitHub Copilot. Copilot requires this specific JSON structure to accept and execute the rewritten command; without it, the rewrite functionality will be ignored by the Copilot client.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="config/claude/hooks/rtk-rewrite.sh">
<violation number="1" location="config/claude/hooks/rtk-rewrite.sh:35">
P1: Parsing only `.tool_input.command` breaks non-Claude payload formats that this hook currently supports, causing command rewrite checks to be skipped.</violation>
<violation number="2" location="config/claude/hooks/rtk-rewrite.sh:85">
P1: Removing the Copilot output path breaks integrations that expect `modifiedArgs` responses instead of Claude `hookSpecificOutput`.</violation>
</file>
<file name="config/codex/hooks/rtk-rewrite.sh">
<violation number="1" location="config/codex/hooks/rtk-rewrite.sh:35">
P1: This now only reads `.tool_input.command`, so valid non-`tool_input` payloads are skipped and never rewritten.</violation>
<violation number="2" location="config/codex/hooks/rtk-rewrite.sh:85">
P1: Removing the Copilot output branch changes the response contract and drops `modifiedArgs`, breaking codex/copilot-style hook consumers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }' | ||
| fi | ||
| elif [ "$EXIT_CODE" -eq 3 ]; then | ||
| if [ "$EXIT_CODE" -eq 3 ]; then |
There was a problem hiding this comment.
P1: Removing the Copilot output path breaks integrations that expect modifiedArgs responses instead of Claude hookSpecificOutput.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/claude/hooks/rtk-rewrite.sh, line 85:
<comment>Removing the Copilot output path breaks integrations that expect `modifiedArgs` responses instead of Claude `hookSpecificOutput`.</comment>
<file context>
@@ -86,37 +78,11 @@ esac
- }'
- fi
-elif [ "$EXIT_CODE" -eq 3 ]; then
+if [ "$EXIT_CODE" -eq 3 ]; then
# Ask: rewrite the command, omit permissionDecision so Claude Code prompts.
jq -n \
</file context>
| // .command | ||
| // empty | ||
| ') | ||
| CMD=$(echo "$INPUT" | jq -r '.tool_input.command // empty') |
There was a problem hiding this comment.
P1: Parsing only .tool_input.command breaks non-Claude payload formats that this hook currently supports, causing command rewrite checks to be skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/claude/hooks/rtk-rewrite.sh, line 35:
<comment>Parsing only `.tool_input.command` breaks non-Claude payload formats that this hook currently supports, causing command rewrite checks to be skipped.</comment>
<file context>
@@ -32,15 +32,7 @@ fi
- // .command
- // empty
-')
+CMD=$(echo "$INPUT" | jq -r '.tool_input.command // empty')
if [ -z "$CMD" ]; then
</file context>
| CMD=$(echo "$INPUT" | jq -r '.tool_input.command // empty') | |
| CMD=$(echo "$INPUT" | jq -r ' | |
| .tool.input.command | |
| // .tool_input.command | |
| // (.toolArgs | if type == "object" then .command else empty end) | |
| // (.toolArgs | if type == "string" then (fromjson? | .command) else empty end) | |
| // .toolInput.command | |
| // .command | |
| // empty | |
| ') |
| }' | ||
| fi | ||
| elif [ "$EXIT_CODE" -eq 3 ]; then | ||
| if [ "$EXIT_CODE" -eq 3 ]; then |
There was a problem hiding this comment.
P1: Removing the Copilot output branch changes the response contract and drops modifiedArgs, breaking codex/copilot-style hook consumers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/codex/hooks/rtk-rewrite.sh, line 85:
<comment>Removing the Copilot output branch changes the response contract and drops `modifiedArgs`, breaking codex/copilot-style hook consumers.</comment>
<file context>
@@ -86,37 +78,11 @@ esac
- }'
- fi
-elif [ "$EXIT_CODE" -eq 3 ]; then
+if [ "$EXIT_CODE" -eq 3 ]; then
# Ask: rewrite the command, omit permissionDecision so Claude Code prompts.
jq -n \
</file context>
| // .command | ||
| // empty | ||
| ') | ||
| CMD=$(echo "$INPUT" | jq -r '.tool_input.command // empty') |
There was a problem hiding this comment.
P1: This now only reads .tool_input.command, so valid non-tool_input payloads are skipped and never rewritten.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/codex/hooks/rtk-rewrite.sh, line 35:
<comment>This now only reads `.tool_input.command`, so valid non-`tool_input` payloads are skipped and never rewritten.</comment>
<file context>
@@ -32,15 +32,7 @@ fi
- // .command
- // empty
-')
+CMD=$(echo "$INPUT" | jq -r '.tool_input.command // empty')
if [ -z "$CMD" ]; then
</file context>
| CMD=$(echo "$INPUT" | jq -r '.tool_input.command // empty') | |
| CMD=$(echo "$INPUT" | jq -r ' | |
| .tool.input.command | |
| // .tool_input.command | |
| // (.toolArgs | if type == "object" then .command else empty end) | |
| // (.toolArgs | if type == "string" then (fromjson? | .command) else empty end) | |
| // .toolInput.command | |
| // .command | |
| // empty | |
| ') |
shunkakinoki
force-pushed
the
chore/upgrade
branch
2 times, most recently
from
417c0f7 to
9194093
Compare
![indent[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| // .command | ||
| // empty | ||
| ') | ||
| CMD=$(echo "$INPUT" | jq -r '.tool_input.command // empty') |
There was a problem hiding this comment.
Copilot integration is silently broken by this sync. This Codex copy is also what gets installed as Copilot's hook (config/copilot/default.nix sets home.file.".copilot/hooks/rtk-rewrite.sh".source = ../codex/hooks/rtk-rewrite.sh, and config/copilot/config.json calls $HOME/.copilot/hooks/rtk-rewrite.sh in its preToolUse array). Copilot CLI sends {"toolName":"shell","toolArgs":{"command":"..."}} (or a stringified JSON variant), but with this PR CMD=$(echo "$INPUT" | jq -r '.tool_input.command // empty') returns empty for those payloads, so the hook falls through the empty-command guard and never rewrites — no error, just a silent no-op for every Copilot Bash invocation. The IS_COPILOT_INPUT/modifiedArgs output branch removed at line 85 below compounds the issue: even if CMD were extracted, Copilot expects modifiedArgs, not Claude's hookSpecificOutput. This regresses the Copilot parity work from #1788. Either re-introduce both branches here (and in the Claude copy, to keep spec/sync_rtk_rewrite_spec.sh's byte-identical invariant) — possibly with a comment to prevent the next scripts/sync-rtk-rewrite.sh run from clobbering it again — or remove the Copilot wiring (config/copilot/config.json entry + config/copilot/default.nix symlink) and the corresponding Copilot specs together.
shunkakinoki
force-pushed
the
chore/upgrade
branch
from
9194093 to
3c9319a
Compare
1 participant
Automated upgrade