Skip to content

fix(tasks): accept completion summaries with result markers#90864

Open
moeedahmed wants to merge 3 commits into
openclaw:mainfrom
moeedahmed:fix/progress-only-completion-results
Open

fix(tasks): accept completion summaries with result markers#90864
moeedahmed wants to merge 3 commits into
openclaw:mainfrom
moeedahmed:fix/progress-only-completion-results

Conversation

@moeedahmed

@moeedahmed moeedahmed commented Jun 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Make the required-completion classifier accept summaries that begin with progress narration but contain a real result/report/verification marker later in the text.

This keeps genuine progress-only messages blocked, while avoiding false Required completion ended with progress-only text outcomes for completed worker reports such as:

  • I'll verify the fix. Verification: all 62 tests passed...
  • I'll start running the suite. Result: 3 files changed...
  • Investigating ... I patched the handler, tests passed

Why

A worker can produce a usable final report, but if the first words look like progress narration the current classifier can still mark it as progress-only. That leaves otherwise completed background work blocked or invisible to the requester.

This PR is deliberately scoped to the classifier. The separate completion_handoff_pending delivery issue already has upstream PR coverage in #90740 / #90144, so this PR avoids duplicating that pending-handoff work.

Changes

  • Add a strong result-marker guard to isProgressOnlyCompletionText().
  • Add focused unit coverage for:
    • progress-only messages that must still block
    • narration-prefixed messages with result/verification markers that must pass
    • terminal-result handling for empty, progress-only, and accepted summaries

Real behavior proof

  • Behavior or issue addressed: OpenClaw's live task completion classifier was treating real worker completion summaries as progress-only when the summary started with narration but later included result/report/verification markers. That could leave completed background work blocked instead of delivered.
  • Real environment tested: Moeed's live macOS OpenClaw setup running OpenClaw 2026.6.1 after the runtime containment was loaded by a safe gateway restart.
  • Exact steps or command run after this patch: Applied the local OpenClaw runtime containment, ran the live classifier smoke with node classifier-test.mjs, performed the OpenClaw safe gateway restart, then ran a managed Claude ACP post-restart smoke.
  • Evidence after fix: Redacted terminal/live output from the real OpenClaw setup:
classifier-test.mjs => 10/10 pass on live
5 regression progress-only cases still blocked
5 narration-plus-result cases accepted
safe gateway restart completed
managed Claude ACP post-restart smoke returned ACP_DELIVERY_SMOKE_OK

Verification

  • pnpm exec oxfmt --check src/tasks/task-completion-contract.ts src/tasks/task-completion-contract.test.ts
  • pnpm exec oxlint src/tasks/task-completion-contract.ts src/tasks/task-completion-contract.test.ts
  • pnpm test src/tasks/task-completion-contract.test.ts

All passed locally on this PR branch.

@openclaw-barnacle openclaw-barnacle Bot added size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 6, 2026
@moeedahmed
moeedahmed marked this pull request as ready for review June 6, 2026 06:24
@clawsweeper

clawsweeper Bot commented Jun 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed July 8, 2026, 5:44 AM ET / 09:44 UTC.

Summary
The PR adds a completion-result marker guard to required task-completion progress-only classification and expands task-completion contract tests.

PR surface: Source +17, Tests +70. Total +87 across 2 files.

Reproducibility: yes. at source level. The PR regex matches planning-only strings before the existing planning blocker runs, and both ACP and subagent callers persist the classifier result into terminal task state.

Review metrics: 1 noteworthy metric.

  • Classifier result guard: 1 added. This guard decides whether required ACP and subagent completions are blocked or recorded successful, so false positives affect durable task state.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦞 diamond lobster
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:

  • [P2] Restrict marker matching to explicit final-result contexts and add future/conditional planning counterexamples.
  • Test the public terminal-result API or document that exporting the classifier helper is an intentional API change.

Risk before merge

  • [P1] The marker regex can mis-record future or conditional planning text as a successful required completion, which affects durable task/session terminal state.
  • [P1] The direct test import expands a helper that current main keeps private, creating avoidable internal API surface unless maintainers intentionally want that contract.

Maintainer options:

  1. Tighten classifier before merge (recommended)
    Restrict result-marker acceptance to explicit final-result contexts, keep future and conditional planning blocked, and add counterexample tests for the planning strings that currently match.
  2. Pause for classifier ownership
    If maintainers want a broader semantic contract for final-result markers, pause this PR until task/session-state owners choose that boundary.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Restrict completion marker matching to explicit final-result contexts, keep future and conditional planning blocked, avoid the test-only helper export or justify it as intentional API, add counterexample tests for planning text such as `Once done, I will report back` and `I will check whether tests passed before continuing`, then run focused format, lint, and task-completion tests.

Next step before merge

  • The remaining blockers are narrow classifier and test-shape repairs that an automated worker can attempt without choosing a new product direction.

Security
Cleared: The diff only touches task classifier TypeScript and colocated tests; no dependency, workflow, credential, package, or supply-chain surface changes were found.

Review findings

  • [P1] Keep planning-only summaries blocked — src/tasks/task-completion-contract.ts:28
  • [P2] Avoid a test-only classifier helper export — src/tasks/task-completion-contract.test.ts:3-5
Review details

Best possible solution:

Land a classifier-only fix that accepts explicit final result, report, or verification evidence while keeping future and conditional planning blocked, with tests through the public terminal-result contract unless maintainers intentionally expose the helper.

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

Yes, at source level. The PR regex matches planning-only strings before the existing planning blocker runs, and both ACP and subagent callers persist the classifier result into terminal task state.

Is this the best way to solve the issue?

