Harden persistent MCP runtime, recovery, and stdio isolation - #362
Closed
andrebarros78 wants to merge 6 commits into
Closed
Harden persistent MCP runtime, recovery, and stdio isolation#362andrebarros78 wants to merge 6 commits into
andrebarros78 wants to merge 6 commits into
Conversation
# Conflicts: # src/windows_mcp/__main__.py # src/windows_mcp/uia/controls.py
PR Summary by QodoHarden persistent MCP runtime supervision and stdio tool isolation (Windows)
AI Description
Diagram
High-Level Assessment
Files changed (55)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
16 rules 1. Hardcoded project root
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| PROFILE_DIR = ROOT / ".tunnel-client" / "profiles" | ||
| PROFILE_NAME = "windows-mcp-gpt-managed" | ||
| PROFILE_FILE = PROFILE_DIR / f"{PROFILE_NAME}.yaml" | ||
| EXPECTED_MCP_COMMAND = "D:/Projetos/WINDOWS-MCP-TEST/.venv/Scripts/python.exe -m windows_mcp serve --transport stdio" |
There was a problem hiding this comment.
1. Long expected_mcp_command line 📘 Rule violation ✧ Quality
The EXPECTED_MCP_COMMAND assignment exceeds the 100-character maximum line length. This reduces readability and violates the configured formatting requirement.
Agent Prompt
## Issue description
A newly added non-comment line exceeds 100 characters.
## Issue Context
The compliance checklist requires a strict 100-character maximum line length for changed files.
## Fix Focus Areas
- scripts/windows_mcp_supervisor.py[40-40]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+26
to
+27
| _CREATE_NO_WINDOW = getattr(subprocess, 'CREATE_NO_WINDOW', 0) | ||
| _CREATE_NEW_PROCESS_GROUP = getattr(subprocess, 'CREATE_NEW_PROCESS_GROUP', 0) |
There was a problem hiding this comment.
2. Single quotes in getattr 📘 Rule violation ✧ Quality
New code uses single-quoted string literals in getattr(...) calls. This violates the requirement to use double quotes for all string literals.
Agent Prompt
## Issue description
New/modified string literals must use double quotes, but single quotes are used.
## Issue Context
The compliance checklist enforces consistent quoting for all string literals.
## Fix Focus Areas
- src/windows_mcp/tools/system_query.py[26-27]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+14
to
+20
| def parse_rect(value: str | None) -> Rect | None: | ||
| if not value: | ||
| return None | ||
| parts = [int(part.strip()) for part in value.split(",")] | ||
| if len(parts) != 4: | ||
| raise ValueError("rect must contain left,top,right,bottom") | ||
| return Rect(*parts) |
There was a problem hiding this comment.
3. parse_rect() missing docstring 📘 Rule violation ✧ Quality
The new public function parse_rect has no Google-style docstring as its first statement. This violates the requirement for structured docstrings on public functions/classes.
Agent Prompt
## Issue description
Public functions/classes must have Google-style docstrings, but `parse_rect` is missing one.
## Issue Context
The compliance checklist requires a triple-quoted docstring with Google-style sections (e.g., `Args:`, `Returns:`) for public APIs.
## Fix Focus Areas
- src/windows_mcp/desktop/screenshot_worker.py[14-20]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+344
to
+373
| def register( | ||
| mcp: Any, | ||
| *, | ||
| get_desktop: Callable[[], Any], | ||
| get_analytics: Callable[[], Any], | ||
| ) -> None: | ||
| @mcp.tool( | ||
| name="SystemQuery", | ||
| description=( | ||
| "Structured read-only Windows and project query. Use instead of PowerShell for date/time, " | ||
| "file listing, ordinary text-file reads, process or service status, Git status, program " | ||
| "versions, tunnel/runtime status, and log tails. It does not accept arbitrary commands, " | ||
| "does not write, blocks secret-like paths, and keeps all file access inside the project." | ||
| ), | ||
| annotations=ToolAnnotations( | ||
| title="Safe System Query", | ||
| readOnlyHint=True, | ||
| destructiveHint=False, | ||
| idempotentHint=True, | ||
| openWorldHint=False, | ||
| ), | ||
| ) | ||
| @with_analytics(get_analytics(), "SystemQuery-Tool") | ||
| def system_query_tool( | ||
| operation: str, | ||
| target: str | None = None, | ||
| limit: int = 50, | ||
| ctx: Context = None, | ||
| ) -> object: | ||
| return execute_query(operation, target, limit) |
There was a problem hiding this comment.
4. systemquery tool bypasses service 📘 Rule violation ⌂ Architecture
The SystemQuery MCP entry-point tool delegates to execute_query within the tool module, which performs direct OS/filesystem/process interrogation instead of delegating to a Desktop service layer. This violates the required layering for MCP entry-point tools.
Agent Prompt
## Issue description
MCP entry-point tools must be thin adapters that delegate business/OS logic to the Desktop service layer, but `SystemQuery` implements/owns the logic in the tool module.
## Issue Context
The compliance checklist requires entry-point tools to limit themselves to input parsing/validation and service calls, avoiding direct platform access and branching logic.
## Fix Focus Areas
- src/windows_mcp/tools/system_query.py[280-337]
- src/windows_mcp/tools/system_query.py[344-373]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+17
to
+41
| ROOT = Path(r"D:\Projetos\WINDOWS-MCP-TEST") | ||
| STATE_ROOT = ROOT / ".orquestrador" / "supervisor" | ||
| CHECKPOINT = ROOT / ".orquestrador" / "checkpoints" / "runtime-supervision.json" | ||
| LEGACY_CHECKPOINTS = ( | ||
| ROOT / ".orquestrador" / "checkpoints" / "loop-280h.json", | ||
| ROOT / ".orquestrador" / "checkpoints" / "loop-15h.json", | ||
| ) | ||
| LOCK_FILE = STATE_ROOT / "lock.json" | ||
| STATE_FILE = STATE_ROOT / "state.json" | ||
| QUEUE_FILE = STATE_ROOT / "queue.json" | ||
| INBOX_DIR = STATE_ROOT / "inbox" | ||
| REJECTED_INBOX_DIR = STATE_ROOT / "inbox-rejected" | ||
| HEARTBEAT_FILE = STATE_ROOT / "heartbeat.json" | ||
| LOG_DIR = STATE_ROOT / "logs" | ||
| LOG_FILE = LOG_DIR / "supervisor.log" | ||
| TASK_EVIDENCE_DIR = STATE_ROOT / "task-evidence" | ||
| TASK_SPEC_DIR = STATE_ROOT / "task-specs" | ||
| TASK_RESULT_DIR = STATE_ROOT / "task-results" | ||
| TASK_RUNNER = ROOT / "scripts" / "run_supervisor_task.py" | ||
| TUNNEL_BIN = ROOT / ".tunnel-client" / "bin" / "tunnel-client.exe" | ||
| PROFILE_DIR = ROOT / ".tunnel-client" / "profiles" | ||
| PROFILE_NAME = "windows-mcp-gpt-managed" | ||
| PROFILE_FILE = PROFILE_DIR / f"{PROFILE_NAME}.yaml" | ||
| EXPECTED_MCP_COMMAND = "D:/Projetos/WINDOWS-MCP-TEST/.venv/Scripts/python.exe -m windows_mcp serve --transport stdio" | ||
| MCP_CONNECTION_MAX_TTL = "336h" |
There was a problem hiding this comment.
5. Hardcoded project root 🐞 Bug ≡ Correctness
The supervisor/task scripts hardcode ROOT (and EXPECTED_MCP_COMMAND) to D:\Projetos\WINDOWS-MCP-TEST, so they will read/write state and attempt to spawn the tunnel/MCP from a non-existent path on other installations. This makes the persistent runtime tooling non-portable and can cause immediate operational failure outside that exact directory layout.
Agent Prompt
## Issue description
Operational supervisor scripts hardcode a workstation-specific project root and Python command path. This will break the supervisor, queue inbox, and managed command enforcement when the repo/venv is not located at `D:\Projetos\WINDOWS-MCP-TEST`.
## Issue Context
These scripts appear intended for production resilience/recovery, so they should either (a) derive paths dynamically from the current checkout / script location, or (b) accept an explicit `--project-root` / env var and validate it.
## Fix Focus Areas
- scripts/windows_mcp_supervisor.py[17-52]
- scripts/enqueue_supervisor_task.py[11-20]
## Suggested fix
- Replace `ROOT = Path(r"D:\Projetos\WINDOWS-MCP-TEST")` with something like:
- `ROOT = Path(os.environ.get("WINDOWS_MCP_PROJECT_ROOT") or Path(__file__).resolve().parents[1]).resolve()`
- Derive `PYTHON`, `TASK_RUNNER`, and `EXPECTED_MCP_COMMAND` from `ROOT` (and/or `sys.executable`) instead of embedding absolute paths.
- Add validation that computed files exist (venv python, tunnel-client.exe, profiles dir) and emit a clear error if not.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+161
to
+190
| def ensure_profile_command() -> dict[str, Any]: | ||
| try: | ||
| data = json.loads(PROFILE_FILE.read_text(encoding="utf-8-sig")) | ||
| mcp = data.setdefault("mcp", {}) | ||
| commands = mcp.get("commands") | ||
| expected = [{"channel": "main", "command": EXPECTED_MCP_COMMAND}] | ||
| current = commands if isinstance(commands, list) else [] | ||
| current_ok = current == expected | ||
| ttl_ok = str(mcp.get("connection_max_ttl") or "") == MCP_CONNECTION_MAX_TTL | ||
| forbidden_uv = any( | ||
| "uv.exe" in normalize_command(item.get("command", "")) | ||
| for item in current | ||
| if isinstance(item, dict) | ||
| ) | ||
| if current_ok and ttl_ok and not forbidden_uv: | ||
| return { | ||
| "ok": True, | ||
| "repaired": False, | ||
| "command": EXPECTED_MCP_COMMAND, | ||
| "connection_max_ttl": MCP_CONNECTION_MAX_TTL, | ||
| "error": "", | ||
| } | ||
| PROFILE_BACKUP_DIR.mkdir(parents=True, exist_ok=True) | ||
| stamp = datetime.now().astimezone().strftime("%Y%m%d-%H%M%S-%f") | ||
| backup = PROFILE_BACKUP_DIR / f"{PROFILE_NAME}.{stamp}.json" | ||
| backup.write_text(json.dumps(data, ensure_ascii=False, indent=2), encoding="utf-8") | ||
| mcp["commands"] = expected | ||
| mcp["connection_max_ttl"] = MCP_CONNECTION_MAX_TTL | ||
| atomic_json(PROFILE_FILE, data) | ||
| log( |
There was a problem hiding this comment.
6. Profile format mismatch 🐞 Bug ☼ Reliability
ensure_profile_command() parses and overwrites a *.yaml tunnel profile using json.loads/json.dumps, so any YAML profile that is not strict JSON syntax (e.g., comments or typical YAML scalars) will be rejected and the supervisor will refuse to start/recover the tunnel. The file extension and parser/writer mismatch also makes the on-disk profile format misleading for operators and tooling.
Agent Prompt
## Issue description
The tunnel profile is stored as `*.yaml` but the supervisor reads/writes it as JSON (`json.loads`, `json.dumps`). This will fail for YAML profiles that contain YAML-only constructs, and it also misleads operators/tools about the file format.
## Issue Context
JSON is a subset of YAML, so JSON text inside a `.yaml` file can work, but the current implementation makes the supervisor brittle to legitimate YAML formatting.
## Fix Focus Areas
- scripts/windows_mcp_supervisor.py[39-40]
- scripts/windows_mcp_supervisor.py[161-190]
## Suggested fix options
1) **Preferable:** Parse and write YAML consistently (e.g., `yaml.safe_load` / `yaml.safe_dump`) and keep `.yaml`.
2) **Alternative (no YAML dep):** Rename the profile to `.json` everywhere (including any tunnel-client expectations/docs/scripts) and keep JSON parsing/writing.
Also ensure the migration scripts/ops docs use the same format choice to avoid split-brain behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+219
to
+224
| def acquire_single_instance() -> None: | ||
| existing = load_json(LOCK_FILE, {}) | ||
| existing_pid = existing.get("pid") if isinstance(existing, dict) else None | ||
| if pid_alive(existing_pid): | ||
| raise SystemExit(0) | ||
| atomic_json(LOCK_FILE, {"pid": os.getpid(), "started_at": now_iso()}) |
There was a problem hiding this comment.
7. Racy supervisor lock 🐞 Bug ☼ Reliability
acquire_single_instance() uses a read-then-write PID file without an OS-level lock, so two supervisors starting concurrently can both pass the check and run at the same time. This violates the documented exclusivity requirement and risks duplicated tunnel management and conflicting state writes.
Agent Prompt
## Issue description
The supervisor attempts single-instance behavior via `lock.json`, but the implementation is not atomic: two processes can simultaneously observe no live PID and both proceed.
## Issue Context
This supervisor is intended to be the sole manager of the persistent runtime; concurrent supervisors can race on queue/state/heartbeat and tunnel restarts.
## Fix Focus Areas
- scripts/windows_mcp_supervisor.py[219-225]
- docs/resilience-operations.md[18-22]
## Suggested fix
- Use a Windows-enforced mutual exclusion primitive:
- A named mutex via `ctypes` (`CreateMutexW` + `GetLastError()==ERROR_ALREADY_EXISTS`), or
- An exclusive lock file handle kept open for process lifetime (e.g., `msvcrt.locking` or `CreateFile` with exclusive sharing).
- Keep writing diagnostic info (PID/timestamps) for observability, but make the *exclusivity decision* depend on the OS-level lock rather than a read/replace JSON file.
- Consider PID reuse/stale locks: on startup, if lock exists but owner process is gone, allow takeover; otherwise exit cleanly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
|
Pls the make the pr into multiple ones so that it is easy for me to go through it |
Author
|
Thank you for the review and the feedback. I will close this PR and reorganize the changes into smaller, focused pull requests to make the review easier. Thanks! |
Member
|
Also pls create an issuse stating the problem you faced the the fix you proposing then make pr because people can help you |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Operational note
Screenshot capture remains safely quarantined when the Windows graphics session cannot support isolated capture. The failure is localized and does not terminate the MCP server or tunnel.