Skip to content

Commit 17ffc0c

Browse files
committed
feat(gh60): parseArraysXml support integer-array + array, G6.4 pulled from C2 (refs #60)
Pulls G6.4 forward from the C2 hardening package by the same precedent as Group 11 (hint/text fix): same file already being modified, ≤5-LOC patch, mirror tests trivial against existing XmlInputTypeTest infra. Pre-fix, parseArraysXml only iterated <string-array> tags. Spinners referencing an <integer-array> or generic <array> via android:entries="@array/foo" got entries=[] in the JSON. Empirical 30-APK sample from JCA-400 (decoded with apktool 2.10.0): <string-array> only: 25 APKs (83%) <array> generic: 5 APKs (17%) <integer-array>: 0 APKs (0%) Cryptoapp source declares <array name="messageDigestAlgorithms"> but the pre-fix baseline already showed the spinner entries populated -- investigation revealed apktool normalizes <array> -> <string-array> during decode when all items are strings. GATOR only sees the decoded XML, never the source. So the real-world uplift of G6.4 is smaller than the proposal originally suggested: the 5 generic-<array> cases that survive normalization tend to be empty placeholders (e.g. net.aliasvault.app's crypto_fingerprint_fallback_prefixes is <array name="X" />). Fix is still worth landing because (a) the patch is ≤5 LOC, (b) unit tests pin behavior so a future apktool upgrade producing <integer-array> more often automatically benefits, (c) the redundancy is harmless when apktool produced <string-array>. 3 new unit tests in XmlInputTypeTest cover the three tag kinds and the all-coexist case. Client suite now 141/141 PASS (was 138 + 3 = 141). Cryptoapp smoke unchanged: (16, 106, 55, 32, 21) — zero regression. This commit also resolves three task-marking issues from the previous honesty pass: - Marks 7.6 (C1g JimpleDefUtils commit 355e4ef) as [x]; the work was factually delivered on 2026-05-25 and my prior excuse of "wasn't me" was lazy. - Kills 9.10 (Open PR); operator decided to push directly to origin/modules with no PR. Cross-LLM review was performed pre-merge per docs/20260515_plano_gator_targets_generic.md §9. - Rewrites 9.11 to remove the "after PR merged" precondition; archive can run anytime now (gh61 precedent: chore(gh61) archive without PR). See design.md D13 for the full apktool-normalization investigation + empirical 30-APK sample. Out of scope (still in C2): cache, menu superclass walk, dead-code expansion, WidgetType drift warn, dual package emission.
1 parent 93586a7 commit 17ffc0c

5 files changed

Lines changed: 175 additions & 19 deletions

File tree

rv-android/openspec/changes/gh60-targets-core/design.md

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,34 @@ Triggered by the D11 investigation. After rebuilding `lib/gator/rvsec-analysis-c
293293

294294
**Out of scope (follow-up):** multi-APK baseline (cryptoapp is a 16-class toy; Compose/R8/lambda-heavy apps need separate fixtures); sweep gate execution on the full 380-APK corpus (Group 9.3/9.4). `modules/rv-agent/` fixtures NEVER touched per CLAUDE.md deprecation policy.
295295

296+
### D13 — `parseArraysXml` covers all three array tag kinds (G6.4 pulled forward from C2, 2026-05-26)
297+
298+
`parseArraysXml` historically called `doc.getElementsByTagName("string-array")` exclusively — `<integer-array>` and `<array>` resource forms were ignored. Any `android:entries="@array/foo"` reference where `foo` was declared as `<integer-array>` (numeric pickers, color palettes, ID lists) or `<array>` (mixed @drawable/@string/@dimen items) silently produced `entries=[]` in the JSON. The agent's spinner-selection logic then saw an empty inventory and skipped those widgets — a quiet false-negative in UI exploration coverage.
299+
300+
The original scope decision (proposal.md §Follow-up Changes) put this in C2 alongside other hardening items (cache, menu superclass walk, dead code expansion). Two arguments for pulling it forward into gh60:
301+
302+
1. **Same-file precedent.** Group 11 already pulled the hint/text fix from "out of scope" into gh60 under the rationale "we are already in this file". The same logic applies: `parseArraysXml` lives in `RvsecAnalysisClient.java` immediately below the `enrichFromElement` Group 11 touched. The patch is ≤5 LOC: replace one `getElementsByTagName` call with a loop over three tag names.
303+
304+
2. **Trivial test coverage.** `XmlInputTypeTest` already exercises `parseArraysXml` for `string-array` (`testParseArraysXmlPlainItems`, `testParseArraysXmlWithStringRefs`). Mirroring three more cases (`integer-array`, generic `array`, all-three-coexist) is mechanical and follows the existing fixture pattern.
305+
306+
Implementation: keep the existing per-item handling (text content + `@string/` resolution) verbatim — `<integer-array>` items are stringified naturally (`"42"`), `<array>` items pass through verbatim or via `@string/` if applicable, downstream consumer (`widget.entries: List<String>`) is unaffected.
307+
308+
**Cryptoapp baseline impact: none — but for a non-obvious reason.** The cryptoapp source XML at `examples/cryptoapp/app/src/main/res/values/arrays.xml` declares `<array name="messageDigestAlgorithms">` (generic `<array>`, NOT `<string-array>`). Yet the pre-G6.4 baseline already showed the spinner's entries populated with all 13 algorithms. Investigation revealed: apktool decodes the binary APK's resource table into `<string-array>` for any array whose items are all strings — the source-XML distinction `<array>` vs `<string-array>` is lost during the build → apktool round-trip. GATOR only sees the decoded XML, never the source. So cryptoapp's `<array>` was already showing up to `parseArraysXml` as `<string-array>`.
309+
310+
**Empirical 30-APK sample from JCA-400 (2026-05-26):**
311+
312+
| Tag observed in decoded XML | APKs (n=30) | Coverage notes |
313+
|------------------------------|-------------|----------------|
314+
| `<string-array>` only | 25 (83%) | apktool normalized any source `<array>` to `<string-array>` |
315+
| `<array>` (generic) | 5 (17%) | apktool preserved generic — items mix non-string types |
316+
| `<integer-array>` | 0 (0%) | rare in modern Android UI resources |
317+
318+
The 5 generic-`<array>` cases need separate verification on whether the items are actually referenced by Spinners' `android:entries`; the ones inspected (e.g. `net.aliasvault.app_2702900.apk` has `<array name="crypto_fingerprint_fallback_prefixes" />`) are empty placeholders, so the practical coverage uplift is smaller than 17%. The fix is still worth landing because (a) the patch is ≤5 LOC, (b) unit tests pin behavior so a future apktool upgrade producing `<integer-array>` more often automatically benefits, (c) the redundancy is harmless when apktool already produced `<string-array>`.
319+
320+
**Decision rationale:** the G6.4 entry in the C2 hardening list assumed source-XML semantics (where the three tags are distinct) without accounting for the apktool normalization layer. The 30-APK empirical sample shows the real-world uplift is small but non-zero; given the same-file proximity and zero-cost test maintenance, pulling it forward into gh60 is justified, but the C2 author should NOT cite "fixes silently-empty spinner inventories" as motivation for the broader package — that motivation barely survives empirical scrutiny.
321+
322+
**Out of scope (still in C2):** G6.2 (`resolveStringReference` cache), G6.3 (`findOnCreateOptionsMenu` superclass walk), G6.5 (dead-code expansion), G6.6 (`WidgetType` drift warn log), G11 (dual `manifestPackage`/`codePackage` emission). Pulling G6.4 alone is justified by the same-file proximity; pulling everything else means re-doing the multi-LLM convergence that produced the 3-change split.
323+
296324
## API Design
297325

298326
### Java: `TargetMethodSource`

rv-android/openspec/changes/gh60-targets-core/proposal.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ GitHub Issue: #60. The `RvsecAnalysisClient` in `rvsec-gator` is a 1625 LOC god
2222
- **Sweep post-merge** — all 380 APKs reprocessed; no legacy `*Mop` JSONs preserved.
2323
- **Widget hint/text inline-literal coverage (gap fix, 2026-05-26)**`enrichFromElement` extended with `android:hint` and `android:text` attribute reads. Path-A (`PropertyManager`) only sees call-graph-tracked strings and `@string/` refs; path-B (gh57 attribute pass) historically excluded these two fields. Result on cryptoapp pre-fix: 0/51 widgets populated for hint/text despite 4 hint + 17 text declarations in the source XML. Fix is idempotent (existing seed preserved when XML carries no literal). See `design.md` D11.
2424
- **Reachability parity gate hardening (post-investigation, 2026-05-26)** — D11 surfaced a deeper bug: `G_paridade_reachability` / `G_paridade_targets` / `G_sentinela_complete` had been silently passing for two months because (a) both sides of the comparison (in-tree baseline + `/tmp/gh60_g_subset/lenient.json` cache) reflected the same pre-gh51 cha-default era; (b) the cache had no `mtime(jar)` invalidation; (c) `pytest.skip` swallowed RVSEC_HOME-less runs as "passed". Bisect with `b2e04a26` worktree proved gh60 is byte-equivalent in reachability output to its pre-merge parent — the 67/61 → 55/32 drop is the intended gh51 D5 cha→spark precision improvement, never reflected in the fixture. Fix: shared `_lenient_cache.ensure_fresh_lenient` helper invalidates cache on jar change; regenerated baseline with current jar; new `test_baseline_freshness.py` tripwire (schema + mtime); `RV_GATOR_REQUIRED=1` env-var contract converts silent skip to fail. Cross-check against historical `/home/pedro/desenvolvimento/RV_ANDROID/ALL_METHODS/cryptoapp.apk.methods` confirms structural method coverage parity. See `design.md` D12.
25+
- **`parseArraysXml` supports `<integer-array>` and `<array>` (G6.4 pulled from C2, 2026-05-26)**`parseArraysXml` historically iterated only `<string-array>` tags, so Spinners whose `android:entries="@array/foo"` referenced an `<integer-array>` (numeric pickers, color palettes) or a generic `<array>` (mixed-resource lists) got `entries=[]` in the JSON. Pulled forward from the C2 hardening package under the same precedent as Group 11: same file already being modified, ≤5-LOC patch, mirror tests trivial against the existing `XmlInputTypeTest` infrastructure. Cryptoapp itself has neither resource type so the baseline is unaffected; new behavior is pinned by 3 unit tests covering each tag kind + the all-kinds-coexist case. See `tasks.md` Group 12.
2526

2627
## Capabilities
2728

rv-android/openspec/changes/gh60-targets-core/tasks.md

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@
161161
- [x] 7.3 Add `JimpleDefUtilsTest.java` covering the three methods with synthetic Jimple inputs (14 cases: single-def / multi-def via if-else / no-def for `definitionRhs`; direct constant + local-walk + wrong-type + empty-string variants for `resolveInt`/`resolveStr`; utility-class shape assertion)
162162
- [x] 7.4 Verify existing `MenuExtractor` and `SpinnerItemExtractor` tests remain green — client unit suite 129/129 PASS after refactor
163163
- [x] 7.5 Coverage of `JimpleDefUtils` — qualitative: every branch of every method exercised by `JimpleDefUtilsTest` (jacoco not wired in `rvsec-gator-parent/pom.xml`; documenting coverage via test inventory in 7.3 rather than adding a build-system dependency for a 60-line utility); `G_jimple_def_utils` (zero private duplicates in extractor classes) verified by grep
164-
- [ ] 7.6 Commit `refactor(gh60): C1g extract JimpleDefUtils (refs #60)`
164+
- [x] 7.6 Commit `refactor(gh60): C1g extract JimpleDefUtils (refs #60)`
165165

166166
## 8. C1h — DROPPED (Phase 1 task-zero verdict, 2026-05-25)
167167

@@ -191,8 +191,8 @@ Atomic write + two-stage parser read removed from scope. Empirical basis: 826/82
191191
- [x] 9.7 Run `openspec validate "gh60-targets-core"` — must pass structural validation
192192
- [ ] 9.8 Invoke `/rv-code-reviewer` via Skill tool on the diff vs `master`; address any high-confidence findings
193193
- [ ] 9.9 Run `/rv-docs-sync rv-static-analysis` and `/rv-docs-sync rv-android-core` to update CLAUDE.md and architecture.md for the renamed components and new abstractions
194-
- [ ] 9.10 Open PR; reference issue #60; ensure PR body includes: `Closes #60`, gates table with green status, sweep outliers (if any), and link to the Phase-0 ideation doc
195-
- [ ] 9.11 After PR merged: run `openspec instructions apply --change "gh60-targets-core"` and `/opsx:verify gh60-targets-core` (Phase 5); `/opsx:archive gh60-targets-core` (Phase 6) — syncs deltas to main specs and archives the change.
194+
- [x] 9.10 ~~Open PR~~**KILLED 2026-05-26 by operator decision**. Work pushed directly to `origin/modules`; no PR will be opened for this change. Rationale: integration with downstream branches happens on the `modules` working branch; PR-style review was already performed pre-merge via the cross-LLM convergence documented in §9 of `docs/20260515_plano_gator_targets_generic.md`.
195+
- [ ] 9.11 Run `/opsx:verify gh60-targets-core` (Phase 5) and `/opsx:archive gh60-targets-core` (Phase 6) — syncs deltas to main specs and archives the change. No "after PR merged" precondition (PR killed in 9.10); archive can run whenever the operator is satisfied with the implementation. Same pattern as `chore(gh61): archive change` (2026-05-26 12:21 reflog entry) which archived gh61 without a PR.
196196

197197
## 10. Follow-up tracking — open issues for C2 and C3 (NOT implementation tasks)
198198

@@ -270,3 +270,27 @@ After C1 merges, open placeholder issues in GitHub PAMunb/rvsec using `docs/2026
270270
- Multi-APK baseline (replace mono-cryptoapp with 5-10 representative APKs). Cryptoapp is a 16-class toy — bugs only manifesting in larger/Compose/R8 corpora are invisible.
271271
- Sweep gate execution (Group 9.3/9.4 G4 ≥80% complete=true on 380 APKs). Comparator written + unit-tested; sweep itself is a multi-hour operator job.
272272
- `modules/rv-agent/` fixtures — **NOT touched** under any circumstance; rv-agent is deprecated per CLAUDE.md (explicit policy).
273+
274+
## 12. C1-fix — `parseArraysXml` covers `<integer-array>` and `<array>` (G6.4 pulled forward from C2, 2026-05-26)
275+
276+
<!-- Originally scoped to C2 (proposal.md §Follow-up Changes G6.4). Pulled
277+
into gh60 by the same precedent as Group 11: (a) touches the same
278+
RvsecAnalysisClient.java file we just modified, (b) trivial patch
279+
(≤5 LOC extension of an existing loop), (c) test infrastructure for
280+
XmlInputTypeTest already covers @array/ entry resolution, so a
281+
mirror test for integer-array/array is mechanical.
282+
283+
Pre-fix, Spinners whose `android:entries="@array/foo"` referenced an
284+
<integer-array> or generic <array> got entries=[] in the JSON. Apps
285+
declaring color palettes, numeric pickers, or generic value lists
286+
via these resource forms had silently empty spinner inventories. -->
287+
288+
- [x] 12.1 Extend `parseArraysXml` (`RvsecAnalysisClient.java:1158-1191`) to iterate `<string-array>`, `<integer-array>`, and `<array>` instead of `<string-array>` only. The per-item handling (text content + `@string/` resolution) stays identical — `<integer-array>` items are stringified, `<array>` items pass through verbatim or via `@string/` if applicable. Idempotent: empty XML or arrays-of-other-tags are no-ops.
289+
- [x] 12.2 Add 3 unit cases to `XmlInputTypeTest.java`:
290+
- `testParseArraysXmlIntegerArray``<integer-array name="ids"><item>1</item><item>42</item></integer-array>``arrays["ids"] == ["1", "42"]`
291+
- `testParseArraysXmlGenericArray``<array name="mixed"><item>literal</item><item>@string/foo</item></array>` + matching `strings.xml` → resolved values present
292+
- `testParseArraysXmlAllKindsCoexist` — file containing all 3 array tags → all 3 keys present in map, values isolated per name
293+
- [x] 12.3 Build + deploy: `cd rvsec-gator && mvn -pl client -am install -DskipTests=true`. Verify jar mtime advances.
294+
- [x] 12.4 Run client unit suite: `mvn -pl client test -Dtest='!*IT' ...`. Baseline pre-12.x is 138/138; post-12.x expectation is 138 + 3 new = 141 PASS.
295+
- [x] 12.5 Smoke validation on cryptoapp pos-fix: confirmed regression-free — baseline `(16, 106, 55, 32, 21)` matches post-G6.4 smoke `(16, 106, 55, 32, 21)`, and `spinnerMessageDigest` entries identical (13 algorithms). **Discovery during validation:** cryptoapp source uses `<array name="messageDigestAlgorithms">` (generic, not string-array) — yet entries were already populated pre-G6.4. Root cause: apktool normalizes `<array>``<string-array>` during decode when items are strings, and GATOR only sees the decoded XML. So the cryptoapp case is structurally handled even pre-G6.4. The fix's real uplift was measured on a 30-APK JCA-400 sample: 0 with `<integer-array>` (0%), 5 with generic `<array>` (17%), 25 string-array-only (83%). The 5 generic-array cases need per-APK inspection on whether the items are Spinner-referenced; ones inspected are empty placeholders, so practical uplift < 17%. Documented in design.md D13.
296+
- [ ] 12.6 Commit `feat(gh60): parseArraysXml support integer-array + array (G6.4 from C2) (refs #60)`. Body: dual rationale (same-file precedent + apps using color-palette/numeric pickers no longer silently empty); cross-reference proposal.md C2 follow-up list (entry now done).

rvsec/rvsec-android/rvsec-gator/client/src/main/java/presto/android/gui/clients/RvsecAnalysisClient.java

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1165,24 +1165,40 @@ Map<String, List<String>> parseArraysXml(String resDir) {
11651165
DocumentBuilder builder = factory.newDocumentBuilder();
11661166
Document doc = builder.parse(arraysFile);
11671167

1168-
NodeList arrayNodes = doc.getElementsByTagName("string-array");
1169-
for (int i = 0; i < arrayNodes.getLength(); i++) {
1170-
Element arrayElem = (Element) arrayNodes.item(i);
1171-
String name = arrayElem.getAttribute("name");
1172-
List<String> items = new ArrayList<>();
1173-
1174-
NodeList itemNodes = arrayElem.getElementsByTagName("item");
1175-
for (int j = 0; j < itemNodes.getLength(); j++) {
1176-
String value = itemNodes.item(j).getTextContent().trim();
1177-
// Resolve @string/ references
1178-
if (value.startsWith("@string/")) {
1179-
String resolved = resolveStringReference(resDir, value.substring("@string/".length()));
1180-
items.add(resolved != null ? resolved : value);
1181-
} else {
1182-
items.add(value);
1168+
// Iterate all three resource-array kinds Android supports.
1169+
// <string-array> covers most cases (textual entries for Spinners);
1170+
// <integer-array> shows up on numeric pickers / color-palette
1171+
// indices; generic <array> is the catch-all for mixed
1172+
// @drawable/@string/@dimen lists. Pre-2026-05-26 this loop only
1173+
// looked at <string-array>, so any spinner whose
1174+
// android:entries="@array/foo" referenced an <integer-array> or
1175+
// <array> resource silently got entries=[] and the agent's
1176+
// inventory missed it. See design.md §D13.
1177+
String[] arrayTagNames = {"string-array", "integer-array", "array"};
1178+
for (String tagName : arrayTagNames) {
1179+
NodeList arrayNodes = doc.getElementsByTagName(tagName);
1180+
for (int i = 0; i < arrayNodes.getLength(); i++) {
1181+
Element arrayElem = (Element) arrayNodes.item(i);
1182+
String name = arrayElem.getAttribute("name");
1183+
List<String> items = new ArrayList<>();
1184+
1185+
NodeList itemNodes = arrayElem.getElementsByTagName("item");
1186+
for (int j = 0; j < itemNodes.getLength(); j++) {
1187+
String value = itemNodes.item(j).getTextContent().trim();
1188+
// @string/ resolution applies uniformly — an
1189+
// <array> can legitimately mix literal values with
1190+
// @string/ refs; <integer-array> items are
1191+
// numeric literals so the @string/ branch is a
1192+
// harmless no-op there.
1193+
if (value.startsWith("@string/")) {
1194+
String resolved = resolveStringReference(resDir, value.substring("@string/".length()));
1195+
items.add(resolved != null ? resolved : value);
1196+
} else {
1197+
items.add(value);
1198+
}
11831199
}
1200+
arrays.put(name, items);
11841201
}
1185-
arrays.put(name, items);
11861202
}
11871203
} catch (Exception e) {
11881204
// Graceful degradation

0 commit comments

Comments
 (0)