Conversation
PATCH release: fix(windows) env-aware management scripts + reliable stdout logging (PR #682) + Gemini review feedback (commit 6cc7752). - _version.py, pyproject.toml, pyproject-lite.toml: 10.36.1 -> 10.36.2 - CHANGELOG.md: new [10.36.2] section, [Unreleased] preserved - README.md: Latest Release updated to v10.36.2 - CLAUDE.md: Current Version callout updated - uv.lock: regenerated Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the project to version 10.36.2, primarily addressing issues in Windows management scripts. It introduces environment-aware URL configuration, ensures reliable capture of Python stdout/stderr, and implements log rotation to prevent overwriting crash data. Feedback suggests enhancing the log rotation strategy to maintain multiple historical logs for better diagnostics during rapid crash loops and correcting a non-greedy regex in the .env parser to ensure configuration values are properly captured.
|
|
||
| - **[#682] Hardcoded server URLs in Windows management scripts**: All 5 Windows PowerShell management scripts (`manage_service.ps1`, `run_http_server_background.ps1`, `install_scheduled_task.ps1`, `uninstall_scheduled_task.ps1`, `update_and_restart.ps1`) used a hardcoded `http://127.0.0.1:8000` URL regardless of `.env` configuration. Health checks failed silently as soon as a user enabled HTTPS (`MCP_HTTPS_ENABLED=true`) or changed the port (`MCP_HTTP_PORT`). Introduced `scripts/service/windows/lib/server-config.ps1` as a shared helper that parses `.env` for `MCP_HTTP_HOST`, `MCP_HTTP_PORT`, and `MCP_HTTPS_ENABLED` and returns a `BaseUrl`/`HealthUrl` hashtable. All scripts now dot-source this helper. Falls back to `http://127.0.0.1:8000` if `.env` is absent or variables are unset — fully backward-compatible. (PR #682) | ||
| - **[#682] Silent Python stdout/stderr dropout in background server wrapper**: `run_http_server_background.ps1` used .NET `OutputDataReceived` event handlers that referenced `$script:LogFile`, a variable not captured in the event handler runspace. Every line of Python stdout and stderr was silently discarded. When Python crashed during initialization, no error was ever surfaced in the log. Replaced with `Start-Process -RedirectStandardOutput`/`-RedirectStandardError` which writes directly to `http-server-python.log` and a `.err` sidecar, eliminating the runspace capture problem entirely. (PR #682) | ||
| - **[#682] Log overwrite destroys crash evidence in restart loop**: `Start-Process` overwrites the redirect target file on each invocation. The previous size-based rotation (`> 10MB`) meant that on a crash-restart cycle, the crash output from attempt N was silently destroyed by attempt N+1 before it could be inspected. Rotation is now unconditional at the start of each loop iteration: the current log is renamed to `.old` before `Start-Process` is called, preserving the most recent crash output. (PR #682, Gemini review feedback) |
There was a problem hiding this comment.
The log rotation strategy described (renaming the current log to .old at the start of each iteration) only preserves the single most recent log file. In a scenario where the service enters a rapid crash-restart loop, the log containing the initial or most relevant crash evidence will be overwritten by subsequent restart attempts. A more robust approach would be to use timestamped log files or maintain a rotating pool of multiple historical logs (e.g., .log.1, .log.2) to ensure diagnostic data is not lost during extended failure periods.
|
|
||
| ### Changed | ||
|
|
||
| - **[#682] `.env` regex strips trailing inline comments**: The `.env` parser in `server-config.ps1` now uses `([^#\r\n]*?)` to capture values, so `MCP_HTTP_PORT=8001 # my custom port` parses correctly as `8001` rather than failing `[int]::TryParse`. (PR #682, Gemini review feedback) |
There was a problem hiding this comment.
The regex ([^#\r\n]*?) described for capturing .env values uses a non-greedy quantifier (*?). Without an anchor or subsequent required characters in the pattern, a unanchored non-greedy match will prefer the shortest possible match, which is zero characters. This will likely result in empty strings being captured for configuration variables. Consider using a greedy quantifier ([^#\r\n]*) to ensure the full value is captured up to the comment or newline.
| - **[#682] `.env` regex strips trailing inline comments**: The `.env` parser in `server-config.ps1` now uses `([^#\r\n]*?)` to capture values, so `MCP_HTTP_PORT=8001 # my custom port` parses correctly as `8001` rather than failing `[int]::TryParse`. (PR #682, Gemini review feedback) | |
| - **[#682] .env regex strips trailing inline comments**: The .env parser in server-config.ps1 now uses ([^#\r\n]*) to capture values, so MCP_HTTP_PORT=8001 # my custom port parses correctly as 8001 rather than failing [int]::TryParse. (PR #682, Gemini review feedback) |
2 participants
Release v10.36.2 (PATCH)
Promotes the changes from PR #682 (
fix/windows-scripts-env-aware) into a versioned release.What changed
PR #682 fixed two bugs in the Windows Scheduled Task wrapper scripts that made them unreliable as soon as a user enabled HTTPS or changed the default port:
Hardcoded URLs - All 5 PowerShell management scripts used
http://127.0.0.1:8000unconditionally. Introducedscripts/service/windows/lib/server-config.ps1to parse.envforMCP_HTTP_HOST/MCP_HTTP_PORT/MCP_HTTPS_ENABLED. Fully backward-compatible (falls back to previous defaults).Silent stdout dropout -
run_http_server_background.ps1used .NETOutputDataReceivedevent handlers that referenced$script:LogFile, which is not captured in the handler runspace. Every line of Python output was silently discarded. Replaced withStart-Process -RedirectStandardOutput.Gemini review feedback addressed in follow-up commit
6cc7752:.envregex now strips trailing inline comments (MCP_HTTP_PORT=8001 # commentparses as8001).-borto preserve TLS 1.3 alongside TLS 1.2.Classification: PATCH
Zero Python code changes, zero runtime/API changes, zero test changes. Windows-only PowerShell scripts.
Manual test evidence (verified by author)
manage_service.ps1 statusreportsHEALTHYonhttps://127.0.0.1:8001/after fix (previously:NOT RESPONDING)http-server-python.loggains 15 stdout lines + 174 stderr lines within seconds of startup (previously: always 0 lines)grepconfirms zero hardcoded8000/http://127.0.0.1references remain inscripts/service/windows/Landing page
Skipped per CLAUDE.md policy: landing page updates are MINOR/MAJOR only. This is a PATCH release.
Version bump
src/mcp_memory_service/_version.pypyproject.tomlpyproject-lite.tomluv.lock