Merge branch 'main' into test/join-failure-message-on-device

Record what working the e2e tickets found

CLAUDE.md

@@ -76,11 +76,11 @@ days. Read it as the current answer, and see the git history if you need the old
   `angle_indirect` and `swangle_indirect` all boot, while `auto`, `off`, `guest` and
   `swiftshader_indirect` do not. `docs/local-emulator.md` has the evidence and the per-API renderer
   table.
-- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Three** of the 60 instrumented
+- **CI runs API 37, and it gates.** The matrix is 33/34/35/36/37. **Three** of the 61 instrumented
   tests cannot pass on that image, for two unrelated reasons: two Media3 hardware transcodes fail
   inside the emulator's own `c2.goldfish.h264.decoder`, and one SAF test takes the framework down
   when it rotates the display. All three carry `@FailsOnEmulatorApi37` and run in a separate
-  `continue-on-error` job; the gating leg runs the other 57.
+  `continue-on-error` job; the gating leg runs the other 58.
 
   That job is still called `E2E API 37 Media3 hardware transcode (advisory)`, which no longer
   describes everything in it. The name is kept deliberately — it is not a required context and
@@ -332,9 +332,24 @@ install for code that can never run — and on API 37 the full APK does not fit
   without the pin they would fail loudly; `HardwareFallbackTest`'s are about the *output*, so it
   passes quietly. **Prefer asserting the path over asserting the artefact** where the two differ.
 
-  The read was a triage, not a test push, and five of its six findings are prose rather than code —
+  The read was a triage, not a test push, and six of its seven findings are prose rather than code —
   the suite itself is in good shape. What had drifted is its self-description.
 
+  **Working the tickets then found the thing the read could not: one production defect.** #238 —
+  joining files picked through the system picker failed outright on the stream-copy path. The
+  concat demuxer whitelists protocols separately from `-safe 0`, and `ffkitsaf` was not on the
+  list; only `STREAM_COPY` feeds it a list file, and every existing join test passed
+  `Uri.fromFile`, so **the one broken combination was the only one a user could reach**. Not a
+  missed line and not an unasserted value — two covered things no test put together, which is the
+  gap shape a coverage number is worst at.
+
+  **E7 is the other reusable result**, because it re-scoped its own ticket. A real
+  `DocumentsProvider` cannot be reached without the picker: an unprotected one is refused at
+  install, instrumentation runs in the app's uid so the test APK's identity is no help, and shell
+  identity is denied too — each denial naming `ACTION_OPEN_DOCUMENT`. So #226 has no cheap headless
+  half. But the *input* bridge needs no documents provider at all, which is what kept #225 headless
+  and is how #238 surfaced.
+
 - **Testable code is not done until it is tested.** If a piece is unit testable, it gets unit
   tests before it counts as done. If it is e2e testable, it gets e2e tests. Both clauses apply —
   a change that is both needs both.

app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt

@@ -25,6 +25,7 @@ import org.junit.runner.RunWith
 import org.libremediaconverter.convert.MediaProbe
 import org.libremediaconverter.convert.StagingNames
 import org.libremediaconverter.ffmpeg.ConcatEngine
+import org.libremediaconverter.ffmpeg.FFmpegEngine
 import org.libremediaconverter.model.ConcatStrategy
 import org.libremediaconverter.work.ConcatWorker
 import java.io.File
@@ -236,6 +237,55 @@ class ConcatEngineTest {
         )
     }
 
