Make Detekt a real gate, and record the convention-plugin form as ADR 0013

.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:
   #

.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 <ID> 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 '^+.*<ID>' || true)
+              removed=$(git diff --cached -- "$f" | grep -c '^-.*<ID>' || true)
+              echo "| \`$f\` | $added | $removed |"
+            done
+            echo ""
+            echo "Rules newly suppressed:"
+            echo ""
+            echo '```'
+            git diff --cached -- '**/detekt-baseline.xml' \
+              | grep '^+.*<ID>' | 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."

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:
 
@@ -177,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`
 

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).

app/detekt-baseline.xml

@@ -2,8 +2,7 @@
 <SmellBaseline>
   <ManuallySuppressedIssues/>
   <CurrentIssues>
-    <ID>ForbiddenComment:MainCoroutineScopeRule.kt$// TODO: refactor</ID>
-    <ID>TopLevelPropertyNaming:TestingConstants.kt$const val fakeErrorName = "Error getting list of beers"</ID>
-    <ID>WildcardImport:MainCoroutineScopeRule.kt$import kotlinx.coroutines.test.*</ID>
+    <ID>EmptyFunctionBlock:FakeSplitInstallManager.kt$FakeSplitInstallManager${}</ID>
+    <ID>MatchingDeclarationName:LocalDataSourceTest2.kt$LocalDataSourceTest</ID>
   </CurrentIssues>
 </SmellBaseline>

beer_data/detekt-baseline.xml

@@ -2,7 +2,7 @@
 <SmellBaseline>
   <ManuallySuppressedIssues/>
   <CurrentIssues>
-    <ID>SwallowedException:BeersRepositoryImpl.kt$BeersRepositoryImpl$e: Exception</ID>
-    <ID>TooGenericExceptionCaught:BeersRepositoryImpl.kt$BeersRepositoryImpl$e: Exception</ID>
+    <ID>CyclomaticComplexMethod:BeersMapper.kt$BeersMapper$fun fromBeersApiResponseItemToBeer(response: BeersApiResponseItem?): Beer</ID>
+    <ID>TooManyFunctions:BeersRepositoryImpl.kt$BeersRepositoryImpl : BeersRepository</ID>
   </CurrentIssues>
 </SmellBaseline>

beer_database/detekt-baseline.xml

@@ -2,7 +2,9 @@
 <SmellBaseline>
   <ManuallySuppressedIssues/>
   <CurrentIssues>
-    <ID>LongParameterList:BeersDao.kt$BeersDao$( id: String, name: String, tagline: String, description: String, imageUrl: String, abv: Double, ibu: Double, foodPairing: String, )</ID>
+    <ID>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, )</ID>
+    <ID>MagicNumber:Migrations.kt$&lt;no name provided&gt;$3</ID>
     <ID>MatchingDeclarationName:Converter.kt$Converters</ID>
+    <ID>TooManyFunctions:BeersDao.kt$BeersDao</ID>
   </CurrentIssues>
 </SmellBaseline>

beer_network/detekt-baseline.xml

@@ -0,0 +1,7 @@
+<?xml version="1.0" ?>
+<SmellBaseline>
+  <ManuallySuppressedIssues/>
+  <CurrentIssues>
+    <ID>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, )</ID>
+  </CurrentIssues>
+</SmellBaseline>

beerdomain/api/detekt-baseline.xml

@@ -0,0 +1,7 @@
+<?xml version="1.0" ?>
+<SmellBaseline>
+  <ManuallySuppressedIssues/>
+  <CurrentIssues>
+    <ID>TooManyFunctions:BeersRepository.kt$BeersRepository</ID>
+  </CurrentIssues>
+</SmellBaseline>

beerdomain/fakes/detekt-baseline.xml

@@ -2,6 +2,7 @@
 <SmellBaseline>
   <ManuallySuppressedIssues/>
   <CurrentIssues>
-    <ID>EmptyFunctionBlock:FakeBeersRepository.kt$FakeBeersRepository${}</ID>
+    <ID>TooManyFunctions:FakeBeersRepository.kt$FakeBeersRepository : BeersRepository</ID>
+    <ID>TopLevelPropertyNaming:BeerFixtures.kt$const val fakeErrorName = "Error getting list of beers"</ID>
   </CurrentIssues>
 </SmellBaseline>