Rejecting Shell Strings in AI-Generated Code: A Security Review of an Agent Tool PR

A Friday production incident brought down a small checkout service. The culprit was a Pull Request submitted by an AI Agent. That patch added a helper function called run_check for smoke testing.

The entire diff was only forty lines and looked fairly clean. But one template string broke that cleanliness — the helper function assembled a shell command by concatenating a branch name. As more teams hand off coding work to AI Agents, this kind of code that looks harmless but isn't is quietly slipping into codebases.

Read the Code First, Then the Summary

The Agent's summary promised a faster review cycle, but the handler function itself told a stricter, narrower story: a branch field from the tool JSON being interpolated directly into a command string.

// Untrusted sketch from the PR. Do not merge it.
function runCheck(branch) {
  const cmd = `git fetch origin ${branch} && npm test`;
  return exec(cmd, { shell: true });
}

The core issue is about trust boundaries. In this handler, the tool JSON is caller-controlled input. A shell string introduces a second parser into the execution path. If tests only ever pass a "clean" branch name like main, that second parser never gets triggered — so the vulnerability lies dormant until a malicious or malformed branch name like feature;echo shows up.

This reveals the first principle of reviewing AI-generated code: don't let the summary set the agenda — look at the real call path first. Agents are good at packaging dangerous operations to look "clean," and security issues tend to hide in that one unremarkable line of string concatenation.

Command Injection refers to a vulnerability where an attacker embeds shell metacharacters (such as ;, &&, |, $(), etc.) into input data, causing a string that was meant to carry only data to be interpreted by the shell as additional commands. When Node.js's exec() or spawn() is called with shell: true, the OS first hands the entire command string to /bin/sh for parsing before executing the result. This means a branch name containing feature;rm -rf / causes the shell to execute two commands in sequence — and the caller has no idea. The reason it's called a "second parser" is that the program code itself is the first parser, and the shell is the second: when two parsers process the same string in series, the boundary between data and instructions disappears.

Preserve the Goal, Remove the Mechanism

The fix isn't about starting from scratch — it's about precisely distinguishing what to keep from what to discard.

What to keep: if an existing task already calls this tool name, keep it; keep the fetch-then-test execution order; keep any comments requiring a timeout. These are business goals and they're fine.

What to discard: delete the template string in the next commit; remove the shell: true flag on this call path; drop any dependency package whose sole purpose is to wrap shell commands. These are dangerous mechanisms, not real requirements.

This "preserve the goal, replace the mechanism" approach is more actionable than vaguely saying "rewrite it," and it's easier for the PR author to accept — because it doesn't reject the requirement, only the implementation.

Enforce a Fixed Argument List

The right approach is to use execFile with a separate argument array, reject non-compliant branch text with a strict regex, and set timeouts so that stuck child processes can terminate.

import { execFile } from "node:child_process";
import { promisify } from "node:util";

const execFileAsync = promisify(execFile);
const BRANCH = /^[A-Za-z0-9._/-]{1,80}$/;

export async function runCheck(branch) {
  if (!BRANCH.test(branch)) {
    throw new Error("branch_rejected");
  }
  await execFileAsync("git", ["fetch", "origin", branch], {
    timeout: 20_000,
    shell: false,
  });
  return execFileAsync("npm", ["test"], {
    timeout: 120_000,
    shell: false,
  });
}

There are three key changes here: arguments are passed as an array so the branch name always travels as pure data, never as an executable script; shell: false completely shuts off the second parser; and the two timeout values ensure that a stuck process doesn't hold up a task indefinitely. It's worth noting that this code should be treated as a "proposed patch" — it defines the shape that a review should demand, not a final solution that has been stress-tested on production hardware.

The fundamental difference between execFile and exec lies in how arguments are passed. exec accepts a single string and by default hands it to the shell for parsing; execFile launches the specified executable directly via the execve system call, with each element of the argument array passed by the OS as a separate byte sequence, completely bypassing the shell layer. This means that even if the branch parameter contains ;, spaces, or $VAR, git will treat it as a single literal argument rather than triggering any shell expansion. promisify wraps a callback-based Node.js core API into a Promise-returning form, enabling use with async/await — this doesn't affect the security semantics, it only improves readability and error handling.

Lock the Rules into the Test File

Fixing the code alone isn't enough — you need tests that lock down the rules and prevent the next PR from sneaking shell strings back in. The approach is to place the test file alongside other tool tests, read the handler's source code from disk during the test, and fail the job if any shell patterns are detected.

import assert from "node:assert/strict";
import { readFileSync } from "node:fs";
import test from "node:test";
import { runCheck } from "../tools/runCheck.js";

const src = readFileSync(
  new URL("../tools/runCheck.js", import.meta.url),
  "utf8"
);

test("handler does not build a shell string", () => {
  assert.equal(src.includes("shell: true"), false);
  assert.equal(src.includes("exec(`"), false);
  assert.match(src, /execFile/);
  assert.match(src, /timeout:/);
});

