Address the review findings from #1: four production bugs and five hardening fixes

[senamakel]

Summary

Addresses the nine findings from CodeRabbit's review of #1, which landed as a
blocking review after the branch had already merged. Every one was real; four
were bugs that would have bitten a host in production rather than style points.

Each fix follows the same rule the module already uses: the judgement goes in a
function that can be tested without a browser, and the test names the failure it
prevents.

Related issue

Follow-up to #1 (review comments on that PR).

API or behavior changes

Two behaviour changes, both fixing something that failed closed or leaked.

• about:blank is now admitted by a non-empty allowed_origins. It is not a
  network destination and it is how a caller clears the page; refusing it meant a
  session that set an allowlist could never let go of the last page it loaded,
  which is the opposite of what the setting is for.
• An allowlist entry may name a port — localhost:3000. It could before, in the
  sense that it was accepted; it matched nothing, because Url::parse reads it
  as the scheme localhost with the path 3000. An operator developing against
  a local server got everything blocked, and the failure looked like their typo.

Everything else is internal.

The findings

Bugs that would have hurt in production:

1. The launched browser's stderr was never drained (cdp/launch.rs). The
   reader was dropped once the debugger URL had been parsed out of it, leaving a
   pipe with no reader. Chrome writes to stderr for its whole life; a full pipe
   blocks the process writing into it. The browser would have appeared to hang at
   some arbitrary later moment, with nothing connecting it to startup. The reader
   now comes back with the URL and is drained until EOF.
2. A failed configure() leaked a browser (session/mod.rs). open
   propagated the error and dropped the session; the target stayed open and a
   launched Chrome kept running, because shutdown is async and Drop cannot
   call it. Every failed open leaked a process and a profile directory.
3. The session limit did not hold under concurrency (engine/mod.rs). The
   read lock was released before the launch, so concurrent callers all saw room
   and all proceeded. Twenty simultaneous opens against a limit of eight gave
   twenty Chromes. The slot is now reserved under the write lock and released by
   a guard if the launch fails.
4. Inline SVG was extracted as text (extract/script.rs). The skip set is
   upper case; tagName keeps its original case for foreign content, so <svg>
   reports svg and walked straight past it. The symptom is an icon's <title>
   turning up in the middle of a paragraph.

Robustness:

5. Screenshots are bounded before decoding (capture/mod.rs). The store
   refused an oversized image, but only after the decode had allocated it.
   Checked now from the length of the encoding, in a pure within_cap with its
   own tests.
6. Held outputs expire on their own (capture/store.rs, engine/mod.rs).
   Expiry ran only from insert and read, so a host that took sixteen large
   screenshots and then went quiet held every byte of them — in somebody else's
   process — indefinitely. A sweeper now runs on an interval, started on the
   first capture rather than in the constructor, because new is not async and
   may be called outside a runtime.
7. Snapshot traversal is bounded independently of rendered depth
   (snapshot/render.rs). depth only advances for nodes that are rendered, so
   a long chain of ignored or filtered wrappers never increased it and could not
   bound the recursion.

Tests and docs:

8. Allowlist cases for ports and about:blank (session/test.rs) — the two
   behaviour changes above, each with the test that would have caught it.
9. The integration doc pins the contract crate to a tag
   (docs/openhuman-integration.md). An unpinned git dependency resolves to the
   default branch, so a host would eventually compile against payload types newer
   than the module it loads, and the mismatch surfaces at runtime in a call.

Validation

• cargo fmt --all -- --check
• cargo clippy --all-targets --all-features -- -D warnings
• cargo build --all-targets --all-features
• cargo test --all-features

Also: the per-file coverage gate passes, and the live suite runs green against a
real Chrome.

Tests

Eleven new tests, all deterministic and none needing a browser:

• within_cap — an ordinary capture passes, an oversized one is refused, and the
  boundary agrees with the store it guards.
• Output expiry — an output past its TTL is gone, one inside it survives, and a
  sweep releases what no other call would have. Time is moved with a test-only
  age() rather than waited out, so the assertions are exact and instant.
• Allowlist — a host-and-port entry matches that port only, a bare host admits
  any port, an origin entry still carries its port, a default port matches an
  entry that spells it out, and about:blank is admitted under an allowlist.

Documentation

docs/openhuman-integration.md pins the dependency and says why the tag and the
registry version move together. The reasoning for each fix is on the code it
belongs to rather than here — the stderr drain, the slot reservation, and the two
traversal limits each carry the failure they prevent.

Checklist

• The change is focused on one logical change (one review, nine findings)
• No new #[allow(...)], #[ignore], or relaxed lints
• No secrets, tokens, or .env contents in the diff or the description

[coderabbitai]

Warning

Review limit reached

Your included review limit has been reached.

You’re in a promotional period — use the checkbox below to run this review for free:

• Run review for free

_{On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file.}

How can I continue?

Run this review now using the option above, or comment @coderabbitai review --use-credits.

You can also wait for the limit to reset (next review available in 13 minutes), then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 16aea53a-acb2-4c96-92a7-dfd877bbdd94

📥 Commits

Reviewing files that changed from the base of the PR and between 15bb036 and 1b06941.

📒 Files selected for processing (11)

• crates/tinybrowser/src/capture/mod.rs
• crates/tinybrowser/src/capture/store.rs
• crates/tinybrowser/src/capture/test.rs
• crates/tinybrowser/src/cdp/launch.rs
• crates/tinybrowser/src/engine/mod.rs
• crates/tinybrowser/src/extract/script.rs
• crates/tinybrowser/src/session/mod.rs
• crates/tinybrowser/src/session/policy.rs
• crates/tinybrowser/src/session/test.rs
• crates/tinybrowser/src/snapshot/render.rs
• docs/openhuman-integration.md

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[tinysweeper]

tinysweeper found nothing blocking. Approving.

$0.0000 · 0 in / 0 out · 741 embedded · openrouter/openai/text-embedding-3-small

[tinysweeper]

How this change flows

5 changed behaviours across 9 relationships. 4 surrounding behaviours are shown (60 graph nodes walked). 42 further behaviours left out to keep the diagram readable.

flowchart LR
  n0["pixels<br/>changed"]:::changed
  n1["OutputStore<br/>changed"]:::changed
  n2["..._than_the_cap_is_refused_rather_than_held<br/>changed"]:::changed
  n3["LaunchedBrowser<br/>changed"]:::changed
  n4["read_websocket_url<br/>changed"]:::changed
  n5["insert"]:::impacted
  n6["screenshot"]:::impacted
  n7["...ing_releases_what_nothing_else_would_have"]:::impacted
  n8["launch_within"]:::impacted
  n2 -->|calls| n5
  n2 -->|tests| n5
  n6 -->|calls| n0
  n6 -->|uses| n1
  n6 -->|calls| n5
  n7 -->|calls| n5
  n7 -->|tests| n5
  n8 -->|uses| n3
  n8 -->|calls| n4
  classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
  classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
  classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
  classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff

Loading

_{Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge.}