Skip to content

fix(codex): require explicit final source delivery#105365

Merged
obviyus merged 2 commits into
openclaw:mainfrom
Glucksberg:fix/codex-explicit-final-delivery
Jul 13, 2026
Merged

fix(codex): require explicit final source delivery#105365
obviyus merged 2 commits into
openclaw:mainfrom
Glucksberg:fix/codex-explicit-final-delivery

Conversation

@Glucksberg

@Glucksberg Glucksberg commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

In Codex message_tool_only runs, a confirmed progress acknowledgement was classified as a completed source reply. The turn could end early, or a later model/tool failure could be suppressed, leaving the user with only the progress message. The durable behavior needs an explicit completion signal without advertising that signal to ordinary message-tool consumers that do not implement it.

Implementation

  • Exposes final: boolean only in the Codex projected message schema when sourceReplyDeliveryMode === "message_tool_only"; automatic delivery and ordinary runtimes do not see it.
  • Keeps confirmed progress sends non-terminal. A turn terminates only when Codex passes final: true and delivery to the current source conversation is confirmed.
  • Records explicit progress/final evidence in messaging telemetry. Payload construction, the initial reply runner, stranded-reply recovery, and queued follow-ups now distinguish “some source message was delivered” from “the completed source reply was delivered.”
  • Surfaces a terminal failure after progress instead of suppressing it, while keeping the model's private fallback text private.
  • Preserves legacy behavior when a runtime emits no progress/final marker. An explicit final:false takes precedence over legacy tool-owned termination; legacy callers without the new field remain compatible.
  • Strips final before provider dispatch and message idempotency calculation. Schema projection clones preserve WeakMap-backed tool ownership metadata.
  • Adds Codex-specific prompt guidance: progress explicitly uses final:false, and only the completed source reply uses final:true.

Evidence

  • Regression coverage exercises progress -> continued work, explicit final release, progress -> terminal model failure, progress -> stranded recovery, queued follow-up failure, internal source-reply evidence, external-target non-termination, schema scoping, legacy compatibility, and idempotency stripping.
  • node scripts/run-vitest.mjs src/auto-reply/reply/followup-runner.test.ts src/auto-reply/reply/agent-runner.misc.runreplyagent.test.ts -- --reporter=dot — 224/224 passed.
  • Focused payload/evidence suites — 125/125 passed.
  • Codex dynamic-tool suite — 98/98 passed after the final compatibility adjustment.
  • pnpm check:test-types — passed for core, extensions, and root test projects.
  • .agents/skills/autoreview/scripts/autoreview --mode uncommitted --stream-engine-output — clean; no accepted/actionable findings, overall confidence 0.86.
  • AI-assisted. No private message content, chat identifiers, or credentials are included.

Relationship to open work

#105290 established the right runtime direction, but its review correctly found that it advertised final in the shared message schema where no other runtime implements it. This is a fresh current-main successor with that scope repaired and with progress-versus-completion telemetry propagated through recovery paths.

#100156 remains the prompt-only alternative. This PR is intentionally independent: maintainers can choose this explicit-final contract, or choose the prompt-only contract instead.

This PR does not depend on #104632. They are complementary: #104632 reconciles one ambiguous timed-out send with the same idempotency key, while this PR decides whether a confirmed source delivery is progress or the terminal reply.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling extensions: codex triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. size: S labels Jul 12, 2026
@Glucksberg
Glucksberg force-pushed the fix/codex-explicit-final-delivery branch from db3348f to 7d54c41 Compare July 12, 2026 12:35
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 12, 2026
@clawsweeper

clawsweeper Bot commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 12, 2026, 11:41 PM ET / July 13, 2026, 03:41 UTC.

Summary
The PR adds a Codex-only final message parameter and propagates progress-versus-completion delivery evidence through turn termination, payload suppression, failure routing, and reply recovery.

PR surface: Source +159, Tests +343. Total +502 across 24 files.

Reproducibility: yes. at source level: the canonical issue records a successful current-source progress send without final followed by immediate terminal release, and this branch's absent-marker fallback preserves that path. A fresh live reproduction was not run in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Model-facing completion controls: 1 optional Codex-only parameter added. Optionality determines whether the original omitted-marker progress failure is actually prevented.
  • Plugin SDK surface: 1 generic export added. The cloning helper becomes a compatibility-sensitive SDK seam to support a Codex-specific projection.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #100153
Summary: The canonical issue tracks progress messages ending Codex message-tool-only turns; three open PRs propose competing fixes, while the timeout-reconciliation PR is adjacent but distinct.

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] Remove the absent-marker terminal fallback or make the Codex-scoped final parameter required with defined omission behavior.
  • [P1] Add redacted exact-head Telegram or equivalent source-channel proof covering progress, continuation, final delivery, and terminal release.
  • Obtain owner confirmation for the permanent completion contract and the new plugin SDK export.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Head 53ffd14 lacks inspected after-fix live evidence showing final=false progress, continued tool work, final=true delivery, and terminal release; add a redacted recording or runtime logs, update the PR body to trigger review, or ask a maintainer for @clawsweeper re-review.

