Repository navigation
Rebuild the Whisper Android AAR for 16 KB memory pages - #5688
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f9b06487b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
shai-almog
added a commit
that referenced
this pull request
Sep 3, 2026
Review finding on #5688: an unreadable `.so` payload was exempted rather than reported, so a corrupted native library could be committed while the check still exited 0. Two ways through. A file named `.so` that is not an ELF at all returned None and was dropped silently, counting toward nothing. And a *truncated* 64-bit library still begins with the ELF magic, so it fell out of the same branch and was announced as "32-bit, not subject to 16 KB pages" -- the exemption for an ABI the rule does not cover was reachable by corruption. MIN_LIBRARIES_SCANNED did not help: the six valid Whisper slices satisfy the floor on their own. This is the rule already applied to archives the gate cannot open, which the ELF parse itself was not holding to: an entry nobody could read is an entry whose alignment nobody checked, and that must not come out reading the same as one checked and found compliant. So `read_load_alignments` now raises for anything that does not parse and returns the ELF class alongside the alignments. A 32-bit library is exempt only once it has been parsed and shown to really be a 32-bit ELF, which means the 32-bit program header layout had to be read too -- the two layouts differ in field order, not just in width. It also rejects a PT_LOAD segment that ends past the end of the file. Program headers sit near the front, so a library truncated mid-file parses happily and reports correct alignment for contents that are not there. The "rebuild with NDK r28" advice is now printed only when something was actually misaligned; for a truncated artifact it pointed at the wrong fix. The same hole existed in the Java check in AndroidWhisperAarProjectTest and is closed the same way, with the entry name threaded in so a failure says which library. Verified against a probe AAR carrying a non-ELF `.so`, a 40-byte truncation, a mid-file truncation, a valid 32-bit slice and a valid aligned 64-bit one: the first three fail in both the script and the test, the last two behave as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abc43040c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
shai-almog
added a commit
that referenced
this pull request
Sep 3, 2026
Review finding on #5688: the whole-tree scan walked past every tracked `.cn1lib`, so a misaligned native library shipped through Codename One's own library format was never inspected and the gate still exited 0. `tests/core/lib/QRScanner.cn1lib` reaches real `.so` files through `nativeand.zip` and then `ZBarScannerLibrary.aar`, three archives deep, and none of them were being read. The reported fix is to add `.cn1lib` to the extension list. The extension list is the defect, though: it is a curated allow-list, it was already wrong about the one format this project defines itself, and adding an entry leaves the next container -- `.kar`, `.srcjar`, `.apks`, whatever a future packaging step emits -- just as invisible. So containers and ELFs are now recognised by their magic bytes and the list is gone. Only the first four bytes of an entry are decompressed unless it turns out to be one or the other, which keeps a whole-tree run at about two seconds over eleven thousand files. MAX_ARCHIVE_DEPTH goes 3 -> 8. The QRScanner chain is exactly three deep, so the old limit had no headroom at all, and since exceeding it is now reported as a failure rather than a silent skip, a limit set that close to real usage would have produced false alarms rather than quiet gaps. This does not change the verdict on the tree as it stands: the six libraries inside that cn1lib are all 32-bit (armeabi, armeabi-v7a, x86), which the 16 KB rule does not cover. The scan simply sees twelve 32-bit libraries now instead of six. Proven against a probe that injects the pre-rebuild 4 KB whisper library into the cn1lib -> nativeand.zip -> aar chain: the previous gate reports "6 libraries aligned" and exits 0, and this one fails naming the full nested path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a78ae943aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
shai-almog
added a commit
that referenced
this pull request
Sep 3, 2026
Review finding on #5688, and a regression I introduced two commits ago. abc4304 made "not an ELF file" a failure; a78ae94 then dispatched on magic bytes instead of extension, which removed the only route to that check. A zeroed or corrupted `libfoo.so` matched no magic, was classified as an ordinary file, and was skipped in silence -- and the six valid Whisper slices satisfy MIN_LIBRARIES_SCANNED on their own, so the run still exited 0. Trading a curated allow-list for a silent skip was not the intended bargain. Content still decides what gets scanned, which is what finds a container or a library under a name nobody anticipated. A name is now a second, separate signal deciding what gets challenged: a file whose name declares a format must actually be that format. Anything named like a shared library goes to the ELF reader whatever its bytes say, so corruption is reported instead of being quietly reclassified as a format the rule does not cover. The same is done for archive names, because a zeroed `.aar` went through the identical hole. Those suffixes are not the old allow-list returning: nothing is discovered by being on them, and a container is still found by its magic under any name. Verified safe against the whole tree before changing anything -- 3461 archive entries and every tracked file, with zero name/content mismatches, so nothing existing newly fails. Probe cases: a zeroed `.so`, a `.so` holding text, and a zeroed `.aar` are all reported now; an ELF named `weirdname.bin` is still found and checked by magic; the truncation and misalignment probes from the previous commits still fail as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5a7dab389
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
shai-almog
added a commit
that referenced
this pull request
Sep 3, 2026
Review finding on #5688: 16 KB pages are a Google Play requirement, not a property of ELF. An ordinary desktop Linux x86_64 `.so` is built for 4 KB pages and is correct at 0x1000, but the gate reads every tracked file and was holding every 64-bit ELF to the Android rule, so a valid artifact would have failed the workflow and blocked every PR. Codename One really can carry one. Cn1libMojo.buildLinux() packages a cn1lib's `linux/src/main/resources` into `nativelinux`, and buildJavase() does the same for `nativese`, so a prebuilt desktop library on either path is a supported thing to check in. Nothing in the tree does today, which is why this was latent rather than red. A library is subject to the rule when its scan path shows Android packaging: an Android container anywhere in the chain (`.aar`, `.apk`, `.aab`, or the `nativeand.zip` a cn1lib carries), or an Android ABI directory sitting inside `jni`/`lib`/`jniLibs` -- the second so a loose `jniLibs/arm64-v8a/libfoo.so` in a port is still covered. The label already carries the whole chain, so an `.so` reached through `QRScanner.cn1lib!nativeand.zip!ZBarScannerLibrary.aar` is recognised from containers three levels up. Only alignment is scoped. A `.so` that is not an ELF, or one that is truncated, still fails wherever it sits: those are bugs on any platform, and scoping them would reopen the silent skip the previous two commits closed. Demonstrated with one byte-identical 4 KB-aligned 64-bit library placed on three paths: exempt as a loose desktop `.so`, exempt inside `cn1lib!nativelinux.zip`, and still failing under `aar!jni/arm64-v8a/`. The six Whisper slices remain enforced, the pre-rebuild AAR still produces its six findings, and a zeroed `.so` on a non-Android path is still reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e48a227de3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
shai-almog
added a commit
that referenced
this pull request
Sep 3, 2026
Review finding on #5688: `looks_like_shared_library` tested `".so." in base`, so `libfoo.so.sha256` and `libfoo.so.asc` were routed to the ELF reader and failed the run with "not an ELF file". The gate reads every tracked path, so committing an ordinary checksum or signature beside a native library would have blocked the PR. A soname version is numeric -- `libfoo.so.1`, `libfoo.so.1.2` -- so that is what is matched now. Being strict costs nothing: a file that really is a library is still found by its magic whatever it is called. This is the fifth review finding on this script, and all five were the same question answered wrong in a different place -- where the boundary sits between enforced, exempt, and unreadable. Two let something unreadable pass as compliant; two rejected a valid artifact under a rule that does not apply to it. That is a boundary worth pinning rather than re-deriving, so the predicates now have tests. scripts/test_check_16k_page_alignment.py covers all three verdicts separately: enforced for a 64-bit library under Android packaging, exempt for 32-bit and for desktop payloads, reported for anything unreadable, truncated or not an ELF. The sharpest case asserts that one byte-identical payload is enforced or exempt purely by where it is packaged. ELF payloads are synthesised in the test, so it needs no toolchain and no checked-in fixture, and it runs in two milliseconds. Each test was checked against the bug it exists for by reverting that fix in turn: the broad `.so.` match, the Android scoping, the name-forces-ELF routing, the segment-past-end-of-file check, and magic-based container detection. Every reversion turns the suite red; restoring it turns it green. The workflow runs the tests before the scan, so a gate broken in a way that makes it scan nothing is reported as a broken gate rather than as a clean tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81315839bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Google Play requires every 64-bit shared library in an upload to be linked for 16 KB memory pages: API 35+ uploads since 2025-11-01, and Wear OS uploads containing native code from 2026-09-15. Every 64-bit library in the checked-in cn1-ai-whisper AAR had PT_LOAD p_align 0x1000. Alignment is settled at link time, so nothing downstream repairs it. AGP aligns the ZIP entries and bundletool reports the outcome, but neither can move a segment inside a prebuilt .so -- and because the artifact is ours rather than the developer's, they could not fix it from their own sources. The failure is also silent: the app builds and runs on every 4 KB device, and only Play review or a real 16 KB device says otherwise. The AAR is rebuilt with NDK r28. r28 is the first release that links this way by default and, more to the point, the first whose prebuilt libc++_shared.so and libomp.so are aligned -- those are copied out of the NDK, so no flag set in this project could have fixed them on r26/r27. The CMake link options cover libcn1aiwhisper.so for a standalone CMake run or an older NDK; a probe under r26 confirms they move a library from 0x1000 to 0x4000. The rebuild is equivalent and not merely aligned: identical AAR entry set, identical JNI exports on all four ABIs, identical DT_NEEDED, the same whisper.cpp commit, and a 2 KB size delta. That commit is now recorded in the README, because nothing in the tree derived it and it was only recoverable from strings inside the shipped library. minSdk moves 19 -> 21, which is not a consequence of the NDK bump. The NDK's floor has been API 21 since r26, so every slice ever shipped here -- 32-bit included -- already carried API 21 in .note.android.ident while the manifest claimed 19. An API 19 install would have failed at System.loadLibrary. PlatformFeatureCatalog raises a consuming app's android.min_sdk_version to match so the manifest merge still succeeds for a default project. scripts/check-16k-page-alignment.py gates this over every tracked archive and loose .so, in its own workflow rather than inside pr.yml, whose paths filter would let a PR that only adds a native artifact skip it. It reports rather than skips an archive it cannot open or that nests too deep, so "could not look" never reads the same as "looked and found nothing". Scope is 64-bit only: 16 KB pages are a 64-bit feature, and NDK r28 itself ships 4 KB 32-bit prebuilts, so a wider rule would be unsatisfiable. Both gates were confirmed non-vacuous against the pre-rebuild artifact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review finding on #5688: an unreadable `.so` payload was exempted rather than reported, so a corrupted native library could be committed while the check still exited 0. Two ways through. A file named `.so` that is not an ELF at all returned None and was dropped silently, counting toward nothing. And a *truncated* 64-bit library still begins with the ELF magic, so it fell out of the same branch and was announced as "32-bit, not subject to 16 KB pages" -- the exemption for an ABI the rule does not cover was reachable by corruption. MIN_LIBRARIES_SCANNED did not help: the six valid Whisper slices satisfy the floor on their own. This is the rule already applied to archives the gate cannot open, which the ELF parse itself was not holding to: an entry nobody could read is an entry whose alignment nobody checked, and that must not come out reading the same as one checked and found compliant. So `read_load_alignments` now raises for anything that does not parse and returns the ELF class alongside the alignments. A 32-bit library is exempt only once it has been parsed and shown to really be a 32-bit ELF, which means the 32-bit program header layout had to be read too -- the two layouts differ in field order, not just in width. It also rejects a PT_LOAD segment that ends past the end of the file. Program headers sit near the front, so a library truncated mid-file parses happily and reports correct alignment for contents that are not there. The "rebuild with NDK r28" advice is now printed only when something was actually misaligned; for a truncated artifact it pointed at the wrong fix. The same hole existed in the Java check in AndroidWhisperAarProjectTest and is closed the same way, with the entry name threaded in so a failure says which library. Verified against a probe AAR carrying a non-ELF `.so`, a 40-byte truncation, a mid-file truncation, a valid 32-bit slice and a valid aligned 64-bit one: the first three fail in both the script and the test, the last two behave as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review finding on #5688: the whole-tree scan walked past every tracked `.cn1lib`, so a misaligned native library shipped through Codename One's own library format was never inspected and the gate still exited 0. `tests/core/lib/QRScanner.cn1lib` reaches real `.so` files through `nativeand.zip` and then `ZBarScannerLibrary.aar`, three archives deep, and none of them were being read. The reported fix is to add `.cn1lib` to the extension list. The extension list is the defect, though: it is a curated allow-list, it was already wrong about the one format this project defines itself, and adding an entry leaves the next container -- `.kar`, `.srcjar`, `.apks`, whatever a future packaging step emits -- just as invisible. So containers and ELFs are now recognised by their magic bytes and the list is gone. Only the first four bytes of an entry are decompressed unless it turns out to be one or the other, which keeps a whole-tree run at about two seconds over eleven thousand files. MAX_ARCHIVE_DEPTH goes 3 -> 8. The QRScanner chain is exactly three deep, so the old limit had no headroom at all, and since exceeding it is now reported as a failure rather than a silent skip, a limit set that close to real usage would have produced false alarms rather than quiet gaps. This does not change the verdict on the tree as it stands: the six libraries inside that cn1lib are all 32-bit (armeabi, armeabi-v7a, x86), which the 16 KB rule does not cover. The scan simply sees twelve 32-bit libraries now instead of six. Proven against a probe that injects the pre-rebuild 4 KB whisper library into the cn1lib -> nativeand.zip -> aar chain: the previous gate reports "6 libraries aligned" and exits 0, and this one fails naming the full nested path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review finding on #5688, and a regression I introduced two commits ago. abc4304 made "not an ELF file" a failure; a78ae94 then dispatched on magic bytes instead of extension, which removed the only route to that check. A zeroed or corrupted `libfoo.so` matched no magic, was classified as an ordinary file, and was skipped in silence -- and the six valid Whisper slices satisfy MIN_LIBRARIES_SCANNED on their own, so the run still exited 0. Trading a curated allow-list for a silent skip was not the intended bargain. Content still decides what gets scanned, which is what finds a container or a library under a name nobody anticipated. A name is now a second, separate signal deciding what gets challenged: a file whose name declares a format must actually be that format. Anything named like a shared library goes to the ELF reader whatever its bytes say, so corruption is reported instead of being quietly reclassified as a format the rule does not cover. The same is done for archive names, because a zeroed `.aar` went through the identical hole. Those suffixes are not the old allow-list returning: nothing is discovered by being on them, and a container is still found by its magic under any name. Verified safe against the whole tree before changing anything -- 3461 archive entries and every tracked file, with zero name/content mismatches, so nothing existing newly fails. Probe cases: a zeroed `.so`, a `.so` holding text, and a zeroed `.aar` are all reported now; an ELF named `weirdname.bin` is still found and checked by magic; the truncation and misalignment probes from the previous commits still fail as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review finding on #5688: 16 KB pages are a Google Play requirement, not a property of ELF. An ordinary desktop Linux x86_64 `.so` is built for 4 KB pages and is correct at 0x1000, but the gate reads every tracked file and was holding every 64-bit ELF to the Android rule, so a valid artifact would have failed the workflow and blocked every PR. Codename One really can carry one. Cn1libMojo.buildLinux() packages a cn1lib's `linux/src/main/resources` into `nativelinux`, and buildJavase() does the same for `nativese`, so a prebuilt desktop library on either path is a supported thing to check in. Nothing in the tree does today, which is why this was latent rather than red. A library is subject to the rule when its scan path shows Android packaging: an Android container anywhere in the chain (`.aar`, `.apk`, `.aab`, or the `nativeand.zip` a cn1lib carries), or an Android ABI directory sitting inside `jni`/`lib`/`jniLibs` -- the second so a loose `jniLibs/arm64-v8a/libfoo.so` in a port is still covered. The label already carries the whole chain, so an `.so` reached through `QRScanner.cn1lib!nativeand.zip!ZBarScannerLibrary.aar` is recognised from containers three levels up. Only alignment is scoped. A `.so` that is not an ELF, or one that is truncated, still fails wherever it sits: those are bugs on any platform, and scoping them would reopen the silent skip the previous two commits closed. Demonstrated with one byte-identical 4 KB-aligned 64-bit library placed on three paths: exempt as a loose desktop `.so`, exempt inside `cn1lib!nativelinux.zip`, and still failing under `aar!jni/arm64-v8a/`. The six Whisper slices remain enforced, the pre-rebuild AAR still produces its six findings, and a zeroed `.so` on a non-Android path is still reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review finding on #5688: `looks_like_shared_library` tested `".so." in base`, so `libfoo.so.sha256` and `libfoo.so.asc` were routed to the ELF reader and failed the run with "not an ELF file". The gate reads every tracked path, so committing an ordinary checksum or signature beside a native library would have blocked the PR. A soname version is numeric -- `libfoo.so.1`, `libfoo.so.1.2` -- so that is what is matched now. Being strict costs nothing: a file that really is a library is still found by its magic whatever it is called. This is the fifth review finding on this script, and all five were the same question answered wrong in a different place -- where the boundary sits between enforced, exempt, and unreadable. Two let something unreadable pass as compliant; two rejected a valid artifact under a rule that does not apply to it. That is a boundary worth pinning rather than re-deriving, so the predicates now have tests. scripts/test_check_16k_page_alignment.py covers all three verdicts separately: enforced for a 64-bit library under Android packaging, exempt for 32-bit and for desktop payloads, reported for anything unreadable, truncated or not an ELF. The sharpest case asserts that one byte-identical payload is enforced or exempt purely by where it is packaged. ELF payloads are synthesised in the test, so it needs no toolchain and no checked-in fixture, and it runs in two milliseconds. Each test was checked against the bug it exists for by reverting that fix in turn: the broad `.so.` match, the Android scoping, the name-forces-ELF routing, the segment-past-end-of-file check, and magic-based container detection. Every reversion turns the suite red; restoring it turns it green. The workflow runs the tests before the scan, so a gate broken in a way that makes it scan nothing is reported as a broken gate rather than as a clean tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… spots Review finding on #5688: `classify` asked the library name first but left the archive name as a fallback *after* the magic bytes. A valid ELF left in place of an `.aar` was therefore read as a library and counted as a compliant Android slice; the same bytes named `.cn1lib` were waved through as non-Android. Either way a destroyed container kept the run green, which is the opposite of what the format-integrity rule is for. Both reproduced before the change. The rule was only ever half-applied. Declarations are now answered before discoveries, for both formats symmetrically: a name that declares a format routes to that format's reader whatever the bytes say, and content decides only when the name declares nothing. Discovery under an unanticipated name is unaffected. Two problems in the test file itself, found while verifying that: Its loader served stale bytecode. `python3 -m py_compile` on the gate leaves a `.pyc` behind, and importlib really did shadow an edit with it, reporting a behaviour the source no longer had -- a pinning test that can pass against code that no longer exists is worse than no test. The gate is now compiled from source every run, demonstrated by mutating the source with a stale `.pyc` in place and watching the suite go red. And a mutation survey of every guard found six with no coverage at all: the program header table bounds, the entry-size floor, the no-program-headers and no-PT_LOAD cases, the unknown ELF class, and the archive depth limit. Each now has a test, and each was confirmed to fail when its guard is removed. Removing any of the twelve guards now turns the suite red; before, half of them could have been deleted silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
force-pushed
the
fix/5682-whisper-16kb-page-alignment
branch
from
September 3, 2026 16:23
8131583 to
4046597
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40465975a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review finding on #5688: the alignment test was a minimum-only comparison, so a `p_align` of 0x4001 cleared it and was announced as 16 KB compliant. ELF permits 0 and 1 ("no alignment required") or a power of two, and nothing else, so 0x4001 is a malformed segment rather than a value above the floor -- the gate would have been vouching for a library no loader would honour. Reported as malformed rather than misaligned. It is not a number a linker produces, so "rebuild it with a newer NDK" would be the wrong advice, and the check sits with the other integrity checks in read_load_alignments rather than in the Android-scoped alignment comparison: a corrupt alignment is corrupt on any platform. Surveyed the tree first: every PT_LOAD in it is 0x1000 or 0x4000, all powers of two, so nothing existing newly fails. Tests pin both directions -- 0x4001 and 0x6000 are reported, and 0x8000 and 0x10000 still pass, because the rule is a floor and a larger legal alignment is compliant. Removing the guard turns the suite red. Practical likelihood is low: no real toolchain emits such a value, so this protects against a corrupted or hand-crafted artifact rather than a build mistake. It is cheap and it keeps one rule intact -- the gate never reports something it cannot vouch for as compliant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e987fbb837
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two review findings on #5688, both variants of the gate vouching for a library it has no grounds to vouch for. ELF requires p_vaddr and p_offset to be congruent modulo p_align. A segment that is not cannot be mapped as it declares itself, and the alignment test never looked, so such a library was reported compliant. The second is about the gate's own reasoning rather than the file. The ABI directory is the evidence used to decide which rule applies, so when the file contradicts it there is no sound verdict left to reach: a 32-bit ELF under jni/arm64-v8a was exempted as "32-bit, rule does not apply", and x86_64 bytes under the same directory were counted as a compliant arm64 slice. Both are guesses dressed as answers, and Android can load neither. Reported now, for the same reason an unreadable file is. That check is deliberately narrow. It fires only where a path actually names an Android ABI directory, and a library outside one is not second-guessed -- this is not general ABI validation, which is another tool's job, but a guard on the inference this gate makes. Measured against every library in the tree before switching either on: 18 libraries, zero congruence violations, and all 18 agreeing with their ABI directory, so nothing existing newly fails. Tests pin both directions -- each ABI directory still accepts its own architecture, and a library outside one is untouched. Removing either guard turns the suite red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20db45ecd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two review findings on #5688, and one missing field behind both. The parser never read `e_type`, so it could neither tell that a file was not a library nor that it was not the kind of library Android can load. The one that mattered is the false positive. A relocatable object from `cc -c` legitimately has no program header table, and the gate demanded one of every ELF it found, rejecting it with "program header entry size 0 is too small". Reproduced with a real object built by the NDK's clang: checking a prebuilt `.o` into the tree would have blocked every PR. That is the same shape as the desktop `.so` and the checksum sidecar -- a valid artifact failed by a rule that was never about it. The other is hardening: an `ET_EXEC` or `ET_CORE` with aligned, in-bounds segments was counted compliant, though Android's linker loads only `ET_DYN`. Both fall out of one rule, the same declaration-versus-discovery split the rest of the gate already uses. A file that claims to be a library -- by a `.so` name, or by sitting in an Android ABI directory -- must be a shared object, and is reported when it is not. An ELF found purely by its magic under a name that claims nothing never claimed to be a library, and is not this gate's business. Reading the header is now separate from reading the segments, so the gate can find out WHAT a file is before committing to parse it as something it may not be. The program header table is required only once `e_type` says it should have one. Verified against the tree: all 18 libraries are ET_DYN, so nothing existing newly fails. Tests pin the split in both directions -- a `.o` found by magic is left alone, the same bytes named `.so` or placed under an ABI directory are reported, and a real shared object is still inspected as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
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
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.
Fixes #5682.
The defect
Every 64-bit library in the checked-in
cn1-ai-whisper-android.aarhadPT_LOAD p_align 0x1000--libcn1aiwhisper.so,libc++_shared.soandlibomp.so, for botharm64-v8aandx86_64. Google Play requires0x4000for every 64-bit shared library in an upload: API 35+ uploads since 2025-11-01, and Wear OS uploads containing native code from 2026-09-15.Alignment is settled at link time. AGP aligns the ZIP entries and bundletool reports the outcome, but neither can move a segment inside a prebuilt
.so, so an app could not repair it -- and the failure is silent, because the app builds and runs on every 4 KB device. Only Play review or a real 16 KB device says otherwise.The fix
The AAR is rebuilt with NDK r28 (
28.2.13676358). r28 is the first release that links this way by default and, more to the point, the first whose prebuiltlibc++_shared.soandlibomp.soare aligned -- those two are copied out of the NDK, so no flag set in this project could have fixed them on r26/r27.CMakeLists.txtalso passes-Wl,-z,max-page-size=16384and-Wl,-z,common-page-size=16384, which coverslibcn1aiwhisper.sofor a standalone CMake run or an older NDK.Verification
0x4000; the six 32-bit ones are untouched.DT_NEEDED, the same whisper.cpp commit (0874de3e), same recorded API level, ~2 KB size delta..cn1libwas checked too, not only the file insrc/main/resources-- that is the artifact an app build actually consumes.0x1000to0x4000, and the flags appear in the generated link command.Two things worth flagging in review
minSdk19 -> 21 is not a consequence of the NDK bump. The NDK's floor has been API 21 since r26, so every slice ever shipped here -- 32-bit included -- already carried API 21 in.note.android.identwhile the manifest claimed 19. The old value was a claim the native code never honoured, and an API 19 install would have failed atSystem.loadLibrary.PlatformFeatureCatalognow raises a consuming app'sandroid.min_sdk_versionto match, so the manifest merge still succeeds for a default project.The gate is 64-bit only, deliberately. 16 KB pages are a 64-bit feature, and NDK r28 itself ships 4 KB 32-bit
libc++_shared.so/libomp.so-- a wider rule would be unsatisfiable. 32-bit slices are still listed under--verbose.The whisper.cpp pin is now recorded in the README. Nothing in the tree derived it; it was only recoverable because the version and abbreviated hash survived as strings inside the shipped library.
Not in this PR
Issue #5682 also asks to check every shipped native dependency. Five CN1-pinned third-party artifacts are non-compliant as well:
com.microsoft.onnxruntime:onnxruntime-android:1.16.3org.tensorflow:tensorflow-lite:2.13.0com.google.ai.edge.litert:litert:1.0.1com.google.mlkit:barcode-scanning:17.2.0com.google.mlkit:face-detection:16.1.5Already compliant: ARCore 1.44.0, SQLCipher 4.17.0, language-id 17.0.6, smart-reply 17.0.4.
tensorflow-litehas no fix by version bump: 2.16.1 is the last real AAR and is still 4 KB, and 2.17.0 is a 1.4 KB stub because the artifact moved tocom.google.ai.edge.litert. That one needs a migration.Held back deliberately -- bumping five ML dependency pins is a different change with its own device-testing needs, and this issue is scoped to the whisper AAR. The audit covers direct pins only, not natives arriving transitively via
play-services-*.Local test plan
Both green.
check-control-characters.pyandcheck-copyright-headers.shpass over the branch range.🤖 Generated with Claude Code