Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ jobs:
node-version: ${{ matrix.node }}
cache: npm
- run: npm ci
# npm >= 11 blocks dependency lifecycle scripts by default, so node-jq's
# preinstall never downloads its jq binary and every --jq call would spawn
# a missing binary. Run its installer directly (not an npm lifecycle hook).
- run: node node_modules/node-jq/scripts/install-binary.mjs
- run: npm run test:coverage
- name: Upload coverage to Codecov
if: matrix.os == 'ubuntu-latest' && matrix.node == 22
Expand Down
5 changes: 5 additions & 0 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ jobs:

- run: npm ci

# npm >= 11 (installed just above for trusted publishing) blocks
# dependency lifecycle scripts, so node-jq's preinstall never fetches its
# jq binary. Run its installer directly so --jq works in the test gate.
- run: node node_modules/node-jq/scripts/install-binary.mjs

# Gate the release on the same checks as CI.
- run: npm run lint
- run: npm run test:coverage
Expand Down
14 changes: 13 additions & 1 deletion src/lib/mcp/invoke.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,18 @@ export function errMessage(e) {
return String(e?.message || e)
}

/**
* After a graceful SIGTERM, force-kill the child if it has not exited within
* `grace` ms. Extracted so the escalation is unit-testable without depending on
* OS signal semantics: Windows terminates a child on the first kill, so a
* real-process test can never exercise this escalation there.
* @param {import('node:child_process').ChildProcess} child
* @param {number} grace
*/
export function scheduleHardKill(child, grace) {
return setTimeout(() => child.kill('SIGKILL'), grace)
}

/**
* Build an executor that spawns the pdcli CLI as a child process. Keeping
* command output in a child process keeps the parent's stdout (the MCP stdio
Expand Down Expand Up @@ -126,7 +138,7 @@ export function makeExec({
const timer = setTimeout(() => {
timedOut = true
child.kill('SIGTERM')
killTimer = setTimeout(() => child.kill('SIGKILL'), grace)
killTimer = scheduleHardKill(child, grace)
}, timeout)
const guard = () => {
if (stdout.length + stderr.length > maxBuffer) {
Expand Down
12 changes: 8 additions & 4 deletions test/lib/mcp/catalog.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -308,10 +308,14 @@ describe('real-config classification audit', () => {
// which would import every command module and skew coverage merging.
function realCommandIds() {
const dir = fileURLToPath(new URL('../../../src/commands', import.meta.url))
return readdirSync(dir, { recursive: true })
.filter((f) => f.endsWith('.js'))
.map((f) => f.replace(/\.js$/, '').split('/').join(':'))
.sort()
return (
readdirSync(dir, { recursive: true })
.filter((f) => f.endsWith('.js'))
// readdirSync yields OS-native separators (backslashes on Windows);
// split on both so command ids normalize to colon form everywhere.
.map((f) => f.replace(/\.js$/, '').split(/[/\\]/).join(':'))
.sort()
)
}

it('classifies every real command exactly as audited', () => {
Expand Down
20 changes: 19 additions & 1 deletion test/lib/mcp/invoke.test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { describe, it, expect } from 'vitest'
import { describe, it, expect, vi } from 'vitest'
import {
toArgv,
runTool,
makeExec,
normalizeExit,
errMessage,
scheduleHardKill,
} from '../../../src/lib/mcp/invoke.js'

const readEntry = {
Expand Down Expand Up @@ -298,6 +299,23 @@ describe('makeExec', () => {
expect((await exec([])).signal).toBe('tool timed out after 0.15s')
})

// Directly exercises the SIGKILL escalation with fake timers and a fake
// child, so it is covered on every OS (a real process on Windows dies on the
// first signal and never reaches the grace escalation).
it('scheduleHardKill force-kills with SIGKILL after the grace period', () => {
vi.useFakeTimers()
try {
const kills = []
const child = { kill: (sig) => kills.push(sig) }
scheduleHardKill(child, 100)
expect(kills).toEqual([])
vi.advanceTimersByTime(100)
expect(kills).toEqual(['SIGKILL'])
} finally {
vi.useRealTimers()
}
})

it('escalates to SIGKILL when the child ignores SIGTERM', async () => {
const exec = makeExec({
command: process.execPath,
Expand Down
11 changes: 5 additions & 6 deletions vitest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@ export default defineConfig({
globals: true,
include: ['test/**/*.test.js'],
setupFiles: ['test/setup.js'],
// node-jq shells out to the jq binary; its subprocess cold-start under
// coverage instrumentation on shared CI runners can spike well past the
// 5s default, timing out --jq tests that run in ~30ms locally. Give CI
// headroom without masking a genuine hang.
testTimeout: 20000,
hookTimeout: 20000,
// The client retry tests exercise real exponential backoff (sleeps that
// sum to ~14s in the worst-case exhaustion path), so the 5s default is too
// tight; give generous headroom, more so on coverage-instrumented CI.
testTimeout: 30000,
hookTimeout: 30000,
coverage: {
include: ['src/**/*.js'],
exclude: ['src/hooks/**'],
Expand Down
Loading