Reject Shell Strings in an Agent Tool Pull Request

A Friday incident stalled a small checkout service. The failing job came from an agent pull request. The patch added a helper named run_check for smoke runs.

The diff held forty lines and looked mostly tidy. One template string broke that tidy picture. The helper built a shell command from a branch name.

Read the code before the summary

The agent summary promised a faster review loop. The handler told a stricter and smaller story. A branch field from tool JSON entered 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 });
}

Tool JSON counts as caller-controlled input in this handler. A shell string adds a second parser to the path. A test that only passes main never sees that parser.

Keep the goal and drop the mechanism

Keep the tool name if existing jobs call it. Keep the sequence of fetch and then test. Keep any comment that asks for a timeout.

Drop the template string on the next commit. Drop the shell true flag on this call path. Drop a package whose only role is wrapping a shell.

Demand a fixed argument list

Call execFile with a separate argument array. Reject branch text that fails a tight pattern. Set a timeout so a stuck child ends.

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,
  });
}

Treat this block as a proposed patch. It was not benchmarked on live hardware. It defines the shape the review should require.

Lock the rule into a test file

Store the harness next to the other tool tests. Read the handler source from disk during the test. Fail the job if shell mode comes back.

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/);
});

Those two rejection cases must fail closed. They should throw before git or npm starts. Run them with node --test on a clean checkout tree.

A local pass is only a starting signal. The same file should run on a disposable runner. Record the exit code in the pull request.

Review the pull request in eight steps

1. Freeze the diff and hide the agent summary for the first pass.
2. List values that arrive from tool JSON, env files, or review text.
3. Flag every value that reaches exec, spawn, or a backtick command.
4. Replace shell interpolation with argument arrays and an explicit reject path.
5. Add the source assertions and both rejection cases from the harness.
6. Run that file on a free server that holds no production secrets.
7. Send only the frozen diff and the test log to a free model.
8. Let a human owner approve the merge after reading both outputs.

Step six needs a scratch workspace with no shared state. Clone the pull request branch into that workspace. Leave production tokens off that review machine.

Copy the command output into the review thread. Treat a nonzero exit as a revert, not a debate. Do not rerun the job with production credentials attached.

Let a free model draft notes, not approval

Disclosure: This article was prepared as part of MonkeyCode's product outreach. The operator describes MonkeyCode as an open-source project. Free model access can draft the first review note.

A free server option can run the harness off a laptop. This draft does not name a specific model. It does not state a token quota, disk size, or keep-alive time.

Those details change, so check the current docs before a long run. Older writeups are not evidence of today's limits. Paste the diff and the test log.

Leave out env files, customer rows, and cookies. Ask for three columns named trust, revert, and test. Drop any line that invents an advisory number.

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

Stop if the note requests a live secret. Rerun the harness if the note rewrites the handler. Keep the model text as a comment.

It is not a merge approval by itself. The human owner still owns the merge decision. A green model note never replaces that owner.

Readers who already have access can rerun this harness on a scratch branch. Attach that exit code beside the review note.

Use a matrix when the diff grows

Signal in the diff	Review action	Reason
Tool field copied into a shell string	Revert	Untrusted text meets a second parser
shell: true beside a fixed command	Revert	execFile does not need that flag
Test covers only the name main	Extend	A bad branch never gets rejected
Missing timeout on git or npm	Add	A stuck child should not hold the job
Package added only to wrap sh	Remove	Supply chain grows without a real gain
Model note with no file citation	Ignore	Uncited advice is not review evidence

This matrix works only as a review aid. It is not a score and not a benchmark.

A row can be wrong for a sanctioned wrapper. Cite that wrapper in the review comment. Still ban raw command strings in the new tool.

State the limits in the same review

The branch pattern is a gate, not a full ref grammar. Valid Unicode names fail closed on purpose. Production CI should call git and npm by absolute path.

A free server image can change without a notice in this article. No uptime, region, or storage promise is made here. A source scan misses calls that sit in other files.

Run the scan across the whole pull request. The execFile call still starts local binaries. A writable PATH stays on the review list.

A comment that mentions shell true can trip the source scan. Confirm the hit against the real call before reverting a doc file.

The free model sees only the pasted context. It can miss a policy note in another doc. Human approval remains required for auth, billing, and migrations.

Do not use this path in three settings

Skip the free server when the runner holds production credentials. Shared capacity is the wrong place for those secrets. Use an internal locked runner for that case.

Skip the path when policy demands a signed private image. A shared option cannot satisfy that control. Record the exception in the team handbook.

Skip it when maintainers must run arbitrary scripts. That need wants a separate allowlist design. A thinner shell string is not that design.

Style-only reviews can safely ignore this harness. Use the harness when tool code can start processes. That boundary keeps the extra work small.

Close the review with one concrete comment

Name the reverted template string in the comment. Link the harness file and the exit code. Say the free-server run covered this file only.

Ask the author to confirm current free-tier limits first. A larger suite can wait for that check. The comment does not need a product pitch.

The checkout service needs a boring handler. A branch name must stay plain data. It must not become a shell script.