Hunt-page ruleset tracking: favorites, rule counts, hunt provenance - #321
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
|
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:
|
The CLI expresses its need for this change set's surfaces as a version requirement — `polyswarm_api>=4.4.0` — rather than probing the installed SDK at runtime. A pin is checkable by pip at install time, before any code runs, and it cannot name a version this repo has not declared. So the bump lands here, in the feature PR, instead of at the release step. Minor, not major: every addition is additive — new methods, a new resource, new optional keywords, new parsed fields — so no existing call site changes. Bumped with bump-my-version and verified the emitted string is a clean `4.4.0`: the serialize config can produce `4.4.0.devN+sha`, and PEP 440 orders that BELOW 4.4.0, which would silently fail the CLI's floor and send its CI to PyPI for a version that does not exist yet. AGENTS.md and specs/05 record the exception and the ordering it forces: this repo must release before the CLI can, since the CLI's floor is unsatisfiable from PyPI until then. Consumer CI is unaffected — it installs this repo from git by branch name.
|
Review — clean on correctness; three process/doc items Read
1. The bump is authorized by policy this PR writes (gitflow)
Worth restating for the merge queue since it follows from the new text: 2. Head branch leaks an internal ticket ID The branch is 3.
Minor:
|
Both AGENTS.md and the specs/02 inline comment framed the attribute as "set this when the identifier isn't `id`". `YaraRulesetFavorite` is the first resource where the key is not the identifier at all: it names `community` so that `id` stays in the PUT body, because the default would move `id` to the query string and the server 400s. specs/02's prose and its per-resource entry already describe it correctly; the orientation doc and the code comment were the two places still under-describing the mechanism.
|
On the two process points from the last review, both answered rather than deferred: Maintainer ack for the in-PR bump, on record. Requested and given explicitly by the maintainer, along with the decision this change set implements: cross-repo feature dependencies are expressed as a version contract — the consumer raises its floor and Yes, the Merge ordering, corrected in the description. You were right that it omitted the release step: On
|
|
Reviewed against
Two minor items: 1. The PR description names a parameter that does not exist. The body says 2. Relatedly, the star itself is unconditional: on a live run where the team five slots are already spent (these two tests each take one, and a hard-killed run leaks one past the |
`favorites_limit == 5` mirrors a code default the deployment can override — `AI_FAVORITE_RULESETS_LIMIT` is read from the environment — which is the same mistake as the MAX_PAGE_SIZE constant this branch already removed: the client asserting a server-owned number it cannot see. Against a live stack with VCR off and a different cap configured, the exact pin fails on a correct server. Assert the relation instead; the used-vs-limit check beside it still carries the meaning.
|
Reviewed against Three things worth action: 1. Spec drift — 2. Bump policy — the PR grants itself the exception it relies on; needs maintainer sign-off. 3. Minor — the new create-response assertions widened the ruleset leak window. |
Six create-response assertions sat above the `try:` whose `finally:` deletes the ruleset, so any of them failing leaks one on the shared e2e stack. Two of them predate this branch; the four tracking-field assertions widened it. The cost is not abstract: a leaked STARRED ruleset holds one of the team's five favorite slots, and the favorite tests need a free one. `try` now opens immediately after `ruleset_create`, in both transports. specs/04 gains the `poll_equals` / `poll_equals_async` entry it was missing while documenting its sibling `run_concurrently`. It carries a non-obvious invariant worth writing down: `want` must never be None, because not-found during the lag window also reads as None and would turn a vanished resource into a passing assertion — poll a boolean instead. Also records the mirror-image limit, that a False poll cannot ride out a stale False. AGENTS.md now names where the bump exception comes from: it is this repo's instance of a workspace-level standard, not a rule the repo grants itself.
|
Reviewed against What I verified
One thing to fix
One thing to confirm, not fix
|
Two comments carried an "(F9)" review token. AGENTS.md keeps internal references out of published artefacts; the rule names commits and PR text, but a committed source comment outlives both. The sentences read the same without it.
Review — clean against the documented conventionsI read the source diff, all seven spec edits, both re-recorded cassettes, and the three test tiers against Verified
Nit (scheduling only, no correctness impact)
Maintainer decisionThe version bump is legitimate under the standing exception, but this PR adds that exception to |
The rules fragment has now been wrong three times — "rules" caught "ruleset", "test_rules" missed "test_async_rules", and "_rules" is itself a prefix of "_ruleset" so it front-loaded the instant unit tests. Every substring of those two test names is a prefix of test_ruleset_*, so no fourth fragment fixes it. Give the matcher a way to say "exactly this test" instead: a fragment starting with "::" is matched as a nodeid SUFFIX. The two long poles now name themselves exactly; every other fragment keeps substring matching, which is right for the ones that legitimately span several tests. Verified: test_rules and test_async_rules rank as long poles, the unit test backfills to the tail.
|
Reviewed against AGENTS.md and specs/01-05 + 99. Correctness and architecture look clean. Verified specifically:
1. The version bump relaxes two documented policies in the PR that needs them, so it needs explicit maintainer sign-off.
The reasoning is sound on both counts: a 2.
3. The public spec now carries production telemetry and deployment config values. specs/05 Documentation-corrections publishes "~197k requests carrying since per 30 days from 10 distinct API keys and 12 user agents", and the new test comments name 4. Spec and test prose narrates the review history. specs/05 argues with its own prior revisions ("An earlier revision of this document claimed...", "An earlier revision sent limit=max_results..."), and several new test comments do the same ("the MAX_PAGE_SIZE mistake again"). Per AGENTS.md a spec is the statement of intent, and "if a PR drifts from the spec, the spec is wrong until proven otherwise" -- superseded revisions belong in git history, not in the independently-readable document. The since-is-seconds and max_results-is-a-total notes should stay; the "an earlier revision said otherwise" framing can go. Same for 5. The branch name puts a ticket ID into public history. Title, body and all 31 commit subjects are clean of ticket IDs, which is good. The branch is Test coverage: no missing case worth naming. The two arms the e2e stack genuinely cannot produce (a populated |
…nternals `as_result_bound` was listed on the documented public surface, which put a four-line internal helper under "rename or removal of a public symbol = major". Nothing outside this package calls it — not the paired CLI — and every sibling helper in core.py is already underscore-private. Renamed to `_as_result_bound` and dropped from the specs/05 surface listing; specs/01 still records it among the helpers, where it belongs. This repo is public, so two other things should not have been in it: - Production telemetry. specs/05 published request volumes, distinct API-key and user-agent counts, and the observed value range for a parameter. The decision it supports — the wire stays seconds, because re-reading live traffic as minutes widens every window 60x silently — needs none of it. - A deployment's configured page cap. The mechanism (a per-deployment env var, set well below a large bound) is what explains the 400; the number is not. Also stops the spec arguing with its own drafts. "An earlier revision of this document claimed…" and "the MAX_PAGE_SIZE mistake again" describe review history, which git already holds; a spec should read as the current state so a reader does not have to work out which claim is live.
|
Reviewed against
Two notes, neither blocking:
I could not execute |
…om source The bullet read as if any sibling needing a new surface justifies bumping the version in a feature PR. It does not. The exception exists because a floor cannot name a version this repo has not declared — which is only a problem for a consumer whose CI installs this repo from source by branch and can therefore adopt before publication. A consumer that installs published artifacts has no such need: it adopts after the release, and this repo bumps at its own release step as usual. Stating the condition keeps the exception from being applied where it does not hold.
|
Reviewed against Actionable: the newly-documented bump procedure does not bump the version that ships
But
So This PRs own bump touched all three (line 7, Note, not a blocker: the exception is added and exercised in the same PR The base What I checked and found clean
Minor, ignore at will: |
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.polyswarm_api>=4.4.0, so its release waits on adevelop → masterPR here, not on this merge: PyPI publishes on aversionchange onmaster, sodevelopcarrying 4.4.0 uploads nothing and that floor stays unsatisfiable from PyPI until the release lands. The CLI's CI is unaffected — it installs this repo from git by branch name.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(ruleset_id, favorite=True)— 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.polyswarm_api>=4.4.0rather than probing the installed SDK at runtime, and a floor cannot name a version this repo has not declared — so the bump lands here instead of at the release step. Minor, because every addition is additive.AGENTS.mdandspecs/05record the exception, the release ordering it forces, and the PEP 440 trap (4.4.0.dev0sorts below4.4.0and would silently fail the CLI's floor).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.