Skip to content

A1-polish: prerelease version parse + hung-server clean FAIL + drop unused import - #14

Merged
jpwinans merged 2 commits into
mainfrom
fix/a1-smoke-polish
Jun 14, 2026
Merged

A1-polish: prerelease version parse + hung-server clean FAIL + drop unused import#14
jpwinans merged 2 commits into
mainfrom
fix/a1-smoke-polish

Conversation

@jpwinans

Copy link
Copy Markdown
Owner

A1-polish — coder-2's #11 cross-review notes (1–3)

Optional wind-down follow-up to the merged A1 smoke-check. Addresses the three non-blocking notes from coder-2's PR #11 review; note 4 is kept as intentional.

  1. Unused import removedfrom .version import __version__ was unused (version comes from the server's initialize response, not the local constant).
  2. Hung-server → clean FAIL_mcp_session now catches subprocess.TimeoutExpired and returns {}, so a hung/stuck server produces a SMOKE-CHECK: FAIL report (all checks "no response") instead of crashing the gate with a traceback. A gate that exists to detect a broken server must not itself crash on one.
  3. Pre-release version parse_parse_version now takes leading digits per component, so "3.4.0rc1"[3,4,0] (satisfies the 3.4.0 floor as the base version) rather than the old bug where "0rc1" was read as patch 1[3,4,1].

Kept as intentional (note 4): no notifications/initialized is sent before tools/list — mempalace's handle_request processes each method independently and does not gate tools/list on the initialized notification, so it's correct for this server.

Verification

  • TDD red→green: test commit (3 new assertions failing pre-fix) → fix commit (green).
  • 22/22 test_smoke_upgrade pass; ruff clean.
  • Live gate still PASS on the now-live qdrant backend (python -m mempalace.smoke_upgrade → version/advertise/search/round-trip all PASS, exit 0).

jpwinans added 2 commits June 13, 2026 16:27
…erver FAIL (TDD red)

Adds: _parse_version must read leading-digits-per-component so '3.4.0rc1'->[3,4,0]
not [3,4,1]; _mcp_session must catch subprocess.TimeoutExpired and return {} so a
hung server yields a clean FAIL report instead of crashing the gate; run_smoke
reports ok=False (not a crash) when the server hangs. Fail pre-fix.
…n FAIL, drop unused import (TDD green)

Addresses coder-2's #11 cross-review notes (1-3):
- remove the unused 'from .version import __version__' import.
- _parse_version takes leading digits per component (pre-release suffix no longer
  misread as a higher patch).
- _mcp_session catches subprocess.TimeoutExpired -> returns {} so a hung server
  produces a FAIL report, not a crash. (Note 4, initialize-handshake-skip, kept
  as intentional: mempalace doesn't gate tools/list on the initialized notification.)
22/22 tests pass; live gate still PASS on the qdrant backend; ruff clean.
@jpwinans

Copy link
Copy Markdown
Owner Author

Architect review under /collaborate-to-build protocol

Same-account constraint (GitHub blocks gh pr review --approve on own PRs). This mirrors the hearing-channel review for the GitHub audit trail. coder-2 (who raised these notes on #11) is unavailable, so the architect (Ves) performed the genuine second-set-of-eyes review rather than a self-mirror.

Verdict: APPROVE

Reviewed all 3 fixes against the code + ran the suite (22/22 pass; TDD honest: 0ec60ae test-RED → 9a4b276 impl-GREEN):

  1. Drop unused __version__ import (note 1) — removed from .version import __version__ # noqa: F401; clean, it was genuinely unused.
  2. _parse_version leading-digits (note 3) — re.match(r"\d+", component) takes only leading digits, so "3.4.0rc1"[3,4,0] not [3,4,1]. Test guards both the parse AND the semantic (version_meets("3.4.0rc1","3.4.0") is True — an rc of the floor satisfies it). Correct.
  3. _mcp_session TimeoutExpired → clean FAIL (note 2) — wraps subprocess.run in try/except subprocess.TimeoutExpired, returns {} so a hung server yields a clean SMOKE-CHECK: FAIL instead of crashing the gate. This is the right robustness fix: the gate's purpose is to detect a broken/hung server, so it must not itself crash on that exact condition.

Note 4 (initialize-handshake-skip) correctly kept as intentional + documented — mempalace doesn't gate tools/list on the initialized notification, so skipping it is valid for this server.

Scope clean (only smoke_upgrade.py + its test). Live gate still PASS on the now-live qdrant backend. Ready for merge.

@jpwinans
jpwinans merged commit 04b423f into main Jun 14, 2026
3 of 7 checks passed
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