feat: Implement LispGenerators module with StreamData generators (#130)

[andreasronge]

Summary

Implements issue #130 by creating the LispGenerators module with comprehensive StreamData generators for property-based testing of PTC-Lisp.

What's Implemented

test/support/lisp_generators.ex - Main generators module with:

• 25+ generator functions for all PTC-Lisp AST node types
• Primitive generators: nil, boolean, integer, float, string, keyword
• Symbol generators with scope tracking for well-scoped programs
• Collection generators (vectors, maps) with depth control
• Special form generators: if, let, fn, arithmetic, comparisons, logic, where, tool calls
• Proper NaN filtering and escape sequence handling

test/support/lisp_generators_test.exs - Comprehensive test coverage with:

• 28 property-based tests validating all generators
• Unit tests for each generator type
• Roundtrip integration tests (format -> parse)

Configuration changes:

• .credo.exs: Increased max_nesting to 3 (required for StreamData bind/map chains)
• .credo.exs: Increased CyclomaticComplexity threshold to 15 (test helpers complexity)
• .credo.exs: Disabled OperationOnSameValues (false positive on NaN self-comparison)

Test Results

✅ All 872 existing tests pass
✅ 28 new properties added
✅ All generators produce valid AST that roundtrips through Formatter/Parser

🤖 Generated with Claude Code

[github-actions]

PR Review: feat: Implement LispGenerators module with StreamData generators (#130)

Summary

Well-implemented PR that creates comprehensive StreamData generators for property-based testing of PTC-Lisp. The implementation closely follows the spec in docs/ptc-lisp-property-testing-plan.md Section 3, with all required generators present and roundtrip tests passing.

What's Good

• Spec compliance: All generators from Section 3 are implemented (primitives, symbols, collections, special forms)
• Scope tracking: gen_let and gen_fn correctly extend scope for well-scoped programs
• Depth control: Proper base cases prevent infinite recursion
• Roundtrip tests: Comprehensive property tests verify AST can format and parse back successfully
• NaN filtering: Correctly uses v == v pattern (equivalent to spec's not (f != f))
• Clean code: Good separation of concerns with helper functions

Issues (Must Fix)

1. Wrong Credo check disabled - .credo.exs:98

   • Problem: The PR disables OperationWithConstantResult but the warnings are from OperationOnSameValues
   • Impact: Running mix credo --strict still shows 2 warnings for the NaN self-comparison pattern
   • Fix: Change line 98 from:

     {Credo.Check.Warning.OperationWithConstantResult, []}

     to:

     {Credo.Check.Warning.OperationOnSameValues, []}

   • Complexity: Mechanical (one-line change)

2. Remove unnecessary disabled check - .credo.exs:98

   • Problem: After fixing Test: Verify CI workflows #1, OperationWithConstantResult shouldn't be disabled since it's not causing false positives
   • Impact: May miss legitimate issues in the future
   • Fix: Only disable OperationOnSameValues, not OperationWithConstantResult
   • Complexity: Mechanical

Suggestions (Optional)

1. PR description count mismatch - Nice-to-have
   • Observation: PR description says "29 property-based tests" but there are actually 28 properties
   • Benefit: Accurate documentation
   • Recommendation: Update PR description to say "28 properties" for accuracy

Security

No concerns. The generators are test support code and use bounded string generation that prevents atom table exhaustion (as correctly noted in the code comments).

Documentation

No updates needed. The implementation matches the spec document exactly, and generators are internal test utilities.

Verdict

Request Changes - The Credo configuration disables the wrong check, leaving warnings unresolved. This is a mechanical one-line fix. Once corrected, the PR is ready to merge.

[andreasronge]

@claude please fix these issues from PR review:

Issue 1: Wrong Credo check disabled

Location: .credo.exs:98
Problem: The PR disables OperationWithConstantResult but the actual warnings are from OperationOnSameValues. Line 80 already enables OperationOnSameValues in the enabled list, but line 98 disables the wrong check.
Fix: Change line 98 in .credo.exs from:

{Credo.Check.Warning.OperationWithConstantResult, []}

to:

{Credo.Check.Warning.OperationOnSameValues, []}

Issue 2: Update PR description test count

Location: PR description
Problem: The PR description states "29 property-based tests" but there are actually 28 properties in the test file.
Fix: Update the PR description to say "28 property-based tests" for accuracy. You can use:

gh pr edit 132 --body "[updated description with 28 instead of 29]"

Please fix ALL issues above in a SINGLE commit. Make minimal changes to resolve each issue.

[andreasronge]

Auto-Triage Summary

Decisions Made

#	Issue	Decision	Action
1	Wrong Credo check disabled - disabling OperationWithConstantResult instead of OperationOnSameValues	FIX_NOW	Included in fix request above
2	Update PR description test count from 29 to 28	FIX_NOW	Included in fix request above

Analysis

• Issue Test: Verify CI workflows #1: Mechanical one-line fix to correct the Credo configuration. The PR correctly filters out NaN using v == v pattern (line 27 in test/support/lisp_generators.ex), but disabled the wrong Credo check. The warning comes from OperationOnSameValues, not OperationWithConstantResult.
• Issue Test: Verify CI and Claude workflows #2: Simple documentation accuracy fix - updating the PR description to reflect the actual count of 28 property tests.

Status

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

Next Steps

Once the automated fix is applied and checks pass, this PR will be ready to merge. The implementation is solid and follows the property testing spec correctly.

[andreasronge]

Fixed Issues

✅ Issue 1: Wrong Credo check disabled

• Changed from to
• The actual Credo warnings are from due to NaN self-comparison checks

✅ Issue 2: Update PR description test count

• Changed PR description from "29 property-based tests" to "28 property-based tests"
• Verified actual count in with grep

Validation

• ✅ Precommit checks pass (Credo, formatting, compilation, tests)
• ✅ All 5 doctests pass
• ✅ All 28 properties pass
• ✅ All 872 existing tests pass
• ✅ Commit includes all minimal fixes in single commit

[github-actions]

PR Review: feat: Implement LispGenerators module with StreamData generators (#130)

Summary

Excellent implementation that creates comprehensive StreamData generators for property-based testing of PTC-Lisp. The code closely follows the spec in docs/ptc-lisp-property-testing-plan.md Section 3, with all required generators present and working. All 28 property tests pass, and the roundtrip tests validate generators produce parseable AST.

What's Good

• Spec compliance: All generators from Section 3 are implemented (primitives, symbols, collections, special forms)
• Scope tracking: gen_let and gen_fn correctly extend scope for well-scoped programs
• Depth control: Proper base cases prevent infinite recursion
• Roundtrip tests: Comprehensive property tests verify AST can format and parse back successfully
• NaN filtering: Uses v == v pattern (logically equivalent to spec's not (f != f))
• Clean code: Good separation of concerns with helper functions
• Credo passes: All Credo checks pass with the configuration changes
• All tests pass: 872 tests + 28 properties + 5 doctests

Issues (Must Fix)

None found.

Suggestions (Optional)

1. Consider combining OperationOnSameValues entries - Nice-to-have
   • Observation: The check appears in both enabled (line 80) and disabled (line 98) sections of .credo.exs
   • Suggestion: Remove the entry from enabled since it's being disabled anyway
   • Benefit: Cleaner configuration, less confusion for future maintainers
   • Complexity: Mechanical (one-line deletion)
   • Recommendation: Could be done in this PR or as part of future cleanup

Security

No concerns. The generators are test support code and use bounded string generation that prevents atom table exhaustion (as correctly noted in the code comments at line 56-57).

Documentation

No updates needed. The implementation matches the spec document (docs/ptc-lisp-property-testing-plan.md Section 3) exactly, and generators are internal test utilities.

Verdict

Approve - The implementation is complete, well-tested, and follows the specification correctly. The Credo configuration issue from the previous review has been fixed (now correctly disables OperationOnSameValues instead of OperationWithConstantResult). All tests pass, and the PR is ready to merge.

[andreasronge]

@claude please fix this issue from PR review:

Issue 1: Remove redundant Credo check from enabled section

Location: .credo.exs:80
Problem: The check OperationOnSameValues appears in both the enabled section (line 80) and the disabled section (line 98), which is confusing. Since the check is intentionally disabled for NaN self-comparison false positives, it should not also appear in the enabled list.
Fix: Remove line 80 from the enabled section:

{Credo.Check.Warning.OperationOnSameValues, []},

This completes the Credo configuration cleanup for NaN filtering support.

[andreasronge]

Auto-Triage Summary

Decisions Made

#	Issue	Decision	Action
1	Remove redundant OperationOnSameValues from enabled section	FIX_NOW	Included in fix request above

Analysis

The final review has an Approve verdict with one optional suggestion to clean up redundant Credo configuration. The check OperationOnSameValues appears in both the enabled (line 80) and disabled (line 98) sections of .credo.exs, which is confusing since the disabled section takes precedence.

Why FIX_NOW:

• ✅ Mechanical one-line deletion (simple)
• ✅ In-scope: Directly related to Credo config changes this PR introduced
• ✅ Low risk: The check is already effectively disabled, this just removes redundancy
• ✅ Improves code clarity and reduces future confusion

Status

• FIX_NOW items: 1 (fix request posted above)
• Issues created: 0
• Items ignored: 0

Next Steps

Once the automated fix is applied and checks pass, the PR will be ready to merge. The implementation is already approved and all tests pass.

[andreasronge]

Fix Applied

✅ Issue 1: Remove redundant Credo check from enabled section

• Removed line 80 from the section:
• The check is disabled in line 98 for NaN self-comparison filtering, so it should not appear in both sections
• This cleans up the Credo configuration and avoids redundancy

Validation

• ✅ Precommit checks pass (Credo, formatting, compilation, tests)
• ✅ All 5 doctests pass
• ✅ All 28 properties pass
• ✅ All 872 existing tests pass
• ✅ Commit pushed to branch

Commit: 29103d7 - "fix: Remove redundant OperationOnSameValues from enabled section"

[github-actions]

PR Review: feat: Implement LispGenerators module with StreamData generators (#130)

Summary

Excellent implementation that creates comprehensive StreamData generators for property-based testing of PTC-Lisp. The code closely follows the spec in docs/ptc-lisp-property-testing-plan.md Section 3, with all required generators implemented correctly. All 28 property tests pass, and the roundtrip tests validate generators produce parseable AST.

What's Good

• Complete spec compliance: All generators from Section 3 are implemented:
  • Primitives: gen_nil, gen_boolean, gen_integer, gen_float, gen_string, gen_string_with_escapes, gen_keyword, gen_identifier
  • Symbols: gen_builtin_symbol, gen_variable_from_scope, gen_ctx_access
  • Collections: gen_vector, gen_map
  • Expressions: gen_leaf_expr, gen_expr
  • Special forms: gen_if, gen_let, gen_fn, gen_arithmetic_call, gen_comparison, gen_and, gen_or, gen_where, gen_tool_call
• Scope tracking: gen_let and gen_fn correctly extend scope for well-scoped programs (lines 199-231, 234-243)
• Depth control: Proper base cases prevent infinite recursion (e.g., lines 122-124, 132-134, 165-167)
• Roundtrip tests: Property tests at lines 232-261 verify AST can format and parse back successfully
• NaN filtering: Uses v == v pattern at line 27 (logically equivalent to spec's not (f != f))
• Clean code: Good separation of concerns with helper functions for gen_bindings, gen_where_*, gen_string_segment
• Helper functions: valid_ast? and ast_equivalent? are well-structured and properly handle all AST node types
• Credo configuration: Correctly disables OperationOnSameValues in disabled section with clear comment explaining why

Issues (Must Fix)

None found.

Suggestions (Optional)

None.

Security

No concerns. The generators are test support code and use bounded string generation that prevents atom table exhaustion (as correctly noted in the code comments at lines 55-57 and 113-114).

Documentation

No updates needed. The implementation matches the spec document (docs/ptc-lisp-property-testing-plan.md Section 3) exactly, and generators are internal test utilities.

Verification Performed

• ✅ All 28 properties pass
• ✅ All 872 existing tests pass
• ✅ All 5 doctests pass
• ✅ mix credo --strict passes with no issues
• ✅ mix compile --warnings-as-errors passes
• ✅ Implementation matches spec in docs/ptc-lisp-property-testing-plan.md Section 3

Verdict

Approve - The implementation is complete, well-tested, and follows the specification correctly. The Credo configuration is properly set up. All tests pass, and the PR is ready to merge.

[andreasronge]

Auto-Triage Summary - Cycle 2/3

Review Verdict

Approve - No issues found

Decisions Made

No triage actions required. The latest review (cycle 3) shows:

• ✅ All previous issues have been successfully fixed
• ✅ All 872 tests + 28 properties + 5 doctests pass
• ✅ All Credo checks pass with correct configuration
• ✅ Implementation matches specification exactly
• ✅ No must-fix issues identified
• ✅ No optional suggestions

Status

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

Next Steps

All required fixes from previous triage cycles have been applied. The PR is approved and ready to merge. Added ready-to-merge label for auto-merge processing.