Harden the Android payload gate: ordering and 16 KB alignment - #869
Draft
a-ghorbani wants to merge 12 commits into
Draft
Harden the Android payload gate: ordering and 16 KB alignment#869a-ghorbani wants to merge 12 commits into
a-ghorbani wants to merge 12 commits into
Conversation
Android packages assets/ once per artifact, so declaring the required DSP libraries under an ABI implied a scoping the packager cannot deliver. They move to a top-level assets block with an explicit scope, and the payload gate checks and reports them once per artifact. The global accelerator guard keys off abis[].requiredLibs and is untouched by the move, so it gains a test that fails when it is lost.
…tifact The assets reach every ABI whether or not it can use them, so usableByAbis is a claim about the shipped lib/<abi>/ trees rather than a packaging rule. It is derived from the trees and compared with the declaration: equality between two readings of the manifest would hold whatever either one said. The accelerator name test the manifest guard already used is now one named function, so the two readings cannot drift apart.
The payload check parsed section headers only, so segment alignment was unreadable. The new reader handles ELF32 as well as ELF64 and either byte order: the DSP assets are 32-bit objects, and a reader that copied the dynsym reader's little-endian 64-bit assertion would throw on them. The synthetic objects the tests build gained a program header table, which they previously declared as absent.
Android 15+ will not load a shared library whose PT_LOAD segments are not page-aligned on a 16 KB-page device. The gate now reads p_align for every lib/<abi>/*.so, not only the ones the manifest names: the platform loads all 68 of them, and the whole scan costs under a second. The requirement is declared per ABI so it is reviewed with the payload it constrains, and floored at 16384 in the script, since lowering the number is otherwise the cheapest edit that unblocks a failing build.
Every Android publishing step already sat after the payload gate, but only as prose in three workflow files: a step reordering, an added if:, or a publishing step in a new workflow would have broken it in silence. The test reads the workflows and the three Fastfiles and asserts ordering, that the gate examined the very artifact being published, that nothing touched it in between, that the gate can still fail its job, that no fastlane lane both builds and publishes, and that anything else resembling a transport is either classified or exempt with a checked reason. Each rule is also exercised against a broken in-memory copy, since a rule only ever seen passing is not known to be able to fail. ci.yml's DCE check moves above the gate: it names the APK, and no step may name the artifact between the gate and the upload. js-yaml is declared rather than relied on transitively, which leaves yarn.lock unchanged.
…line too
The two uses:-based publishers already refused a path that resolves to no
filename — a glob, a bare directory or an expression — because a rule keyed
on literal filenames has nothing to compare and would skip the step.
The command-line publishers did not. 'gh release create v1.0.0 dist/*.apk'
was classified, so ordering held it on position, but path identity ran with
an empty path list: vacuous, which reads exactly like satisfied. Below the
gate it was unguarded outright. Both branches now take the same fallback,
and the new cases assert path identity fires rather than only ordering.
The expression clause is read differently in the two places on purpose: in
a with: path field the whole value is the path, so any expression hides
one; in a shell command a ${{ }} is as likely to be a tag or a secret, so
only one sitting against an .apk/.aab counts. The Fastfile's aab: literal
is untouched and still resolves.
…aimed Every rule passed on the committed files; each of these is a mutation it failed to catch, which is why a green suite was consistent with all of them. The gate. A publishing step carrying if: always() runs on a build the gate just refused — on the release path that is the bundle reaching Play, the tag and the Release. The rule's own reasoning, that a skipped step is not a failed step, had been applied to the gate side only. The gate's own neutering enumeration also missed every form bash -e does not report: &, a leading !, single-line if/while wrappers, and set +o errexit. Alignment. p_align was asserted and the zip data offset was not. The app sets extractNativeLibs=false, so libraries are mapped in place and both must hold; the fixture repacked without padding passed with 68/68 misaligned offsets. Offsets are read from the local header, scoped to stored entries of an APK, and the fixtures gained a stored-mode zip writer — zip -q -r -X deflates, so the rule would otherwise have had an empty subject set. Classification. It was first-match twice over, so a step that is two publishers at once reported one, and a path list naming a gated file beside a glob reported only the file. A with: path is now read entry by entry, and anything that is not a concrete filename counts — which also catches dist/ and artifacts/, invisible while the key was build/outputs. Exemptions. One decided on the job and discarded its step; the probe could not see the class, because its only job made the job half false. A second probe job removes every excuse. Composite actions were read without their with: block, so one uploading the APK read as shipping nothing. Text. Comment stripping was quote-unaware and failed open: --notes "Fixes truncation only ever moves a step toward exempt. Lane names are matched against the known set, not by position — the release job runs no gradlew, so its building-ness rests entirely on that resolution. The abandoned per-ABI asset keys are refused rather than ignored.
Five review rounds produced five spellings of one defect: uses: branches, command-line branches, composite actions, a command-token pre-filter, quote state spanning lines. Each round closed the spellings someone had thought of. Deciding whether a step publishes by recognising what publishing looks like is a blacklist over free-form shell and arbitrary actions, and it will keep leaking. Inside a job that builds Android the default is now inverted. A step is a violation unless something accounts for it: the gate, a classified publisher, an allowlisted action, or a named entry with a reason and an assertion. 55 steps across the three building jobs, of which 27 are named. A new step costs a reviewed line whether or not it looks dangerous — echo hello is refused exactly as bash tools/ship.sh is, because no judgement about the text is being made. The gate step's own shape is pinned the same way: one command, optionally after set -euo pipefail. Enumerating the ways bash -e can be made to report success — &, !, if/while, set +e in four spellings, trap ERR, a trailing command — had leaked in three consecutive rounds. A publisher that must name an artifact and resolves to none is refused; an empty path list was indistinguishable from a satisfied one. Shell commands now get the same concreteness question as with: paths, so an indirect path is unresolvable rather than invisible. A deflated library is refused rather than excused. The skip rested on a platform generality that is false here: the app ships extractNativeLibs= false, so nothing is extracted and a compressed library is unloadable on every device. The manifest declares that, floored at true. Also: the zip reader checks its read lengths and no longer reuses one header buffer; a composite this parse cannot walk is not one that ships nothing; the abandoned per-ABI asset keys gained the negative cases that hold their refusal; the archive-layout case now reaches the branch it names.
The inversion closed the set of steps and left open the set of what a step does. Being recognised — as a publisher, as the gate, as an allowlisted action — ended the enquiry, so the largest steps in the workflow sat outside it: appending a line to Push the release tag moved nothing, and a step could grow any transport it liked while its exemption held. Every step of a building job is now accounted for, publishers and gates included, and each row pins a digest of the step's content. An edit inside an accounted step costs the same reviewed line that adding a step costs. That is what makes the exemption assertions stop being load-bearing: they can only refuse the transports somebody thought of, and a census of ten hostile appends — gcloud, az, nc, ssh, python -m http.server, plain cp — left every one of them holding. All fail on the digest now. The two local composite actions are pinned the same way, being free-form shell behind one uses:. upload-artifact classifies whenever it carries a path, not only when an apk/aab can be read out of it; conditioning that made the empty-resolution refusal unreachable for the one class it was written for. What a report upload may ship is a pinned list of non-artifact paths, asserted disjoint from build/outputs, because a tarball of the output directory is a concrete filename too. Also: the gate step's key set is pinned, not just its script, since env and working-directory decide what runs; a bare git push is an argument list rather than an absence; ambiguity between entries and steps is refused in both directions; line continuations and a wrapperless gradle are seen as building; a composite's inner uses: is subject to the allowlist; the deflate fixture is a real deflate stream; the past-EOF read has a control; the unreachable UNINDEXED branch is gone rather than untested; and the step inventory is asserted instead of described.
The pin covered a step and stopped at its edges. Two boundaries, one
argument: what a step does is decided by more than the step's own text.
The job and its workflow. defaults.run.shell replaces the shell for every
step, so bash -c "bash {0}; true" makes each one exit 0 with no step
touched; a job-level env preloads a module into every node;
defaults.run.working-directory moves the paths a script's flags name;
container and runs-on decide the machine. Seven insertions into the
committed tree left all seven rules clean. Each building job now pins a
digest of its own keys and its workflow's — the same two mechanisms the
gate's key-set pin already named one level down.
The Fastfiles. The release job runs its whole build as 1.3 kB of Ruby
behind one run:, and lane calls were not followed transitively, so a lane
reached only through another lane was invisible. All three are pinned by
content and lanes resolve transitively.
In the gate itself: unzip transliterates bytes it cannot render, so the
subject set taken from its listing and the index built from the central
directory disagree on exactly the entries whose naming is chosen freely — a
deflated library under such a name passed outright, the report contradicting
itself with nothing reading the contradiction. The directory is now the
single name source, the two readings must agree, and the denominator is the
whole subject set. I argued last round that this was unreachable and neither
of us tested it; it was reachable in 23 of 256 byte values.
Also: the declared non-artifact uploads match on the whole path rather than
the basename, and the disjointness the docblock claimed is now asserted
rather than described; any gradle invocation reads as building, since naming
tasks missed build and :app:aPR; the digest is 96-bit and takes step.raw
bare; a mistyped assertion name is refused; the unreachable path sentinel is
gone; the gate key-set pin has its own negative case.
…cked The gate now refuses an APK whose libraries are deflated, so the two fixtures added for the version report have to be stored like every other APK fixture. Their subject is the version line, not the packaging.
a-ghorbani
force-pushed
the
feature/TASK-20260822-1131
branch
from
August 26, 2026 14:26
ba7dd33 to
10fd8fc
Compare
… assertion The mistyped-assertion guard was a disjunction, and 55 of 58 rows carry a digest, so the right side always answered and the left was never read. A typo in an assertion name therefore retired that row's assertion in silence on 55 of 58 rows — and since digests are per-step content, the rule that notices a gate moving out from between a build and its publishes is exactly what a typo switches off. It reads the assertion now. In the gate: a central directory that repeats a name kept the later entry, so the index held fewer libraries than the archive declared and its counts read as complete. Refused by comparing against the declared count. Two pins still enumerated their own subject, which is the shape the previous round fixed one level down. Fastfiles come from git ls-files rather than a literal list, so a fourth tracked one cannot go unread — 3 tracked against 4 on disk, so the source matters. And the gate's report is exempt because a gate step below the upload wrote that path with nothing touching it since, not because it is called payload-report.txt: any step can cp the signed APK to that name. The llama.rn version line is constrained to a semver shape and says it describes the build host, since it is a fact about the machine running the check and package.json is arbitrary JSON from a dependency tree. Also: composite digests widened to 96-bit like the other three; the pinned job map gained the inventory assertion it was the only one missing; the disagreement message names the entries that differ instead of two equal counts; the branch that became unreachable when the subject set moved to the index is gone; the round-4 orphan docblock is deleted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #859. Closes #862 in full and lands two of the four bullets in #863.
#862 — publish-after-payload-check is now a test, not prose.
scripts/__tests__/android-publish-ordering.test.jsparses every workflow and the three Fastfiles and asserts that in each Android building job every publishing step — Play upload, tag push, GitHub Release, artifact upload — sits after a payload gate step; that the gate examined the very artifact being published, compared by filename; that nothing named that artifact between the gate and the publish; that the gate can still fail its job (noif:, nocontinue-on-error, no||, no pipe, noshell:override, no test-only--manifest, and the check as the final command); that no fastlane lane both builds and publishes; and that anything else resembling a transport is either classified or exempt with a reason plus a machine-checked assertion. Both exemption surfaces carry the same floor, and one test proves no exemption rests on an assertion that no input can falsify.A publisher path the parse cannot resolve to a filename — a glob, a bare directory, a
${{ }}expression — is refused rather than skipped, so it cannot escape by being unnameable.#863 — 16 KB page alignment. The gate reads ELF program headers and requires every
PT_LOADof every library shipped underlib/<abi>/to be atp_align >= 16384and a power of two. The subject is every shipped library, not only the ones the manifest names: Android 15+ loads all of them on a 16 KB-page device. The requirement is declared per ABI and floored in the script, since lowering the number is otherwise the cheapest edit that unblocks a failing build. The new reader handles both ELF widths and both byte orders — the DSP assets are 32-bit objects — and was cross-checked against the NDK'sllvm-readelfover all 72 shared objects in the artifact with no mismatches.#863 — DSP asset scoping. The manifest format change this asked for has landed: the required assets move to a top-level
assetsblock with an explicitscope, and a newusableByAbisfield is asserted against the artifact — an ABI is listed if and only if its shippedlib/<abi>/tree carries a library able to load the assets. Every payload report now states per ABI whether it can use the DSP payload.Acceptance criterion 3 is dropped, not delivered
#863 asks that ~2.8 MB of
assets/ggml-hexagon/libggml-htp-v*.sostop shipping tox86_64. That is refused, with evidence, rather than deferred quietly.No supported toolchain mechanism ABI-scopes
assets/. bundletool parses five directory-targeting keys —countries,group,lang,tcf,tier— and there is noabikey; asset packs target min-SDK, device feature, tier, texture format and country, never ABI; AGP asset source sets are per flavour and build type; andsplits { abi }copies the whole asset tree into every split. The AAB base module reaches every device regardless. The two mechanisms that would work — an ABI flavour dimension, or an upstream llama.rn move of the HTP payload intojniLibs— are disproportionate to the measured cost: 2,836,288 bytes of a 233 MB artifact, 1.2%.What landed instead earns its place independently:
usableByAbisis bound to the artifact rather than to the manifest declaring it, so it catches an accelerator variant appearing under an ABI that cannot load the assets, and it makes the wasted 2.8 MB a declared fact in every report. The remaining disposition is the maintainer's.Native verification exemption for
package.jsonAGENTS.mdnamespackage.jsonas a trigger requiringpod installplus an iOS and an Android build, and this PR touches it. The rule applies and is being knowingly exempted, on measurement rather than argument.The entry is one JS-only
devDependenciesline,js-yaml@^4.1.0, used by the new test to parse workflow YAML. Autolinking does scandevDependencies, so "it is a JS package" is not sufficient on its own. The control is direct:node ./node_modules/react-native/cli.js configproduces byte-identical 63,060-byte output with and without the entry, on an instrument proven responsive — removingreact-native-sharemoves the same output to 61,819 bytes and the autolinked count from 33 to 32. Nothing underandroid/,ios/, no Podfile,*.podspec,build.gradle, or native module is touched.js-yamlis already a top-level key inyarn.lock, so--frozen-lockfileneeds no new entry and the lockfile is unchanged.A reviewer who disagrees should say so; this is surfaced rather than left to be discovered.
One committed workflow file changed to satisfy the rules rather than be exempted from them
ci.yml's DCE sanity check moves above the payload gate. It names the APK by path, and no step may name the artifact between the gate and the upload, so it either moved or bought an exemption; moving it is the honest option.The move is behaviour-preserving — same job, same step list,
needs:unchanged, noif:added or removed, and the gate → report upload → APK upload order intact. It has one side effect worth stating: both steps are unconditional and the job halts at the first failure, so a DCE failure now stops the gate from running, and theif: always()report upload finds no report to upload. Before the move, a DCE failure still left a payload report behind. That is a diagnostic-coverage trade, not a weakening of the gate.Verification
npx jest scripts/— 12 suites, 263 tests. Full suite 275 suites, 4,437 passing, 2 skipped.yarn.lockunchanged.EM_QDSP6, arm64-v8a 38/38 and x86_64 30/30 libraries atp_align >= 16384, asset usability read from thelib/trees and matching the declaration.Notes
context/architecture/release.md.Generated by PocketPal Dev Team