diff --git a/.github/workflows/code-health.yml b/.github/workflows/code-health.yml index c04b9b59..1638db60 100644 --- a/.github/workflows/code-health.yml +++ b/.github/workflows/code-health.yml @@ -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 @@ -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 @@ -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 @@ -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 diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index a95ffec9..f6f8dcd1 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -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" @@ -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" @@ -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" @@ -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" @@ -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" diff --git a/AGENTS.md b/AGENTS.md index 455c108c..8046182d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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 @@ -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__`. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8a58c41d..2cb7be2e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -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 . @@ -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: @@ -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: diff --git a/dataretrieval/transport/retry.py b/dataretrieval/transport/retry.py index 577ed334..61366817 100644 --- a/dataretrieval/transport/retry.py +++ b/dataretrieval/transport/retry.py @@ -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) diff --git a/pyproject.toml b/pyproject.toml index 124dea80..5d3ca85d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -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. @@ -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]", @@ -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 ] @@ -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] diff --git a/tests/headers_host_scoping_test.py b/tests/headers_host_scoping_test.py index 82dbbb22..a2901db3 100644 --- a/tests/headers_host_scoping_test.py +++ b/tests/headers_host_scoping_test.py @@ -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: diff --git a/tests/utils_test.py b/tests/utils_test.py index 1649a366..b5d5439a 100644 --- a/tests/utils_test.py +++ b/tests/utils_test.py @@ -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 @@ -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): @@ -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 @@ -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: diff --git a/tests/waterdata_chunking_test.py b/tests/waterdata_chunking_test.py index 06657821..3248fc6b 100644 --- a/tests/waterdata_chunking_test.py +++ b/tests/waterdata_chunking_test.py @@ -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 @@ -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 @@ -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 @@ -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)