diff --git a/casework/README.md b/casework/README.md index aa915f50..82954db3 100644 --- a/casework/README.md +++ b/casework/README.md @@ -197,17 +197,18 @@ against a bound case and left its `evidence` rows and their notes untouched. ### Writing `evidence` or `entities` — read, merge, send the whole list `PATCH /evidence` **replaces the entire list.** The server deletes every existing -row and recreates from exactly what you send (`_write_material_references`, -`cases/api_views.py:647`). Send only your new document and you delete the press -release and court order someone bound last month. +row and recreates from exactly what you send (`cases.api_views._write_material_references`). +Send only your new document and you delete the press release and court order +someone bound last month. Never send a delta. Read the case, merge into what is already there, send it all: ```python -from casework.bind_materials import current_evidence, merge_evidence +from casework.common.evidence import current_evidence, merge_evidence case, etag = api.get_case_with_etag(slug) -merged = merge_evidence(current_evidence(case), [new_iri]) # append, keep order +# (material_iri, note) pairs. Pass "" when the stage has no note to bind yet. +merged = merge_evidence(current_evidence(case), [(new_iri, "")]) api.replace_list(slug, "evidence", merged, if_match=etag) ``` diff --git a/casework/bind_materials.py b/casework/bind_materials.py index f10338eb..05ea83bb 100644 --- a/casework/bind_materials.py +++ b/casework/bind_materials.py @@ -36,6 +36,7 @@ from dataclasses import dataclass, field from casework.common.api import CaseworkApi +from casework.common.evidence import current_evidence, merge_evidence from casework.common.cli import ( _utc_iso_now, add_common_args, basic_auth_from_env, configure_run_logging, log_run_footer, log_run_header, print_summary, @@ -121,44 +122,6 @@ def n_merged(self): return len(self.patch_items) if self.action == "WOULD_PATCH" else self.n_current -def current_evidence(case): - """Normalize the case's evidence into the {material_iri, additional_details} - shape the PATCH expects, preserving order. - - DELIBERATELY DUPLICATED in `casework/enrich_news_articles.py`. Both stages - write the same destructive whole-list `PATCH /evidence`, so both must - normalise it identically; the copy is kept rather than shared because these - are standalone scripts with no shared sequencing. Change one, change the - other -- a divergence here silently drops evidence rows. - """ - return [ - {"material_iri": e.get("material_iri"), - "additional_details": e.get("additional_details") or ""} - for e in (case.get("evidence") or []) - if e.get("material_iri") - ] - - -def merge_evidence(current, add_iris): - """Append each new IRI not already present, preserving existing order and - de-duplicating. Never reorders or drops an existing entry -- the whole-list - replace makes any omission destructive. - - DELIBERATELY DUPLICATED as `merge_news_evidence` in - `casework/enrich_news_articles.py`, which differs in ONE respect: it binds - the Nepali evidence note at the same time instead of `additional_details: - ""` (its deviation 2). The append-only contract is identical and must stay - that way in both. - """ - have = {e["material_iri"] for e in current} - merged = list(current) - for iri in add_iris: - if iri not in have: - merged.append({"material_iri": iri, "additional_details": ""}) - have.add(iri) - return merged - - def missing_candidates(case, candidates): """Return the ``(source, ident)`` candidates NOT already bound to ``case``. @@ -213,7 +176,8 @@ def plan_case(api, case, etag, candidates, required_state=REQUIRED_STATE): dropped=dropped, uncertain=uncertain, probes=probes, reason=f"{len(uncertain)} material(s) uncertain") - merged = merge_evidence(current, add) + # This stage binds no note; the news stage passes a real one. + merged = merge_evidence(current, [(iri, "") for iri in add]) if merged == current: return BindPlan(slug=slug, action="NOOP", state=state, if_match=etag, n_current=len(current), dropped=dropped, probes=probes) diff --git a/casework/common/evidence.py b/casework/common/evidence.py new file mode 100644 index 00000000..60bd19cb --- /dev/null +++ b/casework/common/evidence.py @@ -0,0 +1,35 @@ +"""Read and merge a case's `/evidence` list. Shared by every stage that writes it.""" + + +def current_evidence(case): + """The case's evidence in the `{material_iri, additional_details}` shape PATCH wants.""" + return [ + {"material_iri": e.get("material_iri"), + "additional_details": e.get("additional_details") or ""} + for e in (case.get("evidence") or []) + if e.get("material_iri") + ] + + +def merge_evidence(current, additions): + """Append `(material_iri, note)` pairs not already present, preserving order. + + Never reorders, rewrites or drops an existing entry: `PATCH /evidence` replaces + the whole list, so anything left out is deleted. An IRI already present is + skipped rather than allowed to overwrite a note a human may have edited. + """ + have = {e["material_iri"] for e in current} + merged = list(current) + for addition in additions: + # A bare IRI is the natural mistake here, and a 2-character string would + # unpack into iri="a", note="b" and bind silently. Say so instead. + if isinstance(addition, str): + raise TypeError( + f"merge_evidence takes (material_iri, note) pairs, got the bare " + f"string {addition!r}. Pass [(iri, '')] if there is no note.") + iri, note = addition + if iri in have: + continue + merged.append({"material_iri": iri, "additional_details": note}) + have.add(iri) + return merged diff --git a/casework/enrich_news_articles.py b/casework/enrich_news_articles.py index 72d3ba34..f285f541 100644 --- a/casework/enrich_news_articles.py +++ b/casework/enrich_news_articles.py @@ -40,9 +40,9 @@ byte-compatible with the 48 `/material/news/*` rows already in production; the IRI form is not invented here (see `news_search.news_material_ident`). -DEVIATION 2 -- THE NOTE IS WRITTEN AT BIND TIME, NEVER BLANK. `bind_materials.py -:143` appends new evidence with `additional_details: ""` and leaves a later stage -to backfill. This stage has the article in hand and the verifier has already +DEVIATION 2 -- THE NOTE IS WRITTEN AT BIND TIME, NEVER BLANK. `bind_materials` +appends new evidence with `additional_details: ""` and leaves a later stage to +backfill. This stage has the article in hand and the verifier has already produced the Nepali note, so binding blank would cost a second document fetch of a document already read and strand the entry meanwhile. The register and length come from the 33 news notes on the 15 IN_REVIEW cases, read from production @@ -103,6 +103,7 @@ from dataclasses import dataclass, field from casework.common.api import CaseworkApi +from casework.common.evidence import current_evidence, merge_evidence from casework.common.cli import ( add_common_args, basic_auth_from_env, @@ -190,24 +191,6 @@ def _require_loopback(api): # --------------------------------------------------------------------------- -def current_evidence(case): - """The case's evidence normalised to the `{material_iri, additional_details}` - shape the PATCH expects, order preserved. - - Byte-identical to `bind_materials.current_evidence` and imported-by-copy on - purpose: this is the contract for what a whole-list replace must send back, - and the two writers of `/evidence` must not be able to disagree about it. - Consolidating them into `casework/common/` is a separate change that would - edit a module this port has no other reason to touch. - """ - return [ - {"material_iri": e.get("material_iri"), - "additional_details": e.get("additional_details") or ""} - for e in (case.get("evidence") or []) - if e.get("material_iri") - ] - - def bound_news_urls(case): """Every article URL already bound to this case, normalised for comparison. @@ -228,27 +211,6 @@ def count_news_evidence(case): return len(materials_of_type(case, (NEWS_MATERIAL_TYPE,))) -def merge_news_evidence(current, additions): - """Union-merge `additions` into `current`, preserving existing order. - - `bind_materials.merge_evidence` with one difference: an appended entry - carries its note instead of `""` (deviation 2). Everything else is that - function's contract, and it is the load-bearing half -- an existing entry is - never reordered, rewritten or dropped, because the server deletes every row - and recreates from exactly what is sent, so any omission destroys data. An - addition whose IRI is already present is skipped rather than allowed to - overwrite the note a human may have edited. - """ - have = {e["material_iri"] for e in current} - merged = list(current) - for iri, note in additions: - if iri in have: - continue - merged.append({"material_iri": iri, "additional_details": note}) - have.add(iri) - return merged - - # --------------------------------------------------------------------------- # The read phase. Search -> fetch -> verify -> select. No writes. # --------------------------------------------------------------------------- @@ -531,7 +493,7 @@ def plan_case(case, etag, outcome, *, client=None, save_permalinks=True): materials.append((iri, doc, verdict.summary, article)) additions.append((iri, verdict.summary)) - merged = merge_news_evidence(current, additions) + merged = merge_evidence(current, additions) if merged == current: return NewsPlan(slug=slug, action="NOOP", state=state, if_match=etag, n_current=len(current), outcome=outcome, diff --git a/casework/news_search.py b/casework/news_search.py index 609d65a9..6c985d36 100644 --- a/casework/news_search.py +++ b/casework/news_search.py @@ -282,9 +282,9 @@ def is_bindable(self): All four conditions are load-bearing. `high` is the bar (see the module docstring). `event_type` must be a real lifecycle value because the per-event cap and the bind ordering both key on it. `summary` must be - SUBSTANTIAL because it IS the evidence note -- binding without one is the - blank-note behaviour in `bind_materials.merge_evidence` that this port - exists to avoid, and a one-line note is that behaviour with extra steps. + SUBSTANTIAL because it IS the evidence note -- an entry bound blank is + what this stage exists to avoid, and a one-line note is that with extra + steps. The length floor is not cosmetic. `salvage_json` repairs a reply truncated at `max_tokens` by closing the open string, so an overflowing verify call diff --git a/tests/casework/test_bind_materials.py b/tests/casework/test_bind_materials.py index b5847584..00169625 100644 --- a/tests/casework/test_bind_materials.py +++ b/tests/casework/test_bind_materials.py @@ -7,9 +7,10 @@ import pytest +from casework.common.evidence import merge_evidence from casework.bind_materials import ( BindPlan, _build_api, _ledger_status, apply_plan, candidates_from_row, - merge_evidence, missing_candidates, parse_source_ident, plan_case, run, + missing_candidates, parse_source_ident, plan_case, run, ) HOST = "https://jawafdehi.org/material" @@ -87,13 +88,22 @@ def test_candidates_from_row_strips_status_dedupes_and_orders(): def test_merge_evidence_appends_new_preserving_order(): current = [{"material_iri": PR, "additional_details": ""}] - merged = merge_evidence(current, [CO]) + merged = merge_evidence(current, [(CO, "")]) assert [e["material_iri"] for e in merged] == [PR, CO] def test_merge_evidence_is_idempotent_on_existing(): current = [{"material_iri": PR, "additional_details": "note"}] - assert merge_evidence(current, [PR]) == current + assert merge_evidence(current, [(PR, "")]) == current + + +def test_merge_evidence_refuses_bare_iris(): + """A bare IRI is the natural mistake, and a 2-char one would unpack into + iri="a", note="b" and bind garbage without complaint.""" + with pytest.raises(TypeError, match="material_iri"): + merge_evidence([], [PR]) + with pytest.raises(TypeError, match="material_iri"): + merge_evidence([], ["ab"]) # --------------------------------------------------------------------------- diff --git a/tests/casework/test_enrich_news_articles.py b/tests/casework/test_enrich_news_articles.py index ecb6bc69..f8493af5 100644 --- a/tests/casework/test_enrich_news_articles.py +++ b/tests/casework/test_enrich_news_articles.py @@ -34,6 +34,7 @@ import pytest +from casework import bind_materials from casework import enrich_news_articles as en from casework import news_search as ns from tests.casework.fakes import FakeUsage @@ -725,7 +726,7 @@ def test_the_merge_appends_and_never_disturbs_an_existing_entry(): "additional_details": "a human wrote this"}, {"material_iri": "https://jawafdehi.org/material/court_order/2", "additional_details": ""}] - merged = en.merge_news_evidence( + merged = en.merge_evidence( current, [("https://jawafdehi.org/material/news/20240101.aaaaaaaa", "नयाँ नोट")]) assert merged[:2] == current assert merged[2] == { @@ -736,12 +737,12 @@ def test_the_merge_appends_and_never_disturbs_an_existing_entry(): def test_the_merge_never_overwrites_a_note_on_an_iri_already_present(): iri = "https://jawafdehi.org/material/news/20240101.aaaaaaaa" current = [{"material_iri": iri, "additional_details": "a human edited this"}] - merged = en.merge_news_evidence(current, [(iri, "the model would say this")]) + merged = en.merge_evidence(current, [(iri, "the model would say this")]) assert merged == current def test_a_bound_entry_carries_its_note_rather_than_binding_blank(): - """Deviation 2 -- `bind_materials.merge_evidence` appends `""`; not here.""" + """The news stage passes a real note where the binder passes `""`.""" pair = MATCHES[0] plan = _plan_for_pair(pair, dict(SLOPPY_MEDIUM, confidence="high")) appended = plan.patch_items[-1] @@ -810,7 +811,7 @@ def test_the_write_sends_the_whole_merged_list_not_a_delta(): "https://jawafdehi.org/material/news/20230101.aaaa") api = FakeApi(case_payload(evidence=[existing])) plan = _bindable_plan() - plan.patch_items = en.merge_news_evidence( + plan.patch_items = en.merge_evidence( en.current_evidence(api.case), [(plan.bound_iris[0], "नोट")]) en.apply_plan(api, plan) items = api.replaced[0]["items"] @@ -1747,12 +1748,15 @@ def get(self, url, kind, headers=None, expect_html=False, assert "ABORTED at case 1/1" in out.err -def test_bind_materials_records_the_reciprocal_duplicate_contract(): - """Both stages PATCH the same destructive whole-list /evidence, so both - normalisers must stay identical. Only one side said so.""" - src = pathlib.Path(en.__file__).parent.joinpath("bind_materials.py").read_text(encoding="utf-8") - assert src.count("DELIBERATELY DUPLICATED") == 2 - assert "enrich_news_articles" in src +def test_both_evidence_writers_share_one_normaliser(): + """Two copies of a whole-list-replace normaliser can diverge and silently drop + evidence rows. Identity, not similarity, is what makes that impossible.""" + from casework.common import evidence as shared + + assert en.current_evidence is shared.current_evidence + assert en.merge_evidence is shared.merge_evidence + assert bind_materials.current_evidence is shared.current_evidence + assert bind_materials.merge_evidence is shared.merge_evidence # ---------------------------------------------------------------------------