Skip to content
Source
Sign inSign up

fix(codex): accept hook event names newer than the pinned protocol #35885

Mergedjuliusmarmingemerged 2 commits intomain fromrefs/pull/35885/headmerged by juliusmarminge View on GitHub

juliusmarmingecommented

Codex 0.166+ sends hook/started and hook/completed notifications for preModelRequest and postModelRequest hooks. The generated Codex bindings (pinned to an older protocol ref) closed HookEventName over 12 literal names, so these notifications failed to decode and client.ts dropped them. CodexAdapterV2 never saw the stopped run, and the hook's stop reason was lost. The fix adds HookEventName: { type: "string" } to the generator's DefinitionOverrides, next to PlanType, which is already open for the same reason. Regenerating from the same pinned ref (687a119f) turns the five closed *__HookEventName lists in schema.gen.ts into Schema.String and changes nothing else. The generator also added a header comment that the checked-in files lack; it was removed so meta.gen.ts and namespaces.gen.ts stay unchanged. The only consumer is CodexAdapterV2, which puts eventName into a stop-reason string and works the same with any string. No new regex or parser.

Fixes pingdotgg/t3code#35853

Reproduction on main

On origin/main 95cb068c10 (Node v26.12.1), after vp i in a fresh worktree, I ran the reporter's probe as an ES module from packages/effect-codex-app-server. It uses makeInMemoryStdio plus Client.make, registers a hook/completed handler and an unknown-notification barrier, then sends hook/completed frames for eventName preCompact (control), preModelRequest and postModelRequest. Each frame has a stopped run with a stop entry, followed by a barrier frame. Output: Delivered hook events: ["preCompact"], so both model-request hook notifications were dropped. I then added a schema test to packages/effect-codex-app-server/src/schema.test.ts that checks V2HookStartedNotification and V2HookCompletedNotification against both new event names. vp test run packages/effect-codex-app-server/src/schema.test.ts fails on main (1 failed, 5 passed); the same fixture with eventName preCompact passes. No live Codex process or UI was exercised.

Evidence

Send preCompact (control), preModelRequest and postModelRequest through the real Codex client. Each hook/completed frame contains a stopped run and the stop reason Checkpoint required before continuing., followed by an interrupted terminal notification. The base loses both model-request reasons; the head preserves them. The control works on both.

Environment on both sides: Linux, Node v26.12.1, Chromium 151, light theme, 1440×900 viewport, identically cropped to the 1120×900 conversation area. Real servers and web clients with synthetic CLI-created projects and a controlled Codex transport.

BeforeAfter
95cb068c10729fed007f
Before: both model-request stop reasons missingAfter: both model-request stop reasons preserved
Before recordingAfter recording
Base runtime transcriptHead runtime transcript

Protocol fixture · CLI driver · Reproduction instructions

The scenario replay provider uses ACP and bypasses this Codex decoder, so this proof used the manual path. No live Codex account or actual model request was exercised.

Not shown:

  • Actual Codex hook execution and live model requests were not exercised; controlled protocol frames reproduced the reported decoder failure through the real app.
  • hook/started and arbitrary future hook event names were not exercised; this proof covers hook/completed for preModelRequest and postModelRequest.
  • Desktop and mobile clients were not exercised.

Tests

Before the fix: vp test run packages/effect-codex-app-server/src/schema.test.ts failed 1 of 6 (the repro test), reproducing the bug. After the fix: the same command passed 6 of 6. vp run check:changed passed: format, lint, knip, typecheck of 4 packages, and tests in 2 files (8 passed). No live Codex process was run.

Not in this change

  • Bumping the pinned upstream Codex protocol ref (UPSTREAM_REF) to 0.166+ would add the new literals and any other protocol changes; that is a broader regeneration than this fix.
  • The generator writes a 'generated file / upstream ref' header that the checked-in generated files lack; generator output and checked-in files have drifted on that header.

Who did the work

  • Written by Claude Opus 5.5, in Claude Code
  • Reviewed by GPT-6.1 Sol, in Codex, and by Claude Opus 5.5, each on its own
  • Proof captured by GPT-6.1 Sol and checked by Claude Opus 5.5

