[joannawang-100]

Summary

Adds an optional reach-indexed routing output contract for probes and models that need to evaluate river-network behaviour separately for each reach.

This is the contract-first split requested in the review of #118. It does not add the routing-network-closure criteria, reference models, Wflow extension, or probe gate.

Contract

• Adds optional requires.routing support to probe specifications.
• Adds optional emits.routing support to model manifests.
• Defines the supported routing variables as reach_q_in, q_out, and channel_storage.
• Exposes output/routing.csv through request.json.
• Stores routing output separately from the one-row-per-timestep result.csv.
• Carries the validated routing table on RunResult.routing.
• Requests routing output only on probes that supply routing_network.
• Makes verify-adapter prefer a routing-capable probe for models that declare emits.routing.

routing.csv is a long table with the columns time, reach_id, reach_q_in, q_out, and channel_storage. static.json.routing_network.reaches is defined as a non-empty list of unique, non-empty string IDs. Reach objects are not accepted, and leading zeros in IDs are preserved.

It contains one row for every forcing time and every reach listed under static.json at routing_network.reaches. reach_q_in and q_out are interval-mean rates in m3/s, while channel_storage is absolute end-of-step reach storage in m3.

Existing probes and models remain compatible because both routing declarations are optional and default to empty.

Validation

The protocol checks:

• required routing columns;
• routing probes must declare requires.static: [routing_network];
• routing-network reach IDs must be non-empty strings and unique;
• reach IDs with leading zeros are preserved;
• finite numeric values;
• unique (time, reach_id) pairs;
• exact agreement with the declared reach IDs;
• one row per forcing time and reach, including spinup;
• per-reach time axes matching forcing.csv;
• output size and file allow-list compliance.

ht verify-adapter prefers a routing-capable probe for models that declare emits.routing; on that probe, every declared routing variable must be present in a valid routing.csv.

Tests

Adds routing contract tests covering:

• probe and model declaration loading;
• rejection of unknown routing variables;
• request paths and units;
• valid routing-table round trips;
• missing files and columns;
• duplicate rows;
• missing reaches;
• incorrect time axes;
• non-finite values;
• verify-adapter enforcement.

Follow-up

After this contract is reviewed and merged, I will rebase the draft probe PR #118 and add the routing criteria, reference models, Wflow physical evidence, archived result rows, and merge-checklist updates.

Related to #108.

[kawh1111]

Reviewed at 27fe3fb9. I pulled the head, unpacked it locally and ran the suite on it, then read the contract layer end to end — protocol.py, spec.py, both schemas, the harness.py hook and the new test file.

The design is right and I have no argument with it. There is one thing that has to change before this can be relied on, and it is not in the shipped code.

The new test fails on pandas 3, and CI has not run on this PR

Locally, on this head:

ht validate   ->  validation passed: 29 probe(s), 64 model(s)          (exit 0)
ht gate       ->  GATE PASSED: every probe separates the reference
                  models as declared.                                  (exit 0)
pytest -q     ->  1 failed, 1096 passed, 10 skipped in 481.37s         (exit 1)

The failure is

tests/test_routing_protocol.py::test_routing_values_must_be_finite[not-a-number]
tests/test_routing_protocol.py:209: table.loc[0, "q_in"] = bad_value
TypeError: Invalid value 'not-a-number' for dtype 'float64'

That is the test's own setup line, not the protocol. _routing_table builds q_in as float64, and on pandas 3 assigning a string into a float column raises instead of upcasting to object. The two sibling parameters in the same parametrize are fine — nan and inf are float values — so only the "not-a-number" case is affected. Reduced to one line, outside the suite:

df = pd.DataFrame({"q_in": [2.0, 3.0]})
df.loc[0, "q_in"] = "not-a-number"      # TypeError on pandas 3.0.5
df.loc[0, "q_in"] = float("nan")        # fine

