From 739814ca0ce3a77b24aaffee1dcfc87175572284 Mon Sep 17 00:00:00 2001 From: Xylar Asay-Davis Date: Wed, 9 Sep 2026 15:11:50 -0500 Subject: [PATCH] Add agent instructions and writing rules Adds AGENTS.md with repository-wide instructions for AI agents, a CLAUDE.md that points at it, and per-artifact writing skills under .claude/skills/. The writing rules exist because agent-written GitHub prose has been running an order of magnitude longer than what colleagues write. Each skill carries the rules for one artifact, calibration measured from Compass's own 2023 and 2024 history, and worked examples taken from real pull requests where they exist. Adapted from the equivalent files in Polaris. The environment, contract and human-writing sections carry over; the style, documentation and supported-machine sections follow Compass's own tooling (flake8 and isort, reStructuredText docs, the machines table in the Developer's Guide). All calibration numbers and examples were re-measured against MPAS-Dev/compass. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/skills/design-documents/SKILL.md | 53 ++++++++ .claude/skills/plan-documents/SKILL.md | 28 +++++ .claude/skills/pr-descriptions/SKILL.md | 72 +++++++++++ .claude/skills/review-comments/SKILL.md | 74 +++++++++++ .claude/skills/testing-comments/SKILL.md | 82 ++++++++++++ AGENTS.md | 151 +++++++++++++++++++++++ CLAUDE.md | 1 + 7 files changed, 461 insertions(+) create mode 100644 .claude/skills/design-documents/SKILL.md create mode 100644 .claude/skills/plan-documents/SKILL.md create mode 100644 .claude/skills/pr-descriptions/SKILL.md create mode 100644 .claude/skills/review-comments/SKILL.md create mode 100644 .claude/skills/testing-comments/SKILL.md create mode 100644 AGENTS.md create mode 100644 CLAUDE.md diff --git a/.claude/skills/design-documents/SKILL.md b/.claude/skills/design-documents/SKILL.md new file mode 100644 index 0000000000..c58eaf987c --- /dev/null +++ b/.claude/skills/design-documents/SKILL.md @@ -0,0 +1,53 @@ +--- +name: design-documents +description: Write or revise a design document under docs/design_docs/. Use when proposing a new capability, not when fixing a bug. +--- + +# Design documents + +Long is fine. A design document is scanned and returned to, not read +straight through. What must be scannable is the specification. + +- Normative statements come first in a section and stand alone. Rationale + goes in a marked block below, which a reader can skip. +- Rejected alternatives and superseded drafts go in one `Decisions` + section, cited from the places they affect. Never re-argued in place. +- A principle is stated once. Later sections cite it by name. +- Do not pre-empt objections. Drop "worth noting", "not an accident", + "deliberately", "this is not a stylistic preference". State the decision + and let it stand. +- Open questions go at the top or in their own section, never + mid-paragraph. + +## Calibration + +The two design documents in `docs/design_docs/` run 2,574 and 12,390 words, +both at 28 words per sentence, with no hedging phrases in the first and one +in the second. Sentence length is the thing to fix; aim for twenty words. +Do not import the hedging habit that these documents do not have. + +`docs/design_docs/template.rst` is the prescribed structure: a +`Requirement`, `Algorithm Design`, `Implementation` and `Testing` section +per topic, with the same topic name used in each. It also says requirements +"should not discuss technical software issues, but rather focus on model +capability", which is the rule most often broken. + +## Enough + +From `docs/design_docs/cached_outputs.rst`. The heading states the +requirement and the body is one or two normative sentences. + +> Requirement: updating cached outputs +> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> +> There should be a documented process for creating cached outputs for steps +> and uploading them. +> +> Requirement: either "normal" or "cached" versions of a step +> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +> +> We **do not** require the ability to set up a "normal" and a "cached" +> version of the same step within a ``compass`` test case or suite. + +Five requirements in that document take about 250 words between them, and +saying which capability is *not* required is a requirement worth writing. diff --git a/.claude/skills/plan-documents/SKILL.md b/.claude/skills/plan-documents/SKILL.md new file mode 100644 index 0000000000..0e4184576b --- /dev/null +++ b/.claude/skills/plan-documents/SKILL.md @@ -0,0 +1,28 @@ +--- +name: plan-documents +description: Write a plan for work not yet started, for a colleague or the user to approve before implementation begins. +--- + +# Plan documents + +The reader is deciding whether to let you proceed. + +- Open questions and anything needing a decision go at the top. +- The steps, in order, one line each. +- Do not justify each step. Do not list the files you will touch. Do not + restate the codebase back. +- If a step needs a paragraph to explain, it belongs in a design document. + +## Enough + +Plans are approved in conversation rather than committed, so there is no +human example in this repository to copy. The following is constructed. + +> **Open:** should the `anvil` rows come out of the machines table now, or +> wait until the config file is restored? +> +> 1. Fix the land-ice draft used for pressure in the thin-film cases. +> 2. Restore the culled thin-film region in `initial_state`. +> 3. Take the smoothing from ice thickness, not draft, in both cases. +> 4. Update the `isomip_plus` docs in both guides. +> 5. Rerun the `pr` suite on Chrysalis against a `main` baseline. diff --git a/.claude/skills/pr-descriptions/SKILL.md b/.claude/skills/pr-descriptions/SKILL.md new file mode 100644 index 0000000000..4d9329c838 --- /dev/null +++ b/.claude/skills/pr-descriptions/SKILL.md @@ -0,0 +1,72 @@ +--- +name: pr-descriptions +description: Write or update a pull request description, including drafting pr_description.md before opening a PR. Use when opening a pull request or editing its body. +--- + +# Pull request descriptions + +The reader is deciding whether to review. + +- What changed and why, in a few sentences. Not how. +- Anything needing a reviewer decision goes in its own short list near the + top, never mid-paragraph. +- A list of changed behaviours is fine. A trace of the mechanism is not. +- No commit list. No testing; that goes in a separate `Testing` comment. +- Link the issue or upstream pull request that gives context. +- Several fixes usually means several pull requests. + +## Calibration + +Measured over merged pull requests from 2023 and 2024, before any agent +wrote here, excluding bots. Descriptions run 32 to 36 words at the median, +57 to 60 at the seventy-fifth percentile, 94 to 106 at the ninetieth, and +230 at the longest. Roughly one description in ten is empty; a title that +says it all is an acceptable description. + +## Enough + +A bug fix, stated and done, from #609: + +> The code for changing permissions was being given an incorrect directory +> and then was failing silently. This merge fixes the path and also removes +> the silent failure (an error will be raised if the status of the download +> directory cannot be determined). +> +> This merge also adds the database root and the root for each affected core +> to the list of directories to chmod/chown anytime any files get downloaded. +> +> closes #586 +> closes #608 + +A port, in one sentence, from #519: + +> This PR moves the generation of an initial state for the ocean global +> cosine bell test case from init mode to the initial state compass step. + +A change with a caveat the reviewer needs, from #716: + +> This PR replaces the `mode_init` MPAS-Ocean run in the `initial_state` +> step with local computations. The initial state is unchanged for the +> configuration that is currently used in the drying_slope cases. I did not +> retain all of the configuration options that were previously present in +> `mode_init`, e.g., the idealized transect. +> +> I added some buffer space in the y-dimension (the new config options `Ly` +> as differentiated from `Ly_analysis`) because the 1km case did not include +> the full 25km stretch of wetting and drying. + +## Too much + +Constructed, in the style of a real agent-written description elsewhere. +Five fixes each got a section, and each section traced its mechanism: + +> `compass.ocean.tests.global_ocean` gained a module-scope import of +> `compass.ocean.mesh.remap_topography`, which runs +> `compass/ocean/mesh/__init__.py`, which imports the step classes, which +> import `compass.ocean.tests.global_ocean` back. The module could no longer +> be imported on its own; it worked only when something else imported +> `compass.ocean.mesh` first. + +Someone deciding whether to review does not need the cycle traced. One +sentence would do; the rest belongs in the commit message. Five fixes is +also five pull requests. diff --git a/.claude/skills/review-comments/SKILL.md b/.claude/skills/review-comments/SKILL.md new file mode 100644 index 0000000000..6ae2423c16 --- /dev/null +++ b/.claude/skills/review-comments/SKILL.md @@ -0,0 +1,74 @@ +--- +name: review-comments +description: Write a review comment, review findings, or a reply to review feedback on a GitHub pull request. Use when reviewing code, reporting what testing someone else's branch turned up, or answering a reviewer's question. +--- + +# Review comments + +The reader is deciding what to change. + +- Put each finding as an inline comment on the line it concerns, one point + each. That is where colleagues put them, and it is why their review + bodies are short. +- The review body summarizes: what you ran, and the verdict. Two or three + sentences. +- Use a list in the body only for requests that span files. +- No section on what already works. One line for all of it, if any. +- Say what you could not check. + +## Calibration + +Measured over review comments from 2023 and 2024, before any agent wrote +here. Review bodies run 21 to 23 words at the median, 80 to 92 at the +ninetieth percentile, and 383 at the longest. Inline comments run 14 words +at the median, 58 at the ninetieth percentile, and 517 at the longest. + +## Enough + +Inline, one point and a suggestion, from #547 and #738: + +> I'd be inclined to call this test group `BaroclinicGyre` rather than +> `MitgcmBaroclinicGyre`. I think shorter is better, and I doubt we'll be +> implementing multiple baroclinic gyre cases. + +> Ok. Why don't you leave a comment that mentions that they refer to the old +> METIS partitioning so others know why they are there? + +Inline, a question rather than an assertion, from #817: + +> Is the tilde correct here? Won't this overwrite all the good data and +> leave the missing/bad data in place? + +A body that is the verdict and nothing else, from #523: + +> Thanks for doing this, @xylar. I'm approving based on my testing on Cori, +> in which I built spack with Albany and successfully ran the MALI +> `full_integration` test suite. I also had a quick look through the code +> diffs and nothing jumped out at me, although I can't pretend that I +> understand all of it. + +In the body, when several requests span the whole change, from #637: + +> @scalandr, this is excellent! +> +> I have some small changes and then there are a few other things to do: +> * the test group and all its classes and methods need to be added to the +> `ocean/api.rst` in the developer's guide +> * It would be great if you could build the documentation locally (see +> checklist above) to make sure it looks right. +> * Document the testing you did in a comment in the PR + +## Too much + +Constructed, in the style of a real agent-written review elsewhere, which +spent its first 444 words on "How this was reviewed", "What the previous +review asked for" and four paragraphs of "What works", then traced each +finding's mechanism: + +> The bin-centre array is static: computed once in the step's `setup()` from +> `nBins`, `minLat` and `maxLat`, and never updated. But it is added to the +> output stream like any other field, so every output file in every run +> carries a copy of the same 61 numbers. + +The finding is that a static array is written to every output file. Say +that inline, on the line that adds it, and stop. diff --git a/.claude/skills/testing-comments/SKILL.md b/.claude/skills/testing-comments/SKILL.md new file mode 100644 index 0000000000..4af60476ea --- /dev/null +++ b/.claude/skills/testing-comments/SKILL.md @@ -0,0 +1,82 @@ +--- +name: testing-comments +description: Write a Testing comment on a pull request, recording what was run and what the results were. Use after running suites, test cases or linting for a PR. +--- + +# Testing comments + +What you ran, where, and whether it passed. + +- Name the suite or test case, the machine and the compiler. One sentence. +- Give the work directory or the baseline you compared against. +- Say the result. Bit-for-bit, passed, or the numbers if they matter. +- Use a table or a list only when there are several runs to compare. +- Do not restate in prose what a table or a pasted result already shows. +- Failures unrelated to the branch go under their own heading at the end. + +## Calibration + +Measured over `Testing` comments from 2023 and 2024, before any agent wrote +here. They run 38 to 40 words at the median and 92 to 152 at the ninetieth +percentile. The shortest useful ones are under 25. + +## Enough + +From #663 and #816: + +> ## Testing +> +> I ran the `pr` suite on Chrysalis using `main` as a baseline and all tests +> passed with BFB results. + +> ## Testing +> +> I ran the `pr` suite with these changes, using the current `master` as a +> baseline. All tests passed. + +With the work directory, from #753: + +> ## Testing +> +> This mesh has been run through `files_for_e3sm` and the output is in: +> ``` +> /lcrc/group/e3sm/ac.xylar/compass_1.2/chrysalis/e3smv3-meshes/sowisc12to30e3r2 +> ``` +> +> I have verified that the land-ice mask includes "land-locked" cells that +> were formerly a problem for sea-ice, based on the fix in #752 + +Saying what you could not check, from #791: + +> ## Testing +> +> I successfully ran the `pr` and `nightly` suites on Chrysalis with Intel +> and Open-MPI. I didn't compare with a baseline because none is available +> for the Icos meshes. + +When the results are worth pasting, paste them and stop, from #771: + +> Tests are passing on Chrysalis (Intel/OpenMPI): +> ``` +> Test Runtimes: +> 06:22 PASS ocean_hurricane_DEQU120at30cr10rr2_mesh_fblts +> 46:58 PASS ocean_hurricane_DEQU120at30cr10rr2_init_fblts +> 06:21 PASS ocean_hurricane_DEQU120at30cr10rr2_sandy_fblts +> Total runtime 59:43 +> PASS: All passed successfully! +> ``` +> and the documentation looks good. + +## Too much + +Constructed, in the style of a real agent-written comment elsewhere, which +pasted the results table and then said the same thing again in prose: + +> Every test case now runs to completion. The five diffs are all of the form +> `File ... does not exist`: `main` crashed before writing those outputs, so +> there is nothing to compare against. Every comparison that had a file on +> both sides passed. A clean like-for-like comparison for those five cases +> needs a fresh baseline once this lands. + +The table already showed the passes and the five missing files. The only +new sentence is the last one. diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000000..46580bb08e --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,151 @@ +# COMPASS Agent Instructions + +These instructions apply to the whole repository unless a deeper +`AGENTS.md` overrides them. + +## Source of truth + +- Follow the repo's automated style and lint configuration in + `pyproject.toml`, `.flake8.cfg` and `.pre-commit-config.yaml`. +- If an instruction here conflicts with automated tooling, follow the + automated tooling. + +## Environment + +- If `pixi-env/` exists, it is the preferred development environment for + Python, linting, and `pre-commit`. It is created by `./deploy.py`. +- AI agents should not run `./deploy.py` to create `pixi-env/` + themselves. Creating or refreshing `pixi-env/` is a developer action. +- Prefer running tools from `pixi-env/.pixi/envs/default/bin/` (for + example `python`, `pre-commit`, `flake8`, `isort`, and `mypy`) instead + of relying on the system environment. +- Only fall back to other Python environments if `pixi-env/` does not + exist or is clearly incomplete. + +## Python style + +- Keep Python lines at 79 characters or fewer whenever possible. +- Follow `flake8` and `isort` as configured. Do not preserve manual + formatting or import ordering that `isort` would rewrite. +- Keep imports at module scope whenever possible. Avoid local imports + unless they are needed to prevent circular imports, defer expensive + dependencies, or avoid optional dependency failures. +- Avoid nested functions whenever possible. Prefer private module-level + helpers instead. +- Put public functions before private helper functions whenever + practical. +- Name private helper functions with a leading underscore when that fits + existing repo conventions. + +## Documentation + +- The documentation is reStructuredText built with Sphinx. Follow + `docs/developers_guide/docs.rst`, which is the authority on structure + and on the label conventions for cores, configurations and test cases. +- Every new core, configuration or test case needs documentation in both + the User's Guide and the Developer's Guide, in the same pull request as + the code. +- Add new or modified classes, methods and functions to the relevant + `api.rst` (`docs/developers_guide/api.rst` or the core's own, such as + `docs/developers_guide/ocean/api.rst`). +- Prefer starting from the nearest existing page for a similar test case + rather than writing a documentation page from scratch. + +## GitHub pull requests and issues + +- Do not hard-wrap. Write each paragraph and each bullet as a single + line, however long. GitHub wraps them for display, and hard breaks + make later edits show up as reflowed paragraphs in the diff. +- Start with a paragraph summarizing what the pull request or issue is + about, then use sections for the detail. +- Keep the description in a file at the root of the worktree for the + branch it describes, and never commit it. It is a draft to paste into + GitHub, not part of the branch's content. +- Follow `.github/pull_request_template.md`: the description goes at + the top, keep only the checklist lines that apply, and use closing + keywords for any issue the pull request fixes. +- Do not list individual commits in a pull request description. The + commits are already on the pull request; describe what the change + accomplishes as a whole instead. +- Do not describe testing in a pull request description. Testing goes + in its own `Testing` comment on the pull request, which is what the + template's checklist asks for. +- An issue should say what happens, what was expected instead, and + enough about the configuration and commands used to reproduce it. + +## Writing for human readers + +These rules apply to anything a colleague reads: GitHub comments, pull +request descriptions, issues, plans, design notes. Not code comments or +commit messages, where a reader who wants the mechanism is already in the +right place. Per-artifact rules and worked examples are in +`.claude/skills//SKILL.md`, as plain markdown. Claude Code loads +the matching one automatically; other agents should read it before writing. + +Write less; do not pack the same content into denser sentences. Keep +headings, tables and links. Colleagues mostly write unstructured prose, and +structure is an improvement on it. The problem is length. + +- **Lead with the answer.** The first two sentences say what you found, + changed, or propose. Setup and reproduction go last. +- **One point per paragraph, and few paragraphs.** Colleagues write one to + three per comment; recent AI-written ones ran to eighteen. That gap is + the complaint. Say each thing once. +- **Do not narrate the mechanism.** The chain of calls, and why the fix is + right, go in the commit message. Here, say what broke and where to look. +- **Cut clauses that qualify rather than inform**, and any sentence whose + only job is to justify the one before it. One clause per sentence where + one will do. +- **Use backticks about half as often as feels natural.** They are for what + a reader would type or grep. Code blocks hold artifacts you did not + write, never authored prose. +- **One document, one decision.** Anything still relevant after this merges + is an issue, not a comment. + +Sign anything posted to GitHub on someone's behalf: + +``` +--- + +*Posted by on @'s behalf. The testing, analysis and wording +above are AI-authored; please check them accordingly.* +``` + +Name the agent, not the vendor: `Claude Code`, `Codex`, and so on. + +## Supported machines + +- The table under `Supported Machines` in + `docs/developers_guide/machines/index.rst` is the source of the + supported machine list in the Developer's Guide. Update it whenever + machines are added or removed, or compilers and MPI libraries are + added, removed, or renamed. +- Keep it consistent with the machine config files in + `compass/machines/`: the `mpi_` options under `[deploy]` + define the valid compiler and MPI combinations, and the + `__target` options under `[build]`, where present, + define the MPAS make targets. +- Keep `deploy/albany_supported.txt` consistent with the machines and + compilers that actually support Albany, since MALI functionality + depends on it. +- When a compiler is added or renamed, update every place it appears: + the machine config file in `compass/machines/`, the machine pages in + both the User's and Developer's Guides, and any `load_compass_*.sh` + examples in the documentation and tutorials. + +## Contracts + +- Treat `deploy.py` and `deploy/cli_spec.json` as contract files shared + with the `mache` package. +- Do not modify `deploy.py` or `deploy/cli_spec.json` directly in + Compass. +- If a change appears necessary, stop and note that the change must be + made in `mache` first, then synced back into Compass using the normal + upstream update process. + +## Validation + +- Run pre-commit on changed files is required before finishing; if sandboxed + execution fails, request escalation and do not close the task until it has + run or the user declines. +- Prefer fixing lint and formatting issues rather than suppressing them. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000000..eaf20eec4e --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +See [AGENTS.md](AGENTS.md) for all repository instructions.