fix(mcp): version daemon compatibility separately#183
Conversation
Greptile SummaryThis PR introduces a dedicated Key changes:
Minor observation: The test "refreshes an incompatible daemon version before running list" ( Confidence Score: 5/5Safe to merge — the change is minimal, well-contained, and the key new validation is well-covered in capabilities.test.ts. All remaining findings are P2 (test clarity). The core logic in capabilities.ts, protocol.ts, and server.ts is correct and the unit test in capabilities.test.ts directly covers the primary new behavior (returning null when daemonVersion is missing or mismatched). src/session/commands.test.ts — the 'refreshes an incompatible daemon version' test uses a wrong-API-version mock rather than a daemon-version-only-mismatch mock; functionally covered but the scenario doesn't precisely match the PR's intent.
|
| Filename | Overview |
|---|---|
| src/session/protocol.ts | Adds HUNK_SESSION_DAEMON_VERSION = 1 constant and daemonVersion: number field to SessionDaemonCapabilities; clean protocol extension with good inline documentation. |
| src/session/capabilities.ts | Validates both version and daemonVersion in readHunkSessionDaemonCapabilities; correctly returns null when either is missing or mismatched, triggering daemon refresh. |
| src/mcp/server.ts | Includes daemonVersion: HUNK_SESSION_DAEMON_VERSION in the sessionCapabilities() response; straightforward addition with no side effects. |
| src/session/capabilities.test.ts | New test file with solid coverage: null for non-OK responses, null when daemonVersion is omitted, and success only when both versions match exactly. |
| src/session/commands.test.ts | New test 'refreshes an incompatible daemon version' verifies the restart path via API version mismatch in the mock rather than daemonVersion-only mismatch; covers correct behavior but the test scenario doesn't precisely map to the new failure mode being introduced. |
| src/mcp/client.test.ts | Updated to include daemonVersion: HUNK_SESSION_DAEMON_VERSION in the mock capabilities server response; correct and consistent with protocol changes. |
| src/mcp/server.test.ts | Updated assertion to expect daemonVersion: 1 in the capabilities response; correct and consistent. |
Sequence Diagram
sequenceDiagram
participant CLI as hunk session list (CLI)
participant CAP as readHunkSessionDaemonCapabilities
participant daemon as Running Daemon
CLI->>CAP: getCapabilities()
CAP->>daemon: GET /session-api/capabilities
daemon-->>CAP: { version, daemonVersion, actions }
alt daemonVersion missing or mismatched
CAP-->>CLI: null
CLI->>CLI: reportHunkDaemonUpgradeRestart()
CLI->>daemon: SIGTERM (kill stale daemon)
CLI->>CLI: ensureHunkDaemonAvailable() → new daemon
CLI->>CLI: waitForSessionRegistration()
CLI->>daemon: POST /session-api (action: list)
daemon-->>CLI: { sessions }
else both versions match
CAP-->>CLI: SessionDaemonCapabilities
CLI->>daemon: POST /session-api (action: list)
daemon-->>CLI: { sessions }
end
Comments Outside Diff (1)
-
src/session/commands.test.ts, line 206-274 (link)Test scenario doesn't match the new failure mode it's named after
The test is titled "refreshes an incompatible daemon version before running list" but simulates the stale daemon by returning
version: HUNK_SESSION_API_VERSION - 1(wrong API version) while keepingdaemonVersion: HUNK_SESSION_DAEMON_VERSIONcorrect. In production, the newly introduced failure mode is the opposite: the daemon advertises the correct API version but omits or mismatchesdaemonVersion, causingreadHunkSessionDaemonCapabilitiesto returnnull.Because the fake
getCapabilitiesbypassesreadHunkSessionDaemonCapabilities, the test ends up exercising the same branch as the "refreshes an older daemon without the session API" test above it (both result incapabilitiesnot satisfying the condition inensureRequiredAction). The end-to-end behavior is correct, but the test name and mock values don't precisely cover the scenario the PR describes.A more precise mock for the new case:
getCapabilities: async () => { // Simulates a daemon that is current on the HTTP action surface (same API version) // but is older than this Hunk build on the live-session state shape — so // readHunkSessionDaemonCapabilities returns null, just as it would in production. createdClients.push("stale-capabilities"); return null; // daemonVersion mismatch → null returned by readHunkSessionDaemonCapabilities },
Or, if the intent is to also cover the non-null-capabilities path in
ensureRequiredActionfor an explicitdaemonVersionmismatch, the condition there could be extended to checkdaemonVersiondirectly rather than relying solely on the null-return contract fromreadHunkSessionDaemonCapabilities.
Reviews (1): Last reviewed commit: "test(session): cover comment action daem..." | Re-trigger Greptile
Summary
Why
A newer Hunk build could still talk to an older daemon on the shared default MCP port because the daemon advertised the same session API version and actions. That let
hunk session listreach a stale daemon that was too old for the newer live-session state shape instead of refreshing it first.This makes the latest Hunk proactively refresh that stale daemon so it just takes care of the upgrade path.
Testing
bun run typecheckbun testdaemonVersionandsession list --jsonworked againThis PR description was generated by Pi using OpenAI o3