aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authoramankumarsingh77 <amankumarsingh7702@gmal.com>2026-05-02 16:53:34 +0530
committeramankumarsingh77 <amankumarsingh7702@gmal.com>2026-05-02 16:53:34 +0530
commitc9d1605b1e3b8dbe88cd3674a9256aa0465f3e6d (patch)
treef14e155a6615fb2d7aca27642ec44463aeb2b18c
parent500dd206d300ab39599021ef9e80ed30d1871b9a (diff)
chore: address final-review minors — surface event-consumer crashes, document IIFE-wrap heuristic, log shutdown failures, fix CHANGELOG file-size claim
Five tweaks from the whole-implementation review: - session.ts: log unexpected event-consumer rejections instead of silent swallow - client.ts: comment the return-substring IIFE-wrap false-positive case - transport.ts: comment that ws.onclose post-open settle is no-op-by-design - index.ts: log client.stop() failures during shutdown - CHANGELOG.md: correct largest-file claim (transport.ts and client.ts are ~220 each, not "client.ts at ~180") Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
-rw-r--r--CHANGELOG.md2
-rw-r--r--src/cdp/session.ts8
-rw-r--r--src/cdp/transport.ts3
-rw-r--r--src/client.ts4
-rw-r--r--src/index.ts8
5 files changed, 22 insertions, 3 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0dab13d..ffc11e7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,7 +6,7 @@ All notable changes to pi-browser-harness will be documented in this file.
### Internal rewrite
-- Per-domain module split: every tool now lives in its own `src/domains/<name>.ts` file. The 1140-line `daemon.ts` and 2277-line `tools.ts` are gone; the largest single file is now `src/client.ts` at ~180 LOC.
+- Per-domain module split: every tool now lives in its own `src/domains/<name>.ts` file. The 1140-line `daemon.ts` and 2277-line `tools.ts` are gone; the three largest files are now `src/cdp/transport.ts` (~220 LOC), `src/client.ts` (~220 LOC), and `src/domains/js.ts` (~200 LOC).
- New transport/session/client split: `BrowserDaemon` class replaced with `createBrowserClient()` factory composing a `CdpTransport` (factory) and `CdpSession` (factory).
- All tool handlers now return `Result<T, E>`; one `defineBrowserTool` helper converts to pi's `ToolDefinition` and supplies a uniform `details` shape: `{ ok: true, ... }` on success, `{ ok: false, kind, message, ... }` on error.
- Strict TypeScript flags enabled (`noUncheckedIndexedAccess`, `exactOptionalPropertyTypes`, `noPropertyAccessFromIndexSignature`, `noUnusedLocals`, `noUnusedParameters`). Zero `any` in the codebase. All boundary `as` casts documented.
diff --git a/src/cdp/session.ts b/src/cdp/session.ts
index fffa241..b1ae71c 100644
--- a/src/cdp/session.ts
+++ b/src/cdp/session.ts
@@ -45,7 +45,13 @@ export const createCdpSession = (transport: CdpTransport): CdpSession => {
};
const restartConsumer = (): void => {
- activeConsumer = activeConsumer.then(() => consumeEvents()).catch(() => undefined);
+ activeConsumer = activeConsumer.then(() => consumeEvents()).catch((e: unknown) => {
+ // The .then() chain calls consumeEvents() which iterates the transport's
+ // events() AsyncIterable. The iterable resolves cleanly on close (returns
+ // {done:true}); any rejection here is an unexpected bug in the event
+ // handler, not a normal termination. Surface it on stderr so it's not lost.
+ console.warn("[pi-browser-harness] CDP event consumer crashed:", e);
+ });
};
restartConsumer();
diff --git a/src/cdp/transport.ts b/src/cdp/transport.ts
index 6da25fa..c5a5a41 100644
--- a/src/cdp/transport.ts
+++ b/src/cdp/transport.ts
@@ -156,6 +156,9 @@ export const createCdpTransport = (): CdpTransport => {
clearTimeout(timer);
ws = null;
cleanup("WebSocket closed");
+ // If onopen already settled the connect promise, this settle is a no-op
+ // (guarded by `settled`). cleanup() above still runs unconditionally so
+ // pending requests are rejected and the event queue is rotated.
settle(err(cdpError("transport_closed", "CDP WebSocket closed")));
};
});
diff --git a/src/client.ts b/src/client.ts
index 1fcef67..51dfa9f 100644
--- a/src/client.ts
+++ b/src/client.ts
@@ -117,6 +117,10 @@ export const createBrowserClient = (opts: BrowserClientOptions): BrowserClient =
};
const evaluateJs = async (expression: string, sessionId?: string): Promise<Result<unknown, CdpError>> => {
+ // Heuristic IIFE wrap: legacy convenience that lets agents write
+ // `return foo` instead of `(() => foo)()`. False-positive on substring
+ // `"return "` inside string literals or comments — agents that hit this
+ // can pass an explicit IIFE instead.
const wrapped = expression.includes("return ") && !expression.trim().startsWith("(")
? `(function(){${expression}})()`
: expression;
diff --git a/src/index.ts b/src/index.ts
index f5c7676..ee59f10 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -115,7 +115,13 @@ export default function browserHarnessExtension(pi: ExtensionAPI): void {
pi.on("session_shutdown", async () => {
persistState(pi, state);
if (client) {
- try { await client.stop(); } catch { /* best-effort */ }
+ try {
+ await client.stop();
+ } catch (e) {
+ // Shutdown is best-effort, but a stuck stop() points at a transport
+ // bug worth surfacing for debugging.
+ console.warn("[pi-browser-harness] client.stop() failed during shutdown:", e);
+ }
client = null;
}
toolsRegistered = false;