From 8c16d79b7121056579faa8fbc37ce99d2a8c7b84 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 6 Sep 2026 20:00:35 -0400 Subject: [PATCH] test(scope,executor): two tests took their precondition from the machine (#742) Both passed in CI and failed for any contributor with a configured deployment -- the people most likely to run the suite locally. That is the expensive kind of failure: it teaches you the local suite is unreliable, against a CONTRIBUTING.md that asks for four green gates before review and says the suite is fast enough to run freely. It already cost one round-trip in this repo. **The mechanism is sharper than "the environment leaks in".** CI's empty credential store was silently SUPPLYING a precondition one of the tests needed. `current_credential_fingerprint` is imported independently at three sites -- `commands/scope.py`, `execution/guards.py` (rails 20 and 22), and `execution/executor.py`. `test_confirming_replaces_a_stale_fingerprint_rather_than_ carrying_it_forward` patches only the executor's. With a credential configured, guards' copy resolves a genuine fingerprint, finds it disagrees with the row's "stale000...", and RAIL 20 VETOES THE ENTRY -- so `execute` never reaches the confirm step and the fingerprint stays stale. On CI the unpatched call returns None ("no evidence to disagree with"), no veto fires, and the test passes. The None that made it work came from the machine, not the test. Each test now states its own assumption, in the pattern its own file already uses for every sibling: - `test_scope.py` patches `keel.commands.scope.current_credential_fingerprint` to None, as the three tests above it already patch that same target. What is under test is what the command WRITES when nothing resolves; the "nothing resolves" half belongs in the fixture. - `test_executor.py` patches BOTH sites, deliberately to DIFFERENT values: guards to None so rail 20 has nothing to disagree with and the entry reaches the confirm step, executor to "fresh..." so the overwrite under test is observable. Written into the test, because a reader will otherwise "fix" the asymmetry. **A suite-wide autouse fixture was tried first and is the wrong shape.** Recorded because the idea is tempting: stubbing `_from_keychain` made `test_the_broken_keychain_test_is_not_vacuous` vacuous (it calls that function directly to prove it reaches keyring); pinning `default_env_path` broke five `test_paper_equities_profile.py` tests that legitimately chdir and write their own `.env`; and a narrower `read_secret` wrapper broke twelve, because `test_credential_identity.py` deliberately exercises real `.env`/keychain resolution with exactly those names. This suite tests credential resolution ON PURPOSE, in several places, so a blanket "nothing resolves" fixture fights the tests that exist to exercise it. Verified in the condition that was broken -- the full suite from the repo root with a real `.env` present: 6151 passed, 3 skipped. ruff and mypy clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EXz13qp1UM3pBa6BqsRqvC --- tests/commands/test_scope.py | 12 ++++++++++-- tests/execution/test_executor.py | 18 ++++++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/tests/commands/test_scope.py b/tests/commands/test_scope.py index 9b09042..e80e344 100644 --- a/tests/commands/test_scope.py +++ b/tests/commands/test_scope.py @@ -269,14 +269,22 @@ def test_attest_writes_none_when_no_current_credential_resolves( db_path: Path, valid_config_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: _at_a_terminal(monkeypatch, yes=False) + # STATED, not inherited from the machine (#742). This asserted that the real + # `current_credential_fingerprint` resolves to None, which is true only when the process + # environment, the `.env` at `default_env_path()` and the OS keychain are ALL empty -- true + # in CI, false for any contributor with a configured deployment, where `CDP_API_KEY` + # resolves from the `.env` and this failed. The behaviour under test is what the command + # WRITES when nothing resolves, so the "nothing resolves" half belongs in the fixture. + monkeypatch.setattr( + "keel.commands.scope.current_credential_fingerprint", lambda venue: None + ) result = _run( db_path, valid_config_path, "scope", "attest", "--read-only", "--venue", "coinbase", ) assert result.exit_code == 0, result.output record = _repo_at(db_path).get_venue_trade_scope("coinbase") assert record is not None - # No credentials are configured in this test's environment, so the real - # `current_credential_fingerprint` resolves to None -- written as-is, not defaulted away. + # An unresolvable credential is written AS None, not defaulted away. assert record.credential_fingerprint is None diff --git a/tests/execution/test_executor.py b/tests/execution/test_executor.py index c792c72..73f700f 100644 --- a/tests/execution/test_executor.py +++ b/tests/execution/test_executor.py @@ -3636,8 +3636,22 @@ def test_confirming_replaces_a_stale_fingerprint_rather_than_carrying_it_forward credential_fingerprint="stale" + "0" * 27, ) - with mock.patch.object( - executor, "current_credential_fingerprint", return_value="fresh" + "0" * 27 + # BOTH import sites, and they are patched to DIFFERENT values on purpose (#742). + # `current_credential_fingerprint` is imported independently by `execution/guards.py` + # (rail 20) and by `execution/executor.py`. This test is about what the EXECUTOR writes on + # confirmation, so rail 20 must not veto first -- and it vetoes on a resolvable fingerprint + # that disagrees with the stored "stale000...". `None` is exactly the "no evidence to + # disagree with" case that lets the entry through to the confirm step. + # + # That None was previously supplied by the machine rather than the test: CI has no + # credentials, so the unpatched `guards` call returned None and the test passed. On a + # contributor's box with a `.env`, it returned a real fingerprint, rail 20 vetoed, `execute` + # never reached the confirm, and this assertion failed with the fingerprint still stale. + with ( + mock.patch.object( + executor, "current_credential_fingerprint", return_value="fresh" + "0" * 27 + ), + mock.patch.object(guards, "current_credential_fingerprint", return_value=None), ): execute(_enter_signal(), FakeBroker(), repo, _config(), mode="autonomous", now_ts=NOW_TS)