Move legacy command compatibility tests into an end-to-end build

[jdaugherty]

Addresses the outstanding review feedback on the Grails 7 command compatibility layer.

Build placement

The Grails 7 fixture needs Java 17 - the minimum for a Grails 7 app, so the binary matches what a real Grails 7 plugin is built with - but expressing that as a Gradle toolchain put an unprovisionable JDK requirement into the core build's task graph. Nothing in the repo provisions a 17 (no foojay resolver, no toolchainManagement, no org.gradle.java.installations.*), and because the fixture jar was an implementation dependency, ./gradlew build -PskipTests reached it - so the build failed on the JDK the project documents in .sdkmanrc and in the reproducible-build container. CI only passed because the runner images happen to ship a 17 that Gradle auto-detects.

The three projects now live in a new top-level end-to-end build, so the root build no longer reaches them. Each half declares its JDK in its own .sdkmanrc instead of a toolchain, and a dedicated workflow provisions both, reading the versions out of those files.

Resolution goes through published artifacts rather than project substitution, which is what makes these tests end-to-end: they consume grails-core the way an application does, through real poms and module metadata. The repository is the same build/local-maven that grails-forge points its generated applications at, populated by publishAllPublicationsToTestCaseMavenRepoRepository in both the root and grails-gradle builds. settings.gradle scopes it with exclusiveContent so a remote snapshot cannot quietly satisfy an org.apache.grails request and leave the suite testing something other than the working tree.

That also disposes of the CLI companion problem rather than working around it. grails-core-cli is a secondary capability of :grails-core, not a project, so composite substitution cannot express it and hits a capability self-conflict - but it is a first-class published module whose metadata CliPublishingSupport already rewrites for external consumers, so resolving from the repository gets it for free.

The Grails 7 fixture stays outside all of this as a standalone build under its own JDK, consumed as a prebuilt jar.

Trait-derived command names

Every legacy command in the tree overrode getName()/getDescription(), so the trait's default derivation - what create-command generated on Grails 7, and therefore what most published Grails 7 commands rely on for their registration key - had no coverage. Adds a third precompiled command declaring neither getter.

Factory resource failures are no longer silent

loadFactoryDeclarations dropped a malformed resource with no log line at any level, and it backs modern grails-cli.factories discovery, so one bad file lost every one of a plugin's Grails 8 commands and providers. It now warns with the resource URL and cause, keeping the per-resource isolation.

skipBootstrap resolution is covered

The lookup moves into a static helper so the adapter-target case - where reading the flag off the adapter instead of its target would let BootStrap run during dbm-update - is guarded by a test.

Command discovery: instantiate once, and order deterministically

Two production changes in the discovery path that are not just test scaffolding:

ApplicationContextCommandRegistry collects command classes before instantiating any of them. The dual-classloader scan previously instantiated as it went, per classloader, so a command class reachable through both the registry classloader and the context classloader was constructed twice - its constructor side effects running twice - only for the second instance to be discarded on the name-collision check. Discovery now gathers Class -> origin across both loaders first, then instantiates each distinct class exactly once. instantiate(Class) is also generified so commands and providers share one linkage-error-unwrapping path instead of two near-identical copies.

LegacyApplicationCommandAdapter now implements Ordered and propagates the adapted command's order. Registration is first-wins, so without this, two Grails 7 plugins shipping the same command name were resolved by jar scan order. The adapter resolves Ordered.getOrder() or @Order off the legacy command it wraps, and LegacyApplicationCommandProvider sorts adapters by target class name for a stable baseline and then by declared order - the same two-stage ordering the modern commands already get. A Grails 7 command that used Spring ordering to win a name collision keeps winning it.

ThreadDeath removed from the fatal-error guards

