fix(inner): never report a blank turn error from ACP-style executors (#4281)

[abhay-codes07]

Related issue

Closes #4281

Summary

Every generic-ACP turn that raised an exception surfaced to the operator as {"code": "runner_error", "message": "inner executor error: "} — an empty message, with the real traceback going only to the harness subprocess logger where an operator can't reach it.

Root cause: the ACP / Goose / Qwen executors report a reader failure via str(exc), which is empty for several stdlib exceptions raised without a message (a bare RuntimeError(), TimeoutError(), …). The harness adapter then re-raises f"inner executor error: {event.message}" verbatim, so a blank str(exc) reaches the user as a turn that failed for no stated reason.

This fixes the reporting half of the issue (the actionable one the issue calls out):

• New shared helper describe_exception(exc) in inner/executor.py: returns str(exc) when non-empty, else repr(exc) (which always names the exception class). Uses the same str(exc) or repr(exc) idiom already used elsewhere in the repo.
• Applied at all three identical reader error paths: acp_executor.py, goose_executor.py, qwen_executor.py.
• Defense-in-depth in runtime/harnesses/_executor_adapter.py: an ExecutorError with an empty message from any other path now yields a non-blank "inner executor error" rather than a trailing-blank string.

The underlying per-agent failure (whatever the exception was) is now at least named by its type instead of being invisible.

Test Plan

Added to tests/inner/test_acp_executor.py:

• describe_exception falls back to repr for a bare RuntimeError() (whose str() is ""), naming the type
• preserves a real message verbatim (no repr noise)
• is never blank across a range of stdlib exceptions

OMNIGENT_SKIP_WEB_UI=true uv run pytest tests/inner/test_acp_executor.py -q -k describe_exception
# 6 passed

ruff check + ruff format --check clean. (Three pre-existing failures in this area — two ACP fake-agent e2e tests and one Goose bwrap sandbox test — reproduce identically on clean main in a non-Linux dev env and are unrelated.)

Demo

N/A. Backend error-reporting fix, no visual surface. Before: a bare-exception ACP turn fails with inner executor error: (empty). After: it fails with e.g. inner executor error: RuntimeError(), naming the failure.

Type of change

• Bug fix
• Feature
• UI / frontend change
• Refactor / chore
• Docs
• Test / CI
• Breaking change

Test coverage

• Unit tests added / updated
• Integration tests added / updated
• E2E tests added / updated
• Manual verification completed
• Existing tests cover this change
• Not applicable

Changelog

Generic-ACP / Goose / Qwen turns that fail now report the exception type instead of a blank "inner executor error: " with no detail.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

@abhay-codes07 This PR is a Bug fix, Feature, or UI / frontend change but the Demo section is missing or only contains a placeholder.

These change types require a screenshot or screen recording so reviewers can see the new behaviour without checking out the branch. Please update the Demo section with:

• A screenshot or screen recording of the change, or
• A link to a hosted video or GIF showing the new behaviour.

Use N/A only when the change has no user-visible effect whatsoever (e.g. a pure refactor or test-only change). If that's the case, uncheck the relevant type box and check Refactor / chore or Test / CI instead.

[abhay-codes07]

Extended the fix in a second commit: the same blank-message pattern lives in every executor's run_turn failure path (yield ExecutorError(message=str(exc))), which goes blank for a bare exception. The adapter guard already prevents a blank reaching the operator, but only with a generic fallback. Routed all 15 of those sites through describe_exception too, so every harness (claude-sdk/native, codex, cursor, antigravity, goose, hermes, kimi, kiro, openai-agents, qwen, acp) names the actual exception type. Mechanical single-helper change; ruff clean, all 15 modules import, and the executors' run_turn tests still pass.

[dhruv0811]

/review

[omnigent-ci]

Polly AI Review

Blocking issues

None. The change is a mechanical, low-risk swap of str(exc) → describe_exception(exc) at exception-reporting sites, plus a defensive fallback in the adapter. The core helper (str(exc) or repr(exc)) is correct and the reader error paths (acp/goose/qwen), the adapter guard, and the various run_turn ExecutorError sites all behave as described.

Security vulnerabilities

None. repr(exc) on stdlib exceptions names the type and echoes any message already present — it does not expose new secrets beyond what str(exc) would, and nothing here touches auth, deserialization, or network boundaries.

Non-blocking notes

• PR description understates scope. The description says the fix applies to "all three identical reader error paths" plus the adapter, but the diff actually converts the ExecutorError(message=str(exc)) sites across ~14 additional executors (antigravity, claude native/sdk, codex, cursor, goose native, hermes, kimi, kiro, openai agents sdk, qwen native, etc.). This broader consistency pass is welcome, but the description/Changelog should mention it so a reader isn't surprised by the file count (+90/-19 across 18 files vs. the "three paths" framing).

• Whitespace-only messages still slip through. describe_exception uses str(exc) or repr(exc), so a message that is non-empty but blank (e.g. RuntimeError(" ")) is truthy and returned as-is, yielding inner executor error: . The test test_describe_exception_never_blank uses .strip() != "" which would pass only because its cases are truly empty; the helper itself doesn't strip. This is the same near-blank symptom the PR targets. Consider str(exc).strip() or repr(exc) if you want to fully close it — minor, since bare-message exceptions are the common real case.

• Remaining str(exc) reporting sites. A few sibling reporting paths still use str(exc) (e.g. acp_executor.py _warn_initialize_failed(str(exc)), copilot_executor.py:433 error=str(exc)). Not in scope and not the reported failure path, but they share the same latent blank-message risk if you want a follow-up.

Summary

A clean, well-targeted reporting fix: it guarantees a failed generic-ACP/Goose/Qwen (and now most other executor) turn names its exception type instead of surfacing a blank inner executor error: , with a sensible last-line guard in the adapter and focused regression tests. No correctness or security concerns. The only substantive gaps are cosmetic-adjacent: the description undersells the actual file scope, and the or fallback still lets a whitespace-only message through. Safe to merge as-is; the whitespace edge case is worth a one-line follow-up.

---

_{Automated review by Polly · workflow run}

[dhruv0811]

LGTM! Thanks for adding the better error handling :)

