GitDesktop / Tickets / #70 feat(lan,settings,git): add phone companion preview for LAN repo sharing

Anonymous - 2026-07-17

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

Deploying gitdesktop with Cloudflare Pages

Latest commit: 84548fc

Status: ✅ Deploy successful!

Preview URL: https://d7541062.gitdesktop.pages.dev

Branch Preview URL: https://feat-lan-phone-companion-pre.gitdesktop.pages.dev

View logs

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

assigned_to: theBGuy
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

Originally posted by: theBGuy

🤖 GitDesktop AI security audit · opus · automated

Finding

Severity: High — Confidence: 8/10

path-traversal / arbitrary file read — src-tauri/src/lan/routes/git.rs (diff_file route) exposing crate::git::diff::git_diff_file (src-tauri/src/git/diff.rs:281).

The new LAN route GET /api/repo/diff/file?path=…&staged=…&untracked=… passes the client-supplied path and untracked flag straight into git_diff_file with no validation:

respond(crate::git::diff::git_diff_file(repo, q.path, q.staged, q.untracked).await)

When untracked=true, git_diff_file runs:

run_git_raw(Some(&repo_path), &["diff", "--no-index", "--", "/dev/null", &file_path], …)

git diff --no-index is not confined to the repository — it compares two literal filesystem paths and returns the full contents of the second one as an "added" diff. Because file_path is attacker-controlled, an absolute or ../-relative path escapes the repo entirely. The response body (FileDiff.text) contains the file's contents (up to VIEWER_MAX_BYTES).

Exploit scenario: A device that has paired (the feature explicitly anticipates hostile devices on a coffee-shop/roommate LAN, and pairing only requires a 6-digit PIN that can be shoulder-surfed, or the future companion app may itself be compromised) holds a valid bearer token. It then requests, on Windows, GET /api/repo/diff/file?path=C:/Users/Evan/.ssh/id_rsa&untracked=true (or path=/etc/passwd&untracked=true, path=../../../.aws/credentials&untracked=true, etc.). The require_auth + host_guard layers all pass for a paired device, so the handler runs git diff --no-index -- /dev/null <that path> and returns the file's contents. This escalates the granted authority far beyond the advertised "read-only access to the open repo" (docs/changelog/UI all promise repo-only read access) to arbitrary read of any file the desktop process can open — SSH keys, cloud credentials, tokens. I verified the mechanism against the code path (route → untracked branch → --no-index) and git's documented --no-index semantics; the other branch (untracked=false) uses a repo-scoped pathspec after --, which git restricts to the working tree, so the vector is specifically the --no-index branch. git_commit_details/git_commit_diff are guarded by validate_hash (hex-only) and are not affected.

Remediation: Before invoking git_diff_file from the LAN route, reject the request unless path canonicalizes to a location inside the active repo (resolve against repo_path, verify the canonical path is a descendant, reject absolute paths and ..), or simply drop the untracked / --no-index capability from the LAN surface so only repo-tracked pathspecs are diffable.

Other areas checked and cleared: pairing is a PIN-gated SHA-256 challenge/response with the PIN never on the wire, CSPRNG-backed PIN/token/challenge, constant-time hash comparison, per-IP rate limiting, and a Host/Origin allowlist (DNS-rebind defense); the WebSocket monitor is read-only (inbound frames drained); the QR dangerouslySetInnerHTML renders server-generated SVG encoding a self-derived LAN URL (no untrusted input); forge routes take typed u64 ids and array-arg CLI calls (no injection).

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

I've reviewed the LAN companion feature end-to-end against the actual source. Here's my assessment.

Summary

This adds an opt-in, default-off embedded axum server that shares the open repo read-only with a paired phone over the LAN, with a solid security model (PIN-gated challenge/response pairing, per-device bearer tokens stored only as hashes, host/origin DNS-rebind guard, per-IP rate limiting, hardening headers) and good unit + router-integration test coverage. The architecture is sound and I found no hard blocker, but there are a couple of genuine correctness/UX gaps worth fixing before this is dogfood-ready, plus a repo-convention docs gap.

Correctness

