Action the review of the agent instruction files

docs/src/conf.py

@@ -152,6 +152,7 @@ def _dotv(version):
 # extensions coming with Sphinx (named "sphinx.ext.*") or your custom
 # ones.
 extensions = [
+    "maintainer_notes",
     "matplotlib.sphinxext.mathmpl",
     "matplotlib.sphinxext.plot_directive",
     "page_metadata_validator",

docs/src/sphinxext/maintainer_notes.py

@@ -0,0 +1,49 @@
+# Copyright Iris contributors
+#
+# This file is part of Iris and is released under the BSD license.
+# See LICENSE in the root of the repository for full licensing details.
+"""The ``maintainer-notes`` directive: docstring prose that is never published.
+
+A module docstring is the only prose read on every visit to a module, checked
+in the diff, and impossible to lose track of. That makes it the right home for
+subsystem explanation - but Sphinx publishes it, so prose written for whoever
+maintains the module arrives in front of whoever uses it. This directive is
+the way out: docutils collects its content and this returns nothing, so the
+prose stays in the file and reaches no page.
+
+The content is deliberately never parsed. :meth:`MaintainerNotes.run` returns
+no nodes and never calls ``nested_parse``, so anything inside is free text.
+Broken markup, unbalanced ``>>>`` and stray asterisks are all safe, and the
+doctest builder does not see them either. That is the point of the design, not
+an oversight: an agent writing notes cannot break a docs build with them.
+
+Getting the name wrong fails loudly. ``make html`` runs with ``-W``, so an
+unknown directive stops the build, and the content does not render on the way
+out. A plain reST comment fails the other way: dedent one line and the prose
+is silently published.
+"""
+
+from docutils.parsers.rst import Directive  # type: ignore[import-untyped]
+from sphinx.application import Sphinx
+
+
+class MaintainerNotes(Directive):
+    """Swallow the directive's content, contributing nothing to the page."""
+
+    has_content = True
+    optional_arguments = 1
+    final_argument_whitespace = True
+
+    def run(self):
+        """Return no nodes, leaving the content unparsed and unpublished."""
+        return []
+
+
+def setup(app: Sphinx):
+    """Set up the Sphinx extension.
+
+    This function is expected by Sphinx to register the extension.
+    """
+    app.add_directive("maintainer-notes", MaintainerNotes)
+
+    return {"version": "0.1"}

lib/iris/AGENTS.md

@@ -16,14 +16,13 @@ message here, as well as anything posted to GitHub — read
 
 ## Why This File Exists
 
-Iris is maintained by humans and agents together. Agents read through a
-narrow context window using text search and cannot run the code; humans read
-with intuition but limited patience. Both want explicit names, local
-reasoning and small units. This file records that shared style, plus the few
-places the two diverge and which way to resolve them. Models are trained to
-please diff reviewers rather than to produce code that is cheap to navigate
-later, and drift towards long functions, redundant comments and defensive
-wrapping; correct for that deliberately.
+Iris is maintained by humans and agents together. Agents search text through
+a narrow window and cannot run the code; humans read with intuition but
+limited patience. Both want explicit names, local reasoning and small units.
+This file records that shared style, and the few places the two diverge.
+Models are trained to please diff reviewers rather than to leave code cheap
+to navigate, and drift towards long functions, redundant comments and
+defensive wrapping; correct for that deliberately.
 
 
 ## Fast Rules
@@ -43,10 +42,9 @@ wrapping; correct for that deliberately.
 - Keep behaviour derivable from the function's own body, signature, type
   hints and docstring. Avoid designs that require opening three other files.
 - Prefer composition and explicit delegation over deep inheritance and mixin
-  stacks. Resolving an MRO across modules costs an agent many tool calls and
-  is opaque to a newcomer. Where Iris already uses mixins (e.g.
-  `iris.common.mixin`, metadata managers), follow the existing pattern rather
-  than inventing a parallel one.
+  stacks: resolving an MRO across modules costs many tool calls and is opaque
+  to a newcomer. Where Iris already uses mixins (`iris.common.mixin`,
+  metadata managers), follow that pattern rather than inventing a parallel.
 - Do not add indirection (registry, dispatch dict, plugin hook, base class)
   for a single caller. This governs implementation layers, not API surface;
   a package `__init__.py` that re-exports is not indirection, see below.
@@ -66,15 +64,16 @@ does not exist.
   a greppable literal and is the preferred idiom for optional CF attributes.
 - Give public names enough distinctiveness to search for. Local `data`,
   `cube`, `result` are fine; a public helper called `_process` is not.
-- Declare `__all__` in modules with a public surface. No `import *`.
-- **Re-export freely from a package `__init__.py`.** Surfacing objects at
-  the level users import from is a real convenience, and it keeps the file
-  layout free to change later. `iris.mesh` is the model: explicit
-  `from .components import MeshXY`, gathered into `__all__`. Re-export
-  verbatim — never rename on the way out, never wrap in `try`/conditional
-  imports, never put logic in `__init__.py`. Each object stays defined in
-  one module, so `class MeshXY` still finds it in a single grep.
-  (`iris.common` uses `import *`; that is legacy, not a pattern to copy.)
+- Declare `__all__` in modules with a public surface.
+- **Never `import *`.** (`iris.common` does; legacy, not a pattern to copy.)
+- **Re-export freely from a package `__init__.py`.** It surfaces objects
+  where users import them and leaves the file layout free to change later.
+  `iris.mesh` is the model: explicit `from .components import MeshXY`,
+  gathered into `__all__`. Re-export verbatim — never rename on the way out,
+  and otherwise keep `__init__.py` free of logic — so that each object stays
+  defined in one greppable place. The routine reason to do more is deferring
+  an optional dependency's import, as `iris_grib` and `iris.fileformats.zarr`
+  both do; say in the module docstring which dependency that protects.
 - Make error and warning messages distinctive and mostly static, so a
   traceback maps to exactly one line. Put interpolated values at the end:
   `f"Cannot collapse a coordinate with bounds: {coord.name()}"`.
@@ -109,6 +108,8 @@ Realising data is the caller's decision, never a library function's.
   and `has_lazy_data()` / `has_lazy_points()` answer which you have.
 - Build graphs whole-array with `iris._lazy_data` — `map_complete_blocks`,
   `lazy_elementwise`, `as_lazy_data`. Per-slice graph building is expensive.
+- Indexing does not shrink a graph. Under Dask's multi-level graphs a slice
+  still references every chunk of its source, and is culled only at compute.
 
 
 ## Conform to the Specification, Not to the Sample File
@@ -126,6 +127,11 @@ are evidence that a code path gets exercised, not authority for what it does.
 
 ## Comments and Docstrings
 
+Three channels, three readers. A **docstring** states the contract for
+whoever calls the thing. A **comment** explains why, at the line that needs
+it, for whoever maintains it. A **module docstring** carries what is too big
+for either, in a `maintainer-notes` block that is never published.
+
 A stale comment is worse than no comment — an agent treats it as evidence and
 a human treats it as documentation.
 
@@ -154,8 +160,8 @@ a human treats it as documentation.
   sentence, ask what breaks if it is gone; most rationale survives its own
   deletion. Then ask who each surviving sentence is for — a **user** calling
   the thing, a **maintainer** not breaking it, or a **reviewer** you are
-  answering. Only the first belongs here; the second goes to the module
-  docstring or a test, the third to the pull request.
+  answering. Only the first belongs here; the second goes to
+  `maintainer-notes` or a test, the third to the pull request.
 - Write notes, not an essay. Fragments are correct, articles are optional,
   shorthand is welcome. Say what holds; do not restate the signature, defend
   the design against an alternative nobody proposed, or narrate how you found
@@ -197,39 +203,30 @@ least able to judge how much of it belongs in the file. Assume too much.
   in its docstring, the source keeping one line that names it. This is the
   only home that fails loudly when it stops being true.
 - A surprise no test can pin — a library quirk, an invariant, a coupling —
-  goes in the module docstring's traps.
+  goes in the module docstring's `maintainer-notes`.
 - A surprise caused by an external bug keeps the URL, not a retelling.
 - Write the fact, not your side of the argument about it. The tells that you
   have written a reply are in [`WRITING.md`](../../.github/WRITING.md).
 
 
 ## Module Docstrings Carry the Explanation
 
-Prose explaining a subsystem belongs in the module docstring. Inline comments
-scatter one argument across dozens of sites; a companion Markdown file is
-validated by nothing and rots unseen. Only the docstring is read on every
-visit, published by Sphinx, checked by numpydoc, and reviewed in the diff.
-
-Be generous here, and only here: sixty lines of orientation above a
-two-thousand-line module is cheap, read once per visit rather than once per
-call site. Not a tutorial.
+Prose explaining a subsystem belongs in the module docstring: it is the only
+prose read on every visit and reviewed in the diff. Anything aimed at a
+maintainer rather than a user goes in a `maintainer-notes` block at the end,
+which Sphinx drops, so it never reaches the API page.
 
-Cover whichever apply:
+Be generous here and only here — sixty lines of orientation above a
+two-thousand-line module is cheap, and not a tutorial. Cover whichever apply:
 
 - what the module is for, and what it deliberately does not do;
 - the vocabulary — name the concepts the code assumes you already know;
 - invariants the code depends on but cannot assert;
 - why the design is as it is, including approaches tried and rejected;
 - known traps, and the modules this one is coupled to.
 
-Leave out anything a reader or Sphinx can already derive: API listings,
-signatures, call sequences, per-release history. Explanation scoped to a
-single class belongs in that class's docstring instead.
-
-A public module's docstring is also published as an API page, so it must open
-with a `z_reference` item — a title, `:tags:` carrying at least one `topic_*`,
-and a one-line summary — within its first 25 lines. Copy the form from a
-neighbouring module; `pre-commit run check-docs-page-metadata` checks it.
+Leave out what a reader or Sphinx can derive: API listings, signatures, call
+sequences, per-release history. Class-scoped explanation goes in that class.
 
 **Exemption to "do not document code you did not change":** if you had to work
 out how a subsystem behaves in order to edit it, write that understanding into
@@ -247,9 +244,11 @@ code in a private sibling module instead.
 
 - New modules: aim under ~1000 lines, one coherent concept each.
 - Functions: aim to fit one screen (~50 lines).
-- Use guard clauses and early returns instead of nested conditionals.
-- Give a function one exit where it must still warn, record or close something
-  on the way out. A second exit added later silently skips that work.
+- **One exit.** Nearly every function here has a single `return`; match them.
+  A second exit added later silently skips whatever the first does on the way
+  out — the warning, the record, the close.
+- Flatten a deep conditional with a progress flag carried down a flat
+  sequence of `if`s, rather than with extra returns.
 - Nesting beyond three levels is a signal to extract a named helper.
 
 
@@ -280,10 +279,9 @@ code in a private sibling module instead.
   re-staged and the run repeated until clean.
 - New and changed public functions have type hints and NumPy docstrings.
 - No comment or docstring was left stale by the change.
-- If you debugged during this change, re-read the diff as someone who was not
-  there. Comments added after the first working version are where verbosity
-  collects — and where a comment turns into a reply to a conversation the
-  reader never had.
+- If you debugged here, re-read the diff as someone who was not there.
+  Comments added after the first working version are where verbosity collects,
+  and where a comment becomes a reply to a conversation the reader never had.
 - Laziness behaviour is preserved and documented.
 - Tests updated per [`tests/AGENTS.md`](tests/AGENTS.md); changelog fragment
   added per [`../../changelog/AGENTS.md`](../../changelog/AGENTS.md).

lib/iris/fileformats/_nc_load_rules/actions.py

@@ -4,38 +4,41 @@
 # See LICENSE in the root of the repository for full licensing details.
 """Replacement code for the Pyke rules.
 
-For now, we are still emulating various aspects of how our original Pyke-based
-code used the Pyke 'engine' to hold translation data, both Pyke-specific and
-not :
-
-1) basic details from the iris.fileformats.cf analysis of the file are
-   recorded before translating each output cube, using
-   "engine.assert_case_specific_fact(name, args)".
-
-2) this is also used to store intermediate info passed between rules, which
-   used to be done with a "facts_cf.provides" statement in rule actions.
-
-3) Iris-specific info is (still) stored in additional properties created on
-   the engine object :
-       engine.cf_var, .cube, .cube_parts, .requires, .rules_triggered, .filename
-
-Our "rules" are just action routines.
-The top-level 'run_actions' routine decides which actions to call, based on the
-info recorded when processing each cube output.  It does this in a simple
-explicit way, which doesn't use any clever chaining, "trigger conditions" or
-other rule-type logic.
-
-Each 'action' function can replace several similar 'rules'.
-E.G. 'action_provides_grid_mapping' replaces all 'fc_provides_grid_mapping_<X>'.
-To aid debug, each returns a 'rule_name' string, indicating which original rule
-this particular action call is emulating :  In some cases, this may include a
-textual note that this rule 'failed', aka "did not trigger", which would not be
-recorded in the original implementation.
-
-TODO: remove the use of intermediate "facts" to carry information between
-actions.  This mimics older behaviour, so is still useful while we are still
-comparing behaviour with the old Pyke rules (debugging).  But once that is no
-longer useful, this can be considerably simplified.
+.. maintainer-notes::
+
+   For now, we are still emulating various aspects of how our original
+   Pyke-based code used the Pyke 'engine' to hold translation data, both
+   Pyke-specific and not :
+
+   1) basic details from the iris.fileformats.cf analysis of the file are
+      recorded before translating each output cube, using
+      "engine.assert_case_specific_fact(name, args)".
+
+   2) this is also used to store intermediate info passed between rules, which
+      used to be done with a "facts_cf.provides" statement in rule actions.
+
+   3) Iris-specific info is (still) stored in additional properties created on
+      the engine object :
+          engine.cf_var, .cube, .cube_parts, .requires, .rules_triggered,
+          .filename
+
+   Our "rules" are just action routines. The top-level 'run_actions' routine
+   decides which actions to call, based on the info recorded when processing
+   each cube output.  It does this in a simple explicit way, which doesn't use
+   any clever chaining, "trigger conditions" or other rule-type logic.
+
+   Each 'action' function can replace several similar 'rules'. E.G.
+   'action_provides_grid_mapping' replaces all
+   'fc_provides_grid_mapping_<X>'. To aid debug, each returns a 'rule_name'
+   string, indicating which original rule this particular action call is
+   emulating :  In some cases, this may include a textual note that this rule
+   'failed', aka "did not trigger", which would not be recorded in the
+   original implementation.
+
+   TODO: remove the use of intermediate "facts" to carry information between
+   actions.  This mimics older behaviour, so is still useful while we are
+   still comparing behaviour with the old Pyke rules (debugging).  But once
+   that is no longer useful, this can be considerably simplified.
 
 """
 

lib/iris/fileformats/cf/__init__.py

@@ -12,24 +12,28 @@
 Provides the capability to load netCDF files and interpret them
 according to the 'NetCDF Climate and Forecast (CF) Metadata Conventions'.
 
-This package is a re-export surface and holds no logic of its own. Every
-public name below is defined in exactly one private module and imported here
-verbatim, so ``class CFReader`` still finds its definition in a single grep.
 Import from :mod:`iris.fileformats.cf`, not from the private modules: the file
 layout is deliberately free to change, and it will, as the format-agnostic CF
 machinery grows a dataset abstraction, a loader and a saver.
 
-The re-exported classes keep the ``__module__`` their defining module gives
-them, so ``repr(CFReader)`` names ``iris.fileformats.cf._reader`` rather than
-this package. That is the same trade-off :mod:`iris.mesh` already makes, and
-it is the one that keeps ``inspect.getsource`` and the documentation's
-``[source]`` links working. See the plan, section 8.4.
-
 References
 ----------
     [CF]  NetCDF Climate and Forecast (CF) Metadata conventions.
     [NUG] NetCDF User's Guide, https://docs.unidata.ucar.edu/nug/current/
 
+.. maintainer-notes::
+
+   This package is a re-export surface and holds no logic of its own. Every
+   public name below is defined in exactly one private module and imported
+   here verbatim, so ``class CFReader`` still finds its definition in a single
+   grep.
+
+   The re-exported classes keep the ``__module__`` their defining module gives
+   them, so ``repr(CFReader)`` names ``iris.fileformats.cf._reader`` rather
+   than this package. That is the same trade-off :mod:`iris.mesh` already
+   makes, and it is the one that keeps ``inspect.getsource`` and the
+   documentation's ``[source]`` links working.
+
 """
 
 from ._group import CFGroup