feat(browser): surface bounded runtime diagnostics - #4175
Conversation
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Nice piece of work — the masking helper, the eviction counter, and the drain-on-result design are all the right shapes, and the docs/prompt match the implementation's ordering and draining behaviour. BLOCK on two issues I reproduced directly; both are contained fixes.
BLOCKER 1 — URL path segments are emitted raw, and tool results are persisted
maskBrowserRuntimeUrl (runtime-diagnostics.ts:31-36) masks the query string but passes url.pathname through verbatim:
return `${url.origin}${url.pathname}${url.search ? "?…" : ""}`;I ran it directly:
input https://example.com/account/SESSION_TOKEN_ABC123?k=v
output https://example.com/account/SESSION_TOKEN_ABC123?…
The query is masked; the secret in the path is not. Path-embedded tokens are common — signed URLs, invite/reset links, per-tenant identifiers, /u/<email> style routes.
This matters more than an ordinary log leak because these diagnostics are emitted into tool results at tab-worker.ts:600-605, and tool results are persisted and included in dump/share/export flows (docs/session-operations-export-share-fork-resume.md:72-80). A token from a page the agent merely visited ends up in an artifact the user may hand to someone else.
Fix: emit the origin only, and drop or hash path segments. Same treatment for exception class names — map them through a fixed allowlist or a syntax regex rather than echoing page-controlled strings (my probe also surfaced a page-controlled class name, CustomerAlice123).
BLOCKER 2 — the 20-entry bound is real, but total output size is not bounded
runtime-diagnostics.ts:79-91 correctly caps at 20 entries and counts evictions. But an individual entry has no size limit: the pathname is unlimited, and tab-worker.ts:604 serializes with an uncapped serializer (:186-190).
Measured:
https://example.com/ + "a".repeat(300000) → diagnostic length 300,020
One constructed URL. Twenty entries of that shape is ~6 MB. Because diagnostics go out as display content, they bypass the 256 KiB return-value cap in browser.ts:429-461 — so the cap that exists elsewhere does not protect this path.
Fix: per-field and total serialized-byte limits on the emission path, with an explicit truncation marker. Silent truncation would be its own problem for a diagnostic feature, so please make it visible.
Test gap
Disabling the displays.push(...) branch at tab-worker.ts:601 left all three focused suites green — 24 pass / 0 fail. So nothing currently proves diagnostics actually reach browser output. Worth a worker/supervisor-level test covering output ordering, drain-on-success, and retention after failure.
Checked and clear
- Renderer safety: the display path applies JSON escaping, tab replacement and width wrapping. No TUI control-sequence issue — page-controlled text cannot corrupt rendering.
- Mailbox memory stays capped at 20 regardless of console volume.
- Docs/prompt accurately describe ordering and draining. The one wording issue is "safe error class", which is not currently guaranteed given BLOCKER 1.
- Rebases cleanly onto current
origin/dev; no conflicts.
One thing to flag rather than block on
The feature is always on: one extra CDP session plus Runtime.enable per tab (tab-worker.ts:460-475, 526-533), and every Runtime.consoleAPICalled event — including ordinary non-error console traffic — crosses CDP and invokes the handler. Memory is bounded, so this is a cost question rather than a correctness one, but on a chatty page it is per-event work that nobody opted into. Worth a deliberate decision on whether this should be opt-in.
Fix the two blockers and I think this lands well — the underlying idea is genuinely useful.
Verification note: both blockers were reproduced by me against this PR head, not taken on report.
Maintainer review 4896349024 on PR Yeachan-Heo#4175 reproduced raw pathname leakage into persisted tool results, page-controlled error class names, and an unbounded serialized diagnostics block. Close all three and make the feature opt-in: - Emit http(s) URLs origin-only so path-embedded tokens (signed URLs, invite/ reset links, per-tenant ids) can never reach persisted tool results; non-URL values are hashed irreversibly instead of echoed, and about: paths are restricted to safe fixed names. - Map exception class names through a fixed allowlist of built-in error classes; page-controlled names like CustomerAlice123 are omitted, never echoed. - Bound serialization: url field capped at 256 chars with a visible ellipsis marker and the whole block capped at 4 KiB, shedding oldest entries first and marking runtimeDiagnosticsTruncated: true (never silent). - Make capture opt-in via open(..., { diagnostics: true }) so the extra CDP session and per-console-event traffic are opted into; docs, prompt, and changelog updated. - Add a WorkerCore-level emission test proving diagnostics reach the result (ordering, drain-on-success, retention after failure, byte bound, opt-in off) via a loadPuppeteerInWorkerForTest seam, plus extended unit tests. Constraint: origin-only URLs or irreversible bounded paths only Constraint: fixed allowlist for error class names Constraint: explicit truncation/drop metadata and total byte cap Rejected: query-masked pathname retention | path segments can carry secrets Rejected: identifier-regex class validation | pages control those names Tested: 16 focused diagnostics tests; 42 supervisor/action browser tests; coding-agent check:types; Biome Not-tested: live Chromium probe in this environment Lore-id: 4175-repair Confidence: high Scope-risk: narrow Reversibility: easy Supersedes: 412bd44 review posture
412bd44 to
154d8f7
Compare
Repair evidence — blockers from review 4896349024 are closed on this headMaintainer probepark's
The maintainer's repo owner carried the repair on owner-controlled branch BLOCKER 1 — pathname secret leakage: fixed
Maintainer probe re-run against the new head: BLOCKER 2 — page-controlled exception class: fixed
BLOCKER 3 — unbounded per-entry/total bytes: fixedSerialization is bounded on the emission path: the Test gap — worker output delivery: fixedNew WorkerCore-level emission tests ( CDP overhead flag — decided: opt-inThe extra CDP session plus Local verification on the exact new head
Hosted CI is running against this head; a fresh independent review follows. This repair supersedes the maintainer review posture on — |
Maintainer review 4896349024 on PR #4175 reproduced raw pathname leakage into persisted tool results, page-controlled error class names, and an unbounded serialized diagnostics block. Close all three and make the feature opt-in: - Emit http(s) URLs origin-only so path-embedded tokens (signed URLs, invite/ reset links, per-tenant ids) can never reach persisted tool results; non-URL values are hashed irreversibly instead of echoed, and about: paths are restricted to safe fixed names. - Map exception class names through a fixed allowlist of built-in error classes; page-controlled names like CustomerAlice123 are omitted, never echoed. - Bound serialization: url field capped at 256 chars with a visible ellipsis marker and the whole block capped at 4 KiB, shedding oldest entries first and marking runtimeDiagnosticsTruncated: true (never silent). - Make capture opt-in via open(..., { diagnostics: true }) so the extra CDP session and per-console-event traffic are opted into; docs, prompt, and changelog updated. - Add a WorkerCore-level emission test proving diagnostics reach the result (ordering, drain-on-success, retention after failure, byte bound, opt-in off) via a loadPuppeteerInWorkerForTest seam, plus extended unit tests. Constraint: origin-only URLs or irreversible bounded paths only Constraint: fixed allowlist for error class names Constraint: explicit truncation/drop metadata and total byte cap Rejected: query-masked pathname retention | path segments can carry secrets Rejected: identifier-regex class validation | pages control those names Tested: 16 focused diagnostics tests; 42 supervisor/action browser tests; coding-agent check:types; Biome Not-tested: live Chromium probe in this environment Lore-id: 4175-repair Confidence: high Scope-risk: narrow Reversibility: easy Supersedes: 412bd44 review posture
154d8f7 to
a2ef136
Compare
Page exceptions and console errors were invisible to browser callers when an interaction otherwise succeeded. Capture only bounded metadata and drain it into the next successful act/run response without retaining messages, values, arguments, or stacks. Constraint: keep at most 20 entries per tab and mask URL queries Rejected: domain sandbox and irreversible-action gating | raw Puppeteer run makes those separate policy designs Confidence: high Scope-risk: narrow Reversibility: easy Tested: 24 browser tests; coding-agent typecheck; live Chromium exception and console probe
Maintainer review 4896349024 on PR Yeachan-Heo#4175 reproduced raw pathname leakage into persisted tool results, page-controlled error class names, and an unbounded serialized diagnostics block. Close all three and make the feature opt-in: - Emit http(s) URLs origin-only so path-embedded tokens (signed URLs, invite/ reset links, per-tenant ids) can never reach persisted tool results; non-URL values are hashed irreversibly instead of echoed, and about: paths are restricted to safe fixed names. - Map exception class names through a fixed allowlist of built-in error classes; page-controlled names like CustomerAlice123 are omitted, never echoed. - Bound serialization: url field capped at 256 chars with a visible ellipsis marker and the whole block capped at 4 KiB, shedding oldest entries first and marking runtimeDiagnosticsTruncated: true (never silent). - Make capture opt-in via open(..., { diagnostics: true }) so the extra CDP session and per-console-event traffic are opted into; docs, prompt, and changelog updated. - Add a WorkerCore-level emission test proving diagnostics reach the result (ordering, drain-on-success, retention after failure, byte bound, opt-in off) via a loadPuppeteerInWorkerForTest seam, plus extended unit tests. Constraint: origin-only URLs or irreversible bounded paths only Constraint: fixed allowlist for error class names Constraint: explicit truncation/drop metadata and total byte cap Rejected: query-masked pathname retention | path segments can carry secrets Rejected: identifier-regex class validation | pages control those names Tested: 16 focused diagnostics tests; 42 supervisor/action browser tests; coding-agent check:types; Biome Not-tested: live Chromium probe in this environment Lore-id: 4175-repair Confidence: high Scope-risk: narrow Reversibility: easy Supersedes: 412bd44 review posture
a2ef136 to
b70775c
Compare
Final head
|
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Terminal review — MERGE_READY — exact head b70775c440bf3e63087a5db3653dcc6eedc9d878
gajae.pr-review-verdict.v1 merge-ready sha256:f8d2c89b3a0581c2 reviewer:owner-bot evidence:37-focused-tests-pass,package-check-green,hosted-CI-25-runs-0-fail,architect-APPROVE,qa-93-probes+live-chromium-zero-leak,terminal-critic-OKAY
This review supersedes maintainer review 4896349024 (filed against 412bd44afb, now outdated). All four blocking findings are closed on this head, and the flagged CDP-overhead question is resolved as opt-in:
- Pathname secret leakage — http(s) URLs emit origin-only; non-URL values hash irreversibly;
about:restricted; the masker itself is bounded at 256 chars with a visible…marker. The maintainer'sSESSION_TOKEN_ABC123-in-path probe now yieldshttps://example.com. - Page-controlled exception class — emitted only from a fixed allowlist of built-in error classes;
CustomerAlice123is omitted. - Unbounded per-entry/total bytes — per-field cap plus a 4 KiB total block cap, oldest shed first, explicit
runtimeDiagnosticsTruncated: true(never silent, including the single over-budget entry edge). - Missing worker emission mutation test —
browser-tab-worker-emission.test.tsproves output ordering, drain-on-success, retention after failure, the 300k-byte-pathname bound, and opt-in-off. - CDP overhead decision — capture is opt-in (
open(..., { diagnostics: true })); default-off creates no CDPRuntimesubscription and no per-event traffic.
Verification on the exact head: 37 focused tests pass / 0 fail; bun --cwd=packages/coding-agent run check (Biome + tsc) green; hosted CI 25 check runs with 0 failures; architect lane CLEAR/APPROVE; executor QA 93 real probes with a live headless-Chromium probe (zero secret leakage, bounded block) after fixing and re-verifying two synthetic edge probes; terminal critic gate OKAY.
No merge or release has been performed and none is implied by this verdict; the change is ready for the owner's merge decision. Original contributor commit remains reachable as 412bd44afb.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
3 participants
What
Surface a bounded, secret-safe mailbox of page runtime errors in the next successful browser
actorrunresponse.Runtime.exceptionThrownandconsole.errorevents for each active tab.{ runtimeDiagnostics, runtimeDiagnosticsDropped }.This is observability-only: it does not change action success, navigation, retry, tab lifecycle, or browser state.
Why
The native browser can complete an interaction while the page itself reports a React hydration failure, asynchronous exception, or
console.error. Those page failures were not visible in the tool response, so E2E callers could accept a visually plausible but broken state.This ports only the non-overlapping diagnostics lesson from the user-level webpilot skill. Domain allowlists and irreversible-action confirmation are intentionally excluded: raw Puppeteer
runmakes those separate policy/sandbox designs rather than a safe small patch.Duplicate searches for
browser pageerror,browser console error,browser diagnostics, andbrowser allowlistfound no existing GJC issue or PR covering this behavior.Testing
bun test packages/coding-agent/test/tools/browser-runtime-diagnostics.test.ts packages/coding-agent/test/tools/browser-actions.test.ts packages/coding-agent/test/tools/browser-tab-worker-startup.test.ts— 24 pass, 0 failbun --cwd=packages/coding-agent run check:types— passconsole-errorand onepageerror, masked?token=secretto?…, and emitted neither message/argument secretGJC verdict
No independent role review is claimed; the patch is left for maintainer review rather than extending the session with a provider-dependent critic lane.
devbun checkpasses