Conversation
…istically
The `parseArgs()` test that called parseArgs() with no argument and
expected the usage-error envelope was order-dependent on whatever
flags Jest itself was launched with. Under bare `npx jest` the
argv slice has 2 trailing entries ("jest" + the test path), which
falls through to the same usage-error branch — but as soon as the
launcher passes a third argument (e.g. `npx jest ... --coverage`
or `npx jest ... --forceExit`), parseArgs() would return
"Invalid package_id" instead, and the assertion silently shifted
its target.
Pin the default-branch contract explicitly by stubbing
process.argv with jest.replaceProperty (auto-restored after the
test), and exercise both:
- empty argv → usage error (the original test's contract)
- three valid args → parsed normally through the same fallback
path (the deterministic branch the original test was trying
to pin but couldn't reach reliably)
parseArgs itself is unchanged: its default-to-process.argv
behavior is the right production behavior for the actual CLI
entrypoint, where real argv is what callers want.
Verified with the requested command:
npx jest src/download/__tests__/cli-args.test.js \\
--forceExit --coverage --coverageReporters=text
462 tests pass, lint clean. cli-args.js still at 100% coverage on
every metric.
Jest's coverageThreshold is only checked when the runner has
--coverage enabled. The previous "test": "jest --forceExit" never
set that flag, so the four configured thresholds (src/errors,
src/apk/candidate.js, src/archive, src/download) were documented
intent but never enforced — CI's `npm test` would have run
through happily even if a contributor let coverage drop below the
gate.
Add --coverage to the test script. Local `npm test` and CI's two
calls to `npm test` (ci.yml:102 and ci.yml:164, both the PR and
main-branch gates) now run the same flag, so the threshold rule
is enforced everywhere a developer or CI could regress it.
Also add coverage/ to .gitignore — Jest's HTML report directory
was previously untracked-but-not-ignored, leaving it as noise in
`git status` after every local run.
Verified thresholds pass on the current state:
./src/errors/ 100% stmts / 95.83% branch / 100% / 100%
(≥90 on every metric)
./src/apk/candidate.js 100% / 100% / 100% / 100% (≥90)
./src/archive/ 100% / 100% / 100% / 100% (≥85/90/90/90)
./src/download/ 97.94% / 94.20% / 100% / 98.52% (≥90)
`npm test` exits 0 with the flag set, no threshold errors.
462 tests still pass, lint clean.
Audit item #5: verify whether the directory-scan selection (findPackageCandidate, bestRankedApkInDir) honored config.json's preferred_arch after the migration to rank-candidates.js#selectCandidate. Investigation result: NO. The legacy regex scoreApk() that these functions replaced used a hardcoded weight table arm64-v8a +800, x86_64/x86 -600, armeabi-v7a -300 with no preferred_arch parameter at all. The current selectCandidate call deliberately drops the preferredArchitecture field (it passes only { packageName: 'directory-scan' }), so the new ranking reproduces the legacy fixed arm64-first behavior — the ARCHITECTURE_SCORE table puts arm64-v8a=100 ahead of armeabi-v7a=60, x86_64=40, x86=20, and the comparator's preferredArch bonus is gated on preference.preferredArchitecture, which is undefined here, so it never fires. Where preferred_arch actually shows up: download-supported-apk.js post-validation — apkHasNativeLibsForArch in the BUNDLE-vs-single-APK preference block (line 635) and the post-merge ABI check (line 743, via validateDownloadedApkAbi). The dir-scan keeps its fixed bias; operators who pin a non-arm64 preferred_arch rely on the guardrail to reject a wrong-arch APK and re-trigger a fallback. Strengthen the in-code comment to make this contract explicit and add a test that pins the arm64-first outcome in a non-obvious fixture (arm64-v8a vs armeabi-v7a with a "universal" suffix that might look like it should win). A future contributor who threads preferred_arch into the dir-scan will hit this test and the updated comment, both of which are the tripwire for the intentional behavior change. 463 tests pass, lint clean, all four coverage thresholds green.
…urces Two distinct leaks of the same class as the runCommand leak fixed in commit ed7b613: a setTimeout whose handle is either lost or only cleared on the happy path. (a) verifyUrl (~L275) — the AbortController timeout was cleared inline AFTER `await fetch`, but only when fetch succeeded. On rejection (network error, AbortError from a real timeout firing) or on the `return isValid` short-circuit, the timer fired ~urlVerify (5s) ms later and the closure stayed alive. Fix: move clearTimeout into a finally block. clearTimeout is a no-op when the timer already fired, so this is safe in every branch (success, fetch rejection, real-timeout abort). (b) parallelResolveSources (~L703) — the per-source SOURCE_TIMEOUT setTimeout was anonymous; the handle was never captured. A fast apkeep resolution (~ms) left the 60s SOURCE_TIMEOUT timer armed for the full window because Promise.race resolves on the source winning, leaving the rejected-promise side garbage-collected but the setTimeout handle dangling. Fix: capture the timer handle inside the timeoutPromise constructor and clear it in a finally block after Promise.race settles. Same caveat: clearTimeout is a no-op when the timeout fired (timeout-rejected race), so the finally runs unconditionally on both branches. Tests added in __tests__/unified-downloader-timers.test.js: verifyUrl: - clears timer on success (response.ok=true) - clears timer on fetch rejection (the bug-pin) - clears timer on non-ok response (regression guard for the success-path-only inline clearTimeout if a future refactor moves it back into the try) parallelResolveSources: - fast source wins + hung siblings: clearTimeout is observed via spy on global.setTimeout/clearTimeout (the only way to exercise the per-source cleanup without driving the allSettled wait past the hung sources' 60s timer). - all sources return a winner: jest.getTimerCount() === 0 after settle, the symmetric "all paths clear" pin. - all sources reject: jest.getTimerCount() === 0 after the "All sources failed" throw. verifyUrl is added to module.exports alongside runCommand with a comment explaining the test-only export. 469 tests pass (was 463), lint clean, all four coverage thresholds still green. The new test file exercises unified-downloader.js's hot-path branches without requiring a full download() round-trip.
The README said "first valid URL wins" but the code used
Promise.allSettled, which waited for EVERY source to settle (or
time out at SOURCE_TIMEOUT=60s) before returning the
priority-ordered winner. A hung apkmirror-html scrape would delay
an already-won apkeep result by up to 60s.
Design decision: keep the FIXED PRIORITY ORDER
(apkeep → apkmirror-api → apkmirror), don't use Promise.any.
Rationale: the existing fallback-chain tests pin apkeep-first
ordering (apkeep wins when it succeeds even if apkmirror-api
resolves faster), and that ordering is the canonical semantic —
apkeep is the canonical APKPure resolver; the APKMirror paths
exist for Cloudflare bypass. Losing priority for speed would
silently regress the documented behavior. The bug is the WAIT,
not the priority.
Implementation: kick off every source's Promise.race against
its own SOURCE_TIMEOUT up front (parallel I/O), then iterate
the resulting promises IN PRIORITY ORDER with sequential awaits.
The first source to settle with a valid URL wins; on failure we
fall through to the next. The lower-priority promises that are
still running are abandoned at function exit — their timers
were already cleared by their per-source finally block (commit
93c42e0).
Two key correctness details:
1. Each per-source promise catches its own rejections into a
`{__rejected: true, ...}` sentinel. Without this, an
abandoned promise (its source ran in parallel but we
returned early on a higher-priority winner) would surface
as an unhandled rejection when the rejection finally landed
on the event loop. Promise.allSettled never had this
problem because it absorbs all rejections.
2. The [parallel-resolve] log lines for "Winner" and "<src>
failed" are preserved with the same prefixes the existing
log-filter regexes in pre_download_apks.sh expect
(lines 145 and 187).
Tests in fallback-chain.test.js:
Existing (4 tests updated to reflect the new priority-first
behavior; call-count assertions relaxed where they were
incidentally pinned by the old allSettled shape):
- "apkmirrorapi wins when apkeep fails" (unchanged contract)
- "apkeep wins when it succeeds" (fetch now called by apkeep
itself, not apkmirror-api)
- "picks apkeep when apkmirror-api fails" (unchanged contract)
- "throws when all sources fail" (unchanged contract)
- "does not throw when fetch returns non-OK" (fetch now
called by both apkeep and apkmirror-api in parallel)
New (3 tests):
- "fast source wins while a lower-priority source hangs":
injects hanging resolvers for apkmirror + apkmirror-api,
asserts apkeep's fast result returns in <10s (not 60s).
- "all sources fail (the priority-ordered fall-through
path)": exercises the throw path with all three sources
rejecting — the sentinel-conversion keeps this free of
unhandled rejections.
- "a slow high-priority source beats a fast low-priority
one": injects a fast apkmirror-api resolver with a valid
URL but apkeep (running the real fixture command) still
wins because of priority order. This is the explicit
design-choice pin the audit requested.
README.md wording updated from "first valid result wins" to
"highest-priority valid result wins" with a sentence explaining
the new abandon-loser behavior. The inline `download()`
docstring at line 1478 is updated to match.
472 tests pass (was 469), lint clean, all four coverage
thresholds green. The new code paths in unified-downloader.js
are already exercised by the existing tests; coverage stays
at the same levels.
Move the aapt version-validation logic out of unified-downloader.js
into a focused module that splits the pure regex parsing from the
shell-out wrapper.
- parseVersionFromBadging(stdout) — pure regex match against
the aapt/aapt2 badging dump. No I/O, no shell. Tests pin the
multi-segment version case ('1.2.3-rc4+meta') that the prior
inline implementation handled implicitly.
- validateApkVersion(apkPath, expectedVersion, opts) — the
shell-out wrapper. Tries `aapt` first, falls back to `aapt2`,
returns { valid, actualVersion, error? }. The execFileSyncImpl
injection is preserved (already a pattern in
apk-abi-validator.js) so tests can drive both success and
fallback paths without standing up the real Android SDK
build tools.
11 unit tests in src/download/__tests__/aapt.test.js cover:
parseVersionFromBadging:
- typical aapt dump (YouTube's 20.44.38 fixture)
- multi-segment version with prerelease markers
- no-versionName input → null
- non-string input → null
validateApkVersion:
- valid match
- mismatch with the version-mismatch error message
- aapt ENOENT → aapt2 fallback succeeds
- both aapt and aapt2 fail → "aapt not available" error
- badging output has no versionName → "could not extract"
- argv-form execution contract: a malicious apkPath with
shell metacharacters passes through as a single argv
entry, not split or interpolated
- real execFileSync path (lazy require works end-to-end)
The new module is auto-picked up by the existing ./src/download/
coverage threshold (directory entry, no config change needed) —
current coverage on the new file lands at the same level as the
rest of src/download/.
483 tests pass (was 472), lint clean, all four coverage
thresholds green.
findApkFile was a 16-line pure-fs helper buried inside the
downloader's orchestration layer. Move it to src/download/scan.js
alongside the other directory-traversal helpers and add a stub-
friendly readdir/exists override so tests can drive the function
without a tmp dir on disk.
10 unit tests in src/download/__tests__/scan.test.js cover:
- APK_EXTENSIONS module surface (1)
- findApkFile:
- missing dir → null
- empty dir → null
- dir with no APK-shaped files → null
- first .apk wins in readdir order
- .xapk split package matched
- .apkm split package matched
- case-insensitive extension match (covers the
"app_v1.APK" CDN upstream case)
- non-recursive: APK in a subdirectory is NOT picked up
(pins the flat-APKS_DIR contract)
- stub-friendly readdir/exists override path
493 tests pass (was 483), lint clean, all four coverage
thresholds green. The new file lands inside the existing
./src/download/ coverage threshold (directory entry, no config
change needed).
…ep-variant.js
resolveApkeepVariant's body had two concerns: (a) the network
call to APKPure's protobuf endpoint and (b) the in-memory
parsing of the response to pick the smallest matching XAPK URL
for the requested version. The parsing logic was 50+ lines of
URL regex / base64-decoding / size-sort buried inside the
downloader's async function — hard to test without standing up
the protobuf endpoint.
Move the pure parsing to src/download/apkeep-variant.js as two
helpers:
- decodeApkeepCParam(c) — decodes APKPure's
pipe-separated-base64-encoded `c` query param into a plain
params object. Returns null on any decode error (which the
caller treats as "URL still valid, just sort with size=0").
- pickSmallestMatchingVariant(body, version) — the entire URL
selection pipeline: regex match for XAPK URLs, base64-decode
each `c` param, filter to the requested version, sort by
declared size, return the smallest URL.
resolveApkeepVariant itself keeps only the fetch call and the
chosen-size logging line; the rest delegates to
pickSmallestMatchingVariant. Both functions are pure — no I/O,
no fetch — and unit-testable with a canned body that mimics the
real protobuf shape.
12 unit tests in src/download/__tests__/apkeep-variant.test.js
cover:
decodeApkeepCParam:
- canonical arm64 URL's c-param (Sofascore 26.07.27 fixture)
- empty string / wrong segment count / non-string inputs all
return null
pickSmallestMatchingVariant:
- picks the smallest of three variants in a multi-URL body
- filters out URLs with the wrong version
- returns null when nothing matches the requested version
- empty body / non-string body / empty version → null
- tolerates trailing non-printable protobuf framing bytes
- regression pin: matches the live fallback-chain test
fixture's expected URL exactly, so any drift between the
two test surfaces would surface here
505 tests pass (was 493), lint clean, all four coverage
thresholds green. The new file is auto-picked up by the existing
./src/download/ coverage threshold (directory entry).
This branch was successfully deployed
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
Type of change
fix:) — non-breaking change that restores intended behaviourfeat:) — non-breaking change that adds user-visible capabilityfeat:orfix:with!) — change that requires user actiondocs:) — no production-code changerefactor:) — no user-visible behaviour changechore:) — CI, dependencies, repo maintenancetest:) — test-only changesValidation
npm cisucceeds on a clean clonenpm testpasses (no skipped tests for new code)npm run lintpassesjq '.' config.json && jq '.' patches.jsonsucceedsactionlint .github/workflows/<file>.ymlshellcheck .github/scripts/pipeline/*.sh .github/scripts/pipeline/lib/*.shpython3 -c "import yaml; yaml.safe_load(open('.github/workflows/<file>.yml'))"docs/updated where the public surface changesSecurity review
KEYSTORE_BASE64,KEYSTORE_PASSWORD,KEY_PASSWORD,KEY_ALIAS, GitHub tokens, APKMirror creds)permissions:widening in any workflowpull_request_targettrigger and no new use of untrusted input in shell stepsactions/*versionspull_request_targetis not used; secrets are not exposed to fork PRsDocumentation
docs/configuration.md(ifconfig.jsonshape changes)docs/architecture.md(if the workflow graph changes)docs/release-process.md(if release tag format, pruning, or pinning changes)docs/troubleshooting.md(if a new failure mode is introduced)CONTRIBUTING.md(if commit message or PR conventions change)Additional notes