Run by the t3code factory in T3 Code.

    1. tanstack-source added labelsbug

    2. github-actions[bot](GitHub profile, opens in a new tab) added labelsvouch:trusted

    3. Approvability

      Verdict: Approved at 729fed0

      Macroscope's review found this PR approvable — Codex protocol decoding now accepts newer hook-event names instead of dropping their notifications. Existing stopped-run and turn/thread checks remain intact, keeping this a narrowly scoped compatibility change.

      You can add or adjust custom eligibility rules. [Learn more](https://docs.macroscope.com/approvability#example-custom-eligibility-rules).
    4. Thread transfer impact

      ✅ Thread transfer remains within every enforced ceiling.

      ℹ️ The exact PR base did not have a successful artifact. Baseline uses the latest successful main measurement shown below.

      ProviderMetricMain baselineThis PRImpactPR ceiling
      CodexTotal thread wire5.0 KiB5.0 KiB0 B (0.0%)6.8 KiB✅
      CodexThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
      CodexLive turn WebSocket wire1.2 KiB1.2 KiB0 B (0.0%)2.0 KiB✅
      CodexLive turn WebSocket decoded21.5 KiB21.5 KiB0 B (0.0%)24.4 KiB✅
      CodexLive turn messages110 (0.0%)8✅
      ClaudeTotal thread wire5.0 KiB5.0 KiB0 B (0.0%)6.8 KiB✅
      ClaudeThread snapshot wire3.8 KiB3.8 KiB0 B (0.0%)4.9 KiB✅
      ClaudeLive turn WebSocket wire1.2 KiB1.2 KiB0 B (0.0%)2.0 KiB✅
      ClaudeLive turn WebSocket decoded21.9 KiB21.9 KiB0 B (0.0%)24.4 KiB✅
      ClaudeLive turn messages110 (0.0%)8✅

      Other scenarios

      All 64 scenario measurements match main.

      All 64 scenario measurements
      ProviderScenarioMetricMain baselineThis PRImpactPR ceiling
      Codexshort turn resumeWebSocket decoded10.5 KiB10.5 KiB0 B (0.0%)12.7 KiB✅
      Codexshort turn resumeWebSocket messages220 (0.0%)2✅
      Codexlong turn running, cold openHTTP decoded281.7 KiB281.7 KiB0 B (0.0%)302.7 KiB✅
      Codexlong turn running, cold openrows75750 (0.0%)75✅
      Codexlong turn running, history pageHTTP decoded92.4 KiB92.4 KiB0 B (0.0%)107.4 KiB✅
      Codexlong turn running, history pagerows75750 (0.0%)75✅
      Codexlong turn finished, cold openHTTP decoded381.5 KiB381.5 KiB0 B (0.0%)410.2 KiB✅
      Codexlong turn finished, cold openrows1511510 (0.0%)151✅
      Codexlong turn finished, socket cold openWebSocket decoded381.6 KiB381.6 KiB0 B (0.0%)410.2 KiB✅
      Codexlong turn finished, socket cold openWebSocket messages220 (0.0%)2✅
      Codexlong turn finished, socket cold openrows1511510 (0.0%)151✅
      Codexlong turn finished, work pageHTTP decoded186.2 KiB186.2 KiB0 B (0.0%)200.2 KiB✅
      Codexlong turn finished, work pagerows1501500 (0.0%)150✅
      Codexidle thread resumeWebSocket decoded65 B65 B0 B (0.0%)200 B✅
      Codexidle thread resumeWebSocket messages110 (0.0%)1✅
      Codexfleet shell snapshotHTTP decoded42.4 KiB42.4 KiB0 B (0.0%)53.7 KiB✅
      Codexfleet shell snapshotrows27270 (0.0%)27✅
      Codexfleet shell subscribeWebSocket decoded65 B65 B0 B (0.0%)200 B✅
      Codexfleet shell subscribeWebSocket messages110 (0.0%)1✅
      Codexfleet shell resume checkHTTP decoded18 B18 B0 B (0.0%)100 B✅
      Codexfleet shell resume checkrows0000✅
      Codexstale shell reloadHTTP decoded42.4 KiB42.4 KiB0 B (0.0%)53.7 KiB✅
      Codexstale shell reloadrows27270 (0.0%)27✅
      Codexstale shell resumeWebSocket decoded140 B140 B0 B (0.0%)400 B✅
      Codexstale shell resumeWebSocket messages220 (0.0%)2✅
      Codexstale todo list cold openHTTP decoded72.4 KiB72.4 KiB0 B (0.0%)83.0 KiB✅
      Codexstale todo list cold openrows75750 (0.0%)75✅
      Codexstale todo list cold opennodes220 (0.0%)4✅
      Codexstale todo list cold opensubagents220 (0.0%)4✅
      Codexstale todo list cold openplans110 (0.0%)2✅
      Codexstale todo list cold openproviderTurns110 (0.0%)2✅
      Codexstale todo list cold opencheckpoints110 (0.0%)2✅
      Claudeshort turn resumeWebSocket decoded10.6 KiB10.6 KiB0 B (0.0%)12.7 KiB✅
      Claudeshort turn resumeWebSocket messages220 (0.0%)2✅
      Claudelong turn running, cold openHTTP decoded283.3 KiB283.3 KiB0 B (0.0%)302.7 KiB✅
      Claudelong turn running, cold openrows75750 (0.0%)75✅
      Claudelong turn running, history pageHTTP decoded93.4 KiB93.4 KiB0 B (0.0%)107.4 KiB✅
      Claudelong turn running, history pagerows75750 (0.0%)75✅
      Claudelong turn finished, cold openHTTP decoded384.2 KiB384.2 KiB0 B (0.0%)410.2 KiB✅
      Claudelong turn finished, cold openrows1511510 (0.0%)151✅
      Claudelong turn finished, socket cold openWebSocket decoded384.3 KiB384.3 KiB0 B (0.0%)410.2 KiB✅
      Claudelong turn finished, socket cold openWebSocket messages220 (0.0%)2✅
      Claudelong turn finished, socket cold openrows1511510 (0.0%)151✅
      Claudelong turn finished, work pageHTTP decoded188.1 KiB188.1 KiB0 B (0.0%)200.2 KiB✅
      Claudelong turn finished, work pagerows1501500 (0.0%)150✅
      Claudeidle thread resumeWebSocket decoded66 B66 B0 B (0.0%)200 B✅
      Claudeidle thread resumeWebSocket messages110 (0.0%)1✅
      Claudefleet shell snapshotHTTP decoded43.0 KiB43.0 KiB0 B (0.0%)53.7 KiB✅
      Claudefleet shell snapshotrows27270 (0.0%)27✅
      Claudefleet shell subscribeWebSocket decoded66 B66 B0 B (0.0%)200 B✅
      Claudefleet shell subscribeWebSocket messages110 (0.0%)1✅
      Claudefleet shell resume checkHTTP decoded18 B18 B0 B (0.0%)100 B✅
      Claudefleet shell resume checkrows0000✅
      Claudestale shell reloadHTTP decoded43.0 KiB43.0 KiB0 B (0.0%)53.7 KiB✅
      Claudestale shell reloadrows27270 (0.0%)27✅
      Claudestale shell resumeWebSocket decoded140 B140 B0 B (0.0%)400 B✅
      Claudestale shell resumeWebSocket messages220 (0.0%)2✅
      Claudestale todo list cold openHTTP decoded73.4 KiB73.4 KiB0 B (0.0%)83.0 KiB✅
      Claudestale todo list cold openrows75750 (0.0%)75✅
      Claudestale todo list cold opennodes220 (0.0%)4✅
      Claudestale todo list cold opensubagents220 (0.0%)4✅
      Claudestale todo list cold openplans110 (0.0%)2✅
      Claudestale todo list cold openproviderTurns110 (0.0%)2✅
      Claudestale todo list cold opencheckpoints110 (0.0%)2✅

      Baseline: 36371de · PR result: 729fed0 · Source CI: success

      Scenario and decoded snapshot size

      10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

      • Codex decoded thread snapshot: 108.5 KiB
      • Claude decoded thread snapshot: 108.8 KiB

      Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

    5. Explain Change →

      No actionable comments were generated in the recent review.

      Walkthrough

      The generator now overrides HookEventName with a string schema. Tests check that hook-started and hook-completed notification predicates accept stopped synchronous preModelRequest and postModelRequest notifications.

      Changes: Model-request hook notification schemas

      Layer / File(s)Summary
      Hook event schema and validation
      packages/effect-codex-app-server/scripts/generate.ts, packages/effect-codex-app-server/src/schema.test.ts
      The generator overrides HookEventName with a string schema. Tests check that hook-started and hook-completed predicates accept stopped synchronous notifications for preModelRequest and postModelRequest.

      Estimated code review effort: 2 (Simple) | ~10 minutes

      Change: Bug fix · Severity of issue fixed: Medium

      Suggested reviewers: saphid, macroscopeapp, maria-rcks

      Merge Risk: Minimal · up to 729fe

      This change lets hook notifications for the newer model-request events be accepted instead of being rejected by the schema. No actionable merge-blocking risk remains.

      Pre-merge checks: Passed 3 · Inconclusive 1

      Failed Failed checks: 1 inconclusive
      Check nameStatusExplanationResolution
      Linked Issues checkInconclusiveIssue #35853 requires the typed client to deliver hook/completed notifications with eventName preModelRequest and postModelRequest. The fix depends on the closed HookEventName enum in packages/effect-…Provide review access to the regenerated packages/effect-codex-app-server/src/generated/schema.gen.ts, or confirm its diff (the five *_HookEventName lists changed to Schema.String and no other changes), so the generated decode behavior ca…
      Passed Passed checks: 3 passed

      Check nameStatusExplanation
      Title checkPassedThe title names the fix and its scope: the Codex app-server client now accepts hook event names newer than the pinned protocol. The PR adds a string override for HookEventName in the generator, regene…
      Out of Scope Changes checkPassedAll reviewable changes relate to the objective of issue #35853. The generator override in scripts/generate.ts (+2/-0) is the root cause fix for the closed HookEventName enum. The test additions in src…
      Description checkPassedThe description follows the repository template. It covers the problem (Codex 0.166+ hook/completed notifications for preModelRequest and postModelRequest failed to decode and were dropped), the chang…
      Full details: Title check

      Explanation

      The title names the fix and its scope: the Codex app-server client now accepts hook event names newer than the pinned protocol. The PR adds a string override for HookEventName in the generator, regenerates the five closed HookEventName lists to Schema.String, and adds a schema test for preModelRequest and postModelRequest hook notifications. The title is conventional, specific, and concise.

      Full details: Out of Scope Changes check

      Explanation

      All reviewable changes relate to the objective of issue #35853. The generator override in scripts/generate.ts (+2/-0) is the root cause fix for the closed HookEventName enum. The test additions in src/schema.test.ts (+25/-0) cover the two new event names and reproduce the reported failure. The excluded schema.gen.ts changes are the generated output of that override and are not assessed here. The PR excludes the UPSTREAM_REF bump and broader protocol regeneration. No unrelated reviewable change was found in the whole-PR summary.

      Full details: Description check

      Explanation

      The description follows the repository template. It covers the problem (Codex 0.166+ hook/completed notifications for preModelRequest and postModelRequest failed to decode and were dropped), the change (HookEventName overridden to a string schema in the generator, with regeneration from pinned ref 687a119f), scope (a linked issue, pingdotgg/t3code#35853, is provided), and verification (a before/after reproduction on main, a schema test that fails before and passes after, and check:changed passing). It also states what was not exercised: no live Codex process, no hook execution, and no desktop or mobile clients. Sections are populated with specific evidence. Minor gaps: the 'Problem' section is a short paragraph rather than the one-to-two-sentence format, and the agent attribution line is present. Those are non-critical.

      Full details: Linked Issues check

      Explanation

      Issue #35853 requires the typed client to deliver hook/completed notifications with eventName preModelRequest and postModelRequest. The fix depends on the closed HookEventName enum in packages/effect-codex-app-server/src/generated/schema.gen.ts. That file is excluded by the ignored_files rule '!/_generated/', so its content is not in review. The reviewable evidence is consistent with the fix. scripts/generate.ts adds a HookEventName: { type: "string" } override next to PlanType (+2 lines). src/schema.test.ts adds predicates and a test for V2HookStartedNotification and V2HookCompletedNotification that accept both new event names. The PR description reports that regeneration turns the five closed *_HookEventName lists into Schema.String, and that the test passes 6 of 6 after the fix. None of this can be verified from the reviewed diff. Whether the regenerated schema.gen.ts actually removes the closed literals, and whether the test passes against it, is unknown. The client.ts decode path was not changed in the reviewable scope, so it is not shown to be fixed independently of the generated schema. The issue's coding requirement therefore cannot be confirmed. The Evidence section of the PR description is still a placeholder and is not evidence.

      Resolution

      Provide review access to the regenerated packages/effect-codex-app-server/src/generated/schema.gen.ts, or confirm its diff (the five *_HookEventName lists changed to Schema.String and no other changes), so the generated decode behavior can be checked. Confirm that src/schema.test.ts passes against that generated file. Confirm the regeneration output for the generator-header removal described in the PR description.

      Finishing Touches

      Generate unit tests (beta)

      • Commit to this branch
      • Create a new PR
      • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
      Recent review info

      Run configuration

      Commits

      Reviewing files that changed from the base of the PR and between 95cb068c10a6cf0b33eed0a0cab3b298763fe852 and 729fed007fabcd3d1d00d1a09862d4d38dad0fc5.

      Files ignored due to path filters: 1

      • packages/effect-codex-app-server/src/_generated/schema.gen.ts is excluded by !**/_generated/**
      Files selected for processing: 2

      • packages/effect-codex-app-server/scripts/generate.ts
      • packages/effect-codex-app-server/src/schema.test.ts

      Limit details: You’ve used all 10 included reviews currently available. Your 17 included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour.

    6. juliusmarminge merged this6f108e8

    7. juliusmarminge closed this

Sign in to join this conversation.