Merge 15+ PRs: Security, Windows fixes, Knowledge files, Chat persistence, and more#117
Merge 15+ PRs: Security, Windows fixes, Knowledge files, Chat persistence, and more#117getworken wants to merge 166 commits intoAutoForgeAI:masterfrom
Conversation
|
Important Review skippedToo many files! This PR contains 135 files, which is 35 over the limit of 100. You can disable this status message by setting the
📝 WalkthroughWalkthroughAdds mandatory infrastructure gating (five infra features indices 0–4) and DB decision phase; introduces a SQLAlchemy/SQLite data layer with migrations and repository, structured and project-scoped logging, rate-limit detection/backoff, atomic MCP feature operations and quality gates, many new routers/services, analyzers, CI/deploy artifacts, UI features, and expanded tests. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant UI as Web UI
participant API as FastAPI
participant MCP as MCP Tooling
participant Repo as FeatureRepository
participant DB as SQLite DB
end
UI->>API: POST /api/import/create-features (bulk)
API->>MCP: invoke feature_create_bulk (temp MCP config)
MCP->>Repo: request create features (EXCLUSIVE transaction)
Repo->>DB: INSERT feature rows (ensure infra indices 0..4)
DB-->>Repo: commit -> created IDs
Repo-->>MCP: return created IDs
MCP-->>API: tool response
API-->>UI: return created feature IDs / summary
sequenceDiagram
rect rgba(255,200,200,0.5)
participant Agent as Autonomous Agent
participant LLM as External LLM API
participant RateUtil as rate_limit_utils
participant Repo as FeatureRepository
participant DB as SQLite DB
end
Agent->>LLM: request
alt LLM rate-limits / errors
LLM-->>Agent: 429 / rate-limit message
Agent->>RateUtil: is_rate_limit_error(msg)?
RateUtil-->>Agent: true
Agent->>RateUtil: parse_retry_after(msg)
RateUtil-->>Agent: retry_after or None
Agent->>Agent: sleep (retry_after or exponential backoff)
Agent->>LLM: retry request
else success
LLM-->>Agent: response
Agent->>Repo: record attempt/result
Repo->>DB: persist attempt/result
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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. 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 |
getworken
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
README.md (1)
275-275: Update README.md to reflect React 19.The documentation states "React 18 with TypeScript" on line 275, but the project uses React 19 (^19.0.0 in package.json). Update the README.md to reflect the correct version.
server/routers/assistant_chat.py (1)
221-225: Document the newresumemessage type in the WebSocket protocol.
The protocol comment still lists onlystart,message, andping, so clients won’t see the new handshake option.✍️ Suggested doc update
- - {"type": "start", "conversation_id": int | null} - Start/resume session + - {"type": "start", "conversation_id": int | null} - Start session + - {"type": "resume", "conversation_id": int} - Resume session without greetingserver/routers/schedules.py (1)
135-144: max_concurrency is silently dropped during schedule creation.
ScheduleCreateexposesmax_concurrency(default=3), but the Schedule constructor at lines 135-144 never receives it. User-specified concurrency values are lost, and the database always stores the default (3). Wire through the field or use a bulk assignment to preserve user input:🛠️ Possible fix
- # Create schedule record - schedule = Schedule( - project_name=project_name, - start_time=data.start_time, - duration_minutes=data.duration_minutes, - days_of_week=data.days_of_week, - enabled=data.enabled, - yolo_mode=data.yolo_mode, - model=data.model, - ) + # Create schedule record + schedule_data = data.model_dump(exclude_unset=True) + schedule = Schedule( + project_name=project_name, + **schedule_data, + )server/services/assistant_chat_session.py (1)
246-386:skip_greetingis unused — greeting still sent.
The parameter is documented but not applied; new conversations will always emit the greeting even whenskip_greeting=True.🐛 Suggested fix
- if is_new_conversation: - # New conversations don't need history loading - self._history_loaded = True - try: - greeting = f"Hello! I'm your project assistant for **{self.project_name}**. I can help you understand the codebase, manage features (create, edit, delete, and deprioritize), and answer questions about the project. What would you like to do?" - - # Store the greeting in the database - add_message(self.project_dir, self.conversation_id, "assistant", greeting) - - yield {"type": "text", "content": greeting} - yield {"type": "response_done"} - except Exception as e: - logger.exception("Failed to send greeting") - yield {"type": "error", "content": f"Failed to start conversation: {str(e)}"} + if is_new_conversation: + # New conversations don't need history loading + self._history_loaded = True + if skip_greeting: + yield {"type": "response_done"} + else: + try: + greeting = f"Hello! I'm your project assistant for **{self.project_name}**. I can help you understand the codebase, manage features (create, edit, delete, and deprioritize), and answer questions about the project. What would you like to do?" + + # Store the greeting in the database + add_message(self.project_dir, self.conversation_id, "assistant", greeting) + + yield {"type": "text", "content": greeting} + yield {"type": "response_done"} + except Exception as e: + logger.exception("Failed to send greeting") + yield {"type": "error", "content": f"Failed to start conversation: {str(e)}"}parallel_orchestrator.py (1)
381-448: Respectmax_concurrencywhen determining testing-agent count.Line 422 uses
testing_agent_ratiodirectly; the docstring saysmax_concurrencyalso caps testing agents. Consider clampingdesiredtoself.max_concurrency(or update the doc).🛠️ Suggested update
- desired = self.testing_agent_ratio + desired = min(self.testing_agent_ratio, self.max_concurrency)
🤖 Fix all issues with AI agents
In `@agent.py`:
- Around line 182-203: Move the psutil import into the try block and handle
ModuleNotFoundError explicitly: inside the try, import psutil then perform
parent_process = psutil.Process().parent(), parent_name = parent_process.name()
if parent_process else ""; call clear_stuck_features(project_dir) only when
"python" not in parent_name.lower(); add an except ModuleNotFoundError branch
that falls back to the safe behavior (call clear_stuck_features(project_dir));
keep the existing broad except Exception branch to preserve the current
fallback-on-error behavior.
In `@api/dependency_resolver.py`:
- Around line 173-200: The current fail-safe checks len(visited) and returns
True, which can mislabel large acyclic graphs as cycles; change the traversal to
enforce a depth-based cap using MAX_DEPENDENCY_DEPTH instead. Update the DFS to
have stack entries carry depth (e.g., stack of (node_id, depth) starting at
depth 0 for target_id), check the popped node's depth and if it exceeds
MAX_DEPENDENCY_DEPTH handle as a fail-safe (same behavior as before if you must
assume cycle) rather than using len(visited), and when pushing dependencies push
(dep_id, depth+1); keep checks for source_id, visited set, and feature_map
lookups (symbols: stack, visited, MAX_DEPENDENCY_DEPTH, source_id, target_id,
feature_map, current.get("dependencies")).
- Around line 288-303: When re-queuing the current node for later dependencies
the code uses path[:-1] which drops the current node and can miss cycles; change
the push that appends (node_id, path[:-1] if path else [], dep_index + 1) to
preserve the full path (e.g., use path) so the stack entry is (node_id, path,
dep_index + 1), keeping node_id present for subsequent dependency checks
involving dep_index, deps, stack, node_id, path, visited, rec_stack and
parent_map.
In `@mcp_server/feature_mcp.py`:
- Around line 953-964: Deletion of a Feature can orphan dependencies in other
Feature records; in the delete flow around get_session(),
session.query(Feature).filter(...).first(), and
session.delete(feature)/session.commit(), either prevent deletion when other
Features reference this feature_id or cascade-update those Features to remove
the ID: check for dependents via
session.query(Feature).filter(Feature.dependencies.contains(feature_id)) (or
equivalent depending on how dependencies are stored), and if any exist return an
error message rejecting deletion; alternatively iterate those dependent Feature
instances, remove the feature_id from their dependencies list, commit those
updates before deleting the Feature, and then proceed with
session.delete(feature) and session.commit().
- Around line 353-364: Add a new non-null integer column regression_count to the
Feature model (add a Column(Integer, nullable=False, server_default='0',
default=0) attribute named regression_count on the Feature class) and update the
migrations registry by adding a migration entry that alters the features table
to add the regression_count column (with DEFAULT 0 and backfill/nullable
handling as needed) so existing databases get the new column; ensure the model
attribute name Feature.regression_count matches the migration and any ORM
defaults are applied so the queries and the increment loop work without runtime
errors.
In `@parallel_orchestrator.py`:
- Around line 585-639: _spawn_testing_agent currently launches subprocess.Popen
without stdin redirection or Windows-specific creation flags; update the Popen
call in _spawn_testing_agent to include stdin=subprocess.DEVNULL and, when
os.name == "nt", pass creationflags=subprocess.CREATE_NO_WINDOW (or mirror the
platform-safe logic used in the coding-agent spawner in this file) so testing
agents won't block on stdin or pop up consoles on Windows; keep existing
stdout/stderr/text/cwd/env arguments and reuse the same platform-guard/helper
used elsewhere in this module to avoid duplicating platform detection.
In `@quality_gates.py`:
- Around line 348-353: Replace the deprecated datetime.utcnow() call used for
the "timestamp" value with a timezone-aware timestamp: use
datetime.now(timezone.utc).isoformat() and ensure timezone is imported from
datetime; update the expression that currently reads
datetime.utcnow().isoformat() (in the return dict producing "timestamp") to
datetime.now(timezone.utc).isoformat() so the timestamp is UTC-aware and not
relying on the deprecated API.
In `@requirements.txt`:
- Line 8: Update the tight pinned requirements to a range that allows compatible
minor/patch updates: replace python-multipart~=0.0.17 with a range like
python-multipart>=0.0.17,<0.1.0 and replace slowapi~=0.1.9 with
slowapi>=0.1.9,<0.2.0 so the package names python-multipart and slowapi are
referenced and pip can resolve compatible newer patch/minor releases.
In `@security.py`:
- Around line 52-54: Replace the current pattern re.compile(r'\\x00') with a
regex that matches both the literal sequence "\x00" and an actual NUL byte, e.g.
re.compile(r'(?:\\x00|\x00)'), and update the associated message (currently
"null byte injection (hex)") to something like "null byte injection (hex and
raw)" so the rule in security.py will catch both textual and raw NUL bytes.
In `@server/routers/assistant_chat.py`:
- Around line 292-325: The resume path currently accepts a conversation_id and
may replace the client's active session before validating the ID; update the
logic to first validate the conversation exists and create the new session
successfully before swapping it in. Specifically, check the conversation exists
(e.g., via an existing helper like get_conversation_by_id or a new validation
call) and only call create_session and iterate session.start(skip_greeting=True)
after validation succeeds; do not close or overwrite the current active session
until create_session completes without error, and on any failure retain the
existing session and send the error response (use the existing conversation_id,
create_session, and session variables to locate where to change the flow).
In `@server/routers/projects.py`:
- Around line 615-639: The upload_knowledge_file handler is vulnerable because
it uses file.filename directly; validate/sanitize the filename (the same way
get_knowledge_file and delete_knowledge_file do) before creating filepath:
ensure the provided file.filename contains no path separators or traversal
segments (e.g., compare Path(file.filename).name == file.filename or reject when
".." or os.path.isabs/file.separator present), and if invalid raise
HTTPException(400). Update upload_knowledge_file to use the validated filename
when constructing filepath and writing, referencing symbols file.filename,
upload_knowledge_file, get_knowledge_dir, and filepath.
In `@start_ui.sh`:
- Around line 33-37: The venv activation block uses an undefined variable
SCRIPT_DIR and runs too early; replace the check from "$SCRIPT_DIR/venv" to a
relative path like "./venv" (or define SCRIPT_DIR at the top if you intended to
use it) and move the activation step so it runs after the existing venv
validation/recreation logic (the block that validates and possibly recreates the
virtual environment), ensuring you only source "./venv/bin/activate" after the
validation completes.
In `@structured_logging.py`:
- Around line 436-439: Replace the deprecated datetime.utcnow() calls used to
default the since and until variables with datetime.now(timezone.utc);
specifically update the block that sets since = datetime.utcnow() -
timedelta(hours=24) and until = datetime.utcnow() so both use
datetime.now(timezone.utc) (e.g., since = datetime.now(timezone.utc) -
timedelta(hours=24) and until = datetime.now(timezone.utc)), matching the
timezone-aware usage already used in emit().
In `@tests/conftest.py`:
- Around line 50-69: The temp_db fixture calls create_database which caches DB
engines; add a teardown after yield to dispose the cached engine so temp
dirs/files aren't locked—import the engine-disposal helper from api.database
(the project’s disposal function) and call it with project_dir in temp_db’s
post-yield cleanup; apply the same pattern to async_temp_db, mock_project_dir,
and populated_db fixtures to ensure their cached engines are disposed after each
test.
In `@tests/test_async_examples.py`:
- Around line 91-136: The test test_bash_security_hook_with_project_dir is using
the wrong payload shape for bash_security_hook; change the call to pass the
tool-shaped payload like the other tests (use {"tool_name": "Bash",
"tool_input": {"command": "npm install"}}) while keeping the
context={"project_dir": str(temp_project_dir)} so the bash_security_hook
function is exercised with the same input shape as
test_bash_security_hook_allowed and test_bash_security_hook_blocked.
In `@tests/test_security.py`:
- Around line 676-738: The null-byte test in test_command_injection_prevention
is using the two-character sequence "\\x00" instead of an actual NUL, so
pre_validate_command_safety won't see a real null byte; update the test case
entry (in test_command_injection_prevention) to use a real null byte in the
string (e.g., "cat file\x00.txt" or "cat file\0.txt") so the pre-validation will
detect null-byte injection when calling pre_validate_command_safety.
In `@ui/src/App.tsx`:
- Around line 242-269: handleIDESelect currently calls
updateSettings.mutateAsync without error handling and hides the modal before the
update completes, and handleOpenInIDE calls openProjectInIDE without catching
errors; updateSettings failures cause an unhandled rejection and prevent opening
the IDE. Modify handleIDESelect to await updateSettings.mutateAsync inside a
try/catch, log or surface errors on failure and only call
setShowIDESelection(false) after a successful update (keep the modal open if the
update fails), and then call handleOpenInIDE(ide) (awaiting it if appropriate);
also ensure handleOpenInIDE wraps the openProjectInIDE(selectedProject,
ideToUse) call in try/catch and logs errors while still clearing setIsOpeningIDE
in a finally block so failures don’t silently fail (reference functions:
handleIDESelect, handleOpenInIDE, updateSettings.mutateAsync, openProjectInIDE,
setShowIDESelection, setIsOpeningIDE).
In `@ui/src/hooks/useAssistantChat.ts`:
- Around line 339-352: The timeout/error handling for the WebSocket leaves a
CONNECTING socket alive which blocks future retries; update the logic in the
retry branch where retryCount >= maxRetries and the else branch that handles
closed/error state to explicitly close and null out the WebSocket instance (the
same object used by connect()/start()) before clearing connectTimeoutRef and
calling setIsLoading(false)/onError, ensuring the WebSocket reference is closed
and set to null so subsequent start() calls can create a fresh connection;
target symbols: connectTimeoutRef, retryCount, maxRetries, connect(), start(),
and the WebSocket instance variable.
🧹 Nitpick comments (23)
requirements.txt (1)
3-15: Standardize versioning precision for consistency.The tilde operator (~=) behaves differently with two-component vs. three-component versions:
~=1.0.0allows only1.0.x(tight)~=2.0allows all2.x(broad)Current mix creates inconsistent update windows:
- Very tight:
python-dotenv~=1.0.0,python-multipart~=0.0.17,slowapi~=0.1.9- Broad:
sqlalchemy~=2.0,psutil~=6.0,pyyaml~=6.0,pydantic-settings~=2.0- Medium:
fastapi~=0.115,uvicorn[standard]~=0.32,websockets~=13.0📋 Recommendation: Standardize to three-component versions
Use three-component versions for predictable dependency resolution:
-sqlalchemy~=2.0 +sqlalchemy~=2.0.0 -fastapi~=0.115 +fastapi~=0.115.0 -uvicorn[standard]~=0.32 +uvicorn[standard]~=0.32.0 -websockets~=13.0 +websockets~=13.0.0 -psutil~=6.0 +psutil~=6.0.0 -aiofiles~=24.0 +aiofiles~=24.0.0 -pywinpty~=2.0; sys_platform == "win32" +pywinpty~=2.0.0; sys_platform == "win32" -pyyaml~=6.0 +pyyaml~=6.0.0 -pydantic-settings~=2.0 +pydantic-settings~=2.0.0Alternatively, use two-component versions consistently if broader ranges are desired (though this increases risk of breaking changes).
server/services/terminal_manager.py (1)
14-23: Consider centralizing taskkill usage.There’s now similar taskkill logic in
server/utils/process_utils.py. Exposing a small shared helper (public, not underscored) and reusing it here would reduce duplication and keep Windows cleanup behavior consistent.Also applies to: 470-522
ui/src/components/ProjectSetupRequired.tsx (1)
48-52:handleRetryInitializerredundantly resets to 'idle' before immediately transitioning to 'starting'.The function sets
initializerStatusto'idle'on line 50, then immediately callshandleSpecCompletewhich sets it to'starting'on line 38. The intermediate'idle'state serves no purpose and could cause a brief re-render. Consider simplifying:♻️ Suggested simplification
const handleRetryInitializer = () => { setInitializerError(null) - setInitializerStatus('idle') handleSpecComplete('', yoloModeSelected) }server/schemas.py (1)
450-456: Consider extracting valid IDE list to a module-level constant.The list
['vscode', 'cursor', 'antigravity']is hardcoded in the validator. Extracting it to a constant (similar toVALID_MODELS) would improve maintainability and allow reuse in tests or documentation.♻️ Suggested refactor
+# Valid IDE choices for preferred_ide setting +VALID_IDES = ['vscode', 'cursor', 'antigravity'] + # ... in SettingsUpdate class ... `@field_validator`('preferred_ide') `@classmethod` def validate_preferred_ide(cls, v: str | None) -> str | None: - valid_ides = ['vscode', 'cursor', 'antigravity'] - if v is not None and v not in valid_ides: - raise ValueError(f"Invalid IDE. Must be one of: {valid_ides}") + if v is not None and v not in VALID_IDES: + raise ValueError(f"Invalid IDE. Must be one of: {VALID_IDES}") return vserver/routers/settings.py (1)
111-112: Cannot clearpreferred_ideonce set.The condition
if update.preferred_ide is not Nonemeans sendingnull/Nonewon't clear the setting—it will be ignored. If users should be able to clear their IDE preference (revert to no preference), consider handling empty string or a sentinel value:♻️ Option: Allow empty string to clear
- if update.preferred_ide is not None: - set_setting("preferred_ide", update.preferred_ide) + if update.preferred_ide is not None: + if update.preferred_ide == "": + # Clear the setting by deleting it (if registry supports delete) + # Or set to empty string + set_setting("preferred_ide", "") + else: + set_setting("preferred_ide", update.preferred_ide)This would require updating the schema validator to allow empty strings as a "clear" signal.
.claude/templates/testing_prompt.template.md (1)
24-27: Fenced code block missing language specification.The code block starting at line 24 lacks a language identifier. Adding one (e.g.,
textorplaintext) would satisfy linting and improve syntax highlighting in some renderers.♻️ Add language identifier
-``` +```text # 4. Get progress statistics Use the feature_get_stats tool</details> </blockquote></details> <details> <summary>ui/src/hooks/useAssistantChat.ts (1)</summary><blockquote> `50-82`: **Cover remaining MCP feature tools in getToolDescription.** A few known feature-management tools still fall through to the generic message, which makes the system output inconsistent. <details> <summary>♻️ Suggested additions</summary> ```diff case "feature_get_next": return "Getting next feature..."; + case "feature_claim_next": + return "Claiming next feature..."; case "feature_get_for_regression": return "Getting features for regression testing..."; case "feature_create": return `Creating feature: ${getStringValue(input.name, "new feature")}`; @@ case "feature_update": return `Updating feature #${getFeatureId(input.feature_id)}`; + case "feature_mark_passing": + return `Marking feature #${getFeatureId(input.feature_id)} as passing`; case "feature_delete": return `Deleting feature #${getFeatureId(input.feature_id)}`; + case "feature_add_dependency": + return `Adding dependency to feature #${getFeatureId(input.feature_id)}`; + case "feature_remove_dependency": + return `Removing dependency from feature #${getFeatureId(input.feature_id)}`;Based on learnings, the MCP feature tool set includes additional operations not yet mapped.
server/routers/projects.py (2)
506-534: Missing_init_imports()call in db-health endpoint.The
get_database_healthendpoint doesn't call_init_imports()at the start, unlike all other endpoints. While this particular endpoint doesn't use the lazy-imported functions, it's inconsistent with the pattern used throughout the file.More importantly, lines 525-527 duplicate the sys.path manipulation that
_init_imports()already handles.♻️ Suggested refactor
`@router.get`("/{name}/db-health", response_model=DatabaseHealth) async def get_database_health(name: str): """Check database health for a project. Returns integrity status, journal mode, and any errors. Use this to diagnose database corruption issues. """ + _init_imports() _, _, get_project_path, _, _ = _get_registry_functions() name = validate_project_name(name) project_dir = get_project_path(name) if not project_dir: raise HTTPException(status_code=404, detail=f"Project '{name}' not found") if not project_dir.exists(): raise HTTPException(status_code=404, detail="Project directory not found") - # Import health check function - root = Path(__file__).parent.parent.parent - if str(root) not in sys.path: - sys.path.insert(0, str(root)) - from api.database import check_database_health, get_database_path
566-575: Move import outside the loop.The
from datetime import datetimeimport on line 570 is inside theforloop, which means Python re-evaluates the import statement on every iteration. While Python caches imports, this is still unnecessary overhead and unconventional.♻️ Suggested fix
+ from datetime import datetime + files = [] for filepath in knowledge_dir.glob("*.md"): if filepath.is_file(): stat = filepath.stat() - from datetime import datetime files.append(KnowledgeFile( name=filepath.name, size=stat.st_size, modified=datetime.fromtimestamp(stat.st_mtime) ))structured_logging.py (1)
96-140: Consider using context manager for SQLite connections.The database connections are manually opened and closed, which could lead to resource leaks if an exception occurs between
connect()andclose(). While the lock ensures single access, using a context manager would be more robust.♻️ Example pattern
def _init_database(self) -> None: """Initialize the SQLite database for logs.""" with self._lock: with sqlite3.connect(self.db_path) as conn: cursor = conn.cursor() cursor.execute("PRAGMA journal_mode=WAL") # ... rest of initialization conn.commit() # Connection automatically closed on context exit.claude/templates/coding_prompt.template.md (1)
289-344: Server restart test is well-designed but has a potential timing issue.The server restart persistence test is critical for catching in-memory storage bugs. However, line 323-328 uses
./init.sh &which backgrounds the process, then immediately tries to curl. Thesleep 15may not be sufficient if the server takes longer to start.Consider adding a retry loop for the health check instead of a fixed sleep.
💡 Suggested improvement
# 4. RESTART the server with retry-based health check: ./init.sh & for i in {1..30}; do if curl -sf http://localhost:${PORT:-3000}/api/health >/dev/null 2>&1 || \ curl -sf http://localhost:${PORT:-3000} >/dev/null 2>&1; then echo "Server started successfully" break fi sleep 2 done.claude/templates/initializer_prompt.template.md (2)
45-78: Add language specifier to fenced code block.The code block starting at line 45 lacks a language specifier, which static analysis flagged. While this is a template showing MCP tool usage rather than actual code, adding a language hint improves rendering.
📝 Suggested fix
Change line 45 from:
to:Or if the content is JSON-like, use
jsonas the language specifier.
133-161: JSON comment syntax is not valid JSON.The example JSON block uses
//comments (lines 135, 142, 147, 152, 158), which are not valid in standard JSON. While this is documentation and the comments clarify the structure, it could be confusing sincefeature_create_bulklikely expects valid JSON.Consider using JSONC (JSON with Comments) specifier or restructuring as a table/list format.
ui/src/components/IDESelectionModal.tsx (1)
74-76: Consider adding accessibility for the selection indicator.The visual checkmark indicator (green circle) for the selected IDE lacks screen reader context. Consider adding an
aria-labelor visually hidden text.♿ Suggested accessibility improvement
{selectedIDE === ide.id && ( - <div className="w-4 h-4 rounded-full bg-primary" /> + <div className="w-4 h-4 rounded-full bg-primary" aria-hidden="true" /> )} + <span className="sr-only">{selectedIDE === ide.id ? '(selected)' : ''}</span>client.py (1)
82-88: Inconsistent logging:logger.warning.This function still uses
print()for the warning whileget_playwright_headless()(line 65) was updated to uselogger.warning(). For consistency with the logging migration, this should also use the logger.♻️ Use logger for consistency
value = os.getenv("PLAYWRIGHT_BROWSER", DEFAULT_PLAYWRIGHT_BROWSER).strip().lower() if value not in VALID_PLAYWRIGHT_BROWSERS: - print(f" - Warning: Invalid PLAYWRIGHT_BROWSER='{value}', " - f"valid options: {', '.join(sorted(VALID_PLAYWRIGHT_BROWSERS))}. " - f"Defaulting to {DEFAULT_PLAYWRIGHT_BROWSER}") + logger.warning( + f"Invalid PLAYWRIGHT_BROWSER='{value}', " + f"valid options: {', '.join(sorted(VALID_PLAYWRIGHT_BROWSERS))}. " + f"Defaulting to {DEFAULT_PLAYWRIGHT_BROWSER}" + ) return DEFAULT_PLAYWRIGHT_BROWSERserver/services/expand_chat_session.py (1)
218-229: Remove the stray MCP config dict literal.
That block is a no-op and can be safely deleted to avoid confusion.♻️ Suggested cleanup
- # Build MCP servers config for feature management - { - "features": { - "command": sys.executable, - "args": ["-m", "mcp_server.feature_mcp"], - "env": { - "PROJECT_DIR": str(self.project_dir.resolve()), - "PYTHONPATH": str(ROOT_DIR.resolve()), - }, - }, - }ui/src/lib/api.ts (1)
89-105: TightenopenProjectInIDEtyping toIDEType.Line 100 currently accepts
string; reusing the sharedIDETypekeeps this API consistent with the rest of the UI and prevents invalid values.♻️ Suggested update
-import type { +import type { ProjectSummary, ProjectDetail, ProjectPrompts, FeatureListResponse, Feature, @@ ScheduleUpdate, ScheduleListResponse, NextRunResponse, + IDEType, } from './types' @@ -export async function openProjectInIDE(name: string, ide: string): Promise<{ status: string; message: string }> { +export async function openProjectInIDE( + name: string, + ide: IDEType +): Promise<{ status: string; message: string }> { return fetchJSON(`/projects/${encodeURIComponent(name)}/open-in-ide?ide=${encodeURIComponent(ide)}`, { method: 'POST', }) }api/feature_repository.py (1)
37-72: Rollback session on non-retriable commit failures.Line 58: if commit fails with a non-lock OperationalError (or the final lock retry), the session remains in a failed state. A rollback before re-raising keeps the session reusable.
♻️ Suggested update
except OperationalError as e: error_msg = str(e).lower() # Retry on lock/busy errors if "locked" in error_msg or "busy" in error_msg: last_error = e if attempt < max_retries: logger.warning( f"Database commit failed (attempt {attempt + 1}/{max_retries + 1}), " f"retrying in {delay_ms}ms: {e}" ) time.sleep(delay_ms / 1000) delay_ms *= 2 # Exponential backoff session.rollback() # Reset session state before retry continue - raise + session.rollback() + raiseapi/models.py (1)
21-25: Prefersqlalchemy.orm.declarative_basefor SQLAlchemy 2.x.Line 21 uses
sqlalchemy.ext.declarative, which is deprecated in v2. Consider switching to the ORM import to avoid warnings.♻️ Suggested update
-from sqlalchemy.ext.declarative import declarative_base -from sqlalchemy.orm import relationship +from sqlalchemy.orm import declarative_base, relationshipmcp_server/feature_mcp.py (4)
47-51: Consider reorganizing: helper function defined between imports.The
_utc_now()helper is defined in the middle of the import section (betweendatetimeimports andFastMCPimport). This violates PEP 8 style which expects all imports at the top, followed by code.Note: While
api/models.pyhas the same helper, importing it might risk circular dependencies. If reuse is desired, consider moving this to a shared utility module.Suggested reorganization
Move the helper after all imports (around line 65):
from api.dependency_resolver import ( MAX_DEPENDENCIES_PER_FEATURE, compute_scheduling_scores, would_create_circular_dependency, ) from api.migration import migrate_json_to_sqlite + + +def _utc_now() -> datetime: + """Return current UTC time.""" + return datetime.now(timezone.utc) + # Configuration from environment PROJECT_DIR = Path(os.environ.get("PROJECT_DIR", ".")).resolve()And remove lines 47-51.
818-822: Minor: Inconsistent JSON formatting.This endpoint uses
json.dumps(..., indent=2)while all other endpoints in the file usejson.dumps(...)without indentation. For consistency, consider removingindent=2.return json.dumps({ "success": True, "message": f"Updated feature: {feature.name}", "feature": feature.to_dict() - }, indent=2) + })
1335-1348: Optional: Statistics queries could be combined.Four separate queries are used (feature lookup + 3 counts). This could be optimized into a single aggregate query similar to
feature_get_stats, though the current approach is acceptable for a diagnostic endpoint.Optimized single-query approach
from sqlalchemy import case, func # Single aggregate query for all stats stats = session.query( func.count(FeatureAttempt.id).label('total'), func.sum(case((FeatureAttempt.outcome == "success", 1), else_=0)).label('success'), func.sum(case((FeatureAttempt.outcome == "failure", 1), else_=0)).label('failure') ).filter(FeatureAttempt.feature_id == feature_id).first() total_attempts = stats.total or 0 success_count = int(stats.success or 0) failure_count = int(stats.failure or 0)
1487-1493: Move import to file level.
from sqlalchemy import funcis imported inline here, butfuncis already imported and used infeature_get_stats(line 154). This inline import is redundant and inconsistent.- # Count by type - from sqlalchemy import func type_counts = dict(Ensure
funcis imported at the top of the file with other SQLAlchemy imports (around line 154 where it's already used).
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@server/services/expand_chat_session.py`:
- Around line 202-203: The file write for mcp_config uses open(mcp_config_file,
"w") without specifying encoding, which can cause platform-dependent defaults;
update the open call in expand_chat_session.py to include encoding="utf-8" (so
the block that opens mcp_config_file and calls json.dump(mcp_config, f,
indent=2) uses open(..., "w", encoding="utf-8")), matching the earlier settings
file handling and ensuring consistent UTF-8 output.
- Around line 397-399: The list comprehension that extends
self.created_feature_ids from created_features can raise a KeyError if any
feature dict lacks an "id"; update the logic around created_features so you
extract ids defensively (e.g., use f.get("id") and filter out falsy/None values
or validate presence first), then increment self.features_created by the number
of successfully extracted ids (not len(created_features)) before extending
self.created_feature_ids with only those valid ids (referencing
created_features, self.features_created, and self.created_feature_ids).
- Around line 444-446: In the XML parsing fallback branch, avoid KeyError when a
created feature lacks an "id": instead of len(created) and [f["id"] for f in
created], collect ids defensively (e.g., ids = [f.get("id") for f in created if
f.get("id") is not None]) then do self.features_created += len(ids) and
self.created_feature_ids.extend(ids); apply this same defensive extraction
wherever the code currently assumes f["id"] exists in the XML parsing path.
- Around line 221-231: Remove the orphan dict literal that builds the MCP
servers config (the standalone {"features": {"command": sys.executable, "args":
["-m", "mcp_server.feature_mcp"], "env": {"PROJECT_DIR":
str(self.project_dir.resolve()), "PYTHONPATH": str(ROOT_DIR.resolve())}, }, } )
since it is never assigned or used and duplicates the MCP config created earlier
(see the MCP config block around lines 188-199); simply delete this redundant
literal from expand_chat_session.py so only the intended MCP config remains.
- Around line 381-410: The detection currently looks for block_type ==
"ToolResult" and reads block.tool_name, but the real blocks are ToolUseBlock
(with tool id/name) and ToolResultBlock (with tool_use_id), so update the logic
in the function handling blocks to record mappings from ToolUseBlock.tool_use_id
to its tool name (e.g., track a dict mapping tool_use_id -> tool_name when
encountering ToolUseBlock), then detect results by checking block_type ==
"ToolResultBlock" and matching block.tool_use_id against that mapping to find
"feature_create_bulk"; update references to use tool_use_id rather than
tool_name, set mcp_tool_succeeded when the matched tool is feature_create_bulk,
and keep the existing feature parsing code (self.features_created,
self.created_feature_ids, yield events) under this corrected detection path.
In `@start_ui.py`:
- Around line 139-158: The install_npm_deps function should fail fast if
package.json is missing and treat an existing but empty node_modules as stale;
update install_npm_deps to first check UI_DIR / "package.json" and raise or log
an explicit error (return False) when it's absent, and when
node_modules.exists() is true, also consider it stale if it's empty (no
files/directories inside) by checking for any entries in that directory and
setting needs_install = True if empty, before proceeding with the timestamp
comparisons for package.json and package-lock.json.
🧹 Nitpick comments (2)
server/services/expand_chat_session.py (2)
479-488: Runtimesys.pathmanipulation is a code smell.While this may be necessary for the current project structure, modifying
sys.pathat runtime can lead to import conflicts and makes the code harder to maintain. Consider restructuring imports or using relative imports if possible.A cleaner long-term approach would be to ensure proper package installation or use relative imports consistently, but this may require broader refactoring of the project structure.
487-532: Database session management uses manual try/finally instead of context manager.The session handling is correct, but using SQLAlchemy's context manager pattern would be more idiomatic and reduce boilerplate.
♻️ Optional refactor using context manager
- session = SessionLocal() - - try: + with SessionLocal() as session: # Determine starting priority max_priority_feature = session.query(Feature).order_by(Feature.priority.desc()).first() current_priority = (max_priority_feature.priority + 1) if max_priority_feature else 1 # ... rest of the logic ... session.commit() return created_features - - except Exception: - session.rollback() - raise - finally: - session.close()Note: This requires the
SessionLocalto be configured withexpire_on_commit=Falseif you need to access attributes after commit.
- agent.py: Move psutil import inside try block for defensive error handling
- api/dependency_resolver.py: Use depth-based limit instead of visited set size
- api/dependency_resolver.py: Fix path truncation when re-queuing in cycle detection
- api/migrations.py: Preserve optional columns when rebuilding features table
- api/migrations.py: Add migration for regression_count column
- api/models.py: Add regression_count column to Feature model
- server/services/expand_chat_session.py: Add encoding='utf-8' to MCP config write
- server/services/expand_chat_session.py: Remove dead code (orphan dict literal)
- server/services/expand_chat_session.py: Use defensive .get('id') for feature IDs
- server/services/expand_chat_session.py: Fix ToolResultBlock type detection
- start_ui.py: Guard against missing package.json and empty node_modules
- tests/conftest.py: Dispose cached engines in fixtures to prevent Windows file locks
- tests/test_async_examples.py: Use consistent tool payload shape in security hook test
- tests/test_security.py: Use actual null byte instead of escaped string literal
- ui/src/App.tsx: Add error handling for IDE open/save failures
- ui/src/hooks/useAssistantChat.ts: Close stuck WebSocket on timeout for retries
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp_server/feature_mcp.py (1)
8-33: Missing required MCP tools (feature_get_next,feature_claim_next).The guidelines for this file require these tools, but they’re neither listed here nor implemented below. Please add them or align the MCP contract to avoid breaking clients that expect them. As per coding guidelines, these tools must be present.
🤖 Fix all issues with AI agents
In `@mcp_server/feature_mcp.py`:
- Around line 578-614: The feature_release_testing handler currently clears
in_progress but does not persist the test outcome; update
feature_release_testing to record the regression outcome on the Feature object
before session.commit(): if tested_ok is False, set the Feature to a failing
state (e.g., set a boolean field such as Feature.passed or Feature.passing to
False and set Feature.last_failed_at = datetime.utcnow()); if tested_ok is True,
clear/reset the failure markers (e.g., set Feature.passed = True and
Feature.last_failed_at = None). Ensure you import datetime.utcnow, modify the
Feature instance (accessed as feature), then commit the session and keep the
existing rollback/close logic.
- Around line 330-383: feature_get_for_regression currently uses
with_for_update() which is a no-op on SQLite and allows races; change it to use
the existing application-level serializer by removing with_for_update() and
performing the selection, increment of Feature.regression_count, commit and
refresh inside the _claim_lock() critical section (i.e., acquire _claim_lock()
before running the query/filter/order_by/limit, update each
feature.regression_count, session.commit(), then session.refresh each feature),
leaving the surrounding try/except/finally and session handling intact and
keeping references to feature_get_for_regression, with_for_update,
Feature.regression_count, and _claim_lock to locate the code.
In `@registry.py`:
- Around line 48-52: VALID_MODELS is currently built only from CLAUDE_MODELS
which causes validation to reject Ollama model IDs; update the registry so
VALID_MODELS includes IDs from both CLAUDE_MODELS and OLLAMA_MODELS (and ensure
AVAILABLE_MODELS likewise reflects the detected backend if relevant). Locate the
symbols AVAILABLE_MODELS and VALID_MODELS in registry.py and change VALID_MODELS
to combine both model lists (e.g., extend or concatenate CLAUDE_MODELS and
OLLAMA_MODELS IDs) so the schema validators in server/schemas.py
(AgentStartRequest, AgentSettingsUpdate, AgentTestingRequest,
TestingAgentConfigUpdate) accept Ollama model IDs when Ollama is configured.
Ensure you preserve existing behavior when Ollama is not present by only using
the OLLAMA_MODELS list if defined/populated.
In `@server/routers/settings.py`:
- Around line 152-171: The get_denied_commands_list endpoint treats items from
get_denied_commands() as objects but get_denied_commands returns list[dict];
update the list comprehension in get_denied_commands_list to access dict keys
(e.g., d["command"], d["reason"], d["timestamp"], d["project_dir"]) when
constructing DeniedCommandItem, or alternatively convert each dict to a proper
object before use; ensure you reference get_denied_commands() and
DeniedCommandItem in the fix so the response_model still matches
DeniedCommandsResponse and count stays correct.
In `@server/schemas.py`:
- Around line 429-435: DeniedCommandItem.timestamp is declared as datetime but
the code that records denied commands uses
datetime.now(timezone.utc).isoformat(), producing an ISO string; to fix, either
change DeniedCommandItem.timestamp to accept str (or datetime | str) or stop
serializing to ISO and store a datetime object instead—specifically update the
schema in DeniedCommandItem or modify the place that creates the timestamp (the
datetime.now(timezone.utc).isoformat() call) to supply a datetime instance so
types match.
🧹 Nitpick comments (5)
parallel_orchestrator.py (2)
728-735: Consider addingstdin=asyncio.subprocess.DEVNULLfor the initializer subprocess.The initializer subprocess is spawned without explicit stdin handling. To prevent potential blocking on stdin reads (especially on Windows), consider adding
stdin=asyncio.subprocess.DEVNULL:♻️ Suggested improvement
proc = await asyncio.create_subprocess_exec( *cmd, + stdin=asyncio.subprocess.DEVNULL, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, cwd=str(AUTOCODER_ROOT), env=subprocess_env, )
1206-1211: Consider avoiding global logger reassignment.Line 1208 uses
global loggerto reassign the module-level logger. While this works for single-orchestrator scenarios, it could cause issues if multiple orchestrators run in the same process (e.g., in tests). Consider making the logger an instance attribute instead.♻️ Suggested approach
# In __init__, add: self._logger: logging.Logger = None # In run_loop, replace global logger assignment with: self._logger = setup_orchestrator_logging(DEBUG_LOG_FILE)Then use
self._loggerthroughout the class instead of the module-levellogger.server/routers/projects.py (3)
497-516: Consider usingshell=Falseexplicitly and validating command availability.The subprocess calls look safe since
cmdis from a fixed allowlist, but a few improvements would strengthen robustness:
- On Windows,
CREATE_NO_WINDOWprevents the command window but the fallback tocmd /ccould be problematic if the IDE command contains special characters.- Consider checking if the command exists before spawning to provide better error messages.
♻️ Suggested improvement for error handling
try: if sys.platform == "win32": # Try to find the command in PATH first cmd_path = shutil.which(cmd) if cmd_path: subprocess.Popen([cmd_path, project_path]) else: - # Fall back to cmd /c which uses shell PATH - subprocess.Popen( - ["cmd", "/c", cmd, project_path], - creationflags=subprocess.CREATE_NO_WINDOW, - ) + raise HTTPException( + status_code=404, + detail=f"IDE command '{cmd}' not found in PATH. Please ensure {ide} is installed." + ) else: # Unix-like systems - subprocess.Popen([cmd, project_path], start_new_session=True) + cmd_path = shutil.which(cmd) + if not cmd_path: + raise HTTPException( + status_code=404, + detail=f"IDE command '{cmd}' not found in PATH. Please ensure {ide} is installed." + ) + subprocess.Popen([cmd_path, project_path], start_new_session=True)
711-735: Schema validation addresses path traversal concern.The
KnowledgeFileUploadschema inserver/schemas.py(lines 69-72) includes a pattern validatorr'^[a-zA-Z0-9_\-\.]+\.md$'on thefilenamefield. This validation occurs at the Pydantic layer before the endpoint code executes, which addresses the path traversal concern raised in the previous review.However, for defense-in-depth, you could add an explicit check matching the pattern used in
get_knowledge_fileanddelete_knowledge_file:♻️ Optional defense-in-depth check
if not project_dir.exists(): raise HTTPException(status_code=404, detail="Project directory not found") + # Defense-in-depth: validate filename even though schema validates it + if not re.match(r'^[a-zA-Z0-9_\-\.]+\.md$', file.filename): + raise HTTPException(status_code=400, detail="Invalid filename") + knowledge_dir = get_knowledge_dir(project_dir)
666-671: Move import outside the loop for better performance.The
from datetime import datetimeimport inside the loop is executed on every iteration. Move it to the top of the function or file.♻️ Suggested fix
+from datetime import datetime + `@router.get`("/{name}/knowledge", response_model=KnowledgeFileList) async def list_knowledge_files(name: str): """List all knowledge files for a project.""" ... files = [] for filepath in knowledge_dir.glob("*.md"): if filepath.is_file(): stat = filepath.stat() - from datetime import datetime files.append(KnowledgeFile(
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/routers/settings.py (1)
103-149: Prevent incompatible stored models when switching API modes.If the environment switches to Ollama (or back to Claude),
all_settings["model"]may still hold a model from the previous mode and will be returned as-is (Line 116, Line 144). That can yield an unsupported model and downstream failures. Consider normalizing the stored model against the current mode before returning it.✅ Suggested fix: resolve model against the current mode
def _get_default_model() -> str: """Get the appropriate default model based on API mode.""" return DEFAULT_OLLAMA_MODEL if _is_ollama_mode() else DEFAULT_MODEL +def _resolve_model(selected: str | None) -> str: + models = OLLAMA_MODELS if _is_ollama_mode() else CLAUDE_MODELS + allowed = {m["id"] for m in models} + if selected in allowed: + return selected + return _get_default_model() `@router.get`("", response_model=SettingsResponse) async def get_settings(): """Get current global settings.""" all_settings = get_all_settings() - default_model = _get_default_model() return SettingsResponse( yolo_mode=_parse_yolo_mode(all_settings.get("yolo_mode")), - model=all_settings.get("model", default_model), + model=_resolve_model(all_settings.get("model")), glm_mode=_is_glm_mode(), ollama_mode=_is_ollama_mode(), testing_agent_ratio=_parse_int(all_settings.get("testing_agent_ratio"), 1), preferred_ide=all_settings.get("preferred_ide"), ) `@router.patch`("", response_model=SettingsResponse) async def update_settings(update: SettingsUpdate): @@ # Return updated settings all_settings = get_all_settings() - default_model = _get_default_model() return SettingsResponse( yolo_mode=_parse_yolo_mode(all_settings.get("yolo_mode")), - model=all_settings.get("model", default_model), + model=_resolve_model(all_settings.get("model")), glm_mode=_is_glm_mode(), ollama_mode=_is_ollama_mode(), testing_agent_ratio=_parse_int(all_settings.get("testing_agent_ratio"), 1), preferred_ide=all_settings.get("preferred_ide"), )mcp_server/feature_mcp.py (1)
8-33: Add required MCP tools (feature_get_next,feature_claim_next).
The tool registry lists many handlers, but the requiredfeature_get_nextandfeature_claim_nextentry points are missing. Please add implementations or compatibility aliases so clients relying on these tools don’t break. As per coding guidelines, please include the required MCP tools.server/websocket.py (1)
656-727: Emitfeature_updatemessages when features complete in the WebSocket handler.The
WSFeatureUpdateMessageschema exists (server/schemas.py:280), and the coding guidelines require/ws/projects/{project_name}to supportfeature_updatemessages, but they are never emitted from this handler. Theon_outputcallback currently sendslog,agent_update, andorchestrator_updatemessages, butfeature_update(which should indicate when a feature passes or fails) is missing. This needs to be emitted, likely from the agent completion logic inAgentTracker.
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 293-312: Update the Web UI Authentication docs to include critical
security warnings: state that BASIC_AUTH_USERNAME and BASIC_AUTH_PASSWORD must
only be used over HTTPS (never plain HTTP) because Basic Auth transmits
credentials in base64; mark the WebSocket query parameter method
(?token=base64(user:pass)) as insecure and advise disabling/avoiding it (explain
risks: browser history, server logs, referer leakage, shoulder surfing); add
guidance to secure the .env file (restrict filesystem permissions, never commit
BASIC_AUTH_* values to version control, prefer secrets manager) and clarify
AUTOCODER_ALLOW_REMOTE is for explicit remote enabling while reminding readers
that for localhost development authentication is not required.
In `@server/main.py`:
- Around line 30-32: The FastAPI app imports Limiter and the handler but never
attaches SlowAPIMiddleware, so configured limits are not enforced; import
SlowAPIMiddleware from slowapi.middleware and attach it to the FastAPI app
instance (referencing SlowAPIMiddleware and your limiter instance), ensure
app.state.limiter (or equivalent) is set to the Limiter created with
get_remote_address and default_limits=["200/minute"], and keep the
RateLimitExceeded handler registered via _rate_limit_exceeded_handler so the
middleware can trigger the handler.
In `@server/utils/auth.py`:
- Around line 82-104: The base64 decoding calls in the WebSocket auth flow can
raise binascii.Error on malformed input; update server/utils/auth.py by
importing the binascii module and include binascii.Error in the exception tuples
for both decoding blocks (the Basic header branch that decodes auth_header and
the query-parameter branch that decodes token) so the try/except around
base64.b64decode(...).decode(...) handles ValueError, UnicodeDecodeError and
binascii.Error while keeping the existing verify_basic_auth logic intact.
In `@server/websocket.py`:
- Around line 88-96: Stale-agent cleanup is only scheduled on the no-update path
so agents can linger when early returns emit agent updates; ensure you schedule
the cleanup before any early return by calling the cleanup scheduler (e.g.,
self._schedule_cleanup()) wherever agent updates or other early returns occur —
update the methods that touch active_agents, AGENT_TTL_SECONDS, and
_last_cleanup (including the code paths that emit agent_update) to invoke the
scheduler just prior to returning so cleanup runs under high activity as well.
🧹 Nitpick comments (2)
ui/src/components/ErrorBoundary.tsx (1)
67-117: Consider aligning fallback UI with neobrutalism design system.The fallback UI uses standard Tailwind classes (e.g.,
bg-gray-100,rounded-lg,shadow-lg) rather than the neobrutalism design tokens and custom animations mentioned in the coding guidelines (--color-neo-*tokens,animate-slide-in, etc.). This may create visual inconsistency with the rest of the application.As per coding guidelines, consider using the project's design system tokens and border styles for consistency.
api/migrations.py (1)
83-133: Preserve optional column defaults/constraints during table rebuild.
Currently optional columns are recreated with type only, dropping NOT NULL/DEFAULT (e.g., regression_count). That can drift schema guarantees after the migration.♻️ Suggested refactor to retain NOT NULL/DEFAULT metadata
- optional_columns = [] - for col_name, col_info in columns.items(): - if col_name not in core_columns: - # Preserve the column with its type - col_type = col_info["type"] - optional_columns.append((col_name, col_type)) + optional_columns = [] + for col_name, col_info in columns.items(): + if col_name not in core_columns: + # Preserve type + constraints/defaults + optional_columns.append((col_name, col_info)) ... - for col_name, col_type in optional_columns: - optional_col_defs += f",\n {col_name} {col_type}" - optional_col_names += f", {col_name}" + for col_name, col_info in optional_columns: + col_type = col_info["type"] + notnull = " NOT NULL" if col_info["notnull"] else "" + default = ( + f" DEFAULT {col_info['dflt_value']}" + if col_info["dflt_value"] is not None + else "" + ) + optional_col_defs += f",\n {col_name} {col_type}{notnull}{default}" + optional_col_names += f", {col_name}"
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@security.py`:
- Around line 49-67: The logger currently outputs raw denied command and reason
in record_denied_command, risking secret leakage; change it to redact or hash
sensitive data before logging by replacing the raw command/reason with either a
deterministic hash (e.g., sha256 of the string) and/or a redacted preview (e.g.,
first/last N chars with mask) and log only that hashed/preview value along with
context; update the logger.info call in record_denied_command to use the
hashed/redacted values instead of command and reason while leaving the
DeniedCommand object stored unchanged for internal review.
In `@server/routers/projects.py`:
- Around line 461-518: In open_project_in_ide, remove the Windows "cmd /c"
fallback that invokes the shell and causes a command-injection risk; instead,
after mapping ide_commands and calling shutil.which(cmd) (referenced in
open_project_in_ide and ide_commands), if shutil.which(cmd) returns a path use
subprocess.Popen([cmd_path, project_path]) as before, but if it returns None
raise an HTTPException (400 or 500) indicating the IDE executable wasn't found
in PATH; do not call subprocess with ["cmd", "/c", ...] or any shell invocation
and keep the Unix branch using subprocess.Popen([cmd, project_path],
start_new_session=True).
In `@server/routers/settings.py`:
- Around line 152-160: The docstring for get_denied_commands_list states it
returns the last 100 commands but the call to get_denied_commands() uses its
default (50); update the endpoint to call get_denied_commands(limit=100) so the
returned results match the description (adjusting the DeniedCommandsResponse
construction as needed), or alternatively change the docstring to reflect the
actual default—prefer passing limit=100 to get_denied_commands() in
get_denied_commands_list.
🧹 Nitpick comments (1)
server/routers/projects.py (1)
372-458: Consider deleting SQLite WAL/SHM sidecar files on reset.If the DBs use WAL mode, leftover
-wal/-shmfiles can rehydrate state or trigger health warnings after reset.♻️ Proposed update
files_to_delete = [ "features.db", + "features.db-wal", + "features.db-shm", "assistant.db", + "assistant.db-wal", + "assistant.db-shm", ".claude_settings.json", ".claude_assistant_settings.json", ]
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/src/hooks/useAssistantChat.ts (1)
309-367: Update connectionStatus on timeout/failed connect.When the start polling times out or sees a closed/error state, you null
wsRefbeforeonclosecan run, soconnectionStatuscan stay"connecting"indefinitely. Consider setting"error"(or"disconnected") in those branches.🐛 Proposed fix
if (retryCount >= maxRetries) { // Connection timeout - close stuck socket so future retries can succeed if (wsRef.current) { wsRef.current.close(); wsRef.current = null; } if (connectTimeoutRef.current) { clearTimeout(connectTimeoutRef.current); connectTimeoutRef.current = null; } + setConnectionStatus("error"); setIsLoading(false); onError?.("Connection timeout: WebSocket failed to open"); return; } @@ } else { // WebSocket is closed or in an error state - close and clear ref so retries can succeed if (wsRef.current) { wsRef.current.close(); wsRef.current = null; } if (connectTimeoutRef.current) { clearTimeout(connectTimeoutRef.current); connectTimeoutRef.current = null; } + setConnectionStatus("error"); setIsLoading(false); onError?.("Failed to establish WebSocket connection"); }mcp_server/feature_mcp.py (1)
8-33: Expose required MCP tools (feature_get_next / feature_claim_next).The coding guideline requires these tools. They’re not listed here and I don’t see implementations in this file. Please add them (or provide explicit aliases/wrappers, e.g., map feature_claim_next → feature_claim_and_get). As per coding guidelines, ensure the MCP server exposes the required tool set.
🤖 Fix all issues with AI agents
In `@mcp_server/feature_mcp.py`:
- Around line 318-324: The code currently only sets feature.last_error when
error_message is present, leaving stale text otherwise; modify the update block
that sets feature.passes, feature.in_progress, and feature.last_failed_at so
that it explicitly clears stale errors when no error_message is provided (e.g.,
set feature.last_error = None or empty string when error_message is falsy),
while retaining the truncation logic for non-empty error_message and the
session.commit() call; update references to feature.last_error and error_message
in the same function to ensure old messages are removed when no new message is
passed.
In `@ui/src/hooks/useAssistantChat.ts`:
- Around line 30-83: The getToolDescription function can receive null/non-object
for its input and currently accesses properties directly; normalize input at the
top of getToolDescription (e.g., const inputObj = input && typeof input ===
"object" ? input : {}) and use inputObj everywhere instead of input so property
access won't throw; also ensure the feature array length check in the
"feature_create_bulk" case uses Array.isArray(inputObj.features) and other
helpers (getStringValue, getFeatureId) read from inputObj to be safe.
🧹 Nitpick comments (2)
api/models.py (2)
21-25: Use SQLAlchemy 2.x declarative base import.
sqlalchemy.ext.declarative.declarative_baseis deprecated in 2.x; prefersqlalchemy.orm.declarative_base(or DeclarativeBase) to avoid warnings.♻️ Proposed fix
-from sqlalchemy.ext.declarative import declarative_base -from sqlalchemy.orm import relationship +from sqlalchemy.orm import declarative_base, relationship
296-299: Guard against invalid weekday values.A quick range check prevents negative shifts or unexpected inputs.
♻️ Proposed tweak
def is_active_on_day(self, weekday: int) -> bool: """Check if schedule is active on given weekday (0=Monday, 6=Sunday).""" + if weekday < 0 or weekday > 6: + raise ValueError("weekday must be 0 (Mon) to 6 (Sun)") day_bit = 1 << weekday return bool(self.days_of_week & day_bit)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/websocket.py`:
- Around line 21-22: The project WebSocket endpoint currently emits "progress",
"agent_status", "log", and "agent_update" but omits "feature_update"; update the
handler in server/websocket.py (the project WebSocket endpoint that sends
progress/agent_status/log/agent_update messages) to also emit "feature_update"
whenever the other message types are sent, using the same send/emit mechanism
and payload shape as the existing messages so it follows the same context
(include project id/request context and the feature payload), and ensure the
event name is exactly "feature_update" to satisfy the required five message
types.
🧹 Nitpick comments (2)
security.py (2)
283-310: Redundantimport reinside function.The
remodule is already imported at module level (line 12). The inner import on line 295 is unnecessary and inconsistent with the rest of the codebase.♻️ Suggested fix
def split_command_segments(command_string: str) -> list[str]: """ Split a compound command into individual command segments. Handles command chaining (&&, ||, ;) but not pipes (those are single commands). Args: command_string: The full shell command Returns: List of individual command segments """ - import re - # Split on && and || while preserving the ability to handle each segmentSame applies to
extract_commands(line 329) andvalidate_chmod_command(line 498).
799-802: Minor: Inconsistent error messages for blocked vs dangerous commands.Both
BLOCKED_COMMANDSandDANGEROUS_COMMANDSproduce the same error message saying the command "is in the blocklist," butDANGEROUS_COMMANDSare conceptually different (they can potentially be approved in Phase 3). Consider distinguishing the messages.♻️ Suggested fix
if base_cmd in BLOCKED_COMMANDS: return False, f"Command '{name}' is in the blocklist and cannot be allowed" if base_cmd in DANGEROUS_COMMANDS: - return False, f"Command '{name}' is in the blocklist and cannot be allowed" + return False, f"Command '{name}' is marked as dangerous and requires explicit approval"
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)
mcp_server/feature_mcp.py (1)
8-33: Add missing MCP tools (feature_get_next,feature_claim_next).These tool names are required by the repo guidelines, but they are not declared or implemented in this file. If they were intentionally removed, please document the replacement path; otherwise, reintroduce them to avoid breaking consumers that depend on those tool names.
As per coding guidelines, “Use MCP servers for feature management with tools: feature_get_stats, feature_get_next, feature_claim_next, feature_get_for_regression, feature_mark_passing, feature_skip, feature_create_bulk, feature_add_dependency, and feature_remove_dependency.”
🤖 Fix all issues with AI agents
In `@parallel_orchestrator.py`:
- Around line 729-739: The asyncio subprocess invocation that creates proc via
asyncio.create_subprocess_exec should include stdin=asyncio.subprocess.DEVNULL
to avoid blocking on stdin and, on Windows, pass
creationflags=subprocess.CREATE_NO_WINDOW (when sys.platform == "win32") to
prevent console popups; update the proc creation call in
parallel_orchestrator.py (the asyncio.create_subprocess_exec call) to add the
stdin and conditional creationflags arguments and ensure subprocess is
imported/available for the CREATE_NO_WINDOW constant.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/services/assistant_chat_session.py (1)
371-395: Fix invalid try/else block in greeting logic (syntax error).The
else:clause at line 386 is attached to atryblock without anyexceptorfinally, which is invalid Python syntax. The outertryblock (lines 376-385) must be followed byexceptorfinally, notelse.🐛 Proposed fix
- try: - greeting = f"Hello! I'm your project assistant for **{self.project_name}**. I can help you understand the codebase, manage features (create, edit, delete, and deprioritize), and answer questions about the project. What would you like to do?" - - # Store the greeting in the database - add_message(self.project_dir, self.conversation_id, "assistant", greeting) - - yield {"type": "text", "content": greeting} - yield {"type": "response_done"} - else: - try: - greeting = f"Hello! I'm your project assistant for **{self.project_name}**. I can help you understand the codebase, manage features (create, edit, delete, and deprioritize), and answer questions about the project. What would you like to do?" - - # Store the greeting in the database - add_message(self.project_dir, self.conversation_id, "assistant", greeting) - - yield {"type": "text", "content": greeting} - yield {"type": "response_done"} - except Exception as e: - logger.exception("Failed to send greeting") - yield {"type": "error", "content": f"Failed to start conversation: {str(e)}"} + try: + greeting = f"Hello! I'm your project assistant for **{self.project_name}**. I can help you understand the codebase, manage features (create, edit, delete, and deprioritize), and answer questions about the project. What would you like to do?" + + # Store the greeting in the database + add_message(self.project_dir, self.conversation_id, "assistant", greeting) + + yield {"type": "text", "content": greeting} + yield {"type": "response_done"} + except Exception as e: + logger.exception("Failed to send greeting") + yield {"type": "error", "content": f"Failed to start conversation: {str(e)}"}
🤖 Fix all issues with AI agents
In @.claude/templates/coding_prompt.template.md:
- Around line 39-41: The fenced code block beginning with the line "# 5. Get
progress statistics" is missing a language tag and triggers markdownlint MD040;
update the opening ``` fence to include a language (e.g., use ```text for this
descriptive block or ```bash if it’s a command) so the block reads as a fenced
code block with a specified language and resolves the lint error.
- Around line 33-41: Remove the duplicate knowledge-file loop block under the
"Then use MCP tools" heading (the for f in knowledge/*.md; do ... cat "$f"; done
snippet) and replace the stray/mismatched fenced-code markers so the MCP tools
section uses a single proper fenced block (```text) containing the "# 5. Get
progress statistics" line and the "Use the feature_get_stats tool" instruction;
ensure there are no leftover stray backticks or duplicated loop lines so the
section renders correctly.
🧹 Nitpick comments (5)
README.md (1)
194-213: Consider documenting how to list registered projects.The registry documentation clearly explains the concept and per-project structure. However, users might benefit from knowing how to view their registered projects and their paths after initial setup.
Consider adding a brief note about:
- CLI command to list registered projects (if available)
- Where in the UI registered projects are displayed
- How to find a project's path if the user forgets it
This would complete the user journey documentation for the registry system.
server/utils/process_utils.py (1)
148-160: Final cleanup block may be ineffective after parent termination.After
proc.wait()completes (Line 144), the parent process is dead andpsutil.Process(proc.pid)will always raiseNoSuchProcess. Theremainingchildren check at Line 152 will never succeed since you cannot enumerate children of a terminated process.The
NoSuchProcessexception is caught, so this is harmless, but the block is effectively dead code.Consider moving this check before parent termination (around Line 135) or removing it entirely, since the
taskkillcall at Line 121 already handles remaining children.♻️ Optional: Remove redundant cleanup block
- # On Windows, use taskkill as a final cleanup to catch any orphans - # that psutil may have missed (e.g., conhost.exe, deeply nested processes) - if IS_WINDOWS: - try: - remaining = psutil.Process(proc.pid).children(recursive=True) - if remaining: - logger.warning( - "Found %d remaining children after psutil cleanup, using taskkill", - len(remaining) - ) - _kill_windows_process_tree_taskkill(proc.pid) - except psutil.NoSuchProcess: - pass # Parent already dead, good -server/schemas.py (1)
23-24: ReuseVALID_IDESin validation to avoid drift.
Right now the validator hardcodes a second list; using the shared constant keeps SettingsResponse/Update aligned.♻️ Suggested refactor
- valid_ides = ['vscode', 'cursor', 'antigravity'] - if v is not None and v not in valid_ides: - raise ValueError(f"Invalid IDE. Must be one of: {valid_ides}") + if v is not None and v not in VALID_IDES: + raise ValueError(f"Invalid IDE. Must be one of: {VALID_IDES}")Also applies to: 467-472
server/services/assistant_chat_session.py (1)
246-260: Remove the duplicated Args block instart()docstring.♻️ Suggested cleanup
Yields message chunks as they stream in. - - Args: - skip_greeting: If True, skip sending the greeting (for resuming conversations)server/services/expand_chat_session.py (1)
361-486: Only mark MCP success after a valid tool-result parse.
mcp_tool_succeededis set before JSON parsing; if parsing fails, the XML fallback is skipped even though we didn’t actually extract results.♻️ Suggested refactor
- if "feature_create_bulk" in tool_name: - mcp_tool_succeeded = True + if "feature_create_bulk" in tool_name: logger.info("Detected successful feature_create_bulk MCP tool call") # Extract created features from tool result tool_content = getattr(block, "content", []) if tool_content: for content_block in tool_content: if hasattr(content_block, "text"): try: result_data = json.loads(content_block.text) + mcp_tool_succeeded = True created_features = result_data.get("created_features", [])
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/services/assistant_database.py (1)
197-197: Potential TypeError when sorting messages with timezone-aware timestamps.
datetime.minis timezone-naive, but thetimestampcolumn uses_utc_now()which returns timezone-aware datetimes. If a message has aNonetimestamp, comparingdatetime.minwith a timezone-aware datetime will raise aTypeError.🐛 Suggested fix
- for m in sorted(conversation.messages, key=lambda x: x.timestamp or datetime.min) + for m in sorted(conversation.messages, key=lambda x: x.timestamp or datetime.min.replace(tzinfo=timezone.utc))
🤖 Fix all issues with AI agents
In @.github/workflows/pr-check.yml:
- Around line 51-52: The "Run security tests" step currently runs "python
test_security.py" which is inconsistent with ci.yml; update this step to invoke
pytest the same way as ci.yml by replacing the command with "python -m pytest
tests/test_security.py tests/test_security_integration.py -v" so the workflow
runs the same unit and integration security tests and uses the test files under
tests/ (locate the step named "Run security tests" and change its run command
accordingly).
In `@agent.py`:
- Around line 135-153: The except block calls logger.error unconditionally which
will raise if logger is None; update the exception handling in the except
Exception as e block (around the logger usage) to guard all logger.* calls with
a truthy check (e.g., if logger:) before calling logger.error(...,
exc_info=True), logger.error("Agent session error", ...), and
logger.warning(...). Keep the existing print and rate-limit logic
(is_rate_limit_error and parse_retry_after) unchanged but ensure any logger
usage in that block is only executed when logger is not None.
- Around line 336-344: The except block calls logger.error unconditionally but
logger may be None; update the except handler in the try/except around
run_agent_session so it checks for a valid logger before calling logger.error
(e.g., if logger: logger.error(...)) and only passes exc_info and structured
fields when logger is present; leave the print() and the error return (status,
response = "error", str(e)) unchanged; locate this change in the block that
surrounds run_agent_session and references the variables client, prompt,
project_dir, and logger.
In `@analyzers/stack_detector.py`:
- Around line 118-120: Replace the print-based warning in the exception handler
with the logging module: in the except Exception as e block that currently does
print(f"Warning: {analyzer.stack_name} analyzer failed: {e}"), obtain a logger
(e.g., logging.getLogger(__name__) or reuse an existing module-level logger) and
call logger.warning or logger.exception with the analyzer.stack_name and the
exception information (include exc_info=True or use logger.exception) so the
failure of analyzer.stack_name is recorded in structured logs instead of stdout.
In `@client.py`:
- Around line 354-360: The current logging call that prints raw compaction
instructions (the logger.info f"Compaction custom instructions:
{custom_instructions}" near the compaction trigger handling) leaks user
content/secrets; change it to avoid logging the instruction text directly—log
only that custom instructions were provided and their length or a masked/hashed
summary (e.g., "custom_instructions provided, length=X" or a short fixed-size
hash) so you still have diagnostic info without exposing raw content.
- Around line 308-315: The current code sets CLAUDE_CODE_MAX_OUTPUT_TOKENS
unconditionally before detecting alternative APIs, which can push
Anthropic/standard APIs past their limits; move the detection logic (reading
sdk_env["ANTHROPIC_BASE_URL"] into base_url, computing is_alternative_api and
is_ollama) so it runs before the block that sets
sdk_env["CLAUDE_CODE_MAX_OUTPUT_TOKENS"] = DEFAULT_MAX_OUTPUT_TOKENS, then only
set the default max tokens when is_alternative_api is true (i.e., for
GLM/Ollama); apply this same reorder/fix to the corresponding sections in
spec_chat_session.py, expand_chat_session.py, and assistant_chat_session.py,
referencing the sdk_env dict and the
CLAUDE_CODE_MAX_OUTPUT_TOKENS/DEFAULT_MAX_OUTPUT_TOKENS symbols and the
base_url/is_alternative_api/is_ollama flags to locate the code to change.
In `@design_tokens.py`:
- Around line 39-76: to_hsl() currently assumes a 6‑digit hex and will break for
short 3‑digit forms; update ColorToken.to_hsl to normalize short hex (e.g.,
"#abc" -> "aabbcc") after stripping the leading '#' and before parsing,
handle/validate lengths (accept 3 or 6 hex digits, else raise a clear
ValueError), and ensure characters are lowercased/validated so generate_shades()
that calls to_hsl() works reliably.
In `@git_workflow.py`:
- Around line 303-364: complete_feature() (and similarly abort_feature())
currently performs commits/discards without ensuring the feature branch is
checked out; if the user switched branches these ops will affect the wrong
branch. Fix by retrieving branch_name via get_feature_branch(feature_id), run
self._run_git("checkout", branch_name) before any calls to
_has_uncommitted_changes(), _run_git("add"/"commit") or reset/checkout in
abort_feature, and ensure you restore the original branch on error using the
existing current_branch via _get_current_branch(); use check=False when
restoring to avoid raising in the error handler and keep existing
auto_merge/main_branch logic unchanged.
In `@quality_gates.py`:
- Around line 82-126: The JS and Python linter detection uses Path.exists() on
POSIX paths so it misses Windows shims; replace the hardcoded checks for
"node_modules/.bin/eslint" and "node_modules/.bin/biome" to use shutil.which()
and, if found, return that resolved path (use the returned path as the
executable in the command list), and likewise replace the venv checks in
_detect_python_linter (venv/bin/ruff and venv/bin/flake8) with shutil.which()
lookups (or check both venv/Scripts and use shutil.which to resolve the exact
executable) so detection works on Windows and Unix while preserving the same
return tuples from _detect_python_linter and the JS linter detection code.
In `@server/routers/design_tokens.py`:
- Around line 206-235: The output_dir passed to generate_token_files can be used
for directory traversal; before calling DesignTokensManager.generate_all,
resolve and validate the output_dir: construct target = (project_dir /
output_dir).resolve() (use pathlib.Path.resolve()) and ensure target is within
project_dir.resolve() (e.g., str(target).startswith(str(project_dir.resolve()))
or use a relative path check); if validation fails raise HTTPException(400) with
a clear message; then call manager.generate_all(target) when valid (and keep
existing behavior when output_dir is None).
- Around line 120-130: get_project_dir currently allows arbitrary filesystem
paths; update it to reject raw paths that fall outside registered projects by
reusing the same blocked-path guard used elsewhere: after resolving
Path(project_name) call path.resolve() and check it against the registered
project roots (via get_project_path) and the application's blocked-path checker
(e.g., is_blocked_path / validate_blocked_path logic used in other router code);
if the resolved path is blocked or not under a registered project root, raise
HTTPException(404) instead of returning the path. Ensure you still prefer
get_project_path(project_name) when available, normalize with resolve() and
apply the blocked-path guard before returning.
In `@server/routers/documentation.py`:
- Around line 203-219: Replace the brittle ".." check with a resolved-path
containment check: in get_doc_content (and mirror the same change in delete_doc)
resolve project_dir via project_dir.resolve() and resolve the target path with
(project_dir / filename).resolve(); then ensure the resolved target path is a
child of the resolved project_dir (e.g., compare prefixes or use
os.path.commonpath) and return 400 if it lies outside; keep the existing file
existence and suffix (.md) validations and preserve the try/except for reading,
but remove the simple string ".." check and use the resolved-path containment
check instead to prevent URL-encoded or symlink bypasses.
- Around line 90-100: The get_project_dir function currently accepts arbitrary
filesystem paths when get_project_path(project_name) is falsy, allowing
directory traversal; modify get_project_dir to resolve the candidate Path (use
Path.resolve()) and verify it lies within the allowed projects root (or other
intended base) before returning it—if the resolved path is outside the permitted
base (use Path.is_relative_to or compare commonpath against the projects base),
raise an HTTPException (403 or 404) instead of returning the path; keep
get_project_path usage intact and apply the same resolution/check to any
returned project_path to ensure both code paths enforce the same boundary.
In `@server/routers/logs.py`:
- Around line 163-170: The call to query.count(...) is passing unsupported
kwargs (tool_name and search) causing a TypeError; update the call (where
query.count is invoked) to only pass the supported parameters: level, agent_id,
feature_id, and since—remove tool_name and search from the argument list so it
matches the count method signature.
In `@server/routers/review.py`:
- Around line 320-339: In delete_report(), the filename validation happens after
building report_path which allows a crafted filename to influence path
construction; move the existing validation (the checks for "..", "/" and "\\")
to the top of the function before constructing report_path (i.e., before calling
get_project_dir and before using filename to build the Path), or alternatively
sanitize the input by replacing filename with Path(filename).name; update the
delete_report() function to validate/sanitize filename first, then construct
report_path and proceed with existence check and unlink.
In `@server/routers/templates.py`:
- Around line 252-254: The current check on request.project_dir only looks for
".." and is insufficient; replace it with a robust validation that rejects
absolute paths and ensures the resolved path stays inside an allowed base
directory (e.g., BASE_PROJECTS_DIR). Normalize and resolve the user-supplied
path (use os.path.abspath / os.path.realpath) and then compare via
os.path.commonpath to confirm the resolved project path is a subpath of
BASE_PROJECTS_DIR; if not, raise the existing HTTPException. Also ensure you
define or import a BASE_PROJECTS_DIR constant and apply this check where
request.project_dir is used to prevent path traversal and absolute path attacks.
In `@server/routers/visual_regression.py`:
- Around line 148-158: The get_project_dir function currently accepts raw
filesystem paths and should reuse the existing blocked-path guard to prevent
arbitrary filesystem access: after resolving project_path via get_project_path
and before returning a Path(project_name) when treating project_name as a raw
path, call the project's blocked-path guard (e.g., is_blocked_path or the
existing validate/block helper) to verify the path is allowed; if the guard
indicates the path is blocked, raise HTTPException(status_code=403,
detail="Access to this path is forbidden"), otherwise proceed to return the
Path; keep get_project_path usage as-is and only apply the guard to the raw-path
branch in get_project_dir.
In `@server/services/assistant_chat_session.py`:
- Around line 249-263: The start(self, skip_greeting: bool = False) method
documents skip_greeting but never uses it, so the initial greeting is always
sent/stored; update start to check skip_greeting before creating/sending/storing
the greeting (i.e., guard the code path that constructs and yields the initial
greeting chunks and any DB storage calls), and apply the same conditional to the
other greeting-send branch noted in the later block (the second greeting logic
around lines 379-399) so that when skip_greeting is True no greeting is emitted
or persisted.
🟡 Minor comments (10)
docker-compose.traefik.yml-4-6 (1)
4-6: Pin Traefik to a specific stable version instead oflatest.Using
latestintroduces risk: builds become non-reproducible, harder to roll back, and supply-chain verification becomes ambiguous. Pin to a tested minor release (e.g.,v3.6, the current stable line).♻️ Suggested pinning
- image: traefik:latest + image: traefik:${TRAEFIK_VERSION:-v3.6}review_agent.py-375-408 (1)
375-408: Line count underestimates function size by one.
end_lineno - linenoexcludes the end line, so a 51-line function reads as 50.🐛 Proposed fix
- lines = node.end_lineno - node.lineno + lines = node.end_lineno - node.lineno + 1design_tokens.py-438-497 (1)
438-497: SCSS output omits several token groups.
Font weights/line-heights, border widths, and animations are missing despite being part of the token system.🐛 Proposed fix
if "font_size" in tokens.typography: for name, value in tokens.typography["font_size"].items(): lines.append(f"$text-{name}: {value};") + if "font_weight" in tokens.typography: + for name, value in tokens.typography["font_weight"].items(): + lines.append(f"$font-weight-{name}: {value};") + if "line_height" in tokens.typography: + for name, value in tokens.typography["line_height"].items(): + lines.append(f"$leading-{name}: {value};") lines.append("") lines.append("// Borders") if "radius" in tokens.borders: for name, value in tokens.borders["radius"].items(): lines.append(f"$radius-{name}: {value};") + if "width" in tokens.borders: + for name, value in tokens.borders["width"].items(): + lines.append(f"$border-{name}: {value};") lines.append("") lines.append("// Shadows") for name, value in tokens.shadows.items(): lines.append(f"$shadow-{name}: {value};") + + lines.append("") + lines.append("// Animations") + if "duration" in tokens.animations: + for name, value in tokens.animations["duration"].items(): + lines.append(f"$duration-{name}: {value};") + if "easing" in tokens.animations: + for name, value in tokens.animations["easing"].items(): + lines.append(f"$ease-{name}: {value};")analyzers/python_analyzer.py-110-111 (1)
110-111:routeslist is declared but never populated.The
routesvariable (Line 110) is initialized but never filled, resulting in an always-empty list in the return value (Line 164).RouteInfois imported but unused. Either populateroutesor remove the dead code ifendpointsis sufficient.♻️ Option: Remove unused routes if endpoints suffices
def analyze(self) -> AnalysisResult: """Analyze the Python project.""" - routes: list[RouteInfo] = [] components: list[ComponentInfo] = [] endpoints: list[EndpointInfo] = [] ... return { "stack_name": self._detected_stack, "confidence": 0.85, - "routes": routes, + "routes": [], # Not used for Python analyzers; endpoints provides API info ... }Also remove unused
RouteInfoimport at Line 17.Also applies to: 164-164
Dockerfile-20-22 (1)
20-22: Add a non-root user for security.The
.dockerignorefile already excludesui/dist/, making the current COPY order efficient—COPY . .avoids copying unnecessary UI source files. However, the container runs as root by default and should use a non-root user:♻️ Suggested improvement
+# Run as non-root user for security +RUN useradd --create-home appuser +USER appuser EXPOSE 8888analyzers/vue_analyzer.py-170-172 (1)
170-172: Path separators may differ on Windows.
str(rel_path.with_suffix(""))will produce backslashes on Windows (e.g.,\users\[id]), resulting in incorrect route paths. Useas_posix()to ensure forward slashes.🔧 Proposed fix
- route_path = "/" + str(rel_path.with_suffix("")) + route_path = "/" + rel_path.with_suffix("").as_posix()analyzers/vue_analyzer.py-206-207 (1)
206-207: Same path separator issue for API routes.Apply
as_posix()here as well to ensure correct route paths on Windows.🔧 Proposed fix
- route_path = "/api/" + str(rel_path.with_suffix("")) + route_path = "/api/" + rel_path.with_suffix("").as_posix()Similarly for line 234:
- route_path = "/api/" + str(rel_path.with_suffix("")) + route_path = "/api/" + rel_path.with_suffix("").as_posix()server/routers/cicd.py-205-219 (1)
205-219: Inconsistent YAML extension handling.
list_workflowsonly globs*.ymlfiles (line 206), butget_workflow_contentaccepts both.ymland.yaml(line 240). This inconsistency means.yamlworkflow files won't appear in listings but can still be retrieved.🔧 Proposed fix
- for file in workflows_dir.glob("*.yml"): + for file in workflows_dir.glob("*.y*ml"):Or more explicitly:
- for file in workflows_dir.glob("*.yml"): + import itertools + yml_files = workflows_dir.glob("*.yml") + yaml_files = workflows_dir.glob("*.yaml") + for file in itertools.chain(yml_files, yaml_files):server/routers/security.py-126-135 (1)
126-135:report_savedis hardcoded toTruewithout verification.The response always returns
report_saved=True(line 134), but there's no verification that the scan actually saved a report. Ifscan_projectfails to save or has an option to skip saving, this would be misleading.💡 Consider deriving from actual scan result
return ScanResponse( project_dir=result.project_dir, scan_time=result.scan_time, vulnerabilities=[ VulnerabilityInfo(**v.to_dict()) for v in result.vulnerabilities ], summary=ScanSummary(**result.summary), scans_run=result.scans_run, - report_saved=True, + report_saved=getattr(result, 'report_saved', True), )integrations/ci/github_actions.py-491-516 (1)
491-516: Deploy jobs will fail for Python-only projects due to missing artifact.The
deploy-staginganddeploy-productionjobs unconditionally attempt to download a "build" artifact (lines 495 and 522), but this artifact is only uploaded when Node.js is detected (line 457). For Python-only projects, no artifact is created despitepython -m buildgenerating output in thedist/directory, causing the download step to fail.Consider either:
- Making the artifact download step conditional (e.g.,
if: success() && hashFiles('package.json') != '')- Uploading an artifact for Python builds (name: "python-build", path: "dist/")
- Skipping deploy jobs if no artifact was uploaded (add
continue-on-error: truewith appropriate error handling)
🧹 Nitpick comments (36)
.gitignore (1)
9-10: Remove duplicate.code/entries.The
.code/directory is ignored twice—once at lines 9-10 under "Repository-specific" and again at lines 22-23 under "Local Codex/Claude configuration". This duplication creates maintenance confusion and violates the DRY principle.♻️ Consolidate to a single entry
Choose one location and remove the other. For example, keep the first entry and remove the duplicate:
# Repository-specific .code/ # Browser profiles for parallel agent execution .browser-profiles/ # Log files logs/ *.log npm-debug.log* yarn-debug.log* yarn-error.log* -# Local Codex/Claude configuration (do not commit) -.code/ - # =================== # Node.js # ===================Alternatively, if both comments describe different use cases for
.code/, merge them into a single, clearer comment:-# Repository-specific -.code/ - # Browser profiles for parallel agent execution .browser-profiles/ # Log files logs/ *.log npm-debug.log* yarn-debug.log* yarn-error.log* -# Local Codex/Claude configuration (do not commit) +# Repository-specific / Local Codex/Claude configuration (do not commit) .code/ # ===================Also applies to: 22-23
server/services/assistant_database.py (2)
24-28: Thread-safety additions look good.The module-level lock and cache setup are well-documented. Consider using a more specific type hint for the engine cache.
💡 Optional: Use specific type hint
+from sqlalchemy.engine import Engine + # Engine cache to avoid creating new engines for each request # Key: project directory path (as posix string), Value: SQLAlchemy engine -_engine_cache: dict[str, object] = {} +_engine_cache: dict[str, Engine] = {}
103-107: Consider caching the sessionmaker per engine.Creating a new
sessionmakeron every call toget_session()is inefficient. Whilesessionmakeris lightweight, best practice is to create it once per engine and reuse it.♻️ Suggested improvement
+# Session factory cache: Key: project directory path (as posix string) +_session_factory_cache: dict[str, sessionmaker] = {} def get_session(project_dir: Path): """Get a new database session for a project.""" engine = get_engine(project_dir) - Session = sessionmaker(bind=engine) - return Session() + cache_key = project_dir.as_posix() + if cache_key not in _session_factory_cache: + _session_factory_cache[cache_key] = sessionmaker(bind=engine) + return _session_factory_cache[cache_key]().github/workflows/pr-check.yml (1)
38-46: Good addition: pip caching for faster CI.The
pr-check.ymlincludes pip caching (cache: "pip",cache-dependency-path: requirements.txt) which is missing fromci.yml. Consider adding this toci.ymlas well for consistency and faster builds on push.requirements.txt (1)
16-16: Consider adding an upper bound toopenai.The
openai>=1.52.0dependency lacks an upper bound. OpenAI's SDK has had breaking changes between major versions. Consider constraining to<2.0.0to prevent unexpected breakage.♻️ Suggested fix
-openai>=1.52.0 +openai>=1.52.0,<2.0.0progress.py (1)
256-270: Non-atomic count and update inclear_stuck_features.The SELECT COUNT followed by UPDATE is not atomic—in theory, another process could modify
in_progressbetween these statements. However, since this function is designed for startup recovery (before parallel workers start), this is unlikely to cause issues in practice.If future usage changes to runtime recovery, consider combining into a single UPDATE and checking
cursor.rowcount:♻️ Optional atomic alternative
- # Count how many will be cleared - cursor.execute("SELECT COUNT(*) FROM features WHERE in_progress = 1") - count = cursor.fetchone()[0] - - if count > 0: - # Clear all in_progress flags - cursor.execute("UPDATE features SET in_progress = 0 WHERE in_progress = 1") - conn.commit() - print(f"[Auto-recovery] Cleared {count} stuck feature(s) from previous session") - - return count + # Clear all in_progress flags atomically + cursor.execute("UPDATE features SET in_progress = 0 WHERE in_progress = 1") + conn.commit() + count = cursor.rowcount + if count > 0: + print(f"[Auto-recovery] Cleared {count} stuck feature(s) from previous session") + return countFORK_CHANGELOG.md (1)
60-70: Align tables/code fences with markdownlint rules.markdownlint flags missing blank lines around tables and missing language identifiers on fenced blocks. Fixing the first instance and applying consistently will clear the warnings.
✍️ Example fix
-### Files Modified -| File | Changes | -|------|---------| -| `.claude/templates/initializer_prompt.template.md` | Infrastructure category, features 0-4, extended prohibited patterns | -| `.claude/templates/coding_prompt.template.md` | Extended grep, STEP 5.7 server restart test, checklist updates | -| `.claude/commands/create-spec.md` | Database question, infrastructure in feature count | +### Files Modified + +| File | Changes | +|------|---------| +| `.claude/templates/initializer_prompt.template.md` | Infrastructure category, features 0-4, extended prohibited patterns | +| `.claude/templates/coding_prompt.template.md` | Extended grep, STEP 5.7 server restart test, checklist updates | +| `.claude/commands/create-spec.md` | Database question, infrastructure in feature count | +-``` +```text Infrastructure (0-4): NO dependencies - run first ├── Foundation (5-9): depend on [0,1,2,3,4] │ ├── Auth (10+): depend on [0,1,2,3,4] + foundation │ │ ├── Core Features: depend on auth + infrastructure -``` +```server/gemini_client.py (1)
28-34: Consider reusing the client instance for better connection pooling.
_build_client()is called on everystream_chat()invocation, creating a newAsyncOpenAIclient each time. This prevents connection reuse and may add overhead for frequent calls.♻️ Suggested improvement: cache client instance
+_client: AsyncOpenAI | None = None + + def _build_client() -> AsyncOpenAI: + global _client + if _client is not None: + return _client + api_key = os.getenv("GEMINI_API_KEY") if not api_key: raise RuntimeError("GEMINI_API_KEY is not set") base_url = os.getenv("GEMINI_BASE_URL", DEFAULT_GEMINI_BASE_URL) - return AsyncOpenAI(api_key=api_key, base_url=base_url) + _client = AsyncOpenAI(api_key=api_key, base_url=base_url) + return _clientserver/routers/review.py (3)
151-155: Path traversal check could have false positives.The check
".." in file_pathwould reject legitimate filenames containing..(e.g.,foo..bar.py). Consider usingPathresolution and checking if the resolved path is within the project directory.♻️ Suggested improvement: use path resolution for safer validation
if request.files: for file_path in request.files: - if ".." in file_path or file_path.startswith("/") or file_path.startswith("\\"): - raise HTTPException(status_code=400, detail=f"Invalid file path: {file_path}") + # Resolve and verify the path stays within project_dir + resolved = (project_dir / file_path).resolve() + try: + resolved.relative_to(project_dir.resolve()) + except ValueError: + raise HTTPException(status_code=400, detail=f"Invalid file path: {file_path}")
274-317: Session management should use context manager for reliability.The manual
try/finallypattern for session cleanup is error-prone. If an exception occurs aftersession.add()but beforesession.commit(), the session state may be inconsistent. Consider using a context manager orwithstatement.♻️ Suggested improvement: use context manager for session
- session = None - try: - session = get_session(db_path) - - # Get max priority for ordering - max_priority = session.query(Feature.priority).order_by(Feature.priority.desc()).first() - current_priority = (max_priority[0] if max_priority else 0) + 1 - - for issue in request.issues: - # ... feature creation ... - - session.commit() - - return CreateFeaturesResponse(...) - + with get_session(db_path) as session: + # Get max priority for ordering + max_priority = session.query(Feature.priority).order_by(Feature.priority.desc()).first() + current_priority = (max_priority[0] if max_priority else 0) + 1 + + for issue in request.issues: + # ... feature creation ... + + session.commit() + + return CreateFeaturesResponse(...) + except Exception as e: logger.error(f"Failed to create features: {e}") raise HTTPException(status_code=500, detail=str(e)) - finally: - if session: - session.close()Note: This assumes
get_sessionsupports context manager protocol. If not, consider wrapping it.
209-228: Specify encoding when opening files.
open(report_file)on line 211 uses platform-default encoding. For JSON files that may contain unicode, explicitly specifyencoding="utf-8".♻️ Suggested fix
- with open(report_file) as f: + with open(report_file, encoding="utf-8") as f: data = json.load(f)The same applies to line 249 in
get_report().server/routers/git_workflow.py (1)
27-31: Duplicated helper function across routers.The
_get_project_pathfunction is identical across at least 5 router files (git_workflow.py,import_project.py,logs.py,cicd.py,security.py). Consider extracting this to a shared utility module to reduce duplication.♻️ Suggested improvement: create shared utility
Create a new file
server/routers/utils.py:from pathlib import Path from registry import get_project_path def get_project_path_or_none(project_name: str) -> Path | None: """Get project path from registry.""" return get_project_path(project_name)Then import and use in all routers:
from server.routers.utils import get_project_path_or_noneserver/schemas.py (1)
468-474: Use the module-level VALID_IDES constant for consistency.The validator redefines
valid_ideslocally instead of using theVALID_IDESconstant defined at line 24. This creates a maintenance risk if the lists diverge.♻️ Suggested fix
`@field_validator`('preferred_ide') `@classmethod` def validate_preferred_ide(cls, v: str | None) -> str | None: - valid_ides = ['vscode', 'cursor', 'antigravity'] - if v is not None and v not in valid_ides: - raise ValueError(f"Invalid IDE. Must be one of: {valid_ides}") + if v is not None and v not in VALID_IDES: + raise ValueError(f"Invalid IDE. Must be one of: {VALID_IDES}") return vreview_agent.py (1)
166-214: Avoid reading non-source files before filtering.
Whenfilesorcommitsinclude assets/binaries,read_text()is unnecessary work. Filter by suffix first.♻️ Suggested refactor
for file_path in files_to_review: if not file_path.exists(): continue + if file_path.suffix not in {".py", ".js", ".ts", ".jsx", ".tsx"}: + continue + try: content = file_path.read_text(errors="ignore") if file_path.suffix == ".py":integrations/ci/__init__.py (2)
24-29: Addgenerate_ci_configto__all__.The
generate_ci_configfunction defined in this module (Line 32) is missing from__all__, creating an inconsistency. The parent package (integrations/__init__.py) re-exports it, but this module's__all__should also include it for completeness.♻️ Proposed fix
__all__ = [ "generate_github_workflow", "generate_all_workflows", "GitHubWorkflow", "WorkflowTrigger", + "generate_ci_config", ]
43-53: Consider documenting the file-writing side effect.
generate_all_workflows(project_dir)writes workflow files to disk by default (save=True). Callers expecting a "dry-run" config generation may be surprised. Consider adding asaveparameter togenerate_ci_configor documenting this behavior in the docstring.♻️ Optional: Add save parameter for flexibility
-def generate_ci_config(project_dir, provider: str = "github") -> dict: +def generate_ci_config(project_dir, provider: str = "github", save: bool = True) -> dict: """ Generate CI configuration based on detected tech stack. Args: project_dir: Project directory provider: CI provider ("github" or "gitlab") + save: Whether to save workflow files to disk (default True) Returns: Dict with generated configuration and file paths """ from pathlib import Path project_dir = Path(project_dir) if provider == "github": - workflows = generate_all_workflows(project_dir) + workflows = generate_all_workflows(project_dir, save=save) return {analyzers/python_analyzer.py (3)
44-64: Use consistent file reading with explicit encoding.Lines 46 and 70 use
read_text()without specifying encoding, while later code (Lines 93, 101) uses_read_file_safe()which handles encoding properly. This inconsistency could causeUnicodeDecodeErroron some systems.♻️ Proposed fix for requirements.txt reading
if requirements.exists(): try: - content = requirements.read_text().lower() + content = requirements.read_text(encoding="utf-8").lower() if "fastapi" in content:Apply similar fix at Line 70 for
pyproject.read_text().
256-263: Redundant path check.Lines 258-259 check
if path == "/" then path = "/"which is a no-op. The path is already normalized by Line 257.♻️ Remove redundant check
for match in path_pattern.finditer(content): path = "/" + match.group(1).rstrip("/") - if path == "/": - path = "/" + if path == "": + path = "/" # Handle empty path for root route # Django uses <type:name> for params, convert to :nameActually, after
"/" + match.group(1).rstrip("/"), the result cannot be empty. If you want to handle the case wherematch.group(1)is"", the result would be"/"already. The entire conditional can simply be removed.
365-380: Potential duplicate components from overlapping patterns.A file like
api/views.pycould match both**/views.pyand**/api/**/*.py, resulting in duplicate entries in the components list. Consider deduplicating by file path.♻️ Optional: Deduplicate by file path
+ seen_files: set[str] = set() + for file in view_files: if file.name != "__init__.py": + rel_path = str(file.relative_to(self.project_dir)) + if rel_path in seen_files: + continue + seen_files.add(rel_path) components.append({ "name": file.stem, - "file": str(file.relative_to(self.project_dir)), + "file": rel_path, "type": "view", })analyzers/vue_analyzer.py (2)
40-41: Consider specifying encoding for file reads.The
read_text()call uses the system default encoding. For consistency with_read_file_safein the base class (which usesencoding="utf-8"), consider explicitly specifying the encoding here too.♻️ Suggested improvement
- data = json.loads(package_json.read_text()) + data = json.loads(package_json.read_text(encoding="utf-8"))
131-133: Hardcoded confidence may not reflect actual detection confidence.The
analyze()method always returnsconfidence: 0.85, butcan_analyze()returns varying confidence levels (0.7 to 0.95) based on detection signals. Consider storing the detection confidence and reusing it here for consistency.♻️ Suggested improvement
Store detection confidence in
can_analyze():def can_analyze(self) -> tuple[bool, float]: ... if "nuxt" in deps or "nuxt3" in deps: self._detected_stack = "nuxt" - confidence = 0.95 - return True, confidence + self._confidence = 0.95 + return True, self._confidenceThen use it in
analyze():return { "stack_name": self._detected_stack, - "confidence": 0.85, + "confidence": getattr(self, '_confidence', 0.85),server/routers/documentation.py (1)
171-172: Use ISO format for the modified timestamp.
stat.st_mtime.__str__()produces a raw float string (e.g.,"1706450000.123456"), which is not human-readable. Consider formatting it.♻️ Suggested improvement
+from datetime import datetime ... - modified=stat.st_mtime.__str__(), + modified=datetime.fromtimestamp(stat.st_mtime).isoformat(),analyzers/node_analyzer.py (1)
13-19:RouteInfois imported but not used.The
routeslist inanalyze()is always returned empty (line 161). Either populate routes or remove the unused import.server/main.py (1)
151-164: Duplicate health endpoints exist at different paths.There are two health-check endpoints:
/health(line 151) →{"status": "ok"}/api/health(line 275) →{"status": "healthy"}This may be intentional (root-level for infrastructure probes vs API-prefixed for consistency), but consider consolidating or documenting the purpose of each.
Also applies to: 275-278
server/routers/logs.py (1)
303-322: Good path traversal protection, but consider additional hardening.The filename validation (line 311) checks for
..,/, and\, which covers common traversal attacks. However, for defense in depth, consider also verifying that the resolved path is still within the expected exports directory.🔒 Optional: Add resolved path verification
# Security: validate filename to prevent path traversal if ".." in filename or "/" in filename or "\\" in filename: raise HTTPException(status_code=400, detail="Invalid filename") export_path = project_dir / ".autocoder" / "exports" / filename + # Additional check: ensure resolved path is within exports directory + exports_dir = (project_dir / ".autocoder" / "exports").resolve() + if not export_path.resolve().is_relative_to(exports_dir): + raise HTTPException(status_code=400, detail="Invalid filename") + if not export_path.exists(): raise HTTPException(status_code=404, detail="Export file not found")server/routers/cicd.py (1)
114-116: Silent skip of invalid workflow types may confuse users.When an invalid workflow type is provided in
workflow_types, it's silently skipped. Users won't know their input was ignored. Consider either validating upfront or including skipped types in the response.💡 Option: Validate workflow types upfront
+ VALID_WORKFLOW_TYPES = {"ci", "security", "deploy"} + try: if request.provider == "github": from integrations.ci import generate_github_workflow + invalid_types = set(request.workflow_types) - VALID_WORKFLOW_TYPES + if invalid_types: + raise HTTPException( + status_code=400, + detail=f"Invalid workflow types: {', '.join(invalid_types)}", + ) + workflows = [] for wf_type in request.workflow_types: - if wf_type not in ["ci", "security", "deploy"]: - continue + # Now guaranteed to be validserver/routers/import_project.py (3)
26-31:sys.pathmanipulation is a code smell.Modifying
sys.pathat module level can cause import issues and makes dependencies less explicit. Consider restructuring imports or using proper package installation instead.
40-45: Path validation is minimal and may not cover all attack vectors.The
validate_pathfunction only checks for..and null bytes. This allows absolute paths which could potentially access sensitive system directories. Consider adding more comprehensive validation.💡 Consider additional validations
def validate_path(path: str) -> bool: """Validate path to prevent traversal attacks.""" - # Allow absolute paths but check for common attack patterns - if ".." in path or "\x00" in path: + # Check for common attack patterns + if ".." in path or "\x00" in path or path.startswith("/etc") or path.startswith("/root"): return False + # Optionally: restrict to allowed base directories return True
241-282: Session management looks correct but could use context manager.The session is properly closed in a
finallyblock. However, using a context manager pattern would be cleaner and safer.♻️ Optional: Use context manager
# Initialize database engine, SessionLocal = create_database(project_dir) - session = SessionLocal() - - try: + with SessionLocal() as session: # Get starting priority from sqlalchemy import func max_priority = session.query(func.max(Feature.priority)).scalar() or 0 # ... rest of code ... session.commit() - - finally: - session.close()Note: This requires
SessionLocalto support context manager protocol, which is standard for SQLAlchemy sessions.server/routers/security.py (1)
183-187: Specify encoding when reading JSON files.The
open()calls on lines 184 and 208 don't specify encoding. While Python 3 defaults to the system encoding, explicitly specifying UTF-8 ensures consistent behavior across platforms, especially on Windows.🔧 Add explicit encoding
try: - with open(report_path) as f: + with open(report_path, encoding="utf-8") as f: return json.load(f)Apply similarly to line 208.
Also applies to: 207-211
analyzers/stack_detector.py (1)
95-104: Consider extracting category mapping to a constant.The hardcoded category lists are repeated implicitly across the codebase. Extracting them to constants would improve maintainability.
♻️ Extract to constants
+FRONTEND_STACKS = {"react", "nextjs", "vue", "nuxt", "angular"} +BACKEND_STACKS = {"express", "fastapi", "django", "flask", "nestjs"} +DATABASE_STACKS = {"postgres", "mysql", "mongodb", "sqlite"} + +def _get_category(stack_name: str) -> str: + """Determine category from stack name.""" + name_lower = stack_name.lower() + if name_lower in FRONTEND_STACKS: + return "frontend" + elif name_lower in BACKEND_STACKS: + return "backend" + elif name_lower in DATABASE_STACKS: + return "database" + return "other"server/routers/templates.py (1)
268-270: Specify encoding when writing files.The
open()call doesn't specify encoding. Sinceapp_spec_contentis XML with UTF-8 declaration (per the library snippet), explicitly write with UTF-8 encoding.🔧 Add explicit encoding
app_spec_path = prompts_dir / "app_spec.txt" - with open(app_spec_path, "w") as f: + with open(app_spec_path, "w", encoding="utf-8") as f: f.write(app_spec_content)analyzers/feature_extractor.py (1)
248-251: Consider: Handling "ALL" method loses multi-method endpoint information.When an endpoint has method "ALL" (indicating it handles multiple HTTP methods), converting it to just "GET" loses information. This could result in missing features for POST/PUT/DELETE operations on the same path.
Consider expanding "ALL" endpoints into multiple features for common methods, or keeping it as a generic "API endpoint" feature.
💡 Alternative approach to handle ALL method
# Handle ALL method by creating GET endpoint if method == "ALL": - method = "GET" + # Create features for common methods + for m in ["GET", "POST"]: + feature_key = f"endpoint:{path}:{m}" + if feature_key not in seen_features: + seen_features.add(feature_key) + feature_name = _route_to_feature_name(path, m) + # ... add feature + continue # Skip the rest of the loop iterationanalyzers/react_analyzer.py (1)
173-211: Consider consolidating .tsx and .jsx extraction loops.The extraction logic for
.tsxand.jsxfiles is duplicated. This could be simplified with a single loop over both extensions.♻️ Consolidated extraction
def _extract_app_router_routes(self, app_dir: Path) -> list[RouteInfo]: """Extract routes from Next.js App Router.""" routes: list[RouteInfo] = [] - for page_file in app_dir.rglob("page.tsx"): - rel_path = page_file.relative_to(app_dir) - route_path = "/" + "/".join(rel_path.parent.parts) - - # Handle dynamic routes: [id] -> :id - route_path = re.sub(r"\[([^\]]+)\]", r":\1", route_path) - - # Clean up - if route_path == "/.": - route_path = "/" - route_path = route_path.replace("//", "/") - - routes.append({ - "path": route_path, - "method": "GET", - "handler": "Page", - "file": str(page_file.relative_to(self.project_dir)), - }) - - # Also check .jsx files - for page_file in app_dir.rglob("page.jsx"): + for ext in ["tsx", "jsx"]: + for page_file in app_dir.rglob(f"page.{ext}"): rel_path = page_file.relative_to(app_dir) route_path = "/" + "/".join(rel_path.parent.parts) route_path = re.sub(r"\[([^\]]+)\]", r":\1", route_path) if route_path == "/.": route_path = "/" route_path = route_path.replace("//", "/") routes.append({ "path": route_path, "method": "GET", "handler": "Page", "file": str(page_file.relative_to(self.project_dir)), }) return routesauto_documentation.py (1)
504-514: Consider: Hardcoded assumption about prerequisites order.Lines 508-512 assume the first 2 setup steps are prerequisites and the rest are installation steps. This coupling to
_extract_setup_stepsoutput order is fragile and could produce incorrect documentation if the order changes.💡 Alternative: Separate prerequisites from installation steps
Consider returning a structured dict from
_extract_setup_stepsthat explicitly separates prerequisites from installation:def _extract_setup_steps(self) -> dict: return { "prerequisites": [...], "installation": [...], }integrations/ci/github_actions.py (1)
163-168: Movereimport to module level.The
remodule is imported inside the function (line 164), but it's already used at module level in other files and is a standard library module. For consistency, consider importing it at the top of the file.♻️ Move import to module level
import json +import re from dataclasses import dataclass, field from enum import Enum from pathlib import Path from typing import Any, Literal, Optional import yamlThen remove line 164 (
import re).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.claude/templates/coding_prompt.template.md:
- Around line 493-502: The template references an undefined tool
feature_get_summary in the "TOKEN EFFICIENCY" section; either remove that
mention or add feature_get_summary to the "ALLOWED Feature Tools" list so the
guidance matches available tools. Locate the phrase "feature_get_summary" in the
TOKEN EFFICIENCY block and then update the ALLOWED Feature Tools section (the
list around the existing allowed tool names) to include feature_get_summary with
its correct description and usage, or delete/replace the TOKEN EFFICIENCY line
to reference an existing tool such as feature_get_by_id to keep the template
consistent.
In `@quality_gates.py`:
- Around line 289-293: The call to _run_command currently hardcodes ["bash",
str(script_full_path)] which breaks on Windows where bash may not exist; update
the logic that builds the command for running script_full_path to first detect
available shell (use shutil.which("bash") and shutil.which("sh")), choose "bash"
if present, otherwise "sh", and on Windows (os.name == "nt" or platform.system()
== "Windows") also support running PowerShell (.ps1) or cmd (.bat/.cmd) by
detecting the script extension and invoking the appropriate runner (e.g.,
powershell.exe -File or cmd /c) before calling _run_command; ensure the code
uses the detected executable and arguments instead of the hardcoded ["bash",
...] so cross-platform script execution works.
🧹 Nitpick comments (6)
api/migrations.py (3)
135-138: Consider wrapping table swap in explicit transaction for safety.The three-step table swap (rename old → rename new → drop old) is standard SQLite practice, but if a crash occurs between statements, the database could be left in an inconsistent state. While each DDL is implicitly transactional, the sequence as a whole is not.
SQLite supports
BEGIN IMMEDIATEto ensure the entire swap is atomic. However, since this is a one-time legacy migration and the risk is low, this is acceptable as-is.
273-276: Note: SQLite stores JSON as TEXT.Using
JSONtype in raw SQL works because SQLite accepts it as an alias, but it's stored as TEXT internally. This is consistent with SQLAlchemy's handling but worth noting for database portability.
22-27: SQLite-specific PRAGMA usage limits database portability.All migration functions use
PRAGMA table_infowhich is SQLite-specific. If future requirements include PostgreSQL or MySQL support, consider using SQLAlchemy'sinspect(engine).get_columns()consistently (as done inmigrate_add_schedules_tables).♻️ Example using SQLAlchemy inspect (applies to all PRAGMA usages)
+from sqlalchemy import inspect + def migrate_add_in_progress_column(engine) -> None: """Add in_progress column to existing databases that don't have it.""" with engine.connect() as conn: - # Check if column exists - result = conn.execute(text("PRAGMA table_info(features)")) - columns = [row[1] for row in result.fetchall()] + # Check if column exists (database-agnostic) + inspector = inspect(engine) + columns = [c["name"] for c in inspector.get_columns("features")] if "in_progress" not in columns:.claude/templates/coding_prompt.template.md (3)
279-280: Grep for Map/Set will produce many false positives.
new Map()andnew Set()are legitimate ES6 data structures commonly used for valid purposes (deduplication, lookups, caching, etc.) in production code, not just for anti-pattern in-memory storage. This grep will flag many correct implementations.♻️ Refine the check to reduce noise
Either remove this check entirely or add context to explain that Map/Set hits require manual review to distinguish legitimate use (e.g., short-lived caching, deduplication) from problematic in-memory data stores:
# 5. In-memory collections as data stores -grep -r "new Map\(\)\|new Set\(\)" --include="*.ts" --include="*.tsx" --include="*.js" src/ 2>/dev/null +# NOTE: Map/Set are often legitimate - look for patterns like: +# - Assigned to module-level variables (potential in-memory store) +# - Used to store entities that should persist (users, posts, etc.) +# - Not cleared or scoped to request lifecycle +grep -r "new Map\(\)\|new Set\(\)" --include="*.ts" --include="*.tsx" --include="*.js" src/ 2>/dev/null || trueAlternatively, grep for more specific patterns like module-level Map/Set declarations:
# Look for module-level Map/Set that might be in-memory stores grep -rE "^(const|let|var)\s+\w+\s*=\s*new (Map|Set)" --include="*.ts" --include="*.tsx" --include="*.js" src/
300-318: Windows server kill commands are incomplete.The Windows alternative for stopping the server (lines 309-311) is commented out and requires manual PID extraction, making it harder to use than the Unix version. This reduces portability for Windows developers.
♻️ Provide a more complete Windows example
# Windows alternative (use if lsof not available): - # netstat -ano | findstr :${PORT:-3000} | findstr LISTENING - # taskkill /F /PID <pid_from_above> 2>nul + # FOR /F "tokens=5" %P IN ('netstat -ano ^| findstr :%PORT% ^| findstr LISTENING') DO taskkill /F /PID %P 2>nul + # timeout /t 3 /nobreakOr recommend using a cross-platform tool like
npx kill-port:# Cross-platform alternative (works on Unix and Windows): npx kill-port ${PORT:-3000} sleep 3
427-430: Add usage example for browser_evaluate.The warning about
browser_run_codecrashing is clear, but developers unfamiliar withbrowser_evaluatemay not know how to migrate or use it correctly.♻️ Add a brief browser_evaluate example
### Unavailable Browser Tools - `browser_run_code` - DO NOT USE. This tool causes the Playwright MCP server to crash. Use `browser_evaluate` instead for executing JavaScript in the browser context. + + **Example:** + ```javascript + // Get element text content + browser_evaluate expression="document.querySelector('#status').textContent" + + // Check if element exists + browser_evaluate expression="!!document.querySelector('.error-message')" + ```
- registry.py: Include Ollama models in VALID_MODELS for validation - server/routers/settings.py: Pass limit=100 to get_denied_commands() - server/websocket.py: Add feature_update message emission on completion - parallel_orchestrator.py: Add stdin=DEVNULL and Windows CREATE_NO_WINDOW flags - requirements.txt: Document CVE-2026-24486 python-multipart fix - server/routers/projects.py: Add defense-in-depth filename validation with os.path.basename() - security.py: Simplify regex, add comprehensive type annotations with cast()
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@parallel_orchestrator.py`:
- Around line 1202-1226: The cleanup() method must be made idempotent to avoid
repeated WAL checkpoints and dispose calls: at the start of cleanup(), return
immediately if self._engine is None; otherwise capture the current engine to a
local variable (e.g., engine = self._engine) and set self._engine = None before
performing PRAGMA wal_checkpoint and engine.dispose() so subsequent cleanup()
calls no-op; apply the same idempotency expectation when invoking cleanup() from
cleanup_handler and signal_handler (they should rely on cleanup() being safe to
call multiple times).
- Around line 1253-1278: The current try-except around
orchestrator.get_session() and the Feature queries can leak DB connections
because session.close() is only called on the happy path; modify the block to
guarantee session.close() in a finally clause (or convert to try/except/finally)
so the session is closed whether an exception occurs, guarding the close with a
presence check (e.g., if session is defined and not None) to avoid NameError if
get_session() fails; update the code referencing orchestrator.get_session(), the
session variable, and the Feature model accordingly.
In `@requirements.txt`:
- Line 5: The FastAPI dependency line "fastapi>=0.115.3,<0.116" must be bumped
to a version that pulls in a patched Starlette (≥0.49.1); update that
requirement to "fastapi>=0.128.0,<0.129" (or simply "fastapi>=0.128.0") in
requirements.txt, then regenerate your lock/constraints (pip-compile / poetry
lock / pipenv lock) and run the test suite to ensure compatibility; if any
breaking changes surface, adjust import/use sites accordingly.
In `@server/routers/projects.py`:
- Around line 776-787: The delete_knowledge_file handler contains a
corrupted/duplicated block; remove the truncated/duplicate lines so there is
only one clear flow: check if filepath.exists() and raise HTTPException(404) if
not, then try to filepath.unlink() and return {"success": True, "message":
f"Deleted '{filename}'"} and except Exception as e raise HTTPException(500,
detail=f"Failed to delete file: {e}"); ensure the function delete_knowledge_file
uses a single existence check and one try/except around filepath.unlink()
(referencing variables filepath, filename and/or safe_filename consistently) and
delete the stray truncated string `f"Deleted '{safe_` and duplicate block.
🧹 Nitpick comments (5)
server/routers/settings.py (2)
58-61: Ollama detection relies on hardcoded localhost patterns.The check for Ollama mode only matches
localhost:11434and127.0.0.1:11434. This could miss configurations using other loopback addresses (e.g.,0.0.0.0:11434) or custom hostnames pointing to localhost.♻️ Consider more flexible detection
def _is_ollama_mode() -> bool: """Check if Ollama API is configured via environment variables.""" base_url = os.getenv("ANTHROPIC_BASE_URL", "") - return "localhost:11434" in base_url or "127.0.0.1:11434" in base_url + return ":11434" in base_url # Ollama's default portOr add a dedicated
OLLAMA_MODEenvironment variable for explicit configuration.
174-178: Consider returning the count of cleared commands.The
clear_denied_commands()function returns the count of cleared items, but the endpoint discards it. Returning this count could be useful for clients.♻️ Return cleared count
`@router.delete`("/denied-commands") async def clear_denied_commands_list(): """Clear the denied commands history.""" - clear_denied_commands() - return {"status": "cleared"} + count = clear_denied_commands() + return {"status": "cleared", "count": count}server/routers/projects.py (1)
668-668: Redundant import inside loop.
datetimeis already imported at module level (line 13), but is re-imported inside the loop at line 668.♻️ Remove redundant import
for filepath in knowledge_dir.glob("*.md"): if filepath.is_file(): stat = filepath.stat() - from datetime import datetime files.append(KnowledgeFile( name=filepath.name, size=stat.st_size,security.py (1)
382-393: Add test cases for malformed command input and clarify the fallback behavior.The concern about the shlex failure fallback is partially valid, but the actual security impact is more limited than stated. While it's true that the operator check (line 385) doesn't include semicolons, both commands are still extracted and validated:
For example,
echo "test; rm -rf /"fails the pre-split regex, creating two malformed segments. Both fail shlex, but since neither contains pipe operators (|,||,&&,&), the fallback extracts both "echo" and "rm". Both commands then pass through the allowlist validation at lines 1079–1101. If either is not allowed, the entire command is blocked.The real gap is test coverage: the current test cases (lines 104–111) don't cover malformed input with unclosed quotes or command substitutions. Add tests for:
- Malformed segments with unclosed quotes
- Edge cases with the semicolon pre-split regex
- Verify that unparseable tokens (like
cmd1$(cmd2) correctly fail validationThis ensures the fallback behavior is well-documented and defended against edge cases.
parallel_orchestrator.py (1)
1095-1101: Throttle engine disposal to avoid connection churn.Calling
engine.dispose()on every loop iteration is costly and defeats connection pooling. Instead, dispose only after initialization or on a coarse interval (e.g., every N iterations). For ensuring visibility of subprocess commits in SQLite WAL mode, close/reopen sessions to end stale read transactions rather than disposing the engine; consider expanding thesession.expire_all()pattern from line 1113 to be used more consistently rather than relying on dispose.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@security.py`:
- Around line 88-110: get_denied_commands currently returns full unredacted
command text from _denied_commands which can leak secrets via the GET
/api/settings/denied-commands API; update get_denied_commands to redact
sensitive data before returning (mirror the redaction used in logging) by
transforming cmd.command to a redacted value (call the existing redaction helper
or implement a simple redact_command(cmd.command)) in the returned dict for each
entry (referencing get_denied_commands, _denied_commands, and the cmd fields
like cmd.command, cmd.timestamp, cmd.reason, cmd.project_dir), or alternatively
change DeniedCommand storage to only keep a redacted_command field and return
that instead — pick one approach and apply it consistently so the API never
exposes the raw command text.
In `@server/routers/projects.py`:
- Around line 756-770: After calling get_project_path(name) and before calling
get_knowledge_dir(project_dir), add checks to handle a missing or deleted
project: if project_dir is None or not project_dir.exists(), raise
HTTPException(status_code=404, detail="Project not found") (or similar) so
get_knowledge_dir is never called with None; keep the existing
validate_project_name and filename validation logic unchanged and reference
get_project_path, project_dir, and get_knowledge_dir when locating where to
insert the checks.
In `@server/routers/templates.py`:
- Around line 152-155: The handlers currently include raw exception details in
HTTPException responses (e.g., the except blocks using logger.exception(...)
then raise HTTPException(status_code=500, detail=f"Failed to list templates:
{str(e)}")); update these except blocks to keep the logger.exception(...) call
but replace the HTTPException detail with a generic message (e.g., "Failed to
list templates" or "Internal server error") to avoid leaking internal error
data; apply the same change to the other handlers noted (the other except blocks
around the logger.exception/raise HTTPException pairs at the referenced
locations) so all 500 responses return generic messages while detailed info
remains only in logs.
- Around line 283-285: The app_spec.txt is written without specifying encoding
which can default to a non-UTF-8 platform encoding; update the write operation
in generate_app_spec (where app_spec_path and prompts_dir are used) to
explicitly use UTF-8 (e.g., open the file with encoding="utf-8" or use
Path.write_text(..., encoding="utf-8")) so the XML declaration and any non-ASCII
characters are preserved reliably across platforms.
- Line 133: Add a module-level sys.path setup in server/routers/templates.py
before any "from templates import ..." statements: insert logic to compute the
project root (using Path(__file__).parent.parent.parent), check if it's in
sys.path and insert it at position 0 if missing, so imports like "from templates
import list_templates" (and the other inline imports at the locations that
import templates functions) always succeed; then remove or update the redundant
inline sys.path manipulations and redundant inline imports so all template
imports rely on the single module-level sys.path configuration and reference the
existing symbols such as list_templates, create_template, update_template,
delete_template, and render_template.
🧹 Nitpick comments (3)
server/routers/projects.py (1)
663-672: Move import outside the loop.The
datetimeimport on line 667 is inside the loop body. While Python caches imports, this is inefficient and non-idiomatic.♻️ Proposed fix
if not knowledge_dir.exists(): return KnowledgeFileList(files=[], count=0) + from datetime import datetime + files = [] for filepath in knowledge_dir.glob("*.md"): if filepath.is_file(): stat = filepath.stat() - from datetime import datetime files.append(KnowledgeFile( name=filepath.name, size=stat.st_size,server/routers/templates.py (2)
45-48: UseField(default_factory=...)for mutable defaults.Mutable defaults (
{}/[]) can be shared across model instances (notably in Pydantic v2) if any code mutates them. Safer to use factories.♻️ Proposed fix
class DesignTokensInfo(BaseModel): """Design tokens information.""" - colors: dict[str, str] = {} - spacing: list[int] = [] - fonts: dict[str, str] = {} - border_radius: dict[str, str] = {} + colors: dict[str, str] = Field(default_factory=dict) + spacing: list[int] = Field(default_factory=list) + fonts: dict[str, str] = Field(default_factory=dict) + border_radius: dict[str, str] = Field(default_factory=dict) class TemplateInfo(BaseModel): """Template summary information.""" @@ - tags: list[str] = [] + tags: list[str] = Field(default_factory=list) class TemplateDetail(BaseModel): """Full template details.""" @@ - tags: list[str] = [] + tags: list[str] = Field(default_factory=list)Also applies to: 58-58, 72-72
237-285: Avoid blocking filesystem work insideasynchandlers.This endpoint performs blocking I/O (
mkdir, file writes, template loading). In anasync def, that blocks the event loop. Consider making these handlers sync (def) or offloading I/O to a threadpool.♻️ Minimal change (sync handler)
-@router.post("/apply", response_model=ApplyResponse) -async def apply_template(request: ApplyRequest): +@router.post("/apply", response_model=ApplyResponse) +def apply_template(request: ApplyRequest):
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Fix all issues with AI agents
In `@design_tokens.py`:
- Around line 512-554: validate_contrast currently calls ColorToken.to_hsl which
can raise on invalid color values and cause 500s; wrap the to_hsl() call in a
try/except inside validate_contrast, catch exceptions raised by
ColorToken.to_hsl, and append an entry to the issues list (including the color
name, original value, and the exception message) instead of letting the
exception propagate, then continue to the next token so invalid colors are
reported but do not break the validation flow.
- Around line 555-587: The generate_all method currently calls
generate_css/generate_scss/generate_tailwind_config and returns their string
results (file contents), but those helpers write files when given an output
path; update generate_all to return the actual output file paths instead:
construct the output Path objects (e.g. css_path = output / "tokens.css",
scss_path = output / "_tokens.scss", tailwind_path = output /
"tailwind.tokens.js") and pass those paths to
generate_css/generate_scss/generate_tailwind_config, then set results["css"] =
str(css_path), results["scss"] = str(scss_path) and when tailwind is present set
results["tailwind"] = str(tailwind_path); keep validate_contrast(tokens)
behavior and include its issues under "contrast_issues" as before.
In `@quality_gates.py`:
- Around line 118-151: The _detect_python_linter function can misidentify flake8
as ruff because venv_paths mixes ruff and flake8 binaries and the ruff-detection
loop checks that combined list; fix _detect_python_linter by splitting the
virtualenv checks into distinct lists (e.g., venv_ruff_paths and
venv_flake8_paths) or otherwise checking the filename to ensure you only return
("ruff", ...) for ruff binaries and ("flake8", ...) for flake8 binaries; update
the ruff detection loop to iterate only ruff-specific venv paths and the flake8
detection loop to iterate only flake8-specific venv paths so returns from
_detect_python_linter are unambiguous.
In `@requirements.txt`:
- Around line 1-18: Update the package constraints in requirements.txt: replace
the loose fastapi entry (fastapi>=0.128.0) with a compatible-release constraint
(fastapi~=0.128.0) and replace the openai line (openai>=1.52.0) with a pinned
compatible-release to the current stable (openai~=2.15.0) — or, if you
intentionally must stay on the 1.52 series, use openai~=1.52.2 instead; locate
and update the fastapi and openai lines in the file accordingly.
In `@security.py`:
- Around line 88-116: get_denied_commands currently returns cmd.reason verbatim
which can leak secrets; update the function to redact or hash the reason before
returning. Reuse or extend the existing redact_string helper (or add a small
helper like redact_or_hash_reason) and apply it to cmd.reason in the returned
dict (replace "reason": cmd.reason with "reason": redact_string(cmd.reason) or a
hashed representation such as "sha256:<prefix>"). Ensure the change only affects
the exported response and continue to use _denied_commands and
cmd.timestamp/command/project_dir as before.
In `@server/routers/design_tokens.py`:
- Around line 317-380: In validate_tokens, the color format loop currently calls
hex_pattern.match(value) and will raise if value is not a string; update the
loop that iterates tokens.colors (in the validate_tokens function, using
hex_pattern) to first check isinstance(value, str) (or equivalent) and if not a
string append a color_format issue for that field (e.g., "Non-string color
value") instead of calling match; if it is a string then perform
hex_pattern.match and append the existing "Invalid hex color format" issue when
it fails. Ensure you reference the same issue structure and field naming
(f"colors.{name}") so other parts like
ValidateResponse/manager.validate_contrast remain unchanged.
- Around line 195-234: In update_design_tokens, the current truthy checks (e.g.,
if request.colors:) prevent clients from clearing sections by sending empty
dicts/lists; change all checks to explicit None checks (e.g., if request.colors
is not None, if request.spacing is not None, if request.typography is not None,
etc.) so empty values supplied on the DesignTokensRequest intentionally replace
or clear current.colors, current.spacing, current.typography, current.borders,
current.shadows, and current.animations before calling manager.save(current).
In `@server/routers/documentation.py`:
- Around line 91-136: The validation in _validate_project_path currently only
allows paths under Path.cwd(), which rejects valid projects registered
elsewhere; update validation to consult the project registry (the SQLite DB in
~/.autocoder/registry.db used by get_project_path) and treat any path that
exactly matches or is a subpath of any registered project path as allowed (in
addition to keeping the existing cwd check as a fallback). Modify
_validate_project_path to load registered project paths (POSIX-normalized) from
the registry and allow the input path if path == registered_path or
path.is_relative_to(registered_path); keep raising HTTPException(403) only if
the path is neither under cwd nor any registered project entry, and ensure you
reference get_project_path, get_project_dir and _validate_project_path when
making changes.
- Around line 143-183: The generate_docs endpoint currently constructs a
DocumentationGenerator with an unvalidated output_dir which allows path
traversal; update DocumentationGenerator.__init__ to resolve and enforce that
the computed output directory is contained within the resolved project_dir
(e.g., resolve self.project_dir and resolved_output = (self.project_dir /
output_dir).resolve(), then raise an error if resolved_output is not a subpath
of self.project_dir), and ensure generate_docs handles that exception
(DocumentationGenerator and generate_docs are the symbols to modify).
In `@server/routers/review.py`:
- Around line 117-129: The get_project_dir function currently accepts any
existing directory path; tighten this by validating and containing direct-path
inputs: resolve the provided Path (follow symlinks), then ensure it lies under
an allowed projects root or workspace directory (e.g., compare
resolved_path.is_relative_to(allowed_root) or check string-prefix of
resolved_path against allowed_root.resolve()), and optionally reject known
dangerous locations via a small blocklist (e.g., root, /etc, /var, home dirs)
before returning; if containment or blocklist checks fail, raise the same
HTTPException(404) as when not found. Use get_project_dir and get_project_path
to locate the exact spot to add these checks.
In `@server/routers/templates.py`:
- Around line 132-326: The router functions are shadowing undefined helpers and
causing recursion (e.g., list_templates() and get_template() call themselves)
and missing imports for generate_app_spec/generate_features; import the helper
functions from templates.library with aliases (e.g., from templates.library
import list_templates as list_templates_lib, get_template as get_template_lib,
generate_app_spec as generate_app_spec_lib, generate_features as
generate_features_lib) and then replace calls inside this module: use
list_templates_lib() where templates = list_templates() is called, use
get_template_lib(template_id) / get_template_lib(request.template_id) where
get_template(...) is currently used, and use generate_app_spec_lib(...) and
generate_features_lib(...) where generate_app_spec(...) and
generate_features(...) are called so the router functions don't recurse and the
helpers are defined.
In `@server/routers/visual_regression.py`:
- Around line 321-342: The update_baseline endpoint (function update_baseline)
currently lacks the traversal/separator validation that delete_baseline applies;
add the same safety checks on request.name and request.viewport (rejecting
values containing '..' or path separators) before any file operations and return
an HTTPException with status_code=400 for invalid inputs, or reuse the existing
validation helper used by delete_baseline to centralize the check and then
proceed with VisualRegressionTester.update_baseline only for validated
names/viewports.
🧹 Nitpick comments (5)
server/routers/templates.py (1)
24-28: Useresolve()and explicit parent indexing for clearer sys.path setup.Line 26 uses 4 parent references to reach the repo root, which works but is less idiomatic. Use
.resolve().parents[2]instead for clarity and robustness with absolute paths.Suggested improvement
-# Compute project root and ensure it's in sys.path -project_root = Path(__file__).parent.parent.parent.parent +# Compute project root (repo root) and ensure it's in sys.path +project_root = Path(__file__).resolve().parents[2] if str(project_root) not in sys.path: sys.path.insert(0, str(project_root)).github/workflows/pr-check.yml (1)
38-48: Create a virtualenv before installing requirements.
This keeps CI aligned with the repo guideline for requirements installs.♻️ Suggested update
- uses: actions/setup-python@v5 with: python-version: "3.11" cache: "pip" cache-dependency-path: requirements.txt + - name: Create virtual environment + run: | + python -m venv .venv + echo ".venv/bin" >> $GITHUB_PATH - name: Install dependencies run: pip install -r requirements.txtAs per coding guidelines:
**/requirements.txt: Create and activate a Python virtual environment before installing dependencies.server/routers/review.py (1)
255-318: Consider explicitly settingdependenciesfor clarity, though it's optional.The
Feature.dependenciesfield exists in the model (api/models.pyline 54) and is nullable withdefault=None. Since the model and its helper methods (to_dict(),get_dependencies_safe()) properly handleNonevalues, omitting it from the insertion will work correctly. However, explicitly setting it (e.g.,dependencies=json.dumps(issue.get("dependencies", []))) would improve code clarity and make intent explicit rather than relying on defaults.analyzers/stack_detector.py (1)
99-107: Category classification may miss some backend frameworks.The category classification doesn't include all backend frameworks that the NodeAnalyzer can detect. According to the relevant code snippets, NodeAnalyzer can detect
fastifyandkoain addition toexpressandnestjs, but these aren't in the backend category list.♻️ Proposed fix to include all detected backends
# Determine category stack_name = analyzer.stack_name.lower() if stack_name in ("react", "nextjs", "vue", "nuxt", "angular"): category = "frontend" - elif stack_name in ("express", "fastapi", "django", "flask", "nestjs"): + elif stack_name in ("express", "fastapi", "django", "flask", "nestjs", "fastify", "koa", "nodejs"): category = "backend" elif stack_name in ("postgres", "mysql", "mongodb", "sqlite"): category = "database" else: category = "other"server/routers/logs.py (1)
28-32: Consider extracting_get_project_pathto a shared utility.This helper function is duplicated across multiple routers (
import_project.py,cicd.py,git_workflow.py,security.py, and nowlogs.py). Extracting it to a shared utility module would reduce duplication and ensure consistent behavior.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
parallel_orchestrator.py (1)
544-552: Apply the same Windows/STDIN safeguards to coding agent subprocess.The coding agent
Popencall lacksstdin=subprocess.DEVNULLand WindowsCREATE_NO_WINDOW, which is inconsistent with the initializer subprocess (lines 682-694) that now has these safeguards. This can cause stdin blocking and console popups on Windows.🛠️ Suggested fix
try: - proc = subprocess.Popen( - cmd, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, - text=True, - cwd=str(AUTOCODER_ROOT), - env={**os.environ, "PYTHONUNBUFFERED": "1"}, - ) + popen_kwargs = { + "stdin": subprocess.DEVNULL, + "stdout": subprocess.PIPE, + "stderr": subprocess.STDOUT, + "text": True, + "cwd": str(AUTOCODER_ROOT), + "env": {**os.environ, "PYTHONUNBUFFERED": "1"}, + } + if sys.platform == "win32": + popen_kwargs["creationflags"] = subprocess.CREATE_NO_WINDOW + proc = subprocess.Popen(cmd, **popen_kwargs)security.py (1)
575-582: Documentation inconsistency: docstring and comments don't match implementation behavior.The docstring and inline comment state "only allow ./init.sh", but the implementation at line 579 allows any path ending with
/init.sh(e.g.,/path/to/init.sh,../scripts/init.sh). The test suite explicitly validates this permissive behavior as intentional (test cases at lines 180-181). Either update the docstring/comments to reflect the actual permissive path validation, or tighten the validation to match the documented intent:Option 1: Update docstring to match implementation
""" - Validate init.sh script execution - only allow ./init.sh. + Validate init.sh script execution - allow ./init.sh or any path ending in /init.sh.Option 2: Tighten validation to match docstring
- if script == "./init.sh" or script.endswith("/init.sh"): + if script == "./init.sh": return True, ""
🤖 Fix all issues with AI agents
In `@analyzers/stack_detector.py`:
- Around line 91-94: Wrap calls to analyzer.can_analyze() in a try/except so an
exception in one analyzer won't abort detect()/detect_quick(); if can_analyze()
raises, log the exception (using the same logger used elsewhere in this file,
e.g., self._logger or logger) and treat that analyzer as not able to analyze
(skip it). Update both places where can_analyze() is invoked (the loop around
the confidence check near the existing "if can_analyze and confidence > 0.3" and
the similar block around lines 179-181) to catch exceptions, log the error with
context (including analyzer identity), and continue to the next analyzer.
In `@design_tokens.py`:
- Around line 404-410: The loop iterating tokens.colors should guard against
invalid color values the same way Tailwind config generation does: wrap the
creation of ColorToken(name, value) and the call to generate_shades() in a
try/except (or validate before creating) so an invalid color doesn't raise and
stop processing; on error, log or skip and set colors[name] to {"DEFAULT":
value} (or a safe fallback) and continue. Apply this to the block that uses
tokens.colors, ColorToken, generate_shades, and colors[name] to mirror the
existing error handling pattern.
- Around line 470-476: The SCSS generation loop over tokens.colors must mirror
the invalid-color handling used in generate_css: when creating ColorToken(name)
and calling generate_shades(), catch the same InvalidColorError (or the specific
exception used in generate_css) and skip adding shade variables while still
emitting the base "$color-{name}: {value};" line; optionally append a comment
like "/* invalid color {name}: {value} */" to lines for visibility, then
continue. Update the block that uses tokens.colors, ColorToken, generate_shades,
and lines.append to wrap generate_shades() in a try/except matching the
generate_css pattern.
- Around line 320-326: The CSS generation loop crashes if a color is invalid
because ColorToken.generate_shades() (which calls to_hsl()) can raise; update
the loop that iterates tokens.colors so you catch exceptions around creating
ColorToken(name, value) and calling generate_shades(), log a warning including
the color name/value, and skip adding that color's CSS lines to the lines list;
ensure you still process remaining colors so one bad token doesn't produce a
500.
In `@quality_gates.py`:
- Around line 166-181: The TypeScript local tsc check should remain unchanged
(keep the existing project_dir / "node_modules/.bin/tsc" existence check and the
npx fallback) because shutil.which() is not appropriate for a fixed path; do not
replace it with shutil.which(str(...)). For mypy, replace the single venv_mypy
check with iterating over multiple venv candidates (e.g., venv/bin/mypy,
venv/Scripts/mypy.exe, venv/Scripts/mypy) so Windows venv layouts are covered;
use project_dir to build these paths, test .exists() on each venv_mypy
candidate, and return ("mypy", [str(venv_mypy), "."]) for the first match
(keeping the existing shutil.which("mypy") check as the first option).
In `@server/routers/documentation.py`:
- Around line 289-295: The loop that builds the feature list uses defensive
access for status but uses direct dict access f['name'], which can raise
KeyError; update the feature rendering in the block that iterates over
docs.features (the for f in docs.features[:10] loop) to use defensive access
like f.get("name", "<unknown>") (or a short fallback string) and keep the
existing status logic so missing names do not crash the formatter.
In `@server/routers/review.py`:
- Around line 192-194: The except block currently logs and returns the raw
exception string (logger.error(f"Review failed for {project_dir}: {e}") and
raise HTTPException(..., detail=str(e))), which can leak internals; change it to
log the full exception server-side using logger.exception (or logger.error with
exc_info=True) for debugging, but raise HTTPException with a generic
client-facing message like "Internal server error processing review" (keep the
existing logger and HTTPException calls: the logger call should include exc_info
and the HTTPException detail should be a generic message).
- Around line 312-317: The exception handler currently logs and raises but only
closes the DB session; update it to explicitly call session.rollback() before
session.close() when an exception occurs to ensure the transaction is aborted
and connection state is clean. Locate the try/except/finally around
session.add()/session.commit() (the block using variable session in this file)
and add a conditional session.rollback() in the except or finally path (e.g.,
directly before session.close()) so any pending transaction is rolled back, then
keep the existing logger.error(...) and HTTPException re-raise behavior.
- Around line 152-155: The current file-path validation in the request.files
loop (inside the block that raises HTTPException on invalid paths) misses
Windows absolute paths like "C:\..."—update the validation in the same loop that
iterates request.files to also detect Windows drive-letter absolute paths by
either using a drive-letter pattern check (e.g., leading [A-Za-z]: with a
backslash or slash) or by using a cross-platform absolute-path check
(os.path.isabs or pathlib.Path.is_absolute) and raise the same
HTTPException(status_code=400, detail=f"Invalid file path: {file_path}") when
such a path is detected; keep the existing checks for "..", leading "/" and
leading "\\" and add the new drive-letter check in the same conditional block.
In `@server/routers/templates.py`:
- Around line 22-26: The parent path calculation for project_root is off by one
and points above the repository root; update the calculation in the module that
sets up sys.path by changing the Path ancestor count from 4 to 3 so project_root
= Path(__file__).parent.parent.parent, then ensure the existing sys.path
check/insert (the sys.path.insert(0, str(project_root)) call and the variable
project_root) remain unchanged so the repo-root "templates" package is reachable
for the subsequent imports.
🧹 Nitpick comments (9)
server/routers/review.py (3)
98-109: Consider using typed models instead oflist[dict]for better validation.The
issuesandfeaturesfields uselist[dict]which bypasses Pydantic's validation benefits. Defining explicit models (e.g.,CreateFeatureIssuefor input,CreatedFeaturefor output) would provide runtime validation and clearer API documentation.
209-212: Specify encoding when opening files for cross-platform consistency.Using
open(report_file)without explicit encoding relies on system defaults, which may differ across platforms. Explicitly specify UTF-8.♻️ Proposed fix
- with open(report_file) as f: + with open(report_file, encoding="utf-8") as f: data = json.load(f)
248-252: Same improvements needed: encoding and error message handling.Apply UTF-8 encoding and avoid exposing internal errors.
♻️ Proposed fix
try: - with open(report_path) as f: + with open(report_path, encoding="utf-8") as f: return json.load(f) except Exception as e: - raise HTTPException(status_code=500, detail=f"Error reading report: {e}") + logger.error(f"Error reading report {report_path}: {e}") + raise HTTPException(status_code=500, detail="Error reading report")server/routers/documentation.py (1)
200-208: Consider formattingmodifiedtimestamp as ISO 8601.
stat.st_mtime.__str__()produces a raw float like"1706131200.123456", which is not human-readable. Consider converting to ISO format for better API ergonomics.♻️ Suggested improvement
+from datetime import datetime ... - modified=stat.st_mtime.__str__(), + modified=datetime.fromtimestamp(stat.st_mtime).isoformat(),Apply the same change at line 220.
parallel_orchestrator.py (2)
482-510: Good atomic claim pattern, but remove redundant import.The UPDATE-WHERE pattern correctly prevents race conditions where multiple agents try to claim the same feature. However,
textis already imported at the module level (line 32), making the local import redundant.♻️ Suggested fix
else: # Starting fresh: atomic claim using UPDATE-WHERE pattern (same as testing agent) # This prevents race conditions where multiple agents try to claim the same feature - from sqlalchemy import text result = session.execute( text(""" UPDATE features
1095-1100: Remove redundantengine.dispose()call beforeget_ready_features().The code calls
self._engine.dispose()on every loop iteration (line 1100), but the codebase's own documentation at lines 849–851 explicitly states: "engine.dispose() is only called on orchestrator shutdown, not on every agent completion." This disposal contradicts the stated design pattern.The
session.expire_all()call insideget_ready_features()(line 284) already invalidates cached ORM data and forces fresh reads from the database. A secondsession.expire_all()is also called as a fallback when no ready features are found (lines 1104–1108). Disposing the engine on every loop iteration creates unnecessary connection churn without additional benefit.♻️ Remove the redundant disposal
# Priority 2: Start new ready features - # CRITICAL: Dispose engine to force fresh database reads - # Coding agents run as subprocesses and commit changes (passes=True, in_progress=False). - # SQLAlchemy connection pool may cache stale connections. Disposing ensures we see - # all subprocess commits when checking dependencies. - debug_log.log("DB", "Disposing engine before get_ready_features()") - self._engine.dispose() ready = self.get_ready_features()The existing
session.expire_all()insideget_ready_features()is sufficient to handle subprocess commits.security.py (2)
74-77: Duplicateredact_stringhelper defined in two places.The same helper is defined inside both
record_denied_command(lines 74-77) andget_denied_commands(lines 103-106). Extract it to module level to avoid duplication and ensure consistent behavior.♻️ Proposed refactor
+def _redact_string(s: str, max_preview: int = 20) -> str: + """Redact middle of string if too long, keeping first/last chars.""" + if len(s) <= max_preview * 2: + return s + return f"{s[:max_preview]}...{s[-max_preview:]}" + + def record_denied_command(command: str, reason: str, project_dir: Optional[Path] = None) -> None: ... - def redact_string(s: str, max_preview: int = 20) -> str: - if len(s) <= max_preview * 2: - return s - return f"{s[:max_preview]}...{s[-max_preview:]}" - - command_preview = redact_string(command, 20) + command_preview = _redact_string(command, 20)Also applies to: 103-106
552-552: Redundantimport reinside function.
reis already imported at module level (line 12). Remove this local import.♻️ Proposed fix
# Only allow +x variants (making files executable) # This matches: +x, u+x, g+x, o+x, a+x, ug+x, etc. - import re - if not re.match(r"^[ugoa]*\+x$", mode):server/routers/templates.py (1)
52-58: Avoid mutable defaults in Pydantic models.Use
Field(default_factory=...)for dict/list defaults to prevent shared state across model instances. Please confirm this aligns with the Pydantic version in use.♻️ Suggested refactor
class DesignTokensInfo(BaseModel): @@ - colors: dict[str, str] = {} - spacing: list[int] = [] - fonts: dict[str, str] = {} - border_radius: dict[str, str] = {} + colors: dict[str, str] = Field(default_factory=dict) + spacing: list[int] = Field(default_factory=list) + fonts: dict[str, str] = Field(default_factory=dict) + border_radius: dict[str, str] = Field(default_factory=dict) @@ class TemplateInfo(BaseModel): @@ - tags: list[str] = [] + tags: list[str] = Field(default_factory=list) @@ class TemplateDetail(BaseModel): @@ - tags: list[str] = [] + tags: list[str] = Field(default_factory=list)Also applies to: 68-69, 82-83
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@server/routers/design_tokens.py`:
- Around line 120-162: The resolved registered project path returned by
get_project_dir (resolved_path from get_project_path) must be checked for
existence and that it is a directory before returning; if not present or not a
dir, raise HTTPException(status_code=404, detail=f"Project not found:
{project_name}"). Add this check immediately after resolved_path =
Path(project_path).resolve() (or move it into _validate_project_path so that
_validate_project_path(path: Path) also verifies path.exists() and path.is_dir()
in addition to is_path_blocked) to prevent using stale registry entries.
- Around line 208-221: The update logic uses dict.update(), so passing an empty
dict doesn't clear the section; change each branch that currently calls update()
(current.colors.update(request.colors),
current.typography.update(request.typography),
current.borders.update(request.borders),
current.shadows.update(request.shadows),
current.animations.update(request.animations)) to detect an explicit empty dict
and assign it (e.g., if request.colors == {}: current.colors = {} else:
current.colors.update(request.colors)), leaving spacing assignment as-is; apply
the same pattern for typography, borders, shadows and animations so an explicit
{} clears the token section while non-empty dicts are merged.
In `@server/routers/review.py`:
- Around line 148-179: The root blocklist currently rejects every absolute path
because Path("/") is always a parent; update the logic in dangerous_roots and
its check so only exact matches are blocked for root and home while parent
containment is checked for specific system dirs. Concretely, remove or keep
Path("/") but change the loop that inspects dangerous to: if dangerous is root
or Path.home() then compare equality with resolved_path (dangerous ==
resolved_path) else check containment with (dangerous in resolved_path.parents
or dangerous == resolved_path) for /etc and /var; keep using resolved_path,
dangerous_roots, dangerous, allowed_root, and is_relative_to as identifiers and
preserve the existing try/except and HTTPException behavior.
- Around line 299-303: Replace the current exception detail leak in the
report-reading block: catch exceptions from json.load(report_path) but do not
include the raw exception text in the raised HTTPException; instead log the full
exception server-side (using the existing logger or process logger) and raise
HTTPException(status_code=500, detail="Error reading report") with a generic
message. Keep references to report_path, json.load, and
HTTPException(status_code=500) so you update the same try/except that currently
does "with open(report_path) as f: return json.load(f)" and "except Exception as
e: raise HTTPException(...)".
In `@server/routers/visual_regression.py`:
- Around line 298-319: The get_report handler currently accepts any filename
that passes basic traversal checks; tighten validation in get_report by
enforcing a strict filename pattern (e.g., require a known prefix and extension
such as "report-*.json" or a regex like ^report-[A-Za-z0-9_-]+\.json$), validate
filename against that regex before constructing report_path, and return 400 if
it fails; apply the same validation logic to the corresponding delete endpoint
(the handler covering lines ~413-433) so both read and delete only operate on
expected report file names.
🧹 Nitpick comments (3)
auto_documentation.py (2)
277-298: Consider using a context manager for database session.If an exception occurs between
get_session()andsession.close(), the session won't be properly closed. While the outer try/except catches the error, using a context manager would ensure cleanup.♻️ Proposed refactor
if db_path.exists(): try: from api.database import Feature, get_session - session = get_session(db_path) - db_features = session.query(Feature).order_by(Feature.priority).all() - - for f in db_features: - features.append( - { - "category": f.category, - "name": f.name, - "description": f.description, - "status": "completed" if f.passes else "pending", - } - ) - session.close() + with get_session(db_path) as session: + db_features = session.query(Feature).order_by(Feature.priority).all() + + for f in db_features: + features.append( + { + "category": f.category, + "name": f.name, + "description": f.description, + "status": "completed" if f.passes else "pending", + } + ) except Exception as e: logger.warning(f"Error reading features.db: {e}")Note: This requires
get_sessionto support context manager protocol. If it doesn't, wrap withcontextlib.closing(session).
508-514: Consider defensive access for dictionary keys.While the data sources (
_extract_features,_extract_environment_vars) guarantee these keys exist, using.get()would provide an extra safety net against future changes that might introduce incomplete entries.♻️ Example for features
for cat, features in categories.items(): lines.append(f"### {cat}\n") for f in features: status = "[x]" if f.get("status") == "completed" else "[ ]" - lines.append(f"- {status} {f['name']}") + lines.append(f"- {status} {f.get('name', 'Unnamed')}") lines.append("")server/routers/templates.py (1)
53-84: UseField(default_factory=...)for mutable defaults in Pydantic models.Pydantic's recommended approach is to use
Field(default_factory=...)for dict/list fields in BaseModel classes. While Pydantic protects against shared instances through deep-copying, usingdefault_factoryis the idiomatic pattern and makes the intent clearer.Update
colors,spacing,fonts,border_radiusinDesignTokensInfoandtagsin bothTemplateInfoandTemplateDetail:♻️ Proposed refactor
class DesignTokensInfo(BaseModel): """Design tokens information.""" - colors: dict[str, str] = {} - spacing: list[int] = [] - fonts: dict[str, str] = {} - border_radius: dict[str, str] = {} + colors: dict[str, str] = Field(default_factory=dict) + spacing: list[int] = Field(default_factory=list) + fonts: dict[str, str] = Field(default_factory=dict) + border_radius: dict[str, str] = Field(default_factory=dict) class TemplateInfo(BaseModel): """Template summary information.""" @@ - tags: list[str] = [] + tags: list[str] = Field(default_factory=list) class TemplateDetail(BaseModel): """Full template details.""" @@ - tags: list[str] = [] + tags: list[str] = Field(default_factory=list)
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@analyzers/stack_detector.py`:
- Around line 71-77: The inline comment above the analyzer list incorrectly
mentions "Next.js before React" while the actual list assigned to
self._analyzers contains ReactAnalyzer, VueAnalyzer, NodeAnalyzer, and
PythonAnalyzer; either remove/replace the Next.js reference in that comment to
accurately reflect the loaded analyzers or actually add a NextJsAnalyzer
instance (and import) to self._analyzers if Next.js support is intended—update
the comment or the list accordingly so the comment and the symbol
self._analyzers (and analyzers ReactAnalyzer, VueAnalyzer, NodeAnalyzer,
PythonAnalyzer or NextJsAnalyzer) stay consistent.
In `@server/routers/documentation.py`:
- Around line 195-197: The exception handler in the documentation generation
code currently logs and returns the raw exception string (logger.error and
raising HTTPException with str(e)), which may leak internal details; change it
to log the full error/stacktrace via logger.exception or logger.error with
traceback, and raise HTTPException with a generic, non-sensitive message
consistent with other endpoints (e.g., "Internal server error generating
documentation") instead of str(e) while keeping the detailed error only in
server logs; update the block that references logger and raises HTTPException
accordingly.
- Around line 324-326: The endpoint currently logs the exception and returns the
raw error to clients; change the raise HTTPException(...) to return a generic
message instead of str(e) while keeping the detailed logger.error call (i.e.,
keep logger.error(f"Preview failed: {e}") or expand it) but replace
detail=str(e) with a non-sensitive message like "Preview failed" or "Internal
server error" in the HTTPException raised from the except block where
logger.error and HTTPException are used.
- Around line 361-362: The exception handler currently raises
HTTPException(status_code=500, detail=f"Error deleting file: {e}") which leaks
internal error details; change it to raise a generic HTTPException with a
non-sensitive message like "Error deleting file" (remove the interpolated
exception), and if needed log the original exception internally via the module
logger (e.g., logger.exception or logger.error) before raising; update the
exception handling block that raises HTTPException(status_code=500, ...) in the
delete file route/handler in documentation.py.
- Around line 274-275: The except Exception block in
server/routers/documentation.py currently raises HTTPException with the raw
exception string (detail=f"Error reading file: {e}"), which can leak internal
details; replace that with a generic error response like raise
HTTPException(status_code=500, detail="Error reading file") and record the full
exception details to your internal logs instead (e.g., logger.exception(...) or
capture traceback) so debugging info is kept in logs but not exposed to clients;
keep the HTTPException usage but remove str(e) from the response.
🧹 Nitpick comments (13)
server/routers/design_tokens.py (4)
35-68: Consider removing or utilizing unused Pydantic models.
ColorTokens,TypographyTokens,BorderTokens, andAnimationTokensare defined but not used in any endpoint. The actual endpoints usedicttypes inDesignTokensRequest. Either integrate these models for stricter validation or remove them to reduce dead code.
317-325: Remove unreachable else branch.Lines 323-324 are unreachable since
formatis already validated againstvalid_formatsat line 307. This dead code should be removed for clarity.🧹 Proposed fix
if format == "css": content = manager.generate_css(tokens) elif format == "scss": content = manager.generate_scss(tokens) - elif format == "tailwind": + else: # format == "tailwind" content = manager.generate_tailwind_config(tokens) - else: - content = ""
350-353: Movereimport to module level.The
remodule import should be at the top of the file with other imports rather than inside the function. This follows Python conventions and avoids repeated import overhead on each call.🧹 Proposed fix
Add at the top with other imports:
import reThen remove line 351 from inside the function.
387-399: Clarify spacing validation error message.The error message at line 396 says the scale "should be increasing" with
>=comparison, but the condition checks<=. Consider rewording for clarity.📝 Proposed fix
issues.append( { "type": "spacing_scale", "field": "spacing", "value": tokens.spacing, - "message": f"Spacing scale should be increasing: {tokens.spacing[i-1]} >= {tokens.spacing[i]}", + "message": f"Spacing scale should be strictly increasing: found {tokens.spacing[i-1]} followed by {tokens.spacing[i]}", } )analyzers/stack_detector.py (1)
174-202: Consider adding a TypedDict for the return type.The
detect_quick()method returns a plaindictwhiledetect()returns a well-typedStackDetectionResult. For consistency and better IDE support, consider defining aQuickDetectionResultTypedDict.♻️ Suggested improvement
class QuickDetectionResult(TypedDict): """Result of quick stack detection.""" project_dir: str stacks: list[dict] primary: str | None def detect_quick(self) -> QuickDetectionResult: ...server/routers/templates.py (3)
54-60: Consider usingField(default_factory=...)for mutable defaults.While Pydantic v2 handles mutable defaults safely, using explicit
default_factorymakes the intent clearer and avoids potential linter warnings.♻️ Suggested improvement
class DesignTokensInfo(BaseModel): """Design tokens information.""" - colors: dict[str, str] = {} - spacing: list[int] = [] - fonts: dict[str, str] = {} - border_radius: dict[str, str] = {} + colors: dict[str, str] = Field(default_factory=dict) + spacing: list[int] = Field(default_factory=list) + fonts: dict[str, str] = Field(default_factory=dict) + border_radius: dict[str, str] = Field(default_factory=dict)
256-274: Path validation is solid; consider adding null byte check for defense-in-depth.The
resolve()+relative_to()pattern at lines 267-274 provides robust protection against path traversal. However, adding an explicit null byte check guards against injection attacks on certain platforms.🔒 Add null byte check
raw_path = request.project_dir - if ".." in raw_path: + if ".." in raw_path or "\x00" in raw_path: raise HTTPException(status_code=400, detail="Invalid project directory: path traversal not allowed")
309-337: Consider adding a response model for API consistency.Unlike other endpoints in this router,
get_template_featuresreturns a raw dict without aresponse_model. Adding one would improve OpenAPI documentation consistency and provide response validation.♻️ Suggested improvement
class TemplateFeaturesResponse(BaseModel): """Response with template features.""" template_id: str features: list[dict] # or more specific type count: int by_category: dict[str, int] `@router.get`("/{template_id}/features", response_model=TemplateFeaturesResponse) async def get_template_features(template_id: str): ...server/routers/visual_regression.py (1)
50-56: Consider adding validation constraints for viewport dimensions.Negative or zero dimensions would cause issues in the visual regression tester. Adding Pydantic constraints would provide early validation.
✨ Suggested improvement
class ViewportConfig(BaseModel): """Viewport configuration.""" name: str - width: int - height: int + width: int = Field(..., gt=0, description="Viewport width in pixels") + height: int = Field(..., gt=0, description="Viewport height in pixels")quality_gates.py (1)
166-172: Inconsistent path handling for tsc command.Line 168 returns a relative path string
"node_modules/.bin/tsc"directly, while other detectors (e.g., line 92, 102, 143) usestr(absolute_path). Although this works because_run_commandsetscwdtoproject_dir, it's inconsistent and could cause confusion.Additionally, on Windows the
node_modules/.bin/tsccheck will fail since onlytsc.cmdexists there, but thenpxfallback provides coverage.🔧 Suggested fix for consistency
if (project_dir / "tsconfig.json").exists(): - if (project_dir / "node_modules/.bin/tsc").exists(): - return ("tsc", ["node_modules/.bin/tsc", "--noEmit"]) + local_tsc = project_dir / "node_modules/.bin/tsc" + if local_tsc.exists(): + return ("tsc", [str(local_tsc), "--noEmit"]) if shutil.which("npx"):server/routers/review.py (2)
117-188: Consider extracting path validation to a shared module.The
get_project_dirand_validate_project_dirfunctions are duplicated across multiple routers (review.py,documentation.py,design_tokens.py) with slight variations. This creates maintenance burden and risk of inconsistent security behavior.♻️ Suggested approach
Create a shared utility module (e.g.,
server/utils/project_paths.py) with a single implementation:# server/utils/project_paths.py from pathlib import Path from fastapi import HTTPException from registry import get_project_path def get_project_dir(project_name: str) -> Path: """Shared project directory resolution with validation.""" # ... unified implementation def validate_project_path(path: Path) -> None: """Shared path validation logic.""" # ... unified implementationThen import in each router:
from ..utils.project_paths import get_project_dir
323-323: Consider moving import to module level.The late import of
api.databaseinside the function works but may indicate a circular import issue. If there's no circular dependency, moving it to the top of the file improves clarity and catches import errors at startup.server/main.py (1)
156-159: Consider consolidating duplicate health endpoints.Two health endpoints exist:
GET /health→{"status": "ok"}(lines 156-159)GET /api/health→{"status": "healthy"}(lines 283-286)If both serve the same purpose, consider removing one to avoid confusion. If they're intentionally different (e.g.,
/healthfor infrastructure probes,/api/healthfor API consumers), document the distinction.Also applies to: 283-286
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/main.py`:
- Around line 10-11: Import the binascii module and update the error handling
around base64.b64decode so that binascii.Error is caught (in the same except
block that currently catches other decode/auth errors) and handled by returning
the 401 response instead of allowing a 500; specifically add "import binascii"
and extend the exception handling that surrounds base64.b64decode to catch
binascii.Error (or include it in the existing except tuple) so malformed Base64
input triggers the 401 path.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@analyzers/node_analyzer.py`:
- Around line 95-176: The analyze() function returns a hardcoded "confidence":
0.85 which may contradict the actual detection confidence; update the return to
use the analyzer's detection score (use self._detection_confidence) and fall
back to 0.85 if that attribute is missing or None, ensuring the value is a float
clamped between 0.0 and 1.0; modify the return dict's "confidence" entry to
compute something like confidence = float(getattr(self, "_detection_confidence",
0.85) or 0.85) and clamp it before returning so analyze(), self._detected_stack
and the returned "confidence" stay consistent.
In `@analyzers/react_analyzer.py`:
- Around line 86-147: The analyze() function currently hardcodes "confidence":
0.9 which can conflict with the actual detection confidence from can_analyze();
update analyze() to call self.can_analyze() (or otherwise read the detection
confidence provided by the detector) and set the returned "confidence" to that
value, falling back to 0.9 if can_analyze() does not return a numeric
confidence; reference symbols: analyze(), can_analyze(), and the returned dict's
"confidence" field to locate where to replace the hardcoded value.
In `@api/migrations.py`:
- Around line 124-149: The sequence uses conn.execute(...) which autobegins a
transaction and then calls conn.begin() later, causing InvalidRequestError;
instead wrap the whole migration steps (creating features_new via create_sql,
executing the INSERT built as insert_sql that uses optional_col_names,
performing the atomic table swap of features/features_new/features_old, and
recreating indexes ix_features_id, ix_features_priority, ix_features_passes,
ix_features_in_progress, ix_feature_status) inside a single with conn.begin():
block so no nested/second begin is attempted—move the earlier
conn.execute(text(create_sql)) and the INSERT and all ALTER/DROP and CREATE
INDEX calls into that one transaction and remove the extra conn.begin() usages.
In `@deploy.sh`:
- Around line 143-163: The ensure_packages function hardcodes "linux/ubuntu" in
the Docker APT repo URL which breaks non-Ubuntu systems; update ensure_packages
to detect the OS like scripts/deploy.sh (use /etc/os-release or VERSION_CODENAME
and ID/ID_LIKE) and construct the repo base dynamically (e.g., choose "debian"
vs "ubuntu" or fallback to ID) before writing the sources.list entry and adding
the key, so the curl/gpg and the deb line use the detected distribution rather
than a hardcoded "ubuntu".
In `@scripts/deploy.sh`:
- Around line 114-123: In configure_duckdns(), avoid putting $DUCKDNS_TOKEN in
the curl command line (which exposes it via ps); instead send the token in the
request body or an Authorization header (using curl --data or -H) or read it
from a protected env/file so it never appears as a CLI argument; update the cron
entry written to /etc/cron.d/duckdns and the immediate curl invocation in the
function to use the new body/header approach (and ensure any token file has
restrictive permissions) so the token is not visible in process listings.
In `@server/routers/cicd.py`:
- Around line 206-219: The list_workflows implementation currently only globs
"*.yml" so .yaml files are omitted; update the logic in list_workflows to
include both ".yml" and ".yaml" (e.g., iterate workflows_dir.glob("*.yml") and
workflows_dir.glob("*.yaml") or use a pattern that matches both) so that
WorkflowInfo entries (name, filename, type, path) include .yaml files the same
way get_workflow_content accepts both extensions; ensure the wf_type detection
and WorkflowInfo construction remain unchanged.
In `@server/routers/git_workflow.py`:
- Around line 27-32: The import in _get_project_path is wrong—replace the
non-existent validate_and_get_project_path with the exported helper used
elsewhere: import get_project_dir from server.routers.design_tokens and return
get_project_dir(project_name) (adjust the call inside _get_project_path
accordingly) so _get_project_path uses the correct, existing helper instead of
the missing function.
In `@server/routers/import_project.py`:
- Around line 54-70: Add sensitive user home directories to the existing
blocked_paths list: append Path.home().joinpath(".ssh"),
Path.home().joinpath(".gnupg"), and Path.home().joinpath(".config") (call
.resolve()) so blocked_paths (the list defined in import_project.py) contains
these user-sensitive locations; also, in the Windows-specific extension branch
(where os.name == 'nt') consider adding the user's AppData/Local and
AppData/Roaming equivalents (via Path.home().joinpath("AppData", "Local") and
Path.home().joinpath("AppData", "Roaming")) resolved to prevent access to user
credential/config stores.
🧹 Nitpick comments (27)
api/feature_repository.py (2)
24-25: Remove unusedTYPE_CHECKINGblock.The
TYPE_CHECKINGblock is empty and serves no purpose.🧹 Suggested cleanup
-from typing import TYPE_CHECKING, Callable, Optional +from typing import Callable, Optional from sqlalchemy.exc import OperationalError from sqlalchemy.orm import Session from .database import Feature - -if TYPE_CHECKING: - pass
319-325: Consider usingget_dependencies_safe()for consistency with the model.The
Featuremodel providesget_dependencies_safe()which handles NULL values and filters out non-integer elements from malformed data. Usingf.dependencies or []directly doesn't filter malformed entries.♻️ Suggested change for get_ready_features
ready = [] for f in candidates: - deps = f.dependencies or [] + deps = f.get_dependencies_safe() if all(dep_id in passing_ids for dep_id in deps): ready.append(f)♻️ Suggested change for get_blocked_features
blocked = [] for f in candidates: - deps = f.dependencies or [] + deps = f.get_dependencies_safe() blocking = [d for d in deps if d not in passing_ids] if blocking: blocked.append((f, blocking))Also applies to: 337-342
.claude/templates/coding_prompt.template.md (1)
262-280: Exclude test files in mock-data grep commands.The checklist says “excluding tests,” but the commands currently scan all of
src/, which can surface legitimate test fixtures and slow down verification.♻️ Suggested tweak
- grep -r "globalThis\." --include="*.ts" --include="*.tsx" --include="*.js" src/ - grep -r "dev-store\|devStore\|DevStore\|mock-db\|mockDb" --include="*.ts" --include="*.tsx" --include="*.js" src/ + grep -r "globalThis\." --include="*.ts" --include="*.tsx" --include="*.js" \ + --exclude="*.test.*" --exclude="*.spec.*" --exclude-dir="__tests__" src/ + grep -r "dev-store\|devStore\|DevStore\|mock-db\|mockDb" --include="*.ts" --include="*.tsx" --include="*.js" \ + --exclude="*.test.*" --exclude="*.spec.*" --exclude-dir="__tests__" src/ - grep -r "mockData\|fakeData\|sampleData\|dummyData\|testData" --include="*.ts" --include="*.tsx" --include="*.js" src/ + grep -r "mockData\|fakeData\|sampleData\|dummyData\|testData" --include="*.ts" --include="*.tsx" --include="*.js" \ + --exclude="*.test.*" --exclude="*.spec.*" --exclude-dir="__tests__" src/ - grep -r "TODO.*real\|TODO.*database\|TODO.*API\|STUB\|MOCK" --include="*.ts" --include="*.tsx" --include="*.js" src/ + grep -r "TODO.*real\|TODO.*database\|TODO.*API\|STUB\|MOCK" --include="*.ts" --include="*.tsx" --include="*.js" \ + --exclude="*.test.*" --exclude="*.spec.*" --exclude-dir="__tests__" src/ - grep -r "isDevelopment\|isDev\|process\.env\.NODE_ENV.*development" --include="*.ts" --include="*.tsx" --include="*.js" src/ + grep -r "isDevelopment\|isDev\|process\.env\.NODE_ENV.*development" --include="*.ts" --include="*.tsx" --include="*.js" \ + --exclude="*.test.*" --exclude="*.spec.*" --exclude-dir="__tests__" src/ - grep -r "new Map\(\)\|new Set\(\)" --include="*.ts" --include="*.tsx" --include="*.js" src/ 2>/dev/null + grep -r "new Map\(\)\|new Set\(\)" --include="*.ts" --include="*.tsx" --include="*.js" \ + --exclude="*.test.*" --exclude="*.spec.*" --exclude-dir="__tests__" src/ 2>/dev/null.claude/templates/initializer_prompt.template.md (1)
219-228: Exclude test files in mock-data grep commands.These checks can be tripped by legitimate fixtures under
src/tests; excluding test patterns avoids unnecessary investigation.♻️ Suggested tweak
-1. Run: grep -r "globalThis\." --include="*.ts" --include="*.tsx" --include="*.js" src/ -2. Run: grep -r "dev-store\|devStore\|DevStore\|mock-db\|mockDb" --include="*.ts" --include="*.tsx" --include="*.js" src/ -3. Run: grep -r "mockData\|testData\|fakeData\|sampleData\|dummyData" --include="*.ts" --include="*.tsx" --include="*.js" src/ -4. Run: grep -r "TODO.*real\|TODO.*database\|TODO.*API\|STUB\|MOCK" --include="*.ts" --include="*.tsx" --include="*.js" src/ -5. Run: grep -r "isDevelopment\|isDev\|process\.env\.NODE_ENV.*development" --include="*.ts" --include="*.tsx" --include="*.js" src/ -6. Run: grep -r "new Map\(\)\|new Set\(\)" --include="*.ts" --include="*.tsx" --include="*.js" src/ 2>/dev/null +1. Run: grep -r "globalThis\." --include="*.ts" --include="*.tsx" --include="*.js" --exclude="*.test.*" --exclude="*.spec.*" --exclude-dir="__tests__" src/ +2. Run: grep -r "dev-store\|devStore\|DevStore\|mock-db\|mockDb" --include="*.ts" --include="*.tsx" --include="*.js" --exclude="*.test.*" --exclude="*.spec.*" --exclude-dir="__tests__" src/ +3. Run: grep -r "mockData\|testData\|fakeData\|sampleData\|dummyData" --include="*.ts" --include="*.tsx" --include="*.js" --exclude="*.test.*" --exclude="*.spec.*" --exclude-dir="__tests__" src/ +4. Run: grep -r "TODO.*real\|TODO.*database\|TODO.*API\|STUB\|MOCK" --include="*.ts" --include="*.tsx" --include="*.js" --exclude="*.test.*" --exclude="*.spec.*" --exclude-dir="__tests__" src/ +5. Run: grep -r "isDevelopment\|isDev\|process\.env\.NODE_ENV.*development" --include="*.ts" --include="*.tsx" --include="*.js" --exclude="*.test.*" --exclude="*.spec.*" --exclude-dir="__tests__" src/ +6. Run: grep -r "new Map\(\)\|new Set\(\)" --include="*.ts" --include="*.tsx" --include="*.js" --exclude="*.test.*" --exclude="*.spec.*" --exclude-dir="__tests__" src/ 2>/dev/nullapi/migrations.py (1)
152-155: Redundantconn.rollback()in exception handler.The
with conn.begin():context manager automatically rolls back on exception. The explicitrollback()is unnecessary and can cause confusion (or errors if called outside an active transaction).♻️ Proposed fix
except Exception as e: logger.error(f"Failed to migrate testing columns: {e}") - conn.rollback() raisedocker-compose.traefik.yml (1)
1-1: Consider removing deprecatedversionfield.The
versionkey is deprecated in Docker Compose V2 and ignored by newer versions. It can be safely removed for cleaner configuration.♻️ Proposed fix
-version: "3.9" - services:scripts/deploy.sh (1)
13-25: Address shellcheck warning SC2163 by using${var_name?}syntax.The
export "$var_name"pattern triggers shellcheck SC2163. Using${var_name?}silences the warning while maintaining the same behavior, and also adds a fail-safe ifvar_nameis unexpectedly empty.♻️ Proposed fix
prompt_required() { local var_name="$1" prompt_msg="$2" local value while true; do read -r -p "$prompt_msg: " value if [[ -n "$value" ]]; then printf -v "$var_name" '%s' "$value" - export "$var_name" + export "${var_name?}" return fi echo "Value cannot be empty." done }deploy.sh (1)
55-64: Address shellcheck warning SC2163.Same issue as in scripts/deploy.sh. The
export "$var_name"triggers SC2163.♻️ Proposed fix
while true; do read -r -p "${prompt_msg}: " value if [[ -n "${value}" ]]; then printf -v "${var_name}" "%s" "${value}" - export "${var_name}" + export "${var_name?}" return fi echo "Value cannot be empty." doneagent.py (2)
361-377: Missing logger guard on line 365.While
loggeris initialized inrun_autonomous_agent, this direct call at line 365 doesn't have a guard. For consistency with the pattern elsewhere and defensive coding, consider adding the guard.♻️ Proposed fix
if any(pattern in response_lower for pattern in RATE_LIMIT_PATTERNS): print("Claude Agent SDK indicated rate limit reached.") - logger.warning("Rate limit signal in response") + if logger: + logger.warning("Rate limit signal in response") reset_rate_limit_retries = False
410-415: Missing logger guard on line 414.Same issue - direct logger call without guard for consistency.
♻️ Proposed fix
except Exception as e: - logger.warning(f"Error parsing reset time: {e}, using default delay") + if logger: + logger.warning(f"Error parsing reset time: {e}, using default delay") print(f"Error parsing reset time: {e}, using default delay")server/routers/logs.py (2)
271-279: Sanitizeproject_namein export filename to prevent injection.The
project_nameis used directly in the filename construction without sanitization. Ifproject_namecontains special characters (though unlikely from registry), it could cause issues or be exploited.🔧 Proposed fix
# Create temp file for export suffix = f".{request.format}" if request.format != "jsonl" else ".jsonl" timestamp = datetime.now(timezone.utc).strftime("%Y%m%d_%H%M%S") - filename = f"logs_{request.project_name}_{timestamp}{suffix}" + # Sanitize project name for filename safety + safe_name = "".join(c for c in request.project_name if c.isalnum() or c in "-_") + filename = f"logs_{safe_name}_{timestamp}{suffix}"
28-32: Consider extracting_get_project_pathto a shared utility.This helper function is duplicated across multiple routers (logs.py, cicd.py, security.py, import_project.py). Consider moving it to a shared module like
server/utils/project.pyorserver/routers/common.pyto follow DRY principles.server/routers/cicd.py (2)
114-116: Consider logging skipped invalid workflow types.Invalid workflow types are silently skipped. Adding a debug/warning log would help troubleshoot when users expect a workflow that wasn't generated.
💡 Proposed enhancement
for wf_type in request.workflow_types: if wf_type not in ["ci", "security", "deploy"]: + logger.warning(f"Skipping unsupported workflow type: {wf_type}") continue
253-254: Avoid exposing internal error details in response.The error message includes
str(e)which could leak internal implementation details. Consider using a generic message like other endpoints in this file.🔧 Proposed fix
except Exception as e: - raise HTTPException(status_code=500, detail=f"Error reading workflow: {str(e)}") + logger.exception(f"Error reading workflow {filename}: {e}") + raise HTTPException(status_code=500, detail="Error reading workflow")server/routers/security.py (2)
137-139: Avoid exposing internal error details in security endpoints.The error response includes
str(e)which could leak sensitive implementation details. For a security-focused endpoint, prefer generic error messages.🔧 Proposed fix
except Exception as e: logger.exception(f"Error running security scan: {e}") - raise HTTPException(status_code=500, detail=f"Scan failed: {str(e)}") + raise HTTPException(status_code=500, detail="Security scan failed")
183-187: Same issue: avoid exposing error details.🔧 Proposed fix
try: with open(report_path) as f: return json.load(f) except Exception as e: - raise HTTPException(status_code=500, detail=f"Error reading report: {str(e)}") + logger.exception(f"Error reading report {filename}: {e}") + raise HTTPException(status_code=500, detail="Error reading report")server/routers/design_tokens.py (1)
351-354: Movereimport to module level for efficiency.The
remodule is imported insidevalidate_tokens, which is inefficient if called repeatedly. Move it to the module-level imports.🔧 Proposed fix
import logging from pathlib import Path +import re from typing import OptionalThen remove the import from line 352.
server/routers/import_project.py (3)
27-32: Avoidsys.pathmanipulation at module import time.Modifying
sys.pathduring module import is fragile and can cause import order issues. Consider restructuring imports to use relative imports or proper package installation.
285-319: Add transaction rollback on partial feature creation failure.If an exception occurs after some features are added but before
commit(), the session is closed without explicit rollback. While SQLAlchemy auto-rollbacks uncommitted transactions on close, explicit handling is clearer and safer.🔧 Proposed fix using context manager
try: - # Get starting priority - from sqlalchemy import func - max_priority = session.query(func.max(Feature.priority)).scalar() or 0 - - # Create features - created_count = 0 - for i, f in enumerate(request.features): - # Validate required fields - if not all(key in f for key in ["category", "name", "description", "steps"]): - logger.warning(f"Skipping feature missing required fields: {f}") - continue - - feature = Feature( - priority=max_priority + i + 1, - category=f["category"], - name=f["name"], - description=f["description"], - steps=f["steps"], - passes=False, - in_progress=False, - ) - session.add(feature) - created_count += 1 - - session.commit() - - return CreateFeaturesResponse( - created=created_count, - project_name=request.project_name, - message=f"Created {created_count} features for project '{request.project_name}'", - ) - - finally: - session.close() + with session.begin(): + from sqlalchemy import func + max_priority = session.query(func.max(Feature.priority)).scalar() or 0 + + created_count = 0 + for i, f in enumerate(request.features): + if not all(key in f for key in ["category", "name", "description", "steps"]): + logger.warning(f"Skipping feature missing required fields: {f}") + continue + + feature = Feature( + priority=max_priority + i + 1, + category=f["category"], + name=f["name"], + description=f["description"], + steps=f["steps"], + passes=False, + in_progress=False, + ) + session.add(feature) + created_count += 1 + + return CreateFeaturesResponse( + created=created_count, + project_name=request.project_name, + message=f"Created {created_count} features for project '{request.project_name}'", + ) + finally: + session.close()
286-288: Movesqlalchemy.funcimport to module level.Importing inside the function adds overhead on each call. Since SQLAlchemy is already a dependency, import it at the module level.
🔧 Proposed fix
Add to module-level imports:
from sqlalchemy import funcThen remove line 287.
server/routers/templates.py (1)
309-331: Consider adding a response_model for API consistency.The
get_template_featuresendpoint returns a dict but lacks aresponse_modeldeclaration, unlike the other endpoints in this router. This affects OpenAPI documentation completeness.🔧 Suggested improvement
+class TemplateFeaturesResponse(BaseModel): + """Response with template features.""" + template_id: str + features: list[dict] + count: int + by_category: dict[str, int] + -@router.get("/{template_id}/features") +@router.get("/{template_id}/features", response_model=TemplateFeaturesResponse) async def get_template_features(template_id: str):auto_documentation.py (3)
183-208: Specify encoding when reading JSON files for cross-platform compatibility.According to PEP 0597, "12% of the most popular packages on PyPI fail during installation on Windows" due to not specifying encoding when reading files. "It is a very common mistake that omits the encoding='utf-8' option" when working with text files on Windows.The
package_json.read_text()call on line 186 uses the platform default encoding, which on Windows could be something like cp936 or cp1252, causingUnicodeDecodeErrorif thepackage.jsoncontains non-ASCII characters.🔧 Specify UTF-8 encoding for file reads
package_json = self.project_dir / "package.json" if package_json.exists(): try: - data = json.loads(package_json.read_text()) + data = json.loads(package_json.read_text(encoding="utf-8")) deps = {**data.get("dependencies", {}), **data.get("devDependencies", {})}Apply the same fix to line 468 in
_extract_scripts():if package_json.exists(): try: - data = json.loads(package_json.read_text()) + data = json.loads(package_json.read_text(encoding="utf-8")) scripts = data.get("scripts", {})
573-578: Specify UTF-8 encoding when writing documentation files.The
write_text()calls lack explicit encoding, which uses platform defaults. Since documentation files are typically UTF-8 and may contain non-ASCII characters (e.g., in project names or descriptions), this can cause issues on Windows.🔧 Add encoding to all write_text calls
# Line 577 - readme_path.write_text("\n".join(lines)) + readme_path.write_text("\n".join(lines), encoding="utf-8") # Line 625 - api_docs_path.write_text("\n".join(lines)) + api_docs_path.write_text("\n".join(lines), encoding="utf-8") # Line 692 - setup_path.write_text("\n".join(lines)) + setup_path.write_text("\n".join(lines), encoding="utf-8")
280-297: Consider handling import failure more gracefully.The dynamic import from
api.databaseis wrapped in try/except which is good, but the warning message could be more informative for debugging. Also, ensure the session is closed even if iteration fails.🔧 Improved error handling
if db_path.exists(): + session = None try: from api.database import Feature, get_session session = get_session(db_path) db_features = session.query(Feature).order_by(Feature.priority).all() for f in db_features: features.append( { "category": f.category, "name": f.name, "description": f.description, "status": "completed" if f.passes else "pending", } ) - session.close() except Exception as e: - logger.warning(f"Error reading features.db: {e}") + logger.warning(f"Error reading features from {db_path}: {e}") + finally: + if session is not None: + session.close()server/routers/documentation.py (1)
215-223: Minor: Use idiomatic string conversion.
stat.st_mtime.__str__()works but is unconventional. Usingstr(stat.st_mtime)is more idiomatic Python.🔧 Use str() function
- modified=stat.st_mtime.__str__(), + modified=str(stat.st_mtime),Apply to lines 221, 235.
integrations/ci/github_actions.py (2)
83-92: Specify UTF-8 encoding when writing workflow files.The
savemethod opens files without encoding, which uses platform defaults. YAML workflow files should be UTF-8 encoded for consistency across platforms.🔧 Add encoding parameter
def save(self, project_dir: Path) -> Path: """Save workflow to .github/workflows directory.""" workflows_dir = project_dir / ".github" / "workflows" workflows_dir.mkdir(parents=True, exist_ok=True) output_path = workflows_dir / self.filename - with open(output_path, "w") as f: + with open(output_path, "w", encoding="utf-8") as f: f.write(self.to_yaml()) return output_path
115-144: Specify encoding when reading package.json and handle version parsing edge cases.
- The
open(package_json)call lacks encoding, which can fail on Windows with non-ASCII content.- The version parsing
version.strip(">=^~")is simplistic and may produce unexpected results for complex version specs like>=18.0.0 || ^20.🔧 Proposed improvements
try: - with open(package_json) as f: + with open(package_json, encoding="utf-8") as f: pkg = json.load(f) deps = {**pkg.get("dependencies", {}), **pkg.get("devDependencies", {})} # ... dependency checks ... # Node version from engines engines = pkg.get("engines", {}) if "node" in engines: - version = engines["node"].strip(">=^~") - if version and version[0].isdigit(): - stack["node_version"] = version.split(".")[0] + # Extract first numeric version from potentially complex spec + import re + match = re.search(r'(\d+)', engines["node"]) + if match: + stack["node_version"] = match.group(1)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@deploy.sh`:
- Around line 147-162: The OS detection logic misclassifies Ubuntu as Debian
because it checks ID_LIKE for "debian" before checking ID explicitly; update
deploy.sh to first check for explicit Ubuntu (e.g., if [[ "$OS_ID" == "ubuntu"
|| "$ID_LIKE" == *"ubuntu"* ]]) and set DOCKER_DIST="ubuntu", then check for
Debian (OS_ID=="debian" or ID_LIKE contains "debian") to set
DOCKER_DIST="debian", and finally add a safe fallback branch (e.g.,
DOCKER_DIST="unknown" or default to ubuntu with a warning) so Docker repo setup
uses the correct distribution; reference variables OS_ID, OS_LIKE and
DOCKER_DIST and adjust the conditional order accordingly.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@deploy.sh`:
- Around line 92-141: After assigning APP_PORT (and after
APP_PORT=${APP_PORT:-8888}), validate it is numeric and within 1–65535;
implement a loop that, when AUTOMATED_MODE is 0, re-prompts read -r -p for
APP_PORT until it matches a numeric regex and the integer comparison confirms
1<=APP_PORT<=65535, and when AUTOMATED_MODE is 1 either fall back to the default
8888 or exit with a clear error message; update any subsequent logic that
assumes APP_PORT is valid so deploy fails early with a clear message if
validation fails.
- Around line 143-183: In ensure_packages(), the logic gates creation of the
Docker repository list behind the presence of the GPG key so if the key exists
but the repo list is missing it never gets created; separate the checks so you:
1) check/create the GPG key (using DOCKER_DIST and gpg --dearmor as currently
done) if missing, 2) independently check/create the Docker repository list
(using VERSION_CODENAME and dpkg --print-architecture) if missing or differs,
and 3) only run apt-get update when you actually created/changed the repository
list; keep the existing permission steps and systemctl enable --now docker call.
In `@server/routers/git_workflow.py`:
- Around line 27-30: The relative import in _get_project_path is incorrect and
will raise ModuleNotFoundError; update the import used inside function
_get_project_path to import get_project_path from the repository-root module
(use "from registry import get_project_path") instead of "from ..registry import
get_project_path" so it matches other routers and resolves at runtime.
🧹 Nitpick comments (3)
server/routers/git_workflow.py (2)
38-44: Consider adding validation forfeature_nameused in branch naming.The
feature_namefield is used to construct branch names (e.g.,feature/42-user-can-login). Git branch names have restrictions—no spaces, no special characters like~,^,:,?,*,[, or backslash. Adding validation here provides defense-in-depth.♻️ Suggested validation pattern
+import re + +def validate_branch_safe_name(value: str) -> str: + """Validate that the name is safe for use in git branch names.""" + if not re.match(r'^[a-zA-Z0-9][a-zA-Z0-9_-]*$', value): + raise ValueError('Feature name must be alphanumeric with dashes/underscores only') + return value + class StartFeatureRequest(BaseModel): """Request to start a feature branch.""" project_name: str = Field(..., description="Name of the registered project") feature_id: int = Field(..., description="Feature ID") - feature_name: str = Field(..., description="Feature name for branch naming") + feature_name: str = Field( + ..., + description="Feature name for branch naming", + min_length=1, + max_length=50, + ) + + `@validator`('feature_name') + def validate_feature_name(cls, v): + return validate_branch_safe_name(v)
225-282: Consider consolidating the repeated lazy import.The
from git_workflow import get_workflowimport is repeated in all 6 endpoint functions. While this works, consolidating it would reduce duplication.♻️ Optional: Extract workflow retrieval to helper
+def _get_workflow(project_dir: Path): + """Get git workflow for a project directory.""" + from git_workflow import get_workflow + return get_workflow(project_dir) + + `@router.post`("/commit", response_model=WorkflowResultResponse) async def commit_changes(request: CommitRequest): # ... try: - from git_workflow import get_workflow - - workflow = get_workflow(project_dir) + workflow = _get_workflow(project_dir) result = workflow.commit_feature_progress(request.feature_id, request.message)Otherwise, the endpoints are well-implemented with consistent error handling and proper response model mapping.
deploy.sh (1)
259-268: Harden .env.deploy permissions.
Even if it currently holds non-sensitive values, this file often grows to include secrets later.🔒 Suggested hardening
cat > "${APP_DIR}/.env.deploy" <<EOF DOMAIN=${DOMAIN} LETSENCRYPT_EMAIL=${LETSENCRYPT_EMAIL} APP_PORT=${APP_PORT} EOF +chmod 600 "${APP_DIR}/.env.deploy"
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@deploy.sh`:
- Around line 374-389: In print_notes(), the troubleshooting curl line is
hardcoded to port 8888 which can be wrong if APP_PORT is configured; update the
curl command in print_notes() to reference the APP_PORT variable (with a
sensible default fallback) instead of the literal 8888 so the printed guidance
matches the actual configured port (locate the curl call inside the print_notes
function and replace the hardcoded port with a reference to APP_PORT).
- Around line 391-396: The current run order calls clone_repo before
preserve_env_file, risking the production .env being overwritten by a tracked
file in the repo; move the preserve_env_file call to execute before clone_repo
so the existing production .env is backed up prior to any git clone/pull
operations (i.e., reorder the sequence that contains ensure_packages,
configure_duckdns, clone_repo, preserve_env_file, assert_compose_files,
write_env to ensure preserve_env_file runs immediately before clone_repo or at
the start of the sequence).
🧹 Nitpick comments (4)
deploy.sh (2)
55-61: Silence shellcheck SC2163 warning for consistency.Line 46 uses
"${var_name?}"to silence the warning, but line 59 uses"${var_name}". Use the same pattern for consistency and to satisfy the static analysis tool.🔧 Suggested fix
while true; do read -r -p "${prompt_msg}: " value if [[ -n "${value}" ]]; then printf -v "${var_name}" "%s" "${value}" - export "${var_name}" + export "${var_name?}" return fi
199-203: Consider atomic write for cron file containing sensitive token.There's a brief window between
cat > "${cron_file}"andchmod 600where the file may have default permissions (typically 644), exposing the DuckDNS token momentarily. While/etc/cron.dis root-only, using umask or writing to a temp file first would be more defensive.🔧 Suggested atomic write pattern
configure_duckdns() { echo echo "==> Configuring DuckDNS..." local cron_file="/etc/cron.d/duckdns" + local cron_tmp + cron_tmp=$(mktemp) + chmod 600 "${cron_tmp}" - cat > "${cron_file}" <<EOF + cat > "${cron_tmp}" <<EOF */5 * * * * root curl -fsS "https://www.duckdns.org/update?domains=${DUCKDNS_SUBDOMAIN}&token=${DUCKDNS_TOKEN}&ip=" >/var/log/duckdns.log 2>&1 EOF - chmod 600 "${cron_file}" + mv "${cron_tmp}" "${cron_file}"server/routers/git_workflow.py (2)
78-89: Usedefault_factoryfor list fields to avoid shared mutable defaults.
feature_branches: list[str] = []is non-idiomatic for Pydantic models.♻️ Proposed tweak
class GitStatusResponse(BaseModel): """Response with git status information.""" @@ - feature_branches: list[str] = [] + feature_branches: list[str] = Field(default_factory=list) feature_branch_count: int = 0
113-135: Run git operations off the event loop and avoid leaking raw error text.Blocking git operations via
subprocess.run()are called directly in async handlers, which can stall the event loop. Additionally, raw exception text in error responses exposes internal details across multiple endpoints.♻️ Proposed pattern (apply similarly to other endpoints)
-from fastapi import APIRouter, HTTPException +from fastapi import APIRouter, HTTPException +from fastapi.concurrency import run_in_threadpool @@ - workflow = get_workflow(project_dir) - status = workflow.get_status() + workflow = await run_in_threadpool(get_workflow, project_dir) + status = await run_in_threadpool(workflow.get_status) @@ - raise HTTPException(status_code=500, detail=f"Failed to get status: {str(e)}") + raise HTTPException(status_code=500, detail="Failed to get status")
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@analyzers/react_analyzer.py`:
- Around line 176-215: The extractor currently only globs page.tsx and page.jsx
so JS-only Next/React apps are missed; update _extract_app_router_routes (and
the other similar route-extraction blocks) to include .js pages by iterating a
list of extensions (e.g., ["page.tsx","page.jsx","page.js"]) or adding rglob for
"page.js" (and optionally "page.mjs"/"page.cjs") and deduplicate the shared
logic for building route_path and appending RouteInfo so you don't copy/paste
the same block; ensure the "file" uses page_file.relative_to(self.project_dir)
and that dynamic route substitutions and cleanup remain identical for the new
extensions.
- Around line 300-313: In _parse_api_route, avoid using str(rel_path) which
yields backslashes on Windows; instead build route_path from
rel_path.with_suffix("").as_posix() (or call as_posix() on the intermediate
string) so the subsequent replace and regex operate on forward-slash URLs, and
also normalize the file path by using
api_file.relative_to(self.project_dir).as_posix() when populating the "file"
field.
In `@api/migrations.py`:
- Around line 154-157: The except block currently calls conn.rollback() after
leaving the with conn.begin() context which is redundant and can raise
InvalidRequestError; remove the conn.rollback() call from the except handler in
the migration function so the rollback is handled by the with conn.begin()
context, keeping the logger.error(f"Failed to migrate testing columns: {e}") and
the raise to propagate the exception.
In `@scripts/deploy.sh`:
- Around line 13-25: The export call in function prompt_required uses an
incorrect dynamic export (export "$var_name") which triggers SC2163; replace
that with export "${var_name?}" so the variable name is validated and exported
correctly after assigning via printf -v in prompt_required, keeping the rest of
the loop and read logic unchanged.
- Around line 34-38: The code assigns DUCKDNS_SUBDOMAIN to the full DOMAIN when
DOMAIN does not end with ".duckdns.org", which will break DuckDNS API calls;
update the check around DOMAIN and DUCKDNS_SUBDOMAIN to validate the format and
fail fast: if DOMAIN ends with ".duckdns.org" set
DUCKDNS_SUBDOMAIN="${DOMAIN%.duckdns.org}", otherwise print a clear
error/warning including the invalid DOMAIN and exit with non-zero status (or
prompt for correct input) so the script does not proceed with an invalid DuckDNS
subdomain.
In `@server/routers/cicd.py`:
- Around line 162-170: The endpoint currently calls
_get_project_path(request.project_name) and only checks falsy project_dir but
doesn't verify the filesystem exists, which can cause confusing errors; update
the handler to call project_dir = _get_project_path(...) then verify project_dir
and project_dir.exists() (or equivalent Path.exists()) and raise
HTTPException(status_code=404, detail="Project not found") if either check fails
before proceeding to validate request.workflow_type and calling the workflow
generator (reference symbols: _get_project_path and project_dir).
- Around line 255-256: The except block currently raises
HTTPException(status_code=500, detail=f"Error reading workflow: {str(e)}") which
leaks internal error details; change it to log the full exception internally
(e.g., logger.exception or similar) and raise a generic HTTPException with a
non-sensitive message like "Internal server error while reading workflow" (keep
HTTPException and status_code=500 but remove str(e) from detail), ensuring the
variable e is only used for internal logging not returned to the client.
In `@server/routers/import_project.py`:
- Around line 209-211: The exception handlers currently include internal error
strings in HTTP responses (e.g., in the analyze_project endpoint) — change each
handler to log full details with logger.exception(...) but raise HTTPException
with a generic message (e.g., "Analysis failed" or "Internal server error")
instead of including str(e); update the handlers for analyze_project,
extract_features, create_features, and quick_detect so they all follow this
pattern (log the exception, then raise a 500 HTTPException with a non‑revealing
detail message).
🧹 Nitpick comments (9)
scripts/deploy.sh (1)
97-102: Consider quoting variables in URLs for robustness.While unlikely to cause issues here, quoting
$DOCKER_DISTand$VERSION_CODENAMEis a good defensive practice.♻️ Proposed fix
- curl -fsSL https://download.docker.com/linux/$DOCKER_DIST/gpg | gpg --dearmor -o /etc/apt/keyrings/docker.gpg + curl -fsSL "https://download.docker.com/linux/${DOCKER_DIST}/gpg" | gpg --dearmor -o /etc/apt/keyrings/docker.gpg chmod a+r /etc/apt/keyrings/docker.gpg echo \ - "deb [arch=$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/docker.gpg] https://download.docker.com/linux/$DOCKER_DIST \ - $(. /etc/os-release && echo "$VERSION_CODENAME") stable" > /etc/apt/sources.list.d/docker.list + "deb [arch=$(dpkg --print-architecture) signed-by=/etc/apt/keyrings/docker.gpg] https://download.docker.com/linux/${DOCKER_DIST} \ + $(. /etc/os-release && echo "${VERSION_CODENAME}") stable" > /etc/apt/sources.list.d/docker.listserver/routers/cicd.py (2)
25-30: Consider extracting_get_project_pathto a shared utility module.This helper function is duplicated across multiple routers (
import_project.py,git_workflow.py,logs.py,security.py). Extracting it to a common utility would reduce duplication and centralize any future changes.♻️ Suggested approach
Create a shared utility, e.g.,
server/routers/_utils.py:from pathlib import Path def get_project_path(project_name: str) -> Path | None: """Get project path from registry.""" from registry import get_project_path as registry_get_project_path return registry_get_project_path(project_name)Then import from this shared module in each router.
114-116: Consider informing users about skipped invalid workflow types.Invalid workflow types are silently skipped. Users requesting
["ci", "invalid_type"]would only receive the CI workflow without knowinginvalid_typewas ignored.💡 Suggested approach
Either validate upfront and return a 400 error, or include skipped types in the response:
class GenerateResponse(BaseModel): """Response from workflow generation.""" provider: str workflows: list[WorkflowInfo] output_dir: str message: str + skipped_types: list[str] = []server/routers/import_project.py (4)
30-32: Avoid runtimesys.pathmanipulation.Modifying
sys.pathat import time is fragile and can cause subtle import issues. Consider configuring the Python path viaPYTHONPATHenvironment variable or package installation instead.
145-149: Consider using a typed model for features input.Using
list[dict]for thefeaturesfield loses type safety and makes the API contract less clear. A dedicated Pydantic model would provide validation at deserialization time.💡 Suggested approach
class FeatureInput(BaseModel): """Input model for creating a feature.""" category: str name: str description: str steps: list[str] class CreateFeaturesRequest(BaseModel): """Request to create features in the database.""" project_name: str = Field(..., description="Name of the registered project") features: list[FeatureInput] = Field(..., description="Features to create")
333-340: Consider using POST for path-based operations.Passing filesystem paths as GET query parameters could expose sensitive path information in server logs, browser history, and referrer headers. A POST request with a body would be more appropriate for this operation.
💡 Suggested approach
-@router.get("/quick-detect") -async def quick_detect(path: str): +class QuickDetectRequest(BaseModel): + """Request for quick stack detection.""" + path: str = Field(..., description="Absolute path to the project directory") + +@router.post("/quick-detect") +async def quick_detect(request: QuickDetectRequest): """ Quick detection endpoint for UI preview. ... """ - if not validate_path(path): + if not validate_path(request.path): raise HTTPException(status_code=400, detail="Invalid path") - project_dir = Path(path).resolve() + project_dir = Path(request.path).resolve()
289-326: Use a context manager for database session management.The current manual
try/finallypattern works but lacks explicit rollback on errors. SQLAlchemy 2.0 Sessions support the context manager protocol, which provides automatic rollback on exception and cleaner code.♻️ Proposed fix
# Initialize database engine, SessionLocal = create_database(project_dir) - session = SessionLocal() - - try: + with SessionLocal() as session: # Get starting priority from sqlalchemy import func max_priority = session.query(func.max(Feature.priority)).scalar() or 0 # Create features created_count = 0 for i, f in enumerate(request.features): # Validate required fields if not all(key in f for key in ["category", "name", "description", "steps"]): logger.warning(f"Skipping feature missing required fields: {f}") continue feature = Feature( priority=max_priority + i + 1, category=f["category"], name=f["name"], description=f["description"], steps=f["steps"], passes=False, in_progress=False, ) session.add(feature) created_count += 1 session.commit() return CreateFeaturesResponse( created=created_count, project_name=request.project_name, message=f"Created {created_count} features for project '{request.project_name}'", ) - - finally: - session.close()analyzers/node_analyzer.py (2)
103-172: Ensure analyze() initializes detection when called directly.
Ifcan_analyze()isn’t invoked upstream,_detected_stackstays at the default and confidence falls back to 0.85, which can skew endpoint extraction. Consider initializing detection insideanalyze()when confidence is missing.♻️ Proposed refactor
def analyze(self) -> AnalysisResult: """Analyze the Node.js project.""" + # Ensure stack detection/confidence are initialized if analyze() is called directly + if self._detection_confidence is None: + self.can_analyze() + routes: list[RouteInfo] = [] components: list[ComponentInfo] = [] endpoints: list[EndpointInfo] = []
194-236: De-duplicate route files to avoid duplicate endpoints.
Overlapping glob patterns plus explicit main files can cause repeated entries inroute_files, which then duplicate endpoints in results.♻️ Proposed refactor
# Also check main files for main_file in ["app.js", "app.ts", "server.js", "server.ts", "src/app.js", "src/app.ts", "index.js", "index.ts"]: main_path = self.project_dir / main_file if main_path.exists(): route_files.append(main_path) + + # De-duplicate to avoid repeated endpoints + route_files = list(dict.fromkeys(route_files))
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@analyzers/react_analyzer.py`:
- Around line 360-370: The current method detection in react_analyzer.py only
checks for "export function" patterns and misses arrow-exported handlers; update
the detection (after reading content via _read_file_safe(route_file)) to also
match patterns like "export const|let|var {METHOD} = (async )?(" and "export
const|let|var {METHOD} = (async )?req =>" using a regex test per METHOD (e.g.,
r"export\s+(?:const|let|var)\s+{method}\s*=\s*(?:async\s*)?\(" or r"=>") and
append the method to methods when matched; keep existing "export function"
checks (the methods list and fallback to ["ALL"] remain the same so only expand
detection logic around methods.append).
In `@api/migrations.py`:
- Around line 91-105: The rebuild currently collects optional columns from
columns.items() and only preserves the column type when building
optional_col_defs and optional_col_names; update this to also read
col_info["notnull"] and col_info.get("dflt_value") for each optional column and
append the appropriate SQL constraints (add "NOT NULL" when notnull is truthy
and "DEFAULT <value>" when dflt_value is not None) to the column definition
string you build (the logic that fills optional_columns and constructs
optional_col_defs); ensure DEFAULT values are rendered as the original PRAGMA
dflt_value token (preserving quotes or expressions) and skip adding constraints
when those pragma fields are absent.
In `@deploy.sh`:
- Around line 66-75: derive_duuckdns_subdomain currently exports whatever string
DOMAIN yields and can inject into the cron file and curl calls; validate and
reject/sanitize the derived value in derive_duckdns_subdomain by enforcing
DuckDNS rules (only alphanumeric and hyphens, no leading/trailing hyphen, length
1–63) using a single regex check against DUCKDNS_SUBDOMAIN, and if it fails,
print a clear error and exit non‑zero; ensure the exported DUCKDNS_SUBDOMAIN is
the validated value so subsequent uses (the cron entry generator and the
immediate curl invocation) only ever receive a safe, validated subdomain.
In `@server/routers/cicd.py`:
- Around line 237-250: The endpoint currently calls
_get_project_path(project_name) but doesn't explicitly check the project's
directory existence like list_workflows does; add a clear existence check after
obtaining project_dir (e.g., verify project_dir and project_dir.is_dir()) and
raise HTTPException(status_code=404, detail="Project not found") if it doesn't
exist before performing filename validation and constructing workflow_path;
reference the variables project_dir and workflow_path and the helper
_get_project_path so you update the same endpoint to mirror list_workflows'
directory existence validation.
- Around line 200-206: The list_workflows endpoint uses _get_project_path to
obtain project_dir but doesn't check that the directory actually exists before
accessing workflows_dir; update the list_workflows implementation to call
project_dir.exists() after project_dir is returned and, if false, raise
HTTPException(status_code=404, detail="Project not found") so that workflows_dir
= project_dir / ".github" / "workflows" is only accessed for existing
directories; reference project_dir, _get_project_path, workflows_dir,
list_workflows, and WorkflowListResponse when making the change.
🧹 Nitpick comments (6)
server/routers/cicd.py (1)
114-116: Consider notifying callers when workflow types are skipped.Invalid workflow types are silently skipped without any indication to the caller. If a user requests
["ci", "invalid_type"], they'll receive only one workflow with no explanation thatinvalid_typewas ignored.💡 Suggested improvement
+ skipped_types = [] for wf_type in request.workflow_types: if wf_type not in ["ci", "security", "deploy"]: + skipped_types.append(wf_type) continue # ... rest of loop + message = f"Generated {len(workflows)} workflow(s)" + if skipped_types: + message += f" (skipped invalid types: {skipped_types})" + return GenerateResponse( provider="github", workflows=workflows, output_dir=str(project_dir / ".github" / "workflows"), - message=f"Generated {len(workflows)} workflow(s)", + message=message, )server/routers/import_project.py (2)
27-32: Avoid modifyingsys.pathat module load time.Inserting into
sys.pathat import time is a side effect that can cause import inconsistencies across the codebase. Other routers (e.g.,cicd.py) use local imports without path manipulation.💡 Suggested improvement
Remove the
sys.pathmanipulation and ensure imports work via proper package structure or by running from the correct working directory:-# Root directory -ROOT_DIR = Path(__file__).parent.parent.parent - -# Add root to path for imports -if str(ROOT_DIR) not in sys.path: - sys.path.insert(0, str(ROOT_DIR))The imports from
registry,analyzers, andapi.databaseshould work if the server is run from the project root, which is the standard setup.
145-149: Consider using a typed model for feature input.Using
list[dict]for thefeaturesfield loses type safety. A dedicatedFeatureInputmodel would provide validation and better API documentation.💡 Suggested improvement
+class FeatureInput(BaseModel): + """Feature input for creation.""" + category: str + name: str + description: str + steps: list[str] + + class CreateFeaturesRequest(BaseModel): """Request to create features in the database.""" project_name: str = Field(..., description="Name of the registered project") - features: list[dict] = Field(..., description="Features to create (category, name, description, steps)") + features: list[FeatureInput] = Field(..., description="Features to create")This would eliminate the need for manual field validation at lines 301-303.
analyzers/react_analyzer.py (2)
176-231: Consider consolidating the extension loops to reduce duplication.The three loops for
.tsx,.jsx, and.jsare nearly identical. Iterating over extensions would improve maintainability.♻️ Suggested refactor
def _extract_app_router_routes(self, app_dir: Path) -> list[RouteInfo]: """Extract routes from Next.js App Router.""" routes: list[RouteInfo] = [] - for page_file in app_dir.rglob("page.tsx"): - rel_path = page_file.relative_to(app_dir) - route_path = "/" + "/".join(rel_path.parent.parts) - - # Handle dynamic routes: [id] -> :id - route_path = re.sub(r"\[([^\]]+)\]", r":\1", route_path) - - # Clean up - if route_path == "/.": - route_path = "/" - route_path = route_path.replace("//", "/") - - routes.append({ - "path": route_path, - "method": "GET", - "handler": "Page", - "file": str(page_file.relative_to(self.project_dir)), - }) - - # Also check .jsx files - for page_file in app_dir.rglob("page.jsx"): - rel_path = page_file.relative_to(app_dir) - route_path = "/" + "/".join(rel_path.parent.parts) - route_path = re.sub(r"\[([^\]]+)\]", r":\1", route_path) - if route_path == "/.": - route_path = "/" - route_path = route_path.replace("//", "/") - - routes.append({ - "path": route_path, - "method": "GET", - "handler": "Page", - "file": str(page_file.relative_to(self.project_dir)), - }) - - # Also check .js files - for page_file in app_dir.rglob("page.js"): - rel_path = page_file.relative_to(app_dir) - route_path = "/" + "/".join(rel_path.parent.parts) - route_path = re.sub(r"\[([^\]]+)\]", r":\1", route_path) - if route_path == "/.": - route_path = "/" - route_path = route_path.replace("//", "/") - - routes.append({ - "path": route_path, - "method": "GET", - "handler": "Page", - "file": str(page_file.relative_to(self.project_dir)), - }) + for ext in ["page.tsx", "page.jsx", "page.js"]: + for page_file in app_dir.rglob(ext): + rel_path = page_file.relative_to(app_dir) + route_path = "/" + "/".join(rel_path.parent.parts) + + # Handle dynamic routes: [id] -> :id + route_path = re.sub(r"\[([^\]]+)\]", r":\1", route_path) + + # Clean up + if route_path == "/.": + route_path = "/" + route_path = route_path.replace("//", "/") + + routes.append({ + "path": route_path, + "method": "GET", + "handler": "Page", + "file": str(page_file.relative_to(self.project_dir)), + }) return routes
233-303: Same refactoring opportunity applies here.The three extension loops could be consolidated similarly to reduce code duplication.
deploy.sh (1)
55-63: Address shellcheck SC2163 and maintain consistency with line 46.Line 59 uses
export "${var_name}"while line 46 usesexport "${var_name?}". The${var?}form explicitly errors if the variable is unset and silences the shellcheck warning.🔧 Suggested fix
if [[ -n "${value}" ]]; then printf -v "${var_name}" "%s" "${value}" - export "${var_name}" + export "${var_name?}" return fi
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@analyzers/react_analyzer.py`:
- Around line 37-45: When reading package.json in ReactAnalyzer (the block that
assigns package_json = self.project_dir / "package.json" and later the similar
read at lines ~98-107), ensure you call read_text(encoding="utf-8") and wrap the
json.loads(read_text...) in a try/except that catches UnicodeDecodeError (and
logs or skips gracefully) in addition to the existing JSON decode handling;
update both places where package.json is read so a non-UTF8 file doesn't raise
and abort analysis, but is handled cleanly by the methods in react_analyzer.py.
In `@api/migrations.py`:
- Around line 128-133: The INSERT ... SELECT assumes core columns exist; update
the migration that builds insert_sql to construct the SELECT list dynamically
instead of hardcoding dependencies and last_tested_at: inspect the current
features table schema (or existing_col_names) and create a select_cols string
that always includes id, priority, category, name, description, steps, passes,
in_progress, testing_in_progress but for each of 'dependencies' and
'last_tested_at' append the real column name if present or the placeholder "NULL
AS dependencies"/"NULL AS last_tested_at" when missing, then use that
select_cols when building insert_sql to INSERT INTO features_new SELECT
{select_cols} FROM features (keep optional_col_names logic consistent).
In `@server/routers/cicd.py`:
- Around line 114-116: Request handling currently silently skips invalid entries
in request.workflow_types during the loop (wf_type), causing silent failures;
change this to validate upfront by computing invalid_types = [t for t in
request.workflow_types if t not in {"ci","security","deploy"}] and if any exist
return a 400 error (e.g., raise HTTPException or return a JSONResponse) with a
clear message listing the invalid types and accepted values; update the handler
in cicd.py where request.workflow_types is used to perform this check before
processing workflows.
🧹 Nitpick comments (5)
server/routers/cicd.py (2)
25-29: Consider extracting_get_project_pathto a shared utility.This helper function is duplicated across multiple routers (
import_project.py,git_workflow.py, and now here). Extracting it to a shared module (e.g.,server/routers/utils.pyorserver/utils.py) would reduce duplication and centralize the registry lookup logic.
235-236: Consider adding a response model for consistency.This endpoint returns a dict without a
response_model, unlike the other endpoints in this file. Adding a response model would improve OpenAPI documentation consistency and provide response validation.♻️ Proposed fix
Add a response model:
class WorkflowContentResponse(BaseModel): """Response with workflow file content.""" filename: str content: strThen update the endpoint:
-@router.get("/workflows/{project_name}/{filename}") +@router.get("/workflows/{project_name}/{filename}", response_model=WorkflowContentResponse) async def get_workflow_content(project_name: str, filename: str):And update the return statement:
- return { - "filename": filename, - "content": content, - } + return WorkflowContentResponse( + filename=filename, + content=content, + )deploy.sh (2)
168-175: Fail fast on unsupported OS rather than falling through.The script assumes
apt-getand Debian/Ubuntu Docker repos. For non‑Debian/Ubuntu distros, a clear error is better than a cryptic failure later.🛠️ Proposed fix
if [[ "$OS_ID" == "ubuntu" || "$OS_LIKE" == *"ubuntu"* ]]; then DOCKER_DIST="ubuntu" elif [[ "$OS_ID" == "debian" || "$OS_LIKE" == *"debian"* ]]; then DOCKER_DIST="debian" else - DOCKER_DIST="$OS_ID" + echo "Unsupported OS '${OS_ID}'. This script supports Debian/Ubuntu-based distros only." >&2 + exit 1 fi
277-285: Harden permissions on.env.deploy.Even though it’s not highly sensitive today, restricting it to owner-only avoids accidental exposure and aligns with how
.envbackups are treated.🛠️ Proposed fix
cat > "${APP_DIR}/.env.deploy" <<EOF DOMAIN=${DOMAIN} LETSENCRYPT_EMAIL=${LETSENCRYPT_EMAIL} APP_PORT=${APP_PORT} EOF + chmod 600 "${APP_DIR}/.env.deploy" || true echo "DuckDNS token stored in /etc/cron.d/duckdns (not in repo)."analyzers/react_analyzer.py (1)
176-231: Consider iterating over page extensions to reduce duplication.
This keeps the routing logic single‑sourced and easier to extend.♻️ Proposed refactor
- for page_file in app_dir.rglob("page.tsx"): - rel_path = page_file.relative_to(app_dir) - route_path = "/" + "/".join(rel_path.parent.parts) - - # Handle dynamic routes: [id] -> :id - route_path = re.sub(r"\[([^\]]+)\]", r":\1", route_path) - - # Clean up - if route_path == "/.": - route_path = "/" - route_path = route_path.replace("//", "/") - - routes.append({ - "path": route_path, - "method": "GET", - "handler": "Page", - "file": str(page_file.relative_to(self.project_dir)), - }) - - # Also check .jsx files - for page_file in app_dir.rglob("page.jsx"): - rel_path = page_file.relative_to(app_dir) - route_path = "/" + "/".join(rel_path.parent.parts) - route_path = re.sub(r"\[([^\]]+)\]", r":\1", route_path) - if route_path == "/.": - route_path = "/" - route_path = route_path.replace("//", "/") - - routes.append({ - "path": route_path, - "method": "GET", - "handler": "Page", - "file": str(page_file.relative_to(self.project_dir)), - }) - - # Also check .js files - for page_file in app_dir.rglob("page.js"): - rel_path = page_file.relative_to(app_dir) - route_path = "/" + "/".join(rel_path.parent.parts) - route_path = re.sub(r"\[([^\]]+)\]", r":\1", route_path) - if route_path == "/.": - route_path = "/" - route_path = route_path.replace("//", "/") - - routes.append({ - "path": route_path, - "method": "GET", - "handler": "Page", - "file": str(page_file.relative_to(self.project_dir)), - }) + for pattern in ("page.tsx", "page.jsx", "page.js"): + for page_file in app_dir.rglob(pattern): + rel_path = page_file.relative_to(app_dir) + route_path = "/" + "/".join(rel_path.parent.parts) + + # Handle dynamic routes: [id] -> :id + route_path = re.sub(r"\[([^\]]+)\]", r":\1", route_path) + + # Clean up + if route_path == "/.": + route_path = "/" + route_path = route_path.replace("//", "/") + + routes.append({ + "path": route_path, + "method": "GET", + "handler": "Page", + "file": str(page_file.relative_to(self.project_dir)), + })
| # Check package.json | ||
| package_json = self.project_dir / "package.json" | ||
| if package_json.exists(): | ||
| try: | ||
| data = json.loads(package_json.read_text()) | ||
| deps = { | ||
| **data.get("dependencies", {}), | ||
| **data.get("devDependencies", {}), | ||
| } |
There was a problem hiding this comment.
Handle UnicodeDecodeError when reading package.json.
A non‑UTF8 package.json can currently raise and abort detection/analysis. Consider specifying UTF‑8 and catching UnicodeDecodeError in both reads.
🛠️ Proposed fix
- data = json.loads(package_json.read_text())
+ data = json.loads(package_json.read_text(encoding="utf-8"))
@@
- except (json.JSONDecodeError, OSError):
+ except (json.JSONDecodeError, OSError, UnicodeDecodeError):
pass
@@
- data = json.loads(package_json.read_text())
+ data = json.loads(package_json.read_text(encoding="utf-8"))
@@
- except (json.JSONDecodeError, OSError):
+ except (json.JSONDecodeError, OSError, UnicodeDecodeError):
passAlso applies to: 98-107
🤖 Prompt for AI Agents
In `@analyzers/react_analyzer.py` around lines 37 - 45, When reading package.json
in ReactAnalyzer (the block that assigns package_json = self.project_dir /
"package.json" and later the similar read at lines ~98-107), ensure you call
read_text(encoding="utf-8") and wrap the json.loads(read_text...) in a
try/except that catches UnicodeDecodeError (and logs or skips gracefully) in
addition to the existing JSON decode handling; update both places where
package.json is read so a non-UTF8 file doesn't raise and abort analysis, but is
handled cleanly by the methods in react_analyzer.py.
| # Step 2: Copy data including optional columns | ||
| insert_sql = f""" | ||
| INSERT INTO features_new | ||
| SELECT id, priority, category, name, description, steps, passes, in_progress, | ||
| dependencies, testing_in_progress, last_tested_at{optional_col_names} | ||
| FROM features |
There was a problem hiding this comment.
Handle missing core columns during rebuild.
INSERT ... SELECT assumes dependencies and last_tested_at exist. If a legacy or partially migrated DB has testing_in_progress but lacks one of those columns, the migration will fail. Build the SELECT list dynamically and use NULL AS <col> when missing.
🛠️ Proposed fix
- # Step 2: Copy data including optional columns
- insert_sql = f"""
- INSERT INTO features_new
- SELECT id, priority, category, name, description, steps, passes, in_progress,
- dependencies, testing_in_progress, last_tested_at{optional_col_names}
- FROM features
- """
+ # Step 2: Copy data including optional columns
+ base_select_cols = []
+ for col_name in [
+ "id", "priority", "category", "name", "description", "steps",
+ "passes", "in_progress", "dependencies",
+ "testing_in_progress", "last_tested_at",
+ ]:
+ if col_name in columns:
+ base_select_cols.append(col_name)
+ else:
+ base_select_cols.append(f"NULL AS {col_name}")
+ base_select = ", ".join(base_select_cols)
+ insert_sql = f"""
+ INSERT INTO features_new
+ SELECT {base_select}{optional_col_names}
+ FROM features
+ """🤖 Prompt for AI Agents
In `@api/migrations.py` around lines 128 - 133, The INSERT ... SELECT assumes core
columns exist; update the migration that builds insert_sql to construct the
SELECT list dynamically instead of hardcoding dependencies and last_tested_at:
inspect the current features table schema (or existing_col_names) and create a
select_cols string that always includes id, priority, category, name,
description, steps, passes, in_progress, testing_in_progress but for each of
'dependencies' and 'last_tested_at' append the real column name if present or
the placeholder "NULL AS dependencies"/"NULL AS last_tested_at" when missing,
then use that select_cols when building insert_sql to INSERT INTO features_new
SELECT {select_cols} FROM features (keep optional_col_names logic consistent).
| for wf_type in request.workflow_types: | ||
| if wf_type not in ["ci", "security", "deploy"]: | ||
| continue |
There was a problem hiding this comment.
Invalid workflow types are silently ignored.
If a user passes an invalid workflow type (e.g., ["invalid", "ci"]), the invalid entry is silently skipped. This could be confusing when users receive fewer workflows than expected without any indication of why.
Consider either validating upfront and returning a 400 error, or including skipped types in the response message.
🔧 Option A: Validate upfront
+ valid_types = {"ci", "security", "deploy"}
+ invalid_types = set(request.workflow_types) - valid_types
+ if invalid_types:
+ raise HTTPException(
+ status_code=400,
+ detail=f"Invalid workflow types: {', '.join(invalid_types)}",
+ )
+
try:
if request.provider == "github":
from integrations.ci import generate_github_workflow
workflows = []
for wf_type in request.workflow_types:
- if wf_type not in ["ci", "security", "deploy"]:
- continue🤖 Prompt for AI Agents
In `@server/routers/cicd.py` around lines 114 - 116, Request handling currently
silently skips invalid entries in request.workflow_types during the loop
(wf_type), causing silent failures; change this to validate upfront by computing
invalid_types = [t for t in request.workflow_types if t not in
{"ci","security","deploy"}] and if any exist return a 400 error (e.g., raise
HTTPException or return a JSONResponse) with a clear message listing the invalid
types and accepted values; update the handler in cicd.py where
request.workflow_types is used to perform this check before processing
workflows.
- Fix CRLF to LF line endings for YAML and shell scripts - Fix atomic table swap in migrations.py (rename-old, rename-new, drop-old) - Pin traefik version instead of :latest in docker-compose.traefik.yml - Add path traversal validation in quality_gates.py and templates.py - Update FastAPI and python-multipart versions for CVE fixes - Fix redact_string truncation logic in security.py - Add filename validation to upload_knowledge_file in projects.py - Use VALID_IDES constant in schemas.py validator - Use asyncio.to_thread for blocking calls in terminal_manager.py - Add AUTH_ALLOW_QUERY_TOKEN config flag in auth.py - Move orphan check before proc.wait() in process_utils.py - Fix duplicate loop and add language specifier in templates - Fix LogLevel normalization for critical level in structured_logging.py - Add isOpening state for IDE modal in App.tsx - Fix handleRetryInitializer timing in ProjectSetupRequired.tsx - Prevent modal close during pending reset in ResetProjectModal.tsx Co-authored-by: Claude <noreply@anthropic.com>
- registry.py: Include Ollama models in VALID_MODELS for validation - server/routers/settings.py: Pass limit=100 to get_denied_commands() - server/websocket.py: Add feature_update message emission on completion - parallel_orchestrator.py: Add stdin=DEVNULL and Windows CREATE_NO_WINDOW flags - requirements.txt: Document CVE-2026-24486 python-multipart fix - server/routers/projects.py: Add defense-in-depth filename validation with os.path.basename() - security.py: Simplify regex, add comprehensive type annotations with cast()
- Fix unused variables (F841) in stack_detector.py and chat session services - Fix blank line whitespace (W293) across multiple files - Fix trailing whitespace (W291) in multiple files - Fix f-string without placeholders (F541) in design_tokens router - Fix undefined names (F821) in templates router - Fix import order (I001) in templates router Resolves GitHub Actions CI failure due to ruff linting errors.
…checking, and package constraints
- Add binascii import - Add validate=True to base64.b64decode() - Catch binascii.Error in exception handler to return 401 instead of 500
- Add missing threading.Lock import in logging_config.py - Add _logging_lock variable - Remove trailing whitespace in templates/library.py - Fix import order in api/config.py - Fix import order in api/feature_repository.py - Remove unused imports in git_workflow.py and conftest.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…cked path validation
- Added support for page.js files in App Router - Added support for *.js files in Pages Router - Added support for *.js files in React Router detection - Added support for *.js files in component extraction - Ensures JavaScript-only React/Next.js projects are properly analyzed
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Add DuckDNS domain validation in scripts/deploy.sh - Add project directory existence check in server/routers/cicd.py - Remove internal error details from client responses (security fix)
- Enhanced delete_project endpoint to perform complete cleanup: * Disconnects all WebSocket connections * Stops and removes agent process managers * Stops dev servers * Deletes database files (features.db, assistant.db) * Unregisters from registry * Optionally deletes entire project directory - Added cleanup_manager function in process_manager to remove specific project managers - Added disconnect_all_for_project method in ConnectionManager - Updated ImportProjectModal to detect project name conflicts - Added Delete Existing Project button with confirmation dialog - Fixed 'deleteProject assigned but never used' error This ensures projects are cleanly removed from registry, websockets, API, database, agents, and dev servers, preventing conflicts when reimporting.
Updated pywinpty~=2.1 to pywinpty~=3.0 to match available versions. The latest version of pywinpty is 3.0.2, which resolves the dependency installation error on Windows.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Add arrow function export detection in react_analyzer.py - Preserve NOT NULL/DEFAULT constraints in migrations.py - Add DuckDNS subdomain validation in deploy.sh - Add directory existence check in cicd.py - Fix missing logger import and unsorted imports in projects.py - Remove trailing whitespace in process_manager.py and websocket.py - Add 'error' to Step type in ImportProjectModal.tsx - Convert deploy.sh to Unix LF line endings
9 participants
Summary
This PR consolidates and merges 15+ open PRs from the community, resolving conflicts and ensuring compatibility with the current codebase.
PRs Included:
Security & Stability:
New Features:
Infrastructure:
Skipped (Already in HEAD):
Additional Fixes:
Testing:
npm run buildpasses (2245 modules)ruff checkpasses (0 errors)Note: This PR includes everything manageable and best working changes up until PR #123. PR #123 has NOT been added to this merge.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.