feat: typed error codes on everything raiseHand throws - #6
Merged
Conversation
`raiseHand` threw plain `Error`s, so the only way to tell "your account is at its session cap" from "that mode does not exist" was a message regex — and a message is not a contract. Every throw in the core is now a `HandraiseError` with a `code` (`missing_api_key`, `invalid_mode`, `empty_action`, `browser_unusable`, `relay_start_failed`, `concurrency_limit`, `relay_not_ready`, `relay_kill_failed`), the original SDK, CDP or network error as `cause`, and `isHandraiseError` to narrow a catch binding. Outcomes stay values: nothing that happens after the handoff URL exists became an exception. Two codes the plan did not list, both real throw sites: - `relay_kill_failed` — a relay sandbox that survives `kill()` leaves its public URL, and its last frame, reachable until the idle timeout. That is a different problem from a relay that never started. - `browser_unusable` — new guard. A page whose browser is already gone cannot be driven or screenshotted, so `raiseHand` now refuses it before it spends a sandbox, a QR code and a person's attention to reach `disconnected`. Every code has a test that triggers it, all of them offline: a real `node:http` gateway answering the SDK's 429 shape for `concurrency_limit`, a closed port for `relay_start_failed` and `relay_not_ready`, a page whose `context()` throws for `browser_unusable`. `createSandbox`, `killSandbox` and `waitForHealth` take their attempt/deadline budget as a parameter so those tests cost one request instead of the shipped backoff. A `satisfies Record<HandraiseErrorCode, string>` fixture in errors.test.ts fails `tsc` if a new code arrives without a test, and dist-smoke checks the class survives bundling (`code`, `name`, `cause`, `instanceof`).
Review round 1 on the typed errors. `relay_kill_failed` was documented as something `raiseHand` throws, and it is not: both `kill()` call sites catch it and log `relay_release_failed`, and neither `startRelay` nor `RelayHandle` is exported. A reader who wrote `if (error.code === "relay_kill_failed")` after `await raiseHand(...)` would have written dead code. The code is gone — seven codes, every one of them reachable from the public API — and `killSandbox` throws a plain `Error` again, as it did before. The README says in one sentence what actually happens instead, and the exhaustiveness fixture follows. `checkedPage` claimed `context()` throws on a closed page. It does not: in the shipped Playwright bundle it is a field read. So the documented case — the agent closed the page, the browser is still connected — was not detected, and the handoff would have gone all the way to a QR code before ending in `disconnected`. It now asks `page.isClosed()`, and the test fake models a real page (a working `context()`) so the assertion rests on `isClosed()` alone. Every URL handraise polls carries `?pt_token=…`, a live bearer credential. `relay_not_ready` quoted up to 80 bytes of the proxy's response body, and a proxy that echoes the request URI on 401 — a common default — would have put that token into an exception message, and exception messages get logged. `redactPreviewToken` now runs over every message that can carry proxy or SDK text. The test drives a real `node:http` proxy that echoes the request line. Also: the health poll's dead initial `lastAnswer` value, and a `finally` around the busy-gateway test so a failed expectation cannot leave a listening socket behind. Red-first, both verified: without the `isClosed()` arm the closed-page test fails and times out at 5 s (it reaches the network); without the redaction the leak test receives `pt_token=pt_a1b2c3d4…` in the message.
…andoff
Review round 2.
A caller's `logger` could end a handoff. `raiseHand` fires the webhook
notification and only awaits it in its `finally`, minutes later, so a
`logger.warn` that throws inside `notifyWebhook` left a rejected promise
unhandled for the whole handoff — node ends the process for that — and then
threw an uncoded `Error` out of `raiseHand`, long after a human had been shown
the URL. The same shape sat on the wide-event path, where losing the throw
would have lost the outcome. Log calls are now wrapped once where the logger
enters handraise (`safeLogger`), which also covers everything it is passed on
to, and the webhook promise carries its own `.catch` so "this never rejects"
is structural rather than incidental.
The health poll's `fetch` had no `signal`, so its deadline was only checked
between requests: a preview route that accepts the connection and never
answers held `startRelay` open for minutes — bun has no default request
timeout, node's undici waits 300 s — with a live sandbox burning its idle
window, and then reported "did not answer within 30000ms", which was not what
happened. Each attempt now aborts at the remaining deadline.
Token redaction only knew the literal `pt_token=` prefix. A proxy that
percent-encodes the request URI into a `?next=` parameter, or names the
credential in prose ("invalid preview token pt_…"), walked straight through it
— and the unit test covered only forms that already passed, which is the "a
gate you have never seen fail" case. Second rule for the credential itself,
twelve real forms in the test, and the body is redacted before it is sliced so
no cut can leave a partial token without its prefix.
Documentation, all of it wrong before: the CHANGELOG claimed no outcome became
an exception, when `browser_unusable` is exactly that (main created a relay and
returned `{ outcome: "disconnected" }` for a closed page) — now disclosed with
"add a `catch`"; `instanceof` advice leads with `error.code` and offers
`cause.status === 429`, which survives a second `@solarisdk/core` in the
consumer's tree; the README says the page check runs before the relay and that
a session dying server-side is still the `disconnected` outcome; `cause` is
"whenever there was one"; and `relay_start_failed` no longer promises nothing
was created.
Also: `killSandbox(sandbox, 0)` no longer reports a failure it never attempted.
Red-first, all three verified: without `safeLogger` the webhook test rejects
and the handoff test throws out of `runHandoff`; without the `signal` the
hanging-proxy test times out at 15 s.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Everything
raiseHandthrows now carries a code the caller can branch on.Seven codes, each reachable and each with a test that reaches it offline:
missing_api_key,invalid_mode,empty_action,browser_unusable(a new guard before the relay is started, so a closed page fails in milliseconds instead of after a sandbox was created),relay_start_failed(with the SDK error ascause),concurrency_limit(Solari's 429, mapped from the real SDK error shape against a local fake gateway),relay_not_ready. The message is written for a person reading a log and may change; the code is the contract. Outcomes (timeout,disconnected, …) stay values, and the contract thatraiseHandnever throws after the handoff URL exists still holds (verified by review: the only late failure, a sandbox that will not die, remains arelay_release_failedwarning).Also: any preview token that could enter an error message through a proxy's echoed body is redacted (tested against a real echoing proxy), and
createSandbox/killSandbox/waitForHealthtake their retry budgets as parameters so the tests cost one request instead of the production backoff — production numbers are unchanged.Not breaking: no message was ever a contract. One change worth knowing: SDK errors are wrapped, so
instanceof ConcurrencyLimitErroron a caught error moves toerror.cause(or use the code).Verification: lint, typecheck, 218 tests, build, dist smoke (9 exports). Opus review with one fix round; a GPT-5.6 Sol pass is in progress and will be applied before merge.
🤖 Generated with Claude Code