[omnigent-ci]

Polly AI Review

Blocking issues

None. describe_exception(exc) = str(exc) or repr(exc) is correct: it returns the real message when present and falls back to repr(exc) (which always names the class) for bare exceptions. The call-site substitutions are all in error-reporting paths where str(exc) was previously fed to a message field, so behavior only changes for the empty-message case. The adapter guard (event.message or "no detail reported (…)") is a sound last-line defense and cannot regress a populated message.

Security vulnerabilities

None. The change only widens what a failing turn reports to repr(exc), which contains the exception type and its already-user-facing message — no new secret exposure beyond what str(exc) already surfaced. No lockfile or dependency changes.

Non-blocking notes

• Description understates scope. The summary says the fix is "applied at all three identical reader error paths (acp, goose, qwen)," but the diff actually swaps str(exc) → describe_exception(exc) across ~14 executors (antigravity, claude_native, claude_sdk, codex, cursor, goose_native, hermes, kimi, kiro, openai_agents_sdk, qwen_native, etc.), most of them run_turn/effort-validation paths rather than reader paths. The broader application is reasonable and low-risk, but the PR text should reflect it so a reviewer/merger isn't surprised by the file count.
• repr() can leak more than str(). For exceptions whose repr includes constructor args (e.g. OSError(2, 'No such file')), the operator-facing string now carries slightly more detail than before. This is desirable here and matches the intent, but worth being aware of for any exception type that stuffs sensitive data into its args — not the case for the stdlib types targeted here.
• Test coverage is proportionate. The three new tests cover the blank-fallback, verbatim-message, and never-blank cases directly against the helper. The adapter's event.message or … guard itself isn't unit-tested, but it's trivial and defense-in-depth.

Summary

A clean, correct, low-risk bug fix that ensures a failing inner-executor turn always names its failure instead of surfacing inner executor error: with an empty message. The helper is simple and right, applied uniformly, and backed by a defense-in-depth guard in the adapter plus focused unit tests. No blocking or security concerns; the only follow-up is aligning the PR description's stated scope with the actual (broader) set of touched executors. No visual demonstration is warranted — this is a backend error-reporting change with no user-visible UI surface.

---

_{Automated review by Polly · workflow run}