Skip to content

hol-guard-pretool - #939

Open
kantorcodes wants to merge 6 commits into
agentforce314:mainfrom
kantorcodes:feat/hol-guard-pretool
Open

kantorcodes wants to merge 6 commits into
agentforce314:mainfrom
kantorcodes:feat/hol-guard-pretool

Conversation

@kantorcodes

Copy link
Copy Markdown

No description provided.

@ericleepi314 ericleepi314 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit 9fbfb75. Two issues need fixes before this can reliably guard Bash execution: subprocess failures can become non-blocking hook errors, and inspection does not use the directory in which Bash will run.

Validation: all 46 tests in test_hol_guard_pretool.py, test_hook_executor.py, and test_pretooluse_permission_decision.py passed. Additional subprocess checks through the real hook runner reproduced the non-blocking failures for a non-executable guard and invalid UTF-8 output, and confirmed the working-directory mismatch for both explicit and persisted Bash cwd. These checks used controlled guard executables; I also inspected the upstream HOL Guard command-inspection implementation.

Comment on lines +37 to +40
except FileNotFoundError:
return False, "guard_unavailable"
except subprocess.TimeoutExpired:
return False, "guard_timeout"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Convert subprocess launch and decoding failures into blocking exits

Only FileNotFoundError and TimeoutExpired are caught here. A hol-guard executable without execute permission raises PermissionError; invalid UTF-8 on either captured stream raises UnicodeDecodeError inside subprocess.run(text=True). Both escape main() and make this hook exit with code 1. The existing _execute_command_hook treats code 1 as a non-blocking error, so a Bash command that otherwise has permission can proceed without a successful guard check. I reproduced exit_code=1 and blocking_error=None through the real hook runner, including valid review JSON on stdout with invalid bytes on stderr. Handle launch OSError and decoding failures so these paths return BLOCK_EXIT, and add subprocess-level assertions that they actually deny the tool.

Comment on lines +30 to +35
result = subprocess.run(
["hol-guard", "command", "test", command, "--json"],
capture_output=True,
text=True,
timeout=TIMEOUT_SECONDS,
check=False,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Inspect the command in its effective Bash working directory

This subprocess inherits the ClawCodex process directory, but _bash_call executes in tool_input["cwd"] when supplied, otherwise context.cwd or context.workspace_root. The hook discards the explicit cwd, and execute_pre_tool_hooks does not send the persisted context.cwd. I confirmed that both cases invoke the guard in the launcher directory. HOL Guard command test passes Path.cwd() into inspect_command, whose checks depend on local paths, executables, and repository state; an allow result can therefore describe different files or configuration from those Bash will actually use. Carry the effective Bash cwd into the hook and set it on the guard subprocess, with coverage for explicit cwd and a prior directory change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants