OpsDeck/mobile
Watch
2
0
Fork
You've already forked mobile
 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions

build: the five actionable notes from the #6 review #9

Merged
julian merged 9 commits from chore/pr-check-cost-and-spotless-scope into main 2026-09-02 13:05:18 +02:00
Conversation 6 Commits 9 Files changed 6 +93 -155
julian commented 2026-08-24 16:44:29 +02:00
Owner
Copy link

Closes #8 items 1–5. Nothing here is a fix to #6 — that landed correct. These are the notes the approving review left beside it, all cost, scope or noise rather than correctness.

Four commits, one per note (items 2 and 3 share one because they are the same file pair), plus five rework commits from three review rounds. dde4f70 is a reworded a8d1df2 — force-pushed, tree byte-identical, see the last section for why.

  • 59ae565 + b8a4808 + 2e96246 + ba496e5 — concurrency and types: on the PR gate. Neither changes what runs today, and the first version of this PR claimed otherwise. types: [opened, synchronize, reopened] is exactly the platform default; pinning it only guards against a future default change widening the trigger. What the explicit concurrency block certainly adds is a named per-PR group (github.ref is refs/pull/<n>/head under pull_request on this instance). The comment has now been through four passes on one sentence — it says Forgejo cancels a superseded push run with no group declared, cites #295/#296 and #323/#324 as the evidence and labels both as event: push on main, and marks both remaining halves — synchronize, and opened/reopened — as assumed on the same unmeasured evidence. Every cancelled run this repo has ever had is a push run — all four — and no pull_request run here has ever been superseded, so nothing measures either.
  • f71b16d + a53bb8a + 5da20e0 + ba496e5 — the CRLF paragraph, and the -text types not tracked yet. .gitattributes fixes a fresh clone; it cannot fix a tree that already exists, because git does not re-filter unchanged files. The paragraph exists so the next person does not read the rule as broken. It has taken four passes to get right and the current one is reproduced end to end — see Verification. It warns before the command instead of wrapping it in git stash, it names the wrapper as a thing not to re-add, and it now says which clone this is even about: one made with core.autocrlf=true. On a Linux or macOS default the pre-.gitattributes checkout was already LF, so without that clause a reader there could follow the paragraph into the one command in this repo that discards uncommitted work while chasing a problem they cannot have. Separately, -text grows to png/webp/jpg/jpeg/jks/keystore — none tracked today, so nothing gets renormalized by adding them.
  • c456369 — spotless moves from :app to the root. It was configured inside app/build.gradle.kts targeting src/**/*.kt, so the rule stopped at one module's sources: build scripts exempt, and a second module exempt by default. Now allprojects at the root with the same pinned ktlint("1.8.0") and the same pinned layout, plus a kotlinGradle target relative to each project dir. Honest scope: today the kotlinGradle target guards two import lines in one file; settings.gradle.kts and app/build.gradle.kts have no imports at all. The LF check and any module added later are what it is for.
  • dde4f70 — twelve lint baseline entries that CI can never match. Not "dead" — that was wrong, and it was the subject line, which is why the commit got reworded rather than corrected in a follow-up. See below.

Verification

Real ./gradlew runs against the local Android SDK (JDK 21 here, CI is 17). spotlessCheck, lintDebug and testReleaseUnitTest all pass. Run #679 was green on 2e96246; the two commits since touch one Markdown paragraph and one YAML comment block, and the workflow still parses to the same on: and concurrency: values it did on fc4c762 — checked by parsing the file, not by reading it.

The CRLF recipe is reproduced, wrapper and all. git 2.55. git clone -c core.autocrlf=true --no-checkout, checkout 20985db^ (before .gitattributes), then checkout this head: 91 files land w/crlf, gradlew's shebang is #!/bin/sh^M, and git status --porcelain returns zero paths — the text=auto eol=lf clean filter normalizes CRLF to LF before comparing. That is the whole trap, and it is a stronger reason to keep the paragraph than the one it replaced.

From there, each line of the old recipe:

  • git stash → No local changes to save, no entry created.
  • git rm --cached -r . && git reset --hard → fixes it: 97 w/lf, 1 w/crlf (the .bat, as .gitattributes intends), gradlew shebang #!/bin/sh.
  • git stash pop → No stash entries found., exit 1. With an unrelated entry on the stack, it instead applies and drops it — planted one, watched it land in the working tree and vanish from git stash list.

And the claim that used to justify the paragraph: git add --renormalize . leaves 91 files CRLF before and after, with zero status paths before and after. There was never an M for it to clear.

The .kts half is enforced, not decoration. Hostile test: swap the two imports at the top of the root script and :spotlessKotlinGradleCheck fails with exactly that reorder —

> The following files had format violations:
      @@ -1,5 +1,5 @@
      +import·com.diffplug.gradle.spotless.SpotlessExtension
       import·com.diffplug.spotless.LineEnding
      -import·com.diffplug.gradle.spotless.SpotlessExtension

Four spotless task pairs now run where main runs one (:app:spotlessKotlin{,Check}): :spotlessKotlin{,Check}, :spotlessKotlinGradle{,Check}, :app:spotlessKotlin{,Check}, :app:spotlessKotlinGradle{,Check}. No reflow — every tracked file already satisfied the rule, so the diff carries no .kt or .kts content change.

The Gradle 9 deprecation warnings are not mine. spotlessCheck --warning-mode all --no-configuration-cache prints the same three is--property warnings before and after; all three come from AGP (isCrunchPngs, isUseProguard, isWearAppUnbundled). --no-configuration-cache matters: a stored config-cache entry suppresses them and the head looks like it has zero.

The twelve lint baseline entries are a no-op in CI and noisier locally — not a no-op everywhere. The earlier claim ("an absolute path into another machine's checkout", "changes nothing except the noise") was wrong on both halves, and review was right. Their <location file=...> is $HOME/Projects/git.imhof.cloud/OpsDeck/mobile/… — a $HOME-relative path that lint expands at read time, not a fixed absolute one. Reproduced by putting main's 17-entry baseline into a worktree at exactly that path:

baseline checkout lint reports filtered listed-but-not-found
main (17) anywhere else / CI 13 warnings 2 errors, 2 warnings, 1 hint 12
main (17) $HOME/Projects/git.imhof.cloud/OpsDeck/mobile 5 warnings 2 errors, 10 warnings, 1 hint 4
this PR (5) either 13 warnings 2 errors, 2 warnings, 1 hint 0

So 8 of the 12 do match where the path resolves, and the other 4 are dead because their version strings moved on. Removing them changes nothing in CI, and un-silences 8 dependency-freshness warnings in that one checkout. No gate risk either way: all 8 are warnings, abortOnError = true, no warningsAsErrors.

Removed rather than regenerated on purpose: updateLintBaselineDebug writes 18 entries, not 5. It would swallow the 13 warnings CI currently shows, including a ModifierParameter finding nobody has triaged. The five kept entries use project-relative src/main/… paths, match everywhere, and are byte-identical.

The two nits from the approval, taken

Both were explicitly "take it whenever, not now"; both are one clause, and both are in ba496e5.

  • pr-checks.yml — "extends cancel-in-progress to opened and reopened, which the implicit default does not cover" was the last flat assertion about implicit behaviour left in that comment, and it rests on exactly the evidence the synchronize half two lines above already calls assumed: no opened or reopened run in this repo has ever been superseded either. It now says so rather than claiming the platform does something unmeasured.
  • CLAUDE.md — the paragraph did not say which clone gets a stale tree. Scoped to core.autocrlf=true, with the reason a Linux or macOS reader should stop reading there.

Not in here

Item 6 from #8 — branch protection requiring the check status — is a repo setting, not a diff, and it decides whether merges get blocked. That's a call for a human, not a side effect of this PR.

Items 7 and 8 are known-no-action: the gate tests the branch rather than the merge result, and the cache-warming claim in #6's description did not hold in practice.

Two security findings review raised against this file want their own issue, not a scope grab here: pull_request on a self-hosted runner in a public repo is arbitrary code execution from any fork PR, and the cmdline-tools zip is fetched with no checksum. Both landed in #6 and are filed as #12; this PR only edits comments beside them.

Follow-ups the review named, deliberately left out: the 13 dependency-freshness warnings still print on every CI run — lint { disable += "GradleDependency" } is the lever for a quiet gate. allprojects { apply(...) } is cross-project configuration — fine under org.gradle.configuration-cache (verified, still stores an entry), but a convention plugin is the move if isolated projects ever gets turned on.

Answered

concurrency landed in Forgejo v14.0; this instance is 16.0.3. github.ref under pull_request is refs/pull/<n>/head (run #663's own checkout step), so the group is per PR. What stays unproven is that cancellation fires for any pull_request type with the explicit block in place — which matters less than first thought, because it already fires for push without it. The comment now says so for every type rather than only for synchronize.

Why a8d1df2 was reworded rather than corrected in a new commit: the wrong claim was its subject line ("entries that never matched anything"), and this branch merges with a merge commit, so it would have gone into main's history as-is. Tree is unchanged — git diff between the pre- and post-rebase heads is empty.

No versionName bump: nothing user-visible.

🤖 Generated with Claude Code

Closes #8 items 1–5. Nothing here is a fix to #6 — that landed correct. These are the notes the approving review left beside it, all cost, scope or noise rather than correctness. Four commits, one per note (items 2 and 3 share one because they are the same file pair), plus five rework commits from three review rounds. **`dde4f70` is a reworded `a8d1df2`** — force-pushed, tree byte-identical, see the last section for why. - **`59ae565` + `b8a4808` + `2e96246` + `ba496e5` — `concurrency` and `types:` on the PR gate.** Neither changes what runs today, and the first version of this PR claimed otherwise. `types: [opened, synchronize, reopened]` is exactly the platform default; pinning it only guards against a future default change widening the trigger. What the explicit `concurrency` block certainly adds is a named per-PR group (`github.ref` is `refs/pull/<n>/head` under `pull_request` on this instance). The comment has now been through four passes on one sentence — it says Forgejo cancels a superseded **push** run with no group declared, cites #295/#296 and #323/#324 as the evidence *and* labels both as `event: push` on main, and marks *both* remaining halves — `synchronize`, and `opened`/`reopened` — as assumed on the same unmeasured evidence. Every cancelled run this repo has ever had is a push run — all four — and no `pull_request` run here has ever been superseded, so nothing measures either. - **`f71b16d` + `a53bb8a` + `5da20e0` + `ba496e5` — the CRLF paragraph, and the `-text` types not tracked yet.** `.gitattributes` fixes a *fresh* clone; it cannot fix a tree that already exists, because git does not re-filter unchanged files. The paragraph exists so the next person does not read the rule as broken. It has taken four passes to get right and the current one is reproduced end to end — see Verification. It warns before the command instead of wrapping it in `git stash`, it names the wrapper as a thing not to re-add, and it now says which clone this is even about: one made with `core.autocrlf=true`. On a Linux or macOS default the pre-`.gitattributes` checkout was already LF, so without that clause a reader there could follow the paragraph into the one command in this repo that discards uncommitted work while chasing a problem they cannot have. Separately, `-text` grows to png/webp/jpg/jpeg/jks/keystore — none tracked today, so nothing gets renormalized by adding them. - **`c456369` — spotless moves from `:app` to the root.** It was configured inside `app/build.gradle.kts` targeting `src/**/*.kt`, so the rule stopped at one module's sources: build scripts exempt, and a second module exempt by default. Now `allprojects` at the root with the same pinned `ktlint("1.8.0")` and the same pinned layout, plus a `kotlinGradle` target relative to each project dir. Honest scope: today the `kotlinGradle` target guards two import lines in one file; `settings.gradle.kts` and `app/build.gradle.kts` have no imports at all. The LF check and any module added later are what it is for. - **`dde4f70` — twelve lint baseline entries that CI can never match.** Not "dead" — that was wrong, and it was the subject line, which is why the commit got reworded rather than corrected in a follow-up. See below. ## Verification Real `./gradlew` runs against the local Android SDK (JDK 21 here, CI is 17). `spotlessCheck`, `lintDebug` and `testReleaseUnitTest` all pass. Run #679 was green on `2e96246`; the two commits since touch one Markdown paragraph and one YAML comment block, and the workflow still parses to the same `on:` and `concurrency:` values it did on `fc4c762` — checked by parsing the file, not by reading it. **The CRLF recipe is reproduced, wrapper and all.** git 2.55. `git clone -c core.autocrlf=true --no-checkout`, checkout `20985db^` (before `.gitattributes`), then checkout this head: **91 files land `w/crlf`**, `gradlew`'s shebang is `#!/bin/sh^M`, and **`git status --porcelain` returns zero paths** — the `text=auto eol=lf` clean filter normalizes CRLF to LF before comparing. That is the whole trap, and it is a stronger reason to keep the paragraph than the one it replaced. From there, each line of the old recipe: - `git stash` → `No local changes to save`, no entry created. - `git rm --cached -r . && git reset --hard` → fixes it: 97 `w/lf`, 1 `w/crlf` (the `.bat`, as `.gitattributes` intends), `gradlew` shebang `#!/bin/sh`. - `git stash pop` → `No stash entries found.`, **exit 1**. With an unrelated entry on the stack, it instead applies and drops it — planted one, watched it land in the working tree and vanish from `git stash list`. And the claim that used to justify the paragraph: `git add --renormalize .` leaves **91 files CRLF before and after**, with **zero status paths before and after**. There was never an `M` for it to clear. **The `.kts` half is enforced, not decoration.** Hostile test: swap the two imports at the top of the root script and `:spotlessKotlinGradleCheck` fails with exactly that reorder — ``` > The following files had format violations: @@ -1,5 +1,5 @@ +import·com.diffplug.gradle.spotless.SpotlessExtension import·com.diffplug.spotless.LineEnding -import·com.diffplug.gradle.spotless.SpotlessExtension ``` Four spotless task pairs now run where `main` runs one (`:app:spotlessKotlin{,Check}`): `:spotlessKotlin{,Check}`, `:spotlessKotlinGradle{,Check}`, `:app:spotlessKotlin{,Check}`, `:app:spotlessKotlinGradle{,Check}`. No reflow — every tracked file already satisfied the rule, so the diff carries no `.kt` or `.kts` content change. **The Gradle 9 deprecation warnings are not mine.** `spotlessCheck --warning-mode all --no-configuration-cache` prints the same three `is-`-property warnings before and after; all three come from AGP (`isCrunchPngs`, `isUseProguard`, `isWearAppUnbundled`). `--no-configuration-cache` matters: a stored config-cache entry suppresses them and the head looks like it has zero. **The twelve lint baseline entries are a no-op in CI and noisier locally — not a no-op everywhere.** The earlier claim ("an absolute path into another machine's checkout", "changes nothing except the noise") was wrong on both halves, and review was right. Their `<location file=...>` is `$HOME/Projects/git.imhof.cloud/OpsDeck/mobile/…` — a **`$HOME`-relative path that lint expands at read time**, not a fixed absolute one. Reproduced by putting `main`'s 17-entry baseline into a worktree at exactly that path: | baseline | checkout | lint reports | filtered | listed-but-not-found | |---|---|---|---|---| | `main` (17) | anywhere else / CI | 13 warnings | 2 errors, 2 warnings, 1 hint | 12 | | `main` (17) | `$HOME/Projects/git.imhof.cloud/OpsDeck/mobile` | **5 warnings** | **2 errors, 10 warnings, 1 hint** | **4** | | this PR (5) | either | 13 warnings | 2 errors, 2 warnings, 1 hint | 0 | So 8 of the 12 do match where the path resolves, and the other 4 are dead because their version strings moved on. Removing them changes nothing in CI, and un-silences 8 dependency-freshness warnings in that one checkout. No gate risk either way: all 8 are warnings, `abortOnError = true`, no `warningsAsErrors`. Removed rather than regenerated **on purpose**: `updateLintBaselineDebug` writes 18 entries, not 5. It would swallow the 13 warnings CI currently shows, including a `ModifierParameter` finding nobody has triaged. The five kept entries use project-relative `src/main/…` paths, match everywhere, and are byte-identical. ## The two nits from the approval, taken Both were explicitly "take it whenever, not now"; both are one clause, and both are in `ba496e5`. - **`pr-checks.yml`** — "extends cancel-in-progress to opened and reopened, which the implicit default does not cover" was the last flat assertion about implicit behaviour left in that comment, and it rests on exactly the evidence the `synchronize` half two lines above already calls assumed: no `opened` or `reopened` run in this repo has ever been superseded either. It now says so rather than claiming the platform does something unmeasured. - **`CLAUDE.md`** — the paragraph did not say which clone gets a stale tree. Scoped to `core.autocrlf=true`, with the reason a Linux or macOS reader should stop reading there. ## Not in here Item 6 from #8 — branch protection requiring the `check` status — is a repo setting, not a diff, and it decides whether merges get blocked. That's a call for a human, not a side effect of this PR. Items 7 and 8 are known-no-action: the gate tests the branch rather than the merge result, and the cache-warming claim in #6's description did not hold in practice. Two security findings review raised against this file want their own issue, not a scope grab here: `pull_request` on a self-hosted runner in a public repo is arbitrary code execution from any fork PR, and the cmdline-tools zip is fetched with no checksum. Both landed in #6 and are filed as #12; this PR only edits comments beside them. Follow-ups the review named, deliberately left out: the 13 dependency-freshness warnings still print on every CI run — `lint { disable += "GradleDependency" }` is the lever for a quiet gate. `allprojects { apply(...) }` is cross-project configuration — fine under `org.gradle.configuration-cache` (verified, still stores an entry), but a convention plugin is the move if isolated projects ever gets turned on. ## Answered `concurrency` landed in Forgejo v14.0; this instance is 16.0.3. `github.ref` under `pull_request` is `refs/pull/<n>/head` (run #663's own checkout step), so the group is per PR. What stays unproven is that cancellation fires for any `pull_request` type with the explicit block in place — which matters less than first thought, because it already fires for `push` without it. The comment now says so for every type rather than only for `synchronize`. Why `a8d1df2` was reworded rather than corrected in a new commit: the wrong claim was its subject line ("entries that never matched anything"), and this branch merges with a merge commit, so it would have gone into `main`'s history as-is. Tree is unchanged — `git diff` between the pre- and post-rebase heads is empty. No `versionName` bump: nothing user-visible. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
The PR gate is ~4m30s cold on the one self-hosted runner, and nothing
serialised it: three pushes to a branch started three full runs in
parallel, with the only one anyone would read queued behind two that
were already obsolete.

`concurrency` with cancel-in-progress keyed on the ref keeps one run per
PR. `types:` pins the three events that actually change the diff rather
than trusting the platform default not to fire on a label or title edit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`.gitattributes` fixes a fresh clone: `git clone -c core.autocrlf=true`
gets 0 of 71 .kt files CRLF. It cannot fix a tree that was already
checked out, because git does not re-filter unchanged files — those keep
CRLF on every file no later commit touches, which fails spotlessCheck
and stops ./gradlew from starting at all. That reads as a broken rule
unless the fix is written down next to the command, so it is:
`git add --renormalize .`, once.

The `-text` list also grows to cover png/webp/jpg/jks/keystore. None are
tracked today; an Android repo acquires them, and marking a type is
cheaper before a normalization pass has touched one than after.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Spotless was configured inside app/build.gradle.kts targeting
src/**/*.kt, so the rule stopped at one module's sources: the build
scripts were exempt from it, and a second module would have been exempt
by default — inheriting the rule would have meant remembering to repeat
the block.

Configured once at the root under allprojects instead, with the same
pinned ktlint 1.8.0 and the same pinned imports layout, plus a
kotlinGradle target for the .gradle.kts files. The target is relative to
each project dir, so the root picks up build.gradle.kts and
settings.gradle.kts and every module picks up its own.

Verified the .kts half is enforced and not decoration: swapping the two
imports at the top of the root script fails :spotlessKotlinGradleCheck
with exactly that reorder. No reflow — every tracked file already
satisfies the rule.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
build: drop the twelve lint baseline entries that never matched anything
All checks were successful
PR Checks / check (pull_request) Successful in 2m1s
Details
a8d1df227c
 
Every PR run now prints "12 errors/warnings were listed in the baseline
file but not found in the project" — 12 of the 17 entries are dead.

They are the 2 AndroidGradlePluginVersion and 10 GradleDependency
entries, and they are dead for one reason: their <location file=...> is
an absolute path into another machine's checkout
($HOME/Projects/git.imhof.cloud/...), so they never matched here. Lint
reports those same 12 findings today as unfiltered warnings, which is
why removing them changes nothing except the noise: before and after,
lint filters "2 errors, 2 warnings and 1 hint" and reports 13 warnings.

Removed rather than regenerated on purpose. `updateLintBaseline` writes
18 entries, not 5 — it would swallow the 13 warnings currently visible,
including a ModifierParameter finding nobody has ever triaged. The five
real entries are kept byte-identical.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
julian commented 2026-08-24 16:55:07 +02:00
Author
Owner
Copy link

Closing the loop on the open question in the description: run #663 is green on a8d1df2, event: pull_request, 2m1s.

So on Forgejo 16.0.3 the workflow with concurrency and types: parses and dispatches — the change is not inert and did not break the gate. What run #663 cannot prove is that cancel-in-progress actually cancels, since one push produces one run and there was nothing to cancel. That needs two pushes in quick succession to demonstrate; I have not manufactured one.

Incidental data point for #8 item 8: #663 took 2m1s against #655's 4m31s, so the caches did hit this time. One warm run after one cold run is not a pattern, and it still doesn't show the branch→main direction the original claim rested on — just worth recording next to the "assume ~4m30s cold" note rather than leaving the impression the cache never works.

Closing the loop on the open question in the description: **run #663 is green on `a8d1df2`, `event: pull_request`, 2m1s.** So on Forgejo 16.0.3 the workflow with `concurrency` and `types:` parses and dispatches — the change is not inert and did not break the gate. What run #663 *cannot* prove is that `cancel-in-progress` actually cancels, since one push produces one run and there was nothing to cancel. That needs two pushes in quick succession to demonstrate; I have not manufactured one. Incidental data point for #8 item 8: #663 took 2m1s against #655's 4m31s, so the caches did hit this time. One warm run after one cold run is not a pattern, and it still doesn't show the branch→main direction the original claim rested on — just worth recording next to the "assume ~4m30s cold" note rather than leaving the impression the cache never works.
thisilike requested changes 2026-08-24 19:46:18 +02:00
Dismissed
thisilike left a comment
Copy link

Re-reviewed a8d1df2 in a detached worktree with a real Android SDK and ran every claim instead of reading it. The mechanical work is good and the verification section is unusually honest — the lint arithmetic reproduces to the character. One thing is wrong, and it is the single line in this diff a human will actually type.

Blocking

CLAUDE.md: git add --renormalize . does not fix the tree it is prescribed for. Full repro inline. Short version: --renormalize rewrites the index and never the working tree, and every blob in HEAD is already LF (git grep -I -l $'\r' HEAD -- . returns nothing), so in the stale tree it is a no-op with a side effect — it clears the M from git status and leaves ./gradlew dying on bad interpreter: /bin/sh^M. The paragraph exists so the next person does not read the rule as broken; as written it hands them a clean status and a build that still will not start. git rm --cached -r . && git reset --hard is what works (verified) — and it discards uncommitted changes, so the sentence has to say so.

Two comments that state the platform does something it does not

Both inline, neither harmful to behaviour, both wrong on the merits:

  • types: is exactly the Forgejo/GitHub default. It saves zero runs; the label/title-edit scenario cannot happen.
  • Forgejo already defaults cancel-in-progress to true for push and pull_request (synchronize) with no group declared. "Three pushes mean three full cold runs at once" has never been possible here, and this repo's own history proves it: #295 and #323 are cancelled, each superseded ~2 minutes in by the next push. #8 item 1 rests on the same invented cost.

Both blocks are still worth keeping — pin the events, declare the group explicitly. Just say what they actually buy.

The open question in the description is answered

github.ref under pull_request on this instance is refs/pull/<n>/head. Run #663's own checkout step:

git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin \
  +a8d1df227c21e74d3dcf5ed17f19463a4801a955:refs/remotes/pull/9/head

So pr-${{ github.ref }} is genuinely per-PR and the github.event.pull_request.number swap is not needed. concurrency is supported: it landed in Forgejo v14.0, this is 16.0.3, and the Actions reference lists github among the contexts allowed in concurrency.group. What remains unproven is only that cancellation fires — which matters less than you thought, because it already fired without the block.

Verified and true

Re-ran all of it; the description is accurate here.

  • spotlessCheck passes on the head. Four format/check pairs run where main runs one: :app:spotlessKotlin{,Check}, :spotlessKotlin{,Check}, :app:spotlessKotlinGradle{,Check}, :spotlessKotlinGradle{,Check}. (main runs :app:spotlessKotlin + :app:spotlessKotlinCheck only — one pair, not the two the description claims. Cosmetic.)
  • The hostile test reproduces verbatim. Swapping the two root imports fails :spotlessKotlinGradleCheck with exactly the diff you quoted. The .kts half is enforced.
  • Deprecation warnings: identical three AGP is- properties (getCrunchPngs, getUseProguard, getWearAppUnbundled) before and after. Note for anyone re-checking — you have to pass --no-configuration-cache, otherwise a stored config-cache entry suppresses them and the head appears to have zero.
  • Lint, head: Lint found 13 warnings (and 2 errors, 2 warnings and 1 hint filtered by baseline lint-baseline.xml). Swap main's baseline back in and it is the same 13 and the same 5, plus 12 errors/warnings were listed in the baseline file … but not found in the project. Removing them is exactly a no-op on what lint reports. The 13 are 10 GradleDependency + 2 AndroidGradlePluginVersion + 1 ModifierParameter — the 12 dead entries plus the untriaged one, so the argument against updateLintBaselineDebug holds too.
  • The five kept entries all use project-relative paths (src/main/...), so unlike the twelve they actually match. Keeping them byte-identical is meaningful, not decorative.
  • abortOnError = true with no warningsAsErrors, so the 13 unfiltered warnings cannot turn the gate red.
  • .gitattributes: nothing matching png/webp/jpg/jpeg/jks/keystore is tracked, so "not tracked yet" is correct and nothing gets renormalized by adding them.
  • testReleaseUnitTest and compileDebugKotlin pass. org.gradle.configuration-cache=true still stores an entry with the allprojects block in place.
  • Entry count: the base file has 17 issues, not the 18 #8 item 5 claims. The PR body has it right; the issue is the wrong one.

Follow-ups, not for this PR

  • The baseline cleanup removes the "12 not found" line but leaves 13 dependency-freshness warnings printed on every run. If a quiet gate was the goal, lint { disable += "GradleDependency" } is the lever; the dead entries were never what printed them.
  • allprojects { apply(...) } is cross-project configuration. Fine under org.gradle.configuration-cache (verified), but it is exactly what isolated projects forbids — a convention plugin is the move if that ever gets turned on.
  • The kotlinGradle target currently guards two import lines in one file; settings.gradle.kts and app/build.gradle.kts have no imports at all. The LF check and the future module are the real value — worth not overselling.
  • .gitattributes gains *.jpeg, which the description's "png/webp/jpg/jks/keystore" list omits.
Re-reviewed `a8d1df2` in a detached worktree with a real Android SDK and ran every claim instead of reading it. The mechanical work is good and the verification section is unusually honest — the lint arithmetic reproduces to the character. One thing is wrong, and it is the single line in this diff a human will actually type. ## Blocking **`CLAUDE.md`: `git add --renormalize .` does not fix the tree it is prescribed for.** Full repro inline. Short version: `--renormalize` rewrites the *index* and never the working tree, and every blob in `HEAD` is already LF (`git grep -I -l $'\r' HEAD -- .` returns nothing), so in the stale tree it is a no-op with a side effect — it clears the `M` from `git status` and leaves `./gradlew` dying on `bad interpreter: /bin/sh^M`. The paragraph exists so the next person does not read the rule as broken; as written it hands them a clean status and a build that still will not start. `git rm --cached -r . && git reset --hard` is what works (verified) — and it discards uncommitted changes, so the sentence has to say so. ## Two comments that state the platform does something it does not Both inline, neither harmful to behaviour, both wrong on the merits: - `types:` is exactly the Forgejo/GitHub default. It saves zero runs; the label/title-edit scenario cannot happen. - Forgejo already defaults `cancel-in-progress` to true for `push` and `pull_request` (synchronize) with no group declared. "Three pushes mean three full cold runs at once" has never been possible here, and this repo's own history proves it: #295 and #323 are `cancelled`, each superseded ~2 minutes in by the next push. #8 item 1 rests on the same invented cost. Both blocks are still worth keeping — pin the events, declare the group explicitly. Just say what they actually buy. ## The open question in the description is answered `github.ref` under `pull_request` on this instance is `refs/pull/<n>/head`. Run #663's own checkout step: ``` git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=1 origin \ +a8d1df227c21e74d3dcf5ed17f19463a4801a955:refs/remotes/pull/9/head ``` So `pr-${{ github.ref }}` is genuinely per-PR and the `github.event.pull_request.number` swap is not needed. `concurrency` is supported: it landed in Forgejo v14.0, this is 16.0.3, and the Actions reference lists `github` among the contexts allowed in `concurrency.group`. What remains unproven is only that cancellation fires — which matters less than you thought, because it already fired without the block. ## Verified and true Re-ran all of it; the description is accurate here. - `spotlessCheck` passes on the head. Four format/check pairs run where `main` runs one: `:app:spotlessKotlin{,Check}`, `:spotlessKotlin{,Check}`, `:app:spotlessKotlinGradle{,Check}`, `:spotlessKotlinGradle{,Check}`. (`main` runs `:app:spotlessKotlin` + `:app:spotlessKotlinCheck` only — one pair, not the two the description claims. Cosmetic.) - The hostile test reproduces verbatim. Swapping the two root imports fails `:spotlessKotlinGradleCheck` with exactly the diff you quoted. The `.kts` half is enforced. - Deprecation warnings: identical three AGP `is-` properties (`getCrunchPngs`, `getUseProguard`, `getWearAppUnbundled`) before and after. Note for anyone re-checking — you have to pass `--no-configuration-cache`, otherwise a stored config-cache entry suppresses them and the head appears to have zero. - Lint, head: `Lint found 13 warnings (and 2 errors, 2 warnings and 1 hint filtered by baseline lint-baseline.xml)`. Swap `main`'s baseline back in and it is the same 13 and the same 5, plus `12 errors/warnings were listed in the baseline file … but not found in the project`. Removing them is exactly a no-op on what lint reports. The 13 are 10 `GradleDependency` + 2 `AndroidGradlePluginVersion` + 1 `ModifierParameter` — the 12 dead entries plus the untriaged one, so the argument against `updateLintBaselineDebug` holds too. - The five kept entries all use project-relative paths (`src/main/...`), so unlike the twelve they actually match. Keeping them byte-identical is meaningful, not decorative. - `abortOnError = true` with no `warningsAsErrors`, so the 13 unfiltered warnings cannot turn the gate red. - `.gitattributes`: nothing matching png/webp/jpg/jpeg/jks/keystore is tracked, so "not tracked yet" is correct and nothing gets renormalized by adding them. - `testReleaseUnitTest` and `compileDebugKotlin` pass. `org.gradle.configuration-cache=true` still stores an entry with the `allprojects` block in place. - Entry count: the base file has 17 issues, not the 18 #8 item 5 claims. The PR body has it right; the issue is the wrong one. ## Follow-ups, not for this PR - The baseline cleanup removes the "12 not found" line but leaves 13 dependency-freshness warnings printed on every run. If a quiet gate was the goal, `lint { disable += "GradleDependency" }` is the lever; the dead entries were never what printed them. - `allprojects { apply(...) }` is cross-project configuration. Fine under `org.gradle.configuration-cache` (verified), but it is exactly what isolated projects forbids — a convention plugin is the move if that ever gets turned on. - The `kotlinGradle` target currently guards two import lines in one file; `settings.gradle.kts` and `app/build.gradle.kts` have no imports at all. The LF check and the future module are the real value — worth not overselling. - `.gitattributes` gains `*.jpeg`, which the description's "png/webp/jpg/jks/keystore" list omits.
.forgejo/workflows/pr-checks.yml Outdated
@ -7,1 +7,4 @@
# The three events that change the diff, pinned rather than left to the
# platform default: a run costs ~4m30s cold, so a default that also fires on
# label or title edits would buy one for nothing.
thisilike commented 2026-08-24 19:46:18 +02:00
Owner
Copy link

The comment asserts something the platform does not do. opened, synchronize, reopened is the default — on GitHub by documentation, and on Forgejo by implementation: matchPullRequestEvent with no types: matches exactly HookIssueOpened || HookIssueReOpened || HookIssueSynchronized, and HookIssueSynchronized is mapped to the string synchronize. A label or title edit has never started a run in this repo, so this line saves nothing.

No objection to pinning it — an explicit set is a reasonable hedge against a future default change, and synchronize (not synchronized) is the spelling Forgejo wants, so pushes keep triggering the gate. But the justification in the comment is false, and comments in this repo get read as fact.

The comment asserts something the platform does not do. `opened, synchronize, reopened` **is** the default — on GitHub by documentation, and on Forgejo by implementation: `matchPullRequestEvent` with no `types:` matches exactly `HookIssueOpened || HookIssueReOpened || HookIssueSynchronized`, and `HookIssueSynchronized` is mapped to the string `synchronize`. A label or title edit has never started a run in this repo, so this line saves nothing. No objection to pinning it — an explicit set is a reasonable hedge against a future default change, and `synchronize` (not `synchronized`) is the spelling Forgejo wants, so pushes keep triggering the gate. But the justification in the comment is false, and comments in this repo get read as fact.
.forgejo/workflows/pr-checks.yml Outdated
@ -10,0 +14,4 @@
# One runner, self-hosted. Without this, three pushes to a PR mean three full
# cold runs at once and the newest — the only one anyone will read — queues
# behind two that are already obsolete.
thisilike commented 2026-08-24 19:46:18 +02:00
Owner
Copy link

Same problem, larger. "three pushes to a PR mean three full cold runs at once" is not something that can happen on this instance. Forgejo already defaults cancel-in-progress to true for push and pull_request (synchronize) when no concurrency.group is declared — "any previous invocation of the same workflow with the same branch will be canceled".

The repo's own run history shows it working, twice: #295 cancelled after 1m54s with #296 starting 2m06s later; #323 cancelled after 1m33s with #324 starting 1m46s later. Nobody cancelled those by hand.

Keep the block — it makes the group explicit rather than implicit, and an explicit cancel-in-progress: true also covers opened/reopened, which the default does not. That is the real (small) benefit. As written the comment invents a cost the change never removed, and #8 item 1 rests on the same invented cost.

Support itself is fine: concurrency landed in Forgejo v14.0 (Jan 2026), this instance is 16.0.3, and the Actions reference lists github among the contexts allowed in concurrency.group.

Same problem, larger. "three pushes to a PR mean three full cold runs at once" is not something that can happen on this instance. Forgejo already defaults `cancel-in-progress` to true for `push` and `pull_request` (synchronize) when no `concurrency.group` is declared — "any previous invocation of the same workflow with the same branch will be canceled". The repo's own run history shows it working, twice: #295 `cancelled` after 1m54s with #296 starting 2m06s later; #323 `cancelled` after 1m33s with #324 starting 1m46s later. Nobody cancelled those by hand. Keep the block — it makes the group explicit rather than implicit, and an explicit `cancel-in-progress: true` also covers `opened`/`reopened`, which the default does not. That is the real (small) benefit. As written the comment invents a cost the change never removed, and #8 item 1 rests on the same invented cost. Support itself is fine: concurrency landed in Forgejo v14.0 (Jan 2026), this instance is 16.0.3, and the Actions reference lists `github` among the contexts allowed in `concurrency.group`.
CLAUDE.md Outdated
@ -39,0 +39,4 @@
A clone made before `.gitattributes` existed can still hold CRLF in the files
no later commit touched — git does not re-filter unchanged files on checkout —
which fails `spotlessCheck` and stops `./gradlew` from starting at all. Run
`git add --renormalize .` once in that tree; it is not a broken rule.
thisilike commented 2026-08-24 19:46:18 +02:00
Owner
Copy link

git add --renormalize . cannot fix this tree. It applies the clean filter and rewrites the index; it never touches the working tree. And every blob in HEAD here is already LF —

$ git grep -I -l $'\r' HEAD -- .
(nothing)

— so the clean filter maps the CRLF working copy back onto the identical LF blob, the index does not change, there is nothing to stage, commit or check out, and the CRLF stays on disk. spotlessCheck still fails and ./gradlew still will not start.

Reproduced against the exact shape #8 item 2 describes (tree created first, * text=auto eol=lf committed after, working copies then set to CRLF):

--- git status before ---
 M f.kt
 M gradlew
--- run: git add --renormalize . ---
--- git status after ---
(clean)
--- worktree AFTER renormalize ---
f.kt:    ASCII text, with CRLF line terminators
gradlew: POSIX shell script, ASCII text executable, with CRLF line terminators
--- ./gradlew ---
./gradlew: bad interpreter: /bin/sh^M: no such file or directory

So it is worse than inert: it clears the one signal that something is wrong (M in git status) and leaves the build broken. Whoever follows this sentence ends up with a clean status and a ./gradlew that still does not start — which is precisely the "reads as a broken rule" outcome the paragraph exists to prevent.

What actually works, same repro:

git rm --cached -r . && git reset --hard
f.kt:    ASCII text
gradlew: POSIX shell script, ASCII text executable
hi

git reset --hard discards uncommitted changes to tracked files — the sentence has to say "stash first" or it will cost someone their work in progress. I also tried git add --renormalize . && git checkout -- . and git checkout-index -f -a; both leave the CRLF in place. Deleting the tree and re-cloning is the other honest answer.

Worth adding: #8's alternative, ./gradlew spotlessApply, cannot work either — gradlew is itself one of the CRLF files.

`git add --renormalize .` cannot fix this tree. It applies the clean filter and rewrites the **index**; it never touches the working tree. And every blob in `HEAD` here is already LF — ``` $ git grep -I -l $'\r' HEAD -- . (nothing) ``` — so the clean filter maps the CRLF working copy back onto the identical LF blob, the index does not change, there is nothing to stage, commit or check out, and the CRLF stays on disk. `spotlessCheck` still fails and `./gradlew` still will not start. Reproduced against the exact shape #8 item 2 describes (tree created first, `* text=auto eol=lf` committed after, working copies then set to CRLF): ``` --- git status before --- M f.kt M gradlew --- run: git add --renormalize . --- --- git status after --- (clean) --- worktree AFTER renormalize --- f.kt: ASCII text, with CRLF line terminators gradlew: POSIX shell script, ASCII text executable, with CRLF line terminators --- ./gradlew --- ./gradlew: bad interpreter: /bin/sh^M: no such file or directory ``` So it is worse than inert: it clears the one signal that something is wrong (`M` in `git status`) and leaves the build broken. Whoever follows this sentence ends up with a clean status and a `./gradlew` that still does not start — which is precisely the "reads as a broken rule" outcome the paragraph exists to prevent. What actually works, same repro: ``` git rm --cached -r . && git reset --hard f.kt: ASCII text gradlew: POSIX shell script, ASCII text executable hi ``` **`git reset --hard` discards uncommitted changes to tracked files — the sentence has to say "stash first" or it will cost someone their work in progress.** I also tried `git add --renormalize . && git checkout -- .` and `git checkout-index -f -a`; both leave the CRLF in place. Deleting the tree and re-cloning is the other honest answer. Worth adding: #8's alternative, `./gradlew spotlessApply`, cannot work either — `gradlew` is itself one of the CRLF files.
`git add --renormalize .` applies the clean filter and rewrites the index;
it never touches the working tree. Every blob in HEAD is already LF, so in
a stale clone it maps the CRLF working copy back onto the identical blob,
changes nothing on disk, and clears the `M` from `git status` that was the
only sign something was wrong. `./gradlew` still dies on
`bad interpreter: /bin/sh^M`. `git checkout -- .` and
`git checkout-index -f -a` leave the CRLF too, and `spotlessApply` cannot
run because `gradlew` is one of the CRLF files.

`git rm --cached -r . && git reset --hard` empties the index and re-smudges
every tracked file (verified: `git ls-files --eol` goes from `w/crlf` to
`w/lf`). It discards uncommitted changes to tracked files, so the snippet
stashes first and pops after; the round trip keeps the work in progress and
lands it as LF.

Review finding on #9.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ci: say what concurrency and types actually buy
All checks were successful
PR Checks / check (pull_request) Successful in 1m57s
Details
fc4c7627ae
 
Both comments claimed a cost the platform never charged. `types:
[opened, synchronize, reopened]` is exactly the Forgejo and GitHub default,
so pinning it saves no runs; it only guards against a future default
change. And Forgejo already cancels a superseded run for push and
synchronize with no group declared — this repo's #295 and #323 were
cancelled by the next push — so three parallel cold runs per PR were never
possible. What the explicit block adds is a named group and
cancel-in-progress on opened and reopened.

No behaviour change; comments only. Also records that `github.ref` is
`refs/pull/<n>/head` under `pull_request` on this instance, which closes
the open question in the PR description.

Review finding on #9.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
julian commented 2026-08-24 19:56:05 +02:00
Author
Owner
Copy link

Reworked on fc4c762 — two commits on top of the reviewed head, no code change, both are the words.

Blocking — CLAUDE.md (c1370a8). Reproduced your repro before touching it: scratch repo on git 2.55, tree committed first, * text=auto eol=lf after, working copies set to CRLF. git ls-files --eol stays i/lf w/crlf after git add --renormalize ., after git checkout-index -f -a and after git checkout -- .; goes to i/lf w/lf after git rm --cached -r . && git reset --hard, and ./gradlew runs. The paragraph now names --renormalize and checkout -- . as non-fixes and says why (every HEAD blob already LF; --renormalize also clears the M, which is worse than inert), says spotlessApply cannot help because gradlew is CRLF, and gives

git stash                                 # the next line discards uncommitted changes
git rm --cached -r . && git reset --hard  # empty the index, re-smudge every file
git stash pop

with a fresh clone as the alternative. Checked the stash round trip too: a CRLF-terminated edit in progress survives and lands as LF. Plain git stash is enough — reset --hard only touches tracked files, so untracked work is not at risk.

types: comment (fc4c762). Now says it is exactly the platform default, pinned so a future default change cannot widen the trigger, and that it saves no runs today. The invented label/title-edit cost is gone.

concurrency comment (fc4c762). Now says Forgejo already cancels superseded push/synchronize runs with no group declared (cites #295/#296 and #323/#324), that the block makes the group explicit and extends cancel-in-progress to opened/reopened, and records github.ref = refs/pull/<n>/head so the per-PR claim is not left to the reader.

Description. Rewritten to own all of it: the concurrency bullet no longer claims a cost that never existed and says #8 item 1 rested on the same mistake; the CRLF bullet tells the renormalize story straight; "four pairs where there were two" is now "where main runs one"; *.jpeg is in the list; the kotlinGradle target's actual coverage today (two import lines in one file) is stated rather than oversold; --no-configuration-cache is noted next to the deprecation-warning check; your follow-ups (GradleDependency disable, convention plugin under isolated projects) are recorded as deliberately out of scope.

Verification on the rework. spotlessCheck passes on fc4c762; the workflow still parses to the same on: and concurrency: values. Did not re-run lint/tests — the diff since a8d1df2 is one Markdown paragraph and two YAML comment blocks.

Follow-ups. Left out as you suggested. Also correcting #8 in a comment there: item 1's premise and item 5's entry count (17, not 18).

Requesting re-review.

Reworked on `fc4c762` — two commits on top of the reviewed head, no code change, both are the words. **Blocking — `CLAUDE.md` (c1370a8).** Reproduced your repro before touching it: scratch repo on git 2.55, tree committed first, `* text=auto eol=lf` after, working copies set to CRLF. `git ls-files --eol` stays `i/lf w/crlf` after `git add --renormalize .`, after `git checkout-index -f -a` and after `git checkout -- .`; goes to `i/lf w/lf` after `git rm --cached -r . && git reset --hard`, and `./gradlew` runs. The paragraph now names `--renormalize` and `checkout -- .` as non-fixes and says *why* (every `HEAD` blob already LF; `--renormalize` also clears the `M`, which is worse than inert), says `spotlessApply` cannot help because `gradlew` is CRLF, and gives ```sh git stash # the next line discards uncommitted changes git rm --cached -r . && git reset --hard # empty the index, re-smudge every file git stash pop ``` with a fresh clone as the alternative. Checked the stash round trip too: a CRLF-terminated edit in progress survives and lands as LF. Plain `git stash` is enough — `reset --hard` only touches tracked files, so untracked work is not at risk. **`types:` comment (fc4c762).** Now says it is exactly the platform default, pinned so a future default change cannot widen the trigger, and that it saves no runs today. The invented label/title-edit cost is gone. **`concurrency` comment (fc4c762).** Now says Forgejo already cancels superseded `push`/`synchronize` runs with no group declared (cites #295/#296 and #323/#324), that the block makes the group explicit and extends `cancel-in-progress` to `opened`/`reopened`, and records `github.ref` = `refs/pull/<n>/head` so the per-PR claim is not left to the reader. **Description.** Rewritten to own all of it: the concurrency bullet no longer claims a cost that never existed and says #8 item 1 rested on the same mistake; the CRLF bullet tells the renormalize story straight; "four pairs where there were two" is now "where `main` runs one"; `*.jpeg` is in the list; the `kotlinGradle` target's actual coverage today (two import lines in one file) is stated rather than oversold; `--no-configuration-cache` is noted next to the deprecation-warning check; your follow-ups (`GradleDependency` disable, convention plugin under isolated projects) are recorded as deliberately out of scope. **Verification on the rework.** `spotlessCheck` passes on `fc4c762`; the workflow still parses to the same `on:` and `concurrency:` values. Did not re-run lint/tests — the diff since `a8d1df2` is one Markdown paragraph and two YAML comment blocks. **Follow-ups.** Left out as you suggested. Also correcting #8 in a comment there: item 1's premise and item 5's entry count (17, not 18). Requesting re-review.
thisilike requested changes 2026-09-02 08:31:47 +02:00
Dismissed
thisilike left a comment
Copy link

Re-reviewed fc4c762 by running it: real ./gradlew in throwaway worktrees against the local Android SDK (JDK 21 here, CI is 17), plus the CI log for run #666.

The mechanical work is right and the note-by-note coverage is faithful — I checked your transcription against my own #6 review and all eight notes are there, grouped correctly, nothing invented or dropped. Four of the five actionable ones are done. The fifth is still wrong, and two of the rationales this PR exists to correct are themselves inaccurate.

Blocking

CLAUDE.md:51,53 — the stash wrapper is wrong in the default case and destructive in a common one. I was wrong in my last review to accept the shape of this paragraph without running the whole recipe; the middle line is correct, the wrapper is not.

Reproduced: git clone -c core.autocrlf=true at the pre-.gitattributes commit, then checkout this head. 56 files w/crlf, gradlew shebang #!/bin/sh\r\n — and git status --porcelain returns zero paths, because the text=auto eol=lf clean filter normalizes CRLF→LF on comparison. So:

  • git stash prints No local changes to save and creates no entry.
  • git rm --cached -r . && git reset --hard fixes the tree (97 w/lf, LF gradlew).
  • git stash pop then fails with No stash entries found. (exit 1) — or, if the user has any unrelated stash entry, silently applies and drops it. I planted one and watched it land in the index and vanish from git stash list.

The paragraph exists so the next person does not read the rule as broken. As written it ends in a spurious error, or in someone else's WIP appearing in their tree.

Make stashing a precondition rather than a step: "commit or stash anything you care about first — the next line discards uncommitted changes", then the single git rm --cached -r . && git reset --hard.

CLAUDE.md:44-46 — the justification is false. "--renormalize also clears the M from git status that was the only sign something was wrong": there is no M. git status --porcelain is 0 paths before and after git add --renormalize ., with all 56 files still CRLF. That was my error originally and this PR inherited it. The true version is a stronger argument for the paragraph: nothing in git status ever signals the problem, which is exactly why it reads as a broken rule.

Non-blocking

.forgejo/workflows/pr-checks.yml:15-19 still claims more than the evidence carries. "Forgejo already cancels a superseded run for push and synchronize with no group declared (this repo: #295 cancelled by #296, #323 by #324)" — both cited pairs are event: push runs on main. No pull_request run in this repo has ever been superseded; the four that exist are 20+ minutes apart. The body concedes cancellation is unproven, so the comment should too: scope the parenthetical to push and mark the synchronize half as inferred. My own note c and #8 item 1 were the original source of this over-claim, so this is the third pass on the same sentence — worth getting exact.

The lint baseline entries were not dead, and removing them is not "exactly a no-op". Lint resolves $HOME in baseline paths, and the twelve deleted entries point at $HOME/Projects/git.imhof.cloud/OpsDeck/mobile/… — my own primary checkout, not "another machine's". I put main's baseline into a tree under $HOME with those paths repointed at it: lint goes from 13 warnings (2 errors, 2 warnings and 1 hint filtered) to 5 warnings (2 errors, 10 warnings and 1 hint filtered). Eight of the twelve match when the path resolves; the other four are dead because their version strings moved on, not because of the path.

So the change is a no-op in CI and in a detached worktree — which is all either of us measured — but it makes local lint noisier, the opposite of what the note wanted. No gate risk: all eight are warnings, and abortOnError = true with no warningsAsErrors. Just fix the claim to "no-op in CI".

Pre-existing, not this PR, but this PR is editing the trigger: public repo + pull_request + self-hosted cth-ubuntu-latest running ./gradlew from the PR head is arbitrary code execution on the runner from any fork PR. The new comment reasons about a future default change "widening the trigger" while the live exposure goes unmentioned. Same file, the SDK step wgets the cmdline-tools zip with no checksum. Both landed in #6; they deserve an issue, not a scope grab here.

Verified and true

  • spotlessCheck green on the head. Four pairs run where main runs one — confirmed by --dry-run on both: :spotlessKotlin{,Check}, :spotlessKotlinGradle{,Check}, :app:spotlessKotlin{,Check}, :app:spotlessKotlinGradle{,Check} vs main's :app:spotlessKotlin{,Check}. "main runs one" is right.
  • The .kts half is enforced, and more broadly than you tested. Prepending out-of-order imports to settings.gradle.kts fails :spotlessKotlinGradleCheck; doing it to app/build.gradle.kts fails :app:spotlessKotlinGradleCheck. Both halves die.
  • lintDebug green, Lint found 13 warnings (and 2 errors, 2 warnings and 1 hint filtered), and the "12 not found" line is gone. The arithmetic reproduces exactly.
  • The five kept entries use project-relative src/main/... paths and do match. Keeping them byte-identical is meaningful. The argument against updateLintBaselineDebug holds.
  • types: does not break synchronize. Run #666 is green on this exact head (fc4c762, event: pull_request, refspec refs/remotes/pull/9/head) — so the pin is live and harmless, and github.ref really is per-PR. Worth noting the body still cites #663/a8d1df2 as the green run; #666 is the current one.
  • No version movement anywhere. gradle/libs.versions.toml untouched, spotless still 8.9.0, ktlint("1.8.0") and the layout map carried over byte-identical into both the kotlin and kotlinGradle blocks. No dynamic versions, no new repositories, no softened checks — the baseline removal un-silences findings rather than hiding them.
  • .gitattributes: nothing matching png/webp/jpg/jpeg/jks/keystore is tracked, so nothing renormalizes.
  • The merge is clean and green, which the gate cannot tell you (#8 item 7). The branch is 3 commits behind main; I merged cc702a6 in locally — clean, ort — and spotlessCheck, testReleaseUnitTest and lintDebug all pass with the same 13 warnings.

Fix the CLAUDE.md recipe and the two rationales and this is good to go.

Re-reviewed `fc4c762` by running it: real `./gradlew` in throwaway worktrees against the local Android SDK (JDK 21 here, CI is 17), plus the CI log for run #666. The mechanical work is right and the note-by-note coverage is faithful — I checked your transcription against my own #6 review and all eight notes are there, grouped correctly, nothing invented or dropped. Four of the five actionable ones are done. The fifth is still wrong, and two of the rationales this PR exists to correct are themselves inaccurate. ## Blocking **`CLAUDE.md:51,53` — the stash wrapper is wrong in the default case and destructive in a common one.** I was wrong in my last review to accept the shape of this paragraph without running the whole recipe; the middle line is correct, the wrapper is not. Reproduced: `git clone -c core.autocrlf=true` at the pre-`.gitattributes` commit, then checkout this head. 56 files `w/crlf`, `gradlew` shebang `#!/bin/sh\r\n` — and `git status --porcelain` returns **zero paths**, because the `text=auto eol=lf` clean filter normalizes CRLF→LF on comparison. So: - `git stash` prints `No local changes to save` and creates no entry. - `git rm --cached -r . && git reset --hard` fixes the tree (97 `w/lf`, LF `gradlew`). - `git stash pop` then fails with `No stash entries found.` (exit 1) — or, if the user has any unrelated stash entry, silently **applies and drops it**. I planted one and watched it land in the index and vanish from `git stash list`. The paragraph exists so the next person does not read the rule as broken. As written it ends in a spurious error, or in someone else's WIP appearing in their tree. Make stashing a precondition rather than a step: "commit or stash anything you care about first — the next line discards uncommitted changes", then the single `git rm --cached -r . && git reset --hard`. **`CLAUDE.md:44-46` — the justification is false.** "`--renormalize` also clears the `M` from `git status` that was the only sign something was wrong": there is no `M`. `git status --porcelain` is 0 paths before *and* after `git add --renormalize .`, with all 56 files still CRLF. That was my error originally and this PR inherited it. The true version is a stronger argument for the paragraph: nothing in `git status` ever signals the problem, which is exactly why it reads as a broken rule. ## Non-blocking **`.forgejo/workflows/pr-checks.yml:15-19` still claims more than the evidence carries.** "Forgejo already cancels a superseded run for push and synchronize with no group declared (this repo: #295 cancelled by #296, #323 by #324)" — both cited pairs are `event: push` runs on main. No `pull_request` run in this repo has ever been superseded; the four that exist are 20+ minutes apart. The body concedes cancellation is unproven, so the comment should too: scope the parenthetical to `push` and mark the `synchronize` half as inferred. My own note c and #8 item 1 were the original source of this over-claim, so this is the third pass on the same sentence — worth getting exact. **The lint baseline entries were not dead, and removing them is not "exactly a no-op".** Lint resolves `$HOME` in baseline paths, and the twelve deleted entries point at `$HOME/Projects/git.imhof.cloud/OpsDeck/mobile/…` — my own primary checkout, not "another machine's". I put main's baseline into a tree under `$HOME` with those paths repointed at it: lint goes from `13 warnings (2 errors, 2 warnings and 1 hint filtered)` to `5 warnings (2 errors, 10 warnings and 1 hint filtered)`. Eight of the twelve match when the path resolves; the other four are dead because their version strings moved on, not because of the path. So the change is a no-op in CI and in a detached worktree — which is all either of us measured — but it makes local lint noisier, the opposite of what the note wanted. No gate risk: all eight are warnings, and `abortOnError = true` with no `warningsAsErrors`. Just fix the claim to "no-op in CI". **Pre-existing, not this PR, but this PR is editing the trigger:** public repo + `pull_request` + self-hosted `cth-ubuntu-latest` running `./gradlew` from the PR head is arbitrary code execution on the runner from any fork PR. The new comment reasons about a future default change "widening the trigger" while the live exposure goes unmentioned. Same file, the SDK step `wget`s the cmdline-tools zip with no checksum. Both landed in #6; they deserve an issue, not a scope grab here. ## Verified and true - **`spotlessCheck` green** on the head. Four pairs run where `main` runs one — confirmed by `--dry-run` on both: `:spotlessKotlin{,Check}`, `:spotlessKotlinGradle{,Check}`, `:app:spotlessKotlin{,Check}`, `:app:spotlessKotlinGradle{,Check}` vs `main`'s `:app:spotlessKotlin{,Check}`. "main runs one" is right. - **The `.kts` half is enforced, and more broadly than you tested.** Prepending out-of-order imports to `settings.gradle.kts` fails `:spotlessKotlinGradleCheck`; doing it to `app/build.gradle.kts` fails `:app:spotlessKotlinGradleCheck`. Both halves die. - **`lintDebug` green**, `Lint found 13 warnings (and 2 errors, 2 warnings and 1 hint filtered)`, and the "12 not found" line is gone. The arithmetic reproduces exactly. - **The five kept entries** use project-relative `src/main/...` paths and do match. Keeping them byte-identical is meaningful. The argument against `updateLintBaselineDebug` holds. - **`types:` does not break `synchronize`.** Run #666 is green on this exact head (`fc4c762`, `event: pull_request`, refspec `refs/remotes/pull/9/head`) — so the pin is live and harmless, and `github.ref` really is per-PR. Worth noting the body still cites #663/`a8d1df2` as the green run; #666 is the current one. - **No version movement anywhere.** `gradle/libs.versions.toml` untouched, spotless still 8.9.0, `ktlint("1.8.0")` and the layout map carried over byte-identical into both the `kotlin` and `kotlinGradle` blocks. No dynamic versions, no new repositories, no softened checks — the baseline removal un-silences findings rather than hiding them. - **`.gitattributes`**: nothing matching png/webp/jpg/jpeg/jks/keystore is tracked, so nothing renormalizes. - **The merge is clean and green**, which the gate cannot tell you (#8 item 7). The branch is 3 commits behind main; I merged `cc702a6` in locally — clean, ort — and `spotlessCheck`, `testReleaseUnitTest` and `lintDebug` all pass with the same 13 warnings. Fix the `CLAUDE.md` recipe and the two rationales and this is good to go.
julian force-pushed chore/pr-check-cost-and-spotless-scope from fc4c7627ae
All checks were successful
PR Checks / check (pull_request) Successful in 1m57s
Details
to 2e962465a4
All checks were successful
PR Checks / check (pull_request) Successful in 5m39s
Details
2026-09-02 11:01:25 +02:00 Compare
julian commented 2026-09-02 11:07:52 +02:00
Author
Owner
Copy link

Both blocking items fixed, the non-blocking claim fixed, and the security pair split out to #12. You were right on all three, and on the lint one you were more right than the note said — see below.

Force-pushed: fc4c762 → 2e96246, and a8d1df2 → dde4f70 is a reword, not a rewrite. Tree is byte-identical across the rebase (git diff between the old and new heads is empty). Run #679 is green on 2e96246; spotlessCheck, lintDebug and testReleaseUnitTest also pass locally.

Blocking

CLAUDE.md — the stash wrapper is gone. Reproduced your repro before touching the paragraph. git 2.55, git clone -c core.autocrlf=true --no-checkout, checkout 20985db^, then checkout this head:

  • 91 files w/crlf, gradlew shebang #!/bin/sh^M, git status --porcelain → 0 paths.
  • git stash → No local changes to save, no entry.
  • git rm --cached -r . && git reset --hard → 97 w/lf, 1 w/crlf (the .bat, as intended), gradlew shebang #!/bin/sh.
  • git stash pop with an empty stack → No stash entries found., exit 1. With an unrelated entry planted first → applied it into the working tree and dropped it from git stash list. Watched it happen.

5da20e0 makes stashing a precondition in a comment above the single command, exactly as you suggested, and adds one sentence naming the wrapper as a thing not to re-add — this is the third pass on this paragraph and the next person should not have to rediscover why.

The M justification is gone too, and you are right that the truth is the stronger argument. git add --renormalize .: 91 files CRLF before and after, 0 status paths before and after. Nothing in git status ever signals the problem, which is precisely why the rule reads as broken. That is what the paragraph now says.

Non-blocking

The cancellation claim is scoped to push (2e96246). Confirmed your reading against the run list: this repo has exactly four cancelled runs ever — #138, #158, #295, #323 — and all four are event: push. No pull_request run has ever been superseded. The comment now cites the two pairs and labels them event: push on main, and marks the synchronize half as assumed and unmeasured.

The lint baseline claim was wrong in the commit subject, so the commit got reworded rather than patched. You are right that lint expands $HOME at read time and that "another machine's checkout" was false. Reproduced by dropping main's 17-entry baseline into a worktree at exactly $HOME/Projects/git.imhof.cloud/OpsDeck/mobile:

baseline checkout lint reports filtered listed-but-not-found
main (17) CI / anywhere else 13 warnings 2 errors, 2 warnings, 1 hint 12
main (17) that path 5 warnings 2 errors, 10 warnings, 1 hint 4

Your 8-of-12 number lands exactly. dde4f70 now says: no-op in CI, un-silences 8 dependency-freshness warnings in that one checkout, 4 genuinely dead on version drift, no gate risk. It also carries your lint { disable += "GradleDependency" } note as the actual lever for a quiet gate.

I reworded rather than adding a follow-up commit because the false claim was the subject line — "entries that never matched anything" — and this branch merges with a merge commit, so it would have gone into main's history as written. Sorry for the churn to any worktree you had off fc4c762.

Split out

#12 covers both security findings: pull_request on a self-hosted runner in a public repo is untrusted code execution from any fork PR, and the cmdline-tools zip is fetched with no checksum (same two lines exist in build.yml, which does touch the keystore). Neither is introduced here — this PR only edits comments beside them — and the fork-PR one needs a decision, not a patch, so it did not belong in this diff. Your point that the trigger comment reasons about a hypothetical future widening while ignoring the live exposure is quoted in the issue.

Not taken

The three follow-ups you marked as not-for-this-PR are still not in it: the GradleDependency disable, the convention-plugin move for isolated projects, and the kotlinGradle target's honest scope (which the description already states rather than oversells).

The description is rewritten against the current head, including the run number — you were right that it still cited #663/a8d1df2.

Both blocking items fixed, the non-blocking claim fixed, and the security pair split out to #12. You were right on all three, and on the lint one you were more right than the note said — see below. Force-pushed: `fc4c762` → `2e96246`, and `a8d1df2` → `dde4f70` is a reword, not a rewrite. Tree is byte-identical across the rebase (`git diff` between the old and new heads is empty). Run #679 is green on `2e96246`; `spotlessCheck`, `lintDebug` and `testReleaseUnitTest` also pass locally. ## Blocking **`CLAUDE.md` — the stash wrapper is gone.** Reproduced your repro before touching the paragraph. git 2.55, `git clone -c core.autocrlf=true --no-checkout`, checkout `20985db^`, then checkout this head: - 91 files `w/crlf`, `gradlew` shebang `#!/bin/sh^M`, `git status --porcelain` → **0 paths**. - `git stash` → `No local changes to save`, no entry. - `git rm --cached -r . && git reset --hard` → 97 `w/lf`, 1 `w/crlf` (the `.bat`, as intended), `gradlew` shebang `#!/bin/sh`. - `git stash pop` with an empty stack → `No stash entries found.`, exit 1. With an unrelated entry planted first → applied it into the working tree and dropped it from `git stash list`. Watched it happen. `5da20e0` makes stashing a precondition in a comment above the single command, exactly as you suggested, and adds one sentence naming the wrapper as a thing not to re-add — this is the third pass on this paragraph and the next person should not have to rediscover why. **The `M` justification is gone too, and you are right that the truth is the stronger argument.** `git add --renormalize .`: 91 files CRLF before and after, 0 status paths before and after. Nothing in `git status` ever signals the problem, which is precisely why the rule reads as broken. That is what the paragraph now says. ## Non-blocking **The cancellation claim is scoped to `push`** (`2e96246`). Confirmed your reading against the run list: this repo has exactly four cancelled runs ever — #138, #158, #295, #323 — and all four are `event: push`. No `pull_request` run has ever been superseded. The comment now cites the two pairs *and* labels them `event: push` on main, and marks the `synchronize` half as assumed and unmeasured. **The lint baseline claim was wrong in the commit subject, so the commit got reworded rather than patched.** You are right that lint expands `$HOME` at read time and that "another machine's checkout" was false. Reproduced by dropping `main`'s 17-entry baseline into a worktree at exactly `$HOME/Projects/git.imhof.cloud/OpsDeck/mobile`: | baseline | checkout | lint reports | filtered | listed-but-not-found | |---|---|---|---|---| | `main` (17) | CI / anywhere else | 13 warnings | 2 errors, 2 warnings, 1 hint | 12 | | `main` (17) | that path | **5 warnings** | **2 errors, 10 warnings, 1 hint** | **4** | Your 8-of-12 number lands exactly. `dde4f70` now says: no-op in CI, un-silences 8 dependency-freshness warnings in that one checkout, 4 genuinely dead on version drift, no gate risk. It also carries your `lint { disable += "GradleDependency" }` note as the actual lever for a quiet gate. I reworded rather than adding a follow-up commit because the false claim *was* the subject line — "entries that never matched anything" — and this branch merges with a merge commit, so it would have gone into `main`'s history as written. Sorry for the churn to any worktree you had off `fc4c762`. ## Split out #12 covers both security findings: `pull_request` on a self-hosted runner in a public repo is untrusted code execution from any fork PR, and the cmdline-tools zip is fetched with no checksum (same two lines exist in `build.yml`, which does touch the keystore). Neither is introduced here — this PR only edits comments beside them — and the fork-PR one needs a decision, not a patch, so it did not belong in this diff. Your point that the trigger comment reasons about a hypothetical future widening while ignoring the live exposure is quoted in the issue. ## Not taken The three follow-ups you marked as not-for-this-PR are still not in it: the `GradleDependency` disable, the convention-plugin move for isolated projects, and the `kotlinGradle` target's honest scope (which the description already states rather than oversells). The description is rewritten against the current head, including the run number — you were right that it still cited #663/`a8d1df2`.
thisilike approved these changes 2026-09-02 12:14:12 +02:00
thisilike left a comment
Copy link

Re-reviewed 2e96246 by running it rather than reading it: throwaway worktrees, real ./gradlew against the local Android SDK (JDK 21 here, CI is 17), and a fresh core.autocrlf=true clone for the CRLF recipe. Both blocking items are fixed and both fixes reproduce. Approving.

The two blockers, run-confirmed

CLAUDE.md:52-55 — the stash wrapper is gone and the single command is right. Reproduced the setup first: git clone -c core.autocrlf=true --no-checkout, checkout 20985db^, then this head — 91 files w/crlf, gradlew shebang #!/bin/sh\r\n, git status --porcelain zero paths, and ./gradlew exiting 126 with bad interpreter: /bin/sh^M. Then, with an unrelated entry planted on the stash stack, the prescribed line verbatim:

git rm --cached -r . && git reset --hard    → exit 0

97 w/lf, 1 w/crlf (gradlew.bat, as .gitattributes intends), shebang #!/bin/sh, ./gradlew --version → Gradle 8.14.3, and stash@{0} still on the stack, unapplied. The precondition holds for real work in progress too: a CRLF-terminated content edit is visible to git status, stashes, and comes back as LF after pop.

The :57-60 warning is true as written, and I checked both halves instead of taking it: pop on an empty stack exits 1 with No stash entries found.; with one unrelated entry it applied that entry into the working tree and dropped it from git stash list.

CLAUDE.md:42-49 — the M claim is gone, and the replacement is true rather than reworded. git add --renormalize .: 91 files CRLF before and after, 0 status paths before and after. git checkout -- .: identical. git grep -I -l $'\r' over the head tree returns nothing, so "every blob in HEAD is already LF" is right. The fails spotlessCheck half of the first sentence also holds — running the head's wrapper with -p at the stale tree gives :spotlessKotlinGradleCheck FAILED.

The non-blocking claims

Cancellation is now scoped to what the run list carries (pr-checks.yml:15-21). Checked against the runs, not the prose: exactly four cancelled runs exist in this repo — #138, #158, #295, #323 — all event: push; #296 and #324 start about two minutes after the runs they superseded, and cd39e66/31ff51e are both ancestors of main. Seven pull_request runs exist (#655, #663, #665, #666, #667, #678, #679) and none is cancelled. Accurate.

The lint baseline claim is correct, and the mechanism is now proven rather than inferred. dde4f70 says no-op in CI, un-silences 8 in that one checkout — both reproduce. This time I tested the $HOME expansion itself instead of repointing paths: a worktree at $HOME/.cache/opsdeck-r2-review/mobile, baseline entries written as the literal $HOME/.cache/opsdeck-r2-review/mobile/…, and lint reports 5 warnings (and 2 errors, 10 warnings and 1 hint filtered) with 4 … not found. So lint does expand the token at read time: 8 of 12 match, 4 are dead on version drift.

Methodology note for anyone re-checking, because it nearly talked me out of my own finding: overriding HOME= in the environment does not move this. OpenJDK takes user.home from the passwd entry, not from $HOME, so an env override reports the CI numbers (13 warnings, 12 not found) and looks like a disproof. Put the tree at a real path under the real home instead.

updateLintBaselineDebug does write 18 entries. With the baseline emptied, lint's full finding set is 2 errors, 15 warnings, 1 hint — exactly the 13 reported plus the 2 filtered.

The security pair is correctly out of scope. #12 states both findings accurately, including that the same unchecked wget sits in build.yml, which does touch the keystore. Nothing here widens the exposure: on: moves from bare pull_request to the three types that were already the default, so the effective trigger is unchanged.

Citations fixed. #679 is success on 2e96246, event: pull_request. a8d1df2 and dde4f70 are byte-identical trees (git diff between them is empty), so the reword claim holds.

Re-verified, no regressions

  • spotlessCheck, lintDebug and testReleaseUnitTest all green on 2e96246. Lint: 13 warnings (and 2 errors, 2 warnings and 1 hint filtered), and no "listed in the baseline … but not found" line.
  • Four spotless pairs on this head against one on main, compared by --dry-run on both.
  • The .kts half is enforced in both projects: out-of-order imports prepended to settings.gradle.kts fail :spotlessKotlinGradleCheck, and to app/build.gradle.kts fail :app:spotlessKotlinGradleCheck.
  • gradle/ untouched, spotless 8.9.0, both ktlint("1.8.0") pins and the layout map identical, no new repositories, FAIL_ON_PROJECT_REPOS intact, abortOnError = true with no warningsAsErrors, no secrets.
  • Nothing tracked matches the new -text patterns.
  • Deprecation warnings identical on this head and main under --warning-mode all --no-configuration-cache: the same three AGP is- properties.
  • The branch is behind main; merging cc702a6 locally is clean (ort) and green, with the same 13 warnings. main's only overlapping edit is versionName 0.31.0 → 0.31.1, and no spotless residue is left in app/build.gradle.kts.

Two nits, neither worth another pass

  • pr-checks.yml:19-20: "extends cancel-in-progress to opened and reopened, which the implicit default does not cover" is the last flat assertion about implicit behaviour in that comment, and it is as unmeasured here as the synchronize half two lines above that now says "assumed" — no opened/reopened run in this repo has ever been superseded either. Hedge it or drop the clause if the file is touched again; the sentence has had four passes and does not need a fifth for this.
  • CLAUDE.md:39: the paragraph does not say the stale tree only comes from a clone made with core.autocrlf=true. On a Linux or macOS default a pre-.gitattributes clone already holds LF, so a reader there can follow the paragraph and run the one command in this repo that discards uncommitted work while chasing a problem they cannot have. One clause fixes it — take it whenever, not now.
Re-reviewed `2e96246` by running it rather than reading it: throwaway worktrees, real `./gradlew` against the local Android SDK (JDK 21 here, CI is 17), and a fresh `core.autocrlf=true` clone for the CRLF recipe. Both blocking items are fixed and both fixes reproduce. Approving. ## The two blockers, run-confirmed **`CLAUDE.md:52-55` — the stash wrapper is gone and the single command is right.** Reproduced the setup first: `git clone -c core.autocrlf=true --no-checkout`, checkout `20985db^`, then this head — 91 files `w/crlf`, `gradlew` shebang `#!/bin/sh\r\n`, `git status --porcelain` **zero paths**, and `./gradlew` exiting 126 with `bad interpreter: /bin/sh^M`. Then, with an unrelated entry planted on the stash stack, the prescribed line verbatim: git rm --cached -r . && git reset --hard → exit 0 97 `w/lf`, 1 `w/crlf` (`gradlew.bat`, as `.gitattributes` intends), shebang `#!/bin/sh`, `./gradlew --version` → Gradle 8.14.3, and `stash@{0}` still on the stack, unapplied. The precondition holds for real work in progress too: a CRLF-terminated content edit is visible to `git status`, stashes, and comes back as LF after `pop`. The `:57-60` warning is true as written, and I checked both halves instead of taking it: `pop` on an empty stack exits 1 with `No stash entries found.`; with one unrelated entry it applied that entry into the working tree and dropped it from `git stash list`. **`CLAUDE.md:42-49` — the `M` claim is gone, and the replacement is true rather than reworded.** `git add --renormalize .`: 91 files CRLF before and after, 0 status paths before and after. `git checkout -- .`: identical. `git grep -I -l $'\r'` over the head tree returns nothing, so "every blob in `HEAD` is already LF" is right. The `fails spotlessCheck` half of the first sentence also holds — running the head's wrapper with `-p` at the stale tree gives `:spotlessKotlinGradleCheck FAILED`. ## The non-blocking claims **Cancellation is now scoped to what the run list carries** (`pr-checks.yml:15-21`). Checked against the runs, not the prose: exactly four cancelled runs exist in this repo — #138, #158, #295, #323 — all `event: push`; #296 and #324 start about two minutes after the runs they superseded, and `cd39e66`/`31ff51e` are both ancestors of `main`. Seven `pull_request` runs exist (#655, #663, #665, #666, #667, #678, #679) and none is cancelled. Accurate. **The lint baseline claim is correct, and the mechanism is now proven rather than inferred.** `dde4f70` says no-op in CI, un-silences 8 in that one checkout — both reproduce. This time I tested the `$HOME` expansion itself instead of repointing paths: a worktree at `$HOME/.cache/opsdeck-r2-review/mobile`, baseline entries written as the literal `$HOME/.cache/opsdeck-r2-review/mobile/…`, and lint reports `5 warnings (and 2 errors, 10 warnings and 1 hint filtered)` with `4 … not found`. So lint does expand the token at read time: 8 of 12 match, 4 are dead on version drift. Methodology note for anyone re-checking, because it nearly talked me out of my own finding: overriding `HOME=` in the environment does **not** move this. OpenJDK takes `user.home` from the passwd entry, not from `$HOME`, so an env override reports the CI numbers (13 warnings, 12 not found) and looks like a disproof. Put the tree at a real path under the real home instead. `updateLintBaselineDebug` does write 18 entries. With the baseline emptied, lint's full finding set is `2 errors, 15 warnings, 1 hint` — exactly the 13 reported plus the 2 filtered. **The security pair is correctly out of scope.** #12 states both findings accurately, including that the same unchecked `wget` sits in `build.yml`, which does touch the keystore. Nothing here widens the exposure: `on:` moves from bare `pull_request` to the three types that were already the default, so the effective trigger is unchanged. **Citations fixed.** #679 is `success` on `2e96246`, `event: pull_request`. `a8d1df2` and `dde4f70` are byte-identical trees (`git diff` between them is empty), so the reword claim holds. ## Re-verified, no regressions - `spotlessCheck`, `lintDebug` and `testReleaseUnitTest` all green on `2e96246`. Lint: `13 warnings (and 2 errors, 2 warnings and 1 hint filtered)`, and no "listed in the baseline … but not found" line. - Four spotless pairs on this head against one on `main`, compared by `--dry-run` on both. - The `.kts` half is enforced in both projects: out-of-order imports prepended to `settings.gradle.kts` fail `:spotlessKotlinGradleCheck`, and to `app/build.gradle.kts` fail `:app:spotlessKotlinGradleCheck`. - `gradle/` untouched, spotless 8.9.0, both `ktlint("1.8.0")` pins and the layout map identical, no new repositories, `FAIL_ON_PROJECT_REPOS` intact, `abortOnError = true` with no `warningsAsErrors`, no secrets. - Nothing tracked matches the new `-text` patterns. - Deprecation warnings identical on this head and `main` under `--warning-mode all --no-configuration-cache`: the same three AGP `is-` properties. - The branch is behind `main`; merging `cc702a6` locally is clean (ort) and green, with the same 13 warnings. `main`'s only overlapping edit is `versionName 0.31.0 → 0.31.1`, and no spotless residue is left in `app/build.gradle.kts`. ## Two nits, neither worth another pass - `pr-checks.yml:19-20`: "extends cancel-in-progress to opened and reopened, which the implicit default does not cover" is the last flat assertion about implicit behaviour in that comment, and it is as unmeasured here as the `synchronize` half two lines above that now says "assumed" — no `opened`/`reopened` run in this repo has ever been superseded either. Hedge it or drop the clause if the file is touched again; the sentence has had four passes and does not need a fifth for this. - `CLAUDE.md:39`: the paragraph does not say the stale tree only comes from a clone made with `core.autocrlf=true`. On a Linux or macOS default a pre-`.gitattributes` clone already holds LF, so a reader there can follow the paragraph and run the one command in this repo that discards uncommitted work while chasing a problem they cannot have. One clause fixes it — take it whenever, not now.
docs: hedge the last unmeasured cancellation clause, and scope the CRLF trap
All checks were successful
PR Checks / check (pull_request) Successful in 1m27s
Details
ba496e5cb8
 
The two nits from the approving review, neither of which needed another
round.

`pr-checks.yml`: "extends cancel-in-progress to opened and reopened, which
the implicit default does not cover" was the last flat assertion about
implicit behaviour left in that comment, and it rests on exactly the
evidence the synchronize half two lines above already calls assumed — no
opened or reopened run in this repo has ever been superseded either. It now
says so rather than claiming the platform does something unmeasured.

`CLAUDE.md`: the paragraph did not say which clone gets a stale tree. It is
the one made with `core.autocrlf=true`; on a Linux or macOS default the
pre-`.gitattributes` checkout was already LF, so a reader there could follow
the paragraph into the one command in this repo that discards uncommitted
work while chasing a problem they cannot have.

Comment and Markdown only. `spotlessCheck` green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
julian merged commit bfda2d6a23 into main 2026-09-02 13:05:18 +02:00
julian deleted branch chore/pr-check-cost-and-spotless-scope 2026-09-02 13:05:18 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
thisilike
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
OpsDeck/mobile!9
No description provided.