Skip to content

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

Open
arimu1 wants to merge 4 commits into
oraios:mainfrom
arimu1:fix/1533-codex-cleanup-stop-json
Open

fix(hooks): emit valid Codex Stop-hook JSON from cleanup (#1533)#1738
arimu1 wants to merge 4 commits into
oraios:mainfrom
arimu1:fix/1533-codex-cleanup-stop-json

Conversation

@arimu1

@arimu1 arimu1 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Closes #1533.

Problem

When serena-hooks cleanup --client=codex is configured as a Codex Stop
hook, Codex reports:

Stop hook (failed)
error: hook returned invalid stop hook JSON output

Two causes:

  • SessionEndCleanupHook.execute() only ran shutil.rmtree and printed
    nothing, so stdout was empty — but Codex requires a Stop hook to print
    valid Stop-hook JSON.
  • Hook.__init__ raised ValueError when the payload had no session_id,
    so a Stop payload without one produced a Python traceback.

Fix

  • Hook.__init__ gains a require_session_id flag (default True, so every
    existing hook is unchanged) and tolerates empty stdin.
  • SessionEndCleanupHook opts out of the requirement — cleanup is a
    no-op-safe operation, so a missing session id simply means there is
    nothing to remove — and always emits {"continue": true} for the Codex
    client. Other clients stay silent, preserving current behavior.

Tests

Added unit tests (missing session id / empty stdin no longer raise) and CLI
tests asserting cleanup --client=codex prints {"continue": true} both
with and without a session id. test_hooks.py green (90/90); ruff and
ty type-check clean.

@AmirF194 AmirF194 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The require_session_id opt-in/opt-out split is clean and the new tests cover both the missing-session-id and empty-stdin cases.

One thing to check: SessionStartActivateProjectHook (src/serena/hooks.py:531) doesn't override init, so it still inherits require_session_id=True from the base Hook. Docker-verified on this PR's head (dd9eb68):

printf '%s' '{"hookEventName":"SessionStart"}' | serena-hooks activate --client=codex

raises the same uncaught ValueError: Session ID is required in the hook input data that #1533 reports for cleanup. I don't have evidence Codex ever omits session_id on its SessionStart event, only that #1533's own repro shows it can omit one on Stop, so this might not happen in practice. But if it ever does, activate would crash the same way cleanup used to before this fix. Might be worth deciding whether activate should opt out the same way, or whether SessionStart is guaranteed to always carry a session id.

@arimu1
arimu1 force-pushed the fix/1533-codex-cleanup-stop-json branch from dd9eb68 to 0e99303 Compare July 30, 2026 05:35
arimu1 added 2 commits August 1, 2026 08:35
`serena-hooks cleanup --client=codex`, used as a Codex Stop hook, printed
nothing on success and raised a traceback when the payload carried no
session id, so Codex reported "hook returned invalid stop hook JSON
output". Cleanup now tolerates a missing session id (and empty stdin) as a
no-op and always emits {"continue": true} for the Codex client, while
other clients keep their silent behavior.
ty rejects Path(str | None) after session_persistence_dir became optional
for SessionEndCleanupHook. PreToolUse state persistence still requires a dir.
@arimu1
arimu1 force-pushed the fix/1533-codex-cleanup-stop-json branch from e02a4c1 to 2bf3b85 Compare August 1, 2026 01:35
…n_id

SessionStartActivateProjectHook did not override __init__, so it inherited
require_session_id=True from the base Hook class even though it never reads
session_persistence_dir (it only emits a static reminder message). If a
client ever omits session_id on SessionStart, activate would crash the same
way cleanup did before oraios#1533.

Opt it out of the session-id requirement the same way SessionEndCleanupHook
does, and add tests for missing session_id, empty stdin, and the CLI path.

Addresses review feedback from AmirF194 on oraios#1738.
@arimu1

arimu1 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Good catch — confirmed. SessionStartActivateProjectHook didn't override __init__, so it inherited require_session_id=True from the base Hook, even though execute() never touches session_persistence_dir (it only emits a static reminder message). Pushed a fix (904b071) that opts it out of the session-id requirement the same way SessionEndCleanupHook does, plus tests for missing session_id, empty stdin, and the CLI activate path.

On the failing jvm (windows-latest) job: the failure (test_safe_delete_symbol_succeeds_when_no_references[java_model_user]) is a Java LSP symbol-deletion assertion, unrelated to the hooks.py changes here. I checked recent main CI history and jvm (windows-latest) has failed there before too (for a different reason, a Clojure artifact-resolution error), so this looks like a pre-existing flake rather than something introduced by this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Codex Stop hook: serena-hooks cleanup --client=codex emits invalid Stop-hook stdout

2 participants