Conversation
Skip analysis for special commit types: - Merge commits (Merge branch/pull request/remote-tracking) - Fixup/squash/amend! commits (will be squashed later) - Revert commits (auto-generated) Added shouldSkipAnalysis() to CommitMsgHandler with pattern detection. Updated shell hook to v4 with grep-based skip conditions. Also handles: - Unicode/emoji in commit messages - Very long commit messages (content truncated to 200 chars) - Empty/whitespace-only messages Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42 AI-Model: claude-opus-4-5-20251101 AI-Decision: handle edge cases in commit-msg hook (GIT-85). Skip analysis for special commit types: AI-Confidence: medium AI-Tags: application, handlers, hooks, tests, integration, unit AI-Lifecycle: project AI-Memory-Id: 172b6832
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds pre-analysis skip logic for specific commit messages (merge, fixup/squash/amend, revert) implemented in the commit-msg hook and CommitMsgHandler; bumps hook fingerprint to v4 and updates unit and integration tests to expect skipped behavior for those commits. Changes
Sequence Diagram(s)sequenceDiagram
participant Git as Git (client)
participant Hook as commit-msg hook (shell)
participant Handler as CommitMsgHandler (TS)
participant Analyzer as AI Analyzer/Agent
Git->>Hook: invoke with commit message
Hook->>Hook: check fast-skip patterns (merge/fixup/squash/amend/revert/AI-Memory-Id)
alt Hook decides to skip
Hook-->>Git: exit successfully (no analysis)
else continue
Hook->>Handler: pass prepared JSON message
Handler->>Handler: shouldSkipAnalysis(firstLine)
alt Handler decides to skip
Handler-->>Hook: return success (no trailers)
else
Handler->>Analyzer: autoAnalyze / inferTags / analysis
Analyzer-->>Handler: analysis result (type/tags/decision)
Handler->>Git: append trailers to commit-msg (if applicable)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/hooks/commit-msg.ts`:
- Around line 38-45: The shell commit-msg hook uses case-sensitive grep patterns
for merge/fixup/revert checks which can diverge from the TypeScript handler
CommitMsgHandler.shouldSkipAnalysis() that uses case-insensitive regex; make the
shell hook consistent by adding the -i flag to each grep -qE invocation for the
three checks (merge, fixup/squash, revert) so they perform case-insensitive
matching and align behavior with shouldSkipAnalysis(), or alternatively add a
brief comment above those grep lines explaining the intentional case-sensitive
choice.
In `@tests/unit/hooks/commit-msg.test.ts`:
- Line 105: Update the failing assertion message in the test that checks
content.includes('git-mem:commit-msg v4') so the human-readable message matches
the expected version; change the assertion message text from 'Should be upgraded
to v3' to 'Should be upgraded to v4' in the test containing
assert.ok(content.includes('git-mem:commit-msg v4')).
There was a problem hiding this comment.
Pull request overview
This PR improves the commit-msg hook’s robustness by skipping analysis (and trailer injection) for commit message types that are typically auto-generated or transient, and by hardening handling for Unicode and long commit messages.
Changes:
- Added
shouldSkipAnalysis()toCommitMsgHandlerto bypass analysis for merge/fixup/squash/amend!/revert commit messages. - Updated the installed
commit-msgshell hook fingerprint tov4and added grep-based early-exit skip conditions. - Expanded integration coverage with additional edge-case scenarios (merge, fixup/squash/amend!, revert, unicode/emoji, long messages).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/application/handlers/CommitMsgHandler.ts |
Adds centralized skip logic for special commit message types before analysis/trailer generation. |
src/hooks/commit-msg.ts |
Bumps hook fingerprint to v4 and adds shell-level skip conditions to avoid invoking the Node handler. |
tests/unit/hooks/commit-msg.test.ts |
Updates hook fingerprint expectations for install/wrap/upgrade tests. |
tests/integration/hooks/hook-commit-msg.test.ts |
Adds/updates integration tests for skip cases and unicode/long-message handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add -i flag to grep for case-insensitive matching (matches TS handler) - Fix assertion message: v3 → v4 - Fix truncation test: 250 → 200 chars (matches code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42 AI-Model: claude-opus-4-5-20251101 AI-Gotcha: address PR review comments (GIT-85). - Add -i flag to grep for case-insensitive matching (matches TS handler) AI-Confidence: medium AI-Tags: hooks, tests, integration, unit AI-Lifecycle: project AI-Memory-Id: 9f67cb62
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/hooks/commit-msg.ts`:
- Around line 38-45: Read the commit message first line once into a variable
(e.g., first_line) and reuse it for the three checks instead of calling head -1
"$COMMIT_MSG_FILE" each time; then update the three guards that reference
COMMIT_MSG_FILE (the grep checks for "^Merge (branch|pull
request|remote-tracking)", "^(fixup|squash|amend)! ", and '^Revert "') to test
against the saved variable (first_line) and keep the same exit 0 behavior when a
match is found.
In `@tests/integration/hooks/hook-commit-msg.test.ts`:
- Around line 692-713: The test "should handle very long commit message"
currently only checks truncation if the decision trailer exists; update the
assertions to first assert that the AI-Decision trailer is present (e.g.,
assert.ok(decisionMatch, 'AI-Decision trailer must exist')) before asserting its
length, locating the relevant variables/expressions commitMsg, msgPath,
runHook('commit-msg', ...), modifiedMsg, and decisionMatch so that the test
fails if AI-Decision is missing and still enforces the 200-char truncation from
buildTrailers.
Comment on lines
+38
to
+45
| # Skip merge commits (auto-generated by git) | ||
| head -1 "$COMMIT_MSG_FILE" | grep -qiE "^Merge (branch|pull request|remote-tracking)" && exit 0 | ||
|
|
||
| # Skip fixup/squash commits (will be squashed later) | ||
| head -1 "$COMMIT_MSG_FILE" | grep -qiE "^(fixup|squash|amend)! " && exit 0 | ||
|
|
||
| # Skip revert commits (auto-generated) | ||
| head -1 "$COMMIT_MSG_FILE" | grep -qiE '^Revert "' && exit 0 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: avoid reading the first line three times.
You can capture the first line once and reuse it for the three skip checks to reduce subprocesses and keep the script a bit cleaner.
♻️ Optional refactor
-# Skip merge commits (auto-generated by git)
-head -1 "$COMMIT_MSG_FILE" | grep -qiE "^Merge (branch|pull request|remote-tracking)" && exit 0
-
-# Skip fixup/squash commits (will be squashed later)
-head -1 "$COMMIT_MSG_FILE" | grep -qiE "^(fixup|squash|amend)! " && exit 0
-
-# Skip revert commits (auto-generated)
-head -1 "$COMMIT_MSG_FILE" | grep -qiE '^Revert "' && exit 0
+# Skip merge commits (auto-generated by git)
+FIRST_LINE=$(head -n 1 "$COMMIT_MSG_FILE")
+printf '%s' "$FIRST_LINE" | grep -qiE "^Merge (branch|pull request|remote-tracking)" && exit 0
+
+# Skip fixup/squash commits (will be squashed later)
+printf '%s' "$FIRST_LINE" | grep -qiE "^(fixup|squash|amend)! " && exit 0
+
+# Skip revert commits (auto-generated)
+printf '%s' "$FIRST_LINE" | grep -qiE '^Revert "' && exit 0📝 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.
Suggested change
| # Skip merge commits (auto-generated by git) | |
| head -1 "$COMMIT_MSG_FILE" | grep -qiE "^Merge (branch|pull request|remote-tracking)" && exit 0 | |
| # Skip fixup/squash commits (will be squashed later) | |
| head -1 "$COMMIT_MSG_FILE" | grep -qiE "^(fixup|squash|amend)! " && exit 0 | |
| # Skip revert commits (auto-generated) | |
| head -1 "$COMMIT_MSG_FILE" | grep -qiE '^Revert "' && exit 0 | |
| # Skip merge commits (auto-generated by git) | |
| FIRST_LINE=$(head -n 1 "$COMMIT_MSG_FILE") | |
| printf '%s' "$FIRST_LINE" | grep -qiE "^Merge (branch|pull request|remote-tracking)" && exit 0 | |
| # Skip fixup/squash commits (will be squashed later) | |
| printf '%s' "$FIRST_LINE" | grep -qiE "^(fixup|squash|amend)! " && exit 0 | |
| # Skip revert commits (auto-generated) | |
| printf '%s' "$FIRST_LINE" | grep -qiE '^Revert "' && exit 0 |
🤖 Prompt for AI Agents
In `@src/hooks/commit-msg.ts` around lines 38 - 45, Read the commit message first
line once into a variable (e.g., first_line) and reuse it for the three checks
instead of calling head -1 "$COMMIT_MSG_FILE" each time; then update the three
guards that reference COMMIT_MSG_FILE (the grep checks for "^Merge (branch|pull
request|remote-tracking)", "^(fixup|squash|amend)! ", and '^Revert "') to test
against the saved variable (first_line) and keep the same exit 0 behavior when a
match is found.
Comment on lines
+692
to
+713
| it('should handle very long commit message', () => { | ||
| process.env.GIT_MEM_AGENT = 'TestAgent/2.0'; | ||
|
|
||
| // Create a long commit message (> 1000 chars) | ||
| const longBody = 'This is a detailed explanation. '.repeat(50); | ||
| const commitMsg = `feat: add comprehensive feature\n\n${longBody}\n\nBecause this decision will help with scaling.`; | ||
| const msgPath = createCommitMsgFile(repoDir, commitMsg); | ||
|
|
||
| const result = runHook('commit-msg', { | ||
| commit_msg_path: msgPath, | ||
| cwd: repoDir, | ||
| }); | ||
|
|
||
| assert.equal(result.status, 0); | ||
|
|
||
| const modifiedMsg = readFileSync(msgPath, 'utf8'); | ||
| assert.ok(modifiedMsg.includes('AI-Agent:'), 'Should handle long messages'); | ||
| // Content should be truncated in trailers (200 char limit in buildTrailers) | ||
| const decisionMatch = modifiedMsg.match(/AI-Decision: (.+)/); | ||
| if (decisionMatch) { | ||
| assert.ok(decisionMatch[1].length <= 200, 'Trailer content should be truncated to 200 chars'); | ||
| } |
There was a problem hiding this comment.
Make the truncation test assert the decision trailer exists.
Right now the test passes even if AI-Decision is missing, so it won’t catch truncation regressions.
✅ Tighten the assertion
- const decisionMatch = modifiedMsg.match(/AI-Decision: (.+)/);
- if (decisionMatch) {
- assert.ok(decisionMatch[1].length <= 200, 'Trailer content should be truncated to 200 chars');
- }
+ const decisionMatch = modifiedMsg.match(/AI-Decision: (.+)/);
+ assert.ok(decisionMatch, 'Should include AI-Decision trailer');
+ assert.ok(decisionMatch[1].length <= 200, 'Trailer content should be truncated to 200 chars');🤖 Prompt for AI Agents
In `@tests/integration/hooks/hook-commit-msg.test.ts` around lines 692 - 713, The
test "should handle very long commit message" currently only checks truncation
if the decision trailer exists; update the assertions to first assert that the
AI-Decision trailer is present (e.g., assert.ok(decisionMatch, 'AI-Decision
trailer must exist')) before asserting its length, locating the relevant
variables/expressions commitMsg, msgPath, runHook('commit-msg', ...),
modifiedMsg, and decisionMatch so that the test fails if AI-Decision is missing
and still enforces the 200-char truncation from buildTrailers.
This was referenced Feb 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Merge branch/pull request/remote-tracking)Changes
shouldSkipAnalysis()method toCommitMsgHandlerTest plan
Closes GIT-85
🤖 Generated with Claude Code
Summary by CodeRabbit