Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions .github/workflows/code-health.yml
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
# Periodic structural sweep: duplication, coupling, cohesion, dead code, and the
# maintainability trend.
# Periodic health sweep: dependency vulnerabilities, duplication, coupling,
# cohesion, dead code, and the maintainability trend.
#
# Advisory by design, and nothing here fails the build. These measures move over
# months rather than commits, and they report findings a reviewer has to judge
# rather than obey -- a metric can call good design a violation. The merge gates
# live in python-package.yml. See CONTRIBUTING.md for how to read the output.
# Advisory by design, and nothing here fails the build. Structural measures move
# over months rather than commits, while vulnerability reports and metric
# findings both need human triage. The merge gates live in python-package.yml.
# See CONTRIBUTING.md for how to read the output.

name: Code Health

Expand All @@ -27,20 +27,29 @@ jobs:
runs-on: ubuntu-latest
timeout-minutes: 20
steps:
- uses: actions/checkout@v6
- uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6
with:
persist-credentials: false
# wily indexes git history; a shallow clone gives it one revision to
# trend against, which is no trend at all.
fetch-depth: 0
- name: Set up Python 3.13
uses: actions/setup-python@v6
uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6
with:
python-version: "3.13"
cache: "pip"
- name: Install analysis tooling
run: |
python -m pip install --upgrade pip
pip install -e .[metrics,health]
pip install -e .[metrics,health,security]

- name: Dependency vulnerability audit (pip-audit)
# Advisory like the structural scan: publish new vulnerability reports
# for triage without making an unrelated PR depend on advisory uptime.
continue-on-error: true
run: |
python -m pip_audit --strict --progress-spinner off . \
2>&1 | tee pip-audit-summary.txt

