jamesfredley commented on code in PR #16025:
URL: https://github.com/apache/grails-core/pull/16025#discussion_r3677419634
##########
.github/workflows/release.yml:
##########
@@ -373,11 +374,18 @@ jobs:
find . -depth \( -type f -o -type d \) -exec touch -d
"@${SOURCE_DATE_EPOCH}" {} +
- name: "📦 Create source distribution ZIP"
run: >
- zip -r
+ zip -yr
Review Comment:
Fixed. The normalization step gained a symlink pass:
```bash
find . -depth \( -type f -o -type d \) -exec touch -d
"@${SOURCE_DATE_EPOCH}" {} +
find . -depth -type l -exec touch -h -d "@${SOURCE_DATE_EPOCH}" {} +
```
This matters more now than when you wrote the comment, because the
exclusions for `CLAUDE.md` and the other symlinks were dropped in response to
your other thread, so they are genuinely stored as links by `zip -y` rather
than being skipped. Their targets (`AGENTS.md`, `.agents/skills/**`) are inside
the zip, so no dangling links either.
##########
.github/workflows/release.yml:
##########
@@ -373,11 +374,18 @@ jobs:
find . -depth \( -type f -o -type d \) -exec touch -d
"@${SOURCE_DATE_EPOCH}" {} +
- name: "📦 Create source distribution ZIP"
run: >
- zip -r
+ zip -yr
${DIST_NAME}-${VERSION}-src.zip
grails
-x 'grails/.git/*'
Review Comment:
Fixed by making the comment true rather than by editing the comment: `-x
'grails/.asf.yaml'` is now in the zip exclude list. It is GitHub-side ASF
metadata in the same category as `.github/**`, which is already excluded, so
shipping it was the anomaly.
##########
AGENTS.md:
##########
@@ -79,6 +82,19 @@ export GRADLE_OPTS="-Xms2G -Xmx5G"
| **hibernate-developer** | `.agents/skills/hibernate-developer/SKILL.md` |
Hibernate 7 mapping, binders, generators |
| **violation-fixer** | `.agents/skills/violation-fixer/SKILL.md` | Fix
style/analysis violations (CodeNarc, Checkstyle, PMD, SpotBugs) |
| **test-fixer** | `.agents/skills/test-fixer/SKILL.md` | Aggregate and fix
test failures |
+| **mono-repo-integration** | `.agents/skills/mono-repo-integration/SKILL.md`
| Integrate a standalone repository into the monorepo |
+
+## Repository Conventions
+
+Run `./gradlew validateRepositoryConventions` to check canonical skill
metadata and AGENTS.md synchronization, external GitHub Action SHA pins in
workflows and local composite actions, immutable Docker digests, duplicate
message keys, and RAT license provenance. The task writes
`build/reports/violations/REPOSITORY_CONVENTIONS.md` and is included by
`aggregateStyleViolations` and `aggregateViolations`.
Review Comment:
You are right that this was stronger than a documentation overlap, and it
should not have been the validator's call to make. Fixed by removing the
requirement.
`validateRepositoryConventions` no longer requires a skill to be listed in
AGENTS.md. It now only validates the direction that is true under both models:
a skill path that AGENTS.md *does* reference must exist, so a dangling
reference (like the `codebase-memory` one #15977 fixes) still cannot survive.
Per-skill front matter, name-to-directory match, and name uniqueness are
unchanged, since those are uncontested.
That means #15977's discovery-based AGENTS.md and the current explicit index
both pass this gate, and neither PR has to win before the other can merge. One
note for whichever rebases second: the front-matter rules do require `name`,
`description`, and `license` as strings on every canonical `SKILL.md`, so any
new skill in #15977 missing `license` will need it added.
##########
AGENTS.md:
##########
@@ -52,7 +55,7 @@ export GRADLE_OPTS="-Xms2G -Xmx5G"
9. **Test via public APIs** - Tests must exercise behavior through the same
APIs an end user calls; never invoke internal implementations, package-private
methods, or bypass the public surface directly
10. **Always review and extend tests** - Review existing unit and functional
tests before making changes; every code change must include new or enhanced
tests that cover the affected behavior
11. **Every code touch must update all tests for the changed class** - When a
class is modified, find and update every test that covers it — unit,
integration, and TCK. Do not leave any existing test out of sync with the new
code.
-12. **Clean violations before commit** - Before every automated commit, run
`./gradlew clean aggregateViolations :grails-test-report:check --continue` from
the root and ensure that `build/reports/violations/CHECKSTYLE_VIOLATIONS.md`,
`build/reports/violations/CODENARC_VIOLATIONS.md`,
`build/reports/violations/PMD_VIOLATIONS.md`, and
`build/reports/violations/SPOTBUGS_VIOLATIONS.md` report no issues. Also review
the test result reports under `grails-test-report/build/reports/tests/` and
ensure there are no failures. The aggregate reports are wired as test
finalizers and will be attempted after failures, but `--continue` is required
for comprehensive full-suite reports.
+12. **Clean violations before commit** - Before every automated commit, run
`./gradlew clean aggregateViolations :grails-test-report:check --continue` from
the root. Ensure Checkstyle, CodeNarc, and PMD reports have no issues for their
enabled projects; PMD is enforced only for the project paths in
`grails.code-analysis.enabled.pmd.projects`. Disabled tools report their
disabled status, not a clean result. Also review the test result reports under
`grails-test-report/build/reports/tests/` and ensure there are no failures. The
aggregate reports are wired as test finalizers and will be attempted after
failures, but `--continue` is required for comprehensive full-suite reports.
Review Comment:
Fixed in both places. Rule 12 and Pull Request Guidelines step 4 now name
the Checkstyle, CodeNarc, REPOSITORY_CONVENTIONS, and PMD/SpotBugs reports,
note that PMD and SpotBugs apply to their enabled projects, and state that a
disabled tool reports its disabled status rather than a clean result. The
numbered list in the guidelines also had two items numbered `6`, which is fixed
while I was in there.
The PMD/SpotBugs enablement wording changed with the other review item: it
is now the per-project `grailsCodeAnalysis { pmdEnabled = true }` opt-in in
each module's own `build.gradle`, with the `-P` properties documented as
overrides.
##########
AGENTS.md:
##########
@@ -79,6 +82,19 @@ export GRADLE_OPTS="-Xms2G -Xmx5G"
| **hibernate-developer** | `.agents/skills/hibernate-developer/SKILL.md` |
Hibernate 7 mapping, binders, generators |
| **violation-fixer** | `.agents/skills/violation-fixer/SKILL.md` | Fix
style/analysis violations (CodeNarc, Checkstyle, PMD, SpotBugs) |
| **test-fixer** | `.agents/skills/test-fixer/SKILL.md` | Aggregate and fix
test failures |
+| **mono-repo-integration** | `.agents/skills/mono-repo-integration/SKILL.md`
| Integrate a standalone repository into the monorepo |
+
+## Repository Conventions
+
+Run `./gradlew validateRepositoryConventions` to check canonical skill
metadata and AGENTS.md synchronization, external GitHub Action SHA pins in
workflows and local composite actions, immutable Docker digests, duplicate
message keys, and RAT license provenance. The task writes
`build/reports/violations/REPOSITORY_CONVENTIONS.md` and is included by
`aggregateStyleViolations` and `aggregateViolations`.
+
+Review-only checklist:
Review Comment:
Fair, they were labels rather than guidance. Each one is now a one-line
actionable statement pointing at a concrete in-repo anchor - quote and type
preference at the `## Groovy Style` section, test sufficiency at Critical Rules
9, 10, and 13, compatibility at the `## Technology Stack` table, and so on -
with a leading sentence stating explicitly that these are human-review concerns
that are intentionally not mechanized, and that everything mechanizable is
enforced by `validateRepositoryConventions`.
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsCodeAnalysisPlugin.groovy:
##########
@@ -128,33 +131,38 @@ class GrailsCodeAnalysisPlugin implements Plugin<Project>
{
project.tasks.withType(Pmd).configureEach {
it.group = 'verification'
- it.onlyIf { !project.hasProperty('skipCodeStyle') }
+ it.onlyIf { !skipCodeStyle.present }
it.ignoreFailures = ignoreFailures.get()
if (it.name.contains('Test') || it.name.contains('test')) {
it.enabled = testStylingEnabled.get()
}
+ it.exclude { org.gradle.api.file.FileTreeElement element ->
+
element.file.toPath().toAbsolutePath().normalize().startsWith(projectBuildDirectory.get())
+ }
+
it.reports.xml.required.set(true)
it.reports.xml.outputLocation.set(
- project.extensions.getByType(GrailsCodeAnalysisExtension)
- .reportsDirectory.get()
- .dir('pmd')
- .file("${project.name}-${it.name}.xml")
+
project.extensions.getByType(GrailsCodeAnalysisExtension)
+ .reportsDirectory
+ .file("pmd/${GradleUtils.reportFileName(project,
it.name)}")
)
+ GradleUtils.configureReportMarker(it,
project.rootProject.layout.projectDirectory, it.reports.xml.outputLocation,
+ GradleUtils.reportMarker(project, 'pmd', it.name))
}
}
static void configureSpotbugs(Project project) {
Review Comment:
Fair, that was a real gap: the path was rewired and nothing exercised it.
There is now a TestKit feature that opts a fixture project into SpotBugs, runs
`spotbugsMain` against a deliberately buggy class, and asserts both the marker
and the `SPOTBUGS_VIOLATIONS.md` content, so the allowlist, the
`reports.maybeCreate('xml')` relocation, the marker write, and the
`parseAnalysisViolations` SpotBugs branch are all covered end to end rather
than only in CI passes that cannot fail.
##########
build-logic/plugins/src/test/groovy/org/apache/grails/buildsrc/GrailsViolationAggregationPluginSpec.groovy:
##########
@@ -73,11 +76,33 @@ class GrailsViolationAggregationPluginSpec extends
Specification {
result.output.contains('aggregateStyleViolations')
result.output.contains('aggregateAnalysisViolations')
result.output.contains('aggregateViolations')
+ result.output.contains('validateRepositoryConventions')
result.output.contains('aggregateJacocoCoverage')
}
- def "aggregateStyleViolations writes CodeNarc and Checkstyle reports to
build/reports/violations/"() {
- given: "a root project with a subproject that has codestyle XML
reports"
+ def "canonical roots report a missing AGENTS.md"() {
+ given:
+ testProjectDir.resolve('settings.gradle').toFile().text = ''
+ testProjectDir.resolve('.github/workflows').toFile().mkdirs()
+ testProjectDir.resolve('build.gradle').toFile().text = """
+ plugins {
+ id 'org.apache.grails.gradle.grails-violation-aggregation'
+ }
+ """
+
+ when:
+ def result = GradleRunner.create()
+ .withProjectDir(testProjectDir.toFile())
+ .withArguments('validateRepositoryConventions', '--stacktrace')
+ .withPluginClasspath()
+ .buildAndFail()
+
+ then:
+ result.output.contains('AGENTS.md: file is missing')
+ }
+
+ def "nested builds ignore stale legacy style XML without AGENTS.md"() {
Review Comment:
Correct on both counts, and restored. There is now a Checkstyle
positive-path feature with real sources that asserts report content and module
resolution through the marker, so the marker-based Checkstyle parse branch is
exercised with data again rather than running NO-SOURCE. The style-side
missing-report failure is covered too, so it is no longer only the PMD side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
