From ad60dccd045c0f6477bff89b464da2c133902cdb Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Sat, 3 Oct 2026 04:55:49 +0000 Subject: [PATCH] test(windows): the windowsHide guard fails closed on spawns it can't trace (#2312) (#2315) the watcher's `git config --get core.excludesFile` call. That report is against 1.6.1; the call (and the `ls-files --exclude-standard` one next to it) already runs with windowsHide on main since #2280, which has not been released yet. An audit of every child_process call in src/ found nothing else missing it: 31 call sites, 29 hidden, 2 documented exceptions (macOS-only vm_stat; the detached browser opener). What #2280's AST guard could not see is how the next regression would slip in. It only followed static named/namespace imports and passed silently on anything else, so each of these shipped green with no windowsHide: - child_process loaded by a default import, `import = require`, `require()`, `import()`, or a re-export - a spawner aliased or passed as a value (`const run = execFileSync`, `promisify(execFile)`, `const { spawn } = cp`) - options in a `let`, or a later spread that may set windowsHide back - an options parameter of an exported wrapper (callers in other files), a wrapper used as a callback, or a wrapper call that omits the options The guard now fails closed on all of them, and a second describe pins each shape (plus the compliant ones it must keep passing) so the guard itself can't go blind again. Also credit #2312 on the existing changelog entry and drop the duplicate copy of that entry #2280 left under [Unreleased]. Co-authored-by: Claude Opus 5.5 --- CHANGELOG.md | 2 +- __tests__/child-process-windows-hide.test.ts | 282 +++++++++++++++---- 2 files changed, 233 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a333b1e2..049af59d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,7 +58,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). #### Windows -- On Windows, terminal windows no longer flash open and closed while CodeGraph runs in the background. Since 1.6.1, a background MCP server popped up a console window (a full Windows Terminal window when that is the default terminal) several times when it started and again every time it re-synced a changed file. All of CodeGraph's git calls now run hidden. Thanks @Suharaz, @23q3, @A-Van-Gestel and @HarryMuc. (#2094, #2096) +- On Windows, terminal windows no longer flash open and closed while CodeGraph runs in the background. Since 1.6.1, a background MCP server popped up a console window (a full Windows Terminal window when that is the default terminal) several times when it started and again every time it re-synced a changed file. All of CodeGraph's git calls now run hidden. Thanks @Suharaz, @23q3, @A-Van-Gestel, @HarryMuc and @MzaGuille. (#2094, #2096, #2312) - On Windows, a project opened with a different drive-letter or folder-name case, like `d:\work\app` and `D:\Work\App`, now joins the CodeGraph background server that is already running for it. Before, each spelling started a server of its own that exited at once, so the session quietly served the graph by itself with no shared file watching or auto-sync, and `codegraph list` showed the project twice. Thanks @greatjackzhou. (#2278) - On Windows, `codegraph upgrade` no longer breaks the install while an agent session is using CodeGraph. It used to stop partway and leave CodeGraph unable to start; it now swaps the new files in with CodeGraph running and puts the previous version back if anything fails, and re-running the PowerShell installer works the same way. If an earlier upgrade already broke your install, re-run `irm https://raw.githubusercontent.com/colbymchenry/codegraph/main/install.ps1 | iex` to repair it. Thanks @tippmar-nr for the report. (#2185) - On Windows, CodeGraph no longer sometimes crashes with exit code 3221225477 (an access violation) on a busy machine: right after parsing in `codegraph init`, `codegraph index` and background syncs, or as the background server shuts down. Stopping one of its helper threads (the ones that parse files, resolve references, write the index and count changes for `codegraph_status`) while it was still starting up, or before it had finished cleaning up its memory, took the whole process down. Threads now finish starting and clean up before they exit. diff --git a/__tests__/child-process-windows-hide.test.ts b/__tests__/child-process-windows-hide.test.ts index adb7056b..0e969934 100644 --- a/__tests__/child-process-windows-hide.test.ts +++ b/__tests__/child-process-windows-hide.test.ts @@ -6,13 +6,21 @@ * Windows, a console program (git, …) it starts without windowsHide gets a * brand-new visible console, so a terminal window flashes on screen and closes * once per call: on daemon start and again on every auto-sync (#485, #928, - * #1092, #2094, #2096). With Windows Terminal set as the default terminal, each - * flash is a full Windows Terminal window. + * #1092, #2094, #2096, #2312). With Windows Terminal set as the default + * terminal, each flash is a full Windows Terminal window. * * windowsHide is Windows-only behavior the POSIX test runs can't observe, so it * is asserted on the TypeScript AST. Options passed through a variable or a * function parameter are traced back to the object literal that defines them * (for a parameter: through every call site of that function in the file). + * + * The guard fails closed: a spawn it can't trace is an offender, not a pass. + * That covers child_process reached any way other than a static import + * (`require`, `import()`, a re-export), a spawner used as a value instead of + * called (`const run = execFileSync`, `promisify(execFile)`, `const { spawn } = + * cp`), options in a `let`, and an options parameter of an exported function + * (its callers in other files are out of sight). The second `describe` pins + * each of those shapes so the guard itself can't silently go blind. */ import { describe, it, expect } from 'vitest'; import * as fs from 'fs'; @@ -36,6 +44,8 @@ const SPAWNERS: Record = { fork: 'file', }; +const CHILD_PROCESS_MODULES = new Set(['child_process', 'node:child_process']); + /** * Spawns that intentionally run without windowsHide, keyed by * `:()`, valued by the reason. @@ -56,11 +66,18 @@ function listSourceFiles(dir: string): string[] { return out; } +function isChildProcessModule(node: ts.Node | undefined): boolean { + return node !== undefined && ts.isStringLiteralLike(node) && CHILD_PROCESS_MODULES.has(node.text); +} + /** Local names bound to child_process exports in one file. */ interface ChildProcessBindings { /** Local name → imported child_process export (`import { spawn as s }`). */ named: Map; - /** Namespace aliases (`import * as cp from 'child_process'`). */ + /** + * Names bound to the module object: `import * as cp`, `import cp` (with + * esModuleInterop) and `import cp = require(…)`. + */ namespaces: Set; } @@ -68,9 +85,16 @@ function childProcessBindings(sf: ts.SourceFile): ChildProcessBindings { const named = new Map(); const namespaces = new Set(); for (const stmt of sf.statements) { - if (!ts.isImportDeclaration(stmt) || !ts.isStringLiteral(stmt.moduleSpecifier)) continue; - if (!['child_process', 'node:child_process'].includes(stmt.moduleSpecifier.text)) continue; - const bindings = stmt.importClause?.namedBindings; + if (ts.isImportEqualsDeclaration(stmt)) { + const ref = stmt.moduleReference; + if (ts.isExternalModuleReference(ref) && isChildProcessModule(ref.expression)) namespaces.add(stmt.name.text); + continue; + } + if (!ts.isImportDeclaration(stmt) || !isChildProcessModule(stmt.moduleSpecifier)) continue; + const clause = stmt.importClause; + if (!clause) continue; // bare `import 'child_process'` binds nothing + if (clause.name) namespaces.add(clause.name.text); + const bindings = clause.namedBindings; if (!bindings) continue; if (ts.isNamespaceImport(bindings)) namespaces.add(bindings.name.text); else for (const el of bindings.elements) named.set(el.name.text, (el.propertyName ?? el.name).text); @@ -104,6 +128,26 @@ function unwrap(expr: ts.Expression): ts.Expression { return e; } +function isCallee(node: ts.Node): boolean { + return ts.isCallExpression(node.parent) && node.parent.expression === node; +} + +/** + * True when `id` reads the binding it names as a value. False for a type + * position (`typeof execFileSync`), and for an identifier that only names a + * declaration or a member: `x.exec`, `{ exec: … }`, `const spawn = …`, the + * import specifier itself. A re-export (`export { spawn }`) and a shorthand + * property (`{ spawn }`) DO read the value. + */ +function isValueReference(id: ts.Identifier): boolean { + for (let n: ts.Node | undefined = id.parent; n; n = n.parent) if (ts.isTypeNode(n)) return false; + const p = id.parent as ts.Node & { name?: ts.Node; propertyName?: ts.Node }; + if (ts.isShorthandPropertyAssignment(p) || ts.isExportSpecifier(p)) return true; + if (p.name === id || p.propertyName === id) return false; + if (ts.isQualifiedName(p) && p.right === id) return false; + return true; +} + /** Variable initializer or function parameter that `name` refers to at `from`. */ function resolveBinding(name: string, from: ts.Node): ts.VariableDeclaration | ts.ParameterDeclaration | null { for (let scope: ts.Node | undefined = from.parent; scope; scope = scope.parent) { @@ -122,51 +166,86 @@ function resolveBinding(name: string, from: ts.Node): ts.VariableDeclaration | t return null; } -/** Calls to the function that declares `param`, found by name in the same file. */ -function callSitesOf(param: ts.ParameterDeclaration, sf: ts.SourceFile): ts.Expression[] | null { +/** + * The argument every call of the function that declares `param` passes for it + * (undefined where a call omits it), found by name in the same file. Null when + * the callers can't all be seen: the function is unnamed, exported (callers in + * other files), or used as a value (a callback receives whatever its caller + * passes). + */ +function callSitesOf(param: ts.ParameterDeclaration, sf: ts.SourceFile): (ts.Expression | undefined)[] | null { const fn = param.parent; - const fnName = ts.isFunctionDeclaration(fn) && fn.name ? fn.name.text : null; - if (!fnName) return null; + if (!ts.isFunctionDeclaration(fn) || !fn.name) return null; + if (fn.modifiers?.some((m) => m.kind === ts.SyntaxKind.ExportKeyword)) return null; + const fnName = fn.name.text; const index = fn.parameters.indexOf(param); - const args: ts.Expression[] = []; + const args: (ts.Expression | undefined)[] = []; + let usedAsValue = false; const visit = (node: ts.Node): void => { - if (ts.isCallExpression(node) && ts.isIdentifier(node.expression) && node.expression.text === fnName) { - const arg = node.arguments[index]; - if (arg) args.push(arg); + if (ts.isIdentifier(node) && node.text === fnName && node !== fn.name && isValueReference(node)) { + if (!isCallee(node)) usedAsValue = true; + else { + const call = node.parent as ts.CallExpression; + // A spread before the parameter's slot hides which value lands in it. + if (call.arguments.slice(0, index + 1).some(ts.isSpreadElement)) usedAsValue = true; + else args.push(call.arguments[index]); + } } ts.forEachChild(node, visit); }; visit(sf); - return args.length > 0 ? args : null; + return usedAsValue || args.length === 0 ? null : args; } -/** True when `expr` provably evaluates to options with `windowsHide: true`. */ -function hidesWindow(expr: ts.Expression | undefined, sf: ts.SourceFile, seen = new Set()): boolean { - if (!expr) return false; +/** + * What options `expr` evaluates to do with windowsHide: provably set to true + * (`hidden`), provably not set at all (`absent`, e.g. no options passed, or a + * spread of `{ cwd }`), or anything else (`unhidden`: false, a non-literal, or + * unknowable). `absent` matters for spreads: `{ windowsHide: true, ...extra }` + * stays hidden only when `extra` provably doesn't set it back. + */ +type HideState = 'hidden' | 'absent' | 'unhidden'; + +function hideState(expr: ts.Expression | undefined, sf: ts.SourceFile, seen = new Set()): HideState { + if (!expr) return 'absent'; const e = unwrap(expr); - if (seen.has(e)) return false; - seen.add(e); + if (seen.has(e)) return 'unhidden'; // a cycle proves nothing + // Path-local: two call sites passing the same options object both resolve. + const trail = new Set(seen).add(e); if (ts.isObjectLiteralExpression(e)) { - let hidden = false; + let state: HideState = 'absent'; for (const prop of e.properties) { if (ts.isPropertyAssignment(prop) && prop.name.getText(sf) === 'windowsHide') { - hidden = unwrap(prop.initializer).kind === ts.SyntaxKind.TrueKeyword; + state = unwrap(prop.initializer).kind === ts.SyntaxKind.TrueKeyword ? 'hidden' : 'unhidden'; } else if (ts.isShorthandPropertyAssignment(prop) && prop.name.text === 'windowsHide') { - hidden = false; // not provable statically - } else if (ts.isSpreadAssignment(prop) && hidesWindow(prop.expression, sf, seen)) { - hidden = true; + state = 'unhidden'; // not provable statically + } else if (ts.isSpreadAssignment(prop)) { + const spread = hideState(prop.expression, sf, trail); + if (spread !== 'absent') state = spread; } } - return hidden; // last write wins, matching object-literal semantics + return state; // last write wins, matching object-literal semantics } if (ts.isIdentifier(e)) { const binding = resolveBinding(e.text, e); - if (!binding) return false; - if (ts.isVariableDeclaration(binding)) return hidesWindow(binding.initializer, sf, seen); + if (!binding) return 'unhidden'; + if (ts.isVariableDeclaration(binding)) { + // A `let` or `var` can be reassigned after its initializer. + const list = binding.parent; + if (!ts.isVariableDeclarationList(list) || !(list.flags & ts.NodeFlags.Const)) return 'unhidden'; + return binding.initializer ? hideState(binding.initializer, sf, trail) : 'unhidden'; + } const sites = callSitesOf(binding, sf); - return sites !== null && sites.every((arg) => hidesWindow(arg, sf, seen)); + if (sites === null) return 'unhidden'; + const states = new Set(sites.map((arg) => hideState(arg, sf, trail))); + return states.size === 1 ? [...states][0] : 'unhidden'; } - return false; + return 'unhidden'; +} + +/** True when `expr` provably evaluates to options with `windowsHide: true`. */ +function hidesWindow(expr: ts.Expression | undefined, sf: ts.SourceFile): boolean { + return hideState(expr, sf) === 'hidden'; } function commandLabel(call: ts.CallExpression, sf: ts.SourceFile): string { @@ -176,33 +255,136 @@ function commandLabel(call: ts.CallExpression, sf: ts.SourceFile): string { return first.getText(sf); } -describe('child processes set windowsHide (#1092, #2094)', () => { +/** Every spawn in one file, and each one that isn't provably hidden. */ +function scanSource(rel: string, text: string): { seen: number; offenders: string[] } { + const sf = ts.createSourceFile(rel, text, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); + const bindings = childProcessBindings(sf); + const offenders: string[] = []; + let seen = 0; + const flag = (node: ts.Node, what: string): void => { + const { line } = sf.getLineAndCharacterOfPosition(node.getStart(sf)); + offenders.push(`${rel}:${line + 1} ${what}`); + }; + const visit = (node: ts.Node): void => { + if (ts.isExportDeclaration(node) && isChildProcessModule(node.moduleSpecifier)) { + flag(node, 're-exports child_process: spawns through the re-export are out of sight'); + } + if (ts.isCallExpression(node)) { + const callee = node.expression; + const loads = + (ts.isIdentifier(callee) && callee.text === 'require') || callee.kind === ts.SyntaxKind.ImportKeyword; + if (loads && isChildProcessModule(node.arguments[0])) { + flag(node, `${callee.getText(sf)}(child_process): import it statically so its spawns can be checked`); + } + const spawner = spawnerOf(node, bindings); + if (spawner) { + seen++; + const key = `${rel}:${spawner}(${commandLabel(node, sf)})`; + if (!Object.hasOwn(ALLOWED, key) && !hidesWindow(optionsArg(node, spawner), sf)) flag(node, key); + } + } + const bound = ts.isIdentifier(node) && (bindings.named.has(node.text) || bindings.namespaces.has(node.text)); + if (bound && isValueReference(node)) { + const imported = bindings.named.get(node.text); + if (imported !== undefined && Object.hasOwn(SPAWNERS, imported) && !isCallee(node)) { + flag(node, `${node.text} used as a value: call it directly so its options can be checked`); + } else if (bindings.namespaces.has(node.text)) { + const member = node.parent; + const direct = + ts.isPropertyAccessExpression(member) && + member.expression === node && + (!Object.hasOwn(SPAWNERS, member.name.text) || isCallee(member)); + if (!direct) flag(node, `${node.getText(sf)} used as a value: call its spawners directly so their options can be checked`); + } + } + ts.forEachChild(node, visit); + }; + visit(sf); + return { seen, offenders }; +} + +describe('child processes set windowsHide (#1092, #2094, #2312)', () => { it('every child_process spawn under src/ sets windowsHide: true', () => { const offenders: string[] = []; let seen = 0; for (const file of listSourceFiles(SRC_DIR)) { - const text = fs.readFileSync(file, 'utf8'); - const sf = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); - const bindings = childProcessBindings(sf); - if (bindings.named.size === 0 && bindings.namespaces.size === 0) continue; const rel = path.relative(SRC_DIR, file).split(path.sep).join('/'); - const visit = (node: ts.Node): void => { - if (ts.isCallExpression(node)) { - const spawner = spawnerOf(node, bindings); - if (spawner) { - seen++; - const key = `${rel}:${spawner}(${commandLabel(node, sf)})`; - if (!Object.hasOwn(ALLOWED, key) && !hidesWindow(optionsArg(node, spawner), sf)) { - const { line } = sf.getLineAndCharacterOfPosition(node.getStart(sf)); - offenders.push(`${rel}:${line + 1} ${key}`); - } - } - } - ts.forEachChild(node, visit); - }; - visit(sf); + const result = scanSource(rel, fs.readFileSync(file, 'utf8')); + seen += result.seen; + offenders.push(...result.offenders); } expect(seen).toBeGreaterThan(0); // guard against a false pass if the imports move expect(offenders, `spawned without windowsHide:\n${offenders.join('\n')}`).toEqual([]); }); }); + +describe('the windowsHide guard fails closed', () => { + const scan = (text: string) => scanSource('fixture.ts', text); + + it.each([ + [ + 'the core.excludesFile read the watcher repeats (#2312)', + `import { execFileSync } from 'child_process'; + execFileSync('git', ['-C', root, 'config', '--get', 'core.excludesFile'], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] });`, + ], + ['windowsHide: false', `import { spawn } from 'child_process'; spawn('git', [], { windowsHide: false });`], + ['a later spread overriding it', `import { spawn } from 'child_process'; spawn('git', [], { windowsHide: true, ...{ windowsHide: false } });`], + ['a later spread that might override it', `import { spawn } from 'child_process'; spawn('git', [], { windowsHide: true, ...extra() });`], + ['a default import', `import cp from 'child_process'; cp.execFileSync('git', ['status'], { encoding: 'utf8' });`], + ['an import-equals', `import cp = require('child_process'); cp.spawnSync('git', ['status']);`], + ['a require()', `const { execFileSync } = require('child_process'); execFileSync('git', ['status'], { windowsHide: true });`], + ['a dynamic import()', `async function f() { const cp = await import('node:child_process'); }`], + ['a re-export', `export { spawn } from 'child_process';`], + ['a re-export of the import', `import { spawn } from 'child_process'; export { spawn };`], + ['an alias', `import { execFileSync } from 'child_process'; const run = execFileSync; run('git', ['status']);`], + ['promisify', `import { execFile } from 'child_process'; import { promisify } from 'util'; const run = promisify(execFile);`], + ['a shorthand property', `import { spawn } from 'child_process'; const deps = { spawn };`], + ['a destructured namespace', `import * as cp from 'child_process'; const { spawn } = cp; spawn('git', []);`], + ['a namespace spawner as a value', `import * as cp from 'child_process'; [].forEach(cp.spawn);`], + [ + 'options in a let', + `import { execFileSync } from 'child_process'; + let opts = { windowsHide: true }; opts = {}; execFileSync('git', [], opts);`, + ], + [ + 'an exported wrapper (its callers elsewhere are out of sight)', + `import { execFileSync } from 'child_process'; + export function git(args: string[], opts: object) { return execFileSync('git', args, opts); } + git(['status'], { windowsHide: true });`, + ], + [ + 'a wrapper call that omits the options', + `import { execFileSync } from 'child_process'; + function git(args: string[], opts?: object) { return execFileSync('git', args, opts); } + git(['status'], { windowsHide: true }); git(['log']);`, + ], + [ + 'a wrapper passed as a callback', + `import { execFileSync } from 'child_process'; + function git(opts: object) { return execFileSync('git', [], opts); } + git({ windowsHide: true }); [{}].map(git);`, + ], + ])('flags %s', (_label, text) => { + expect(scan(text).offenders).not.toEqual([]); + }); + + it.each([ + ['a literal', `import { execFileSync } from 'child_process'; execFileSync('git', ['status'], { windowsHide: true });`], + ['(file, options)', `import { execFileSync } from 'child_process'; execFileSync('git', { windowsHide: true });`], + ['a command spawner', `import { exec } from 'child_process'; exec('git status', { windowsHide: true }); /a/.exec('a');`], + ['a const', `import { spawn } from 'child_process'; const o = { windowsHide: true }; spawn('git', [], o);`], + ['a spread const', `import { spawn } from 'child_process'; const base = { windowsHide: true }; spawn('git', [], { ...base, cwd: '.' });`], + ['a later spread that leaves it alone', `import { spawn } from 'child_process'; const extra = { cwd: '.' }; spawn('git', [], { windowsHide: true, ...extra });`], + ['a namespace', `import * as cp from 'node:child_process'; let c: cp.ChildProcess | undefined; cp.spawn('git', [], { windowsHide: true });`], + [ + 'a wrapper whose every call passes the same hidden options', + `import { execFileSync } from 'child_process'; + function git(opts: Parameters[2]) { return execFileSync('git', [], opts); } + const gitOpts = { cwd: '.', windowsHide: true }; + git(gitOpts); git(gitOpts);`, + ], + ['types only', `import type { ChildProcess } from 'child_process'; let c: ChildProcess | undefined;`], + ])('passes %s', (_label, text) => { + expect(scan(text).offenders).toEqual([]); + }); +});