ci: add cognitive-complexity and dependency-direction gates, plus a weekly structural sweep

[thodson-usgs]

Follows the survey in Python code quality tools beyond linting. The repo already ratchets cyclomatic complexity with xenon and trends it with wily; this evaluates the rest of that list, adopts the three that earn their keep, and — the larger half of the change — retires the hand-rolled architecture tests that the right tool expresses better.

What lands

	Tool	Where	Blocking?
Cognitive complexity	complexipy	pre-commit + CI	yes, ratchet at 27
Dependency direction	import-linter	pre-commit + CI	yes, 7 contracts
Structural sweep	pyscn	weekly cron + workflow_dispatch	no, advisory report

Both gates run in well under a second and are wired the way xenon already is: identical command in the hook and in CI, so a clean pre-commit run means CI agrees.

.importlinter replaces 11 of 23 architecture tests

tests/architecture_test.py hand-parses the AST to assert dependency rules. Eleven of those were plain statements about the import graph, which is exactly what a boundary checker is for. They are now seven declarative contracts, and the tests are deleted — not kept alongside, since one rule with two homes is one rule that gets updated in one place.

Three import-linter features made a bigger cut possible than I first estimated:

• acyclic_siblings replaces both hand-written DFS cycle detectors, and states the claim package-wide instead of once per subsystem.
• protected expresses "only NGWMN and Water Data may consume OGC" as an allowlist, so a new service module is refused by default rather than permitted until someone remembers to extend a denylist.
• ** wildcards make the NGWMN facade-only rule cover OGC submodules that don't exist yet.

Equivalence was verified, not assumed. For each of the 11 tests: inject the violation it exists to catch, confirm both the test and the replacement contract fail, revert.

The first run came back 2 of 11 not equivalent, and the failure was worth the exercise. The protected contract matches allowed_importers with as_packages, so listing the root package there made every module in the distribution an allowed importer — the contract could not fail on anything. A contract that can't fail is worse than no contract, because it reads as coverage. The root's two legitimate OGC imports are narrow ignore_imports entries now, and the rerun was 11 of 11 with both sides firing.

TEST                                                       TEST     CONTRACT
--------------------------------------------------------------------------------
ogc_does_not_depend_on_service_adapters                    failed   broken
modern_modules_do_not_depend_on_legacy_nwis                failed   broken
top_level_ogc_consumers_match_documented_variances         failed   broken
ogc_runtime_graph_is_acyclic                               failed   broken
shaping_has_no_engine_dependency                           failed   broken
ngwmn_uses_ogc_facade                                      failed   broken
transport_does_not_depend_on_ogc_or_services               failed   broken
wateruse_has_no_ogc_dependency                             failed   broken
transport_runtime_graph_is_acyclic                         failed   broken
waterdata_collection_families_do_not_import_each_other     failed   broken
service_adapters_do_not_reach_through_each_other           failed   broken
--------------------------------------------------------------------------------
ALL 11 CASES EQUIVALENT

What deliberately stays a test

The 12 remaining tests are the ones an import graph genuinely cannot express, and the module docstring now says so — including the instruction that a new module-to-module rule belongs in .importlinter, not here:

• Symbol-level seams — ogc.engine's compatibility surface is capped by count; ogc.requests may borrow header policy but not the executing calls. A graph sees module edges, not which names cross them.
• Declared public surface — __all__ presence, single ownership per getter, the facade's exact export union.
• AST shape — the facade proving it contains no logic; _default_headers calls proving they pass a destination URL; only credentials naming the API-key host.
• A positive assertion — shaping must import ogc.schema. lint-imports can forbid an edge, never require one.

Net: 23 tests → 12, and the module runs in 0.64s instead of 1.6s.

Report from the first full pyscn run

