Skip to content

docs: iocs_by_hash accepts sha1/md5 and documents AMBIGUOUS_HASH - #335

Merged
vhmartinezm merged 3 commits into
developfrom
DN-8581
Oct 28, 2026
Merged

vhmartinezm merged 3 commits into
developfrom
DN-8581

Conversation

@vhmartinezm

@vhmartinezm vhmartinezm commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

TL;DR

  • Document that iocs_by_hash accepts sha256, sha1 or md5 (the server now resolves a sha1/md5 to one artifact on both the ioc and ioc-beta routes).
  • Document the new machine-readable refusal for a sha1/md5 that names several artifacts: a 400 RequestException with exc.request.errors == {'code': 'AMBIGUOUS_HASH', 'hash_type', 'sha256s': [≤10], 'truncated'}. No typed exception, same rule as FAVORITE_LIMIT.
  • New dual-transport (sync + async) respx test test/ioc_by_hash_respx_test.py pinning that refusal on both routes.
  • No code change: the hash type was already passed through unvalidated, and RequestException already exposes errors. The two docstrings that wrongly described hash_value as a field list are fixed.

Tests

pytest test/: 433 passed locally.

Requires

The server-side change in polyswarm/artifact-index#2021. Against an older server a sha1/md5 is a plain 400, as before.

Not in this PR: e2e coverage of the sha1/md5 happy path

The reviewer rightly asks for test_iocs_by_hash / test_async_iocs_by_hash to also query by md5/sha1. That needs the cassettes re-recorded against a full stack running polyswarm/artifact-index#2021, which I could not bring up locally. The gap and the exact steps to close it are recorded in specs/99-open-questions.md.

…refusal

The server now resolves a sha1/md5 to one artifact on both IOC routes and
refuses one naming several artifacts with a machine-readable 400. The SDK
needs no code change (RequestException already exposes errors); document
the contract and pin the refusal with a dual-transport respx test.
@claude

claude Bot commented Oct 27, 2026

Copy link
Copy Markdown

Review: one issue needs action. Docs, gitflow (base develop, no version bump) and the downstream contract are fine.

1. Test coverage: the main new behaviour has no test (AGENTS.md "e2e-first"). The PR's headline is that a sha1/md5 now resolves to one artifact on both ioc and ioc-beta. That path can run on the e2e stack: every EICAR variant has its own md5/sha1, so this isn't the case AGENTS.md reserves for respx ("only when the scenario can't reasonably run on the e2e stack"). Today only the refusal is tested, and it's mocked. As it stands, nothing would catch the server dropping sha1/md5 resolution, or a route mismatch on ioc-beta.

  • Fix: in test_iocs_by_hash (test/client_scan_test.py) and test_async_iocs_by_hash (test/async_client_test.py), once the sha256 poll has settled, also call iocs_by_hash('md5', md5) and iocs_by_hash('sha1', sha1). Use the same artifact's hashes, and call both with beta=False and beta=True. Assert each returns the same ips/ttps as the sha256 lookup.
  • Then delete and re-record both cassettes against a fresh stack running the server change.
  • Update the 04-testing.md line to match. Respx stays the right tier for AMBIGUOUS_HASH only.

Minor (no action required): the refusal test runs both routes in one loop on one ClientTestCase. That's fine, but a failure on ioc-beta will show as the same test name; the path assert messages are what tell you which route failed.

@claude

claude Bot commented Oct 27, 2026

Copy link
Copy Markdown

Base (develop), version (untouched), no-typed-exception rule, and the docstring/spec updates all check out. One item needs action:

The deferred e2e follow-up has no entry in specs/99-open-questions.md. The PR body leaves the sha1/md5 happy path for later: test_iocs_by_hash / test_async_iocs_by_hash querying by md5/sha1 once the server change reaches e2e. Today no tier covers that path, not even respx; the new test only pins the refusal. The precedent for this kind of gap is search_by_ioc(with_artifacts=True) (99-open-questions.md § "is not pinned against a live server"), which records:

  • what is unpinned
  • what stands in for it meanwhile
  • the exact steps to close it

Please add a matching entry: extend the live pair with an md5 and a sha1 query on both ioc and ioc-beta, then re-record the four cassettes against a fresh stack running the server change. Without it, the follow-up exists only in a PR description. AGENTS.md: "Update the spec in the same PR as the code change."

@claude

claude Bot commented Oct 27, 2026

Copy link
Copy Markdown

Clean against AGENTS.md and specs/: targets develop, has no version bump (right for a docs-only change), regenerates the sync mirror, uses the dual-transport respx test the right way for a refusal the stack can't produce, and records the e2e gap for the sha1/md5 happy path in 99-open-questions.md. No action needed.

@alanjds alanjds left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reviewer rightly asks for test_iocs_by_hash / test_async_iocs_by_hash to also query by md5/sha1. That needs the cassettes re-recorded against a full stack running https://github.com/polyswarm/artifact-index/pull/2021, which I could not bring up locally.

This is interesting. Are we having issues with e2e locally?

@vhmartinezm
vhmartinezm merged commit 59b6bca into develop Oct 28, 2026
2 checks passed
@vhmartinezm
vhmartinezm deleted the DN-8581 branch October 28, 2026 16:46
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.

2 participants