diff --git a/docs/RELEASING.md b/docs/RELEASING.md index 45465542..52cfb3a5 100644 --- a/docs/RELEASING.md +++ b/docs/RELEASING.md @@ -72,7 +72,11 @@ A **fresh deployment** does not. `keel init` seeds every rule at `candidate` fro deliberately set to `budget_usd: 25` comes back as the built-in `50`, unpromoted, on a box that otherwise looks correctly provisioned. Nothing errors. (A worked example, not a description of today's deployment: the live DCA rule is now `50`, deliberately matching both the constructor -default and `config.dca.budget_usd`, which is the value the live executor actually spends.) +default and `config.dca.budget_usd`, which is the value the live executor actually spends. That +coincidence means the value alone can no longer prove the rule wasn't reseeded — `keel init`'s +default and the operator's intended value are now the same number. `test_committed_manifest_is_valid` +in `tests/test_rule_manifest.py` covers the gap by also asserting every committed rule's *status* +is `live`, since `keel init` always seeds at `candidate` regardless of what the params say.) `deploy/live-rules.json` is the committed record of the live deployment's rule set, so that state is a diff in a PR rather than a fact stored on one laptop: diff --git a/scripts/rule_manifest.py b/scripts/rule_manifest.py index d92e20f6..38bf5b4d 100644 --- a/scripts/rule_manifest.py +++ b/scripts/rule_manifest.py @@ -8,8 +8,11 @@ built-in `50`, at `candidate`, on a box that otherwise looks correctly provisioned. Nothing errors. (That is a worked example rather than a description of today's deployment -- the live DCA rule is now `50` on purpose, matching the constructor default and `config.dca.budget_usd`, which -is the value the live executor actually spends.) This module makes that state an artifact you can -diff in a PR instead of a fact that lives only on one laptop. +is the value the live executor actually spends. Because it now matches the default, the VALUE +alone can no longer prove the rule wasn't reseeded; `tests/test_rule_manifest.py`'s +`test_committed_manifest_is_valid` also asserts every committed rule's `status` is `live`, since a +`keel init` reseed always lands at `candidate` no matter what the params say.) This module makes +that state an artifact you can diff in a PR instead of a fact that lives only on one laptop. **`export`** writes the manifest. It is the source of truth's snapshot, not the source of truth: re-run it and commit the diff whenever the live rule set changes deliberately. diff --git a/tests/test_rule_manifest.py b/tests/test_rule_manifest.py index ae475928..0f9646d9 100644 --- a/tests/test_rule_manifest.py +++ b/tests/test_rule_manifest.py @@ -21,6 +21,7 @@ from keel.data.db import connect, migrate # noqa: E402 from keel.data.repository import Repository # noqa: E402 +from keel.strategy.rules.dca import Dca # noqa: E402 from scripts.rule_manifest import apply, export # noqa: E402 DCA = { @@ -152,9 +153,24 @@ def test_committed_manifest_is_valid(tmp_path: Path) -> None: # only by the account simulator, which means the divergence also made the sim model a # position size the live path would never take. # - # 50 is the intended budget, so all three now agree, and the assertion checks the AGREEMENT - # rather than a literal -- a hardcoded number here would just re-create the drift it is - # supposed to catch, one deploy later. + # 50 is the intended budget, so all three now agree, and assertion (1) below checks the + # AGREEMENT rather than a literal -- a hardcoded number here would just re-create the drift + # it is supposed to catch, one deploy later. + # + # But agreement alone is now a WEAKER guard than the "25" literal it replaced, and that + # weakness needs to be guarded explicitly rather than left as a silent regression: 50 is + # *also* `Dca.__init__`'s constructor default (see `keel/strategy/rules/dca.py`), and + # `deploy/live-rules.json`'s DCA params are, right now, EXACTLY those constructor defaults. + # That means "the manifest's budget equals the config's budget" is satisfied both by a + # correctly-provisioned box AND by a box where `keel init` silently reseeded the rule at + # `candidate` from constructor defaults -- the exact failure mode this test used to exist to + # catch. Values can no longer tell those two states apart. Two more assertions below make up + # for that: (2) status, which is the only thing that still can, and (3) a pinned check on + # the coincidence itself, so that if the operator ever moves the budget off the default, the + # value check regains its old power and (3) is what tells them so. + + # (1) AGREEMENT -- the manifest's budget must match config.dca.budget_usd, the value the live + # executor actually spends. live_config = REPO_ROOT / "config.live-sandbox.yaml" configured = re.search(r"^dca:\n(?:.*\n)*? budget_usd: (\S+)$", live_config.read_text(), re.M) assert configured is not None, "config.live-sandbox.yaml no longer declares dca.budget_usd" @@ -162,3 +178,41 @@ def test_committed_manifest_is_valid(tmp_path: Path) -> None: "the live DCA rule's budget_usd must match config.dca.budget_usd, which is the value the " "executor actually spends -- if they diverge, the simulator and the live path disagree" ) + + # (2) NOT SEED-SHAPED -- status is the only discriminator assertion (1) leaves standing + # between "the operator's 50" and "keel init's 50". `keel init` always seeds fresh rules at + # `candidate` (`docs/RELEASING.md`), and nothing in this test path promotes them, so a + # reseeded box's manifest would show `candidate` even though its budget_usd matches the + # config byte-for-byte. A correctly-provisioned deployment has every rule at `live`. + assert dca[0]["status"] == "live", ( + "the live DCA rule is not status=live -- if this is a candidate, keel init likely " + "reseeded it from Dca's constructor defaults rather than preserving an operator's tuned " + "value, and assertion (1) above cannot catch that on its own (see comment)" + ) + assert all(r["status"] != "candidate" for r in rebuilt), ( + "a rule in the committed live manifest is status=candidate -- that shape matches a fresh " + "`keel init` reseed from constructor defaults, not a deliberately-provisioned deployment" + ) + + # (3) THE COINCIDENCE IS PINNED, NOT ASSUMED -- assertion (2) only carries the weight it does + # because the operator's intended DCA params happen, today, to equal Dca's constructor + # defaults. Pin that equality explicitly instead of taking it on faith. If it ever stops + # being true -- e.g. the operator deliberately moves the budget off 50 -- THIS assertion is + # what fails, and failing here is good news dressed as a test failure: it means assertion (1) + # has regained the ability to catch a `keel init` revert on its own (a reseed would then + # produce a *different* number, not a coincidentally-matching one), and the status check in + # (2) is no longer the last line of defense. Whoever hits this failure should update this + # comment, not just delete the assertion. + manifest_params = dca[0]["params"] + default_params = Dca(product_id=manifest_params["product_id"]).describe()["params"] + for key, expected in default_params.items(): + if key == "product_id": + continue # trivially equal -- it's the constructor arg we just passed in + assert Decimal(str(manifest_params[key])) == Decimal(str(expected)), ( + f"deploy/live-rules.json's DCA {key} ({manifest_params[key]!r}) no longer matches " + f"Dca's constructor default ({expected!r}). Assertions (1)/(2) above still hold, but " + "the reasoning behind assertion (2) -- that status is the ONLY thing distinguishing " + "a deliberate value from a reseeded default -- no longer applies to this parameter: " + "a reseed would now produce a visibly different value, so assertion (1) alone would " + "catch it again." + )