feat(bngir): round-trip units, count observables, and builtin rate laws; fail closed on the rest

[akutuva21]

What changed

python/bionetgen/bngir.py (versioned, source-free BNG IR) plus its two test
files. Six reader/writer gaps found by roundtripping hand-built models through
to_bngir/from_bngir before any edit (AGENTS.md reproduce-first); each
expansion is pinned by a semantic-equality regression test, each non-expansion
fails closed with a named ValueError.

Also adds a writer/reader closure: to_bngir(version="0.2") validates its
own output with the reader's validation (_load_document_v02) before returning,
so every read gate is automatically a write gate and the two lists cannot
drift apart.

Gap -> expanded / refused

#	Gap found (measured)	Resolution	Test that proves it
1	v0.2 builtin_call rendered wire names verbatim: arrhenius(...), saturation(...), hill(...) are not BNGL spellings; restored model has undefined functions and cannot even be re-serialized (RuntimeError: cannot create structural semantic snapshot from invalid model)	Expanded: _BUILTIN_NAMES maps to Arrhenius/Sat/Hill (verified against BNGLexer.g4 spellings); unknown builtins refused by a whitelist that exactly matches C++ builtinName() (38 names, excludes "unknown")	test_bngir_v02_round_trip_preserves_builtin_rate_laws, test_v02_builtin_calls_render_bngl_keyword_spellings, test_v02_refuses_unknown_builtin_calls
2	v0.2 Species R2 R==2 re-emitted as R()==2, which does not parse (ParseError) - BNGL only spells == >= < <= after a bare molecule name	Expanded: bare-molecule rendering for non-> relations; sited/multi-molecule patterns with non-> relations refused - the grammar cannot spell them	test_bngir_v02_round_trip_preserves_observable_count_relations (includes sited > terms like R(l,l).A()>1, which take the pattern branch), test_v02_observable_count_relations_render_bare_molecule_patterns, test_v02_refuses_sited_count_relation_patterns
3	v0.2 reader silently dropped unit metadata (unit_defaults, unit_definitions, [unit] on parameters/compartments/seeds): roundtrip lost units; v0.1 dropped them at write with semantic_equal blind to the loss	Expanded (v0.2): emit begin units block + [unit] annotations, mirroring BnglWriter. Refused (v0.1): the 0.1 wire/schema has no unit fields and expanding the published schema is outside this lane - named write refusal physical-unit metadata; use version '0.2', surfaced by semantic_equal too (docstring updated)	test_bngir_v02_round_trip_preserves_unit_metadata (asserts native unit_defaults, parameter/compartment/seed .unit on the reconstructed model, not just doc equality), test_bngir_v01_refuses_unit_metadata_instead_of_dropping_it, test_v02_renders_unit_defaults_definitions_and_annotations, test_v02_refuses_unsupported_unit_default_roles, test_v02_refuses_builtin_unit_definitions
4	Counter Ca A() serializes as kind "unknown" and the reader mutated it to Species (silent corruption)	Refused: write-side named refusal (message points to 0.1, which preserves Counter verbatim); read-side named refusal for kind not in {molecules, species}. Unfixable in 0.2 without a semantic decision that is not mine: the compiled snapshot does not record which non-Molecules/Species kind it was, so rendering anything would be fabrication - that decision (extend CompiledModel observable kinds) belongs to the C++ lane	test_bngir_v02_refuses_counter_observables_and_v01_preserves_them, test_bngir_v02_rejects_unknown_observable_kind
5	v0.2 barrier reader expected the v0.1 field shape (reactants/products/bidirectional) which never exists on the wire -> 0 -> 0 + misleading "expression must be an object"; wire transition is BarrierPattern::toString() (label + arrow + energy)	Expanded: render transition alone (exactly once - count-pinned). Refused: unresolved reaction centers, missing transition/energy	test_v02_barrier_patterns_render_from_transition_text (source.count("Gbar") == 1, source.count("slow:") == 1), test_v02_refuses_unresolved_barrier_centers
6	Reader accepted-with-mutation: reverse direction on a non-bidirectional rule silently dropped; unknown modifier kinds silently dropped; include_* modifier without its direction.filters entry silently dropped	Refused: three named diagnostics; closure test proves writer output always satisfies the pairing	test_bngir_v02_rejects_reverse_direction_on_irreversible_rule, test_bngir_v02_rejects_unrepresentable_rule_modifiers, test_bngir_v02_round_trip_preserves_rule_modifiers
7	v0.1 population_maps: writer emits the section, reader refuses (pre-existing, documented in docs/adr/0001)	Kept refused: the map's mapping rate is only in C++ PopulationMap::rateText (cpp/ast/PopulationMap.hpp:31), which the Python binding does not expose - cpp/bindings/bind_model.cpp:162-166 binds only label/pattern/function/args - so expanding needs a binding outside this lane	existing test_bngir_population_reconstruction_fails_closed (unchanged)

