feat(hooks): add branch context verification hook#1208
Merged
Conversation
Implements git command verification hook that prevents cross-PR contamination by checking if current branch matches session log context before allowing git commit/push operations. Root cause: PR co-mingling from PR #669 retrospective where agents made commits without branch awareness. Changes: - Add invoke_branch_context_guard.py PreToolUse hook - Extract branch from session log JSON and compare with git branch - Block commit/push if mismatch detected, with clear remediation steps - Add is_git_push_command and is_git_commit_or_push_command utilities - Update .claude/settings.json to wire hook into PreToolUse pipeline - Comprehensive test coverage (20 tests, 100% pass) - Keep hook_utilities in sync between .claude/lib and scripts/ Fixes #682 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
PR Validation ReportNote ✅ Status: PASS Description Validation
QA Validation
⚡ Warnings
Powered by PR Validation workflow |
Contributor
✅ Pass: Memory ValidationNo memories with citations found. 📊 Validation Details
|
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new PreToolUse hook to prevent cross-PR contamination by verifying that the current Git branch matches the expected branch from the session log before allowing commit or push operations. New utility functions for detecting Git push commands have been added and integrated into the hook. The changes also include comprehensive tests for the new hook and utilities, ensuring robust behavior across various scenarios, including graceful failure when context is unavailable. The configuration has been updated to enable this new hook for both commit and push actions.
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds a Claude PreToolUse hook that intercepts git commit/push, compares the current git branch to today's session log branch, emits a structured BLOCKED response and exits 2 on mismatch (fails open on errors). Also adds git-push detection helpers, re-exports them, updates settings, and adds tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent/User
participant Hook as Branch Context Guard
participant Git as Git
participant SessionLog as Session Log
Agent->>Hook: pre-tool invoked with command (stdin)
Hook->>Hook: parse input, detect git commit/push
Hook->>Git: git branch --show-current
Git-->>Hook: current-branch or error
Hook->>SessionLog: read today's session log (expected branch)
SessionLog-->>Hook: expected-branch or missing
alt branch mismatch
Hook->>Agent: emit BLOCKED JSON + diagnostic (stdout)
Hook-->>Agent: exit code 2
else match or insufficient data
Hook-->>Agent: exit code 0 (allow)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
.claude/hooks/PreToolUse/invoke_branch_context_guard.py (1)
35-39: Remove unusednoqadirective.Ruff reports
E402is not enabled, so thenoqa: E402comment is unnecessary.♻️ Proposed fix
-from hook_utilities import ( # noqa: E402 +from hook_utilities import ( get_project_directory, get_today_session_log, is_git_commit_or_push_command, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/PreToolUse/invoke_branch_context_guard.py around lines 35 - 39, Remove the unnecessary "noqa: E402" directive on the import from hook_utilities in invoke_branch_context_guard.py; update the import statement that currently reads "from hook_utilities import ( ... )" to drop the "# noqa: E402" suffix so the import block no longer contains the unused noqa comment.tests/test_invoke_branch_context_guard.py (2)
23-28: Remove unusednoqadirective.Ruff reports
E402is not enabled.♻️ Proposed fix
-from invoke_branch_context_guard import ( # noqa: E402 +from invoke_branch_context_guard import ( get_current_branch, get_session_branch, main, write_block_response, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_invoke_branch_context_guard.py` around lines 23 - 28, Remove the unused "# noqa: E402" directive from the import statement that brings in get_current_branch, get_session_branch, main, and write_block_response; simply delete the trailing noqa comment on that import line so the linter no longer reports an unnecessary directive and rerun ruff to confirm the warning is gone.
299-313: Consider verifying stderr output.Test confirms fail-open return value (0) but doesn't verify the error message is written to stderr per the implementation.
🧪 Suggested improvement
def test_fails_open_on_exception( - self, mock_stdin: StringIO, mock_project_dir: MagicMock + self, mock_stdin: StringIO, mock_project_dir: MagicMock, capsys: pytest.CaptureFixture[str] ) -> None: mock_project_dir.side_effect = RuntimeError("unexpected error") hook_input = {"tool_input": {"command": "git commit -m 'test'"}} mock_stdin.write(json.dumps(hook_input)) mock_stdin.seek(0) with patch.object(mock_stdin, "isatty", return_value=False): result = main() assert result == 0 # Fail open + captured = capsys.readouterr() + assert "branch_context_guard error" in captured.err + assert "RuntimeError" in captured.err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_invoke_branch_context_guard.py` around lines 299 - 313, Update TestFailOpen.test_fails_open_on_exception to also capture and assert stderr contains the thrown error message: when get_project_directory raises RuntimeError("unexpected error"), patch sys.stderr (e.g., with new_callable=StringIO) alongside the existing sys.stdin patch, run main(), then assert the stderr.getvalue() includes "unexpected error" (or the expected error prefix from invoke_branch_context_guard) in addition to asserting result == 0; reference get_project_directory and main to locate the behavior to validate.tests/test_hook_utilities.py (1)
83-107: Add partial match test for consistency.
TestIsGitCommitCommandhastest_returns_false_for_partial_matchthat tests "nogit commit". Consider adding the same for push to ensure "nogit push" or "git pushall" don't match.🧪 Suggested test
def test_returns_true_when_preceded_by_whitespace(self) -> None: assert is_git_push_command(" git push") is True + + def test_returns_false_for_partial_match(self) -> None: + assert is_git_push_command("nogit push") is False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_hook_utilities.py` around lines 83 - 107, Add a partial-match negative test inside TestIsGitPushCommand to mirror TestIsGitCommitCommand: create a method (e.g., test_returns_false_for_partial_match) that calls is_git_push_command with inputs like "nogit push" and "git pushall" (or another non-exact variant) and asserts False; this ensures is_git_push_command only matches exact/whitespace-prefixed "git push" forms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.claude/hooks/PreToolUse/invoke_branch_context_guard.py:
- Around line 35-39: Remove the unnecessary "noqa: E402" directive on the import
from hook_utilities in invoke_branch_context_guard.py; update the import
statement that currently reads "from hook_utilities import ( ... )" to drop the
"# noqa: E402" suffix so the import block no longer contains the unused noqa
comment.
In `@tests/test_hook_utilities.py`:
- Around line 83-107: Add a partial-match negative test inside
TestIsGitPushCommand to mirror TestIsGitCommitCommand: create a method (e.g.,
test_returns_false_for_partial_match) that calls is_git_push_command with inputs
like "nogit push" and "git pushall" (or another non-exact variant) and asserts
False; this ensures is_git_push_command only matches exact/whitespace-prefixed
"git push" forms.
In `@tests/test_invoke_branch_context_guard.py`:
- Around line 23-28: Remove the unused "# noqa: E402" directive from the import
statement that brings in get_current_branch, get_session_branch, main, and
write_block_response; simply delete the trailing noqa comment on that import
line so the linter no longer reports an unnecessary directive and rerun ruff to
confirm the warning is gone.
- Around line 299-313: Update TestFailOpen.test_fails_open_on_exception to also
capture and assert stderr contains the thrown error message: when
get_project_directory raises RuntimeError("unexpected error"), patch sys.stderr
(e.g., with new_callable=StringIO) alongside the existing sys.stdin patch, run
main(), then assert the stderr.getvalue() includes "unexpected error" (or the
expected error prefix from invoke_branch_context_guard) in addition to asserting
result == 0; reference get_project_directory and main to locate the behavior to
validate.
Adds stderr logging when get_session_branch encounters exceptions while reading or parsing the session log JSON. This aids debugging while preserving the fail-open behavior required for graceful degradation. Addresses Gemini Code Assist review comment on PR #1208. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
AI Quality Gate ReviewTip ✅ Final Verdict: PASS WalkthroughThis PR was reviewed by six AI agents in parallel, analyzing different aspects of the changes:
Review Summary
💡 Quick Access: Click on individual agent jobs (e.g., "🔒 security Review", "🧪 qa Review") in the workflow run to see detailed findings and step summaries. Security Review DetailsI have completed my security review of PR #1208. Let me provide my analysis. Security Review: PR #1208 - Branch Context Verification HookPR Type Classification
Findings
Analysis Details1. Command Execution (CWE-78) - [PASS] The hook uses a fixed command array with no user input interpolation: subprocess.run(["git", "branch", "--show-current"], ...)This pattern prevents command injection. No dynamic command construction from untrusted input. 2. Path Traversal (CWE-22) - [PASS] Session directory is constructed from project root using 3. Secret Detection - [PASS] No hardcoded credentials, API keys, or tokens detected. No 4. JSON Parsing (CWE-502) - [PASS] Standard 5. Error Handling (CWE-209) - [PASS] Exceptions are caught and logged to stderr with type/message only. No stack traces exposed. The hook fails open on errors, which is intentional behavior for non-blocking hooks. 6. Input Validation (CWE-20) - [PASS] Hook input is parsed as JSON and validated at each step:
7. Log Injection (CWE-117) - Low Risk Line 163 echoes the truncated command in error output. While the command could contain special characters, this is displayed to stdout (the agent's context) not persisted logs. Risk is minimal since the output goes to the Claude agent, not external logging systems. 8. Fail-Open Design - [INFO] The hook returns 0 (allow) on all error paths. This is documented behavior to avoid blocking legitimate operations when session context is unavailable. Acceptable tradeoff for this hook's purpose. 9. Test Coverage - [PASS] 20 tests cover allow and block paths including:
Recommendations
Verdict{
"verdict": "PASS",
"message": "No security vulnerabilities found. Hook uses safe subprocess execution patterns and proper input validation.",
"agent": "security",
"timestamp": "2026-03-06T12:47:29.101Z",
"findings": [
{
"severity": "low",
"category": "misconfiguration",
"description": "Command echo in error output truncates but does not sanitize special characters",
"location": "invoke_branch_context_guard.py:163",
"cwe": "CWE-117",
"recommendation": "Consider stripping control characters from branch names and commands before display. Low priority as output is agent-facing."
}
]
}QA Review DetailsNow I have a complete picture of the PR. Let me compile my QA review. QA Review: PR #1208 - Branch Context Verification HookTest Coverage Assessment
Quality Concerns
Code Quality Analysis
Error Handling Review
Fail-Safe Pattern Verification
Test-Implementation Alignment
Coverage: 8/8 criteria covered (100%) Regression Risk Assessment
Configuration VerificationThe hook is correctly wired in
Library Sync VerificationBoth locations contain identical implementations:
Both {
"verdict": "PASS",
"message": "Branch context guard hook fully tested with 20 hook tests and 28 utility tests; fail-open pattern correctly implemented; all 6052 pytest tests pass.",
"agent": "qa",
"timestamp": "2026-03-06T12:48:34Z",
"findings": [
{
"severity": "low",
"category": "code-quality",
"description": "Synced library copy contains canonical source reference in docstring",
"location": ".claude/lib/hook_utilities/utilities.py:1",
"recommendation": "No action needed - intended design per sync_plugin_lib.py pattern"
}
]
}Analyst Review DetailsNow let me check if the files are synced properly and examine the session log file referenced in the PR: Now I have enough context to complete the analysis. Let me prepare my findings. Analysis: PR #1208 - Branch Context Verification HookCode Quality Score
Overall: 5/5 Impact Assessment
Findings
Recommendations
Architectural AlignmentPositive patterns observed:
Root cause addressed: PR #669 retrospective identified branch co-mingling. This hook enforces branch-session consistency before commit/push. Verdict{
"verdict": "PASS",
"message": "Well-designed hook with comprehensive testing, fail-open safety, and clear remediation guidance.",
"agent": "analyst",
"timestamp": "2026-03-06T12:47:42Z",
"findings": [
{
"severity": "low",
"category": "documentation",
"description": "Episode file has empty task field",
"location": ".agents/memory/episodes/episode-2026-02-20-session-682.json:7",
"recommendation": "Populate task field with session objective"
},
{
"severity": "low",
"category": "documentation",
"description": "Episode metrics files_changed is 0 but PR has 10 files",
"location": ".agents/memory/episodes/episode-2026-02-20-session-682.json:23",
"recommendation": "Update episode metrics to reflect actual changes"
},
{
"severity": "low",
"category": "consistency",
"description": "Docstring header differs between canonical and synced copies",
"location": ".claude/lib/hook_utilities/utilities.py:1",
"recommendation": "Verify sync_plugin_lib.py handles docstring injection correctly"
}
]
}Architect Review DetailsI have reviewed all the key files. Now I can provide the architectural assessment. Design Quality Assessment
Overall Design Score: 4.6/5 Architectural Concerns
Breaking Change Assessment
Technical Debt Analysis
ADR Assessment
Rationale: This PR introduces a new defensive hook using established patterns. It does not introduce new frameworks, change data storage, modify integration patterns, or make security architecture changes. The fail-open strategy and exit code semantics follow existing ADRs. Recommendations
Verdict{
"verdict": "PASS",
"message": "Well-structured defensive hook following established patterns with proper separation of concerns.",
"agent": "architect",
"timestamp": "2026-03-06T12:47:31Z",
"findings": [
{
"severity": "low",
"category": "design-pattern",
"description": "Duplicate code between .claude/lib/ and scripts/ directories",
"location": ".claude/lib/hook_utilities/utilities.py and scripts/hook_utilities/utilities.py",
"recommendation": "Existing sync mechanism via sync_plugin_lib.py handles this. No action needed."
}
]
}DevOps Review DetailsI have all the context needed for this DevOps review. Let me provide my analysis. DevOps Review: PR #1208PR Scope Detection
This PR adds a Python hook with configuration. No workflow YAML changes. Pipeline Impact Assessment
CI/CD Quality Checks
Shell Script Quality Review
Findings
Configuration Review
The hook is correctly wired into PreToolUse pipeline:
No schema violations detected. Test Coverage
Test structure follows pytest best practices with proper mocking and fixtures. Library Sync VerificationBoth copies are consistent:
Proper docstring references canonical source per Template Assessment
Automation Opportunities
The hook implementation is clean and follows existing patterns. Recommendations
Verdict{
"verdict": "PASS",
"message": "Python hook follows best practices with fail-open semantics and comprehensive test coverage",
"agent": "devops",
"timestamp": "2026-03-06T12:48:00Z",
"findings": [
{
"severity": "low",
"category": "shell-quality",
"description": "Subprocess calls git binary without pre-checking availability",
"location": "invoke_branch_context_guard.py:45",
"recommendation": "Already mitigated by try/except FileNotFoundError handling"
}
]
}Roadmap Review DetailsNow I have enough context to provide the roadmap review. Strategic Alignment Assessment
Feature Completeness
Impact Analysis
KANO ClassificationMust-Be - Session integrity is a baseline expectation for any multi-agent workflow system. Users expect commits to land on the correct branch. This hook prevents violations of that expectation. RICE Score
Concerns
Recommendations
Verdict{
"verdict": "PASS",
"message": "Hook prevents documented PR contamination issue with comprehensive tests and right-sized scope",
"agent": "roadmap",
"timestamp": "2026-03-06T12:47:34.232Z",
"findings": [
{
"severity": "low",
"category": "documentation",
"description": "Consider adding PR contamination prevention to v1.0 success metrics",
"location": ".agents/roadmap/product-roadmap.md",
"recommendation": "Add metric: Zero cross-PR contamination incidents post-deployment"
}
]
}Run Details
Powered by AI Quality Gate workflow |
rjmurillo
previously approved these changes
Feb 25, 2026
Both branches added new exports to __init__.py: the feature branch added is_git_commit_or_push_command and is_git_push_command, while main added is_project_repo and skip_if_consumer_repo. Keep all four exports. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Session Protocol Compliance ReportTip ✅ Overall Verdict: PASS All session protocol requirements satisfied. What is Session Protocol?Session logs document agent work sessions and must comply with RFC 2119 requirements:
See Compliance Summary
Detailed Validation ResultsClick each session to see the complete validation report with specific requirement failures. 📄 sessions-2026-02-20-session-682=== Session Validation === [PASS] Session log is valid ✨ Zero-Token ValidationThis validation uses deterministic script analysis instead of AI:
Powered by 📊 Run Details
Powered by Session Protocol Validator workflow |
Contributor
Spec-to-Implementation ValidationCaution ❌ Final Verdict: FAIL What is Spec Validation?This validation ensures your implementation matches the specifications:
Validation Summary
Spec References
Requirements Traceability DetailsNow I have all the information needed. Let me compile the requirements traceability analysis. Requirements Coverage Matrix
Summary
Gaps
Design Decision AnalysisThe PR description indicates a deliberate design choice: the hook blocks on mismatch and provides 3 remediation options (switch branch, update session log, start new session). This replaces the spec's interactive confirmation with a deterministic blocking approach. This is arguably more robust for agent automation since agents should not make judgment calls about branch mismatches.
Implementation Completeness DetailsLet me check the utilities file for the new functions and verify documentation was updated. Let me check if documentation about the hook was added to AGENT-INSTRUCTIONS.md and verify the test coverage for the new utilities. Based on my analysis, I can now produce the acceptance criteria checklist. Acceptance Criteria Checklist
Missing Functionality
Edge Cases Not Covered
Implementation Quality
Run Details
Powered by AI Spec Validator workflow |
Owner
|
Retriggering CI |
Owner
Review Triage RequiredNote Priority: NORMAL - Human approval required before bot responds Review Summary
Next Steps
Powered by PR Maintenance workflow - Add triage:approved label |
Owner
|
Re-triggering CI after description fix. |
10 tasks
This was referenced Apr 7, 2026
This was referenced Jun 1, 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
2 participants
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
Implements git command verification hook that prevents cross-PR contamination by checking if current branch matches session log context before allowing git commit/push operations.
Root Cause
PR co-mingling from PR #669 retrospective where agents made commits without branch awareness, leading to work from multiple PRs contaminating each other.
Changes
invoke_branch_context_guard.py- PreToolUse hook that intercepts git commit/pushbranchfield) and compares withgit branch --show-currentis_git_push_command()- Detects git push commandsis_git_commit_or_push_command()- Unified check for commit/ci/push.claude/settings.jsonPreToolUse pipeline for both commit and push matchers.claude/libandscripts/locationsTest Plan
Verification
Tested scenarios:
🤖 Generated with Claude Code
Fixes #682