fix: fix-apk-selector (WPB-25905)

[emmaoke-w]

https://wearezeta.atlassian.net/browse/WPB-25905

---

PR Submission Checklist for internal contributors

• The PR Title

  • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
  • contains a reference JIRA issue number like SQPIT-764
  • answers the question: If merged, this PR will: ... ³

• The PR Description

  • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

---

What's new in this PR?

• Fixes QA Android CI APK selection so latest and automatic upgrade fallback use S3 upload recency instead of the wrapped 5-digit APK suffix.

Issues

Briefly describe the issue you have solved or implemented with this pull request. If the PR contains multiple issues, use a bullet list.

Causes (Optional)

Briefly describe the causes behind the issues. This could be helpful to understand the adopted solutions behind some nasty bugs or complex issues.

Solutions

Briefly describe the solutions you have implemented for the issues explained above.

Dependencies (Optional)

If there are some other pull requests related to this one (e.g. new releases of frameworks), specify them here.

Needs releases with:

• GitHub link to other pull request

Testing

Test Coverage (Optional)

• I have added automated test to this contribution

How to Test

Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.

Notes (Optional)

Specify here any other facts that you think are important for this issue.

Attachments (Optional)

Attachments like images, videos, etc. (drag and drop in the text box)

---

PR Post Submission Checklist for internal contributors (Optional)

• Wire's Github Workflow has automatically linked the PR to a JIRA issue

---

PR Post Merge Checklist for internal contributors

• If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

---

References

1. https://sparkbox.com/foundry/semantic_commit_messages
2. https://github.com/wireapp/.github#usage
3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[yamilmedina]

@claude review

[github-actions]

Claude review — PR 4871 (fix: APK selector / WPB-25905)

Scope: ~80 lines across two CI files. Intent is correct (use S3 LastModified instead of the wrapping 5-digit suffix), backward compatibility with the previous JSON shape is preserved. A few things worth a second look before merge.

---

1. Behaviour change in the direct-path upgrade branch (most important)

In select_apks.py around lines 111-122, when appBuildNumber ends in .apk and contains a slash, new_key is set to the user-supplied string directly without being looked up against the S3 listing. If is_upgrade=true and old_input is empty, the code now calls previous_key(new_key), which does ordered_keys.index(new_key). If the typed path does not match an entry verbatim (extra slash, casing, object rotated out of the prefix), the function returns None and the upgrade flow fails with ERROR: Upgrade requested but OLD apk could not be resolved.

Previously second_latest_key (version-based) provided a stable fallback in exactly this case. This is a real regression risk for the manual direct-path workflow.

Suggested fix: if previous_key(new_key) returns None in the direct-path branch, fall back to ordered_keys[-2] when at least two entries are available — that is the recency analogue of the previous second_latest_key. Or normalise new_key against ordered_keys (lookup by basename) before calling previous_key.

---

2. previous_key uses list.index, which returns the first match

select_apks.py:131-136 — if two entries somehow share the same key (should not normally happen, but pick_by_substring can produce surprises with overlapping filenames), index() silently returns the earliest. Minor — worth being aware of, not a blocker.

---

3. Non-deterministic ordering when LastModified ties

select_apks.py:122 sorts purely by last_modified. S3 timestamps have second resolution and the CI publish step uploads multiple APKs in quick succession. Two APKs with identical timestamps are ordered by their original position in apk_entries (Python sort is stable), which is the S3 listing order — alphabetical by key. That is usually what you want, but make it explicit by adding the filename as a secondary sort key. Documents the tiebreaker and makes runs reproducible across SDK versions and pagination orderings.

---

4. Silent parse failures hide CI issues

select_apks.py:25-32 — parse_last_modified returns None on any ValueError. If AWS ever changes the timestamp format, the script silently falls back to version-based ordering (i.e. the bug this PR is fixing comes back) without any signal. Suggest logging a stderr warning when parsing fails — CI logs are the only diagnostic surface for this script, so it is cheap insurance.

---

5. JMESPath payload size (optional polish)

execution_setup.sh:85 — query changed from selecting just .Key to selecting the entire object. The file now also includes ETag, Size, StorageClass, Owner etc. for every APK. Functionally harmless. If you want to keep the file small, project just the two fields the Python script actually consumes (Key and LastModified) in the JMESPath query.

---

6. Test coverage

The PR checkbox says no automated tests were added, and select_apks.py now has measurable logic (recency vs version vs insertion fallback, plus the previous_key lookup). The Python module is self-contained (env vars in, stdout out), so a small pytest file would be high-leverage. Suggested cases:

• empty data → exits with error
• legacy string-only data → falls back to version sort
• objects with LastModified → recency wins over version
• mixed entries (some missing LastModified) → only timestamped entries participate in recency, rest fall through to version
• is_upgrade=true, old_input empty, app_build=latest → previous_key returns the second-most-recent
• direct-path branch with a path not in the listing (the regression in §1)
• LastModified tie → deterministic tiebreaker

These do not need fixtures beyond a temp dir with a JSON file, so they fit the existing CI shape.

---

7. Minor / nits

• apk_entries items are loose dicts — a NamedTuple or @dataclass would document the shape and let static checkers catch typos. Not required.
• The fallback chain in ordered_keys (select_apks.py:124-128) is clear, but a single-line comment naming the three tiers (recency → parsed-version → listing order) would help the next reader debugging a CI miss.
• The new behaviour means the QA upgrade test will now exercise the previously uploaded build, not the previous semantic version. That is the right call given the bug, but worth flagging to the QA team so they are not surprised when an upgrade run pairs e.g. a hotfix APK with the build immediately before it rather than the prior minor release.

---

Summary

Area	Status
Code quality	Good — clear fallback chain, defensive parsing
Correctness	One regression risk in the direct-path branch (§1)
Performance	N/A
Security	No concerns — CI scripts only
Test coverage	Missing — select_apks.py would benefit from pytest cases

Nothing here is a hard blocker, but §1 is worth addressing because it silently weakens an existing fallback that operators rely on for manual upgrade runs.