fix: Respect declared input types for CLI parameters

[spinje]

Summary

Fixes #83 - Numeric string inputs (like Discord snowflake IDs) were silently coerced to int despite being declared as type: string in workflow inputs.

Changes

• Type coercion in prepare_inputs(): Added coerce_input_to_declared_type() that converts CLI-inferred values to match declared input types (e.g., int → string when type: string)
• Template resolver fix: Changed JSON auto-parsing to only parse containers (dict/list), not primitives - prevents "1234" from becoming 1234
• IR schema expansion: Accept Python type aliases (str, int, float, bool, dict, list) alongside JSON Schema types
• Helper function extraction: Reduced prepare_inputs() complexity from 12 to 7 by extracting _resolve_missing_input() and _coerce_provided_input()

Explanation

The bug had two root causes:

1. CLI parsing: infer_type() converts channel_id="1458..." to an integer before the workflow is loaded, so the declared type: string was never consulted
2. Template resolution: JSON auto-parsing in resolve_nested() converted string values like "42" back to integers

The fix addresses both:

1. prepare_inputs() now coerces values to match declared types after CLI parsing
2. Template resolver only auto-parses JSON into containers (dict/list), preserving primitive strings

Python type aliases were added to the IR schema so users can write type: int or type: str naturally, especially as we add more code node languages (TypeScript planned).

Testing

• 69 new tests covering all coercion scenarios
• All 3712 tests pass
• Manual verification with Discord snowflake ID workflow

Run make test to verify all tests pass.

[claude]

Code Review

This PR successfully fixes issue #83 by ensuring CLI-provided inputs respect workflow input type declarations. The implementation is well-structured with comprehensive test coverage.

---

✅ Strengths

1. Clear Problem/Solution Mapping: The two-part fix (input coercion + template resolver) correctly addresses both root causes identified in the issue.

2. Excellent Code Organization: Extracting helper functions (_resolve_missing_input, _coerce_provided_input, and individual coercion functions) significantly improved prepare_inputs() readability.

3. Comprehensive Test Coverage: 69 new tests covering all coercion paths, edge cases, and integration scenarios. Test quality is high - focused on actual behavior rather than coverage metrics.

4. Type Aliases Addition: Supporting Python type names (str, int, etc.) alongside JSON Schema types is user-friendly and forward-looking for TypeScript nodes.

5. Good Documentation: Docstrings clearly explain the "why" behind decisions, with helpful examples.

---

⚠️ Warnings — Should be addressed

1. Identity Check Pattern May Be Fragile (workflow_validator.py:267)

coerced_value = _coerce_provided_input(input_name, input_spec, provided_value)
if coerced_value is not provided_value:
    defaults[input_name] = coerced_value

Issue: This relies on coerce_input_to_declared_type() returning the same object when no coercion is needed. While this works for immutable types (int, str, bool), it's a fragile contract that's easy to break during refactoring.

Problem scenario: If someone later adds string normalization (.strip(), .lower()) to _coerce_to_string(), it would create a new string object even when type is already correct, causing unnecessary defaults entries.

Recommendation: Use explicit sentinel or boolean return:

def _coerce_provided_input(...) -> tuple[Any, bool]:
    """Returns (value, was_coerced)."""
    coerced = coerce_input_to_declared_type(...)
    was_coerced = coerced is not provided_value
    return coerced, was_coerced

# Usage:
coerced_value, was_coerced = _coerce_provided_input(...)
if was_coerced:
    defaults[input_name] = coerced_value

This makes the intent explicit and prevents subtle bugs from identity check assumptions.

---

2. Template Resolver Change May Break Existing Workflows (template_resolver.py:348)

if success and isinstance(parsed, (dict, list)):  # Changed from: if success:

Concern: This changes observable behavior for existing workflows that may rely on auto-parsing of numeric/boolean strings.

Example breakage:

# Before: This worked
inputs:
  count: ${shell.stdout}  # shell outputs "42", used as integer in calculations

# After: This breaks
# Now ${shell.stdout} stays as string "42" instead of int 42

Questions:

• Were there no existing tests for primitive auto-parsing? (The test update suggests there were)
• Has this been validated against existing example workflows?
• Should there be a brief migration note in CHANGELOG or docs?

Note: The PR description says "all 3712 tests pass", but the test test_parses_json_primitives was updated to expect strings instead of parsed primitives. This suggests the behavior change is intentional, but it's worth confirming no real workflows depend on primitive auto-parsing.

---

3. Error Handling Could Be More Explicit (param_coercion.py:158-161)

def _coerce_to_boolean(value: Any, log_context: dict[str, Any]) -> Any:
    if isinstance(value, str):
        lower_val = value.lower()
        if lower_val in ("true", "1", "yes"):
            return True
        elif lower_val in ("false", "0", "no"):
            return False
        else:
            logger.warning(f"Cannot coerce '{value}' to boolean", ...)
    return value  # Returns original value on failure

Issue: Coercion failures are logged as warnings but silently return the original value. This means invalid type conversions only fail later at runtime (in the code node), making debugging harder.

Example:

# Workflow declares: enabled: { type: boolean }
# User provides: enabled="maybe"
# Result: "maybe" (string) passed to code node expecting bool
# Error: Happens in code node, not during input validation

Recommendation: Consider validating coerced values and returning errors from prepare_inputs() instead of silently passing through invalid conversions. This gives users immediate, actionable feedback at the input validation phase.

Alternative: Document this as intended behavior (lenient coercion) in the function docstring.

---

4. Type Alias Normalization Inconsistency (param_coercion.py:30)