The shipped path is unaffected: _read_routing does pd.to_numeric(table[var], errors="coerce"), which returns [2.0, nan, 3.0] for the same input, so the behaviour the test means to check is correct. It is the test that cannot reach it.

Why this is not a local quirk. pyproject.toml declares pandas>=1.5 with no upper bound, and .github/workflows/probe-pr.yml installs pip install -e '.[dev]' on python-version: '3.11' and then runs pytest -q. The installed pandas here is 3.0.5, whose metadata says Requires-Python: >=3.11. So the CI job's unpinned install resolves to pandas 3 and this test fails there too — I have not been able to run the job myself, so I am flagging it as a prediction with its reasoning rather than as an observation.

And nobody has seen it, because the job has not run. GET /commits/27fe3fb9/check-runs returns an empty list, and the probe workflow run for this head is #207, completed / action_required — parked for approval rather than executed. Other contributors' runs on the same day completed normally (#205 on bccf21b8, #206 on 27c4c84d).

This is not specific to this branch: the same author's earlier pull request #118 also sits at action_required (run #202, on head 0363e00e). So it follows the account rather than the change, and it means neither branch has had its pytest run by CI yet.

Fix. Give the column room for the bad token before writing it — for example

table = _routing_table(case)
table["q_in"] = table["q_in"].astype(object)
table.loc[0, "q_in"] = bad_value

or write the malformed value through the CSV instead of through .loc, which is closer to what an adapter actually produces. A pandas<3 pin in pyproject.toml would also silence it, but that trades a one-line test fix for a dependency ceiling on the whole project, and I would not do that here.

What holds

• Both declarations are genuinely optional and default to empty. test_routing_declarations_load_and_old_defaults_remain_empty pins base_probe.requires_routing == () and reference_bucket.emits_routing == (), so nothing on main changes shape, and ht validate still reports 29 probes / 64 models.
• The acceptance gate is untouched: ht gate passes with every must_pass model passing and every must_fail model tripping exactly the criteria it declares. A contract change that moved a probe's separation would have shown up here and did not.
• The new output file sits inside the existing safety checks rather than beside them: ROUTING_CSV is in the _validate_output_files allow-list and is counted by the same loop, so a routing table is subject to the same regular-file and max_output_mb rules as result.csv.
• verify-adapter is bound to the model's own declaration (harness.py, requires_routing=model.emits_routing), which is the right choice: a model that declares emits.routing and then writes nothing fails the smoke test even when the probe in hand does not ask for routing. test_verify_adapter_checks_declared_routing exercises exactly that.
• The reader is reachable by criteria. evaluate_criteria hands a non-paired criterion the control RunResult, so result.routing is available to the criteria #118 will add. The contract is not a dead end.
• The AGENTS.md prose is unusually complete for a contract change: the /io table, a worked routing.csv example, the (time, reach_id) uniqueness rule, the per-reach time-axis rule, and the instruction not to construct q_in from a budget residual.

Findings, in order of what I would fix first

1. The three new AGENTS.md rows are not enforced by anything

tests/test_docs_in_sync.py exists for exactly this pairing — its docstring says it turns the AGENTS.md checklist into something the workflow enforces — and its test_agents_doc_defines_every_variable asserts that a variable spec.py accepts has a row in the AGENTS.md table. But its import line is still

from hydroturing.spec import DIAG_VARS, FLUX_VARS, REPO_ROOT, STATE_VARS, load_model, load_probe

and the parametrization is still FLUX_VARS + STATE_VARS + DIAG_VARS. ROUTING_VARS was added to spec.py by this PR and appears in neither, so the q_in, q_out and channel_storage rows are carried by hand.

Measured: I deleted all three rows from AGENTS.md and ran tests/test_docs_in_sync.py — 203 passed in 1.43s, exit 0. The rows can go missing and nothing goes red, which is the drift that test was written to stop.

Fix: add ROUTING_VARS to the import and to the parametrization. One line each.

2. request.json's notes loses a space, so every adapter reads check.Routing

protocol.py builds notes by implicit string concatenation. Line 155 ends "...interval-mean boundary fluxes for the soil-storage check." with no trailing space and line 156 opens "Routing q_in and q_out are interval-mean m3 s-1; ". Generated on this head:

...interval-mean boundary fluxes for the soil-storage check.Routing q_in and q_out are interval-mean m3 s-1; channel_storage is absolute end-of-step m3.

No test asserts on notes, so the suite cannot see it, and it is the one string every adapter author reads. Fix: one trailing space on line 155.

3. docs/adapting-a-model.md was not updated

This PR adds a fourth emits group and a fourth /io output file, and docs/adapting-a-model.md is the guide that describes both. Section 1 is "Decide what your model honestly emits"; line 40 reads "Outputs that are not a flux or a storage have an optional diagnostics group." — an enumeration this PR extends to four; line 82 tells the adapter to "Read /io/request.json, read the forcing, call your model, write the table."

Nothing enforces it. tests/test_docs_in_sync.py does not read that file, and AGENTS.md's merge checklist names only the six prose places that test covers, so a contributor following the adapter guide will not learn that routing exists or that a second output table is possible.

Fix: a paragraph next to the diagnostics example, and a line in the /io walk-through.

4. Three branches of _read_routing are never executed by the new tests

The new file covers the happy path and five malformed shapes well. A line trace over tests/test_routing_protocol.py, restricted to protocol.py:216-303, leaves 28 executable lines unexecuted, of which these three are whole branches:

lines	branch	why the tests cannot reach it
252-254	routing output requires static.json routing_network.reaches	every fixture case carries routing_network; none is built without it
259-261	static.json routing_network.reaches contains duplicates	no test declares duplicate reach IDs
272-273	expected_reaches read from the table when nothing is declared	every test that writes a table also declares reaches

The first is the one worth pinning. It is the error a probe author will actually meet — a probe declaring requires.routing whose generate.py never writes routing_network.reaches gets this message — and nothing in the suite proves the message is produced at all.

5. Two sentences this PR adds to AGENTS.md are statements nothing checks

Not blocking and I would not hold the merge for them, but they are new claims and should be known to be unenforced:

• "Variables declared under emits.routing belong in routing.csv, not in result.csv" — read_result only checks that the probe's required columns are present in result.csv; it never rejects extra ones, so a model writing q_in into both tables is not caught.
• "Declaring them means the adapter must write the requested long table" — true only through verify-adapter, and only when someone runs it. ht run on a probe that does not require routing will not notice.

One thing that is not a problem, with the number

max_output_mb is now shared between result.csv and routing.csv, so a routing probe has less headroom for its own table. At the scale #118 will use this does not bind: 455 forcing steps over 3 reaches is 1365 rows, and at roughly 29 bytes a row that is 39,626 bytes — 38.7 KB against the 2.0 MB most probes declare and the 5.0 MB default. A sentence in the probe README when #118 lands is enough; no change here.

Review status

I have not approved. Findings 1 and 2 are one-line changes inside the contract layer this PR exists to settle, so I would rather see them in than filed as follow-ups. Finding 3 is the same class and cheap. The unrun CI and the pandas-3 failure are the pair I would resolve first, since a contributor who clones this branch and runs pytest today gets a red suite.

Nothing here questions the design: optional on both sides, a long table keyed by reach, the same validation shape as the result table, and verify-adapter bound to the model's own declaration is the right call for a contract that changes what every adapter can report.

[chrimerss]

/review

[github-actions]

Automated review by Claude (changes-suggested), not a maintainer approval.

This is the contract-only split out of #118: an optional requires.routing on probes and emits.routing on models, three new reach-indexed variables (q_in, q_out, channel_storage), a new output/routing.csv in the /io contract, validation of that long table in protocol.py, and RunResult.routing. Both declarations default to empty, so existing probes and models are untouched, and the new test file covers the loader, the request, and most of the table validation.

