Compare commits

..

1 Commits

Author SHA1 Message Date
gnanam1990
bd73bcc9d7 fix(mcp): disable MCP_SKILLS feature flag — source not mirrored
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)
2026-04-24 07:54:11 +05:30
3 changed files with 56 additions and 21 deletions

View File

@@ -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
} }

View 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)
}
})

View File

@@ -1,6 +1,6 @@
import { execFileSync, spawn } from 'child_process' import { execFileSync, spawn } from 'child_process'
import { constants as fsConstants, readFileSync, unlinkSync } from 'fs' import { constants as fsConstants, readFileSync, unlinkSync } from 'fs'
import { type FileHandle, mkdir, open, stat } from 'fs/promises' import { type FileHandle, mkdir, open, realpath } from 'fs/promises'
import memoize from 'lodash-es/memoize.js' import memoize from 'lodash-es/memoize.js'
import { isAbsolute, resolve } from 'path' import { isAbsolute, resolve } from 'path'
import { join as posixJoin } from 'path/posix' import { join as posixJoin } from 'path/posix'
@@ -217,34 +217,22 @@ export async function exec(
let cwd = pwd() let cwd = pwd()
// Recover if the current working directory no longer exists on disk, // Recover if the current working directory no longer exists on disk.
// or was replaced by a non-directory (e.g., the path was renamed and a file // This can happen when a command deletes its own CWD (e.g., temp dir cleanup).
// was created in its place). realpath() succeeds on any existing path
// regardless of type, so we must also verify it's a directory — otherwise
// spawn would fail later with ENOTDIR / exit 126.
let cwdIsValidDir = false
try { try {
cwdIsValidDir = (await stat(cwd)).isDirectory() await realpath(cwd)
} catch { } catch {
cwdIsValidDir = false
}
if (!cwdIsValidDir) {
const fallback = getOriginalCwd() const fallback = getOriginalCwd()
logForDebugging( logForDebugging(
`Shell CWD "${cwd}" is not a valid directory, recovering to "${fallback}"`, `Shell CWD "${cwd}" no longer exists, recovering to "${fallback}"`,
) )
let fallbackIsValidDir = false
try { try {
fallbackIsValidDir = (await stat(fallback)).isDirectory() await realpath(fallback)
} catch {
fallbackIsValidDir = false
}
if (fallbackIsValidDir) {
setCwdState(fallback) setCwdState(fallback)
cwd = fallback cwd = fallback
} else { } catch {
return createFailedCommand( return createFailedCommand(
`Working directory "${cwd}" is no longer a valid directory. Please restart Claude from an existing directory.`, `Working directory "${cwd}" no longer exists. Please restart Claude from an existing directory.`,
) )
} }
} }