Merged
Conversation
Allow .block files to scope protection rules to specific Claude Code subagent types using new `agents` and `disable_main_agent` config keys. This enables fine-grained control over which agent types are blocked, while maintaining full backward compatibility with existing configs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds subagent-aware blocking: new SubagentStart/SubagentStop hooks and a cross-platform subagent tracker; protect_directories.py is extended to resolve and merge agent-scoped fields and gate blocking decisions by agent type; docs, tests, and hook configs updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant HookEngine as Hook Engine
participant Runner as run-subagent-hook.cmd
participant Tracker as subagent_tracker.py
participant FS as Transcript FS
participant Protect as protect_directories.py
participant BlockFile as .block File
Client->>HookEngine: Trigger hook (SubagentStart / SubagentStop)
HookEngine->>Runner: execute run-subagent-hook.cmd
Runner->>Tracker: invoke tracker with event JSON
Tracker->>FS: lock & update subagents/.agent_types.json
FS-->>Tracker: unlock
Client->>Protect: Request protection decision (file edit/write)
Protect->>BlockFile: load merged .block config
Protect->>FS: read transcript + subagents/.agent_types.json to resolve agent_type
Protect->>Protect: should_apply_to_agent(config, agent_type)
alt Agent exempt / not matched
Protect-->>Client: allow operation
else Agent matches block rule
Protect-->>Client: block operation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
tests/conftest.py (1)
93-138: Consider reducing duplication with existingmake_*_inputhelpers.The
_with_agentvariants duplicate the base dict construction frommake_edit_input,make_write_input, andmake_bash_input. You could refactor to have the agent variants call the base helpers and inject the extra fields, or make the agent fields optional on the originals. Not critical for test utilities, just a note for future maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 93 - 138, The three helper functions make_edit_input_with_agent, make_write_input_with_agent, and make_bash_input_with_agent duplicate payload construction; refactor by either (a) updating the base helpers make_edit_input, make_write_input, and make_bash_input to accept optional tool_use_id and transcript_path parameters and set those fields when provided, or (b) have each _with_agent variant call its corresponding base helper (make_edit_input, make_write_input, make_bash_input) to get the JSON string or dict, parse/convert to a dict if needed, inject "tool_use_id" and "transcript_path" when non-empty, and then return the JSON string—this removes duplication while preserving behavior and signatures of the helper utilities.CHANGELOG.md (1)
7-7: Missing PR reference in changelog entry.Existing entries consistently include PR references (e.g.,
(PR#24),(PR#23)). Add(PR#26)to this entry to match the established format. As per coding guidelines, updateCHANGELOG.mdwith "brief descriptions and PR references".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 7, Update the changelog entry titled "Subagent-aware blocking rules" so it includes the PR reference "(PR `#26`)"; specifically, append the PR reference to the end of the existing sentence describing `.block` files and the new `agents` and `disable_main_agent` keys so the entry matches the format used by other entries.tests/test_agent_rules.py (1)
449-479: Dict iteration order dependency in multi-subagent tests.
test_multiple_subagents_first_match(line 449) andtest_multiple_subagents_second_match(line 465) rely onresolve_agent_typeiterating theagent_mapdict in insertion order. This is guaranteed in Python 3.7+, so it's fine for the 3.8+ requirement, but worth a brief inline comment for future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_agent_rules.py` around lines 449 - 479, The two tests (test_multiple_subagents_first_match and test_multiple_subagents_second_match) depend on resolve_agent_type iterating the provided agent_map in insertion order to decide which subagent matches first; add a brief inline comment in the tests (near the calls to create_agent_tracking_file or the resolve_agent_type invocation) stating that the test intentionally relies on Python dict insertion order (preserved in 3.7+) so readers know this dependency and won’t incorrectly refactor agent_map into an order-insensitive structure.hooks/subagent_tracker.py (1)
74-92: Lock file opened with"w"(truncate) may cause a brief window without lock content on Windows.Opening the lock file with
"w"truncates it. On Windows,msvcrt.lockingrequires bytes to exist for the range being locked. If two processes race to open the lock file simultaneously, one might truncate it between another's write and lock call. ThepassonOSErrormakes this best-effort, which is reasonable for this use case.If you want to harden this slightly, consider
"a+"mode to avoid truncation, seeking to 0 before locking:🔒 Optional: avoid lock file truncation
- with open(lock_path, "w", encoding="utf-8") as lock_f: + with open(lock_path, "a+", encoding="utf-8") as lock_f:Then adjust
_lock_filefor Windows to handle the case where the file already has content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/subagent_tracker.py` around lines 74 - 92, In _write_tracking_file replace opening the lock file with "w" by opening with "a+" and seek(0) before calling _lock_file to avoid truncation races on Windows (i.e., change the open(lock_path, "w", ...) to open(lock_path, "a+", ...), call lock_f.seek(0) just before _lock_file), and update the _lock_file/_unlock_file implementations to handle a possibly non-empty file on Windows (ensure _lock_file uses the current file size/position when calling msvcrt.locking and _unlock_file releases the same range); keep the existing OSError pass behavior if desired.hooks/protect_directories.py (1)
1073-1162: Extract repeated agent-gating pattern to reduce duplication.The same agent-resolution-and-gating pattern (check
_config_has_agent_rules→ lazy resolve →should_apply_to_agent) is repeated three times inmain(). This makes it easy to introduce inconsistencies when modifying the logic later.Consider extracting a small helper:
♻️ Suggested helper to eliminate duplication
def _agent_exempt(config, data, agent_state): """Check if the current agent is exempt from this config's rules. agent_state is a mutable dict with 'resolved' and 'type' keys (lazy cache). """ if not _config_has_agent_rules(config): return False if not agent_state["resolved"]: agent_state["type"] = resolve_agent_type(data) agent_state["resolved"] = True return not should_apply_to_agent(config, agent_state["type"])Then in
main()each block simplifies to:agent_state = {"resolved": False, "type": None} # ... for each path ... if protection_info: config = protection_info["config"] if not _agent_exempt(config, data, agent_state): block_result = test_should_block(...) # ... handle block ... # directory check if dir_info: if not _agent_exempt(dir_info["config"], data, agent_state): block_with_message(...) # descendant check if desc_info: if not _agent_exempt(desc_info["config"], data, agent_state): block_with_message(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/protect_directories.py` around lines 1073 - 1162, The main() function repeats the "agent gating" pattern (_config_has_agent_rules → lazy resolve via resolve_agent_type → should_apply_to_agent) in three places; extract that logic into a small helper (e.g., _agent_exempt or similar) that accepts a config, data, and a mutable agent_state cache (with keys like "resolved" and "type") and returns True if the agent is exempt, then replace the three duplicate blocks in main() (the checks around protection_info["config"], dir_info["config"], and desc_info["config"]) with calls to this helper so you only resolve the agent once and consistently skip blocking when exempt.tests/test_subagent_tracker.py (1)
60-63: Duplicatehooks_dirfixture — already defined inconftest.py.
conftest.py(lines 19–21) defines the samehooks_dirfixture, and pytest automatically makes it available to all tests undertests/. This local definition shadows it unnecessarily.♻️ Remove the duplicate fixture
-@pytest.fixture -def hooks_dir(): - """Path to the hooks directory.""" - return Path(__file__).parent.parent / "hooks" - -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_subagent_tracker.py` around lines 60 - 63, Remove the locally declared pytest fixture named hooks_dir in the tests/test_subagent_tracker.py test module because it duplicates the hooks_dir fixture provided globally in conftest.py; delete the def hooks_dir() block so tests use the shared fixture and avoid shadowing, and run pytest to ensure imports still resolve.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/protect_directories.py`:
- Around line 220-226: The code stores data["agents"] and
data["disable_main_agent"] into config without type checks, causing unintended
behavior (e.g., substring matches when agents is a string or non-boolean values
for disable_main_agent); add validation in the parsing block to ensure "agents"
is a list (or list-like of strings) before assigning to config["agents"] and set
config["has_agents_key"] only when the type is correct, and ensure
"disable_main_agent" is strictly a bool (coerce or reject non-bool) before
assigning config["disable_main_agent"] and setting
config["has_disable_main_agent_key"]; update related logic in
should_apply_to_agent to rely on these validated types.
In `@hooks/run-subagent-hook.cmd`:
- Around line 1-34: Update repository .gitattributes to ensure Windows batch
files get CRLF endings so the new polyglot script run-subagent-hook.cmd is
parsed correctly on Windows; add or change the rule to include hooks/*.cmd with
"text eol=crlf" (so both hooks/run-hook.cmd and hooks/run-subagent-hook.cmd are
covered) and commit the .gitattributes update.
In `@hooks/subagent_tracker.py`:
- Line 19: Remove the unused import by deleting the `from pathlib import Path`
statement in the file; the symbol `Path` is not referenced anywhere in this
module, so simply remove that import line (the `from pathlib import Path`
import) to clean up the unused dependency and satisfy linting.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 7: Update the changelog entry titled "Subagent-aware blocking rules" so
it includes the PR reference "(PR `#26`)"; specifically, append the PR reference
to the end of the existing sentence describing `.block` files and the new
`agents` and `disable_main_agent` keys so the entry matches the format used by
other entries.
In `@hooks/protect_directories.py`:
- Around line 1073-1162: The main() function repeats the "agent gating" pattern
(_config_has_agent_rules → lazy resolve via resolve_agent_type →
should_apply_to_agent) in three places; extract that logic into a small helper
(e.g., _agent_exempt or similar) that accepts a config, data, and a mutable
agent_state cache (with keys like "resolved" and "type") and returns True if the
agent is exempt, then replace the three duplicate blocks in main() (the checks
around protection_info["config"], dir_info["config"], and desc_info["config"])
with calls to this helper so you only resolve the agent once and consistently
skip blocking when exempt.
In `@hooks/subagent_tracker.py`:
- Around line 74-92: In _write_tracking_file replace opening the lock file with
"w" by opening with "a+" and seek(0) before calling _lock_file to avoid
truncation races on Windows (i.e., change the open(lock_path, "w", ...) to
open(lock_path, "a+", ...), call lock_f.seek(0) just before _lock_file), and
update the _lock_file/_unlock_file implementations to handle a possibly
non-empty file on Windows (ensure _lock_file uses the current file size/position
when calling msvcrt.locking and _unlock_file releases the same range); keep the
existing OSError pass behavior if desired.
In `@tests/conftest.py`:
- Around line 93-138: The three helper functions make_edit_input_with_agent,
make_write_input_with_agent, and make_bash_input_with_agent duplicate payload
construction; refactor by either (a) updating the base helpers make_edit_input,
make_write_input, and make_bash_input to accept optional tool_use_id and
transcript_path parameters and set those fields when provided, or (b) have each
_with_agent variant call its corresponding base helper (make_edit_input,
make_write_input, make_bash_input) to get the JSON string or dict, parse/convert
to a dict if needed, inject "tool_use_id" and "transcript_path" when non-empty,
and then return the JSON string—this removes duplication while preserving
behavior and signatures of the helper utilities.
In `@tests/test_agent_rules.py`:
- Around line 449-479: The two tests (test_multiple_subagents_first_match and
test_multiple_subagents_second_match) depend on resolve_agent_type iterating the
provided agent_map in insertion order to decide which subagent matches first;
add a brief inline comment in the tests (near the calls to
create_agent_tracking_file or the resolve_agent_type invocation) stating that
the test intentionally relies on Python dict insertion order (preserved in 3.7+)
so readers know this dependency and won’t incorrectly refactor agent_map into an
order-insensitive structure.
In `@tests/test_subagent_tracker.py`:
- Around line 60-63: Remove the locally declared pytest fixture named hooks_dir
in the tests/test_subagent_tracker.py test module because it duplicates the
hooks_dir fixture provided globally in conftest.py; delete the def hooks_dir()
block so tests use the shared fixture and avoid shadowing, and run pytest to
ensure imports still resolve.
Comment on lines
+1
to
+34
| : << 'CMDBLOCK' | ||
| @echo off | ||
| REM Polyglot entry point - works as both Windows batch and Unix shell | ||
| REM Calls subagent_tracker.py with Python (silent exit if Python not found) | ||
|
|
||
| setlocal EnableDelayedExpansion | ||
| set "HOOK_DIR=%~dp0" | ||
|
|
||
| REM Call Python to track subagent events | ||
| where python >nul 2>&1 | ||
| if %errorlevel% equ 0 ( | ||
| python "%HOOK_DIR%subagent_tracker.py" | ||
| exit /b 0 | ||
| ) | ||
|
|
||
| REM Python not found - tracker is optional, exit silently | ||
| exit /b 0 | ||
| CMDBLOCK | ||
|
|
||
| # Unix: here-doc above discards batch code | ||
| HOOK_DIR="$(cd "$(dirname "$0")" && pwd)" | ||
|
|
||
| # Call Python to track subagent events | ||
| if command -v python3 >/dev/null 2>&1; then | ||
| python3 "$HOOK_DIR/subagent_tracker.py" | ||
| exit 0 | ||
| fi | ||
| if command -v python >/dev/null 2>&1; then | ||
| python "$HOOK_DIR/subagent_tracker.py" | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Python not found - tracker is optional, exit silently | ||
| exit 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check line endings of .cmd files and whether .gitattributes handles them
echo "=== .gitattributes ==="
fd '.gitattributes' --exec cat {} 2>/dev/null || echo "No .gitattributes found"
echo ""
echo "=== Line endings in .cmd files ==="
fd '\.cmd$' hooks/ --exec file {} 2>/dev/null
echo ""
echo "=== Check first few bytes of run-subagent-hook.cmd ==="
if [ -f "hooks/run-subagent-hook.cmd" ]; then
od -c "hooks/run-subagent-hook.cmd" | head -20
else
echo "File not found"
fi
echo ""
echo "=== Check first few bytes of run-hook.cmd ==="
if [ -f "hooks/run-hook.cmd" ]; then
od -c "hooks/run-hook.cmd" | head -20
else
echo "File not found"
fiRepository: kodroi/block
Length of output: 318
🏁 Script executed:
#!/bin/bash
# Search for .cmd files in entire repo
echo "=== All .cmd files in repo ==="
find . -name "*.cmd" -type f 2>/dev/null
echo ""
echo "=== Contents of hooks/ directory ==="
ls -la hooks/ 2>/dev/null || echo "hooks/ directory not found"
echo ""
echo "=== Check if run-subagent-hook.cmd exists ==="
if [ -f "./hooks/run-subagent-hook.cmd" ]; then
echo "File exists. First 100 bytes (hex):"
head -c 100 "./hooks/run-subagent-hook.cmd" | hexdump -C 2>/dev/null || xxd - <(head -c 100 "./hooks/run-subagent-hook.cmd") 2>/dev/null || python3 -c "
import sys
data = open('./hooks/run-subagent-hook.cmd', 'rb').read(100)
print(' '.join(f'{b:02x}' for b in data))
print('Line endings: CRLF' if b'\r\n' in data else 'Line endings: LF')
"
else
echo "File does not exist"
fi
echo ""
echo "=== Check if .gitattributes exists ==="
[ -f ".gitattributes" ] && cat ".gitattributes" || echo "No .gitattributes file"Repository: kodroi/block
Length of output: 1204
Add run-subagent-hook.cmd to .gitattributes for CRLF line endings.
The new polyglot script currently has LF-only line endings, while the existing hooks/run-hook.cmd is configured in .gitattributes with text eol=crlf. Update .gitattributes to cover both .cmd files consistently:
hooks/*.cmd text eol=crlf
This ensures Windows batch parsing works reliably on all systems where git may normalize line endings differently.
🧰 Tools
🪛 Blinter (1.0.112)
[error] 18-18: Unreachable code after EXIT or GOTO. Explanation: Code after EXIT or GOTO statements will never execute. Recommendation: Remove unreachable code or restructure script logic. Context: Code after EXIT on line 17 will never execute. Code after EXIT on line 26 will never execute. Code after EXIT on line 30 will never execute
(E008)
[error] 1-1: Unix line endings detected. Explanation: Batch file uses Unix line endings (LF-only) which can cause GOTO/CALL label parsing failures and script malfunction due to Windows batch parser 512-byte boundary bugs. Recommendation: Convert file to Windows line endings (CRLF). Use tools like dos2unix, notepad++, or configure git with 'git config core.autocrlf true'. Context: File uses Unix line endings (LF-only) - 34 LF sequences found
(E018)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/run-subagent-hook.cmd` around lines 1 - 34, Update repository
.gitattributes to ensure Windows batch files get CRLF endings so the new
polyglot script run-subagent-hook.cmd is parsed correctly on Windows; add or
change the rule to include hooks/*.cmd with "text eol=crlf" (so both
hooks/run-hook.cmd and hooks/run-subagent-hook.cmd are covered) and commit the
.gitattributes update.
Owner
Author
|
Resolving CodeRabbit comment #2836162580 ✅ Addressed and implemented Note: Added type validation: agents must be a list and disable_main_agent must be a bool. Invalid types are silently ignored (keys not set in config). Resolved via CodeRabbit MCP |
Owner
Author
|
Resolving CodeRabbit comment #2836162582 ✅ Addressed and implemented Note: Updated .gitattributes to use hooks/*.cmd glob pattern with eol=lf. Note: kept eol=lf (not crlf) because these are polyglot scripts that run as both batch and shell — CRLF would break the Unix shell portion. Resolved via CodeRabbit MCP |
Owner
Author
|
Resolving CodeRabbit comment #2836162583 ✅ Addressed and implemented Note: Removed unused Resolved via CodeRabbit MCP |
Add type validation for agents/disable_main_agent config, extract _agent_exempt helper to reduce duplication in main(), fix lock file truncation race, update .gitattributes glob, remove unused import and duplicate fixture, add README agent-specific rules section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ot reads) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
tests/test_subagent_tracker.py (1)
24-29:subprocess.runhas no timeout — a hung tracker will stall the entire test suite.
msvcrt.LK_LOCK(Windows) andfcntl.LOCK_EX(POSIX) both block indefinitely if the lock is held. A stale lock file (e.g., after a process crash) would cause tests to hang silently.⏱️ Suggested fix
result = subprocess.run( [sys.executable, str(tracker_script)], input=input_json, capture_output=True, text=True, + timeout=10, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_subagent_tracker.py` around lines 24 - 29, The test invokes subprocess.run([sys.executable, str(tracker_script)], input=input_json, capture_output=True, text=True) with no timeout, so a hung tracker can stall the suite; update the call in tests/test_subagent_tracker.py to pass a reasonable timeout (e.g., a few seconds), and wrap the call in a try/except that catches subprocess.TimeoutExpired to fail the test cleanly (fail the assertion or call pytest.fail) and capture/kill the child if needed so stale locks don't hang CI; reference the subprocess.run call and the variables tracker_script and input_json when making the change.hooks/subagent_tracker.py (1)
29-33: Unnecessaryf.write(" ")causes the.lockfile to grow indefinitely on Windows.
open(lock_path, "a+")opens in append mode — writes always go to EOF regardless ofseek. Each call to_lock_fileappends one byte to the lock file.msvcrt.lockingoperates on byte ranges and doesn't require the file to have content at the locked range, so the write is not needed and the accumulated bytes serve no purpose.🧹 Proposed fix
def _lock_file(f): """Acquire an exclusive lock on a file (platform-specific, blocking).""" try: if sys.platform == "win32": import msvcrt - # Ensure file has content to lock against - f.write(" ") - f.flush() - f.seek(0) msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, _LOCK_SIZE) else: import fcntl fcntl.flock(f, fcntl.LOCK_EX) except (OSError, ImportError): pass # Best-effort lockingAlso update
_write_tracking_fileand_remove_from_tracking_fileto remove the now-redundantlock_f.seek(0)before_lock_file:with open(lock_path, "a+", encoding="utf-8") as lock_f: - lock_f.seek(0) _lock_file(lock_f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/subagent_tracker.py` around lines 29 - 33, Remove the unnecessary file write and seek before acquiring the Windows lock in _lock_file: stop calling f.write(" "), f.flush(), and f.seek(0) since open(..., "a+") already opens the file and msvcrt.locking does not require pre-populated bytes; simply open the file and call msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, _LOCK_SIZE) in _lock_file. Also update _write_tracking_file and _remove_from_tracking_file to drop the redundant lock_f.seek(0) calls made immediately before invoking _lock_file so they no longer alter the file pointer before locking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/protect_directories.py`:
- Around line 549-553: The conditional block that returns True/False for the
main agent is verbose; replace the if is_main: ... block with a direct boolean
return that evaluates whether it's the main agent and not disabled (i.e. return
is_main and not (has_disable_key and disable_main)), referencing the variables
is_main, has_disable_key, and disable_main to locate the code.
In `@hooks/subagent_tracker.py`:
- Around line 16-18: The import block in hooks/subagent_tracker.py is
misformatted (I001); fix it by running the formatter linter (e.g., `ruff check
--fix hooks/subagent_tracker.py`) or manually reformat the top-level imports so
standard library modules (json, os, sys) are grouped, sorted, and separated from
other import groups, ensuring the import block in that file follows PEP8/ruff
ordering and spacing.
In `@README.md`:
- Line 145: The README incorrectly states the PreToolUse hook blocks agents from
reading sensitive files; instead, update the doc to clarify that PreToolUse only
intercepts modification tools (Edit, Write, NotebookEdit, Bash) and does not
intercept read-only tools (Read, Glob, LS), so scope protection applies to
writes/edits only; reference the PreToolUse hook and the specific tool names in
the sentence to make the distinction explicit.
In `@tests/test_agent_rules.py`:
- Around line 12-28: Remove the unused imports sys, pytest and
create_local_block_file from the import list to fix F401, and move the
importlib.util import into the top (stdlib) import block so import ordering
follows isort (I001); keep the existing comment explaining why importlib.util is
used and only import it where needed, and update the from tests.conftest import
(...) list to exclude create_local_block_file.
---
Nitpick comments:
In `@hooks/subagent_tracker.py`:
- Around line 29-33: Remove the unnecessary file write and seek before acquiring
the Windows lock in _lock_file: stop calling f.write(" "), f.flush(), and
f.seek(0) since open(..., "a+") already opens the file and msvcrt.locking does
not require pre-populated bytes; simply open the file and call
msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, _LOCK_SIZE) in _lock_file. Also
update _write_tracking_file and _remove_from_tracking_file to drop the redundant
lock_f.seek(0) calls made immediately before invoking _lock_file so they no
longer alter the file pointer before locking.
In `@tests/test_subagent_tracker.py`:
- Around line 24-29: The test invokes subprocess.run([sys.executable,
str(tracker_script)], input=input_json, capture_output=True, text=True) with no
timeout, so a hung tracker can stall the suite; update the call in
tests/test_subagent_tracker.py to pass a reasonable timeout (e.g., a few
seconds), and wrap the call in a try/except that catches
subprocess.TimeoutExpired to fail the test cleanly (fail the assertion or call
pytest.fail) and capture/kill the child if needed so stale locks don't hang CI;
reference the subprocess.run call and the variables tracker_script and
input_json when making the change.
When `agents` key is present in a .block file, the main agent is now
exempt — the file only applies to subagents. This simplifies config:
`{"agents": ["TestCreator"]}` blocks TestCreator without affecting
the main agent, no `disable_main_agent` needed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kodroi
force-pushed
the
feat/subagent-aware-blocking
branch
from
e104b7a to
3bd7cd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
hooks/subagent_tracker.py (1)
82-91: Consider atomic temp-file +os.replaceto prevent tracking file corruption.The current pattern truncates the tracking file (via
open(..., "w")) before completing the write. A crash or SIGKILL mid-write leaves the file empty, causingresolve_agent_typeto returnNonefor all subsequent tool calls until the nextSubagentStart— potentially exempting a subagent that should be blocked. The same applies in_remove_from_tracking_file(line 107).♻️ Proposed atomic write helper
+import tempfile + def _write_tracking_file(tracking_path: str, agent_map: dict) -> None: """Write the tracking file with file locking.""" os.makedirs(os.path.dirname(tracking_path), exist_ok=True) lock_path = tracking_path + ".lock" try: with open(lock_path, "a+", encoding="utf-8") as lock_f: lock_f.seek(0) _lock_file(lock_f) try: # Re-read inside lock to avoid races current = _read_tracking_file(tracking_path) current.update(agent_map) - # Write atomically-ish - with open(tracking_path, "w", encoding="utf-8") as f: - json.dump(current, f) + # Write atomically via temp file + rename + dir_ = os.path.dirname(tracking_path) + with tempfile.NamedTemporaryFile( + "w", dir=dir_, delete=False, encoding="utf-8", suffix=".tmp" + ) as tmp: + json.dump(current, tmp) + tmp_path = tmp.name + os.replace(tmp_path, tracking_path) finally: _unlock_file(lock_f) except OSError: passApply the same change to
_remove_from_tracking_file(lines 107–108).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/subagent_tracker.py` around lines 82 - 91, The tracking-file write must be made atomic: instead of opening tracking_path with "w" and truncating directly, write the updated JSON to a temporary file in the same directory (e.g., tracking_path + ".tmp" or use tempfile.NamedTemporaryFile with dir=os.path.dirname(tracking_path)), fsync the file and its directory if possible, close it, then atomically replace the original with os.replace(temp_path, tracking_path); do this inside the same lock/unlock sequence used around the existing write in the function that calls _read_tracking_file (and ensure _unlock_file(lock_f) still runs in finally). Apply the identical atomic-temp-write + os.replace pattern to _remove_from_tracking_file as well so both additions and removals cannot leave the tracking file truncated on crash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/protect_directories.py`:
- Around line 469-471: The CI mypy errors come from implicit Any returned by
untyped dict lookups; in _config_has_agent_rules wrap the boolean expression
with bool(...) so it returns a proper bool, and in resolve_agent_type use the
already-imported cast from typing to cast the dict lookup result to str (e.g.,
cast(str, config.get(...))) before returning to ensure the function returns str
rather than Any; update the return expressions in the functions
_config_has_agent_rules and resolve_agent_type accordingly.
- Around line 474-483: _tool_use_id_in_transcript currently does a bare
substring search which can yield false positives for short IDs (e.g., "tu_1"
matching "tu_10"); change it to perform a whole-token/word-boundary match using
a regex: import re and use re.search with escaped tool_use_id wrapped with
lookarounds like (?<!\w)RE(?!\w) (where RE is re.escape(tool_use_id)) so only
exact token matches return True, keep the same file reading and OSError
handling, and otherwise return False.
In `@README.md`:
- Around line 147-150: The fenced code block showing the directory snippet (the
triple-backtick block containing "tests/ └── .block → {\"agents\":
[\"TDD_agent\"], \"disable_main_agent\": true}") should include a language
specifier (e.g., text) after the opening backticks; update the README's code
fence around that snippet to use a language tag like "text" so syntax
highlighters render it correctly.
- Around line 145-152: The README's example prose incorrectly describes the
semantics of the .block "agents" key (it acts as a block-list, not an
allow-list); update the example text around the tests/ .block entry to state
that {"agents": ["TDD_agent"], "disable_main_agent": true} blocks only TDD_agent
from writing to tests/ while the main agent and all other subagents remain
exempt, and add a clarifying note that the current "agents" semantics correspond
to should_apply_to_agent's block behavior and therefore cannot be used to
implement an allow-list ("only allow TDD_agent") without changing the code
(e.g., should_apply_to_agent semantics).
In `@tests/test_agent_rules.py`:
- Around line 30-35: The import currently assumes
importlib.util.spec_from_file_location returned a ModuleSpec with a loader;
guard the optional values by checking that _spec is not None and _spec.loader is
not None before calling _spec.loader.exec_module(_pd). If either is None, raise
a clear RuntimeError or use pytest.skip with a descriptive message referencing
the target hook ("protect_directories.py") so tests fail fast/skip instead of
raising AttributeError; adjust the symbols _spec, _spec.loader, and _pd in the
import block accordingly.
---
Nitpick comments:
In `@hooks/subagent_tracker.py`:
- Around line 82-91: The tracking-file write must be made atomic: instead of
opening tracking_path with "w" and truncating directly, write the updated JSON
to a temporary file in the same directory (e.g., tracking_path + ".tmp" or use
tempfile.NamedTemporaryFile with dir=os.path.dirname(tracking_path)), fsync the
file and its directory if possible, close it, then atomically replace the
original with os.replace(temp_path, tracking_path); do this inside the same
lock/unlock sequence used around the existing write in the function that calls
_read_tracking_file (and ensure _unlock_file(lock_f) still runs in finally).
Apply the identical atomic-temp-write + os.replace pattern to
_remove_from_tracking_file as well so both additions and removals cannot leave
the tracking file truncated on crash.
Comment on lines
+474
to
+483
| def _tool_use_id_in_transcript(transcript_path: str, tool_use_id: str) -> bool: | ||
| """Check if a tool_use_id appears in a transcript file (simple string search).""" | ||
| try: | ||
| with open(transcript_path, encoding="utf-8") as f: | ||
| for line in f: | ||
| if tool_use_id in line: | ||
| return True | ||
| except OSError: | ||
| pass | ||
| return False |
There was a problem hiding this comment.
_tool_use_id_in_transcript performs a bare substring search — false-positive risk with short IDs.
if tool_use_id in line matches any line where tool_use_id is a substring. A short ID such as "tu_1" matches a line containing "tu_10", misattributing the tool call to the wrong agent. Production Claude Code IDs are long UUIDs, so this is unlikely in practice, but the function's "simple string search" approach could cause incorrect agent resolution during local/test runs with short synthetic IDs.
♻️ Suggested fix — whole-word boundary check
def _tool_use_id_in_transcript(transcript_path: str, tool_use_id: str) -> bool:
- """Check if a tool_use_id appears in a transcript file (simple string search)."""
+ """Check if a tool_use_id appears as an exact value in a transcript file."""
try:
with open(transcript_path, encoding="utf-8") as f:
for line in f:
- if tool_use_id in line:
+ # Use JSON-value boundary to avoid substring false positives.
+ if f'"{tool_use_id}"' in line:
return True
except OSError:
pass
return False📝 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
| def _tool_use_id_in_transcript(transcript_path: str, tool_use_id: str) -> bool: | |
| """Check if a tool_use_id appears in a transcript file (simple string search).""" | |
| try: | |
| with open(transcript_path, encoding="utf-8") as f: | |
| for line in f: | |
| if tool_use_id in line: | |
| return True | |
| except OSError: | |
| pass | |
| return False | |
| def _tool_use_id_in_transcript(transcript_path: str, tool_use_id: str) -> bool: | |
| """Check if a tool_use_id appears as an exact value in a transcript file.""" | |
| try: | |
| with open(transcript_path, encoding="utf-8") as f: | |
| for line in f: | |
| # Use JSON-value boundary to avoid substring false positives. | |
| if f'"{tool_use_id}"' in line: | |
| return True | |
| except OSError: | |
| pass | |
| return False |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/protect_directories.py` around lines 474 - 483,
_tool_use_id_in_transcript currently does a bare substring search which can
yield false positives for short IDs (e.g., "tu_1" matching "tu_10"); change it
to perform a whole-token/word-boundary match using a regex: import re and use
re.search with escaped tool_use_id wrapped with lookarounds like (?<!\w)RE(?!\w)
(where RE is re.escape(tool_use_id)) so only exact token matches return True,
keep the same file reading and OSError handling, and otherwise return False.
Add explicit bool()/str() casts to satisfy mypy no-any-return checks. Replace msvcrt.LK_LOCK (1s timeout) with LK_NBLCK + custom retry loop (10s timeout) to prevent silent lock failures on slow CI runners. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix README: agents key lists blocked subagents, not allowed ones - Add language specifier to README code fence - Guard importlib spec/loader against None in test_agent_rules.py - Add timeout=10 to subprocess.run in test_subagent_tracker.py Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hooks/subagent_tracker.py (1)
84-124: Redundantlock_f.seek(0)before_lock_filein both locking sites.
_lock_filealready handles its own file positioning: on Windows it doesf.seek(0)internally (after thewrite/flush); on POSIX,flockignores file position entirely. The pre-seek at line 91 and 114 is a no-op in all cases.♻️ Proposed cleanup (same change applies to both `_write_tracking_file` and `_remove_from_tracking_file`)
with open(lock_path, "a+", encoding="utf-8") as lock_f: - lock_f.seek(0) _lock_file(lock_f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/subagent_tracker.py` around lines 84 - 124, Remove the redundant lock_f.seek(0) calls before acquiring the lock in both _write_tracking_file and _remove_from_tracking_file: the helper _lock_file already handles any required file positioning, so delete the lock_f.seek(0) lines that appear where lock_f is opened and before calling _lock_file (referencing the variable lock_f and the call to _lock_file) to avoid no-op seeks.tests/test_agent_rules.py (1)
791-815:"code-reviewer"in the block list is never exercised by this test.The block file declares
["Explore", "code-reviewer"], but the tracking file uses"Plan"as the second agent type. The test verifies Explore (blocked) and Plan (not blocked), but never asserts that a"code-reviewer"agent is actually blocked.♻️ Proposed addition — add a code-reviewer assertion
+ # code-reviewer → blocked (in list) + create_agent_tracking_file(tmp_path, {"agent_cr": "code-reviewer"}) + create_agent_transcript(tmp_path, "agent_cr", ["tu_cr"]) + input_json = make_edit_input_with_agent(target, "tu_cr", str(transcript)) + code, stdout, _ = run_hook(hooks_dir, input_json) + assert is_blocked(stdout)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_agent_rules.py` around lines 791 - 815, The test test_two_different_types_resolved_correctly currently never exercises the "code-reviewer" entry in the block list; update the test to add a third check for a code-reviewer agent: add an agent tracking entry mapping a new agent id (e.g., "agent_code_reviewer") to "code-reviewer" via create_agent_tracking_file, create its transcript with create_agent_transcript (e.g., "agent_code_reviewer" -> ["tu_code_reviewer"]), then call make_edit_input_with_agent with that id and run_hook, and assert the result is blocked (use is_blocked on stdout) just like the Explore assertion so the "code-reviewer" blocker is actually validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@hooks/protect_directories.py`:
- Around line 474-483: _tool_use_id_in_transcript currently does a bare
substring search which yields false positives (e.g., "tu_1" matching "tu_10");
change the check to match the exact ID as a discrete token using a regex with
re.escape(tool_use_id) — e.g., compile a pattern that enforces token boundaries
like r'(?<!\w){}(?!\w)'.format(re.escape(tool_use_id)) or use a quoted-delimited
pattern r'["\']{}["\']' and call re.search on each line; update
_tool_use_id_in_transcript to use this regex search while preserving the
file-open and OSError handling.
---
Nitpick comments:
In `@hooks/subagent_tracker.py`:
- Around line 84-124: Remove the redundant lock_f.seek(0) calls before acquiring
the lock in both _write_tracking_file and _remove_from_tracking_file: the helper
_lock_file already handles any required file positioning, so delete the
lock_f.seek(0) lines that appear where lock_f is opened and before calling
_lock_file (referencing the variable lock_f and the call to _lock_file) to avoid
no-op seeks.
In `@tests/test_agent_rules.py`:
- Around line 791-815: The test test_two_different_types_resolved_correctly
currently never exercises the "code-reviewer" entry in the block list; update
the test to add a third check for a code-reviewer agent: add an agent tracking
entry mapping a new agent id (e.g., "agent_code_reviewer") to "code-reviewer"
via create_agent_tracking_file, create its transcript with
create_agent_transcript (e.g., "agent_code_reviewer" -> ["tu_code_reviewer"]),
then call make_edit_input_with_agent with that id and run_hook, and assert the
result is blocked (use is_blocked on stdout) just like the Explore assertion so
the "code-reviewer" blocker is actually validated.
Remove f.write(" ") from _lock_file — msvcrt.locking can lock beyond
EOF so no file content is needed. Switch lock file from text "a+" to
binary "a+b" to avoid text mode translation issues on Windows CI.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kodroi
pushed a commit
that referenced
this pull request
Feb 21, 2026
PR #26 was squash-merged without +semver: minor, causing GitVersion to produce 1.2.1 instead of 1.3.0. Fix the opencode package version and trigger correct version calculation. +semver: minor Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
1 participant
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
.blockfiles can now scope protection rules to specific Claude Code subagent types using newagentsanddisable_main_agentconfiguration keysSubagentStart/SubagentStophooks maintains a mapping of active agent IDs to types, enabling tool invocations to be correlated to the originating agent.blockfiles without agent keys continue to block all agents identicallyConfig examples
{"agents": ["Explore"]}Blocks main agent + Explore subagents. Other subagent types are allowed.
{"disable_main_agent": true}Allows main agent. All subagents are blocked.
{"agents": ["Explore"], "disable_main_agent": true}Allows main agent. Blocks only Explore subagents. Other subagent types are allowed.
Changes
hooks/protect_directories.py— New config fields (agents,disable_main_agent), agent resolution (resolve_agent_type), decision logic (should_apply_to_agent), lazy integration inmain()hooks/subagent_tracker.py— New script handling SubagentStart/Stop events with file-locked trackinghooks/run-subagent-hook.cmd— Polyglot wrapper for the trackerhooks/hooks.json— Registered SubagentStart/SubagentStop hookstests/conftest.py— Agent-aware test helperstests/test_agent_rules.py— 60 tests (unit, parsing, merging, resolution, e2e, parallel)tests/test_subagent_tracker.py— 15 tests (start, stop, concurrency, lifecycle)CHANGELOG.md— Unreleased entryTest plan
pytest tests/ -v), 0 failures.blockwithagents: ["Explore"], verify main agent allowed, Explore blocked, other subagents allowed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests