Skip to content

fix(codex): restore bounded recovery continuity#88652

Merged
steipete merged 2 commits into
mainfrom
codex/bounded-continuity-recovery
May 31, 2026
Merged

fix(codex): restore bounded recovery continuity#88652
steipete merged 2 commits into
mainfrom
codex/bounded-continuity-recovery

Conversation

@steipete

@steipete steipete commented May 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • restore bounded OpenClaw-visible continuity for Codex native recovery paths: fresh/transient starts and stale bindings with newer visible messages
  • keep healthy native resumes from replaying mirrored transcript history, including messages already covered by a successful native turn
  • keep post-start continuity as turn input only so native Codex thread developer instructions stay stable
  • filter only Codex app-server mirror echoes, not other backend mirror identities

Codex source checked

  • ../codex/codex-rs/app-server-protocol/src/protocol/v2/thread.rs:95 and ../codex/codex-rs/app-server-protocol/src/protocol/v2/thread.rs:330: thread/start and thread/resume carry developer_instructions.
  • ../codex/codex-rs/app-server-protocol/src/protocol/v2/turn.rs:66: turn/start carries input and turn settings, not thread-level developer instructions.
  • ../codex/codex-rs/app-server/src/request_processors/thread_processor.rs:830 and ../codex/codex-rs/app-server/src/request_processors/thread_processor.rs:2474: start/resume map developer instructions into thread config overrides.
  • ../codex/codex-rs/app-server/src/request_processors/turn_processor.rs:413: turn input maps into Op::UserInput.
  • ../codex/codex-rs/core/src/session/mod.rs:573 and ../codex/codex-rs/core/src/session/mod.rs:2692: Codex stores thread developer instructions in session config and emits them as developer messages.

Verification

  • pnpm exec oxfmt --check --threads=1 extensions/codex/src/app-server/run-attempt.ts extensions/codex/src/app-server/run-attempt.test.ts
  • git diff --check
  • node scripts/run-vitest.mjs run --config test/vitest/vitest.extension-codex.config.ts extensions/codex/src/app-server/run-attempt.test.ts
  • .agents/skills/autoreview/scripts/autoreview --mode local --prompt "..." clean after fixing accepted findings

Real behavior proof

Behavior addressed: Codex app-server recovery paths could lose OpenClaw-visible continuity when no active native Codex thread already covered the visible transcript, while healthy native resumes must still avoid duplicated mirrored history.

Real environment tested: Local OpenClaw source checkout with mocked Codex app-server lifecycle tests exercising thread/start, thread/resume, turn/start, binding rotation, mirrored transcript filtering, and successful-turn binding coverage.

Observed result after fix: fresh native starts receive bounded continuity; stale bindings receive only newer visible messages; active-turn persisted messages are not replayed on the next resume; Codex app-server mirror echoes are suppressed without suppressing other backend mirror identities; thread-start developer instructions remain stable after native start.

What was not tested: packaged app-server binary or live upstream model completion.

Closes #88352.
Closes #88354.

@clawsweeper

clawsweeper Bot commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed May 31, 2026, 10:21 AM ET / 14:21 UTC.

Summary
Restores Codex app-server bounded continuity projection for fresh/transient and stale-binding recovery paths, adds binding coverage watermarking after successful turns, and expands mocked lifecycle tests.

PR surface: Source +145, Tests +234. Total +379 across 2 files.

Reproducibility: yes. source-reproducible: current main has empty Codex model-input history plus transient/fresh thread paths, and the linked issues give focused run-attempt test shapes. I did not execute tests because this review is read-only.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted real Codex app-server recovery proof for at least one fresh/transient start and one stale-binding resume path.
  • Include the exact command or run setup used for that proof in the PR body.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body reports local mocked lifecycle tests only; external merge readiness needs real app-server proof such as redacted terminal output, logs, or a linked artifact, and updating the PR body should trigger re-review or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] This changes Codex session-continuity and binding watermark semantics; green unit tests alone do not prove a live or packaged app-server recovery path will avoid either lost continuity or duplicated native history.
  • [P1] The PR has the protected maintainer label, so cleanup automation should not close or land it without explicit maintainer handling.

Maintainer options:

  1. Require live recovery proof (recommended)
    Before merge, require terminal output, logs, or a linked artifact from a real Codex app-server recovery run that shows fresh/transient start and stale-binding resume preserve continuity without replaying covered native history.
  2. Accept mocked-only coverage
    Maintainers can intentionally accept the session-state risk based on the focused mocked lifecycle tests and their domain knowledge of the Codex app-server bridge.
  3. Pause for binding contract clarity
    If the updatedAt coverage watermark is not the intended long-term contract, pause the PR until the session-state ownership rule is documented or narrowed.

Next step before merge

  • [P1] Human review should decide whether the protected-label PR has enough live recovery proof or needs additional Codex app-server validation before merge.

Security
Cleared: The diff only changes Codex app-server runtime/test code and does not touch workflows, dependencies, lockfiles, secrets handling, or package execution surfaces.

Review details

Best possible solution:

Land after maintainer review with real Codex app-server recovery proof, keeping continuity as bounded turn input while leaving native thread developer instructions stable.

Do we have a high-confidence way to reproduce the issue?

Yes, source-reproducible: current main has empty Codex model-input history plus transient/fresh thread paths, and the linked issues give focused run-attempt test shapes. I did not execute tests because this review is read-only.

Is this the best way to solve the issue?

Yes, likely: a separate bounded continuity projection while keeping parallel model history empty matches the inspected Codex thread/turn contract. The remaining gap is live recovery proof rather than an obvious better code path.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 1bfae9d45890.

Label changes

