Improve agent guidance: skill discovery, module skills, review workflow - #15977

Agents should run an adversarial review pass (e.g. /code-review or a
fresh-context agent) against AGENTS.md's rules and the change's own
logic before requesting human review, so scarce reviewer time goes to
judgment calls rather than mechanical rule violations. Supplements,
does not replace, the human review gate.

Also fixes the pre-existing duplicate "6." numbering in this list.

PR chains (e.g. the Neo4j GormRegistry migration: apache#15779-apache#15817,
consolidated into apache#15972) were used to keep diffs reviewable, but
GitHub's Copilot reviewer silently skips PRs over ~300 files -
apache#15972's consolidated diff got no automated review, and the
integration between its sub-PRs was never itself reviewed.

Since adversarial self-review is now mandatory on every PR regardless
of size, splitting for reviewability no longer buys coverage, only
coordination overhead. Default to one PR per feature/migration; split
only for independently revertable/mergeable units.

The task->skill bullet list and the skills table both had to be kept
in sync with .agents/skills/ by hand, and had already drifted twice:
a reference to codebase-memory/SKILL.md that never existed, and
mono-repo-integration/SKILL.md existing on disk but referenced
nowhere in this file.

Point agents at the directory instead: enumerate .agents/skills/*/
and match on each SKILL.md's own front-matter description. Adding,
renaming, or removing a skill no longer requires touching AGENTS.md,
and this works for any agent that reads front-matter, not just ones
that follow a Claude-specific list.

.claude/worktrees/ accumulates agent-created worktrees with no
documented cleanup process - found 3 stale ones from a 2026-06-25
workflow run still present today, plus a since-removed one for a
branch whose PR had long since merged.

Age/idle time is the wrong staleness signal here: this repo's PR
chains go quiet between review rounds without being abandoned, so a
worktree is valid as long as its branch has a live remote or open PR.
The skill classifies each worktree under .claude/worktrees/ by GitHub
PR state and merge-into-default-branch status instead, checks for
uncommitted work before recommending anything, and never touches
worktrees outside .claude/worktrees/ or removes anything without
explicit confirmation.

No AGENTS.md edit needed to make this discoverable - it surfaces
through the directory-based skill discovery added in the previous
commit.

grails-forge is a Micronaut application (project generator behind
start.grails.org), not a Grails app, but nothing in .agents/skills/
covered its actual idioms - DI, HTTP controllers, MicronautTest+Spock,
Picocli CLI, Rocker templating, or the Feature extension-point system
(82 files use @singleton, 32 @introspected, 20 @Inject, 9 @controller,
8 @MicronautTest; zero skill coverage for any of it before this).

Root AGENTS.md is also actively wrong for this subproject in at least
one place: its "@GrailsCompileStatic, not @CompileStatic" rule is
inverted here (0 @GrailsCompileStatic usages, 8 correct @CompileStatic
ones), since grails-forge has no Grails artefacts. This skill
documents where root rules do and don't transfer, verified against
actual source rather than assumed.

Grounds the earlier context-management finding: an agent working in
grails-forge paid the full cost of root AGENTS.md's GORM/Hibernate/
artefact content with no corresponding Micronaut guidance to fill the
actual gap. This is the prerequisite for a later scoped
grails-forge/AGENTS.md.

Root AGENTS.md (2073 words) was loaded in full for every grails-forge
task despite being ~0% applicable: grails-forge is a Micronaut
application (the generator behind start.grails.org), not a Grails
app, and has its own settings.gradle/build. At least one root rule is
actively wrong there - "@GrailsCompileStatic, not @CompileStatic" is
inverted, since grails-forge correctly uses plain @CompileStatic (8
files, 0 @GrailsCompileStatic).

Adds grails-forge/AGENTS.md (+ CLAUDE.md symlink, matching root's
agent-neutral pattern) scoped to what's actually true here: corrected
critical rules, this subproject's own build/test/style commands
(verified against its CI job, .github/workflows/codestyle.yml's
check_forge_projects), its module table, and a pointer to the
micronaut-developer skill for code patterns. PR/branch/review/security
policy is explicitly not repeated, to avoid drift from the root copy.

Root AGENTS.md's Project Structure table now points to the nested
file so an agent starting at repo root knows it exists.

The root CLAUDE.md -> AGENTS.md symlink exists because some tools
hard-require that exact filename; replicating it in every subproject
with a nested AGENTS.md doesn't scale and shouldn't be necessary if
AGENTS.md is discovered natively in nested directories. About to test
that assumption directly rather than assume it.

An A/B test (agent working in grails-forge with vs without the scoped
guidance, identical prompt) surfaced a real gap: this skill documented
@MicronautTest+Spock for HTTP controllers but had nothing on
ApplicationContextSpec, the different, lighter-weight pattern actually
used to test Feature classes (no server startup, just a shared
ApplicationContext). Both test runs had to reverse-engineer it from
MongoSyncSpec.groovy; the "after" agent's own report even claimed this
was "the pattern the skill describes" - it wasn't, grep confirmed zero
hits for ApplicationContextSpec/CommandOutputFixture/BuildBuilder
before this commit.

Adds the pattern grounded in the real source (ApplicationContextSpec,
CommandOutputFixture, BuildBuilder, MongoSyncSpec) and clarifies the
two testing patterns aren't interchangeable - MicronautTest is for
grails-forge-api controllers, ApplicationContextSpec is for
grails-forge-core Feature classes.

Answers a real question: the repo has Codecov integrated
(codecov.yml, coverage.yml), but nothing said whether that data is
visible without a CI round-trip. Checked the actual task sources
(GrailsJacocoPlugin, GrailsViolationAggregationPlugin in build-logic):

- aggregateJacocoCoverage -> JACOCO_COVERAGE.md (already used by
  violation-fixer, just never connected to "Codecov" by name)
- jacocoAggregateReport -> the exact XML CI uploads to Codecov

Both are ordinary local Gradle tasks, no token or network needed.
What's actually CI-only is Codecov's diff-against-base-branch
comparison, PR comment, and dashboard - and per codecov.yml both
status checks are informational: true, so they don't block merge
today regardless.

Also notes coverage.yml only wires up grails-core and grails-gradle -
grails-forge and build-logic have no Codecov data at all, in CI or
locally.

Diff coverage (coverage of only the lines you actually changed) is
what Codecov computes in CI, but the underlying data - JaCoCo line
hit/miss counters - is already produced locally by an ordinary
:module:test run (GrailsJacocoPlugin, finalizedBy jacocoTestReport).
The only missing piece was cross-referencing it against git diff.

Considered wiring in form-com/diff-coverage-gradle (a real, maintained
plugin for exactly this) but that's a build.gradle change with its
own review burden and a new external dependency - a different risk
category from the rest of this branch. This skill gets the same
signal with zero build changes: run the affected module's tests, then
cross-reference its existing JaCoCo XML against git diff hunks.

The script and procedure are verified against real data in this repo,
not assumed from JaCoCo's schema docs:
- Confirmed module derivation must walk up to the nearest build.gradle,
  not assume module = first path segment (grails-gsp/grails-taglib and
  grails-gsp/core are separate modules nested two levels deep).
- Confirmed some .java files live under src/main/groovy/, not
  src/main/java/ (grails-i18n/.../AvailableLocaleResolver.java).
- Found and documented a real gotcha: a stale JaCoCo report doesn't
  error, it silently cross-references the wrong code against shifted
  line numbers - must regenerate after every edit.
- Verified end-to-end against a real mixed-coverage diff in
  grails-i18n (one covered line, one genuinely uncovered dead branch),
  confirming the script correctly identifies exactly which changed
  line needs a test.

Two problems the previous commit left behind:

1. The Coverage section still claimed diff-coverage "genuinely
   requires CI/Codecov's cloud service (not reproducible locally)" -
   wrong as of the diff-coverage-check skill added in the prior
   commit. Corrected.

2. The skill was only discoverable (an agent might find it via the
   mandatory pre-code skill scan, or if explicitly asked about
   coverage), never required. PR checklist step 5 still just said
   "ensure coverage" with no mechanism to check it. Now points
   directly at the skill.

Same gap as the previous commit, mirrored: test-fixer already existed
for diagnosing test failures, but PR checklist step 2 ("Run tests")
never pointed to it, and neither skill referenced the other despite
being sequential steps in the same workflow (run tests -> fix
failures via test-fixer -> once passing, check coverage via
diff-coverage-check). A failing test run also produces an untrustworthy
JaCoCo report, so the ordering matters, not just the pointer.

hibernate-developer's own description already said "use this when
changing code under grails-data-hibernate7" - but that's prose an
agent has to interpret, not a structural signal, so a task could
plausibly load it (or fail to) based on how well the description
matched the agent's phrasing of its own task.

Adds an optional paths: glob to front-matter (Agent Skills
Specification allows arbitrary metadata) and teaches the directory-
based discovery instruction in AGENTS.md to check it: if the file(s)
being touched match a skill's paths, load it regardless of whether
the task description alone would have matched. This is deliberately
generic, not a hibernate-only fix - it's the same mechanism fix apache#4
already established (directory is the source of truth, not a
hand-maintained list), just extended to path-scope in addition to
semantic description matching.

Applied to all three currently path-scoped skills to prove it
generalizes: hibernate-developer (grails-data-hibernate7/**),
micronaut-developer (grails-forge/**), worktree-hygiene
(.claude/worktrees/**). Repo-wide skills (grails-developer,
groovy-developer, etc.) intentionally have no paths: field.

Same treatment as hibernate-developer: grails-data-mongodb is part of
the root aggregate build (no own settings.gradle), so root AGENTS.md's
rules apply unmodified - it just needed deeper technical guidance
layered on top.

Grounded in real source across all 8 subprojects (core, bson, ext,
boot-plugin, spring-data, grails-plugin, gson-templates, docs):
mapping/codec pipeline (MongoMappingContext, BsonPersistentEntityCodec/
PersistentEntityCodec), confirmed-implemented concepts (embedded docs,
GeoJSON, TTL indexes via collMod reconciliation, DBRef vs embedding,
multi-tenancy), and one real trap verified by absence-of-evidence: the
`shard "name"` mapping keyword is a documented no-op - zero
shardCollection calls anywhere in core/src/main, it silently falls
through to Entity's generic methodMissing catch-all.

Testing pattern confirmed: GrailsDataTckSpec<GrailsDataMongoTckManager>
(111+ specs, spot-checked), real MongoDB via Testcontainers
(mongo:7.0.19, spot-checked), core module's tests deliberately serial
(maxParallelForks=1) - don't casually "fix" that.

Includes paths: grails-data-mongodb/** front-matter, consistent with
the structural discovery mechanism added for hibernate-developer.

Two more independent sub-builds with zero agent guidance, found the
same way grails-forge was: own settings.gradle, own test harness, but
neither fits the grails-forge template cleanly - both are hybrids that
share root's dependencies.gradle/gradle.properties (via a purpose-built
SharedPropertyPlugin that walks up the directory tree, since "Gradle
can't share properties across buildSrc or composite projects") while
diverging on everything else: Groovy 4.0.32 (Gradle's embedded
version) instead of root's 5.0.x, plain @CompileStatic never
@GrailsCompileStatic (verified: 0 real annotation usages in either
module, only javadoc mentions), and Gradle TestKit (GradleRunner)
instead of Spock/GORM-mock testing.

grails-gradle: the actual Plugin<Project> classes an app's build.gradle
applies (org.apache.grails.gradle.grails-app etc.) - catalogued all 12
plugin-ID-to-class mappings, the GradleSpecification TestKit base
class, and the model/common/plugins/tasks/bom module split (spock
excluded transitively via grails-gradle-model, verified).

build-logic: the convention-plugin layer every other build in this
repo (root, grails-forge, grails-gradle) consumes transitively via
includeBuild - implements GrailsJacocoPlugin, GrailsViolationAggregationPlugin,
and 11 others that other skills already cited by name without
documentation. Catalogued all 13 plugins with what each actually
configures, both testing patterns (TestKit for task-graph behavior,
ProjectBuilder for pure logic), and flagged that only 4 of 13 plugins
have dedicated specs. No CI job of its own is expected here, not a
dormancy signal like grails-data-neo4j - confirmed live via a
substantive commit (GROOVY-12146 workaround) the day before this was
written, not just "exercised transitively" hand-waving.

Root AGENTS.md's Project Structure table now points to both nested
files, same pattern as grails-forge.

~50 real functional test apps live under grails-test-examples/<name>/
(hibernate7, mongodb, micronaut, spring-security, geb, etc.) - each a
real, standalone Grails app exercising a specific framework feature -
and nothing pointed grails-developer (the app-development skill) at
them. Real risk: several share a name with a framework-internals
module (grails-test-examples/hibernate7/ vs grails-data-hibernate7/),
so without an explicit signal an agent could reach for the wrong
skill's mental model in the wrong place.

Adds paths: grails-test-examples/** (additive, not restrictive - this
is still a repo-wide skill per its own description) plus a short
section grounding the skill's generic app-layout guidance in this
repo's actual location, and the real -PonlyFunctionalTests/
-PskipFunctionalTests gating flags (verified wired into 9 gradle
config files, not just README claims).

The July 6 OpenAI Codex-plugin insight this rule was originally based
on is specifically about cross-model review - a different vendor's
model catches what the authoring model is blind to. But mandating
that excludes any contributor without a second AI subscription, which
isn't a realistic bar for most people working on this repo.

The actual mechanism that does the work is the adversarial framing
itself (explicitly instructed to refute, not confirm) combined with a
genuinely fresh context (no memory of writing the change) - neither
requires a second vendor. Reframed the rule around what every
contributor actually has access to, with cross-model review named as
the stronger upgrade when available, not the baseline requirement.

Closes a gap from earlier in this branch's work: we found and cleaned
up real branch sprawl from the Hibernate7/GORM registry migration
(four branches independently attempting the same architectural work -
8.0.x-hibernate7, gorm-registry-refactor, gorm-scaling-clean,
fix/gorm-api-registration-scaling), but never turned that into
guidance for the next time it happens. This closes that loop.

Two-step process: classify a task as mechanical (bounded, just do it)
or architectural (a project, not a patch - check prior art first) -
then, for architectural work, check local/remote branches for prior
attempts using git merge-base --is-ancestor, not branch names or
commit dates. The real case is the worked example, including its
counterintuitive findings: the branch that actually won had an OLDER
last-commit date than the dead ends, and the largest diff (456 files)
was the most thoroughly abandoned one - both direct warnings against
the naive heuristics (recency, size) an agent might otherwise use.

Cross-references worktree-hygiene's PR-state check so "orphaned" isn't
confused with "in review."