[dashorst]

Adds REVIEW.md, the checklist a review of a pull request applies, and points to it from AGENTS.md. Closes #1868 once agreed.

AGENTS.md covers how to write a change. REVIEW.md adds what is easy to miss when a change otherwise looks right:

• License compliance: ASF headers, the ASF license categories of new dependencies, and LICENSE, LICENSE-binary, NOTICE and licenses/ for third-party material copied into the tree.
• Security: the security model in SECURITY.md as checks on the diff: context-specific escaping, untrusted request input, listener invocation, IPackageResourceGuard, encryption and logging, no deserialization of client data. A change that departs from the model updates SECURITY.md.
• OSGi and JPMS: exports and imports stay consistent between osgi.* properties, module-info.java and the pom. wicket-core and wicket-extensions list their exports explicitly, and optional dependencies end up as resolution:=optional.
• Performance: hot paths keep their complexity and footprint, or the commit justifies the cost with numbers from wicket-benchmarks. Removing a dependency is no reason to give up a performance gain (see the first draft of Remove dependencies Wicket's modules don't need #1865).
• Serialization: page size and compatibility of stored pages; a changed serialized form only in a major release, described in the migration guide.
• Accessibility: WCAG 2.2 level AA for markup, components and JavaScript that render UI, examples included.
• Documentation: Javadoc, user guide and migration-to-<major>.md updated in the same pull request.
• Tests: a failing test for every fix, *Test.java naming, test placement.

Points for discussion: WCAG 2.2 level AA is a target I picked as the common baseline; the project hasn't stated one before. Enforcement here means agents check against the list before handing a change over. A pull request template with the same checklist would put it in front of human contributors too, which I left out to keep the PR small.

🤖 Generated with Claude Code

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.72%. Comparing base (2c719d9) to head (1689006).
⚠️ Report is 18 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1869      +/-   ##
============================================
+ Coverage     62.70%   62.72%   +0.01%     
- Complexity    11507    11515       +8     
============================================
  Files          1254     1254              
  Lines         48660    48665       +5     
  Branches       6844     6845       +1     
============================================
+ Hits          30512    30524      +12     
+ Misses        15468    15467       -1     
+ Partials       2680     2674       -6     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dashorst]

