Consolidate 16 hook PRs: security hardening, dedup refactor, tests + CI#17
Consolidate 16 hook PRs: security hardening, dedup refactor, tests + CI#17zknpr wants to merge 2 commits into
Conversation
Consolidates 16 overlapping PRs into one coherent change. Security: - Fix path traversal / arbitrary file write: sanitize session_id before building the ~/.claude state path (affected both hooks). - Fix broken cross-process dedup in security_guard: hash(command) is per-process randomized, so warnings never deduped and the state file grew unbounded; use sha256 instead. Refactor: - Extract duplicated debug_log/get_state_file/load_shown/save_shown into hooks/utils.py (stdlib-only, OSError-safe). - Log state-save failures instead of silently passing. - api_key exclude now distinguishes dummy *values* from dummy *variable names*, cutting false negatives (e.g. test_api_key = '<real secret>'). Tests/CI: - pytest suite: all security_guard rules, secret_scanner patterns + exclusions, path-traversal regression, main() JSON-error handling. - GitHub Actions matrix (3.11-3.13), .gitignore. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a shared hooks/utils module for debug logging and per-session JSON state, refactors secret_scanner and security_guard to use it (tightening detection and using SHA256 dedup keys), adds pytest suites for hooks/utils, and configures a GitHub Actions matrix plus .gitignore updates. ChangesHook Refactoring and Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 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 secret_scanner and security_guard hooks by centralizing shared logging and state management into a new utils.py module. Key improvements include replacing the randomized hash() function with hashlib.sha256 for consistent deduplication and adding a value_exclude mechanism to the secret scanner to better handle placeholder values. The PR also introduces a suite of unit tests covering the hooks and utility functions. Review feedback suggests making the string encoding more resilient to non-UTF-8 characters to prevent potential crashes and highlights a maintenance concern where the new exclusion logic depends on specific named capture groups in regex patterns.
| # Dedup: only show each rule+command combo once per session | ||
| warning_key = f"{rule_name}:{hash(command)}" | ||
| shown = load_shown(session_id) | ||
| warning_key = f"{rule_name}:{hashlib.sha256(command.encode('utf-8')).hexdigest()}" |
There was a problem hiding this comment.
While using hashlib.sha256 is a significant improvement over the per-process randomized hash(), ensure that the command string is consistently encoded. If the input contains non-UTF-8 characters, command.encode('utf-8') will raise a UnicodeEncodeError, which would crash the hook. Consider using errors='replace' or errors='ignore' to make the hook more resilient to unexpected binary data in shell commands.
| warning_key = f"{rule_name}:{hashlib.sha256(command.encode('utf-8')).hexdigest()}" | |
| warning_key = f"{rule_name}:{hashlib.sha256(command.encode('utf-8', errors='replace')).hexdigest()}" |
There was a problem hiding this comment.
Fixed in 1ab2953 -- encode now uses errors='replace', so lone surrogate chars can't raise UnicodeEncodeError and crash the hook.
| if rule.get("value_exclude"): | ||
| quoted_value = match.groupdict().get("quoted_value", "") | ||
| if rule["value_exclude"].search(quoted_value): | ||
| continue |
There was a problem hiding this comment.
The logic for value_exclude relies on the existence of a named capture group quoted_value in the rule's regex. Currently, only the api_key_assignment rule defines this group. If a new rule is added with a value_exclude pattern but without the quoted_value group in its regex, the exclusion will silently fail (as quoted_value will default to an empty string). It would be safer to verify the group exists or document this requirement for future rules.
There was a problem hiding this comment.
Addressed in 1ab2953 -- scan_content now falls back to the full match when a rule has no quoted_value group, and a comment above the rule documents that value_exclude rules must define that group.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/tests.yml (1)
1-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit permissions block to follow least-privilege principle.
The workflow inherits default GitHub Actions permissions, which may be broader than necessary. For a test-only workflow that doesn't need to write comments, create releases, or modify the repository, restrict permissions to read-only.
🔒 Proposed fix to add minimal permissions
name: Tests +permissions: + contents: read + on: push: pull_request:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/tests.yml around lines 1 - 21, Add a top-level permissions block to this "Tests" workflow to enforce least-privilege: at the root of the YAML (alongside name/on/jobs), add permissions with at least contents: read (and packages: read if you need registry access); ensure no write permissions are granted for repository contents or pull requests. This change targets the workflow definition (name: Tests) and the pytest job/steps so the CI runs with explicit read-only permissions instead of inheriting broader defaults.hooks/security_guard.py (1)
266-285:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winKeep blocking repeated dangerous commands.
Line 271 only gates the message, but Lines 279-283 are also inside that branch. After the first hit, the same
BLOCKEDcommand falls through to Line 285 and exits0, so a repeated dangerous command is allowed for the rest of the session. Dedup should suppress duplicate output, not the enforcement. A regression test that runs the same blocked command twice would catch this.Suggested fix
if rule_name and message: # Dedup: only show each rule+command combo once per session warning_key = f"{rule_name}:{hashlib.sha256(command.encode('utf-8')).hexdigest()}" shown = load_shown(session_id, STATE_PREFIX) + is_blocked = "BLOCKED" in message if warning_key not in shown: shown.add(warning_key) save_shown(session_id, STATE_PREFIX, shown, DEBUG_LOG) print(message, file=sys.stderr) debug_log(f"BLOCKED: {rule_name} — {command[:100]}", DEBUG_LOG) - # Exit 2 = block for BLOCKED rules, 0 = warn-only for WARNING rules - if "BLOCKED" in message: - sys.exit(2) - else: - # Warnings: show message but allow execution - sys.exit(0) + # Exit 2 = block for BLOCKED rules, 0 = warn-only for WARNING rules + if is_blocked: + sys.exit(2) + else: + # Warnings: show message but allow execution + sys.exit(0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hooks/security_guard.py` around lines 266 - 285, The dedup logic currently gates both the message display and the enforcement: when a warning_key is already in shown the function skips to sys.exit(0) and thus repeated BLOCKED commands are not enforced; change the flow so dedup only suppresses printing/saving (use load_shown/save_shown with warning_key for display side) but always perform the enforcement branch based on message content (i.e., ensure sys.exit(2) for BLOCKED and sys.exit(0) for WARNING runs regardless of whether warning_key was already seen), touching the code that builds warning_key, calls load_shown/save_shown, and the sys.exit calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/tests.yml:
- Around line 3-5: The workflow lacks a concurrency policy which allows
overlapping runs; add a top-level concurrency block to the workflow (adjacent to
the existing on: push / pull_request) that defines a group (e.g. using
github.workflow and github.ref or github.ref_name) and sets cancel-in-progress:
true so new runs cancel outdated ones; update the workflow to include the
concurrency key and the cancel-in-progress setting to prevent parallel test runs
for the same branch/PR.
- Line 15: Update the actions/checkout step to disable persisted credentials:
modify the checkout usage (actions/checkout@v4) to include the input
persist-credentials: false so GitHub credentials are not left available to tests
or third‑party code; locate the checkout step in the workflow where "uses:
actions/checkout@v4" appears and add the persist-credentials: false setting to
that step.
- Around line 15-16: The workflow is using mutable action tags
actions/checkout@v4 and actions/setup-python@v5 which can be moved; replace
those tag references with the corresponding immutable commit SHAs (e.g.,
actions/checkout@<sha> and actions/setup-python@<sha>) so the jobs always run
the pinned commits, and consider adding Dependabot/Renovate to keep the SHAs
updated automatically; ensure you update both occurrences of actions/checkout
and actions/setup-python in the workflow.
In `@hooks/secret_scanner.py`:
- Around line 113-115: The value_exclude regex in hooks/secret_scanner.py
currently matches those placeholder tokens by substring and should be anchored
to the whole value; update the value_exclude pattern (both occurrences that
define value_exclude) to require full-string matches e.g. use something like
r"^(?:YOUR_.*|REPLACE_.*|xxx|placeholder|example|test|fake|dummy)$" with
re.IGNORECASE so only entire quoted values that are placeholders are suppressed
(not values that merely contain those words).
In `@hooks/utils.py`:
- Around line 35-37: The load_shown function currently does set(json.load(f))
and only catches json.JSONDecodeError and OSError, so malformed-but-valid JSON
that yields a non-iterable or contains unhashable items can raise TypeError and
escape; update the exception handling around set(json.load(f)) in load_shown to
also catch TypeError (and optionally ValueError) and return an empty set on
those cases, and add a unit test exercising load_shown with valid JSON of the
wrong shape (e.g., dict or list of unhashable objects) to ensure it returns an
empty set.
---
Outside diff comments:
In @.github/workflows/tests.yml:
- Around line 1-21: Add a top-level permissions block to this "Tests" workflow
to enforce least-privilege: at the root of the YAML (alongside name/on/jobs),
add permissions with at least contents: read (and packages: read if you need
registry access); ensure no write permissions are granted for repository
contents or pull requests. This change targets the workflow definition (name:
Tests) and the pytest job/steps so the CI runs with explicit read-only
permissions instead of inheriting broader defaults.
In `@hooks/security_guard.py`:
- Around line 266-285: The dedup logic currently gates both the message display
and the enforcement: when a warning_key is already in shown the function skips
to sys.exit(0) and thus repeated BLOCKED commands are not enforced; change the
flow so dedup only suppresses printing/saving (use load_shown/save_shown with
warning_key for display side) but always perform the enforcement branch based on
message content (i.e., ensure sys.exit(2) for BLOCKED and sys.exit(0) for
WARNING runs regardless of whether warning_key was already seen), touching the
code that builds warning_key, calls load_shown/save_shown, and the sys.exit
calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2936809-c9ef-40a2-be79-717ba8996462
📒 Files selected for processing (9)
.github/workflows/tests.yml.gitignorehooks/secret_scanner.pyhooks/security_guard.pyhooks/utils.pytests/conftest.pytests/test_secret_scanner.pytests/test_security_guard.pytests/test_utils.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (5)
tests/test_secret_scanner.py (1)
hooks/utils.py (1)
debug_log(13-20)
tests/test_utils.py (1)
hooks/utils.py (1)
get_state_file(23-26)
tests/test_security_guard.py (2)
hooks/utils.py (1)
debug_log(13-20)hooks/security_guard.py (1)
check_command(233-238)
hooks/security_guard.py (1)
hooks/utils.py (4)
debug_log(13-20)load_shown(29-38)save_shown(41-50)get_state_file(23-26)
hooks/secret_scanner.py (1)
hooks/utils.py (4)
debug_log(13-20)load_shown(29-38)save_shown(41-50)get_state_file(23-26)
🪛 OpenGrep (1.21.0)
tests/test_secret_scanner.py
[WARNING] 34-34: Hardcoded AWS access key detected. Use environment variables or a secrets manager instead.
(coderabbit.secrets.aws-access-key)
🪛 Ruff (0.15.13)
tests/test_secret_scanner.py
[warning] 68-71: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
hooks/utils.py
[warning] 16-16: datetime.datetime.now() called without a tz argument
(DTZ005)
[warning] 34-34: Unnecessary mode argument
Remove mode argument
(UP015)
tests/test_security_guard.py
[warning] 51-54: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
hooks/security_guard.py
[error] 25-25: Probable insecure usage of temporary file or directory: "/tmp/seal-security-guard.log"
(S108)
hooks/secret_scanner.py
[error] 30-30: Probable insecure usage of temporary file or directory: "/tmp/seal-secret-scanner.log"
(S108)
🪛 zizmor (1.25.2)
.github/workflows/tests.yml
[warning] 15-15: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 1-21: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 15-15: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 16-16: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[info] 8-8: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[warning] 3-5: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🔇 Additional comments (4)
tests/conftest.py (1)
1-8: LGTM!.gitignore (1)
1-4: LGTM!hooks/utils.py (1)
13-27: LGTM!Also applies to: 41-50
tests/test_utils.py (1)
1-18: LGTM!
- security_guard: encode command with errors='replace' to avoid UnicodeEncodeError crash on lone surrogates - utils.load_shown: also catch TypeError on malformed (valid-but-wrong-shape) state JSON - secret_scanner: api_key value_exclude now fullmatches placeholder forms, fixing false negatives where a real value merely contained a dummy substring (e.g. live_test_token_...) - value_exclude falls back to the full match when a rule lacks a quoted_value group - CI: add concurrency cancel-in-progress, persist-credentials: false, pin actions to commit SHAs Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ab295390b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ts = datetime.now().strftime("%Y-%m-%d %H:%M:%S.%f")[:-3] | ||
| with open(log_file, "a") as f: | ||
| f.write(f"[{ts}] {msg}\n") | ||
| except OSError: |
There was a problem hiding this comment.
debug_log now only suppresses OSError, but it formats and writes untrusted text from hook input (command/file_path) that can contain non-UTF-8-surrogate data (for example JSON "\ud800"). In that case f.write(...) raises UnicodeEncodeError, which escapes this helper and can terminate security_guard/secret_scanner before rule evaluation. The previous implementation swallowed all exceptions, so this is a regression in hook robustness; at minimum UnicodeError should also be handled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
hooks/secret_scanner.py (1)
27-31: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUnused import:
get_state_file.
get_state_fileis imported but never directly called in this module—it's only used internally byload_shownandsave_shown.Suggested fix
sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) -from utils import debug_log, get_state_file, load_shown, save_shown +from utils import debug_log, load_shown, save_shown🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hooks/secret_scanner.py` around lines 27 - 31, The import list includes get_state_file which is unused directly in this module; remove get_state_file from the from utils import ... statement so only debug_log, load_shown, and save_shown are imported (leave sys.path insertion and DEBUG_LOG/STATE_PREFIX unchanged) to avoid the unused-import warning and rely on load_shown/save_shown which already call get_state_file internally.hooks/security_guard.py (1)
21-26: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUnused import:
get_state_file.Same as in
secret_scanner.py—get_state_fileis imported but never directly called.Suggested fix
sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) -from utils import debug_log, get_state_file, load_shown, save_shown +from utils import debug_log, load_shown, save_shown🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hooks/security_guard.py` around lines 21 - 26, The import list in hooks/security_guard.py includes get_state_file but it is never used; remove get_state_file from the utils import (keep debug_log, load_shown, save_shown) to eliminate the unused import and mirror the fix applied in secret_scanner.py so the import statement only imports symbols actually referenced.tests/test_utils.py (1)
6-11: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider strengthening the path traversal test.
The current test verifies that
".."is absent and the prefix is correct, but it doesn't validate the sanitized filename component. According to the PR objectives, the sanitization usesre.sub(r"[^a-zA-Z0-9_-]", "_", ...), so"../../../etc/passwd"should become something like"___etc_passwd"in the filename.🔍 Suggested assertion to verify sanitization behavior
def test_get_state_file_sanitizes_session_id_path_traversal(): path = get_state_file("../../../etc/passwd", "seal_guard_state") prefix = os.path.expanduser("~/.claude/.seal_guard_state_") assert ".." not in path assert path.startswith(prefix) + # Verify the sanitized filename component is present + assert path.endswith("___etc_passwd.json") or "_etc_passwd" in path🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_utils.py` around lines 6 - 11, The test for get_state_file should assert that the filename portion is sanitized per the regex re.sub(r"[^a-zA-Z0-9_-]", "_", ...): update test_get_state_file_sanitizes_session_id_path_traversal to extract the basename from get_state_file("../../../etc/passwd", "seal_guard_state") and assert the basename contains the sanitized token (e.g., that the original "../../../etc/passwd" maps to a component like "___etc_passwd" or contains no illegal chars), and still keep the existing checks for no ".." and the prefix; reference the get_state_file function and the re.sub pattern when making the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/tests.yml:
- Line 25: The workflow step that runs "pip install pytest" should pin a
specific pytest version to ensure reproducible CI runs; update the run command
in the tests.yml job to install a fixed version (e.g., change the "pip install
pytest" invocation to install a specific release) or reference a development
requirements file (create a requirements-dev.txt with a pinned pytest entry and
change the workflow step to install from that file) so the CI consistently uses
the same pytest version.
---
Outside diff comments:
In `@hooks/secret_scanner.py`:
- Around line 27-31: The import list includes get_state_file which is unused
directly in this module; remove get_state_file from the from utils import ...
statement so only debug_log, load_shown, and save_shown are imported (leave
sys.path insertion and DEBUG_LOG/STATE_PREFIX unchanged) to avoid the
unused-import warning and rely on load_shown/save_shown which already call
get_state_file internally.
In `@hooks/security_guard.py`:
- Around line 21-26: The import list in hooks/security_guard.py includes
get_state_file but it is never used; remove get_state_file from the utils import
(keep debug_log, load_shown, save_shown) to eliminate the unused import and
mirror the fix applied in secret_scanner.py so the import statement only imports
symbols actually referenced.
In `@tests/test_utils.py`:
- Around line 6-11: The test for get_state_file should assert that the filename
portion is sanitized per the regex re.sub(r"[^a-zA-Z0-9_-]", "_", ...): update
test_get_state_file_sanitizes_session_id_path_traversal to extract the basename
from get_state_file("../../../etc/passwd", "seal_guard_state") and assert the
basename contains the sanitized token (e.g., that the original
"../../../etc/passwd" maps to a component like "___etc_passwd" or contains no
illegal chars), and still keep the existing checks for no ".." and the prefix;
reference the get_state_file function and the re.sub pattern when making the
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 09822466-a06e-400e-a7fe-c3ea9d7148de
📒 Files selected for processing (7)
.claude-plugin/plugin.json.github/workflows/tests.ymlhooks/secret_scanner.pyhooks/security_guard.pyhooks/utils.pytests/test_secret_scanner.pytests/test_utils.py
📜 Review details
🔇 Additional comments (19)
.github/workflows/tests.yml (3)
7-9: LGTM!
19-21: LGTM!
19-22: Pinned action SHAs match the claimed releases (v4/v5).
actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5matches thev4tag, andactions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065matches thev5tag..claude-plugin/plugin.json (1)
3-3: LGTM!hooks/secret_scanner.py (2)
102-117: The past review concern about substring matching has been addressed by switching tofullmatch()at line 246. The pattern now correctly requires the entire captured value to match a placeholder form rather than containing a placeholder substring.Also applies to: 242-247
254-302: LGTM!hooks/utils.py (4)
13-20: LGTM!
23-26: LGTM!
29-38: LGTM!
41-50: LGTM!hooks/security_guard.py (2)
266-276: LGTM!
241-285: LGTM!tests/test_utils.py (2)
14-17: LGTM!
20-31: LGTM!tests/test_secret_scanner.py (5)
9-16: LGTM!
19-22: LGTM!
57-70: LGTM!
73-82: LGTM!
36-41: ⚖️ Poor tradeoffFix test expectations: warn-only behavior is consistent with the scanner.
The
api_key_assignmentrule is configured withblock=False, andvalue_excludeonly skips the match when the entire quoted value full-matches the placeholder regex. For"live_test_token_1234567890"and"dummyservice_prod_key_1234567890",value_exclude.fullmatch(quoted_value)is false, so the rule correctly matches and returnsblock=False(warn), not “no match” or “block”.
| - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
| - run: pip install pytest |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider pinning the pytest version for reproducible builds.
Installing pytest without a version constraint means different test runs may use different pytest versions, potentially causing inconsistent behavior. While test dependencies are less critical than production dependencies, pinning improves reproducibility across environments and over time.
📌 Proposed fix to pin pytest version
- - run: pip install pytest
+ - run: pip install pytest==8.3.4Alternatively, create a requirements-dev.txt:
pytest==8.3.4
Then update the workflow:
- - run: pip install pytest
+ - run: pip install -r requirements-dev.txt📝 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.
| - run: pip install pytest | |
| - run: pip install pytest==8.3.4 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/tests.yml at line 25, The workflow step that runs "pip
install pytest" should pin a specific pytest version to ensure reproducible CI
runs; update the run command in the tests.yml job to install a fixed version
(e.g., change the "pip install pytest" invocation to install a specific release)
or reference a development requirements file (create a requirements-dev.txt with
a pinned pytest entry and change the workflow step to install from that file) so
the CI consistently uses the same pytest version.
1 participant
Summary
Consolidates the 16 open Jules PRs (#1–#16) into one coherent change. They overlapped heavily and conflicted with each other — three competing path-traversal fixes, two incompatible
hooks/utils.pyrefactors, and several PRs creating the same test file — so they could not be merged independently (GitHub marks each "mergeable" only because it diffs against base, not against its siblings). This PR folds in every sound idea and supersedes all 16.Changes
Security
session_idis now sanitized (re.sub(r"[^a-zA-Z0-9_-]", "_", ...)) before building the~/.claudestate path. Previously a craftedsession_idescaped the directory in both hooks. (from 🔒 Fix Path Traversal in hooks/secret_scanner.py and hooks/security_guard.py #6 / 🔒 Fix Path Traversal / Arbitrary File Write Vulnerability in Hooks State Management #12 / 🔒 Fix arbitrary file write / path traversal viasession_id#13)security_guard— replaced per-process-randomizedhash(command)withhashlib.sha256. The old key changed every invocation, so warnings were never deduplicated and the state file grew without bound. (from ⚡ perf: Use deterministic hashing for consistent cache keys across processes #5)Refactor / robustness
debug_log/get_state_file/load_shown/save_shownintohooks/utils.py(stdlib-only,OSError-safe). (from 🧹 Refactor duplicate state management logic in hooks #3)api_keyexclusion now distinguishes a dummy value from a dummy variable name (via a named-groupvalue_exclude), so a real secret assigned to e.g.test_api_keyis no longer missed. (from 🧹 Fix fragile regex exclude in secret scanner #10, reworked)Tests + CI
pytestsuite (tests/): everysecurity_guardrule, thesecret_scannerpatterns and their exclusions, a path-traversal regression, andmain()JSON-error handling. (consolidated from 🧪 Add comprehensive tests for security_guard.py rules #7, 🧪 Add test for JSON decode error in security_guard #8, 🧪 Add test coverage for secret scanner hook #9, 🧪 Add test coverage forscan_contentlogic insecret_scanner.py#14, 🧪 Testing Improvement: Add check_command security guard tests #15, 🧪 Add test for invalid JSON parsing in secret_scanner main #16).gitignore. (from 🔒 Fix arbitrary file write / path traversal viasession_id#13)Disposition of the 16 PRs
utils.pyrefactor (messy wrappers, redundant constant), incompatible signatures with #3security_guard, leftsecret_scannervulnerable (its.gitignore+ path-traversal test idea were kept)debug_logtoOSError+ printing to stderr makes a best-effort logger less crash-safe and noisier inside a hookVerification
python -m pytest→ 38 passed.utilsimport resolves at runtime.Summary by CodeRabbit
Tests
Refactor
Chores