fix: resolve optional inputs without defaults to None (fixes #47)

[spinje]

Summary

Fixes the bug where optional workflow inputs declared with required: false but no default value would fail template resolution with "Unresolved variables" error.

Now, optional inputs without defaults resolve to None, allowing templates to work correctly.

Changes

• workflow_validator.py: Add None to context for optional inputs without explicit defaults
• node_wrapper.py: Filter error messages to only report actually unresolved variables (fixes misleading errors)
• test_null_defaults.py: Updated existing test + added regression tests
• test_node_wrapper_template_validation.py: Added tests for accurate error messages

 src/pflow/runtime/node_wrapper.py                       |   7 ++-
 src/pflow/runtime/workflow_validator.py                 |   7 ++-
 tests/test_runtime/test_node_wrapper_template_validation.py | 77 +++++++++++++++
 tests/test_runtime/test_null_defaults.py                |  80 +++++++++++++++-
 4 files changed, 164 insertions(+), 9 deletions(-)

Before/After

Before:

$ pflow workflow.json  # optional input not provided
❌ Error: Unresolved variables in parameter 'stdin': ${optional_param}

After:

$ pflow workflow.json  # optional input not provided
✓ Workflow completed
{"value": null}

Testing

• All 3858 tests pass
• make check passes (lint, type check, formatting)
• Manually verified with bug report test cases

Run make test to verify.

[claude]

Code Review

Summary

This PR fixes a legitimate bug where optional workflow inputs without explicit defaults fail template resolution. The fix is sound: optional inputs now resolve to None instead of being absent from the context. Error messages are also improved to only show actually unresolved variables.

---

Critical — must fix before merge

1. Missing edge case test: nested path access on None-valued optional inputs

Issue: When an optional input without a default resolves to None, attempting to access nested paths like ${optional_param.field} may cause unexpected behavior.

Current behavior (based on existing test test_path_traversal_with_null):

• context = {"parent": {"child": None}}
• ${parent.child.field} → returns the unresolved template string (fails resolution)

What happens with this PR:

• Optional input optional_param with required: false, no default
• User doesn't provide it → resolves to None
• Node has "params": {"data": "${optional_param.field}"}
• Will this fail with "Unresolved variables" error?

Required action: Add test case to test_null_defaults.py:

def test_nested_path_on_none_optional_input(self):
    """Test that nested path access on None-valued optional input fails gracefully."""
    workflow_ir = {
        "ir_version": "0.1.0",
        "nodes": [{
            "id": "test",
            "type": "shell",
            "params": {
                "stdin": "${optional_param.nested_field}"  # optional_param will be None
            }
        }],
        "edges": [],
        "inputs": {
            "optional_param": {
                "type": "object",
                "required": False
                # No default - will resolve to None
            }
        }
    }

    registry = Registry()

    # This should either:
    # 1. Fail validation with clear error ("Cannot access .nested_field on None")
    # 2. Resolve to None (if that's the desired behavior)
    #
    # Document which behavior is expected and test for it
    with pytest.raises(ValueError, match="optional_param"):
        compile_ir_to_flow(workflow_ir, registry, initial_params={})

Why this matters: This is a real-world scenario where users might template ${config.api_key} assuming config is optional, but fail at runtime when config is None.

---

Warnings — should be addressed

2. Potential confusion with type declarations

Issue: The tests use "type": "string" for inputs that will resolve to None:

"optional_param": {
    "type": "string",  # Says it's a string...
    "required": False,
    # No default specified - will resolve to None  # ...but will be None
}

Question: Should optional inputs without defaults have type validation relaxed to allow None, or should the type be declared as "string | null" or similar?

Current behavior: Type is declared as string, but the actual value passed to nodes is None (not a string).

Suggestion:

• Either document that required: false implicitly makes the type nullable
• Or consider adding validation that warns when type != null but no default provided

This isn't a blocker, but could prevent user confusion down the line.

---

3. Error message filtering logic could have false negatives

Location: node_wrapper.py:751

variables = {v for v in all_variables if not TemplateResolver.variable_exists(v, context)}

Issue: This filters out variables that "exist" in context, but what if a variable exists but path traversal fails?

Example:

context = {"data": {"nested": None}}
template = "${data.nested.field}"  # data exists, data.nested exists, but .field fails

Current code:

• variable_exists("data.nested.field", context) returns False (correct)
• Variable IS included in error message (correct)

Actually, this looks fine! variable_exists does full path traversal validation. Just wanted to flag this for confirmation.

---

Suggestions — optional improvements

4. Test organization

Observation: test_null_defaults.py now contains tests for two distinct features:

1. Null default value handling (original purpose)
2. Optional inputs without defaults resolving to None (new feature from this PR)

Suggestion: Consider renaming the file or splitting it:

• test_optional_inputs.py — broader scope
• Keep test_null_defaults.py focused on explicit "default": null behavior

Not urgent, but improves discoverability as the codebase grows.

---

5. Documentation clarity

Location: workflow_validator.py:176-177

# Optional with no default key - use None so templates can resolve
# This allows ${optional_param} to resolve to None instead of failing

Suggestion: Enhance comment to explain the semantic decision:

# Optional inputs without explicit default resolve to None.
# Rationale: "required: false" means "can be omitted", not "must have explicit default".
# This allows templates like ${optional_param} to resolve (to None) rather than fail validation.
# Nodes can then use None to apply their own smart defaults or skip optional behavior.

This helps future maintainers understand why this design choice was made.

---

6. Consider adding integration test

Current tests: All tests use compile_ir_to_flow at the workflow level.

Suggestion: Add one end-to-end test that actually executes a workflow with optional input = None to verify runtime behavior:

def test_execute_workflow_with_none_optional_input(self):
    """Integration test: workflow execution with None optional input succeeds."""
    workflow_ir = {
        "ir_version": "0.1.0",
        "nodes": [{
            "id": "echo",
            "type": "shell",
            "params": {
                "command": "echo",
                "stdin": "${optional_message}"
            }
        }],
        "edges": [],
        "inputs": {
            "optional_message": {
                "type": "string",
                "required": False
            }
        }
    }

    # Execute workflow without providing optional input
    registry = Registry()
    flow = compile_ir_to_flow(workflow_ir, registry, initial_params={})

    shared = {}
    result = flow.run(shared)  # Should execute successfully with stdin=None

    # Verify execution completed (specific assertion depends on shell node behavior with None stdin)

This would catch issues where compilation succeeds but runtime fails.

---

Security Considerations

✅ No security concerns identified

The changes correctly handle None values and don't introduce injection risks or expose sensitive data.

---

Test Coverage Assessment

Test quality: ✅ Excellent

The PR adds:

• Regression tests for the specific bug (optional inputs without defaults)
• Tests for error message accuracy (secondary bug fix)
• Tests for value override behavior
• Tests cover compilation phase

Test quantity: ✅ Appropriate

77 new lines in test_node_wrapper_template_validation.py, 80 new lines in test_null_defaults.py — focused and purposeful.

Missing coverage: See Critical #1 (nested path access edge case)

---

Performance Considerations

✅ No performance concerns

The filtering in node_wrapper.py:751 adds minimal overhead (set comprehension over variable names, typically <10 items).

---

Code Quality

Strengths

• ✅ Clear, focused changes
• ✅ Excellent test coverage for primary use case
• ✅ Helpful comments explaining the "why"
• ✅ Follows existing code style and patterns
• ✅ Error messages improved (more accurate, less confusing)

Style adherence

• ✅ Type hints present and correct
• ✅ Modern Python patterns (f-strings, comprehensions)
• ✅ No shadowing of built-ins
• ✅ Consistent with project conventions

---

Final Recommendation

Status: ⚠️ Conditionally approve — fix Critical #1, then merge

This is a solid bug fix that addresses a real usability issue. The implementation is clean and well-tested. Once the nested path edge case is covered, this is ready to merge.

Merge checklist:

• Add test for nested path access on None-valued optional inputs (Critical docs: Set up Mintlify documentation (Task 93) #1)
• Consider type declaration clarity (Warning feat: unify LLM usage via Simon Willison's llm library (Task 95) #2) — at minimum, add a code comment
• Optional: Enhance documentation per Suggestion Shell node errors show generic 'Workflow failed with action: error' without details #5

---

Detailed Code Review

workflow_validator.py:176-182

else:
    # Optional with no default key - use None so templates can resolve
    # This allows ${optional_param} to resolve to None instead of failing
    logger.debug(
        f"Optional input '{input_name}' not provided, using None as default",
        extra={"phase": "input_validation", "input": input_name},
    )
    defaults[input_name] = None

✅ Correct: This is the right semantic choice. Optional means "can be omitted", and the fix makes that work.

---

node_wrapper.py:747-751

all_variables = TemplateResolver.extract_variables(str(template))

# Filter to only actually unresolved variables (not in context)
# This prevents misleading errors like "${provided}, ${missing}" when only ${missing} failed
variables = {v for v in all_variables if not TemplateResolver.variable_exists(v, context)}

✅ Correct: This fixes the secondary bug where error messages were misleading. The filtering is sound because variable_exists does full path traversal.

⚠️ Minor: Consider adding assertion or comment that variables won't be empty at this point (because we only call this on failed resolution).

---

Tests: test_null_defaults.py:298-337

✅ Excellent: Direct regression test for the reported bug. Clear, focused, well-documented.

---

Tests: test_node_wrapper_template_validation.py:520-593

✅ Excellent: Tests verify the error message improvement works correctly. Good edge case coverage (one missing, multiple missing, available keys display).

---

Summary

This is high-quality work that fixes a real bug with minimal risk. The main gap is testing the edge case of nested path access on None-valued optional inputs. Once that's covered, this PR demonstrates the kind of thorough, well-tested bug fix we want in the codebase.

Great job! 🎉

[claude]

Code Review - PR #49

This PR fixes a bug where optional workflow inputs without explicit defaults failed template resolution. The fix is well-designed and production-ready, with excellent test coverage and thoughtful edge case handling.

---

✅ Approved - No blocking issues

Critical — must fix before merge

None.

---

Warnings — should be addressed

None. The implementation is solid.

---

Suggestions — optional improvements

1. Comment Clarity in workflow_validator.py (lines 176-182)

The comment explaining the design decision is excellent. Consider a minor rewording for even better clarity:

# Current:
# Optional inputs without explicit default resolve to None.
# Rationale: "required: false" means "can be omitted", and omitted
# values should still be available in context (as None) so templates
# like ${optional_param} can resolve rather than fail validation.
# Note: Nested access like ${optional_param.field} will still fail
# at runtime since you can't traverse into None - this is intentional.

# Suggested alternative (slightly more direct):
# Optional inputs without defaults resolve to None to enable template resolution.
# Design rationale:
#   - "required: false" means "can be omitted"
#   - Templates like ${optional_param} should resolve (to None) rather than fail
#   - Nested access ${optional_param.field} intentionally fails at runtime
#     (you can't traverse into None - this provides clear error messages)

This is purely stylistic - the current comment is already clear.

---

2. Test Organization (test_null_defaults.py)

The new tests are comprehensive and well-named. Consider extracting the edge case tests into a separate test class for better organization:

class TestOptionalInputsWithoutDefaults:
    """Test optional inputs without explicit defaults resolve to None."""

    def test_optional_input_without_default_resolves_to_none(self): ...
    def test_optional_input_without_default_can_be_overridden(self): ...
    def test_nested_path_on_none_optional_input_fails_gracefully(self): ...
    def test_nested_path_on_provided_optional_input_succeeds(self): ...

This would improve discoverability when scanning test failures. Current organization is fine, just a minor suggestion.

---

🎯 What This PR Does Well

1. Minimal, Surgical Changes

Only 2 production files changed with 7 lines of actual logic:

• workflow_validator.py: Added defaults[input_name] = None (1 line + comment)
• node_wrapper.py: Filter error messages to only unresolved variables (4 lines)

This is textbook "fix the root cause, nothing more."

2. Excellent Test Coverage

The test additions are exemplary:

✅ Primary bug fix tested (test_optional_input_without_default_resolves_to_none)
✅ Override behavior tested (test_optional_input_without_default_can_be_overridden)
✅ Edge cases documented (nested path access on None - both failure and success cases)
✅ Error message accuracy tested (new TestErrorMessageAccuracy class)
✅ Regression prevention (tests document exact failure scenarios from bug report)

The test names clearly describe expected behavior, making them excellent living documentation.

3. Thoughtful Error Handling

The fix correctly handles the distinction between:

• Simple template ${optional} → resolves to None (type preserved)
• Complex template "Value: ${optional}" → resolves to empty string (None coerced in string context)
• Nested access ${optional.field} → fails with clear error (can't traverse None)

This provides predictable behavior without surprises.

4. Improved Error Messages

The secondary fix (filtering error messages to only show actually unresolved variables) is a great UX improvement:

Before:

Error: Unresolved variables: ${provided}, ${missing}

After:

Error: Unresolved variables: ${missing}

This eliminates confusion when debugging template resolution issues.

5. Comprehensive Documentation

The scratchpads/optional-input-resolution/ directory provides:

• Detailed bug report with root cause analysis
• Multiple reproduction test cases
• Impact assessment and design rationale
• Comparison of alternative solutions

This is excellent practice for complex bug fixes.

---

🔍 Code Quality Assessment

Architectural Quality: ✅ Excellent

• Single Responsibility: Each change targets exactly one concern
• Testable Components: Changes are in pure functions with clear inputs/outputs
• Defensive Programming: Edge cases explicitly tested (None traversal, partial resolution)
• Clear Intent: Variable names and comments explain the "why" not just "what"

Code Quality: ✅ Excellent

• Type Safety: Properly typed (defaults[input_name] = None is type-correct)
• Performance: No performance impact (same code path, just different default value)
• Maintainability: Small, focused changes with comprehensive test coverage
• Error Handling: Intentional failure modes documented in tests

Security: ✅ Safe

• No injection risks (None value handled safely by template resolver)
• No credential exposure (test cases use dummy data)
• No unvalidated input (validation layer unchanged)

---

🎓 Lessons for Future Work

This PR exemplifies the principles from CLAUDE.md:

✅ "Write code optimized for change" - Minimal diff, clear separation of concerns
✅ "Prefer boring and obvious" - Simple default value change, no clever abstractions
✅ "Comprehensive tests document expected behavior" - Tests serve as specification
✅ "Focus on facts and problem-solving" - Root cause analysis before implementation

The only thing this PR doesn't do is break existing functionality - and that's verified by the passing test suite.

---

📊 Final Assessment

Criterion	Rating	Notes
Correctness	✅ Excellent	Fixes root cause, handles edge cases
Test Coverage	✅ Excellent	Comprehensive regression tests
Code Quality	✅ Excellent	Small, focused, well-documented
Architecture	✅ Excellent	No architectural changes needed
Security	✅ Safe	No risks introduced
Performance	✅ Neutral	No performance impact
Documentation	✅ Excellent	Code comments + scratchpad analysis

Recommendation: ✅ Approve and merge immediately.

This is production-ready code that improves both functionality and developer experience.

---

🙏 Acknowledgment

Great work on the second commit (fcf3f25) adding edge case tests per code review feedback. The nested path access tests (test_nested_path_on_none_optional_input_fails_gracefully and test_nested_path_on_provided_optional_input_succeeds) document exactly the right boundary conditions. This shows excellent responsiveness to feedback.

The enhanced comment in workflow_validator.py addressing the "nested access will fail" concern demonstrates thoughtful consideration of how users might misunderstand the feature.

---

Ready to merge. 🚀