Label changes:

  • add P1: The PR targets linked P1 Codex session-continuity regressions where recovered turns can silently lose visible transcript context.
  • add merge-risk: 🚨 session-state: The diff changes how Codex native thread bindings decide which OpenClaw-visible transcript messages are covered, stale, or replayed.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports local mocked lifecycle tests only; external merge readiness needs real app-server proof such as redacted terminal output, logs, or a linked artifact, and updating the PR body should trigger re-review or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P1: The PR targets linked P1 Codex session-continuity regressions where recovered turns can silently lose visible transcript context.
  • merge-risk: 🚨 session-state: The diff changes how Codex native thread bindings decide which OpenClaw-visible transcript messages are covered, stale, or replayed.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports local mocked lifecycle tests only; external merge readiness needs real app-server proof such as redacted terminal output, logs, or a linked artifact, and updating the PR body should trigger re-review or a maintainer can comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +145, Tests +234. Total +379 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 147 2 +145
Tests 1 250 16 +234
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 397 18 +379

What I checked:

Likely related people:

  • steipete: Current-main blame for the Codex app-server run, binding, and lifecycle files points to Peter Steinberger's recent commits; the linked regression provenance commit and this PR are also authored by the same handle. (role: recent area contributor and current PR author with prior merged history; confidence: high; commits: 2fe019ccaed8, 530351e394a1, b9906495607a; files: extensions/codex/src/app-server/run-attempt.ts, extensions/codex/src/app-server/thread-lifecycle.ts, extensions/codex/src/app-server/session-binding.ts)
  • Vincent Koc: Recent history shows adjacent Codex auth/resume handling work in the same lifecycle and binding area, making this a useful secondary routing candidate if auth-bound resume behavior is affected. (role: recent adjacent contributor; confidence: medium; commits: 4a4f52b09730, 859eb0666282; files: extensions/codex/src/app-server/thread-lifecycle.ts, extensions/codex/src/app-server/session-binding.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed May 31, 2026
View reviewed changes

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b990649560

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread extensions/codex/src/app-server/run-attempt.ts
: typeof message.timestamp === "string"
? Date.parse(message.timestamp)
: Number.NaN;
return Number.isFinite(timestamp) && timestamp > bindingUpdatedAt;

@chatgpt-codex-connector chatgpt-codex-connector Bot May 31, 2026

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.

P2 Badge Advance the continuity cutoff only after coverage

When a stale binding is resumed and the attempt then fails before a successful turn/completed (for example a transient turn/start error), startOrResumeThread has already rewritten the binding during thread_resume_write_binding, so binding.updatedAt now points at the failed attempt rather than the last covered native turn. On the next retry this new cutoff drops the post-binding visible messages here even though they were never covered by Codex, so a single transient failure can permanently lose the recovery continuity this patch is restoring. Use a separate covered-through timestamp or only advance the cutoff after a successful turn.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels May 31, 2026
chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed May 31, 2026
View reviewed changes

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4f2e3d63a1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread extensions/codex/src/app-server/run-attempt.ts
Comment on lines +897 to +899
if (action === "started") {
applyFreshThreadContinuityProjection();
return true;

@chatgpt-codex-connector chatgpt-codex-connector Bot May 31, 2026

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.

P2 Badge Preserve stale-binding cutoff for fresh starts

When a stale binding has newer post-binding messages, this branch only reuses that bounded projection if rotateStartupBindingForProjectedTurn set staleBindingContinuityForcedFreshStart. Other normal paths inside startOrResumeThread can also convert the run to action: "started" after the precompute, such as dynamic-tool/MCP/plugin/env fingerprint changes or a resume failure, and then this fallback calls applyFreshThreadContinuityProjection() and replays the entire mirrored transcript instead of only messages newer than the binding cutoff. In those fresh-start recoveries, already-covered native turns can be duplicated into the prompt and large old history can blow the bounded recovery this change is trying to provide.

Useful? React with 👍 / 👎.

@steipete

steipete commented May 31, 2026

Copy link
Copy Markdown
Contributor Author

Land-ready verification for head 4f2e3d63a17231534652f76549405e0f5198dc5f.

Local proof:

  • pnpm exec oxfmt --check --threads=1 extensions/codex/src/app-server/run-attempt.ts extensions/codex/src/app-server/run-attempt.test.ts
  • git diff --check
  • node scripts/run-vitest.mjs run --config test/vitest/vitest.extension-codex.config.ts extensions/codex/src/app-server/run-attempt.test.ts
  • pnpm tsgo:extensions:test
  • pnpm run lint:extensions:bundled
  • pnpm lint --threads=8
  • .agents/skills/autoreview/scripts/autoreview --mode local --prompt "..." clean after fixup

CI proof:

  • CI run 26715813596 clear on the PR head.

Behavior proof:

  • Fresh Codex native starts project bounded visible continuity.
  • Stale native bindings project only newer visible user/assistant messages.
  • Codex app-server mirror echoes are not replayed.
  • Other backend mirror identities remain eligible continuity.
  • Successful native turns advance binding coverage so active-turn persisted messages are not replayed on the next resume.
  • Thread-start developer instructions remain stable; recovery continuity is turn input only.

Known proof gap: no packaged app-server binary or live upstream model completion run for this PR.

@steipete
steipete merged commit 827ceb5 into main May 31, 2026
137 checks passed
@steipete
steipete deleted the codex/bounded-continuity-recovery branch May 31, 2026 14:55
This was referenced May 31, 2026
Closed
Closed
This was referenced Jun 15, 2026
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

Assignees

No one assigned

Labels

extensions: codex maintainer Maintainer-authored PR rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

1 participant

@steipete