Clear the 35 findings the new tools reported

detekt found 29 and Android lint 6, on a codebase neither had ever seen. Each one was either fixed or relaxed with the reason written next to it; nothing was suppressed to make the build quiet. Fixed, because the tool was right: - ConversionDependencies constructs Media3Engine, which is @UnstableApi, and was not marked. Every other type here that touches Media3 propagates the marker rather than swallowing it with @OptIn, so this one does too. Lint was the only thing that had ever noticed. - MediaProbe converted microseconds to milliseconds with a bare 1000, twice, in a file that also handles a seconds-based duration from a different API. US_PER_MS and MS_PER_SECOND now say which is which -- that confusion is a real bug source in media code, not a style question. - Foreground service types compared SDK_INT against 34 and 35 as raw ints while the doc comment above spelled the version names out. VERSION_CODES says it in the code. - take(3) is a product decision about how many alternatives an error offers. It means nothing until it is named; MAX_SUGGESTIONS does. - setProgress(100, ...) is a percentage max, now PERCENT_MAX. Relaxed, because the rule did not fit: - The model package is excluded from ReturnCount and CyclomaticComplexMethod ONLY. It is the decision layer: ConversionRouter.route scores 17 because the app can give 17 distinct answers to "which engine, and why", each with its own user-visible reason, and route's own comment records that their ORDER decides which message is shown. Counting those as complexity measures how many answers exist, not how hard the code is to follow. Everything else -- LongMethod, NestedBlockDepth, ComplexCondition -- still applies there. - A flat `when` used as a lookup table scores a point per entry, so MediaProbe's demuxer-name to Container map read as complexity 21 with no nesting and no state. ignoreSingleWhenExpression is the rule's own answer. - TooGenericExceptionCaught off. MediaProbe, ConversionWorker and ConcatWorker sit in front of native code that reports a malformed file as anything from IllegalArgumentException to a bare RuntimeException, undocumented. Enumerating that list means guessing, and a wrong guess crashes the app on a file it could have reported as unreadable. SwallowedException stays on, so these still have to log and handle. - SI thresholds in the byte formatter, via ignoreNumbers. Each literal sits on the line with the unit string it belongs to; BYTES_PER_MB would need a Long and a Double and say nothing the line does not. - allowedFunctionsPerObject, which the first pass simply missed. Lint's three version-freshness nags are off. They do not describe this code -- they go red the day someone else publishes a release, which turns a PR red for something its author cannot see in their diff, and they want the network at lint time. Upgrades here are deliberate; Kotlin in particular is pinned to AGP's bundled KGP and is not free to follow the newest release. UsableSpace is informational rather than disabled, because it is a real finding that this commit is choosing not to act on. hasSpaceFor reads File.usableSpace, which ignores reclaimable cache, so the app can refuse a conversion it had room for. StorageManager.getAllocatableBytes is the better answer, but it changes when a job is rejected and can throw -- a behaviour change to a safety check, which deserves its own commit and its own test rather than a drive-by here. informational keeps it in every lint report instead of hiding it. Also adds the CI gate and a CLAUDE.md. Coverage is reported and not gated: the measured baseline is 31% of lines, which is exactly why LibreMail's 0.84 floor was evidence about LibreMail and not a number to copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-22 14:21:15 -05:00
parent 496f1c7e73
commit 65a94b4ec1
9 changed files with 251 additions and 7 deletions
@@ -100,6 +100,69 @@ jobs:
          name: unit-test-report
          path: app/build/reports/tests/

