feat: Install the Flagsmith CLI and authenticate it via OIDC by khvn26 · Pull Request #1 · Flagsmith/setup-cli

khvn26 · 2026-08-18T17:29:47Z

In this PR, we implement the Flagsmith/setup-cli action.

Here's what it does:

Grabs the installer script for the runner's platform, and requested (or latest) version.
Resolves the actual CLI version via installer's --dry-run mode.
Installs the CLI 🎉
Puts the installed binary in GitHub Actions' tool cache.
Detects whether the static credentials were set for the requested API URL.
If no static credentials detected and the action is not run against a fork PR, performs the OIDC exchange against the requested API URL.
Warns if something went wrong during the OIDC exchange.

Smoke-tested in #2.

Adds the composite action from Flagsmith/actions#9: resolve a CLI version, run the release's own install.sh (or install.ps1 on Windows), then exchange the job's GitHub OIDC token at POST /api/v1/auth/oidc/token/. The minted token is exported under the CLI's host-scoped credential name, because the CLI trusts unscoped FLAGSMITH_ACCESS_TOKEN only for its default host — a self-hosted api-url would otherwise silently go unauthenticated. Jobs without id-token: write install the CLI and warn, so the action stays usable with a Master API key from secrets.

Replaces the action's reimplementation of the release layout with a call to install.sh/install.ps1, pinned to the version being installed and run with --bin-dir and --no-modify-path. Platform detection, archive naming and checksum verification move back to the repository that publishes the releases, where the CLI's own CI exercises them on every platform; this action keeps only the installer's flags, which fail loudly rather than silently when they change. The binary is still cached by version and architecture, and PATH is still ours to set. Also, from reading depot/setup-action: - Skip the exchange when the job already carries a credential the CLI would use for this api-url, following the CLI's own precedence, so a workflow with a Master API key neither pays for nor fails an exchange it never asked for. - Name the fork pull request case explicitly. GitHub withholds an OIDC identity from those runs, so the generic advice to add id-token: write sent people to change something that could not help.

Everything they carried is already available from the CLI the action just installed: `flagsmith --version`, `flagsmith auth status`, and `flagsmith auth token` for scripts that need the raw credential. The access token in particular is better left out of the outputs, where it would invite copying a credential between steps and jobs for no gain. `authenticated` and `expires-in` existed largely so the workflow had something to assert; the tests now assert behaviour instead, which is a stronger check: `flagsmith auth status` for the authenticated path, and the absence of an exported token for the bring-your-own-credential path.

installerScript, binaryName and installerInvocation all branched on the same platform check, so they are one platformInstaller now. ExchangedToken.tokenType was set and never read.

install.sh already fails with its own message when neither is on PATH, so the pre-flight check was 23 lines of nicer wording plus its own test suite.

They were one-liners exported only so their own tests could reach them.

isForkPullRequest took a readEvent parameter no caller ever passed; the tests point GITHUB_EVENT_PATH at a tmp file now.

Trim comments to the ones that carry information the code does not.

The JSON-only reading suppressed exactly the bodies that carry a diagnosis this action cannot produce itself. A proxy demanding authentication, a captive portal or a load balancer with no backend answers in HTML, and Flagsmith never sees the request at all; dropping that body left the user with a hint about trust relationships and nothing about the proxy. Collapsed to one line and truncated, so the annotation stays readable.

The parser already strips surrounding whitespace and lowercases the host, so urlHost needs no bare-host fallback and only a single trailing slash is left to trim.

An unpinned install now runs the installer from main, whose default version is the latest release, instead of resolving the tag through the GitHub API. Only a pinned version is cached, so the cache key can never be a moving target.

The dry run reports the version it would install without downloading anything, so an unpinned install is cached under the same concrete tag a pinned one is.

index.ts exists to start run() and nothing else, so main.ts has no module-level side effect and needs no test-environment guard.

fetchOk owns the status check, the user agent and the body snippet; callers supply the first line of the failure.

tsconfig extends @tsconfig/node24 and now checks the test files too, which the excluded config never did.

Typecheck, tests, and the dist check already run in GitHub Actions and cannot run in pre-commit.ci (no npm toolchain). Replace them with lint-only hooks: pre-commit-hooks basics, prettier (configured to match existing style), actionlint, and workflow/renovate schema validation.