Health Score: 78/100 (Grade: B)
  Complexity:      95/100 ✅  (avg: 2.5, high-risk: 20 functions)
  Dead Code:      100/100 ✅  (0 issues, 0 critical)
  Duplication:     60/100 ⚠️  (11.4% fragments cloned, 8 groups)
  Coupling (CBO): 100/100 ✅  (avg: 1.6, 0/7 high-coupling)
  Cohesion (LCOM):100/100 ✅  (avg: 1.1, 0/26 high-lcom)
  Dependencies:    70/100 ⚠️  (no cycles, depth: 11)
  Architecture:    85/100 👍  (85% compliant)

No correctness issues to resolve — zero dead code even at --min-severity info, zero cycles, no high-coupling or low-cohesion classes, and lizard agrees with xenon on the three functions over CCN 15. No package source changed in this PR.

The two soft numbers, read rather than obeyed:

• Duplication (60). All eight groups are false positives for this package. Six are the per-collection getter families — thin, heavily documented wrappers whose bodies necessarily rhyme. Two are deliberate pairs: utils.query / _query_with_retry, identical but for retry policy, and ChunkCall.partial_frame / partial_response. Collapsing any of them trades the documented public surface for a metric. This is the main reason pyscn is advisory and not a gate.
• Architecture (85) / SRP warnings. It flags exceptions and credentials as multi-responsibility because many packages import them — afferent coupling on a stable leaf, which is what ADR 0003 asks for, scored as if it were the opposite. Its DI check similarly flags module-level singletons and one function it misidentifies as a class (planning._extract_axes).

Score analysis: what actually hurts, and what is worth fixing

Asked what drags the 78/100 down and whether it can be honestly improved, I went through each sub-score. Most of the gap is the tool disagreeing with decisions this project made on purpose. Full write-up in #361; summary:

Sub-score	Driver	Honest fix?
Duplication 60	8 clone groups, all the documented getter families and deliberate wrapper pairs	No — collapsing trades the public API surface for a metric
Dependencies 70	max graph depth, via utils.py (Ca=19)	Substantial → #361
Architecture 85	SRP heuristic flagging exceptions/credentials for being widely imported	No — that is afferent coupling on a stable leaf, i.e. what ADR 0003 requires
Modularity 50 / 19 "high-risk modules"	abstractness = 0 across all 50 modules, so every stable leaf sits at distance 1	No — raising it means adding ABCs to exceptions, codes, rdb; textbook gaming
Complexity 95	17 of the 20 "high-risk functions" are synthetic <module> rows where pyscn sums a whole file (planning.py: cognitive 140, cyclomatic 1)	3 real functions; one fixed here

The one real fix, included here

ogc.filters._split_top_level_or was the only genuine hotspot — cognitive 36 in a package where nothing else exceeded 27. Four levels of nesting went to one question ("does a <space>OR<space> separator start here?") written as inline boundary arithmetic. Extracted as _resume_after_or, returning the next clause's start or None; the scan loop becomes mutually exclusive elif arms over a single character. Same algorithm, named intermediate.

It parses user-supplied filter input, so behavior preservation is checked, not assumed — a differential harness runs the original against the new implementation over 34 adversarial cases (the ORDER BY token trap, doubled-quote escapes, unbalanced parens, unclosed quotes, tabs/newlines), every 1–4 token sequence exhaustively, and 200k randomized expressions. 222,654 expressions, zero mismatches.

Package ceiling 36 → 27, so the ratchet tightens 25% in the hook, CI, and CONTRIBUTING.

The pyscn composite did not move — still 78/100, because Complexity was already 95 and rounds the same. Worth stating plainly: the durable win is the tighter gate, not the score.

A second fix, found by wily's history

wily indexes metrics across git history, and that dimension found something a point-in-time scan cannot. Over the last 120 revisions the Water Data refactor moved 706 lines out of waterdata/utils.py (MI 34.5 → 74.3, cyclomatic −108) while the shared utils.py grew 234 → 527 lines and 5 → 14 functions (MI 66.6 → 56.1, cyclomatic +27). Everything around it improved; that one module absorbed the difference.

