fix: allow arrays/dicts to serialize to JSON in string contexts

[spinje]

Summary

Fixes #18 - Arrays and dicts can now be embedded in shell command strings (and other string parameters) by auto-serializing to JSON.

Changes

• Type checker update: Added str, string to compatible types for dict, list, array, object
• Shell-unsafe JSON warning: New _warn_shell_unsafe_json() method warns when JSON contains characters that could break shell parsing (', `, $()
• Documentation: Updated architecture/reference/template-variables.md with new type matrix and "Shell Command Limitations" section
• Tests updated: Modified tests that expected dict→str to fail, added new tests for list→str

Behavior

Template	Data	Result
"echo '${items}'"	["a", "b"]	✅ Works → echo '["a", "b"]'
"${items}"	["a", "b"]	❌ Blocked (simple template, runtime check preserved)
"echo '${data}'"	{"msg": "it's"}	⚠️ Warning about shell-unsafe chars

Guardrails Preserved

1. Simple templates (${items} alone) → runtime check still blocks dict/list→str
2. Shell-unsafe JSON → warning emitted, suggests using stdin
3. Type mismatches for non-string targets (dict→int) → still caught

File Changes

 architecture/reference/template-variables.md       | 62 ++++++++++-
 src/pflow/nodes/shell/shell.py                     | 44 ++++++++
 src/pflow/runtime/template_validator.py            |  2 +-
 src/pflow/runtime/type_checker.py                  |  8 +-
 tests/test_runtime/test_template_validator_types.py| 73 ++++++++++---
 tests/test_runtime/test_type_checker.py            | 38 ++++---
 6 files changed, 190 insertions(+), 37 deletions(-)

Testing

Run make test to verify all tests pass (3612 passed).
Run make check to verify linting and type checks pass.

[claude]

Code Review for PR #19: Array/Dict to JSON String Serialization

Summary

This PR enables arrays and dictionaries to be embedded in string parameters by auto-serializing them to JSON. This solves a real problem where shell nodes output JSON strings that need to flow into batch processing or be embedded in commands. The implementation is well-structured with proper guardrails, but there are some areas that need attention before merge.

---

✅ Strengths

1. Solves a Real Problem: The webpage-to-markdown example clearly demonstrates the value - shell output is always strings, and needing to parse JSON is a common pattern.

2. Pattern Reuse: JSON auto-parsing follows the proven pattern from node_wrapper.py (lines 746-781), ensuring consistency across the codebase.

3. Comprehensive Documentation: The updates to template-variables.md clearly explain the shell command limitations and provide safe alternatives (using stdin).

4. Safety Guardrails Preserved:

   • Simple templates (${items} alone) still block dict/list→str at runtime
   • Shell-unsafe JSON triggers warnings
   • Type mismatches for non-string targets still caught

5. Thorough Testing: 25 new tests for batch processing, updated type checker tests - solid coverage.

6. Excellent Bug Documentation: The bugfix-log entry is exemplary - clear repro steps, root cause analysis, and lessons learned.

---

⚠️ Critical Issues - Must Fix Before Merge

1. Shell Injection Risk in _warn_shell_unsafe_json() ⚠️ SECURITY

Location: src/pflow/nodes/shell/shell.py:306-345

Issue: The regex pattern r"\[.*?\]|\{.*?\}" with re.DOTALL can match across thousands of lines, causing:

• ReDoS (Regular Expression Denial of Service) on malicious input
• False positives matching unrelated brackets in long commands

Current Code:

json_pattern = r"\[.*?\]|\{.*?\}"
json_matches = re.findall(json_pattern, command, re.DOTALL)

Fix: Limit the pattern to single lines and add size constraints:

# Only match JSON-like patterns on a single line (no DOTALL)
# Limit to reasonable JSON snippet size (500 chars)
json_pattern = r"\[.{0,500}?\]|\{.{0,500}?\}"
json_matches = re.findall(json_pattern, command)

Why This Matters: Without this fix, a malicious workflow with a very long command could hang the validator.

---

2. Type Checker Matrix Inconsistency

Location: src/pflow/runtime/type_checker.py:30-58

Issue: The type compatibility matrix allows dict → str and list → str, but the comment says "dict can serialize to JSON string" when it should say "dict/list auto-parse FROM JSON strings when coming FROM str".

Current Comment (line 11):

# Note on str → dict/list compatibility (auto-parse feature):

The matrix (lines 54-57):

"dict": ["any", "dict", "object", "str", "string"],  # dict can serialize to JSON string
"list": ["any", "list", "array", "str", "string"],   # list can serialize to JSON string

Issue: This creates confusion - the matrix allows BOTH directions:

• str → dict/list (auto-parse JSON strings) ✅ Correct
• dict/list → str (serialize to JSON) ✅ Also correct now

But the comments don't clarify this is bidirectional. This needs clearer documentation.

Fix: Update the comments to be explicit:

"dict": ["any", "dict", "object", "str", "string"],  # dict ↔ str bidirectional (serialize/parse JSON)
"list": ["any", "list", "array", "str", "string"],   # list ↔ str bidirectional (serialize/parse JSON)

---

3. Batch Node JSON Parsing Size Limit Too High

Location: src/pflow/runtime/batch_node.py:247

Issue: 10MB limit for JSON parsing is very generous and could cause memory issues or slow parsing on malicious input.

Current Code:

MAX_JSON_SIZE = 10 * 1024 * 1024  # 10MB limit

Recommendation: Consider 1MB (1024 * 1024) instead. Batch items with 10MB of JSON are likely doing something wrong. If there's a legitimate use case for 10MB arrays, document it in a comment.

Suggested Change:

# Reasonable limit for batch items JSON
# Larger arrays should be processed from files, not inline JSON
MAX_JSON_SIZE = 1 * 1024 * 1024  # 1MB limit

---

⚠️ Warnings - Should Be Addressed

4. Inconsistent Validation Between Template Validator and Runtime

Location: Multiple files

Issue: The PR adds runtime JSON auto-parsing but the template_validator.py static validation doesn't know about batch wrappers. This was documented in the pitfalls file, but it's not fixed.

Evidence: From .taskmaster/knowledge/pitfalls.md:360+:

Template validation happens at compile-time using static analysis. It has no knowledge of runtime wrappers that transform output structures.

Impact: Valid batch workflows might fail validation with confusing errors like:

• ${item} reported as "undefined input"
• ${node.results} fails with suggestion to use ${node.response}

Status: Documented in pitfalls but not fixed in this PR.

Recommendation: Either:

1. Fix the validation in this PR (add batch-aware output detection to template_validator.py)
2. File a follow-up issue and link it in the PR description

---

5. Shell Node Warning Is Too Noisy

Location: src/pflow/nodes/shell/shell.py:336-345

Issue: The warning triggers on the FIRST unsafe char and returns immediately. This is good for avoiding spam, but the warning message doesn't indicate this is just one of potentially many issues.

Current Code:

logger.warning(
    f"Command contains JSON with {name} ('{char}') which may break shell parsing. "
    f"Consider using 'stdin' parameter instead for reliable data passing.",
    ...
)
return  # Only warn once per command

Suggestion: Make it clear this is a general check:

logger.warning(
    f"Command contains JSON with shell-unsafe characters (found: {name} '{char}'). "
    f"This may break shell parsing. Consider using 'stdin' parameter instead.",
    ...
)
return  # Only warn once per command (multiple issues may exist)

---

6. Test Naming Inconsistency

Location: tests/test_runtime/test_batch_node.py

Issue: New test classes use different naming patterns:

• TestItemsJsonAutoParsing (10 tests) - Clear
• TestConfigTypeCoercion (15 tests) - Clear

But existing tests use:

• TestBatchExecution
• TestInputOutputFormats

Minor Issue: Not critical, but consistency helps navigation. Consider prefixing all test classes with TestBatch for easy filtering.

---

7. Boolean Coercion Edge Case

Location: src/pflow/runtime/batch_node.py:144-157

Issue: The boolean coercion handles "yes"/"no" but not "y"/"n" or "1"/"0" (numeric). This might surprise users.

Current Code:

if lower in ("true", "1", "yes"):
    return True
if lower in ("false", "0", "no", ""):
    return False

Observation: This is actually fine - it handles the common cases. Just noting it for future reference if users request "y"/"n" support.

---

💡 Suggestions - Optional Improvements

8. Consider Extracting Shell Safety Checks to Utility Module

Location: src/pflow/nodes/shell/shell.py

Observation: The shell node now has THREE safety check methods:

• _check_command_template_safety() (lines 257-305)
• _warn_shell_unsafe_json() (lines 306-345)
• _adapt_stdin_to_string() (lines 347-421)

Suggestion: These could be extracted to src/pflow/nodes/shell/safety_utils.py for:

• Better testability (unit test the safety checks in isolation)
• Reusability if other nodes need similar checks
• Cleaner shell.py file (currently 760 lines)

Not Blocking: This can be a follow-up refactor.

---

9. Add Integration Test for Real-World Workflow

Location: tests/test_integration/

Suggestion: The webpage-to-markdown-parallel.json workflow is a great real-world example. Consider adding an integration test that:

1. Mocks the HTTP fetch to return known HTML
2. Validates the shell→batch→LLM flow works
3. Verifies JSON array parsing happens correctly

Value: Ensures the real use case that motivated this PR doesn't regress.

---

10. Document the Bidirectional Nature in Architecture Docs

Location: architecture/reference/template-variables.md

Observation: The doc update explains shell command limitations well, but doesn't explicitly call out that the type system is now bidirectional for dict/list ↔ str.

Suggested Addition (after line 602):

#### Bidirectional Type Compatibility

The type system now supports bidirectional conversion between structured types and strings:

- **dict/list → str**: Automatically serializes to JSON (e.g., for embedding in commands)
- **str → dict/list**: Automatically parses JSON (e.g., shell output into batch items)

Both directions are validated at compile-time and executed at runtime.

---

📊 Test Quality Assessment

Test Coverage: ✅ Excellent

• 25 new batch processing tests
• Updated type checker tests to reflect bidirectional compatibility
• Edge cases covered (None values, empty arrays, large JSON)

Test Clarity: ✅ Good

• Test names clearly describe what they verify
• Good mix of positive and negative cases

Potential Gaps:

1. No test for the ReDoS scenario in _warn_shell_unsafe_json() (see Critical Issue docs: Set up Mintlify documentation (Task 93) #1)
2. No test verifying the 10MB limit in batch JSON parsing actually works
3. No integration test for the real-world webpage-to-markdown workflow

---

🎯 Summary & Recommendation

Overall Assessment: This is a well-thought-out feature with good documentation and testing. The core design is sound, but there are security and clarity issues that must be addressed.

Before Merge:

1. ✅ Must Fix: ReDoS vulnerability in _warn_shell_unsafe_json() (Critical docs: Set up Mintlify documentation (Task 93) #1)
2. ✅ Must Fix: Clarify bidirectional nature in type_checker.py comments (Critical feat: unify LLM usage via Simon Willison's llm library (Task 95) #2)
3. ⚠️ Should Fix: Decide on batch JSON size limit (10MB vs 1MB) and document reasoning (Critical feat: Smart output display for registry run #3)
4. ⚠️ Should Fix: Either fix batch validation or file follow-up issue (Warning feat: add smart output display for registry run (closes #3) #4)

After Addressing Critical Issues: ✅ Approve and Merge

The suggestions and warnings are valuable for follow-up work but shouldn't block the PR.

---

💬 Questions for Author

1. What's the largest batch items JSON you've seen in practice? Does 10MB limit have a real use case?
2. Is the batch validation issue (Warning feat: add smart output display for registry run (closes #3) #4) being tracked separately, or should we fix it in this PR?
3. Did you consider extracting shell safety checks to a separate module (Suggestion feat: detect API keys from env vars and pflow settings #8)?

---

Great work on solving a real problem with a thoughtful approach! 🚀

[spinje]

Review Fixes Applied

Thanks for the thorough review! I've addressed the critical issues in commit 1027630:

✅ Fixed

#1 ReDoS Vulnerability (Critical)

• Changed regex from r"\[.*?\]|\{.*?\}" with re.DOTALL to r"\[.{0,500}?\]|\{.{0,500}?\}"
• Removed DOTALL flag - now only matches within single lines
• Added 500 char limit to prevent catastrophic backtracking
• Verified: 10,000 char command completes in <1ms

#2 Type Checker Comment Clarity (Critical)

• Added explicit "BIDIRECTIONAL JSON compatibility" header
• Documented both directions with examples:
  • str → dict/list (auto-parse)
  • dict/list → str (auto-serialize)
• Updated inline comments to use ↔ notation

#5 Warning Message Clarity (Warning)

• Changed to: "shell-unsafe characters (found: {name} '{char}')"
• Added comment: # Only warn once per command (multiple issues may exist)

---

❌ Not Addressed (Pre-existing, out of scope)

#3 Batch Node 10MB Limit

• This is pre-existing code, not introduced in this PR
• Would require separate discussion about appropriate limits

#4 Batch Validation Inconsistency

• Already documented in .taskmaster/knowledge/pitfalls.md
• Tracked as known limitation, not a regression from this PR

---

Answers to Questions

What's the largest batch items JSON you've seen in practice?

In the webpage-to-markdown workflow, batch items are image descriptions - typically a few KB each. 10MB would be extreme. However, since this limit is pre-existing, I'd prefer to address it in a separate PR if we want to change it.

Is the batch validation issue being tracked separately?

Yes, documented in pitfalls.md (lines 360+). It's a fundamental limitation of static validation vs runtime wrappers.

Did you consider extracting shell safety checks?

Good suggestion for follow-up. The shell.py file is getting large (760 lines). A safety_utils.py extraction would improve testability.