Skip to content

fix: handle -32601/-32602 in session/load fallback#174

Open
Bortlesboat wants to merge 1 commit intoopenclaw:mainfrom
Bortlesboat:fix-fallback-session-load-error-codes
Open

fix: handle -32601/-32602 in session/load fallback#174
Bortlesboat wants to merge 1 commit intoopenclaw:mainfrom
Bortlesboat:fix-fallback-session-load-error-codes

Conversation

@Bortlesboat
Copy link

Summary

  • shouldFallbackToNewSession() now handles JSON-RPC -32601 (Method not found) and -32602 (Invalid params) unconditionally, falling back to session/new instead of crashing
  • These codes indicate the agent doesn't properly support session/load (e.g., cursor-agent returns -32602), so fallback should happen regardless of whether the session has prior agent messages
  • The extractAcpError() call is hoisted above the sessionHasAgentMessages guard to cache the result and avoid redundant extraction

Test plan

  • Added test: -32602 with agent messages triggers fallback (was crashing before)
  • Added test: -32601 with agent messages triggers fallback
  • All 8 connect-load tests pass
  • Existing -32603 behavior unchanged (still gated behind sessionHasAgentMessages)
  • tsc --noEmit, oxlint, oxfmt clean

Closes #152

When an ACP agent returns JSON-RPC -32601 (Method not found) or -32602
(Invalid params) during session/load, acpx now gracefully falls back to
session/new instead of crashing. These codes indicate the agent does not
properly support session/load (e.g. cursor-agent) and should trigger
fallback unconditionally, regardless of whether the session already has
agent messages.

The extractAcpError() call is also hoisted above the
sessionHasAgentMessages guard to avoid redundant extraction.

Closes openclaw#152
@osolmaz
Copy link
Contributor

osolmaz commented Mar 25, 2026

doing some testing with some automated workflows on this PR, fyi

@osolmaz
Copy link
Contributor

osolmaz commented Mar 25, 2026

Triage result

Quick read

  • Intent valid: ✅ Yes
  • Solves the right problem: ✅ Yes
  • Validation: ✅ Bug reproduced and fixed
  • Close PR: ✅ No
  • Refactor needed: ✅ None
  • Human attention: ⚠️ Required
  • Recommendation: 🏁 escalate to a human
  • Human decision needed: ready for human landing decision

Intent

Make acpx recover cleanly when an ACP backend cannot actually restore a saved session, so reopening a persistent session starts a fresh ACP session instead of crashing.

Why

  • The change targets the actual fallback gate that was dropping -32601 and -32602 load failures on the floor.
  • The reported failure mode was reproduced locally by ablating the new fallback logic, then the restored PR fix changed the outcome back to passing.
  • The patch is narrow, the added tests cover the claimed crash path, and no refactor is needed first.

Codex review

  • Status: ✅ Clear
  • Notes: No existing GitHub Codex review findings were present. Fresh local review against the refreshed merge base did not surface a blocking P0/P1 issue before Codex transport/auth interruptions.

CI/CD

  • Status: 🚦 Green
  • Notes: The fork PR CI run was initially waiting on approval. I approved it, reran it, and the current workflow/checks for head 831d8e0 completed successfully.

Recommendation

🏁 escalate to a human

@osolmaz
Copy link
Contributor

osolmaz commented Mar 25, 2026

Triage result

Quick read

  • Intent valid: ✅ Yes
  • Solves the right problem: ✅ Yes
  • Validation: ✅ Reproduced and fixed
  • Refactor needed: ✅ None
  • Human attention: ⚠️ Required
  • Recommendation: 🏁 escalate to a human
  • Human decision needed: decide whether unconditional -32602 fallback on session/load is acceptable product behavior, or whether this PR should be narrowed or extended before merge

Intent

Make persistent acpx sessions reconnect cleanly by falling back to a fresh session when an agent's session/load support is broken or missing, instead of crashing.

Why

  • The PR targets the reported bug in connectAndLoadSession, where session/load errors with -32601 or -32602 were rethrown instead of falling back to session/new.
  • The linked issue describes the real user-facing failure: Cursor-backed persistent sessions crash on resume because session/load returns -32602.
  • Targeted validation reproduced the bug shape and fix behavior: the regression test passed on the PR head, failed after local ablation of the code change, and passed again after restoring the PR branch state.
  • The patch is small and focused, and the added tests cover the claimed failure path.

Codex review

  • Normalized GitHub Codex review data for the current head is clear: no GitHub Codex review findings were present.
  • The stored local Codex review result could not be established reliably: localCodexReview is null, and the captured raw/error output is malformed.
  • That malformed local review output still contains a possible P1 concern: treating every session/load -32602 as recoverable may silently rotate ACP sessions and lose remote continuity for adapters that support session/load but reject the specific request shape.
  • Because the only remaining blocker is this ambiguous local review evidence, this needs a human judgment call rather than an automated clear.

CI/CD

  • CI/CD status was not provided in the current run state for this handoff step.
  • The validation evidence above is the available source of truth here; no additional CI judgment was established in this step.

Recommendation

🏁 escalate to a human

A maintainer should decide whether the current behavior is the desired compatibility policy: if session/load returns -32602, should acpx always fall back to a fresh ACP session, or should that case stay visible unless there is stronger evidence that the adapter truly lacks usable load support. If the current policy is acceptable, this PR looks otherwise ready to continue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

shouldFallbackToNewSession does not handle -32602 from session/load, causing crash with cursor-agent

2 participants