Auditing what its 19 dependents actually wanted: 15 of the 21 import sites wanted one name — BaseMetadata, the second half of every getter's (DataFrame, metadata) contract. A class whose only dependency is httpx.Response, sitting beside the legacy query machinery, so importing it dragged transport + credentials + error policy in transitively.

Extracted to dataretrieval/response_metadata.py, a dependency-free leaf. dataretrieval.utils.BaseMetadata still resolves (re-export), since tests and users import that path.

Afferent coupling on utils.py: 20 → 10. Ten modules — ngwmn, ogc.shaping, and eight waterdata modules — no longer depend on utils at all. Max depth stays 14; I simulated that before making the change, since depth is driven by legitimate waterdata → ogc → transport layering rather than this edge.

The .importlinter exhaustive stack refused to pass until the new module was placed deliberately — the contract doing its job.

The strongest evidence for keeping pyscn advisory

That extraction is unambiguously good design: dependency-free leaf, coupling halved, public path preserved, all tests and contracts green.

pyscn's architecture score went 85 → 81 for it.

The new leaf is now itself an "SRP violation" — responsibilities = [ngwmn, nwis, ogc, waterdata, wateruse, wqp], i.e. penalized for being depended upon — and every importer gained a "responsibility". A controlled before/after where a real improvement moves the metric backwards. Had pyscn been a merge gate with a threshold, it would have blocked this change. It stays advisory.

Checked and rejected

codes/states.py shows the sharpest MI drop in the whole history (100 → 70.5, cyclomatic +18) and looks alarming. It is a false alarm: the file went from a pure lookup table to a table plus a documented state-normalization API. Logic living where it belongs is not erosion, and "fixing" it would mean moving state handling somewhere worse.

Deliberately not done

The rest of the utils.py split — separating the legacy HTTP-query half from the pure stdlib helpers (Ambient, _require_positive_int, to_str) — stays in #361. The legacy-query half is entangled with the NWIS deprecation, so it may be cheaper to wait for that removal than to move it twice.

Considered and skipped

• tach — benchmarked rather than argued about, since Rust suggested it should win: 222ms vs import-linter's 160ms, while import-linter checked more contracts. grimp's graph builder is already Rust, so at 50 modules CLI startup dominates. With no speed case, a second boundary config to keep in sync isn't worth it. (tach init is also interactive-only — it blocks on Enter and pitches a hosted service — so the only automatable path is hand-writing tach.toml.)
• lizard — cyclomatic complexity and duplication, both already covered. Ran it as a cross-check; same three functions.
• cohesion / lcom / module_coupling_metrics — these measure class cohesion. This package is function-oriented by policy, and pyscn already reports LCOM4 at 1.1 average across 26 classes with zero offenders.

Packaging note

pyscn stays in its own [health] extra. It ships a compiled Go binary with no linux/aarch64 wheel — that's a container built on Apple Silicon, and an ARM CI runner — and [metrics] is what the blocking CI job and every contributor installs, so it's worth keeping a compiled third-party binary off that path regardless.

One duplication I did not remove: the 36 threshold appears in both the complexipy hook args and the CI command, as xenon's thresholds already do. complexipy has no config-file support, so removing it would mean CI invoking pre-commit instead — a change to this repo's CI convention, and out of scope here.

Verification

• lint-imports — 7 contracts kept, and all 7 shown to fail on an injected violation
• pytest tests/ — 751 passed, 12 deselected (762 − 11 deleted)
• dataretrieval.utils.BaseMetadata verified to still resolve to the same object after the move
• The _split_top_level_or refactor differential-tested against the original over 222,654 expressions, zero mismatches
• pre-commit run --all-files — all 14 hooks pass
• pip install -e .[metrics,health] resolves clean on 3.13; every CI command line re-run from that fresh env

🤖 Generated with Claude Code