Skip to content

fix(agents): preserve completed post-tool replies#100655

Merged
steipete merged 2 commits into
mainfrom
codex/pr-94637-takeover
Jul 6, 2026
Merged

fix(agents): preserve completed post-tool replies#100655
steipete merged 2 commits into
mainfrom
codex/pr-94637-takeover

Conversation

@steipete

@steipete steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Related: #94637
Closes #80918

What Problem This Solves

Fixes an issue where an agent turn that completed after tool execution could return a stale pre-tool assistant snapshot, replacing the completed post-tool answer with an incomplete-turn error.

Why This Change Was Made

The runner now chooses one canonical terminal assistant for the current attempt, falling back to the model-matched session assistant only when the attempt has none. Incomplete-turn classification, failover, result metadata, payloads, and stop reasons all consume that same value so they cannot disagree.

This is a maintainer takeover of #94637 because the contributor branch does not permit maintainer edits. Contributor credit is preserved with Co-authored-by and the changelog thanks @LiuwqGit.

User Impact

Completed answers after tool calls are delivered as successful final replies instead of being replaced by stale incomplete-tool-use errors.

Evidence

  • Added a focused regression covering a stale pre-tool session snapshot followed by a completed current-attempt assistant.
  • Fresh full-branch autoreview: clean; no accepted or actionable findings.
  • Rebased on current main after the prior fork head's unrelated stale-base Control UI build failure.
  • Exact-head hosted CI/Testbox will gate the replacement before merge.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S maintainer Maintainer-authored PR labels Jul 6, 2026
@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 6, 2026, 3:07 AM ET / 07:07 UTC.

Summary
The branch changes the embedded agent runner to prefer a current-attempt assistant for terminal classification, failover and success metadata, adds regression tests, and adds a changelog entry.

PR surface: Source -11, Tests +122, Docs +1. Total +112 across 5 files.

Reproducibility: yes. for the review blockers at source level: current main derives both attempt assistant selectors from the same messagesSnapshot, and the PR head's payloadCount guard can skip incomplete-turn handling for pre-tool text. I did not run live channel proof in this read-only review.

Review metrics: 1 noteworthy metric.

  • Release-owned changelog edit: 1 added. The repository reserves CHANGELOG.md for release generation, so this normal bugfix PR should keep release-note context in the PR body instead.

Stored data model
Persistent data-model change detected: vector/embedding metadata: CHANGELOG.md. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #80918
Summary: This PR is a candidate fix for the canonical stale post-tool final-reply issue, but current evidence shows the branch still needs production-valid proof and a terminal-guard repair before it can safely close that 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: 🌊 off-meta tidepool
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

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

Rank-up moves:

  • Replace the fabricated divergent attempt fixture with a producer-level regression or redacted real attempt capture.
  • Preserve the pre-tool toolUse guard so pre-tool payloads cannot mask empty post-tool stops.
  • Remove the normal PR edit to CHANGELOG.md.

Risk before merge

  • [P1] Merging this as-is could close the linked message-loss issue while leaving the real attempt-finalization or session-persistence boundary unfixed.
  • [P1] The classifier change can let pre-tool commentary mask an empty post-tool terminal assistant turn, causing a partial pre-tool reply to be treated as success.
  • [P1] The branch edits release-owned CHANGELOG.md; that should be removed before merge and kept as PR-body release-note context.

Maintainer options:

  1. Rework against the terminal producer (recommended)
    Require a producer-level regression or redacted real attempt capture, then keep the current-attempt selector only where it matches a producible state and preserves the [Bug] Agent tool-chain final text segments silently dropped — invisible to user and next-turn context #76477 safety guard.
  2. Pause or close this takeover
    If the production state cannot be shown, close this branch and leave the linked issue as the canonical place to collect a real attempt snapshot.
  3. Accept as intentional hardening
    Maintainers can accept a narrowed defensive cleanup, but should explicitly keep the linked message-loss issue open for the actual finalization/session race.

Next step before merge

  • [P1] The protected maintainer-labeled PR has blocking correctness findings and needs a maintainer rework-or-close decision; automation cannot supply the missing real attempt capture or proof decision.

