fix(version): a frozen release must not ask git whether it is dirty

keel/cli.py

@@ -202,6 +202,7 @@
 from keel.data import history as history_mod  # noqa: F401 -- fetch/simulate tests patch this alias
 from keel.data import repair as repair_mod  # noqa: F401 -- fetch tests patch cli_module.repair_mod
 from keel.data.db import connect, migrate
+from keel.install import install_plan_cmd
 from keel.research import ledger as trials_ledger
 from keel.version import build_info, check_install
 
@@ -272,9 +273,7 @@ def _print_version(ctx: click.Context, param: object, value: bool) -> None:
     "logging.verbose. Errors/exceptions are always logged regardless of this flag.",
 )
 @click.pass_context
-def cli(
-    ctx: click.Context, db_path: str, config_path: str, verbose: bool
-) -> None:
+def cli(ctx: click.Context, db_path: str, config_path: str, verbose: bool) -> None:
     """keel: an offline-first, halal, guard-railed Coinbase auto-trading agent."""
     ctx.ensure_object(dict)
     ctx.obj["db_path"] = db_path
@@ -299,7 +298,10 @@ def cli(
     # `keel_core.paths` would make `mkdir x && cd x && keel init` write somewhere else entirely,
     # because an empty folder is not yet a deployment root. Once written, the folder IS one and
     # every other command resolves against it (#434).
-    "--config", "config_path", default=DEFAULT_CONFIG_PATH, show_default=True,
+    "--config",
+    "config_path",
+    default=DEFAULT_CONFIG_PATH,
+    show_default=True,
     help="Where to write the config file.",
 )
 @click.option("--force", is_flag=True, default=False, help="Overwrite an existing config.")
@@ -329,7 +331,10 @@ def init_config(config_path: str, force: bool, live: bool) -> None:
 
 @cli.command("init")
 @click.option(
-    "--config", "config_path", default=DEFAULT_CONFIG_PATH, show_default=True,
+    "--config",
+    "config_path",
+    default=DEFAULT_CONFIG_PATH,
+    show_default=True,
     help="Config file to write.",
 )
 @click.option("--force", is_flag=True, default=False, help="Overwrite an existing config.")
@@ -505,11 +510,16 @@ def assets_group() -> None:
 
 @assets_group.command("holdings")
 @click.option(
-    "--min-balance", default="0", show_default=True,
+    "--min-balance",
+    default="0",
+    show_default=True,
     help="Ignore balances at or below this (dust from airdrops, forks and rounding).",
 )
 @click.option(
-    "--screen", "run_screen", is_flag=True, default=False,
+    "--screen",
+    "run_screen",
+    is_flag=True,
+    default=False,
     help="Also run each holding through the admission screen.",
 )
 @click.pass_context
