fix(arcup): guard curl 8.14.x retry/exit-code bug in download paths

[batuhankocyigit]

Closes #309

Summary

curl 8.14.0 and 8.14.1 can exit 0 on a permanent HTTP failure (e.g. 404)
when --retry is set but not triggered, while leaving the output file
empty or absent. This made several download paths in arcup silently
"succeed" with no content — most seriously in arcup/install, which has
no checksum step downstream to catch it.

Changes

• arcup/arcup: download_file_with_gh also upgraded [[ -f ]] to
  [[ -s ]]. This is unrelated hardening (this path uses gh, not
  curl, so the 8.14.x --retry bug never applies here) rather than a
  fix for the same root cause.
• arcup/arcup: download_file now also treats a zero-size output
  file as failure after the plain curl fallback.
• arcup/arcup: download_file_with_github_api upgraded its existing
  [[ -f ]] check to [[ -s ]] to also reject empty files.
• arcup/install: the installer's own curl call (no checksum
  verification anywhere in this file) now gets the same [[ ! -s ]]
  guard, changing the failure mode from "silently installs a 0-byte
  arcup" to an actual error.
• arcup/test_arcup.sh: added regression tests that simulate the bug
  (stub exits 0, writes an empty file) for all three fixed call sites.
  The existing test suite could not have caught this, since it stubs
  failures as return 22, which is not the shape the real bug produces.