The rethrowIfFatal helpers added for the linkage-error split checked both VirtualMachineError and ThreadDeath. ThreadDeath is dropped, in the four helpers and in the eight tests that fed it in (now OutOfMemoryError): on the Java 21 baseline Thread.stop() throws UnsupportedOperationException, so ThreadDeath can no longer be thrown by the JVM and the branch is unreachable and deprecated. VirtualMachineError rethrow - the branch that actually fires - is unchanged, at every boundary. Calling this out explicitly because it narrows a guard that was added at review request.

Also

• Drops a duplicated fixture version pin in the integration spec, and corrects comments describing the fixture as an included composite build.
• Adds AbstractProfileSpec, covering the shell unknown-command path appending the runtime legacy-command hint (and not appending it when there is no hint).
• grails-forge CommandSpec: raises the generated-application timeout to 600s and adds early-failure diagnostics.

[jdaugherty]

@jamesfredley The tests were timing out in CI (only 5-10% head room on these). So i bumped the timeout and made them fail fast so in the future you aren't waiting the full timeout. This change is really not needed for the original issue, but it removes teh flakiness.

[jamesfredley]

The build boundary here is right, and it resolves the objection I raised on #16011 properly rather than around it. I verified the part that actually matters: the Grails 7 fixture is still un-substitutable after the move. It is a separate Gradle invocation with its own settings.gradle and its own mavenCentral()-only repository, so the new exclusiveContent scoping cannot reach it, enforcedPlatform('org.apache.grails:grails-bom:7.0.14') survives, and the trait-woven Groovy 4 bytecode is still genuinely Groovy 4. The root build no longer reaches any of the three projects and there is not one stale grails-test-examples-legacy-* reference left in the tree.

The skipBootstrap extraction is clean - run() is byte-for-byte equivalent in control flow, and the mutation I asked about (reading the flag off command instead of the resolved target) does fail the adapter-target feature. It locks the resolver rather than run() itself, since the integration spec still hand-unwinds the autowire instead of driving the runner, but that is the smaller half and I am fine with it.

Comments inline. The workflow ones are the substantive set: this suite is now the only thing that runs these tests, so its triggers and its gates carry weight they did not carry when the root build reached the projects. Three things that are not line-level:

1. LegacyApplicationCommandAdapterSpec was not updated for the new Ordered behaviour. The adapter gained implements Ordered plus resolveOrder, and the new coverage for it lives in LegacyCommandRegistryLoadingSpec and only exercises the @Order annotation branch. The legacyCommand instanceof Ordered branch in resolveOrder has no test, and the adapter's own direct spec is now out of sync with the class it covers.

2. AGENTS.md still lists three independent Gradle builds. The project table at AGENTS.md:97-106 (build-logic, grails-gradle, grails-forge) is now incomplete - end-to-end is a fourth, with its own settings.gradle, its own wrapper and its own workflow. Worth adding a row pointing at end-to-end/README.md and ./gradlew check, since a contributor following that table will not know the suite exists.

3. I have updated #16011's description to match the post-merge state: the fixture paths (grails-test-examples/* -> end-to-end/*), the fixture version claim (it said grails-core:7.0.10 / Groovy 4.0.30; it is grails-bom:7.0.14 / Groovy 4.0.32), the three precompiled commands including the derived-name one, and the published-artifact resolution model. I have also expanded this PR's description, which described about half of its own production diff - the registry restructure, the Ordered propagation, and the ThreadDeath removal were not mentioned at all.

None of this is a blocker on the approach. Fix the workflow items and I am happy to see this merged into feat/8.0.x-legacy-command-compat.

[jamesfredley]

${fixture_java%%.*} truncates 17.0.18-librca to 17, and the same for 21.0.7-librca -> 21. So the comment two lines up - "so the workflow cannot drift from what a developer gets with sdk env" - is not what the code does: it pins only the major, and the actual JDK is whatever Liberica major the runner image currently ships. A runner update silently changes the JDK both the fixture and the suite are built on, which is precisely the drift this step exists to prevent.

