From 2bed40d0806c4e7aa28dd8a9790bc733f01f8c26 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 15:58:48 -0500 Subject: [PATCH 1/4] Hold the three pickers to the constant they hand back The format, quality and engine pickers are the same dozen lines with a different enum substituted, and both ways they can go wrong are silent. An onClick that closes over the picker's `selected` parameter instead of the chip's own entry returns one constant for every chip; an inverted `entry == selected` lights every chip but the right one. Neither throws, neither changes the labels on screen, and a test that only asserted the callback ran would pass over the first of them. So each click test presses every chip in the row and compares the whole recorded list against `entries`, which makes the constant load-bearing rather than the click count, and each selection test asserts over every chip rather than only the one that should be lit. Verified by mutation, not by the suite going green: - `onSelect(format)` -> `onSelect(OutputFormat.MP4_H264)` fails with `expected:<[MP4_H264, MP4_H265, WEBM_VP9, ...]> but was:<[MP4_H264, MP4_H264, MP4_H264, ...]>` - `format == selected` -> `format != selected` fails both format selection tests on `Selected = 'true'` for a chip that should not be - `onSelect(preference)` -> `{}` fails with `expected:<[AUTO, PREFER_HARDWARE, FORCE_SOFTWARE]> but was:<[]>` - `selected.description` -> `QualityTier.FAST.description` fails the quality prose test on the missing BEST line Labels are read off the enums so a reword cannot redden this file for the wrong reason. `EnginePreference` has no label of its own, so the screen's own `label()` supplies that set. The one display literal with no symbol behind it, the custom-spec line, was copied out of the source byte for byte because it holds a U+2014 that would fail silently if retyped. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/ConverterPickerSelectionTest.kt | 169 ++++++++++++++++++ 1 file changed, 169 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/convert/ConverterPickerSelectionTest.kt diff --git a/app/src/test/java/org/libremediaconverter/convert/ConverterPickerSelectionTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConverterPickerSelectionTest.kt new file mode 100644 index 0000000..3fdbbb6 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ConverterPickerSelectionTest.kt @@ -0,0 +1,169 @@ +package org.libremediaconverter.convert + +import androidx.compose.ui.test.SemanticsNodeInteraction +import androidx.compose.ui.test.assertIsNotSelected +import androidx.compose.ui.test.assertIsSelected +import androidx.compose.ui.test.hasAnyAncestor +import androidx.compose.ui.test.hasTestTag +import androidx.compose.ui.test.hasText +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.media3.common.util.UnstableApi +import org.junit.Assert.assertEquals +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.createDrainedComposeRule +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.QualityTier +import org.libremediaconverter.ui.TestTags +import org.robolectric.RobolectricTestRunner + +/** + * Each picker lights the chip it was handed and reports the constant that was pressed. + * + * The defect this bites on is a picker that renders perfectly and answers wrongly. All three are + * the same dozen lines with a different enum substituted, so the failure mode is a copy-paste that + * survives review: an `onClick` that closes over the picker's `selected` parameter instead of the + * chip's own entry hands back one constant no matter which chip was tapped, and an inverted + * `entry == selected` lights every chip except the right one. Neither throws, neither changes the + * set of labels on screen, and a test that only asserted "the callback ran" would pass over both. + * + * Clicking every chip in turn and comparing the whole recorded list against `entries` is what makes + * the constant load-bearing rather than the click count -- a hardcoded `onSelect` fires the same + * number of times as a correct one. Selection is asserted over every chip for the same reason: the + * one that should be lit proves nothing on its own, because `!=` lights it too whenever the enum + * has exactly one entry, and lights all its siblings whenever it has more. + * + * Labels come from `OutputFormat.label` and `QualityTier.label`; [label], which the screen owns + * because `EnginePreference` carries no label of its own, supplies the third set. Retyping any of + * them here would turn a rename into a red test that named the wrong cause. + * + * Not covered, deliberately: the `"Output format"`, `"Quality"` and `"Engine"` headings, which are + * untagged `Text` calls with no enum behind them and no behaviour to bite on. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ConverterPickerSelectionTest { + + @get:Rule + val composeRule = createDrainedComposeRule() + + /** + * The chip carrying [label] inside the row tagged [rowTag]. + * + * By ancestor rather than by direct child: how many semantics nodes Material 3 puts between a + * `FlowRow` and its chips is that library's business, and a matcher that assumed "one" would + * break on an upgrade that changed nothing this test is about. + */ + private fun chipIn(rowTag: String, label: String): SemanticsNodeInteraction = + composeRule.onNode(hasAnyAncestor(hasTestTag(rowTag)) and hasText(label)) + + private fun assertOnlySelected(rowTag: String, labels: List, selected: String?) { + labels.forEach { label -> + val chip = chipIn(rowTag, label) + if (label == selected) chip.assertIsSelected() else chip.assertIsNotSelected() + } + } + + @Test + fun `the format picker lights the selected format and no other`() { + composeRule.setContent { FormatPicker(OutputFormat.WEBM_VP9) {} } + + assertOnlySelected( + rowTag = TestTags.Converter.FORMAT_CHIPS, + labels = OutputFormat.entries.map { it.label }, + selected = OutputFormat.WEBM_VP9.label, + ) + } + + /** A spec no preset can express lights nothing, which is what the custom line stands in for. */ + @Test + fun `the format picker lights nothing when the spec is custom`() { + composeRule.setContent { FormatPicker(null) {} } + + assertOnlySelected( + rowTag = TestTags.Converter.FORMAT_CHIPS, + labels = OutputFormat.entries.map { it.label }, + selected = null, + ) + composeRule.onNodeWithText(CUSTOM_SPEC_NOTE).assertExists() + } + + @Test + fun `a selected format hides the custom line`() { + composeRule.setContent { FormatPicker(OutputFormat.MP3) {} } + + composeRule.onNodeWithText(CUSTOM_SPEC_NOTE).assertDoesNotExist() + } + + @Test + fun `clicking a format chip reports that format`() { + val picked = mutableListOf() + composeRule.setContent { FormatPicker(null) { picked += it } } + + OutputFormat.entries.forEach { chipIn(TestTags.Converter.FORMAT_CHIPS, it.label).performClick() } + + assertEquals(OutputFormat.entries.toList(), picked) + } + + @Test + fun `the quality picker lights the selected tier and no other`() { + composeRule.setContent { QualityPicker(QualityTier.BEST) {} } + + assertOnlySelected( + rowTag = TestTags.Converter.QUALITY_CHIPS, + labels = QualityTier.entries.map { it.label }, + selected = QualityTier.BEST.label, + ) + } + + /** The line under the chips describes what was chosen, not whichever tier was written first. */ + @Test + fun `the quality picker explains the tier that is selected`() { + composeRule.setContent { QualityPicker(QualityTier.BEST) {} } + + composeRule.onNodeWithText(QualityTier.BEST.description).assertExists() + composeRule.onNodeWithText(QualityTier.FAST.description).assertDoesNotExist() + } + + @Test + fun `clicking a quality chip reports that tier`() { + val picked = mutableListOf() + composeRule.setContent { QualityPicker(QualityTier.FAST) { picked += it } } + + QualityTier.entries.forEach { chipIn(TestTags.Converter.QUALITY_CHIPS, it.label).performClick() } + + assertEquals(QualityTier.entries.toList(), picked) + } + + @Test + fun `the engine picker lights the selected preference and no other`() { + composeRule.setContent { EnginePicker(EnginePreference.FORCE_SOFTWARE) {} } + + assertOnlySelected( + rowTag = TestTags.Converter.ENGINE_CHIPS, + labels = EnginePreference.entries.map { it.label() }, + selected = EnginePreference.FORCE_SOFTWARE.label(), + ) + } + + @Test + fun `clicking an engine chip reports that preference`() { + val picked = mutableListOf() + composeRule.setContent { EnginePicker(EnginePreference.AUTO) { picked += it } } + + EnginePreference.entries.forEach { chipIn(TestTags.Converter.ENGINE_CHIPS, it.label()).performClick() } + + assertEquals(EnginePreference.entries.toList(), picked) + } + + private companion object { + /** + * Copied byte for byte out of `ConverterScreen.kt` -- it holds a U+2014 em dash, which + * retyped as ASCII would match nothing and fail as "no node found" rather than as a reword. + */ + const val CUSTOM_SPEC_NOTE: String = "Custom — set below." + } +} From baaaa934e0cc0826aecf2fb8d818e0919caef3ea Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 16:03:14 -0500 Subject: [PATCH 2/4] Check the shell, and stop one-off issues falling off the board Two gaps, both found the same way -- by something going wrong quietly. `gh issue create` does not touch the project board. The issue is created, carries its labels, and is invisible in the Kanban, which looks exactly like a ticket nobody filed. On 2026-08-24 eight issues filed as a scripted batch all reached the board and one filed as a one-off minutes later did not; it surfaced only because someone went looking for it. A batch carries the board step inside its loop. One-offs are where it slips, so tools/github/file-issue.sh is for one-offs. Three things it does that a two-command shell snippet would not: - Resolves the project, Status field and option ids BY NAME, every run. Caching them is the obvious optimisation and the wrong one -- a renamed or reordered column would then have this writing a stale id into the board with no error anywhere. - Reads the item back. A mutation returning 200 says the request was accepted, not that the board shows what was asked for; the read-back is the only step that checks the claim this script exists to make. It is a GraphQL query because REST cannot do it -- the `fields` array REST returns on a project item carries Title and nothing else, so a REST-only check reports every item's Status as unset. - Exits 3, loudly, with the issue number on a line of its own, when the issue was created but the board step failed. That exact combination is the failure being prevented; it must never be the quiet path. Shell was the other language here with nothing checking it -- four scripts, one of them the CI entry point. shellcheck now runs in the Static analysis job over `git ls-files '*.sh'`, so a script added later is covered without editing the workflow, and it runs at full severity with `info` included. That raises two findings today and both are the tool being wrong, so both are answered with a targeted `disable` carrying its reason rather than by lowering the severity: run-e2e.sh's `on_signal` is reported as never invoked when it is installed as the INT and TERM trap eleven lines below it, and the `$names` inside file-issue.sh's queries are GraphQL variables that must not expand -- expanding them would send the shell's idea of $owner to the API instead of declaring a parameter. A blanket --severity=warning would have hidden both, and the next real finding with them. The gradle step gains `if: !cancelled()` so a shellcheck failure cannot cost the ktlint/detekt/lint lists -- the same reason that step already passes --continue. Not covered, deliberately: shellcheck here reads .sh files, not the inline `run:` blocks in the workflows, where a good deal of this repo's bash actually lives. actionlint does read them, and finds one pre-existing info-level issue in build.yml. Wiring it in means pinning a container digest, because every action here is pinned by SHA and actionlint's usual installer is a curl-pipe-bash off a moving branch. Its own ticket, not this commit. --- .github/workflows/status_check.yml | 19 +++ CLAUDE.md | 28 ++++ tools/github/file-issue.sh | 253 +++++++++++++++++++++++++++++ tools/local-emulator/run-e2e.sh | 1 + 4 files changed, 301 insertions(+) create mode 100755 tools/github/file-issue.sh diff --git a/.github/workflows/status_check.yml b/.github/workflows/status_check.yml index f650df4..d555764 100644 --- a/.github/workflows/status_check.yml +++ b/.github/workflows/status_check.yml @@ -156,7 +156,26 @@ jobs: key: gradle-${{ runner.os }}-${{ hashFiles('**/*.gradle.kts', 'gradle/libs.versions.toml', 'gradle/wrapper/gradle-wrapper.properties') }} restore-keys: gradle-${{ runner.os }}- + # Shell is the other language in this repo -- four scripts, one of them the CI + # entry point itself -- and nothing was checking it. `git ls-files` rather than a + # fixed list, so a script added later is covered without editing this workflow. + # + # Full severity, `info` included. The two findings it raises today are answered + # with targeted `disable` directives carrying their reason, the same way + # config/detekt/detekt.yml carries only the rules this codebase legitimately + # breaks. A blanket --severity=warning would have hidden them and the next real + # one alike. shellcheck is preinstalled on the ubuntu runner image; the version is + # printed so a finding that appears out of nowhere can be pinned to an upgrade. + - name: shellcheck + run: | + shellcheck --version + git ls-files -z '*.sh' | xargs -0 -r shellcheck + + # `!cancelled()` rather than a plain sequence: a shellcheck failure above must not + # cost the ktlint/detekt/lint lists. Same reason this step passes --continue -- one + # round trip should produce every list, not stop at the first. - name: ktlint, detekt and Android lint + if: '!cancelled()' run: ./gradlew :app:ktlintCheck :app:detekt :app:lintDebug --continue --stacktrace # The XML matters as much as the HTML: it is the one that can be diffed between diff --git a/CLAUDE.md b/CLAUDE.md index af6e7b0..efe7fc1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -123,6 +123,34 @@ install for code that can never run — and on API 37 the full APK does not fit - `kotlin.code.style=official`. Gradle stays Kotlin DSL. +- **File one-off issues with `tools/github/file-issue.sh`, not `gh issue create`.** `gh issue + create` does not touch the project board, so the issue exists, carries its labels, and is + invisible in the Kanban — indistinguishable from never having been filed. Measured 2026-08-24: + eight issues filed as a scripted batch all reached the board; one filed as a one-off minutes + later did not. A batch carries the board step in its loop; **one-offs are where it slips**, which + is what the script is for. It resolves the project and Status ids by name rather than caching + them, and it **reads the item back** — a mutation returning 200 is not evidence the board shows + what was asked for. Exit 3 means the issue was created but did not reach the board, and prints + the number so it cannot be lost quietly. + + `above-cut` and `backlog` are **labels from the 2026-08-22 triage pass** — "worked autonomously + overnight" and "held for manual review". They are not board columns. Status carries board state; + do not put a cut label on a newly filed ticket. + +- **shellcheck runs in CI**, inside the Static analysis job, over `git ls-files '*.sh'` so a new + script is covered without editing the workflow. It runs at full severity, `info` included: the + two findings that raises today are answered with targeted `disable` directives carrying their + reason, exactly as `config/detekt/detekt.yml` carries only the rules this codebase legitimately + breaks. Do not silence it with `--severity=warning` — that hides the next real finding too. + Locally there is no shellcheck package installed; `podman run --rm -v "$PWD:/mnt:z" + docker.io/koalaman/shellcheck:stable ` is what was used. + + **It does not cover inline `run:` blocks in the workflows**, and a good deal of this repo's bash + lives there. `actionlint` does cover them — it runs shellcheck over each `run:` — and reports one + pre-existing `info` finding in `build.yml`. It is not wired in because every action here is + pinned by SHA, and actionlint's usual installer is a `curl | bash` off a moving branch; doing it + properly means pinning a container digest. Tracked separately rather than bolted on. + ## Dependency versions Libraries **float on minor + patch** (`coreKtx = "1.+"`). Three groups deliberately do not: diff --git a/tools/github/file-issue.sh b/tools/github/file-issue.sh new file mode 100755 index 0000000..a48a866 --- /dev/null +++ b/tools/github/file-issue.sh @@ -0,0 +1,253 @@ +#!/usr/bin/env bash +# +# Files a GitHub issue AND puts it on the project board, as one operation. +# +# Usage: tools/github/file-issue.sh --title TITLE (--body TEXT | --body-file PATH) [options] +# +# --status NAME board column, matched case-insensitively against the board's own +# options; a miss lists what is available. Default: Backlog +# --label NAME repeatable. Passed through to `gh issue create` unchanged. +# --project N project number. Default: $ISSUE_PROJECT_NUMBER, else 6 +# --repo OWNER/NAME default: whatever `gh repo view` resolves in the working directory +# --dry-run resolve and validate everything, create nothing +# +# EXIT CODE: 0 only when the issue exists, is on the board, AND reads back carrying the +# Status that was asked for. 2 for a usage or validation error, before anything is created. +# **3 means the issue was created but did not reach the board** -- the number is printed on +# a line of its own, because that combination is the entire failure this script exists to +# prevent and it must never be quiet. +# +# WHY THIS EXISTS +# +# `gh issue create` does not touch the project board. The issue is created, carries its +# labels, and is invisible in the Kanban -- which looks exactly like a ticket nobody filed. +# Measured 2026-08-24: eight issues filed as a scripted batch all reached the board; one +# filed as a one-off a few minutes later did not, and was caught only because someone went +# looking. A batch carries the board step inside its loop. One-offs are where it slips, so +# one-offs are what this is for. +# +# Adding an item and setting a field value are GraphQL-only. REST can list project items +# and field definitions, but the `fields` array it returns on an item carries Title and +# nothing else -- a REST-only check reports every item's Status as unset, which is why the +# read-back at the end is a GraphQL query rather than the cheaper REST one. +# +# WHAT IT DELIBERATELY DOES NOT DO +# +# It does not cache the project, field or option ids. Resolving them by name costs one +# GraphQL query per run, and it means a renamed or reordered column cannot make this write +# a stale id. The ids are the fragile part; the names are what people actually use. +# +# It does not apply triage labels for you. `above-cut` and `backlog` are labels from one +# specific 2026-08-22 triage pass -- they mean "worked autonomously overnight" and "held for +# manual review", not "this is in the Backlog column". Status carries board state. Pass +# --label only for things that are true about the issue itself. +# +# It does not create the project, the Status field, or a missing option. Anything absent is +# an error to report, not to invent. + +set -euo pipefail + +readonly EXIT_USAGE=2 +readonly EXIT_ORPHANED=3 + +die() { + printf 'file-issue: %s\n' "$1" >&2 + exit "${2:-$EXIT_USAGE}" +} + +title="" +body="" +body_file="" +status="Backlog" +project="${ISSUE_PROJECT_NUMBER:-6}" +repo="" +dry_run=0 +labels=() + +while [ $# -gt 0 ]; do + case "$1" in + --title) [ $# -ge 2 ] || die "--title needs a value"; title="$2"; shift 2 ;; + --body) [ $# -ge 2 ] || die "--body needs a value"; body="$2"; shift 2 ;; + --body-file) [ $# -ge 2 ] || die "--body-file needs a path"; body_file="$2"; shift 2 ;; + --status) [ $# -ge 2 ] || die "--status needs a value"; status="$2"; shift 2 ;; + --label) [ $# -ge 2 ] || die "--label needs a value"; labels+=("$2"); shift 2 ;; + --project) [ $# -ge 2 ] || die "--project needs a number"; project="$2"; shift 2 ;; + --repo) [ $# -ge 2 ] || die "--repo needs OWNER/NAME"; repo="$2"; shift 2 ;; + --dry-run) dry_run=1; shift ;; + -h|--help) awk 'NR > 1 && /^#/ { sub(/^# ?/, ""); print; next } NR > 1 { exit }' "$0" + exit 0 ;; + *) die "unknown argument: $1" ;; + esac +done + +[ -n "$title" ] || die "--title is required" +if [ -n "$body" ] && [ -n "$body_file" ]; then + die "pass --body or --body-file, not both" +fi +[ -n "$body" ] || [ -n "$body_file" ] || die "one of --body or --body-file is required" +if [ -n "$body_file" ] && [ ! -r "$body_file" ]; then + die "--body-file is not readable: $body_file" +fi +case "$project" in + ''|*[!0-9]*) die "--project must be a number, got: $project" ;; +esac + +command -v gh >/dev/null 2>&1 || die "gh is not on PATH" + +if [ -z "$repo" ]; then + repo=$(gh repo view --json nameWithOwner --jq '.nameWithOwner') \ + || die "could not resolve the repository; pass --repo OWNER/NAME" +fi +owner="${repo%%/*}" +[ -n "$owner" ] || die "could not read an owner out of: $repo" + +# --------------------------------------------------------------------------- +# Resolve the board by NAME. Every id below is read fresh; none is hardcoded. +# --------------------------------------------------------------------------- + +# The $names in the query are GraphQL variables, declared by the query and bound by the +# -f flags. Expanding them in the shell would send this shell's idea of $owner to the +# API instead of declaring a parameter -- which is why every query here is single-quoted. +# shellcheck disable=SC2016 +board=$(gh api graphql \ + -f query=' + query($owner: String!, $number: Int!) { + user(login: $owner) { + projectV2(number: $number) { + id + title + field(name: "Status") { + ... on ProjectV2SingleSelectField { id options { id name } } + } + } + } + }' \ + -f owner="$owner" -F number="$project" 2>&1) \ + || die "could not read project $project for $owner. A 403 naming scopes means gh is +missing 'project'; a 403 naming a rate limit is the GraphQL budget, not permissions. The +API said: $board" + +project_id=$(printf '%s' "$board" | jq -r '.data.user.projectV2.id // empty') +field_id=$(printf '%s' "$board" | jq -r '.data.user.projectV2.field.id // empty') +project_title=$(printf '%s' "$board" | jq -r '.data.user.projectV2.title // empty') + +[ -n "$project_id" ] || die "no project number $project under user $owner" +[ -n "$field_id" ] || die "project $project has no single-select field named 'Status'" + +# Case-insensitive match, so "backlog" and "Backlog" both work. The canonical name is +# what gets reported back, so a sloppy argument still produces an exact log line. +option=$(printf '%s' "$board" | jq -r --arg want "$status" ' + .data.user.projectV2.field.options[] + | select((.name | ascii_downcase) == ($want | ascii_downcase)) + | "\(.id)\t\(.name)"' | head -n 1) + +if [ -z "$option" ]; then + printf 'file-issue: no Status option named %s. Available:\n' "$status" >&2 + printf '%s' "$board" | jq -r '.data.user.projectV2.field.options[] | " " + .name' >&2 + exit "$EXIT_USAGE" +fi +option_id="${option%%$'\t'*}" +status_canonical="${option#*$'\t'}" + +printf 'repo %s\n' "$repo" +printf 'board %s (project %s)\n' "$project_title" "$project" +printf 'status %s\n' "$status_canonical" +printf 'labels %s\n' "${labels[*]:-(none)}" +printf 'title %s\n' "$title" + +if [ "$dry_run" -eq 1 ]; then + printf '\ndry run: everything above resolved; nothing was created.\n' + exit 0 +fi + +# --------------------------------------------------------------------------- +# Create. Past this line a failure can leave an issue off the board, so every +# error path prints the number. +# --------------------------------------------------------------------------- + +create_args=(--repo "$repo" --title "$title") +if [ -n "$body_file" ]; then + create_args+=(--body-file "$body_file") +else + create_args+=(--body "$body") +fi +for label in ${labels[@]+"${labels[@]}"}; do + create_args+=(--label "$label") +done + +issue_url=$(gh issue create "${create_args[@]}") || die "gh issue create failed; nothing was filed" +issue_number="${issue_url##*/}" +case "$issue_number" in + ''|*[!0-9]*) die "could not read an issue number out of: $issue_url" ;; +esac + +orphaned() { + printf 'file-issue: %s\n' "$1" >&2 + printf 'file-issue: THE ISSUE EXISTS BUT IS NOT ON THE BOARD. Fix it by hand:\n' >&2 + printf '%s\n' "$issue_url" >&2 + exit "$EXIT_ORPHANED" +} + +content_id=$(gh api "/repos/$repo/issues/$issue_number" --jq '.node_id') \ + || orphaned "could not read the node id for #$issue_number" + +# shellcheck disable=SC2016 # GraphQL variables, as above +item_id=$(gh api graphql \ + -f query=' + mutation($project: ID!, $content: ID!) { + addProjectV2ItemById(input: {projectId: $project, contentId: $content}) { + item { id } + } + }' \ + -f project="$project_id" -f content="$content_id" \ + --jq '.data.addProjectV2ItemById.item.id') \ + || orphaned "could not add #$issue_number to the board" +[ -n "$item_id" ] || orphaned "the board add returned no item id for #$issue_number" + +# shellcheck disable=SC2016 # GraphQL variables, as above +gh api graphql \ + -f query=' + mutation($project: ID!, $item: ID!, $field: ID!, $option: String!) { + updateProjectV2ItemFieldValue(input: { + projectId: $project, itemId: $item, fieldId: $field, + value: {singleSelectOptionId: $option} + }) { projectV2Item { id } } + }' \ + -f project="$project_id" -f item="$item_id" -f field="$field_id" -f option="$option_id" \ + >/dev/null \ + || orphaned "#$issue_number is on the board but its Status could not be set" + +# --------------------------------------------------------------------------- +# Read back. A mutation returning 200 is not evidence the board shows what was +# asked for -- this is the only check that is. +# --------------------------------------------------------------------------- + +# shellcheck disable=SC2016 # GraphQL variables, as above +readback=$(gh api graphql \ + -f query=' + query($item: ID!) { + node(id: $item) { + ... on ProjectV2Item { + content { ... on Issue { number } } + fieldValueByName(name: "Status") { + ... on ProjectV2ItemFieldSingleSelectValue { name } + } + } + } + }' \ + -f item="$item_id") \ + || orphaned "#$issue_number was written but could not be read back" + +seen_number=$(printf '%s' "$readback" | jq -r '.data.node.content.number // empty') +seen_status=$(printf '%s' "$readback" | jq -r '.data.node.fieldValueByName.name // empty') + +if [ "$seen_number" != "$issue_number" ]; then + orphaned "read-back names issue #${seen_number:-}, expected #$issue_number" +fi +if [ "$seen_status" != "$status_canonical" ]; then + orphaned "read-back Status is ${seen_status:-}, expected $status_canonical" +fi + +printf '\n#%s on %s as %s -- verified by read-back\n' \ + "$issue_number" "$project_title" "$seen_status" +printf '%s\n' "$issue_url" diff --git a/tools/local-emulator/run-e2e.sh b/tools/local-emulator/run-e2e.sh index 811e809..5e13dc8 100755 --- a/tools/local-emulator/run-e2e.sh +++ b/tools/local-emulator/run-e2e.sh @@ -456,6 +456,7 @@ cleanup() { # bash runs a trap only between commands, so this starts when whatever was in the foreground # returns -- which for Ctrl-C is immediately, because the same interrupt reached that command # too. `kill -INT` aimed at this script alone waits for the foreground command to finish. +# shellcheck disable=SC2329 # invoked indirectly -- installed as the INT and TERM trap a few lines below. on_signal() { echo echo "interrupted (SIG$1) -- stopping the emulator and removing the AVDs this run created" From 7c69d0699a8a0345c8824031fa1fddb8257d691e Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 16:03:00 -0500 Subject: [PATCH 3/4] Hold the Advanced panel's gate, and the error card outside it `AdvancedPicker` is the one leaf on the converter screen that carries its own state, and `ValidationError` is deliberately invoked after the `AnimatedVisibility` that gates the chip rows -- so an invalid spec explains itself and offers one-tap fixes while the section is collapsed. That is the only route out of an invalid spec for a user who never opened Advanced, it was completely untested, and folding the two `if` blocks into one is a plausible tidy-up that compiles. `AdvancedPickerTest` covers the gate in both directions, clicks each of the four colliding chip labels through its own row tag, and does every assertion about the error card with the toggle untouched. `AdvancedPanelSavedStateTest` is the `DestinationSaverTest` split for `expanded`: `StateRestorationTester` saves into an in-memory map, so it proves `rememberSaveable` is in use and nothing about the representation. Driving a real `SaveableStateRegistry` shows the picker saves the `MutableState` itself rather than the `Boolean`, which only survives a rotation because `mutableStateOf` on Android returns a `Parcelable` one. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/AdvancedPanelSavedStateTest.kt | 159 +++++++++ .../convert/AdvancedPickerTest.kt | 321 ++++++++++++++++++ 2 files changed, 480 insertions(+) create mode 100644 app/src/test/java/org/libremediaconverter/convert/AdvancedPanelSavedStateTest.kt create mode 100644 app/src/test/java/org/libremediaconverter/convert/AdvancedPickerTest.kt diff --git a/app/src/test/java/org/libremediaconverter/convert/AdvancedPanelSavedStateTest.kt b/app/src/test/java/org/libremediaconverter/convert/AdvancedPanelSavedStateTest.kt new file mode 100644 index 0000000..4961381 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/AdvancedPanelSavedStateTest.kt @@ -0,0 +1,159 @@ +package org.libremediaconverter.convert + +import android.os.Bundle +import android.os.Parcel +import android.os.Parcelable +import androidx.compose.runtime.CompositionLocalProvider +import androidx.compose.runtime.MutableState +import androidx.compose.runtime.saveable.LocalSaveableStateRegistry +import androidx.compose.runtime.saveable.SaveableStateRegistry +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.performClick +import androidx.media3.common.util.UnstableApi +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.createDrainedComposeRule +import org.libremediaconverter.model.AudioCodec +import org.libremediaconverter.model.Container +import org.libremediaconverter.model.OutputSpec +import org.libremediaconverter.model.Validation +import org.libremediaconverter.model.VideoCodec +import org.libremediaconverter.ui.TestTags +import org.robolectric.RobolectricTestRunner + +/** + * What `AdvancedPickerTest`'s restoration test cannot see. + * + * `StateRestorationTester` saves into an **in-memory map**, never a `Bundle`. That is enough to + * discriminate `rememberSaveable` from `remember`, and it is where it stops: the map holds object + * references, so a value the platform could never parcel goes in and comes back out looking green. + * `AppRootRestorationTest` has the same blind spot and `DestinationSaverTest` is the split it + * prompted; this is that split for `expanded`, the only `rememberSaveable` on either screen's + * leaves. + * + * ### The saved representation is not the Boolean + * + * `var expanded by rememberSaveable { mutableStateOf(false) }` passes no `stateSaver`, so + * `autoSaver` saves **the `MutableState` itself**, not the `false` inside it. That works only + * because `mutableStateOf` on Android returns a `Parcelable` implementation -- the same call on a + * plain JVM returns one that is not. So what stands between an open panel and a rotation that + * closes it is a platform-specific detail of a factory function nothing here names directly, and + * an in-memory map cannot tell the two apart. + * + * Pinning it is the move `DestinationSaverTest` makes about names versus ordinals. Passing an + * explicit `stateSaver` would save a bare `Boolean` instead and is a perfectly reasonable edit -- + * it is just not the one in the tree, and it should be made on purpose rather than discovered + * after a rotation. + * + * ### Shared bite, stated rather than implied + * + * `rememberSaveable` -> `remember` empties the registry, so it reddens this file *and* the + * restoration test in `AdvancedPickerTest`. Both failures belong in any report of that mutation. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class AdvancedPanelSavedStateTest { + + // Not `createComposeRule()` directly: see [drainEscapedCoroutineErrors]. + @get:Rule + val composeRule = createDrainedComposeRule() + + /** + * `canBeSaved = { true }` deliberately. + * + * A predicate mirroring what a `Bundle` accepts would be a hand-written copy of the thing + * under test, and a `false` from it *drops* the entry silently -- so the test would fail by + * finding nothing saved, which is also how a `remember` regression fails. Two causes, one + * symptom, is not a test. The type is checked on the way out instead. + */ + private val registry = SaveableStateRegistry(restoredValues = null, canBeSaved = { true }) + + @Test + fun `the panel registers its open state with the registry, and nothing else`() { + setPicker() + + // Collapsed is a saved value, not an absent one: `rememberSaveable` registers its provider + // on first composition, whatever the state happens to be. Exactly one, because `expanded` + // is the only saveable in the subtree -- a second would mean something else began saving. + assertEquals(1, savedValues().size) + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + val saved = theOneSavedValue() + assertTrue("saved as ${saved?.javaClass?.name}", saved is MutableState<*>) + assertEquals(true, (saved as MutableState<*>).value) + } + + @Test + fun `the open panel survives a real Parcel, not just an in-memory map`() { + setPicker() + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + val saved = theOneSavedValue() + + // The claim the restoration test cannot make. A `MutableState` that was not `Parcelable` + // would satisfy `StateRestorationTester` and then be dropped by the platform. + assertTrue("saved as ${saved?.javaClass?.name}", saved is Parcelable) + + val restored = throughARealBundle(saved as Parcelable) + + assertTrue("restored as ${restored.javaClass.name}", restored is MutableState<*>) + assertEquals(true, (restored as MutableState<*>).value) + } + + private fun setPicker() { + composeRule.setContent { + CompositionLocalProvider(LocalSaveableStateRegistry provides registry) { + AdvancedPicker( + spec = OutputSpec(Container.MP4, VideoCodec.H264, AudioCodec.AAC), + validation = Validation.Valid, + onContainer = {}, + onVideoCodec = {}, + onAudioCodec = {}, + onSuggestion = {}, + ) + } + } + } + + /** Every value the picker hands the host to persist, keys dropped -- they are positional. */ + private fun savedValues(): List = composeRule.runOnIdle { registry.performSave().values.flatten() } + + /** + * The single saved value, asserted rather than assumed. + * + * `single()` on an empty list throws `NoSuchElementException: List is empty`, which names + * neither the panel nor the registry -- and an empty registry is exactly how the + * `rememberSaveable` -> `remember` regression shows up here. + */ + private fun theOneSavedValue(): Any? { + val values = savedValues() + assertEquals("the panel should register exactly one saved value", 1, values.size) + return values.first() + } + + /** A write and a read through a real `Parcel`, which is what the tester's map stands in for. */ + private fun throughARealBundle(value: Parcelable): Parcelable { + val bundle = Bundle().apply { putParcelable(KEY, value) } + val parcel = Parcel.obtain() + return try { + parcel.writeBundle(bundle) + parcel.setDataPosition(0) + val restored = requireNotNull(parcel.readBundle(javaClass.classLoader)) { + "the Bundle did not survive the Parcel" + } + requireNotNull(restored.getParcelable(KEY, Parcelable::class.java)) { + "the saved state did not survive the Parcel" + } + } finally { + parcel.recycle() + } + } + + private companion object { + const val KEY = "expanded" + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/AdvancedPickerTest.kt b/app/src/test/java/org/libremediaconverter/convert/AdvancedPickerTest.kt new file mode 100644 index 0000000..342b54e --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/AdvancedPickerTest.kt @@ -0,0 +1,321 @@ +package org.libremediaconverter.convert + +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.assertTextEquals +import androidx.compose.ui.test.hasAnyAncestor +import androidx.compose.ui.test.hasTestTag +import androidx.compose.ui.test.hasText +import androidx.compose.ui.test.junit4.StateRestorationTester +import androidx.compose.ui.test.onAllNodesWithTag +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.media3.common.util.UnstableApi +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.createDrainedComposeRule +import org.libremediaconverter.model.AudioCodec +import org.libremediaconverter.model.Container +import org.libremediaconverter.model.ContainerCapabilities +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputSpec +import org.libremediaconverter.model.Validation +import org.libremediaconverter.model.VideoCodec +import org.libremediaconverter.ui.TestTags +import org.robolectric.RobolectricTestRunner + +/** + * The gate over the Advanced chips, and the error card that deliberately sits outside it. + * + * Two defects, and they pull in opposite directions. + * + * The first is the chips escaping the gate, or never being reachable through it. `AdvancedPicker` + * is the one leaf on this screen that is not stateless -- `expanded` is its own `rememberSaveable` + * -- and Container, Video and Audio live inside `AnimatedVisibility(visible = expanded)`. Nothing + * else on the screen hides anything, so a refactor that flattened the panel, or wired the toggle to + * a state nobody reads, would render an app that looks reasonable in a screenshot and is wrong. + * + * The second is the opposite mistake, and it is the one this file exists for: **moving the + * `ValidationError` call inside the `AnimatedVisibility`**. It is invoked after that block, so an + * invalid spec explains itself and offers one-tap fixes *while the section is collapsed*. That is + * the only route out of an invalid spec for a user who never opened Advanced -- and since the only + * way to reach an invalid spec is through Advanced, hiding the way out behind the same toggle looks + * locally sensible and is a trap. Tidying the two `if` blocks into one is a plausible edit, it + * compiles, and until this file existed nothing went red. Every assertion about the error card here + * therefore runs with the toggle untouched, and asserts the panel is absent in the same test, so a + * future `expanded = true` default cannot quietly satisfy it either. + * + * The invalid specs come from [ContainerCapabilities.validate] rather than from a hand-built + * [Validation.Invalid], so the messages and the suggestions are the real pairing. A hand-built one + * would keep passing after `validate` stopped producing anything like it. + * + * Node location is by the three separate chip-row tags, never by text. `"Copy"` and `"None"` are + * each both a [VideoCodec] and an [AudioCodec], and `"MP3"` and `"FLAC"` are each both a + * [Container] and an [AudioCodec], so a text matcher over the open panel is ambiguous for four + * chips -- which is what the separate tags are for. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class AdvancedPickerTest { + + // Not `createComposeRule()` directly: see [drainEscapedCoroutineErrors]. + @get:Rule + val composeRule = createDrainedComposeRule() + + private val restoration = StateRestorationTester(composeRule) + + private val containers = mutableListOf() + private val videoCodecs = mutableListOf() + private val audioCodecs = mutableListOf() + private val applied = mutableListOf() + + // --- the expand gate ---------------------------------------------------- + + @Test + fun `the three chip rows appear only while the panel is expanded`() { + setPicker() + + assertPanelHidden() + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_PANEL).assertExists() + ROW_TAGS.forEach { composeRule.onNodeWithTag(it).assertExists() } + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + // The exit transition outlives the click, so absence has to be waited for rather than + // asserted straight away -- unlike the initial collapsed state, which has no animation + // in flight. + composeRule.waitUntil { nodeCount(TestTags.Converter.ADVANCED_PANEL) == 0 } + assertPanelHidden() + } + + /** The toggle is the only affordance the collapsed picker offers, so it has to say so. */ + @Test + fun `the toggle names the direction it will move in`() { + setPicker() + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).assertTextEquals("Advanced") + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE) + .assertTextEquals("Hide advanced") + } + + /** + * The four colliding labels, one per row. + * + * `"Copy"` is a video codec *and* an audio codec; `"MP3"` is a container *and* an audio codec. + * Clicking each through its own row is what proves the rows are wired to different callbacks + * -- a picker that handed every chip to `onAudioCodec` would look identical on screen. + */ + @Test + fun `each chip row reports to its own callback, including the labels that collide`() { + setPicker() + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + chipIn(TestTags.Converter.ADVANCED_VIDEO_CHIPS, "Copy").performClick() + + assertEquals(listOf(VideoCodec.COPY), videoCodecs) + assertEquals(emptyList(), audioCodecs) + + chipIn(TestTags.Converter.ADVANCED_AUDIO_CHIPS, "Copy").performClick() + + assertEquals(listOf(AudioCodec.COPY), audioCodecs) + + chipIn(TestTags.Converter.ADVANCED_CONTAINER_CHIPS, "MP3").performClick() + + assertEquals(listOf(Container.MP3), containers) + // Still only the one audio click. `MP3` is an AudioCodec label too, and the container row + // must not be reporting through that callback. + assertEquals(listOf(AudioCodec.COPY), audioCodecs) + } + + // --- the error card, which is outside the gate -------------------------- + + /** + * The headline case. Dropping both tracks is reachable from the collapsed screen -- the + * `None`/`None` pair is set inside Advanced, but the user can close it again -- and the + * explanation has to still be there. + */ + @Test + fun `an empty output explains itself while the section is collapsed`() { + val spec = OutputSpec(Container.MP4, VideoCodec.NONE, AudioCodec.NONE) + val invalid = invalidFor(spec) + + assertEquals("This would produce an empty file — keep at least one track.", invalid.message) + + setPicker(spec, invalid) + + assertPanelHidden() + composeRule.onNodeWithTag(TestTags.Converter.VALIDATION_ERROR).assertExists() + composeRule.onNodeWithText(invalid.message).assertIsDisplayed() + } + + @Test + fun `a codec the container cannot hold explains itself while the section is collapsed`() { + val spec = OutputSpec(Container.WEBM, VideoCodec.H264, AudioCodec.OPUS) + val invalid = invalidFor(spec) + + assertEquals("WebM cannot hold H.264 video.", invalid.message) + + setPicker(spec, invalid) + + assertPanelHidden() + composeRule.onNodeWithText(invalid.message).assertIsDisplayed() + } + + /** + * Clicking a suggestion, with the toggle never touched. + * + * The second suggestion rather than the first, and its count pinned first: with one suggestion + * a picker that handed every chip `suggestions[0]` would pass, and `onNodeWithTag` on a + * suggestion index that no longer exists reports an unhelpful matcher failure rather than + * saying the list shrank. + */ + @Test + fun `a suggestion chip applies its own spec without the section ever being opened`() { + val spec = OutputSpec(Container.WEBM, VideoCodec.H264, AudioCodec.OPUS) + val invalid = invalidFor(spec) + + assertEquals(2, invalid.suggestions.size) + val second = invalid.suggestions[1] + + setPicker(spec, invalid) + + assertPanelHidden() + composeRule.onNodeWithTag(TestTags.Converter.suggestion(1)).assertTextEquals(describe(second)) + composeRule.onNodeWithTag(TestTags.Converter.suggestion(1)).performClick() + + assertEquals(listOf(second), applied) + // What the chips offer is what `validate` said would work, not a repair of the test's own. + assertTrue( + "suggestion $second should itself validate", + ContainerCapabilities.validate(second, PROBE).isValid, + ) + } + + /** A valid spec has nothing to say, collapsed or not. */ + @Test + fun `a valid spec renders no error card`() { + setPicker() + + composeRule.onNodeWithTag(TestTags.Converter.VALIDATION_ERROR).assertDoesNotExist() + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + + composeRule.onNodeWithTag(TestTags.Converter.VALIDATION_ERROR).assertDoesNotExist() + } + + // --- recreation --------------------------------------------------------- + + /** + * `expanded` is the only `rememberSaveable` on either screen's leaves. + * + * `MainActivity` declares no `configChanges`, so a rotation destroys and rebuilds the whole + * composition. A panel the user opened, set three chips in, and left open must not close + * itself on the way back. `remember` would. + * + * What this cannot see is the saved *representation* -- `StateRestorationTester` saves into an + * in-memory map rather than a `Bundle`. `AdvancedPanelSavedStateTest` covers that half. + */ + @Test + fun `an open panel is still open after recreation`() { + restoration.setContent { + AdvancedPicker( + spec = VALID_SPEC, + validation = Validation.Valid, + onContainer = {}, + onVideoCodec = {}, + onAudioCodec = {}, + onSuggestion = {}, + ) + } + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE).performClick() + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_PANEL).assertExists() + + restoration.emulateSavedInstanceStateRestore() + + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_PANEL).assertExists() + ROW_TAGS.forEach { composeRule.onNodeWithTag(it).assertExists() } + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_TOGGLE) + .assertTextEquals("Hide advanced") + } + + /** The default has to survive too, or the panel would spring open on every rotation. */ + @Test + fun `a collapsed panel is still collapsed after recreation`() { + restoration.setContent { + AdvancedPicker( + spec = VALID_SPEC, + validation = Validation.Valid, + onContainer = {}, + onVideoCodec = {}, + onAudioCodec = {}, + onSuggestion = {}, + ) + } + + assertPanelHidden() + + restoration.emulateSavedInstanceStateRestore() + + assertPanelHidden() + } + + // --- helpers ------------------------------------------------------------ + + private fun setPicker(spec: OutputSpec = VALID_SPEC, validation: Validation = Validation.Valid) { + composeRule.setContent { + AdvancedPicker( + spec = spec, + validation = validation, + onContainer = { containers += it }, + onVideoCodec = { videoCodecs += it }, + onAudioCodec = { audioCodecs += it }, + onSuggestion = { applied += it }, + ) + } + } + + /** The whole panel, by every tag it owns, so a partial escape counts as a failure. */ + private fun assertPanelHidden() { + composeRule.onNodeWithTag(TestTags.Converter.ADVANCED_PANEL).assertDoesNotExist() + ROW_TAGS.forEach { composeRule.onNodeWithTag(it).assertDoesNotExist() } + } + + private fun nodeCount(tag: String) = composeRule.onAllNodesWithTag(tag).fetchSemanticsNodes().size + + private fun chipIn(rowTag: String, label: String) = + composeRule.onNode(hasText(label) and hasAnyAncestor(hasTestTag(rowTag))) + + private fun invalidFor(spec: OutputSpec): Validation.Invalid { + val validation = ContainerCapabilities.validate(spec, PROBE) + return validation as? Validation.Invalid + ?: throw AssertionError("$spec was expected to be invalid, but validate said $validation") + } + + private companion object { + val ROW_TAGS = listOf( + TestTags.Converter.ADVANCED_CONTAINER_CHIPS, + TestTags.Converter.ADVANCED_VIDEO_CHIPS, + TestTags.Converter.ADVANCED_AUDIO_CHIPS, + ) + + val VALID_SPEC = OutputSpec(Container.MP4, VideoCodec.H264, AudioCodec.AAC) + + /** An ordinary H.264/AAC MP4, so the suggestions have a real source to repair towards. */ + val PROBE = InputProbe( + videoCodec = "h264", + audioCodec = "aac", + durationMs = 90_000, + container = Container.MP4, + ) + } +} From 6cd17f25aa01c3d20527add32a9313d522bf13ef Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 16:13:05 -0500 Subject: [PATCH 4/4] Pin shellcheck, because the unpinned one disagreed with the local run The step added in the previous commit went red on its own PR, and the reason is the one CLAUDE.md already gives for pinning ktlint, detekt and JaCoCo: "a new rule in a linter makes files nobody touched stop passing, so CI goes red on a PR whose diff cannot explain it." Here it was not even a new rule, just a different version of the same tool. The runner's ambient shellcheck is 0.9.0. The container used to check locally was 0.11.0. They disagree about how to report `on_signal`, which is installed as the INT and TERM trap eleven lines below its declaration and so is never called by name: 0.11.0 SC2329, once, on the function declaration -- "never invoked" 0.9.0 SC2317, seven times, one per command in the body -- "appears to be unreachable" The disable directive named SC2329, so 0.11.0 was silent and 0.9.0 reported seven findings. Nothing about the script was wrong; the local check simply was not the check CI ran. Two changes, because either alone still leaves a way to be surprised: - CI runs shellcheck from an image pinned by digest, so an upgrade is a line in this file that someone chose, not something that arrives on a Tuesday. The version is still printed, so a finding out of nowhere can be tied to that line. - The directive names SC2317 and SC2329 both, so a contributor whose distro ships 0.9.0 gets the same answer locally as CI gives. Verified against both images: clean under 0.9.0 and clean under 0.11.0. CLAUDE.md now says to check with the pinned digest rather than with whatever is installed, which is what would have caught this before the push. --- .github/workflows/status_check.yml | 23 +++++++++++++++++------ CLAUDE.md | 12 ++++++++++-- tools/local-emulator/run-e2e.sh | 5 ++++- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/.github/workflows/status_check.yml b/.github/workflows/status_check.yml index d555764..1c95fff 100644 --- a/.github/workflows/status_check.yml +++ b/.github/workflows/status_check.yml @@ -160,16 +160,27 @@ jobs: # entry point itself -- and nothing was checking it. `git ls-files` rather than a # fixed list, so a script added later is covered without editing this workflow. # - # Full severity, `info` included. The two findings it raises today are answered - # with targeted `disable` directives carrying their reason, the same way + # Full severity, `info` included. The findings it raises today are answered with + # targeted `disable` directives carrying their reason, the same way # config/detekt/detekt.yml carries only the rules this codebase legitimately # breaks. A blanket --severity=warning would have hidden them and the next real - # one alike. shellcheck is preinstalled on the ubuntu runner image; the version is - # printed so a finding that appears out of nowhere can be pinned to an upgrade. + # one alike. + # + # PINNED BY DIGEST, for the reason CLAUDE.md already gives for pinning ktlint, + # detekt and JaCoCo: a new rule in a linter makes files nobody touched stop + # passing, so CI goes red on a PR whose diff cannot explain it. That is not + # hypothetical here. The first cut of this step used the runner's ambient + # shellcheck, which is 0.9.0, and 0.9.0 reports a trap handler as seven + # unreachable commands (SC2317) where 0.11.0 reports it once on the declaration + # (SC2329) -- same script, same directive, different answer, and a red build on + # the PR that introduced the step. The version is printed so a finding that + # appears out of nowhere can be tied to a bump of this line. - name: shellcheck + env: + SHELLCHECK: koalaman/shellcheck@sha256:61862eba1fcf09a484ebcc6feea46f1782532571a34ed51fedf90dd25f925a8d run: | - shellcheck --version - git ls-files -z '*.sh' | xargs -0 -r shellcheck + docker run --rm "$SHELLCHECK" --version + git ls-files -z '*.sh' | xargs -0 -r docker run --rm -v "$PWD:/mnt" "$SHELLCHECK" # `!cancelled()` rather than a plain sequence: a shellcheck failure above must not # cost the ktlint/detekt/lint lists. Same reason this step passes --continue -- one diff --git a/CLAUDE.md b/CLAUDE.md index efe7fc1..33d0ff9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -142,8 +142,16 @@ install for code that can never run — and on API 37 the full APK does not fit two findings that raises today are answered with targeted `disable` directives carrying their reason, exactly as `config/detekt/detekt.yml` carries only the rules this codebase legitimately breaks. Do not silence it with `--severity=warning` — that hides the next real finding too. - Locally there is no shellcheck package installed; `podman run --rm -v "$PWD:/mnt:z" - docker.io/koalaman/shellcheck:stable ` is what was used. + **It is pinned by image digest, and joins ktlint/detekt/JaCoCo in the "Dependency versions" + rule above** — for exactly the reason stated there, demonstrated the day it was added. The first + cut used the runner's ambient shellcheck. That is **0.9.0**, while the container used to check + locally was 0.11.0, and the two disagree about how to report a trap handler: 0.11.0 says + `SC2329` once on the declaration, 0.9.0 says `SC2317` on each of seven lines in the body. Same + script, same directive, one green and one red. Directives that must survive both name both codes. + + Locally, use the same pin rather than whatever is installed: + `podman run --rm -v "$PWD:/mnt:z" docker.io/koalaman/shellcheck@sha256:61862eba... ` + (the digest is in `status_check.yml`; there is no shellcheck system package on this host). **It does not cover inline `run:` blocks in the workflows**, and a good deal of this repo's bash lives there. `actionlint` does cover them — it runs shellcheck over each `run:` — and reports one diff --git a/tools/local-emulator/run-e2e.sh b/tools/local-emulator/run-e2e.sh index 5e13dc8..ed34159 100755 --- a/tools/local-emulator/run-e2e.sh +++ b/tools/local-emulator/run-e2e.sh @@ -456,7 +456,10 @@ cleanup() { # bash runs a trap only between commands, so this starts when whatever was in the foreground # returns -- which for Ctrl-C is immediately, because the same interrupt reached that command # too. `kill -INT` aimed at this script alone waits for the foreground command to finish. -# shellcheck disable=SC2329 # invoked indirectly -- installed as the INT and TERM trap a few lines below. +# Invoked indirectly -- installed as the INT and TERM trap a few lines below. Both codes, +# because shellcheck 0.9.0 reports this as unreachable commands (SC2317) and 0.11.0 as an +# uninvoked function (SC2329); CI pins 0.11.0 but a local install may be either. +# shellcheck disable=SC2317,SC2329 on_signal() { echo echo "interrupted (SIG$1) -- stopping the emulator and removing the AVDs this run created"