diff --git a/CLAUDE.md b/CLAUDE.md index c56e2e12..de441098 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -456,6 +456,34 @@ bin/coverage_check app/controllers/users_controller.rb - **Priority files**: Controllers, services, models with business logic - **Low coverage files**: Immediate attention required for < 80% coverage +### Code Quality Grading + +**Jev Grader** (`bin/grade-code`) - scores source and spec files against the +house rules with Jev, one file per call. No mechanical checks: those are +CodeStandardsChecker, standardrb and `bin/lint`. + +- **Usage**: `bin/grade-code [files or dirs]` - defaults to `app spec lib` +- **Single file**: prints the full per-check report; **2+ files**: worst-first + ranked table with the most-failed checks across the batch + +```bash +bin/grade-code # whole source and test tree +bin/grade-code app/models --workers 6 # parallel sweep of a directory +bin/grade-code --kinds model,controller # only these kinds +bin/grade-code app/models/inspection.rb # one file in detail +bin/grade-code spec --limit 20 --csv tmp/grades.csv # first 20, written to CSV +bin/grade-code --list-checks # the rubric +``` + +- **What it scores**: idiomatic Rails, nice simple methods, short comments that + explain WHY with no history notes, plain technical English, no dead code or + defensive fallbacks, I18n for user-facing strings, semantic HTML, form + partial conventions, and for specs: not tautological, succinct names, happy + path plus edge cases +- **Needs** `OPENCODE_API_KEY` (or `/run/secrets/opencode_api_key`, or `.env`) +- **Cost**: ~80s for the whole 560-file tree at `--workers 6`; advisory only, + nothing is blocked on the score + ### Code Organization - **Create partials for repeated code** - DRY principle diff --git a/bin/grade-code b/bin/grade-code new file mode 100755 index 00000000..f23bdfcb --- /dev/null +++ b/bin/grade-code @@ -0,0 +1,942 @@ +#!/usr/bin/env python3 +""" +grade-code — grade this repo's source and spec files against the house coding +rules in CLAUDE.md using Jev (TypeSafe System One) via the OpenCode zen API. + +Every check is a judgement call, not a grep: one Jev systemone call per file, +one question per check, all questions for that file asked in a single call. +The script carries no mechanical checks — the repo already runs those +(CodeStandardsChecker, standardrb, the custom cops, bin/lint). What is left is +the taste: idiomatic Rails, small single-purpose methods, short comments that +explain WHY and carry no history notes, plain technical English, no dead code +or defensive fallbacks, I18n for every user-facing string, semantic HTML, and +specs that are not tautological and read as a clear story. + +Targets are files or directories; the default is the whole of app, spec and +lib. Files are graded one at a time. + +Usage: + ./bin/grade-code # app, spec, lib + ./bin/grade-code app/models spec/features # chosen dirs + ./bin/grade-code app/models/inspection.rb # full single-file report + ./bin/grade-code --kinds model,controller,service # only these kinds + ./bin/grade-code --limit 20 # first 20 discovered + ./bin/grade-code spec --workers 6 # parallel (default 1) + ./bin/grade-code --csv grades.csv --json + ./bin/grade-code --list-checks # print the rubric + +The Jev API key is read from OPENCODE_API_KEY, then /run/secrets/opencode_api_key, +then a .env file in the repo root. Batch mode always exits 0 (it is a report); +single-file mode exits 2 when the file could not be graded. +""" + +import argparse +import csv +import json +import os +import re +import sys +import time +import urllib.error +import urllib.request +from collections import Counter, OrderedDict +from concurrent.futures import ThreadPoolExecutor, as_completed + +ZEN_SYSTEMONE_URL = "https://opencode.ai/zen/v1/systemone" +DEFAULT_MODEL = "jev-1.13" +DEFAULT_KEY_FILE = "/run/secrets/opencode_api_key" + +REPO_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +DEFAULT_TARGETS = ["app", "spec", "lib"] + +MAX_SOURCE_CHARS = 60000 +GRADABLE_EXTS = (".rb", ".erb", ".js") +SKIP_DIR_NAMES = { + ".bundle", ".git", "coverage", "gems", "log", "node_modules", + "public", "sorbet", "spencers_app", "storage", "tmp", "vendor", +} +SKIP_PATH_PREFIXES = ("app/assets/",) +# third-party code, not house code: the upstream highlight.js distribution +VENDORED_FILES = ("app/javascript/highlight.js",) + +KIND_LABELS = { + "controller": "Rails controller", + "helper": "Rails helper", + "job": "Rails job", + "lib": "library code", + "model": "Rails model", + "serializer": "Rails serializer", + "service": "Rails service object", + "spec": "RSpec spec", + "spec_feature": "RSpec feature spec", + "spec_support": "RSpec support code (helper, factory, harness)", + "view": "ERB template", + "javascript": "JavaScript", +} + +RAILS_KINDS = { + "controller", "helper", "job", "model", "serializer", "service", +} +RUBY_KINDS = RAILS_KINDS | {"lib", "spec", "spec_feature", "spec_support"} +APP_KINDS = RAILS_KINDS | {"lib"} +TEST_KINDS = {"spec", "spec_feature"} +ANY_KINDS = RUBY_KINDS | {"javascript", "view"} + +FORM_RE = re.compile(r"form_with|form_context|simple_form| gradable files with a kind +# --------------------------------------------------------------------------- + + +def classify(rel_path): + """Map a repo-relative path to a file kind, or None if not gradable. + + Only the app, spec and lib trees carry house coding rules worth grading: + config, migrations, seeds data and vendored assets do not. + """ + if rel_path.startswith(SKIP_PATH_PREFIXES): + return None + if not rel_path.endswith(GRADABLE_EXTS): + return None + if rel_path.endswith(".min.js") or rel_path in VENDORED_FILES: + return None + if rel_path.startswith("app/views/"): + return "view" + if rel_path.startswith("app/javascript/"): + return "javascript" + if rel_path.startswith("spec/"): + if rel_path.startswith("spec/features/"): + return "spec_feature" + support = ("spec/support/", "spec/factories/", "spec/fixtures/") + if rel_path.startswith(support): + return "spec_support" + if rel_path in ("spec/rails_helper.rb", "spec/spec_helper.rb"): + return "spec_support" + return "spec" + for prefix, kind in ( + ("app/models/", "model"), + ("app/controllers/", "controller"), + ("app/services/", "service"), + ("app/helpers/", "helper"), + ("app/jobs/", "job"), + ("app/serializers/", "serializer"), + ): + if rel_path.startswith(prefix): + return kind + if rel_path.startswith("lib/"): + return "lib" + return None + + +def discover(targets, kinds=None): + """Expand file and dir targets into (abs_path, rel_path, kind) triples.""" + found = [] + for target in targets: + path = target if os.path.isabs(target) else os.path.join(REPO_ROOT, target) + if os.path.isfile(path): + candidates = [path] + elif os.path.isdir(path): + candidates = [] + for root, dirs, files in os.walk(path): + dirs[:] = [d for d in dirs if d not in SKIP_DIR_NAMES] + candidates.extend(os.path.join(root, f) for f in files) + else: + print(f"no such file or directory: {target}", file=sys.stderr) + continue + for candidate in candidates: + rel = os.path.relpath(candidate, REPO_ROOT) + kind = classify(rel) + if kind and (not kinds or kind in kinds): + found.append((candidate, rel, kind)) + return sorted(set(found)) + + +def read_source(abs_path): + """Read a file for grading. Returns (source, truncated).""" + with open(abs_path, encoding="utf-8", errors="replace") as f: + text = f.read() + if len(text) <= MAX_SOURCE_CHARS: + return text, False + return text[:MAX_SOURCE_CHARS], True + + +# --------------------------------------------------------------------------- +# The rubric +# +# engine "jev" -> the question is sent to Jev (one call per file, all +# questions together) and the answer is graded against the thresholds below. +# +# For noul checks the probability is the goodness, unless invert is set: then +# the question asks for a *bad* thing and a low probability is good. +# score checks grade a 0..N level against score_pass / score_warn. +# --------------------------------------------------------------------------- + +CHECKS = [ + dict(id="idiomatic_rails", label="Idiomatic Rails", types=RAILS_KINDS, weight=6, + question=dict( + type="score", + instructions=( + "Is this Rails code idiomatic? House standards: scopes for " + "non-trivial queries, `find_by` rather than `where(...).first`, " + "validations for data integrity, meaningful associations, " + "RESTful actions, business logic in models or services rather " + "than controllers, service objects for work that spans models, " + "delegation where it earns its place, and ActiveRecord idioms " + "instead of hand-rolled SQL." + ), + criteria=[ + "Not idiomatic - hand-rolled SQL, non-RESTful actions, logic in the wrong layer", + "Mostly conventional with a few unidiomatic spots", + "Solid, idiomatic Rails throughout", + "Textbook - conventions, scopes, validations and delegation used exactly where they belong", + ], + ), + score_pass=2, score_warn=1), + dict(id="method_simplicity", label="Nice simple methods", types=RUBY_KINDS, weight=6, + question=dict( + type="score", + instructions=( + "Are the methods nice and simple? Each should do one thing, stay " + "short (house limit is 20 lines), use guard clauses instead of " + "nesting, and carry a descriptive name. Complex conditions belong " + "in a named predicate method rather than inline in a branch." + ), + criteria=[ + "Long multi-purpose methods, deep nesting, cryptic names", + "One or two methods are too long or do too much", + "Methods are short, single-purpose and well named", + "Every method is a small obvious unit whose name says what it does", + ], + ), + score_pass=2, score_warn=1), + dict(id="comments_why_not_what", label="Comments explain WHY", types=ANY_KINDS, weight=4, + question=dict( + type="noul", + instructions=( + "Do the comments in `source` earn their place - do they explain " + "WHY (business context, a regulatory requirement, a non-obvious " + "decision a reader could not infer) rather than restating WHAT " + "the next line of code already says? Judge only the comments that " + "are there: a file with no comments passes, because it neither " + "explains nor misleads. Comments that merely narrate the code - " + "'# increment the counter', '# verify we're on the right tab' - " + "count against it." + ), + criteria={ + "true": "Comments that exist carry real reasons, or there are none", + "false": "Comments narrate what the code does, or repeat it back", + }, + ), + threshold=0.5), + dict(id="comments_short_plain", label="Short comments, plain English", types=ANY_KINDS, weight=3, + question=dict( + type="noul", + instructions=( + "Are the comments short and in simple technical English - a " + "sentence or two, plain words, British spelling - rather than " + "essays, chatty prose, jargon-stacking or American spelling? " + "Judge only the comments that are there: a file with no comments " + "passes." + ), + criteria={ + "true": "Comments are brief, plain and use British spelling", + "false": "Comments are long-winded, chatty, flowery or American", + }, + ), + threshold=0.5), + dict(id="no_history_notes", label="No history notes or stale TODOs", types=ANY_KINDS, weight=4, + question=dict( + type="noul", + instructions=( + "Are the comments free of history notes - 'changed this to that', " + "'previously we did X', notes about who changed what and when, " + "dated or stale TODO/FIXME notes, and commentary that the next " + "refactor will make wrong?" + ), + criteria={ + "true": "No history notes, changelog-style comments or stale TODOs", + "false": "Changelog-style notes, dated commentary or stale TODOs", + }, + ), + threshold=0.5), + dict(id="no_dead_code", label="No dead code or speculative abstraction", types=ANY_KINDS, weight=4, + question=dict( + type="noul", + instructions=( + "Does `source` carry dead weight - methods nothing calls, " + "commented-out code, speculative abstractions, or a fallback for " + "a case that cannot happen?" + ), + criteria={ + "true": "Dead code, commented-out blocks or needless abstraction", + "false": "Nothing unused - every method earns its place", + }, + ), + invert=True, threshold=0.5), + dict(id="no_defensive_coding", label="No defensive coding or silent fallbacks", types=RUBY_KINDS, weight=4, + question=dict( + type="noul", + instructions=( + "Does `source` defend against problems that cannot happen - " + "`respond_to?` checks against interfaces the app owns, rescues " + "that swallow errors, nil-guards on values the app guarantees, " + "or `||` fallbacks that hide missing data? House style prefers " + "crashing loudly over quietly failing." + ), + criteria={ + "true": "Defensive checks, silent rescues or fallbacks that hide failures", + "false": "Trusts its own interfaces and lets errors bubble up", + }, + ), + invert=True, threshold=0.5), + dict(id="modern_ruby", label="Modern, tidy Ruby", types=RUBY_KINDS, weight=3, + question=dict( + type="score", + instructions=( + "How modern and tidy is the Ruby? House style prefers endless " + "methods for simple computation, shorthand hashes (`{width:, " + "height:}`), numbered block parameters (`map { _1.upcase }`), " + "pattern matching for dispatch, `filter_map`, rightward " + "assignment where it reads well, and no backslash continuations " + "or parameter-alignment padding." + ), + criteria=[ + "Dated Ruby throughout - old syntax the language no longer needs", + "Mostly legacy syntax; modern idioms only where the old ones bite", + "Acceptable - modern idioms where they help, some dated syntax left", + "Consistently the newest tidy syntax, in the right places", + ], + ), + score_pass=2, score_warn=1), + dict(id="naming_british", label="British, descriptive naming", types=ANY_KINDS, weight=3, + question=dict( + type="noul", + instructions=( + "Are the identifiers in `source` British English and idiomatic " + "for their language - snake_case in Ruby, camelCase in " + "JavaScript - with no American spellings (color, organize, " + "initialize) and no opaque abbreviations? Ordinary names such as " + "`name`, `email` or `user` count as fine; only genuinely cryptic " + "or misleading names count against it." + ), + criteria={ + "true": "Names are British, idiomatic for the language and readable", + "false": "American spellings or opaque abbreviations", + }, + ), + threshold=0.5), + dict(id="sorbet_sigs", label="Sorbet sigil and sigs", types=APP_KINDS, weight=3, + question=dict( + type="noul", + instructions=( + "Does `source` follow the repo's Sorbet convention: a `# typed:` " + "sigil at the top of the file, and `sig` blocks on the methods of " + "a file marked `# typed: strict` or `# typed: strong`?" + ), + criteria={ + "true": "Typed sigil present, and sigs wherever the file type requires them", + "false": "Missing the typed sigil, or methods in a strict/strong file lack sigs", + }, + ), + threshold=0.5), + dict(id="i18n_strings", label="I18n for user-facing strings", types=RAILS_KINDS | {"view"}, weight=6, + question=dict( + type="noul", + instructions=( + "Ignoring strings nobody sees (log messages, file paths, class " + "names, error classes, format strings), is every user-facing " + "string routed through I18n - `t(...)` in views and helpers, " + "locale keys or `I18n.t` in models, controllers, services and " + "serializers - with no hardcoded English the user could read?" + ), + criteria={ + "true": "All user-facing strings come from locale files", + "false": "Hardcoded user-facing strings", + }, + ), + threshold=0.5), + dict(id="skinny_controller", label="Thin controller", types={"controller"}, weight=5, + question=dict( + type="noul", + instructions=( + "Does this controller stay thin - find the record, set instance " + "variables, delegate to a model or service, use semantic params, " + "and choose the right HTTP method - rather than holding business " + "logic, queries or data munging of its own?" + ), + criteria={ + "true": "Thin: sets up and delegates, logic lives elsewhere", + "false": "Business logic or heavy data handling in the controller", + }, + ), + threshold=0.5), + dict(id="no_css_classes", label="No class or style attributes", types={"view"}, weight=4, + question=dict( + type="noul", + instructions=( + "Is this template free of `class` and `style` attributes? " + "House rule: zero class attributes, zero inline styles - " + "all styling comes from the MVP.css framework's semantic " + "selectors, so a class here is always a breach." + ), + criteria={ + "true": "Every class and style attribute absent - markup carries no styling hooks", + "false": "One or more class or style attributes", + }, + ), + threshold=0.5), + dict(id="semantic_structure", label="Semantic HTML structure", types={"view"}, weight=3, + question=dict( + type="noul", + instructions=( + "Is the structure of this template semantic - `
`, " + "`
`, `