From 333339a10f804056f8f8b87d1e732b5b76ff034a Mon Sep 17 00:00:00 2001 From: arch-colony Date: Tue, 18 Aug 2026 19:27:55 +0100 Subject: [PATCH] --limit is a budget of analyses, and the fetch window stays materialised MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review items that look unrelated and are not: fixing the first the obvious way breaks the second. (1) --limit MEANT "posts fetched". Local memory, --days and own-post filters all run AFTER the fetch, so asking for 10 routinely analysed three and said nothing about it. That is the same shape as the bug in Now the scan fetches a candidate window of N * SCAN_FETCH_MULTIPLIER (capped at SCAN_FETCH_CAP) and stops at N ANALYSES. Anything past the budget is left unanalysed AND unmarked, so the next run takes it. When the window runs dry first the run says so, instead of finishing quietly: Analysed 2 of a requested 10 — the 30-post candidate window held no more eligible posts (already analysed, older than --days 7, or the sentinel's own). (2) THE OBVIOUS FIX FOR (1) IS WRONG. Consuming iter_posts lazily until N are analysed would make --limit honest and silently break the scan: _process_post marks each post scanned, which REMOVES it from the sentinel_scanned=false set the server is paginating, and iter_posts pages by offset. Mark k posts and the next page starts k further into a set that just lost k members — skipping k posts that were never seen. So list(...) is load-bearing, not incidental. It now says so at the call site, in the README, and in a test that asserts nothing is pulled off the generator after the first analysis begins. That test is the only thing standing between a future "why materialise? just iterate" and a scanner that skips one post for every post it finishes. Mutation-verified both ways: drop list(...) -> 2 tests fail, both in TestTheWindowIsMaterialised drop the break -> 4 tests fail, incl. the budget and untouched-posts 210 tests pass, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Pxqw6ZyFv5f4rt5HFwtafs --- README.md | 42 ++++++- sentinel.py | 40 ++++++- tests/test_scan_batch_semantics.py | 175 +++++++++++++++++++++++++++++ 3 files changed, 255 insertions(+), 2 deletions(-) create mode 100644 tests/test_scan_batch_semantics.py diff --git a/README.md b/README.md index cb136b9..24eca34 100644 --- a/README.md +++ b/README.md @@ -147,7 +147,9 @@ CLI flags shown in `python3 sentinel.py {scan,webhook,webhook-register} --help`. | `DEFAULT_MODEL` | `qwen3.5:9b-q4_k_m` | `SENTINEL_MODEL` | Ollama model. Must be an installed tag — Ollama lowercases tags on pull, and a startup preflight fails fast (with `ollama pull …`) if it's missing. Also overridable per-run via `--model`. | | `REQUIRE_GPU` | `1` (on) | `SENTINEL_REQUIRE_GPU` | Refuse to run if the model is answering from CPU. See **GPU requirement** below. Per-run escape hatch: `--allow-cpu`. | | `GPU_MIN_OFFLOAD` | `0.9` | `SENTINEL_GPU_MIN_OFFLOAD` | Fraction of the model that must sit in VRAM before the run is considered healthy. Below it: a warning. At zero: abort. | -| `DEFAULT_LIMIT` | 20 | — (`--limit`) | Posts per scan run | +| `DEFAULT_LIMIT` | 20 | — (`--limit`) | Posts **analysed** per scan run — a budget, not a fetch size. See **How `--limit` is counted** below. | +| `SCAN_FETCH_MULTIPLIER` | 3 | `SCAN_FETCH_MULTIPLIER` | Candidates fetched per post the run intends to analyse, so post-fetch skips don't shrink the batch | +| `SCAN_FETCH_CAP` | 200 | `SCAN_FETCH_CAP` | Hard ceiling on that candidate window | | `MAX_COMMENTS` | 6 | — | Top comments included in analysis | | `DEFAULT_DAYS` | 7 | — (`--days`) | Only analyze posts newer than this (scan mode) | | `OLLAMA_TIMEOUT` | 180 | `OLLAMA_TIMEOUT` | Seconds before a single Ollama generation is cut off (the runaway bound — a wedged model can't hang the scan) | @@ -216,6 +218,44 @@ Three outcomes: To run on CPU deliberately — a box with no GPU where a slow scan is acceptable — pass `--allow-cpu`, or set `SENTINEL_REQUIRE_GPU=0`. +## How `--limit` is counted + +`--limit N` means **analyse up to N posts**, not "fetch N and analyse whatever +survives". + +Several filters run *after* the fetch — posts already in local memory, posts +older than `--days`, and the sentinel's own posts. Fetching exactly N therefore +routinely analysed fewer than N, and said nothing about it. `--limit 10` +quietly delivering three is the same shape as the bug where the scanner fetched +the newest ten and skipped all ten: a run that reports success having done less +work than it was asked for. + +So the scan fetches a **candidate window** of `N × SCAN_FETCH_MULTIPLIER` +(capped at `SCAN_FETCH_CAP`), and stops once N posts have actually been +analysed. Anything past the budget is left unanalysed *and unmarked*, so the +next run picks it up. If the window runs dry first, the run says so explicitly +rather than finishing quietly: + +``` +Analysed 2 of a requested 10 — the 30-post candidate window held no more +eligible posts (already analysed, older than --days 7, or the sentinel's own). +``` + +### Why the window is read all at once + +The fetch is deliberately materialised (`list(...)`) before the first post is +processed, and that is load-bearing rather than incidental. + +Processing a post marks it scanned, which **removes it** from the +`sentinel_scanned=false` set the server is paginating — and `iter_posts` pages +by offset. Consuming the fetch lazily would have the window slide underneath +the scan: mark *k* posts, and the next page starts *k* further into a set that +just lost *k* members, skipping *k* posts that were never seen. Reading the +whole window before the first mark is what keeps the offsets stable. + +`tests/test_scan_batch_semantics.py` pins this by asserting no post is pulled +off the generator after the first analysis begins. + ## Vote-exemption lists Two optional files on the sentinel's own filesystem name users this instance must never vote on: diff --git a/sentinel.py b/sentinel.py index 21efee4..38d86d0 100644 --- a/sentinel.py +++ b/sentinel.py @@ -141,6 +141,19 @@ def _env_int(name: str, default: int) -> int: # the next run re-burns the model over all of them. The write is atomic. SCAN_SAVE_EVERY = _env_int("SCAN_SAVE_EVERY", 10) +# --limit means "analyse up to N posts", not "fetch N and analyse whatever +# survives". Several filters run AFTER the fetch — local memory, --days, and +# the sentinel's own posts — so a straight fetch of N routinely analysed fewer +# than N and said nothing about it. That is the same shape as the bug where +# the scanner fetched the newest ten and skipped all ten: a run that reports +# success having done less work than asked. +# +# So over-fetch, then stop at N analyses. The multiplier is a candidate +# budget, not a promise; SCAN_FETCH_CAP bounds it so a large --limit cannot +# turn into an enormous request. +SCAN_FETCH_MULTIPLIER = _env_int("SCAN_FETCH_MULTIPLIER", 3) +SCAN_FETCH_CAP = _env_int("SCAN_FETCH_CAP", 200) + DEFAULT_USERNAME = "qwen-jorwhol-analyzer" DEFAULT_DISPLAY_NAME = "Qwen 3.5 Jorwhol Analyzer" DEFAULT_BIO = ( @@ -1416,10 +1429,20 @@ def _checkpoint() -> None: new_analyses += 1 else: scanned_filter = scanned_filter_for(args) + fetch_n = min(args.limit * SCAN_FETCH_MULTIPLIER, SCAN_FETCH_CAP) try: + # MATERIALISED ON PURPOSE — do not make this lazy. + # + # _process_post marks each post scanned as it goes, which + # REMOVES it from the sentinel_scanned=false set the server is + # paginating. iter_posts pages by offset, so a lazily-consumed + # generator would have the window slide underneath it and skip + # one unseen post for every post already marked. Collecting the + # whole window before the first mark is what makes the offsets + # stable. Pinned by tests/test_scan_batch_semantics.py. posts = list(client.iter_posts( sort=args.sort, - max_results=args.limit, + max_results=fetch_n, sentinel_scanned=scanned_filter, )) except ColonyAPIError as e: @@ -1427,6 +1450,10 @@ def _checkpoint() -> None: sys.exit(1) for post in posts: + if new_analyses >= args.limit: + # --limit is a budget of ANALYSES. Everything past this is + # left unmarked, so the next run picks it up. + break post_id = post.get("id") title = (post.get("title") or "")[:70] created_at = post.get("created_at") @@ -1461,6 +1488,17 @@ def _checkpoint() -> None: # Always persist what we finished — even on Ctrl-C / crash / sys.exit. _checkpoint() + # Say so when the run could not fill its budget. Silence here is what let + # "analysed 3 of a requested 10" read as a healthy scan. + if not args.post_id and new_analyses < args.limit: + logger.info( + "Analysed %d of a requested %d — the %d-post candidate window held " + "no more eligible posts (already analysed, older than --days %d, or " + "the sentinel's own). Raise --limit, widen --days, or raise " + "SCAN_FETCH_MULTIPLIER if you expected more.", + new_analyses, args.limit, fetch_n, args.days, + ) + if not args.dry_run: logger.info("Memory updated: %d posts (added %d new)", len(memory), new_analyses) else: diff --git a/tests/test_scan_batch_semantics.py b/tests/test_scan_batch_semantics.py new file mode 100644 index 0000000..e5911b4 --- /dev/null +++ b/tests/test_scan_batch_semantics.py @@ -0,0 +1,175 @@ +"""`--limit N` means "analyse up to N posts", and the fetch is materialised. + +Two properties that look unrelated and are not: fixing the first naively +breaks the second. + +**--limit is a budget of ANALYSES.** Several filters run AFTER the fetch — +local memory, `--days`, and the sentinel's own posts — so fetching exactly N +routinely analysed fewer than N and said nothing about it. `--limit 10` +delivering three is the same shape as the bug in #22: a run that reports +success having done less work than asked. So the scan over-fetches a +candidate window and stops at N analyses. + +**The window is materialised before the first mark.** The obvious way to make +--limit honest is to consume `iter_posts` lazily until N are analysed. That is +wrong here, and quietly so. `_process_post` marks each post scanned, which +REMOVES it from the `sentinel_scanned=false` set the server is paginating, and +`iter_posts` pages by offset. A lazy generator would have the window slide +underneath it: mark k posts, and the next page starts k posts further into a +set that just lost k members, skipping k posts that were never seen. Collecting +the whole window before the first mark is what keeps the offsets stable. + +That is why `list(...)` is load-bearing rather than incidental, and why +`test_the_window_is_fully_read_before_anything_is_marked` exists — it is the +only thing standing between a future "optimisation" and a scanner that +silently skips a post for every post it completes. +""" +from __future__ import annotations + +from types import SimpleNamespace + +import pytest + +import sentinel as s + + +def _args(**over): + base = dict( + dry_run=True, no_vote=True, no_pii=True, model="m", username=None, + force=False, post_id=None, sort="new", limit=3, days=7, confirm=False, + include_scanned=False, allow_cpu=True, + ) + base.update(over) + return SimpleNamespace(**base) + + +def _post(i, *, author="someone", created=None): + from datetime import datetime, timedelta, timezone + when = created or (datetime.now(timezone.utc) - timedelta(hours=1)).isoformat() + return {"id": f"post-{i:03d}", "title": f"Post {i}", + "created_at": when, "author": {"username": author}} + + +@pytest.fixture +def harness(monkeypatch, tmp_path): + """Drive cmd_scan with every side effect stubbed except the batch logic.""" + monkeypatch.setattr(s, "MEMORY_FILE", tmp_path / "mem.json") + monkeypatch.setattr(s, "ensure_model_available", lambda m: None) + monkeypatch.setattr(s, "ensure_gpu_available", lambda m: None, raising=False) + monkeypatch.setattr(s, "log_vote_exemptions", lambda: None) + + state = SimpleNamespace(requested=[], analysed=[], yielded=[], events=[]) + + client = SimpleNamespace() + + def iter_posts(**kw): + state.requested.append(kw) + for p in state.available: + state.yielded.append(p["id"]) + state.events.append(("yield", p["id"])) + yield p + + client.iter_posts = iter_posts + monkeypatch.setattr(s, "get_or_register_client", + lambda u: (client, {"username": "sentinel-bot"})) + monkeypatch.setattr(s, "fetch_post_with_comments", + lambda c, pid: {"post": {"id": pid}}) + + def process(client_, data, args, memory, results): + pid = data["post"]["id"] + state.analysed.append(pid) + state.events.append(("analyse", pid)) + memory[pid] = {"score": 5} + return True + + monkeypatch.setattr(s, "_process_post", process) + state.available = [] + return state + + +class TestLimitIsABudgetOfAnalyses: + def test_it_stops_at_the_limit(self, harness): + harness.available = [_post(i) for i in range(10)] + s.cmd_scan(_args(limit=3)) + assert harness.analysed == ["post-000", "post-001", "post-002"] + + def test_it_over_fetches_so_skips_do_not_shrink_the_batch(self, harness): + """The whole point: ask for more candidates than the budget, because + some will be filtered out after the fetch.""" + harness.available = [_post(i) for i in range(30)] + s.cmd_scan(_args(limit=5)) + assert harness.requested[0]["max_results"] == 5 * s.SCAN_FETCH_MULTIPLIER + + def test_the_over_fetch_is_capped(self, harness): + """A large --limit must not turn into an enormous request.""" + harness.available = [] + s.cmd_scan(_args(limit=10_000)) + assert harness.requested[0]["max_results"] == s.SCAN_FETCH_CAP + + def test_skipped_posts_do_not_consume_the_budget(self, harness): + """Three of the first four are the sentinel's own; a budget of 3 must + still analyse 3, not 1. This is the behaviour that was broken.""" + harness.available = [ + _post(0, author="sentinel-bot"), + _post(1), + _post(2, author="sentinel-bot"), + _post(3), + _post(4), + _post(5), + ] + s.cmd_scan(_args(limit=3)) + assert harness.analysed == ["post-001", "post-003", "post-004"] + + def test_posts_beyond_the_budget_are_left_untouched(self, harness): + """Anything past the limit must stay unanalysed AND unmarked, so the + next run picks it up rather than it being silently consumed.""" + harness.available = [_post(i) for i in range(10)] + s.cmd_scan(_args(limit=2)) + assert "post-002" not in harness.analysed + assert len(harness.analysed) == 2 + + def test_a_short_batch_is_reported(self, harness, caplog): + """Silence here is what let 'analysed 3 of a requested 10' read as a + healthy scan.""" + harness.available = [_post(0), _post(1)] + with caplog.at_level("INFO"): + s.cmd_scan(_args(limit=10)) + assert any("Analysed 2 of a requested 10" in r.message for r in caplog.records) + + def test_a_full_batch_is_not_reported_as_short(self, harness, caplog): + harness.available = [_post(i) for i in range(10)] + with caplog.at_level("INFO"): + s.cmd_scan(_args(limit=3)) + assert not any("of a requested" in r.message for r in caplog.records) + + +class TestTheWindowIsMaterialised: + def test_the_window_is_fully_read_before_anything_is_marked(self, harness): + """The load-bearing one. + + Every post must be pulled off the generator BEFORE the first analysis + (and therefore before the first mark_post_scanned). If this ever + interleaves, the server-side offset window shifts under the scanner + and it skips one unseen post per post completed — silently, with a + healthy-looking log. + """ + harness.available = [_post(i) for i in range(6)] + s.cmd_scan(_args(limit=6)) + + kinds = [k for k, _ in harness.events] + first_analyse = kinds.index("analyse") + assert "yield" not in kinds[first_analyse:], ( + "the fetch is being consumed lazily — posts were still being " + "pulled after marking began, so the offset window will slide and " + "the scan will skip work. Keep the list(...) around iter_posts." + ) + assert harness.yielded == [f"post-{i:03d}" for i in range(6)] + + def test_the_whole_window_is_read_even_when_the_budget_is_smaller(self, harness): + """A consequence worth stating: over-fetching means we read more than + we analyse. That is the price of stable offsets, and it is cheap — the + fetch is one request, the analysis is an LLM call per post.""" + harness.available = [_post(i) for i in range(9)] + s.cmd_scan(_args(limit=3)) + assert len(harness.yielded) == 9 + assert len(harness.analysed) == 3