Skip to content

fix(update): a failed update check must say so, not look like "up to date" - #792

Merged
tadelv merged 5 commits into
decentespresso:mainfrom
ChampionDesigns:ben/update-check-honesty
Sep 10, 2026
Merged

tadelv merged 5 commits into
decentespresso:mainfrom
ChampionDesigns:ben/update-check-honesty

Conversation

@ChampionDesigns

@ChampionDesigns ChampionDesigns commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

A failed update check was indistinguishable from "you are up to date". Three ways it could fail
silently, all fixed here.

  • android_updater.dart logged a warning and returned null when the release fetch failed. 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 build that cannot check. It now replies with an
    explicit "not supported on this build" plus the releases URL, gated on the new
    UpdateCheckService.canCheck.

canCheck is !isMacOS && !externallyManaged. The second clause was added in review: upstream's
externally managed builds (App Store / TestFlight) no-op inside checkForUpdate() and emit an
idle frame, 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 .dart file, run on Dart 3.13.3, the version CI uses.
  • flutter test — the focused files pass: update_check_service, android_updater_download,
    update_handler and app_update_state.
  • Red proof. With && !externallyManaged reverted out of canCheck, exactly two tests fail
    and nothing else: the service test, and the handler test with Bad state: No element — no reply
    at 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.md
