Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions docs/retros/2026-06-01-v6.3.1-precompact-codex-empty-json-retro.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# Retro: v6.3.1 — PreCompact invalid-JSON on Codex (#66)

**PR:** #67 — fix(hooks): PreCompact emits {} no-op (never empty) so Codex accepts the JSON
**Merged / Released:** 2026-06-01 · v6.3.1 (Latest)
**Issue:** #66 (regression/uncovered-path of #41)
**Design:** docs/plans/2026-06-01-precompact-codex-empty-json-design.md (adversarial PASS @ cycle 1+revision)

Comment on lines +1 to +7
## Root cause (systematic-debugging)

`hooks/pre-compact-snapshot` emitted **empty stdout** on its no-locked-plans path (the common
case at compaction) + the disabled/TTY/no-jq/empty-input guards. Claude Code tolerates empty
PreCompact output (no-op); **Codex rejects empty** as "invalid PreCompact hook JSON output".
The v6.3.0 wrapper (#41) recovered JSON behind diagnostics but its empty-output branch still
emitted nothing — so the empty path was uncovered on the wrapper route too.

## Invariant (backprop)

**Every hook emits a valid JSON object on stdout on every exit path; empty → `{}`.** `{}` is a
universal no-op on Claude Code and valid JSON for Codex. Encoded at two layers (hook +
wrapper) + a Codex-style direct-invocation regression. Stated for adversarial review next
time: *the strictest host (Codex) requires valid JSON on every hook exit; "Claude Code
tolerates empty" is a false assumption to challenge.*

## What worked

- **systematic-debugging over guessing:** root-caused to the empty-stdout common path (not a
diagnostic leak) by tracing the hook's exit paths against the exact error wording.
- **The adversarial design review caught a CI-red landmine:** it found **3 existing tests** that
assert pre-compact emits *empty* output (scope-lock-complete, draft-plan, single-workspace-
lock) — all would have broken on the `{}` change. Updated them to "no *populated* snapshot"
(`jq -e .hookSpecificOutput.additionalContext`) pre-merge. Without that catch, CI goes red.
- **Defense in depth:** fixed at the hook (Codex-direct path) AND the wrapper (all hooks on the
wrapper path), per #66's "not only the Claude wrapper path."

## What didn't / honest limitation

- **Could not reproduce on a live Codex** from the dev environment. The fix is verified on the
**source layout** (the hook now emits valid JSON on every path). `{}` resolves the
*not-valid-JSON* rejection class; a *typed-schema* rejection (if Codex enforces one) would
surface a different error — the issue-close note asks the reporter to reopen with Codex output
if so. This honesty (verification-before-completion: don't claim "fixed on Codex" without
Codex evidence) is the right call for a host I can't run.
- **The original #41 fix didn't cover this path** — it focused on diagnostic-leak recovery
(non-JSON behind a warning) and missed that *empty* stdout is also invalid for Codex. Lesson:
"valid JSON discipline" must include "never empty," not just "strip non-JSON."

## Follow-ups

- Other hooks' **direct-invocation** empty paths (session-start, scope-guards, etc.) are only
covered when Codex routes through `run-hook.cmd` (the wrapper `{}` fix). If Codex invokes them
directly too, they'd need the same hook-level `noop_json`. Not yet observed; noted.
- Consider a Codex+Claude hook-contract doc capturing "every hook emits valid JSON on every exit."

## Project guidance updates

| File | Change | Reason |
|---|---|---|
| (none) | no change | The invariant is encoded as the fix + regression tests + this retro; no separate guidance file exists yet. |
Comment on lines +54 to +58