def _normalize_type(type_name: str) -> str:
    """Normalize type name to canonical form."""
    lower = type_name.lower()
    return _TYPE_ALIASES.get(lower, lower)

Issue: This lowercases ALL type names, not just aliases. So "STRING" becomes "string", but unknown types like "CustomType" become "customtype" (losing case information).

Potential issue: If pflow later supports custom/user-defined types, case-sensitive type names would be silently lowercased and not match.

Recommendation: Only lowercase known aliases:

def _normalize_type(type_name: str) -> str:
    normalized = _TYPE_ALIASES.get(type_name.lower())
    return normalized if normalized else type_name  # Preserve original case for unknowns

---

💡 Suggestions — Optional improvements

1. Duplicate Coercion Logic (param_coercion.py)

The integer/float coercion logic appears in both:

• coerce_to_declared_type() (for MCP tool parameters)
• coerce_input_to_declared_type() (for CLI inputs)

These could potentially share implementation to avoid duplication, but given they serve slightly different use cases, keeping them separate is acceptable for clarity.

---

2. Test Organization Opportunity

The new test file test_prepare_inputs_coercion.py has excellent coverage. Consider adding a brief module docstring explaining how it differs from test_param_coercion.py:

• test_param_coercion.py: Unit tests for coercion functions
• test_prepare_inputs_coercion.py: Integration tests for coercion in workflow validation pipeline

---

3. Consider Performance Profile

The coercion dispatch table is elegant, but each coercion function creates a new log_context dict. For workflows with many inputs, this could add overhead. Unlikely to matter in practice (input validation is <1ms vs node execution 100-1000ms), but worth noting.

---

4. Documentation Gap

The PR adds Python type aliases to IR schema but doesn't update user-facing docs explaining:

• Which type names are supported
• When to use JSON Schema vs Python types
• Migration advice for existing workflows

Consider adding a brief note to docs/ or architecture docs.

---

🎯 Summary

Decision: ⚠️ Approve with recommendations

The fix is correct and well-implemented, but I recommend addressing:

1. Identity check pattern (Warning docs: Set up Mintlify documentation (Task 93) #1) - Most important for maintainability
2. Error handling (Warning feat: Smart output display for registry run #3) - Important for user experience
3. Template resolver breaking change (Warning feat: unify LLM usage via Simon Willison's llm library (Task 95) #2) - Needs validation/documentation

The test coverage is exemplary and the code is clean. Great work on the systematic approach to solving this bug!

---

📋 Pre-merge Checklist

• Address identity check pattern fragility
• Validate template resolver change against existing workflows
• Document error handling behavior (lenient coercion)
• Run make check and make test locally
• Consider adding migration notes if primitive auto-parsing was used

[spinje]

Critical — must fix before merge

• None found.

Warnings — should be addressed

• coerce_input_to_declared_type() uses _coerce_to_string() which calls str(value) for dict/list. If CLI parsed JSON into a dict/list and the workflow declares type: string, this will emit a Python repr ({'a': 1}) rather than JSON ({"a": 1}), which is inconsistent with coerce_to_declared_type() and will break nodes expecting JSON strings. Consider JSON-serializing dict/list before falling back to str().

  Suggested patch:

  def _coerce_to_string(value: Any, log_context: dict[str, Any]) -> Any:
      if isinstance(value, (dict, list)):
          return json.dumps(value)
      if not isinstance(value, str):
          return str(value)
      return value

• Coercion only applies to CLI-provided values. Inputs resolved from os.environ, settings_env, or workflow defaults are not coerced to the declared type. This leaves type: integer inputs coming from env/defaults as strings and undermines the “respect declared input types” behavior. Consider applying coerce_input_to_declared_type() to resolved values as they enter defaults (and add tests in test_settings_env_integration.py or the new test_prepare_inputs_coercion.py).

Suggestions — optional improvements

• Consider trimming whitespace before coercing booleans/numbers (" true ", " 3.14 ") to be more forgiving of CLI input.
• If JSON primitive auto-parsing is now intentionally disabled in TemplateResolver, add a brief note in user-facing docs so workflows relying on "null" → None aren’t surprised.

Test coverage looks solid overall (new unit + integration tests), but I’d add at least one settings/env coercion case to lock in the behavior.

[spinje]

Review Feedback Addressed

Thanks for the thorough reviews! Here's what was fixed in commit ccf9a99:

From Review #1 (claude[bot])

Warning	Action
#1: Identity check fragile	✅ Fixed - _coerce_provided_input() now returns (value, was_coerced) tuple instead of relying on identity checks
#2: Template resolver breaking change	✅ Documented - Added comments explaining why primitives are not auto-parsed (intentional fix for the bug)
#3: Error handling	✅ Documented lenient coercion behavior + Created Task 120 for strict input type validation as future work
#4: Type normalization	✅ Fixed - _normalize_type() now preserves case for unknown types

From Review #2 (spinje)

Warning	Action
dict/list → string uses Python repr	✅ Fixed - _coerce_to_string() now uses json.dumps() for containers, producing valid JSON instead of {'a': 1}
Env/defaults values not coerced	✅ Fixed - Coercion now applies to values from os.environ, settings_env, and workflow defaults

Additional Fixes

• None handling: None values are now preserved (not coerced to string "None")
• Tests: Added 4 new tests for env/settings coercion

Deferred

• Whitespace trimming for booleans/numbers - nice-to-have, not critical
• User-facing docs update - code comments added, docs can come later

All 3711 tests pass, make check passes, bug fix verified working.