Skip to content

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

Merged
simtop merged 2 commits into
masterfrom
feat/detekt-gates-and-adr-0013
Aug 9, 2026
Merged

Make Detekt a real gate, and record the convention-plugin form as ADR 0013#159
simtop merged 2 commits into
masterfrom
feat/detekt-gates-and-adr-0013

Conversation

@simtop

@simtop simtop commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Two things: Detekt becomes a gate that can actually fail, and ADR 0013 records the convention-plugin form so it stops being re-litigated.

Detekt was not a gate

ignoreFailures = true meant it could not fail on anything, however bad — while AGENTS.md §5 advertises make lint as rung 5 of the verification ladder. It produced reports nobody was required to read. 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 never analysed at all, even in advisory mode.

Both are fixed the way the Android Lint gate here already works: the backlog present at adoption is grandfathered into per-module baselines, and a new finding fails the build. That preserves the property that matters — clearing the existing findings stays a batching decision, taken whenever it suits, rather than something that blocks work today.

Adoption cost: 36 findings in main sources, plus 13 more once test/androidTest are scanned, across 17 modules. What is being grandfathered, so it is a reviewed decision rather than a silent one:

module rules
beer_data CyclomaticComplexMethod, TooManyFunctions
beer_database LongParameterList, MagicNumber, TooManyFunctions
beerdomain/fakes TooManyFunctions, TopLevelPropertyNaming
core/designsystem MagicNumber
feature/beerslist LongParameterList
+ 12 more mostly MagicNumber / TooManyFunctions in processors and test sources

Nothing was regenerated silently. Pre-existing baselines were restored first and refreshed only for modules that actually failed — the rule AGENTS.md states for lint-baseline.xml ("never by regenerating the baseline to bury a regression") applies identically here and is now written into the convention.

A bug that had to be fixed first

The extension sets baseline to null until the file exists. That 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. The create task now gets the path unconditionally; it is an output, not an input.

Verified by mutation probe

A file tripping LongParameterList and MagicNumber:

> Task :core-common:detekt FAILED
LongParameterList - 7/6 - [The function tooMany(...) has too many parameters]

Removing it returns the build to green.

ADR 0013: convention plugins stay precompiled

I proposed converting them to binary plugins earlier this month. That was re-litigating a decision already taken deliberately — the project moved to precompiled scripts from binary plugins, for simplicity and maintenance — and my case was weaker than I presented it:

  • The "you must go binary to share logic" argument is false. Precompiled script plugins can call top-level functions from a plain .kt in the same source set, which is how Versions.kt and AndroidCommon.kt already work. Both large reuse cleanups landed with no migration at all.
  • The configuration-time argument leans on Now in Android's 12.7s → 0.76s, but this build has the configuration cache on, so that cost is not paid on a hit; and the measured numbers here (3.3s warm clean build, 1.6s incremental) are not consistent with a 12-second configuration phase anyway.

What survives is narrower and honest: the Gradle 9 accessor workaround would disappear, and build logic would become unit-testable — the only substantial untested part of a repo that enforces 13 invariants on everything else.

So the ADR keeps precompiled, and turns the comparison into a measurement task with a written method (convert one plugin, measure configuration time on a cache miss both ways, medians per ADR 0011) rather than a judgement call. It also records where Declarative Gradle sits: on top of binary plugins, not against them, and not before a stable release.

AGENTS.md's settled-decisions table points at it so the next reader does not re-propose it.

Verification

make test, make konsist, make lint, make screenshot-verify, spotlessCheck, plus the mutation probe above.

simtop added 2 commits August 9, 2026 18:05
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.
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.
@simtop
simtop enabled auto-merge (squash) August 9, 2026 16:19
@simtop
simtop merged commit 7e20c7c into master Aug 9, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant