Uh oh!
There was an error while loading. Please reload this page.
[eslint-miner] eslint: add no-exec-interpolated-command rule - #46586
Conversation
Adds a new custom ESLint rule that flags calls to exec.exec() and
exec.getExecOutput() where the first (command) argument is an
interpolated template literal or a dynamic string concatenation.
The @actions/exec runner splits the command string by spaces internally;
variable values containing spaces silently break argument boundaries,
causing subtle argument-splitting bugs. The safe pattern is to pass a
static command string and move all dynamic values to the args array:
// unsafe — if branchName contains spaces, arguments are split incorrectly
await exec.exec(`git checkout -B ${branchName} ${ref}`);
// safe — arguments are passed verbatim without shell splitting
await exec.exec('git', ['checkout', '-B', branchName, ref]);
Evidence: 10 existing violations found across actions/setup/js:
- create_pull_request.cjs (7 violations)
- push_to_pull_request_branch.cjs (3 violations)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>✅ Test Quality Sentinel completed test quality analysis. |
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46586 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
✅ PR Code Quality Reviewer completed the code quality review. |
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds an ESLint rule preventing dynamically interpolated @actions/exec command strings.
Changes:
- Detects interpolated templates and dynamic concatenations.
- Adds rule tests and plugin registration.
- Enables the rule as a warning.
Show a summary per file
| File | Description |
|---|---|
eslint-factory/src/rules/no-exec-interpolated-command.ts | Implements detection and diagnostics. |
eslint-factory/src/rules/no-exec-interpolated-command.test.ts | Tests valid and invalid calls. |
eslint-factory/src/index.ts | Exports the new rule. |
eslint-factory/eslint.config.cjs | Enables the rule. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Medium
| * binary `+` of two static expressions. | ||
| */ | ||
| function isStaticExpression(node: TSESTree.Node): boolean { | ||
| if (node.type === "Literal") return typeof node.value === "string"; |
| interpolatedCommand: | ||
| "Avoid passing a {{kind}} as the exec command — @actions/exec splits the command string by spaces, so values containing spaces silently break argument boundaries. " + | ||
| "Use a static command string and pass all arguments in the args array: exec.exec('git', ['checkout', branchName]).", |
| "gh-aw-custom/require-new-url-try-catch": "warn", | ||
| "gh-aw-custom/prefer-core-logging": "warn", | ||
| "gh-aw-custom/no-core-error-then-process-exit": "warn", | ||
| "gh-aw-custom/no-exec-interpolated-command": "warn", |
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 30.1 AIC · ⌖ 7.14 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
eslint-factory/src/rules/no-exec-interpolated-command.ts:156
False negative: rule only matches the hardcoded object name exec, missing any import alias.
<details>
<summary>💡 Details</summary>
isExecCall checks obj.name !== "exec", which means the rule is silently bypassed if a developer imports @actions/exec under any alias:
import*asefrom"`@actions/exec`";e.exec(`git checkout ${branch}`);// not flagged — obj.name is "e", not "exec"This is a documented limitation (PR description calls it "low false-positive risk"), but…
eslint-factory/src/rules/no-exec-interpolated-command.test.ts:57
Missing test for exec.exec(...) called with a namespace import alias — the valid-case comment says "Not exec.exec" but the test only covers someOther.exec, not e.g. execUtil.exec.
<details>
<summary>💡 Details</summary>
The rule explicitly checks for the object name exec only. The existing valid test for someOther.exec(git checkout ${branch}) is good, but the flip side — execAlias.exec(...) — is also a false negative (not flagged) and there's no test that documents/ass…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design — commenting with minor improvements on test coverage, type safety, and scope documentation.
📋 Key Themes & Highlights
Key Themes
- Scope ambiguity:
isExecCallonly matches the literalexecidentifier — aliased imports are silently skipped. Either document this is intentional (github-script injection only) or broaden the match. - Missing edge-case tests: multi-segment static+dynamic
BinaryExpressionconcat andSpreadElementfirst argument both lack coverage, leaving correct-but-unverified behaviour. - Type annotation gap:
firstArgshould have an explicitSpreadElementguard for clarity and future type safety.
Positive Highlights
- ✅ Solid rule logic: the
isStaticExpressionrecursion handles nestedBinaryExpressionchains correctly - ✅ Good test coverage of the happy path (5 valid + 5 invalid cases)
- ✅ Clear, actionable error message with the safe pattern to follow
- ✅ Clean plugin integration — minimal surface area in
index.tsandeslint.config.cjs - ✅ PR description includes real evidence (10 existing violations with file/line detail)
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 36.5 AIC · ⌖ 4.96 AIC · ⊞ 6.7K
Comment /matt to run again
| * exec.exec(cmd, args?, opts?) | ||
| * exec.getExecOutput(cmd, args?, opts?) | ||
| */ | ||
| function isExecCall(node: TSESTree.CallExpression): boolean { |
There was a problem hiding this comment.
[/codebase-design]isExecCall matches only the literal object name "exec" — aliased or namespaced imports will silently bypass the rule.
💡 Why this matters
Code like import * as actions from "@actions/exec" followed by actions.exec(...), or const runner = exec then runner.exec(...), will never be flagged. The rule description says it targets exec.exec() / exec.getExecOutput() in .cjs/.js/.ts files — but those files commonly alias the import.
If the intent is specifically the exec identifier injected by github-script, that should be documented explicitly in the rule docs. If broader coverage is desired, consider matching on the property names alone (callee.property.name === "exec" on any member expression whose object is not a well-known safe type) or using TypeScript type information to resolve the callee.
@copilot please address this.
| CallExpression(node) { | ||
| if (!isExecCall(node)) return; | ||
| const firstArg = node.arguments[0]; |
There was a problem hiding this comment.
[/tdd]node.arguments[0] can be a SpreadElement (e.g. exec.exec(...args)), but getDynamicCommandKind receives a TSESTree.Node and passes it to isInterpolatedTemplateLiteral / isDynamicStringConcatenation — both will safely return false for a SpreadElement, so no crash. However the type annotation TSESTree.Node is misleading: CallExpression.arguments is typed as (Expression | SpreadElement)[], so firstArg could be a SpreadElement. An explicit type guard keeps the intent clear and avoids a future TypeScript error if the parameter type is tightened.
💡 Suggested fix
constfirstArg=node.arguments[0];if(!firstArg||firstArg.type===AST_NODE_TYPES.SpreadElement)return;This makes the SpreadElement exclusion explicit and the remaining code can narrow firstArg to TSESTree.Expression.
@copilot please address this.
| // Bare exec() call — not a member expression | ||
| { code: `exec(\`git checkout \${branch}\`);` }, | ||
| ], | ||
| invalid: [ |
There was a problem hiding this comment.
[/tdd] Missing test: a mixed static+dynamic binary concat chain like "git checkout " + branchName + " " + ref is not covered. The recursive isStaticExpression handles it correctly today, but a test would pin that behaviour and catch regressions.
💡 Suggested test case
// Multi-segment dynamic concatenation — should be flagged{code: `exec.exec("git checkout " + branchName + " " + ref, [], opts);`,errors: [{messageId: "interpolatedCommand"}],},Also worth adding a valid case for a BinaryExpression that is fully static: exec.exec("git" + " checkout", [branch]) — verifying the rule does not fire on all + expressions.
@copilot please address this.
| // Bare exec() call — not a member expression | ||
| { code: `exec(\`git checkout \${branch}\`);` }, | ||
| ], | ||
| invalid: [ |
There was a problem hiding this comment.
[/tdd] No test covers exec.exec(...spreadArg) — a call whose first argument is a spread element. The rule currently silently skips it (correct behaviour), but without a test that invariant is invisible and can regress.
💡 Suggested valid case
// Spread first argument — rule should not fire{code: `exec.exec(...args);`},@copilot please address this.
| * Returns true when the node is a template literal that contains at least one | ||
| * interpolated expression. | ||
| */ | ||
| function isInterpolatedTemplateLiteral(node: TSESTree.Node): boolean { |
There was a problem hiding this comment.
[/codebase-design]isInterpolatedTemplateLiteral is only ever called from getDynamicCommandKind and is a thin one-liner. Inlining it into getDynamicCommandKind (or removing the helper) would reduce indirection without losing readability. The current split adds a navigation hop without providing a reusable seam.
💡 Simplified form
functiongetDynamicCommandKind(node: TSESTree.Node): string|null{if(node.type==="TemplateLiteral"&&node.expressions.length>0)return"interpolated template literal";if(isDynamicStringConcatenation(node))return"dynamic string concatenation";returnnull;}This is a minor style note — not blocking.
@copilot please address this.
There was a problem hiding this comment.
Review: no-exec-interpolated-command ESLint rule
Three issues found — two correctness bugs and one type-mismatch in the error message. Blocking on the file-glob misconfiguration since it prevents the rule from ever running on .ts source files.
Summary of issues
| # | File | Line | Severity | Description |
|---|---|---|---|---|
| 1 | eslint.config.cjs | 36 | Blocking | Rule added inside the *.cjs-only override — never runs on .ts/.js files |
| 2 | no-exec-interpolated-command.ts | 78 | Blocking | Error message prescribes exec.exec() as the fix for exec.getExecOutput() calls, changing the return type |
| 3 | no-exec-interpolated-command.ts | 19 | Non-blocking | isStaticExpression only accepts string literals, so "cmd" + 3 is incorrectly flagged as dynamic |
The rule logic itself (AST traversal, callee shape detection) looks correct and the test cases are well-structured.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 20 AIC · ⌖ 5.46 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 88/100 — Excellent
📊 Metrics (12 tests)
Test Classification
Analysis SummaryStrengths:
Minor observations:
Result: 16.7% implementation tests (threshold: 30%) — well under limit. No violations. All critical design paths covered. Verdict✅ APPROVED. Test Quality Sentinel: 88/100. 16.7% implementation tests (threshold: 30%). Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 88/100 APPROVED
Excellent test coverage with 83% design tests, 100% edge/error coverage, and only 16.7% implementation tests (threshold: 30%). All design invariants verified with comprehensive boundary testing. Zero violations. Test inflation is healthy at 0.54:1.
pelikhan
commented
Jul 19, 2026
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the pr-finisher pass. Local fixes are in |
Uh oh!
There was an error while loading. Please reload this page.
gh-aw-bot
commented
Jul 19, 2026
@copilot please run the Unresolved review threads (newest first):
Please also refresh this branch from its base branch before rerunning checks. Run: https://github.com/github/gh-aw/actions/runs/29683215651 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new ESLint rule
gh-aw-custom/no-exec-interpolated-commandto theeslint-factoryplugin. The rule flags interpolated template literals and dynamic string concatenation used as the first (command) argument ofexec.exec()orexec.getExecOutput()calls in github-script CommonJS action scripts.Motivation
@actions/execsplits the command string on spaces internally. Variables containing spaces silently break argument boundaries, causing hard-to-diagnose failures. The safe pattern is a static command string with all arguments in the second array parameter.Changes
eslint-factory/src/rules/no-exec-interpolated-command.tseslint-factory/src/rules/no-exec-interpolated-command.test.tseslint-factory/src/index.tsnoExecInterpolatedCommandRulein the plugineslint-factory/eslint.config.cjswarnseveritypkg/linters/README.mdtrimleftrightGo analyzer (import + registration example)Rule Behaviour
Flagged patterns: interpolated template literals (
exec.exec(\git checkout ${branch}`)) and dynamic string concatenation (exec.exec("git checkout " + branchName)). Applies to bothexec.execandexec.getExecOutput`.Allowed patterns: static command strings/template literals, static concatenations, identifier arguments, and non-
execcallees.Breaking Changes
None. The rule is enabled at
warnseverity.