Render matched strings on hunt results - #267
Conversation
Shows the yara strings behind a hit, between Tags and Download Url. None is deliberately not rendered as silence. The feature exists to answer "why did this rule hit", and an absent line answers nothing -- it is indistinguishable from a rule that matched with nothing to show. Each of the three states gets a line saying which it is. The None line names its causes rather than the command that would carry the evidence. A `try polyswarm live result <id>` hint is tempting, since the dominant None case is the list route omitting the strings rather than fetching a blob per row. But this method renders both routes -- live feed loops over it -- and nothing on the resource tells them apart, so on a detail fetch the hint would say to re-run the command you just ran. Getting that right means threading a route flag into a new parameter on every BaseOutput implementation, which this does not earn. The four text cassette expectations are regenerated through click_vcr's own record path; the recorded HTTP is untouched. They predate the field, so they pin only the None line.
Style 3 -- the formatter driven directly with constructed resources, since the question is which line a field value produces, not command behaviour. The cassettes above reach only the None line, so they are not a substitute. Asserts the empty and absent lines stay different from each other, that truncation is marked only where it applies, and that the block sits between Tags and Download Url rather than being appended last. The absent-line assertions match substrings rather than the whole sentence, so rewording the prose does not turn this into a spelling check.
The spec asks for a resource's rendering rules once they are non-obvious or contested, and this one is both: it forbids rendering the absent state as silence, and it explains why the absent line names causes instead of a command to run, so the hint is not helpfully reintroduced later.
The absent state used to render a line explaining why the evidence was missing. On a list route that is a permanent false alarm: those endpoints never look for the strings, so every row of every page carried an explanation for a lookup that was never attempted. Absent now renders nothing at all. The case worth explaining survives untouched -- an empty list still says the rule matched with no byte evidence, and that one only ever reaches a detail route, so its line is never noise. This method renders both routes (the feed loops over it) and nothing on the resource distinguishes them, so the choice is per-state rather than per-route. Route awareness would mean threading a flag from the command layer into a new parameter on every BaseOutput implementation, for a line that is unwanted on one route and near-vestigial on the other: once the analyzer always reports strings, absent on a detail route means only that the result predates the feature. The absent-state test inverts rather than disappears -- it now asserts no line is emitted -- so this cannot be quietly undone. The four text cassette expectations are regenerated through click_vcr's own record path; the recorded HTTP is untouched.
Comment text only -- the parsed AST is identical before and after. The largest cut of the pass, because most of what was there argued against a design that is no longer in the code: a hint naming the command that would carry the evidence, which was tried, found to be circular on the detail route, and dropped. The reasoning for the choice that was actually made -- absent renders nothing, empty keeps its line -- is what remains.
|
Review Base ( 1.
The PR body-s justification does not hold:
Two ways out, and the spec effectively picks one:
Whichever you take, the new §Matched strings spec section should say so. It is currently silent on the SDK-version question while the section directly above it documents its own in detail — a reader cannot tell whether the omission is a decision or an oversight. (CI will not catch this: the SDK branch name matches this one exactly, so 2. Pre-existing spec drift you will land on while fixing the above
3. Minor — inconsistent entry-key access
Nothing else: placement, the three-state rendering, the |
`result.matched_strings` was a bare attribute read while the dependency pin still admits SDKs that predate the attribute, so on any released SDK in range it raises AttributeError -- and not only on the new output. All six text-mode hunt commands funnel through these two formatter methods, so `live feed`, `live result`, `live results-delete` and the three historical equivalents would all break. Now read with getattr, matching the defence the known-good attributes in this same file have used since they landed, and for the same reason. A missing attribute lands on the silent None branch, which is also the honest reading: an SDK that cannot see the field does not know. CI could not have caught this. The paired SDK branch shares this branch's name, so the archive install resolves to it and the feature-branch SDK gets installed -- green here means the paired-PR mechanism works, not that the pin is right. Nor could the existing tests: they build resources from the installed SDK, so a bare read passes all of them. The new test deletes the attribute to stand in for an older SDK and is the only guard; it fails with AttributeError against the previous code. Also subscripts `truncated` like the other four keys. Mixing a subscript for `length` with `.get()` for `truncated` defended against a shape the contract says cannot occur while a genuinely partial entry still raised two lines earlier. Unrelated and pre-existing: the specs still named `>=4.2.0` as the dependency floor, which moved to 4.3.0 in cdb7926 without a spec change. Corrected, with the provenance recorded -- the per-behaviour writeup below it is still the 4.2.0 one, accurate about why 4.2.0 was needed but no longer the binding constraint.
|
Reviewed against 1.
2. The §Version pin paragraph still reads "For the current floor both were read from `origin/develop`: `version = "4.2.0"` and `version = '4.2.0'`, no suffix", while §Current floor (line 77) now says 4.3.0. The new note scopes its disclaimer to "the per-behaviour writeup below" — this sentence is above the heading, so it isn't covered. Reword to "For the 4.2.0 floor…". 3. The pin has no recorded follow-up. Not bumping now is correct — §Version pin's precondition is that the version is on PyPI, and the paired SDK isn't released. But the invariant is "Floor it at the lowest SDK version exposing every method/behaviour the CLI relies on", and on a 4.3.0 install 4.
Minor, no action needed unless you disagree: the PR body says the attribute is "read with |
Renders a final line inside the block when the server reports a withheld count:
… 19 more not shown (result size limit)
Yellow rather than white -- it is the one line here reporting something the
platform did not send. Omitted entirely when nothing was withheld.
Not cosmetic. Without it a truncated list reads as the whole truth and a user
concludes their rule hit twice when it hit twenty-one times, which is the same
wrong-inference class the three-state contract already exists to prevent, one
level down.
Read with getattr for the same reason as `matched_strings` itself: the
dependency floor admits SDKs predating the field, and a bare attribute read
would AttributeError on every text-mode hunt command rather than just the new
output. A test deletes the attribute to stand in for an older SDK.
142 tests pass.
ReviewMostly clean against 1.
|
…d count The empty branch returned early, before the dropped check. So an empty list arriving with a non-zero count printed "the rule matched without byte evidence" and silently dropped the number -- a confident false statement about the rule, and the precise wrong inference the count was added to prevent. That combination should be unreachable: the analyzer keeps a match's first string, so [] implies dropped == 0. But the renderer was trusting an invariant owned by another repo while making a positive claim, in the same function that defends with getattr against shapes it does not control. Now it reports what is certain -- "none shown (N withheld, result size limit)" -- and asserts nothing about why the rule fired. The normal empty case is unchanged. Also records in 05-sdk-contract.md that the floor does not yet cover matched_strings / matched_strings_dropped. Not bumping it is right -- the SDK carrying them is not on PyPI, so neither precondition in the version-pin section is met -- but with no version written down nothing would later prompt the decision of whether the graceful degradation is still wanted. 146 tests pass.
|
Reviewed against Gitflow is clean: base is 1. The two "older SDK" tests error on exactly the SDKs they model.
That configuration is inside the declared pin: Cheapest fix that keeps the guard honest: module-level 2. Per-entry keys are hard-subscripted while everything above them is defended.
The inline comment says this is deliberate ("fail loudly on a partial entry rather than render half-right"), which is a defensible call — but 3.
4. Minor — first non-ASCII in text output.
5. Minor — the deferred floor decision has no home.
|
Three things, the first two of which were broken rather than merely untidy. The two "older SDK" tests errored on exactly the SDKs they modelled. Both did `del result.matched_strings[_dropped]`, and the attribute is set in __init__ -- so on an SDK that never set it, `del` raises AttributeError and the test errors instead of passing. That configuration is inside the declared pin: 4.3.0 is released without these fields, so `pip install .[tests] && pytest` against the floor errored on those two and failed a further ten. Now popped from __dict__, which works either way, with a module-level skip so a run against a floor SDK skips honestly rather than failing wholesale. CI was green throughout only because the paired SDK branch shares this branch's name. `data` is now sanitised before rendering. It is the one field in a hunt result derived from the sample, so it is attacker-controlled end to end, and it was interpolated into terminal output unescaped. yara escapes non-printables and the analyzer preserves that rendering, so the substitution is a no-op on anything valid -- it exists because that guarantee lives in another repo, and a raw CSI sequence reaching a terminal would repaint or clear an analyst's screen. Trusting an upstream promise is not the same as holding one. The em dash and ellipsis were the first non-ASCII characters TextOutput emitted; under a C/POSIX locale stdout degrades them to '?'. Replaced with ASCII and pinned by a test. Also records in specs/03 why the attribute is read with getattr while the keys inside an entry are subscripted -- they answer different questions, version skew versus a producer violating its own contract -- since side by side they read as an inconsistency. And 99-open-questions.md now carries the deferred floor decision, including that raising the floor is the trigger to drop both the getattr defence and the new skip guard. 152 tests pass.
|
Reviewed against 1.
Related: the new spec bullet "ASCII only. 2. The module-level skip disables the two older-SDK tests in exactly the configuration they model (
3. The spec's own examples contradict the ASCII rule it introduces ( The Minor: the |
…guard Two defects in the previous commit, both of which left a stated protection not actually in place. The sanitiser blacklisted [\x00-\x1f\x7f] -- C0 and DEL -- and let U+0080-U+009F through. U+009B IS the 8-bit CSI, honoured by xterm and VTE in UTF-8 mode, so `\x9b2J` still cleared the terminal: a hole in the precise byte the filter was written to block. The threat model is that the upstream escaping guarantee lives in another repo and may slip, and bytes that slip do not stay conveniently in the low range. Now a printable-ASCII whitelist, which also makes the ASCII-only rule true of this field rather than only of the literals around it. The test grows a \x9b case and a non-ASCII one. The module-level skipif took the two older-SDK tests out of the only install where they guard anything. They build a resource and pop the attribute off, so they pass fine on a floor SDK -- and on that SDK a regression to a bare attribute read would have skipped green and failed only in the field. The previous commit called one of them "the only guard" while disabling it there. The mark is now applied per-test, to the fifteen that genuinely need the fields. Also corrects the ASCII claim in specs/03, which asserted TextOutput emits no non-ASCII: server-supplied rule_name and tags pass through unfiltered and are outside it. And the spec's example blocks showed an em dash and an ellipsis while the code emits -- and ..., which matters because they read as literal expected output in a section that warns non-ASCII degrades under a C locale. Minor: two getattr comments pointed at the known-good reads as "below"; they are above. 152 tests pass.
|
Reviewed against Gitflow is clean: base is Four things to action, one of them substantive. 1. The load-bearing premise is unverified for the historical list route. The whole silent-
The verification note reads ambiguously on precisely this point — "the empty state across 25 historical results from a structural rule". Twenty-five results sounds like the list route. Please say which route that was. If it was the list route returning 2.
It does not, and the final commit deliberately removed one — the file comment reads "Applied per-test, NOT as a module-level pytestmark. The two older-SDK tests below build a resource and pop the attribute off, so they pass on a floor SDK -- and that is the ONE install where they guard anything." Worth fixing rather than shrugging off as a typo, because that same section is the instruction sheet for the later floor decision ("drop both the 3. Two paragraphs above the new heading:
"The current floor" is 4.3.0 as of the heading directly below it. Since the PR is already correcting the 4.2.0/4.3.0 drift in this section, fold this sentence in too — either re-read the pair off 4.
Everything else checks out — the |
`identifier` was interpolated raw inside the matched-strings block -- the same block test_output_is_ascii_only asserts is ASCII, which passed only because the fixture uses ASCII identifiers. Routed through _safe_data alongside `data`, so the ASCII rule is now true of the whole block rather than needing another exception clause. yara's identifier grammar makes a hostile value close to impossible; this costs nothing and removes the caveat. Three spec statements had gone stale against changes in this same PR: - 99-open-questions said the test module "carries a module-level skip". The previous commit deliberately removed exactly that, and this section is the instruction sheet for the later floor decision -- a reader following it would hunt for a pytestmark that is not there and might reintroduce it. - 05-sdk-contract's version-pin guidance still used 4.2.0 as "the current floor" two paragraphs above the heading this PR corrected to 4.3.0. Reframed as the worked example it has become, since the check is the point rather than the number. - The ASCII bullet excluded rule_name and tags but not identifier. Also records that the silent-None branch depends on list routes sending null, and that this is measured rather than assumed: artifact-index's test_list_serializers_never_touch_storage pins present-and-null on BOTH hunt pairs against rows that carry evidence. This repo cannot verify it, so the spec now names what it relies on. 152 tests pass.
|
Reviewed against 1. Test coverage — the yellow of both dropped-count lines is pinned by nothing.
This is the exact gap
2. Spec drift inside this PR — the state table contradicts the section below it.
3. Minor — the branch name puts a ticket ID in public history.
|
specs/03 states the colour as a deliberate signal -- it is the one line in the block reporting something the platform did not send -- but every assertion in this module ran through TextOutput(color=False) and click.unstyle, so both lines could have regressed to white with the suite staying green. Adds a _render_styled helper mirroring the established pattern in known_good_field_test.py, and three cases: each withheld line is yellow, and the ordinary block is not -- without that last one the first two would pass on a formatter that painted everything. Also splits the `[]` row of the state table, which gave a single unconditional rendering while the section below it documents that a non-zero count overrides that line. The table is the part written to be consulted, and it was answering the case immediately below it incorrectly. 158 tests pass.
|
Review Formatter change itself is correct and matches the specs it adds: three-state handled, 1.
That header only renders if the installed SDK parsed Make it floor-independent the same way the sibling is, by injecting rather than relying on the SDK to parse: build the resource from 2. Branch name carries a ticket ID onto a public PR
Minor The |
…cated comments test_older_sdk_without_the_dropped_attribute_does_not_raise was left unmarked so it would run on a floor SDK -- the only install where it guards anything -- but it passed matched_strings through the content dict and then asserted the block renders. That requires the SDK to have PARSED matched_strings, which 4.3.0 does not, so it failed at the floor: the inverse of the intent, and it made the 99-open-questions claim false for one of the two tests. The field is now injected into __dict__ instead, so the test depends on nothing the floor omits. Verified against a stand-in resource class that sets neither field. That is the third correction to this pair -- del raising on old SDKs, then a module-level skip removing them from old SDKs, now a dependency on parsing. The common cause each time was a test that models an older SDK while quietly relying on a newer one. Also trims the formatter comments, which restated specs/03 nearly verbatim -- the CSI rationale, the three-state reasoning, the subscript-vs-getattr argument. Two copies of one argument is two places to keep in sync, and this PR already spent commits reconciling prose that drifted between them. The decisions stay in the code; the reasoning stays in the spec, which owns it. 158 tests pass.
|
Reviewed against AGENTS.md, Verified as claimed: base is Three things. 1. From
There are more than two server-supplied fields in that block.
Cheapest fix, and consistent with the stance already in the file (subscript strictly, because a malformed entry is a producer breaking its own contract and crashing is correct): give both numbers a Related, same lines: 2. Nothing drives the populated block through the command tree —
The letter of that is satisfied by the existing 3. The branch name carries an internal ticket code — AGENTS.md: Do not reference ticket IDs or internal project codes in commit messages, PR titles, or PR descriptions. This repo is public. The commits, title and body are all clean — but the default merge-commit subject is Nothing blocking on the spec side. |
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 merges last. The only member that can merge today is the SDK, which must merge third — the path of least resistance is the wrong order.
Coherence: F5's fix is a Gaps: Findings (round 1)Every finding below is work for this change set; each entry's [MODERATE] F5. This PR's two conflicting files are exactly the ones
|
| Status | Raised | The ask | Disposition |
|---|---|---|---|
| not addressed | round 9 | coerce the server-supplied numbers in the ASCII block | → F12 |
| not addressed | round 9 | drive the populated block through the command tree | → F13 |
| partially addressed | round 8 | trim in-code comments that restate the spec | trimmed once; four duplicated sites remain across the set → F7 |
| open — not a defect | rounds 6–9 | 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. | — |
Two earlier points are worth recording as resolved rather than merely closed. Round 6 asked which route the "25 historical results" verification actually used, because if the historical list route returned [] per row, every row of a large hunt would carry the loud "matched without byte evidence" line. It does not — the server's ScanResultListSerializer renders a literal None for both fields, and its new test pins that for both hunt pairs. Your specs/03 premise holds; this repo simply could not verify it alone. And the round-8 floor-independence fix to test_older_sdk_without_the_dropped_attribute_does_not_raise is correct as far as it goes — F5 removes the test entirely, for a different reason.
One point I am reversing rather than confirming: round 1 asked for the getattr defence and every round since endorsed it. It was the right call against specs/03's known-good precedent, and it is the wrong call against §16, which postdates that advice by three days and which develop has already adopted in these exact files. See F5.
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 the SDK against a running stack before this interface work.
- 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 — which is also what makes the SDK archive resolve to the paired branch here. That is why F4's hazard is invisible until the first merge. ## Requireslinkage — ✗ → F4.
Change-level (this repo): §16 cross-repo dependencies → F5 · §15 comment the fact, specify the design → F7.
Project-level (non-clean rows only)
| § | Evidence | |
|---|---|---|
| 16 | ✗ | runtime probes plus 18 shape-keyed test skips (F5) |
polyswarm-api / polyswarm-cli — Clean: §6, §10, §15. Not applicable: §3–§5, §7–§9, §11–§13.
I did not verify §2 beyond the SDK-archive resolution F4 and F5 needed.
Checked and refuted, so not reported as a finding: that raising the floor to an unreleased 4.4.0 would break pip install polyswarm for real users. Neither repo publishes from develop, so the floor is inert until a develop → master, and develop has already carried it since before this branch's last commit.
|
Reviewed against Gitflow / floor check out too, and I verified rather than assumed: base is Two things worth fixing: 1. 2. Doc nits in the new
|
sbneto
left a comment
sbneto
left a comment
There was a problem hiding this comment.
Reviewed. 3 LOW open — historical result still reaches the formatter in no test, the silence test can pass on a usage error because the helper discards the exit code, and the new formatter spec section carries three inaccuracies (a cross-repo doc reference that resolves nowhere here, a miscount, and a claim that one line reports a withheld count when two do). Verified this round: the runtime probes are gone in favour of direct attribute reads, the floor is >=4.4.0, the numeric fields carry :d, and the sibling-test citation is corrected.
The dependency floor is now 4.4.0, which is the release that parses both fields, so pip refuses the install the getattr defence guarded against. specs/05-project-standards.md §16 is explicit that a consumer's need for a library surface is a version requirement, checked once by the packaging tool and never re-derived at runtime -- no attribute probing, and no per-test skip guards keyed on the dependency's shape. So the four getattr reads become plain attribute access, and the suite loses _sdk_carries_the_fields, its 18 skip decorators and the two tests that popped an attribute off a resource to model an install the floor now forbids. Their real job survives in test_absent_renders_nothing, which renders from a fixture that sets no key, so the None branch stays covered by a real resource rather than a mutilated one. 36 tests, none skipped. The entry keys stay subscripted, deliberately: a partial entry is a producer violating its contract, not version skew, and the two are answering different questions. Also in this diff, all small: - length and dropped gain an integer format spec, so every server-supplied value in a block whose spec claims ASCII is pinned by construction -- strings by _safe_data, numbers by :x / :d. offset was safe only by accident of already having one. - The server-side test cited as pinning the silent-None design was renamed on this same branch; the citation now names the test that exists. - The open question about this floor is resolved, so it is removed rather than left describing a probe that is gone.
specs/04-testing.md is explicit that a command whose rendering is covered by the formatter-only tier "still needs at least one CliRunner test proving the command reaches the formatter". This adds it. What it buys, measured rather than assumed: rewiring the command to render through the wrong formatter method leaves all 36 assertions in the sibling module passing -- they invoke the formatter themselves and cannot see the command tree above it -- and fails this module. The cassettes cannot catch it either, since they predate the field and every result they render takes the silent None branch. Uses real SDK resources rather than mocks: the formatter reads polyscore and the rest of the row, so a bare stub dies before reaching the block under test.
54ef680 to
28fe88c
Compare
Review — against
|
2 participants
TL;DR
polyswarm live result <id>andpolyswarm historical result <id>now show the yara strings behind a match, betweenRule:/Tags:andDownload Url:..json.specs/03-formatters.mdrecords the rendering rule; tests drive the formatter directly.Requires
matched_stringsattribute this renders.This PR must not merge before that one is on the SDK's
develop: CI installs the SDK from$CI_COMMIT_BRANCH.zip || develop.zip, so once the branches diverge the fallback would be an SDK without the attribute.What it looks like
Why absent renders nothing but empty does not
The two look like the same case and are not:
live feedrenders through this same method, with nothing on the resource to tell the routes apart.Matched Strings: none — the rule matched without byte evidence (a structural or negative match, or private strings). This only ever reaches a detail route, where it is a real answer to "why did this hit".An earlier revision named the command that would carry the evidence. It was dropped: the same method renders both routes, so on a detail fetch the hint told you to re-run the command you just ran, and fixing that properly would mean a new parameter on every
BaseOutputimplementation.truncatedis deliberately not rendered as a byte count — the stored length is capped server-side, so it means "there was more than this".Notes for review
.clickcassette expectations net out unchanged againstdevelop— the block adds no output to any path they exercise. The diff is pure addition.specs/04-testing.mdStyle 3): the cassettes predate the field, so every result they render takes the silent branch. They pin that no stray line appears, and nothing more.pyproject.tomlversion bump; the SDK pin is unchanged, since the new attribute is additive and read with.get().Verification
132 tests pass. Verified end to end against a locally running stack with matching server and SDK branches: populated strings on a live-hunt result, and the empty state across 25 historical results from a structural rule.