Skip to content

fix(codex): terminate message-tool source replies#95839

Closed
shad0wca7 wants to merge 1 commit into
openclaw:mainfrom
shad0wca7:kit/fix-codex-message-tool-source-replies
Closed

fix(codex): terminate message-tool source replies#95839
shad0wca7 wants to merge 1 commit into
openclaw:mainfrom
shad0wca7:kit/fix-codex-message-tool-source-replies

Conversation

@shad0wca7

@shad0wca7 shad0wca7 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Marks delivered implicit message_tool_only source replies as terminal for Codex dynamic-tool runs.
  • Passes sourceReplyDeliveryMode into the Codex dynamic tool hook context.
  • Adds regression coverage proving implicit source replies terminate while explicit cross-channel message sends do not.

This is a minimal alternative/companion to #95826. It keeps the fix scoped to the Codex dynamic-tool termination path and shared harness export without the additional iMessage/docs changes in that draft.

Root Cause

The embedded-agent path already has shared logic for detecting when a message.send call delivered the visible source reply in message_tool_only mode. The Codex app-server dynamic-tool bridge was not receiving sourceReplyDeliveryMode, so those delivered source replies were not included in Codex's dynamic-tool termination decision. Codex could therefore keep waiting after the user-visible reply had already been sent.

Test Plan

  • node scripts/run-vitest.mjs run extensions/codex/src/app-server/dynamic-tools.test.ts
  • node scripts/run-vitest.mjs run src/agents/embedded-agent-runner/run/message-tool-terminal.test.ts src/agents/embedded-agent-message-tool-source-reply.test.ts
  • git diff --check origin/main...HEAD
  • pnpm build currently fails on current origin/main declaration generation in unrelated src/commands/doctor/shared/preview-warnings.ts type errors:
    • ToolProfilePolicy | undefined is not assignable to ToolPolicyConfig at lines 450, 501, and 637.
    • This PR does not touch that file.

@openclaw-barnacle openclaw-barnacle Bot added extensions: codex size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jun 22, 2026
@clawsweeper

clawsweeper Bot commented Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 23, 2026, 12:03 PM ET / 16:03 UTC.

Summary
The PR threads sourceReplyDeliveryMode into Codex dynamic-tool handling, marks delivered message_tool_only source replies as terminal, and adds focused regression tests.

PR surface: Source +12, Tests +33. Total +45 across 4 files.

Reproducibility: yes. source-reproducible: current main has the shared source-reply detector and dynamic-tool termination hook, but the Codex bridge does not pass sourceReplyDeliveryMode into that decision. I did not run a live channel reproduction in this read-only review.

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 live output, logs, terminal screenshot, or linked artifact showing a Codex message_tool_only run terminates immediately after the visible source reply.
  • After adding proof, update the PR body so ClawSweeper re-reviews automatically; if it does not, ask a maintainer to comment @clawsweeper re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists focused tests and diff checks, but no after-fix live Codex/channel proof; the contributor should post redacted logs, terminal output, screenshot, recording, or artifact and update the PR body to trigger re-review.

Risk before merge

  • [P1] This external PR has only test/diff-check evidence; it still needs redacted live output, logs, terminal screenshot, or an artifact showing a Codex message_tool_only turn terminates after the visible source reply.
  • [P1] A broader related PR has stronger live proof and route handling, but it is currently conflicting, so maintainers need to choose whether to repair that branch or keep this narrower branch as the landing path.

Maintainer options:

  1. Decide the mitigation before merge
    Keep the local non-enumerable termination signal, thread sourceReplyDeliveryMode into the Codex bridge, and require redacted live proof that a message_tool_only Codex turn completes immediately after the visible source reply.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The branch is a plausible narrow fix, but an external PR needs real behavior proof and a maintainer choice versus the broader conflicting replacement before merge.

Security
Cleared: The diff only changes TypeScript runtime/test wiring and a local SDK re-export; it does not touch secrets, dependencies, workflows, package scripts, or downloaded code execution.

Review details

Best possible solution:

Keep the local non-enumerable termination signal, thread sourceReplyDeliveryMode into the Codex bridge, and require redacted live proof that a message_tool_only Codex turn completes immediately after the visible source reply.

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

Yes, source-reproducible: current main has the shared source-reply detector and dynamic-tool termination hook, but the Codex bridge does not pass sourceReplyDeliveryMode into that decision. I did not run a live channel reproduction in this read-only review.

Is this the best way to solve the issue?

Yes for the narrow implicit-send path: the PR reuses the existing shared detector and leaves Codex's JSON protocol unchanged. The remaining gap is live behavior proof and maintainer choice versus the broader conflicting replacement.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: The PR targets a normal-priority Codex/message-tool runtime bug with limited scope.
  • 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 lists focused tests and diff checks, but no after-fix live Codex/channel proof; the contributor should post redacted logs, terminal output, screenshot, recording, or artifact and update the PR body to trigger re-review.

Label justifications:

  • P2: The PR targets a normal-priority Codex/message-tool runtime bug with limited scope.
  • 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 lists focused tests and diff checks, but no after-fix live Codex/channel proof; the contributor should post redacted logs, terminal output, screenshot, recording, or artifact and update the PR body to trigger re-review.
Evidence reviewed

PR surface:

Source +12, Tests +33. Total +45 across 4 files.

View PR surface stats
Area Files Added Removed Net
Source 3 12 0 +12
Tests 1 33 0 +33
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 4 45 0 +45

What I checked:

Likely related people:

  • Vincent Koc: Focused blame and -S history show recent work on the Codex dynamic-tool bridge, termination path, and shared source-reply detector. (role: recent area contributor; confidence: medium; commits: 2af06042c2ad, c645ec4555c0; files: extensions/codex/src/app-server/dynamic-tools.ts, extensions/codex/src/app-server/run-attempt.ts, src/agents/embedded-agent-message-tool-source-reply.ts)
  • Peter Steinberger: History for createCodexDynamicToolBridge includes the earlier move that kept the app-server implementation inside the Codex extension source tree. (role: introduced/relocated adjacent Codex app-server surface; confidence: medium; commits: d5698038d71c; files: extensions/codex/src/app-server/dynamic-tools.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.

@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 Jun 23, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 23, 2026
Merged
@omarshahine

omarshahine commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

@/var/folders/ks/54l0_v5j441gbf_8nbz9kgb80000gn/T/tmp.imm790J0Rs

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: XS 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@shad0wca7 @omarshahine