Skip to content

Commit f6244d2

Browse files
dmartinochoaclaude
andcommitted
fix: harden 4 medium-severity edge cases + 13 new tests (245 total)
1. scanWorkspace single-flight guard. The user-initiated 'Scan workspace' and 'Refresh Findings' commands both call scanWorkspace; a double-click of one (or scan + refresh in quick succession) used to spawn two concurrent progress notifications iterating the same URI list. A module-level in-flight flag now collapses every concurrent call to one scan; the second caller returns skippedAsBusy: true and (in noisy mode) surfaces a friendly 'already in progress' info toast. 2. installInTerminal reuses any live 'Pipeline-Check install' terminal instead of stacking identical ones in the dropdown. Exited terminals (exitStatus set) are treated as dead and a fresh one takes their place. 3. scan-on-save short-circuits when the saved file's provider is in `pipelineCheck.disabledProviders`. The scanOnSave dep `isPipelineFile` was renamed to `shouldScanOnSave` and now bundles the path-classifier with the disabled-provider check. Saving a Dockerfile with `dockerfile` disabled used to trigger a full workspace re-open whose findings the middleware would just drop. 4. whatsNew rc → ga transition. Per semver §11 a pre-release version is LOWER precedence than the corresponding release. The previous compare stripped the suffix and treated `1.0.0-rc.1` and `1.0.0` as equal, so pre-release testers missed the GA toast. Now compares pre-release suffixes lexicographically when core triples match. Tests: +13 (232 → 245). - workspaceScan: +5 busy-guard tests (skippedAsBusy result shape, noisy-toast surfaces it, quiet-mode silences it, finally releases on throw, preconditions don't lock the flag) - install: +4 terminal-reuse tests (second call reuses, show + sendText still fire, exited terminal triggers fresh, foreign-name terminals ignored) - whatsNew: replaced one stale "strip suffix" test with 4 semver-correct tests (rc → ga is upgrade, ga → rc is downgrade, rc → rc lexicographic, higher core triple wins) - scanOnSave: replaced the dep-name test with one for live disabled re-evaluation Stub additions: __stubLiveTerminals (terminal roster for the reuse lookup) and __stubOpenTextDocumentGate (controllable pause inside openTextDocument so the busy-guard test can interleave a second scan call mid-loop). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 6014c69 commit f6244d2

11 files changed

Lines changed: 499 additions & 90 deletions