release-verify.yml already has the shape you want here - it keeps the patch with ${SDKMAN_JAVA%-*}, stripping only the vendor suffix. Same thing here would make the comment true:

echo "fixture-java=${fixture_java%-*}" >> "$GITHUB_OUTPUT"
echo "build-java=${build_java%-*}" >> "$GITHUB_OUTPUT"

If the full patch version is deliberately not pinned because setup-java cannot always satisfy an exact Liberica patch, then say that in the comment rather than claiming no-drift.

[jdaugherty]

FYI: this is intentionally not pinned because we don't verify test apps as part of being reproducible.

[jamesfredley]

Two problems that compound, and together they defeat the stated purpose of the verify step.

JAVA_HOME_17_X64 hardcodes the very version the step above just derived. The 17 in that variable name is a literal. If legacy-g7-command-plugin/.sdkmanrc ever moves off 17, steps.jdks.outputs.fixture-java follows it and setup-java provisions the new major - but env.JAVA_HOME_17_X64 is then unset, JAVA_HOME resolves to empty, and the fixture builds on the default JDK (21) instead. Nothing fails. Deriving the variable name would fix it:

env:
  JAVA_HOME: ${{ env[format('JAVA_HOME_{0}_X64', steps.jdks.outputs.fixture-java)] }}

And the verify step cannot catch that. Its comment says "a fixture silently built by the wrong toolchain would still pass the suite while proving nothing, so fail loudly here instead" - but it only greps Grails-Compile-Version and Groovy-Compile-Version, which come from the resolved BOM, not from the JDK. A fixture compiled on 21 against Grails 7.0.14 / Groovy 4.0.32 still prints 7. and 4. and sails through.

That matters more than usual here because legacy-g7-command-plugin/build.gradle sets no release, sourceCompatibility or toolchain (deliberately - that was the point of the move), so the class file version is determined entirely by JAVA_HOME. The check that would actually be loud is the bytecode major:

unzip -p "$jar" legacy/g7/commands/HelloG7PrecompiledCommand.class | od -An -t u1 -j 6 -N 2
# expect major 61 for Java 17

Stamp the JDK into the manifest at jar time the same way Grails/Groovy are, and assert it here.

[jamesfredley]

These path filters are now the only thing that decides whether this suite runs at all - the root build no longer reaches these projects, so nothing else will catch a break. They are missing inputs the build genuinely consumes:

