Expose matched strings on hunt-result resources - #322
Conversation
A hunt result says which rule matched and its tags, never why. The server now returns the yara strings behind a hit; parse them onto LiveHuntResult and HistoricalHuntResult, and so onto their list subclasses. Read with .get() rather than a subscript. The key is additive, so a server predating it omits it entirely and a subscript would raise on every result. Three states reach callers and they are not interchangeable: None (not reported -- an older server, removed evidence, or a list endpoint, which omits it rather than fetch a blob per row), [] (the rule matched with no byte evidence to show), and a populated list (the evidence, as a lower bound rather than a match count).
Pure-unit: the resources parse a dict, so no HTTP and no stack is involved. Covers all four affected classes -- the list subclasses inherit __init__ and must behave identically -- and asserts the states stay distinguishable: an absent key and an explicit null both read as None, while an empty list stays an empty list rather than collapsing into it. Entries pass through verbatim, since only yara knows whether `data` is a hex dump or escaped text.
Records what each state means, the per-entry dict shape, and the two properties consumers get wrong: a populated list is a lower bound rather than a match count, and `truncated` means "there was more than this" rather than an exact size. Also states that the evidence rides on the detail routes only -- the paginated feed methods will always yield None -- so a caller looking for strings knows to fetch a single result.
Comment text only -- the parsed AST is identical before and after. Both copies edited identically, since the two resource classes carry the same note. Keeps what the comment exists for: why .get() and not a subscript, and that the three states are distinct.
|
Reviewed against The code is correct: 1. No live-e2e assertion pins the server shape ( The hook already exists and costs one line each: assert result.matched_strings, 'detail route should carry the yara evidence'
assert my_results[0].matched_strings is None, 'list rows omit the evidence'then re-record per the delete-driven workflow ( 2. Nothing else. The three-state framing in |
The suite was pure-unit only, so nothing asserted that the detail route actually emits the key or that the list route omits it. Those tests exercise dict.get and would pass identically if the server never grew the field and the attribute were a permanent None -- which is the gap the e2e-first invariant exists to close: a fabricated response asserts what we think the server returns, a cassette asserts what it actually returned. test_live and test_async_live already poll the detail route on a rule that matched their own artifact, so the assertions cost two lines each and pin the list-vs-detail split that was previously stated only in prose. Cassettes re-recorded delete-driven against a live stack running the matching server and analyzer branches. Both now carry real evidence -- the per-test rule keys on the test's uid, so the recorded hit is `$u` at offset 69 with the uid's own length -- and null on the list rows. Verified the assertions are load-bearing: against the previous cassettes they fail with "detail route should carry the yara evidence / assert None".
The resources spec is what to read before changing a resource's parsing, and it carries the two closest precedents -- known_good / known_good_sources and state are both documented there as additive, .get()-parsed fields in exactly this situation. matched_strings had no note, so a reader following the spec index would not find it. Points at the three-state table in the downstream-contract spec rather than restating it, and records the part a parser needs up front: which value means "not reported" versus "matched with no evidence", and that the list endpoints always yield the former by design.
|
Reviewed against AGENTS.md + Two things worth fixing: 1. Spec says the list route omits the key; the recorded server actually sends an explicit null.
The re-recorded cassettes disagree: every 2.
Minor / non-blocking: the branch name Nothing blocking on correctness, contract shape, or gitflow. |
`matched_strings_dropped` on both hunt-result resources, inherited by their list subclasses. The server bounds how many matched-string bytes one result may carry, and a truncated list is otherwise indistinguishable from a complete one: a caller reading 75 entries concludes the rule hit 75 times when it hit 400. A sibling attribute rather than a key inside `matched_strings`, which stays a plain list -- so this is additive to the shape already documented, not a change to it. Parsed with .get() like the field beside it: None means nothing was withheld, which is also what a server predating the bound reports, so callers need no special case for the older shape. It can never accompany an empty list, since a match's first string is never withheld. Also corrects the downstream contract, which told consumers fast-scan reports only the first offset per string and that this was one reason the list is a lower bound. Measured against yara 4.5.2 that is false -- fast mode collapses repeats of a single string only when the rule's condition does not need them, and never limits how many distinct strings a rule reports. The remaining reasons are sound and the byte bound is now a fourth. 195 tests pass.
|
Review Core change ( Three things need action. 1. The cassettes were re-recorded in That makes
That is exactly the state Please confirm artifact-index emits that exact key, and if so re-record the cassettes against that build so at least the 2. Both specs describe the list-route wire shape incorrectly
Related, in the 3. PR description is stale against the final diff The TL;DR still asserts "Fast-scan reports one offset per string" — a claim Minor
Checked and fine
|
…e wire shape The cassettes were recorded before this attribute existed -- two commits before -- so neither carried the key. The pure-unit tests exercise dict.get and would have passed identically if the server never emitted it, which is the exact gap the earlier commit here argued the e2e-first invariant exists to close. Applied unevenly is worse than not applied. Re-recorded delete-driven against a stack running the matching server branch. The assertion reads `'matched_strings_dropped' in result.json` rather than checking the attribute for None, because `is None` cannot distinguish a served null from an absent key -- and what needs pinning is that the server SENDS the field. Its value is null there: the per-test rule is small and withholds nothing, so the null arm is what a passing hunt actually looks like. Also corrects both specs, which said list endpoints OMIT the key. They send an explicit null; omitted is the older-server case. That wording survived from a design that was reverted, and the recorded cassettes disagreed with it. And in the new section, "None means nothing was withheld" was stated flatly while the section above it is careful that None on matched_strings means "we don't know" -- the same conflation, now reading the same way in both places. 195 tests pass.
|
Reviewed against Four things to fix. 1. Commit 8's Commit 8 rewrote
The flat claim it replaced is still live in two other spots, both introduced by commit 7 and untouched by 8:
That is the exact conflation commit 8's own message calls out — except it is not, in fact, "now reading the same way in both places." A reader following the spec index hits Fix spec 02 and both code comments to match the corrected 05 wording. 2.
result = cls(_content(cls, matched_strings=_STRINGS, matched_strings_dropped=19))
assert isinstance(result.matched_strings, list)
assert len(result.matched_strings) == 2The docstring says "the count is what says it is short," but nothing reads While in there: 3. Lines 21-27 enumerate every test module and its tier. 4. Both new spec sections assert the contract for "all four classes"; the e2e assertions cover only the live pair. Not a blocker — the stack limitation is real and pre-existing. But commit 5's own argument is that a fabricated response only asserts what we think the server returns, and the specs currently state the historical half as established fact. Either soften them to say the historical routes follow by symmetry with live rather than being pinned, or record the gap in Minor The branch name carries a ticket ID. |
… their claims The previous commit said the withheld-count conflation now "reads the same way in both places". It did not: the flat claim -- None means nothing was withheld -- was corrected in one spec and left standing in specs/02-resources.md and in both copies of the resources.py comment. specs/02 contradicted itself four lines apart, telling a reader None means nothing was withheld and then that the list endpoints always send None by design, so for a list row it asserted both "nothing withheld" and "we did not look". All three now carry the same reading. Two tests did not test what they were named for: - test_dropped_is_independent_of_the_strings_list passed a count and never read it -- removing the kwarg left it passing identically, duplicating the test above it. Renamed for the property it actually pins and now asserts the count, so the populated-list-plus-count pairing is covered. - test_raw_json_still_carries_the_key compared a dict to itself: __init__ does self.json = content, so the assertion could not fail. Replaced with the property that could -- parsing must not drop keys from .json, and the parsed attributes must agree with the raw payload. Also records what is and is not pinned against a real server. The live pair is verified end to end; the historical pair follows by SYMMETRY, because the e2e stack does not reliably populate historical results inside a test window. Both specs stated the contract for "all four classes" as established fact, which is the same "asserts what we think the server returns" problem the e2e-first invariant exists to prevent. Softened, with the gap recorded in 99-open-questions.md alongside what would close it. specs/04-testing.md's module inventory gains this module, and known_good_test.py which was already missing. 195 tests pass.
ReviewParsing change itself is clean: both fields are additive, read with 1. Test coverage — the per-entry dict shape is contract but is only pinned by a fixture. assert set(result.matched_strings[0]) == {'offset', 'identifier', 'length', 'data', 'truncated'}Without that, a server-side rename of a per-entry key passes the whole suite VCR-off. 2. Spec drift (minor) — 3. Merge ordering. The new live assertions fail VCR-off until artifact-index#1949 is in the image the e2e job resolves — Nothing else: the |
…oute framing The per-entry dict shape is contract -- specs/05 documents offset, identifier, length, data and truncated -- but the only assertion on it compared against a hand-written fixture, which is what we THINK the server sends rather than what it does. A server-side key rename would have passed the whole suite VCR-off. Both cassettes already carry a real entry, so the live tests now assert the key set against the recorded response, which costs nothing and closes the gap. Separately, the spec framed the `…List` subclasses as the list-route parsers. That is not true of the live pair: live_feed builds its request with LiveHuntResult.list(...), which hits /hunt/live/list but parses rows as LiveHuntResult. LiveHuntResultList is only ever a delete builder and is never instantiated from a response; only historical_results yields …List instances. The framing matters rather than being pedantry: the section exists to say that None is ambiguous, and a reader who takes the class as the route concludes a LiveHuntResult must have come from the detail route and reads its None as "nothing to show". Corrected in the spec and in the test module's comment. 195 tests pass.
|
Reviewed against The code change itself is clean: two One thing to fix:
The first clause is right, the rest isn't. This isn't pedantry given what the section is for. The paragraph exists to stop a reader inferring the route from the class, and it currently teaches the inverse error: a caller who iterates Suggested correction: " Nothing else. The |
The previous commit corrected the class-to-route framing and got the replacement wrong. It said LiveHuntResultList "is never instantiated from a response; only historical_results() yields ...List instances". Both halves are false: _build_request sets result_parser=cls, so the paginated body returned by DELETE /hunt/live/list is parsed through LiveHuntResultList. The cassette re-recorded in this PR contains exactly that exchange. That makes delete responses a FOURTH source of None -- alongside an older server, a list route and removed evidence -- and none of the three places that enumerate the causes listed it. A caller iterating live_feed_delete() holds ...List objects with both fields None while the spec told them that class only comes from historical_results(), which is the worst combination: an ambiguous value plus a mental model that resolves it wrongly. Corrected in specs/02, the three-state table in specs/05, and both copies of the resources.py comment. 195 tests pass.
|
Review — checked against Verified clean:
1. assert result.json is content
...
assert set(content) <= set(result.json), "parsing must not drop keys from .json"Given the identity assertion two lines above, this reduces to Separately, nothing here tests the property the test is named for: because Fix both at once: raw = _content(cls, matched_strings=_STRINGS, matched_strings_dropped=19)
content = copy.deepcopy(raw)
result = cls(content)
assert content == raw, "parsing must not mutate the payload it was handed"
assert set(raw) <= set(result.json), "parsing must not drop keys from .json"That makes both assertions falsifiable, and it survives a future change that defensively copies 2. Minor: the non-null
Neither blocks merge; (1) is a two-line change. |
The previous commit replaced a tautological assertion and moved the tautology rather than removing it: `assert result.json is content` followed by `set(content) <= set(result.json)` reduces to `set(content) <= set(content)`. Nothing checked the property the test is named for either, since self.json keeps a live reference to the dict that was passed in. Now compared against an independent deepcopy, so "parsing did not mutate the payload" and "parsing did not drop keys" can both actually fail. It also survives a future change that defensively copies content, which the identity assertion would have failed for the wrong reason. Also records that the non-null matched_strings_dropped path is not pinned against a live server. The live tests assert only the None arm -- correctly, since the per-test rule withholds nothing -- so the two strongest claims the contract makes about the field rest on hand-written dicts. Producing an over-budget match on the e2e stack is disproportionate to what it would pin, so this is recorded in 99-open-questions.md rather than tested, alongside the historical-pair gap. 195 tests pass.
|
Reviewed against The parsing change itself is clean and matches the specs: Three things: 1. Merge ordering — this will red the e2e job if it lands first (blocking).
assert result.matched_strings, ...
assert 'matched_strings_dropped' in result.json, ...
assert set(result.matched_strings[0]) == {'offset', 'identifier', 'length', 'data', 'truncated'}The cassettes were recorded against the unmerged 2. The list-row assertion is the exact weakness the PR argues against, two lines above it. assert my_results[0].matched_strings is None, \
'list rows do not carry the evidence -- it is a per-row blob fetch'The next comment reads " 3.
|
…icated comments Two assertions could not fail: - The list-row check read `my_results[0].matched_strings is None`, two lines above a comment arguing that `is None` cannot distinguish a served null from an absent key -- which is why the count below it reads .json. The same argument applies here and specs/05 makes the stronger claim (an explicit null), so it now reads .json too. - test_populated_list_is_passed_through_verbatim passed the module-level _STRINGS into the content dict, which stores it BY REFERENCE, so the assertions compared the object with itself and no in-place reshape could fail them. Deep-copied, the same fix test_parsing_does_not_mutate_the_raw_json already needed. Also trims the two resources.py comment blocks, which restated the contract spec nearly verbatim at both call sites. Three copies of one argument is three places to drift, and several commits in this PR were spent reconciling exactly that. The decision and a pointer are enough; specs/05 owns the reasoning. 195 tests pass.
|
Review The code change itself is clean: four Two things worth acting on. 1. Spec drift — the "fourth cause" only landed on one of the two fields
But the three prose enumerations written before the delete-response commit still say three, and they are the ones a parser author reads first:
Given this PR spent three commits reconciling exactly this, worth finishing: make all four enumerations read the same four causes, or drop the enumerations from the summaries and point at the one table in 2. Merge ordering against the server PR
Notes (no action)
|
SummaryGives a hunt hit its yara evidence: the analyzer now scans with Severity: 0 HIGH · 5 MODERATE · 8 LOW. Prior feedback: 35 checked · 8 open.
Fixes are proposed, not applied; nothing was run; no independent fix review in this run. Cross-repo coordination
Merge order: analyzer producer → artifact-index#1949 → polyswarm-api#322 → polyswarm-cli#267 (§14: the API is the contract, and the analyzer produces the bytes). This PR is the only mergeable member of the four, and it must merge third — the path of least resistance is the wrong order. That is F4.
Coherence: F5's fix here is a Gaps: Findings (round 1)Every finding below is work for this change set; each entry's [MODERATE] F4. The merge-order instructions never name the analyzer that produces the evidenceWhat happens: merging this PR before the analyzer change reddens
Lands in: polyswarm-api. Also touches: artifact-index (its PR body). Proposed fix (untested): restate [MODERATE] F5. The CLI's two conflicting files are exactly the ones
|
| Status | Raised | The ask | Disposition |
|---|---|---|---|
| not addressed | round 7 | make all four None-cause enumerations agree |
→ F10 |
| open — not a defect | round 5 | merge ordering against the server PR | → F4, now with the analyzer added as the missing third dependency |
| open — not a defect | rounds 2–7 | the head ref carries an internal ticket code, so a default merge-commit subject would land it in this public repo's history. Commits, title and body are all clean. Not fixable in the diff — squash-merge with a hand-written subject, or rename the branch first. | — |
Everything else from the seven earlier rounds is addressed, and I re-derived the ones worth re-checking: the cassettes are genuinely re-recorded and carry a populated detail row plus explicit nulls on the list and delete rows, so all four live assertions are load-bearing; both raw-json tests are falsifiable after the deep-copy change; the …List classes really do parse delete responses; and 99-open-questions.md scopes the two unmeasured arms honestly rather than asserting them.
Standards conformity
Set-level
- §14 delivery order — clean, and unusually so. One externally-facing capability, not a layer: producer, API, SDK and CLI in one change set, exercised through this SDK against a running stack before the interface work. This is the case §14 describes.
- Rule 6 name identity — clean. All four branches are byte-identical, and
e2e's_tag_friendly_branch_namelowercases exactly likeCI_COMMIT_REF_SLUG, so branch CI genuinely resolves and exercises all four together today. That is also why F4's hazard is invisible until the first merge. ## Requireslinkage — ✗ → F4.
Change-level (this repo): §16 cross-repo dependencies → F5 (the version-declaration half).
Project-level (non-clean rows only)
| § | Evidence | |
|---|---|---|
| 16 | ✗ | the version declaration is absent from this branch; develop already has it (F5) |
polyswarm-api / polyswarm-cli — Clean: §6, §10, §15. Not applicable: §3–§5, §7–§9, §11–§13.
I did not verify §2 beyond the release-job wiring F4 needed.
Checked and refuted, so not reported as a finding: that a red develop e2e here fans out and breaks every downstream repo's pipeline. It is the other way round — that job gates the release stage, so :latest freezes rather than breaking, and siblings keep passing against a stale SDK. F4 carries the corrected consequence, which is the quieter and more durable failure.
|
Reviewed against 1. The per-entry key assertion is exact-set equality, which forbids the additive server growth this PR is built around.
assert set(result.matched_strings[0]) == {
'offset', 'identifier', 'length', 'data', 'truncated'}The whole justification for assert {'offset', 'identifier', 'length', 'data', 'truncated'} <= set(result.matched_strings[0])2. Merge order is a hard gate, not a note. The PR body says it, but it is worth pinning as a merge condition rather than context: 3. Version bump — one thing to confirm, not necessarily to change. No 4. Minor: one unit assertion is still a restatement.
Also inconsistent, though harmless: the live tests membership-check one field ( Nothing else: |
sbneto
left a comment
sbneto
left a comment
There was a problem hiding this comment.
Reviewed. 1 LOW open — the per-entry key assertion in test_live / test_async_live is exact set equality, where this suite uses a subset check at five other sites; a subset catches renames and removals identically and would not red develop on a purely additive server key. Nothing fires today: the producer emits exactly those five keys. Verified this round: the cassettes are genuinely re-recorded, the four-cause None enumeration now reads the same in every place, and the Requires section states the merge order and its consequence.
02-resources.md states that None reaches a caller from four places, not three -- and then two summaries in that same file enumerate three, as does the matched_strings_dropped paragraph in 05. The file contradicted itself within one section, which is the defect an earlier commit was written to fix. Rather than restate the list a fourth time, the summaries now defer to the one table that owns it. A pointer cannot drift out of step with what it points at.
The live assertions are the only thing distinguishing served evidence from a served null -- the pure-unit tier exercises dict.get and passes identically whether or not the server ever grew the field. So the assertion stays exactly as strict. Only the message widens. The value is null unless the producer that emits the strings is deployed, so the likeliest cause of this failing is an image that predates it -- while everything else about the stack looks healthy, which makes the failure read as a server bug. The message now names the real cause.
ef3e02c to
1d2dab9
Compare
|
Review The code change itself is clean: 10 lines, 1. Spec drift — "the other three causes" contradicts the four-cause table it points at
But the table in This is the same defect commit 2. Test coverage — the newly-documented fourth The four-cause table is new contract in this PR, and two of its arms have no assertion in either tier:
Both are already in the recorded cassettes: Notes, no action
|
2 participants
TL;DR
LiveHuntResult.matched_strings/HistoricalHuntResult.matched_strings— the yara strings behind a hunt hit, so a caller can see why a rule fired rather than only which one did.matched_strings_dropped— a sibling count of strings the server withheld past a size limit, so a truncated list can't be read as complete.…Listsubclasses inherit both, so all four hunt-result classes are covered..get(), never a subscript: both keys are additive, so a server predating them omits them and a subscript would raise on every result.Three states, and consumers must not collapse them
Nonenullrather than fetch a blob per row). We don't know, not there was nothing.[]strings:section, all-privatestrings, or a match on absence.[…]Each entry is
{'offset', 'identifier', 'length', 'data', 'truncated'}.datais kept exactly as yara rendered it (hex strings as byte pairs, text as ASCII with escapes) because only yara knows which applies.lengthis the stored length, capped server-side, andtruncatedmeans "there was more than this" rather than an exact size.Why it's a lower bound
any of themprints only the strings that hit;privatestrings never appear; and the server's byte budget may withhold the tail — which is whatmatched_strings_droppedreports.Note this list previously included "fast-scan reports one offset per string". That is false and has been removed from the spec: measured against yara 4.5.2,
-fcollapses repeats of a single string only when the rule's condition doesn't need them ($aover ten hits reports 1,#a > 3reports all 10), and it never limits how many distinct strings a rule reports.matched_strings_droppedA sibling
int/None, not a key insidematched_strings— that stays a plain list, so this is additive to the shape rather than a change to it. It exists because a truncated list is otherwise indistinguishable from a complete one: a caller reading twelve entries would conclude the rule hit twelve times when it hit thirty-one. It can never accompany an empty list, since a match's first string is never withheld.Evidence rides on the detail routes only
live_feed()andhistorical_results()page over list endpoints and always yieldNonefor both fields — reading the payload is a per-row blob fetch on the server. Fetch a single result (live_result(id)/historical_result(id)) to get the strings.Notes for review
aio/api.pyor the generated sync mirror — this is entirely inresources.py, which is hand-written, so the codegen-staleness check is not involved.pyproject.tomlversion bump, per the gitflow rules.test_live/test_async_liveassert the detail route carries evidence, that list rows don't, and that the server actually servesmatched_strings_dropped— that last one asserts onresult.jsonrather than the attribute, becauseis Nonecan't tell a served null from an absent key. Cassettes re-recorded delete-driven against a stack running the matching server branch.Nonewhile[]stays[].Requires
This is third of four in one change set:
stringseverything else carriesrecorded against it
Merging ahead of 1 and 2 leaves
test_liveasserting a field nothing serves. That reddensdevelop's e2e, which gates the release stage — so:lateststops being promoted andevery downstream pipeline keeps testing a stale SDK, silently.