Release 4.3.0 - #319
Merged
Merged
Conversation
Removes per-test verbose lines so the suite prints one progress char per test plus the end-of-run summary; failures still print full tracebacks, and -ra adds a short-summary recap of failures/errors/skips. Keeps live streaming (-s).
Dropping -v (prior commit) was necessary but not sufficient: pytest enables live logging when log_cli is true OR the log_cli_level option is set, and conftest was unconditionally setting that option — which force-enables live logging, and live logging prints every test's nodeid on its own line (verbose-style) regardless of -v. That was ~130 [gwN] PASSED noise lines on a green 8-way run streamed into the harness log. - pyproject: log_cli = false (was true). - conftest: set only log_level (captured, shown under a failing test) from TESTS_LOG_LEVEL by default; set the log_cli_level option only when TESTS_LOG_CLI=1 is passed. An explicit --log-cli-level is still honored. Green run -> dots + summary; a failing test still shows its traceback + captured logs. Opt into live streaming with TESTS_LOG_CLI=1. Documented in specs/04-testing.
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.
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.
test: quieter pytest output for the live e2e run
…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.
….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.
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.
…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.
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.
… 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.
…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`).
… 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 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).
…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.
…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.
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.
The sandbox tests dispatched 'triage' with vm_slug='win10-build-15063' at 8 call sites. That is not a valid triage VM: the server rewrites any slug containing 'win10' to 'windows11-21h2-x64' for backward compatibility and logs an ERROR each time it does. Every e2e run therefore carried 8 server-side ERROR lines that were entirely self-inflicted. Send the real slug instead. 'triage' offers windows11-21h2-x64 (plus ubuntu-22.04-amd64 and android-11-x64); the win10-style slug belongs to a different provider and does not apply here. Cassettes are deliberately NOT touched. vm_slug travels in the POST body and the VCR matcher is [method, scheme, host, port, path, query], so replay is unaffected — and hand-editing cassettes is against the recording convention. The old recorded bodies simply get refreshed the next time these cassettes are re-recorded delete-driven against a live stack. Verified: 13 sandbox/sample tests pass on replay.
test: use the current triage VM slug, not the deprecated win10 one
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.
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.
… 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.
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.
feat: typed exception for a withheld known-good binary
|
Reviewed against 1. The version bump is not in this PR — merging as-is ships 4.3.0 code with no release.
So merging #319 first puts the whole 4.3.0 surface on 2. Sandbox cassettes were not re-recorded after the triage VM-slug change.
Fix is the documented loop against a fresh stack: delete the eight cassettes above, confirm live with Minor while you are there: the TL;DR changelog does not mention this slug fix (#317), which is the one behavioural change in the release that is not about known-good or the probe. Notes, no action needed
|
mjbradford89
previously approved these changes
Aug 4, 2026
release: bump version to 4.3.0
|
Release review — reads clean against Verified
Items
None of these block the release. |
mjbradford89
approved these changes
Aug 4, 2026
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
3 participants
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.
TL;DR — changelog for 4.3.0
KnownGoodWithheldException, carrying the server's machine-readable refusal code and the flagging sources, on both the sync and async clients.sourceslist is surfaced explicitly instead of being silently dropped.Requires