From 4a8e30099eb6ff9ab204ae0e216d9e1b20c34017 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 25 Aug 2026 15:38:14 -0500 Subject: [PATCH] Notice if the release job loses the permission that lets it publish build.yml's `release` job declares `contents: write`, and nothing checked it. Deleting those lines leaves actionlint clean and CodeQL silent -- a narrower permission is not an alert -- and the job is `if: startsWith(github.ref, 'refs/tags/v')`, so no pull request and no merge can exercise it. Measured with the declaration removed: every gating check still passed. The first thing that would notice is a release failing to publish, at the moment someone is trying to cut one. The deletion also looks like tidying. #106 has just put a top-level `permissions: contents: read` directly above it, so a reader could reasonably take the job-level block for a duplicate. It is an override, and a comment saying so is not a check. BackupExclusionsTest is the precedent: configuration rather than code, load bearing, and unguarded because nothing compiles it. The part worth reading twice is the second commit-worth of work in here. The test passed, and then the mutation that is supposed to redden it did not: BUILD SUCCESSFUL in 614ms Gradle cannot infer that a test depends on a file outside the source set, so the task stayed UP-TO-DATE and the test never ran. Under --rerun-tasks the same mutation failed it properly, which is the tell: the assertion was right and the wiring was not. A guard that does not re-run when its subject changes is not a guard -- it is a test that will be green on the day it matters, which is worse than no test because it reads as cover. Fixed by declaring the workflow as a task input. Verified the whole way round afterwards, without --rerun-tasks: mutate the file and the task re-runs and fails; restore it and the task re-runs and passes. What this pins and what it does not: it asserts the declaration exists in the release job's block. It cannot assert a release actually publishes -- that needs a tag push, which is the thing no PR can do. A tripwire against silent removal, not proof the path works, and the KDoc says so. Closes #107. --- app/build.gradle.kts | 11 +++ .../ci/ReleasePermissionTest.kt | 67 +++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/ci/ReleasePermissionTest.kt diff --git a/app/build.gradle.kts b/app/build.gradle.kts index b0de8c9..e7f7716 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -1,3 +1,4 @@ +import org.gradle.api.tasks.PathSensitivity import org.gradle.testing.jacoco.tasks.JacocoReport plugins { @@ -206,6 +207,16 @@ detekt { // `excludes` is not optional. Without it JaCoCo walks JDK-internal classes that Robolectric has // no location for either, and the test JVM dies rather than reporting a number. tasks.withType().configureEach { + // ReleasePermissionTest reads .github/workflows/build.yml, and Gradle cannot infer that a + // test depends on a file outside the source set. Without this the task stays UP-TO-DATE + // when the workflow changes, so the guard goes stale exactly when it matters. Measured: + // deleting the release job's `contents: write` and re-running gave "BUILD SUCCESSFUL in + // 614ms" with the test never executing; the same mutation under --rerun-tasks failed it. + // A guard that does not re-run when its subject changes is not a guard. + inputs.file(rootProject.file(".github/workflows/build.yml")) + .withPropertyName("releaseWorkflow") + .withPathSensitivity(PathSensitivity.RELATIVE) + extensions.configure { isIncludeNoLocationClasses = true excludes = listOf("jdk.internal.*") diff --git a/app/src/test/java/org/libremediaconverter/ci/ReleasePermissionTest.kt b/app/src/test/java/org/libremediaconverter/ci/ReleasePermissionTest.kt new file mode 100644 index 0000000..e9ec637 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/ci/ReleasePermissionTest.kt @@ -0,0 +1,67 @@ +package org.libremediaconverter.ci + +import org.junit.Assert.assertTrue +import org.junit.Test +import java.io.File + +/** + * That the release job still holds the one permission it needs to publish. + * + * `build.yml`'s `release` job declares `contents: write`, and nothing was checking it. Deleting + * those two lines leaves actionlint clean and CodeQL silent — a *narrower* permission is not an + * alert — and the job is `if: startsWith(github.ref, 'refs/tags/v')`, so no pull request and no + * merge to `main` can exercise it. Measured: with the declaration removed, every gating check + * still passes. The first thing that would notice is a release failing to publish, at the moment + * someone is trying to cut one. + * + * The deletion also looks like tidying. A top-level `permissions: contents: read` now sits + * directly above it, so a reader could reasonably take the job-level block for a duplicate. It is + * an override, not a duplicate, and a comment saying so is not a check. + * + * `BackupExclusionsTest` is the precedent: a file that is configuration rather than code, load + * bearing, and unguarded because nothing compiles it. + * + * **What this pins, and what it does not.** It asserts the declaration exists in the `release` + * job's block. It cannot assert that a release actually publishes — that needs a tag push, which + * is the thing no PR can do. So this is a tripwire against silent removal, not proof the release + * path works. + */ +class ReleasePermissionTest { + + @Test + fun `the release job declares the write permission it needs to publish`() { + val release = jobBlock("release") + assertTrue( + "build.yml's `release` job no longer declares `contents: write`. It is the only " + + "permission that lets the job create a release, the top-level block above it is " + + "`contents: read`, and nothing else in CI would catch this until a tag failed to " + + "publish. If the release moved elsewhere, delete this test deliberately.", + release.any { it.trimStart().startsWith("contents: write") }, + ) + } + + /** + * The lines of one top-level job, from its ` :` header to the next job at that indent. + * + * Line-based rather than parsed: the module has no YAML dependency, and adding one to read two + * lines would be a worse trade than a scan that fails loudly when the shape changes. + */ + private fun jobBlock(name: String): List { + val lines = workflow.readLines() + val start = lines.indexOfFirst { it == " $name:" } + check(start >= 0) { "no ` $name:` job in ${workflow.path} — has the file been restructured?" } + val rest = lines.drop(start + 1) + val end = rest.indexOfFirst { it.matches(Regex("^ {2}[A-Za-z0-9_-]+:.*")) } + return if (end < 0) rest else rest.take(end) + } + + /** + * Found by walking up rather than by a fixed relative path: Gradle's working directory for the + * unit tests is the module, but that is a default rather than a promise. + */ + private val workflow: File + get() = generateSequence(File(".").absoluteFile) { it.parentFile } + .map { File(it, ".github/workflows/build.yml") } + .firstOrNull { it.isFile } + ?: error("could not find .github/workflows/build.yml above ${File(".").absolutePath}") +} -- 2.47.3