build: spotless import-order check, so review stops sorting imports by hand #6

julian · 2026-08-13T23:24:03+02:00

julian commented

2026-08-13 23:24:03 +02:00

Closes #3.

Of the two options in the issue, this is the first — the small rule with the small reflow — with one twist forced by the tooling: spotless has no importOrder() step for Kotlin (Java and Groovy only). The same shape is built from the other direction instead: the ktlint engine through spotless, with every standard rule disabled except import-ordering. It sorts imports and touches nothing else; the whole-style claim stays out, as the issue asked.

Three commits:

e298d41 — the rule: spotless 8.9.0 in the version catalog, applied in app, plus a spotlessCheck step in build.yml with line endings pinned to LF.
232c86e — the one-time reflow the issue priced in: mechanical spotlessApply output, nothing hand-written. 12 files. Eleven are pure import reorders; the twelfth, AnsiTerminal.kt, was the one file committed with CRLF, so its normalization to LF shows as a whole-file diff — git diff --ignore-all-space confirms it carries no other change.
20985db — review rework:
- pr-checks.yml: the no-secrets half of the pipeline — spotlessCheck, lintDebug, testReleaseUnitTest — now runs on pull_request. Before this, all three gates ran post-merge only, so the first scrambled import would have surfaced as a red release-and-publish build on main. Separate workflow rather than a second trigger on build.yml: branch builds never touch the keystore.
- .gitattributes: * text=auto eol=lf, with *.bat kept CRLF for cmd.exe and *.ttf/*.jar explicitly -text. Git now enforces the LF that lineEndings = UNIX assumed, so a stock Windows clone (core.autocrlf=true) no longer checks out CRLF everywhere and fails spotlessCheck on all files. gradlew.bat renormalizes once (LF in repo, CRLF on disk) — the same class of diff AnsiTerminal.kt paid in the reflow.
- ktlint("1.8.0") pinned, ij_kotlin_imports_layout pinned to *,java.**,javax.**,kotlin.**,^: a spotless bump carrying a new ktlint, or an IDE-exported .editorconfig, can no longer cost a second repo-wide reflow. spotlessCheck stays green with both pins in place — the pinned layout is the reflowed order.
- build.yml: spotlessCheck moved above the keystore steps (cheapest gate, needs no secrets); CLAUDE.md documents spotlessApply in Commands.

Not taken: reordering the rule/reflow commits for per-commit bisect green (review item 6) — priced in review as not worth a force-push.

spotlessCheck and compileDebugKotlin pass locally. No versionName bump: nothing user-visible.

🤖 Generated with Claude Code

Closes #3. Of the two options in the issue, this is the first — the small rule with the small reflow — with one twist forced by the tooling: spotless has no `importOrder()` step for Kotlin (Java and Groovy only). The same shape is built from the other direction instead: the ktlint engine through spotless, with **every standard rule disabled except `import-ordering`**. It sorts imports and touches nothing else; the whole-style claim stays out, as the issue asked. Three commits: - **e298d41** — the rule: spotless 8.9.0 in the version catalog, applied in `app`, plus a `spotlessCheck` step in `build.yml` with line endings pinned to LF. - **232c86e** — the one-time reflow the issue priced in: mechanical `spotlessApply` output, nothing hand-written. 12 files. Eleven are pure import reorders; the twelfth, `AnsiTerminal.kt`, was the one file committed with CRLF, so its normalization to LF shows as a whole-file diff — `git diff --ignore-all-space` confirms it carries no other change. - **20985db** — review rework: - `pr-checks.yml`: the no-secrets half of the pipeline — `spotlessCheck`, `lintDebug`, `testReleaseUnitTest` — now runs on `pull_request`. Before this, all three gates ran post-merge only, so the first scrambled import would have surfaced as a red release-and-publish build on main. Separate workflow rather than a second trigger on `build.yml`: branch builds never touch the keystore. - `.gitattributes`: `* text=auto eol=lf`, with `*.bat` kept CRLF for cmd.exe and `*.ttf`/`*.jar` explicitly `-text`. Git now enforces the LF that `lineEndings = UNIX` assumed, so a stock Windows clone (`core.autocrlf=true`) no longer checks out CRLF everywhere and fails `spotlessCheck` on all files. `gradlew.bat` renormalizes once (LF in repo, CRLF on disk) — the same class of diff `AnsiTerminal.kt` paid in the reflow. - `ktlint("1.8.0")` pinned, `ij_kotlin_imports_layout` pinned to `*,java.**,javax.**,kotlin.**,^`: a spotless bump carrying a new ktlint, or an IDE-exported `.editorconfig`, can no longer cost a second repo-wide reflow. `spotlessCheck` stays green with both pins in place — the pinned layout is the reflowed order. - `build.yml`: `spotlessCheck` moved above the keystore steps (cheapest gate, needs no secrets); `CLAUDE.md` documents `spotlessApply` in Commands. Not taken: reordering the rule/reflow commits for per-commit bisect green (review item 6) — priced in review as not worth a force-push. `spotlessCheck` and `compileDebugKotlin` pass locally. No `versionName` bump: nothing user-visible. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

julian added 2 commits

2026-08-13 23:24:03 +02:00

build: spotless import-order check, the rule review kept enforcing by hand e298d419bb

Spotless has no importOrder() for Kotlin, so this is ktlint with every
standard rule disabled except import-ordering — the whole-style claim
from #3 stays out. Line endings pinned to LF: without a .gitattributes
the plugin falls back to platform-native and a Windows checkout would
rewrite every file.

CI gains a spotlessCheck step beside lintDebug; a formatter nothing
runs is the same as no formatter.

Closes #3

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

style: sort imports repo-wide, the one-time reflow #3 priced in 232c86ea91

Mechanical spotlessApply output, nothing hand-written. Kept apart from
the tooling commit so a reviewer can diff the rule and skim the reflow.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

julian requested review from thisilike

2026-08-13 23:24:07 +02:00

julian referenced this pull request

2026-08-13 23:37:16 +02:00

fix(download): a final name the filesystem let us claim, not one a check saw free #7

thisilike requested changes

2026-08-23 20:58:40 +02:00

thisilike left a comment

Re-reviewed by running it, not by reading it: worktree at 232c86e, real ./gradlew runs against the local Android SDK.

Every claim in the description checks out

"spotless has no importOrder() for Kotlin" — true. Swapped the ktlint step for importOrder() in the kotlin {} block and configuration dies with app/build.gradle.kts:18:9: Unresolved reference: importOrder. The ktlint detour is the right shape, not over-engineering.
"sorts imports and touches nothing else" — true, and I tried to break it. A probe file with class ugly_Class, fun Bad( ), a wildcard import, an unused local, trailing whitespace and eight-space indent produced exactly one violation: the misplaced import kotlin.math.max. Everything else passed untouched.
"12 files. Eleven pure import reorders" — true. Per file the import set is byte-identical before/after (sorted + hashed), and git diff --ignore-all-space 31461a4..HEAD -- app/src/** has zero non-import lines.
AnsiTerminal.kt — true. Only CRLF file in main (514 CR), byte-identical to the new version after stripping CR, and no .kt file at the head has a CR left.
"spotlessCheck and compileDebugKotlin pass locally" — both pass here too (JDK 21; CI uses 17). spotlessApply on the head is a no-op, so the reflow is complete and idempotent, and the configuration cache survives --no-daemon.
Bonus: spotless 8.9.0 is real (8.10.0 is current), and PR #7 merged on top of this still passes spotlessCheck — merge order doesn't matter.

Two things need fixing, both about the guard rather than the reflow.

1. The check never runs on a pull request (blocking)

.forgejo/workflows/build.yml triggers on push: branches: [main] only, and it is the repo's sole workflow. So the first time spotlessCheck ever looks at a scrambled import is after the merge — as a red release-and-publish build on main. The title says "so review stops sorting imports by hand", but a reviewer looking at the next PR still sees no check at all; nothing changed for them.

Issue #3 asked for the step "beside the lintDebug step", and that is literally what landed, so this is the issue under-specifying rather than the PR ignoring it — but the outcome the issue wanted (review stops spending a line on it) needs a PR-triggered run. Don't just add pull_request to this workflow: it would decode the keystore and publish APKs from branch builds. A second job or workflow on pull_request — checkout, JDK, SDK cache, spotlessCheck (lintDebug and testReleaseUnitTest have the same post-merge-only problem and belong there too), no secrets — is the shape.

2. No `.gitattributes`, so LF is asserted in one place and not enforced anywhere (blocking)

The comment above lineEndings says the repo has no .gitattributes and then works around it. That leaves two live traps, and I reproduced the first:

Convert one tracked file to CRLF in the working tree and spotlessCheck fails on it (-package cloud.imhof.opsdeck.ui.common\r\n …). Git for Windows sets core.autocrlf=true by default, which puts CRLF on disk for every file — so a contributor with a stock Windows install gets 71 violations on first run, and spotlessApply then fights the checkout filter on every reset.
Nothing stops another CRLF file being committed. AnsiTerminal.kt is proof that it already happened once — and after this PR, the next one fails CI on main (see #1: post-merge) rather than just sitting there quietly.

* text=auto eol=lf (or *.kt text eol=lf) in .gitattributes is the missing half; keep lineEndings = UNIX so spotless and git agree instead of one compensating for the other's absence.

Non-blocking

The sort order itself is unpinned: with no .editorconfig the layout is ktlint's default (*,java.**,javax.**,kotlin.**,^ — verified with a probe: androidx, zzz, then java/javax/kotlin last, which is also Android Studio's Kotlin default, so IDE "Optimize imports" agrees today). The day someone exports an .editorconfig from the IDE, or spotless bumps its bundled ktlint, the repo eats a second repo-wide reflow. Adding "ij_kotlin_imports_layout" to the same editorConfigOverride — and optionally ktlint("<version>") — puts the order next to the rule that enforces it.
CLAUDE.md's Commands section still lists only compileDebugKotlin / assembleDebug. A contributor's first encounter with the new rule is a failing build; one line for ./gradlew spotlessApply fixes that.
Step placement: Check import order sits after the keystore decode/verify and version-metadata steps. The cheapest gate that needs no secrets should run right after Cache Gradle.
e298d41 alone fails spotlessCheck on all 12 files (confirmed). Reflow-first, rule-second would leave every commit individually green for a bisect. Only matters if you bisect build health; not worth a force-push on its own.

Re-reviewed by running it, not by reading it: worktree at 232c86e, real `./gradlew` runs against the local Android SDK. ## Every claim in the description checks out - **"spotless has no `importOrder()` for Kotlin"** — true. Swapped the ktlint step for `importOrder()` in the `kotlin {}` block and configuration dies with `app/build.gradle.kts:18:9: Unresolved reference: importOrder`. The ktlint detour is the right shape, not over-engineering. - **"sorts imports and touches nothing else"** — true, and I tried to break it. A probe file with `class ugly_Class`, `fun Bad( )`, a wildcard import, an unused local, trailing whitespace and eight-space indent produced exactly one violation: the misplaced `import kotlin.math.max`. Everything else passed untouched. - **"12 files. Eleven pure import reorders"** — true. Per file the import *set* is byte-identical before/after (sorted + hashed), and `git diff --ignore-all-space 31461a4..HEAD -- app/src/**` has **zero** non-`import` lines. - **`AnsiTerminal.kt`** — true. Only CRLF file in `main` (514 CR), byte-identical to the new version after stripping CR, and no `.kt` file at the head has a CR left. - **"`spotlessCheck` and `compileDebugKotlin` pass locally"** — both pass here too (JDK 21; CI uses 17). `spotlessApply` on the head is a no-op, so the reflow is complete and idempotent, and the configuration cache survives `--no-daemon`. - Bonus: spotless 8.9.0 is real (8.10.0 is current), and PR #7 merged on top of this still passes `spotlessCheck` — merge order doesn't matter. Two things need fixing, both about the guard rather than the reflow. ## 1. The check never runs on a pull request (blocking) `.forgejo/workflows/build.yml` triggers on `push: branches: [main]` only, and it is the repo's sole workflow. So the first time `spotlessCheck` ever looks at a scrambled import is *after* the merge — as a red release-and-publish build on main. The title says "so review stops sorting imports by hand", but a reviewer looking at the next PR still sees no check at all; nothing changed for them. Issue #3 asked for the step "beside the `lintDebug` step", and that is literally what landed, so this is the issue under-specifying rather than the PR ignoring it — but the outcome the issue wanted (review stops spending a line on it) needs a PR-triggered run. Don't just add `pull_request` to this workflow: it would decode the keystore and publish APKs from branch builds. A second job or workflow on `pull_request` — checkout, JDK, SDK cache, `spotlessCheck` (`lintDebug` and `testReleaseUnitTest` have the same post-merge-only problem and belong there too), no secrets — is the shape. ## 2. No `.gitattributes`, so LF is asserted in one place and not enforced anywhere (blocking) The comment above `lineEndings` says the repo has no `.gitattributes` and then works around it. That leaves two live traps, and I reproduced the first: - Convert one tracked file to CRLF in the working tree and `spotlessCheck` fails on it (`-package cloud.imhof.opsdeck.ui.common\r\n` …). Git for Windows sets `core.autocrlf=true` by default, which puts CRLF on disk for *every* file — so a contributor with a stock Windows install gets 71 violations on first run, and `spotlessApply` then fights the checkout filter on every reset. - Nothing stops another CRLF file being committed. `AnsiTerminal.kt` is proof that it already happened once — and after this PR, the next one fails CI on main (see #1: post-merge) rather than just sitting there quietly. `* text=auto eol=lf` (or `*.kt text eol=lf`) in `.gitattributes` is the missing half; keep `lineEndings = UNIX` so spotless and git agree instead of one compensating for the other's absence. ## Non-blocking 3. The sort order itself is unpinned: with no `.editorconfig` the layout is ktlint's default (`*,java.**,javax.**,kotlin.**,^` — verified with a probe: `androidx`, `zzz`, then `java`/`javax`/`kotlin` last, which is also Android Studio's Kotlin default, so IDE "Optimize imports" agrees today). The day someone exports an `.editorconfig` from the IDE, or spotless bumps its bundled ktlint, the repo eats a second repo-wide reflow. Adding `"ij_kotlin_imports_layout"` to the same `editorConfigOverride` — and optionally `ktlint("<version>")` — puts the order next to the rule that enforces it. 4. `CLAUDE.md`'s Commands section still lists only `compileDebugKotlin` / `assembleDebug`. A contributor's first encounter with the new rule is a failing build; one line for `./gradlew spotlessApply` fixes that. 5. Step placement: `Check import order` sits after the keystore decode/verify and version-metadata steps. The cheapest gate that needs no secrets should run right after `Cache Gradle`. 6. `e298d41` alone fails `spotlessCheck` on all 12 files (confirmed). Reflow-first, rule-second would leave every commit individually green for a bisect. Only matters if you bisect build health; not worth a force-push on its own.

.forgejo/workflows/build.yml Outdated

					
				@ -86,0 +86,4 @@

				      - name: Check import order

				        # spotless runs ktlint with only import-ordering enabled (#3) —

				        # a formatter nothing runs is the same as no formatter

				        run: ./gradlew --no-daemon spotlessCheck

This workflow is on: push: branches: [main] and it is the only one in .forgejo/workflows/, so this step never runs on a PR. First enforcement of the new rule is a red release-and-publish build on main, after the merge — the reviewer whose hand-sorting motivated #3 still sees nothing.

Adding pull_request to this workflow is not the fix (it would decode the keystore and publish APKs from branch builds). A separate pull_request job/workflow — checkout, JDK, SDK cache, spotlessCheck, no secrets — is. lintDebug and testReleaseUnitTest have the same post-merge-only blind spot and belong there with it.

Secondary: this step needs no secrets and no keystore, so it should run before Decode signing keystore / Verify keystore and alias rather than after them.

This workflow is `on: push: branches: [main]` and it is the only one in `.forgejo/workflows/`, so this step never runs on a PR. First enforcement of the new rule is a red release-and-publish build on main, after the merge — the reviewer whose hand-sorting motivated #3 still sees nothing. Adding `pull_request` to *this* workflow is not the fix (it would decode the keystore and publish APKs from branch builds). A separate `pull_request` job/workflow — checkout, JDK, SDK cache, `spotlessCheck`, no secrets — is. `lintDebug` and `testReleaseUnitTest` have the same post-merge-only blind spot and belong there with it. Secondary: this step needs no secrets and no keystore, so it should run before `Decode signing keystore` / `Verify keystore and alias` rather than after them.

app/build.gradle.kts

					
				@ -6,0 +9,4 @@

				spotless {

				    // no .gitattributes: the default (git-then-platform-native) would let a

				    // Windows checkout rewrite every LF file to CRLF on apply

				    lineEndings = com.diffplug.spotless.LineEnding.UNIX

This comment identifies the missing .gitattributes and then compensates for it instead of adding it. Two consequences, the first reproduced here:

Convert any tracked .kt to CRLF in the working tree and spotlessCheck fails on it (-package cloud.imhof.opsdeck.ui.common\r\n …). Git for Windows defaults to core.autocrlf=true, which puts CRLF on disk for every file — a stock Windows clone gets 71 violations on the first run, and spotlessApply then loses to the checkout filter on the next reset.
Nothing prevents the next CRLF commit. AnsiTerminal.kt proves that already happened once, and with this PR that now turns main red after a merge instead of being invisible.

Add * text=auto eol=lf (or *.kt text eol=lf) and keep lineEndings = UNIX, so git and spotless agree rather than one covering for the other's absence.

This comment identifies the missing `.gitattributes` and then compensates for it instead of adding it. Two consequences, the first reproduced here: 1. Convert any tracked `.kt` to CRLF in the working tree and `spotlessCheck` fails on it (`-package cloud.imhof.opsdeck.ui.common\r\n` …). Git for Windows defaults to `core.autocrlf=true`, which puts CRLF on disk for every file — a stock Windows clone gets 71 violations on the first run, and `spotlessApply` then loses to the checkout filter on the next reset. 2. Nothing prevents the next CRLF commit. `AnsiTerminal.kt` proves that already happened once, and with this PR that now turns main red after a merge instead of being invisible. Add `* text=auto eol=lf` (or `*.kt text eol=lf`) and keep `lineEndings = UNIX`, so git and spotless agree rather than one covering for the other's absence.

app/build.gradle.kts Outdated

					
				@ -6,0 +15,4 @@

				        // Import order only. Spotless has no importOrder() for Kotlin, so

				        // this is ktlint with every standard rule disabled except the one

				        // review kept enforcing by hand — the whole-style claim stays out.

				        ktlint().editorConfigOverride(

Verified the narrow claim: a probe file with bad indentation, a wildcard import, an unused local, trailing whitespace and a non-standard class name produced exactly one violation — the misplaced import. ktlint_standard = disabled plus the single re-enable does what the comment says.

What isn't pinned is the order. With no .editorconfig in the repo, the layout is ktlint's built-in default (*,java.**,javax.**,kotlin.**,^ — confirmed by probe: androidx, zzz, then java/javax/kotlin last). That happens to match Android Studio's Kotlin default today, so IDE Optimize imports agrees — but an .editorconfig exported from the IDE later, or a spotless bump carrying a new ktlint, changes it and costs a second repo-wide reflow. Consider "ij_kotlin_imports_layout" in this same map, and an explicit ktlint("<version>").

Verified the narrow claim: a probe file with bad indentation, a wildcard import, an unused local, trailing whitespace and a non-standard class name produced exactly one violation — the misplaced import. `ktlint_standard = disabled` plus the single re-enable does what the comment says. What isn't pinned is the *order*. With no `.editorconfig` in the repo, the layout is ktlint's built-in default (`*,java.**,javax.**,kotlin.**,^` — confirmed by probe: `androidx`, `zzz`, then `java`/`javax`/`kotlin` last). That happens to match Android Studio's Kotlin default today, so IDE `Optimize imports` agrees — but an `.editorconfig` exported from the IDE later, or a spotless bump carrying a new ktlint, changes it and costs a second repo-wide reflow. Consider `"ij_kotlin_imports_layout"` in this same map, and an explicit `ktlint("<version>")`.

julian referenced this pull request from a commit

2026-08-23 22:40:02 +02:00

build: run the gates where review looks, and enforce the LF the build assumed

julian added 1 commit

2026-08-23 22:40:02 +02:00

build: run the gates where review looks, and enforce the LF the build assumed

PR Checks / check (pull_request) Successful in 4m31s

Details

20985dbc16

Rework for the #6 review:

- pr-checks.yml: the no-secrets half of build.yml, on pull_request —
  spotlessCheck, lintDebug, testReleaseUnitTest. Until now all three ran
  post-merge only, so the first scrambled import would have surfaced as
  a red release-and-publish build on main, and the reviewer the rule was
  written for still saw no check at all. Separate workflow, not a second
  trigger: branch builds must never touch the keystore.
- .gitattributes: * text=auto eol=lf, with *.bat kept CRLF for cmd.exe.
  lineEndings = UNIX was compensating for its absence; now git enforces
  what spotless assumed, and a stock Windows clone (autocrlf=true) no
  longer checks out CRLF everywhere to fail spotlessCheck on all files.
  gradlew.bat renormalizes once (LF in repo, CRLF on disk) — the same
  class of diff AnsiTerminal.kt paid in the reflow commit.
- ktlint("1.8.0") pinned, ij_kotlin_imports_layout pinned to ktlint's
  default: a spotless bump carrying a new ktlint, or an IDE-exported
  .editorconfig, no longer costs a second repo-wide reflow. spotlessCheck
  stays green with both pins — the pinned layout is the reflowed order.
- build.yml: spotlessCheck moved above the keystore steps — the cheapest
  gate, and the only one before it that needed no secrets.
- CLAUDE.md: spotlessApply in Commands, so first contact with the rule
  is a documented command rather than a failing build.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

julian commented

2026-08-23 22:40:37 +02:00

Rework pushed as 20985db, item by item:

1. Check never runs on a PR (blocking) — new workflow .forgejo/workflows/pr-checks.yml, on: pull_request: checkout, JDK 17, the same SDK and Gradle caches as build.yml, then spotlessCheck, lintDebug, testReleaseUnitTest. No keystore decode, no secrets, no publish — a separate workflow rather than a second trigger on build.yml, exactly so branch builds can never reach the signing path. The two workflows share cache keys, so a PR run warms the cache the main build reuses. lintDebug and testReleaseUnitTest are included per the review — their post-merge-only blind spot was the same one.

2. No .gitattributes (blocking) — added: * text=auto eol=lf, with *.bat text eol=crlf (cmd.exe mis-parses labels under LF) and *.ttf / *.jar explicitly -text so no normalization pass can ever touch them. lineEndings = UNIX stays, and its comment now says what it does — agrees with git — instead of compensating for git's silence. git add --renormalize . changed exactly one file: gradlew.bat (blob LF, checkout CRLF), the one-time diff that pins it. A stock core.autocrlf=true clone now checks out LF and passes spotlessCheck clean.

3. Unpinned sort order — ktlint("1.8.0") (the version spotless 8.9.0 bundles, so no behavior change) and "ij_kotlin_imports_layout" to "*,java.**,javax.**,kotlin.**,^" in the same editorConfigOverride. spotlessCheck passes with both pins in place, which is the proof the pinned layout is the order the reflow already produced.

4. CLAUDE.md — ./gradlew spotlessApply added to Commands with a note that CI fails on unsorted imports.

5. Step placement — Check import order in build.yml now sits directly after Cache Gradle, before the keystore steps.

6. Commit order — left as is, per your own pricing: not worth a force-push.

Verified locally: spotlessCheck and compileDebugKotlin green on 20985db.

Rework pushed as 20985db, item by item: **1. Check never runs on a PR (blocking)** — new workflow `.forgejo/workflows/pr-checks.yml`, `on: pull_request`: checkout, JDK 17, the same SDK and Gradle caches as `build.yml`, then `spotlessCheck`, `lintDebug`, `testReleaseUnitTest`. No keystore decode, no secrets, no publish — a separate workflow rather than a second trigger on `build.yml`, exactly so branch builds can never reach the signing path. The two workflows share cache keys, so a PR run warms the cache the main build reuses. `lintDebug` and `testReleaseUnitTest` are included per the review — their post-merge-only blind spot was the same one. **2. No `.gitattributes` (blocking)** — added: `* text=auto eol=lf`, with `*.bat text eol=crlf` (cmd.exe mis-parses labels under LF) and `*.ttf` / `*.jar` explicitly `-text` so no normalization pass can ever touch them. `lineEndings = UNIX` stays, and its comment now says what it does — agrees with git — instead of compensating for git's silence. `git add --renormalize .` changed exactly one file: `gradlew.bat` (blob LF, checkout CRLF), the one-time diff that pins it. A stock `core.autocrlf=true` clone now checks out LF and passes `spotlessCheck` clean. **3. Unpinned sort order** — `ktlint("1.8.0")` (the version spotless 8.9.0 bundles, so no behavior change) and `"ij_kotlin_imports_layout" to "*,java.**,javax.**,kotlin.**,^"` in the same `editorConfigOverride`. `spotlessCheck` passes with both pins in place, which is the proof the pinned layout is the order the reflow already produced. **4. `CLAUDE.md`** — `./gradlew spotlessApply` added to Commands with a note that CI fails on unsorted imports. **5. Step placement** — `Check import order` in `build.yml` now sits directly after `Cache Gradle`, before the keystore steps. **6. Commit order** — left as is, per your own pricing: not worth a force-push. Verified locally: `spotlessCheck` and `compileDebugKotlin` green on 20985db.

julian requested review from thisilike

2026-08-23 22:40:39 +02:00

thisilike approved these changes

2026-08-24 14:15:15 +02:00

thisilike left a comment

Re-reviewed the rework the same way as the first pass: worktree at 20985db, real ./gradlew runs (JDK 21 local, CI is 17), plus the actual CI run log. Every item from the last review is addressed, and I could not find a new defect that blocks. Approving; four notes below, one of them reproduced.

The rework checks out

1 — the gate now runs on the PR. pr-checks.yml is on: pull_request, no keystore, no secrets.* reference anywhere, and it is not a second trigger on build.yml. This is not just plausible-on-paper: run #655 (event: pull_request, 4m31s) is green on 20985db, so Forgejo does pick the workflow up from the head branch. lintDebug and testReleaseUnitTest are in there too, as asked. Both also pass locally (BUILD SUCCESSFUL, testReleaseUnitTest + lintDebug).
2 — .gitattributes does what it claims, for a fresh clone. Simulated the stock Windows case: git clone -c core.autocrlf=true checked out at 20985db → 0 of 71 .kt files CRLF, gradlew LF, gradlew.bat CRLF, spotlessCheck green. The same clone at 232c86e gets 71 of 71 CRLF, which is the trap being closed. git ls-files --eol at the head: every text file i/lf, gradlew.bat i/lf w/crlf attr/text eol=crlf, the three .ttf and the wrapper jar -text. git add --renormalize . at the head changes nothing, so the one-time gradlew.bat diff is the whole cost — and it is CR-only (tr -d '\r' hashes match base).
3 — both pins are real, not decoration. Extracted KtLintStep's default version out of spotless-lib-4.9.0.jar: it is exactly 1.8.0, so ktlint("1.8.0") is a no-op today and a wall tomorrow, as described. The layout pin is honored, tested by hostile substitution: flip it to kotlin.**,java.**,javax.**,*,^ and spotlessCheck fails with the java.* imports demanding to move. So the map key is being read, not silently ignored.
Scope is still import-order only — re-probed against the pinned config, since the pins changed it: a file with class ugly_Class, fun Bad( x:Int ), a wildcard import, an unused local, trailing whitespace and eight-space indent produced exactly one violation, the misplaced import kotlin.math.max. spotlessApply moved that import and left every other ugliness untouched.
The reflow is still only imports. git diff --ignore-all-space 31461a4..HEAD -- app/src/** has zero non-import lines, and per file the import set is byte-identical before/after. AnsiTerminal.kt is CR-only.
Merge order still doesn't matter — re-checked against the pins, not just the old config: #7 (4d18c66) merged onto 20985db passes spotlessCheck.
4/5/6 — CLAUDE.md line is there; Check import order now sits directly after Cache Gradle and before the keystore steps, with nothing lost in the move; commit order left as priced.

Notes (none blocking)

a. The claim in the description is true for a fresh clone, not an existing one. Reproduced: a working tree that was already checked out with core.autocrlf=true before this branch keeps CRLF on every file the branch does not touch — 37 .kt files here — because git does not re-filter unchanged files on checkout. spotlessCheck then fails (src/main/java/cloud/imhof/opsdeck/MainActivity.kt), and ./gradlew won't even start in that tree (CRLF shebang → No such file or directory, which is pre-existing, not caused by this PR). Self-service fix is ./gradlew spotlessApply or git add --renormalize ., so this is one sentence next to the new CLAUDE.md line, not a code change.

b. "A PR run warms the cache the main build reuses" is not supported by the one run we have. The keys are identical, but run #655's log shows both caches missing on keys main has written before: it downloaded platform-36_r02.zip + platform-tools from scratch, and gradle-8.14.3-bin.zip from scratch, despite restore-keys: gradle-. Either Forgejo scopes caches per ref (in which case the branch→main direction is the one GitHub forbids outright) or the 10-day-old entries had been evicted. Worth dropping the promise from the description rather than relying on it; the practical shape is that a PR check costs ~4m30s of runner time cold.

c. No concurrency and no types: on the new workflow. Three pushes to a PR start three full cold runs, in parallel, on one self-hosted runner. concurrency: { group: pr-${{ github.ref }}, cancel-in-progress: true } and types: [opened, synchronize, reopened] are the cheap version.

d. The check is advisory. The repo has zero branch-protection rules, so a red PR check is one click away from being merged anyway. That still satisfies #3 — a reviewer now sees a red X instead of hand-sorting — but "CI prevents it" needs the repo setting, which is not this PR's job.

Two smaller things, take or leave: spotless is applied in :app only and targets src/**/*.kt, so the .kts build scripts and any future module sit outside the rule; and while you are in .gitattributes, an Android repo usually acquires *.png/*.webp/*.jks sooner or later, which are cheaper to mark -text now than after a normalization pass touches one.

Unrelated to this diff, but the new PR gate will now show it on every run: lintDebug reports "12 errors/warnings were listed in the baseline file but not found in the project" — the baseline has stale entries.

Re-reviewed the rework the same way as the first pass: worktree at 20985db, real `./gradlew` runs (JDK 21 local, CI is 17), plus the actual CI run log. Every item from the last review is addressed, and I could not find a new defect that blocks. Approving; four notes below, one of them reproduced. ## The rework checks out - **1 — the gate now runs on the PR.** `pr-checks.yml` is `on: pull_request`, no keystore, no `secrets.*` reference anywhere, and it is not a second trigger on `build.yml`. This is not just plausible-on-paper: run **#655** (`event: pull_request`, 4m31s) is green on 20985db, so Forgejo does pick the workflow up from the head branch. `lintDebug` and `testReleaseUnitTest` are in there too, as asked. Both also pass locally (`BUILD SUCCESSFUL`, `testReleaseUnitTest` + `lintDebug`). - **2 — `.gitattributes` does what it claims, for a fresh clone.** Simulated the stock Windows case: `git clone -c core.autocrlf=true` checked out at 20985db → **0 of 71** `.kt` files CRLF, `gradlew` LF, `gradlew.bat` CRLF, `spotlessCheck` green. The same clone at 232c86e gets **71 of 71** CRLF, which is the trap being closed. `git ls-files --eol` at the head: every text file `i/lf`, `gradlew.bat` `i/lf w/crlf attr/text eol=crlf`, the three `.ttf` and the wrapper jar `-text`. `git add --renormalize .` at the head changes nothing, so the one-time `gradlew.bat` diff is the whole cost — and it is CR-only (`tr -d '\r'` hashes match base). - **3 — both pins are real, not decoration.** Extracted `KtLintStep`'s default version out of `spotless-lib-4.9.0.jar`: it is exactly `1.8.0`, so `ktlint("1.8.0")` is a no-op today and a wall tomorrow, as described. The layout pin is honored, tested by hostile substitution: flip it to `kotlin.**,java.**,javax.**,*,^` and `spotlessCheck` fails with the `java.*` imports demanding to move. So the map key is being read, not silently ignored. - **Scope is still import-order only** — re-probed against the *pinned* config, since the pins changed it: a file with `class ugly_Class`, `fun Bad( x:Int )`, a wildcard import, an unused local, trailing whitespace and eight-space indent produced exactly one violation, the misplaced `import kotlin.math.max`. `spotlessApply` moved that import and left every other ugliness untouched. - **The reflow is still only imports.** `git diff --ignore-all-space 31461a4..HEAD -- app/src/**` has zero non-`import` lines, and per file the import *set* is byte-identical before/after. `AnsiTerminal.kt` is CR-only. - **Merge order still doesn't matter** — re-checked against the pins, not just the old config: #7 (4d18c66) merged onto 20985db passes `spotlessCheck`. - **4/5/6** — `CLAUDE.md` line is there; `Check import order` now sits directly after `Cache Gradle` and before the keystore steps, with nothing lost in the move; commit order left as priced. ## Notes (none blocking) **a. The claim in the description is true for a *fresh* clone, not an existing one.** Reproduced: a working tree that was already checked out with `core.autocrlf=true` before this branch keeps CRLF on every file the branch does not touch — 37 `.kt` files here — because git does not re-filter unchanged files on checkout. `spotlessCheck` then fails (`src/main/java/cloud/imhof/opsdeck/MainActivity.kt`), and `./gradlew` won't even start in that tree (CRLF shebang → `No such file or directory`, which is pre-existing, not caused by this PR). Self-service fix is `./gradlew spotlessApply` or `git add --renormalize .`, so this is one sentence next to the new `CLAUDE.md` line, not a code change. **b. "A PR run warms the cache the main build reuses" is not supported by the one run we have.** The keys are identical, but run #655's log shows *both* caches missing on keys main has written before: it downloaded `platform-36_r02.zip` + platform-tools from scratch, and `gradle-8.14.3-bin.zip` from scratch, despite `restore-keys: gradle-`. Either Forgejo scopes caches per ref (in which case the branch→main direction is the one GitHub forbids outright) or the 10-day-old entries had been evicted. Worth dropping the promise from the description rather than relying on it; the practical shape is that a PR check costs ~4m30s of runner time cold. **c. No `concurrency` and no `types:` on the new workflow.** Three pushes to a PR start three full cold runs, in parallel, on one self-hosted runner. `concurrency: { group: pr-${{ github.ref }}, cancel-in-progress: true }` and `types: [opened, synchronize, reopened]` are the cheap version. **d. The check is advisory.** The repo has zero branch-protection rules, so a red PR check is one click away from being merged anyway. That still satisfies #3 — a reviewer now sees a red X instead of hand-sorting — but "CI prevents it" needs the repo setting, which is not this PR's job. Two smaller things, take or leave: spotless is applied in `:app` only and targets `src/**/*.kt`, so the `.kts` build scripts and any future module sit outside the rule; and while you are in `.gitattributes`, an Android repo usually acquires `*.png`/`*.webp`/`*.jks` sooner or later, which are cheaper to mark `-text` now than after a normalization pass touches one. Unrelated to this diff, but the new PR gate will now show it on every run: `lintDebug` reports "12 errors/warnings were listed in the baseline file but not found in the project" — the baseline has stale entries.

.forgejo/workflows/pr-checks.yml

					
				@ -0,0 +6,4 @@

				name: PR Checks

				on:

				  pull_request:

Confirmed working end to end — run #655 (event: pull_request) is green on this head, so Forgejo does take the workflow from the head branch, and the log shows no keystore step and no secrets.* expansion. Three follow-ups, all cost rather than correctness:

No concurrency, so three pushes to a PR mean three full cold runs at once on the one self-hosted runner. concurrency: { group: pr-${{ github.ref }}, cancel-in-progress: true } plus types: [opened, synchronize, reopened] is the cheap fix.
The cache sharing the description promises did not happen in the run we have: identical keys, yet #655 downloaded platform-36_r02.zip, platform-tools and gradle-8.14.3-bin.zip from scratch, with restore-keys: gradle- in place. Either Forgejo scopes caches per ref or the 10-day-old entries were evicted — either way don't count on the warm-up.
The log fetches +20985db…:refs/remotes/pull/6/head and checks that out, so this gate tests the branch, not the merge result. Green here is not green after merge; I verified the #7 merge by hand, which is exactly the thing the check cannot do for you.

Confirmed working end to end — run #655 (`event: pull_request`) is green on this head, so Forgejo does take the workflow from the head branch, and the log shows no keystore step and no `secrets.*` expansion. Three follow-ups, all cost rather than correctness: 1. No `concurrency`, so three pushes to a PR mean three full cold runs at once on the one self-hosted runner. `concurrency: { group: pr-${{ github.ref }}, cancel-in-progress: true }` plus `types: [opened, synchronize, reopened]` is the cheap fix. 2. The cache sharing the description promises did not happen in the run we have: identical keys, yet #655 downloaded `platform-36_r02.zip`, platform-tools *and* `gradle-8.14.3-bin.zip` from scratch, with `restore-keys: gradle-` in place. Either Forgejo scopes caches per ref or the 10-day-old entries were evicted — either way don't count on the warm-up. 3. The log fetches `+20985db…:refs/remotes/pull/6/head` and checks that out, so this gate tests the branch, not the merge result. Green here is not green after merge; I verified the #7 merge by hand, which is exactly the thing the check cannot do for you.

.gitattributes

					
				@ -0,0 +2,4 @@

				# lineEndings = UNIX and git agree instead of one compensating for the

				# other's absence. A stock Windows clone (core.autocrlf=true) would

				# otherwise check out CRLF everywhere and fail spotlessCheck on all files.

				* text=auto eol=lf

Verified for a fresh clone: git clone -c core.autocrlf=true at this head gives 0 of 71 .kt files CRLF, LF gradlew, CRLF gradlew.bat, and a green spotlessCheck. Same clone at 232c86e: 71 of 71 CRLF. The line does its job.

What it cannot do is fix a working tree that already exists. Reproduced: a tree checked out with core.autocrlf=true before this branch keeps CRLF on the 37 .kt files the branch does not touch — git does not re-filter unchanged files — and spotlessCheck fails there (MainActivity.kt). ./gradlew spotlessApply or git add --renormalize . clears it. One sentence next to the new CLAUDE.md command would keep whoever hits it from reading it as a broken rule.

Verified for a *fresh* clone: `git clone -c core.autocrlf=true` at this head gives 0 of 71 `.kt` files CRLF, LF `gradlew`, CRLF `gradlew.bat`, and a green `spotlessCheck`. Same clone at 232c86e: 71 of 71 CRLF. The line does its job. What it cannot do is fix a working tree that already exists. Reproduced: a tree checked out with `core.autocrlf=true` before this branch keeps CRLF on the 37 `.kt` files the branch does not touch — git does not re-filter unchanged files — and `spotlessCheck` fails there (`MainActivity.kt`). `./gradlew spotlessApply` or `git add --renormalize .` clears it. One sentence next to the new `CLAUDE.md` command would keep whoever hits it from reading it as a broken rule.