From 88061e1d3f37b5c96e8eb3c587b66b527c224451 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 15:06:24 -0400 Subject: [PATCH 1/2] Drop the LNG_BOUNDARY_LEAF_REUSE escape hatch; setting it is now an error The flag existed to ask one question -- does refusing to reuse a node the root complex can reach lose anything? -- and that question is answered: held-out +173 conditioned, p<1e-4, false change 1437->1150 (deltasignal specs/018). A flag whose question is settled is a second code path nobody measures. Removing it outright would make a stale LNG_BOUNDARY_LEAF_REUSE=any in an environment or a benchmark script silently measure the default while the operator believed they were measuring the old mode -- exactly the silent-input bug class this project has hit four times. So the name is kept as a rejection: setting it to anything raises, naming the spec. The two mode tests are replaced by one that asserts that error for three values, including the two that used to be valid. Co-Authored-By: Claude Opus 5 (1M context) --- src/logic_network_generator.py | 27 +++++++++++++++------------ tests/test_boundary_leaf_reuse.py | 26 ++++++++------------------ 2 files changed, 23 insertions(+), 30 deletions(-) diff --git a/src/logic_network_generator.py b/src/logic_network_generator.py index 9656e0b..fbfa2ca 100755 --- a/src/logic_network_generator.py +++ b/src/logic_network_generator.py @@ -1712,11 +1712,18 @@ def _emit_boundary_decomposition_edges( # takes TP53's strongly connected component from 836 nodes to 56 and DSB's # from 1,127 to ~290 (deltasignal specs/018). Reachability is computed over # every edge emitted so far (bridges and depletion edges included). - # LNG_BOUNDARY_LEAF_REUSE=any restores the old behaviour. + # + # This is unconditional. The LNG_BOUNDARY_LEAF_REUSE escape hatch that + # restored the welding behaviour was removed once the question it existed + # to ask was answered (held-out +173, p<1e-4). Setting it is an error + # rather than a no-op, so a stale value in an environment or a script is + # not silently ignored while the run measures something else. from collections import defaultdict - reuse_mode = os.environ.get("LNG_BOUNDARY_LEAF_REUSE", "downstream_free") - if reuse_mode not in ("downstream_free", "any"): - raise ValueError(f"LNG_BOUNDARY_LEAF_REUSE={reuse_mode!r}: expected 'downstream_free' or 'any'") + if "LNG_BOUNDARY_LEAF_REUSE" in os.environ: + raise ValueError( + "LNG_BOUNDARY_LEAF_REUSE was removed: boundary leaves never reuse a node " + "the root complex can reach. See deltasignal specs/018-derived-edge-loops." + ) _succ: Dict[str, List[str]] = defaultdict(list) for e in pathway_logic_network_data: _succ[str(e.get("source_id"))].append(str(e.get("target_id"))) @@ -1739,14 +1746,10 @@ def _downstream_of(root_uuid: str) -> Set[str]: def _leaf_uuid(leaf_stid: str, root_uuid: str) -> str: candidates = stid_to_existing_uuids.get(leaf_stid, []) - if reuse_mode == "any": - if candidates: - return candidates[0] - else: - downstream = _downstream_of(root_uuid) - for cand in candidates: - if cand not in downstream: - return cand + downstream = _downstream_of(root_uuid) + for cand in candidates: + if cand not in downstream: + return cand if leaf_stid not in leaf_uuid_registry: leaf_uuid_registry[leaf_stid] = str(uuid.uuid4()) reactome_id_to_uuid[leaf_uuid_registry[leaf_stid]] = leaf_stid diff --git a/tests/test_boundary_leaf_reuse.py b/tests/test_boundary_leaf_reuse.py index 47a64cd..756e3df 100644 --- a/tests/test_boundary_leaf_reuse.py +++ b/tests/test_boundary_leaf_reuse.py @@ -5,7 +5,6 @@ v97 catalog; deltasignal specs/018). A produced node that is NOT downstream is a real feed-forward link (hasComponent with no reaction) and must still be reused -- severing those cost Mitotic G1 28 cases in the first version of this fix.""" -import os import pytest import src.neo4j_connector as nc import src.logic_network_generator as lng @@ -40,7 +39,6 @@ def assembly_sources(data): def test_produced_node_is_not_reused_as_a_boundary_leaf(stub, monkeypatch): - monkeypatch.delenv("LNG_BOUNDARY_LEAF_REUSE", raising=False) data, r2u = network() _emit_boundary_decomposition_edges(data, r2u) asm = assembly_sources(data) @@ -60,7 +58,6 @@ def test_produced_node_is_not_reused_as_a_boundary_leaf(stub, monkeypatch): def test_a_copy_reached_only_through_a_bridge_is_downstream_too(stub, monkeypatch): # P has two copies: u_Pout (reaction output, downstream of C) and u_Pin (an input copy whose # only incoming edge is a diagram bridge from u_Pout -- also downstream). Neither may be reused. - monkeypatch.delenv("LNG_BOUNDARY_LEAF_REUSE", raising=False) data, r2u = network() data.append({"source_id": "u_Pout", "target_id": "u_Pin", "pos_neg": "pos", "and_or": "and", "edge_type": "diagram_bridge", "stoichiometry": 1}) data.append({"source_id": "u_Pin", "target_id": "r3", "pos_neg": "pos", "and_or": "and", "edge_type": "input", "stoichiometry": 1}) @@ -75,7 +72,6 @@ def test_a_produced_copy_that_is_not_downstream_is_reused(stub, monkeypatch): # The Mitotic G1 shape: P's only copy is produced by a reaction the root complex C does NOT # reach (r2, fed by Q). Reusing it adds no cycle and keeps the feed-forward link # P -> (assembly) -> C that Reactome joins only by hasComponent. - monkeypatch.delenv("LNG_BOUNDARY_LEAF_REUSE", raising=False) data = [ {"source_id": "u_C", "target_id": "r1", "pos_neg": "pos", "and_or": "and", "edge_type": "input", "stoichiometry": 1}, {"source_id": "r1", "target_id": "u_W", "pos_neg": "pos", "and_or": "or", "edge_type": "output", "stoichiometry": 1}, @@ -93,7 +89,6 @@ def test_a_produced_copy_that_is_not_downstream_is_reused(stub, monkeypatch): def test_two_root_complexes_sharing_a_subunit_each_get_an_acyclic_leaf(stub, monkeypatch): # C reaches P's produced copy (u_Pout) -> C gets a fresh leaf. C2 does NOT reach it -> # C2 reuses u_Pout (a real feed-forward link). Neither assembly edge closes a cycle. - monkeypatch.delenv("LNG_BOUNDARY_LEAF_REUSE", raising=False) data, r2u = network() data.append({"source_id": "u_C2", "target_id": "r4", "pos_neg": "pos", "and_or": "and", "edge_type": "input", "stoichiometry": 1}) data.append({"source_id": "r4", "target_id": "u_Z", "pos_neg": "pos", "and_or": "or", "edge_type": "output", "stoichiometry": 1}) @@ -118,16 +113,11 @@ def reach(u): assert not (srcs & reach(cpx)) -def test_legacy_reuse_is_available_and_welds_the_cycle(stub, monkeypatch): - monkeypatch.setenv("LNG_BOUNDARY_LEAF_REUSE", "any") - data, r2u = network() - _emit_boundary_decomposition_edges(data, r2u) - asm = assembly_sources(data) - assert "u_Pout" in asm # the old behaviour: the produced copy is reused -> C -> r1 -> u_Pout -> C - - -def test_bad_mode_is_an_error(stub, monkeypatch): - monkeypatch.setenv("LNG_BOUNDARY_LEAF_REUSE", "unproduced") - data, r2u = network() - with pytest.raises(ValueError): - _emit_boundary_decomposition_edges(data, r2u) +def test_removed_flag_is_an_error_not_a_noop(stub, monkeypatch): + # The escape hatch was removed; a stale value must fail loudly rather than + # letting a run silently measure the default while claiming the old mode. + for val in ("any", "downstream_free", "unproduced"): + monkeypatch.setenv("LNG_BOUNDARY_LEAF_REUSE", val) + data, r2u = network() + with pytest.raises(ValueError, match="was removed"): + _emit_boundary_decomposition_edges(data, r2u) From f5cf8527785e3acfc614a4b73c5a07fcfb63bb7e Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Mon, 21 Sep 2026 15:38:23 -0400 Subject: [PATCH 2/2] Make the removed-flag guard actually stop a run, and test it for real Adversarial review of the diff found the guard was decorative on both halves. It had NO runtime coverage. Its only test asserted that a string appeared in inspect.getsource(), which passes whether or not the guard runs -- mutating it to `if False and ...` left the whole suite green, and exporting the flag and running the suite gave the baseline result, because no test reaches that line. Replaced with a shared _reject_removed_env() helper, unit-tested with monkeypatch and pytest.raises across five values including "0" and "". And it could not stop a run even when it fired. bin/create-pathways.py catches every per-pathway exception, counts it, and returns None -- so a stale flag meant every pathway raised, nothing was written, the PREVIOUS catalog stayed on disk, and the shell saw exit 0. A benchmark reading that directory would have silently scored the old catalog. Now checked once at the top of main() before any work, and the command exits 1 if any pathway failed. Six other error paths in main() also returned 0 on failure; all now exit 1. All three mutations are caught by the new tests: dead guard body, missing entry-point call, and restored exit-0-on-failure. The error message also claimed a guarantee the code does not provide. _succ is a snapshot taken before the emission loop and is not updated as this function appends its own assembly edges, so a later root complex can still be handed a leaf that an earlier one made reachable. The reviewer constructed the weld. That bug is PRE-EXISTING and identical on both sides of this flag removal, so the comment and message now state the rule precisely rather than overclaiming; fixing it changes emitted networks and would invalidate the measurement this rule rests on, so it is tracked separately. Also: a conftest clears removed names for the whole suite (this diff otherwise turned a stale export into 10 spurious failures, up from 1), unused monkeypatch fixtures dropped, and PIPELINE_SETTINGS.md gains a removed-flags table. Co-Authored-By: Claude Opus 5 (1M context) --- PIPELINE_SETTINGS.md | 13 +++++ bin/create-pathways.py | 30 +++++++++--- src/logic_network_generator.py | 40 ++++++++++++---- tests/conftest.py | 19 ++++++++ tests/test_boundary_leaf_reuse.py | 6 +-- tests/test_removed_env_flags.py | 80 +++++++++++++++++++++++++++++++ 6 files changed, 170 insertions(+), 18 deletions(-) create mode 100644 tests/conftest.py create mode 100644 tests/test_removed_env_flags.py diff --git a/PIPELINE_SETTINGS.md b/PIPELINE_SETTINGS.md index f5b8bc4..002ec05 100644 --- a/PIPELINE_SETTINGS.md +++ b/PIPELINE_SETTINGS.md @@ -99,3 +99,16 @@ If you drive DeltaSignal over its HTTP API, two things matter: is an in-memory, per-process cache, so re-parse if the server restarts. Full request/response schemas: `deltasignal/docs/API.md`. + +## Removed flags + +These names are still **read**, and setting one is a hard error rather than a +no-op. A flag is removed once the question it existed to ask has been answered: +the spec keeps the evidence, the code does not keep a second path nobody +measures. Rejecting the name rather than ignoring it means a stale value in a +shell, a compose file or an unversioned benchmark script cannot quietly make a +run measure the default while the operator believes otherwise. + +| flag | removed | why | +|---|---|---| +| `LNG_BOUNDARY_LEAF_REUSE` | 2026-09-21 | Boundary leaves never reuse a node the root complex can reach at the time the leaf is chosen. Held-out +173, p<1e-4 (deltasignal `specs/018-derived-edge-loops`). | diff --git a/bin/create-pathways.py b/bin/create-pathways.py index ecd46b7..f007bb9 100755 --- a/bin/create-pathways.py +++ b/bin/create-pathways.py @@ -22,6 +22,7 @@ sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) from src.argument_parser import configure_logging, logger, parse_args +from src.logic_network_generator import _reject_removed_env from src.pathway_generator import generate_pathway_file from src.neo4j_connector import get_top_level_pathways, get_pathway_name @@ -40,6 +41,14 @@ def canonical_pathway_id(raw: str) -> str: def main() -> None: + # Fail before ANY work if a removed flag is set. The per-pathway + # guards inside the generator would each raise, but the loop below catches + # every exception and continues, so without this check a stale flag would + # produce "93 failed" buried in the log, leave the PREVIOUS run's + # logic_network.csv files untouched on disk, and still exit 0 -- and the + # benchmark that reads that directory would silently score the old catalog. + _reject_removed_env() + dotenv_path = os.path.join(os.path.dirname(__file__), "..", ".env") # load_dotenv populates os.environ (without clobbering already-set vars) so # NEO4J_URL/USER/PASSWORD in .env actually reach neo4j_connector.get_graph(), @@ -63,15 +72,15 @@ def main() -> None: if pathway_list_file: if not os.path.exists(pathway_list_file): logger.error(f"Pathway list file '{pathway_list_file}' does not exist.") - return + sys.exit(1) elif not os.access(pathway_list_file, os.R_OK): logger.error(f"Pathway list file '{pathway_list_file}' is not readable.") - return + sys.exit(1) elif not args.pathway_list and not args.pathway_id and not args.top_level_pathways: logger.error( "One of the following is required: '--pathway-id', '--pathway-list', '--top-level-pathways', or 'PATHWAY_LIST_FILE' environment variable." ) - return + sys.exit(1) pathway_list: List[Tuple[str, str]] = [] @@ -84,7 +93,7 @@ def main() -> None: logger.info(f"Found {len(pathway_list)} top-level pathways") except Exception as e: logger.error(f"Error fetching top-level pathways: {e}") - return + sys.exit(1) elif args.pathway_id: # Single pathway by ID - fetch name from database pathway_id = args.pathway_id @@ -93,10 +102,10 @@ def main() -> None: logger.info(f"Found pathway: {pathway_name} (stId: {pathway_id})") except ValueError: logger.error(f"Pathway with ID {pathway_id} not found in database") - return + sys.exit(1) except Exception as e: logger.error(f"Error fetching pathway name: {e}") - return + sys.exit(1) pathway_list = [(pathway_id, pathway_name)] elif pathway_list_file: try: @@ -106,7 +115,7 @@ def main() -> None: pathways_df["pathway_name"])] except Exception as e: logger.error(f"Error reading pathway list file: {e}") - return + sys.exit(1) logger.info(f"Processing {len(pathway_list)} pathway(s)") logger.info(f"Output directory: {output_dir}") @@ -124,6 +133,13 @@ def main() -> None: continue logger.info(f"Completed: {successful} successful, {failed} failed") + if failed: + # A partial catalog is not a catalog: downstream benchmarks glob the + # output directory and would score whatever mix of new and stale + # pathways happens to be there. Exit non-zero so a shell `&&` chain, + # a Makefile or CI stops instead of proceeding. + logger.error(f"{failed} pathway(s) failed; exiting non-zero") + sys.exit(1) if __name__ == "__main__": diff --git a/src/logic_network_generator.py b/src/logic_network_generator.py index fbfa2ca..622a59a 100755 --- a/src/logic_network_generator.py +++ b/src/logic_network_generator.py @@ -481,6 +481,26 @@ def _register_entity_uuid( return entity_uuid_registry[key] +# Flags that were removed once the question they existed to ask was answered +# (see the flag-expiry policy in deltasignal specs/020). The names are still +# READ so that a stale value in a shell, a compose file or an unversioned +# benchmark script fails loudly instead of being silently ignored while the run +# measures the default. That silent-input failure mode has bitten this project +# four times (deltasignal DS #40-43). +_REMOVED_ENV = { + "LNG_BOUNDARY_LEAF_REUSE": + "boundary leaves never reuse a node the root complex can reach at the time " + "the leaf is chosen. See deltasignal specs/018-derived-edge-loops.", +} + + +def _reject_removed_env() -> None: + """Raise if any removed LNG_* flag is set. Safe to call repeatedly.""" + for name, why in sorted(_REMOVED_ENV.items()): + if name in os.environ: + raise ValueError(f"{name} was removed: {why}") + + def _register_phase1( vr_entities: Dict[str, tuple], entity_uuid_registry: Dict[tuple, str], @@ -1715,15 +1735,19 @@ def _emit_boundary_decomposition_edges( # # This is unconditional. The LNG_BOUNDARY_LEAF_REUSE escape hatch that # restored the welding behaviour was removed once the question it existed - # to ask was answered (held-out +173, p<1e-4). Setting it is an error - # rather than a no-op, so a stale value in an environment or a script is - # not silently ignored while the run measures something else. + # to ask was answered (held-out +173, p<1e-4). + # + # SCOPE OF THE RULE, stated precisely: a leaf is refused if the root + # complex can reach it *at the moment that leaf is chosen*. `_succ` is a + # snapshot taken before the loop and is NOT updated as this function + # appends its own assembly edges, so a later root complex can still be + # handed a leaf that became reachable through an edge emitted for an + # earlier one. That residual weld is a real pre-existing bug -- it exists + # identically on both sides of the flag removal -- and it is tracked + # separately rather than fixed here, because fixing it changes emitted + # networks and would invalidate the measurement this rule rests on. from collections import defaultdict - if "LNG_BOUNDARY_LEAF_REUSE" in os.environ: - raise ValueError( - "LNG_BOUNDARY_LEAF_REUSE was removed: boundary leaves never reuse a node " - "the root complex can reach. See deltasignal specs/018-derived-edge-loops." - ) + _reject_removed_env() _succ: Dict[str, List[str]] = defaultdict(list) for e in pathway_logic_network_data: _succ[str(e.get("source_id"))].append(str(e.get("target_id"))) diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..fb416e7 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,19 @@ +"""Shared fixtures. + +Removed LNG_* flags are rejected at runtime (see `_REMOVED_ENV`), so a stale +value exported in a developer's shell or a CI runner would turn every test that +builds a network into a spurious failure. Tests should exercise the code, not +the ambient environment, so clear those names for the whole suite. The guard +itself is tested explicitly in tests/test_removed_env_flags.py. +""" + +import pytest + +from src.logic_network_generator import _REMOVED_ENV + + +@pytest.fixture(autouse=True) +def _clear_removed_env_flags(monkeypatch): + for name in _REMOVED_ENV: + monkeypatch.delenv(name, raising=False) + yield diff --git a/tests/test_boundary_leaf_reuse.py b/tests/test_boundary_leaf_reuse.py index 756e3df..c081bbc 100644 --- a/tests/test_boundary_leaf_reuse.py +++ b/tests/test_boundary_leaf_reuse.py @@ -38,7 +38,7 @@ def assembly_sources(data): return {e["source_id"]: e for e in data if e["edge_type"] == "assembly"} -def test_produced_node_is_not_reused_as_a_boundary_leaf(stub, monkeypatch): +def test_produced_node_is_not_reused_as_a_boundary_leaf(stub): data, r2u = network() _emit_boundary_decomposition_edges(data, r2u) asm = assembly_sources(data) @@ -55,7 +55,7 @@ def test_produced_node_is_not_reused_as_a_boundary_leaf(stub, monkeypatch): assert not (sources_into_C & produced) -def test_a_copy_reached_only_through_a_bridge_is_downstream_too(stub, monkeypatch): +def test_a_copy_reached_only_through_a_bridge_is_downstream_too(stub): # P has two copies: u_Pout (reaction output, downstream of C) and u_Pin (an input copy whose # only incoming edge is a diagram bridge from u_Pout -- also downstream). Neither may be reused. data, r2u = network() @@ -68,7 +68,7 @@ def test_a_copy_reached_only_through_a_bridge_is_downstream_too(stub, monkeypatc assert "u_Pout" not in asm and "u_Pin" not in asm -def test_a_produced_copy_that_is_not_downstream_is_reused(stub, monkeypatch): +def test_a_produced_copy_that_is_not_downstream_is_reused(stub): # The Mitotic G1 shape: P's only copy is produced by a reaction the root complex C does NOT # reach (r2, fed by Q). Reusing it adds no cycle and keeps the feed-forward link # P -> (assembly) -> C that Reactome joins only by hasComponent. diff --git a/tests/test_removed_env_flags.py b/tests/test_removed_env_flags.py new file mode 100644 index 0000000..aaa8a51 --- /dev/null +++ b/tests/test_removed_env_flags.py @@ -0,0 +1,80 @@ +"""The removed-flag guard, tested for real. + +This file exists because the first version of the guard was tested only by +asserting that a string appeared in `inspect.getsource(...)`. That passes +whether or not the guard runs: mutating it to `if False and ...` left the whole +suite green. These tests call the guard and the entry point that uses it. +""" +import subprocess +import sys +from pathlib import Path + +import pytest + +from src.logic_network_generator import _REMOVED_ENV, _reject_removed_env + +REPO = Path(__file__).resolve().parents[1] + + +def test_clean_environment_is_fine(monkeypatch): + for name in _REMOVED_ENV: + monkeypatch.delenv(name, raising=False) + _reject_removed_env() # must not raise + + +@pytest.mark.parametrize("name", sorted(_REMOVED_ENV)) +@pytest.mark.parametrize("value", ["1", "0", "any", "", "downstream_free"]) +def test_any_value_of_a_removed_flag_raises(monkeypatch, name, value): + # Including "0" and "": the point is that the name is gone, so there is no + # value that means "use the old behaviour" and none that means "off". + monkeypatch.setenv(name, value) + with pytest.raises(ValueError, match=f"{name} was removed"): + _reject_removed_env() + + +@pytest.mark.parametrize("name", sorted(_REMOVED_ENV)) +def test_the_message_names_the_spec_that_holds_the_evidence(monkeypatch, name): + monkeypatch.setenv(name, "1") + with pytest.raises(ValueError) as exc: + _reject_removed_env() + assert "specs/" in str(exc.value) + + +@pytest.mark.parametrize("name", sorted(_REMOVED_ENV)) +def test_entry_point_exits_nonzero_before_doing_any_work(monkeypatch, name, tmp_path): + # The generator's per-pathway guard is not enough on its own: the pathway + # loop catches every exception and continues, so a stale flag would leave + # the previous run's files on disk and still exit 0. + env = {**dict(__import__("os").environ), name: "1"} + out = tmp_path / "out" + proc = subprocess.run( + [sys.executable, "bin/create-pathways.py", "--pathway-id", "R-HSA-73894", + "--output-dir", str(out)], + cwd=REPO, env=env, capture_output=True, text=True, timeout=180, + ) + assert proc.returncode != 0, "a removed flag must stop the run" + assert f"{name} was removed" in (proc.stderr + proc.stdout) + assert not out.exists() or not any(out.iterdir()), "must fail before writing output" + + +def test_a_failing_pathway_makes_the_run_exit_nonzero(tmp_path): + """A partial catalog must not look like a successful run. + + The pathway loop catches every exception and continues, so before this the + command logged "N failed" and still exited 0. A benchmark that globs the + output directory would then score whatever mix of new and stale pathways + happened to be there. Any failure reason serves here -- a bogus stable id + or an unreachable database both take the same path. + """ + listing = tmp_path / "list.tsv" + listing.write_text("id\tpathway_name\nR-HSA-NOT-A-REAL-PATHWAY\tbogus\n") + out = tmp_path / "out" + proc = subprocess.run( + [sys.executable, "bin/create-pathways.py", + "--pathway-list", str(listing), "--output-dir", str(out)], + cwd=REPO, capture_output=True, text=True, timeout=300, + ) + assert proc.returncode != 0, ( + "a run in which every pathway failed must not exit 0\n" + f"stdout:\n{proc.stdout[-2000:]}\nstderr:\n{proc.stderr[-2000:]}" + )