fix: key known-goodness on the instance state, not on the feed list - #262
Conversation
`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.
Review —
|
…tate
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.
|
Review. The formatter change itself is right, and I checked the load-bearing assumptions rather than taking them on trust: in SDK 4.1.0 both 1. The SDK floor should be
polyswarm-api#314 "Release 4.2.0" merged to the SDK's 2. Minor — the assertion loop is not gated by The Nothing else: the |
…ample is unscanned 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.
|
Review — keying known-goodness on the instance state The core change ( 1. A known-good instance with malicious detections now renders green. So the scenario in the TL;DR — a fully scanned malicious sample whose hash is catalogued — still gets a benign-coloured presentation once the server reconciles it to Related coverage gap: no test can observe the colour. Every case goes through 2. It also pins a configuration this PR explicitly declares unsupported — 3. The SDK floor bump is undisclosed and unverified (
If the floor genuinely needs raising for the report-community / 204-streaming reasons the new §Current floor documents, that is a legitimate fix — but it is a separate concern from known-goodness and should at least be called out in the description. 4. The PR body understates the change. 5. Clean
|
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.
Review — clean against the documented conventionsChecked the diff against AGENTS.md and specs/03-formatters.md, 04-testing.md, 05-sdk-contract.md. No correctness, spec-drift, downstream-contract or gitflow defects found. (No code spans below — the review tooling here rejects backticks.) Verified specifically:
Two minor items, both optional:
One precondition I could not verify from this checkout (no network access in the review environment): the two your own new specs/05 §Version pin bullet adds — polyswarm-api 4.2.0 published on PyPI, and the SDK's develop declaring at least 4.2.0. Your commit message says both were checked; worth a second pair of eyes before merge, since the failure mode (CI silently installing PyPI's SDK over the branch archive) is invisible. |
…t goes red specs/03 states it as a rule; _detections_line() filters the styled render down to the Detections line, so nothing observed it.
|
Reviewed against 1. Two places state the wrong mechanism:
The second half of that docstring is right; the Adjacent and pre-existing, but this PR is the one that promotes colour to a documented spec rule: because 2. The spec header says "majority-malicious"; the code reddens on any malicious assertion. 3. Test gap: the open-window colour is not pinned. Nothing else. The |
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.
|
Reviewed against 1. 2. The three-way switch assumes a reconciled row keeps Non-blocking: |
…inned `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.
|
Review The known-good rendering change is correct and matches the specs it updates. Four things that need action, roughly by severity. 1.
res_color = CliRunner().invoke(polyswarm_cli, ['--color', 'search', 'hash', SHA], color=True)
res_nocolor = CliRunner().invoke(polyswarm_cli, ['--no-color', 'search', 'hash', SHA], color=True)
assert '\x1b[' in res_color.output and '\x1b[' not in res_nocolor.outputAny existing 2.
3. The 4.2.0 floor precondition — was the archive's declared version checked, dev suffix included? The new 4. Ticket ID will land in git history via the branch name
The commits and title are clean, but the branch is Non-blocking
|
…o end 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'.
Review — PR #262Base is Four things worth acting on, none of them in the known-good rendering. 1. Undisclosed second change: 2. Test gap: the log-prefix half of that fix is untested. 3. 4. Minor: the branch name carries an internal ticket ID ( Not flagged, for the record: the |
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.
|
Review Correctness of the known-good rendering itself looks right, and it matches Two things to fix. 1. The new colour tests do not mock at the SDK boundary (
The seam one level down works identically here: patch 2. Both Notes, no action needed
|
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.<method>. 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.
|
Review Clean against the documented conventions on gitflow (base 1. The known-good colour predicate does not match the text predicate — The text switches on Concretely: an instance whose only assertion has
2. The floor precondition names the wrong archive — The new Current floor bold line is right ("must be satisfied by the SDK archive CI installs, and by PyPI"), but it then narrows the precondition to "the SDK's 3.
Not flagged
Merge Squash-merge, per the PR body: the branch name carries an internal ticket ID and a default merge commit would embed it in this public repo's history. |
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.
|
Review — PR #262 Checked the diff against The rendering change is correct. One thing needs action. 1. The Merging section asserts a paired, unlanded SDK branch:
Worth resolving before merge, because the second reading also changes the merge instruction the PR gives. |
|
This one ran against an earlier revision of the description — the current body already takes reading B, and explicitly.
Re-verified the premise rather than taking the body's word for it, since it's the whole basis for reading B: SDK One correction on the rename remedy: a ticket ID in a branch ref is explicitly permitted by the workspace convention for these public repos — what must stay clean is the published commit/PR text, which is why the ID would only become a problem via a default merge commit's subject. Hence squash-merge, with rename as the equally valid alternative. Leaving the choice with the maintainer. No code change needed; 118 passed, CI green. |
2 participants
TL;DR
is_known_goodwasstate == 'KNOWN_GOOD' or bool(known_good_sources). The state alone now decides.N/M engines reported malicious) — in red when any engine reported malicious, so the colour is no weaker than before the sample was reconciled.polyswarm_api>=4.2.0. Unrelated to known-goodness; two other behaviours the CLI depends on fail silently on 4.1.0.Context
The flag drives the Detections line, and when it is set the CLI replaces the engine verdicts with a known-good sentence.
The feed-list fallback was harmless only because the server emitted that list exclusively for instances already in the known-good state. It no longer does: the list is now present whenever the server's current understanding of the file is known-good, including on a fully scanned instance with real detections and a PolyScore. The state is the single reliable signal for "known-good, bytes withheld", so it alone decides and the feed list only shapes the message.
Re-verified against artifact-index's known-good model rework (#1919, DN-8425 v2): no CLI change is needed.
statestill reportsKNOWN_GOODexactly while the file is currently known-good, andknown_good_sourcesstill accompanies it, so both reads this PR makes are unchanged. The rework also hands the CLI a case it now renders correctly for free: a submission the server declined as known-good whose hash has since been un-flagged reports the newNOT_STOREDstate, which the CLI prints as an ordinary state string rather than claiming "known-good … it is not scanned" about a file that is scannable again.Changes
Rendering —
src/polyswarm/formatters/text.pystateonly;known_good_sourcesis read after it and short-circuits to[]when not known-good. Keeps the defensivegetattrfor older SDKs.N/M engines reported maliciouswhen there are valid assertions and the window is closed, "its scan has not finished running yet" when the window is open (parity with every other branch, which guards its counts onwindow_closed), "it is not scanned" otherwise.Status: Known goodstays green — it labels the catalogue status, not the verdict.A second, unrelated fix:
--no-colorwas a no-op for text output.TextOutputassignedself.colorand read it nowhere, so every_white/_red/… helper styled unconditionally; thelog prefix did the same. Both now honour the flag (
formatters/text.py_paint,client/polyswarm.pysetup_logging(verbosity, color=…)), covered by twoCliRunnertests(
tests/cli_test.py). It rides along because the colour rule this PR documents isunenforceable while the flag is dead — but it is a user-visible change to every command run with
--no-coloron a tty, so it belongs in a release note rather than being discovered later.Dependency —
pyproject.tomlpolyswarm_api>=4.0.0→>=4.2.0. The known-good attributes ship in 4.1.0, so this is not what the fix needs; the floor is set byllm_report_createsending the client's community, and a streaming204 No ContentraisingNoResultsException(the no-results exit code1). Both are silent no-ops on 4.1.0. Verified before raising it: 4.2.0 is published to PyPI, and the SDK'sdevelopdeclares4.2.0, so CI's archive install is not silently upgraded over by PyPI.Docs —
specs/03-formatters.md,specs/05-sdk-contract.md,AGENTS.md03: the state as the signal, the feed list as message-shaping, the three-way results clause, and the colour rule.05: a## Current floorsection explaining the two behaviours above, plus the two preconditions any floor bump has to satisfy (published on PyPI; declared by the SDK'sdeveloparchive).AGENTS.md+specs/04-testing.md: Style 3 — pure rendering logic unit-tested against the formatter directly, with the rule that a Style-3-covered command still needs oneCliRunnertest proving the command reaches the formatter.Tests —
tests/known_good_field_test.pyTextOutput(color=True)and assert the ANSI wrapper — every other test unstyles, which made the green/red decision (the point of the known-good rendering) unobservable.Tests
pytest -s -v— 115 passed, 0 failed.Requires
Traceability, not merge ordering: this change is correct both before and after that one lands — against an
older server
stateis simply absent and the instance renders as an ordinary one, which a test now pins.installs this branch's SDK archive because the branch names match (
.gitlab-ci.ymltries$CI_COMMIT_BRANCH.zipbeforedevelop.zip), so a reviewer should be able to see what is actually beingtested against. The CLI itself needs only released SDK surface: the floor is
>=4.2.0, satisfied by bothPyPI and the SDK's
develop, and none of that PR's new surface is used here.Merging
Squash-merge, or rename the branch — either is fine. The branch name carries an internal
ticket ID, so a default merge commit (whose subject embeds it) would put that ID in this public
repo's permanent history. The commits, title and description are clean (AGENTS.md §Commit +
hygiene).
To be precise about the branch name, because an earlier revision of this section overstated it:
CI does install the same-named
polyswarm-apibranch archive (.gitlab-ci.ymltries$CI_COMMIT_BRANCH.zipbefore falling back todevelop.zip), so while both branches exist thisPR is tested against the paired SDK — which is useful. But nothing here requires it. The
floor this PR sets (
>=4.2.0) is satisfied by the SDK'sdevelopand by PyPI, both verified,and none of the SDK's new surface (
KnownGoodWithheldException) is used by the CLI. So the nameis a convenience, not a dependency, and renaming the branch is a valid way to solve the
public-history concern outright.