The most important thing: as written, declaring emits.routing makes a model fail ht verify-adapter, and therefore CI, unconditionally (harness.py:439). The smoke probe inherits requires_routing from the model, but the case comes from whichever probe the smoke test picked — in practice mass/catchment-closure, which puts no routing_network in static.json. _read_routing then raises either "routing output requires static.json routing_network.reaches" or "adapter did not write required output/routing.csv", cmd_verify_adapter exits 2, and model-eval.yml fails the workflow. The follow-up probe PR does not fix this, because the closure probe is still tried first. The new test asserts a raise here, so it locks the behaviour in rather than catching it.

Related: the contract never says what routing_network looks like. _read_routing assumes a list of scalar IDs (str(reach)), which is the one shape a real routing probe — needing topology per reach — is least likely to use. Worth settling in this PR, since defining the contract is the whole point of it.

---

tests/test_docs_in_sync.py:200 low: ROUTING_VARS is not covered by the AGENTS.md variable-table test

test_agents_doc_defines_every_variable is parametrized over FLUX_VARS + STATE_VARS + DIAG_VARS. The PR adds ROUTING_VARS to spec.py and the matching rows to AGENTS.md, but not to this parametrization, so the enforcement AGENTS.md:352 describes ("This file, if the probe introduced a variable: a row in the table above") does not apply to routing variables — the next one can be added to spec.py with no row in AGENTS.md and the suite stays green.

Suggested fix: import ROUTING_VARS and add it to the parametrize list.

---

src/hydroturing/protocol.py:183 low: The 5 MB output cap is not sized for a reach-indexed table

max_output_mb defaults to 5.0 (spec.py:509) and _validate_output_files now sums routing.csv into that total. A routing probe at PT1H over a year with 60 reaches is 8760 x 60 = 525,600 rows; at roughly 45 bytes a row that is ~24 MB, and a correct adapter is failed with adapter output is 24.00 MB, exceeding the 5 MB limit — a message that reads as adapter misbehaviour.

Since this PR defines the contract the probe will be written against, note in AGENTS.md that a probe requesting routing must raise case.max_output_mb to cover n_steps x n_reaches rows, so the follow-up probe does not discover this by failing its own gate.

_{Posted by the pr-review workflow. Run}

[joannawang-100]

@kawh1111 Thank you for pulling the branch and running the full suite. I have addressed the requested changes:

• fixed the pandas 3 test setup by converting q_in to object dtype before inserting the malformed string;
• added ROUTING_VARS to the AGENTS.md variable-table synchronization test;
• fixed the missing separator in the generated request.json notes;
• updated docs/adapting-a-model.md with the routing declaration and output workflow;
• added tests for missing, duplicate, and malformed reach declarations;
• defined routing_network.reaches as unique, non-empty string IDs and preserved leading-zero IDs;
• required routing probes to declare requires.static: [routing_network];
• documented routing-table output sizing through case.max_output_mb.

I also addressed the automated review's verify-adapter concern without dropping routing verification: ordinary probes no longer request routing output, while models declaring emits.routing preferentially use a routing-capable probe for contract verification.

CI has still not run on this PR (Checks: 0 / the workflow is awaiting approval), so I am not claiming a green suite yet. Could a maintainer approve the workflow run? I will address any failures it reports.

[joannawang-100]

@kawh1111 Thank you for pulling the updated branch, running the full suite, and re-checking the earlier fixes. I appreciate the approval and the clarification about the routing-aware verify-adapter path. Once #130 is merged, I will update and complete the routing probe in #118 against the merged contract.

[chrimerss]

Reviewed at 272bf07. The head is based on current main (1aff225, 0 behind), so it fast-forwards and the merged tree is the head tree. It also merges cleanly with #146 at c267865. Fork CI has still not run; everything below was run locally, from a scratch venv on Python 3.12.

