From 252d0da329c47ef741980e2fbd920a3a8e36418e Mon Sep 17 00:00:00 2001 From: Jorge Rivera Date: Sun, 25 Oct 2026 19:06:11 +0100 Subject: [PATCH] fix: keep two Search entities written alike apart data-mcp's SearchEntity takes an optional name or dcid, never both, and sends them, so an agent can post "Congo" twice and settle an entity_ambiguous refusal by resending the row with the candidate's dcid. The digest lists each ambiguous entity group by its entity_choice index, with its words, role and candidate dcids. The harness keeps Search's entity_refs per restatement row: BoundEntity carries the row's identity (dcid, else name, else text), text, role and refs, and SelectedSource.bound_refs is a tuple of them. Evidence resolution matches each requirement entity to its own row, by identity then text, fitting the role first, so two Congos in one role column or in two both resolve. A saved plan with the old dict form fails validation and is planned again. The search capability's input hash is re-pinned for the new fields. Co-authored-by: Claude --- docs/upstream-asks.md | 31 +- services/data-mcp/README.md | 3 +- services/data-mcp/src/data_mcp/digest.py | 66 +++- services/data-mcp/src/data_mcp/search_tool.py | 44 ++- .../fixtures/resolve_aid_congo_to_congo.json | 328 ++++++++++++++++++ .../tests/test_digest_captured_envelope.py | 21 ++ services/data-mcp/tests/test_search_tool.py | 42 +++ services/harness/src/compiler/capabilities.py | 2 +- services/harness/src/compiler/evidence.py | 115 ++++-- services/harness/src/compiler/sources.py | 67 ++-- .../harness/src/harness/orchestrator_tools.py | 6 + .../harness/src/harness/tool_contracts.py | 17 +- services/harness/src/planner/workflow.py | 31 +- .../harness/tests/compiler/test_evidence.py | 105 +++++- .../harness/tests/compiler/test_sources.py | 96 ++++- .../tests/fixtures/revision_base_fields.json | 6 + .../tests/translator/test_recovery_actions.py | 9 +- 17 files changed, 896 insertions(+), 93 deletions(-) create mode 100644 services/data-mcp/tests/fixtures/resolve_aid_congo_to_congo.json diff --git a/docs/upstream-asks.md b/docs/upstream-asks.md index 59bf31d5..3bbfdaf5 100644 --- a/docs/upstream-asks.md +++ b/docs/upstream-asks.md @@ -120,18 +120,25 @@ names as an override. entity column; a wrong name costs one replan and one further approval, after which the turn closes naming the step that could not run. -## 10. A part refused for its denominator names neither side (per-head questions) +## 10. A part refused for its denominator does not say whose it is (per-head questions) Search answers "debt service per head" for Kenya `no_data` with reason `denominator_not_available`. The `no_data` block carries `reason`, -`category`, `message`, `elected_count`, `entity_terms`, -`ambiguous_entities` and `unbound_qualifiers`. It omits the -measure search read once the per-capita phrasing is set aside and the -denominator it looked for. - -**Ask:** on `denominator_not_available`, return the measure's variable -text without the normalising phrase, the denominator as a variable text -or StatVar, and which of the part's entities the denominator is about. -**Meanwhile:** the agent reads the `no_data` reason and has to work out -the measure and the denominator itself before it can search for them as -two parts. +`cause`, `category`, `message`, `nearest_real`, `gate`, `flagged_match`, +`alternate_match`, `elected_count`, `entity_terms`, `ambiguous_entities`, +`source_terms`, `ambiguous_sources`, `unit_alternatives`, +`unbound_qualifiers`, `denominator` and `options`. On a +`denominator_not_available` refusal after a series was elected, +`denominator.asked` names the denominator the question named and +`denominator.found` the one the flagged match divides by, and +`nearest_real` holds the level to divide. Each `ambiguous_entities` group +is settled by the options whose `entity_choice` is its index, so two +groups written alike ("Congo" and "Congo") are told apart. The block +still omits which of the part's entities the denominator is about. + +**Ask:** on `denominator_not_available`, say which of the part's +entities the denominator is about. +**Meanwhile:** data-mcp's digest shows the agent `nearest_real` but not +`denominator`, so the agent works out the denominator and the entities it +is about itself before it can search for the level and the denominator +as two parts. diff --git a/services/data-mcp/README.md b/services/data-mcp/README.md index b3011ec7..5d98c534 100644 --- a/services/data-mcp/README.md +++ b/services/data-mcp/README.md @@ -14,10 +14,11 @@ The `search` tool finds datasets and indicators matching a structured query. It #### Parameters -- `parts`: List of 1 to 6 `SearchPart` objects. Each part contains `variable` (string, 1 to 200 characters) and optional fields `entities` (list of `SearchEntity` objects with `text`, optional `role`, and optional `members`), `time` (string, 1 to 100 characters), `unit` (string, 1 to 100 characters), and `sources` (list of 1 to 8 `SearchSource` objects with `text`, 1 to 120 characters, and optional `exclude` and `ref` (1 to 200 characters)). +- `parts`: List of 1 to 6 `SearchPart` objects. Each part contains `variable` (string, 1 to 200 characters) and optional fields `entities` (list of `SearchEntity` objects with `text`, optional `role`, optional `members`, and an optional `name` or `dcid`), `time` (string, 1 to 100 characters), `unit` (string, 1 to 100 characters), and `sources` (list of 1 to 8 `SearchSource` objects with `text`, 1 to 120 characters, and optional `exclude` and `ref` (1 to 200 characters)). - `variable` and each entity's `text` take the caller's own words. `role` and `time` are matched against a fixed grammar. `role` names which side of a flow the entity sits on: a word for where something originates resolves to "from", a word for where it is directed resolves to "to". `time` takes a bare year, a range ("2015 to 2020"), an open start ("since 2015"), or "latest". A `role_text_not_recognised` or `time_text_not_parsed` warning means that field needs rephrasing in the grammar. - An entity whose `text` names a set of places (a continent, a subregion, or a group such as the European Union) takes `members: true` to ask for each member as its own series: "African countries" is `{"text": "Africa", "members": true}`. Without it the set reads as one place where the data holds a figure for it, and as its members where it holds none. The flag has no effect on a single place, and the wire request carries it only when true. - `sources` holds each publisher or dataset the user names for a part, in the user's words, with `exclude: true` for one the user asks to leave out: "aid to Kenya, not from CRS" is `{"text": "CRS", "exclude": true}`. Each reaches apis as a part's `sources` entry, with `exclude` as `polarity` (`include` or `exclude`). apis resolves the words against every source it holds, so the tool description lists none. A lender or recipient the flow runs to or from is an entity with its role, not a source. Words apis cannot resolve come back as `no_data` with reason `source_not_resolved`, and words that name several sources as `source_ambiguous`. Ask which candidate the user means, then retry the same source entry with its chosen `ref`, retaining `text` and `exclude`. +- An entity's `name` or `dcid` says which entity its `text` means, so two entities written alike stay two rows: "aid from Congo to Congo" sends `{"text": "Congo", "role": "from", "name": "Democratic Republic of the Congo"}` and `{"text": "Congo", "role": "to", "name": "Republic of the Congo"}`. A row carrying both is refused before the call, as apis refuses it with `request_invalid`. A `no_data` part with reason `entity_ambiguous` lists each ambiguous entity by number, with its role and each candidate's label, dcid, and kind; the agent asks the user which one is meant and resends that row with the candidate's `dcid`, retaining `text` and `role`. - `raw_query`: Optional string up to 2000 characters holding the original query text. A compound ask splits into one part per measured variable. diff --git a/services/data-mcp/src/data_mcp/digest.py b/services/data-mcp/src/data_mcp/digest.py index dbe2ea3e..da7ac387 100644 --- a/services/data-mcp/src/data_mcp/digest.py +++ b/services/data-mcp/src/data_mcp/digest.py @@ -322,6 +322,7 @@ def _render_no_data_part(part: Mapping[str, Any]) -> str: sections = [header] if message := no_data.get("message"): sections.append(message) + sections.extend(_ambiguous_entity_lines(no_data, part.get("sentence_plan") or [])) sections.extend(_ambiguous_source_lines(no_data.get("ambiguous_sources") or [])) checked = _checked_count(no_data) if checked: @@ -360,6 +361,67 @@ def _unapplied_phrase_header(unbound_qualifiers: Sequence[Mapping[str, Any]]) -> ) +def _ambiguous_entity_lines( + no_data: Mapping[str, Any], sentence_plan: Sequence[Mapping[str, Any]] +) -> list[str]: + """One line per `ambiguous_entities` group, naming each candidate's dcid, then how to settle it. + + Groups are numbered by their index, since two groups can share a wording + ("aid from Congo to Congo" asks "Congo" for each end). A group's role is + the `role_text` of the row its options pin to a candidate, else the role + of the sentence slot sharing its `entity_choice`. An option settles the + group its `entity_choice` names or, without one, every group its + mentions name. Candidates are capped at `_CANDIDATE_CAP`, with a trailing + count of the rest. + """ + groups = no_data.get("ambiguous_entities") or [] + if not groups: + return [] + options = no_data.get("options") or [] + lines = [] + for index, group in enumerate(groups): + text = group.get("text") + candidates = group.get("candidates") or [] + settling = [ + option + for option in options + if option.get("entity_choice") == index + or (option.get("entity_choice") is None and text in (option.get("mentions") or [])) + ] + role = _ambiguous_entity_role( + settling, refs={candidate.get("ref") for candidate in candidates} + ) or next( + ( + token.get("role") + for token in sentence_plan + if token.get("entity_choice") == index and token.get("role") + ), + None, + ) + shown = [_candidate_piece(candidate) for candidate in candidates[:_CANDIDATE_CAP]] + remaining = len(candidates) - len(shown) + tail = f", and {remaining} more" if remaining > 0 else "" + named = f'Entity {index + 1}, "{text}"' + (f" ({role})" if role else "") + lines.append(f"{named}, could mean: {'; '.join(shown)}{tail}.") + lines.append( + "Ask the user which one each entity means, then resend its row with the chosen " + "candidate's dcid as `dcid`, keeping its `text` and `role`." + ) + return lines + + +def _ambiguous_entity_role(options: Sequence[Mapping[str, Any]], *, refs: set[Any]) -> str | None: + """The one `role_text` on the rows `options` pin to a dcid in `refs`, or None when they state none or several.""" + roles = { + entity.get("role_text") + for option in options + for data_part in (option.get("data_query") or {}).get("parts") or [] + for entity in data_part.get("entities") or [] + if entity.get("dcid") in refs and entity.get("role_text") + } + return roles.pop() if len(roles) == 1 else None + + def _ambiguous_source_lines(groups: Sequence[Mapping[str, Any]]) -> list[str]: """One line per ambiguous source wording, naming each source it could mean. @@ -370,14 +432,14 @@ def _ambiguous_source_lines(groups: Sequence[Mapping[str, Any]]) -> list[str]: lines = [] for group in groups: candidates = group.get("candidates") or [] - shown = [_source_candidate(candidate) for candidate in candidates[:_CANDIDATE_CAP]] + shown = [_candidate_piece(candidate) for candidate in candidates[:_CANDIDATE_CAP]] remaining = len(candidates) - len(shown) tail = f", and {remaining} more" if remaining > 0 else "" lines.append(f'"{group.get("text")}" could mean: {"; ".join(shown)}{tail}.') return lines -def _source_candidate(candidate: Mapping[str, Any]) -> str: +def _candidate_piece(candidate: Mapping[str, Any]) -> str: return f"{candidate.get('label')} ({candidate.get('ref')}, {candidate.get('kind')})" diff --git a/services/data-mcp/src/data_mcp/search_tool.py b/services/data-mcp/src/data_mcp/search_tool.py index 96f7cc2d..d177a796 100644 --- a/services/data-mcp/src/data_mcp/search_tool.py +++ b/services/data-mcp/src/data_mcp/search_tool.py @@ -72,7 +72,12 @@ class SearchEntity(BaseModel): - """One entity mentioned in a part, with an optional relational phrase.""" + """One entity mentioned in a part, with an optional relational phrase. + + `name` or `dcid` says which entity `text` means, so two entities whose + words are alike ("aid from Congo to Congo") stay two rows. Search refuses + a row carrying both with `request_invalid`, so the model refuses it first. + """ model_config = ConfigDict(extra="forbid") @@ -96,6 +101,30 @@ class SearchEntity(BaseModel): "set as one place: 'Africa' is {text: 'Africa'}" ), ) + name: str | None = Field( + default=None, + min_length=1, + max_length=_MAX_ENTITY_TEXT_CHARS, + description=( + "the entity's full name, when `text` could name more than one entity: " + "'aid from Congo to Congo' sends {text: 'Congo', name: 'Republic of the " + "Congo'}. Leave unset beside `dcid`" + ), + ) + dcid: str | None = Field( + default=None, + min_length=1, + description=( + "the graph dcid of the entity `text` names, e.g. 'country/COG', as a " + "search refusal's candidate lists it. Leave unset beside `name`" + ), + ) + + @model_validator(mode="after") + def _name_or_dcid(self) -> Self: + if self.name is not None and self.dcid is not None: + raise ValueError("set name or dcid, not both: a dcid already names the entity") + return self class SearchOperation(BaseModel): @@ -330,6 +359,10 @@ def _wire_entity(entity: SearchEntity) -> dict[str, Any]: wire["role_text"] = entity.role if entity.members: wire["members"] = True + if entity.name is not None: + wire["name"] = entity.name + if entity.dcid is not None: + wire["dcid"] = entity.dcid return wire @@ -571,6 +604,15 @@ async def search( the code ambiguous (`source_code_ambiguous`), retry with a `sources` entry naming one of the sources it lists. + Two entities written alike stay two rows when each carries its `name` or + `dcid`: "aid from Congo to Congo" sends `{"text": "Congo", "role": "from", + "name": "Democratic Republic of the Congo"}` and `{"text": "Congo", "role": + "to", "name": "Republic of the Congo"}`. A row takes `name` or `dcid`, + never both. When Search refuses `entity_ambiguous`, the digest lists each + ambiguous entity by number with its candidates' dcids: ask the user which + one is meant, then resend that row with the candidate's `dcid`, keeping + its `text` and `role`. + A comparison of two categories of one measure ("health versus education aid") is one part per category, each with `separate: true`, and so is any other comparison sent as several parts. Search may otherwise merge diff --git a/services/data-mcp/tests/fixtures/resolve_aid_congo_to_congo.json b/services/data-mcp/tests/fixtures/resolve_aid_congo_to_congo.json new file mode 100644 index 00000000..e021dc1c --- /dev/null +++ b/services/data-mcp/tests/fixtures/resolve_aid_congo_to_congo.json @@ -0,0 +1,328 @@ +{ + "comment": "A real `/api/search/resolve` response from the deployed dev apis (engine build 99e62038), for the raw-text query \"aid from Congo to Congo\": it refused `entity_ambiguous` with two `ambiguous_entities` groups sharing the text \"Congo\", one per flow end, each offering Congo [DRC] and Congo [Republic]; each option names its group by `entity_choice`, as does each heading slot token. Unchanged apart from this comment.", + "schema_version": 1, + "query_echo": { + "entry_path": "raw_text", + "raw_query": "aid from Congo to Congo", + "normalized_query": "aid from Congo to Congo", + "extract_skipped": true + }, + "diagnostics": { + "engine_build": "99e62038ceb4b459c2ac699549076ff123c166aa", + "trace_id": "cf06d4ead57a494aa302b4126cd5a60b", + "warnings": [], + "timing_ms": { + "total": 1, + "by_step": { + "coverage": 0, + "response_cache_layer1": 1 + } + }, + "llm_usage": { + "input_tokens": 0, + "output_tokens": 0, + "cached_tokens": 0 + }, + "timed_out": false + }, + "kind": "resolution", + "parts": [ + { + "variable_text": "aid", + "warnings": [], + "timing_ms": { + "total": 133, + "by_step": { + "recall": 133, + "phase_resolve_places": 0, + "phase_check_entities": 0 + } + }, + "rendered_sentence": "\"Congo\" (from) and \"Congo\" (to) each match more than one entity: \"Congo [DRC]\" and \"Congo [Republic]\". Be more specific.", + "sentence_plan": [ + { + "kind": "text", + "text": "\"", + "segment": "heading", + "entity_refs": [], + "states": [] + }, + { + "kind": "slot", + "text": "Congo", + "segment": "heading", + "role": "origin", + "entity_choice": 0, + "entity_refs": [], + "states": [] + }, + { + "kind": "text", + "text": "\" (from) and \"", + "segment": "heading", + "entity_refs": [], + "states": [] + }, + { + "kind": "slot", + "text": "Congo", + "axis": "where", + "segment": "heading", + "role": "destination", + "entity_choice": 1, + "entity_refs": [], + "states": [] + }, + { + "kind": "text", + "text": "\" (to) each match more than one entity: \"Congo [DRC]\" and \"Congo [Republic]\". Be more specific.", + "segment": "heading", + "entity_refs": [], + "states": [] + } + ], + "restatement": { + "variable_text": "aid", + "entities": [ + { + "text": "Congo", + "name": "Democratic Republic of the Congo", + "role_text": "from" + }, + { + "text": "Congo", + "name": "Republic of the Congo", + "role_text": "to" + } + ], + "sources": [], + "operations": [] + }, + "entity_refs": [ + { + "text": "Congo", + "refs": [] + }, + { + "text": "Congo", + "refs": [] + } + ], + "operations": [], + "departures": [], + "assumptions": [], + "status": "no_data", + "no_data": { + "reason": "entity_ambiguous", + "category": "unresolved", + "message": "\"Congo\" (from) and \"Congo\" (to) each match more than one entity: \"Congo [DRC]\" and \"Congo [Republic]\". Be more specific.", + "entity_terms": [ + "Congo", + "Congo" + ], + "ambiguous_entities": [ + { + "text": "Congo", + "candidates": [ + { + "ref": "country/COD", + "label": "Congo [DRC]", + "kind": "country" + }, + { + "ref": "country/COG", + "label": "Congo [Republic]", + "kind": "country" + } + ] + }, + { + "text": "Congo", + "candidates": [ + { + "ref": "country/COD", + "label": "Congo [DRC]", + "kind": "country" + }, + { + "ref": "country/COG", + "label": "Congo [Republic]", + "kind": "country" + } + ] + } + ], + "source_terms": [], + "ambiguous_sources": [], + "unbound_qualifiers": [], + "options": [ + { + "mentions": [ + "Congo" + ], + "label": "Congo [DRC]", + "data_query": { + "kind": "parsed", + "parts": [ + { + "variable_text": "aid", + "entities": [ + { + "text": "Congo", + "role_text": "from", + "dcid": "country/COD" + }, + { + "text": "Congo", + "name": "Republic of the Congo", + "role_text": "to" + } + ], + "sources": [], + "operations": [] + } + ] + }, + "entity_choice": 0 + }, + { + "mentions": [ + "Congo" + ], + "label": "Congo [Republic]", + "data_query": { + "kind": "parsed", + "parts": [ + { + "variable_text": "aid", + "entities": [ + { + "text": "Congo", + "role_text": "from", + "dcid": "country/COG" + }, + { + "text": "Congo", + "name": "Republic of the Congo", + "role_text": "to" + } + ], + "sources": [], + "operations": [] + } + ] + }, + "entity_choice": 0 + }, + { + "mentions": [ + "Congo" + ], + "label": "Congo [DRC]", + "data_query": { + "kind": "parsed", + "parts": [ + { + "variable_text": "aid", + "entities": [ + { + "text": "Congo", + "name": "Democratic Republic of the Congo", + "role_text": "from" + }, + { + "text": "Congo", + "role_text": "to", + "dcid": "country/COD" + } + ], + "sources": [], + "operations": [] + } + ] + }, + "entity_choice": 1 + }, + { + "mentions": [ + "Congo" + ], + "label": "Congo [Republic]", + "data_query": { + "kind": "parsed", + "parts": [ + { + "variable_text": "aid", + "entities": [ + { + "text": "Congo", + "name": "Democratic Republic of the Congo", + "role_text": "from" + }, + { + "text": "Congo", + "role_text": "to", + "dcid": "country/COG" + } + ], + "sources": [], + "operations": [] + } + ] + }, + "entity_choice": 1 + } + ] + } + } + ], + "rendered_sentence": "\"Congo\" (from) and \"Congo\" (to) each match more than one entity: \"Congo [DRC]\" and \"Congo [Republic]\". Be more specific.", + "sentence_plan": [ + { + "kind": "text", + "text": "\"", + "segment": "heading", + "entity_refs": [], + "states": [], + "part_index": 0 + }, + { + "kind": "slot", + "text": "Congo", + "segment": "heading", + "role": "origin", + "entity_choice": 0, + "entity_refs": [], + "states": [], + "part_index": 0 + }, + { + "kind": "text", + "text": "\" (from) and \"", + "segment": "heading", + "entity_refs": [], + "states": [], + "part_index": 0 + }, + { + "kind": "slot", + "text": "Congo", + "axis": "where", + "segment": "heading", + "role": "destination", + "entity_choice": 1, + "entity_refs": [], + "states": [], + "part_index": 0 + }, + { + "kind": "text", + "text": "\" (to) each match more than one entity: \"Congo [DRC]\" and \"Congo [Republic]\". Be more specific.", + "segment": "heading", + "entity_refs": [], + "states": [], + "part_index": 0 + } + ], + "status": "no_data" +} diff --git a/services/data-mcp/tests/test_digest_captured_envelope.py b/services/data-mcp/tests/test_digest_captured_envelope.py index 25bbe63b..bc24557a 100644 --- a/services/data-mcp/tests/test_digest_captured_envelope.py +++ b/services/data-mcp/tests/test_digest_captured_envelope.py @@ -99,3 +99,24 @@ def test_captured_aid_envelope_names_the_elected_measure_and_its_sibling() -> No "donor and recipient country or region. Covers its measures, multilateral agencies " "reporting…" ) + + +def test_captured_congo_to_congo_refusal_lists_each_ambiguous_entity_by_number() -> None: + """`resolve_aid_congo_to_congo.json` is a real dev `entity_ambiguous` refusal with two + groups sharing the text "Congo" (its `comment` says how it was recorded). The digest + keeps the groups apart by number and role and names each candidate's dcid, so a model + can resend the right row with the dcid the user picks. + """ + envelope = _load_captured_envelope("resolve_aid_congo_to_congo.json") + + assert render_digest(envelope) == ( + "The ask could not be resolved (reason: entity_ambiguous).\n\n" + '"Congo" (from) and "Congo" (to) each match more than one entity: "Congo [DRC]" ' + 'and "Congo [Republic]". Be more specific.\n\n' + 'Entity 1, "Congo" (from), could mean: Congo [DRC] (country/COD, country); ' + "Congo [Republic] (country/COG, country).\n\n" + 'Entity 2, "Congo" (to), could mean: Congo [DRC] (country/COD, country); ' + "Congo [Republic] (country/COG, country).\n\n" + "Ask the user which one each entity means, then resend its row with the chosen " + "candidate's dcid as `dcid`, keeping its `text` and `role`." + ) diff --git a/services/data-mcp/tests/test_search_tool.py b/services/data-mcp/tests/test_search_tool.py index 697d3ad8..0e039f93 100644 --- a/services/data-mcp/tests/test_search_tool.py +++ b/services/data-mcp/tests/test_search_tool.py @@ -276,6 +276,48 @@ def test_to_wire_request_sends_separate_only_when_set(separate: bool | None, sen assert part.get("separate", False) is sent +@pytest.mark.parametrize( + ("identities", "expected"), + [ + ( + [{"name": "Democratic Republic of the Congo"}, {"name": "Republic of the Congo"}], + [ + {"text": "Congo", "role_text": "from", "name": "Democratic Republic of the Congo"}, + {"text": "Congo", "role_text": "to", "name": "Republic of the Congo"}, + ], + ), + ( + [{"dcid": "country/COD"}, {"dcid": "country/COG"}], + [ + {"text": "Congo", "role_text": "from", "dcid": "country/COD"}, + {"text": "Congo", "role_text": "to", "dcid": "country/COG"}, + ], + ), + ], + ids=["name", "dcid"], +) +def test_two_entities_written_alike_go_out_as_two_rows_with_their_identities( + identities: list[dict[str, str]], expected: list[dict[str, str]] +) -> None: + entities = [ + SearchEntity(text="Congo", role=role, **identity) + for role, identity in zip(("from", "to"), identities, strict=True) + ] + + wire = to_wire_request([SearchPart(variable="aid", entities=entities)], raw_query=None) + + assert wire["input"]["parts"][0]["entities"] == expected + + +def test_an_entity_with_both_a_name_and_a_dcid_is_refused() -> None: + with pytest.raises(ValidationError) as refused: + SearchEntity.model_validate( + {"text": "Congo", "name": "Republic of the Congo", "dcid": "country/COG"} + ) + + assert [error["type"] for error in refused.value.errors()] == ["value_error"] + + def test_an_entity_with_an_unknown_field_is_refused() -> None: with pytest.raises(ValidationError) as refused: SearchEntity.model_validate({"text": "Africa", "each_member": True}) diff --git a/services/harness/src/compiler/capabilities.py b/services/harness/src/compiler/capabilities.py index 47d16739..203cb367 100644 --- a/services/harness/src/compiler/capabilities.py +++ b/services/harness/src/compiler/capabilities.py @@ -144,7 +144,7 @@ def _analysis(name: str) -> ToolId: ToolOutput(extract=Extract.SPEC_IDS_FROM_ENVELOPE, artifact=ArtifactKind.SPEC_ID), ), implementation_version="1", - input_schema_hash="600b9684dbf35e24868084ff8dfa77c3b0fbfc4726df57d9ca573a594ec1fa7d", + input_schema_hash="36583e526d1531471a9ce1b8b768679da1396e0b9add58c9e5800242676c32d3", # Above data-mcp's `Settings.upstream_read_timeout_s` (50 s), itself above # the apis service's 45 s search deadline, so a slow search returns the apis service's own # 503 or 504 as a typed refusal before this call times out. diff --git a/services/harness/src/compiler/evidence.py b/services/harness/src/compiler/evidence.py index 1a9e95a4..35496d49 100644 --- a/services/harness/src/compiler/evidence.py +++ b/services/harness/src/compiler/evidence.py @@ -96,7 +96,13 @@ SpecId, StrictModel, ) -from planner.workflow import OpenEntityAxis, ResolvedEntity, ResolvedGroup, SelectedSource +from planner.workflow import ( + BoundEntity, + OpenEntityAxis, + ResolvedEntity, + ResolvedGroup, + SelectedSource, +) from pydantic import ConfigDict, Field, JsonValue, ValidationError from compiler.frame_columns import columns_on_axes @@ -157,7 +163,7 @@ class RetrievalEvidence(StrictModel): `groups` are the named sets the source binds (`SelectedSource.groups`), `entity_refs` the refs the graph resolved the requirement's unmatched names to (`SelectedSource.entity_refs`), and `bound_refs` the refs - Search bound for each entity text (`SelectedSource.bound_refs`), all discovery's alone: at run time + Search bound for each entity row (`SelectedSource.bound_refs`), all discovery's alone: at run time the approved source's are read instead, as its open axes are. `axis_by_column` is the manifest's alone: each coverage dimension's label @@ -189,7 +195,7 @@ class RetrievalEvidence(StrictModel): open_entity_axes: tuple[OpenEntityAxis, ...] | None = None groups: tuple[ResolvedGroup, ...] = () entity_refs: dict[str, str] = Field(default_factory=dict) - bound_refs: dict[str, tuple[str, ...]] = Field(default_factory=dict) + bound_refs: tuple[BoundEntity, ...] = () axis_by_column: dict[str, str | None] | None = None unit_ref: str | None = None unit_label: str | None = None @@ -397,27 +403,73 @@ def resolve_entity(entity: Entity, resolved: Sequence[ResolvedEntity]) -> Resolv return matches[0] if len(matches) == 1 else None +def bound_refs_of(entity: Entity, source: SelectedSource | RetrievalEvidence) -> tuple[str, ...]: + """The refs Search bound for the entity rows `entity` names (`SelectedSource.bound_refs`). + + `Entity.name` names the rows whose identity, a pinned dcid or a + stated name, it equals case-folded and whose `role_text` fits its + role: names it, sits on its side of the flow, or is unstated, as is a + role the requirement leaves unstated. Else it names the rows whose + text it equals and whose role fits. So of a row written "Congo" from + and one written "Congo" to and named "Republic of the Congo", a + recipient "Congo" names the second. When no row fits its role, it + names the rows of its identity, else of its text, so a row bound on + the other side is still reported against the role asked. Empty when + it names no row. + """ + wanted = entity.name.casefold() + by_identity = [row for row in source.bound_refs if row.identity.casefold() == wanted] + by_text = [row for row in source.bound_refs if row.text.casefold() == wanted] + + def fitting(rows: list[BoundEntity]) -> list[BoundEntity]: + return [row for row in rows if row.role is None or _role_fits(entity.role, row.role)] + + rows = fitting(by_identity) or fitting(by_text) or by_identity or by_text + return tuple(dict.fromkeys(ref for row in rows for ref in row.refs)) + + +def resolve_bound( + entity: Entity, source: SelectedSource | RetrievalEvidence +) -> ResolvedEntity | None: + """Return the one entity of `source` whose ref Search bound for the rows `entity` names + (`bound_refs_of`), or `None` when none or several do. + + A stated role tells apart a ref resolved under several role columns. + """ + return _one_by_ref(entity, source.entities, bound_refs_of(entity, source)) + + def resolve_name( entity: Entity, source: SelectedSource | RetrievalEvidence ) -> ResolvedEntity | None: - """Return the one entity of `source` that `entity` names, by label or else by ref. + """Return the one entity of `source` that `entity` names, by the row Search bound, by label, or else by ref. - A name no label matches (`resolve_entity`) is looked up in the refs - Search bound for it as an entity text (`SelectedSource.bound_refs`), - so "G7" names the published total labelled "Group of Seven", and else + The refs Search bound for the rows `entity` names answer first + (`resolve_bound`), so "G7" names the published total labelled "Group + of Seven", and a donor "Congo" the Congo its own row pins whatever + the other Congo is labelled. Else its name is matched by label + (`resolve_entity`), and a name Search bound no row for is looked up in the refs the graph resolved the requirement's names to (`SelectedSource.entity_refs`), so "United States" names the entity labelled "United States of America". A stated role tells apart a ref resolved under several role columns, as it does a label. """ - resolved = resolve_entity(entity, source.entities) - if resolved is not None: + bound = bound_refs_of(entity, source) + resolved = _one_by_ref(entity, source.entities, bound) or resolve_entity( + entity, source.entities + ) + if resolved is not None or bound: return resolved name = entity.name.casefold() - refs = source.bound_refs.get(name) or ( - (source.entity_refs[name],) if name in source.entity_refs else () - ) - matches = [one for one in source.entities if one.ref in refs] + refs = (source.entity_refs[name],) if name in source.entity_refs else () + return _one_by_ref(entity, source.entities, refs) + + +def _one_by_ref( + entity: Entity, resolved: Sequence[ResolvedEntity], refs: Sequence[str] +) -> ResolvedEntity | None: + """The one of `resolved` whose ref is among `refs`, told apart by `entity`'s role; `None` when none or several are.""" + matches = [one for one in resolved if one.ref in refs] if len(matches) > 1: matches = [one for one in matches if _role_fits(entity.role, one.role)] return matches[0] if len(matches) == 1 else None @@ -429,14 +481,14 @@ def resolve_group( """Return the one named set of `source` that `entity` names, or `None` when none or several do. Matches `Entity.name` against the set's `label`, case-folded, or names - the set whose ref or published total Search bound for it as an entity - text (`SelectedSource.bound_refs`), so "G7" names the set Search - labels "Group of Seven". A stated role has to name the set's role + the set whose ref or published total Search bound for the rows it + names (`bound_refs_of`), so "G7" names the set Search labels "Group + of Seven". A stated role has to name the set's role column or sit on its side of the flow (`donor` and `origin`), so "G7" asked for as a recipient is not the G7 whose members give. """ wanted = entity.name.casefold() - bound = source.bound_refs.get(wanted, ()) + bound = bound_refs_of(entity, source) matches = [ group for group in source.groups @@ -719,6 +771,19 @@ def _resolved_by_ref( return resolved +def _resolved_by_bound( + entity: Entity, evidence: RetrievalEvidence, approved: SelectedSource | None +) -> ResolvedEntity | None: + """The entity whose ref Search bound for the rows `entity` names (`resolve_bound`), or `None`. + + At discovery the source's own rows answer. A manifest states none, so + at run time the approved source's do. + """ + if evidence.origin == "discovery": + return resolve_bound(entity, evidence) + return resolve_bound(entity, approved) if approved is not None else None + + def _named_group( entity: Entity, evidence: RetrievalEvidence, approved: SelectedSource | None ) -> ResolvedGroup | None: @@ -926,10 +991,12 @@ def _named_entity_issues( A name is met by a named set the source binds (`_group_present`), as its members or, unless the entity asks for its members, as its - published total (`_group_reading_issues`); by a resolved entity's - label; or, through `resolve_name`, by a ref Search bound for the name - as an entity text or the ref the graph resolved the name to, at run - time as the approved source resolved it (`_resolved_by_ref`). + published total (`_group_reading_issues`); by the entity whose ref + Search bound for the rows it names (`_resolved_by_bound`), at run time + as the approved source resolved it, so two rows written "Congo" each + meet their own; by a resolved entity's label; or, through + `resolve_name`, by the ref the graph resolved the name to, at run time + as the approved source resolved it (`_resolved_by_ref`). A name the approved source resolves and the retrieval holds no series for, such as a donor whose reporting stopped before the requirement's @@ -954,6 +1021,12 @@ def _named_entity_issues( if group is not None and _group_present(entity, evidence, approved): issues.extend(_group_reading_issues(requirement, entity, group)) continue + if (bound := _resolved_by_bound(entity, evidence, approved)) is not None: + if evidence.origin == "discovery": + issues.extend(_role_issues(requirement, entity, bound)) + elif all(one.ref != bound.ref for one in evidence.entities): + missing.append(entity) + continue matches = entity_matches(entity, evidence.entities) if not matches: if (by_ref := _resolved_by_ref(entity, evidence, approved)) is not None: diff --git a/services/harness/src/compiler/sources.py b/services/harness/src/compiler/sources.py index a1ebcc53..5317eefa 100644 --- a/services/harness/src/compiler/sources.py +++ b/services/harness/src/compiler/sources.py @@ -33,6 +33,7 @@ StrictModel, ) from planner.workflow import ( + BoundEntity, ElectedMeasure, MeasureAlternative, OpenAxis, @@ -143,25 +144,40 @@ def source_constraint_matches( ) -def bound_refs(part: Mapping[str, object]) -> dict[str, tuple[str, ...]]: - """The refs Search bound for each entity text of `part`, by the text case-folded. - - Search states, beside the part's `restatement`, one `entity_refs` row - per entity text it restates: the text and the refs it finally bound. - "G7" binds `GroupOfSeven`, whatever label Search gives the set. A text - bound to nothing, being ambiguous, not held or not found, states no - refs and is left out, and so is every text of a part without - `entity_refs`, from a Search that predates the field. Such a name is - matched by label or through the graph's resolve door. A text stated - twice binds every ref either row states. +def bound_refs(part: Mapping[str, object]) -> tuple[BoundEntity, ...]: + """The refs Search bound for each entity row of `part`, one `BoundEntity` per row, in row order. + + Search states one `entity_refs` entry per `restatement.entities` row, + at the row's position: the row's text and the refs it finally bound. + "G7" binds `GroupOfSeven`, whatever label Search gives the set. Each + entry is kept with its row's identity (`dcid`, else `name`, else + `text`) and `role_text`, so two rows written "Congo" pinned to + `country/COD` and to `country/COG` stay two entities. A row bound to + nothing, being ambiguous, not held or not found, states no refs and is + left out, and so is every row of a part whose `entity_refs` does not + hold one entry per row, as a Search that predates the field or states + one entry per text answers. Such a name is matched by label or through + the graph's resolve door. """ - refs: dict[str, tuple[str, ...]] = {} - for row in _sequence(part.get("entity_refs")): - text, bound = _text(row.get("text")), _strings(row.get("refs")) - if text is not None and bound: - key = text.casefold() - refs[key] = tuple(dict.fromkeys((*refs.get(key, ()), *bound))) - return refs + rows = _sequence(_mapping(part.get("restatement")).get("entities")) + entries = _sequence(part.get("entity_refs")) + if len(entries) != len(rows): + return () + bound: list[BoundEntity] = [] + for row, entry in zip(rows, entries, strict=True): + text, refs = _text(row.get("text")), _strings(entry.get("refs")) + if text is None or not refs: + continue + identity = _text(row.get("dcid")) or _text(row.get("name")) or text + bound.append( + BoundEntity( + identity=identity, + text=text, + role=_text(row.get("role_text")), + refs=tuple(dict.fromkeys(refs)), + ) + ) + return tuple(bound) def outcome_from_envelope(envelope: Mapping[str, object]) -> DiscoveryOutcome: @@ -243,10 +259,14 @@ def source_from_spec_block(block: Mapping[str, object]) -> SelectedSource: `None`. `coverage.window` is a series window only when `coverage.kind` is `exact`; a `breadth` or `bare` coverage carries none. + The block's `_bound_refs`, the rows of the part that carried it + (`bound_refs`, written by `harness.tool_contracts.resolver_digest`), + are the source's `bound_refs`; a block without them binds none. Raises: - ValueError: If the block lacks a usable `spec_id`, or an entity it - resolves lacks its role, value or label. + ValueError: If the block lacks a usable `spec_id`, an entity it + resolves lacks its role, value or label, or its `_bound_refs` + are not a list of `BoundEntity` rows. """ resolved = _mapping(block.get("resolved")) coverage = _mapping(resolved.get("coverage")) @@ -271,11 +291,7 @@ def source_from_spec_block(block: Mapping[str, object]) -> SelectedSource: shape_member_digest=_text(shape.get("member_digest")), entities=tuple(_entity(entity) for entity in _sequence(resolved.get("entities"))), groups=_groups(_mapping(resolved.get("entity_groups"))), - bound_refs={ - text: tuple(refs) - for text, raw in _mapping(block.get("_bound_refs")).items() - if (refs := _strings(raw)) - }, + bound_refs=_BOUND_ENTITIES.validate_python(block.get("_bound_refs", ())), open_entity_axes=open_entity_axes_of(resolved), unit_ref=_text(unit.get("ref")), unit_label=_text(unit.get("label")), @@ -1032,6 +1048,7 @@ def _strings(value: object) -> list[str]: _PARTY_CLASSES: TypeAdapter[tuple[PartyClass, ...]] = TypeAdapter(tuple[PartyClass, ...]) +_BOUND_ENTITIES: TypeAdapter[tuple[BoundEntity, ...]] = TypeAdapter(tuple[BoundEntity, ...]) def _party_classes(value: object) -> tuple[PartyClass, ...]: diff --git a/services/harness/src/harness/orchestrator_tools.py b/services/harness/src/harness/orchestrator_tools.py index 661388bf..6e10c837 100644 --- a/services/harness/src/harness/orchestrator_tools.py +++ b/services/harness/src/harness/orchestrator_tools.py @@ -1677,6 +1677,12 @@ async def search( "members": true}`, and an explicitly named country stays an entity, independently of the ranking axis. Do not add "World" as a stand-in for an open population; retain it when the user asks for the World total. + Two entities written alike stay two rows when each carries its `name` + or `dcid`, never both: "aid from Congo to Congo" sends `{"text": + "Congo", "role": "from", "name": "Democratic Republic of the Congo"}` + and `{"text": "Congo", "role": "to", "name": "Republic of the Congo"}`, + and an `entity_ambiguous` row is resent with the chosen candidate's + `dcid`, keeping its `text` and `role`. A comparison of two categories of one measure ("health versus education aid") is one part per category, each with `separate: diff --git a/services/harness/src/harness/tool_contracts.py b/services/harness/src/harness/tool_contracts.py index 5d4bdb37..b3cf2cc0 100644 --- a/services/harness/src/harness/tool_contracts.py +++ b/services/harness/src/harness/tool_contracts.py @@ -484,10 +484,11 @@ def resolver_digest(envelope: dict[str, Any]) -> dict[str, Any]: candidate its digest offers keyed by its spec_id (`_offered_specs`). `harness.graph.state.store_search` stores them, so `fetch` and the planner's pins read a part's or a picked candidate's block with no - second `search` call. Each block a part carries holds the refs the - part's entity texts bound (`compiler.sources.bound_refs`) under - `_bound_refs`, which `compiler.sources.source_from_spec_block` reads - back, so a pin names an entity by the refs its text bound. The + second `search` call. Each block a part carries holds the refs each + of the part's entity rows bound (`compiler.sources.bound_refs`), one + `BoundEntity` per row in row order, under `_bound_refs`, which + `compiler.sources.source_from_spec_block` reads back, so a pin names + an entity by the refs its own row bound. The top-level `spec_block` holds the first part's block the same way. `kind` is what the envelope says of itself: a resolution, a catalogue, or a declined question, `resolution` when it states none. The skills it names for `get_skill` are @@ -517,9 +518,11 @@ def resolver_digest(envelope: dict[str, Any]) -> dict[str, Any]: def _with_bound_refs(part: Mapping[str, Any], spec: Any) -> Any: - """`spec`, a block `part` carries, with the part's `bound_refs` under `_bound_refs` when it has any.""" - refs = bound_refs(part) - return {**spec, "_bound_refs": refs} if refs and isinstance(spec, Mapping) else spec + """`spec`, a block `part` carries, with the part's `bound_refs` rows under `_bound_refs` when it has any.""" + rows = bound_refs(part) + if not rows or not isinstance(spec, Mapping): + return spec + return {**spec, "_bound_refs": [row.model_dump(mode="json") for row in rows]} def _offered_specs(part: Mapping[str, Any]) -> list[Mapping[str, Any]]: diff --git a/services/harness/src/planner/workflow.py b/services/harness/src/planner/workflow.py index 1c97028e..9b119341 100644 --- a/services/harness/src/planner/workflow.py +++ b/services/harness/src/planner/workflow.py @@ -112,6 +112,24 @@ def reads_total(self) -> bool: return self.total is not None or bool(self.whole_of) +class BoundEntity(StrictModel): + """One entity row of a search part's restatement and the refs Search bound for it. + + `identity` is the row's identity within the part, as Search keys it: + the `dcid` the row pins, else its `name`, else its `text`. `text` is + the row's words and `role` its `role_text`, `None` when it states + none. `refs` are the part's `entity_refs` entry at the row's position: + a place's node, a set's own node or published total, or a set's ref + followed by its members. Two rows written alike, "Congo" pinned to + `country/COD` and to `country/COG`, are two entities. + """ + + identity: str = Field(min_length=1) + text: str = Field(min_length=1) + role: str | None = None + refs: tuple[str, ...] = Field(min_length=1) + + class OpenAxis(StrictModel): """One constraint property a source's family of StatVars varies along. @@ -288,10 +306,11 @@ class SelectedSource(StrictModel): (`compiler.evidence.named_members`). Empty until that resolve runs, and for a source whose names all match by label. - `bound_refs` maps each entity text of the search part, case-folded, to - the refs Search states it bound (the part's `entity_refs`, - `compiler.sources.bound_refs`), so "G7" names the set Search labels - "Group of Seven" by its ref `GroupOfSeven` + `bound_refs` holds one `BoundEntity` for each entity row of the search + part that Search bound refs for, in row order (the part's + `entity_refs`, `compiler.sources.bound_refs`), so "G7" names the set + Search labels "Group of Seven" by its ref `GroupOfSeven`, and two rows + written "Congo" each name the Congo they pin (`compiler.evidence.named_members`). Empty for a part stating no `entity_refs`, and for a source read from a block no search part carried: a pin copied over another window @@ -356,8 +375,8 @@ class SelectedSource(StrictModel): ) # Left out of every dump while empty, so a source saved without it keeps # its serialised form. - bound_refs: Annotated[dict[str, tuple[str, ...]], HASHED_WHEN_SET] = Field( - default_factory=dict, exclude_if=lambda refs: not refs + bound_refs: Annotated[tuple[BoundEntity, ...], HASHED_WHEN_SET] = Field( + default=(), exclude_if=lambda refs: not refs ) # Left out of every dump while empty, so a source saved without it keeps # its serialised form. diff --git a/services/harness/tests/compiler/test_evidence.py b/services/harness/tests/compiler/test_evidence.py index 35909e12..466f6cec 100644 --- a/services/harness/tests/compiler/test_evidence.py +++ b/services/harness/tests/compiler/test_evidence.py @@ -22,10 +22,11 @@ open_axes_for, opens_place_axis, resolve_entity, + resolve_name, unlabelled_series_refs, with_series_labels, ) -from compiler.sources import member_digest, source_from_spec_block, stat_var_reading +from compiler.sources import bound_refs, member_digest, source_from_spec_block, stat_var_reading from data_mcp.manifest import Manifest, SeriesStatisticsModel from data_mcp.series_statistics import SERIES_STATISTICS_ARROW_SCHEMA, series_statistics_table from planner.plan import Coverage, DatasetRequirement, Entity, Unit, open_window @@ -3260,6 +3261,108 @@ def test_a_retrieval_meets_a_member_by_the_ref_its_name_resolved_to_at_approval( ] == missing +# "Aid from Congo to Congo": Search restates two rows written "Congo", each +# pinned or named apart, and binds each its own country. The data labels the +# Republic of the Congo "Congo". +_COD = ResolvedEntity(role="origin", ref="country/COD", label="Congo, Dem. Rep.") +_COG = ResolvedEntity(role="destination", ref="country/COG", label="Congo") + + +def _congo_source( + source: SourceFactory, rows: list[dict[str, str]], entities: tuple[ResolvedEntity, ...] +) -> SelectedSource: + part = { + "restatement": {"entities": rows}, + "entity_refs": [{"text": "Congo", "refs": [entity.ref]} for entity in entities], + } + return source(entities=entities, bound_refs=bound_refs(part), identity=None) + + +_PINNED_FROM_AND_TO = [ + {"text": "Congo", "dcid": "country/COD", "role_text": "from"}, + {"text": "Congo", "dcid": "country/COG", "role_text": "to"}, +] +_PINNED_BOTH_FROM = [ + {"text": "Congo", "dcid": "country/COD", "role_text": "from"}, + {"text": "Congo", "dcid": "country/COG", "role_text": "from"}, +] +_NAMED_BOTH_FROM = [ + {"text": "Congo", "name": "Democratic Republic of the Congo", "role_text": "from"}, + {"text": "Congo", "name": "Republic of the Congo", "role_text": "from"}, +] + + +@pytest.mark.parametrize( + ("rows", "entities", "asked"), + [ + ( + _PINNED_FROM_AND_TO, + (_COD, _COG), + [Entity(name="Congo", role="donor"), Entity(name="Congo", role="recipient")], + ), + ( + _PINNED_BOTH_FROM, + (_COD, _COG.model_copy(update={"role": "origin"})), + [Entity(name="country/COD", role="donor"), Entity(name="country/COG", role="donor")], + ), + ( + _NAMED_BOTH_FROM, + (_COD, _COG.model_copy(update={"role": "origin"})), + [ + Entity(name="Democratic Republic of the Congo", role="donor"), + Entity(name="Republic of the Congo", role="donor"), + ], + ), + # The unnamed row's identity is its text, which a recipient "Congo" + # equals too, but only the named row is bound on the recipient side. + ( + [ + {"text": "Congo", "role_text": "from"}, + {"text": "Congo", "name": "Republic of the Congo", "role_text": "to"}, + ], + (_COD, _COG), + [Entity(name="Congo", role="donor"), Entity(name="Congo", role="recipient")], + ), + ], + ids=["pinned-other-columns", "pinned-one-column", "named-one-column", "named-beside-unnamed"], +) +def test_two_entities_written_alike_each_resolve_to_their_own_row( + requirement: RequirementFactory, + source: SourceFactory, + rows: list[dict[str, str]], + entities: tuple[ResolvedEntity, ...], + asked: list[Entity], +) -> None: + found = _congo_source(source, rows, entities) + + resolved = [resolve_name(entity, found) for entity in asked] + issues = check_evidence(requirement(entities=asked), evidence_from_source(found)) + + assert resolved == list(entities) + assert _entity_codes(issues) == [] + + +@pytest.mark.parametrize( + ("refs", "missing"), + [(("country/COD", "country/COG"), 0), (("country/COD",), 1)], + ids=["holds-both", "holds-the-donor"], +) +def test_a_retrieval_meets_each_entity_written_alike_by_its_own_rows_ref( + requirement: RequirementFactory, + source: SourceFactory, + refs: tuple[str, ...], + missing: int, +) -> None: + # Holding the donor's Congo alone leaves out the recipient's, and only it. + approved = _congo_source(source, _PINNED_FROM_AND_TO, (_COD, _COG)) + asked = [Entity(name="Congo", role="donor"), Entity(name="Congo", role="recipient")] + + issues = check_evidence(requirement(entities=asked), _g7_retrieval(*refs), approved=approved) + + codes = [issue.code for issue in errors(issues)] + assert codes.count(EvidenceIssueCode.ENTITY_MISSING) == missing + + @pytest.mark.parametrize("exclude", [False, True]) def test_search_confirmed_source_wording_is_authoritative(requirement, source, exclude): provenance = "provenance/OECDDAC2A" diff --git a/services/harness/tests/compiler/test_sources.py b/services/harness/tests/compiler/test_sources.py index e1251aba..9be99f7c 100644 --- a/services/harness/tests/compiler/test_sources.py +++ b/services/harness/tests/compiler/test_sources.py @@ -22,7 +22,7 @@ wire_groups, ) from planner.records import Period -from planner.workflow import OpenEntityAxis +from planner.workflow import BoundEntity, OpenEntityAxis SPEC_KENYA = "spec_" + "a" * 32 SPEC_OTHER = "spec_" + "b" * 32 @@ -79,38 +79,92 @@ def _envelope(status: str, **payload: Any) -> dict[str, Any]: return {"schema_version": 1, "status": status, "parts": [_part(status, **payload)]} +def _rows(*rows: dict[str, Any]) -> dict[str, Any]: + """The restatement entity rows of a part.""" + return {"entities": list(rows)} + + +_CONGO_PART = { + "restatement": _rows( + {"text": "Congo", "dcid": "country/COD", "role_text": "from"}, + {"text": "Congo", "dcid": "country/COG", "role_text": "to"}, + ), + "entity_refs": [ + {"text": "Congo", "refs": ["country/COD"]}, + {"text": "Congo", "refs": ["country/COG"]}, + ], +} + + @pytest.mark.parametrize( ("part", "expected"), [ ( { + "restatement": _rows( + {"text": "G7", "role_text": "from"}, {"text": "Africa", "role_text": "to"} + ), "entity_refs": [ {"text": "G7", "refs": ["GroupOfSeven"]}, {"text": "Africa", "refs": ["DACAfrica"]}, - ] + ], }, - {"g7": ("GroupOfSeven",), "africa": ("DACAfrica",)}, + [ + BoundEntity(identity="G7", text="G7", role="from", refs=("GroupOfSeven",)), + BoundEntity(identity="Africa", text="Africa", role="to", refs=("DACAfrica",)), + ], + ), + # A row Search bound nothing for names nothing by ref. + ( + { + "restatement": _rows({"text": "Atlantis"}), + "entity_refs": [{"text": "Atlantis", "refs": []}], + }, + [], ), - # A text Search bound nothing for names nothing by ref. - ({"entity_refs": [{"text": "Atlantis", "refs": []}]}, {}), # A Search that predates `entity_refs` states none, whatever it restates. - ({"restatement": {"entities": [{"text": "G7", "role_text": "from"}]}}, {}), - # A text stated twice binds every ref either row states. + ({"restatement": _rows({"text": "G7", "role_text": "from"})}, []), + # Nor does one stating an entry per text rather than per row. ( { + "restatement": _rows({"text": "Congo"}, {"text": "Congo"}), + "entity_refs": [{"text": "Congo", "refs": ["country/COD", "country/COG"]}], + }, + [], + ), + # Two rows written alike stay two entities, each keyed by what it pins or names. + ( + _CONGO_PART, + [ + BoundEntity( + identity="country/COD", text="Congo", role="from", refs=("country/COD",) + ), + BoundEntity(identity="country/COG", text="Congo", role="to", refs=("country/COG",)), + ], + ), + ( + { + "restatement": _rows( + {"text": "Congo", "name": "Republic of the Congo"}, + {"text": "Congo"}, + ), "entity_refs": [ - {"text": "Kenya", "refs": ["country/KEN"]}, - {"text": "kenya", "refs": ["KEN2"]}, - ] + {"text": "Congo", "refs": ["country/COG"]}, + {"text": "Congo", "refs": ["country/COD"]}, + ], }, - {"kenya": ("country/KEN", "KEN2")}, + [ + BoundEntity(identity="Republic of the Congo", text="Congo", refs=("country/COG",)), + BoundEntity(identity="Congo", text="Congo", refs=("country/COD",)), + ], ), ], + ids=["texts", "unbound", "no-entity-refs", "per-text", "pinned-alike", "named-alike"], ) -def test_each_entity_text_maps_to_the_refs_it_bound( - part: dict[str, Any], expected: dict[str, tuple[str, ...]] +def test_each_entity_row_keeps_the_refs_it_bound( + part: dict[str, Any], expected: list[BoundEntity] ) -> None: - assert bound_refs(part) == expected + assert list(bound_refs(part)) == expected def test_a_definite_answer_yields_the_one_source_it_resolved() -> None: @@ -764,6 +818,20 @@ def test_a_resubmit_posts_a_whole_total_without_its_marker() -> None: assert wire_groups(source.groups) == {"origin": {"label": "all donors"}} +def test_a_block_carries_its_parts_bound_rows_back_into_the_source() -> None: + rows = bound_refs(_CONGO_PART) + block = {**_spec(), "_bound_refs": [row.model_dump(mode="json") for row in rows]} + + assert source_from_spec_block(block).bound_refs == rows + + +def test_a_block_holding_bound_refs_by_text_is_not_usable() -> None: + block = {**_spec(), "_bound_refs": {"congo": ["country/COD", "country/COG"]}} + + with pytest.raises(ValueError, match="not usable as a source"): + source_from_spec_block(block) + + def test_a_source_saved_before_entity_refs_dumps_as_it_did() -> None: source = source_from_spec_block(_spec()) diff --git a/services/harness/tests/fixtures/revision_base_fields.json b/services/harness/tests/fixtures/revision_base_fields.json index ad912982..e336b823 100644 --- a/services/harness/tests/fixtures/revision_base_fields.json +++ b/services/harness/tests/fixtures/revision_base_fields.json @@ -317,6 +317,12 @@ "from_step", "output" ], + "planner.workflow.BoundEntity": [ + "identity", + "text", + "role", + "refs" + ], "planner.workflow.ElectedMeasure": [ "label", "alternatives" diff --git a/services/harness/tests/translator/test_recovery_actions.py b/services/harness/tests/translator/test_recovery_actions.py index 2376d3e1..37b0f739 100644 --- a/services/harness/tests/translator/test_recovery_actions.py +++ b/services/harness/tests/translator/test_recovery_actions.py @@ -674,7 +674,9 @@ def oda(**overrides: Any) -> DatasetRequirement: {"role": "destination", "label": "Kenya", "ref": "country/KEN"}, ], ), - "_bound_refs": {"united states": ["country/USA"]}, + "_bound_refs": [ + {"identity": "United States", "text": "United States", "refs": ["country/USA"]} + ], } _G7_TO_KENYA = _spec( "spec_9c7a11e2" + "0" * 24, @@ -699,7 +701,10 @@ def oda(**overrides: Any) -> DatasetRequirement: {"role": "destination", "label": "Kenya", "ref": "country/KEN"}, ], ), - "_bound_refs": {"united states": ["country/USA"], "united kingdom": ["country/GBR"]}, + "_bound_refs": [ + {"identity": "United States", "text": "United States", "refs": ["country/USA"]}, + {"identity": "United Kingdom", "text": "United Kingdom", "refs": ["country/GBR"]}, + ], } _KEEP_G7 = "name 'G7' as donor in `entities` and drop 'donor' from `across`"