-
Notifications
You must be signed in to change notification settings - Fork 279
feat: add Create Spec button and fix Windows asyncio subprocess #78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
UI Changes: - Add "Create Spec with AI" button in empty kanban when project has no spec - Button opens SpecCreationChat to guide users through spec creation - Shows in Pending column when has_spec=false and no features exist Windows Fixes: - Fix asyncio subprocess NotImplementedError on Windows - Set WindowsProactorEventLoopPolicy in server/__init__.py - Remove --reload from uvicorn (incompatible with Windows subprocess) - Add process cleanup on startup in start_ui.bat Spec Chat Improvements: - Enable full tool access (remove allowed_tools restriction) - Add "user" to setting_sources for global skills access - Use bypassPermissions mode for auto-approval - Add WebFetch/WebSearch auto-approve hook Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request introduces automatic approval for web tools (WebFetch/WebSearch) via a new security hook, fixes Windows asyncio subprocess support across multiple entry points, broadens chat session tool/skill scope and permission handling, and adds a Spec Creation UI flow with integrated chat components and conditional Kanban rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PreToolUseMatcher as Matcher
participant SecurityHook as web_tools_auto_approve_hook
participant ToolExecutor as Tool (WebFetch/WebSearch)
Client->>Matcher: Request to use tool (tool_name)
Matcher->>SecurityHook: match for WebFetch/WebSearch
SecurityHook-->>Matcher: return {} (auto-approve)
Matcher->>ToolExecutor: invoke tool with approved permissions
ToolExecutor-->>Client: tool result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@start_ui.bat`:
- Around line 12-16: The script indiscriminately force-kills any PID listening
on port 8888 using the for loop and taskkill, which can terminate unrelated
services; update the block around the for /f (...) loop and taskkill usage to
(1) resolve the PID(s) for port 8888, (2) for each PID call tasklist /FI "PID eq
<pid>" or equivalent to retrieve the Image Name, (3) prompt the user with the
PID and process name and require confirmation before calling taskkill, and/or
add an explicit whitelist of expected process names to auto-kill; ensure the
flow handles multiple PIDs and aborts safely if the user declines or no
whitelist match is found.
In `@start_ui.py`:
- Around line 189-207: The docstring and printed message in
start_production_server are misleading because the code launches uvicorn without
--reload (so there is no hot reload) and the comment about PYTHONASYNCIODEBUG
doesn't match since the env copy never sets that variable; update the
start_production_server function to (1) change the docstring and the print() to
state that the server is started in production mode without hot reload, and (2)
either remove/adjust the PYTHONASYNCIODEBUG comment or actually set
env["PYTHONASYNCIODEBUG"] = "1" before passing env into subprocess.Popen
(referencing start_production_server, venv_python, env, and the subprocess.Popen
call to locate the code).
🧹 Nitpick comments (3)
ui/src/components/KanbanColumn.tsx (1)
88-97: Add explicit button type for safety.Optional, but avoids accidental form submission if this component ever renders inside a
<form>.♻️ Proposed tweak
- <button + <button + type="button" onClick={onCreateSpec} className="neo-btn neo-btn-primary inline-flex items-center gap-2" >ui/src/App.tsx (1)
452-466: Add modal accessibility attributes to the overlay container.This improves screen-reader handling for the full-screen modal.
♿ Proposed accessibility tweak
- {showSpecChat && selectedProject && ( - <div className="fixed inset-0 z-50 bg-[var(--color-neo-bg)]"> + {showSpecChat && selectedProject && ( + <div + className="fixed inset-0 z-50 bg-[var(--color-neo-bg)]" + role="dialog" + aria-modal="true" + aria-label="Spec creation" + >server/services/spec_chat_session.py (1)
182-185: Clarify the security and architectural intent for unrestricted tool access in spec_chat_session.The code intentionally enables full tool access via
setting_sources=["project", "user"]andpermission_mode="bypassPermissions"with noallowed_toolsconstraint, as documented in the code comment. However, there is no documented rationale for why spec generation requires broader access than the read-only assistant service (which explicitly restricts tools). Consider adding a comment explaining the business requirement for unrestricted access, or implement environment-based gating (as suggested in the original diff) if this scope should be toggleable.
| REM Kill any existing processes on port 8888 | ||
| echo Cleaning up old processes... | ||
| for /f "tokens=5" %%a in ('netstat -aon ^| findstr ":8888" ^| findstr "LISTENING"') do ( | ||
| taskkill /F /PID %%a >nul 2>&1 | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid killing unrelated processes on port 8888.
The current block force-kills any listener on 8888, which can terminate unrelated services. Consider prompting the user (or filtering to expected process names) before killing.
🐛 Proposed safer prompt before killing
-REM Kill any existing processes on port 8888
-echo Cleaning up old processes...
-for /f "tokens=5" %%a in ('netstat -aon ^| findstr ":8888" ^| findstr "LISTENING"') do (
- taskkill /F /PID %%a >nul 2>&1
-)
+REM Kill any existing processes on port 8888 (with confirmation)
+echo Checking for processes on port 8888...
+for /f "tokens=5" %%a in ('netstat -aon ^| findstr ":8888" ^| findstr "LISTENING"') do (
+ echo Found PID %%a listening on 8888.
+ choice /M "Terminate PID %%a?"
+ if errorlevel 2 (
+ echo Skipping PID %%a
+ ) else (
+ taskkill /F /PID %%a >nul 2>&1
+ )
+)🤖 Prompt for AI Agents
In `@start_ui.bat` around lines 12 - 16, The script indiscriminately force-kills
any PID listening on port 8888 using the for loop and taskkill, which can
terminate unrelated services; update the block around the for /f (...) loop and
taskkill usage to (1) resolve the PID(s) for port 8888, (2) for each PID call
tasklist /FI "PID eq <pid>" or equivalent to retrieve the Image Name, (3) prompt
the user with the PID and process name and require confirmation before calling
taskkill, and/or add an explicit whitelist of expected process names to
auto-kill; ensure the flow handles multiple PIDs and aborts safely if the user
declines or no whitelist match is found.
| def start_production_server(port: int): | ||
| """Start FastAPI server in production mode.""" | ||
| """Start FastAPI server in production mode with hot reload.""" | ||
| venv_python = get_venv_python() | ||
|
|
||
| print(f"\n Starting server at http://127.0.0.1:{port}") | ||
| print(f"\n Starting server at http://127.0.0.1:{port} (with hot reload)") | ||
|
|
||
| # Set PYTHONASYNCIODEBUG to help with Windows subprocess issues | ||
| env = os.environ.copy() | ||
|
|
||
| # NOTE: --reload is NOT used because on Windows it breaks asyncio subprocess | ||
| # support (uvicorn's reload worker doesn't inherit the ProactorEventLoop policy). | ||
| # This affects Claude SDK which uses asyncio.create_subprocess_exec. | ||
| # For development with hot reload, use: python start_ui.py --dev | ||
| return subprocess.Popen([ | ||
| str(venv_python), "-m", "uvicorn", | ||
| "server.main:app", | ||
| "--host", "127.0.0.1", | ||
| "--port", str(port) | ||
| ], cwd=str(ROOT)) | ||
| "--port", str(port), | ||
| ], cwd=str(ROOT), env=env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix misleading hot‑reload messaging and env comment.
--reload is explicitly not used, so “with hot reload” in the docstring/log is inaccurate, and the PYTHONASYNCIODEBUG comment doesn’t match the code.
📝 Suggested cleanup
-def start_production_server(port: int):
- """Start FastAPI server in production mode with hot reload."""
+def start_production_server(port: int):
+ """Start FastAPI server in production mode."""
@@
- print(f"\n Starting server at http://127.0.0.1:{port} (with hot reload)")
+ print(f"\n Starting server at http://127.0.0.1:{port}")
@@
- # Set PYTHONASYNCIODEBUG to help with Windows subprocess issues
env = os.environ.copy()
+ # Optional: enable asyncio debug when troubleshooting Windows subprocess issues
+ # env["PYTHONASYNCIODEBUG"] = "1"🤖 Prompt for AI Agents
In `@start_ui.py` around lines 189 - 207, The docstring and printed message in
start_production_server are misleading because the code launches uvicorn without
--reload (so there is no hot reload) and the comment about PYTHONASYNCIODEBUG
doesn't match since the env copy never sets that variable; update the
start_production_server function to (1) change the docstring and the print() to
state that the server is started in production mode without hot reload, and (2)
either remove/adjust the PYTHONASYNCIODEBUG comment or actually set
env["PYTHONASYNCIODEBUG"] = "1" before passing env into subprocess.Popen
(referencing start_production_server, venv_python, env, and the subprocess.Popen
call to locate the code).
- Verified kanban board renders 149 features successfully - Scroll performance: 0.10ms (well under 100ms threshold) - Tested smooth scrolling from top to bottom and back - No JavaScript errors (only non-critical favicon 404) - Screenshots captured at each verification step Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Changes
UI:
has_spec=falseand no featuresWindows Fixes:
WindowsProactorEventLoopPolicyinserver/__init__.py--reloadfrom uvicorn (breaks Windows subprocess)start_ui.batSpec Chat:
allowed_toolsrestriction for full tool access"user"tosetting_sourcesfor global skillsWebFetch/WebSearchauto-approve hookTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
UX
✏️ Tip: You can customize this high-level summary in your review settings.