Evidence

	head 272bf07 (= merge with main)	main 1aff225
pytest -q, pandas 3.0.6	1358 passed, 2 skipped	1332 passed, 2 skipped
pytest -q, pandas 2.3.3	1358 passed, 2 skipped	—
ht validate	passed: 37 probes, 84 models	same
ht gate	GATE PASSED, 37 probes, 194 checks	same, log identical

• The 26 extra tests are the 23 in tests/test_routing_protocol.py plus 3 new ROUTING_VARS cases in the docs-sync test. Run against main's src/, all 23 fail or error, so they do test the change.
• ht verify-adapter passes for all 77 subprocess models in models/ on both trees, with identical output.
• ht run --gate-seeds gives identical rows on head and main for flex_lumped, sacsma_snow17 and reference_bucket (37 each). The flex_lumped and sacsma_snow17 rows also match the archived ones field for field. models/result.csv is untouched, and no row can move. For a model without emits.routing, the only change is two new keys in request.json (request.routing: [] and output.routing) and one sentence in notes. No adapter in models/, Docker ones included, reads those fields.
• None of the new names collides with an existing one. q_out, channel_storage, reach_q_in, reach_id and routing_network appear nowhere on main or in #146, and q_in is still only the forcing. The units are stated, and they agree with dis: m3 s-1 for the two discharges and m3 for storage.
• I wrote malformed routing.csv files. Each of these is rejected with a clear ProtocolError: a missing reach, an extra reach, a missing or extra (time, reach) row, a duplicate pair, an empty reach_id cell, NaN/inf/"1e400"/text in a value, a ragged row, a header-only file, rows out of time order, and a missing requested column. An extra column is accepted and kept, which matches how result.csv treats undeclared columns. A 0-byte file surfaces as pandas' EmptyDataError and becomes an ERROR. result.csv behaves the same way on main, so that is not new.

The rename and kawh1111's points are all in. The shipped path cannot affect any existing model or row. Three things still need fixing before #118 builds on this, and none of them is large.

Findings

1. medium: a model that follows the documented emits.routing example is N/A (INCOMPATIBLE) on every routing probe (AGENTS.md:314, docs/adapting-a-model.md:74, src/hydroturing/spec.py:623)

load_probe forces every routing probe to carry requires.static: [routing_network]. run_probe (harness.py:422-430) then requires the model to list routing_network in needs_static or uses_static. Neither example says so, and neither paragraph around them does. I ran reference_bucket with emits.routing: [reach_q_in, q_out, channel_storage] and a wrapper that writes a correct three-reach routing.csv, on a routing variant of mass/catchment-closure:

ht run path      -> N/A INCOMPATIBLE ['model does not declare that it consumes static routing_network']
verify-adapter   -> OK, 12045 routing rows read
+ uses_static: [routing_network] -> PASS

So verify-adapter passes the model, and ht run then refuses it. Fix: add needs_static: [routing_network] to both examples. Better, have load_model raise a SpecError when emits.routing is non-empty and routing_network is in neither list, mirroring the probe-side check at spec.py:527, and add a test for it.

2. medium: a routing.csv that was not requested turns an ordinary probe into ERROR (src/hydroturing/protocol.py:237)

_read_routing parses the file whenever it exists, including when probe.requires_routing is empty. A model with its own river network (wflow, LISFLOOD, CWatM) may write it on every run. That is what an adapter does if it writes every declared output, the habit the old request comment encouraged. Every non-routing probe then fails. Same wrapper as above, but always writing the file:

mass/catchment-closure (ht run) -> FAIL  ERROR  routing output requires static.json routing_network.reaches
verify-adapter on closure      -> exit 2, same message

The message blames the case, and an ERROR counts against the model on all 37 current probes. Unrequested result.csv columns are ignored, and this file can be treated the same way. Fix: put if not probe.requires_routing: return None at the top of _read_routing. _validate_output_files still applies the size and regular-file checks. With that change, both cases above are accepted, and tests/test_routing_protocol.py plus the docs-sync tests still pass (279) on pandas 3 and pandas 2. Add a test that writes the file on a non-routing probe.

