feat: default to stdio transport, background daemon mode, and service persistence #184
Conversation
With the new daemon architecture (Alpha 2), CodeWeaver now supports running background services separately from MCP server instances. The stdio transport now proxies to the HTTP backend daemon, providing universal MCP client compatibility while maintaining shared state and background indexing. Changes: - Default transport changed from 'streamable-http' to 'stdio' across CLI, main.py, and configuration files - Updated Dockerfile to use stdio by default (for MCP client-spawned containers) - Updated docker-compose.yml to explicitly use HTTP transport for service mode - Updated documentation (DOCKER.md, README.md, docs/CLI.md) to reflect new architecture and defaults - Updated server.json MCP registry definitions - Updated init command messaging and examples - Simplified StdioCodeWeaverConfig args (no longer needs --transport stdio) - Rewrote MCP documentation in mcp.py to explain daemon architecture The new recommended workflow is: 1. Start the daemon: `cw start` 2. MCP clients connect via stdio (default) which proxies to the daemon
When running `codeweaver server` with stdio transport (the new default), the server now checks if the daemon is running and starts it automatically if needed. This provides a seamless experience where users don't need to manually start the daemon before using MCP clients. Changes: - Added `_check_daemon_health()` to verify management server is running - Added `_start_daemon_if_needed()` to auto-start daemon with wait-for-healthy - Updated `_run_stdio_server()` to ensure daemon is running before proxying - Enhanced `cw start` to also start MCP HTTP server (port 9328) alongside management server (port 9329), enabling stdio proxy connections - Updated `start_cw_services()` to include MCP HTTP server by default - Updated docstrings to reflect new daemon architecture The daemon now starts both: - Management server on port 9329 (health, status, metrics endpoints) - MCP HTTP server on port 9328 (for stdio proxy connections)
…aults Since stdio is now the default transport, the args for stdio config no longer need to explicitly specify --transport stdio. Updated default args from ["server", "--transport", "stdio"] to just ["server"].
Changes to start command: - Default behavior now runs daemon in background and returns immediately - Added --foreground/-f flag to run in current terminal (old behavior) - Shows success message and endpoint info when daemon starts - Waits up to 30 seconds for daemon to become healthy New service persistence feature: - Added `cw init service` command to install as system service - Linux: Creates systemd user service (~/.config/systemd/user/) - macOS: Creates launchd user agent (~/Library/LaunchAgents/) - Windows: Provides NSSM setup instructions - Added `cw start persist` as convenience alias - Supports --enable/--no-enable and --uninstall flags
Tests cover: - Start command background/foreground modes (--foreground flag) - Daemon health check and process spawning - init service command for systemd/launchd/Windows - Service file generation (systemd unit, launchd plist) - start persist alias behavior 22 tests passing, 1 skipped (macOS-specific on Linux).
- Add cw start and cw stop to CLI commands section - Add "Running as a System Service" subsection - Document cw init service for systemd/launchd setup
AGENTS.md: - Add architecture overview (daemon, management server, MCP HTTP, stdio proxy) - Update status to Alpha Release 2 - Add start.py and stop.py to CLI commands structure - Update server/ structure with new components ARCHITECTURE.md (v1.2.0): - Add comprehensive Daemon Architecture section with ASCII diagram - Document components: daemon, management server (9329), MCP HTTP server (9328) - Explain transport modes (stdio default, streamable-http) - List CLI commands: start, stop, init service, server - Add rationale for the architecture decisions
bashandbone
added
documentation
Reviewer's GuideSwitches CodeWeaver’s default MCP transport to stdio (proxying to an HTTP daemon), makes the daemon auto-start and run in the background by default, and adds CLI support plus docs for installing/running CodeWeaver as a persistent system service across platforms. Sequence diagram for cw start foreground vs background daemon modessequenceDiagram
actor User
participant StartCLI as cw_start_command
participant StartFunc as start_function
participant BGStarter as start_daemon_background
participant HealthWait as wait_for_daemon_healthy
participant DaemonProc as daemon_process
participant Mgmt as Management_server_9329
participant MCPHTTP as MCP_HTTP_server_9328
User->>StartCLI: cw start [--foreground?]
StartCLI->>+StartFunc: start(foreground_flag, options)
StartFunc->>StartFunc: are_services_running()
alt already_running
StartFunc-->>User: warn("background services already running")
StartFunc-->>User: show_management_url
else not_running
alt foreground_mode
StartFunc-->>User: info("Starting daemon in foreground")
StartFunc->>+DaemonProc: start_cw_services(start_mcp_http_server=true)
DaemonProc->>MCPHTTP: create_http_server(9328)
DaemonProc->>Mgmt: start_management_server(9329)
DaemonProc-->>StartFunc: run_until_interrupted
User-->>DaemonProc: Ctrl+C
DaemonProc-->>StartFunc: shutdown_services
StartFunc-->>User: exit
else background_default
StartFunc-->>User: info("Starting daemon in background")
StartFunc->>+BGStarter: start_daemon_background(options)
BGStarter->>DaemonProc: spawn_detached("cw start --foreground ...")
BGStarter-->>StartFunc: success_flag
alt spawn_failed
StartFunc-->>User: error("Failed to start daemon process")
else spawn_ok
StartFunc-->>User: info("Waiting for daemon to start")
StartFunc->>+HealthWait: wait_for_daemon_healthy(mgmt_host, 9329)
loop until_healthy_or_timeout
HealthWait->>Mgmt: GET /health
end
alt healthy
HealthWait-->>StartFunc: true
StartFunc-->>User: success("daemon started")
StartFunc-->>User: info("Management http://host:9329")
StartFunc-->>User: info("MCP HTTP http://host:9328/mcp/")
StartFunc-->>User: info("Stop with: cw stop")
else timeout
HealthWait-->>StartFunc: false
StartFunc-->>User: error("daemon did not become healthy")
end
end
end
end
Flow diagram for choosing stdio vs HTTP transport and daemon behaviorflowchart TD
A[User configures transport] --> B{Transport_mode}
B -->|stdio_default| C[Use stdio transport]
B -->|streamable_http| D[Use HTTP transport]
%% stdio path
C --> E[MCP client spawns cw server]
E --> F[run_stdio_server]
F --> G{Daemon_healthy?}
G -->|yes| H[Create stdio proxy to MCP HTTP server]
G -->|no| I[start_daemon_if_needed]
I --> J{Daemon_started_and_healthy?}
J -->|yes| H
J -->|no| K[InitializationError: instruct user to run cw start]
H --> L[Proxy MCP traffic over HTTP to daemon]
%% HTTP path
D --> M[User runs cw server --transport streamable-http]
M --> N[Start background services + MCP HTTP + management]
N --> O[Clients connect directly via HTTP 9328]
%% Daemon lifecycle
P[User runs cw start] --> Q{foreground_flag}
Q -->|false_default| R[start_daemon_background]
R --> S[spawn cw start --foreground detached]
S --> T[wait_for_daemon_healthy]
T --> U{healthy_within_timeout?}
U -->|yes| V[Print success and endpoints]
U -->|no| W[Print error and hint to use foreground]
Q -->|true| X[start_cw_services in current terminal]
%% Service persistence
AA[User runs cw init service or cw start persist] --> AB{Platform}
AB -->|Linux| AC[Install systemd user service]
AB -->|macOS| AD[Install launchd user agent]
AB -->|Windows| AE[Show NSSM/Task_Scheduler_instructions]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ CLA Check PassedAll contributors to this PR are exempt from the CLA requirement:
Knitli organization members and recognized bots do not need to sign the CLA. Thanks for your contribution to codeweaver! 🧵 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The daemon startup and health-check logic is duplicated between
_start_daemon_backgroundinstart.pyand_start_daemon_if_neededinmain.py; consider extracting a shared helper (for spawning the process and polling/health) to avoid divergence in behavior and make future changes easier. - In
_run_stdio_server,_start_daemon_if_neededis called withmanagement_host=host, but the management server may be bound to a different host than the MCP HTTP host; it may be safer to parameterize or decouple these so changing the MCP host does not accidentally break daemon health checks. - The background daemon spawning helpers (
_start_daemon_backgroundand_start_daemon_if_needed) currently ignore the intended working directory (e.g., project path); consider passing an explicitcwdtosubprocess.Popenso the daemon consistently runs relative to the selected project root.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The daemon startup and health-check logic is duplicated between `_start_daemon_background` in `start.py` and `_start_daemon_if_needed` in `main.py`; consider extracting a shared helper (for spawning the process and polling `/health`) to avoid divergence in behavior and make future changes easier.
- In `_run_stdio_server`, `_start_daemon_if_needed` is called with `management_host=host`, but the management server may be bound to a different host than the MCP HTTP host; it may be safer to parameterize or decouple these so changing the MCP host does not accidentally break daemon health checks.
- The background daemon spawning helpers (`_start_daemon_background` and `_start_daemon_if_needed`) currently ignore the intended working directory (e.g., project path); consider passing an explicit `cwd` to `subprocess.Popen` so the daemon consistently runs relative to the selected project root.
## Individual Comments
### Comment 1
<location> `src/codeweaver/cli/commands/init.py:958-967` </location>
<code_context>
)
+def _get_systemd_unit(cw_cmd: str, working_dir: Path) -> str:
+ """Generate systemd user service unit file content.
+
+ Args:
+ cw_cmd: Path to the codeweaver executable
+ working_dir: Working directory for the service
+
+ Returns:
+ Systemd unit file content as a string
+ """
+ return f"""[Unit]
+Description=CodeWeaver MCP Server - Semantic Code Search
+Documentation=https://github.com/knitli/codeweaver
+After=network.target
+
+[Service]
+Type=simple
+ExecStart={cw_cmd} start --foreground
+WorkingDirectory={working_dir}
+Restart=on-failure
</code_context>
<issue_to_address>
**issue (bug_risk):** Quote the ExecStart command to handle paths with spaces in systemd units.
Because systemd tokenizes `ExecStart` on whitespace, this will break if `cw_cmd` or `working_dir` ever contains spaces (e.g. a home directory with a space). Please ensure the command path is safely quoted or otherwise structured to avoid bare string interpolation, for example by using `shlex.quote` when building the `ExecStart` line.
</issue_to_address>
### Comment 2
<location> `src/codeweaver/main.py:292-294` </location>
<code_context>
+ return False
+
+
async def _run_stdio_server(
*,
config_file: FilePath | None = None,
</code_context>
<issue_to_address>
**issue:** Docstring for stdio server still suggests manual daemon start, which conflicts with the new auto-start behavior.
Since `_run_stdio_server` now calls `_start_daemon_if_needed`, the daemon is auto-started when needed. The `run()` docstring still instructs users to run `codeweaver start` first, which no longer reflects the behavior. Please update the docstring to describe the auto-start behavior and, if helpful, mention `codeweaver start` only for users who want to manage the daemon lifecycle manually.
</issue_to_address>
### Comment 3
<location> `src/codeweaver/cli/commands/start.py:325-328` </location>
<code_context>
# Check if already running
if await are_services_running():
display.print_warning("Background services already running")
- display.print_info("Management server: http://127.0.0.1:9329", prefix="🌐")
+ display.print_info(f"Management server: http://{management_host}:{management_port}", prefix="🌐")
return
</code_context>
<issue_to_address>
**question (bug_risk):** Reported management host/port may not match what `are_services_running()` actually checked.
Because `are_services_running()` is invoked without arguments, it probably checks a default host/port (e.g. `127.0.0.1:9329`), while the log prints the user-provided `management_host`/`management_port`. If these differ, the log won’t reflect what was actually checked. Consider passing host/port into `are_services_running()` (if supported) or logging the exact values it uses.
</issue_to_address>
### Comment 4
<location> `Dockerfile:111-114` </location>
<code_context>
-# Default command: start the CodeWeaver MCP server
-# Users can override this with custom config via docker-compose or docker run
+# Default command: start the CodeWeaver MCP server in stdio mode
+# For persistent HTTP service (docker-compose), use: --transport streamable-http
ENTRYPOINT ["/entrypoint.sh"]
-CMD ["codeweaver", "server", "--host", "0.0.0.0", "--port", "9328"]
+CMD ["codeweaver", "server"]
</code_context>
<issue_to_address>
**issue (bug_risk):** Default Docker CMD runs stdio transport, which conflicts with exposed port and healthcheck expectations.
With the new stdio-only `CMD`, the image exposes 9328 and has an HTTP-style healthcheck, but no HTTP listener actually runs on that port. A simple `docker run` will expose an unused port and likely fail healthchecks, while docker-compose correctly switches to `--transport streamable-http`. To avoid this inconsistent behavior, either keep the default `CMD` in HTTP mode (binding to `0.0.0.0:9328`), or update EXPOSE/healthcheck/docs to indicate that the default image is for stdio use only, not an HTTP service.
</issue_to_address>
### Comment 5
<location> `tests/unit/cli/test_start_command.py:40-42` </location>
<code_context>
+ return project
+
+
+@pytest.mark.unit
+@pytest.mark.cli
+class TestStartCommandBehavior:
+ """Tests for the `start` command behavior - background vs foreground modes."""
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that exercise the `start` coroutine end‑to‑end for foreground/background and already‑running cases
Current tests only cover CLI argument handling (`foreground` flag, `are_services_running` wiring) but never call `codeweaver.cli.commands.start.start` itself. Given that `start` now contains all the core behavior (already‑running short‑circuit, foreground vs background branching, daemon startup + health wait, and URL/error messaging), please add targeted tests that:
- Exercise the already‑running path by mocking `are_services_running=True`, asserting early return, correct management URL output (including custom host/port), and no daemon spawn.
- Exercise `foreground=True` with `are_services_running=False`, asserting `start_cw_services` is awaited with the expected arguments (`config_file`, `project`, `mcp_host`, `mcp_port`).
- Exercise `foreground=False` with `are_services_running=False` for:
- Successful daemon startup + health check: assert success messages and correct management + MCP URLs.
- Daemon startup failure: assert an error is printed and `_wait_for_daemon_healthy` is not awaited.
- Health‑wait timeout: assert the timeout message and the suggestion to use `--foreground` are printed.
This will ensure the new start logic is covered end‑to‑end and protected against regressions.
</issue_to_address>
### Comment 6
<location> `src/codeweaver/cli/commands/start.py:130` </location>
<code_context>
subprocess.Popen([cw_cmd, *cw_args], **kwargs)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 7
<location> `src/codeweaver/main.py:272` </location>
<code_context>
subprocess.Popen([cw_cmd, *cw_args], **kwargs)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 8
<location> `src/codeweaver/cli/commands/init.py:1182` </location>
<code_context>
@app.command
def service(
*,
project: Annotated[Path | None, cyclopts.Parameter(name=["--project", "-p"])] = None,
enable: Annotated[
bool,
cyclopts.Parameter(
name=["--enable", "-e"],
help="Enable and start the service immediately (Linux/macOS only)",
),
] = True,
uninstall: Annotated[
bool,
cyclopts.Parameter(
name=["--uninstall", "-u"],
help="Remove the installed service",
),
] = False,
) -> None:
"""Install CodeWeaver as a system service for automatic startup.
This command configures CodeWeaver to start automatically when you log in.
**Linux (systemd):**
Creates a user systemd service at ~/.config/systemd/user/codeweaver.service
**macOS (launchd):**
Creates a user launch agent at ~/Library/LaunchAgents/li.knit.codeweaver.plist
**Windows:**
Provides instructions for setting up with NSSM or Task Scheduler.
Examples:
cw init service # Install and enable service
cw init service --no-enable # Install without enabling
cw init service --uninstall # Remove the service
cw start persist # Alias for 'cw init service'
"""
display = _display
error_handler = CLIErrorHandler(display)
display.print_command_header("init service", "Install CodeWeaver as a system service")
# Determine project path
project_path = (project or resolve_project_root()).resolve()
display.print_info(f"Working directory: {project_path}\n")
# Find the codeweaver executable
cw_cmd = shutil.which("cw") or shutil.which("codeweaver")
if not cw_cmd:
cw_cmd = f"{sys.executable} -m codeweaver"
display.print_warning(f"Could not find cw/codeweaver in PATH, using: {cw_cmd}")
else:
display.print_info(f"Executable: {cw_cmd}")
platform = sys.platform
if uninstall:
# Handle uninstallation
display.print_section("Uninstalling Service")
if platform == "linux":
import subprocess
service_file = Path.home() / ".config" / "systemd" / "user" / "codeweaver.service"
if service_file.exists():
try:
subprocess.run(["systemctl", "--user", "stop", "codeweaver.service"], capture_output=True)
subprocess.run(["systemctl", "--user", "disable", "codeweaver.service"], capture_output=True)
service_file.unlink()
subprocess.run(["systemctl", "--user", "daemon-reload"], check=True, capture_output=True)
display.print_success("Removed systemd service")
except Exception as e:
error_handler.handle_error(CodeWeaverError(f"Failed to remove service: {e}"), "Service removal")
else:
display.print_warning("Service not installed")
elif platform == "darwin":
import subprocess
plist_file = Path.home() / "Library" / "LaunchAgents" / "li.knit.codeweaver.plist"
if plist_file.exists():
try:
subprocess.run(["launchctl", "unload", str(plist_file)], capture_output=True)
plist_file.unlink()
display.print_success("Removed launchd agent")
except Exception as e:
error_handler.handle_error(CodeWeaverError(f"Failed to remove agent: {e}"), "Service removal")
else:
display.print_warning("Agent not installed")
elif platform == "win32":
display.print_info("To remove Windows service:")
display.print_info(" nssm remove CodeWeaver confirm")
return
# Handle installation
display.print_section("Installing Service")
if platform == "linux":
success = _install_systemd_service(display, cw_cmd, project_path, enable)
if success:
display.print_section("Management Commands")
display.print_list(
[
"Status: systemctl --user status codeweaver.service",
"Stop: systemctl --user stop codeweaver.service",
"Start: systemctl --user start codeweaver.service",
"Logs: journalctl --user -u codeweaver.service -f",
"Disable: systemctl --user disable codeweaver.service",
],
title="",
)
elif platform == "darwin":
success = _install_launchd_service(display, cw_cmd, project_path, enable)
if success:
display.print_section("Management Commands")
display.print_list(
[
"Status: launchctl list | grep codeweaver",
"Stop: launchctl unload ~/Library/LaunchAgents/li.knit.codeweaver.plist",
"Start: launchctl load ~/Library/LaunchAgents/li.knit.codeweaver.plist",
"Logs: tail -f ~/Library/Logs/codeweaver.log",
],
title="",
)
elif platform == "win32":
_show_windows_instructions(display, cw_cmd, project_path)
else:
display.print_warning(f"Unsupported platform: {platform}")
display.print_info("Manual setup required. Run: cw start --foreground")
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Use named expression to simplify assignment and conditional [×2] ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Functions should be less than 40 lines (`no-long-functions`)
- Low code quality found in service - 16% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
<br/><details><summary>Explanation</summary>
Prefer small and focused functions.
We recognize that long functions are sometimes appropriate, so no hard limit is placed on function length. If a function exceeds about 40 lines, think about whether it can be broken up without harming the structure of the program.
Even if your long function works perfectly now, someone modifying it in a few months may add new behavior. This could result in bugs that are hard to find. Keeping your functions short and simple makes it easier for other people to read and modify your code.
From Google Style Guide [3.18](https://google.github.io/styleguide/pyguide.html#318-function-length)
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>
### Comment 9
<location> `src/codeweaver/cli/commands/start.py:249` </location>
<code_context>
async def start_cw_services(
display: StatusDisplay,
config_path: Path | None = None,
project_path: Path | None = None,
*,
start_mcp_http_server: bool = True, # Start MCP HTTP server for stdio proxy support
mcp_host: str | None = None,
mcp_port: PositiveInt | None = None,
) -> None:
"""Start background services and optionally MCP HTTP server.
By default, starts both the management server (port 9329) and MCP HTTP server
(port 9328) to support stdio proxy connections.
"""
from codeweaver.common.statistics import get_session_statistics
from codeweaver.config.settings import get_settings
from codeweaver.server.lifespan import background_services_lifespan
from codeweaver.server.management import ManagementServer
# Load settings
settings = get_settings()
if config_path:
settings.config_file = config_path # type: ignore
if project_path:
settings.project_path = project_path
statistics = get_session_statistics()
# Resolve MCP server host/port
_mcp_host = mcp_host or getattr(settings, "mcp_host", "127.0.0.1")
_mcp_port = mcp_port or getattr(settings, "mcp_port", 9328)
# Use background_services_lifespan (the new Phase 1 implementation)
async with background_services_lifespan(
settings=settings,
statistics=statistics,
status_display=display,
verbose=False,
debug=False,
) as background_state:
# Start management server
mgmt_host = getattr(settings, "management_host", "127.0.0.1")
mgmt_port = getattr(settings, "management_port", 9329)
management_server = ManagementServer(background_state)
await management_server.start(host=mgmt_host, port=mgmt_port)
display.print_success("Background services started successfully")
display.print_info(
f"Management server: http://{mgmt_host}:{mgmt_port}", prefix="🌐"
)
# Start MCP HTTP server if requested (needed for stdio proxy)
mcp_server_task = None
if start_mcp_http_server:
from codeweaver.mcp.server import create_http_server
mcp_state = await create_http_server(
host=_mcp_host, port=_mcp_port, verbose=False, debug=False
)
display.print_info(
f"MCP HTTP server: http://{_mcp_host}:{_mcp_port}/mcp/", prefix="🔌"
)
# Start MCP HTTP server as background task
mcp_server_task = asyncio.create_task(
mcp_state.app.run_http_async(**mcp_state.run_args)
)
try:
# Keep services running until interrupted
tasks_to_wait = [t for t in [management_server.server_task, mcp_server_task] if t]
if tasks_to_wait:
await asyncio.gather(*tasks_to_wait)
else:
# Shouldn't happen: no server tasks set
raise RuntimeError("No server tasks were created. This should not happen; please check server startup logic.")
except (KeyboardInterrupt, asyncio.CancelledError):
display.print_warning("Shutting down background services...")
finally:
if mcp_server_task and not mcp_server_task.done():
mcp_server_task.cancel()
with asyncio.suppress(asyncio.CancelledError):
await mcp_server_task
await management_server.stop()
</code_context>
<issue_to_address>
**issue (code-quality):** Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
</issue_to_address>
### Comment 10
<location> `tests/unit/cli/test_start_command.py:118-127` </location>
<code_context>
def test_start_daemon_background_finds_executable(self, temp_project: Path) -> None:
"""Test that _start_daemon_background finds the codeweaver executable."""
from codeweaver.cli.commands.start import _start_daemon_background
from codeweaver.cli.ui import StatusDisplay
display = StatusDisplay()
# Mock subprocess.Popen and shutil.which
with patch("codeweaver.cli.commands.start.shutil.which") as mock_which, \
patch("codeweaver.cli.commands.start.subprocess.Popen") as mock_popen:
# Simulate finding cw executable
mock_which.return_value = "/usr/local/bin/cw"
mock_popen.return_value = MagicMock()
result = _start_daemon_background(display)
assert result is True
mock_popen.assert_called_once()
# Verify --foreground is passed (background spawns foreground process)
call_args = mock_popen.call_args[0][0]
assert "--foreground" in call_args
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Extract code out into method ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
- Inline variable that is only used once ([`inline-variable`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/inline-variable/))
</issue_to_address>
### Comment 11
<location> `tests/unit/cli/test_start_command.py:139-149` </location>
<code_context>
def test_start_daemon_background_uses_python_fallback(self, temp_project: Path) -> None:
"""Test fallback to python -m codeweaver when executable not found."""
from codeweaver.cli.commands.start import _start_daemon_background
from codeweaver.cli.ui import StatusDisplay
display = StatusDisplay()
with patch("codeweaver.cli.commands.start.shutil.which") as mock_which, \
patch("codeweaver.cli.commands.start.subprocess.Popen") as mock_popen:
# Simulate not finding cw executable
mock_which.return_value = None
mock_popen.return_value = MagicMock()
result = _start_daemon_background(display)
assert result is True
call_args = mock_popen.call_args[0][0]
# Should use sys.executable with -m codeweaver
assert sys.executable in call_args[0]
assert "-m" in call_args
assert "codeweaver" in call_args
</code_context>
<issue_to_address>
**issue (code-quality):** Extract code out into method ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Review - PR #184
This PR introduces significant architectural improvements to CodeWeaver's daemon and MCP transport system. The changes are well-structured and the implementation is solid overall. However, there are several critical security and correctness issues that must be addressed before merging.
🔴 Critical Issues (Must Fix)
1. **Command Injection Risk in ** (Security)
Location: ,
The code constructs subprocess commands dynamically without proper escaping:
subprocess.Popen([cw_cmd, *cw_args], **kwargs)While using list form (not shell=True) provides some protection, the cw_cmd variable is derived from user-controlled paths via shutil.which(). If an attacker can manipulate the PATH or place malicious executables, this could be exploited.
Recommendation:
- Validate
cw_cmdis a trusted path - Consider using absolute paths from known safe locations
- Add explicit validation that the command is the expected CodeWeaver binary
2. Unquoted Paths in systemd Unit Files (Bug Risk)
Location: (ExecStart line)
ExecStart={cw_cmd} start --foreground
WorkingDirectory={working_dir}If cw_cmd or working_dir contains spaces, systemd will fail to parse the unit file correctly.
Recommendation: Use shlex.quote():
import shlex
ExecStart={shlex.quote(cw_cmd)} start --foreground
WorkingDirectory={shlex.quote(str(working_dir))}3. Docker CMD/Healthcheck Mismatch (Critical Bug)
Location: ,
The default CMD now runs codeweaver server (stdio mode), but:
- Port 9328 is exposed
- Healthcheck expects HTTP on
localhost:9328/health/ - stdio mode won't open an HTTP listener
Result: The default docker run will:
- Expose an unused port
- Fail all healthchecks
- Not work as expected
Recommendation: Either:
- Keep the default CMD as HTTP:
CMD ["codeweaver", "server", "--host", "0.0.0.0", "--port", "9328", "--transport", "streamable-http"] - OR remove EXPOSE/HEALTHCHECK and document that the default is for stdio-only use
⚠️ High Priority Issues
4. Inconsistent Host/Port Checking
Location:
are_services_running() checks a default host/port but the displayed URL uses management_host/management_port parameters. If these differ, the message will be misleading.
Recommendation: Pass host/port to are_services_running() or document which values it uses.
5. Missing Working Directory in Daemon Spawn
Location: ,
Both daemon spawn helpers don't set cwd in subprocess.Popen. The daemon will inherit the parent's cwd, which may not be the intended project directory.
Recommendation: Add cwd=project to the Popen kwargs when project is specified.
6. Duplicate Daemon Startup Logic
Location: and
The daemon startup and health-check logic is duplicated between two files, increasing maintenance burden and risk of divergence.
Recommendation: Extract shared helper to a common module (e.g., codeweaver.daemon.spawn).
7. Outdated stdio Server Docstring
Location: (around _run_stdio_server)
The docstring still suggests users must manually run codeweaver start, but the server now auto-starts the daemon.
Recommendation: Update docstring to reflect auto-start behavior.
📝 Code Quality Suggestions
8. Long service() Function (Code Quality)
Location:
The service() function is 100+ lines with nested conditionals and low code quality score (16%).
Recommendation: Refactor into smaller functions:
_handle_service_uninstall(platform, display)_handle_service_install(platform, cw_cmd, working_dir, enable, display)
9. Use Named Expressions (Code Quality)
Location: ,
Code like:
cw_cmd = shutil.which("cw") or shutil.which("codeweaver")
if not cw_cmd:Could use walrus operator:
if not (cw_cmd := shutil.which("cw") or shutil.which("codeweaver")):10. Missing End-to-End Tests (Testing)
Location:
Current tests only verify argument handling. Missing tests for:
- Full
start()coroutine with foreground=True/False - Already-running short-circuit behavior
- Daemon spawn + health wait timeout scenarios
Recommendation: Add integration tests that call start() with mocked services.
✅ What's Good
- Architecture: The daemon separation is well-designed and addresses real UX pain points
- Documentation: Excellent updates to ARCHITECTURE.md and AGENTS.md
- User Experience: Auto-start daemon feature is a great improvement
- Test Coverage: Good foundation with 22 passing tests
- Platform Support: Thoughtful systemd/launchd/Windows service installation
- Commit Structure: Clean, well-organized commits with clear messages
📊 Summary
| Category | Status |
|---|---|
| Architecture | ✅ Excellent |
| Functionality | ✅ Good |
| Security | 🔴 Critical issues |
| Docker Config | 🔴 Critical issues |
| Code Quality | |
| Testing | |
| Documentation | ✅ Excellent |
Recommendation
Request Changes - The critical security and Docker issues must be addressed before merging. Once fixed, this will be a strong addition to the codebase.
Must Fix Before Merge:
- Add input validation for subprocess commands (security)
- Quote paths in systemd unit files (correctness)
- Fix Docker CMD/healthcheck mismatch (functionality)
- Add
cwdparameter to daemon spawning (correctness)
Strongly Recommended:
- Extract duplicate daemon startup logic
- Update outdated docstrings
- Refactor long
service()function - Add end-to-end tests for
startcommand
Great work on the architecture design! Once these issues are addressed, this will significantly improve CodeWeaver's usability. 🚀
Pull Request ReviewI've reviewed PR #184 and found the changes to be well-architected with comprehensive test coverage. However, there is one critical issue blocking merge. ❌ Critical Issue - Must Fix Before Merge
✅ StrengthsArchitecture & Design:
Code Quality:
Testing:
Documentation:
💡 Suggestions (Non-blocking)src/codeweaver/cli/commands/init.py:360-367: src/codeweaver/cli/commands/start.py:94-136: src/codeweaver/main.py:94-100: docker-compose.yml:45-46: 🔒 Security & Performance
📋 Before Merge
SummaryThis is a well-executed PR that significantly improves CodeWeaver's deployment story and user experience. The daemon architecture is clean, the stdio-as-default choice is smart, and the service persistence feature rounds out the production readiness. Once the Recommendation: Request changes for the Files reviewed: 14 changed files, 1462 additions, 135 deletions |
PR Review: Default stdio transport, background daemon, and service persistenceOverviewThis PR introduces significant architectural improvements to CodeWeaver's daemon and MCP architecture. I've reviewed the changes and have feedback on both the implementation and a blocking CI failure. 🚨 Blocking Issue: server.json validation failureThe Location: Required fix: {
"registryType": "docker",
"identifier": "knitli/codeweaver",
"version": "latest", // <-- ADD THIS FIELD
"transport": {
"type": "stdio",
...
}
}You need to add a Architecture & Design ✅Strengths:
Design considerations:
Code Quality ✅Positive observations:
Minor suggestions:
Security & Performance
|
There was a problem hiding this comment.
PR Review: Default stdio Transport, Background Daemon Mode, and Service Persistence
Summary
This PR introduces significant architectural improvements to CodeWeaver by implementing a daemon-based architecture with stdio transport as the default. The changes are well-documented and represent a substantial improvement to the user experience.
Overall Assessment: The PR is well-structured with comprehensive documentation and testing. However, there are blocking security and configuration issues that must be addressed before merging.
Critical Issues (Must Fix)
1. Security: Command Injection Risk
Files: src/codeweaver/cli/commands/start.py:130, src/codeweaver/main.py:272
The subprocess.Popen([cw_cmd, *cw_args], **kwargs) calls are flagged for potential command injection. While the current implementation appears safe (using list form), the concern is valid:
cw_cmdis derived fromshutil.which()which is safe- However, the code should add input validation or comments documenting the safety
Recommendation: Add a comment explaining why this is safe, or add explicit validation that cw_cmd is a valid path.
2. systemd Unit File: Path with Spaces
File: src/codeweaver/cli/commands/init.py:975
The ExecStart={cw_cmd} start --foreground line will break if cw_cmd contains spaces. systemd requires proper quoting or an alternate format.
Fix Required:
import shlex
# In _get_systemd_unit():
return f"""[Unit]
...
[Service]
Type=simple
ExecStart={shlex.quote(cw_cmd)} start --foreground
WorkingDirectory={working_dir}
...
"""3. Docker Configuration Mismatch
File: Dockerfile:111-114
The default Docker CMD now runs codeweaver server (stdio mode), but:
- Port 9328 is exposed (expects HTTP)
- Healthcheck expects HTTP endpoint
- This will cause failures in plain
docker run
Options:
- Keep HTTP as default in Docker:
CMD ["codeweaver", "server", "--transport", "streamable-http", "--host", "0.0.0.0", "--port", "9328"] - Or update EXPOSE/healthcheck to reflect stdio-only mode (less practical)
Recommendation: Revert Dockerfile CMD to HTTP mode (option 1) since containers typically run persistent services.
High Priority Issues
4. Duplicate Code: Daemon Startup Logic
Files: start.py:\_start_daemon_background and main.py:\_start_daemon_if_needed
Both functions implement similar daemon spawning and health-check logic. This duplication will lead to maintenance issues.
Recommendation: Extract shared logic into a common helper function in a shared module (e.g., codeweaver/common/daemon_utils.py).
5. Working Directory Not Passed to subprocess.Popen
Files: start.py:130, main.py:272
The daemon is spawned without cwd parameter, so it inherits the current working directory. This may not be the intended project path.
Fix:
subprocess.Popen([cw_cmd, *cw_args], cwd=str(project_path), **kwargs)6. are_services_running() Doesn't Accept Host/Port Parameters
File: start.py:325-328
The function checks default host/port but logs custom management_host/management_port. If users configure custom ports, the check will be incorrect.
Fix: Add parameters to are_services_running(host, port) and pass them through.
Code Quality Issues (Medium Priority)
7. Test Coverage: Missing End-to-End Tests
File: tests/unit/cli/test_start_command.py
Current tests only check CLI arguments and mocking. Missing:
- Tests for
start()function itself - Already-running detection
- Foreground vs background paths
- Health-check timeout scenarios
Recommendation: Add integration-style tests that exercise the full start() command flow.
8. Function Length: init service() is Too Long
File: init.py:1182 (106 lines)
The service() function handles installation, uninstallation, and multiple platforms in one function. This violates the single responsibility principle.
Recommendation: Extract platform-specific logic:
_handle_linux_service()_handle_macos_service()_handle_windows_service()
9. Code Quality: Extract Repeated Patterns
Files: test_start_command.py:118-127, 139-149
The test setup for mocking shutil.which and subprocess.Popen is repeated. Extract to a fixture or helper.
Minor Issues / Suggestions
10. Docstring Inconsistency
File: main.py:292-294
The docstring still mentions "run codeweaver start first" but the daemon auto-starts now. Update docs to reflect auto-start behavior.
11. Named Expressions
Files: start.py:249, init.py:1182
Several locations could use Python 3.8+ walrus operator for cleaner code (e.g., if (cmd := shutil.which("cw"))). This is optional but improves readability.
What Went Well ✅
- Excellent Documentation: ARCHITECTURE.md and AGENTS.md updates clearly explain the daemon model
- Comprehensive Testing: 22 unit tests added for start command
- User Experience: Auto-starting daemon is a great UX improvement
- Service Persistence: systemd/launchd support is well-implemented
- Backwards Compatibility: HTTP transport still supported via flag
Test Status
Passing:
- Lint and Format ✅
- Python CodeQL ✅
- CLA Check ✅
- Socket Security ✅
Failed:
- Validate server.json ❌ (needs investigation)
- Sourcery review ❌ (code quality issues documented above)
Pending:
- Test Python 3.12 & 3.13 ⏳
- Build Docker Image ⏳
Recommendations
Before Merge:
- ✅ Fix systemd ExecStart quoting (security)
- ✅ Fix Docker CMD mismatch (functionality)
- ✅ Add
cwdparameter to subprocess calls - ✅ Fix
are_services_running()parameter handling ⚠️ Investigate server.json validation failure
Post-Merge Improvements:
- Refactor duplicate daemon startup code
- Extract
service()into smaller functions - Add end-to-end tests for
startcommand - Document command injection safety
Verdict
Requesting Changes ❌
This PR represents excellent work on a significant architectural improvement. However, the critical issues (systemd quoting, Docker configuration, and parameter mismatches) must be fixed before merging to avoid security and functionality problems in production.
Once the critical issues are addressed, this PR will be ready to merge. The code quality issues can be addressed in follow-up PRs if preferred.
Testing Checklist:
- Verify systemd service with spaces in path
- Test Docker container startup with default CMD
- Verify custom management host/port configuration
- Test daemon auto-start from stdio MCP clients
- Verify service persistence on Linux and macOS
There was a problem hiding this comment.
Pull Request Review: Default to stdio Transport and Background Daemon Mode
Summary
This PR introduces a significant architectural improvement to CodeWeaver by making stdio the default MCP transport (proxying to an HTTP backend daemon), implementing background daemon mode by default, and adding system service persistence. This is a well-thought-out enhancement that improves the user experience significantly.
✅ Strengths
1. Architecture & Design (Excellent)
- The daemon architecture is well-designed with clear separation of concerns:
- Background services (indexing, file watching, health monitoring)
- Management server (port 9329) for health/status/metrics
- MCP HTTP server (port 9328) for MCP protocol
- stdio proxy for universal MCP client compatibility
- Auto-starting daemon when stdio server is requested is a great UX improvement
- The decision to make stdio default while maintaining HTTP as an option provides flexibility
2. Code Quality (Very Good)
- Clean, well-structured implementation with proper separation in
start.py,main.py, andinit.py - Excellent error handling and fallback mechanisms (e.g., finding codeweaver executable or using python -m)
- Good use of async/await patterns throughout
- Platform-specific handling for Windows, Linux (systemd), and macOS (launchd)
3. Testing (Excellent)
- Comprehensive test coverage with 22 tests added in
test_start_command.py - Tests cover critical paths: background/foreground modes, daemon health checks, service installation
- Platform-specific tests properly marked with skipif decorators
- Good use of mocking to avoid actual system changes during tests
4. Documentation (Very Good)
- Extensive documentation updates across ARCHITECTURE.md, AGENTS.md, README.md, DOCKER.md
- Clear ASCII diagram showing the daemon architecture
- Updated CLI help text and examples to reflect new defaults
- Good inline code comments and docstrings
5. User Experience
- Background mode by default is the right choice
cw init servicecommand makes persistent installation straightforward- Clear messaging to users about the new architecture
- Backwards compatibility maintained with
--transport streamable-httpflag
⚠️ Issues Found
Critical: CI Failure - server.json Validation
The Validate server.json check is failing with:
❌ Validation failed with errors:
• Package 1: Missing required field: version
Issue: The OCI package (Docker) in server.json at line ~662 is missing the version field.
Location: server.json:662
{
"registryType": "oci",
"registryBaseUrl": "https://docker.io/",
"identifier": "knitli/codeweaver:0.1.0a2",
"runtimeHint": "docker",
// Missing: "version": "0.1.0a2",
"transport": {
"type": "stdio"
},
...
}Fix Required: Add the version field to the OCI package definition:
{
"registryType": "oci",
"registryBaseUrl": "https://docker.io/",
"identifier": "knitli/codeweaver:0.1.0a2",
"version": "0.1.0a2", // Add this line
"runtimeHint": "docker",
...
}🔍 Minor Observations
1. Process Detachment (Good)
The background daemon spawning logic in start.py:117-130 handles platform differences well:
- Windows:
CREATE_NEW_PROCESS_GROUP - Unix:
start_new_session=True - Proper redirection of stdin/stdout/stderr to DEVNULL
2. Health Check Timeout
The 30-second wait for daemon health (start.py:363) is reasonable, but consider adding a progress indicator for better UX during long startups.
3. Service Files
The generated systemd unit and launchd plist files are well-formed and follow best practices:
- Systemd: Uses
Type=simple,Restart=on-failure, logs to journal - Launchd: Uses
RunAtLoad,KeepAlive, proper log paths
🔒 Security Review
No security concerns identified.
- No credential handling issues
- No command injection vulnerabilities (proper subprocess usage)
- Service files don't expose sensitive information
- Environment variables properly documented as optional
⚡ Performance Considerations
Positive impacts:
- Single daemon shared across all MCP clients reduces resource usage
- Background indexing persists between sessions (major improvement)
- stdio proxy is lightweight and stateless
No performance concerns identified.
📋 Code Style & Consistency
Excellent adherence to project standards:
- Consistent use of type hints throughout
- Proper SPDX license headers
- Follows existing naming conventions
- Uses project's UI display utilities consistently
✅ Recommendation
Request Changes - Fix the server.json validation error, then approve.
This is a high-quality PR that represents a significant improvement to CodeWeaver's architecture and user experience. The implementation is solid, well-tested, and thoroughly documented. Once the server.json validation issue is fixed, this PR should be merged.
The architectural decision to default to stdio with daemon auto-start is excellent and aligns with modern MCP server best practices. The addition of service persistence (cw init service) makes production deployments much easier.
Great work! 🚀
Action Items
- ✅ Fix
server.jsonOCI package: Add"version": "0.1.0a2"field - ⏳ Wait for CI to pass
- ✅ Ready to merge after CI passes
PR Review: Default to stdio transport, background daemon mode, and service persistenceSummaryThis is an excellent architectural improvement that significantly enhances CodeWeaver's deployment model. The PR introduces a sophisticated daemon architecture with stdio as the default transport, automatic daemon management, and system service persistence. Architecture Changes ✅Daemon Architecture (Well-designed)
Default Transport Change (Good decision)
Code Quality ✅start.py (src/codeweaver/cli/commands/start.py:1-446)
init.py Service Installation (src/codeweaver/cli/commands/init.py:918-1313)
Test Coverage ✅ (tests/unit/cli/test_start_command.py:1-482)
Documentation ✅ARCHITECTURE.md updates are excellent:
README.md changes improve clarity:
Docker documentation appropriately distinguishes stdio vs HTTP for containerized deployments Issues Found 🔴1. CI Failure: server.json validation (BLOCKING) Location: Error: The Docker package is missing the required {
"registryType": "oci",
"registryBaseUrl": "https://docker.io/",
"identifier": "knitli/codeweaver:0.1.0a2",
// Missing: "version": "0.1.0a2",
"runtimeHint": "docker",
"transport": { "type": "stdio" }
}Fix required: Add Potential Improvements (Non-blocking)1. Error messaging for daemon conflicts
2. Daemon PID file
3. Service installation validation
4. Health check timeout configuration
5. Windows service support
Security Considerations ✅
Performance Considerations ✅
Breaking Changes
|
There was a problem hiding this comment.
PR Review: Default to stdio transport, background daemon mode, and service persistence
Overview
This is an excellent architectural improvement that introduces a proper daemon-based architecture with multiple transport modes. The PR is well-structured with 7 commits, comprehensive tests (22 passing), and thorough documentation updates.
Summary of Changes
✅ Architecture improvements:
- Default transport changed from HTTP to stdio (proxies to HTTP backend)
- Daemon auto-starts when stdio server is requested
- Background mode is now default for
cw start(with--foregroundflag) - New
cw init servicecommand for system service installation (systemd/launchd/Windows) cw start persistalias for convenience
✅ Code Quality:
- Clean separation of concerns between daemon, management server, and MCP server
- Well-structured async code with proper error handling
- Comprehensive test coverage (481 lines of new tests)
- Good use of dependency injection and mocking in tests
- Platform-specific handling (Linux/macOS/Windows) is well done
✅ Documentation:
- Excellent updates to ARCHITECTURE.md with ASCII diagrams
- Clear explanations in README.md, DOCKER.md, AGENTS.md
- Auto-generated CLI.md updated correctly
Issues Found
🔴 Critical: CI Failure - Missing version field in server.json
File: server.json:661-665
The OCI/Docker package definition is missing the required version field:
{
"registryType": "oci",
"registryBaseUrl": "https://docker.io/",
"identifier": "knitli/codeweaver:0.1.0a2",
"runtimeHint": "docker",
// MISSING: "version" field
}Fix: Add "version": "0.1.0a2" after the identifier field to match the PyPI package structure (line 17).
Validation error:
❌ Validation failed with errors:
• Package 1: Missing required field: version
Security Considerations
✅ Good practices:
- Daemon runs as user process (not root)
- Proper subprocess isolation with
start_new_session/CREATE_NEW_PROCESS_GROUP - No credential storage in service files
- Environment variables properly documented for sensitive data
✅ No security concerns identified - the implementation follows defensive security best practices.
Performance Considerations
✅ Efficient design:
- Background daemon reduces startup overhead for repeated MCP client connections
- Health check polling is reasonable (0.5s intervals, 30s timeout)
- Proper async/await usage throughout
- Subprocess spawning is non-blocking
Code Review Highlights
Excellent Patterns:
-
Clean command structure (src/codeweaver/cli/commands/start.py:71-136):
- Clear separation between background and foreground modes
- Proper fallback logic (cw → codeweaver → python -m codeweaver)
-
Robust health checking (src/codeweaver/cli/commands/start.py:138-176):
- Proper timeout handling
- Clear return values (bool)
- Good use of async/await
-
Comprehensive test coverage (tests/unit/cli/test_start_command.py):
- 22 tests covering all major functionality
- Platform-specific tests properly marked with
skipif - Good mocking patterns
Minor Suggestions:
-
Consider adding logging to background daemon (src/codeweaver/cli/commands/start.py:117-130):
Currently stdout/stderr are redirected to DEVNULL. Consider adding optional log file output for debugging:log_dir = Path.home() / ".codeweaver" / "logs" log_dir.mkdir(parents=True, exist_ok=True) log_file = log_dir / "daemon.log"
-
Error handling in service installation (src/codeweaver/cli/commands/init.py):
The Windows instructions function could return a bool for consistency with Linux/macOS handlers. -
Type hints (src/codeweaver/cli/commands/start.py:178-263):
Thestart_cw_servicesfunction could benefit from more specific return type annotation (currentlyNone, but raises on error).
Documentation Quality
✅ Excellent documentation updates:
- ARCHITECTURE.md v1.2.0 includes clear ASCII diagram of daemon architecture
- README.md properly updated with new workflow
- DOCKER.md explains both standalone and client-spawned modes
- All docstrings are clear and comprehensive
Testing
✅ Strong test suite:
- 22 new tests, all passing
- Good coverage of edge cases (already running, not running, timeout)
- Platform-specific tests properly isolated
- Mock usage is clean and appropriate
Breaking Changes
- Default transport changed from
streamable-httptostdio - Users must now run
cw startbefore MCP clients connect (though auto-start mitigates this) cw startnow runs in background by default (was foreground)
✅ Well handled: The PR includes clear migration guidance in documentation and init command output.
Recommendations
- MUST FIX before merge: Add missing
versionfield to OCI package in server.json:661 - SHOULD consider: Add optional daemon logging to a log file
- NICE TO HAVE: Make health check timeouts configurable
Conclusion
This is a high-quality PR that significantly improves CodeWeaver's architecture. The implementation is clean, well-tested, and thoroughly documented. The only blocking issue is the server.json validation failure.
Recommendation: Request changes to fix the server.json issue, then approve.
Files reviewed: 14 files (+1462/-135 lines)
Tests: 22 passing, 0 failing
CI Status: 2 failing (Sourcery, server.json validation), tests pending
Security: ✅ No concerns
Performance: ✅ Good design
There was a problem hiding this comment.
Pull Request Review: #184
Summary
This PR introduces a significant architectural improvement to CodeWeaver by implementing a daemon-based architecture with stdio as the default MCP transport. The changes are well-structured, thoroughly tested, and properly documented.
✅ Strengths
1. Architecture Design
- Excellent separation of concerns: Daemon (background services) + Management Server (port 9329) + MCP HTTP Server (port 9328) + stdio proxy
- Smart transport strategy: stdio proxy forwarding to HTTP backend provides both compatibility and shared state
- Auto-start capability: Daemon automatically starts when stdio server is requested if not running
- Background mode by default: Sensible default that improves UX
2. Code Quality
- Clean implementation in
start.py:71-136: Proper process spawning with platform-specific handling (Windows vs Unix) - Health checking in
main.py:194-223: Robust daemon health verification before proceeding - Error handling: Good exception handling throughout, especially in service installation functions
- Type hints: Proper use of type annotations for maintainability
3. Test Coverage
- 22 comprehensive unit tests added in
test_start_command.py - Tests cover:
- Background vs foreground modes
- Health check behavior (success and timeout)
- Service file generation (systemd, launchd)
- Platform-specific installation
- Executable discovery and fallback logic
- Good use of mocking for external dependencies
4. Documentation
- Excellent documentation updates in ARCHITECTURE.md with clear diagrams
- README updated with new workflow
- AGENTS.md reflects architectural changes
- DOCKER.md clarifies transport usage for containerized deployments
- Inline code comments are clear and helpful
5. User Experience
cw start persistalias for service installation is intuitive- Clear messaging and next-steps guidance in CLI output
- Seamless daemon management with health checks
⚠️ Areas for Improvement
1. CI Failure - server.json Validation
The "Validate server.json" check is failing. While the file content looks valid, this needs to be resolved before merging.
2. Security Considerations
Minor - Process Spawning (start.py:117-135):
- The daemon is spawned with all stdio streams redirected to DEVNULL, which is appropriate for a background daemon
- Command construction uses list format which prevents shell injection ✅
- Platform-specific process group handling is correct ✅
Recommendation: Consider adding a PID file to track the daemon process, which would help with:
- Preventing multiple daemon instances
- Clean shutdown management
- Process monitoring
3. Resource Management
main.py:314-327: Auto-start daemon feature
- Good: Checks if daemon is already running before starting
- Concern: No resource limits or throttling for daemon restarts
- Recommendation: Add a retry limit or backoff strategy to prevent infinite restart loops if daemon fails to start
4. Service Persistence (init.py:1047-1095)
Systemd service:
- ✅ Uses user-level service (
~/.config/systemd/user/) - ✅ Proper
Restart=on-failurewithRestartSec=5 ⚠️ NoRestartPreventExitStatusto distinguish fatal vs recoverable errors⚠️ No resource limits (CPUQuota, MemoryMax)
Recommendation: Consider adding:
[Service]
RestartPreventExitStatus=1 2 3 # Don't restart on fatal config errors
CPUQuota=200% # Limit to 2 cores
MemoryMax=2G # Reasonable limit for indexing serviceLaunchd service (init.py:1097-1145):
- ✅ Proper use of
KeepAlivewithSuccessfulExit=false - ✅ Log file paths configured
- Good: Environment variable support commented out for user customization
5. Minor Code Issues
main.py:254: Unclear error message
raise RuntimeError("No server tasks were created. This should not happen; please check server startup logic.")This should probably be more specific about what went wrong.
init.py:365: Redundant comment removal
The PR removes --transport stdio from default args (good!), but there's leftover comment about it.
📊 Technical Debt / Future Improvements
- Daemon lifecycle management: Consider using a proper daemon manager library (e.g.,
python-daemon) for more robust daemon behavior - Log rotation: The launchd service logs to fixed files without rotation configuration
- Windows service: Only provides instructions, doesn't automate installation like systemd/launchd
🔒 Security Assessment
Overall: APPROVED ✅
The code follows security best practices:
- No shell command injection vulnerabilities (uses list-based subprocess args)
- Proper file permissions for service files
- No hardcoded secrets or credentials
- Appropriate use of DEVNULL for daemon stdout/stderr
- Health check timeout prevents hanging
Minor recommendations above are for defense-in-depth, not critical issues.
🎯 Recommendation
APPROVE with minor conditions:
- ✅ Code quality: Excellent
- ✅ Test coverage: Comprehensive
- ✅ Documentation: Thorough and clear
- ✅ Architecture: Well-designed
⚠️ CI checks: Need to resolve the server.json validation failure⚠️ Tests pending: Wait for Python 3.12/3.13 test results
Action Items Before Merge
- Fix CI: Resolve the "Validate server.json" check failure
- Wait for tests: Ensure Python 3.12 and 3.13 tests pass
- Optional: Consider adding PID file management for daemon tracking
Conclusion
This is a high-quality PR that significantly improves CodeWeaver's architecture and user experience. The daemon-based approach with stdio transport is well-executed and properly tested. Once the CI issues are resolved, this is ready to merge.
Great work! 🚀
Reviewed by: Claude Code
Review Date: 2025-12-02
There was a problem hiding this comment.
Code Review - PR #184
Summary
This is a substantial and well-executed architectural improvement to CodeWeaver. The PR successfully transitions from HTTP-first to stdio-first transport, implements a robust daemon architecture, and adds service persistence capabilities. The implementation is thoughtful, well-tested, and properly documented.
Strengths
✅ Excellent Architecture Design
- Clean separation of concerns: daemon, management server, MCP HTTP server, and stdio proxy
- Smart auto-start mechanism for the daemon when stdio is requested
- Background mode as default is a great UX choice
✅ Comprehensive Testing
- 22 new unit tests covering all major functionality (start command, service persistence, health checks)
- Tests are well-structured with proper mocking and platform-specific handling
- Good coverage of edge cases (daemon already running, timeout scenarios, etc.)
✅ Outstanding Documentation
- Updated ARCHITECTURE.md with clear diagrams and explanations
- Consistent updates across README, DOCKER.md, and AGENTS.md
- Clear user guidance on the new workflow
✅ Code Quality
- Clean separation of background daemon spawning logic in
src/codeweaver/cli/commands/start.py:71-136 - Proper async/await patterns throughout
- Good error handling and user feedback
- Platform-specific handling for Windows vs Unix process spawning
✅ Service Persistence
- Well-implemented systemd and launchd service generation
- Proper working directory and restart policies
- Good Windows guidance (NSSM/Task Scheduler)
Issues Found
🔴 Critical: CI Failure in server.json
The Validate server.json check is failing because the second package (OCI Docker package) is missing the required version field:
File: server.json:662
The OCI package definition needs a version field added:
{
"registryType": "oci",
"registryBaseUrl": "https://docker.io/",
"identifier": "knitli/codeweaver:0.1.0a2",
"version": "0.1.0a2", // <-- ADD THIS
"runtimeHint": "docker",
"transport": {
"type": "stdio"
},
// ... rest of config
}This is preventing the PR from passing CI checks.
Recommendations
Code Improvements
-
Health Check Timeout Configuration - Consider making health check timeouts configurable via settings rather than hardcoded (30s in
src/codeweaver/main.py:228andsrc/codeweaver/cli/commands/start.py:142) -
Error Messages - In
src/codeweaver/main.py:324-327, the error message could be more helpful by checkingcw statusfirst to confirm daemon state -
Process Management - Consider adding PID file management to make daemon status checks more robust (currently relying solely on HTTP health checks)
Security & Performance
✅ Security
- No security concerns identified
- Proper subprocess spawning with detached sessions
- No credential exposure
✅ Performance
- Daemon architecture reduces startup overhead
- Shared indexing state across clients is efficient
- Background mode prevents blocking the user
Testing Notes
The test suite is comprehensive and well-designed:
tests/unit/cli/test_start_command.pycovers all major scenarios- Good use of fixtures and mocking
- Platform-specific tests properly marked with
skipif
Migration Path
The documentation provides clear guidance for users:
- Default behavior change is well communicated
- Backward compatibility maintained (can still use
--transport streamable-http) - Clear upgrade instructions in README
Verdict
Approve with required fix: This is excellent work that significantly improves CodeWeaver's architecture and UX. The only blocking issue is the missing version field in server.json which causes CI failure.
Action Required:
- Add
"version": "0.1.0a2"to the OCI package definition in server.json (line ~662) - Re-run CI to confirm the fix
Once the server.json validation passes, this PR is ready to merge.
Overall Assessment: This is a major architectural improvement executed with care, proper testing, and excellent documentation. Great work! 🎉
Code Review SummaryStatus: 🔴 Request Changes Critical Issue - Blockingserver.json validation failure: The OCI/Docker package definition is missing the required Location: Fix needed: {
"registryType": "oci",
"registryBaseUrl": "https://docker.io/",
"identifier": "knitli/codeweaver:0.1.0a2",
"version": "0.1.0a2", // <-- ADD THIS LINE
"runtimeHint": "docker",
...
}The validation error shows: While the Overall Assessment ✅Once the server.json issue is fixed, this is an excellent PR with: Strengths
Code Quality
Security 🔒
Minor recommendations:
Performance ⚡
Minor optimization: Consider exponential backoff in health check polling Architecture 🏗️The daemon architecture is well-designed with excellent separation of concerns. The stdio-to-HTTP proxy pattern is clever and provides great MCP client compatibility while maintaining stateful backend services. Action Items
Once the blocking issue is resolved, this will be ready to merge. Great work on this feature! |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/unit/cli/test_start_command.py
Outdated
| # Call persist with some test arguments | ||
| persist(enable=True, uninstall=False, project="test_project") | ||
| # Assert that init_service was called | ||
| assert mock_init_service.called |
There was a problem hiding this comment.
Missing blank line before class decorator. PEP 8 requires two blank lines before class definitions at module level.
| assert mock_init_service.called | |
| assert mock_init_service.called |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Escape special characters for XML content.""" | ||
| return ( | ||
| text.replace("&", "&") | ||
| .replace("<", "<") | ||
| .replace(">", ">") | ||
| .replace('"', """) | ||
| .replace("'", "'") | ||
| ) |
There was a problem hiding this comment.
The XML escaping function _escape_xml does not handle newlines or other control characters. While the current usage may not need it, XML should escape or handle control characters properly. Consider whether newlines in paths or command strings could cause issues, or add validation to reject such values.
| """Escape special characters for XML content.""" | |
| return ( | |
| text.replace("&", "&") | |
| .replace("<", "<") | |
| .replace(">", ">") | |
| .replace('"', """) | |
| .replace("'", "'") | |
| ) | |
| """Escape special characters and control characters for XML content.""" | |
| import re | |
| # Escape XML special characters | |
| text = ( | |
| text.replace("&", "&") | |
| .replace("<", "<") | |
| .replace(">", ">") | |
| .replace('"', """) | |
| .replace("'", "'") | |
| ) | |
| # Replace control characters (except tab, carriage return, line feed) with XML numeric character references | |
| def _control_char_replacer(match): | |
| ch = match.group(0) | |
| return f"&#x{ord(ch):X};" | |
| # Control chars: 0x00-0x08, 0x0B-0x0C, 0x0E-0x1F | |
| return re.sub(r"[\x00-\x08\x0B-\x0C\x0E-\x1F]", _control_char_replacer, text) |
| try: | ||
| # Keep services running until interrupted | ||
| if management_server.server_task: | ||
| await management_server.server_task | ||
| tasks_to_wait = [t for t in [management_server.server_task, mcp_server_task] if t] | ||
| if tasks_to_wait: | ||
| await asyncio.gather(*tasks_to_wait) | ||
| else: | ||
| # Shouldn't happen: server_task not set | ||
| raise RuntimeError("ManagementServer.server_task was not set. This should not happen; please check server startup logic.") | ||
| # Shouldn't happen: no server tasks set | ||
| raise RuntimeError("No server tasks were created. This should not happen; please check server startup logic.") | ||
| except (KeyboardInterrupt, asyncio.CancelledError): | ||
| display.print_warning("Shutting down background services...") | ||
| finally: | ||
| if mcp_server_task and not mcp_server_task.done(): | ||
| mcp_server_task.cancel() | ||
| with contextlib.suppress(asyncio.CancelledError): | ||
| await mcp_server_task |
There was a problem hiding this comment.
Potential race condition: Lines 214-228 create tasks for both management and MCP servers, but if the management server fails to start, the MCP server might still start. The error handling doesn't check if both servers started successfully before entering the wait loop. Consider adding validation that both servers are running before proceeding, or handle partial startup failures more gracefully.
src/codeweaver/cli/commands/init.py
Outdated
| [ | ||
| "Download NSSM from: https://nssm.cc/download", | ||
| "Open an Administrator Command Prompt", | ||
| f"Run: nssm install CodeWeaver \"{cw_cmd}\" start --foreground", |
There was a problem hiding this comment.
The Windows instructions in _show_windows_instructions suggest using NSSM with a command line that includes quotes (line 1191: nssm install CodeWeaver \"{cw_cmd}\" start --foreground). However, the escaping here is for Python string formatting, not shell escaping. The actual NSSM command would have literal \" characters. This should likely be:
f'Run: nssm install CodeWeaver "{cw_cmd}" start --foreground'or use raw string formatting to make the intent clearer.
| f"Run: nssm install CodeWeaver \"{cw_cmd}\" start --foreground", | |
| f'Run: nssm install CodeWeaver "{cw_cmd}" start --foreground', |
| # For stdio-only mode (MCP clients), use: codeweaver server | ||
| ENTRYPOINT ["/entrypoint.sh"] | ||
| CMD ["codeweaver", "server", "--host", "0.0.0.0", "--port", "9328"] | ||
| CMD ["codeweaver", "start", "--foreground"] |
There was a problem hiding this comment.
[nitpick] The Dockerfile changes the health check from port 9328 (MCP HTTP server) to port 9329 (management server), which makes sense. However, the default CMD is changed to run codeweaver start --foreground which starts both servers. The docker-compose.yml overrides this to use --transport streamable-http, but this creates inconsistency: the Dockerfile's default command doesn't actually expose a working MCP endpoint on 9328 in the way docker-compose expects. Consider whether the default CMD should explicitly start with HTTP transport for Docker usage, or update documentation to clarify the daemon mode in containers.
| CMD ["codeweaver", "start", "--foreground"] | |
| CMD ["codeweaver", "start", "--foreground", "--transport", "streamable-http"] |
| _start_daemon_background( | ||
| display, | ||
| config_file=Path("/path/to/config.toml"), | ||
| project=temp_project, | ||
| management_host="0.0.0.0", | ||
| management_port=9999, | ||
| mcp_host="0.0.0.0", | ||
| mcp_port=8888, | ||
| ) |
There was a problem hiding this comment.
[nitpick] In test_start_daemon_background_passes_options, the test passes a project parameter with value temp_project (line 166). However, in the actual spawn_daemon_process function, the working directory is only set if project.exists() (daemon.py line 103). Since this is a real temp directory that exists, the test will pass. But the test doesn't verify that the cwd kwarg was actually set in the Popen call. Consider adding an assertion to verify the working directory was properly passed.
| # The daemon includes both management server (9329) and MCP HTTP server (9328) | ||
| daemon_ready = await _start_daemon_if_needed( | ||
| management_host=host, | ||
| management_port=9329, # Management server port |
There was a problem hiding this comment.
The daemon auto-start uses hardcoded management port 9329 (line 222) but accepts a host parameter. However, the management server typically runs on the same host as the MCP HTTP server. If host is something other than "127.0.0.1" or "localhost", the hardcoded management port check may fail to detect a running daemon or start multiple daemons. Consider whether host should also be used for the management server health check.
| management_port=9329, # Management server port | |
| management_port=port, # Use the provided port for management server health check |
| start_mcp_http_server: bool = True, # Start MCP HTTP server for stdio proxy support | ||
| mcp_host: str | None = None, | ||
| mcp_port: PositiveInt | None = None, | ||
| ) -> None: | ||
| """Start background services using the new lifespan architecture.""" | ||
| """Start background services and optionally MCP HTTP server. | ||
| By default, starts both the management server (port 9329) and MCP HTTP server | ||
| (port 9328) to support stdio proxy connections. | ||
| """ |
There was a problem hiding this comment.
[nitpick] The comment on line 150 says "Start MCP HTTP server for stdio proxy support" but the parameter name is start_mcp_http_server which was previously unused/reserved for future use. The change to default to True is significant, but there's inconsistency: the function is called with the default True from background daemon spawn, but when called from foreground mode (line 303), it uses the default. Verify this is intentional - both foreground and background modes should start the MCP HTTP server for stdio support.
| if working_dir: | ||
| kwargs["cwd"] = working_dir | ||
| if sys.platform == "win32": | ||
| kwargs["creationflags"] = subprocess.CREATE_NEW_PROCESS_GROUP # type: ignore[attr-defined] |
There was a problem hiding this comment.
[nitpick] The CREATE_NEW_PROCESS_GROUP attribute is accessed from subprocess with a type ignore comment. However, CREATE_NEW_PROCESS_GROUP is actually defined in the subprocess module on Windows (typically with value 0x00000200). The type: ignore comment suggests there may be type checking issues. Consider importing it explicitly or using the numeric value with a comment explaining it's Windows-specific to avoid type checker warnings.
| kwargs["creationflags"] = subprocess.CREATE_NEW_PROCESS_GROUP # type: ignore[attr-defined] | |
| # 0x00000200 is the value of CREATE_NEW_PROCESS_GROUP in subprocess on Windows | |
| kwargs["creationflags"] = 0x00000200 # Windows-only: CREATE_NEW_PROCESS_GROUP |
PR Review: Default stdio transport, background daemon mode, and service persistenceOverviewThis PR introduces significant architectural improvements to CodeWeaver's daemon and MCP infrastructure. The changes are well-structured and address important usability concerns. Overall, this is excellent work with thoughtful design decisions and comprehensive testing. ✅ Strengths1. Architecture & Design
2. Code Quality
3. User Experience
4. Documentation
|
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
| from __future__ import annotations | ||
|
|
||
| import asyncio | ||
| import contextlib |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adam Poulemanos <89049923+bashandbone@users.noreply.github.com>
Pull Request Review: Daemon Architecture and stdio Transport DefaultSummaryThis is an excellent architectural improvement that introduces a daemon-based architecture with stdio as the default transport. The changes are well-designed and properly documented. However, there is one critical issue blocking approval. 🚨 Critical Issue - CI FailureIssue: The Error: Location: Fix Required: Add a ✅ Strengths1. Excellent Architecture Design
2. Security Improvements (src/codeweaver/cli/commands/init.py:958-1016)
3. Code Quality
4. User Experience
5. Default Transport Change
📋 Minor Observations1. Working Directory Handling (src/codeweaver/daemon.py:115)working_dir = project.resolve() if isinstance(project, Path) and project.exists() else None✅ Good: Validates path existence before setting working directory 2. Process Spawning (src/codeweaver/daemon.py:132)subprocess.Popen([cw_cmd, *cw_args], **kwargs) # noqa: S603✅ Good: Uses list form to avoid shell injection 3. Network Validation (src/codeweaver/cli/commands/start.py:230-246)
4. Error Handling
🔍 Code-Specific Commentssrc/codeweaver/cli/commands/start.py:333if not (project or not isinstance(project, Path) or not project.exists()):This logic seems potentially confusing. Consider simplifying to: if project and isinstance(project, Path) and project.exists():
# proceed
else:
display.print_warning(...)
returnsrc/codeweaver/daemon.py:66-69cw_cmd = sys.executable
cw_args = ["-m", "codeweaver", "start", "--foreground"]✅ Good fallback when 📊 Test Coveragetests/unit/cli/test_start_command.py: Comprehensive test suite
✅ 22 tests passing (1 skipped on platform mismatch) 🎯 Recommendations
📖 Documentation Quality
Final AssessmentOverall: This is a high-quality PR that represents a significant architectural improvement. The code is well-structured, properly tested, and thoroughly documented. The security improvements (path escaping) are excellent. Blocking Issue: The Recommendation: Request changes to fix the validation error, then approve. Action Items for PR Author
Once the validation issue is fixed, this PR will be ready to merge. Review conducted with focus on architecture, security, code quality, and adherence to best practices. |
There was a problem hiding this comment.
PR Review: Daemon Architecture and Stdio Transport
This PR introduces significant architectural improvements to CodeWeaver's MCP server infrastructure. The changes are well-designed and move the project toward a more robust daemon-based architecture. However, there are critical issues that must be fixed before merging, along with several important recommendations.
Critical Issues (Must Fix)
1. server.json validation failure ⛔
Location: server.json line ~103+
The CI is failing because the second package (OCI registry) is missing the required version field:
{
"registryType": "oci",
"identifier": "knitli/codeweaver:0.1.0a2",
"version": null // ❌ Must be a string, not null
}Fix: Add the version field:
{
"registryType": "oci",
"identifier": "knitli/codeweaver:0.1.0a2",
"version": "0.1.0a2"
}2. Subprocess security warnings ⚠️
Locations:
src/codeweaver/daemon.py:132- (Previously in start.py, now refactored)
The PR already addresses the previous Sourcery review comments by:
✅ Extracting shared daemon logic into daemon.py
✅ Using shlex.quote() for systemd service paths
✅ XML escaping for launchd plist paths
✅ Adding cwd parameter to subprocess.Popen
However, the subprocess.Popen([cw_cmd, *cw_args], **kwargs) call still triggers security warnings. While the code constructs the command from trusted sources (shutil.which + internal args), consider adding a comment explaining why this is safe to silence the warning:
# Security note: cw_cmd is resolved via shutil.which() from system PATH,
# and cw_args are constructed internally with validated parameters.
# No user-controlled input is passed to the subprocess.
subprocess.Popen([cw_cmd, *cw_args], **kwargs) # noqa: S603Important Issues (Should Fix)
3. Docker CMD inconsistency 🐳
Location: Dockerfile:114
The Dockerfile has been updated to run cw start --foreground by default, but docker-compose overrides this with explicit HTTP transport. This is good! However, the comment in docker-compose.yml could be clearer:
Current:
# Use HTTP transport for persistent service mode (daemon not needed in Docker)
command: ["codeweaver", "server", "--host", "0.0.0.0", "--port", "9328", "--transport", "streamable-http"]Suggestion: Clarify why HTTP is preferred in containerized environments (shared state, no need for stdio proxy when container is the daemon).
4. Test coverage gap 🧪
Location: tests/unit/cli/test_start_command.py
As noted by Sourcery, the tests cover argument handling and helper functions but don't test the main start() coroutine end-to-end. Consider adding integration-style tests that:
- Test the already-running path with custom host/port
- Test foreground mode invocation
- Test background mode with daemon spawn + health wait
- Test error handling (spawn failure, health timeout)
This would significantly increase confidence in the new architecture.
5. Potential host/port mismatch 🔌
Location: src/codeweaver/main.py:220-221
In _run_stdio_server, the function receives host parameter (intended for MCP HTTP server) but uses it for the management server health check:
daemon_ready = await _start_daemon_if_needed(
management_host=host, # ⚠️ This assumes management server uses same host as MCP
management_port=9329,
...
)Concern: If a user passes --host 0.0.0.0 to bind the MCP server publicly but the management server is only on 127.0.0.1, the health check will fail.
Recommendation: Either:
- Document that
hostparameter applies to both servers - Or add separate
management_hostparameter for stdio mode - Or always check management server on
127.0.0.1(localhost only)
Positive Highlights ✨
-
Excellent refactoring - The extraction of shared daemon logic into
daemon.pyis clean and eliminates duplication between start.py and main.py. -
Security improvements - The use of
shlex.quote()for systemd and XML escaping for launchd shows attention to security best practices. -
Comprehensive documentation - The updates to ARCHITECTURE.md with ASCII diagrams and rationale are excellent. The documentation clearly explains the new daemon architecture.
-
Auto-start behavior - The stdio transport auto-starting the daemon provides a seamless user experience.
-
Service persistence - The
cw init servicecommand for systemd/launchd is a great feature for production deployments. -
Test additions - 22 tests for the new start command functionality provide good coverage of the CLI interface.
Code Quality Observations
Architecture
- ✅ Clean separation of concerns (daemon, management server, MCP HTTP server, stdio proxy)
- ✅ Proper use of async/await throughout
- ✅ Good error handling and health checks
- ✅ Working directory support for daemon spawning
Code Style
- ✅ Consistent with project conventions
- ✅ Type hints throughout
- ✅ Clear docstrings
- ✅ Appropriate use of logging
Potential Improvements
- Consider adding a timeout parameter to
start_daemon_if_needed - The
NetworkConfigNamedTuple could be a dataclass for better extensibility - Consider extracting service installation logic into separate module (init.py is getting large at 1000+ lines)
Performance & Security
Performance: ✅ No concerns. The daemon architecture should improve performance by maintaining persistent state.
Security:
Recommendations
Before Merge (Required)
- ✅ Fix
server.jsonvalidation error (add version to OCI package) ⚠️ Add security comment to subprocess.Popen call- ✅ Ensure all CI checks pass (currently: CLA check failing, tests pending)
After Merge (Suggested)
- Add end-to-end integration tests for
start()coroutine - Document the host parameter behavior for stdio mode
- Consider adding a
cw restartcommand for convenience - Add telemetry/metrics for daemon uptime and stdio connections
Final Verdict
This is a well-architected and well-implemented PR that significantly improves CodeWeaver's architecture. The daemon-based approach with stdio proxy is the right design for MCP servers.
However, I cannot approve until the server.json validation error is fixed as this is blocking CI.
Once the server.json fix is committed and CI passes, this PR will be ready to merge. The other issues I've raised are important but not blocking.
Great work on this significant architectural improvement! 🎉
Next Steps:
- Fix
server.jsonOCI package version field - Wait for CI to pass
- Address subprocess security comment
- Consider test coverage improvements for future work
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 13 comments.
Comments suppressed due to low confidence (1)
src/codeweaver/cli/commands/start.py:17
- Import of 'contextlib' is not used.
import contextlib
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| settings_map = get_settings_map_for(config_file=config_file, project_path=project) | ||
| display = _display | ||
| error_handler = CLIErrorHandler(display, verbose=False, debug=False) | ||
|
|
||
| network_config = get_network_settings() | ||
| management_host = management_host or network_config.management_host | ||
| management_port = management_port or network_config.management_port | ||
| mcp_host = mcp_host or network_config.mcp_host | ||
| mcp_port = mcp_port or network_config.mcp_port | ||
| if ( | ||
| not _is_valid_host(management_host) | ||
| or not _is_valid_host(mcp_host) | ||
| or not _is_valid_port(management_port) | ||
| or not _is_valid_port(mcp_port) | ||
| ): | ||
| display.print_error("Invalid host or port provided. Please check your inputs.") | ||
| return | ||
| get_project_path = lazy_import("codeweaver.common.utils.git", "get_project_path") | ||
| project = ( | ||
| project or project_path | ||
| if (project_path := settings_map.get("project_path")) is not Unset | ||
| else get_project_path._resolve()() | ||
| ) |
There was a problem hiding this comment.
Potential issue with settings retrieval order. On line 311, get_settings_map_for is called with both config_file and project_path, but looking at the implementation in cli/utils.py, it only uses config_file if it's not None, otherwise checks project_path. This means if config_file is provided, the project parameter passed to this function is ignored in the settings retrieval.
Then on lines 328-332, the code tries to resolve the project path from settings or by auto-detection, but this creates a confusing flow where:
- Settings are loaded potentially without the right project context
- Then project is resolved
- But settings are never reloaded with the correct project
Consider either:
- Loading settings after resolving the project path, or
- Documenting this behavior clearly, or
- Ensuring settings are reloaded if project resolution differs from initial settings
| if (project_path := settings_map.get("project_path")) is not Unset | ||
| else get_project_path._resolve()() | ||
| ) | ||
| if not (project or not isinstance(project, Path) or not project.exists()): |
There was a problem hiding this comment.
The condition logic appears inverted. The condition not (project or not isinstance(project, Path) or not project.exists()) will be True when project is None or when it's not a Path or when it doesn't exist. However, the error message suggests we want to show the warning when there's NO valid project.
The correct logic should be:
if not (project and isinstance(project, Path) and project.exists()):This checks if project is falsy, OR not a Path, OR doesn't exist - any of which should trigger the warning.
| if not (project or not isinstance(project, Path) or not project.exists()): | |
| if not (project and isinstance(project, Path) and project.exists()): |
| } | ||
| if working_dir: | ||
| kwargs["cwd"] = working_dir | ||
| if sys.platform == "win32": |
There was a problem hiding this comment.
[nitpick] Missing error handling for Windows platform constant. The code uses subprocess.CREATE_NEW_PROCESS_GROUP which is only available on Windows. While there's a platform check (sys.platform == "win32"), the type: ignore[attr-defined] comment suggests mypy doesn't recognize this constant. This could cause issues if the constant is removed or renamed in future Python versions.
Consider importing the constant conditionally:
if sys.platform == "win32":
kwargs["creationflags"] = subprocess.CREATE_NEW_PROCESS_GROUP
else:
kwargs["start_new_session"] = TrueThis is already the current implementation, so the type ignore is appropriate, but documenting why it's needed would be helpful.
| if sys.platform == "win32": | |
| if sys.platform == "win32": | |
| # subprocess.CREATE_NEW_PROCESS_GROUP is only defined on Windows. | |
| # The type ignore is needed for mypy/static checkers on non-Windows platforms. |
src/codeweaver/daemon.py
Outdated
|
|
||
|
|
||
| async def check_daemon_health( | ||
| management_host: str = "127.0.0.1", management_port: int = 9329, timeout_at: float = 2.0 |
There was a problem hiding this comment.
The timeout_at parameter name is misleading. In HTTP clients, timeout typically refers to the duration to wait, not an absolute timestamp. Consider renaming to timeout or timeout_seconds for clarity.
| context: . | ||
| dockerfile: Dockerfile | ||
| container_name: codeweaver-server | ||
| # Use HTTP transport for persistent service mode (daemon not needed in Docker) |
There was a problem hiding this comment.
[nitpick] Incomplete documentation in comment. The comment states "Use HTTP transport for persistent service mode (daemon not needed in Docker)" but this is misleading. The daemon architecture is still relevant in Docker - it's just that in docker-compose mode, you're running the full daemon with the start --foreground command.
Consider clarifying:
# Use HTTP transport in docker-compose for direct HTTP connections
# The daemon runs in foreground mode, providing both management and MCP HTTP servers
command: ["codeweaver", "server", "--host", "0.0.0.0", "--port", "9328", "--transport", "streamable-http"]| # Use HTTP transport for persistent service mode (daemon not needed in Docker) | |
| # Use HTTP transport in docker-compose for direct HTTP connections | |
| # The daemon runs in foreground mode, providing both management and MCP HTTP servers |
| elapsed = 0.0 | ||
| while elapsed < max_wait_seconds: | ||
| await asyncio.sleep(check_interval) | ||
| elapsed += check_interval | ||
|
|
||
| if await check_daemon_health(management_host, management_port): | ||
| logger.info("Daemon started successfully") | ||
| return True |
There was a problem hiding this comment.
Race condition in daemon health check loop. The elapsed time is incremented by check_interval immediately after sleeping, but if the health check itself takes time, the actual elapsed time will be greater than calculated. This could cause the function to wait longer than max_wait_seconds.
Consider using time.monotonic() to track actual elapsed time:
import time
start_time = time.monotonic()
while time.monotonic() - start_time < max_wait_seconds:
await asyncio.sleep(check_interval)
if await check_daemon_health(management_host, management_port):
return True| This creates a stdio proxy that forwards MCP requests to an HTTP backend. | ||
| The HTTP backend should already be running via `codeweaver server`. | ||
| If the daemon is not running, it will be started automatically. |
There was a problem hiding this comment.
[nitpick] Incomplete sentence in docstring. The sentence "If the daemon is not running, it will be started automatically." is missing context about how it determines if the daemon is running. Consider expanding:
"""Run stdio MCP server (proxies to HTTP backend).
This creates a stdio proxy that forwards MCP requests to an HTTP backend.
If the daemon is not running, it will be started automatically by checking
the management server health endpoint.
Args:
...| If the daemon is not running, it will be started automatically. | |
| If the daemon is not running, it will be started automatically by checking | |
| the health of the management server (on the management port, typically 9329). | |
| If the management server is unresponsive, the daemon will be started and | |
| the function will wait until it becomes healthy before proceeding. |
| subprocess.run(["/bin/launchctl", "unload", str(plist_file)], capture_output=True) # noqa: S603 | ||
| # Load the service | ||
| subprocess.run( # noqa: S603 | ||
| ["/bin/launchctl", "load", str(plist_file)], check=True, capture_output=True |
There was a problem hiding this comment.
Potential command injection vulnerability. While the launchctl command uses a hardcoded path /bin/launchctl, the plist_file path could potentially contain special characters that, while escaped in the XML, may not be properly handled by the shell. The S603 suppression indicates this was flagged.
Consider using plist_file.resolve() to ensure it's an absolute path and add validation that the path doesn't contain unexpected characters, or use check=True and handle CalledProcessError appropriately (which is already done).
| async def test_start_detects_already_running( | ||
| self, temp_project: Path, capsys: pytest.CaptureFixture[str] | ||
| ) -> None: | ||
| """Test that start detects when services are already running.""" | ||
| from codeweaver.cli.commands.start import are_services_running | ||
|
|
||
| # Mock httpx at the httpx module level (it's lazily imported inside the function) | ||
| mock_client = AsyncMock() | ||
| mock_response = MagicMock() | ||
| mock_response.status_code = 200 | ||
| mock_client.get = AsyncMock(return_value=mock_response) | ||
| mock_client.__aenter__ = AsyncMock(return_value=mock_client) | ||
| mock_client.__aexit__ = AsyncMock(return_value=None) | ||
|
|
||
| with patch.dict( | ||
| "sys.modules", {"httpx": MagicMock(AsyncClient=MagicMock(return_value=mock_client))} | ||
| ): | ||
| result = await are_services_running("127.0.0.1", 9329) | ||
| assert result is True |
There was a problem hiding this comment.
Test doesn't verify actual mock call behavior. The test checks that are_services_running returns True when httpx returns a 200 status, but doesn't verify that:
- The correct URL was called (should be
http://127.0.0.1:9329/health) - The correct timeout was used
- Only one call was made
Consider adding assertions:
result = await are_services_running("127.0.0.1", 9329)
assert result is True
mock_client.get.assert_called_once_with(
"http://127.0.0.1:9329/health",
timeout=2.0 # or whatever the default is
)| mcp_server_task.cancel() | ||
| try: | ||
| await mcp_server_task | ||
| except asyncio.CancelledError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except asyncio.CancelledError: | |
| except asyncio.CancelledError: | |
| # Task cancellation is expected during shutdown; ignore this exception. |
Security fix for stop command that was incorrectly sending SIGTERM to itself rather than the daemon process. Changes: - Add /shutdown POST endpoint to management server for graceful termination - Add ManagementServer.request_shutdown() and is_shutdown_requested() methods - Add request_daemon_shutdown() and wait_for_daemon_shutdown() to daemon.py - Update stop command to use management API with proper host/port parameters - Add --timeout flag to stop command for configurable shutdown wait - Add actionable error messages with troubleshooting tips - Increase default health check timeout from 2s to 5s for cold starts - Update start_cw_services to monitor for shutdown requests - Fix tests for shlex.quote() path handling in systemd unit files
| task.cancel() | ||
| try: | ||
| await task | ||
| except asyncio.CancelledError: |
| mcp_server_task.cancel() | ||
| try: | ||
| await mcp_server_task | ||
| except asyncio.CancelledError: |
There was a problem hiding this comment.
PR Review: Daemon Architecture with stdio Transport (PR #184)
I've completed a comprehensive review of this PR. Overall, this is a well-architected change that significantly improves CodeWeaver's usability and deployment model. The implementation is solid with good security practices, though there are a few minor improvements recommended below.
✅ Strengths
-
Excellent Architecture Design
- Clean separation of concerns: daemon, management server (9329), MCP HTTP server (9328), and stdio proxy
- The daemon architecture is well-documented in ARCHITECTURE.md with clear ASCII diagrams
- Smart default of stdio transport that auto-starts the daemon provides seamless UX
-
Security Best Practices
- ✅ Uses
shlex.quote()for systemd service paths (src/codeweaver/cli/commands/init.py:976) - ✅ Uses XML escaping for launchd plist paths (init.py:993-999)
- ✅ Proper subprocess security with
subprocess.DEVNULLfor daemon spawning - ✅ Management API shutdown endpoint requires POST method (management.py:341)
- ✅ Uses
-
Code Quality
- Good extraction of shared daemon utilities into
daemon.pymodule (DRY principle) - Comprehensive unit tests added (22 tests covering start command behavior)
- Proper timeout handling throughout (health checks, daemon startup, shutdown)
- Excellent error messages with actionable troubleshooting tips (stop.py:108-127)
- Good extraction of shared daemon utilities into
-
Documentation
- Thorough updates to README, ARCHITECTURE.md, DOCKER.md, and AGENTS.md
- Clear migration path and usage instructions
- Good commit messages with detailed explanations
🔍 Minor Issues to Address
1. Empty Exception Handler (Priority: Low - Code Quality)
Location: src/codeweaver/cli/commands/start.py:212, 226
The bot correctly flagged empty except asyncio.CancelledError: pass blocks without explanatory comments. While the intent is clear from context, adding a brief comment improves code clarity:
except asyncio.CancelledError:
# Expected during graceful shutdown - task cancellation is intentional
pass2. Subprocess Security (Priority: Low - Already Mitigated)
Location: src/codeweaver/daemon.py:134
The # noqa: S603 disables the Bandit security warning for subprocess without shell. This is correctly handled (no shell=True, proper argument passing), but consider adding a comment explaining why this is safe:
subprocess.Popen([cw_cmd, *cw_args], **kwargs) # noqa: S603 - Safe: no shell, validated cmd path3. Broad Exception Catching (Priority: Low - Acceptable Pattern)
Location: src/codeweaver/daemon.py:52, 136, 230
Multiple instances of except Exception: are used. This is generally acceptable for:
- Health checks that should degrade gracefully (daemon.py:52)
- Process spawning that needs to return False on any failure (daemon.py:136)
However, consider logging the exception type for debugging: logger.debug("Health check failed: %s", type(e).__name__)
📊 Performance Considerations
- Daemon Startup Time: 30-second timeout for daemon health checks (daemon.py:147) is reasonable for cold starts with indexing
- Health Check Timeout: Increased from 2s to 5s (daemon.py:29) - good change for production reliability
- Shutdown Timeout: 30s default (stop.py:70) is appropriate for graceful cleanup
No performance concerns identified.
🔒 Security Assessment
✅ No security issues found. The PR demonstrates good security practices:
- No credential handling in this change
- Proper path escaping for system service files
- Management API restricted to localhost by default
- No shell injection vectors
- Subprocess spawning is properly sanitized
🏗️ Architecture Assessment
The new architecture is sound:
- Process isolation: Daemon runs separately from MCP transports
- Graceful degradation: stdio auto-starts daemon if needed
- Proper cleanup: Shutdown API triggers orderly termination
- Monitoring: Management server provides observability regardless of MCP transport
- Platform support: Handles Linux (systemd), macOS (launchd), and Windows (NSSM guidance)
📝 Recommendations
- ✅ Address the bot's comment: Add explanatory comments to empty exception handlers at line 212 and 226
- Consider adding a troubleshooting section to docs for common daemon issues (port conflicts, permission errors)
- Future enhancement: Add daemon log rotation configuration to systemd/launchd service files
✨ Conclusion
This is a high-quality PR that significantly improves CodeWeaver's architecture and user experience. The implementation is production-ready with proper error handling, security practices, and comprehensive documentation.
Recommendation: APPROVE ✅ pending the minor comment addition for the empty exception handlers.
The changes are well-tested, thoroughly documented, and follow Python best practices. The daemon architecture will make CodeWeaver much easier to deploy and use in production environments.
Review completed by Claude Code
Pull Request ReviewThis is a high-quality, well-architected PR that significantly improves CodeWeaver's daemon architecture and MCP transport handling. The stdio-first approach with auto-starting daemon will provide excellent UX for MCP clients. ✅ Strengths
🔴 Blocking Issueserver.json validation failure - The OCI/Docker package (Package 1, line 661) is missing the required Fix needed: {
"registryType": "oci",
"registryBaseUrl": "https://docker.io/",
"identifier": "knitli/codeweaver:0.1.0a2",
"version": "0.1.0a2", // Add this field
"runtimeHint": "docker",
...
}The CI validation check requires all packages to have 🟡 Other Issues
📋 SummaryArchitecture: ✅ Excellent - daemon design is sound with proper separation Status: Once the |
bashandbone
deleted the
claude/default-stdio-mcp-01BbnneBHSGU7YeXZiWSnid5
branch
This PR introduces several improvements to CodeWeaver's daemon and MCP architecture:
Default transport changed from HTTP to stdio - stdio now proxies to the HTTP backend, providing better compatibility with MCP clients
Daemon auto-starts when stdio server is requested (
cw server) if not running already - seamless experience without manual daemon managementBackground mode is now the default for cw start - use --foreground flag for blocking mode
New cw
init servicecommand - install CodeWeaver as a persistent system service (systemd on Linux, launchd on macOS, Windows guidance)Added
cw start persistalias - convenience alias for cw init serviceCommits
9f42101feat: change default MCP transport from HTTP to stdioaebc546feat: auto-start daemon when stdio server is requestedfe3677cfix: remove redundant --transport stdio from _create_stdio_config defaults0327decfeat: daemon runs in background by default, add service persistence6de88d2test: add unit tests for start command and service persistence7c84dccdocs: update README with start/stop commands and init service337ac63docs: document daemon architecture in AGENTS.md and ARCHITECTURE.mdTest plan
[x] Unit tests added for start command behavior (22 tests passing)
[x] Manual test: cw start runs daemon in background and returns
[x] Manual test: cw start --foreground runs in blocking mode
[x] Manual test: cw init service installs systemd/launchd service
[x] Manual test: cw init service --uninstall removes the service
[x] Manual test: stdio MCP transport works with MCP clients
(note: docs/CLI.md is auto-generated and will also be updated)
Summary by Sourcery
Default the MCP transport to stdio with an auto-starting daemon-backed architecture, introduce background daemon mode as the standard startup path, and add CLI support for installing CodeWeaver as a persistent system service.
New Features:
Enhancements:
Documentation:
Tests: