Skip to content

fix: support .cmd/.bat shims on Windows in exec() (#446) #469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nikos1005 wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: support .cmd/.bat shims on Windows in exec() (#446) #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2180683
fix: guard against undefined recordId parts in googledns deleteRecord…
May 29, 2026
ee8b387
fix: correct tsconfig.base.json extends paths for bot packages (#452)
May 29, 2026
7a2923b
fix: support .cmd/.bat shims on Windows in exec() helper (#446)
May 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/bot/core/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
{
"extends": "../../tsconfig.base.json",
"extends": "../../../tsconfig.base.json",
"compilerOptions": {
"outDir": "./dist",
"rootDir": "./src"
},
"include": ["src/**/*.ts"]
}
"include": [
"src/**/*.ts"
]
}
8 changes: 5 additions & 3 deletions packages/bot/discord/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
{
"extends": "../../tsconfig.base.json",
"extends": "../../../tsconfig.base.json",
"compilerOptions": {
"outDir": "./dist",
"rootDir": "./src"
},
"include": ["src/**/*.ts"]
}
"include": [
"src/**/*.ts"
]
}
8 changes: 5 additions & 3 deletions packages/bot/signal/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
{
"extends": "../../tsconfig.base.json",
"extends": "../../../tsconfig.base.json",
"compilerOptions": {
"outDir": "./dist",
"rootDir": "./src"
},
"include": ["src/**/*.ts"]
}
"include": [
"src/**/*.ts"
]
}
8 changes: 5 additions & 3 deletions packages/bot/telegram/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
{
"extends": "../../tsconfig.base.json",
"extends": "../../../tsconfig.base.json",
"compilerOptions": {
"outDir": "./dist",
"rootDir": "./src"
},
"include": ["src/**/*.ts"]
}
"include": [
"src/**/*.ts"
]
}
8 changes: 5 additions & 3 deletions packages/bot/whatsapp/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
{
"extends": "../../tsconfig.base.json",
"extends": "../../../tsconfig.base.json",
"compilerOptions": {
"outDir": "./dist",
"rootDir": "./src"
},
"include": ["src/**/*.ts"]
}
"include": [
"src/**/*.ts"
]
}
6 changes: 5 additions & 1 deletion packages/core/src/exec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,14 @@ export async function exec(cmd: string, args: string[], opts: ExecOptions): Prom
}

return new Promise<ExecResult>((resolve, reject) => {
const child = spawn(cmd, args, {
// On Windows, .cmd/.bat shims must be spawned with shell:true or via cmd.exe
const isWin = process.platform === 'win32';
const needsShell = isWin && (cmd.endsWith('.cmd') || cmd.endsWith('.bat'));
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Case-insensitive extension matching needed on Windows

Windows file systems are case-insensitive, so shims named foo.CMD or foo.BAT (uppercase) will not be detected, and spawning them without the shell wrapper will fail with ENOENT. Lowercasing before the check avoids this edge case.

Suggested change
const needsShell = isWin && (cmd.endsWith('.cmd') || cmd.endsWith('.bat'));
const needsShell = isWin && (cmd.toLowerCase().endsWith('.cmd') || cmd.toLowerCase().endsWith('.bat'));

const child = spawn(needsShell ? 'cmd' : cmd, needsShell ? ['/d', '/s', '/c', cmd, ...args] : args, {
cwd: opts.cwd,
env: { ...process.env, ...extraEnv },
stdio: ['ignore', 'pipe', 'pipe'],
...(needsShell ? { shell: true } : {}),
});
Comment on lines +34 to 39
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Double-wrapping through cmd.exe

When needsShell is true, the code manually sets the executable to 'cmd' and prepends /d /s /c to the args, but it also passes shell: true. On Windows, shell: true causes Node.js to wrap the command as cmd.exe /d /s /c "<exe> <args>" — so the actual invocation becomes cmd.exe /d /s /c cmd /d /s /c foo.cmd <args>, spawning cmd.exe twice. Choose one approach: either manually invoke cmd without shell: true, or keep shell: true and just pass the original cmd/args.

Suggested change
const child = spawn(needsShell ? 'cmd' : cmd, needsShell ? ['/d', '/s', '/c', cmd, ...args] : args, {
cwd: opts.cwd,
env: { ...process.env, ...extraEnv },
stdio: ['ignore', 'pipe', 'pipe'],
...(needsShell ? { shell: true } : {}),
});
const child = spawn(needsShell ? 'cmd' : cmd, needsShell ? ['/d', '/s', '/c', cmd, ...args] : args, {
cwd: opts.cwd,
env: { ...process.env, ...extraEnv },
stdio: ['ignore', 'pipe', 'pipe'],
});


let stdout = '';
Expand Down
9 changes: 7 additions & 2 deletions packages/dns/googledns/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,19 @@ export default defineDns<Config>({
const token = await getAccessToken();
const project = config.projectId ?? _secret('GOOGLE_PROJECT_ID');
if (!project) throw new Error('GOOGLE_PROJECT_ID not set');
const [type, name] = recordId.split('/');
const parts = recordId.split('/');
const type = parts[0];
const name = parts.slice(1).join('/');
if (!type || !name) throw new Error(`Invalid recordId "${recordId}" — expected "<type>/<FQDN>"`);
// Need to fetch the rrset to get current rrdatas for the deletion entry.
const existing = (await this.listRecords(zoneId, config)).filter(
r => r.type === type && (r.name === name || r.name === name.replace(/\.$/, '')),
);
if (existing.length === 0) return;
const first = existing[0];
if (!first) return;
Comment on lines 134 to +136
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Redundant null guard after a length check

existing.length === 0 is checked on the line immediately above, so when execution reaches const first = existing[0], the array is guaranteed to be non-empty and first can never be undefined. The if (!first) return; guard is dead code and can be removed.

Suggested change
if (existing.length === 0) return;
const first = existing[0];
if (!first) return;
if (existing.length === 0) return;
const first = existing[0];

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

const fqdn = name.endsWith('.') ? name : `${name}.`;
const ttl = existing[0].ttl;
const ttl = first.ttl;
const res = await fetch(`${API}/projects/${project}/managedZones/${zoneId}/changes`, {
method: 'POST',
headers: { Authorization: `Bearer ${token}`, 'Content-Type': 'application/json' },
Expand Down