No as submitted. The classifier layer is the right place, but marker acceptance needs to be narrower and tests should use the public terminal-result API unless the helper export is intentional.

Full review comments:

  • [P1] Keep planning-only summaries blocked — src/tasks/task-completion-contract.ts:28
    This remains unfixed from the prior review. The marker regex runs before the progress-only checks and treats broad result words anywhere in the text as final evidence, so planning-only summaries like Once done, I will report back or conditional text like I will check whether tests passed before continuing can be accepted as successful required completions.
    Confidence: 0.92
  • [P2] Avoid a test-only classifier helper export — src/tasks/task-completion-contract.test.ts:3-5
    This remains unfixed from the prior review. The new tests import isProgressOnlyCompletionText directly, but current main keeps that helper private; test through resolveRequiredCompletionTerminalResult unless maintainers intentionally want this classifier helper exposed as an internal API.
    Confidence: 0.82

Overall correctness: patch is incorrect
Overall confidence: 0.92

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: The PR targets a real task-completion classifier bug with limited surface area, but it still needs focused correctness repair before merge.
  • merge-risk: 🚨 session-state: The diff changes terminal task-completion classification and can mis-record planning-only summaries as successful completion state.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦞 diamond lobster and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body supplies redacted terminal/live output from a macOS OpenClaw setup showing classifier smoke results, progress-only cases blocked, narration-plus-result cases accepted, and an ACP post-restart smoke after the patch.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies redacted terminal/live output from a macOS OpenClaw setup showing classifier smoke results, progress-only cases blocked, narration-plus-result cases accepted, and an ACP post-restart smoke after the patch.
Evidence reviewed

PR surface:

Source +17, Tests +70. Total +87 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 18 1 +17
Tests 1 71 1 +70
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 89 2 +87

Acceptance criteria:

  • [P1] pnpm exec oxfmt --check src/tasks/task-completion-contract.ts src/tasks/task-completion-contract.test.ts.
  • [P1] pnpm exec oxlint src/tasks/task-completion-contract.ts src/tasks/task-completion-contract.test.ts.
  • [P1] pnpm test src/tasks/task-completion-contract.test.ts src/agents/subagent-registry-lifecycle.test.ts src/acp/control-plane/manager.turn-results.test.ts.

What I checked:

  • Root AGENTS.md policy read: Root AGENTS.md was read fully and applied; its PR-review guidance requires checking the changed module, callers, tests, current main behavior, proof, and merge risk for task/session-state surfaces. (AGENTS.md:1, 4ae8d735bf54)
  • PR marker guard still runs before planning checks: At PR head, isProgressOnlyCompletionText returns false immediately when containsCompletionResultMarker matches, before hasNonProgressFollowupSentence and matchesProgressOnlyPrefix can block planning-only text. (src/tasks/task-completion-contract.ts:73, 7522a9caf23f)
  • Source probe confirms false positives: A read-only Node probe using the PR regex matched planning-only strings including 'Once done, I will report back' and 'I will check whether tests passed before continuing'. (src/tasks/task-completion-contract.ts:28, 7522a9caf23f)
  • Current main keeps helper private: Current main defines isProgressOnlyCompletionText as a private module helper; the PR changes it to an export only so the new tests can import it directly. (src/tasks/task-completion-contract.ts:54, 4ae8d735bf54)
  • Subagent caller persists classifier result: Required subagent completions call resolveRequiredCompletionTerminalResult before writing terminalOutcome and terminalSummary into finalized task state. (src/agents/subagent-registry-completion.ts:101, 4ae8d735bf54)
  • ACP caller also gates terminal state: ACP background-task terminal handling calls resolveRequiredCompletionTerminalResult before permission/output blocked checks, so false acceptance affects ACP task terminal state too. (src/acp/control-plane/manager.background-task.ts:65, 4ae8d735bf54)

Likely related people:

  • IWhatsskill: Authored PR fix(codex): block progress-only completions [AI-assisted] #85110, whose squash commit introduced the required-completion progress-only classifier and related regression tests now being modified. (role: introduced current classifier behavior; confidence: high; commits: 36e76ef42409; files: src/tasks/task-completion-contract.ts, src/agents/subagent-registry-lifecycle.test.ts)
  • takhoffman: The classifier-introducing squash commit records an approval signal from this handle, making them relevant for the classifier boundary and follow-up review routing. (role: review and approval signal; confidence: medium; commits: 36e76ef42409; files: src/tasks/task-completion-contract.ts, src/agents/subagent-registry-lifecycle.test.ts)
  • vincentkoc: Current-main blame and log show recent work carrying the task-completion contract and test file through release/refactor commits in the same surface. (role: recent area contributor; confidence: medium; commits: 3dbe98a1f737, e085fa1a3ffd; files: src/tasks/task-completion-contract.ts, src/tasks/task-completion-contract.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 (2 earlier review cycles)
  • reviewed 2026-06-30T17:10:15.343Z sha 676f3c2 :: needs changes before merge. :: [P1] Keep planning-only summaries blocked | [P2] Avoid a test-only classifier helper export
  • reviewed 2026-07-08T09:37:39.124Z sha 7522a9c :: needs changes before merge. :: [P1] Keep planning-only summaries blocked | [P2] Avoid a test-only classifier helper export

@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. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jun 6, 2026
@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 6, 2026
@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 6, 2026
This was referenced Jun 19, 2026
Closed
Closed
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 23, 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 the stale Marked as stale due to inactivity label Jul 23, 2026
@clawsweeper

clawsweeper Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(tasks): accept completion summaries with result markers This is item 1/1 in the current shard. Shard 2/4.

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

merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S stale Marked as stale due to inactivity status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@moeedahmed