Writer-guard accounting (per review)

Read gate	Status
metadata.unit_definitions builtin	writer-guarded (closure); not observed from writer (snapshot carries authored defs, builtin: false on every fixture)
observable kind	writer-guarded: explicit _payload_v02 guard + closure, tested
observable count relation	writer-guarded (closure); writer-emitted shapes roundtrip-tested
modifier/filter pairing	writer-guarded (closure) + MODIFIER_MODEL roundtrip
reverse on non-bidirectional rule	writer-guarded (closure)
barrier center_resolved	writer-guarded (closure); fixture unconstructible from Python - proof below

Findings that outlive this change (candidates for docs, owner docsHygiene)

1. parse_string/parse_file never call finalizeThermodynamicMetadata() -
   contrast: cpp/parser/BNGAstVisitor.cpp:2039 calls it inside parseModel,
   while the Python path's do_parse (cpp/bindings/bind_parser.cpp:25)
   runs visitor.visit + takeModel() with no finalize step. Measured consequences through the
   Python API (command: parse a barrier/driven model, inspect the native
   model): driven_by() is stripped by the source normalizer and its work
   silently lost (has_driving_work prints False, pending options never
   flushed); native.barrier_patterns prints 0 [] and the barrier leaks as
   a __bng3_barrier_N-labelled ordinary rule; ordinary rules are not renamed
   R<n>. This is upstream of BNGIR and outside this lane (escalated via
   orchScope). It is also why the barrier read gate is currently
   defence-in-depth against a path no Python caller can reach - the gate exists
   for whoever wires finalizeThermodynamicMetadata() up.
2. ObservableKind::Unknown is reachable from real models: Counter
   observables compile to kind "unknown" (measured), which is what made the
   gap-4 mutation possible.
3. Published schema divergences (pre-existing; provenance/schemas is
   read-only in this lane): bngir-0.2.schema.json's metadata def lacks
   unit_defaults/unit_definitions, so unit-carrying writer output does not
   validate against the schema (schema tests only run on unit-free fixtures);
   and population_maps items require function while the C++ writer emits
   population/rate. Both need schema updates by whoever owns them.

Verification (worktree /Users/akutuva/Documents/BioNetGen/BNG3-py-bngir, commit 92c24e7)

Harness provenance - on this host bionetgen is a scikit-build editable
install whose meta-path finder shadows sys.path (and PYTHONPATH), so a
plain python -m pytest tests/python -q in any worktree silently tests
main's code. All suite numbers below were produced with a runner that
inserts a finder claiming exactly bionetgen.bngir from this worktree and
leaves the rest of the editable chain intact, then prints the binding:

UNDER TEST: /Users/akutuva/Documents/BioNetGen/BNG3-py-bngir/python/bionetgen/bngir.py

This lane is pure Python (git status: only bngir.py + the two test files
change); the C++ extension comes from the shared build, which memWatch
bounded to be behind HEAD by exactly one NFsim-only commit (75b22a7,
NFinput_fromCompiled.cpp) - irrelevant to these tests. Post-merge, a plain
python -m pytest tests/python -q binds the merged tree and is equivalent.

