diff --git a/keel/cli.py b/keel/cli.py index 5967224..fc63ef9 100644 --- a/keel/cli.py +++ b/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 diff --git a/keel/install.py b/keel/install.py index 9fd8071..361d2b2 100644 --- a/keel/install.py +++ b/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) diff --git a/keel/version.py b/keel/version.py index 56cc608..7625659 100644 --- a/keel/version.py +++ b/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)) diff --git a/tests/test_install.py b/tests/test_install.py index fead45d..4740982 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -226,3 +226,187 @@ def test_keel_update_refuses_a_packaged_install_and_says_why( # And it is the FIRST thing said: every other refusal talks about venv layouts and `uv`, # which is exactly the advice a desktop user cannot act on. assert RELEASES_URL in packaged.refusal_reasons[0] + + +# -- the marker, and the command an installer script calls ------------------------------------ + + +def test_a_marker_round_trips(tmp_path: Path) -> None: + from keel.install import read_installed_version, write_install_marker + + write_install_marker(tmp_path, "0.10.0", commit="abc123") + assert read_installed_version(tmp_path) == "0.10.0" + + +def test_the_marker_goes_in_the_program_directory_and_is_ini(tmp_path: Path) -> None: + """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.""" + from keel.install import INSTALL_MARKER, write_install_marker + + path = write_install_marker(tmp_path, "0.10.0") + assert path.name == INSTALL_MARKER + assert path.suffix == ".ini" + assert "[keel]" in path.read_text() + + +@pytest.mark.parametrize( + "content", ["", "not ini at all", "[keel]\n", "[keel]\nversion=\n", "[other]\nversion=1\n"] +) +def test_an_unusable_marker_reads_as_unknown_and_never_raises(tmp_path: Path, content: str) -> None: + """Missing, unreadable, malformed and empty all mean "cannot establish what is installed", + which `plan_install` turns into a confirmation rather than a silent overwrite. An installer + that crashed while deciding whether to overwrite would be worse than one that asks.""" + from keel.install import INSTALL_MARKER, plan_install_into, read_installed_version + + (tmp_path / INSTALL_MARKER).write_text(content) + assert read_installed_version(tmp_path) is None + assert plan_install_into(tmp_path, "0.11.0").decision is InstallDecision.FRESH + + +def test_a_directory_with_no_marker_reads_as_nothing_installed(tmp_path: Path) -> None: + from keel.install import read_installed_version + + assert read_installed_version(tmp_path) is None + assert read_installed_version(tmp_path / "does-not-exist") is None + + +@pytest.mark.parametrize( + ("installed", "incoming", "code"), + [ + (None, "0.11.0", 0), + ("0.10.0", "0.11.0", 0), + ("0.10.0", "0.10.0", 2), + ("0.11.0", "0.10.0", 2), + ], +) +def test_install_plan_exits_zero_to_proceed_and_two_to_confirm( + tmp_path: Path, installed: str | None, incoming: str, code: int +) -> None: + """The exit code carries the decision, so a script that reads nothing but the status still + fails safe: anything non-zero means stop and ask.""" + from click.testing import CliRunner + + from keel.cli import cli + from keel.install import write_install_marker + + if installed is not None: + write_install_marker(tmp_path, installed) + result = CliRunner().invoke( + cli, ["install-plan", "--target", str(tmp_path), "--incoming", incoming] + ) + assert result.exit_code == code, result.output + + +def test_install_plan_json_carries_the_warning(tmp_path: Path) -> None: + import json as json_mod + + from click.testing import CliRunner + + from keel.cli import cli + from keel.install import write_install_marker + + write_install_marker(tmp_path, "0.11.0") + result = CliRunner().invoke( + cli, ["install-plan", "--target", str(tmp_path), "--incoming", "0.10.0", "--json"] + ) + payload = json_mod.loads(result.output) + assert payload["decision"] == "downgrade" + assert payload["needs_confirmation"] is True + assert "forward-only" in payload["warning"] + + +# -- the stamp, and why a frozen build must not ask git ---------------------------------------- + + +def _stamped(monkeypatch: pytest.MonkeyPatch, commit: str) -> None: + import types + + import keel.version as version_mod + + stamp = types.SimpleNamespace(VERSION="0.10.0", COMMIT=commit, DIRTY=False) + monkeypatch.setattr(version_mod, "_embedded", lambda: stamp) + + +def test_a_frozen_release_is_not_marked_dirty_by_an_unrelated_git_repo( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`_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 one people learn to ignore, and this is the warning + that must never be ignored.""" + import keel.version as version_mod + + _stamped(monkeypatch, "aaaaaaaaaaaa") + monkeypatch.setattr(version_mod, "_git", lambda *args: "bbbbbbbbbbbb") + monkeypatch.setattr(version_mod, "is_packaged", lambda: True) + + info = version_mod.build_info() + assert info.source == "release" + assert info.dirty is False + assert info.is_reproducible + + +def test_a_VENV_release_is_still_marked_dirty_when_git_disagrees( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The fix must not weaken the case it was written for. A stale stamp in a working checkout + would otherwise claim `[release]` and hide a dirty tree -- the exact misreport + `keel/version.py` exists to prevent -- and that hazard is real for a venv install and + impossible for a bundle, which has no working tree to have edited.""" + import keel.version as version_mod + + _stamped(monkeypatch, "aaaaaaaaaaaa") + monkeypatch.setattr(version_mod, "_git", lambda *args: "bbbbbbbbbbbb") + monkeypatch.setattr(version_mod, "is_packaged", lambda: False) + + info = version_mod.build_info() + assert info.source == "release" + assert info.dirty is True + assert not info.is_reproducible + + +def test_an_unstamped_frozen_bundle_is_unknown_and_never_checkout( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """It is not a checkout -- there is no working tree. `unknown` also keeps `plan_update`'s + `source != "release"` refusal correct for a bundle that was built without a stamp.""" + import keel.version as version_mod + + monkeypatch.setattr(version_mod, "_embedded", lambda: None) + monkeypatch.setattr(version_mod, "is_packaged", lambda: True) + + info = version_mod.build_info() + assert info.source == "unknown" + assert not info.is_reproducible + + +def test_a_frozen_build_never_shells_out_to_git(monkeypatch: pytest.MonkeyPatch) -> None: + """Not merely that the answer is right, but that git is not consulted at all: a subprocess + per `--version` on a desktop app is also a visible pause, and on a machine with no git it is + an exception handler doing nothing useful.""" + import keel.version as version_mod + + _stamped(monkeypatch, "aaaaaaaaaaaa") + monkeypatch.setattr(version_mod, "is_packaged", lambda: True) + + calls: list[tuple[str, ...]] = [] + + def _record(*args: str) -> str | None: + calls.append(args) + return None + + monkeypatch.setattr(version_mod, "_git", _record) + version_mod.build_info() + assert calls == [] + + +def test_there_is_one_packaged_detector_not_two() -> None: + """`keel.install.is_packaged` is `keel.version`'s, re-exported. Two detectors would + eventually disagree about the same process.""" + from keel import install, version + + assert install.is_packaged is version.is_packaged