fix(mcp): disable MCP_SKILLS feature flag — source not mirrored (#872)
Closes #856. MCP servers that expose resources (e.g. RepoPrompt) failed to load their tools in the open build with: Error fetching tools/commands/resources: fetchMcpSkillsForClient is not a function Root cause: scripts/build.ts set MCP_SKILLS: true, which made feature('MCP_SKILLS') evaluate to true at build time. The guards around the dynamic skill discovery path therefore stayed live. The underlying source file src/skills/mcpSkills.ts is not mirrored into the open tree, so the bundler fell back to its generic missing-module stub — which only exports `default` for require()-style imports, not the named `fetchMcpSkillsForClient` binding. At runtime the require returned an object without that property, and calling it threw. `openclaude mcp doctor` reported RepoPrompt as healthy because doctor does not exercise the skills-fetch path. Fix: flip MCP_SKILLS to false and move it into the "Disabled: missing source" group. With the flag off, every `if (feature('MCP_SKILLS'))` guard becomes a no-op at build time, the require() branch is dead code, and MCP servers with resources load normally via the existing `Promise.resolve([])` fallbacks already present at each call site. Also adds scripts/feature-flags-source-guard.test.ts to fail fast if MCP_SKILLS (or any future flag in the same category) is re-enabled without the corresponding source file being mirrored first. Verification: - Test fails on main, passes with this fix - `bun run build` produces a bundle with no `missing-module-stub:../../skills/mcpSkills.js` reference - Full `bun test` — 1222 pass / 12 fail (same pre-existing 12 as main; new test adds the +1 pass)
This commit is contained in:
@@ -34,6 +34,7 @@ const featureFlags: Record<string, boolean> = {
|
|||||||
WEB_BROWSER_TOOL: false, // Built-in browser automation (source not mirrored)
|
WEB_BROWSER_TOOL: false, // Built-in browser automation (source not mirrored)
|
||||||
CHICAGO_MCP: false, // Computer-use MCP (native Swift modules stubbed)
|
CHICAGO_MCP: false, // Computer-use MCP (native Swift modules stubbed)
|
||||||
COWORKER_TYPE_TELEMETRY: false, // Telemetry for agent/coworker type classification
|
COWORKER_TYPE_TELEMETRY: false, // Telemetry for agent/coworker type classification
|
||||||
|
MCP_SKILLS: false, // Dynamic MCP skill discovery (src/skills/mcpSkills.ts not mirrored; enabling this causes "fetchMcpSkillsForClient is not a function" when MCP servers with resources connect — see #856)
|
||||||
|
|
||||||
// ── Enabled: upstream defaults ──────────────────────────────────────
|
// ── Enabled: upstream defaults ──────────────────────────────────────
|
||||||
COORDINATOR_MODE: true, // Multi-agent coordinator with worker delegation
|
COORDINATOR_MODE: true, // Multi-agent coordinator with worker delegation
|
||||||
@@ -56,7 +57,6 @@ const featureFlags: Record<string, boolean> = {
|
|||||||
EXTRACT_MEMORIES: true, // Auto-extract durable memories from conversations
|
EXTRACT_MEMORIES: true, // Auto-extract durable memories from conversations
|
||||||
FORK_SUBAGENT: true, // Implicit context-forking when omitting subagent_type
|
FORK_SUBAGENT: true, // Implicit context-forking when omitting subagent_type
|
||||||
VERIFICATION_AGENT: true, // Built-in read-only agent for test/verification
|
VERIFICATION_AGENT: true, // Built-in read-only agent for test/verification
|
||||||
MCP_SKILLS: true, // Discover skills dynamically from MCP server resources
|
|
||||||
PROMPT_CACHE_BREAK_DETECTION: true, // Detect & log unexpected prompt cache invalidations
|
PROMPT_CACHE_BREAK_DETECTION: true, // Detect & log unexpected prompt cache invalidations
|
||||||
HOOK_PROMPTS: true, // Allow tools to request interactive user prompts
|
HOOK_PROMPTS: true, // Allow tools to request interactive user prompts
|
||||||
}
|
}
|
||||||
|
|||||||
47
scripts/feature-flags-source-guard.test.ts
Normal file
47
scripts/feature-flags-source-guard.test.ts
Normal file
@@ -0,0 +1,47 @@
|
|||||||
|
import { existsSync, readFileSync } from 'fs'
|
||||||
|
import { join } from 'path'
|
||||||
|
import { expect, test } from 'bun:test'
|
||||||
|
|
||||||
|
// Regression guard for #856. Several build feature flags require source files
|
||||||
|
// that are not mirrored into the open build. When such a flag is set to `true`
|
||||||
|
// without the source present, the bundler falls back to a missing-module stub
|
||||||
|
// that only exports `default`, which causes runtime errors like
|
||||||
|
// `fetchMcpSkillsForClient is not a function` when downstream code reaches
|
||||||
|
// through the `require()` to a named export.
|
||||||
|
//
|
||||||
|
// This test fails fast at test-time if someone re-enables one of these flags
|
||||||
|
// without first mirroring the corresponding source file.
|
||||||
|
|
||||||
|
const BUILD_SCRIPT = join(import.meta.dir, 'build.ts')
|
||||||
|
const REPO_ROOT = join(import.meta.dir, '..')
|
||||||
|
|
||||||
|
type FlagGuard = {
|
||||||
|
flag: string
|
||||||
|
source: string // path relative to repo root
|
||||||
|
}
|
||||||
|
|
||||||
|
const FLAG_REQUIRES_SOURCE: FlagGuard[] = [
|
||||||
|
{ flag: 'MCP_SKILLS', source: 'src/skills/mcpSkills.ts' },
|
||||||
|
]
|
||||||
|
|
||||||
|
test('build feature flags are not enabled without their source files', () => {
|
||||||
|
const buildScript = readFileSync(BUILD_SCRIPT, 'utf-8')
|
||||||
|
|
||||||
|
for (const { flag, source } of FLAG_REQUIRES_SOURCE) {
|
||||||
|
const enabledRe = new RegExp(`^\\s*${flag}\\s*:\\s*true\\b`, 'm')
|
||||||
|
const isEnabled = enabledRe.test(buildScript)
|
||||||
|
const sourceExists = existsSync(join(REPO_ROOT, source))
|
||||||
|
|
||||||
|
if (isEnabled && !sourceExists) {
|
||||||
|
throw new Error(
|
||||||
|
`Feature flag ${flag} is enabled in scripts/build.ts, but its required source file "${source}" does not exist. ` +
|
||||||
|
`Enabling this flag without the source will cause runtime errors (missing named exports from the missing-module stub). ` +
|
||||||
|
`Either mirror the source file or set ${flag}: false.`,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
// When the source IS present, the flag can be either true or false; either
|
||||||
|
// is fine. We only care about the "enabled but missing" combination.
|
||||||
|
expect(true).toBe(true)
|
||||||
|
}
|
||||||
|
})
|
||||||
Reference in New Issue
Block a user