From 4a8e30099eb6ff9ab204ae0e216d9e1b20c34017 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 25 Aug 2026 15:38:14 -0500 Subject: [PATCH 1/3] 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 From 0702916229a45fc9f20a70b05bc82653b4964d5a Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 25 Aug 2026 16:04:13 -0500 Subject: [PATCH 2/3] Say what the advisory API 37 job actually found, so a new failure is not invisible That job is continue-on-error and red on every PR by design, which CLAUDE.md states plainly -- and that instruction is exactly why nobody reads it. Nothing in a red X separates "the known three" from "the known three plus yours". A bare failure count would not have fixed it, and this is measured rather than assumed. The run is usually truncated: seven of eight advisory runs read on 2026-08-25 ended in `Test run failed to complete. Expected 3 tests, received 2.` with INSTRUMENTATION_ABORTED, and one did not. A count taken from a truncated run misleads in both directions -- a fourth marked test can still yield the same number if the abort lands earlier, and the known set getting worse can lower it. The test XML does not rescue it either, which was the thing worth checking before building on it: it IS written for an aborted run, and it reports a tidy tests="3" failures="3" for a run the runner had just described as truncated. So the XML is the authority on how many results landed, the runner's own output is the only authority on whether the run finished, and the report reads both and says which number came from where. The baseline is one number beside the marker, because the marker means "cannot pass on this image": the count is both how many tests the advisory leg runs and how many should fail. A smaller failure count is the interesting direction -- it means one now passes, which is the documented trigger for deleting the annotation. Nothing about the job's status changes. It stays continue-on-error, stays red, stays out of the required contexts; a deviation is a ::notice::, never an ::error::. The report is a separate script so it can be run against a real log saved from a real CI run, which is how the comparison was shown to fire. The gating legs get the shape without the comparison: they run the whole suite, so comparing there would announce a deviation five times a run -- but a truncated run reporting fewer results than it ran is what #108 looks like, and "completed cleanly" is the field that would show it. Closes #83 --- .github/scripts/e2e-report-shape.sh | 270 ++++++++++++++++++ .github/scripts/e2e-run.sh | 37 ++- .github/workflows/status_check.yml | 8 + CLAUDE.md | 9 + .../FailsOnEmulatorApi37.kt | 29 ++ 5 files changed, 352 insertions(+), 1 deletion(-) create mode 100755 .github/scripts/e2e-report-shape.sh diff --git a/.github/scripts/e2e-report-shape.sh b/.github/scripts/e2e-report-shape.sh new file mode 100755 index 0000000..865bef7 --- /dev/null +++ b/.github/scripts/e2e-report-shape.sh @@ -0,0 +1,270 @@ +#!/usr/bin/env bash +# +# Reports the SHAPE of an instrumented run -- how many tests were expected, how many +# reported, how many failed, and whether the run completed at all -- to the step log and to +# the job summary. In advisory mode it also compares that shape against a committed baseline +# and says plainly whether it matches. +# +# WHY THIS EXISTS (#83): the advisory API 37 leg is red on every PR by design, so a NEW failure +# joining the known ones is invisible -- nothing in a red X distinguishes "the known ones" from +# "the known ones plus yours". CLAUDE.md tells everyone not to read that job's red as their +# change breaking something, which is correct, and which also means nobody looks. +# +# WHY NOT A BARE FAILURE COUNT, measured rather than assumed. On this image the run is usually +# truncated: `Test run failed to complete. Expected 3 tests, received 2.` with +# `INSTRUMENTATION_ABORTED: System has crashed.` A count taken from a truncated run misleads in +# both directions -- a fourth marked test can still yield the same number if the abort lands +# earlier, and the known set getting worse can LOWER it. So all four fields are recorded, and +# the one saying the run was truncated is recorded with them. +# +# WHY IT IS A SEPARATE SCRIPT rather than a function inside e2e-run.sh: it is a pure seam. It +# reads a captured log plus the test XML and writes a report, so it can be run against a REAL +# log saved from a REAL CI run -- which is how the baseline comparison was shown to fire +# without waiting on an emulator. `git ls-files '*.sh'` also picks it up for shellcheck for +# free. +# +# THIS SCRIPT NEVER FAILS A RUN. It is a diagnostic, and e2e-run.sh's header explains why that +# rule is absolute here. Every field defaults to `unknown` and every comparison is guarded, +# because an unset variable under `set -u`, or a `[ "" -eq 3 ]`, is exactly how a diagnostic +# becomes the thing that turns a leg red. It exits 0 unconditionally. +# +# Usage: +# e2e-report-shape.sh