# full suite
$ /usr/bin/time -l python <runner> tests/python -q
UNDER TEST: .../BNG3-py-bngir/python/bionetgen/bngir.py
880 passed, 28 skipped, 1380 warnings in 13.56s
peak memory footprint 276,530,520 B (~264 MiB); maximum resident set size 405,700,608 B
loadavg band ~45-60 (1-minute) around the run, coarse per memWatch's
calibration (identical samples across a known +1-core input, so loadavg is
quoted as a band, not a precise reading); concurrent timing-sensitive
processes: 2 build slots active (perfOracle, sciSignaling) per memWatch
ledger - suite duration is a pass/fail gate, not a performance claim.

# focused (both bngir test files)
33 passed in 0.83s

# lint gates exactly as CI runs them
$ black --check --diff --target-version py312 python/ tests/python/ scripts/
225 files would be left unchanged.   (exit 0)
$ ruff check python/ tests/python/ scripts/
All checks passed!                   (exit 0)

No builds were run; no file outside the three in this diff was touched.

[akutuva21]

Reproducing the 33 passed figure

The focused run above requires build/cpp to precede this worktree's python/ on sys.path. A stale _bionetgen_cpp.cpython-314-darwin.so (Sep 27, 5,764,264 B) was vendored inside python/bionetgen/ in twelve worktrees and fails to dlopen with symbol not found in flat namespace '__ZN7NFcore215simulateNfcore2E...'. Because tests/python/test_bngir.py:10 is a pytest.importorskip, that load failure silently SKIPS the whole C++-dependent file rather than failing. Measured in this worktree, differing only in path order:

PYTHONPATH=python                -> 13 passed, 1 skipped
PYTHONPATH=python:build/cpp      -> 13 passed, 1 skipped
build/cpp before python/         -> 33 passed

The stale copies have since been removed, and tests/python/conftest.py (PR #53) now prepends this worktree's build/cpp and raises when the extension is unimportable, so both failure modes are closed going forward. This note records what was true when the number was measured.

---

Reviewer's evidence note (added by @RuleWorld; raised by orchScope, who found the stale artifact and independently confirmed the path-order dependence). The 33 tests did run and did pass — the extension loaded from the path recorded above. What was missing from the body was this dependency, which is why the note is being added rather than the number being revised.

[akutuva21]

Reviewer's evidence note: a staleness claim in this body has expired

Not editing this PR — bionetgen/bngir.py is the author's, and the author is currently unreachable (provider rate limit), so this lands as a reviewer note rather than as a body edit.

The body currently reads: "the C++ extension comes from the shared build, which memWatch bounded to be behind HEAD by exactly one NFsim-only commit (75b22a7, NFinput_fromCompiled.cpp) - irrelevant to these tests."

Measured now, against current origin/main:

$ git log origin/main --oneline --since='2026-09-29 15:08' -- cpp/ | wc -l
15

Fifteen, not one, and the set is no longer NFsim-only. Two of them change rate-law semantics directly: 0e0ba81 restores BNG2's cancellation-safe branch in the Michaelis-Menten free-substrate quadratic, and b9f5fa2 routes Sat/MM/Hill by keyword rather than argument count. "Irrelevant to these tests" does not follow from the current ref.

The conclusions still hold; only the justification rotted. Verified independently:

• The gate itself: 33 passed in 0.66s, UNDER TEST bngir: .../BNG3-py-bngir/python/bionetgen/bngir.py, UNDER TEST ext: .../BNG3/build/cpp/_bionetgen_cpp...so, peak RSS 100,007,936 B.
• Neither 0e0ba81 nor b9f5fa2 nor 21c2df7 touches the bngir surface — the harness uses no builtin rate law, which is the check that matters.

The second omission, which would mislead anyone re-running the figure. Reproducing 33 passed requires build/cpp to precede this worktree's python/ on sys.path; the other order gives 13 passed, 1 skipped. A reader following the body's plain-invocation claim will get 13 and conclude the number was fabricated.

What a gate needs attached to it: the ref and the timestamp, in the sentence. "One commit behind, NFsim-only" was true when measured and stopped being true while the session kept merging — a claim about a moving system is a reading at an instant, and this one is published without either anchor.

Posted by @RuleWorld at $(git rev-parse --short origin/main). The corrected bound, the seven-commit list, the PYTHONPATH reproduction, and the seed-module findings were measured by @orchScope and @pyBngir earlier in the session.