Skip to content
Open
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
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -172,3 +172,6 @@ cython_debug/

# PyPI configuration file
.pypirc

# Local Claude/agent tool settings (machine-specific)
.claude/
41 changes: 41 additions & 0 deletions docs/pathways-audit-2026-06.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# pathways — Audit (2026-06)

Dimensions run: **core** (correctness; security & provenance; maintainability & dependencies) plus conditionals **Python conventions** and **regulatory defensibility** — the tool builds and reports DFO (Fisheries and Oceans Canada) Pathways-of-Effects graphs for fish-habitat assessments, so silent omission of an effect pathway is a defensibility problem. Statistical-validity and financial-data dimensions do not apply (no estimators, no money). Security scan: `pi_audit.py --root` reported **0 findings**; git provenance is clean (single author `shepherd70@gmail.com`; the one `noreply@github.com` committer is the normal GitHub-web initial commit; no unexpected `Co-authored-by` trailers).

**Summary.** The code is readable and the `poe.py` version is reasonably structured, but the repository is mid-migration and currently holds **three-to-four divergent implementations/data models of the same tool with no single source of truth**, and two of its core features are silently broken. The graph in `stressors.json` is built purely by string-matching node names, and **seven misspelled child nodes become dead-ends that truncate effect pathways before they reach the impairment endpoint** — wrong output a user cannot see. The "Mitigation Measures" feature **never finds a match** and always prints "No mitigation information available." A newer Excel data model (`pathways.xlsx`) and a risk-scoring report (`output/2_report.txt`) reference logic and data that exist in **no committed code**. None of this is a security issue; it is correctness, reproducibility, and source-of-truth hygiene. Nothing here was changed — these are findings only.

## Critical

- `stressors.json` (graph as built in `poe.py:80-93`) — **Five misspelled child node names silently truncate regulatory effect pathways.** Because edges are keyed on free-text node names, any child whose spelling does not exactly match a defined key becomes an undefined terminal node, so the pathway dead-ends instead of continuing to the canonical endpoint *"Potential direct or indirect impairment of the habitat capacity to support one or more life processes."* Confirmed dead-ends and their intended targets:
- `:213,217` `"Change in channel morphology or shoreline morphology"` → should be `…shoreline morphometry` (defined at `:160`).
- `:172` `"Change of loss of wetted area"` (typo "of") → `"Change or loss of wetted area"` (defined at `:38`).
- `:77` `"Deposit of deleterious substances/ sediment"` (stray space after `/`) → `"Deposit of deleterious substances/sediment"` (defined at `:65`).
- `:96` `"Decreased water quality"` → `"Decrease in water quality"` (defined at `:73`).
- `:76,112` `"Decrease in food supply"` → almost certainly `"Altered food supply"` (defined at `:29`, which routes to the impairment endpoint).

Two further dead-ends are lower impact: `"Sublethal effect and/or mortality"` (`:177,189`, singular) maps to the already-terminal `"Sublethal effects and/or mortality"` (`:138`); `"Entrainment of fish"` (`:182`) appears genuinely undefined (distinct from `"Entrainment/impingement"`). Fix: correct the spellings so every child matches a defined key, and add a build-time validation step in `PoEGraph.build_graph` that errors (or warns loudly) when a referenced child is never defined as a key — that single guard would have caught all seven. Longer term, key edges on stable IDs (as `pathways.xlsx` already does) so spelling never affects connectivity.

## Major

- `poe.py:57-70` + `poe.py:208-216` — **The "Mitigation Measures" feature can never match a file.** `identify_mitigation()` looks up mitigation for each entry in `self.active_stressors`, which are the `PRIMARY_ACTIVITIES` (e.g. `"Use of Machinery on Land / Alteration of Riparian Vegetation"`). `get_text()` sanitizes only `/`→`_`, producing `"Use of Machinery on Land _ Alteration…"`. But the files in `mitigation_texts/` are named for *effect* nodes using a different convention — spaces→underscores, with inconsistent casing (mostly lowercase, e.g. `decrease_in_shade.txt`, `change_or_loss_of_fish_passage.txt`, but also the capitalized `Increased_input_of_woody_material.txt`). The keys therefore never match the filenames, so every selected activity prints `"No mitigation information available…"`. Two distinct defects: (a) the sanitizer does not lowercase or replace spaces, so it cannot reproduce the file-naming convention; (b) the files are keyed to effects while the code only ever queries primary activities. Fix: decide the intended design — most likely mitigation should be shown for the **effect nodes discovered along the active pathways**, not the primary activities — then normalize keys and filenames through one shared function (`name.lower().replace(' ', '_')…`).

