diff --git a/CHANGELOG.md b/CHANGELOG.md index e5a26bc99..c5767086e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,12 @@ Status of the `main` branch. Changes prior to the next official version change w - PreToolUse remind hook: coerce non-string shell command values instead of failing, and recognize `target_file`/`targetFile` file-path keys (shared payload parsing, applies to all hook clients). - Fix hook input parsing for clients that emit raw control characters in JSON string values #1743. + - Fix: `serena-hooks cleanup --client=codex` used as a Codex `Stop` hook now always prints valid + Stop-hook JSON (`{"continue": true}`) and no longer crashes when the payload has no session id or + stdin is empty, so Codex stops reporting `hook returned invalid stop hook JSON output` (#1533). + - Fix: `serena-hooks activate` (`SessionStartActivateProjectHook`) no longer requires a session id; + it only emits a static reminder message and never touches session state, so a client omitting + `session_id` on `SessionStart` no longer crashes it the same way cleanup used to (#1533 review). # v1.6.1 (2026-07-21) diff --git a/src/serena/hooks.py b/src/serena/hooks.py index 6c2aafa2d..1cfd19921 100644 --- a/src/serena/hooks.py +++ b/src/serena/hooks.py @@ -29,17 +29,24 @@ class HookClient(Enum): class Hook(ABC): - def __init__(self, client: HookClient): + def __init__(self, client: HookClient, require_session_id: bool = True): raw = sys.stdin.read() - input_data = json.loads(raw, strict=False) + # a hook may be invoked with no stdin (e.g. a session-end cleanup that + # is a no-op); treat that as an empty payload rather than crashing + # strict=False: some clients embed raw control characters in JSON strings (#1743) + input_data = json.loads(raw, strict=False) if raw.strip() else {} self._input_data = input_data self._client = client session_id = input_data.get("session_id") or input_data.get("sessionId") if not session_id: - raise ValueError("Session ID is required in the hook input data") - self._session_id = str(session_id) - self.session_persistence_dir = os.path.join(serena_home_dir, "hook_data", self._session_id) + if require_session_id: + raise ValueError("Session ID is required in the hook input data") + self._session_id: str | None = None + self.session_persistence_dir: str | None = None + else: + self._session_id = str(session_id) + self.session_persistence_dir = os.path.join(serena_home_dir, "hook_data", self._session_id) # tool input has a timestamp but using now is enough self.triggered_at_timestamp = datetime.now() @@ -195,7 +202,13 @@ def is_hook_active(self, now: datetime) -> bool: @classmethod def _get_persistence_path(cls, hook: Hook) -> Path: - return Path(hook.session_persistence_dir) / cls._FILE_NAME + # session_persistence_dir is None only when require_session_id=False + # (SessionEndCleanupHook); this State is used by PreToolUse hooks that + # always have a session id. + persistence_dir = hook.session_persistence_dir + if persistence_dir is None: + raise ValueError("session_persistence_dir is required for hook state persistence") + return Path(persistence_dir) / cls._FILE_NAME @classmethod def load(cls, hook: Hook) -> Self: @@ -521,6 +534,12 @@ def _build_non_symbolic_deny(self) -> "PreToolUseHook.OutputData": class SessionStartActivateProjectHook(Hook): + def __init__(self, client: HookClient): + # this hook only emits a static reminder message and never touches + # session_persistence_dir, so a missing session id must not abort it + # (see PR #1738 review): some clients may omit session_id on SessionStart + super().__init__(client, require_session_id=False) + def execute(self) -> None: message = ( "**IMPORTANT**: If the current directory is a coding project you are working on:" @@ -538,8 +557,19 @@ def execute(self) -> None: class SessionEndCleanupHook(Hook): + def __init__(self, client: HookClient): + # cleanup is safe to run as a no-op, so a missing session id must not + # abort it (see #1533): without one there is simply nothing to remove + super().__init__(client, require_session_id=False) + def execute(self) -> None: - shutil.rmtree(self.session_persistence_dir, ignore_errors=True) + if self.session_persistence_dir is not None: + shutil.rmtree(self.session_persistence_dir, ignore_errors=True) + # Codex requires a Stop hook's stdout to be valid Stop-hook JSON, so + # always emit a "continue" response there; other clients treat empty + # stdout as "no action" and must stay silent to preserve behavior. + if self._client == HookClient.CODEX: + click.echo(json.dumps({"continue": True})) class PreToolUseAutoApproveSerenaHook(PreToolUseHook): diff --git a/test/serena/test_hooks.py b/test/serena/test_hooks.py index f0e015b61..082da2c39 100644 --- a/test/serena/test_hooks.py +++ b/test/serena/test_hooks.py @@ -14,6 +14,7 @@ PreToolUseHook, PreToolUseRemindAboutSymbolicToolsHook, SessionEndCleanupHook, + SessionStartActivateProjectHook, hook_commands, ) @@ -951,6 +952,40 @@ def test_cleanup_is_idempotent(self, tmp_path: Path): with patch("sys.stdin", _make_stdin(stdin_data)), patch("serena.hooks.serena_home_dir", str(tmp_path)): SessionEndCleanupHook(HookClient.CLAUDE_CODE).execute() + def test_missing_session_id_does_not_raise(self, tmp_path: Path): + """Unlike other hooks, cleanup tolerates a missing session id (#1533).""" + stdin_data = {"hookEventName": "stop"} + with patch("sys.stdin", _make_stdin(stdin_data)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + SessionEndCleanupHook(HookClient.CODEX).execute() + + def test_empty_stdin_does_not_raise(self, tmp_path: Path): + """A cleanup hook invoked with no stdin is a no-op, not a crash (#1533).""" + with patch("sys.stdin", StringIO("")), patch("serena.hooks.serena_home_dir", str(tmp_path)): + SessionEndCleanupHook(HookClient.CODEX).execute() + + +class TestSessionStartActivateProjectHook: + def test_missing_session_id_does_not_raise(self, tmp_path: Path): + """Activate only emits a static reminder and never touches session_persistence_dir, + so a missing session id must not abort it, mirroring cleanup's tolerance (PR #1738 review). + """ + stdin_data = {"hookEventName": "SessionStart"} + with patch("sys.stdin", _make_stdin(stdin_data)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + SessionStartActivateProjectHook(HookClient.CODEX).execute() + + def test_empty_stdin_does_not_raise(self, tmp_path: Path): + """Activate invoked with no stdin must not crash.""" + with patch("sys.stdin", StringIO("")), patch("serena.hooks.serena_home_dir", str(tmp_path)): + SessionStartActivateProjectHook(HookClient.CODEX).execute() + + def test_with_session_id_still_works(self, tmp_path: Path): + """A present session id is still accepted and stored as before.""" + stdin_data = {"session_id": "activate-session"} + with patch("sys.stdin", _make_stdin(stdin_data)), patch("serena.hooks.serena_home_dir", str(tmp_path)): + hook = SessionStartActivateProjectHook(HookClient.CLAUDE_CODE) + assert hook.session_persistence_dir is not None + hook.execute() + class TestHookCli: """Tests for the Click CLI entry point (serena-hooks).""" @@ -980,6 +1015,29 @@ def test_cleanup_command_grok_camelcase_session(self, tmp_path: Path): assert result.exit_code == 0 assert not session_dir.exists() + def test_cleanup_command_codex_emits_continue_json(self, tmp_path: Path): + """Codex requires the Stop hook's stdout to be valid JSON (#1533).""" + session_id = "cli-codex-cleanup" + session_dir = tmp_path / "hook_data" / session_id + session_dir.mkdir(parents=True) + (session_dir / "somefile").write_text("data") + + stdin_json = json.dumps({"session_id": session_id}) + runner = CliRunner() + with patch("serena.hooks.serena_home_dir", str(tmp_path)): + result = runner.invoke(hook_commands, ["cleanup", "--client", "codex"], input=stdin_json) + assert result.exit_code == 0 + assert not session_dir.exists() + assert json.loads(result.output) == {"continue": True} + + def test_cleanup_command_codex_without_session_id(self, tmp_path: Path): + """Codex cleanup still emits valid JSON when no session id is present (#1533).""" + runner = CliRunner() + with patch("serena.hooks.serena_home_dir", str(tmp_path)): + result = runner.invoke(hook_commands, ["cleanup", "--client", "codex"], input=json.dumps({"hookEventName": "stop"})) + assert result.exit_code == 0 + assert json.loads(result.output) == {"continue": True} + def test_remind_command(self, tmp_path: Path): """Invoke the remind command enough times to trigger a deny.""" runner = CliRunner() @@ -1124,6 +1182,14 @@ def test_auto_approve_command_grok_permission_modes( else: assert json.loads(result.output) == expected_output + def test_activate_command_without_session_id_does_not_raise(self, tmp_path: Path): + """Activate must not crash if a client omits session_id on SessionStart (PR #1738 review).""" + stdin_json = json.dumps({"hookEventName": "SessionStart"}) + runner = CliRunner() + with patch("serena.hooks.serena_home_dir", str(tmp_path)): + result = runner.invoke(hook_commands, ["activate", "--client", "codex"], input=stdin_json) + assert result.exit_code == 0 + def test_client_default_is_claude_code(self, tmp_path: Path): """When --client is omitted, it defaults to claude-code.""" stdin_json = json.dumps({"session_id": "cli-default"})