Consolidate sync + async clients via unasync codegen (httpx-only)#298
Consolidate sync + async clients via unasync codegen (httpx-only)#298sbneto wants to merge 49 commits into
Conversation
…lt via inheritance Phase 1 of the sync+async consolidation tracked by DN-8225. What changes: * New ``polyswarm_api._base.PolyswarmAPIBase`` holds the shared constructor, instance state, and ``__repr__``. Both ``polyswarm_api.api.PolyswarmAPI`` (sync) and ``polyswarm_api.aio.PolySwarmAsyncAPI`` (async) now subclass it. * ``aio.core.AsyncPolyswarmRequest`` inherits from sync ``core.PolyswarmRequest``. ``parse_result``, ``_extract_json_body``, and ``_bad_status_message`` are no longer duplicated — they live on the sync request and the async subclass inherits them. Only ``execute``, ``consume_results``, and ``next_page`` are overridden with their async-specific bodies. * Both API clients expose new ``_single`` / ``_paginate`` / ``_sleep`` hooks declared on the base. Async ``_single`` / ``_paginate`` are polymorphic: they accept either a pre-built ``AsyncPolyswarmRequest`` / ``PolyswarmRequest`` (the Phase 2 style) or a legacy ``(request_dict, result_parser=)`` pair (still used by every endpoint in this file). The legacy ``_exec`` / ``_generate`` helpers stay as thin wrappers around ``_paginate``. What doesn't change yet (Phase 2 follow-ups, same DN-8225): * Sync transport is still ``requests`` via ``PolyswarmSession``. The migration to ``httpx.Client`` lands with the bulk endpoint consolidation. * Endpoint methods (~110 per class) are still defined twice. Each one becomes a single ``return self._single(resources.X.method(...))`` on ``PolyswarmAPIBase`` once the resource classmethods are switched from auto-execute to returning unexecuted requests. * ``requests`` is still a runtime dep (and is still used directly by ``resources.py`` for S3 uploads). * No version bump per the repo's gitflow rule (``AGENTS.md``). All 68 existing tests pass.
…g onto base
Migrate the simplest endpoint family — ``metadata_mapping`` and the
four ``metadata_field_properties_*`` CRUD methods — onto
``PolyswarmAPIBase``. The duplicate bodies in ``api.py`` (5 sync
methods, ~50 LoC) and ``aio/__init__.py`` (5 async methods, ~60 LoC)
are deleted; the base method is what both clients now expose.
Demonstrates the trick:
class PolyswarmAPIBase:
def metadata_field_properties_list(self):
return self._paginate({...}, result_parser=...)
# sync subclass: ``_paginate`` is a generator function.
# for x in api.metadata_field_properties_list(): ...
# async subclass: ``_paginate`` is an async generator function.
# async for x in api.metadata_field_properties_list(): ...
The base method body is sync-shaped — no ``async def`` — and returns
whatever ``self._paginate(...)`` returned. Each subclass's
``_paginate`` produces the right iterator flavour; the caller picks
the matching iteration syntax.
This is the pattern Phase 2 (DN-8225 follow-up) will apply to the
remaining ~110 endpoint methods.
All 68 tests still pass.
Introduce a ``ClientTestCase`` base class with an ``__init_subclass__`` hook that auto-emits ``<Name>Sync`` and ``<Name>Async`` sibling classes for every test class. The original is hidden from pytest via ``__test__ = False`` — only the parametrised siblings run. The same test methods now exercise both clients: * Sync sibling: instantiates ``PolyswarmAPI``, mocks via ``responses``. * Async sibling: wraps ``PolySwarmAsyncAPI`` in an ``_AsyncToSync`` facade so unittest-style test bodies (``result = self.api.foo(...)``) stay unchanged; mocks via ``respx``. A small ``_MockBoundary`` helper unifies the two mock libraries behind ``mock.add(method, url, json=...)`` — tests don't care which transport they're hitting. Test count goes from 4 to 8 for ``metadata_field_properties_test.py`` (4 endpoints x sync + async). Full suite: 68 -> 72. Same pattern lands on the rest of the test files as Phase 2 endpoints migrate to the base.
Add three new sections to AGENTS.md (which CLAUDE.md symlinks to): * **Layout** — describe the new ``_base.py`` ``PolyswarmAPIBase`` plus how sync / async clients subclass it and where the request/response machinery lives now (``AsyncPolyswarmRequest`` inherits from ``PolyswarmRequest``). * **Sync + async via shared base** — explain the single-method-body trick (``def method(self, ...): return self._single(...)`` works for both clients because async ``_single`` returns a coroutine the base method passes through). Note the migration status (DN-8225 Phase 2 remaining for ~110 endpoints). Cross-reference the worked akm example in ``/home/sam/repos/api-key-management``. * **VCR cassette workflow** — capture the delete-to-rerecord pattern, the ``match_on=['method', 'uri']`` rule that lets the same cassette serve both ``requests`` and ``httpx``, and the workspace invariant that tests must pass with VCR off. Also update **When adding a new resource** to point at the new base class as the home for endpoint methods — no more dual-defining in ``api.py`` and ``aio/__init__.py``.
Both clients now use httpx end-to-end — there is no longer a sync/async split at the HTTP library layer. What changes: * ``polyswarm_api.core.PolyswarmSession`` becomes a thin wrapper over ``httpx.Client`` instead of subclassing ``requests.Session``. Same ``request()`` / ``verify`` / ``headers`` / ``close()`` surface that ``PolyswarmRequest`` already uses. Retries, auth header, user agent preserved. * ``polyswarm_api.core.HttpxResponseAdapter`` (formerly ``aio.core._HttpxResponseAdapter``) moves to the shared module. Both sync ``PolyswarmRequest.execute`` and async ``AsyncPolyswarmRequest.execute`` now wrap non-JSON responses through it so file-download parsers (``LocalArtifact``, etc.) still see the ``iter_content`` surface they expect. * ``polyswarm_api.resources.LocalArtifact.upload_file`` (and the duplicate in ``SampleBundle.upload_file``) call ``httpx.put`` instead of ``requests.put``. The empty-file workaround comment and dead-code branch are dropped — we send ``content=b''`` explicitly. * ``_normalise_bool_params`` lives in ``core.py`` and is invoked by both sessions. ``requests`` serialised Python booleans as ``str(bool)`` (``'True'`` / ``'False'``); httpx writes them lowercase (``'true'`` / ``'false'``). Existing VCR cassettes were recorded against the requests format, so we normalise on the way out. Tests: * ``test/client_scan_test.py::test_resolve_engine_name`` was mocked via ``responses`` (which only intercepts ``requests``). Switched to ``respx`` since both clients are now httpx-backed. * ``test/metadata_field_properties_test.py`` similarly drops the ``responses`` arm of its ``_MockBoundary`` — ``respx`` handles both client kinds. All 72 tests still pass against the live e2e stack (cassettes replay without re-recording).
No code in ``polyswarm_api`` imports ``requests`` anymore (the sync transport migrated to httpx and the S3 upload helpers in ``resources.py`` now use ``httpx.put``). Promote ``httpx`` from the ``[async]`` extra to the core ``dependencies``. The ``[async]`` extra is kept as an empty list so downstream consumers that pin ``polyswarm-api[async]`` (polykg, neonscan, polyswarm-cli) keep working unchanged.
…ync API routes through _single/_paginate Step 2 of the bulk endpoint migration tracked by DN-8225. * ``BaseJsonResource.create`` / ``get`` / ``head`` / ``update`` / ``delete`` / ``list`` and every per-resource classmethod in ``resources.py`` now return an UNEXECUTED ``PolyswarmRequest``. The ``.execute()`` chain moves up to the API client, so a single resource builder works for both sync and async transports. * ``PolyswarmAPIBase._coerce_request`` rebuilds a ``PolyswarmRequest`` as the subclass's ``_request_cls`` (sync ``PolyswarmRequest`` or async ``AsyncPolyswarmRequest``). Sync and async ``_single`` / ``_paginate`` now accept either an unexecuted request or a request-parameters dict. * All ~100 ``resources.X.method(self, …).result()`` chains in ``api.py`` rewritten to ``self._single(resources.X.method(self, …))`` via a small Python script. Six resource-instance chains (``task.download_zip(…).result()`` etc.) updated by hand. * ``sandbox_providers`` keeps its previous quirk of returning the executed ``PolyswarmRequest`` itself so callers can read ``.json``. This is purely a sync-side wiring change. The async client still has its own duplicate endpoint methods; the next commit hoists everything onto ``PolyswarmAPIBase`` and deletes the async duplicates. 72 / 72 tests pass.
This is the bulk consolidation step. The shared base class now holds
every endpoint method; both ``PolyswarmAPI`` (sync) and
``PolySwarmAsyncAPI`` (async) inherit them.
What moved to ``polyswarm_api._base.PolyswarmAPIBase``:
* All ~100 endpoint methods from ``polyswarm_api.api.PolyswarmAPI``
(search, lookup, rescan, hunts, rulesets, tags, families, IOCs,
downloads, sandbox tasks, reports, templates, prompt configs,
webhooks, …). Each body is the same one- or two-liner —
``return self._single(resources.X.method(self, …))`` for single
responses, ``return self._paginate(…)`` for paginated ones.
What stays sync/async-specific (kept on the subclasses):
* ``__init__`` (each constructs its own HTTP client)
* Context-manager methods (``__enter__``/``__exit__`` vs
``__aenter__``/``__aexit__`` vs ``close``/``aclose``)
* ``_single`` / ``_paginate`` / ``_sleep`` (the three hooks the
shared methods delegate to)
* Polling helpers: ``wait_for``, ``report_wait_for`` (use sync
``time.sleep`` vs async ``asyncio.sleep`` inside loops)
* File-upload paths: ``submit``, ``sandbox_file``, ``sandbox_url``
(sync uses ``LocalArtifact.upload_file`` via ``httpx.put``; async
uses ``async_upload_file`` from ``polyswarm_api.aio.upload`` —
neonscan's monkey-patch site)
* ``refresh_engine_cache`` / ``engines`` property (mutates
``self._engines``; sync property can't call async refresh)
* ``sandbox_providers`` (returns the executed ``PolyswarmRequest`` so
callers can read ``.json`` — a quirk of the existing surface)
Resource classmethods (``BaseJsonResource.create`` / ``get`` / etc.,
plus per-resource builders like ``ArtifactInstance.search_hash``) were
already changed to return UNEXECUTED ``PolyswarmRequest`` instances in
the previous commit. The base methods call them and pass the result to
``_single`` / ``_paginate``, which rebuild as the subclass's
``_request_cls`` (sync ``PolyswarmRequest`` vs async
``AsyncPolyswarmRequest``) before executing.
The trick that makes a single sync-shaped method body work for both:
class PolyswarmAPIBase:
def search(self, hash_, hash_type=None):
hash_ = resources.Hash.from_hashable(hash_, hash_type=hash_type)
return self._paginate(resources.ArtifactInstance.search_hash(
self, hash_.hash, hash_.hash_type,
))
* Sync ``_paginate`` is a generator function — ``for x in api.search(…)``.
* Async ``_paginate`` is an async generator function — ``async for x in
api.search(…)``.
* No ``async def`` on the inherited public method; the base method just
returns whatever ``_paginate`` returned.
The same applies to ``_single`` for non-paginated endpoints.
Test changes:
* ``test/client_scan_test.py::test_check_known_host`` materialises the
paginated result with ``list(...)`` before indexing — the inherited
method is now ``_paginate``-shaped.
* ``test/async_client_test.py`` VCR matcher switched from strict
``uri`` to ``[method, scheme, host, port, path, query]`` so cassettes
recorded against requests-style param ordering replay against httpx,
which serialises params in a different (but equivalent) order.
* Five async cassettes copied from their sync counterparts where the
test scenarios are identical (``test_async_iocs_by_hash``,
``test_async_search_by_ioc``, ``test_async_sandboxtask_latest``,
``test_async_historical_results``, ``test_async_live``).
Net diff: -1,514 lines across the client modules. Both client files
collapsed to ~330 lines each (was ~1,300 sync + ~1,900 async).
All 72 tests pass against the live e2e stack.
…st work The migration-status section now describes the completed state: every endpoint on `PolyswarmAPIBase`, sync on httpx, requests dropped from runtime deps, both clients reduced to ~330 lines each, ~1,500 net lines removed. Calls out the remaining sub-task on DN-8225: rolling out the parametrised `ClientTestCase` harness to `client_scan_test.py` and `async_client_test.py`. The pattern is established in `metadata_field_properties_test.py`.
sbneto
changed the title
Introduce a `specs/` directory as the authoritative home for the SDK's contracts, invariants, and per-area design. AGENTS.md now points at the specs for detail; it itself stays orientation-shaped (gitflow, conventions, architectural snapshot, links to specs). Specs landed (each opens with Scope + Invariants, then file references, then content): * `00-overview.md` — what the SDK ships, platform context, repo layout * `01-architecture.md` — PolyswarmAPIBase, the sync/async pattern, the request and response pipelines, the sync/async carve-outs * `02-resources.md` — BaseJsonResource + per-domain resource classes, classmethod builder convention (return unexecuted PolyswarmRequest) * `03-endpoints.md` — full method catalogue + classification grid (single vs paginate, base vs carve-out, with rationale per method) * `04-testing.md` — three mocking layers (respx, vcrpy, pure), ClientTestCase parametrisation harness, VCR record-on-delete workflow, matcher conventions, outstanding test rollout * `05-downstream-contract.md` — public surface, monkey-patch sites, backward-compat invariants, versioning rules * `99-open-questions.md` — known follow-ups (test rollout, upload divergence, sandbox_providers quirk, VCR-off CI, …) Also: `.github/workflows/claude-code-review.yml` adds an automated review pass on every PR. Reads CLAUDE.md / AGENTS.md and `specs/` for context. Posts review via `gh pr comment`. Requires repo / org secret `ANTHROPIC_API_KEY`. Disable by setting `if: false` on the job. AGENTS.md slimmed down — the long migration-status section moves into the specs (status is captured in the spec content itself rather than as a separate progress doc). Reading order is now: this file (gitflow + shape), then specs/00-overview.md, then the spec for whichever area the change touches.
Pinning to a SHA (rather than the mutable v1 tag) closes a supply-chain leak vector: a compromised v1.x release would otherwise inherit access to ANTHROPIC_API_KEY on its next run. The trailing '# v1' comment keeps the corresponding tag readable for humans and Dependabot.
…ed-base # Conflicts: # .github/workflows/claude-code-review.yml
The action's workflow-validation check requires the file on a PR branch to match the default branch byte-for-byte. develop currently references @v1; the SHA pin in the previous commit on this branch broke that match and blocked the auto-review. Revert the pin here so the review runs on this PR. The supply-chain hardening (SHA pin) can land in a follow-up that updates develop and this branch together.
ReviewThree real issues, all in the hoisted base class / async carve-outs. The bulk consolidation pattern is sound; these are leftover edges where the new "resource classmethods return unexecuted 1.
|
…c engines property notification_webhook_test was building a PolyswarmRequest and never executing it, so non-2xx responses were silently swallowed and async callers got back an unexecuted request object instead of awaiting a coroutine. Route it through self._single() like every other endpoint method. As a side effect of that fix: parse_result previously gated non-2xx error mapping on result_parser being set, which meant fire-and-forget endpoints (no parser) would not raise on server errors. Restructure parse_result so non-2xx mapping runs regardless of result_parser. HEAD method short-circuits first to preserve exists_hash's "404 means absent" behaviour. Webhook.test keeps no result_parser since the test endpoint returns a generic OK envelope, not a Webhook-shaped payload. The async engines property had a stacked @Property decorator above the AttributeError raiser, leaving a no-op decorator dangling. Remove it.
Review — PR #2981. Correctness —
|
The aio package should mirror the root layout: classes live in named modules, not in __init__.py. aio/api.py now holds the async client class (mirrors src/polyswarm_api/api.py); aio/__init__.py is a thin re-export so external `from polyswarm_api.aio import PolySwarmAsyncAPI` keeps working unchanged. No functional change.
Sync sandbox_providers returns an executed PolyswarmRequest so callers read .json['result'][slug][...]; the async client was diverging by exposing an async generator of parsed SandboxProvider items, which is a different downstream contract for the same method name. Sync is the source of truth: async now mirrors it, returning an executed AsyncPolyswarmRequest the caller awaits then reads .json on. Test updated to match the sync test's access pattern.
exists() reads the _single result (str(result) == '200') so it cannot share a body with both transports — on async, _single returns a coroutine and the string comparison silently returns False for every hash. Moved to PolyswarmAPI (sync) and PolySwarmAsyncAPI (async) as a pair, with the async version inserting `await` at the _single call. While here: collapsed the if/else into a single boolean return.
download, download_id, download_sandbox_artifact, and download_archive all read the _single result to close the LocalArtifact handle before returning. On async, _single returns a coroutine and `artifact.handle.close()` raises AttributeError. Moved each to PolyswarmAPI (sync, unchanged body) and PolySwarmAsyncAPI (async, with `await` at the _single call) as pairs.
sample_bundle_download and llm_report_download both issue two HTTP calls — fetch the task, then download its presigned-url payload. They read the first _single result to branch on state and to build the second request. On async, both operations operate on a coroutine and fail. Moved to PolyswarmAPI / PolySwarmAsyncAPI as pairs.
report_download fetches the report task (via self.report_get, which is itself a _single call) then state-branches and downloads. Both reads break on async since the base report_get returns a coroutine for the async client. Moved to PolyswarmAPI / PolySwarmAsyncAPI as a pair. The async version awaits report_get directly rather than chaining through the base.
report_template_logo_download / _delete / _upload each fetch the template (one _single) then act on the returned ReportTemplate (another _single via its bound download_logo / delete_logo / upload_logo). On async the first _single returns a coroutine and `report.<op>()` raises AttributeError. Moved to PolyswarmAPI / PolySwarmAsyncAPI as pairs.
…rved out Codify the rule the carve-out work enforces: - Endpoint methods on PolyswarmAPIBase must be single-statement bodies (`return self._single(...)` or `return self._paginate(...)`). The shared-body trick relies on _single's return value being inert until awaited, so anything that consumes the result needs `await` on the async side and can't be shared. - Multi-statement bodies live on the subclasses as sync+async pairs. Sync is the source of truth; async mirrors the body with `await` inserted at each _single call. - PolyswarmRequest is one HTTP call (paginated counts as one templated call). Composing multiple round-trips inside a single request type is not allowed. - File paths updated: aio/api.py now holds PolySwarmAsyncAPI; aio/__init__.py is a thin re-export. - sandbox_providers shape clarified: both transports return the executed request; caller reads .json['result'][slug]. - Catalogued the 11 multi-statement carve-outs in the endpoint table. - Noted the async-cassette coverage gap as a follow-up.
Code reviewGitflow ✅ (base = 1. Correctness — async
|
Pre-requisite for unasync codegen: sync and async upload paths must share the same structural shape (module-level function taking a client, upload_url, artifact). Previously sync did the upload inline on the resource method via httpx.put; async had a module-level async_upload_file. unasync can't bridge that asymmetry. Now: - polyswarm_api.upload.upload_file is a module-level sync function mirroring polyswarm_api.aio.upload.async_upload_file. - LocalArtifact.upload_file / SandboxTask.upload_file are thin shims that route through the module-level function with the api session's httpx.Client — preserves the existing instance-method surface so no downstream caller breaks. - Connection pooling improves on the sync side as a side effect (the session client is reused instead of httpx.put creating a fresh client per call). Also drops unused async_upload_logo (imported but never called).
Lands the tooling (script + pre-commit hook + GitLab CI staleness check) inert. Phase 3 will actually run the codegen and replace the hand-written sync client with generated mirrors. - scripts/unasync.py drives the transformation from aio/*.py to root. Rename map covers class names, httpx types, typing helpers, asyncio primitives, and the polyswarm_api.aio. import-path prefix. - .pre-commit-config.yaml hooks the script on any change under aio/. - .gitlab-ci.yml gains a test-unasync-mirror job that fails CI if the generated sync is stale relative to canonical async. - pyproject.toml grows a [dev] extra with unasync + ruff.
aio/api.py becomes a self-contained async client: ~75 endpoint methods that previously lived on PolyswarmAPIBase are now async def methods on PolySwarmAsyncAPI directly. Each `return self._single(...)` becomes `return await self._single(...)`; each `return self._paginate(...)` becomes an `async for ... yield ...` async-generator body. The class no longer inherits from PolyswarmAPIBase — _coerce_request and __repr__ are pulled in. _parse_rule is pulled in. The 11 multi-statement methods that lived as sync+async carve-outs (exists, download family, sample_bundle_download, llm_report_download, report_download, report_template_logo_*) stay where they already were on the async side. The sync versions in api.py remain until Phase 3 runs unasync and replaces them with generated mirrors. _base.py is still present but no longer referenced from aio/api.py; it gets deleted in Phase 4.
scripts/regenerate_sync.py drives the codegen. Running it overwrites the sync surface (api.py, core.py, upload.py at the package root) from the canonical async source under aio/. The script: - Runs unasync with a rename map covering class names (Async* -> sync equivalents), httpx types, asyncio.sleep -> time.sleep, the polyswarm_api.aio. import-path prefix, and the upload helper name. - Patches the engines @Property (sync gets a working cached property; async raises AttributeError — Python properties can't await). - Dedupes the duplicate `import time` produced by the asyncio -> time rename. - Adds a `# DO NOT EDIT` header to the generated files. - Runs ruff format on the output for stable bytes. Structural changes: - New module polyswarm_api/_bases.py holds BaseJsonResource, BaseResource, HttpxResponseAdapter, Hashable, and the small helpers. These were duplicated when the codegen produced both sync and async cores, which broke `issubclass` checks across modules. Pulling them into a hand-written shared module gives them a single class identity. - aio/core.py now imports the bases from _bases and defines only the transport-specific AsyncPolyswarmSession / AsyncPolyswarmRequest. - aio/api.py drops the docstring scaffolding that pretended to describe both transports; _coerce_request is now duck-typed (no isinstance against the sync request class). - The `close` method on the async client is renamed to `aclose` so the unasync map can rewrite it to `close` on the sync side. The script is now named scripts/regenerate_sync.py (previously scripts/unasync.py) because the original name shadowed the third-party `unasync` package on import. Pre-commit hook and GitLab CI job updated for the rename.
ReviewSpec coverage is solid — Correctness1. 2.
Pick one: add Spec driftNone substantive. The Downstream contractBehaviour change to flag for the release PR. Test coverage
GitflowBase |
Bot review + import-surface audit caught real issues across two areas. Bug fix (bot #1): ReportTemplate.upload_logo built a PolyswarmRequest with `data=` carrying a file-like. httpx 0.27 reserves `data=` for Mapping form bodies — a raw byte payload must go through `content=`. Under requests this worked; under httpx it didn't. Read the file into bytes and pass `content=` so the outbound PUT body is non-empty. Import-surface compat (preserving what was importable on develop): - `from polyswarm_api import PolyswarmAPI` is documented (spec 05, AGENTS.md "Caller surface unchanged", canonical aio/api.py docstring) but wasn't actually exposed at the top level — the bot caught this. Added the re-export. - `from polyswarm_api.aio import AsyncPolyswarmRequest, AsyncPolyswarmSession` worked on develop (re-exported through the giant inline `aio/__init__.py`). The slim codegen-era `aio/__init__.py` had dropped them. Restored as explicit re-exports from `aio.core`. - `async_upload_logo` (function + monkey-patch site) had been removed as "dead code" in an earlier round. It's part of the develop import surface (`polyswarm_api.aio.async_upload_logo`, `polyswarm_api.aio.upload.async_upload_logo`). Restored on the canonical async side; the rename map (`async_upload_logo` -> `upload_logo`) generates a sync mirror at `polyswarm_api.upload.upload_logo`. Note: the function is preserved as a public callable but not used internally — the resource-method upload path (ReportTemplate.upload_logo via `_single(...)`) is what `report_template_logo_upload` actually calls. Tests: - `test_async_report_template_logo_upload` exercises the full `report_template_logo_upload` flow against respx, asserts the PUT body is the file bytes and the Content-Type header is set. Would have caught the data=/content= bug at PR time; locks in the fix. Other bot notes addressed in chat (not code): - `_coerce_request` / `_exec` docstrings reference `AsyncPolyswarmRequest` in the generated sync `core.py` — string literals; unasync doesn't rewrite them. The canonical docstring already calls out the sync mirror; tolerable as the bot noted. - `wait_for` / `report_wait_for` test gap — the asyncio->time codegen rule it would exercise is documented as a constraint, and breakage would manifest loudly (e.g. `time.gather`) rather than silently. Not blocking; better tested at the codegen smoke layer if at all. - Major-version bump (3.x -> 4.0) for the develop->master step is noted in the PR description; this PR stays at 3.21.0 per gitflow.
Review against AGENTS.md +
|
…ownload test
Bot review flagged two real spec-drift sites and a test gap on the
post-_single download paths. User asked for a broader audit of
spec ↔ implementation alignment in the same pass.
Spec fixes:
- specs/00-overview.md: rewrote the Architectural snapshot and the
repo-layout listing — they still described the retired
PolyswarmAPIBase shared-base model. Now they describe the
async-canonical + codegen architecture, name the relevant files
(_bases.py, aio/api.py, aio/core.py, aio/upload.py canonicals;
api.py / core.py / upload.py generated mirrors), and put the
PolyswarmAPIBase mention in historical context only.
- specs/02-resources.md: three sites still said
`PolyswarmAPIBase._single` / `PolyswarmAPIBase.search` / "_base.py's
sync-only carve-outs". Updated to PolyswarmAPI / PolySwarmAsyncAPI
and to describe the actual multi-step flow as it lives on the
canonical async side. Also corrected the `LocalArtifact.upload_file`
description: it goes through `polyswarm_api.upload.upload_file`
with the session client (or a one-shot sync client when the api
is async), not raw `httpx.put`.
- specs/05-downstream-contract.md: documented `async_upload_logo` as a
preserved-for-compat monkey-patch site. It was importable on
develop but isn't called internally — the bot flagged that
shipping a module-level callable the SDK doesn't use itself locks
it into the contract silently. Now it's explicit: kept for import
surface compat, not used internally, candidate for removal on a
major version willing to break the import surface.
Test added:
- test_async_report_download_multistep_handle_close — respx-driven
full async exercise of `report_download` (GET task → state branch →
GET rendered file → close local handle). The post-_single logic
(state == 'PENDING' raise, `result.handle.close()`) is exactly the
multi-statement-async regression risk that motivated the codegen
architecture. Locks the path in without needing the live e2e.
Other bot notes addressed in chat (no code change):
- _coerce_request / _exec docstrings reference AsyncPolyswarmRequest
in the generated sync core.py — string literals; unasync doesn't
rewrite them. Tolerable; bot agreed.
- Major-version bump for develop → master is flagged in the PR
description for the maintainer; this PR stays at 3.21.0 per
gitflow.
Broader spec audit results:
- grep for PolyswarmAPIBase / _base.py in specs/: clean except the
two historical mentions in 00-overview.md and 01-architecture.md
("the previous / historical PolyswarmAPIBase architecture") which
are intentional.
- grep for httpx.put / requests.put / requests.Session: only
contextual mentions of the 3.x → httpx migration remain (in
05-downstream-contract.md and 99-open-questions.md) which are
correct.
- 03-endpoints.md and 04-testing.md scan clean.
ReviewArchitecture matches A few concrete things to address: Correctness
Test coverage
Nits (no action required)
|
…eal retry
Bot review caught four correctness items.
1. next_page() dropped parser_kwargs. Page 1 forwarded the request's
parser_kwargs to parse_result_list; page 2+ did not, so paginated
resources that need extra parser kwargs (none today, but the
contract documents the forwarding) would silently lose them after
the first page. One-line fix: forward `**self.parser_kwargs` to
the next-page request constructor. Mirrored on both
AsyncPolyswarmRequest and the generated sync PolyswarmRequest.
2. Upload helpers leaked the session-level Authorization header to
pre-signed S3 URLs. async_upload_file / async_upload_logo called
client.put() directly, and the httpx client merges its session-
level Authorization (the PolySwarm API key) into every outgoing
request. Pre-signed URLs handle auth via the query parameters; we
don't want to ship the API key to a third-party object store.
Fixed with a new private helper `_put_without_session_auth` that
uses `client.build_request` + `.headers.pop("Authorization")` +
`client.send(req)` — same pattern the session uses for explicit
None-suppression.
3. Upload helpers had a dead retry loop. `while attempts > 0 and not
r:` with `r.raise_for_status()` inside the body meant any non-2xx
raised straight out, never decrementing attempts. The `attempts`
parameter was a lie. Replaced with a real retry: catches
httpx.HTTPStatusError + httpx.TransportError, re-raises the last
one if all attempts fail.
4. (Verified-not-fixed) sandbox_url's PUT finalize uses
`params={"sandbox_task_id": ...}` while sandbox_file uses
`params={"id": ...}`. Both shapes ship on the develop branch (the
server presumably accepts both); no VCR cassette covers the URL-
finalize path. Per the preserve-import-surface directive, not
reconciling blindly. Flagged in specs/99-open-questions.md for a
future PR with a cassette.
Tests added (covers items 1, 2 implicitly + bot's coverage gap):
- test_async_upload_helper_strips_session_authorization — calls
async_upload_file with a session that has an Authorization header,
asserts the outgoing PUT has no Authorization.
- test_async_download — single-step canonical async path: GET
/consumer/download/sha256/<hash>, write file, close handle.
- test_async_sample_bundle_download_multistep — full multi-step
canonical async path: GET /bundle (task) → state branch → GET
presigned zip → close handle.
81 tests pass.
Nit deferred (no action):
- client_scan_test.py uses default VCR matchers while
async_client_test.py is explicit. Both effectively match on
scheme/host/port/path/query; sync cassettes already replay
correctly. Not worth churning.
|
Reviewed against Correctness
Spec / docs
Downstream contract
Tests
Gitflow / version
LGTM. |
Establishes the design baseline before any code changes land. The 4.0 shape: pure description (core.py) → transport (session.py) → client (api.py); PolyswarmRequest becomes a pure dataclass descriptor; parse_response is a pure function; the session is the only place HTTP I/O happens; resources stay transport-agnostic; customization is via session subclassing + injection (no module-level monkey-patch sites). - AGENTS.md: rewrite architectural shape; update "adding a resource" flow around core.py / session.py / api.py. - 00-overview.md: new 4.0 file tree and three-tier architectural snapshot. - 01-architecture.md: invariants, file table, layer-by-layer detail, call flow, customization hooks, codegen workflow. - 02-resources.md: invariants forbidding transport methods on resources; PolyswarmRequest dataclass shape; parse_response truth table; resource catalogue without instance HTTP methods. - 03-endpoints.md: file-upload paths now go through self.session; sandbox_providers via session.execute. - 04-testing.md: add pure-unit as a first-class tier alongside respx-mocked and VCR-cassette. - 05-downstream-contract.md: 4.0 public surface, session injection examples, full 3.x → 4.0 migration with code samples. - 99-open-questions.md: drop items resolved by the redesign (file-upload monkey-patch, BaseJsonResource location); preserve sandbox_url finalize-param inconsistency and missing async cassettes.
Land the architecture described in the just-committed specs. Three layers — pure description (core.py) → transport (session.py) → client (api.py) — and two transports (async-canonical + unasync-generated sync). Breaking surface changes (see 05-downstream-contract.md for migration): - PolyswarmRequest is now a @DataClass with keyword fields (method, url, params, json, headers, content, data, files, timeout, result_parser, parser_kwargs). No .execute() method. After execution the .json field is overwritten with the parsed response body (legacy semantics preserved). - parse_response is a pure function. The session executes a request and delegates to parse_response — no I/O outside the session. - The session class gains three I/O methods: execute(request), upload_file(url, artifact, …), upload_logo(url, file, content_type, …). Custom HTTP behaviour is via subclass + inject via PolySwarmAsyncAPI(session=…) / PolyswarmAPI(session=…). - Module-level upload sites (upload.py, aio/upload.py) are gone; their semantics live on the session. Resource instance methods ArtifactInstance.upload_file, SandboxTask.upload_file, and the presigned-S3 helpers are also gone — call session.upload_file directly with the resource's upload_url. - _bases.py is gone; its shared content (BaseJsonResource, Hashable, HttpxResponseAdapter, helpers) moved into the new hand-written core.py. aio/core.py is replaced by aio/session.py. Codegen: - regenerate_sync.py rename map slimmed (no more AsyncPolyswarmRequest, async_upload_file, async_upload_logo). Generates session.py + api.py from aio/session.py + aio/api.py. - engines-property escape hatch unchanged. Public surface re-exports: - polyswarm_api.PolyswarmAPI, PolyswarmSession, PolySwarmAsyncAPI, AsyncPolyswarmSession. Testing: - New test/core_test.py adds 37 pure-unit tests for PolyswarmRequest, parse_response (HEAD, 2xx, 204, 404, 422, 429, 500, non-JSON 404/500, fire-and-forget), a sampling of resource builders, hash validators, and HttpxResponseAdapter. - async_client_test.py updated to drive the new session.execute / session.upload_file surface in the three tests that touched the pre-4.0 module-level upload helpers and the session.request method. Version bumped to 4.0.0. Total: 118 tests pass; codegen verified byte-stable across two consecutive runs.
Review notes (against
|
Three categories of fixes from the latest claude-bot review. Code: - Drop session.upload_logo. Production never called it: the report- template logo upload PUTs to the auth'd /reports/templates/logo endpoint, not a pre-signed S3 URL, so it rides on session.execute via the descriptor ReportTemplate.upload_logo() returns. The dead method was surface area without callers. - Drop the unused PolySwarmAsyncAPI._sleep helper — wait_for and report_wait_for call asyncio.sleep / time.sleep directly. - Revert the version bump 4.0.0 → 3.21.0. AGENTS.md gitflow: version bumps belong to the develop → master release PR, not feature PRs. pyproject.toml ([project].version + [tool.bumpversion].current_version) and src/polyswarm_api/__init__.py:__version__ all back to 3.21.0. - Make canonical session.py / api.py docstrings transport-neutral so the unasync-generated sync mirror reads correctly. Unasync rewrites tokens, not string contents, so phrases like "async source lives here" leaked into the generated sync file. Specs (specs were drifting; code was right): - Exception attribute is .request (set by RequestException.__init__), not .result. The session does not catch and rewrap. Fixed in 01-architecture.md, 02-resources.md, 05-downstream-contract.md. - PolyswarmRequest field is .json, not .json_body. After execution parse_response overwrites the same .json field with the parsed response body (legacy 3.x semantics). Fixed in 01 and 02. - PolyswarmRequest dataclass signature in 02 and 05 was missing api / data / files / timeout and listed a nonexistent `stream` field. Replaced with the real fields. - tag_link_list was mis-classified under the _paginate table in 03; it actually uses _single (the endpoint returns a single page). Moved to the relevant _single table with a note. - Report-template logo upload was wrongly documented as going through session.upload_logo in 03 and 05; it goes through session.execute via report.upload_logo() (a normal authenticated PUT). Fixed and explicitly called out that session has only two I/O methods now (execute + upload_file). Sync mirrors regenerated; 118 tests still pass; codegen idempotent.
ReviewCorrectness
Spec drift
Minor
Gitflow / versionBase is |
Bot review caught a real correctness bug plus three spec/cleanup items. Correctness — _next_page clone preserves the input send-body, not the response body. - ``PolyswarmRequest.json`` is overloaded: input pre-execute, parsed response body post-execute (legacy 3.x semantics, preserved on purpose so callers like sandbox_providers can read .json['result']). _next_page cloned ``json=request.json`` which, after the first page executed, was the response body. Page 2's GET shipped page 1's response body as its request body. VCR didn't catch it (matcher excludes body) and most servers tolerate ignored GET bodies, but it's still wrong. - Add ``_input_json`` shadow field, set in __post_init__, untouched by parse_response. _next_page now clones via ``json=request._input_json``. - New pure-unit regression test in test/core_test.py confirms the snapshot survives a simulated parse_response overwrite of .json. Spec drift sweep. - AGENTS.md §"Architectural shape" and specs/00-overview.md (the artefact table, the file-tree comments, the architectural-snapshot ASCII, and the customization callout) still listed session.upload_logo as a third I/O method. Removed everywhere — the session has only execute and upload_file. - core.py parse_response docstring still claimed "the caller (the session) attaches `.result = request` to the exception before re-raising". Wrong on both counts: the attribute is .request, and attachment happens in RequestException.__init__ at construction time, not in the session. Rewritten. Minor. - resources.py:1195 ``upload_logo(self, logo_file, content_tpe)`` param typo renamed to content_type. Callers pass positionally so no public-surface impact; just IDE / inspect clarity. The legacy spelling stays on the api-level ``report_template_logo_upload`` keyword (explicit back-compat alias documented there). Sync mirrors regenerated; all 119 tests pass.
Deleted every .vcr cassette and ran the full suite against the local e2e (http://localhost:9696/v3). 22 cassettes regenerate cleanly under the new 4.0 transport — they confirm that the new ``PolyswarmRequest`` + ``session.execute`` pipeline produces wire output the live server accepts and parses responses correctly. The pre-existing ``test_sandboxtask_get.vcr`` is dropped — it served a disabled test method (``ytest_sandboxtask_get``). 25 cassettes stay pinned to their prior recordings because the local e2e currently can't replay them: server-side 500s on metadata-query / ioc-search / artifact-metadata-list, a 400 on the artifact-metadata POST, and missing fixture data (ioc id=1, sandbox tasks, historical hunts, EICAR sample, stream feed, expected ruleset count). These are data / server-side issues, not SDK issues — the cassettes already on disk continue to replay cleanly under the new transport. Tracked in ``specs/99-open-questions.md`` with the affected-cassette list and the required priming work. Result: full 119-test suite passes against the mix of fresh + restored cassettes.
|
Reviewed against Commit history hygiene (AGENTS.md §Commit + PR hygiene)
Internal ticket
Commit
These are baked into the merged history; if this PR is squash-merged the squash commit message can be sanitised, otherwise the existing commits should be rewritten with Docstring drift —
|
ReviewPR is large but well-aligned with the documented 4.0 architecture (three layers + two transports + async-canonical/unasync-generated sync, per Correctness / behaviour
Spec drift / convention
Test infra
All other invariants (canonical-async source of truth, session as the single I/O entry point, |
…p + respx coverage
Bot review found one code cleanup and several spec drift / coverage gaps.
Code:
- refresh_engine_cache now uses resources.Engine.list(self) instead of
building the descriptor inline. Engine._list_headers already returns
{'Authorization': None}, so the builder produces the same wire shape.
Brings the method in line with AGENTS.md §"When adding a new resource"
and specs/02-resources.md.
- Canonical docstrings in aio/session.py rephrased to be transport-
neutral ("AsyncClient on the async transport; Client on the sync
transport"). Reads correctly from both the canonical and generated
files.
Specs:
- specs/01-architecture.md: dropped the "PR #298" reference (this PR;
rots after merge). Added a note linking HttpxResponseAdapter to the
streaming-downloads follow-up in 99-open-questions.
- specs/04-testing.md: added an explicit note that _AsyncToSync is
respx-only — the harness builds the AsyncClient outside any event
loop and drives every call via asyncio.run, which doesn't survive a
live-HTTP integration test (per-call event loop vs. pooled
AsyncClient connections).
- specs/05-downstream-contract.md: documents two new entries under
"Backward compatibility — what changes":
* check_known_hosts now returns a generator on sync too (was a
list in 3.x due to the polymorphic-return trick; the docstring
already promised "Generator of IOC resources").
* Downloads buffer the full body in memory before chunking
(regression vs. requests-backed 3.x streaming).
- specs/99-open-questions.md: new entry "Streaming downloads —
HttpxResponseAdapter fully buffers" with concrete refactor proposal
(use client.stream + async-aware adapter); rewrote the "cassettes
that need an e2e refresh" entry to document why the 24 stale
cassettes can't be regenerated cleanly (hard-coded primary keys,
order-coupled state, count assertions on shared resources, missing
fixtures, eventual-consistency assertions, surviving server bugs).
Tests:
- Added respx test test_async_report_template_logo_delete pinning the
two-step flow (GET template → DELETE /reports/templates/logo?id=...).
Closes the gap the bot flagged: the upload + download paths already
had respx coverage; the delete path was VCR-only.
VCR cassettes:
- Round-2 re-record pass: deleted all cassettes, re-ran against the
live e2e. 23 cassettes regenerated cleanly under the new transport
+ the _input_json snapshot fix. 24 stay pinned to their prior
recordings — the corresponding tests aren't hermetic (hard-coded
IDs, order-coupled state, etc.). See specs/99-open-questions.md
for the full breakdown.
Sync mirrors regenerated; all 120 tests pass.
ReviewArchitectural shape, codegen, and test coverage all line up with the specs. Three small issues worth fixing in this PR; gitflow and version-bump policy are clean. Spec drift —
|
Refactor the VCR-backed integration suite so every test stands on its own against a freshly-provisioned artifact-index (make reset-database + docker/provision.sh). No hard-coded primary keys, no count assertions on shared resources, no leftover-state dependencies. Each test provisions exactly what it needs via SDK calls in its body and captures real identifiers from API responses. Patterns applied: - Submit a fresh artifact and use the returned id/sha256 instead of pinning to a frozen instance_id from a prior recording. - For the known-good-host suite, capture the id from add_known_good_host's response and feed it into update/delete; tolerate NotFoundException in cleanup because the ioc_cache divergence (cache returns stale ids the delete endpoint no longer finds) is still upstream-pending. - For sandbox dispatch, add a small retry helper riding out the storage-pointer lag window (submit returns 200 before the storage path is committed; sandbox 422s for a few seconds after). - For historical results / live / sandbox-latest, accept the empty- feed outcomes the local e2e produces without a microengine cluster via @pytest.mark.skip with explicit upstream reasons. - For test_rules, switch from "assert len == 1" to a presence check so the test tolerates leftover rulesets across runs. 10 cassettes are now @Skip'd against four documented upstream artifact-index issues (see specs/99-open-questions.md for the full breakdown): 1. GET /v3/artifact/metadata/list 500s ("Something went wrong" via the middleware catch-all). Affects tool_metadata sync + async. 2. iocs_by_hash / search_by_ioc return empty even when the matching metadata is attached to the instance — either a stale get_fields_with_tag memoize cache or extract_iocs not walking the cape_sandbox_v2 root. Affects iocs_by_hash + search_by_ioc sync + async. 3. sandbox_task_latest depends on SandboxTaskSearchHash, populated only on SUCCEEDED — needs cape/triage workers running. Affects sandboxtask_latest sync + async. 4. live feed requires the microengine bounty pipeline running. Affects live sync + async. specs/99-open-questions.md rewritten with the four issues fully spelled out (endpoint, what it does, what it should do, where to look in artifact-index). Cassettes for the four blocked groups are removed from disk — they'll re-record when the upstream issues are resolved. Result: 110 / 110 non-skipped tests pass. 36 cassettes regenerated cleanly against the freshly-provisioned local e2e. Codegen idempotent.
ReviewArchitecture lines up with A handful of spec/code drifts worth fixing in the same PR:
No correctness bugs found. The pagination semantics ( |
artifact-index #1877 fixed the GET /v3/artifact/metadata/list 500 (autobegin + bigint-vs-varchar). With a restart of the local e2e the fix is live, so test_tool_metadata (sync + async) is un-skipped: - Submits to provision a real instance. - POSTs two tool_metadata blobs. - Polls tool_metadata_list (the formerly-broken endpoint) until both rows surface from the asynchronous persist_external_metadata Celery task. 30s timeout is comfortable; Celery roundtrip is ~3s in isolation and can stretch under suite load. For the remaining IOC tests (iocs_by_hash + search_by_ioc), the skip reason is refined now that the memoize-cache theory has been ruled out by direct in-process verification: get_fields_with_tag returns the 17 ip-ioc paths correctly and extract_iocs walks the matching tool_metadata to the IP. The bug is somewhere between the HTTP view's input shape and the extract_iocs call — most likely the search-by-hash fallback (SandboxTaskSearchHash empty → last_scanned_instance) resolves to a sibling instance that doesn't carry the freshly-attached cape_sandbox_v2 metadata. Final tally: 112 / 120 endpoint tests pass against a fresh e2e. 8 skip with explicit upstream reasons (4 unique tests × sync+async). 38 cassettes regenerated cleanly; replay run also green.
Review — clean against the documented conventionsI read AGENTS.md and the seven specs, then walked the PR. The 4.0 refactor matches the specs it ships alongside: pure A few small things worth surfacing — none blocking: Minor / footguns
Tracked in 99-open-questions and acknowledged
Nothing in the changeset contradicts a spec invariant; no breaking change is undocumented; the codegen mechanic and the LGTM. |
… + slow Celery) The skip reasons were wrong. Two non-bug behaviours combined: 1. filter_known_good_iocs correctly drops IPs that are marked known-good. The original test used 1.2.3.4 — that IP gets added and deleted by test_check_known_host, but artifact-index's app.ioc_cache invalidates async via long_running.reload_ioc_cache, so the cache keeps serving "known-good" long after the DB row is gone. The IOC view's filter then drops the IP from the response. This is correct security behaviour; the test just picked an unfortunate IP. Switching to fixed IPs in the 9.42.0.0/24 range (public IBM netblock, never touched by other tests) avoids the filter. 2. persist_external_metadata (Celery task) takes ~30s under suite load (single worker, multi-queue contention). The original poll was 15s. Bumped to 60s. Both tests now self-provision and pass cleanly. Cassettes use the fixed IPs so VCR record→replay matches deterministically. Final tally: 116 passing / 4 skipped (sandbox_task_latest + live, sync+async — both environment-dependent: sandbox needs cape/triage workers running, live needs the microengine bounty pipeline). 42 cassettes regenerated and replay green.
Review against
|
…live + cold-index tests The e2e now runs the released artifact-index image (the metadata/list ro_session + bigint-cast fix), so tool_metadata round-trips and the suite can be recorded against real data again. - live (sync+async): bound the feed with since= (seconds) so the cassette no longer pages the entire global feed — test_live drops from ~3MB/1355 interactions to ~72K/43. Wrap the hunt lifecycle in try/finally (live_stop then ruleset_delete, both tolerant) so a mid-test failure can't leave an active hunt capturing every later EICAR submit — that leak is what accumulated 700+ stale LiveResult rows and bloated the cassette in the first place. - metadata_search (sync+async): was a one-shot list() that assumed instant ES indexing; now polls (the ES metadata write indexes in ~1s idle but lags under full-suite Celery load, so a generous window). - one-shot searches gained a shared _poll_results helper to ride out cold-index latency on a freshly-provisioned e2e. - stream + search_by_ioc (sync+async): skipped with precise reasons — genuine e2e-capability gaps, not SDK bugs. The archiver service that feeds /stream and the reverse-IOC search index behind /v3/search/ioc don't run in the local container stack (forward iocs_by_hash indexes in ~28s; the reverse never populates past 90s). Same class as the pre-existing sandbox_task_latest skip (needs cape/triage workers). Result: 115 passed, 5 skipped; replay green; total cassettes 5.7MB -> 652KB; codegen mirror unchanged.
|
Review against AGENTS.md / specs/ Major architectural shape lines up cleanly with A handful of action items, ordered by severity. Correctness
Spec drift
Style / minor
Strengths
Nothing blocking — the constructor mismatch (1) and the |
…threshold The stream feed wasn't empty because the archiver is missing; it's threshold-triggered. The e2e sets ARTIFACT_ARCHIVES_INSTANCE_COUNT=3 with ARCHIVES_CREATION_DELAY=30s, so an archive is created only once the accumulated instance count *exceeds* 3. The earlier single submit never crossed it (and the 4-min timeout path hadn't elapsed), so the feed looked empty. Submit 5 EICAR to cross the >3 threshold, then poll the stream feed (generous window to cover the ~30s creation delay) and download + verify the archive. Consume only the first archive via next(iter(...)) rather than list(api.stream()) so paging the growing archive feed doesn't bloat the cassette (44K, one stream page). 116 passed, 4 skipped (search_by_ioc x2 — reverse IOC index not backed locally; sandbox_task_latest x2 — needs cape/triage workers).
|
Reviewed against AGENTS.md + specs/. Architecturally clean — gitflow respected (base develop, no version bump), three-layer split matches specs/01-architecture.md, codegen pipeline and CI staleness check are in place, breaking-change inventory in specs/05-downstream-contract.md covers the surface deltas. Findings below are minor spec/code drift or pre-existing quirks worth either fixing or surfacing in the open-questions list before this lands. CORRECTNESS
SPEC DRIFT
DOWNSTREAM CONTRACT
TEST COVERAGE
GITFLOW Clean. Base develop, version untouched (init.py + pyproject.toml stay at 3.21.0), version bump deferred to the develop → master step as AGENTS.md Gitflow section requires. |
Both the forward (iocs_by_hash) and reverse (search_by_ioc) IOC tests attach an artificial cape_sandbox_v2 blob via tool_metadata_create rather than waiting on a real sandbox run — the end-to-end stack has no cape sandbox worker, so the metadata pipeline never produced one. With the stack's Elasticsearch flush interval now at 5s, the reverse search (which reads Elasticsearch) resolves in seconds and no longer needs to be skipped. - Forward: keep the shared EICAR fixture. The forward view resolves a hash via the sandbox-task search index (seeded only for EICAR) and reads that instance's metadata, so the mock blob surfaces directly. - Reverse: submit a unique, deterministic artifact. The ES document is built from the search row's last_scanned_instance, and a real sandbox task would overwrite cape_sandbox_v2 in it — so a fresh sha (sole instance, no sandbox task) is required for the mock blob to survive. The submit response returns sha256=None for a brand-new artifact, so hash the bytes locally, and poll until our sha resolves (not merely any result, which an ip reused across artifacts can transiently return). Re-recorded the four IOC cassettes against the live stack.
ReviewScoped against Correctness
Spec drift
Test coverage
Housekeeping
|
2 participants
TL;DR
Consolidates the sync + async clients around a three-layer architecture and
unasynccodegen. One canonical async source, one generated sync mirror; both share a pure description layer and a pure response parser.core.py, hand-written, no I/O):PolyswarmRequest(@dataclassdescriptor),parse_response(pure function),BaseJsonResource,Hashable,HttpxResponseAdapter, helpers.aio/session.pycanonical →session.pygenerated): one session class per transport owns an httpx client and exposes the SDK's two I/O methods,execute(request)andupload_file(url, artifact, …). No other module reads or writes HTTP.aio/api.pycanonical →api.pygenerated):PolySwarmAsyncAPI/PolyswarmAPIown a session, acceptsession=for injection, drive pagination by cloning descriptors viadataclasses.replace.requestsis dropped as a runtime dependency. Customization is via session subclass +PolyswarmAPI(session=…), replacing the 3.x module-level monkey-patch pattern.Architecture
scripts/regenerate_sync.pyruns unasync with a rename map (AsyncPolyswarmSession→PolyswarmSession,AsyncClient→Client,asyncio→time, etc.), patches the one divergent block (theenginesproperty), dedupes imports, prepends a# DO NOT EDITheader, and runsruff formatfor stable output. CI (test-unasync-mirror) reruns the script andgit diff --exit-codeto reject stale mirrors. Pre-commit runs it locally.Detailed contracts
Live under
specs/. Each spec is independently readable and opens with Scope + Invariants:specs/00-overview.md— what the SDK ships, repo layoutspecs/01-architecture.md— three layers, two transports, call flow, codegen, customizationspecs/02-resources.md—BaseJsonResource, builder convention,PolyswarmRequestdataclass,parse_responsetruth tablespecs/03-endpoints.md— full endpoint catalogue,_singlevs_paginate, special methodsspecs/04-testing.md— three test tiers (pure unit / respx-mocked / VCR),ClientTestCaseharnessspecs/05-downstream-contract.md— public surface + 3.x → 4.0 migration notesspecs/99-open-questions.md— known follow-upsHow a method body works on both transports
Canonical async (
aio/api.py):Generated sync (
api.py):Caller surface unchanged:
Multi-step methods (
submit,download,report_download,sandbox_file, etc.) are ordinaryasync defmethods on the canonical side — unasync handles the mirror.The one escape hatch
@property def engines(self):is the single site where sync and async legitimately need divergent code. Python properties can'tawait, so the async version raisesAttributeError; the sync version is a working cached property.scripts/regenerate_sync.pyrecognises the canonical AttributeError block by exact text and patches in the working sync property after unasync runs. If the canonical block drifts, the script fails fast.Public surface
Customization is per-instance, via session injection (full migration table + worked examples in
specs/05-downstream-contract.md):The
[async]extra inpyproject.tomlstays as an empty list so downstreampolyswarm-api[async]pins keep parsing.Breaking changes (4.0 surface)
These are documented in
specs/05-downstream-contract.md§"Backward compatibility — what changes". A version bump to 4.0.0 will land on the eventualdevelop → masterrelease PR, not in this feature PR (perAGENTS.mdgitflow).polyswarm_api.aio.upload.async_upload_file/async_upload_logoand their sync mirrors are gone. Customization moves to subclassing the session.LocalArtifact.upload_file(artifact),SandboxTask.upload_file(artifact)are gone. Replace withawait api.session.upload_file(instance.upload_url, artifact).PolyswarmRequestis a@dataclass. Old:PolyswarmRequest(api, request_dict, result_parser=cls, **parser_kwargs). New:PolyswarmRequest(api=api, method='…', url='…', params=…, json=…, headers=…, result_parser=cls, parser_kwargs={…}). No.execute()method — the session executes it.parse_responseis a pure function atpolyswarm_api.core.parse_response. The oldPolyswarmRequest.parse_result(...)method is gone.execute(request),upload_file(url, artifact, …). TheAuthorization-strip + retry behaviour is shared by both via_put_off_domain. There is noupload_logoon the session — the report-template logo upload PUTs to the authenticated PolySwarm endpoint viasession.execute, not a pre-signed S3 URL.Tests
test/core_test.pyadds 37 pure-unit tests for the dataclass, the parser (HEAD / 2xx / 204 / 404 / 422 / 429 / 500 / non-JSON 404 / non-JSON 500 / fire-and-forget), a sampling of resource builders, hash validators, andHttpxResponseAdapter. No httpx, no async, no fixtures.async_client_test.pyupdated in three places to drive the newsession.execute/session.upload_filesurface (the tests that previously poked the module-level upload helpers orsession.request).['method', 'scheme', 'host', 'port', 'path', 'query']— cassettes replay cleanly through the new transport).Outstanding follow-ups
Tracked in
specs/99-open-questions.md:ClientTestCaseharness out frommetadata_field_properties_test.pytoclient_scan_test.py/async_client_test.py— mechanical per-test rewrite.sandbox_providersexecuted-request quirk — preserved for backward compatibility; decide whether to convert to a parsed resource list in a future major.sandbox_urlfinalize-param inconsistency (sandbox_task_idvsid) — preserved pending cassette.Gitflow
Base =
develop✅. No version bump in this PR ✅ —__init__.pystays at3.21.0,pyproject.tomlstays at3.21.0; the bump to 4.0.0 happens at thedevelop → masterstep when the maintainer cuts the release.