From 68e1cabd899875b8a7182faf4487be0dcfcf60e2 Mon Sep 17 00:00:00 2001 From: Conal <33135619+Conalh@users.noreply.github.com> Date: Wed, 3 Jun 2026 07:29:39 -0700 Subject: [PATCH] fix: make sample/template MCP scanning opt-in so it stops reading as drift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `.mcp.json.template` / `.sample` / `.disabled` / example config never loads into an agent runtime — only the live `.mcp.json` (and the editor configs) do. A change to one therefore cannot alter what an agent is allowed to do, so surfacing it in a *drift* report is noise, not drift. Gate the sample/template detector behind an explicit opt-in rather than removing it, so teams who want copy-paste hygiene on shipped examples can still ask for it: - detectMcpDrift takes { includeSamples } (default false); the sample-scan loop is extracted into detectMcpSampleDrift and only runs when opted in. - git-snapshot skips the full `git ls-tree -r` sample-discovery walk unless includeSamples is set — the detector would ignore the files anyway. - CLI: new `--include-samples` flag, off by default, documented in --help. - Action: new `include-samples` input (default 'false'). - Benchmark passes --include-samples so the labeled mcp-sample fixtures stay exercised; RESULTS.md is unchanged. - Docs (README / TRUST / PILOT) reframed: sample review is opt-in, and why. - Tests: default run yields no sample findings (CLI + detector level); the existing sample tests now opt in explicitly. Follow-up to #49. Co-Authored-By: Claude Opus 4.8 --- README.md | 4 +-- action.yml | 17 +++++++++++- benchmark/run-benchmark.mjs | 7 ++++- dist/detectors/mcp.js | 16 ++++++++++- dist/git-snapshot.js | 18 ++++++++----- dist/index.js | 28 ++++++++++++++----- docs/PILOT.md | 2 +- docs/TRUST.md | 2 +- src/detectors/mcp.ts | 34 ++++++++++++++++++++++- src/git-snapshot.ts | 28 ++++++++++++++----- src/index.ts | 28 ++++++++++++++----- test/git-diff.test.mjs | 54 ++++++++++++++++++++++++++++++------- test/mcp-drift.test.mjs | 27 ++++++++++++++++--- test/public-docs.test.mjs | 6 +++++ 14 files changed, 224 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index 8931965..3405507 100644 --- a/README.md +++ b/README.md @@ -138,7 +138,7 @@ ScopeTrail is **local-only**. It reads the checked-out repository, materializes The detectors cover the surfaces an AI agent can actually escalate through: -- **MCP** — `.mcp.json`, `.cursor/mcp.json`, `.vscode/mcp.json`, `.codeium/windsurf/mcp_config.json`, sample/template/disabled variants, and prefixed sample files such as `claude_mcp_config.json`. +- **MCP** — `.mcp.json`, `.cursor/mcp.json`, `.vscode/mcp.json`, `.codeium/windsurf/mcp_config.json`. Sample/template/disabled variants (and prefixed sample files such as `claude_mcp_config.json`) are reviewed only with `--include-samples` / the Action's `include-samples: true` — those files never load into an agent, so a change to one is not permission drift. - **Claude Code settings** — `.claude/settings.json`, including widened allow rules, removed deny rules, and added / removed / command-swapped hooks. - **Codex** — `.codex/config.toml`, including sandbox elevation, weakened approval policy, network access, trusted-project changes, and `[mcp_servers.NAME]` additions / unpinned commands. @@ -159,7 +159,7 @@ ScopeTrail ships a labeled precision/recall benchmark over **35 fixture PRs** (2 The 8 benign cases include seven engineered **false-positive traps** — narrowly-scoped Claude grants (a textual diff sees new `allow` lines), an all-tightening Codex posture, network access that was *already* on, a brand-new Codex config pinned to the narrowest posture, a dropped MCP `env` var, a removed MCP server, and a `.mcp.json` with reordered keys but an identical launch command — plus one byte-identical snapshot. None produce a finding, because the detectors compare semantics and flag only *widening*. -**Severity is calibrated, not maximized.** At a strict `fail-on: high` gate, recall is 85% — by design: sample/template MCP additions, pinned version bumps, broad `Read` allows, and newly-enabled Codex network access sit at `low`/`medium` because they widen the surface without being directly exploitable. The `high`/`critical` band is reserved for executable or secret-facing changes — a bare `Bash` grant, a removed `Read(.env)` deny, a `danger-full-access` sandbox, an unencrypted remote MCP endpoint. Full confusion matrix at every gate, per-category and per-case breakdowns: [benchmark/RESULTS.md](benchmark/RESULTS.md). Methodology and labels: [benchmark/labels.json](benchmark/labels.json). +**Severity is calibrated, not maximized.** At a strict `fail-on: high` gate, recall is 85% — by design: opt-in sample/template MCP additions, pinned version bumps, broad `Read` allows, and newly-enabled Codex network access sit at `low`/`medium` because they widen the surface without being directly exploitable. The `high`/`critical` band is reserved for executable or secret-facing changes — a bare `Bash` grant, a removed `Read(.env)` deny, a `danger-full-access` sandbox, an unencrypted remote MCP endpoint. Full confusion matrix at every gate, per-category and per-case breakdowns: [benchmark/RESULTS.md](benchmark/RESULTS.md). Methodology and labels: [benchmark/labels.json](benchmark/labels.json). ## Design choices worth flagging diff --git a/action.yml b/action.yml index e00ef17..0a1073a 100644 --- a/action.yml +++ b/action.yml @@ -23,6 +23,12 @@ inputs: description: Severity that fails the action. Use none, low, medium, high, or critical. required: false default: none + include-samples: + description: >- + Also review sample/template/disabled MCP configs (.mcp.json.template, .sample, prefixed examples). + Off by default because those files never load into an agent, so changes to them are not permission drift. + required: false + default: 'false' outputs: rating: @@ -47,6 +53,7 @@ runs: SCOPE_BASE: ${{ inputs.base }} SCOPE_HEAD: ${{ inputs.head }} SCOPE_FAIL_ON: ${{ inputs.fail-on }} + SCOPE_INCLUDE_SAMPLES: ${{ inputs.include-samples }} DEFAULT_BASE: ${{ github.event.pull_request.base.sha || github.event.before }} DEFAULT_HEAD: ${{ github.event.pull_request.head.sha || github.sha }} run: | @@ -57,6 +64,13 @@ runs: head="${SCOPE_HEAD:-$DEFAULT_HEAD}" fail_on="${SCOPE_FAIL_ON:-none}" + # Optional opt-in: review sample/template/disabled MCP configs too. + # Empty unless requested, so the unquoted expansion below adds no arg. + include_samples="" + if [ "${SCOPE_INCLUDE_SAMPLES:-false}" = "true" ]; then + include_samples="--include-samples" + fi + if [ -z "$base" ] || [ -z "$head" ]; then echo "::error::ScopeTrail needs base and head refs. Pass base/head inputs or run on pull_request with actions/checkout fetch-depth: 0." exit 2 @@ -77,7 +91,8 @@ runs: --format github \ --out-markdown "$report_file" \ --out-json "$json_file" \ - --fail-on "$fail_on" + --fail-on "$fail_on" \ + $include_samples cli_status=$? set -e diff --git a/benchmark/run-benchmark.mjs b/benchmark/run-benchmark.mjs index 0888295..9867c8d 100644 --- a/benchmark/run-benchmark.mjs +++ b/benchmark/run-benchmark.mjs @@ -44,7 +44,12 @@ function runDiff(fixture) { '--new', join(caseDir, 'new'), '--format', - 'json' + 'json', + // The corpus labels sample/template MCP fixtures (category `mcp-sample`) as + // expected detections, so the benchmark opts into that surface explicitly. + // The default CLI report leaves it off — those files never load into an + // agent, so a change to one is not permission drift. + '--include-samples' ]; const stdout = execFileSync('node', [cli, ...args], { encoding: 'utf8', diff --git a/dist/detectors/mcp.js b/dist/detectors/mcp.js index e308ea3..dfeb789 100644 --- a/dist/detectors/mcp.js +++ b/dist/detectors/mcp.js @@ -47,7 +47,7 @@ const IGNORED_SAMPLE_SCAN_DIRS = new Set([ // reads. Keeping the source of truth in the detector prevents the // snapshot list and the detector list from drifting (they did, before). export const MCP_TARGET_PATHS = MCP_CONFIGS.map((config) => config.path); -export async function detectMcpDrift(oldRoot, newRoot) { +export async function detectMcpDrift(oldRoot, newRoot, options = {}) { const findings = []; for (const config of MCP_CONFIGS) { // Surface invalid JSON as a finding instead of silently producing @@ -139,6 +139,20 @@ export async function detectMcpDrift(oldRoot, newRoot) { } } } + // Sample/template configs are an opt-in surface — see McpDriftOptions for why + // a file that no agent loads can't be drift. Off by default keeps the report + // scoped to live configuration. + if (options.includeSamples) { + findings.push(...(await detectMcpSampleDrift(oldRoot, newRoot))); + } + return findings; +} +// Diff sample/template/disabled MCP configs on their own low-severity track so +// a noisy template change can be reviewed for copy-paste hygiene without ever +// being mistaken for a change to what an agent can actually do. Only runs when +// the caller opts in via McpDriftOptions.includeSamples. +async function detectMcpSampleDrift(oldRoot, newRoot) { + const findings = []; for (const path of await listMcpSampleConfigPaths(oldRoot, newRoot)) { const config = { path, serverKeys: ['mcpServers', 'servers'] }; const newSource = await readJsonObjectWithSource(configPath(newRoot, path)); diff --git a/dist/git-snapshot.js b/dist/git-snapshot.js index 5d291c3..478d282 100644 --- a/dist/git-snapshot.js +++ b/dist/git-snapshot.js @@ -18,12 +18,12 @@ export const SNAPSHOT_PATHS = [ ...CLAUDE_TARGET_PATHS, ...CODEX_TARGET_PATHS ]; -export async function materializeGitSnapshot(repo, ref) { +export async function materializeGitSnapshot(repo, ref, options = {}) { await verifyGitRef(repo, ref); const root = await mkdtemp(join(tmpdir(), 'scopetrail-snapshot-')); let completed = false; try { - for (const relativePath of await snapshotPathsForRef(repo, ref)) { + for (const relativePath of await snapshotPathsForRef(repo, ref, options.includeSamples ?? false)) { const content = await readPathAtRef(repo, ref, relativePath); if (content === null) { continue; @@ -53,11 +53,17 @@ export async function materializeGitSnapshot(repo, ref) { } } } -async function snapshotPathsForRef(repo, ref) { +async function snapshotPathsForRef(repo, ref, includeSamples) { const paths = new Set(SNAPSHOT_PATHS); - for (const relativePath of await listPathsAtRef(repo, ref)) { - if (isMcpSampleConfigPath(relativePath)) { - paths.add(relativePath); + // Sample/template configs are opt-in (see McpDriftOptions). When the caller + // hasn't asked for them, skip the full `git ls-tree -r` walk entirely — the + // detector would ignore the materialized files anyway, so listing every + // tracked path on every PR is wasted work. + if (includeSamples) { + for (const relativePath of await listPathsAtRef(repo, ref)) { + if (isMcpSampleConfigPath(relativePath)) { + paths.add(relativePath); + } } } return [...paths].sort(); diff --git a/dist/index.js b/dist/index.js index 6ec1037..8fa10b8 100644 --- a/dist/index.js +++ b/dist/index.js @@ -33,12 +33,16 @@ async function runDiff(argv) { } else { try { - const baseSnapshot = await materializeGitSnapshot(parsed.repo, parsed.base); + const baseSnapshot = await materializeGitSnapshot(parsed.repo, parsed.base, { + includeSamples: parsed.includeSamples + }); // `cleanup` is only assigned once BOTH snapshots exist, so if head // materialization fails (an unresolvable head ref, a max-buffer error) // the base snapshot's temp dir would leak. Clean it explicitly before // the error propagates to the handler below. - const headSnapshot = await materializeGitSnapshot(parsed.repo, parsed.head).catch(async (headError) => { + const headSnapshot = await materializeGitSnapshot(parsed.repo, parsed.head, { + includeSamples: parsed.includeSamples + }).catch(async (headError) => { await baseSnapshot.cleanup(); throw headError; }); @@ -62,7 +66,7 @@ async function runDiff(argv) { // the CLI three times for markdown/json/github, which repeated // git snapshot materialization and detector work on each call. const findings = [ - ...(await detectMcpDrift(oldRoot, newRoot)), + ...(await detectMcpDrift(oldRoot, newRoot, { includeSamples: parsed.includeSamples })), ...(await detectClaudeSettingsDrift(oldRoot, newRoot)), ...(await detectCodexConfigDrift(oldRoot, newRoot)) ]; @@ -94,6 +98,7 @@ function parseDiffArgs(argv) { let outMarkdown; let outJson; let failOn = 'none'; + let includeSamples = false; for (let index = 0; index < argv.length; index += 1) { const arg = argv[index]; const value = argv[index + 1]; @@ -145,6 +150,12 @@ function parseDiffArgs(argv) { failOn = value; index += 1; } + else if (arg === '--include-samples') { + // Boolean flag — opts into reviewing sample/template/disabled MCP configs + // (`.mcp.json.template`, prefixed examples, ...). Off by default because + // those files never load into an agent, so a change to one is not drift. + includeSamples = true; + } else { return { ok: false, error: `Unknown argument: ${arg}` }; } @@ -161,7 +172,7 @@ function parseDiffArgs(argv) { if (!head) { return { ok: false, error: 'Missing required --head argument.' }; } - return { ok: true, mode: 'git', repo, base, head, format, outMarkdown, outJson, failOn }; + return { ok: true, mode: 'git', repo, base, head, format, outMarkdown, outJson, failOn, includeSamples }; } if (!oldRoot) { return { ok: false, error: 'Missing required --old argument or --base argument.' }; @@ -169,7 +180,7 @@ function parseDiffArgs(argv) { if (!newRoot) { return { ok: false, error: 'Missing required --new argument.' }; } - return { ok: true, mode: 'directories', oldRoot, newRoot, format, outMarkdown, outJson, failOn }; + return { ok: true, mode: 'directories', oldRoot, newRoot, format, outMarkdown, outJson, failOn, includeSamples }; } function isReportFormat(value) { return value === 'text' || value === 'markdown' || value === 'json' || value === 'github'; @@ -198,7 +209,10 @@ if (isMainModule()) { function usage() { return [ 'Usage:', - ' scopetrail diff --old --new [--format text|markdown|json|github] [--out-markdown PATH] [--out-json PATH] [--fail-on none|low|medium|high|critical]', - ' scopetrail diff --repo --base --head [--format text|markdown|json|github] [--out-markdown PATH] [--out-json PATH] [--fail-on none|low|medium|high|critical]' + ' scopetrail diff --old --new [--format text|markdown|json|github] [--out-markdown PATH] [--out-json PATH] [--fail-on none|low|medium|high|critical] [--include-samples]', + ' scopetrail diff --repo --base --head [--format text|markdown|json|github] [--out-markdown PATH] [--out-json PATH] [--fail-on none|low|medium|high|critical] [--include-samples]', + '', + ' --include-samples Also review sample/template/disabled MCP configs (.mcp.json.template, .sample, prefixed examples).', + ' Off by default: those files never load into an agent, so changes to them are not permission drift.' ].join('\n'); } diff --git a/docs/PILOT.md b/docs/PILOT.md index 3a1f7b1..83ff8fd 100644 --- a/docs/PILOT.md +++ b/docs/PILOT.md @@ -36,7 +36,7 @@ Useful checks during the trial: - Did ScopeTrail catch real permission drift? - Did any warning feel noisy or too broad? -- Did sample/template/disabled MCP config findings, including platform-suffixed and prefixed MCP config examples, correctly stay separate from active MCP server drift? +- If you opted in with `include-samples`, did sample/template/disabled MCP config findings, including platform-suffixed and prefixed MCP config examples, correctly stay separate from active MCP server drift? - Did it miss an agent config surface your repository uses? - Would a team workflow need cross-repo visibility, policy ownership, exception workflow, or reporting? diff --git a/docs/TRUST.md b/docs/TRUST.md index d1a2088..93ad17e 100644 --- a/docs/TRUST.md +++ b/docs/TRUST.md @@ -6,7 +6,7 @@ ScopeTrail is a local-only GitHub Action and CLI for reviewing AI-agent permissi ScopeTrail reads the checked-out repository and compares supported agent configuration files between the pull request base and head refs. Supported active files include `.mcp.json`, `.cursor/mcp.json`, `.vscode/mcp.json`, `.codeium/windsurf/mcp_config.json`, `.claude/settings.json`, and `.codex/config.toml`. -ScopeTrail also reviews sample/template/disabled MCP config files such as `.mcp.json.sample`, `.mcp.json.template`, `.mcp.json.disabled`, `.mcp.json.example`, platform-suffixed MCP example files such as `.mcp.json.windows.example` and `.mcp.json.example.mac`, nested `mcp_config.json.example` variants, and prefixed MCP config example files such as `example_mcp_config.json`, `claude_mcp_config.json`, `cursor_mcp_config.json`, and `vscode_mcp_config.json`. Those findings are reported separately from active MCP server drift so copied examples can be reviewed without implying they are live configuration. +ScopeTrail can also review sample/template/disabled MCP config files such as `.mcp.json.sample`, `.mcp.json.template`, `.mcp.json.disabled`, `.mcp.json.example`, platform-suffixed MCP example files such as `.mcp.json.windows.example` and `.mcp.json.example.mac`, nested `mcp_config.json.example` variants, and prefixed MCP config example files such as `example_mcp_config.json`, `claude_mcp_config.json`, `cursor_mcp_config.json`, and `vscode_mcp_config.json`. This is **off by default**: those files never load into an agent runtime, so a change to one cannot alter what an agent is allowed to do, and reporting it as drift would be noise. Pass `--include-samples` (CLI) or set `include-samples: true` (Action) to opt in. When enabled, those findings are reported separately from active MCP server drift so copied examples can be reviewed without implying they are live configuration. In GitHub Actions, `fetch-depth: 0` is required so ScopeTrail can compare the pull request base and head commits instead of only seeing the latest checkout. diff --git a/src/detectors/mcp.ts b/src/detectors/mcp.ts index c7ebe99..198a062 100644 --- a/src/detectors/mcp.ts +++ b/src/detectors/mcp.ts @@ -74,7 +74,22 @@ interface McpServerModel extends McpServerConfig { sourceText?: string; } -export async function detectMcpDrift(oldRoot: string, newRoot: string): Promise { +export interface McpDriftOptions { + // Sample/template/disabled MCP configs (`.mcp.json.template`, `.sample`, + // prefixed examples, ...) never load into an agent runtime — only the live + // `.mcp.json` and editor configs do. A change to a template therefore can't + // alter what an agent is actually allowed to do, so for a *drift* detector it + // is noise, not drift. Scanning them is opt-in: teams who want copy-paste + // hygiene on shipped examples ask for it explicitly; the default report stays + // scoped to surfaces an agent actually loads. + includeSamples?: boolean; +} + +export async function detectMcpDrift( + oldRoot: string, + newRoot: string, + options: McpDriftOptions = {} +): Promise { const findings: Finding[] = []; for (const config of MCP_CONFIGS) { @@ -173,6 +188,23 @@ export async function detectMcpDrift(oldRoot: string, newRoot: string): Promise< } } + // Sample/template configs are an opt-in surface — see McpDriftOptions for why + // a file that no agent loads can't be drift. Off by default keeps the report + // scoped to live configuration. + if (options.includeSamples) { + findings.push(...(await detectMcpSampleDrift(oldRoot, newRoot))); + } + + return findings; +} + +// Diff sample/template/disabled MCP configs on their own low-severity track so +// a noisy template change can be reviewed for copy-paste hygiene without ever +// being mistaken for a change to what an agent can actually do. Only runs when +// the caller opts in via McpDriftOptions.includeSamples. +async function detectMcpSampleDrift(oldRoot: string, newRoot: string): Promise { + const findings: Finding[] = []; + for (const path of await listMcpSampleConfigPaths(oldRoot, newRoot)) { const config = { path, serverKeys: ['mcpServers', 'servers'] }; const newSource = await readJsonObjectWithSource(configPath(newRoot, path)); diff --git a/src/git-snapshot.ts b/src/git-snapshot.ts index ddb6229..b25fc0a 100644 --- a/src/git-snapshot.ts +++ b/src/git-snapshot.ts @@ -26,13 +26,23 @@ export interface GitSnapshot { cleanup: () => Promise; } -export async function materializeGitSnapshot(repo: string, ref: string): Promise { +export interface GitSnapshotOptions { + // Mirror of McpDriftOptions.includeSamples: when the detector won't look at + // sample/template configs, don't walk the tree to materialize them either. + includeSamples?: boolean; +} + +export async function materializeGitSnapshot( + repo: string, + ref: string, + options: GitSnapshotOptions = {} +): Promise { await verifyGitRef(repo, ref); const root = await mkdtemp(join(tmpdir(), 'scopetrail-snapshot-')); let completed = false; try { - for (const relativePath of await snapshotPathsForRef(repo, ref)) { + for (const relativePath of await snapshotPathsForRef(repo, ref, options.includeSamples ?? false)) { const content = await readPathAtRef(repo, ref, relativePath); if (content === null) { continue; @@ -65,11 +75,17 @@ export async function materializeGitSnapshot(repo: string, ref: string): Promise } } -async function snapshotPathsForRef(repo: string, ref: string): Promise { +async function snapshotPathsForRef(repo: string, ref: string, includeSamples: boolean): Promise { const paths = new Set(SNAPSHOT_PATHS); - for (const relativePath of await listPathsAtRef(repo, ref)) { - if (isMcpSampleConfigPath(relativePath)) { - paths.add(relativePath); + // Sample/template configs are opt-in (see McpDriftOptions). When the caller + // hasn't asked for them, skip the full `git ls-tree -r` walk entirely — the + // detector would ignore the materialized files anyway, so listing every + // tracked path on every PR is wasted work. + if (includeSamples) { + for (const relativePath of await listPathsAtRef(repo, ref)) { + if (isMcpSampleConfigPath(relativePath)) { + paths.add(relativePath); + } } } diff --git a/src/index.ts b/src/index.ts index 66bd2ed..b092985 100644 --- a/src/index.ts +++ b/src/index.ts @@ -46,12 +46,16 @@ async function runDiff(argv: string[]): Promise { newRoot = parsed.newRoot; } else { try { - const baseSnapshot = await materializeGitSnapshot(parsed.repo, parsed.base); + const baseSnapshot = await materializeGitSnapshot(parsed.repo, parsed.base, { + includeSamples: parsed.includeSamples + }); // `cleanup` is only assigned once BOTH snapshots exist, so if head // materialization fails (an unresolvable head ref, a max-buffer error) // the base snapshot's temp dir would leak. Clean it explicitly before // the error propagates to the handler below. - const headSnapshot = await materializeGitSnapshot(parsed.repo, parsed.head).catch( + const headSnapshot = await materializeGitSnapshot(parsed.repo, parsed.head, { + includeSamples: parsed.includeSamples + }).catch( async (headError: unknown) => { await baseSnapshot.cleanup(); throw headError; @@ -77,7 +81,7 @@ async function runDiff(argv: string[]): Promise { // the CLI three times for markdown/json/github, which repeated // git snapshot materialization and detector work on each call. const findings = [ - ...(await detectMcpDrift(oldRoot, newRoot)), + ...(await detectMcpDrift(oldRoot, newRoot, { includeSamples: parsed.includeSamples })), ...(await detectClaudeSettingsDrift(oldRoot, newRoot)), ...(await detectCodexConfigDrift(oldRoot, newRoot)) ]; @@ -107,6 +111,7 @@ interface CommonDiffArgs { outMarkdown?: string; outJson?: string; failOn: DriftRating; + includeSamples: boolean; } type ParsedDiffArgs = @@ -124,6 +129,7 @@ function parseDiffArgs(argv: string[]): ParsedDiffArgs { let outMarkdown: string | undefined; let outJson: string | undefined; let failOn: DriftRating = 'none'; + let includeSamples = false; for (let index = 0; index < argv.length; index += 1) { const arg = argv[index]; @@ -168,6 +174,11 @@ function parseDiffArgs(argv: string[]): ParsedDiffArgs { } failOn = value; index += 1; + } else if (arg === '--include-samples') { + // Boolean flag — opts into reviewing sample/template/disabled MCP configs + // (`.mcp.json.template`, prefixed examples, ...). Off by default because + // those files never load into an agent, so a change to one is not drift. + includeSamples = true; } else { return { ok: false, error: `Unknown argument: ${arg}` }; } @@ -189,7 +200,7 @@ function parseDiffArgs(argv: string[]): ParsedDiffArgs { return { ok: false, error: 'Missing required --head argument.' }; } - return { ok: true, mode: 'git', repo, base, head, format, outMarkdown, outJson, failOn }; + return { ok: true, mode: 'git', repo, base, head, format, outMarkdown, outJson, failOn, includeSamples }; } if (!oldRoot) { @@ -200,7 +211,7 @@ function parseDiffArgs(argv: string[]): ParsedDiffArgs { return { ok: false, error: 'Missing required --new argument.' }; } - return { ok: true, mode: 'directories', oldRoot, newRoot, format, outMarkdown, outJson, failOn }; + return { ok: true, mode: 'directories', oldRoot, newRoot, format, outMarkdown, outJson, failOn, includeSamples }; } function isReportFormat(value: string | undefined): value is ReportFormat { @@ -232,7 +243,10 @@ if (isMainModule()) { function usage(): string { return [ 'Usage:', - ' scopetrail diff --old --new [--format text|markdown|json|github] [--out-markdown PATH] [--out-json PATH] [--fail-on none|low|medium|high|critical]', - ' scopetrail diff --repo --base --head [--format text|markdown|json|github] [--out-markdown PATH] [--out-json PATH] [--fail-on none|low|medium|high|critical]' + ' scopetrail diff --old --new [--format text|markdown|json|github] [--out-markdown PATH] [--out-json PATH] [--fail-on none|low|medium|high|critical] [--include-samples]', + ' scopetrail diff --repo --base --head [--format text|markdown|json|github] [--out-markdown PATH] [--out-json PATH] [--fail-on none|low|medium|high|critical] [--include-samples]', + '', + ' --include-samples Also review sample/template/disabled MCP configs (.mcp.json.template, .sample, prefixed examples).', + ' Off by default: those files never load into an agent, so changes to them are not permission drift.' ].join('\n'); } diff --git a/test/git-diff.test.mjs b/test/git-diff.test.mjs index 0f3aae6..b445ec6 100644 --- a/test/git-diff.test.mjs +++ b/test/git-diff.test.mjs @@ -10,12 +10,12 @@ const execFileAsync = promisify(execFile); const testDir = dirname(fileURLToPath(import.meta.url)); const packageRoot = join(testDir, '..'); -async function runDiff(repo, base, head) { - const { stdout } = await execFileAsync( - process.execPath, - ['dist/index.js', 'diff', '--repo', repo, '--base', base, '--head', head, '--format', 'json'], - { cwd: packageRoot } - ); +async function runDiff(repo, base, head, { includeSamples = false } = {}) { + const args = ['dist/index.js', 'diff', '--repo', repo, '--base', base, '--head', head, '--format', 'json']; + if (includeSamples) { + args.push('--include-samples'); + } + const { stdout } = await execFileAsync(process.execPath, args, { cwd: packageRoot }); return JSON.parse(stdout); } @@ -122,7 +122,7 @@ test('CLI git diff snapshots sample MCP config paths', async () => { 'add sample mcp config' ); - const report = await runDiff(fx.repo, base, head); + const report = await runDiff(fx.repo, base, head, { includeSamples: true }); assert.deepEqual( report.findings.map((finding) => finding.kind), @@ -135,6 +135,42 @@ test('CLI git diff snapshots sample MCP config paths', async () => { } }); +test('CLI git diff leaves sample MCP config paths unscanned without --include-samples', async () => { + // Regression for the opt-in default: a committed `.mcp.json.sample` must not + // surface in the report unless the caller passes --include-samples. Template + // and sample files never load into an agent, so they are not permission drift. + const fx = await makeGitRepo({ + prefix: 'scopetrail-git-sample-default-', + initialFiles: { 'README.md': 'base\n' }, + initialMessage: 'base', + }); + try { + const base = await fx.head(); + const head = await fx.commit( + { + 'examples/.mcp.json.sample': `${JSON.stringify( + { + mcpServers: { + 'copy-risk': { + command: 'npx', + args: ['-y', '@acme/copy-risk@latest'] + } + } + }, + null, + 2 + )}\n`, + }, + 'add sample mcp config (default run)' + ); + + const report = await runDiff(fx.repo, base, head); + assert.deepEqual(report.findings, [], 'sample paths must not be scanned by default'); + } finally { + await fx.cleanup(); + } +}); + test('CLI git diff snapshots platform-suffixed MCP example paths', async () => { const fx = await makeGitRepo({ prefix: 'scopetrail-git-platform-sample-', @@ -161,7 +197,7 @@ test('CLI git diff snapshots platform-suffixed MCP example paths', async () => { 'add platform sample mcp config' ); - const report = await runDiff(fx.repo, base, head); + const report = await runDiff(fx.repo, base, head, { includeSamples: true }); assert.deepEqual( report.findings.map((finding) => finding.kind), @@ -311,7 +347,7 @@ test('CLI git diff snapshots prefixed MCP config example paths', async () => { 'add prefixed sample mcp config' ); - const report = await runDiff(fx.repo, base, head); + const report = await runDiff(fx.repo, base, head, { includeSamples: true }); assert.deepEqual( report.findings.map((finding) => finding.kind), diff --git a/test/mcp-drift.test.mjs b/test/mcp-drift.test.mjs index 370af4f..cb8be00 100644 --- a/test/mcp-drift.test.mjs +++ b/test/mcp-drift.test.mjs @@ -97,7 +97,7 @@ test('mcp_sample_remote_endpoint: http:// fires high severity, https:// stays me }, null, 2) ); - const findings = await detectMcpDrift(oldDir, newDir); + const findings = await detectMcpDrift(oldDir, newDir, { includeSamples: true }); const remoteEndpoint = findings.filter((f) => f.kind === 'scope_trail.mcp_sample_remote_endpoint'); const bySubject = Object.fromEntries(remoteEndpoint.map((f) => [f.subject, f])); @@ -382,11 +382,30 @@ test('detects MCP drift in Windsurf config files', async () => { ); }); +test('sample MCP configs are not reviewed unless includeSamples is set (opt-in)', async () => { + // The Signal that drove this gate: a `.mcp.json.sample` / `.template` never + // loads into an agent runtime, so a change to one cannot widen what the agent + // is allowed to do. For a *drift* detector that makes it noise, not drift — + // the default report must stay silent on samples. Review is opt-in. + const oldDir = join(testDir, 'fixtures', 'mcp-sample-drift', 'old'); + const newDir = join(testDir, 'fixtures', 'mcp-sample-drift', 'new'); + + const defaultFindings = await detectMcpDrift(oldDir, newDir); + assert.deepEqual(defaultFindings, [], 'sample configs must produce no findings by default'); + + // The opt-in re-enables the same fixture's sample findings — gated, not gone. + const optedIn = await detectMcpDrift(oldDir, newDir, { includeSamples: true }); + assert.ok( + optedIn.some((finding) => finding.kind === 'scope_trail.mcp_sample_server_added'), + 'includeSamples must re-enable sample review' + ); +}); + test('detects sample MCP config drift without treating it as active server drift', async () => { const oldDir = join(testDir, 'fixtures', 'mcp-sample-drift', 'old'); const newDir = join(testDir, 'fixtures', 'mcp-sample-drift', 'new'); - const findings = await detectMcpDrift(oldDir, newDir); + const findings = await detectMcpDrift(oldDir, newDir, { includeSamples: true }); assert.equal(findings.some((finding) => finding.kind === 'scope_trail.mcp_server_added'), false); assert.equal(findings.some((finding) => finding.kind === 'scope_trail.unpinned_mcp_command'), false); @@ -423,7 +442,7 @@ test('detects platform-suffixed MCP example drift without treating it as active const oldDir = join(testDir, 'fixtures', 'mcp-platform-sample-drift', 'old'); const newDir = join(testDir, 'fixtures', 'mcp-platform-sample-drift', 'new'); - const findings = await detectMcpDrift(oldDir, newDir); + const findings = await detectMcpDrift(oldDir, newDir, { includeSamples: true }); assert.equal(findings.some((finding) => finding.kind === 'scope_trail.mcp_server_added'), false); assert.equal(findings.some((finding) => finding.kind === 'scope_trail.unpinned_mcp_command'), false); @@ -442,7 +461,7 @@ test('detects prefixed MCP config example drift without treating it as active se const oldDir = join(testDir, 'fixtures', 'mcp-prefixed-sample-drift', 'old'); const newDir = join(testDir, 'fixtures', 'mcp-prefixed-sample-drift', 'new'); - const findings = await detectMcpDrift(oldDir, newDir); + const findings = await detectMcpDrift(oldDir, newDir, { includeSamples: true }); assert.equal(findings.some((finding) => finding.kind === 'scope_trail.mcp_server_added'), false); assert.equal(findings.some((finding) => finding.kind === 'scope_trail.unpinned_mcp_command'), false); diff --git a/test/public-docs.test.mjs b/test/public-docs.test.mjs index 699a712..97e2040 100644 --- a/test/public-docs.test.mjs +++ b/test/public-docs.test.mjs @@ -55,6 +55,12 @@ test('public docs describe active and sample MCP config coverage', async () => { assert.match(trust, /prefixed MCP config example files/i); assert.match(pilot, /sample\/template\/disabled MCP config findings/i); assert.match(pilot, /prefixed MCP config examples/i); + + // Sample review is opt-in (off by default) — the docs must say so, and name + // the flag, so a reader never expects template changes to flag by default. + assert.match(readme, /include-samples/); + assert.match(trust, /off by default/i); + assert.match(trust, /include-samples/); }); test('adoption checklist defines advisory-first rollout and feedback path', async () => {