Skip to content

Commit 784ed89

Browse files
docs(webui): record the tenth review round and fix the round counts
The ninth codex review read the tree the eighth round left behind and raised two findings. Both were real, and both are fixed on the branch: - The session declared a liveness ping and never sent one. It held the pending-ping table, it answered a pong, and its own option comment said the session emits a ping — but no ping was ever written to the socket. An admitted session now pings every ten seconds and closes when the pong does not come within one interval, so a browser gone behind an open socket is noticed and the extension's reconnect ladder takes the session back up. - The real-browser acceptance failed on a machine without the installed Edge, reporting a broken bridge rather than an absent browser. Those cases skip with the reason now. The document also stops carrying two rounds with the same numbers: the two probe rounds are Probe A and Probe B, and the counts in the header, in the directory index and in the closing evidence say ten review rounds and twenty-one acceptance cases.
1 parent 25e4a58 commit 784ed89

2 files changed

Lines changed: 43 additions & 6 deletions

File tree

‎packages/webui/docs/browser-use/07-review-findings.md‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,17 @@
11
# Review findings — WebUI Browser Use
22

3-
**Nine review rounds** have been run against this change. Eight of them were run
3+
**Ten review rounds** have been run against this change. Nine of them were run
44
by a `codex` `gpt-6-luna` reviewer reading the source rather than the
55
description; the seventh was the final acceptance pass, run against the
66
finished tree and against the acceptance criteria the tickets state. This
77
document records what each round found and what was done about it.
88

9+
Two **probe rounds** — a real browser against the built extension, and the
10+
external path driven against it — are recorded here as well. They are numbered
11+
`Probe A` and `Probe B` rather than as rounds, because they were findings of
12+
the implementation, not of a reviewer, and because the two used to carry the
13+
same numbers as rounds 5 and 6.
14+
915
Every status below is a claim about the **tree**, and each one names the file
1016
that carries it. A finding that is still open is marked open; nothing here is
1117
rounded up.
@@ -321,7 +327,7 @@ tested without a DOM renderer — the client test environment has none.
321327
- **N4** — The stale claims in this document and in
322328
`03-extension-admission.md` are corrected.
323329

324-
## Round 5 — found by probing the real browser
330+
## Probe A — found by probing the real browser
325331

326332
No review round caught this one, because every test of the extension runs
327333
against its **TypeScript source**. A probe against a real Chrome shows the
@@ -343,7 +349,7 @@ This is the kind of gap that only a real browser finds, and it is the reason
343349
the programme's evidence says "not yet measured" for the external surfaces
344350
rather than claiming they work.
345351

346-
## Round 6 — the external path, against a real browser
352+
## Probe B — the external path, against a real browser
347353

348354
A Playwright spec now drives a **real Chrome with the built extension loaded**
349355
against a **real bridge server**: `packages/webui/test/browser/browser-bridge-real-browser.spec.ts`.
@@ -609,6 +615,37 @@ eight are closed.
609615
sealed credential is in the frame that carries it, so the check is about the
610616
sealing and not about a frame that never arrived.
611617

