-
Notifications
You must be signed in to change notification settings - Fork 36
Fix #233: Loop mode treats all files as unit test files #260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes issue #233 where pdd fix --loop incorrectly treated the last file as an error file instead of a unit test file. The fix addresses Click's argument parsing behavior with nargs=-1 by recombining the error_file back into unit_test_files when in loop mode. Additionally, it enforces that --verification-program is required for loop mode.
Key changes:
- Fixed loop mode file parsing logic in
pdd/commands/fix.py - Added validation requiring
--verification-programfor loop mode - Updated documentation to clarify loop vs non-loop mode behavior
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pdd/commands/fix.py | Added argument validation and recombination logic for loop mode; enforces verification-program requirement |
| tests/test_commands_fix.py | Added two new tests verifying loop mode behavior and validation |
| context/commands/fix_example.py | Updated documentation to clarify loop mode requirements and remove error_file from loop example |
| README.md | Enhanced documentation with separate examples for loop and non-loop modes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Loop mode: ERROR_FILE is treated as an additional unit test file. All files | ||
| are test files. Errors are generated by running the verification program. | ||
| Non-loop mode: UNIT_TEST_FILES are test files, ERROR_FILE contains static errors. |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring incorrectly states "ERROR_FILE is treated as an additional unit test file" in loop mode. According to the code logic (lines 108-111), ERROR_FILE is recombined into unit_test_files and then set to empty string - it's not "treated as" a unit test file, it was incorrectly parsed as ERROR_FILE and is being corrected. Consider rephrasing to: "Loop mode: All arguments are unit test files. ERROR_FILE is recombined into UNIT_TEST_FILES (correcting Click's parsing behavior)."
| Loop mode: ERROR_FILE is treated as an additional unit test file. All files | |
| are test files. Errors are generated by running the verification program. | |
| Non-loop mode: UNIT_TEST_FILES are test files, ERROR_FILE contains static errors. | |
| Loop mode: All arguments are unit test files. ERROR_FILE is recombined into | |
| UNIT_TEST_FILES (correcting Click's parsing behavior), and errors are | |
| generated by running the verification program. | |
| Non-loop mode: UNIT_TEST_FILES are test files, and ERROR_FILE contains | |
| static errors. |
| ``` | ||
| pdd [GLOBAL OPTIONS] fix [OPTIONS] PROMPT_FILE CODE_FILE UNIT_TEST_FILE ERROR_FILE | ||
| pdd [GLOBAL OPTIONS] fix [OPTIONS] PROMPT_FILE CODE_FILE UNIT_TEST_FILES... [ERROR_FILE] |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The syntax [ERROR_FILE] suggests this argument is optional in all cases, but according to lines 1470-1471, ERROR_FILE is required in non-loop mode. The syntax should indicate this requirement more clearly, perhaps with a note or different formatting to distinguish between loop and non-loop mode requirements.
| pdd [GLOBAL OPTIONS] fix --output-code src/factorial_fixed.py --output-results results/fix_results.log \ | ||
| factorial_python.prompt src/factorial.py tests/test_factorial.py tests/test_edge_cases.py errors.log |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this explanation is helpful, it could be confusing because the command line shown on line 1500 doesn't make it obvious which file is ERROR_FILE versus a unit test file. Consider explicitly labeling the arguments in the command example or adding a comment showing which file serves which purpose.
| pdd [GLOBAL OPTIONS] fix --output-code src/factorial_fixed.py --output-results results/fix_results.log \ | |
| factorial_python.prompt src/factorial.py tests/test_factorial.py tests/test_edge_cases.py errors.log | |
| pdd [GLOBAL OPTIONS] fix \ | |
| --output-code src/factorial_fixed.py \ # OUTPUT_CODE (fixed code file) | |
| --output-results results/fix_results.log \ # OUTPUT_RESULTS (LLM + test results log) | |
| factorial_python.prompt \ # PROMPT_FILE | |
| src/factorial.py \ # CODE_FILE | |
| tests/test_factorial.py tests/test_edge_cases.py \# TEST_FILE(S) | |
| errors.log # ERROR_FILE (static error messages) |
Fixes the issue #233 where
pdd fix --loopincorrectly treats the last file as ERROR_FILE instead of a unit test file.Change
pdd/commands/fix.py--verification-programfor loop modeTest Files
tests/test_commands_fix.py- Added 2 new testsTests
test_fix_loop_mode_treats_all_files_as_unit_tests- PASSES (verifies all files treated as unit tests)test_fix_requires_verification_program_in_loop_mode- PASSES (verifies validation)Cause
The bug was in
pdd/commands/fix.py. Click's argument parsing withnargs=-1followed by a required positional argument causes the last file to be assigned toerror_file, even in loop mode where all files should be unit test files.The fix recombines
error_fileback intounit_test_fileswhen in loop mode, then setserror_file = ""(auto-generated by verification program)Also added that for --loop verification-program is needed