Skip to content

Conversation

@arabold
Copy link
Owner

@arabold arabold commented Feb 5, 2026

Summary

  • add openspec proposal for daemon management feature
  • include design decisions, requirements, and task breakdown
  • cover cross-platform service support and CLI subcommand structure

Testing

  • not run (proposal-only change)

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request adds a comprehensive proposal for implementing daemon management functionality in the docs-mcp-server. The proposal includes design decisions, technical specifications, and a detailed task breakdown for implementing system daemon/service management across macOS, Windows, and Linux platforms.

Changes:

  • Adds proposal document outlining the need for daemon management and the proposed CLI command structure
  • Includes design document with architectural decisions, including the use of node-windows/node-mac/node-linux packages and platform-agnostic abstraction layer
  • Provides detailed specification with requirements and scenarios for install, uninstall, start, stop, restart, status commands, plus cross-platform support and automatic restart behavior
  • Includes comprehensive task breakdown covering dependencies, core service module, CLI implementation, configuration, testing, documentation, and finalization

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 13 comments.

File Description
openspec/changes/add-daemon-management/proposal.md Outlines the motivation, proposed changes, command structure, platform support, and impact of adding daemon management CLI commands
openspec/changes/add-daemon-management/design.md Details architectural decisions including package selection, command structure, configuration approach, service abstraction, risks, and open questions
openspec/changes/add-daemon-management/specs/daemon-management/spec.md Defines requirements and scenarios for all daemon lifecycle operations (install, uninstall, status, start, stop, restart), cross-platform support, and automatic restart behavior
openspec/changes/add-daemon-management/tasks.md Provides a comprehensive implementation roadmap with 7 sections covering dependencies, core module development, CLI implementation, configuration, testing, documentation, and finalization

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- MUST use exponential backoff (default: 1 second initial, 25% growth)
- MUST cap restart attempts to prevent infinite loops (default: 5 restarts per 60 seconds)
- MUST log restart attempts

Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specification requires "exponential backoff (default: 1 second initial, 25% growth)" for restart behavior, but this requirement is not mentioned in the design document's Decision 1 or Risk 4 sections. The design.md should explain how this exponential backoff will be implemented using the node-windows/node-mac/node-linux packages, or if this is a deviation from their default behavior. Additionally, verify that these packages support exponential backoff with the specific parameters mentioned (1s initial, 25% growth), as they may use different restart delay mechanisms.

Suggested change
Implementation notes:
- The CLI SHALL use `node-windows`, `node-mac`, and `node-linux` for registering and managing the native services. These packages rely on the underlying service managers (Windows Service Manager, launchd, systemd/init.d) for restart policies and do not, by themselves, guarantee an exact “1 second initial, 25% growth” exponential backoff.
- To satisfy this requirement consistently across platforms, docs-mcp-server SHALL implement restart throttling at the application level (e.g., tracking crash history and applying delays that approximate a 1 second initial delay with 25% multiplicative growth) while configuring the OS service manager to restart the process as promptly as the platform allows.
- Where a platform’s service manager does not permit configuring backoff parameters to match the specification exactly, the implementation SHALL select the closest supported behavior and document any deviation and associated risks in `design.md` (see Decision 1 and Risk 4).

Copilot uses AI. Check for mistakes.
Comment on lines +51 to +73
### Requirement: Daemon Uninstallation

The system SHALL provide a CLI command to uninstall the docs-mcp-server daemon/service.

The uninstallation:
- MUST stop the service if running
- MUST remove the service registration from the operating system
- MUST remove service-related files (plist, wrapper executables, etc.)
- MUST NOT delete user data (database, configuration files, logs)
- MUST require elevated privileges

#### Scenario: Successful daemon uninstallation
- **GIVEN** the daemon is installed and running
- **WHEN** the user runs `docs-mcp-server daemon uninstall` with elevated privileges
- **THEN** the service is stopped
- **AND** the service registration is removed
- **AND** a success message is displayed

