fix(hooks): emit valid Codex Stop-hook JSON from cleanup (#1533)

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)

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):

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"})