CHANGELOG.md

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,31 @@ versions follow [SemVer](https://semver.org/).
1313
1414
## [Unreleased]
1515

16+
### Fixed (round 2: medium-severity batch)
17+
18+
- **Two scan-workspace runs in parallel no longer spawn two
19+
notifications.** A module-level in-flight guard collapses every
20+
concurrent `scanWorkspace` caller to one scan. A second call (from
21+
a double-clicked button, or scan + refresh) returns immediately
22+
with `skippedAsBusy: true` and, in noisy mode, surfaces a
23+
"scan already in progress" info toast.
24+
- **`installInTerminal` reuses the existing "Pipeline-Check install"
25+
terminal.** Repeated clicks on the welcome-panel CTA used to stack
26+
identical terminals in the dropdown. Now the second click reuses
27+
the live terminal; an exited terminal is treated as dead and a
28+
fresh one takes its place.
29+
- **`scan-on-save` short-circuits when the saved file's provider is
30+
disabled.** Saving a Dockerfile in a workspace that has
31+
`dockerfile` in `pipelineCheck.disabledProviders` used to trigger
32+
a full workspace re-open even though every published diagnostic
33+
would be dropped by the middleware. The handler now consults the
34+
live `disabledProviders` setting and skips the scan.
35+
- **`whatsNew` rc → ga transition is no longer silently swallowed.**
36+
Per semver §11 a pre-release version is LOWER precedence than the
37+
corresponding release. The previous version-compare stripped the
38+
suffix and treated `1.0.0-rc.1` and `1.0.0` as equal, so
39+
pre-release testers never saw the "What's New" toast for the GA.
40+
1641
### Fixed
1742

1843
- **Welcome panel stops lying after an LSP crash.** Subscribe to

src/__testStubs__/vscode.ts

Lines changed: 56 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,26 @@ declare global {
9090
// URI strings that `openTextDocument` should reject for (simulating
9191
// a read error / unsupported encoding). All other URIs resolve.
9292
var __stubOpenTextDocumentFailures: Set<string> | undefined;
93+
// Optional gate the openTextDocument stub awaits before resolving.
94+
// Tests that want to inspect mid-scan state (e.g. the in-flight
95+
// busy-guard test) set this to a Promise they control, kick off
96+
// scanWorkspace, then interrogate state while the open is paused.
97+
var __stubOpenTextDocumentGate: Promise<unknown> | undefined;
9398
// When true, `withProgress` reports cancellation back to the task
9499
// immediately. Used by the scanWorkspace cancellation test.
95100
var __stubProgressCancelled: boolean | undefined;
101+
// Live terminals roster surfaced via `vscode.window.terminals`. The
102+
// stub auto-pushes any createTerminal result here; tests can also
103+
// pre-populate to simulate a pre-existing (or exited) terminal.
104+
var __stubLiveTerminals:
105+
| Array<{
106+
name: string;
107+
exitStatus: { code: number | undefined } | undefined;
108+
show: () => void;
109+
sendText: (text: string, addNewLine?: boolean) => void;
110+
dispose: () => void;
111+
}>
112+
| undefined;
96113
var __stubCalls: StubCalls | undefined;
97114
}
98115

@@ -126,7 +143,9 @@ export function resetStubState(): void {
126143
globalThis.__stubFindFilesByPattern = undefined;
127144
globalThis.__stubWorkspaceFolders = undefined;
128145
globalThis.__stubOpenTextDocumentFailures = undefined;
146+
globalThis.__stubOpenTextDocumentGate = undefined;
129147
globalThis.__stubProgressCancelled = undefined;
148+
globalThis.__stubLiveTerminals = undefined;
130149
globalThis.__stubCalls = {
131150
findFiles: [],
132151
terminals: [],
@@ -277,13 +296,20 @@ export function vscodeStub(): Record<string, unknown> {
277296
// Resolves with a minimal TextDocument-shaped object for any
278297
// URI not listed in `__stubOpenTextDocumentFailures`, where it
279298
// rejects instead. scanWorkspace counts those rejections as
280-
// `failed` without aborting the rest of the scan.
281-
openTextDocument: (uri: { toString: () => string }) => {
299+
// `failed` without aborting the rest of the scan. When
300+
// `__stubOpenTextDocumentGate` is set, the resolution awaits
301+
// it first — that lets tests interleave a second scan call
302+
// mid-loop to exercise the in-flight busy guard.
303+
openTextDocument: async (uri: { toString: () => string }) => {
282304
const key = uri.toString();
283305
if (globalThis.__stubOpenTextDocumentFailures?.has(key)) {
284-
return Promise.reject(new Error(`stub: open failed for ${key}`));
306+
throw new Error(`stub: open failed for ${key}`);
307+
}
308+
const gate = globalThis.__stubOpenTextDocumentGate;
309+
if (gate) {
310+
await gate;
285311
}
286-
return Promise.resolve({ uri });
312+
return { uri };
287313
},
288314
},
289315
languages: {
@@ -319,9 +345,18 @@ export function vscodeStub(): Record<string, unknown> {
319345
openExternal: () => Promise.resolve(true),
320346
},
321347
window: {
348+
// Live terminals roster used by code that walks
349+
// `vscode.window.terminals` (e.g. installInTerminal's reuse
350+
// check). Tests can pre-populate this slot to simulate a
351+
// pre-existing terminal; createTerminal pushes into it.
352+
get terminals() {
353+
return globalThis.__stubLiveTerminals ?? [];
354+
},
322355
// Terminal factory captures the name and returns a stub whose
323-
// show/sendText calls land on the shared slot. Each call returns
324-
// a fresh terminal with its own observation buffer.
356+
// show/sendText calls land on the shared slot. The returned
357+
// terminal is ALSO pushed onto the live `terminals` roster so
358+
// an installInTerminal-style reuse lookup finds it. Each call
359+
// returns a fresh terminal with its own observation buffer.
325360
createTerminal: (name: string) => {
326361
const sent: Array<{ text: string; addNewLine: boolean }> = [];
327362
const record = {
@@ -330,18 +365,30 @@ export function vscodeStub(): Record<string, unknown> {
330365
sent,
331366
};
332367
ensureCalls().terminals.push(record);
333-
return {
368+
const terminal: {
369+
name: string;
370+
exitStatus: { code: number | undefined } | undefined;
371+
show: () => void;
372+
sendText: (text: string, addNewLine?: boolean) => void;
373+
dispose: () => void;
374+
} = {
334375
name,
376+
// Undefined => alive. Tests can mutate this slot directly
377+
// (or pre-populate __stubLiveTerminals with a closed one)
378+
// to exercise the exited-terminal path.
379+
exitStatus: undefined,
335380
show: () => {
336-
// Mutate the captured record so tests see `shown: true`
337-
// without having to drill into a closure.
338381
(record as { shown: boolean }).shown = true;
339382
},
340383
sendText: (text: string, addNewLine?: boolean) => {
341384
sent.push({ text, addNewLine: addNewLine ?? true });
342385
},
343386
dispose: () => undefined,
344387
};
388+
const roster = globalThis.__stubLiveTerminals ?? [];
389+
roster.push(terminal);
390+
globalThis.__stubLiveTerminals = roster;
391+
return terminal;
345392
},
346393
createStatusBarItem: (
347394
_alignment?: number,

src/extension.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -483,7 +483,20 @@ export async function activate(
483483
vscode.workspace
484484
.getConfiguration("pipelineCheck")
485485
.get<boolean>("scanOnSave", false),
486-
isPipelineFile: (fsPath) => providerForPath(fsPath) !== undefined,
486+
// Trigger a scan only when the saved file is (a) something
487+
// Pipeline-Check actually scans and (b) belongs to a provider
488+
// the user has NOT silenced. Saving a Dockerfile in a workspace
489+
// that has `dockerfile` in `disabledProviders` should be a
490+
// no-op: re-scanning would just produce a publish the
491+
// middleware drops on arrival.
492+
shouldScanOnSave: (fsPath) => {
493+
const provider = providerForPath(fsPath);
494+
if (!provider) return false;
495+
const disabled = vscode.workspace
496+
.getConfiguration("pipelineCheck")
497+
.get<string[]>("disabledProviders", []);
498+
return !disabled.includes(provider);
499+
},
487500
scan: () => scanWorkspace({ quiet: true }),
488501
});
489502
context.subscriptions.push(vscode.workspace.onDidSaveTextDocument(onSave));

src/install.test.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,69 @@ describe("installInTerminal", () => {
6262
installInTerminal();
6363
expect(globalThis.__stubCalls?.clipboardWrites).toEqual([]);
6464
});
65+
66+
it("reuses the existing 'Pipeline-Check install' terminal on a second call", () => {
67+
// A user who clicks "Install in terminal" twice — once from the
68+
// welcome panel, then again from the LSP-failure toast — used to
69+
// get two identical terminals stacked in the dropdown. Now the
70+
// second call surfaces the same terminal. The createTerminal
71+
// call count is the canonical assertion.
72+
const first = installInTerminal();
73+
const second = installInTerminal();
74+
expect(second).toBe(first);
75+
expect(globalThis.__stubCalls?.terminals).toHaveLength(1);
76+
});
77+
78+
it("the reused terminal still gets show() + sendText() so it's visible and primed", () => {
79+
installInTerminal();
80+
installInTerminal();
81+
const t = globalThis.__stubCalls!.terminals[0];
82+
expect(t.shown).toBe(true);
83+
// sendText fired twice (once per call); the same command text.
84+
expect(t.sent).toHaveLength(2);
85+
expect(t.sent[0].text).toBe(PIP_INSTALL_COMMAND);
86+
expect(t.sent[1].text).toBe(PIP_INSTALL_COMMAND);
87+
});
88+
89+
it("treats an exited terminal as dead and creates a fresh one", () => {
90+
// A user closed the prior install terminal after running pip,
91+
// then triggered the install again. Reusing the dead terminal
92+
// would silently fail (sendText is a no-op on an exited
93+
// pty); a fresh one is the right answer.
94+
const first = installInTerminal();
95+
// Simulate the user closing the terminal: mark the live entry
96+
// as exited.
97+
const live = globalThis.__stubLiveTerminals?.[0];
98+
if (live) live.exitStatus = { code: 0 };
99+
const second = installInTerminal();
100+
expect(second).not.toBe(first);
101+
expect(globalThis.__stubCalls?.terminals).toHaveLength(2);
102+
});
103+
104+
it("ignores other terminals whose name differs", () => {
105+
// A user's `bash` / `python REPL` terminal must not be hijacked
106+
// by the install command. Only terminals named exactly
107+
// 'Pipeline-Check install' qualify for reuse.
108+
// Pre-populate the live roster with a same-named-but-foreign
109+
// terminal... actually the simpler check: a foreign terminal
110+
// with a different name must not be reused.
111+
globalThis.__stubLiveTerminals = [
112+
{
113+
name: "bash",
114+
exitStatus: undefined,
115+
show: () => undefined,
116+
sendText: () => undefined,
117+
dispose: () => undefined,
118+
},
119+
];
120+
installInTerminal();
121+
// createTerminal was called (because the bash terminal didn't
122+
// match by name), and the resulting terminal is the one we just
123+
// sent the command to.
124+
const terminals = globalThis.__stubCalls?.terminals ?? [];
125+
expect(terminals).toHaveLength(1);
126+
expect(terminals[0].name).toBe("Pipeline-Check install");
127+
});
65128
});
66129

67130
describe("copyInstallCommandToClipboard", () => {

src/install.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,19 +10,28 @@ const TERMINAL_NAME = "Pipeline-Check install";
1010
const CONFIRM_TTL_MS = 2500;
1111

1212
/**
13-
* Open a new integrated terminal, type the pip install command, and
13+
* Open the integrated terminal, type the pip install command, and
1414
* focus the terminal — but do NOT press Enter. The user reviews the
1515
* command (and activates their conda env / venv first when relevant)
1616
* before running it. Auto-running here would install into whatever
1717
* Python the shell's default `pip` points at — usually wrong when the
1818
* user has a project venv they haven't activated yet.
1919
*
20+
* Reuses any existing "Pipeline-Check install" terminal that is still
21+
* alive (`exitStatus === undefined`) so repeated clicks on the
22+
* welcome-panel CTA don't stack identical terminals in the dropdown.
23+
* A terminal the user already closed (exitStatus is set) is treated
24+
* as dead and a fresh one takes its place.
25+
*
2026
* Pulled out as a module-level function (rather than an extension-
2127
* internal closure) so the welcome-panel command, the LSP-failure
2228
* toast, and the test suite all hit the same code path.
2329
*/
2430
export function installInTerminal(): vscode.Terminal {
25-
const terminal = vscode.window.createTerminal(TERMINAL_NAME);
31+
const existing = vscode.window.terminals.find(
32+
(t) => t.name === TERMINAL_NAME && t.exitStatus === undefined,
33+
);
34+
const terminal = existing ?? vscode.window.createTerminal(TERMINAL_NAME);
2635
terminal.show();
2736
// The second argument to sendText is `addNewLine`; passing `false`
2837
// suppresses the Enter press, which is the whole point.

src/scanOnSave.test.ts

Lines changed: 40 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ function makeDeps(
3131
const calls = { scan: 0 };
3232
const deps: ScanOnSaveDeps = {
3333
isEnabled: overrides.isEnabled ?? (() => true),
34-
isPipelineFile: overrides.isPipelineFile ?? (() => true),
34+
shouldScanOnSave: overrides.shouldScanOnSave ?? (() => true),
3535
scan:
3636
overrides.scan ??
3737
(() => {
@@ -55,7 +55,7 @@ describe("createScanOnSaveHandler", () => {
5555
// The handler closes over a single boolean so the busy semantics
5656
// and the gate checks can be locked down without a real save
5757
// stream. The cheap-gates-first ordering (isEnabled before
58-
// isPipelineFile before busy) is the contract — it means a
58+
// shouldScanOnSave before busy) is the contract — it means a
5959
// non-CI save in a workspace with scanOnSave off pays only one
6060
// config read.
6161

@@ -66,8 +66,12 @@ describe("createScanOnSaveHandler", () => {
6666
expect(fake.scanCalls).toBe(0);
6767
});
6868

69-
it("returns immediately for non-CI files even when enabled", async () => {
70-
const fake = makeDeps({ isPipelineFile: () => false });
69+
it("returns immediately when shouldScanOnSave gates the file out", async () => {
70+
// shouldScanOnSave returns false for either (a) non-CI files
71+
// (package.json, mkdocs.yml, etc.) or (b) CI files whose
72+
// provider the user has put in `disabledProviders`. Either way
73+
// the handler bails before locking the busy flag.
74+
const fake = makeDeps({ shouldScanOnSave: () => false });
7175
const handler = createScanOnSaveHandler(fake.deps);
7276
await handler(doc("/repo/package.json"));
7377
expect(fake.scanCalls).toBe(0);
@@ -120,7 +124,7 @@ describe("createScanOnSaveHandler", () => {
120124
// rest of the session.
121125
const deps: ScanOnSaveDeps = {
122126
isEnabled: () => true,
123-
isPipelineFile: () => true,
127+
shouldScanOnSave: () => true,
124128
scan: () => Promise.reject(new Error("transient")),
125129
};
126130
const handler = createScanOnSaveHandler(deps);
@@ -131,7 +135,7 @@ describe("createScanOnSaveHandler", () => {
131135
let secondScanFired = false;
132136
const handler2 = createScanOnSaveHandler({
133137
isEnabled: () => true,
134-
isPipelineFile: () => true,
138+
shouldScanOnSave: () => true,
135139
scan: () => {
136140
secondScanFired = true;
137141
return Promise.resolve();
@@ -158,24 +162,49 @@ describe("createScanOnSaveHandler", () => {
158162
await inFlight;
159163
});
160164

161-
it("checks isEnabled BEFORE isPipelineFile (cheap gate first)", async () => {
165+
it("checks isEnabled BEFORE shouldScanOnSave (cheap gate first)", async () => {
162166
// When scanOnSave is off, we shouldn't even bother classifying
163167
// the saved path. Locks down the cheap-gates-first ordering so
164168
// a future refactor doesn't accidentally invert it and add a
165169
// providerForPath call on every save in a workspace that has
166170
// scan-on-save disabled.
167-
let pipelineFileCalls = 0;
171+
let shouldScanCalls = 0;
168172
const deps: ScanOnSaveDeps = {
169173
isEnabled: () => false,
170-
isPipelineFile: () => {
171-
pipelineFileCalls += 1;
174+
shouldScanOnSave: () => {
175+
shouldScanCalls += 1;
172176
return true;
173177
},
174178
scan: () => Promise.resolve(),
175179
};
176180
const handler = createScanOnSaveHandler(deps);
177181
await handler(doc("/repo/.gitlab-ci.yml"));
178-
expect(pipelineFileCalls).toBe(0);
182+
expect(shouldScanCalls).toBe(0);
183+
});
184+
185+
it("re-evaluates shouldScanOnSave on every save (disabled-providers can flip)", async () => {
186+
// A user updates `pipelineCheck.disabledProviders` mid-session
187+
// and saves again; the handler must consult the live value, not
188+
// a snapshot taken at construction.
189+
let disabled = false;
190+
const fake = makeDeps({
191+
shouldScanOnSave: () => !disabled,
192+
});
193+
const handler = createScanOnSaveHandler(fake.deps);
194+
const first = handler(doc("/repo/Dockerfile"));
195+
fake.scanResolvers[0]();
196+
await first;
197+
expect(fake.scanCalls).toBe(1);
198+
// Now silence the provider.
199+
disabled = true;
200+
await handler(doc("/repo/Dockerfile"));
201+
expect(fake.scanCalls).toBe(1); // no new scan
202+
// And unsilence.
203+
disabled = false;
204+
const third = handler(doc("/repo/Dockerfile"));
205+
fake.scanResolvers[1]();
206+
await third;
207+
expect(fake.scanCalls).toBe(2);
179208
});
180209

181210
it("each handler instance has its own busy flag (no cross-instance lock)", async () => {

0 commit comments

Comments
 (0)