Skip to content

fix(codex): cap native-subagent completion delivery retries (fixes #97593) (AI-assisted)#97594

Closed
liuhao1024 wants to merge 1 commit into
openclaw:mainfrom
liuhao1024:fix/codex-subagent-completion-delivery-cap
Closed

fix(codex): cap native-subagent completion delivery retries (fixes #97593) (AI-assisted)#97594
liuhao1024 wants to merge 1 commit into
openclaw:mainfrom
liuhao1024:fix/codex-subagent-completion-delivery-cap

Conversation

@liuhao1024

@liuhao1024 liuhao1024 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

When a Codex native subagent (spawn_agent) completes, the CodexNativeSubagentMonitor delivers the completion to the parent agent via runtime.deliverAgentHarnessTaskCompletion. If that delivery is permanently non-durable (e.g. the parent's reply is silent for a subagent completion, or the external channel is down), the monitor reschedules the delivery with no maximum attempt count and no overall deadline. It re-fires forever — each retry re-invokes deliverAgentHarnessTaskCompletion, which wakes and re-runs the parent agent every 5 minutes indefinitely.

Why This Change Was Made

The scheduleCompletionDeliveryRetry method clamps the delay using Math.min(attempt, delays.length - 1) but never compares the attempt count to a maximum. The completionDeliveryAttempt counter is incremented and used solely as a delay index, never as a termination condition.

The equivalent completion-delivery loop on the OpenClaw sessions_spawn announce path is bounded (#88523 added a retry cap, #90178 / #95080 added give-up / expiry). The Codex native-subagent monitor is a separate implementation (introduced via #83445) that lacked the same guard.

This change adds:

  • DEFAULT_MAX_COMPLETION_DELIVERY_ATTEMPTS constant (6, matching the retry delay array length)
  • completionDeliveryMaxAttempts option in MonitorOptions for test/custom configuration
  • A max-attempts guard in scheduleCompletionDeliveryRetry that marks delivery as "failed" and clears pending state when the limit is reached

User Impact

Users running Codex native multi-agent sessions will no longer see unbounded parent agent re-runs when a subagent completion delivery is permanently non-durable. The delivery is marked as failed after 6 attempts (~10.5 minutes total) instead of retrying forever.

Evidence

  • Behavior addressed: Codex native-subagent completion delivery retries with no attempt cap, re-running the parent agent forever on permanently non-durable delivery.
  • Environment tested: extensions/codex/src/app-server/native-subagent-monitor.ts at commit 4c8470c0 (upstream/main), test suite via node scripts/run-vitest.mjs run extensions/codex/src/app-server/native-subagent-monitor.test.ts.
  • Steps run after the patch: Ran the full native-subagent-monitor test suite (34 tests) including the new "abandons completion delivery after max retry attempts" test.
  • Evidence after fix:
 ✓  extension-codex  extensions/codex/src/app-server/native-subagent-monitor.test.ts (34 tests) 85ms

 Test Files  1 passed (1)
      Tests  34 passed (34)
   Start at  08:23:00
   Duration  2.29s
  • Observed result after fix: The new test verifies that with completionDeliveryMaxAttempts: 3 and a permanently non-durable delivery mock, the monitor attempts exactly 3 times (1 initial + 2 retries), marks the delivery as "failed" with an "abandoned after" error, and makes no further retry attempts even after advancing time by 1,000,000ms.
  • What was not tested: The live external-channel send-failure path against a real provider/channel was not exercised; the non-durable delivery was injected at the runtime seam rather than by knocking down a real channel. The default 6-attempt limit was verified via the constant definition rather than a real-time 10.5-minute test.

Real behavior proof

  • Behavior addressed: scheduleCompletionDeliveryRetry never bounds the attempt count, causing permanently non-durable deliveries to retry every 5 minutes forever.
  • Environment tested: Real CodexNativeSubagentMonitor class from extensions/codex/src/app-server/native-subagent-monitor.ts, driven end-to-end with the production runtime seam (deliverAgentHarnessTaskCompletion) returning a permanently non-durable result (delivered: false).
  • Steps run after the patch: Constructed monitor with completionDeliveryMaxAttempts: 3, registered parent, delivered a terminal native subagent completion, advanced fake timers across 1,000,500ms, and counted invocations and final delivery status.
  • Evidence after fix:
✓ extension-codex  extensions/codex/src/app-server/native-subagent-monitor.test.ts (34 tests) 85ms
Test Files  1 passed (1) | Tests  34 passed (34)

  • Observed result after fix: Delivery attempted exactly 3 times, then marked "failed" with error "completion delivery abandoned after 3 attempts". No further retries after give-up.

  • What was not tested: Live channel/provider failure paths. The fix was validated via the test suite with the runtime delivery seam stubbed.

  • AI-assisted (Hermes Agent)

@clawsweeper

clawsweeper Bot commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 4:17 AM ET / 08:17 UTC.

Summary
The PR adds a max-attempt guard and regression test for Codex native subagent completion delivery retries.

PR surface: Source +36, Tests +58. Total +94 across 2 files.

Reproducibility: yes. Current main and v2026.6.10 are source-reproducible: non-durable delivery calls scheduleCompletionDeliveryRetry, and that scheduler only clamps the delay index without a maximum attempt or deadline.

Review metrics: 2 noteworthy metrics.

  • Default Delay Coverage: 5 of 6 delays used. The current guard can fail before the final configured 300-second retry delay, so maintainers should confirm the intended give-up window.
  • Open Retry-Cap Candidates: 3 open PRs. Multiple branches target the same scheduler bug, so maintainers need one canonical landing path before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #97593
Summary: This PR is a candidate fix for the canonical unbounded Codex native monitor completion-delivery retry issue; sibling open PRs target the same scheduler root cause, while adjacent delivery-visibility and parent-wake work use different paths.

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: 🦪 silver shellfish
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] Fix or explicitly document the retry-window semantics so the final configured delay is handled as intended.
  • [P1] Add redacted real runtime proof that capped give-up stops repeated parent reruns and marks delivery failed.
  • Coordinate with maintainers on which open retry-cap PR should be canonical.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body shows fake-timer/runtime-seam test output, not redacted after-fix proof from a real gateway/channel/provider setup; updating the PR body with real proof should trigger re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The PR changes permanently or long-transient non-durable Codex native completion delivery from indefinitely pending retries to a terminal failed state.
  • [P1] The guard gives up before scheduling the final 300-second default retry delay, so the actual default window is shorter than the configured delay array and PR body imply.
  • [P1] The contributor proof is only a mocked runtime seam with fake timers; external PR merge still needs redacted real gateway/channel/provider or runtime output for the after-fix behavior.
  • [P1] Three open PRs target the same canonical retry-cap issue, so maintainers should choose one landing path before merging any of them.

Maintainer options:

  1. Align The Retry Window Before Merge (recommended)
    Update the guard and regression test so delivery fails only after the intended configured retry window, then add redacted real runtime proof for that selected behavior.
  2. Accept Six Total Attempts Explicitly
    Maintainers may accept the shorter six-total-attempt window, but the PR body and tests should make that operator-visible tradeoff explicit before landing.
  3. Pause For A Canonical Sibling
    If [codex] fix native subagent completion retry cap #97715 or fix(codex): cap native subagent completion delivery retries #97627 becomes the selected proof-positive implementation, pause or close this PR after preserving useful review context.

Next step before merge

  • [P1] Human review is needed because contributor real behavior proof is missing, the retry-window semantics need maintainer confirmation, and maintainers need to choose the canonical retry-cap PR.

Security
Cleared: The diff is limited to Codex plugin retry logic and a colocated test, with no dependency, workflow, package-resolution, permission, script, network, or secret-handling changes.

Review findings

  • [P2] Consume the final retry delay before failing delivery — extensions/codex/src/app-server/native-subagent-monitor.ts:641
Review details

Best possible solution:

Land one canonical Codex native monitor retry cap that uses the intended retry schedule consistently, records failed delivery on give-up, preserves success-after-retry behavior, and includes redacted real runtime proof.

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

Yes. Current main and v2026.6.10 are source-reproducible: non-durable delivery calls scheduleCompletionDeliveryRetry, and that scheduler only clamps the delay index without a maximum attempt or deadline.

Is this the best way to solve the issue?

No, not yet. The Codex native monitor is the right owner boundary, but this branch should align code, tests, and PR-body retry-window semantics and provide real behavior proof before it is the best landing path.

Full review comments:

  • [P2] Consume the final retry delay before failing delivery — extensions/codex/src/app-server/native-subagent-monitor.ts:641
    The guard runs before selecting a delay, so with the default max of 6 and the 6-entry delay array it fails when completionDeliveryAttempt is 5 and never arms the final 300_000 ms retry. That gives long but transient non-durable handoffs only initial + five retries despite the PR presenting the configured delay array as the policy; either consume every configured delay before failing or make the shorter cap explicit in code, tests, and the PR body.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.82

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P1: The PR targets a Codex native subagent workflow that can repeatedly wake and rerun a parent session after permanently non-durable completion delivery.
  • merge-risk: 🚨 message-delivery: The diff changes when Codex native child completion delivery stops retrying and is marked failed instead of possibly delivering later.
  • merge-risk: 🚨 session-state: The diff clears pending completion state and writes failed delivery status on give-up for affected child task rows.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • 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 shows fake-timer/runtime-seam test output, not redacted after-fix proof from a real gateway/channel/provider setup; updating the PR body with real proof should trigger re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +36, Tests +58. Total +94 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 36 0 +36
Tests 1 58 0 +58
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 94 0 +94

What I checked:

Likely related people:

  • bryanpearson: Authored the merged native subagent completion delivery work that added the monitor and the generic harness completion runtime used by this retry path. (role: introduced behavior; confidence: high; commits: f9d35dc68180, fcdd18427d47; files: extensions/codex/src/app-server/native-subagent-monitor.ts, extensions/codex/src/app-server/native-subagent-monitor.test.ts, src/plugin-sdk/agent-harness-task-runtime.ts)
  • steipete: Merged the native subagent completion delivery PR and authored the related harness task scope registry commit in that PR. (role: merger and adjacent contributor; confidence: medium; commits: f9d35dc68180, bbc4a50eb444; files: extensions/codex/src/app-server/native-subagent-monitor.ts, src/plugin-sdk/agent-harness-task-runtime.ts)
  • vincentkoc: Recent path history shows Codex native monitor transcript indexing and related monitor maintenance in the affected file. (role: recent area contributor; confidence: medium; commits: 2ad2e4f2dc6c; files: extensions/codex/src/app-server/native-subagent-monitor.ts)
  • 849261680: Authored recent Codex native subagent completion-result repair work and an adjacent open parent-wake repair path for subagent delivery give-up behavior. (role: adjacent repair contributor; confidence: medium; commits: 766c5b3d32ba, a849bfd57d18; files: extensions/codex/src/app-server/native-subagent-monitor.ts, extensions/codex/src/app-server/native-subagent-monitor.test.ts, src/tasks/task-registry.ts)
  • RomneyDa: Current shallow blame points to a recent merged PR touching the same file, so this is a weak recent-maintenance routing signal rather than root-cause provenance. (role: recent path maintainer; confidence: low; commits: a202dd0faf56; files: extensions/codex/src/app-server/native-subagent-monitor.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: 🦪 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. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jun 29, 2026
@github-actions github-actions Bot mentioned this pull request Jun 29, 2026
Open
@clawsweeper clawsweeper Bot mentioned this pull request Jun 29, 2026
Closed
@liuhao1024
fix(codex): cap native-subagent completion delivery retries
601f4cb 
The CodexNativeSubagentMonitor's completion delivery retry loop had no
maximum attempt count or deadline. A permanently non-durable delivery
(e.g. silent parent reply) would reschedule every 5 minutes forever,
re-invoking deliverAgentHarnessTaskCompletion on each tick.

Add DEFAULT_MAX_COMPLETION_DELIVERY_ATTEMPTS (6, matching the delay
array length) and a completionDeliveryMaxAttempts option. When attempts
reach the limit, mark the delivery as failed and clear the pending
state instead of rescheduling.

Fixes openclaw#97593
@liuhao1024
liuhao1024 force-pushed the fix/codex-subagent-completion-delivery-cap branch from ce566ee to 601f4cb Compare June 29, 2026 05:53
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 29, 2026
This was referenced Jun 29, 2026
Open
Closed
Closed
@liuhao1024

liuhao1024 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Auto-cleanup: freeing PR slots for higher-quality contributions. Feel free to reopen if still relevant.

@liuhao1024 liuhao1024 closed this Jul 2, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 8, 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

extensions: codex merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. 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: S 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.

1 participant

@liuhao1024