fix(pm): auto-generate PM document on interview completion#248
Closed
fix(pm): auto-generate PM document on interview completion#248
Conversation
Remove auto-answer + recursion for DECIDE_LATER classified questions in PMInterviewEngine.ask_next_question(). Questions are now returned to the main session with classification metadata, allowing users to explicitly choose "decide later" via the UI. This eliminates infinite recursion that caused MCP 120s timeouts when consecutive technical questions were generated. DEFERRED questions remain auto-skipped (unchanged behavior). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ing (#C1) Remove auto-answer + recursion for DECIDE_LATER classified questions in PMInterviewEngine.ask_next_question(). Questions are now returned to the main session with classification metadata, allowing users to explicitly choose "decide later" via the UI. This eliminates infinite recursion that caused MCP 120s timeouts when consecutive technical questions were generated. DEFERRED questions remain auto-skipped (unchanged behavior). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ping Apply the same pattern as DECIDE_LATER to DEFERRED questions: return them to the main session so the user can choose to defer or answer. Adds skip_as_deferred() method and [deferred] answer handling in pm_handler. This eliminates ALL recursion in ask_next_question(), fully resolving the MCP 120s timeout issue. Both DEFERRED and DECIDE_LATER are now user-driven, with skip_eligible metadata signaling the UI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…erage - CLI now intercepts "decide later"/"skip"/"defer" input and calls skip_as_decide_later()/skip_as_deferred() instead of recording verbatim - CLI shows defer hint for DEFERRED questions (not just DECIDE_LATER) - question_for_pm returns original_question for DEFERRED (was empty string) - MCP hint text uses bracketed format [decide_later]/[deferred] consistently - Added MCP handler test for answer="[deferred]" path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…uff format - Fix PM generation summary: separate deferred/decide-later counts instead of combining them into one misleading number - Update ClassifierOutputType docstring: DEFERRED and DECIDE_LATER are now returned to user with skip option, not auto-answered - Run ruff format on all changed files to fix CI Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When the interview ambiguity score passes the threshold, the MCP now automatically generates the PM document instead of returning is_complete=true and waiting for a separate action="generate" call. This eliminates a race where the LLM response text sounded like completion, causing the skill to call generate before the engine had actually marked the interview complete. The action="generate" endpoint is kept as a fallback for retries when auto-generation fails (meta.generation_failed=true). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![ouroboros-agent[bot]](https://avatars.githubusercontent.com/in/3091302?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
67d493b| Triggered by: PR opened
Branch: fix/pm-auto-generate-on-complete | 9 files, +689/-332 | CI: no checks reported
Issue #N/A Requirements
| Requirement | Status |
|---|---|
| Auto-generate the PM document when interview completion is detected in the MCP flow | PARTIAL — implemented in src/ouroboros/mcp/tools/pm_handler.py:928-999, but the new CLI skip path regresses state persistence in src/ouroboros/cli/commands/pm.py:466-489. |
Let callers present skip / decide-later affordances instead of auto-answering inside ask_next_question |
MET — implemented in src/ouroboros/bigbang/pm_interview.py:485-523 and surfaced through src/ouroboros/mcp/tools/pm_handler.py:503-541,1068-1122. |
Previous Review Follow-up
| Previous Finding | Status |
|---|---|
| First review — no previous findings. | N/A |
Blocking Findings
| # | File:Line | Severity | Confidence | Finding |
|---|---|---|---|---|
| 1 | src/ouroboros/cli/commands/pm.py:466-489 |
blocker | high | Both new skip branches call engine.skip_as_decide_later(...) / engine.skip_as_deferred(...) but then persist the old state object instead of record_result.value. That loses the answered round whenever record_response returns a new InterviewState instance rather than mutating in place, so a user who types skip/defer can resume into the same unanswered question or miss the deferred/decide-later bookkeeping on disk. The normal answer path already has the same shape, but this regression is introduced by the new skip flow in the diff. |
Test Coverage
I could not re-run the touched pytest suites in this sandbox because dependency downloads are blocked by DNS/network restrictions. Given the bug above, please add a CLI-level regression test that exercises skip / defer and asserts the saved state comes from the returned updated state object rather than the pre-call state reference.
Design Notes
The core direction looks good: returning DECIDE_LATER / DEFERRED questions to the caller instead of recursively auto-answering should reduce timeout pressure and makes the UX more honest. The MCP auto-generation path is also a sensible simplification. The remaining issue is consistency of state ownership: once the engine API returns Result[InterviewState, ...], callers need to treat the returned state as authoritative everywhere, including the newly added CLI skip branches.
Follow-up Recommendations
- Mirror the MCP handler’s
state = skip_result.valuepattern in the CLI so all answer paths use the updated state object consistently. - Add an end-to-end PM interview test for the completion path, since the PR body already notes that the real flow is still unverified.
Files Reviewed
skills/pm/SKILL.mdsrc/ouroboros/bigbang/pm_interview.pysrc/ouroboros/bigbang/question_classifier.pysrc/ouroboros/cli/commands/pm.pysrc/ouroboros/mcp/tools/pm_handler.pytests/unit/bigbang/test_decide_later_items.pytests/unit/bigbang/test_pm_interview.pytests/unit/bigbang/test_question_classifier.pytests/unit/mcp/tools/test_pm_handler.py
Reviewed by ouroboros-agent[bot] via Codex deep analysis
cohemm
force-pushed
the
fix/pm-auto-generate-on-complete
branch
from
67d493b to
edef812
Compare
Contributor
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
edef812| Triggered by: new push by @cohemm
Branch: fix/pm-auto-generate-on-complete | 9 files, +694/-340 | CI: Ruff Lint failing; MyPy + tests passing
Issue #N/A Requirements
| Requirement | Status |
|---|---|
| Auto-generate the PM document when interview completion is detected in the MCP flow | PARTIAL — the MCP path now auto-generates and returns the PM document paths in src/ouroboros/mcp/tools/pm_handler.py:928-999, but the CLI skip/defer path still saves the pre-update state in src/ouroboros/cli/commands/pm.py:466-489. |
Let callers present skip / decide-later affordances instead of auto-answering inside ask_next_question |
MET — src/ouroboros/bigbang/pm_interview.py:485-523 now returns skippable questions to the caller, and both the CLI/MCP surfaces expose that affordance in src/ouroboros/cli/commands/pm.py:413-489 and src/ouroboros/mcp/tools/pm_handler.py:503-541,805-1122. |
Previous Review Follow-up
| Previous Finding | Status |
|---|---|
CLI skip/defer persisted the old state object instead of the updated Result.value state. |
STILL OPEN — the new head still calls engine.skip_as_decide_later(...) / engine.skip_as_deferred(...) and then immediately saves state rather than record_result.value in src/ouroboros/cli/commands/pm.py:466-489. |
Blocking Findings
| # | File:Line | Severity | Confidence | Finding |
|---|---|---|---|---|
| 1 | src/ouroboros/cli/commands/pm.py:466-489 |
High | High | Both new CLI skip branches still discard the updated interview state returned by skip_as_decide_later() / skip_as_deferred() and persist the old state object instead. Because those helpers delegate to record_response() and explicitly return the authoritative updated InterviewState (see tests/unit/bigbang/test_decide_later_items.py:494-504), a user who types skip / defer can resume into stale state: the answered round may be missing on disk and the deferred/decide-later bookkeeping can be lost after restart. This was the previous blocker and it remains unfixed on the current HEAD. |
Test Coverage
I could not re-run the touched pytest suites in this sandbox because pytest is not installed here. Please add or keep a CLI-level regression test that drives the interactive skip/defer path in src/ouroboros/cli/commands/pm.py and asserts the saved state comes from the updated Result.value, not the pre-call state reference. The PR body also still lists the full PM interview completion flow as unverified E2E coverage.
Design Notes
The overall direction is good. Returning DECIDE_LATER / DEFERRED questions to the caller is a cleaner UX than silently auto-answering them, and auto-generating the PM document on MCP completion removes a brittle extra round-trip from the skill flow. The remaining merge blocker is just state ownership consistency: every path that records or skips a response needs to treat the engine's returned state as canonical before persisting.
Follow-up Recommendations
- Fix the CLI path to assign
state = record_result.value/state = skip_result.valuebeforesave_state(...), matching the MCP handler's updated pattern. - Once that lands, add an end-to-end PM interview completion test covering the auto-generated
pm.md/ seed artifacts called out in the PR body. - Ruff is currently failing in CI; even if unrelated to the blocker above, please make sure the final head is lint-clean before merge.
Files Reviewed
skills/pm/SKILL.mdsrc/ouroboros/bigbang/pm_interview.pysrc/ouroboros/bigbang/question_classifier.pysrc/ouroboros/cli/commands/pm.pysrc/ouroboros/mcp/tools/pm_handler.pytests/unit/bigbang/test_decide_later_items.pytests/unit/bigbang/test_pm_interview.pytests/unit/bigbang/test_question_classifier.pytests/unit/mcp/tools/test_pm_handler.py
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Collaborator
Author
|
Merged into feat/pm-decide-later-user-choice (PR #238) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
is_complete: true반환 → SKILL.md가 별도action="generate"호출 → LLM 텍스트에 혼동되어 미완료 상태에서 generate 호출하는 버그 발생meta.pm_path로 결과 반환, 별도 generate 호출 불필요action="generate"는 auto-generate 실패 시 재시도용으로 유지Test plan
test_pm_handler.py119 passedtest_pm_handler_generate.py+test_pm_interview.py70 passed🤖 Generated with Claude Code