Mantis proof suggestion
A native Telegram capture can directly show that progress remains visible without terminating the turn and that only the explicit final reply releases it. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis telegram desktop proof: verify that `message(final=false)` delivers visible progress, the same turn continues through additional tool work, and `message(final=true)` delivers the final reply and only then releases the turn.

Risk before merge

  • [P1] Keeping absent-marker compatibility means a Codex progress send that omits final can still terminate the turn and suppress the requested result.
  • [P2] Removing the absent-marker fallback or requiring final changes a model-facing completion contract and could strand callers that do not emit the new marker unless the scope and failure behavior are explicit.
  • [P1] The patch adds a generic plugin SDK export to support Codex schema cloning, creating a public compatibility surface that needs owner confirmation if no existing SDK seam suffices.
  • [P1] No exact-head real setup proves progress, continuation, final delivery, terminal failure routing, and stranded recovery together.
  • [P1] Direct upstream Codex protocol/runtime inspection was blocked by the review environment, so dependency-contract compatibility is not proven.

Maintainer options:

  1. Make completion intent unambiguous (recommended)
    Require the Codex-scoped marker, define omission as non-terminal or an explicit error, add compatibility-focused tests, and provide exact-head live proof before merge.
  2. Adopt the narrower proven alternative
    Select the prompt-only approach in fix(codex): keep message-tool replies final-only #100156 and pause or close this competing API-contract branch.
  3. Accept optional-marker recurrence risk
    Merge with the legacy fallback only if maintainers intentionally accept that omitted markers can still end progress turns.

Next step before merge

  • [P1] A maintainer must choose the permanent Codex completion/API contract and request exact-head live proof; the remaining blocker is not a safe autonomous repair decision.

Maintainer decision needed

  • Question: Should Codex message-tool-only completion use a mandatory explicit final contract, or should OpenClaw keep the existing terminal contract and rely on the narrower prompt-only fix?
  • Rationale: Making final mandatory is structurally robust but changes the Codex model-facing tool contract; preserving omitted-marker compatibility leaves the reported failure possible, and the open alternatives embody different permanent policies.
  • Likely owner: omarshahine — Their merged work owns the current terminal source-reply classifier and provides the strongest available history signal for selecting its successor contract.
  • Options:
    • Require explicit final (recommended): Make the Codex-only schema require final, keep progress non-terminal, define absent-marker failure behavior, and require exact-head live proof.
    • Use prompt-only steering: Prefer the proof-positive narrower approach in fix(codex): keep message-tool replies final-only #100156 and close the competing runtime-contract branches after selection.
    • Keep optional compatibility: Retain absent-marker terminal behavior and explicitly accept that prompt noncompliance can reproduce the early-release bug.

Security
Cleared: No concrete security or supply-chain regression was found; the patch changes message completion semantics and an SDK export without changing dependencies, workflow permissions, secrets, package resolution, or downloaded code.

Review findings

  • [P1] Make omitted final non-terminal in Codex mode — extensions/codex/src/app-server/dynamic-tools.ts:701
Review details

Best possible solution:

Choose one canonical completion contract; if the explicit control is retained, make final mandatory for Codex message-tool-only source sends, treat absence as non-terminal or a clear tool error, avoid unnecessary plugin SDK expansion, and prove the exact head in a real source-channel turn.

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

Yes at source level: the canonical issue records a successful current-source progress send without final followed by immediate terminal release, and this branch's absent-marker fallback preserves that path. A fresh live reproduction was not run in this read-only review.

Is this the best way to solve the issue?

No, not as currently authored: an optional marker plus legacy terminal fallback is only a prompt-assisted mitigation, not a durable explicit-completion contract. The best fix must either require the marker or select the narrower prompt-only policy explicitly.

Full review comments:

  • [P1] Make omitted final non-terminal in Codex mode — extensions/codex/src/app-server/dynamic-tools.ts:701
    The original failing progress call omitted final, but this fallback still treats a tool-confirmed source reply as final whenever the marker is absent. Because the projected schema also leaves the field optional, the same progress send can still release the turn; require the field or treat omission as non-terminal/a clear tool error so the runtime fix does not depend on prompt compliance.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P1: The affected source-channel workflow can visibly stop after a progress acknowledgement and omit the requested answer.
  • merge-risk: 🚨 compatibility: The branch introduces a new Codex completion parameter and a plugin SDK export while retaining ambiguous legacy omission behavior.
  • merge-risk: 🚨 message-delivery: Incorrect marker handling can either terminate after progress or fail to release after the completed source reply.
  • 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: Head 53ffd14 lacks inspected after-fix live evidence showing final=false progress, continued tool work, final=true delivery, and terminal release; add a redacted recording or runtime logs, update the PR body to trigger review, or ask a maintainer for @clawsweeper re-review.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The change directly controls visible Telegram progress and final replies and can be demonstrated clearly in a short Telegram Desktop recording.