matthewelwell

Submitting an initial review based mostly on reading through the readme. I will dive into the actual code now, but I wanted to get my initial feedback over sooner.

matthewelwell

Some follow up comments following a review of the code (sans tests for now)

matthewelwell · 2026-08-19T11:16:12Z

+function lookupFold(env: NodeJS.ProcessEnv, name: string): string | undefined {
+  const wanted = name.toLowerCase()
+  return Object.keys(env).find(
+    (key) => key.toLowerCase() === wanted && env[key],
+  )
+}


I don't fully understand the need for this function? Can't we just enforce that env vars are case sensitive, and just replace all uses of this with something like process.env.key? ?

It's a little tricky. Environment name lookups are case-sensitive on Linux and MacOS. Hostnames, however, are case-insensitive, so a FLAGSMITH_API_KEY_test__flagsmith__com and FLAGSMITH_API_KEY_TEST__FLAGSMITH__COM should resolve for the same api-url. Since we want to make sure that the CLI will not be provided appropriate static credentials before moving on to the OIDC exchage, we have to replicate the case-insensitive lookup in here. In c1b5161, I've added a comment and removed CI logic for the bare env vars so the intent is clearer.

(BTW I'm really proud of how the CLI handles the credentials env vars for self-hosted! Even the gh cli has this problem today.)

Ok, I'm getting closer to understanding, but this line still doesn't really make sense to me.

// The unscoped form is an exact name, also matching the CLI.

... the 'unscoped form' of what? TBH I don't really understand what 'scoped' / 'unscoped' means in this context.

I also think that the lookupFold function could do with some documentation itself.

There aren't even any dedicated tests for it, and it's now only used in that one place, so maybe we should just remove the function and write the code inline at the calling location?

TBH I don't really understand what 'scoped' / 'unscoped' means in this context.

"Scoped" is FLAGSMITH_API_KEY_TEST__FLAGSMITH__COM. "unscoped" is FLAGSMITH_API_KEY. We need to check both FLAGSMITH_API_KEY, which is a Master API Key the CLI will prefix with Authorization: Api-Key, and FLAGSMITH_TOKEN, which is an OAuth token prepended with Authorization: Bearer.

How would you put it better?

Inlined lookupFold in b4ed3d5.

Ok, it definitely reads better now, but I still think it could be improved.

I think my main issue is that scoped means nothing on it's own. So, I have to read the Object.keys(env).find(...) statement to actually understand what scoped is... and that's where my lack of typescript fluency makes my brain hurt. What about changing it to something like:

const envVarName = scopedEnvName(base, apiUrl).toLowerCase() // Credential can be either a Token or API key const scopedCredential = Object.keys(env).find( (key) => key.toLowerCase() === envVarName && env[key], ) if (apiUrlScopedCredential) { return apiUrlScopedCredential }

Hope d4b6af8 clears this up a bit.

matthewelwell · 2026-08-19T11:18:08Z

+/**
+ * The body of a 200 response, or a throw of the caller's message with a
+ * one-line snippet of the body appended.
+ */
+export async function fetchOk(


The name of this function, and the esoteric comment read as slop to me. Can we improve?

Improved the docs in 7eaf216. Can't think of a better name for the function, suggestions welcome.

Something like doFetchWithErrorHandling or something would be more explicit, right? But on the whole, I don't love this function... It feels like it's trying to do too much?

doFetchWithErrorHandling is maybe ok, but I don't regard it as better than fetchOk.

I agree it's a lot, but it does precisely what all the HTTP calls we make need:

Make an HTTP call via the Actions API's wrapper.

Throw an error on non-200 response.

Provide a partial response body so action users can debug the failure.

Renamed to fetchOrThrow in 467d113 — let me know this reads better for you.

matthewelwell

Responding to all remaining threads, but I'm still looking at install.ts to try and understand why my view is so far off from where we're at.

matthewelwell · 2026-08-19T14:51:19Z

+function lookupFold(env: NodeJS.ProcessEnv, name: string): string | undefined {
+  const wanted = name.toLowerCase()
+  return Object.keys(env).find(
+    (key) => key.toLowerCase() === wanted && env[key],
+  )
+}


Ok, I'm getting closer to understanding, but this line still doesn't really make sense to me.

// The unscoped form is an exact name, also matching the CLI.

... the 'unscoped form' of what? TBH I don't really understand what 'scoped' / 'unscoped' means in this context.

matthewelwell · 2026-08-19T14:53:15Z

+function lookupFold(env: NodeJS.ProcessEnv, name: string): string | undefined {
+  const wanted = name.toLowerCase()
+  return Object.keys(env).find(
+    (key) => key.toLowerCase() === wanted && env[key],
+  )
+}


I also think that the lookupFold function could do with some documentation itself.

There aren't even any dedicated tests for it, and it's now only used in that one place, so maybe we should just remove the function and write the code inline at the calling location?

matthewelwell · 2026-08-19T14:58:35Z

+/**
+ * The body of a 200 response, or a throw of the caller's message with a
+ * one-line snippet of the body appended.
+ */
+export async function fetchOk(


Something like doFetchWithErrorHandling or something would be more explicit, right? But on the whole, I don't love this function... It feels like it's trying to do too much?

for more information, see https://pre-commit.ci

matthewelwell

Sorry, added a bunch of suggestions in an attempt to make the code more readable for someone who's eyes don't read typescript well...

matthewelwell · 2026-08-19T15:12:33Z

+ * Install the CLI, add it to PATH, cache in GHA tool cache,
+ * and return the directory it lives in.
+ */
+export async function installCli(requested: string): Promise<string> {


I think we can be more explicit here to help the readability.

Suggested change

export async function installCli(requested: string): Promise<string> {

export async function installCli(requestedVersion: string): Promise<string> {

Done in e9658c0.

matthewelwell · 2026-08-19T15:13:01Z

+ * and return the directory it lives in.
+ */
+export async function installCli(requested: string): Promise<string> {
+  const pinned = pinnedVersion(requested)


Another example here. From the name pinnedVersion() I don't really know what it does, so I have to go and read the function. A change like the following would help with that (imo).

Suggested change

const pinned = pinnedVersion(requested)

const parsedVersionNumber = parseVersionFromUserInput(requestedVersion)

Decided on parseVersionInput for the function and pinnedVersion for the variable. e9658c0.

matthewelwell · 2026-08-19T15:17:46Z

+ */
+export async function installCli(requested: string): Promise<string> {
+  const pinned = pinnedVersion(requested)
+  const temp = process.env.RUNNER_TEMP ?? process.env.TMPDIR ?? '/tmp'


Suggested change

const temp = process.env.RUNNER_TEMP ?? process.env.TMPDIR ?? '/tmp'

const tempDir = process.env.RUNNER_TEMP ?? process.env.TMPDIR ?? '/tmp'

Done in e9658c0.

matthewelwell · 2026-08-19T15:18:34Z

+  const binDir = path.join(temp, 'flagsmith-cli-install')
+  await fs.promises.mkdir(binDir, { recursive: true })
+
+  const installer = platformInstaller(pinned, temp, binDir)


Suggested change

const installer = platformInstaller(pinned, temp, binDir)

const installerDetails = getInstallerDetailsForPlatform(pinned, temp, binDir)

I think "details" would describe the thing we just got rid of with the interface refactor.

Renamed to getInstallerForPlatform in e9658c0.

matthewelwell · 2026-08-19T15:20:59Z

+  await fs.promises.mkdir(binDir, { recursive: true })
+
+  const installer = platformInstaller(pinned, temp, binDir)
+  await fetchInstaller(pinned || 'main', installer.script, installer.scriptPath)


Suggested change

await fetchInstaller(pinned || 'main', installer.script, installer.scriptPath)

await fetchInstallScript(pinned || 'main', installer.script, installer.scriptPath)

Done in e9658c0.

matthewelwell · 2026-08-19T15:29:44Z

+  // Belt and braces: if the dry run named no version, ask the binary itself.
+  const installed = version || (await binaryVersion(installer.binary))
+
+  // Succeed without caching rather than cache under a made-up key.


'Succeed' feels wrong here - to get here, aren't we essentially saying that we failed to install the binary. Why would we exit successfully?

This is a technically unreachable branch when the installation succeeds but we can't parse flagsmith --version. Reworded in e9658c0.

matthewelwell · 2026-08-19T15:30:08Z

+  }
+
+  // Belt and braces: if the dry run named no version, ask the binary itself.
+  const installed = version || (await binaryVersion(installer.binary))


Suggested change

const installed = version || (await binaryVersion(installer.binary))

const installedVersion = version || (await binaryVersion(installer.binary))

Done in e9658c0.

matthewelwell · 2026-08-19T15:30:58Z

+}
+
+/** What the installed binary reports as its version, or `''`. */
+async function binaryVersion(binary: string): Promise<string> {


Suggested change

async function binaryVersion(binary: string): Promise<string> {

async function getVersionFromBinary(binary: string): Promise<string> {

See e9658c0.

matthewelwell · 2026-08-19T15:31:27Z

+  script: InstallScript
+  scriptPath: string
+  /** Where the installer script leaves the binary. */
+  binary: string


Suggested change

binary: string

binaryPath: string

See e9658c0.

matthewelwell · 2026-08-19T15:41:04Z

+function lookupFold(env: NodeJS.ProcessEnv, name: string): string | undefined {
+  const wanted = name.toLowerCase()
+  return Object.keys(env).find(
+    (key) => key.toLowerCase() === wanted && env[key],
+  )
+}


Ok, it definitely reads better now, but I still think it could be improved.

I think my main issue is that scoped means nothing on it's own. So, I have to read the Object.keys(env).find(...) statement to actually understand what scoped is... and that's where my lack of typescript fluency makes my brain hurt. What about changing it to something like:

const envVarName = scopedEnvName(base, apiUrl).toLowerCase() // Credential can be either a Token or API key const scopedCredential = Object.keys(env).find( (key) => key.toLowerCase() === envVarName && env[key], ) if (apiUrlScopedCredential) { return apiUrlScopedCredential }

khvn26 added 30 commits August 18, 2026 15:07

refactor: collapse the platform branches and drop the unread token type

ca672a5

installerScript, binaryName and installerInvocation all branched on the same platform check, so they are one platformInstaller now. ExchangedToken.tokenType was set and never read.

refactor: stop pre-checking for curl and wget

f6bee56

install.sh already fails with its own message when neither is on PATH, so the pre-flight check was 23 lines of nicer wording plus its own test suite.

refactor: look failure hints up in a table

236cb00

refactor: inline the version helpers into resolveVersion

85d7cbc

They were one-liners exported only so their own tests could reach them.

test: read a real event file instead of injecting a reader

1275a2d

isForkPullRequest took a readEvent parameter no caller ever passed; the tests point GITHUB_EVENT_PATH at a tmp file now.

refactor: order each module public-first and high-level-first

d0db1b8

Trim comments to the ones that carry information the code does not.

refactor: find the folded credential name in one expression

f409361

style: drop a blank line from an import list

1be14d4

refactor: inline the api-url normalisation into its only caller

5b8aebd

refactor: normalise api-url with the URL parser

3afe754

The parser already strips surrounding whitespace and lowercases the host, so urlHost needs no bare-host fallback and only a single trailing slash is left to trim.

refactor: read GITHUB_TOKEN where it is used

e9123ad

refactor: one error for a release response we cannot read

2b14fa1

fix: show the release response body when it cannot be read

9f75a39

refactor: let the installer choose the latest release

656147b

An unpinned install now runs the installer from main, whose default version is the latest release, instead of resolving the tag through the GitHub API. Only a pinned version is cached, so the cache key can never be a moving target.

feat: resolve an unpinned version with the installer's dry run

aa07fd1

The dry run reports the version it would install without downloading anything, so an unpinned install is cached under the same concrete tag a pinned one is.

fix: degrade to an uncached install when the dry run says nothing

4b1588f

refactor: inline the dry-run parse into installCli

7195aac

feat: fall back to the installed binary for the cache key

570516f

refactor: take the version straight from the binary's output

a48eb3b

refactor: one truthiness test for the resolved version

fe69ab5

refactor: name the installer scripts with a literal union

2396f60

refactor: split the entrypoint from the action logic

93c6ead

index.ts exists to start run() and nothing else, so main.ts has no module-level side effect and needs no test-environment guard.

refactor: one throw for a response that is not a 200

b3eaa99

fetchOk owns the status check, the user agent and the body snippet; callers supply the first line of the failure.

improve docs

fd43813

chore: pre-commit, security-only renovate, release-please, PR tests

dd357b9

tsconfig extends @tsconfig/node24 and now checks the test files too, which the excluded config never did.

chore: pin workflow actions to commit SHAs

01ce478

khvn26 added 6 commits August 18, 2026 18:17

chore: drop the CI test workflow

b501213

fix: name the version that ships the exchange endpoint

e0f39f3

improve docs

229ecac

chore: empty commit to trigger CI

610d53c

chore: skip the npm hooks on pre-commit.ci

41ea715

matthewelwell reviewed Aug 19, 2026

View reviewed changes

Comment thread dist/index.cjs

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md Outdated

docs: clarify the audience input default

7a998a7

khvn26 force-pushed the feat/setup-cli-action branch 2 times, most recently from f6a586a to 80d5f23 Compare August 19, 2026 11:25

khvn26 added 2 commits August 19, 2026 12:29

docs: show a conventional bring-your-own-key example

0d93bef

docs: stop enumerating the exported environment variables

cd8cc94

khvn26 force-pushed the feat/setup-cli-action branch from 80d5f23 to cd8cc94 Compare August 19, 2026 11:29

matthewelwell reviewed Aug 19, 2026

View reviewed changes

khvn26 added 5 commits August 19, 2026 13:00

ci: keep the README usage examples on the released major

ee4a8c0

refactor: split constants out of a renamed credentials module

025d1ad

fix: look up unscoped credentials by their exact name, as the CLI does

c1b5161

docs: describe fetchOk in plain language

7eaf216

refactor: return installer behavior instead of an argument bag

8adfee8

khvn26 requested a review from matthewelwell August 19, 2026 13:26

khvn26 force-pushed the feat/setup-cli-action branch from b3dd6e8 to 8adfee8 Compare August 19, 2026 14:17

matthewelwell reviewed Aug 19, 2026

View reviewed changes

khvn26 and others added 3 commits August 19, 2026 16:15

refactor: inline the case-insensitive lookup at its only call site

b4ed3d5

[pre-commit.ci] auto fixes from pre-commit.com hooks

e393d0d

for more information, see https://pre-commit.ci

refactor: rename fetchOk to fetchOrThrow

467d113

khvn26 force-pushed the feat/setup-cli-action branch from 70ccd33 to 467d113 Compare August 19, 2026 15:19

matthewelwell reviewed Aug 19, 2026

View reviewed changes

khvn26 added 2 commits August 19, 2026 18:04

refactor: name the install steps for what they do

e9658c0

docs: explain scoped and unscoped credential names

d4b6af8

	export async function installCli(requested: string): Promise<string> {
	export async function installCli(requestedVersion: string): Promise<string> {

	const pinned = pinnedVersion(requested)
	const parsedVersionNumber = parseVersionFromUserInput(requestedVersion)

	const temp = process.env.RUNNER_TEMP ?? process.env.TMPDIR ?? '/tmp'
	const tempDir = process.env.RUNNER_TEMP ?? process.env.TMPDIR ?? '/tmp'

	const installer = platformInstaller(pinned, temp, binDir)
	const installerDetails = getInstallerDetailsForPlatform(pinned, temp, binDir)

	await fetchInstaller(pinned \|\| 'main', installer.script, installer.scriptPath)
	await fetchInstallScript(pinned \|\| 'main', installer.script, installer.scriptPath)

	const installed = version \|\| (await binaryVersion(installer.binary))
	const installedVersion = version \|\| (await binaryVersion(installer.binary))

	async function binaryVersion(binary: string): Promise<string> {
	async function getVersionFromBinary(binary: string): Promise<string> {

Uh oh!

Conversation

khvn26 commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthewelwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matthewelwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khvn26 Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khvn26 Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khvn26 Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matthewelwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthewelwell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

khvn26 commented Aug 18, 2026 •

edited

Loading

khvn26 Aug 19, 2026 •

edited

Loading

khvn26 Aug 19, 2026 •

edited

Loading

khvn26 Aug 19, 2026 •

edited

Loading