From 240ea407a4282b40b8615657e8f4d6bb2a5e4d28 Mon Sep 17 00:00:00 2001 From: Tarik Moody Date: Sat, 22 Aug 2026 18:12:53 -0500 Subject: [PATCH] fix: the engine only knew Node (3.5.1) Five defects with one cause underneath them. Two were saying something false about any repository a user ran them on today. - auth-2 now recognizes middleware, webhook signature verification and shared secrets. Where matcher-based middleware exists it reports a LOW "confirm the matcher covers these" instead of a HIGH "these are unguarded", and lists the public patterns. On the live test app the finding drops from 18 of 25 to 9 of 25, all genuinely middleware- dependent. auth-3 unchanged. - package_json() returns {} with no manifest. The truthy empty dict made seven callers believe a package.json existed, so Go repos were told "package.json exists but does not define a test script". - Python (FastAPI) and Go fixtures added. They immediately found two more: PEP 621 dependencies quoted inside a pyproject.toml array parsed as nothing, and health routes declared in code rather than as a file path. Both fixed; both dependency call sites now share one parser. - ci-3 recognizes mypy, golangci-lint, go vet, gofmt, staticcheck, rubocop, clippy, cargo fmt, dotnet format and nine more. - The "any git repo regardless of language" claim is replaced in SKILL.md and README.md with what is actually so, naming the four gaps. - Both readiness skill descriptions now open with the real split: repository controls versus user journeys. The coach's lists Access Control, missing since 3.5.0. - Tests must now prove a check stays quiet, not only that it fires. Four silence tests on auth-2. 68 to 86 tests. Decisions 006 and 007 record the two judgement calls. --- .claude-plugin/marketplace.json | 4 +- .claude-plugin/plugin.json | 2 +- CHANGELOG.md | 20 ++ README.md | 2 +- .../006-auth-2-middleware-and-signatures.md | 19 ++ docs/decisions/007-supported-languages.md | 19 ++ skills/prod-readiness-coach/SKILL.md | 31 ++- .../scripts/audit/checks_access.py | 107 +++++++++-- .../scripts/audit/checks_build.py | 20 +- .../scripts/audit/checks_runtime.py | 19 +- .../scripts/audit/repo.py | 27 ++- .../tests/test_prod_audit.py | 179 ++++++++++++++++++ skills/product-readiness-review/SKILL.md | 2 +- 13 files changed, 418 insertions(+), 33 deletions(-) create mode 100644 docs/decisions/006-auth-2-middleware-and-signatures.md create mode 100644 docs/decisions/007-supported-languages.md diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index c86d13f..1a267ef 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.0", + "version": "3.5.1", "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.0", + "version": "3.5.1", "author": { "name": "Tarik Moody" }, diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 68e8bee..7abfe7a 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "clean-code-toolkit", - "version": "3.5.0", + "version": "3.5.1", "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 04fb936..7d81c0c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,25 @@ # Changelog +## 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. + +**`auth-2` was wrong about half the time it spoke.** On a live application it reported "18 of 25 request handlers never mention an auth check" as a HIGH finding. At least 14 of the flagged files were authenticated, by three mechanisms a word search could not see: middleware that guards every route not on a public list, webhook routes that verify a cryptographic signature because the caller is a machine and cannot hold a browser session, and routes that compare a shared secret from the environment. The check now recognizes all three. When it finds request middleware it stops asserting the handlers are unguarded and instead reports, at LOW, that they rely on the middleware, naming the file and listing the public route patterns it found so a person can confirm the matcher in about a minute. On the same application the finding is now 9 of 25, and each of the 9 is genuinely middleware-dependent. `auth-3`, which found a real fail-open owner check, is unchanged. See `docs/decisions/006`. + +**`package_json()` broke every repository that is not JavaScript.** A monorepo change in 3.2.3 made it always return a dict of empty sections, and that dict is truthy, so all seven `if pkg:` callers thought a manifest existed. A Go project was reported as `languages: ['go', 'javascript']` and told, at HIGH, "package.json exists but does not define a `test` script", about a file that was not there. It now returns nothing when there is no manifest and no workspace members. + +**Python and Go fixtures, and the two failures they found immediately.** Every meaningful fixture in the suite was a JavaScript repository, which is why none of the above was caught. Adding a FastAPI service and a Go service surfaced two more on the first run: dependencies declared PEP 621 style in `pyproject.toml`, quoted inside an array, were parsed as nothing, so `fastapi-users` was invisible and the service was told it had no authentication at all. Both call sites now share one dependency-name parser. And a health route declared in code, `@app.get("/health")` or `r.Get("/health", ...)`, which is how every framework except Next.js declares one, was invisible to a check that only globbed file paths. It now reads code declarations too, marked as weak evidence because a path in a string is not proof the route answers. + +**Lint and typecheck steps outside Node.** `ci-3` knew `eslint`, `ruff`, `flake8`, `pylint`, `tsc` and `typecheck`. A pipeline running `mypy app` was told it had no lint or typecheck step. Added: `mypy`, `pyright`, `black --check`, `golangci-lint`, `go vet`, `gofmt`, `staticcheck`, `rubocop`, `clippy`, `cargo fmt`, `dotnet format`, `ktlint`, `detekt`, `checkstyle`, `phpstan`, `psalm`, `biome`, `oxlint`, `prettier --check`. + +**The "any language" claim is retired.** `SKILL.md` said "It works on any git repo regardless of language/framework." Repository-level checks do. Stack-specific ones are strongest on Node and Next.js, good on Python, and have no rules for Go, Rust, Ruby, Java, PHP or C#. Both `SKILL.md` and `README.md` now say which is which and name the four specific gaps. This toolkit's whole thesis is that a tool must not overstate what it checked; that sentence was the tool overstating where it looked. See `docs/decisions/007`. + +**The two readiness skills are distinguishable at a glance.** Both descriptions used to open with audit and production-readiness language, and a tester reported that Claude could not reliably choose between them. They now open with the actual split: `prod-readiness-coach` scans repository controls and scripts, `product-readiness-review` judges user journeys and product behavior. The coach's description also lists Access Control, which has been one of its nine categories since 3.5.0 and was missing from the list. + +**Tests now have to prove a check stays quiet.** Every one of the 68 existing tests asked only "does this check fire". None asked "does it stay silent when it should", which is precisely the hole `auth-2` fell through. Borrowed from KICS, which refuses a rule that ships without a negative fixture, and from Checkov, which requires both a passing and a failing case. `auth-2` now has four silence tests: middleware-protected, signature-verified, shared-secret, and correctly-fired. + +Tests: 86. + ## 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. diff --git a/README.md b/README.md index 53af7c2..b2b2569 100644 --- a/README.md +++ b/README.md @@ -86,7 +86,7 @@ It reads files, so it can tell you a control is missing or that a guard lets eve Both ask "is it ready?" They answer different halves. -- **`prod-readiness-coach`** runs a script. Same repo, same findings every time. It checks the plumbing: automatic tests, error alerts, secrets, undo plans, and whether a rollback on one platform leaves another out of sync. Run it first, the day you go live, and again before any launch. +- **`prod-readiness-coach`** runs a script. Same repo, same findings every time. It checks the plumbing: automatic tests, error alerts, secrets, undo plans, and whether a rollback on one platform leaves another out of sync. The checks that read your repository work in any language. The checks that need to know your stack are strongest on Node and Next.js, good on Python, and tell you plainly when they have no rules for your language instead of guessing. Run it first, the day you go live, and again before any launch. - **`product-readiness-review`** is Claude's judgment. Does the product do what it says? What would break first for a real person? Run it after the coach, when the plumbing is in. ## What CI is, if nobody ever told you diff --git a/docs/decisions/006-auth-2-middleware-and-signatures.md b/docs/decisions/006-auth-2-middleware-and-signatures.md new file mode 100644 index 0000000..fe534b5 --- /dev/null +++ b/docs/decisions/006-auth-2-middleware-and-signatures.md @@ -0,0 +1,19 @@ +# 006: `auth-2` reports middleware as something to confirm, not as a failure + +**Decision.** `auth-2` no longer calls a request handler unguarded just because the word "auth" is missing from the file. It now recognizes three ways a handler can be authenticated, and when it finds request middleware, it drops from a HIGH failure to a LOW warning that names the middleware and lists its public route patterns. + +**Why this came up.** On a live application, `auth-2` reported "18 of 25 request handlers never mention an auth check" as a HIGH finding. Reading the flagged files, at least 14 of them were authenticated. Three mechanisms the check could not see: middleware that guards every route not on a public list, webhook routes that verify a cryptographic signature because a machine is calling and cannot carry a browser session, and routes that compare a shared secret from the environment. What was at stake: a HIGH finding that is wrong about half the time is worse than no finding, because it teaches people to scroll past the real ones. The same report contained a genuine critical finding, `auth-3`, sitting underneath eighteen false ones. + +**Options.** +1. Delete `auth-2` until it can be right. Cost: a genuinely unguarded handler, which is the most common way a small app leaks data, goes unreported entirely. +2. Teach it the three mechanisms and keep the HIGH severity. Cost: a file scan still cannot prove a middleware route pattern covers a given path, because those patterns are globs evaluated when a request arrives. The check would still be asserting something it cannot know. +3. Teach it the three mechanisms, and when middleware exists, change what the finding claims: not "these are unguarded" but "these rely on middleware, confirm the matcher covers them". Drop the severity, and print the public patterns so a person can check in about a minute. Cost: a route that middleware genuinely misses is now a LOW warning rather than a HIGH failure, and a hurried reader may skip it. + +**What we chose and why.** Option 3. Joint call: the handoff leaned this way and Claude implemented it. The deciding argument is the toolkit's own rule, that a tool must never claim more than it checked. The tool checked that middleware exists. It did not check that the matcher covers these paths. So that is exactly what the finding should say. The severity follows the certainty, not the topic. + +**What we gave up.** Loudness on a real class of bug. If someone adds a route that their middleware matcher happens to exclude, this now arrives as a LOW warning in a list, not a HIGH finding at the top. We accepted that because the alternative was eighteen false HIGH findings burying one true CRITICAL. + +**How we'll know if this was right.** On the live test application the finding goes from 18 flagged to 9, and each of the 9 is genuinely middleware-dependent rather than authenticated some other way. Longer term: nobody reports a handler that this warning mentioned and they dismissed, which turned out to be open. + +**What actually happened.** +(Tarik fills this in.) diff --git a/docs/decisions/007-supported-languages.md b/docs/decisions/007-supported-languages.md new file mode 100644 index 0000000..9f118ab --- /dev/null +++ b/docs/decisions/007-supported-languages.md @@ -0,0 +1,19 @@ +# 007: Node and Next.js and Python are supported. Everything else gets the checks that do not care. + +**Decision.** The tool no longer claims to work on any repository regardless of language. Checks split into two kinds. Checks that read the repository itself, is there CI, does it gate pull requests, is `.env` ignored, is there a runbook with real words in it, run on anything. Checks that need to understand the stack are supported on Node, Next.js and Python only, and on any other language they say what they could not check instead of guessing. + +**Why this came up.** Five separate bugs turned out to be one bug wearing different hats: the engine only really knew Node. A helper returned a fake empty `package.json`, which is truthy in Python, so a Go repository was told "package.json exists but does not define a test script" about a file that was not there. The lint-step check knew `eslint` and `ruff` but not `mypy` or `golangci-lint`. The health-endpoint check could only find a route that was a file path, which is how Next.js declares one and how no other framework does. Sixty-eight tests caught none of it, because every fixture in the suite was a JavaScript repository. What was at stake: the tool's entire value is that it does not overstate what it looked at, and the sentence "it works on any git repo regardless of language/framework" was the tool overstating where it looked. + +**Options.** +1. Keep the "any language" claim and keep patching. Cost: every new language is an edit in five files, the claim stays false in between, and the tool earns a reputation for lying about ecosystems it never supported. +2. Support Node, Next.js, Python and Go properly. Cost: Go is a fourth surface to keep correct, and Go is not who this tool is for. Its audience is self-taught developers shipping AI-assisted apps, which in practice means Next.js and Python. +3. Support Node, Next.js and Python. Split the checks so the language-agnostic ones still run everywhere, and make "no rules for Go" an explicit, named outcome that is left out of the score rather than counted as a failure. + +**What we chose and why.** Option 3. Tarik's call, after research into how seven mature audit tools handle the same problem. OpenSSF Scorecard has a third outcome besides pass and fail, an inconclusive result scored `-1`, used when a check does not apply or the repository's language is unsupported, and it is excluded from the aggregate rather than penalized. Checkov has the same idea under the name `UNKNOWN`. That is the honest shape, and this codebase already had the machinery: an `n/a` status that is dropped from scoring. + +**What we gave up.** The broad claim, which was the more impressive-sounding one. A Go or Ruby user now gets a smaller report. That is the correct trade: a smaller true report beats a larger one with invented findings in it. + +**How we'll know if this was right.** A Go repository's report contains no statement about a file that does not exist, and names what it skipped. Adding a language later is one PR that adds an applicability tag and a fixture pair, not an edit across five files. + +**What actually happened.** +(Tarik fills this in.) diff --git a/skills/prod-readiness-coach/SKILL.md b/skills/prod-readiness-coach/SKILL.md index b873fb0..53a5964 100644 --- a/skills/prod-readiness-coach/SKILL.md +++ b/skills/prod-readiness-coach/SKILL.md @@ -1,6 +1,6 @@ --- name: prod-readiness-coach -description: "Audits any code repository for production-readiness gaps (CI/CD pipeline, structured logging, error tracking, secrets management, CLAUDE.md/AGENTS.md, resilience/runbooks, multi-surface deployment and coordinated rollback risk, irreversible migrations, test coverage, dependency security) and produces two plain-English documents: a beginner-friendly audit explainer and a phase-gated Claude Code fix brief. Detects the repo's stack (frameworks, deploy platforms, runtimes) and loads matching stack-specific reference material so advice reflects real platform gotchas instead of generic checklist language. Use when a user asks to audit, check, or review a repo for production readiness, best practices, or launch-blockers, or asks to turn code-quality/DevOps findings into something a non-expert or self-taught (\"vibe coder\") developer can understand and act on. Use for repository controls and static scanning; run it first on broad production-readiness requests. Do not use alone to judge user journeys or product correctness; that is product-readiness-review." +description: "Scans repository controls and scripts, not user journeys: access control on request handlers, CI/CD pipeline, structured logging, error tracking, secrets management, CLAUDE.md/AGENTS.md, resilience and runbooks, multi-surface deployment and coordinated rollback risk, irreversible migrations, test coverage, and dependency security. Runs a deterministic script, then produces two plain-English documents: a beginner-friendly audit explainer and a phase-gated Claude Code fix brief. Detects the repo's stack (frameworks, deploy platforms, runtimes) and loads matching stack-specific reference material so advice reflects real platform gotchas instead of generic checklist language. Repository-level checks run on any language; stack-specific checks are strongest on Node and Next.js, good on Python, and report what they could not check elsewhere. Use when a user asks to audit, check, or review a repo for production readiness, best practices, or launch-blockers, or asks to turn code-quality/DevOps findings into something a non-expert or self-taught (\"vibe coder\") developer can understand and act on. Run it first on broad production-readiness requests. Do not use it alone to judge user journeys or product correctness; that is product-readiness-review." --- # Production Readiness Coach @@ -348,9 +348,32 @@ 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 -recognized deploy platform still get a clean fingerprint (empty surfaces, -`multi_surface: false`) rather than a crash. See its own `--help` output +Language coverage is not even, and the report has to say so. Checks that +read the repository itself work on anything: is there CI, does it run on +pull requests, is `.env` ignored, is there a runbook with real words in it, +are there secrets in the code, is there a CLAUDE.md. Checks that need to +know the stack are narrower. They are strongest on Node and Next.js, good +on Python, and have no rules for Go, Rust, Ruby, Java, PHP or C#. + +On a language with no rules, this is what that looks like: + +- `ci-6` (a test command in the manifest) reads a `test` script in + package.json and a pytest configuration, nothing else. On a Go or Rust + repo it reports "no test entry point found" and names the two things it + read, so the reader can see the gap is in the tool. +- `auth-1` looks for an authentication package in package.json, + requirements.txt or pyproject.toml. A Go or Ruby auth library is invisible + to it. +- `auth-2` finds request handlers by Next.js, Express and FastAPI path + shapes. A chi router or a Rails controller is never scanned. +- When no framework is recognized, the profile is "unknown" and every + runtime check reports n/a with "insufficient evidence" rather than + failing. Pass `--profile api` to run them anyway. + +It never crashes on an unfamiliar repo, and it never invents a finding +about a file that is not there. Repos with no recognized deploy platform +still get a clean fingerprint (empty surfaces, `multi_surface: false`). +What it did not check, it says it did not check. 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 diff --git a/skills/prod-readiness-coach/scripts/audit/checks_access.py b/skills/prod-readiness-coach/scripts/audit/checks_access.py index 9f2b362..6a7864c 100644 --- a/skills/prod-readiness-coach/scripts/audit/checks_access.py +++ b/skills/prod-readiness-coach/scripts/audit/checks_access.py @@ -25,9 +25,65 @@ r"getToken|clerkClient|withAuth|ensureAuthenticated)\b", re.IGNORECASE) ROUTE_GLOBS = [ "**/app/api/**/route.ts", "**/app/api/**/route.js", + "**/app/api/**/route.tsx", "**/app/api/**/route.jsx", "**/pages/api/**/*.ts", "**/pages/api/**/*.js", "**/routes/**/*.ts", "**/routes/**/*.js", "**/api/**/*.py", ] +# A route can be authenticated without a session. These are the two ways a +# machine caller proves itself, and the first version of auth-2 could see +# neither, so it called signed webhooks unguarded. +SIGNATURE_RX = re.compile( + r"\bsvix\b|createHmac|timingSafeEqual|compare_digest|hmac" + r"|x-[\w-]*-signature|signature[-_]?header|verifyHeader|verify_header" + r"|constructEvent|verifyKey|nacl\.sign|ed25519", + re.IGNORECASE) +SHARED_SECRET_RX = re.compile( + r"(?:process\.env\.|import\.meta\.env\.|os\.environ(?:\.get)?[\[(]\s*[\'\"]|os\.getenv\(\s*[\'\"])" + r"[A-Za-z0-9_]*(?:SECRET|TOKEN|API_?KEY|PASSWORD)", + re.IGNORECASE) +# Auth that runs before the handler does. A file scan cannot prove a matcher +# covers a given route, so finding one lowers the claim rather than clearing it. +MIDDLEWARE_GLOBS = [ + "middleware.ts", "middleware.js", "src/middleware.ts", "src/middleware.js", + "proxy.ts", "proxy.js", "src/proxy.ts", "src/proxy.js", + "**/middleware.py", "**/middleware.ts", "**/middleware.js", +] +MIDDLEWARE_RX = re.compile( + r"clerkMiddleware|createRouteMatcher|authMiddleware|auth\.protect\(|withAuth\(" + r"|app\.use\s*\(\s*[\w.]*(?:auth|passport|requireAuth|ensureAuth|jwt)" + r"|add_middleware\s*\(|AuthenticationMiddleware" + r"|dependencies\s*=\s*\[\s*Depends|before_request|before_action", + re.IGNORECASE) +PUBLIC_PATTERN_RX = re.compile(r"[\'\"`](/[^\'\"`\s]*)[\'\"`]") + + +def _route_auth_mechanism(text: str) -> str: + """Which in-file mechanism, if any, proves who is calling. '' when none.""" + if GUARD_RX.search(text): + return "session or identity check" + if SIGNATURE_RX.search(text): + return "webhook signature verification" + if SHARED_SECRET_RX.search(text): + return "shared secret" + return "" + + +def _matcher_middleware(repo: Repo) -> list[tuple[str, list[str]]]: + """Files that guard requests before the handler, with any public paths listed.""" + found = [] + candidates = [f for f in repo.find_any(MIDDLEWARE_GLOBS) if "node_modules" not in f] + candidates += [f for f in repo.code_files() + if f not in candidates and re.search(r"(?:^|/)(?:app|main|server|index)\.(?:py|ts|js)$", f)] + for f in candidates: + text = repo.read(f) + if not MIDDLEWARE_RX.search(text): + continue + public = [] + block = re.search(r"createRouteMatcher\s*\(\s*\[(.*?)\]", text, re.S) + if block: + public = PUBLIC_PATTERN_RX.findall(block.group(1)) + found.append((f, public)) + return found # 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; @@ -67,9 +123,7 @@ def check_access_control(repo: Repo) -> list[CheckResult]: 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("#")} + dep_names |= repo.requirement_names() found = [lib for lib in AUTH_LIBS if lib.lower() in dep_names] results.append(CheckResult( @@ -88,14 +142,44 @@ def check_access_control(repo: Repo) -> list[CheckResult]: 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: + mechanisms = {f: _route_auth_mechanism(repo.read(f)) for f in route_files} + unguarded = [f for f, m in mechanisms.items() if not m] + middleware = _matcher_middleware(repo) + if not unguarded: + kinds = sorted({m for m in mechanisms.values() if m}) + results.append(CheckResult( + "auth-2", "Access Control", "Request handlers consult an identity", + "pass", "info", + f"All {len(route_files)} request handler(s) carry an auth mechanism " + f"({', '.join(kinds)}). This says the check is mentioned, not that it is correct.", + evidence=route_files[:10], + best_practice_ref=REF, + )) + elif middleware: + mw_files = ", ".join(f for f, _ in middleware) + public = sorted({p for _, pats in middleware for p in pats}) + results.append(CheckResult( + "auth-2", "Access Control", "Request handlers consult an identity", + "warn", "low", + f"{len(unguarded)} of {len(route_files)} request handler(s) hold no auth check " + f"of their own. They appear to rely on {mw_files}, which guards requests before " + "a handler runs. Reading files cannot prove that its route patterns cover these " + "paths, so this is a thing to confirm, not a finding.", + "Open the middleware and check its matcher against the handlers listed. Anything " + "the matcher skips has no guard at all. Then call one protected route while " + "signed out and confirm it is refused.", + evidence=[f"middleware: {f}" for f, _ in middleware] + + [f"public pattern: {p}" for p in public[:8]] + + [f"relies on middleware: {f}" for f in unguarded[:8]], + best_practice_ref=REF, + )) + else: 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.", + "auth check, and no request middleware was found that could guard them first. " + "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 " @@ -103,15 +187,6 @@ def check_access_control(repo: Repo) -> list[CheckResult]: 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(): diff --git a/skills/prod-readiness-coach/scripts/audit/checks_build.py b/skills/prod-readiness-coach/scripts/audit/checks_build.py index 3719605..6b66013 100644 --- a/skills/prod-readiness-coach/scripts/audit/checks_build.py +++ b/skills/prod-readiness-coach/scripts/audit/checks_build.py @@ -128,7 +128,17 @@ def check_ci_pipeline(repo: Repo) -> list[CheckResult]: best_practice_ref=ref, )) - lint_patterns = [r"\blint\b", r"eslint", r"ruff", r"flake8", r"pylint", r"tsc\b", r"typecheck"] + # A lint step is a lint step in any language. The first version of this list + # only knew the Node and Python names, so a CI running `mypy app` was told it + # had no typecheck step at all. + lint_patterns = [ + r"\blint\b", r"typecheck", r"\btsc\b", + r"eslint", r"biome\s+(?:check|lint)", r"oxlint", r"prettier\s+--check", + r"\bruff\b", r"flake8", r"pylint", r"\bmypy\b", r"pyright", r"\bblack\s+--check", + r"golangci-lint", r"\bgo\s+vet\b", r"\bgofmt\b", r"staticcheck", + r"rubocop", r"\bclippy\b", r"cargo\s+fmt", r"dotnet\s+format", + r"ktlint", r"detekt", r"checkstyle", r"\bphpstan\b", r"psalm", + ] has_lint_step = any(re.search(p, combined, re.IGNORECASE) for p in lint_patterns) results.append(CheckResult( "ci-3", "CI/CD Pipeline", "Pipeline runs lint / typecheck", @@ -217,8 +227,12 @@ def check_test_scripts_defined(repo: Repo) -> CheckResult: return CheckResult( "ci-6", "CI/CD Pipeline", "Test command defined in project manifest", "warn", "medium", - "Could not determine project manifest / test entry point.", - "Ensure the project has a documented, single-command way to run tests.", + "No test entry point was found. This check reads a `test` script in package.json " + "or a pytest configuration; it has no rules for other ecosystems, so a Go, Rust or " + "Ruby project will land here even when its test command is fine.", + "Ensure the project has a documented, single-command way to run tests, and check " + "the CI findings above, which do look at every ecosystem.", + evidence=["scope: checked package.json scripts.test and pytest configuration"], best_practice_ref=ref, ) diff --git a/skills/prod-readiness-coach/scripts/audit/checks_runtime.py b/skills/prod-readiness-coach/scripts/audit/checks_runtime.py index 3e6eff7..1320f29 100644 --- a/skills/prod-readiness-coach/scripts/audit/checks_runtime.py +++ b/skills/prod-readiness-coach/scripts/audit/checks_runtime.py @@ -29,10 +29,7 @@ def check_structured_logging(repo: Repo) -> list[CheckResult]: pkg = repo.package_json() dep_names = {n.lower() for n in ((pkg.get("dependencies", {}) | pkg.get("devDependencies", {})).keys() if pkg else [])} - # Python requirements: one package per line, strip version specifiers. - 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("#")} + dep_names |= repo.requirement_names() # A dependency named @types/pino is a type stub, not a logger. Match the package # name (allowing a scope prefix), never a substring of a serialized blob. def _installed(lib: str) -> bool: @@ -92,11 +89,25 @@ def _installed(lib: str) -> bool: "*/api/health/*", "*/api/health.*", "*/healthz*", "*/health.*", "app/api/health/route.*", "pages/api/health.*", ]) + # Only Next.js puts a route in the file path. Every other framework declares + # it in code: @app.get("/health"), r.Get("/health", ...), get "/health". + health_decls = repo.grep(r"""["'`]/(?:health|healthz|livez|readyz|_health)["'`/]""") if health_paths: results.append(CheckResult( "log-3", "Structured Logging & Observability", "Health-check endpoint exposed", "pass", "info", f"Health-check endpoint found: {', '.join(health_paths)}.", + evidence=health_paths[:5], + best_practice_ref=ref, + )) + elif health_decls: + results.append(CheckResult( + "log-3", "Structured Logging & Observability", "Health-check endpoint exposed", + "pass", "info", + f"A health route is declared in code ({len(health_decls)} reference(s)). This is a " + "text match on the path, not proof the route responds, so call it once to be sure.", + evidence=[f"{f}: {line[:80]}" for f, line in health_decls[:5]], + confidence="weak", best_practice_ref=ref, )) else: diff --git a/skills/prod-readiness-coach/scripts/audit/repo.py b/skills/prod-readiness-coach/scripts/audit/repo.py index 0220245..4b48fbd 100644 --- a/skills/prod-readiness-coach/scripts/audit/repo.py +++ b/skills/prod-readiness-coach/scripts/audit/repo.py @@ -154,10 +154,17 @@ def package_json(self) -> dict: if self._pkg_cache is not None: return self._pkg_cache root = self._read_json("package.json") + members = self.workspace_package_files() + # No manifest means no manifest. Returning a dict of empty sections here + # would be truthy, and seven callers branch on `if pkg:`, so a Go or + # Python repo would be told its package.json is missing a test script. + if not root and not members: + self._pkg_cache = {} + return self._pkg_cache merged = dict(root) for key in ("dependencies", "devDependencies", "scripts"): merged[key] = dict(root.get(key, {})) - for f in self.workspace_package_files(): + for f in members: member = self._read_json(f) for key in ("dependencies", "devDependencies"): merged[key] = {**member.get(key, {}), **merged[key]} @@ -166,6 +173,24 @@ def package_json(self) -> dict: self._pkg_cache = merged return merged + def requirement_names(self) -> set[str]: + """Lower-cased Python dependency names from any manifest style. + + requirements.txt lists them bare (`fastapi-users>=13`), pyproject.toml + lists them quoted inside an array (` "fastapi-users",`). Stripping the + array punctuation first is what makes the second style readable; without + it a PEP 621 project looked like it had no dependencies at all. + """ + names = set() + for line in self.requirements_text().splitlines(): + token = line.strip().strip("[](),'\"").strip() + if not token or token.startswith("#"): + continue + name = re.split(r"[=<>!~\[;,'\"() ]", token, maxsplit=1)[0].strip().lower() + if name: + names.add(name) + return names + def requirements_text(self) -> str: parts = [] for f in ("requirements.txt", "pyproject.toml", "Pipfile"): diff --git a/skills/prod-readiness-coach/tests/test_prod_audit.py b/skills/prod-readiness-coach/tests/test_prod_audit.py index 48ebd9e..0579359 100644 --- a/skills/prod-readiness-coach/tests/test_prod_audit.py +++ b/skills/prod-readiness-coach/tests/test_prod_audit.py @@ -533,5 +533,184 @@ def test_unittest_step_counts_as_running_tests(self): self.assertEqual(check(r, "ci-2")["status"], "pass") +# -------------------------------------------------------------------------- +# Every check has to prove two things: that it fires when it should, and that +# it stays quiet when it should not. Only the first half was ever tested, which +# is how auth-2 shipped wrong about half the time it spoke. Borrowed from KICS, +# which refuses a rule that has no negative fixture. +# -------------------------------------------------------------------------- + +ROUTE = "src/app/api/thing/route.ts" + + +class Auth2StaysQuiet(unittest.TestCase): + """auth-2 must not call an authenticated route unguarded.""" + + BASE = {"package.json": json.dumps({"dependencies": {"@clerk/nextjs": "6.0.0", "next": "15.0.0"}}), + "tsconfig.json": "{}"} + + def test_fires_on_a_genuinely_unguarded_route(self): + r = audit({**self.BASE, ROUTE: "export async function GET() { return Response.json({ok:1}) }"}) + k = check(r, "auth-2") + self.assertEqual(k["status"], "fail") + self.assertEqual(k["severity"], "high") + + def test_quiet_when_middleware_guards_requests_first(self): + mw = ('import { clerkMiddleware, createRouteMatcher } from "@clerk/nextjs/server";\n' + 'const isPublicRoute = createRouteMatcher(["/", "/sign-in(.*)"]);\n' + 'export default clerkMiddleware(async (auth, req) => {\n' + ' if (!isPublicRoute(req)) await auth.protect();\n});\n') + r = audit({**self.BASE, ROUTE: "export async function GET() { return Response.json({ok:1}) }", + "src/middleware.ts": mw}) + k = check(r, "auth-2") + self.assertEqual(k["status"], "warn", k["detail"]) + self.assertEqual(k["severity"], "low") + self.assertTrue(any("middleware" in e for e in k["evidence"])) + self.assertTrue(any("public pattern: /sign-in(.*)" in e for e in k["evidence"]), + k["evidence"]) + + def test_quiet_on_a_signature_verified_webhook(self): + route = ('import { Webhook } from "svix";\n' + '// Called by a machine. It cannot carry a browser session.\n' + 'export async function POST(req) {\n' + ' const wh = new Webhook(process.env.SVIX_SECRET);\n' + ' wh.verify(await req.text(), headers);\n return Response.json({ok:1});\n}\n') + r = audit({**self.BASE, "src/app/api/inbound/route.ts": route}) + k = check(r, "auth-2") + self.assertEqual(k["status"], "pass", k["detail"]) + + def test_quiet_on_a_shared_secret_route(self): + route = ('export async function POST(req) {\n' + ' const secret = process.env.TOOL_SECRET;\n' + ' if (req.headers.get("x-secret") !== secret) return new Response("no", {status:401});\n' + ' return Response.json({ok:1});\n}\n') + r = audit({**self.BASE, ROUTE: route}) + self.assertEqual(check(r, "auth-2")["status"], "pass") + + def test_auth_3_still_catches_the_fail_open_guard(self): + owner = ('const owner = process.env.OWNER_EMAIL;\n' + 'export function isOwner(email) {\n if (!owner) return true;\n' + ' return email === owner;\n}\n') + r = audit({**self.BASE, ROUTE: "export async function GET() {}", "src/lib/owner.ts": owner}) + k = check(r, "auth-3") + self.assertEqual(k["status"], "fail") + self.assertEqual(k["severity"], "critical") + + +class GoRepoIsNotLiedTo(unittest.TestCase): + """Go is not a supported stack. It must still never be told something false.""" + + FILES = { + "go.mod": "module example.com/svc\n\ngo 1.23\n", + "go.sum": "", + "main.go": ('package main\n\nimport (\n\t"net/http"\n\t"github.com/go-chi/chi/v5"\n)\n\n' + 'func main() {\n\tr := chi.NewRouter()\n' + '\tr.Get("/health", func(w http.ResponseWriter, _ *http.Request) {\n' + '\t\tw.WriteHeader(http.StatusOK)\n\t})\n' + '\thttp.ListenAndServe(":8080", r)\n}\n'), + "main_test.go": 'package main\n\nimport "testing"\n\nfunc TestHealth(t *testing.T) {}\n', + ".github/workflows/ci.yml": ( + "on:\n pull_request:\njobs:\n build:\n runs-on: ubuntu-latest\n steps:\n" + " - run: go vet ./...\n - run: golangci-lint run\n - run: go test ./...\n"), + "CLAUDE.md": "Build with go build. Test with go test ./... . Lint with golangci-lint run. " + "The service listens on 8080 and exposes /health for the load balancer to poll " + "before it sends traffic to a new instance.", + ".env.example": "DATABASE_URL=\n", + ".gitignore": ".env\n", + } + + def test_no_javascript_is_claimed(self): + r = audit(self.FILES) + self.assertEqual(r["stack_fingerprint"]["languages"], ["go"]) + + def test_ci_6_never_says_package_json_exists(self): + k = check(audit(self.FILES), "ci-6") + self.assertNotIn("package.json exists", k["detail"]) + self.assertEqual(k["status"], "warn") + + def test_go_lint_steps_are_recognised(self): + self.assertEqual(check(audit(self.FILES), "ci-3")["status"], "pass") + + def test_runtime_checks_report_n_a_rather_than_guessing(self): + """No Go framework is recognised, so the profile is unknown. Silence, not a fail.""" + self.assertEqual(check(audit(self.FILES), "log-3")["status"], "n/a") + + def test_code_declared_health_route_is_found_when_the_profile_is_known(self): + k = check(audit(self.FILES, profile="api"), "log-3") + self.assertEqual(k["status"], "pass", k["detail"]) + self.assertEqual(k["confidence"], "weak") + + +class PythonServiceRepo(unittest.TestCase): + """A well-built FastAPI service. Python is a supported stack, so it must score like one.""" + + FILES = { + "pyproject.toml": ( + '[project]\nname = "svc"\ndependencies = [\n "fastapi",\n "fastapi-users",\n' + ' "structlog",\n "sentry-sdk",\n "alembic",\n]\n\n' + '[dependency-groups]\ndev = ["pytest", "pytest-cov", "mypy"]\n\n' + '[tool.pytest.ini_options]\naddopts = "--cov=app"\n'), + "app/main.py": ( + 'from fastapi import FastAPI\nimport structlog\nimport sentry_sdk\n\n' + 'sentry_sdk.init(dsn=os.environ["SENTRY_DSN"])\nlog = structlog.get_logger()\n' + 'app = FastAPI()\n\n\n@app.get("/health")\nasync def health():\n' + ' return {"status": "ok"}\n'), + "app/api/routes.py": ( + 'from fastapi import APIRouter, Depends\nfrom app.users import current_active_user\n\n' + 'router = APIRouter(dependencies=[Depends(current_active_user)])\n\n\n' + '@router.get("/items")\nasync def items():\n return []\n'), + "tests/test_health.py": 'def test_health(client):\n assert client.get("/health").status_code == 200\n', + ".github/workflows/ci.yml": ( + "on:\n pull_request:\njobs:\n test:\n runs-on: ubuntu-latest\n steps:\n" + " - run: mypy app\n - run: pytest --cov=app\n"), + "CLAUDE.md": "FastAPI service. Run with uvicorn app.main:app. Test with pytest. Typecheck " + "with mypy app. Migrations are alembic. Auth is fastapi-users with cookie " + "sessions; every router under app/api requires an active user.", + ".env.example": "DATABASE_URL=\nSENTRY_DSN=\n", + ".gitignore": ".env\n.venv\n", + "docs/runbook.md": ( + "# Runbook\n\nTo roll back, run alembic downgrade -1 and redeploy the previous image " + "tag from the registry. The health endpoint is /health and the load balancer polls it " + "every ten seconds. If the database is unreachable the service returns 503 and the " + "balancer drains it automatically. Page the on-call engineer listed in the team " + "directory when error rate passes two percent for five minutes running.\n"), + } + + def test_language_is_python_only(self): + self.assertEqual(audit(self.FILES)["stack_fingerprint"]["languages"], ["python"]) + + def test_mypy_counts_as_a_typecheck_step(self): + self.assertEqual(check(audit(self.FILES), "ci-3")["status"], "pass") + + def test_pytest_config_is_a_test_entry_point(self): + self.assertEqual(check(audit(self.FILES), "ci-6")["status"], "pass") + + def test_fastapi_users_counts_as_an_auth_mechanism(self): + self.assertEqual(check(audit(self.FILES), "auth-1")["status"], "pass") + + def test_decorator_health_route_is_found(self): + self.assertEqual(check(audit(self.FILES), "log-3")["status"], "pass") + + def test_no_check_claims_a_package_json(self): + r = audit(self.FILES) + for c in r["categories"]: + for k in c["checks"]: + self.assertNotIn("package.json exists", k["detail"], k["id"]) + + +class NoManifestMeansNoManifest(unittest.TestCase): + def test_package_json_is_empty_without_a_manifest(self): + root = make_repo({"go.mod": "module x\ngo 1.23\n"}) + self.assertEqual(prod_audit.Repo(root).package_json(), {}) + + def test_package_json_still_merges_workspace_members(self): + root = make_repo({ + "package.json": json.dumps({"workspaces": ["apps/*"]}), + "apps/web/package.json": json.dumps({"dependencies": {"next": "15.0.0"}}), + }) + pkg = prod_audit.Repo(root).package_json() + self.assertIn("next", pkg["dependencies"]) + + if __name__ == "__main__": unittest.main() diff --git a/skills/product-readiness-review/SKILL.md b/skills/product-readiness-review/SKILL.md index 2b14386..7ac552c 100644 --- a/skills/product-readiness-review/SKILL.md +++ b/skills/product-readiness-review/SKILL.md @@ -1,6 +1,6 @@ --- name: product-readiness-review -description: "Perform a read-only, evidence-based assessment of whether an AI-assisted or early-stage product is ready for users, production, technical due diligence, or developer handoff. Use when the user asks whether a product is production-ready, wants a product audit, wants to improve more than code style, or asks what must be fixed before launch or handoff. Use for user journeys and product behavior. For broad production-readiness requests, run prod-readiness-coach first for the repository-controls scan, then this review for product judgment." +description: "Judges user journeys and product behavior, not repository controls: whether a real person can finish the core flows, what happens when they do the wrong thing, and whether the product is ready for users, production, technical due diligence, or developer handoff. A read-only, evidence-based assessment that reads code and docs and reasons about the product as a working system. Use when the user asks whether a product is production-ready, wants a product audit, wants to improve more than code style, or asks what must be fixed before launch or handoff. For broad production-readiness requests, run prod-readiness-coach first for the repository-controls scan, then this review for product judgment." --- # Product Readiness Review