Skip to content

fix(mcp): version daemon compatibility separately#183

Merged
benvinegar merged 3 commits intomainfrom
fix/mcp-daemon-compat-version
Apr 8, 2026
Merged

fix(mcp): version daemon compatibility separately#183
benvinegar merged 3 commits intomainfrom
fix/mcp-daemon-compat-version

Conversation

@benvinegar
Copy link
Copy Markdown
Member

Summary

  • add a dedicated daemon compatibility version to the session capabilities handshake
  • treat daemons that omit or mismatch that version as stale even when they still expose the same HTTP action list
  • cover the new handshake in MCP/session capability tests

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 list reach 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 typecheck
  • bun test
  • manual default-port smoke check with a stale daemon present: verified the refreshed daemon reported daemonVersion and session list --json worked again

This PR description was generated by Pi using OpenAI o3

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 8, 2026

Greptile Summary

This PR introduces a dedicated daemonVersion field to the session capabilities handshake, enabling newer Hunk builds to detect and refresh stale daemons that advertise the same HTTP action surface but carry an older internal session-state shape. The change is small and well-scoped: protocol.ts adds HUNK_SESSION_DAEMON_VERSION = 1 and the daemonVersion field to SessionDaemonCapabilities; capabilities.ts validates it in readHunkSessionDaemonCapabilities; and server.ts includes it in the capabilities response.

Key changes:

  • src/session/protocol.ts — adds HUNK_SESSION_DAEMON_VERSION = 1 constant and daemonVersion: number to the SessionDaemonCapabilities interface
  • src/session/capabilities.ts — gates readHunkSessionDaemonCapabilities on both version and daemonVersion matching; returns null for mismatches (including daemons that omit the field entirely)
  • src/mcp/server.ts — includes daemonVersion in the sessionCapabilities() response
  • Test files — updated to pass daemonVersion in fake capability responses and add targeted unit tests for the new validation logic

Minor observation: The test "refreshes an incompatible daemon version before running list" (commands.test.ts) verifies the restart path by returning { version: HUNK_SESSION_API_VERSION - 1, daemonVersion: HUNK_SESSION_DAEMON_VERSION, ... } from the fake getCapabilities. In production, a daemon that has the correct API version but wrong/missing daemonVersion would cause readHunkSessionDaemonCapabilities to return null, not a non-null object with a wrong version. The actual new scenario is therefore covered only indirectly through capabilities.test.ts. The end-to-end behavior is still correct (both paths ultimately hit the same restart branch in ensureRequiredAction), but the test name and simulated input don't align precisely with the daemon-version-only mismatch case the PR describes.

Confidence Score: 5/5

Safe 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.

Vulnerabilities

No security concerns identified. The changes are confined to the loopback MCP handshake: adding a daemonVersion field to capabilities responses and validating it client-side. All traffic remains local-only, no new network surface is exposed, and no sensitive data is handled.

Important Files Changed

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
Loading

Comments Outside Diff (1)

  1. src/session/commands.test.ts, line 206-274 (link)

    P2 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 keeping daemonVersion: HUNK_SESSION_DAEMON_VERSION correct. In production, the newly introduced failure mode is the opposite: the daemon advertises the correct API version but omits or mismatches daemonVersion, causing readHunkSessionDaemonCapabilities to return null.

    Because the fake getCapabilities bypasses readHunkSessionDaemonCapabilities, the test ends up exercising the same branch as the "refreshes an older daemon without the session API" test above it (both result in capabilities not satisfying the condition in ensureRequiredAction). 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 ensureRequiredAction for an explicit daemonVersion mismatch, the condition there could be extended to check daemonVersion directly rather than relying solely on the null-return contract from readHunkSessionDaemonCapabilities.

Reviews (1): Last reviewed commit: "test(session): cover comment action daem..." | Re-trigger Greptile

@benvinegar benvinegar merged commit 502d381 into main Apr 8, 2026
3 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