- `mitigation_texts/test.py` — **A committed, broken, misfiled second implementation.** This 121-line earlier prototype expects the schema `{stressor: {"endpoints": [...]}}` (`test.py:28,51,58`), but the actual `stressors.json` maps each key to a **bare list**. Running it raises `TypeError: list indices must be integers… not 'str'` in `build_graph` on first load — it cannot work against the current data. It is also misplaced *inside the data directory* `mitigation_texts/` (harmless to the loader, which filters `.txt`, but confusing for provenance). Fix: delete it, or if it has reference value move it to `archive/` and mark it non-runnable. Do not leave a broken entry point in the data folder.

- Repo-wide — **No single source of truth: 3–4 parallel models of the same tool.** (1) `poe.py` reads `stressors.json` as a flat `{node: [children]}` graph; (2) `mitigation_texts/test.py` reads it as nested `{node: {"endpoints": […]}}` (incompatible, above); (3) `pathways.xlsx` reimplements the graph properly with `Nodes_Data` + `Edges_Data` keyed on stable `Node_ID`s (`N000…`, `E001…`) plus `Node_Type`, `Risk_Score`, coordinates and an interactive `Dashboard`; (4) `output/2_report.txt` implies a fourth, risk-scoring version (below). The Excel migration is **half-done**: the cleaner ID-based data model is committed but **no code reads it** (neither script imports `openpyxl`/`pandas` or references `.xlsx`). Fix: pick the target (the ID-based Excel model resolves the Critical string-matching fragility and is the natural choice), make one implementation authoritative, and retire the others. Until then it is unclear which artifact is current.

- `output/2_report.txt` — **Orphaned, non-reproducible output.** This report contains Project metadata, per-stressor Magnitude/Duration/Certainty, **numeric Risk Scores** (e.g. `3.4 (MEDIUM RISK)`), and Recommendations — none of which `poe.py` or `test.py` collect or compute, and neither script writes a report file at all. It also cites endpoints absent from `stressors.json` (`"Change or loss of riparian habitat"`, `"Change or loss of natural structure and cover"`). So committed output corresponds to no committed code or data and a reviewer cannot reproduce it. Fix: locate and commit the version of the tool that produced it (it appears to be the most capable variant and may be the real work-in-progress), or remove the stale artifact. Flag for regulatory defensibility: a report no one can regenerate from the repo will not survive scrutiny.

## Minor

- No dependency manifest — `networkx` and `matplotlib` (plus `openpyxl` for the Excel path) are undeclared and unpinned; there is no `requirements.txt`/`pyproject.toml` and no README. Reproducibility matters for a tool feeding habitat assessments. Fix: add a pinned manifest and a short README stating which artifact is authoritative and how to run it. (Verified versions in this env: networkx 3.4.2, matplotlib 3.10.0, Python 3.11.9.)
- `poe.py:95-121` `get_paths()` — calls `nx.all_simple_paths` for every (active stressor × every terminal node) pair; since `all_simple_paths` already yields nothing for unreachable targets, the terminal-node loop is unnecessary work, and the `except nx.NetworkXNoPath` (`:116`) is dead — `all_simple_paths` does not raise it. Fix: iterate targets only where needed, or compute reachable descendants once; drop the dead except.
- `poe.py:166` / `test.py:79` `plt.show()` — blocks on an interactive window and offers no save-to-file path, so the tool cannot run headless/automated. Fix: add a `--save <path>` option using `plt.savefig`.
- `pathways.xlsx` `Nodes_Data` — display-string typos that will surface in regulatory output even though they no longer break connectivity (edges use IDs): `N000` `"Use of Machinery **of** Land"` (should be "on"), `N005` `"deli**te**rious"` (deleterious), `N008` `"Altered external nutrients / energy input"` differs from `stressors.json`'s `"…energy inputs"` (spaces around the slash **and** singular vs plural). Fix: proofread node names against the DFO PoE source wording.
- `pathways.xlsx` is a scaffold/WIP — `Instructions` and `Analysis_Helpers` sheets are empty; `Lists_Data` holds placeholder *descriptions* of what the Instructions sheet should contain (overview, how-to, node-type definitions, support contact). Fine as in-progress, but note it is not a finished deliverable.
- Binary `pathways.xlsx` (182 KB) committed to git — acceptable as the data source, but diffs are opaque; consider also exporting `Nodes_Data`/`Edges_Data` to CSV for reviewable history.

## Could not verify

- **Intent of `pathways.xlsx`**: whether it is meant to *replace* the Python CLI as a standalone interactive workbook (it has a `Dashboard` with node-selection data validation) or to *feed* the Python tool as a data source. This determines what the migration should target.
- **Intended scope of mitigation**: whether mitigation should be shown for primary activities or for the effect nodes along discovered pathways. The file naming points to effect nodes; the code queries primary activities. Confirm the design before fixing the Major mitigation bug.
- **Existence of the risk-scoring tool version** implied by `output/2_report.txt` — it is not in the repo; confirm whether it exists elsewhere (possible lost/uncommitted work).