Skip to content

fix(codex): clarify unsafe app-server completion stalls#87793

Open
slatem wants to merge 1 commit into
openclaw:mainfrom
slatem:fix-codex-terminal-event-release
Open

fix(codex): clarify unsafe app-server completion stalls#87793
slatem wants to merge 1 commit into
openclaw:mainfrom
slatem:fix-codex-terminal-event-release

Conversation

@slatem

@slatem slatem commented May 28, 2026

Copy link
Copy Markdown

Summary

  • clarify the replay-unsafe Codex app-server timeout message so users know OpenClaw intentionally did not auto-replay after tool activity
  • include completed item id/type/status/tool metadata in app-server idle-timeout diagnostics, matching the raw response diagnostics already emitted
  • add focused coverage for completed item diagnostic details and update timeout outcome expectations

Why

When Codex app-server emits completed tool/item notifications but never sends the terminal turn/completed event, OpenClaw currently reports that work may already have happened but does not explain that replay was intentionally suppressed to avoid duplicate side effects. This makes the incident look like a generic bot failure instead of a replay-safety guard. The extra diagnostic fields also make future reports easier to triage from trajectory/log data.

Tests

  • OPENCLAW_VITEST_INCLUDE_FILE=/tmp/openclaw-codex-tests.json node scripts/run-vitest.mjs run --config test/vitest/vitest.extension-codex.config.ts
  • pnpm exec oxfmt --check --threads=1 extensions/codex/src/app-server/attempt-notifications.ts extensions/codex/src/app-server/attempt-notifications.test.ts extensions/codex/src/app-server/attempt-results.ts extensions/codex/src/app-server/attempt-results.test.ts extensions/codex/src/app-server/run-attempt.turn-watches.test.ts

Related: #87781 handles replay-safe app-server stalls. This PR covers the replay-unsafe/user-facing side-effect path.

@openclaw-barnacle openclaw-barnacle Bot added extensions: codex size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 28, 2026
@clawsweeper

clawsweeper Bot commented May 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 2:38 PM ET / 18:38 UTC.

Summary
The branch updates Codex app-server replay-unsafe timeout wording, adds item/completed id/type/status/tool activity metadata, and adds focused tests for notification metadata and timeout outcome text.

PR surface: Source +9, Tests +29. Total +38 across 5 files.

Reproducibility: yes. at source level: current main and v2026.6.10 still use the older replay-unsafe timeout copy and method-only item/completed activity. I did not run a live app-server timeout, and the contributor has not supplied real-run proof.

Review metrics: 1 noteworthy metric.

  • Diagnostic field forwarding: 2 activity keys added, 0 structured result fields added. The PR adds status/tool activity keys, but the public typed timeout diagnostics do not expose those keys before merge.

Root-cause cluster
Relationship: partial_overlap
Canonical: #87744
Summary: This PR is a narrow unsafe-stall messaging/diagnostics improvement within the broader Codex missing-terminal completion-stall cluster; the canonical remaining behavior thread is the open reliability issue.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • [P2] Add redacted real app-server output, logs, terminal screenshot, linked artifact, or recording showing the changed timeout message and item metadata after the patch.
  • Rebase or refresh the branch against current main so the dependency guard and mergeability can be re-evaluated.
  • Forward lastNotificationItemStatus and lastNotificationTool through typed structured timeout diagnostics, or narrow the PR to log-only diagnostics.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body and comments list tests only; no redacted real Codex app-server run output, logs, terminal screenshot, linked artifact, or recording shows the changed message and metadata after the patch. 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

  • [P2] External PR policy still needs redacted real Codex app-server output, logs, terminal screenshot, linked artifact, or recording showing the changed timeout message and metadata after the patch.
  • [P1] The branch is currently conflicting/dirty against main, so the patch needs a refresh before mergeability and dependency guard state are meaningful.
  • [P1] The added status/tool activity fields are not forwarded through the typed codexAppServerFailure.diagnostics surface, so maintainers need either a forwarding fix or an explicit decision that those fields are log-only.

Maintainer options:

  1. Decide the mitigation before merge
    Rebase on current main, forward item/completed status/tool through typed structured timeout diagnostics or narrow the PR claim to log-only metadata, then attach redacted real Codex app-server timeout proof.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The remaining blockers are contributor-supplied real behavior proof, branch refresh, and a maintainer-visible diagnostics-surface repair, so this should not enter an automated repair lane yet.

Security
Cleared: The diff only changes Codex app-server TypeScript helpers and tests, with no dependency, workflow, secret, packaging, or code-execution surface change.

Review findings

  • [P2] Forward new item fields through timeout diagnostics — extensions/codex/src/app-server/attempt-notifications.ts:40-41
Review details

Best possible solution:

Rebase on current main, forward item/completed status/tool through typed structured timeout diagnostics or narrow the PR claim to log-only metadata, then attach redacted real Codex app-server timeout proof.

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

Yes, at source level: current main and v2026.6.10 still use the older replay-unsafe timeout copy and method-only item/completed activity. I did not run a live app-server timeout, and the contributor has not supplied real-run proof.

Is this the best way to solve the issue?

No, not as submitted: the wording change is in the right Codex app-server attempt-result layer, but the diagnostic metadata half is incomplete because the new status/tool keys are dropped before the structured failure result. The safer path is to rebase, forward or intentionally omit those fields, and prove the changed real behavior.

Full review comments:

  • [P2] Forward new item fields through timeout diagnostics — extensions/codex/src/app-server/attempt-notifications.ts:40-41
    The helper now returns lastNotificationItemStatus and lastNotificationTool, but buildCodexAppServerTimeoutDiagnostics and EmbeddedRunAttemptResult.codexAppServerFailure.diagnostics only forward the existing method/id/type/role/preview allowlist. The new test passes at the helper boundary while the structured timeout diagnostics the PR says it enriches still drop the new fields.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.87

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 82dfd8910743.

Label changes

Label justifications:

  • P2: This is a bounded Codex app-server diagnostic and user-facing timeout-message improvement with limited blast radius, but it still needs proof and a focused repair.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • 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 and comments list tests only; no redacted real Codex app-server run output, logs, terminal screenshot, linked artifact, or recording shows the changed message and metadata after the patch. 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 +9, Tests +29. Total +38 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 2 12 3 +9
Tests 3 33 4 +29
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 45 7 +38

What I checked:

  • PR state and proof gate: Live PR metadata shows head 1f272b3 is cross-repository, maintainer-editable, mergeable=CONFLICTING, mergeStateStatus=DIRTY, and still labeled as needing real behavior proof. (1f272b3dc3b9)
  • Current main still has the older side-effect timeout copy: Current main still reports the replay-unsafe completion-idle message without saying OpenClaw intentionally skipped replay after tool activity. (extensions/codex/src/app-server/attempt-results.ts:16, 82dfd8910743)
  • Current main still treats non-raw item/completed diagnostics as method-only: describeNotificationActivity currently returns only lastNotificationMethod for non-rawResponseItem/completed notifications, so the PR's central metadata improvement is not already on main. (extensions/codex/src/app-server/attempt-notifications.ts:31, 82dfd8910743)
  • Structured diagnostics allowlist drops the new fields: buildCodexAppServerTimeoutDiagnostics forwards method, item id/type/role, assistant preview, counts, and watch state, but not lastNotificationItemStatus or lastNotificationTool. (extensions/codex/src/app-server/run-attempt.ts:3398, 82dfd8910743)
  • Result type lacks status/tool diagnostic fields: EmbeddedRunAttemptResult.codexAppServerFailure.diagnostics has typed fields for item id/type/role and assistant preview, but no typed status/tool fields for consumers of the structured failure result. (src/agents/embedded-agent-runner/run/types.ts:174, 82dfd8910743)
  • PR diff adds status/tool only at the notification helper boundary: The patch adds lastNotificationItemStatus and lastNotificationTool in describeNotificationActivity, but does not update the downstream typed diagnostics/result surface. (extensions/codex/src/app-server/attempt-notifications.ts:40, 1f272b3dc3b9)

Likely related people:

  • Kevin Lin: Authored the merged completion-timeout diagnostics commit that introduced the current structured codexAppServerFailure.diagnostics allowlist adjacent to this PR's missing fields. (role: recent diagnostics contributor; confidence: high; commits: ab7c92282522; files: extensions/codex/src/app-server/run-attempt.ts, extensions/codex/src/app-server/run-attempt.turn-watches.test.ts, src/agents/embedded-agent-runner/run/types.ts)
  • keshavbotagent: Authored the merged completion-stall mitigation PR that changed the central Codex app-server timeout, notification, run-attempt, and recovery paths this PR builds on. (role: recent related contributor; confidence: high; commits: 5f89fbe6699e; files: extensions/codex/src/app-server/attempt-notifications.ts, extensions/codex/src/app-server/run-attempt.ts, extensions/codex/src/app-server/run-attempt.turn-watches.test.ts)
  • steipete: Merged the related completion-stall mitigation and authored later commits around replay-safe completion timeout recovery and side-effect timeout surfacing. (role: merger and adjacent contributor; confidence: high; commits: 5f89fbe6699e, 2d0ff138b698, 1c1b7be2e045; files: extensions/codex/src/app-server/run-attempt.ts, src/agents/embedded-agent-runner/run/codex-app-server-recovery.ts)
  • joshavant: Authored the merged predecessor PR that handled Codex app-server turns that stop before matching turn/completed. (role: prior missing-terminal fix author; confidence: high; commits: 7cda26aa6c72; files: extensions/codex/src/app-server/run-attempt.ts, extensions/codex/src/app-server/event-projector.ts, extensions/codex/src/app-server/run-attempt.test.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.
Review history (1 earlier review cycle)
  • reviewed 2026-06-21T05:46:25.660Z sha 1f272b3 :: needs real behavior proof before merge. :: [P2] Forward the new item fields through timeout diagnostics

@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. P2 Normal backlog priority with limited blast radius. labels May 28, 2026
@RomneyDa

RomneyDa commented May 29, 2026

Copy link
Copy Markdown
Member

Heads up: this PR needs to be updated against current main before the new required Dependency Guard check can pass.

@clawsweeper clawsweeper Bot mentioned this pull request May 29, 2026
Merged
@clawsweeper

clawsweeper Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

@slatem thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward.

Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive.

Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it.

@clawsweeper clawsweeper Bot mentioned this pull request Jun 21, 2026
Open
@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

@slatem thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward.

Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive.

Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it.

@openclaw-barnacle

openclaw-barnacle Bot commented Jul 20, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added stale Marked as stale due to inactivity triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed stale Marked as stale due to inactivity labels Jul 20, 2026
@clawsweeper

clawsweeper Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(codex): clarify unsafe app-server completion stalls This is item 1/1 in the current shard. Shard 15/22.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

extensions: codex P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@slatem @RomneyDa