Render hunt-page ruleset tracking and hunt provenance fields - #266
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. |
|
Reviewed against Three findings, all docs/low severity. 1. Line 12 justifies covering the same path twice with: "dropping the cassette would leave the CLI asserting a shape nothing checks." Line 16 then says: "the SDK's own respx suite pins the envelope shape." Both cannot be true. Either drop 2. The stated VCR-off failure mode for that test is wrong, which makes the
3. Untested branch: the
Also noted, no action implied: the |
…st that cannot meet it" The carve-out was wrong three ways, and reverting is cheaper than repairing it. Wrong on the count. It named ONE test that cannot pass with VCR off. Three other cassettes added here pin server-generated values a live stack will not reproduce (`Favorited at: 2026-08-26 ...`, `Favorites used: 2 of 5`), because .click snapshots are exact string equality. So is a cassette already on develop, which pins `Created at: 2022-05-26 ...`. The next contributor would have counted wrong. Wrong on the mechanism. It said a VCR-off run "gets a 200 and the assertions fail". The recorded ruleset id does not exist on a fresh stack, so such a run 404s and exits 1. Same conclusion, wrong reason — and the re-recording steps were written around that wrong reason. Wrong on the venue. The condition it described is not something this change introduces: exact-match cassettes have never been runnable against a fresh stack. Weakening a project-wide invariant to accommodate a pre-existing condition is a maintainer decision, not a line item in a feature PR. AGENTS.md goes back to develop's wording verbatim. The tension between the invariant and snapshot-style cassettes is real and stays on the record as-is, for a decision made on its own terms. Also drops the claim that dropping the cassette "would leave the CLI asserting a shape nothing checks" — the paired SDK's respx suite does check it. What the cassette actually pins is the seam between the two, which is what it now says.
…igit count The envelope-spelling test assigned request.json itself, so it could not detect the rename it was written to detect — its own comment explains that a real request was chosen over a Mock because "a Mock fabricates whatever attribute it is asked for", and assigning the attribute does the same thing. It now pins the spelling on the untouched request first, before assigning: .json present, the mistaken .result spelling absent. Verified by renaming the attribute across the SDK (the class annotation as well as the assignments) and watching it fail. The empty-remedy arm of the FAVORITE_LIMIT handler had never executed: every test reaching that branch invoked without --unfavorite, so inverting the conditional would not have failed one. Verified by inverting it. Now covered. The known-good getattr guard carried the rationale this branch spent its commits removing — "a CLI running against an older installed SDK won't have them". Those attributes ship three releases below the floor, so the pin already guarantees them; the guard survives as belt-and-braces for an unsupported configuration, which is what the comment now says. Left as-is it invites both mistakes: deleting the guard, or adding siblings for versions the floor covers. --livescan-id's help promised "a 17-digit number"; this repo's own cassettes carry 16-digit ids. specs/02 records that a negative --since is now refused at parse time rather than forwarded, and that exit 2 does not identify a server refusal — click uses it for usage errors too. specs/05's cutoff gains the release-note obligation for behaviour changes to existing invocations, since there is no CHANGELOG to carry them.
|
Three rounds landed together and converged on the VCR carve-out from three different angles. They were Checking the count claim settled it. The carve-out named one test that cannot pass with VCR off; three That makes the condition pre-existing, which settles the venue question: weakening a project-wide Fixed:
Two need a person, not a commit:
One correction to round 3: 148 tests pass. |
…nd the wrong failure The section pointed at 'the VCR invariant above' for an exception that no longer exists there, and repeated the mechanism the revert corrected: a fresh-stack recording does not yield a 200. The recorded ruleset id is not on a fresh stack at all, so that run 404s; the 200 is what a stack WITH the ruleset and an unsaturated budget returns. Both states have to be set up, and saturating the budget takes slots the sibling favorite tests need.
Review — against
|
|
Reviewed against 1. The floor precondition is not evidenced by the recordings ( 2. 3. Not a defect, just the reminder the spec asks for: the |
…-since 0 rests on Both nested guards in the ruleset leg had only their True arm exercised. The tracking-fields test builds a count with no refresh marker — the inner guard's False arm — but never asserted the marker was absent, and no test rendered a favorite without a timestamp at all. Inverting either guard failed nothing. Verified by inverting both; both new assertions fail, and neither did before. --since 0's help text promises "no time filter at all", which is a claim about the server, not the CLI. Confirmed against the endpoint: the window is applied only when since is truthy, so 0 and an absent parameter are the same request. specs/02 now says so, and says what the CLI test can actually pin — that 0 is forwarded rather than dropped, which is the half that can regress in this repo. The re-record steps named one state-dependent cassette. All three ruleset-favorite cassettes are, because the rendered block is compared verbatim and the budget counter is inside it: one pins "Favorites used: 2 of 5", another "1 of 5". They have to be recorded together from a known starting state or the counters contradict each other.
|
Items 1, 3 and 4 fixed in 4 — the two omission arms. Correct, and worse than stated: neither nested guard in the 3 — 1 — the re-record guidance. Extended to all three ruleset-favorite cassettes. The counter is inside Note this is guidance on recording, not an exemption. The invariant bullet stays unamended — an earlier 2 — the floor. Confirmed and demonstrated: 5 — the branch name. Acknowledged and outstanding by decision, not oversight: the merge subject is 149 tests pass. |
ReviewCode, formatters and spec updates line up well against 1.
|
| order | test | recorded expectation |
|---|---|---|
| 1 | test_ruleset_favorite_json |
favorites_used: 1 |
| 2 | test_ruleset_favorite_limit_text |
server refuses, 5 of 5 used |
| 3 | test_ruleset_favorite_text |
Favorites used: 2 of 5 |
| 4 | test_ruleset_unfavorite_text |
Favorites used: 1 of 5 |
1 → 3 → 4 are internally coherent (+1, then −1 on the same ruleset). #2 is not satisfiable in the same run: it needs the team budget saturated at run time, and _assert_text_result compares the whole rendered block verbatim, so
- budget full → update readme #3 gets a 400 and exits 2, failing its
expected_return_code=0; - budget not full → Feature/since flag #2 gets a 200 and its
assert 'Favorite limit reached (5 of 5 used)' in expectedfails.
The new specs/04 §Re-recording paragraph names this tension ("saturating the budget consumes slots the other favorite tests use") but doesn't resolve it — it documents a cassette-only test rather than fixing one. The FAVORITE_LIMIT path is already covered at the CLI boundary by RulesFavoriteCommandTest.test_favorite_limit_refusal_is_a_clean_message_at_exit_2 plus the no-counters / bare-request / list-shaped variants; the only thing the cassette adds is where in the envelope the code lives, which specs/05 correctly identifies. Options that keep that value without breaking the invariant: pin the envelope shape in a test that doesn't also depend on the team's live budget, or drop the exact-counter assertions from #3/#4 so a live run can land anywhere. As written, the suite is no longer VCR-off-clean.
2. Confirm the paired SDK branch actually declares 4.4.0 (and without a dev suffix)
Every re-recorded cassette carries user-agent: polyswarm_api/4.3.0 — including the three new favorite ones, which exercise a surface 4.3.0 doesn't publish. So the recording environment had the paired branch's code but its declared version was still 4.3.0. specs/05 §Current floor is explicit about the check this PR now rests on: "Read the declared version off the archive's own tree, and mind pre-release suffixes … Check the version string in the SDK branch's pyproject.toml / __init__.py, not the last release tag." With >=4.4.0 here, if the SDK branch still declares 4.3.0 or 4.4.0.dev0, pip install .[tests] goes to PyPI for a version that doesn't exist and CI fails. The PR body asserts the SDK PR declares 4.4.0 — worth pasting the two version strings read off polyswarm-api's branch, since the cassettes are the only in-repo evidence and they say 4.3.0.
3. Branch name publishes an internal ticket ID
AGENTS.md: "Don't reference ticket IDs or internal project codes in commit messages, PR titles, or PR descriptions. This repo is public." The title and body are clean, but DN-8480-hunting-schema-migration is published on a public repo and lands in the default merge-commit subject. Merge with a message that drops it, or rename the branch.
Checked and fine
- Exit-code routing:
exceptions.PolyswarmException→Exit(2), matched before the ancestry-name transport branch that would otherwise takeapi_exceptions.RequestExceptionto exit 1.ExitCodeHierarchyTestpins the subclass relation the ordering rests on, andspecs/01now documents that the clause order is load-bearing — good thing to have written down. - The
--since 0forwarded /--max-results 0dropped asymmetry is deliberate, tested in both directions, andIntRange(min=0)refuses negatives at parse time with an exit-2 / "Invalid value" assertion rather than a bare!= 0. --since1440 → 86400 is a real behaviour change to an unchanged command line; correctly kept out ofpyproject.toml'sversion(that's the release step) and flagged for thedevelop → masterrelease note in bothspecs/02andspecs/05.rules listsends no kwargs when unfiltered (favorites_only or None), signature-checked viaautospec.isinstance(errors, dict), thegetattron.request.json, and the._result-vs-.jsonspelling are all pinned against realPolyswarmRequestobjects rather than Mocks — the reasoning intest_favorite_limit_without_counters_uses_the_server_messageabout why a Mock couldn't catch it is exactly right.output.huntis only reached fromhistorical.py, so the new provenance fields can't hit a resource class that lacks them; provenance is null in every recording and unit-pinned instead, asspecs/03states.specs/02's claims thatdownload stream --sinceis genuinely minutes (IntRange(1, 2880), default 1440) andhistorical list --sinceis seconds both check out against the code.
…atisfiable The three ruleset-favorite cassettes describe one coherent sequence: star a ruleset (1 of 5), star another (2 of 5), unstar it (1 of 5). The cap refusal needs 5 of 5. With VCR off, saturating the budget to record or reproduce the refusal turns the other two into 400s, failing their expected exit 0; leaving it unsaturated turns the refusal into a 200. There is no stack state where all four pass, so the suite could not be run against a live stack at all. The previous commits tried to write that down instead of resolving it -- first as an exception to the VCR invariant (reverted), then as re-recording guidance. Documenting a test that cannot run is not the same as having one. Removing it costs little now, and less than it would have earlier in this branch. The message and exit code are pinned at the SDK boundary; the envelope spelling is pinned against a real PolyswarmRequest, which is the part a cassette was carrying until this branch added that pin; and the wire shape is pinned by the SDK's stubbed-transport suite -- which declined to record this same refusal, for this same reason, in the paired PR. Recording it on one side while stubbing it on the other was the inconsistency.
|
Item 1 is right, and the mutual-unsatisfiability framing is what settled it. The cassette is removed Verified before acting: the three surviving favorite cassettes describe one coherent sequence — star Two earlier attempts on this PR tried to write that down — first as an exception to the VCR invariant What tipped the cost/benefit is a change made earlier in this same branch: The invariant bullet stays exactly as 2 — the floor. 3 — the branch name. Outstanding by decision, not oversight; handled in the merge subject. 148 tests pass. |
|
Review — against Code side is clean: SDK surfaces used match PR #321 ( Four things to action. 1. The new invariant at line 12 asserts a recording that does not exist:
Line 52 of the same file says the opposite, and is the true statement:
2. The The 1440 -> 86400 correction is right on the merits (the wire has always been seconds;
3. Branch name leaks a ticket ID into public history. AGENTS.md: "Don't reference ticket IDs or internal project codes in commit messages, PR titles, or PR descriptions. This repo is public." Title and body are clean, but the branch is 4.
Nothing blocking on correctness. (1) is the one I would want fixed before merge, since it is a documented invariant that is now false. |
…exists The previous commit removed the favorite-cap cassette but left specs/04's "one sanctioned exception" paragraph asserting it — "the recorded 400 pins where in the envelope the machine-readable code actually lives", "the cassette is what pins the seam between them". Both describe a file this branch deletes, and the same paragraph licensed mock-plus-VCR coverage of one path, which is the rule it was nested under as an exception. Nothing needs the exception now: the refusal is covered by unit tests on this side and a stubbed transport on the SDK's. Also moves development history out of user-facing help and out of comments that had grown into review transcripts. `live feed --help` no longer explains that an old default was written as 24*60 in the belief that the unit was minutes — it says what the default is and how to get the old window back. "The badge" had no referent for someone reading --help and is now named for what it is. The kwargs and FAVORITE_LIMIT comments state the fact at hand and point at the spec for the reasoning, instead of naming test functions that will drift.
|
1 — correct, and my own miss. Deleting the cassette last commit left 4 — taken, and it matches a standing convention here (facts inline, rationale in the spec). 2 and 3 — outstanding by decision, and both belong to the merge, not to a commit here.
148 tests pass. |
|
Reviewed against
Three minor items: 1. The
2. The new cassette pair encodes an implicit ordering dependency
Related: 3. Branch name carries a ticket ID
No action needed on the |
…y is
`errors` is isinstance-guarded so a non-mapping envelope falls through to the
generic path instead of raising on `.get`. The `.json` read two lines down had
no such guard: `(getattr(...) or {}).get('result')` only substitutes `{}` for a
falsy value, so a list envelope reaches `.get` and raises AttributeError —
inside the handler whose entire purpose is turning this refusal into a clean
message. Reproduced: full traceback plus "Unhandled exception happened. Please
contact support."
Needs three things at once (a dict `errors` carrying the code, no counters, and
a non-dict `.json`), which is why no existing test caught it. One does now;
verified by reverting the guard.
specs/04 also names the mechanism the favorite cassettes' sequence rests on:
unittest orders methods by sorted name, so renaming one breaks the recorded
star/star/unstar sequence silently — replay keeps passing and only the next
re-record surfaces it. And the VCR bullet now points at the re-recording section
rather than appearing to contradict it: the invariant is about a test's logic,
while a .click snapshot pinning server-generated values is a fixture concern.
|
1 — real, and reproduced before fixing. 2 — taken, both halves. The sequence rests on On the cross-link: added, phrased so it points rather than weakens. The distinction that makes them 3 — outstanding by decision. The branch name reaches public history only through the default merge 149 tests pass. |
Review — against
|
The hygiene rule names commit messages, PR titles and PR descriptions. Branch names sit outside it, and reviewers have repeatedly and reasonably read that as an oversight — a branch ref is just as public and just as permanent. It isn't an oversight, but the reasoning lived nowhere in this repo. A change spanning this repo and the SDK has to use the identical branch name in both, because CI resolves the companion SDK by $CI_COMMIT_BRANCH; for a ticket-tracked change that name is usually the ticket. So the prefix is deliberate. What actually leaks is the default merge subject, which interpolates the head ref. Squash-merging with an explicit subject contains it, and that is now written down next to the rule it looks like an exception to, along with the preference for a descriptive shared name where one reads just as well.
|
No new code findings this round — all four items are the standing ones, and three of them are decisions 3 — the branch name. This has come up in every round, so I've written the policy down instead of 4 — squash on merge. Agreed, and it is the same action as item 3: one squash with an explicit clean 1 — the floor. Confirmed again: 2 — the release note. Cannot be satisfied in this PR by construction, which is why the obligation was On the two no-action notes: the order-dependence of the favorite cassettes is a knowing trade-off and is 149 tests pass. |
|
Reviewed against 1.
Concretely: 2. Merge-time: this PR must be squash-merged with an explicit subject. The new Nothing else: the |
… happened The recipe said to record the three favorite cassettes together, in sorted-name order, from a known starting state. Two things are wrong with it. It omits test_ruleset_list_json, which sorts between favorite_text and unfavorite_text and pins "favorite": false on every ruleset in its inventory. Anyone following the recipe records it while a ruleset is starred, so that snapshot changes as well. And the shipped cassettes were not made that way: favorite_text acts on 96652060989160147, which does not appear anywhere in list_json's inventory (77454540525125655, 44051669277897879, 78562964231669682). They were recorded against different stack states, so following the recipe would surface the contradiction rather than reproduce the fixtures. Now states the couplings instead of a sequence, keeps the silent-failure warning about renaming a method, and points at the escape hatch that exists already: _assert_text_result takes a replace= hook, so normalising the budget counter makes each cassette independent of what ran before it. Better to break the coupling than to document a wider one.
|
Both halves check out, and the recipe is corrected in The interleaved test. Confirmed: sorted-name order is The provenance. Also confirmed, and more bluntly than stated: The section now states the two couplings rather than a sequence, keeps the warning that renaming a method Not taking the normalisation itself in this PR — it changes what three shipped cassettes assert, and the 149 tests pass. |
|
Review Checked the CLI diff against Three things that need action. 1.
2. The new cassette set ships a documented inconsistency instead of the fix the spec itself names
Please apply that rather than document the landmine. 3. Merge mechanics
Non-blocking
|
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 read the fields directly; the pin guarantees the SDK parses them, soNonemeans the server had no answer.Requires
developfirst, and release it first. This PR pinspolyswarm_api>=4.4.0, the version that PR declares. Two distinct orderings follow:developbranches are tested against each other by design, so a change spanning the pair lands on both together. Merging this side alone leaves itsdevelopasking for an SDKdevelop.zipthat does not yet declare the floor, and the install fails — the pairing being broken, not a trap to design around.developpublishes nothing; PyPI only sees a version atdevelop → master. At that point the SDK version is confirmed (bumped if the repo files do not already carry it) and this repo's dependency is set to the version actually released. The>=4.4.0here is the working valuedevelopintegration validates.This branch's own CI is unaffected either way — it resolves the paired SDK branch by name.
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_favorite. Nogetattr, no upgrade message: the pin makes an SDK without it uninstallable, so the command calls it directly.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— the server's stored badge, deliberately unlabelled with a window since the response carries none and a caller cannot choose one — with thenew_results_counted_atstaleness marker beside it, which is what tells a reader how current the number is. (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 — a False flag is not a filter.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.getattrfallbacks, no per-test skip guards. A probe would restate what the pin already says with nothing keeping the two in sync, and it fails in both directions — check less than the call site uses and execution reaches the missing surface; check more and the fallback fires while the surface is present, which in a test is a silent skip that drops coverage.specs/04andspecs/05carry the rule; the workspace repo carries it as an org-wide standard.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 render legs against real SDK resources, 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 exception hierarchy the exit-code mapping depends on. 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_idrenders as a string when present), with new recordings for the favorite/unfavorite round-trip.