From 7651ff6f0470a3ec62670a825e288babb3083f66 Mon Sep 17 00:00:00 2001 From: Samuel Date: Tue, 28 Jul 2026 17:13:52 -0300 Subject: [PATCH 01/13] fix: key known-goodness on the instance state, not on the feed list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_known_good` was `state == 'KNOWN_GOOD' or bool(known_good_sources)`, and the Detections line it drives replaces the engine verdicts with "this artifact is a known-good binary … it is not scanned". That was harmless only because the server emitted the feed list exclusively for instances already in the known-good state. It no longer does: the list is now present whenever the sha256 matches a known-good record, including on a fully scanned instance with real detections and a PolyScore, so the fallback would have reported such a sample as never scanned. The state is the single reliable signal for "known-good, bytes withheld", so it alone decides; the feed list only shapes the message. Keeps the defensive getattr for older SDKs — one consequence, now documented: an SDK without `.state` renders a known-good instance as an ordinary one rather than mislabelling a scanned one. --- specs/03-formatters.md | 29 ++++++++------ src/polyswarm/formatters/text.py | 13 ++++--- tests/known_good_field_test.py | 65 +++++++++++++++++++++----------- 3 files changed, 67 insertions(+), 40 deletions(-) diff --git a/specs/03-formatters.md b/specs/03-formatters.md index 062aa63..7e57ea6 100644 --- a/specs/03-formatters.md +++ b/specs/03-formatters.md @@ -45,19 +45,26 @@ Formatters don't set exit codes — that's `ExceptionHandlingGroup`'s job, by ex **green** "Known Good" signal (a stronger benign signal than "clean") instead of the misleading "no engines responded — rescan now" a window-closed/no-assertion instance would otherwise get. It is gated by a single -`is_known_good = (state == 'KNOWN_GOOD') or known_good_sources` flag: +`is_known_good = (state == 'KNOWN_GOOD')` flag: - **`state == 'KNOWN_GOOD'`** (the SDK's `ArtifactInstance.state`, the friendly - bounty-state name) is the reliable signal — it fires even for a scan-bypassed - instance that carries **no** feed metadata. + bounty-state name) is **the** signal, and the only one — it is the server's single + reliable statement that this artifact is known-good and its bytes are withheld, and + it fires even for a scan-bypassed instance that carries **no** feed metadata. There + is deliberately no separate "bytes withheld" field to consult. - **`known_good_sources`** (the sorted flagging-feed names, from - `ArtifactInstance.known_good`) is the richer signal: when present, the - **Detections** line names the feeds ("…known-good binary (flagged by: …); it is - not scanned."); otherwise it reads "…is a known-good binary; it is not scanned." - It also keeps known-good rendering working against an older SDK that has the feed - list but not `.state`. + `ArtifactInstance.known_good`) only **shapes the message** for an instance already + known-good by state: when present, the **Detections** line names the feeds + ("…known-good binary (flagged by: …); it is not scanned."); otherwise it reads + "…is a known-good binary; it is not scanned." It must **never** decide + known-goodness — the server emits the feed list for *any* instance whose sha256 + matches a known-good record, including a fully scanned one with real + detections/PolyScore, so treating it as the signal would render a scanned artifact + as "known-good … not scanned". It is therefore read as `[]` unless `is_known_good`. The **Status** line reads "Known good" whenever `is_known_good`. Both attributes are -read with `getattr(..., None)` so a CLI on an older SDK (missing either field) -renders exactly as before. `JSONOutput` needs no change — it dumps the resource's -`.json`, which already carries the raw `state` and `known_good` keys. +read with `getattr(..., None)` so a CLI on an older SDK (missing either field) never +raises `AttributeError`; an SDK without `.state` simply never takes the known-good +branch, which is the safe fallback — the pre-known-good rendering. `JSONOutput` needs +no change — it dumps the resource's `.json`, which already carries the raw `state` and +`known_good` keys. diff --git a/src/polyswarm/formatters/text.py b/src/polyswarm/formatters/text.py index 9c3fddf..dca0589 100644 --- a/src/polyswarm/formatters/text.py +++ b/src/polyswarm/formatters/text.py @@ -79,12 +79,13 @@ def artifact_instance(self, instance, write=True, timeout=False): # Defensive getattr: these attributes ship in the paired SDK release, but a # CLI running against an older installed SDK won't have them (no AttributeError). - known_good_sources = getattr(instance, 'known_good_sources', None) or [] - # A known-good binary is signalled by the bounty state (KNOWN_GOOD) — reliable - # even for a scan-bypassed instance with no feed metadata. known_good_sources - # (the flagging feeds) is the richer signal when present, and also covers an - # older SDK that lacks .state but still carries the feed list. - is_known_good = getattr(instance, 'state', None) == 'KNOWN_GOOD' or bool(known_good_sources) + # The bounty state (KNOWN_GOOD) is the only reliable signal that this artifact is + # a known-good binary whose bytes are withheld — it alone decides. known_good_sources + # (the flagging feeds) is emitted for any instance whose sha256 matches a known-good + # record, including a fully scanned one carrying real detections, so it only shapes + # the message; reading it as the signal would render a scanned artifact "not scanned". + is_known_good = getattr(instance, 'state', None) == 'KNOWN_GOOD' + known_good_sources = (getattr(instance, 'known_good_sources', None) or []) if is_known_good else [] if is_known_good and not instance.failed: if known_good_sources: diff --git a/tests/known_good_field_test.py b/tests/known_good_field_test.py index f35f9b7..68bc6b3 100644 --- a/tests/known_good_field_test.py +++ b/tests/known_good_field_test.py @@ -1,10 +1,11 @@ """Pure-unit rendering tests for the known_good field on an artifact instance. No CliRunner / VCR — these drive the text formatter directly with a constructed -ArtifactInstance (the SDK resource), asserting that a known-good binary renders -its flagging feeds and a "Known good" status instead of the misleading -"no engines responded — rescan now" message, and that a normal instance is -unchanged. +ArtifactInstance (the SDK resource), asserting that a known-good binary — signalled +by its state, the only reliable signal — renders its flagging feeds and a +"Known good" status instead of the misleading "no engines responded — rescan now" +message, that the flagging feeds on their own never make an instance known-good, +and that a normal instance is unchanged. """ import click from polyswarm_api import resources @@ -13,6 +14,13 @@ SHA = 'a' * 64 +FEEDS = [ + {'tool': 'commercial', 'tool_metadata': {}, 'created': '2026-06-11T00:00:00', + 'updated': '2026-06-11T00:00:00'}, + {'tool': 'nsrl', 'tool_metadata': {}, 'created': '2026-06-11T00:00:00', + 'updated': '2026-06-11T00:00:00'}, +] + def _instance(**overrides): content = { @@ -26,25 +34,18 @@ def _instance(**overrides): return resources.ArtifactInstance(content) +def _assertion(engine, verdict): + return { + 'author': '0x' + 'd' * 40, 'author_name': engine, 'engine': {'name': engine}, + 'bid': '1000000000000000', 'mask': True, 'metadata': None, 'verdict': verdict, + } + + def _render(instance): return click.unstyle('\n'.join(TextOutput(color=False).artifact_instance(instance, write=False))) class TestKnownGoodTextRendering: - def test_known_good_instance_renders_feeds_and_status(self): - feeds = [ - {'tool': 'commercial', 'tool_metadata': {}, 'created': '2026-06-11T00:00:00', - 'updated': '2026-06-11T00:00:00'}, - {'tool': 'nsrl', 'tool_metadata': {}, 'created': '2026-06-11T00:00:00', - 'updated': '2026-06-11T00:00:00'}, - ] - text = _render(_instance(known_good=feeds)) - # Feeds are listed (sorted) and the status reflects known-good. - assert 'known-good binary (flagged by: commercial, nsrl); it is not scanned.' in text - assert 'Status: Known good' in text - # A known-good binary must NOT be told to rescan / that no engines responded. - assert 'trigger a rescan' not in text - def test_normal_instance_unchanged(self): text = _render(_instance()) assert 'known-good' not in text.lower() @@ -67,14 +68,32 @@ def test_state_known_good_without_feeds(self): assert 'No engines responded' not in text def test_state_known_good_with_feeds_lists_them(self): - feeds = [{'tool': 'commercial', 'tool_metadata': {}, - 'created': '2026-06-11T00:00:00', 'updated': '2026-06-11T00:00:00'}] - text = _render(_instance(state='KNOWN_GOOD', known_good=feeds)) - # When feeds are present the richer message still lists them. - assert 'known-good binary (flagged by: commercial); it is not scanned.' in text + text = _render(_instance(state='KNOWN_GOOD', known_good=FEEDS)) + # When feeds are present the richer message still lists them (sorted). + assert 'known-good binary (flagged by: commercial, nsrl); it is not scanned.' in text assert 'Status: Known good' in text def test_non_known_good_state_unchanged(self): text = _render(_instance(state='SETTLED')) assert 'known-good' not in text.lower() assert 'Status: Known good' not in text + + +class TestKnownGoodFeedsAreNotTheSignal: + """The flagging-feed list is served for every instance whose sha256 matches a + known-good record — including one that was scanned before it was flagged — so it + only shapes the message for an instance already known-good by state.""" + + def test_feeds_without_known_good_state_are_ignored(self): + text = _render(_instance(state='SETTLED', known_good=FEEDS)) + assert 'known-good' not in text.lower() + assert 'Status: Known good' not in text + assert 'Status: Assertion window closed' in text + + def test_scanned_instance_with_feeds_reports_its_detections(self): + assertions = [_assertion('engine-a', True), _assertion('engine-b', False)] + text = _render(_instance(state='SETTLED', known_good=FEEDS, assertions=assertions, + polyscore=0.9)) + # The real scan results are reported, never overwritten by a "not scanned" claim. + assert 'Detections: 1/2 engines reported malicious' in text + assert 'it is not scanned' not in text From 4bac08c71e49cdde63910080824a145989005bec Mon Sep 17 00:00:00 2001 From: Samuel Date: Wed, 29 Jul 2026 13:49:45 -0300 Subject: [PATCH 02/13] fix: floor the SDK pin at the release that exposes ArtifactInstance.state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The change made `.state` load-bearing with no fallback, but the dependency floor still allowed 4.0.0, which predates it — so a user on an in-range SDK would get a known-good instance rendered as an ordinary window-closed one ("No engines responded… trigger a rescan"), silently. `.state` shipped in 4.1.0, and the SDK contract says the floor must exclude releases missing behaviour the CLI relies on. Also: specs/04-testing.md's invariant said tests drive the CLI through CliRunner and never call internal functions, which as written forbade the formatter-unit tests this change adds — the spec now sanctions that style and says when it is the right choice. specs/03-formatters.md no longer overstates the feed-list guard as load-bearing. And two branches where the flag has to lose or be ignored are now pinned: a failed known-good instance reports the failure, and an instance with feeds but no state renders as an ordinary one — the assertion that would catch the removed fallback being reintroduced. --- AGENTS.md | 5 +++-- pyproject.toml | 2 +- specs/03-formatters.md | 26 ++++++++++++++++++-------- specs/04-testing.md | 10 ++++++++-- tests/known_good_field_test.py | 32 +++++++++++++++++++++++++++++++- 5 files changed, 61 insertions(+), 14 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2eb0a7f..eec96c6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -88,8 +88,9 @@ Mirror the existing patterns (e.g. `field-property`, `prompt-config`, `ruleset`) Details in [`specs/04-testing.md`](./specs/04-testing.md). The shape: -- Tests live in `tests/` and drive the CLI through `click.testing.CliRunner` — no live PolySwarm stack needed. -- Two styles: **SDK-boundary mocks** (`mock.patch('polyswarm_api.api.PolyswarmAPI.')`, e.g. `tests/field_property_test.py`) and **VCR cassettes** (`tests/cli_test.py`) that replay recorded HTTP for end-to-end CLI runs. Cassettes live in `tests/vcr/` (`.vcr` for the HTTP interactions, `.click` for the expected rendered output). +- Tests live in `tests/` and drive command behaviour through `click.testing.CliRunner` — no live PolySwarm stack needed. +- Two styles for that: **SDK-boundary mocks** (`mock.patch('polyswarm_api.api.PolyswarmAPI.')`, e.g. `tests/field_property_test.py`) and **VCR cassettes** (`tests/cli_test.py`) that replay recorded HTTP for end-to-end CLI runs. Cassettes live in `tests/vcr/` (`.vcr` for the HTTP interactions, `.click` for the expected rendered output). +- Pure rendering logic — which line a given field set produces, no command-tree behaviour — is instead unit-tested against the formatter directly (e.g. `tests/known_good_field_test.py`); see the spec's *Style 3* for when that's the right choice. - VCR is an **efficiency cache, not a requirement** — the suite must pass against a live e2e stack with VCR off. Re-record a cassette by deleting it and re-running the test against a live stack; never hand-edit a cassette or `cp` one from a sibling test. ## Commit + PR hygiene diff --git a/pyproject.toml b/pyproject.toml index 933b174..7ae9a9c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,7 +22,7 @@ classifiers = [ ] dependencies = [ - "polyswarm_api>=4.0.0,<5.0.0", + "polyswarm_api>=4.1.0,<5.0.0", "click>=7.1", "colorama>=0.4.6", "click-log>=0.4.0", diff --git a/specs/03-formatters.md b/specs/03-formatters.md index 7e57ea6..176771e 100644 --- a/specs/03-formatters.md +++ b/specs/03-formatters.md @@ -60,11 +60,21 @@ instance would otherwise get. It is gated by a single known-goodness — the server emits the feed list for *any* instance whose sha256 matches a known-good record, including a fully scanned one with real detections/PolyScore, so treating it as the signal would render a scanned artifact - as "known-good … not scanned". It is therefore read as `[]` unless `is_known_good`. - -The **Status** line reads "Known good" whenever `is_known_good`. Both attributes are -read with `getattr(..., None)` so a CLI on an older SDK (missing either field) never -raises `AttributeError`; an SDK without `.state` simply never takes the known-good -branch, which is the safe fallback — the pre-known-good rendering. `JSONOutput` needs -no change — it dumps the resource's `.json`, which already carries the raw `state` and -`known_good` keys. + as "known-good … not scanned". It is therefore read as `[]` unless `is_known_good` — + a statement of that coupling, not a guard: the list is only ever read inside the + known-good branch, so the ternary can't change what renders today. Keep it, and keep + reading the feeds through it if a second call site ever appears. + +The **Status** line reads "Known good" whenever `is_known_good`, except on a **failed** +instance — "Status: Failed" is ordered first and the known-good **Detections** branch is +gated on `not instance.failed`, so a failure is reported as a failure and never as +"…it is not scanned". + +Both attributes are read with `getattr(..., None)` so a CLI on an older SDK (missing +either field) never raises `AttributeError`; an SDK without `.state` simply never takes +the known-good branch, which is the safe fallback — the pre-known-good rendering. That +degradation is belt-and-braces, not a supported configuration: `.state` is load-bearing +here with no substitute, so the dependency floor is `polyswarm_api>=4.1.0` — the release +that exposes it (see [`05-sdk-contract.md`](./05-sdk-contract.md) §Version pin). +`JSONOutput` needs no change — it dumps the resource's `.json`, which already carries the +raw `state` and `known_good` keys. diff --git a/specs/04-testing.md b/specs/04-testing.md index 5aa424c..7b100e6 100644 --- a/specs/04-testing.md +++ b/specs/04-testing.md @@ -2,11 +2,11 @@ ## Scope -How the CLI is tested: the `CliRunner` harness, the two mocking styles (SDK-boundary mocks vs VCR cassettes), the cassette layout and record workflow, and how to run the suite. Files: `tests/`, `tests/vcr/`, `src/conftest.py`, `pyproject.toml` (`[project.optional-dependencies].tests`, `[tool.pytest.ini_options]`). +How the CLI is tested: the `CliRunner` harness, the two mocking styles (SDK-boundary mocks vs VCR cassettes), the formatter-unit style for pure rendering, the cassette layout and record workflow, and how to run the suite. Files: `tests/`, `tests/vcr/`, `src/conftest.py`, `pyproject.toml` (`[project.optional-dependencies].tests`, `[tool.pytest.ini_options]`). ## Invariants -- **Tests drive the CLI through `click.testing.CliRunner`** — they invoke the real command tree, not internal functions. No live PolySwarm stack is required. +- **Anything that is command behaviour is driven through `click.testing.CliRunner`** — argument parsing, the SDK call, the wiring, the exit code: exercise the real command tree, never an internal function standing in for it. No live PolySwarm stack is required. The one sanctioned exception is pure rendering logic — see [Style 3](#style-3--formatter-unit-tests). - **Mock at the SDK boundary, or replay HTTP with VCR — never both for the same path.** A test either patches `polyswarm_api.api.PolyswarmAPI.` (unit-style) or lets VCR replay recorded HTTP (end-to-end). The CLI's own code is exercised either way. - **VCR is an efficiency cache, not a load-bearing requirement.** The suite must pass against a live e2e stack with VCR off. Don't hardcode `record_mode='none'`; if a test only works against its recorded cassette, that's a bug in the test. - **Never `cp` a cassette from a sibling test, never hand-edit cassette bytes.** Re-record against a live stack. @@ -46,6 +46,12 @@ pytest tests/cli_test.py:::: # records against whatever stack you Point your environment at a live e2e stack, run the test, and commit the freshly recorded `.vcr` (+ updated `.click`). The deletion is what makes VCR record. +## Style 3 — formatter unit tests + +For **rendering logic with no command-tree behaviour** — which labelled line a given field set produces — construct the formatter directly (`TextOutput(color=False)`) and call the resource method with an SDK resource built from a literal dict, asserting on the returned lines (`write=False`, no stream, no cassette). Example: `tests/known_good_field_test.py` renders `ArtifactInstance`s that differ only in `state` / `known_good` and asserts which Detections/Status line comes out. This is the right choice when the branch matrix is wide and every branch is a function of the resource's fields — a cassette per branch would mean recording a server state that only the formatter cares about. + +Use it **only** for that. Argument parsing, SDK calls, generator consumption, `ctx.obj` wiring and exit codes are command behaviour: a formatter unit test can't observe them, so those need Style 1 or Style 2. A command whose rendering is covered by Style 3 still needs at least one `CliRunner` test proving the command reaches the formatter at all. + ## What to test for a new command 1. The command parses its arguments and calls the expected SDK method with the expected arguments. diff --git a/tests/known_good_field_test.py b/tests/known_good_field_test.py index 68bc6b3..232aaef 100644 --- a/tests/known_good_field_test.py +++ b/tests/known_good_field_test.py @@ -5,7 +5,8 @@ by its state, the only reliable signal — renders its flagging feeds and a "Known good" status instead of the misleading "no engines responded — rescan now" message, that the flagging feeds on their own never make an instance known-good, -and that a normal instance is unchanged. +that a failure outranks the known-good signal, and that a normal instance is +unchanged. """ import click from polyswarm_api import resources @@ -78,6 +79,19 @@ def test_non_known_good_state_unchanged(self): assert 'known-good' not in text.lower() assert 'Status: Known good' not in text + def test_failed_instance_reports_the_failure_not_known_good(self): + # A failure outranks the known-good signal: the Detections branch is gated on + # `not instance.failed` and 'Status: Failed' is ordered ahead of 'Status: Known + # good'. Dropping either would tell the user a failed lookup is a benign binary. + text = _render(_instance(state='KNOWN_GOOD', known_good=FEEDS, failed=True, + failed_reason='artifact storage unavailable')) + assert 'Detections: This scan has failed. Please try again.' in text + assert 'Status: Failed' in text + assert 'Failure Reason: artifact storage unavailable' in text + # Never claim the artifact is a known-good binary that was not scanned. + assert 'it is not scanned' not in text + assert 'Status: Known good' not in text + class TestKnownGoodFeedsAreNotTheSignal: """The flagging-feed list is served for every instance whose sha256 matches a @@ -90,6 +104,22 @@ def test_feeds_without_known_good_state_are_ignored(self): assert 'Status: Known good' not in text assert 'Status: Assertion window closed' in text + def test_feeds_without_any_state_render_as_an_ordinary_instance(self): + # The no-state path: an older server omits `state` (parses to None) and an SDK + # predating `.state` has no such attribute at all. Both leave the feed list as + # the only known-good hint, and it must not be used — this is the assertion that + # catches the removed feed-list fallback being reintroduced. + no_state = _instance(known_good=FEEDS) + older_sdk = _instance(known_good=FEEDS) + del older_sdk.state + for instance in (no_state, older_sdk): + text = _render(instance) + assert 'known-good' not in text.lower() + assert 'Status: Known good' not in text + # Rendered exactly like any other window-closed instance with no assertions. + assert 'Detections: No engines responded to this scan. You can trigger a rescan now.' in text + assert 'Status: Assertion window closed' in text + def test_scanned_instance_with_feeds_reports_its_detections(self): assertions = [_assertion('engine-a', True), _assertion('engine-b', False)] text = _render(_instance(state='SETTLED', known_good=FEEDS, assertions=assertions, From 16fcdc8cbc150b7561ea72135e5971a865a68d32 Mon Sep 17 00:00:00 2001 From: Samuel Date: Wed, 29 Jul 2026 14:39:51 -0300 Subject: [PATCH 03/13] fix: floor the SDK at 4.2.0, and stop claiming a scanned known-good sample is unscanned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two behaviours the CLI relies on arrived in 4.2.0, not 4.1.0: llm_report_create only started sending the client's community then (and `report llm-create` does not pass one itself, so on 4.1.0 a private-community report is created without it), and streaming downloads only started raising NoResultsException on a 204 then, which the download command's no-results exit code depends on. The assertion loop was not gated by the known-good flag, so an instance carrying that state *and* assertions rendered "it is not scanned" immediately followed by per-engine verdicts. That pairing is no longer hypothetical: the server now reconciles a previously scanned instance into the known-good state while deliberately preserving its assertions, polyscore and detections, so it is exactly what a client receives. The rendering now says both things without contradicting itself — the bytes are withheld, and here are the results we already had. --- pyproject.toml | 2 +- specs/03-formatters.md | 31 ++++++++++++++++++++--- specs/05-sdk-contract.md | 10 ++++++++ src/polyswarm/formatters/text.py | 16 +++++++----- tests/known_good_field_test.py | 43 ++++++++++++++++++++++++++++++-- 5 files changed, 89 insertions(+), 13 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 7ae9a9c..8d12053 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,7 +22,7 @@ classifiers = [ ] dependencies = [ - "polyswarm_api>=4.1.0,<5.0.0", + "polyswarm_api>=4.2.0,<5.0.0", "click>=7.1", "colorama>=0.4.6", "click-log>=0.4.0", diff --git a/specs/03-formatters.md b/specs/03-formatters.md index 176771e..1e5019a 100644 --- a/specs/03-formatters.md +++ b/specs/03-formatters.md @@ -65,6 +65,28 @@ instance would otherwise get. It is gated by a single known-good branch, so the ternary can't change what renders today. Keep it, and keep reading the feeds through it if a second call site ever appears. +### A known-good instance that also carries results + +Known-goodness and collected results are **not** mutually exclusive. An instance that was +scanned before its artifact was catalogued gets reconciled to `KNOWN_GOOD` with its +assertions, detections and PolyScore deliberately preserved, so that pairing is something +a client really receives — the rendering must state both facts without claiming the +artifact was never scanned. The trailing clause of the known-good **Detections** line +therefore switches on `instance.valid_assertions`: + +- **no valid assertions** → "…is a known-good binary; it is not scanned." — the withheld, + never-scanned case. +- **valid assertions** → "…is a known-good binary; N/M engines reported malicious." — the + same count the ordinary window-closed branch renders, folded into the known-good + sentence. + +The flagging-feed attribution ("(flagged by: …)") is orthogonal and applies to both. In +either branch the per-engine verdict list, the PolyScore line and "Status: Known good" +render as usual; the switch exists so "it is not scanned" is never printed directly above a +list of engine verdicts. The line stays **green** both ways — known-goodness is the +dominant signal (the bytes are withheld whatever the preserved assertions say), and the +detections are reported rather than suppressed. + The **Status** line reads "Known good" whenever `is_known_good`, except on a **failed** instance — "Status: Failed" is ordered first and the known-good **Detections** branch is gated on `not instance.failed`, so a failure is reported as a failure and never as @@ -74,7 +96,8 @@ Both attributes are read with `getattr(..., None)` so a CLI on an older SDK (mis either field) never raises `AttributeError`; an SDK without `.state` simply never takes the known-good branch, which is the safe fallback — the pre-known-good rendering. That degradation is belt-and-braces, not a supported configuration: `.state` is load-bearing -here with no substitute, so the dependency floor is `polyswarm_api>=4.1.0` — the release -that exposes it (see [`05-sdk-contract.md`](./05-sdk-contract.md) §Version pin). -`JSONOutput` needs no change — it dumps the resource's `.json`, which already carries the -raw `state` and `known_good` keys. +here with no substitute. Both attributes ship in SDK **4.1.0**, but the dependency floor is +`polyswarm_api>=4.2.0` — set by two *other* behaviours the CLI depends on, both of which +fail silently on 4.1.0 (see [`05-sdk-contract.md`](./05-sdk-contract.md) §Version pin) — so +every supported install has them. `JSONOutput` needs no change — it dumps the resource's +`.json`, which already carries the raw `state` and `known_good` keys. diff --git a/specs/05-sdk-contract.md b/specs/05-sdk-contract.md index 2b3f154..5fa5db0 100644 --- a/specs/05-sdk-contract.md +++ b/specs/05-sdk-contract.md @@ -69,6 +69,16 @@ When a CLI feature needs an SDK surface that doesn't exist yet: - The pin lives in `pyproject.toml` `dependencies` (`polyswarm_api>=…`). Floor it at the lowest SDK version exposing everything the CLI uses; cap it below the next known-incompatible major when one is anticipated. - The CLI is **sync-only** — it imports `polyswarm_api.api.PolyswarmAPI`, never `polyswarm_api.aio`. Don't add the `polyswarm_api[async]` extra. - Bumping the pin is a normal code change; bumping the CLI's *own* version is a release step (`AGENTS.md` §Gitflow). They're unrelated. +- There is **no lock file / compiled requirements** to keep in step: `pyproject.toml` is the only place the SDK version is expressed, and CI installs the SDK straight from the SDK repo's branch archive (see §Coordinated changes). A pin change is a one-file change. + +### Current floor — `polyswarm_api>=4.2.0` + +Two behaviours the CLI relies on only exist from **4.2.0**; on 4.1.0 both fail *silently*, which is why the floor is a hard requirement rather than a preference: + +1. **`llm_report_create` sends the client's community.** 4.2.0 passes `community=self.community` when it builds the report resource; 4.1.0 omits it. `report llm-create` (`client/report.py`) supplies no community of its own — it relies entirely on the client's — so on 4.1.0 a report requested for a sample in a private community is created without one. No error, wrong resource. +2. **A streaming download answered `204 No Content` raises `NoResultsException`.** The streaming path bypasses `parse_response`, so the 204 has to be raised by the session itself; 4.2.0 does that, 4.1.0 has no such raise anywhere in its session. The CLI's `download` commands depend on it for the no-results **exit code `1`** (§No-results signalling); against 4.1.0 an empty response reads as a successful download and exits `0`. + +The known-good rendering attributes (`ArtifactInstance.state`, `.known_good`/`.known_good_sources`, read by `formatters/text.py` — see [`03-formatters.md`](./03-formatters.md) §Known-good artifact instances) ship in **4.1.0**, so they are *not* what sets the floor; they are simply covered by it. ## Worked example — the httpx SDK migration diff --git a/src/polyswarm/formatters/text.py b/src/polyswarm/formatters/text.py index dca0589..cf2fb68 100644 --- a/src/polyswarm/formatters/text.py +++ b/src/polyswarm/formatters/text.py @@ -88,13 +88,17 @@ def artifact_instance(self, instance, write=True, timeout=False): known_good_sources = (getattr(instance, 'known_good_sources', None) or []) if is_known_good else [] if is_known_good and not instance.failed: - if known_good_sources: - feeds = ', '.join(known_good_sources) - output.append(self._green( - f'Detections: This artifact is a known-good binary (flagged by: {feeds}); it is not scanned.')) + # A known-good binary can still carry results: an instance scanned before the + # artifact was catalogued is reconciled to KNOWN_GOOD with its assertions, + # detections and PolyScore preserved. Report those instead of the "not scanned" + # clause, which would contradict the per-engine verdicts printed just below. + attribution = f' (flagged by: {", ".join(known_good_sources)})' if known_good_sources else '' + if len(instance.valid_assertions) > 0: + detections = f'{len(instance.malicious_assertions)}/{len(instance.valid_assertions)} engines reported malicious' else: - output.append(self._green( - 'Detections: This artifact is a known-good binary; it is not scanned.')) + detections = 'it is not scanned' + output.append(self._green( + f'Detections: This artifact is a known-good binary{attribution}; {detections}.')) elif instance.community == 'stream': output.append(self._white('Detections: This artifact has not been scanned. You can trigger a scan now.')) elif len(instance.valid_assertions) == 0 and instance.window_closed and not instance.failed: diff --git a/tests/known_good_field_test.py b/tests/known_good_field_test.py index 232aaef..a7b6fd9 100644 --- a/tests/known_good_field_test.py +++ b/tests/known_good_field_test.py @@ -5,8 +5,9 @@ by its state, the only reliable signal — renders its flagging feeds and a "Known good" status instead of the misleading "no engines responded — rescan now" message, that the flagging feeds on their own never make an instance known-good, -that a failure outranks the known-good signal, and that a normal instance is -unchanged. +that a failure outranks the known-good signal, that a known-good instance which +also carries preserved results reports them instead of claiming it was not +scanned, and that a normal instance is unchanged. """ import click from polyswarm_api import resources @@ -74,6 +75,44 @@ def test_state_known_good_with_feeds_lists_them(self): assert 'known-good binary (flagged by: commercial, nsrl); it is not scanned.' in text assert 'Status: Known good' in text + def test_known_good_with_results_reports_them_instead_of_not_scanned(self): + # A previously scanned instance reconciled to KNOWN_GOOD keeps its assertions, + # detections and PolyScore. The assertion loop is not gated on the known-good + # branch, so the old copy printed "it is not scanned." immediately above the + # per-engine verdicts — a self-contradiction. The Detections line must state + # both facts: still a known-good binary, and here is what the engines found. + assertions = [_assertion('engine-a', True), _assertion('engine-b', False)] + text = _render(_instance(state='KNOWN_GOOD', known_good=FEEDS, assertions=assertions, + polyscore=0.9)) + assert ('Detections: This artifact is a known-good binary (flagged by: commercial, nsrl); ' + '1/2 engines reported malicious.') in text + assert 'it is not scanned' not in text + # The verdicts that made the old phrasing contradictory still render, as does the + # known-good status and the preserved PolyScore. + assert 'engine-a: Malicious' in text + assert 'engine-b: Clean' in text + assert 'Status: Known good' in text + assert 'PolyScore: 0.9' in text + + def test_known_good_with_results_and_no_feeds(self): + # Same reconciliation without a flagging-feed attribution: the feed clause is + # orthogonal to the results clause, so dropping one must not drop the other. + assertions = [_assertion('engine-a', False)] + text = _render(_instance(state='KNOWN_GOOD', assertions=assertions)) + assert 'Detections: This artifact is a known-good binary; 0/1 engines reported malicious.' in text + assert 'it is not scanned' not in text + assert 'Status: Known good' in text + + def test_known_good_with_only_non_responding_engines_is_still_not_scanned(self): + # Assertions that all declined (mask False) are not results — valid_assertions is + # empty — so the withheld/never-scanned clause is still the accurate one. Keying + # the switch on `instance.assertions` instead would render "0/0 engines". + declined = _assertion('engine-a', None) + declined['mask'] = False + text = _render(_instance(state='KNOWN_GOOD', assertions=[declined])) + assert 'Detections: This artifact is a known-good binary; it is not scanned.' in text + assert 'engines reported malicious' not in text + def test_non_known_good_state_unchanged(self): text = _render(_instance(state='SETTLED')) assert 'known-good' not in text.lower() From 6534ff43921b6c99bbf230f4887b8a2a30cfb3bd Mon Sep 17 00:00:00 2001 From: Samuel Date: Wed, 29 Jul 2026 15:36:11 -0300 Subject: [PATCH 04/13] fix(text): colour a known-good line by its verdict, not by its status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the known-good rendering: - A known-good instance carrying malicious detections rendered the whole Detections line green, including the "N/M engines reported malicious" clause the ordinary branch renders red. Green on 40/50 is a weaker warning than the same instance gave before it was reconciled, which is the class of mis-signal this rendering exists to fix. Red when there are malicious assertions and the window is closed; green otherwise. - The counts are now guarded on window_closed like every other branch, so an open window's numbers are never presented as final. - Pin the colour decision against TextOutput(color=True): every existing test unstyles its output, so the green/red choice was unobservable. - Drop the `del instance.state` half of the no-state test — it mutates an SDK resource's internals, which specs/05 forbids, and pins a configuration specs/03 declares unsupported. The server-omits-state case it shared covers the real behaviour. - Document the floor's two preconditions (published on PyPI, and declared by the SDK's develop archive) — both verified for 4.2.0. --- specs/03-formatters.md | 33 ++++++++++----- specs/05-sdk-contract.md | 3 +- src/polyswarm/formatters/text.py | 19 +++++++-- tests/known_good_field_test.py | 69 +++++++++++++++++++++++++------- 4 files changed, 97 insertions(+), 27 deletions(-) diff --git a/specs/03-formatters.md b/specs/03-formatters.md index 1e5019a..f57bc9a 100644 --- a/specs/03-formatters.md +++ b/specs/03-formatters.md @@ -76,16 +76,31 @@ therefore switches on `instance.valid_assertions`: - **no valid assertions** → "…is a known-good binary; it is not scanned." — the withheld, never-scanned case. -- **valid assertions** → "…is a known-good binary; N/M engines reported malicious." — the - same count the ordinary window-closed branch renders, folded into the known-good - sentence. - -The flagging-feed attribution ("(flagged by: …)") is orthogonal and applies to both. In -either branch the per-engine verdict list, the PolyScore line and "Status: Known good" +- **valid assertions, window closed** → "…is a known-good binary; N/M engines reported + malicious." — the same count the ordinary window-closed branch renders, folded into the + known-good sentence. +- **valid assertions, window still open** → "…is a known-good binary; its scan has not + finished running yet." Every other **Detections** branch guards its counts on + `window_closed`, because an open window's numbers are not final. Unreachable through the + server's reconciliation (it moves only `STORED` rows, which carry no assertions, and + `SETTLED` ones, which have a closed window), so this is a guard against a future state + rather than a case seen in practice — stated here so the parity with the other branches + reads as intentional. + +The flagging-feed attribution ("(flagged by: …)") is orthogonal and applies to all three. In +every branch the per-engine verdict list, the PolyScore line and "Status: Known good" render as usual; the switch exists so "it is not scanned" is never printed directly above a -list of engine verdicts. The line stays **green** both ways — known-goodness is the -dominant signal (the bytes are withheld whatever the preserved assertions say), and the -detections are reported rather than suppressed. +list of engine verdicts. + +**Colour: a majority-malicious verdict outranks the withheld-bytes signal.** The line is +**green** except when the instance has malicious assertions and a closed window, where it is +**red** — the same colour the ordinary branch gives that count. Known-goodness is the +dominant *fact*, but it is not a stronger *warning*: rendering "40/50 engines reported +malicious" in green would be a weaker signal than the very same instance produced before it +was reconciled, which is the class of mis-signal this rendering exists to fix. "Status: Known +good" stays green in both cases — it labels the catalogue status (the counterpart of "Status: +Assertion window closed"), not the verdict. The colour decision is invisible to a test that +unstyles its output, so it is pinned directly against `TextOutput(color=True)`. The **Status** line reads "Known good" whenever `is_known_good`, except on a **failed** instance — "Status: Failed" is ordered first and the known-good **Detections** branch is diff --git a/specs/05-sdk-contract.md b/specs/05-sdk-contract.md index 5fa5db0..2cca943 100644 --- a/specs/05-sdk-contract.md +++ b/specs/05-sdk-contract.md @@ -69,7 +69,8 @@ When a CLI feature needs an SDK surface that doesn't exist yet: - The pin lives in `pyproject.toml` `dependencies` (`polyswarm_api>=…`). Floor it at the lowest SDK version exposing everything the CLI uses; cap it below the next known-incompatible major when one is anticipated. - The CLI is **sync-only** — it imports `polyswarm_api.api.PolyswarmAPI`, never `polyswarm_api.aio`. Don't add the `polyswarm_api[async]` extra. - Bumping the pin is a normal code change; bumping the CLI's *own* version is a release step (`AGENTS.md` §Gitflow). They're unrelated. -- There is **no lock file / compiled requirements** to keep in step: `pyproject.toml` is the only place the SDK version is expressed, and CI installs the SDK straight from the SDK repo's branch archive (see §Coordinated changes). A pin change is a one-file change. +- There is **no lock file / compiled requirements** to keep in step: `pyproject.toml` is the only place the SDK version is expressed, and CI installs the SDK straight from the SDK repo's branch archive (see §Coordinated changes). A pin change is a one-file change *in this repo*, but it is not free of interactions — see below. +- **The floor must be satisfied by the SDK archive CI installs, and by PyPI.** CI installs the archive build and *then* runs `pip install .[tests]`; if the archive's declared version is below the floor, that second install silently pulls a newer SDK from PyPI **over** the archive build, and CI stops testing the SDK branch at all — the mechanism §Coordinated changes rests on, defeated with no error. Symmetrically, a floor above the newest **published** version breaks `pip install polyswarm-cli` for every consumer the moment it reaches `master`. So a floor bump has two preconditions: the version is on PyPI, and the SDK's `develop` declares at least that version. ### Current floor — `polyswarm_api>=4.2.0` diff --git a/src/polyswarm/formatters/text.py b/src/polyswarm/formatters/text.py index cf2fb68..cb71fbd 100644 --- a/src/polyswarm/formatters/text.py +++ b/src/polyswarm/formatters/text.py @@ -93,12 +93,25 @@ def artifact_instance(self, instance, write=True, timeout=False): # detections and PolyScore preserved. Report those instead of the "not scanned" # clause, which would contradict the per-engine verdicts printed just below. attribution = f' (flagged by: {", ".join(known_good_sources)})' if known_good_sources else '' - if len(instance.valid_assertions) > 0: + if len(instance.valid_assertions) > 0 and instance.window_closed: detections = f'{len(instance.malicious_assertions)}/{len(instance.valid_assertions)} engines reported malicious' + elif len(instance.valid_assertions) > 0: + # Every other branch guards its counts on window_closed, because an open + # window's numbers are not final. Not reachable through reconciliation today + # (a STORED row carries no assertions and a SETTLED one has a closed window), + # stated rather than left implied. + detections = 'its scan has not finished running yet' else: detections = 'it is not scanned' - output.append(self._green( - f'Detections: This artifact is a known-good binary{attribution}; {detections}.')) + line = f'Detections: This artifact is a known-good binary{attribution}; {detections}.' + # A majority-malicious verdict outranks the withheld-bytes signal for colour: this + # branch replaced one that rendered the very same count in red, and green on + # "40/50 engines reported malicious" is a weaker warning than the instance had + # before it was reconciled. The preserved results still mean what they meant. + if instance.malicious_assertions and instance.window_closed: + output.append(self._red(line)) + else: + output.append(self._green(line)) elif instance.community == 'stream': output.append(self._white('Detections: This artifact has not been scanned. You can trigger a scan now.')) elif len(instance.valid_assertions) == 0 and instance.window_closed and not instance.failed: diff --git a/tests/known_good_field_test.py b/tests/known_good_field_test.py index a7b6fd9..3345dbd 100644 --- a/tests/known_good_field_test.py +++ b/tests/known_good_field_test.py @@ -47,6 +47,17 @@ def _render(instance): return click.unstyle('\n'.join(TextOutput(color=False).artifact_instance(instance, write=False))) +def _render_styled(instance): + """Keeps the ANSI wrapper, so the green/red decision itself is observable — every other + test here unstyles, which makes the colour (the whole point of the known-good rendering) + invisible.""" + return '\n'.join(TextOutput(color=True).artifact_instance(instance, write=False)) + + +def _detections_line(styled_text): + return next(line for line in styled_text.split('\n') if 'Detections:' in line) + + class TestKnownGoodTextRendering: def test_normal_instance_unchanged(self): text = _render(_instance()) @@ -94,6 +105,37 @@ def test_known_good_with_results_reports_them_instead_of_not_scanned(self): assert 'Status: Known good' in text assert 'PolyScore: 0.9' in text + def test_malicious_detections_outrank_the_known_good_colour(self): + # The colour is the signal here, and no other test can see it (they all unstyle). + # Rendering "1/2 engines reported malicious" in green — as this branch first did, + # replacing code that rendered the identical count in red — is a weaker warning + # than the same instance gave before it was reconciled. + assertions = [_assertion('engine-a', True), _assertion('engine-b', False)] + line = _detections_line(_render_styled( + _instance(state='KNOWN_GOOD', known_good=FEEDS, assertions=assertions))) + assert line == click.style( + 'Detections: This artifact is a known-good binary (flagged by: commercial, nsrl); ' + '1/2 engines reported malicious.', fg='red') + + def test_clean_known_good_stays_green(self): + # The other side of the same decision: nothing reported malicious, so the benign + # colouring is right and the red must not leak into it. + line = _detections_line(_render_styled( + _instance(state='KNOWN_GOOD', known_good=FEEDS, + assertions=[_assertion('engine-a', False)]))) + assert line == click.style( + 'Detections: This artifact is a known-good binary (flagged by: commercial, nsrl); ' + '0/1 engines reported malicious.', fg='green') + + def test_open_window_does_not_present_counts_as_final(self): + # Every other Detections branch guards its counts on window_closed. Not reachable + # through reconciliation (a STORED row carries no assertions, a SETTLED one has a + # closed window), pinned so the missing guard cannot come back by accident. + text = _render(_instance(state='KNOWN_GOOD', known_good=FEEDS, window_closed=False, + assertions=[_assertion('engine-a', True)])) + assert 'its scan has not finished running yet.' in text + assert 'engines reported malicious' not in text + def test_known_good_with_results_and_no_feeds(self): # Same reconciliation without a flagging-feed attribution: the feed clause is # orthogonal to the results clause, so dropping one must not drop the other. @@ -144,20 +186,19 @@ def test_feeds_without_known_good_state_are_ignored(self): assert 'Status: Assertion window closed' in text def test_feeds_without_any_state_render_as_an_ordinary_instance(self): - # The no-state path: an older server omits `state` (parses to None) and an SDK - # predating `.state` has no such attribute at all. Both leave the feed list as - # the only known-good hint, and it must not be used — this is the assertion that - # catches the removed feed-list fallback being reintroduced. - no_state = _instance(known_good=FEEDS) - older_sdk = _instance(known_good=FEEDS) - del older_sdk.state - for instance in (no_state, older_sdk): - text = _render(instance) - assert 'known-good' not in text.lower() - assert 'Status: Known good' not in text - # Rendered exactly like any other window-closed instance with no assertions. - assert 'Detections: No engines responded to this scan. You can trigger a rescan now.' in text - assert 'Status: Assertion window closed' in text + # The no-state path: an older server omits `state`, which parses to None, leaving the + # feed list as the only known-good hint — and it must not be used. This is the + # assertion that catches the removed feed-list fallback being reintroduced. + # (`getattr(instance, 'state', None)` also tolerates an SDK with no such attribute at + # all, but that is belt-and-braces rather than a supported configuration, and pinning + # it would mean deleting an attribute off a parsed SDK resource — reverse-engineering + # internals the CLI is not allowed to depend on. See specs/05.) + text = _render(_instance(known_good=FEEDS)) + assert 'known-good' not in text.lower() + assert 'Status: Known good' not in text + # Rendered exactly like any other window-closed instance with no assertions. + assert 'Detections: No engines responded to this scan. You can trigger a rescan now.' in text + assert 'Status: Assertion window closed' in text def test_scanned_instance_with_feeds_reports_its_detections(self): assertions = [_assertion('engine-a', True), _assertion('engine-b', False)] From b6bbf1fb6d070d00ed3249f4f34a47e832dee43a Mon Sep 17 00:00:00 2001 From: Samuel Date: Wed, 29 Jul 2026 16:16:30 -0300 Subject: [PATCH 05/13] test: pin that the known-good status line stays green when the verdict goes red specs/03 states it as a rule; _detections_line() filters the styled render down to the Detections line, so nothing observed it. --- tests/known_good_field_test.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/known_good_field_test.py b/tests/known_good_field_test.py index 3345dbd..eb54f93 100644 --- a/tests/known_good_field_test.py +++ b/tests/known_good_field_test.py @@ -111,11 +111,15 @@ def test_malicious_detections_outrank_the_known_good_colour(self): # replacing code that rendered the identical count in red — is a weaker warning # than the same instance gave before it was reconciled. assertions = [_assertion('engine-a', True), _assertion('engine-b', False)] - line = _detections_line(_render_styled( - _instance(state='KNOWN_GOOD', known_good=FEEDS, assertions=assertions))) - assert line == click.style( + styled = _render_styled( + _instance(state='KNOWN_GOOD', known_good=FEEDS, assertions=assertions)) + assert _detections_line(styled) == click.style( 'Detections: This artifact is a known-good binary (flagged by: commercial, nsrl); ' '1/2 engines reported malicious.', fg='red') + # The Status line is the counterpart of "Assertion window closed": it labels the + # catalogue status, not the verdict, so it stays green while the verdict goes red. + # Stated as a rule in specs/03 — pinned here so the two can't drift. + assert click.style('Status: Known good', fg='green') in styled def test_clean_known_good_stays_green(self): # The other side of the same decision: nothing reported malicious, so the benign From c2f018379748ac81f9d2965d380d3073c5f96223 Mon Sep 17 00:00:00 2001 From: Samuel Date: Wed, 29 Jul 2026 16:29:10 -0300 Subject: [PATCH 06/13] fix(text): honour --no-color, and say what the colour tests actually pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups. `self.color` was assigned twice in __init__ and read nowhere: every _white/_red/_green helper called click.style() unconditionally, so `--no-color` was a no-op for text output while JSONOutput honoured it and specs/03 claimed TextOutput did too. Masked in practice because click strips ANSI when stdout is not a tty. All five helpers now go through one `_paint`, which is the only place the flag is read. That also corrects what this PR's own colour tests pin: it is the absent click.unstyle, not the `color=` argument, that made the green/red decision observable. The spec and the helper docstring said otherwise. Also: - The spec header said "majority-malicious" while the code reddens on any malicious assertion — the same threshold the ordinary branch uses. - Pin the open-window colour. That is the one combination where the `and window_closed` conjunct is load-bearing: the instance has a malicious assertion, so dropping it turns the line red while it reads "its scan has not finished running yet", and every existing test still passed. --- specs/03-formatters.md | 14 ++++++++------ src/polyswarm/formatters/text.py | 31 +++++++++++++++++++++---------- tests/known_good_field_test.py | 29 ++++++++++++++++++++++++++--- 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/specs/03-formatters.md b/specs/03-formatters.md index f57bc9a..1bfa336 100644 --- a/specs/03-formatters.md +++ b/specs/03-formatters.md @@ -20,7 +20,7 @@ How command output is rendered: the `BaseOutput` interface, the concrete formatt | Formatter | Module | Output | |---|---|---| -| `TextOutput` | `text.py` | Human-readable, labelled blocks; honours `--color/--no-color`. Dates via `polyswarm_api.core.parse_isoformat`. | +| `TextOutput` | `text.py` | Human-readable, labelled blocks; honours `--color/--no-color` (through `_paint`, the single place `self.color` is read — the five `_white`/`_red`/… helpers go through it, so a new one cannot style unconditionally). Dates via `polyswarm_api.core.parse_isoformat`. | | `JSONOutput` | `json.py` | Machine-readable JSON (typically the resource's `.json` plus derived fields). | | `SHA256Output` / `SHA1Output` / `MD5Output` | `hashes.py` | Hash-only output — prints the relevant digest per result. | @@ -92,15 +92,17 @@ every branch the per-engine verdict list, the PolyScore line and "Status: Known render as usual; the switch exists so "it is not scanned" is never printed directly above a list of engine verdicts. -**Colour: a majority-malicious verdict outranks the withheld-bytes signal.** The line is -**green** except when the instance has malicious assertions and a closed window, where it is -**red** — the same colour the ordinary branch gives that count. Known-goodness is the +**Colour: any malicious verdict outranks the withheld-bytes signal.** The line is +**green** except when the instance has **at least one** malicious assertion and a closed +window, where it is **red** — the same threshold and the same colour the ordinary branch +gives that count (1/50 reddens there too). Known-goodness is the dominant *fact*, but it is not a stronger *warning*: rendering "40/50 engines reported malicious" in green would be a weaker signal than the very same instance produced before it was reconciled, which is the class of mis-signal this rendering exists to fix. "Status: Known good" stays green in both cases — it labels the catalogue status (the counterpart of "Status: -Assertion window closed"), not the verdict. The colour decision is invisible to a test that -unstyles its output, so it is pinned directly against `TextOutput(color=True)`. +Assertion window closed"), not the verdict. The colour decision is invisible to a test that unstyles its +output, so it is pinned against the **styled** render — `TextOutput` output read without `click.unstyle`, +which is what `_render_styled` in `known_good_field_test.py` exists for. The **Status** line reads "Known good" whenever `is_known_good`, except on a **failed** instance — "Status: Failed" is ordered first and the known-good **Detections** branch is diff --git a/src/polyswarm/formatters/text.py b/src/polyswarm/formatters/text.py index cb71fbd..ecfa890 100644 --- a/src/polyswarm/formatters/text.py +++ b/src/polyswarm/formatters/text.py @@ -32,7 +32,6 @@ def __init__(self, color=True, output=sys.stdout, **kwargs): super().__init__(output) self.color = color self._depth = 0 - self.color = color def _get_score_format(self, score): if score < 0.3: @@ -104,10 +103,11 @@ def artifact_instance(self, instance, write=True, timeout=False): else: detections = 'it is not scanned' line = f'Detections: This artifact is a known-good binary{attribution}; {detections}.' - # A majority-malicious verdict outranks the withheld-bytes signal for colour: this - # branch replaced one that rendered the very same count in red, and green on - # "40/50 engines reported malicious" is a weaker warning than the instance had - # before it was reconciled. The preserved results still mean what they meant. + # ANY malicious verdict outranks the withheld-bytes signal for colour (the same + # threshold the ordinary branch uses): this branch replaced one that rendered the + # very same count in red, and green on "40/50 engines reported malicious" is a + # weaker warning than the instance had before it was reconciled. The preserved + # results still mean what they meant. if instance.malicious_assertions and instance.window_closed: output.append(self._red(line)) else: @@ -905,25 +905,36 @@ def sample(self, result, write=True): return self._output(output, write) + def _paint(self, text, fg): + """Apply a colour, or don't — the one place `self.color` is honoured. + + It used to be assigned and never read, so every one of these helpers styled + unconditionally and `--no-color` was a no-op for text output (invisible in practice + because click strips ANSI when stdout is not a tty, and because JSONOutput does + honour the flag). Not decorated with `is_grouped`: the callers already are, and the + indent must be applied exactly once. + """ + return click.style(text, fg=fg) if self.color else text + @is_grouped def _white(self, text): - return click.style(text, fg='white') + return self._paint(text, 'white') @is_grouped def _yellow(self, text): - return click.style(text, fg='yellow') + return self._paint(text, 'yellow') @is_grouped def _red(self, text): - return click.style(text, fg='red') + return self._paint(text, 'red') @is_grouped def _blue(self, text): - return click.style(text, fg='blue') + return self._paint(text, 'blue') @is_grouped def _green(self, text): - return click.style(text, fg='green') + return self._paint(text, 'green') def _open_group(self): self._depth += 1 diff --git a/tests/known_good_field_test.py b/tests/known_good_field_test.py index eb54f93..591942f 100644 --- a/tests/known_good_field_test.py +++ b/tests/known_good_field_test.py @@ -50,7 +50,8 @@ def _render(instance): def _render_styled(instance): """Keeps the ANSI wrapper, so the green/red decision itself is observable — every other test here unstyles, which makes the colour (the whole point of the known-good rendering) - invisible.""" + invisible. It is the absent `click.unstyle`, not the `color=` argument, that makes the + difference; `color=True` is the default and is passed only for emphasis.""" return '\n'.join(TextOutput(color=True).artifact_instance(instance, write=False)) @@ -135,10 +136,17 @@ def test_open_window_does_not_present_counts_as_final(self): # Every other Detections branch guards its counts on window_closed. Not reachable # through reconciliation (a STORED row carries no assertions, a SETTLED one has a # closed window), pinned so the missing guard cannot come back by accident. - text = _render(_instance(state='KNOWN_GOOD', known_good=FEEDS, window_closed=False, - assertions=[_assertion('engine-a', True)])) + instance = _instance(state='KNOWN_GOOD', known_good=FEEDS, window_closed=False, + assertions=[_assertion('engine-a', True)]) + text = _render(instance) assert 'its scan has not finished running yet.' in text assert 'engines reported malicious' not in text + # ...and it stays GREEN. This is the one combination where the `and window_closed` + # conjunct in the colour check is load-bearing: the instance has a malicious + # assertion, so without it the line reddens while saying the scan is unfinished. + assert _detections_line(_render_styled(instance)) == click.style( + 'Detections: This artifact is a known-good binary (flagged by: commercial, nsrl); ' + 'its scan has not finished running yet.', fg='green') def test_known_good_with_results_and_no_feeds(self): # Same reconciliation without a flagging-feed attribution: the feed clause is @@ -178,6 +186,21 @@ def test_failed_instance_reports_the_failure_not_known_good(self): assert 'Status: Known good' not in text +class TestColorFlag: + """`--no-color` reaches the formatter as `TextOutput(color=False)`. It used to be + assigned and never read, so text output styled unconditionally and the flag did + nothing — masked in practice by click stripping ANSI off a non-tty.""" + + def test_no_color_emits_no_ansi(self): + assertions = [_assertion('engine-a', True), _assertion('engine-b', False)] + instance = _instance(state='KNOWN_GOOD', known_good=FEEDS, assertions=assertions) + plain = '\n'.join(TextOutput(color=False).artifact_instance(instance, write=False)) + assert '\x1b[' not in plain + # Same text, just unpainted — the flag drops the wrapper, never the content. + assert plain == click.unstyle('\n'.join( + TextOutput(color=True).artifact_instance(instance, write=False))) + + class TestKnownGoodFeedsAreNotTheSignal: """The flagging-feed list is served for every instance whose sha256 matches a known-good record — including one that was scanned before it was flagged — so it From 4cb756f8f112c82357b495a416aaf319e0f55b93 Mon Sep 17 00:00:00 2001 From: Samuel Date: Wed, 29 Jul 2026 16:37:31 -0300 Subject: [PATCH 07/13] docs(formatters): record where the state wire shape is verified and pinned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `state` is load-bearing with no fallback, and a Style-3 formatter test cannot see the transport boundary. Rather than leave that inferred: the key and the label were read off the server's ArtifactInstanceSerializer (`'state': instance.state.name`, and BountyState.KNOWN_GOOD.name is exactly 'KNOWN_GOOD'), and the thing that would catch a future rename is the server's own suite — this repo replays frozen cassettes with no VCR-off e2e job, so a recorded body would keep replaying the old shape. Same treatment for the open-window branch: the claim that reconciliation cannot produce one is now stated against the mechanism it rests on (RECONCILABLE_STATES, and SETTLED requiring window_closed), with the fix to apply if that ever stops holding. --- specs/03-formatters.md | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/specs/03-formatters.md b/specs/03-formatters.md index 1bfa336..2904441 100644 --- a/specs/03-formatters.md +++ b/specs/03-formatters.md @@ -52,6 +52,16 @@ instance would otherwise get. It is gated by a single reliable statement that this artifact is known-good and its bytes are withheld, and it fires even for a scan-bypassed instance that carries **no** feed metadata. There is deliberately no separate "bytes withheld" field to consult. + +**The wire dependency, and where it is actually pinned.** `state` is load-bearing with no +fallback, and a Style-3 test cannot see the transport boundary — it compares against a dict the +test wrote itself. So the key and the label were **read off the server's serializer**, not +inferred: `ArtifactInstanceSerializer` emits `'state': instance.state.name`, and +`BountyState.KNOWN_GOOD.name` is the exact string `'KNOWN_GOOD'`. What would catch a *future* +rename is the server's own suite (which asserts that field and the `/v3/sample` status), not a +cassette here: this repo's CI replays frozen recordings with no VCR-off e2e job, so a recorded +body would keep replaying the old shape after a rename. If a live-e2e job is ever added here, a +`cli_test.py` cassette over a known-good hash becomes the right place to pin it. - **`known_good_sources`** (the sorted flagging-feed names, from `ArtifactInstance.known_good`) only **shapes the message** for an instance already known-good by state: when present, the **Detections** line names the feeds @@ -82,10 +92,14 @@ therefore switches on `instance.valid_assertions`: - **valid assertions, window still open** → "…is a known-good binary; its scan has not finished running yet." Every other **Detections** branch guards its counts on `window_closed`, because an open window's numbers are not final. Unreachable through the - server's reconciliation (it moves only `STORED` rows, which carry no assertions, and - `SETTLED` ones, which have a closed window), so this is a guard against a future state - rather than a case seen in practice — stated here so the parity with the other branches - reads as intentional. + server's reconciliation — verified against its `RECONCILABLE_STATES`, which is `STORED` + (a row that was never submitted, so it carries no assertions) plus `SETTLED` **and only + when `window_closed`**; the ingest gate's own path sets `window_closed = True`. So this is + a guard against a future state rather than a case seen in practice, stated here so the + parity with the other branches reads as intentional. If a reconciled row ever *can* carry + an open window alongside preserved malicious assertions, redden on `malicious_assertions` + alone and keep only the *text* guarded on the window — the sentence would still be + accurate, and green on a malicious detection is the mis-signal this rule exists to fix. The flagging-feed attribution ("(flagged by: …)") is orthogonal and applies to all three. In every branch the per-engine verdict list, the PolyScore line and "Status: Known good" From 62ef8d32be2674bfdd6433d430a1fe52382aca1c Mon Sep 17 00:00:00 2001 From: Samuel Date: Wed, 29 Jul 2026 16:58:00 -0300 Subject: [PATCH 08/13] fix: honour --no-color for the log prefix too, and pin the flag end to end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups. - The previous commit made TextOutput read self.color, but setup_logging never received the flag, so `polyswarm --no-color -v …` still emitted a green log prefix on a tty — the same half-honoured flag, one layer over. Threaded through, and the spec now states the flag's scope. - TestColorFlag pinned _paint, not the option: the flag travels through `formatters[output_format](color=color, …)` in the command group, which a formatter unit test cannot observe (specs/04 says so itself). Added a CliRunner test with color=True, which stops click stripping ANSI off the non-tty capture so the two runs differ only in the flag. - specs/03 claimed _paint means "a new helper cannot style unconditionally". It is a convention, not an enforcement point — reworded. - specs/05: the floor precondition needs the version read off the archive's own tree, because PEP 440 orders 4.2.0.dev1 < 4.2.0 and the SDK has a bumpversion dev part. Both files on origin/develop say 4.2.0 with no suffix. - Anchor the PolyScore assertion on the parsed line: 'PolyScore: 0.9' also matches 0.95, and the :.20f render is not the literal '0.9'. --- specs/03-formatters.md | 12 ++++++++++-- specs/05-sdk-contract.md | 2 ++ src/polyswarm/client/polyswarm.py | 12 ++++++++---- tests/cli_test.py | 32 +++++++++++++++++++++++++++++-- tests/known_good_field_test.py | 5 ++++- 5 files changed, 54 insertions(+), 9 deletions(-) diff --git a/specs/03-formatters.md b/specs/03-formatters.md index 2904441..af1aba4 100644 --- a/specs/03-formatters.md +++ b/specs/03-formatters.md @@ -20,7 +20,7 @@ How command output is rendered: the `BaseOutput` interface, the concrete formatt | Formatter | Module | Output | |---|---|---| -| `TextOutput` | `text.py` | Human-readable, labelled blocks; honours `--color/--no-color` (through `_paint`, the single place `self.color` is read — the five `_white`/`_red`/… helpers go through it, so a new one cannot style unconditionally). Dates via `polyswarm_api.core.parse_isoformat`. | +| `TextOutput` | `text.py` | Human-readable, labelled blocks; honours `--color/--no-color` through `_paint`, the single place `self.color` is read. The five `_white`/`_red`/… helpers all route through it — a convention, not an enforcement point: a new helper calling `click.style` directly would re-break the flag, so add colours by adding a `_paint` caller. Dates via `polyswarm_api.core.parse_isoformat`. | | `JSONOutput` | `json.py` | Machine-readable JSON (typically the resource's `.json` plus derived fields). | | `SHA256Output` / `SHA1Output` / `MD5Output` | `hashes.py` | Hash-only output — prints the relevant digest per result. | @@ -116,7 +116,15 @@ was reconciled, which is the class of mis-signal this rendering exists to fix. " good" stays green in both cases — it labels the catalogue status (the counterpart of "Status: Assertion window closed"), not the verdict. The colour decision is invisible to a test that unstyles its output, so it is pinned against the **styled** render — `TextOutput` output read without `click.unstyle`, -which is what `_render_styled` in `known_good_field_test.py` exists for. +which is what `_render_styled` in `known_good_field_test.py` exists for. Whether the +`--color/--no-color` **flag** reaches that rendering at all is a separate question a formatter +unit test cannot answer; `test_color_flag_reaches_the_text_formatter` (`cli_test.py`) covers it +through `CliRunner(… color=True)`. + +**Scope of `--no-color`.** It governs the text formatter (via `_paint`), `JSONOutput`, and the +log prefix (`setup_logging(verbosity, color=…)` — the `NamedColorFormatter` used to style +unconditionally, so `polyswarm --no-color -v …` still emitted a green prefix on a tty). It does +not attempt to suppress colour inside third-party output. The **Status** line reads "Known good" whenever `is_known_good`, except on a **failed** instance — "Status: Failed" is ordered first and the known-good **Detections** branch is diff --git a/specs/05-sdk-contract.md b/specs/05-sdk-contract.md index 2cca943..3957a91 100644 --- a/specs/05-sdk-contract.md +++ b/specs/05-sdk-contract.md @@ -72,6 +72,8 @@ When a CLI feature needs an SDK surface that doesn't exist yet: - There is **no lock file / compiled requirements** to keep in step: `pyproject.toml` is the only place the SDK version is expressed, and CI installs the SDK straight from the SDK repo's branch archive (see §Coordinated changes). A pin change is a one-file change *in this repo*, but it is not free of interactions — see below. - **The floor must be satisfied by the SDK archive CI installs, and by PyPI.** CI installs the archive build and *then* runs `pip install .[tests]`; if the archive's declared version is below the floor, that second install silently pulls a newer SDK from PyPI **over** the archive build, and CI stops testing the SDK branch at all — the mechanism §Coordinated changes rests on, defeated with no error. Symmetrically, a floor above the newest **published** version breaks `pip install polyswarm-cli` for every consumer the moment it reaches `master`. So a floor bump has two preconditions: the version is on PyPI, and the SDK's `develop` declares at least that version. + **Read the declared version off the archive's own tree, and mind pre-release suffixes.** PEP 440 orders `4.2.0.dev1 < 4.2.0`, so a `develop` head carrying a dev suffix (the SDK's `pyproject.toml` has a `[tool.bumpversion.parts.dev]`) would *not* satisfy a `>=4.2.0` floor even though it looks like 4.2.0 — and the archive build would be silently replaced from PyPI. Check the version string in the SDK branch's `pyproject.toml` / `__init__.py`, not the last release tag. For the current floor both were read from `origin/develop`: `version = "4.2.0"` and `__version__ = '4.2.0'`, no suffix. + ### Current floor — `polyswarm_api>=4.2.0` Two behaviours the CLI relies on only exist from **4.2.0**; on 4.1.0 both fail *silently*, which is why the floor is a hard requirement rather than a preference: diff --git a/src/polyswarm/client/polyswarm.py b/src/polyswarm/client/polyswarm.py index 48bd1e1..d9760aa 100644 --- a/src/polyswarm/client/polyswarm.py +++ b/src/polyswarm/client/polyswarm.py @@ -92,7 +92,7 @@ def resolve_api_uri(api_uri, api_uri_from_cli, shortcuts): return PROD_API_URI -def setup_logging(verbosity): +def setup_logging(verbosity, color=True): # explicitly set to stderr just in case # this is the new default for click_log it seems core.ClickHandler._use_stderr = True @@ -105,8 +105,12 @@ def format(self, record): level = record.levelname.lower() msg = record.getMessage() if level in self.colors: - prefix = click.style(f'{level} [{record.name}]: ', - **self.colors[level]) + # `--no-color` governs the log prefix too. Without the flag here it + # styled unconditionally, so `polyswarm --no-color -v …` still emitted a + # green prefix on a tty — the same half-honoured flag the formatters had. + prefix = f'{level} [{record.name}]: ' + if color: + prefix = click.style(prefix, **self.colors[level]) msg = '\n'.join(prefix + x for x in msg.splitlines()) return msg return logging.Formatter.format(self, record) @@ -209,7 +213,7 @@ def polyswarm_cli(ctx, api_key, api_uri, output_file, output_format, color, verb This is a PolySwarm CLI client, which allows you to interact directly with the PolySwarm network to scan files, search hashes, and more. """ - setup_logging(verbose) + setup_logging(verbose, color=color) logger.info('Running polyswarm-cli version %s with polyswarm-api version %s', polyswarm.__version__, polyswarm_api.__version__) diff --git a/tests/cli_test.py b/tests/cli_test.py index 7df6967..e63f405 100644 --- a/tests/cli_test.py +++ b/tests/cli_test.py @@ -7,6 +7,7 @@ from pathlib import Path import vcr as vcr_ +import click from click.testing import CliRunner from polyswarm.client import polyswarm as client @@ -53,9 +54,12 @@ def click_vcr(self, result, name='result', replace=None): yaml.dump(data, f) return entry - def _run_cli(self, commands): + def _run_cli(self, commands, color=False): commands = ['-a', self.api_key, '-u', self.api_url, '-c', self.community] + commands - return self.cli.invoke(client.polyswarm_cli, commands, catch_exceptions=False) + # color=False is CliRunner's default and strips ANSI, which is what every cassette + # expectation was recorded against. Pass color=True only to assert the styling itself. + return self.cli.invoke(client.polyswarm_cli, commands, catch_exceptions=False, + color=color) def _assert_text_result(self, result, expected_result, expected_return_code=0, replace=None): current_result = self._replace(replace, result.output) @@ -319,6 +323,30 @@ def test_search_hash_with_text_output(self): '--output-format', 'text', 'search', 'hash', self.eicar_hash]) self._assert_text_result(result, self.click_vcr(result)) + # Rides the text-output cassette rather than recording its own — the subject is the flag, + # not the response. Two identical GETs replay from the one recorded interaction, hence + # allow_playback_repeats. + @vcr.use_cassette('test_search_hash_with_text_output', allow_playback_repeats=True) + def test_color_flag_reaches_the_text_formatter(self): + # The bug was that `--color/--no-color` never reached the rendering: TextOutput + # assigned `self.color` and read it nowhere. A formatter unit test cannot see this — + # the flag travels through `formatters[output_format](color=color, …)` in the command + # group, and specs/04 says argument parsing and ctx.obj wiring need Style 1 or 2. + # CliRunner's color=True stops click stripping ANSI off the non-tty capture, so the + # two runs differ only in the flag. + colored = self._run_cli( + ['--color', '--output-format', 'text', 'search', 'hash', self.eicar_hash], + color=True) + plain = self._run_cli( + ['--no-color', '--output-format', 'text', 'search', 'hash', self.eicar_hash], + color=True) + assert colored.exit_code == 0, colored.output + assert plain.exit_code == 0, plain.output + assert '\x1b[' in colored.output + assert '\x1b[' not in plain.output + # Same content either way — the flag drops the wrapper, never the text. + assert click.unstyle(colored.output) == plain.output + @vcr.use_cassette() def test_search_hash_with_no_results(self): result = self._run_cli([ diff --git a/tests/known_good_field_test.py b/tests/known_good_field_test.py index 591942f..4865e20 100644 --- a/tests/known_good_field_test.py +++ b/tests/known_good_field_test.py @@ -104,7 +104,10 @@ def test_known_good_with_results_reports_them_instead_of_not_scanned(self): assert 'engine-a: Malicious' in text assert 'engine-b: Clean' in text assert 'Status: Known good' in text - assert 'PolyScore: 0.9' in text + # Anchored on the line, not on a prefix: `PolyScore: 0.9` also matches 0.95, and the + # renderer's :.20f means the line is not the literal '0.9' either. + polyscore_line = next(line for line in text.split('\n') if line.startswith('PolyScore:')) + assert float(polyscore_line.removeprefix('PolyScore:').strip()) == 0.9 def test_malicious_detections_outrank_the_known_good_colour(self): # The colour is the signal here, and no other test can see it (they all unstyle). From 55169a46f847a31fcea2c8e60997bb7f2ca253a9 Mon Sep 17 00:00:00 2001 From: Samuel Date: Wed, 29 Jul 2026 17:13:47 -0300 Subject: [PATCH 09/13] test: cover the log half of --no-color, and mock at the SDK boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups. - The colour test ran at default verbosity, so the level was WARNING and no record ever reached NamedColorFormatter: the log-prefix half of the fix was untested. Added the -v pair. - Both colour tests now mock `Polyswarm.search_hashes` (specs/04 Style 2) instead of replaying a cassette. The response content is irrelevant to whether the flag reaches the renderer, so a cassette would have to be recorded against a live stack for a test that never exercises the server — and borrowing another test's cassette coupled the two through the re-record path, since unittest orders methods alphabetically and this one sorts first. - `_render_styled`'s docstring said `color=True` was "passed only for emphasis". That was true before `_paint`; it is now load-bearing, and saying otherwise invites a future editor to drop it and silently disarm every colour assertion. Corrected in the docstring and in specs/03. --- specs/03-formatters.md | 5 ++- tests/cli_test.py | 67 +++++++++++++++++++++++++++------- tests/known_good_field_test.py | 5 ++- 3 files changed, 60 insertions(+), 17 deletions(-) diff --git a/specs/03-formatters.md b/specs/03-formatters.md index af1aba4..69d78ea 100644 --- a/specs/03-formatters.md +++ b/specs/03-formatters.md @@ -115,8 +115,9 @@ malicious" in green would be a weaker signal than the very same instance produce was reconciled, which is the class of mis-signal this rendering exists to fix. "Status: Known good" stays green in both cases — it labels the catalogue status (the counterpart of "Status: Assertion window closed"), not the verdict. The colour decision is invisible to a test that unstyles its -output, so it is pinned against the **styled** render — `TextOutput` output read without `click.unstyle`, -which is what `_render_styled` in `known_good_field_test.py` exists for. Whether the +output, so it is pinned against the **styled** render — `TextOutput(color=True)` read without +`click.unstyle`, which is what `_render_styled` in `known_good_field_test.py` exists for (both +halves matter: the flag makes it paint, the missing `unstyle` keeps the codes). Whether the `--color/--no-color` **flag** reaches that rendering at all is a separate question a formatter unit test cannot answer; `test_color_flag_reaches_the_text_formatter` (`cli_test.py`) covers it through `CliRunner(… color=True)`. diff --git a/tests/cli_test.py b/tests/cli_test.py index e63f405..9a8c0cd 100644 --- a/tests/cli_test.py +++ b/tests/cli_test.py @@ -3,7 +3,9 @@ import json import yaml import traceback -from unittest import TestCase +from unittest import TestCase, mock + +from polyswarm_api import resources from pathlib import Path import vcr as vcr_ @@ -323,10 +325,37 @@ def test_search_hash_with_text_output(self): '--output-format', 'text', 'search', 'hash', self.eicar_hash]) self._assert_text_result(result, self.click_vcr(result)) - # Rides the text-output cassette rather than recording its own — the subject is the flag, - # not the response. Two identical GETs replay from the one recorded interaction, hence - # allow_playback_repeats. - @vcr.use_cassette('test_search_hash_with_text_output', allow_playback_repeats=True) + @staticmethod + def _one_instance(): + """A minimal parsed instance for the colour tests. They mock at the SDK boundary + (specs/04 Style 2) rather than replaying a cassette: the response content is + irrelevant to whether `--color` reaches the renderer, a cassette would have to be + recorded against a live stack for a test that never exercises the server, and + borrowing another test's cassette couples the two through the re-record path + (unittest orders methods alphabetically, so this one would end up authoring it). + """ + return resources.ArtifactInstance({ + 'sha256': 'a' * 64, 'md5': 'c' * 32, 'sha1': 'b' * 40, + 'mimetype': 'text/plain', 'size': 68, 'extended_type': '', + 'first_seen': '2020-01-01T00:00:00', 'upload_url': '', 'metadata': [], + 'id': '111', 'community': 'gamma', 'assertions': [], 'votes': [], + 'failed': False, 'window_closed': True, 'polyscore': None, + }) + + def _run_color_pair(self, extra_args=()): + """The same command twice, differing only in the colour flag.""" + outputs = [] + for flag in ('--color', '--no-color'): + with mock.patch('polyswarm.polyswarm.Polyswarm.search_hashes', + return_value=iter([self._one_instance()])): + result = self._run_cli( + [*extra_args, flag, '--output-format', 'text', + 'search', 'hash', 'a' * 64], + color=True) + assert result.exit_code == 0, result.output + outputs.append(result) + return outputs + def test_color_flag_reaches_the_text_formatter(self): # The bug was that `--color/--no-color` never reached the rendering: TextOutput # assigned `self.color` and read it nowhere. A formatter unit test cannot see this — @@ -334,19 +363,31 @@ def test_color_flag_reaches_the_text_formatter(self): # group, and specs/04 says argument parsing and ctx.obj wiring need Style 1 or 2. # CliRunner's color=True stops click stripping ANSI off the non-tty capture, so the # two runs differ only in the flag. - colored = self._run_cli( - ['--color', '--output-format', 'text', 'search', 'hash', self.eicar_hash], - color=True) - plain = self._run_cli( - ['--no-color', '--output-format', 'text', 'search', 'hash', self.eicar_hash], - color=True) - assert colored.exit_code == 0, colored.output - assert plain.exit_code == 0, plain.output + colored, plain = self._run_color_pair() + assert '\x1b[' in colored.output assert '\x1b[' not in plain.output # Same content either way — the flag drops the wrapper, never the text. assert click.unstyle(colored.output) == plain.output + def test_color_flag_reaches_the_log_prefix(self): + # The other half of the flag, and it needs -v: at the default verbosity the level is + # WARNING, so no record ever reaches NamedColorFormatter and the formatter-only test + # above cannot observe this branch. The prefix used to be styled unconditionally, so + # `--no-color -v` still emitted a green `info [polyswarm]: ` on a tty. + colored, plain = self._run_color_pair(extra_args=('-v',)) + + # The version line is logged at INFO by the group itself, so it is always present. + assert 'Running polyswarm-cli version' in click.unstyle(colored.output) + assert 'Running polyswarm-cli version' in plain.output + # The prefix names the emitting logger, e.g. `info [polyswarm.client.polyswarm]: `. + assert 'info [polyswarm' in click.unstyle(colored.output) + assert 'info [polyswarm' in plain.output + # Styled only under --color: the prefix is what carries the ANSI here, and the + # formatter output alongside it is white-styled the same way. + assert '\x1b[32minfo [polyswarm' in colored.output + assert '\x1b[' not in plain.output + @vcr.use_cassette() def test_search_hash_with_no_results(self): result = self._run_cli([ diff --git a/tests/known_good_field_test.py b/tests/known_good_field_test.py index 4865e20..24b5248 100644 --- a/tests/known_good_field_test.py +++ b/tests/known_good_field_test.py @@ -50,8 +50,9 @@ def _render(instance): def _render_styled(instance): """Keeps the ANSI wrapper, so the green/red decision itself is observable — every other test here unstyles, which makes the colour (the whole point of the known-good rendering) - invisible. It is the absent `click.unstyle`, not the `color=` argument, that makes the - difference; `color=True` is the default and is passed only for emphasis.""" + invisible. **Both** halves are load-bearing: `color=True` is what makes TextOutput paint + at all (see `_paint`), and the absent `click.unstyle` is what keeps the codes in the + string. Flip either and every colour assertion below stops asserting anything.""" return '\n'.join(TextOutput(color=True).artifact_instance(instance, write=False)) From 7e0eca99a06c1d19497562c17d56bad12ca99097 Mon Sep 17 00:00:00 2001 From: Samuel Date: Wed, 29 Jul 2026 19:57:33 -0300 Subject: [PATCH 10/13] test: mock the colour tests at the real SDK boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups. - _run_color_pair patched Polyswarm.search_hashes, which is CLI code, so it cut utils.parallel_executor_iterable_results out of the run — and the docstring cited it as "specs/04 Style 2", which defines that style as patching polyswarm_api.api.PolyswarmAPI.. Now patches PolyswarmAPI.search: same assertions, correct seam, and the citation is true. - specs/03 and _paint's docstring both credited JSONOutput with honouring --no-color. It has no __init__ and emits plain JSON; PrettyJSONOutput is the one that reads the flag. The contrast is the stated rationale for _paint, so naming the wrong class undercut it. --- specs/03-formatters.md | 10 ++++++---- src/polyswarm/formatters/text.py | 7 ++++--- tests/cli_test.py | 17 +++++++++++------ 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/specs/03-formatters.md b/specs/03-formatters.md index 69d78ea..3b8edde 100644 --- a/specs/03-formatters.md +++ b/specs/03-formatters.md @@ -122,10 +122,12 @@ halves matter: the flag makes it paint, the missing `unstyle` keeps the codes). unit test cannot answer; `test_color_flag_reaches_the_text_formatter` (`cli_test.py`) covers it through `CliRunner(… color=True)`. -**Scope of `--no-color`.** It governs the text formatter (via `_paint`), `JSONOutput`, and the -log prefix (`setup_logging(verbosity, color=…)` — the `NamedColorFormatter` used to style -unconditionally, so `polyswarm --no-color -v …` still emitted a green prefix on a tty). It does -not attempt to suppress colour inside third-party output. +**Scope of `--no-color`.** It governs the text formatter (via `_paint`), `PrettyJSONOutput` +(whose `_to_json` skips the pygments `ClickFormatter` when the flag is off — plain `JSONOutput` +emits unstyled JSON and has nothing to honour), and the log prefix +(`setup_logging(verbosity, color=…)` — the `NamedColorFormatter` used to style unconditionally, +so `polyswarm --no-color -v …` still emitted a green prefix on a tty). It does not attempt to +suppress colour inside third-party output. The **Status** line reads "Known good" whenever `is_known_good`, except on a **failed** instance — "Status: Failed" is ordered first and the known-good **Detections** branch is diff --git a/src/polyswarm/formatters/text.py b/src/polyswarm/formatters/text.py index ecfa890..f765dbd 100644 --- a/src/polyswarm/formatters/text.py +++ b/src/polyswarm/formatters/text.py @@ -909,9 +909,10 @@ def _paint(self, text, fg): """Apply a colour, or don't — the one place `self.color` is honoured. It used to be assigned and never read, so every one of these helpers styled - unconditionally and `--no-color` was a no-op for text output (invisible in practice - because click strips ANSI when stdout is not a tty, and because JSONOutput does - honour the flag). Not decorated with `is_grouped`: the callers already are, and the + unconditionally and `--no-color` was a no-op for text output — invisible in practice + because click strips ANSI when stdout is not a tty. (`PrettyJSONOutput` did honour it, + which is why the flag looked wired up; plain `JSONOutput` emits unstyled JSON and has + nothing to honour.) Not decorated with `is_grouped`: the callers already are, and the indent must be applied exactly once. """ return click.style(text, fg=fg) if self.color else text diff --git a/tests/cli_test.py b/tests/cli_test.py index 9a8c0cd..6f23b7c 100644 --- a/tests/cli_test.py +++ b/tests/cli_test.py @@ -327,12 +327,17 @@ def test_search_hash_with_text_output(self): @staticmethod def _one_instance(): - """A minimal parsed instance for the colour tests. They mock at the SDK boundary - (specs/04 Style 2) rather than replaying a cassette: the response content is + """A minimal parsed instance for the colour tests. + + They mock at the **SDK boundary** — `polyswarm_api.api.PolyswarmAPI.search`, which is + what specs/04 Style 2 means — rather than replaying a cassette: the response content is irrelevant to whether `--color` reaches the renderer, a cassette would have to be - recorded against a live stack for a test that never exercises the server, and - borrowing another test's cassette couples the two through the re-record path - (unittest orders methods alphabetically, so this one would end up authoring it). + recorded against a live stack for a test that never exercises the server, and borrowing + another test's cassette couples the two through the re-record path (unittest orders + methods alphabetically, so this one would end up authoring it). + + Patching `Polyswarm.search_hashes` instead would be the wrong seam: that is CLI code, + so it would cut `utils.parallel_executor_iterable_results` out of the run. """ return resources.ArtifactInstance({ 'sha256': 'a' * 64, 'md5': 'c' * 32, 'sha1': 'b' * 40, @@ -346,7 +351,7 @@ def _run_color_pair(self, extra_args=()): """The same command twice, differing only in the colour flag.""" outputs = [] for flag in ('--color', '--no-color'): - with mock.patch('polyswarm.polyswarm.Polyswarm.search_hashes', + with mock.patch('polyswarm_api.api.PolyswarmAPI.search', return_value=iter([self._one_instance()])): result = self._run_cli( [*extra_args, flag, '--output-format', 'text', From f86a2704e831aeeddc662b5cb0a93c9489615958 Mon Sep 17 00:00:00 2001 From: Samuel Date: Wed, 29 Jul 2026 20:21:20 -0300 Subject: [PATCH 11/13] refactor(text): pick the known-good clause and its colour in one branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. The text keyed on `valid_assertions and window_closed`; the colour keyed on `malicious_assertions and window_closed`. Two differently-shaped conditions that had to agree, held together only by the SDK guaranteeing malicious_assertions is a subset of valid_assertions (both filter on `mask`, one additionally on `verdict`). It is a subset today — verified — so there is no live defect, but a rendering rule in this repo should not depend on a property of another repo's resource class to stay coherent. Each branch now names its own paint function, so the count branch is the only one that can redden and the clause it renders is the one being coloured. --- specs/03-formatters.md | 11 +++++++---- src/polyswarm/formatters/text.py | 27 +++++++++++++++++---------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/specs/03-formatters.md b/specs/03-formatters.md index 3b8edde..b717ae1 100644 --- a/specs/03-formatters.md +++ b/specs/03-formatters.md @@ -106,10 +106,13 @@ every branch the per-engine verdict list, the PolyScore line and "Status: Known render as usual; the switch exists so "it is not scanned" is never printed directly above a list of engine verdicts. -**Colour: any malicious verdict outranks the withheld-bytes signal.** The line is -**green** except when the instance has **at least one** malicious assertion and a closed -window, where it is **red** — the same threshold and the same colour the ordinary branch -gives that count (1/50 reddens there too). Known-goodness is the +**Colour: any malicious verdict outranks the withheld-bytes signal.** The line is **green** +except on the *count* branch (valid assertions, window closed) when at least one of them is +malicious, where it is **red** — the same threshold and the same colour the ordinary branch +applies to that same count (1/50 reddens there too). The clause and its colour are chosen in +the **same** `if`/`elif`, not by a second condition: written separately they had to agree by +coincidence, and only the SDK's guarantee that `malicious_assertions ⊆ valid_assertions` (both +filter on `mask`) kept them in step — a fact about another repo propping up this rendering. Known-goodness is the dominant *fact*, but it is not a stronger *warning*: rendering "40/50 engines reported malicious" in green would be a weaker signal than the very same instance produced before it was reconciled, which is the class of mis-signal this rendering exists to fix. "Status: Known diff --git a/src/polyswarm/formatters/text.py b/src/polyswarm/formatters/text.py index f765dbd..1810eba 100644 --- a/src/polyswarm/formatters/text.py +++ b/src/polyswarm/formatters/text.py @@ -92,26 +92,33 @@ def artifact_instance(self, instance, write=True, timeout=False): # detections and PolyScore preserved. Report those instead of the "not scanned" # clause, which would contradict the per-engine verdicts printed just below. attribution = f' (flagged by: {", ".join(known_good_sources)})' if known_good_sources else '' + # The clause and its colour are decided in the SAME branch, deliberately. They + # were two differently-shaped conditions that had to agree — the text keyed on + # `valid_assertions and window_closed`, the colour on `malicious_assertions and + # window_closed` — and only the SDK's guarantee that malicious_assertions is a + # subset of valid_assertions (both filter on `mask`, one additionally on `verdict`) + # kept them in step. That is a fact about another repo holding this rendering + # together; deciding both at once removes the dependency. if len(instance.valid_assertions) > 0 and instance.window_closed: detections = f'{len(instance.malicious_assertions)}/{len(instance.valid_assertions)} engines reported malicious' + # ANY malicious verdict outranks the withheld-bytes signal, which is the same + # threshold the ordinary branch applies to this same count: green on "40/50 + # engines reported malicious" is a weaker warning than the instance gave before + # it was reconciled. Red only here, because this is the only branch that renders + # a final count. + paint = self._red if instance.malicious_assertions else self._green elif len(instance.valid_assertions) > 0: # Every other branch guards its counts on window_closed, because an open # window's numbers are not final. Not reachable through reconciliation today # (a STORED row carries no assertions and a SETTLED one has a closed window), # stated rather than left implied. detections = 'its scan has not finished running yet' + paint = self._green else: detections = 'it is not scanned' - line = f'Detections: This artifact is a known-good binary{attribution}; {detections}.' - # ANY malicious verdict outranks the withheld-bytes signal for colour (the same - # threshold the ordinary branch uses): this branch replaced one that rendered the - # very same count in red, and green on "40/50 engines reported malicious" is a - # weaker warning than the instance had before it was reconciled. The preserved - # results still mean what they meant. - if instance.malicious_assertions and instance.window_closed: - output.append(self._red(line)) - else: - output.append(self._green(line)) + paint = self._green + output.append(paint( + f'Detections: This artifact is a known-good binary{attribution}; {detections}.')) elif instance.community == 'stream': output.append(self._white('Detections: This artifact has not been scanned. You can trigger a scan now.')) elif len(instance.valid_assertions) == 0 and instance.window_closed and not instance.failed: From cdb7926f3e8d7b6fe92d43faf83e3bbf7e038568 Mon Sep 17 00:00:00 2001 From: Samuel Date: Tue, 4 Aug 2026 15:15:43 -0300 Subject: [PATCH 12/13] fix: floor the SDK pin at 4.3.0 (typed known-good refusal + probe fixes) --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index be2203b..423bab2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,7 +22,7 @@ classifiers = [ ] dependencies = [ - "polyswarm_api>=4.2.0,<5.0.0", + "polyswarm_api>=4.3.0,<5.0.0", "click>=7.1", "colorama>=0.4.6", "click-log>=0.4.0", From b92315f2339bf3975cc25cf5accfdd1a34da2d50 Mon Sep 17 00:00:00 2001 From: Samuel Date: Tue, 4 Aug 2026 15:15:47 -0300 Subject: [PATCH 13/13] =?UTF-8?q?Bump=20version:=204.2.1=20=E2=86=92=204.3?= =?UTF-8?q?.0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pyproject.toml | 4 ++-- src/polyswarm/__init__.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 423bab2..03e289b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -5,7 +5,7 @@ build-backend = "setuptools.build_meta" [project] name = "polyswarm" -version = "4.2.1" +version = "4.3.0" description = "CLI for using the PolySwarm Customer APIs" readme = "README.md" authors = [{ name = "PolySwarm Developers", email = "info@polyswarm.io" }] @@ -51,7 +51,7 @@ include-package-data = true where = ["src"] [tool.bumpversion] -current_version = "4.2.1" +current_version = "4.3.0" commit = true tag = false sign_tags = true diff --git a/src/polyswarm/__init__.py b/src/polyswarm/__init__.py index 0d6a4f2..5ee6158 100644 --- a/src/polyswarm/__init__.py +++ b/src/polyswarm/__init__.py @@ -1 +1 @@ -__version__ = '4.2.1' +__version__ = '4.3.0'