feat(cli): add interactive PRD generation via Socratic discovery #317
Conversation
Implements `cf prd generate` command for CLI-native PRD creation through interactive Socratic discovery. Features: - Interactive question/answer flow with progress tracking - 5 required questions covering problem, users, features, constraints, tech stack - AI-powered follow-up questions when ANTHROPIC_API_KEY available - Pause/resume support via blocker system - Input validation (min length, invalid pattern detection) - Automatic PRD generation from structured answers - Resume incomplete sessions automatically New files: - codeframe/core/prd_discovery.py: Headless discovery session manager - tests/core/test_prd_discovery.py: 18 unit tests - tests/cli/test_prd_generate.py: 11 CLI integration tests Special commands during discovery: - /pause - Save progress and create blocker for later resume - /skip - Skip optional questions - /quit - Exit without saving - /help - Show available commands Part of Phase 1: CLI Completion (v2 Strategic Roadmap) Addresses issue #307
WalkthroughAdds an interactive Changes
Sequence DiagramsequenceDiagram
participant User as User
participant CLI as CLI
participant Session as PrdDiscoverySession
participant DB as Discovery_DB
participant LLM as LLM_Provider
participant PRD as PRD_Store
User->>CLI: cf prd generate --workspace <path> [--resume <id>]
CLI->>Session: start_or_resume(session_id?)
Session->>DB: _ensure_discovery_schema()
Session->>DB: _save_session() [INITIAL]
loop Interactive Discovery Loop
Session->>Session: get_current_question()
alt AI enabled
Session->>LLM: generate_question(context)
LLM-->>Session: question / follow-up
else Static fallback
Session-->>Session: return_canned_question()
end
Session-->>CLI: show question & progress
CLI-->>User: prompt for answer
User->>CLI: answer or /command
alt /pause or /quit
CLI->>Session: pause_discovery()
Session->>DB: _save_session() [PAUSED + blocker]
CLI-->>User: show blocker ID
else normal answer
CLI->>Session: submit_answer(response)
Session->>Session: validate & update coverage
Session->>DB: _save_session() [UPDATED]
Session-->>CLI: next question or completion
end
end
alt Completed
CLI->>Session: generate_prd()
Session->>PRD: store_prd(record)
Session->>DB: _save_session() [COMPLETED]
CLI-->>User: show PRD preview & next steps
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Add interactive PRD discovery and generation to CLI via
|
PR Review: Interactive PRD Generation via Socratic DiscoveryThis is a well-implemented feature that aligns excellently with the v2 architecture principles. The implementation is clean, headless, and follows the Golden Path methodology. Here's my detailed review: ✅ StrengthsArchitecture Alignment
Code Quality
User Experience
🔍 Areas for Improvement1. Database Migration SafetyIssue: # prd_discovery.py:850
def _ensure_discovery_schema(workspace: Workspace) -> None:
cursor.execute("""
CREATE TABLE IF NOT EXISTS discovery_sessions (...)
""")Recommendation: Consider adding a migration system or version tracking to handle future schema changes gracefully. For now, this is acceptable as CREATE IF NOT EXISTS is idempotent, but document this decision. 2. LLM Provider Error HandlingObservation: Silent fallback when LLM initialization fails (line 95-100) except Exception as e:
logger.warning(f"Failed to initialize LLM provider: {e}")
self._llm_provider = NoneRecommendation: This is good defensive programming, but consider informing the user when falling back to static questions in the CLI output for transparency. 3. PRD Title Extraction LogicPotential Issue: if "." in text[:100]:
return text.split(".")[0].strip()[:60]
return text[:50].strip() + "..."Recommendation: Consider using the LLM to extract/generate a concise title from the problem description, or prompt user for an explicit title in the discovery flow. 4. Test Coverage GapMinor: No explicit test for concurrent session creation While unlikely in CLI usage, Recommendation: Add workspace-level locking or document single-user assumption. 5. Type Annotation ConsistencyMinor: Line 80 uses _llm_provider: Any = field(default=None, init=False)Recommendation: Use 🛡️ Security Considerations✅ No SQL injection: Uses parameterized queries throughout 📊 Performance Considerations✅ Database indexes: Proper indexes on 🧪 Test Quality AssessmentUnit Tests (
CLI Integration Tests (
Coverage appears comprehensive - likely 90%+ on new code. 📝 Documentation Quality✅ Module docstring explains purpose and headless nature 🎯 Alignment with Phase 1 GoalsFrom
This PR directly advances Phase 1 completion. 🚀 Recommendations for MergeMust Fix Before MergeNone - Code is production-ready as-is. Should Consider (Post-Merge)
Nice to Have (Future Enhancement)
✨ Overall AssessmentCode Quality: 9/10 Final Verdict: ✅ Approve with enthusiastic recommendation This is exemplary v2 implementation. The code is clean, well-tested, follows all architectural principles, and provides genuine user value. The session-based approach with pause/resume is particularly elegant. Great work! 🎉 📚 Related Documentation
Reviewed by: Claude Sonnet 4.5 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
codeframe/core/prd_discovery.py
Outdated
| ValidationError: If answer is invalid | ||
| ValueError: If no current question | ||
| """ | ||
| answer_text = answer_text.strip() |
There was a problem hiding this comment.
🟢 Low
Consider adding consistent session/state guards. Require non-None session_id and state == DISCOVERING in submit_answer, a started session in pause_discovery, and prevent resume_discovery from reopening COMPLETED sessions. A shared helper would keep this consistent.
- answer_text = answer_text.strip()
+ if self.session_id is None:
+ raise ValueError("No active session - call start_discovery() first")
+
+ answer_text = answer_text.strip()🚀 Want me to fix this? Reply ex: "fix it for me".
| Raises: | ||
| ValueError: If session not found | ||
| """ | ||
| conn = get_db_connection(self.workspace) |
There was a problem hiding this comment.
🟢 Low
Consider wrapping DB operations in try/finally (or a context manager) so connections always close on exceptions across all DB callers (e.g., load_session, _save_session, _ensure_discovery_schema, get_active_session).
🚀 Want me to fix this? Reply ex: "fix it for me".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@codeframe/cli/app.py`:
- Around line 1246-1253: The /skip branch currently breaks out of the input loop
without advancing the session, so the same question is shown again; fix by
invoking a session-level skip or submitting a placeholder answer: either add a
skip_question() method on PrdDiscoverySession (implementing the behavior
suggested in the review: validate optional, record an entry with "skipped":
True, _save_session and return True) and call session.skip_question() from the
CLI when answer.lower() == "/skip", or call the existing
session.submit_answer(...) (or equivalent) with an empty/placeholder payload
that the session recognizes as skipped; ensure you use the unique symbols
session.skip_question (new) or session.submit_answer /
session.get_current_question to locate and update the code and handle the skip
result to continue to the next question.
🧹 Nitpick comments (1)
codeframe/core/prd_discovery.py (1)
374-406: Consider storing session_id in blocker metadata instead of parsing from question text.The current approach parses
Session ID:from the blocker's question text, which is fragile if the question format changes. If the blocker system supports metadata fields, that would be more robust.However, this works correctly for the current implementation and the format is controlled by
pause_discovery.
codeframe/cli/app.py
Outdated
| if answer.lower() == "/skip": | ||
| if is_optional or skip_optional: | ||
| # Move to next question by submitting a placeholder | ||
| console.print("[dim]Skipping...[/dim]\n") | ||
| break | ||
| else: | ||
| console.print("[yellow]Cannot skip required questions[/yellow]") | ||
| continue |
There was a problem hiding this comment.
The /skip command doesn't actually skip the question.
When the user types /skip, the code breaks out of the inner answer loop without submitting anything. However, since no answer is recorded, session.get_current_question() will return the same question on the next iteration of the outer loop.
To properly skip optional questions, you need to either:
- Submit a placeholder answer that the session recognizes as "skipped"
- Add a
skip_question()method toPrdDiscoverySessionthat advances to the next question without recording an answer
🔧 Possible fix: Add a skip_question method to PrdDiscoverySession
In codeframe/core/prd_discovery.py, add a method to skip optional questions:
def skip_question(self) -> bool:
"""Skip the current optional question.
Returns:
True if question was skipped, False if not optional
"""
context = {qid: data.get("text", "") for qid, data in self._answers.items()}
question = self._framework.get_next_question(context)
if question is None:
return False
if question.get("importance") != "optional":
return False
# Record as skipped (empty marker)
self._answers[question["id"]] = {
"text": "",
"question_text": question["text"],
"category": question["category"],
"timestamp": _utc_now().isoformat(),
"skipped": True,
}
self._save_session()
return TrueThen in the CLI:
if answer.lower() == "/skip":
if is_optional or skip_optional:
- # Move to next question by submitting a placeholder
- console.print("[dim]Skipping...[/dim]\n")
- break
+ if session.skip_question():
+ console.print("[dim]Skipping...[/dim]\n")
+ break
+ else:
+ console.print("[yellow]Cannot skip this question[/yellow]")
+ continue
else:
console.print("[yellow]Cannot skip required questions[/yellow]")
continue🤖 Prompt for AI Agents
In `@codeframe/cli/app.py` around lines 1246 - 1253, The /skip branch currently
breaks out of the input loop without advancing the session, so the same question
is shown again; fix by invoking a session-level skip or submitting a placeholder
answer: either add a skip_question() method on PrdDiscoverySession (implementing
the behavior suggested in the review: validate optional, record an entry with
"skipped": True, _save_session and return True) and call session.skip_question()
from the CLI when answer.lower() == "/skip", or call the existing
session.submit_answer(...) (or equivalent) with an empty/placeholder payload
that the session recognizes as skipped; ensure you use the unique symbols
session.skip_question (new) or session.submit_answer /
session.get_current_question to locate and update the code and handle the skip
result to continue to the next question.
- AI is now Plan A - no fallback to static questions - Context-sensitive questions via AI generation - AI validates answer adequacy (relevance, specificity, completeness) - Coverage scoring system (problem, users, features, constraints, tech_stack) - "Question tax" - AI only asks if answer meaningfully improves PRD - Completion based on coverage threshold (avg 70, all categories 60+) - Updated tests with comprehensive mocked LLM responses
Follow-up Review NotesI've reviewed this PR and my comprehensive review from earlier still stands. The implementation is excellent and ready to merge. However, I want to highlight one item from CodeRabbit's review that deserves attention: Documentation Discrepancy: /skip CommandIssue: The PR description mentions a /skip command in the "Special Commands During Discovery" table, but this is not actually implemented. The actual implementation:
Recommendation: Update the PR description to remove the /skip command reference. The AI-driven discovery doesn't have a concept of "optional questions" - the AI determines when to stop based on coverage assessment, so skip doesn't make sense in this context. This is purely a documentation issue - the code itself is correct. Other NotesAll other items from my previous review and CodeRabbit's suggestions are minor improvements for future work, not blockers:
Verdict: Ready to merge once PR description is updated to remove /skip reference. Great work on this feature! |
| # Ensure schema exists and save initial session | ||
| _ensure_discovery_schema(self.workspace) | ||
| self._save_session() | ||
|
|
||
| # Generate first question | ||
| self._current_question = self._generate_opening_question() | ||
|
|
There was a problem hiding this comment.
🟢 Low
Consider moving _save_session() after _generate_opening_question() to avoid persisting an inconsistent session state if the LLM call fails.
| # Ensure schema exists and save initial session | |
| _ensure_discovery_schema(self.workspace) | |
| self._save_session() | |
| # Generate first question | |
| self._current_question = self._generate_opening_question() | |
| # Ensure schema exists | |
| _ensure_discovery_schema(self.workspace) | |
| # Generate first question before saving to ensure consistent state | |
| self._current_question = self._generate_opening_question() | |
| # Save session only after successful question generation | |
| self._save_session() |
🚀 Want me to fix this? Reply ex: "fix it for me".
| try: | ||
| session = PrdDiscoverySession(workspace) | ||
| session.resume_discovery(resume) | ||
| except (ValueError, NoApiKeyError) as e: |
There was a problem hiding this comment.
🟠 High
Session resume is fragile due to discovery DB schema handling. Suggest ensuring the discovery_sessions table exists without dropping data (use CREATE TABLE IF NOT EXISTS in _ensure_discovery_schema()), call it before get_active_session()/resume_discovery(), and optionally catch sqlite3.OperationalError during load/resume.
🚀 Want me to fix this? Reply ex: "fix it for me".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@codeframe/core/prd_discovery.py`:
- Around line 700-703: The current code unconditionally drops the
discovery_sessions table via cursor.execute("DROP TABLE IF EXISTS
discovery_sessions"), which destroys saved sessions and breaks resume logic
(e.g., get_active_session); replace this destructive reset with a guarded
migration: remove the DROP TABLE call, implement a schema-migration path that
checks the current schema/version and issues ALTER TABLE or creates missing
columns/tables as needed, or add a guarded reset flag (e.g., force_reset
parameter) that only runs the DROP when explicitly true; update functions that
assume table presence (like get_active_session) to handle missing columns
gracefully and log the migration action via cursor.execute and schema version
checks instead of blindly dropping discovery_sessions.
- Around line 257-273: start_discovery currently calls _save_session before
generating and persisting the opening question, leaving current_question NULL if
the user exits early; move the persistence so that _current_question is set by
_generate_opening_question and then call _save_session (or call _save_session
again after setting _current_question) to ensure the session record stores the
opening question; update the start_discovery method to call
_generate_opening_question first (or call _save_session a second time) so
_current_question is persisted for resume logic.
- Around line 464-468: Update _coverage_is_sufficient to also enforce a
minimum-question requirement (5) before allowing discovery to be considered
complete: in the method _coverage_is_sufficient check the current
question/answer count (use self._questions_count if present, otherwise
len(self._questions or [])) and return False if that count is less than 5, then
keep the existing check of self._coverage.get("ready_for_prd", False); i.e.,
require both minimum questions and ready_for_prd to be true before returning
True.
Updated Review: AI-Native PRD DiscoveryI've reviewed the latest changes (commit eda4af4) that make discovery fully AI-native. This is an excellent evolution from the initial hybrid approach. Here's my assessment: ✅ New Strengths in Latest VersionAI-Native Architecture
Improved Design
🔍 Issues from CodeRabbit's Review1. ❌ CRITICAL: Destructive Schema Reset (line 700-703)CodeRabbit correctly identified that the code contains: cursor.execute("DROP TABLE IF EXISTS discovery_sessions")Impact: This destroys all saved sessions and breaks resume functionality. This would be a production incident. Required fix: Remove the DROP TABLE entirely. The 2.
|
- Remove destructive DROP TABLE statement (was destroying saved sessions) - Persist opening question after generation (enables early-exit resume) - Remove /skip from available commands (not applicable to AI-driven discovery) AI completion remains flexible - comprehensive answers can complete discovery quickly without minimum question requirements.
✅ Final Review - All Issues Addressed (commit 2169955)I've reviewed the latest commit that addresses all feedback from my previous reviews and CodeRabbit's analysis. This PR is now ready to merge. 🎯 Changes in Latest Commit (2169955)The commit successfully resolves all blocking issues:
📋 Architecture Assessment✅ v2 Principles (All Met)
✅ Code Quality
✅ Database Design
🔍 Minor Observations (Non-Blocking)These are suggestions for future enhancement, not merge requirements:
🎉 HighlightsThis PR exemplifies excellent v2 implementation:
✨ Final VerdictStatus: ✅ APPROVED - Ready to Merge Code Quality: 9/10 This feature directly advances Phase 1: CLI Completion from the v2 Strategic Roadmap and closes issue #307. The AI-driven approach is the right architectural decision and aligns perfectly with headless, CLI-first principles. Excellent work! 🚀 Reviewed by: Claude Sonnet 4.5 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@codeframe/core/prd_discovery.py`:
- Around line 766-773: The current block silently returns None when
ANTHROPIC_API_KEY is missing which hides configuration errors; update the logic
around os.getenv("ANTHROPIC_API_KEY") so that instead of returning None you log
a clear warning (use the module logger or processLogger) and raise a descriptive
exception (e.g., RuntimeError or ValueError) indicating the missing
ANTHROPIC_API_KEY so callers and tests can distinguish "session not present"
from "session could not be loaded"; reference PrdDiscoverySession and the
subsequent session.load_session(row[0]) call when implementing this change.
🧹 Nitpick comments (2)
codeframe/core/prd_discovery.py (2)
427-439: Consider extracting JSON parsing into a reusable helper.The markdown code block parsing logic (lines 431-434) is duplicated in
_validate_answerand_update_coverage. While the fallback handling makes this safe, extracting it into a helper would improve maintainability.♻️ Suggested helper extraction
def _parse_json_response(content: str) -> Optional[dict[str, Any]]: """Parse JSON from LLM response, handling markdown code blocks.""" content = content.strip() if content.startswith("```"): parts = content.split("```") if len(parts) >= 2: content = parts[1] if content.startswith("json"): content = content[4:] try: return json.loads(content.strip()) except json.JSONDecodeError: return None
558-590: Fragile string parsing for session resume.The resume logic relies on parsing the blocker's question text to extract the session ID (lines 575-580) and uses substring matching to identify discovery blockers (line 571). This is fragile:
- Any blocker containing "discovery" would pass the check (false positives)
- The
Session ID:format must exactly match whatpause_discoverywritesConsider storing the session ID in a structured field or using a more robust marker.
♻️ Alternative: Use blocker metadata or a dedicated field
If the blocker system supports metadata, store session info there:
# In pause_discovery: blocker = blockers.create( self.workspace, question=f"Discovery session paused: {reason}", task_id=None, metadata={"type": "discovery", "session_id": self.session_id} ) # In resume_discovery: if blocker.metadata.get("type") != "discovery": raise ValueError("Blocker is not a discovery session blocker") session_id = blocker.metadata.get("session_id")If metadata isn't available, at minimum use a more specific marker string (e.g.,
[DISCOVERY_SESSION:uuid]) that's less likely to collide.
Co-authored-by: macroscopeapp[bot] <170038800+macroscopeapp[bot]@users.noreply.github.com>
…ssion When ANTHROPIC_API_KEY is missing but a session exists, now logs a warning and raises NoApiKeyError instead of returning None. This allows callers to distinguish between "no session exists" and "session could not be loaded due to missing API key".
Final Consolidated Review - PR #317Based on reviewing all commits and previous feedback iterations, here's my final assessment: ✅ Overall Status: APPROVED - Ready to MergeThis PR successfully implements AI-native PRD discovery through the 🎯 What Changed Across IterationsInitial Implementation (commits 8f257b1 - 42049d6)
Evolution to AI-Native (commit eda4af4)
Bug Fixes (commit 2169955)
Latest (commit bbdd8c1)
✅ Architecture CompliancePerfect adherence to v2 principles from
🧪 Test Coverage: Excellent
🏆 Key Strengths
📊 Code Quality Metrics
🔍 Minor Items for Future Enhancement (Non-Blocking)These are not merge requirements, just observations for future work:
🚀 Impact AssessmentCloses: #307 (Interactive PRD discovery) ✨ Final RecommendationLGTM - Ready to Merge 🎉 This PR represents high-quality implementation that:
The AI-native approach is the right architectural choice for this feature. Great work! Reviewed by: Claude Sonnet 4.5 |
✅ Final Confirmation - PR Ready to MergeI have completed a comprehensive review of all commits in this PR. All previously identified issues have been successfully addressed. Summary of Resolved Issues
Architecture Assessment ✅v2 Compliance - Perfect:
Code Quality - Excellent:
Database Design - Solid:
Remaining Observations (Non-Blocking)These are suggestions for future enhancement, not merge requirements:
Highlights 🎉This PR exemplifies excellent v2 implementation:
Final VerdictStatus: ✅ APPROVED - READY TO MERGE Ratings:
This feature directly advances Phase 1: CLI Completion from the V2 Strategic Roadmap and properly closes issue #307. Excellent work! 🚀 Reviewed by: Claude Sonnet 4.5 |
| session = active | ||
| console.print("[green]✓[/green] Resuming previous session") |
There was a problem hiding this comment.
🟢 Low
When auto-resuming an active session at line 1176, consider calling session.state = SessionState.DISCOVERING (and session._save_session()) to match the behavior of resume_discovery(). Currently the PAUSED state persists to the database.
- session = active
- console.print("[green]✓[/green] Resuming previous session")
+ session = active
+ session.state = SessionState.DISCOVERING
+ session._save_session()
+ console.print("[green]✓[/green] Resuming previous session")🚀 Want me to fix this? Reply ex: "fix it for me".
Summary
Implements
cf prd generatecommand for CLI-native PRD creation through interactive Socratic discovery.Changes
codeframe/core/prd_discovery.pycodeframe/cli/app.pyprd generatecommand (+222 lines)tests/core/test_prd_discovery.pytests/cli/test_prd_generate.pyUsage
Special Commands During Discovery
/pause/quit/helpDiscovery Flow
Each answer is validated (minimum length, no "n/a" patterns) and stored in a new
discovery_sessionstable.Generated PRD Structure
Test Plan
PrdDiscoverySession(18 tests)Related
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.