@@ -558,7 +568,9 @@ def assets_holdings(ctx: click.Context, min_balance: str, run_screen: bool) -> N
 @assets_group.command("discover")
 @click.option("--quote", default=None, help="Settlement currency (default: config.quote_currency).")
 @click.option(
-    "--min-volume-24h", default="100000", show_default=True,
+    "--min-volume-24h",
+    default="100000",
+    show_default=True,
     help="Cheap pre-filter on the venue's reported 24h quote volume. Bounds the request count; it "
     "is NOT a liquidity criterion -- a 24h snapshot is a different statistic from the median the "
     "gate applies, so use --probe-liquidity for that. Deliberately set well BELOW the admission "
@@ -575,7 +587,9 @@ def assets_holdings(ctx: click.Context, min_balance: str, run_screen: bool) -> N
     # typical sweep at no extra cost: with NEITHER probe flag, `discover` makes exactly ONE venue
     # request regardless of --limit -- filtering and sorting are local. --probe-history and
     # --probe-liquidity are the ones with a per-row cost, called out below.
-    "--limit", default=100, show_default=True,
+    "--limit",
+    default=100,
+    show_default=True,
     help="Show at most this many candidates. With neither --probe-history nor --probe-liquidity, "
     "raising this costs nothing extra -- discovery still makes exactly one venue request. Each "
     "probe flag adds one venue request PER CANDIDATE SHOWN (two requests per row if both are "
@@ -667,7 +681,9 @@ def assets_screen(ctx: click.Context, products: str | None) -> None:
 
 @assets_group.command("propose")
 @click.option(
-    "--from", "from_file", required=True,
+    "--from",
+    "from_file",
+    required=True,
     type=click.Path(exists=True, dir_okay=False),
     help="JSON shortlist file produced OUTSIDE keel (an LLM + web-search scout).",
 )
@@ -1021,9 +1037,7 @@ def _print_loop_result(result: agent.LoopResult) -> None:
 
 
 @cli.command()
-@click.option(
-    "--loop", is_flag=True, default=False, help="Run the scheduled loop, not one cycle."
-)
+@click.option("--loop", is_flag=True, default=False, help="Run the scheduled loop, not one cycle.")
 @click.option(
     "--interval",
     "interval_sec",
@@ -1159,9 +1173,7 @@ def _parse_products_option(products: str | None, config: Config) -> list[str]:
     `--products` option and polls `_default_sim_products` directly.
     """
     try:
-        product_list, warnings = parse_products_option(
-            products, config, settlement_is_fatal=False
-        )
+        product_list, warnings = parse_products_option(products, config, settlement_is_fatal=False)
     except ValueError as exc:
         raise click.BadParameter(str(exc), param_hint="--products") from exc
     for warning in warnings:
@@ -1338,6 +1350,15 @@ def simulate(
 cli.add_command(setup_cmd)
 
 
+# -- install-plan (the machine interface an installer script calls) ------------------------------
+
+# The desktop product has no self-update (#439), so the INSTALLER is the update path -- and "what
+# should happen when this build meets the one already on disk" has to be right every time. An
+# Inno Setup script and a `.pkg` postinstall are both places where that answer cannot be tested,
+# so it lives in `keel/install.py` and they shell out to this.
+cli.add_command(install_plan_cmd)
+
+
 # -- insights (read-only promotion-gate + journal reporting, no broker call) --------------------
 
 # A pure VIEW over `gather_status`/`StatusReport`, the repository read methods, and the

keel/install.py

@@ -25,26 +25,27 @@
 
 from __future__ import annotations
 
+import json
 import os
 import sys
+from configparser import ConfigParser
 from dataclasses import dataclass
 from enum import Enum
 from pathlib import Path
 
+import click
+
+from keel.version import is_packaged as _is_packaged
+
 #: Where a release is downloaded from, named in every refusal that tells a desktop user to update
 #: by downloading rather than by running a command.
 RELEASES_URL = "https://github.com/CodeGateSoftware/keel/releases/latest"
 
 
-def is_packaged() -> bool:
-    """True when running inside a frozen bundle rather than from a venv.
-
-    Both markers are checked because PyInstaller sets `sys.frozen` for every build mode but only
-    sets `sys._MEIPASS` for `--onefile`, and other freezers set one or the other. A false positive
-    here costs a refusal an operator can work around; a false negative sends a desktop user to
-    install `uv`, which is the outcome #439 exists to stop.
-    """
-    return bool(getattr(sys, "frozen", False)) or hasattr(sys, "_MEIPASS")
+#: Re-exported from `keel.version`, its home: that module is a leaf, and "how is this running"
+#: is its subject. Re-exported rather than re-implemented so the two can never disagree about the
+#: same process -- `keel.version.build_info` reads it to decide whether to consult git at all.
+is_packaged = _is_packaged
 
 
 # -- where things go ---------------------------------------------------------------------------
@@ -245,3 +246,115 @@ def packaged_update_refusal() -> str:
         f"from the command line. Get it from {RELEASES_URL} and run it -- it will keep your "
         "config, database and credentials exactly as they are."
     )
+
+
+# -- the marker an installer reads -------------------------------------------------------------
+#
+# `plan_install` needs the installed version, and #438 requires it be read from the artifact's
+# METADATA rather than by executing the installed binary. A frozen bundle has no `.dist-info` an
+# installer script can parse, so the installer writes this instead: one small file, in the program
+# directory, in a format a `.iss` script or a shell one-liner can read without a Python
+# interpreter it does not yet have.
+
+#: Written into the PROGRAM directory (never the deployment). INI rather than JSON because Inno
+#: Setup reads INI natively (`GetIniString`) and would otherwise need a JSON parser in Pascal --
+#: and a hand-rolled parser deciding whether to overwrite someone's install is not a trade worth
+#: making.
+INSTALL_MARKER = "keel-install.ini"
+
+_MARKER_SECTION = "keel"
+
+
+def marker_path(program_dir: Path | str) -> Path:
+    return Path(program_dir) / INSTALL_MARKER
+
+
+def write_install_marker(program_dir: Path | str, version: str, *, commit: str = "") -> Path:
+    """Record what was installed, for the NEXT installer to read.
+
+    Deliberately tiny and deliberately not authoritative about anything else: it answers one
+    question, and a file that answered several would grow reasons to be trusted for things it
+    cannot know."""
+    path = marker_path(program_dir)
+    path.parent.mkdir(parents=True, exist_ok=True)
+    body = f"[{_MARKER_SECTION}]\nversion={version}\n"
+    if commit:
+        body += f"commit={commit}\n"
+    path.write_text(body, encoding="utf-8")
+    return path
+
+
+def read_installed_version(program_dir: Path | str) -> str | None:
+    """The version recorded in `program_dir`, or `None` if there is no keel there.
+
+    `None` for a missing file, an unreadable one, a malformed one and an empty version alike: all
+    four mean "cannot establish what is installed", and `plan_install` turns that into a
+    confirmation rather than a silent overwrite. Never raises -- an installer that crashes while
+    deciding whether to overwrite is worse than one that asks."""
+    path = marker_path(program_dir)
+    try:
+        text = path.read_text(encoding="utf-8")
+    except OSError:
+        return None
+    parser = ConfigParser()
+    try:
+        parser.read_string(text)
+    except Exception:
+        return None
+    version = parser.get(_MARKER_SECTION, "version", fallback="").strip()
+    return version or None
+
+
+def plan_install_into(program_dir: Path | str, incoming_version: str) -> InstallPlan:
+    """`plan_install`, reading the installed version from the target directory itself."""
+    return plan_install(incoming_version, read_installed_version(program_dir))
+
+
+# -- the command an installer script calls -----------------------------------------------------
+
+
+@click.command("install-plan")
+@click.option(
+    "--target",
+    required=True,
+    type=click.Path(file_okay=False, path_type=Path),
+    help="The program directory the installer is about to write into.",
+)
+@click.option("--incoming", default=None, help="Version being installed (default: this build's).")
+@click.option("--json", "as_json", is_flag=True, default=False, help="Machine-readable output.")
+@click.pass_context
+def install_plan_cmd(ctx: click.Context, target: Path, incoming: str | None, as_json: bool) -> None:
+    """What installing into `--target` would do, and whether it needs the user to agree.
+
+    A machine interface, like `keel versions`: an installer script shells out to it and acts on
+    the answer, so the rule that a downgrade must warn lives in tested Python rather than in a
+    Pascal script or a shell fragment where it cannot be tested at all.
+
+    Exit code is 0 when the install may proceed without asking, and 2 when it must stop and
+    confirm -- so a script that reads nothing but the status still fails safe.
+    """
+    from keel.version import build_info
+
+    version = incoming or build_info().version
+    plan = plan_install_into(target, version)
+    if as_json:
+        click.echo(
+            json.dumps(
+                {
+                    "decision": plan.decision.value,
+                    "installed_version": plan.installed_version,
+                    "incoming_version": plan.incoming_version,
+                    "needs_confirmation": plan.needs_confirmation,
+                    "summary": plan.summary,
+                    "warning": plan.warning,
+                },
+                indent=2,
+            )
+        )
+    else:
+        click.echo(plan.summary)
+        if plan.warning:
+            click.echo(plan.warning)
+    # `ctx.exit`, not `raise SystemExit`: click owns the exit path, and a bare SystemExit
+    # is swallowed into a generic failure that loses both the code and the output above it.
+    ctx.exit(2 if plan.needs_confirmation else 0)

keel/version.py

@@ -29,6 +29,7 @@
 
 import re
 import subprocess
+import sys
 from dataclasses import dataclass
 from importlib import metadata
 
@@ -88,6 +89,20 @@ def _package_version() -> str:
     return "unknown"
 
 
+def is_packaged() -> bool:
+    """True when running inside a frozen bundle rather than from a venv or a checkout.
+
+    Lives here rather than in `keel/install.py` (its first home) because `keel.version` is a leaf
+    -- it imports nothing from keel -- and "how was this built, and how is it running" is exactly
+    this module's subject. `keel.install` re-exports it so there is one detector, not two that
+    can disagree about the same process.
+
+    Both markers are checked: PyInstaller sets `sys.frozen` for every build mode but `sys._MEIPASS`
+    only for `--onefile`, and other freezers set one or the other.
+    """
+    return bool(getattr(sys, "frozen", False)) or hasattr(sys, "_MEIPASS")
+
+
 def _git(*args: str) -> str | None:
     try:
         result = subprocess.run(
@@ -118,6 +133,29 @@ def build_info() -> BuildInfo:
     """Resolve the running build. Never raises."""
     embedded = _embedded()
 
+    if is_packaged():
+        # A frozen bundle has no checkout, so there is nothing for git to tell us about it -- and
+        # asking is actively harmful. `_git` inherits the process CWD, so a packaged app launched
+        # from inside ANY git repository reads that repository's HEAD, finds it disagrees with the
+        # stamp, and marks a legitimate signed release DIRTY. The user then reads "this build is
+        # NOT reproducible -- do not run it against live funds" about a build that is both. A
+        # warning that fires on correct builds is a warning people learn to ignore, and this is
+        # the one that must never be ignored.
+        #
+        # The stale-stamp hazard the git cross-check below exists for cannot arise here: there is
+        # no working tree to have edited. An UNSTAMPED bundle is `unknown`, never `checkout` --
+        # it is not one -- which also keeps `plan_update`'s `source != "release"` refusal correct.
+        if embedded is None:
+            return BuildInfo(
+                version=_package_version(), commit="unknown", dirty=False, source="unknown"
+            )
+        return BuildInfo(
+            version=getattr(embedded, "VERSION", _package_version()),
+            commit=getattr(embedded, "COMMIT", "unknown"),
+            dirty=bool(getattr(embedded, "DIRTY", False)),
+            source="release",
+        )
+
     if embedded is not None:
         stamped_commit = getattr(embedded, "COMMIT", "unknown")
         dirty = bool(getattr(embedded, "DIRTY", False))