3. medium: the reach budget's terms are not pinned down (AGENTS.md:220-222)

reach_q_in is "inflow to the identified reach". That does not say whether lateral inflow along the reach (local runoff, baseflow, a prescribed q_in boundary) is included, or only the upstream inflow. Nor does it give a sign: positive into the reach for reach_q_in, and positive downstream for q_out, which can also be negative under backwater. channel_storage does not say whether floodplain or overbank water counts. #118's README at 0363e00 excludes it, but the contract dropped that sentence. Here is a case that goes wrong. A model's river cell receives lateral inflow L. One adapter reports reach_q_in as upstream inflow only, so its reach residual (reach_q_in - q_out)·dt - ΔV is -L·dt on every wet step. Another adapter reports upstream plus lateral inflow and closes. The same model gets two verdicts from two readings of one sentence. #118's case has no rain, so it will not bite there. It will bite the first routing probe that has rain. Fix: define reach_q_in as the total inflow into the reach's control volume (upstream, lateral and prescribed), positive into the reach, and q_out as the outflow at its downstream end, positive downstream. Say which stores channel_storage covers. Alternatively, add a separate lateral-inflow variable now, while the names are still free.

4. low: reach IDs such as NA, None, null or nan are read as missing (src/hydroturing/protocol.py:242)

These IDs pass the static.json check, because they are non-empty strings. pd.read_csv(path, dtype={REACH_COL: str}) still applies pandas' default NA strings, so seven of them (NA, N/A, n/a, nan, None, null, NULL) come back as NaN. The result is "routing result has missing 'reach_id' values" from a correct table. Fix: pd.read_csv(path, dtype={REACH_COL: str}, keep_default_na=False). The value columns still go through pd.to_numeric(errors="coerce"), so blanks and nan there are still rejected as non-finite. An empty reach_id cell is then caught by the reach-set comparison (extra=['']). I checked all of this on both pandas versions.

5. low: a malformed network is charged to the adapter, after it has run (src/hydroturing/protocol.py:261)

The routing_network shape checks (present, a non-empty list, string IDs, unique) run inside read_result and raise ProtocolError, the adapter's error. Take a probe whose generator writes "reaches": [1, 2]. Every routing model runs the full case and is then archived as ERROR. Fix: validate case.static["routing_network"] in stage(), or wherever the case is built, when probe.requires_routing is set, and raise it as a probe error before the adapter is invoked.

6. low: ht list and the probe template don't show routing (src/hydroturing/cli.py:173, templates/probe.template.yaml:55)

ht list prints required_vars and emitted, and neither includes routing. A routing probe's needs: column is blank for its routing variables, and a routing model's emits: column leaves them out. docs/writing-a-probe.md and the probe template don't mention requires.routing either, so a probe author finds it only in AGENTS.md.

Not findings

• The per-reach loop in _read_routing is O(reaches × rows). It takes 0.4 s for 50 reaches × 4015 steps, 2.4 s for 500 × 395, and 22.5 s for 500 × 4015. That is fine at #118's three reaches, and worth a groupby if a distributed model ever reports hundreds of reaches.
• Negative q_out or channel_storage passes the contract layer, as negative states do in result.csv. Rejecting them is the criteria's job.

Of these, 1 and 2 are a few lines each and should land with this PR. 3 is a decision about the contract, and it is best settled before #118 adopts the names.

[joannawang-100]

@chrimerss Thanks, I addressed all six findings at 6cc4db9.

1. Models declaring emits.routing must now also declare routing_network in needs_static or uses_static. load_model enforces this, both model examples include it, and a regression test covers the invalid manifest.

2. _read_routing now returns immediately when the probe does not request routing. An unrequested routing.csv is ignored semantically while remaining subject to the existing file-safety and size checks. A regression test covers this path.

