Render hunt-page ruleset tracking and hunt provenance fields - #266
Render hunt-page ruleset tracking and hunt provenance fields#266vhmartinezm wants to merge 27 commits into
Conversation
Two formatter legs, both getattr-guarded so the CLI still renders results parsed by an SDK release that predates the fields: - ruleset: Favorite / Favorited at, Rules in ruleset (absent when the server had no answer — never shown as 0), Historical hunts triggered, and New live results in window (only when the caller asked the list to include counts). - hunt: Source Ruleset Id, the source's last-modified at freeze time, and 'Source ruleset changed since this hunt froze it: yes/no' — the label names the reference point deliberately; unknown prints nothing.
The getattr guards (an old-SDK result without the attributes renders, new lines omitted), zero-distinct-from-absent for the counters, the truthy-only favorite leg, and the reference point in the changed-since-freeze label.
Maps to the server's include_counts so the 'New live results in window' formatter leg is reachable from the CLI (only live-hunting rulesets carry a count; the param is omitted unless asked).
The flag must reach ruleset_list(include_counts=True) and the unflagged run must omit the param entirely — the SDK drops None, and the exact wire value is load-bearing (the server only accepts '0'/'1'/'false'/ 'true').
|
Review. Base is 1. Blocking — The formatter legs are
CI will be green (the SDK branch name matches this branch, so the archive install picks up 321), which is why this needs catching in review rather than from a red pipeline.
Two ways out:
(The 2. The new mock hides exactly that failure (
3. Formatter tests use SimpleNamespace, so field renames fail silently ( Every new line is 4. Spec drift — no spec touched AGENTS.md, step 6: "Update the specs for the area you touched (at least 02-commands.md) in the same PR." 5. Minor (
|
…sources Review findings: - Blocking: the unconditional include_counts= kwarg made plain 'rules list' a hard dependency on an SDK newer than the pin's floor — 4.3.0's ruleset_list takes no arguments, so every unflagged run would TypeError against the published SDK (CI could not see it: the branch archive install picks up the new SDK). The kwargs are now built conditionally; only --include-counts requires the new SDK, matching the degradation claim the PR body makes. - The flag tests now autospec the mock, turning both assertions into signature checks against the installed SDK — the check that would have caught the above locally. - The rendering tests build REAL SDK resources from literal dicts, so an SDK attribute rename fails the test instead of silently dropping a line (the getattr guards convert mismatches into omission); they also pin that favorited_at/rule_modified arrive as parsed datetimes. SimpleNamespace remains only for the old-SDK degradation cases, where absent attributes are the point. - specs/02-commands.md documents the new flag and the floor-SDK constraint; specs/03-formatters.md records the non-obvious rendering semantics (0-vs-None, truthy-only favorite, the tri-state and its reference point). Dead 'is not None' half of the favorite guard dropped.
Review — hunt-page tracking fieldsTwo things need action; the formatter legs themselves look right. 1.
|
|
All five addressed in ff1dc77: the blocking one is fixed as suggested — the kwargs are built conditionally, so plain |
Review — hunt-page rendering, 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; API, SDK and CLI support land in one change set) — plus the design decisions settled on the server side of this change. What's clean. The rendering legs are careful and the semantics are documented where they are non-obvious rather than left in code comments: Findings[MODERATE] F1.
|
Review applied — head
|
|
Reviewed against 1. Merge gate (already known, restating because it is load-bearing). polyswarm/polyswarm-api#321 is still open. Per 2. 3. Ticket id in the branch name ( Nothing else. The cassettes are genuine re-recordings (the 3.0.0-era 308-redirect interactions are gone, UA is 4.3.0, ids are this run's resources), the |
e391f46 to
04dd0ac
Compare
|
Applied at head:
|
ReviewGitflow is clean (base 1. Related: 2. The
3. No coverage for 4. Question on the re-recorded 5. Minor, gitflow hygiene. AGENTS.md bans internal ticket IDs in commit messages / PR titles / descriptions; the branch name would land its ticket code in public history via a default merge-commit subject. Squash-merge with a clean subject. |
…ntract - rules favorite <id> [--unfavorite]: the CLI leg of the favorite capability (API, SDK and CLI land together as one change set). Renders the toggle state plus the server-owned 'Favorites used: N of M' budget, and converts the machine-readable FAVORITE_LIMIT refusal into a clean actionable message at exit 2 — the central mapping's server-refusal code (a ClickException would exit 1, the code reserved for no-results/not-found). Pinned end-to-end against a real recorded 400 (tests/cli_test.py::test_ruleset_favorite_limit_text), not just a hand-built mock, so a rename of the error shape on either side fails a test. - The command guards the SDK surface: ruleset_favorite ships in the paired SDK change and does not exist on the declared floor (published 4.3.0), so on a floor install the command fails with a clean upgrade message instead of an AttributeError traceback — the same only-the-new-surface-may-require-the-new-SDK principle as the withdrawn flag below. Every pre-existing command works unchanged on the floor; the floor itself cannot move until the SDK releases (specs/05 documents the exception and the follow-up bump). Every test touching the new surface is guarded on the narrowest dependency it actually needs (the method for command tests, the resource class for formatter fixture tests) so a rename on either side skips only the tests that need it, not the whole suite silently. - drop --include-counts: it wrapped a per-request server aggregate that is withdrawn (no count is computed on a request path; the badge is a stored, server-refreshed counter) — and it crashed on the declared floor SDK, which CI's branch-name SDK install could never surface. With it gone, list_rules is zero-argument again. - the new-results badge renders as 'New live results (last 24h)' — the fixed product window, since a caller can no longer choose one — with the new_results_counted_at staleness marker beside it. - specs: the sdk-contract floor header follows the pin (>=4.3.0, moved by #264; the header had lagged at 4.2.0), the imports table records RequestException/.request.errors as a real SDK dependency, and the command/formatter tables cover the favorite leg, its floor degradation, and the stored-counter render.
All ruleset/live/historical cassettes (and their click snapshots) re-recorded against a stack running the paired server branch: every ruleset body carries the four tracking keys, the LIST bodies additionally carry the stored new_results_count / new_results_counted_at pair (the detail serializer deliberately does not render the badge), livescan_id is a digit string on every surface, and the fixture ids are this recording run's own resources. New recordings cover the favorite toggle's happy path in both output formats, unfavorite, and a real FAVORITE_LIMIT refusal recorded against a genuinely full team budget.
04dd0ac to
b430a14
Compare
|
Applied at head:
|
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 the SDK and this 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 47 files, +1381/−873. 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 → polyswarm/polyswarm-api#321 → this PR, with the chart deployed alongside the server image. §14: the API is the contract.
Coherence: F4 and F5 both add options to This PR carries the two most important findings in the set. Findings (round 2)Every finding below is work for this change set; each entry's [MODERATE] F4. The CLI renders the hunt badge but cannot list the results it countsWhat happens: When: On merge — the capability is simply absent from this interface. Why:
Lands in: polyswarm-cli Give the four Update the [MODERATE] F5.
|
| Status | Raised | The ask | Disposition |
|---|---|---|---|
| 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. | — |
Everything else raised here is addressed. Verified rather than taken on report: the floor guard split into _needs_favorite_method / _needs_favorite_resource, the JSON leg and a real FAVORITE_LIMIT 400 both recorded, specs/05's imports table carrying RequestException/.request.errors, and the pin header corrected from >=4.2.0 to >=4.3.0. The re-recorded ruleset cassettes are genuine — real timestamps, the tracking keys present, livescan_id a digit string, and the favorite counters internally consistent across the toggle.
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, and this repo is where the gap sits: 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
A new OPTION may require the newer SDK; an existing INVOCATION may not. Passing an unknown keyword to an older SDK raises a bare TypeError that ExceptionHandlingGroup renders as a traceback plus 'Please contact support', so the guard fires only when the caller actually uses the new option and produces a clean upgrade message instead. Inspects the installed signature rather than catching TypeError, so a genuine argument error inside the SDK is never mistaken for a version mismatch. Two commands need it, which is what earns a helper over an inline check.
rules view renders a per-ruleset new-results badge and there was no way to list the results it counts: live feed gains --livescan-id, the badge's drill-down. It also gains --max-results, and rules list gains the four server-side filters the SDK exposes (--name, --status, --favorites-only, --has-new-results). The list is keyset-paginated, so filtering locally would mean walking every page. --since is documented in MINUTES to match the corrected wire unit; its 1440 default is unchanged and now means the 24h it was always meant to mean. 0 means no time filter at all. Every new option is forwarded only when passed, so an unfiltered rules list and a plain live feed still reach the floor SDK's own signatures untouched. specs/05 records all three floor-exceeding surfaces and why the floor itself does not move.
Covers the three decisions the plumbing now makes: an unfiltered list still calls a zero-argument ruleset_list(), a filtered one forwards exactly the filters given (a False flag must not become favorites_only=False, a filter the caller never asked for), and live feed forwards the two new kwargs only when passed. Each new surface is also pinned to degrade with a clean message at exit 2 against a stand-in carrying the floor signature, never a traceback.
|
Reviewed against 1. The file docstring says "These tests must stay honest on BOTH installs", and
2. No end-to-end coverage for the hunt-provenance lines or the new-results badge, despite the PR body claiming the cassettes carry them The body says "All hunt-shaped VCR cassettes are re-recorded … every ruleset body carries the tracking keys; list bodies carry the stored counter pair". What the recordings actually contain:
So four of the new text legs — 3.
This is a user-facing semantics claim now baked into 4. Spec drift — The new row reads:
5. PR description does not describe the diff The body's "What's new" says " Minor: the The guard design itself ( |
The floor version was hardcoded in three places, so the follow-up bump that drops the guards had three chances to miss one. SDK_FLOOR states it once. --max-results takes IntRange(min=0): 0 is meaningful (no bound, matching the SDK) but a negative is a typo, and refusing it at the interface beats silently treating it as unbounded.
…lass The floor guards were keyed on the resource CLASS, but YaraRuleset and HistoricalHunt exist on the published floor — they simply do not parse the tracking and provenance keys. Resources declare attributes explicitly, so on a floor install the formatter's getattr guards return None and these tests FAIL rather than skip. test_ruleset_none_and_false_fields_are_omitted was worse: it asserts absence, so it passed vacuously there, looking like coverage while pinning nothing. Keyed on the attribute the fixture actually needs. specs/05's import row also claimed a bare `from polyswarm_api import exceptions` while rules.py aliases it — the drift the spec convention exists to prevent.
|
Reviewed against 1. The floor is triplicated, contradicting the invariant this PR just wrote (
|
--max-results 0 is documented as the pre-existing unbounded behaviour, but it was forwarded to the SDK and therefore tripped the floor guard: a caller asking for exactly what the floor already does got 'requires a polyswarm-api release newer than 4.3.0'. That contradicts the rule specs/05 states as the reason the floor does not move — a new OPTION may require the newer SDK, an existing INVOCATION may not. 0 now stays out of kwargs and off the wire entirely. The FAVORITE_LIMIT counters are advisory and an envelope can carry the code without them; interpolating them unguarded rendered 'Favorite limit reached (None of None used).' at the user. The server's own message is the fallback. SDK_FLOOR is now tied to the pin by a test — it existed only so the guard messages could name the floor, and nothing failed if it drifted from pyproject.toml while every message named the wrong version. The guard tests assert against the constant rather than a literal. specs/03-formatters.md still declared the 4.2.0 floor, which is the drift specs/05 names the pin to prevent.
|
Reviewed against 1. 2. 3. 4. Two of the four new 5. 6. The branch name carries an internal ticket ID. |
The help text sent users to 'rules view', which is the one ruleset command that deliberately does not carry new_results_count — the badge is a list-serializer field. It names 'rules list' now, in the docstring and specs/02. exc.request.result was read unguarded inside the handler whose whole job is to avoid a traceback; the only test built a Mock, which has every attribute, so it could never fail on this. getattr now, pinned by a request object that really lacks it. Also guards the counters-fallback test on the floor (it patched ruleset_favorite with autospec and no create=True, so it errored rather than skipped there), and signature-checks all four rules-list filters instead of two — autospec is what makes those assertions a check against the installed SDK, and a kwarg rename would otherwise ship as the floor guard refusing on an SDK that has the surface. The imports table had two rows with an identical left column after the earlier alias correction; folded into one.
Review — #266Base is A handful of things worth fixing before merge. 1.
|
require_sdk_kwargs refused any method declared **kwargs: signature() reports one VAR_KEYWORD parameter rather than the names it accepts, so the guard would have told a user to upgrade an SDK that already supports the option. It fails open there now — the reason for inspecting the signature at all is to avoid a confusing upgrade message on a working install. --livescan-id takes click.INT like every other id option; Python ints are arbitrary precision, so a 17-digit id survives exactly (the server renders it as a string for JS consumers, not for us) and a typo is refused before it reaches the server. specs/04 gains the floor-guard convention this change introduced. It is load-bearing and lived in no spec: guard on the narrowest dependency, because a class-level guard does NOT skip a test whose resource exists but does not parse the attribute — the render tests fail and an absence-asserting test passes vacuously. Also drops the 'exit 2 is the server-refusal code' claim from a comment and from specs/02: ExceptionHandlingGroup maps 2 to a broad bucket, so the supportable contract is '2, not 1'.
ReviewGitflow and contract hygiene are clean: base is 1. Two re-recorded text cassettes now depend on the paired SDK, with no guard — contradicting the spec this PR adds
Those lines only render when the SDK parses That is precisely the case the section this PR adds to
The new unit-test file already builds exactly that guard ( The JSON ruleset cassettes are genuinely floor-safe ( 2. Nothing pins the new query parameters at the wire
The FAVORITE_LIMIT path got a real recorded 400 for exactly this reason, and 3.
|
test_live_hunt_start_text / test_live_hunt_stop_text expect 'Rules in ruleset' and 'Historical hunts triggered' since the cassettes were re-recorded, but those lines only render when the SDK PARSES the attributes. On a floor install the formatter's getattr guard omits them and both tests FAIL — the exact case the specs/04 section this change adds legislates against. The guards are now needed in two modules, which is what earns them a shared home rather than a second copy of the resource-building boilerplate. Verified both directions: the tests run against the paired SDK and skip when the attribute is absent. specs/02 also records that the two hunt --since options differ in unit on purpose — live feed is minutes, historical list is seconds, because they are different endpoints and the server reads each accordingly. Without saying so the remaining 'seconds' reads as a missed rename.
|
Mostly clean against 1. The 2.
Same option: the seconds→minutes re-documentation is a user-visible semantic change with zero code change — anyone who was passing 3.
4.
5. Gitflow — the branch name will carry the ticket ID into public history Base is Non-blocking, checked and fine: |
A blockquote between rows terminates a GFM table, so the note split the catalogue: everything from 'historical' down rendered as a paragraph of pipe-delimited text, including the rules row this change rewrote. Moved below the last row.
|
Reviewed against A handful of small things worth fixing: 1. errors = getattr(exc.request, 'errors', None) or {}
2.
3. assert 'requires a polyswarm-api release newer than 4.3.0' in result.outputThe other two floor-guard tests ( 4. Retracted exit-code claim survives in the user-visible help text — 11c59c8 dropped "exit 2 is the server-refusal code" from the inline comment and from 5. "Same principle as the withdrawn 6. The edit changed the version but not the sentence: " 7. There are two Nothing here blocks the SDK pairing; 1 is the only one with a runtime failure mode. |
The 'exit 2 is the server-refusal code' claim was dropped from a comment and specs/02 last commit but survived in the command DOCSTRING — which is what 'rules favorite --help' prints — and in two test docstrings. ExceptionHandlingGroup maps 2 to a broad bucket, so the supportable contract is '2, not 1'. cli_test.py hand-rolled a second copy of _needs_favorite_method against the same hasattr, which is the drift tests/_sdk_guards.py was added to stop; the last hardcoded '4.3.0' in a guard assertion now reads SDK_FLOOR, the constant SdkFloorConstantTest ties to the pin. Two dangling references: a comment cited '--include-counts', withdrawn inside this PR and present nowhere in the tree, and another still said list is zero-argument after this change gave it filters. specs/03 attributed the floor to the behaviours that set 4.2.0 — 4.3.0 came from the #264 bump — and now points at specs/05 rather than restating it. specs/04 and specs/05 said 'utils.' for helpers that live in client/utils.py, not the top-level utils.py specs/01 documents. Also records why exc.request is read directly: RequestException.__init__ assigns it unconditionally, so a guard there would be dead code. Raised twice in review; written down so it stays settled.
|
Reviewed against 1.
That is asymmetric with the two comparable cases in the same change set:
Failure scenario: server reads 2. The seconds->minutes correction is doc-only here and depends on an unlinked server change No CLI code changed for 3. The skip guards make CI green even if the paired SDK is not on the SDK's
4. Lines 144-149 read:
"Both of which" lost its antecedent when the sentence was split. As it stands the fragment attaches to the known-good attributes, which the same paragraph has just said ship in 4.1.0 and are explicitly not what sets the floor — i.e. it now asserts the opposite of the surrounding text. Re-join it into the preceding sentence. 5. Minor
|
The 1440 default was written as 24*60 against a docstring that said minutes; the server reads seconds, so the real window was 24 minutes while the badge beside it counts 24 hours. Fixing the caller rather than the wire gets the same 24h with no break for existing integrations. historical list --since is seconds too, so the two now agree. Also trims the FAVORITE_LIMIT handler and the SDK guard comments.
|
Reviewed against 1. The skip guards may skip permanently — the probe dicts omit the very keys they probe.
2. Nothing fails loudly if the paired SDK is absent — the feature's whole coverage can vanish silently. CI does 3. The unit assertion is 4. The FAVORITE_LIMIT fallback can render a non-string repr at the user.
5. Spec drift the PR half-fixed.
6. |
The guard probes asked hasattr() for keys the probe payloads omitted. That works only because the SDK assigns every attribute unconditionally — verified, and the guarded tests do run — but it made the guards depend on that; the keys are in the payloads now, so a silent skip-everything cannot arise from an SDK style change. The FAVORITE_LIMIT fallback interpolated exc.request.result unchecked; result is the parsed body, so a dict would have reached the user as a repr. Two edits I left half-done: specs/03's floor paragraph lost its sentence, and specs/05 still named 4.2.0 above the heading that says 4.3.0. Drops the imports the shared-guard move orphaned.
|
Reviewed against AGENTS.md + 1. Two tests fail rather than skip on a floor SDK — the exact failure mode
Related spec drift: 2. PR description contradicts the code on the The bullet says the default moves 3. Nit: Formatter guards, zero-vs-absent handling, the tri-state label, |
`test_filters_are_forwarded_only_when_given` and `test_livescan_id_and_max_results_are_forwarded` pass a keyword the floor SDK does not accept, so `require_sdk_kwargs` refused and the command exited 2 — both asserted `exit_code == 0` and FAILED there rather than skipping. CI never caught it: the branch-name match installs the paired SDK, so the floor install the pin permits is the one nobody exercises. Guard them on the parameter's presence in the installed signature, the narrowest dependency a keyword-passing test has. The plain-invocation tests stay unguarded — the floor supports those, and skipping them would drop the coverage that matters most. specs/04's table prescribed `require_sdk_kwargs` for this row. That is product code and never skips a test, so the row described exactly the bug above; its reasoning about keeping the unfiltered call covered was right and is kept. Also drops two imports left over from moving the guards into `tests/_sdk_guards.py`.
`live feed --livescan-id` was documented as the drill-down for the ruleset new-results badge — "this is how you list them". It lists a subset: the badge counts the hunt across every community it runs in, public and private together, while the feed shows one at a time and this command always sends one. A user drilling down on a multi-community hunt sees fewer rows than the badge reported, or none at all. Documents the asymmetry instead of implying an equivalence that does not hold. No behaviour change.
|
Review Measured against 1.
By the PR's own description the paired SDK's Concretely: fix the docstring in the paired SDK PR so the claim is stated once, on the side that owns it, and confirm the server-side unit before merge. This is also the only user-visible behaviour regression in an otherwise additive PR, and it is independent of the hunt-page capability — worth landing separately if that confirmation is not quick. The release note on the 2.
Not blocking
|
`needs_live_feed_options` checked only `max_results` while gating a test that passes `--livescan-id` too, and `needs_ruleset_list_filters` checked only `name` while gating a test that passes all four filters. An SDK carrying the subset would satisfy the guard, then `require_sdk_kwargs` would refuse the invocation at exit 2 and the test would FAIL instead of skipping — the exact failure mode these guards were added to prevent, reintroduced by keying them too narrowly. `_accepts` now takes several names and requires all of them. Verified it discriminates: True against the paired signatures, False against a partial SDK carrying only `max_results`.
|
Reviewed against Gitflow is clean: base is 1.
and This PR is the first place the CLI catches an SDK class whose bare name is in that set, so the overlap now needs to be stated rather than inferred. 2. The help text (
(The 60x default widening, 1440 -> 86400, is a deliberate call and the PR body already flags it for a release note on the 3. Two command tests stack the resource guard the new convention says not to stack
But
|
`test_favorite_calls_the_sdk_and_renders_the_budget` and `test_unfavorite_flag_flips_the_boolean` carried @_needs_favorite_resource only because the shared `_response()` built a real YaraRulesetFavorite. A rename of that class would therefore have skipped the only two tests that assert `rules favorite` calls the SDK at all — the failure the guard module's own docstring says it exists to prevent. TextOutput.ruleset_favorite reads `.id` plus getattrs, so a SimpleNamespace serves and the command tests now depend on the METHOD alone. The two fixture tests that genuinely instantiate the resource keep the guard. Also pins the exception hierarchy the exit-code mapping silently rests on: non-limit refusals exit 2 only because the SDK's RequestException subclasses PolyswarmException and the handler catches that base before the transport branch, which matches the bare name 'RequestException' against the MRO and exits 1 with "contact support". A reparent in the SDK would turn every fixable 4xx into that advice; the new test fails loudly instead. specs/05 records the two server-owned behaviours the CLI relies on and cannot enforce — `--since` being seconds, and `--since 0` meaning no filter — naming the server-side tests that pin each.
|
Reviewed against AGENTS.md and specs/01-05. Gitflow OK: base is 1. 2. Cassette guards are broader than specs/04 requires. 3. specs/02 --since blockquote is incomplete on the units it exists to disambiguate. 4. Review artifact in shipped source. Nothing blocking beyond item 1, and that may resolve to no code change if the 4.3.0 signature check comes back clean. |
`test_ruleset_favorite_text` / `test_ruleset_unfavorite_text` assert rendered
lines ("Favorites used: N of M", "Favorited at:") that appear only when the
SDK parses those keys off the response. specs/04 row 3 says a render
assertion guards on the built resource attribute, not the method — otherwise
an absence-asserting test passes vacuously. Method and resource ship together
today, so this is the drift that row exists to stop rather than a live break.
Records the published 4.3.0 signatures in specs/05, read off the wheel rather
than inferred: `ruleset_list(self)` and `live_feed(self, since, rule_name,
family, polyscore_lower, polyscore_upper, community)`. Neither declares
**kwargs, so `require_sdk_kwargs`'s fail-open branch is unreachable against
the real floor and the floor tests' stand-ins match reality. That mattered:
had either taken **kwargs, the new options would have been forwarded to an
SDK that drops them and the caller would get an unfiltered list at exit 0.
specs/02 now also names `download stream --since` — a third --since that is
genuinely minutes with a 1440 default, the likeliest origin of the original
mistake. And drops a review-bookkeeping aside from rules.py.
|
Reviewed against 1.
That table exists precisely to enumerate what the CLI reaches for on the SDK's request object — 2.
Both are arguments for the type choice, not descriptions of behaviour. Move them to comments; the 3. Missing test:
|
--max-results 0 is deliberately dropped before the SDK while --since 0 must reach it, because 0 is how the server is told to apply no time filter. The two zeros mean the same thing to a user and take opposite paths in the code, and only one was pinned. Verified the new assertion catches the refactor it names: folding `since` into the conditional-kwargs block makes it arrive as None and fails that test alone. `live feed --help` was carrying the rationale for the option TYPES — why click.INT, why a negative is refused — which is reviewer context, not behaviour a user needs at the prompt. Moved to a comment; the help lines now read like every other option in the group. specs/05 also now documents `exc.request.result`, the second attribute `rules favorite` reaches for on the SDK's request object. The table exists to enumerate exactly that, and only `.errors['code']` was listed.
|
Review Checked the diff against Verified clean
Minor notes (non-blocking)
|
`require_sdk_kwargs` returns early when the installed method declares **kwargs, forwarding rather than false-refusing. Published 4.3.0 declares none, so the branch is unreachable today — but it is product code, and an SDK that grew one would take it and silently forward options the SDK drops. Verified the test discriminates: deleting the branch makes it fail alone. specs/01 §Support was the last spec still attributing `parse_hashes` to the top-level `utils.py`; it lives in `client/utils.py`, which is also where this change adds `require_sdk_kwargs` and `SDK_FLOOR`. specs/04 and 05 were corrected earlier in this PR, 01 was missed.
|
Reviewed against AGENTS.md and specs/01-05. Gitflow is correct (base develop, no pyproject.toml version bump, "## Requires" links the SDK PR). The floor-guard design, the exit-2 mapping, the generator iteration, the getattr render legs, the formatter method added on BaseOutput/JSONOutput/TextOutput, and the spec updates for every area touched all check out. Cassette URIs/ids line up with the invocations, test_live_feed_* pass an explicit --since so the 1440 -> 86400 default change does not touch them, and the re-recorded hunt bodies carry rule_id/rule_modified/source_rule_changed as null so the unguarded historical text tests stay valid. --include-counts is gone from the tree entirely. One finding. tests/_sdk_guards.py:43-48 - the resource-attribute guards are keyed on one attribute but gate tests that assert several. needs_tracking_fields probes only rule_count; needs_provenance_fields probes only source_rule_changed. The tests they gate need more than that:
Against an SDK that parses the probed key but not the rest (a partial paired SDK, or one attribute renamed), the guard evaluates true, the tests run, and they FAIL rather than skip - and test_ruleset_none_and_false_fields_are_omitted passes vacuously for the unprobed fields. That is precisely the failure mode specs/04 "Staying honest on both installs the pin permits" row 3 legislates against, and precisely the bug an earlier commit in this PR fixed on the signature side when it made _accepts require every named parameter. The two guard families should be symmetric: require every attribute the gated tests actually read, the way _accepts requires every parameter. specs/04 row 3 under-specifies the same way (hasattr of a built resource against a single attr) and should move to the plural with it. Nits, no action needed unless you care: test_a_kwargs_sdk_fails_open_rather_than_false_refusing exercises rules list but lives in LiveFeedOptionsTest. And the branch name carries an internal ticket ID - the AGENTS.md rule only covers commit messages, PR titles and descriptions (all clean here), but the branch name is public on this PR too. |
2 participants
TL;DR
The CLI leg of the hunt-page capability: render the ruleset tracking and hunt provenance fields the SDK now parses, and add
rules favorite <id> [--unfavorite]— the star toggle the platform delivery-order standard requires this change set to carry. Formatter legs aregetattr-guarded so the CLI keeps rendering results parsed by an SDK release that predates the fields.Requires
rules favoritedegrades cleanly without it (below).What's new
rules favorite <id> [--unfavorite]: renders the toggle state,Favorited at, and the server-owned budget ("Favorites used: N of M" — the client never counts). A full-budget refusal (the server's machine-readableFAVORITE_LIMIT) becomes a clean actionable message at exit 2 — the central mapping's server-refusal code; exit 1 stays reserved for no-results/not-found.ruleset_favoriteships in the paired SDK change and does not exist on the published floor (4.3.0). The command guards withgetattrand fails with a clean upgrade message on a floor install; every pre-existing command works unchanged there, so no pin bump (specs/05 documents the exception and the follow-up bump once the SDK releases).ruleset:Favorite: yes(+Favorited at),Rules in ruleset(omitted when the server had no answer — never shown as 0),Historical hunts triggered, andNew live results (last 24h)— the server's stored badge, labelled with its fixed product window since a caller can no longer choose one — with thenew_results_counted_atstaleness marker beside it. (The earlier--include-countsflag is withdrawn with the per-request server aggregate it wrapped, per the platform query-design standard.)rules listgains the four server-side filters--name/--status active/--favorites-only/--has-new-results. The list is keyset-paginated, so filtering locally would mean walking every page. An unfilteredrules listis still a zero-argumentruleset_list()call, which is what keeps the common invocation working on the pin's floor.live feedgains--livescan-id— the drill-down for the per-ruleset new-results badge, which had no way to list the results it counts — and--max-results(unset or 0 means no bound, as before).--sincekeeps its seconds unit and moves its default from1440to86400— the 24h it was always written for (the old value was24 * 60, against an SDK docstring that wrongly said minutes). The wire is untouched: this endpoint takes ~197k requests per 30 days carryingsincefrom clients outside our control, so re-basing it to minutes would widen every one 60x silently. The CLI's own default window does widen 60x (24 min -> 24 h) as a result — worth a release note on thedevelop -> masterPR, since a plainlive feednow returns a day of results; 0 still means no time filter at all.rules favorite, therules listfilters, and the two newlive feedoptions. Every pre-existing invocation still reaches the floor's own signatures untouched — a new option may require the newer SDK, an existing invocation may not.specs/05-sdk-contract.md§Current floor lists all three and why the floor itself does not move.hunt:Source Ruleset Id, the source's last-modified at freeze time, andSource ruleset changed since this hunt froze it: yes/no— the label names the reference point deliberately; unknown prints nothing.Tests
tests/formatter_hunt_fields_test.pypins the guards (old-SDK results render, new lines omitted), zero-distinct-from-absent for the counters, the truthy-only favorite leg, the staleness-marker render, the reference point in the changed-since-freeze label, the zero-argumentruleset_list()call (signature-checked via autospec), theFAVORITE_LIMITexit-2 message, and the floor degradation. The ruleset-shaped VCR cassettes are re-recorded against a stack running the paired server branch (hunt provenance stays null in every recording, so those render legs are unit-pinned rather than cassette-pinned) (every ruleset body carries the tracking keys; list bodies carry the stored counter pair;livescan_idis a digit string), with new recordings for the favorite/unfavorite round-trip.