From 6eb3587dcc66a848cb32b9d9c24e51d52aab7522 Mon Sep 17 00:00:00 2001 From: Michael Panchenko Date: Mon, 4 May 2026 16:59:37 +0200 Subject: [PATCH] Hooks: adjust output to work for codex, extended docs --- docs/02-usage/030_clients.md | 64 ++++++++++++++++- src/serena/hooks.py | 136 +++++++++++++++++++++++++++-------- test/serena/test_hooks.py | 122 +++++++++++++++++++++++-------- 3 files changed, 260 insertions(+), 62 deletions(-) diff --git a/docs/02-usage/030_clients.md b/docs/02-usage/030_clients.md index cbfe0d4e..a9378826 100644 --- a/docs/02-usage/030_clients.md +++ b/docs/02-usage/030_clients.md @@ -318,9 +318,67 @@ The Codex app does not start a session in the project's directory, so when using asking Codex to "Activate the current dir as project using serena" at the start of each session (though Codex might do this automatically). -**Hooks.** The Codex hook system is less refined than of VSCode or Claude Code, but Codex also displays less drift -and is better in using Serena's tools out of the box. If you use the Codex app, you can consider setting up the -SessionStart hook (see above) to remind the agent to activate the project directory. +**Hooks.** +Codex supports lifecycle hooks; see the +[Codex hooks documentation](https://developers.openai.com/codex/hooks) for details. To enable +Serena's hooks for Codex, add this feature flag to `~/.codex/config.toml`: + +```toml +[features] +codex_hooks = true +``` + +Then create `~/.codex/hooks.json` with the following content: + +```json +{ + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "serena-hooks remind --client=codex" + } + ] + } + ], + "SessionStart": [ + { + "matcher": "startup|resume", + "hooks": [ + { + "type": "command", + "command": "serena-hooks activate --client=codex" + } + ] + } + ], + "Stop": [ + { + "hooks": [ + { + "type": "command", + "command": "serena-hooks cleanup --client=codex" + } + ] + } + ] + } +} +``` + +The hooks will: + +- **`activate`**: Prompt the agent to activate the current project and read Serena's instructions + when a Codex session starts or resumes. +- **`remind`**: Nudge the agent to use Serena's symbolic tools when it makes too many consecutive + code-search or code-file-read calls without using Serena tools in between. +- **`cleanup`**: Clean up hook session data when the session ends. + +The `PreToolUse` matcher is intentionally restricted to `Bash`. The Serena reminder hook for Codex +tracks shell-based grep and code-file reads, so running it for every tool call is unnecessary. ## Claude Desktop diff --git a/src/serena/hooks.py b/src/serena/hooks.py index 02f022e9..153f13b3 100644 --- a/src/serena/hooks.py +++ b/src/serena/hooks.py @@ -13,7 +13,6 @@ from typing import Literal, Self import click from serena.util.cli_util import AutoRegisteringGroup -from solidlsp.ls_config import Language # copied from serena_config.py, we don't want to import anything here to keep the hook commands fast serena_home_dir = os.getenv("SERENA_HOME", "").strip() or str(Path.home() / ".serena") @@ -67,15 +66,16 @@ class PreToolUseHook(Hook, ABC): permission_decision_reason: str additional_context: str = "" - def to_json_string(self) -> str: + def to_json_string(self, client: HookClient) -> str: hook_output = { "hookSpecificOutput": { "hookEventName": "PreToolUse", "permissionDecision": self.permission_decision, "permissionDecisionReason": self.permission_decision_reason, - "additionalContext": self.additional_context, } } + if client != HookClient.CODEX: + hook_output["hookSpecificOutput"]["additionalContext"] = self.additional_context return json.dumps(hook_output) def is_serena_tool(self) -> bool: @@ -245,18 +245,70 @@ class PreToolUseRemindAboutSerenaHook(PreToolUseHook): _READ_FILE_VERB_SUBSTRINGS: frozenset[str] = frozenset(("read", "view", "open", "show")) #: Shell commands that perform grep-like search; used to classify Codex - #: ``exec_command`` calls whose ``cmd`` field starts with one of these. + #: shell-command tool calls whose ``cmd`` or ``command`` field starts with one of these. _GREP_SHELL_COMMANDS: frozenset[str] = frozenset(("grep", "rg", "ag", "ack", "fgrep", "egrep")) #: Shell commands that perform file-read operations; used to classify Codex - #: ``exec_command`` calls whose ``cmd`` field starts with one of these. - _READ_SHELL_COMMANDS: frozenset[str] = frozenset(("cat", "head", "tail", "sed", "less", "more", "bat")) + #: shell-command tool calls whose ``cmd`` or ``command`` field starts with one of these. + _READ_SHELL_COMMANDS: frozenset[str] = frozenset(("cat", "head", "tail", "sed", "less", "more", "bat", "get-content", "gc")) + + #: file suffixes for source-like files where symbolic tools are usually more + #: appropriate than repeated raw reads. Lowercase and extension-only. + _CODE_FILE_EXTENSIONS: frozenset[str] = frozenset( + ( + ".al", + ".bash", + ".c", + ".clj", + ".cljs", + ".cpp", + ".cs", + ".css", + ".dart", + ".elm", + ".ex", + ".exs", + ".fs", + ".fsx", + ".go", + ".groovy", + ".h", + ".hpp", + ".hs", + ".html", + ".java", + ".jl", + ".js", + ".jsx", + ".kt", + ".kts", + ".lean", + ".lua", + ".m", + ".matlab", + ".php", + ".ps1", + ".py", + ".r", + ".rb", + ".rs", + ".scala", + ".sh", + ".sol", + ".svelte", + ".swift", + ".ts", + ".tsx", + ".vue", + ".zig", + ) + ) def __init__(self, client: HookClient): super().__init__(client) self._tool_call_counter = self.ToolUseCounter.load(self) - # extract a shell ``cmd``/``command`` field (Codex ``exec_command``-style payloads): + # extract a shell ``cmd``/``command`` field (Codex shell-command-style payloads): # split into command name (basename, lowercased) and the remaining argument string # so both bare names (``rg``) and path-prefixed invocations (``/usr/bin/grep``) are # normalised the same way. Stays ``None`` when no shell command is present. @@ -276,26 +328,10 @@ class PreToolUseRemindAboutSerenaHook(PreToolUseHook): file_path = self._tool_input.get("file_path") or self._tool_input.get("filePath") or "" self._file_path = str(file_path).strip() or None - def _call_targets_code_file(self) -> bool: - """:return: whether the call's payload references a source-code file. Heuristic that - scans every payload field that may hold a path — the shell-command argument string - (e.g. Codex ``exec_command``) and the direct ``file_path`` field (Claude Code's - ``Read``) — for a substring matching any supported language's source extensions. - """ - candidates = [s for s in (self._command_args_str, self._file_path) if s] - if not candidates: - return False - for lang in Language.iter_all(include_non_programming_languages=False): - file_matcher = lang.get_source_fn_matcher() - for candidate in candidates: - if file_matcher.string_contains_relevant_filename(candidate): - return True - return False - def is_grep_call(self) -> bool: if self._client == HookClient.CLAUDE_CODE: return self._tool_name == "grep" - if self._client == HookClient.CODEX and self._tool_name == "exec_command": + if self._client == HookClient.CODEX and self._is_shell_command_call(): return self._command_name in self._GREP_SHELL_COMMANDS # heuristic for other clients return "grep" in self._tool_name @@ -303,7 +339,7 @@ class PreToolUseRemindAboutSerenaHook(PreToolUseHook): def is_read_call(self) -> bool: if self._client == HookClient.CLAUDE_CODE: return self._tool_name == "read" - if self._client == HookClient.CODEX and self._tool_name == "exec_command": + if self._client == HookClient.CODEX and self._is_shell_command_call(): return self._command_name in self._READ_SHELL_COMMANDS # heuristic for other clients name = self._tool_name @@ -311,8 +347,48 @@ class PreToolUseRemindAboutSerenaHook(PreToolUseHook): return False return any(verb in name for verb in self._READ_FILE_VERB_SUBSTRINGS) + def _is_shell_command_call(self) -> bool: + """:return: whether the hook payload represents a shell-command tool call.""" + return self._command_name is not None + + def is_read_file_call(self) -> bool: + """:return: whether the tool call reads a file-like target.""" + return self.is_read_call() + def is_read_code_file_call(self) -> bool: - return self.is_read_call() and self._call_targets_code_file() + """:return: whether the tool call reads a source-like file target.""" + if not self.is_read_file_call(): + return False + + if self._file_path is not None: + return self._is_code_file_path(self._file_path) + + if self._client == HookClient.CODEX and self._command_args_str is not None: + return any(self._is_code_file_path(argument) for argument in self._iter_shell_path_arguments()) + + return True + + def _iter_shell_path_arguments(self) -> list[str]: + """:return: path-like shell arguments with quoting and common options removed.""" + arguments: list[str] = [] + if self._command_args_str is None: + return arguments + + for raw_argument in self._command_args_str.split(): + argument = raw_argument.strip().strip("'\"") + if not argument or argument.startswith("-"): + continue + arguments.append(argument) + + return arguments + + @classmethod + def _is_code_file_path(cls, file_path: str) -> bool: + """:return: whether ``file_path`` has a source-like suffix.""" + cleaned_path = file_path.strip().strip("'\"") + if not cleaned_path: + return False + return Path(cleaned_path).suffix.lower() in cls._CODE_FILE_EXTENSIONS def execute(self) -> None: # gate the entire hook on the rate-limit window: while we are within @@ -351,7 +427,7 @@ class PreToolUseRemindAboutSerenaHook(PreToolUseHook): # ensures we never reach this branch within the rate-limit window. self._tool_call_counter.reset() self._tool_call_counter.last_deny_timestamp = self.triggered_at_timestamp - click.echo(output_data.to_json_string()) + click.echo(output_data.to_json_string(self._client)) self._tool_call_counter.save(self) def _build_grep_deny(self) -> "PreToolUseHook.OutputData": @@ -368,10 +444,10 @@ class PreToolUseRemindAboutSerenaHook(PreToolUseHook): def _build_code_read_deny(self) -> "PreToolUseHook.OutputData": return self.OutputData( permission_decision="deny", - permission_decision_reason="Too many consecutive read calls of code files without using symbolic tools. " + permission_decision_reason="Too many consecutive read calls of files without using symbolic tools. " "You can continue using read now if needed, the counter was reset.", additional_context=( - "You were using many read calls on code files recently. Consider using Serena's symbolic " + "You were using many read calls on files recently. Consider using Serena's symbolic " "mcp tools instead for more targeted reads. You can continue using read now if needed, the counter was reset." ), ) @@ -449,7 +525,7 @@ class PreToolUseAutoApproveSerenaHook(PreToolUseHook): permission_decision="allow", permission_decision_reason=f"Auto-approved: Serena tool call while client is in {self._permission_mode} mode.", ) - click.echo(output_data.to_json_string()) + click.echo(output_data.to_json_string(self._client)) _client_option = click.option( diff --git a/test/serena/test_hooks.py b/test/serena/test_hooks.py index 25c4ece5..3bed4ec4 100644 --- a/test/serena/test_hooks.py +++ b/test/serena/test_hooks.py @@ -36,14 +36,7 @@ def _base_input( def _read_input(tool_name: str = "read", session_id: str = "test-session-123", file_path: str = "src/foo.py") -> dict: - """Build a hook payload for a read-style tool call against a code file. - - Read-style tools are gated on :meth:`PreToolUseRemindAboutSerenaHook._call_targets_code_file`, - which scans the payload for a path with a known source-code extension. Tests that exercise - the read-counter path therefore must supply such a path; we use ``file_path`` (the field - Claude Code's ``Read`` tool emits) by default, which the hook also picks up for VS Code- - shaped payloads in non-Claude-Code clients. - """ + """Build a hook payload for a read-style tool call against a file.""" return { "session_id": session_id, "tool_name": tool_name, @@ -96,22 +89,34 @@ class TestPreToolUseRemindAboutSerenaHook: hook = PreToolUseRemindAboutSerenaHook(HookClient.VSCODE) assert hook.is_grep_call() == expected, f"is_grep_tool() wrong for {name} (vscode)" + def test_grep_tool_detection_codex_shell_commands(self, tmp_path: Path): + """Codex shell-command tools are classified by the command embedded in their payload.""" + cases = [ + ("exec_command", {"cmd": "rg -n foo README.md"}, True), + ("functions.shell_command", {"command": "rg -n foo README.md"}, True), + ("functions.shell_command", {"command": "Get-Content README.md"}, False), + ] + for tool_name, tool_input, expected in cases: + with ( + patch("sys.stdin", _make_stdin(_base_input(tool_name=tool_name, tool_input=tool_input))), + patch("serena.hooks.serena_home_dir", str(tmp_path)), + ): + hook = PreToolUseRemindAboutSerenaHook(HookClient.CODEX) + assert hook.is_grep_call() == expected, f"is_grep_tool() wrong for {tool_name} / {tool_input}" + def test_read_file_tool_detection_claude_code(self, tmp_path: Path): - """Claude Code uses the exact tool name ``Read`` (lowercased to ``read``); the call - only counts as a *code-file* read when the payload points at a source file. - """ + """Claude Code uses the exact tool name ``Read`` (lowercased to ``read``).""" for name, expected in [("read", True), ("read_file", False), ("readFile", False), ("grep", False)]: with ( patch("sys.stdin", _make_stdin(_read_input(tool_name=name))), patch("serena.hooks.serena_home_dir", str(tmp_path)), ): hook = PreToolUseRemindAboutSerenaHook(HookClient.CLAUDE_CODE) - assert hook.is_read_code_file_call() == expected, f"is_read_file_tool() wrong for {name} (claude-code)" + assert hook.is_read_file_call() == expected, f"is_read_file_tool() wrong for {name} (claude-code)" def test_read_file_tool_detection_non_claude_code(self, tmp_path: Path): """Non-Claude-Code clients accept any read-style verb (``read``/``view``/``open``/``show``) - combined with ``file``; the call still only counts as a *code-file* read when the payload - points at a source file. + combined with ``file``. """ cases = [ # canonical names @@ -133,18 +138,45 @@ class TestPreToolUseRemindAboutSerenaHook: patch("serena.hooks.serena_home_dir", str(tmp_path)), ): hook = PreToolUseRemindAboutSerenaHook(HookClient.VSCODE) - assert hook.is_read_code_file_call() == expected, f"is_read_file_tool() wrong for {name} (vscode)" + assert hook.is_read_file_call() == expected, f"is_read_file_tool() wrong for {name} (vscode)" - def test_read_non_code_file_does_not_count_as_code_read(self, tmp_path: Path): - """A ``Read`` call against a non-source file (e.g. plain text) must NOT trip the - code-file read counter, even though the tool name itself is recognised as a read. - """ + def test_read_non_code_file_counts_as_file_read(self, tmp_path: Path): + """A ``Read`` call against a non-source file still counts as a file read.""" payload = _read_input(tool_name="read", file_path="notes/todo.txt") with patch("sys.stdin", _make_stdin(payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): hook = PreToolUseRemindAboutSerenaHook(HookClient.CLAUDE_CODE) assert hook.is_read_call() is True + assert hook.is_read_file_call() is True assert hook.is_read_code_file_call() is False + def test_read_code_file_detection(self, tmp_path: Path): + """Only source-like read targets count for the code-read reminder.""" + cases = [ + (HookClient.CLAUDE_CODE, _read_input(tool_name="read", file_path="README.md"), False), + (HookClient.CLAUDE_CODE, _read_input(tool_name="read", file_path="src/foo.py"), True), + (HookClient.CODEX, _base_input("functions.shell_command", tool_input={"command": "Get-Content README.md"}), False), + (HookClient.CODEX, _base_input("functions.shell_command", tool_input={"command": "Get-Content src/foo.py"}), True), + ] + for client, payload, expected in cases: + with patch("sys.stdin", _make_stdin(payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + hook = PreToolUseRemindAboutSerenaHook(client) + assert hook.is_read_code_file_call() == expected, f"is_read_code_file_call() wrong for {client} / {payload}" + + def test_read_file_tool_detection_codex_shell_commands(self, tmp_path: Path): + """Codex shell-command tools count PowerShell file reads, including non-code files.""" + cases = [ + ("exec_command", {"cmd": "cat README.md"}, True), + ("functions.shell_command", {"command": "Get-Content README.md"}, True), + ("functions.shell_command", {"command": "rg -n foo README.md"}, False), + ] + for tool_name, tool_input, expected in cases: + with ( + patch("sys.stdin", _make_stdin(_base_input(tool_name=tool_name, tool_input=tool_input))), + patch("serena.hooks.serena_home_dir", str(tmp_path)), + ): + hook = PreToolUseRemindAboutSerenaHook(HookClient.CODEX) + assert hook.is_read_file_call() == expected, f"is_read_file_tool() wrong for {tool_name} / {tool_input}" + def test_serena_tool_detection(self, tmp_path: Path): for name, expected in [("mcp_serena_find_symbol", True), ("serena_overview", True), ("grep_search", False)]: with patch("sys.stdin", _make_stdin(_base_input(tool_name=name))), patch("serena.hooks.serena_home_dir", str(tmp_path)): @@ -182,11 +214,24 @@ class TestPreToolUseRemindAboutSerenaHook: assert hook_output["permissionDecision"] == "deny" assert "grep" in hook_output["additionalContext"].lower() + def test_deny_output_after_threshold_greps_codex_shell_command(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]): + """After reaching the grep threshold, Codex ``functions.shell_command`` emits a deny.""" + payload = _base_input("functions.shell_command", tool_input={"command": "rg -n foo README.md"}) + for _ in range(ToolUseCounter._GREP_USES_THRESHOLD): + with patch("sys.stdin", _make_stdin(payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PreToolUseRemindAboutSerenaHook(HookClient.CODEX).execute() + + output = capsys.readouterr().out.strip() + result = json.loads(output) + hook_output = result["hookSpecificOutput"] + assert hook_output["permissionDecision"] == "deny" + assert "additionalContext" not in hook_output + assert "grep" in hook_output["permissionDecisionReason"].lower() + def test_deny_output_after_threshold_reads(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]): """After reaching the read file threshold, the hook should output a deny. - The read counter only ticks up when the payload actually targets a source-code file, - so ``_read_input`` is used to supply a plausible ``.py`` ``file_path``. + ``_read_input`` supplies the direct ``file_path`` field emitted by file-read tools. """ for _ in range(ToolUseCounter._READ_FILE_USES_THRESHOLD): with patch("sys.stdin", _make_stdin(_read_input("read"))), patch("serena.hooks.serena_home_dir", str(tmp_path)): @@ -197,7 +242,29 @@ class TestPreToolUseRemindAboutSerenaHook: hook_output = result["hookSpecificOutput"] assert hook_output["permissionDecision"] == "deny" assert "read" in hook_output["additionalContext"].lower() - assert "code file" in hook_output["additionalContext"].lower() + assert "files" in hook_output["additionalContext"].lower() + + def test_no_deny_after_threshold_get_content_markdown_codex(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]): + """PowerShell ``Get-Content`` calls on markdown files do not count for the code-read reminder.""" + payload = _base_input("functions.shell_command", tool_input={"command": "Get-Content README.md"}) + for _ in range(ToolUseCounter._READ_FILE_USES_THRESHOLD): + with patch("sys.stdin", _make_stdin(payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PreToolUseRemindAboutSerenaHook(HookClient.CODEX).execute() + + assert capsys.readouterr().out == "" + + def test_deny_output_after_threshold_get_content_code_file_codex(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]): + """PowerShell ``Get-Content`` calls on source files count for the code-read reminder.""" + payload = _base_input("functions.shell_command", tool_input={"command": "Get-Content src/foo.py"}) + for _ in range(ToolUseCounter._READ_FILE_USES_THRESHOLD): + with patch("sys.stdin", _make_stdin(payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PreToolUseRemindAboutSerenaHook(HookClient.CODEX).execute() + + result = json.loads(capsys.readouterr().out.strip()) + hook_output = result["hookSpecificOutput"] + assert hook_output["permissionDecision"] == "deny" + assert "additionalContext" not in hook_output + assert "read" in hook_output["permissionDecisionReason"].lower() def test_serena_tool_resets_counters(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]): """Using a Serena tool should reset counters, so the threshold is not reached.""" @@ -359,10 +426,8 @@ class TestToolUseCounter: assert counter.n_recent_read_file_uses == 1 - def test_update_does_not_count_read_of_non_code_file(self): - """A read call whose payload points at a non-source file (e.g. ``.txt``) must - leave the read-file counter untouched — the per-tool counter is for *code* reads. - """ + def test_update_ignores_read_of_non_code_file(self): + """A read call whose payload points at a non-source file does not increment the code-read counter.""" counter = ToolUseCounter() now = datetime.now() @@ -465,9 +530,8 @@ class TestToolUseCounter: ``grep_search`` / ``read_file`` which are only recognized under the non-Claude-Code branch (Claude Code uses exact names ``grep`` / ``read``). - :param file_path: optional payload path; when set to a source-extension path (e.g. - ``src/foo.py``), the stub will be classified as a *code-file* read by - :meth:`PreToolUseRemindAboutSerenaHook._call_targets_code_file`. + :param file_path: optional payload path; the hook classifies read-style tools as + file reads regardless of the path extension. :param command_args_str: optional shell-argument tail; equivalent to ``file_path`` but populates the ``cmd``-derived field instead of ``file_path``. """