fix(acp): map error states to end_turn instead of unconditional refusal by pejmanjohn · Pull Request #41187 · openclaw/openclaw

pejmanjohn · 2026-03-09T15:31:24Z

Summary

Problem: handleChatEvent maps all gateway "error" states to ACP stop reason "refusal". This is semantically wrong — "refusal" means the agent deliberately refused (e.g., safety filter), but most errors are transient failures (timeouts, rate limits, API crashes)
Why it matters: Users see "agent refused to respond" in their IDE when the actual problem was a timeout or API error — confusing UX that makes users second-guess their prompts instead of retrying
What changed: Error states now resolve as "end_turn" instead of "refusal". Added a TODO noting that proper refusal detection requires a structured errorKind field in ChatEventSchema (which does not currently exist)
What did NOT change: "final" → "end_turn"/"max_tokens" mapping, "aborted" → "cancelled" mapping, delta event handling

Why not detect refusals heuristically?

The gateway error event schema (ChatEventSchema) only carries errorMessage (free text from formatForLog(error)). There are no structured fields like reason or errorCode — the schema enforces additionalProperties: false. Heuristic substring matching on free text is unreliable and would reintroduce the false-positive problem this PR fixes. The correct path is adding a structured errorKind field upstream.

Change Type

Bug fix

Scope

Gateway / orchestration
API / contracts

Linked Issue/PR

Related: ACP protocol gap analysis

User-visible / Behavior Changes

Gateway errors now report end_turn instead of refusal to ACP clients

Security Impact

New permissions/capabilities? No
Secrets/tokens handling changed? No
New/changed network calls? No
Command/tool execution surface changed? No
Data access scope changed? No

Repro + Verification

Environment

OS: macOS 15.4 (arm64)
Runtime: Node v24.13.0

Steps

Start an ACP session via openclaw acp
Trigger a gateway error (e.g., kill gateway mid-request, misconfigure model)
Observe the stop reason reported to the client

Expected

Client receives end_turn

Actual (before fix)

Client receives refusal

Evidence

Failing test/log before + passing after
New test file: src/acp/translator.stop-reason.test.ts (3 tests: error→end_turn, bare error→end_turn, aborted→cancelled regression guard)
All 124 existing ACP tests pass with no regressions

Human Verification

Verified: All ACP unit tests pass (pnpm vitest run src/acp/ — 124 tests, 16 files)
Verified: ChatEventSchema uses additionalProperties: false — structured error fields cannot exist on error events, confirming heuristic matching would be dead code
Not verified: End-to-end with a live ACP client (Zed)

AI-Assisted 🤖

Initial code generation via Codex CLI, significantly revised during human review
Testing: fully tested, 3 focused unit tests

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

Backward compatible? Yes — changes stop reason for errors from refusal to end_turn. Clients should handle all stop reason values.
Config/env changes? No
Migration needed? No

Failure Recovery

Revert this single commit

Risks and Mitigations

Risk: If a genuine content-filter refusal surfaces through the error path, it will now report end_turn instead of refusal
- Mitigation: The gateway does not currently emit structured refusal signals on error events. When ChatEventSchema gains an errorKind field, this mapping should be updated (noted in TODO comment). Until then, end_turn is strictly more correct than the unconditional refusal that was there before.

greptile-apps · 2026-03-09T15:33:48Z

Greptile Summary

This PR correctly fixes the semantic mismatch where all gateway "error" states were unconditionally mapped to the ACP "refusal" stop reason. The new isRefusalErrorPayload() helper ensures only genuine content-filter/safety refusals produce "refusal", while transient failures (timeouts, API crashes) now produce the more appropriate "end_turn".

Key observations:

The core logic change is sound and well-motivated.
The isRefusalErrorPayload function checks 10 payload paths, which is a pragmatic approach given that gateway error payloads are not formally typed.
One concern: The function checks free-text fields (payload.errorMessage, error?.message) for the broad keyword "safety". This can cause false positives — e.g., a gateway error message like "Type safety violation", "Memory safety check failed", or "safety subsystem unavailable" would all resolve to "refusal" instead of "end_turn", directly contradicting the PR's stated preference for erring toward end_turn in ambiguous cases. Restricting the "safety" hint to structured/semantic fields (reason, code, etc.) would eliminate this class of false positives without meaningfully reducing true-positive coverage.
The test suite covers the three main scenarios well but is missing a test that exercises the free-text errorMessage + "safety" false-positive path.

Confidence Score: 3/5

Safe to merge with the core logic fix, but the broad "safety" keyword check on free-text message fields can reintroduce a variant of the false-positive problem it set out to solve.
The fix is correct in intent and the structured-field checks are solid. The risk of false positives from checking errorMessage/error.message for the generic word "safety" is real and addressable, which keeps confidence from being higher. The failure mode (a server error being reported as "refusal" to the user) is the exact UX problem this PR is trying to fix.
Pay close attention to the isRefusalErrorPayload function in src/acp/translator.ts, specifically the containsRefusalErrorHint(payload.errorMessage) and containsRefusalErrorHint(error?.message) checks with the "safety" hint.

_{Last reviewed commit: e51a4a5}

src/acp/translator.ts

pejmanjohn · 2026-03-09T15:50:56Z

Addressed @greptile-apps feedback on free-text false positives, plus additional hardening:

Two-tier hint matching: Restructured isRefusalErrorPayload to distinguish between structured fields and free-text fields:

Structured fields (reason, code, errorCode, error.type, etc.): Check all hints — content_filter, safety, refusal
Free-text fields (errorMessage, error.message): Only check content_filter — the only truly unambiguous signal