should-fix — src/App.tsx active-repo effect (useEffect(… , [repoPath])) never clears the shared repo on close. The effect is if (!repoPath) return; invoke("lan_set_active_repo", { repoPath }). closeRepo() in ui.ts sets repoPath: null, so the early-return fires and the backend active_repo keeps its last value. Concrete path: open repo A (pushed to backend) → Close repo (back to the welcome screen, repoPath = null) → the LAN server still answers /api/repo/* with repo A's contents, and the "Sharing on" banner is still shown even though no repo is open. This contradicts the feature's own promise ("share the repository that's currently open") and is a privacy leak to any already-paired device. Fix: add a lan_clear_active_repo command (set active_repo to None, so the routes 409 via repo_or_409!) and call it in the else branch — e.g. if (!repoPath) { invoke("lan_clear_active_repo").catch(()=>{}); return; }.

should-fix — src-tauri/src/lan/server.rs resolve_ips + src-tauri/src/lan/mod.rs lan_pairing_start: the pairing QR encodes the numerically-smallest interface IP, which is often unreachable. resolve_ips does ips.sort() over every non-loopback IPv4, and lan_pairing_start builds the QR from rs.urls.first() (i.e. the smallest IP). On a common dev machine with Docker (172.17.0.1) or a VPN (10.x), those sort ahead of the real LAN address (192.168.x), so the primary pairing path — scanning the QR — points the phone at an address it can't reach. The text URL list is a fallback, but the headline UX is the QR. Consider filtering to private-LAN ranges / preferring 192.168.* and de-prioritizing known virtual bridges, or letting the user pick which advertised URL the QR encodes.

Edge cases

should-fix — src/features/settings/PairDeviceDialog.tsx newly-paired detection (lines ~117-124). The detector accepts any device whose createdAt >= startTimeRef.current - 5000. If the user pairs a device and then reopens the dialog within ~5s (or pairs two devices back-to-back), a pre-existing device falls inside the 5s slack window and is reported as "just paired" — showing the wrong device name and a spurious success with no actual pairing having occurred in the new session. Also .find() returns the first array match, not the newest. Tightening the match (e.g. compare against a device-id snapshot taken at begin(), or track the max known createdAt) would remove the false positive. Low-frequency, but it's the confirmation the whole flow hinges on.

Docs / conventions

should-fix — missing README + marketing-site updates. The repo guide (CLAUDE.md) requires that a user-facing feature update README.md (Highlights/Features) and site/src/pages/index.astro (capabilities, non-AI → both views) in the same change. The diff adds the changelog fragment, the in-app help section, and the palette action (good), but neither README.md nor the marketing site appears in the changed files. Add the capability line at minimum.

What's solid

The security layering is well done and well tested: require_auth sits inside the protected subtree and host_guard wraps everything (verified the WS upgrade is bearer- and host-gated by review_routes_are_bearer_gated / review_routes_are_host_guarded); tokens are stored only as sha256 hashes with constant-time comparison; the store file is never clobbered on parse failure; the StreamGuard Drop cleanly bounds registry entry lifetime across every exit path; and the pairing brute-force surface is bounded by the 120s TTL × 5-per-60s rate limit against a 6-digit PIN. No concerns there.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

Originally posted by: theBGuy

Round-1 triage — every finding verified against the source before disposition. Fixes described below land in the next push.

Security audit — path traversal via diff/file + untracked=true: confirmed, fixed

Verified the exact chain: lan/routes/git.rs::diff_file → git_diff_file untracked branch → git diff --no-index -- /dev/null <path>, which is not repo-confined. Fix (validation, not capability removal — the companion legitimately needs untracked-file diffs):

Lexical guard on both branches: the client path must be non-empty and built only of Normal/CurDir components — any RootDir (leading /), Prefix (Windows drive), or ParentDir (..) → HTTP 400.

On-disk containment for the --no-index branch: canonicalize(repo.join(path)) must be a descendant of the canonicalized repo root — this also closes the untracked-symlink-pointing-outside-the-repo case a lexical check can't.

An INVARIANT comment now sits at the --no-index call site in git/diff.rs stating it is not repo-confined and untrusted callers must validate containment first.

Router-integration test (diff_file_rejects_paths_escaping_the_repo): against a real temp repo with a seeded paired device, ../x, /etc/passwd, and (on Windows) C:/x all → 400; a legitimate untracked file → 200 with its content in the diff body.

General review

Active repo never cleared on close — confirmed, fixed (converged with Copilot's inline thread): lan_set_active_repo widened to Option<String> (None clears → routes 409), App effect always pushes repoPath ?? null. Chose widening over a separate lan_clear_active_repo command — one command, same effect. Unit test added.

QR encodes the numerically-smallest IP — confirmed, deferred to epic slice 2. This is real (observed live during the pre-PR E2E pass: a VPN 10.x address ranked ahead of the machine's 192.168.x). The epic's slice-2 scope already includes ranking real private-LAN addresses above VPN/virtual-bridge/link-local in both the URL list and the QR; fixing it properly belongs there with the companion bundle it serves.

Pairing-dialog 5s slack — confirmed, fixed. The slack is gone entirely: createdAt is minted by our own Rust at millisecond precision on the same machine clock as Date.now(), and the session start is marked before the offer is requested, so createdAt >= startTime holds strictly for any device minted in the session — reopening within 5s of a prior pair can no longer match the old device. Detection now also picks the newest match rather than .find() first-match. (The suggested id-snapshot-at-begin() alternative has the flaw the original comment documented — the device query may not have resolved at open, leaving an empty snapshot — so the timestamp gate stays, minus the slack.)

README + marketing site — deferred by the epic-branch workflow. This PR targets epic/lan-companion, not master; public docs land with the final epic→master PR. The in-app help + changelog fragment ride this slice because they version with the preview surface itself.

Gates after the fix round

cargo test 749/749, cargo clippy --all-targets -- -D warnings clean, tsc -b clean, pnpm build green.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Summary

This adds an opt-in, default-off embedded axum server that shares the currently-open repo read-only with a paired phone over the LAN. The security model is solid (PIN-gated challenge/response pairing, per-device bearer tokens stored only as hashes, per-IP rate limiting, Host/Origin DNS-rebind guard, structural read-only allowlist), the state/lifecycle handling is careful, and coverage is good. It's broadly sound. The main thing worth addressing before the phone client ships is that the file-diff route can serve .git/ internals; there are also two small performance costs on always-on paths.

Security

should-fix — src-tauri/src/lan/routes/git.rs, diff_file. The new containment guard blocks ../ and absolute/drive paths, but is_safe_relative_path accepts .git as a normal component and the canonicalize check only enforces "under the repo root" — so .git/ is inside the root and passes. A paired device can call GET /api/repo/diff/file?path=.git/config&untracked=true, which reaches the git diff --no-index -- /dev/null .git/config branch (--no-index ignores tracking and gitignore and just cats the file), returning the raw .git/config. On the main checkout .git is a directory so this resolves and is served. That exceeds the "read-only access to the open repo" the feature advertises: it exposes .git/ internals (hooks, logs, refs) and, for repos whose origin URL embeds a token (https://user:token@host/… in [remote "origin"]), leaks that credential to a device that was only meant to read repo content. Concrete trigger: a roommate given the PIN once (token retained) fetches .git/config. Fix: reject any path whose first component is .git (case-insensitively) in addition to the existing check, or — more robustly — restrict the untracked branch to paths git actually reports as untracked working-tree entries (git status/ls-files --others) rather than any on-disk file under the root.

Performance

should-fix — src-tauri/src/agent.rs, FanoutSink::send. let _ = self.tx.send(ev.clone()); clones every ReviewEvent before broadcasting, unconditionally — including the token/chunk Delta events, which can be numerous in a long review/session. Because agent_review/agent_session now register a stream and wrap the sink on every run regardless of LAN state, this clone-per-event cost is paid on the hot desktop path even when the companion server is disabled (the default) and there are zero subscribers. Fix: guard the broadcast on live subscribers, e.g. if self.tx.receiver_count() > 0 { let _ = self.tx.send(ev.clone()); }, so the no-phone case (the norm) skips the clone/send entirely.

nit — src-tauri/src/lan/mod.rs, LanState::status. Both the running and non-running branches call auth::device_count(), which opens/reads lan-devices.json from disk. useLanStatus polls lan_status every 5s app-wide for the life of the process, so the disabled case does a disk read every 5s forever for a feature most users never enable — and the count isn't shown in the UI while disabled (the panel reads devices via a separate enabled-gated query). Compute device_count only in the Some(rs) (running) branch.

Resolved since last review

Verified against the current diff:

diff_file now rejects repo-escaping paths (../, leading /, Windows drive prefix) via is_safe_relative_path + canonicalize-and-starts_with, with a dedicated test — the arbitrary-absolute-path read is closed (the .git/ residual above aside).

lan_set_active_repo now takes Option<String> and App.tsx pushes null on repo close, so paired devices stop seeing the last repo (routes 409); covered by set_active_repo_none_clears.

write_devices now threads the previously-read store map through and replaces only the "devices" key, so unknown top-level keys survive a write; covered by unknown_top_level_keys_survive_a_write.

now_iso() uses SecondsFormat::Millis, so PairDeviceDialog's removal of the 5s slack in favor of a strict createdAt >= startTimeRef newest-match is safe (same clock, ms precision, ref set before the offer).

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

Originally posted by: theBGuy

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

.git/ internals via the untracked diff route — confirmed, fixed. The round-1 guard's containment check indeed passes .git/config (a Normal component inside the root). Fixed in the untracked branch: after canonicalize + containment, the path relative to the repo root is rejected if any component is .git (case-insensitive) — mirroring git's own refusal to track .git components (covers submodule gitdirs). The check runs on the canonicalized path, not the raw input, so Windows 8.3 short names (GIT~1) and case tricks (.GIT) resolve to their true form before inspection. Went with the component check over the suggested ls-files --others restriction — same containment result without spawning a git process per request. Tests extended: .git/config and .GIT/config → 400 (the latter portable across case-sensitive and -insensitive filesystems); legit untracked file still 200.

FanoutSink clone-per-event — confirmed, fixed. The LAN broadcast leg now checks receiver_count() > 0 before ev.clone(), so the default-off / no-subscriber case pays nothing on the desktop hot path. The comment documents the benign race (a subscriber attaching between check and send misses that one event — identical to connecting a moment later, which the WS route's mid-stream-join handling covers).

Disabled lan_status disk read — confirmed, fixed. Verified deviceCount has zero frontend consumers (only the type declares it; the settings panel uses the separate enabled-gated devices query), so the disabled branch now reports 0 and skips the store read — the app-wide 5s poll stays off-disk unless sharing is on. LanStatus's shape is unchanged.

Gates after the round: cargo test 749/749, clippy --all-targets -- -D warnings clean (frontend untouched this round).

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

I've verified the current state of the three previous findings and examined the new logic. Here's my review.

Summary

This is an incremental revision of the LAN phone-companion PR. The three changes since the last review — guarding the LAN broadcast on live subscribers, skipping the device-store disk read while disabled, and adding a .git-component guard to the untracked file-diff route — all land cleanly and address exactly what was raised. No blockers; the feature remains sound. One repo-convention docs gap is worth closing before merge.

Resolved since last review

Confirmed against the current diff:

.git/ internals leak (lan/routes/git.rs, diff_file) — the untracked branch now canonicalizes the target, strips the repo-root prefix, and rejects any path whose components contain .git case-insensitively (lines 132–141). Because the guard inspects the canonicalized path, .GIT/config, .git-symlink indirection, and Windows short-name/case tricks all resolve before the check, and the two new router tests (.git/config → 400, .GIT/config → 400) cover it. The --no-index arbitrary-read is only reachable via untracked=true, and the tracked/staged branches can't serve .git/config (git ignores pathspecs under .git), so the guard placement is correct.

FanoutSink::send unconditional clone (agent.rs) — now gated on self.tx.receiver_count() > 0, so the default-off desktop path skips the per-event clone/send. The desktop leg still runs unconditionally afterward, so desktop delivery is unchanged; the documented "attach between check and send misses one event" race is consistent with the existing no-replay design.

device_count disk read while disabled (lan/mod.rs, LanState::status) — the None branch now returns device_count: 0 and skips auth::device_count(), keeping the app-wide 5s lan_status poll off-disk in the default case. device_count isn't surfaced in the UI while disabled (the panel reads the list via the enabled-gated useLanDevices), so no display regression.

Docs

should-fix — repo docs-sync convention (README.md, site/). Per CLAUDE.md's standing rule, a user-facing feature must update the README Highlights/Features and the marketing-site capability catalog in the same change. The changelog fragment and in-app guide (help/content.ts) were added, but neither README.md nor any site/ file appears in the change set. The partial coverage suggests an oversight rather than a deliberate "too minor" call; either add the README bullet + site capability line, or make the skip an explicit decision. (Copilot also flagged the README/capability gap.)

Tests

nit — agent.rs, fanout_sink_delivers_to_both_legs. The new conditional in FanoutSink::send introduced a behavior branch, but the test only exercises the live-subscriber path. A companion assertion that with zero subscribers the desktop leg still receives the event (and the broadcast is skipped) would pin the invariant the change actually added.

Dismissed prior/third-party findings

Copilot's App.tsx "active repo never cleared on close" is resolved — the effect now invokes lan_set_active_repo with repoPath ?? null, so closing the repo pushes null and the routes 409.

Copilot's secondsLeft() NaN concern isn't real in practice: expiresAt is always the backend's now_iso()-format string (LanPairing.expires_at), never an invalid date, so getTime() won't return NaN.

Copilot's help-text "badge in the top corner" mismatch is resolved — the guide now reads "a Sharing on banner runs across the top of the window," matching the full-width strip in App.tsx.

Copilot's auth.rs "write_devices drops unknown keys" concerns code not present in this revision's diff, so I can't confirm it from what's shown here — not relaying it as a finding.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

Originally posted by: theBGuy

Round-3 triage — both remaining items addressed on the record; no code changes this round (the next push contains only formatting fixes for the CI biome ci gate: an import sort + two line rewraps in PairDeviceDialog.tsx / CompanionSection.tsx / api.ts).

README/site docs-sync — this IS the explicit decision the finding asks for, made and recorded in round 1 (see the resolved changelog.d/added-lan-companion-preview.md thread and the round-1 triage comment, §4): this PR targets the epic branch epic/lan-companion, not master. The epic-branch workflow exists so partial slices never reach master or public-facing docs — README.md and the marketing site get their LAN-companion entries in the final epic→master PR once the companion is usable end-to-end. The in-app help + changelog fragment ride this slice because they version with the preview surface itself. Not an oversight; deferred by design with a recorded home.

FanoutSink zero-subscriber test — already exists. The premise ("the test only exercises the live-subscriber path") doesn't hold: fanout_sink_with_no_subscribers_still_delivers_to_desktop (src-tauri/src/agent.rs, directly below fanout_sink_delivers_to_both_legs) drops the sole receiver so receiver_count() == 0, sends through the sink, and asserts the desktop leg received exactly one message — which pins precisely the invariant the new guard branch added (broadcast skipped, desktop delivery unchanged). The skip itself has no other externally observable effect, so desktop-delivery-under-zero-subscribers is the complete assertion.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

I read server.rs and mod.rs in full to verify the shutdown path. Here's the review.

Summary

This adds an opt-in, default-off embedded axum server that shares the open repo read-only with a paired phone over the LAN, with a solid security model (PIN-gated challenge/response pairing, hashed per-device bearers, Host/Origin DNS-rebind guard, per-IP rate limiting, structural read-only allowlist) and good router-integration coverage. The three items from the last round (.git/ leak guard, the default-off broadcast clone, the disabled-path disk read) are all genuinely fixed. The delta since that review is cosmetic (import ordering, whitespace, EOF newline) and introduces no new behavior. No hard blocker; one concurrency smell and the docs-sync gap are worth closing before merge.

Concurrency

should-fix — src-tauri/src/lan/server.rs, ServerHandle::shutdown / start. Shutdown signals via self.shutdown.notify_waiters(), but the serve task that registers the matching waiter is tauri::async_runtime::spawn-ed and only registers shutdown_signal.notified() on its first poll. notify_waiters() wakes currently-registered waiters and stores no permit, so if a handle.shutdown().await runs before that first poll, the wakeup is dropped, the graceful-shutdown future never completes, and let _ = self.task.await; blocks forever — while lan_disable/lan_enable hold the lifecycle async mutex, permanently wedging every subsequent enable/disable. The concrete path: lan_enable returns as soon as start() binds (the serve task not yet polled), then a lan_disable (rapid toggle) or a mode-switch lan_enable calls shutdown() inside that window. Human click-timing makes the window very narrow in practice, but the tokio scheduler gives no ordering guarantee, and this is exactly the notify_waiters-vs-notify_one lost-wakeup pattern. Fix: make shutdown: Arc<Notify> signal with notify_one() — it stores a permit, so a signal sent before the waiter registers is still delivered on the next notified().

Docs

should-fix — repo docs-sync convention (README.md, site/). Per CLAUDE.md's standing rule, a user-facing feature updates the README Highlights/Features and the marketing-site capability catalog in the same change. The changelog fragment and in-app guide (help/content.ts) are here, but no README.md or site/ file appears in the change set. The partial coverage reads as an oversight rather than a deliberate "too minor" call — either add the README bullet + site capability line, or make the skip explicit. (Also flagged by Copilot.)

Tests

nit — src-tauri/src/agent.rs, fanout_sink_delivers_to_both_legs. The test only exercises the live-subscriber path, but the behavior branch the change actually added is receiver_count() > 0 — the zero-subscriber case. A companion assertion (no subscribers → desktop leg still receives, broadcast skipped) would pin that invariant.

Resolved since last review

Confirmed against the current diff / source:

.git/ internals leak (lan/routes/git.rs, diff_file) — the untracked branch canonicalizes, enforces repo-root containment, and rejects any component matching .git case-insensitively; the two router tests (.git/config → 400, .GIT/config → 400) plus a legitimate-file 200 cover it.

FanoutSink::send unconditional clone (agent.rs) — now gated on receiver_count() > 0; the desktop leg still runs unconditionally afterward.

device_count disk read while disabled (lan/mod.rs, status) — the None branch returns device_count: 0 and skips auth::device_count().

Active repo not cleared on close (App.tsx) — the effect now pushes repoPath ?? null, and lan_set_active_repo(None) clears state so routes 409; set_active_repo_none_clears covers it.

Dismissed prior/third-party findings

Copilot's secondsLeft() NaN (PairDeviceDialog.tsx) — not real: expiresAt is always LanPairing.expires_at, produced by the backend's expires_at_iso (valid RFC3339), so new Date(...).getTime() never yields NaN.

Copilot's help-text "badge in the top corner" mismatch — resolved: the guide now reads "a Sharing on banner runs across the top of the window," matching the full-width strip in App.tsx.

Copilot's auth.rs "write_devices drops unknown keys" concerns code not shown in this diff and I did not open that region — not relaying it as a finding.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

Originally posted by: theBGuy

Round-4 triage — one new finding accepted and fixed, two re-raises closed against the existing record. Fixes land in the next push.

Shutdown lost-wakeup — confirmed, fixed. Verified against server.rs: the serve task's graceful-shutdown future registers its notified() waiter only on the task's first poll, and ServerHandle::shutdown used notify_waiters(), which stores no permit — so a rapid enable→disable inside that window drops the signal, task.await never completes, and the lifecycle mutex wedges every later toggle. Exactly the diagnosis given. Fixed with notify_one() (stores a permit; the sole waiter — one serve task per handle — consumes it on registration), plus an invariant comment at the call site explaining why notify_waiters is wrong here. cargo test 749/749 + clippy clean after the change. (No deterministic regression test: forcing the pre-first-poll interleaving would require controlling the spawned task's scheduling, which the runtime doesn't expose — the invariant comment carries the reasoning.)

README/site docs-sync — decision already on the record (third raise). See the round-1 resolved thread on changelog.d/added-lan-companion-preview.md and the round-1/round-3 triage comments: the skip is the explicit, deliberate call — this PR targets epic/lan-companion, and public docs land with the final epic→master PR so master's README/site never advertise a partial feature. Nothing has changed since that disposition.

FanoutSink zero-subscriber test — exists; second raise, previously refuted in round 3. fanout_sink_with_no_subscribers_still_delivers_to_desktop (src-tauri/src/agent.rs, directly below fanout_sink_delivers_to_both_legs) drops the sole receiver (receiver_count() == 0), sends, and asserts the desktop leg received exactly one message — the precise invariant requested. This round's preamble notes it read server.rs and mod.rs in full; the companion test is in agent.rs. A cross-reference comment now sits on fanout_sink_delivers_to_both_legs pointing at it, so it's visible in the flagged region.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

I read the LAN server, router, auth, reviews routes, and the frontend query/type contracts in full to ground this. Here's the review.

Summary

This adds an opt-in, default-off embedded axum server that shares the open repo read-only with a paired phone over the LAN. The security model is solid (PIN-gated challenge/response pairing, hashed per-device bearers, Host/Origin DNS-rebind guard, per-IP rate limiting, structural read-only allowlist, path-containment on the untracked diff route), lifecycle handling is careful, and coverage is good. Broadly sound with no hard blocker. One real scoping gap in the live-monitoring routes is worth fixing before the phone client ships, plus the docs-sync convention.

Correctness / Security

should-fix — src-tauri/src/lan/routes/reviews.rs (list, stream) + src-tauri/src/state.rs (StreamInfo). The two live-monitoring routes never consult the active/shared repo, unlike every git/forge route (which 409 via repo_or_409! when active_repo is None). list runs snapshot_streams(&state.streams) over the entire registry, and stream subscribes by id across it — and StreamInfo carries only tx/kind/started_at, no repo path, so the routes couldn't filter even if they wanted to. Concrete leak: user shares repo A, starts an agent review/session on A (registered in active_streams for the whole run), then either closes the repo (App.tsx pushes null → lan_set_active_repo(None), git routes now 409) or switches to repo B (active_repo → B). Throughout the review/session's lifetime, a paired device can GET /api/reviews and watch A's ReviewEvent stream (agent output, diffs, file contents) — content from a repo that is not the one currently shared. This contradicts the PR's own containment promise ("paired devices stop seeing the last repo"). Fix: record the originating repo path in StreamInfo at register_stream time, and in both routes filter/authorize against the current active_repo (empty list / 404 for streams not belonging to the shared repo; 409 when none is shared).

Docs

should-fix — repo docs-sync convention (README.md, site/). Per CLAUDE.md, a user-facing feature updates the README Highlights/Features and the marketing-site capability catalog in the same change. The changelog fragment and in-app guide are present, but no README.md or site/ file is in the change set. Given the new Settings surface + app-wide banner + tray row + command, this reads as an oversight rather than a deliberate "too minor" call — either add the README bullet + site capability line, or make the skip explicit. (Also flagged by Copilot.)

Performance

nit — src-tauri/src/lan/auth.rs, require_auth → authenticate_bearer. Every authenticated request re-reads and JSON-parses lan-devices.json from disk under the global store_lock. Harmless in this slice (no phone client hits the HTTP surface yet; the desktop's own lan_status poll goes through the Tauri command, not these routes), but once the companion polls /api/repo/* this is a per-request disk read on the hot path. Consider an in-memory device cache invalidated on pair/revoke.

Resolved since last review

Shutdown lost-wakeup (server.rs, ServerHandle::shutdown) — confirmed fixed: now self.shutdown.notify_one(), which stores a permit so a shutdown signalled before the serve task's first poll of shutdown_signal.notified() is still consumed. The notify_waiters wedge (task pending forever with lifecycle held) is closed.

Zero-subscriber fanout test — confirmed added: fanout_sink_with_no_subscribers_still_delivers_to_desktop (agent.rs:3039) pins the receiver_count() == 0 branch.

Dismissed prior / third-party findings

Copilot App.tsx active-repo-not-cleared — resolved: the effect now invokes lan_set_active_repo with repoPath ?? null, and lan_set_active_repo(None) clears state (mod.rs:243); set_active_repo_none_clears covers it.

Copilot secondsLeft() NaN (PairDeviceDialog.tsx) — not real: secondsLeft is only ever called on p.expiresAt, and p comes from lan_pairing_start, whose expires_at is session.expires_at_iso — a valid RFC3339 string from iso_after (auth.rs:394). new Date(...).getTime() won't be NaN.

Copilot auth.rs:215 "comment is misleading about write_devices dropping keys" — not valid against current code: the comment at auth.rs:213-216 accurately distinguishes the two cases — unknown top-level keys ARE preserved (write_devices mutates the previously-read store map and replaces only "devices"), while unknown per-record fields are NOT (records round-trip through the typed StoredDevice). The behavior matches the comment.

Copilot help "badge in the top corner" mismatch — resolved: the guide now reads "a Sharing on banner runs across the top of the window," matching the full-width strip in App.tsx.

Prior path-traversal via diff/file?...&untracked=true — resolved: is_safe_relative_path rejects RootDir/Prefix/ParentDir on both branches, the untracked branch adds canonicalized repo-root containment plus a case-insensitive .git-component guard, and diff_file_rejects_paths_escaping_the_repo covers ../x, /etc/passwd, C:/x (Windows), .git/config, .GIT/config → 400 and a legit file → 200.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

Originally posted by: theBGuy

Round-5 triage — the scoping gap is confirmed and fixed (with one product upgrade beyond the finding's letter), the perf nit is deferred with a recorded home, and the docs item stands as previously decided. Fixes land in the next push.

Live-monitoring routes not scoped to the shared repo — confirmed, fixed. Verified all three layers matched the finding: StreamInfo carried no repo, list snapshotted the whole registry, stream subscribed unchecked. Fix: StreamInfo now records the originating repo at register_stream time; both LAN routes resolve the shared repo first (repo_or_409! — 409 when none is shared, same as every git route) and then filter/authorize — list returns only the shared repo's streams, stream 404s for an out-of-scope id with the same noSuchStream shape as an unknown one (deliberately no oracle distinguishing "exists elsewhere" from "doesn't exist"). Wire shape [{id, kind, startedAt}] unchanged — repo paths never reach the phone. Desktop-side surfaces stay unscoped (the desktop sees all its own streams).
One deliberate upgrade: registering sessions under the path they run in would have made them silently invisible to phones — write sessions run in gd/session/* worktrees, whose path never matches the shared repo. Sessions now register under the repo they were spawned from (agent_session gains an origin_repo_path param, threaded from every frontend call site; read-only Plan/Research sessions pass the live repo, identical to before). So the containment the finding asks for holds — a stream from another repo is never served — while a session spawned from the shared repo stays watchable, which is the monitor feature's whole point. Tests: scoped snapshot/subscribe filtering (matching repo listed incl. a worktree-spawned session; other-repo stream omitted; unknown + out-of-scope ids → None), router-level list scoping, and 409-when-none-shared. Suite is now 752 passing.

Per-request device-store read in authenticate_bearer — mechanism confirmed, deferred to epic slice 2 with a recorded home. Agreed it's harmless in this slice exactly as the finding notes (no HTTP client exists until the companion bundle ships). An in-memory device cache invalidated on pair/revoke is now an explicit slice-2 work item alongside the cookie-based WS auth that slice already requires — it belongs where its consumer (the polling phone client) materializes.

README/site docs-sync — fourth raise; the recorded decision stands (round-1 resolved thread on the changelog fragment, round-1/3/4 triage comments): public docs land with the final epic→master PR. Docs written against this slice would describe a bundle-less preview and be rewritten in slice 3 when the actual companion app exists — the deferral is the deliberate call, not an oversight.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-17

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

I've now read the full LAN server, auth, routes, state scoping, and the frontend companion/pairing components. Here's the review.

Summary

This adds an opt-in, default-off embedded axum server that shares the open repo read-only with a paired phone over the LAN, and (in this revision) scopes the live agent-stream monitor to the currently-shared repo. The security scaffolding is careful and well-tested (PIN-gated challenge/response, hashed per-device bearers, Host/Origin DNS-rebind guard, per-IP rate limiting, structural read-only allowlist, --no-index path containment). It's broadly sound. The one substantive gap is that live WebSocket monitor connections aren't tied to the sharing/repo lifecycle, which weakens the containment the docs promise.

Resolved since last review

The live-monitor routes (reviews.rs list/stream) are now scoped to the shared repo via repo_or_409! + snapshot_streams_for/subscribe_in_for, with StreamInfo.repo_path threaded through register_stream and origin_repo_path plumbed from all four runAgentSession call sites (plan, research ×2, sessions). The previous should-fix is addressed, and it's covered by scoped_snapshot_and_subscribe_filter_by_repo + the two router-level tests. repoPath is a non-optional field on AgentSession, so the origin is always set.

Correctness / Security

should-fix — src-tauri/src/lan/routes/reviews.rs stream/forward_stream + src-tauri/src/lan/mod.rs lan_disable/lan_set_active_repo. Repo/sharing scoping is enforced only at subscribe time; a live WebSocket is never re-scoped or cut. ws.on_upgrade(move |socket| forward_stream(socket, rx)) runs forward_stream in a tokio task that axum spawns detached from the serve task, and forward_stream(socket, rx) holds only the broadcast receiver — it has no reference to active_repo or any shutdown signal. Concrete case: a phone opens /api/reviews/{id}/stream for a running agent session while repo A is shared; the user then clicks Stop sharing (lan_disable) or closes/switches the repo (lan_set_active_repo(None)). lan_disable shuts down the serve task and drops the listener, but the already-accepted WS socket is independent and the detached task keeps forwarding the run's live ReviewEvents until the run ends (RecvError::Closed when the producer's tx drops) or the phone disconnects. So the module claim "a stream on a repo the desktop has since closed or switched away from is invisible" and the user-facing promise "Turn sharing off when you're done. It doesn't turn itself off" don't hold for an in-flight monitor stream. Fix: give forward_stream a cancel signal (a broadcast/Notify in RouterState, or a live-connection registry) and select! on it in the loop, firing it from lan_disable and whenever the active repo changes.

should-fix — src-tauri/src/lan/auth.rs pair_submit. The "single active session → at most one device" invariant is breakable by concurrent submissions. The expected proof is computed inside the state.pairing lock, but the session is cleared (*guard = None) only after mint_device/persist_device, in a separate lock acquisition. Two concurrent POST /api/pair requests carrying the correct proof both acquire the lock in turn, both still see the live session (neither has cleared it), both pass constant_time_eq, and both mint + persist a bearer — a single PIN entry yields multiple tokens. Exploitability is low (requires the PIN + concurrent timing on the LAN), but it contradicts the documented single-session model and means one visible "pair" can leave extra tokens. Fix: guard.take() the session (or null its challenge) inside the same locked block once the proof verifies, before releasing, so a racing request hits pairing_inactive.

Docs / project conventions

should-fix — repo docs-sync. Per CLAUDE.md, a user-facing feature must update README.md (Highlights/Features) and the marketing site capabilities list (site/src/pages/index.astro, non-AI → both views) in the same change. This PR updates the in-app help guide (content.ts) and adds a changelog fragment but touches neither README nor the site. Given the size of the added help section, this reads as user-facing enough to warrant at least the README bullet + site capability line — or make the deferral a deliberate, stated call (the convention only sanctions skipping those for very minor features).

Readability / UX

nit — src/features/settings/CompanionSection.tsx, useLanDevices({ enabled }). The device list is gated on enabled, so while sharing is off the list is empty and a previously-paired device can't be seen or revoked — but the help guide states you can "Revoke any of them at any time," and a persisted device becomes valid again the moment sharing is re-enabled. Consider allowing the list (and revoke) while off, or softening the guide wording.

Everything else I checked holds up: the FanoutSink desktop leg is byte-for-byte unchanged with the zero-subscriber clone guard, StreamGuard clears on every exit path, the diff_file containment (lexical + canonicalized starts_with + case-insensitive .git-component reject) is correct and well-tested, the host/origin guard is a sound DNS-rebind defense, and the lifecycle mutex correctly serializes enable/disable.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-18

Ticket changed by: theBGuy

status: open --> closed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-18

Originally posted by: theBGuy

Round-6 closure — this PR was (accidentally) squash-merged into epic/lan-companion mid-round, so the round-6 findings' fixes land in the follow-up PR #72 (fix/lan-round6-hardening → epic/lan-companion), which contains exactly the round-6 delta and carries the full dispositions on its own record:

Live WS monitors survive disable/switch → accepted, fixed in [#72] via a monitor_cut broadcast severing in-flight sockets on disable, rebind, actual repo change, and (an addition beyond the finding — same hole class, upgrade-time-only auth) device revoke.

pair_submit double-mint race → accepted, fixed in [#72]: verify + consume (guard.take()) are now atomic under the pairing lock, mint/persist outside; duplicate submissions get pairingInactive.

Device list gated on sharing (nit) → accepted, fixed in [#72]: paired devices are visible and revocable while sharing is off, making the help guide's "revoke any of them at any time" true.

README/site docs-sync → the recorded decision stands (rounds 1/3/4/5): public docs ship with the final epic→master PR.

Review continues on [#72].

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

Related

Tickets: ~~#72~~
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Latest commit:	`84548fc`
Status:	✅ Deploy successful!
Preview URL:	https://d7541062.gitdesktop.pages.dev
Branch Preview URL:	https://feat-lan-phone-companion-pre.gitdesktop.pages.dev

feat(lan,settings,git): add phone companion preview for LAN repo sharing

Milestone

Searches

Help

#70 feat(lan,settings,git): add phone companion preview for LAN repo sharing

Backend (Rust: LAN server, pairing, and API)

Frontend (React: Settings UI, pair flow, banner)

API & Data Layer (TypeScript/React)

Backend plumbing and integration

Related

Discussion

Deploying gitdesktop with Cloudflare Pages

Finding

Summary

Correctness

Edge cases

Docs / conventions

What's solid

Security audit — path traversal via diff/file + untracked=true: confirmed, fixed

General review

Gates after the fix round

Summary

Security

Performance

Resolved since last review

Summary

Resolved since last review

Docs

Tests

Dismissed prior/third-party findings

Summary

Concurrency

Docs

Tests

Resolved since last review

Dismissed prior/third-party findings

Summary

Correctness / Security

Docs

Performance

Resolved since last review

Dismissed prior / third-party findings

Summary

Resolved since last review

Correctness / Security

Docs / project conventions

Readability / UX

Related

Security audit — path traversal via `diff/file` + `untracked=true`: confirmed, fixed