Repository navigation
fix(android): report split-required manifests as a JNI library unknown - #9769
2SinceJuly wants to merge 8 commits into
Conversation
inspect-android-jni-bindings reported library_inventory.status "complete" with zero libraries for a base APK whose manifest requires ABI and density splits, even though the libraries live in those splits. The unqualified complete let an agent conclude the app has no native code while DEX still listed native declarations. Read the admitted APK's binary AndroidManifest.xml and add an explicit unknown when it declares required split types or isSplitRequired. APKs whose manifest declares neither keep the same inventory as before, and a manifest that cannot be read or parsed reports its own unknown rather than a bare complete.
Codex Review Summary: ✅ CompletedThis 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: 8b79781c96
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| chunk.start, | ||
| chunk.chunkSize, | ||
| ); | ||
| console.log("DBG", JSON.stringify(el?.name), el?.attributes.length); |
There was a problem hiding this comment.
When inspect-android-jni-bindings reads any ordinary APK with a root manifest, the eager .map() executes this call for every XML start element and writes DBG ... records to process stdout. That prefixes CLI JSON output and, under the default stdio MCP transport, interleaves non-protocol lines with JSON-RPC responses, so otherwise successful inspections can become unparsable; remove the call and avoid production stdout diagnostics.
AGENTS.md reference: AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removed in efecd1cbe. The call was a leftover from developing the split-facts probe and had no reader; nothing else in src/android writes to stdout.
| input.inventory.unknowns.push( | ||
| `The manifest requires split APKs (${splitTypes.join(", ")}); native libraries may live in those splits rather than in this APK.`, | ||
| ); |
There was a problem hiding this comment.
When a manifest says splits are required and the caller selects an ABI, appending only this unknown leaves inventory.status as complete. projectAndroidJniBindings at src/domain/android/androidJniBindings.ts:448-455 ignores inventory.unknowns, so with no base library—or a base library lacking a method—it emits an unmatched binding with complete coverage even though a missing split can contain the candidate; make this limitation participate in incomplete binding coverage.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 35c9cd5c6. libraryInventory.unknowns now participates in the incomplete test in projectAndroidJniBindings, so a split-required manifest widens the binding coverage to partial and the status to unknown instead of claiming complete/unmatched. src/domain/android/ tests pass (89).
| const fail = (): void => { | ||
| strings.push(null); | ||
| }; |
There was a problem hiding this comment.
When a string-pool offset or encoding is malformed, or the cumulative string budget is exhausted, fail() inserts null and the parser can still return ok; if that entry is the Android namespace, split attribute name, or split value, later code maps it to an absent declaration and the caller emits no inventory unknown. Preserve a decode/capacity limitation and fail or qualify the result when it affects the split facts.
AGENTS.md reference: AGENTS.md:L37-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed on the failure mode: a string-pool entry lost to a malformed offset or to the cumulative allowance can leave a split attribute's name, namespace or value null, and the caller then reports an absent declaration. Not fixed in this push — the qualification needs a distinct marker from readStringPool through to the attribute resolution, and I did not want to land that half-verified. I will follow up on this PR with the capacity/malformed distinction and a boundary test that exhausts the string allowance on a retained split value.
The eager map over every XML start element printed a DBG line to process stdout on each manifest read. That output prefixes the CLI JSON result and, over the default stdio MCP transport, interleaves non-protocol lines with JSON-RPC responses, so an otherwise successful inspection can become unparsable for the caller.
The library inventory records a split-required manifest as an unknown, because the native library a caller asks about may live in a split that is not part of this APK. Projection ignored that entry: the status stayed 'complete', so a declaration with no base library was reported as an unmatched binding at complete coverage. A missing split can still contain the candidate, so the binding has to stay partial.
noteUnbundledSplitLibraries was awaited after the scan's try/catch and before checkpoint construction and resources.release(owner). A cancellation observed while that helper opens or streams AndroidManifest.xml escaped both: the interrupted scan lost its completed-library checkpoint and the ZIP reader was left unclosed and unowned by the scope. Move the call inside the guarded block so the existing catch marks the inventory partial and the existing checkpoint and release stages still run.
An encrypted or non-regular AndroidManifest.xml member made noteUnbundledSplitLibraries return without recording anything, so the inventory stayed complete with no unknowns even though split declarations could not be ruled out. Record the entry kind as an unknown instead.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4937c1d1de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| : (strings[requiredFlag.data] ?? null) === "false" | ||
| ? false | ||
| : null | ||
| : null; |
There was a problem hiding this comment.
When a valid manifest encodes android:isSplitRequired="@bool/...", binary XML carries a TYPE_REFERENCE, so this fallback returns null; noteUnbundledSplitLibraries only qualifies coverage for literal true, allowing a reference that resolves true at install time to leave bindings unmatched with complete coverage even though the required split may contain the library. Resolve the reference or treat a present non-literal value as unknown rather than absent.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a3db6d91. is_split_required and required_split_types now return "unknown" when the attribute is present but carries no literal (a TYPE_REFERENCE, or any other typed value), and AndroidJniLibraries adds an inventory unknown for it. Absent stays null. Regression: reports a resource-reference isSplitRequired as unknown rather than absent plus the JNI-boundary counterpart.
| if (entry.path === "AndroidManifest.xml" && manifest === undefined) | ||
| manifest = entry; |
There was a problem hiding this comment.
For an APK with multiple root AndroidManifest.xml occurrences, this retains only the first; if that occurrence has no split declaration but a later occurrence requires splits (or is unreadable), no inventory unknown is added and JNI absence can still be reported as complete. The ZIP reader preserves duplicate occurrences, so detect this ambiguity or conservatively inspect all root-manifest occurrences instead of silently discarding the rest.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a3db6d91. The scan now tracks a repeated root AndroidManifest.xml member and adds an inventory unknown ("which split declarations apply is unknown") instead of reading only the first. Regression: leaves split declarations unknown when the APK holds two root manifests, built by writing two members and renaming one, since zip.js refuses a duplicate at write time.
| rows | ||
| .flatMap((row) => | ||
| row.resourceId === undefined ? [] : bytes32(row.resourceId), | ||
| ) |
There was a problem hiding this comment.
When an attribute supplies resourceId, this emits only the present IDs in attribute order, but an AXML resource map is indexed by string-pool index; here attribute names begin at index 2 while their IDs are written at slot 0. Consequently these fixtures never make resourceIds[nameIndex] match and the tests pass only through the name fallback, leaving the new resource-ID parser path unvalidated; serialize zero placeholders through each attribute name's string index.
AGENTS.md reference: AGENTS.md:L105-L105
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 700021855. The resource map was written in attribute order with only the IDs that existed, while the parser indexes it by string-pool index, so resourceIds[nameIndex] never matched and every ID-carrying case resolved through the name fallback.
The fixture now spans the pool with 0 in slots that carry no ID, and finds a split attribute by its resource ID rather than its name reads requiredSplitTypes under a different name so it can only pass through the ID. Setting resourceId: undefined in the parser makes that case and no other fail; I checked both directions. tests/boundary/android/ 339/339, tsc and prettier clean.
The other two threads are separate parser behaviour and I have not touched them in this push.
The fixture wrote only the IDs it had, in attribute order, so resourceIds[nameIndex] never lined up: the manifest parser indexes the map by string-pool index and every ID-carrying case silently resolved through the name fallback. Slot 0 is an empty string and attribute names start at index 2, so the first ID landed at slot 0. The map now spans the pool with 0 in slots that carry no ID, and a case reads requiredSplitTypes under a different name so it can only pass through the ID.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7000218550
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }; | ||
| if ( | ||
| chunk.chunkSize < 8 || | ||
| chunk.headerSize < 8 || |
There was a problem hiding this comment.
For a malformed APK whose RES_XML_START_ELEMENT advertises a header size of 8–15 bytes, this generic lower bound accepts the chunk even though the node header is 16 bytes. readStartElement then reads the name and counts at fixed offsets but shifts the attribute table by the advertised size, so a manifest containing split-required attributes can be returned ok with no matching attributes and the JNI inventory emits no unknown; validate the type-specific header minimum before decoding.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63dc1bde5. readStartElement now takes the chunk header size and returns null when it is below the 16-byte ResXMLTree_node, so the document fails as malformed instead of parsing as a with no attributes. Added "treats an undersized start-element header as unreadable rather than empty", which rewrites the one start-element header to 12 and asserts the malformed error; removing the guard makes that case and no other fail. tests/boundary/android/ 340/340, tsc and prettier clean.
…ests A RES_XML_START_ELEMENT whose header advertises 8-15 bytes passed the generic chunk bound even though ResXMLTree_node is 16 bytes. readStartElement reads the name at a fixed offset and shifts the attribute table by the advertised size, so a manifest carrying split-required attributes parsed as a start element with no matching attributes and the JNI inventory reported complete coverage with no unknown. Reject the chunk in readStartElement so the document is reported malformed, which the caller already qualifies as an unreadable manifest.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63dc1bde59
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const attributeCount = reader.u16(start + 28); | ||
| if (attributeSize < ATTRIBUTE_ROWS) return null; | ||
| const name = strings[reader.u32(start + 20)] ?? null; | ||
| const table = start + reader.u16(start + 2) + attributeStart; |
There was a problem hiding this comment.
When a start-element chunk has attributeStart below the 20-byte ResXMLTree_attrExt (for example 0) while attributeCount is nonzero, this points the table into the extension; the root still decodes as manifest, but its real requiredSplitTypes row is skipped and the function returns ok with all split facts absent. The JNI inventory then adds no unknown and can emit complete/unmatched coverage, so reject an overlapping table as malformed before reading attributes.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a3db6d91. readStartElement now rejects attributeStart < ResXMLTree_attrExt (20), so a table pointing into the node extension fails the read as malformed rather than decoding the root with every declaration absent. Regression: treats an attribute table that starts inside the node extension as malformed.
| kind: "malformed", | ||
| message: "A binary XML chunk declares a size outside the document.", | ||
| }); | ||
| chunks.push(chunk); |
There was a problem hiding this comment.
On an admitted 4 MiB manifest containing a valid pool/root plus many minimum-size chunks, this loop can retain about 524k JavaScript chunk objects before any manifest lookup; the later filter/map passes add more arrays, so the nominal byte cap expands into tens of megabytes and avoidable CPU even though the result only needs the root. Process chunks incrementally or impose a representation/count limit.
AGENTS.md reference: AGENTS.md:L57-L58
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a3db6d91. Chunk scanning moved to walkChunks, a visitor over declared chunk positions: the string pool, resource map and <manifest> occurrence are recorded by position and the walk stops at the root, so nothing proportional to chunk count is retained. Malformed extents still fail closed during the walk. The 4 MiB member cap is unchanged.
A typed resource reference carries no literal, so readAndroidBinaryManifestSplitFacts returned null for it and the JNI inventory read the declaration as absent: an isSplitRequired that resolves true at install could leave native bindings reported complete. Report a present non-literal declaration as unknown and let the inventory qualify coverage for it. The same pass fails closed on an attribute table that starts inside the ResXMLTree_attrExt extension, and walks chunks through a visitor instead of retaining every one for a single root lookup. A repeated root AndroidManifest.xml member is now an explicit unknown: the installer picks one occurrence, which this reader cannot.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3db6d911e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (element?.name !== "manifest") return true; | ||
| manifest = element; | ||
| return false; |
There was a problem hiding this comment.
When a bundle-generated base APK puts android:isSplitRequired="true" on its <application> element, this visitor stops as soon as it finds the root <manifest> and the later lookup considers only that root's attributes. Such an APK therefore contributes no library-inventory unknown, so a native declaration whose .so is in a required split can still be projected as unmatched with complete coverage; retain the application start element and its uncertainty as well as the root split-type attributes.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| chunk.chunkSize < 8 || | ||
| chunk.headerSize < 8 || | ||
| start + chunk.chunkSize > declared |
There was a problem hiding this comment.
When an AXML chunk advertises headerSize > chunkSize, this predicate accepts it. In particular, an oversized resource-map header makes the resource-ID loop produce no IDs; an Android split attribute identified by its framework ID but not its canonical name then disappears, and JNI coverage remains complete instead of treating the manifest as malformed. Add headerSize <= chunkSize to the structural validation.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
Fixes #9756
inspect-android-jni-bindingsreportedlibrary_inventory.status: "complete"withselected_libraries: 0for a base APK whose manifest requires ABI and density splits, even though the native libraries live in those splits. The unqualifiedcompletelet an agent conclude the app has no native code while DEX still listed native declarations.What changed
src/android/AndroidBinaryManifestSplitFacts.ts: a bounded reader for the admitted APK's binaryAndroidManifest.xml(RES_XMLchunks, UTF-16/UTF-8 string pool, resource map, and the<manifest>start element). It resolvesandroid:requiredSplitTypesandandroid:isSplitRequiredby framework resource ID, falling back to the attribute name.readAndroidJniLibrariesrecords theAndroidManifest.xmlmember during its existing single entry pass and, after the library scan, adds one explicit inventory unknown when the manifest declares required split types orisSplitRequiredtrue. A manifest that cannot be read, exceeds the 4 MiB read allowance, or cannot be parsed produces its own unknown rather than a barecomplete.AndroidManifest.xml, keep the same inventory as before.Out of scope, as the issue states: accepting a split set as input (#1813 / #3495). This only stops a base APK from claiming complete library coverage when its own manifest says libraries may live elsewhere.
Validation
npm run check:fast(typecheck + lint): passed.npx oxfmt --checkon all changed files: passed.npx vitest run tests/boundary/android/ src/domain/android/ src/android/: 436 passed, 1 skipped.tests/boundary/android/AndroidBinaryManifestSplitFacts.test.ts(5) and 4 added cases intests/boundary/android/androidJniLibraries.test.ts(15 total in that file).Not run
0x0101064c/0x0101064d/0x01010549are from published framework resources but were not re-verified against a device framework in this environment; the attribute-name fallback covers a mismatch.Author readiness
Human owner: @2SinceJuly