Skip to content

Enhance dataset versioning and align Zarr resolution with STAC - #9

Open
eloramirez1356 wants to merge 5 commits into
mainfrom
dns-migration
Open

Enhance dataset versioning and align Zarr resolution with STAC#9
eloramirez1356 wants to merge 5 commits into
mainfrom
dns-migration

Conversation

@eloramirez1356

@eloramirez1356 eloramirez1356 commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

This pull request adds robust support for dataset version history and related metadata to the dClimate client. It introduces new methods to resolve and expose dataset version information, provenance, and citation APIs, as well as improved handling of Zarr group resolution and metadata propagation. The changes also enhance the STAC integration, allowing the client to dynamically discover and utilize versioned dataset services.

Dataset version history and metadata support:

  • Added a new listDatasetVersions method to DClimateClient, which resolves dataset details and fetches version history from the advertised dclimate:versions_api URL in STAC metadata. This allows clients to retrieve version listings, with optional filters, without hard-coding dataset routing. [1] [2]
  • Extended dataset metadata propagation throughout the codebase to include versioning fields such as versionsApi, provenanceApi, citationApi, streamId, commitId, versionLabel, isCitable, and retentionClass, making these available to client applications. [1] [2] [3] [4] [5]

STAC and Zarr group improvements:

  • Enhanced STAC catalog and server resolution to extract and propagate Zarr group information (zarrGroup) from both assets and properties, and improved logic for resolving dataset details from STAC. [1] [2] [3] [4] [5]
  • Improved Zarr group handling in dataset opening logic, including fallback to a default group ("0") if an explicit group is required but not provided, and tracking the used group in dataset attributes. [1] [2] [3] [4] [5] [6] [7]

API and error handling enhancements:

  • Exported new versioning-related types and helper methods (e.g., listVersionsFromUrl, getExactVersionFromUrl, getCitationFromUrl) from the package entry point for use in advanced applications. [1] [2]
  • Introduced new error types for version history and API errors, improving error granularity and developer experience.

These changes make the client more flexible, future-proof, and capable of supporting advanced dataset versioning and provenance workflows.

Summary by CodeRabbit

  • New Features
    • Added dataset version-history discovery, filtering, exact-version lookup, and citation retrieval.
    • Added release, provenance, version, retention, and citability metadata to dataset information.
    • Added explicit multiresolution dataset selection by resolution or Zarr group.
    • Improved Zarr asset and group detection, including grouped dataset fallbacks and direct-CID support.
  • Bug Fixes
    • Added clear errors for unavailable version history, invalid or conflicting resolution selections, and version service failures.
  • Documentation
    • Documented version history, citations, multiresolution datasets, selection rules, and service limitations.

@eloramirez1356 eloramirez1356 self-assigned this Aug 5, 2026
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fb255ec5-d6b7-4eee-94cc-b3fc3816eabd

📥 Commits

Reviewing files that changed from the base of the PR and between 7ccfcfa and afaff48.

📒 Files selected for processing (5)
  • src/client.ts
  • src/stac/stac-catalog.ts
  • src/types.ts
  • tests/review-fixes/autoconcat-variant-crash.test.ts
  • tests/stac-version-discovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • tests/stac-version-discovery.test.ts
  • src/client.ts
  • src/stac/stac-catalog.ts

📝 Walkthrough

Walkthrough

The client discovers dataset version services from STAC metadata, exposes version listing and exact-version APIs, propagates release metadata, and resolves Zarr groups for direct and concatenated dataset loads.

Changes

Dataset version discovery

