diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 593ed5a..c86d13f 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-marketplace.json", "name": "clean-code-toolkit", - "version": "3.4.0", + "version": "3.5.0", "description": "Clean-code and product-handoff tools for AI-assisted builders.", "owner": { "name": "Tarik Moody" @@ -10,7 +10,7 @@ { "name": "clean-code-toolkit", "description": "Review code, assess product readiness, refactor safely, and prepare a developer handoff.", - "version": "3.4.0", + "version": "3.5.0", "author": { "name": "Tarik Moody" }, diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 02062a8..68e8bee 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "clean-code-toolkit", - "version": "3.4.0", + "version": "3.5.0", "description": "Practical clean-code, product-readiness, and developer-handoff workflows for AI-assisted projects.", "author": { "name": "Tarik Moody" diff --git a/CHANGELOG.md b/CHANGELOG.md index 9de6863..04fb936 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,31 @@ # Changelog +## 3.5.0 + +3.4.0 made the engine honest about what it found. This release is about what it was never looking at, and about the toolkit holding together as one thing. + +**Access Control, a new category. The tool shipped three versions with no authentication check at all.** +- `auth-1`: is there an authentication mechanism, and which one. +- `auth-2`: do request handlers consult an identity, or serve anyone who reaches the URL. Reported as a text match, because mentioning a guard is not the same as being guarded. +- `auth-3`: does a permission check grant access when its own environment variable is unset. Real code rarely tests the variable inline, so the check learns the local names first, then looks for the fail-open branch on them. +- Why it matters: this was written the same afternoon an independent review of a live application found a send endpoint reachable by any signed-in user and an owner check returning true when its variable was unset. The audit had graded that repository "A, strong evidence of controls". It now quotes the exact line and grades it D. + +**Warnings cost points.** Eight checks reported as `warn` with a severity badge and subtracted nothing, so a repository could display three MEDIUM findings and still score 100 in those categories: 55 points shown but not counted. A warning now costs half of the same finding failing. Less certain, never free. + +**The installer can deliver a correction.** `/add-clean-code` printed "already installed" and exited 0 without comparing anything, so the going-live rule corrected in 3.2.1 could never reach anyone who installed before it. The managed block now carries a version; a re-run diffs it and exits 3 when an update is available; `--update` replaces only the block between the markers; `--check` reports without touching anything. The dangling "that day" left in the template by the 3.2.1 edit is fixed. + +**The editing skill now says what its verification is worth.** `boy-scout-cleanup` claimed "behavior-preserving" while `/refactor`, its sibling with the same risk, carried the discipline that makes such a claim meaningful. A mutation test showed three of four real behavior changes passing a green suite. The skill now requires an undo to exist, requires checking that the suite covers the behavior being touched, and requires saying which evidence was used. + +**Every skill was run once against a real repository by an independent tester, and every one came back "partly works".** +- `clean-code-scaffold` invented a stack on an empty directory without saying it was guessing, and produced a scaffold that failed on first run. It now asks when there is nothing to detect, and runs what it built the way its own README says to. +- `developer-handoff` scoped verification to setup commands, and its template's empty sections pulled toward filling them. It now marks each statement as ran it, read it, or told to me, and deletes sections with no referent. +- `product-readiness-review` promised in its description to run the coach first and never mentioned it in the workflow, called builds "safe" when they write into the repository, and judged against a milestone it never asked for. All three fixed. +- `clean-code-review` never said where the review goes. + +**House style.** Em dashes are gone from every skill, command and template, so the report linter no longer rejects text this toolkit ships. + +Tests: 68. + ## 3.4.0 An audit of the auditor. A deliberately hollow repo, every control a costume, scored 78 out of 100 with nine passing checks, five of them false. Fixing that exposed more. diff --git a/README.md b/README.md index 6c805d7..53af7c2 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,7 @@ You do not need to remember tool names. Ask what you want and Claude picks the t | You want to... | Say this | What happens | |---|---|---| -| Know what is missing before going live | "Audit this repo for production readiness" | A script scans for the things that bite you in production: no automatic test runner, no error alerts, secrets in the code, no undo plan. You get a plain-English report that explains each gap, and a step-by-step fix list you can hand back to Claude. Writes two report files to a scratch folder, not your repo, unless you ask. | +| Know what is missing before going live | "Audit this repo for production readiness" | A script scans for the things that bite you in production: no automatic test runner, no error alerts, secrets in the code, a page anyone can reach that should be locked, no undo plan. You get a plain-English report that explains each gap, and a step-by-step fix list you can hand back to Claude. Writes two report files to a scratch folder, not your repo, unless you ask. | | Know if the product actually works for users | "Is this product ready to launch?" | Claude reads the app as a whole, not file by file. Does it do what it claims? What would a real user hit first? Changes nothing. | | Understand the quality of your code | "Review this code in plain English" | A report on what is unclear, untested, or fragile. Changes nothing. | | Get a quick cleanup list | `/code-smells` | A short list of small things worth a look. Changes nothing. | @@ -78,6 +78,10 @@ So the audit takes waivers. Add `.prod-audit-waivers.json` to your repo: Every field is required. A waived check stops counting toward the score and the exit code, but it appears in every report with its reason, its evidence, and the name of the person who accepted it. Waivers expire after 180 days, and then the finding comes back. Claude will never write this file for you. It shows you the entry and you decide. +## What it does not check + +It reads files, so it can tell you a control is missing or that a guard lets everyone through when a setting is absent. It cannot tell you your permissions model is correct, that your business rules are right, or that the app works. The access-control checks are deliberately shallow and say so in their own wording. A clean run is a starting point for a human, never a security review. + ## The two readiness tools, and when to use which Both ask "is it ready?" They answer different halves. diff --git a/commands/code-smells.md b/commands/code-smells.md index 79bbd14..6dee95e 100644 --- a/commands/code-smells.md +++ b/commands/code-smells.md @@ -7,7 +7,7 @@ Perform a read-only smell scan on the file, directory, or pasted code the user i 1. Read repository guidance and detect the language, framework, and configured linter or analyzer. 2. Run the narrowest safe configured tool when practical. Do not install dependencies or rewrite files. 3. Inspect for duplicated business rules, unrelated responsibilities, confusing interfaces, deep nesting, misleading names, dead code, unexplained domain values, hidden side effects, and speculative abstractions. -4. Treat counts such as 20 lines, four parameters, or three nesting levels as prompts to inspect context—not automatic failures. +4. Treat counts such as 20 lines, four parameters, or three nesting levels as prompts to inspect context, not automatic failures. 5. Report only actionable findings with file, line, evidence, impact, and a concrete recommendation. 6. Separate deterministic tool output from judgment-based findings and avoid duplicating the same issue. 7. If no linter is configured, note it as a tooling opportunity only when a linter would materially help this project. Do not automatically rank it above code defects. diff --git a/scripts/add-clean-code.sh b/scripts/add-clean-code.sh index c291fad..2f1128b 100755 --- a/scripts/add-clean-code.sh +++ b/scripts/add-clean-code.sh @@ -1,36 +1,92 @@ #!/usr/bin/env bash +# +# Install or update the Clean Code Standards block in a project's CLAUDE.md. +# +# add-clean-code.sh [target-dir] [--update] [--check] +# +# The block is delimited by BEGIN/END markers and carries a version, so an +# installed copy can be compared with the shipped one. Without that, a +# correction shipped in a later release could never reach anyone who had +# already installed, which is exactly what happened between v3.1.0 and v3.2.1. +# +# (no flag) install if absent; if present but out of date, say so and exit 3 +# --check report only, change nothing (exit 3 if an update is available) +# --update replace the managed block in place, leaving everything else alone set -euo pipefail -target_dir="${1:-.}" +target_dir="." +mode="install" +for arg in "$@"; do + case "${arg}" in + --update) mode="update" ;; + --check) mode="check" ;; + --*) echo "Unknown option: ${arg}" >&2; exit 64 ;; + *) target_dir="${arg}" ;; + esac +done + script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" template_file="${script_dir}/../templates/CLAUDE.md" target_file="${target_dir}/CLAUDE.md" -managed_marker="" +begin_prefix="" legacy_heading="# Clean Code Standards" temporary_file="" -cleanup() { - if [[ -n "${temporary_file}" && -f "${temporary_file}" ]]; then - rm -f "${temporary_file}" - fi -} - +cleanup() { [[ -n "${temporary_file}" && -f "${temporary_file}" ]] && rm -f "${temporary_file}"; return 0; } trap cleanup EXIT -if [[ ! -d "${target_dir}" ]]; then - echo "Directory not found: ${target_dir}" >&2 - exit 1 -fi +[[ -d "${target_dir}" ]] || { echo "Directory not found: ${target_dir}" >&2; exit 1; } +[[ -f "${template_file}" ]] || { echo "Template not found: ${template_file}" >&2; exit 1; } -if [[ ! -f "${template_file}" ]]; then - echo "Template not found: ${template_file}" >&2 - exit 1 -fi +shipped_version="$(grep -m1 -o "${begin_prefix}[^>]*" "${template_file}" | sed "s|${begin_prefix} *||; s| *-*$||")" +shipped_version="${shipped_version:-unversioned}" + +write_atomically() { # $1 = content-producing command writing to stdout + temporary_file="$(mktemp "${target_file}.tmp.XXXXXX")" + "$@" > "${temporary_file}" + mv "${temporary_file}" "${target_file}" + temporary_file="" +} + +strip_leading_blank() { awk 'NR == 1 && $0 == "" { next } { print }' "${template_file}"; } if [[ -f "${target_file}" ]]; then - if grep -qF "${managed_marker}" "${target_file}"; then - echo "Clean Code Standards are already installed in ${target_file}." + if grep -qF "${begin_prefix}" "${target_file}"; then + installed_version="$(grep -m1 -o "${begin_prefix}[^>]*" "${target_file}" | sed "s|${begin_prefix} *||; s| *-*$||")" + installed_version="${installed_version:-unversioned}" + + if [[ "${installed_version}" == "${shipped_version}" ]] && \ + diff -q <(sed -n "/${begin_prefix}/,/${end_marker}/p" "${target_file}") \ + <(sed -n "/${begin_prefix}/,/${end_marker}/p" "${template_file}") >/dev/null; then + echo "Clean Code Standards are up to date in ${target_file} (${installed_version})." + exit 0 + fi + + if [[ "${mode}" != "update" ]]; then + echo "An update is available for ${target_file}." + echo " installed: ${installed_version}" + echo " shipped: ${shipped_version}" + echo "What would change inside the managed block:" + diff <(sed -n "/${begin_prefix}/,/${end_marker}/p" "${target_file}") \ + <(sed -n "/${begin_prefix}/,/${end_marker}/p" "${template_file}") || true + echo "Re-run with --update to replace the managed block. Nothing outside it is touched." + exit 3 + fi + + if ! grep -qF "${end_marker}" "${target_file}"; then + echo "The managed block in ${target_file} has no END marker, so its boundary is unclear." >&2 + echo "Fix it by hand rather than risk overwriting your own instructions." >&2 + exit 2 + fi + + write_atomically awk -v begin="${begin_prefix}" -v endm="${end_marker}" -v tpl="${template_file}" ' + index($0, begin) == 1 { inblock = 1; while ((getline line < tpl) > 0) print line; close(tpl); next } + inblock && index($0, endm) == 1 { inblock = 0; next } + !inblock { print } + ' "${target_file}" + echo "Updated Clean Code Standards in ${target_file} (${installed_version} -> ${shipped_version})." exit 0 fi @@ -40,20 +96,19 @@ if [[ -f "${target_file}" ]]; then exit 2 fi + [[ "${mode}" == "check" ]] && { echo "Not installed in ${target_file}."; exit 3; } + temporary_file="$(mktemp "${target_file}.tmp.XXXXXX")" cp -p "${target_file}" "${temporary_file}" printf '\n\n' >> "${temporary_file}" - awk 'NR == 1 && $0 == "" { next } { print }' "${template_file}" >> "${temporary_file}" + strip_leading_blank >> "${temporary_file}" mv "${temporary_file}" "${target_file}" temporary_file="" action="Appended" else - temporary_file="$(mktemp "${target_file}.tmp.XXXXXX")" - awk 'NR == 1 && $0 == "" { next } { print }' "${template_file}" > "${temporary_file}" - mv "${temporary_file}" "${target_file}" - temporary_file="" + [[ "${mode}" == "check" ]] && { echo "No CLAUDE.md in ${target_dir}."; exit 3; } + write_atomically strip_leading_blank action="Created" fi -line_count="$(wc -l < "${target_file}" | tr -d ' ')" -echo "${action} Clean Code Standards in ${target_file} (${line_count} lines)." +echo "${action} Clean Code Standards in ${target_file} (${shipped_version}, $(wc -l < "${target_file}" | tr -d ' ') lines)." diff --git a/skills/boy-scout-cleanup/SKILL.md b/skills/boy-scout-cleanup/SKILL.md index cd60efb..6e7532c 100644 --- a/skills/boy-scout-cleanup/SKILL.md +++ b/skills/boy-scout-cleanup/SKILL.md @@ -13,10 +13,13 @@ Before editing: 1. Read repository guidance and inspect the working tree so user changes are preserved. 2. Identify the file's callers, exports, tests, and configured checks. -3. Decide what evidence can verify behavior: focused tests, type checks, lint, build, or careful call-site inspection. -4. If a proposed change could affect behavior and verification is weak, either add a characterization test with the user's approval or leave the change as a recommendation. +3. Confirm there is an undo. If the project is not under version control and has no backup, say so and stop: a cleanup you cannot reverse is not a cleanup. +4. Decide what evidence can verify behavior: focused tests, type checks, lint, build, or careful call-site inspection. +5. **A passing test suite is evidence only if it covers the behavior you are about to touch.** Check that it does. The cheap way: change the value you are about to extract, or invert the condition you are about to simplify, run the suite, and see whether anything fails. If the suite stays green while the behavior is different, it does not cover this code, and green afterwards will mean nothing. +6. When the suite does not cover it, pick one: propose a characterization test first and get approval, verify by comparing real output before and after (same input, same bytes), or leave the change as a recommendation and say why. +7. Say which of those you did. "Tests pass" without saying what they cover is the sentence this step exists to prevent. -No edit is literally zero-risk. Unused imports may have side effects, comments may preserve important context, and renames may cross public boundaries. Inspect before removing or renaming. +"Behavior-preserving" is a claim you are making, not a property the edits have. No edit is literally zero-risk. Unused imports may have side effects, comments may preserve important context, and renames may cross public boundaries. Inspect before removing or renaming. ## Good cleanup candidates @@ -24,7 +27,7 @@ No edit is literally zero-risk. Unused imports may have side effects, comments m - Remove proven unreachable code or a proven-unused import. - Reduce nesting while preserving the exact conditions and evaluation order. - Extract a domain value whose meaning is otherwise unclear. -- Use the project's formatter or import organizer. +- Use the project's formatter or import organizer. If none is configured, leave formatting alone: reflowing code by hand is a repository-wide style rewrite wearing a small diff. - Improve a misleading comment or delete one that demonstrably restates the code. ## Out of scope diff --git a/skills/clean-code-review/SKILL.md b/skills/clean-code-review/SKILL.md index 8b4990e..86cd745 100644 --- a/skills/clean-code-review/SKILL.md +++ b/skills/clean-code-review/SKILL.md @@ -45,6 +45,10 @@ For a beginner or vibe coder, explain each important finding in three short part Avoid unexplained acronyms and pattern-name trivia. Teach the decision, not the vocabulary. +## Where the review goes + +Report in chat by default. Write a file only if the user asks, and then to a path they name. Do not create files in their repository unannounced. + ## Boundaries - For a product-level assessment, use `product-readiness-review`. diff --git a/skills/clean-code-scaffold/SKILL.md b/skills/clean-code-scaffold/SKILL.md index f96c6bc..0a185f4 100644 --- a/skills/clean-code-scaffold/SKILL.md +++ b/skills/clean-code-scaffold/SKILL.md @@ -16,12 +16,14 @@ Do not impose `components/services/utils/tests` on every stack. Next.js, Django, ## Workflow 1. Determine the product type, language, framework, runtime, package manager, and deployment target from repository evidence. -2. Identify the product's primary domains or features and the code that changes together. -3. Preserve an established, coherent repository pattern unless the user explicitly requests a migration. -4. Propose the smallest structure that clarifies ownership and dependency direction. -5. For a new project, create the agreed scaffold and starter files. -6. For an existing project, show the proposed moves and risks before moving files or changing imports. -7. Run framework checks after implementation. +2. **When there is no evidence, say so and ask.** An empty directory has nothing to detect, so anything you pick comes from the user's sentence and your own habits, not from the repository. Name the stack you would choose, name the one real alternative, and get an answer before creating files. Never present a guess in the voice of a detection. +3. Identify the product's primary domains or features and the code that changes together. +4. Preserve an established, coherent repository pattern unless the user explicitly requests a migration. +5. Propose the smallest structure that clarifies ownership and dependency direction. +6. For a new project, propose the layout and the reason for it, wait for a yes, then create the scaffold and starter files. "Agreed" means the user said yes to this layout, not that they asked for a project. +7. For an existing project, show the proposed moves and risks before moving files or changing imports. +8. **Run it.** Not "checks": run the thing you just created, exactly as your own README tells the user to run it, from a clean shell. A scaffold that errors on first command is worse than no scaffold, and it is the most common way this step fails. Then run the tests you created, and the project's formatter, linter, or type checker if you configured one. +9. If the first run needs anything the README does not say, either fix the layout so it does not, or put the missing step in the README. A Python `src/` layout, for example, is not importable until the package is installed or `PYTHONPATH` is set; say which one you chose and why. ## Principles diff --git a/skills/developer-handoff/SKILL.md b/skills/developer-handoff/SKILL.md index 23fee12..0d4b5ed 100644 --- a/skills/developer-handoff/SKILL.md +++ b/skills/developer-handoff/SKILL.md @@ -11,12 +11,14 @@ Produce a factual map that lets an incoming developer become productive without 1. Read repository guidance and existing documentation before creating anything. 2. Detect the stack, entry points, package manager, environments, core product flows, domain modules, persistence, integrations, test commands, build, deployment, and CI. -3. Run safe commands needed to verify setup instructions when practical. Do not install, deploy, or mutate external systems without authorization. -4. Identify unknowns and stale documentation. Do not fill gaps with guesses. -5. Redact values for secrets, credentials, private URLs, tokens, and personal information. Document variable names and purpose only. -6. Follow the repository's documentation convention. If none exists, create `DEVELOPER_HANDOFF.md` at the project root using [references/handoff-template.md](references/handoff-template.md). -7. Keep the handoff concise and link to authoritative files instead of duplicating their contents. -8. Report what was verified, what remains unknown, and the highest-risk handoff gaps. +3. Verify by running, not by reading. Run the setup and test commands, and then go further: the defects that hurt an incoming developer are in the claims a README makes about *integrations*, not in its install line. Check that a service the docs name is the service actually called, that a documented cost or limit still matches, and that a documented capability exists. Do not install, deploy, or mutate external systems without authorization. +4. Mark every statement with where it came from: **ran it**, **read it in a file**, or **told to me**. A command transcribed from a README and never executed is "read it in a file". This single distinction is the difference between a handoff and a plausible story. +5. Identify unknowns and stale documentation. Do not fill gaps with guesses. +6. **Delete template sections that have no referent.** If there is no database, the "Data and integrations" heading must not survive with prose under it. An empty heading pulls you toward filling it, and that is how a handoff acquires fiction. Write "no database" once, or remove the section. +7. Redact values for secrets, credentials, private URLs, tokens, and personal information. Document variable names and purpose only. +8. Ask where it should go. If the repository has a `docs/` convention, offer that path; otherwise offer `DEVELOPER_HANDOFF.md` at the project root. Write outside the repository if the user prefers. Create `DEVELOPER_HANDOFF.md` at the project root by default using [references/handoff-template.md](references/handoff-template.md). +9. Keep the handoff concise and link to authoritative files instead of duplicating their contents. +10. Close with three lists, using the template's own section names: what was verified by running it, what remains unknown, and the known issues that carry the most risk for whoever picks this up. ## Quality bar diff --git a/skills/prod-readiness-coach/SKILL.md b/skills/prod-readiness-coach/SKILL.md index d54e7c0..b873fb0 100644 --- a/skills/prod-readiness-coach/SKILL.md +++ b/skills/prod-readiness-coach/SKILL.md @@ -10,7 +10,7 @@ description: "Audits any code repository for production-readiness gaps (CI/CD pi Use this whenever someone wants to know if their project is ready for production, or wants a code-quality/DevOps audit translated into language a self-taught or AI-assisted ("vibe coding") developer can actually learn -from — not just a checklist of jargon. Typical requests this matches: +from, not just a checklist of jargon. Typical requests this matches: - "Audit my repo for production readiness" - "Is my app ready to launch? What am I missing?" @@ -26,17 +26,17 @@ underlying concept, not just told to fix it. ## What This Skill Produces 1. **`AUDIT_PLAIN_ENGLISH.md`** (written to a scratch location unless the - user asks for it in the repo) — the findings explained in plain English: + user asks for it in the repo), the findings explained in plain English: what was checked, what was found, why it actually matters in practice, and a short teaching explanation of the underlying concept. No unexplained jargon. -2. **`FIX_BRIEF_FOR_CLAUDE_CODE.md`** — an execution-ready brief the user +2. **`FIX_BRIEF_FOR_CLAUDE_CODE.md`**, an execution-ready brief the user can paste into Claude Code (or any AI coding agent) inside their repo. Split into severity phases with explicit acceptance criteria and a mandatory re-audit gate before the next phase starts, plus a separate "manual steps for a human" list for anything an AI agent should not attempt unattended. -3. (Optional, on request) **`audit-report.json` / `audit-report.md`** — the +3. (Optional, on request) **`audit-report.json` / `audit-report.md`**, the raw technical report from the underlying tool, for users who want the unfiltered file-level evidence, best-practice citations, and detected stack fingerprint. @@ -62,16 +62,16 @@ underlying concept, not just told to fix it. Before running anything, ask (in your own words, one short question): *"If this app lost data or went down for a full day, what's actually at -stake — mostly an inconvenience, or something with real consequences +stake, mostly an inconvenience, or something with real consequences (money, safety, someone's only copy of something)?"* Skip this only if the user already volunteered enough context unprompted (e.g. "this handles patient intake data"), explicitly says to skip it, or -no user is present to answer (autonomous run) — in that case omit +no user is present to answer (autonomous run), in that case omit `--context` and say so in the audit's short version. -Store their answer verbatim — it gets passed to the audit tool via +Store their answer verbatim, it gets passed to the audit tool via `--context` and used in step 6 to decide which findings get emphasized in -the prose. **It never changes the deterministic score** — a `critical` +the prose. **It never changes the deterministic score**, a `critical` finding is `critical` regardless of context; the context only changes how hard you lean on it in the writeup (e.g. a "no backups documented" finding that's `medium` by default reads very differently once you know the app @@ -83,16 +83,16 @@ Ask (if not already clear from context): - Which repo? Accept a local path, a `git clone`-able URL, or (if a GitHub connector/CLI is available) an `owner/repo` slug. - If it's remote and not already checked out locally, clone it into the - workspace first (shallow clone is fine — `git clone --depth 1`). + workspace first (shallow clone is fine, `git clone --depth 1`). -Don't ask about output format or tone — plain English + Claude Code brief +Don't ask about output format or tone, plain English + Claude Code brief is this skill's whole point, always produce both unless the user explicitly says they only want one. ### 3. Run the audit tool The bundled script does all the technical detection AND the stack -fingerprinting — never hand-roll detection logic in prose; run the real +fingerprinting, never hand-roll detection logic in prose; run the real tool and translate its real output. ```bash @@ -116,17 +116,17 @@ a finding or a win. A detected framework or deploy surface implies rather than failing. In that case tell the user plainly that the scan could not determine the project type and ask: web app, API, worker, CLI, or library. Then re-run with `--profile`. Top-level fields that matter: -- `stack_fingerprint` — see step 4, this drives which adapter files to load. -- `product_context` — echoes back whatever you passed via `--context`. +- `stack_fingerprint`, see step 4, this drives which adapter files to load. +- `product_context`, echoes back whatever you passed via `--context`. - `categories` → each has `key`, `title`, `score`, and `checks`; each check has `id`, `title`, `status` (`pass`/`fail`/`warn`/`info`), `severity`, `detail`, `recommendation`, `evidence` (file paths/line snippets), `best_practice_ref` (a citation URL), and `confidence` (`verified` or `weak`). A `weak` pass - means the tool only found matching text — write it up as "looks like + means the tool only found matching text, write it up as "looks like this may exist; confirm by hand," never as a confirmed win. Each category also has `has_weak_evidence`: when true, that category is not a clean win no matter how high it scored. -- `contradictions` — pairs where one check passed while another failed in +- `contradictions`, pairs where one check passed while another failed in a way that undercuts it (a clean secret scan with no `.gitignore` guard; tests that no pipeline runs). **Write these up before the phases**, in their own short section, because a phase plan built on a pass that is @@ -136,7 +136,7 @@ or library. Then re-run with `--profile`. Top-level fields that matter: (an org-level pipeline, a platform dashboard, a vault). Waived checks have `status: "waived"` and `waived_from`, and they stop counting toward the score and the exit code. `problems` lists waivers that were rejected - (missing a field, bad date, expired) — those findings still count. + (missing a field, bad date, expired), those findings still count. Treat this JSON as the authoritative record of **what the scanner observed**, not as final truth. The scanner is pattern-based static @@ -160,7 +160,7 @@ analysis. Rules: ### 4. Load fingerprint-matched stack adapters -Read `stack_fingerprint.adapters_matched` from the JSON — a list of stack +Read `stack_fingerprint.adapters_matched` from the JSON, a list of stack names like `["nextjs-vercel", "convex"]`. For each name in that list, read `references/stacks/.md`. Available adapters: `nextjs-vercel`, `convex`, `fly`, `cloudflare-workers`, `netlify`. @@ -174,47 +174,47 @@ facts specifically in the **Multi-Surface Deployment & Coordinated Rollback** category writeup and anywhere else they contradict or refine a generic finding. -- If `adapters_matched` is empty, skip this step entirely — write the +- If `adapters_matched` is empty, skip this step entirely, write the generic findings as-is. Don't guess at platform facts that aren't in an adapter file or in the JSON evidence. - Never load an adapter that isn't in `adapters_matched` for this repo, - even if you recognize the stack from the file list — the fingerprint is + even if you recognize the stack from the file list, the fingerprint is the single source of truth for what applies here. - Each adapter starts with a `last_verified` date. If that date is more than six months before today, add one sentence wherever you use a fact from it: "this platform detail was last checked on and may have changed." Vendor rollback rules and setup wizards do change. -- Each adapter file also has an "Agentic-safety notes" section — fold +- Each adapter file also has an "Agentic-safety notes" section, fold those directly into the fix brief's phase gates and the manual-steps list in step 7, don't leave them as trivia. ### 5. Load the writing references Before drafting either document, read: -- `references/plain-english-glossary.md` — the term-by-term translation +- `references/plain-english-glossary.md`, the term-by-term translation table and tone rules. Use it for every technical term that appears in the JSON `detail`/`recommendation` fields, including the newer multi-surface/rollback/migration terms. -- `references/report-templates.md` — the exact structure for both output +- `references/report-templates.md`, the exact structure for both output documents, including the phase-gated fix-brief structure. ### 6. Write `AUDIT_PLAIN_ENGLISH.md` Follow the Document 1 template in `references/report-templates.md` exactly. Substance rules: -- Translate every finding through the glossary — a reader with zero DevOps +- Translate every finding through the glossary, a reader with zero DevOps background should understand every sentence without looking anything up. - Lead with what's already working, named specifically and warmly, before - the gaps — this is a coach, not an inspector. + the gaps, this is a coach, not an inspector. - Order gaps critical → high → medium → low. For each, explain the real - consequence (not "poses a risk" — say what actually happens) and teach + consequence (not "poses a risk", say what actually happens) and teach the underlying concept in a short paragraph. - Weave in `product_context` (step 1) when it changes how much a finding - matters — name the actual stakes the user told you about, don't just + matters, name the actual stakes the user told you about, don't just restate the generic finding. - Close with a "next-lesson roadmap": 3-6 ordered, achievable steps framed as skills to learn, not chores to finish. -- Calibrate urgency honestly — don't inflate a `low`/`warn` finding into +- Calibrate urgency honestly, don't inflate a `low`/`warn` finding into something scary just because the product context sounds high-stakes. Part of the lesson is learning to triage; the JSON severity is the floor, the product context can only shift emphasis, not invent urgency. @@ -222,18 +222,18 @@ exactly. Substance rules: ### 7. Write `FIX_BRIEF_FOR_CLAUDE_CODE.md` Follow the Document 2 template in `references/report-templates.md` -exactly — it's phase-gated by severity (Phase 1 = critical, Phase 2 = +exactly, it's phase-gated by severity (Phase 1 = critical, Phase 2 = high, Phase 3 = medium/low), each phase with acceptance criteria and a hard gate ("re-run the audit and confirm zero remaining findings at this severity before starting the next phase"). Substance rules: - The "what to ask Claude Code to do" block per finding can and should use precise technical language and real file paths pulled from the JSON - `evidence` field — that part is FOR the AI agent executing it. + `evidence` field, that part is FOR the AI agent executing it. - Everything else (the framing, the "why," the "what you'll learn") stays in the same plain-English voice as Document 1. - Include a verification step for every fix so the user learns to confirm fixes rather than assume they worked. -- **Agentic-environment safety — apply these to every instruction you +- **Agentic-environment safety, apply these to every instruction you write, not just ones an adapter file happens to mention:** - Never write an instruction that runs an interactive setup wizard (a CLI that prompts for input, opens a browser for OAuth, etc.) as an @@ -248,7 +248,7 @@ severity before starting the next phase"). Substance rules: not from what is typical for the stack. - Never assume a paid-tier or plan-gated platform feature (private-repo branch protection, arbitrary-deployment rollback on Vercel Hobby, - etc.) is available — either confirm the plan first or flag it as a + etc.) is available, either confirm the plan first or flag it as a manual, plan-dependent item. - Anything requiring a human judgment call with real consequences (choosing to run a destructive migration's "fix," deciding whether to @@ -278,18 +278,18 @@ you to make explicit. Then check by eye what it cannot: -- Scan for any leftover jargon that isn't immediately explained — if you +- Scan for any leftover jargon that isn't immediately explained, if you used a technical term, either the glossary already covers it or you need to add a one-clause explanation inline. - Confirm severity ordering and phase grouping matches the JSON (critical items must appear first, in Phase 1, and be unambiguous about urgency). - Confirm every file path/evidence snippet quoted was actually present in - the JSON output — never fabricate a file path. + the JSON output, never fabricate a file path. - Confirm the manual-steps list actually captures everything flagged - agentic-unsafe in steps 4 and 7 — this list existing and being accurate + agentic-unsafe in steps 4 and 7, this list existing and being accurate matters as much as the phases themselves. - **Name the source of every fact that isn't in the JSON.** Plan tiers - ("you're on Vercel Pro"), team size, traffic, who uses the app — if it + ("you're on Vercel Pro"), team size, traffic, who uses the app, if it came from the user, write "you told me"; if you read it from a file, name the file. Never state a platform or plan fact as if the tool found it when it didn't. If a fix depends on such a fact (e.g. rollback target @@ -329,11 +329,11 @@ of it; the tool does not run the app. ## Notes on the underlying tool `scripts/prod_audit.py` is a dependency-free Python 3 script (stdlib only) -that performs static analysis — no code execution, no network calls. It +that performs static analysis, no code execution, no network calls. It first computes a **stack fingerprint** (languages, package managers, frameworks, deploy surfaces + evidence, runtimes, migration tooling, `multi_surface` flag + evidence, `adapters_matched`), then runs checks -across eight categories: AI Agent Context, CI/CD Pipeline, Structured +across nine categories: AI Agent Context, Access Control, CI/CD Pipeline, Structured Logging & Observability, Secrets & Environment Management, Resilience & Failover, **Multi-Surface Deployment & Coordinated Rollback**, Testing & Quality Gates, and Dependency & Supply-chain Security. @@ -344,22 +344,22 @@ docker-compose stack with a dedicated migration service): it flags when a repo has more than one independently-rollback-able surface, checks whether a coordinated rollback procedure is actually documented across all of them, and separately flags irreversible-migration risk (destructive SQL -with no documented reverse procedure) — that last check runs for every +with no documented reverse procedure), that last check runs for every repo regardless of surface count, since a single-surface app can still lose data permanently to a migration that a code rollback can't undo. -It works on any git repo regardless of language/framework — repos with no +It works on any git repo regardless of language/framework, repos with no recognized deploy platform still get a clean fingerprint (empty surfaces, `multi_surface: false`) rather than a crash. See its own `--help` output for CLI flags; the ones that matter here are `--json` (for this skill's translation step), `--context` (for the product-context calibration in -step 1 — stored verbatim, never alters the score), and `--fail-on +step 1, stored verbatim, never alters the score), and `--fail-on critical` (for CI gating, worth mentioning to the user as a follow-up). -Secret detection is regex-based and intentionally conservative — always +Secret detection is regex-based and intentionally conservative, always tell the user it's not a substitute for a dedicated secret scanner (gitleaks/truffleHog) run against full git history if any hardcoded-secret finding shows up. Same caveat applies to the destructive-migration regex -in the Multi-Surface category — it catches common patterns +in the Multi-Surface category, it catches common patterns (`DROP TABLE`, `DROP COLUMN`, `TRUNCATE`, destructive `ALTER TABLE`), not every way a migration can be irreversible. diff --git a/skills/prod-readiness-coach/scripts/audit/checks_access.py b/skills/prod-readiness-coach/scripts/audit/checks_access.py new file mode 100644 index 0000000..9f2b362 --- /dev/null +++ b/skills/prod-readiness-coach/scripts/audit/checks_access.py @@ -0,0 +1,133 @@ +"""Checks about who is allowed to do what. + +Broken access control is the most common way a small app hurts its users, and +it is the gap this tool shipped with for its first three versions. These checks +are deliberately shallow: they can tell you a guard is missing or that one +fails open, and they cannot tell you an authorization model is correct. Any +pass here is a starting point for a human, never a security review. +""" +import re + +from .model import CheckResult +from .repo import Repo + +AUTH_LIBS = [ + "@clerk/nextjs", "@clerk/clerk-react", "@clerk/backend", "next-auth", "@auth/core", + "lucia", "better-auth", "@supabase/auth-helpers-nextjs", "@supabase/ssr", + "@auth0/nextjs-auth0", "@workos-inc/node", "firebase-admin", "passport", + "express-jwt", "jsonwebtoken", "@nestjs/passport", "django-allauth", "flask-login", + "authlib", "devise", "pyjwt", "python-jose", "fastapi-users", +] +# Words that indicate a request handler actually consults an identity before acting. +GUARD_RX = re.compile( + r"\b(?:auth|getSession|getServerSession|currentUser|requireUser|requireAuth|protect|" + r"authorize|isOwner|isAdmin|verifyToken|jwt_required|login_required|current_user|" + r"getToken|clerkClient|withAuth|ensureAuthenticated)\b", re.IGNORECASE) +ROUTE_GLOBS = [ + "**/app/api/**/route.ts", "**/app/api/**/route.js", + "**/pages/api/**/*.ts", "**/pages/api/**/*.js", + "**/routes/**/*.ts", "**/routes/**/*.js", "**/api/**/*.py", +] +# A guard that returns "allowed" when its own configuration is missing. +# Real code rarely tests the env var inline. It reads it into a local first: +# const owner = process.env.OWNER_EMAIL; +# if (!owner) return true; +# so we learn the local names first, then look for the fail-open branch on them. +ENV_LOCAL_RX = re.compile( + r"(?:const|let|var)\s+(\w+)\s*=\s*(?:process\.env\.|import\.meta\.env\.)\w+" + r"|(\w+)\s*=\s*os\.(?:environ\.get|getenv)\(") +FAIL_OPEN_DIRECT_RX = re.compile( + r"if\s*\(\s*!\s*(?:process\.env\.|import\.meta\.env\.)\w+\s*\)\s*\{?\s*return\s+true" + r"|if\s+not\s+os\.(?:environ\.get|getenv)\([^)]*\)\s*:\s*return\s+True", + re.IGNORECASE) + + +def _fail_open_lines(text: str) -> list[str]: + """Lines where a permission check grants access because its setting is absent.""" + env_locals = {m.group(1) or m.group(2) for m in ENV_LOCAL_RX.finditer(text)} - {None} + hits = [] + for line in text.splitlines(): + stripped = line.strip() + if FAIL_OPEN_DIRECT_RX.search(stripped): + hits.append(stripped) + continue + for name in env_locals: + if re.search(rf"if\s*\(\s*!\s*{re.escape(name)}\s*\)\s*\{{?\s*return\s+true", stripped, re.I) \ + or re.search(rf"if\s+not\s+{re.escape(name)}\s*:\s*return\s+True", stripped): + hits.append(stripped) + break + return hits + + +REF = "https://owasp.org/Top10/A01_2021-Broken_Access_Control/" + + +def check_access_control(repo: Repo) -> list[CheckResult]: + results = [] + pkg = repo.package_json() + dep_names = {n.lower() for n in ((pkg.get("dependencies", {}) | pkg.get("devDependencies", {})).keys() + if pkg else [])} + dep_names |= {re.split(r"[=<>!~\[; ]", line.strip(), 1)[0].lower() + for line in repo.requirements_text().splitlines() + if line.strip() and not line.lstrip().startswith("#")} + found = [lib for lib in AUTH_LIBS if lib.lower() in dep_names] + + results.append(CheckResult( + "auth-1", "Access Control", "Authentication mechanism present", + "pass" if found else "fail", + "info" if found else "high", + f"Authentication dependency detected: {', '.join(found)}." if found else + "No authentication library found in the dependency list. If this app has " + "users or non-public data, nothing here proves who a request belongs to.", + "" if found else "Add an authentication library appropriate to the stack, or, if the " + "app is genuinely public and read-only, say so in the README so the next reader knows " + "it is a decision rather than an omission.", + evidence=[f"dependency: {lib}" for lib in found], + best_practice_ref=REF, + )) + + route_files = [f for f in repo.find_any(ROUTE_GLOBS) if "node_modules" not in f] + if route_files: + unguarded = [f for f in route_files if not GUARD_RX.search(repo.read(f))] + if unguarded: + results.append(CheckResult( + "auth-2", "Access Control", "Request handlers consult an identity", + "fail", "high", + f"{len(unguarded)} of {len(route_files)} request handler(s) never mention an " + "auth check. A handler that does not ask who is calling will serve anyone who " + "can reach the URL.", + "Open each file listed. If it is meant to be public, note that in a comment so " + "the next reader can tell 'public on purpose' from 'forgotten'. If it is not, " + "add the guard your auth library provides, and test it by calling the route " + "while signed out and while signed in as somebody else.", + evidence=unguarded[:10], + best_practice_ref=REF, + )) + else: + results.append(CheckResult( + "auth-2", "Access Control", "Request handlers consult an identity", + "pass", "info", + f"All {len(route_files)} request handler(s) reference an auth check. This says " + "the check is mentioned, not that it is correct.", + evidence=route_files[:10], + best_practice_ref=REF, + )) + + fail_open = [] + for f in repo.code_files(): + for line in _fail_open_lines(repo.read(f)): + fail_open.append(f"{f}: {line[:90]}") + results.append(CheckResult( + "auth-3", "Access Control", "No guard that fails open on missing configuration", + "fail" if fail_open else "pass", + "critical" if fail_open else "info", + "A permission check appears to grant access when its own environment variable is " + "unset. On a fresh deploy, or a fork, or after a rename, that means everyone passes." + if fail_open else + "No permission check was found that returns 'allowed' when its configuration is missing.", + "Make the guard deny by default: when the variable is missing, refuse and log loudly. " + "A missing setting is a broken deploy, not an open door." if fail_open else "", + evidence=fail_open[:5] or ["scope: searched code files for guards that return true on missing config"], + best_practice_ref=REF, + )) + return results diff --git a/skills/prod-readiness-coach/scripts/audit/model.py b/skills/prod-readiness-coach/scripts/audit/model.py index 2ca637d..8882dc6 100644 --- a/skills/prod-readiness-coach/scripts/audit/model.py +++ b/skills/prod-readiness-coach/scripts/audit/model.py @@ -22,6 +22,8 @@ # Points removed from the 100-point category score when a check with this # severity fails. Weighted so a single CRITICAL miss visibly tanks the score. SEVERITY_PENALTY = {"critical": 30, "high": 18, "medium": 10, "low": 5, "info": 0} +# A warning is the same finding held with less certainty, so it costs half, never zero. +SEVERITY_PENALTY_WARN = {k: (v + 1) // 2 for k, v in SEVERITY_PENALTY.items()} @dataclass @@ -44,7 +46,7 @@ class CheckResult: # Checks whose pass is based on a text search, not a structural check. -WEAK_PASS_IDS = {"log-4", "res-3", "sec-4", "ms-2"} +WEAK_PASS_IDS = {"log-4", "res-3", "sec-4", "ms-2", "auth-2", "auth-3"} # A control can live outside the repository (an org-level pipeline, a platform # dashboard, a secrets vault). Without a way to say so, --fail-on can never @@ -163,7 +165,7 @@ def find_contradictions(categories: list["Category"]) -> list[dict]: # surface implies web-app (strict). A language alone implies nothing, so the # profile is "unknown" and runtime checks report insufficient evidence. PROFILES = ("web-app", "api", "worker", "cli", "library", "unknown") -_SERVICE_ONLY = {"log-3", "res-3"} # needs an HTTP surface +_SERVICE_ONLY = {"log-3", "res-3", "auth-1", "auth-2", "auth-3"} # needs an HTTP surface _DEPLOYED_ONLY = {"log-1", "log-2", "log-4", "res-1", "res-2", "res-4", "res-5", "sec-5", "ms-1", "ci-5"} | _SERVICE_ONLY # needs to run somewhere CHECK_SKIPS_BY_PROFILE = { @@ -188,10 +190,14 @@ class Category: @property def score(self) -> int: + # A "warn" is a real finding shown with a severity badge, so it has to move the + # number. It costs half of the same finding failing: less certain, still not free. score = 100 for c in self.checks: if c.status == "fail": score -= SEVERITY_PENALTY.get(c.severity, 5) + elif c.status == "warn": + score -= SEVERITY_PENALTY_WARN.get(c.severity, 0) return max(0, score) @property diff --git a/skills/prod-readiness-coach/scripts/audit/repo.py b/skills/prod-readiness-coach/scripts/audit/repo.py index 3e1aed8..0220245 100644 --- a/skills/prod-readiness-coach/scripts/audit/repo.py +++ b/skills/prod-readiness-coach/scripts/audit/repo.py @@ -144,6 +144,10 @@ def workspace_package_files(self) -> list[str]: return [f for f in self.find_any(["apps/*/package.json", "packages/*/package.json", "services/*/package.json"]) if "node_modules" not in f] + def code_files(self) -> list[str]: + """Tracked source files, the same set grep() searches by default.""" + return [f for f in self.git_files() if Path(f).suffix in CODE_EXTS] + def package_json(self) -> dict: """Root package.json, with workspace members' dependencies and scripts merged in. A monorepo's frameworks live in apps/* and packages/*, not at the root.""" diff --git a/skills/prod-readiness-coach/scripts/audit/runner.py b/skills/prod-readiness-coach/scripts/audit/runner.py index dcae507..655113a 100644 --- a/skills/prod-readiness-coach/scripts/audit/runner.py +++ b/skills/prod-readiness-coach/scripts/audit/runner.py @@ -12,6 +12,7 @@ from .checks_build import check_claude_md, check_ci_pipeline, check_test_scripts_defined from .checks_runtime import (check_multi_surface_deployment, check_resilience_and_runbooks, check_secrets_management, check_structured_logging) +from .checks_access import check_access_control from .checks_quality import check_dependency_security, check_testing_quality_gates # -------------------------------------------------------------------------- @@ -23,6 +24,10 @@ "Documentation that lets AI coding agents and human contributors operate " "safely and consistently on the codebase.", "https://docs.claude.com/en/docs/claude-code/memory"), + ("Access Control", "Access Control", + "Who is allowed to do what. Shallow by design: these checks can show a guard is " + "missing or fails open, never that an authorization model is correct.", + "https://owasp.org/Top10/A01_2021-Broken_Access_Control/"), ("CI/CD Pipeline", "CI/CD Pipeline", "Automated build/test/lint gates that block broken code from reaching production.", "https://docs.github.com/en/actions/learn-github-actions"), @@ -88,6 +93,7 @@ def run_audit(repo_path: Path, profile: Optional[str] = None) -> tuple[list[Cate categories = {key: Category(key, title, desc, ref) for key, title, desc, ref in CATEGORY_META} categories["AI Agent Context"].checks.append(check_claude_md(repo)) + categories["Access Control"].checks.extend(check_access_control(repo)) categories["CI/CD Pipeline"].checks.extend(check_ci_pipeline(repo)) categories["CI/CD Pipeline"].checks.append(check_test_scripts_defined(repo)) categories["Structured Logging & Observability"].checks.extend(check_structured_logging(repo)) diff --git a/skills/prod-readiness-coach/scripts/prod_audit.py b/skills/prod-readiness-coach/scripts/prod_audit.py index 0bebc24..e55f0e9 100644 --- a/skills/prod-readiness-coach/scripts/prod_audit.py +++ b/skills/prod-readiness-coach/scripts/prod_audit.py @@ -24,7 +24,7 @@ from audit.model import ( # noqa: F401 (re-exported for callers and tests) CHECK_SKIPS_BY_PROFILE, CONTRADICTIONS, MIN_DOC_WORDS, PROFILES, - SEVERITY_LABEL, SEVERITY_PENALTY, WAIVER_FIELDS, WAIVER_FILE, WAIVER_MAX_AGE_DAYS, + SEVERITY_LABEL, SEVERITY_PENALTY, SEVERITY_PENALTY_WARN, WAIVER_FIELDS, WAIVER_FILE, WAIVER_MAX_AGE_DAYS, WEAK_PASS_IDS, Category, CheckResult, find_contradictions, has_substance, load_waivers, runs_a_test_suite, ) diff --git a/skills/prod-readiness-coach/tests/test_prod_audit.py b/skills/prod-readiness-coach/tests/test_prod_audit.py index a6a22df..48ebd9e 100644 --- a/skills/prod-readiness-coach/tests/test_prod_audit.py +++ b/skills/prod-readiness-coach/tests/test_prod_audit.py @@ -441,6 +441,91 @@ def test_a_real_logger_still_counts_and_cites_it(self): self.assertIn("dependency: pino", k["evidence"]) +NEXT_BASE = {"package.json": json.dumps({"dependencies": {"next": "15.0.0"}}), "package-lock.json": "{}"} + + +class AccessControl(unittest.TestCase): + """Broken access control is the likeliest way a vibe-coded app hurts its users, + and until now nothing here looked at it.""" + + def test_no_auth_library_on_a_web_app_is_a_finding(self): + r = audit(NEXT_BASE) + self.assertEqual(check(r, "auth-1")["status"], "fail") + + def test_an_installed_auth_library_is_found_and_cited(self): + r = audit({**NEXT_BASE, "package.json": json.dumps( + {"dependencies": {"next": "15.0.0", "@clerk/nextjs": "6.0.0"}})}) + k = check(r, "auth-1") + self.assertEqual(k["status"], "pass") + self.assertIn("dependency: @clerk/nextjs", k["evidence"]) + + def test_routes_that_never_reference_the_guard_are_flagged(self): + r = audit({**NEXT_BASE, + "package.json": json.dumps({"dependencies": {"next": "15.0.0", "@clerk/nextjs": "6.0.0"}}), + "src/app/api/send/route.ts": "export async function POST() { return Response.json({ok:1}); }\n", + "src/app/api/list/route.ts": "export async function GET() { return Response.json([]); }\n"}) + k = check(r, "auth-2") + self.assertEqual(k["status"], "fail") + self.assertTrue(any("send" in e for e in k["evidence"])) + + def test_a_route_that_checks_auth_is_not_flagged(self): + r = audit({**NEXT_BASE, + "package.json": json.dumps({"dependencies": {"next": "15.0.0", "@clerk/nextjs": "6.0.0"}}), + "src/app/api/send/route.ts": + "import { auth } from '@clerk/nextjs/server';\n" + "export async function POST() { const { userId } = await auth(); " + "if (!userId) return new Response('no', {status:401}); return Response.json({ok:1}); }\n"}) + self.assertEqual(check(r, "auth-2")["status"], "pass") + + def test_a_guard_that_fails_open_when_its_env_var_is_unset_is_critical(self): + """The exact shape found in a real live app: no OWNER_EMAIL set means everyone is the owner.""" + r = audit({**NEXT_BASE, "src/lib/owner.ts": + "export function isOwner(email: string) {\n" + " const owner = process.env.OWNER_EMAIL;\n" + " if (!owner) return true;\n" + " return email === owner;\n}\n"}) + k = check(r, "auth-3") + self.assertEqual(k["status"], "fail") + self.assertEqual(k["severity"], "critical") + # A failing check quotes the offending line, so it is evidenced. What it must not + # do is claim certainty about intent: this is a pattern, and the wording says so. + self.assertTrue(any("owner.ts" in e for e in k["evidence"])) + self.assertIn("appears", k["detail"].lower()) + self.assertIn("open", k["recommendation"].lower() + k["detail"].lower()) + + def test_a_guard_that_fails_closed_is_not_flagged(self): + r = audit({**NEXT_BASE, "src/lib/owner.ts": + "export function isOwner(email: string) {\n" + " const owner = process.env.OWNER_EMAIL;\n" + " if (!owner) return false;\n" + " return email === owner;\n}\n"}) + self.assertNotEqual(check(r, "auth-3")["status"], "fail") + + def test_access_control_is_not_applied_to_a_cli(self): + r = audit({"tool.py": "print(1)"}, profile="cli") + self.assertEqual(check(r, "auth-1")["status"], "n/a") + + +class WarningsCost(unittest.TestCase): + """A finding printed with a severity badge must move the number, or the badge is theatre.""" + + def test_a_warn_reduces_the_category_score(self): + r = audit({**NEXT_BASE, "package.json": json.dumps( + {"dependencies": {"next": "15.0.0"}, "scripts": {"test": "vitest"}}), + "tests/a.test.ts": "test('x',()=>{})"}) + for c in r["categories"]: + warns = [k for k in c["checks"] if k["status"] == "warn" and k["severity"] != "info"] + if warns and c["applicable"]: + self.assertLess(c["score"], 100, + f"{c['key']} shows {[w['id'] for w in warns]} as findings but scores 100") + return + self.skipTest("no warn findings in this fixture") + + def test_a_warn_costs_less_than_the_same_finding_failing(self): + self.assertLess(prod_audit.SEVERITY_PENALTY_WARN["medium"], prod_audit.SEVERITY_PENALTY["medium"]) + self.assertGreater(prod_audit.SEVERITY_PENALTY_WARN["medium"], 0) + + class CiDetection(unittest.TestCase): def test_unittest_step_counts_as_running_tests(self): wf = "on: push\njobs:\n t:\n runs-on: ubuntu-latest\n steps:\n - run: python -m unittest discover tests\n" diff --git a/skills/product-readiness-review/SKILL.md b/skills/product-readiness-review/SKILL.md index 667f71f..2b14386 100644 --- a/skills/product-readiness-review/SKILL.md +++ b/skills/product-readiness-review/SKILL.md @@ -9,13 +9,16 @@ Assess the product as a working system, not merely as a collection of clean file ## Workflow -1. Infer the product purpose, intended users, primary journeys, stack, and deployment model from repository evidence. -2. State important unknowns rather than silently assuming them. -3. Define milestone-specific acceptance checks for the product's primary journeys. For an agent, plugin, or developer tool, include discovery, representative invocation, safety boundaries, and generated artifacts. -4. Run safe configured tests, builds, analyzers, and repository checks when practical. -5. Evaluate the relevant dimensions in [references/readiness-rubric.md](references/readiness-rubric.md). -6. Separate launch blockers, handoff blockers, and later improvements. -7. Produce an evidence-based report using [references/report-format.md](references/report-format.md). +1. Ask one question first: which milestone is this for, and what happens if it goes wrong? "Ready for my first ten users" and "ready for a paying customer" are different reviews, and the report format asks you to judge against a milestone you were never told. +2. If the user has not already run `prod-readiness-coach`, run it now and read its JSON. It covers the repository controls (CI, secrets, rollback, tests) deterministically, so this review can spend its attention on what a script cannot judge: whether the product works for the people using it. Do not re-derive its findings by hand. +3. Infer the product purpose, intended users, primary journeys, stack, and deployment model from repository evidence. +4. State important unknowns rather than silently assuming them. +5. Define milestone-specific acceptance checks for the product's primary journeys. For an agent, plugin, or developer tool, include discovery, representative invocation, safety boundaries, and generated artifacts. +6. Run configured tests and read-only analyzers. "Safe" means it does not write into the user's repository, does not deploy, and does not touch a live service. A build usually writes artifacts (`.next/`, `dist/`, `*.tsbuildinfo`), so either run it in a copy or skip it and say you skipped it. Never guess at a result you did not run. +7. Scope honestly. On a large repository you will not read everything. Say what you sampled and what you did not, and pick by risk: the paths that handle money, identity, or someone else's data first. +8. Evaluate the relevant dimensions in [references/readiness-rubric.md](references/readiness-rubric.md). +9. Separate launch blockers, handoff blockers, and later improvements. +10. Report in chat by default; write a file only to a path the user names. Produce an evidence-based report using [references/report-format.md](references/report-format.md). ## Rules diff --git a/templates/CLAUDE.md b/templates/CLAUDE.md index 5c70526..336b659 100644 --- a/templates/CLAUDE.md +++ b/templates/CLAUDE.md @@ -1,4 +1,4 @@ - + # Clean Code Standards Build software that is correct, safe to change, and understandable to the next developer. Apply these rules to new code. In existing code, report broader problems instead of rewriting them unless the user asks. @@ -19,7 +19,7 @@ Build software that is correct, safe to change, and understandable to the next d - Use names that communicate intent at their scope and match product vocabulary. - Keep responsibilities cohesive. Readability beats arbitrary function-length or parameter-count limits. -- Comments explain decisions, business rules, constraints, or non-obvious techniques—not syntax. +- Comments explain decisions, business rules, constraints, or non-obvious techniques, not syntax. - Avoid abstraction until it protects a real boundary or repeated variation. ## Correctness and safety @@ -31,7 +31,7 @@ Build software that is correct, safe to change, and understandable to the next d ## Tests and verification -- Test important behavior, business rules, boundaries, and regressions—not every trivial line. +- Test important behavior, business rules, boundaries, and regressions, not every trivial line. - Before risky changes to untested code, add a characterization test or explain the risk and ask how to proceed. - Run the narrowest relevant checks after edits and report what ran. @@ -44,8 +44,8 @@ Build software that is correct, safe to change, and understandable to the next d ## Going live - Add CI before a project's first shared or live deployment, and earlier if it already has real tests, a collaborator, or automatic deploys. "Live" means a real person, a scheduled job, or another service depends on it; connecting a deploy platform counts. -- Before that day, do not add CI ceremony to a sketch. -- On that day, or whenever a live repo has no CI: add a workflow that runs typecheck, tests, and build on every pull request and push to `main`. Prove it by breaking one test, watching it fail, then reverting. Then require that check via branch protection, which is a manual settings step for the human. +- Before then, while it is still a sketch nobody depends on, do not add CI ceremony. +- At that point, or whenever a live repo has no CI: add a workflow that runs typecheck, tests, and build on every pull request and push to `main`. Prove it by breaking one test, watching it fail, then reverting. Then require that check via branch protection, which is a manual settings step for the human. - Before a launch, schema change, or handoff, offer the `prod-readiness-coach` skill. - Never merge over a failing check "just this once."