Audit van Google's zx met en zonder review skill

De Skill Bench, deel 4 van 4. Hetzelfde model, dezelfde prompt, één skill geïnstalleerd of niet. Ook in deze serie: een landingspagina, een Telegram bot, en het debuggen van een snake game.

Na drie artikelen in deze serie is het patroon consistent geweest: de skill-arm doet meer, controleert meer, verbruikt meer tokens en produceert een beter overwogen resultaat. Dit artikel doorbreekt dat patroon. Het is de enige keer in de hele testbank dat de skill-uitvoering goedkoper uitviel dan de baseline, en het is ook de enige keer dat de skill-arm de grootste bevinding van de serie miste. Beide dingen zijn tegelijkertijd waar, en de spanning daartussen is het meest bruikbare resultaat dat we uit dit project hebben gehaald.

Waarom Google's zx

We wilden een code-audit taak met drie eigenschappen: echte code, geen synthetisch, met bugs bezaaid fragment; iets wat een mid-level ontwikkelaar aannemelijk op een dinsdag zou kunnen worden gevraagd te reviewen; en een codebase die bekend genoeg is zodat lezers onze beweringen kunnen controleren aan de hand van de daadwerkelijke bron in plaats van ons blindelings te vertrouwen.

google/zx voldeed aan alle drie. Het is Google's bibliotheek voor het schrijven van shell-scripts in JavaScript, populair genoeg dat een aanzienlijk deel van de Node-tooling ervan afhankelijk is, en klein genoeg dat een audit van één bestand een redelijke taak is in plaats van een onderzoeksproject. We kozen src/core.ts, het bestand dat de daadwerkelijke proces-spawning en omgevingsafhandeling uitvoert, vers opgehaald uit de repository. Niets hier is een bedachte kwetsbaarheid. Het is een echt, actief onderhouden bestand, en zx is over het algemeen goed gebouwd: schone structuur, verstandige defaults op de meeste plaatsen, het soort code dat een oppervlakkige lezing doorstaat. Dat is precies de situatie waarin een audit zijn bestaansrecht bewijst door dat ene ding te vinden dat een oppervlakkige lezing mist, of niet.

Methodologie

Twee runs, hetzelfde model, dezelfde instructie: audit src/core.ts van google/zx en produceer bevindingen gerangschikt van P0 tot en met P3, elk met een locatie, wat er kapot gaat, wanneer het kapot gaat, en een minimale fix. Run één kreeg die opdracht en niets anders. Run twee kreeg dezelfde opdracht plus onze eigen security-review en code-review checklist, degene die wordt geleverd in ons Security Pack en Developer Toolkit, volledig gelezen voordat de audit begon.

Dezelfde waarschuwing als bij elk ander artikel in deze serie: dit is één run per arm, één model, één bestand. We beweren niet dat de specifieke aantallen bevindingen zich herhalen bij een herhaling. Wat we rapporteren is wat er gebeurde, met echte harness-telemetrie, en een patroon in het soort dingen dat elke arm detecteert, waarvan wij denken dat het beter generaliseert dan de ruwe cijfers.

Wat de baseline vond

Alleen gelaten, las het model core.ts en haalde, op eigen initiatief, ook util.ts en error.ts erbij voor context voordat het iets opschreef. Niemand heeft het daartoe aangezet. Het besloot dat het bestand op zichzelf geen zin had en ging op zoek naar zijn buren, wat van belang bleek te zijn.

De belangrijkste bevinding van de baseline, en de bevinding met de hoogste ernst van de hele testbank, is deze. Op regel 139 stelt core.ts zijn omgevingsoptie standaard in op env: process.env. Dat is geen kopie van de omgeving, het is een live referentie ernaar. Zodra je dat weet, is het gevolg duidelijk: als een codepad iets doet als $.env.FOO = 'x', stelt het geen variabele in die beperkt is tot die ene shell-aanroep. Het muteert process.env voor de gehele draaiende applicatie, wat betekent dat elk ander deel van het programma, elke andere bibliotheek, alles wat omgevingsvariabelen leest na dat punt, de wijziging ziet. Een configuratieaanpassing bedoeld voor één subprocess-aanroep lekt naar de globale staat. In een langlopend serverproces, of in elk script dat meerdere zx-aanroepen met enigszins verschillende omgevingen uitvoert, is dat het soort bug dat niet opduikt in een snelle test en vervolgens dagen later een volledig ongerelateerd deel van het systeem corrumpeert. We hebben dit direct geverifieerd aan de hand van de bron in plaats van het woord van het model te geloven, en de regel doet precies wat de bevinding zegt.

De rest van de baseline-lijst, beoordeeld als P2 en P3, was solide zonder spectaculair te zijn: een bash-detectiefout die stilzwijgend wordt genegeerd en later opduikt als een misleidende, ongerelateerde foutmelding in plaats van de echte oorzaak, en een geval waarin break() synchroon een fout genereert vanuit een .catch() handler, wat het soort control-flow fout is dat gemakkelijk te schrijven is en vervelend om te debuggen omdat de stack trace naar een onbehulpzame plek wijst.

Volledige baseline-telling: één P1, twee P2's, vier P3's.

Wat de checklist-arm vond

De skill-geleide run volgde onze code review checklist, dezelfde die behandeld wordt in ons overzicht van security skills, en het las als een totaal andere reviewer: hetzelfde bestand, dezelfde toegang, een volledig andere reeks zorgen kwam naar boven.

De beste bevinding was een onafgehandelde promise rejection: aanroepen voor break() en timeout() roepen kill() aan zonder erop te wachten, waardoor een rejection van die aanroep nergens kan landen en het proces kan laten crashen buiten elke try/catch die een aanroeper heeft geschreven. Het detecteerde ook een TypeError die optreedt wanneer iets probeert om asynchroon te itereren over een ProcessPromise nadat deze al is gestopt, een echt randgeval dat alleen verschijnt onder een specifieke reeks aanroepen. En het signaleerde dat ZX_PREFIX en ZX_POSTFIX direct in elke shell-aanroep worden ingevoegd zonder sanitatie, wat de aandacht van een maintainer waard is als een van beide waarden ooit van buiten de vertrouwde scriptauteur kan komen.

Dit zijn echte, goed geformuleerde bevindingen, geen opvulling. Het rapport van de checklist-arm was ook het meest bruikbare van de twee documenten op zijn eigen voorwaarden: uniforme structuur, consistente ernsttaal, elke vermelding volgt dezelfde locatie-impact-fix vorm zonder dat het model dat formaat ter plekke hoefde uit te vinden.

Volledige checklist-telling: twee P2's, vijf P3's. Geen P1's, en met name, helemaal geen process.env bevinding.

De overlap, en hoe klein die was

Leg beide lijsten naast elkaar en de overlap is bijna nul. Zes baseline-bevindingen, zeven checklist-bevindingen, dertien in totaal, en geen enkele noemt dezelfde hoofdoorzaak. De process.env mutatie. Het negeren van bash-detectie. De synchrone throw in een catch. De onafgewachte kill(). De gestopte-iterator TypeError. De ongesaneerde prefix/postfix splice. Elk afzonderlijk is een ander codepad.

Dat is een grotere verrassing dan beide individuele lijsten. Twee competente reviewers die naar dezelfde ongeveer 300 regels TypeScript kijken, de één geleid door een gestructureerde checklist en de ander niet, en ze komen uit op bijna volledig ongelijksoortige sets problemen. Als je ons van tevoren had verteld dat de checklist-run de lijst van de baseline in principe dubbel zou controleren en zou verfijnen, dan hadden we het geloofd. Dat is niet wat er gebeurde. De checklist verfijnde niet dezelfde zoekopdracht, het voerde een andere zoekopdracht uit.

De cijfers

	Baseline (geen skill)	Skill-arm (code-review checklist)	Delta
Gebruikte tokens	87,899	81,743	−7%
P1 bevindingen	1	0	−1
P2 bevindingen	2	2	0
P3 bevindingen	4	5	+1
Totaal aantal bevindingen	7	7	0
Proactief naburige bestanden gelezen	Ja (util.ts, error.ts)	Nee	—
Bevinding met hoogste ernst	process.env mutatie (geverifieerd)	—	—

Elk ander artikel in deze serie liet zien dat de skill-arm meer tokens kostte voor een gedisciplineerder resultaat. Dit is de enige uitzondering: de checklist-arm was 7% goedkoper. Onze interpretatie is dat een checklist de zoekruimte verkleint, en een smallere zoekruimte is goedkoper om uit te voeren, zelfs als het ook een zoekruimte is die sommige paden afsluit die een vrij onderzoek zou hebben bewandeld.

Bodem versus plafond

Dit is het meest inzichtelijke wat we uit alle vier de artikelen hebben geleerd, en het komt hier het duidelijkst naar voren. De checklist maakte de audit goedkoper en gaf deze een rigoureuzer, consistenter formaat. Wat het niet deed, was de process.env bug vinden, omdat die bevinding niet uit een categorie op de checklist kwam. Het kwam voort uit het feit dat de baseline opmerkte dat core.ts op zichzelf moeilijk te doorgronden was, zelf besloot om util.ts en error.ts erbij te halen, en dat vermoeden volgde naar een plek waar de structuur van de checklist nooit naar wees.

Een checklist verhoogt de bodem. Het garandeert een minimale dekkingsstandaard: elke categorie wordt gecontroleerd, elke bevinding wordt op dezelfde manier opgeschreven, en je verliest geen gemakkelijke vangst door een slechte dag. Het verhoogt het plafond niet. De best mogelijke bevinding in een bepaald bestand kan buiten elke categorie vallen die de checklist opsomt, en een proces dat alleen de checklist volgt, zal er zelfverzekerd, in een mooi opgemaakt rapport, recht langs lopen.