This prevents false positives like "Memory safety check failed" or "Type safety violation" from triggering refusal. We also excluded "refusal" from free-text to guard against edge cases like "Connection refusal timeout".

Additional changes beyond Greptile feedback:

Added diagnostic logging: error state mapped to <reason> for session <key>
Added 4 new test cases (7 total): bare error payload, free-text "safety" false positive guard, structured "safety" → refusal, free-text "content_filter" → refusal

All 128 ACP tests pass.

greptile-apps · 2026-03-09T15:51:48Z

Excellent work! The implementation exactly matches your description:

Verified:

✅ Two-tier hint matching: REFUSAL_HINTS_STRUCTURED vs REFUSAL_HINTS_FREETEXT arrays (lines 64-65)
✅ 8 structured fields check all hints (reason, errorReason, stopReason, code, errorCode, error.reason, error.type, error.code)
✅ 2 free-text fields (errorMessage, error.message) only check content_filter
✅ Diagnostic logging at line 517: error state mapped to <reason> for session <key>
✅ All 7 test cases present, including the false-positive guards

The comment on lines 92-93 clearly documents the rationale: "'safety' and 'refusal' excluded to avoid non-refusal false positives". This prevents exactly the scenario you mentioned: "Memory safety check failed" won't trigger a false refusal.

The structured/free-text distinction is a smart design choice — it maximizes coverage for semantic fields while preventing incidental keyword matches in error strings. Well done!

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f4653ebedc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

src/acp/translator.ts

pejmanjohn · 2026-03-09T16:02:36Z

Significantly revised after deeper investigation. @greptile-apps

What changed: Removed the isRefusalErrorPayload heuristic entirely. Investigating the ChatEventSchema revealed that error events only carry errorMessage (free text) — there are no structured fields like reason or errorCode (additionalProperties: false). The heuristic was checking 10 fields that cannot exist on real gateway error events, plus unreliable substring matching on free text.

New approach: Error states simply map to end_turn. A TODO comment notes that proper refusal detection requires adding a structured errorKind field to the schema upstream.

The diff from main is now one behavioral line ("refusal" → "end_turn") plus a comment explaining why and what the real fix would be.

greptile-apps · 2026-03-09T16:03:47Z

Good call on the investigation. You're absolutely right — ChatEventSchema only has errorMessage with additionalProperties: false, so all those structured fields (reason, errorCode, error.type, etc.) can't exist on actual gateway events. The heuristic was dead code.

Mapping all errors to end_turn is strictly more correct than the unconditional refusal that was there before. The TODO comment properly documents that the real fix requires adding a structured errorKind field upstream.

The approach is sound. ✅

…pejmanjohn

…pejmanjohn)

onutc · 2026-03-09T16:38:03Z

Landed via temp rebase onto main.

Gate: pnpm lint && pnpm build && pnpm test
Land commit: b1e23ba
Merge commit: eab39c7

Thanks @pejmanjohn!

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b1e23baac9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector · 2026-03-09T16:43:08Z

src/acp/translator.ts

+      // do not treat transient backend errors (timeouts, rate-limits) as deliberate
+      // refusals.  TODO: when ChatEventSchema gains a structured errorKind field
+      // (e.g. "refusal" | "timeout" | "rate_limit"), use it to distinguish here.
+      this.finishPrompt(pending.sessionId, pending, "end_turn");


Preserve failure signal for chat error events

Mapping state: "error" to stopReason: "end_turn" turns backend failures into what ACP clients interpret as a normal completion, so timeouts/rate-limit/API failures can no longer be distinguished from successful turns. The gateway explicitly emits state: "error" for failed runs (with errorMessage), but this branch now resolves the prompt as success and drops that error context, which means clients lose retry/error-handling signals and can silently continue as if the agent finished normally.

Useful? React with 👍 / 👎.

@pejmanjohn

…al (#41187) * fix(acp): map error states to end_turn instead of unconditional refusal * fix: map ACP error stop reason to end_turn (#41187) (thanks @pejmanjohn) --------- Co-authored-by: Pejman Pour-Moezzi <[email protected]> Co-authored-by: Onur <[email protected]>

* main: (123 commits) acp: fail honestly in bridge mode (openclaw#41424) Gateway: tighten node pending drain semantics (openclaw#41429) Gateway: add pending node work primitives (openclaw#41409) fix(auth): reset cooldown error counters on expiry to prevent infinite escalation (openclaw#41028) fix(cron): do not misclassify empty/NO_REPLY as interim acknowledgement (openclaw#41401) iOS: reconnect gateway on foreground return (openclaw#41384) Doctor: fix non-interactive cron repair gating (openclaw#41386) Agents: add embedded error observations (openclaw#41336) Cron: enforce cron-owned delivery contract (openclaw#40998) fix(telegram): bridge direct delivery to internal message:sent hooks (openclaw#40185) plugins: harden global hook runner state (openclaw#40184) fix(acp): propagate setSessionMode gateway errors to client (openclaw#41185) fix(acp): map error states to end_turn instead of unconditional refusal (openclaw#41187) Update CONTRIBUTING.md Add Robin Waslander to maintainers Update CONTRIBUTING.md Allow ACP sessions.patch lineage fields on ACP session keys (openclaw#40995) fix(agents): bound compaction retry wait and drain embedded runs on restart (openclaw#40324) test(context-engine): add bundle chunk isolation tests for registry (openclaw#40460) fix(swiftformat): exclude HostEnvSecurityPolicy.generated.swift from formatters (openclaw#39969) ...

@pejmanjohn