Code review 2026-10-02: fail loudly instead of shipping a silently wrong bundle (default output unchanged)

PIPELINE_SETTINGS.md

@@ -67,23 +67,22 @@ what NOT to touch. Overriding is for A/B benchmarking only.
 | `LNG_COMPLEX_AS_NODE` | `1` | a Complex is one node (the key connectivity fix) |
 | `LNG_SET_EXPAND` | `1` | expand EntitySets to members at matching granularity |
 | `LNG_DIAGRAM_CONNECTIVITY` | `1` | union diagram-drawn reaction pairs into connectivity |
-| `LNG_DIAGRAM_BRIDGE` | `1` | add (don't merge) diagram bridge edges |
+| `LNG_DIAGRAM_BRIDGE` | `0` | legacy additive diagram bridges (measured harmful; off) |
+| `LNG_BOUNDARY_HIERARCHY` | `1` | decompose a root complex one `hasComponent` level at a time (deltasignal specs/030) |
+| `LNG_SET_POOL` | `1` | one OR node per set-valued catalyst or regulator (deltasignal specs/033) |
+| `LNG_POOL_ACTIVE_VIA` | `made_from` | how a pool's active form is found (deltasignal specs/039) |
 | `LNG_CATALYST_BUNDLE` | `0` | legacy; subsumed by `LNG_COMPLEX_AS_NODE` |
 | `LNG_HANDOFF_EDGES` | `0` | precedingEvent hand-off bridges (net-negative, off) |
 
-**Solver (DeltaSignal):**
+A switch takes exactly `0` or `1`; any other value, or an `LNG_*` name the
+generator does not read (a typo), stops the run at startup. The full list and
+defaults are `BOOL_FLAGS` in `src/env_flags.py`.
 
-| Variable | Default | Meaning |
-|---|---|---|
-| `DS_INHIBITION_MODE` | `divide` | de-repression-capable inhibition |
-| `DS_AND_MODE` | `hill_log` | continuous log-fold AND aggregation |
-| `DS_OR_MODE` | `mean` | OR alternatives each carry weight |
-| `DS_ASSEMBLY_LIMITING` | `1` | a complex can't exceed its scarcest subunit |
-| `DS_HILL_LOG_ZMAX` | `10.0` | AND saturation point |
-| `DS_SCC_SOLVE` | `1` | SCC-condensation solve (handles feedback loops) |
-
-(There are more `DS_*` switches for other inhibition/AND modes, loop handling,
-etc. — all default to the validated config. If you didn't set it, it's right.)
+**Solver (DeltaSignal):** the defaults live in the deltasignal repo's
+CLAUDE.md ("Solver Configuration"), which CI keeps in step with the code; they
+are not repeated here, because this copy went stale (it gave `DS_AND_MODE`
+`hill_log` and `DS_ASSEMBLY_LIMITING` `1`; the defaults are `hill_sat` and `0`).
+If you didn't set a `DS_*` variable, it is at the validated default.
 
 ## 4. Running the solver
 

src/diagram_connectivity.py

@@ -18,6 +18,7 @@
 """
 import json
 import os
+from src.env_flags import env_flag
 from collections import defaultdict
 from pathlib import Path
 from typing import Dict, List, Set, Tuple
@@ -212,7 +213,7 @@ def augment_reaction_connections(pathway_id: str,
     event_status='Diagram Shared Product' for traceability. No-op (returns input
     unchanged) when disabled or no diagram is available.
     """
-    if os.environ.get("LNG_DIAGRAM_CONNECTIVITY", "1") == "0":
+    if not env_flag("LNG_DIAGRAM_CONNECTIVITY"):
         return reaction_connections
 
     pairs = diagram_shared_product_pairs(pathway_id)

src/env_flags.py

@@ -0,0 +1,64 @@
+"""One reader for the generator's LNG_* switches.
+
+Before this (code review 2026-10-02) each flag was compared to a literal at its
+call site, in two opposite ways: `!= "0"` flags turned ON for any other value
+(LNG_HANDOFF_EDGES=false enabled the measured-harmful handoff edges) and
+`== "1"` flags turned OFF (LNG_BOUNDARY_HIERARCHY=true disabled the adopted
+hierarchy). A misspelt name was simply never read, while catalog.sh recorded it
+in BUILD.json as if applied. Now a switch takes exactly "0" or "1", and an
+LNG_* name the generator does not read is an error at startup.
+"""
+import os
+
+# Boolean switches and their defaults.
+BOOL_FLAGS = {
+    "LNG_BOUNDARY_EXPANSION": True,
+    "LNG_BOUNDARY_HIERARCHY": True,
+    "LNG_CAP_POOLS": False,
+    "LNG_CATALYST_BUNDLE": False,
+    "LNG_COMPLEX_AS_NODE": True,
+    "LNG_COMPOSITION_EDGES": False,
+    "LNG_DEBUG_TRACEBACKS": False,
+    "LNG_DIAGRAM_BRIDGE": False,
+    "LNG_DIAGRAM_CONNECTIVITY": True,
+    "LNG_DIAGRAM_SET_MEMBER": False,
+    "LNG_EMIT_ONE_SIDED": True,
+    "LNG_HANDOFF_EDGES": False,
+    "LNG_SET_EXPAND": True,
+    "LNG_SET_MEMBERS_OR": False,
+    "LNG_SET_POOL": True,
+}
+
+# Read elsewhere with their own parsing and validation.
+OTHER_FLAGS = frozenset({
+    "LNG_ALLOW_NONDETERMINISM",   # bin/create-pathways.py, before imports
+    "LNG_DIAGRAM_DIR",            # a path
+    "LNG_HANDOFF_HUB_MAX",        # _int_env
+    "LNG_MAX_VARIANTS",           # _int_env
+    "LNG_POOL_ACTIVE_VIA",        # pool_active_via()
+    "LNG_PYTHON",                 # catalog.sh: the interpreter, not a switch
+})
+
+
+def env_flag(name: str) -> bool:
+    """The value of boolean switch `name`: unset or empty is its default,
+    "1" is on, "0" is off, anything else is an error."""
+    default = BOOL_FLAGS[name]          # KeyError: an unregistered switch
+    raw = os.environ.get(name, "").strip()
+    if raw == "":
+        return default
+    if raw not in ("0", "1"):
+        raise ValueError(f"{name} must be 0 or 1, got {raw!r}. Unset it for the default "
+                         f"({int(default)}).")
+    return raw == "1"
+
+
+def validate_env(removed=()) -> None:
+    """Every LNG_* in the environment is a known name with a valid value."""
+    known = set(BOOL_FLAGS) | OTHER_FLAGS | set(removed)
+    unknown = sorted(n for n in os.environ if n.startswith("LNG_") and n not in known)
+    if unknown:
+        raise ValueError(f"Unknown LNG_* variable(s) {unknown}: the generator does not read "
+                         "them, so they would have no effect. Check the spelling.")
+    for name in BOOL_FLAGS:
+        env_flag(name)

src/logic_network_generator.py

@@ -1,4 +1,5 @@
 import os
+from src.env_flags import env_flag, validate_env
 import uuid
 from collections import Counter
 from typing import Dict, List, Any, NamedTuple, Optional, Set, Tuple
@@ -504,6 +505,7 @@ def _reject_removed_env() -> None:
     for name, why in sorted(_REMOVED_ENV.items()):
         if name in os.environ:
             raise ValueError(f"{name} was removed: {why}")
+    validate_env(removed=_REMOVED_ENV)
     pool_active_via()
 
 
@@ -800,7 +802,7 @@ def _map_annotated_entity_to_nodes(entity_id: str, member_set: Set[str]) -> Set[
         # (all plain stId), so a complex produced by one reaction and
         # catalysing/regulating another unifies instead of siloing into
         # disconnected ::variant:: nodes. See plan luminous-percolating-llama.
-        if os.environ.get("LNG_COMPLEX_AS_NODE", "1") != "0":
+        if env_flag("LNG_COMPLEX_AS_NODE"):
             return {str(entity_id)}
         # Set-variant node: OUTERMOST complex stId + a FLAT sorted list of the
         # variant's FULL terminal membership. We enumerate the complex's true
@@ -835,7 +837,7 @@ def _map_annotated_entity_to_nodes(entity_id: str, member_set: Set[str]) -> Set[
         # intersection came up empty and the set fell back to a bare, producer-less
         # node. Aligning the two granularities lets the set expand to its members
         # (sets should never survive as nodes — see docs/DESIGN_DECISIONS.md).
-        if os.environ.get("LNG_SET_EXPAND", "1") == "0":
+        if not env_flag("LNG_SET_EXPAND"):
             # Old behavior (leaf-granularity intersection) for A/B comparison.
             present = member_set & get_terminal_components(entity_id)
         else:
@@ -891,7 +893,7 @@ def _annotated_stoich(reaction_id: str, io: str) -> Dict[str, int]:
             stoich_cache[key] = get_reaction_io_stoichiometry(reaction_id, io)
         return stoich_cache[key]
 
-    cap_pools = os.environ.get("LNG_CAP_POOLS", "0") == "1"
+    cap_pools = env_flag("LNG_CAP_POOLS")
 
     def _resolve_io(reaction_id: str, io: str, members: Set[str]) -> tuple:
         # Node identity comes from the reaction's annotated entities; the
@@ -1352,7 +1354,10 @@ def _emit_substrate_depletion_edges(
                 if subst_stids:
                     ubiquitin_subst_by_rstid[rxn] = (subst_genes, subst_stids)
         except Exception as exc:
-            logger.warning(f"Ubiquitin topology lookup failed: {exc}")
+            # Raise: carrying on dropped every ubiquitin depletion edge from
+            # logic_network.csv with only a warning (code review 2026-10-02).
+            logger.error(f"Ubiquitin topology lookup failed: {exc}")
+            raise
 
     # Build network stid → list of UUIDs index from pathway_logic_network_data,
     # so we can target the substrate's NETWORK NODES (not just the leaf
@@ -1922,7 +1927,7 @@ def _is_complex(entity_id: str) -> bool:
     # a second, causal encoding of something already available structurally.
     #
     # Default ON pending the measurement that decides it.
-    boundary_expansion = os.environ.get("LNG_BOUNDARY_EXPANSION", "1") == "1"
+    boundary_expansion = env_flag("LNG_BOUNDARY_EXPANSION")
     # LNG_BOUNDARY_HIERARCHY=1 (deltasignal specs/030): decompose a root complex
     # ONE hasComponent level at a time instead of straight to its base leaves.
     # A component that already exists as a node -- e.g. ISGF3 [cytosol], which
@@ -1935,7 +1940,7 @@ def _is_complex(entity_id: str) -> bool:
     # root ISGF3:KPNA1:KPNB1 is translocated to the nucleus, and every IFN
     # alpha/beta perturbation upstream of ISGF3 was severed there (200 held-out
     # cases). The downstream-reuse rule (specs/018) applies unchanged.
-    hierarchy = os.environ.get("LNG_BOUNDARY_HIERARCHY", "1") == "1"   # default since deltasignal specs/030 (held-out +197, re-measured)
+    hierarchy = env_flag("LNG_BOUNDARY_HIERARCHY")   # default since deltasignal specs/030 (held-out +197, re-measured)
     from src.neo4j_connector import get_complex_components
     nested_registry: Dict[tuple, str] = {}
     seen_edges: Set[tuple] = set()
@@ -2068,7 +2073,7 @@ def _decompose_hier(container_uuid: str, container_stid: str, root_uuid: str, de
 
     # Composition hierarchy (LNG_COMPOSITION_EDGES). Runs after boundary
     # expansion so dissociation sinks are known and excluded as sources.
-    if os.environ.get("LNG_COMPOSITION_EDGES", "0") == "1":
+    if env_flag("LNG_COMPOSITION_EDGES"):
         _emit_composition_edges(pathway_logic_network_data, reactome_id_to_uuid)
 
 
@@ -2134,19 +2139,19 @@ def append_regulators(
         # neg) to its plain stId so it matches the produced/consumed occurrence's
         # node id and unifies instead of siloing. Subsumes the older opt-in
         # LNG_CATALYST_BUNDLE (pos-only).
-        complex_as_node = os.environ.get("LNG_COMPLEX_AS_NODE", "1") != "0"
-        bundle_on = os.environ.get("LNG_CATALYST_BUNDLE", "0") == "1"
+        complex_as_node = env_flag("LNG_COMPLEX_AS_NODE")
+        bundle_on = env_flag("LNG_CATALYST_BUNDLE")
         variant_decomposition = (pos_neg == "neg") and not complex_as_node
         bundle_complex = complex_as_node or ((pos_neg == "pos") and bundle_on)
         # Emit set-derived positive regulator members as OR alternatives
         # (see the and_or comment below). Opt-in while it is being A/B'd.
-        set_members_or = os.environ.get("LNG_SET_MEMBERS_OR", "0") == "1"
+        set_members_or = env_flag("LNG_SET_MEMBERS_OR")
         # deltasignal specs/033: a bare EntitySet regulator becomes ONE pool node
         # (the set itself) fed by its members, instead of every member wired
         # onto every reaction copy as a separate required (or blocking) term.
         # Default ON since specs/033's arms (held-out +6, tuning +10, exp +3 with
         # DS_SET_POOL_MODE=product); "0" restores the member fan-out.
-        set_pool = os.environ.get("LNG_SET_POOL", "1") == "1"
+        set_pool = env_flag("LNG_SET_POOL")
         from src.neo4j_connector import get_labels, get_set_members
 
         for _, row in map_df.iterrows():
@@ -2572,7 +2577,7 @@ def create_pathway_logic_network(
     # the consumption needs a depletion edge back onto the input, and depletion
     # is currently emitted for phosphatase reactions only. That is a separate
     # decision and is deliberately not bundled here.
-    emit_one_sided = os.environ.get("LNG_EMIT_ONE_SIDED", "1") == "1"
+    emit_one_sided = env_flag("LNG_EMIT_ONE_SIDED")
     for vr_uid, (input_ids, output_ids, input_stoich, output_stoich) in vr_entities.items():
         if not input_ids and not output_ids:
             continue  # genuinely empty: nothing to say about it
@@ -2747,7 +2752,7 @@ def create_pathway_logic_network(
     # injects roughly as much spurious coupling as real signal it recovers (same
     # reason the member-exploded network tied set-variant at ~69%). Kept for
     # future work but OFF by default. See memory project_complex_as_node_result.
-    if os.environ.get("LNG_HANDOFF_EDGES", "0") != "0":
+    if env_flag("LNG_HANDOFF_EDGES"):
         _emit_precedingevent_handoff_edges(
             pathway_logic_network_data=pathway_logic_network_data,
             reaction_connections=reaction_connections,
@@ -3281,10 +3286,10 @@ def export_containment(reactome_id_to_uuid: Dict[str, str],
     # positional copies would otherwise pay 40 times for the same answer.
     leaves_by_stid: Dict[str, Set[str]] = {}
     for stid in {str(v) for v in reactome_id_to_uuid.values() if v}:
-        try:
-            leaves = get_terminal_components(stid)
-        except Exception:  # noqa: BLE001 - a missing entity must not fail the pathway
-            leaves = set()
+        # A failed lookup raises: treating it as "contains only itself" put a
+        # wrong row in containment.csv, which the consumer's default model
+        # reads (code review 2026-10-02).
+        leaves = get_terminal_components(stid)
         leaves_by_stid[stid] = {str(x) for x in leaves} | {stid}
 
     # Keyed by STABLE ID, not uuid. Containment is a property of the entity, not
@@ -4071,15 +4076,10 @@ def add(stable_id: str, node_uuid: str, relation: str, depth: int,
 
     # 4. THE POINT: set -> the member nodes it was split into.
     excluded: List[Dict[str, str]] = []
-    try:
-        participating = get_pathway_participating_entities(pathway_id)
-    except Exception:
-        logger.warning("could not list participating entities; set rows omitted")
-        participating = set()
-    try:
-        atomic = set(get_modifier_isoform_entity_set_ids())
-    except Exception:
-        atomic = set()
+    # Both raise on failure: an empty answer silently omitted every set row
+    # from node_resolution.csv (code review 2026-10-02).
+    participating = get_pathway_participating_entities(pathway_id)
+    atomic = set(get_modifier_isoform_entity_set_ids())
     resolve = make_neo4j_resolver(get_set_members, get_labels)
 
     for stable_id in sorted(participating):

src/neo4j_connector.py

@@ -1,4 +1,5 @@
 import os
+from src.env_flags import env_flag
 
 from src.credential_redaction import scrub
 from typing import Any, Dict, List, Optional, Set, Tuple, Union
@@ -105,7 +106,7 @@ def _traceback_kwargs() -> Dict[str, Any]:
     debugging, off by default so nothing credential-bearing reaches
     debug_log.txt (mode 0664) or stdout.
     """
-    return {"exc_info": os.getenv("LNG_DEBUG_TRACEBACKS") == "1"}
+    return {"exc_info": env_flag("LNG_DEBUG_TRACEBACKS")}
 
 
 # Module-level caches for bulk pre-fetched data

src/pathway_generator.py

@@ -1,6 +1,7 @@
 import hashlib
 import json
 import os
+from src.env_flags import env_flag
 from pathlib import Path
 from typing import Any, Dict
 
@@ -211,6 +212,15 @@ def _write_cache_fingerprint(
 
 
 
+# Every file a pathway bundle is made of. Cleared before a regeneration.
+BUNDLE_FILES = (
+    "logic_network.csv", "stid_to_uuid_mapping.csv", "entity_reaction_proxy_mapping.csv",
+    "nodes.csv", "node_reaction_context.csv", "node_resolution.csv", "node_exclusions.csv",
+    "cofactors.csv", "containment.csv", "drugs.csv",
+    "pools.csv", "pool_transitions.csv", "pool_carriers.csv",
+)
+
+
 def generate_pathway_file(
     pathway_id: str,
     pathway_name: str,
@@ -267,6 +277,12 @@ def generate_pathway_file(
         )
     pathway_output_dir = base_output_dir / folder_name
     pathway_output_dir.mkdir(exist_ok=True)
+    # Regenerating into an existing directory must not leave a previous run's
+    # bundle files behind: a file this run fails to write would otherwise
+    # survive with node ids that no longer exist (code review 2026-10-02).
+    # The cache/ subdirectory is handled by its own fingerprint check.
+    for stale in BUNDLE_FILES:
+        (pathway_output_dir / stale).unlink(missing_ok=True)
 
     # Create cache subdirectory for intermediate files
     cache_dir = pathway_output_dir / "cache"
@@ -373,7 +389,7 @@ def generate_pathway_file(
             diagram_shared_product_pairs,
         )
         diagram_bridge_pairs = None
-        if os.environ.get("LNG_DIAGRAM_BRIDGE", "0") == "1":
+        if env_flag("LNG_DIAGRAM_BRIDGE"):
             connectivity = reaction_connections
             diagram_bridge_pairs = diagram_shared_product_pairs(pathway_id)
         else:
@@ -398,8 +414,8 @@ def generate_pathway_file(
         # catalog produced mixed and/or clusters. This produces them. The two
         # therefore have to be enabled and measured TOGETHER, and neither alone.
         set_member_pairs = None
-        if (os.environ.get("LNG_DIAGRAM_SET_MEMBER", "0") == "1"
-                and os.environ.get("LNG_DIAGRAM_CONNECTIVITY", "1") != "0"):
+        if (env_flag("LNG_DIAGRAM_SET_MEMBER")
+                and env_flag("LNG_DIAGRAM_CONNECTIVITY")):
             try:
                 set_member_pairs = diagram_set_member_pairs(pathway_id)
             except Exception:
@@ -433,9 +449,9 @@ def generate_pathway_file(
                 str(uuid_to_reactome_file)
             )
             logger.info(f"Successfully exported stable ID to UUID mapping: {uuid_to_reactome_file}")
-        except IOError as e:
+        except Exception as e:
             logger.error(f"Failed to write stable ID to UUID mapping file {uuid_to_reactome_file}: {e}")
-            # Don't raise - this is supplementary
+            raise
 
         # Export entity→reaction proxy mapping. Curated species (often Complexes)
         # that were expanded into virtual variants lose their own stId from the
@@ -454,7 +470,7 @@ def generate_pathway_file(
             logger.info(f"Successfully exported entity-reaction proxy mapping: {proxy_mapping_file}")
         except Exception as e:
             logger.error(f"Failed to write entity-reaction proxy mapping file {proxy_mapping_file}: {e}")
-            # Don't raise - this is supplementary
+            raise
 
         # Schema-backed provenance files (schema/logic_network.linkml.yaml):
         # nodes.csv (node_kind, diagram_entity_id, member_leaves, set provenance)
@@ -496,8 +512,12 @@ def generate_pathway_file(
                 str(pathway_output_dir / "containment.csv"),
             )
         except Exception as e:
+            # These are not supplementary: containment.csv, cofactors.csv and
+            # node_resolution.csv feed the consumer's default model. Swallowing
+            # this let the orphan guard's ValueError skip every file after it
+            # while the pathway counted as built (LNG code review 2026-10-02).
             logger.error(f"Failed to write node provenance files: {e}", exc_info=True)
-            # Don't raise - supplementary
+            raise
 
         # Drug-derived entities (deltasignal specs/032), in their OWN block: a
         # failed drug query must not cost containment.csv, which the default
@@ -511,6 +531,7 @@ def generate_pathway_file(
             )
         except Exception as e:
             logger.error(f"Failed to write drugs.csv: {e}", exc_info=True)
+            raise
 
         # Interconversion pools (deltasignal specs/039), in their own block.
         try:
@@ -525,6 +546,7 @@ def generate_pathway_file(
             )
         except Exception as e:
             logger.error(f"Failed to write pools.csv: {e}", exc_info=True)
+            raise
 
         logger.info(f"Output directory: {pathway_output_dir}")