Read against the writing-for-agents guidance (https://github.com/mattpocock/skills/blob/main/docs/productivity/writing-for-agents.md). REVIEW.md works as reference behind a pointer, and it is short enough. The comments cover four things: a trigger for the review case, a checkable completion criterion, duplication with AGENTS.md, and a few negations.

One point is outside the diff: AGENTS.md:104-107 says the migration guide lives on the wiki, where only a human can put it. That is stale now that migration-to-<major>.md lives in each branch, and it contradicts REVIEW.md:106. It should be fixed in this PR.

[dashorst]

This pointer fires only when a change is handed over. An agent asked to review a pull request has nothing that sends it to REVIEW.md. Covering both cases, and dropping "beyond these conventions", which REVIEW.md already says itself:

Suggested change

	- Before you hand a change over, check it against `REVIEW.md`: it lists what a review looks for
	beyond these conventions — licensing, OSGi and JPMS, performance, serialization, accessibility,
	documentation and tests.
	- Before handing a change over, and when reviewing a pull request, apply every section of
	`REVIEW.md` the change triggers: licensing, OSGi and JPMS, performance, serialization,
	accessibility, documentation and tests.

[dashorst]

"Applies" has no observable bound. The agent decides it, and the cheap way out is to call a section not applicable. I suggest opening each section with a checkable trigger instead of its rationale paragraph, for example:

• OSGi and JPMS: "Applies when the change adds, moves, renames or removes a package, adds a dependency, or touches module-info.java or an osgi.* property."
• Serialization: "Applies when a component, behavior or model gains or changes a field, or stores a lambda or anonymous class."
• Performance: fold the hot-path list from line 65 into the trigger.

The rationale can stay as a clause after the trigger. This paragraph then becomes the completion criterion for both the self-check and a review, and it also says what a review produces:

Suggested change

	Most changes touch only a few of the sections below. A section that doesn't apply needs no
	comment, but one that does apply needs an answer in the change itself, not a promise to follow
	up.
	Each section opens with when it applies. A review is done when every section the change
	triggers is accounted for: confirmed, or a finding naming the file and line. A finding is
	resolved in the change itself, in the same pull request.

[dashorst]

AGENTS.md already requires the header, and RAT enforces it. The .rat-excludes part is the only thing here an agent can't find out by looking.

Suggested change

	- Every new file carries the ASF license header. Apache RAT checks this during the build, so a
	missing header fails it; files that can't carry one belong in `.rat-excludes`, with a reason.
	- A file that can't carry the ASF license header is listed in `.rat-excludes`, with a reason;
	RAT fails the build on any other file without one.

[dashorst]

The last sentence is a rule shaped by one incident (the first draft of #1865), and "every operation Wicket relies on" already covers it.

Suggested change

	- A replacement for a library class — a collection, a cache, a parser — keeps the complexity of
	every operation Wicket relies on, not only the common ones. Removing a dependency is no reason to
	give up a performance gain.
	- A replacement for a library class — a collection, a cache, a parser — keeps the complexity of
	every operation Wicket relies on, not only the common ones.

[dashorst]

This sets two bars, and an agent will pick the weaker one. Split them by case:

Suggested change

	- Markup, components and JavaScript that render UI meet WCAG 2.2 at level AA, or at least don't
	make an existing component less accessible than it was.
	- New markup, components and JavaScript that render UI meet WCAG 2.2 at level AA. A change to
	existing UI leaves it at least as accessible as it was.

[dashorst]

Stated positively, so the exemption isn't what gets primed:

Suggested change

	- An example or a test page is not exempt: the examples are what applications copy.
	- Examples and test pages meet the same bar: they are what applications copy.

[dashorst]

This line is right about where the migration guide lives, but AGENTS.md:104-107 still says it is on the wiki and only a human can write it. The guides are in the branches now: migration-to-12.md on master, migration-to-11.md on wicket-11.x, migration-to-10.md on wicket-10.x. I suggest fixing AGENTS.md in this PR so it defines the migration path once (an entry in migration-to-<major>.md in the root of the branch, plus an OpenRewrite recipe where the change is mechanical), with this section pointing to it:

Suggested change

	- A change users notice — a new feature, a changed default, a removed or renamed API, a change in
	behavior or in dependencies — updates the documentation in the same pull request: the Javadoc,
	the user guide in `wicket-user-guide`, and for anything that breaks or changes an existing
	application, `migration-to-<major>.md` in the root of the branch.
	- A mechanical API change comes with an OpenRewrite recipe in `wicket-migration`, as `AGENTS.md`
	describes.
	- A change users notice — a new feature, a changed default, a removed or renamed API, a change in
	behavior or in dependencies — updates the Javadoc and the user guide in `wicket-user-guide`.
	- A change that breaks or changes an existing application comes with its migration path, as
	`AGENTS.md` describes.

[dashorst]

Lines 115-116 repeat AGENTS.md:69 almost word for word. Line 117 already points to AGENTS.md, which is the better approach for both.

Suggested change

	- A behavior change or a bug fix comes with a test that fails without it.
	- A test class is named `*Test.java`; Surefire runs nothing else, so a `*Tests` class or a
	`Test*` class passes review and never runs.
	- A test sits where `AGENTS.md` says: in the module's `-tests` companion where one exists, with
	its markup next to it in `src/test/java`.
	- A behavior change or a bug fix comes with a test that fails without it.
	- The test is named and placed as `AGENTS.md` says under "Build and test" and "Where code goes".

[papegaaij]

The security wording has to match SECURITY.md before this can go in; the rest are suggestions and one question.

[papegaaij]

As written, a reviewer, and certainly an agent following this file literally, records a possible vulnerability as "a finding naming the file and line" and has it resolved in the same pull request: a public comment describing the vulnerability. SECURITY.md asks not to report vulnerabilities through pull requests. An explicit exception here:

Suggested change

	resolved in the change itself, in the same pull request.
	resolved in the change itself, in the same pull request. A finding that may be a vulnerability
	is the exception: it never goes into a review comment, but to security@apache.org, as
	`SECURITY.md` describes.

[dashorst]

Applied as suggested in 1689006, folded into the PR's single commit.

[papegaaij]

This reads as if a public pull request for an unreleased vulnerability is fine as long as its title, commit message and branch name are vague. AGENTS.md and SECURITY.md keep it out of a pull request altogether, and a reviewer who spots a vulnerability in code the change touches needs to know where it goes:

Suggested change

	- A fix for an unreleased vulnerability keeps that out of its title, commit message and branch
	name, as `AGENTS.md` describes.
	- A suspected vulnerability, in the change or in code it touches, is reported to
	security@apache.org and not mentioned in the pull request. A fix for an unreleased
	vulnerability is not prepared in a public pull request; see `SECURITY.md`.

[dashorst]

Applied as suggested in 1689006, folded into the PR's single commit.

[papegaaij]

This is new policy rather than a review check: nothing in AGENTS.md or SECURITY.md says a maintenance line keeps its serialized form, and bug fixes on the maintenance lines have changed component fields before. As a rule it's too strict. It would block ordinary fixes on 10.x and 11.x, and "a new field type" reads as if adding a field breaks reading back, which it doesn't with a fixed serialVersionUID. If you want a rule like this, could you raise it on dev@ first, and drop it from this file until there's agreement?

Suggested change

	- A change to the serialized form — a new field type, a different collection class, a changed
	`serialVersionUID` — means pages stored by the previous version can't be read back. Such a
	change lands in a major release on `master` and is described in the migration guide; a
	maintenance line keeps the serialized form it was released with.

[papegaaij]

Test pages in the test modules are never seen by a user, and are often deliberately minimal or deliberately broken (an input without a label, an image without alt) because that's what the test exercises. Holding them to WCAG AA doesn't help anyone and will be ignored in practice, which weakens the rest of the list. "They are what applications copy" holds for the examples, so I'd keep the bar there:

Suggested change

	Applies when the change touches markup, a component or JavaScript that renders UI, examples and
	test pages included. Components produce the HTML of every application that uses them, so their
	Applies when the change touches markup, a component or JavaScript that renders UI, examples
	included. Components produce the HTML of every application that uses them, so their

[papegaaij]

Same as above: examples, not test pages.

Suggested change

	- Examples and test pages meet the same bar: they are what applications copy.
	- Examples meet the same bar: they are what applications copy.

[papegaaij]

This list repeats the section headings of REVIEW.md, and has to be kept in step with them by hand. "Every section the change triggers" already says it:

Suggested change

	- Before handing a change over, and when reviewing a pull request, apply every section of
	`REVIEW.md` the change triggers: licensing, security, OSGi and JPMS, performance,
	serialization, accessibility, documentation and tests.
	- Before handing a change over, and when reviewing a pull request, apply every section of
	`REVIEW.md` the change triggers.

[papegaaij]

The list of AGENTS.md sections is already incomplete: it leaves out "Where code goes" and "Poms", and it will drift further as sections are added. The reference alone does the job:

Suggested change

	The conventions for writing a change — format, commits, build, API compatibility, Javadoc,
	comments, language and security — are in `AGENTS.md`. A review checks the change against those as
	well; this file adds what is easy to miss when a change otherwise looks right.
	The conventions for writing a change are in `AGENTS.md`. A review checks the change against
	those as well; this file adds what is easy to miss when a change otherwise looks right.

[papegaaij]

Do we want to maintain migration documents for several versions on several branches at the same time? "In the root of the branch" means a change cherry-picked down gets its entry in a different file on every line, as #1866 did this week in migration-to-10.md on 10.x and migration-to-11.md on 11.x. The copies already drift: the 11.1.0 entry is in migration-to-11.md on wicket-11.x but not in the one on master.

I'd rather keep all migration documents on master and keep them up to date there. Breaking changes only land on master anyway, and a behaviour change in a maintenance release can get its entry in that version's guide on master. The rule would then point at master instead of the root of the branch.

[dashorst]

I'd rather have each in their respective branches than a long list of migration guides in our main branch, which is the current status AFAIK.

[papegaaij]

The problem I see with that solution, is that it makes it very difficult to render them all in in a single document. I guess there's no good solution for this.