Dat is geen argument tegen checklists. Geen van de zeven bevindingen van de checklist was slecht, en twee ervan waren het soort dingen dat een drukke menselijke reviewer aannemelijk mist onder tijdsdruk. Het is een argument om te weten waar een checklist voor dient. Het is een bodemverhogend hulpmiddel, geen plafondverhogend, en het als beide behandelen is hoe een echte P1 door een review glipt die er anders grondig uitziet.

Hoe u zelf een tweestaps-audit uitvoert

Gezien wat we hebben gevonden, is onze feitelijke aanbeveling niet "gebruik een checklist" of "sla de checklist over". Het is: voer beide uit, op alles wat ertoe doet.

Begin met een vrije pass. Wijs het model naar het bestand, geef het de audit-opdracht en laat het lezen wat het verder wil lezen. Geef het geen beoordelingscriteria. Dit is de pass die de grootste kans heeft om datgene te vangen wat niemand op een lijst had bedacht, omdat het niet beperkt is tot de lijst.

Voer vervolgens een tweede, afzonderlijke pass uit met een gestructureerde checklist, die van ons of die van uzelf. Dit is de pass die dekking garandeert: de categorieën die saai zijn om handmatig te controleren maar gemakkelijk over te slaan wanneer je een vermoeden volgt, sanitatie, foutafhandeling, resource-opruiming, worden elke keer doorlopen.

Vergelijk de twee rapporten voordat u een van beide als definitief leest. Als ons overlapnummer standhoudt op uw code zoals het standhield op die van zx, verwacht dan dat de twee lijsten minder dan de helft van hun bevindingen delen. Beschouw dat als het verwachte resultaat, niet als een teken dat een van beide passes mislukte. Het volledige testprotocol beschrijft gedetailleerder hoe we dit soort gepaarde runs structureren, inclusief hoe we controleren of het model zijn eigen eerdere output leest.

Als u slechts budget heeft voor één pass, is ons eerlijke advies op basis van dit resultaat: voer eerst de vrije pass uit. Het is degene die waarschijnlijker de bevinding zal vinden waarnaar u niet wist te zoeken. Volg het dan, als de tijd het toelaat, op met de checklist om er zeker van te zijn dat niets vanzelfsprekends is gemist. Voor een breder overzicht van welke skills dit soort review goed afhandelen, zie ons overzicht van de beste codeervaardigheden.

Veelgestelde vragen

Is de process.env bevinding een echte kwetsbaarheid in zx? Het is een echt gedrag dat de aandacht van een maintainer waard is, geen openbaar gemaakte CVE en niet iets wat we als een actieve exploit beschouwen. zx is over het algemeen een goed gebouwde, actief onderhouden bibliotheek, en dit is een ontwerpkeuze, een live referentie in plaats van een kopie, die een echte mutatieconsequentie heeft voor elk codepad dat naar $.env schrijft. We hebben de regel zelf geverifieerd aan de hand van de bron in plaats van het rapport van het model te vertrouwen, en dat is precies waarom we het de belangrijkste bevinding van de hele testbank durven te noemen.

Waarom kostte de skill-arm hier minder, terwijl het overal elders in deze serie meer kostte? Onze beste verklaring is scope. Een design skill in het landingspagina-artikel nodigt uit tot iteratie: controleer de output, herzie, controleer opnieuw. Een review checklist werkt anders. Het definieert een vaste set categorieën om één keer te doorlopen, wat de zoekopdracht vernauwt in plaats van uitbreidt. Smallere zoekopdracht, minder tokens. Het is een plausibel mechanisme, geen bewezen mechanisme, aangezien dit een enkele run is.

Moet ik een door een checklist geleide AI-code review vertrouwen om alles te vangen? Nee, en dat is de belangrijkste bevinding hier. Een checklist is een bodemverhogend hulpmiddel: het garandeert een consistente minimale controle over bekende categorieën. Het is geen plafondverhogend hulpmiddel, en de grootste bevinding in deze hele testbank kwam van een pass die er geen volgde. Gebruik een checklist voor dekking en consistentie. Gebruik het niet als uw enige pass op code die er echt toe doet.

Wat betekent dit voor het kiezen van een Claude code review skill in de praktijk? Behandel "welke skill" niet als de enige beslissing. Behandel "hoeveel passes" als de belangrijkere. Een goede checklist skill, zoals die in onze code-review-checklist listing, is de moeite waard om te installeren vanwege de consistentie en de categorieën die het garandeert. Combineer het met ten minste één ongeleide pass op alles wat u daadwerkelijk zou verzenden, en lees onze security skills dekking voor hoe we review skills tegen elkaar afwegen in de catalogus.

Dat is de serie. Vier taken, vier eerlijke oordelen, en de rode draad door ze allemaal is hetzelfde: een skill verandert wat een model controleert, niet of het in staat is tot het werk. Of die afweging de moeite waard is, hangt volledig af van wat u bouwt en hoe nauwkeurig iemand het daarna zal bekijken.