feat(planning): add PRD template system for customizable output formats (#316) #321
Conversation
…ts (#316) Implements a template system for PRD generation: - Add PrdTemplateSection and PrdTemplate dataclasses for template structure - Add PrdTemplateManager for template loading, validation, and rendering - Create 5 built-in templates: standard, lean, enterprise, user-story-map, technical-spec - Implement Jinja2-based template rendering with custom filters (bullet_list, numbered_list, table) - Add YAML serialization for template import/export CLI commands: - `cf prd templates list` - List available templates - `cf prd templates show <id>` - Show template details - `cf prd templates import <file>` - Import custom template - `cf prd templates export <id> <output>` - Export template to YAML - `cf prd generate --template <id>` - Generate PRD using specific template Templates support: - Global templates in ~/.codeframe/templates/prd/ - Project templates in .codeframe/templates/prd/ - Template validation (required fields, Jinja2 syntax) - Custom template functions for list formatting Tests: 44 new tests covering template system and CLI commands
WalkthroughAdds a PRD template system (models, manager, built-ins, Jinja2 helpers), CLI subcommands to list/show/export/import templates, integrates template selection into prd generate with upfront validation and template-driven prompt building, and adds tests plus a Jinja2 dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI (app.py)
participant Manager as PrdTemplateManager
participant Jinja as Jinja2 Engine
participant Discovery as Discovery Session
participant FS as File System
User->>CLI: prd templates list
CLI->>Manager: list_templates()
Manager-->>CLI: templates metadata
CLI-->>User: display templates
User->>CLI: prd generate --template <id>
CLI->>Manager: get_template(<id>)
alt template exists
Manager-->>CLI: template
CLI->>Discovery: run discovery -> gather discovery_data
Discovery-->>CLI: discovery_data
CLI->>Manager: render_template(template, discovery_data)
loop per section
Manager->>Jinja: render section with discovery_data + TEMPLATE_FUNCTIONS
Jinja-->>Manager: rendered section or error placeholder
end
Manager-->>CLI: combined PRD markdown
CLI->>FS: save PRD (include template_id metadata)
CLI-->>User: output/save PRD
else missing template
Manager-->>CLI: not found
CLI-->>User: error + list available templates
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 PRD template system and route
|
Code Review: PRD Template System (#316)I've completed a thorough review of this PR. Overall, this is a well-architected feature with solid foundations, but there's a critical integration gap that prevents the templates from being used in PRD generation. Here's my detailed feedback: 🚨 Critical Issue: Template Not Actually UsedThe template is validated but never passed to the PRD generation logic. In
Impact: The Required fix:
✅ StrengthsArchitecture & Design
Code Quality
Testing
🔧 Code Quality Issues1. Missing Error Handling in
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
The --template flag was validated but never passed to generate_prd(). Changes: - Add template_id parameter to PrdDiscoverySession.generate_prd() - Add _build_prd_prompt() to construct template-aware prompts - Pass template_id from CLI to session.generate_prd() - Store template_id in PRD metadata for traceability - Add tests for template integration The LLM prompt is now dynamically constructed based on the selected template's sections, ensuring the generated PRD follows the chosen format.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@codeframe/planning/prd_templates.py`:
- Around line 753-758: The project template load path is skipped when
workspace_path is None, so project templates in the current repo aren't loaded;
change the logic that calls get_project_template_dir to default to the current
working directory when workspace_path is omitted. Specifically, update the
PrdTemplateManager code that calls
self._load_from_directory(get_project_template_dir(workspace_path)) to compute
project_dir = workspace_path or Path.cwd() (or equivalent) and then call
self._load_from_directory(get_project_template_dir(project_dir)) so
get_project_template_dir and _load_from_directory still get used but with a CWD
fallback.
- Around line 860-875: import_template currently only loads into memory; update
it to validate the loaded template and persist it to the templates directory so
it survives process restarts. After calling load_template_from_file(source_path)
in import_template, run the template validation routine (e.g.,
template.validate() or a shared validate_template(template) helper) and
raise/log on validation errors; then write the template YAML/json to the
persistent templates directory (use the existing templates storage helper or
write to self.templates_dir with a filename based on template.id), update
self.templates[template.id] only after successful validation and file write, and
keep the logger.info call to reflect the persisted import.
| # Load custom templates from global directory | ||
| self._load_from_directory(get_global_template_dir()) | ||
|
|
||
| # Load project templates (override global) | ||
| if workspace_path: | ||
| self._load_from_directory(get_project_template_dir(workspace_path)) |
There was a problem hiding this comment.
Project templates are skipped when workspace_path is omitted.
CLI commands construct PrdTemplateManager() without a workspace path, so .codeframe/templates/prd in the current project is never loaded. That makes project-level templates undiscoverable and breaks --template for local custom templates.
💡 Suggested fix (load project templates from CWD by default)
- # Load project templates (override global)
- if workspace_path:
- self._load_from_directory(get_project_template_dir(workspace_path))
+ # Load project templates (override global)
+ project_dir = (
+ get_project_template_dir(workspace_path)
+ if workspace_path
+ else get_project_template_dir()
+ )
+ self._load_from_directory(project_dir)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Load custom templates from global directory | |
| self._load_from_directory(get_global_template_dir()) | |
| # Load project templates (override global) | |
| if workspace_path: | |
| self._load_from_directory(get_project_template_dir(workspace_path)) | |
| # Load custom templates from global directory | |
| self._load_from_directory(get_global_template_dir()) | |
| # Load project templates (override global) | |
| project_dir = ( | |
| get_project_template_dir(workspace_path) | |
| if workspace_path | |
| else get_project_template_dir() | |
| ) | |
| self._load_from_directory(project_dir) |
🤖 Prompt for AI Agents
In `@codeframe/planning/prd_templates.py` around lines 753 - 758, The project
template load path is skipped when workspace_path is None, so project templates
in the current repo aren't loaded; change the logic that calls
get_project_template_dir to default to the current working directory when
workspace_path is omitted. Specifically, update the PrdTemplateManager code that
calls self._load_from_directory(get_project_template_dir(workspace_path)) to
compute project_dir = workspace_path or Path.cwd() (or equivalent) and then call
self._load_from_directory(get_project_template_dir(project_dir)) so
get_project_template_dir and _load_from_directory still get used but with a CWD
fallback.
codeframe/planning/prd_templates.py
Outdated
| def import_template(self, source_path: Path) -> PrdTemplate: | ||
| """Import a template from a file. | ||
|
|
||
| Args: | ||
| source_path: Path to YAML template file | ||
|
|
||
| Returns: | ||
| Imported template | ||
|
|
||
| Raises: | ||
| FileNotFoundError: If source file doesn't exist | ||
| """ | ||
| template = load_template_from_file(source_path) | ||
| self.templates[template.id] = template | ||
| logger.info(f"Imported template: {template.id}") | ||
| return template |
There was a problem hiding this comment.
import_template doesn’t persist or validate templates.
Right now the import only adds the template to the in-memory registry, so cf prd templates import reports success but the template isn’t available in later commands. Invalid templates can also be imported without detection.
🛠️ Suggested fix (validate + persist to template directory)
def import_template(self, source_path: Path) -> PrdTemplate:
"""Import a template from a file.
@@
Raises:
FileNotFoundError: If source file doesn't exist
"""
template = load_template_from_file(source_path)
+ errors = self.validate_template(template)
+ if errors:
+ raise ValueError("Invalid template: " + "; ".join(errors))
+
+ target_dir = (
+ get_project_template_dir(self.workspace_path)
+ if self.workspace_path
+ else get_project_template_dir()
+ )
+ target_path = target_dir / f"{template.id}.yaml"
+ save_template_to_file(template, target_path)
+
self.templates[template.id] = template
logger.info(f"Imported template: {template.id}")
return template🤖 Prompt for AI Agents
In `@codeframe/planning/prd_templates.py` around lines 860 - 875, import_template
currently only loads into memory; update it to validate the loaded template and
persist it to the templates directory so it survives process restarts. After
calling load_template_from_file(source_path) in import_template, run the
template validation routine (e.g., template.validate() or a shared
validate_template(template) helper) and raise/log on validation errors; then
write the template YAML/json to the persistent templates directory (use the
existing templates storage helper or write to self.templates_dir with a filename
based on template.id), update self.templates[template.id] only after successful
validation and file write, and keep the logger.info call to reflect the
persisted import.
Updated Code Review: PRD Template System (#316)I've reviewed the updates since my last review. Excellent work addressing the critical integration issue! The template is now properly wired into PRD generation. Here's my updated assessment: ✅ Critical Issue RESOLVEDThe template integration is now working correctly: # codeframe/cli/app.py:1433
prd_record = session.generate_prd(template_id=template)
# codeframe/core/prd_discovery.py:592-650
def generate_prd(self, template_id: Optional[str] = None) -> prd.PrdRecord:
# Build prompt based on template
prompt = self._build_prd_prompt(qa_history, template_id)
...
# Store PRD with template_id in metadata
record = prd.store(
self.workspace,
content=content,
title=title,
metadata={
"template_id": template_id or "standard",
...
},
)✅ Template parameter is passed to 🎯 Remaining Issues1. Missing Error Handling in
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@codeframe/cli/app.py`:
- Around line 685-712: prd_templates_import currently imports the template only
in memory via PrdTemplateManager.import_template, so the template disappears
after the process exits; after calling manager.import_template(source_path)
persist the template to the templates store (e.g., call a persistence method on
PrdTemplateManager such as manager.save_template(template) or implement
manager.persist_template(template) if missing) so the template YAML/metadata is
written into the global or project template directory; ensure the persistence
uses the same template.id/name and raises/handles errors similarly to the import
step so the CLI prints the success or error messages consistently.
- Around line 607-610: The CLI is instantiating PrdTemplateManager without a
workspace_path so it only loads built‑in/global templates; update the
PrdTemplateManager() calls (the ones followed by manager.list_templates()) to
pass the current project workspace path (e.g. Path.cwd() or your CLI's workspace
root variable) as the workspace_path argument so .codeframe/templates in the
repo is included in template resolution; make the same change for both
occurrences around the PrdTemplateManager instantiations.
In `@codeframe/core/prd_discovery.py`:
- Around line 662-670: PrdTemplateManager is being instantiated without the
workspace scope so project templates in .codeframe/templates aren’t loaded;
update the instantiation of PrdTemplateManager to pass the repository/workspace
path (e.g., PrdTemplateManager(workspace_path=workspace_path) or the appropriate
repo path variable available in this module) before calling
get_template(template_id), so project-scoped templates are considered when
building the prompt (still keep the existing fallback to PRD_GENERATION_PROMPT
when template_id is empty or get_template returns None).
Fixes three issues with template resolution and persistence: 1. CLI prd_templates_import now persists templates to disk - Added persist_template() method to PrdTemplateManager - Import with persist=True saves to .codeframe/templates/prd/ - Shows save location in CLI output 2. CLI template commands now include project templates - Pass workspace_path=Path.cwd() to PrdTemplateManager - Project templates in .codeframe/templates/prd/ are discovered 3. prd_discovery._build_prd_prompt uses workspace scope - Passes self.workspace.path to PrdTemplateManager - Project templates can be used in PRD generation Added 4 new tests for persist functionality.
PR Review: PRD Template System ImplementationThis is a solid implementation of the PRD template system. The code is well-structured, thoroughly tested, and follows the v2 architecture guidelines. Here's my detailed review: ✅ Strengths1. Architecture Alignment
2. Code Quality
3. Security
4. Test Coverage
5. User Experience
🔧 Issues to Address1. Missing The # Current (line 1296):
template_manager = PrdTemplateManager()
# Should be:
template_manager = PrdTemplateManager(workspace_path=workspace_path)This is inconsistent with the other template commands ( Impact: Users can't use project-specific templates with 2. Template Validation Missing in The # Missing validation after loading:
def load_template_from_file(path: Path) -> PrdTemplate:
with open(path, encoding="utf-8") as f:
data = yaml.safe_load(f)
# No validation of required fields or structure
sections = [...]
return PrdTemplate(...)Potential issues:
Recommendation: Add validation after loading:
3. Jinja2 Template Injection Risk (Low Severity) While Current risk: Low - only affects users who import untrusted templates from jinja2 import SandboxedEnvironment
self._env = SandboxedEnvironment(loader=BaseLoader())💡 Suggestions (Non-Blocking)1. Template Validation Method Consider adding a def validate(self) -> None:
"""Validate template structure."""
if not self.id or not self.name:
raise ValueError("Template must have id and name")
if not self.sections:
raise ValueError("Template must have at least one section")
for section in self.sections:
if not section.id or not section.source:
raise ValueError(f"Invalid section: {section}")This would make validation explicit and reusable. 2. Template Version Compatibility The 3. Better Error Messages in Line 854 catches except TemplateSyntaxError as e:
logger.warning(f"Template syntax error in {section.id}: {e}")
except UndefinedError as e:
logger.warning(f"Missing variable in {section.id}: {e}")📝 Minor Issues
🧪 Testing RecommendationsThe existing tests are comprehensive, but consider adding:
📊 Summary
Recommendation: Request changes for issue #1 (workspace_path), then approve. Issues #2-3 are nice-to-haves that can be addressed in follow-up PRs. 🎯 Action ItemsMust fix before merge:
Should fix (high priority): Nice to have (lower priority): Great work overall! This feature is well-implemented and will provide significant value. The issues identified are fixable and mostly edge cases. Looking forward to the next iteration! 🚀 |
Code Review: PRD Template SystemI've reviewed PR #321 implementing the PRD template system. Overall, this is excellent work with solid architecture, comprehensive testing, and good adherence to the project's v2 principles. Here's my detailed feedback: ✅ Strengths1. Architecture & Design
2. Template System Features
3. CLI Integration
4. Testing
🔍 Issues & RecommendationsCritical: Template Validation Not Used During ImportLocation: # Import and persist to project directory
template = manager.import_template(source_path, persist=True)Issue: When importing a template, the code doesn't validate it before persisting. A user could import a broken template with invalid Jinja2 syntax, and it would be saved to disk. Later attempts to use it would fail. Recommendation: template = manager.import_template(source_path, persist=False)
errors = manager.validate_template(template)
if errors:
console.print(f"[red]Error:[/red] Template validation failed:")
for error in errors:
console.print(f" - {error}")
raise typer.Exit(1)
# Now persist after validation
manager.persist_template(template, to_project=True)Medium: Inconsistent Error Handling in Template LoadingLocation: def _load_from_directory(self, directory: Path) -> None:
# ...
for path in directory.glob("*.yaml"):
try:
template = load_template_from_file(path)
self.templates[template.id] = template
logger.debug(f"Loaded template from {path}")
except Exception as e:
logger.warning(f"Failed to load template from {path}: {e}")Issue: If a custom template has duplicate ID with a built-in, it silently overwrites without warning the user. This could be confusing. Recommendation: Add a check and warning: if template.id in self.templates:
logger.warning(f"Template '{template.id}' already exists, overriding with {path}")Minor: Template Not Actually RenderedLocation: The Current behavior: LLM is instructed to follow template format Recommendation: Either:
This isn't blocking, but the current implementation is more of a "template-guided generation" rather than "template-based rendering." Minor: Missing Template Validation in TestsLocation: Tests validate that individual templates pass validation, but don't test the
Code Quality: Minor
🛡️ Security
📊 Performance
No performance concerns. 📝 Documentation
Suggestion: Add a section to ✅ Approval StatusRecommendation: Approve with minor fixes This PR is ready to merge after addressing the Critical issue (template validation on import). The other issues are nice-to-haves that can be addressed in follow-up PRs. Priority Fixes Before Merge:
Optional Follow-ups (future PRs):
Great work on this feature! The template system is well-designed and will provide real value to users. 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/core/prd_discovery.py (1)
592-650:⚠️ Potential issue | 🟡 MinorAlign default template handling across prompt and metadata.
generate_prd()documents a default"standard"template, but_build_prd_prompt()bypasses templates whentemplate_idisNonewhile metadata/logging always record"standard". Resolve the default once and use it consistently.🔧 Suggested fix
- # Build prompt based on template - prompt = self._build_prd_prompt(qa_history, template_id) + resolved_template_id = template_id or "standard" + # Build prompt based on template + prompt = self._build_prd_prompt(qa_history, resolved_template_id) @@ - "template_id": template_id or "standard", + "template_id": resolved_template_id, @@ - logger.info(f"Generated PRD {record.id} from session {self.session_id} using template '{template_id or 'standard'}'") + logger.info( + f"Generated PRD {record.id} from session {self.session_id} using template '{resolved_template_id}'" + )
🤖 Fix all issues with AI agents
In `@tests/planning/test_prd_templates.py`:
- Around line 505-534: The test test_persist_template_to_project uses a string
containment check against saved_path which is OS-dependent; change the assertion
to compare Path objects or use saved_path.as_posix() to make it OS-agnostic.
Specifically, update the assertion after calling
PrdTemplateManager.persist_template (saved_path variable) to compare saved_path
to an expected Path built with pathlib (e.g., expected = workspace_path /
".codeframe" / "templates" / "prd" / "persisted-test.yaml") and assert
saved_path == expected or assert saved_path.exists() and saved_path ==
expected.resolve(), ensuring use of saved_path and persist_template identifiers
from the test.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/core/prd_discovery.py (1)
615-642:⚠️ Potential issue | 🟠 MajorResolve and persist the actual template used (fallbacks are mislabeled).
When
template_idis missing or invalid,_build_prd_promptfalls back toPRD_GENERATION_PROMPT, but metadata/logging still record"standard"or the invalid ID. That mislabels the stored PRD and can break downstream template lookups/exports. Please resolve the template first and persist the resolved ID (optionally also the requested one).🛠️ Proposed fix to track requested vs resolved template IDs
- # Build prompt based on template - prompt = self._build_prd_prompt(qa_history, template_id) + # Build prompt based on template + requested_template_id = template_id or "standard" + prompt, resolved_template_id = self._build_prd_prompt(qa_history, requested_template_id) @@ - "template_id": template_id or "standard", + "template_id": resolved_template_id, + "requested_template_id": requested_template_id, @@ - logger.info(f"Generated PRD {record.id} from session {self.session_id} using template '{template_id or 'standard'}'") + logger.info( + f"Generated PRD {record.id} from session {self.session_id} " + f"using template '{resolved_template_id or 'default'}'" + ) @@ - def _build_prd_prompt(self, qa_history: str, template_id: Optional[str] = None) -> str: + def _build_prd_prompt( + self, qa_history: str, template_id: Optional[str] = None + ) -> tuple[str, Optional[str]]: @@ - if not template_id: - return PRD_GENERATION_PROMPT.format(qa_history=qa_history) + if not template_id: + return PRD_GENERATION_PROMPT.format(qa_history=qa_history), None @@ - if template is None: + if template is None: logger.warning(f"Template '{template_id}' not found, using default prompt") - return PRD_GENERATION_PROMPT.format(qa_history=qa_history) + return PRD_GENERATION_PROMPT.format(qa_history=qa_history), None @@ - return prompt + return prompt, template_idAlso applies to: 649-706
🧹 Nitpick comments (1)
codeframe/core/prd_discovery.py (1)
678-683: Include section format hints to honor template structure.
section.formatis currently ignored, so templates can’t reliably steer lists/tables in the generated output. Consider surfacing this as a format hint in the prompt.💡 Optional tweak to include format hints
- required_note = " (required)" if section.required else " (optional)" - sections_spec.append(f"## {section.title}{required_note}\n{section.source} - related content") + required_note = " (required)" if section.required else " (optional)" + format_note = f" (format: {section.format})" if section.format else "" + sections_spec.append( + f"## {section.title}{required_note}{format_note}\n" + f"{section.source} - related content" + )
When template_id is missing or invalid, _build_prd_prompt falls back to the default prompt. Previously, metadata would incorrectly record the original requested template ID. Changes: - _build_prd_prompt now returns tuple of (prompt, resolved_template_id) - Resolved ID is "default" when no template used or fallback occurred - Metadata stores resolved template_id for accurate tracking - If requested template differs from resolved, store requested_template_id - Logging now reflects actual template used
- Update generate_prd docstring to match actual fallback behavior (uses "default" not "standard" when template not found) - Replace string path assertion with Path object comparison for cross-platform compatibility in test_persist_template_to_project
Code Review - PRD Template SystemExcellent implementation overall! The PRD template system is well-architected, thoroughly tested, and aligns well with v2 architecture principles. ✅ Strengths
Issues & Recommendations1. Jinja2 Autoescape Not Enabled (Security - Medium Priority) 2. Template Rendering Error Handling Too Broad (Code Quality - Low) 3. Template ID Validation (Enhancement - Optional) 4. Documentation - Template Source Field (Documentation - Low) Alignment with v2 ArchitecturePerfect adherence to v2 principles:
Final VerdictApprove with minor suggestions. The security and code quality issues are low-severity and can be addressed in a follow-up PR. The core implementation is solid, well-tested, and ready to merge. Recommendation: Merge this PR. Consider follow-up issues for:
Great work! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codeframe/core/prd_discovery.py (1)
592-655:⚠️ Potential issue | 🟡 MinorAlign default template behavior with the docstring.
Docstring says the default is
"standard", but_build_prd_promptuses the legacy prompt and returns"default"whentemplate_idisNone. Please align the docstring with actual behavior (or pass"standard"by default if that’s the intended behavior).📝 Suggested docstring fix (if legacy default is intended)
- template_id: Optional PRD template ID to use for formatting. - Defaults to "standard" if not provided. + template_id: Optional PRD template ID to use for formatting. + If omitted, falls back to the legacy default prompt ("default").
🤖 Fix all issues with AI agents
In `@codeframe/core/prd_discovery.py`:
- Around line 657-715: The prompt builder _build_prd_prompt currently returns
the caller-supplied template_id and labels the section block "Required Sections"
even when optional sections are included; update it to return the resolved
template.id (use template.id when returning the tuple) and change the heading
logic for template.sections so the prompt either uses a neutral "Sections"
heading or splits into "Required Sections" and "Optional Sections" by iterating
template.sections and grouping by section.required; keep the rest of prompt
construction and return (prompt, template.id).
| def _build_prd_prompt( | ||
| self, qa_history: str, template_id: Optional[str] = None | ||
| ) -> tuple[str, str]: | ||
| """Build PRD generation prompt based on template. | ||
|
|
||
| Args: | ||
| qa_history: Formatted Q&A history string | ||
| template_id: Template ID to use (defaults to None, which uses default prompt) | ||
|
|
||
| Returns: | ||
| Tuple of (prompt string, resolved template ID) | ||
| The resolved template ID is "default" if no template was used, | ||
| or the actual template ID that was successfully loaded. | ||
| """ | ||
| from codeframe.planning.prd_templates import PrdTemplateManager | ||
| from pathlib import Path | ||
|
|
||
| # Use default prompt if no template specified | ||
| if not template_id: | ||
| return (PRD_GENERATION_PROMPT.format(qa_history=qa_history), "default") | ||
|
|
||
| # Pass workspace path to include project templates | ||
| workspace_path = Path(self.workspace.repo_path) if self.workspace.repo_path else None | ||
| manager = PrdTemplateManager(workspace_path=workspace_path) | ||
| template = manager.get_template(template_id) | ||
|
|
||
| if template is None: | ||
| logger.warning(f"Template '{template_id}' not found, falling back to default prompt") | ||
| return (PRD_GENERATION_PROMPT.format(qa_history=qa_history), "default") | ||
|
|
||
| # Build dynamic prompt from template sections | ||
| sections_spec = [] | ||
| for section in template.sections: | ||
| required_note = " (required)" if section.required else " (optional)" | ||
| sections_spec.append(f"## {section.title}{required_note}\n{section.source} - related content") | ||
|
|
||
| sections_text = "\n\n".join(sections_spec) | ||
|
|
||
| prompt = f"""Generate a Product Requirements Document based on the discovery conversation. | ||
|
|
||
| ## Discovery Conversation | ||
| {qa_history} | ||
|
|
||
| ## Template: {template.name} | ||
| {template.description} | ||
|
|
||
| ## Required Sections | ||
| Generate a markdown PRD with these sections in order: | ||
|
|
||
| # [Project Title - infer from conversation] | ||
|
|
||
| {sections_text} | ||
|
|
||
| --- | ||
|
|
||
| Keep it concise but complete. Focus on actionable requirements. | ||
| Follow the template structure exactly. This PRD should be sufficient to generate development tasks.""" | ||
|
|
||
| return (prompt, template_id) |
There was a problem hiding this comment.
Use the resolved template ID and clarify the section heading.
- Return the actual
template.idto avoid storing/logging a caller-supplied alias. - The heading says “Required Sections” but includes optional sections; consider renaming to “Sections” (or split required vs optional).
🔧 Suggested adjustment
-## Required Sections
+## Sections
@@
- return (prompt, template_id)
+ return (prompt, template.id)🤖 Prompt for AI Agents
In `@codeframe/core/prd_discovery.py` around lines 657 - 715, The prompt builder
_build_prd_prompt currently returns the caller-supplied template_id and labels
the section block "Required Sections" even when optional sections are included;
update it to return the resolved template.id (use template.id when returning the
tuple) and change the heading logic for template.sections so the prompt either
uses a neutral "Sections" heading or splits into "Required Sections" and
"Optional Sections" by iterating template.sections and grouping by
section.required; keep the rest of prompt construction and return (prompt,
template.id).
Ruff F401: Path import was redundant since tmp_path fixture already provides a Path object for division operations.
Code Review: PRD Template SystemI've reviewed PR #321 implementing the PRD template system. This is a well-designed feature that aligns with the v2 architecture. Here's my feedback: ✅ StrengthsArchitecture & Design
Code Quality
Testing
🔍 Issues & Recommendations1. Template Function Security (Low Priority)The Jinja2 custom filters in
row_values = [str(item.get(col, "")).replace("|", "\\|") for col in columns]2. Template Validation Gap (Medium Priority)
3. CLI UX Enhancement (Low Priority)The
4. Documentation in Templates (Low Priority)Built-in templates have hardcoded placeholder content (e.g., lines 304-307, 373-387). Consider:
5. Template Metadata (Enhancement)The
6. Error Message Clarity (Low Priority)
📋 Code Specific ObservationsGood Patterns Observed
Minor Improvements
def _setup_jinja_env(self) -> Environment:
env = Environment(loader=BaseLoader())
for name, func in TEMPLATE_FUNCTIONS.items():
env.filters[name] = func
return env
🎯 Testing RecommendationsAdditional Test Cases to Consider
🔐 Security AssessmentOverall: SECURE ✅
One consideration: Users can create custom templates with arbitrary Jinja2 logic. While safe by default, document that template files should be treated like code (don't load untrusted templates). 📊 Performance Considerations
✨ Final VerdictLGTM with minor suggestions 🎉 This is a well-crafted feature that:
The suggestions above are enhancements, not blockers. The code is production-ready as-is. Recommended Next Steps
Great work on maintaining the high quality bar while adding meaningful functionality! Review conducted following |
Code Review - PR #321: PRD Template SystemThis is a well-designed and comprehensive implementation of a PRD template system. The code quality is excellent with strong architecture, thorough testing, and good integration with existing systems. StrengthsArchitecture & Design
Code Quality
Testing
CLI Design
Observations & Recommendations1. Template Rendering Error Handling (prd_templates.py:853-855)Current behavior inserts error placeholders when sections fail to render. This is good for robustness but might hide issues during development. Recommendation: Consider adding a strict_mode parameter to render_template() that raises on errors, or log at ERROR level instead of WARNING to help developers catch template bugs early. 2. Template Function SecuritySince templates can be imported from arbitrary files, consider adding autoescape=True to the Jinja2 environment initialization for defense in depth. 3. Template Validation Timing (prd_templates.py:767-782)_load_from_directory() logs warnings but continues on failure. Enhancement: Add a validate parameter to import_template() to give users immediate feedback on broken templates instead of discovering issues at render time. 4. Integration with Discovery Session (prd_discovery.py:592-656)Excellent integration with optional template_id parameter, metadata tracking, and dynamic prompt construction. Note: Consider adding an example output in the prompt showing the expected format for LLM guidance. Potential Issues1. Template Validation in CLIVerify that the CLI (app.py around line 1440) validates the template exists before starting the discovery session, with clear error messages listing available templates. 2. Template ID Collision HandlingWhen loading templates from multiple directories, later templates silently override earlier ones (prd_templates.py:779). This could cause confusion. Suggestion: Add a warning when overriding existing templates. SecurityGenerally secure, but note:
DocumentationStrong documentation throughout with one gap: Missing: Add a brief section to docs/CLI_WIREFRAME.md documenting the new prd templates subcommand tree. Test CoverageExcellent coverage with one gap: Template validation during import. Consider adding a test that verifies import rejects templates with syntax errors when validation is enabled. SummaryThis PR is ready to merge with minor enhancements recommended for future iterations: Priority recommendations (before merge):
Suggested enhancements (future PRs):
Score: 9/10 - Excellent implementation with solid architecture and testing Great work! This is a well-crafted feature that significantly improves PRD customization. |
1. CLI template validation now includes project templates - Pass workspace_path to PrdTemplateManager during validation - Project templates in .codeframe/templates/prd/ are now discoverable 2. Template ID collision handling - Log warning when a template overrides an existing one - Helps users understand which template version is active Added test for override warning behavior.
Code Review - PRD Template SystemThis PR implements a well-designed PRD template system with Jinja2 rendering. The implementation is solid overall, with good test coverage and clean separation of concerns. Below are my findings: ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@codeframe/planning/prd_templates.py`:
- Around line 869-872: The docstring for import_template() claims the persist
parameter controls saving to project/global directories but the implementation
always calls persist_template(..., to_project=True); update import_template to
either accept a new parameter (e.g., to_project: bool or
target=('project'|'global')) and pass it through to persist_template, or change
the docstring to state it always persists to the project directory; adjust
references to persist_template and the persist parameter in import_template() to
reflect the chosen behavior (ensure the function signature, docstring, and the
call to persist_template are consistent).
1. Jinja2 autoescape (XSS prevention) - Enable autoescape=True in Jinja2 Environment - Prevents HTML injection if PRDs are rendered downstream 2. Path traversal prevention - Validate file exists and is a YAML file in load_template_from_file() - Reject non-.yaml/.yml files 3. Improved error handling in render_template() - Catch TemplateSyntaxError separately with specific message - Re-raise KeyboardInterrupt/SystemExit - Log full traceback for unexpected errors 4. Template validation on import - Validate template before registering in import_template() - Raise ValueError with validation errors Added 4 security-focused tests covering all validations.
Code Review SummaryI've reviewed this PRD template system implementation. The architecture is solid and aligns well with CodeFRAME's v2 headless, CLI-first principles. Here are my observations: ✅ Strengths
🔍 Observations & Suggestions1. Template Resolution Transparency (Minor)The current implementation tracks resolved templates correctly in metadata ( # prd_discovery.py:685
if template is None:
logger.warning(f"Template '{template_id}' not found, falling back to default prompt")
console.print(f"[yellow]Warning:[/yellow] Template '{template_id}' not found, using default")
return (PRD_GENERATION_PROMPT.format(qa_history=qa_history), "default")This would improve UX when users typo a template name. 2. Section Format Hints (Enhancement Opportunity)As CodeRabbit noted,
3. Template Validation Timing ✅Good: Validation happens at import time ( 4. Jinja2 Environment Reuse (Performance - Very Minor)The Jinja2 environment is created once per manager instance ( 5. Test Path Assertions
|
Use .resolve() on both paths to handle Windows path normalization differences (e.g., case sensitivity, symlink resolution).
Code Review - PR #321: PRD Template SystemThis is a well-designed and comprehensive implementation of the PRD template system. The feature adds significant value by allowing users to customize PRD output formats. Strengths
Issues to Address (from CodeRabbit reviews)1. CLI Commands Missing workspace_path (Medium Priority)Problem: CLI commands instantiate PrdTemplateManager() without workspace path, missing project-scoped templates. Fix: Pass workspace path in CLI commands (app.py lines 607, 633, 668, 696, 1271): manager = PrdTemplateManager(workspace_path=Path.cwd())2. Project Template Directory Fallback (Medium Priority)Problem: When workspace_path=None, project templates in .codeframe/templates/prd/ are not loaded. Fix: Either default to cwd or require callers to pass workspace_path. 3. Template Resolution Consistency (Low Priority)The code already correctly returns tuple from _build_prd_prompt(). Just ensure "default" template ID is documented as the legacy prompt format. Suggestions for Improvement
Alignment with CLAUDE.md Guidelines
SummaryHigh-quality PR with good architecture, comprehensive tests, and security considerations. Before merging:
Recommendation: APPROVE with minor fixes for workspace path handling Great work! The template system is well-designed and will be very useful. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/planning/test_prd_templates.py`:
- Around line 495-504: The test test_get_global_template_dir currently patches
only HOME which fails on Windows because Path.home() respects USERPROFILE there;
update the test to set both environment vars (setenv("HOME", ...) and
setenv("USERPROFILE", ...)) before calling get_global_template_dir so
Path.home() resolves correctly on all platforms and the assertions around
"templates" and "prd" continue to hold.
Code quality improvements based on CodeRabbit review: 1. Empty YAML file handling - Add null check after yaml.safe_load() with clear error message 2. list_templates sorting robustness - Handle name=None with fallback to empty string 3. import_template docstring accuracy - Fix to match behavior (always saves to project directory) 4. Support both .yaml and .yml extensions - _load_from_directory now globs both patterns 5. Cross-platform test compatibility - Set both HOME and USERPROFILE for Path.home() tests 6. Section heading clarity - Change "Required Sections" to "Sections" in prompt Added test for empty YAML file rejection.
Code Review - PR #321: PRD Template SystemI've completed my review of this PR, comparing it against the previous reviews and the current state of the code. This is an excellent implementation that has addressed all the critical issues from earlier reviews. ✅ Previously Identified Issues - ALL RESOLVED1. Template Not Passed to PRD Generation (CRITICAL) ✅ FIXEDPrevious Issue: Template was validated but never passed to Current State: RESOLVED
2. Workspace Path Not Passed to Template Manager (MEDIUM) ✅ FIXEDPrevious Issue: CLI commands instantiated Current State: RESOLVED
3. Security Concerns ✅ ADDRESSEDPrevious State: Jinja2 autoescape was already enabled. Current State: VERIFIED SECURE
🔍 Architecture ReviewAlignment with CLAUDE.md Guidelines ✅v2 Architecture Compliance:
Module Structure: This is exactly the right architecture for v2. 💎 Code Quality Highlights1. Robust Template Resolution (
|
| format_template=s["format_template"], | ||
| required=s.get("required", True), | ||
| ) | ||
| for s in data.get("sections", []) |
There was a problem hiding this comment.
🟢 Low
Null sections (e.g., YAML sections: null) can cause iteration errors during loading and validation. Suggest normalizing sections to [] when loading and guarding (or early-returning) in validate_template before iterating.
| for s in data.get("sections", []) | |
| for s in data.get("sections") or [] |
🚀 Want me to fix this? Reply ex: "fix it for me".
| self.templates[template.id] = template | ||
|
|
||
| if persist: | ||
| self.persist_template(template) |
There was a problem hiding this comment.
🟢 Low
When persist=True but workspace_path is None, the template is added to self.templates before persist_template raises ValueError, leaving inconsistent state. Consider calling persist_template before registering, or checking workspace_path upfront.
| self.templates[template.id] = template | |
| if persist: | |
| self.persist_template(template) | |
| if persist: | |
| self.persist_template(template) | |
| self.templates[template.id] = template |
🚀 Want me to fix this? Reply ex: "fix it for me".
Summary
Implements a PRD template system that allows users to customize the output format of generated PRDs. This addresses issue #316.
New CLI Commands
Built-in Templates
standardleanenterpriseuser-story-maptechnical-specFiles Changed
codeframe/planning/prd_templates.py- Core template systemcodeframe/planning/prd_template_functions.py- Jinja2 custom filterscodeframe/cli/app.py- CLI commands for template managementcodeframe/planning/__init__.py- Module exportspyproject.toml/uv.lock- Added jinja2 dependencyTest Plan
Closes #316
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.