Skip to content

fix(relay): apply the host LAN pairing mode to already-paired devices - #19870

Closed
nwparker wants to merge 12 commits into
mainfrom
fix/18211-lan-mode-applies-to-paired-devices
Closed

nwparker wants to merge 12 commits into
mainfrom
fix/18211-lan-mode-applies-to-paired-devices

Conversation

@nwparker

@nwparker nwparker commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 18 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1215 $\color{#cf222e}{\Huge{\mathbf{−}}}$​35 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1180
Prod 19 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​672 $\color{#cf222e}{\Huge{\mathbf{−}}}$​307 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​365

Closes #18211

ELI5

Orca has one "Connection" switch for phones: Orca Relay (anywhere) or LAN (same network). Before this change the switch only affected the next QR code you generated. A phone you had already paired on Relay stayed on Relay forever, and the desktop kept its Relay session open forever, no matter what the switch said. Now the switch is a real policy: flip it to LAN and the desktop stops offering Relay to every paired phone right away; flip it back to Orca Relay and the phones get Relay back without re-pairing.

The bug

settings.mobilePairingConnectionMode is asked three questions and implements one:

  1. What should the next QR encode? — implemented (renderer → mobile:getPairingQR → createMobilePairingOffer).
  2. May this desktop serve mobile over Relay right now? — no implementation at all. src/main/** had zero readers of the setting.
  3. How was this device paired? — DeviceEntry.mobilePairingConnectionMode, frozen at pair time.

Q2 being unimplemented is the bug: the live path answers it with Q3, making a historical record the authority on a live question. A device paired as automatic stays on Relay forever.

Three concrete places:

  • relay-demand-ledger.ts hasDemand consulted neither mode; any non-expired relayBinding was standing demand and held the broker open.
  • The same function short-circuited on transientRefs.size > 0 before the device loop, and refs carried no device id — in-flight work outvoted the policy and could not be filtered.
  • desktop-relay-service.ts gated getEndpoints/provisionRelay on the device's pair-time mode, but createPairingRelay — the door that grants — had no gate at all.

Repro gate (failed before the fix, on unmodified main)

src/main/runtime/relay/repro-18211.test.ts, run against 2bf298d1dc1 with no other change:

 ❯ src/main/runtime/relay/repro-18211.test.ts (3 tests | 2 failed)
     × a standing relay binding stops being demand once the host picks LAN
       AssertionError: expected true to be false     // hasDemand after the user picked LAN
     × a policy change withdraws the open relay session promptly instead of lingering
       AssertionError: expected "vi.fn()" to be called once, but got 0 times   // closeNow not called
     ✓ pairing churn keeps the ten-minute linger

The first measurement is the defect made executable: RelayDemandLedger had no parameter that could even carry the user's choice. The second measures the 10-minute linger that would otherwise make the fix invisible (user flips to LAN, sees Relay for ten more minutes, re-files this bug).

What changed

Layer Change
src/shared/mobile-relay-policy.ts (new) isMobileRelayAllowed({hostConnectionMode, deviceConnectionMode}) → host !== 'local-only' && device !== 'local-only'. Restrictive-only: the host setting withdraws Relay from an automatic device but never grants it to a local-only one; a device the registry can't place contributes no opinion. mobile-pairing-connection-mode.ts still owns the mint question.
relay-demand-ledger.ts Optional isRelayAllowedForDevice. Transient refs now carry a deviceId (acquireTransient(key, deviceId)); the size > 0 short-circuit is a loop counting only refs whose device is still allowed; the same guard applies inside the device loop.
desktop-relay-service.ts Optional hostMobilePairingConnectionMode?: () => mode (default 'automatic', so prototype-built tests keep working). Private isRelayAllowedForDevice composes host + registry mode. withTransientDemand(kind, deviceId, op) refuses at the choke point with relay_disabled_for_device — this is what finally covers createPairingRelay. getEndpoints keeps its early soft {v:1, relay:null}, provisionRelay its early throw, both now policy-aware. New pairingPolicyChanged() wake signal.
relay-auth-coordinator.ts reconcile({ skipLinger }) threaded to the hasDemand === false branch: cancelLinger() + invalidateOwnership() instead of scheduleLinger. Every existing caller is unchanged, so pairing churn keeps its linger.
src/main/startup/desktop-relay-startup.ts (new, extracted from main-process-runtime-launch.ts) Passes hostMobilePairingConnectionMode: () => store.getSettings().mobilePairingConnectionMode ?? 'automatic' and subscribes store.onSettingsChanged → pairingPolicyChanged() when mobilePairingConnectionMode is in the change set. Wake signal only — the decision is the pull, same family as authMutated() / demandStateChanged(). The store already drops no-op writes, so a re-save of the same value does not wake anything.
MobilePane.tsx, MobilePage.tsx changeConnectionMode(next, { persist?: boolean }). The Relay-mint-failure "Use LAN" recovery buttons pass { persist: false }: now that the setting is a host policy, one click on that button would otherwise revoke Relay from a phone in another city, which the button's copy does not promise. No copy change — once the setting is a policy the existing LAN string is accurate for all devices.

Line-budget extractions (behavior-preserving): relay-auth-coordinator-contract.ts (types), relay-pairing-authorization.ts (same file name/content as #19659's extraction so they merge cleanly), relay-pairing-endpoints-result.ts, mobile-pane-device-revoke.ts, mobile-pane-test-mount.tsx.

Decision: already-open Relay session

Withdraw promptly, preserve the binding. relayBinding stays on disk, so flipping back to Orca Relay restores Relay in one reconcile with no re-pairing — the leak-never-kill direction from docs/reference/ssh-execution-boundary.md. Nothing is deleted or revoked server-side; the desktop simply stops holding a control open and stops handing out an endpoint.

Tradeoff, head-on: a LAN dial that fails could leave an off-LAN phone stranded until the user flips back. The judgment is that honouring an explicit, reversible choice beats silently ignoring it — the alternative is the status quo, where the switch lies. The "Use LAN" recovery buttons are deliberately exempted (above) so the one place a user is nudged toward LAN under failure does not carry this consequence.

Rule 3 (remote-wire-compatibility) — measured, with an honest gap

Claim: no capability negotiation is needed because an old mobile client receives {v:1, relay:null} from pairing.getEndpoints (already its normal answer for a local-only device) and the pre-existing relay_disabled_for_device error from pairing.provisionRelay.

What I verified:

  • pnpm exec vitest run --config config/vitest.config.ts tests/e2e/cross-version-wire/cross-version-terminal-wire.unit.test.ts → 10/10 pass. However, that suite exercises the terminal stream and RPC envelope; it does not drive pairing.getEndpoints / pairing.provisionRelay, so it is evidence the envelope is untouched, not evidence of the pairing contract.
  • Contract reading of the current mobile client (mobile/src/transport/): mobile-relay-direct-upgrade.ts:74 throws 'relay endpoint unavailable for direct pairing upgrade' on relay: null and retains the durable journal; mobile-relay-credential-rotation.ts:68 throws 'relay credential rotation endpoint state missing' and the supervisor's catch keeps pending material durable for the next authenticated direct opportunity. Both are existing paths, not new ones; the wire shape and error vocabulary are unchanged. mobile-relay-direct-upgrade.test.ts:161 pins the relay: null case.
  • I could not run the mobile test suite in this worktree (mobile/node_modules is not installed here), so the mobile-side behavior is contract-reading plus an existing pinned test, not a fresh run. If a reviewer wants a measured mobile run before merge, that is the gap.

Verification

  • pnpm tc clean; repo-wide oxlint clean; pnpm run check:code-quality:changed 0 new findings; check:max-lines-ratchet OK (no new bypasses — files were split instead); verify:localization-extraction unchanged.
  • pnpm test on src/main/runtime/relay/, src/main/startup/desktop-relay-startup.test.ts, main-process-ready-phase-ordering.test.ts, src/shared/mobile-relay-policy.test.ts, MobilePane.test.tsx, MobilePage.test.tsx → 23 files, 218 tests pass.

Mutation results (each mutation targets one specific regression; nothing reverted wholesale)

# Mutation Result
1 Ledger device loop drops the policy guard Killed by repro-18211 L54 only
2 Ledger restores the old transientRefs.size > 0 short-circuit Killed by repro-18211 L58 only (the in-flight-outvotes-policy assertion)
3 Coordinator ignores skipLinger Killed by repro-18211 "withdraws promptly" only
4 withTransientDemand no longer refuses Killed by the two service policy tests (incl. the createPairingRelay assertion)
5 pairingPolicyChanged() keeps the linger Killed by desktop-relay-service.test.ts "reconciles without the pairing-churn linger" only
6 Service accessor ignores the host mode Killed by "refuses every relay grant…" only
7a/7b "Use LAN" recovery buttons persist again (MobilePane / MobilePage) Each killed by exactly its own new not.toHaveBeenCalledWith assertion
8 Shared policy ignores host mode Killed by 2 of 3 policy tests
9 Coordinator always skips the linger Killed by "pairing churn keeps the ten-minute linger" and the pre-existing linger test

No survivors.

Visual proof (dev build, CDP, background launch)

Attached to this worktree's own instance (devWorktreeName: agent-accd9b4b96e6f876f) and clicked the real LAN radio; the store and the persisted settings:get both flipped to local-only, which is the write that now wakes pairingPolicyChanged().

Before (Orca Relay selected; this dev build has no cloud auth so Relay shows Unavailable):

before

After clicking LAN:

after

Not shown visually: the broker actually closing, because the dev build is not signed in to a Relay-entitled cloud profile. That path is covered by the coordinator/service tests above rather than a screenshot.

Stacked on #19659 → #19436

Base is main because both parents live on other forks, so this diff shows their commits until they merge. This PR's own change is the top two commits (9ebf37d9607, 2aa0fdc1cf4). Verified three-way: the requireActiveBroker hunk (#19659) and the refreshDemand hunk (this PR) in desktop-relay-service.ts merge cleanly; the only conflicts were the import block and the pairingAuthorizationForContext deletion, both of which resolve by taking the superset.

Review follow-ups (second commit)

  • skipLinger survives a retried reconcile. If readContext() throws during the policy-change reconcile, the armed retry now carries the reconcile options, so when it finally observes no demand it still closes promptly instead of applying the ten-minute linger. Test: repro-18211 "a policy change that failed transiently still skips the linger on its retry"; mutation "retry drops the reconcile options" is killed by it alone.
  • A queued revoke still holds the relay open under LAN — deliberately. A revoke is server-side cleanup for a credential the user already withdrew, not a grant. Dropping it on a LAN pick would leave that credential live on the relay until the user flipped back. The session closes as soon as the revoke flushes (flushRevoke → refreshDemand). Pinned by a test so the choice is visible, not accidental.

Readiness self-review

Ran the checklist against the diff. No candidate P0/P1 findings, so counsel validation was skipped per the skill's zero-findings rule. Residual risk (not proven defects): the stranded-off-LAN-phone tradeoff above; the mobile suite was not run in this worktree.

Rebase record (2026-09-11)

Rebased onto main at 5fa62feda7c264c6eda99ba4788ffebab9aae26c.

SHA
old head 9ce190247edbdb508b452e8c62a63c4c2cdd0089
new head 13eee7eacf8a6fcd4bb77ff680d18f727fca1171

One conflict, in src/main/startup/main-process-runtime-launch.ts: main (#19935) extracted the relay onStatus callback into main-process-relay-status.ts and added startDesktopPushService calls; this branch extracted the whole relay block into desktop-relay-startup.ts. Resolved by keeping both — startDesktopPushService(runtimeRpc) then startDesktopRelayService(runtimeRpc) — and pointing the extracted desktop-relay-startup.ts at main s publishDesktopRelayStatus instead of re-inlining the callback.

Gates on the rebased head: run-typecheck-projects-in-parallel.mjs exit 0 (gate proven failable by injecting a type error); vitest run --config config/vitest.config.ts over desktop-relay-startup.test.ts, repro-18211.test.ts, mobile-relay-policy.test.ts — 3 files / 10 tests passed.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The change applies host and device connection-mode policy to relay demand and service entry points. It adds immediate reconciliation for policy changes while preserving normal linger behavior. Desktop relay startup now owns settings and resume wiring. Renderer recovery can switch to LAN without persisting the global setting. Paired-device revocation uses a shared asynchronous helper. Regression tests cover policy enforcement, demand lifecycle, startup wiring, and recovery behavior.

Priority: ➖ Normal

Severity of issue fixed: Medium

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The pull request extracts device revocation into mobile-pane-device-revoke.ts and changes the MobilePane revocation path. The linked issue covers host LAN policy, Relay demand and sessions, and at… Remove the unrelated device-revocation extraction and its dedicated test plumbing, or move those changes to a separate pull request.
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 36 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: applying the host LAN pairing mode to already-paired devices.
Description check ✅ Passed The description is detailed and covers the user impact, implementation, rationale, linked issue, visual proof, testing, compatibility considerations, risks, and follow-up behavior. It does not reprodu…
Linked Issues check ✅ Passed Issue #18211 requirements are met. isMobileRelayAllowed withdraws Relay when the host or device uses local-only. RelayDemandLedger filters standing bindings and transient references. `DesktopRel…
Full details: Out of Scope Changes check

Explanation

The pull request extracts device revocation into mobile-pane-device-revoke.ts and changes the MobilePane revocation path. The linked issue covers host LAN policy, Relay demand and sessions, and attempt-scoped Relay-failure recovery. Device revocation is unrelated. The associated shared test-mount changes also do not implement these objectives.

✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

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

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 9740f0d5-cd67-4e0f-8e46-3e3d23a42c06

📥 Commits

Reviewing files that changed from the base of the PR and between 2bf298d and 5d64d4b.

📒 Files selected for processing (21)
  • src/main/runtime/relay/desktop-relay-service.test.ts
  • src/main/runtime/relay/desktop-relay-service.ts
  • src/main/runtime/relay/relay-auth-coordinator-contract.ts
  • src/main/runtime/relay/relay-auth-coordinator.ts
  • src/main/runtime/relay/relay-demand-ledger.test.ts
  • src/main/runtime/relay/relay-demand-ledger.ts
  • src/main/runtime/relay/relay-pairing-authorization.ts
  • src/main/runtime/relay/relay-pairing-endpoints-result.ts
  • src/main/runtime/relay/repro-18211.test.ts
  • src/main/startup/desktop-relay-startup.test.ts
  • src/main/startup/desktop-relay-startup.ts
  • src/main/startup/main-process-ready-phase-ordering.test.ts
  • src/main/startup/main-process-runtime-launch.ts
  • src/renderer/src/components/mobile/MobilePage.test.tsx
  • src/renderer/src/components/mobile/MobilePage.tsx
  • src/renderer/src/components/settings/MobilePane.test.tsx
  • src/renderer/src/components/settings/MobilePane.tsx
  • src/renderer/src/components/settings/mobile-pane-device-revoke.ts
  • src/renderer/src/components/settings/mobile-pane-test-mount.tsx
  • src/shared/mobile-relay-policy.test.ts
  • src/shared/mobile-relay-policy.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Comment on lines 55 to 56
if (this.options.revokeOutbox.pendingFor(ownerIdentityKey, this.options.relayHostId).length) {
return true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not let queued revokes override the LAN policy.

pendingFor(...) returns demand before any isRelayAllowed check. If a revoke remains queued when the host selects LAN, hasDemand() stays true. The immediate reconciliation then keeps the Relay session open.

Filter pending items by relayDeviceId, or suppress queued-revoke demand when the host policy disables Relay.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Live host pairing policy — isMobileRelayAllowed composes the host mobilePairingConnectionMode with each device's pair-time mode, restrictively: a LAN pick withdraws Relay from already-paired automatic devices and never grants it to a local-only one; an unplaceable device contributes no opinion.
  • Demand-ledger policy awareness — RelayDemandLedger transient refs now carry a deviceId, and hasDemand counts only refs/devices the policy still allows, so in-flight work can no longer outvote a LAN pick.
  • Choke-point gate plus prompt withdrawal — withTransientDemand(kind, deviceId, op) refuses with relay_disabled_for_device (now covering createPairingRelay, which previously had no per-device gate); pairingPolicyChanged() reconciles with skipLinger so a deliberate LAN pick closes the broker immediately instead of after the ten-minute linger.
  • Startup wiring — desktop-relay-startup.ts supplies the host-mode accessor from settings and wakes the relay service only when mobilePairingConnectionMode changes.
  • Recovery-button exemption — the Relay-mint-failure "Use LAN" buttons pass { persist: false } so a failure-recovery click mints a LAN QR without revoking Relay from every paired phone.
  • Line-budget extractions — coordinator contract types plus the endpoint/authorization builders moved to dedicated files with no behavior change.

The mobile-facing claims check out against the sources: mobile-relay-direct-upgrade.ts:74 throws on relay: null with the durable journal retained, and mobile-relay-credential-rotation.ts:68 throws with pending material left durable — both pre-existing paths, so the wire shape and error vocabulary are unchanged.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@nwparker
nwparker force-pushed the fix/18211-lan-mode-applies-to-paired-devices branch 2 times, most recently from 276a1e6 to 2aa0fdc Compare September 10, 2026 11:34

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: a7186a78-bc6e-4e05-a211-300c0ee7b59e

📥 Commits

Reviewing files that changed from the base of the PR and between 5d64d4b and 2aa0fdc.

📒 Files selected for processing (20)
  • src/main/runtime/relay/desktop-relay-service-failure-cause.test.ts
  • src/main/runtime/relay/desktop-relay-service.ts
  • src/main/runtime/relay/relay-auth-coordinator-contract.ts
  • src/main/runtime/relay/relay-auth-coordinator-offline-reason.test.ts
  • src/main/runtime/relay/relay-auth-coordinator-recovery.test.ts
  • src/main/runtime/relay/relay-auth-coordinator.ts
  • src/main/runtime/relay/relay-drain-retry-schedule.ts
  • src/main/runtime/relay/relay-offline-reason.test.ts
  • src/main/runtime/relay/relay-offline-reason.ts
  • src/main/runtime/relay/relay-origin-pool.ts
  • src/main/runtime/relay/relay-retry-backoff.test.ts
  • src/main/runtime/relay/relay-retry-backoff.ts
  • src/main/runtime/relay/relay-retry-schedule.test.ts
  • src/main/runtime/relay/relay-retry-schedule.ts
  • src/main/runtime/relay/repro-18211.test.ts
  • src/renderer/src/components/mobile/MobilePage.tsx
  • src/renderer/src/components/mobile/mobile-relay-diagnostics-payload.test.ts
  • src/renderer/src/components/mobile/mobile-relay-diagnostics-payload.ts
  • src/renderer/src/components/settings/MobilePane.tsx
  • src/shared/mobile-relay-mint-failure.test.ts
💤 Files with no reviewable changes (1)
  • src/main/runtime/relay/relay-drain-retry-schedule.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

Comment on lines +291 to +293
if (!this.isRelayAllowedForDevice(deviceId)) {
throw new Error('relay_disabled_for_device')
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- desktop-relay-service.ts ---'
sed -n '240,380p' src/main/runtime/relay/desktop-relay-service.ts
printf '%s\n' '--- coordinator policy/reconcile symbols ---'
rg -n -C 8 'pairingPolicyChanged|reconcile\(|beginReconcile|fenceAndCloseNow|isCurrent' src/main/runtime/relay/desktop-relay-service.ts src/main/runtime/relay/relay-auth-coordinator.ts
printf '%s\n' '--- broker current/close behavior ---'
rg -n -C 8 'assertCurrent|closeNow|closed|isCurrent' src/main/runtime/relay/relay-session-broker.ts

Repository: stablyai/orca

Length of output: 29357


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- policy-change callers ---'
rg -n -C 12 'pairingPolicyChanged|hostMobilePairingConnectionMode|isRelayAllowedForDevice' src/main/runtime/relay

Repository: stablyai/orca

Length of output: 28069


🏁 Script executed:

#!/bin/bash
set -eu
sed -n '54,115p' src/main/runtime/relay/relay-auth-coordinator.ts
sed -n '178,285p' src/main/runtime/relay/relay-auth-coordinator.ts
printf '%s\n' '--- existing policy tests ---'
sed -n '90,190p' src/main/runtime/relay/desktop-relay-service.test.ts
printf '%s\n' '--- coordinator ownership invalidation definitions ---'
rg -n -C 12 'invalidatePendingOwnerships|invalidateOwnership|ownership =' src/main/runtime/relay/relay-auth-coordinator.ts

Repository: stablyai/orca

Length of output: 18654


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- origin pool close and endpoint state ---'
rg -n -C 10 'closeNow|activeAssignment|activeControl|hasLiveControl' src/main/runtime/relay/relay-origin-pool.ts
printf '%s\n' '--- endpoint result helper ---'
sed -n '1,80p' src/main/runtime/relay/relay-pairing-endpoints-result.ts
printf '%s\n' '--- provision ordering ---'
sed -n '178,212p' src/main/runtime/relay/desktop-relay-service.ts

Repository: stablyai/orca

Length of output: 6442


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- control request implementations ---'
rg -n -C 14 'createInvite|installCredential' src/main/runtime/relay

Repository: stablyai/orca

Length of output: 29494


Authorization Bypass

Reachability: External
Exploitability: Moderate
CWE: CWE-367 — Time-of-check Time-of-use (TOCTOU) Race Condition

Prevent in-flight Relay grants after a LAN-only change.

RelaySessionBroker.assertCurrent() rejects stale results only after createInvite or installCredential has sent its request. A LAN-only change can therefore leave an invite or credential installed even when the caller receives an error. Cancel or compensate these remote operations, and add an in-flight regression test.

it('carries broker_unavailable once the wait budget outlasts a retried open failure', async () => {
// Why the budget: a transient open failure arms a retry, and the waiter sits
// through it; the cause surfaces only when the wait itself gives up.
vi.useFakeTimers()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Restore real timers in an afterEach hook.

If an assertion fails before vi.useRealTimers(), later tests in this suite can inherit fake timers and stall on timer-backed awaits. Add suite-level cleanup and remove the trailing restores.

Comment on lines +156 to +159
const payload = await collectMobileRelayDiagnosticsPayload({
connectionMode,
failure: relayMintFailure
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- MobilePage.tsx ---'
sed -n '120,185p' src/renderer/src/components/mobile/MobilePage.tsx
printf '%s\n' '--- MobilePane.tsx ---'
sed -n '275,340p' src/renderer/src/components/settings/MobilePane.tsx
printf '%s\n' '--- collector definition and environment resolver references ---'
rg -n -C 8 'collectMobileRelayDiagnosticsPayload|resolveClientEnvironmentInfo' src/renderer/src

Repository: stablyai/orca

Length of output: 22138


🤖 get_repo_knowledge executed:

get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings

Length of output: 41093


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- client environment resolver ---'
cat -n src/renderer/src/lib/client-environment-info.ts
printf '%s\n' '--- diagnostics collector ---'
cat -n src/renderer/src/components/mobile/mobile-relay-diagnostics-payload.ts
printf '%s\n' '--- tests for the affected actions and collector ---'
rg -n -l 'copyRelayDiagnostics|diagnosticsCopyFailed|collectMobileRelayDiagnosticsPayload|resolveClientEnvironmentInfo' src --glob '*.{test,spec}.{ts,tsx}'

Repository: stablyai/orca

Length of output: 4407


Catch diagnostics collection failures in both Copy diagnostics actions.

If resolveClientEnvironmentInfo() rejects through collectMobileRelayDiagnosticsPayload(), the await before each try exits the action before the clipboard catch runs. The user may not see the Failed to copy diagnostics toast. Move collection into the existing try block, or provide a safe collector fallback. Add rejection tests for both actions.

  • src/renderer/src/components/mobile/MobilePage.tsx#L156-L159
  • src/renderer/src/components/settings/MobilePane.tsx#L310-L313
📍 Affects 2 files
  • src/renderer/src/components/mobile/MobilePage.tsx#L156-L159 (this comment)
  • src/renderer/src/components/settings/MobilePane.tsx#L310-L313

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one minor suggestion inline on the skipLinger carry-through.

Reviewed changes

  • Mint-failure cause taxonomy — the coordinator now records an offlineReason (signed-out / not_entitled / auth_unavailable / broker_unavailable / broker_rejected), waitForLiveBrokerResult carries it, and requireActiveBroker maps it to a specific code instead of the generic relay_control_not_active.
  • Retry-schedule refactor — extracted RelayRetrySchedule + computeRetryDelayMs (replacing the drain-only RelayDrainRetrySchedule), adding a settled signal so a waiter sits through an armed retry instead of surfacing a cold-start hiccup; RelayOriginPool was migrated behavior-preservingly.
  • Diagnostics payload sharing — collectMobileRelayDiagnosticsPayload unifies both "Copy diagnostics" buttons and drops the selectedAddress that MobilePane previously leaked.
  • skipLinger carry-through + queued-revoke pin — scheduleRetry now carries the reconcile options, and a new test pins that a queued revoke still holds demand under LAN until it flushes (a revoke is cleanup, not a grant).

The offline-reason mapping, the settled wake-up ordering, and the queued-revoke decision all check out against the sources.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

if (this.isEpochCurrent(epoch)) {
// Retry still re-reads entitlement and demand; the timer grants no authority.
this.beginReconcile(false, expectedIdentityKey)
this.beginReconcile(false, expectedIdentityKey, options)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skipLinger is threaded through the retry chain, but the intent is still ephemeral. If the policy reconcile's readContext fails transiently and any other reconcile (authMutated, demandStateChanged, the demand-expiry timer, or the refreshDemand() a revoke flush fires) lands before the retry fires, beginReconcile's this.retry.cancel() drops the armed skipLinger retry, and the superseding reconcile() — which carries no skipLinger — schedules the full ten-minute linger.

Technical details
# Make the LAN-pick withdrawal sticky

## Affected sites
- src/main/runtime/relay/relay-auth-coordinator.ts:66 — `this.retry.cancel()` drops the armed
  `skipLinger` retry whenever a fresh `reconcile()` supersedes it.
- src/main/runtime/relay/relay-auth-coordinator.ts:206-215 — the superseding `reconcileEpoch` runs
  with `options === undefined`, so a still-valid ownership with no remaining demand falls into
  `scheduleLinger` instead of closing promptly.

## Required outcome
- A broker withdrawn by a LAN pick closes promptly even when a transient `readContext` failure is
  superseded by an unrelated reconcile before its retry fires.

## Suggested approach
- Track the withdrawal as sticky state on the coordinator (e.g. set by a policy-change reconcile
  and cleared only once the broker actually closes or demand returns), rather than as an ephemeral
  per-reconcile `options` flag — or re-derive `skipLinger` from the current policy in the no-demand
  branch instead of trusting the passed-in flag.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in this delta — non-approving only because the earlier skipLinger carry-through note (see relay-auth-coordinator.ts:295) is unchanged by this commit and remains open.

Reviewed changes

  • Named the mid-mint LAN flip — withTransientDemand now re-asks the live policy in its catch and rethrows relay_disabled_for_device when a LAN flip lands during an in-flight grant, so createPairingRelay / provisionRelay stop surfacing the generic relay_control_not_active for a failure the policy actually caused.
  • Added a policy-flip failure-cause test — desktop-relay-service-policy-flip-cause.test.ts pins all four outcomes: the flip names relay_disabled_for_device, unrelated causes pass through untouched, the entry gate still refuses, and a successful grant is left alone.

The re-ask logic is sound: once the flip clears the offline reason, requireActiveBroker can only throw the generic code, so checking the live policy in the catch is the correct signal, and the finally still releases the transient ref.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 9f3b02f2-69ff-4500-b749-10dc51706e3b

📥 Commits

Reviewing files that changed from the base of the PR and between 2aa0fdc and 9ce1902.

📒 Files selected for processing (2)
  • src/main/runtime/relay/desktop-relay-service-policy-flip-cause.test.ts
  • src/main/runtime/relay/desktop-relay-service.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

Comment on lines +298 to +306
} catch (error) {
// Why re-ask instead of trusting the thrown code: a flip lands mid-operation, and
// `hasDemand` filters this ref's own demand through the live policy, so the coordinator
// reaches `standby` and clears the offline reason. The wait then ends with no cause at all
// — the generic `relay_control_not_active` — when the flip is exactly the cause.
if (!this.isRelayAllowedForDevice(deviceId)) {
throw new Error('relay_disabled_for_device')
}
throw error

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | 🏗️ Heavy lift

Authorization Bypass

Reachability: External
Exploitability: Moderate
CWE: CWE-367 — Time-of-check Time-of-use (TOCTOU) Race Condition

Reject or compensate successful in-flight Relay grants after a LAN policy change.

Lines 298-306 only recheck the policy when operation() fails. A device can pass the entry gate, remain in createPairingRelay, getEndpoints, or installCredential, and then resolve after the host selects LAN. The service returns the Relay result in that case.

Recheck policy after a successful operation. If the policy now blocks Relay, revoke or otherwise compensate the minted remote grant before returning relay_disabled_for_device. Update src/main/runtime/relay/desktop-relay-service-policy-flip-cause.test.ts because lines 78-87 currently require this prohibited success path.

nwparker added a commit that referenced this pull request Sep 11, 2026
…online

Reproduced first: enqueue a revoke, reject it 50 times the way a deleted device
is rejected, and `hasDemand` is still true. It stays true for the life of the
install.

Two deliberate properties combine into the bug. `flushRevoke` removes the outbox
item only on a SUCCESSFUL flush and swallows every failure in a bare `catch {}`
with no attempt cap and no expiry, so a permanently-rejected revoke is pending
forever. And the revoke arm of `hasDemand` is deliberately NOT filtered by the
host's pairing-connection policy, because a credential must be killed on the relay
whatever the user has since chosen. Forever-pending plus policy-exempt means the
relay is held online permanently and a `local-only` pick is defeated outright —
the regression path for what #19870 closes, previously with no test at all.

THE CHOICE, stated because the convenient one is wrong. The revoke is NOT
abandoned. Failing to revoke leaves a live credential on the relay, so discarding
the intent to tidy up demand would trade a visible stuck relay for an invisible
live credential. The item stays in the outbox and keeps retrying on every future
connection, unchanged.

What expires is only its CLAIM ON DEMAND, via `demandingFor` alongside the
unchanged `pendingFor`: after 24h an unlanded revoke stops being a reason to force
the relay online BY ITSELF. The reasoning is that holding the relay open is not
what makes the revoke succeed — it has had a day of attempts — so holding it open
buys nothing and costs the user the setting they chose. A revoke that can still
land is unaffected inside the window, and one that lands is removed as before.

Uses the existing `createdAt`, so nothing new is persisted and the window survives
restart.

Mutation-tested: putting `pendingFor` back in `hasDemand` fails the new test at
the post-window assertion. The success path is pinned separately so the fix cannot
be mistaken for aging out an item that should have been removed.

Audited the rest of the file for the same shape, since a bare `catch {}` on a
queue is rarely alone: `flushRevoke` is the only one in desktop-relay-service.ts.
tan7vir and others added 7 commits September 11, 2026 01:07
Problem: every Anywhere/Relay pairing mint failure surfaced to the user
as the generic relay_control_not_active code, whether the real cause
was a revoked/absent cloud session, a missing relay.use entitlement,
or a broker cold-start/transport failure. RelayAuthCoordinator already
knew which of these happened on every path that publishes 'offline'
but discarded it, so DesktopRelayService.requireActiveBroker() could
only throw the one generic code.

Fix: RelayAuthCoordinator now records why it last went offline
(RelayOfflineReason, reusing the existing RELAY_HOST_CLOSE_REASON wire
vocabulary for signed-out rather than inventing a parallel constant)
and clears it once it goes back online. requireActiveBroker() maps
that reason to relay_signed_out / relay_not_entitled /
relay_broker_unavailable via relayOfflineReasonMintFailureCode(),
falling back to relay_control_not_active when no reason is known. The
code already flowed unchanged through mobileRelayMintFailureFromUnknown
into the mobile_pairing_relay_failure payload (verified with a
regression test), and neither renderer consumer maps failure.code to
UI text today (only .stage and a separate auth-status flag), so no
new user-facing strings or localization entries were needed.

Also extends the "Copy diagnostics" payload (MobilePage.tsx) with the
live relay status and app version via a new pure
buildMobileRelayDiagnosticsPayload() builder, deliberately omitting
any address (matching the existing LAN/Tailscale-leak precedent) —
only relayStatus and appVersion are added.

relay-auth-coordinator.ts and desktop-relay-service.ts were already at
this repo's 300-line oxlint ceiling, so the offline-reason mapping,
the retry-backoff jitter math, and the already independently-tested
pairingAuthorizationForContext helper were extracted into their own
small files (relay-offline-reason.ts, relay-retry-delay.ts,
relay-pairing-authorization.ts) instead of disabling the rule.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The Settings → Mobile pane had its own inline "Copy diagnostics" builder
that still wrote the selected address into a payload users are asked to
share, and lacked the relay status and app version. Both entry points now
call one collectMobileRelayDiagnosticsPayload, so they emit one shape
with no network identifier.

RelayDrainRetrySchedule reuses relayRetryDelayMs instead of carrying a
second copy of the same backoff constants and formula.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…retryable from rejected

- waitForLiveBrokerResult() returns { broker } | { broker: null, offlineReason }
  so requireActiveBroker maps the reason the wait actually observed instead of
  re-reading coordinator state after the wait returned.
- broker-open failures split three ways by what the failure proves:
  auth_unavailable (session read/refresh failed, never reached the relay),
  broker_unavailable (relay refused transiently, retry armed),
  broker_rejected (4xx, no retry armed).
- retry backoff extraction renamed to relay-retry-backoff.ts::computeRetryDelayMs
  with retryAfterMs folded in; RelayDrainRetrySchedule uses the same function.
- coordinator types move to relay-auth-coordinator-contract.ts to stay under
  the file cap without a lint bypass.
…e offline cause

- RelayRetrySchedule (was RelayDrainRetrySchedule) owns the single armed retry
  for both the coordinator and the origin pool; `settled` resolves when the
  retry fires or is cancelled so a waiter can sit through it.
- waitForLiveBrokerResult keeps waiting while a retry is armed and returns the
  observed offline cause only when the wait gives up. Terminal causes (signed
  out, unentitled, rejected) arm nothing, so they return immediately.
- The wait is budgeted (30s default, matching the phone's RPC budget) so a
  sustained outage fails the caller instead of holding a demand ref open.

Co-authored-by: Neil Parker <nwparker@users.noreply.github.com>
settings.mobilePairingConnectionMode was only consulted when minting the next
QR. The live question — may this desktop serve mobile over Relay right now —
had no implementation, so a device paired as `automatic` kept the desktop on
Relay forever after the user picked LAN (#18211).

- src/shared/mobile-relay-policy.ts: isMobileRelayAllowed composes the host
  setting with the per-device pair-time mode, restrictive-only.
- RelayDemandLedger consults the policy for standing bindings and for in-flight
  transient refs (now keyed with a device id) so in-flight work cannot outvote
  the policy.
- DesktopRelayService gates every grant path at withTransientDemand, which is
  what finally covers createPairingRelay; the host mode is pulled through an
  optional callback so existing prototype-built tests keep working.
- RelayAuthCoordinator.reconcile({ skipLinger }) closes the broker promptly on
  a deliberate policy change; pairing churn keeps its ten-minute linger.
- desktop-relay-startup.ts wires the settings read and the settings-changed
  wake signal; the launch file shrinks below the line budget.
- The Relay-mint-failure "Use LAN" recovery buttons no longer persist the host
  policy, since revoking Relay from every paired phone is not what they promise.

Closes #18211
…not_active

withTransientDemand checks the host pairing policy once, at entry. But
RelayDemandLedger.hasDemand filters the in-flight operation's OWN transient
ref through the live policy, so a flip to local-only while createPairingRelay
or provisionRelay is awaiting withdraws the demand that operation is holding.
The coordinator then reaches its no-demand branch and publishes 'standby',
publish() clears offlineReason to null for any non-offline status, and the
broker wait returns with no cause at all — relay_control_not_active, the
generic code this stack exists to remove.

The flip is exactly the cause and relay_disabled_for_device is already its
word; the entry gate just could not see a flip that had not happened yet.
Re-ask the policy on the failure path rather than trusting the thrown code.

Reproduced first: the added test failed with "expected ... to throw error
including 'relay_disabled_for_device' but got 'relay_control_not_active'".
Controls cover the two ways this could over-reach — a failure the policy had
nothing to do with is not rewritten, and a grant that succeeded under a flip
is left alone.

Top follow-up found alongside this and deliberately not fixed here:
flushRevoke swallows every error with a bare catch, no attempt cap and no
item expiry, while the revoke check in hasDemand is deliberately not filtered
through the policy. A revoke that fails permanently server-side therefore
holds demand open forever and defeats the LAN pick entirely — the same
symptom this change is part of closing, by a different route, with no test
coverage.
@nwparker
nwparker force-pushed the fix/18211-lan-mode-applies-to-paired-devices branch from 9ce1902 to 13eee7e Compare September 11, 2026 08:10
nwparker added a commit that referenced this pull request Sep 11, 2026
…online

Reproduced first: enqueue a revoke, reject it 50 times the way a deleted device
is rejected, and `hasDemand` is still true. It stays true for the life of the
install.

Two deliberate properties combine into the bug. `flushRevoke` removes the outbox
item only on a SUCCESSFUL flush and swallows every failure in a bare `catch {}`
with no attempt cap and no expiry, so a permanently-rejected revoke is pending
forever. And the revoke arm of `hasDemand` is deliberately NOT filtered by the
host's pairing-connection policy, because a credential must be killed on the relay
whatever the user has since chosen. Forever-pending plus policy-exempt means the
relay is held online permanently and a `local-only` pick is defeated outright —
the regression path for what #19870 closes, previously with no test at all.

THE CHOICE, stated because the convenient one is wrong. The revoke is NOT
abandoned. Failing to revoke leaves a live credential on the relay, so discarding
the intent to tidy up demand would trade a visible stuck relay for an invisible
live credential. The item stays in the outbox and keeps retrying on every future
connection, unchanged.

What expires is only its CLAIM ON DEMAND, via `demandingFor` alongside the
unchanged `pendingFor`: after 24h an unlanded revoke stops being a reason to force
the relay online BY ITSELF. The reasoning is that holding the relay open is not
what makes the revoke succeed — it has had a day of attempts — so holding it open
buys nothing and costs the user the setting they chose. A revoke that can still
land is unaffected inside the window, and one that lands is removed as before.

Uses the existing `createdAt`, so nothing new is persisted and the window survives
restart.

Mutation-tested: putting `pendingFor` back in `hasDemand` fails the new test at
the post-window assertion. The success path is pinned separately so the fix cannot
be mistaken for aging out an item that should have been removed.

Audited the rest of the file for the same shape, since a bare `catch {}` on a
queue is rarely alone: `flushRevoke` is the only one in desktop-relay-service.ts.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in this rebase delta — non-approving only because the earlier skipLinger carry-through note (see relay-auth-coordinator.ts:295) is unchanged by the rebase and remains open.

Reviewed changes

  • Rebased onto main — resolved the single conflict by keeping startDesktopPushService(runtimeRpc) then startDesktopRelayService(runtimeRpc) in main-process-runtime-launch.ts, and pointing the extracted desktop-relay-startup.ts at main's publishDesktopRelayStatus (from main-process-relay-status.ts) instead of re-inlining the onStatus callback.
  • Updated the startup-ordering test — main-process-ready-phase-ordering.test.ts now anchors on startDesktopRelayService( rather than new DesktopRelayService(.

The conflict resolution is sound: publishDesktopRelayStatus is semantically identical to the previously inlined callback (sets desktopRelayStatus/desktopRelayCellUrl, then sends mobile:relayStatusChanged through getDesktopRelayStatus()), the push service stays ahead of relay bring-up after the proxy await, and the imports (powerMonitor, MobileRelayStatusDetail, the inline DesktopRelayService block) are removed cleanly from main-process-runtime-launch.ts without leaving unused references.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

Resolves two conflicts:

- desktop-relay-service.ts: keep main's relay-region-preference-reader
  import path alongside this branch's added pairing imports. Main's
  measureRegionDecision wiring is untouched by this branch and preserved.

- relay-origin-pool.ts: keep main's RelayControlRotation constructor and
  parameter-property options, with this branch's RelayRetrySchedule in
  place of the deleted RelayDrainRetrySchedule.

Also disarms the drain retry on close (closeNow now calls cancel() rather
than reset(), which only zeroed the attempt counter and left the armed
timer running past close). This branch's RelayRetrySchedule.cancel() does
not reset(), unlike the RelayDrainRetrySchedule it replaces, so the old
closeNow call site had become a no-op for the timer.
The merge swapped RelayDrainRetrySchedule for RelayRetrySchedule, whose
cancel() deliberately keeps the attempt count (the coordinator gates its
own reset on resetRetry). closeNow still called reset(), which only zeroes
that counter and leaves the armed drain timer running past close.

Nothing caught it: reverting closeNow to reset() left all 222 relay tests
passing. This is #20258's teardown test, ported verbatim so the two merge
as identical content; it fails with reset() and passes with cancel().
The mint-failure "Use LAN" button called changeConnectionMode('local-only',
{persist:false}), which moved the radio without persisting. Two consequences,
both the wrong way round for #18211:

- The radio read LAN while host policy was still Relay, so the control lied
  about what every already-paired phone was actually getting.
- The equality guard (`nextMode === connectionMode`) then swallowed a later
  click on LAN, so the user could not persist LAN at all without toggling
  back through Relay first.

Root cause was one piece of state doing two jobs. Split them: connectionMode
stays host policy (what the radio shows, what persists), and a new
mintModeOverride carries "mint the next QR over LAN" for the recovery path
only. An explicit radio pick clears the override.

Both symptoms are pinned: reintroducing the radio move fails these tests.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Merging main brought the new casting gate (#19462) onto this branch, and this PR's test files now fail the blocking static analysis check — Enforce changed-code quality is red on head 353ddd2. The closeNow fix and its test are otherwise sound; only the type assertions need attention before merge.

Reviewed changes

  • Merged main into the branch (215b60dab3) — resolved the two conflicts by keeping main's relay-region-preference-reader import in desktop-relay-service.ts and main's RelayControlRotation wiring in relay-origin-pool.ts alongside this branch's RelayRetrySchedule.
  • Disarmed the drain retry on pool close — RelayOriginPool.closeNow() now calls drainRetry.cancel() instead of reset(), so an armed drain retry no longer leaves a timer running past close. RelayRetrySchedule.cancel() clears the timer (and resolves the settled waiter) without zeroing the attempt count, so the old reset() call had become a no-op for the timer.
  • Pinned the teardown property with a test (353ddd2) — relay-origin-pool-teardown.test.ts arms a director-refused drain retry through a real broker and asserts vi.getTimerCount() is 0 after closeNow(). I confirmed it fails with reset() and passes with cancel(), so it guards the right property rather than something incidental.

⚠️ The merged casting gate leaves static analysis red

main added a changed-lines casting scan (config/oxlint-code-quality-casting.json, typescript/consistent-type-assertions with assertionStyle: never, no test-file exemption). The merge pulled it onto this branch, and Enforce changed-code quality now fails on fifteen as casts on added lines across this PR's test files. That is the failing step of the static analysis job on 353ddd2; the fix is to give each cast a line-specific SAFETY:-rationale suppression or replace it with a typed declaration.

Technical details
# Add SAFETY: rationale to the flagged casts (or remove them)

## Affected sites
- `src/main/runtime/relay/relay-origin-pool-teardown.test.ts:8` — `controls: [] as {...}[]`
- `src/main/runtime/relay/relay-origin-pool-teardown.test.ts:55` — the `authConfig` object `as OrcaCloudAuthConfig`
- `src/main/runtime/relay/relay-origin-pool-teardown.test.ts:63` — `mobileSocketWiring` object `as never`
- `src/main/runtime/relay/desktop-relay-service-failure-cause.test.ts:18`
- `src/main/runtime/relay/desktop-relay-service-policy-flip-cause.test.ts:36, :45, :59, :70, :79`
- `src/main/startup/desktop-relay-startup.test.ts:5, :16, :17, :63`

## Required outcome
- `pnpm run check:code-quality:changed -- <base.sha>` exits 0, i.e. `casting code quality: 0 new finding(s)`.
- Every suppression that remains carries a `SAFETY:` rationale — the gate has a separate check that fails a casting directive without one.

## Suggested approach
- Add `// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: <verified invariant>` above each cast (or `oxlint-disable-line` for same-line casts), matching the `AGENTS.md` pattern.
- Where a cast is avoidable, prefer a typed declaration (e.g. annotate the hoisted `controls` array) over suppressing.

## Open questions for the human
- CI's annotations also include two `mobile/*` `Array.flatMap` warnings this PR never touches, which implies the job's comparison base includes main commits this branch already merged. Those are not yours to fix, but the base may need to advance before the job can go green.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Replaces the single drain-retry case with #20258's current it.each table over
every timer the pool and its broker can leave armed (rotation + lease refresh,
drain retry, retirement grace). Taken wholesale at c2d6cf3 so the two merge
as identical content rather than conflicting.

The mock needs pendingRequestCount as a getter over the hoisted fakes, not an
instance field, or the retirement case passes vacuously.

Measured: 3 passed; reverting closeNow to drainRetry.reset() fails the
drain-retry case only, as expected.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one minor consistency gap inline in the new mint override.

Reviewed changes

  • Stopped the LAN recovery button moving the host-policy radio — MobilePage / MobilePane now keep a separate mintModeOverride. The Relay-failure "Use LAN" button sets the override and mints a LAN QR without touching the radio or persisting policy, and an explicit radio pick clears the override. Previously the button set the local connectionMode to local-only, so the radio showed a policy that was not in force and the nextMode === connectionMode guard swallowed any later click on LAN.
  • Restored the shared diagnostics payload in MobilePage — the copy-relay payload now goes through collectMobileRelayDiagnosticsPayload instead of an inline object, matching MobilePane.
  • Updated the pairing tests — recovery tests now assert the radio keeps reading host policy and that a later LAN pick persists; added restores a saved local-only preference without user interaction.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

const [connectionMode, setConnectionMode] = useMobilePairingConnectionMode()
// Which path the next QR mints over, when that differs from host policy. The
// mint-failure LAN recovery sets it; the radio keeps showing host policy.
const [mintModeOverride, setMintModeOverride] = useState<MobilePairingConnectionMode | null>(null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mintModeOverride is honored by the generate paths, but the custom-address precheck at MobilePage.tsx:234 (beforeCustomAddressChange) still mints with the host connectionMode. After a LAN recovery under automatic policy, confirming a custom address runs that precheck against Relay — the very path that just failed — so the address is rejected with “This address could not produce a scannable pairing code” even though the LAN mint would succeed. Thread the override through the precheck (mintModeOverride ?? connectionMode).

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No new issues in this delta — non-approving only because the earlier review threads (the casting gate and the mintModeOverride precheck) remain open.

Reviewed changes

  • Widened the teardown ratchet to match #20258 — relay-origin-pool-teardown.test.ts now parameterizes over three armed states (an idle live broker, a director-refused drain retry, and a retirement grace armed by a cross-cell rotation) and asserts vi.getTimerCount() is 0 after closeNow() for each. The getTimerCount() > 0 guard before close means none of the three cases can pass vacuously, so the ratchet still fails if rotation.cancel(), drainRetry.cancel(), or retirement.clear() is dropped. Test-only; verified all three pass on this head.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: f8603881-6271-4bcb-b04f-40bee999ab66

📥 Commits

Reviewing files that changed from the base of the PR and between 13eee7e and 32eb062.

📒 Files selected for processing (8)
  • src/main/runtime/relay/desktop-relay-service.ts
  • src/main/runtime/relay/relay-origin-pool-teardown.test.ts
  • src/main/runtime/relay/relay-origin-pool.ts
  • src/main/startup/main-process-runtime-launch.ts
  • src/renderer/src/components/mobile/MobilePage.test.tsx
  • src/renderer/src/components/mobile/MobilePage.tsx
  • src/renderer/src/components/settings/MobilePane.test.tsx
  • src/renderer/src/components/settings/MobilePane.tsx

Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.

Comment on lines +155 to +156
setMintModeOverride('local-only')
void generatePairing(false, undefined, 'local-only')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Scope the LAN recovery override to one mint. Both callbacks retain local-only, so later Relay retries use LAN while the host policy remains automatic.

  • src/renderer/src/components/mobile/MobilePage.tsx#L155-L156: remove the persistent override or clear it after the recovery mint completes.
  • src/renderer/src/components/settings/MobilePane.tsx#L292-L295: remove the persistent override or clear it after the recovery mint completes.
📍 Affects 2 files
  • src/renderer/src/components/mobile/MobilePage.tsx#L155-L156 (this comment)
  • src/renderer/src/components/settings/MobilePane.tsx#L292-L295

check-changed-code-quality.mjs lints the merge commit with current main's
rules against a merge base predating them, so every `as` in a file this PR
adds counts as new. Local oxlint stays green throughout; the gate reported 15.

Four of them were removable rather than suppressible: annotate the variable
(`const mode: { current: MobilePairingConnectionMode }`) instead of asserting
the literal. The rest carry a line-specific SAFETY rationale.

relay-origin-pool-teardown.test.ts is re-taken wholesale from #20258 at
b3783ad, which already carries its three directives, so the two still
merge as identical content.

Measured: gate exit 0 (casting and SAFETY-rationale both 0 new), tc exit 0,
relay+startup 83 files / 728 tests passed.
@Jinwoo-H

Copy link
Copy Markdown
Contributor

Closing: the Connection radio lives under "Pair a phone" and is per-pairing by design. A phone paired on Relay keeps Relay, a phone paired on LAN keeps LAN, and the radio only decides what the next QR encodes. Turning it into a host-wide policy that withdraws Relay from already-paired phones is a semantics change we do not want. #18211 is closed as working as intended. The PRs stacked on this one (#20055, #20061, #20063, #20064, #20065, and #19963 via #20055) are independent correctness fixes and should be re-based onto main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Mobile pairing connection mode LAN does not apply to already-paired devices — desktop keeps a Relay session alive forever

4 participants