- name: Structural analysis (pyscn)
# One output format per invocation, so run it twice: JSON to read the
Expand Down Expand Up @@ -92,6 +101,11 @@ jobs:
if: always()
run: |
{
echo '## Dependency audit'
echo '```'
cat pip-audit-summary.txt 2>/dev/null \
|| echo 'pip-audit produced no output'
echo '```'
echo '## Structural analysis'
echo '```'
if [[ -s pyscn-summary.txt ]]; then
Expand All @@ -111,13 +125,14 @@ jobs:

- name: Upload reports
if: always()
uses: actions/upload-artifact@v4
uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4
with:
name: code-health-report
path: |
.pyscn/reports/
pyscn-summary.txt
pyscn-sanity.txt
pip-audit-summary.txt
wily-summary.txt
retention-days: 90
if-no-files-found: warn
32 changes: 22 additions & 10 deletions .github/workflows/python-package.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,18 @@ on:
pull_request:
branches: ['main']

permissions:
contents: read

jobs:
lint:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6
- uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6
with:
persist-credentials: false
- name: Set up Python 3.14
uses: actions/setup-python@v6
uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6
with:
python-version: "3.14"
cache: "pip"
Expand All @@ -30,13 +35,14 @@ jobs:
complexity:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6
- uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6
with:
persist-credentials: false
# wily needs history to compare against the base; the gate itself
# only needs the working tree.
fetch-depth: 0
- name: Set up Python 3.14
uses: actions/setup-python@v6
uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6
with:
python-version: "3.14"
cache: "pip"
Expand Down Expand Up @@ -73,9 +79,11 @@ jobs:
package-artifact:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6
- uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6
with:
persist-credentials: false
- name: Set up Python 3.13
uses: actions/setup-python@v6
uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6
with:
python-version: "3.13"
cache: "pip"
Expand Down Expand Up @@ -112,9 +120,11 @@ jobs:
type-check:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6
- uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6
with:
persist-credentials: false
- name: Set up Python 3.13
uses: actions/setup-python@v6
uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6
with:
python-version: "3.13"
cache: "pip"
Expand All @@ -135,9 +145,11 @@ jobs:
python-version: ["3.10", "3.13", "3.14"]

steps:
- uses: actions/checkout@v6
- uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6
with:
persist-credentials: false
- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v6
uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6
with:
python-version: ${{ matrix.python-version }}
cache: "pip"
Expand Down
7 changes: 6 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@

## Environment
- Use `pip install .[test,nldi]` (CI uses pip, not uv despite `uv.lock`). Docs: `pip install .[doc,nldi]`.
- Full local gates additionally need `pip install .[metrics]` and `pre-commit`; periodic health analysis uses `pip install .[health,security]`.

## Commands
- Full feedback loop: `pre-commit run --all-files`, then `coverage run -m pytest tests/ && coverage report -m`.
- Lint: `ruff check .` and `ruff format --check .`.
- Tests: `coverage run -m pytest tests/ && coverage report -m`, or focused like `pytest tests/waterdata_test.py::test_mock_get_samples`.
- Tests: the coverage command enforces a reported 90.00% package branch-coverage floor; use focused tests like `pytest tests/waterdata_test.py::test_mock_get_samples` while iterating.
- Dependencies: `pip-audit --strict --progress-spinner off .` is a periodic advisory check of the latest compatible core resolution, not optional extras or every allowed version.
- Docs: install docs deps, `ipython kernel install --name "python3" --user`, then `make html` from `docs/`. `make docs` adds doctest+linkcheck (network-dependent).

## Testing Gotchas
Expand All @@ -25,6 +28,8 @@
- `tests/waterdata_test.py` and `tests/waterdata_ratings_test.py` skip on Python <3.10, so a 3.9 run does not cover them.

## Implementation Notes
- Treat Ruff `S` findings as defects unless a narrow, documented suppression explains why the flagged operation is safe in context.
- Before copying a similar implementation, check whether the behavior belongs in a shared helper; keep explicit public getter signatures and docstrings even when their thin bodies intentionally rhyme.
- HTTP client is `httpx` (migrated from `requests` in #289); new code should use `httpx` and tests should mock with `httpx_mock`.
- Public download helpers return `(DataFrame, metadata)`.
- `dataretrieval/__init__.py` star-imports service modules; `dataretrieval/waterdata/__init__.py` controls Water Data exports via `__all__`.
Expand Down
28 changes: 27 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,20 @@ about the upstream service rather than about this package.
### Coding Standards and Style

The continuous integration and pre-commit configurations enforce formatting,
linting, and strict type checking. Run the relevant checks before opening a PR:
linting, security-smell checks, and strict type checking. The shortest complete
local feedback loop before opening a PR is:

```bash
pre-commit run --all-files
coverage run -m pytest tests/
coverage report -m
```

Coverage measures branches in the package itself, excludes the generated
``_version.py``, and fails when the reported total rounds below 90.00%. This is
a repository-wide floor, not a claim that every changed line is tested.

The equivalent individual checks are:

```bash
ruff check .
Expand Down Expand Up @@ -154,6 +167,11 @@ ADR 0003). If you are adding a rule and it is purely "module A must not import
module B", it belongs in `.importlinter`. A boundary that legitimately moves is
one edit there, plus the ADR it cites.

Ruff's ``S`` rules provide the static security-smell gate. Suppress a finding
only at the narrowest justified scope and explain why it is safe; tests ignore
``assert`` warnings, pickle compatibility tests ignore pickle warnings, and
retry jitter explicitly marks its non-cryptographic randomness.

To see the *trend* rather than a pass/fail, that extra also installs
[`wily`](https://github.com/tonybaloney/wily), which indexes metrics across git
history:
Expand All @@ -177,6 +195,14 @@ which attaches an HTML and a JSON report to each run. Nothing gates on it. These
measures move over months rather than commits, and a threshold nobody agreed to
is either noise or theatre.

The same workflow runs `pip-audit` against the latest compatible resolution of
the core runtime dependencies and includes its output in the weekly report. Run
it on demand with `pip install -e '.[security]'` followed by
`pip-audit --strict --progress-spinner off .`. This catches known
vulnerabilities in the resolution users receive today; it does not prove every
version allowed by broad constraints is safe, and project-path mode does not
audit optional extras.

You do not need it to contribute, but it is the right tool for "what should we
clean up next?" -- including for an agent working on this repo, which gets a
whole-package structural picture from one command:
Expand Down
7 changes: 5 additions & 2 deletions dataretrieval/transport/retry.py
Original file line number Diff line number Diff line change
Expand Up @@ -286,8 +286,11 @@ def backoff(self, attempt: int, retry_after: float | None) -> float:
"""
ceiling = min(self.max_backoff, self.base_backoff * 2 ** (attempt - 1))
if retry_after is None:
return random.uniform(0.0, ceiling)
nudge = random.uniform(0.0, min(self.max_backoff, _RETRY_AFTER_JITTER))
# Retry jitter decorrelates clients; it is not security-sensitive.
return random.uniform(0.0, ceiling) # noqa: S311
nudge = random.uniform( # noqa: S311
0.0, min(self.max_backoff, _RETRY_AFTER_JITTER)
)
return min(retry_after + nudge, self.retry_after_cap)


Expand Down
21 changes: 18 additions & 3 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,16 @@ dataretrieval = ["py.typed"]
max-complexity-allowed = 25
failed = true

[tool.coverage.run]
branch = true
source = ["dataretrieval"]
omit = ["dataretrieval/_version.py"]

[tool.coverage.report]
fail_under = 90
precision = 2
show_missing = true

[project.optional-dependencies]
# Minimal set the CI ``type-check`` job installs — just mypy + the package,
# not the whole test stack.
Expand All @@ -67,6 +77,10 @@ metrics = [
health = [
"pyscn==1.29.0",
]
# Audit the latest compatible resolution of the core runtime dependencies.
security = [
"pip-audit==2.10.1",
]
test = [
"pytest > 5.0.0",
"pytest-cov[all]",
Expand Down Expand Up @@ -113,9 +127,10 @@ extend-exclude = ["demos"]
[tool.ruff.lint]
preview = true
# Select rules: Pyflakes(F), pycodestyle(E,W), isort(I), pyupgrade(UP),
# flake8-bugbear(B), flake8-quotes(Q), flake8-simplify(SIM), flake8-tidy-imports(TID)
# flake8-bugbear(B), flake8-quotes(Q), flake8-simplify(SIM),
# flake8-tidy-imports(TID), flake8-bandit(S)
select = [
"F", "E", "W", "I", "UP", "B", "Q", "SIM", "TID",
"F", "E", "W", "I", "UP", "B", "Q", "SIM", "TID", "S",
"C90", # mccabe
"E501", # line-length
]
Expand All @@ -128,7 +143,7 @@ ignore = [
max-complexity = 20

[tool.ruff.lint.per-file-ignores]
"tests/*" = ["SIM108"]
"tests/*" = ["SIM108", "S101"]
"**/__init__.py" = ["F403"]

[tool.ruff.format]
Expand Down
2 changes: 1 addition & 1 deletion tests/headers_host_scoping_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
class TestDefaultHeadersHostScoping:
"""_default_headers only sends X-Api-Key to the authorized Water Data host."""

FAKE_TOKEN = "test-fake-token-abc123"
FAKE_TOKEN = "test-fake-token-abc123" # noqa: S105 - deliberate test token

@pytest.fixture(autouse=True)
def _api_token(self, monkeypatch: pytest.MonkeyPatch) -> None:
Expand Down
12 changes: 8 additions & 4 deletions tests/utils_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ def test_typed_errors_survive_pickle_and_deepcopy(self):
back from multiprocessing / lithops workers, and their constructor fields
(status_code, retry_after, url) must survive the trip."""
import copy
import pickle
import pickle # noqa: S403 - testing trusted compatibility round-trips

import httpx

Expand All @@ -213,7 +213,10 @@ def test_typed_errors_survive_pickle_and_deepcopy(self):
exceptions.NetworkError("could not reach the service"),
]
for err in samples:
for revived in (pickle.loads(pickle.dumps(err)), copy.deepcopy(err)):
for revived in (
pickle.loads(pickle.dumps(err)), # noqa: S301 - created above
copy.deepcopy(err),
):
assert type(revived) is type(err)
assert str(revived) == str(err)
if isinstance(err, exceptions.HTTPError):
Expand Down Expand Up @@ -302,7 +305,7 @@ def test_init_with_response(self):

def test_pickle_keeps_historical_import_path(self):
"""New pickles remain readable by releases predating the class move."""
import pickle
import pickle # noqa: S403 - testing trusted compatibility round-trips

from dataretrieval._response_metadata import BaseMetadata

Expand All @@ -315,7 +318,8 @@ def test_pickle_keeps_historical_import_path(self):
payload = pickle.dumps(md)

assert b"dataretrieval.utils" in payload
assert pickle.loads(payload).__class__ is BaseMetadata
revived = pickle.loads(payload) # noqa: S301 - payload created above
assert revived.__class__ is BaseMetadata


class Test_to_str:
Expand Down
8 changes: 4 additions & 4 deletions tests/waterdata_chunking_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -801,7 +801,7 @@ def test_chunk_interrupted_pickles_as_degraded_across_process_boundary():
ProcessPoolExecutor pool could not ship it back. ``__getstate__`` drops
``.call`` and pickles the documented degraded ``call=None`` state -- counts
and retry hint preserved, ``.resume()`` gone (un-resumable cross-process)."""
import pickle
import pickle # noqa: S403 - testing trusted compatibility round-trips

plan = ChunkPlan(
{"monitoring_location_id": ["A", "B", "C"]}, _fake_build, url_limit=8000
Expand All @@ -815,7 +815,7 @@ def test_chunk_interrupted_pickles_as_degraded_across_process_boundary():
with pytest.raises((pickle.PicklingError, AttributeError)):
pickle.dumps(exc.call.fetch)

revived = pickle.loads(pickle.dumps(exc))
revived = pickle.loads(pickle.dumps(exc)) # noqa: S301 - created above
assert isinstance(revived, QuotaExhausted)
assert revived.call is None # degraded: no cross-process resume handle
assert revived.completed_chunks == exc.completed_chunks
Expand All @@ -830,7 +830,7 @@ def test_chunk_interrupted_with_partial_data_pickles_intact():
Exercises the path the no-completed-chunks case above doesn't: a real
``partial_frame`` (rows) and ``partial_response`` (a live ``httpx.Response``,
which must itself remain picklable)."""
import pickle
import pickle # noqa: S403 - testing trusted compatibility round-trips

plan = ChunkPlan(
{"monitoring_location_id": ["A", "B", "C"]}, _fake_build, url_limit=8000
Expand All @@ -849,7 +849,7 @@ def test_chunk_interrupted_with_partial_data_pickles_intact():
assert exc.completed_chunks == 1
assert not exc.partial_frame.empty and exc.partial_response is not None

revived = pickle.loads(pickle.dumps(exc))
revived = pickle.loads(pickle.dumps(exc)) # noqa: S301 - created above
assert revived.call is None
assert revived.partial_frame["id"].tolist() == ["A"]
assert isinstance(revived.partial_response, httpx.Response)
Expand Down