3. The routing terms are now pinned down consistently in AGENTS.md, the request notes, both schemas, and the adapter guide:

   • reach_q_in is the interval-mean total inflow into the reach control volume, including upstream, lateral, and prescribed inflows, positive into the reach.
   • q_out is interval-mean flow across the downstream boundary, positive downstream and negative for reverse or backwater flow.
   • channel_storage is absolute end-of-step channel water volume and excludes land-surface, soil, groundwater, floodplain, and overland storage.

4. routing.csv is now read with keep_default_na=False. A parameterized regression test covers NA, N/A, n/a, nan, None, null, and NULL, in addition to the existing leading-zero test.

5. Routing-network validation is factored into _routing_reaches and runs at the beginning of stage(), before input/output directories or request.json are created. Malformed networks raise ValueError as a probe/case error rather than ProtocolError after the adapter runs. _read_routing reuses the same validation defensively.

6. ht list now includes routing variables in probe needs: and model emits: output. The probe template exposes requires.routing, and docs/writing-a-probe.md documents the routing declaration, required network structure, and long-table output.

Validation on a clean snapshot with Python 3.12 and pandas 3.0.6:

pytest -q
1360 passed, 10 skipped

pytest -q tests/test_routing_protocol.py
33 passed

ht validate
validation passed: 37 probe(s), 84 model(s), suite 0.1.0

ht gate
GATE PASSED: every probe separates the reference models as declared.

The ten skipped tests require optional dependencies not installed in the base + dev test environment. GitHub still reports no status contexts for this head, so the evidence above is local.

Could you please re-review the updated head?

[kawh1111]

Reviewed at 6cc4db9. I re-ran the head locally and checked each of the six findings from the previous review rather than only the tests that claim to cover them.

Evidence

ht validate -> validation passed: 37 probe(s), 84 model(s)        (exit 0)
pytest -q   -> 1360 passed, 10 skipped                           (exit 0)
ht gate     -> GATE PASSED: every probe separates the reference
               models as declared.                               (exit 0)

These match the numbers in your comment. The PR is twelve files, +806/−24, all contract, documentation or tests; no criterion, probe, model manifest or archive row moves. CI is still action_required on this head, so the evidence is local.

The six findings

1. Fixed. load_model now rejects emits.routing without routing_network in needs_static/uses_static (spec.py:627), both examples carry needs_static: [routing_network], and test_routing_model_must_declare_network covers the invalid manifest. The verify-adapter-passes-then-ht run-refuses mismatch is gone.
2. Fixed. _read_routing returns early when probe.requires_routing is empty (protocol.py:272); an unrequested routing.csv is ignored semantically while _validate_output_files still applies the size and regular-file rules. test_unrequested_routing_output_is_ignored covers it.
3. Fixed. AGENTS.md, both schema descriptions and docs/adapting-a-model.md now define reach_q_in as the total inflow into the reach control volume (upstream, lateral and prescribed), q_out as the downstream-boundary flow, positive downstream and negative under backwater, and channel_storage as the routed channel volume excluding floodplain and overland storage. That is the reading #118 needs.
4. Fixed. pd.read_csv(..., keep_default_na=False) preserves NA, None, null and the rest as reach IDs; the parameterized test covers all seven tokens plus leading zeros.
5. Fixed in the part that matters, with one attribution left. The network is validated in stage() before request.json or any adapter run, and test_stage_rejects_malformed_network pins that. But stage() is called from Runner.run, inside run_probe's broad except Exception, so the ValueError is still recorded as FAIL (ERROR) against the model rather than as a probe/case N/A. That is the same failure mode the finding named, minus the adapter invocation. If you want the verdict to name the probe, the check has to move into run_probe's preparation block or be caught separately; it is not needed for #118, whose network is well formed.
6. Fixed. ht list now includes requires_routing in a probe's needs: and emits_routing in a model's emits:; the template exposes requires.routing, and docs/writing-a-probe.md documents the declaration, the network shape and the long-table output.

