< fresh machine 37269> Please review <https github com OpenH OpenHands #automations-debug

<@U0BME18CF7S> Please review <github.com/OpenHands...

calm-artist-7206

10/06/2026, 11:32 PM

@fresh-machine-37269 Please review github.com/OpenHands/OpenHands/pull/18051 . Feel free to approve it if you think fit, or request changes or comment as appropriate.

👀 1

fresh-machine-37269

10/06/2026, 11:32 PM

🤖 On it! View progress here: oss-agent-canvas.ngrok.dev/conversations/…

fresh-machine-37269

10/06/2026, 11:33 PM

✅ Done! This PR is already merged, so there's nothing left to approve or request changes on. Here's the summary: PR #18051 — "fix(errors): show the server's message instead of raw HttpError text in generic error toasts" • Author: @enyst • Status: ✅ Merged today (2026-10-06 at 21:53 UTC) by @enyst • Review: Approved by the review bot at 21:28 UTC What it does:

retrieveAxiosErrorMessage

now short-circuits on

isSdkHttpError

and returns

getApiErrorMessage(error, "")

, so an SDK

HttpError

displays the server's `message`/`detail`/`error` (or FastAPI validation text) rather than the raw

HTTP request failed (<status>): {json}

string. When the body has no usable message, callers fall back to their own localized text (e.g.

ERROR$GENERIC

). Axios/plain-`Error`/string paths are unchanged, including the "Disconnected (...)" wording. It also switches

getApiOrConnectionErrorMessage

to call

getUserFacingConnectionErrorMessage

directly to avoid a circular import. Verification noted at merge time: focused tests (56 passed), CI green on the head SHA, and live before/after evidence for all four affected flows (422 condenser save, 404 stale MCP delete, 404 shared-conversation link, 502 backend down). One thing worth flagging for follow-up (already called out in the PR notes): a 502 with a plain-text body now shows the generic "An error occurred" instead of text hinting the backend was down, and Agent Server 500s will show

Internal Server Error

until #18025 lands to surface the

exception

reason. Neither blocks the merged change.

calm-artist-7206

10/06/2026, 11:51 PM

please also review @fresh-machine-37269 https://github.com/OpenHands/OpenHands/pull/18087 and do the same rules

👀 1

fresh-machine-37269

10/07/2026, 12:04 AM

✅ Done! Done — I reviewed PR #18087 and approved it. PR #18087 — "test(e2e): give the preset automation's Slack stand-in the catalog identity" (@enyst, fixes #18083) What it does: The

mock-llm-preset-automation.spec.ts

fixture seeded Slack as

{ command: "echo", args: ["dummy-slack-mcp"] }

. Since #18034, Canvas recognizes an installed stdio integration by the catalog's command + leading args, so that stand-in stopped matching and the card opened the install dialog instead of a conversation. The spec now reads Slack's stdio transport from

INTEGRATION_CATALOG

(so it follows catalog changes) and adds

npm_config_offline: "true"

to keep the stand-in inert. Why I approved: • Test-only change, no

src/

touched — no Agent Server contract or

minimumAgentServer

bump needed. • I confirmed the pinned catalog (

@openhands/extensions@0.29.0

) gives exactly

npx -y @zencoderai/slack-mcp-server

, which is what

transportMatchesServer

now compares against. • Verified the stand-in really is inert: offline

npx

fails in ~0.6s with

ENOTCACHED

and downloads nothing. • CI on the exact head passes (

test-and-build

ubuntu/windows, PR description, title, artifacts, Docker build);

live-e2e

is skipped by design on PRs. • All of the issue's acceptance criteria are met, and the seeded env still uses placeholders with

afterEach

clearing

mcp_config

. One non-blocking note in the review: since

mock-llm-e2e.yml

runs on `main`/dispatch rather than PR events, the "both tests pass" evidence is a local run — fine for a low-risk test-only change, and a maintainer can optionally dispatch the workflow against the branch for independent confirmation.

Open in Slack

Previous Next