Maintainer decision needed

  • Question: Should this maintainer-labeled takeover be reworked to prove and fix a production-valid attempt-finalization/session-snapshot failure, or closed while the linked issue waits for a redacted capture?
  • Rationale: The remaining blocker is not a normal style or test gap: the patch changes terminal message delivery while its main regression still bypasses the producer boundary, so maintainer intent is needed before accepting, reworking, or retiring the takeover.
  • Likely owner: steipete — He has recent history in the classifier path and authored the prior decision explaining why this exact proof shape was insufficient.
  • Options:
    • Require production-valid rework (recommended): Block merge until the branch proves the real returned attempt/session divergence with a redacted capture or producer-level regression and fixes that owner boundary while preserving the tool-use guard.
    • Close the takeover: Close this PR and keep the linked issue open for the requested current reproduction data if maintainers agree this branch is not salvageable as the fix.
    • Accept defensive cleanup only: Maintainers may intentionally merge a narrowed selector cleanup, but it should not close the linked message-loss issue without separate proof of the production boundary.

Security
Cleared: No concrete security or supply-chain concern was found; the diff does not change dependencies, secrets handling, CI permissions, release scripts, or downloaded code.

Review findings

  • [P1] Exercise a producible terminal-assistant state — src/agents/embedded-agent-runner/run.incomplete-turn.test.ts:1749-1764
  • [P2] Keep pre-tool text from masking empty post-tool stops — src/agents/embedded-agent-runner/run/incomplete-turn.ts:253
Review details

Best possible solution:

Rework the branch around a production-valid attempt/session capture or producer-level regression, preserve the pre-tool toolUse safety guard, and keep release-note context out of CHANGELOG.md.

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

Yes for the review blockers at source level: current main derives both attempt assistant selectors from the same messagesSnapshot, and the PR head's payloadCount guard can skip incomplete-turn handling for pre-tool text. I did not run live channel proof in this read-only review.

Is this the best way to solve the issue?

No. The current selector cleanup is only a plausible defensive change; the best fix needs a production-valid attempt/session proof and must preserve the pre-tool toolUse safety contract.

Full review comments:

  • [P1] Exercise a producible terminal-assistant state — src/agents/embedded-agent-runner/run.incomplete-turn.test.ts:1749-1764
    The previously raised blocker remains: this test still mocks lastAssistant=toolUse while currentAttemptAssistant=stop, but current main computes both from the same messagesSnapshot, with the current assistant selected from that array's suffix. When a current-attempt assistant exists, the global last assistant cannot be an older pre-tool message, so this fixture can pass while the real finalization/session race remains unfixed.
    Confidence: 0.88
  • [P2] Keep pre-tool text from masking empty post-tool stops — src/agents/embedded-agent-runner/run/incomplete-turn.ts:253
    Line 253 now returns null whenever earlier assistantTexts produced any payload and the selected current assistant is not toolUse or length. A producible post-tool empty stop turn can therefore deliver the earlier pre-tool commentary as success instead of retrying or surfacing the incomplete-turn error, regressing the safety contract from [Bug] Agent tool-chain final text segments silently dropped — invisible to user and next-turn context #76477. Late discovery: this helper was unchanged since the earlier reviewed head.
    Confidence: 0.86
    Late finding: first raised on code an earlier review cycle already covered.

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 e66f43e27616.

Label changes

Label justifications:

  • P2: The PR targets a bounded embedded-agent message-delivery bug, but the remaining work is normal maintainer review and rework rather than an emergency runtime outage.
  • merge-risk: 🚨 message-delivery: The changed terminal classifier and metadata path decide whether final assistant text is delivered, retried, or replaced by an incomplete-turn warning.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Not applicable for the external-contributor proof gate because this is a maintainer-labeled takeover; the remaining blocker is whether the source proof represents a production-valid state.
Evidence reviewed

PR surface:

Source -11, Tests +122, Docs +1. Total +112 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 2 40 51 -11
Tests 2 144 22 +122
Docs 1 1 0 +1
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 185 73 +112

What I checked:

