Release 4.5.0 - #271
Release 4.5.0#271
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.
`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.
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.
…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.
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.
…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.
`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.
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.
…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.
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.
Render matched strings on hunt results
Forwards `ruleset_list(sort='active_first')` — the hunt page's order, rulesets with a running live hunt first (as recorded by the server's live-hunt link, the same one Livescan Id renders from), newest first within each block. Server-side like the filters: the list is keyset-paginated, so a local sort would only ever reorder one page. The option is a closed click.Choice (hyphenated CLI spelling, underscored server token) and is forwarded only when given, so the unsorted default request is unchanged. The autospec tests double as the signature check against the installed SDK, alone and combined with the filters.
…ort=) The floor names the SDK version that adds the keyword `rules list --sort` forwards (specs/05 §Current floor follows the pin). Mergeable once the SDK's develop declares 4.5.0; releasable once that version is on PyPI — the SDK releases first. CI resolves the SDK from source by branch name, falling back to develop, so the paired SDK branch must carry the identical name.
…n Live Hunt Id The help text said the order came from "the same one Live Hunt Id renders from". The server ranks on the raw link and renders the id under a stricter predicate, so a legacy row whose hunt was stopped without clearing the link leads the list with an empty Live Hunt Id. Someone reading the list top-down for what is running would have stopped at that row.
…y Live Hunt Id `rules list` walks every page and exposes no limit, which makes it exactly the consumer the SDK puts the dedupe obligation on. Under --sort active-first the ordering key is the live-hunt link, so a ruleset whose hunt stops between two page fetches drops below the cursor and the server serves it again: the run printed it twice and any script counting the output double-counted it. Rows are now emitted at most once per run. The dedupe is unconditional — the id is unique under either order and one set of ids costs nothing next to the rendered rows. The symmetric case cannot be repaired from here and is documented rather than hidden: a hunt STARTED mid-walk moves its row above the cursor and it never reaches this client until the next run. Two doc corrections in the same push. The --sort help promised that a stale-link row "leads the list with an empty Live Hunt Id", but the formatter gates the whole Live Hunt Id pair on a truthy value, so such a row prints no such line at all and reads as idle. And the commands spec called that row "stopped-but-unlinked", the inverse of the state it means: it is stopped and STILL linked, which is why it leads a list ranked on the link being present.
The two dedupe tests repeated the same row wholesale and compared rows that differed in every field, so an implementation keyed on the name — or on the rendered block — passed both. Ruleset names are not unique, so that variant would swallow a real row from any inventory listing. The new case is the one that separates them: two distinct rulesets sharing a name must both render. Re-keying the dedupe on the name fails it and nothing else.
…y so Round two of review. The dedupe keeps the first copy — the only choice a streaming printer has, since that copy is already on stdout when the second arrives — and the first copy carries the values from BEFORE the transition. So the one row the dedupe acts on prints the Live Hunt Id of a hunt that has already stopped. The help said to read the field rather than the position; under a mid-walk move neither is authoritative, and the help, the command docstring and the commands catalogue now say that instead of promising it. The dedupe test repeated a row wholesale, which is not the shape a re-serve takes: the two copies differ in exactly the field the sort ranks on, because that is why the row moved. It uses that shape now, and a new case pins first-wins as a decision — a last-wins rewrite fails it and nothing else. The invariant itself moved to specs/05 §Consuming the SDK correctly, beside the other SDK-consumption rules, where the next command that walks a mutably ordered endpoint will find it; the commands catalogue points at it.
…e duplicated prose Seven tests about sort forwarding and mid-walk dedupe had accumulated inside a class whose docstring promised only the zero-argument call, so nobody landing on the name would expect the dedupe pins to be there. They are their own class now, with a docstring that states both contracts it holds; the module docstring enumerates the new one alongside the others. The same ~150 words about stale Live Hunt Id values were restated in the help, the commands catalogue and the SDK-contract spec. The help keeps them — the user reading --help is the one who acts on a stale field — and the catalogue cell, the copy most likely to drift, is back to the one-line claim plus its existing pointer.
The inverse of --favorites-only, which the server refuses to combine with it. It exists for a client that renders the favorites as their own list: leaving them in the paginated list too makes a page repeat a row or come back short. A False flag is not a filter here either, so an unflagged invocation sends nothing new.
feat(rules): list --sort active-first
|
Reviewed against 1. Release PR, but
|
Release bump for 4.5.0 — `rules list --sort active-first` and `--exclude-favorites`. The SDK floor `polyswarm_api>=4.5.0` is already pinned; polyswarm-api Release 4.5.0 must be on PyPI before this repo's develop → master merge.
vhmartinezm
changed the title
Bump version: 4.4.0 → 4.5.0
Review — release 4.5.0Gitflow is right for a release PR ( 1.
|
3 participants
Release — 4.5.0
polyswarm rules list --sort active-firstand--exclude-favorites— the CLI half of the hunt page's active-first ruleset order. Without the flags nothing changes.Requires — merge order is hard
Bump version: 4.4.0 → 4.5.0) must merge into develop first. Until it does, this PR promotes 4.4.0 a second time.polyswarm_api>=4.5.0; until 4.5.0 is on the index, an install from PyPI cannot satisfy it.Behaviour change to an existing invocation
rules listwalks every page and exposes no limit, which makes it the consumer the SDK puts the dedupe obligation on. Rows are now emitted at most once per run, keyed on id — unconditionally, including under the default order where it is inert.Two costs, documented in the
--sorthelp, the command docstring andspecs/05-sdk-contract.md: