Skip to content

Render hunt-page ruleset tracking and hunt provenance fields - #266

Open
vhmartinezm wants to merge 5 commits into
developfrom
DN-8480-hunting-schema-migration
Open

Render hunt-page ruleset tracking and hunt provenance fields#266
vhmartinezm wants to merge 5 commits into
developfrom
DN-8480-hunting-schema-migration

Conversation

@vhmartinezm

Copy link
Copy Markdown

TL;DR

Render the new hunt-page ruleset tracking and hunt provenance fields the SDK now parses. Two formatter legs, both getattr-guarded so the CLI keeps rendering results parsed by an SDK release that predates the fields.

Requires

What's new

  • ruleset: Favorite: yes (+ Favorited at), Rules in ruleset (omitted 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.

No version-pin bump: the getattr guards are exactly the degradation path for the current polyswarm_api>=4.3.0,<5.0.0 range.

Tests

tests/formatter_hunt_fields_test.py pins the guards (old-SDK results render, 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.

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').
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

Review. Base is develop OK, commit messages carry no ticket IDs OK. Four things need action, one of them blocking.

1. Blocking — --include-counts is a hard SDK dependency the pin cannot express (src/polyswarm/client/rules.py:41)

The formatter legs are getattr-guarded, but this call site is not:

for ruleset in api.ruleset_list(include_counts=include_counts or None):

ruleset_list() takes no arguments before polyswarm-api PR 321, and that PR does not bump the SDK version — the SDK pyproject still declares 4.3.0. This repo floors at polyswarm_api>=4.3.0,<5.0.0, which the published 4.3.0 satisfies while lacking the kwarg. So once this merges, pip install polyswarm-cli against SDK 4.3.0 turns a plain polyswarm rules list — no flag — into TypeError: ruleset_list() got an unexpected keyword argument, which ExceptionHandlingGroup renders as a traceback plus "Unhandled exception happened. Please contact support." and exit 2. Arguments bind at call time even for a generator function, so nothing defers it to iteration.

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.

specs/05-sdk-contract.md: "The SDK version pin in pyproject.toml is the compatibility contract. Floor it at the lowest SDK version that exposes every method/behaviour the CLI relies on" — and a floor bump has two preconditions there: the version is on PyPI, and the SDK develop declares at least that version. Neither holds today. The PR body reasoning "No version-pin bump: the getattr guards are exactly the degradation path" is true of the formatter fields and not of this line.

Two ways out:

  • have PR 321 bump the SDK to 4.4.0, release it, then floor this PR at >=4.4.0; or
  • keep the default path degradable, matching the claim the PR body already makes — build the kwargs conditionally (dict(include_counts=True) when flagged, empty otherwise) and splat them, so rules list keeps working on 4.3.0 and only the new flag needs the new SDK.

(The or None itself is fine — the existing test_ruleset_list_json cassette pins that the SDK drops None params, since the default query matcher would reject an added include_counts key.)

2. The new mock hides exactly that failure (tests/formatter_hunt_fields_test.py:96)

mock.patch(...PolyswarmAPI.ruleset_list) without autospec=True replaces the method with a signature-free MagicMock, so test_flag_sends_include_counts_true and test_no_flag_omits_the_param both pass green against an SDK whose ruleset_list() accepts no arguments. Add autospec=True and the test becomes a real signature check against the installed SDK — the one thing that would have surfaced (1) locally.

3. Formatter tests use SimpleNamespace, so field renames fail silently (tests/formatter_hunt_fields_test.py:29-40)

Every new line is getattr-guarded, which converts an attribute-name mismatch into silent omission rather than an error. Fed hand-built namespaces, these tests stay green if the SDK renames historical_hunt_count or source_rule_changed and the CLI quietly stops rendering it. specs/04-testing.md Style 3 asks for "an SDK resource built from a literal dict" — known_good_field_test.py does that with ArtifactInstance. Constructing real resources.YaraRuleset / resources.HistoricalHunt objects from literal dicts here couples the guards to the real attribute names, and additionally pins that favorited_at / rule_modified arrive as parse_isoformat datetimes rather than the raw strings the tests currently feed.

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." rules list grew a user-facing option and the rules row in specs/02-commands.md does not mention it. specs/03-formatters.md says rendering rules get documented "when they become non-obvious or contested" — the semantics this PR encodes in code comments are exactly that: rule_count=None is not 0, favorite is truthy-only (so "not favorited" and "old SDK" render identically), source_rule_changed is tri-state with None meaning unknown rather than unchanged, and the label deliberately names the freeze as its reference point.

5. Minor (src/polyswarm/formatters/text.py:301)

if getattr(result, 'favorite', None) is not None and result.favorite: — the is not None half is dead, since None is falsy. if getattr(result, 'favorite', None): says the same thing.

…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.
@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

Review — hunt-page tracking fields

Two things need action; the formatter legs themselves look right.

1. --include-counts needs a pin bump, and getattr guards don't cover it (correctness / spec drift)

specs/05-sdk-contract.md §Invariants: "The SDK version pin in pyproject.toml is the compatibility contract. Floor it at the lowest SDK version that exposes every method/behaviour the CLI relies on."

The PR body says "No version-pin bump: the getattr guards are exactly the degradation path for the current polyswarm_api>=4.3.0,<5.0.0 range." That reasoning holds for the two formatter legs — a missing attribute silently omits a line. It does not hold for src/polyswarm/client/rules.py:45. The comment right above it states the problem itself:

an installed SDK at the pin's floor (4.3.0) has a zero-argument ruleset_list

So on a conforming install (polyswarm_api==4.3.0, which the pin explicitly permits), polyswarm rules list --include-counts hits TypeError: ruleset_list() got an unexpected keyword argument 'include_counts'. ExceptionHandlingGroup has no branch for that — it falls through to the terminal except Exception (client/polyswarm.py:164) and the user gets a full logger.exception traceback plus "Unhandled exception happened. Please contact support." at exit 2. The conditional-kwarg trick protects plain rules list, not the flag the PR is adding; a flag that is guaranteed to crash on the declared floor is a flag the pin doesn't cover.

Per spec 05 the floor has two preconditions before it can move (the version is on PyPI, and the SDK's develop declares at least that version, no .devN suffix). If polyswarm-api#321's release satisfies both, bump the floor here. If it doesn't yet, the flag can't ship in this PR — either way the PR needs an explicit bump decision rather than "no bump", and the "keeps working on the pin's floor SDK" claim now in specs/02-commands.md:33 needs to say the same thing.

2. specs/05-sdk-contract.md §"Current floor" is stale (spec drift)

The section header and body still read polyswarm_api>=4.2.0, but pyproject.toml:25 has said >=4.3.0 since #264. Pre-existing, but this PR's entire no-bump argument (and the new comment in rules.py) is reasoning off "the pin's floor", and the PR already edits two specs — fix it here rather than leave the authoritative doc contradicting the file it documents. Whatever lands for #1 goes in the same section.

Minor

  • specs/04-testing.md §Style 3 asks for TextOutput(color=False) called with write=False, asserting on the returned lines — no stream. tests/formatter_hunt_fields_test.py renders through an io.StringIO instead. Equivalent in effect, but it diverges from the convention known_good_field_test.py sets; worth matching for consistency.

Clean

  • Base is develop, no CLI version bump, no ticket IDs in commit messages or PR text — gitflow and hygiene rules all satisfied.
  • JSONOutput correctly needs no change (both hunt and ruleset dump result.json).
  • Zero-vs-absent handling, the truthy-only favorite leg, and the tri-state source_rule_changed guard all match the invariants the PR adds to specs/03-formatters.md, and each is pinned by a test.
  • Building the render fixtures from real resources.YaraRuleset / resources.HistoricalHunt instances is the right call — with getattr guards, a hand-built namespace would turn an attribute-name mismatch into a silently passing test.

@vhmartinezm

Copy link
Copy Markdown
Author

All five addressed in ff1dc77: the blocking one is fixed as suggested — the kwargs are built conditionally, so plain rules list keeps working on the pin's floor SDK and only --include-counts needs the new one (specs/02 documents that constraint); the flag tests are autospec'd (the signature check that would have caught it locally); the rendering tests now build real resources.YaraRuleset/HistoricalHunt from literal dicts — SimpleNamespace remains only for the old-SDK degradation cases, where absent attributes are the point — and pin the parse_isoformat datetimes; specs/03-formatters records the 0-vs-None, truthy-only-favorite and tri-state semantics; the dead is not None half is gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant