We Audited Google's zx With and Without a Review Skill

The Skill Bench, part 4 of 4. Same model, same prompt, one skill installed or not. Also in the series: a landing page, a Telegram bot, and debugging a snake game.

Three entries into this series, the pattern has been consistent: the skill arm does more, checks more, spends more tokens, and produces a more considered result. This entry breaks that pattern. It’s the one time in the whole bench where the skill run came in cheaper than the baseline, and it’s also the one time the skill arm missed the single biggest finding of the series. Both things are true at once, and the tension between them is the most useful result we’ve gotten out of this project.

Why Google’s zx

We wanted a code audit task with three properties: real code, not a synthetic bug-planted snippet; something a mid-level developer could plausibly be asked to review on a Tuesday; and a codebase well-known enough that readers can go check our claims against the actual source instead of trusting us blind.

google/zx fit all three. It’s Google’s library for writing shell scripts in JavaScript, popular enough that a meaningful slice of Node tooling depends on it, and small enough that a single file audit is a fair task rather than a research project. We picked src/core.ts, the file that does the actual process-spawning and environment handling, fetched fresh from the repository. Nothing here is a contrived vulnerability. It’s a real, actively maintained file, and zx is generally well-built: clean structure, sensible defaults in most places, the kind of code that passes a casual read. That’s exactly the setup where an audit either earns its keep by finding the one thing a casual read misses, or it doesn’t.

Methodology

Two runs, same model, same instruction: audit src/core.ts from google/zx and produce findings ranked P0 through P3, each with a location, what breaks, when it breaks, and a minimal fix. Run one got that brief and nothing else. Run two got the identical brief plus our own security-review and code-review checklist, the one that ships in our Security Pack and Developer Toolkit, read in full before the audit started.

Same caveat as every other entry in this series: this is one run per arm, one model, one file. We’re not claiming the specific finding counts replicate on a re-run. What we’re reporting is what happened, with real harness telemetry, and a pattern in the kind of thing each arm catches that we think generalizes better than the raw numbers do.

What the baseline found

Left alone, the model read core.ts and, on its own initiative, also pulled in util.ts and error.ts for context before writing anything up. Nobody told it to. It decided the file didn’t make sense in isolation and went looking at its neighbors, which turned out to matter.

The baseline’s top finding, and the highest-severity finding of the entire bench, is this one. At line 139, core.ts defaults its environment option to env: process.env. That’s not a copy of the environment, it’s a live reference to it. Once you know that, the consequence is straightforward: if any code path does something like $.env.FOO = 'x', it isn’t setting a variable scoped to that one shell call. It’s mutating process.env for the entire running application, which means every other part of the program, every other library, anything that reads environment variables after that point, sees the change. A configuration tweak meant for one subprocess call leaks into global state. In a long-running server process, or in any script that fans out multiple zx calls with slightly different environments, that’s the kind of bug that doesn’t show up in a quick test and then corrupts a completely unrelated part of the system days later. We verified this directly against the source rather than taking the model’s word for it, and the line does exactly what the finding says.

The rest of the baseline’s list, rated P2 and P3, was solid without being spectacular: a bash-detection failure that gets silently swallowed and later surfaces as a misleading, unrelated error message instead of the real cause, and a case where break() throws synchronously from inside a .catch() handler, which is the kind of control-flow mistake that’s easy to write and annoying to debug because the stack trace points somewhere unhelpful.

Full baseline tally: one P1, two P2s, four P3s.

What the checklist arm found

The skill-guided run followed our code review checklist, the same one covered in our security skills roundup, and it read like a different reviewer entirely: same file, same access, a completely different set of concerns surfaced.

Its best find was an unhandled promise rejection: call sites for break() and timeout() invoke kill() without awaiting it, so a rejection from that call has nowhere to land and can crash the process outside of any try/catch a caller wrote. It also caught a TypeError that surfaces when something tries to async-iterate a ProcessPromise after it’s already been halted, a real edge case that only shows up under a specific sequence of calls. And it flagged that ZX_PREFIX and ZX_POSTFIX get spliced directly into every shell invocation without sanitization, worth a maintainer’s attention if either value can ever come from outside the trusted script author.

These are real, well-formed findings, not padding. The checklist arm’s report was also the more usable of the two documents on its own terms: uniform structure, consistent severity language, every entry following the same location-impact-fix shape without the model having to invent that format on the fly.

Full checklist tally: two P2s, five P3s. No P1s, and notably, no process.env finding at all.

The overlap, and how small it was

Line up both lists and the overlap is close to zero. Six baseline findings, seven checklist findings, thirteen total, and not one of them names the same root cause. The process.env mutation. The bash-detection swallow. The synchronous throw in a catch. The un-awaited kill(). The halted-iterator TypeError. The unsanitized prefix/postfix splice. Every single one is a different code path.

