jamesfredley commented on code in PR #16025: URL: https://github.com/apache/grails-core/pull/16025#discussion_r3677417923
########## build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/RepositoryConventionsTask.groovy: ########## @@ -0,0 +1,474 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.buildsrc + +import groovy.transform.CompileStatic + +import org.gradle.api.DefaultTask +import org.gradle.api.GradleException +import org.gradle.api.file.ConfigurableFileCollection +import org.gradle.api.file.DirectoryProperty +import org.gradle.api.file.RegularFileProperty +import org.gradle.api.tasks.InputFiles +import org.gradle.api.tasks.Internal +import org.gradle.api.tasks.OutputFile +import org.gradle.api.tasks.PathSensitive +import org.gradle.api.tasks.PathSensitivity +import org.gradle.api.tasks.TaskAction +import org.yaml.snakeyaml.LoaderOptions +import org.yaml.snakeyaml.Yaml +import org.yaml.snakeyaml.constructor.SafeConstructor +import org.yaml.snakeyaml.error.YAMLException + +import java.nio.charset.StandardCharsets +import java.util.Set +import java.util.regex.Matcher +import java.util.regex.Pattern + +@CompileStatic +abstract class RepositoryConventionsTask extends DefaultTask { + + private static final Pattern AGENT_SKILL_PATH = Pattern.compile(/\.agents\/skills\/[A-Za-z0-9_-]+\/SKILL\.md/) + private static final Pattern COMMIT_SHA = Pattern.compile(/^[0-9a-f]{40}$/) + private static final Pattern DOCKER_IMAGE_DIGEST = Pattern.compile(/^docker:\/\/[^@\s]+@sha256:[0-9a-f]{64}$/) + private static final Pattern CONTAINER_IMAGE_DIGEST = Pattern.compile(/^[^@\s]+@sha256:[0-9a-f]{64}$/) + + @Internal + abstract DirectoryProperty getRepositoryDirectory() + + @InputFiles + @PathSensitive(PathSensitivity.RELATIVE) + abstract ConfigurableFileCollection getConventionSources() + + @OutputFile + abstract RegularFileProperty getReportFile() + + @TaskAction + void validateRepositoryConventions() { + File root = repositoryDirectory.get().asFile + List<File> files = conventionSources.files.toList() + List<String> violations = [] + validateSkills(root, files, violations) + validateActions(root, files, violations) + validateProperties(root, files, violations) + writeReport(violations) + if (!violations.isEmpty()) { + List<String> safeViolations = violations.collect { String violation -> sanitizeViolation(violation) } + throw new GradleException("Repository convention violations:\n - ${safeViolations.join('\n - ')}\nSee ${reportFile.get().asFile}") + } + } + + private static void validateSkills(File root, List<File> files, List<String> violations) { + List<File> skills = files.findAll { relativePath(root, it) ==~ /^\.agents\/skills\/[^\/]+\/SKILL\.md$/ }.sort() + Map<String, File> names = [:] + Set<String> canonicalPaths = [] + skills.each { File skill -> + String path = relativePath(root, skill) + String directoryName = skill.parentFile.name + Map<String, String> metadata = frontMatter(skill, path, violations) + ['name', 'description', 'license'].each { String key -> + if (!metadata[key]) { + violations.add("${path}: skill front matter is missing '${key}'".toString()) + } + } + String name = metadata['name'] + if (name && name != directoryName) { + violations.add("${path}: skill name '${name}' does not match directory '${directoryName}'".toString()) + } + if (name && names.containsKey(name)) { + violations.add("${path}: skill name '${name}' duplicates ${relativePath(root, names[name])}".toString()) + } else if (name) { + names[name] = skill + } + canonicalPaths << path + } + + File agents = new File(root, 'AGENTS.md') + if (!agents.isFile()) { + violations << 'AGENTS.md: file is missing' + return + } + Set<String> documentedPaths = [] + Matcher matcher = AGENT_SKILL_PATH.matcher(agents.text) + while (matcher.find()) { + documentedPaths << matcher.group() + } + canonicalPaths.each { String path -> + if (!documentedPaths.contains(path)) { + violations.add("AGENTS.md: missing canonical skill path '${path}'".toString()) + } + } + documentedPaths.each { String path -> + if (!new File(root, path).isFile()) { + violations.add("AGENTS.md: skill path '${path}' does not exist".toString()) + } + } + } + + private static Map<String, String> frontMatter(File skill, String path, List<String> violations) { + List<String> lines = skill.readLines() + if (lines.isEmpty() || lines[0] != '---') { + return [:] + } + int end = -1 + for (int index = 1; index < lines.size(); index++) { + if (lines[index] == '---') { + end = index + break + } + } + if (end < 0) { + return [:] + } + Object document + try { + LoaderOptions options = new LoaderOptions() + options.setAllowDuplicateKeys(false) + document = new Yaml(new SafeConstructor(options)).load(lines.subList(1, end).join('\n')) + } catch (YAMLException exception) { + violations.add("${path}: malformed skill front matter: ${exception.message}".toString()) + return [:] + } + if (!(document instanceof Map)) { + violations.add("${path}: skill front matter must be a YAML mapping".toString()) + return [:] + } + Map<String, String> values = [:] + ['name', 'description', 'license'].each { String key -> + Object value = ((Map<?, ?>) document).get(key) + if (value instanceof String) { + values[key] = (String) value + } else if (value != null) { + violations.add("${path}: skill front matter field '${key}' must be a string".toString()) + } + } + values + } + + private static void validateActions(File root, List<File> files, List<String> violations) { + Map<String, String> actionShas = [:] + Map<String, String> actionFiles = [:] + Set<String> validatedManifests = [] + files.findAll { File file -> isActionManifest(root, file) }.sort().each { File manifest -> + validateActionManifest(root, manifest, actionShas, actionFiles, violations, validatedManifests) + } + } + + private static void validateActionManifest(File root, File manifest, Map<String, String> actionShas, + Map<String, String> actionFiles, List<String> violations, Set<String> validatedManifests) { + String canonicalPath = manifest.canonicalPath + if (!validatedManifests.add(canonicalPath)) { + return + } + String path = relativePath(root, manifest) + Object document = parseYaml(manifest, path, violations) + if (document != null) { + validateDockerActionImage(document, path, violations) + if (isWorkflowManifest(root, manifest)) { + validateWorkflowContainerImages(document, path, violations) + validateWorkflowUses(root, document, path, actionShas, actionFiles, violations, validatedManifests) + } else { + validateCompositeActionUses(root, document, path, actionShas, actionFiles, violations, validatedManifests) + } + } + } + + private static boolean isActionManifest(File root, File file) { + String path = relativePath(root, file) + isWorkflowManifest(root, file) || + path ==~ /(?:^|.*\/)action\.ya?ml$/ + } + + private static boolean isWorkflowManifest(File root, File file) { + relativePath(root, file) ==~ /^\.github\/workflows\/[^\/]+\.ya?ml$/ + } + + private static Object parseYaml(File manifest, String path, List<String> violations) { + try { + LoaderOptions options = new LoaderOptions() + options.setAllowDuplicateKeys(false) + new Yaml(new SafeConstructor(options)).load(manifest.text) + } catch (YAMLException exception) { + violations.add("${path}: malformed YAML: ${exception.message}".toString()) + null + } + } + + private static void validateDockerActionImage(Object document, String path, List<String> violations) { + if (!(document instanceof Map)) { + return + } + Object runs = ((Map<?, ?>) document).get('runs') + Object using = runs instanceof Map ? ((Map<?, ?>) runs).get('using') : null + if (!(using instanceof String) || !((String) using).equalsIgnoreCase('docker')) { + return + } + Object image = ((Map<?, ?>) runs).get('image') + String location = '$.runs.image' + if (!(image instanceof String)) { + violations.add("${path}:${location}: Docker action image must be a string".toString()) + } else if (((String) image).regionMatches(true, 0, 'docker://', 0, 'docker://'.length()) && !DOCKER_IMAGE_DIGEST.matcher((String) image).matches()) { + violations.add("${path}:${location}: Docker action image '${image}' must use an immutable sha256 digest".toString()) + } + } + + private static void validateWorkflowContainerImages(Object document, String path, List<String> violations) { + if (!(document instanceof Map)) { + return + } + Object jobs = ((Map<?, ?>) document).get('jobs') + if (!(jobs instanceof Map)) { + return + } + ((Map<?, ?>) jobs).each { Object jobName, Object job -> + if (!(job instanceof Map)) { + return + } + String jobLocation = "\$.jobs.${jobName}" + Map<?, ?> jobDefinition = (Map<?, ?>) job + if (jobDefinition.containsKey('container')) { + Object container = jobDefinition.get('container') + if (container instanceof Map) { + validateContainerImage(((Map<?, ?>) container).get('image'), "${jobLocation}.container.image", path, violations) + } else { + validateContainerImage(container, "${jobLocation}.container", path, violations) + } + } + Object services = jobDefinition.get('services') + if (services instanceof Map) { + ((Map<?, ?>) services).each { Object serviceName, Object service -> + if (service instanceof Map && ((Map<?, ?>) service).containsKey('image')) { + validateContainerImage(((Map<?, ?>) service).get('image'), "${jobLocation}.services.${serviceName}.image", path, + violations) + } + } + } + } + } + + private static void validateContainerImage(Object image, String location, String path, List<String> violations) { + if (!(image instanceof String)) { + violations.add("${path}:${location}: container image must be a string".toString()) + } else if (!CONTAINER_IMAGE_DIGEST.matcher((String) image).matches()) { + violations.add("${path}:${location}: container image '${image}' must use an immutable sha256 digest".toString()) + } + } + + private static void validateWorkflowUses(File root, Object document, String path, Map<String, String> actionShas, + Map<String, String> actionFiles, List<String> violations, Set<String> validatedManifests) { + if (!(document instanceof Map)) { + return + } + Map<?, ?> workflow = (Map<?, ?>) document + validateStepUses(root, workflow.get('steps'), '$.steps', path, actionShas, actionFiles, violations, validatedManifests) + Object jobs = workflow.get('jobs') + if (!(jobs instanceof Map)) { + return + } + ((Map<?, ?>) jobs).each { Object jobName, Object job -> + if (!(job instanceof Map)) { + return + } + Map<?, ?> jobDefinition = (Map<?, ?>) job + String jobLocation = "\$.jobs.${jobName}" + if (jobDefinition.containsKey('uses')) { + validateActionUse(root, jobDefinition.get('uses'), "${jobLocation}.uses", path, actionShas, actionFiles, violations, + validatedManifests) + } + validateStepUses(root, jobDefinition.get('steps'), "${jobLocation}.steps", path, actionShas, actionFiles, violations, + validatedManifests) + } + } + + private static void validateCompositeActionUses(File root, Object document, String path, Map<String, String> actionShas, + Map<String, String> actionFiles, List<String> violations, Set<String> validatedManifests) { + if (!(document instanceof Map)) { + return + } + Object runs = ((Map<?, ?>) document).get('runs') + if (runs instanceof Map) { + validateStepUses(root, ((Map<?, ?>) runs).get('steps'), '$.runs.steps', path, actionShas, actionFiles, violations, + validatedManifests) + } + } + + private static void validateStepUses(File root, Object steps, String location, String path, Map<String, String> actionShas, + Map<String, String> actionFiles, List<String> violations, Set<String> validatedManifests) { + if (!(steps instanceof Iterable)) { + return + } + int index = 0 + ((Iterable<?>) steps).each { Object step -> + if (step instanceof Map && ((Map<?, ?>) step).containsKey('uses')) { + validateActionUse(root, ((Map<?, ?>) step).get('uses'), "${location}[${index}].uses", path, actionShas, actionFiles, + violations, validatedManifests) + } + index++ + } + } + + private static void validateActionUse(File root, Object value, String location, String path, Map<String, String> actionShas, + Map<String, String> actionFiles, List<String> violations, Set<String> validatedManifests) { + if (!(value instanceof String)) { + violations.add("${path}:${location}: 'uses' must be a string".toString()) + return + } + String use = (String) value + if (use.startsWith('./')) { + validateLocalAction(root, use, location, path, actionShas, actionFiles, violations, validatedManifests) + return + } + if (use.startsWith('docker://')) { + if (!DOCKER_IMAGE_DIGEST.matcher(use).matches()) { + violations.add("${path}:${location}: Docker action '${use}' must use an immutable sha256 digest".toString()) + } + return + } + int separator = use.lastIndexOf('@') + if (separator <= 0 || separator == use.length() - 1) { + violations.add("${path}:${location}: action '${use}' must use a lowercase 40-hex commit SHA".toString()) + return + } + String action = use.substring(0, separator) + String sha = use.substring(separator + 1) + if (!COMMIT_SHA.matcher(sha).matches()) { + violations.add("${path}:${location}: action '${action}' uses '${sha}', not a lowercase 40-hex commit SHA".toString()) + } else if (actionShas.containsKey(action) && actionShas[action] != sha) { + violations.add("${path}:${location}: action '${action}' uses ${sha}, inconsistent with ${actionShas[action]} in ${actionFiles[action]}".toString()) + } else { + actionShas[action] = sha + actionFiles[action] = path + } + } + + private static void validateLocalAction(File root, String use, String location, String path, + Map<String, String> actionShas, Map<String, String> actionFiles, List<String> violations, + Set<String> validatedManifests) { + File canonicalRoot = root.canonicalFile + File target = new File(root, use.substring(2)).canonicalFile Review Comment: Fixed. The repository root is canonicalized once in `validateRepositoryConventions` and the single canonical `File` is threaded through `relativePath`, `isWorkflowManifest`, `isActionManifest`, and `validateLocalAction`. `relativePath` also canonicalizes the file before relativizing, so a symlink component in the checkout path can no longer produce a `../..` result, misclassify a locally referenced workflow as a composite action, and then have `validatedManifests` dedup-skip the file when the outer loop reaches it. Latent as you say, but it is the designated local-action path, so it is worth being correct. The direct-file branch now has spec coverage too, in the thread below. ########## build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/RepositoryConventionsTask.groovy: ########## @@ -0,0 +1,474 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.buildsrc + +import groovy.transform.CompileStatic + +import org.gradle.api.DefaultTask +import org.gradle.api.GradleException +import org.gradle.api.file.ConfigurableFileCollection +import org.gradle.api.file.DirectoryProperty +import org.gradle.api.file.RegularFileProperty +import org.gradle.api.tasks.InputFiles +import org.gradle.api.tasks.Internal +import org.gradle.api.tasks.OutputFile +import org.gradle.api.tasks.PathSensitive +import org.gradle.api.tasks.PathSensitivity +import org.gradle.api.tasks.TaskAction +import org.yaml.snakeyaml.LoaderOptions +import org.yaml.snakeyaml.Yaml +import org.yaml.snakeyaml.constructor.SafeConstructor +import org.yaml.snakeyaml.error.YAMLException + +import java.nio.charset.StandardCharsets +import java.util.Set +import java.util.regex.Matcher +import java.util.regex.Pattern + +@CompileStatic +abstract class RepositoryConventionsTask extends DefaultTask { + + private static final Pattern AGENT_SKILL_PATH = Pattern.compile(/\.agents\/skills\/[A-Za-z0-9_-]+\/SKILL\.md/) + private static final Pattern COMMIT_SHA = Pattern.compile(/^[0-9a-f]{40}$/) + private static final Pattern DOCKER_IMAGE_DIGEST = Pattern.compile(/^docker:\/\/[^@\s]+@sha256:[0-9a-f]{64}$/) + private static final Pattern CONTAINER_IMAGE_DIGEST = Pattern.compile(/^[^@\s]+@sha256:[0-9a-f]{64}$/) + + @Internal + abstract DirectoryProperty getRepositoryDirectory() + + @InputFiles + @PathSensitive(PathSensitivity.RELATIVE) + abstract ConfigurableFileCollection getConventionSources() + + @OutputFile + abstract RegularFileProperty getReportFile() + + @TaskAction + void validateRepositoryConventions() { + File root = repositoryDirectory.get().asFile + List<File> files = conventionSources.files.toList() + List<String> violations = [] + validateSkills(root, files, violations) + validateActions(root, files, violations) + validateProperties(root, files, violations) + writeReport(violations) + if (!violations.isEmpty()) { + List<String> safeViolations = violations.collect { String violation -> sanitizeViolation(violation) } + throw new GradleException("Repository convention violations:\n - ${safeViolations.join('\n - ')}\nSee ${reportFile.get().asFile}") + } + } + + private static void validateSkills(File root, List<File> files, List<String> violations) { + List<File> skills = files.findAll { relativePath(root, it) ==~ /^\.agents\/skills\/[^\/]+\/SKILL\.md$/ }.sort() + Map<String, File> names = [:] + Set<String> canonicalPaths = [] + skills.each { File skill -> + String path = relativePath(root, skill) + String directoryName = skill.parentFile.name + Map<String, String> metadata = frontMatter(skill, path, violations) + ['name', 'description', 'license'].each { String key -> + if (!metadata[key]) { + violations.add("${path}: skill front matter is missing '${key}'".toString()) + } + } + String name = metadata['name'] + if (name && name != directoryName) { + violations.add("${path}: skill name '${name}' does not match directory '${directoryName}'".toString()) + } + if (name && names.containsKey(name)) { + violations.add("${path}: skill name '${name}' duplicates ${relativePath(root, names[name])}".toString()) + } else if (name) { + names[name] = skill + } + canonicalPaths << path + } + + File agents = new File(root, 'AGENTS.md') + if (!agents.isFile()) { + violations << 'AGENTS.md: file is missing' + return + } + Set<String> documentedPaths = [] + Matcher matcher = AGENT_SKILL_PATH.matcher(agents.text) + while (matcher.find()) { + documentedPaths << matcher.group() + } + canonicalPaths.each { String path -> + if (!documentedPaths.contains(path)) { + violations.add("AGENTS.md: missing canonical skill path '${path}'".toString()) + } + } + documentedPaths.each { String path -> + if (!new File(root, path).isFile()) { + violations.add("AGENTS.md: skill path '${path}' does not exist".toString()) + } + } + } + + private static Map<String, String> frontMatter(File skill, String path, List<String> violations) { + List<String> lines = skill.readLines() + if (lines.isEmpty() || lines[0] != '---') { + return [:] + } + int end = -1 + for (int index = 1; index < lines.size(); index++) { + if (lines[index] == '---') { + end = index + break + } + } + if (end < 0) { + return [:] + } + Object document + try { + LoaderOptions options = new LoaderOptions() + options.setAllowDuplicateKeys(false) + document = new Yaml(new SafeConstructor(options)).load(lines.subList(1, end).join('\n')) + } catch (YAMLException exception) { + violations.add("${path}: malformed skill front matter: ${exception.message}".toString()) + return [:] + } + if (!(document instanceof Map)) { + violations.add("${path}: skill front matter must be a YAML mapping".toString()) + return [:] + } + Map<String, String> values = [:] + ['name', 'description', 'license'].each { String key -> + Object value = ((Map<?, ?>) document).get(key) + if (value instanceof String) { + values[key] = (String) value + } else if (value != null) { + violations.add("${path}: skill front matter field '${key}' must be a string".toString()) + } + } + values + } + + private static void validateActions(File root, List<File> files, List<String> violations) { + Map<String, String> actionShas = [:] + Map<String, String> actionFiles = [:] + Set<String> validatedManifests = [] + files.findAll { File file -> isActionManifest(root, file) }.sort().each { File manifest -> + validateActionManifest(root, manifest, actionShas, actionFiles, violations, validatedManifests) + } + } + + private static void validateActionManifest(File root, File manifest, Map<String, String> actionShas, + Map<String, String> actionFiles, List<String> violations, Set<String> validatedManifests) { + String canonicalPath = manifest.canonicalPath + if (!validatedManifests.add(canonicalPath)) { + return + } + String path = relativePath(root, manifest) + Object document = parseYaml(manifest, path, violations) + if (document != null) { + validateDockerActionImage(document, path, violations) + if (isWorkflowManifest(root, manifest)) { + validateWorkflowContainerImages(document, path, violations) + validateWorkflowUses(root, document, path, actionShas, actionFiles, violations, validatedManifests) + } else { + validateCompositeActionUses(root, document, path, actionShas, actionFiles, violations, validatedManifests) + } + } + } + + private static boolean isActionManifest(File root, File file) { + String path = relativePath(root, file) + isWorkflowManifest(root, file) || + path ==~ /(?:^|.*\/)action\.ya?ml$/ + } + + private static boolean isWorkflowManifest(File root, File file) { + relativePath(root, file) ==~ /^\.github\/workflows\/[^\/]+\.ya?ml$/ + } + + private static Object parseYaml(File manifest, String path, List<String> violations) { + try { + LoaderOptions options = new LoaderOptions() + options.setAllowDuplicateKeys(false) + new Yaml(new SafeConstructor(options)).load(manifest.text) + } catch (YAMLException exception) { + violations.add("${path}: malformed YAML: ${exception.message}".toString()) + null + } + } + + private static void validateDockerActionImage(Object document, String path, List<String> violations) { + if (!(document instanceof Map)) { + return + } + Object runs = ((Map<?, ?>) document).get('runs') + Object using = runs instanceof Map ? ((Map<?, ?>) runs).get('using') : null + if (!(using instanceof String) || !((String) using).equalsIgnoreCase('docker')) { + return + } + Object image = ((Map<?, ?>) runs).get('image') + String location = '$.runs.image' + if (!(image instanceof String)) { + violations.add("${path}:${location}: Docker action image must be a string".toString()) + } else if (((String) image).regionMatches(true, 0, 'docker://', 0, 'docker://'.length()) && !DOCKER_IMAGE_DIGEST.matcher((String) image).matches()) { + violations.add("${path}:${location}: Docker action image '${image}' must use an immutable sha256 digest".toString()) + } + } + + private static void validateWorkflowContainerImages(Object document, String path, List<String> violations) { + if (!(document instanceof Map)) { + return + } + Object jobs = ((Map<?, ?>) document).get('jobs') + if (!(jobs instanceof Map)) { + return + } + ((Map<?, ?>) jobs).each { Object jobName, Object job -> + if (!(job instanceof Map)) { + return + } + String jobLocation = "\$.jobs.${jobName}" + Map<?, ?> jobDefinition = (Map<?, ?>) job + if (jobDefinition.containsKey('container')) { + Object container = jobDefinition.get('container') + if (container instanceof Map) { + validateContainerImage(((Map<?, ?>) container).get('image'), "${jobLocation}.container.image", path, violations) + } else { + validateContainerImage(container, "${jobLocation}.container", path, violations) + } + } + Object services = jobDefinition.get('services') + if (services instanceof Map) { + ((Map<?, ?>) services).each { Object serviceName, Object service -> + if (service instanceof Map && ((Map<?, ?>) service).containsKey('image')) { + validateContainerImage(((Map<?, ?>) service).get('image'), "${jobLocation}.services.${serviceName}.image", path, + violations) + } + } + } + } + } + + private static void validateContainerImage(Object image, String location, String path, List<String> violations) { + if (!(image instanceof String)) { + violations.add("${path}:${location}: container image must be a string".toString()) + } else if (!CONTAINER_IMAGE_DIGEST.matcher((String) image).matches()) { Review Comment: The strictness is intended, and you are right that it needed explaining rather than just rejecting. An expression value cannot be checked for immutability at validation time, so accepting it would mean the rule silently stops enforcing anything for matrix jobs. Rather than relax it, the rejection now explains itself with its own message: container images must be literal `name@sha256:<digest>` values because expression values cannot be verified as immutable. That is a distinct violation from the ordinary mutable-tag one, so the first author of a matrix-image job gets told why rather than being left guessing. Documented in AGENTS.md as well, and covered by a spec. If a matrix-image job actually becomes necessary, I would rather revisit it then with a concrete case in hand. ########## build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/RepositoryConventionsTask.groovy: ########## @@ -0,0 +1,474 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.buildsrc + +import groovy.transform.CompileStatic + +import org.gradle.api.DefaultTask +import org.gradle.api.GradleException +import org.gradle.api.file.ConfigurableFileCollection +import org.gradle.api.file.DirectoryProperty +import org.gradle.api.file.RegularFileProperty +import org.gradle.api.tasks.InputFiles +import org.gradle.api.tasks.Internal +import org.gradle.api.tasks.OutputFile +import org.gradle.api.tasks.PathSensitive +import org.gradle.api.tasks.PathSensitivity +import org.gradle.api.tasks.TaskAction +import org.yaml.snakeyaml.LoaderOptions +import org.yaml.snakeyaml.Yaml +import org.yaml.snakeyaml.constructor.SafeConstructor +import org.yaml.snakeyaml.error.YAMLException + +import java.nio.charset.StandardCharsets +import java.util.Set +import java.util.regex.Matcher +import java.util.regex.Pattern + +@CompileStatic +abstract class RepositoryConventionsTask extends DefaultTask { + + private static final Pattern AGENT_SKILL_PATH = Pattern.compile(/\.agents\/skills\/[A-Za-z0-9_-]+\/SKILL\.md/) + private static final Pattern COMMIT_SHA = Pattern.compile(/^[0-9a-f]{40}$/) Review Comment: Documented rather than re-pinned. There is now a comment on the `COMMIT_SHA` constant recording that a 40-hex ref may be either a commit or an annotated-tag object, that both are immutable, and that the validator cannot distinguish them. AGENTS.md says the same in the contributor-facing policy. I left `release-drafter@e1247478...` alone deliberately: it is correct and immutable today, that file's own bump checklist already documents the peeling step, and re-pinning it here would put an unrelated third-party action bump into a build-policy PR. Happy to normalize it in a follow-up if you would rather have every pin be a peeled commit. ########## build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/RepositoryConventionsTask.groovy: ########## @@ -0,0 +1,474 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.grails.buildsrc + +import groovy.transform.CompileStatic + +import org.gradle.api.DefaultTask +import org.gradle.api.GradleException +import org.gradle.api.file.ConfigurableFileCollection +import org.gradle.api.file.DirectoryProperty +import org.gradle.api.file.RegularFileProperty +import org.gradle.api.tasks.InputFiles +import org.gradle.api.tasks.Internal +import org.gradle.api.tasks.OutputFile +import org.gradle.api.tasks.PathSensitive +import org.gradle.api.tasks.PathSensitivity +import org.gradle.api.tasks.TaskAction +import org.yaml.snakeyaml.LoaderOptions +import org.yaml.snakeyaml.Yaml +import org.yaml.snakeyaml.constructor.SafeConstructor +import org.yaml.snakeyaml.error.YAMLException + +import java.nio.charset.StandardCharsets +import java.util.Set Review Comment: Fixed. `import java.util.Set` is gone and the `java.*` block now sits above the third-party imports, matching `GrailsCodeAnalysisPlugin` and the rest of build-logic. Also took the related note in the other thread and replaced the inline fully-qualified `org.gradle.api.file.FileTreeElement` with a proper import. ########## .github/workflows/codeanalysis.yml: ########## @@ -60,6 +60,24 @@ jobs: file="build/reports/violations/$report" [ -f "$file" ] && cat "$file" >> $GITHUB_STEP_SUMMARY || true done + - name: "🔎 Run Core SpotBugs Advisory" + if: always() Review Comment: Fixed in both jobs. The advisory run, upload, and summary steps now use `if: success() || failure()`, so a superseded PR run actually frees its runner instead of finishing a full advisory SpotBugs pass on a doomed job. The pre-existing `if: always()` on the enforced-report upload and summary steps is left as it was, since those predate this PR. ########## .github/workflows/codeanalysis.yml: ########## @@ -60,6 +60,24 @@ jobs: file="build/reports/violations/$report" [ -f "$file" ] && cat "$file" >> $GITHUB_STEP_SUMMARY || true done + - name: "🔎 Run Core SpotBugs Advisory" + if: always() + continue-on-error: true + run: | + rm -f build/reports/violations/SPOTBUGS_VIOLATIONS.md + ./gradlew aggregateAnalysisViolations --continue "-Pgrails.code-analysis.enabled.pmd.projects=" -Pgrails.code-analysis.enabled.spotbugs=true -Pgrails.code-analysis.ignoreFailures=true + - name: "📤 Upload SpotBugs Advisory Reports" + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: core-spotbugs-advisory-reports + path: build/reports/violations/ Review Comment: Fixed. Both advisory uploads are scoped to the single file the pass legitimately produces - `build/reports/violations/SPOTBUGS_VIOLATIONS.md` and `grails-gradle/build/reports/violations/SPOTBUGS_VIOLATIONS.md` - so the artifact can no longer contradict the enforced PMD step that ran in the same job. The related cause is gone too: the advisory pass no longer passes `-Pgrails.code-analysis.enabled.pmd.projects=` to switch PMD off, because with the per-project opt-in there is no central list to blank out. It simply runs with `ignoreFailures`, and the scoped upload keeps the artifact honest. ########## .github/workflows/codeanalysis.yml: ########## @@ -60,6 +60,24 @@ jobs: file="build/reports/violations/$report" [ -f "$file" ] && cat "$file" >> $GITHUB_STEP_SUMMARY || true done + - name: "🔎 Run Core SpotBugs Advisory" + if: always() + continue-on-error: true + run: | + rm -f build/reports/violations/SPOTBUGS_VIOLATIONS.md + ./gradlew aggregateAnalysisViolations --continue "-Pgrails.code-analysis.enabled.pmd.projects=" -Pgrails.code-analysis.enabled.spotbugs=true -Pgrails.code-analysis.ignoreFailures=true + - name: "📤 Upload SpotBugs Advisory Reports" + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: core-spotbugs-advisory-reports + path: build/reports/violations/ + - name: "📋 Publish SpotBugs Advisory Report in Job Summary" + if: always() Review Comment: Fixed, following the `vulnerability-scan.yml` pattern you pointed at. Each advisory run step now has an `id` and the summary echoes its outcome: ```yaml echo "Advisory run outcome: ${{ steps.core_spotbugs_advisory.outcome }}" >> $GITHUB_STEP_SUMMARY ``` Same for the gradle-plugin variant. A crashed advisory run now shows as `failure` in the summary instead of leaving a bare heading on a green job. ########## .github/workflows/release.yml: ########## @@ -224,7 +224,7 @@ jobs: # staged from a JDK 25 runner. This is a NEW reproducibility pin - # keep $JAVA_VERSION_MICRONAUT synced with the secondary JDK in # etc/bin/Dockerfile so verifiers can reproduce the resulting JARs. - uses: actions/setup-java@v4 + uses: actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654 # v5.2.0 Review Comment: You are right, and this is resolved by the revert. Both steps are back to `actions/setup-java@v4`, so there is no major-version crossing in the reproducibility pins at all and no need to re-verify Liberica archive equivalence for either `JAVA_VERSION` value. The v4 to v5 jump was an unintended side effect of mechanically replacing tags with the SHAs their tags currently resolve to, which is exactly the maintenance burden you describe. -- 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]
