test/publish-to-a-real-saf-destination
40
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  JMR-devandClaude Opus 5
|
e7c3e5688f |
Stop the match line claiming a failure count nobody measured
This PR's own advisory leg caught it. With five markers and a truncated run it printed failed: 4 ... baseline: matches (5 expected, 5 failed) three lines apart. The match line has always printed the baseline twice, which was true while `failed` had to equal it to get there -- and the previous commit removed that requirement for truncated runs without noticing this line depended on it. So the truncated spelling says what happened: `matches (5 expected; 4 of 5 failed, on a run the abort truncated -- not compared)`. Pinned by a third case beside the two from that commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
7fd95ddede |
Merge main, and re-derive every count it moved
main landed 25 commits while this branch was open, including a third @FailsOnEmulatorApi37 on Media3EngineTest.cancellingARunningExportStopsIt and a batch of new instrumented tests. Every number this branch touches moved with them. Re-derived rather than adjusted, and cross-checked against run 34020234606: the API 34 leg (no filter) reports 68 tests and the API 37 gating leg 64, which is 68 minus main's four markers. With the picker test marked that is five markers, baseline 5, and 63 on the gating leg. The conflict in FailsOnEmulatorApi37.kt is resolved main's way: it had replaced the hardcoded "grows by two" with a reference to the constant, which is the same drift this file exists to prevent and a better fix than the number I put there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
557b3edab4 |
Compare the advisory failure count only on a run that finished
The verification dispatch of the reworked harness (34011072884) came back 4 expected / 3 received / 3 failed, where the one before it (34008889182) had been 4/4/4 on the identical configuration. Nothing about the test list changed between them: the abort landed one test earlier and the picker test never started. The baseline check would have called that "one now passes", which is the wrong reading and the kind of notice #120 is about -- a deviation that is wrong often enough to teach everyone to skim past deviation notices. So `failed` is compared only when `completed cleanly` is yes, and `expected` is compared always, because `Starting N tests` is printed before anything can abort and is what actually answers "is the marked set the size the baseline says". Two cases in e2e-report-shape-test.sh, as a pair: a truncated run short by one is not a deviation, and a CLEAN run short by one still is -- so the first cannot have bought its quiet by disabling the check. This is a consequence of adding the fourth marker rather than a pre-existing bug worth its own ticket: with three, the advisory leg had been completing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
97558c259f |
The root fix was wrong, and this is what it found: the disable does nothing
Three commits back I gave `disable_region_sampling` the `adb root` it needed, on the strength of `Must be root` appearing in every API 37 leg's log. That part was right and the conclusion drawn from it was not. api37-debug run 34010167885, with the restart finally real: pm attempt 1: Package com.android.systemui new state: disabled-user restarting the framework adbd is running as root system_server down after 2 s NOT DISABLED after the restart -- the package state did not survive three rounds of it, `final state: SystemUI STILL ENABLED`, and the leg reported `expected: 0, received: 0`. Making the restart work cost the leg every test it had. Bisected locally on android-37.0: a `stop` 2 s after `pm disable-user` kills system_server before PackageManager flushes its delayed write, and a 15 s pause makes the state survive. That repairs the wrong thing. With the package verified disabled before AND after a clean restart, `com.android.systemui` comes up 3 s after `system_server` regardless -- and CI's own logcat says the same with no restart at all: run 34006456986 verifies the package disabled at 02:29:33 and has SystemUI pid 4275 alive from 02:28:52 for the whole run. So `pm disable-user` does not stop SystemUI starting on this image, with or without a restart, and the restart is removed from all three copies rather than repaired. What is kept is the 45-second window with no new aborts, which is what was always doing the work: the boot aborts land at 02:28:18 and 02:28:43 and the wait is what puts instrumentation at 02:32:42, after them rather than inside one. `pm disable-user` is kept too, because every green leg and every number quoted about this row was measured with it applied. The prose the earlier commits got wrong is corrected in place, and one of the corrections is good news: status_check.yml's caveat that this row runs a configuration no other leg or Pixel run uses, so nothing depending on system UI may trust it, describes a state that has never existed. The row is more comparable to API 33-36 than it has been claiming, not less. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
393b931fff |
Give api37-debug's own SystemUI disable the same root, and say why there are two
The debug workflow's header says it does not fork e2e-run.sh, and it does not -- but it drives the SystemUI disable from its own probe step, so `disable_system_ui` can be turned off for a dispatch. That is a second copy of the same logic, and run 34008889182 showed it carrying the same defect the real leg had: `Must be root` twice, and `system_server down after 40 s` printed for a stop that did nothing. Same fix, and a header note so the next person changing one knows to change both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
d759ef32f1 |
Take the picker test off the API 37 gating leg, and give the SystemUI disable root
The gating `E2E API 37` leg failed three of the last ten status_check runs. Four runs were read logcat-first -- 34006456986, 34001744574, 34001377499 and the green 34002313300 -- and each carries exactly two `hasReadColorBufferDma` aborts before the suite (surfaceflinger, during boot and the SystemUI disable) and exactly one during it: `system_server`, thread `TaskSnapshotPer`, always inside `pickingAFileThroughTheSystemPickerFillsInTheFileCard`'s window. Nothing else in the gating set reaches the mapper. So that test kills the framework on this image whether it passes or not, and whether the leg goes red is luck: 34001377499 passed it and lost the leg anyway with `failed: 0`, 34002313300 passed it 0.6 s after the abort and went green. That is #108. The test now carries `@FailsOnEmulatorApi37` and the baseline goes 3 -> 4; the marker's own wording widens from "does not pass on this image" to "cannot be run on this image", because this carrier passes about half the time. `docs/api-37-emulator-crash.md` had counted those aborts on 2026-08-24, put them in its table, and then read the pass/fail column alone. The correction is recorded beside the original rather than replacing it. Probed on the same image and recorded there too: there is no shell knob for task snapshots -- not in `getprop`, `settings`, `device_config` or `cmd window` -- so the marker is the available answer rather than the lazy one. Two separate defects came out of the same logcats. `disable_region_sampling` has never restarted the framework on CI. `adb shell stop` and `start` are root-only and every API 37 leg has printed `Must be root` for both, so SystemUI stayed up for the whole run -- visible directly as `WindowManagerShell ... app=com.android.systemui` minutes after "final state: SystemUI disabled". Both waits also printed their own exhaustion as an elapsed time, so "system_server down after ~40 s" is what a stop that did nothing looks like. Measured on the local android-37.0 AVD, same fingerprint as CI: `adb root` makes `stop` return 0 with `pidof system_server` empty. Root is dropped again before Gradle runs, and both waits now say whether they observed anything. And when the picker test does fail, the abort is the coda rather than the cause: `InputDispatcher: No new touched window at (539.0, 525.0)` is in both reds and absent from the green, so the tap on the root is discarded, the picker is never navigated, and all four back presses land on an activity WindowManager says has not added a window yet. `forceStopThePicker` goes around input entirely so `pickTheFixture`'s whole-picker retry -- which exists for exactly this -- becomes reachable. That one is a fix on every API level, not just 37. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
0f41bc3f6b |
Count the annotation, not the comment saying a test does not carry it
The advisory baseline check has announced a deviation on every PR since #113: the tree "carries 4 tests marked @FailsOnEmulatorApi37" where it carries three and FAILS_ON_EMULATOR_API37_BASELINE says three. The fourth is a KDoc in Media3EngineTest saying the opposite -- "Deliberately not `@FailsOnEmulatorApi37`: nothing here decodes or encodes" -- which the old matcher counted because it looked for the string anywhere on any line. Neither ingredient was wrong on its own, and the number is not the real damage. #83 added this check so that a new failure joining the known ones could not be invisible; a notice that is wrong every single time teaches everyone to skim past deviation notices, which is precisely the signal it was built to create. Editing the baseline to 4 would have silenced it by breaking it -- the check would then have been wrong the moment someone added or removed a real marker. Anchor the pattern at line start and require whitespace or end-of-line after the name. The second half is the part that is easy to get wrong: "only the annotation on a line of its own" also stops counting `@FailsOnEmulatorApi37 @Test`, which is legal Kotlin, and undercounting is the dangerous direction -- it hides a genuine new marker, the one thing this exists to catch. Measured against a fixture carrying every shape at once: the old matcher 5, own-line-only 2, this one 3; on the real tree 4 / 3 / 3, so the baseline is untouched. `grep -v import` goes too, since `^[[:space:]]*@` cannot match an import. The check is a pure function of the working tree, so the fixture is committed and e2e-report-shape-test.sh runs the real report against it -- inside a throwaway repo root, which the script finds from BASH_SOURCE, so no knob had to be added that could point the live count somewhere else. The fixture sits under .github/, where Gradle does not compile it and :app's ktlint and detekt do not see it; running the report against the real root with it committed still reports 3. Every other path through the report is byte-identical to the previous version on both stdout and the job summary -- passing, failing, wedged, no-run, and advisory-with-an-unreadable-baseline all diff empty -- and the two advisory legs differ only by the false line disappearing. No job's status or pass/fail rules change; the advisory leg stays continue-on-error and stays red by design. The test is deliberately not wired into CI: adding a step to Static analysis would add a new way for a gating job to go red, which #120 ruled out. shellcheck still covers the file, since that step reads `git ls-files '*.sh'`. Closes #120 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
25aac95db9 |
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) <noreply@anthropic.com> |
||
|
JMR-dev
|
994ea8a3dd |
Say in the step log that the summary was written, since nothing else can
The job summary is the deliverable #83 asked for -- "readable without opening a log" -- and GitHub exposes no API that reads a job summary back: the check-run output for the advisory job returns summary: null, so a write that silently did not happen would be invisible to everything except a human on the run page. The step log can be read, so it now carries one line saying which of the two happened, including the case where GITHUB_STEP_SUMMARY is unset entirely, which is what running the script by hand looks like. |
||
|
JMR-dev
|
3e9528454c |
Announce a baseline it cannot read, rather than falling quiet
"A comparison was asked for" and "a number was found to compare against" were one variable, and collapsing them put the report one refactor away from being the thing #83 filed. The sed that reads FAILS_ON_EMULATOR_API37_BASELINE is anchored at the line start, so indenting the const into an object -- or renaming it, or moving it -- empties it, and the old code then skipped the whole comparison while the table kept printing exactly as before. Silent, and indistinguishable from a run that matched. Now an unreadable baseline is itself a deviation, with the notice naming the const so the fix is obvious. Verified against the real captured log of run 32865281555 three ways: baseline file absent, const indented into an object, and the committed file unchanged -- the first two announce, the third stays silent. |
||
|
JMR-dev
|
0702916229 |
Say what the advisory API 37 job actually found, so a new failure is not invisible
That job is continue-on-error and red on every PR by design, which CLAUDE.md states plainly -- and that instruction is exactly why nobody reads it. Nothing in a red X separates "the known three" from "the known three plus yours". A bare failure count would not have fixed it, and this is measured rather than assumed. The run is usually truncated: seven of eight advisory runs read on 2026-08-25 ended in `Test run failed to complete. Expected 3 tests, received 2.` with INSTRUMENTATION_ABORTED, and one did not. A count taken from a truncated run misleads in both directions -- a fourth marked test can still yield the same number if the abort lands earlier, and the known set getting worse can lower it. The test XML does not rescue it either, which was the thing worth checking before building on it: it IS written for an aborted run, and it reports a tidy tests="3" failures="3" for a run the runner had just described as truncated. So the XML is the authority on how many results landed, the runner's own output is the only authority on whether the run finished, and the report reads both and says which number came from where. The baseline is one number beside the marker, because the marker means "cannot pass on this image": the count is both how many tests the advisory leg runs and how many should fail. A smaller failure count is the interesting direction -- it means one now passes, which is the documented trigger for deleting the annotation. Nothing about the job's status changes. It stays continue-on-error, stays red, stays out of the required contexts; a deviation is a ::notice::, never an ::error::. The report is a separate script so it can be run against a real log saved from a real CI run, which is how the comparison was shown to fire. The gating legs get the shape without the comparison: they run the whole suite, so comparing there would announce a deviation five times a run -- but a truncated run reporting fewer results than it ran is what #108 looks like, and "completed cleanly" is the field that would show it. Closes #83 |
||
|
JMR-dev
|
8d8703ab49 | Merge branch 'main' into ci/build-workflow-permissions | ||
|
JMR-dev
|
49c483d877 |
Declare build.yml's token reach in build.yml
CodeQL alert #1, the only open one on this repository: actions/missing-workflow-permissions, warning / medium, build.yml:23 Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Alerts 2, 3 and 4 were the same rule against status_check.yml and are fixed -- that file has a top-level block. build.yml declares permissions in exactly one place, the release job's `contents: write`, and has no top-level default, so the `test` job inherits the repository setting. **Nothing is over-privileged today.** The repository default is already `read` (default_workflow_permissions: read, can_approve_pull_request_reviews: false, read from the API rather than assumed), so the test job holds a read token now. Saying so matters: this is hygiene, and a commit that implied it was closing a live hole would be overstating it. What it buys is that the default CANNOT widen these jobs later without someone editing this file. That is not invented for the occasion -- it is the argument status_check.yml already makes, which even names this file: the token's reach should be readable here, and a default that widens later should not silently widen these jobs with it. build.yml's release job makes the opposite declaration for the same reason. So the principle was decided, applied in two workflows and in one job of this one, and the top level of build.yml was the gap. Verified the thing that would actually break: the release job's `contents: write` still wins. Top level is a default, not a ceiling -- parsed and printed both, test inherits `contents: read`, release keeps `contents: write`. Also ran the ticket's mutation, and it found something. Deleting the release job's `contents: write` leaves actionlint green and CodeQL quiet -- a narrower permission is not an alert -- so nothing would catch it until a tagged release failed to publish. That is a separate gap and is filed rather than fixed here. actionlint clean at the pinned digest. Comment and permissions only; no step, job or trigger changes. Closes #100. |
||
|
JMR-dev
|
1b220856ab |
Say 37.0 is the choice, not the only api-level that exists
R19 raised two things about this comment. One resolved itself: it used to explain why the matrix had no API 37 row at all, and #56 added the gating row, so that half is gone. The other survived, and this is it. The comment read api-level must be "37.0". A bare 37 is not an SDK package and fails during setup The second sentence is true and was measured -- it cost a run to find. The first overstates it. What must be true is that the api-level is a POINT release; 37.0 is one of several. api37-debug.yml's own input descriptions already say so: API level, as the SDK spells it. 37.0, 37.1, 37.2-beta3, 36 ... System image target. android-37.1 and 37.2-beta* ship ONLY as google_apis_ps16k and docs/api-37-emulator-crash.md measures android-37.0 rev 6 and android-37.1 rev 8 side by side, both aborting. So the repo already knows 37.1 exists and behaves the same; only this comment implied otherwise. That matters for the reader it is written for. Someone debugging this row and wondering whether a newer image helps reads "must be 37.0" as a constraint and stops. The measured answer is that it does not help, which is a better thing to learn than a rule that is not one -- and the ps16k-only wrinkle above 37.0 is the detail that would actually bite them. Comment only. No job, matrix, filter or gating behaviour changes. actionlint clean at the pinned digest. Closes #28. |
||
|
JMR-dev
|
3f140fc2b1 |
Lint the bash inside the workflows, not only the bash in files
The shellcheck step added a few hours ago reads `git ls-files '*.sh'`. That is four files. It does not read the inline `run:` blocks, and a good deal of this repo's bash lives there: the release verification in build.yml, the emulator setup and teardown in status_check.yml and api37-debug.yml. "shellcheck runs in CI" was true of the files and not of the blocks, and CLAUDE.md said so rather than pretending otherwise. actionlint closes that half. It parses each workflow and runs shellcheck over every `run:`, on top of its own checks for expression syntax, `needs:` references, matrix keys and action input names. Pinned by digest, for the reason shellcheck is pinned -- a new rule making untouched files fail is a red build whose diff cannot explain it -- and for a second reason of its own. actionlint's documented install is bash <(curl -s https://raw.githubusercontent.com/.../download-actionlint.bash) off a moving branch. Running that in a repository that pins every action by SHA would contradict its own supply-chain posture more than the linter is worth. That is why #70 was filed instead of bolted onto the shellcheck commit. It reported exactly one finding, and it is fixed here rather than suppressed: build.yml parsed `ls` to pick the release APK (SC2012). The glob was already in the line, so a bash array reads it without the pipe. Gradle's output names have no spaces today, which is the kind of assumption that holds right up until it does not. Proved it catches something, rather than trusting a green run: planting `if [ $UNQUOTED = bad ]` into a build.yml `run:` block produces shellcheck reported issue in this script: SC2086:info:4:6: Removed again afterwards. A linter that cannot be shown to catch a plant is not wired in, it is just running -- and SC2086 in a `run:` block is invisible to the .sh-file step, which is the whole argument for this commit. CLAUDE.md loses the "does not cover inline run: blocks" caveat, because it no longer does. Both linters verified clean at their pinned digests. Closes #70. |
||
|
JMR-dev
|
b9abe85580 |
Say three where a third test joined, and stop the name claiming to be exact
#80 added SafPickerRoundTripTest's rotation case to @FailsOnEmulatorApi37, because a real rotation aborts the framework on android-37.0. Three tests carry the marker now -- two in Media3EngineTest, one in SafPickerRoundTripTest -- and five statements still described two. Four were counts, and wrong: status_check.yml "notAnnotation removes the two tests that do not pass" status_check.yml "The two API 37 tests the gating row above excludes" CLAUDE.md "the gating leg runs the other 55" (59 - 3 = 56) CLAUDE.md "do not read a green run as evidence those two tests pass" The fifth was worse, because it was not a count. The advisory job's header justified its name with an invariant: "It is named for WHAT IT RUNS, deliberately. Both tests drive a full H.264 -> H.265 hardware transcode through Media3Engine" The rotation case drives no transcode. So the comment did not merely miscount -- it asserted a property of the job's contents that had stopped being true, and that property was the entire argument for the name. The name is unchanged, deliberately, and the header now says so instead of implying the question never arose. This is not a required context, it is red on every PR by design, and it is one people have learned to look for; renaming a check costs more than the imprecision does. What replaced the invariant is the honest rule: THE MARKER IS THE DEFINITION, NOT THE NAME -- this job holds the tests that cannot pass on the API 37 emulator image, whatever their subject. Two things stay as they were because they are still true. "the two Media3EngineTest cases that pass here" is correct: that class has four tests and two carry the marker. And the decoder theory is still a claim about the Media3 pair alone, so it now says so rather than being read as covering a rotation failure it has nothing to do with. Nothing about the job's behaviour changes: same name, same continue-on-error, same marker, same selection on both rows. Verified: yaml parses, five jobs, matrix still 33/34/35/36/37. The check to re-run when a test next joins or leaves the marker, which is the event that broke this twice: grep -rn "@FailsOnEmulatorApi37" app/src/androidTest --include='*.kt' | grep -v import | grep -c FailsOn It must equal the number every corrected comment states. It is 3. Closes #81. |
||
|
JMR-devandClaude Opus 5
|
3925f1aa9f |
Re-find the picker node when it goes stale, and re-measure API 37
CI found a flake this workstation could not, and fixing it overturned half of what
the previous commit recorded about API 37.
THE FLAKE. UiObject2 caches the AccessibilityNodeInfo it was found with, and
DocumentsUI is still settling when a node first appears -- its list rebinds, the
roots strip lays out, a window animates. If the node is replaced in that gap,
click() throws against the handle rather than missing the target:
androidx.test.uiautomator.StaleObjectException
at androidx.test.uiautomator.UiObject2.getAccessibilityNodeInfo(UiObject2.java:1042)
at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526)
at SafPickerRoundTripTest.pickTheFixture(SafPickerRoundTripTest.kt:223)
It is not intermittent on a COLD emulator -- CI hit it on API 33, 34 and 35, every
one of them, on the first run. It never appeared here because the local emulator had
been warm for an hour. tapPickerNode now re-finds the node and taps again, three
attempts. That retries acquiring a handle to a node that has to be there anyway:
every attempt still goes through awaitPickerNode, which fails outright if it is
absent, so the MIME mutation's bite is untouched. Verified with `pm clear
com.google.android.documentsui` between runs, five for five green on API 34.
AND THE CORRECTION IT FORCED. The previous commit marked the whole class
@FailsOnEmulatorApi37 on the strength of two measured failures. One of them was
this bug. Re-measured with the fix, one method per fresh android-37.0 emulator:
thePickedInputSurvivesARealRotation INSTRUMENTATION_ABORTED:
System has crashed.
pickingAFileThroughTheSystemPickerFillsInTheFileCard PASSED
So a rotation, which rebuilds every surface at once, is what the gralloc mapper does
not survive; starting another app's activity is not. The marker moves to the one
method that earned it, and the picker test runs on the gating API 37 leg like
anything else. The workflow comment, run-e2e.sh and the doc all say that now.
The lesson is worth more than the measurement, and the doc keeps it: an annotation
is a claim about an IMAGE, and a broken test makes every image look broken. Both a
framework abort and a stale node read as "the run fell over". Re-measure after
fixing a test before deciding what the platform did.
Also measured rather than assumed, since it is what keeps the gating leg green: the
runner's annotation filter honours a class-level marker, expanding it to every
method. On API 34, `annotation=` selected exactly 4 tests (2 Media3EngineTest + 2
here) and `notAnnotation=` selected 55 with neither of these in it. CI's own gating
API 37 leg then reported 55 / 0 on the previous push. That is why moving the marker
to a single method is a narrowing rather than a repair.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
a3c835b7c9 |
Keep the picker test off the API 37 gating leg, having measured why
The API 37 emulator images abort surfaceflinger inside the guest's Gralloc5 mapper,
init SIGKILLs zygote with it, and the framework restarts under the run. run-e2e.sh
and the CI leg disable SystemUI to remove the trigger -- but that removes the IDLE
one, RegionSamplingThread's nav-bar luma sampling. Driving DocumentsUI and rotating
the display are not idle. They are the first things in this suite that generate
surface traffic of their own.
Both tests were measured on android-37.0 under swangle_indirect with SystemUI
disabled and verified quiet, and measured SEPARATELY -- inferring the second from
the first is the mistake docs/api-37-emulator-crash.md opens by correcting. They
fail in the two shapes a framework restart produces:
thePickedInputSurvivesARealRotation
INSTRUMENTATION_ABORTED: System has crashed.
Expected 59 tests, received 50
(5 hasReadColorBufferDma aborts; the framework dies DURING the test, so six
later tests never run and the XML carries a failure with no text at all)
pickingAFileThroughTheSystemPickerFillsInTheFileCard
androidx.test.uiautomator.StaleObjectException
at androidx.test.uiautomator.UiObject2.click(UiObject2.java:526)
(3 aborts; the picker's root node was rebuilt between finding it and tapping it)
Both pass on API 33 and API 36 locally -- whole suite, 59/0/0/2 on each -- which is
the same evidence pattern that made the Media3EngineTest pair the image rather than
the app.
So the class carries @FailsOnEmulatorApi37 and runs on the advisory leg.
THREE PLACES SAID "nothing in this suite touches system UI", and that is what makes
the SystemUI-disable deviation defensible. It is no longer true of the suite, and all
three are corrected rather than left to rot -- the workflow comment, run-e2e.sh's
header, and the doc. The rule they state is being APPLIED, not broken: the thing that
depends on system UI is excluded from the leg that cannot be trusted for it.
Two consequences stated rather than left to be discovered:
- run-e2e.sh applies no annotation filter, unlike CI, so a local `run-e2e.sh 37`
reports these two on top of the Media3 pair AND DOES NOT FINISH. Its totals come
back short and which later tests ran is arbitrary. The summary row now says so;
it previously promised "exactly two failures", which would have read as a
regression in someone else's diff.
- The advisory job is still named "E2E API 37 Media3 hardware transcode", and half
of what it now runs is neither. Renaming a check touches branch protection, so it
is deliberately not done here; the doc records the staleness and the revisit
trigger now says the marker covers two unrelated bugs that can go green apart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-dev
|
6cd17f25aa |
Pin shellcheck, because the unpinned one disagreed with the local run
The step added in the previous commit went red on its own PR, and the reason is the one
CLAUDE.md already gives for pinning ktlint, detekt and JaCoCo: "a new rule in a linter
makes files nobody touched stop passing, so CI goes red on a PR whose diff cannot explain
it." Here it was not even a new rule, just a different version of the same tool.
The runner's ambient shellcheck is 0.9.0. The container used to check locally was 0.11.0.
They disagree about how to report `on_signal`, which is installed as the INT and TERM trap
eleven lines below its declaration and so is never called by name:
0.11.0 SC2329, once, on the function declaration -- "never invoked"
0.9.0 SC2317, seven times, one per command in the body -- "appears to be unreachable"
The disable directive named SC2329, so 0.11.0 was silent and 0.9.0 reported seven findings.
Nothing about the script was wrong; the local check simply was not the check CI ran.
Two changes, because either alone still leaves a way to be surprised:
- CI runs shellcheck from an image pinned by digest, so an upgrade is a line in this
file that someone chose, not something that arrives on a Tuesday. The version is
still printed, so a finding out of nowhere can be tied to that line.
- The directive names SC2317 and SC2329 both, so a contributor whose distro ships 0.9.0
gets the same answer locally as CI gives. Verified against both images: clean under
0.9.0 and clean under 0.11.0.
CLAUDE.md now says to check with the pinned digest rather than with whatever is installed,
which is what would have caught this before the push.
|
||
|
JMR-dev
|
baaaa934e0 |
Check the shell, and stop one-off issues falling off the board
Two gaps, both found the same way -- by something going wrong quietly.
`gh issue create` does not touch the project board. The issue is created, carries its
labels, and is invisible in the Kanban, which looks exactly like a ticket nobody filed.
On 2026-08-24 eight issues filed as a scripted batch all reached the board and one filed
as a one-off minutes later did not; it surfaced only because someone went looking for it.
A batch carries the board step inside its loop. One-offs are where it slips, so
tools/github/file-issue.sh is for one-offs.
Three things it does that a two-command shell snippet would not:
- Resolves the project, Status field and option ids BY NAME, every run. Caching them
is the obvious optimisation and the wrong one -- a renamed or reordered column would
then have this writing a stale id into the board with no error anywhere.
- Reads the item back. A mutation returning 200 says the request was accepted, not that
the board shows what was asked for; the read-back is the only step that checks the
claim this script exists to make. It is a GraphQL query because REST cannot do it --
the `fields` array REST returns on a project item carries Title and nothing else, so
a REST-only check reports every item's Status as unset.
- Exits 3, loudly, with the issue number on a line of its own, when the issue was
created but the board step failed. That exact combination is the failure being
prevented; it must never be the quiet path.
Shell was the other language here with nothing checking it -- four scripts, one of them
the CI entry point. shellcheck now runs in the Static analysis job over
`git ls-files '*.sh'`, so a script added later is covered without editing the workflow,
and it runs at full severity with `info` included.
That raises two findings today and both are the tool being wrong, so both are answered
with a targeted `disable` carrying its reason rather than by lowering the severity:
run-e2e.sh's `on_signal` is reported as never invoked when it is installed as the INT and
TERM trap eleven lines below it, and the `$names` inside file-issue.sh's queries are
GraphQL variables that must not expand -- expanding them would send the shell's idea of
$owner to the API instead of declaring a parameter. A blanket --severity=warning would
have hidden both, and the next real finding with them.
The gradle step gains `if: !cancelled()` so a shellcheck failure cannot cost the
ktlint/detekt/lint lists -- the same reason that step already passes --continue.
Not covered, deliberately: shellcheck here reads .sh files, not the inline `run:` blocks
in the workflows, where a good deal of this repo's bash actually lives. actionlint does
read them, and finds one pre-existing info-level issue in build.yml. Wiring it in means
pinning a container digest, because every action here is pinned by SHA and actionlint's
usual installer is a curl-pipe-bash off a moving branch. Its own ticket, not this commit.
|
||
|
JMR-devandClaude Opus 5
|
225ecdd7e6 |
Split the API 37 leg so the part that works can gate
CI has never run the API level this app targets. The reason it did not was never "API 37 is untestable" -- it was that two tests fail on the emulator image, so one row would be permanently red or permanently allow-listed. This splits that row instead of choosing between those two. E2E API 37 gates. It runs 55 of the suite's 57 instrumented tests and must be green. E2E API 37 Media3 hardware transcode runs the other two, reports, and never blocks (continue-on-error). Both are driven off ONE marker, @FailsOnEmulatorApi37: the gating job passes notAnnotation, the advisory job passes annotation. Two lists would drift, and drift is silent in both directions -- a test that ends up in neither job reads as green. Excluding by class was not an option either: Media3EngineTest has four tests and two of them pass here, so notClass would have thrown away real coverage. The advisory job is named for what it runs, not for what we think is wrong. Both its tests drive a full H.264 -> H.265 hardware transcode, which is what distinguishes them from the two Media3EngineTest cases that pass -- those never decode video. The goldfish-decoder theory sits in a comment inside the job, where it can be corrected without renaming a check people have learned to look for; docs/api-37-emulator-crash.md keeps measurement and inference apart. The SystemUI disable moves into .github/scripts/e2e-run.sh behind E2E_DISABLE_SYSTEM_UI, unset everywhere but the two API 37 jobs, so the other four legs run byte-identical commands -- the same shape as E2E_EXTRA_GRADLE_ARGS. It runs BEFORE the streamed logcat starts, deliberately: `adb shell stop` would end that logcat and nothing restarts it, so a disable placed after it would cost the leg its diagnostics for the part of the run that matters. The body is probe v2 from api37-debug.yml -- the version measured 4/4 -- not the older one-round form: three rounds, waits for system_server to actually be gone, verifies against `pm list packages -d`, and requires a 45 s window with zero new aborts. The weaker probe reported success on a run that then started SystemUI eight more times. The caveat is written next to the row rather than left implicit: this leg runs with SystemUI disabled and the framework restarted under it, a device configuration no other leg and no Pixel run uses. Anything that touches system UI must not trust it, and the Pixel check before each release is still the only API 37 run with SystemUI intact. docs/api-37-emulator-crash.md's "So should CI take API 37?" said no on three reasons. Two were claims about CI that had never been measured; the section now carries the eight runs that measured them, and the third reason is what the split answers. docs/local-emulator.md and api37-debug.yml's header carried the same "the matrix stops at 36" claim and are corrected with it. CLAUDE.md is left alone deliberately -- its "CI's matrix therefore stops at API 36" clause is now false, and that correction is parked in the doc's existing "Correction owed to CLAUDE.md" section, where two others are already waiting. Making E2E API 37 an actually-required check is a repository-settings change and must come after this is on main: adding a required context that does not exist on the default branch blocks every PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
b3a705e3da |
Measure the API 36 control and record what CI cannot measure
Three additions to docs/api-37-emulator-crash.md, all from a CI investigation run through .github/workflows/api37-debug.yml. A third measured bullet: API 36 against API 37, back to back, same two tests, same renderer, same SystemUI-disable path. 37.0 fails both on c2.goldfish.h264.decoder (32660148155); 36 passes both in 4.603 s with the same decoder in its logcat (32660152961). That falsifies "the stripped configuration is what breaks these tests" -- a reading the other measurements never addressed, because they all compare against a device that still had SystemUI. It carries its two uncontrolled variables rather than dropping them: API 36's framework restart happened with zero aborts logged where API 37's had two, so a restart under an active abort loop is still uncontrolled; and the images differ on the encoder side, which is a second reason "broken h264 decoder" is the wrong shape of claim. The decoder-mechanism bullet is unchanged and still labelled inference. This adds a measurement next to it; it does not retract anything. The intact-SystemUI counterfactual is unmeasurable on a GitHub runner, and now says why. Seven dispatches, zero verdicts, with a mechanism rather than bad luck: while the framework crash-loops the guest cannot reliably create per-user private directories, so an app installed during the loop has no cache dir and the fixture copy dies in @Before before any codec exists. googlesdksetup and nexuslauncher hit the same thing. The result XML masks it behind an UninitializedPropertyAccessException in tearDown, which reads as a defect in this repository and is not one. Abort cadence corrected. "Roughly every 20 s" was the watchdog's sampling interval, not the cadence: measured gaps are 20-90 s, median 60-70 s, three to five per run, with sys.boot_completed held at 1 throughout. The wrong figure lived in api37-debug.yml's own comments, so that line is corrected too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
acc71bcaee |
Verify the SystemUI disable instead of trusting what pm reported
Four dispatches of one configuration -- API 37.0, swiftshader_indirect,
SystemUI disabled -- came back three green and one not, and the odd one out
was not a different failure so much as the same run without the fix applied.
In 32646029143 `pm disable-user` reported `new state: disabled-user` and
SystemUI then started eight more times:
14:41:24 ActivityManager: Start proc 6412:com.android.systemui ... GradientColorWallpaper
14:45:05 ActivityManager: Start proc 17299:com.android.systemui ... GradientColorWallpaper
with ten more RegionSampling aborts and a surfaceflinger pid that never sat
still (489, 1570, 3524, 4396, 6038, 7987, 9732, 11520, 13208, 15048). The
framework is being SIGKILLed every twenty seconds while this runs, so a
package-state change can go down with the system_server that accepted it.
Two things were wrong, and the second is why the first went unnoticed:
- one disable attempt was treated as sufficient
- the wait after `adb shell stop` was not a wait. It asked `service check`
0.3 s later and got `found` from the system_server that was still on its
way out, so it never waited for anything. Both the good and the bad run
printed `services back after 5 s`, which is how a broken fix looked
identical to a working one.
Now: up to three rounds of disable -> take the framework down and confirm
system_server is actually gone -> bring it back -> verify the package is in
`pm list packages -d` -> require a 45 s window with zero new aborts. Nothing
is believed because a command said so.
Also adds measure_baseline, default true. The 45 s pre-measurement is what
makes the rate comparable with the local figures, but it is 45 s of
crash-looping before the disable has to land, which is a worse starting
point than a real leg would have.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
93398c4616 |
Resolve adb by path in the API 37 watchdog
The first three dispatches came back with every watchdog sample reading `boot=? surfaceflinger=none zygote64=none dma_aborts=0`, on runs where the device demonstrably booted and the action's own adb was working two steps away. The watchdog was not measuring anything. The emulator action puts platform-tools on PATH with core.addPath, which writes GITHUB_PATH and therefore only affects LATER steps. The watchdog is started before the action -- that is the whole point of it -- so it inherits the runner's own PATH, where a bare `adb` is not necessarily anything. Every call failed into `2>/dev/null` and the sampler dutifully recorded the silence as zero. It now resolves adb by path, preferring ANDROID_HOME, re-resolving on every iteration in case platform-tools arrives later, and echoing the path it settled on. The launch step prints ANDROID_HOME and `command -v adb` for the same reason: a repeat of this failure should be one line to spot, not three runs of quiet zeros. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
8fdad6e20b |
Add a dispatch-only workflow for the API 37 CI question
status_check.yml stops its E2E matrix at 36 and says the android-37.0 image is why. That is established locally under -gpu host and under ANGLE, and it is not established for CI: runners use -gpu swiftshader_indirect, and the one local measurement of that mode was void for a local reason -- Fedora denies execheap to SwiftShader's JIT, so the emulator died before the guest mattered. What CI does at API 37 has therefore never actually been measured. This is that E2E job with the matrix replaced by workflow_dispatch inputs, so a hypothesis costs a dispatch rather than a commit: renderer, API level, image target, channel, SystemUI disable, boot timeout, whether the suite runs at all, and free-form emulator and Gradle arguments. It triggers on nothing else and gates nothing. It calls .github/scripts/e2e-run.sh rather than forking it, and pins the same disk-size, ram-size, action SHAs and KVM setup as the job it copies, so a run here measures the renderer and not a different device. The watchdog is load-bearing rather than decorative. The emulator action calls killEmulator() from its own catch block, so a run whose emulator never boots is torn down before any script: line executes and leaves nothing behind -- which is the exact failure shape API 37 is suspected of. It starts before the action, samples sys.boot_completed, the surfaceflinger and zygote pids and the hasReadColorBufferDma abort count every 20 s, and keeps a rolling copy of the crash buffer so the last read survives the teardown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
22c7914395 |
Find out why the emulators segfault, and make them run
CLAUDE.md has said "Emulators segfault on this host -- qemu dies on every AVD" since the E2E matrix landed, and the PR that introduced it called the failure "exit 139 across three AVDs and both GPU backends, environmental". That is accurate about the symptom and wrong about the cause, and the cost of being wrong was the whole instrumented suite being unrunnable here. SwiftShader's Reactor JIT writes generated GLES shader code onto the heap and mprotects it executable. Fedora's SELinux policy denies that -- execheap is not granted to unconfined_t and selinuxuser_execheap is off -- so the mprotect fails and the emulator takes SIGSEGV the moment it calls the routine it just generated. The AVC denial and the core are the same event, one second apart. The predictor is mechanical and held 7 for 7 across every -gpu mode: a run crashes if and only if it dlopens gles_swiftshader/libGLESv2.so. host, angle_indirect and swangle_indirect boot. auto, off, guest and swiftshader_indirect crash -- and auto is the default, which is why the failure looked universal rather than renderer-specific. tools/local-emulator/run-e2e.sh picks a renderer that works and refuses the ones that do not. It reuses .github/scripts/e2e-run.sh rather than forking it, so the local and CI diagnostics cannot drift; the one change there adds an optional E2E_EXTRA_GRADLE_ARGS that is unset in CI, so CI runs byte-identical commands. The API 33-36 sweep has now been run and is written down. All four levels are green on a local emulator and match the physical Pixel 10 Pro XL baseline exactly: 49 tests, 0 failures, 0 errors, 2 skipped, every level. Those counts come from the result XML, not the UTP console counter, which double-counts skips and reported "Finished 51 tests" on all four. No boot log dlopens SwiftShader GLES and the sweep window holds no AVC denial and no qemu core -- which is confirmation of the mode matrix's first row rather than new coverage, since every one of these runs is -gpu host. The table is still seven modes measured once each. Two things the sweep surfaced that the doc now records: pre-build before sweeping, or a fresh checkout spends API 33's 20-minute wrapper budget compiling and wedges before a test runs; and the device pinning is untested by this run, because the Pixel dropped off USB five seconds before it started. Still offered for review rather than applied: the CLAUDE.md correction the doc drafts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
39e0900928 |
Adapt LibreMail's emulator instrumentation for the E2E matrix
The E2E legs could fail with almost nothing to show for it. The previous handler
was a single line of semicolons printing meminfo and 60 lines of crash logcat,
and it only ran when gradle RETURNED non-zero -- a hang left nothing at all, and
`adb logcat -d` at the end only holds whatever survived in the ring buffer, which
a chatty run evicts.
The two failure shapes want different evidence, so they are handled separately:
FAILED -- gradle returned non-zero. The test reports already say which test and
why, so this captures the surrounding state: guest memory and
storage, whether the app even installed, native crashes, and the
runner's own kvm/memory/disk.
WEDGED -- gradle never returned and the wrapper timeout killed it. There are no
reports, so the evidence has to come off the live device: which test
was in flight per the TestRunner logcat, whether the binder services
are published, and SIGQUIT thread dumps of both processes. That last
one is the point -- ART writes full stacks to logcat and /data/anr,
which is what separates a deadlocked test from a stuck MediaCodec
from a device that stopped answering. dumpsys media.player is in
there because both engines transcode through MediaCodec, so a hung
conversion shows up in it.
Logcat is now streamed to a file from the start of the step and uploaded whichever
way the leg goes, since the leg worth reading is usually the one that went red once
and green on re-run -- by which time the emulator is gone.
It is a script rather than inline YAML because it has to be. The action splits its
`script` input on newlines and runs each line as its own `sh -c`, so functions and
`if` blocks cannot survive there; that constraint is what produced the one-line
handler in the first place. One line calls the script now.
The wrapper timeout is 1200s against measured ~5-minute healthy legs, so it cannot
trip on a slow-but-working run, and sits far enough under the 60-minute cap to
leave room for the capture. It wraps only the foreground gradle client, never the
emulator the action owns, so it cannot hang the leg itself.
Not adopted from LibreMail: the hand-provisioned AVD boot, its SDK-integrity
installer and its focus gate. Those answer failures this repo has not had, and
replacing a boot path that works to fix problems we do not have is how a working
matrix breaks. Every emulator setting here -- ram-size, disk-size, the ABI filter,
swiftshader -- is untouched, along with the reasoning already written next to it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
8727fccba1 |
Declare read-only permissions on the status-check workflow
CodeQL flagged the new static-analysis job for relying on the repository's default GITHUB_TOKEN scope. Fair, and the repo already holds the opposite opinion elsewhere: build.yml's release job spells out contents: write with a comment saying the token's reach should be visible at the point of use. Set at workflow level rather than on the one job that was flagged, because none of these four write anything -- they read the code, build it and attach reports. It also means a repository default that widens later cannot quietly widen these jobs with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
fd6e5325cf |
Raise Kotlin to 2.4.10 so the bytecode can join the toolchain on Java 25
The previous commit settled for Java 24 everywhere because Kotlin 2.2.10 refuses jvmTarget 25. That was the wrong constraint to accept, for two reasons. The first is that 24 turned out to be unbuyable. Adoptium's repository carries 8, 11, 17, 21, 25 and 26 -- no 24, because it is a non-LTS that went end of life in July 2025. The builds passed only because Gradle quietly auto-provisioned 24.0.2+12 through foojay, and .idea/misc.xml had been pointed at a temurin-24 that cannot be installed. A toolchain nobody can install is not pinned, it is lucky. The second is that the cap was never on the toolchain at all. Kotlin's ceiling applies to jvmTarget -- the bytecode -- and the JDK running the build is a separate axis. Conflating them is what steered this at 24 in the first place. So the fix is the one the sibling repo already uses: put KGP on the root buildscript classpath, where AGP's built-in Kotlin picks it up instead of the 2.2.10 it bundles. Kotlin 2.4.10 supports jvmTarget through 26, which lifts the ceiling above the toolchain rather than under it. The Compose compiler plugin is versioned in lockstep and reads the same catalog entry, so the two cannot drift, and the module now applies both by id() because they come from the classpath rather than from plugin resolution. Checked rather than assumed, since a silent downgrade would look identical to success: compiled classes report major version 69, which is Java 25. D8 dexes them, R8 minifies them, and ktlint, detekt, lint, the unit tests and the androidTest compile are all green on top. 25 is the right landing place independent of all this: it is LTS, it is in the Adoptium repository, and temurin-25-jdk is already installed here -- so the daemon runs on a real system JDK rather than a provisioned copy of an unpatched one. Two catalog plugin aliases went with it. android-application and kotlin-compose now resolve from the buildscript classpath, so leaving aliases behind would have left two entries that read like the source of truth and control nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
3841f58c74 |
Put the whole toolchain on Java 24, and take Gradle to 9.7.1
Java was scattered across four numbers that nobody had chosen together: the
daemon ran on 25 (pinned in gradle-daemon-jvm.properties), CI installed 17, the
IDE was set to 25, and the app compiled to 17 bytecode. Now all four say 24.
24 rather than 25 because 25 is not reachable end to end. Kotlin 2.2.10 refuses
jvmTarget 25 outright -- "available targets are 1.8 ... 23, 24" -- so the app's
bytecode could never have joined a 25 toolchain, and "everything on the same
version" would have stayed false in the one place it is hardest to notice. 24 is
the highest number all four can actually hold. Checked, not assumed: D8 dexes
Java 24 class files, and R8 full mode minifies them, so the shipped artifact
builds on this too.
Floating where floating is native:
- java-version: '24' -- setup-java resolves the newest 24.x at run time.
- toolchainVersion=24 -- Gradle reports it as "Compatible with Java 24, any
vendor", and provisions whatever 24.x it finds or downloads.
The Gradle wrapper deliberately does NOT float, because it cannot: distributionUrl
names one archive and distributionSha256Sum is the checksum of that exact file.
That pairing is the wrapper's integrity check, and it is the same reasoning the
workflows already apply to action SHAs. Set via `./gradlew wrapper`, not by hand,
so the checksum matches the URL.
Dependencies were audited against Google Maven and Maven Central rather than
guessed at, and almost everything was already current: AGP, the Compose BOM,
core-ktx, activity, lifecycle, navigation, work, datastore, media3, room,
documentfile, annotation, espresso, androidx-junit, junit and ktlint are all at
their newest stable. Only two had moved -- detekt to 2.0.0-alpha.6 and JaCoCo to
0.8.15 -- and both are here.
Kotlin stays at 2.2.10 and that is now recorded as a verified fact rather than a
warning: the AGP 9.3.1 POM declares kotlin-gradle-plugin 2.2.10 at runtime scope,
which is what AGP's built-in Kotlin actually compiles with. Android lint suggests
2.4.10 and taking that suggestion breaks the build unless KGP is also forced onto
the root buildscript classpath. agp, kotlin and ksp move together or not at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
JMR-devandClaude Opus 5
|
65a94b4ec1 |
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>
|
||
|
JMR-devandClaude Opus 5
|
4e6fe6b75a |
Drop API 37 from the E2E matrix and write down why
The android-37.0 emulator image crash-loops surfaceflinger inside its own gralloc mapper: RegionSamplingThread calls GraphicBuffer::lock, which reaches GoldfishMapper::readFromHost, which asserts that the host has not negotiated ReadColorBufferDma. It has, so surfaceflinger aborts, restarts, and aborts again. Nothing this app does can survive that, and it reproduces on a GitHub runner under swiftshader_indirect and on a workstation under -gpu host alike. There is no ATD image at android-37.0 to fall back to, and -feature -GLDMA is accepted by the emulator but does not prevent the assertion. Correcting the previous commit, which is already pushed so its message stands: ram-size was not the cause of that failure. Setting it did move the job from failing at install to failing during the test run, which is how the real crash became visible, but at 2560M the guest had 1.5 GB free when it died. The setting is kept because the emulator's own floor varies by API level -- 2048M at 33, 2560M at 34 to 36 -- and pinning it makes the matrix uniform. Also corrected: a comment claiming this could not be reproduced locally. It can, and the local crash was the same one all along. Dropped the dmesg probe. adb shell is not root, so klogctl is denied and it only ever printed a permission error -- which a later reader would reasonably misread as "no OOM kills". docs/api-37-emulator-crash.md carries the evidence, the ruled-out fixes, the reproduction, and how to file it upstream, so re-adding the row later starts from what is already known rather than from scratch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2fe1aa9f9c |
Stop the memory probe from being able to fail the run
The probe line runs before the tests and its exit status is grep's, so a run where adb returned nothing would have exited 1 on the first line and reded the job before Gradle started -- on all five levels, four of them currently green. The action passes no ignoreReturnCode, so exec throws straight into setFailed. A diagnostic must never be the thing that turns a run red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
a79ff62b61 |
Give the API 37 emulator the RAM every other level already gets
API 37 was the only red job in the matrix, and the last two fixes each corrected a real problem only to reveal the next one. This is the cause of the third failure. The emulator raises an undersized guest to 2560M on its own, but only for API levels it recognises, and it does not recognise "37.0". Comparing the two CI logs from the same emulator binary (37.1.11.0) shows the asymmetry directly: the API 36 job logs "Increasing RAM size to 2560MB" and the API 37 job has no such line. So four levels were quietly running at 2560M while API 37 ran at the pixel_6 default of 1536M, lost system_server partway through installing the 82 MB APK, and surfaced it as "Can't find service: package". 2560M is not a guess at a sufficient value -- it is the value the other four levels already pass at, so this makes the matrix uniform rather than introducing a fifth configuration. Verified that the setting actually lands: the action appends hw.ramSize to a config.ini that already has one from the profile, so the fix only works if the later key wins. Appending a distinctive 3072M to an API 36 AVD produced MemTotal 3047924 kB and suppressed the automatic bump, confirming it does. This failure cannot be reproduced locally -- API 37 will not boot on a workstation under either GPU mode, aborting surfaceflinger in the goldfish mapper under -gpu host and segfaulting the emulator under swiftshader_indirect -- so the job now reports guest memory on every run and dumps OOM kills and native crashes on failure. That makes the next run conclusive either way instead of producing another bare "Can't find service: package". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
97cddea973 |
Pin build.yml's actions and verify what it publishes
Brings the release workflow in line with the status check. It matters more here, not less: these jobs publish the artifacts people install, so running whatever a mutable tag points at on the day is a worse bargain than it is on a pull request. Every action is pinned to a commit with its release in a trailing comment, and each hash was checked to resolve to the tag it claims. gradle/actions is dropped for the same reason as before -- its v6 caching component is closed source and carries separate terms -- with Gradle running through the committed wrapper, which verifies its own distribution against distributionSha256Sum. The release job now checks what it is about to publish. A release that shipped a single ABI, or that lost 16 KB alignment in a rebuild, installs fine on a test device and then fails for users or at Play submission. Both are cheap to assert and expensive to discover afterwards. It deliberately does not pass -PabiFilters: that override exists so emulator jobs skip libraries they cannot execute, and a released artifact must carry every ABI. The contents permission is declared explicitly rather than inherited from the repository default, so the token's reach is visible in the file that uses it. The corresponding-source tarball now includes bin/README.md as PREBUILT.md, so the GPL source drop carries the shipped binary's SHA-256 and configure line rather than only the recipe that produces it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
5b47764c70 |
Build only the emulator's own ABI for instrumented tests
API 37 got as far as running the suite this time and then failed to install: 'package install-create ... -S 117817978' java.io.IOException: Requested internal only, but not enough space The 117 MB debug APK did not fit on the emulator's data partition. The DELETE_FAILED_INTERNAL_ERROR that followed was the same exhaustion, not a second problem. It surfaced on API 37 because that system image is the largest and leaves the least free userdata. The margin was thin at every level, so this was never really an API 37 bug -- the others were simply further from the edge and would have caught up as the APK grew. Roughly half that APK is arm64-v8a FFmpeg libraries that an x86_64 emulator can never load. abiFilters is now overridable, so a test run builds only what it will execute: 114 MB becomes 80 MB. Release builds ignore the property and still ship both ABIs, so nothing about what gets distributed changes. disk-size is raised to 8G for every level rather than only the one that failed, since fixing just API 37 would leave the rest waiting their turn. Verified locally on an API 36 emulator with an x86_64-only APK: 40 instrumented tests, 0 failures, and the installed APK contains lib/x86_64 only. 66 unit tests still pass, and a release build still carries both ABIs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
2d2687aa3c |
Pin CI actions to commit hashes and fix the API 37 emulator run
The API 37 job failed after 23 seconds, before an emulator ever started. The CI log names the cause exactly: sdkmanager --install 'build-tools;37.0.0' platform-tools 'platforms;android-37' Warning: Failed to find package 'platforms;android-37' There is no platforms;android-37. The release is published as android-37.0, alongside 37.1 and the 37.2 betas. The earlier attempt to fix this with system-image-api-level was aimed at the wrong package: that input only names the system image, while the platform is installed from api-level directly. Setting api-level to 37.0 resolves all three packages, and build-tools is a hardcoded constant in the action rather than derived from api-level, so it is unaffected. A separate label field keeps the job name reading "API 37". Every action is now pinned to a commit hash with its release in a trailing comment. A tag is mutable: the owner can repoint v4 at new code whenever they like, so a tag reference amounts to running whatever that repository contains tomorrow. Each hash was verified to resolve to the tag its comment claims, because a wrong hash is worse than a tag -- it looks deliberate. Versions moved a long way in the process: checkout v4 -> v7.0.1, setup-java v4 -> v5.7.0, upload-artifact v4 -> v7.0.1. gradle/actions is gone rather than upgraded. Its v6 release moved the caching component closed-source and states that upgrading accepts Gradle's Terms of Use for it. That has no bearing on the project's own licence -- a CI tool is never combined with or distributed alongside the app, unlike the FFmpeg libraries that make the APK GPL -- but it is a component in the build path that cannot be audited or forked. Gradle now runs through the committed wrapper, which verifies its own distribution against distributionSha256Sum, and caching is a handful of lines of actions/cache. The rest of the matrix passed on this run: API 33, 34, 35 and 36 all green, along with the unit tests and the FFmpeg archive check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
128763e99c |
Commit the FFmpeg binary so test runs stop depending on a rebuild
CI rebuilt FFmpeg on every cold cache, which made results ambiguous: a red run could mean the code was broken or that a forty-minute cross-compile of FFmpeg, x264, x265 and SVT-AV1 had hiccuped. Those are not the same signal, and only one of them is worth a developer's attention. The archive is now checked in under bin/, so a failing run points at code. It also removes roughly forty minutes from a cold run and lets a fresh clone build without a container toolchain. bin/README.md records provenance -- upstream tag, FFmpeg version, NDK, ABIs, SHA-256 and the full configure line read back out of the shipped libavutil -- so the binary is auditable rather than opaque. The recipe in tools/ffmpeg remains the authority: this archive is its output, and is also what satisfies the GPL corresponding-source obligation. The status check is now seven independent runners: one validating the archive, one for the JVM tests, and one per API level from 33 to 37. The FFmpeg job verifies rather than builds. It asserts native libraries are present for both ABIs and that every one is 16 KB aligned, which is a Play requirement that is easy to lose in a rebuild and expensive to discover at submission. Checking for file existence alone would not do: a Git LFS pointer checked out without LFS passes that and then surfaces as an obscure linker error much later. It is a separate job rather than a step in each emulator run so a bad archive reports once, clearly, instead of five confusing emulator failures. build.yml no longer builds FFmpeg either, and keeps only its post-merge and release duties. Two costs, deliberately accepted. The repository goes from about 1 MB to 35 MB, and every future rebuild adds another 35 MB blob to history permanently, so bin/README.md says to regenerate only when the FFmpeg version or the configure flags actually change. And F-Droid's scanner flags checked-in native libraries, so submitting there needs a scandelete entry for bin/ -- noted in bin/README.md, and nothing prevents a from-source build. Verified against the relocated archive: 66 unit tests, and 40 instrumented tests on an API 36 emulator, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
54d6e9c57b |
Add a pull-request status check across API 33-37
Runs the JVM tests and the instrumented suite on every pull request to main, with one emulator job per supported API level. The matrix is the whole range rather than a single level because the foreground-service type differs across it -- none below 34, dataSync at 34, mediaProcessing from 35 -- so testing one level would leave two thirds of that branch unexercised. Running the range locally is what caught a test that had baked in an assumption about the host's encoders. API 37 needs its image level stated separately. It is published as android-37.0, not android-37, so a plain integer resolves to nothing and the image download silently finds no package. FFmpeg is built once and shared. The AAR is not committed -- 35 MB of native code, and F-Droid strips checked-in binaries -- but every job needs it, since the app compiles against it and the instrumented tests exercise it for real. Building it is a full cross-compile of FFmpeg, x264, x265 and SVT-AV1, so it is cached on the contents of tools/ffmpeg, which is what actually determines the output. The job also asserts the AAR carries native libraries for both ABIs: a truncated or stub archive would otherwise pass a file-exists check and send the matrix off to fail confusingly five times over. fail-fast is off. Knowing whether a failure is universal or specific to one API level is most of the diagnosis. build.yml no longer runs on pull requests. It triggered on every PR with no branch filter, so both workflows would have run, and its unit job falls back to a stub AAR -- a weaker check that could mask a compile break the real one would catch. It keeps its post-merge and release duties. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
JMR-devandClaude Opus 5
|
bb969ece64 |
Enable R8 and add release, store and F-Droid infrastructure
Turning on R8 immediately surfaced a latent runtime bug: the ffmpeg-kit-next wrapper references com.arthenica.smartexception.java.Exceptions from AbstractSession.fail() in eighteen places, but a local .aar carries no transitive dependencies, so nothing was pulling it in. Debug builds tolerate this through lazy class loading -- the class is only touched on an error path -- so it would have shipped as a crash the first time an FFmpeg conversion failed. Declared explicitly now. Keep rules cover the JNI boundary. The native library resolves classes and methods by name, which R8 cannot see, so without them the FFmpeg calls fail with NoSuchMethodError in release builds only. Workers are kept too, since WorkManager reconstructs them reflectively from a class name persisted in its database, and a rename breaks jobs enqueued before the update. Verified on the produced artifacts rather than assumed: all 22 native libraries survive minification and every one is still 16 KB aligned inside the APK. Release is 82 MB against 115 MB for debug; the AAB is 40 MB and Play splits it per ABI. The privacy policy lists every permission, including the three WorkManager adds automatically (WAKE_LOCK, RECEIVE_BOOT_COMPLETED, ACCESS_NETWORK_STATE). Checking the merged manifest showed those, and a policy that omitted them would look dishonest to anyone who inspected the app. INTERNET is genuinely absent, so "files stay on the device" is enforced by the OS rather than a promise. CI runs unit tests on every push and builds the FFmpeg AAR only for release tags, since that is a full cross-compile. Releases attach the FFmpeg corresponding source next to the APK: GPL-3.0 requires it, and FFmpeg's instruction to host it "on the same webserver" cannot be satisfied by a Play listing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |