From d0c9a1ecacad1dd34843d811c8445b8049767b0d Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Sun, 20 Sep 2026 18:41:35 +0000 Subject: [PATCH] Test the two bounds that were written and never exercised Spec 011 Phase 9, less the two tasks that need a deploy. T034 and T035 were implemented in Phase 3 by copying the answer endpoint's patterns, and neither had a test. Written code with no test is not the same as working code, and both of these only run on paths nobody exercises deliberately. The bound now has one: a stuck model, the timeout patched to 0.25s, and the stream must still end with a terminal `failed` rather than holding the connection. The stand-in hangs for a finite five seconds so a regression fails quickly instead of hanging the suite. And a second test the task did not ask for: a timed-out generation must not store its partial text. `put` already refuses empty text, but a truncated summary is not empty -- it is worse, a plausible fragment ending mid-sentence that would then be served as the summary forever. The abandoned-stream log has one too, driven through the response iterator rather than a client, because the point is to close it mid-stream and a TestClient will not. An abandoned stream is identical to a healthy one in every other signal: the request 200s, tokens flow, and then nothing happens. Also, at the website's request, expression summaries now refer to columns as `column 1`, `column 2` and in no other form. They hold the real labels and we never do, so they substitute them into our prose -- which only works if our wording is fixed. They had decided to show labels alongside rather than splice on brittle matching; pinning the form is the better half of that trade, and it is in the contract now. All three verified by sabotage, with each sabotage asserted to have applied. Co-Authored-By: Claude Opus 5 --- .../contracts/summary_endpoint.md | 7 ++ specs/011-summarise-analysis-results/tasks.md | 6 +- src/analysis/summarise.py | 9 +- tests/analysis/test_summarise.py | 16 +++ tests/api/test_analysis_summary.py | 106 +++++++++++++++++- 5 files changed, 136 insertions(+), 8 deletions(-) diff --git a/specs/011-summarise-analysis-results/contracts/summary_endpoint.md b/specs/011-summarise-analysis-results/contracts/summary_endpoint.md index dc162bb..d8f01a9 100644 --- a/specs/011-summarise-analysis-results/contracts/summary_endpoint.md +++ b/specs/011-summarise-analysis-results/contracts/summary_endpoint.md @@ -104,6 +104,13 @@ 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. +**Expression summaries refer to columns as `column 1`, `column 2`**, numbered +from one in the order the values appear, and in no other form. The labels are +user-supplied text this service never receives, so the caller is the only side +that can name them -- and can only substitute them into the prose if the +wording is fixed. Anything else ("the first column", "the leftmost sample") +reaches the reader as written. + **`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 diff --git a/specs/011-summarise-analysis-results/tasks.md b/specs/011-summarise-analysis-results/tasks.md index d1c9432..b62ea20 100644 --- a/specs/011-summarise-analysis-results/tasks.md +++ b/specs/011-summarise-analysis-results/tasks.md @@ -127,12 +127,12 @@ carried. Everything in this phase can be built and tested before that lands. - [x] T031b Key the rate limiter on `human_sub` when present, falling back to the caller identity, in `src/api/analysis_summary.py` — per-person throttling rather than per-proxy-address. Also limit per analysis token: twenty summaries of one analysis is not a scientist - [x] T031c Test that a `human` claim with a stale `human_iat` is refused with **zero model calls**, in `tests/api/test_analysis_summary.py` — the freshness bound is the half most likely to be dropped, because the claim being present looks like success - [x] T032 [US1] Verify the assertion in `src/util/caller_token.py` -- `human_presence_reason` checks `human` and a 30-minute `human_iat`, refusing before any model call -- [ ] T033 [P] Test that a request without the assertion is refused and makes **zero model calls**, counted on a patched graph rather than inferred from timing, in `tests/api/test_analysis_summary.py` (SC-004) +- [x] T033 [P] Test that a request without the assertion is refused and makes **zero model calls**, counted on a patched graph rather than inferred from timing, in `tests/api/test_analysis_summary.py` (SC-004) ## Phase 9: Polish -- [ ] T034 [P] Bound the summary in `src/api/analysis_summary.py` as the answer endpoint is, so a stuck upstream cannot hold a connection -- [ ] T035 [P] Log an abandoned summary stream in `src/api/analysis_summary.py`, as the answer endpoint does, so a caller that starts summaries it does not want is visible +- [x] T034 [P] Bound the summary in `src/api/analysis_summary.py` as the answer endpoint is, so a stuck upstream cannot hold a connection. **The bound existed since Phase 3 and was untested until now**; the test also pins that a truncated summary is never stored, which would otherwise be served forever +- [x] T035 [P] Log an abandoned summary stream in `src/api/analysis_summary.py`, as the answer endpoint does, so a caller that starts summaries it does not want is visible - [ ] T036 Run the [quickstart](./quickstart.md) scenarios against beta with a real analysis token and record the outcome, including first-token timing - [ ] T037 Tell the website session the endpoint exists, what it does not yet do, and the `gone` outcome they must handle — only once it is live on beta, not when it merges diff --git a/src/analysis/summarise.py b/src/analysis/summarise.py index 3654459..a9755aa 100644 --- a/src/analysis/summarise.py +++ b/src/analysis/summarise.py @@ -162,8 +162,13 @@ def prompt_input(payload: dict[str, Any]) -> dict[str, Any]: "highlighted pathways behave *across* those columns -- rising, " "falling, mixed -- rather than treating the result as a single " "enrichment. **The columns are unlabelled here and you must not " - "guess what they are**: say 'the first column' and so on, never a " - "condition, timepoint or sample name." + "guess what they are**: never a condition, timepoint or sample name. " + "Refer to them in exactly this form -- `column 1`, `column 2`, " + "numbered from one in the order the values appear -- and in no other " + "form, because the interface holds the real labels and substitutes " + "them by matching that exact wording. 'The first column' or 'the " + "leftmost sample' will not be matched and will reach the reader as " + "written." ), "SPECIES_COMPARISON": ( "This is a species comparison. The findings are **inferred by " diff --git a/tests/analysis/test_summarise.py b/tests/analysis/test_summarise.py index 9d63439..1a0c414 100644 --- a/tests/analysis/test_summarise.py +++ b/tests/analysis/test_summarise.py @@ -239,3 +239,19 @@ def test_no_type_instruction_claims_another_types_reading() -> None: assert "orthology" not in TYPE_INSTRUCTION["EXPRESSION"] assert "columns" not in TYPE_INSTRUCTION["SPECIES_COMPARISON"] assert "columns" not in TYPE_INSTRUCTION["OVERREPRESENTATION"] + + +def test_expression_columns_have_one_stable_reference_form() -> None: + # The website holds the real column labels and we never do, so it + # substitutes them into our prose -- which only works if our wording is + # fixed. They said they would rather show labels alongside than splice + # on brittle matching, so the wording is pinned instead: `column 1`, + # `column 2`, numbered from one. + from analysis.summarise import TYPE_INSTRUCTION + + expression = TYPE_INSTRUCTION["EXPRESSION"] + assert "`column 1`" in expression + assert "numbered from one" in expression + assert "in no other form" in expression + # And the forms that would break their matching are named as wrong. + assert "The first column" in expression diff --git a/tests/api/test_analysis_summary.py b/tests/api/test_analysis_summary.py index 2fbc516..1b0d176 100644 --- a/tests/api/test_analysis_summary.py +++ b/tests/api/test_analysis_summary.py @@ -9,10 +9,11 @@ no model call happens without a person -- none of which depends on content. """ +import asyncio import json import time -from collections.abc import AsyncIterator -from typing import Any +from collections.abc import AsyncGenerator, AsyncIterator +from typing import Any, cast import jwt import pytest @@ -23,7 +24,7 @@ from analysis.client import Fetched from analysis.store import SummaryStore -from api.analysis_summary import router +from api.analysis_summary import SummaryRequest, analysis_summary, router from util.caller_token import DEFAULT_AUDIENCE from util.rate_limit import SlidingWindowLimiter @@ -607,3 +608,102 @@ async def _gone(_token: str) -> Fetched: _events(_post(public, caller_token=_token(private)).text)[-1][1]["state"] == "gone" ) + + +class _HangingModel(_Counter): + """Stands in for a model that has stopped answering.""" + + async def astream(self, _messages: Any) -> AsyncIterator[Any]: + self.calls += 1 + yield type("Chunk", (), {"content": "starting"})() + # Finite, so a regression fails in five seconds rather than hanging + # the suite for the full timeout. + await asyncio.sleep(5) + yield type("Chunk", (), {"content": "never arrives"})() + + +def test_a_stuck_model_still_ends_the_stream( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # T034. FR-008 says a failure must be a terminal state the caller can + # render, never a broken panel -- which means never an open connection + # either. An analysis page must not hang because this service did. + monkeypatch.setattr("api.analysis_summary.get_llm", lambda *a, **k: _HangingModel()) + monkeypatch.setattr("api.analysis_summary.SUMMARY_TIMEOUT_SECONDS", 0.25) + private, public = keys + + started = time.monotonic() + response = _post(public, caller_token=_token(private)) + elapsed = time.monotonic() - started + + assert response.status_code == 200 + assert _events(response.text)[-1][1]["state"] == "failed" + assert elapsed < 2, f"stream ran {elapsed:.1f}s; the bound did not fire" + + +def test_a_stuck_summary_is_not_stored( + keys: tuple[str, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # The partial text of a timed-out generation must not become the summary + # served forever after. `put` refuses empty text, but this one is not + # empty -- it is worse, being a plausible fragment that ends mid-sentence. + store = SummaryStore() + monkeypatch.setattr("api.analysis_summary._store", store) + monkeypatch.setattr("api.analysis_summary.get_llm", lambda *a, **k: _HangingModel()) + monkeypatch.setattr("api.analysis_summary.SUMMARY_TIMEOUT_SECONDS", 0.25) + private, public = keys + _post(public, caller_token=_token(private)) + assert len(store) == 0, "a truncated summary was stored" + + +def test_an_abandoned_summary_stream_is_recorded( + keys: tuple[str, str], + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + # T035. A caller that hangs up mid-summary leaves no other trace: the + # request 200s, tokens flow, and then nothing more happens -- identical + # to a healthy stream in every signal. The website already hit this on + # the answer route, where a keystroke unmounted their panel mid-answer. + # + # Driven through the response iterator rather than a client, because the + # point is to close it mid-stream and a TestClient will not. + from types import SimpleNamespace + + class _SlowModel(_Counter): + async def astream(self, _messages: Any) -> AsyncIterator[Any]: + self.calls += 1 + for index in range(50): + await asyncio.sleep(0.01) + yield type("Chunk", (), {"content": f"t{index} "})() + + monkeypatch.setattr("api.analysis_summary.get_llm", lambda *a, **k: _SlowModel()) + private, public = keys + request = SimpleNamespace( + app=SimpleNamespace(state=SimpleNamespace(caller_token_key=public)) + ) + + async def drive() -> None: + response = await analysis_summary( + SummaryRequest( + token="MjAyNjA5MTkxODExNDJfMTE", + caller_token=_token(private), + disclosure="aggregate", + ), + request, # type: ignore[arg-type] + ) + iterator = cast("AsyncGenerator[str, None]", response.body_iterator) + seen = 0 + async for _chunk in iterator: + seen += 1 + if seen == 4: + break + await iterator.aclose() + + with caplog.at_level("INFO", logger="api.analysis_summary"): + asyncio.run(drive()) + + messages = [record.getMessage() for record in caplog.records] + assert any( + "abandoned" in message for message in messages + ), f"no record of the abandoned stream; logged: {messages}"