Code Review and Quality — Agent-Skills  🔗 **Source:** [Code Review and Quality — Agent-Skills](https://skills.addy.ie/skills/code-review-and-quality/) > [!note]+ Document Notes > tags: [code-review, agent-skills, addy-osmani, review-severity-labels, change-sizing, structural-remedies, dependency-review, multi-model-review, google-eng-practices, claude-code-skills, review-checklists, ai-code-quality, attribution] > author: Addy Osmani > source: skills.addy.ie (addyosmani/agent-skills, MIT) > reference: https://skills.addy.ie/skills/code-review-and-quality/ > date created: 2026-09-03 10:40 > > > [!info] Generated analysis — not the author's words. Sections below are machine-written > > from the captured source. Only text inside `> [!quote]` blocks is verbatim from the author. > > Do not quote or attribute any other line to the named author. > > > [!warning] Three things to know before trusting this note > > **1. Re-saved capture.** The original Reader save of `skills.addy.ie/skills/code-review-and-quality/` captured 84 words (the catalog card: description, install command, "Read the full SKILL.md"). This doc was re-created on 2026-09-03 from the raw `SKILL.md` in `addyosmani/agent-skills` at `main` (3,262 words; 14 commits, first 2026-02-15, last 2026-08-07), rendered to plain HTML so highlights can anchor. The 84-word original is archived. The catalog page's "How this skill is structured" and related-skill links (code-simplification, security-and-hardening, performance-optimization) are not in this capture. > > **2. Unattributed derivation.** Several passages match Google's public *Engineering Practices* documentation (github.com/google/eng-practices, licensed CC-BY 3.0, which requires attribution): the "Delete the FizzBuzz RPC" first-line example, the "Fix bug" anti-pattern, "Technical facts and data overrule opinions," and the four splitting strategies (stacking, by file, horizontal, vertical) all appear there under the same names. `SKILL.md` mentions Google zero times. Treat the review doctrine as Google's, repackaged for agents, not as original research. > > **3. Practice document, not evidence.** No study, benchmark, or incident is cited anywhere. Every threshold (~100/300/1000 changed lines, ~1000 total lines per file, "one business day") is asserted. The repo has 91,862 stars as of today, which measures adoption, not efficacy. > > # 🏷️ Title: Code Review and Quality — agent-skills (SKILL.md) > <!-- deep-read:sections start --> ## 🧭 Table of Contents - [📝 Summary](#-summary) - [📖 Main Content](#-main-content) - [🗒️ Notes](#️-notes) - [📚 References](#-references) - [🗣️ Paraphrase](#️-paraphrase) - [📰 Newsletter](#-newsletter) - [✅ Action Items](#-action-items) - [🧪 Hypotheses](#-hypotheses) - [📊 Methodology](#-methodology) - [❓ Questions](#-questions) - [🔮 Future Research](#-future-research) - [💡 Implications](#-implications) - [🧠 Learning Styles Assessment](#-learning-styles-assessment) - [📊 Accessibility Assessment](#-accessibility-assessment) - [🌍 DEIB Assessment](#-deib-assessment) - [✅ Logical Validity](#-logical-validity) - [🛡️ DARVO Analysis](#-darvo-analysis) - [🎯 Conclusion](#-conclusion) ### 🔗 References - Source page: https://skills.addy.ie/skills/code-review-and-quality/ - Raw file processed: https://github.com/addyosmani/agent-skills/blob/main/skills/code-review-and-quality/SKILL.md - Repo: https://github.com/addyosmani/agent-skills (MIT, created 2026-02-15, 25 skills, 91,862 stars at capture) - Install path named in the page: `npx skills add addyosmani/agent-skills --skill code-review-and-quality` via https://github.com/vercel-labs/skills - Sibling skills the file defers to: `security-and-hardening`, `performance-optimization`; `../../references/security-checklist.md`, `../../references/performance-checklist.md` (not fetched) - Google Engineering Practices (unattributed source of several passages): https://github.com/google/eng-practices — `review/reviewer/standard.md`, `review/developer/cl-descriptions.md`, `review/developer/small-cls.md`, `review/reviewer/speed.md`; LICENSE is CC-BY 3.0 ## 📝 Summary **What this is.** One skill file from Addy Osmani's `agent-skills` pack: a workflow an AI coding agent follows to review a change before merge. It defines five review axes (correctness, readability, architecture, security, performance), a severity-label scheme (Critical / no-prefix Required / Optional / Nit / FYI), change-size targets, a five-step process that starts with tests, a dead-code hygiene ritual, a dependency-review discipline that treats upgrades as code changes, a rationalization table, and a markdown checklist for the agent to fill in. **Why it matters.** It's the most-installed public articulation of "what a code review is" written for agents rather than humans. With 25 skills and 91,862 stars, whatever this file says about severity labels and change sizing is now the default behavior of a lot of Claude Code, Codex, and Cursor sessions. The file's own claim about AI code is the load-bearing one: "AI code needs more scrutiny, not less. It's confident and plausible, even when wrong." **Key stakeholders.** Addy Osmani (author of record on the first commit and the site owner); anyone installing the pack through the Vercel `skills` CLI; Google, whose Engineering Practices guide supplies the CL-description example, the disagreement hierarchy, the splitting taxonomy, and the one-business-day norm without credit. **Risks and tradeoffs.** The doctrine is sound and portable, and that is also the problem: it's generic. Nothing in it is WordPress-, PHP-, or framework-specific; the security axis is eight generic questions that defer to another skill. The attribution gap is a license question, not a quality one. And the "Multi-Model Review Pattern" ends with "Human makes the final call" while the rest of the file is written so an agent can approve a change alone. **Bottom line.** Worth stealing three moves: the severity prefixes with "no prefix means required," the "one structural problem and ten nits" ordering rule, and the presumptive-blockers list at the end. Worth rejecting one: the file as a whole as a substitute for a stack-specific reviewer with a stance. Courtney's three reviewer agents already do what this file's security axis cannot. ## 📖 Main Content **The approval standard is Google's, and it's the right one.** Approve when the change "definitely improves overall code health, even if it isn't perfect." That single rule does more work than the checklist below it, because it converts review from gatekeeping ("is this how I'd write it") to a directional test ("is the codebase better after merge"). The rest of the file is machinery for deciding that question consistently. **Five axes, unevenly developed.** Correctness gets five questions. Readability gets ten, and they're the most opinionated in the file: "1000 lines where 100 suffice is a failure," "Don't generalize until the third use case," and two structural smells (a conditional bolted onto an unrelated flow; repeated conditionals on the same shape, which "signal a missing model or dispatcher"). Architecture adds the sharpest test in the document: "Does this refactor reduce complexity or just relocate it? Count the concepts a reader must hold." Security and performance are stubs that defer to sibling skills; their eight and six questions are a syllabus, not a method. **Structural remedies name the move.** The file insists a review that says "this is complex" is incomplete; the reviewer must propose a named restructuring (replace a chain of conditionals with a dispatcher, collapse duplicate branches, separate orchestration from business logic, delete a pass-through wrapper). This is the section that distinguishes it from a checklist: it asks the reviewer to do design work, not just detection. **Change sizing.** ~100 changed lines "good," ~300 "acceptable if it's a single logical change," ~1000 "too large. Split it." A second threshold: ~1000 total lines in a file is an inspection signal. Four splitting strategies (stack, by file group, horizontal, vertical) in a table. Refactoring and feature work are two changes. All of this is Google's Small CLs guidance with the CL vocabulary removed. **Change descriptions.** First line imperative and standalone ("Delete the FizzBuzz RPC" not "Deleting the FizzBuzz RPC"); body says what and why; anti-patterns "Fix bug," "Fix build," "Phase 1." Also Google's, nearly word for word. **The process.** Understand intent → review tests first → walk the implementation on the five axes → label findings → verify the author's verification story (tests run, build passed, manual check, screenshots). Step 2 is the notable ordering choice: tests before code, because "tests reveal intent and coverage." **Severity labels.** No prefix = required. **Critical:** blocks merge. **Nit:** optional. **Optional:/Consider:** suggestion. **FYI** informational. Then the ordering rule: correctness and security first, then structural regressions and missed simplifications, then everything else; "If you have one structural problem and ten nits, the structural problem *is* the review." **Multi-model review.** Model A writes, Model B reviews, A addresses, human decides. The justification is one sentence: "different models have different blind spots." No evidence offered; it's a plausible design, stated as practice. **Dead code hygiene.** After any change, list orphaned code explicitly and ask before deleting. The example block is the shape Courtney's own sessions use ("DEAD CODE IDENTIFIED … → Safe to remove these?"). **Review speed and disagreements.** Respond within one business day as a maximum; multiple rounds in a day is normal. Disputes resolve by hierarchy: facts and data, then style guide, then engineering principles, then codebase consistency. "Don't accept 'I'll clean it up later.'" Both sections are Google's. **Honesty in review.** Don't rubber-stamp, don't soften, quantify ("this N+1 query will add ~50ms per item"), push back, accept override gracefully, "comment on code, not people." The sycophancy line is the one aimed squarely at model reviewers. **Dependency discipline.** Five questions before adding a dependency; then a five-rule upgrade workflow: read the changelog, one dependency per change, let the tests decide, mind the transitive graph via the lockfile diff, never hand-edit the lockfile. "Every dependency is a liability." Supply-chain verdicts are deferred to `security-and-hardening`. **The checklist, rationalizations, red flags, verification.** A markdown checklist the agent fills in; a nine-row table of excuses and rebuttals; sixteen red flags; and a closing "presumptive blockers" list, which is the file's best compression of its own structural doctrine: relocated complexity, size growth without decomposition, feature logic in a shared module, near-duplicate helpers, silent fallbacks hiding an unclear invariant. **What's uncertain.** Whether any of the thresholds hold outside Google-scale monorepos; whether "one business day" means anything for an agent that reviews in seconds; whether the multi-model pattern catches more than a single model with a strong persona does. None of these are tested in the file. ## 🗒️ Notes - Frontmatter description doubles as the trigger: "Use before merging any change. Use when reviewing code written by yourself, another agent, or a human." - The overview's "no exceptions" and the approval standard's "even if it isn't perfect" sit two sentences apart. Both are needed; together they say "always review, rarely block." - Readability axis carries the pack's house philosophy in bold: fewer lines, abstractions must earn complexity, no dead code artifacts, no backwards-compat shims left behind. Same vocabulary as the `code-simplification` sibling skill the catalog page links. - Architecture axis question "Are type boundaries explicit?" targets TypeScript (`any`/`unknown`/casts). The file is JS/TS-shaped throughout: `npm audit`, lockfile, bundle impact, re-renders. Nothing for PHP, Composer, or WordPress hooks. - Security axis line "Is data from external sources (APIs, logs, user content, config files) treated as untrusted?" is the closest it comes to prompt-injection awareness, and it doesn't name it. - Change-size table is a fenced block, not a markdown table; the splitting strategies are a real table. Both survived the HTML conversion (as `<pre>` and `<table>`). - The dead-code section asks permission before deleting; the "presumptive blockers" section escalates to Required only when "the change actively makes structure worse." The file is careful about agent autonomy in exactly two places and silent about it everywhere else. - "Multi-Model Review Pattern" example prompt: "Review this code change for correctness, security, and adherence to our project conventions. The spec says [X]. The change should [Y]. Flag any issues as Critical, Required, Optional, or Nit." This is a prompt template inside a skill, which is the pack's stated design ("a workflow the agent follows, not a doc it reads"). - Dependency upgrade rule 3, "If coverage around the dependency's behavior is thin, that gap is the real finding — add a test first," is the one line in the file that turns a review finding into a maker task. - Rationalization table row: "AI-generated code is probably fine" → "AI code needs more scrutiny, not less. It's confident and plausible, even when wrong." Written by the author of a pack whose purpose is to make AI code better; the admission is against interest and therefore the most credible line in the file. - "See Also" points at `../../references/security-checklist.md` and `performance-checklist.md`; the last commit message (2026-08-07) is "fix(skills): resolve references/ links from the skill directory," so those paths were broken for some period. - Link audit: the `SKILL.md` body contains zero hyperlinks. Every external idea is named, not linked. The Google derivation is therefore "unattributed," not "uncited" in the narrow sense; there was nothing to cite from. ## 📚 References - [addyosmani/agent-skills — code-review-and-quality/SKILL.md](https://github.com/addyosmani/agent-skills/blob/main/skills/code-review-and-quality/SKILL.md) - [skills.addy.ie catalog page](https://skills.addy.ie/skills/code-review-and-quality/) - [google/eng-practices — The Standard of Code Review](https://github.com/google/eng-practices/blob/master/review/reviewer/standard.md) - [google/eng-practices — Writing good CL descriptions](https://github.com/google/eng-practices/blob/master/review/developer/cl-descriptions.md) - [google/eng-practices — Small CLs](https://github.com/google/eng-practices/blob/master/review/developer/small-cls.md) - [google/eng-practices — Speed of Code Reviews](https://github.com/google/eng-practices/blob/master/review/reviewer/speed.md) - [vercel-labs/skills CLI](https://github.com/vercel-labs/skills) - Courtney's reviewer agents for comparison: `~/.claude/agents/wp-security-reviewer.md`, `wp-a11y-reviewer.md`, `docs-reviewer.md`; global rules in `~/.claude/CLAUDE.md` ("Agent code-analysis / audit tasks") ## 🗣️ Paraphrase - Review every change before it merges, on five axes, and approve it when it makes the codebase better even if you'd have written it differently. Read the tests first, then the code. Label each finding so the author knows what's required and what's a preference, and put the one structural problem above the ten nits. - Keep changes around a hundred lines, split anything near a thousand, and don't mix refactoring with features. Write a first line someone can search for. Answer reviews within a day. When you and the author disagree, facts beat opinions and the style guide beats both. - When you flag structure, name the fix. Don't leave dead code, but ask before deleting. Treat every dependency, including upgrades, as a code change with a changelog to read and a lockfile diff to review. Don't rubber-stamp, don't soften, and don't trust AI-written code more because it reads well. ## 📰 Newsletter - **Review doctrine for agents, in one file.** Addy Osmani's `agent-skills` pack ships a `code-review-and-quality` skill that gives Claude Code, Codex, or Cursor a five-axis review with severity labels and change-size limits. The good parts are the severity scheme (no prefix = required) and the closing "presumptive blockers" list. Install: `npx skills add addyosmani/agent-skills --skill code-review-and-quality`. - **Read the fine print.** Much of the doctrine (FizzBuzz first lines, one-business-day turnaround, stacking/horizontal/vertical splits) is Google's Engineering Practices guide, CC-BY 3.0, unattributed. And the security axis is eight generic questions with `npm audit` as its only tool. WordPress reviewers still need a Security Trinity persona; this file doesn't know what a nonce is. - **One line to keep:** "AI code needs more scrutiny, not less. It's confident and plausible, even when wrong." ## ✅ Action Items #### DevRel - Developer advocacy: a short talk or post, "Three moves to steal from agent-skills' review checklist, and the one thing it can't do for WordPress." Measurable outcome: one reusable severity-label convention adopted in a plugin repo's CONTRIBUTING. - Education: pair the severity table with the WordPress plugin review team's own feedback categories in a workshop exercise; students label ten real review comments. - Developer experience: publish a `CONTRIBUTING.md` snippet with the no-prefix/Critical/Nit scheme for owned plugins. - Developer marketing: none; this is a tooling note. - Community management: n/a. - Internal/external/cross-project: file a courteous issue on `addyosmani/agent-skills` asking for attribution of the Google eng-practices passages (CC-BY 3.0 requires it). Link the four source files. #### OSPO - Policy & Governance: n/a. - License Compliance: the attribution gap is the OSPO item. Before adopting any skill pack as an internal standard, run a derivation check against known CC-BY sources; add "attribution present for derived text" to the pack-adoption checklist. - Security Management: the pack's security axis defers to `security-and-hardening`; do not treat installing this skill as satisfying a security-review requirement. - Software Inventory: track skill packs as dependencies with the same "last commit, open issues, license" questions this very file prescribes for npm packages. - Developer Support: n/a. - Community Engagement: n/a. - Strategic Alignment: n/a. #### Community - Maintainers: adopt "one structural problem and ten nits: the structural problem is the review" as a written norm; it cuts review noise on volunteer time. - Contributors: the change-description rules (imperative first line, no "Fix bug") are a good "first PR" checklist item. - Sponsors: n/a. - Ecosystem: n/a. #### WordPress - Core: the disagreement hierarchy (facts → style guide → engineering principles → consistency) maps onto Trac ticket disputes; worth a Make/Core handbook comparison. - Core Performance: the six performance questions are web-app generic (re-renders, pagination); none reference WP_Query, object cache, or autoloaded options. - Core AI: this file is what a lot of agents will now use to review WordPress code. Its security axis has no nonce, capability, or escaping concept. Core AI could publish a WordPress-specific review skill in the same format; the gap is concrete. - **Core Program Team:** n/a. - Mobile: n/a. - Playground: n/a. - Community: n/a. - Training: a lesson on review severity labels using this table as the worked example. - Marketing: n/a. - Support: n/a. - Design: n/a. - Documentation: the "change descriptions stand alone in version control history" rule applies to Trac commit messages; check the Core handbook's commit-message guidance against it. - Photos / TV / Polyglots / Openverse / Themes: n/a. - Accessibility: absent from all five axes. A review skill with no accessibility axis will approve UI changes that fail WCAG 2.2 AA. - Meta: n/a. - CLI: n/a. - Hosting: n/a. - Tide: the "Verification" section's checklist shape could feed Tide's scoring transparency work. - Plugins: the dependency-review discipline (changelog, one upgrade per change, lockfile diff) is directly applicable to plugin vendored libraries; the plugin review team's guidance could cite it. #### The WPOCC - Programs: n/a. - Projects: a governance clinic exercise: rewrite this checklist for a WordPress plugin release and count what had to change. - Funding experiments: n/a. ### FAIR - Technical independence: n/a. - Policies and covenants: n/a. - Productized governance artifacts: the rationalization table format (excuse → reality) is a reusable shape for any governance FAQ. #### Personal (Courtney) - **Compare against your three reviewer agents.** `wp-security-reviewer` has nine named failure modes in priority order and a stance ("every input is hostile until sanitized"); this file's security axis has eight questions and no stance. `wp-a11y-reviewer` covers an axis this file lacks entirely. `docs-reviewer` has an output contract; this file has a checklist. Your agents are narrower and deeper. What this file has that yours don't: an explicit severity vocabulary and the "presumptive blockers" list. Steal those two into the agents' output contracts. - **Adopt the severity prefixes** in the review persona outputs: Critical / (required, no prefix) / Optional / Nit / FYI. Your `code-review` skill already returns findings ranked most-severe first; the prefixes make the rank legible to a human reading a PR comment. - **The Google derivation is a small, checkable finding.** Four source files, one license. Filing the attribution issue is a five-minute contribution to a 91k-star repo and a real one. - **Your CLAUDE.md audit rules are stricter than this file** on the point that matters most for agents: "A finding is a claim until something runs" and "force empirical reproduction in a clean environment." This file's Step 5 checks the author's verification story; it never tells the reviewer to run anything. Keep yours. - Don't install the whole 25-skill pack on the strength of this one; the file's own dependency rule applies ("Does the existing stack solve this? Often it does."). ## 🧪 Hypotheses - H-A: Agents given the severity-prefix table produce fewer total review comments and a higher share of accepted comments than agents given a checklist without labels. - H-B: "Review tests first" ordering changes which defects an agent finds (more spec mismatches, fewer style nits) versus code-first ordering. - H-C: A stack-specific reviewer persona (e.g., `wp-security-reviewer`) finds more confirmed vulnerabilities in WordPress diffs than this generic five-axis skill, at equal token budget. - H-D: The ~100/300/1000 changed-line thresholds do not predict review defect-detection rate in repos under 50k lines; the ~1000-total-lines-per-file signal does. - H-E: Two different models in the multi-model pattern find no more distinct defects than one model run twice with different personas. - H-F: Skill packs with high star counts are adopted without derivation or license checks in most installs (this file's Google passages went unattributed through 14 commits and 91k stars). ### 📊 Methodology - H-A/H-B: take 60 real PRs with known post-merge defects; run the agent under four prompt conditions (labels on/off × tests-first on/off); measure comments per PR, precision (accepted/total), and recall of the known defects. - H-C: 40 WordPress plugin diffs seeded with Security Trinity defects (unprepared SQL, missing capability check, unescaped output, `__return_true` permission callback); compare confirmed findings between this skill and `wp-security-reviewer` at matched effort. - H-D: mine 20 open-source repos of varying size for post-merge bug-fix commits; regress defect probability on changed-lines and on file total size. - H-E: same 60 PRs; condition 1 = Model A then Model B; condition 2 = Model A with persona P1 then persona P2; count distinct confirmed defects. - H-F: sample 30 popular skill/prompt repos; run a phrase-level diff against google/eng-practices, OWASP, and MDN; record attribution presence. ### ❓ Questions - Does "one business day" have any meaning when the reviewer is an agent, or should the norm become "before the author's next commit"? - Who "makes the final call" when the multi-model pattern runs unattended in CI, and where is that recorded? - Why does a file that asks permission before deleting dead code not ask permission before approving a merge? - What would the security axis look like if it named prompt injection through "external data sources" explicitly? - Will the author add Google attribution if asked, and does the MIT license on the repo conflict with CC-BY 3.0 on the derived text? ### 🔮 Future Research - A WordPress-specific review skill in the same SKILL.md format, with the Security Trinity, nonce/capability checks, escaping at output, and an accessibility axis. The gap is concrete and the format is now standard. - Empirical work on review-comment severity labels and author response time in agent-authored PRs; none exists that this file cites. - Attribution tooling for prompt/skill repos: a phrase-level scanner against known CC-BY engineering docs. ### 💡 Implications - Practice: teams installing this pack inherit Google's review culture (fast turnaround, small changes, no deferred cleanup) whether or not that culture fits a volunteer or agency context. Volunteer WordPress projects should keep the sizing and severity rules and drop the business-day norm. - Policy: skill packs are dependencies. The file's own "every dependency is a liability" applies to itself. - Security: a generic five-axis review is not a security review. Plugin authors who believe otherwise will ship the "almost right" tax (sanitized but not escaped) with a green checklist. - Education: the severity table and the rationalization table are teachable in one session; the structural-remedies list is a semester. - Funding: none. ### 🧠 Learning Styles Assessment - Reading level estimate: Flesch-Kincaid grade ~10–11 by inspection (short imperative sentences, technical nouns, frequent bold leads, tables and fenced blocks). Written for practitioners. #### 👀 Visual learners - Present: three tables (splitting strategies, severity prefixes, rationalizations), one ASCII flow diagram (multi-model), several fenced checklists. - Improve: a before/after diff for one "structural remedy"; a diagram of the five-step process. #### 🔊 Aural learners - Present: nothing. - Improve: a three-minute walkthrough of one review using the severity prefixes aloud. #### 🗣️ Verbal learners - Present: bold lead-ins, named smells, the excuse→reality table. - Improve: a glossary (dispatcher, pass-through wrapper, presumptive blocker); worked review comments. #### 🧩 Kinesthetic learners - Present: the fill-in checklist; the dead-code prompt block; the install command. - Improve: a sample PR to practice on with an answer key. #### 🧠 Logical learners - Present: severity hierarchy, disagreement hierarchy, size thresholds, five-rule upgrade workflow. - Improve: decision tree from "diff size and file size" to "split / approve / request changes." #### 🧑‍🤝‍🧑 Social learners - Present: "comment on code, not people"; multi-model pattern; "ask before deleting." - Improve: a pairing exercise where one person plays author and one reviewer with the labels. #### 🧘‍♀️ Solitary learners - Present: the self-check checklist and the rationalization table. - Improve: a journaling prompt: "Which rationalization did I use this week?" #### 🌿 Naturalistic learners - Present: "Every dependency is a liability"; lifecycle language around upgrades. - Improve: tie change size to review energy and sustainable pace. ### 📊 Accessibility Assessment #### Screen reader semantics - Present: markdown headings in a consistent hierarchy; tables with header rows; code blocks. - Improve: the change-size table is a fenced code block, so it reads as one preformatted blob; make it a real table. The ASCII flow diagram has no text alternative. #### Keyboard navigation - Present: n/a (a markdown file; the catalog page is a static site). - Improve: n/a. #### Contrast and readability - Present: unknown for the catalog page (not assessed here); the raw file is plain text. - Improve: n/a. #### Text resizing and reflow - Present: markdown reflows. - Improve: the wide splitting-strategies table may overflow at 200% zoom on the rendered site. #### Media and captions - Present: no media. - Improve: n/a. #### Forms and interactions - Present: none. - Improve: n/a. #### Standards reference - WCAG 2.2 AA 1.1.1 (text alternative for the ASCII diagram), 1.3.1 (table semantics for the sizing block). Note the file's own review axes contain no accessibility check at all. ### 🌍 DEIB Assessment #### Cultural breadth - Present: none; examples are US big-tech (FizzBuzz RPC, npm). - Improve: examples from non-JS ecosystems; acknowledge the Google origin. #### Inclusive language - Present: "comment on code, not people"; gender-neutral throughout. - Improve: define "nit," "LGTM," "CL"-derived terms for newcomers. #### Neurodiversity support - Present: consistent section shape, bold leads, tables, short bullets. - Improve: the file is long (3,262 words); a one-screen summary at the top would help. #### Representation - Present: single author, single tradition. - Improve: credit the source guide; invite maintainers of other review cultures (volunteer OSS, regulated industries) to contribute variants. #### Psychological safety - Present: "Accept override gracefully," "Don't soften real issues," explicit severity so authors aren't guessing. - Improve: say who arbitrates when the "human final call" is absent in automated pipelines. ### ✅ Logical Validity #### Strong reasoning - The approval standard (Overview) is a clean decision rule with a stated rationale ("Perfect code doesn't exist"). - "Count the concepts a reader must hold" (Architecture) gives an operational test for "cleaner," which most review guidance lacks. - The dependency-upgrade section reasons from a mechanism: bulk bumps hide which package broke the build, so one per change. Valid and specific. - The rationalization table pairs each excuse with a consequence rather than a counter-assertion. #### Fallacies - Appeal to unstated authority: thresholds and norms presented as findings ("Experience shows deferred cleanup rarely happens") with no source. Fix: cite Google's guide and say the numbers are its. - Hasty generalization: "different models have different blind spots" is offered as sufficient justification for a two-model workflow. Fix: name one measured case or mark it as a hypothesis. - Internal tension: "Every change gets reviewed before merge — no exceptions" versus "When large changes are acceptable" and "Small cleanups… can be included at reviewer discretion." Not a contradiction, but the "no exceptions" is rhetorical. #### Tone of voice - Imperative, confident, checklist-driven. Bold lead sentences carry rules; the rationalization table is mildly combative ("Later never comes"). No hedging anywhere, which is a feature for a workflow document and a defect for its empirical-sounding claims. ### 🛡️ DARVO Analysis - Deny: none. No accusation is being answered. - Attack: none directed at people. The rationalization table attacks excuses, not their holders, and the honesty section explicitly says "comment on code, not people." - Reverse victim/offender: none. - Related rhetoric: the only reversal in the file is against the reader's own reasoning ("AI-generated code is probably fine" → "needs more scrutiny"). That's argument, not DARVO. Not applicable to this source beyond that. ## 🎯 Conclusion - The doctrine is Google's, the packaging is Osmani's, and both are good. Take the severity prefixes, the ordering rule, the "count the concepts" test, and the presumptive-blockers list. - Don't take it as a security or accessibility review. It has no axis for the second and only a syllabus for the first. - Next steps for readers: adopt the severity vocabulary in your review persona's output contract; keep your stack-specific reviewers; file the attribution issue. ## 🔄 Refactor Appendix — Code Review and Quality — agent-skills (SKILL.md) ### 📚 Literature Atoms (H3) #### [H1] Approve when the change definitely improves code health > [!quote] ID:rw_01m1kqqnazak6540fmcs4s0vhr > "Approve a change when it definitely improves overall code health, even if it isn't perfect. Perfect code doesn't exist — the goal is continuous improvement." Directional test replaces preference test. Google's standard, restated for agents. Overview section. Tags: `#atom/literature/h1` #### [H2] Watch file size, not just diff size > [!quote] ID:rw_01m1kqqnazak6540fmcs4s0vhr > "A small diff can still push a file past a healthy boundary — around 1000 *total* lines in a single file … is a common inspection signal, not a hard cap." Two thresholds, two failure modes; the second catches the slow-growth case the first misses. Change Sizing. Tags: `#atom/literature/h2` #### [H3] One structural problem outranks ten nits > [!quote] ID:rw_01m1kqqnazak6540fmcs4s0vhr > "Don't bury a real issue under cosmetic nits — a few high-conviction comments beat a long list. If you have one structural problem and ten nits, the structural problem *is* the review." Ordering rule for review output; the same rule Courtney's `code-review` skill enforces by ranking. Step 4. Tags: `#atom/literature/h3` #### [H4] Deferred cleanup rarely happens; require it before merge > [!quote] ID:rw_01m1kqqnazak6540fmcs4s0vhr > "Experience shows deferred cleanup rarely happens. Require cleanup before submission unless it's a genuine emergency. If surrounding issues can't be addressed in this change, require filing a bug with self-assignment." Asserted from "experience," no source; matches Google's guide. The self-assigned bug is the enforceable half. Handling Disagreements. Tags: `#atom/literature/h4` #### [H5] Presumptive blockers: relocated complexity, size growth, shared-module leakage > [!quote] ID:rw_01m1kqqnazak6540fmcs4s0vhr > "a refactor that relocates complexity instead of reducing it; a change that pushes a file past the size boundary with no decomposition; feature logic added to a shared module; a near-duplicate of an existing canonical helper; a silent fallback that hides an unclear invariant." Five named smells that escalate to Required only when structure gets worse. The file's own best compression. Verification. Tags: `#atom/literature/h5` ### 🧠 Permanent Candidates (H4) #### [P1] Severity labels make review rank legible; "no prefix means required" is the load-bearing rule **Claim** (Courtney's voice): A review output that ranks findings is only useful to a human if the rank is written on each finding, and the cheapest legible scheme is a prefix set where the absence of a prefix means required. **Evidence pointer**: [[#H3]] **Scope & conditions**: Holds for PR comments read by authors who didn't watch the review run. Does not replace ranking; complements it. Fails if the team lets "Nit" become a dumping ground for real issues. **Links**: - [[Code Review Personas]] — because applies-in the output contract of `wp-security-reviewer`, `wp-a11y-reviewer`, `docs-reviewer` - [[Maker Checker Loop]] — because extends the checker's report format - [[AI Tooling MOC]] — because applies-in <!-- atomicity: 1 idea ✓ --> #### [P2] A generic multi-axis review is a syllabus for security, not a security review **Claim** (Courtney's voice): A review skill whose security axis is a list of questions with no stack-specific failure modes will pass "almost right" code, so it cannot substitute for a reviewer persona with a stance. **Evidence pointer**: [[#H5]] (structural axis is deep; security axis defers) **Scope & conditions**: Holds wherever the stack has a known defect class the generic questions don't name (WordPress: nonce ≠ authorization, escaping at output). Counterexample: a codebase whose security surface is fully generic (pure CLI tool with no I/O). **Links**: - [[WordPress Security Trinity]] — because contrasts on specificity - [[Wp2shell Audit Method]] — because extends: reproduction, not checklist, is the gate - [[Core Security Initiative]] — because applies-in the AI-driven report volume context <!-- atomicity: 1 idea ✓ --> #### [P3] Skill packs are dependencies and inherit the dependency rules they prescribe **Claim** (Courtney's voice): A prompt or skill pack installed into an agent is a dependency, so the questions its own text prescribes for npm packages (maintained, licensed, attributed, does the existing stack already do this) apply to the pack before installation. **Evidence pointer**: [[#H4]]; the unattributed Google eng-practices passages under CC-BY 3.0 **Scope & conditions**: Holds for any installed instruction set that changes agent behavior. The attribution check is cheap; the "does the existing stack solve this" check is the one that usually says no. **Links**: - [[Slop Squatting and Package Verification]] — because extends the CLAUDE.md verify-before-install rule to prompts - [[Instruction Decay Across Model Versions]] — because contrasts: decay is the temporal risk, provenance is the origin risk - [[Credit Operating Model]] — because applies-in routing decisions about which skills to load <!-- atomicity: 1 idea ✓ --> ### ✏️ Fleeting Queue - [F1] File the attribution issue on addyosmani/agent-skills: four Google eng-practices files, CC-BY 3.0, zero mentions. — https://github.com/addyosmani/agent-skills/issues #fleeting #inbox - [F2] Add Critical / Required / Optional / Nit / FYI prefixes to the three reviewer agents' output contracts. — `~/.claude/agents/*.md` #fleeting #inbox - [F3] "Count the concepts a reader must hold" as a one-line test for whether a refactor helped; try it on the next `simplify` run. #fleeting #inbox - [F4] Reader's v3/save with the same URL created a new doc (201) instead of updating the 84-word one; the dedupe-by-URL assumption in the fork brief is wrong for re-saves with html. #fleeting #inbox ### → Things 3 (max 2 actions) - [ ] Open an attribution issue on addyosmani/agent-skills citing google/eng-practices standard.md, cl-descriptions.md, small-cls.md, speed.md and the CC-BY 3.0 LICENSE — 5 minutes, real contribution #things - [ ] Add severity prefixes (Critical / required / Optional / Nit / FYI) to the output contract in wp-security-reviewer, wp-a11y-reviewer, docs-reviewer — one edit each #things <!-- deep-read:sections end --> 📄 **Full text:** [[Full Document Contents/Articles/Code Review and Quality — Agent-Skills.md|Code Review and Quality — Agent-Skills by Addy Osmani]] ## Highlights - **The approval standard:** Approve a change when it definitely improves overall code health, even if it isn't perfect. Perfect code doesn't exist — the goal is continuous improvement. Don't block a change because it isn't exactly how you would have written it. If it improves the codebase and follows the project's conventions, approve it. ([View Highlight](https://read.readwise.io/read/01m1kr1gyys5w8dacs7av68c56)) - Note: H1 — Approve when the change definitely improves code health (Google's standard, restated for agents) #atom/literature/h1 - Tags: atom/literature/h1 - **Watch file size, not just diff size.** A small diff can still push a file past a healthy boundary — around 1000 \*total\* lines in a single file (distinct from the ~1000 \*changed\*-lines threshold above) is a common inspection signal, not a hard cap. When a change materially grows an already-large file, ask whether to extract helpers, subcomponents, or modules \*first\*, before piling more on. Decompose, then add. ([View Highlight](https://read.readwise.io/read/01m1kr1kqnvp4c767ztb07dgyz)) - Note: H2 — Watch file size, not just diff size: two thresholds for two failure modes #atom/literature/h2 - Tags: atom/literature/h2 - **Lead with what matters.** Order findings by leverage: correctness and security first, then structural regressions and missed simplifications, then everything else. Don't bury a real issue under cosmetic nits — a few high-conviction comments beat a long list. If you have one structural problem and ten nits, the structural problem \*is\* the review. ([View Highlight](https://read.readwise.io/read/01m1kr1ppz23wybm29kfj6zf5m)) - Note: H3 — One structural problem outranks ten nits; the ordering rule for review output #atom/literature/h3 - Tags: atom/literature/h3 - **Don't accept "I'll clean it up later."** Experience shows deferred cleanup rarely happens. Require cleanup before submission unless it's a genuine emergency. If surrounding issues can't be addressed in this change, require filing a bug with self-assignment. ([View Highlight](https://read.readwise.io/read/01m1kr1sv84s1qkpa4rr11hvex)) - Note: H4 — Deferred cleanup rarely happens; require it before merge or a self-assigned bug (asserted from "experience," no source; matches Google's guide) #atom/literature/h4 - Tags: atom/literature/h4 - **Presumptive blockers:** surface and propose the simpler design for each of these; escalate to Required only when the change actively makes structure worse: a refactor that relocates complexity instead of reducing it; a change that pushes a file past the size boundary with no decomposition; feature logic added to a shared module; a near-duplicate of an existing canonical helper; a silent fallback that hides an unclear invariant. ([View Highlight](https://read.readwise.io/read/01m1kr1z59vc52jtv7jq133x8v)) - Note: H5 — Presumptive blockers: five named structural smells, escalated only when structure gets worse #atom/literature/h5 - Tags: atom/literature/h5