Comprehensive audit: 12 proven bugs fixed, 160x spatial selection, test suite restored from 14 to 166 local tests - #14
Conversation
…source pin Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…async skips, stray collection, assertion-free test_debug) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ction to tests/ - conftest: replace the autouse skip-everything IPFS gate with pytest_collection_modifyitems gating only @pytest.mark.ipfs tests - pyproject: testpaths=["tests"], asyncio_mode=auto - mark genuinely gateway/daemon-dependent tests with ipfs/integration - relocate root-level test_stac_integration.py to scripts/ (manual script) - delete assertion-free tests/test_debug.py (coverage exists elsewhere) - meta-test hardening per grok + adversarial review: probe asyncio_mode with a generated unmarked async test; drop dead fixture overrides Locally-executed tests: 14 -> 109 (47 network-dependent skip cleanly). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nce typo Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ttrs, s3 attr guard, stac-server variants, quarter alias) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aggregation NaN wipe Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…0e-5 -> 1e-5 __aexit__ now closes KuboCAS via try/finally even when the Siren client's aclose() raises, then re-raises the original error. The snap_to_grid=False tolerance in point()/points() was 10e-5 (= 1e-4, ~11 m), silently snapping points documented to raise NoDataFoundError. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ted variants, STAC pagination, substring error classification) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ceiver); stop rolling_aggregation wiping NaN-celled grids polygons(): import rioxarray in-function (registers the .rio accessor and keeps the dependency optional), catch rioxarray.exceptions.NoDataInBounds instead of the nonexistent errors.NoDataInBounds, call reduce_polygon_to_point on the GeotemporalData wrapper rather than the Dataset, and drop the inplace rio mutations of the caller's dataset. rolling_aggregation(): dropna(time, how=any) emptied the whole time axis whenever any grid cell was NaN (ubiquitous in masked climate grids); trim only the incomplete leading windows instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ention, CancelledError priority) Follow-up to grok + adversarial review of WP-low: on dual cleanup failure propagate the later (Kubo) error with the Siren error as __context__ instead of a false __cause__ chain; a CancelledError from either cleanup always outranks an ordinary error. Adds dual-failure, cancellation, and with-block-forwarding tests; drops dead no-op fixtures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…or rectangle()/circle() Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… and O(n^2) concatenate Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ll chunks, eager heavy imports Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s3 attrs, variant item-id fallback, QE quarter alias - check_dataset_size: reduce initializer 1 (zero-dim datasets = 1 point) - to_netcdf: serialize from a copy; caller attrs no longer stripped - get_dataset_from_s3: optional update_in_progress/initial_parse/ update_is_append_only attrs no longer raise AttributeError - stac_server: variant matching falls back to the item id when properties lack dclimate:variant, and tolerates missing properties - temporal_aggregation: pandas quarter alias Q -> QE Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ession test, dead fixture cleanup) Per grok + adversarial review: rioxarray set_spatial_dims defaults to inplace=True, so pass inplace=False explicitly; replace the post-fix duplicate fallback test with a caller-dataset non-mutation pin; drop the no-op check_ipfs_connection overrides copied across the new test files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rectangle(): .where(mask, drop=True) deep-copied the full dataset via xr.align before cropping (measured 160x slower, 379 MB peak for a 4 KB result); replaced with .sel coordinate slices handling ascending and descending coord order, with empty selections normalized to the old drop=True shape. circle(): pre-crop to the circle's bounding box (pole crossings widen to the full longitude band; antimeridian-spanning boxes fall back to the uncropped path) before the haversine mask, output unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pe-based connection-error classification - stac_server/stac_catalog: dataset/variant parsing out of item ids is now shared and dataset-aware, so hyphenated dataset ids and hyphenated variants resolve identically in both resolvers - stac_server: /search follows rel=next links (bounded at 50 pages, loop guard) instead of silently truncating at the first 100 items - ipfs_retrieval: connection errors classified by exception type (requests/httpx/OSError families, incl. __cause__ chain) instead of substring matching on str(exc); existing substring-pinned test updated to real exception types Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…calar/variant test pins Per grok + adversarial review of WP-med: the listing API reports items without a variant segment as 'default', but resolve derived None for the same items, breaking list->resolve round-trips and skipping unnamed base items in the no-variant preference order. Bare items now count as the 'default' variant in both the page-scan early-exit and final selection. Adds reviewer-requested pins: default round-trip, unnamed-over-latest preference, s3 update-flag branch coverage, scalar=1-point semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ssion; single-pass concatenate - load_dataset/list fallback paths run resolve_cid_from_stac_server, load_stac_catalog, list_available_datasets and resolve_dataset_cid_from_stac via asyncio.to_thread (measured 204 ms event-loop stall per STAC call previously froze py-hamt's concurrent chunk fetches); catalog lazy-init guarded by an asyncio.Lock - IPFSStacIO uses one pooled requests.Session with a 30 s timeout instead of a fresh TCP+TLS handshake per ipfs:// read - concatenate_datasets collects deduplicated slices and concatenates once (iterative xr.concat was O(n^2) time and ~2x peak memory) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… imports - points() uses GeoSeries .y/.x vectorized accessors (16x at 50k points vs the per-Point python loop) - EncryptionCodec runs encrypt/decrypt inline for buffers < 128 KiB and keeps the asyncio.to_thread offload for larger ones (+79% overhead measured at 16 KB) - s3fs, geopandas and pystac import lazily (function-level / module __getattr__); package import time 973 ms -> 484 ms - red tests gain a key-restoration fixture (they leaked the process-wide encryption key into later tests); assertions unchanged Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…circle() Per grok + adversarial (differential/mutation) review of the zero-copy rewrite: .sel slicing silently drops rows on non-monotonic coords and errors on non-index/NaN/scalar coords that the old mask handled. Both selectors now verify coords are 1-D NaN-free monotonic dimension indexes and fall back to the legacy .where path otherwise. Adds the two pins mutation testing showed missing (descending-longitude rectangle, high-latitude cos-widened circle bbox) plus non-monotonic/NaN fallback pins; documents the zero-copy view and int-dtype preservation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, STAC merge next-links, unified lister parsing Review follow-ups for the suspects package (grok BLOCKER + adversarial FIX FIRST both addressed): - _is_connection_error no longer treats every OSError as a gateway failure: FileNotFoundError/PermissionError (and requests.HTTPError, which subclasses IOError) previously skipped the HAMT fallback; the classifier now matches only connection/timeout/DNS families plus requests/urllib3/httpx-transport/aiohttp types, with positive and negative parametrized pins. Note: gateway 504s (HTTPStatusError) now intentionally take the HAMT fallback instead of IpfsConnectionError. - resolve_dataset_cid_from_stac treats bare items as variant 'default', mirroring the STAC-server resolver, so the documented server->catalog fallback resolves identically (adversarial repro: bare 'chirps-temp' succeeded on the server path and raised on the catalog path) - _search_pages honors the STAC next-link 'merge: true' contract so page-2 requests keep the collections filter (poisoning repro fixed) - the IPFS catalog lister now uses the shared hyphen-aware id parsing and reports bare items as 'default', matching the server lister Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…param, thread-safety notes) Per grok + adversarial review: async alist_datasets() offloads the blocking STAC/catalog work and shares the catalog lazy-init lock (the sync list_datasets now documents that it blocks); get_root_catalog_cid gains the missing 30 s timeout (it is the first call of every catalog load); find_split_index drops its never-read combined_coords parameter; session/set_default thread-safety constraints documented in place. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…est, __dir__ for lazy modules, typed catalog) Per grok review: large-chunk roundtrips now assert exactly two thread offloads (encode + decode); the 128 KiB dispatch boundary itself is pinned; lazy modules implement __dir__ so introspection matches __all__; _stac_catalog regains its pystac.Catalog annotation via TYPE_CHECKING. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adversarial review of the vectorized points() found GeoSeries.y maps missing (None) geometries to NaN, which .sel(method='nearest') silently snaps to an arbitrary grid cell — now raises InvalidSelectionError. Also applies ruff format across the repo (pre-existing violations were mixed with new hunks; pre-commit runs ruff-format). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nused ones Every runtime dep now has a minimum version. aiohttp, urllib3 and multiformats are imported directly and are now declared instead of arriving transitively. pyarrow (unused) and python-dotenv (used only by examples/, now an 'examples' extra) removed from runtime deps. scipy stays with a comment: the peer review flagged it unused, but xarray's in-memory to_netcdf() path requires the scipy netCDF backend (removal made test_to_netcdf_preserves_original_dataset_attributes fail). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…trings in query() README documented a geo_utils.py that no longer exists (now GeotemporalData in geotemporal_data.py) and the codecov badge pointed at the old dClimate-Zarr-Client repo. query()'s forecast_reference_time is widened to str | datetime — xarray .sel accepts ISO strings and client.geo_temporal_query has always passed one (mypy arg-type error). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ge-link and HTTPError pins, forecast typing) Per the final overall grok review: the IPFS catalog lister now prefers dclimate:dataset_id/variant properties like the server lister (its id-only split invented wrong names for hyphenated property-bearing items); adds regression pins for the merge:true next-link collections filter and for requests.HTTPError staying non-connection; drops the dead None entry from the catalog preference order; forecast() accepts ISO strings to match query(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-parse fallbacks, dead code removal) The catalog lister now parses ids unhinted and overlays dclimate:* properties exactly like the server lister — the hint-parse approach dropped items whose ids don't encode their properties (e.g. properties variant 'default' on a bare id); both match paths gain the same fallback. Deletes the ~30-line commented-out org-inference block, fixes circle()'s return annotation/docstring typo, and repositions the rolling_aggregation comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request adds lazy imports, asynchronous STAC and catalog access, structured dataset variant resolution, optimized geospatial selection, revised retrieval behavior, dependency updates, and expanded regression and pytest infrastructure coverage. ChangesRuntime, retrieval, and data processing
STAC resolution
Project and test infrastructure
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…way h2 benchmark script dClimateClient now accepts concurrency, headers, auth, max_retries, initial_delay, backoff_factor and client_factory, forwarded to KuboCAS only when set so py-hamt's defaults stay authoritative — unblocking authenticated gateways, tuned pools, and h2-on/off comparisons. scripts/benchmark_gateway.py implements the py-hamt #58 methodology (median wall time, --http2/--no-http2, --concurrency, --repetitions) and documents the companion infra change: raising nginx keepalive_requests on the gateway to stop routine GOAWAY churn. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ncy list, pooled client) + fallback non-regression pin Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…llib3 stac_server and stac_catalog use pooled sync httpx.Clients (IPFSStacIO keeps a sync client because pystac's StacIO interface is synchronous); fallback guards in dclimate_client catch httpx.HTTPError; the connection-error classifier drops the requests/urllib3 families; the nine transport-pinned test files migrate to an httpx.MockTransport seam with every invariant preserved (request bodies, pagination sequences, single-pooled-client reuse, the event-loop heartbeat bound); requests and urllib3 leave [project] dependencies (still present transitively). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review follow-up for db7444b: All 12 inline review threads have been addressed, replied to, and resolved. I also fixed the substantive top-level pystac issue by binding IPFSStacIO per catalog load instead of mutating pystac process-global state. Additional reproduced fixes included in the same commit:
Validation completed:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dclimate_client_py/stac_catalog.py`:
- Around line 508-515: Update the property_variant branch around
_dataset_and_variant_from_item_id so variant-only IDs that do not encode the
variant still produce a valid parsed dataset/variant result. Mirror the fallback
behavior already used by the resolver, using the item's dclimate:variant value
(including “default”) rather than allowing parsed_variant to remain None and be
omitted by the later listing logic.
In `@dclimate_client_py/stac_server.py`:
- Around line 376-386: Update the pre-scan around the search_features loop to
collect each valid dclimate:dataset_id in a separate per-collection hints map
without requiring collection_id to already exist in accumulators. When creating
accumulators for previously unseen collections in the later flow, seed their
known_datasets from the corresponding hints so legacy hyphenated IDs are parsed
correctly.
- Around line 426-435: Update the item parsing flow around
_dataset_and_variant_from_item_id so that when a variant-aware parse yields no
dataset for a bare item ID, it retries _dataset_and_variant_from_known_datasets
using entry["known_datasets"], matching _feature_matches_dataset behavior.
Preserve the existing property_dataset and variant fallback logic after the
retry.
In `@pyproject.toml`:
- Around line 41-43: Remove the PEP 508 git URL dependency for py_hamt from the
published dependency metadata in pyproject.toml. Replace it with a released
normal version constraint if available; otherwise keep the immutable git pin
only in a non-published development dependency configuration.
In `@tests/conftest.py`:
- Around line 188-194: Update is_ipfs_rpc_running to require a successful HTTP
status from the Kubo /api/v0/id request, rather than accepting any response
below 500. Validate the response using the appropriate successful-status check
and retain the existing RequestException fallback to False.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aab216c8-8bd1-4401-9798-5ed5778cd038
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
dclimate_client_py/dclimate_client.pydclimate_client_py/geotemporal_data.pydclimate_client_py/ipfs_retrieval.pydclimate_client_py/s3_retrieval.pydclimate_client_py/stac_catalog.pydclimate_client_py/stac_server.pypyproject.tomltests/conftest.pytests/ipfs_config.pytests/test_ipfs_retrieval.pytests/test_list_datasets_parity.pytests/test_pytest_infra.pytests/test_review_bugs_client.pytests/test_review_bugs_geotemporal.pytests/test_review_suspects.pytests/test_s3_retrieval.pytests/test_stac_catalog.pytests/test_stac_server_listing.pytests/test_zarr_encryption_ipfs.py
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/test_zarr_encryption_ipfs.py
- tests/test_review_bugs_geotemporal.py
- tests/test_list_datasets_parity.py
- dclimate_client_py/s3_retrieval.py
- dclimate_client_py/ipfs_retrieval.py
- tests/test_pytest_infra.py
- dclimate_client_py/dclimate_client.py
- dclimate_client_py/geotemporal_data.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
dclimate_client_py/stac_catalog.py (2)
129-132: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDefine and close the reusable session.
IPFSStacIOcreates arequests.Sessionper load, but there’s no close path here or on the returned catalog, so repeated loads can leave pooled connections open for the lifetime of each instance. Make the session owned by the caller and close it explicitly, or add anIPFSStacIO.close()lifecycle and invoke it reliably.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dclimate_client_py/stac_catalog.py` around lines 129 - 132, The reusable requests session created by IPFSStacIO must have an explicit lifecycle. Add an IPFSStacIO.close() method that closes self.session, and ensure the catalog-loading flow invokes it reliably after the returned catalog has finished using the IO object, including failure paths.
188-196: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass the gateway into root-CID discovery
get_root_catalog_cid()still hitshttps://ipfs-gateway.dclimate.net/stac, soload_stac_catalog(gateway_url, root_cid=None)can’t bootstrap private/local/offline gateways unless callers always supplyroot_cidexplicitly.dclimate_client_py/stac_catalog.py:170-196🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dclimate_client_py/stac_catalog.py` around lines 188 - 196, Update load_stac_catalog so the root-CID discovery call to get_root_catalog_cid receives the gateway_url argument. Preserve the existing explicit root_cid path, while ensuring gateway-specific discovery is used when root_cid is None.tests/test_stac_catalog.py (1)
11-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate these tests against the endpoint they actually use.
@pytest.mark.ipfsis evaluated againstIPFS_GATEWAY_URL, butget_root_catalog_cid()always callshttps://ipfs-gateway.dclimate.net/stac. This can skip the tests when the configured gateway is down even though the public endpoint works, or run them when only the configured gateway is healthy. Add a separate gate for the public CID endpoint or mock this lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_stac_catalog.py` around lines 11 - 14, Update the tests using get_root_catalog_cid() so their availability gate targets the hard-coded public CID endpoint at https://ipfs-gateway.dclimate.net/stac rather than IPFS_GATEWAY_URL. Add a dedicated gate for that endpoint, or mock the CID lookup to avoid endpoint-dependent gating, and apply it to the affected tests while preserving the existing IPFS gate for tests that use the configured gateway.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@dclimate_client_py/stac_catalog.py`:
- Around line 129-132: The reusable requests session created by IPFSStacIO must
have an explicit lifecycle. Add an IPFSStacIO.close() method that closes
self.session, and ensure the catalog-loading flow invokes it reliably after the
returned catalog has finished using the IO object, including failure paths.
- Around line 188-196: Update load_stac_catalog so the root-CID discovery call
to get_root_catalog_cid receives the gateway_url argument. Preserve the existing
explicit root_cid path, while ensuring gateway-specific discovery is used when
root_cid is None.
In `@tests/test_stac_catalog.py`:
- Around line 11-14: Update the tests using get_root_catalog_cid() so their
availability gate targets the hard-coded public CID endpoint at
https://ipfs-gateway.dclimate.net/stac rather than IPFS_GATEWAY_URL. Add a
dedicated gate for that endpoint, or mock the CID lookup to avoid
endpoint-dependent gating, and apply it to the affected tests while preserving
the existing IPFS gate for tests that use the configured gateway.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 455463ee-f5b0-43c8-a17a-4d8ecd8a1b08
📒 Files selected for processing (2)
dclimate_client_py/stac_catalog.pytests/test_stac_catalog.py
![da-code-reviewer[bot]](https://avatars.githubusercontent.com/u/78453614?s=60&v=4){kind=small}
|
Addressed the three outside-diff findings in 572f4aa:
Validation on this commit: 191 passed / 45 skipped in the fast suite; 26/26 live catalog tests passed; fresh built wheel passed the same 191-test suite; pre-commit, ruff, format, sdist/wheel build, and publishable METADATA checks are clean. |
|
Final review-pong status for
There are no unresolved review threads. The remaining |
…ttpx/ResolvedDataset line Reconciliation decisions: - upstream's requests-era additions translated to httpx: headers-aware _search_pages (pooled module client), get_root_catalog_cid(catalog_url), conftest RPC/pointer probes - upstream's known_datasets longest-match parsing and collect-all-pages resolve adopted wholesale, fused with ResolvedDataset returns and _effective_variant selection; the 'default on page 1 stops paging' optimization is superseded by upstream's authoritative full walk (call-count pin updated 1 -> 2 accordingly) - upstream's per-catalog stac_io binding + weakref close lifecycle adopted; IPFSStacIO now owns a per-instance httpx.Client (closable) while one-shot pointer reads keep the module-pooled client - upstream's urllib3 MaxRetryError refinements dropped as moot on the httpx stack (HTTPStatusError already classifies non-connection) - upstream's new tests ported from requests monkeypatching to the httpx MockTransport seams with all assertions preserved Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- IPFSStacIO.read_text implements the http(s) fallback for real (the old super().read_text was abstract and raised NotImplementedError behind a type-ignore); unsupported schemes raise a clear ValueError - query() kwargs validation is value-aware (required keys must be non-None) and consistent: empty polygon/multiple_points kwargs now raise like the other selectors - dClimateClient: eager ValueError for client_factory combined with headers/auth (KuboCAS rejected it only at context entry); gateway-None split semantics documented and the fallback ternary hoisted; auth forwarding pinned in tests - mypy meta-test runs the project-pinned mypy and asserts the checked file count; py.typed presence pinned inside a built wheel - benchmark: one-line failures to stderr, follow_redirects=True factory, JSON-only stdout - probe redirect parity, managed mock clients in test helpers, stale requests.HTTPError reference in DATASET_CATALOG_USAGE.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_stac_catalog.py`:
- Around line 139-140: Add the pytest.mark.ipfs decorator alongside
pytest.mark.stac_pointer on TestLoadStacCatalog, TestResolveDatasetCidFromStac,
TestListAvailableDatasets, and TestIntegrationEndToEnd in
tests/test_stac_catalog.py, so all catalog-loading tests require both services.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21b534e7-f868-4a76-b6a8-e513e42ce662
📒 Files selected for processing (14)
dclimate_client_py/geotemporal_data.pydclimate_client_py/ipfs_retrieval.pydclimate_client_py/stac_catalog.pydclimate_client_py/stac_server.pypyproject.tomltests/conftest.pytests/ipfs_config.pytests/test_ipfs_retrieval.pytests/test_list_datasets_parity.pytests/test_pytest_infra.pytests/test_review_perf_misc.pytests/test_review_suspects.pytests/test_stac_catalog.pytests/test_stac_server_listing.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/ipfs_config.py
- tests/test_review_perf_misc.py
- tests/test_list_datasets_parity.py
- pyproject.toml
- dclimate_client_py/ipfs_retrieval.py
- dclimate_client_py/stac_server.py
- dclimate_client_py/geotemporal_data.py
| @pytest.mark.stac_pointer | ||
| class TestLoadStacCatalog: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Gate catalog-loading tests on the IPFS gateway too.
stac_pointer only covers the root-CID endpoint; these classes also load catalog content through IPFS_GATEWAY_URL. Without @pytest.mark.ipfs, they run and fail when the pointer is reachable but the gateway is not.
tests/test_stac_catalog.py#L139-L140: add@pytest.mark.ipfstoTestLoadStacCatalog.tests/test_stac_catalog.py#L202-L203: add@pytest.mark.ipfstoTestResolveDatasetCidFromStac.tests/test_stac_catalog.py#L353-L354: add@pytest.mark.ipfstoTestListAvailableDatasets.tests/test_stac_catalog.py#L444-L445: add@pytest.mark.ipfstoTestIntegrationEndToEnd.
📍 Affects 1 file
tests/test_stac_catalog.py#L139-L140(this comment)tests/test_stac_catalog.py#L202-L203tests/test_stac_catalog.py#L353-L354tests/test_stac_catalog.py#L444-L445
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_stac_catalog.py` around lines 139 - 140, Add the pytest.mark.ipfs
decorator alongside pytest.mark.stac_pointer on TestLoadStacCatalog,
TestResolveDatasetCidFromStac, TestListAvailableDatasets, and
TestIntegrationEndToEnd in tests/test_stac_catalog.py, so all catalog-loading
tests require both services.
| dumps(request_headers, sort_keys=True, default=str), | ||
| ) | ||
| if page_key in seen: | ||
| return |
There was a problem hiding this comment.
MEDIUM
A repeated next request silently ends pagination as if the search completed. This can make listing return a partial catalog or make resolution select a first-page non-default variant, preventing the catalog fallback. Raise ValueError here, as the page-limit branch does, so callers detect truncation.
| [tool.uv.sources] | ||
| # Development integration pin for py-hamt PR #88. Published artifacts retain | ||
| # the released runtime floor because PyPI rejects direct-URL requirements. | ||
| py_hamt = { git = "https://github.com/dClimate/py-hamt", rev = "942580df089be13d6b8b803fc942932d65f2fc7f" } |
There was a problem hiding this comment.
MEDIUM
[tool.uv.sources] is ignored by wheels, sdists, pip, and non-uv installers. Since this PR explicitly depends on the pinned py-hamt revision, published installs will still resolve py_hamt>=3.4.1 and miss those required fixes. Wait for a release containing the revision and raise the runtime floor before publishing/merging.
Follow-ups: variant provenance, mypy 0 + py.typed, KuboCAS knobs, httpx consolidation (+ upstream review-commit merge)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
==========================================
+ Coverage 65.42% 74.79% +9.37%
==========================================
Files 8 16 +8
Lines 778 1956 +1178
Branches 111 363 +252
==========================================
+ Hits 509 1463 +954
- Misses 221 344 +123
- Partials 48 149 +101 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dclimate_client_py/stac_server.py (2)
453-506: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winParsed dataset names aren't fed back into
known_datasetsduring listing.Unlike
stac_catalog.py'slist_available_datasets(which doesknown_datasets.add(item_dataset)after each item so later items in the same collection benefit from siblings already resolved), this loop never updatesentry["known_datasets"]after computingdataset_name(Lines 474-506). For a collection with no/collectionsmetadata and no items carrying explicitdclimate:dataset_id/dclimate:variantproperties,known_datasetsstays empty for the whole pass, so every legacy hyphenated item id falls back to naive first-hyphen splitting (misparsing e.g.precip-daily-finalas datasetprecip) instead of self-reinforcing from siblings resolved earlier in the same loop.♻️ Proposed fix
dataset_variants = entry["datasets"].setdefault(dataset_name, {}) dataset_variants[variant_name] = variant_entry + entry["known_datasets"].add(dataset_name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dclimate_client_py/stac_server.py` around lines 453 - 506, After computing a valid dataset_name in the item-processing loop, add it to entry["known_datasets"] before resolving subsequent items. Update the logic surrounding dataset_name and dataset_variants in the listing flow so sibling items can reuse datasets discovered earlier, while preserving the existing skip behavior when dataset_name is missing.
169-238: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLoop-detected pagination silently truncates instead of raising.
At Line 176-177,
if page_key in seen: returnends pagination silently when anextlink repeats an already-seen request. This is the exact failure mode the page-limit branch below (231-238) was fixed to reject — a still-open past review comment ("A repeatednextrequest silently ends pagination as if the search completed... RaiseValueErrorhere, as the page-limit branch does") appears unaddressed in the current code (no confirmation reply, unlike every sibling fix in this file).Silent truncation here means
resolve_cid_from_stac_server/list_available_datasets_from_stac_servercan return an incomplete result (wrong variant selected, or a partial catalog) without triggering the caller's IPFS-catalog fallback, since no exception propagates.🐛 Proposed fix
if page_key in seen: - return + raise ValueError( + "STAC search next link repeated an already-seen request; " + "pagination cannot make progress" + ) seen.add(page_key)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dclimate_client_py/stac_server.py` around lines 169 - 238, Update the repeated-request branch in the pagination loop around the seen set to raise ValueError instead of returning when page_key is already present. Keep the existing page-limit failure behavior and provide a descriptive message indicating that STAC pagination detected a repeated next request, so callers receive the incomplete-result error rather than silent truncation.
🧹 Nitpick comments (1)
tests/test_review_fu_httpx.py (1)
75-144: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer mocking the STAC transport over a real socket connection attempt.
stac_server_url="http://127.0.0.1:9"depends on the sandbox/CI network stack immediately refusing the connection; a mocked transport (as used intest_review_perf_async.py'sinstall_httpx_mock) would make this deterministic and avoid depending on OS-level connection-refused semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_review_fu_httpx.py` around lines 75 - 144, Update test_load_dataset_falls_back_when_stac_transport_is_unreachable to mock the STAC HTTP transport using the existing install_httpx_mock pattern from test_review_perf_async.py instead of relying on stac_server_url port 9. Configure the mock to produce the transport failure needed to exercise catalog fallback, while preserving the existing assertions and dataset-loading setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/conftest.py`:
- Around line 194-197: Update the gateway availability predicate around the
httpx.head call in the IPFS collection gating logic to accept only successful
responses or the expected missing-CID 404; reject 401, 403, 429, and all other
statuses so inaccessible gateways cause the tests to be skipped.
---
Outside diff comments:
In `@dclimate_client_py/stac_server.py`:
- Around line 453-506: After computing a valid dataset_name in the
item-processing loop, add it to entry["known_datasets"] before resolving
subsequent items. Update the logic surrounding dataset_name and dataset_variants
in the listing flow so sibling items can reuse datasets discovered earlier,
while preserving the existing skip behavior when dataset_name is missing.
- Around line 169-238: Update the repeated-request branch in the pagination loop
around the seen set to raise ValueError instead of returning when page_key is
already present. Keep the existing page-limit failure behavior and provide a
descriptive message indicating that STAC pagination detected a repeated next
request, so callers receive the incomplete-result error rather than silent
truncation.
---
Nitpick comments:
In `@tests/test_review_fu_httpx.py`:
- Around line 75-144: Update
test_load_dataset_falls_back_when_stac_transport_is_unreachable to mock the STAC
HTTP transport using the existing install_httpx_mock pattern from
test_review_perf_async.py instead of relying on stac_server_url port 9.
Configure the mock to produce the transport failure needed to exercise catalog
fallback, while preserving the existing assertions and dataset-loading setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4eed9706-69ca-4dc7-9847-a9dfb6cc8755
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
DATASET_CATALOG_USAGE.mddclimate_client_py/__init__.pydclimate_client_py/client.pydclimate_client_py/dclimate_client.pydclimate_client_py/encryption_codec.pydclimate_client_py/geotemporal_data.pydclimate_client_py/ipfs_retrieval.pydclimate_client_py/py.typeddclimate_client_py/siren/siren_client.pydclimate_client_py/stac_catalog.pydclimate_client_py/stac_server.pypyproject.tomlscripts/benchmark_gateway.pytests/conftest.pytests/test_ipfs_retrieval.pytests/test_list_datasets_parity.pytests/test_pytest_infra.pytests/test_review_bugs_stac_server.pytests/test_review_fu_gateway_none.pytests/test_review_fu_httpx.pytests/test_review_fu_kubo_knobs.pytests/test_review_fu_kwargs_validation.pytests/test_review_fu_typing.pytests/test_review_fu_variant.pytests/test_review_perf_async.pytests/test_review_suspects.pytests/test_stac_catalog.pytests/test_stac_server.pytests/test_stac_server_listing.py
🚧 Files skipped from review as they are similar to previous changes (7)
- dclimate_client_py/init.py
- tests/test_pytest_infra.py
- tests/test_ipfs_retrieval.py
- pyproject.toml
- tests/test_review_suspects.py
- tests/test_stac_catalog.py
- dclimate_client_py/geotemporal_data.py
| response = httpx.head( | ||
| f"{gateway_url.rstrip('/')}/ipfs/{known_cid}", | ||
| timeout=5, | ||
| follow_redirects=True, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not treat all sub-500 responses as a usable gateway.
The current predicate accepts 401, 403, 429, and other client-error responses, so collection gating can run IPFS tests against an inaccessible or rate-limited gateway instead of skipping them. Match the documented behavior by accepting successful responses or the expected missing-CID 404.
Proposed fix
- if response.status_code < 500:
+ if response.is_success or response.status_code == 404:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| response = httpx.head( | |
| f"{gateway_url.rstrip('/')}/ipfs/{known_cid}", | |
| timeout=5, | |
| follow_redirects=True, | |
| if response.is_success or response.status_code == 404: |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 193-197: Request-controlled URL passed to httpx; validate against an allowlist to prevent SSRF.
Context: httpx.head(
f"{gateway_url.rstrip('/')}/ipfs/{known_cid}",
timeout=5,
follow_redirects=True,
)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(avoid-ssrf)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/conftest.py` around lines 194 - 197, Update the gateway availability
predicate around the httpx.head call in the IPFS collection gating logic to
accept only successful responses or the expected missing-CID 404; reject 401,
403, 429, and all other statuses so inaccessible gateways cause the tests to be
skipped.
There was a problem hiding this comment.
Codex Automated Review
Found five actionable issues: one credential-exposure risk, two STAC correctness bugs, and two deterministic test failures.
Posted 3 inline comment(s).
Findings that could not be placed inline:
dclimate_client_py/stac_catalog.py:215[high] The client contains gatewayheaders/auth, but it is also used for arbitrary HTTP(S) links supplied by the catalog. A malicious catalog can point a child link at another origin and receive the gateway credentials. Restrict authenticated requests to the gateway origin or use an unauthenticated client for external links.dclimate_client_py/stac_server.py:506[medium] After inferringdataset_name, it is not added toentry["known_datasets"]. If a variant-only item establishes a hyphenated dataset such asprecip-daily, a following property-less sibling is reparsed as datasetprecip. Add each resolved dataset name to the hint set before processing subsequent items.
| dumps(request_headers, sort_keys=True, default=str), | ||
| ) | ||
| if page_key in seen: | ||
| return |
There was a problem hiding this comment.
MEDIUM
A repeated next request silently terminates pagination and makes the accumulated features appear complete. Listing can therefore return partial results, and resolution can select the wrong fallback variant instead of invoking the catalog fallback. Raise ValueError here as the page-limit branch does.
| } | ||
| ) | ||
|
|
||
| monkeypatch.setattr(stac_server.requests, "post", post) |
There was a problem hiding this comment.
MEDIUM
stac_server no longer defines requests, so this test deterministically raises AttributeError before exercising pagination. Patch the new pooled _client() transport (or use the existing HTTPX helper) instead.
| organization="org", | ||
| ) | ||
|
|
||
| assert cid == "bafy-catalog-precip-daily-final" |
There was a problem hiding this comment.
MEDIUM
The resolver now returns ResolvedDataset, so comparing it directly with a CID string always fails. Assert cid.cid or compare against the complete ResolvedDataset; as written, this leaves the test suite red.
| self._stac_catalog = await asyncio.to_thread( | ||
| load_stac_catalog, | ||
| gateway_url=self._catalog_gateway_base_url, | ||
| headers=self._headers, |
There was a problem hiding this comment.
HIGH
Gateway credentials are forwarded to load_stac_catalog() without a catalog_url, so a client configured with a custom authenticated gateway sends its headers/basic-auth to the hard-coded public dClimate pointer whenever STAC-server fallback occurs. Derive or expose the custom gateway's pointer URL, or only forward credentials when the pointer origin matches the gateway.
| return super().read_text(source, *args, **kwargs) | ||
| scheme = urlsplit(source_text).scheme.lower() | ||
| if scheme in {"http", "https"}: | ||
| response = self.client.get(source_text) |
There was a problem hiding this comment.
HIGH
This client was constructed with the gateway's headers/basic-auth, but it is also used for arbitrary HTTP(S) links supplied by the remote catalog. A cross-origin catalog link therefore receives the user's gateway credentials. Restrict authenticated requests to the gateway origin and use an unauthenticated client for external links.
| dumps(request_headers, sort_keys=True, default=str), | ||
| ) | ||
| if page_key in seen: | ||
| return |
There was a problem hiding this comment.
MEDIUM
A repeated next request silently ends pagination and treats the accumulated pages as complete. This can return an incomplete listing or select a lower-priority first-page variant without triggering catalog fallback. Raise ValueError here, as the page-limit branch does.
4 participants
This PR is the output of a full library audit followed by a strict TDD pipeline (the same process as dClimate/py-hamt#88). Every item below was implemented as a pair of commits — a red commit with a failing test that reproduces the defect, then a green commit with the fix — so reviewers can check out any red commit and watch the test fail. Every fix was then reviewed by two independent reviewers (Codex implemented under a verifying harness; Grok and an adversarial Claude reviewer audited each diff, with blocking findings driving remediation commits).
Results: 166 tests pass locally (up from 14 on
main— the old autouse IPFS gate silently skipped 144 tests including ~85 fully-offline unit tests), ruff lint + format clean. Two HIGH bugs that made shipped features completely unusable (polygons(),rolling_aggregation()on any grid with a masked cell) are fixed, all three previously-unproven suspicions from the audit were confirmed real and fixed, and the client now tracks py-hamt PR #88 (review-fixes-integration) — validated against the live gateway: the network suite passes on that branch with no upstream issues found.Benchmarks (same machine, measured by the red tests / adversarial reviewers)
mainrectangle()tiny selection on 41 MB dataset (peak alloc).selview)circle()small radius (peak alloc)concatenate_datasets()40 datasets × 200k float64load_dataset()asyncio.to_thread)points()coordinate extractionGeoSeries.y/.xEncryptionCodec16 KB chunkimport dclimate_client_py1. Geotemporal core (
geotemporal_data.py)polygons()was broken three independent ways:rioxarraywas never imported soself.data.rioraisedAttributeErrorin a fresh interpreter;except errors.NoDataInBoundsreferenced an exception class that doesn't exist; and the fallback calledreduce_polygon_to_pointon the xarrayDatasetinstead of the wrapper. Every path crashed. Now: function-levelrioxarrayimport (keeps the dep optional), catchesrioxarray.exceptions.NoDataInBounds, correct receiver — and the inplace rio mutations of the caller's dataset are gone (ed0b248 addsinplace=False+ a non-mutation regression pin after review caught rioxarray's surprisinginplace=Truedefault).rolling_aggregation()used.dropna("time")(how="any"), so a single masked ocean/coast cell — ubiquitous in climate grids — emptied the entire time dimension. Now trims only the incomplete leading windows (isel(time=slice(window_size-1, None))); the adversarial reviewer verified the new semantics match the docstring and areidentical()to the old output on dense data.check_dataset_size()crashed withTypeErroron scalar/zero-dim datasets (reducewith no initializer). Initializer1; a scalar counts as one point (pinned both directions).to_netcdf()permanently deleteddate range,bbox, etc. from the caller's dataset attrs — attrs thatas_dict()and s3 retrieval later read. Serializes from a copy; byte-identical output verified.temporal_aggregation("quarter")used the pandas alias'Q'— deprecated on installed pandas 2.2.3, removed in pandas 3. Now'QE'; resample output verified identical, warning-free.tolerance=10e-5(= 1e-4 ≈ 11 m) inpoint()/points()snap_to_grid=Falsesilently snapped points documented to raiseNoDataFoundError. Now1e-5; the adversarial reviewer's float-representation attack (worst-case float32 half-ulp 7.6e-6) confirmed the tighter bound is safe for real grids.rectangle()'s.where(mask, drop=True)deep-copied the full dataset viaxr.alignbefore cropping — 89% of a profiled end-to-end query. Replaced with.selcoordinate slices (ascending and descending coords);circle()pre-crops to a haversine bounding box (pole crossings widen to the full longitude band, antimeridian spans fall back to the old path). Output pinned identical by 6 semantics tests; 97f832d adds a mask-path fallback for exotic layouts (non-monotonic/NaN/non-index coords) after the adversarial reviewer's 400-case differential fuzzer + mutation testing found silent row-drops there and two missing pins (descending-longitude, high-latitude cos-widening).points()iterated shapely points in Python ([p.y for p in mask]). VectorizedGeoSeries.y/.x; 8e75c68 adds a guard rejecting missing (None) geometries, which the vectorized path would otherwise silently snap to an arbitrary grid cell.2. STAC resolution, catalog and listing (
stac_server.py,stac_catalog.py)KeyErroron features lackingproperties. Variant now derives from the item id when properties are absent; all property access isNone-safe.chirps-precip-daily-final-p05as datasetprecip. Both resolvers and both listers now share one dataset-aware, hyphen-aware grammar (_dataset_and_variant_from_item_id)./searchused a fixedlimit: 100and ignoredrel=next— datasets beyond the first page were unresolvable. Pagination now follows next links (POST/GET, bounded 50 pages, repeat-page guard); da83c25 also honors the STACmerge: truebody contract after the adversarial reviewer demonstrated page-2 requests dropping thecollectionsfilter could resolve a poisoned foreign-collection CID."default", butresolve(variant="default")failed on them — breaking list→resolve round-trips, with server and catalog disagreeing on the documented fallback path. Bare items now count as"default"uniformly across both resolvers and both listers.dclimate:dataset_id/dclimate:variantproperties (inventing wrong names for hyphenated property-bearing items) — and the first fix's hint-parsing dropped items whose ids don't encode their properties (caught by the final adversarial gate). Both listers now parse unhinted and overlay explicit properties; both match paths gained the same fallback.IPFSStacIOissued a bare sessionlessrequests.getperipfs://read — a fresh TCP+TLS handshake each time, with no timeout. One pooledrequests.Sessionwith a 30 s timeout (8040d54 extends the timeout toget_root_catalog_cid, the first call of every catalog load).3. Client, async and I/O (
dclimate_client.py,concatenate.py,ipfs_retrieval.py,s3_retrieval.py,encryption_codec.py)__aexit__leaked the KuboCAS session if Sirenaclose()raised. Now try/finally with full dual-failure semantics per review: AsyncExitStack convention (later error propagates with the earlier as__context__),CancelledErrorfrom either cleanup outranks ordinary errors,_kubo_cascleared on every path — the whole exception matrix is pinned by tests.requestscalls ran directly on the event loop inside asyncload_dataset(), freezing py-hamt's concurrent chunk fetches (~204–256 ms per STAC call). All blocking resolution now runs viaasyncio.to_thread, with anasyncio.Lock-guarded catalog lazy-init (double-checked locking verified race-free with a widened race window); 8040d54 addsalist_datasets()since the synclist_datasets()had the same disease.concatenate_datasets()did iterativexr.concat— O(n²) time, ~2× peak memory. Slices are collected and concatenated once; the adversarial reviewer's 13-case differential harness (including a dataset overlapping its grand-predecessor) found zero output differences.str(exc)— unrelated Zarr metadata text containing "timeout" was treated as a gateway failure and skipped the HAMT fallback. Now classified by exception type. The first typed version used blanketOSError, which grok proved was a new BLOCKER (FileNotFoundError,requests.HTTPError— anIOErrorsubclass — also skipped the fallback); the final classifier matches only connection/timeout/DNS families + requests/urllib3/httpx-transport/aiohttp types, pinned positive and negative.get_dataset_from_s3()crashed withAttributeErrorwhen the optionalupdate_in_progressattr was absent (the same attr is guarded everywhere else). All optional update flags guarded; all three update-flag branches now covered.EncryptionCodechopped throughasyncio.to_thread(~60 µs) for every chunk — +79% overhead at 16 KB. Crypto runs inline under 128 KiB, still offloads above; roundtrips, exception parity and the dispatch boundary itself are pinned.__getattr__+ function-level imports; all 29 public names, pickling,isinstancestability and star-imports verified;__dir__added per PEP 562.4. Test infrastructure (the biggest process risk)
127.0.0.1:8080— including ~85 fully-offline mocked unit tests. Only 14 tests actually ran locally. The gate is now marker-based (ipfsfor gateway/daemon tests, existingintegrationgating kept); the adversarial reviewer verified marker triage empirically with a socket-blocking harness (109 unit tests pass fully offline, zero over- or under-marking).PytestUnhandledCoroutineWarning; a hard error on pytest 9).asyncio_mode = "auto"— and after both reviewers independently caught the meta-test being tautological, it now generates an unmarkedasync deftest to prove the config is load-bearing.pytestcollected the stray root-leveltest_stac_integration.pyandexamples/;tests/test_debug.pycould not fail (every check wrapped in try/except, zero assertions).testpaths = ["tests"], script relocated toscripts/,test_debug.pydeleted.5. Build, deps, docs
py_hamtpinned to PR dClimate/py-hamt#88 (review-fixes-integration) via[tool.uv.sources]— drop this pin once #88 merges/releases. Validated against the live gateway: 24 network tests pass on that branch; the single failure needs a local Kubo daemon (RPC upload), not a py-hamt defect.aiohttp/urllib3/multiformats(direct imports) declared;pyarrowremoved,python-dotenvmoved to anexamplesextra. The audit's "scipy unused" claim was wrong — xarray's in-memoryto_netcdf()needs the scipy netCDF backend (removal made a test fail); kept with a comment.geo_utils.pythat no longer exists (→geotemporal_data.py); codecov badge pointed at the old dClimate-Zarr-Client repo.query()/forecast()accept ISO strings (mypy arg-type error at the only call site).Compatibility notes
rectangle()on regular grids now returns a zero-copy view of the parent dataset (documented), and integer data variables keep their dtype instead of being upcast to float64 by.where— both intentional; values are pinned identical.rolling_aggregation()on grids with NaN cells now returns NaN-holding rows instead of a silently truncated (typically empty) time axis — this is the documented behavior; dense-data output is unchanged.IpfsConnectionError(the old behavior matched only when the status text happened to contain "timeout").How this was verified
__aexit__exception matrix; live probing ofapi.stac.dclimate.netfor the pagination contract). Reviewer findings drove 10 remediation commits, including one BLOCKER (over-broadOSErrorclassification) and one FIX-FIRST (server/catalogdefault-variant divergence).pytest166 passed / 47 skipped (network-marked; the ipfs-marked set passes against the live gateway except the one test requiring a local Kubo daemon),ruff checkandruff format --checkclean.Depends on dClimate/py-hamt#88 (tracked via the uv source pin).
🤖 Generated with Claude Code
Summary by CodeRabbit
alist_datasets()for async dataset discovery without blocking.