• gradle/** - end-to-end/legacy-commands/build.gradle applies ../gradle/grails-extension-gradle-config.gradle directly
• build-logic/** - end-to-end/settings.gradle includes it, and the projects apply its org.apache.grails.buildsrc.* plugins
• gradle-bootstrap/** - it generates this build's wrappers, including the fixture's via legacyG7Wrapper
• dependencies.gradle / grails-bom/** - the suite resolves org.apache.grails:grails-bom:$projectVersion from build/local-maven
• gradle.properties and the root .sdkmanrc - the workflow reads the latter to pick its JDK

A PR touching only those merges without this suite ever running. Same list applies to the push: filters above.

[jamesfredley]

check alone leaves the moved sources outside the project's violation gate. end-to-end applies neither GrailsCodeStylePlugin nor GrailsCodeAnalysisPlugin, and the root aggregateStyleViolations / aggregateAnalysisViolations only walk root subprojects - which these no longer are. So CodeNarc/Checkstyle/PMD/SpotBugs no longer see legacy-commands, legacy-commands-plugin, or the fixture, and ./gradlew clean aggregateViolations at the root will report clean regardless of what is in them.

That is a real regression from the move rather than a pre-existing gap: before this PR these projects were in that graph. Either apply the style/analysis convention plugins in end-to-end/build.gradle and run ./gradlew check codeStyle here, or state in end-to-end/README.md that the suite is deliberately outside the violation surface and why.

[jamesfredley]

Self-documented as temporary - flagging it only so it does not ride along. Once this merges into feat/8.0.x-legacy-command-compat and that merges to 8.0.x, drop the branch entry. Worth noting the pull_request trigger already covers the visibility this was added for, so it may be droppable now.

[jamesfredley]

This is the comment class the PR description says it corrected, and this one is inverted: "here includeBuild('..') substitutes the org.apache.grails coordinates instead" is exactly what this build does not do. end-to-end/settings.gradle deliberately has no includeBuild('..') - it resolves org.apache.grails from build/local-maven through exclusiveContent, which is the whole reason the suite is end-to-end and the reason the CLI companion capability problem goes away.

A future reader taking this comment at face value would conclude composite substitution is in play and reason about the fixture's isolation completely backwards. Suggest: "...which only makes sense inside the core build; here the org.apache.grails coordinates resolve from the published artifacts in build/local-maven instead."

[jamesfredley]

This test does not exercise the case its name claims, and it passes with the production change reverted.

useFactoryResources only swaps the thread context classloader (a URLClassLoader over the generated jars, parented to the test's own loader). The registry's other loader is ApplicationContextCommandRegistry.classLoader, i.e. the plain test classpath, which never sees shared-counting-command.jar. So only one of the two scans ever finds the declaration, and constructorCalls == 1 holds under the old per-classloader instantiate-as-you-go code just as well as under the new collect-then-instantiate code.

That is unfortunate, because avoiding the double construction is the entire motivation for restructuring ApplicationContextCommandRegistry. To make it bite, the class has to be reachable from both loaders - e.g. put the factories resource on the registry loader too (the command class itself is already on the test classpath, so both scans would resolve the same Class), then assert constructorCalls == 1. Reverting to the old code should then produce 2.

The sibling at :80 has the same limitation, but it at least got stricter with this change (dropping the registryClassLoaderConstructions baseline), so it is only this new one that is asserting nothing.

[jamesfredley]

Two things about how this warning is captured.

It asserts the message but not the cause. The point of the GrailsFactoriesLoader change is that a malformed resource stops vanishing and says why - the IllegalArgumentException("Malformed \\uxxxx encoding") is the actionable half for the user chasing missing commands. Dropping the throwable argument from the log.warn would leave all three of these assertions green. Worth asserting the exception type or its message text too.

Swapping global System.err is not safe here. captureStandardError mutates process-global state, and this module runs tests with maxParallelForks > 1 - a concurrently executing feature in the same fork writing to stderr lands in this buffer, and anything this one emits vanishes from the other's. It also couples the assertion to logback's console appender configuration rather than to the log event. A logback ListAppender attached to the GrailsFactoriesLoader logger (and detached in cleanup) tests the actual contract and is fork-safe. LegacyCommandRegistryLoadingSpec already has attachProviderAppender doing exactly this - same approach would work here.

[jamesfredley]

This drops the ThreadDeath rethrow I asked for on #16011, and the same deletion lands in ApplicationCommandDiagnostics, LegacyApplicationCommandProvider and ApplicationContextCommandFactory, with the eight tests that fed new ThreadDeath() switched to OutOfMemoryError.

I am fine with it on the merits - Thread.stop() throws UnsupportedOperationException on the Java 21 baseline, so ThreadDeath can no longer be thrown by the JVM, the branch is unreachable, and the type is deprecated for removal. VirtualMachineError, the guard that actually fires, is untouched at all four boundaries, which is the part that mattered.

But it narrows a guard that was added at review request, and the description did not mention it - I only found it by reading the diff. I have added it to the PR body. Nothing further needed here.

[jamesfredley]

Good change, and a real behavioural fix rather than a refactor: collecting Class -> origin across both loaders before instantiating anything means a command reachable through both is constructed once instead of twice-then-discarded, which for a command with constructor side effects was an observable bug. Folding instantiateCommand and instantiate into one generified instantiate(Class<? extends T>) is the right cleanup alongside it.

Same for the Ordered propagation in LegacyApplicationCommandAdapter - registration is first-wins, so two Grails 7 plugins shipping the same command name were previously resolved by jar scan order, and a G7 command that used Spring ordering to win a collision now keeps winning it.

My only issue is that neither appeared in the PR description, which read as if this PR were build placement plus four small fixes. I have added both to the body. The Ordered branch also needs its adapter spec updated - see the top-level comment.

[jdaugherty]

@jamesfredley I believe these are all addressed.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: 1ed1a30
▶️ Tests: 27823 executed
⚪️ Checks: 57/57 completed

---

Learn more about TestLens at testlens.app.

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Moves Grails 7 legacy-command compatibility coverage out of the root build and into a dedicated end-to-end Gradle build that resolves published artifacts, while improving command discovery robustness and related test coverage.

Changes:

• Introduces a new end-to-end/ Gradle build (with workflow) that consumes build/local-maven artifacts and builds a Grails 7 fixture under JDK 17.
• Updates command discovery to instantiate each command class once across classloaders, adds deterministic ordering for legacy adapters, and improves factory-resource failure visibility.
• Adds/adjusts tests for trait-derived command names, skipBootstrap resolution, ordering behavior, and fatal-error guard behavior.

Reviewed changes

Copilot reviewed 36 out of 51 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
settings.gradle	Removes legacy command example projects from the root composite, preventing JDK/toolchain leakage into the main build.
grails-test-examples/legacy-g7-command-plugin/src/main/resources/META-INF/grails.factories	Registers an additional precompiled command to cover trait-derived naming.
grails-test-examples/legacy-g7-command-plugin/settings.gradle	Appears to contain a Groovy command class instead of Gradle settings (likely incorrect file/content).
grails-test-examples/legacy-g7-command-plugin/build.gradle	Removes toolchain usage and stamps build JDK info into the jar manifest for CI verification.
grails-test-examples/legacy-commands/src/integration-test/groovy/legacycommands/LegacyCommandCompatibilityIntegrationSpec.groovy	Adds end-to-end coverage for derived-name commands and loosens strict version pin assertions.
grails-test-examples/legacy-commands/build.gradle	Switches to published BOM/artifacts, consumes the Grails 7 fixture as a prebuilt jar, and removes root-only substitution config.
grails-test-examples/legacy-commands-plugin/build.gradle	Switches to published BOM and removes root-only substitution config usage.
grails-shell-cli/src/test/groovy/org/grails/cli/profile/commands/factory/ApplicationContextCommandFactorySpec.groovy	Updates fatal-error coverage to use OutOfMemoryError instead of ThreadDeath.
grails-shell-cli/src/test/groovy/org/grails/cli/profile/AbstractProfileSpec.groovy	Adds tests ensuring unknown-command messaging appends (or does not append) the legacy hint.
grails-shell-cli/src/main/groovy/org/grails/cli/profile/commands/factory/ApplicationContextCommandFactory.groovy	Removes unreachable ThreadDeath fatal-guard branch.
grails-forge/grails-forge-cli/src/test/groovy/org/grails/forge/cli/CommandSpec.groovy	Increases timeouts and improves early-failure diagnostics for generated app builds.
grails-core/src/test/groovy/org/apache/grails/core/cli/ApplicationCommandProviderSpec.groovy	Adds classloader-cross-scan instantiation test, improves malformed-resource reporting assertions, and updates fatal-error inputs.
grails-core/src/main/groovy/org/grails/core/io/support/GrailsFactoriesLoader.groovy	Logs a warning when a factory resource is malformed/unreadable instead of silently skipping.
grails-core/src/cli/groovy/org/apache/grails/core/cli/ApplicationContextCommandRegistry.groovy	Refactors discovery to collect classes across loaders first, instantiate once, and keep deterministic ordering; removes ThreadDeath guard.
grails-core/src/cli/groovy/org/apache/grails/core/cli/ApplicationCommandDiagnostics.groovy	Removes unreachable ThreadDeath fatal-guard branch.
grails-core-cli-legacy/src/test/groovy/org/apache/grails/core/cli/compat/LegacyApplicationCommandAdapterSpec.groovy	Adds tests for order propagation (Ordered / @Order) and default ordering.
grails-core-cli-legacy/src/test/groovy/org/apache/grails/core/cli/LegacyCommandRegistryLoadingSpec.groovy	Adds tests for deterministic/ordered legacy collision resolution and updates fatal-error inputs.
grails-core-cli-legacy/src/main/groovy/org/apache/grails/core/cli/compat/LegacyApplicationCommandProvider.groovy	Sorts legacy adapters deterministically and by Spring order before first-wins registration; removes ThreadDeath guard.
grails-core-cli-legacy/src/main/groovy/org/apache/grails/core/cli/compat/LegacyApplicationCommandAdapter.groovy	Implements Ordered and resolves order from legacy command (Ordered preferred over @Order).
grails-console/src/test/groovy/grails/ui/command/GrailsApplicationContextCommandRunnerSpec.groovy	Adds tests for resolveSkipBootstrap, including adapter-target behavior.
grails-console/src/main/groovy/grails/ui/command/GrailsApplicationContextCommandRunner.groovy	Extracts resolveSkipBootstrap helper and ensures it reads from the autowire target (adapter target) and enforces Boolean type.
gradle-bootstrap/build.gradle	Adds a dedicated legacyG7Wrapper task to generate the fixture wrapper from its own .sdkmanrc, and bootstraps end-to-end wrapper files.
end-to-end/settings.gradle	New independent build that resolves org.apache.grails* exclusively from build/local-maven and includes end-to-end projects.
end-to-end/legacy-g7-command-plugin/settings.gradle	New settings for the standalone Grails 7 fixture build (Develocity, cache, plugin repos).
end-to-end/legacy-g7-command-plugin/gradlew.bat	Adds a dedicated wrapper script for the standalone fixture build.
end-to-end/legacy-g7-command-plugin/gradlew	Adds a dedicated wrapper script for the standalone fixture build.
end-to-end/legacy-g7-command-plugin/gradle/wrapper/gradle-wrapper.properties	Pins the fixture build’s Gradle wrapper distribution.
end-to-end/legacy-g7-command-plugin/.sdkmanrc	Pins JDK 17 and Gradle for the Grails 7 fixture build.
end-to-end/gradlew.bat	Adds wrapper script for the end-to-end build.
end-to-end/gradlew	Adds wrapper script for the end-to-end build.
end-to-end/gradle/wrapper/gradle-wrapper.properties	Pins the end-to-end build’s Gradle wrapper distribution.
end-to-end/gradle.properties	Sets snapshot version and Gradle build settings for the end-to-end build.
end-to-end/build.gradle	Disables caching for changing/dynamic versions and enforces local-maven publish precondition before compiling.
end-to-end/README.md	Documents purpose, setup, local run steps, and rationale for avoiding composite substitution.
AGENTS.md	Adds end-to-end build to the list of independent Gradle projects with run instructions.
.github/workflows/end-to-end.yml	New workflow that provisions two JDKs, publishes artifacts to build/local-maven, builds/verifies the fixture, then runs end-to-end check.

Comments suppressed due to low confidence (1)

gradle-bootstrap/build.gradle:1

• legacyG7Wrapper customizes scriptFile and jarFile but does not redirect the wrapper propertiesFile (and likely the Windows batch script target as well). As written, Gradle will still generate gradle/wrapper/gradle-wrapper.properties (and gradlew.bat) under gradle-bootstrap/, leaving the fixture directory potentially missing or stale wrapper metadata. Configure the wrapper task to write all wrapper outputs into end-to-end/legacy-g7-command-plugin (POSIX script, Windows script, wrapper jar, and wrapper properties).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.