Layer / File(s) Summary
Version API contracts and client
src/versions/*, src/errors.ts, src/types.ts, src/index.ts, README.md, package.json, tests/version-client.test.ts
Adds version types, URL-based helpers, filtering, citation retrieval, API errors, public exports, documentation, and package version 0.7.0.
STAC release and Zarr metadata resolution
src/stac/*, tests/stac-version-discovery.test.ts
Extracts release metadata and version-service URLs from STAC items. Resolves CIDs and asset or item Zarr groups for hosted and IPFS-backed STAC data.
Client loading and group selection
src/client.ts, src/ipfs/open-dataset.ts, tests/fetch-dataset-cid.test.ts, tests/open-dataset.test.ts, tests/review-fixes/*
Shares dataset resolution across loading and version requests. Propagates metadata, validates resolution and group selections, and applies normalized groups to direct and concatenated variants.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested reviewers: faolain

Sequence Diagram(s)

sequenceDiagram
  participant DClimateClient
  participant STACResolution
  participant VersionAPI
  DClimateClient->>STACResolution: resolve dataset metadata
  STACResolution-->>DClimateClient: return version API URL and release metadata
  DClimateClient->>VersionAPI: request filtered versions
  VersionAPI-->>DClimateClient: return DatasetVersionListing
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the pull request's main changes: dataset versioning and STAC-aligned Zarr resolution handling.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dns-migration

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 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 `@src/client.ts`:
- Around line 206-208: Update the openedZarrGroup initialization near
explicitZarrGroup to validate dataset.attrs?._ipfs_zarr_group is a runtime
string before passing it to normalizeZarrGroup; otherwise treat it as absent and
preserve the explicitZarrGroup precedence.
- Line 165: Update the request flow around listVersionsFromUrl to validate
resolved.versionsApi against the configured trusted HTTPS origins before
fetching, reject non-HTTPS or untrusted destinations, and enforce the same
validation for every redirect rather than only the initial URL.

In `@src/versions/version-client.ts`:
- Around line 15-17: Update the version API fetch flow around fetchImpl to
require HTTPS for the initial URL and every redirect target, rejecting non-HTTPS
URLs by default. Permit HTTP only when the explicit local-development opt-in is
enabled, and validate each redirect before following it.
🪄 Autofix

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: cd17c9ea-5aaf-4435-a691-1af79873a297

📥 Commits

Reviewing files that changed from the base of the PR and between 2a35fe7 and c1c3611.

📒 Files selected for processing (16)
  • README.md
  • src/client.ts
  • src/errors.ts
  • src/index.ts
  • src/ipfs/open-dataset.ts
  • src/stac/index.ts
  • src/stac/stac-catalog.ts
  • src/stac/stac-server.ts
  • src/types.ts
  • src/versions/index.ts
  • src/versions/types.ts
  • src/versions/version-client.ts
  • tests/fetch-dataset-cid.test.ts
  • tests/open-dataset.test.ts
  • tests/stac-version-discovery.test.ts
  • tests/version-client.test.ts

Comment thread src/client.ts
`Version history is not available for ${collection}/${dataset}/${resolved.variant}.`
);
}
return listVersionsFromUrl(resolved.versionsApi, filters);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- relevant files ---'
git ls-files | rg '(^|/)(client\.ts|version-client\.ts|utils\.ts|stac-(catalog|server)\.ts|types\.ts)$'
printf '%s\n' '--- client version path ---'
rg -n -A35 -B15 'listDatasetVersions|versionsApi|getStacReleaseMetadata|listVersionsFromUrl' src/client.ts src/stac src/versions
printf '%s\n' '--- request implementation and redirect options ---'
rg -n -A35 -B15 'function requestJson|const requestJson|requestJson|redirect|FetchImplementation|fetchImpl' src
printf '%s\n' '--- URL policy, trusted origins, and version endpoint configuration ---'
rg -n -i -A8 -B8 'allowlist|allow-list|trusted.?origin|trusted.?host|versions.?api|versionsApi|https?://|gatewayUrl|serverUrl' src tests

Repository: dClimate/dclimate-client-js

Length of output: 50384


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- resolveDatasetDetails and catalog acquisition ---'
sed -n '80,145p' src/client.ts
sed -n '1,90p' src/client.ts
rg -n -A25 -B10 'getStacCatalog|loadStacCatalog|resolveDatasetDetails|stacServerUrl|DEFAULT_STAC_SERVER_URL' src/client.ts src/stac
printf '%s\n' '--- package runtime targets and documented usage ---'
cat package.json
rg -n -i -A8 -B8 'node|browser|deno|runtime|server|client-side|stacServerUrl|listDatasetVersions' README.md docs src tests 2>/dev/null | head -300

Repository: dClimate/dclimate-client-js

Length of output: 50385


🏁 Script executed:

#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path

client = Path("src/client.ts").read_text()
versions = Path("src/versions/version-client.ts").read_text()
catalog = Path("src/stac/stac-catalog.ts").read_text()
package_json = Path("package.json").read_text()

checks = {
    "client forwards resolved versionsApi": "listVersionsFromUrl(resolved.versionsApi, filters)" in client,
    "metadata reads dclimate versions property": '"dclimate:versions_api"' in catalog,
    "URL is constructed from supplied value": "const url = new URL(versionsUrl)" in versions,
    "URL reaches fetch through requestJson": "requestJson<DatasetVersionListing>(url.toString(), fetchImpl)" in versions,
    "request uses fetch implementation": "const response = await fetchImpl(url, {" in versions,
    "no redirect option in request helper": "redirect:" not in versions,
    "node build exists": '"main": "./dist/node/index.js"' in package_json,
    "browser build exists": '"module": "./dist/browser/index.js"' in package_json,
}
for name, passed in checks.items():
    print(f"{'PASS' if passed else 'FAIL'}: {name}")
if not all(checks.values()):
    raise SystemExit(1)
PY

Repository: dClimate/dclimate-client-js

Length of output: 479


SSRF (CWE-918): Server-Side Request Forgery (SSRF)

Exploitability: Moderate

Reachability path
● Entry
  src/stac/stac-server.ts:49
  ResolvedCidFromServer
│
▼
● Hop
  src/stac/stac-catalog.ts:345
  response
│
▼
● Hop
  tests/stac-version-discovery.test.ts
│
▼
● Sink
  src/client.ts

Restrict versionsApi requests to trusted HTTPS origins.

resolved.versionsApi comes from dclimate:versions_api and reaches the Node fetch implementation without destination validation. An untrusted STAC item can make a Node caller request an internal address. Enforce configured HTTPS origins and apply the same policy after redirects.

🤖 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 `@src/client.ts` at line 165, Update the request flow around
listVersionsFromUrl to validate resolved.versionsApi against the configured
trusted HTTPS origins before fetching, reject non-HTTPS or untrusted
destinations, and enforce the same validation for every redirect rather than
only the initial URL.

Comment thread src/client.ts
Comment on lines +206 to +208
const openedZarrGroup =
explicitZarrGroup ??
normalizeZarrGroup(dataset.attrs?._ipfs_zarr_group as string | undefined);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- client.ts context ---'
sed -n '170,225p' src/client.ts
printf '%s\n' '--- normalizeZarrGroup definitions and usages ---'
rg -n -C 5 'normalizeZarrGroup|_ipfs_zarr_group|explicitZarrGroup|openedZarrGroup' .
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(client|zarr|dataset|test|spec)|package.json|tsconfig'

Repository: dClimate/dclimate-client-js

Length of output: 12908


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- client.ts definitions and direct-CID flow ---'
sed -n '1,65p' src/client.ts
sed -n '225,325p' src/client.ts
printf '%s\n' '--- dataset and metadata types ---'
rg -n -C 4 'interface DatasetMetadata|type DatasetMetadata|interface.*Dataset|attrs' src tests/helpers tests/fetch-dataset-cid.test.ts tests/metadata.test.ts
printf '%s\n' '--- direct-CID client tests ---'
rg -n -C 8 'loadDataset|direct_cid|zarrGroup|_ipfs_zarr_group' tests src/client.ts
printf '%s\n' '--- runtime probe for normalizeZarrGroup inputs ---'
node - <<'JS'
function normalizeZarrGroup(group) {
  const normalized = group?.replace(/^\/+/, '').replace(/\/+$/, '');
  return normalized || undefined;
}
for (const value of [undefined, '0', '/0/', '', 0, false, {}, []]) {
  try {
    console.log(JSON.stringify(value), '=>', JSON.stringify(normalizeZarrGroup(value)));
  } catch (error) {
    console.log(JSON.stringify(value), '=> throws', error.name + ': ' + error.message);
  }
}
JS

Repository: dClimate/dclimate-client-js

Length of output: 50383


🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- focused runtime verifier ---'
node - <<'JS'
function normalizeZarrGroup(group) {
  const normalized = group?.replace(/^\/+/, '').replace(/\/+$/, '');
  return normalized || undefined;
}
function current(attrs, explicitZarrGroup) {
  return explicitZarrGroup ?? normalizeZarrGroup(attrs?._ipfs_zarr_group);
}
function guarded(attrs, explicitZarrGroup) {
  const discoveredZarrGroup = attrs?._ipfs_zarr_group;
  return explicitZarrGroup ??
    (typeof discoveredZarrGroup === 'string'
      ? normalizeZarrGroup(discoveredZarrGroup)
      : undefined);
}
for (const value of [undefined, ' /0/ ', '/0/', 0, false, {}, [], null]) {
  const attrs = { _ipfs_zarr_group: value };
  let currentResult;
  try {
    currentResult = JSON.stringify(current(attrs, undefined));
  } catch (error) {
    currentResult = `throws ${error.name}: ${error.message}`;
  }
  let guardedResult;
  try {
    guardedResult = JSON.stringify(guarded(attrs, undefined));
  } catch (error) {
    guardedResult = `throws ${error.name}: ${error.message}`;
  }
  console.log(`${JSON.stringify(value)} | current: ${currentResult} | guarded: ${guardedResult}`);
}
console.log('explicit-group short-circuit:', current({ _ipfs_zarr_group: 0 }, 'explicit'));
JS
printf '%s\n' '--- package/type context ---'
sed -n '25,45p' src/types.ts
sed -n '125,145p' src/types.ts
sed -n '1,125p' src/ipfs/open-dataset.ts
printf '%s\n' '--- existing client test mocks ---'
rg -n -C 5 'vi\.mock|openDatasetFromCid|DClimateClient' tests --glob '*client*' --glob '*fetch*' --glob '*root*'

Repository: dClimate/dclimate-client-js

Length of output: 25539


Guard _ipfs_zarr_group before normalization. If the attribute is not a string, skip normalization. The type assertion does not change the runtime value, and normalizeZarrGroup throws when .replace is called on a number, boolean, object, or array after the dataset opens.

🤖 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 `@src/client.ts` around lines 206 - 208, Update the openedZarrGroup
initialization near explicitZarrGroup to validate
dataset.attrs?._ipfs_zarr_group is a runtime string before passing it to
normalizeZarrGroup; otherwise treat it as absent and preserve the
explicitZarrGroup precedence.

Comment on lines +15 to +17
const response = await fetchImpl(url, {
headers: { Accept: "application/json" },
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect all STAC-to-version-API URL flows and test fixtures.
ast-grep outline src/versions/version-client.ts --items all
rg -n -C 5 'dclimate:versions_api|versionsApi|citationApi|listVersionsFromUrl|fetchImpl' src tests README.md

# Identify whether the repository intentionally supports remote HTTP endpoints
# or redirects for version and citation services.
rg -n -C 3 'https?://|redirect' tests src README.md

Repository: dClimate/dclimate-client-js

Length of output: 43070


Security Misconfiguration (CWE-345)

Reachability: External · Exploitability: Moderate

Reachability path
● Entry
  src/client.ts:147
  listDatasetVersions
│
▼
● Sink
  src/versions/version-client.ts

Require HTTPS for STAC-discovered version APIs.

Reject non-HTTPS URLs and validate redirect targets before following them. Allow HTTP only through an explicit local-development opt-in.

🤖 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 `@src/versions/version-client.ts` around lines 15 - 17, Update the version API
fetch flow around fetchImpl to require HTTPS for the initial URL and every
redirect target, rejecting non-HTTPS URLs by default. Permit HTTP only when the
explicit local-development opt-in is enabled, and validate each redirect before
following it.

@da-code-reviewer da-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Codex Automated Review

Found 2 actionable issues.
Posted 2 inline comment(s).

Comment thread src/client.ts
: {}),
...(resolved.citationApi ? { citationApi: resolved.citationApi } : {}),
...(resolved.streamId ? { streamId: resolved.streamId } : {}),
...(resolved.commitId ? { commitId: resolved.commitId } : {}),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LOW
When STAC omits a group and openDatasetFromCid falls back to group 0, this value remains undefined, so the returned metadata omits the group actually opened. After opening, fall back to dataset.attrs._ipfs_zarr_group, as the direct-CID branch already does.

fetchImpl: FetchImplementation = fetch
): Promise<DatasetVersion> {
const url = new URL(versionsUrl);
url.pathname = `${url.pathname.replace(/\/$/, "")}/${encodeURIComponent(commitId)}`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LOW
Reject an empty commitId before constructing the URL. An empty value produces the listing URL with a trailing slash, which can return DatasetVersionListing while this helper silently casts it to DatasetVersion.

@da-code-reviewer da-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Codex Automated Review

Found three actionable issues: an SSRF risk and two metadata/version-resolution correctness gaps.
Posted 3 inline comment(s).

Comment thread src/client.ts
`Version history is not available for ${collection}/${dataset}/${resolved.variant}.`
);
}
return listVersionsFromUrl(resolved.versionsApi, filters);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MEDIUM
Validate STAC-advertised service URLs before fetching. versionsApi comes directly from catalog metadata, so a custom or compromised STAC source can point Node callers at loopback, private-network, or cloud-metadata endpoints; automatic redirects can bypass validation of only the initial URL. Enforce a trusted HTTPS-origin policy for the initial request and every redirect, including the exact-version path.

Comment thread src/client.ts Outdated
const metadataVariant = resolved.variant || "";
const metadataOrganization =
resolved.organizationId ?? resolvedOrganization;
const zarrGroup = explicitZarrGroup ?? normalizeZarrGroup(resolved.zarrGroup);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LOW
Record the group selected by the opener fallback. When STAC provides no group and openDatasetFromCid retries group "0", this precomputed value remains undefined, so the returned metadata omits the group actually opened. After opening, fall back to the string-valued dataset.attrs._ipfs_zarr_group, as the direct-CID branch does.

fetchImpl: FetchImplementation = fetch
): Promise<DatasetVersion> {
const url = new URL(versionsUrl);
url.pathname = `${url.pathname.replace(/\/$/, "")}/${encodeURIComponent(commitId)}`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LOW
Reject an empty commitId before constructing the URL. An empty value produces the versions-list URL with a trailing slash; services with non-strict routing can return DatasetVersionListing, which is then silently cast to DatasetVersion. Validate that the identifier is non-empty before appending it.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 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 `@src/stac/stac-catalog.ts`:
- Around line 222-239: Update getStacZarrResolutions to accept item properties
and include a Zarr resolution descriptor from those properties when
dclimate:spatial_resolution and dclimate:zarr_group are present, while
preserving asset discovery and deduplication. In src/stac/stac-catalog.ts lines
745-750 and src/stac/stac-server.ts lines 227-232, pass selectedItem.properties
into the updated function.

In `@src/types.ts`:
- Line 37: Remove the inactive options.resolution field from the relevant type,
or wire it into DClimateClient.loadDataset’s resolution selection and validation
so it behaves consistently with request.resolution. Do not leave an exposed
option that is ignored.
🪄 Autofix

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: 6cf010b8-5a1f-4b7f-b910-8956b687d90d

📥 Commits

Reviewing files that changed from the base of the PR and between ec3f5e7 and 7ccfcfa.

📒 Files selected for processing (14)
  • README.md
  • src/client.ts
  • src/errors.ts
  • src/index.ts
  • src/ipfs/open-dataset.ts
  • src/stac/index.ts
  • src/stac/stac-catalog.ts
  • src/stac/stac-server.ts
  • src/types.ts
  • tests/fetch-dataset-cid.test.ts
  • tests/geotemporal-dataset.test.ts
  • tests/open-dataset.test.ts
  • tests/review-fixes/concat-items-resolved-id.test.ts
  • tests/stac-version-discovery.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/stac/index.ts
  • src/index.ts

Comment thread src/stac/stac-catalog.ts
Comment on lines +222 to 239
export function getStacZarrResolutions(
assets: Record<string, StacAsset>
): StacZarrResolution[] {
const choices = Object.entries(assets).flatMap(([assetKey, asset]) => {
if (assetKey === "data") return [];
const resolution = getStringProperty(asset, "dclimate:spatial_resolution");
const group = getStringProperty(asset, "dclimate:zarr_group");
return resolution && group ? [{ assetKey, resolution, group }] : [];
});
return choices.filter(
(choice, index) =>
choices.findIndex(
(candidate) =>
candidate.resolution === choice.resolution &&
candidate.group === choice.group
) === index
);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Read item-level Zarr metadata during resolution discovery.

getStacZarrResolutions reads only asset fields. Both resolvers discard selectedItem.properties. A STAC item that advertises its Zarr group or spatial resolution in item properties therefore exposes no selectable resolution.

  • src/stac/stac-catalog.ts#L222-L239: Accept item properties and create the applicable Zarr resolution descriptor from them.
  • src/stac/stac-catalog.ts#L745-L750: Pass selectedItem.properties into resolution discovery.
  • src/stac/stac-server.ts#L227-L232: Pass selectedItem.properties into resolution discovery.
📍 Affects 2 files
  • src/stac/stac-catalog.ts#L222-L239 (this comment)
  • src/stac/stac-catalog.ts#L745-L750
  • src/stac/stac-server.ts#L227-L232
🤖 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 `@src/stac/stac-catalog.ts` around lines 222 - 239, Update
getStacZarrResolutions to accept item properties and include a Zarr resolution
descriptor from those properties when dclimate:spatial_resolution and
dclimate:zarr_group are present, while preserving asset discovery and
deduplication. In src/stac/stac-catalog.ts lines 745-750 and
src/stac/stac-server.ts lines 227-232, pass selectedItem.properties into the
updated function.

Comment thread src/types.ts Outdated

@da-code-reviewer da-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Codex Automated Review

Found 3 actionable issues.
Posted 3 inline comment(s).

Comment thread src/client.ts
);
}
return listVersionsFromUrl(resolved.versionsApi, filters);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MEDIUM
versionsApi comes directly from remotely supplied STAC metadata and is fetched without an endpoint policy; getDatasetVersion does the same. In Node, a malicious catalog can target loopback/private services and return their JSON or error body to the caller. Reject non-HTTP(S) and private destinations, or require an explicit allowlist/validation callback before following advertised URLs.

Comment thread src/stac/stac-catalog.ts
const zarrResolutions = getStacZarrResolutions(selectedItem.assets);
const selectedAsset =
selectedItem.assets.data ??
(zarrResolutions[0]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MEDIUM
The CID is fixed from assets.data or the first resolution asset before the requested resolution is selected. If resolution assets have different hrefs, selecting another resolution still opens the first CID with the selected group, producing wrong data or an open failure. Derive the CID from the selected assetKey, or validate that all resolution assets share one href.

Comment thread src/types.ts Outdated
returnJaxrayDataset?: boolean;
autoConcatenate?: boolean;
zarrGroup?: string;
resolution?: string;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MEDIUM
LoadDatasetOptions.resolution is exposed publicly but never read; selection only consults request.resolution. Consequently { options: { resolution: "2km" } } type-checks but is ignored and a multiresolution dataset still throws. Remove this field or incorporate it into selection and conflict validation.

@eloramirez1356

Copy link
Copy Markdown
Collaborator Author

@codex review this pr and concretely the last commit

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create a Codex account and connect to github.

@da-code-reviewer da-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Codex Automated Review

Found 3 actionable issues.
Posted 3 inline comment(s).

Comment thread src/client.ts
`Version history is not available for ${collection}/${dataset}/${resolved.variant}.`
);
}
return listVersionsFromUrl(resolved.versionsApi, filters);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MEDIUM
Validate STAC-discovered service URLs. versionsApi is remotely supplied metadata and reaches Node's fetch unchanged, allowing a malicious catalog to target loopback/private services; redirects are followed automatically. Apply a configurable trusted-origin/destination policy to this and the exact-version path, including redirect targets.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not addressing this in the current patch. dClimate generates and controls the supported STAC catalogs and their advertised service URLs, which are treated as trusted first-party configuration. Arbitrary third-party catalogs are outside the current trust model. If external catalogs are supported later, we will define a dedicated URL and redirect security policy. This does not block the multiresolution change.

Comment thread src/stac/stac-catalog.ts
export function getStacZarrResolutions(
assets: Record<string, StacAsset>
): StacZarrResolution[] {
const choices = Object.entries(assets).flatMap(([assetKey, asset]) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MEDIUM
Include item-level Zarr metadata. Resolution discovery only inspects assets, so items placing dclimate:zarr_group and dclimate:spatial_resolution in properties expose no choices: request.resolution is rejected and grouped roots cannot be selected. Merge a property-derived choice and pass item properties from both resolvers.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not applicable to the current STAC contract. Selectable resolutions are represented exclusively by named assets containing dclimate:spatial_resolution and dclimate:zarr_group. assets.data is only a temporary compatibility alias and must not become selectable. Item-level resolution/group metadata is unsupported, so no fallback will be added.

fetchImpl: FetchImplementation = fetch
): Promise<DatasetVersion> {
const url = new URL(versionsUrl);
url.pathname = `${url.pathname.replace(/\/$/, "")}/${encodeURIComponent(commitId)}`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LOW
Reject an empty commitId. An empty identifier produces the listing URL with a trailing slash; services using non-strict routing can return DatasetVersionListing, which is then exposed as DatasetVersion. Validate the identifier before constructing the path.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Valid but unrelated to the multiresolution feature. Empty or whitespace-only commit IDs should be rejected in a separate version-lookup follow-up. This is low priority and does not block the current merge.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant