diff --git a/.agents/skills/auditing-comments/SKILL.md b/.agents/skills/auditing-comments/SKILL.md new file mode 100644 index 0000000000..1cc148f1e6 --- /dev/null +++ b/.agents/skills/auditing-comments/SKILL.md @@ -0,0 +1,142 @@ +--- +name: auditing-comments +description: Audits the comments and docstrings a branch adds, deleting the redundant ones and rewriting the code behind the rest. Use before opening a pull request or asking for review, after finishing a change, or when a reviewer says a comment is unclear, inaccurate, or misleading. +--- + +# Auditing comments + +A comment is a debt the reviewer pays. This audit settles it before a human reads the +diff. + +The rule: **a comment that explains what the code does is a bug report against the +code.** Fix the code. Rewriting the comment leaves the defect in place, and a reviewer +who catches one inaccurate comment stops trusting the rest of the branch. + +## Workflow + +Copy this checklist and check items off: + +``` +Comment audit: +- [ ] Step 1: List the comments the branch added +- [ ] Step 2: Judge each one against the four outcomes +- [ ] Step 3: Apply the edits +- [ ] Step 4: Re-run, and confirm the survivors are all "why" comments +- [ ] Step 5: Run the tests, because step 3 changed code +``` + +**Step 1.** Run it: + +```bash +.agents/tools/fdk explain --range main...HEAD +``` + +Each block prints the added comment, then the code it annotates with the comment +stripped out. Read the stripped code first. That is what the reviewer sees. + +**Step 2.** For each block, answer one question: *with the comment gone, does the code +still say this?* Then take the matching outcome below. + +**Step 3.** Apply the edits. Prefer renaming and extracting over adding words. + +**Step 4.** Run `fdk explain` again. Every surviving comment must say *why*, not *what*. + +**Step 5.** Rewriting code breaks tests. Run them. + +## The four outcomes + +**1. The code already says it. Delete the comment.** + +```python +# Loop over the cells and sum the volumes. <- delete +total = sum(cell.volume for cell in cells) +``` + +**2. The code does not say it. Rewrite the code, not the comment.** + +This is the common case and the one that costs review time. Name the intermediate, +extract the helper, or split the expression until the sentence is unnecessary. + +```python +# WRONG: the comment carries the meaning +# result is -1 where the coarse cell has no child on this rank +result = np.full(size, -1, dtype=IntType) + +# RIGHT: the code carries it +NO_CHILD_ON_THIS_RANK = -1 +child_cells = np.full(size, NO_CHILD_ON_THIS_RANK, dtype=IntType) +``` + +**3. The comment is false. Delete the claim, then ask what the code was for.** + +Check every identifier and every concept the comment names. If the code around it does +not touch that thing, the sentence is wrong. Do not soften it. Do not qualify it. A +half-true comment reads as carelessness and costs more than no comment. + +Then keep going, because a false comment is seldom only a writing mistake. The sentence +was there to justify the code. If the justification is not true, the code may have no +reason to exist: + +```python +# WRONG: the comment states a constraint that is not real +# Copy before assembling, because assembly renumbers the dofs. +work = coefficient.copy(deepcopy=True) +result = assemble(inner(work, v) * dx) +``` + +Assembly renumbers nothing. Once that is settled, the question is not how to word the +comment. It is what the copy was for. Nothing writes to `work`, so the copy guards +nothing: + +```python +# RIGHT: the copy went with the claim that justified it +result = assemble(inner(coefficient, v) * dx) +``` + +Try this outcome before outcome 2. Rewriting code to carry a sentence is wasted work if +the code should not be there at all. + +**4. The reason is genuinely not in the code. Keep the comment, and say only the +reason.** + +Legitimate cases: a non-local invariant the caller must hold, why an obvious +alternative was rejected, a citation, a workaround for a named upstream bug. Say +*why*. Never restate *what*. + +```python +# `distributeSection` overwrites the section it is handed, so let it build its +# own and only keep the root offsets it broadcasts. +remote_offsets, distributed_section = point_sf.distributeSection(root_section) +``` + +## Why rewriting the comment is the wrong move + +A reviewer says a comment is unclear. The tempting answer is a better comment. It fails +twice over. + +The first failure is that the replacement is usually still wrong. The sentence was +written from the same misunderstanding that produced the code, so a second attempt says +the same false thing at greater length. A reviewer who did not follow the first sentence +follows the second one less. + +The second failure is worse. An inaccurate comment does not merely confuse a reader. It +puts the code under suspicion, and the reviewer starts asking why the code is there at +all. That question is usually a good one. Code defended by a false claim is often code +that nothing needed, and a reviewer pulling on the sentence is pulling on the real +defect. + +So take the question seriously rather than deflecting it with better wording. When a +comment cannot be written truthfully, the reading to try first is that the code beneath +it should go. + +## Notes + +Judge only the lines the branch adds. Comments already on `main` are not this branch's +debt, and rewriting them enlarges the diff a reviewer has to read. + +Tests and demos still want a short summary of what is being checked and why. This audit +targets explanatory comments inside function bodies, and docstrings that describe +mechanism rather than contract. + +Renaming beats commenting, but renaming a public name is an API change. Inside a +function, rename freely. diff --git a/.agents/tools/check-prose.py b/.agents/tools/check-prose.py new file mode 100755 index 0000000000..b5c1e32303 --- /dev/null +++ b/.agents/tools/check-prose.py @@ -0,0 +1,507 @@ +#!/usr/bin/env python3 +"""Check prose against the rules in AGENTS.md. + +Checks the part of those rules that a machine can judge, at the moment of +writing rather than at review. Only lines that an edit added are reported, +found by diffing against git. A file that git does not track counts as new in +full. + +The checks are a floor, not a substitute for reading the prose. Clause-stacking +needs judgement, and so does an argument against a branch that is no longer +there: "would" is far too common a word to match on. + +A Markdown or reStructuredText file holds things that are not prose. The checks +skip a fenced code block, and the YAML frontmatter that opens the file. A +document that shows badly written prose as an example therefore keeps its +examples, as long as it fences them. + +Checks +------ +sphinx-field-list + ``:arg x:``/``:param x:``/``:returns:``/``:rtype:``, anywhere in a + docstring an edit touches -- not only on the lines it added. AGENTS.md + allows numpydoc only, and says that copying the neighbouring style is the + wrong instinct here: touching a docstring migrates the whole docstring, + not just the lines the edit added. +long-sentence + A sentence of more than MAX_WORDS words in a docstring or a comment. + ASD-STE100 asks for short sentences, one idea each. +past-tense + Wording that describes code which is not there any more. +hasattr-guard + ``if not hasattr(self, ...)`` standing in for a setup flag. +google-section + ``Args:``/``Returns:``/``Raises:``, the Google docstring headings, anywhere + in a docstring an edit touches. AGENTS.md allows numpydoc only, which + underlines a heading instead of following it with a colon. Reported over + the whole docstring for the reason sphinx-field-list is. +dof-loop + A Python ``for`` over ``.dat.data``, ``num_cells()`` or ``node_count``. + AGENTS.md asks for a form, a ``par_loop``, or Cython instead. Files under + ``firedrake/cython/`` and under a ``tests/`` directory are not checked: + the first exists to loop over mesh entities, and the second asserts over + meshes small enough for the loop to cost nothing. + +Usage +----- +This is a plain command-line program. Any agent, or any person, can run it. It +needs Python and ``git``, and nothing particular to one assistant. + +Check files from the command line. This exits 1 when it finds something, so a +pre-commit script can use it:: + + .agents/tools/check-prose.py firedrake/mg/utils.py + +Check a whole branch with ``--range``, which reads the lines a commit range +added rather than the lines the working tree changed. Give it the files to +look at, or none to take every file the range touches:: + + .agents/tools/check-prose.py --range main...HEAD + +Check out the head of the range first. The line numbers come from the diff, +and the prose comes from the working tree, so the two must agree. + +One assistant can also drive it automatically. Claude Code checks every edit +as you make it, through a ``PostToolUse`` hook. Put this in +``.claude/settings.local.json``, which git does not track, so that each +checkout opts in for itself:: + + { + "hooks": { + "PostToolUse": [ + { + "matcher": "Write|Edit", + "hooks": [ + { + "type": "command", + "command": "python3 \"$CLAUDE_PROJECT_DIR/.agents/tools/check-prose.py\"", + "timeout": 15 + } + ] + } + ] + } + } + +As a hook it reads the payload on stdin, always exits 0, and reports through +``systemMessage`` and ``additionalContext``. +""" +import ast +import io +import json +import os +import re +import subprocess +import sys +import tokenize + +MAX_WORDS = 25 +SOURCE_SUFFIXES = (".py", ".pyx", ".pxd", ".md", ".rst") +# These state the rules, so they quote the words they ban. This file names +# itself: the hook only ever sees the file an edit names, but --range sweeps +# every file a branch touches, and reaches this one. +EXEMPT_NAMES = {"AGENTS.md", "CLAUDE.md", "check-prose.py"} + +SPHINX = re.compile(r":(?:arg|param|returns?|rtype|raises|type|vartype)\b") +TELLS = re.compile( + r"(? set[int]: + """Find the lines of a Markdown or reStructuredText file that hold no prose. + + Parameters + ---------- + lines : list of str + The lines of the file, without their line endings. + + Returns + ------- + set of int + The 1-based numbers of the lines a fenced code block holds, and of the + YAML frontmatter that opens the file. Both fences and both frontmatter + markers are included. + + """ + skip = set() + body = 0 + # Frontmatter opens the file with a --- line, and a second one ends it. A + # file that opens with a rule and never closes one keeps all of its lines. + if lines and lines[0].strip() == "---": + for number in range(2, len(lines) + 1): + if lines[number - 1].strip() == "---": + skip.update(range(1, number + 1)) + body = number + break + fence = None + for number in range(body + 1, len(lines) + 1): + match = FENCE.match(lines[number - 1]) + if fence is None: + if match: + # The fence opens a block. Only the same marker closes it, so + # that ``` inside a ~~~ block stays part of the block. + fence = match.group(1) + skip.add(number) + else: + skip.add(number) + if match and match.group(1) == fence: + fence = None + return skip + + +def sentences(text): + """Split prose into sentences, ignoring code-like fragments.""" + flat = " ".join(text.split()) + return [s for s in SENTENCE_SPLIT.split(flat) if s] + + +def prose_blocks(path, source, added): + """Yield (line number, prose) for docstrings and comment runs that were added.""" + if not path.endswith((".py", ".pyx", ".pxd")): + # Treat a run of added prose lines as one block. + run, start = [], None + for number, line in enumerate(source.splitlines(), 1): + if number in added and line.strip() and not line.lstrip().startswith(("|", ">")): + run.append(line.strip()) + start = start or number + else: + if run: + yield start, " ".join(run) + run, start = [], None + if run: + yield start, " ".join(run) + return + + try: + tree = ast.parse(source) + except SyntaxError: + return + for node in ast.walk(tree): + if not isinstance(node, (ast.Module, ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef)): + continue + doc = ast.get_docstring(node) + if not doc: + continue + target = node.body[0] + span = range(target.lineno, (target.end_lineno or target.lineno) + 1) + if any(n in added for n in span): + # Stop at the first numpydoc section: those are structured, not prose. + body = re.split(r"\n\s*(?:Parameters|Returns|Raises|Notes|Examples)\s*\n", doc)[0] + yield target.lineno, body + + run, start = [], None + try: + tokens = list(tokenize.generate_tokens(io.StringIO(source).readline)) + except (tokenize.TokenError, IndentationError): + tokens = [] + for token in tokens: + if token.type == tokenize.COMMENT and token.start[0] in added: + run.append(token.string.lstrip("#").strip()) + start = start or token.start[0] + elif run and token.type not in (tokenize.NL, tokenize.NEWLINE, tokenize.INDENT, tokenize.DEDENT): + yield start, " ".join(run) + run, start = [], None + if run: + yield start, " ".join(run) + + +def touched_docstring_span(path, source, added): + """Return every line of a docstring that an edit touched, not just those it added. + + Touching one line of a docstring migrates the whole docstring: this widens + the sphinx-field-list check from the added lines to the docstring's full + span, so an old field-list line the edit left alone still gets caught. + + Parameters + ---------- + path : str + The file the docstring lives in. + source : str + The file's current contents. + added : set of int + The 1-based line numbers the edit added. + + Returns + ------- + set of int + The 1-based line numbers of every docstring with at least one line in + ``added``. + """ + span_lines = set() + if not path.endswith((".py", ".pyx", ".pxd")): + return span_lines + try: + tree = ast.parse(source) + except SyntaxError: + return span_lines + for node in ast.walk(tree): + if not isinstance(node, (ast.Module, ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef)): + continue + if not ast.get_docstring(node): + continue + target = node.body[0] + span = range(target.lineno, (target.end_lineno or target.lineno) + 1) + if any(n in added for n in span): + span_lines.update(span) + return span_lines + + +def check(path, commit_range=None): + """Return the findings for one file, as (line, rule, why, excerpt) tuples.""" + if not path or not os.path.isfile(path): + return [] + if os.path.basename(path) in EXEMPT_NAMES or not path.endswith(SOURCE_SUFFIXES): + return [] + # git resolves a path against -C, so give it one that does not move. + path = os.path.abspath(path) + + added = added_line_numbers(path, commit_range) + if not added: + return [] + with open(path, encoding="utf-8", errors="replace") as handle: + source = handle.read() + lines = source.splitlines() + if not path.endswith((".py", ".pyx", ".pxd")): + added -= unprosed_lines(lines) + + docstring_span = touched_docstring_span(path, source, added) + checks_dof_loop = not any(part in path for part in DOF_LOOP_EXEMPT_DIRS) + + findings = [] + for number in sorted(added | docstring_span): + if number > len(lines): + continue + line = lines[number - 1] + if SPHINX.search(line): + why = "Use numpydoc sections, not Sphinx field lists" + if number not in added: + why += " -- pre-existing, but this docstring was touched elsewhere" + findings.append((number, "sphinx-field-list", why, line.strip())) + if GOOGLE_SECTION.match(line): + why = "Use numpydoc sections, not Google headings" + if number not in added: + why += " -- pre-existing, but this docstring was touched elsewhere" + findings.append((number, "google-section", why, line.strip())) + if number not in added: + continue + if checks_dof_loop and DOF_LOOP.search(line): + findings.append((number, "dof-loop", + "Use a form, a par_loop, or Cython, not a Python loop " + "over mesh data", line.strip())) + if TELLS.search(line) or USED_TO.search(line): + findings.append((number, "past-tense", + "Describes code that may not be there any more", line.strip())) + if HASATTR.search(line): + findings.append((number, "hasattr-guard", + "Declare a boolean or use functools.cached_property", line.strip())) + + for number, text in prose_blocks(path, source, added): + for sentence in sentences(text): + words = len(sentence.split()) + if words > MAX_WORDS: + findings.append((number, "long-sentence", + f"{words} words; ASD-STE100 asks for one idea per sentence", + sentence[:110] + ("..." if len(sentence) > 110 else ""))) + + findings.sort() + return findings + + +def report(path, findings, limit=12): + """Format findings for one file as indented lines.""" + return "\n".join(f" {path}:{n} [{rule}] {why}\n {excerpt}" + for n, rule, why, excerpt in findings[:limit]) + + +def run_as_hook(): + """Report on the file a PostToolUse payload names. Always succeeds.""" + try: + payload = json.load(sys.stdin) + except (json.JSONDecodeError, ValueError): + return 0 + tool_input = payload.get("tool_input") or {} + path = tool_input.get("file_path") or (payload.get("tool_response") or {}).get("filePath") + findings = check(path) + if not findings: + return 0 + rules = sorted({rule for _, rule, _, _ in findings}) + json.dump({ + "systemMessage": f"AGENTS.md: {len(findings)} finding(s) in " + f"{os.path.basename(path)} ({', '.join(rules)})", + "hookSpecificOutput": { + "hookEventName": "PostToolUse", + "additionalContext": ( + "AGENTS.md check on lines this edit added. Fix these, or say why " + "each one is a false positive:\n\n" + report(path, findings) + ), + }, + }, sys.stdout) + return 0 + + +def annotated_code(path, source, added, span=10): + """Yield (line, comment, code) for the comment runs an edit added. + + ``code`` is what follows the comment, with every comment taken out, which + is what a reader who skips the prose actually sees. + """ + if not path.endswith((".py", ".pyx", ".pxd")): + return + lines = source.splitlines() + + def strip(number): + """Return a source line with any comment removed, or None if it is only one.""" + text = lines[number - 1] + bare = text.split("#")[0] if "#" in text and not _in_string(text) else text + return None if not bare.strip() else bare.rstrip() + + run, start = [], None + for number, text in enumerate(lines, 1): + stripped = text.strip() + if stripped.startswith("#") and number in added: + run.append(stripped.lstrip("#").strip()) + start = start or number + continue + if run: + code = [] + for following in range(number, min(number + span, len(lines)) + 1): + kept = strip(following) + if kept is None and code: + break + if kept is not None: + code.append(f"{following}\t{kept}") + yield start, " ".join(run), "\n".join(code) + run, start = [], None + + +def _in_string(text): + """Whether the first ``#`` of a line sits inside a string literal.""" + return text.count('"', 0, text.index("#")) % 2 or text.count("'", 0, text.index("#")) % 2 + + +def run_explain(paths, commit_range=None): + """Print each added comment beside the code it annotates. Always succeeds.""" + if commit_range and not paths: + paths = files_in_range(commit_range) + total = 0 + for path in paths: + path = os.path.abspath(path) + if not os.path.isfile(path) or os.path.basename(path) in EXEMPT_NAMES: + continue + added = added_line_numbers(path, commit_range) + if not added: + continue + with open(path, encoding="utf-8", errors="replace") as handle: + source = handle.read() + for line, comment, code in annotated_code(path, source, added): + print(f"\n=== {os.path.relpath(path)}:{line}") + print(f" COMMENT {comment}") + print(" CODE WITHOUT IT") + print("\n".join(f" {row}" for row in code.splitlines()) or " (nothing)") + total += 1 + print(f"\n{total} added comment(s). For each: does the code still say it?") + print("Yes -> delete the comment. No -> rewrite the code, not the comment.") + return 0 + + +def files_in_range(commit_range): + """Return the source files a commit range touches, relative to its root.""" + root = git("git", "rev-parse", "--show-toplevel").stdout.strip() + listing = git("git", "diff", "--name-only", commit_range).stdout + return [os.path.join(root, name) for name in listing.splitlines() + if name.endswith(SOURCE_SUFFIXES)] + + +def run_as_command(paths, commit_range=None): + """Report on the named files. Returns 1 if anything was found.""" + if commit_range and not paths: + paths = files_in_range(commit_range) + total = 0 + for path in paths: + findings = check(path, commit_range) + if findings: + print(report(os.path.relpath(path), findings, limit=len(findings))) + total += len(findings) + if total: + print(f"\n{total} finding(s). See AGENTS.md, and this file's docstring.") + return 1 + return 0 + + +if __name__ == "__main__": + args = sys.argv[1:] + if args and args[0] in ("-h", "--help"): + print(__doc__) + sys.exit(0) + explain = False + if args and args[0] == "--explain": + explain, args = True, args[1:] + commit_range = None + if args and args[0] == "--range": + if len(args) < 2: + print("--range needs a commit range, such as main...HEAD", file=sys.stderr) + sys.exit(2) + commit_range = args[1] + args = args[2:] + if explain: + sys.exit(run_explain(args, commit_range)) + if commit_range or args: + sys.exit(run_as_command(args, commit_range)) + sys.exit(run_as_hook()) diff --git a/.agents/tools/fdk b/.agents/tools/fdk new file mode 100755 index 0000000000..7b334d5d85 --- /dev/null +++ b/.agents/tools/fdk @@ -0,0 +1,637 @@ +#!/usr/bin/env bash +# fdk - helper for working on Firedrake. +# +# Gives the repeated work of a Firedrake session one stable command, so the +# rules that are easy to get wrong by hand are encoded once. Run +# `.agents/tools/fdk help` for the subcommands. +# +# Nothing here is specific to one machine or one checkout. + +set -uo pipefail + +TOOLS="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +# A copy kept outside the repo keeps working on a branch that predates this +# directory, but it can not find the source tree from its own location, so +# FIREDRAKE_SRC tells it. A copy in the repo needs no configuration. +SRC="${FIREDRAKE_SRC:-$(cd "$TOOLS/../.." && pwd)}" + +# Prefer the checked-out tools, which are versioned with the branch under test. +TOOLS_IN_REPO="$SRC/.agents/tools" +[ -d "$TOOLS_IN_REPO" ] || TOOLS_IN_REPO="$SRC/.claude/tools" +[ -d "$TOOLS_IN_REPO" ] || TOOLS_IN_REPO="$TOOLS" + +# --- Locate the virtual environment -------------------------------------- +# An install puts the source tree at $VENV/src/firedrake, so a checkout can +# usually find its own venv two directories up. An explicit FIREDRAKE_VENV +# wins, then an activated environment. +if [ -z "${FIREDRAKE_VENV:-}" ]; then + guess="$(cd "$SRC/../.." 2>/dev/null && pwd)" + if [ -n "${VIRTUAL_ENV:-}" ] && [ -x "$VIRTUAL_ENV/bin/python" ]; then + FIREDRAKE_VENV="$VIRTUAL_ENV" + elif [ -n "$guess" ] && [ -x "$guess/bin/python" ]; then + FIREDRAKE_VENV="$guess" + fi +fi + +if [ -z "${FIREDRAKE_VENV:-}" ] || [ ! -x "$FIREDRAKE_VENV/bin/python" ]; then + echo "fdk: cannot find the virtual environment; set FIREDRAKE_VENV" >&2 + exit 2 +fi + +VENV="$FIREDRAKE_VENV" +PY="$VENV/bin/python" + +# Firedrake asks for this on every import that does not have it. One thread +# per rank is also what a parallel test run wants. +export OMP_NUM_THREADS="${OMP_NUM_THREADS:-1}" + +# --- Kernel caches ------------------------------------------------------- +# Firedrake defaults these to $VIRTUAL_ENV/.cache, and to $HOME/.cache when +# no environment is activated, so an activated shell and a bare one compile +# into different caches and clear different ones. Pin them to the +# environment this run uses, so that a cache, the fingerprint recorded beside +# it, and `fdk clean` all name the same directory. An explicit setting wins, +# for a caller who is deliberately isolating a run. +export FIREDRAKE_TSFC_KERNEL_CACHE_DIR="${FIREDRAKE_TSFC_KERNEL_CACHE_DIR:-$VENV/.cache/tsfc}" +export PYOP2_CACHE_DIR="${PYOP2_CACHE_DIR:-$VENV/.cache/pyop2}" + +# --- Kernel cache freshness ---------------------------------------------- +# TSFC keys its cached kernels on the form signature and the compiler +# parameters; PyOP2 keys its compiled code on the generated source. Neither +# keys on the code generator itself, so an edit to tsfc/, pyop2/, FIAT or UFL +# leaves every kernel already on disk in place, and the next run tests the +# code generator that produced them rather than the one in the tree. That +# reports failures a correct run does not have, and hides ones it does. The +# fingerprint of those sources travels with the caches, so a run whose +# sources have moved clears them first. +STAMP="$PYOP2_CACHE_DIR/.fdk-codegen-fingerprint" + +# The trees a cached kernel is generated from. Firedrake's own tree is +# excluded except for the two packages it vendors: the rest of it is +# interpreted afresh on every run. +codegen_trees() { + local tree + for tree in "$SRC/tsfc" "$SRC/pyop2"; do + [ -d "$tree" ] && printf '%s\n' "$tree" + done + for tree in "$VENV"/src/*/; do + tree="${tree%/}" + [ -d "$tree/.git" ] || continue + [ "$tree" = "$SRC" ] && continue + printf '%s\n' "$tree" + done + return 0 +} + +# Hash the file list as well as the contents: adding or removing a file +# changes what runs even when no surviving file's text does. Content rather +# than timestamps, so that checking a commit out again, as a bisect does, +# matches the caches it already built. +codegen_fingerprint() { + local trees + mapfile -t trees < <(codegen_trees) + [ "${#trees[@]}" -gt 0 ] || { printf 'no-codegen-trees\n'; return 0; } + { + find "${trees[@]}" -type f -name '*.py' -print 2>/dev/null | LC_ALL=C sort + find "${trees[@]}" -type f -name '*.py' -print0 2>/dev/null \ + | LC_ALL=C sort -z | xargs -0 cat 2>/dev/null + } | sha1sum | cut -d' ' -f1 +} + +record_fingerprint() { + mkdir -p "$(dirname "$STAMP")" + codegen_fingerprint > "$STAMP" +} + +# Called before every test run. Silent, and costs a few tens of milliseconds, +# when nothing has moved. +ensure_fresh_caches() { + local now + now="$(codegen_fingerprint)" + [ "$now" = "$(cat "$STAMP" 2>/dev/null)" ] && return 0 + cat >&2 <<'EOF' +fdk: the code generators changed since these caches were built. Clearing + them, so that this run tests the tree rather than the kernels an + earlier tsfc/, pyop2/, FIAT or UFL left behind. +EOF + "$VENV/bin/firedrake-clean" >&2 || return 1 + mkdir -p "$(dirname "$STAMP")" + printf '%s\n' "$now" > "$STAMP" +} + +usage() { + cat <<'EOF' +fdk - helper for working on Firedrake + + fdk test [pytest args/paths...] + Run tests at the given process count, and print one deduplicated + summary instead of one report per rank. nprocs=1 runs pytest directly; + nprocs>1 runs it under an explicit mpiexec. Filters on the + parallel[match] marker, so only tests whose own nprocs equals the + launched communicator size run. + + Use this rather than a bare `pytest` on parallel-marked tests. A bare + pytest self-forks one mpiexec per test, which is slow and reports + failures that a correct run does not show. + + Warns on stderr and exits nonzero if nothing matched nprocs=N. Without + this, 0 tests selected prints no summary line and looks like a clean + pass. + + Clears the kernel caches first if the code generators have changed + since they were built, so that an edit to tsfc/, pyop2/, FIAT or UFL + cannot be tested against the kernels the previous version cached. See + `fdk clean`. + + fdk testraw [pytest args/paths...] + As `test`, but unfiltered, to read tracebacks. + + fdk lint [paths...] + `make srclint`, which is what CI runs, over the whole source tree. + Given paths, flake8 over those alone, to iterate on one file. + + fdk prose [paths...] + Check the lines an edit added against the prose rules in AGENTS.md. + + fdk prose --range ... [paths...] + As `prose`, but over the lines a commit range added, to check a whole + branch. Check out the head of the range first. + + fdk py [args...] + The interpreter of the virtual environment. A bare `python` is usually + the system one, which has no firedrake, pytest or flake8. + + fdk clean + firedrake-clean: drop the cached TSFC kernels and PyOP2 code. Do this + when a cached kernel links against an extension that a rebuild has + changed. The symptom is `undefined symbol: ...` from a cached .so. + + TSFC keys its cached kernels on the form, and PyOP2 keys its compiled + code on the generated source, so neither notices that the generator + itself has changed. `test`, `testraw` and `baseline` therefore run this + for you whenever tsfc/, pyop2/ or a dependency checkout has moved since + the caches were built; the fingerprint they compare against is recorded + here. + + fdk build + Rebuild the Cython extensions. Do this after a branch switch that + changes firedrake/cython/*.pyx. + + fdk baseline [pytest args/paths...] + Say which failures this branch introduced. Runs the tests at the merge + base with the upstream default branch, then at HEAD, and prints the + difference. Use this before calling a failure pre-existing: a failure + that neither run shares is the only kind this branch caused. + + Run this before reading any code, and before repeating someone else's + claim that a failure is new. A caller's belief about what upstream + does is not evidence, and a branch behind main shows failures that + main has already fixed, or has never had. + + This checks out the merge base and rebuilds, so it takes twice as long + as one run. Uncommitted work is stashed and restored. It restores the + branch at the end, including when a run fails. + + fdk explain [--range ...] [paths...] + Print each comment a change added, next to the code it annotates with + the comment stripped out. Read the stripped code and ask whether it + still says what the comment claims. If it does, the comment is + redundant; if it does not, the code needs the rewrite, not the comment. + See the auditing-comments skill. + + fdk testfile + Find the test file(s) that cover a source file, ranked by how many of + its top-level names they reference. Firedrake's test layout does not + mirror its source layout (variational_solver.py is covered by + test_solving_interface.py), so grepping for the module's own basename + misses the file. + + fdk testfile --from-content + As above, given a new test's own content on stdin instead of a source + file's path -- find what already covers it, before writing it. + + fdk show + Print one function or class, found by name rather than by line number. + Firedrake has files of several thousand lines, and the line numbers in + them move. + + fdk deps [name] + Report the component packages: where each one is installed, which + branch it has, and whether it is dirty or behind its remote. A bug can + live in any of them, and an API must be read from the installed source + rather than recalled. Given a name, print where that module comes from. + + fdk stack + Print the pull request stack: for each branch with an open PR, its + number, base, and how far it has diverged from its remote. A stacked + branch is rebased onto its base, so the bases are the order to do it in. + + fdk pr [--title ] [--body-file <path>] + Retitle or redescribe a pull request. `gh pr edit` fails against + repositories that still carry a classic project, so this goes through + the REST API, which does not read the project fields. + + fdk status + Say whether a test run is in flight. + + fdk where + Print the source tree and the virtual environment in use. + +Keep a copy outside the repo to work on a branch that predates this +directory, and point it at the source tree: + + export FIREDRAKE_SRC=/path/to/src/firedrake +EOF +} + +# Keep the FAILED/ERROR lines and the counts, shorten the paths, drop the +# duplicate report that each rank writes. Anchor the path substitution after +# the FAILED/ERROR word: a pattern that starts at the line eats that word too, +# and then the summary no longer says which test failed. +summarize() { + sed -E 's#^(FAILED|ERROR) .*/tests/#\1 tests/#' \ + | grep -aE '^(FAILED|ERROR)|[0-9]+ (passed|failed|error|skipped)' \ + | sort -u +} + +# Just the test identifiers that failed, one per line and sorted, so that two +# runs can be compared with `comm`. Rank order varies between runs, and every +# rank reports, so this deduplicates too. +failures() { + sed -E 's#^(FAILED|ERROR) .*/tests/#\1 tests/#' \ + | grep -aE '^(FAILED|ERROR) ' \ + | sed -E 's/ +- .*$//' \ + | sort -u +} + +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK"' EXIT + +pytest_common=(-q -p no:randomly --color=no -rf -m "parallel[match]") + +run_pytest() { + local nprocs="$1"; shift + if [ "$nprocs" = "1" ]; then + # Do not use `mpiexec -n 1`, which can hang at MPI_Finalize. + "$PY" -m pytest "$@" + else + mpiexec -n "$nprocs" --bind-to none "$PY" -m pytest "$@" + fi +} + +# A test only runs at the nprocs it is marked for (default 1). Selecting zero +# tests at a given nprocs is silent from `test` (summarize drops pytest's own +# "no tests ran" line along with mpiexec's abort noise) and, at nprocs>1, +# noisy and misleading from `testraw` (mpiexec's own error, not a real +# crash). Warn either way, on the same signal: no count line in the output. +warn_if_nothing_ran() { + local n="$1" out="$2" + grep -qE '[0-9]+ (passed|failed|error|skipped)' <<<"$out" && return 0 + cat >&2 <<EOF +fdk: nothing ran at nprocs=$n. A test runs only at the nprocs it is marked + for (default 1, from @pytest.mark.parallel(nprocs=N)); this selection + has none marked for $n. Check what a path is marked with: + grep -n 'pytest.mark.parallel' <path> +EOF + return 1 +} + +cmd="${1:-help}" +[ $# -gt 0 ] && shift + +case "$cmd" in + test) + [ $# -ge 1 ] || { echo "fdk test needs <nprocs>" >&2; exit 2; } + n="$1"; shift + ensure_fresh_caches + echo "########## nprocs=$n ##########" + out="$(run_pytest "$n" "${pytest_common[@]}" --tb=line "$@" 2>&1)" + printf '%s\n' "$out" | summarize + warn_if_nothing_ran "$n" "$out" + ;; + testraw) + [ $# -ge 1 ] || { echo "fdk testraw needs <nprocs>" >&2; exit 2; } + n="$1"; shift + ensure_fresh_caches + out="$(run_pytest "$n" "${pytest_common[@]}" --tb=short "$@" 2>&1)" + printf '%s\n' "$out" + warn_if_nothing_ran "$n" "$out" + ;; + lint) + if [ $# -eq 0 ]; then + # `make srclint` is what CI runs. It reaches the script directories + # too, which a plain flake8 skips: those hold files with no .py + # suffix, so they need `--filename=*`. + PATH="$VENV/bin:$PATH" make -C "$SRC" srclint && echo "srclint: clean" + else + "$PY" -m flake8 "$@" && echo "flake8: clean" + fi + ;; + prose) + "$PY" "$TOOLS_IN_REPO/check-prose.py" "$@" && echo "prose: clean" + ;; + py) + "$PY" "$@" + ;; + clean) + "$VENV/bin/firedrake-clean" && record_fingerprint + ;; + build) + PATH="$VENV/bin:$PATH" make -C "$SRC" extforce + ;; + baseline) + [ $# -ge 1 ] || { echo "fdk baseline needs <nprocs>" >&2; exit 2; } + n="$1"; shift + # Uncommitted work is the normal state when a failure needs + # attributing, and refusing it here is what pushes a session into + # reading code instead of running this. Stash it, and restore it + # however the run exits. + stashed="" + if [ -n "$(git -C "$SRC" status --porcelain --untracked-files=no)" ]; then + git -C "$SRC" stash push --quiet --message "fdk baseline $$" || exit 2 + stashed="yes" + echo "fdk baseline: stashed uncommitted work; restored when this exits" >&2 + fi + # A detached HEAD has no branch name to go back to, so remember the + # commit either way. + here="$(git -C "$SRC" symbolic-ref --quiet --short HEAD \ + || git -C "$SRC" rev-parse HEAD)" + unstash() { + [ -n "$stashed" ] || return 0 + git -C "$SRC" stash pop --quiet && stashed="" + } + upstream="$(git -C "$SRC" rev-parse --verify --quiet origin/main \ + || echo main)" + base="$(git -C "$SRC" merge-base HEAD "$upstream")" + echo "########## base $base ##########" + git -C "$SRC" checkout --quiet "$base" || exit 2 + # Restore the branch however this exits, so a failed run does not + # leave the tree on the merge base. This replaces the cleanup trap, so + # it has to remove the working directory too. + trap 'git -C "$SRC" checkout --quiet "$here"; \ + PATH="$VENV/bin:$PATH" make -s -C "$SRC" extforce >/dev/null 2>&1; \ + unstash; rm -rf "$WORK"' EXIT + PATH="$VENV/bin:$PATH" make -s -C "$SRC" extforce >/dev/null 2>&1 + ensure_fresh_caches + run_pytest "$n" "${pytest_common[@]}" --tb=no "$@" 2>&1 \ + | failures > "$WORK/base.txt" + git -C "$SRC" checkout --quiet "$here" + PATH="$VENV/bin:$PATH" make -s -C "$SRC" extforce >/dev/null 2>&1 + unstash + trap 'rm -rf "$WORK"' EXIT + echo "########## head $here ##########" + ensure_fresh_caches + run_pytest "$n" "${pytest_common[@]}" --tb=no "$@" 2>&1 \ + | failures > "$WORK/head.txt" + echo "########## introduced by $here ##########" + comm -13 "$WORK/base.txt" "$WORK/head.txt" | sed 's/^/ NEW /' + echo "########## fixed by $here ##########" + comm -23 "$WORK/base.txt" "$WORK/head.txt" | sed 's/^/ GONE /' + echo "########## pre-existing ##########" + comm -12 "$WORK/base.txt" "$WORK/head.txt" | sed 's/^/ SAME /' + ;; + explain) + "$PY" "$TOOLS_IN_REPO/check-prose.py" --explain "$@" + ;; + testfile) + [ $# -ge 1 ] || { echo "fdk testfile needs <path>, or --from-content" >&2; exit 2; } + # A script file, not a `python - <<PYEOF` heredoc: --from-content + # reads its own input from stdin, and a heredoc would consume stdin + # to deliver the script itself, leaving none for that. + cat > "$WORK/testfile.py" <<'PYEOF' +import ast +import re +import subprocess +import sys +from collections import Counter +from pathlib import Path + +# Boilerplate almost every test file imports, regardless of what it actually +# tests. Left in, when ranking by usage rather than by definition (see +# below), these would swamp the one name that names the feature under test, +# such as `NonlinearVariationalSolver`. +BOILERPLATE = { + "Function", "FunctionSpace", "VectorFunctionSpace", "TensorFunctionSpace", + "MixedFunctionSpace", "TestFunction", "TrialFunction", "TestFunctions", + "TrialFunctions", "Constant", "DirichletBC", "SpatialCoordinate", + "Mesh", "UnitSquareMesh", "UnitIntervalMesh", "UnitCubeMesh", + "RectangleMesh", "ExtrudedMesh", "PETSc", +} + + +def ranked_by_usage(names, tests_dir, min_breadth=2, limit=15): + """Rank test files by how many of `names` each one *references*. + + For content with only a handful of names to search on -- a new test, + not a whole source module -- counting occurrences favours a long file + that mentions one name in passing, or a short file fixated on just one + of them, over a file actually built around the same combination. This + counts distinct names present instead, and requires more than one, so + a single incidental match does not count as coverage. + """ + names = {n for n in names if len(n) >= 5} - BOILERPLATE + if len(names) < min_breadth: + return [] + pattern = "|".join(re.escape(n) for n in names) + proc = subprocess.run( + ["grep", "-rlE", pattern, "--include=test_*.py", str(tests_dir)], + capture_output=True, text=True, + ) + breadth = {} + for line in proc.stdout.splitlines(): + text = Path(line).read_text(encoding="utf-8", errors="replace") + breadth[line] = sum(1 for n in names if n in text) + ranked = sorted((p for p, b in breadth.items() if b >= min_breadth), + key=breadth.get, reverse=True) + return ranked[:limit] + + +def ranked_by_definition(names, tests_dir, limit=15): + """Rank test files by how many times each one *mentions* `names`. + + For a source module's full set of top-level definitions -- many, and + each one distinctive -- a raw count already tracks relevance well; the + file that uses a module's own vocabulary the most is its test file. + """ + pattern = "|".join(re.escape(n) for n in names) + proc = subprocess.run( + ["grep", "-rlE", pattern, "--include=test_*.py", str(tests_dir)], + capture_output=True, text=True, + ) + counts = Counter() + for line in proc.stdout.splitlines(): + text = Path(line).read_text(encoding="utf-8", errors="replace") + counts[line] = sum(text.count(n) for n in names) + return counts.most_common(limit) + + +src, arg = sys.argv[1], sys.argv[2] +tests_dir = Path(src) / "tests" + +if arg == "--from-content": + content = sys.stdin.read() + try: + tree = ast.parse(content) + except SyntaxError as error: + sys.exit(f"fdk testfile: cannot parse stdin: {error}") + # A single capitalized letter is a local (`F` the form, `V` the space), + # not a class, however common a coincidental match on it would be. + names = {node.id for node in ast.walk(tree) + if isinstance(node, ast.Name) and node.id[:1].isupper()} + if not names: + sys.exit("fdk testfile: stdin has no capitalized name to search for") + ranked = ranked_by_usage(names, tests_dir) + for test_path in ranked: + print(test_path) + sys.exit(0 if ranked else 1) + +path = arg +with open(path, encoding="utf-8", errors="replace") as handle: + source = handle.read() +try: + tree = ast.parse(source) +except SyntaxError as error: + sys.exit(f"fdk testfile: cannot parse {path}: {error}") +names = { + node.name + for node in ast.iter_child_nodes(tree) + if isinstance(node, (ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef)) + and not node.name.startswith("_") +} +if not names: + sys.exit(f"fdk testfile: {path} defines no top-level name to search for") +ranked = ranked_by_definition(names, tests_dir) +if not ranked: + sys.exit(f"fdk testfile: no test under {tests_dir} references a name from {path}") +width = len(str(ranked[0][1])) +for test_path, count in ranked: + print(f"{count:>{width}} {test_path}") +PYEOF + "$PY" "$WORK/testfile.py" "$SRC" "$1" + ;; + show) + [ $# -ge 2 ] || { echo "fdk show needs <file> <name>" >&2; exit 2; } + "$PY" - "$@" <<'PYEOF' +import ast +import sys + +path, name = sys.argv[1], sys.argv[2] +with open(path, encoding="utf-8", errors="replace") as handle: + source = handle.read() +try: + tree = ast.parse(source) +except SyntaxError as error: + sys.exit(f"fdk show: cannot parse {path}: {error}") +lines = source.splitlines() +found = False +for node in ast.walk(tree): + if not isinstance(node, (ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef)): + continue + if node.name != name: + continue + # A decorator sits above the def, and is part of what the reader needs. + start = min([node.lineno] + [d.lineno for d in node.decorator_list]) + for number in range(start, (node.end_lineno or start) + 1): + print(f"{number}\t{lines[number - 1]}") + found = True + print() +if not found: + sys.exit(f"fdk show: {path} defines no {name}") +PYEOF + ;; + deps) + if [ $# -ge 1 ]; then + "$PY" -c "import importlib,sys; m=importlib.import_module(sys.argv[1]); print(m.__name__, m.__file__)" "$1" + exit $? + fi + for repo in "$VENV"/src/*/; do + [ -d "$repo/.git" ] || continue + name="$(basename "$repo")" + branch="$(git -C "$repo" symbolic-ref --quiet --short HEAD || echo detached)" + dirty="" + [ -n "$(git -C "$repo" status --porcelain --untracked-files=no)" ] && dirty=" dirty" + track="$(git -C "$repo" rev-list --left-right --count HEAD...@{upstream} 2>/dev/null)" + [ -n "$track" ] && track=" ahead/behind ${track// //}" + printf '%-18s %s%s%s\n' "$name" "$branch" "$track" "$dirty" + done + echo + echo "modules:" + "$PY" - <<'PYEOF' +import importlib +for name in ("firedrake", "pyop2", "tsfc", "finat", "FIAT", "ufl", "petsc4py"): + try: + print(f" {name:<10} {importlib.import_module(name).__file__}") + except ImportError as error: + print(f" {name:<10} not importable: {error}") +PYEOF + ;; + stack) + for branch in $(git -C "$SRC" for-each-ref --format='%(refname:short)' refs/heads); do + info="$(gh pr view "$branch" --json number,baseRefName,title \ + -q '"#\(.number)\t\(.baseRefName)\t\(.title)"' 2>/dev/null)" || continue + track="$(git -C "$SRC" rev-list --left-right --count "$branch...origin/$branch" 2>/dev/null)" + [ -n "$track" ] && track="${track// //}" || track="-" + printf '%-38s %s [%s]\n' "$branch" "$info" "$track" + done + echo + echo "columns: branch, PR, base, title, [ahead/behind origin]" + ;; + pr) + [ $# -ge 1 ] || { echo "fdk pr needs <number>" >&2; exit 2; } + "$PY" - "$@" <<'PYEOF' +import json +import subprocess +import sys + +number, args = sys.argv[1], sys.argv[2:] +payload = {} +while args: + flag, args = args[0], args[1:] + if flag == "--title": + payload["title"], args = args[0], args[1:] + elif flag == "--body-file": + with open(args[0], encoding="utf-8") as handle: + payload["body"] = handle.read() + args = args[1:] + else: + sys.exit(f"fdk pr: unknown option {flag}") +if not payload: + sys.exit("fdk pr: give --title or --body-file") + +repo = subprocess.run(["gh", "repo", "view", "--json", "nameWithOwner", + "-q", ".nameWithOwner"], capture_output=True, text=True) +if repo.returncode: + sys.exit(repo.stderr.strip()) +# gh pr edit reads the project fields, which fail on a repository that still +# carries a classic project. The REST API does not read them. +done = subprocess.run(["gh", "api", "-X", "PATCH", + f"repos/{repo.stdout.strip()}/pulls/{number}", "--input", "-"], + input=json.dumps(payload), capture_output=True, text=True) +if done.returncode: + sys.exit(done.stderr.strip()) +result = json.loads(done.stdout) +print(f"#{result['number']} {result['title']}") +print(result["html_url"]) +PYEOF + ;; + status) + if pgrep -f "m pytest" >/dev/null 2>&1; then + echo "test run in flight ($(pgrep -cf 'm pytest') pytest processes)" + else + echo "no test run in flight" + fi + ;; + where) + echo "source: $SRC" + echo "environment: $VENV" + ;; + help|-h|--help) + usage + ;; + *) + echo "fdk: unknown subcommand '$cmd'" >&2 + usage >&2 + exit 2 + ;; +esac diff --git a/.agents/tools/require-fdk.py b/.agents/tools/require-fdk.py new file mode 100755 index 0000000000..844823e72c --- /dev/null +++ b/.agents/tools/require-fdk.py @@ -0,0 +1,168 @@ +#!/usr/bin/env python3 +"""Deny a bare python/pytest/flake8 call, or a new test file, and name the fdk +command instead. + +AGENTS.md says to call the virtual environment's interpreter, never a bare +`python`. It says to use `fdk test`/`fdk testraw`/`fdk lint`, never pytest or +flake8 directly. It says to add a test to the file `fdk testfile` names, +never a new one. An agent that never reads any of that still meets this +hook, before the command runs. It is a Claude Code PreToolUse hook, wired +into `.claude/settings.json` so it applies by default, to every session in +this checkout, including a subagent's. + +`python` is the one to deny hardest. It is the first command almost any +session reaches for, well before a test or a lint pass. A bare `python` in +this checkout is the system interpreter, which has no firedrake. Denying it +turns that first command into the moment a session finds fdk, instead of a +stale-looking `ModuleNotFoundError` it might paper over. + +Usage +----- +Reads the PreToolUse payload on stdin, and always exits 0; a denial goes in +the JSON reply, not the exit code. Test a case from the command line, +without the JSON envelope:: + + .agents/tools/require-fdk.py --check "python -c 'import firedrake'" + .agents/tools/require-fdk.py --check-file tests/firedrake/regression/test_new.py < new.py +""" +import json +import os +import re +import subprocess +import sys + +# A command word at the start of a command, or right after a shell separator +# or substitution. Not the word turning up inside a quoted argument, such as +# `grep -n pytest.mark.parallel`. Not part of a path either, such as +# `$VIRTUAL_ENV/bin/python`. +BOUNDARY = r"(?:^|[;&|`]|\$\()\s*" +PYTEST = re.compile(BOUNDARY + r"(?:python3?\s+-m\s+)?pytest\b", re.MULTILINE) +FLAKE8 = re.compile(BOUNDARY + r"(?:python3?\s+-m\s+)?flake8\b", re.MULTILINE) +PYTHON = re.compile(BOUNDARY + r"python3?\b", re.MULTILINE) +# A command naming fdk is never what this denies -- not only its own +# subcommands, but a person reading the script's source, e.g. `cat +# .agents/tools/fdk`. fdk's own python/pytest/flake8 calls never reach this +# hook at all: they are a subprocess fdk spawns, not a separate Bash tool +# call. +FDK = re.compile(r"\bfdk\b") + + +def verdict(command): + """Return a denial reason for `command`, or None to let it through.""" + if not command or FDK.search(command): + return None + if PYTEST.search(command): + return ( + "DENIED: pytest does not run directly in this repo. fdk is the " + "only tool for tests here -- not a preference, a requirement. " + "Use `.agents/tools/fdk test <nprocs> <paths>` or " + "`.agents/tools/fdk testraw <nprocs> <paths>`. " + "Run `.agents/tools/fdk help` for every subcommand." + ) + if FLAKE8.search(command): + return ( + "DENIED: flake8 does not run directly in this repo. fdk is the " + "only tool for linting here -- not a preference, a requirement. " + "Use `.agents/tools/fdk lint [paths]`. " + "Run `.agents/tools/fdk help` for every subcommand." + ) + if PYTHON.search(command): + return ( + "DENIED: a bare python/python3 does not run in this repo -- it " + "is the system interpreter, which has no firedrake. fdk is the " + "only tool for this -- not a preference, a requirement. " + "Use `.agents/tools/fdk py [args]`. " + "Run `.agents/tools/fdk help` for every subcommand." + ) + return None + + +# A test file, by Firedrake's own convention: a `test_*.py` under a `tests` +# directory component, wherever that component sits in the path. +TEST_FILE = re.compile(r"(^|/)tests/.*/test_[^/]+\.py$") +FDK_BIN = os.path.join(os.path.dirname(os.path.abspath(__file__)), "fdk") + + +def candidate_test_files(content, limit=5): + """Return existing test files `fdk testfile --from-content` names. + + Delegates to fdk rather than ranking files itself. fdk is the one place + that heuristic should live. That keeps it a single implementation an + agent can run directly, not a second copy only this hook uses. + """ + proc = subprocess.run( + [FDK_BIN, "testfile", "--from-content"], + input=content, capture_output=True, text=True, + ) + return proc.stdout.splitlines()[:limit] if proc.returncode == 0 else [] + + +def new_test_file_verdict(path, content): + """Return a (decision, reason) pair for writing a new test file, or None. + + A genuinely new feature area can have nothing to extend, so this is not + always wrong the way a bare python/pytest/flake8 is. When an existing + file plausibly already covers this one, deny and name the candidates. + Otherwise ask the person in the session to confirm. An unattended + subagent has nobody to answer, so it gets the safe default, while a + present human can approve the one call that needs it. + """ + if not path or not TEST_FILE.search(path) or os.path.exists(path): + return None + candidates = [c for c in candidate_test_files(content) if c] + if candidates: + listed = ", ".join(os.path.relpath(c) for c in candidates) + return "deny", ( + f"DENIED: this may already be covered -- {listed} reference the " + "same names, per `fdk testfile --from-content`. Run " + "`.agents/tools/fdk testfile <source-path>` on the file you are " + "actually changing, which reads far more than that content-only " + "search can, and add the test to what it names." + ) + return "ask", ( + "No existing test file references what this one tests, per `fdk " + "testfile --from-content`. Run `.agents/tools/fdk testfile " + "<source-path>` to check properly before confirming that a new " + "file is really needed." + ) + + +def main(): + if len(sys.argv) == 3 and sys.argv[1] == "--check": + print(verdict(sys.argv[2]) or "allowed") + return 0 + if len(sys.argv) == 3 and sys.argv[1] == "--check-file": + # Content comes from stdin, not from `path` on disk: the whole point + # is to judge a file as though it does not exist there yet. + content = sys.stdin.read() + print((new_test_file_verdict(sys.argv[2], content) or (None, "allowed"))[1]) + return 0 + + try: + payload = json.load(sys.stdin) + except (json.JSONDecodeError, ValueError): + return 0 + tool_name = payload.get("tool_name") + tool_input = payload.get("tool_input") or {} + if tool_name == "Bash": + decision, reason = "deny", verdict(tool_input.get("command", "")) + elif tool_name == "Write": + decision, reason = new_test_file_verdict( + tool_input.get("file_path", ""), tool_input.get("content", "") + ) or (None, None) + else: + decision, reason = None, None + if reason is None: + return 0 + json.dump({ + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": decision, + "permissionDecisionReason": reason, + }, + }, sys.stdout) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 0000000000..b858af6cfe --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,19 @@ +{ + "worktree": { + "bgIsolation": "none" + }, + "hooks": { + "PreToolUse": [ + { + "matcher": "Bash|Write", + "hooks": [ + { + "type": "command", + "command": "python3 \"$CLAUDE_PROJECT_DIR/.agents/tools/require-fdk.py\"", + "timeout": 5 + } + ] + } + ] + } +} diff --git a/.claude/skills b/.claude/skills new file mode 120000 index 0000000000..2b7a412b8f --- /dev/null +++ b/.claude/skills @@ -0,0 +1 @@ +../.agents/skills \ No newline at end of file diff --git a/.claude/tools b/.claude/tools new file mode 120000 index 0000000000..3b7dea709d --- /dev/null +++ b/.claude/tools @@ -0,0 +1 @@ +../.agents/tools \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index de62cc1d29..4ca5272a98 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -33,137 +33,155 @@ toolchain: ## Core Working Rules -* **Mathematical Root Causes:** Bug fixes must address the underlying core mathematical or - architectural issue. Do not merely patch particular failing test cases or edge cases. -* **Generality Over Complexity:** Avoid increasing code complexity with complicated bookkeeping or - special-case logic. Firedrake relies on the mathematical generality of finite elements. -* **Unified Abstractions:** Proper Firedrake code avoids branching on the wide range of discretizations - (e.g., cell type, polynomial degree, or finite element family) or execution states (serial vs. MPI - parallel). Rely on UFL, TSFC, PyOP2, and PETSc abstractions to handle these variations - transparently. -* **Preserve Style:** Preserve Firedrake style and naming conventions. Keep edits minimal and local to - the requested change. Match existing patterns in the package you are modifying. -* **Avoid Duplication:** Avoid unnecessary code duplication. Prefer reusing or extending nearby logic - when it keeps behavior clear and local. Do not add speculative abstractions or broad refactors unless - explicitly requested. -* **Do Not Trust Memorized API Shapes:** Firedrake, UFL, and PETSc/petsc4py APIs change over time — - properties become methods, arguments get renamed, call signatures get deprecated. An LLM's trained - knowledge reflects a snapshot that may already be stale, and will confidently reproduce the old, - no-longer-correct form (e.g. calling a now-method as a bare attribute, or vice versa). Before calling - an API you have not just seen used in this codebase, verify its actual current signature by reading - the installed Firedrake/UFL/PETSc source rather than relying on memorized patterns. -* **Document The Present, Not The Past:** When fixing code that was wrong, do not leave comments or - prose explaining what the removed, incorrect approach used to do or why it was wrong. Keep comments - and documentation focused on the current, correct code; a reader should never need the history of - what used to be there to understand why the present code is right. +* **Mathematical Root Causes:** Fix the underlying mathematical or architectural cause. Do not patch + individual failing test cases. +* **Generality Over Complexity:** Rely on the mathematical generality of finite elements. Do not add + special-case bookkeeping or branching. +* **Unified Abstractions:** Do not branch on cell type, polynomial degree, element family, or serial vs. + MPI-parallel execution. Use the UFL/TSFC/PyOP2/PETSc abstraction that already handles it — see + Anti-Patterns. +* **Preserve Style:** Match the naming and patterns of the package you are editing. Keep edits minimal + and local to the requested change. +* **Avoid Duplication:** Reuse or extend nearby logic instead of duplicating it. Do not add speculative + abstractions or broad refactors unless asked. +* **Verify API Shapes:** Read a Firedrake, UFL, or PETSc/petsc4py API's current signature from the + installed source before calling it, unless you have just seen it used in this codebase. `fdk deps` + finds where each component package lives. +* **Document The Present, Not The Past:** Do not describe a removed or rejected approach in a comment or + docstring. Document only what the current code does. Checked by `fdk prose` — see Anti-Patterns. ## Coding Style And Conventions -* **Class Attributes:** Every attribute a class can hold must be declared in one visible place, either - initialized in the constructor (`__init__`) or, for state that is expensive or unnecessary to compute - eagerly, declared as a `functools.cached_property`. Avoid discovering an attribute's existence via - `hasattr`/`setattr`/`getattr` scattered across methods — laziness is fine, ad hoc laziness is not. -* **No Python Mesh Loops:** The Firedrake style strictly avoids using Python `for` loops to iterate - over degrees of freedom (DoFs) or cells in a mesh. -* **Prefer Code Generation/PETSc:** All mesh-level or DoF-level operations must be implemented using - PyOP2-driven kernels or DMPlex operations. These should be accessed either through `petsc4py` or - Firedrake's internal Cython wrappers. -* **NumPy Is Fine, Repeatedly Touching Whole Arrays Is Not:** NumPy is the right tool for index - computations, small metadata configurations, and vectorized pre/post-processing. The anti-pattern is - not "using NumPy" but iterating a large array element-by-element (in a Python `for` loop) or - otherwise touching the same whole array repeatedly outside of a single vectorized expression — that - is what defeats NumPy's own performance model, on top of bypassing PyOP2/code-generation for - mesh-bound data. -* **Docstrings:** All public-facing APIs must include properly formatted `numpydoc`-style docstrings. -* **Type Hints:** New code should include type hints on function/method signatures. +* **Class Attributes:** Declare every attribute in `__init__`, or as a `functools.cached_property` for + state that is expensive to compute eagerly. Do not discover an attribute via + `hasattr`/`setattr`/`getattr`. See Anti-Patterns; the setup-guard case is checked by `fdk prose`. +* **No Python Mesh Loops:** Never iterate over degrees of freedom or cells with a Python `for` loop. +* **Prefer Code Generation/PETSc:** Implement mesh-level or DoF-level operations through PyOP2-driven + kernels or DMPlex, via `petsc4py` or Firedrake's Cython wrappers. See Anti-Patterns. +* **NumPy For Vectorized Work Only:** Use NumPy for index computation and vectorized pre/post-processing. + Do not iterate a large array element-by-element, or touch the same whole array repeatedly outside one + vectorized expression. +* **Docstrings Are Always `numpydoc`:** Use numpydoc section headings (`Parameters`, `Returns`, `Raises`, + `Notes`) in every docstring you write or touch, including private helpers and Cython functions in + `firedrake/cython/*.pyx`. Never the old Sphinx field-list style (`:arg x:`, `:param x:`, `:returns:`, + `:rtype:`), even where the surrounding file already uses it. Tests (`tests/**`) need only a one- or + two-sentence summary. Checked by `fdk prose`. +* **Plain English (ASD-STE100) In Docstrings And Comments:** Short sentences, one idea each, active + voice, subject named up front rather than buried in a relative clause. Checked by `fdk prose` — see + the Clause-Stacked anti-pattern below. +* **Type Hints:** Add type hints to new function and method signatures, and to any parameter or + return value you add to an existing one. The codebase is mid-migration and inconsistently typed + elsewhere — do not retrofit a signature beyond what you touch. +* **Demos Are Literate Programs:** Keep `demos/<name>/<name>.py.rst` prose and code in step. A paragraph + ending in `::` makes the following indented block executable; a `.. code-block:: python` directive + renders in the docs but does not run. Prefer `::`. ## Testing Requirements -* **Pull Requests:** All PRs must include comprehensive tests demonstrating that the new feature works - or the bug is fixed. -* If behavior changes, update the relevant test blocks and ensure that parallel runs (MPI) yield - correct and identical mathematical results to serial runs. -* Keep tests targeted. Add or update the narrowest test that proves the behavior you changed. -* Do not create new test files for this. Add the new test(s) to the existing test file(s) that already - cover the feature or module being changed. +* Add tests that demonstrate the new feature or bug fix, in the existing test file for that module — + do not create a new one. `fdk testfile <path>` finds it: Firedrake's test layout does not mirror + its source layout, so the file's own basename is not a reliable guide. +* When behavior changes, update the affected tests and confirm parallel (MPI) runs match serial results. +* Add or update the narrowest test that proves the change. ## Pull Request Expectations -* All changes are expected to arrive through GitHub Pull Requests. -* Keep diffs reviewable and focused. -* Before concluding work, ensure `make srclint` passes, and verify that the relevant subset of the - pytest test suite succeeds locally. +* All changes land through GitHub pull requests. Keep diffs focused. +* Before requesting review: `fdk lint`, `fdk prose --range main...HEAD`, and the relevant `fdk test`. ## Development Toolchain +### Agent Tools + +`.agents/tools/fdk` is the required interface for tests and lint in this repo, not one option among +several: a bare `pytest` or `flake8` is denied outright, by a `PreToolUse` hook this checkout runs +by default. It is not on `PATH`, so call it by that path (or symlink it onto your own). Run +`.agents/tools/fdk help` for the full reference; the commands in daily use: + +```bash +.agents/tools/fdk test <nprocs> [paths] # tests at a process count, one deduplicated summary +.agents/tools/fdk testraw <nprocs> [paths] # as test, unfiltered, to read a traceback +.agents/tools/fdk baseline <nprocs> [paths] # which failures this branch introduced +.agents/tools/fdk lint [paths] # make srclint, or flake8 on given paths +.agents/tools/fdk prose [--range base...head] [paths] # prose rules, on an edit or over a branch +.agents/tools/fdk explain [--range base...head] [paths] # each added comment beside its code +.agents/tools/fdk testfile <path> # the test file(s) that cover a source file +.agents/tools/fdk show <file> <name> # one function or class, found by name +.agents/tools/fdk deps [name] # component packages: location, branch, dirty state +.agents/tools/fdk stack # the PR stack, and how far each branch has drifted +.agents/tools/fdk pr <number> [--title ...] [--body-file ...] # retitle/redescribe a PR +.agents/tools/fdk build / clean / py [args] / status +``` + +Use `fdk`, not the shell line it wraps: it always calls the virtual environment's interpreter, +filters parallel tests correctly, and needs no configuration in a normal checkout. + +Run `fdk prose` on files you edit, and `fdk prose --range main...HEAD` on the whole branch before +requesting review. `check-prose.py --help` prints the `PostToolUse` hook config that runs it on +every edit automatically inside Claude Code; that is one way to drive it, not the only one. + ### Environment Setup -* **Editable installs across the stack:** A bug can live in Firedrake or in any of its component - packages (PETSc, petsc4py, UFL, FIAT, FInAT, TSFC, PyOP2, loopy). Follow the - ["Editing subpackages"](https://firedrakeproject.org/install.html#editing-subpackages) instructions - in the install docs to get a component installed in editable mode so source edits take effect without - reinstalling, and check which branch/commit of each component is actually active before assuming a - fix belongs in Firedrake itself. -* **`petsc4py`/PETSc version skew:** `petsc4py` is a compiled extension built against one specific - PETSc checkout. If you switch the PETSc branch/commit underneath an existing venv (e.g. to bisect a - PETSc-side issue) without rebuilding `petsc4py` against it, `import firedrake` fails with a confusing - `undefined symbol: ...` error from `petsc4py`'s `.so` — not a Firedrake traceback, and easy to - misattribute to whatever you were just changing. Rebuild `petsc4py` (and re-run - `pip install --no-build-isolation -e .` for it) after switching PETSc, rather than debugging the - symptom. -* **Caching:** Generated TSFC kernels and compiled PyOP2 code are cached on disk, under - `FIREDRAKE_TSFC_KERNEL_CACHE_DIR`/`PYOP2_CACHE_DIR`. These are not pre-set shell variables — do not - expect `echo $PYOP2_CACHE_DIR` to show anything. `firedrake.configuration.setup_cache_dirs()` sets - them in-process, defaulting to `$VIRTUAL_ENV/.cache/{tsfc,pyop2}`, as one of the first things - `import firedrake` does (right after PETSc initialization, before PyOP2 loads) unless you already - exported them yourself beforehand. This also means that if PETSc initialization itself fails (e.g. - the version-skew symptom above), these variables never get set at all. If a code-generation change - does not seem to take effect, or you suspect a stale kernel, run `firedrake-clean` before re-testing - (it prints the actual paths in use). +* **Editable installs across the stack:** Install components in editable mode (see + ["Editing subpackages"](https://firedrakeproject.org/install.html#editing-subpackages)) so source + edits take effect without reinstalling. `fdk deps` reports each component's location, branch, and + dirty state — check it before assuming a fix belongs in Firedrake itself. +* **`petsc4py`/PETSc version skew:** After switching the PETSc branch or commit under an existing venv, + rebuild `petsc4py` (`pip install --no-build-isolation -e .`) before doing anything else. A stale + `petsc4py` fails `import firedrake` with an `undefined symbol: ...` error that looks unrelated to + PETSc. +* **Caching:** Generated TSFC kernels and compiled PyOP2 code are cached under + `FIREDRAKE_TSFC_KERNEL_CACHE_DIR`/`PYOP2_CACHE_DIR` (default `$VIRTUAL_ENV/.cache/{tsfc,pyop2}`), set + in-process by `firedrake.configuration.setup_cache_dirs()` on `import firedrake`. TSFC keys a cached + kernel on the form and the compiler parameters, and PyOP2 keys compiled code on the generated source. + Neither keys on the code generator, so an edit to `tsfc/`, `pyop2/`, FIAT or UFL leaves every kernel + already on disk in place. `fdk test`, `fdk testraw` and `fdk baseline` compare a fingerprint of those + sources against the caches and clear them when it has moved, so you do not have to remember `fdk clean`. +* **A stale kernel reads as a wrong answer, not as a stale kernel:** the run imports your edited Python + and executes someone else's C. The result is a plausible number, a solver that diverges, or a test + that fails on numerics. Treat any code-generation change whose test result you have not re-run + from cleared caches as unmeasured. * **Smoke test after install/rebuild:** `firedrake-check` runs a small grouped-by-process-count subset - of the regression suite; use it to sanity-check an environment before investing time in a full test - run. + of the regression suite; use it to sanity-check an environment before a full test run. ### Testing -* **Parallel tests:** Tests that must run under MPI are marked `@pytest.mark.parallel` (optionally - `@pytest.mark.parallel(nprocs=N)` or `@pytest.mark.parallel([1, 3])` for multiple process counts), via - the `mpi-pytest` plugin. Plain `pytest test_foo.py` does exercise them: for each parallel test it - self-forks an `mpiexec` subprocess with the right `nprocs`, one test at a time, which is slow and - produces one nested pytest report per test. To instead run every `nprocs=3` test in `test_foo.py` - together, directly under a single outer `mpiexec`, filter on the `parallel[match]` marker that the - plugin attaches to tests whose `nprocs` equals the launched communicator size: - ```bash - mpiexec -n 3 python -m pytest -m "parallel[match]" test_foo.py - ``` - Tests requiring a different `nprocs` are collected but skipped (not run) by this invocation; do not - conclude a parallel code path is untested just because a plain, unmarked `pytest` run was green. -* **Splitting for CI:** `firedrake-run-split-tests` shards the suite by process count for CI; look at - it (and `.github/workflows/pr.yml`/`core.yml`) if a failure only reproduces in CI and not locally. -* **Narrow reproduction first:** Run the single failing test node (`pytest path::test_name -k ...`) - before the full module; the suite is large and full-module reruns are slow to iterate against. +* **Attribute a failure before you analyse it.** Run `fdk baseline <nprocs> <paths>` on any failure + you are asked to fix, before reading code. It reports which failures this branch introduced, which + it fixed, and which it shares with the merge base. A failure the merge base also has is not yours. +* **A premise you were handed is not evidence.** "This passes on main", "this test is untouched, so + the regression is in my change", and "the other configuration works" each name a fact that one + command settles and that hours of reading cannot. Check the ones your search depends on first. A + branch is often behind main as well: `git log --oneline HEAD..origin/main | wc -l` says how far, and + a failure that main has already fixed is not a bug in this branch. +* Tests that must run under MPI are marked `@pytest.mark.parallel` (optionally + `@pytest.mark.parallel(nprocs=N)` or `@pytest.mark.parallel([1, 3])`); an unmarked test's own nprocs + is 1. Use `fdk test <nprocs> <paths>` or `fdk testraw`, never a bare `pytest`, on parallel-marked + tests. + `firedrake-run-split-tests <nprocs> <njobs> <pytest args> <paths>` shards a run the way CI does + (`.github/workflows/core.yml`); run it from a scratch directory. +* Pick `<nprocs>` from what the target actually declares, not from a guess: `grep -n + 'pytest.mark.parallel' <path>` lists its markers. `fdk test`/`fdk testraw` select zero tests, and + say so on stderr, when nothing in the given paths is marked for the `<nprocs>` you passed. +* Run the relevant subset, not a plain serial `pytest <dir>`: the tests that exercise the lines you + changed, at the process counts where those lines are live. +* Reproduce narrowly first: run the single failing test node (`pytest path::test_name -k ...`) before + the full module. ### Debugging -* **Generated kernels (niche, rarely needed):** By default, generated C is compiled optimized and - without debug symbols, so a debugger attached to the Python process cannot meaningfully step through - it. Set `PYOP2_DEBUG=1` to compile with `-O0 -g` instead, which is the prerequisite for using - `gdb`/`cgdb` on the compiled kernel at all. -* **Cross-rank code-generation mismatches:** If a parallel run raises `CompilationError: Generated code - differs across ranks`, the mismatching per-rank source is dumped under - `<cache_dir>/mismatching-kernels/src-rank*.c`. Diffing the two sources only tells you *what* differs; - the actual fix is almost always upstream of that, in whatever Python-level parameter or branch is - computed differently per rank and fed into code generation (e.g. a rank-local decision that should be - a collective/global one) — make that decision the same on every rank, rather than patching the - generated source or the difference itself. -* **Parallel deadlocks (niche, rarely needed):** `PYOP2_SPMD_STRICT=1` adds barriers around calls - marked `@collective` and around cache access, trading overhead for a much narrower failure point when - ranks disagree about control flow. -* **Logging:** `firedrake.logging.set_log_level()` (or the `PYOP2_LOG_LEVEL` environment variable) - raises verbosity of Firedrake's/PyOP2's own logger, independent of PETSc's `-log_view`/`-info`. -* **PETSc-level diagnostics:** Since the linear/nonlinear solve ultimately runs through petsc4py, - standard PETSc options (`-ksp_view`, `-snes_view`, `-ksp_monitor`, `-log_view`, `-start_in_debugger`) - can be passed through Firedrake's `solver_parameters` or the command line exactly as in a plain PETSc +* **Generated kernels (niche, rarely needed):** Set `PYOP2_DEBUG=1` to compile generated C with + `-O0 -g`, the prerequisite for `gdb`/`cgdb` on a compiled kernel. +* **Cross-rank code-generation mismatches:** `CompilationError: Generated code differs across ranks` + dumps the mismatching per-rank source under `<cache_dir>/mismatching-kernels/src-rank*.c`. Fix the + Python-level value that is computed differently per rank and fed into code generation — make that + decision the same on every rank, rather than patching the generated source. +* **Parallel deadlocks (niche, rarely needed):** `PYOP2_SPMD_STRICT=1` adds barriers around + `@collective` calls and cache access, to narrow down where ranks disagree about control flow. +* **Logging:** `firedrake.logging.set_log_level()` (or `PYOP2_LOG_LEVEL`) sets Firedrake/PyOP2 log + verbosity, independent of PETSc's `-log_view`/`-info`. +* **PETSc-level diagnostics:** Pass PETSc options (`-ksp_view`, `-snes_view`, `-ksp_monitor`, + `-log_view`, `-start_in_debugger`) through `solver_parameters` or the command line, as in any PETSc application. ### Reproducible Environments @@ -367,3 +385,48 @@ code generation depends on. These loops are compiled and typed (`cdef`/`PetscInt objects — that combination, not mere placement in a `.pyx` file, is what makes them acceptable. Do not use this as license to write a plain Python loop over `.dat.data` and call it fine because "Firedrake has C-level loops elsewhere." + +### Clause-Stacked Docstrings And Comments + +WRONG — the subject hides inside a relative clause the reader must unwind before finding the verb: + +```python +def scale_boundary_nodes(u, factor): + """Give the nodes a boundary condition constrains their scaled values.""" +``` + +RIGHT — subject named up front, one short sentence, active voice: + +```python +def scale_boundary_nodes(u, factor): + """Scale the values of the nodes that a boundary condition constrains.""" +``` + +### Documenting Code That Is Not There + +A reader has only the file in front of them. A comment can describe a removed approach. It can also +argue against a branch the code does not take. Either one sends the reader looking for something +that is not there. + +WRONG — the first sentence describes deleted code, and the second argues with an absent branch: + +```python +def cell_average(u): + # This no longer divides by the number of cells, which was wrong when + # the cells had different sizes. A test for an empty mesh here would + # return a nan. + return assemble(u*dx) / assemble(1*dx) +``` + +RIGHT — say what the present code does, and state the condition it relies on: + +```python +def cell_average(u): + # Divide by the measured volume, so that cells of different sizes + # contribute in proportion. The caller passes a non-empty mesh. + return assemble(u*dx) / assemble(1*dx) +``` + +Some words give this away on sight: "used to", "previously", "no longer", "instead of", "we removed", +"this replaces". Watch equally for "would" when its subject is code that does not exist. An argument +against a branch that nobody can see is still a description of the past. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000000..43c994c2d3 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +@AGENTS.md