Root-caused and verified against upstream curl commit b42776b
(curl/curl#17559); affected range is 8.14.0–8.14.1, fixed in 8.15.0.

[osr21]

Reviewed at bdcfc7d. The root cause is right, the guards are on the correct paths, and the regression tests are real — I verified each one fails on reverted code rather than just passing vacuously. Two things I'd fix before merge, one of which I think is blocking, plus an overlap with #314 that someone needs to decide.

Verified: the guards are on paths that genuinely carry --retry

The bug only bites when --retry is present, so I checked that each guarded call actually has it rather than taking the body's word for it. CURL_RETRY_ARGS=(--retry 3 --retry-delay 2 --connect-timeout 15) at arcup:32, and curl_with_headers (arcup:79–84) prepends it on both branches. So:

• arcup:327 download_file curl fallback — via curl_with_headers, in scope ✅
• arcup:352 download_file_with_github_api — explicit "${CURL_RETRY_ARGS[@]}", in scope ✅
• install:75 — via curl_with_headers, in scope ✅

Verified: the tests are not tautological

Suite passes at head (28 assertions). Reverting each production file individually and re-running makes exactly the matching new test fail:

revert arcup/arcup   -> not ok - download_file rejects curl's empty-file success (8.14.x bug)
  (skipping that one) -> not ok - download_file_with_github_api rejects curl's empty-file success (8.14.x bug)
revert arcup/install -> not ok - install rejects empty arcup download (8.14.x bug)

All three genuinely encode the fix. The observation in your PR body — that the old stubs return 22 and therefore could never have caught a bug whose whole shape is exit 0 plus an empty file — is the correct diagnosis, and the new stubs (: > "$out"; exit 0) model it accurately. Asserting that no empty file is left behind is a good extra touch; it stops a later [[ -f ]] from tripping over a stale artifact.

Blocking: ARCUP_INSTALLER_VERSION is not bumped

arcup:7–9 carries an explicit instruction:

# NOTE: if you make modifications to this script, please increment the version number.
# WARNING: the SemVer pattern: major.minor.patch must be followed as we use it to determine if the script is up to date.
ARCUP_INSTALLER_VERSION="0.2.0"

This PR leaves it at 0.2.0. That isn't just a convention miss — it's the difference between shipping the fix and delivering it. Tracing it through:

• check_installer_up_to_date (arcup:416) fetches the remote arcup, awks out its ARCUP_INSTALLER_VERSION, and only warns when version_gt remote local. Remote stays 0.2.0, so no user is ever told an update exists.
• update_arcup (arcup:441) downloads the new script, greps the same variable, and when ! version_gt remote local prints "Arcup is already up to date" and exit 0 — before copying anything into place.

So after this merges, every already-installed arcup keeps the buggy download_file and download_file_with_github_api indefinitely, and arcup --self-update will actively refuse to repair it. The fix would reach only fresh install runs. Bumping to 0.2.1 is a one-line change and I'd treat it as part of the fix.

The diff has four production changes; the body and tests cover three

download_file_with_gh (arcup:377) also goes [[ -f ]] → [[ -s ]], which isn't in the summary's bullet list and has no test. Two small notes:

• That path shells out to gh release download, not curl, so it is not the 8.14.x bug. It's unrelated (and welcome) hardening — worth describing as such so the commit history doesn't imply curl was at fault there.
• It's the only changed call site without a regression test, which is a slightly odd asymmetry in a PR whose main contribution is test coverage.

Neither is a correctness problem. -s can't cause a false rejection here either, since no real release asset or arcup script is zero bytes.

Overlap with #314 — both are open and both close #309

#314 (huklaa) targets the same issue and is still unmerged. I tested the merge rather than guessing: #314 merges clean onto main, and #318 merges clean on top of it — the two shared hunks are byte-identical, so nothing will explode. But merging both is redundant, and right now neither is moving. Concretely:

	#314	#318
install guard	✅ (identical)	✅ (identical)
download_file_with_github_api -f→-s	✅ (identical)	✅ (identical)
download_file curl-fallback guard	—	✅
download_file_with_gh	—	✅
ARCUP_INSTALLER_VERSION bump	✅ 0.2.1	❌
tests	test_install.sh, new file, 39 lines	test_arcup.sh, existing harness, 211 lines

On the fix side #318 is a superset of #314 except for the version bump. It's also the better-placed test work — extending the existing test_arcup.sh rather than adding a second harness, and correctly mode 755 from the start. One more small point in #318's favour: #314 strips the trailing newline from arcup/arcup (\ No newline at end of file), which #318 doesn't.

So the cleanest outcome looks like: take #318, add the version bump, close #314 with credit to @huklaa for filing first and for the download_file_with_github_api catch. That's a maintainer call, not mine — but it's worth someone making it, since two PRs fixing one issue are currently both sitting blocked.

The tests still don't run anywhere

At this head, nothing under .github/workflows/ references arcup at all — I grepped. So these 211 lines execute only when a contributor remembers to run arcup/test_arcup.sh by hand. That's pre-existing (I raised it on #314, and test_arcup.sh was already unwired before either PR), and it isn't this PR's job to fix. But #318 is now by some distance the largest investment in that suite, which makes the gap more expensive: a future regression in exactly this code path would go unnoticed despite the coverage existing. A three-line workflow step invoking bash arcup/test_arcup.sh would make all 28 assertions load-bearing.

Nice work on the root-causing — pinning it to curl commit b42776b / curl/curl#17559 and the 8.14.0–8.14.1 range, and explaining why the old stub shape couldn't reproduce it, is more diligence than most fixes of this size get.

[osr21]

Flagging as request-changes for visibility rather than as a gate — I don't have write access here, so treat it as advisory. Full analysis is in my comment above; pulling out the one blocker so it doesn't get lost in the length.

ARCUP_INSTALLER_VERSION is still 0.2.0. arcup:7–9 explicitly asks for an increment on any change to the script, and it is load-bearing rather than decorative:

• check_installer_up_to_date (arcup:416) only warns when version_gt remote local, so with the remote still at 0.2.0 no user is ever told an update exists.
• update_arcup (arcup:441) greps the same variable and, when ! version_gt remote local, prints "Arcup is already up to date" and exit 0 — before copying anything into place.

So merging at 0.2.0 means every already-installed arcup keeps the buggy download_file and download_file_with_github_api, and arcup --self-update actively refuses to repair it. The fix would reach fresh install runs only. Bumping to 0.2.1 is what turns this from shipped into delivered.

(I couldn't anchor this inline — arcup:9 isn't part of the diff.)

Non-blocking, detailed in the inline notes: download_file_with_gh is a fourth production change absent from both the summary and the tests and isn't the curl bug, and #314 overlaps on two byte-identical hunks while carrying the version bump this PR lacks.

Otherwise verified sound, and worth saying clearly: all three new tests fail on reverted code, and every guarded path genuinely carries --retry. This is good work — the blocker is one line.

[osr21]

Verified this path is genuinely in scope: curl_with_headers (arcup:79–84) prepends CURL_RETRY_ARGS=(--retry 3 ...) on both branches, so the 8.14.x exit-code bug can fire here.

Worth noting for reviewers weighing severity: an empty archive reaching this point would still be caught downstream by verify_checksum_file and tar -tzf, so this one fails safe today. The guard's real value is converting a confusing downstream checksum error into an accurate failure at the point it actually happened. Correct change, lower severity than install:75.

[osr21]

This is a fourth production change that isn't in the PR summary's three bullets, and it's the only changed call site with no regression test.

It's also not the curl 8.14.x bug — this branch shells out to gh release download, not curl, so --retry never enters the picture. It reads as unrelated (and welcome) hardening against gh leaving a zero-byte artifact behind.

No correctness objection; -s can't false-reject here since no real release asset is zero bytes. But the summary and commit message should describe it as separate hardening, otherwise the history implies curl was at fault on a path curl never touches.

[osr21]

This is the change that carries the actual severity, and the PR body is right to single it out: there is no checksum, signature or archive-integrity step anywhere in install, so pre-fix a 0-byte download went straight through install_bin and got chmod +x'd into place. Nothing downstream would have caught it.

Heads-up on overlap: this hunk is byte-identical to the one in #314, which is also open and also closes #309. I tested it — #314 merges clean onto main and this PR merges clean on top, so nothing will break either way, but merging both is redundant. #318 is a superset of #314's fix except for the ARCUP_INSTALLER_VERSION bump, which only #314 has. Worth someone deciding which lands rather than leaving both blocked.

[batuhankocyigit]

Bumped ARCUP_INSTALLER_VERSION to 0.2.1 per @osr21's review, and
updated the summary to call out the download_file_with_gh hardening
separately. Re: #314 overlap happy to have maintainers pick whichever
lands; this PR should now be a superset with the version bump included.