fix(update): a failed update check must say so, not look like "up to date" - #792
Conversation
…date" Three ways the update check could fail silently: - android_updater.dart logged a warning and returned null when the release fetch failed, which is indistinguishable from "no update available". It now throws a typed UpdateCheckException carrying the reason. - update_check_service.dart called checkForUpdate() from two places with no guard, so a failure escaped as an unhandled error. Both sites now catch and log; the error state was already emitted by then. - UpdateHandler answered nothing useful on a platform that cannot check. It now replies with an explicit "not supported on this platform" plus the releases URL, gated on the new UpdateCheckService.canCheck. Verified: flutter analyze clean; update_check_service, android_updater_download, update_handler and app_update_state 48 passed; dart format clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tadelv
left a comment
There was a problem hiding this comment.
Two required fixes before merge: keep the shared GitHub User-Agent as Decaid, and update the authoritative WebSocket spec for the new macOS check command behavior. The implementation/tests otherwise look coherent, and CI is green.
Both from @tadelv's review on decentespresso#792. The shared GitHub User-Agent goes back to `Decaid`. It had become `Decide` here, which is canary identity that should never have reached a PR — the header is shared by every GitHub call, not this fork's. `assets/api/websocket_v1.yml` now documents what `{"command": "check"}` does on macOS: it is not supported there and replies with a transient `{"error", "url"}` carrying the releases page, rather than polling. The spec already described that shape for `install`; `check` joins it, which is what this PR's canCheck gate introduced. Verified: flutter analyze clean, the four update test files 48 passed, dart format clean under Dart 3.13.3 (the version CI uses), websocket_v1.yml parses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tadelv
left a comment
There was a problem hiding this comment.
The two code/spec findings from my previous pass are fixed: the unrelated User-Agent change is gone, and assets/api/websocket_v1.yml now matches the macOS check behavior. I found no new implementation blocker in the updated diff, and current-head CI is green.
One required repository-guidance item remains from the existing WebSocket review thread: there is still no evidence of the prescribed runtime smoke check for the changed /ws/v1/update behavior. AGENTS.md requires API/spec changes to be exercised via scripts/sb-dev.sh + websocat/curl, and .agents/skills/decent-app/verification.md explicitly says runtime-facing changes need the relevant sb-dev smoke before completion. The follow-up commit records analyze/tests/format/spec parsing, but not that endpoint smoke. Please run the affected /ws/v1/update flow against a running app and add the result to the PR verification. Once that is recorded, I don't see another required change.
|
The Run on Linux from an isolated worktree checked out at this branch's head
{"phase":"idle","currentVersion":"0.8.5","latestVersion":null,"releaseNotes":null,
"releaseUrl":"https://github.com/decentespresso/decaid/releases","installable":false,
"progress":null,"error":null}
The The failure path, which is what this PR is about. A successful check that That is the behaviour this PR exists for, on the live endpoint. I then restored What the smoke could not reach, stated plainly. The macOS Both API specs parse. |
upstream/main moved to 9cbc20f. Its cf8348c, "respect externally managed app builds", rewrote lib/src/services/update_check_service.dart, which this lane also changes. Merged rather than rebased, so the three-dot diff stays the lane's own work. One conflict, in that file, resolved as a union. Upstream's externallyManaged field, its availableUpdate getter, and its three early returns are kept, and so are the lane's canCheck and canInstall gates and the error emission in the checkForUpdate catch block. In the merged checkForUpdate the externallyManaged return comes first, then the macOS return, then the body, so an externally managed build reports idle exactly as upstream intends. flutter analyze clean. update_check_service_test, update_handler_test and settings_view_updates_test all pass. The full suite is 3989 passed, 1 skipped, 8 failed, and all 8 failures are in test/webui_support/webui_token_injection_test.dart with "Address already in use, port = 3000": another process on the build box held that port. This lane touches no server binding.
|
Brought current with
The one conflict resolved as a union. Your One interaction worth your call, which I did not decide in a merge: Verification: On the local full suite I have to be exact rather than claim green: 3989 passed, |
tadelv
left a comment
There was a problem hiding this comment.
The requested runtime smoke is now sufficient, and I’ve resolved the prior WebSocket/spec thread. Current-head CI is green.
One new blocker appeared when this branch merged the newer main: upstream now has externallyManaged app builds whose checkForUpdate() intentionally returns without polling. canCheck still only excludes macOS, so an App Store/TestFlight build reports itself as check-capable; /ws/v1/update accepts {"command":"check"}, calls requestCheck(), and produces no command-level refusal — effectively recreating the silent unsupported-check behavior this PR is fixing.
Please fold externallyManaged into the capability gate, update the AsyncAPI wording accordingly, and add service/handler coverage for that case. I don’t see another blocker beyond this upstream-integration fix.
canCheck was `!_isMacOS`, so an App Store or TestFlight build reported itself as
check-capable. `/ws/v1/update` then accepted `{"command":"check"}` and called
requestCheck(), whose externallyManaged branch clears the update and emits idle.
The client received a frame indistinguishable from a real "checked, up to date" —
the failure this lane exists to end, reached by a second route.
canCheck now reads the same `externallyManaged` field checkForUpdate() reads, so
the gate and the no-op cannot disagree. The refusal names the build rather than
the platform, because on an App Store build the platform is not the reason.
The AsyncAPI description and doc/Api.md carry the ownership case; doc/Api.md had
never been touched by this lane, which CONTRIBUTING.md requires for a WebSocket
change. A scenario covers the refusal end to end.
Tests at both tiers: the service pins canCheck false for an externally managed
build and that the two clauses compose; the handler pins the reply and that
requestCheck is never called. Both fail with the one line reverted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Fixed in The gate. One correction to your description, and it makes the bug worse rather than better. You wrote that the handler "silently settles/returns". It did not: on a non-macOS The refusal now names the build, not the platform — Nothing else changes. Spec and docs. Tests, both tiers you named. The service test pins
|
tadelv
left a comment
There was a problem hiding this comment.
The externally-managed capability bug from my previous pass is fixed correctly: canCheck now matches checkForUpdate(), the AsyncAPI and doc/Api.md cover the ownership case, the service/handler tests pin it, and current-head CI is green against current main. I resolved that old thread.
One verification blocker remains in the new follow-up. The newly added E2E scenario is meant to catch regression of the externallyManaged clause, but its precondition does not require a non-macOS target. On macOS the scenario still passes if !externallyManaged is removed, because !_isMacOS independently refuses the command. That makes the persistent regression recipe unable to distinguish the new ownership gate from the older platform gate on the most obvious developer host. The latest PR comment also reports focused tests but not a run of this new scenario; the earlier sb-dev smoke predates the externally-managed fix and used an ordinary Linux build.
Please make the scenario explicitly exercise APP_STORE=true on a non-macOS target (or split the macOS and externally-managed cases so each clause is independently testable), then run that managed-build scenario on this head and record the result. I found no additional production-code blocker beyond that verification issue.
canCheck is `!isMacOS && !externallyManaged`, and either clause alone refuses the command. The scenario set APP_STORE=true but named no host, so on macOS the platform clause refused first and the recipe passed with `!externallyManaged` deleted — it pinned nothing. Its control section had the same fault in reverse: an ordinary build does not poll on macOS either. Part A is the ownership clause and requires a non-macOS host; its control run on the same host is what shows the refusal came from the build and not the platform. Part B is the platform clause and requires macOS. Each states what deleting its clause does: the ownership clause leaks an idle state frame, the platform clause emits nothing at all and the client waits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You were right, and the scenario was worse than you said — its control section had the same fault in reverse. Fixed in The scenario now splits the two clauses
The control section — "an ordinary build still polls" — had your fault inverted: on macOS an ordinary build refuses too, so it would fail there for a reason that is not a regression. It is now inside Part A and inherits the non-macOS precondition, which is what makes it evidence that the refusal came from the build rather than the host. Part A, run on this head, Linux
Connect frame: {"phase":"idle","latestVersion":null,"installable":false,
"releaseUrl":"https://github.com/decentespresso/decaid/releases","error":null}
{"error":"App update checks are not supported on this build",
"url":"https://github.com/decentespresso/decaid/releases"}One-shot assertion
Control, same host, ordinary build (no {"phase":"idle","currentVersion":"0.8.6-beta.1","error":null,"url":null}
{"phase":"checking","currentVersion":"0.8.6-beta.1","error":null,"url":null}
{"phase":"idle","currentVersion":"0.8.6-beta.1","error":null,"url":null}A state frame carrying Part B is not runIt needs a macOS host and I do not have one. The scenario says so explicitly rather than implying it was skipped for convenience. The clause it covers is the pre-existing One note on the toolingThe scenario is written with |
tadelv
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head adversarially. The externally-managed capability gate, API/spec/docs updates, unit/handler coverage, and the split E2E regression scenario are now coherent. Part A explicitly isolates externallyManaged on a non-macOS host and has been run on this head with the same-host ordinary-build control; the previous verification thread is therefore resolved. Current-head CI is green. main has advanced since that run, but the intervening commits do not touch this PR's update/API/scenario surface and the PR remains mergeable. I found no remaining merge blocker.
Summary
A failed update check was indistinguishable from "you are up to date". Three ways it could fail
silently, all fixed here.
android_updater.dartlogged a warning and returnednullwhen the release fetch failed. It nowthrows a typed
UpdateCheckExceptioncarrying the reason.update_check_service.dartcalledcheckForUpdate()from two places with no guard, so a failureescaped as an unhandled error. Both sites now catch and log; the error state was already emitted
by then.
UpdateHandleranswered nothing useful on a build that cannot check. It now replies with anexplicit "not supported on this build" plus the releases URL, gated on the new
UpdateCheckService.canCheck.canCheckis!isMacOS && !externallyManaged. The second clause was added in review: upstream'sexternally managed builds (App Store / TestFlight) no-op inside
checkForUpdate()and emit anidleframe, which reads on the wire as "checked, you are up to date" — the same defect as above,reached by a different route.
Base:
main. Independent.Linked Issue
N/A
Verification
flutter analyze— clean.dart format— clean on every changed.dartfile, run on Dart 3.13.3, the version CI uses.flutter test— the focused files pass:update_check_service,android_updater_download,update_handlerandapp_update_state.&& !externallyManagedreverted out ofcanCheck, exactly two tests failand nothing else: the service test, and the handler test with
Bad state: No element— no replyat all, the reviewed defect demonstrated at the handler tier. Restoring the one line returns
them to green.
End-to-end, run on this head.
.agents/skills/decent-app/scenarios/update-check-capability.mdsplits the two
canCheckclauses, because either alone is enough to refuse and one recipe cannottest both.
Part A — the ownership clause, Linux host,
--dart-define APP_STORE=true:{"error":"App update checks are not supported on this build", "url":"https://github.com/decentespresso/decaid/releases"}GET /api/v1/updateunchanged:{"phase":"idle","latestVersion":null,"installable":false}.Control, same host, ordinary build, same command:
{"phase":"idle","currentVersion":"0.8.6-beta.1"} {"phase":"checking","currentVersion":"0.8.6-beta.1"} {"phase":"idle","currentVersion":"0.8.6-beta.1"}A state frame, not a refusal — same box, same command, opposite outcome, so the refusal is the
ownership clause and not the host.
Part B — the platform clause requires a macOS host and was not run. It covers pre-existing
!isMacOSbehaviour that this PR does not change, and it is pinned at the unit tier.machine — they reached that fork on 30 Aug 2026 and the tablet has run them since v0.8.31
(1 Sep). The
externallyManagedclause added in review is not on that build; its evidenceis the scenario run and the tests above.
Impact
available", which is the same thing a user sees when genuinely up to date.
UpdateCheckExceptionis new and thrown wherenullwas returned. A caller thatignored the null result now sees an exception — the two call sites in this repo are both updated.
canCheckis additive. A build that cannot check now receives an explicitrefusal where it previously received an
idlestate frame or, on macOS, nothing at all.checkcommand's AsyncAPI description anddoc/Api.mdcarry theownership case, and a new end-to-end scenario covers the refusals.
Contributor Responsibility
AI-assisted development is allowed. The submitter remains responsible for the submitted work.