AGENTS.md: more patterns/antipatterns - #5344
Open
pbrubeck wants to merge 14 commits into
Open
Conversation
Adds WRONG/RIGHT pairs for docstring/comment grammar (third-person summary, buried passive, nominalised verbs, stacked clauses, long sentences, and unnecessary "why"), expands "Document The Present, Not The Past" into a full anti-pattern, and replaces three easy-to-forget command-line habits (unactivated venv, single-file lint, memorized API shapes) with pairs instead of a wrapper script. Points Pull Request Expectations at the existing pre-submission checklist in docs/source/contribute.rst. Adds CLAUDE.md pointing at AGENTS.md.
Six separate WRONG/RIGHT pairs made the rule look like six rules. Fold them into one docstring that packs in passive voice, a nominalised verb, stacked relative clauses, a 50-word sentence, and an unneeded why, then one fix for all of it — ASD-STE100 named as the standard the pattern is checked against.
Rewrite the WRONG docstring around the failure mode that's actually common: dropped relative pronouns and stacked passive participles with ambiguous attachment, not explicit stacked which/that clauses. Drop "active voice" from the RIGHT callout — SS05 already catches the one piece of that this pattern would otherwise claim as judgement.
"Gives the ... rule its mechanism" talked about the document instead of the mistake. State the actual risk instead.
Cut narrative justification across every section -- the mechanism behind a rule, the history of why it exists, the consequence of ignoring it -- keeping the fact and the command. Cross-reference Core Working Rules and Coding Style bullets to their Anti-Patterns instead of restating the rule twice.
"the code a branch changes" reads as two clauses until the reader backtracks. Recast as a participial phrase instead of restoring "that" -- this pattern shows up in PR/comment prose more than the docstring example above does.
Reverts the standalone pair added last commit. Work "the operator the KSP wraps" (dropped relative pronoun, the construction that causes the most real confusion) into the single WRONG docstring instead of listing it separately.
Eight instances in the file's own prose read as two clauses until the reader backtracks -- "the base class every preconditioner inherits," "the closure map code generation depends on," "not evidence the parallel tests passed," and so on. Restore "that"/"where" or recast as a participle in each; also fixes a missing "there are" found along the way.
The old WRONG/RIGHT pair packed in nominalization, passive participles, a dropped relative pronoun, and sentence length all at once, so it taught several rules through one example instead of showing any one of them clearly. Split the two easiest-to-miss mistakes -- a dropped relative pronoun and a noun stacked in place of a verb -- into their own minimal pair, with nothing else wrong in it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Most patterns already carry no check reference; the two that did added a sentence of tool-naming context per pattern for no benefit readers pay for on every read. Read the WRONG/RIGHT pair instead.
Found by reading the whole file for the two rules the new Grammar pattern names, since fdk prose does not check for either: a numpydoc/ long-sentence/clause-stacking checker has no rule for a missing "that" or three nouns in a row. Eight instances, all pre-existing text. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
"Indexed" was a synonym substituted in without checking; "keyed" is
this codebase's own term for it, as the rest of the file already says
("TSFC keys a cached kernel on the form...").
"On" is the codebase's own preposition for this; "by" changed the idiom without needing to when the previous fix swapped "indexed" for "keyed".
pbrubeck
force-pushed
the
pbrubeck/agents-md-grammar-patterns
branch
from
August 12, 2026 22:09
fc933a1 to
225e807
Compare
pbrubeck
force-pushed
the
pbrubeck/agents-md-grammar-patterns
branch
from
August 12, 2026 22:18
225e807 to
f531e01
Compare
pbrubeck
requested review from
JHopeCollins and
connorjward
and removed request for
JHopeCollins
August 14, 2026 12:50
connorjward
reviewed
Aug 20, 2026
| Firedrake's full contribution process is documented at | ||
| [Contributing to Firedrake](https://firedrakeproject.org/contribute.html). In short, for AI-assisted | ||
| contributions: declare that AI was used and which tool; a human must lead the PR, understand every | ||
| change, and answer reviewer questions themselves rather than relaying them to the AI; the code must |
Contributor
There was a problem hiding this comment.
Is this a change in policy?
Contributor
There was a problem hiding this comment.
Though it seems weird to be telling the AI what the human should be doing. The human should be policing the AI not the other way around.
Contributor
Author
There was a problem hiding this comment.
There's no change. Why do you ask?
Contributor
There was a problem hiding this comment.
understand every change, and answer reviewer questions themselves rather than relaying them to the AI;
vs
understands every change, and answers reviewer questions directly;
But in general I don't know if this text should be here. The human should know these things, not the AI.
Contributor
Author
There was a problem hiding this comment.
I think this file should not be pointing to other files or be giving too much information that is not going to facilitate workflow in our codebase
Contributor
There was a problem hiding this comment.
So this should go entirely?
Contributor
Author
There was a problem hiding this comment.
Not entirely, I think at least we should tell AI to add a declaration to the PR
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description