Read an analysis result, and define what may leave this service - #269
Merged
Merged
Conversation
Spec 011 Phases 1 and 2: the client and the disclosure allow-list. No endpoint yet, and nothing calls a model. `src/analysis/client.py` fetches a completed result by token from beta, never production, with `ANALYSIS_BASE_URL` to point elsewhere without the default ever being production. Three behaviours were measured against beta rather than read from the OpenAPI, because it is wrong or silent about all three: 404 is an unknown token, 410 is a result deleted by a release and stays distinct because the reader can re-run, and **500 is what a malformed token returns** -- undocumented, and treating it as a fault would mean a `failed` state or a retry loop against a service that answers identically every time. A library user-agent gets 403 with an HTML body on every endpoint, so requests carry a browser one. `src/analysis/disclosure.py` is an allow-list, and that is the design rather than an implementation detail. "Strip the identifiers" misses `summary.fileName`, `summary.sampleName` and `expression.columnNames`, all user-supplied free text. A denial list is wrong the moment the service adds a field; an allow-list is wrong only by omission, which costs a sentence instead of a disclosure. A test adds fields the service does not have -- `patientNotes`, `donorIdentifier`, `submitterComment` -- and asserts they do not reach the payload, which is the property that distinguishes the two. Three things the real service corrected. `/database/version` answers text/plain and rejects `Accept: application/json` with **406**. Every mocked test passed, because a mock cannot refuse a header it was never told about. Found by calling beta; pinned now as the property rather than the status code. The result shape is not quite the data model's: `summary` carries no `species`, and `entities` has no `curatedFound`/`interactorsFound` unless interactors were requested. Every allow-listed field is therefore optional and absence is normal. An eight-gene list produced 1,280 pathways, so the payload is bounded to the top twelve with the total kept -- 5.4kB against a result that would otherwise be enormous. T002 said to add `tests/analysis/__init__.py` "alongside the existing tests/api/". No test directory here has one, and adding it put `tests/analysis` on sys.path as the top-level `analysis` package, shadowing `src/analysis`. Test basenames must be unique for the same reason, so this is `test_analysis_client.py` rather than colliding with the MCP one. The task is corrected in place rather than silently done differently. Verified end to end against beta: release 97, a real token summarised to 12 of 1,280 pathways with no forbidden field in the payload, unknown and malformed tokens both a clean negative outcome. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The worst of three, and it defeats the point of the feature.
The analysis token is caller-supplied and interpolated into a URL path, so
`<real token>%3D/notFound` addresses `GET /token/{token}/notFound` -- the
endpoint returning the user's *unmatched identifiers*. That is the identifier
tier, reached by a caller who asked only for an aggregate summary, and the
allow-list cannot help because the disclosure happens at fetch time. Shown
against beta: it returned the submitted identifiers.
It did not disclose anything today, and only by accident. A list body made
`is_gsa` raise AttributeError out of a function whose contract is that it
never raises, so the second bug masked the first. Two bugs cancelling is not
a defence and neither is fixed by fixing one.
Tokens are now validated against the form the service issues -- base64 with
percent-encoded padding -- and rejected before any request. Rejecting rather
than escaping, because the token arrives already percent-encoded and quoting
it again would break every valid one. A rejected token yields `not_found`,
which is what the service answers for a malformed token anyway (measured:
500, mapped to not_found), so this adds no new outcome for a caller to
handle. A non-object body is now `failed` rather than an exception.
Six escape strings are pinned, including the one that matters; all five
relevant tests fail against the unfixed client.
Third, smaller, and recorded rather than solved. `warnings` was allow-listed
by field, but it is the one field whose *content* the service composes
freely, and nothing in its contract stops it quoting what the user submitted.
On beta it is service-level ("Missing header. Using a default one."), and one
observation is not a guarantee. This service never sees the user's
identifiers so it cannot filter them out of a warning it is handed; what it
can do is bound the exposure, so five warnings of 200 characters. Kept rather
than dropped because what the service flagged is how a summary knows a result
is untrustworthy.
The general lesson for the allow-list: it guarantees *fields*, and a field
carrying free text is a hole in that guarantee rather than an exception to it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Spec 011 Phase 3, and Phase 8 with it, since the contract settled while this was being built and leaving the gate unimplemented would have meant reworking the endpoint immediately. `POST /api/analysis-summary` streams SSE in the answer endpoint's shape -- citations as their own events, failure as a terminal state, always HTTP 200 -- because an analysis page must not break because this service had a problem. The authorisation bar is stricter. `verify` asserts caller identity and says nothing about a person; `human_presence_reason` additionally requires `human` and an `human_iat` within 30 minutes, checked before any model call. The refusal carries a reason -- `no_caller`, `no_human`, `stale_human` -- because "failed" is not something a panel can say to a reader, and stale is the only one they can act on. Citations are emitted from the result, not parsed out of the model's prose, so an invented or mismatched identifier is impossible by construction rather than caught afterwards (SC-003). The verdict -- empty, nothing_significant, has_findings -- is computed in code and handed to the model as an instruction. A model given rows sorted by p-value and asked to be careful writes a confident account of the top row; one told "nothing passed correction" does not (FR-004). Two things the tests caught. The freshness bound was compared with a float clock, which makes the inclusive edge unreachable: a claim issued exactly 1800s ago is 1800.0003s old by the time it is checked. Now whole seconds on both sides, matching the website's `nowSeconds - floor(solvedAt/1000)`. This is the same precision mistake I had just raised with them, in my own code. And the served path is tested over HTTP on a mounted app, not by calling the handler: that the user's filename never reaches the model is a different claim from the allow-list unit test, and only the served path can make it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ontain Found by calling the real model with a real result, which no test did -- every one of them stubs the LLM, so this was invisible to all of them. The payload is bounded to the top twelve pathways of 1,280. The prompt input reported twelve significant beside 1,280 total, and the model wrote "a total of 12 significant pathways out of 1280 pathways assessed". False: twelve were sent, all twelve passed, so the true count is at least twelve and unknown above. FR-002 forbids exactly this, and the truncation created it rather than the model. The prompt input now separates `pathways_shown` from `pathways_total`, reports `significant_among_shown`, and carries `significant_count_is_exact`. Exact only when a non-significant pathway appears among those shown -- in a p-value ordered list, everything below it is non-significant too. The ordering is checked rather than assumed, because relying on another service's default sort is how a claim goes quietly wrong. When it is a lower bound the model is told so, and the system prompt forbids the claim. Re-run against the real model: the sentence is gone. Two smaller ones, both about telling the caller something untrue. A rate-limited caller was refused with `no_caller`, which an interface renders as "not verified" to a reader who is verified and merely asked too often. It has its own reason now. And `disclosure: identifiers` was silently served an aggregate summary, because the tier is agreed but unbuilt. That discloses nothing extra and is still wrong: the reader chose the disclosing option and would be given the other one with nothing saying so. A choice quietly overridden is worse than a choice refused, because it looks like it was honoured. Refused as `unsupported_tier` until Phase 4. The general note is in research D9: bounding a payload for cost is not neutral. It changes what the data appears to say, and a model reads the appearance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Spec 011 Phases 1 and 2 — the client and the disclosure allow-list. No endpoint yet, and nothing calls a model.
The client
Fetches a completed result by token from beta, never production (
ANALYSIS_BASE_URLpoints elsewhere without the default ever being production). Three behaviours measured against beta rather than read from the OpenAPI, which is wrong or silent about all three:failedstate or a retry loop against a service that answers identically every timeA library user-agent gets 403 with an HTML body on every endpoint, so requests carry a browser one.
The allow-list
disclosure.pyis an allow-list, and that's the design rather than an implementation detail. "Strip the identifiers" missessummary.fileName,summary.sampleNameandexpression.columnNames— all user-supplied free text. A denial list is wrong the moment the service adds a field; an allow-list is wrong only by omission, which costs a sentence instead of a disclosure.The test that pins this adds fields the service doesn't have —
patientNotes,donorIdentifier,submitterComment— and asserts they never reach the payload. That's the property distinguishing the two designs; asserting only on today's known-bad fields would pass for a denial list too.Three things the real service corrected
/database/versionanswers text/plain and rejectsAccept: application/jsonwith 406. Every mocked test passed, because a mock cannot refuse a header it was never told about. Found by calling beta, and now pinned as the property rather than the status code.The result shape isn't quite the data model's —
summarycarries nospecies, andentitieshas nocuratedFound/interactorsFoundunless interactors were requested. Every allow-listed field is therefore optional and absence is normal.An eight-gene list produced 1,280 pathways, so the payload is bounded to the top twelve with the total kept: 5.4kB.
A task whose premise was wrong
T002 said to add
tests/analysis/__init__.py"alongside the existingtests/api/". No test directory here has one, and adding it puttests/analysisonsys.pathas the top-levelanalysispackage, shadowingsrc/analysis. Test basenames must be unique for the same reason, so this istest_analysis_client.py. Corrected in the task rather than silently done differently.Verified against beta
Release 97; a real token summarised to 12 of 1,280 pathways with no forbidden field in the payload; unknown and malformed tokens both a clean negative outcome.
533 passed, 1 skipped; mypy over 144 files, ruff and ruff format clean.
🤖 Generated with Claude Code