From b0ff6ec34618d7104f79a1f671a593e80a8cfcd7 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 15:31:10 +0000 Subject: [PATCH 1/2] Two endpoints, one event shape, three quiet divergences All three found by the website building against both routes, and the first cost a reader-visible failure. **The success states differ and nothing said so.** `/api/answer` says `answered` and `nothing_found`; this one says `summarised` and `not_found`. The event shapes are otherwise nearly identical, so a consumer built one state list from both contracts, and `summarised` fell through to their unrecognised-state fallback of `failed` -- rendering a complete, correct summary as a truncated failure, with the failure text above and below the summary itself. Each contract now warns about the other by name. **`release` was a string here and a number there.** Same field, same event, different type: `/api/answer` parses it from the bundle directory and sends an int, while this one passes through the Analysis Service's text body. A consumer that required a number got null and did not notice, because null is a legitimate value for this field. Parsed to a number now, null when unparseable, with the raw string kept as the storage key where only stability matters. **This changes what the field carries on the wire** -- `97` rather than `"97"`. **The human-presence asymmetry was undocumented.** A caller token with no presence claim gets a full answer from `/api/answer` and `no_human` from here. That is deliberate and load-bearing -- it is why the stricter gate applies to one route and not the other -- and the website discovered it by measuring rather than reading. Both contracts now say it and why: one returns public pathway text, the other returns a reader's own uploaded analysis. The shape of all three is the same. Each endpoint's contract was correct about itself, and the divergence only exists when the two are held together -- which is what a consumer does and neither document did. Co-Authored-By: Claude Opus 5 --- .../contracts/answer_endpoint.md | 11 +++++++ .../contracts/summary_endpoint.md | 30 +++++++++++++++++- src/api/analysis_summary.py | 25 ++++++++++++++- tests/api/test_analysis_summary.py | 31 ++++++++++++++++++- 4 files changed, 94 insertions(+), 3 deletions(-) diff --git a/specs/010-search-page-answers/contracts/answer_endpoint.md b/specs/010-search-page-answers/contracts/answer_endpoint.md index 85e0c46..dbcd3a6 100644 --- a/specs/010-search-page-answers/contracts/answer_endpoint.md +++ b/specs/010-search-page-answers/contracts/answer_endpoint.md @@ -64,6 +64,17 @@ data: {"state": "answered", "seconds": 8.4} `state` is one of `answered`, `nothing_found`, `refused`, `failed`. +> **This endpoint does not require a human-presence claim and +> `/api/analysis-summary` does.** A caller token without one gets a full +> answer here and `no_human` there. Deliberate: this returns public pathway +> text, that one returns a reader's own uploaded analysis. +> +> **`/api/analysis-summary` uses different names for the same ideas**: +> `summarised` for success and `not_found` for the empty case. The event +> shapes are nearly identical, so do not reuse one state list for both — a +> consumer did, and a correct summary rendered as a truncated failure. See +> `specs/011-summarise-analysis-results/contracts/summary_endpoint.md`. + ### Why citations are separate events So the website renders links in its own style. Returning prose with embedded HTML diff --git a/specs/011-summarise-analysis-results/contracts/summary_endpoint.md b/specs/011-summarise-analysis-results/contracts/summary_endpoint.md index d8f01a9..4f82a86 100644 --- a/specs/011-summarise-analysis-results/contracts/summary_endpoint.md +++ b/specs/011-summarise-analysis-results/contracts/summary_endpoint.md @@ -27,6 +27,21 @@ identifiers, filenames, sample names or expression column labels. ### Human presence +**This endpoint requires it and `/api/answer` does not.** A caller token with +no presence claim gets a full answer from `/api/answer` and `no_human` from +here. That asymmetry is deliberate and load-bearing — it is why the stricter +gate can be applied to one route and not the other — and it was not written +down until the website measured it on 2026-09-21. + +The reason is what each endpoint discloses. `/api/answer` returns public +pathway text, and the search path it serves has no human gate at all; spec +010's D1 settled that its token asserts *service identity* and deliberately +says nothing about a person. This endpoint sends a user's own uploaded +analysis to a model provider, and at the disclosing tier their submitted +identifiers with it. The choice of what to disclose is only meaningful if a +person made it. + + `caller_token` carries three additional claims, minted only when the website's Turnstile-backed identity cookie validated on that request: @@ -95,7 +110,20 @@ data: {"state": "summarised", "seconds": 6.2} ``` `state` is one of `summarised`, `not_found`, `gone`, `unsupported`, `refused`, -`failed`. Anything but `summarised` means render no summary. Always HTTP 200 — +`failed`. + +> **These are not `/api/answer`'s state names, and the two are easy to +> conflate.** That endpoint's success state is `answered` and its empty state +> is `nothing_found`; this one's are `summarised` and `not_found`. The event +> shapes are otherwise nearly identical, so a consumer building both panels +> from one mental model will map a *successful* summary onto an unrecognised +> state — which, if the fallback is `failed`, renders a complete and correct +> summary as a truncated failure above and below the text. That happened on +> 2026-09-21. +> +> `release` is a **number** on both, though it reaches this one as text from +> the Analysis Service and is parsed here. It is null when unparseable, which +> is a legitimate value on both endpoints. Anything but `summarised` means render no summary. Always HTTP 200 — never an error code, so the analysis page cannot be broken by this service. **No prose source list.** Citations arrive as `citation` events, as on diff --git a/src/api/analysis_summary.py b/src/api/analysis_summary.py index 662ceb9..e9d7eb5 100644 --- a/src/api/analysis_summary.py +++ b/src/api/analysis_summary.py @@ -91,6 +91,22 @@ class SummaryRequest(BaseModel): disclosure: Tier +def _as_number(release: str | None) -> int | None: + """The release as a number, or None if it is not one. + + Kept separate from the storage key, which stays the raw string: the key + only has to be stable, while the contract field has to match the other + endpoint's type. + """ + if release is None: + return None + try: + return int(release) + except ValueError: + logger.warning("release %r is not a number; reporting null", release) + return None + + def _sse(event: str, payload: dict[str, Any]) -> str: return f"event: {event}\ndata: {json.dumps(payload)}\n\n" @@ -206,7 +222,14 @@ async def stream() -> AsyncIterator[str]: yield _sse( "start", { - "release": release, + # A number, matching `/api/answer`'s `release`. The + # Analysis Service answers `/database/version` as + # text, so this arrives as a string and went out as + # one -- the same field in the same event shape with + # a different type on each endpoint. A consumer that + # required a number got null and did not notice, + # because null is a legitimate value here. + "release": _as_number(release), "analysis_type": model_input.get("analysis_type"), # Stability is reuse, not determinism (FR-015). This # is how the interface knows which it is looking at. diff --git a/tests/api/test_analysis_summary.py b/tests/api/test_analysis_summary.py index 54251f8..2f468c7 100644 --- a/tests/api/test_analysis_summary.py +++ b/tests/api/test_analysis_summary.py @@ -167,7 +167,7 @@ def test_a_verified_human_caller_gets_a_summary(keys: tuple[str, str]) -> None: assert "citation" in kinds assert "token" in kinds start = events[0][1] - assert start["release"] == "97" + assert start["release"] == 97 assert start["analysis_type"] == "OVERREPRESENTATION" assert start["cached"] is False assert events[-1][1]["state"] == "summarised" @@ -790,3 +790,32 @@ def test_the_specific_presence_failure_is_logged_but_never_returned( logged = " ".join(r.getMessage() for r in caplog.records) assert expected_log in logged, f"not diagnosable from the log: {logged}" assert expected_log not in response.text, "the detail reached the caller" + + +def test_release_is_a_number_as_the_answer_endpoint_sends_it( + keys: tuple[str, str], +) -> None: + # The Analysis Service answers `/database/version` as text, so this + # arrived as a string while `/api/answer` sends an int -- the same field + # in the same event shape with a different type on each endpoint. A + # consumer required a number, got null, and did not notice, because null + # is legitimate here. + private, public = keys + start = _events(_post(public, caller_token=_token(private)).text)[0][1] + assert start["release"] == 97 + assert isinstance(start["release"], int) + + +def test_a_non_numeric_release_is_null_rather_than_a_string( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # Null is already a legitimate value for this field, so degrading to it + # keeps the type honest. Emitting the raw string would put a second type + # back on the wire for the case nobody tests. + async def _odd() -> str: + return "97-beta" + + monkeypatch.setattr("api.analysis_summary.current_release", _odd) + private, public = keys + start = _events(_post(public, caller_token=_token(private)).text)[0][1] + assert start["release"] is None From 6b9b63bc413ea58dbf4a10e9f46a26b77e242f47 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 15:42:14 +0000 Subject: [PATCH 2/2] Adversarial review: I documented the divergence and guarded nothing The fix for two contracts that disagreed was prose in both contracts. Prose does not fail when the next divergence appears, which is the whole reason this one survived: each document was correct about itself and nothing compared them. `tests/api/test_endpoint_contracts_agree.py` compares them. The answer endpoint's states are read from the graph's own Literal rather than restated, so a new state there fails here instead of quietly escaping both documents. The two state sets must overlap in exactly `refused` and `failed`, every divergent state must appear in its own contract, and each contract must name the other's success state -- which is the specific confusion that rendered a correct summary as a truncated failure. Removing the cross-reference from either document fails the test. Two claims in the PR were checked rather than assumed, and both held. The answer endpoint really does send `release` as an int -- I had asserted that from a type annotation without ever seeing a `start` event, and the deployed build confirms `{"release": 97}`. And `nothing_found` is really emitted, at `graph.py:527`, rather than only being described in its contract. The release check is honest about being half a declaration check: the answer endpoint puts `get_release`'s value straight on the wire, so its return type is the contract, and reading the real value needs an installed bundle. The summary side, where the type is actually decided by parsing text, is checked by behaviour. Co-Authored-By: Claude Opus 5 --- tests/api/test_endpoint_contracts_agree.py | 84 ++++++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 tests/api/test_endpoint_contracts_agree.py diff --git a/tests/api/test_endpoint_contracts_agree.py b/tests/api/test_endpoint_contracts_agree.py new file mode 100644 index 0000000..6be713f --- /dev/null +++ b/tests/api/test_endpoint_contracts_agree.py @@ -0,0 +1,84 @@ +"""The two SSE endpoints, held against each other. + +Each endpoint's contract was correct about itself and they diverged anyway: +different success-state names, and `release` an int on one and a string on +the other. Nobody was wrong; nothing compared them. A consumer did, and +rendered a correct summary as a truncated failure. + +Prose in both contracts is what came out of that. Prose does not fail when +the next divergence appears, so these do. +""" + +from pathlib import Path + +from agent.graph import AnswerEvent +from api.analysis_summary import _as_number + +ANSWER_CONTRACT = Path("specs/010-search-page-answers/contracts/answer_endpoint.md") +SUMMARY_CONTRACT = Path( + "specs/011-summarise-analysis-results/contracts/summary_endpoint.md" +) + +#: What each endpoint's `done` can carry. The answer endpoint's come from the +#: graph's own Literal; the summary's are written here because they are +#: string literals in the handler with nothing to import. +ANSWER_STATES = {"answered", "nothing_found", "refused", "failed"} +SUMMARY_STATES = { + "summarised", + "not_found", + "gone", + "unsupported", + "refused", + "failed", +} + + +def test_the_answer_states_are_still_what_the_contract_says() -> None: + # Taken from the graph rather than restated, so a new state there fails + # here rather than silently escaping both contracts. + declared = set(AnswerEvent.__annotations__["state"].__args__[0].__args__) + assert declared == ANSWER_STATES + + +def test_the_two_endpoints_disagree_only_where_both_contracts_say_so() -> None: + # `refused` and `failed` mean the same thing on both. Everything else is + # deliberately different, and each contract has to warn about the other + # -- because the shapes are near-identical and a consumer will reuse one + # state list for both. One did. + shared = ANSWER_STATES & SUMMARY_STATES + assert shared == {"refused", "failed"} + + answer_doc = ANSWER_CONTRACT.read_text() + summary_doc = SUMMARY_CONTRACT.read_text() + for state in SUMMARY_STATES - shared: + assert state in summary_doc, f"{state} undocumented" + for state in ANSWER_STATES - shared: + assert state in answer_doc, f"{state} undocumented" + + # And each must name the other's divergent success state, which is the + # one that caused actual harm. + assert "summarised" in answer_doc, "the answer contract does not warn" + assert "answered" in summary_doc, "the summary contract does not warn" + + +def test_release_is_the_same_type_on_both_endpoints() -> None: + # It was an int on one and a string on the other, in the same field of + # the same event. A consumer requiring a number got null from one of + # them and did not notice, because null is legitimate on both. + import typing + + from util.embedding_environment import EmbeddingEnvironment + + # The answer endpoint puts `get_release`'s value straight on the wire, + # so its declared return type is the contract. A declaration check, not + # a behavioural one -- reading the real value needs an installed bundle, + # and it is checked against the deployed build in the quickstart. + declared = typing.get_type_hints(EmbeddingEnvironment.get_release)["return"] + assert declared == (int | None) + + # The summary endpoint's arrives as text from the Analysis Service, so + # the normalisation is where its type is decided. That half is checked + # by behaviour. + assert _as_number("97") == 97 + assert _as_number(None) is None + assert _as_number("97-beta") is None