matrei commented on PR #15805: URL: https://github.com/apache/grails-core/pull/15805#issuecomment-5874851334
Thanks @jdaugherty. I reviewed `caf108c684` and the merge of `8.0.x` on top of it (`a504c33f8c`). Points 2 to 4 and the inline comments are all resolved. I checked each fix below. On your question about point 1: no, I don't want to block this PR on the upstream behaviour. What I'd like before merge is the one troubleshooting sentence in our own guide. The guide describes both setups that end up in this silent state: - `agentSkills.adoc:73-82`: a build without the Grails Gradle plugin, which has to add the `platform` line itself - `agentSkills.adoc:96`: "Once the application is on Grails 8, the version can be dropped" A reader who misses the `platform` line, or drops the version too early, gets `Found 0 SkillsJar(s)`, a successful build and an emptied directory. Whoever's bug that is, it's our guide they are following. Something like this after the WARNING at `:65` would cover it: ```asciidoc If `extractSkillsJars` reports `Found 0 SkillsJar(s)`, the `skill` dependencies did not resolve; `./gradlew dependencies --configuration skill` shows why. ``` The upstream issue can come after the merge. I can open it if you'd like; nothing is filed there yet, and 0.1.3 is still the latest release. Run locally at `a504c33f8c`: | What | Result | |---|---| | `AgentSkillsPluginSpec` (`build-logic`, `--no-build-cache --rerun-tasks`) | 12/12 pass | | full `publishAllPublicationsToTestCaseMavenRepoRepository` (grails-gradle + core), then `end-to-end :agent-skills:test` with `DO_NOT_CACHE_TESTS=1 --rerun-tasks` | 8/8 pass, including the new link check; both builds report `Found 2 SkillsJar(s)` and extract the reworked `SKILL.md` text | | Every doc URL in the upgrade skill's source table, against `8.0.0-RC1` and `snapshot` | all return 200, and every anchor exists on the page | | `.agents/skills/*` and `.claude/skills/*/SKILL.md` links | all resolve | ### Minor 1. **Gradle line in `grails-developer`** (`grails-skills/developer/skills/grails-developer/SKILL.md:37`). The list is headed "Grails is built on:" and now says `9.7.x`. Since #16403 merged into this branch, the build itself uses 9.8.0, so the line reads as out of date. `9.7 or later` would match the upgrade skill and the upgrade guide's section 1.1, and it would not go stale with the next Gradle bump. 2. **Generated Micronaut anchor** (`grails-8-upgrade/SKILL.md:40`). `#_7_micronaut_integration` is generated from the section number "7.", so inserting a section before it in `upgrading80x.adoc` breaks the link without any error. The other rows use stable ids. This PR already touches `upgrading80x.adoc`, and the file already uses explicit anchors such as `[[jackson3-default]]`. An explicit `[[micronaut-integration]]` on that heading, with the skill pointing at it, would make the link stable. The published RC1 page would not have the new id, but the skill points agents at the version they are upgrading to, which will have it. ### Verified as correct - **Undertow.** The new text matches `SpringBootUndertowFeature` in Forge (`implementation 'org.apache.grails:grails-undertow'`, no Tomcat starter). It also matches `UndertowServerProperties`, where `maxHttpPostSize` defaults to `DataSize.ofMegabytes(2)` and a non-positive value maps to unlimited. - **Gradle line in the upgrade skill.** "Gradle 9.7 or later, which the Grails 8 Gradle plugins require" matches `upgrading80x.adoc` section 1.1. - **Micronaut section of the upgrade skill.** Grails Micronaut moved to `apache/grails-micronaut` in #16320, so I checked the section against that repository. The BOM coordinates, the `enforcedPlatform` requirement and the JDK 25 requirement match its README and `docs/micronaut.adoc`. - **Directory links.** `.agents/skills/grails-developer` and `grails-8-upgrade` are directory links. `.claude/skills/grails-developer/SKILL.md` still resolves through them. The new `AgentSkillsSpec` feature checks the canonical path, so a link replaced by a copy would fail it. - **Frontmatter validation.** The data table covers a missing frontmatter, a missing `name`, a missing or blank `description`, and a `description:` line after the closing `---`. Each case checks for `InvalidUserDataException`. - **Plugin version note.** `agentSkills.adoc:54` matches what I found upstream: before 0.1.0 only `com.skillsjars` artifacts are extracted, and 0.1.0 to 0.1.2 hit skillsjars-gradle-plugin#10. -- 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]
