feat(install): what an installer must decide, in one testable place - #457
Merged
Conversation
#439's decision is option A: the desktop product has no self-update. Bundle-aware self-update buys convenience and costs an update channel that must itself be secured, and for a tool that moves real money a user deliberately downloading a signed installer is the better trust posture. That decision has a consequence worth taking seriously: **the installer is the update path**, so "what should happen when this build meets the one already on disk" is a question something has to answer correctly every single time. An Inno Setup script and a `.pkg` postinstall are both places where that answer cannot be tested, so it does not live in either. It lives in `keel/install.py`, which the installer calls and `keel update` reads to explain itself. THE RULE THAT IS NOT OBVIOUS. "Versions differ, so update" is right in one direction only. `keel/data/db.py` migrates with `if current < target` and ships no down-migrations. A database already at schema N, opened by a build that expects N-2, does NOT fail loudly: `migrate` finds nothing to apply and returns, and the old code then runs against tables and columns it was never written against. Silence is the entire hazard. So a downgrade is a confirmation carrying a specific warning -- including the recovery, which is a database backup taken BEFORE the upgrade, never running the older build anyway. An uncomparable pair carries the same warning, because an uncomparable pair might BE a downgrade. Only two outcomes proceed silently: a fresh install, and a genuine upgrade. Same-version and downgrade both stop and ask. That closed statement is itself a test. THE TRAP THIS AVOIDS. The program directory and the deployment directory are different places, and conflating them is how an upgrade destroys an operator's work: the program directory is replaced WHOLESALE, so a config that lived there would not survive one. `default_deployment_dir` delegates to `keel_core.paths.app_data_dir` (#434) rather than restating it -- an installer that proposed a folder the runtime does not discover would produce a deployment that appears EMPTY on first launch: config written, database written, and a dashboard reporting a healthy install with no history. `NEVER_TOUCHED` names every piece of operator state, and is tested against real filenames rather than eyeballed, so a typo in `keel*.db` cannot silently protect nothing. Defaults are per-user on both platforms, because an elevation prompt on a first run is precisely the friction this milestone exists to remove. macOS offers `~/Applications` when `/Applications` is not writable; Windows needs no fallback because its default is already per-user, and inventing one would be a second path for no reason. AND THE MESSAGE THAT REACHES A DESKTOP USER. Every refusal `keel update` produces today is correct and useless to one: they talk about `site-packages` layouts and tell the reader to put `uv` on PATH. A packaged user has no venv and no `uv` and never will. `is_packaged()` now refuses FIRST, naming the actual update path -- download the next signed release, and your config, database and credentials stay exactly as they are. Both freezer markers are checked, because PyInstaller sets `sys.frozen` for every build mode but `sys._MEIPASS` only for `--onefile`, and a false negative here is the exact outcome #439 exists to stop. Version comparison has one home: `plan_install` reads `keel.commands.update.version_key` rather than parsing semver again, pinned by test. A second reader would disagree with the first on exactly the strings nobody tested. 3988 passed, 3 skipped (22 new). ruff clean repo-wide; mypy clean over keel + packages. Refs #439, #438, #18. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eaitbrahim
added a commit
that referenced
this pull request
Aug 20, 2026
…459) A stamped release build reports `(DIRTY)` when run from inside ANY git repository. `_git` runs `subprocess.run(["git", ...])` with no `cwd`, so it inherits the process working directory, reads whatever HEAD is there, finds it disagrees with the stamp, and marks the build dirty. Demonstrated before fixing: the same stamped build reported clean in its own checkout and DIRTY from an unrelated repo two directories away. For a venv deployment that rarely bites -- `~/keel` is not a git repo. For a packaged desktop app it is the normal case, because a double-clicked binary runs wherever the user happens to be. And the consequence is not cosmetic: a legitimate signed release prints warning: this build is NOT reproducible -- it does not correspond to a commit. Do not run it against live funds. A warning that fires on correct builds is a warning people learn to ignore, and this is the one that must never be ignored. A frozen bundle now trusts its stamp and does not consult git at all -- not merely "reaches the right answer", but never spawns the subprocess, which on a desktop app is also a visible pause per `--version` and, on a machine with no git, an exception handler doing nothing useful. The stale-stamp hazard the cross-check exists for cannot arise in a bundle: there is no working tree to have edited. A test pins that a VENV release is still marked dirty when git disagrees, because the fix must not weaken the case the check was written for. An unstamped bundle is `unknown`, never `checkout` -- it is not one -- which also keeps `plan_update`'s `source != "release"` refusal correct. `is_packaged()` moves to `keel/version.py` and `keel/install.py` re-exports it. `keel.version` is a leaf and "how was this built, and how is it running" is its subject; two detectors would eventually disagree about the same process, which a test now forbids. ALSO: the marker and the command that make the installer implementable. #457 landed the decision logic and #438 requires the installed version be read from METADATA rather than by executing the installed binary. A frozen bundle has no `.dist-info` for an installer script to parse, so the installer writes `keel-install.ini` into the PROGRAM directory -- INI rather than JSON because Inno Setup reads INI natively and would otherwise need a JSON parser written in Pascal, and a hand-rolled parser deciding whether to overwrite someone's install is not a trade worth making. `keel install-plan --target DIR` is the machine interface over it, like `keel versions`: exit 0 to proceed, exit 2 to stop and confirm, so a script that reads nothing but the status still fails safe. Missing, unreadable, malformed and empty markers all read as "cannot establish what is installed", which becomes a confirmation rather than a silent overwrite -- an installer that crashed while deciding whether to overwrite would be worse than one that asks. Verified against a real stamped PyInstaller bundle: $ keel --version # from the keel repo AND from an unrelated one keel 0.10.0+52a035aa1111 [release] $ keel install-plan --target <empty> -> exit 0, "Install keel 0.10.0." $ keel install-plan --target <holding 0.11.0> -> exit 2, "Replace ... with the OLDER ...", plus the forward-only migrations warning 4018 passed, 3 skipped (18 new). ruff clean repo-wide; mypy clean over keel + packages. Refs #438, #439, #18. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #439 (D6), #438 (D5). Milestone: Desktop distribution (#18).
Implements the install-location and existing-install requirements added to #438, and the "either way" half of #439.
The decision, and its consequence
#439 recommends option A: the desktop product has no self-update. Bundle-aware self-update buys convenience and costs an update channel that must itself be secured — and for a tool that moves real money, a user deliberately downloading a signed installer is arguably the better trust posture.
That has a consequence worth taking seriously: the installer is the update path. So "what should happen when this build meets the one already on disk" is a question something has to answer correctly every single time. An Inno Setup script and a
.pkgpostinstall are both places where that answer cannot be tested, so it lives in neither.The rule that is not obvious
"Versions differ, so update" is right in one direction only.
keel/data/db.py:499migrates withif current < targetand ships no down-migrations. A database already at schema N, opened by a build that expects N−2, does not fail loudly —migratefinds nothing to apply and returns, and the old code then runs against tables and columns it was never written against.Silence is the entire hazard. So:
FRESHUPGRADEREINSTALLDOWNGRADEnightlyUNCOMPARABLEAn uncomparable pair carries the downgrade warning too, because an uncomparable pair might be a downgrade. The warning names the recovery and not just the risk: restore a database backup taken before the upgrade, rather than running the older build anyway.
"Only a fresh install or a genuine upgrade proceeds silently" is itself a test, so a fourth silent path cannot be added without deleting an assertion.
The trap this avoids
The program directory and the deployment directory are different places, and conflating them is how an upgrade destroys an operator's work — the program directory is replaced wholesale, so a config that lived there would not survive one.
default_deployment_dirdelegates tokeel_core.paths.app_data_dir(#434) rather than restating it. An installer that proposed a folder the runtime does not discover would produce a deployment that appears empty on first launch: config written, database written, and a dashboard reporting a healthy install with no history.NEVER_TOUCHEDnames every piece of operator state and is tested against real filenames, not eyeballed — so a typo inkeel*.dbcannot silently protect nothing./Applications/keel.app, falling back to~/Applications~/Library/Application Support/keel%LOCALAPPDATA%\Programs\keel%LOCALAPPDATA%\keelPer-user on both, because an elevation prompt on a first run is precisely the friction this milestone exists to remove. Windows needs no fallback — its default is already per-user, and inventing one would be a second path for no reason.
The message that reaches a desktop user
Every refusal
keel updateproduces today is correct and useless to one: they talk aboutsite-packageslayouts and tell the reader to putuvon PATH. A packaged user has no venv and nouv, and never will.is_packaged()now refuses first, naming the real update path — download the next signed release, and your config, database and credentials stay exactly as they are. Driven through the realplan_updateagainst an otherwise-valid deployment layout, so the packaged refusal is what's being observed rather than one the layout would have produced anyway.Both freezer markers are checked: PyInstaller sets
sys.frozenfor every build mode butsys._MEIPASSonly for--onefile. A false negative is the exact outcome #439 exists to stop.Verification
3988 passed, 3 skipped (22 new).
ruff checkclean repo-wide,mypyclean overkeel+packages.Version comparison has one home —
plan_installreadskeel.commands.update.version_keyrather than parsing semver again, pinned by test. A second reader would disagree with the first on exactly the strings nobody tested.What this does not do
Build anything. The PyInstaller spec, the Inno Setup script, the
.appbundle and the signed release job are D5 (#438), and D5 needs certificates I cannot obtain — Apple Developer ID at $99/yr, and a Windows OV cert or Azure Trusted Signing. This is the logic those scripts will call, landed first so it arrives tested rather than embedded in a build script where it never can be.🤖 Generated with Claude Code