Skip to content

refactor(runtime): paired sockets authenticate to a device principal - #27480

Merged
OrcaWin merged 3 commits into
mainfrom
np/d2-device-principal
Oct 11, 2026
Merged

OrcaWin merged 3 commits into
mainfrom
np/d2-device-principal

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Oct 11, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 12 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​233 $\color{#cf222e}{\Huge{\mathbf{−}}}$​21 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​212
Prod 10 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​119 $\color{#cf222e}{\Huge{\mathbf{−}}}$​56 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​63

ELI5

Inside the desktop, a connected phone or paired computer is now tracked by its device ID, not by its secret pairing code. Nothing changes for users. This is the groundwork that later lets a device sign in with a key instead of a reusable code.

What Changed

Before: the host used one string, the device's pairing token, as its identity everywhere:

  • the socket's client id, used for close bookkeeping and for "does this device have other live sockets";
  • the revoke key that terminates sockets;
  • the argument to per-request re-validation (validateToken(token) on every request);
  • the orchestration caller fingerprint (sha256(token)).

After: an authenticated socket carries a DevicePrincipal: { deviceId, scope, clientId, callerFingerprint, credentialRevision }, built once from the registry row at auth time.

  • Socket bookkeeping (transport client id, terminateDeviceConnections, the other-live-sockets check on close) is keyed by deviceId. Revoke paths pass the device id.
  • Every request on an authenticated socket re-resolves the row by (deviceId, credentialRevision) (plus the token for token rows). A revoke, or any later credential change that bumps the revision, refuses the socket's next request.
  • DeviceEntry gains two optional top-level fields, credentialRevision and callerFingerprint. Rows written today never set them, the loader validates them, and an older build's loader keeps them (it spreads unknown fields).
  • fingerprintAuthenticatedPairingCredential moved into device-principal.ts; the formula is unchanged.

Deliberately unchanged for token rows: clientId, the key for per-client runtime state such as terminal drivers, dictation and file grants, is still the token. The caller fingerprint is still sha256(token). Shipped phones stamp their token into terminal client.id, mobileClient.id and subscription ids, so keeping it avoids rewriting request params for old phones. Rows without a token (device keys, in a later PR) will use their id.

Why

The pairing token currently does four jobs at once: secret, credential, identity and authority. Device-key sign-in cannot ship while identity is the secret. Splitting identity first, with no wire change, keeps the later credential PR small. Binding sockets to a revision gives that PR a way to fence sockets authenticated under an old credential.

Alternative considered: rewrite params.client.id to the device id for every paired caller. Rejected for now. Desktop runtime clients use distinct per-pane client.ids that must not be merged, and old phones embed the token in several params besides client.id. Keeping the token as the client id for token rows is behavior-identical.

Linked Issue

Part of #17394 / #17398 (groundwork; no issue closes here)

Visual Proof

N/A: no UI or wire change.

Testing

  • New src/main/runtime/device-principal.test.ts:

    • principal shape for token rows;
    • optional fields round-trip, and malformed values are dropped on load;
    • resolveAuthenticatedPrincipal refuses a removed row, a revision mismatch and a token mismatch;
    • an old phone's request is dispatched with its token as clientId and an unchanged fingerprint, and is refused once the row's revision changes.
  • mobile-socket-wiring.test.ts: the transport client id is now the device id; new case for two live sockets of one device (hasOtherConnections).

  • Fixtures building E2EEAuthenticatedDevice updated for the new required principal fields.

  • pnpm tc, pnpm test src/main/runtime, oxlint, check:code-quality:changed.

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Mixed versions: no wire change; old phones keep their token as the per-client key. The registry gains only optional fields, which no code writes yet.
  • SSH / Relay / WSL: Relay sockets use the same wiring and are keyed by device id the same way. Nothing is platform-specific.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 73d11dda-57d1-438e-b49c-8b8c87b699d8

📥 Commits

Reviewing files that changed from the base of the PR and between 2af11656d0f4c8c62df2481e0897a7a61196c6d5 and 0e73592.


You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 50aec0f1-764a-47a2-8751-87c70caa76a4


📥 Commits

Reviewing files that changed from the base of the PR and between 2ed980dace1c73d34b576eeedfbd487ea82b1c90 and 2af11656d0f4c8c62df2481e0897a7a61196c6d5.



📒 Files selected for processing (1)
  • mobile/src/tasks/mobile-agent-launch-architecture.test.ts


Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.




📝 Walkthrough
📝 Walkthrough

Walkthrough

The changes add a shared device-principal type and helpers, normalize optional principal fields when loading registry rows, and validate authenticated principals against the current device revision and token. Socket tracking, RPC dispatch metadata, connection termination, and disconnect callbacks now use principal fields and device IDs. Tests cover legacy defaults, malformed registry values, principal mismatches, and multiple sockets for one device.



Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 2af11

This PR changes paired-socket identity, and the reviewed mobile test fixture now supplies the corresponding principal and token. No concrete user-impacting failure is established, so the remaining merge risk is minimal.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 18.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 21 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: paired sockets now authenticate to a device principal.
Description check Passed The description is complete and aligned with the changes. It explains the user impact, implementation, rationale, compatibility behavior, linked issues, testing, and lack of UI changes. Manual testing…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.



✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR



  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@OrcaWin
OrcaWin force-pushed the np/d2-device-principal branch from 2af1165 to 0e73592 Compare October 11, 2026 14:02
@OrcaWin
OrcaWin merged commit c414640 into main Oct 11, 2026
31 checks passed
@OrcaWin

OrcaWin commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator Author

Review (P0/P1 only): codex reviewer was unavailable (orchestration consumer_fenced, retried) so an Opus reviewer ran instead — No P0/P1 findings (14-item coverage log: disconnect cleanup keys, revoke targeting across ws/relay transports, delegated children, per-request re-resolution, registry downgrade). Own review: no P0/P1; two nits fixed (formatting, stale comment). One CI failure (mobile launch-authority test fixture lacked the principal) fixed in-PR.

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.

1 participant