fix: contain async loggers and redact real preview tokens (#7)

Answers the GPT-5.6 Sol review of the error codes: safeLogger contains async-rejecting loggers and throwing getters; the exact preview token (a JWT) and its encoded forms are redacted from every message; the SDK cause is a redacted clone that keeps prototype, status, code and the nested cause chain; isConnected() is guarded. Opus verification pass plus one fix round.

Author: Sy-D

CHANGELOG.md

@@ -13,29 +13,46 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   Everything `raiseHand` throws now carries a `code` you can branch on —
   `missing_api_key`, `invalid_mode`, `empty_action`, `browser_unusable`,
   `relay_start_failed`, `concurrency_limit`, `relay_not_ready` — plus the
-  original SDK, CDP or network error as `cause`. `concurrency_limit` is the
+  SDK, CDP or network error as `cause`, with any credential in it redacted. `concurrency_limit` is the
   one worth retrying: it means your Solari account is at its concurrent
   session cap, not that anything is broken. When to expect each code, and what
   to do about it, is in the README's [Errors](README.md#errors) table. The
   messages were never a contract; they can still be reworded in any release.
   Outcomes are unchanged and still values: a human who never came, a session
   that died mid-handoff and a webhook that 500s are not exceptions.
-- **A logger that throws can no longer end a handoff.** `logger` is your
-  object — a pino instance over a closed transport throws — and handraise
-  calls it from `catch` blocks and promise callbacks. One of those was the
-  webhook notification, which `raiseHand` fires and only awaits minutes later:
-  a throw there was an unhandled rejection (node ends the process for that)
-  and then an uncoded `Error` out of `raiseHand`, long after the URL existed.
-  Log calls are now wrapped where the logger enters handraise. A broken logger
-  costs a log line.
+- **A broken logger can no longer end a handoff.** `logger` is your object,
+  and handraise calls it from `catch` blocks and promise callbacks. Three ways
+  it breaks are contained where the logger enters handraise: a method that
+  throws (a pino instance over a closed transport), a method that is a getter
+  and throws on the property read, and a method that is `async` and rejects —
+  TypeScript accepts one where `Logger` declares `void`, and the rejection then
+  belongs to a promise nobody holds, which ends the process. One of the call
+  sites is the webhook notification, which `raiseHand` fires and only awaits
+  minutes later, long after the handoff URL exists. A broken logger costs a log
+  line.
 - **The relay health poll enforces its deadline.** Each attempt carries
   `AbortSignal.timeout`, so a preview URL that accepts the connection and never
   answers ends as `relay_not_ready` at the deadline instead of blocking
-  `raiseHand` for minutes with a live sandbox burning its idle window.
-- **No preview token can reach an error message.** Anything a gateway or proxy
-  says is redacted before it is quoted — `pt_token=…` in any case or
-  separator, percent-encoded inside a `?next=` parameter, or the bare
-  credential in prose.
+  `raiseHand` for minutes with a live sandbox burning its idle window. The
+  "Last answer" in that message is now the URL's own — a 401 from the preview
+  proxy, a refused connection — instead of the abort of a final request that
+  had no time left to make.
+- **The preview token is redacted out of error messages and out of `cause`.**
+  It is a live bearer credential for the relay, and a proxy that echoes the
+  request URI in its 401 body would otherwise put it in an exception message.
+  Where the exact value is known — the health poll, the teardown failure and
+  the wrapped start failure all hold the URL that carries it — that value is
+  removed by comparison in each of the forms an escaping proxy produces: bare,
+  percent-encoded, and with its dots written `%2E`, `%2e` or `.`. Three
+  patterns are the net for foreign text where the value is not known:
+  `pt_token=…` in any case or separator, a `pt_`-prefixed value, and the JWT
+  shape the preview token actually has (three base64url segments, separator
+  literal or escaped — see `docs/measurements/01-preview-transport.md` §3). The
+  SDK error attached as `cause` goes through the same redaction, because every
+  error serialiser prints the whole chain. A proxy that invents an encoding
+  none of those cover — folding the value across lines, say — is still a leak;
+  this is a net, not a proof.
+
 ### Changed
 
 - **A page that is already dead is now refused instead of handed off.**
@@ -51,9 +68,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   wrapped in a `HandraiseError`, with the SDK's error kept as `error.cause`.
   Branch on `error.code === "concurrency_limit"`; if you must have the class,
   it is `error.cause`, and `error.cause.status === 429` is the check that
-  survives a second copy of `@solarisdk/core` in your tree. Apart from the
-  page check above, nothing throws that did not throw before, and no outcome
-  became an exception.
+  survives a second copy of `@solarisdk/core` in your tree. `cause` is the SDK's
+  error with credentials redacted: a copy carrying the same prototype and the
+  same property descriptors — so `name`, `status`, `code`, the non-enumerable
+  `message` and `stack`, and the `cause` chain hanging off it all survive, and
+  `JSON.stringify(cause)` still produces what it did — with `message`, `stack`,
+  the parsed `body` and every nested `cause` rewritten. An error that cannot be
+  copied without running its own code (a throwing getter, a body that
+  references itself) becomes a plain redacted `Error` rather than an exception.
+  Apart from the page check above, nothing throws that did not throw before,
+  and no outcome became an exception.
 
 ## [0.5.1] - 2026-09-02
 

README.md

@@ -296,7 +296,11 @@ asked for anything yet. Everything after that is an `outcome`, never an
 exception. What it throws is a `HandraiseError` with a `code`: the code is the
 contract, the message is for whoever reads the log and may be reworded in any
 release. `isHandraiseError` narrows a `catch` binding, and `cause` keeps the
-original SDK, CDP or network error whenever there was one.
+original SDK, CDP or network error whenever there was one — the same class,
+`name`, `status` and `code`, its own non-enumerable properties, and its own
+`cause` chain — with credentials redacted out of every `message`, `stack` and
+response body along it. Every error serialiser prints the whole chain, so a
+clean outer message on its own would not be worth much.
 
 The first thing `raiseHand` does is look at your page, before it creates
 anything: a page you have closed, or a browser you have disconnected, is
@@ -323,7 +327,7 @@ try {
 | `invalid_mode` | `mode` is neither `"takeover"` nor `"approval"`. | Fix the call. TypeScript already refuses it; this is for JavaScript callers. |
 | `empty_action` | `mode: "approval"` without a non-empty `action`. | Name the step the human says yes or no to. |
 | `browser_unusable` | The page is closed, or its browser has disconnected — checked before anything is created. | Open a new page or relaunch the session (restore `storageState` if you kept it) and retry. |
-| `relay_start_failed` | The relay sandbox could not be created or deployed. | Read `cause` — it is the Solari SDK's own error. Retry. Nothing is left behind unless you also see `relay_release_failed` (below). |
+| `relay_start_failed` | The relay sandbox could not be created or deployed. | Read `cause` — it is the Solari SDK's own error, redacted. Retry. Nothing is left behind unless you also see `relay_release_failed` (below). |
 | `concurrency_limit` | Your Solari account is at its concurrent session cap (429). | Free a session, or wait and retry. The one relay failure that is purely temporary. |
 | `relay_not_ready` | The sandbox started but its public URL never answered. | Retry. Persisting means the preview proxy or the region is unhealthy. |
 

src/core/handoff.test.ts

@@ -688,6 +688,34 @@ test("a page whose state cannot be read at all is refused too", async () => {
   })
 })
 