618+
## Round 10 — the ninth `codex` review, of round 9's fixes
619+
620+
The ninth `codex` review was run against the tree round 9 left behind, with
621+
the instruction to attack the fixes themselves. It raised two findings; both
622+
are closed.
623+
624+
| # | Severity | Finding | Status |
625+
| --- | --- | --- | --- |
626+
| S1 | MAJOR | **The session declared a liveness ping and never sent one.** It held the pending-ping table, it answered a `pong`, and its own option comment said the session emits a `ping` — but no ping was ever written to the socket. A browser that was gone behind an open socket was therefore never noticed. | `fixed` |
627+
| S2 | MAJOR | **The real-browser acceptance failed on a machine without the installed Edge.** The Edge cases drive `/usr/bin/microsoft-edge`, so a CI image or a container that does not ship Edge reported a broken bridge rather than an absent browser. | `fixed` |
628+
629+
### What was done
630+
631+
- **S1** — an admitted session now pings the peer every ten seconds and closes
632+
the session when the pong does not come within one interval. A WebSocket can
633+
stay open long after the browser behind it is gone: the extension's worker is
634+
terminated, the profile is closed, the machine sleeps. The socket's close is
635+
then the only signal, and it can take a while to arrive; the unanswered ping
636+
is the faster signal, and the extension's reconnect ladder picks the session
637+
back up. The close is transient at the system level — the peer's state lives
638+
in its own storage, so nothing is lost. The extension already answered a
639+
`ping` with a `pong`, so nothing on that side changed. The timer is cleared
640+
on teardown and does not hold the process open (`session.ts`,
641+
`startLiveness`).
642+
- **S2** — the Edge cases now **skip with the reason** when the installed Edge
643+
is not on the machine, rather than failing on a browser that was never there
644+
(`real-browser-bridge.ts`, `isBrowserInstalled`). The bundled Chromium is
645+
installed by the gate itself, so the Chromium cases always run. The record
646+
names the browser each case ran in, and the published matrix names the host,
647+
so a skipped case is visible rather than silent.
648+
612649
## What this document does not establish
613650

614651
- That the external path works end to end against a real Edge or Chrome for

‎packages/webui/docs/browser-use/README.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ status line of its own:
1111

1212
Where a document mixes the three, they are separated inline rather than merged into one sentence.
1313

14-
**Read [`07-review-findings.md`](07-review-findings.md) before anything else.** **Nine review rounds**
15-
have run against this change — eight by a `codex` `gpt-6-luna` reviewer reading the source rather
14+
**Read [`07-review-findings.md`](07-review-findings.md) before anything else.** **Ten review rounds**
15+
have run against this change — nine by a `codex` `gpt-6-luna` reviewer reading the source rather
1616
than the description, and a final acceptance pass against the finished tree. That document records
1717
every finding and states, for each, whether it is closed or still open. **Every finding is closed**;
1818
the ones that changed a decision are recorded in the decision record's amendments, and the ones that
@@ -74,7 +74,7 @@ open are listed in [`07-review-findings.md`](07-review-findings.md).
7474
| [`04-pairing-credentials.md`](04-pairing-credentials.md) | Credential lifecycle, the per-OS secure store on each platform, and the explicit-failure rule when no store exists | You implement pairing, credential storage, revocation or re-enable |
7575
| [`05-target-identity.md`](05-target-identity.md) | The three-part target identity and its generation, what invalidates a generation, and how a stale reference is rejected | You work with targets, input forwarding, tab selection or run scoping |
7676
| [`06-runtime-authorization-trace.md`](06-runtime-authorization-trace.md) | Each of the four authorization switches traced to the place it must be enforced, with the current implementation status of each check | You implement an authorization check, or you are auditing whether one exists |
77-
| [`07-review-findings.md`](07-review-findings.md) | All the findings from the seven review rounds, what was done about each, and what is still open | You want to know what is known to be broken, or what has been corrected |
77+
| [`07-review-findings.md`](07-review-findings.md) | All the findings from the ten review rounds, what was done about each, and what is still open | You want to know what is known to be broken, or what has been corrected |
7878
| [`packages/browser-extension/STORE-DISCLOSURES.md`](../../../browser-extension/STORE-DISCLOSURES.md) | The store disclosures for the Chrome Web Store and the Edge Add-ons, and the differences between a packaged build and a development build | You are preparing a store submission, or you need to know what a packaged build is |
7979
| [`09-acceptance-record.md`](09-acceptance-record.md) | The acceptance matrix: every surface, every case, the browser version, the operating system, the settings and the observed result — written from the records the suites produce | You want to know what was actually run, and where the evidence is thin |
8080

0 commit comments

Comments
 (0)