fix(agents): replace prose terminal classifiers#93228
Conversation
Co-authored-by: fuller-stack-dev <263060202+fuller-stack-dev@users.noreply.github.com>
|
Land-ready verification for
No known proof gaps. |
|
Codex review: needs changes before merge. Reviewed June 15, 2026, 5:51 AM ET / 09:51 UTC. Summary Reproducibility: yes. Source inspection shows current main lacks the structured terminal/replay path, and the PR head still has a concrete Codex dynamic-tool ordering path where concurrent server requests can finish out of order without ordinals. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Pass model-order tool outcome ordinals through the Codex dynamic-tool path, add a focused out-of-order terminal-presentation test, then land only after maintainers accept the experimental harness SDK surface. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main lacks the structured terminal/replay path, and the PR head still has a concrete Codex dynamic-tool ordering path where concurrent server requests can finish out of order without ordinals. Is this the best way to solve the issue? No, not yet. The structured protocol-state direction is the right layer, but the Codex dynamic-tool path needs ordinal propagation and maintainer acceptance of the SDK surface before this is the best merge shape. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 773ffd87a1e1. Label changesLabel changes:
Label justifications:
Evidence reviewedAcceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baeb8d3eca
ℹ️ 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".
| recordToolPrepStage: (name) => { | ||
| toolBuildStages.mark(name); | ||
| }, | ||
| onToolOutcome: params.onToolOutcome, |
There was a problem hiding this comment.
Pass Codex tool outcome ordinals
In Codex app-server turns that issue more than one dynamic tool call concurrently (the client dispatches server requests with void this.handleServerRequest(...)), this newly enabled onToolOutcome path records terminal presentations without the allocateToolOutcomeOrdinal that the normal embedded path passes. observeToolOutcome then falls back to completion order, so a slower earlier tool can overwrite/appear after a later tool and the incomplete-turn terminal summary can cite the wrong tool. Please pass params.allocateToolOutcomeOrdinal through here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c83b4e543
ℹ️ 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".
| return; | ||
| } | ||
| this.terminalPresentationClearedItemIds.add(item.id); | ||
| this.params.onToolOutcome?.({ |
There was a problem hiding this comment.
Preserve ordering when clearing terminal presentations
For Codex turns where a native tool item only appears in the final turn snapshot and an OpenClaw dynamic tool later recorded a terminal presentation, this unordered onToolOutcome clear runs after the dynamic outcome and has no toolCallOrdinal; observeToolOutcome then treats it as the newest observation and erases the later trusted terminal summary, so fallback-exhausted incomplete turns show the generic error instead of the tool-authored summary. Fresh evidence beyond the existing dynamic-tool ordinal comment is this newly added native snapshot clearing path.
Useful? React with 👍 / 👎.
* fix(agents): replace prose terminal classifiers Co-authored-by: fuller-stack-dev <263060202+fuller-stack-dev@users.noreply.github.com> * fix(agents): preserve terminal failure lifecycles * fix(agents): order parallel terminal summaries * fix(agents): preserve structured post-tool silence * fix(agents): preserve structured replay provenance --------- Co-authored-by: fuller-stack-dev <263060202+fuller-stack-dev@users.noreply.github.com>
Commit 0314819 (openclaw#93228) refactored the Codex dynamic-tool bridge to consume before_tool_call adjusted params into executedArgs and then fed that merged view to BOTH the native agentToolResultMiddleware runner and the legacy codex app-server extension runner. The native middleware runner is meant to observe OpenClaw's executed args (merged adjustments), matching the after_tool_call hook view — that part is correct and covered by dynamic-tools.test.ts. But the legacy codex app-server extension runner has a different historical contract: it observes the tool's prepared call args, before before_tool_call rewrites them (openclaw-owned-tool-runtime-contract.test.ts). With the merged view the contract extension's args assertion threw, the runner swallowed the error, and the extension's compacted result was silently discarded — so after_tool_call and the bridge return observed the raw result. Restore the legacy extension runner's prepared-args view; the native middleware runner keeps executedArgs. executedArgs (merged adjustments) remains reserved for telemetry, messaging, replay-safety, and after_tool_call. Note: this regression reproduces on pristine upstream/main HEAD (32d1ccd), not only on the merge — confirmed by running the contract test in an upstream worktree. Verified: full vitest.extension-codex-app-server-tools shard, 307/307. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Commit 0314819 (openclaw#93228) refactored the Codex dynamic-tool bridge to consume before_tool_call adjusted params into executedArgs and then fed that merged view to BOTH the native agentToolResultMiddleware runner and the legacy codex app-server extension runner. The native middleware runner is meant to observe OpenClaw's executed args (merged adjustments), matching the after_tool_call hook view — that part is correct and covered by dynamic-tools.test.ts. But the legacy codex app-server extension runner has a different historical contract: it observes the tool's prepared call args, before before_tool_call rewrites them (openclaw-owned-tool-runtime-contract.test.ts). With the merged view the contract extension's args assertion threw, the runner swallowed the error, and the extension's compacted result was silently discarded — so after_tool_call and the bridge return observed the raw result. Restore the legacy extension runner's prepared-args view; the native middleware runner keeps executedArgs. executedArgs (merged adjustments) remains reserved for telemetry, messaging, replay-safety, and after_tool_call. Note: this regression reproduces on pristine upstream/main HEAD (32d1ccd), not only on the merge — confirmed by running the contract test in an upstream worktree. Verified: full vitest.extension-codex-app-server-tools shard, 307/307. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(agents): replace prose terminal classifiers Co-authored-by: fuller-stack-dev <263060202+fuller-stack-dev@users.noreply.github.com> * fix(agents): preserve terminal failure lifecycles * fix(agents): order parallel terminal summaries * fix(agents): preserve structured post-tool silence * fix(agents): preserve structured replay provenance --------- Co-authored-by: fuller-stack-dev <263060202+fuller-stack-dev@users.noreply.github.com>
* fix(agents): replace prose terminal classifiers Co-authored-by: fuller-stack-dev <263060202+fuller-stack-dev@users.noreply.github.com> * fix(agents): preserve terminal failure lifecycles * fix(agents): order parallel terminal summaries * fix(agents): preserve structured post-tool silence * fix(agents): preserve structured replay provenance --------- Co-authored-by: fuller-stack-dev <263060202+fuller-stack-dev@users.noreply.github.com>
Summary
Release note: prevents silent agent turns without parsing natural-language progress text and avoids replaying side effects during model fallback.
Verification
pnpm tsgo:corepnpm check:test-typespnpm buildcheck:changed:run_2a378f6aa965(proof)Source Study