From e9b4a3164104f3381e5df6af4eb1775add6c5b67 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Mon, 1 Jun 2026 11:31:35 -0400 Subject: [PATCH] docs(retro): v6.3.1 PreCompact Codex empty-JSON fix (#66) --- ...6.3.1-precompact-codex-empty-json-retro.md | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 docs/retros/2026-06-01-v6.3.1-precompact-codex-empty-json-retro.md diff --git a/docs/retros/2026-06-01-v6.3.1-precompact-codex-empty-json-retro.md b/docs/retros/2026-06-01-v6.3.1-precompact-codex-empty-json-retro.md new file mode 100644 index 0000000..3914a9d --- /dev/null +++ b/docs/retros/2026-06-01-v6.3.1-precompact-codex-empty-json-retro.md @@ -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) + +## 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. |