+      # Reported, not gated. A coverage floor is only meaningful against a measured
+      # baseline, and this is the thing that measures it -- currently 31% of lines. Once
+      # that number has settled, a jacocoTestCoverageVerification task can hold it.
+      - name: Coverage report
+        run: ./gradlew :app:jacocoTestReport
+
+      - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
+        if: always()
+        with:
+          name: coverage-report
+          path: app/build/reports/jacoco/jacocoTestReport/
+
+  # ---------------------------------------------------------------------------
+  # Three tools, one job, because they answer three different questions and a
+  # developer wants all three answers at once rather than one per push.
+  #
+  #   ktlint  -- formatting. Owns it outright; detekt's formatting ruleset is off,
+  #              so the two can never disagree about the same line.
+  #   detekt  -- static analysis. Its config lives in config/detekt/detekt.yml and
+  #              overrides only the rules this codebase legitimately breaks.
+  #   lint    -- the Android-specific things neither of the others can see: opt-in
+  #              markers, API-level misuse, manifest and resource problems.
+  #
+  # --continue is what makes it one round trip: a ktlint failure still lets detekt
+  # and lint report, so a red run hands over the whole list rather than the first
+  # item on it.
+  #
+  # No emulator and no FFmpeg archive needed, so this is the cheapest gate here and
+  # deliberately does not depend on the ffmpeg job.
+  # ---------------------------------------------------------------------------
+  static-analysis:
+    name: Static analysis
+    runs-on: ubuntu-latest
+    timeout-minutes: 20
+    steps:
+      - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+
+      - uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5.7.0
+        with:
+          distribution: temurin
+          java-version: '17'
+
+      - uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
+        with:
+          path: ${{ env.GRADLE_CACHE_PATHS }}
+          key: gradle-${{ runner.os }}-${{ hashFiles('**/*.gradle.kts', 'gradle/libs.versions.toml', 'gradle/wrapper/gradle-wrapper.properties') }}
+          restore-keys: gradle-${{ runner.os }}-
+
+      - name: ktlint, detekt and Android lint
+        run: ./gradlew :app:ktlintCheck :app:detekt :app:lintDebug --continue --stacktrace
+
+      # The XML matters as much as the HTML: it is the one that can be diffed between
+      # runs to see what a change actually moved.
+      - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
+        if: always()
+        with:
+          name: static-analysis-reports
+          path: |
+            app/build/reports/ktlint/
+            app/build/reports/detekt/
+            app/build/reports/lint-results-debug.html
+            app/build/reports/lint-results-debug.xml
+
  # ---------------------------------------------------------------------------
  # One runner per API level, across the whole supported range.
  #
