diff --git a/keel/cli.py b/keel/cli.py index 32ab1029..e87a01ea 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -328,16 +328,22 @@ def _assess_products( ) -> list[tuple[freshness_mod.Freshness, int]]: """Read-only sweep over every (product, granularity). No network. - Returns `(freshness, unexplained_gaps)` -- the second being missing bars NOT yet proven - absent at the venue. `--fail-on-gaps` judges that number, not the raw gap count, because a - hole the venue genuinely does not have can never be closed and would keep the alert red - forever. + Returns `(freshness, unexplained_gaps)`, BOTH bounded to the same window starting at + `start_ts`. That shared window is the point: `_print_freshness` subtracts the second from + `freshness.gaps`, and `coverage()` counts those gaps over `get_candles(.., start_ts, None)` + -- so an unexplained count taken over the WHOLE series goes negative the moment bars are + missing older than `start_ts`. The field saw exactly that on 2026-08-17 (`keel fetch` + printed `-2 proven absent at venue`: an impossible claim that made still-unexplained gaps + look reconciled). `--fail-on-gaps` does NOT judge this count -- it keeps whole-series + scope, see the `check` branch of `fetch`. """ out: list[tuple[freshness_mod.Freshness, int]] = [] for product in products: for granularity in _SIM_GRANULARITIES: info = history_mod.coverage(repo, product, granularity, start_ts) - unexplained = repair_mod.unexplained_gap_count(repo, product, granularity) + unexplained = repair_mod.unexplained_gap_count( + repo, product, granularity, start_ts + ) out.append((freshness_mod.assess(info, now_ts, tolerance_bars), unexplained)) return out @@ -358,6 +364,10 @@ def _print_freshness(rows: list[tuple[freshness_mod.Freshness, int]]) -> None: if row.missing: detail = "nothing cached" else: + # No max(0, ...) clamp: `unexplained` comes from the same window-bounded read as + # `row.gaps` (see `_assess_products`), so the subtraction is consistent by + # construction. A clamp would only re-hide the mismatch that once printed + # "-2 proven absent at venue" for a series with real unexplained gaps. proven = row.gaps - unexplained suffix = f" ({proven} proven absent at venue)" if proven else "" detail = f"{row.bars_behind} bars behind, {row.gaps} internal gaps{suffix}" @@ -442,17 +452,26 @@ def fetch( if check: actionable = [r for r, _ in before if r.needs_fetch] - unexplained = [r for r, gaps in before if gaps > 0] if actionable: raise click.ClickException(f"{len(actionable)} series missing or stale") + # `--fail-on-gaps` judges the WHOLE series, not the window the display above is bounded + # to: a hole older than `start_ts` is invisible to those counts, yet `repair_series` + # probes holes wherever they sit, so it is still fixable, still unproven, and still + # this flag's business. That is also why `unexplained_gap_count`'s default stays + # whole-series -- the two calls differ on purpose, and this is the one place that + # wants the unbounded number. + unexplained = sum( + repair_mod.unexplained_gap_count(repo, product, granularity) > 0 + for product in product_list + for granularity in _SIM_GRANULARITIES + ) if unexplained and fail_on_gaps: raise click.ClickException( - f"{len(unexplained)} series have unexplained gaps -- run `keel fetch " - "--repair-gaps`" + f"{unexplained} series have unexplained gaps -- run `keel fetch --repair-gaps`" ) if unexplained: click.echo( - f"\nall series current. {len(unexplained)} have UNEXPLAINED gaps -- run " + f"\nall series current. {unexplained} have UNEXPLAINED gaps -- run " "`keel fetch --repair-gaps` to probe them." ) else: diff --git a/keel/data/repair.py b/keel/data/repair.py index 5f4389fd..42dfd54f 100644 --- a/keel/data/repair.py +++ b/keel/data/repair.py @@ -166,9 +166,32 @@ def repair_series( return result -def unexplained_gap_count(repo, product: str, granularity: Granularity) -> int: - """Missing bars NOT yet proven absent at the venue -- what `--fail-on-gaps` should judge.""" +def unexplained_gap_count( + repo, product: str, granularity: Granularity, start_ts: int | None = None +) -> int: + """Missing bars NOT yet proven absent at the venue -- what `--fail-on-gaps` should judge. + + `start_ts` exists because the fetch display subtracts this number from `coverage()`'s gap + count, and `coverage()` counts gaps over `get_candles(.., requested_start_ts, None)` -- a + window-bounded slice. The two counts must describe the SAME window or the subtraction goes + negative: on 2026-08-17 `keel fetch` printed `5 internal gaps (-1 proven absent at venue)` + for a series whose whole-series unexplained count (6) exceeded its window-bounded gap + count (5) because bars were missing older than the fetch window. Default `None` reads the + whole series, exactly the pre-parameter behavior -- which is the scope `--fail-on-gaps` + keeps, since `repair_series` probes holes wherever they sit, not only inside a fetch + window. + + Honest boundary caveat: a hole that STRADDLES `start_ts` is seen by the bounded read only + in its in-window remainder, which is not interior to the bounded slice (the bar before the + hole falls outside the window) -- so neither this count nor `coverage()`'s can see it, and + a `candle_gap_probes` record keyed by the whole-series window applies to nothing the + bounded view detects. Such a hole therefore displays as neither gapped nor unexplained. + That is the conservative direction (the window can only under-report a hole crossing its + own start boundary, never claim one absent) and it is rare: it requires a hole crossing + the fetch window's start exactly. + """ granularity = Granularity(granularity) - windows = gaps_mod.detect(repo.get_candles(product, granularity), product, granularity) + candles = repo.get_candles(product, granularity, start_ts, None) + windows = gaps_mod.detect(candles, product, granularity) known = set(repo.get_gap_probes(product, granularity)) return gaps_mod.total_missing(gaps_mod.subtract_known_absent(windows, known)) diff --git a/tests/data/test_fetch_cli.py b/tests/data/test_fetch_cli.py index 70dfd733..00f4cfef 100644 --- a/tests/data/test_fetch_cli.py +++ b/tests/data/test_fetch_cli.py @@ -308,6 +308,161 @@ def _fake_repair(client, repo_arg, product, granularity, **kwargs): assert "repairing interior gaps" in result.output +# -- the gap-proven suffix must share the fetch window (field repro, 2026-08-17) --------------- +# +# `keel fetch` printed, for real series: +# +# GAPS SOL-USD ONE_DAY n=1819 0 bars behind, 5 internal gaps (-1 proven absent at venue) +# +# A NEGATIVE "proven absent" count is an impossible claim -- you cannot prove fewer than zero +# gaps absent -- and it masked the real state: the suffix subtracted a WHOLE-SERIES unexplained +# count from a WINDOW-BOUNDED gap count, so bars missing older than the fetch window drove the +# number negative and the still-unexplained in-window gaps looked reconciled. The two counts +# must describe the same slice. + + +class _FrozenClock: + """Fixed `time` stand-in for `keel.cli`, so the rendered line is deterministic. + + `fetch` reads `now_ts` itself, so a real clock can cross a bar boundary between the test + seeding its fixture and the command computing `bars_behind` -- midnight-aligned so ONE_DAY + bars land exactly on the window edge. + """ + + def __init__(self, now_ts: int): + self._now_ts = now_ts + + def time(self) -> float: + return float(self._now_ts) + + def sleep(self, seconds: float) -> None: + return None + + +_NOW = 1_799_971_200 # midnight UTC, day-aligned +_START = _NOW - 365 * _DAY # what `fetch --years 1` computes as the window start + + +def _seed_sol_field_repro(repo: Repository) -> None: + """SOL-USD ONE_DAY with the field's exact shape: (a) an in-window hole recorded absent in + `candle_gap_probes`, (b) an in-window hole NOT recorded, and (c) a 2-bar hole entirely + OLDER than the fetch window.""" + day = Granularity.ONE_DAY + last_day = _NOW - _DAY # newest complete bar: 0 bars behind + + # (c): 8 bars ending the day before the window starts, with a 2-bar hole inside them. + old = [_START - i * _DAY for i in range(1, 11) if i not in (5, 6)] + # (a)+(b): every window bar, minus one hole at +10d and one at +20d. + n_window_days = (last_day - _START) // _DAY + 1 + window = [ + _START + i * _DAY + for i in range(n_window_days) + if i not in (10, 20) + ] + _seed(repo, "SOL-USD", day, old + window) + + # The venue was asked about the +10d hole and had nothing -- as `repair_series` records. + repo.record_gap_probe("SOL-USD", day, _START + 10 * _DAY, _START + 10 * _DAY, 1, _NOW) + + # ONE_HOUR current, so `--check` is not distracted by a missing series. + last_hour = _NOW - _HOUR + _seed(repo, "SOL-USD", Granularity.ONE_HOUR, [last_hour - i * _HOUR for i in range(48)]) + + +def test_the_gap_suffix_shares_the_fetch_window_so_it_cannot_go_negative( + tmp_path, valid_config_path, monkeypatch +): + """The exact rendered line for the field fixture. + + Window-bounded truth: 2 in-window gaps, exactly 1 of them proven absent. The 2 bars + missing BEFORE the window must perturb neither number -- under the old whole-series + subtraction they turned the suffix into `(-1 proven absent at venue)`, and the unproven + +20d hole rode along looking reconciled. + """ + _no_network(monkeypatch) + monkeypatch.setattr(cli_module, "time", _FrozenClock(_NOW)) + db_path = tmp_path / "t.db" + repo = _repo_at(db_path) + _seed_sol_field_repro(repo) + + result = CliRunner().invoke( + cli, + ["--db", str(db_path), "--config", str(valid_config_path), + "fetch", "--check", "--years", "1", "--products", "SOL-USD"], + ) + + assert ( + " GAPS SOL-USD ONE_DAY n=363 0 bars behind, " + "2 internal gaps (1 proven absent at venue)" in result.output + ), result.output + # The whole-series truth the display no longer pretends to state: bars are still missing + # outside the window, and the closing message still says so. + assert "1 have UNEXPLAINED gaps" in result.output + + +def test_every_assessed_row_keeps_proven_absent_never_negative(tmp_path, monkeypatch): + """`row.gaps >= unexplained` in every `_assess_products` row, by construction. + + The two counts now read the same window, so `proven = gaps - unexplained` cannot go + negative -- the field printed `(-2 proven absent at venue)` from exactly this pair + disagreeing about scope. Pinned directly so the invariant survives refactors of the + renderer that no longer visibly subtract. + """ + db_path = tmp_path / "t.db" + repo = _repo_at(db_path) + _seed_sol_field_repro(repo) + + rows = cli_module._assess_products(repo, ["SOL-USD"], _NOW, _START, tolerance_bars=2) + assert rows, "fixture must assess something for this test to mean anything" + for row, unexplained in rows: + assert row.gaps >= unexplained, (row.product, row.granularity, row.gaps, unexplained) + + +def test_fail_on_gaps_still_judges_holes_older_than_the_fetch_window( + tmp_path, valid_config_path, monkeypatch +): + """`--fail-on-gaps` scope is deliberately UNCHANGED: the whole series. + + The window display cannot see a hole before `start_ts` (neither of its counts covers it), + but `repair_series` probes holes wherever they sit, so such a hole is still fixable, still + unproven, and must still fail the strict check. Narrowing the flag to the window would + green-light a series the repair command itself still lists work for. Born green, on + purpose: it pins preserved semantics, not the regression the sibling tests reproduce. + """ + _no_network(monkeypatch) + monkeypatch.setattr(cli_module, "time", _FrozenClock(_NOW)) + db_path = tmp_path / "t.db" + repo = _repo_at(db_path) + + # Current and CONTIGUOUS inside the window (displays "ok"); 4 bars missing before it. + last_day = _NOW - _DAY + n_window_days = (last_day - _START) // _DAY + 1 + _seed( + repo, + "SOL-USD", + Granularity.ONE_DAY, + [_START - 6 * _DAY, _START - _DAY] + + [_START + i * _DAY for i in range(n_window_days)], + ) + _seed(repo, "SOL-USD", Granularity.ONE_HOUR, [_NOW - _HOUR - i * _HOUR for i in range(48)]) + + args = ["--db", str(db_path), "--config", str(valid_config_path), + "fetch", "--check", "--years", "1", "--products", "SOL-USD"] + + plain = CliRunner().invoke(cli, args) + assert plain.exit_code == 0, plain.output + # The display is window-bounded and honest about it: the day series shows "ok"... + assert " ok SOL-USD ONE_DAY n=365 0 bars behind, 0 internal gaps" in ( + plain.output + ) + # ...while the whole-series judgment still counts the outside-window hole. + assert "1 have UNEXPLAINED gaps" in plain.output + + strict = CliRunner().invoke(cli, [*args, "--fail-on-gaps"]) + assert strict.exit_code != 0 + assert "unexplained gaps" in strict.output + + # -- --products validation: a SHAPE error is fatal, a SETTLEMENT mismatch is not --------------- # # Feasibility study R2, corrected. Validating `--products` where the operator types it is right diff --git a/tests/data/test_gap_repair.py b/tests/data/test_gap_repair.py index 74f2c0be..9a9f7afc 100644 --- a/tests/data/test_gap_repair.py +++ b/tests/data/test_gap_repair.py @@ -252,6 +252,56 @@ def test_unexplained_gap_count_excludes_proven_absences(repo): assert repair_mod.unexplained_gap_count(repo, "BTC-USD", Granularity.ONE_DAY) == 0 +def test_unexplained_gap_count_default_still_reads_the_whole_series(repo): + """Pin the default: no `start_ts` means the WHOLE series, exactly as before the parameter + existed. + + `--fail-on-gaps` judges this default. A hole older than any fetch window is still a hole + `repair_series` will probe (it reads the whole series itself), so the flag's count must + keep seeing it -- narrowing the default here would silently narrow the flag. + """ + _seed(repo, missing={2, 7}) + assert repair_mod.unexplained_gap_count(repo, "BTC-USD", Granularity.ONE_DAY) == 2 + + +def test_unexplained_gap_count_bounded_ignores_holes_before_start_ts(repo): + """With `start_ts`, only holes at/after the boundary count. + + The fetch display subtracts this number from `coverage()`'s gap count, and `coverage()` + counts gaps over `get_candles(.., requested_start_ts, None)` -- so the two counts must + describe the SAME slice or the subtraction goes negative (see the field repro in + `tests/data/test_fetch_cli.py`). + """ + _seed(repo, missing={2, 7}) + boundary = _BASE + 5 * _DAY # between the two holes + bounded = repair_mod.unexplained_gap_count( + repo, "BTC-USD", Granularity.ONE_DAY, start_ts=boundary + ) + assert bounded == 1 # only the hole at index 7; the one at index 2 is before the window + + +def test_a_hole_straddling_start_ts_is_invisible_to_the_bounded_count(repo): + """The honest boundary caveat, pinned. + + A hole whose span crosses `start_ts` is seen by the bounded read only in its in-window + remainder, which is NOT interior to the bounded slice (the bar before the hole falls + outside the window), so neither the bounded gap count nor this count can see it -- and a + `candle_gap_probes` record keyed by the whole-series window does not apply to any window + the bounded view does see. The conservative direction: the window display can only + UNDER-report a hole that crosses its own start boundary, never claim one absent, and the + whole-series default above still sees it for `--fail-on-gaps`. + """ + _seed(repo, missing={4, 5, 6}) + boundary = _BASE + 5 * _DAY # inside the hole + bounded = repair_mod.unexplained_gap_count( + repo, "BTC-USD", Granularity.ONE_DAY, start_ts=boundary + ) + assert bounded == 0 + # The docstring's closing claim, asserted for THIS fixture: the whole-series default + # still sees all three bars, so `--fail-on-gaps` keeps its un-narrowed scope. + assert repair_mod.unexplained_gap_count(repo, "BTC-USD", Granularity.ONE_DAY) == 3 + + def test_a_shifted_window_is_treated_as_new_and_re_probed(repo): """Conservative on purpose: exact-key matching only.