#### Scenario: Uninstallation when not installed
- **GIVEN** the daemon is not installed
- **WHEN** the user runs `docs-mcp-server daemon uninstall`
- **THEN** an informative message is displayed indicating no service is installed

Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tasks (line 36) mention adding a "confirmation prompt" for the uninstall command, but the specification doesn't include a scenario or requirement for user confirmation before uninstallation. Consider either adding a scenario to the spec that covers the confirmation flow (e.g., "Scenario: User cancels uninstallation via confirmation prompt"), or removing the confirmation prompt from the tasks if it's not a required feature.

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +35
#### Scenario: Successful daemon installation on macOS
- **GIVEN** the user has administrative privileges
- **WHEN** the user runs `docs-mcp-server daemon install --port 3000`
- **THEN** a launchd plist is created in `/Library/LaunchDaemons/`
- **AND** the service is started automatically
- **AND** a success message is displayed with service status

#### Scenario: Successful daemon installation on Windows
- **GIVEN** the user has administrative privileges
- **WHEN** the user runs `docs-mcp-server daemon install --port 3000`
- **THEN** a Windows Service is registered
- **AND** the service is started automatically
- **AND** a success message is displayed with service status

#### Scenario: Successful daemon installation on Linux
- **GIVEN** the user has root privileges
- **WHEN** the user runs `docs-mcp-server daemon install --port 3000`
- **THEN** a systemd unit or init.d script is created
- **AND** the service is started automatically
- **AND** a success message is displayed with service status
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The macOS installation scenario specifies the exact file location ("/Library/LaunchDaemons/") where the plist is created, but the Windows and Linux scenarios don't specify equivalent implementation details. For consistency, either all three scenarios should specify file/registry locations, or none should (focusing instead on the observable behavior that "the service is registered with the system"). Consider whether this level of detail is necessary in the specification or if it's better suited for implementation documentation.

Copilot uses AI. Check for mistakes.
| Platform | Mechanism | Service Location |
|----------|-----------|------------------|
| macOS | launchd | `/Library/LaunchDaemons/` (plist) |
| Windows | Windows Service Manager | Windows Services (winsw) |
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table mentions "(winsw)" for Windows, which is an implementation detail. Since this is a proposal document describing the user-facing feature, consider removing the "(winsw)" reference to keep the focus on what the feature does rather than how it's implemented internally.

Suggested change
| Windows | Windows Service Manager | Windows Services (winsw) |
| Windows | Windows Service Manager | Windows Services |

Copilot uses AI. Check for mistakes.
- `DOCS_MCP_STORE_PATH` from --store-path
- `DOCS_MCP_RESUME` from --resume
- `DOCS_MCP_READ_ONLY` from --read-only
- [ ] 4.2 Ensure `loadConfig` in server startup respects these env vars
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tasks suggest using a DOCS_MCP_RESUME environment variable for the daemon, but there is currently no environment variable mapping for the resume flag in the config system (see src/utils/config.ts lines 300-344). Before implementing the daemon feature, task 4.2 should also include adding a config mapping for the resume flag to ensure it can be properly configured via environment variables. This would align with the existing pattern used for other flags like DOCS_MCP_READ_ONLY and DOCS_MCP_PORT.

Suggested change
- [ ] 4.2 Ensure `loadConfig` in server startup respects these env vars
- [ ] 4.2 Add a config/env mapping for the `resume` flag (`DOCS_MCP_RESUME`) in `loadConfig` (see `src/utils/config.ts` alongside `DOCS_MCP_READ_ONLY`/`DOCS_MCP_PORT`), and ensure server startup respects all of these env vars

