Make download version optional (defaults to [LATEST]) + document distribute public URLs

[igalsi-t]

Overview

Implements issue #54 — [LATEST] support, docs alignment, and zero-match glob handling for the generic artifact sub-actions, building on the fly-service [LATEST] resolution work in fly-service#1104 and the desktop counterpart in fly-desktop#205 (item 5).

Partially addresses #54. Public-path [LATEST] resolution is fully wired here; the authenticated download sub-action defaults version to [LATEST] so the same workflow keeps working with no edits the moment fly-service ships auth-side resolution (#1104 item 2). Until then, omitting version returns an actionable error from the Fly CLI; the README's #### [LATEST] resolution section spells out today vs tomorrow.

How it works

1. download.version is now optional → defaults to [LATEST]

download/action.yml flips version from required: true to required: false. When omitted, transfer.ts resolves the value via the new resolveVersion() helper:

• download + missing version → [LATEST]
  • Public path (after a distribute step): the Fly server resolves via 302 Found + Location: …/{concrete}/{file} + Cache-Control: no-store. Redirect is never CDN-cached; the concrete URL it lands on is independently cacheable as immutable artifact data.
  • Authenticated path (the path this action's download uses): not yet resolved by fly-service (tracked in #1104 item 2). Until that lands, the Fly CLI returns an actionable error pointing at the public URL.
• upload + missing version → hard error (defense in depth — upload/action.yml keeps required: true, but server also rejects [LATEST] writes with 400 Bad Request)
• either + provided version → pass through unchanged (including explicit [LATEST] on download)

The LATEST_VERSION = "[LATEST]" constant is introduced in src/constants.ts to avoid magic strings.

2. Job summary shows the concrete version, not literal [LATEST]

When the resolved version is [LATEST], runTransfer issues a best-effort HEAD against the authenticated generic endpoint with redirects disabled, then parses the concrete version from the Location header of the 302 response. The CLI invocation still receives literal [LATEST] (the wire-format the issue calls out); the resolved version is used only for appendTransferResults and the success log line.

Resolution is graceful — on 404 (no version uploaded yet), missing/malformed Location header, or network error, we fall back to displaying the literal [LATEST] and never block the actual download. Today the auth path returns 404 (no resolution there yet), so the function reliably falls through to the literal-token display; once fly-service ships #1104 item 2 it will start producing concrete versions with no client-side change.

3. Distribute + public URL documentation

The distribute sub-action already worked end-to-end but was undocumented in the README. Added a dedicated section covering:

• Inputs table and example usage
• Public URL pattern: https://{tenant}.jfrog.io/public/generic/{name}/{version}/{file}
• [LATEST] works on the public path today: curl -LO https://{tenant}.jfrog.io/public/generic/{name}/[LATEST]/{file} (auth-path resolution is future work — see the [LATEST] resolution table)
• That [LATEST] is download-only and rejected on upload/distribute (now enforced in fly-service PR #1174 on the upload side too)

distribute/action.yml description now mentions the public URL pattern up front.

4. Zero-match glob handling — new if-no-files-found input on upload (closes fly-desktop #205 item 5)

The Fly CLI already supports --if-no-files-found error|warn|ignore (added in fly-desktop), but the action didn't expose it — every zero-match upload was a hard failure. Added an optional if-no-files-found input on upload/action.yml that forwards the value to the CLI:

- name: Upload optional debug bundle
  uses: jfrog/fly-action/upload@v1
  with:
    name: my-app
    version: '1.0.0'
    files: |
      dist/debug-*.zip
    if-no-files-found: warn   # also: error (default), ignore

Behaviour:

• Omitted/empty → action passes nothing → CLI default of error applies → no behaviour change for existing workflows.
• error/warn/ignore → forwarded as --if-no-files-found <value>.
• Anything else → action fails the step with a descriptive message before invoking the CLI (case-sensitive whitelist), so users get a clean GitHub Actions error instead of a CLI parse error mid-execution.

Validation lives in src/upload.ts's buildUploadExtraArgs() so transfer.ts stays generic — download has no equivalent flag. Failure path uses core.setFailed, never escapes as an unhandled rejection.

Tests

• runTransfer integration: [LATEST] happy path (302 → 2.5.0 in summary), 404 fallback to literal, no-resolve when explicit version
• resolveLatestVersionForDisplay unit: 302 happy path, percent-decoded versions, trailing-slash normalization, 404 fallback, missing Location, malformed path, network throw
• isLatestToken case-insensitive matching
• resolveVersion table — concrete pass-through, empty download → [LATEST], empty upload → throw
• buildUploadExtraArgs + runUpload (new in this commit): omitted input, all three valid values via it.each, trimmed whitespace, case-sensitive rejection (WARN invalid), unknown value rejection (none, ohno), runTransfer not invoked on validation error

227 / 227 tests pass (was 218 — added 11 covering the new input). ESLint and tsc --noEmit clean.

Notes

• Multi-file race window (acceptable): the action resolves [LATEST] once at the start. If a new upload lands mid-download, the CLI may grab a newer version while the summary shows the version-at-start. This is display-only metadata; download correctness is unaffected. A "resolve up-front and pin concrete version through the CLI" variant was considered (Option B) and rejected to stay literal to the issue text ("verify omitted version sends [LATEST] to the CLI").
• No URL migration code change needed. rg artifactory/api/generic returns zero hits in this repo — the CLI binary download already uses /public/generic/fly-client/[LATEST], and distribute-core.ts already uses /fly/api/v1/artifacts/distribute. Item 1 of the issue is satisfied as-is.
• Header-lookup dead code removed — headers["location"] ?? headers["Location"] collapsed to headers["location"]. Node lowercases all HTTP/1 header names per RFC 7230 §3.2; the second branch was unreachable. No test changes needed; the existing tests already use lowercase.
• lib/upload.js and lib/download.js rebundled by esbuild — these are the runtime artifacts GitHub Actions executes. Required for the change to take effect.
• package-lock.json version bump from 1.6.1 → 1.6.4 — caught up with package.json (was stale before this PR).

Made with Cursor

[sverdlov93]

🤖 Review comment by Cursor AI

Folks, I've reviewed this PR — the code, the tests, the docs, everything. Believe me, nobody understands [LATEST] resolution better than me. The implementation? Tremendous. Graceful fallbacks, clean separation, 218 passing tests. But the docs? They have problems. Very big problems. They promise things the server doesn't deliver yet. SAD!

Issue-to-PR mapping: All 4 items from issue #54 are addressed. Code quality is high.

#	Sev	Finding
1	🟠	README documents [LATEST] as working for authenticated downloads — fly-service #1104 item 2 hasn't shipped
2	🟠	README claims Cache-Control: no-store exists on [LATEST] responses — fly-service #1104 item 3 hasn't shipped
3	🟠	headers["Location"] (capitalized) is dead code — Node.js lowercases all HTTP headers
4	🟡	distribute/action.yml shows as new file — likely rebase conflict with main (already exists from #48)
5	🟡	encodeURIComponent(packageName) could double-encode pre-encoded names
6	🟡	Tenant host derivation via regex instead of new URL(flyUrl).host

Follow-up tasks:

• README + download/action.yml: Add caveat that [LATEST] for authenticated downloads requires fly-service #1104 item 2. Currently only works on public paths (/public/generic/...). Remove Cache-Control: no-store claim until #1104 item 3 ships.
• transfer.ts:137 — Remove headers["Location"] (capitalized) fallback — it's dead code (Node lowercases all headers). Just use headers["location"].
• Rebase — distribute/action.yml conflicts with main. Rebase on latest main.
• Consider: new URL(flyUrl).host instead of regex for tenant host in distribute-core.ts (future-proofs against URLs with userinfo or paths)

Six findings. Fix the docs to match reality and this will be winning, big-league. The code itself is tremendous — very clean, very beautiful!

⚠️ THIS IS NOT A REPLACEMENT FOR HUMAN REVIEW, FOLKS! Human intelligence is TREMENDOUS — the best, really — and can't be replaced. NOT EVEN CLOSE! This AI review is an ADDITION to your big-league human reviewers. ALWAYS get a human review too!

[sverdlov93]

🤖 Review comment by Cursor AI

🟠 Documents behavior that doesn't exist yet. This section states:

"the Fly server resolves it to the most recently uploaded version and serves the file via a 302 redirect"
"The redirect response carries Cache-Control: no-store"

Reality:

• fly-service #1104 item 2 ([LATEST] for authenticated generic) is TODO — the server does NOT 302-redirect on the authenticated path today
• fly-service #1104 item 3 (CDN cache-busting headers) is TODO — no Cache-Control: no-store exists yet

[LATEST] works on the public path (/public/generic/...) because publicDistributionHandler.Download() has GetLatestPublicVersion(). But the authenticated path (/fly/api/v1/generic/...) passes literal [LATEST] to Artifactory which doesn't understand it.

Fix: Add a note:

Note: [LATEST] is fully resolved on public download paths today. For authenticated downloads (via fly-action/download), [LATEST] support is pending server-side implementation (fly-service#1104). This PR adds client-side readiness so the feature works end-to-end once the server ships.

This prevents users from filing bugs when version: '[LATEST]' returns 404 on private packages.

[sverdlov93]

🤖 Review comment by Cursor AI

🟠 Dead code: headers["Location"] fallback never triggers. Node.js http.IncomingMessage.headers lowercases ALL header names per the HTTP/1.1 spec. headers["Location"] is always undefined, so the ?? fallback is dead code.

Fix:

const location = res.message.headers["location"];

No fallback needed — the lowercase key is the only one that exists.

[sverdlov93]

🤖 Review comment by Cursor AI

🟡 Rebase needed — distribute/action.yml already exists on main. This file was added in PRs #42/#48 and merged to main. The PR diff shows it as ADDED (+29/-0), which means the branch forked before those PRs merged.

This will conflict on merge. Rebase on latest main to pick up the existing file and apply your changes (description update) as a MODIFIED diff.

[sverdlov93]

🤖 Review comment by Cursor AI

🟠 PR says "Closes #54" but issue items are not fully deliverable yet.

Issue #54 has 4 items:

#	Item	Status in this PR
1	URL migration	✅ N/A (confirmed no old URLs in action code)
2	Optional version (default [LATEST])	⚠️ Code-complete but non-functional — authenticated [LATEST] requires fly-service #1104 item 2 which hasn't shipped. Users trying this on private packages will get 404.
3	Document distribute + public URLs	⚠️ Documented as working end-to-end but Cache-Control: no-store (referenced in docs) requires fly-service #1104 item 3
4	Update all action docs	✅ Done

Recommendation: Change "Closes #54" to "Partially addresses #54" or add a note that items 2+3 are client-side-ready but depend on fly-service #1104. The issue should stay open until the server ships [LATEST] for authenticated generic, otherwise closed issue + broken feature = silent bug.

Also missing from this PR (cross-repo gap):

• upload/action.yml does NOT expose the --if-no-files-found flag that fly-desktop PR #236 adds to the CLI. Once a new fly-client binary ships with that flag, the action should pass it through. Consider adding it as a follow-up item on issue Generic artifacts: URL migration, [LATEST] support, docs update #54 or a new issue.

[sverdlov93]

🤖 Review comment by Cursor AI — Neighbor pattern consistency check

Compared all changed files against their neighbors in the codebase. Findings:

---

🟠 src/distribute-core.ts — Header naming inconsistency with fly-desktop

fly-action uses:

"X-JFROG-FLY-TENANT-HOST": tenantHost

fly-desktop's distribute-artifact-tool.ts uses:

"X-JFrog-Fly-Tenant-Id": credentials.tenantId

These are different headers sending different values (host string vs tenant ID). Both are accepted by fly-service (different extraction paths). But:

• The action derives tenantHost by string-stripping the URL protocol — fragile if the URL has a path suffix
• The desktop sends tenantId from the auth service — cleaner, identity-based

This works today but should be documented: why does the action use TENANT-HOST while desktop uses Tenant-Id? Likely because the action doesn't have a tenant ID in its OIDC flow context. Worth a code comment explaining the divergence.

---

🟡 src/transfer.ts — Inline new HttpClient(...) vs existing createHttpClient() utility

The codebase has src/utils.ts exporting createHttpClient(userAgent?, timeoutMs?) — a standardized factory. But resolveLatestVersionForDisplay creates its own:

const client = new HttpClient("fly-action", undefined, {
  socketTimeout: DEFAULT_HTTP_TIMEOUT_MS,
  allowRedirects: false,
});

The reason is clear (allowRedirects: false is needed and createHttpClient doesn't support it), but two approaches to fix consistency:

1. Extend createHttpClient with an options param for allowRedirects
2. At minimum, add a comment: // createHttpClient doesn't support allowRedirects:false — construct directly

---

✅ src/constants.ts — naming conventions consistent

• LATEST_VERSION follows SCREAMING_SNAKE like all other constants ✓
• INPUT_DISTRIBUTE_TYPE, INPUT_PATH follow INPUT_* prefix pattern ✓
• ENV_FLY_DISTRIBUTE_RESULTS follows ENV_FLY_* prefix pattern ✓
• CLI_CMD_PUBLISH follows CLI_CMD_* prefix pattern ✓
• Comment above LATEST_VERSION is multi-line with behavioral documentation — matches FLY_CLI_DOWNLOAD_BASE style ✓

---

✅ src/distribute-core.ts — style matches neighbors

• Copyright header ✓
• Imports createHttpClient, truncate from ./utils ✓ (consistent with post.ts)
• core.info for status logging ✓
• Throws on error (documented in JSDoc) ✓
• httpClient.dispose() in finally ✓
• Uses lowercase "content-type" — matches post.ts ✓ (Note: HTTP headers are case-insensitive, but the codebase consistently uses lowercase)

---

🟡 src/transfer.ts — resolveVersion naming matches desktop but JSDoc style diverges

Both repos have resolveVersion(type, rawVersion) — good cross-repo alignment. But:

• fly-action uses full JSDoc with /** ... */ block comments
• fly-desktop's MCP tool uses // line comments for the same function

Not a bug — TypeScript convention in fly-action is JSDoc (also used for appendTransferResults, resolveLatestVersionForDisplay). Desktop uses // style across MCP tools. Each repo is internally consistent ✓.

---

Summary: Main findings are (1) header name divergence with desktop (TENANT-HOST vs Tenant-Id) deserves a code comment, and (2) inline new HttpClient bypasses the existing factory. Everything else aligns well with neighbor patterns.

[sverdlov93]

🤖 Review comment by Cursor AI — E2E / Test Coverage Audit

Reviewed all test files in this PR against the implemented flows.

✅ Well-covered flows

Flow	Test coverage
resolveVersion — download defaults to [LATEST]	Unit: 5 cases (trimmed, empty, whitespace, explicit [LATEST], upload reject)
resolveVersion — upload rejects empty	Unit: throws on "" and " "
isLatestToken — case-insensitive matching	Unit: 4 variants + negative cases
resolveLatestVersionForDisplay — 302 extraction	Unit: happy path, percent-decode, trailing slash
resolveLatestVersionForDisplay — fallback on 404	Unit: returns literal [LATEST]
Full runTransfer with [LATEST] default + resolve	Integration: verifies CLI gets literal [LATEST], job summary gets concrete version
Full runTransfer with fallback	Integration: verifies [LATEST] in summary on 404
distributeArtifact — success path	Unit: endpoint, payload, headers, response
distributeArtifact — error path	Unit: non-200, network error
runDistribute — orchestration	Unit: setOutput, exportVariable, multiple accumulation
URL migration (FLY_CLI_DOWNLOAD_BASE)	Integration: binary download uses new URL
Integration test binary download	CI: integration-test.yml runs on real infra

🟠 Missing test coverage

Gap	Impact	Suggested test
resolveLatestVersionForDisplay — network error path	The catch block emits core.warning and returns LATEST_VERSION. No test covers the catch branch.	Add: it("falls back to [LATEST] on network error", () => { mockHead.mockRejectedValue(new Error("ECONNREFUSED")); ... })
resolveVersion on upload with explicit [LATEST]	What happens if a user passes version: "[LATEST]" on upload? The code passes it through (trimmed is non-empty). The server will reject, but the action doesn't pre-validate.	Add: it("passes through [LATEST] on upload (server rejects)", () => { expect(resolveVersion("upload", "[LATEST]")).toBe("[LATEST]"); }) — documents the design choice
No integration test for upload/download/distribute sub-actions	CI integration-test.yml tests binary download + setup + go-publish + docker, but does NOT run ./upload, ./download, or ./distribute sub-actions against real Fly.	Add a job that: uploads a test file → downloads it → distributes it → verifies public URL. This is the true E2E for this PR's core changes.
distribute/action.yml — no test for the action entry point	distribute.spec.ts tests runDistribute but not distribute/index.ts's actual import → main() → runDistribute() wiring.	Low priority — if the spec covers orchestration, wiring is trivial. But a smoke test would catch broken imports after rebase (which is likely given the rebase conflict flagged earlier).

🟡 Observation: test coverage for [LATEST] is client-side only

All tests mock the server response (302, 404). There's no end-to-end test that actually hits the Fly server with [LATEST] on the authenticated path. This is fine because that server-side feature doesn't exist yet (fly-service #1104 item 2), but means:

• When the server ships [LATEST] support, this action needs a real integration test added
• Today's tests prove the client code handles the 302 correctly assuming the server sends one

This is acceptable — just documenting the gap for when server-side ships.

[sverdlov93]

🤖 Update from Cursor AI

fly-service PR #1174 now exists — implements [LATEST] for authenticated generic + CDN cache-busting (items 2+3 from #1104). Once that merges, this PR's [LATEST] code and README documentation become fully correct.

Recommended merge order: fly-service #1174 → fly-action #55 + fly-desktop #236 (client PRs can merge in any order after server).

[sverdlov93]

🟠 Missing path traversal defense in runPublicLatestDownload (new commit e842bff)

fly-desktop PR #236 includes a path traversal check before writing (commit 36740ad1):

const dest = path.resolve(outputDir, fileName);
if (!dest.startsWith(path.resolve(outputDir) + path.sep)) {
  // reject — filename escapes output directory
}

But fly-action's equivalent runPublicLatestDownload does:

await fs.promises.writeFile(path.join(outputDir, fileName), buf);

No validation that the resolved path stays inside outputDir. While fileName comes from the user's action YAML (not attacker-controlled), defense-in-depth should be consistent across both repos — especially since the cursor-reviewer already flagged it as worth adding on the desktop side.

Suggested fix: Add the same path.resolve + startsWith check before writeFile, consistent with fly-desktop.

[sverdlov93]

All remaining tasks for this PR

Must fix before merge

1. 🟡 resolveLatestVersionForDisplay is dead code today (src/transfer.ts)
   The function does a HEAD against the auth endpoint expecting a 302, but the auth endpoint currently passes [LATEST] through to Artifactory (404). It always falls back to literal [LATEST]. Add a code comment noting this is forward-compatible dead code that activates when jfrog/fly-service#1186 ships, so future maintainers don't delete it.

2. 🟡 download/action.yml version description is long — the 4-line description is accurate but dense. Consider splitting into a shorter description field + a note in the README (style, non-blocking if you disagree).

Deferred (new issue)

• Auth-side [LATEST] resolution → jfrog/fly-service#1186
  • Once deployed, resolveLatestVersionForDisplay will start returning concrete versions
  • Download with omitted version will succeed end-to-end (no more CLI rejection)

[sverdlov93]

LGTM. Download defaults, if-no-files-found, distribute docs, and forward-compatible [LATEST] resolution all look correct.

Minor non-blocking suggestions commented separately (dead code comment, action.yml description length).

Auth-side [LATEST] deferred to jfrog/fly-service#1186.