Repository navigation
fix(server): only offer desktop app updates the desktop can install - #30804
juliusmarminge wants to merge 2 commits into
Conversation
…ithout updater These tests reproduce #30784: a desktop-mode server with a control fd advertises desktopAppUpdate and desktopAppChannelSwitch even when its desktop reports it cannot update (ServerConfig desktopAppUpdate: false). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…install them A desktop-managed server advertised desktopAppUpdate (plus the channel switch and self-update progress capabilities) whenever it had a desktop control fd. Builds whose updater is disabled, such as an extracted AppImage run without $APPIMAGE, therefore offered remote updates that then failed. DesktopUpdates.configure now records whether the updater has a disabled reason in DesktopState. It runs before the first backend start. The primary backend bootstrap carries that as desktopAppUpdate, and ServerEnvironment advertises the capabilities only when it is true. A nightly channel blocked by policy still counts as updatable, because the user can switch back to stable. Fixes #30784 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — Desktop update offers now reflect whether the desktop’s existing updater can actually install them. The change preserves update policy and installation behavior, with a reported Linux runtime comparison confirming unsupported installs stop advertising updates while AppImage installs retain them. You can add or adjust custom eligibility rules. Learn more. |
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. |
|
No actionable comments were generated in the recent review. Walkthrough
Merge Risk:
|
Wladefant
left a comment
There was a problem hiding this comment.
Worker16 — independent technical review (COMMENT; not APPROVED)
Receiving identity: setup marker HL-SB-20261102; registry task Superboard OS and desktop, task ID 6ae9048924888191ba619eb5249ca846; active invocation 0babc564-c830-8328-88ed-b3eabc0cdb74. Review scope recorded on Superboard #1778 comment #6630164022.
Candidate: T3 Code PR #30804, branch fix/issue-30784, exact HEAD 8ccb2b02f28c559cb6b78b05f2d37d4246d40f93 → main@8b44f42b14c8e273156474a924ccf29680dd7857; all ten changed paths were inspected.
Review result: no demonstrated scoped P1/P2 source defect.
Source/dataflow checks
DesktopUpdates.configureruns before desktopbootstrapinDesktopApp.ts, so the capability Ref is set before the first backend launch. The app foundation supplies one sharedDesktopState; the backend reads it for each primary resolve/restart.- The updater's existing disabled-reason predicate covers missing feed config, development/unpackaged builds, managed-policy
off, the disable env flag, and Linux installs that are neither AppImage nor .deb. The nightly-channel restriction remains separate, correctly preserving the ability to switch back to stable. - The WSL bootstrap path omits the desktop control fd as before, so it does not gain desktop-update authority from this flag.
- Server config carries the optional bootstrap boolean;
ServerEnvironmentadvertises desktop update/channel-switch/progress only for a desktop-managed server withdesktopAppUpdate === trueand the existing control fd. Omitted/false values fail closed.
CI evidence read independently: normal PR workflow #41824672117 completed successfully. The Test job recorded the focused desktop suite at 59/59 across six files; Test Server 7 recorded ServerEnvironment.test.ts 8/8. Lint, Build and Typecheck, selected server shards, and workflow summary also succeeded. The test/build jobs checked out the synthetic PR merge candidate 62aaae8a9a7b00c3709cb70b964d4765d9df2cbb (PR head merged with the then-current base), not a raw head-only checkout. Desktop macOS preview and other preview workflows were skipped.
Remaining evidence boundary: author-supplied Linux AppImage/extracted-install runtime transcript is useful but was not independently reproduced in this review. Packaged Windows/macOS behavior and a full remote-client update handoff remain unverified; native host acceptance is NOT RUN, not a blanket source-merge veto.
Nonblocking P3 coverage suggestion: add an undefined desktopAppUpdate case to the ServerEnvironment capability table while the control fd is present. The field is optional in the bootstrap schema, and the strict-true fail-closed behavior for an omitted/older payload is correct but not explicitly locked by the current false/true cases.
This is a technical COMMENT only. It is not formal APPROVED, a distinct-account endorsement, merge authorization, CI admission, or host/device acceptance. No source or branch changes were made.
| }), | ||
| ); | ||
| it.effect.each([ | ||
| { desktopAppUpdate: false, advertised: false }, |
There was a problem hiding this comment.
Nonblocking P3 coverage suggestion: add desktopAppUpdate: undefined with desktopTelemetryControlFd present. The bootstrap field is optional, and this strict-true check intentionally fails closed for omitted/older payloads; the current false/true table does not explicitly lock that compatibility boundary. The implementation appears correct.
Problem
A server run by the desktop app advertised
desktopAppUpdatewhenever the desktop's control fd was present, even when the desktop's own updater was disabled. On Linux installs that are neither an AppImage nor a.deb(for example a pacman package that ships the extracted AppImage), remote clients and the host's own UI offered an update that always failed with "Automatic updates on Linux require the AppImage or the .deb package."Reproduced on main 8b44f42: I built the Linux AppImage, extracted it the way the pacman package does, and ran it without
$APPIMAGE./.well-known/t3/environmentreporteddesktop-managedwithdesktopAppUpdate: true, andserver.updateServerWithProgressfrom a second paired client failed with that error.Change
desktopAppUpdate.DesktopUpdates.configurederives it from the existingresolveDisabledReason, so the rule for whether updates work still lives in one place. It sets the value onDesktopStatebefore the first backend start, and every start and restart reads it again.ServerConfig.ServerEnvironmentadvertisesdesktopAppUpdate,desktopAppChannelSwitchandserverSelfUpdateProgressonly when the desktop reported that it can install updates.desktop-managedhost withoutdesktopAppUpdateby saying the desktop app on that machine has to be updated.Scope and approval
Fixes #30784
Triaged via the automated issue flow.
Verification
apps/server/src/environment/ServerEnvironment.test.ts, testadvertises desktop app updates only when the desktop can update: thedesktopAppUpdate: falserow fails on main (advertises desktopAppUpdate although the desktop cannot update: expected { desktopAppUpdate: true, … }) and passes here. Result: 8/8 passed.apps/desktop/src/backend/DesktopBackendConfiguration.test.tschecks that the primary bootstrap carries the updater's state.apps/desktop/src/updates/DesktopUpdates.test.tschecks thatconfigurereports a.debinstall as updatable and an unmarked Linux install withoutAPPIMAGEas not. Result: 59/59 passed.vp run check:changedpassed: format, lint, knip, typecheck of 36 packages, and tests in 5 files.Runtime transcript. I built a real
vp run dist:desktop:linuxAppImage, ran--appimage-extract, added a release-styleresources/app-update.yml(GitHub feed), and launched it under Xvfb with an empty T3 home. Capabilities excerpt:The same mechanism also reproduced on main with a built but unpackaged desktop app: it advertised
desktopAppUpdate: true, and the update failed with "Automatic updates are not available because no update feed is configured."Not checked: macOS and Windows packaged builds. Their updater has no disabled reason, so the code keeps advertising the capability for them. Also not checked: a full desktop update handoff from a remote client. The fix does not change that flow.
There is no UI change. Clients render the existing "update the desktop app on that machine" state, so this PR has no captures.
AI-assisted; verified with the tests and tools listed above.
🤖 Generated with Claude Code