+    /**
+     * A failed join tells the user the return code and what FFmpeg said.
+     *
+     * **This is the device half of #203/#217**, whose PR closed by noting the join legs had not
+     * been run. Running them would not have answered it: nothing on either source set drove a real
+     * join *failure*, so the unified message was asserted only against values a JVM test hands to
+     * `sessionOutcome` directly.
+     *
+     * What is device-only here is that the three reads behind that message work against a real
+     * native session at all — `getReturnCode`, `getFailStackTrace` and `getAllLogsAsString`. If
+     * the log tail came back null or empty on a device, the user would get `Joining failed (1): `
+     * with nothing after the colon and every JVM test would still pass.
+     *
+     * **What this deliberately does not pin is the preference between the two detail sources.** On
+     * an ordinary non-zero return code FFmpegKit reports no fail stack trace, so the stack-trace-
+     * first rule and the log-tail-first rule produce the same text and no assertion here can tell
+     * them apart. That ordering is [SessionOutcomeTest][org.libremediaconverter.ffmpeg.SessionOutcomeTest]'s
+     * job, where both sources can be non-blank at once. Asserting it here would be a test whose
+     * KDoc claims more than it checks — the `probeForConcat` mistake wave 3 caught.
+     *
+     * The failure is forced with an input that does not exist, which the concat demuxer rejects
+     * the same way on every FFmpeg build, rather than with malformed media whose handling varies.
+     */
+    @Test
+    fun aFailedJoinReportsTheReturnCodeAndWhatFFmpegSaid(): Unit = runBlocking {
+        val missing = File(context.cacheDir, "no_such_clip.mp4").also { it.delete() }
+        val out = output("joined_failure.mp4")
+
+        val failure = runCatching {
+            engine.join(listOf(Uri.fromFile(clipA), Uri.fromFile(missing)), out)
+        }.exceptionOrNull()
+
+        assertTrue(
+            "a join over a missing input must fail, got $failure",
+            failure is FFmpegEngine.FFmpegException,
+        )
+        val message = failure?.message.orEmpty()
+        assertTrue(
+            "the message must name the operation and carry the return code, was: '$message'",
+            message.startsWith("Joining failed ("),
+        )
+        // The half a JVM test cannot reach: a real session actually produced detail to show.
+        val detail = message.substringAfter("): ", "")
+        assertTrue(
+            "the message stopped at the return code and told the user nothing, was: '$message'",
+            detail.isNotBlank(),
+        )
+    }
+
     @Test
     fun theListFileIsCleanedUpAfterJoining(): Unit = runBlocking {
         val out = output("joined_cleanup.mp4")

docs/e2e-read-findings.md

@@ -1,6 +1,6 @@
 # E2E-read findings
 
-**Status:** six findings, none fixed, none urgent — **plus one confirmed vacuous test, which is a
+**Status:** seven findings; E4 fixed, the rest standing, none urgent — **plus one confirmed vacuous test, which is a
 ticket rather than an entry here** (see [Not covered here](#not-covered-here)). `E1`–`E6` came from
 the 2026-09-05 read of the instrumented suite. Every entry here is a *test-suite* observation —
 something a new test would not fix, because the test already exists and the problem is what it
@@ -242,6 +242,49 @@ visible skip or a red test, and that decision is the ticket's.
 
 ---
 
+## E7 — a real `DocumentsProvider` cannot be reached without the picker, so there is no cheap SAF test
+
+**Severity: n/a · Confirmed by measurement · this is a platform rule, not a gap**
+
+Added 2026-09-06, from doing #225 and #226 rather than from reading.
+
+`OutputPublisher.publish`'s destination side is asserted only against Robolectric fakes —
+`FakeSafProvider`, registered with `asDocumentsProvider = true`, which is the flag that *makes*
+`DocumentsContract.isDocumentUri` answer true. #226 split that into a cheap headless half (drive a
+real `DocumentsProvider` directly) and an expensive picker-driven half.
+
+**The cheap half does not exist.** Three approaches, all measured on an API 34 emulator:
+
+| approach | result |
+|---|---|
+| a second `DOCUMENTS_PROVIDER` declared **without** `MANAGE_DOCUMENTS` | refused at install: `SecurityException: Provider must be protected by MANAGE_DOCUMENTS` |
+| create the document as the **test APK**, which owns the provider | denied — instrumentation runs *in the target app's process*, so it carries the app's uid whatever `Context` is asked |
+| `uiAutomation.adoptShellPermissionIdentity(MANAGE_DOCUMENTS)` | denied identically |
+
+The denial names the only way in:
+
+> `Permission Denial: opening provider …FixtureDocumentsProvider from
+> ProcessRecord{… org.libremediaconverter/u0a192} requires that you obtain access using
+> ACTION_OPEN_DOCUMENT or related APIs`
+
+And the intent filter is not optional: without it `isDocumentUri` returns false, which is exactly
+the branch guarding `deletePartialOutput` — so a provider without the filter tests nothing the
+ticket is about.
+
+**So any test of `publish` against a real `DocumentsProvider` must drive DocumentsUI**, and pays
+#190's flake tax. The work is one item at that cost, not two, and #226 was updated to say so.
+
+### What this does *not* block, which is the useful half
+
+`FFmpegKitConfig.getSafParameterForRead` — the bridge on every real conversion and join — needs no
+documents provider. It opens a descriptor through the resolver, so **any readable `content://` URI
+exercises it**, and an ordinary `ContentProvider` may be exported without a permission. That is what
+`FixtureContentProvider` is, and it made #225 headless.
+
+**That distinction was worth the trouble**: the first test ever to hand the join path a real
+`content://` input found #238, a defect that broke joining for every user who picks matched files.
+The expensive gate protects the *destination* side; the *input* side never needed it.
+
 ## Summary
 
 | ID | Finding | Severity | Evidence | Action |
@@ -249,11 +292,12 @@ visible skip or a red test, and that decision is the ticket's.
 | E1 | `RemuxTest`'s KDoc claims engine assertions three of its tests correctly omit | low | confirmed by inspection; traced through `MEDIA3_CONTAINERS` | **one line of KDoc** — the tests are right |
 | E2 | Three of the 60 instrumented tests assert nothing; two never run | n/a | confirmed by inspection; `docs/local-emulator.md:305` | **no action** — deliberate; but 60 ≠ 60 |
 | E3 | `…AndReportsProgress` does not assert progress fired | low | confirmed by inspection; reason inline | **no action** — the name overstates, the KDoc corrects it |
-| E4 | The API 37 marker's KDoc says "two"; three tests carry it | low | confirmed by inspection; baseline const says 3 | **fix the sentence** |
+| E4 | The API 37 marker's KDoc says "two"; three tests carry it | low | confirmed by inspection; baseline const says 3 | **fixed** in #243 — it names the constant now |
 | E5 | `coverage-read-findings.md` F7's "uncovered" half is stale | low | confirmed by inspection; `RemuxTest.kt:111` drives it | **amend F7** — "device-only" stands, "uncovered" does not |
 | E6 | The device-capability assertion asks the class under test what to expect | low | confirmed by inspection; no third oracle exists on a device | **no action** — read with **#223** |
+| E7 | A real `DocumentsProvider` is unreachable without the picker, so #226 has no cheap half | n/a | measured three ways on API 34; each denial names `ACTION_OPEN_DOCUMENT` | **no action** — it re-scoped #226 |
 
-**Five of the six are prose, not code**, and that is the shape of this read. The instrumented suite
+**Six of the seven are prose, not code**, and that is the shape of this read. The instrumented suite
 is in good condition: 57 of its 60 tests bite, the fixtures are committed with their generation
 recipes, and the one class that asserts nothing says so in its first line. What this read found is
 that **the suite's self-description has drifted from the suite** in five small places and one large
@@ -312,14 +356,25 @@ decision, not a detail — see **E6** for why no third option exists — and **#
 
 | # | Gap |
 |---|---|
-| **#223** | `HardwareFallbackTest` never attempts the hardware path on any emulator leg |
+| # | Gap | Outcome |
-| **#224** | Cancelling a *running* native session, in any of the three engines |
+|---|---|---|
-| **#225** | No `content://` input has reached a *successful* conversion — the ffkitsaf bridge |
+| **#223** | `HardwareFallbackTest` never attempts the hardware path on any emulator leg | closed — it skips instead of passing vacuously |
-| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider`, and the SAF premise it rests on |
+| **#224** | Cancelling a *running* native session, in any of the three engines | closed — all three engines |
-| **#227** | The notification's Cancel action has never been fired |
+| **#225** | No `content://` input has reached a *successful* conversion — the ffkitsaf bridge | closed, and it found **#238** |
-| **#228** | `encodesFlacLosslessAudio` and `encodesOpus` pass on any non-empty file |
+| **#226** | `OutputPublisher.publish` against a real `DocumentsProvider` | **open** — re-scoped by E7; one picker-driven item, not two |
-| **#229** | FFmpeg's progress percentage is computed everywhere and asserted nowhere |
+| **#227** | The notification's Cancel action has never been fired | closed |
-| **#230** | *(spike)* whether a running conversion's process can be killed under instrumentation |
+| **#228** | `encodesFlacLosslessAudio` and `encodesOpus` pass on any non-empty file | closed |
+| **#229** | FFmpeg's progress percentage is computed everywhere and asserted nowhere | closed |
+| **#230** | *(spike)* whether a running conversion's process can be killed | closed — it cannot; the runner shares the app's process |
+
+**The read's own result, once the tickets were worked: one production defect.** #238 — joining files
+picked through the system picker failed outright on the stream-copy path, because the concat demuxer
+whitelists protocols separately from `-safe 0` and `ffkitsaf` was not on the list. Only `STREAM_COPY`
+feeds the demuxer a list file, and every existing join test passed `Uri.fromFile`, so the one broken
+combination was the only one a user could reach.
+
+That is the argument for this kind of read in one line: the gap was not a missed line or an
+unasserted value, it was **a combination of two covered things that no test put together**.
 
 **Nothing here was filed as a coverage delta.** Each names the mutation that has to go red, which is
 the acceptance criterion wave 4 established and which caught two vacuous tests in that wave before