Evidence reviewed

PR surface:

Source +159, Tests +343. Total +502 across 24 files.

View PR surface stats
Area Files Added Removed Net
Source 12 197 38 +159
Tests 12 366 23 +343
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 24 563 61 +502

What I checked:

Likely related people:

  • omarshahine: Merged pull request Fix Codex message-tool-only source reply completion #95942 introduced the current Codex message-tool-only terminal source-reply recognition that this branch refines. (role: introduced related behavior; confidence: high; commits: 9b9a124cc520; files: extensions/codex/src/app-server/dynamic-tools.ts, src/agents/embedded-agent-message-tool-source-reply.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 (7 earlier review cycles)
  • reviewed 2026-07-12T12:39:37.591Z sha 7d54c41 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-12T12:49:11.135Z sha 7d54c41 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-12T13:10:23.539Z sha 3c34fe3 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-12T20:46:33.000Z sha 35fcd9d :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-12T20:54:36.870Z sha 35fcd9d :: needs real behavior proof before merge. :: [P1] Require final:false for progress messages
  • reviewed 2026-07-12T21:13:28.997Z sha 1c18b9e :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-12T21:53:05.418Z sha 1c18b9e :: needs real behavior proof before merge. :: none

@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 12, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 12, 2026
Closed
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 12, 2026
@Glucksberg
Glucksberg force-pushed the fix/codex-explicit-final-delivery branch from 7d54c41 to 3c34fe3 Compare July 12, 2026 12:54
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. and removed mantis: telegram-visible-proof Mantis should capture Telegram visible proof. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 12, 2026
@Glucksberg

Glucksberg commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot mentioned this pull request Jul 12, 2026
Merged
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 12, 2026
@Glucksberg
Glucksberg force-pushed the fix/codex-explicit-final-delivery branch from 35fcd9d to 1c18b9e Compare July 12, 2026 21:00
@Glucksberg

Glucksberg commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@Glucksberg
fix(codex): require explicit final source delivery
53ffd14 
Distinguish progress from terminal source delivery across Codex telemetry,
payload suppression, terminal failure routing, and stranded-reply recovery.
Preserve legacy behavior when explicit markers are absent.
@Glucksberg
Glucksberg force-pushed the fix/codex-explicit-final-delivery branch from 1c18b9e to 53ffd14 Compare July 13, 2026 02:54
Glucksberg added a commit to Glucksberg/OpenClaw that referenced this pull request Jul 13, 2026
@Glucksberg
Merge PR openclaw#105365: require explicit final source delivery
fe2567a
@obviyus

obviyus commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Maintainer follow-up pushed: 4e0f0af3ff9 (review findings from pre-land review).

  1. Omitted final no longer misclassifies as progress. The bridge previously recorded sourceReplyFinal: false and skipped termination when the model omitted the optional final on an evidence-confirmed source reply — downstream treated the delivered reply as incomplete, and stranded-reply recovery could re-deliver a duplicate. Now omitted final degrades to main's legacy terminate-on-delivery semantics with a completed marker; only an explicit final: false defers termination. Restored real omission coverage in dynamic-tools.test.ts alongside the explicit true/false cases.
  2. Plugin-SDK surface gate fixed without growing the SDK. cloneAgentRuntimeToolWithParameters (added by this PR) is gone; the Codex-only final property is projected in place on the attempt-fresh message tool (createOpenClawCodingTools builds fresh instances per buildDynamicTools call — verified through src/agents/agent-tools.ts → src/agents/openclaw-tools.ts), preserving WeakMap ownership metadata with object identity. src/plugin-sdk/agent-harness-runtime.ts and src/agents/runtime-plan/tools.ts are now byte-identical to main.

Validation on the new head: codex app-server suites (157/157), delivery-evidence + plugin-SDK surface report (10/10), plus the full focused matrix and pnpm tsgo:core|extensions|test:src|test:extensions — all exit 0. Waiting on hosted CI before merge.

@github-actions github-actions Bot mentioned this pull request Jul 13, 2026
Open
@obviyus
fix(codex): preserve legacy termination when final is omitted
e7b4035 
Omitted final on a confirmed message-tool-only source reply keeps main's terminate-on-delivery semantics and records a completed marker; only explicit final=false defers termination. Project the Codex-only final schema property in place on attempt-fresh tools instead of adding a public plugin-SDK clone export, resolving the SDK surface budget failure.
@obviyus
obviyus force-pushed the fix/codex-explicit-final-delivery branch from 4e0f0af to e7b4035 Compare July 13, 2026 04:19
@obviyus
obviyus merged commit 4dc6e18 into openclaw:main Jul 13, 2026
105 of 109 checks passed
This was referenced Jul 13, 2026
Closed
Closed
This was referenced Jul 17, 2026
Open
Open
Open
@cgluttrell cgluttrell mentioned this pull request Jul 23, 2026
Merged
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

agents Agent runtime and tooling extensions: codex mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: L status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Glucksberg @obviyus