jamesfredley opened a new pull request, #16025: URL: https://github.com/apache/grails-core/pull/16025
## Description This PR turns recurring, objective review feedback into deterministic repository checks while deliberately leaving subjective engineering judgment in review. The starting point was an audit of recent merged and proposed changes in the `#16012` through `#15688` PR-number window. The recurring mechanical concerns were action immutability, static-analysis drift, duplicate message keys, agent-skill metadata drift, report trustworthiness, and license/provenance checks. Repeating those comments by hand does not scale. This change creates a ratchet: enforce the clean baseline now, preserve compatibility for broader opt-in analysis, and expand enforcement only as the recorded backlog is resolved. No Grails runtime API or application behavior changes in this PR. ### What this moves forward #### 1. PMD becomes blocking where it is already clean PMD is now enabled by full Gradle project path for the four modules that produce clean, non-empty PMD XML today: - `:grails-data-graphql-core` - `:grails-data-mongodb-spring-data` - `:grails-datasource` - `:grails-testing-support-core` The implementation supports both staged project allowlists and the existing global opt-in flags. It also preserves `grails.code-analysis.ignoreFailures=true` for report-only baseline runs. Analysis and style reports are now tied to exact task-owned outputs instead of scanning a broad directory. Before analyzers run, active outputs are cleaned. Each analyzer that actually executes writes a relocatable marker containing the configured XML output's root-relative path, so custom directories and filenames remain associated without serializing an absolute machine path. A genuinely missing report always fails while an unscheduled or legitimate `NO-SOURCE` task is omitted. One writer task owns each pair of Markdown summaries, runs after scheduled analyzers, and is reached through a no-output finalizer even when an analyzer fails. Direct single-module analyzer tasks therefore remain usable, consumers cannot race a second report writer, and build-cache relocation remains portable. Marker filenames encode the full project path, so nested projects with the same leaf name cannot collide. Generated files below each project's configured build directory are excluded from PMD inputs without excluding normal sources when a checkout ancestor happens to be named `build`. #### 2. Repository conventions have one public Gradle surface `./gradlew validateRepositoryConventions` now validates the canonical repository root and writes `build/reports/violations/REPOSITORY_CONVENTIONS.md`. It checks these scenarios: - Canonical `.agents/skills/*/SKILL.md` files start with valid top-level YAML front matter containing string `name`, `description`, and `license` values. - Skill names match their directory and are unique. - Every canonical skill path is listed in `AGENTS.md`, and every listed path exists. - Workflow and local action-manifest YAML is parsed with SnakeYAML's data-only `SafeConstructor`. - Every external `uses` reference is pinned to a lowercase 40-character commit SHA. - A given external action uses one consistent SHA across workflows and composite actions. - Docker `uses`, Docker action `runs.image`, and workflow job/service container images use immutable lowercase `sha256` digests rather than mutable tags. - Duplicate YAML keys and malformed YAML fail closed. - Logical keys in `messages*.properties` are unique, including escaped separators and continued lines. - Generated/build trees do not create false duplicate-key findings. - RAT runs before repository conventions so license provenance remains part of the same developer-facing gate. The task is wired into `aggregateStyleViolations` and `aggregateViolations` only for the canonical root. Independent builds such as `grails-gradle` retain their existing aggregate style behavior and do not require a local `AGENTS.md`. #### 3. GitHub Actions are immutable by construction Remaining symbolic external action references were replaced with verified commit SHAs, including Checkout, Cache, Setup Java, and the shared Grails action repository. The Mongo service image is pinned to its Docker Hub manifest digest rather than the mutable `mongo:8` tag. The new validator covers ordinary steps, reusable workflow jobs, quoted keys, inline YAML maps, YAML aliases, and repository-local composite actions in any `action.yml` or `action.yaml` location, including a path segment named `build`. Local `./...` references remain valid, but any external actions used inside their manifests are still validated. #### 4. Release provenance matches RAT provenance Agent-local state, configuration symlinks, and guidance files excluded from RAT are now excluded from the source ZIP with matching patterns. The source checkout no longer persists GitHub credentials, and Info-ZIP stores any remaining repository symlinks as links rather than dereferencing their targets. This prevents an excluded file or linked external path from being shipped unaudited. #### 5. Existing deterministic drift is repaired - Removed the duplicate `spring.security.ui.forgotPassword.email.line4` message key. - Corrected stale agent-skill guidance and added the canonical mono-repository integration skill to `AGENTS.md`. - Updated the violation-fixer skill with the staged-analysis properties, truthful disabled-tool report semantics, and the new repository convention workflow. ### Why the rollout is staged Enabling every analyzer everywhere in this PR would require thousands of unrelated source edits and would make the convention work impossible to review safely. This PR goes as far as the repository can go without mixing a broad cleanup campaign into the build-policy change: - It makes currently clean, report-producing PMD modules blocking. - It keeps global PMD and SpotBugs baseline modes available. - It preserves repository-wide SpotBugs execution in separate non-blocking CI advisory passes with distinct artifacts while #16014 tracks task-wiring and detector failures. - It reports disabled tools explicitly instead of calling them clean. - It files quantified follow-up issues for every known area that cannot be enabled safely yet. - It adds the infrastructure needed to extend the allowlists one module at a time. Subjective concerns such as architecture, compatibility judgment, semantic documentation quality, test sufficiency, and quote/type preferences remain review-only. They are documented for agents but are not encoded as brittle mechanical rules. ### Remaining work and follow-up issues | Issue | Remaining work | Why it is not in this PR | | :--- | :--- | :--- | | #16014 | Stabilize SpotBugs task wiring and detector execution | Broad runs currently expose undeclared generated-output dependencies and detector `IndexOutOfBoundsException` failures. Tool execution must be reliable before enforcement. | | #16018 | Triage the provisional 287-finding SpotBugs baseline | The baseline must be regenerated after #16014, then fixed or narrowly documented before modules can become blocking. | | #16017 | Resolve 5,551 PMD findings across the remaining root modules | Fixing these findings would create a large, unrelated source diff. The issue records maintained-source counts and the incremental allowlist process. | | #16016 | Resolve 113 PMD findings in `grails-gradle-model` | The independent Gradle-plugin build remains report-only until its existing model findings are fixed. | | #16015 | Resolve 224 maintained-source PMD findings in Grails Forge and add supported staged CI enforcement | Forge intentionally has no aggregate analysis task, so its explicit source-bearing PMD task set has a separate cleanup and enforcement path. | The clean PMD baseline was corrected during verification: `grails-scaffolding`, `grails-spring-security`, and `grails-spring-security-acl` initially appeared as zero-finding modules, but their PMD tasks produce no XML because they have no PMD-analyzable Java source. They are not presented as clean and are not allowlisted. ### Suggested next steps 1. Resolve #16014 so SpotBugs completes without task-wiring or detector failures. 2. Regenerate and triage the SpotBugs baseline in #16018, then introduce a staged SpotBugs project allowlist. 3. Start PMD cleanup with the smallest root modules in #16017 and add each report-producing clean module to the blocking allowlist in the same PR. 4. Complete the focused `grails-gradle-model` cleanup in #16016. 5. Clean Forge's 224 maintained-source findings from the smallest modules upward, then make its explicit source-bearing PMD task set blocking in Forge CI under #16015. ### Verification - `build-logic`: `..\gradlew.bat build --no-watch-fs --no-daemon` - Independent `grails-gradle`: `.\gradlew.bat aggregateStyleViolations --no-watch-fs --no-daemon --max-workers=2` - Independent `grails-gradle` PMD baseline: `.\gradlew.bat aggregateAnalysisViolations --continue "-Pgrails.code-analysis.enabled.pmd=true" "-Pgrails.code-analysis.ignoreFailures=true" --rerun-tasks --no-watch-fs --no-daemon --max-workers=2`; it reports only the 113 maintained-source findings in `:grails-gradle-model` and omits three legitimate `NO-SOURCE` tasks. - Direct analyzer surface: `.\gradlew.bat :grails-datasource:pmdMain --no-watch-fs --no-daemon --max-workers=2`; the single requested analyzer and its finalizer succeed without false missing reports for other allowlisted modules. - Root selective analysis: `.\gradlew.bat aggregateAnalysisViolations --continue --no-daemon --max-workers=2 --no-watch-fs` - Code-analysis CI preserves repository-wide SpotBugs execution as separate `continue-on-error` advisory passes for the root and `grails-gradle`, each uploaded under a distinct artifact name after the enforced PMD reports. - Root report-only baseline: `.\gradlew.bat aggregateAnalysisViolations --continue "-Pgrails.code-analysis.enabled.pmd=true" "-Pgrails.code-analysis.ignoreFailures=true" --no-watch-fs --no-daemon --max-workers=2`; 5,551 maintained-source findings across 53 source-bearing modules. - Mandatory pre-commit gate: `.\gradlew.bat clean aggregateViolations :grails-test-report:check --continue --no-daemon --no-parallel --max-workers=2 --no-watch-fs` - The mandatory run completed 1,984 tasks and generated the full test-report matrix. All convention, style, selective-analysis, and report aggregation tasks passed. The aggregate integration report recorded 3,617 tests, 2 failures, 0 errors, and 98 skipped. One unchanged Windows-only mail example failed because its GSP renders CRLF while the test hard-codes LF; a serial targeted run reproduced the same `Hello\r\nWorld!` versus `Hello\nWorld!` mismatch in `MailServiceSpec`. The unchanged SiteMesh async multiple-layout scenario received a transient HTTP 500 during the full run; an isolated serial rerun of `EndToEndSpec` then passed all 14 tests. Neither example implementation nor test is changed by this PR. - Final root `aggregateViolations` after all review fixes and rebasing onto `origin/8.0.x`: 751 tasks completed successfully. - Final reports: Checkstyle, CodeNarc, PMD, and repository conventions are clean; SpotBugs explicitly reports `SpotBugs is disabled.` - Manual failure path: a quoted mutable action, inline Docker tag, mutable Docker action image, mutable service container, build-path local action, and repository path escape were rejected with structural YAML locations, then the fixtures were removed and the clean task rerun. - Invalid PMD root/unknown project paths, configured build-directory exclusion without checkout-path false positives, unsuppressible missing reports, failed-analyzer report writing, stale-output removal after a later writer failure, direct single-project style/analysis, legitimate `NO-SOURCE`, custom report directories and filenames, portable root-relative markers, single-writer ordering, duplicate nested project names, top-level skill metadata typing, duplicate YAML keys, malformed YAML, semantic versus ordinary `uses` fields, recursive build-path actions, repository path escapes, case-insensitive Docker metadata, immutable container images, and configuration-cache reuse are covered by TestKit. The Windows validation run disabled Gradle file-system watching because Gradle's native `gradle-fileevents.dll` crashed a long-lived daemon. The same checks passed with `--no-watch-fs`; no product or test failure was hidden. Open PR #15977 also changes the `AGENTS.md` skill inventory. It is not a semantic duplicate of this work, but the documentation overlap should be resolved when either branch is rebased. ## Contributor Checklist ### Issue and Scope - [x] No pre-existing implementation issue covered this cross-cutting convention work. The audit background and rationale are documented above; all discovered legacy cleanup was separated into #16014 through #16018. - [x] This PR completes its stated scope: deterministic enforcement infrastructure and the currently clean baseline. The linked follow-up issues are explicitly separate cleanup projects, not unfinished code in this PR. - [x] This PR contains one focused change: turn recurring objective repository review conventions into automated checks and a staged analysis ratchet. - [x] This PR targets `8.0.x`, the current major-development branch. ### Code Quality - [x] Tests were added or updated for every changed build-logic behavior. - [ ] The separate `./gradlew build --rerun-tasks` command was not run because no runtime module behavior changed. The mandatory `:grails-test-report:check` matrix did run: 3,617 integration tests were reported before the single unchanged Windows CRLF mismatch described above, and a serial targeted rerun confirmed it. The affected build-logic suite, independent `grails-gradle` surfaces, and 750-task root aggregate gate pass. - [x] The project code-style and aggregate violation workflows pass. - [x] This PR does not contain mass reformatting or broad legacy-analysis cleanup. - [x] Generative tooling used a quality model and the final diff was independently reviewed for goal fit, code quality, security, hands-on QA, and repository context. ### Licensing and Attribution - [x] New source files contain Apache License 2.0 headers, and RAT passes. - [x] The contribution is original work prepared for this repository. - [x] Generative tooling use follows ASF policy and is attributed in the commit metadata. ### Documentation - [x] Developer-facing guidance in `AGENTS.md` and the violation-fixer skill documents the new commands, properties, reports, and remediation paths. - [x] This build-policy change does not add a Grails user-facing feature, so no What's New entry is required. - [x] This change does not alter runtime APIs or require application upgrade steps, so no Upgrade Notes entry is required. - [x] The description explains the problem, each enforced scenario, the staged boundary, verification, remaining issues, and next steps. -- 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]