What I checked beyond the findings

• The early return does not weaken the requested path: a routing probe still gets the full table checks (columns, unique (time, reach_id), exact reach set, n_steps × n_reaches, per-reach time axis, finite values).
• keep_default_na=False does not let non-finite values through: the value columns still go through pd.to_numeric(..., errors="coerce") and are rejected.
• The new load_model rule is not retroactive trouble: no model on main declares emits.routing.

Approving. The only remaining item is the error attribution in finding 5, which is low and does not affect a well-formed network.

[chrimerss]

Re-reviewed at 6cc4db9. It is still based on main 1aff225, and nothing is behind. All six findings from my review at 272bf07 are fixed, and the fixes match what I asked for. Thank you for turning them around so quickly.

Checked locally (Python 3.12, scratch venv)

	6cc4db9
pytest -q, pandas 3.0.6	1368 passed, 2 skipped
pytest -q, pandas 2.3.3	1368 passed, 2 skipped
ht validate	passed: 37 probes, 84 models
ht gate	GATE PASSED; log identical to main's
ht verify-adapter, all 77 subprocess models	exit 0, output identical to main's
ht run --gate-seeds, flex_lumped / sacsma_snow17 / reference_bucket	rows identical to main's
Docker smoke, modflow6 + wflow_sbm verify-adapter	OK

I ran tests/test_routing_protocol.py from this head against the 272bf07 code: 14 of the new tests fail there and pass here. Each fix has a test that fails without it.

Each finding, end to end

1. A manifest with emits.routing and no routing_network is now refused by load_model with a clear SpecError. With uses_static: [routing_network], the same model, writing a three-reach table, PASSes a routing variant of mass/catchment-closure. verify-adapter reads all 12045 routing rows.
2. An adapter that writes routing.csv when it was not asked to now PASSes mass/catchment-closure (it was ERROR). verify-adapter returns routing=None.
3. The terms are defined in AGENTS.md, both schemas, the request notes and the adapter guide, and all of them say the same thing.
4. NA, N/A, n/a, nan, None, null and NULL are kept as reach IDs. An empty reach_id cell is still rejected (extra=['']).
5. A malformed network ([1, 2], [], ["A", "A"], or no routing_network at all) now fails in stage() in 0.0 s, before the adapter runs. The archived detail is the plain message ("static.json routing_network.reaches must be a non-empty list of non-empty string IDs"). That matches how a generator failure is already reported.
6. ht list, the probe template and docs/writing-a-probe.md now show routing.

Non-blocking, for #118 rather than this PR

reach_q_in now counts every inflow, and channel_storage excludes floodplain, overland and groundwater water. The contract still doesn't say where channel losses go: open-water evaporation, seepage to the aquifer, spill onto a floodplain. A reach that loses water to one of those would show the loss as a residual. #118's case zeros all of them, so it does not bite there. Before a probe with any of those terms is scored, one sentence settles it. Either net them into reach_q_in's lateral term (negative when water leaves the channel), or add a reach-level loss variable.

[joannawang-100]

Thank you, @chrimerss and @kawh1111, for the thorough re-reviews, independent validation, approvals, and merge.

I have noted the remaining error-attribution distinction raised by @kawh1111. Since malformed networks are now rejected before the adapter runs and the current reporting is consistent with the existing generator-failure path, I will treat any further attribution change as a separate follow-up rather than part of #118.

Now that this contract has merged, I will rebase #118 onto the updated main and continue its implementation. I will make explicit that the current case sets open-water evaporation, aquifer seepage, and floodplain spill to zero, so no additional loss variable is required for this case. Before a future routing probe scores nonzero channel losses, we can separately decide whether to treat them as negative lateral contributions to reach_q_in or introduce a reach-level loss variable.

Thanks again for the detailed reviews and for merging the contract!