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
This commit is contained in:
Amir Fathi authored and Dr. Dominik Jain committed 2026-09-02 13:21:42 +02:00
1 parent 9f4a2b4830
commit 47c0a4bb66
4 files changed
+296 -20

No files matched your search

+6
View File
@@ -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)
+18 -2
View File
@@ -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
+77 -18
View File
@@ -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()
+195
View File
@@ -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(