Codex review: needs real behavior proof before merge. Reviewed June 15, 2026, 12:37 PM ET / 16:37 UTC.

Summary
The PR broadens ACP persistent-session stale-resume recovery to include Kiro-style resume-required errors and adds positive and negative manager turn tests.

PR surface: Source +3, Tests +192. Total +195 across 2 files.

Reproducibility: yes. at source level: the linked issue provides exact Kiro stale-resume logs, and current source shows the retry path depends on the manager matcher. I did not run a live Kiro backend in this read-only review.

Review metrics: 2 noteworthy metrics.

• Recovery predicate change: 1 matcher family added. This predicate decides when OpenClaw clears persisted ACP identity and replays a user turn, so the accepted error shape matters before merge.
• Structured signal gap: 1 detailCode path still unhandled. The ACP contract carries detailCode, but current thrown manager errors drop it before the retry matcher runs.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
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] Propagate or match SESSION_RESUME_REQUIRED as structured data and add a startTurn result regression test.
• [P1] Add redacted live Kiro ACP terminal output, runtime logs, recording, or linked artifact showing stale-resume recovery after the patch.
• After updating proof in the PR body, let ClawSweeper re-review automatically or ask a maintainer to comment @clawsweeper re-review if it does not run.

Proof guidance:

• [P1] Needs real behavior proof before merge: Mock-only: the PR body shows unit-test output and explicitly says no live Kiro backend was used, so the contributor should add redacted live terminal/log/recording proof and update the PR body for re-review or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

• [P1] ACPX startTurn failures can carry detailCode: SESSION_RESUME_REQUIRED while the thrown manager error only preserves message text, so this PR can still miss stale Kiro resume failures.
• [P1] The contributor proof is mock-only and explicitly says no live Kiro ACP backend was used.
• [P1] This retry path clears persisted ACP identity and replays the user turn once, so matcher mistakes can affect session state and whether a stuck thread receives a normal reply.

Maintainer options:

1. Propagate structured resume matching (recommended)
   Preserve or pass SESSION_RESUME_REQUIRED into prepareFreshManagerRuntimeHandleRetry, add a startTurn result regression, and then require real Kiro proof or an explicit proof override.
2. Accept maintainer-owned proof gap
   A maintainer may intentionally accept mock-only proof only if they have independent Kiro runtime evidence and choose to track any structured-detail cleanup separately.
3. Pause for error-shape cleanup
   If maintainers want to stop matching rendered error text, pause this PR and first make ACP runtime errors preserve structured detail codes.

Next step before merge

• [P1] Human follow-up is needed because the PR needs both a structured error-contract repair and contributor or maintainer real Kiro proof before merge.

Security
Cleared: The diff only changes ACP manager error matching and tests; it does not touch secrets, dependencies, workflows, package scripts, permissions, or code-loading surfaces.

Review findings

• [P1] Propagate the structured resume-required signal — src/acp/control-plane/manager.runtime-resume-state.ts:26

Thanks for jumping on this. I agree with the P1 review that adding internal error directly to the matcher is too broad for this retry path. This helper clears persisted resume identity and replays the turn once, so the new case should require a stale-resume signal in the error chain, not just the persistent wrapper plus a generic internal error.

Concrete boundary I would suggest:

• keep the existing resource not found / no matching session cases
• for Kiro, require the persistent resume wrapper plus SessionResumeRequiredError or SESSION_RESUME_REQUIRED plus Internal error, ideally by inspecting the formatted ACP error chain instead of only the top-level message
• add negative tests for wrapper + generic Internal error without the resume-required signal, SessionResumeRequiredError without the persistent wrapper, and failures after user-visible output where retry must not happen

That should recover stale Kiro resumes without broadening unrelated ACP -32603 internal failures.

@yzhkali
▎ Thanks for the review! I've revised the fix based on your feedback:
▎
▎ 1. Narrowed the matcher: Replaced the generic internal error with SessionResumeRequiredError/SESSION_RESUME_REQUIRED check in the error chain (<-), using the formatAcpErrorChain output
▎ format. The outer persistent acp session .* could not be resumed guard is preserved, so only persistent-resume wrappers with a confirmed stale-resume signal trigger recovery.
▎ 2. Added negative test: does not retry after a persistent resume wrapper with generic Internal error without SessionResumeRequiredError — verifies that a matching wrapper with plain
▎ Internal error (no <- SessionResumeRequiredError) does NOT trigger retry and rejects with ACP_TURN_FAILED.
▎
▎ All 17 tests pass (15 existing + 1 Kiro positive + 1 negative).
▎
▎ The SessionResumeRequiredError without persistent wrapper case and the sawTurnOutput guard are already covered by existing code paths (params.meta.mode !== "persistent" and
▎ params.sawTurnOutput).

@clawsweeper re-review

🦞🧹
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:

• State: Complete
• Detail: The targeted re-review finished, the durable review comment was updated, and the synced verdict was routed.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/27557987036
• Updated: 2026-06-15T15:55:25.919Z

Hi @xialonglee — I worked the same issue (#87830) and wanted to flag a concern with the message-regex approach here, with evidence, in case it's helpful before this lands.

The added alternative requires session resume required / SessionResumeRequiredError to appear in the error message. But acpx builds that error's message without the class name — from the acpx dist (makeSessionResumeRequiredError):

new SessionResumeRequiredError(
  `Persistent ACP session ${params.record.acpSessionId} could not be resumed: ${params.reason}`,
  { cause: ... }
)

So for the real Kiro failure the message is "Persistent ACP session <id> could not be resumed: Internal error" — SessionResumeRequiredError is the error's .name, not part of the message. And the gate receives the raw message: both construction seams build the AcpRuntimeError with normalizeText(event.message) / normalizeText(result.error.message) (manager.turn-stream.ts), not the formatAcpErrorChain rendering (the <- SessionResumeRequiredError: … part only appears in log output).

In the retry test here, the event message is "…could not be resumed: Internal error <- SessionResumeRequiredError: …" — the <- SessionResumeRequiredError: … is concatenated in the fixture, but the live pipeline doesn't put that into event.message, so I believe the regex wouldn't match the real error and the turn would still fail to recover (the negative test passes for the same reason — message without the substring → no match).

An approach that avoids depending on the message wording is to key on acpx's structured detailCode: "SESSION_RESUME_REQUIRED" (set on the error regardless of the backend's reason text). I put that up as #93547 with red-on-main/green-with-fix coverage including the real Kiro message shape — happy to defer to whichever direction the maintainers prefer, just wanted to make sure the real message shape was on the record. Thanks for digging into this one.

Thanks @amersheeny for the thorough analysis — you are absolutely right. I verified the same code paths and confirmed that:

1. The real acpx makeSessionResumeRequiredError only puts the class name in .name, not in .message, so my message-based regex would not match in production.
2. errorFromTurnResult in manager.turn-stream.ts drops detailCode when building the AcpRuntimeError, so the chain-rendered format in my test fixture does not reflect real pipeline behavior.
3. Using structured detailCode: "SESSION_RESUME_REQUIRED" is the correct approach, and fix(acp): recover stale persistent sessions by structured resume-required code [AI-assisted] #93547 implements it properly.

Closing this in favor of #93547. Thanks again for catching this before it landed.

Closing in favor of #93547 which uses the structured detailCode approach — the correct fix for this issue.