splits the two canCheck clauses, because either alone is enough to refuse and one recipe cannot
test 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/update unchanged: {"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
!isMacOS behaviour that this PR does not change, and it is pinned at the unit tier.

  • On hardware, scoped. The first three fixes above ship in the Decaid-Canary build on Ben's
    machine — they reached that fork on 30 Aug 2026 and the tablet has run them since v0.8.31
    (1 Sep). The externallyManaged clause added in review is not on that build; its evidence
    is the scenario run and the tests above.

Impact

  • User-visible: a failed check now says it failed. Previously it looked like "no update
    available", which is the same thing a user sees when genuinely up to date.
  • API: UpdateCheckException is new and thrown where null was returned. A caller that
    ignored the null result now sees an exception — the two call sites in this repo are both updated.
  • Compatibility: canCheck is additive. A build that cannot check now receives an explicit
    refusal where it previously received an idle state frame or, on macOS, nothing at all.
  • Documentation: the check command's AsyncAPI description and doc/Api.md carry the
    ownership case, and a new end-to-end scenario covers the refusals.
  • Security: none.

Contributor Responsibility

AI-assisted development is allowed. The submitter remains responsible for the submitted work.

  • I have reviewed and understand all changes in this PR and take responsibility for their correctness, security, behavior, licensing, and provenance, including any AI-assisted or AI-generated work.

…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 tadelv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread lib/src/util/github_archive.dart Outdated
Comment thread lib/src/services/webserver/update_handler.dart
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 tadelv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@ChampionDesigns

Copy link
Copy Markdown
Collaborator Author

The sb-dev smoke of /ws/v1/update is done. No code change; this is the
verification evidence you asked for.

Run on Linux from an isolated worktree checked out at this branch's head
b67fcbcd, so the app under test was this branch's tree rather than a merge —
currentVersion reports 0.8.5, this branch's pubspec version, which is how I
know it. websocat is not installed on that box, so the client was Python
websockets sending the same frames.

scripts/sb-dev.sh start --platform linux --connect-machine MockBengle
scripts/sb-dev.sh status     # Running, HTTP reachable at :8080, MockBengle connected

GET /api/v1/update:

{"phase":"idle","currentVersion":"0.8.5","latestVersion":null,"releaseNotes":null,
 "releaseUrl":"https://github.com/decentespresso/decaid/releases","installable":false,
 "progress":null,"error":null}

ws://localhost:8080/ws/v1/update. Each connection first receives the current
snapshot, which is the documented connect behaviour; the replies below follow it.

sent received
{"command":"check"} phase:"checking" then phase:"idle", error:null
{"command":"install"} {"error":"In-app install is not supported on this platform","url":"https://github.com/decentespresso/decaid/releases"}
{"command":"bogus"} {"error":"Unknown command: bogus"}
{"nocommand":1} {"error":"Missing \"command\" field"}
{not json {"error":"Invalid JSON: FormatException: Unexpected character (at character 2)..."}

The install reply is the transient {error, url} shape. It is the same refusal
code and the same shape that check returns on macOS, which
assets/api/websocket_v1.yml now documents.

The failure path, which is what this PR is about. A successful check that
finds nothing returns to idle with error:null, which proves nothing on its
own. So I pointed the updater at a nonexistent repository, hot-restarted, and
sent check again:

recv {"phase":"idle",...}
recv {"phase":"checking",...}
recv {"phase":"error", ..., "error":"Update check failed: Failed to fetch releases: HTTP 404"}

That is the behaviour this PR exists for, on the live endpoint. I then restored
the file, hot-restarted, and confirmed check returns to idle with
error:null as above. scripts/sb-dev.sh logs --filter error showed only the two
deliberate 404 lines from that step and nothing else.

What the smoke could not reach, stated plainly. The macOS check refusal.
canCheck is !Platform.isMacOS, so on Linux that branch is unreachable. Its
shape was exercised through install, which uses the identical refusal code, and
the macOS branch itself is covered by unit tests that inject
platformIsMacOS: true.

Both API specs parse. flutter analyze clean, flutter test green.

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.
@ChampionDesigns

Copy link
Copy Markdown
Collaborator Author

Brought current with main in 5046d51f, since this branch had gone
conflicting.

main is now 9cbc20f5, and cf8348c2 — "respect externally managed app
builds" — rewrote lib/src/services/update_check_service.dart, which this branch
also changes. Merged rather than rebased, so the three-dot diff is still only
this branch's own work: 7 files, no upstream file among them.

The one conflict resolved as a union. Your externallyManaged field, the
availableUpdate getter and all three early returns are kept, and so are this
branch'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 you intended.

One interaction worth your call, which I did not decide in a merge:
canCheck is !_isMacOS and does not consider externallyManaged. So on an
externally managed Linux or Android build, {"command":"check"} over
/ws/v1/update is accepted, requestCheck() runs, and checkForUpdate returns
at your early return with phase: "idle". That matches your intent — an
externally managed build has no update of its own — but it is the one remaining
place where a check does nothing and reports idle, which is the shape this PR
otherwise removes. If you would rather it answer the transient {error, url}
refusal instead, that is a one-line change to canCheck. Say which you prefer.

Verification: flutter analyze clean. update_check_service_test,
update_handler_test and settings_view_updates_test all pass.
dart format --set-exit-if-changed clean under Dart 3.13.3.

On the local full suite I have to be exact rather than claim green: 3989 passed,
1 skipped, 8 failed, and all 8 are in
test/webui_support/webui_token_injection_test.dart with
SocketException: Address already in use, port = 3000. Another process on my
build box held port 3000 for the whole run. This branch touches no server
binding, so there is no path from it to that file. CI runs on a clean runner
with no such conflict, so treat the checks here as the authority for that file.

@tadelv tadelv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread lib/src/services/update_check_service.dart Outdated
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>
@ChampionDesigns

Copy link
Copy Markdown
Collaborator Author

Fixed in 30a6881b. All three parts are in: the gate, the AsyncAPI wording, and tests at both tiers.

The gate. canCheck is now !_isMacOS && !externallyManaged, reading the same plain field checkForUpdate() reads, so the gate and the no-op cannot disagree. No --dart-define seam was needed — externallyManaged is already an instance field on the service whose constructor default is BuildInfo.appStore.

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 APP_STORE=true build, checkForUpdate() reaches _availableUpdate = null; _emit(AppUpdatePhase.idle); return null, and _emit calls _state.add, which the socket subscription forwards. I probed it — the stream emitted exactly one frame, phase idle. So the client received a frame byte-indistinguishable from a genuine "checked, you are up to date", which is precisely the failure this PR is named for. macOS is the genuinely silent case. After the fix the guard fires, requestCheck() is never called, and no idle frame is produced.

The refusal now names the build, not the platformApp update checks are not supported on this build. On an App Store or TestFlight build the platform is not the reason, and the old wording would have misattributed it.

Nothing else changes. canCheck has exactly one production caller. The Flutter UI reads externallyManaged directly and already hides the update controls on managed builds. GET /api/v1/update is byte-identical — toJson() carries no capability key — so rest_v1.yml needed nothing. Worth noting a Mac App Store build was already covered by !_isMacOS; the new clause adds iOS/TestFlight and any future non-macOS APP_STORE=true build.

Spec and docs. assets/api/websocket_v1.yml covers the ownership case, and so does doc/Api.md — which this lane had never touched, a standing CONTRIBUTING.md requirement for a WebSocket change that is now closed. I also added the end-to-end scenario scenarios/update-check-capability.md and indexed it, since this changes the observable behaviour of an existing command.

Tests, both tiers you named. The service test pins canCheck false for an externally managed build and that the two clauses compose. The handler test pins the reply and asserts requestChecks == 0, which is what proves the no-op path is never entered. With the one line reverted to !_isMacOS, exactly those two fail and nothing else — the handler one with Bad state: No element, i.e. no reply at all, your blocker demonstrated at the handler tier.

flutter analyze clean, dart format clean on Dart 3.13.3, the two test files 39 passing.

@tadelv tadelv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment thread .agents/skills/decent-app/scenarios/update-check-capability.md
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>
@ChampionDesigns

Copy link
Copy Markdown
Collaborator Author

You were right, and the scenario was worse than you said — its control section had the same fault in reverse. Fixed in 43afc681, and run.

The scenario now splits the two clauses

canCheck is !isMacOS && !externallyManaged and either clause alone refuses, so one recipe cannot test both. It is now Part A — the ownership clause, which asserts a non-macOS host, and Part B — the platform clause, which asserts macOS. Each states what deleting its own clause does, and I checked both against the source rather than assuming:

  • delete !externallyManagedcheckForUpdate() reaches _emit(AppUpdatePhase.idle), so the client gets a state frame that reads as "checked, up to date"
  • delete !_isMacOS → that branch returns with no emit at all, so the client waits for a reply that never arrives

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

scripts/sb-dev.sh start --platform linux --connect-machine MockDe1 --connect-scale MockScale --dart-define APP_STORE=true

Connect frame:

{"phase":"idle","latestVersion":null,"installable":false,
 "releaseUrl":"https://github.com/decentespresso/decaid/releases","error":null}

{"command":"check"} — last frame:

{"error":"App update checks are not supported on this build",
 "url":"https://github.com/decentespresso/decaid/releases"}

One-shot assertion (.error|contains("not supported")) and (.url|contains("releases"))true, exit 0.

GET /api/v1/update{"phase":"idle","latestVersion":null,"installable":false}, unchanged.

Control, same host, ordinary build (no APP_STORE define), {"command":"check"}:

{"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 phase, not an {error, url} refusal. Same host, same command, opposite outcome — so the refusal above is the ownership clause and nothing else.

Part B is not run

It 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 !_isMacOS behaviour, unchanged by this PR, and it is pinned at the unit tier in update_check_service_test.dart.

One note on the tooling

The scenario is written with websocat to match the other scenarios and verification.md. websocat was not on my machine, so the runs above used a small python websockets client that speaks the same frames. The frame content is what it is; only the client differs.

@tadelv tadelv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@tadelv
tadelv merged commit b9f4621 into decentespresso:main Sep 10, 2026
5 checks passed
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.

2 participants