feat: ruleset_list(sort=) for the server's active-first order (4.5.0) - #324
Conversation
…r-side `ruleset_list(sort='active_first')` asks the server for rulesets with a running live hunt first — as recorded by the server's live-hunt link, the same one `livescan_id` renders from — newest first within each block (`GET /v3/hunt/rule/list?sort=active_first`). Unset sends no `sort`, so the request stays byte-compatible with the pre-sort contract and the list keeps its newest-first default. The SDK never re-orders rows: the list is keyset-paginated, so a client-side sort would reorder one page and misrepresent the rest; a page's `offset` is only valid under the same `sort`, and the server refuses a cursor minted under the other order. Canonical change in aio/api.py; api.py is the regenerated unasync mirror (scripts/regenerate_sync.py, ruff on PATH). YaraRuleset.list already forwards arbitrary keywords through core._params, so the resource needs no change. Tests on two tiers. Pure-unit pins the wire shape on both transports, the omitted default, composition with the filters, and that `_next_page` carries `sort` onto page 2. Live-e2e (sync + async, cassettes recorded against a stack running the server branch) pins what no builder test can: the server actually applies the order — this test's running ruleset precedes its newer idle one under `sort='active_first'` and follows it under the default — and refuses an unknown sort rather than ignoring it.
The CLI client adopts `ruleset_list(sort=)` in the paired change set and expresses that as `polyswarm_api>=4.5.0` rather than probing the installed SDK (the workspace's cross-repo dependency standard; AGENTS.md's standing exception). A floor cannot name a version this repo has not declared, so the bump lands here, in the feature PR, not at the release step. Minor, not major: one new optional keyword with a default that preserves today's behaviour. Bumped with bump-my-version; the emitted string is a clean `4.5.0` (a `.devN` form would sort below the floor and send the CLI's CI to PyPI for a version that does not exist). Order is forced as before: this repo releases before the CLI can.
…he endpoints table
…pe by id The server assigns that obligation to clients and pins it with boundary tests; it appeared nowhere on this side. Documented on the async method (the canonical source), regenerated into the sync mirror, and recorded in the endpoints table. No behaviour change: dedupe does not belong in the shared streaming generator, which every list endpoint uses and which must not grow an unbounded id set for one of them.
…livescan_id renders Three separate things the sort docstring got wrong or over-promised: The rank is not "the same link livescan_id renders from". The server orders on the stored link and serializes the id under a stricter predicate, so a legacy row whose hunt was stopped without clearing the link leads the list while rendering a null id. A caller reading the leading block as "running" — or taking rows until the first null id — reads it backwards. The docstring now says to read the field and never the position. "A fresh walk from the first page is always self-consistent" was wrong in the same breath as the duplicate it describes: the mutable key repeats and skips rows WITHIN a walk, so starting fresh does not avoid it. Retracted. And the live ordering test's poll called list.index() on a row a lagging replica may not have returned yet. poll_equals absorbs NotFound/NoResults, not ValueError, so the lag the poll exists for would have errored the test on its first attempt instead of retrying. Both twins now read as "not yet".
The sort paragraph called the unsorted list "the id-desc default", which reads as a promise about the id callers can see. It is not one: the server orders on its own insertion key and renders a random 17-digit number as id, so a caller who recorded the smallest id yielded and resumed below it would silently skip or repeat rows. The dedupe advice in the same paragraph stands — that only needs uniqueness — and now says so explicitly.
|
Reviewed against The mechanics are right: 1. The 4.5.0 bump has no sibling PR to justify it. Either open the paired CLI PR and link it (as #321 ↔ polyswarm-cli#266 did), or drop 2. 3. The default-order assertion is the one unpolled read of a helper built to return assert _running_precedes_idle() is False
assert poll_equals(lambda: _running_precedes_idle(), False)
Nit, not blocking: the branch name carries |
…uilder test Two leftovers from the ordering correction, both caught in review. The default-order assertion was the one unpolled call of a helper that now returns None while either row is missing — deliberate, so the poll can retry. Unpolled, a lagging read replica turns that into 'None is False' instead of a retry, against the invariant that these pass on the live stack with VCR off. It is polled now, with want=False, which the helper's own guard accepts. And the builder test's class docstring still called the unsorted list id-desc, the exact claim the rest of this branch exists to correct — the cassette shows the default page returning the lower visible id first.
|
Clean against the documented conventions — no correctness, spec-drift, contract, coverage, or gitflow issues found. Checked and confirmed:
One minor thing, worth handling before merge rather than after: Branch name carries a ticket ID. |
…es separately The server gained the inverse of favorites_only: a paginated list with the favorites taken out. It exists because the favorites are a separate, unpaginated fetch bounded by the account's budget, so leaving them in the page too makes a client either render a row twice or render a short page. Appended to the signature rather than placed beside favorites_only, so a caller passing the later filters positionally keeps working.
…till untested The only coverage the parameter shipped with called the generic resource builder, which this change does not touch — dropping the keyword from both transports left the whole suite green. The new cases drive the sync and async client methods, and deleting the pass-through now fails exactly one test. The e2e arm specs/04 asks for is still missing, and the comment at the live sort test says so plainly, along with what compensates and what does not: a rename made in lockstep with the server's spelling is the case only a live request catches. Recording it needs a stack whose key-management service carries the fixture account.
|
Reviewed against Clean on the things that usually go wrong here: base is One finding. The
That is not what the committed cassettes show. So the case the comment correctly identifies as uncovered — a token renamed in lockstep with the server's spelling, which the builder and pass-through tests cannot see because the server ignores unknown query args — is not blocked by anything. It is two assertions inserted into the existing lifecycle tests at excluded = {r.id for r in api.ruleset_list(exclude_favorites=True)}
assert rule.id not in excluded
assert rule.id in {r.id for r in api.ruleset_list()}The second line is what makes it a rename detector: an ignored (renamed) token collapses the two reads into the same set and the first assertion fails. Then delete Two smaller things fall out of the same block. It documents a gap in Non-blocking
|
|
Approving. Nothing below blocks the merge. The one MODERATE is an edge whose mechanism predates this branch, and the rest is documentation — land any of it here or in a follow-up, whichever you prefer. SummaryAdds an opt-in active-first order to the ruleset list — rulesets with a live hunt ahead of idle ones — plus an Severity: 0 HIGH · 1 MODERATE · 5 LOW (set-wide). Prior feedback: 12 checked · 7 open (set-wide).
Fixes are proposed, not applied; nothing was run; no independent fix review in this run. Cross-repo coordination
Private companion repos are referred to by category here rather than by name, per this repo's Merge order: the server-side API PR → this PR → polyswarm-cli#270 (§14: the API is the contract; and #270's floor cannot resolve until this one declares 4.5.0 on
Fixes are independent; F1 is the only one spanning members, and its own fix names both sides. Findings (round 1)Every finding below is work for this change set; each entry's [MODERATE] F1. A ruleset walk whose last page empties exits 1 as "no results"Lands in: polyswarm-api (this PR) · Also touches: What happens:
Proposed fix (untested): In try:
request = await self._next_page(request)
except exceptions.NoResultsException:
# A 204 on a page after the first ends the walk: rows were
# already yielded, so it is not the no-results signal. The
# first page's 204 still raises, from _paginate/_single
# outside this generator.
logger.debug('Ending pagination: the next page had no content.')
return
elsewhere: F2 → the two internal repos (AI-attribution trailers on their commits and PR bodies — this repo and polyswarm-cli are clean, because Outstanding review feedback
Addressed and not printed: the round-1 point that the 4.5.0 bump had no sibling PR to justify it — polyswarm-cli#270 now exists, pins Standards conformitySet-level. §14 delivery order is satisfied — one externally-facing capability, API + SDK + CLI in one change set, no UI ahead of it, exercised through this SDK against a running stack. Rule 6 name identity holds byte-for-byte across all four repos. Project-level (non-clean rows only). None. polyswarm-api — Clean: §14, §15, §16. Not applicable: §2–§13, §17. Checked and clean, so not reported: |
sbneto
left a comment
sbneto
left a comment
There was a problem hiding this comment.
Reviewed. Nothing blocking. One MODERATE lands here — a 204 on a page after the first raises NoResultsException out of _consume_results instead of ending the walk, so a complete listing exits 1; the mechanism predates this branch and the mutable sort key is what makes it routine. Plus three LOW (no upstream ordering note in the body, the e2e waiver's stated blocker contradicted by the committed cassettes, exclude_favorites left unversioned in the endpoints table). Merge after the server-side change and before polyswarm-cli#270. Details in the review comment; fix here or in a follow-up.
sbneto
left a comment
sbneto
left a comment
There was a problem hiding this comment.
Reviewed as part of the DN-8445 change set alongside polyswarm/portal#2535. No findings land in this PR; the contracts it produces (the active-first sort, the two-directional cursor guard, and exclude_favorites) were re-derived against the consumer and are correct.
2 participants
What
ruleset_list(sort=None)— an opt-in pass-through for the API's new active-first rulesetorder, where rulesets carrying a live hunt link come back first. Omit it and the call is
byte-for-byte what it was.
Written on the async method, which is canonical, and regenerated into the sync mirror with
scripts/regenerate_sync.py.Why the 4.5.0 bump rides this PR
The standing exception: the sibling raises its
polyswarm_api>=floor to the version thatintroduces the surface, and its CI resolves this repo from source by branch name. The paired
PR is polyswarm-cli#270, which pins
polyswarm_api>=4.5.0forruleset_list(sort=); itspipeline installs
$POLYSWARM_API_ARCHIVE/$CI_COMMIT_BRANCH.zipand that branch is pushedhere under the identical name, so the floor is satisfiable before 4.5.0 reaches PyPI.
Two things callers have to know
The rank is the stored hunt link, which is wider than what
livescan_idrenders from.The server orders on the link and serializes the id under a stricter predicate, so a legacy
row whose hunt was stopped without clearing the link leads the list while rendering a null
id. Read the field to decide what is running, never the position in the list.
The key is mutable, unlike the id-desc default. A ruleset whose live hunt stops
part-way through a walk falls back into the idle block below the cursor and is yielded
twice; one started part-way through moves above the cursor and is skipped for the rest of
that walk. Starting fresh from the first page does not avoid it — it is a property of the
walk, not of a stale cursor.
The generator streams pages and deliberately does not dedupe. It is the shared streaming
helper behind every list endpoint, and giving it an unbounded id set for the sake of one
caller-opted sort is the wrong trade. Callers consuming more than one page dedupe by
id.All of this is documented on the method, mirrored into the sync client, and recorded in the
endpoints table.
exclude_favoritesA second opt-in filter, appended to the signature so a positional caller keeps working. It is the
inverse of
favorites_onlyand the server refuses the pair. It exists for clients that render thefavorites as their own list: leaving them in the paginated list too makes a page repeat a row or
come back short.
Verification
Full suite green (228), plus recorded live-request tests for the sorted walk on both the sync
and async clients. Both ordering reads — sorted and default — are polled in a
membership-tolerant form, so a lagging read replica retries rather than raising out of the
poll or asserting on a
None.