-
Notifications
You must be signed in to change notification settings - Fork 48
Add steerable option to pdd sync + fix textual sync animation resize issues #267
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 introduces interactive steering to the PDD sync process via a new --steer flag (disabled by default), and resolves TUI corruption issues caused by terminal resizes by implementing a fixed-width rendering approach during sync runs.
Key changes:
- Added optional interactive steering allowing users to override sync operation choices at decision points
- Implemented
--steerCLI flag (opt-in) to enable steering without changing default behavior - Fixed TUI resize-related visual corruption by freezing animation/layout width at mount time
Reviewed changes
Copilot reviewed 179 out of 184 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
pdd/sync_tui.py |
Added steering infrastructure (ChoiceScreen, maybe_steer_operation), fixed resize handling with frozen UI width, added on_resize handler |
pdd/sync_orchestration.py |
Integrated steering calls into sync loop, added graceful handling for mock exhaustion in tests |
pdd/commands/maintenance.py |
Added --steer flag with environment variable control |
tests/test_sync_tui.py |
New comprehensive test file with unit tests and Z3 formal verification of steering logic |
tests/test_sync_orchestration.py |
Added missing imports for resize logic testing |
tests/test_llm_invoke.py |
Made model ID matching more robust with fallback candidates and graceful skipping |
pdd/fix_code_loop.py |
Fixed pipe streaming to use readline() instead of single-byte reads, reordered thread joins |
pdd/agentic_fix.py |
Added harvest-first strategy, updated claude binary resolution with shutil.which |
README.md |
Documented --steer flag and interactive steering behavior |
| Various metadata/backup files | Test run results and backup snapshots |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gltanaka
left a comment
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.
please clean out the temp files to make the review easier
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.
don't commit these temp files
gltanaka
left a comment
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.
Thanks for your work. Can you also submit your prompt changes to the pdd_cap?
README.md
Outdated
| - `--skip-tests`: Skip unit test generation and fixing | ||
| - `--target-coverage FLOAT`: Desired code coverage percentage (default is 90.0) | ||
| - `--dry-run`: Display real-time sync analysis for this basename instead of running sync operations. This performs the same state analysis as a normal sync run but without acquiring exclusive locks or executing any operations, allowing inspection even when another sync process is active. | ||
| - `--steer`: Enable interactive steering during the sync process. When enabled, PDD may pause at key decision points (e.g., choosing the next operation) and allow you to select between multiple valid options instead of automatically choosing one. |
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.
Is this a countdown timer or does it just stop until action is taken?
I think having it as a countdown timer might be better?
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.
it uses a countdown timer with a default value of 8 seconds. Currently hardcoded, but I'll work on making it configurable via CLI
calculator.py
Outdated
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.
should you put this example in examples/ dir instead of root?
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 change right now is in the context/ dir with other examples, which seems to be the default. Should I move it elsewhere?
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.
maybe make steer the default and have no steer if the user doesn't want delays
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.
sure, I'll make that change
| @@ -54,11 +54,6 @@ You are operating in an isolated git worktree at: {worktree_path} | |||
| This worktree is already checked out to branch `fix/issue-{issue_number}`. | |||
| Do NOT create a new branch - just stage, commit, and push. | |||
|
|
|||
| % Files to Stage | |||
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.
what is the purpose of this removal?
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 LLM did this at some point as part of syncing sync_tui. I didn't know what the intention was, but does it seem like an error to you?
project_dependencies.csv
Outdated
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.
use the latest pdd version so that this is hashes vs. timestamps
|
Just submitted the prompt changes to pdd_cap as a PR in the branch |
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
Copilot reviewed 14 out of 15 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi can you resolve the git conflict? I'll test this bug fix. |
|
target 1/20 |
|
@beknobloch It looks like there are still conflicts |
|
Hi. We refactored agentic_fix.py just yesterday to use the agentic_common.py module so seems like there is git conflict again. I can help with resolving git conflict and testing if you can kindly note why agentic fix was updated and what changes we're expected to keep. Thanks |
|
Hi! The change to agentic_fix.py was a tentative change I made while debugging sync - it's unconnected with this pull request, so I accepted your refactors and removed the changes I made in my commit. Sorry for the confusion there. |
|
Hi I am getting a compilation error because there was a missing import. Would you fix it or would you mind if I pushed the fix? The patch for sync_orchestration.py only shows:
Missing: from .sync_tui import DEFAULT_STEER_TIMEOUT_S This would cause exactly the NameError you saw. The fix I applied to your branch adds the missing import: from .sync_tui import maybe_steer_operation, DEFAULT_STEER_TIMEOUT_S You may want to comment on PR #267 about this bug, or the PR author needs to fix it before it can be merged. To test your fixed local version, reinstall from the local directory: pip install -e . |
|
Good catch, I had made an env variable so I didn't notice this bug. You can go ahead and push the fix. Thanks so much! |
The original PR #267 was missing this import, causing a NameError when the module is loaded. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The original PR promptdriven#267 was missing this import, causing a NameError when the module is loaded. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
======================== short test summary info ========================= |
|
I pushed the fix for missing import. However, I am getting unit tests errors. Does this PR pass all unit tests and regressions? |
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
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tests/test_sync_tui.py:1
- Use
Anyfrom typing instead of lowercaseanyfor type hints.
import pytest
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The original PR promptdriven#267 was missing this import, causing a NameError when the module is loaded. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1f71174 to
51d7843
Compare
I just fixed the remaining issues caused by the merge conflict resolutions, did a full rebase, and reran unit tests and regression tests, and all pass. These issues should be resolved and the PR good to merge. |
|
Somehow I am getting conflicts: ```=========================== short test summary info ============================ |
…nd ignore vertical terminal resizes
…nd ignore vertical terminal resizes
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… user to choose how long `sync` waits for user steering before progressing with the default option chore(steer): changed --steer to --no-steer, making steering the default behavior of `sync` test(sync): added additional tests to verify the functionality of steering and related edge cases
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The original PR promptdriven#267 was missing this import, causing a NameError when the module is loaded. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…nflict resolution
- Add pdd/prompts/*.prompt to .gitignore (local convenience files) - Add temporary files to .gitignore (error_log.txt, *.backup) - Fix duplicate --local flag handling in regression.sh - Add max_output_tokens field to ModelInfo in prompts.py
…dividual path/color references instead of dictionaries. Update _detect_mtime_changes function in agentic_fix.py to handle new, modified, and deleted files more effectively. Introduce run_agentic_task wrapper for better testability. Adjust fix_code_loop to incorporate prior costs. Enhance error handling in SyncLock class for improved robustness.
…M invocations - Added timeout handling for SSH session simulations in test_e2e_subprocess_issue_399_ssh_url_message.py to skip tests on timeout. - Improved error handling in test_llm_invoke_integration.py to skip tests when Vertex AI API is unavailable. - Updated comments for clarity on potential issues during test execution.
- Deleted backup files for agentic_fix and comment_line modules, including code and program files. - Cleaned up unnecessary temporary files to streamline the project structure.
- Updated sync_orchestration.py to streamline decision handling, replacing StopIteration with a direct all_synced decision for improved workflow completion. - Introduced a _debug_swallow function in sync_tui.py to better manage non-critical exception logging, enhancing robustness in UI interactions. - Modified test_sync_orchestration.py to verify the new decision handling and updated test_sync_tui.py for improved environment variable restoration during tests. - Adjusted .gitignore to include additional staging log files and ensure proper exclusion of temporary files.
…o steering-sync
- Introduced logging at the beginning of the sync_orchestration function to capture the start of the synchronization process. - Implemented a try-except block to ensure that the sync proceeds even if logging setup fails, enhancing robustness.
|
Okay! I resolved all the issues you noted and also verified the following:
Let me know if the auto merge succeeds on your end - I'm pretty sure the unit failures are unrelated to this PR (I'm looking into them on another branch) but I'm not certain. |
The original PR promptdriven#267 was missing this import, causing a NameError when the module is loaded. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The original PR promptdriven#267 was missing this import, causing a NameError when the module is loaded. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
feat: Modified
pdd syncto be steerable. Before taking an action, the TUI will present the user with a list of options, including the default option thatpdd syncrecommends. If no action is selected after a certain timeout length, the default option will be chosen automatically. Steering may be disabled with a new CLI option--no-steer, and the timeout length may be adjusted with a new option--steer-timeout <FLOAT>.fix: Modified
pdd syncTUI behavior to ignore resizes, fixing a previous issue which caused visuals to become corrupted and unreadable when the terminal window was resized.Test Results
Manual Testing
Manually tested steering, disabling steering, and window resizing to verify stability and usability.
Fixes #169
Fixes #236