Skip to content

fix(setup): repair Claude MCP tool discovery#192

Merged
Alan-TheGentleman merged 2 commits intoGentleman-Programming:mainfrom
jgarrone82:issue-190-mcp-readiness
Apr 27, 2026
Merged

fix(setup): repair Claude MCP tool discovery#192
Alan-TheGentleman merged 2 commits intoGentleman-Programming:mainfrom
jgarrone82:issue-190-mcp-readiness

Conversation

@jgarrone82
Copy link
Copy Markdown
Contributor

@jgarrone82 jgarrone82 commented Apr 16, 2026

Fixes #190

Summary

This reworks the original readiness-delay approach into a deterministic Claude Code setup repair.

Changes

  • Removes the fixed MCP startup delay approach. No --init-delay, no time.Sleep, and no stderr readiness contract.
  • Derives Claude Code MCP permission names from the actual agent tool profile via mcp.ResolveTools("agent").
  • Adds both current durable user-level server id permissions (mcp__engram__...) and legacy plugin-scoped permissions (mcp__plugin_engram_engram__...).
  • Updates the Claude memory skill fallback so it no longer hardcodes stale plugin-scoped ToolSearch names.
  • Updates setup docs to describe re-running engram setup claude-code as the repair path.

Validation

  • go test ./cmd/engram ./internal/setup ./internal/mcp
  • git diff --check

Copy link
Copy Markdown
Collaborator

@Alan-TheGentleman Alan-TheGentleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes because this PR is not merge-ready yet.

Blockers:

  1. It does not compile
  • The new readiness subtests in cmd/engram/main_extra_test.go declare stdout and do not use it.
  • I applied the PR patch in a temp copy and ran go test ./cmd/engram ./internal/mcp -count=1.
  • Result: cmd/engram fails to build, while internal/mcp passes.
  1. --init-delay=<duration> skips the next flag
  • In cmd/engram/main.go, the --init-delay= branch increments i.
  • That is only valid for the separated form (--init-delay 1s), not the equals form.
  • Example: engram mcp --init-delay=1s --project=testproj will skip --project. Same risk with --tools.
  1. This is not yet a real readiness handshake
  • internal/mcp/mcp.go shows tools are registered before serveMCP() is called.
  • The current change adds a sleep plus a stderr log, but does not wire any supported client/setup path in this repo to wait for that signal.
  • internal/setup/setup.go still generates engram mcp --tools=agent with no readiness consumer.

Suggested fixes:

  • Remove the unused variables so the PR compiles.
  • Remove the extra i++ from the --init-delay=<duration> branch.
  • Add a regression test combining --init-delay with --project / --tools.
  • Either wire an actual consumer for the readiness signal or add a reproducible test proving the server-side-only change resolves issue #190.

@jgarrone82
Copy link
Copy Markdown
Contributor Author

Hi @Reviewer, thanks for the detailed review. All four blockers have been addressed in the latest push (312c007). Here's how each point was resolved:

  1. Compilation error — unused stdout variables
    Fixed. Removed the unused stdout bindings from all three readiness subtests, replacing them with the discard pattern:
    _, stderr, recovered := captureOutputAndRecover(t, func() { cmdMCP(cfg) })
    Files: cmd/engram/main_extra_test.go (3 occurrences).

  1. --init-delay= skips the next flag
    Fixed. Removed the erroneous i++ from the --init-delay= (equals-form) branch. The increment is only valid for the separated form (--init-delay 1s), where the next element is the value. In the equals form, the value is self-contained, so incrementing i incorrectly consumed the following flag.
    Before:
    } else if strings.HasPrefix(os.Args[i], "--init-delay=") {
    if d, err := time.ParseDuration(...); err == nil {
    initDelay = d
    }
    i++ // ← removed
    }
    File: cmd/engram/main.go.

  1. Regression test combining --init-delay with --project / --tools
    Added. New subtest "--init-delay with equals does not skip subsequent flags" exercises the exact scenario you called out:
    engram mcp --init-delay=1s --project=testproj --tools=agent
    The test verifies that:
  • newMCPServerWithConfig receives a non-nil allowlist (proving --tools=agent was parsed)
  • The ready signal contains delay=1s (proving --init-delay=1s was parsed)
  • The ready signal contains project="testproj" (proving --project=testproj was NOT skipped)
    File: cmd/engram/main_extra_test.go.

  1. Reproducible test proving the server-side change resolves issue [Bug] MCP tools not found on session start causing ~5 minute timeout per occurrence #190
    Added. New subtest "initialization delay precedes serving" uses time.Now() to measure that serveMCP is invoked after the delay has elapsed:
    start := time.Now()
    serveMCP = func(...) error {
    serveStart = time.Now() // record when serving actually begins
    return nil
    }
    // ... invoke cmdMCP with --init-delay=100ms ...
    if serveStart.Before(start.Add(90 * time.Millisecond)) {
    t.Fatalf("serveMCP called too early")
    }
    This demonstrates the exact ordering required to resolve the race condition:
  2. Server created, tools registered
  3. time.Sleep(initDelay) executes
  4. Ready signal emitted to stderr
  5. Only then serveMCP starts accepting connections
    Since the actual consumers (Claude Code plugin, OpenCode plugin, etc.) live outside this repo, this server-side test provides the reproducible proof that the delay correctly prevents clients from hitting tools before initialization is complete.
    File: cmd/engram/main_extra_test.go.

@Alan-TheGentleman Alan-TheGentleman force-pushed the issue-190-mcp-readiness branch from 312c007 to 790f86b Compare April 27, 2026 09:00
@Alan-TheGentleman Alan-TheGentleman changed the title fix: add MCP server readiness protocol to prevent tool invocation race condition fix(setup): repair Claude MCP tool discovery Apr 27, 2026
@Alan-TheGentleman
Copy link
Copy Markdown
Collaborator

I pushed a rework of this PR to avoid the fixed startup delay. The branch now repairs Claude Code MCP tool discovery directly: setup derives permissions from the actual agent tool profile, writes both current mcp__engram__... and legacy plugin-scoped permission names, and removes the stale hardcoded ToolSearch fallback from the Claude memory skill. I also rebased it onto main, updated the PR title/body, and verified go test ./cmd/engram ./internal/setup ./internal/mcp plus git diff --check. No checks are currently reported by GitHub for this branch, but it is now mergeable.

@Alan-TheGentleman Alan-TheGentleman added the type:bug Bug fix label Apr 27, 2026
Copy link
Copy Markdown
Collaborator

@Alan-TheGentleman Alan-TheGentleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scoped, tested, linked to the approved issue. CI and PR validation are green. Approving.

@Alan-TheGentleman Alan-TheGentleman merged commit a477c9d into Gentleman-Programming:main Apr 27, 2026
6 of 8 checks passed
@jgarrone82 jgarrone82 deleted the issue-190-mcp-readiness branch April 29, 2026 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] MCP tools not found on session start causing ~5 minute timeout per occurrence

2 participants