Fix case sensitivity bug: S-expression node types are lowercase
Bug: S-expression plans produce lowercase node types (source, compound) but code was checking uppercase (SOURCE, COMPOUND). Fix: Use .upper() for node type comparisons. Add TestNodeTypeCaseSensitivity tests to catch this regression. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -732,7 +732,7 @@ def execute_recipe(self, recipe_sexp: str, input_hashes: Dict[str, str], run_id:
|
|||||||
input_paths.append(Path(input_path))
|
input_paths.append(Path(input_path))
|
||||||
|
|
||||||
# Handle SOURCE nodes
|
# Handle SOURCE nodes
|
||||||
if step.node_type == "SOURCE":
|
if step.node_type.upper() == "SOURCE":
|
||||||
source_cid = step.config.get("cid")
|
source_cid = step.config.get("cid")
|
||||||
|
|
||||||
# If source has :input true, resolve CID from input_hashes
|
# If source has :input true, resolve CID from input_hashes
|
||||||
@@ -772,7 +772,7 @@ def execute_recipe(self, recipe_sexp: str, input_hashes: Dict[str, str], run_id:
|
|||||||
raise ValueError(f"SOURCE step has no cid and no :input flag: {step.config}")
|
raise ValueError(f"SOURCE step has no cid and no :input flag: {step.config}")
|
||||||
|
|
||||||
# Handle COMPOUND nodes (collapsed effect chains)
|
# Handle COMPOUND nodes (collapsed effect chains)
|
||||||
if step.node_type == "COMPOUND":
|
if step.node_type.upper() == "COMPOUND":
|
||||||
import subprocess
|
import subprocess
|
||||||
import tempfile
|
import tempfile
|
||||||
|
|
||||||
|
|||||||
@@ -393,6 +393,61 @@ class TestPlanStepTypes:
|
|||||||
assert step.node_type == "SEQUENCE"
|
assert step.node_type == "SEQUENCE"
|
||||||
|
|
||||||
|
|
||||||
|
class TestNodeTypeCaseSensitivity:
|
||||||
|
"""
|
||||||
|
Tests for node type case handling.
|
||||||
|
|
||||||
|
Bug fixed: S-expression plans use lowercase (source, compound, effect)
|
||||||
|
but code was checking uppercase (SOURCE, COMPOUND, EFFECT).
|
||||||
|
"""
|
||||||
|
|
||||||
|
def test_source_lowercase_from_sexp(self):
|
||||||
|
"""S-expression plans produce lowercase node types."""
|
||||||
|
# From plan: (source :cid "Qm...")
|
||||||
|
step = MockStep("s1", "source", {"cid": "Qm123"}, "cache1")
|
||||||
|
|
||||||
|
# Code should handle lowercase
|
||||||
|
assert step.node_type.upper() == "SOURCE"
|
||||||
|
|
||||||
|
def test_compound_lowercase_from_sexp(self):
|
||||||
|
"""COMPOUND from S-expression is lowercase."""
|
||||||
|
# From plan: (compound :filter_chain ...)
|
||||||
|
step = MockStep("c1", "compound", {"filter_chain": []}, "cache2")
|
||||||
|
|
||||||
|
assert step.node_type.upper() == "COMPOUND"
|
||||||
|
|
||||||
|
def test_effect_lowercase_from_sexp(self):
|
||||||
|
"""EFFECT from S-expression is lowercase."""
|
||||||
|
# From plan: (effect :effect "invert" ...)
|
||||||
|
step = MockStep("e1", "effect", {"effect": "invert"}, "cache3")
|
||||||
|
|
||||||
|
assert step.node_type.upper() == "EFFECT"
|
||||||
|
|
||||||
|
def test_sequence_lowercase_from_sexp(self):
|
||||||
|
"""SEQUENCE from S-expression is lowercase."""
|
||||||
|
# From plan: (sequence :transition ...)
|
||||||
|
step = MockStep("seq1", "sequence", {"transition": {}}, "cache4")
|
||||||
|
|
||||||
|
assert step.node_type.upper() == "SEQUENCE"
|
||||||
|
|
||||||
|
def test_node_type_comparison_must_be_case_insensitive(self):
|
||||||
|
"""
|
||||||
|
Node type comparisons must be case-insensitive.
|
||||||
|
|
||||||
|
This is the actual bug - checking step.node_type == "SOURCE"
|
||||||
|
fails when step.node_type is "source" from S-expression.
|
||||||
|
"""
|
||||||
|
sexp_types = ["source", "compound", "effect", "sequence"]
|
||||||
|
code_types = ["SOURCE", "COMPOUND", "EFFECT", "SEQUENCE"]
|
||||||
|
|
||||||
|
for sexp, code in zip(sexp_types, code_types):
|
||||||
|
# Wrong: direct comparison fails
|
||||||
|
assert sexp != code, f"{sexp} should not equal {code}"
|
||||||
|
|
||||||
|
# Right: case-insensitive comparison works
|
||||||
|
assert sexp.upper() == code, f"{sexp}.upper() should equal {code}"
|
||||||
|
|
||||||
|
|
||||||
class TestExecuteRecipeErrorHandling:
|
class TestExecuteRecipeErrorHandling:
|
||||||
"""Tests for error handling in execute_recipe."""
|
"""Tests for error handling in execute_recipe."""
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user