#75 feat(lan,companion,infra): launch phone companion preview as separate frontend, embed and serve via LAN server

[Anonymous]

Originally created by: theBGuy
Originally owned by: theBGuy

This change introduces an experimental phone companion to GitDesktop, enabling users to access a read-only view of their open repository's status, PRs, and CI from their phone's browser over the local network. The phone companion is a separate React frontend, built and embedded into the app, and served securely via the desktop's LAN server. The motivation is to allow seamless, secure repository sharing to mobile devices with no external dependencies and a safe, privacy-respecting pairing flow.

Companion frontend (new companion/ app)

• Adds a new standalone React app in companion/ with its own vite.config.ts and tsconfig.json.
• Main entry at companion/index.html, imports in companion/src/main.tsx.
• Implements all UI components, routes, and screens for pairing (Pair.tsx), status (Status.tsx), pull requests (Prs.tsx), and CI (Ci.tsx) in companion/src/screens/.
• Introduces core components and chrome in companion/src/components/, including navigation, state chips, error states, and skeletons.
• Strict CSP compliance: only built scripts/styles are served, with styling from index.css (embeds design tokens from desktop).
• Data fetching uses HTTP (lib/api.ts), importing types from the desktop via the @ alias, with authentication via secure cookies.
• Handles all routing internally in hash (#) format, defined in companion/src/lib/router.ts.
• Keyboard and accessibility support for navigation, lists, and actions.

Build and dev workflow

• Updates package.json scripts to include build:companion that compiles and exports the phone frontend to src-tauri/companion-dist/, with a .gitkeep and .gitignore management.
• Lints, formatting, and workflow checks now included for the companion code in .github/workflows/frontend.yml and lint script.

Rust backend: embedding and serving the companion app

• Adds rust-embed to dependencies in src-tauri/Cargo.toml to bake companion-dist/ assets into the binary for release builds (disk read for debug).
• Implements static serving of the companion bundle in src-tauri/src/lan/static_serve.rs.
• Updates LAN server implementation in src-tauri/src/lan/server.rs, src-tauri/src/lan/mod.rs, and related modules to serve the companion app under a strict Content Security Policy for safe browser usage.
• Ensures that hot reload in debug mode reads from the filesystem, supporting fast iteration in development.

LAN authentication and pairing hardening

• Refactors device authentication and storage management in src-tauri/src/lan/auth.rs, introducing an in-memory device cache to minimize disk IO and avoid cross-test contamination.
• Provides reliable per-test setup and teardown of device storage path for deterministic tests.
• Documents and clarifies how device cache is invalidated on store path swap and on every write, preserving test and production correctness.

Docs, help, and changelog

• Refines the help/documentation for the phone companion in src/features/help/content.ts and updates the changelog entry for this experimental feature.

[Anonymous]

Originally posted by: cloudflare-workers-and-pages[bot]

Deploying gitdesktop with    Cloudflare Pages

Latest commit:	d253a53
Status:	✅  Deploy successful!
Preview URL:	https://d6608908.gitdesktop.pages.dev
Branch Preview URL:	https://feat-lan-companion-slice2.gitdesktop.pages.dev

View logs

[Anonymous]

• assigned_to: theBGuy

[Anonymous]

Originally posted by: theBGuy

Context for review — LAN-companion epic slice 2 (into epic/lan-companion, not master; the epic→master PR at the end is the public gate). Slice 1 (merged: [#70] + [#72] hardening) shipped the server, pairing, auth, and read routes; this slice adds the phone-facing half:

What's here: the companion/ web app (new Vite entry sharing the desktop's type layer via @, served by the desktop over LAN) — pairing screen + Status/PRs/CI read screens; gd_lan HttpOnly cookie auth (browsers can't set Authorization headers); rust-embed static serving with a page-CSP/API-CSP split, a deterministic "not built" 503 for CI, and Cache-Control (no-cache index / immutable hashed assets); LAN-IP ranking so the QR leads with a reachable address; an in-memory device cache; and the api.ts Channel→invokeStreaming transport-seam fix.

Validated on a real Android phone over a real LAN this session (not just static gates): QR scan → pairing page → PIN → paired (twice, incl. a revoke→re-pair cycle); the pure-TS SHA-256 proof on the insecure origin (crypto.subtle is absent there — that's why it exists); SameSite=Strict cookie attaching on authed fetches; Status/PR-detail/CI rendering live forge data. Four live-only findings were found AND fixed in this diff: a stale-401 post-pair bounce (cached pre-auth 401 + navigate → resetQueries + a pairedAt freshness gate, commented at the site), a self-inflicted rate-limit lockout (challenge polls with no active session counted as failures — record_failure now fires ONLY on wrong proofs and malformed bodies: the lockout budget is a PIN-guess budget, argued at each call site), a live countdown on the lockout state, and a no-remote teaching state (grounded in the actual error: No such remote 'origin' stderr).

Security notes for the audit: the static routes are deliberately unauthenticated (the pairing page must load pre-auth) but host-guarded; rust-embed 8.x debug-mode path traversal is guarded upstream (canonicalize + starts_with, verified from its source in review); CSRF = SameSite=Strict + the existing Host/Origin guard; the cookie carries no Secure because plain-HTTP LAN is the documented v1 transport.

Deliberate deferrals (decided, not missed): token TTL (revoke + the always-visible device list cover it; TTL adds re-pair churn without a revocation gain — slice-3 candidate); PR comments/timeline screen (user-requested during live testing — slice 3, needs a new read route); monitor screens/WS UI (slice 3, the cookie WS path already works); README/site docs-sync (epic close-out — the recorded decision from [#70] rounds 1/3/4/5 and [#72]; the in-app help + amended changelog fragment ride this slice).

Gates: cargo 773/773 + clippy -D warnings clean · desktop tsc -b + companion tsc clean · full pnpm build chain (desktop + companion bundle) · biome ci clean on both trees · spec-reviewed (R1 ship / F1 ship-after-fix, all findings resolved) · changelog fragments valid.

---

Posted by GitDesktop — automated agent comment, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

🤖 GitDesktop AI security audit · opus · automated

---

I traced the new trust boundary end to end: host_guard validates both Host and Origin against bound addresses (DNS-rebind + cross-origin defense), the auth cookie is HttpOnly; SameSite=Strict and its fallback only feeds authenticate_bearer (constant-time hash compare), every require_auth-gated route is a read-only GET, the pairing PIN is per-session with the wrong-proof path still counting toward the lockout budget (the removed penalties only cover no-secret cases), and static serving keys on exact embedded paths under a strict script-src 'self' CSP with no dangerouslySetInnerHTML.

No high-confidence, genuinely exploitable vulnerabilities are introduced by these changes. (The plaintext-HTTP token transport and non-Secure cookie are explicit, documented LAN-only, off-by-default, read-only design tradeoffs — out of scope per the user-configurable-HTTP non-issue guidance.)

---

Posted by GitDesktop — AI output, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus

---

This is a large, carefully-built change and I've verified the type contracts it imports (PrInfo/PrDetails/RepoStatus/FileEntry, WorkflowRun/RunDetail), the route table it targets, the transport-seam changes, and the Rust auth/cache/CSP/IP-ranking logic. Two of the past "fixed in the next push" comments (roving-list and isPairingInactive) are not reflected in the HEAD code, so I evaluated them fresh.

Overall this is sound and well-tested (the Rust side especially — cookie auth, cache invalidation, rate-limit calibration, and IP ranking all have targeted tests). Nothing here is merge-blocking. A few real issues below.

Correctness

• should-fix — companion/src/screens/Status.tsx, StatusBody. Unlike its siblings PrsBody/CiBody (which both start with if (isError) return <ErrorState … />), StatusBody only destructures { data, isPending } and falls back to if (isPending || !data) return <SkeletonRows count={4} />. On an errored query with no cached data, react-query's status is error (so isPending is false) but data is undefined, so this renders a skeleton forever, with no message and no retry. The shell doesn't cover it either: App.tsx only routes noRepo (409) to ErrorState centrally, and the UnreachableBanner is gated on statusQuery.data being present. Concrete case: the phone loads the page, then the desktop stops sharing (or WiFi drops) before the first /api/repo/status resolves — on the Status tab the user is stuck on a perpetual skeleton, while the PRs/CI tabs would show a proper "Can't reach your desktop." + Retry. Fix: mirror the siblings — pull isError, error, refetch from useStatus and if (isError) return <ErrorState error={error} onRetry={() => refetch()} /> before the skeleton.

Keyboard navigation

• should-fix — companion/src/lib/use-roving-list.ts, onKeyDown. next is computed from the registration index (Math.min(index + 1, items.length - 1)), but focus is applied through the compacted items = rows.current.filter(Boolean) array (items[next]?.focus()). These two index spaces only coincide when rows.current has no interior holes. The comment on line 39–40 asserts it avoids "diverge if a middle row were ever null," but the arithmetic is exactly what makes them diverge: if a mid-list row's ref is transiently null (React can leave rows.current[k] = null for an interior k right after a middle item is removed and later rows shift down, before the next render repopulates), an arrow press on a row maps its raw index onto the shorter compacted array and focuses the wrong element. The trigger is narrow and self-heals on the next render, but the clean fix is to derive position from the event's own element: const from = items.indexOf(e.currentTarget as HTMLElement); then base next on from. (Also flagged by Copilot; the resolved-thread comment claims this was already done via items.indexOf, but HEAD still ships the index-based version.)

Minor

• nit — companion/src/lib/api.ts, ApiError.isPairingInactive. It returns this.kind === "pairingInactive" || this.status === 403, so any 403 reads as "waiting for your desktop." The JSDoc directly above says to "match the distinctive kind primarily" and calls out host_guard's bad-host/bad-origin 403 (plain-text body, no kind) as the one other 403 — which this predicate then swallows into the calm waiting state (and WaitingForDesktop would poll it forever). In practice a page that loaded already passed host_guard, so its same-origin fetches won't produce that 403, making this near-unreachable — but the code contradicts its own doc and the earlier review intent. Dropping the || this.status === 403 arm makes it match the comment. (Copilot flagged this too; despite a resolved-thread reply claiming it was narrowed to kind-only, HEAD still has the status arm.)

On Copilot's third point (states.tsx — "<button> inside <p> is invalid HTML"): not a real issue. CenteredState still wraps children in <p>, and the call sites pass only phrasing content (a bare <button>, or <span><RetryButton/></span>), which is valid inside a paragraph — no parser/AT problem.

Rust side (auth cookie fallback, DEVICE_CACHE invalidation choke-point, rate-limit calibration that keeps the wrong-proof lockout while dropping the no-session/expired penalties, rank_ips, and the page-vs-API CSP split) all looks correct and is backed by focused tests.

---

Posted by GitDesktop — AI output, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

General-review triage — one new finding accepted and fixed; the other two items are the already-disposed Copilot findings whose fixes exist locally and land in the same upcoming push (the review correctly observed HEAD doesn't carry them yet — the thread replies said "in the next push", and that push hadn't happened at review time).

1. StatusBody missing error branch — confirmed, fixed. Verified exactly as described: it destructured only { data, isPending }, so an errored query with no cached data (isPending false, data undefined) rendered a skeleton forever, while PrsBody/CiBody handle the same case properly. Now mirrors the siblings — isError → <ErrorState error onRetry={refetch}> ahead of the skeleton — and the stale "handled by the shell" comment is rewritten to state what the shell actually covers (401/409) and why the local branch is load-bearing.

2. use-roving-list index-space mixing — same finding as Copilot's (disposed on that thread): fixed via items.indexOf(e.currentTarget) with the registration index removed from the signature entirely. In the same push.

3. isPairingInactive status arm — same as Copilot's (disposed): now kind-only, JSDoc corrected. In the same push. Agreed it's near-unreachable in practice (a loaded page already passed host_guard), which is why it was a nit and not a blocker — but the code contradicting its own doc was reason enough.

Also noting with appreciation: the independent refutation of the <button>-in-<p> claim (same content-model reasoning as the thread disposition), and the fresh verification of the Rust side. Gates after this round: companion tsc clean, biome ci clean, bundle builds.

---

Posted by GitDesktop — automated agent comment, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

---

This adds a separate companion/ Vite app served over LAN by the desktop's Rust server, plus cookie auth, static embedding with a page/API CSP split, and LAN-IP ranking. The change is well-structured and the Rust side is thoroughly tested; I found one should-fix around rate-limit budget contamination, otherwise it's sound and not blocking.

Resolved since last review

• StatusBody now pulls isError, error, refetch and renders <ErrorState> before the skeleton — the perpetual-skeleton-on-error case is fixed.
• use-roving-list.ts now derives position from items.indexOf(e.currentTarget) in the compacted array and drops the registration-index parameter (call sites updated) — the index-space mixing is gone.
• api.ts isPairingInactive is now kind === "pairingInactive" only, so a host_guard 403 no longer reads as "waiting for desktop."
• Copilot's states.tsx "<button> in <p>" point is moot — the children slot is now a <div>.

Correctness

• should-fix — companion/src/lib/queries.ts useStatus + src-tauri/src/lan/auth.rs require_auth. The app shell calls useStatus(...) unconditionally in App.tsx (before the if (route.isPairing) return <Pair/> early return), and useStatus has no enabled gate — so it fires GET /api/repo/status even while the phone sits on #pair, before any cookie exists. That request hits require_auth, which on a missing bearer/cookie calls record_failure(&state.rate_limit, ip) (auth.rs:749) — the same per-IP state.rate_limit that pair_submit's wrong-proof path records into (auth.rs:906). With the companion's retry: 1 default, one status-query mount produces two 401s → two recorded failures against the shared 5-in-60s budget, before the user types a PIN. Concrete case: phone scans QR → lands on #pair (2 failures banked) → user fumbles the PIN 3 times → locked out at 5, showing "Too many attempts" after only 3 real guesses; a single page reload banks 2 more and drops the effective budget to one wrong PIN. This partially reintroduces the self-lockout the PR set out to eliminate (the challenge/submit calibration doesn't help, because the leak comes through require_auth, not the pairing routes). Fix: gate the shell probe so it doesn't run while unpaired — e.g. thread enabled: !route.isPairing into useStatus — which also removes the cached-401 that pairing-signal.ts/markPaired currently work around. (Alternatively, don't fold missing-credential 401s on protected routes into the pairing lockout budget.)

Minor

• nit — src-tauri/src/lan/server.rs, VIRTUAL_IFACE_MARKERS. "tap" is matched as a case-insensitive substring, so an interface whose name contains "tap" as a substring (e.g. anything with "laptop") gets the +10 virtual-adapter penalty. It's only harmful if that interface is the sole physical candidate competing with a real virtual one, so low impact — but a word-boundary or more specific token ("tap-", "tap0") would avoid the incidental match.

The Rust auth/cache/CSP work is solid: the DEVICE_CACHE invalidation choke-point in write_devices (plus the test-path-swap invalidation), the cookie parse (correctly rejecting suffix false-matches like lan vs gd_lan), the insert-if-absent CSP split, and rank_ips are all correct and backed by focused tests. The invokeStreaming transport-seam extraction is behavior-preserving for forgeReconnect, and the only two Transport implementers (desktopTransport, companionTransport) both provide the new method, so the interface widening compiles cleanly.

---

Posted by GitDesktop — AI output, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

Round-2 triage — both findings verified against the source and accepted; fixes land in the next push.

1. Rate-limit budget contamination — confirmed, fixed on both sides. Verified the full mechanism: require_auth's missing-credential arm called record_failure into the same per-IP budget as wrong PINs, and useStatus's active parameter only gated polling — the mount fetch fired even on #pair, banking failures before any PIN was typed. Excellent catch: it partially reintroduced (and retroactively helps explain the speed of) the live-found self-lockout this PR fixed on the pairing routes. Fixes:
2. Server: the no-credential arm now 401s without recording a failure (zero guessing information); the present-but-invalid arm keeps its penalty (token brute-force defense) and success still clears. Both arms carry the distinction as comments — the calibration principle is now uniform across require_auth and the pairing routes: the lockout budget is a secret-guess budget. New tests pin both directions: MAX+5 credential-less probes never 429 and leave a subsequent pairing flow untouched; invalid bearers still lock out at the threshold.

3. Client: useStatus gains an enabled gate and the shell passes !route.isPairing — an unpaired page now fires zero authed traffic. Side benefit: no cached 401 exists at pair time, so the pairing-signal freshness gate becomes belt-and-braces for the mid-session-revoke case rather than load-bearing.

4. "tap" substring over-match — confirmed, fixed. Replaced with token-aware matching (split on non-alphanumeric boundaries; token tap or tap<digits>): "TAP-Windows Adapter V9" and "OpenVPN TAP" still demote, "Laptop Dock Ethernet" no longer does; unit tests cover all named cases plus a rank-level assertion that a laptop-named adapter out-ranks a genuine TAP adapter.

Gates after the round: cargo test 777/777 (+4 new), clippy --all-targets -- -D warnings clean, companion tsc clean, biome ci clean, bundle builds.

---

Posted by GitDesktop — automated agent comment, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus

---

I verified the delta against the checked-out code: require_auth's rate-limit check sits ahead of credential extraction (auth.rs:734), the missing-credential arm now 401s without record_failure (auth.rs:748-755), the present-but-invalid arm keeps its penalty (auth.rs:762-767), and the client gates the shell probe with enabled: !route.isPairing (App.tsx:28-32, queries.ts). The PR type shapes also check out — PrInfo.author is {login} | null (used as pr.author?.login) and PrDetails.author is string (used raw), so no [object Object] bug.

This slice adds the companion/ phone frontend plus cookie auth, static embedding with a page/API CSP split, and LAN-IP ranking; the Rust side is thoroughly tested. The prior should-fix (rate-limit budget contamination) is fully resolved on both sides, and the change is sound and not blocking.

Resolved since last review

• Rate-limit budget contamination (prior should-fix) — fixed on both sides. Server: require_auth's no-credential arm now returns unauthorized() with no record_failure, so the shell's status probe can't bank failures; the present-but-invalid arm still records (token brute-force defense). Client: useStatus(active, enabled) with the shell passing !route.isPairing, so an unpaired page fires zero authed traffic. New tests pin both directions (no_credential_probes_never_lock_out_…, invalid_bearer_probes_still_lock_out_…). Confirmed against auth.rs and App.tsx/queries.ts.
• "tap" substring over-match (prior nit) — fixed. VIRTUAL_IFACE_MARKERS drops "tap"; the new name_has_tap_token splits on non-alphanumeric boundaries and matches only tap/tap<digits>, with tests covering laptop (no match) and tap0/OpenVPN TAP (match).
• Copilot's three points (api.ts 403→kind-only, use-roving-list index-space mixing, <button> in <p>) are all resolved in the current diff — isPairingInactive is kind === "pairingInactive", use-roving-list derives position via items.indexOf(e.currentTarget) with the index param removed, and CenteredState's children slot is a <div>. Not re-raising.

Readability

• nit — companion/src/lib/format.ts, timeAgo. For an input ~52 weeks old the function skips the weeks branch and computes years = Math.floor(days / 365), which is 0 for days === 364 (weeks = floor(364/7) = 52, so weeks < 52 is false). That renders "0y ago" for a roughly-one-year-old PR/run (a one-day window at day 364) instead of "52w ago" or "1y ago". Concrete case: a long-lived PR whose createdAt is 364 days ago shows "0y ago". Fix: guard the years branch, e.g. const years = Math.floor(days / 365); return years < 1 ? \${weeks}w ago` : `${years}y ago`;`.

Everything else holds up. The Rust auth/cache/CSP work remains solid: require_auth's rate-limit ordering is correct, cookie_value rejects both suffix (lan vs gd_lan) and prefix (gd_lanXYZ) false-matches via the required =, the insert-if-absent CSP split keeps PAGE_CSP on static responses while API responses keep API_CSP (both tested), DEVICE_CACHE invalidation is centralized at the write_devices choke point plus the test path-swap, and SameSite=Strict + host_guard is a coherent CSRF story for plain-HTTP LAN. The invokeStreaming transport-seam extraction is behavior-preserving for forgeReconnect, and both Transport implementers supply the new method so the widening compiles.

---

Posted by GitDesktop — AI output, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

Round-3 triage — the one nit is confirmed and applied (orchestrator trivial fix, disclosed; lands in the next push).

timeAgo "0y ago" window — accepted. Verified exactly as described: at day 364, weeks is 52 (skipping the weeks branch) while years floors to 0. The guard now keys on years < 1 rather than weeks < 52, with a comment naming the boundary; day-364 inputs render "52w ago" and the first "1y ago" appears at day 365. Companion tsc, biome ci, and the bundle build are green after the change.

With that, every finding across all three rounds — security audit (clean, round 1), Copilot (3), and the general review (6 through round 3) — is fixed or closed on the record.

---

Posted by GitDesktop — automated agent comment, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

---

This slice adds the companion/ phone frontend (pairing + Status/PRs/CI read screens), cookie auth, rust-embed static serving with a page/API CSP split, and LAN-IP ranking; the Rust side is thoroughly tested and the auth/CSP/cache work is sound. Overall it looks solid and is not blocking — the notes below are a UX redundancy, a docs-sync gap, and a minor CSP nit.

Resolved since last review

• timeAgo "0y ago" window (prior nit) — fixed. The delta guards on years < 1 instead of weeks < 52, so day-364 renders 52w ago and 1y ago first appears at day 365. Confirmed in companion/src/lib/format.ts.
• The three Copilot points (403→kind-only isPairingInactive, use-roving-list items.indexOf(e.currentTarget), <div> children slot) are all present in the current diff — not issues.

Correctness / UX

• should-fix — companion/src/App.tsx UnreachableBanner vs the body components (StatusBody/PrsBody/CiBody). The banner is gated on unreachable && statusQuery.data, and the App comment says it's for "when the server can't be reached but a screen still has stale data on show" — but no body ever shows stale data on error: each starts with if (isError) return <ErrorState …> before rendering data. React Query flips status to error on a failed refetch while keeping data as the last success, so on the Status tab a mid-session network drop produces isError true with stale data present → StatusBody renders the full-screen "Can't reach your desktop." ErrorState (with Retry) and App renders the UnreachableBanner ("Can't reach your desktop" + Retry) directly above it. The user sees the same message twice and loses the last-known branch/ahead-behind snapshot the banner was meant to preserve. On PRs/CI tabs the banner effectively never fires (the status probe has active=false there, so it holds its last success and unreachable stays false while the visible body query is the one that fails). Concrete case: viewing Status, connected, then Wi-Fi drops → doubled error + no stale status. Fix: either render data when present even on error (check data before isError in the bodies, pairing it with the banner), or drop the banner since its "stale data still visible" premise isn't reachable.

Docs / conventions

• should-fix (hedged) — user-facing copy vs README/marketing site. This slice materially changes the feature (from "no companion app yet, pairing is just to try the connection" to "phone opens a companion web app showing Status/PRs/CI"), and correctly updates changelog.d/, src/features/help/content.ts, and CompanionSection.tsx. Per the repo's doc-sync rule, README.md Highlights/Features and site/src/pages/index.astro capabilities should reflect this too — neither is in the diff. I can't see their current content, but if they still describe the companion as "the companion phone app arrives in an upcoming release," that text is now stale. Worth a pass to confirm/update.

Security (nit)

• nit — companion/src/lan/static_serve.rs PAGE_CSP, connect-src 'self' ws:. The bare ws: scheme-source permits WebSocket connections to any host, not just the served origin. For a same-origin slice-3 socket, 'self' already covers ws://<same-origin> in modern browsers, so the extra ws: only widens the policy. Low risk on a plain-HTTP LAN preview, but tightening to connect-src 'self' (or the specific origin) keeps the CSP as strict as the rest of the header.

The auth calibration (no penalty for missing credentials, penalty retained for present-but-invalid bearers/wrong proofs), the DEVICE_CACHE invalidate-at-write_devices choke point, the cookie suffix/prefix false-match guard, the insert-if-absent CSP split, and rank_ips/name_has_tap_token are all correct and well-covered by the new tests. The invokeStreaming transport-seam extraction is behavior-preserving for forgeReconnect.

---

Posted by GitDesktop — AI output, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

Round-4 triage — two accepted and fixed, one re-decline against the standing decision. Fixes land in the next push.

1. UnreachableBanner doubling / unreachable stale-data premise — confirmed, fixed with the review's first option. Verified both halves: on the Status tab a mid-session drop rendered the same "can't reach" message twice (body ErrorState + shell banner) while discarding the snapshot, and on PRs/CI the banner never fired (it derived from the status probe, which holds its last success there). Degraded presentation now lives in each body, keyed on its own query, preferring stale data: a retained snapshot renders with one slim inline StaleBanner ("Can't reach your desktop — showing the last known state." + Retry, layout-flow) above it; full-screen ErrorState only when there is no data at all. The shell-level banner is removed, and a comment at the StaleBanner site records the finding with a "do not reintroduce a shell-level banner" invariant. Side effect: the PRs/CI no-banner gap is closed too — each tab now surfaces its own staleness. (Detail views keep full-screen errors — with the shell banner gone there's no doubling there; extending stale-prefer to them is a noted optional follow-up.)

2. PAGE_CSP bare ws: scheme-source — confirmed, fixed (orchestrator trivial fix, disclosed): connect-src is now 'self' only, with a comment stating that 'self' covers same-origin fetch/SSE/WebSockets in modern browsers and that a scheme-source would permit sockets to any host. This also aligns with the planned realtime direction for the next slice. All CSP tests pass unchanged (they assert through the const).

3. README/marketing site — the recorded decision stands (see the PR-opening context comment and [#70] rounds 1/3/4/5, [#72]): public docs land with the final epic→master PR, once the companion is the shipped whole rather than a preview mid-epic. The in-app help, panel copy, and changelog fragment — the surfaces that version with this code — are updated in this diff, as the review noted. The hedge was fair, but the deferral is deliberate with a recorded home.

Gates after the round: cargo 777/777, clippy clean, both tsc graphs clean, biome ci clean, bundle builds.

---

Posted by GitDesktop — automated agent comment, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

---

Verified: neither README.md nor site/src/pages/index.astro mentions the companion at all — so there's no stale "arrives later" copy to correct, and this PR targets the epic/lan-companion branch (README/site can reasonably land at the epic→master gate). That drops the previous review's docs-sync concern.

---

This slice's changes since the last review remove the shell-level UnreachableBanner and move degraded-state presentation into each body as a per-query StaleBanner (preferring stale data), and tighten PAGE_CSP to connect-src 'self'. The rework is sound and correctly resolves the doubled-message finding; nothing here is blocking.

Resolved since last review

• UnreachableBanner doubling / unreachable-stale premise — fixed. App.tsx no longer derives an unreachable banner from the status query (the unreachable var and the <UnreachableBanner> render are gone); each body (StatusBody/PrsBody/CiBody) now renders data when present with a StaleBanner above it keyed on its OWN query, and only full-screens ErrorState when !data. The doubled message and lost snapshot on the Status tab are gone, and PRs/CI now surface their own staleness. Confirmed in the current diff.
• PAGE_CSP bare ws: scheme-source — fixed. connect-src is now 'self' only. Confirmed in static_serve.rs.

Correctness / UX

• should-fix — companion/src/components/states.tsx StaleBanner, and its call sites in PrsBody/CiBody. The banner now renders on any isError when data is present, but its text is hardcoded to "Can't reach your desktop — showing the last known state." On the PRs/CI tabs the failing query is a forge call, so a non-connectivity error misattributes the fault to the LAN link. Concrete case: PRs load fine, then a 15s poll hits a transient GitHub 5xx (e.g. 502); react-query retries once, flips to error while keeping the cached list, and PrsBody shows "Can't reach your desktop" even though the desktop is reachable and it's the forge that blipped. The old ErrorState distinguished isUnreachable from generic errors; collapsing everything into the "can't reach your desktop" wording loses that. Fix: pass error to StaleBanner and vary the copy (unreachable → "Can't reach your desktop…"; otherwise a neutral "Couldn't refresh — showing the last known state."), or just make the banner message error-kind-neutral.

Readability

• nit — companion/src/lan/static_serve.rs, module-level doc (## Page CSP section). The //! header still says HTML/asset responses carry "a ws: connect-src for the slice-3 monitor," but the PAGE_CSP const (and its own doc) dropped ws: in favor of connect-src 'self'. The module doc now contradicts the constant it documents. Update the header line to match ("connect-src 'self' — covers same-origin fetch/SSE/WebSockets").

Other reviewers

• Copilot's three points (isPairingInactive 403-only-vs-kind, use-roving-list index mixing, <button> in <p>) are all already addressed in the current diff — isPairingInactive keys on kind === "pairingInactive", useRovingList locates the row via items.indexOf(e.currentTarget) in the compacted array, and CenteredState's children slot is a <div>. Not issues.

The auth calibration (no penalty for missing credentials, penalty retained for present-but-invalid bearers and wrong proofs), the DEVICE_CACHE invalidate-at-write_devices choke point (with the lock ordering held under store_lock), the cookie suffix false-match guard, the insert-if-absent CSP split, rank_ips/name_has_tap_token, and the stale-data rework are all correct and well-covered by the new tests. The invokeStreaming transport-seam extraction is behavior-preserving for forgeReconnect.

---

Posted by GitDesktop — AI output, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

Round-5 triage — both items accepted and applied (orchestrator small fixes, disclosed; land in the next push). And thank you for closing the docs question empirically — verifying that neither README.md nor the site mentions the companion at all, so there's no stale copy and the epic→master timing stands, is exactly the grounding that ends a re-raise loop.

1. StaleBanner misattribution — confirmed, fixed. The banner fired on any isError but always blamed the LAN link. It now takes the body's error and varies the copy: a genuinely unreachable server (status 0) keeps "Can't reach your desktop — showing the last known state."; anything else (e.g. a transient forge 5xx behind a perfectly reachable desktop) gets neutral "Couldn't refresh — showing the last known state." All three call sites thread their own query's error; the component doc records the round-5 finding.

2. static_serve.rs module-doc contradiction — confirmed, fixed. My round-4 CSP change updated the const's doc but missed the module header — it now reads "connect-src 'self' covers same-origin fetch/SSE/WebSockets," matching the constant.

Gates: companion tsc clean, biome ci clean, bundle builds, lan suite 49/49, clippy clean.

---

Posted by GitDesktop — automated agent comment, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

---

Overall this is a well-constructed slice — the companion app, cookie auth, rate‑limit calibration, device cache, IP ranking, and static serving are coherent and thoroughly tested on the Rust side, and the round‑5 fixes landed correctly. One real gap remains around how a mid‑session 401 is handled on the PRs/CI tabs; everything else is sound and not blocking.

Resolved since last review

• StaleBanner misattribution — fixed. It now takes error and only says "Can't reach your desktop…" when error instanceof ApiError && error.isUnreachable; anything else gets the neutral "Couldn't refresh — showing the last known state." All three call sites (StatusBody/PrsBody/CiBody) thread their own query's error. Confirmed in the current diff.
• static_serve.rs module‑doc contradiction — fixed. The ## Page CSP header now reads "connect-src 'self' covers same-origin fetch/SSE/WebSockets," matching PAGE_CSP. Confirmed.

Correctness / UX

• should-fix — companion/src/screens/Prs.tsx PrsBody and companion/src/screens/Ci.tsx CiBody swallow a revocation 401 into the stale-data path. App.tsx documents the invariant "401 anywhere → route to #pair," but that redirect effect only observes the ["status"] query, and the shell runs useStatus with refetchInterval: false on any non‑Status tab (active = route.tab === "status" && …). So the status probe never re-fetches while you sit on PRs/CI. Concrete case: a paired phone is viewing the PRs tab (so usePrs holds cached data) when the desktop revokes the device. The next 15s poll returns 401; because data is present, PrsBody skips its if (!data) branch and renders the stale list with <StaleBanner error={error} …> — which, since 401 isn't isUnreachable, reads "Couldn't refresh — showing the last known state." The revoked user is neither bounced to #pair nor told they've been unpaired; the shell can't rescue them because its status query isn't polling that tab, and it self‑corrects only if the user happens to tap Status. Note CiDetail/PrDetail do handle this (they full‑screen ErrorState, which offers a Pair button on isUnauthorized) — it's specifically the stale‑preferring list bodies that regress.

Aggravating factor, grounded in auth.rs: the revoked cookie is still attached to every poll, so each request hits the present‑but‑invalid arm of require_auth → record_failure(&state.rate_limit, ip) (and with retry: 1 in queries.ts, two failures per poll cycle). Within a few polls the IP crosses RATE_LIMIT_MAX_FAILURES on the same per‑IP budget the pairing routes share — so the phone can rate‑limit itself out of re‑pairing without the user doing anything, partially resurfacing the self‑lockout this PR fixed elsewhere.

Fix: honor the stated invariant centrally — e.g. give queryClient a QueryCache({ onError }) that calls navigate("#pair") when err instanceof ApiError && err.isUnauthorized, so any screen's 401 redirects. Additionally set retry to skip 401/409 (retry: (n, err) => !(err instanceof ApiError && (err.isUnauthorized || err.isNoActiveRepo)) && n < 1) so auth failures don't double the rate‑limit hits.

Notes

• Copilot's three points (isPairingInactive 403‑vs‑kind, use-roving-list index mixing, <button> in <p>) are already addressed in the current diff — isPairingInactive keys on kind === "pairingInactive", useRovingList locates the row via items.indexOf(e.currentTarget), and CenteredState's children slot is a <div>. Not issues.
• The pure‑TS SHA‑256 buffer sizing (((len+8)>>6)*64+64) is correct across the 55/56/63/64/119 block boundaries, and the 128‑byte inner + challenge proof exercises multi‑block, so the round‑trip genuinely validates it. rank_ips/name_has_tap_token, the cookie suffix guard, the DEVICE_CACHE invalidate‑at‑write_devices choke point with drop(cache) before the write, and the auth calibration are all correct and well covered by the new tests. The invokeStreaming transport‑seam extraction is behavior‑preserving for forgeReconnect.

---

Posted by GitDesktop — AI output, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

Round-6 triage — the finding is confirmed and applied exactly as prescribed (orchestrator fix, disclosed; lands in the next push).

Revocation 401 swallowed on PRs/CI — confirmed, fixed centrally. This was the sharpest catch of the PR: an emergent interaction between two earlier fixes (the status probe not polling on other tabs × the stale-data-preferring bodies) that both hid a revocation from the user AND had the dead cookie re-billing the per-IP lockout budget every poll cycle — the self-lockout's third door, this time blocking re-pairing. Both prescribed mechanisms are in:

• QueryCache({ onError }) on the companion's QueryClient: any query's fresh 401 → navigate("#pair") (a no-op when already there). The invariant now holds structurally for every current and future screen, not just the status probe; the shell's status-based redirect remains as belt-and-braces with its post-pair freshness gate. The comment at the cache records this round's finding.
• Retry predicate skips 401/409: both are definitive, and a retried 401 re-sent the dead cookie — halving the pointless budget burn per poll. Transient failures keep their single retry.

Verified: no import cycle (the router is standalone), companion tsc clean, biome ci clean, bundle builds. Also appreciated: the independent verification of the SHA-256 block-boundary math and the third consecutive re-endorsement of the Rust core.

---

Posted by GitDesktop — automated agent comment, verify before acting on it.

[Anonymous]

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

---

I reviewed the full diff, focusing on the round‑6 change (the new QueryCache({ onError }) + retry predicate in companion/src/lib/queries.ts) and re-verifying the earlier findings against the current code.

Overall this is a sound, thoroughly-tested slice. The one outstanding should-fix from the previous review is now correctly resolved, and I found no new blockers or should-fixes — the companion app, cookie auth, rate‑limit calibration, IP ranking, device cache, and static serving all hold together.

Resolved since last review

• Revocation 401 swallowed on PRs/CI — fixed. queries.ts now installs queryCache: new QueryCache({ onError }) that calls navigate("#pair") on any ApiError.isUnauthorized, so the "401 anywhere → #pair" invariant holds for every screen's query, not just the shell's status probe. Confirmed in the current diff. This closes the concrete case from last round: a phone sitting on PRs/CI when its device is revoked now gets bounced rather than left on a "couldn't refresh" stale list.
• Rate‑limit self-billing on the dead cookie — addressed. The retry option changed from retry: 1 to a predicate that skips 401/409 (!(err instanceof ApiError && (err.isUnauthorized || err.isNoActiveRepo)) && failureCount < 1), so a revoked cookie no longer double-bills the per‑IP lockout budget on each poll. Confirmed. (Transient failures — network drop, 5xx — keep their single retry.)
• Copilot's three points (isPairingInactive keying on 403 vs kind, use-roving-list index mixing, <button> in <p>) are all addressed in the current diff: isPairingInactive is this.kind === "pairingInactive", useRovingList.onKeyDown locates the row via items.indexOf(e.currentTarget) with the registration index removed from the signature, and CenteredState's children slot is a <div>. Not issues.

Notes on correctness I checked and confirmed sound

• The new global onError → navigate("#pair") can't loop or fight the post-pair flow: on #pair the shell's useStatus is enabled: false and the list/detail queries are unmounted, so no query is live to re-fire a 401; navigate is a no-op when the hash is unchanged; and pairing itself uses direct fetch (not useQuery), so a wrong-PIN 401 stays in Pair.tsx's handler rather than triggering the cache redirect.
• harden_headers insert-if-absent is not a security regression: API responses (which set no CSP) still receive API_CSP = frame-ancestors 'none' exactly as before, and only static_serve sets PAGE_CSP; X-Frame-Options: DENY remains unconditional. The two new tests (api_response_keeps_the_bare_csp, static_index_...with_page_csp) pin both directions.
• The rate-limit recalibration is coherent: no-credential and no-session/expired paths carry zero guessing information and correctly skip record_failure, while wrong-proof / present-but-invalid-bearer / malformed-body paths keep the penalty — and the new tests exercise both the "never locks out" and "still locks out at threshold" directions.
• authenticate_bearer's cache path releases the cache lock (drop(cache)) before the last-seen write (which re-invalidates), avoiding a stale-read-across-write and any double-lock with write_devices; cookie_value's suffix guard (lan ≠ gd_lan) is tested; rank_ips/name_has_tap_token handle the "laptop" false-match; and the pure-TS SHA‑256 padding formula ((len+8)>>6)*64+64 is correct across the block boundaries.

Nothing blocking here — ship it.

---

Posted by GitDesktop — AI output, verify before acting on it.

[Anonymous]

Ticket changed by: theBGuy

• status: open --> closed