fix(recapture): re-read the Egress per request in the Wayback gate#1920

closed
main
claude/github-pr-comment-vmswqr
opened10d agobyMMatt Donovan
View on GitHubTwo-way sync enabledPR last imported 8d ago

No issue: the second review round on the Egress pull request, opened against its branch to merge into it

Summary

The Wayback replay pane now checks the saved Egress on every request, so an open pane stops loading when the settings file becomes unreadable or is switched to a proxy from outside the app, and its WebRTC rule follows the same check. An update check that waited for one change of route no longer goes out on that route when the setting changed again while it waited. A proxy the pane refused is retried once another proxy has applied, so the pane no longer stays shut for good. Proxy hosts that are neither a host name nor an IP address are refused, and Settings follows the saved Egress when it changes after the pane first loads. This branch merges into the Egress pull request's branch; the two items that need wording in that pull request's own description are listed in the details.

Changes

Files changed, grouped by purpose, with the review finding each answers
  • src/main/services/egress.ts (findings 1 and 5): createRoutedSession re-reads the setting in allowsRequests on every call and opens only when no apply is in flight and the session carries that route. It tracks what the session carries (carried) apart from the latest apply in flight (pending), so an unavailable event or a second change while an apply is in flight keeps the gate shut until the latest lands. A successful apply clears refused. A new optional onRoute callback hears each Egress the gate follows, the unavailable one included.
  • src/main/index.ts (finding 1): the Wayback guests' WebRTC policy is set from onRoute, so a change the gate notices on a request, not only an in-app write, re-sets it. The policy at guest creation is unchanged.
  • src/main/services/updater.ts (finding 4): check() and download() wait through egressSettled, which re-calls egressReady() after each await until the result is settled for the current setting.
  • src/shared/schemas.ts, src/main/services/backgroundRenderer.ts (finding 7): parseProxyUrl checks the host with isHostLiteral, so a dotted-quad IPv4 address with every octet at most 255, DNS labels, or a bracketed IPv6 address; .., -, 1.2.3.999 and 999.999.999.999 are refused. proxyFailure also matches ERR_NO_SUPPORTED_PROXIES.
  • src/shared/types.ts (finding 8): the app.egress_apply_failed comment says the pane refuses every request after a refused apply.
  • src/renderer/components/settings/OperatorConfig.tsx (findings 9 and 2): the radios, address and label follow the saved Egress whenever it changes, using the adjust-state-during-render pattern rather than a one-time seed. The intro now begins "The connection these routes use", so it no longer reads as the full list of Birdbrain's traffic.
  • Tests: egress.test.ts (six cases: unreadable while open, changed outside the app, unavailable during a Direct apply, two changes landing out of order, a refused proxy retried after another applied, onRoute), updater.test.ts (a second change while a check waits), settings.test.ts (four refused hosts, three accepted), backgroundRenderer.test.ts (ERR_NO_SUPPORTED_PROXIES named), SettingsMaintenance.test.tsx (radios follow a changed saved Egress). Each new case fails on the previous head and passes here.
  • Not changed, for the Egress pull request's own description (findings 2 and 3): its list of what stays unrouted should name PDF export of a capture stored as plain HTML, which still fetches that page's remote files over the device's own connection; and its Evidence impact section should say that while the saved Egress cannot be read, the verifier prints "skipped because the Egress was not Direct" for an extension capture even when the unreadable file held Direct, a sentence only a verifier release can change.
  • Verification below fails on one test, tests/main/services/staging.test.ts "keeps the row when the bytes cannot be removed". This container runs as root, so a permission-denied removal succeeds; the same test fails the same way on the previous head f7a890c2, and no file it exercises is in this diff. Because that run failed, coverage was not scored locally; CI's run is the one to read.
  • Not changed (finding 6): the request gate and the WebRTC wiring are now in egress.ts and unit-tested there; the per-guest loop in index.ts stays uncovered, as index.ts is not instrumented.

Evidence-affecting

Yes

What could change for evidence, and how that is checked

Evidence impact

  • Evidentiary result or interpretation that could change: none in a signed entry. The changes decide when the Wayback pane and the updater may send and which proxy hosts the setting accepts. A host now refused could have been stored before, and it now reads back as an unknown Egress, which sends nothing.
  • What verification proves and does not prove after this change: unchanged. No verifier, entry shape or schema version changes.
  • Backward verification preserved (existing evidence packages still verify): yes, no verifier code changed.
  • Known-answer test added or extended, or why none applies: none applies, since no signed field changes; the unit tests above pin the gate, the updater wait and the host check.

Verification

A pre-merge check failed

pnpm preflight at 051f7cc60a47ce4fb890bfc9c7b1e2048110dbdf on Node v20.20.0; diff scored against origin/main (merge base 4ae473a68). Result: fail.

  • pnpm lint - pass (exit 0)
  • pnpm format:check - pass (exit 0)
  • pnpm lint:boundaries - pass (exit 0)
  • pnpm lint:agents-md - pass (exit 0) - CLAUDE.md and AGENTS.md match
  • pnpm lint:adr - pass (exit 0)
  • vale-prose --base - pass (exit 0) - 1 changed Markdown file checked
  • pnpm typecheck - pass (exit 0)
  • pnpm build - pass (exit 0)
  • pnpm build:extension - skipped - no extension/ changes
  • pnpm test:coverage - fail (exit 1) - 1 failed, 6555 passed, 28 skipped (6584), failed before thresholds were evaluated
  • pnpm coverage:diff - fail (exit 1) - Missing /home/user/birdbrain/coverage/coverage-final.json. Run "pnpm test:coverage" before scoring diff coverage.

Pull request description generated by Claude Code


Generated by Claude Code

Checks

1 failing · 18 successful · 10 skipped
Some checks failed

Failing1

2f9f631
Synced from GitHub

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: Repository: thebristolsound/birdbrain/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 870d9345-24f0-4dd5-9fea-de8d54abef65

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

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

Synced from GitHub

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

ReviewStatusCommitReview trigger
📝 Code Review⚠️ Failed 2026-10-17T16:55:00.468727Z051f7ccDraft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Merge conflict resolved at 2f9f6310, a merge commit on top of 051f7cc6, with no force-push.

The base branch feat/1695-egress-setting was rebuilt onto main after #1887 merged, so this branch's copies of its old commits conflicted in 12 files. The resolution takes the base's new head 762d4914 plus 051f7cc6 unchanged: the diff from 762d4914 to 2f9f6310 has the same patch-id as 051f7cc6, and #1896's rebuild changed none of the 12 files this PR touches.

Checked at 2f9f6310: pnpm typecheck exit 0, eslint on the touched source and test files exit 0, and the five touched test files pass (206 passed, 7 skipped). The full suite and build are left to CI at this sha.


Generated by Claude Code

Synced from GitHub

Merge request refused: the base branch is feat/1695-egress-setting, not main

Remove the cause, then apply the merge label again.

Synced from GitHub

Merge request refused: the base branch is feat/1695-egress-setting, not main

Remove the cause, then apply the merge label again.

Synced from GitHub

Merge request refused: the base branch is feat/1695-egress-setting, not main

Remove the cause, then apply the merge label again.

Note

Written by claude-fable-5-1 via Claude Code on behalf of Matt.

Closed without merging. GitHub treats this pull request and the Egress setting (#1896) as a stack, which blocks #1896 from merging and blocks retargeting this one to main. #1896's current head passed its third review, which covered the Wayback-gate behaviour this change addressed. After #1896 merges, anything in this branch that main still lacks is opened as a follow-up against main.