diff --git a/src/cli/bib.py b/src/cli/bib.py index 80a0ed8..04d07b9 100644 --- a/src/cli/bib.py +++ b/src/cli/bib.py @@ -220,7 +220,14 @@ def fetch_pfs_comments( docket: str = typer.Option( "", "--docket", - help="Only pull this reg.gov docket (e.g. CMS-2026-2377); other dockets are skipped before any API call once known.", + help=( + "Only pull this reg.gov docket (e.g. CMS-2026-2377). Filtered " + "before any API call ONLY once the docket is known (a `dockets` " + "row already exists for its CMS id) — a CMS id with no `dockets` " + "row yet still costs one Federal Register rule-metadata fetch " + "plus one resolve_docket call, because the reg.gov docket id " + "isn't known until those calls resolve it." + ), ), force: bool = typer.Option( False, @@ -232,7 +239,10 @@ def fetch_pfs_comments( on each docket from its stored watermark. Sealed dockets (comment period closed + quiet period + an empty pull) - cost nothing: no rule-metadata fetch, no resolve call, no walk. + cost nothing: no rule-metadata fetch, no resolve call, no walk. The + same is true for a docket already known via `--docket` that resolves + to a different id. A CMS id with no `dockets` row at all is not free + even under `--docket`: see that option's help. """ from bib import connect from bib.federalregister import pfs_rules, split_docket_ids @@ -279,6 +289,12 @@ def fetch_pfs_comments( rule.add_tag("module:pfs") for cid in cms_ids: rule.add_tag(f"cms-rule:{cid}") + # Upsert now, before any docket resolution: a CMS id whose + # docket never resolves (resolve_docket returns nothing) must + # not cost the rule item its only upsert. Each docket below + # re-upserts to add its reg-docket tag; upsert_status makes + # the extra call a no-op when nothing changed. + store.upsert(rule) for cms_id in todo: d = known[cms_id] diff --git a/src/cli/comments.py b/src/cli/comments.py index 2818a72..7c8532f 100644 --- a/src/cli/comments.py +++ b/src/cli/comments.py @@ -239,22 +239,32 @@ def extract_ocr( return body_lookup = _bib_lookup_factory(True) - engine = RapidOcrEngine() - done = errors = 0 - for i, comment_dir in enumerate(pending, 1): - try: - extract_comment( - comment_dir, - inline_body=body_lookup(comment_dir.name), - force=True, - ocr_engine=engine, - ) - done += 1 - except Exception as e: # noqa: BLE001 - log.warning("ocr extract failed for %s: %s", comment_dir.name, e) - errors += 1 - if i % 25 == 0: - typer.echo(f" {i}/{len(pending)} ok={done} errors={errors}", err=False) + try: + engine = RapidOcrEngine() + done = errors = 0 + for i, comment_dir in enumerate(pending, 1): + try: + extract_comment( + comment_dir, + inline_body=body_lookup(comment_dir.name), + force=True, + ocr_engine=engine, + ) + done += 1 + except Exception as e: # noqa: BLE001 + log.warning("ocr extract failed for %s: %s", comment_dir.name, e) + errors += 1 + if i % 25 == 0: + typer.echo( + f" {i}/{len(pending)} ok={done} errors={errors}", err=False + ) + finally: + # _bib_lookup_factory opens its own read connection to bib.sqlite + # when the store isn't :memory: (see _build_bib_helpers); close + # it here the same way extract() closes its own. + close = getattr(body_lookup, "close", None) + if close is not None: + close() typer.echo(f" done: {done} re-extracted, {errors} errors") diff --git a/tests/cli/test_bib_exercise.py b/tests/cli/test_bib_exercise.py index 911a95c..cedfdd9 100644 --- a/tests/cli/test_bib_exercise.py +++ b/tests/cli/test_bib_exercise.py @@ -287,6 +287,7 @@ class TestFetchPfsComments: @patch("bib.translate.federal_register") @patch("bib.regulations_gov.Client") def test_no_docket(self, mc_client_cls, mc_translate, mc_split, mc_pfs, mc_connect): + from bib.item import Rule from bib.store import Store store = Store(":memory:", storage_dir="/tmp/nope") @@ -299,7 +300,7 @@ class TestFetchPfsComments: doc.html_url = "https://example.com" mc_pfs.return_value = [doc] - rule = MagicMock() + rule = Rule(title="CY2024 PFS NPRM", url="https://example.com") mc_translate.return_value = rule api = MagicMock() @@ -312,6 +313,50 @@ class TestFetchPfsComments: assert result.exit_code == 0 assert "skip" in result.output + @patch("bib.connect") + @patch("bib.federalregister.pfs_rules") + @patch("bib.federalregister.split_docket_ids", return_value=["CMS-1676-P"]) + @patch("bib.translate.federal_register") + @patch("bib.regulations_gov.Client") + def test_unresolvable_docket_still_upserts_rule( + self, mc_client_cls, mc_translate, mc_split, mc_pfs, mc_connect + ): + """refs #680: a rule whose only CMS id never resolves to a reg.gov + docket must still be upserted — the upsert used to live only + inside the per-docket loop, so it was skipped entirely here.""" + from bib.item import Rule + from bib.store import Store + + store = Store(":memory:", storage_dir="/tmp/nope") + mc_connect.return_value = store + + doc = MagicMock() + doc.type = "Proposed Rule" + doc.publication_date = "2023-01-01" + doc.dockets = ["CMS-1676-P"] + doc.html_url = "https://example.com/unresolvable" + mc_pfs.return_value = [doc] + + rule = Rule(title="CY2024 PFS NPRM", url="https://example.com/unresolvable") + mc_translate.return_value = rule + + api = MagicMock() + api.__enter__ = MagicMock(return_value=api) + api.__exit__ = MagicMock(return_value=False) + mc_client_cls.return_value = api + api.resolve_docket.return_value = None + + result = runner.invoke(app, ["fetch-pfs-comments"]) + assert result.exit_code == 0 + + con = store._con() # noqa: SLF001 + row = con.execute( + "SELECT title FROM items WHERE url = ?", + ("https://example.com/unresolvable",), + ).fetchone() + assert row is not None + assert row["title"] == "CY2024 PFS NPRM" + class TestIngestMail: @patch("bib.connect") diff --git a/tests/cli/test_comments.py b/tests/cli/test_comments.py index 59a467e..b93b39e 100644 --- a/tests/cli/test_comments.py +++ b/tests/cli/test_comments.py @@ -48,6 +48,33 @@ def test_extract_ocr_empty_queue(tmp_path: Path): assert "ocr queue: 0" in result.output +def test_extract_ocr_closes_bib_lookup_connection(tmp_path: Path, monkeypatch): + """refs #680: _bib_lookup_factory opens its own read connection to + bib.sqlite (via a `.close` attribute on the returned callable — + the same contract `extract()` honours in its `finally`); extract_ocr + used to never call it, leaking the connection.""" + cdir = tmp_path / "CMS-2024-0001" / "CMS-2024-0001-0001" + cdir.mkdir(parents=True) + (cdir / "combined.md").write_text( + "---\nattachments:\n- status: ocr_needed\n---\nbody\n" + ) + + closed = {"called": False} + + def fake_lookup(comment_id: str) -> str: + return "" + + fake_lookup.close = lambda: closed.__setitem__("called", True) + + monkeypatch.setattr("cli.comments._bib_lookup_factory", lambda use_bib: fake_lookup) + monkeypatch.setattr("rex.comments.combine.extract_comment", lambda *a, **k: None) + monkeypatch.setattr("rex.comments.ocr.RapidOcrEngine", lambda: lambda p: "") + + result = runner.invoke(app, ["extract-ocr", "--root", str(tmp_path)]) + assert result.exit_code == 0, result.output + assert closed["called"] is True + + def test_stats_after_extract(tmp_path: Path): _seed(tmp_path) runner.invoke( @@ -127,3 +154,69 @@ def test_extract_attaches_combined_md_as_note(tmp_path: Path, monkeypatch): runner.invoke(app, ["extract", "--root", str(tmp_path), "--workers", "1"]) assert len(notes()) == 1 assert md_attachments() == [] + + +def test_extract_reattach_repairs_missing_note_for_current_dir( + tmp_path: Path, monkeypatch +): + """refs #680: --reattach at the CLI level is the repair path — a dir + already "current" (combined.md up to date) is normally skipped + without touching bib at all, so a note lost from under it (e.g. a + bad Zotero sync) never comes back on a plain re-run. --reattach + re-fires the attach callback for every skipped dir.""" + from bib import connect + from bib.item import Source + + bib_db = tmp_path / "bib.sqlite" + monkeypatch.setattr("conf.path", lambda _: bib_db) + + cdir = _seed(tmp_path) + comment_id = cdir.name + + store = connect(str(bib_db)) + item = Source( + title=f"Comment {comment_id}", + url=f"https://www.regulations.gov/comment/{comment_id}", + ) + item_key = store.upsert(item) + store.close() + + def note_count() -> int: + store = connect(str(bib_db)) + n = ( + store._con() + .execute( + "SELECT count(*) FROM notes n JOIN items i ON n.item_id = i.id " + "WHERE i.key = ?", + (item_key,), + ) + .fetchone()[0] + ) + store.close() + return n + + result = runner.invoke(app, ["extract", "--root", str(tmp_path), "--workers", "1"]) + assert result.exit_code == 0, result.output + assert note_count() == 1 + + # The note vanishes without combined.md changing at all — the dir + # stays "current" and a plain re-run must skip it untouched. + store = connect(str(bib_db)) + store._con().execute( + "DELETE FROM notes WHERE item_id = (SELECT id FROM items WHERE key = ?)", + (item_key,), + ) + store._con().commit() + store.close() + assert note_count() == 0 + + result = runner.invoke(app, ["extract", "--root", str(tmp_path), "--workers", "1"]) + assert result.exit_code == 0, result.output + assert "skipped: 1" in result.output.lower() + assert note_count() == 0 # not repaired without --reattach + + result = runner.invoke( + app, ["extract", "--root", str(tmp_path), "--workers", "1", "--reattach"] + ) + assert result.exit_code == 0, result.output + assert note_count() == 1 # repaired