Skip to content

fix(hooks): emit valid Codex cleanup output - #1687

Open
Bortlesboat wants to merge 1 commit into
oraios:mainfrom
Bortlesboat:fix/1533-codex-stop-json
Open

fix(hooks): emit valid Codex cleanup output#1687
Bortlesboat wants to merge 1 commit into
oraios:mainfrom
Bortlesboat:fix/1533-codex-stop-json

Conversation

@Bortlesboat

Copy link
Copy Markdown
Contributor

Summary

  • emit the Codex Stop-hook success object after cleanup
  • keep cleanup diagnostics on stderr so stdout remains parseable JSON
  • return valid JSON even when Codex input lacks a session ID
  • preserve existing behavior for all other hook clients

Closes #1533

Verification

  • uv run pytest test/serena/test_hooks.py::TestHookCli::test_cleanup_command test/serena/test_hooks.py::TestHookCli::test_codex_cleanup_emits_valid_stop_hook_json test/serena/test_hooks.py::TestHookCli::test_codex_cleanup_without_session_id_still_emits_valid_json -q (3 passed)
  • uv run ruff check src/serena/hooks.py test/serena/test_hooks.py
  • uv run ruff format --check src/serena/hooks.py test/serena/test_hooks.py
  • uv run ty check src/serena/hooks.py
  • git diff --check

@Bortlesboat

Copy link
Copy Markdown
Contributor Author

CI note: the Ubuntu catch-all failure is an unrelated timeout in test_dynamically_activated_mode_is_provided_once_per_session[python] after 1,000 tests passed; the tool returned TimeoutError after two four-minute waits. The changed Codex cleanup-hook tests pass, and the other completed platform/language shards plus build, spelling, and CodeQL are green.

@Bortlesboat

Copy link
Copy Markdown
Contributor Author

The remaining two red Ubuntu lanes are infrastructure/flaky rather than branch failures:
ative was cancelled after exactly six hours without completing, and catch-all reached 1,000 passing tests before one unrelated TestPromptProvision call timed out after ~15 minutes. All 15 other executed matrix jobs pass. I tried rerunning failed jobs, but GitHub restricts workflow reruns here to repository admins. Could a maintainer rerun failed jobs for workflow 29207580519 when capacity permits?

@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.

This resolves both scenarios from #1533 for the cleanup/Stop hook, confirmed against this branch (4231098) in a clean container: cleanup --client=codex with a session_id now prints {"continue": true}, and with a missing session_id it prints the same JSON to stdout while the ValueError goes to stderr, exit 0 either way.

One related gap this PR doesn't touch: activate (the SessionStart hook, hooks.py:586-588) has the identical failure shape as scenario 2. It has no try/except, so a missing session_id raises uncaught through Hook.__init__, same as cleanup did before this fix. Reproduced on this branch:

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

exits nonzero with a raw traceback, no JSON on stdout, for every client including codex. If Codex's SessionStart hook has the same "must be valid JSON" contract as Stop, this would trip the same class of error this PR fixes for cleanup. Worth a follow-up PR applying the same guard to activate, or is SessionStart's contract different enough that it's a non-issue there?

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