@@ -0,0 +1,84 @@
+# CLAUDE.md
+
+This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository.
+
+LibreMediaConverter is an Android media converter (Kotlin, Jetpack Compose, Material 3) with two
+conversion engines: Media3 Transformer for the hardware path and FFmpeg for everything the platform
+cannot do. See `@README.md` for the architecture and `@LICENSES/README.md` for the split license —
+this file covers only what is not obvious from the code.
+
+## Build, test, lint
+
+Use a **JDK 17–21** for the Gradle daemon. AGP 9 will not run on anything older than 17, and does
+not support 25+ — if `JAVA_HOME` points at 25, builds fail.
+
+```bash
+./gradlew :app:assembleDebug          # build debug APK
+./gradlew :app:testDebugUnitTest      # JVM unit tests
+./gradlew :app:ktlintCheck            # formatting
+./gradlew :app:ktlintFormat           # fix formatting in place
+./gradlew :app:detekt                 # static analysis
+./gradlew :app:lintDebug              # Android lint
+./gradlew :app:jacocoTestReport       # coverage (XML+HTML under app/build/reports/jacoco/)
+# single unit test:
+./gradlew :app:testDebugUnitTest --tests "org.libremediaconverter.model.ConversionRouterTest"
+```
+
+CI's "Static analysis" gate is exactly `./gradlew :app:ktlintCheck :app:detekt :app:lintDebug
+--continue`. Run it with `--continue` locally too: one round trip gives you all three lists instead
+of the first one that fails.
+
+**Before treating a change as done**, run: `assembleDebug` + `testDebugUnitTest` +
+`compileDebugAndroidTestKotlin` + `ktlintCheck` + `detekt` + `lintDebug`.
+`compileDebugAndroidTestKotlin` matters more here than it looks — the instrumented suite cannot run
+on this machine (below), so without it an androidTest compile error is not discovered until CI.
+ktlint and detekt also cover the `test`/`androidTest` source sets that `lintDebug` skips.
+
+## Instrumented tests do not run locally
+
+Two independent reasons, so do not spend time on either:
+
+- **Emulators segfault on this host.** qemu dies on every AVD. Instrumented tests run on CI or on
+  the physical Pixel, never in a local emulator.
+- **The API 37 image is broken.** `android-37.0` crash-loops surfaceflinger inside its own gralloc
+  mapper, so every test fails there regardless of this app. `docs/api-37-emulator-crash.md` records
+  the evidence and the ruled-out fixes; CI's matrix therefore stops at API 36 even though targetSdk
+  is 37. **API 37 needs a manual check on the Pixel 10 Pro XL before each release.**
+
+On a device or emulator, build only the ABI it can execute:
+
+```bash
+./gradlew :app:connectedDebugAndroidTest -PabiFilters=x86_64
+```
+
+FFmpeg's native libraries dominate the APK, so shipping arm64 to an x86_64 emulator doubles the
+install for code that can never run — and on API 37 the full APK does not fit at all.
+
+## Conventions
+
+- **ktlint owns formatting, detekt owns static analysis.** detekt's formatting ruleset is off, so
+  the two can never disagree about the same line. Never hand-fix a formatting complaint — run
+  `ktlintFormat`. Style is `intellij_idea` at 120 columns, set in `.editorconfig`.
+- **detekt config is `config/detekt/detekt.yml`**, merged onto detekt's defaults
+  (`buildUponDefaultConfig = true`), so it carries only the rules this codebase legitimately
+  breaks — each with the reason written next to it. Relax a rule that way or fix the code; never a
+  bare `@Suppress`. Do not invent config keys: unknown ones are rejected.
+- The `model` package is excluded from `ReturnCount` and `CyclomaticComplexMethod` only. It is the
+  decision layer, where one branch is one documented user-visible outcome and the metric counts
+  answers rather than complexity. Every other rule still applies there.
+- **Coverage is reported, not gated** — currently ~31% of lines. A floor needs a baseline that has
+  settled first.
+- `kotlin.code.style=official`. Gradle stays Kotlin DSL.
+
+## Traps
+
+- **Do not apply `org.jetbrains.kotlin.android`.** AGP 9 has built-in Kotlin; applying the legacy
+  plugin fails the build. This is why `libs.versions.toml` pins `kotlin` to AGP's bundled KGP
+  version rather than the newest Kotlin release — the Compose compiler plugin must match it.
+- **The FFmpeg AAR is committed** under `bin/`, deliberately. It is not on any Maven repo
+  (ffmpeg-kit was archived and delisted). Rebuilding per CI run made red builds ambiguous: broken
+  code, or a cross-compile that hiccuped? `bin/README.md` has provenance and how to regenerate it.
+- **Anything touching Media3 carries `@UnstableApi`** rather than swallowing the marker with
+  `@OptIn`. Android lint's `UnsafeOptInUsageError` catches a missed one.
+- **Release builds ship both ABIs.** `-PabiFilters` is a test-run override only; `build.yml`
+  verifies the released APK carries every ABI and that all native libraries are 16 KB aligned.
@@ -80,6 +80,24 @@ android {
        warningsAsErrors = true
        // Already the default. Stated so a later edit cannot turn the gate off by accident.
        abortOnError = true
+
+        // Dependency-freshness nags. These do not describe this code: they go red the day
+        // someone else publishes a release, which would turn a PR red for a reason its
+        // author cannot see in their own diff and cannot fix by changing anything they
+        // wrote. They also want the network at lint time. Upgrades are a deliberate act
+        // here -- the Kotlin version in particular is pinned to AGP's bundled KGP and is
+        // NOT free to follow the newest release -- so they are chosen, not nagged for.
+        disable += setOf("AndroidGradlePluginVersion", "NewerVersionAvailable", "GradleDependency")
+
+        // A real suggestion, deliberately not acted on in this commit. hasSpaceFor() reads
+        // File.usableSpace, which under-reports because it ignores cache the system could
+        // reclaim -- so the app can refuse a conversion it actually had room for.
+        // StorageManager.getAllocatableBytes is the better answer, but swapping it in
+        // changes when a job is rejected and can throw IOException, which is a behaviour
+        // change to a safety check and deserves its own commit and its own test rather
+        // than a drive-by in a tooling change. `informational` keeps it visible in every
+        // lint report instead of hiding it, while letting the gate pass until then.
+        informational += "UsableSpace"
    }

    packaging {
@@ -115,7 +115,7 @@ object MediaProbe {
                    mime.startsWith("audio/") && audio == null -> audio = shortName(mime)
                }
            }
-            Extracted(video, audio, durationUs / 1000, width, height)
+            Extracted(video, audio, durationUs / US_PER_MS, width, height)
        } catch (e: Exception) {
            Log.i(TAG, "Platform extractor could not read $uri.", e)
            null
@@ -162,7 +162,7 @@ object MediaProbe {
            container = containerFrom(formatName, video?.getCodec()),
            videoCodec = video?.getCodec(),
            audioCodec = audio?.getCodec(),
-            durationMs = info.getDuration()?.toDoubleOrNull()?.times(1000)?.toLong() ?: 0L,
+            durationMs = info.getDuration()?.toDoubleOrNull()?.times(MS_PER_SECOND)?.toLong() ?: 0L,
            width = video?.getWidth()?.toInt() ?: 0,
            height = video?.getHeight()?.toInt() ?: 0,
            isImage = isImageFormat(formatName),
@@ -284,4 +284,10 @@ object MediaProbe {
    }

    private const val TAG = "MediaProbe"
+
+    /** MediaExtractor reports KEY_DURATION in microseconds; InputProbe carries milliseconds. */
+    private const val US_PER_MS = 1000
+
+    /** MediaMetadataRetriever's ffprobe-style duration is in seconds, as a decimal string. */
+    private const val MS_PER_SECOND = 1000
 }
@@ -2,6 +2,7 @@ package org.libremediaconverter.convert

 import android.content.Context
 import android.net.Uri
+import androidx.media3.common.util.UnstableApi
 import org.libremediaconverter.codec.AndroidDeviceCodecs
 import org.libremediaconverter.ffmpeg.FFmpegEngine
 import org.libremediaconverter.model.ConversionRequest
@@ -58,6 +59,7 @@ interface SoftwareTranscoder {
 * with a content URI pointing at a provider that does not exist, or by constructing a
 * worker's input Data by hand rather than through its request() helper.
 */
+@UnstableApi
 object ConversionDependencies {

    @Volatile
@@ -261,9 +261,15 @@ object ContainerCapabilities {
            .filter { it != exclude }
            .distinct()
            .filter { validate(it, probe).isValid }
-            .take(3)
+            .take(MAX_SUGGESTIONS)
    }

+    /**
+     * How many alternatives an [Validation.Invalid] offers. Enough to show a real choice,
+     * few enough that the error stays readable.
+     */
+    private const val MAX_SUGGESTIONS = 3
+
    /** Best valid spec for this container, preserving as much of the request as possible. */
    private fun repair(spec: OutputSpec, probe: InputProbe): OutputSpec? {
        val container = spec.container
@@ -30,8 +30,15 @@ object ConversionForegroundType {
     * `mediaProcessing` constant does not exist to pass in the first place.
     */
    fun current(): Int = when {
-        Build.VERSION.SDK_INT >= 35 -> ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PROCESSING
-        Build.VERSION.SDK_INT >= 34 -> ServiceInfo.FOREGROUND_SERVICE_TYPE_DATA_SYNC
-        else -> 0
+        Build.VERSION.SDK_INT >= Build.VERSION_CODES.VANILLA_ICE_CREAM ->
+            ServiceInfo.FOREGROUND_SERVICE_TYPE_MEDIA_PROCESSING
+
+        Build.VERSION.SDK_INT >= Build.VERSION_CODES.UPSIDE_DOWN_CAKE ->
+            ServiceInfo.FOREGROUND_SERVICE_TYPE_DATA_SYNC
+
+        else -> NO_TYPE_REQUIRED
    }
+
+    /** API 33 wants no type at all, and `ForegroundInfo` reads 0 as exactly that. */
+    private const val NO_TYPE_REQUIRED = 0
 }
@@ -42,7 +42,7 @@ class ConversionNotifications(private val context: Context) {
            // Progress updates far outpace what the UI can use; alerting once keeps
            // the system UI from being hammered.
            .setOnlyAlertOnce(true)
-            .setProgress(100, percent, indeterminate)
+            .setProgress(PERCENT_MAX, percent, indeterminate)
            .addAction(
                android.R.drawable.ic_menu_close_clear_cancel,
                context.getString(R.string.action_cancel),
@@ -64,5 +64,8 @@ class ConversionNotifications(private val context: Context) {
    companion object {
        const val CHANNEL_ID = "conversions"
        private const val TAG = "ConversionNotifications"
+
+        /** `setProgress` takes a max and a current; progress is reported as a percentage. */
+        private const val PERCENT_MAX = 100
    }
 }
@@ -16,6 +16,19 @@ complexity:
  CyclomaticComplexMethod:
    # Branchy layout code (when/if inside a UI tree) isn't algorithmic complexity.
    ignoreAnnotated: ['Composable']
+    # A flat `when` used as a lookup table scores one point per entry, so MediaProbe's
+    # demuxer-name -> Container map reads as complexity 21 while having no nesting, no
+    # state and nothing to follow. That is the metric measuring table size. A `when` with
+    # real logic in its branches still counts.
+    ignoreSingleWhenExpression: true
+    ignoreSimpleWhenEntries: true
+    # Same reason the model package is excluded from ReturnCount below: there, one branch
+    # is one documented outcome. ConversionRouter.route scores 17 because the app can give
+    # 17 distinct answers to "which engine, and why", not because it is hard to follow --
+    # it is a flat guard chain with a comment per rule. Only this metric and ReturnCount
+    # are relaxed for that package; LongMethod, NestedBlockDepth, ComplexCondition and the
+    # rest still apply there.
+    excludes: ['**/model/**']
  TooManyFunctions:
    # Screen files group many small @Composable helpers next to their screen, and the codec /
    # container matrix files are intentionally operation-rich cohesive APIs. detekt's default of
@@ -24,6 +37,10 @@ complexity:
    allowedFunctionsPerFile: 40
    allowedFunctionsPerClass: 40
    allowedFunctionsPerInterface: 40
+    # ContainerCapabilities is a single object holding the container x codec matrix and the
+    # queries over it. Splitting it to satisfy a count would scatter one table across files.
+    allowedFunctionsPerObject: 40
+    allowedFunctionsPerEnum: 40

 naming:
  FunctionNaming:
@@ -36,9 +53,34 @@ style:
    ignoreAnnotated: ['Composable']
    ignorePropertyDeclaration: true
    ignoreNamedArgument: true
+    # SI thresholds in the byte formatter. Each literal sits on the same line as the unit
+    # string it belongs to -- `bytes >= 1_000_000 -> "%.1f MB"` -- so a BYTES_PER_MB pair
+    # (Long and Double, since the compare and the divide need different types) would add
+    # six names and say nothing the line does not already say. Counts, limits and tuning
+    # values still flag; these are unit boundaries.
+    ignoreNumbers:
+      - '-1'
+      - '0'
+      - '1'
+      - '2'
+      - '1e3'
+      - '1e6'
+      - '1e9'
+      - '1_000'
+      - '1_000_000'
+      - '1_000_000_000'
  ReturnCount:
    # Allow guard-clause-style early returns; detekt's default of 2 is overly strict.
    max: 4
+    excludeGuardClauses: true
+    # The model package IS the decision layer: ConversionRouter picks an engine,
+    # ContainerCapabilities validates a container x codec pair, the planners pick a
+    # strategy. Each `return` there is one specific, user-visible reason, and the count
+    # equals the number of reasons the app can give -- ConversionRouter.route even
+    # documents that their ORDER is what decides which message the user sees. Collapsing
+    # them into one exit would bury that. Everywhere else -- UI, workers, engines -- the
+    # limit still applies.
+    excludes: ['**/model/**']
  LoopWithTooManyJumpStatements:
    # Clear early-continue / early-return loops read fine; the default of 1 is strict.
    maxJumpCount: 3
@@ -47,3 +89,16 @@ style:
    # preconditions.
    excludeGuardClauses: true
    max: 3
+
+exceptions:
+  TooGenericExceptionCaught:
+    # The engine boundaries catch broadly on purpose. MediaProbe drives the platform
+    # extractor and MediaMetadataRetriever over arbitrary user files; ConversionWorker and
+    # ConcatWorker wrap Media3 and FFmpeg. All three sit in front of native code that
+    # reports a malformed file as anything from IllegalArgumentException to
+    # IllegalStateException to a bare RuntimeException, and the list is not documented.
+    # Enumerating it would mean guessing, and a guess that is wrong crashes the app on a
+    # file it could have simply reported as unreadable. Every one of these catches logs
+    # and handles -- falls back to FFmpeg, or fails the job with a reason -- and the
+    # SwallowedException rule stays active to keep it that way.
+    active: false