+test("a browser whose liveness accessor throws is refused too", async () => {
+  // The last unguarded read in the pre-flight check: a browser proxy — a
+  // remote-CDP wrapper, a pooled session object, a page handed over between
+  // processes — whose `isConnected()` throws instead of answering. Outside the
+  // `try` that would leave `raiseHand` rejecting with a plain `Error`, which
+  // is exactly what typed codes exist to stop.
+  const browserPartial: Partial<Browser> = {
+    isConnected: () => {
+      throw new Error("Browser has been closed")
+    },
+  }
+  const contextPartial: Partial<BrowserContext> = {
+    // SAFETY: the guard reads only `browser()` off the context.
+    browser: () => browserPartial as Browser,
+  }
+  const pagePartial: Partial<Page> = {
+    isClosed: () => false,
+    // SAFETY: the guard reads only `context().browser()` on the page.
+    context: () => contextPartial as BrowserContext,
+  }
+
+  // SAFETY: the guard touches `isClosed` and `context` and nothing else.
+  await expect(askOn(pagePartial as Page)).rejects.toMatchObject({
+    name: "HandraiseError",
+    code: "browser_unusable",
+  })
+})
+
 test("an open page whose browser has disconnected is refused too", async () => {
   // The Solari session hit its ~10-minute hard lifetime while the agent was
   // still working. The page is not closed and `context()` answers — only the
@@ -1520,3 +1548,73 @@ test("a logger that throws does not break the handoff", async () => {
   // take `onEvent` with it.
   expect(events).toHaveLength(1)
 })
+
+test("a logger whose methods reject does not break the handoff either", async () => {
+  // The same option, one shape further out: `debug(event, fields): void`
+  // accepts an `async` implementation, so the failure arrives as a rejected
+  // promise nobody holds rather than as a throw. Unhandled, that ends the
+  // agent's process mid-handoff — before the relay sandbox is released, which
+  // leaves a public URL and its last frame reachable until the idle timeout.
+  //
+  // The gate here is the runner: `bun test` fails a test that leaves an
+  // unhandled rejection behind, which is how this was watched failing against
+  // the unfixed wrapper. The listener below is NOT that gate — bun claims the
+  // rejection first and never calls it, so `unhandled` stays empty either way.
+  // It is kept because it costs nothing and states the invariant for a runner
+  // that only warns; do not read it as the thing that catches a regression.
+  const port = await startRelayProcess()
+  const human = await connectHuman(port)
+  const cdp = fakeCdp()
+  const unhandled: string[] = []
+  const record = (cause: unknown): void => {
+    unhandled.push(String(cause))
+  }
+  process.on("unhandledRejection", record)
+  try {
+    let calls = 0
+    const down = async (): Promise<never> => {
+      calls += 1
+      throw new Error("log shipper is gone (async)")
+    }
+    const rejecting: Logger = {
+      debug: down,
+      info: down,
+      warn: down,
+      error: down,
+    }
+    const events: HandoffEvent[] = []
+
+    const handoff = runHandoff({
+      page: fakePage(cdp.cdp),
+      agentWsUrl: `ws://127.0.0.1:${port}/ws?role=agent`,
+      options: {
+        reason: "the logger ships its lines over a socket that went away",
+        logger: rejecting,
+        onEvent: (event) => events.push(event),
+      },
+      timeoutMs: 5000,
+      url: "https://relay.example/?pt_token=x",
+      handoffId: "async-rejecting-logger",
+      relayColdStartMs: 5,
+      logger: rejecting,
+    })
+
+    await until("the phone to connect", () => human.inbox.length >= 0)
+    human.send({ type: "handback" })
+
+    const end = await handoff
+    expect(end.outcome).toBe("resolved")
+    // The wide event still reaches the caller, and the logger was really
+    // called — a containment that stopped logging would pass vacuously.
+    expect(events).toHaveLength(1)
+    expect(calls).toBeGreaterThan(0)
+
+    // Long enough for the loop turn on which an unhandled rejection is
+    // reported, after the handoff has fully torn down. Inert under bun — see
+    // the note above the listener.
+    await Bun.sleep(50)
+    expect(unhandled).toEqual([])
+  } finally {
+    process.off("unhandledRejection", record)
+  }
+})

src/core/raise-hand.ts

@@ -632,17 +632,26 @@ function checkedMode(options: RaiseHandOptions): HandoffMode {
  * connected? `context()` is a field read and throws nothing in Playwright, so
  * it is `isClosed()` that catches a closed page; the try/catch is for the page
  * object that is not a working Playwright page at all.
+ *
+ * Every read is inside the `try` and every branch after it. A browser proxy —
+ * a remote-CDP wrapper, a pooled session, a page handed between processes —
+ * can throw from its liveness accessor too, and a plain `Error` out of the
+ * guard whose whole job is to produce `browser_unusable` would be the last
+ * uncoded rejection on this path.
  */
 function checkedPage(page: Page): void {
   let closed: boolean
-  let browser: Browser | null
+  let connected: boolean
   try {
     closed = page.isClosed()
-    browser = page.context().browser()
+    const browser: Browser | null = page.context().browser()
+    // A context with no browser is a persistent context: there is no session
+    // object to ask, and `isClosed()` above has already spoken for the page.
+    connected = browser?.isConnected() ?? true
   } catch (cause) {
     throw new HandraiseError(
       "browser_unusable",
-      `handraise: this page cannot be handed to a human — reading its state (page.isClosed(), page.context()) threw. A dead CDP connection does that, and so does a page-like object that is not a Playwright page. ${String(cause)}`,
+      `handraise: this page cannot be handed to a human — reading its state (page.isClosed(), page.context().browser().isConnected()) threw. A dead CDP connection does that, and so does a page-like object that is not a Playwright page. ${String(cause)}`,
       { cause },
     )
   }
@@ -652,7 +661,7 @@ function checkedPage(page: Page): void {
       "handraise: this page is already closed, so there is nothing for a human to take over. Open a new page (its `storageState` from an earlier handoff, if you kept it, restores the human's work) and retry.",
     )
   }
-  if (browser && !browser.isConnected()) {
+  if (!connected) {
     throw new HandraiseError(
       "browser_unusable",
       "handraise: the browser session behind this page is already disconnected, so there is nothing for a human to take over. Relaunch the session (its `storageState` from an earlier handoff, if you kept it, restores the human's work) and retry.",

src/errors.ts

@@ -31,7 +31,7 @@
  *   session that has died server-side while the CDP socket is still up looks
  *   alive here and still ends as the `disconnected` outcome.
  * - `relay_start_failed` — the relay sandbox could not be created or deployed;
- *   `cause` holds the SDK error.
+ *   `cause` holds the SDK error, with credentials redacted.
  * - `concurrency_limit` — the Solari account is at its concurrent session cap
  *   (HTTP 429). The one relay failure that is worth retrying later.
  * - `relay_not_ready` — the sandbox started but its public URL never answered.
@@ -52,7 +52,11 @@ export type HandraiseErrorCode =
 
 /**
  * Everything handraise throws on purpose. `cause` carries the original SDK,
- * CDP or network error whenever there was one, so the wrapping hides nothing.
+ * CDP or network error whenever there was one — same class, same `name`,
+ * `status` and `code`, same own properties and same `cause` chain — so the
+ * wrapping hides nothing. Every `message`, `stack` and response body along
+ * that chain is redacted, because the relay's preview token is a live bearer
+ * credential and every error serialiser prints the whole chain.
  */
 export class HandraiseError extends Error {
   override readonly name = "HandraiseError"

src/logger.test.ts

@@ -13,6 +13,7 @@ import {
   type Logger,
   noopLogger,
   quietLogger,
+  safeLogger,
 } from "./logger"
 
 /** The console methods, captured so a test can restore them. */
@@ -117,3 +118,93 @@ test("quietLogger drops debug/info but forwards warn/error — the library defau
   expect(err[0]?.parsed.event).toBe("w")
   expect(err[1]?.parsed.event).toBe("e")
 })
+
+// --- safeLogger ----------------------------------------------------------
+//
+// `Logger` is the caller's object, and the two ways it breaks that a plain
+// `try` does not cover are a method that is `async` — TypeScript accepts one
+// where the interface declares `void` — and a property that is a getter.
+
+test("safeLogger contains a logger whose methods reject", async () => {
+  // The gap a `try` cannot see: `debug(event, fields): void` accepts an
+  // `async` implementation, so the throw happens after `safeLogger` has
+  // already returned. The rejection then belongs to a promise nobody holds,
+  // and the runtime ends the process for that — mid-handoff, before the relay
+  // sandbox is released. `bun test` fails a test that leaves one behind, so
+  // the red signal here is this test failing with "log shipper is gone".
+  //
+  // Deliberately not inside `expect(...).not.toThrow()`: that wrapper marks
+  // rejections raised during the call as handled, which would hide exactly
+  // what is under test.
+  let calls = 0
+  const down = async (): Promise<never> => {
+    calls += 1
+    throw new Error("log shipper is gone (async)")
+  }
+  const rejecting: Logger = {
+    debug: down,
+    info: down,
+    warn: down,
+    error: down,
+  }
+  const safe = safeLogger(rejecting)
+
+  safe.debug("d", { a: 1 })
+  safe.info("i")
+  safe.warn("w")
+  safe.error("e")
+
+  // Long enough for the microtask queue to settle and for the loop turn on
+  // which an unhandled rejection is reported.
+  await Bun.sleep(50)
+  // The wrapper still calls the logger — containment is not silence.
+  expect(calls).toBe(4)
+})
+
+test("safeLogger survives a logger whose method is a throwing getter", () => {
+  // A proxy over a closed transport, or a class that builds its methods
+  // lazily: the throw happens on the property read, before any call.
+  const exploding = (): never => {
+    throw new Error("the sink was torn down")
+  }
+  const brokenGetters: Logger = {
+    get debug(): never {
+      return exploding()
+    },
+    get info(): never {
+      return exploding()
+    },
+    get warn(): never {
+      return exploding()
+    },
+    get error(): never {
+      return exploding()
+    },
+  }
+  const safe = safeLogger(brokenGetters)
+
+  expect(() => {
+    safe.debug("d")
+    safe.info("i")
+    safe.warn("w")
+    safe.error("e")
+  }).not.toThrow()
+})
+
+test("safeLogger still forwards to a working logger", () => {
+  // The containment above may not turn the wrapper into a second noopLogger.
+  const seen: string[] = []
+  const inner: Logger = {
+    debug: (event) => seen.push(`debug:${event}`),
+    info: (event) => seen.push(`info:${event}`),
+    warn: (event) => seen.push(`warn:${event}`),
+    error: (event) => seen.push(`error:${event}`),
+  }
+  const safe = safeLogger(inner)
+  safe.debug("d")
+  safe.info("i")
+  safe.warn("w")
+  safe.error("e")
+
+  expect(seen).toEqual(["debug:d", "info:i", "warn:w", "error:e"])
+})