Hunt-page ruleset tracking: favorites, rule counts, hunt provenance - #321
Hunt-page ruleset tracking: favorites, rule counts, hunt provenance#321vhmartinezm wants to merge 25 commits into
Conversation
…nance New fields parsed on existing resources (all additive; an older server leaves them None): - YaraRuleset: favorite, favorited_at, rule_count (None means the server had no answer, distinct from 0), historical_hunt_count, and new_results_count (only when the list is asked to include counts). - HistoricalHunt: rule_id (the source ruleset), rule_modified (freeze-time audit value), and source_rule_changed — a tri-state answering "has the source ruleset's body changed since the hunt froze it?" (None = unknown, not 'unchanged'). New endpoints and filters: - ruleset_favorite(id, favorite): idempotent star/unstar; the response carries favorites_used/favorites_limit; over-budget refusals surface a machine-readable FAVORITE_LIMIT error. - ruleset_list(name=, status=, favorites_only=, has_new_results=, since=, include_counts=): the hunt-page filters, conjunctive and optional. - live_results_count(since=): per-live-hunt result counts in a window, one aggregate for every 'new results' badge. - live_feed(livescan_id=): scope the feed to one live hunt. Sync and asyncio clients both. The rules live-suite tests now create a uid-namespaced single-rule ruleset (deterministic rule_count, no name collisions on the shared stack) and exercise the favorite round-trip, provenance, counter increment, and the changed-since-freeze flip; their cassettes are removed to re-record against a stack that serves the new fields.
The sync api.py was hand-edited; scripts/regenerate_sync.py places live_results_count in aio's order and applies ruff's formatting, which is what the unasync-mirror CI gate diffs against. The rules live-tests' three read-after-write assertions (the counter, and both sides of the changed-since-freeze flip) now poll: those GETs read the replica, and on a real-replica stack the stale read of the flip is a silent False. Sleeps are free on VCR replay.
Ids exceed JavaScript's safe-integer range; the counts entries carry the same digit string YaraRuleset.livescan_id does.
Recorded against the branch server image (both tests green live first); the offline suite replays them — 163 passed with no stack.
|
Reviewed against
Nothing under
No cassette in
Per
Related, minor:
|
Review findings, all four: - specs updated in the same PR as required: 03-endpoints gains ruleset_favorite / live_results_count rows, the real ruleset_list signature and live_feed's livescan_id; 02-resources catalogues both new resources — including why YaraRulesetFavorite empties RESOURCE_ID_KEYS (the server reads the toggle from the PUT body; the empty key list is the only thing routing id there) and the bool→int body serialisation; 05's commonly-imported list carries both. - the rules live-tests now exercise every previously-uncovered surface against the real stack (and the cassettes record it): live_start → status=active filter → include_counts observed as a computed 0 (distinct from null) → live_results_count (our zero-result hunt ABSENT from counts, keyed by the same digit strings ruleset_get renders) → the livescan_id-scoped feed → live_stop, with the stop in a finally because a running hunt blocks ruleset deletion. - pure-unit builder tests (hunt_tracking_builder_test.py, the known_good_test pattern) pin the request shapes: the favorite PUT's body routing incl. 1/0 bools, counts query routing + None omission, the list filters' int bools and byte-compatible no-filter request, and livescan_id stringification. - the unstar stays a contract assertion with slot hygiene documented: ruleset_delete soft-deletes and the budget counts only deleted=false rows, so a failed run's star frees itself with the rule. The limit pin vs used bound is now commented as deliberate. Also: test/eicar.yara deleted (no test references it since the uid_yara move; the helper docstring no longer names the file).
|
All four addressed in 40bc463: specs 02/03/05 updated in-PR (including the RESOURCE_ID_KEYS=[] rationale and the 1/0 body bools); the live tests now exercise every flagged surface against the real stack and the cassettes record it — live_start → status=active → include_counts observed as a computed 0 → live_results_count (zero-result hunt absent, digit-string keys matching ruleset_get) → livescan_id feed → live_stop-in-finally; pure-unit builder tests added (hunt_tracking_builder_test.py, the known_good pattern); the unstar/slot question is answered in a comment (ruleset_delete soft-deletes and the budget counts deleted=false only, so a failed run self-heals) with the limit-pin-vs-used-bound distinction made explicit; test/eicar.yara deleted. |
|
Reviewed against 1. 2.
The last commit correctly notes nothing references 3. The 4. 5. (minor) |
…ew 2)
All five follow-ups:
- live_results_count moved to the _single Live-hunts table in
specs/03 — it returns one resource, and _single-vs-_paginate is that
document's organizing invariant.
- specs/04's fixture inventory drops the retired test/eicar.yara and
names the new pure-unit module.
- Parse-side pins for the counts resource: the cassettes only carry
EMPTY counts (fresh zero-result hunt), so the {livescan_id, count}
entry shape, the digit-string join key and the null-counts coalesce
now have canned-payload tests.
- The livescan_id feed assertions no longer read as if they verify the
scoping: with a zero-result hunt they pin the wire shape and the
empty pass-through only, and the comments now say so (the scoping
semantics are pinned by the server's own HTTP suite).
- FAVORITE_LIMIT's machine-readable contract is now documented
(specs/05: no typed exception by design; the path is
exc.request.errors with the code plus the same counters a successful
toggle returns) and pinned by a respx refusal test — mocked because a
genuinely full budget on the shared stack would race every other run.
|
All five addressed in b80f825: the counts row moved to the _single Live-hunts table (it returns one resource); specs/04's inventory drops the retired fixture and names the new module; parse-side pins added for the counts entry shape, the digit-string join key and the null-counts coalesce (canned payloads — the cassettes only carry empty counts by construction); the feed assertions' comments now say exactly what they pin (wire shape + empty pass-through — the scoping semantics are pinned by the server's own suite); and FAVORITE_LIMIT's machine-readable contract is documented in specs/05 (no typed exception by design; the path is exc.request.errors with the code plus the same counters a successful toggle returns) and pinned by a respx refusal test, mocked because a genuinely full budget on the shared stack would race every other run. |
|
Reviewed against Three things worth acting on, all in tests/docs. 1. The favorite round-trip is not actually pinned on VCR replay. Star and unstar are both 2. 3. Latent: the empty Minor / no action needed: |
|
Review Src side is clean: Four things worth action. 1. Every recorded
Both are cheap to add inside the existing running-hunt block, and one is an assertable negative: the hunt has zero results, so 2.
3. specs/04 invariant 7: "Prefer the pure-unit tier for builder + parse logic … Use this tier for any bug that can be reproduced without network involvement." This test asserts exactly one thing — a 400 envelope populates If it stays a respx body, invariant 5 requires a new off-harness respx body to "say why in its docstring." The comment argues respx-over-e2e (fair — you cannot hold five team slots on a shared stack) but not why it is sync-only rather than on 4. Ticket ID in the branch name.
Minor. |
…ts died at get_sources)
ReviewChecked against Verdict: the src-side change is clean. All four touched specs are updated in the same PR, the resource/builder plumbing is right ( 1.
|
ReviewBase ( 1.
|
Review — hunt-page tracking, measured against the platform query-design and delivery-order standardsReviewed against our org-wide project standards — §13 Query design (no aggregate computed on a request path; client-visible counts are stored columns refreshed by a scheduled job with an observable staleness marker) and §14 Delivery order (a capability ships API → SDKs and CLI → UI, and a unit of work is a capability, never a layer) — plus the design decisions settled on the server side of this change. What's clean. The cross-repo mechanics are right: identical branch name, base Findings[MODERATE] F1. Two of the new surfaces wrap a server aggregate that is being withdrawnWhat happens: When: On merge, if the server change lands as designed. The SDK is the published contract, so a surface shipped here is one we then have to support or break. Why:
Proposed fix (untested). Remove, on this branch:
Keep Then add [MODERATE] F2.
|
Review applied — head
|
ReviewBase is 1.
|
13f4e48 to
e292cc1
Compare
|
All four applied at head:
On the branch name: agreed, squash-merge with a clean subject on the day (same note as the CLI PR). |
|
Reviewed against AGENTS.md + Verified in detail:
One item, low severity:
Nothing else blocking. |
|
Review Correctness, spec alignment, and the downstream contract all check out. Verified against the code rather than the prose:
Three things worth acting on, none blocking. 1. The tri-state
One pure-unit parse test alongside 2.
3. They gained |
e292cc1 to
8b57309
Compare
8b57309 to
68ec25c
Compare
|
Reviewed against AGENTS.md and Overall: clean. Builder shapes check out against Two things worth acting on: 1. The PR calls this hazard out explicitly for by_name = [r.id for r in api.ruleset_list(name=uid)]
assert rule.id in by_name
...
favorites = list(api.ruleset_list(favorites_only=True))
assert any(r.id == rule.id and r.favorite for r in favorites)Both hold identically if the server drops the query param and returns the unfiltered list — which is exactly the regression an SDK-side filter test should catch, and the one most likely to happen when the paired server change lands. Tighten to the universal arm: by_name = list(api.ruleset_list(name=uid))
assert rule.id in {r.id for r in by_name}
assert all(uid.lower() in (r.name or "").lower() for r in by_name)
...
favorites = list(api.ruleset_list(favorites_only=True))
assert any(r.id == rule.id for r in favorites)
assert all(r.favorite for r in favorites)No re-record needed — the requests are unchanged and both recorded responses carry a single row that already satisfies the stronger form ( 2. The branch name carries the internal ticket ID into public history
Smaller notes, no action needed:
|
|
Reviewed against What I checked:
One low-priority nit, take it or leave it:
|
|
All applied at head:
Thanks for catching the teardown one — that was a real leak risk on the shared stack. |
SummaryAdds the hunt page's backend: a team-shared favorite star with a 5-slot budget, ruleset-list filters, a per-hunt "new results" badge kept in a stored column by a scheduled job, a per-hunt feed scope, and provenance linking each historical hunt back to the ruleset it froze — plus this SDK and the CLI that read all of it, and the deploy entries that run the jobs. 4 PRs on the shared branch name; 93 files, +7320/−1277 across the set. This member is 16 files, +2435/−366. Severity: 0 HIGH · 4 MODERATE · 6 LOW. Prior feedback: 54 checked · 4 open. Fixes are proposed, not applied; nothing was run; findings verified by single-pass reading against the current heads. Cross-repo coordination
Merge order: internal API → this PR → polyswarm/polyswarm-cli#266, with the chart deployed alongside the server image. §14: the API is the contract.
Coherence: F3's wording depends on F5's unit decision — F5 lands first and F3 states whatever unit it lands. Do not apply F3 in isolation, or the docstring will be corrected to a unit that is about to change again. Findings (round 2)Every finding below is work for this change set; each entry's [MODERATE] F3. The docstring documents the inverse of the server's
|
| Status | Raised | The ask | Disposition |
|---|---|---|---|
| not addressed | round 1 | move the in-flight since=0 note out of the shipped docstring |
→ F3 |
| not addressed | round 1 | narrow specs/99 §2, contradicted by the new tests |
→ F8 |
| not addressed | round 1 | tighten the presence-only filter assertions to the universal arm | → F9 |
| open — not a defect | both public repos | The branch name carries an internal ticket id into public history. The cross-repo CI seam matches on branch name, so it cannot be renamed now; a squash merge with a clean subject satisfies both and needs no rebase. Nothing in the diff can settle it — it is a merge-time action, and it is the only thing between this repo and the no-internal-refs rule. | — |
Everything else raised here is addressed, including the three items applied at the current head (the absent-arms parse tests, the nested teardown finally, and the long-pole fragment).
Standards conformity
The project-level audit stands from round 1. This round introduces no new change-level violation here.
Set-level rows, which belong to every member:
- §14 delivery order — the set is one capability (API + SDK + CLI + deploy) with the UI legitimately following. Coverage is incomplete: the feed scope and the list filters have no CLI leg.
→ F4 - Rule 6 name identity — clean. All four branches are byte-identical, so the harness resolved every sibling at the matching image tag and CI exercised the change together.
## Requireslinkage — present on all four, but two entries misdescribe the deploy member.→ F2
The docstring said SECONDS and claimed the server was tightening the parameter to `is not None` so that since=0 would mean an empty window. No such change was ever made, and the opposite is the contract: the server applies the filter on a truthiness test, so absent-or-0 means no time filter at all and the feed pages over everything. The unit is now minutes on the wire, which is a break worth naming — a caller passing an explicit since in seconds gets a 60x wider window, silently — so specs/05 records it as a behaviour change rather than a documentation correction, for the develop -> master bump decision. max_results is additive and defaults to None, the historical behaviour: every page, up to the client's page cap. It bounds the yielded count and sizes the page request via core.page_size_for, so a small ask does not fetch a full default page. The bound is client-side by nature — the server has never served an unbounded query, it caps every page; this client is what follows cursors until has_more clears. page_size_for lives in core.py: pure, shared by both transports, and not unasync-processed, so there is exactly one copy. Sync mirror regenerated.
It said live_start never gets a livescan_id on the local e2e stack. The rules tests now assert the opposite as a hard contract and test/vcr/test_rules.vcr records a real one. What is still true is that no microengines process submissions, so the feed stays empty — which is what the zero-result feed check in those tests rests on.
`rule.id in by_name` and `any(... and r.favorite ...)` both hold if the server ignored the query param and returned the unfiltered list — which is the regression an SDK-side filter test exists to catch, and the one most likely when the paired server change lands. Both arms now assert over every row. No re-record: the recorded responses already satisfy the stronger form.
Truncation and page-size selection are both decisions the code now makes and neither had a test. Pinned in the pure-unit tier: producing more feed rows than a page holds on the shared e2e stack would mean generating real live-hunt volume, and _paginate is the seam that would otherwise keep following cursors.
The truncation tested `is not None` while page_size_for treats 0 as no bound, so max_results=0 asked for a server-default page and then stopped after the first result. 0 now means no bound on both halves, which is also what `since` means by 0 on the same call.
|
Reviewed against Three things need action. 1. Spec drift — That is the pre- 2. Correctness — The feature reads # aio/api.py:533 (and api.py:608)
yield item
yielded += 1
if max_results is not None and yielded >= max_results:
returnWith 3. Minor — Non-blocking: the PR description still says " |
|
Reviewed against 1.
Three surfaces say minutes, one spec plus the PR body say seconds. This is a 60x window either way for anyone who reads the wrong one, and the SDK ships no conversion — it forwards 2. The bump decision reads the PR body, and the PR body contradicts
3.
4. Test gap: nothing pins that
So Same test also only covers the sync client; the async |
max_results read 0 two contradictory ways: page_size_for treated falsy as unbounded while the generator tested `is not None`, so max_results=0 asked for a full server-default page and then stopped after one row. Negatives were worse — page_size_for returned -1, which would have put limit=-1 on the wire and the server answers that with nothing. as_result_bound is now the single definition (None/0/negative -> no bound) and both halves read it, so they cannot disagree again. The bound and the page it implies stay separate numbers on purpose: a caller asking for 5000 gets 1000-row pages and still stops at 5000. Also pins the wiring itself. The truncation tests replace _paginate wholesale, so the descriptor was never built and dropping `limit=page_size_for(...)` entirely kept every test green; TestLiveFeedLimitOnTheWire fails without it (verified), and asserts the unbounded case sends no limit at all, which is what keeps the default request byte-compatible with the recorded cassettes. specs/01, specs/03 and specs/05 pick up the new core symbols, the MINUTES unit and the absent-or-0 contract — specs/03 still documented SECONDS, contradicting the docstrings and specs/05 inside the same change.
ReviewArchitecture, gitflow, and spec discipline are clean: canonical async edited first with the sync mirror regenerated, resources stay pure, both live-e2e VCR bodies + pure-unit builder tests + a dual-transport Four things worth acting on, none blocking: 1. 2. 3. 4. Branch name |
…r arm 'rules' is a prefix of 'ruleset', so the long-pole fragment also matched the instant ruleset_favorite respx suite and scheduled it ahead of real long poles. 'test_rules' matches only the two intended nodeids. Nothing covered an explicit favorites_only=False / has_new_results=False: they serialise to query 0 (core._params coerces bools before routing), and an inverted filter is the one failure mode that silently returns the wrong rows. Also records where MAX_PAGE_SIZE comes from — the server's AI_MAX_QUERY_RESULTS — and why it sits in core.py rather than settings.py.
ReviewClean against Three things worth fixing, all minor. 1.
2. 3. Async |
The long-pole fragment went from 'rules' to 'test_rules' to fix a false positive and introduced a false negative: 'test_async_rules' is test_ + async_rules, so the heaviest new test in the suite fell to the backfill tail. '_rules' matches both nodeids and still misses the respx suite, where the character before 'rules' is a slash. The replaced line also left its old trailing comment behind. Every max_results test drove the GENERATED sync mirror. The async for + yielded/return shape is the part unasync rewrites rather than copies, so the canonical loop had no coverage at all — verified the new tests fail when the canonical bound check alone is broken.
|
Reviewed against Correctness — clean.
Spec drift — none; specs Downstream contract — Test coverage — the Gitflow — base is Nit (non-blocking, no code change): the branch name |
The wire is not moving to minutes (prod traffic makes it a silent 60x widening for clients we don't control), so this is a documentation correction again rather than a behaviour change — which also removes it from the develop -> master bump decision. max_results and the absent-or-0 contract are unaffected. Also shortens the core helper comments: the argument belongs in specs/05, not beside every line that touches it.
|
Review — hunt-page ruleset tracking Checked against 1.
Every test of this path stubs
2. The favorite read-after-write does not poll, unlike every other one in the test
This is the sharpest read-after-write in the test — the star is written on the line above — yet it is the only one that neither polls nor guards 3.
This PR adds optional kwargs to two public methods ( 4. Docstrings pre-commit a release number
|
MAX_PAGE_SIZE mirrored the server's AI_MAX_QUERY_RESULTS code default of 1000, but the chart sets 300 in every environment — so live_feed(max_results=500) would have sent limit=500 and got a 400. The cap is an env var the deployment chooses, so the SDK cannot know it: max_results now bounds only how many results the generator yields, and the request is unchanged (which also keeps the default call byte-compatible with every cassette). Also polls the favorite read-after-write — the star is written on the line above, and it was the one such assertion here that neither polled nor guarded 204. One read per attempt, so the recorded interactions are unchanged.
ReviewBase is Four things worth acting on, none of them correctness bugs: 1. Version-bump decision is implied but never stated, and 2. Docstrings hardcode a version this PR does not set. 3. PR description contradicts the code and 4. Duplicated comment paragraph in both live tests. Minor: I could not run |
The two are independent: the page stays the server's to choose (50 for web, capped by AI_MAX_QUERY_RESULTS) and _next_page echoes it, so a bounded read keeps paginating in those same small chunks and stops once it has enough. Sending limit=max_results conflated them — the 400 above the deployment's cap was a symptom of that, not the reason.
The two-line read-after-write note was pasted twice in a row, in both the sync and async live tests.
The Versioning table had no row for adding an optional keyword to an
existing public method, so the nearest match was `Signature change on a
public method | major` — which scores this change major even though every
existing call site keeps working untouched.
The table already carves out the additive exception cases on exactly that
reasoning ("no consumer has to change"); this applies the same rule to
keyword arguments, and narrows the signature row to the changes a caller
must actually react to.
Three accuracy fixes from an audit of the revert commits: - The `since` docstring said the parameter "said minutes before 4.4". The version is chosen at the `develop -> master` step, not here, so if that release cuts as anything else the shipped docstring is wrong and nothing would catch it. "in earlier releases" carries the same meaning with no forward reference. - `specs/05` described the CLI's `1440` default in the present tense inside a paragraph that is otherwise entirely historical. That default was retired in this same change set. - `core.py`'s module docstring lists the pure helpers; `as_result_bound` was added to `specs/01` but never to the list beside the code.
|
Reviewed against Checks that passed:
One item for the maintainer, not a defect:
|
2 participants
TL;DR
SDK support for the hunt-page ruleset tracking the internal artifact API now serves: favorites with a server-owned budget, ruleset-list filters, per-live-hunt result counts, a per-hunt feed scope, and source-rule provenance on historical hunts. Sync and asyncio clients both; all field parsing is additive (an older server leaves the new attributes
None).Requires
releasepublishes:latest), then this PR todevelop.What's new
Resources:
YaraRuleset:favorite,favorited_at,rule_count(Nonemeans the server had no answer — distinct from 0),historical_hunt_count, and the STOREDnew_results_countwith its staleness markernew_results_counted_at(the server refreshes the counter on a schedule;None= not yet refreshed / no live hunt, never 0). The per-request count surfaces from the earlier revision (LiveHuntResultCounts,live_results_count(),ruleset_list(include_counts=, since=)) are withdrawn per the query-design standard (§13) — no aggregate rides a request path.HistoricalHunt:rule_id(the source ruleset),rule_modified(freeze-time audit value),source_rule_changed— tri-state: has the source ruleset's body changed since the hunt froze it?None= unknown, not "unchanged"; the create response answers the knowableFalsedirectly.YaraRulesetFavorite(the toggle's response: star state +favorites_used/favorites_limit).RESOURCE_ID_KEYS = ['community']: the server readsid/favoritefrom the PUT body (the default['id']would moveidinto the query — a 400), whilecommunityrides the query to match the ruleset GET/list placement.Methods (sync + asyncio):
ruleset_favorite(id, favorite)— idempotent star/unstar; over-budget refusals carry a machine-readableFAVORITE_LIMITerror.ruleset_list(name=, status=, favorites_only=, has_new_results=)—has_new_resultsselects on the stored counter; there is no per-request window parameter.live_feed(livescan_id=), pluslive_feed(max_results=)— bounds how many results the generator yields and nothing else: the request is unchanged, so paging continues in the server's own chunks until the total is reached.None/0/negative all mean no bound, which is the historical behaviour, so no existing caller's results change.live_feed(since=)stays SECONDS. The docstring said minutes for years and was simply wrong; the server has always read seconds, so this is a documentation correction, not a behaviour change, and it forces no bump. Moving the wire to minutes was considered and rejected — the endpoint takes ~197k requests per 30 days carryingsincefrom clients outside our control, and re-reading those as minutes widens each 60x with no error.specs/05§Documentation corrections records the measurement.sinceabsent or0means no time filter at all — the feed pages over everything. That is the server's contract (it applies the filter on a truthiness test), now stated inspecs/03.Tests
The two rules live-tests build a uid-namespaced single-rule ruleset (unique name on the shared stack, deterministic
rule_count) and exercise the favorite round-trip, the name/favorites filters, the stored-counter contract (null until the server's refresh job runs — it doesn't on the e2e stack — and excluded fromhas_new_results), hunt provenance, the counter increment, and the changed-since-freeze flip. Read-after-write assertions whose write is line-adjacent poll (replica-lag tolerant; sleeps are free on VCR replay); the list reads that sit several calls after their create do not. Cassettes re-recorded against a live stack running the paired server branch. A new dual-transport respx suite (ruleset_favorite_respx_test.py, on theClientTestCaseharness) pins theFAVORITE_LIMITenvelope and the toggle's query/body split; pure-unit builder tests pin the request shapes. The sync client is regenerated viascripts/regenerate_sync.py.