Hotfix: Fix some commands template logic and helper scripts#8
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds many new Jinja2 agent/report templates, introduces a scaffold-agent CLI for discovering and rendering agent templates, refactors template path resolution into file-ops, enhances spec-tree generation with gitignore awareness, updates multiple scripts for multi-assistant context handling and safer file reads, and bumps the project version. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as scaffold-agent CLI
participant Disc as Template Discovery
participant Ctx as Project Context
participant Render as Template Renderer
participant FS as Filesystem
User->>CLI: scaffold --agents <names> [--args JSON]
CLI->>Disc: Discover templates (project or package)
Disc-->>CLI: Agent types list
CLI->>Ctx: Build project context (feature, branch, timestamps)
loop For each agent
CLI->>Render: Render template with context + args
Render->>FS: Write YYYY-MM-DD-XXX-*.md under base_output_dir/<agent>/
FS-->>Render: Path
Render-->>CLI: Success/Failure + path
end
CLI-->>User: Summary (JSON or table)
sequenceDiagram
autonumber
actor User
participant UAC as update-agent-context
participant CFG as Project Config
participant SH as ScriptHelpers
participant AI as Assistant API (per assistant)
participant FS as Filesystem
User->>UAC: run [assistant?] [--json]
alt assistant provided
UAC->>SH: Locate spec dir & build context
UAC->>AI: Update assistant context
AI-->>UAC: Result + (optional) memory refresh
else update all
UAC->>CFG: Load enabled assistants
loop For each assistant
UAC->>SH: Locate spec dir & build context
UAC->>AI: Update assistant context
AI-->>UAC: Result per assistant
end
end
UAC->>FS: Write/resolve context files
UAC-->>User: Per-assistant results, overall success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.specify/scripts/update-agent-context.py (1)
75-80: Honor --json: avoid printing non-JSON from update_context_file.update_context_file prints console messages regardless of json_output, breaking consumers.
-def update_context_file( - context_file: Path, context_info: Dict[str, Any], assistant_type: str -) -> bool: +def update_context_file( + context_file: Path, context_info: Dict[str, Any], assistant_type: str, json_output: bool = False +) -> bool: @@ - if not context_file.exists(): - console.print(f"[yellow]Context file not found: {context_file}[/yellow]") + if not context_file.exists(): + if not json_output: + console.print(f"[yellow]Context file not found: {context_file}[/yellow]") return False @@ - if not sections: - console.print("[yellow]No project context found to add[/yellow]") + if not sections: + if not json_output: + console.print("[yellow]No project context found to add[/yellow]") return False @@ - context_file.write_text(updated_content) - console.print( - f"[green]Updated {context_file.name} with comprehensive project context[/green]" - ) + context_file.write_text(updated_content) + if not json_output: + console.print( + f"[green]Updated {context_file.name} with comprehensive project context[/green]" + ) return TrueAnd pass the flag at call site:
- success = update_context_file(context_file, context_info, assistant_name) + success = update_context_file(context_file, context_info, assistant_name, json_output=json_output)Also applies to: 97-134
🧹 Nitpick comments (6)
src/specify_cli/templates/scripts/scaffold-agent.py.j2 (4)
457-462: Update message to reflect package-based discovery.The text still references .specify/agent-templates. It should mention package agent-prompts.
- if not agent_types: - echo_info( - "No agent templates found in .specify/agent-templates/", quiet - ) - echo_info("Create agent templates to use this feature", quiet) + if not agent_types: + echo_info( + "No agent prompts found in package resources (agent-prompts).", quiet + ) + echo_info("Ensure your package includes agent-prompts or update to a version that does.", quiet)
74-113: Deterministic agent listing.Sort discovered templates by filename to make --list output stable.
- for template_file in agent_prompts_dir.iterdir(): + for template_file in sorted(agent_prompts_dir.iterdir(), key=lambda p: p.name): if template_file.is_file() and template_file.name.endswith( ".md.j2" ):
541-544: Guard relative_to() for non-repo output dirs.Path.relative_to() raises ValueError if the output dir is outside repo root. Fall back to absolute path.
- rel_path = Path(result).relative_to(helpers.get_repo_root()) - echo_success(f"Generated: {rel_path}", quiet) + try: + rel_path = Path(result).relative_to(helpers.get_repo_root()) + except ValueError: + rel_path = Path(result) + echo_success(f"Generated: {rel_path}", quiet)
71-118: Python 3.8 compatibility for importlib.resources.files.files() requires Python 3.9+. If you target 3.8, add a backport shim to use importlib_resources.
-import importlib.resources +try: + # Python 3.9+ + import importlib.resources as _ires + resource_files = _ires.files + as_file = _ires.as_file +except Exception: + # Python 3.8 fallback + import importlib_resources as _ires # type: ignore + resource_files = _ires.files # type: ignore + as_file = _ires.as_file # type: ignore @@ - agent_prompts_dir = ( - importlib.resources.files(templates_pkg) / "agent-prompts" - ) + agent_prompts_dir = resource_files(templates_pkg) / "agent-prompts" @@ - with importlib.resources.as_file(template_path) as real_template_path: + with as_file(template_path) as real_template_path:Based on learnings
.specify/scripts/update-agent-context.py (1)
168-197: Search spec dirs from repo root, not CWD.Using Path.cwd() can scan unintended paths. Base discovery on repo root for consistency.
- cwd = Path.cwd() - spec_dir = None + repo_root = helpers.get_repo_root() + spec_dir = None @@ - if (cwd / "spec.md").exists() or (cwd / "plan.md").exists(): - spec_dir = cwd + if (repo_root / "spec.md").exists() or (repo_root / "plan.md").exists(): + spec_dir = repo_root else: # Look for most recent spec directory spec_dirs = [] - for pattern in ["specs/*/", "features/*/", "*/specs/"]: - spec_dirs.extend(cwd.glob(pattern)) + for pattern in ["specs/*/", "features/*/", "*/specs/"]: + spec_dirs.extend(repo_root.glob(pattern)).specify/agent-templates/context.md.j2 (1)
421-421: Fix join/default order in metadata-- **Data Sources**: {{ data_sources | join(", ") | default("Not specified") }} +- **Data Sources**: {{ (data_sources | default([])) | join(", ") if data_sources is defined else "Not specified" }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.specify/agent-templates/architecture-reviewer.md.j2(1 hunks).specify/agent-templates/code-reviewer.md.j2(1 hunks).specify/agent-templates/context.md.j2(1 hunks).specify/agent-templates/documentation-reviewer.md.j2(1 hunks).specify/agent-templates/generic-agent.md.j2(1 hunks).specify/agent-templates/implementer.md.j2(1 hunks).specify/agent-templates/spec-reviewer.md.j2(1 hunks).specify/agent-templates/test-reviewer.md.j2(1 hunks).specify/scripts/check-prerequisites.py(1 hunks).specify/scripts/generate-tasks.py(1 hunks).specify/scripts/scaffold-agent.py(1 hunks).specify/scripts/setup-plan.py(2 hunks).specify/scripts/update-agent-context.py(4 hunks)pyproject.toml(1 hunks)src/specify_cli/services/template_service/template_file_operations.py(1 hunks)src/specify_cli/services/template_service/template_service.py(2 hunks)src/specify_cli/templates/commands/implement.md.j2(1 hunks)src/specify_cli/templates/scripts/scaffold-agent.py.j2(8 hunks)src/specify_cli/templates/scripts/update-agent-context.py.j2(1 hunks)src/specify_cli/utils/script_helpers.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/specify_cli/services/template_service/template_file_operations.py (1)
src/specify_cli/services/template_registry/models.py (1)
filename(37-39)
src/specify_cli/services/template_service/template_service.py (1)
src/specify_cli/services/template_service/template_file_operations.py (1)
determine_output_path_for_ai(100-162)
.specify/scripts/update-agent-context.py (3)
src/specify_cli/services/config_service/config_service.py (1)
TomlConfigService(122-838)src/specify_cli/utils/script_helpers.py (2)
get_repo_root(67-103)load_project_config(137-155)src/specify_cli/services/memory_service/memory_service.py (2)
MemoryManager(17-158)generate_import_section(105-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (8)
pyproject.toml (1)
3-3: Version bump matches the hotfix scope.Patch increment is appropriate given the bugfix updates elsewhere in the PR.
.specify/scripts/check-prerequisites.py (3)
256-265: LGTM: Size limit prevents resource exhaustion.The 10MB safety limit is a sensible defense against accidentally loading oversized contract files into memory. The error handling correctly stops processing and provides a clear message.
266-266: LGTM: Explicit UTF-8 encoding ensures cross-platform consistency.Specifying
encoding="utf-8"prevents platform-dependent encoding issues and aligns with JSON's UTF-8 requirement.
272-275: LGTM: Comprehensive error handling for file operations.Catching
OSErrorandMemoryErrorprovides robust handling for file system errors and memory exhaustion scenarios. This complements the size check at lines 256-265 by handling edge cases where issues occur despite the safety limit.src/specify_cli/templates/commands/implement.md.j2 (1)
24-34: Ensurecheck-implementation-statuscommand is implemented and supports--spec-idand--json
The docs in src/specify_cli/templates/commands/implement.md.j2 reference this command; verify it’s registered in the CLI and that its flags match the template..specify/agent-templates/documentation-reviewer.md.j2 (1)
185-186: Verify custom Jinjaternaryfilter registration
The template uses the non-standardternaryfilter:{{ issue.auto_fixable | ternary("Yes", "No") }}Jinja doesn’t include
ternaryby default—ensure you’ve registered it in your Environment or replace it with an inline expression:{{ "Yes" if issue.auto_fixable else "No" }}src/specify_cli/services/template_service/template_service.py (2)
22-22: No changes required; get_assistant is already imported in template_file_operations.py (line 16).
682-685: Confirm all categories and extension logic in determine_output_path_for_ai
- commands: delegates to determine_output_filename (removes .j2, applies command_files.file_format)
- context: returns assistant.config.context_file.file (static JSON path)
- scripts / memory: strips .j2 → SPECIFY_SCRIPTS_DIR/, SPECIFY_MEMORY_DIR/
- agent-prompts: strips .j2 → TEMPLATE_REGISTRY.resolve_target/
- agent-templates: preserves .j2 → TEMPLATE_REGISTRY.resolve_target/.j2
| **Service Mesh**: {{ service_architecture.service_mesh | ternary("Implemented", "Not Implemented") }} | ||
| **Data Consistency**: {{ service_architecture.data_consistency_pattern }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify custom Jinja ‘ternary’ filter availability.
The template relies on ‘ternary’. Ensure it’s registered, or refactor to inline if/else to avoid runtime template errors.
Example refactor:
- {{ service_architecture.service_mesh | ternary("Implemented", "Not Implemented") }}
- becomes: {{ "Implemented" if service_architecture.service_mesh else "Not Implemented" }}
Run to verify filter registration:
Also applies to: 114-116, 261-262, 383-387, 393-397
🏁 Script executed:
#!/bin/bash
rg -n -C2 -P 'filters\s*\[\s*[\"\']ternary[\"\']\s*\]|add_filter\(\s*[\"\']ternary[\"\']' -g '!**/node_modules/**'
rg -n -C2 -P '\|\s*ternary\s*\('Length of output: 68
🏁 Script executed:
#!/bin/bash
rg -n -C2 "filters\[\s*['\"]ternary['\"]\s*\]|add_filter\(\s*['\"]ternary['\"]\)" -g '!**/node_modules/**'
rg -n -C2 "def\s+ternary" -g '*.py'
rg -n -C2 "\|\s*ternary\s*\(" -g '*.j2'Length of output: 8735
Ensure the ternary filter is registered or refactor to inline conditionals.
No ternary filter definition was found in the codebase; either register it on your Jinja environment (e.g. env.filters['ternary']=lambda v,a,b: a if v else b) or replace all {{ foo | ternary("A","B") }} with {{ "A" if foo else "B" }} to prevent runtime errors.
🤖 Prompt for AI Agents
.specify/agent-templates/architecture-reviewer.md.j2 around lines 104-105: the
template uses a non-existent Jinja filter `ternary`, which will cause runtime
errors; either register the filter on the Jinja environment (e.g. add
env.filters['ternary']=...) or replace the usage with an inline conditional
expression `{{ "Implemented" if service_architecture.service_mesh else "Not
Implemented" }}` (apply the same replacement pattern for all other `|
ternary(...)` occurrences) so the template does not rely on an undefined filter.
| #### {{ issue.severity.upper() }}: {{ issue.title }} | ||
| **File**: `{{ issue.file_path }}` | ||
| **Line**: {{ issue.line_number | default("N/A") }} | ||
| **Description**: {{ issue.description }} |
There was a problem hiding this comment.
🧩 Analysis chain
Blocker: non-existent Jinja filter and unsafe attribute call
- Jinja2 has no built-in ternary filter; will raise “no filter named ‘ternary’”.
- Calling .upper() before default will error when severity is missing.
- Apply default before length; otherwise length on undefined may error.
Fix examples:
-#### {{ issue.severity.upper() }}: {{ issue.title }}
+#### {{ (issue.severity | default("HIGH")) | upper }}: {{ issue.title }}
- - **Fix**: {{ violation.auto_fixable | ternary("Auto-fixable", "Manual fix required") }}
+ - **Fix**: {{ "Auto-fixable" if violation.auto_fixable else "Manual fix required" }}
-**Files Reviewed**: {{ files_reviewed | length | default(0) }}
+**Files Reviewed**: {{ (files_reviewed | default([])) | length }}Search for similar patterns project‑wide:
Also applies to: 65-66, 187-188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Ternary filter uses:" >&2
rg -nP '\|\s*ternary\s*\(' .specify/agent-templates -C1 || true
echo "upper() before default:" >&2
rg -nP '\.upper\(\)\s*\|\s*default' .specify/agent-templates -n -C1 || true
echo "join before default:" >&2
rg -nP '\|\s*join\([^)]*\)\s*\|\s*default' .specify/agent-templates -n -C1 || trueLength of output: 7995
Blocker: unsupported Jinja patterns in templates
- Remove all uses of
| ternary(…); replace with inline… if … else …expressions. - Always apply
| default(…)before calling filters/attributes (e.g.,upper,join) to avoid undefined‐variable errors.
Affects multiple files under.specify/agent-templates/(code-reviewer.md.j2,implementer.md.j2,spec-reviewer.md.j2,context.md.j2,generic-agent.md.j2, etc.).
🤖 Prompt for AI Agents
In .specify/agent-templates/code-reviewer.md.j2 around lines 30 to 33, the
template uses unsupported Jinja patterns: remove all uses of the | ternary(...)
filter and replace them with inline conditional expressions using the Jinja
ternary syntax (value_if_true if condition else value_if_false); also ensure any
variable that will be filtered or have attributes called (e.g., | upper, | join)
is first wrapped with | default(...) to provide a safe fallback before applying
further filters. Apply these same changes across the other templates under
.specify/agent-templates/ (implementer.md.j2, spec-reviewer.md.j2,
context.md.j2, generic-agent.md.j2, etc.), replacing | ternary(...) usages and
moving/adding | default(...) before subsequent filters or attribute lookups so
templates no longer use unsupported filter patterns.
| - **CI/CD**: {{ test_strategy.cicd_integration | ternary("Integrated", "Not integrated") }} | ||
| {% endif %} |
There was a problem hiding this comment.
Fix join/default order to avoid errors
Default to a list before join:
-- **Test Types**: {{ test_strategy.test_types | join(", ") | default("Unit, Integration, E2E") }}
+- **Test Types**: {{ (test_strategy.test_types | default(["Unit","Integration","E2E"])) | join(", ") }}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .specify/agent-templates/context.md.j2 around lines 318-319, the template
uses a value that may be None before calling join; ensure you default that
variable to an empty list before joining. Update the template expression to
apply default([]) (or an equivalent) before |join so the join always operates on
a list and won't error when the value is missing or null.
| - **Coverage Target**: {{ test_strategy.coverage_target | default("80") }}% | ||
| - **Test Types**: {{ test_strategy.test_types | join(", ") | default("Unit, Integration, E2E") }} | ||
| - **CI/CD**: {{ test_strategy.cicd_integration | ternary("Integrated", "Not integrated") }} | ||
| {% endif %} |
There was a problem hiding this comment.
Replace non-existent ‘ternary’ filter
-- **CI/CD**: {{ test_strategy.cicd_integration | ternary("Integrated", "Not integrated") }}
+- **CI/CD**: {{ "Integrated" if test_strategy.cicd_integration else "Not integrated" }}📝 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.
| {% endif %} | |
| - **CI/CD**: {{ "Integrated" if test_strategy.cicd_integration else "Not integrated" }} | |
| {% endif %} |
🤖 Prompt for AI Agents
.specify/agent-templates/context.md.j2 around line 319: the template uses a
non-existent 'ternary' filter — locate where '| ternary(...)' is used and
replace it with standard Jinja2 conditional syntax or built-in filters;
specifically refactor to either the inline expression "value_if_true if
condition else value_if_false" or use the 'default' filter for fallback values,
ensuring the logic and whitespace/escaping remain correct.
| - **Frequency**: {{ refresh_schedule.frequency }} | ||
| - **Next Refresh**: {{ refresh_schedule.next_refresh }} | ||
| - **Auto-refresh**: {{ refresh_schedule.auto_refresh | ternary("Enabled", "Disabled") }} | ||
| {% endif %} |
There was a problem hiding this comment.
Replace ‘ternary’ filter for refresh schedule
-- **Auto-refresh**: {{ refresh_schedule.auto_refresh | ternary("Enabled", "Disabled") }}
+- **Auto-refresh**: {{ "Enabled" if refresh_schedule.auto_refresh else "Disabled" }}📝 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.
| - **Frequency**: {{ refresh_schedule.frequency }} | |
| - **Next Refresh**: {{ refresh_schedule.next_refresh }} | |
| - **Auto-refresh**: {{ refresh_schedule.auto_refresh | ternary("Enabled", "Disabled") }} | |
| {% endif %} | |
| - **Frequency**: {{ refresh_schedule.frequency }} | |
| - **Next Refresh**: {{ refresh_schedule.next_refresh }} | |
| - **Auto-refresh**: {{ "Enabled" if refresh_schedule.auto_refresh else "Disabled" }} | |
| {% endif %} |
🤖 Prompt for AI Agents
.specify/agent-templates/context.md.j2 around lines 431 to 434: the template
uses the non-standard 'ternary' filter to render auto-refresh state; replace it
with a Jinja2 inline conditional expression so the line outputs "Enabled" when
refresh_schedule.auto_refresh is truthy and "Disabled" otherwise (i.e., change
the filter use to an inline if-else expression).
| #### {{ issue.severity.upper() }}: {{ issue.title }} | ||
| **Section**: {{ issue.section | default("General") }} | ||
| **Issue**: {{ issue.description }} | ||
| **Impact**: {{ issue.impact_on_implementation }} | ||
|
|
There was a problem hiding this comment.
Guard severity before upper()
-#### {{ issue.severity.upper() }}: {{ issue.title }}
+#### {{ (issue.severity | default("HIGH")) | upper }}: {{ issue.title }}📝 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.
| #### {{ issue.severity.upper() }}: {{ issue.title }} | |
| **Section**: {{ issue.section | default("General") }} | |
| **Issue**: {{ issue.description }} | |
| **Impact**: {{ issue.impact_on_implementation }} | |
| #### {{ (issue.severity | default("HIGH")) | upper }}: {{ issue.title }} | |
| **Section**: {{ issue.section | default("General") }} | |
| **Issue**: {{ issue.description }} | |
| **Impact**: {{ issue.impact_on_implementation }} |
🤖 Prompt for AI Agents
In .specify/agent-templates/spec-reviewer.md.j2 around lines 36 to 40, the
template calls issue.severity.upper() without guarding for a
missing/null/undefined severity which will raise an error; change the template
to safely access and uppercase severity (for example use a default or
conditional: render (issue.severity or 'UNKNOWN').upper() or wrap with a
conditional check and fallback to 'UNKNOWN'/'UNSPECIFIED') so the template never
fails when severity is absent.
| template_path = repo_root / ".specify" / "agent-templates" / agent_template_name | ||
|
|
||
| if not template_path.exists(): | ||
| # Try generic agent template as fallback | ||
| generic_template = ( | ||
| repo_root / ".specify" / "agent-templates" / "generic-agent.md.j2" | ||
| ) | ||
| if generic_template.exists(): | ||
| template_path = generic_template | ||
| else: | ||
| return False, f"Agent template not found: {template_path}" |
There was a problem hiding this comment.
Honor package-discovered templates when rendering
discover_agent_types happily finds templates shipped in the package, but scaffold_agent ignores that information and unconditionally looks under <repo>/.specify/agent-templates/. When the template only exists in the package (the entire point of the fallback), template_path.exists() is false and we fall back to generic or fail outright. Please plumb the discovered AgentType.template_path through so we render the actual template we just advertised.
🤖 Prompt for AI Agents
In .specify/scripts/scaffold-agent.py around lines 299 to 309, scaffold_agent
currently always checks for templates under <repo>/.specify/agent-templates and
ignores the AgentType discovered template path; update the function to prefer
and use the AgentType.template_path (if provided by discover_agent_types) before
falling back to the repo local template or generic-template. Concretely: accept
or obtain the AgentType object (or its .template_path string/Path) into
scaffold_agent, use that path as the primary template_path if present, and only
if it is missing/None proceed to check repo_root/.specify/agent-templates and
then generic; also handle package-embedded templates by detecting non-filesystem
template paths (or strings referring to package resources) and render them via
importlib.resources or pkgutil when Path.exists() is false.
| available_names = [agent.name for agent in agent_types] | ||
| if ( | ||
| request.agent_name not in available_names | ||
| and not ( | ||
| base_output_dir.parent | ||
| / ".specify" | ||
| / "agent-templates" | ||
| / "generic-agent.md.j2" | ||
| ).exists() |
There was a problem hiding this comment.
Fix generic-template fallback path
The guard that should allow unknown agents to fall back to generic-agent.md.j2 never succeeds—the path resolves to .specify/.specify/agent-templates/... when output_dir is the default .specify/agents. As a result, even with a generic template present, we wrongly reject the request.
Reuse the repo root when building this path, e.g.:
- and not (
- base_output_dir.parent
- / ".specify"
- / "agent-templates"
- / "generic-agent.md.j2"
- ).exists()
+ and not (
+ helpers.get_repo_root()
+ / ".specify"
+ / "agent-templates"
+ / "generic-agent.md.j2"
+ ).exists()📝 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.
| available_names = [agent.name for agent in agent_types] | |
| if ( | |
| request.agent_name not in available_names | |
| and not ( | |
| base_output_dir.parent | |
| / ".specify" | |
| / "agent-templates" | |
| / "generic-agent.md.j2" | |
| ).exists() | |
| available_names = [agent.name for agent in agent_types] | |
| if ( | |
| request.agent_name not in available_names | |
| and not ( | |
| helpers.get_repo_root() | |
| / ".specify" | |
| / "agent-templates" | |
| / "generic-agent.md.j2" | |
| ).exists() |
🤖 Prompt for AI Agents
.specify/scripts/scaffold-agent.py around lines 512 to 520: the fallback check
builds the generic template path relative to base_output_dir causing
`.specify/.specify/...` and never matching; change the path construction to use
the repository root (e.g., locate repo_root =
Path(__file__).resolve().parents[...] or use a known project root) and join
repo_root / ".specify" / "agent-templates" / "generic-agent.md.j2" so the
existence check correctly finds the template and allows unknown agents to fall
back to it.
| error_msg = f"Failed to get context file for {assistant_name}: {e}" | ||
| if json_output: | ||
| print(json.dumps({"error": error_msg})) | ||
| else: | ||
| echo_error(error_msg) | ||
| results[assistant_name] = {"success": False, "error": str(e)} | ||
| continue |
There was a problem hiding this comment.
Don’t print per-assistant JSON errors when aggregating results.
Printing JSON mid-loop pollutes final JSON. Accumulate errors in results only.
- if json_output:
- print(json.dumps({"error": error_msg}))
- else:
- echo_error(error_msg)
+ if not json_output:
+ echo_error(error_msg)
results[assistant_name] = {"success": False, "error": str(e)}
continue📝 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.
| error_msg = f"Failed to get context file for {assistant_name}: {e}" | |
| if json_output: | |
| print(json.dumps({"error": error_msg})) | |
| else: | |
| echo_error(error_msg) | |
| results[assistant_name] = {"success": False, "error": str(e)} | |
| continue | |
| error_msg = f"Failed to get context file for {assistant_name}: {e}" | |
| if not json_output: | |
| echo_error(error_msg) | |
| results[assistant_name] = {"success": False, "error": str(e)} | |
| continue |
| # Try to get template from package agent-prompts | ||
| try: | ||
| agent_prompts_dir = ( | ||
| importlib.resources.files(templates_pkg) / "agent-prompts" | ||
| ) | ||
| if generic_template.exists(): | ||
| template_path = generic_template | ||
| else: | ||
| return False, f"Agent template not found: {template_path}" | ||
| template_path = agent_prompts_dir / agent_template_name | ||
|
|
||
| if not template_path.is_file(): | ||
| return False, f"Agent prompt template not found: {agent_template_name}. Available agents: code-reviewer, documentation-reviewer, implementer, spec-reviewer, architecture-reviewer, test-reviewer" | ||
| except Exception as e: | ||
| return False, f"Failed to locate agent prompt templates: {str(e)}" | ||
|
|
There was a problem hiding this comment.
Fix validation/rendering inconsistency (package prompts vs project templates).
Validation allows a project-level generic fallback, but rendering only loads package agent-prompts. This can pass validation then fail at render. Remove the dead fallback or add a package-aware fallback consistently.
Also, avoid hard-coded “Available agents” in the error; compute dynamically from discovery.
@@
- if not template_path.is_file():
- return False, f"Agent prompt template not found: {agent_template_name}. Available agents: code-reviewer, documentation-reviewer, implementer, spec-reviewer, architecture-reviewer, test-reviewer"
+ if not template_path.is_file():
+ # Build dynamic available list for the error
+ ok, all_types = discover_agent_types(helpers)
+ available = ", ".join(sorted([t.name for t in all_types])) if ok else "none"
+ return False, f"Agent prompt template not found: {agent_template_name}. Available agents: {available}"
@@
- if (
- request.agent_name not in available_names
- and not (
- base_output_dir.parent
- / ".specify"
- / "agent-templates"
- / "generic-agent.md.j2"
- ).exists()
- ):
+ if request.agent_name not in available_names:
error_msg = f"Agent type '{request.agent_name}' not found. Available: {', '.join(available_names)}"
failed_agents.append({"agent": agent_spec, "error": error_msg})
if not json_mode:
echo_error(f"❌ {error_msg}")
continueAlso applies to: 513-523
🤖 Prompt for AI Agents
In src/specify_cli/templates/scripts/scaffold-agent.py.j2 around lines 300 to
311 (also apply same fix at 513-523): the render step only looks in the package
"agent-prompts" and returns a hard-coded "Available agents" list which can
mismatch validation; change rendering to mirror validation by first attempting
to load the template from the package agent-prompts, and if not found then
attempt the project-level templates fallback (or conversely remove the fallback
from validation to keep behavior consistent), and replace the hard-coded
available-agents message by computing the list dynamically (discover agent
template names by listing files in the package agent-prompts and the project
templates directory, deduplicate, and present that joined list in the error).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/specify_cli/templates/scripts/scaffold-agent.py.j2 (1)
300-311: Fix validation/rendering inconsistency and hard-coded agent list.This issue was flagged in a previous review and remains unresolved. The rendering logic only loads templates from package
agent-prompts, but the error message contains a hard-coded list of agents that may become stale. Additionally, validation at lines 515-523 checks for a project-levelgeneric-agent.md.j2fallback, but the rendering here doesn't support that fallback, creating an inconsistency.Apply this diff to dynamically generate the available agents list and align validation with rendering:
try: agent_prompts_dir = ( importlib.resources.files(templates_pkg) / "agent-prompts" ) template_path = agent_prompts_dir / agent_template_name if not template_path.is_file(): - return False, f"Agent prompt template not found: {agent_template_name}. Available agents: code-reviewer, documentation-reviewer, implementer, spec-reviewer, architecture-reviewer, test-reviewer" + # Build dynamic available list for the error + ok, all_types = discover_agent_types() + available = ", ".join(sorted([t.name for t in all_types])) if ok else "none" + return False, f"Agent prompt template not found: {agent_template_name}. Available agents: {available}" except Exception as e: return False, f"Failed to locate agent prompt templates: {str(e)}"Additionally, at lines 515-523, remove the project-level fallback check to align with the package-only rendering approach:
# Validate agent type exists available_names = [agent.name for agent in agent_types] - if ( - request.agent_name not in available_names - and not ( - base_output_dir.parent - / ".specify" - / "agent-templates" - / "generic-agent.md.j2" - ).exists() - ): + if request.agent_name not in available_names: error_msg = f"Agent type '{request.agent_name}' not found. Available: {', '.join(available_names)}" failed_agents.append({"agent": agent_spec, "error": error_msg}) if not json_mode: echo_error(f"❌ {error_msg}") continue
🧹 Nitpick comments (1)
src/specify_cli/templates/scripts/scaffold-agent.py.j2 (1)
69-69: Remove unused parameter.The
helpersparameter is assigned to_but never used in the function. Consider removing it from the signature if it's not needed for future extensibility.Apply this diff:
-def discover_agent_types(helpers: ScriptHelpers) -> Tuple[bool, List[AgentType]]: +def discover_agent_types() -> Tuple[bool, List[AgentType]]: """ Auto-discover available agent types from package agent-prompts. Args: - helpers: ScriptHelpers instance Returns: Tuple[bool, List[AgentType]]: (success, agent_types) """ - _ = helpersAnd update the call site at line 438:
- success, agent_types = discover_agent_types(helpers) + success, agent_types = discover_agent_types()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.specify/scripts/setup-plan.py(2 hunks)src/specify_cli/templates/commands/guide.md.j2(1 hunks)src/specify_cli/templates/commands/implement.md.j2(1 hunks)src/specify_cli/templates/scripts/scaffold-agent.py.j2(10 hunks)src/specify_cli/templates/scripts/update-agent-context.py.j2(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: test (windows-latest, 3.13)
- GitHub Check: test (windows-latest, 3.11)
🔇 Additional comments (8)
src/specify_cli/templates/commands/guide.md.j2 (1)
14-107: LGTM! Comprehensive manual workflow guidance.The updated template provides clear, well-structured instructions for manual feature implementation. The phases (Setup, Task Execution Strategy, Manual Progress Tracking, Task Categories, Mixed AI/Human Workflow, Error Recovery) are logically organized and actionable.
src/specify_cli/templates/scripts/update-agent-context.py.j2 (2)
170-172: LGTM! Centralized spec directory discovery.Using
ScriptHelpers.find_feature_directory_for_workflow()centralizes the logic and reduces code duplication. This approach handles both traditional and no-branch workflows correctly.
72-137: LGTM! JSON output support added correctly.The
json_outputparameter is properly threaded through the function, with conditional console prints suppressed when JSON mode is enabled. This maintains backward compatibility while adding useful structured output support.src/specify_cli/templates/scripts/scaffold-agent.py.j2 (3)
199-199: LGTM! Centralized workflow directory discovery.Using
find_feature_directory_for_workflow()standardizes spec directory discovery across the codebase, supporting both feature branch and no-branch workflows.
221-245: LGTM! Enhanced task parsing with structured objects.The updated task parsing creates structured task objects with
id,title,status, andpriorityfields. The ID parsing logic handles both explicit task IDs (e.g., "T001 Task description") and auto-generates IDs when not present. The field name change fromtaskstocurrent_tasks(line 257) aligns with the updated context structure.
346-352: LGTM! Safe template rendering with package resources.Using
importlib.resource 35B2 s.as_file()properly converts theTraversableto a real filesystemPathfor template rendering. This ensures compatibility with the rendering function while maintaining package-based resource loading.src/specify_cli/templates/commands/implement.md.j2 (2)
14-33: LGTM! Subagent workflow with graceful fallback.The addition of subagent support using
scaffold-agentprovides a modern workflow for environments that support it (e.g., Claude Code's Task tool), while maintaining a clear fallback path for environments without subagent support. The instructions for both paths are clear and actionable.
36-67: LGTM! Streamlined execution phases.The phase updates improve clarity:
- Phase 2 renamed to "TDD Enforcement (MANDATORY)" emphasizes test-first development
- Merging Phase 3 and Phase 4 into a single "Task Execution" phase simplifies the workflow
- Error handling and progress tracking guidance is concise and practical
| "branch_name": result["BRANCH"], | ||
| "author_name": "", | ||
| "creation_date": "2025-09-18", | ||
| "creation_date": "2025-10-04", |
There was a problem hiding this comment.
Use dynamic date generation instead of hardcoded dates.
The creation_date is hardcoded to "2025-10-04", which means all generated plan and spec files will have the same creation date regardless of when they're actually created. This defeats the purpose of timestamp metadata and will require constant manual updates.
Apply this diff to use the current date dynamically:
+from datetime import datetime
+
#!/usr/bin/env python3
"""
Setup implementation plan structure for current branch.Then update both occurrences:
context_dict={
"feature_name": result["BRANCH"],
"branch_name": result["BRANCH"],
"author_name": "",
- "creation_date": "2025-10-04",
+ "creation_date": datetime.now().strftime("%Y-%m-%d"),
"project_name": "spec-kit-improved",
},Also applies to: 169-169
🤖 Prompt for AI Agents
In .specify/scripts/setup-plan.py around lines 138 and 169, the creation_date
fields are hardcoded to "2025-10-04"; replace the hardcoded string with a
dynamically generated current date string (ISO YYYY-MM-DD) by importing or using
datetime.date.today().isoformat() (or equivalent) and assign that value to
creation_date in both locations so generated plan/spec files use the actual
creation date at runtime.
There was a problem hiding this comment.
Bug: Template Path Construction Error
The fallback logic for generic-agent.md.j2 constructs an incorrect path. Using base_output_dir.parent leads to a nested .specify/.specify directory, which prevents the generic agent template from being found.
.specify/scripts/scaffold-agent.py#L515-L520
spec-kit-improved/.specify/scripts/scaffold-agent.py
Lines 515 to 520 in 751a286
src/specify_cli/templates/scripts/scaffold-agent.py.j2#L514-L520
spec-kit-improved/src/specify_cli/templates/scripts/scaffold-agent.py.j2
Lines 514 to 520 in 751a286
| except (OSError, MemoryError) as e: | ||
| success = False | ||
| result = {"error": f"Failed to read {contract_file}: {e}"} | ||
| break |
There was a problem hiding this comment.
Bug: Indentation Error in Exception Handling
The new except (OSError, MemoryError) block is incorrectly indented, placing it outside the try block that handles file opening and JSON loading. This prevents it from catching OSError or MemoryError exceptions that occur during these operations, leading to unhandled errors.
| "status": status, | ||
| "priority": "Medium", | ||
| }) | ||
| task_counter += 1 |
There was a problem hiding this comment.
Bug: Template Context Structure Altered
The context dictionary passed to templates has changed its structure. specs and plans are now nested as {'spec_content': '...'} instead of providing content directly. Also, the tasks variable, now current_tasks, changed from a list of strings to a list of dictionaries. These changes will likely break existing templates expecting the previous data structures.
| """Update the assistant context file with comprehensive project context.""" | ||
| if not context_file.exists(): | ||
| console.print(f"[yellow]Context file not found: {context_file}[/yellow]") | ||
| if not json_output: |
There was a problem hiding this comment.
Bug: Missing Parameter Causes Function Call Error
The update_context_file function in .specify/scripts/update-agent-context.py is missing the json_output parameter in its definition. This causes a TypeError when the function is called, as the call site now passes this argument.
Summary by CodeRabbit
New Features
Improvements
Chores