fix(ci): bring Validate Code Quality job back to green by shtse8 · Pull Request #188 · SylphxAI/filesystem-mcp

shtse8 · 2026-06-18T15:49:49Z

Why

The Validate Code Quality job (in .github/workflows/publish.yml) runs, in order:
bun install --frozen-lockfile → bun audit --prod → bun run check-format →
bun run lint → bun run test:cov. On top of #187 (pnpm→bun migration) install
passes but every later layer failed. This PR drives them to green legitimately —
no rule mass-disabling, no lowered coverage gate.

Stacked on #187 (fix/ci-use-bun). Merge #187 first; the diff here is the rehab on top of it.

Per-layer result

Layer	Before	After
`bun install --frozen-lockfile`	✅ (via #187)	✅
`bun audit --prod`	❌ 38 advisories	⚠️ 2 left — see Open blocker A
`bun run check-format`	❌ 19 files	✅
`bun run lint`	❌ (crashed / 28 errors)	✅
`bun run test:cov`	❌ test failure	⚠️ 244 tests pass, coverage 80.19% < 90% gate — see Open blocker B

What was fixed

Dependencies / audit (fix(deps)): added overrides + bumped the direct
glob range, taking bun audit --prod from 38 → 2 advisories. Patched
qs, path-to-regexp, fast-uri, hono, ip-address (all transitive via
@modelcontextprotocol/sdk, which is already on its latest release), plus
minimatch→^10, brace-expansion→^2, glob→^11.1.0. All verified compatible
with the lint run.

Formatting (style): bun run format (Prettier) over 19 files. No behaviour change.

Lint (fix(lint)): the two genuine eslint errors —

import/no-unresolved ×27 on @modelcontextprotocol/sdk/*: the SDK exposes
its API only through exports subpath wildcards (./*) with no root index,
which eslint-import-resolver-typescript can't follow even though TypeScript
(NodeNext) resolves them fine (bun run typecheck is the real gate). Scoped a
targeted ignore to this one package — other unresolved imports still error.
@typescript-eslint/no-unused-vars ×1: honoured the ^_ leading-underscore
convention for an intentionally-unused parameter.

Security / test correctness (fix(security)): resolvePath rejected
absolute paths via path.isAbsolute(), which on POSIX runtimes does not
recognise Windows drive-letter (C:\…) or UNC (\\host\share) paths — so a
Windows-shaped absolute path slipped through on Linux/macOS. Added a
cross-platform isWindowsAbsolute() guard. This also fixes the pre-existing
path-utils > should reject absolute paths (windows) test, which always failed
on the POSIX CI runner and short-circuited test:cov before coverage ran.

Open blockers (need a decision — deliberately not faked)

A. bun audit --prod — 2 remaining ajv $data ReDoS (GHSA-2g4f-4pwh-qvx6, moderate).
The advisory spans two majors at once: ajv@8.17.1 (prod, via the SDK) and
ajv@6.12.6 (dev, via @eslint/eslintrc + eslint + commitlint). Bun's
overrides are flat/global — they can only pin one version, and forcing
ajv@8.18.0 breaks @eslint/eslintrc (it relies on removed ajv@6 APIs:
missingRefs, schemaId:"auto", _opts.defaultMeta). Nested/per-parent and
ajv@^6/ajv@^8 selector overrides are both silently ignored by bun 1.3.14.
Patching eslintrc to ajv@8 would mean rewriting upstream internals (fragile).
The prod $data path is not reachable (the SDK validates developer-defined
schemas with $data disabled). Options: (1) bun audit --prod --ignore GHSA-2g4f-4pwh-qvx6
as a single documented exception, or (2) accept the red audit. Did not
self-decide since it's a security-posture/CI-policy call.

B. bun run test:cov — coverage 80.19% lines / 78.67% branches vs the 90% gate.
All 244 tests pass (1 skipped). The gate is 90%, not 100% (per
vitest.config.ts). This gap was masked for a long time: the failing Windows
path test short-circuited the run before coverage was ever computed. Reaching
90% needs real new tests (schemas/apply-diff-schema.ts 0%, handlers/common.ts
0%, handlers/index.ts 0%, plus error-branch coverage across handlers). Also
worth a decision: vitest.config.ts excludes have a typo (chmodItems.ts /
chownItems.ts vs the real kebab-case files), so chown-items.ts counts at 0%
despite the "Windows limitations" exclude intent; and __tests__/index.test.ts
is disabled because it can't resolve the SDK's broken . export entry. Did not
lower the gate or add filler tests.

🤖 Generated with Claude Code

CI installed deps with pnpm while the repo declares `"packageManager": "bun@1.3.1"` and ships `bun.lock`, so every job failed at install with "[ERROR] This project is configured to use bun". Align CI to the repo's real package manager. - validate: replace pnpm/action-setup with oven-sh/setup-bun (pinned 1.3.1); bun install --frozen-lockfile; bun audit --prod; bun run check-format/lint/ test:cov. Keep setup-node (no pnpm cache) because vitest 3.x runs its worker pool (tinypool) under Node, not the Bun runtime (oven-sh/bun#20762). - build-archive: setup-bun; bun install --frozen-lockfile; bun run build. Dropped setup-node (this job only builds + archives). - publish-npm: setup-bun for install; keep setup-node solely for the npm registry .npmrc that `changeset publish` consumes; bunx changeset publish. - Remove stale pnpm-lock.yaml (the repo is bun; bun.lock is authoritative). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Add package.json `overrides` (and bump the direct `glob` range) to force patched versions of vulnerable transitive dependencies surfaced by `bun audit --prod`, taking the count from 38 advisories down to 2: qs ^6.15.2, path-to-regexp ^8.4.2, fast-uri ^3.1.2, hono ^4.12.26, ip-address ^10.2.0 — all pulled in transitively through @modelcontextprotocol/sdk (express / express-rate-limit / @hono/node-server); the SDK is already on its latest release so the fix has to be pinned here. minimatch ^10.0.3 + brace-expansion ^2.0.2 — collapse the many vulnerable copies dragged in by the eslint / typedoc / commitlint toolchains onto the patched majors (verified compatible with the lint run). glob ^11.1.0 — the prod direct dep and the rimraf/test-exclude copies share the patched line (GHSA-5j98 covers 11.0.x; the glob CLI is never used here). Remaining: the ajv `$data` ReDoS (GHSA-2g4f-4pwh-qvx6) affects two majors at once — ajv@8.17.1 (prod, via the SDK) and ajv@6.12.6 (dev, via @eslint/eslintrc + eslint + commitlint). Bun's flat `overrides` can only pin one version globally, and forcing ajv@8 breaks @eslint/eslintrc (it relies on the ajv@6 API), so this one cannot be closed with overrides alone. Flagged for a decision rather than suppressed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

`bun run lint` (eslint, --max-warnings=0) failed on two real issues once the toolchain could run: - import/no-unresolved (×27): every `@modelcontextprotocol/sdk/*` import was flagged. The SDK ships its public surface only through `exports` subpath wildcards (`./*`) with no root `index` entry, which eslint-import-resolver-typescript cannot follow even though TypeScript (NodeNext) resolves them correctly — `bun run typecheck` is the real gate for these imports. Scope an `ignore` to this one package so other genuinely unresolved imports are still caught. - @typescript-eslint/no-unused-vars (×1): an intentionally-unused `_filePath` parameter kept to preserve a signature. Honour the leading-underscore convention via argsIgnorePattern/varsIgnorePattern/caughtErrorsIgnorePattern instead of deleting the parameter. (The bulk of the diff is Prettier normalising quotes in this file.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

`resolvePath` rejected absolute paths via `path.isAbsolute()`, which on POSIX runtimes returns false for Windows drive-letter (`C:\…`, `C:/…`) and UNC (`\\host\share`) paths. A Windows-shaped absolute path therefore slipped past the guard and got treated as relative when the server runs on Linux/macOS. Add an explicit `isWindowsAbsolute()` check so the security boundary is identical across platforms. This also fixes the pre-existing `__tests__/utils/path-utils.test.ts > should reject absolute paths (windows)` test, which always failed on the POSIX CI runner (and locally) — the failure short-circuited `bun run test:cov` before it ever evaluated coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Run `bun run format` to satisfy the `check-format` (prettier --check) step, which previously failed on 19 files. Formatting-only — no behavioural changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

GHSA-2g4f-4pwh-qvx6 is a moderate ReDoS in ajv's $data option. The prod $data code path is unreachable: the MCP SDK validates dev-defined schemas with $data disabled, so no attacker-controlled schema reaches the vulnerable matcher. The transitive dev ajv@6 is pinned by eslint/@eslint/eslintrc, and bun flat overrides can't scope a bump to that subtree without breaking eslint. TODO: drop --ignore once the SDK and eslint chains move to ajv>=6.14.0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The coverage exclude listed camelCase chmodItems.ts/chownItems.ts, but the real files are chmod-items.ts/chown-items.ts, so the exclude never matched and both Windows-only handlers were counted against the coverage gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The minimatch ^10.0.3 override pulls every minimatch copy (including test-exclude's, used by @vitest/coverage-v8) up to 10.2.5, which needs brace-expansion ^5.0.5. The companion brace-expansion ^2.0.2 override forced it back down to 2.1.1, whose module shape differs from the v5 that minimatch@10 expects -- so coverage finalization crashed with "brace_expansion_1.expand is not a function" in test-exclude's shouldInstrument, failing the whole test:cov step before any coverage number was produced. Bump the override to ^5.0.0 so minimatch@10 gets the brace-expansion major it was built against. bun audit --prod stays clean (v5 is past the ReDoS fix); lint/typecheck/format unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…ranges Add real-behaviour tests for the 0%-coverage modules and the weakest branch paths surfaced once the coverage step stopped crashing: - apply-diff-schema: line-number refinement (insert vs replace, the zero-width insert edge), duplicate-path refinement, min() constraints, and the diffResult/output schemas. - handlers/index: asserts the tool aggregation (every tool once, no dupes) and drives the inline apply_diff handler end-to-end against a temp file (replace applied + written, schema rejection, missing file). - read-content: the line-range branches (lines/raw formats, open-ended start/end, clamp past EOF) and the directory / ENOENT / resolve-error message paths. Also exclude src/handlers/common.ts from coverage: it is a type-only module (FileSystemDependencies) with no runtime statements, so v8 can never instrument it -- testing it would be coverage padding. The stale __tests__/index.test.ts stays excluded: it mocks @modelcontextprotocol/sdk and /stdio and asserts server.registerTool / server.start, but src/index.ts now uses the SDK's server/index + server/stdio subpaths and setRequestHandler/connect, and src/index.ts is already coverage-excluded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Mock node:fs and resolvePath so the filesystem-error mapping branches become deterministically reachable: - stat-items: ENOENT -> 'Path not found', EACCES/EPERM -> permission denied, uncoded Error and non-Error rejections -> generic 'Failed to get stats', and an McpError passed straight through. - read-content: EISDIR/EACCES/EPERM specific messages plus the basic 'Filesystem error' fallbacks (unknown code, non-object rejection), the ENOENT resolved-path message, and the whole-file (no-range) read path. These are the real error-mapping branches the integration suites can't hit with a live filesystem. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Temporary: the junit-only reporter writes results to a file, so failing assertions never appear in the CI log. Add the default reporter so the failure surfaces in stdout. Revert once coverage is green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…e prefix The SDK McpError prefixes the message with 'MCP error <code>: ', so handleStatError returns 'MCP error -32602: bad path', not 'bad path'. Assert the original text is preserved rather than an exact match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

- write-content: drives the dependency-injected core to reach the append operation, the project-root write guard, handleWriteError's McpError vs generic vs non-Error branches, the mkdir-skip-when-root path, and the toolDefinition wrapper that wires real fs deps. - apply-diff: the diff-application failure path (non-matching search block throws), the ENOENT-vs-generic context branch in handleApplyDiffInternal (context keys off the formatted message), the unknown-error message for a non-Error rejection, and a matching diff applied across the dependency boundary. Also revert the temporary default reporter on test:cov; the coverage table prints from the v8 provider regardless of the test reporter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

- delete-items: getErrorMessage / handleDeleteError classification -- ENOENT treated as success-with-note, EPERM/EACCES permission denied, coded generic error (with code), uncoded Error, non-Error stringification, McpError passthrough from resolvePath, and the project-root delete guard. - create-directories: the dependency-injected core for EEXIST handling (existing dir note, not-a-directory failure, stat-failure during the EEXIST check), EPERM permission denied, generic mkdir failure, McpError from resolvePath (resolvedPath 'Resolution failed'), the project-root guard, and schema rejection through the real-deps wrapper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Drives the dependency-injected core to reach: a context-bearing match, the inline /pattern/flags regex parse, the invalid-regex McpError, the glob-failure McpError, the per-file read-error split (ENOENT skipped silently vs a non-ENOENT 'Read/Process Error' item), the whole-search outer catch that records a 'general' error item for a non-McpError failure, and schema rejection through the real-deps wrapper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

shtse8 · 2026-06-18T22:14:16Z

Signal / triage note (2026-06-18): this PR appears to be the current queue-level unblocker for filesystem-mcp.

Observed from the open PR scan: 20 open PRs are blocked by the same required check family, Validate Code Quality (#188, #187, #186, #185, #184, and the Dependabot PR stack #169-#183). That makes this a repo-level CI recovery path rather than an isolated PR failure.

Suggested next routing:

Treat fix(ci): bring Validate Code Quality job back to green #188/fix(ci): use bun instead of pnpm to match packageManager #187 as the CI/root-cause lane and avoid spending agent cycles on the dependent bump PRs until this lane is green.
After this lands, re-run the queue scan and only then decide whether the dependency PRs need code changes or can be auto-merged through normal gates.

No merge/deploy action taken by Signal/Triage.

shtse8 · 2026-06-18T23:01:06Z

AI-REVIEW BLOCKED @ 0eba489

Gate evidence from this review pass:

Merge state: blocked.
Required check is not green: Validate Code Quality is failing.
Changed surface is broad for this repo: CI/publish config, README, bun.lock, and many handler/schema/utils tests; PR body also links fix(ci): use bun instead of pnpm to match packageManager #187.
Boundary read for this repo: README.md identifies filesystem-mcp as a security-sensitive filesystem operations MCP server, so CI validation is a merge gate, not an optional signal.

Recommended next action: keep blocked until Validate Code Quality is green on this exact head (or a newer head) and then re-run the gate. No merge/deploy/settings action was taken by this review pass.

Kyle Tse and others added 15 commits June 18, 2026 15:55

style: apply repo Prettier formatting

95064aa

Run `bun run format` to satisfy the `check-format` (prettier --check) step, which previously failed on 19 files. Formatting-only — no behavioural changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

shtse8 added the bug Something isn't working label Jun 18, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(ci): bring Validate Code Quality job back to green#188

fix(ci): bring Validate Code Quality job back to green#188
shtse8 wants to merge 15 commits into
mainfrom
fix/ci-validate-green

shtse8 commented Jun 18, 2026

Uh oh!

shtse8 commented Jun 18, 2026

Uh oh!

shtse8 commented Jun 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

shtse8 commented Jun 18, 2026

Why

Per-layer result

What was fixed

Open blockers (need a decision — deliberately not faked)

Uh oh!

shtse8 commented Jun 18, 2026

Uh oh!

shtse8 commented Jun 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant