Agent tooling: sharper prose rules, a prose checker, and an `fdk` helper by pbrubeck · Pull Request #5338 · firedrakeproject/firedrake

pbrubeck · 2026-08-08T10:57:02Z

Description

AI-assisted (Claude Code)

Goal

An AI coding agent should get rules it will actually follow. A rule that lives only as prose in a
document gets missed — an agent (or a person, skimming) reads past it. A rule a program checks
gets caught every time. This PR is the tooling behind that idea: sharper rules in AGENTS.md, a
checker that enforces the ones a machine can judge, and a helper command (fdk) that wraps the
things a session on this repo does over and over, the correct way, so nobody has to rediscover the
right shell incantation from scratch each time.

No Firedrake code changes — only AGENTS.md and three small tools under .agents/.

What's in it

AGENTS.md, rewritten as a short reference. It states each rule as an instruction — what to
do, not why — and saves the reasoning for a handful of worked "wrong way / right way" examples at
the end. Those examples are the part that demonstrably gets followed; everything else is trimmed
to fit that.
.agents/tools/check-prose.py. Checks the lines an edit just added (or a whole branch, with
--range) against the handful of AGENTS.md rules a program can actually judge: numpydoc-only
docstrings, short sentences, no leftover mention of code that got deleted, and no hasattr
standing in for a plain setup flag. It can run itself automatically after every edit inside
Claude Code, or by hand — same checks either way.
.agents/tools/fdk. One command for the things a session on this repo needs repeatedly: run
the tests at the right process count, lint the way CI does, find which test file already covers
a piece of code, pull up one function out of a five-thousand-line file, see which branch each
component package (PETSc, UFL, ...) is on, and more. Run .agents/tools/fdk help for the full
list.
.agents/skills/auditing-comments/. A companion habit for reviewing the comments an edit
adds. Read what the comment claims, then check whether the code still says it: if it does, the
comment is redundant and can go; if it does not, the code needs the fix, not the comment.

Why it's shaped this way

Three fixes, each answering a gap the last one left. Prose rules in AGENTS.md alone were not
reliable — a rule only stated in a paragraph is easy for an agent to miss. So the part of those
rules a machine can judge moved into check-prose.py, which catches a violation the moment it's
written, not at review. A checker only helps if someone remembers to run it, so its commands — and
the rest of the repeated work — moved into fdk, one stable command instead of a different shell
line each time.

Try it

.agents/tools/fdk help                              # every subcommand, with what each is for
.agents/tools/fdk test 1 tests/firedrake/regression  # tests at nprocs=1, one summary
.agents/tools/fdk prose --range main...HEAD          # the prose rules over this whole branch

Notes for review

Why these live in the repo, not a machine-local config. In the repo they travel with the
branch and work for anyone pointing an agent at Firedrake — a person included, since both tools
are plain command-line programs that need only bash/Python and git.

Why worked examples, not more prose, for the anti-patterns. They're the part of AGENTS.md
that demonstrably gets followed. A rule stated only as prose has been violated by an agent while
sitting in the file at the time — which is what prompted writing a checker for the parts of it a
program can enforce, rather than trusting prose alone.

Follow ASD-STE100: short sentences, one idea each, active voice, and the subject named up front rather than buried in a relative clause. The AGENTS.md rules this follows are in #5338. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Add the ASD-STE100 rule for docstrings and comments, with an anti-pattern for the clause-stacked phrasing that unedited AI prose tends to produce. Add an anti-pattern for documenting code that is not there. The existing "Document The Present, Not The Past" rule was prose only, and it stated the obvious case. The subtle case is an argument against a branch that was removed, which reads as present-tense but sends the reader looking for absent code. Give the rule a test that a reader can apply, and list the words that give it away. Exempt tests from the full numpydoc Parameters/Returns structure. A sentence on what is checked and why is enough there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Three sentences, one idea each, in place of one of 27 words. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Four of the rules above are ones a machine can judge: the Sphinx field-list docstring style, sentences too long for ASD-STE100, wording that describes code which is not there any more, and hasattr standing in for a setup flag. Check them at the moment of writing, when they are free to fix, rather than at review. The script reports only on the lines an edit added, found by diffing against git. It is advisory and always exits 0. Opt in per checkout through .claude/settings.local.json, since it runs on every edit. The checks are a floor. Clause-stacking needs judgement, and so does an argument against a branch that is no longer there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Move the usage out of AGENTS.md and into the tool. The rules belong in AGENTS.md; how to run a script does not. Give the script a command line, so it works without a hook payload: .claude/hooks/check-prose.py firedrake/mg/utils.py It exits 1 when it finds something, so a pre-commit script can use it, and --help prints the whole docstring. Running it this way turned up two faults: git resolved a relative path against -C and so read every tracked file as new, and the summary raced the findings through a separate stream. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

A session runs the same handful of commands over and over: tests at a given process count, flake8, the prose check, a Cython rebuild, a cache clean. Each one has a detail that is easy to get wrong by hand. The interpreter must come from the virtual environment, because a bare python is the system one. A parallel test run needs an explicit mpiexec, because a bare pytest self-forks one mpiexec per test and reports failures that a correct run does not show. A run at one process must not use mpiexec -n 1, which can hang at MPI_Finalize. Collect these in .claude/tools/fdk, and point AGENTS.md at it. The helper takes no configuration: it finds the source tree from its own location, and the environment from FIREDRAKE_VENV, from an activated environment, or from the directory the source tree sits in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

check-prose.py grows a --range flag, which reads the lines a commit range added rather than the lines the working tree changed. The hook reports an edit as it happens, so it says nothing about the commits a branch already has, which is what review sees. Sweeping every file of a branch reaches check-prose.py itself, which quotes the words it bans. Exempt it, as AGENTS.md and CLAUDE.md already are. fdk lint now calls make srclint, which is what CI runs. A plain flake8 skips the script directories, whose files have no .py suffix.

A branch that predates .claude/ has no copy of these tools, and switching to one removes them from the working tree. A copy kept elsewhere survives that, but it can not find the source tree from its own location. FIREDRAKE_SRC tells it, and the hook is taken from the checkout, so it is always the one versioned with the branch under test.

A hook and a tool differ in who invokes them, but check-prose.py is both: it reads a payload on stdin, or a command line, and runs the same checks either way. Neither directory name is load-bearing, because the settings name the hook by its full path, so the split only mislabelled one of the two files. Say in AGENTS.md that both are ordinary command-line programs, which any agent can run. Driving check-prose.py from a PostToolUse hook is one way to reach it, not the only one.

baseline runs the tests at the merge base and again at HEAD, and prints the difference. A failure that only the second run shows is the only kind the branch caused; anything else was already there. deps reports the component packages and where each one is installed. A bug can live in any of them, and an API has to be read from the installed source. show prints one function or class by name, because the files are long and their line numbers move. stack prints the pull request stack. pr edits a title or a body through the REST API, which gh pr edit can not do against a repository that still carries a classic project. Also set OMP_NUM_THREADS, which firedrake asks for on every import.

A comment that explains what the code does is a bug report against the code. Firedrake PR #5215 shows the cost of answering one by rewriting the comment: the second attempt was longer, more specific and still false, and the reviewer replied that it made him wonder why the code did what it did. An inaccurate comment does not only confuse, it puts the code under suspicion. fdk explain prints each added comment beside the code it annotates, with the comment stripped out, so the question is unavoidable: does the code still say this? The skill supplies the judgement the script can not, because no checker can tell that a sentence is false. Move the tools to .agents/, which is not particular to one assistant, and symlink .claude/ to them. PETSc main does this, and AGENTS.md is the vendor-neutral standard the tools already document themselves in.

The example quoted a named reviewer's words and tied them to one pull request. A permanent instruction file is the wrong place for that, however public the thread. It also stopped a step short of the real ending. The code that the false comment defended was not kept and renamed, it was deleted, because nothing needed it. That is the sharper lesson, and the version here missed it: when a comment cannot be written truthfully, try removing the code before rewriting it. Outcome 3 now says so, and carries a made-up example that reaches the same end.

The tools moved to .agents/, and .claude/ is a symlink to it. Name the directory that holds them, so that a reader who has no .claude/ still finds them.

The relevant subset of the suite is the tests that reach the changed lines, at the process counts where those lines are live. A serial run does not reach the code that only MPI reaches. Give the firedrake-run-split-tests invocation that CI uses, say where it writes its logs, and say when to reach for it rather than for fdk test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

A fenced code block and the YAML frontmatter are not prose, and the checks read them as if they were. A skill document that shows a badly written comment as an example was reported for the example, and a frontmatter description was reported as one long sentence. Take those lines out of the set an edit added, so that every check drops them, not the sentence length alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The prose checker bans "no longer", because it usually introduces code that a reader cannot see. Here it only weakened a sentence about a question. Say what the question is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

A heading like `**Do this.**` ends its sentence before the closing markers, so the split found no space after the stop and joined the sentence to the one after it. The pair was then reported as one long sentence. Step over the closing markers to find the space. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

fdk and check-prose.py now enforce several of these rules deterministically, so the prose explaining them is redundant. State each rule as an instruction, keep the verbose reasoning only in Anti-Patterns, and cut the toolchain section down to a command reference for fdk help. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…e docstring fdk test/testraw silently selected nothing when a path's tests were not marked for the given nprocs -- no summary line from `test`, a bare mpiexec abort from `testraw`. Both now check for a passed/failed/error/skipped count and say so on stderr when there isn't one. fdk testfile <path> finds the test file(s) that cover a source file by matching its top-level names against tests/**/test_*.py, since Firedrake's test layout does not mirror its source layout. check-prose.py's sphinx-field-list check now covers a whole docstring once any line of it is touched, not just the lines an edit added, so a partial edit can no longer leave old-style lines behind uncaught. AGENTS.md: point at fdk testfile and the nprocs warning, tell readers to check a target's actual @pytest.mark.parallel markers before picking <nprocs>, and clarify that Type Hints applies to what you touch, not a retrofit of an untouched signature -- the codebase is mid-migration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

"this used to divide by N" describes removed code, but "the function used to define F" just names the function's purpose -- a reduced relative clause hanging off a noun, not the sentence's main verb. Only require the past-tense reading when a pronoun subject sits right before "used to", the pattern the real removed-code case actually has. Found reviewing a Sonnet-generated docstring that used the phrase correctly and got flagged anyway. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

The AGENTS.md rewrite says to use fdk over a bare pytest/flake8/python and to find a test's home with fdk testfile, but a session that skips reading AGENTS.md never sees any of that -- two runs of the same cold exercise confirmed it. Enforce it with a PreToolUse hook instead: - require-fdk.py denies a bare python/pytest/flake8 Bash call outright, and denies (or asks, when nothing already covers it) writing a new tests/**/test_*.py file, deferring to `fdk testfile --from-content` for that judgment rather than duplicating it. - CLAUDE.md imports AGENTS.md, so every session -- including a subagent's -- has it in context before choosing a first command, instead of needing to go find and open it. - .claude/settings.json wires the hook on by default for this checkout, on Bash and Write. - fdk testfile gains --from-content, the source `require-fdk.py` calls for the new-test-file check; AGENTS.md and fdk's own help text now say fdk is required, not preferred. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

TSFC keys a cached kernel on the form signature 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. The next run then imports the edited Python and executes the C that the previous generator wrote, which reads as a wrong number or a diverging solver rather than as a stale kernel, and sends the reader to the numerics. Fingerprint those trees before each run and clear the caches when the fingerprint has moved. Content rather than timestamps, so that checking a commit out again, as a bisect does, matches the caches it already built. Nothing to remember, and about 60ms when nothing has changed. Pin the cache directories too. Firedrake defaults them to $VIRTUAL_ENV/.cache, and to $HOME/.cache when no environment is activated, so an activated shell and a bare one compiled into different caches and `fdk clean` cleared only one of them. Let `fdk baseline` stash instead of refusing a dirty tree. Uncommitted work is the normal state when a failure needs attributing, and refusing it is what pushes a session into reading code instead of running the one command that says whose failure it is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

The caching note told the reader to run `fdk clean` when a code-generation change "does not seem to take effect". That describes the easy case. The hard one is that the run imports the edited Python and executes the C the previous generator wrote, so the result is a plausible number or a diverging solver, and the reader goes to the numerics. Say that, and say that fdk now clears the caches itself. Say to attribute a failure before analysing it, too. `fdk baseline` was in the command list with nothing telling anyone when to reach for it, and the answer is: first, before reading code, and before repeating anyone's claim that a failure is new. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

dof-loop matches a `for` over `.dat.data`, `num_cells()` or `node_count`. Files under firedrake/cython/ and under tests/ are exempt: the first exists to loop over mesh entities, and does it compiled and typed; the second asserts over meshes small enough for the loop to cost nothing. Without the tests/ exemption the check reports 15 legitimate assertion loops. With it, it reports the two in firedrake/adjoint_utils/function.py, which are real -- see #5342. google-section matches Args:/Returns:/Raises:, which numpydoc writes with an underline rather than a colon. It reports over a whole touched docstring, as sphinx-field-list does, and matches 5 lines in adjoint_utils/. Neither check adds a finding to main...HEAD. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

dham · 2026-08-11T16:09:33Z

I don't believe that AI scaffolding like this is. the right approach. It's a huge technical debt for unproven and doubtful benefit.

Attempting to make our intrinsic workflow more LLM robust (e.g. by clearing caches from pytest) would be a better approach.

pbrubeck mentioned this pull request Aug 8, 2026

Add the prose rules to AGENTS.md firedrakeproject/fiat#278

Open

pbrubeck mentioned this pull request Aug 8, 2026

Multigrid: skip re-evaluation of unrefined nodes #5288

Open

pbrubeck changed the title ~~Sharpen the prose rules in AGENTS.md~~ Agent tooling: sharper prose rules, a prose checker, and an fdk helper Aug 8, 2026

pbrubeck and others added 19 commits August 9, 2026 17:35

AGENTS.md: shorten an over-long sentence

d721471

Three sentences, one idea each, in place of one of 27 words. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Point the documentation at the tools' real directory

c8e5f8f

The tools moved to .agents/, and .claude/ is a symlink to it. Name the directory that holds them, so that a reader who has no .claude/ still finds them.

Say what the question is, not what it stopped being

c578dff

The prose checker bans "no longer", because it usually introduces code that a reader cannot see. Here it only weakened a sentence about a question. Say what the question is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

pbrubeck force-pushed the pbrubeck/agents-md-prose-rules branch from 7d76cad to 44ee385 Compare August 9, 2026 16:36

pbrubeck changed the base branch from main to release August 9, 2026 16:43

pbrubeck and others added 4 commits August 9, 2026 17:44

pbrubeck requested a review from miguelcoolchips August 11, 2026 09:06

dham closed this Aug 11, 2026

Navigation Menu

Uh oh!

Agent tooling: sharper prose rules, a prose checker, and an `fdk` helper - #5338

Agent tooling: sharper prose rules, a prose checker, and an `fdk` helper#5338
pbrubeck wants to merge 23 commits into
releasefrom
pbrubeck/agents-md-prose-rules

pbrubeck commented Aug 8, 2026 •

edited

Loading

Uh oh!

dham commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

pbrubeck commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Goal

What's in it

Why it's shaped this way

Try it

Notes for review

Uh oh!

dham commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pbrubeck commented Aug 8, 2026 •

edited

Loading