diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 1a267ef..006ed60 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.5.1", + "version": "3.5.2", "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.5.1", + "version": "3.5.2", "author": { "name": "Tarik Moody" }, diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 7abfe7a..5c3a7c7 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "clean-code-toolkit", - "version": "3.5.1", + "version": "3.5.2", "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 7d81c0c..7a63ebc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ # Changelog +## 3.5.2 + +A developer-experience audit of the toolkit itself: install it fresh, run the CLI, break it on purpose, read every doc link. Four things it found. + +**An empty folder used to get a grade.** Pointed at a directory with nothing in it, the tool reported "Repository Controls Score: 65/100, D, release blockers present" with four CRITICAL findings, and never said the folder was empty. A mistyped path produced a confident report about nothing, which is the same failure 3.5.1 shipped three fixes for. It now refuses: it says what is wrong, says to pass `--repo .` instead, and exits 2. **This is a behavior change.** A pipeline pointed at a path with no files used to exit 0 and now exits 2. See `docs/decisions/008`. + +**A folder that is not a git repository now says so, at the top of the report.** The scan still runs, because there are real files and the findings mean something, but `git ls-files` is unavailable, so `.gitignore` is not applied and `node_modules` or build output can be read as source. That caveat now appears above the score instead of nowhere. + +**The toolkit shipped ten files its own linter rejects.** `check_report.py` refuses em and en dashes in generated documents. Eight shipped files contained em dashes, including `plain-english-glossary.md`, the file that defines the plain-English house style, and all five stack reference files. Two contained en dashes. The 3.5.0 entry claiming em dashes were "gone from every skill, command and template" was true of those three directories and never covered `references/`. All ten are fixed, and `validate-toolkit.sh` now enforces the rule, so it cannot come back. The rule was previously enforced only on documents the tool generated, never on the documents it ships. + +**`add-clean-code.sh --help` was a dead end.** It printed "Unknown option: --help" and exited 64. The usage text already existed as a comment at the top of the same file and was never printed. `--help` and `-h` now print it and exit 0, and an unknown flag prints it too. + +Tests: 92. + ## 3.5.1 Five defects, and one thing underneath all of them: the engine only really knew Node. Two of these were saying something false about any repository a user ran them on today. diff --git a/docs/decisions/008-refuse-to-grade-an-empty-folder.md b/docs/decisions/008-refuse-to-grade-an-empty-folder.md new file mode 100644 index 0000000..09f2e12 --- /dev/null +++ b/docs/decisions/008-refuse-to-grade-an-empty-folder.md @@ -0,0 +1,21 @@ +# 008: An empty folder gets refused, not graded + +**Decision.** When the target directory contains no files, the audit stops with an error and exit code 2 instead of producing a report. A directory that has files but is not a git repository still runs, and the report opens with a warning that `.gitignore` was not applied. + +**Why this came up.** A developer experience audit ran the tool against an empty directory. It returned "Repository Controls Score: 65/100, D, release blockers present" and four CRITICAL findings, and never mentioned that the folder was empty. A person who mistypes a path gets a confident grade about nothing. This is the same failure the v3.5.1 release existed to fix, three times over: a tool claiming more than it checked. What was at stake is the only thing this tool sells, which is that its output can be trusted without re-checking it by hand. + +**Options.** +1. Leave it. The report does say "none detected" on every stack line, so a careful reader could work it out. Cost: the score, the grade and the word CRITICAL are the parts people actually read, and all three are false. +2. Produce the report but add a banner saying the folder is empty. Cost: it still prints a grade. A grade for nothing is not made true by a banner above it, and CI gating on `--fail-on` would still act on it. +3. Refuse. Print what is wrong, print what to do instead, exit non-zero. Cost: exit code 2 on an empty directory is a behavior change. Anyone whose pipeline pointed at the wrong path was previously getting a silent pass and will now get a failure. + +**What we chose and why.** Option 3. Joint call. The cost of option 3 is a pipeline that breaks and tells you why, which is the correct outcome for a pipeline that was auditing nothing. The cost of options 1 and 2 is a false grade that somebody acts on. + +The "not a git repository" case is deliberately treated differently. There are real files, so the checks have something to read and the findings mean something. But `git ls-files` is unavailable, so `.gitignore` is not applied and `node_modules` or build output can be read as source. That is a caveat on a real result, not a refusal. + +**What we gave up.** A pipeline pointed at a wrong path used to exit 0 and now exits 2. We judged that a fix, not a regression, but it is a breaking change in a patch release and the CHANGELOG says so. + +**How we'll know if this was right.** Nobody reports a score for a directory they did not mean to audit. If someone's CI breaks after upgrading, the error message tells them their path is wrong in one line, and that is the finding. + +**What actually happened.** +(Tarik fills this in.) diff --git a/scripts/add-clean-code.sh b/scripts/add-clean-code.sh index 2f1128b..2df7f5b 100755 --- a/scripts/add-clean-code.sh +++ b/scripts/add-clean-code.sh @@ -15,13 +15,31 @@ set -euo pipefail +usage() { + cat <<'USAGE' +Install or update the Clean Code Standards block in a project's CLAUDE.md. + +Usage: add-clean-code.sh [target-dir] [--update] [--check] [--help] + + target-dir project to install into (default: the current directory) + (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 + --help show this message + +The block is delimited by BEGIN/END markers and carries a version, so an +installed copy can be compared with the shipped one. +USAGE +} + target_dir="." mode="install" for arg in "$@"; do case "${arg}" in --update) mode="update" ;; --check) mode="check" ;; - --*) echo "Unknown option: ${arg}" >&2; exit 64 ;; + -h|--help) usage; exit 0 ;; + --*) echo "Unknown option: ${arg}" >&2; echo >&2; usage >&2; exit 64 ;; *) target_dir="${arg}" ;; esac done diff --git a/scripts/validate-toolkit.py b/scripts/validate-toolkit.py index 4022139..7d41d46 100755 --- a/scripts/validate-toolkit.py +++ b/scripts/validate-toolkit.py @@ -159,16 +159,38 @@ def validate_installer() -> None: raise ValidationError("Installer did not preserve a legacy Clean Code Standards section") +def validate_house_style() -> None: + """check_report.py rejects em and en dashes in generated docs. The toolkit + that enforces that rule on other people's reports cannot ship them itself.""" + offenders = [] + for base in ("skills", "commands", "templates"): + directory = ROOT / base + if not directory.exists(): + continue + for path in sorted(directory.rglob("*.md")): + text = path.read_text(encoding="utf-8", errors="ignore") + for lineno, line in enumerate(text.splitlines(), 1): + if "\u2014" in line or "\u2013" in line: + offenders.append(f"{path.relative_to(ROOT)}:{lineno}") + if offenders: + raise ValidationError( + "em or en dash in shipped text (house style uses plain punctuation): " + + ", ".join(offenders[:10]) + + (f" and {len(offenders) - 10} more" if len(offenders) > 10 else "") + ) + + def main() -> int: try: validate_skills() validate_plugin() validate_installer() + validate_house_style() except (ValidationError, OSError, subprocess.CalledProcessError) as error: print(f"Validation failed: {error}") return 1 - print("Validated skills, plugin manifests, and installer runtime successfully.") + print("Validated skills, plugin manifests, installer runtime, and house style successfully.") return 0 diff --git a/skills/boy-scout-cleanup/SKILL.md b/skills/boy-scout-cleanup/SKILL.md index 6e7532c..280841e 100644 --- a/skills/boy-scout-cleanup/SKILL.md +++ b/skills/boy-scout-cleanup/SKILL.md @@ -1,6 +1,6 @@ --- name: boy-scout-cleanup -description: "Make 3–5 small, local, behavior-preserving improvements to existing code. Use when the user asks to tidy a file, clean up code while working nearby, remove local clutter, or make a module easier to read. Do not use for a read-only review, public API changes, file moves, broad rewrites, or feature work." +description: "Make 3 to 5 small, local, behavior-preserving improvements to existing code. Use when the user asks to tidy a file, clean up code while working nearby, remove local clutter, or make a module easier to read. Do not use for a read-only review, public API changes, file moves, broad rewrites, or feature work." --- # Boy Scout Cleanup @@ -42,7 +42,7 @@ Use `/refactor` for structural work and `clean-code-review` for read-only assess ## Workflow -1. Choose at most 3–5 related improvements. +1. Choose at most 3 to 5 related improvements. 2. Apply the smallest possible patches; do not rewrite the whole file. 3. Preserve strictness, ordering, side effects, mutation, exceptions, and public names. 4. Run the narrowest relevant checks, followed by broader configured checks when practical. diff --git a/skills/clean-code-review/SKILL.md b/skills/clean-code-review/SKILL.md index 86cd745..4ea85b9 100644 --- a/skills/clean-code-review/SKILL.md +++ b/skills/clean-code-review/SKILL.md @@ -53,5 +53,5 @@ Report in chat by default. Write a file only if the user asks, and then to a pat - For a product-level assessment, use `product-readiness-review`. - For a developer-ready handoff document, use `developer-handoff`. -- For 3–5 local, behavior-preserving improvements, use `boy-scout-cleanup`. +- For 3 to 5 local, behavior-preserving improvements, use `boy-scout-cleanup`. - For structural edits, use `/refactor` and verify behavior before and after. diff --git a/skills/clean-code-review/references/report-format.md b/skills/clean-code-review/references/report-format.md index 7500e3f..1a2a74f 100644 --- a/skills/clean-code-review/references/report-format.md +++ b/skills/clean-code-review/references/report-format.md @@ -11,10 +11,10 @@ State what was reviewed, overall confidence, and whether the code appears safe t Order findings by severity. For each finding include: 1. **Title and severity** -2. **Location** — file and tight line range -3. **Evidence** — what the code demonstrably does -4. **Impact** — realistic user, operational, or maintenance consequence -5. **Recommendation** — smallest credible fix +2. **Location**: file and tight line range +3. **Evidence**: what the code demonstrably does +4. **Impact**: realistic user, operational, or maintenance consequence +5. **Recommendation**: smallest credible fix For beginner-facing reports, express evidence, impact, and recommendation as What / Why / Fix. diff --git a/skills/clean-code-review/references/review-rubric.md b/skills/clean-code-review/references/review-rubric.md index 9a3daf4..256596c 100644 --- a/skills/clean-code-review/references/review-rubric.md +++ b/skills/clean-code-review/references/review-rubric.md @@ -36,7 +36,7 @@ Only make claims supported by code or configured tools. Recommend a dedicated se - Prefer visible, unsurprising flow and manageable nesting. - Flag duplicated decisions, invalid states that are easy to construct, and mutation that is hard to track. -- Early returns, tables, composition, or polymorphism are options—not automatic answers. +- Early returns, tables, composition, or polymorphism are options, not automatic answers. ## 6. Interfaces and dependencies @@ -55,7 +55,7 @@ Only make claims supported by code or configured tools. Recommend a dedicated se ## 8. Tests and change safety - Test important behavior, business rules, boundaries, and regressions. -- Evaluate whether the tests would catch the proposed failure—not merely whether a test file exists. +- Evaluate whether the tests would catch the proposed failure, not merely whether a test file exists. - Prefer characterization tests before risky behavior-preserving refactors of untested code. - Do not require a unit test for trivial pass-through code when higher-level coverage is clearer. diff --git a/skills/prod-readiness-coach/references/plain-english-glossary.md b/skills/prod-readiness-coach/references/plain-english-glossary.md index 5592102..8152f92 100644 --- a/skills/prod-readiness-coach/references/plain-english-glossary.md +++ b/skills/prod-readiness-coach/references/plain-english-glossary.md @@ -2,43 +2,43 @@ Use these translations whenever a check or finding uses the term on the left. Lead with the plain-English phrase; mention the technical term once -in parentheses so the reader can Google it or recognize it later — don't +in parentheses so the reader can Google it or recognize it later, don't hide the vocabulary, just don't lead with it. | Technical term | Plain-English translation | |---|---| | CI/CD pipeline | An automatic checker that tests your code every time you (or an AI) push a change, *before* it can break the live site. Think of it as a robot proofreader that runs your test suite for you, every single time, so you don't have to remember to. | -| CI runs on pull requests | The robot proofreader checks your work *before* it's allowed to merge into the main version of the app — not after. | +| CI runs on pull requests | The robot proofreader checks your work *before* it's allowed to merge into the main version of the app, not after. | | Structured logging | Writing down what your app is doing in a consistent, computer-readable format (like a form with labeled fields) instead of scattered `console.log("it broke")` notes. It's the difference between a filled-out incident report and a sticky note. | -| console.log / print statements | The "sticky note" way of debugging — quick and easy while coding, but these notes get lost, aren't searchable, and vanish once your terminal closes. Fine for local development, risky as your *only* way of knowing what happened in production. | -| Error tracking / APM (Application Performance Monitoring) | A tool (like Sentry) that automatically emails or messages you the moment something breaks for a real user — instead of you finding out because that user complained (or, worse, silently left). | +| console.log / print statements | The "sticky note" way of debugging, quick and easy while coding, but these notes get lost, aren't searchable, and vanish once your terminal closes. Fine for local development, risky as your *only* way of knowing what happened in production. | +| Error tracking / APM (Application Performance Monitoring) | A tool (like Sentry) that automatically emails or messages you the moment something breaks for a real user, instead of you finding out because that user complained (or, worse, silently left). | | Secrets management | Keeping passwords, API keys, and tokens out of your code and out of GitHub, storing them instead in a separate, locked-away place (environment variables, a secrets vault, or your hosting platform's dashboard). | | .env file | A private file on your computer (or your hosting platform) that holds your actual secret values. It should never be uploaded to GitHub. | -| .env.example | A *public*, safe-to-share list of which secret names your app needs (with fake/blank values) — like a packing list without the actual passport number filled in. | +| .env.example | A *public*, safe-to-share list of which secret names your app needs (with fake/blank values), like a packing list without the actual passport number filled in. | | Hardcoded secret | A password or API key typed directly into your code instead of pulled from a `.env` file. Dangerous because anyone who can see your code (including the public, if the repo is public) can see the secret too. | | Environment-specific config | Making sure your "practice" version (staging/development) and your "real" version (production) use different logins and different databases, so a mistake in testing can't accidentally mess up real users' data. | -| Runbook / incident response doc | A written "break glass in case of emergency" instruction sheet — what to do, in order, when something goes wrong at 2am. Without one, every outage is improvised. | -| Rollback | Un-doing a bad deploy and going back to the last version that worked — like `Ctrl+Z` for your whole live application. | +| Runbook / incident response doc | A written "break glass in case of emergency" instruction sheet, what to do, in order, when something goes wrong at 2am. Without one, every outage is improvised. | +| Rollback | Un-doing a bad deploy and going back to the last version that worked, like `Ctrl+Z` for your whole live application. | | Rate limiting | A speed bump that stops one user (or a bot, or a bug) from hammering your app or your AI API bill thousands of times a minute. | -| Database migration | A tracked, repeatable, undo-able way of changing your database's structure (adding a column, etc.) — instead of manually editing the live database and hoping you remember what you did. | -| Health check endpoint | A tiny webpage (like `/health`) that just says "yes, I'm alive and my database connection works" — so automated systems can notice and restart your app if it ever stops responding, without a human having to notice first. | +| Database migration | A tracked, repeatable, undo-able way of changing your database's structure (adding a column, etc.), instead of manually editing the live database and hoping you remember what you did. | +| Health check endpoint | A tiny webpage (like `/health`) that just says "yes, I'm alive and my database connection works", so automated systems can notice and restart your app if it ever stops responding, without a human having to notice first. | | Correlation ID / request ID / trace ID | A tracking number attached to one user's action as it moves through your system, so when something goes wrong you can find *all* the log lines related to that one specific request instead of guessing which of thousands of log lines matter. | | Test coverage | The percentage of your code that your automated tests actually run through and check. Not a perfect measure of quality, but a decent smoke alarm for "nobody is testing this part at all." | | Static type checking / strict mode | Having the computer double-check, before you even run the code, that you're not accidentally treating a number like a word or forgetting to handle a value that could be missing. Catches a whole category of bugs before they ever run. | -| Lockfile (package-lock.json, etc.) | A frozen, exact list of every dependency version your app is using — so "works on my machine" reliably means "works everywhere," including in production. | +| Lockfile (package-lock.json, etc.) | A frozen, exact list of every dependency version your app is using, so "works on my machine" reliably means "works everywhere," including in production. | | Dependabot / Renovate | A robot that automatically opens a pull request when one of your dependencies has a security fix available, so you don't have to manually check for updates. | | Vulnerability scan | An automated check of your dependencies against a public database of known security holes. | -| CLAUDE.md / AGENTS.md | A "welcome guide" file for AI coding assistants (and human teammates) explaining how your project is set up, what commands to run, and any house rules — so every session starts with the right context instead of re-guessing it. | -| Multi-surface deployment | Your app doesn't live in just one place — maybe your website is on one platform and your database/backend is on another. Each piece rolls back independently, which means "undo the bad deploy" isn't always one button. | -| Coordinated rollback | A rollback plan that accounts for *every* piece your app is split across, not just the one that's easiest to undo. Without it, you can "fix" the frontend while leaving the backend on the version that caused the problem — now they don't match, and things break in new ways. | -| Irreversible migration | A database change (like deleting a column or a table) that can't be undone just by putting the old app code back. The old code doesn't bring the deleted data back with it — only a backup can. | +| CLAUDE.md / AGENTS.md | A "welcome guide" file for AI coding assistants (and human teammates) explaining how your project is set up, what commands to run, and any house rules, so every session starts with the right context instead of re-guessing it. | +| Multi-surface deployment | Your app doesn't live in just one place, maybe your website is on one platform and your database/backend is on another. Each piece rolls back independently, which means "undo the bad deploy" isn't always one button. | +| Coordinated rollback | A rollback plan that accounts for *every* piece your app is split across, not just the one that's easiest to undo. Without it, you can "fix" the frontend while leaving the backend on the version that caused the problem, now they don't match, and things break in new ways. | +| Irreversible migration | A database change (like deleting a column or a table) that can't be undone just by putting the old app code back. The old code doesn't bring the deleted data back with it, only a backup can. | | Stack fingerprint | The specific combination of language, framework, and hosting platform this tool detected for your repo, used to tailor advice to your actual setup instead of giving generic one-size-fits-all tips. | ## Tone rules for any text you write using this glossary 1. **Never lead with jargon.** Say what it *does* and why it matters before naming it. -2. **Celebrate wins genuinely.** If a category passed, say so plainly — don't manufacture urgency where none exists. +2. **Celebrate wins genuinely.** If a category passed, say so plainly, don't manufacture urgency where none exists. 3. **Frame gaps as "next skill to learn," not "you did this wrong."** The audience is learning; treat every gap as a lesson, not a failure. 4. **Use a real consequence, not an abstract one.** "If this breaks, you won't know until a user emails you" lands harder than "lacks observability." 5. **Keep sentences short.** One idea per sentence. Avoid stacked subordinate clauses. -6. **It's OK to be a little encouraging/informal** — this is a coach, not an auditor filing a compliance report. +6. **It's OK to be a little encouraging/informal**: this is a coach, not an auditor filing a compliance report. diff --git a/skills/prod-readiness-coach/references/stacks/cloudflare-workers.md b/skills/prod-readiness-coach/references/stacks/cloudflare-workers.md index cc80ff5..97a2a1d 100644 --- a/skills/prod-readiness-coach/references/stacks/cloudflare-workers.md +++ b/skills/prod-readiness-coach/references/stacks/cloudflare-workers.md @@ -12,13 +12,13 @@ Load this file when `stack_fingerprint.adapters_matched` includes `cloudflare-wo - Up to 100 previous versions are retained for rollback, increased from 10 in September 2025 ([Cloudflare changelog](https://developers.cloudflare.com/changelog/post/2025-09-11-increased-version-rollback-limit/)). If the audit is being run against an older mirror of these docs or a repo with tooling - written before that change, don't assume only 10 versions are available — the current limit + written before that change, don't assume only 10 versions are available, the current limit is 100. ## The gap: `wrangler.toml` config is not covered by rollback `wrangler rollback` reverts the Worker's **code** at the edge. It does **not** revert changes -to `wrangler.toml` — routes, bindings (KV namespaces, D1 databases, R2 buckets, environment +to `wrangler.toml`. Routes, bindings (KV namespaces, D1 databases, R2 buckets, environment variables, etc.) are not part of what gets rolled back ([Cloudflare docs](https://developers.cloudflare.com/workers/versions-and-deployments/rollbacks/), confirmed in @@ -26,7 +26,7 @@ confirmed in about routes specifically). If a bad deploy changed a binding or route alongside the code, a `wrangler rollback` alone leaves that change in place. Any coordinated-rollback narrative for this stack needs to explicitly call out config/binding drift as a second thing that must be -manually reverted — it is not automatic. +manually reverted. It is not automatic. ## Agentic-safety notes for this stack diff --git a/skills/prod-readiness-coach/references/stacks/convex.md b/skills/prod-readiness-coach/references/stacks/convex.md index e6b3ee1..a51bf8c 100644 --- a/skills/prod-readiness-coach/references/stacks/convex.md +++ b/skills/prod-readiness-coach/references/stacks/convex.md @@ -14,26 +14,26 @@ Convex ships two distinct execution runtimes: opt-out. Actions also run here by default ([Convex runtimes docs](https://docs.convex.dev/functions/runtimes)). - **Opt-in Node.js runtime:** enabled only by putting the `"use node"` directive at the very - top of an *action* file. Queries and mutations can **never** use `"use node"` — that + top of an *action* file. Queries and mutations can **never** use `"use node"`. That directive is action-only ([Convex actions docs](https://docs.convex.dev/functions/actions)). Implication for the audit: if the Structured Logging & Observability category recommends a Node-native logging or APM SDK (pino, `@sentry/node`, most classic Node instrumentation), that recommendation is only valid for action files that have opted into `"use node"`. It is -categorically invalid for any query, mutation, or default-runtime action — those need a +categorically invalid for any query, mutation, or default-runtime action, those need a fetch-based or edge-compatible transport instead. Don't let a generic "add a Node logger" recommendation pass through unqualified for a Convex repo. ## The cost of moving to Node-runtime actions -Node-runtime actions lose direct database access — no `ctx.db` calls. They must instead go +Node-runtime actions lose direct database access, no `ctx.db` calls. They must instead go through `ctx.runQuery` / `ctx.runMutation`, which round-trips back into the default runtime and adds overhead. Convex's own best-practices guidance explicitly warns against overusing `runAction` for this reason. Treat "just add `"use node"` everywhere" as a bad fix -recommendation — it should be scoped narrowly to the specific actions that actually need a +recommendation. It should be scoped narrowly to the specific actions that actually need a Node-only dependency. -Node-runtime actions also require a matching Node version — the default is Node 20 +Node-runtime actions also require a matching Node version, the default is Node 20 ([Convex local deployments docs](https://docs.convex.dev/cli/local-deployments)). If a fix brief adds a Node-only dependency to an action, note the Node version constraint so it doesn't silently break in a different local dev environment. @@ -41,13 +41,12 @@ silently break in a different local dev environment. ## Rollback and the paired-surface trap Convex does not have a single documented CLI "rollback deployment" command equivalent to -`vercel rollback` in the same sense — a `convex deploy` pushes both function code and any -schema changes together. When Convex is paired with a frontend host (most commonly Vercel — -see the `nextjs-vercel` adapter, and note this repo's own `vercel.json` typically runs +`vercel rollback` in the same sense, a `convex deploy` pushes both function code and any +schema changes together. When Convex is paired with a frontend host (most commonly Vercel, see the `nextjs-vercel` adapter, and note this repo's own `vercel.json` typically runs `npx convex deploy` as part of the Vercel build step), rolling back the frontend via `vercel rollback` does **not** roll back whatever the Convex deploy already applied to the backend schema/functions. This is the canonical instance of the "paired rollback trap" that -the Multi-Surface Deployment & Coordinated Rollback category is designed to catch — when +the Multi-Surface Deployment & Coordinated Rollback category is designed to catch, when writing that category's narrative for a Convex+Vercel repo, be specific: state that a Vercel rollback reverts the frontend to an older version that may now be calling a Convex schema/API shape that no longer matches, and that the coordinated procedure must address which side moves @@ -60,7 +59,7 @@ first and what compatibility window is required between them. (or the equivalent deploy key env var for the target deployment) is set so the command runs non-interactively rather than assuming it will proceed unattended. - Do not assume a Convex project is on a paid tier with extended log retention, scheduled - function limits, or other quota-gated features when writing recommendations — Convex's free + function limits, or other quota-gated features when writing recommendations, Convex's free tier has real limits on things like function execution time and scheduled job frequency; flag tier-dependent recommendations as needing confirmation rather than presenting them as unconditionally available. diff --git a/skills/prod-readiness-coach/references/stacks/fly.md b/skills/prod-readiness-coach/references/stacks/fly.md index dbfe301..1774aea 100644 --- a/skills/prod-readiness-coach/references/stacks/fly.md +++ b/skills/prod-readiness-coach/references/stacks/fly.md @@ -13,7 +13,7 @@ dedicated rollback command exists [#4586](https://community.fly.io/t/manual-rollback-to-earlier-version/4586), [#24312](https://community.fly.io/t/how-to-delete-old-releases/24312)). A third-party wrapper CLI ([fly-rollback-cli](https://github.com/sudhanshug16/fly-rollback-cli)) exists specifically -to fill this gap — its existence is itself evidence that the platform gap is real and commonly +to fill this gap, its existence is itself evidence that the platform gap is real and commonly felt. **Never write a fix brief or manual-steps list that says "run `fly rollback`."** That command @@ -23,8 +23,8 @@ does not exist and an agent or human following the brief will hit an error. Per [Fly's own rollback guide](https://fly.io/docs/blueprints/rollback-guide/): -1. `fly releases --image` — lists previous release image hashes for the app. -2. `fly deploy -i ` — redeploys that exact previous image. +1. `fly releases --image` lists previous release image hashes for the app. +2. `fly deploy -i ` redeploys that exact previous image. This is a full redeploy of an old image, not an instant routing-layer switch like Vercel's rollback. Write fix briefs and runbook recommendations using this exact two-step procedure, @@ -34,7 +34,7 @@ not a generic "roll back to the previous deploy" instruction. Fly apps commonly run schema migrations via `release_command` in `fly.toml`, executed once per deploy before the new release goes live. Redeploying an older image via the procedure above -does **not** reverse a migration that already ran against the database — the database state +does **not** reverse a migration that already ran against the database, the database state stays at whatever the most recent `release_command` left it at. If this repo's `stack_fingerprint.migration_tooling` shows any migration tool alongside a `fly` deploy surface, treat this as a first-class multi-surface risk: the "surfaces" here are the app @@ -47,5 +47,5 @@ rather than folding it into a generic "document your rollback procedure" note. contexts, authenticate via the `FLY_API_TOKEN` environment variable, not an interactive `fly auth login` (which opens a browser flow). - Before recommending `fly deploy -i ` as an automated remediation step, confirm the - target image hash actually exists in `fly releases --image` output for this app — don't + target image hash actually exists in `fly releases --image` output for this app, don't fabricate or guess an image reference. diff --git a/skills/prod-readiness-coach/references/stacks/netlify.md b/skills/prod-readiness-coach/references/stacks/netlify.md index d7b9ae9..4a4606b 100644 --- a/skills/prod-readiness-coach/references/stacks/netlify.md +++ b/skills/prod-readiness-coach/references/stacks/netlify.md @@ -11,22 +11,21 @@ Load this file when `stack_fingerprint.adapters_matched` includes `netlify`. ([Netlify docs](https://docs.netlify.com/deploy/manage-deploys/manage-deploys-overview/)). - **CLI/API:** there is no dedicated `netlify rollback` command. The documented mechanism is the generic escape-hatch API call: `netlify api restoreSiteDeploy --site-id=... --deploy-id=...`. - If a fix brief needs a scriptable rollback step for Netlify, use this exact command form — - don't invent a `netlify rollback` subcommand that doesn't exist. + If a fix brief needs a scriptable rollback step for Netlify, use this exact command form, don't invent a `netlify rollback` subcommand that doesn't exist. ## Important nuance: a failed build has nothing to roll back -If the most recent deploy attempt failed to build, it never went live — the previous good +If the most recent deploy attempt failed to build, it never went live. The previous good deploy is still serving traffic. In that case there is nothing to "roll back": the correct action is to fix the failing build, not to publish an older deploy that's already live ([Netlify deploy skill reference](https://tessl.io/registry/skills/github/netlify/context-and-tools/netlify-deploy)). -Don't recommend a rollback procedure as the fix for a failed-build incident — diagnose the +Don't recommend a rollback procedure as the fix for a failed-build incident, diagnose the build failure instead. ## The paired rollback trap, Netlify edition If this repo uses Netlify's managed database (Netlify DB), publishing a previous deploy does -**not** automatically restore the database from a backup — reverting the app's deployed code +**not** automatically restore the database from a backup, reverting the app's deployed code and reverting its data are two separate actions ([Netlify backup/recovery docs](https://docs.netlify.com/build/data-and-storage/netlify-database/backup-and-recovery/)). This is the same failure pattern as the Vercel+Convex trap covered in the `convex` and @@ -39,4 +38,4 @@ category narrative rather than a generic "make sure backups exist" line. - CI/agentic Netlify CLI usage should authenticate via the `NETLIFY_AUTH_TOKEN` environment variable, not an interactive `netlify login`. - Confirm a deploy actually succeeded (reached "Published" state) before treating it as a - valid rollback target — don't assume the most recent entry in deploy history was live. + valid rollback target, don't assume the most recent entry in deploy history was live. diff --git a/skills/prod-readiness-coach/references/stacks/nextjs-vercel.md b/skills/prod-readiness-coach/references/stacks/nextjs-vercel.md index cd47532..59ea572 100644 --- a/skills/prod-readiness-coach/references/stacks/nextjs-vercel.md +++ b/skills/prod-readiness-coach/references/stacks/nextjs-vercel.md @@ -4,13 +4,12 @@ _last_verified: 2026-08-20_. Platform facts below were checked against vendor do Load this file when `stack_fingerprint.adapters_matched` includes `nextjs-vercel` (requires both the `vercel` deploy surface AND the `nextjs` framework detected). -Use it to write accurate, stack-specific prose on top of the generic audit findings — -do not repeat the generic checklist language when this file has a more precise fact. +Use it to write accurate, stack-specific prose on top of the generic audit findings, do not repeat the generic checklist language when this file has a more precise fact. ## Rollback mechanics - The rollback command is `vercel rollback `. It operates at the - routing layer only — no rebuild — and typically completes in about 60 seconds + routing layer only, no rebuild, and typically completes in about 60 seconds ([Vercel CLI rollback docs](https://vercel.com/docs/cli/rollback)). - **Plan gate:** Hobby plan can only roll back to the *immediately previous* production deployment. Pro/Enterprise can target any past deployment. Do not write a fix brief that @@ -19,8 +18,7 @@ do not repeat the generic checklist language when this file has a more precise f - `vercel promote ` undoes a rollback or promotes any deployment; add `--yes` to skip the interactive confirmation prompt when this needs to run non-interactively (CI, or an agent acting on the user's behalf). -- Because Instant Rollback does not rebuild, it does **not** refresh environment variables — - the rolled-back deployment runs with whatever env vars were baked in at that deployment's +- Because Instant Rollback does not rebuild, it does **not** refresh environment variables. The rolled-back deployment runs with whatever env vars were baked in at that deployment's *original* build time. If an env var was rotated since, the rolled-back version is running with the old value. Flag this explicitly if a fix brief mentions secret rotation. @@ -31,7 +29,7 @@ BaaS (see the `convex` adapter if Convex is also detected), or any external serv ([documented explicitly here](https://salsadocs.vercel.app/docs/deployment/updates), under "Database Migrations Are Not Rolled Back"). If the fingerprint shows `multi_surface: true` with Vercel plus any backend surface, this is exactly the failure mode the Multi-Surface -Deployment & Coordinated Rollback category exists to catch — cite this fact directly in that +Deployment & Coordinated Rollback category exists to catch, cite this fact directly in that category's narrative rather than a generic "make sure your rollback plan is documented" line. ## Observability gotchas specific to this stack @@ -39,7 +37,7 @@ category's narrative rather than a generic "make sure your rollback plan is docu - **Sentry's setup command is an interactive CLI wizard:** `npx @sentry/wizard@latest -i nextjs` ([Sentry Next.js docs](https://docs.sentry.io/platforms/javascript/guides/nextjs/)). It prompts for project/org selection. Never suggest running this command inside an agentic or - CI pipeline step expecting it to complete unattended — it will hang waiting for input. If a + CI pipeline step expecting it to complete unattended. It will hang waiting for input. If a fix brief needs Sentry added, either instruct a human to run the wizard interactively once, or hand-write the three runtime config files it would have generated (client, server, edge). - Sentry's own SDK is edge-safe: it supports `enableLogs: true` across all three Next.js @@ -56,7 +54,7 @@ category's narrative rather than a generic "make sure your rollback plan is docu ([debugging writeup](https://medium.com/@sibteali786/debugging-pino-logger-issues-in-a-next-js-4e0c3368ef14)). If the audit's Structured Logging check recommends "adopt pino/winston" and this repo has any edge middleware or edge-runtime routes, downgrade that specific recommendation to - Sentry's own logging or a fetch-based transport instead — don't repeat the generic advice + Sentry's own logging or a fetch-based transport instead, don't repeat the generic advice verbatim. ## Agentic-safety notes for this stack @@ -66,7 +64,7 @@ category's narrative rather than a generic "make sure your rollback plan is docu ([GitHub docs](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches), confirmed in [this community thread](https://github.com/orgs/community/discussions/190190)). Never assume a CI/CD or release-gating recommendation that depends on branch protection is - achievable for a private repo without confirming the org's GitHub plan first — probe (`gh api` + achievable for a private repo without confirming the org's GitHub plan first, probe (`gh api` or ask) or list it as a manual, plan-dependent item rather than an automatable fix. - Vercel CLI actions in CI should authenticate via `VERCEL_TOKEN`, not an interactive - `vercel login` — the latter opens a browser flow that hangs non-interactively. + `vercel login`. The latter opens a browser flow that hangs non-interactively. diff --git a/skills/prod-readiness-coach/scripts/audit/fingerprint.py b/skills/prod-readiness-coach/scripts/audit/fingerprint.py index e9640d9..7a73126 100644 --- a/skills/prod-readiness-coach/scripts/audit/fingerprint.py +++ b/skills/prod-readiness-coach/scripts/audit/fingerprint.py @@ -30,6 +30,7 @@ class StackFingerprint: waiver_problems: list = field(default_factory=list) profile: str = "web-app" profile_source: str = "default" # "given" | "guessed" | "default" + is_git_repo: bool = True multi_surface: bool = False multi_surface_evidence: list[str] = field(default_factory=list) adapters_matched: list[str] = field(default_factory=list) @@ -78,6 +79,7 @@ def parse_compose_services(content: str) -> dict[str, str]: def detect_stack_fingerprint(repo: Repo) -> StackFingerprint: fp = StackFingerprint() + fp.is_git_repo = repo.is_git_repo() pkg = repo.package_json() deps = {**pkg.get("dependencies", {}), **pkg.get("devDependencies", {})} diff --git a/skills/prod-readiness-coach/scripts/audit/report.py b/skills/prod-readiness-coach/scripts/audit/report.py index d00e816..86bc2a8 100644 --- a/skills/prod-readiness-coach/scripts/audit/report.py +++ b/skills/prod-readiness-coach/scripts/audit/report.py @@ -59,6 +59,14 @@ def render_markdown(categories: list[Category], repo_name: str, fp: Optional[Sta "which stack-specific reference material gets applied on top of this report." ) lines.append("") + if not fp.is_git_repo: + lines.append( + "> **This folder is not a git repository.** Files were found by walking the " + "directory, so `.gitignore` was not applied and build output or dependencies " + "may have been read as source. Checks about tracked files and untracked files " + "cannot run. Run `git init` and commit before trusting this report." + ) + lines.append("") lines.append(f"- **Languages:** {', '.join(fp.languages) or '_none detected_'}") lines.append(f"- **Package manager(s):** {', '.join(fp.package_managers) or '_none detected_'}") lines.append(f"- **Frameworks:** {', '.join(fp.frameworks) or '_none detected_'}") diff --git a/skills/prod-readiness-coach/scripts/prod_audit.py b/skills/prod-readiness-coach/scripts/prod_audit.py index e55f0e9..394eaf0 100644 --- a/skills/prod-readiness-coach/scripts/prod_audit.py +++ b/skills/prod-readiness-coach/scripts/prod_audit.py @@ -50,9 +50,20 @@ def main(): "skipped in scoring. Guessed from the stack when omitted (strict when unsure).") args = parser.parse_args() + HINT = ("Check the path, or pass `--repo .` to audit the directory you are in.") repo_path = Path(args.repo) if not repo_path.exists(): - print(f"error: repo path does not exist: {repo_path}", file=sys.stderr) + print(f"error: repo path does not exist: {repo_path}\n{HINT}", file=sys.stderr) + sys.exit(2) + if not repo_path.is_dir(): + print(f"error: --repo must be a directory, and {repo_path} is a file.\n{HINT}", + file=sys.stderr) + sys.exit(2) + # Grading an empty folder produces a confident score about nothing, which is + # the exact failure this tool exists to catch. Refuse instead. + if not Repo(repo_path).git_files(): + print(f"error: {repo_path} has no files to audit. Nothing was scanned, so no " + f"score would mean anything.\n{HINT}", file=sys.stderr) sys.exit(2) categories, fingerprint = run_audit(repo_path, args.profile) diff --git a/skills/prod-readiness-coach/tests/test_prod_audit.py b/skills/prod-readiness-coach/tests/test_prod_audit.py index 0579359..752d3db 100644 --- a/skills/prod-readiness-coach/tests/test_prod_audit.py +++ b/skills/prod-readiness-coach/tests/test_prod_audit.py @@ -712,5 +712,59 @@ def test_package_json_still_merges_workspace_members(self): self.assertIn("next", pkg["dependencies"]) +class RefusesToGradeNothing(unittest.TestCase): + """Grading an empty folder is the same lie as grading a file that is not there.""" + + SCRIPT = str(Path(__file__).resolve().parent.parent / "scripts" / "prod_audit.py") + + def run_cli(self, root: Path, *extra): + return subprocess.run([sys.executable, self.SCRIPT, "--repo", str(root), + "--output", "/dev/null", *extra], + capture_output=True, text=True) + + def test_an_empty_directory_is_refused_not_graded(self): + out = self.run_cli(make_repo({})) + self.assertNotEqual(out.returncode, 0) + self.assertIn("no files", (out.stderr + out.stdout).lower()) + self.assertNotIn("Score:", out.stdout) + + def test_the_refusal_says_what_to_do_next(self): + out = self.run_cli(make_repo({})) + self.assertIn("--repo", out.stderr) + + def test_a_missing_path_says_what_to_do_next(self): + out = self.run_cli(Path("/tmp/prod-audit-no-such-path-xyz")) + self.assertEqual(out.returncode, 2) + self.assertIn("--repo", out.stderr) + + def test_a_directory_with_files_but_no_git_still_runs_and_says_so(self): + root = make_repo({"package.json": "{}", "index.js": "console.log(1)"}) + out = self.run_cli(root, "--output", "/dev/null") + self.assertEqual(out.returncode, 0) + + def test_the_report_flags_that_it_is_not_a_git_repository(self): + md = audit_markdown({"package.json": "{}", "index.js": "console.log(1)"}) + self.assertIn("not a git repository", md.lower()) + + +class HouseStyleAppliesToOurOwnFiles(unittest.TestCase): + """check_report.py rejects em and en dashes in generated docs. The toolkit + that enforces that rule cannot ship them itself.""" + + ROOT = Path(__file__).resolve().parents[3] + + def test_no_em_or_en_dashes_in_shipped_markdown(self): + offenders = [] + for base in ("skills", "commands", "templates"): + d = self.ROOT / base + if not d.exists(): + continue + for f in d.rglob("*.md"): + text = f.read_text(encoding="utf-8", errors="ignore") + if "\u2014" in text or "\u2013" in text: + offenders.append(str(f.relative_to(self.ROOT))) + self.assertEqual(offenders, [], f"em or en dash in shipped text: {offenders}") + + if __name__ == "__main__": unittest.main()