Likely related people:

  • steipete: Recent GitHub path history shows steipete authored terminal-classifier simplification in the same helper, and he posted the predecessor close rationale for this exact production-boundary concern. (role: recent classifier contributor and likely decision owner; confidence: high; commits: 0314819f918a, 62ecc2306c90; files: src/agents/embedded-agent-runner/run/incomplete-turn.ts, src/agents/embedded-agent-runner/run.incomplete-turn.test.ts, src/agents/embedded-agent-runner/run.ts)
  • fuller-stack-dev: Recent history credits fuller-stack-dev on post-tool empty final-turn retry work that changed the same incomplete-turn and terminal-delivery boundary. (role: adjacent runtime contributor; confidence: medium; commits: 77a682c5def2, 414ecd2b9689; files: src/agents/embedded-agent-runner/run/incomplete-turn.ts, src/agents/embedded-agent-runner/run.incomplete-turn.test.ts, src/agents/embedded-agent-runner/run.ts)
  • vincentkoc: Recent path history shows post-tool empty reply recovery and embedded runner work near this terminal-classification area. (role: recent area contributor; confidence: medium; commits: 67dc8053149d, 7dfc499299ff; files: src/agents/embedded-agent-runner/run/incomplete-turn.ts, src/agents/embedded-agent-runner/run.incomplete-turn.test.ts, src/agents/embedded-agent-runner/run.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-07-06T06:32:18.194Z sha 4a8a648 :: needs real behavior proof before merge. :: [P1] Exercise a producible terminal-assistant state

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jul 6, 2026
@steipete
steipete force-pushed the codex/pr-94637-takeover branch from 4a8a648 to 58c6f0e Compare July 6, 2026 06:36

@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: aafa998f4d

ℹ️ 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".


if (
(params.payloadCount !== 0 && !toolUseTerminal && !lengthTerminal && !thinkingOnlyTerminal) ||
(params.payloadCount !== 0 && !incompleteTerminalAssistant && !thinkingOnlyTerminal) ||

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 Keep pre-tool text from masking empty post-tool stops

When the session assistant is the stale pre-tool toolUse turn but currentAttemptAssistant is a blank/empty stop, buildEmbeddedRunPayloads can still produce a payload from earlier assistantTexts (the pre-tool commentary). This payloadCount !== 0 guard then returns null, so a turn that never produced a post-tool answer is treated as a successful final reply instead of retrying or surfacing the incomplete-turn error; that regresses the #76477 guard for the stale-session shape this change introduces. Please keep the tool-use guard unless the current assistant has visible terminal text, or base this check on terminal-assistant output rather than any attempt payload.

Useful? React with 👍 / 👎.

@steipete
steipete force-pushed the codex/pr-94637-takeover branch 2 times, most recently from 8f8edee to a017680 Compare July 6, 2026 07:21
@steipete
steipete force-pushed the codex/pr-94637-takeover branch from a017680 to 9fb6ac2 Compare July 6, 2026 07:38
@steipete
steipete merged commit c1cbc15 into main Jul 6, 2026
44 checks passed
@steipete
steipete deleted the codex/pr-94637-takeover branch July 6, 2026 07:38
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Merged via squash.

@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: 9fb6ac2633

ℹ️ 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 CHANGELOG.md
- **Android network recovery:** reconnect Gateway sessions immediately when Android regains a validated network instead of waiting for the current reconnect backoff. (#100347) Thanks @ly85206559.
- **Android Voice layout:** keep Voice settings controls within their intended width so nested cards do not overflow or clip on constrained screens. (#100491) Thanks @IWhatsskill.
- **Android camera logging:** remove release-path camera clip diagnostics that exposed temporary file details and added noisy invoke logs. (#99484) Thanks @NianJiuZst.
- **Agent final replies:** prefer the current attempt's terminal assistant across incomplete-turn classification and success metadata so a stale pre-tool snapshot cannot replace a completed post-tool answer with an error. (#94637) Thanks @LiuwqGit.

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 Remove the normal-PR changelog entry

Root AGENTS.md says CHANGELOG.md is release-only and normal PR/direct fixes should carry release-note context in the PR body, squash message, or commit instead; this maintainer-takeover fix adds an Unreleased entry for #94637 directly. Please move this note out of CHANGELOG.md so release generation remains the single owner of changelog edits.

Useful? React with 👍 / 👎.

github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 6, 2026
* fix(agents): prefer current attempt terminal state

Co-authored-by: weiqinl <[email protected]>

* test(agents): model current attempt terminal state

---------

Co-authored-by: weiqinl <[email protected]>
giodl73-repo pushed a commit to giodl73-repo/openclaw that referenced this pull request Jul 8, 2026
* fix(agents): prefer current attempt terminal state

Co-authored-by: weiqinl <[email protected]>

* test(agents): model current attempt terminal state

---------

Co-authored-by: weiqinl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling maintainer Maintainer-authored PR merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Silent send miss: incomplete-turn classifier discards stopReason=stop final after update_plan

1 participant