That’s a bigger surprise than either individual list. Two competent reviewers looking at the same 300-ish lines of TypeScript, one guided by a structured checklist and one not, and they land on almost entirely disjoint sets of problems. If you’d told us going in that the checklist run would basically double-check the baseline’s list and add polish, we’d have believed it. That’s not what happened. The checklist didn’t refine the same search, it ran a different search.

The numbers

	Baseline (no skill)	Skill arm (code-review checklist)	Delta
Tokens used	87,899	81,743	−7%
P1 findings	1	0	−1
P2 findings	2	2	0
P3 findings	4	5	+1
Total findings	7	7	0
Proactively read neighboring files	Yes (util.ts, error.ts)	No	—
Highest-severity find	process.env mutation (verified)	—	—

Every other entry in this series showed the skill arm costing more tokens for a more disciplined result. This is the one exception: the checklist arm was 7% cheaper. Our read is that a checklist narrows the search space, and a narrower search space is a cheaper one to execute, even when it’s also one that closes off some of the paths a free-roaming pass would have wandered down.

Floor vs. ceiling

This is the sharpest thing we’ve learned across all four entries, and it shows up most clearly here. The checklist made the audit cheaper and gave it a more rigorous, more consistent format. What it didn’t do is find the process.env bug, because that finding didn’t come from any category on the checklist. It came from the baseline noticing that core.ts alone was hard to reason about, deciding on its own to go pull util.ts and error.ts, and following that hunch somewhere the checklist’s structure never pointed.

A checklist raises the floor. It guarantees a minimum standard of coverage: every category gets checked, every finding gets written up the same way, and you don’t lose an easy catch to an off day. It does not raise the ceiling. The best possible finding on a given file might live outside every category the checklist enumerates, and a process that only walks the checklist will walk right past it, confidently, in a nicely formatted report.

That’s not an argument against checklists. Zero of the checklist’s seven findings were bad, and two of them were the kind of thing a busy human reviewer plausibly misses under time pressure. It’s an argument for knowing what a checklist is for. It’s a floor-raising tool, not a ceiling-raising one, and treating it as both is how a real P1 slips through a review that otherwise looks thorough.

How to run a two-pass audit yourself

Given what we found, our actual recommendation isn’t “use a checklist” or “skip the checklist.” It’s run both, on anything that matters.

Start with a free pass. Point the model at the file, give it the audit brief, and let it read whatever else it wants to read. Don’t hand it a rubric. This is the pass that has the best odds of catching the thing nobody thought to put on a list, because it isn’t constrained to the list.

Then run a second, separate pass with a structured checklist, ours or your own. This is the pass that guarantees coverage: the categories that are boring to check by hand but easy to skip when you’re following a hunch, sanitization, error handling, resource cleanup, get walked every time.

Diff the two reports before you read either one as final. If our overlap number holds up on your code the way it held up on zx’s, expect the two lists to share less than half their findings. Treat that as the expected outcome, not a sign either pass failed. The full testing protocol covers how we structure this kind of paired run in more detail, including how we control for the model reading its own prior output.

If you only have budget for one pass, our honest advice based on this result is: run the free pass first. It’s the one more likely to find the finding you didn’t know to look for. Then, if time allows, follow it with the checklist to make sure nothing boring got missed. For a wider view of which skills handle this kind of review well, see our best coding skills roundup.

FAQ

Is the process.env finding a real vulnerability in zx? It’s a real behavior worth a maintainer’s look, not a disclosed CVE and not something we’re framing as an active exploit. zx is a well-built, actively maintained library overall, and this is a design choice, a live reference instead of a copy, that has a genuine mutation consequence for any code path that writes to $.env. We verified the line ourselves against the source rather than trusting the model’s report, which is exactly why we’re comfortable calling it the top finding of the whole bench.

Why did the skill arm cost less here, when it cost more everywhere else in this series? Our best explanation is scope. A design skill in the landing page entry invites iteration: check the output, revise, check again. A review checklist works differently. It defines a fixed set of categories to walk once, which narrows the search rather than expanding it. Narrower search, fewer tokens. It’s a plausible mechanism, not a proven one, since this is a single run.

Should I trust a checklist-guided AI code review to catch everything? No, and that’s the headline finding here. A checklist is a floor-raising device: it guarantees a consistent minimum sweep across known categories. It’s not a ceiling-raising device, and the biggest finding in this whole bench came from a pass that wasn’t following one. Use a checklist for coverage and consistency. Don’t use it as your only pass on code that actually matters.

What does this mean for choosing a claude code review skill in practice? Don’t treat “which skill” as the only decision. Treat “how many passes” as the more important one. A good checklist skill, like the one in our code-review-checklist listing, is worth installing for the consistency and the categories it guarantees. Pair it with at least one unguided pass on anything you’d actually ship, and read our security skills coverage for how we weigh review skills against each other in the catalog.

That’s the series. Four tasks, four honest verdicts, and the throughline across all of them is the same: a skill changes what a model checks, not whether it’s capable of the work. Whether that trade is worth it depends entirely on what you’re building and how closely anyone’s going to look at it afterward.