diff --git a/src/codeLens.test.ts b/src/codeLens.test.ts index 1b961d9..b8981dc 100644 --- a/src/codeLens.test.ts +++ b/src/codeLens.test.ts @@ -176,4 +176,106 @@ describe("FindingsCodeLensProvider — pipelineCheck.codeLens.enabled toggle", ( const p = new FindingsCodeLensProvider(ctx); expect(p.provideCodeLenses(document)).toEqual([]); }); + + it("anchors the lens at line 0 column 0 (top of file)", () => { + // The lens sits *above* line 1 so it doesn't push the first + // line of YAML out of view. A future refactor that moves it to + // the first finding's line would change UX without anyone + // noticing; lock it down. + const p = new FindingsCodeLensProvider(ctx); + const lenses = p.provideCodeLenses(document) as unknown[]; + expect(lenses).toHaveLength(1); + const lens = lenses[0] as { range: { start: { line: number; character: number } } }; + expect(lens.range.start.line).toBe(0); + expect(lens.range.start.character).toBe(0); + }); + + it("targets the pipelineCheck.findings.focus command so click reveals the panel", () => { + // Other plausible click targets (vscode.open, the file at the + // finding location, the rule docs URL) all do different things. + // The lens is meant as a *drill-in* — surface the panel grouped + // by severity so the user can scan the per-file count in context. + const p = new FindingsCodeLensProvider(ctx); + const lenses = p.provideCodeLenses(document) as unknown[]; + const lens = lenses[0] as { + command?: { command: string; title: string }; + }; + expect(lens.command?.command).toBe("pipelineCheck.findings.focus"); + }); + + it("renders the title from the live count, not the constructor snapshot", () => { + // Without rebuilding the provider, the lens text must reflect + // whatever the diagnostic store says right now. (The provider + // calls summariseCounts inside provideCodeLenses each time, but + // the test exists to catch a future "cache once at construction" + // refactor.) + const p = new FindingsCodeLensProvider(ctx); + const lenses1 = p.provideCodeLenses(document) as unknown[]; + const title1 = (lenses1[0] as { command?: { title: string } }).command + ?.title; + expect(title1).toContain("1 critical"); + + // Now swap the stub diagnostics underneath the provider and + // request fresh lenses. + (globalThis as { __stubDiagnostics?: unknown }).__stubDiagnostics = [ + [ + { toString: () => "file:///a.yml" }, + [ + { + source: "pipeline-check", + message: "", + range: { + start: { line: 0, character: 0 }, + end: { line: 0, character: 0 }, + }, + severity: 0, + data: { severity: "HIGH" }, + }, + { + source: "pipeline-check", + message: "", + range: { + start: { line: 0, character: 0 }, + end: { line: 0, character: 0 }, + }, + severity: 0, + data: { severity: "HIGH" }, + }, + ], + ], + ]; + const lenses2 = p.provideCodeLenses(document) as unknown[]; + const title2 = (lenses2[0] as { command?: { title: string } }).command + ?.title; + expect(title2).toContain("2 high"); + expect(title2).not.toContain("critical"); + }); + + it("only considers the document's OWN diagnostics, not the workspace total", () => { + // The lens is per-file; the Findings panel is the workspace + // aggregate. Confusing the two would show "10 critical" on a + // file with zero findings just because the workspace has them. + (globalThis as { __stubDiagnostics?: unknown }).__stubDiagnostics = [ + [ + { toString: () => "file:///OTHER.yml" }, + [ + { + source: "pipeline-check", + message: "", + range: { + start: { line: 0, character: 0 }, + end: { line: 0, character: 0 }, + }, + severity: 0, + data: { severity: "CRITICAL" }, + }, + ], + ], + ]; + const p = new FindingsCodeLensProvider(ctx); + // The stub's getDiagnostics(uri) returns diagnostics keyed by + // `uri.toString()`; our document is "file:///a.yml", which is + // not in the stubbed store. Should yield no lenses. + expect(p.provideCodeLenses(document)).toEqual([]); + }); }); diff --git a/src/extension.ts b/src/extension.ts index 728b38a..ca80ed2 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -26,14 +26,15 @@ import { installInTerminal, } from "./install"; import * as clientLog from "./log"; +import { startWithTimeout } from "./lspStart"; import { setLspReady } from "./lspState"; +import { transformDiagnostics } from "./middleware"; import { goToFinding } from "./navigate"; import { providerForPath, - type ProviderId, TRIGGER_DOCUMENT_SELECTOR, } from "./providers"; -import { filterByThreshold } from "./severityFilter"; +import { createScanOnSaveHandler } from "./scanOnSave"; import { registerStatusBar } from "./statusBar"; import { scanWorkspace } from "./workspaceScan"; import { showWhatsNewIfUpgraded } from "./whatsNew"; @@ -155,30 +156,21 @@ function buildClient(): LanguageClient { }, outputChannelName: OUTPUT_CHANNEL, middleware: { - // Drop diagnostics below the user-configured severity threshold - // before they reach VS Code's Problems panel. The config is - // re-read on each publish so a settings change takes effect on - // the next scan without needing a server restart. Diagnostics - // whose `data.severity` is missing (older server, or a - // not-from-pipeline-check publish that somehow flowed through) - // pass through unconditionally so the filter never hides - // legitimate signal when the metadata is absent. + // Two-stage filter (composition lives in middleware.ts): drop + // every diagnostic for a URI whose provider the user has + // silenced via `disabledProviders`, otherwise drop those below + // the configured `severityThreshold`. Re-reads the config on + // each publish so a settings change takes effect on the next + // scan without a server restart. handleDiagnostics: (uri, diagnostics, next) => { const config = vscode.workspace.getConfiguration("pipelineCheck"); - // Per-provider toggle: if this URI maps to a provider the - // user has disabled, drop every diagnostic for it. We still - // accept the publish (so a future "unset disable" causes a - // fresh publish to reach us), we just blank the list. - const disabled = new Set( - config.get("disabledProviders", []) as ProviderId[], + next( + uri, + transformDiagnostics(uri, diagnostics, { + disabledProviders: config.get("disabledProviders", []), + severityThreshold: config.get("severityThreshold", "low"), + }), ); - const provider = providerForPath(uri.fsPath); - if (provider && disabled.has(provider)) { - next(uri, []); - return; - } - const threshold = config.get("severityThreshold", "low"); - next(uri, filterByThreshold(diagnostics, threshold)); }, }, }; @@ -304,41 +296,6 @@ async function stopClient(): Promise { } } -/** - * Race the LSP startup handshake against a hard ceiling. On timeout - * we call `client.stop()` to kill the stranded subprocess (best - * effort — the server may already be hung past saving) and throw a - * recognisable error the caller's catch surfaces in the failure - * toast. - */ -async function startWithTimeout( - c: LanguageClient, - timeoutMs: number, -): Promise { - let timer: NodeJS.Timeout | undefined; - const timeout = new Promise((_, reject) => { - timer = setTimeout(() => { - // Fire-and-forget the cleanup stop(); we don't await it because - // a hung interpreter probably won't respond, and the timeout - // toast wants to be on screen now, not in 30 seconds when stop - // gives up. - void c.stop().catch(() => undefined); - reject( - new Error( - `Language server did not finish startup within ${Math.round(timeoutMs / 1000)}s. Check the server log; common causes are an empty pipelineCheck.serverArgs, an interpreter that drops into the REPL, or a corrupted pipeline_check install.`, - ), - ); - }, timeoutMs); - }); - try { - await Promise.race([c.start(), timeout]); - } finally { - if (timer) { - clearTimeout(timer); - } - } -} - export async function activate( context: vscode.ExtensionContext, ): Promise { @@ -518,32 +475,18 @@ export async function activate( // `didSave`, so this is purely about picking up cross-file effects in // *other* CI files (a Jenkinsfile that includes the just-edited // library, a GHA workflow that calls the just-edited composite - // action). A simple in-flight guard prevents save-storms (autosave, - // Save All) from queueing redundant scans. - let scanOnSaveBusy = false; - context.subscriptions.push( - vscode.workspace.onDidSaveTextDocument(async (doc) => { - const config = vscode.workspace.getConfiguration("pipelineCheck"); - if (!config.get("scanOnSave", false)) { - return; - } - // Only react to saves of CI-relevant files. providerForPath - // returns `undefined` for anything that doesn't match our - // glob patterns, so package.json / random YAML never triggers. - if (!providerForPath(doc.uri.fsPath)) { - return; - } - if (scanOnSaveBusy) { - return; - } - scanOnSaveBusy = true; - try { - await scanWorkspace({ quiet: true }); - } finally { - scanOnSaveBusy = false; - } - }), - ); + // action). Busy-guard semantics + the gate logic live in + // src/scanOnSave.ts so they're unit-testable without a real save + // event source; this wiring just plumbs VS Code's dependencies in. + const onSave = createScanOnSaveHandler({ + isEnabled: () => + vscode.workspace + .getConfiguration("pipelineCheck") + .get("scanOnSave", false), + isPipelineFile: (fsPath) => providerForPath(fsPath) !== undefined, + scan: () => scanWorkspace({ quiet: true }), + }); + context.subscriptions.push(vscode.workspace.onDidSaveTextDocument(onSave)); // Fire-and-forget the one-time "what's new" toast for users who // just upgraded. Detached so a not-yet-dismissed notification never diff --git a/src/lspStart.test.ts b/src/lspStart.test.ts new file mode 100644 index 0000000..84bc813 --- /dev/null +++ b/src/lspStart.test.ts @@ -0,0 +1,198 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +import { + formatTimeoutMessage, + startWithTimeout, + type StartableClient, +} from "./lspStart"; + +// vitest fake timers let us watch the timeout race fire in millisecond +// time even though the real ceiling is 30 seconds — a real-time test +// would either be flaky (small ceiling, slow CI) or slow (real +// ceiling, multi-second per test). + +beforeEach(() => { + vi.useFakeTimers(); +}); + +afterEach(() => { + vi.useRealTimers(); +}); + +/** + * Build a StartableClient whose `start()` resolves only when the test + * explicitly calls `resolveStart()`. Captures every `stop()` invocation + * so the test can assert the timeout fires the kill switch. + */ +function makeFakeClient(): { + client: StartableClient; + resolveStart: () => void; + rejectStart: (err: unknown) => void; + stopCalls: number; +} { + let resolveStart!: () => void; + let rejectStart!: (err: unknown) => void; + const startPromise = new Promise((res, rej) => { + resolveStart = res; + rejectStart = rej; + }); + const calls = { stop: 0 }; + const client: StartableClient = { + start: () => startPromise, + stop: async () => { + calls.stop += 1; + }, + }; + return { + client, + resolveStart, + rejectStart, + get stopCalls() { + return calls.stop; + }, + } as unknown as ReturnType; +} + +/** + * Helper: chain a no-op `.catch` onto the test promise to suppress + * Node's "unhandled rejection" warning. Fake timers fire the rejection + * synchronously, well before any `await expect(p).rejects.toThrow(...)` + * gets a chance to register a handler. Pre-arming `.catch(() => {})` + * means a handler is on the promise before the timer rejects it, + * silencing the warning without changing the test's outcome — vitest + * `.rejects.toThrow(...)` still observes the rejection. + */ +function armRejectionHandler(p: Promise): Promise { + p.catch(() => undefined); + return p; +} + +describe("startWithTimeout", () => { + it("resolves when start() wins the race", async () => { + const { client, resolveStart } = makeFakeClient(); + const p = startWithTimeout(client, 30_000); + // Settle start() before the timer has any chance to tick. + resolveStart(); + await expect(p).resolves.toBeUndefined(); + }); + + it("rejects with the documented message when the timer wins", async () => { + const { client } = makeFakeClient(); + const p = armRejectionHandler(startWithTimeout(client, 30_000)); + // Don't resolve start. Push past the ceiling. + await vi.advanceTimersByTimeAsync(30_000); + await expect(p).rejects.toThrow(/did not finish startup within 30s/); + }); + + it("fires client.stop() exactly once on timeout", async () => { + // The whole point: an interpreter hung at start() needs to be + // killed so the next startClient() doesn't inherit a half-alive + // child. Failing to call stop() leaves a zombie subprocess. + const fake = makeFakeClient(); + const p = armRejectionHandler(startWithTimeout(fake.client, 30_000)); + await vi.advanceTimersByTimeAsync(30_000); + await expect(p).rejects.toThrow(); + expect(fake.stopCalls).toBe(1); + }); + + it("does NOT call stop() when start() resolves before the ceiling", async () => { + // The cleanup stop() is the timeout's escape hatch. If start + // succeeded, calling stop() afterwards would kill the LSP we + // just successfully launched. + const fake = makeFakeClient(); + const p = startWithTimeout(fake.client, 30_000); + fake.resolveStart(); + await expect(p).resolves.toBeUndefined(); + expect(fake.stopCalls).toBe(0); + }); + + it("clears the timer when start() rejects so the timer-side stop() never fires", async () => { + // If start() rejects on its own (e.g. spawn ENOENT) the timer + // should be cleared — otherwise we'd later trigger an + // unnecessary stop() against a client that already failed. + const fake = makeFakeClient(); + const p = armRejectionHandler(startWithTimeout(fake.client, 30_000)); + fake.rejectStart(new Error("spawn ENOENT")); + await expect(p).rejects.toThrow(/ENOENT/); + // Advance well past the ceiling — if the timer was still armed, + // the second stop() would have landed here. + await vi.advanceTimersByTimeAsync(60_000); + expect(fake.stopCalls).toBe(0); + }); + + it("swallows a rejection from the timeout-side stop() without surfacing it", async () => { + // A hung interpreter's stop() can reject ("server did not + // acknowledge"); we still want the user to see the + // "did not finish startup" error, not a confusing + // "stop failed" error. + let resolveStart!: () => void; + const startPromise = new Promise((res) => { + resolveStart = res; + }); + const client: StartableClient = { + start: () => startPromise, + stop: () => + Promise.reject(new Error("server did not acknowledge stop")), + }; + const p = armRejectionHandler(startWithTimeout(client, 30_000)); + await vi.advanceTimersByTimeAsync(30_000); + await expect(p).rejects.toThrow(/did not finish startup/); + // Settle the unused start promise so its handlers don't dangle. + resolveStart(); + }); + + it("rounds odd timeouts to whole seconds in the toast copy", async () => { + // 1234ms → 1s in the message, not 1.234s — the round-down keeps + // the toast readable without dropping precision the user cares + // about (they care about "did it hang", not the exact number). + const { client } = makeFakeClient(); + const p = armRejectionHandler(startWithTimeout(client, 1_234)); + await vi.advanceTimersByTimeAsync(1_234); + await expect(p).rejects.toThrow(/within 1s/); + }); + + it("ignores a late start() resolution after the timer has won", async () => { + // The settled-flag guard is what prevents this. Without it, a + // tardy start() resolution after the timeout would re-resolve + // the deferred, but Promise's once-settled invariant means the + // re-resolution is a no-op; the visible bug would be in + // intermediate state (stopCalls fired twice, or timer re-armed). + const fake = makeFakeClient(); + const p = armRejectionHandler(startWithTimeout(fake.client, 30_000)); + await vi.advanceTimersByTimeAsync(30_000); + await expect(p).rejects.toThrow(); + // Now start() finally settles. Nothing should change. + fake.resolveStart(); + // stop() count stays at 1 (the timeout's invocation), not 0. + expect(fake.stopCalls).toBe(1); + }); +}); + +describe("formatTimeoutMessage", () => { + // Exporting the message builder pins the user-facing copy as part + // of the module's API contract. Future maintainers who reword the + // toast will trip the test and have to update it deliberately. + + it("mentions the timeout in seconds", () => { + expect(formatTimeoutMessage(30_000)).toContain("within 30s"); + expect(formatTimeoutMessage(5_000)).toContain("within 5s"); + }); + + it("lists the three most likely root causes", () => { + // Diagnosis breadcrumbs save the maintainer a round trip when a + // user pastes the toast into an issue. Don't lose them. + const msg = formatTimeoutMessage(30_000); + expect(msg).toContain("serverArgs"); + expect(msg).toContain("REPL"); + expect(msg).toContain("pipeline_check install"); + }); + + it("points the user at the server log", () => { + expect(formatTimeoutMessage(30_000)).toContain("server log"); + }); + + it("rounds to whole seconds", () => { + expect(formatTimeoutMessage(1_499)).toContain("within 1s"); + expect(formatTimeoutMessage(1_500)).toContain("within 2s"); + }); +}); diff --git a/src/lspStart.ts b/src/lspStart.ts new file mode 100644 index 0000000..949aef4 --- /dev/null +++ b/src/lspStart.ts @@ -0,0 +1,81 @@ +// Bounded-time LSP startup, extracted from extension.ts so the timeout +// race semantics can be exercised in isolation. The shape it accepts +// is structural rather than nominally `LanguageClient` from +// vscode-languageclient — extension.ts passes the real LanguageClient, +// the tests pass a fake. Anything with `start(): Promise` and +// `stop(): Promise` works. + +/** + * Minimal shape `startWithTimeout` needs from a LanguageClient. We do + * NOT depend on the concrete `LanguageClient` type here so the unit + * tests don't have to import vscode-languageclient (which pulls in the + * VS Code runtime). + */ +export interface StartableClient { + start(): Promise; + stop(): Promise; +} + +/** + * Race the LSP startup handshake against a hard ceiling. On timeout + * we fire-and-forget `client.stop()` to kill the stranded subprocess + * (best effort — the server may already be hung past saving) and + * throw a recognisable error the caller's catch surfaces in the + * failure toast. + * + * The fire-and-forget shape on the timeout-side stop() is deliberate: + * awaiting it would mean the user waits for a hung interpreter to + * respond before seeing the "did not finish startup" toast, which + * defeats the point of the timeout. + * + * Internally the implementation is a single deferred with a `settled` + * flag rather than `Promise.race([start, timeoutPromise])`. The race + * shape leaks a "rejection was handled asynchronously" warning in + * Node when fake timers fire the rejection synchronously — Promise.race + * attaches its handler one microtask later, which Node briefly treats + * as unhandled. The deferred avoids that two-promise dance. + */ +export function startWithTimeout( + client: StartableClient, + timeoutMs: number, +): Promise { + return new Promise((resolve, reject) => { + let settled = false; + const timer = setTimeout(() => { + if (settled) return; + settled = true; + // Best-effort kill of the stranded subprocess; swallow any + // stop() rejection so the timeout error is what the caller sees. + void client.stop().catch(() => undefined); + reject(new Error(formatTimeoutMessage(timeoutMs))); + }, timeoutMs); + client.start().then( + () => { + if (settled) return; + settled = true; + clearTimeout(timer); + resolve(); + }, + (err) => { + if (settled) return; + settled = true; + clearTimeout(timer); + reject(err); + }, + ); + }); +} + +/** + * Exported for unit testing the timeout message contract — the toast + * copy is part of the user-facing API and should not drift silently. + */ +export function formatTimeoutMessage(timeoutMs: number): string { + const seconds = Math.round(timeoutMs / 1000); + return ( + `Language server did not finish startup within ${seconds}s. ` + + `Check the server log; common causes are an empty ` + + `pipelineCheck.serverArgs, an interpreter that drops into the ` + + `REPL, or a corrupted pipeline_check install.` + ); +} diff --git a/src/middleware.test.ts b/src/middleware.test.ts new file mode 100644 index 0000000..c7f69bf --- /dev/null +++ b/src/middleware.test.ts @@ -0,0 +1,237 @@ +import { describe, it, expect, vi } from "vitest"; + +vi.mock("vscode", () => ({})); + +import { transformDiagnostics, type DiagnosticConfig } from "./middleware"; + +// Minimal Diagnostic-shaped objects. The real vscode.Diagnostic +// carries much more, but transformDiagnostics + filterByThreshold +// only read `data.severity` and the array shape — anything else is +// passed through untouched. +function diag(severity: string | undefined, message = "x"): unknown { + return severity === undefined + ? { message, data: {} } + : { message, data: { severity } }; +} + +// Mirror the URI shape providerForPath consumes: `.fsPath` is the +// only field it reads. +function uri(path: string) { + return { fsPath: path } as unknown as Parameters[0]; +} + +const NO_FILTERS: DiagnosticConfig = { + disabledProviders: [], + severityThreshold: "low", +}; + +describe("transformDiagnostics", () => { + // The two stages compose: disabled-provider wins over the severity + // filter (blanket drop) when both apply; severity filter applies to + // the survivors otherwise. Tests pin both individual stages AND the + // composition — neither piece is interesting on its own but their + // ordering changes the user-visible behaviour. + + it("passes diagnostics through unchanged when neither filter applies", () => { + const ds = [diag("HIGH"), diag("MEDIUM"), diag("LOW")]; + expect( + transformDiagnostics( + uri("/repo/.gitlab-ci.yml"), + ds as never, + NO_FILTERS, + ), + ).toEqual(ds); + }); + + describe("disabledProviders stage", () => { + it("drops every diagnostic for a URI whose provider is disabled", () => { + // gitlab disabled → blanket empty. + expect( + transformDiagnostics( + uri("/repo/.gitlab-ci.yml"), + [diag("CRITICAL"), diag("HIGH")] as never, + { disabledProviders: ["gitlab"], severityThreshold: "low" }, + ), + ).toEqual([]); + }); + + it("returns an EMPTY ARRAY (not a passthrough skip) so the publish still propagates", () => { + // The middleware caller relies on `next(uri, [])` to wake up + // consumers like the Findings tree — a same-URI publish with + // zero diagnostics still triggers a refresh, which clears any + // stale leaves from a now-disabled file. + const result = transformDiagnostics( + uri("/repo/.gitlab-ci.yml"), + [diag("CRITICAL")] as never, + { disabledProviders: ["gitlab"], severityThreshold: "low" }, + ); + expect(Array.isArray(result)).toBe(true); + expect(result).toHaveLength(0); + }); + + it("leaves OTHER providers' diagnostics alone when only one is disabled", () => { + // Dockerfile disabled but the URI is a GHA workflow — should + // pass through unchanged. + const ds = [diag("HIGH")]; + expect( + transformDiagnostics( + uri("/repo/.github/workflows/ci.yml"), + ds as never, + { disabledProviders: ["dockerfile"], severityThreshold: "low" }, + ), + ).toEqual(ds); + }); + + it("matches lowercase Dockerfile / Jenkinsfile (providerForPath is case-insensitive)", () => { + // Regression fence for the high-severity fix: a Windows user + // with `dockerfile` (lowercase) on disk would otherwise slip + // past the disable filter. + expect( + transformDiagnostics( + uri("/repo/dockerfile"), + [diag("CRITICAL")] as never, + { disabledProviders: ["dockerfile"], severityThreshold: "low" }, + ), + ).toEqual([]); + expect( + transformDiagnostics( + uri("/repo/JENKINSFILE"), + [diag("CRITICAL")] as never, + { disabledProviders: ["jenkins"], severityThreshold: "low" }, + ), + ).toEqual([]); + }); + + it("ignores unknown provider IDs in the config without affecting other rows", () => { + // VS Code's JSON schema enforces the enum, but a manual edit + // can sneak through. An unknown ID must not silently drop + // unrelated diagnostics. + const ds = [diag("HIGH")]; + expect( + transformDiagnostics( + uri("/repo/.gitlab-ci.yml"), + ds as never, + { disabledProviders: ["nope"], severityThreshold: "low" }, + ), + ).toEqual(ds); + }); + + it("disables both dockerfile and containerfile under the single 'dockerfile' provider id", () => { + // Containerfile and Dockerfile share a provider entry; the + // disable knob silences both with one setting. + const result1 = transformDiagnostics( + uri("/repo/Containerfile"), + [diag("CRITICAL")] as never, + { disabledProviders: ["dockerfile"], severityThreshold: "low" }, + ); + expect(result1).toEqual([]); + }); + }); + + describe("severityThreshold stage", () => { + it("drops diagnostics strictly below the threshold", () => { + const result = transformDiagnostics( + uri("/repo/.github/workflows/ci.yml"), + [ + diag("CRITICAL"), + diag("HIGH"), + diag("MEDIUM"), + diag("LOW"), + ] as never, + { disabledProviders: [], severityThreshold: "high" }, + ); + expect( + result.map( + (d) => + (d as unknown as { data: { severity: string } }).data.severity, + ), + ).toEqual(["CRITICAL", "HIGH"]); + }); + + it("preserves order across the filter (no in-place mutation surfaces)", () => { + const ds = [ + diag("HIGH", "first"), + diag("LOW", "skipped"), + diag("CRITICAL", "third"), + ]; + const result = transformDiagnostics( + uri("/repo/.github/workflows/ci.yml"), + ds as never, + { disabledProviders: [], severityThreshold: "medium" }, + ); + expect(result.map((d) => (d as { message: string }).message)).toEqual([ + "first", + "third", + ]); + // Source list is untouched. + expect(ds).toHaveLength(3); + }); + + it("never drops a diagnostic whose data.severity is missing", () => { + // The first-line-of-defence invariant: missing metadata = pass. + // Otherwise a server upgrade that briefly publishes without + // `data` would blank the editor. + const result = transformDiagnostics( + uri("/repo/.github/workflows/ci.yml"), + [diag(undefined)] as never, + { disabledProviders: [], severityThreshold: "critical" }, + ); + expect(result).toHaveLength(1); + }); + }); + + describe("filter composition (the user-visible behaviour)", () => { + it("when a provider is disabled, the severity threshold is irrelevant — blanket drop", () => { + // The disabled-provider stage runs first; if it bites, the + // severity threshold is never consulted. A CRITICAL finding on + // a disabled provider still vanishes from the editor. + const result = transformDiagnostics( + uri("/repo/.gitlab-ci.yml"), + [diag("CRITICAL")] as never, + { disabledProviders: ["gitlab"], severityThreshold: "low" }, + ); + expect(result).toEqual([]); + }); + + it("when the provider is allowed, only the severity stage filters", () => { + const result = transformDiagnostics( + uri("/repo/.github/workflows/ci.yml"), + [diag("CRITICAL"), diag("LOW")] as never, + { disabledProviders: ["gitlab"], severityThreshold: "high" }, + ); + expect(result).toHaveLength(1); + expect( + (result[0] as unknown as { data: { severity: string } }).data + .severity, + ).toBe("CRITICAL"); + }); + + it("URI that maps to NO provider always bypasses the disable stage", () => { + // Random YAML that we wouldn't have published in the first + // place; the LSP-side filter should have caught it. Defensive + // here: providerForPath returns undefined → the disable filter + // is a no-op even when the config lists every provider. + const ds = [diag("CRITICAL")]; + const result = transformDiagnostics( + uri("/repo/random.yml"), + ds as never, + { + disabledProviders: [ + "github-actions", + "gitlab", + "azure", + "bitbucket", + "circleci", + "cloud-build", + "buildkite", + "drone", + "jenkins", + "dockerfile", + ], + severityThreshold: "low", + }, + ); + expect(result).toEqual(ds); + }); + }); +}); diff --git a/src/middleware.ts b/src/middleware.ts new file mode 100644 index 0000000..c885680 --- /dev/null +++ b/src/middleware.ts @@ -0,0 +1,50 @@ +// Diagnostic-publish transform that the LanguageClient middleware +// chains. Extracted out of extension.ts so the composition of the +// per-provider disable filter and the per-severity threshold filter +// can be unit-tested without booting an LSP — neither piece is +// individually interesting (each has its own test file), but their +// composition is what the user actually sees, and that was untested. +// +// Two filters in order: +// 1. If the URI maps to a `disabledProviders[*]` entry, drop ALL +// diagnostics for that URI. Returns an empty array so the +// middleware still calls next(uri, []) and the publish wakes +// consumers up (so a later "unset disable" produces a refresh). +// 2. Otherwise, drop every diagnostic below +// `pipelineCheck.severityThreshold`. + +import type * as vscode from "vscode"; + +import { providerForPath, type ProviderId } from "./providers"; +import { filterByThreshold } from "./severityFilter"; + +/** + * Anything the middleware needs from the live VS Code configuration. + * Passing this as a value (rather than reading `vscode.workspace` + * here) keeps the function pure and testable. + */ +export interface DiagnosticConfig { + readonly disabledProviders: readonly string[]; + readonly severityThreshold: string; +} + +/** + * Filter a freshly-published diagnostic batch through the two-stage + * settings filter. Pure: identical inputs produce identical outputs; + * no side effects. + */ +export function transformDiagnostics( + uri: vscode.Uri, + diagnostics: readonly vscode.Diagnostic[], + config: DiagnosticConfig, +): vscode.Diagnostic[] { + const disabled = new Set(config.disabledProviders as ProviderId[]); + const provider = providerForPath(uri.fsPath); + if (provider && disabled.has(provider)) { + // Empty array (not "skip the call") so the publish still + // propagates — consumers like the Findings tree rely on the + // batch arrival to re-render even when the count is zero. + return []; + } + return filterByThreshold(diagnostics, config.severityThreshold); +} diff --git a/src/scanOnSave.test.ts b/src/scanOnSave.test.ts new file mode 100644 index 0000000..a4b5df4 --- /dev/null +++ b/src/scanOnSave.test.ts @@ -0,0 +1,197 @@ +import { describe, it, expect, vi } from "vitest"; + +vi.mock("vscode", () => ({})); + +import { + createScanOnSaveHandler, + type SavedDocument, + type ScanOnSaveDeps, +} from "./scanOnSave"; + +function doc(fsPath: string): SavedDocument { + return { uri: { fsPath } }; +} + +/** + * Build a deps object with sensible defaults that the tests can + * override field-by-field. `scan` returns a promise the test can + * resolve/reject explicitly, which is how the busy-guard tests + * inspect the in-flight window. + */ +function makeDeps( + overrides: Partial & { + scanPromise?: Promise; + } = {}, +): { + deps: ScanOnSaveDeps; + scanCalls: number; + scanResolvers: Array<() => void>; +} { + const scanResolvers: Array<() => void> = []; + const calls = { scan: 0 }; + const deps: ScanOnSaveDeps = { + isEnabled: overrides.isEnabled ?? (() => true), + isPipelineFile: overrides.isPipelineFile ?? (() => true), + scan: + overrides.scan ?? + (() => { + calls.scan += 1; + if (overrides.scanPromise) return overrides.scanPromise; + return new Promise((res) => { + scanResolvers.push(res); + }); + }), + }; + return { + deps, + get scanCalls() { + return calls.scan; + }, + scanResolvers, + } as unknown as ReturnType; +} + +describe("createScanOnSaveHandler", () => { + // The handler closes over a single boolean so the busy semantics + // and the gate checks can be locked down without a real save + // stream. The cheap-gates-first ordering (isEnabled before + // isPipelineFile before busy) is the contract — it means a + // non-CI save in a workspace with scanOnSave off pays only one + // config read. + + it("returns immediately when scanOnSave is disabled", async () => { + const fake = makeDeps({ isEnabled: () => false }); + const handler = createScanOnSaveHandler(fake.deps); + await handler(doc("/repo/.gitlab-ci.yml")); + expect(fake.scanCalls).toBe(0); + }); + + it("returns immediately for non-CI files even when enabled", async () => { + const fake = makeDeps({ isPipelineFile: () => false }); + const handler = createScanOnSaveHandler(fake.deps); + await handler(doc("/repo/package.json")); + expect(fake.scanCalls).toBe(0); + }); + + it("scans when both gates pass", async () => { + const fake = makeDeps(); + const handler = createScanOnSaveHandler(fake.deps); + // Start the save handler but don't await yet — the scan promise + // stays unresolved so we can inspect the in-flight state. + const inFlight = handler(doc("/repo/.gitlab-ci.yml")); + expect(fake.scanCalls).toBe(1); + fake.scanResolvers[0](); + await inFlight; + }); + + it("collapses a save-storm to a single scan via the busy guard", async () => { + // Save All / autosave can drop ten saves in flight at once. The + // guard turns this into one scan; tail saves arriving while the + // scan runs are dropped. + const fake = makeDeps(); + const handler = createScanOnSaveHandler(fake.deps); + const first = handler(doc("/repo/.gitlab-ci.yml")); + void handler(doc("/repo/.github/workflows/ci.yml")); + void handler(doc("/repo/Jenkinsfile")); + expect(fake.scanCalls).toBe(1); + fake.scanResolvers[0](); + await first; + }); + + it("releases the busy lock so a later save still scans", async () => { + // The busy guard is per-call — once a scan finishes, the next + // save proceeds normally. Tail saves at the very end of a storm + // were the original motivation for this fix. + const fake = makeDeps(); + const handler = createScanOnSaveHandler(fake.deps); + const first = handler(doc("/repo/.gitlab-ci.yml")); + fake.scanResolvers[0](); + await first; + expect(fake.scanCalls).toBe(1); + const second = handler(doc("/repo/.gitlab-ci.yml")); + expect(fake.scanCalls).toBe(2); + fake.scanResolvers[1](); + await second; + }); + + it("releases the busy lock even when the scan rejects", async () => { + // A rejected scan must NOT leave the lock stuck on — otherwise + // a single transient failure would silence scan-on-save for the + // rest of the session. + const deps: ScanOnSaveDeps = { + isEnabled: () => true, + isPipelineFile: () => true, + scan: () => Promise.reject(new Error("transient")), + }; + const handler = createScanOnSaveHandler(deps); + await expect(handler(doc("/repo/.gitlab-ci.yml"))).rejects.toThrow( + "transient", + ); + // The next save must still get a fresh scan. + let secondScanFired = false; + const handler2 = createScanOnSaveHandler({ + isEnabled: () => true, + isPipelineFile: () => true, + scan: () => { + secondScanFired = true; + return Promise.resolve(); + }, + }); + await handler2(doc("/repo/.gitlab-ci.yml")); + expect(secondScanFired).toBe(true); + }); + + it("re-evaluates isEnabled on every save (not cached)", async () => { + // A user toggles the setting from off to on mid-session; the + // very next save should pick it up without an extension reload. + let enabled = false; + const fake = makeDeps({ + isEnabled: () => enabled, + }); + const handler = createScanOnSaveHandler(fake.deps); + await handler(doc("/repo/.gitlab-ci.yml")); + expect(fake.scanCalls).toBe(0); + enabled = true; + const inFlight = handler(doc("/repo/.gitlab-ci.yml")); + expect(fake.scanCalls).toBe(1); + fake.scanResolvers[0](); + await inFlight; + }); + + it("checks isEnabled BEFORE isPipelineFile (cheap gate first)", async () => { + // When scanOnSave is off, we shouldn't even bother classifying + // the saved path. Locks down the cheap-gates-first ordering so + // a future refactor doesn't accidentally invert it and add a + // providerForPath call on every save in a workspace that has + // scan-on-save disabled. + let pipelineFileCalls = 0; + const deps: ScanOnSaveDeps = { + isEnabled: () => false, + isPipelineFile: () => { + pipelineFileCalls += 1; + return true; + }, + scan: () => Promise.resolve(), + }; + const handler = createScanOnSaveHandler(deps); + await handler(doc("/repo/.gitlab-ci.yml")); + expect(pipelineFileCalls).toBe(0); + }); + + it("each handler instance has its own busy flag (no cross-instance lock)", async () => { + // Two extension hosts (two windows on the same workspace) get + // their own handler instance. A save in one mustn't lock the + // other. + const first = makeDeps(); + const second = makeDeps(); + const h1 = createScanOnSaveHandler(first.deps); + const h2 = createScanOnSaveHandler(second.deps); + const p1 = h1(doc("/repo/.gitlab-ci.yml")); + const p2 = h2(doc("/repo/.gitlab-ci.yml")); + expect(first.scanCalls).toBe(1); + expect(second.scanCalls).toBe(1); + first.scanResolvers[0](); + second.scanResolvers[0](); + await Promise.all([p1, p2]); + }); +}); diff --git a/src/scanOnSave.ts b/src/scanOnSave.ts new file mode 100644 index 0000000..23ceb7e --- /dev/null +++ b/src/scanOnSave.ts @@ -0,0 +1,61 @@ +// Scan-on-save handler factory. Extracted from extension.ts's +// activate() closure so the busy-guard semantics, the config gate, +// and the provider gate can be exercised without a real +// onDidSaveTextDocument event source. +// +// Pure injection: the factory takes everything it needs as deps. The +// real wiring in extension.ts plugs in vscode.workspace.getConfiguration +// for the enable flag, providerForPath for the file-match check, and +// scanWorkspace for the scanner. Tests swap each for synthetic +// versions that count calls and let the test drive timing. + +/** Minimal text-document shape the handler reads. */ +export interface SavedDocument { + readonly uri: { readonly fsPath: string }; +} + +/** + * Anything the scan-on-save handler needs from the outside world. + * Keeping every dependency on this interface lets the test instantiate + * the handler without importing vscode, providers.ts, or + * workspaceScan.ts. + */ +export interface ScanOnSaveDeps { + /** True when `pipelineCheck.scanOnSave` is enabled. Re-read on each save. */ + readonly isEnabled: () => boolean; + /** True when the saved file matches a Pipeline-Check provider glob. */ + readonly isPipelineFile: (fsPath: string) => boolean; + /** Scan the workspace. Receives no arguments — the handler always quiets. */ + readonly scan: () => Promise; +} + +/** + * Build a save handler closing over a single `busy` flag. Returning a + * closure (rather than a class) gives every call site its own flag — + * two extension hosts in two windows can't accidentally share the + * lock. + * + * The busy guard collapses save-storms (autosave, Save All) to one + * scan. A storm-tail save that arrives after the scan finishes still + * starts a fresh scan, which is the right semantics for cross-file + * effects: we don't want to skip the LAST save of a storm. + */ +export function createScanOnSaveHandler( + deps: ScanOnSaveDeps, +): (doc: SavedDocument) => Promise { + let busy = false; + return async (doc) => { + // Order matters: the cheapest gates run first so a non-CI save + // (the common case) costs only one config read and one regex + // test, not a full scan setup. + if (!deps.isEnabled()) return; + if (!deps.isPipelineFile(doc.uri.fsPath)) return; + if (busy) return; + busy = true; + try { + await deps.scan(); + } finally { + busy = false; + } + }; +}