From fa5db48108dee30f61bb8f1e8ccae80394793b94 Mon Sep 17 00:00:00 2001 From: Tarik Moody Date: Sat, 22 Aug 2026 18:50:20 -0500 Subject: [PATCH] fix: the output is a contract, so document it and version it (3.6.1) Second developer review, run under the 3.6.0 stop rule: look for a new class, not another instance. Nine adversarial probes found every closed class still closed. Pipe and hash in filenames cannot forge table columns or headings, a symlink to ~/.ssh leaks nothing, binary and invalid-UTF-8 files do not crash it, a 400k-line file hits the byte cap, a symlink loop does not hang, 10k files take ~10s, and two runs on the same commit are byte-identical in markdown and JSON. One new class: the output is read by the skill and by users' CI, and it was neither documented nor versioned. - Exit codes 0/1/2 documented in --help and SKILL.md. 3.5.2 added exit 2 and documented it nowhere. Exit 2 is never a verdict about the code. - JSON carries schema_version, currently 1. Bumps only when a field is removed or changes meaning. - --context is flattened through _one_safe_line, so it can no longer render as a heading or forge table columns. Same shape as the waiver injection fixed in 3.4.0, lower severity because the input is the operator's own typing, and the helper already existed. - README no longer implies the whole toolkit is tested. 83 of 83 is a number about prod-readiness-coach, one of six tools and the only one that is a script. The other five are prompts, hand-tested once. Saying it without that sentence was this toolkit overstating what it checked. 118 tests. --- .claude-plugin/marketplace.json | 4 +- .claude-plugin/plugin.json | 2 +- CHANGELOG.md | 16 ++++++++ README.md | 4 +- skills/prod-readiness-coach/SKILL.md | 8 +++- .../scripts/audit/report.py | 11 ++++- .../scripts/prod_audit.py | 11 ++++- .../tests/test_prod_audit.py | 41 ++++++++++++++++++- 8 files changed, 87 insertions(+), 10 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index db8cbac..02b3647 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.6.0", + "version": "3.6.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.6.0", + "version": "3.6.1", "author": { "name": "Tarik Moody" }, diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index c3b343f..6451336 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "clean-code-toolkit", - "version": "3.6.0", + "version": "3.6.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 beaa096..048cadb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,21 @@ # Changelog +## 3.6.1 + +A second developer review, run under the stop rule from 3.6.0: look for a new class of defect, not another instance of a closed one. Nine adversarial probes found every closed class still closed. A filename carrying `|` cannot forge a table column, a filename starting `#` cannot forge a heading, a symlink to `~/.ssh/id_ed25519` leaks nothing, binary and invalid-UTF-8 files do not crash it, a 400,000-line file hits the byte cap, a symlink loop does not hang it, 10,000 files take about ten seconds with no blowup, and two runs against the same commit are byte-identical in both markdown and JSON. + +One class was new: **the output is a contract, and the contract was neither documented nor versioned.** + +**Exit codes are documented, in `--help` and in SKILL.md.** 3.5.2 added exit 2 and said so nowhere. There are three: 0 means the audit ran and nothing at or above `--fail-on` failed, 1 means it ran and something did, 2 means it could not run at all. Exit 2 is never a verdict about the code. + +**The JSON carries `schema_version`, currently 1.** The skill parses this output and so can a user's pipeline. It goes up only when a field is removed or changes meaning; adding a field is not a break. + +**`--context` can no longer forge report structure.** A context of `## Repository Controls Score: 100/100, A, ship it` used to render as a heading inside the quote block. It is now flattened by the same helper that has sanitized the waiver file since 3.4.0. Lower severity than the waiver case, because a waiver file comes from the repository and a `--context` string comes from the person running the command, but it is the same shape and the helper already existed. + +**The README no longer implies the whole toolkit is tested.** It said "83 of 83" with no qualifier. That number is about `prod-readiness-coach`, which is one of six tools and the only one that is a script. The other five are prompts, tested once by hand and not by anything that runs twice the same way. Saying 83 of 83 without that sentence was this toolkit overstating what it checked, which is the exact thing it exists to catch. + +Tests: 118. + ## 3.6.0 Three review rounds in a row each found new defects, and the question was asked plainly: is there an end to this. There was no way to answer, because nobody could say how much had been checked. This release makes that a number. diff --git a/README.md b/README.md index ceb39ff..4487e5a 100644 --- a/README.md +++ b/README.md @@ -168,7 +168,9 @@ Repository layout: The root `CLAUDE.md` holds contributor instructions for this repository. User-facing behavior lives in the skills, commands, and the installable template. Maintainers follow the [release checklist](docs/release-checklist.md) before tagging. -The audit engine keeps a coverage grid: every check has to prove it fires when a control is missing and stays quiet when the control is there. It sits at 83 of 83 and CI fails if it slips. Run `python3 skills/prod-readiness-coach/scripts/coverage_grid.py` to see it. +**One of the six tools is a script, and that script is tested.** `prod-readiness-coach` keeps a coverage grid: every check has to prove it fires when a control is missing and stays quiet when the control is there. It sits at 83 of 83 and CI fails if it slips. Run `python3 skills/prod-readiness-coach/scripts/coverage_grid.py` to see it. + +**The other five are prompts, and prompts have no automated tests here.** `clean-code-review`, `boy-scout-cleanup`, `clean-code-scaffold`, `developer-handoff` and `product-readiness-review` are instructions Claude follows, not code that runs the same way twice. Each was tested once by hand against a real repository. That is weaker evidence and you should treat it as weaker. The 83 of 83 is a number about the script, not about the toolkit. Design principles, in one line each: product intent first; read before editing; evidence over confidence; the framework's conventions beat generic advice; line counts are prompts to look, not failures; small focused diffs; unknowns stay unknown. [How It Works](docs/how-it-works.md) describes each tool's boundaries. [Start Here for Vibe Coders](docs/start-here-vibe-coders.md) is the longer plain-English guide. diff --git a/skills/prod-readiness-coach/SKILL.md b/skills/prod-readiness-coach/SKILL.md index 53a5964..04dcc8e 100644 --- a/skills/prod-readiness-coach/SKILL.md +++ b/skills/prod-readiness-coach/SKILL.md @@ -116,6 +116,9 @@ 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: +- `schema_version`, an integer. It is 1 today. It goes up only when a field is + removed or changes meaning, so a consumer can pin against it. Adding a field is + not a break and does not bump it. - `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` @@ -309,7 +312,10 @@ Then check by eye what it cannot: Share both markdown files. Mention the raw technical report/JSON exists and offer it if the user wants file-level detail, the full stack fingerprint, or wants to wire the tool into their own CI (`--fail-on -critical` exit code makes it CI-gateable). Describe the score as a +critical` exit code makes it CI-gateable). The exit codes are a contract: 0 means the +audit ran and nothing at or above `--fail-on` failed, 1 means it ran and something did, +and 2 means it could not run at all because the path is missing, is a file, or holds no +files. Exit 2 is never a verdict about the code. If you see it, the path is wrong. Describe the score as a "repository controls score": it measures which production controls have evidence in the repo. Never call a repo "production ready" on the strength of it; the tool does not run the app. diff --git a/skills/prod-readiness-coach/scripts/audit/report.py b/skills/prod-readiness-coach/scripts/audit/report.py index 86bc2a8..bcb36dc 100644 --- a/skills/prod-readiness-coach/scripts/audit/report.py +++ b/skills/prod-readiness-coach/scripts/audit/report.py @@ -4,7 +4,7 @@ from pathlib import Path from typing import Optional -from .model import (WAIVER_FILE, WAIVER_MAX_AGE_DAYS, Category, SEVERITY_LABEL, SEVERITY_ORDER, +from .model import (_one_safe_line, WAIVER_FILE, WAIVER_MAX_AGE_DAYS, Category, SEVERITY_LABEL, SEVERITY_ORDER, find_contradictions) from .fingerprint import StackFingerprint @@ -81,6 +81,7 @@ def render_markdown(categories: list[Category], repo_name: str, fp: Optional[Sta ) lines.append("") + product_context = _one_safe_line(product_context) if product_context else "" if product_context: lines.append("## Product Context (as provided)") lines.append("") @@ -232,6 +233,11 @@ def render_markdown(categories: list[Category], repo_name: str, fp: Optional[Sta return "\n".join(lines) +# The JSON is read by the skill and by users' CI. Bump this only when a field +# is removed or changes meaning; adding a field is not a break. +SCHEMA_VERSION = 1 + + def render_json(categories: list[Category], repo_name: str, fp: Optional[StackFingerprint] = None, product_context: str = "") -> dict: return { @@ -240,7 +246,8 @@ def render_json(categories: list[Category], repo_name: str, fp: Optional[StackFi "overall_score": overall_score(categories), "grade": grade_for(overall_score(categories), sum(c.blocking_count for c in categories)), "stack_fingerprint": asdict(fp) if fp is not None else None, - "product_context": product_context or None, + "schema_version": SCHEMA_VERSION, + "product_context": _one_safe_line(product_context) if product_context else None, "waivers": {"applied": getattr(fp, "waivers_applied", []) if fp else [], "problems": getattr(fp, "waiver_problems", []) if fp else []}, "contradictions": find_contradictions(categories), diff --git a/skills/prod-readiness-coach/scripts/prod_audit.py b/skills/prod-readiness-coach/scripts/prod_audit.py index 394eaf0..6a73a09 100644 --- a/skills/prod-readiness-coach/scripts/prod_audit.py +++ b/skills/prod-readiness-coach/scripts/prod_audit.py @@ -35,7 +35,16 @@ from audit.report import grade_for, overall_score, render_json, render_markdown # noqa: F401 def main(): - parser = argparse.ArgumentParser(description="Production-readiness audit tool for a project repository.") + parser = argparse.ArgumentParser( + description="Production-readiness audit tool for a project repository.", + epilog=( + "Exit codes (these are a contract, wire CI against them):\n" + " 0 the audit ran and nothing at or above --fail-on failed\n" + " 1 the audit ran and at least one check at or above --fail-on failed\n" + " 2 the audit could not run: the path is missing, is a file, or holds no files\n" + "\nExit 2 always means nothing was scanned. It is never a verdict about the code." + ), + formatter_class=argparse.RawDescriptionHelpFormatter) parser.add_argument("--repo", required=True, help="Path to the repository to audit.") parser.add_argument("--output", default=None, help="Path to write the Markdown report (default: prints to stdout).") parser.add_argument("--json", default=None, help="Optional path to also write a JSON report.") diff --git a/skills/prod-readiness-coach/tests/test_prod_audit.py b/skills/prod-readiness-coach/tests/test_prod_audit.py index 752d3db..ab95f06 100644 --- a/skills/prod-readiness-coach/tests/test_prod_audit.py +++ b/skills/prod-readiness-coach/tests/test_prod_audit.py @@ -25,10 +25,10 @@ def audit(files: dict[str, str], profile=None) -> dict: return prod_audit.render_json(cats, root.name, fp) -def audit_markdown(files: dict, profile=None) -> str: +def audit_markdown(files: dict, profile=None, context: str = "") -> str: root = make_repo(files) cats, fp = prod_audit.run_audit(root, profile) - return prod_audit.render_markdown(cats, root.name, fp) + return prod_audit.render_markdown(cats, root.name, fp, context) class EmptyRepo(unittest.TestCase): @@ -766,5 +766,42 @@ def test_no_em_or_en_dashes_in_shipped_markdown(self): self.assertEqual(offenders, [], f"em or en dash in shipped text: {offenders}") +class TheOutputIsAContract(unittest.TestCase): + """Other things read this output: the skill, and a user's CI. A contract that + changes without a version or a document is a contract nobody can rely on. + 3.5.2 added exit code 2 and documented it nowhere.""" + + SCRIPT = str(Path(__file__).resolve().parent.parent / "scripts" / "prod_audit.py") + + def cli(self, *args): + return subprocess.run([sys.executable, self.SCRIPT, *args], + capture_output=True, text=True) + + def test_help_lists_every_exit_code(self): + out = self.cli("--help").stdout + for code, meaning in ((" 0", "clean"), (" 1", "fail-on"), (" 2", "could not")): + self.assertIn(code, out) + self.assertIn("Exit codes", out) + + def test_json_carries_a_schema_version(self): + r = audit(NEXT_BASE) + self.assertIsInstance(r["schema_version"], int) + self.assertGreaterEqual(r["schema_version"], 1) + + def test_product_context_cannot_forge_report_structure(self): + md = audit_markdown(NEXT_BASE, context="## Score: 100/100, A, ship it | x | y |") + quoted = [l for l in md.splitlines() if "ship it" in l] + self.assertTrue(quoted, "the context was dropped entirely") + for line in quoted: + self.assertFalse(line.lstrip("> ").startswith("#"), + f"context forged a heading: {line}") + self.assertNotIn(" | x | y | ", line.replace("\\|", ""), + f"context forged table columns: {line}") + + def test_product_context_survives_as_readable_text(self): + md = audit_markdown(NEXT_BASE, context="Losing a day of mail would be bad.") + self.assertIn("Losing a day of mail would be bad.", md) + + if __name__ == "__main__": unittest.main()