diff --git a/specs/011-summarise-analysis-results/contracts/summary_endpoint.md b/specs/011-summarise-analysis-results/contracts/summary_endpoint.md index 9b196de..dc162bb 100644 --- a/specs/011-summarise-analysis-results/contracts/summary_endpoint.md +++ b/specs/011-summarise-analysis-results/contracts/summary_endpoint.md @@ -81,7 +81,8 @@ comparable time, and a caller must be able to ignore it safely. ``` event: start -data: {"release": 97, "analysis_type": "OVERREPRESENTATION", "cached": false} +data: {"release": 97, "analysis_type": "OVERREPRESENTATION", "cached": false, + "disclosure": "aggregate"} event: token data: {"text": "Of the 312 pathways hit, four remain significant after "} @@ -103,6 +104,21 @@ never an error code, so the analysis page cannot be broken by this service. pattern it could not write correctly. This endpoint's prompt is its own, so the right fix here is not to ask for one in the first place. +**`disclosure` on `start` is the tier the summary was actually built from**, +which is not always the one requested. If `identifiers` was asked for and the +unmatched identifiers could not be retrieved, the summary is the aggregate one +and this says `aggregate`. + +Without it the reader chooses to disclose, the lookup fails, and they are +handed the other summary with nothing to distinguish it -- disclosure with no +benefit, and no way for the interface to say so. A caller should surface the +difference rather than silently presenting an aggregate summary as the +disclosing one. + +Nothing to disclose is **not** a failed disclosure: a result where every +identifier matched reports `identifiers`, because the tier was honoured and +there was simply nothing to retrieve. + `cached` on `start` says whether this text was generated now or reused. It exists because the interface must not imply determinism it does not have: a reader who regenerates may get different wording, and `cached: false` is when that happens. diff --git a/src/api/analysis_summary.py b/src/api/analysis_summary.py index 2fde18f..1e17576 100644 --- a/src/api/analysis_summary.py +++ b/src/api/analysis_summary.py @@ -141,6 +141,9 @@ async def stream() -> AsyncIterator[str]: payload = for_tier(fetched.result, body.disclosure) model_input = prompt_input(payload) + # Which tier the answer was actually built from, which is + # not always the one asked for. + applied: Tier = body.disclosure if body.disclosure == "identifiers": # The only place this service asks for the reader's own # identifiers, reached only because they chose it. A @@ -149,6 +152,20 @@ async def stream() -> AsyncIterator[str]: unmatched = await fetch_not_found(body.token) if unmatched: model_input["identifiers_not_found_names"] = unmatched + elif model_input.get("identifiers_not_found"): + # There were unmatched identifiers and we could not + # retrieve them. The summary is therefore the + # aggregate one, and saying so is the whole point: + # this is the same "disclosure with no benefit" the + # tier was just fixed for, arriving down the failure + # path instead. A reader who chose to disclose and + # silently got the other summary has been told + # nothing and given nothing. + applied = "aggregate" + logger.warning( + "identifier tier requested but the not-found " + "lookup returned nothing; serving aggregate" + ) release = await current_release() yield _sse( "start", @@ -159,6 +176,10 @@ async def stream() -> AsyncIterator[str]: # Reported rather than omitted, because the interface # must not imply a determinism this does not have. "cached": False, + # What the summary was built from. Equal to the + # request's `disclosure` except when the disclosing + # tier could not be honoured. + "disclosure": applied, }, ) diff --git a/tests/api/test_analysis_summary.py b/tests/api/test_analysis_summary.py index 9b81306..412e306 100644 --- a/tests/api/test_analysis_summary.py +++ b/tests/api/test_analysis_summary.py @@ -383,3 +383,51 @@ async def astream(self, messages: Any) -> AsyncIterator[Any]: _post(public, caller_token=_token(private), disclosure=tier) told = "Name them" in json.dumps(sent, default=str) assert told is expected, f"{tier}: instruction to name identifiers {told}" + + +def test_a_disclosure_that_could_not_be_honoured_is_reported( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # The failure path of the bug this phase fixed. The reader chose to + # disclose, the lookup failed, and they got the aggregate summary with + # nothing saying the disclosure had not happened -- disclosure with no + # benefit again, arriving down the error path instead of the prompt. + async def _fails(_token: str, **_kwargs: Any) -> None: + return None + + monkeypatch.setattr("api.analysis_summary.fetch_not_found", _fails) + private, public = keys + start = _events( + _post(public, caller_token=_token(private), disclosure="identifiers").text + )[0][1] + assert start["disclosure"] == "aggregate", "the caller was not told" + + +def test_the_applied_disclosure_matches_the_request_when_it_is_honoured( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + _with_not_found_spy(monkeypatch) + private, public = keys + for tier in ("aggregate", "identifiers"): + start = _events( + _post(public, caller_token=_token(private), disclosure=tier).text + )[0][1] + assert start["disclosure"] == tier + + +def test_nothing_to_disclose_is_not_a_failed_disclosure( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # A result where every identifier matched has nothing to retrieve. That + # is the disclosing tier honoured, not denied -- reporting it as a + # downgrade would tell the reader something untrue. + async def _empty(_token: str, **_kwargs: Any) -> list[str]: + return [] + + monkeypatch.setattr("api.analysis_summary.fetch_not_found", _empty) + monkeypatch.setitem(RESULT, "identifiersNotFound", 0) + private, public = keys + start = _events( + _post(public, caller_token=_token(private), disclosure="identifiers").text + )[0][1] + assert start["disclosure"] == "identifiers"