From 47c0a4bb6657b42201c713f0df48c9c8e23a135e Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Fri, 14 Aug 2026 01:07:49 +0000 Subject: [PATCH] fix(hooks): reset reminder counters after a successful Codex Serena call Codex's documented hook wiring only routes `serena-hooks remind` through PreToolUse on the `Bash` matcher, so `ToolUseCounter.update()`'s reset-on-Serena-tool-use branch is never reached there: it only fires when `remind` itself is invoked for a `mcp__serena__*` tool name, which the Bash-only matcher excludes. Reminder counters therefore never clear after a successful Serena call, and an unrelated grep/read burst afterwards can trip the deny threshold on state that should have been reset. Add a `serena-hooks reset` command, wired to PostToolUse on Serena's own tools, that resets the counters after a successful call. Gated on the call having succeeded (`tool_response` carrying no `isError: true`, the MCP `tools/call` result shape) so a failed Serena call does not mask a real grep/read streak, per the issue's own acceptance criteria. The symbolic-tool classification is shared with the existing PreToolUse hook via a small extracted helper so both agree on what counts as a Serena tool. Fixes #1852 --- CHANGELOG.md | 6 ++ docs/02-usage/030_clients.md | 20 +++- src/serena/hooks.py | 95 +++++++++++++---- test/serena/test_hooks.py | 195 +++++++++++++++++++++++++++++++++++ 4 files changed, 296 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83ee56b5..940eedad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,12 @@ Status of the `main` branch. Changes prior to the next official version change w the IDE's own config-directory lock; JetBrains IDE launches are now serialized per launch command and Serena waits for the plugin server to become reachable before proceeding (#1864) +* Hooks: + - Fix: Codex's documented hook wiring only routes `remind` through `PreToolUse` on `Bash`, so its + reset-on-Serena-tool-use branch was unreachable there and reminder counters never cleared after a + successful Serena call. Add a `serena-hooks reset` command and a `PostToolUse` example matched to + Serena's own tools to close the gap (#1852) + * Language Servers: - Fix: Exceptions raised during `LanguageServerManager.start` did not stop the language server subprocess if it was already started (#1949) diff --git a/docs/02-usage/030_clients.md b/docs/02-usage/030_clients.md index c6a51846..da204dba 100644 --- a/docs/02-usage/030_clients.md +++ b/docs/02-usage/030_clients.md @@ -344,6 +344,17 @@ Then create `~/.codex/hooks.json` with the following content: ] } ], + "PostToolUse": [ + { + "matcher": "^mcp__serena__.*$", + "hooks": [ + { + "type": "command", + "command": "serena-hooks reset --client=codex" + } + ] + } + ], "SessionStart": [ { "matcher": "startup|resume", @@ -381,10 +392,15 @@ The hooks will: 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. +- **`reset`**: Clear the reminder counters after a successful Serena symbolic tool call, so using + Serena's tools starts a fresh count instead of leaving the prior grep/read streak in place. - **`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. +The `PreToolUse` matcher is intentionally restricted to `Bash`: the reminder hook tracks shell-based +grep and code-file reads, so running it for every tool call is unnecessary. That matcher never sees +`mcp__serena__*` tool names, though, so it cannot also perform the counter reset on Serena tool use +the way it does for clients whose `PreToolUse` hook observes every tool call. The separate `reset` +hook above, matched to `PostToolUse` on Serena's own tools, covers that case for Codex instead. ## Grok diff --git a/src/serena/hooks.py b/src/serena/hooks.py index cd1fb84f..e2c037c1 100644 --- a/src/serena/hooks.py +++ b/src/serena/hooks.py @@ -48,23 +48,31 @@ class Hook(ABC): pass -class PreToolUseHook(Hook, ABC): - _NON_SYMBOLIC_SERENA_TOOL_NAME_SUBSTRINGS = frozenset( - ( - "pattern", - "read", - "diagnostics", - "memory", - "onboarding", - "config", - "list_file", - "find_file", - "shell", - "dashboard", - "restart_language_server", - ) +#: substrings that mark a "serena"-containing tool name as one of Serena's own non-symbolic +#: utilities (read/config/dashboard/shell) rather than a code-navigation tool; shared across +#: PreToolUse and PostToolUse hooks so both classify a call the same way. +_NON_SYMBOLIC_SERENA_TOOL_NAME_SUBSTRINGS = frozenset( + ( + "pattern", + "read", + "diagnostics", + "memory", + "onboarding", + "config", + "list_file", + "find_file", + "shell", + "dashboard", + "restart_language_server", ) +) + +def _is_serena_symbolic_tool_name(tool_name: str) -> bool: + return "serena" in tool_name and not any(substring in tool_name for substring in _NON_SYMBOLIC_SERENA_TOOL_NAME_SUBSTRINGS) + + +class PreToolUseHook(Hook, ABC): def __init__(self, client: HookClient): super().__init__(client) _tool_name = self._input_data.get("tool_name") or self._input_data.get("toolName", "") or "" @@ -107,9 +115,7 @@ class PreToolUseHook(Hook, ABC): return json.dumps(hook_output) def is_serena_symbolic_tool(self) -> bool: - return "serena" in self._tool_name and not any( - substring in self._tool_name for substring in self._NON_SYMBOLIC_SERENA_TOOL_NAME_SUBSTRINGS - ) + return _is_serena_symbolic_tool_name(self._tool_name) class PreToolUseRemindAboutSymbolicToolsHook(PreToolUseHook): @@ -521,6 +527,48 @@ class PreToolUseRemindAboutSymbolicToolsHook(PreToolUseHook): ) +class PostToolUseResetSymbolicToolCounterHook(Hook): + """Post-tool-use hook that resets :class:`PreToolUseRemindAboutSymbolicToolsHook`'s + persisted counters after a successful Serena symbolic tool call. + + ``PreToolUseRemindAboutSymbolicToolsHook`` already resets on a Serena tool call, but + only when it is itself invoked for that call, which requires the client's PreToolUse + matcher to observe ``mcp__serena__*`` tool names. Codex's documented wiring (see + docs/02-usage/030_clients.md) attaches ``remind`` to the ``Bash`` matcher only, so it + is never invoked for Serena's own tools there and the reset branch is unreachable. + This hook closes that gap from the other side of the call: wired to PostToolUse with a + matcher on Serena's tools, it fires once the call has completed. + + Gated on the call having succeeded (``tool_response`` carrying no ``isError: true``, + the MCP ``tools/call`` result shape) so a failed Serena call does not mask a real + grep/read-drift streak the agent is still in. + """ + + def __init__(self, client: HookClient): + super().__init__(client) + raw_tool_name = self._input_data.get("tool_name") or self._input_data.get("toolName", "") or "" + tool_name = str(raw_tool_name).lower().strip() + if not tool_name: + raise ValueError("Tool name is required in the hook input data") + self._tool_name = tool_name + raw_tool_response = self._input_data.get("tool_response") or self._input_data.get("toolResponse") + self._tool_response: dict | None = raw_tool_response if isinstance(raw_tool_response, dict) else None + + def _call_succeeded(self) -> bool: + # no structured response to check: be conservative and treat it as not confirmed + # successful, rather than resetting on data we can't actually read + if self._tool_response is None: + return False + return self._tool_response.get("isError") is not True + + def execute(self) -> None: + if not _is_serena_symbolic_tool_name(self._tool_name) or not self._call_succeeded(): + return + counter = PreToolUseRemindAboutSymbolicToolsHook.ToolUseCounter.load(self) + counter.reset() + counter.save(self) + + class SessionStartActivateProjectHook(Hook): def execute(self) -> None: message = ( @@ -630,5 +678,16 @@ class HookCommands(AutoRegisteringGroup): def auto_approve(client: str) -> None: PreToolUseAutoApproveSerenaHook(HookClient(client)).execute() + @staticmethod + @click.command( + "reset", + help="Set this as hook at PostToolUse, matched to Serena's own tools, to reset the grep/read-drift " + "counters after a successful Serena tool call. For clients whose PreToolUse wiring does not observe " + "Serena tool calls (e.g. Codex, matched to Bash only); complements `remind`'s own reset branch.", + ) + @_client_option + def reset(client: str) -> None: + PostToolUseResetSymbolicToolCounterHook(HookClient(client)).execute() + hook_commands = HookCommands() diff --git a/test/serena/test_hooks.py b/test/serena/test_hooks.py index f0e015b6..2e4f1aa8 100644 --- a/test/serena/test_hooks.py +++ b/test/serena/test_hooks.py @@ -10,6 +10,7 @@ from click.testing import CliRunner from serena.hooks import ( HookClient, + PostToolUseResetSymbolicToolCounterHook, PreToolUseAutoApproveSerenaHook, PreToolUseHook, PreToolUseRemindAboutSymbolicToolsHook, @@ -36,6 +37,21 @@ def _base_input( } +def _post_tool_use_input( + tool_name: str, + session_id: str = "test-session-123", + tool_response: dict | None = None, +) -> dict: + """Build a Codex-shaped PostToolUse payload, with ``tool_response`` carrying the MCP + ``tools/call`` result shape (``isError`` per the MCP spec). + """ + return { + "session_id": session_id, + "tool_name": tool_name, + "tool_response": tool_response if tool_response is not None else {"content": [], "isError": False}, + } + + def _grok_input( tool_name: str, tool_input: dict | None = None, @@ -932,6 +948,150 @@ class TestPreToolUseAutoApproveSerenaHook: assert capsys.readouterr().out == "" +class TestPostToolUseResetSymbolicToolCounterHook: + """Tests for the PostToolUse hook that resets the reminder counters after a + successful Serena symbolic tool call, for clients (Codex) whose PreToolUse wiring + never observes Serena's own tools. + """ + + def test_missing_tool_name_raises(self, tmp_path: Path): + stdin_data = {"session_id": "s1", "tool_response": {"isError": False}} + with patch("sys.stdin", _make_stdin(stdin_data)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + with pytest.raises(ValueError, match="Tool name is required"): + PostToolUseResetSymbolicToolCounterHook(HookClient.CODEX) + + def test_missing_session_id_raises(self, tmp_path: Path): + stdin_data = {"tool_name": "mcp__serena__find_symbol", "tool_response": {"isError": False}} + with patch("sys.stdin", _make_stdin(stdin_data)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + with pytest.raises(ValueError, match="Session ID is required"): + PostToolUseResetSymbolicToolCounterHook(HookClient.CODEX) + + def _persisted_counter(self, tmp_path: Path, session_id: str) -> ToolUseCounter: + path = tmp_path / "hook_data" / session_id / "tool_use_counter.pkl" + with open(path, "rb") as f: + return pickle.load(f) + + def _seed_counter(self, tmp_path: Path, session_id: str, counter: ToolUseCounter) -> None: + path = tmp_path / "hook_data" / session_id / "tool_use_counter.pkl" + path.parent.mkdir(parents=True, exist_ok=True) + with open(path, "wb") as f: + pickle.dump(counter, f) + + def test_resets_persisted_counter_on_successful_serena_call(self, tmp_path: Path): + session_id = "reset-success" + seeded = ToolUseCounter(n_recent_grep_uses=2, n_recent_read_file_uses=1, n_recent_non_symbolic_uses=3) + self._seed_counter(tmp_path, session_id, seeded) + + payload = _post_tool_use_input("mcp__serena__find_symbol", session_id=session_id, tool_response={"isError": False}) + with patch("sys.stdin", _make_stdin(payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PostToolUseResetSymbolicToolCounterHook(HookClient.CODEX).execute() + + result = self._persisted_counter(tmp_path, session_id) + assert result.n_recent_grep_uses == 0 + assert result.n_recent_read_file_uses == 0 + assert result.n_recent_non_symbolic_uses == 0 + + def test_does_not_reset_on_failed_serena_call(self, tmp_path: Path): + """A Serena call that itself errored must not mask a real grep/read streak.""" + session_id = "reset-failure" + seeded = ToolUseCounter(n_recent_grep_uses=2) + self._seed_counter(tmp_path, session_id, seeded) + + payload = _post_tool_use_input("mcp__serena__find_symbol", session_id=session_id, tool_response={"isError": True}) + with patch("sys.stdin", _make_stdin(payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PostToolUseResetSymbolicToolCounterHook(HookClient.CODEX).execute() + + assert self._persisted_counter(tmp_path, session_id).n_recent_grep_uses == 2 + + def test_does_not_reset_without_a_tool_response(self, tmp_path: Path): + """No structured response to confirm success against: stay conservative, do not reset.""" + session_id = "reset-no-response" + seeded = ToolUseCounter(n_recent_grep_uses=2) + self._seed_counter(tmp_path, session_id, seeded) + + payload = {"session_id": session_id, "tool_name": "mcp__serena__find_symbol"} + with patch("sys.stdin", _make_stdin(payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PostToolUseResetSymbolicToolCounterHook(HookClient.CODEX).execute() + + assert self._persisted_counter(tmp_path, session_id).n_recent_grep_uses == 2 + + def test_does_not_reset_for_non_serena_tool(self, tmp_path: Path): + session_id = "reset-non-serena" + seeded = ToolUseCounter(n_recent_grep_uses=2) + self._seed_counter(tmp_path, session_id, seeded) + + payload = _post_tool_use_input("exec_command", session_id=session_id, tool_response={"isError": False}) + with patch("sys.stdin", _make_stdin(payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PostToolUseResetSymbolicToolCounterHook(HookClient.CODEX).execute() + + assert self._persisted_counter(tmp_path, session_id).n_recent_grep_uses == 2 + + def test_does_not_reset_for_non_symbolic_serena_tool(self, tmp_path: Path): + """``read_file``-like Serena tools are excluded, same as the PreToolUse classification.""" + session_id = "reset-non-symbolic" + seeded = ToolUseCounter(n_recent_grep_uses=2) + self._seed_counter(tmp_path, session_id, seeded) + + payload = _post_tool_use_input("mcp__serena__read_file", session_id=session_id, tool_response={"isError": False}) + with patch("sys.stdin", _make_stdin(payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PostToolUseResetSymbolicToolCounterHook(HookClient.CODEX).execute() + + assert self._persisted_counter(tmp_path, session_id).n_recent_grep_uses == 2 + + def test_creates_fresh_counter_when_none_persisted_yet(self, tmp_path: Path): + """A successful Serena call as the very first hook invocation of a session must not raise.""" + session_id = "reset-fresh" + payload = _post_tool_use_input("mcp__serena__find_symbol", session_id=session_id, tool_response={"isError": False}) + with patch("sys.stdin", _make_stdin(payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PostToolUseResetSymbolicToolCounterHook(HookClient.CODEX).execute() + + assert self._persisted_counter(tmp_path, session_id).n_recent_grep_uses == 0 + + def test_codex_acceptance_scenario_successful_serena_call_prevents_deny(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]): + """The issue's own acceptance test: Bash(rg) -> Bash(rg) -> successful Serena call -> Bash(rg) + must not deny the final call, using exactly the documented Codex hook wiring + (``remind`` on PreToolUse/Bash, ``reset`` on PostToolUse/``mcp__serena__.*``). + """ + session_id = "codex-acceptance-success" + grep_shell_payload = _base_input(tool_name="exec_command", session_id=session_id, tool_input={"cmd": "rg -n foo README.md"}) + + for _ in range(ToolUseCounter._GREP_USES_THRESHOLD - 1): + with patch("sys.stdin", _make_stdin(grep_shell_payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PreToolUseRemindAboutSymbolicToolsHook(HookClient.CODEX).execute() + assert capsys.readouterr().out == "" + + serena_call = _post_tool_use_input("mcp__serena__find_symbol", session_id=session_id, tool_response={"isError": False}) + with patch("sys.stdin", _make_stdin(serena_call)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PostToolUseResetSymbolicToolCounterHook(HookClient.CODEX).execute() + + with patch("sys.stdin", _make_stdin(grep_shell_payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PreToolUseRemindAboutSymbolicToolsHook(HookClient.CODEX).execute() + + assert capsys.readouterr().out == "" + + def test_codex_acceptance_scenario_failed_serena_call_still_denies(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]): + """The issue's second acceptance test: a failed Serena call must not reset the streak, + so the threshold is still reached. + """ + session_id = "codex-acceptance-failure" + grep_shell_payload = _base_input(tool_name="exec_command", session_id=session_id, tool_input={"cmd": "rg -n foo README.md"}) + + for _ in range(ToolUseCounter._GREP_USES_THRESHOLD - 1): + with patch("sys.stdin", _make_stdin(grep_shell_payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PreToolUseRemindAboutSymbolicToolsHook(HookClient.CODEX).execute() + assert capsys.readouterr().out == "" + + failed_serena_call = _post_tool_use_input("mcp__serena__find_symbol", session_id=session_id, tool_response={"isError": True}) + with patch("sys.stdin", _make_stdin(failed_serena_call)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PostToolUseResetSymbolicToolCounterHook(HookClient.CODEX).execute() + + with patch("sys.stdin", _make_stdin(grep_shell_payload)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + PreToolUseRemindAboutSymbolicToolsHook(HookClient.CODEX).execute() + + output = json.loads(capsys.readouterr().out.strip()) + assert output["hookSpecificOutput"]["permissionDecision"] == "deny" + + class TestSessionEndCleanupHook: def test_removes_session_dir(self, tmp_path: Path): session_dir = tmp_path / "hook_data" / "cleanup-session" @@ -1020,6 +1180,41 @@ class TestHookCli: assert "grep" in output["reason"].lower() assert "hookSpecificOutput" not in output + def test_reset_command(self, tmp_path: Path): + """The ``reset`` CLI command clears a persisted counter after a successful Serena call.""" + session_id = "cli-reset" + session_dir = tmp_path / "hook_data" / session_id + session_dir.mkdir(parents=True) + with open(session_dir / "tool_use_counter.pkl", "wb") as f: + pickle.dump(ToolUseCounter(n_recent_grep_uses=2), f) + + stdin_json = json.dumps({"session_id": session_id, "tool_name": "mcp__serena__find_symbol", "tool_response": {"isError": False}}) + runner = CliRunner() + with patch("serena.hooks.serena_home_dir", str(tmp_path)): + result = runner.invoke(hook_commands, ["reset", "--client", "codex"], input=stdin_json) + assert result.exit_code == 0 + assert result.output == "" + + with open(session_dir / "tool_use_counter.pkl", "rb") as f: + assert pickle.load(f).n_recent_grep_uses == 0 + + def test_reset_command_stays_silent_on_failed_call(self, tmp_path: Path): + """The ``reset`` CLI command must not clear the counter for a failed Serena call.""" + session_id = "cli-reset-failed" + session_dir = tmp_path / "hook_data" / session_id + session_dir.mkdir(parents=True) + with open(session_dir / "tool_use_counter.pkl", "wb") as f: + pickle.dump(ToolUseCounter(n_recent_grep_uses=2), f) + + stdin_json = json.dumps({"session_id": session_id, "tool_name": "mcp__serena__find_symbol", "tool_response": {"isError": True}}) + runner = CliRunner() + with patch("serena.hooks.serena_home_dir", str(tmp_path)): + result = runner.invoke(hook_commands, ["reset", "--client", "codex"], input=stdin_json) + assert result.exit_code == 0 + + with open(session_dir / "tool_use_counter.pkl", "rb") as f: + assert pickle.load(f).n_recent_grep_uses == 2 + def test_auto_approve_command(self, tmp_path: Path): """The ``auto-approve`` CLI command emits an allow for a Serena tool in acceptEdits mode.""" stdin_json = json.dumps(