fix(#244): keep pm logs visible without corrupting input#247
fix(#244): keep pm logs visible without corrupting input#247
Conversation
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
d5112d4| Triggered by: PR opened by @srausser
Branch: sam/issue-244-pm-interactive-logging | 2 files, +185/-10 | CI: MyPy Type Check pass 41s https://github.com/Q00/ouroboros/actions/runs/23671647666/job/68966253818
Scope: diff-only
HEAD checked: d5112d4
What Improved
pm_command()now restores the prior console logging state even on interruption, which avoids leaking the PM command's temporary logging mode into later CLI work._run_pm_interview()adds explicit progress panels for interview startup and next-question generation while correctly skipping the generation message when resuming with a pending saved question.- The new unit tests cover the key interactive logging behaviors: default suppression, debug override, interrupt restoration, new-session progress messages, and resume behavior.
Issue #244 Requirements
| Requirement | Status |
|---|---|
| the pasted response appears visually cut off or overwritten | MET — reviewed against current diff |
| the terminal can look frozen while the next question is being generated because there is no clean user-facing progress indicator | MET — reviewed against current diff |
Prior Findings Status
| Prior Finding | Status |
|---|---|
| First review | MAINTAINED — no previous findings to re-check |
Blockers
| # | File:Line | Severity | Confidence | Finding |
|---|
Follow-ups
| # | File:Line | Priority | Confidence | Suggestion |
|---|
Test Coverage
- Adequate for the diff: the added tests exercise the newly introduced logging-state transitions and progress-message branches in
pm.py. - I could not execute the tests in this environment because
pytestis not installed, so this assessment is based on static review of the test code.
Design Notes
- The design is appropriate for the PR goal: console log suppression remains a temporary command-scoped concern in
pm_command(), while user-facing progress feedback stays inside the interview loop via Rich panels instead of structured logs. - Using
finallyto restore the prior global logging flag is the right containment boundary for this diff and avoids coupling interview internals to logging teardown.
Merge Recommendation
- Ready to merge if maintainers are satisfied with CI and project-level expectations.
Files Reviewed
src/ouroboros/cli/commands/pm.pytests/unit/cli/test_pm_interactive_logging.py
ouroboros-agent[bot]
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
567ceb9| Triggered by: new push by @srausser
Branch: sam/issue-244-pm-interactive-logging | 2 files, +198/-2 | CI: passing
Issue #244 Requirements
| Requirement | Status |
|---|---|
| Linked issue referenced in PR body | MET — PR body references #244. |
Previous Review Follow-up
| Previous Finding | Status |
|---|---|
| Prior bot review exists | Re-checked against current head 567ceb9; see blocking findings below for any still-open issues. |
Blocking Findings
| # | File:Line | Severity | Confidence | Finding |
|---|---|---|---|---|
| <your findings here, minimum 5, with specific file:line references> |
Test Coverage
Design Notes
Follow-up Recommendations
- Re-run this review after any follow-up push so the findings can be checked against the new PR head.
- If any blocking items are intentionally deferred, document that decision in the PR body before requesting approval.
Files Reviewed
src/ouroboros/cli/commands/pm.pytests/unit/cli/test_pm_interactive_logging.py
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
Follow-up to the automated review above: the current blocking issue on head Specifically:
So the CLI still advertises The multiline-input and progress-indicator changes otherwise look directionally correct for #244. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
0e7ebba| Triggered by: new push by @srausser
Branch: sam/issue-244-pm-interactive-logging | 2 files, +223/-2 | CI: MyPy Type Check pass 43s https://github.com/Q00/ouroboros/actions/runs/23687643747/job/69009703757
Issue #N Requirements
| Requirement | Status |
|---|---|
| ## Summary | Refer to review findings for verification against current diff. |
Interactive ouroboros pm sessions write structured logs into the terminal while the user |
Refer to review findings for verification against current diff. |
| ## Repro | Refer to review findings for verification against current diff. |
1. Configure ouroboros and run ouroboros pm in an interactive terminal (especially ove |
Refer to review findings for verification against current diff. |
| 2. Paste a multi-paragraph opening response. | Refer to review findings for verification against current diff. |
Previous Review Follow-up
| Previous Finding | Status |
|---|---|
| ## Review — ouroboros-agent[bot] | Re-checked against commit 0e7ebba in this review. |
| Verdict: REQUEST_CHANGES | Re-checked against commit 0e7ebba in this review. |
> Reviewing commit 567ceb9 |
Triggered by: new push by @srausser |
Branch: sam/issue-244-pm-interactive-logging |
2 files, +198/-2 |
| ### Issue #244 Requirements | Re-checked against commit 0e7ebba in this review. |
Blocking Findings
| # | File:Line | Severity | Confidence | Finding |
|---|---|---|---|---|
| 1 | src/ouroboros/cli/commands/pm.py:330 | High | Medium | _multiline_prompt_async() wraps the prompt in patch_stdout(raw=True), but Ouroboros' structured logger writes console logs to stderr, not stdout. That means the new prompt wrapper still does not protect the interactive prompt from the debug logs this PR is trying to restore, so the main issue can still reproduce when --debug is enabled. |
| 2 | tests/unit/cli/test_pm_interactive_logging.py:88 | Medium | High | The new prompt test hard-codes patch_stdout(raw=True) as the expected mechanism, but it never validates the stream the application actually logs to. As written, the test suite will pass even if debug logs continue to corrupt the prompt, so it gives false confidence on the PR’s primary behavior change. |
| 3 | tests/unit/cli/test_pm_interactive_logging.py:119 | Medium | Medium | The new-session test stubs engine.record_response() and engine.complete_interview() to return the same mutable state object the CLI already holds. That masks whether _run_pm_interview() is correctly honoring returned state values, so the test will not catch regressions in completion/save behavior if the engine starts returning a replacement InterviewState. |
| 4 | tests/unit/cli/test_pm_interactive_logging.py:153 | Low | High | The resume-path test only covers the /done exit from an already-persisted pending question. It does not exercise the key interruption case this PR comments about: saving a pending question, receiving an interrupt, and then confirming --resume re-displays that same question cleanly. |
| 5 | src/ouroboros/cli/commands/pm.py:309 | Low | High | This PR forks a second copy of _multiline_prompt_async() instead of reusing or aligning the existing implementation in src/ouroboros/cli/commands/init.py. The two copies have already diverged on logging/prompt behavior, which materially raises the chance that future fixes land in one CLI path but not the other. |
Test Coverage
tests/unit/cli/test_pm_interactive_logging.py improves direct coverage for the new prompt helper, debug flag wiring, and the “resume without regenerating” branch, but there are still important gaps:
- No test demonstrates that log output arriving on
stderris rendered above the prompt without corrupting input state. - No test covers the interrupt/resume contract described in
src/ouroboros/cli/commands/pm.py:448-455. - No test verifies behavior when
PromptSession.prompt_async()raisesEOFErrororKeyboardInterruptinside_multiline_prompt_async(). - I could not execute the test file in this environment because
pytestis not installed, so this review is based on static verification only.
Design Notes
The contributor improved two things relative to the prior review context: --debug logging is re-enabled in pm_command(), and the PM CLI now mirrors the richer multiline input UX already used elsewhere. The remaining design issue is that the fix is split between duplicated prompt helpers and a stream-patching assumption that does not match the repo’s logging backend. That leaves the implementation fragile: the UI path and the logging path are being fixed independently, so the PR restores more output but still does not verify that the restored output is routed through the same stream the prompt layer protects.
Follow-up Recommendations
- Re-run the review after the next push if any blocking findings are addressed.
- If a finding reflects intentional behavior, add targeted tests or documentation in the touched area.
Files Reviewed
src/ouroboros/cli/commands/pm.pytests/unit/cli/test_pm_interactive_logging.py
Reviewed by ouroboros-agent[bot] via Codex deep analysis
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Branch: sam/issue-244-pm-interactive-logging | 5 files, +312/-46 | CI: all required checks passing
Scope: diff-only
HEAD checked: 4acacda
What Improved
- The multiline prompt implementation is now shared between
initandpm, which removes the earlier duplication and keeps the interactive input behavior aligned across both entrypoints. - The PM flow now keeps explicit user-facing progress messages (
Starting interview...,Generating next question...) while preserving pending questions for resume flows. - The state-handling fixes now honor replacement state objects returned by both
record_response()andcomplete_interview(), which closes the stale-state concern from the prior review. - The new tests cover prompt stream proxying, debug-mode logging, resume-with-pending-question behavior, and interrupted-session recovery.
Issue #244 Requirements
| Requirement | Status |
|---|---|
| Interactive PM mode should keep pasted multiline input usable instead of letting live terminal output corrupt the prompt UI. | MET — the shared prompt helper now wraps PromptSession.prompt_async() in patch_stdout(raw=True) and the corresponding unit test verifies both sys.stdout and sys.stderr are proxied during input (src/ouroboros/cli/formatters/prompting.py:12, tests/unit/cli/formatters/test_prompting.py:14). |
| PM mode should show explicit progress feedback while the session starts and while the next question is generated. | MET — _run_pm_interview() emits progress messages before ask_opening_and_start() and ask_next_question(), while correctly skipping the generation message when resuming an already-persisted pending question (src/ouroboros/cli/commands/pm.py:378, src/ouroboros/cli/commands/pm.py:398, tests/unit/cli/test_pm_interactive_logging.py:72, tests/unit/cli/test_pm_interactive_logging.py:120). |
| Resume behavior should preserve and re-display a pending unanswered question after interruption. | MET — the PM loop persists a synthetic unanswered round before waiting for input, and the interrupt/resume test verifies the saved question is re-displayed on resume (src/ouroboros/cli/commands/pm.py:406, src/ouroboros/cli/commands/pm.py:421, tests/unit/cli/test_pm_interactive_logging.py:150). |
Prior Findings Status
| Prior Finding | Status |
|---|---|
Prompt protection only covered stdout, so restored debug logs could still cut through the active prompt. |
WITHDRAWN — the new helper explicitly relies on patch_stdout(raw=True) for both stdout and stderr, and the added unit test asserts both streams are proxied during the prompt (src/ouroboros/cli/formatters/prompting.py:33, tests/unit/cli/formatters/test_prompting.py:18). |
The PM command still duplicated multiline prompt logic instead of sharing one implementation with init. |
WITHDRAWN — both commands now import and use the shared helper from ouroboros.cli.formatters.prompting (src/ouroboros/cli/commands/init.py:26, src/ouroboros/cli/commands/pm.py:22). |
The tests did not prove the CLI honored replacement state returned by record_response() / complete_interview(). |
WITHDRAWN — the new tests use replacement state objects and assert the final save occurs with the returned state, matching the updated runtime logic (src/ouroboros/cli/commands/pm.py:439, src/ouroboros/cli/commands/pm.py:455, tests/unit/cli/test_pm_interactive_logging.py:76, tests/unit/cli/test_pm_interactive_logging.py:123). |
| Resume coverage did not exercise the interruption case that persists a pending question and re-displays it later. | WITHDRAWN — test_run_pm_interview_preserves_pending_question_across_interrupt_and_resume now covers that contract directly (tests/unit/cli/test_pm_interactive_logging.py:150). |
Blockers
| # | File:Line | Severity | Confidence | Finding |
|---|
Follow-ups
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | tests/unit/cli/test_pm_interactive_logging.py:150 |
Low | 74% | The targeted test run passes, but it emits an unraisable RuntimeWarning about an AsyncMock coroutine never being awaited. It would be worth tightening this test setup in a follow-up so the interactive-path tests stay warning-clean under pytest. |
Test Coverage
- I ran the targeted unit coverage for the touched interactive paths:
uv run pytest tests/unit/cli/formatters/test_prompting.py tests/unit/cli/test_pm_interactive_logging.py tests/unit/cli/test_init_runtime.py -q→10 passed. - Coverage for the diff is solid: prompt stream proxying, PM debug flag behavior, progress messaging, replacement-state handling, and interrupt/resume persistence are all exercised by the added tests.
- The only follow-up I noticed from execution was a non-failing pytest
RuntimeWarningabout an unawaitedAsyncMockin the interactive logging test module.
Merge Recommendation
- Ready to merge. The current HEAD addresses the previously raised concerns, the linked issue requirements are met in the diff, and the targeted tests passed on the reviewed commit.
Files Reviewed
src/ouroboros/cli/commands/init.pysrc/ouroboros/cli/commands/pm.pysrc/ouroboros/cli/formatters/prompting.pytests/unit/cli/formatters/test_prompting.pytests/unit/cli/test_pm_interactive_logging.py
ouroboros-agent[bot]
Summary
ouroboros pmsessions instead of hiding them by default--debugmeaningful again by enabling verboseDEBUG-level logging for the PM sessioninitandpmpatch_stdout(raw=True)so live logs do not cut through the input bufferrecord_response()andcomplete_interview()Testing
uv run ruff check src/ tests/uv run pytest tests/unit/cli/formatters/test_prompting.py tests/unit/cli/test_pm_interactive_logging.py tests/unit/cli/test_pm_brownfield.py tests/unit/cli/test_pm_overwrite.py tests/unit/cli/test_pm_select_repos.py tests/unit/cli/test_init_runtime.py -qCloses #244