From baaaa934e0cc0826aecf2fb8d818e0919caef3ea Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 16:03:14 -0500 Subject: [PATCH 1/2] 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 6cd17f25aa01c3d20527add32a9313d522bf13ef Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 24 Aug 2026 16:13:05 -0500 Subject: [PATCH 2/2] 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"