Repository navigation
fix(web): switching both T3 Connect switches off unlinks this PC - #178
Conversation
Switching T3 Connect off while publishing is on needs a relink in publish-only mode. With another account signed in, this PC refuses that relink, so the switch stays ON. Switching publishing off next then only wrote the preference, because a tunnel still counted as wanted. The link stayed and unlink was never called. A refused T3 Connect switch-off is now carried into the next request, so switching publishing off next unlinks. Only the request right after the refusal carries it. Also in this change: - The Unlink this PC row stays when the link state cannot be read, and says the link is unknown. - The different-account conflict has a typed reason in the contracts package. The client matches on it and no longer reads the message. - Unlink this PC sends the signed-in account's token with the relay revoke when the accounts differ. That clears the entry the refused link attempts left under the signed-in account. - After an unlink that leaves the linking account's relay entry in place, a warning says that account still lists this PC until it is removed there with Deregister.
github-actions
Bot
added
vouch:trusted
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
Review - standards and specTwo independent read-only reviews of c20f115. Security limits hold: Blocking, to fix before merge:
Should fix in the same round:
Noted, not blocking: four concerns in one pull request (the task asked for that); naming and duplication points in the reports below. Full reports follow. Spec review - PR 178The four points are implemented and the security limits hold. Nine findings; the main one is that the carried switch-off never expires. Nothing was run; this is from reading the code. Confirmed safe
(c) Implemented, looks wrong
(a) Missing or partial (b) Scope beyond the brief, for the user to accept
Upstream file additions are small. The Next action: decide whether finding 1 needs a fix before merge. Standards review of PR 178: no hard blocker. One hard violation, eleven judgement calls. Hard violation
Documented standards, judgement calls
Baseline smells, all judgement calls
Clean
Nothing was run or changed. This is a read of the diff and the four standards files. Next action: decide on finding 3, the onboarding path, first. |
Review of the previous commit found the carried switch-off too wide. It was set on any failed T3 Connect switch-off, never expired, and also wrapped the first-run Connect dialog. - The switch-off is carried only when this PC refused it for the typed different-account reason. A network error, a missing token or a signed-out session no longer sets it. - It only counts for the request that directly follows, and only while the signed-in account, the environment and the link state are unchanged. - Only the two Settings switches use it. Onboarding keeps the plain reconcileCloudState and never unlinks. - The local unlink runs first, on the row and on the switches. The relay revoke follows on its own, so a token read that never settles no longer holds the unlink up. The revoke is skipped when the PC has been linked again by the time the token arrives. - The warning after an unlink is worded per case: another account, signed out, or the user's own account when the revoke did not go through. The row and the switches warn in the same cases. - The unlink toasts no longer depend on the relay list refresh. - The conflict reason is a plain string on the wire, so a reason from a newer server still decodes and falls back to the plain message. The reason type is exported from the contracts package and used by the web client and the server.
Follow-up to #177. It fixes the four points that review left owed, and the points from the review of this pull request's first commit.
1. Why switching both switches off did not unlink
Cause
reconcileCloudStateunlinks only when the request asks for no tunnel and no publishing.ConnectionsSettings.tsxbuilds each request from the switch that moved plus what the server holds for the other one.The PC held a link made under account A, with the tunnel up and publishing on. Signed in as account B:
{ managedTunnel: false, publish: true }. A link is still wanted, and the tunnel mode changed, so the client relinks in publish-only mode with B's token. The relay accepts. The PC refuses the relay config with the different-account 409 (validateLinkedCloudUser). The switch stays ON.{ managedTunnel: true, publish: false }, because the server still holds a tunnel. A link is still wanted and the tunnel mode did not change, so the client only writes the preference.The request
{ false, false }is never made, soenvironment.cloud.unlinkis never called. This matches the reported server trace: link proof accepted,applyRelayConfigrefused, then preference writes.The other order works on
main. Publishing off first is a preference write. T3 Connect off second asks for{ false, false }and unlinks. I ran that order onmainas the check that could have disproved the cause. It unlinked.Fix
A refused T3 Connect switch-off is carried into the next request from the Settings switches. If that request is "publishing off", it unlinks. The carry is narrow on purpose:
linked_to_different_cloud_account. A network error, a relay failure, a token failure or a signed-out session does not set it.reconcileSettingsSwitches). The first-run Connect dialog keeps the plainreconcileCloudState, so onboarding can never tear down a link.I did not carry a switch-off that the relay refused. The client cannot tell a relay refusal of the publish-only downgrade from a relay outage with certainty, so that case keeps the link, as on
main.The switch still shows ON after the refusal, as before. No server logic changed: the unlink is the same local call and clears only this PC's own link.
Real run
Throwaway dev server on an empty home under
%TEMP%/fm-unlink-followup. Made-up account ids. Nobody signed in to any service. The link under account A was made through the real endpoint:A headless browser then clicked the two real switches, signed in as made-up account B.
Before, on
main(f690eff):After, on this branch (be464e4), same state and same clicks:
The toast list is top first. The warning is on top.
On this branch, with the relay failing the link request as an outage would, the same two clicks keep the link:
main: T3 Connect off is refusedmain: publishing off, still linkedLimits of this run, none of which changed product code:
@clerk/reactmodule. It reports a signed-in made-up account B and a made-up token. No sign-in service was contacted.infra/relaydoes. The live relay was not used.T3CODE_CLOUDFLARED_PATHset to a stand-in executable, so the made-up connector token counts as a running tunnel. No tunnel existed.window.desktopBridgewas set after boot, as in fix(web): Unlink this PC clears a T3 Connect link from any state #177. The T3 Connect row only renders when it exists.2. The row when the link state fails to load
linkedfalls back tofalsewhen the link state cannot be read, so the row was hidden. The controller now returnsunlinkOffer:linked,unknownornone. The row shows forlinkedandunknown, and its wording forunknowndoes not claim a link exists.Real run, with the link-state response replaced by a 500 in the headless page and a real link on the server:
main: no row3. Typed conflict reason
packages/contractsexportsEnvironmentConflictReason, a schema and a type with one value:linked_to_different_cloud_account.EnvironmentHttpConflictErrorgains an optionalreason.validateLinkedCloudUsersets it, typed againstEnvironmentConflictReason. The check itself is unchanged. On the wire:The web client finds the decoded error in the failure's cause chain and compares
reasonwith a constant of typeEnvironmentConflictReason. The substring match on the message is gone.Compatibility:
apps/mobile/src/features/cloud/linkEnvironment.tsdecodes the same error and reads onlymessage. It decodes any reason value for the same cause. I left mobile unchanged: the "Unlink this PC" row lives on the host, so mobile has no control to point at and keeps showing the server's own text, which already says to unlink before switching accounts.4. Stray relay entries, and what the user is told
Read in
infra/relay, not checked against the live relay:relay_environment_linkshas the primary key(user_id, environment_id)(persistence/schema.ts).revokeForUserfilters on the caller's user id and the environment id (environments/EnvironmentLinks.ts). The caller's id comes from the token. So a revoke with B's token removes B's entry and cannot touch A's.revokeForEnvironmentPublicKey), so B's revoke does not break A's entry either.Unlink order, the same for the row and for the switches:
A token read that never settles, a failed revoke or a failed relay list refresh cannot block or undo step 1, and cannot drop the toasts. The check in step 3 exists because the token can arrive late: if the PC was linked again in the meantime, the revoke is skipped so it cannot remove the new entry.
Warning after an unlink of a known link, the same on both paths:
Real run on this branch, Unlink clicked on a link made under account A:
User-visible strings as they now read
Row, title "Unlink this PC", button "Unlink":
Success toasts:
Warning toasts:
Error, when this PC refuses a link for another account: "This PC is already linked to a different T3 Connect account. Use "Unlink this PC" in Settings > Connections, then try again." (unchanged wording, now matched by reason)
Docs,
docs/user/remote-access.md: "If you are signed out, or signed in to a different account, the account that made the link still lists the PC until you sign in as that account and choose Deregister."Review points taken and skipped
Taken:
CloudLinkDesiredStateis reused in place of a copy.reconcileSettingsSwitchesfor the switch entry point,refusedTunnelOffInfor the state a switch-off was refused in.Skipped, with the reason:
useForkUnlinkThisPc. The row and the switches now share one unlink, one revoke and one set of warnings. Splitting would put that shared part behind a third piece.offerprop. It matches its type,UnlinkThisPcOffer.Tests
apps/web/src/cloud/forkUnlinkThisPc.test.tsxmountsuseCloudLinkControllerwith the commands mocked. It asserts what the user ends up with: whether the PC was unlinked, whether the link was kept, and what the user was told.apps/server/src/server.test.tsgains one assertion on the 409 body.On this branch:
Against the first commit of this pull request (c20f115)
This shows the review-round tests fail before the review-round fix. I restored that commit's source files and kept the new test file. That commit has no
reconcileSettingsSwitches, so I added one line to its controller that exposes its wrappedreconcileCloudStateunder that name. The two "never settles" tests hang on that commit, so I ran them apart from the rest.By review point:
Against
main(f690eff)From the first commit, unchanged: with
main's sources and that commit's 17 tests, 11 failed and 6 passed. The server assertion fails onmainwithexpected undefined to equal 'linked_to_different_cloud_account'. The four original points each have a test that fails onmain: the refused switch-off then publishing off (point 1), the unreadable-state offer (point 2), the typed reason with a reworded message (point 3), the other-account warning (point 4).Checks
The two red results are this Windows machine, not this change. CI runs both on Linux and is green.
Server. The 3 failures are in
src/cloud/bootService.test.ts, which this change does not touch.src/server.test.tspasses. The full server suite ran on the first commit: 16 failures in 10 files (cli/update,cloud/bootService,device/LocalDeviceHost,mcp/McpHttpServer,project/AgentSessionScanner,provider/Layers/OpenCodeProvider,provider/userInputAttachments,serviceLauncher,vcs/GitVcsDriver,vcs/GitVcsDriverCore). The same 10 files withmain's sources:The failing test names were identical on both. The server change since then is one type annotation, so I reran only
src/server.test.tsandsrc/cloudthis round.Fork features check. The 11 failures are all in
scripts/build-desktop-artifact.test.ts. Withmain's sources:Every other fork feature test passes, including
forkUnlinkThisPc.test.tsx.Not changed
validateLinkedCloudUserstill refuses a relay config from another account. It only adds the reason to the error it already returned.relay:writescope. No authentication or origin check changed.Surfaces
ConfiguredCloudLinkRow, shared by every place that renders the T3 Connect rows. The first-run Connect dialog keepsreconcileCloudStateas upstream wrote it.EnvironmentHttpConflictError, and the exported reason schema and type.docs/user/remote-access.md.fork-features.json: thet3-connect-unlink-this-pcentry describes the behaviour, lists the files and gives the keep rule for each.Left for later
infra/relaywas not checked.