test("bad branch names fail before a process starts", async () => {
  await assert.rejects(() => runCheck("feature;echo"), /branch_rejected/);
  await assert.rejects(() => runCheck(""), /branch_rejected/);
});

There are two types of assertions here: one scans the source text directly to ensure that shell: true and backtick exec no longer appear; the other is behavioral testing that verifies malformed branch names (feature;echo and empty string) are rejected before any process starts. These two rejection cases must "fail closed" (reject by default) and should be run with node --test on a clean checkout. Passing locally is just the initial signal — the same file should also be run on a disposable runner, with the exit code recorded in the PR.

"Fail closed" (reject by default) is a fundamental principle in security design, as opposed to "fail open" (allow by default). In validation logic, fail closed means that when input doesn't satisfy the allowlist rules, the system must explicitly reject it rather than "trying to allow" it or silently ignoring it. For branch name validation, this means empty strings, names containing special characters, and overly long strings should all throw errors before any child process is launched. Simultaneously asserting on the source text in tests (checking for the literal presence of certain strings) is a lightweight regression guard — it doesn't test runtime behavior but instead locks down code structure directly, making it impossible for a subsequent PR to quietly reintroduce dangerous patterns without breaking the tests.

An Eight-Step Process for Reviewing AI-Submitted PRs

Here is a directly applicable review workflow, built around the principle of "isolate first, verify second, human decision last":

1. Freeze the diff and hide the Agent's summary during the first review pass
2. List every value sourced from tool JSON, environment files, or review text
3. Flag every value that flows into exec, spawn, or backtick commands
4. Replace shell interpolation with argument arrays and explicit rejection paths
5. Add source assertions and two rejection test cases
6. Run the test file on an idle server that holds no production secrets
7. Send only the frozen diff and test logs to a free model
8. Have a human owner approve the merge after reading both outputs

Step six requires a "draft workspace" with no shared state: clone the PR branch into it, and keep production tokens away from the review machine. Copy the command output back into the review thread — a non-zero exit code means revert, not debate. Don't rerun the job with production credentials.

Let Free Models Draft Notes, Not Approve Merges

(Disclosure: this article was prepared as promotional content for MonkeyCode, which its operators describe as an open-source project.)

Free models can be used to draft an initial review note, and a free server can run this test suite outside your laptop. But keep firm boundaries: only paste the diff and test logs — never include environment files, customer data rows, or cookies. You can ask the model to return conclusions in trust, revert, and test columns, and strip out any fabricated "security advisory numbers."

Review this frozen diff for shell interpolation.
Return trust, revert, and test rows.
Do not suggest new network calls.

If the note asks you to read live secrets, stop immediately; if the note rewrites the handler, re-run the tests. Model text can only be kept as a comment — it does not constitute merge approval. A green model note can never substitute for a decision by a human owner.

Use a Review Matrix When the Diff Gets Large

Here is a practical signal-to-action reference table you can use as a review aid:

Signal in the diff	Review action	Reason
Tool field interpolated into a shell string	Revert	Untrusted text meets the second parser
shell: true next to a fixed command	Revert	execFile does not need this flag
Tests only cover the name main	Supplement	Malformed branches will never be rejected
git or npm missing a timeout	Add one	Stuck child processes shouldn't hold up a job
New dependency added solely to wrap sh	Remove	Supply chain bloat with no real benefit
Model notes with no file references	Ignore	Unsourced suggestions don't count as review evidence

This matrix is a review aid, not a scoring rubric or benchmark. For an approved "allowlisted wrapper," one row may not apply — in that case, reference the wrapper in the review comment, but still prohibit bare command strings in new tools.

Honest Limitations and Scope

A good review states its own limitations in the same place: the branch regex is only a gate, not a complete ref syntax — legitimate Unicode names will be rejected "safely"; production CI should call git and npm using absolute paths; source scanning will miss calls scattered in other files, so the scan should be run across the entire PR scope. Also note that execFile still launches local binaries, so a writable PATH should remain on the review checklist.

There are three scenarios where this path should not be used: when the runner holds production credentials (shared compute is not the place for secrets — use an internally locked runner instead); when policy requires signed private images; and when maintainers must run arbitrary scripts (this requires a separate allowlist design, not a "thinner shell string"). Pure style reviews can safely skip this test suite — it's only needed when tool code can launch processes. This boundary keeps the extra workload minimal.

Close the review with a specific comment: name the template string being reverted, link to the test file and exit code, and note that the free server run only covered this one file. The checkout service needs a "boring" handler — a branch name must always be pure data, not a shell script.

The writable PATH risk mentioned here means: although execFile avoids shell injection, the git and npm it calls are still looked up via the PATH environment variable. If an attacker can modify the process's PATH (for example, by writing to a .env file or contaminating the parent process environment), they could place a malicious executable with the same name earlier in the search path, causing execFile('git', ...) to actually run an attacker-controlled program. Using absolute paths (such as /usr/bin/git) eliminates this vector, at the cost of coupling to a specific system environment. This also illustrates that security hardening is layered — eliminating shell injection does not eliminate all risks on the execution path, and every layer of defense has its own boundary.