feat: typed exception for a withheld known-good binary - #316
Conversation
A download refused because the sample is a known-good binary now carries a stable machine-readable code in the error envelope, so a caller can tell it apart from a deleted, expired or never-uploaded artifact instead of matching prose. `KnownGoodWithheldException` subclasses `NotFoundException` and is raised from the shared 404 arm when the payload's code says so, exposing the flagging feeds as `.sources`. Subclassing is the point: the status is unchanged and every existing `except NotFoundException` handler keeps catching it, which the downstream contract requires. Every other 404 is unaffected. Also documents the server's `require_scan` semantics: a known-good hash counts as present, because it is a decided terminal record — the platform will never scan it and never stores its binary. The 200-vs-204 contract is unchanged.
|
Reviewed against Correctness / architecture / downstream contract / gitflow: clean. The 404 arm lives in 1. New server-contract behaviour is only covered by fabricated bodies — and the path that actually raises it is untested. (
Specific missing cases, both recordable as VCR lifecycle tests (sync in
2. Confirm the server side is merged first, and link it properly.
Minor: |
The server renamed the withheld-binary cause from KNOWN_GOOD_WITHHELD to KNOWN_GOOD — every member of that closed set is a cause of a refusal, so the suffix restated what the set already implies. The 404 arm has to match the value the server actually sends, or a withheld-binary refusal quietly degrades to a plain NotFoundException. The exception class keeps its name: it describes the outcome a caller catches, not the cause code it was dispatched from.
ReviewMechanics are right: base is 1. No test touches the path the exception exists for, and nothing pins the server envelope (test coverage). The refusal happens on a download, i.e. with pytest.raises(exceptions.KnownGoodWithheldException) as ei:
v3api.download(tmpdir, sha)
assert ei.value.sources == ['nsrl']Same test is the place to pin the other new claim: 2. errors = '\n'.join(str(error) for error in request.errors)The specs now call list-shaped |
…streaming refusal _bad_status_message assumed `errors` was a list, so it joined over it directly. Iterating a dict yields its keys, so any non-404 status carrying the new dict-shaped envelope rendered as "code\nknown_good\nsources" — every value silently dropped from the message the caller sees. The specs now call the list shape legacy and the dict shape the way forward, so this change is what makes the dict expected; both shapes are handled. The exception exists for a refused download, which flows through the streaming arm rather than parse_response, and no test drove that path — the coverage was pure-unit against a body this branch wrote itself. There is now a test that refuses a download at the transport and asserts the typed exception with its sources.
|
Review Mechanics are clean: base is 1. The server contract is only asserted against fabricated responses — Everything new here is pinned by a respx mock (
And this scenario is not one the e2e stack cannot reasonably produce — it is already produced. The commit log shows why this matters:
Please add the cassette-backed slice (sync 2. The new claim — a known-good sha reports present under 3. The new paragraph legitimises a third slice on the respx tier and names an async-only test as the pattern. Invariant 5, unchanged, says:
The consequence is that the sync Minor
|
….sources The streaming-refusal test added last round was an async-only respx body, which the testing spec's harness invariant forbids — and the consequence was that the sync transport's refusal arm had no coverage at all. It now runs on the parametrised harness so one body covers both, and the spec paragraph no longer sanctions a parallel async-only body. The harness moved to its own module rather than being imported from a collected test file. `.sources` was passed through from the wire unvalidated while the downstream contract promises a list of feed names, so a bare string or the feed-dict shape would have survived and made iteration surprising. It is normalised at the boundary: a list of strings stays, a bare string becomes one element, anything else becomes empty.
|
Re: the cassette-backed live-e2e coverage for the refusal and for What recording needs, and where it stops. The refusal only exists once the server half of the paired artifact-index change is running, so the cassette has to be recorded against an e2e stack built from that branch. Building that image fails locally:
What that leaves. The refusal mapping is covered on both transports through the parametrised harness ( Concretely, to close it — on any host that can build the artifact-index image (or in CI with
Placing them after the first create (before the |
|
Base is 1. The server-side contract is only asserted against bodies this branch wrote itself Every assertion on the new envelope —
This PR's own history is the failure mode: commit 2 exists solely because the fabricated body said The respx transport-arm test is well justified and should stay — The slot already exists.
Both need the companion server PR merged before the cassettes can be recorded, which is fine — that PR is already listed under Requires. 2.
3. Stale spec pointer after the harness move
|
The suite asserted the error envelope only against bodies it fabricated itself, which asserts what we think the server sends. This PR's own history is the failure mode: the fabricated body said KNOWN_GOOD_WITHHELD while the server said KNOWN_GOOD, and nothing in the suite noticed. Both known-good lifecycle tests (sync + async) now drive the real endpoint: - download() raises KnownGoodWithheldException and .sources names the flagging feeds, re-checked after a second feed extends the entry - exists(require_scan=True) is True — semantics documented in four places with no coverage until now Cassettes re-recorded against a live stack; the recordings confirm the server's shape is a list of feed-name strings. Also harden _normalise_sources: extract feed['tool'] from dict entries instead of dropping them. That is the other shape the platform uses for this concept (the instance-level known_good field), and silently emptying .sources would take the exception's whole added value with it.
|
Reviewed against Two small things: 1. @property
def last_request_url(self) -> str:
return str(self._router.calls[0].request.url)Carried over verbatim from 2. The new dict branch is right, and the reasoning in the comment ("iterating a mapping yields only its KEYS") applies verbatim to a string — which is exactly why if isinstance(request.errors, dict):
errors = '\n'.join(f'{k}={v}' for k, v in request.errors.items())
elif isinstance(request.errors, (list, tuple)):
errors = '\n'.join(str(error) for error in request.errors)
else:
errors = str(request.errors)Non-blocking: the prose is heavy relative to the code. The |
…r per line Review follow-ups: - The dict branch added earlier reasoned that iterating a mapping yields only its keys; the same applies to a bare string, which iterates as characters. An `"errors": "some prose"` envelope rendered one letter per line. Only list/tuple payloads now get the line-per-entry treatment. - _client_harness: last_request_url read calls[0] while last_request_body read calls[-1]. Invisible in a single-request test, silently wrong in a multi-request one — and this is now the shared harness every respx module imports, not one module's private helper. - Trim the specs/05 paragraph to the contract and let _normalise_sources' docstring carry the per-shape rationale; drop a test comment that narrated review history rather than the risk.
|
Reviewed against 1. The docstring justifies the feed-dict branch on the grounds that silently dropping a recognisable shape would empty 2. The live-e2e tests do not pin the "plain miss stays plain" half of the contract ( The post-delete assertion is with pytest.raises(exceptions.NotFoundException) as ei:
v3api.known_good_get(sha256=sha)
assert not isinstance(ei.value, exceptions.KnownGoodWithheldException)No re-record needed — the cassette already has the interaction. 3. Spec overstates where the mapping-shaped Both now say the mapping shape "is not 404-only — the server forwards it on every arm (400 / 401 / 403 / 413 / 5xx)". True of the server, but Nothing blocking. |
Review follow-ups. - _normalise_sources' docstring argues that a silent discard is the failure mode it exists to prevent, then its fall-through did exactly that for every unrecognised shape. It now logs the discard at debug level. - Both live-e2e lifecycle tests asserted the post-delete 404 with pytest.raises(NotFoundException), which the subclass also satisfies — so the one place the real server returns a plain miss never checked that it stayed the base class. No re-record needed; the cassettes already hold the interaction. - specs/01 and specs/02 said the mapping-shaped errors render applies on every status. True of what the server sends, not of what the caller sees: _bad_status_message has one call site, so the 404/422/429 arms build their messages from _result alone. Narrowed to the RequestException arm, with the payload's reachability at exc.request.errors stated separately.
|
Reviewed against The core mapping is right: 1. Its docstring and the new paragraph in Pick one: move that single assertion into the two live bodies and drop the respx module, or keep the module and rewrite both the docstring and the 2.
3. Comment density, and "regressions" that never shipped.
|
… respx tier Review follow-up, and a spec contradiction this branch introduced. download_refusal_test.py's docstring and the specs/04 paragraph that legitimised it both claimed the streaming refusal was "reachable by neither a pure-unit parse test nor a cassette-backed endpoint test". A later commit on this same branch made that false: both known-good lifecycle tests now drive download() against the live server on both transports, which runs _execute_download's 404 arm end to end. The only assertion unique to the respx module was that the refusal writes nothing to the destination, so that moved into the two live bodies and the module is gone. Keeping it would have left a tier whose stated justification reads as licence to reach for respx on any transport arm. specs/04 now says what actually covers that arm, and when respx is still the right tool (a connection reset, a retry ladder, a truncated body). Also, on prose: specs/05 committed the published surface to accepting the feed-dict sources shape the envelope has never sent. The defensive branch stays (it costs three lines and prevents a silent empty), but the contract is narrowed to the one promise consumers can rely on — always a list of strings — with the per-shape reasoning left in the helper's docstring. Trimmed that docstring and dropped the "Regression:" label from a comment describing a defect that only existed between commits on this branch.
|
Reviewed against 1.
2. Step 3 now reads 3. The internal ticket ID is in the PR description —
The §Merging note spells out 4. Stale Changes list. It credits 5. Record the bump size. "No version bump: that belongs to the develop→master step" is the right call, but Minor, take or leave: |
…ontradicted Review follow-ups, both about statements that were wrong rather than code. - The new os.listdir assertion was justified with "the streaming path opens its destination before the response is read". It does not: _execute_download checks the status before calling open_destination. The assertion is still worth keeping — it pins that the ordering never inverts, since an empty file is indistinguishable from a successful download — so it now says that. - specs/04's decision tree still listed "an SDK transport arm" among the scenarios respx is for, one sentence before the new section saying it is not. That parenthetical was the licence the section exists to remove. - specs/05's Versioning table had no row for a new exception class, so the release PR would have had to infer the bump. Added it, with the reasoning that makes it minor rather than major (the subclass preserves every existing `except NotFoundException`).
ReviewClean against the documented conventions — no action items found. What I checked:
Not verified: I couldn't execute the suite in this environment, so the 157-pass and byte-identical-mirror claims rest on your run. |
… the server
artifact-index reverted the change that made its hash existence probe answer
"found" for a known-good sha256 (PR #1919, commits 5c9268a8 / 0b90f54c). Those
status codes are a frozen contract, documented there in
specs/09-hash-search-head-contract.md: 200 = found, 204 = not found, anything
else = the request was wrong. A KnownGood row is a fact about the sample, not a
searchable record of an artifact, so widening the probe to include it moved a
caller's case from 204 to 200 — which this SDK reads as a bare status code with
no error channel, i.e. a wrong boolean and nothing else.
This side documented and asserted the widened behaviour, which is what turned
artifact-index's e2e job red: the live suite asserted
exists(..., require_scan=True) is True against a server that now correctly
answers 204.
Corrected in all four places (both exists() docstrings, specs/02, specs/03) and
in both lifecycle tests, which now assert what the server actually does for a
hash catalogued through the CRUD:
plain -> True (create_known_good builds a searchable reference
instance, so a real record exists)
require_scan -> False (nothing was ever scanned for it)
Cassettes re-recorded against a live stack running the reverted server, so they
now carry the first HEAD interactions in test/vcr/ — 200 for the plain probe and
204 for require_scan. That matters beyond this fix: every other assertion about
this endpoint in the suite is respx-mocked, so it pins the SDK's own mapping and
would stay green through a server-side flip. That is exactly how the 4.0
exists() inversion survived to ship in 4.0.0 and 4.1.0. These two tests would
not.
|
Review — mostly clean against AGENTS.md / specs/: base is 1. The 2. The new bare-string arm of Minor — specs/04-testing.md:22 describes |
Review follow-ups. - The exists() docstring said a non-2xx never raises "because the probe carries no result parser". Wrong cause, and it generalises to a falsehood: parse_response maps non-2xx to an exception whether or not a parser is set — test_500_raises_even_without_parser pins exactly that. What actually happens is the HEAD short-circuit, which sets the status as the result and returns before the mapping. exists_hash has both properties, so the conclusion held while the reason did not. - Cover _bad_status_message's third arm: a 500 whose errors is a bare string renders on one line. Both specs promise it; the dict and list arms had tests and this one did not. - specs/04 described _client_harness as what "every respx module imports" (two modules use respx directly) and credited the `_` prefix for keeping it uncollected (it is not matching *_test.py / test_*.py that does).
ReviewCorrectness looks sound. Three small things: 1. 2. 3. Merging: the "squash-merge or rename" instruction is correct but depends on a human honouring it — worth setting the merge method on the PR now rather than at merge time, since the branch name is the one internal reference |
…mock The probe's 200/204 arms were covered only by a respx mock, which asserts this SDK's mapping and not the server's behaviour — so a server-side flip left the suite green. That is exactly how the 4.0 exists() inversion shipped in 4.0.0 and 4.1.0, and artifact-index's specs/09 named the gap. test_hash_existence_probe_against_the_real_server (+ async twin) now provisions its own resources the way every other e2e test here does — two deterministic EICAR variants, one submitted and scanned, one nothing ever submits — and asserts: never submitted -> False (204), plain and require_scan submitted+scanned -> True (200), plain and require_scan Deriving the absent case from its own uid rather than probing the sha about to be submitted is what keeps the test re-runnable against a reused stack. Cassettes recorded live, so test/vcr/ now carries HEAD interactions for both codes and both run VCR-off against the stack in every e2e job. The mock keeps exactly one arm — 404 -> False — because a well-formed probe never produces a 404 (artifact-index reserves it for the request being wrong), and its docstring says so rather than implying e2e could not cover the rest. Also from review: - exists()' note leaked a test name into public help() output and said "anything else means the request was wrong", which reads against specs/03 (404 also maps to absent). Rewritten. - _normalise_sources logged its discards at DEBUG while the docstring and specs/05 promise a discard is never silent. A library under an app configured at INFO would show nothing. WARNING matches the promise. - specs/04 claimed every respx module imports ClientTestCase (one does) and invariant 5's only exemption was live/VCR tests, which left the direct-respx bodies in the two client modules non-compliant by its own text. Invariant 5 now names the single-transport exemption, and the harness docstring agrees.
ReviewCorrectness of the SDK change itself looks right: the 404 arm reads the already-extracted 1.
|
…ndex Review follow-ups on the tests added last commit. - test_known_good_lifecycle asserted exists() unpolled immediately after known_good_create, in the same PR whose sibling test explains that the search row is written by an async task. known_good_create returns an artifact_instance_id, so it goes through that same write — the assertion was a flake under TESTS_VCR=off by this PR's own reasoning. - The probe test polled the BROAD form and then asserted the NARROW one. require_scan filters on the scan state of the row the plain form only needs to exist, so it can become true no earlier — polling the loose condition and asserting the strict one is the wrong way round. Both now poll the strict form and assert the captured value, which also keeps each test's request count at or below what its cassette recorded, so no re-record was needed. - Registered `existence_probe` in _LONG_POLE_FRAGMENTS: both tests submit, wait for a settle and then tolerate index lag, but neither substring-matched, so on the live xdist run they would have backfilled the tail — the straggler pattern that list exists to prevent. - specs/04's amended invariant 5 claimed every direct-respx body states why it is not on the harness. Most predate the harness and say nothing, so the claim is now forward-looking, and the count is honest (~27, not "a handful"). - specs/05 relates KnownGoodWithheldException.sources to ArtifactInstance.known_good_sources: same concept, different normalisation (the latter sorted and deduped), so a consumer does not assume parity.
ReviewCore change is sound: Three things. 1. Both new probe cassettes were recorded from an earlier version of the test body.
and the async twin records the same The recorded shape is
2. All the docstring work landed on 3. The 404 -> Nothing blocking beyond (1). |
Both were recorded before the body was restructured to poll the strict require_scan form, so they carried the old order (false, false, true) — the exact 'poll the broad form, assert the narrow one' race the comment above them argues against. Replay passed only because VCR matches by request identity, not order, and left the extra interaction unplayed. Recorded fresh against a live stack: 2 plain + 2 require_scan, matching what the body now runs.
All the docstring work landed on exists(), which never raises it. Every download* method can, and the endpoint catalogue said only 'closes the handle' — so a consumer reading either had no pointer to the new exception. Adds the :raises: (with the NotFoundException-subclass note, .sources, and the nothing-is-written guarantee) and the catalogue rows, including that only a dropped file can be withheld out of the sandbox-artifact route.
The mapping is transport-independent and the sync client reaches it through the same generated `int(result) == 200`, but the test sat in the async module — in the PR that extracted the harness so one body could cover both. Moved onto ClientTestCase and added the require_scan form plus the 5xx case, which is worth pinning as a recorded decision: with no error channel on a HEAD, a server error also collapses to False, i.e. a fabricated negative.
|
All three addressed. 1. Stale probe cassettes — re-recorded ( 2. 3. The 404 arm staying async-only — fixed ( 163 passed. |
|
Reviewed against Three things to fix. 1. The new paragraph says “Every Notably the code already gets this right — the 2. This PR adds a 30x1s poll loop to 3.
|
artifact-index reworked known-good to its authoritative model (its specs/05,
DN-8425): the refusal now fires on the CURRENT understanding — a catalogue entry
for the sha256 AND that entry's extension passing an executable allow-list —
evaluated live per request. Two consumer-visible consequences, now in the
downstream contract: a refused download can start working again with no action
(entry deleted, or the policy narrows), and `ArtifactInstance.state` can carry
the new `NOT_STORED` value for a submission the server declined as known-good
whose hash is no longer currently known-good.
No code change: `state` is a plain string the SDK does not enumerate, the 404
`errors.code` strings are unchanged, and KnownGoodWithheldException keeps its
NotFoundException base. The lifecycle test already catalogues with an eligible
filename ('kg-sample.exe'), so its assertions hold unchanged.
|
Solid change overall: the 404 narrowing is correctly a Three things to fix. 1. Internal ticket ID committed into a public spec — The new paragraph reads: ...as of artifact-index's two-predicate model (its AGENTS.md: "Don't reference ticket IDs or internal project codes in commit messages, PR titles, or PR descriptions. This repo is public; published artefacts shouldn't leak internal references." A checked-in spec file is more permanently published than a commit subject, and this PR's own merge note argues the branch name must not reach history for exactly that reason. Drop the ID — the 2. The new prose says every
The code has this right — 3. Test name asserts the opposite of its body —
Nothing else. |
Review fixes, plus one drift the review missed: - Drop the internal ticket reference from the downstream-contract spec — this repo is public, and the artifact-index specs/05 pointer alone carries it. - specs/03 claimed every download* method can raise KnownGoodWithheldException. download_archive cannot: it fetches the caller-supplied pre-signed object-store URL from the stream() feed (Authorization suppressed), so an error there is the store's own XML body — no coded JSON envelope — and surfaces through the generic arms. The code had this right (it is the one download* method with no :raises docstring); the spec drifted. Scoped the claim to the four artifact-index-served downloads and gave the archive row its own honest note. - The download_sandbox_artifact row still described the retired sample-vs-evidence exemption; the server-side model gates every sandbox artifact by its own sha256. - Renamed test_a_server_error_is_not_reported_as_absent_silently — its body pins the opposite (a 5xx does collapse to a fabricated negative, as a recorded decision), so anyone grepping test names read the inverse of the guarantee.
|
Review Code side is clean: Four things to fix, all documentation:
Only one can be true, and the docstring is the surface a consumer actually reads: if evidence is not exempt, that docstring tells callers not to handle a refusal they will get. Pick one against the real server behaviour and make the two agree (both mirrors + the spec row).
Nothing blocking on tests: the live refusal + probe coverage is the right tier per |
… withheld signal - download_sandbox_artifact's docstring (both mirrors) still described the retired server-side sample-vs-evidence exemption, telling callers not to handle a refusal they will get; specs/03 already had it right. The gate applies to every sandbox artifact by its own sha256. - specs/02 and specs/05 called state == KNOWN_GOOD "the signal that bytes are withheld" in general. Qualified: it signals the *typed refusal*; a NOT_STORED instance has no bytes either but 404s plainly, because nothing is being withheld by policy any more. A consumer branching on state to predict availability would have gotten NOT_STORED wrong. - resources.py: rejoined a comment fragment the rewrite orphaned.
|
Reviewed against Spot checks that hold up:
One thing to fix:
Nothing else needs action. |
The shared :raises block said 'the status is checked before the file is opened' — copy-pasted from the folder-destination methods. download_to_handle never opens anything: the handle is the caller's and arrives open. The guarantee holds (the status check precedes any write); only the justification was wrong, on the one method where a reader most wants it restated.
|
Reviewed against Verified: the 404 arm reads |
2 participants
TL;DR
KnownGoodWithheldException(NotFoundException), raised from the shared 404 arm when the error envelope'scodesays the artifact is a known-good binary whose bytes are withheld — so "withheld by design" is separable from "deleted" without matching prose.HEADexistence probe against the real server, replacing fabricated bodies.exists().Context
A download refused because the sample is a known-good binary used to be indistinguishable from one refused because the artifact was deleted — both a 404 with prose. The server now sends a stable machine-readable code in the error envelope, so a caller can branch on it.
Requires
errors.codestrings are unchanged by #1919.Changes
exceptions.py—KnownGoodWithheldException, with the flagging feeds exposed as.sources. SubclassingNotFoundExceptionis the point: the status is unchanged and every existingexcept NotFoundExceptionhandler keeps catching it, whichspecs/05-downstream-contract.mdinvariant 3 requires._normalise_sourcesguarantees the one documented shape (always a list of strings) and logs what it discards.core.py— the shared 404 arm inspects the already-extractederrorspayload and raises the subclass whencode == 'KNOWN_GOOD'; every other 404 falls through unchanged. Both the JSON and the streaming-download paths go through this mapper, so they behave identically. Also fixes_bad_status_messagefor dict- and string-shapederrorspayloads — a dict used to render as itsreprand a string as one character per line.aio/api.py+ the regenerated mirror —exists()'s docstring corrected:require_scannarrows what counts as found, and the reason a non-2xx does not raise is thatparse_responseshort-circuits HEAD before the status mapping (not a missing parser).404is documented as also meaning absent. Everydownload*method gained a:raises:for the new exception, with the nothing-is-written guarantee.specs/03,specs/04,specs/05— the endpoint catalogue's download rows, the testing tiers, and the downstream contract.Tests
download()raises the typed exception,.sourcesnames the flagging feeds (re-checked after a second feed extends the entry), and the probe's200/204boundary is exercised in both the plain andrequire_scanforms. That isspecs/04-testing.mdinvariant 1, and it matters here: an earlier commit on this branch exists only because a fabricated body saidKNOWN_GOOD_WITHHELDwhile the server saidKNOWN_GOOD, and nothing in the suite noticed. Cassettes recorded against a live stack, and re-recorded when the bodies changed — an earlier pair had been recorded from a previous version of the test and only replayed because VCR matches by request identity rather than order.test/exists_probe_mapping_test.py— the arms the e2e stack cannot produce (404→False, plus the5xxfabricated-negative case recorded as a decision, not an accident), on the sharedClientTestCaseharness so both transports are covered. The200/204arms stay live, because only those can catch a server-side flip.test/_client_harness.py— the parametrised harness extracted so any respx module can import it (andlast_request_urlfixed to read the last call, matchinglast_request_body).pytest— 163 passed. The sync mirror is byte-identical after regeneration, so CI's staleness check is clean. No version bump: that belongs to the develop→master step.What artifact-index's model rework means here (no code change)
#1919 replaced the server's known-good design with a two-predicate model. Two consequences are worth knowing as a consumer, and are now in
specs/05-downstream-contract.md:ArtifactInstance.statecan carry a new value,NOT_STORED: a submission the server declined as known-good at the time, whose hash is no longer currently known-good. Nothing was ever stored for it and a fresh submit of the same file works.stateis a plain string the SDK does not enumerate, so a new member needs no SDK release.The
errors.codestrings (KNOWN_GOOD/DELETED/EXPIRED/NOT_STORED) are unchanged, and the known-good lifecycle test already catalogues with an eligible filename (kg-sample.exe), so its assertions hold as written.