Fix reload_plugins MCP hot-swap + robustness/metrics pass - #48
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Claude Agent SDK MCP server hot-swap during reload_plugins by tracking the in-flight SDK Query per chat and updating its MCP server config immediately after plugin reload, so newly reloaded plugin tools are usable in the same turn.
Changes:
- Track and expose the active Claude SDK
Queryper chat to enable mid-turn control operations (e.g.,setMcpServers()). - Export/reuse MCP server construction (
buildMcpServers) and wire a new optional backend hookrefreshMcpServers(chatId). - Invoke
refreshMcpServersafterreload_pluginsand surface added/removed/errors info in the action response.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/backend/claude-sdk/handler.ts |
Stores active in-flight Query in a map and exposes getActiveQuery() for hot-swap operations. |
src/backend/claude-sdk/options.ts |
Exports buildMcpServers() for reuse outside the options builder. |
src/backend/claude-sdk/index.ts |
Re-exports getActiveQuery and buildMcpServers from the backend barrel. |
src/core/types.ts |
Adds optional refreshMcpServers(chatId) to the QueryBackend interface. |
src/bootstrap.ts |
Implements refreshMcpServers for the Claude SDK backend by calling Query.setMcpServers(). |
src/core/gateway-actions.ts |
Calls backend.refreshMcpServers() after plugin reload and appends a status summary to the response. |
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/frontend/telegram/actions.ts:76
- In
sendText, thecatchassumes anysendMessage(..., { parse_mode: "HTML" })failure is an HTML parse error and then retries withoutparse_mode. This can be misleading in logs (network/403/etc will also be reported as βHTML parse failedβ) and may cause an unintended second send attempt for non-parse failures. Consider only falling back when the error is a Telegram βcan't parse entitiesβ/400 parse error, and otherwise rethrow (or at least log the original error as a generic send failure).
try {
const sent = await bot.api.sendMessage(chatId, html, {
parse_mode: "HTML",
reply_parameters: replyTo ? { message_id: replyTo } : undefined,
});
return sent.message_id;
} catch (err) {
logWarn(
"bot",
`sendText HTML parse failed (chat=${chatId}): ${err instanceof Error ? err.message : err}`,
);
const sent = await bot.api.sendMessage(chatId, text, {
reply_parameters: replyTo ? { message_id: replyTo } : undefined,
});
return sent.message_id;
}
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
β¦fe exit - handlers.ts: replace setTimeout-based verifiedGroups cache with timestamp expiry (no timers to leak on clear), evict expired entries before falling back to full clear - metrics.ts: drop NaN/Infinity/non-finite values from histograms - mcp-server.ts: use process.exitCode + stderr write callback to ensure unhandledRejection message flushes before exit - tests: add NaN/Infinity filtering test (1335 total)
β¦handler - handlers.ts: use timestamp-based object in isAdminInGroup catch block (was still using old boolean + setTimeout pattern) - mcp-server.ts: log err.stack instead of err.message for full traces
β¦rowth - actions.ts: more accurate log message for sendText fallback (not just HTML parse β could be any sendMessage failure) - metrics.ts: cap counters and histograms at 500 keys each β new keys silently dropped above cap, existing keys always work - tests: add key cap tests for counters and histograms (1337 total)
- actions.ts, media-index.ts: pass err object to logError() instead of string interpolation β preserves stack traces and structured details - metrics.ts: replace splice-based histogram with O(1) ring buffer - cron.ts: warn once per bad schedule instead of every 60s tick
β¦ection - cron.ts: cap warnedBadSchedule at 200 entries, clear stale warnings when schedule parses successfully again - mcp-server.ts: add 1s force-exit timer (unref'd) so subprocess always exits even if stderr is backpressured
- Exposes counters and histograms via /metrics (admin-only) - Groups counters by prefix (tool_calls, errors, general) - Shows latency percentiles (p50/p95/p99/avg) for histograms - Registered in bot menu and help text
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2d96dea to
6346355
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
β¦n reload Previously, reload_plugins would not restart unchanged MCP subprocesses because the Claude SDK reuses connections when command/args are identical. Source file changes to plugin scripts were silently ignored. Fix: stamp a new ISO timestamp into TALON_RELOAD_AT on every reloadPlugins() call. Since every reload now produces a distinct env, the SDK always spawns a fresh subprocess β picking up any source-file edits without a full Talon restart.
The Claude SDK spawns fresh subprocesses when env changes but does not SIGTERM
old ones, causing a resource leak on every reload_plugins call.
Fix: after a 2s grace period (for the SDK to settle), scan /proc for any
plugin subprocesses whose TALON_RELOAD_AT env var doesn't match the current
reload timestamp and SIGTERM them. Runs async to not block the reload response.
Linux-only (/proc-based), safe on non-Linux (readdirSync('/proc') will fail
and be caught).
β¦derMetricsMessages
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
π‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Remove the Linux-only killOrphanedPluginSubprocesses that scanned /proc
to find and kill stale MCP subprocesses. Instead:
- Two-phase teardown in refreshMcpServers: call setMcpServers({}) first
to explicitly tell the SDK to close all existing MCP server connections
(sending shutdown via stdio), then install the fresh server set
- MCP server subprocess listens for stdin 'end' to self-terminate
gracefully when the parent closes the pipe
- Log MCP refresh failures in gateway-actions (was only surfaced in
response text, not in server logs)
This is fully OS-agnostic β works on Linux, macOS, and Windows with no
platform-specific code paths.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Spawns real MCP server processes using the same SDK transport as production and verifies the stdin-close teardown mechanism: - Server exits gracefully (code 0) when stdin is closed - Old server exits while new server keeps running (reload simulation) - Multiple servers all exit when their stdin is batch-closed Uses a minimal test fixture with McpServer + StdioServerTransport to keep startup fast while testing the real mechanism. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
β¦w, lockfile portability, tarball validation Per Dylan's "overhaul the CI and make it much more comprehensive and robust." Five new jobs gated through the existing `CI Status` aggregate, plus a real package-distribution fix the new tarball checker uncovered. ## What's new ### `integration` β MCP-functional tier in CI (Ubuntu/macOS/Windows Γ Node 22) The new MCP-functional tier from #131 (`talon-mcp-functional.test.ts`) now runs on every PR + push across the full OS matrix. Catches SDKβMCPβbridgeβhandler integration bugs (the PR #122 prefix-mismatch class) without needing a live external service. Builds the stub-claude SEA binary on Windows (Node 20.19+ blocks spawning .cmd shims via child_process.spawn β CVE-2024-27980 mitigation). ### `secrets` β gitleaks scan on every push + PR `gitleaks/gitleaks-action@v2` with `fetch-depth: 0` so it sees full git history, not just the latest commit. Catches API keys, bot tokens, .env files committed by accident, OAuth secrets, private keys. ### `dep-review` β github/dependency-review-action on PRs Diffs lockfile changes against the GitHub Advisory Database. `fail-on-severity: moderate` β moderate or higher CVE in any newly introduced dep blocks the PR. Comments summary on PR on failure. PR-only (no main-branch run); `ci-ok` treats `skipped` as ok. ### `lockfile` β portability check in fresh tmpdir Copies ONLY `package.json` + `package-lock.json` to a fresh tmpdir and runs `npm ci --ignore-scripts --no-audit` there. Same check as `tools/verify-lockfile.sh` locally. Catches the "lockfile drift" / "lockfile only validates with `--include=optional`" classes the heartbeat journal documented (hb #48 / #103). The other test jobs run `npm ci` in the source tree where `node_modules` state could mask portability bugs. This one starts from zero, every run. ### `tarball` β validates `npm pack` contents New `.github/scripts/tarball-check.mjs` β runs `npm pack`, inspects the resulting tarball for forbidden patterns: - `.env*`, `credentials.json`, `*.key`, `*.pem`, `id_rsa`, etc. - `__tests__/`, `*.test.ts`, `*.spec.ts` - `node_modules`, `.git`, `coverage`, sourcemaps, `.vscode`, `.idea`, `.DS_Store` - Tarball size > 5MB (catches accidental large-file commits) - `package.json` / `README` missing Also prints a sorted top-10 largest files for the CI step summary. ## Real bug uncovered The tarball checker's first run flagged **270+ test files were being shipped in the npm tarball**. `package.json#files` listed `"src/"` which swept in the entire test suite. Fixed by replacing the broad `"src/"` entry with explicit subdirs: - "src/backend/", "src/core/", "src/frontend/", "src/plugins/", "src/storage/", "src/util/" - "src/bootstrap.ts", "src/cli.ts", "src/index.ts", "src/login.ts" Tarball size dropped from including 270+ test files to a clean 99 source-only files (180KB β still 180KB but with no test scaffolding). Future versions of `talon-agent` on npm won't ship test fixtures. ## Coverage config `vitest.config.ts`: added an exclude list (test files, integration scaffolding, entry points) and `lcov` reporter for tooling compatibility (codecov, IDE plugins). Thresholds left at 60% for now β ratcheting up is out of scope for this PR (would risk breaking CI on unrelated paths). ## ci-ok gate Updated to require all 10 jobs (was 6). Branch protection in `.github/branch-protection.json` already gates merges on `CI Status`, so adding a new required job means adding it to the `needs:` list + the `check` calls in the gate script β no separate config change. ## Test plan - [x] `tsc --noEmit` clean - [x] `prettier --check` clean on touched files - [x] `npm run test:integration` passes locally (4 tests) - [x] `npm run tarball:check` passes locally β clean tarball - [x] Lockfile portability passes (replicated tmpdir check, 7s npm ci) - [x] YAML syntax valid - [x] Existing test suite unaffected (1711 tests pass + 1 pre-existing main flake)
β¦iew + lockfile portability + tarball validation (#137) * ci: comprehensive overhaul β integration tier, secret scan, dep review, lockfile portability, tarball validation Per Dylan's "overhaul the CI and make it much more comprehensive and robust." Five new jobs gated through the existing `CI Status` aggregate, plus a real package-distribution fix the new tarball checker uncovered. ## What's new ### `integration` β MCP-functional tier in CI (Ubuntu/macOS/Windows Γ Node 22) The new MCP-functional tier from #131 (`talon-mcp-functional.test.ts`) now runs on every PR + push across the full OS matrix. Catches SDKβMCPβbridgeβhandler integration bugs (the PR #122 prefix-mismatch class) without needing a live external service. Builds the stub-claude SEA binary on Windows (Node 20.19+ blocks spawning .cmd shims via child_process.spawn β CVE-2024-27980 mitigation). ### `secrets` β gitleaks scan on every push + PR `gitleaks/gitleaks-action@v2` with `fetch-depth: 0` so it sees full git history, not just the latest commit. Catches API keys, bot tokens, .env files committed by accident, OAuth secrets, private keys. ### `dep-review` β github/dependency-review-action on PRs Diffs lockfile changes against the GitHub Advisory Database. `fail-on-severity: moderate` β moderate or higher CVE in any newly introduced dep blocks the PR. Comments summary on PR on failure. PR-only (no main-branch run); `ci-ok` treats `skipped` as ok. ### `lockfile` β portability check in fresh tmpdir Copies ONLY `package.json` + `package-lock.json` to a fresh tmpdir and runs `npm ci --ignore-scripts --no-audit` there. Same check as `tools/verify-lockfile.sh` locally. Catches the "lockfile drift" / "lockfile only validates with `--include=optional`" classes the heartbeat journal documented (hb #48 / #103). The other test jobs run `npm ci` in the source tree where `node_modules` state could mask portability bugs. This one starts from zero, every run. ### `tarball` β validates `npm pack` contents New `.github/scripts/tarball-check.mjs` β runs `npm pack`, inspects the resulting tarball for forbidden patterns: - `.env*`, `credentials.json`, `*.key`, `*.pem`, `id_rsa`, etc. - `__tests__/`, `*.test.ts`, `*.spec.ts` - `node_modules`, `.git`, `coverage`, sourcemaps, `.vscode`, `.idea`, `.DS_Store` - Tarball size > 5MB (catches accidental large-file commits) - `package.json` / `README` missing Also prints a sorted top-10 largest files for the CI step summary. ## Real bug uncovered The tarball checker's first run flagged **270+ test files were being shipped in the npm tarball**. `package.json#files` listed `"src/"` which swept in the entire test suite. Fixed by replacing the broad `"src/"` entry with explicit subdirs: - "src/backend/", "src/core/", "src/frontend/", "src/plugins/", "src/storage/", "src/util/" - "src/bootstrap.ts", "src/cli.ts", "src/index.ts", "src/login.ts" Tarball size dropped from including 270+ test files to a clean 99 source-only files (180KB β still 180KB but with no test scaffolding). Future versions of `talon-agent` on npm won't ship test fixtures. ## Coverage config `vitest.config.ts`: added an exclude list (test files, integration scaffolding, entry points) and `lcov` reporter for tooling compatibility (codecov, IDE plugins). Thresholds left at 60% for now β ratcheting up is out of scope for this PR (would risk breaking CI on unrelated paths). ## ci-ok gate Updated to require all 10 jobs (was 6). Branch protection in `.github/branch-protection.json` already gates merges on `CI Status`, so adding a new required job means adding it to the `needs:` list + the `check` calls in the gate script β no separate config change. ## Test plan - [x] `tsc --noEmit` clean - [x] `prettier --check` clean on touched files - [x] `npm run test:integration` passes locally (4 tests) - [x] `npm run tarball:check` passes locally β clean tarball - [x] Lockfile portability passes (replicated tmpdir check, 7s npm ci) - [x] YAML syntax valid - [x] Existing test suite unaffected (1711 tests pass + 1 pre-existing main flake) * ci: drop Windows from integration job β MCP-dispatch chain doesn't run there yet The MCP-functional tier passes on Ubuntu + macOS but fails on Windows: the stub's SEA-bundled MCP client + StdioClientTransport spawn-and-fetch chain to the recording handler doesn't complete under SEA-on-Windows. Three of four cases see zero captures at the recording handler, even though `PostToolBatch: terminating SDK loop on mcp__telegram-tools__end_turn` fires (so the SDK loop sees the tool, but MCP dispatch never lands). Likely culprits to investigate (out of scope for this PR): - esbuild SEA bundle handling of the dynamic `await import("@modelcontextprotocol/sdk/...")` - StdioClientTransport.spawn behavior under SEA-on-Windows - Localhost loopback / fetch from spawned subprocess on Windows The other Windows test paths still run β `Tests (windows-latest, Node 22)` and `Functional (windows-latest)` cover the platform without MCP dispatch (sdk-stub, talon-functional). Reinstating Windows for `integration` is gated on root-causing the failure. * Revert "ci: drop Windows from integration job β MCP-dispatch chain doesn't run there yet" This reverts commit 6410446. * fix(integration): static MCP imports + protocol-log-on-failure for Windows debugging Two changes targeting the Windows integration failure: 1. fake-claude.mjs: convert dynamic `await import("@modelcontextprotocol/sdk/...")` to static top-level imports. Likely root cause of the Windows failure β esbuild's CJS bundling preserves `await import(literal)` as `Promise.resolve(require(literal))`, which works locally where node_modules exists, but the SEA binary on Windows ships only the bundled .cjs without node_modules. Static imports force esbuild to inline the SDK module bodies into the bundle. Verified: rebuilt SEA bundle locally, `grep "var StdioClientTransport" fake-claude.cjs` confirms the SDK is now inlined. 2. talon-mcp-functional.test.ts: add `withProtocolLogOnFailure()` helper. The MCP-functional tier has subprocess hops (SDK loop β MCP server spawn β bridge HTTP) that produce no Vitest output when something goes wrong upstream of the actual `expect(...)`. This wrapper dumps the last 60 lines of the stub's protocol log on assertion failure so future Windows regressions are diagnosable from the CI log directly. Wraps the canary first test only (the others' assertions are all downstream of the same MCP path). Local: all 11 MCP-functional tests still pass on Linux. Windows verification has to come from the CI run on this branch. * fix(mcp-server): unify Windows + POSIX command path β node --import tsx, not npx tsx Real root cause of the Windows integration failure. `buildMcpServers` was spawning `npx tsx <path>` on Windows and `node --import <tsx-loader> <path>` on POSIX. The Windows path goes through the MCP launcher (which proxies stdio), so the launcher then runs `child_process.spawn("npx", [...])` to start the actual MCP server. Node 20.19+ refuses to execute `.cmd` shims via `child_process.spawn` without `shell: true` (CVE-2024-27980 mitigation). `npx` on Windows IS a .cmd shim. Result: the launcher's spawn fails immediately, the MCP server never starts, the stub's MCP client tries to connect over a dead pipe and gets `MCP error -32000: Connection closed`. The earlier hypothesis (SEA bundle missing dynamic imports) was wrong β the static-imports change in 9507333 is still the right fix (removes a dynamic-import gotcha entirely), but the actual blocker was this command shape. Fix: use `node --import <tsx-loader> <path>` on every platform. tsx as a Node loader is platform-identical, and we never spawn `npx.cmd`. The launcher's spawn now hits a plain `node` binary, which works under Node's CVE mitigation. Verified locally: 11 / 11 MCP-functional tests still pass on Linux. Windows verification: this branch's CI run. Diagnostic from the previous failed Windows CI run that pinned this: MCP_CONFIG servers: ["telegram-tools"] MCP connect error for telegram-tools: MCP error -32000: Connection closed The launcher's spawned child died before completing initialize. * fix(mcp-server): convert tsx loader path to file:// URL for Windows Second Windows iteration. Same `MCP error -32000: Connection closed` after the previous fix, even with `node --import <path>` everywhere. The remaining issue: `--import` accepts URLs or paths, but on Windows a raw backslash path like `D:\a\talon\talon\node_modules\tsx\dist\esm\index.mjs` is ambiguous between path and URL. Node's loader-hook resolution silently fails to register tsx, every subsequent `.ts` import in `mcp-server.ts` throws, the MCP server crashes, the stub's MCP client gets "Connection closed" on its initialize handshake. Fix: convert via `pathToFileURL(resolve(...)).href` so the loader URL is always a clean `file:///D:/.../index.mjs` regardless of platform. Linux already worked because `/path/to/foo.mjs` is unambiguous as both a path and a URL. Local: 11 / 11 still pass on Linux. Windows: this commit's CI run is the truth-or-not test.
Summary
reload_pluginstool call caused hanging responses because the plugin registry updated but the active Claude Agent SDK query retained stale MCP server config.Queryreference per chat and callsetMcpServers()on it after plugin reload to hot-swap MCP servers immediately.Changes
MCP hot-swap fix
src/backend/claude-sdk/handler.tsQueryin aMap, exposegetActiveQuery(), clean up infinallyblock, wire metricssrc/backend/claude-sdk/options.tsbuildMcpServers()for reuse outsidebuildSdkOptions()src/backend/claude-sdk/index.tsgetActiveQueryandbuildMcpServerssrc/core/types.tsrefreshMcpServers?(chatId)toQueryBackendinterfacesrc/bootstrap.tsrefreshMcpServersβ directsetMcpServers()call (no timeout, SDK handles it)src/core/gateway-actions.tsbackend.refreshMcpServers()after reload, preferbody._chatIdfor non-numeric IDsSilent error swallowing fixes
src/frontend/telegram/actions.tssrc/frontend/telegram/handlers.tssrc/core/cron.tssrc/storage/media-index.tsEmpty catch block fixes
src/core/heartbeat.tssrc/core/dream.tssrc/util/trace.tssrc/util/cleanup-registry.tsRobustness
src/frontend/telegram/handlers.tsverifiedGroups(1000) andunauthorizedCooldown(5000) maps to prevent unbounded growthsrc/core/tools/mcp-server.tsunhandledRejectionhandler to prevent silent MCP subprocess crashesMetrics
src/util/metrics.tssrc/backend/claude-sdk/handler.tsresponse_latency_ms,queries_total,tool_calls.*,errors.*metricsTests
src/__tests__/metrics.test.tssrc/__tests__/sessions.test.tsexpect(true).toBe(true)placeholder testsrc/__tests__/reload-plugins.test.ts_chatIdoverride,String(chatId)fallback, MCP refresh lifecycleTest plan
tsc --noEmit)