From dce516224cd467f58f946b46be4e99f1103cbfe9 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 25 Aug 2026 21:32:55 -0500 Subject: [PATCH 1/3] Offer a fix that works when the file has no video to copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refusing "copy the video" for a file that has none built its one suggestion by hand — drop the video track and leave everything else alone. That is valid only when the audio axis already happened to be fine. For any audio the target cannot carry (Vorbis or PCM into MP4, MP3 into WebM) the offer is refused in the next breath, so the Advanced picker showed a one-tap fix leading straight to a second error. Nothing unsafe shipped — ConversionWorker re-validates — but it is a dead end, and it contradicted the promise Validation.Invalid makes in its own KDoc. Route it through the shared repair-and-filter path instead, as every other branch does. Excluding what the *user* asked for rather than the already-repaired spec is what keeps the case that worked working: an MP3 into MP4 still gets its copy offered, because the repair of a copyable track is that same copy. Only a branch that builds its own list can break that promise at all, since suggestions() ends by filtering on validate().isValid. The property test now covers both of them — this one and the image output — rather than reaching them by luck, which is how a dead-end chip survived two earlier widenings of it. Its failures name the probe too: three rows share a spec and differ only in the input. Closes #114 Co-Authored-By: Claude Opus 5 (1M context) --- .../model/ContainerCapabilities.kt | 7 ++- .../model/ContainerCapabilitiesTest.kt | 60 ++++++++++++++++++- 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt b/app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt index 294ee7d..aa19b89 100644 --- a/app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt +++ b/app/src/main/java/org/libremediaconverter/model/ContainerCapabilities.kt @@ -178,7 +178,12 @@ object ContainerCapabilities { if (!probe.hasVideo) { return Validation.Invalid( "This file has no video track to copy.", - listOf(spec.copy(videoCodec = VideoCodec.NONE)), + // Dropping the video is the right shape of answer, but it is only half of one: + // `spec.copy(videoCodec = NONE)` is valid exactly when the audio axis already + // happened to be fine, and refused otherwise — a Vorbis or PCM source into MP4, + // an MP3 into WebM. Handing it to the shared path repairs both axes and drops + // anything that still fails, so the chip cannot lead to a second error. + suggestions(spec.copy(videoCodec = VideoCodec.NONE), probe, exclude = spec), ) } val source = CodecNames.videoFromName(probe.videoCodec) diff --git a/app/src/test/java/org/libremediaconverter/model/ContainerCapabilitiesTest.kt b/app/src/test/java/org/libremediaconverter/model/ContainerCapabilitiesTest.kt index 9315ba0..daae30b 100644 --- a/app/src/test/java/org/libremediaconverter/model/ContainerCapabilitiesTest.kt +++ b/app/src/test/java/org/libremediaconverter/model/ContainerCapabilitiesTest.kt @@ -11,6 +11,11 @@ import org.junit.Test * `OutputFormat` used to be twelve hand-picked triples, and its KDoc defended that on the grounds * that a closed set was what made routing decidable. Opening it up moves that burden here, so this * is where decidability now has to be proven. + * + * That includes what a refusal offers instead. `Validation.Invalid` promises every suggestion is + * itself valid and names this class as the proof, so a branch that assembles its own suggestion + * list rather than going through `suggestions()` is only checked here if some row happens to reach + * it — which is how a dead-end chip survived two widenings of that table. */ class ContainerCapabilitiesTest { @@ -35,6 +40,29 @@ class ContainerCapabilitiesTest { container = Container.MP3, ) + /** + * An audio-only input carrying a codec MP4 has no place for at all. + * + * Vorbis lives in Ogg and Matroska; MP4 carries AAC, MP3, Opus and FLAC. That gap is what turns + * a suggestion which merely drops the video track into a second refusal. + */ + private val vorbisSource = InputProbe( + videoCodec = null, + audioCodec = "vorbis", + hasVideo = false, + kind = InputKind.AUDIO_ONLY, + container = Container.OGG, + ) + + /** The same shape, for the other codec MP4 refuses. One case is a coincidence; two is the rule. */ + private val pcmSource = InputProbe( + videoCodec = null, + audioCodec = "pcm_s16le", + hasVideo = false, + kind = InputKind.AUDIO_ONLY, + container = Container.WAV, + ) + // --- copy and encode are different questions ---------------------------- /** @@ -97,7 +125,17 @@ class ContainerCapabilitiesTest { } } - /** A suggestion that is itself invalid is worse than no suggestion. */ + /** + * A suggestion that is itself invalid is worse than no suggestion. + * + * Only a branch that assembles its own suggestion list can break that promise: [suggestions] + * ends by filtering on `validate(...).isValid`, so everything routed through it is valid by + * construction. Those branches are what this table has to cover — the image output, and copy + * the video from a file that has none, which built its list by hand and came back refused for + * a Vorbis or PCM source into MP4 and an MP3 into WebM. The Advanced picker showed a one-tap + * fix that led straight to a second error, through two widenings of this table that never + * reached the branch. + */ @Test fun `every suggestion is itself valid`() { val cases = listOf( @@ -109,15 +147,31 @@ class ContainerCapabilitiesTest { OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.NONE) to mp3Source, OutputSpec(Container.MP4, VideoCodec.NONE, AudioCodec.NONE) to mp3Source, OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.AAC) to mp3Source, + // Copy-the-video-from-a-file-with-no-video, the last branch that built its offer by + // hand. It escaped the five rows above because `spec.copy(videoCodec = NONE)` is valid + // exactly when the audio axis happens to be fine — true for the AAC and MP3 sources + // used there, false for any audio the target container cannot carry. + OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.COPY) to vorbisSource, + OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.COPY) to pcmSource, + OutputSpec(Container.WEBM, VideoCodec.COPY, AudioCodec.COPY) to mp3Source, + // The same branch with audio the container *can* hold, which is the half that already + // worked and must keep working: the repair here is a copy, so the offer is the very + // spec the caller handed to `suggestions`. It survives only because the exclusion is + // against what the user asked for rather than against the repair. + OutputSpec(Container.MP4, VideoCodec.COPY, AudioCodec.COPY) to mp3Source, + // The one branch that still builds its list by hand, so that it is asserted rather + // than merely reasoned about: an image container takes `None + None` and nothing else, + // which makes its single offer valid by construction. + OutputSpec(Container.GIF, VideoCodec.H264, AudioCodec.AAC) to h264Source, ) cases.forEach { (spec, probe) -> val invalid = ContainerCapabilities.validate(spec, probe) as? Validation.Invalid ?: throw AssertionError("expected $spec to be rejected") - assertTrue("no alternatives offered for $spec", invalid.suggestions.isNotEmpty()) + assertTrue("no alternatives offered for $spec on $probe", invalid.suggestions.isNotEmpty()) invalid.suggestions.forEach { suggestion -> assertTrue( - "suggested $suggestion for $spec is itself invalid", + "suggested $suggestion for $spec on $probe is itself invalid", ContainerCapabilities.validate(suggestion, probe).isValid, ) } From 25aac95db9ab7c042f51be039322882a0eda260a Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Tue, 25 Aug 2026 21:57:32 -0500 Subject: [PATCH 2/3] Say in the run-shape table when the wedge timeout was what killed the leg The report added by #111 runs on every path out of e2e-run.sh, including the wedge, and until now it answered a question it had not been asked. On job 98035980326 -- API 34, a docs-only PR -- it printed `received: 59` and `completed cleanly: yes` six seconds before `##[warning] ... WEDGED`, for a leg the WEDGE_TIMEOUT had killed 22 minutes in. `completed cleanly` means only "instrumentation was not aborted", which was true; a reader scanning the table had to notice a separate warning line to learn the leg had died. The wedge cannot be read out of the log, which is why it is passed in: a wedge is gradle never returning, so gradle printed no verdict, no truncation line and no INSTRUMENTATION_ABORTED, and the log it leaves is the log of a run that just stops. Only e2e-run.sh saw `timeout` exit 124. It now derives that fact once and tells the report as E2E_WEDGED_AFTER, and reuses the same variable for capture_wedge so the two cannot drift. The table gains a `wedged:` row above `completed cleanly`, and `completed cleanly` flips to no -- but only where it would have said yes. An abort already says no and names the abort, which the wedge row does not, and a run that left no evidence still says unknown; a wedge on top of either prints both facts. `received`'s source line told the same lie in the same table -- "the run was not truncated, so every expected test reported" is only "gradle never got as far as saying so" when the leg was killed -- so it is qualified on that path. The number itself is unchanged, and so is `failed: unknown`: gradle printed no summary line, so that count genuinely is not knowable. Nothing here decides anything. No exit status, no pass/fail rule, no baseline comparison and no `::notice::` behaviour changes; the leg already failed correctly and still does. Verified against captured CI output rather than a live emulator, as #111 was and for the same reason -- this host cannot run API 37 and cannot wedge on demand. Four real logs (the wedged leg, a green API 34 leg, a failing gating leg, and an advisory leg with its baseline deviation) through both versions of the script, in both env states, comparing stdout and the job summary: only the wedged run with the signal set differs, byte for byte. Co-Authored-By: Claude Opus 5 (1M context) --- .github/scripts/e2e-report-shape.sh | 36 +++++++++++++++++++++++++++++ .github/scripts/e2e-run.sh | 20 +++++++++++++--- CLAUDE.md | 7 ++++++ 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/.github/scripts/e2e-report-shape.sh b/.github/scripts/e2e-report-shape.sh index 4521738..a835674 100755 --- a/.github/scripts/e2e-report-shape.sh +++ b/.github/scripts/e2e-report-shape.sh @@ -30,16 +30,31 @@ # # Usage: # e2e-report-shape.sh