feat: implement if conditional operation for Phase 3 (#32)

[andreasronge]

Summary

Implements the if conditional operation for Phase 3, enabling branching logic in DSL programs.

Changes

• operations.ex: Added eval("if", ...) following the let pattern for nested expression evaluation
• validator.ex: Added validation for required fields (condition, then, else) with recursive validation of nested expressions
• ptc_runner_test.exs: Added 23 comprehensive tests covering:
  • Truthiness semantics (false/nil are falsy, everything else including 0, [], "", {} is truthy)
  • Nested conditions
  • Comparisons with piped input
  • Error propagation from condition expressions
  • Validation errors for missing fields
  • E2E scenarios with pipes and complex conditions

Test Plan

• All 169 tests pass (3 doctests + 166 unit/e2e tests)
• Tests verify all acceptance criteria from the issue
• Error propagation from condition expressions tested
• Variable bindings from outer let accessible in if branches

Addresses Review Feedback

✓ Truthiness edge cases covered ([], "", 0, {} are all truthy)
✓ Error propagation from condition expression tested
✓ Variables from outer scopes accessible in branches
✓ Nested conditions supported
✓ Proper context passing for piped input in comparisons

[github-actions]

PR Review: feat: implement if conditional operation for Phase 3 (#32)

Summary

This PR implements the if conditional operation for Phase 3, adding branching logic to the DSL. The implementation correctly follows established patterns (let operation) and provides comprehensive test coverage. Overall a solid implementation that meets all requirements from issue #32.

What's Good

• Correct truthiness semantics: Only false and nil are falsy, following Elixir semantics as specified in architecture.md:267-269
• Follows established patterns: The eval("if", ...) implementation mirrors the let pattern for nested expression evaluation
• Comprehensive tests: 23 tests covering truthiness edge cases (0, [], "", {}), validation errors, error propagation, nested conditions, and integration with pipe and let
• Proper error propagation: Errors in condition expression correctly short-circuit evaluation
• Variable scope preserved: Tests verify that outer let bindings are accessible in if branches

Issues (Must Fix)

1. Duplicate test - test/ptc_runner_test.exs:2063-2078 and test/ptc_runner_test.exs:2220-2235
   • Problem: "if with nested if conditions" and "E2E: nested if conditions" are exactly identical - same program, same assertion
   • Impact: Test redundancy; inflates test count without adding coverage
   • Fix: Remove one of the duplicate tests
   • Complexity: Mechanical (delete 13 lines)

Suggestions (Optional)

1. Simplify nested with in validation - lib/ptc_runner/validator.ex:77-81
   • Suggestion: The triple-nested with could be flattened to a single with chain for consistency with Elixir idioms:

     with :ok <- validate_node(Map.get(node, "condition")),
          :ok <- validate_node(Map.get(node, "then")) do
       validate_node(Map.get(node, "else"))
     end

   • Benefit: More idiomatic, matches pattern in let validation
   • Complexity: Mechanical style change
   • Recommendation: Low priority, current code is functionally correct

Security

No concerns. The if operation evaluates expressions through the existing sandboxed interpreter and doesn't introduce any new attack surface.

Verdict

Request Changes - One duplicate test should be removed before merge. The validation style improvement is optional and can be addressed in a future cleanup.

[andreasronge]

@claude please fix these issues from PR review:

Issue 1: Duplicate test for nested if conditions

Location: test/ptc_runner_test.exs:2063-2078 and test/ptc_runner_test.exs:2220-2235
Problem: Two identical tests exist - "if with nested if conditions" and "E2E: nested if conditions" have the exact same program and assertion, inflating test count without adding coverage.
Fix: Remove the duplicate test at lines 2220-2235 ("E2E: nested if conditions"). Keep the first one at lines 2063-2078.

Please fix the issue above in a SINGLE commit. Make minimal changes to resolve the issue.

[andreasronge]

Auto-Triage Summary

Decisions Made

#	Issue	Decision	Action
1	Duplicate test (lines 2063-2078 and 2220-2235)	FIX_NOW	Included in fix request above
2	Nested with validation simplification (validator.ex:77-81)	IGNORE	Pattern is consistent with existing let validation (lines 66-68); marked optional/low-priority in review

Status

• FIX_NOW items: 1 (batched in single fix comment above)
• Issues created: 0
• Items ignored: 1

Rationale for IGNORE Decision

The nested with pattern in the if validation actually follows the same pattern used in the let validation (validator.ex:66-68), so it's consistent with the existing codebase. The reviewer marked this as optional and low priority, and since it doesn't improve consistency (it already is consistent), I've deferred this style preference.

[andreasronge]

Summary

I've successfully fixed the duplicate test issue:

What was done:

• Removed duplicate test at lines 2220-2235 ("E2E: nested if conditions")
• Kept the first test at lines 2063-2078 ("if with nested if conditions")

Both tests had identical programs and assertions, so removing the duplicate eliminates test count inflation without losing any coverage.

Verification:

• ✅ All 168 tests pass
• ✅ Format check passed
• ✅ Compilation check passed
• ✅ Credo analysis found no issues

Commit:

• Commit: 491c80b - "test: remove duplicate nested if conditions test"
• Branch: claude/implement-if-operation
• 1 file changed, 17 deletions (-)

[github-actions]

PR Review: feat: implement if conditional operation for Phase 3 (#32)

Summary

Implements the if conditional operation for Phase 3, enabling branching logic in DSL programs. The implementation correctly follows established patterns and fully addresses the issue requirements.

What's Good

• Correct implementation: The if operation follows the let pattern for nested expression evaluation, using Interpreter.eval directly for each branch
• Proper truthiness semantics: Only false and nil are falsy, matching Elixir semantics as specified in architecture.md:267-269
• Comprehensive test coverage: 23 tests covering all truthiness edge cases (0, [], "", {} are truthy), nested conditions, error propagation, validation errors, and E2E scenarios
• Error propagation: Properly handles errors from condition evaluation
• Context preservation: Piped input and let bindings work correctly in if branches
• All tests pass: 168 tests + 3 doctests pass

Issues (Must Fix)

None identified.

Suggestions (Optional)

1. Nested with blocks in validator - Nice-to-have
   • Location: lib/ptc_runner/validator.ex:77-81
   • Suggestion: The nested with blocks for validating if branches could be simplified to match the let pattern more closely:

     # Current (works but more nested)
     with :ok <- validate_node(Map.get(node, "condition")) do
       with :ok <- validate_node(Map.get(node, "then")) do
         validate_node(Map.get(node, "else"))
       end
     end
     
     # Could be (single with chain)
     with :ok <- validate_node(Map.get(node, "condition")),
          :ok <- validate_node(Map.get(node, "then")) do
       validate_node(Map.get(node, "else"))
     end

   • Benefit: More consistent with the let pattern and slightly more readable
   • Complexity: Trivial
   • Recommendation: Out of scope for this PR, could be addressed in a future cleanup

Security

No concerns. The implementation uses the existing sandboxed interpreter and does not introduce any new attack vectors.

Verdict

Approve - The implementation is correct, well-tested, and follows established patterns. The minor style suggestion for validator simplification is non-blocking and can be addressed separately.

[andreasronge]

Auto-Triage Summary - Cycle 2

Review Status

✅ PR Approved - All required fixes have been completed.

Previous Actions (Cycle 1)

• Fixed duplicate test issue (commit 491c80b)
• All 168 tests passing

Current Review Findings

#	Issue	Decision	Action
1	Nested with simplification (validator.ex:77-81)	IGNORE	Pattern is internally consistent with let validation; marked optional/out-of-scope by reviewer

Status

• FIX_NOW items: 0
• Issues created: 0
• Items ignored: 1

Rationale

The nested with pattern in if validation matches the pattern used in let validation (lines 66-68). Both operations use nested with blocks rather than chaining. Changing this would require refactoring both operations and is a pure style preference with no functional benefit. The reviewer marked this as optional and approved the PR.

Next Steps

Adding ready-to-merge label - PR is approved and all required changes are complete.