Copilot uses AI. Check for mistakes.
- **GIVEN** the system is running on Linux
- **WHEN** any daemon command is executed
- **THEN** the system uses systemd or init.d based service management

Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design document's Risk 2 (line 125) and tasks.md (line 47) mention WSL detection and guidance, but there is no corresponding requirement or scenario in the specification. Consider adding a scenario under the Cross-Platform Support requirement that describes the expected behavior when the daemon commands are run in a WSL environment. This would ensure the WSL edge case is properly specified, not just mentioned in the design and tasks.

Suggested change
#### Scenario: Platform detection on WSL
- **GIVEN** the system is running inside Windows Subsystem for Linux (WSL)
- **WHEN** any daemon command is executed
- **THEN** the system detects that it is running in WSL
- **AND** the system does not attempt to register a native Linux daemon inside WSL
- **AND** the system presents clear guidance to the user on how to install and manage the daemon as a native Windows service instead

Copilot uses AI. Check for mistakes.
The daemon management system SHALL support macOS, Windows, and Linux operating systems.

- On macOS, the system MUST use launchd for service management
- On Windows, the system MUST use Windows Service Manager (via winsw)
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The specification mentions "winsw" (Windows Service Wrapper) which is an implementation detail of the node-windows package. The specification should describe what the system does from a user's perspective, not how it's internally implemented. Consider removing the "(via winsw)" reference and simply stating "Windows Service Manager".

Suggested change
- On Windows, the system MUST use Windows Service Manager (via winsw)
- On Windows, the system MUST use Windows Service Manager

Copilot uses AI. Check for mistakes.
- `DOCS_MCP_STORE_PATH` from --store-path
- `DOCS_MCP_RESUME` from --resume
- `DOCS_MCP_READ_ONLY` from --read-only
- [ ] 4.2 Ensure `loadConfig` in server startup respects these env vars
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Task 4.2 states "Ensure loadConfig in server startup respects these env vars" which is vague. The loadConfig function in src/utils/config.ts already has a system for mapping environment variables to config values. This task should be more specific about what needs to be verified or modified. If new environment variable mappings need to be added (like DOCS_MCP_RESUME), that should be called out as a separate, concrete task in this section or in section 2.

Suggested change
- [ ] 4.2 Ensure `loadConfig` in server startup respects these env vars
- [ ] 4.2 Verify `loadConfig` in server startup reads and maps each of the above `DOCS_MCP_*` env vars to the correct config fields, and add tests covering this behavior. If any mapping is missing in `src/utils/config.ts`, add a separate, concrete task (e.g., in section 2) to implement that mapping.

Copilot uses AI. Check for mistakes.
```
src/daemon/
├── DaemonService.ts # Main abstraction class
├── types.ts # DaemonOptions, DaemonStatus interfaces
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The directory structure shown here is missing the barrel export file (index.ts) that is mentioned in tasks.md section 2.3. Consider adding "├── index.ts # Barrel export" to this structure for completeness and consistency with the task breakdown.

Suggested change
├── types.ts # DaemonOptions, DaemonStatus interfaces
├── types.ts # DaemonOptions, DaemonStatus interfaces
├── index.ts # Barrel export

Copilot uses AI. Check for mistakes.

### Risk 4: Service Recovery Loops
- **Risk**: If the server has a fatal bug, the service might restart endlessly.
- **Mitigation**: Use the built-in `maxRestarts` and `maxRetries` configuration to cap restart attempts. Default to sensible limits (e.g., 5 restarts in 60 seconds).
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor terminology inconsistency: The spec (line 191) uses "5 restarts per 60 seconds" while the design uses "5 restarts in 60 seconds". Consider using "per" instead of "in" to match the spec's terminology.

Suggested change
- **Mitigation**: Use the built-in `maxRestarts` and `maxRetries` configuration to cap restart attempts. Default to sensible limits (e.g., 5 restarts in 60 seconds).
- **Mitigation**: Use the built-in `maxRestarts` and `maxRetries` configuration to cap restart attempts. Default to sensible limits (e.g., 5 restarts per 60 seconds).

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant