From bdc28bb0d4742d74be20d16288e8cb10e02e2ce3 Mon Sep 17 00:00:00 2001 From: simtop Date: Sun, 9 Aug 2026 18:05:33 +0200 Subject: [PATCH 1/2] feat: make Detekt a real gate, and scan test sources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Detekt had ignoreFailures = true, so it could not fail on anything however bad, while AGENTS.md §5 advertises `make lint` as a rung of the verification ladder. It was output, not a gate. And `source` covered only src/main, so roughly a third of the repo's Kotlin - the part that decides whether the rest is correct - was invisible to it even in advisory mode. Both fixed the way the Android Lint gate already works here: the backlog present at adoption is grandfathered into per-module detekt-baseline.xml files, and a NEW finding fails the build. That keeps the property that matters - fixing the existing findings stays a batching decision, taken whenever suits, rather than something that blocks work today. Adoption cost: 36 findings in main sources plus 13 more once test and androidTest sources are scanned, across 17 modules. Nothing was silently regenerated - pre-existing baselines were restored first and only refreshed for modules that actually failed, and the grandfathered rules are listed in the PR. One bug had to be fixed to get here. The extension sets `baseline` to null until the file exists, which is right for the check task but left the task that CREATES the baseline with nowhere to write: `detektBaseline` failed with "property 'baseline' doesn't have a configured value" on exactly the modules that needed one. The create task now gets the path unconditionally. Verified with a mutation probe: a file tripping LongParameterList and MagicNumber fails `make lint`, and removing it returns the build to green. Also adds ADR 0013, recording that convention plugins stay precompiled script plugins. That is a deliberate earlier decision - the project moved to them from binary plugins - and it was re-proposed this month on grounds that do not hold: precompiled scripts can share helper functions (AndroidCommon.kt does), and the configuration-time argument is largely neutralised by the configuration cache being on. The ADR turns the comparison into a measurement task with a written method rather than a judgement call, and records where Declarative Gradle sits. --- AGENTS.md | 1 + app/detekt-baseline.xml | 5 +- beer_data/detekt-baseline.xml | 4 +- beer_database/detekt-baseline.xml | 4 +- beer_network/detekt-baseline.xml | 7 ++ beerdomain/api/detekt-baseline.xml | 7 ++ beerdomain/fakes/detekt-baseline.xml | 3 +- .../kotlin/billionbeers.detekt.gradle.kts | 32 ++++++- catalog-processor/detekt-baseline.xml | 12 +++ core-common/detekt-baseline.xml | 13 +++ core/designsystem/detekt-baseline.xml | 2 +- docs/adr/0013-convention-plugin-form.md | 91 +++++++++++++++++++ feature/beerbrowse/detekt-baseline.xml | 10 ++ feature/beersearch/detekt-baseline.xml | 8 ++ feature/beerslist/detekt-baseline.xml | 2 +- konsist/detekt-baseline.xml | 7 ++ presentation_utils/detekt-baseline.xml | 8 ++ snapshot-processor/detekt-baseline.xml | 15 +++ testing-utils-android/detekt-baseline.xml | 7 ++ 19 files changed, 227 insertions(+), 11 deletions(-) create mode 100644 beer_network/detekt-baseline.xml create mode 100644 beerdomain/api/detekt-baseline.xml create mode 100644 catalog-processor/detekt-baseline.xml create mode 100644 core-common/detekt-baseline.xml create mode 100644 docs/adr/0013-convention-plugin-form.md create mode 100644 feature/beerbrowse/detekt-baseline.xml create mode 100644 feature/beersearch/detekt-baseline.xml create mode 100644 konsist/detekt-baseline.xml create mode 100644 presentation_utils/detekt-baseline.xml create mode 100644 snapshot-processor/detekt-baseline.xml create mode 100644 testing-utils-android/detekt-baseline.xml diff --git a/AGENTS.md b/AGENTS.md index 00d229ef..201afbb2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -150,6 +150,7 @@ left `make konsist` green at "3 up-to-date". The whole gate, not one rule. | **Non-goals** — auth, cert pinning, encrypted storage, integrity/anti-tamper, push, server-side `available` sync, a shipped analytics/crash SDK, consent flows, automatic retry, multi-process. All declined because the premise is absent (the backend isn't ours, is read-only, and is unauthenticated), each with its reopen trigger. Read it before "adding what a real app has" — and delete a row in the same PR that builds it | `docs/adr/0010` | | Dispatchers are chosen where the work is, not where the coroutine starts. ViewModels use a bare `viewModelScope.launch { }` — Room and Retrofit suspend calls are already main-safe, so an IO hop only moves state assignment off Main. Inject `CoroutineDispatcherProvider` only where a class actually calls `withContext`/`flowOn` (a blocking SDK, CPU work). StrictMode in debug is the detector that makes this safe | `docs/adr/0012` | | Build time is budgeted in `config/build-time-budget.txt` and measured **locally** by `make build-budget`, never in CI — a CI wall-clock number measures which runner the job drew. Clean build 37s cold / 4s warm; a deep ABI change costs only 1.11x a leaf one, so **per-build overhead dominates, not compilation** — do not justify a new module by build speed | `docs/adr/0011` | +| Convention plugins stay **precompiled script plugins**; the project moved to them from binary plugins on purpose. Do not propose converting back on the usual grounds — precompiled scripts *can* share helper functions (see `AndroidCommon.kt`), and the configuration-time argument is largely neutralised by the configuration cache. Converting is a *measurement* task with a written method, not a judgement call. Declarative Gradle sits on top of this, not against it | `docs/adr/0013` | Also settled, without an ADR: diff --git a/app/detekt-baseline.xml b/app/detekt-baseline.xml index 48f844ce..eb5785bf 100644 --- a/app/detekt-baseline.xml +++ b/app/detekt-baseline.xml @@ -2,8 +2,7 @@ - ForbiddenComment:MainCoroutineScopeRule.kt$// TODO: refactor - TopLevelPropertyNaming:TestingConstants.kt$const val fakeErrorName = "Error getting list of beers" - WildcardImport:MainCoroutineScopeRule.kt$import kotlinx.coroutines.test.* + EmptyFunctionBlock:FakeSplitInstallManager.kt$FakeSplitInstallManager${} + MatchingDeclarationName:LocalDataSourceTest2.kt$LocalDataSourceTest diff --git a/beer_data/detekt-baseline.xml b/beer_data/detekt-baseline.xml index f27b1d1c..786adc31 100644 --- a/beer_data/detekt-baseline.xml +++ b/beer_data/detekt-baseline.xml @@ -2,7 +2,7 @@ - SwallowedException:BeersRepositoryImpl.kt$BeersRepositoryImpl$e: Exception - TooGenericExceptionCaught:BeersRepositoryImpl.kt$BeersRepositoryImpl$e: Exception + CyclomaticComplexMethod:BeersMapper.kt$BeersMapper$fun fromBeersApiResponseItemToBeer(response: BeersApiResponseItem?): Beer + TooManyFunctions:BeersRepositoryImpl.kt$BeersRepositoryImpl : BeersRepository diff --git a/beer_database/detekt-baseline.xml b/beer_database/detekt-baseline.xml index ec31a94c..0c06ecee 100644 --- a/beer_database/detekt-baseline.xml +++ b/beer_database/detekt-baseline.xml @@ -2,7 +2,9 @@ - LongParameterList:BeersDao.kt$BeersDao$( id: String, name: String, tagline: String, description: String, imageUrl: String, abv: Double, ibu: Double, foodPairing: String, ) + LongParameterList:BeersDao.kt$BeersDao$( id: String, name: String, tagline: String, description: String, imageUrl: String, abv: Double, ibu: Double, foodPairing: String, styleName: String, breweryName: String, srm: Int?, releasedYear: Int?, minServingTemperature: Int?, maxServingTemperature: Int?, fermentationMethod: String, ingredients: String, recommendedGlasses: String, ) + MagicNumber:Migrations.kt$<no name provided>$3 MatchingDeclarationName:Converter.kt$Converters + TooManyFunctions:BeersDao.kt$BeersDao diff --git a/beer_network/detekt-baseline.xml b/beer_network/detekt-baseline.xml new file mode 100644 index 00000000..edd8e04b --- /dev/null +++ b/beer_network/detekt-baseline.xml @@ -0,0 +1,7 @@ + + + + + LongParameterList:BeersService.kt$BeersService$( @Query("_page") page: Int, @Query("_limit") perPage: Int = DEFAULT_ITEMS_PER_PAGE, @Query("translations.language.code") languageCode: String = DEFAULT_LANGUAGE_CODE, // Filters; null omits the param entirely, so the catalog's unfiltered fetch stays byte- // identical on the wire. The server ANDs whichever are present. @Query("q") search: String? = null, @Query("typology.id") typologyId: String? = null, @Query("brewery.id") breweryId: String? = null, ) + + diff --git a/beerdomain/api/detekt-baseline.xml b/beerdomain/api/detekt-baseline.xml new file mode 100644 index 00000000..1fa7c171 --- /dev/null +++ b/beerdomain/api/detekt-baseline.xml @@ -0,0 +1,7 @@ + + + + + TooManyFunctions:BeersRepository.kt$BeersRepository + + diff --git a/beerdomain/fakes/detekt-baseline.xml b/beerdomain/fakes/detekt-baseline.xml index 1e519738..0d2a531c 100644 --- a/beerdomain/fakes/detekt-baseline.xml +++ b/beerdomain/fakes/detekt-baseline.xml @@ -2,6 +2,7 @@ - EmptyFunctionBlock:FakeBeersRepository.kt$FakeBeersRepository${} + TooManyFunctions:FakeBeersRepository.kt$FakeBeersRepository : BeersRepository + TopLevelPropertyNaming:BeerFixtures.kt$const val fakeErrorName = "Error getting list of beers" diff --git a/build-logic/convention/src/main/kotlin/billionbeers.detekt.gradle.kts b/build-logic/convention/src/main/kotlin/billionbeers.detekt.gradle.kts index 5f7439a3..25b6d1ba 100644 --- a/build-logic/convention/src/main/kotlin/billionbeers.detekt.gradle.kts +++ b/build-logic/convention/src/main/kotlin/billionbeers.detekt.gradle.kts @@ -9,12 +9,34 @@ val libs = the() configure { toolVersion = libs.versions.detekt.get() - source.setFrom(files("src/main/java", "src/main/kotlin")) + // Test sources are scanned too. They were excluded, so a third of the repo's Kotlin - the part + // that decides whether the rest is correct - was invisible to static analysis even in the + // advisory mode this used to run in. Measured cost of adding them: 13 findings across 6 + // modules, all grandfathered into the per-module baselines at adoption. + source.setFrom( + files( + "src/main/java", + "src/main/kotlin", + "src/test/java", + "src/test/kotlin", + "src/androidTest/java", + "src/androidTest/kotlin", + ) + ) config.setFrom(files("$rootDir/config/detekt/detekt.yml")) baseline = file("detekt-baseline.xml").takeIf { it.exists() } buildUponDefaultConfig = true autoCorrect = false - ignoreFailures = true + // A NEW finding fails the build; the backlog present at adoption does not, because it is + // frozen in each module's detekt-baseline.xml. This is the same shape as the Android Lint gate + // (`abortOnError = true` over `app/lint-baseline.xml`), and AGENTS.md §5 already advertises + // `make lint` as a rung of the verification ladder - with ignoreFailures it was output, not a + // gate, and could not fail on anything however bad. + // + // Burn the baselines down over time. Never regenerate one to bury a regression: that is the + // rule AGENTS.md states for lint-baseline.xml and it applies identically here. Regenerate only + // when the finding is genuinely fixed, and only for the module you fixed. + ignoreFailures = false } // Detekt supports max java 22 for now @@ -24,4 +46,10 @@ tasks.withType().configureEach { tasks.withType().configureEach { jvmTarget = DETEKT_JAVA_VERSION + // The extension's `baseline` is null until the file exists, which is correct for the *check* + // task - a missing baseline should mean "no exemptions", not an error. But it left the task + // that CREATES the baseline with nowhere to write, so `detektBaseline` failed with "property + // 'baseline' doesn't have a configured value" on precisely the modules that needed one. Give + // the create task the path unconditionally; it is an output, not an input. + baseline.set(layout.projectDirectory.file("detekt-baseline.xml")) } diff --git a/catalog-processor/detekt-baseline.xml b/catalog-processor/detekt-baseline.xml new file mode 100644 index 00000000..0ab6758a --- /dev/null +++ b/catalog-processor/detekt-baseline.xml @@ -0,0 +1,12 @@ + + + + + CyclomaticComplexMethod:CatalogProcessor.kt$CatalogProcessor$private fun generateWrapper( func: KSFunctionDeclaration, wrapperName: String, demoContainer: String, ) + LongMethod:CatalogProcessor.kt$CatalogProcessor$private fun generateWrapper( func: KSFunctionDeclaration, wrapperName: String, demoContainer: String, ) + MaxLineLength:CatalogProcessor.kt$CatalogProcessor$"TextField(value = $paramName.toString(), onValueChange = { $paramName = it.toIntOrNull() ?: 0 }, label = { Text(%S) }, keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Number))" + WildcardImport:CatalogProcessor.kt$import com.google.devtools.ksp.processing.* + WildcardImport:CatalogProcessor.kt$import com.google.devtools.ksp.symbol.* + WildcardImport:CatalogProcessor.kt$import com.squareup.kotlinpoet.* + + diff --git a/core-common/detekt-baseline.xml b/core-common/detekt-baseline.xml new file mode 100644 index 00000000..663623ea --- /dev/null +++ b/core-common/detekt-baseline.xml @@ -0,0 +1,13 @@ + + + + + CyclomaticComplexMethod:PagedListReducer.kt$PagedListReducer$fun reduce(items: List<T>, state: PagingState<E>): CommonUiState<PagedListUiModel<T>> + EmptyFunctionBlock:AnalyticsTracker.kt$NoOpAnalyticsTracker${} + EmptyFunctionBlock:Logger.kt$NoOpLogger${} + ReturnCount:PagingMediator.kt$PagingMediator$@Suppress("TooGenericExceptionCaught") override suspend fun loadNextPage() + TooGenericExceptionThrown:PagingMediatorTest.kt$PagingMediatorTest$throw RuntimeException("count failed") + TooGenericExceptionThrown:PagingMediatorTest.kt$PagingMediatorTest.Harness$throw RuntimeException("fetch $key failed") + TooGenericExceptionThrown:PagingMediatorTest.kt$PagingMediatorTest.RecordingStorage$throw RuntimeException("write failed") + + diff --git a/core/designsystem/detekt-baseline.xml b/core/designsystem/detekt-baseline.xml index 3b17b23d..9bacb7c6 100644 --- a/core/designsystem/detekt-baseline.xml +++ b/core/designsystem/detekt-baseline.xml @@ -21,7 +21,7 @@ MagicNumber:DialogWithProgressBar.kt$0.01f MagicNumber:DialogWithProgressBar.kt$50 MagicNumber:DialogWithProgressBar.kt$500 - MatchingDeclarationName:DialogWithProgressBar.kt$CatalogSettings + MagicNumber:DialogWithProgressBar.kt$DialogProgressProvider$0.5f MatchingDeclarationName:Spacing.kt$BillionBeersSpacing SwallowedException:ComposeExtensions.kt$e: Exception TooGenericExceptionCaught:ComposeExtensions.kt$e: Exception diff --git a/docs/adr/0013-convention-plugin-form.md b/docs/adr/0013-convention-plugin-form.md new file mode 100644 index 00000000..325cc3a5 --- /dev/null +++ b/docs/adr/0013-convention-plugin-form.md @@ -0,0 +1,91 @@ +# 0013: Convention plugins stay precompiled script plugins, and the alternative gets measured before it gets adopted + +## Status + +Accepted. + +## Context + +The build's conventions live in `build-logic/convention/src/main/kotlin` as **precompiled script +plugins** — sixteen `billionbeers.*.gradle.kts` files whose plugin id is their filename. Two +**binary plugins** (`DuplicateClassesPlugin`, `UnusedDependenciesPlugin`) sit beside them as +ordinary `Plugin` classes, so the build already runs both forms and the wiring for either +is proven. + +This is not the original shape. The project moved *from* binary plugins *to* precompiled scripts +deliberately, on the grounds that they are simpler, easier to maintain, and faster. That history is +the reason this ADR exists: without it written down, the reverse migration looks like an obvious +improvement to anyone reading the code fresh, and it was in fact proposed on exactly that basis in +August 2026. + +The case for converting back is real but narrower than it first appears: + +- **The Gradle 9 accessor workaround would disappear.** Six conventions apply their siblings with + `apply(plugin = "billionbeers.…")` instead of a `plugins { }` block, to route around an + accessor-generation bug. Binary plugins generate no accessors, so the bug cannot apply and the + workaround could be deleted. +- **Build logic would become unit-testable** with `ProjectBuilder`/TestKit. In a repo that + mechanically enforces thirteen architecture invariants on application code, the build logic is + the only substantial untested part. +- **Configuration time.** Gradle generates a precompiled script plugin's class at configuration + time, and the widely-cited Now in Android figure is 12.7s → 0.76s on conversion. + +Two things weaken that third point specifically, which is the one usually used to justify the +migration: + +1. This build has `org.gradle.configuration-cache=true`. On a cache *hit* the configuration phase + is not executed at all, so the cost is not paid. It surfaces only on cache misses — CI cold + runs, build-script edits, and `build-logic` changes. +2. The measured numbers here are not consistent with a 12-second configuration phase. ADR 0011 + records a 4s warm clean build, and `make build-budget` on 2026-08-09 measured a 3.3s warm clean + build and 1.6s incremental. Whatever configuration costs in this project, it is a small fraction + of that. + +Against the migration: the precompiled form is what the team chose for readability and maintenance, +and one commonly-cited disadvantage of it turns out to be false. Precompiled script plugins **can** +share helper functions — a top-level declaration in a plain `.kt` file in the same source set is +visible to every script, which is how `Versions.kt` and `AndroidCommon.kt` already work. So the +"you must go binary to factor out duplication" argument does not hold, and the two largest reuse +cleanups (a shared `configureBillionBeersAndroid`, and a `billionbeers.android.testing` plugin) +were both delivered in the precompiled form with no migration at all. + +## Decision + +**Conventions stay precompiled script plugins.** No migration, opportunistic or otherwise. + +The two existing binary plugins stay binary — they are task-and-extension implementations rather +than conventions, which is what that form is good at. + +**The comparison is a measurement task, not a judgement call.** Before this is reopened, someone +should measure it on this repo rather than argue from another project's numbers: + +1. Pick one convention — `billionbeers.android.testing` is the smallest and has no accessor use. +2. Convert it to a `Plugin` on a branch, registered via `gradlePlugin { plugins { … } }`. + Consumers do not change; the plugin id is the same. +3. Measure configuration time on a **configuration-cache miss** both ways, since that is the only + state where the difference can appear: + `./gradlew help --no-configuration-cache` and a `build-logic`-touching run, several iterations. +4. Compare against `config/build-time-budget.txt` methodology — medians, not single runs, and on a + machine doing nothing else (ADR 0011). + +## Consequences + +- The `apply(plugin = "billionbeers.…")` workaround stays, and stays commented, until either the + Gradle bug is confirmed fixed or the measurement above justifies conversion. Flipping one back to + a `plugins { }` block and running `./gradlew help` is the cheap way to test the former. +- Build logic remains untested. That is a real cost and the strongest argument on the other side; + it is accepted for now because the conventions are small and heavily commented, and because the + invariants in `:konsist` cover the *outcomes* the conventions produce. +- **Reopen triggers:** a measured configuration-time difference that matters at this repo's scale; + a decision to publish a convention (Gradle's docs recommend converting to binary before + publishing — note the two artifacts currently earmarked for extraction are already binary); or a + Gradle release that makes precompiled scripts materially worse. + +## Related + +- Declarative Gradle (`.gradle.dcl`) is a separate question and further out. It replaces the + *consumer* build files, not the build logic, and requires binary plugins registering software + types underneath — so it sits on top of this decision rather than competing with it. It remains + experimental, with prototype-grade AGP support, and has no expression for several things this + build does (the baseline-profile `finalizeDsl` hook, the Paparazzi source generation, the + auto-discovering `settings.gradle.kts`). Not before a stable release. diff --git a/feature/beerbrowse/detekt-baseline.xml b/feature/beerbrowse/detekt-baseline.xml new file mode 100644 index 00000000..6d56e2b3 --- /dev/null +++ b/feature/beerbrowse/detekt-baseline.xml @@ -0,0 +1,10 @@ + + + + + EmptyFunctionBlock:BrowseBeersViewModelTest.kt$BrowseBeersViewModelTest${} + LongParameterList:BrowseBeersScreen.kt$( title: String, viewState: CommonUiState<PagedListUiModel<Beer>>, onBack: () -> Unit, onBeerClick: (Beer) -> Unit, onScrollToBottom: () -> Unit, onRetryLoadMore: () -> Unit, // Serves both first-page reloads: the full-screen error retry and the pull-to-refresh gesture. onRetryFirstPage: () -> Unit, ) + LongParameterList:BrowseHomeScreen.kt$( styles: CommonUiState<List<BeerStyle>>, breweries: CommonUiState<List<Brewery>>, selectedTab: Int, onTabSelected: (Int) -> Unit, onStyleClick: (BeerStyle) -> Unit, onBreweryClick: (Brewery) -> Unit, onBack: () -> Unit, onRetryStyles: () -> Unit, onRetryBreweries: () -> Unit, ) + MatchingDeclarationName:BeerBrowseScreenImpl.kt$BrowseSelection + + diff --git a/feature/beersearch/detekt-baseline.xml b/feature/beersearch/detekt-baseline.xml new file mode 100644 index 00000000..c7612e7d --- /dev/null +++ b/feature/beersearch/detekt-baseline.xml @@ -0,0 +1,8 @@ + + + + + EmptyFunctionBlock:BeersSearchViewModelTest.kt$BeersSearchViewModelTest${} + LongParameterList:BeersSearchScreen.kt$( viewState: CommonUiState<PagedListUiModel<Beer>>, query: String, onQueryChange: (String) -> Unit, onBeerClick: (Beer) -> Unit, onBack: () -> Unit, onScrollToBottom: () -> Unit, onRetryLoadMore: () -> Unit, onRetrySearch: () -> Unit, autoFocus: Boolean = true, ) + + diff --git a/feature/beerslist/detekt-baseline.xml b/feature/beerslist/detekt-baseline.xml index 6fc2e5b7..ed22813b 100644 --- a/feature/beerslist/detekt-baseline.xml +++ b/feature/beerslist/detekt-baseline.xml @@ -2,6 +2,6 @@ - MagicNumber:BeersListScreen.kt$300 + LongParameterList:BeersListScreen.kt$( viewState: CommonUiState<PagedListUiModel<Beer>>, onBeerClick: (Beer) -> Unit, onSearchClick: () -> Unit, onBrowseClick: () -> Unit, onScrollToBottom: () -> Unit, onRefresh: () -> Unit, onRetry: () -> Unit, onRetryLoadMore: () -> Unit, ) diff --git a/konsist/detekt-baseline.xml b/konsist/detekt-baseline.xml new file mode 100644 index 00000000..f8f43e03 --- /dev/null +++ b/konsist/detekt-baseline.xml @@ -0,0 +1,7 @@ + + + + + ForbiddenComment:TestFixturesPluginBoundaryTest.kt$TestFixturesPluginBoundaryTest$* Backs ADR 0001 (docs/adr/0001-test-fixtures-via-sibling-modules.md): test fixtures live in * sibling `:module:fakes` / `:module:fixtures` modules, never Gradle's `java-test-fixtures` plugin. * That was decided on measured build-time cost, not taste - the plugin adds a variant to every * consumer and the ADR records what that did to build times here. * * The decision was drifting back in on its own: `beerdomain/api/build.gradle.kts` carried a "TODO: * try testFixtures instead" with commented-out `testFixturesImplementation` lines, which is how a * settled ADR quietly becomes a suggestion. Deleting the TODO fixes today; this rule fixes the next * time. * * Matching is on the plugin id and deliberately ignores comment lines - see [uncommentedText]. + + diff --git a/presentation_utils/detekt-baseline.xml b/presentation_utils/detekt-baseline.xml new file mode 100644 index 00000000..864dc8d9 --- /dev/null +++ b/presentation_utils/detekt-baseline.xml @@ -0,0 +1,8 @@ + + + + + LongParameterList:DynamicFeatureInstallerTest.kt$DynamicFeatureInstallerTest$( status: Int, sessionId: Int = SESSION_ID, errorCode: Int = SplitInstallErrorCode.NO_ERROR, downloaded: Long = 0, total: Long = 0, modules: List<String> = listOf(MODULE), ) + MatchingDeclarationName:InfiniteListHandler.kt$ListPosition + + diff --git a/snapshot-processor/detekt-baseline.xml b/snapshot-processor/detekt-baseline.xml new file mode 100644 index 00000000..a5475fe0 --- /dev/null +++ b/snapshot-processor/detekt-baseline.xml @@ -0,0 +1,15 @@ + + + + + ReturnCount:SnapshotProcessor.kt$SnapshotProcessor$private fun isValidPreview(func: KSFunctionDeclaration): Boolean + SpreadOperator:SnapshotProcessor.kt$SnapshotProcessor$(true, *resolver.getAllFiles().toList().toTypedArray()) + SwallowedException:SnapshotProcessor.kt$SnapshotProcessor$e: Exception + SwallowedException:SnapshotProcessor.kt$SnapshotProcessor$e: FileAlreadyExistsException + TooGenericExceptionCaught:SnapshotProcessor.kt$SnapshotProcessor$e: Exception + UnusedPrivateProperty:SnapshotProcessor.kt$SnapshotProcessor$val previewSymbols = resolver.getSymbolsWithAnnotation("androidx.compose.ui.tooling.preview.Preview") + WildcardImport:SnapshotProcessor.kt$import com.google.devtools.ksp.processing.* + WildcardImport:SnapshotProcessor.kt$import com.google.devtools.ksp.symbol.* + WildcardImport:SnapshotProcessor.kt$import com.squareup.kotlinpoet.* + + diff --git a/testing-utils-android/detekt-baseline.xml b/testing-utils-android/detekt-baseline.xml new file mode 100644 index 00000000..65727533 --- /dev/null +++ b/testing-utils-android/detekt-baseline.xml @@ -0,0 +1,7 @@ + + + + + TooManyFunctions:BaseTestRobot.kt$BaseTestRobot + + From ae4fa04b0bbd2c310a9ec53f7a9798a34e9991e5 Mon Sep 17 00:00:00 2001 From: simtop Date: Sun, 9 Aug 2026 18:13:26 +0200 Subject: [PATCH 2/2] feat: run Detekt in CI, and give it an unblock path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps in the gate from the previous commit. Detekt has never run in ANY workflow - grep the whole .github/workflows tree and the only hits are the new baseline job. It was local-and-advisory, so flipping ignoreFailures made it fail `make lint` and nothing else. It now runs on the format-check job, which already has Gradle set up and always runs. Added as a step on an existing job rather than a new one, so "CI Gate" needs no branch-protection change. It uses `success() || failure()` so a Spotless failure cannot hide a Detekt failure and send you round twice. And a blocking gate needs an unblock path, the way formatting has one: `make detekt-baseline [MODULE=:foo]`, plus detekt_baseline.yml to run it on a branch. The workflow is deliberately NOT the format_fix twin, and the header says why. spotlessApply is a deterministic mechanical transform, so committing its output needs no review. A baseline is a suppression list: running it automatically would mean every new finding silences itself, which is the state this replaced, reached automatically instead of by a config flag. So it is dispatch-only, refuses to run on master, and writes the rules it suppressed into the job summary - the committed diff being the real review surface. The CI hint is worded differently from the Format Fix hint for the same reason. For Spotless the advice is always "run the fixer". For Detekt it is "fix the code", with re-baselining as the explicit exception for a pre-existing or deliberate finding. AGENTS.md §5 said "make lint is Detekt only" in a way that implied it was not a CI gate, which was true and no longer is. It now describes all three lint gates and the baseline rule. --- .github/workflows/ci.yml | 41 +++++++++ .github/workflows/detekt_baseline.yml | 118 ++++++++++++++++++++++++++ AGENTS.md | 16 +++- Makefile | 4 +- 4 files changed, 173 insertions(+), 6 deletions(-) create mode 100644 .github/workflows/detekt_baseline.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4ff43be9..961df0c3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,6 +40,21 @@ jobs: id: spotless-check run: ./gradlew spotlessCheck + # Detekt had never run in ANY workflow - it was a local-only, advisory tool, and with + # `ignoreFailures = true` it could not fail even there. Both halves are fixed now, so it + # belongs on the lane that already has Gradle set up and always runs. + # + # `success() || failure()` rather than plain ordering: a Spotless failure must not hide a + # Detekt failure, or fixing one just uncovers the other on the next push. + # + # Added as a step on an existing job, deliberately. "CI Gate" lists jobs, not steps, so this + # needs no branch-protection change (AGENTS.md: add new lanes to ci-gate needs, not to the + # protection rule). + - name: Run Detekt + id: detekt-check + if: success() || failure() + run: ./gradlew detekt + # Lives here because this lane always runs and already has Gradle set up. Guards the one # verification gap CI is otherwise blind to: artifacts only Android Studio's sync resolves. # Without it a stale ledger is invisible until a human hits it in the IDE (ADR 0007). @@ -74,6 +89,32 @@ jobs: echo "" >> $GITHUB_STEP_SUMMARY echo "Or trigger it manually from the Actions tab (pick this branch in the \"Run workflow\" dropdown)." >> $GITHUB_STEP_SUMMARY + # Deliberately worded differently from the Format Fix hint above. Spotless output is a + # mechanical transform, so "run the fixer" is always the right advice. A Detekt baseline is a + # suppression, so the right advice is "fix the code", and the workflow is the exception. + - name: Detekt Hint + if: always() && steps.detekt-check.outcome == 'failure' + run: | + BRANCH_NAME="${{ github.head_ref || github.ref_name }}" + echo "### 🔍 Detekt Found New Issues" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "The backlog that existed when this gate was adopted is already baselined, so a failure here means **new** findings. Fix them." >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "Reproduce locally:" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "\`\`\`bash" >> $GITHUB_STEP_SUMMARY + echo "make lint" >> $GITHUB_STEP_SUMMARY + echo "\`\`\`" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "If — and only if — the finding is pre-existing or a deliberate exception, re-baseline it. **This suppresses the rule, so the diff is the review**:" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "\`\`\`bash" >> $GITHUB_STEP_SUMMARY + echo "make detekt-baseline MODULE=:the:module # locally, then commit the diff" >> $GITHUB_STEP_SUMMARY + echo "gh workflow run detekt_baseline.yml --ref $BRANCH_NAME # or on the branch" >> $GITHUB_STEP_SUMMARY + echo "\`\`\`" >> $GITHUB_STEP_SUMMARY + echo "" >> $GITHUB_STEP_SUMMARY + echo "Never re-baseline to silence a finding you just introduced — that is how the gate stops working." >> $GITHUB_STEP_SUMMARY + # Change-scope detector with per-lane verdict adoption. The heavy test lanes (unit / screenshot # / instrumented) key their `if:` off one output each. Two levels: # diff --git a/.github/workflows/detekt_baseline.yml b/.github/workflows/detekt_baseline.yml new file mode 100644 index 00000000..4c54425d --- /dev/null +++ b/.github/workflows/detekt_baseline.yml @@ -0,0 +1,118 @@ +name: Detekt Baseline + +# Dispatch against a branch to regenerate the Detekt baselines and commit them to that same branch. +# The sibling of format_fix.yml, for the gate that replaced `ignoreFailures = true`. +# +# IT IS DELIBERATELY NOT AUTOMATIC, and that is the whole design note. spotlessApply is a +# deterministic mechanical transform, so format_fix.yml can run and commit with nothing for a +# reviewer to approve. A Detekt baseline is the opposite: it is a *suppression list*. Running this +# on every push, or on a red PR, would mean any new finding silences itself and the gate never +# fails again - the exact state this replaced, reached automatically instead of by a config flag. +# +# So: manual dispatch only, never on `push` or `pull_request`, and it prints what it suppressed +# into the job summary. The baseline files are committed, so the PR diff is the real review surface +# - a reviewer can see precisely which rules were added and object. +# +# When to dispatch it: +# - onboarding a module that had no baseline +# - after genuinely FIXING findings, to shrink a baseline +# - a deliberate, reviewed decision to grandfather something +# +# When NOT to: CI went red on a finding you just introduced. Fix the code. That is the point. +# +# Push uses the write deploy key, not GITHUB_TOKEN, for the reason in ADR 0007: events created with +# GITHUB_TOKEN do not trigger workflow runs, so the commit would move an open PR's head to a SHA CI +# never runs on, leaving "CI Gate" pending forever and `gh pr merge --auto` hanging silently. + +on: + workflow_dispatch: {} + +permissions: + contents: read + +concurrency: + group: detekt-baseline-${{ github.ref_name }} + cancel-in-progress: false + +jobs: + baseline: + name: Regenerate Detekt baselines on the branch + runs-on: ubuntu-latest + steps: + - name: Refuse to run on the default branch + # Baselines suppress findings; doing that on master without review is precisely the failure + # this workflow's design is trying to avoid. Branch protection would reject the push anyway, + # so fail here with a message that says why. + if: github.ref_name == github.event.repository.default_branch + run: | + echo "::error::Dispatch this against a feature branch, not ${{ github.ref_name }}." + echo "It commits directly to the branch it runs on, and a baseline is a suppression." + exit 1 + + - name: Checkout branch + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + ref: ${{ github.ref_name }} + ssh-key: ${{ secrets.VERIFICATION_METADATA_DEPLOY_KEY }} + + - name: Set up JDK 23 + uses: actions/setup-java@03ad4de0992f5dab5e18fcb136590ce7c4a0ac95 # v5 + with: + java-version: '23' + distribution: 'temurin' + + - name: Setup Gradle + uses: gradle/actions/setup-gradle@3f131e8634966bd73d06cc69884922b02e6faf92 # v6 + with: + gradle-version: wrapper + + - name: Make gradlew executable + run: chmod +x gradlew + + - name: Regenerate Detekt baselines + run: ./gradlew detektBaseline + + - name: Summarise what changed, then commit + run: | + git config user.name "github-actions[bot]" + git config user.email "github-actions[bot]@users.noreply.github.com" + + # -u, not -A: only files already tracked. The repo deliberately carries untracked + # local-only content (planning notes) that must never be committed. + git add -u + # Newly created baselines are untracked, so add those explicitly by name. + git ls-files --others --exclude-standard -- '**/detekt-baseline.xml' \ + | xargs -r git add -- + + if git diff --cached --quiet; then + echo "No baseline changes - Detekt findings already match the committed baselines." + echo "No baseline changes." >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi + + # Make the suppression visible in the run itself, not only in the eventual diff. Counting + # added lines per file is enough to see at a glance whether this quietly grew. + { + echo "## Detekt baseline changes" + echo "" + echo "A baseline is a suppression list. Review these before merging." + echo "" + echo "| file | rules added | rules removed |" + echo "|---|---|---|" + for f in $(git diff --cached --name-only -- '**/detekt-baseline.xml'); do + added=$(git diff --cached -- "$f" | grep -c '^+.*' || true) + removed=$(git diff --cached -- "$f" | grep -c '^-.*' || true) + echo "| \`$f\` | $added | $removed |" + done + echo "" + echo "Rules newly suppressed:" + echo "" + echo '```' + git diff --cached -- '**/detekt-baseline.xml' \ + | grep '^+.*' | sed 's/^+\s*//' | sort | uniq -c | sort -rn || true + echo '```' + } >> "$GITHUB_STEP_SUMMARY" + + git commit -m "chore: regenerate Detekt baselines" + git push origin HEAD:"$GITHUB_REF_NAME" + echo "Pushed a baseline commit to $GITHUB_REF_NAME - review the diff before merging." diff --git a/AGENTS.md b/AGENTS.md index 201afbb2..81f9a49f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -178,10 +178,18 @@ declaring done. 4. **Screenshots** — `make screenshot-verify` (record with `make screenshot-record` and inspect the PNGs; they are your eyes on the UI) 5. **Lint / format** — `make lint`, `make format`, and **`make android-lint` whenever resources - change**. `make lint` is Detekt only; the Android Lint gate CI runs is `make android-lint` - (`:app:lintDebug`, checkDependencies across the whole graph). A string added to - `values/strings.xml` without its `values-fr` / `values-es` siblings passes every other rung and - fails CI with `MissingTranslation` — that is how PR #140 broke master. + change**. These are three separate gates and all three now fail: + - `make lint` is **Detekt**, and since the gate was adopted it **fails on new findings** and runs + in CI on the `format-check` job. The backlog present at adoption is frozen in per-module + `detekt-baseline.xml`; burn those down, and **never regenerate one to bury a new finding** + (same rule as `lint-baseline.xml`). To grandfather something deliberately: + `make detekt-baseline MODULE=:foo`, or dispatch `detekt_baseline.yml` against the branch — the + committed diff is the review. Test and `androidTest` sources are scanned too. + - `make android-lint` is **Android Lint** (`:app:lintDebug`, checkDependencies across the whole + graph), gated by `app/lint-baseline.xml`. A string added to `values/strings.xml` without its + `values-fr` / `values-es` siblings passes every other rung and fails CI with + `MissingTranslation` — that is how PR #140 broke master. + - `make format` is Spotless; CI runs `spotlessCheck` and `format_fix.yml` can apply it for you. 6. **Device** — instrumented tests, install, logcat: use the `billionbeers-android` skill, not ad-hoc `adb` diff --git a/Makefile b/Makefile index 9ccf75c1..143dbf36 100644 --- a/Makefile +++ b/Makefile @@ -17,7 +17,7 @@ UI_TEST_PREFIX = $(if $(MODULE_TRIMMED),$(MODULE_TRIMMED):,:app:) # Wrapper for Gradle to support build-brief (bb) or rtk if available GRADLE_RUNNER := $(shell if command -v bb >/dev/null 2>&1; then echo "bb ./gradlew"; elif command -v build-brief >/dev/null 2>&1; then echo "build-brief --gradle ./gradlew"; elif command -v rtk >/dev/null 2>&1; then echo "rtk ./gradlew"; else echo "./gradlew"; fi) -.PHONY: help setup setup-ai-tools update-android-skills build bundle-release install clean test konsist compose-metrics ui-test screenshot-record screenshot-verify screenshot-clean lint android-lint format check check-duplicates check-unused-deps dependency-guard dependency-guard-baseline verification-metadata health benchmark-micro benchmark-macro benchmark-check generate-baseline gradle-benchmark build-budget build-budget-check jacoco-report coverage-check install-profiler install-diffuse new-feature-module new-dev-app play-listing-check play-listing-capture play-listing-reset store-frames +.PHONY: detekt-baseline help setup setup-ai-tools update-android-skills build bundle-release install clean test konsist compose-metrics ui-test screenshot-record screenshot-verify screenshot-clean lint android-lint format check check-duplicates check-unused-deps dependency-guard dependency-guard-baseline verification-metadata health benchmark-micro benchmark-macro benchmark-check generate-baseline gradle-benchmark build-budget build-budget-check jacoco-report coverage-check install-profiler install-diffuse new-feature-module new-dev-app play-listing-check play-listing-capture play-listing-reset store-frames help: ## Show this help message. @echo "\n📊 BillionBeers Makefile Help" @@ -144,7 +144,7 @@ lint: ## Run static analysis (Detekt). android-lint: ## Run Android Lint over the app and its whole library graph (checkDependencies), gated by app/lint-baseline.xml. $(GRADLE_RUNNER) :app:lintDebug -detekt-baseline: ## Update Detekt baselines for all modules. +detekt-baseline: ## Re-baseline Detekt (all modules, or MODULE=:foo). ALWAYS review the diff - a baseline is a suppression, and regenerating one to silence a NEW finding buries it. $(GRADLE_RUNNER) $(MODULE_PREFIX)detektBaseline format: ## Apply code formatting (Spotless).