Skip to content

test: run the CLI as a script - #69

Open
khvn26 wants to merge 34 commits into
mainfrom
test/testscript-migration
Open

test: run the CLI as a script#69
khvn26 wants to merge 34 commits into
mainfrom
test/testscript-migration

Conversation

@khvn26

@khvn26 khvn26 commented Aug 1, 2026

Copy link
Copy Markdown
Member

In this PR, we break down the cmd_test.go megafile by migrating most of command tests to testscript.

  • Every case carries a Given/When/Then description. TestEveryScriptCaseIsGivenWhenThen serves as a linter.
  • The fake Flagsmith instance is factored out to fake_test.go. It logs every request, so bodies, call counts and query parameters are assertable in the txtar files.
  • Scripts are pinned to the fake by the environment and the keychain is mocked in-process.

Adjacent behaviour changes:

  • the arrow-key picker was gated on stdin alone, so flagsmith init 2>log drew a full-screen selector into the capture. It now needs stderr to be a terminal too, falling back to the numbered picker otherwise.
  • the fake instance returned segments in map order, so segment list rows swapped between runs — invisible to strings.Contains, immediate against a golden.

14 tests stay in Go, each for a reason a script can't reach: browser login, concurrency, the confirmation-deadline case (needs an answer that arrives late), and two unit tests of internal functions.

khvn26 and others added 21 commits August 1, 2026 16:35
@khvn26
test: run the CLI as a script, starting with evaluate
11407f0 
cmd_test.go asserts on command behaviour a fact at a time: a substring of
the output here, a recording field on the fake there, and an `if err != nil`
guard around each. Over half its assertion sites are that guard. A new case
costs a subtest of the same shape, so the file grows faster than the
behaviour it covers — `evaluate` alone arrived with 998 lines of it.

A testscript script says the same things in one file: the config it starts
from, the commands it runs, and the output, requests and exit status it
expects. The CLI runs as a real subprocess, so exit codes, os.Stderr and
process-global state are the real thing rather than something the harness
simulates — which retires captureOSStderr, and makes "the SDK's logger never
reaches stderr" a one-line `! stderr .`.

The fake now logs every request it serves, naming the credential by kind
rather than value. That makes bodies, query parameters and call counts
assertable without a recording field per endpoint, and makes the negatives
free: no refetch after a write, no write before confirmation.

Each case carries a Given/When/Then triple describing it in full, as the
engine's test data does. TestEveryScriptCaseIsGivenWhenThen enforces both
the triple and its self-sufficiency, so a case cannot come to rely on its
neighbours.

Setup pins FLAGSMITH_API_URL to the fake and mocks the keychain inside the
impersonated binary. Nothing under test can reach api.flagsmith.com or the
developer's keychain by forgetting a flag — which the Go tests only avoid by
remembering to pass --api-url.

beep boop
@khvn26
test: evaluate's identity and environment-name cases become scripts
e6a98d4 
The remaining two evaluate tests turn on things the request log already
records — which identity was sent, with which traits and whether transiently,
and which environment key reached the SDK API. Asserting on the log covers
them without identifyBody(), and covers the trait typing more precisely:
the wire value is in the transcript, so "42 is sent as an integer" is the
literal `"trait_value":42` rather than a Go-side comparison after decoding.

Two new script commands carry the fixtures these need. `cache` seeds the
local name cache, resolving the path against the script's own HOME — going
through cache.Merge would write to the developer's cache directory, since
the builtin runs in the test process. `fake environments` and
`fake sdk-identity` install the Admin and SDK fixtures.

$MASTER_KEY joins the exported variables: an `env` lasts to the end of a
script, so a case that clears the credential would otherwise decide what its
neighbours start from.

The back-reference check now matches whole words. It was reading "against"
as "again" — and, once fixed, caught two descriptions of mine that leant on
the case above them.

beep boop
@khvn26
test: organisation becomes a script
d89ecfd 
The create and update cases asserted on lastOrgBody; the request log carries
the same body, so the boolean --force-2fa sets is checked as the wire value
rather than after decoding. Delete asserted through orgByID on the fake — a
following `organisation list` says the same thing in the CLI's own terms.

`fake orgs` and `fake org-fields` set the organisations a script sees.
Fixtures persist for the length of a script, as environment variables do, so
each case installs the ones it needs rather than inheriting them.

beep boop
@khvn26
test: config becomes a script
5a562ae 
Source resolution is what this command is for, so the layered case now shows
the whole document at once — file, environment variable and flag each winning
where they should, ids carrying their cached names — rather than eight
separate lookups into a parsed map.

These scripts clear FLAGSMITH_API_URL. Setup sets it so that nothing under
test can reach the real API by accident, but `config` reads no API at all and
reports the variable as the apiUrl source, which is the opposite of what the
default case is about.

`cache` grew an instance argument and now merges, since a config names an
instance of its own rather than the fake.

beep boop
@khvn26
test: api and the --api-url flag become scripts
2a2c9ce 
/api/v1/echo/ reflects the request back, so what the CLI sent is already on
stdout: these cases read it there rather than decoding it into a Go map
first. Two of them go through --jq, because the fake's encoder escapes & as
& and the raw body would have the scripts asserting on the fake's
encoding rather than on the request.

The flag-position cases compare against the first invocation's output with
cmp, which is what "position changed behaviour" meant.

beep boop
@khvn26
test: segment becomes a script
8df2044 
The rule-tree cases asserted by walking a decoded body — rules[0].rules[0]
.conditions[0].value — to check that IN travels as the packed string the API
wants. The request log carries that string directly, so the script says what
the wire looks like instead of how to navigate to it.

Comparing `segment list` against a golden turned up an ordering bug in the
fake: it holds segments in a map and returned them in whatever order the map
iterated, so the two rows swapped between runs. Contains-assertions never
saw it. The handler now sorts by id, as a paginated API would.

`segment get` puts its nudge on stderr, which the merged-output helper hid.

beep boop
@khvn26
test: flag list becomes a script
14a1e7f 
The override views asserted position by index arithmetic — beta-optin's
offset in the output being less than us-adults' — to check priority order.
A golden shows the rows in order, so the ordering is the assertion rather
than something inferred from it.

Fixtures now come from the script: `fake features blob.json` and its
neighbours read a JSON file out of the txtar, so the features a case turns
on sit next to the case. The verbs that need existing fixture helpers
delegate to them.

That meant cmdFake could no longer hold the fake's lock across the whole
switch, since those helpers take it themselves. Dropping the blanket lock
left four fixture writes unsynchronised against the server reading them,
which -race caught: they are wrapped individually now.

flag list --segment's fan-out test stays in Go. It asserts that the
per-feature reads overlap, and a script can see how many requests were made
but not whether any two were in flight at once.

beep boop
@khvn26
test: flag get becomes a script
0528f17 
The detail views are compared whole rather than probed for one substring at
a time, so the rows nobody asserted on — Description, Lifecycle stage — are
covered too, and a null identity-override count reading 0 is visible as the
row it renders rather than inferred from the output containing a "0"
somewhere.

The by-id cases move together into one script: naming a feature by id is one
behaviour across get, update, delete and reorder, and each asserts the same
thing, that the canonical name is what travels.

beep boop
@khvn26
test: flag update, enable/disable and priority become scripts
1e7d5a7 
These asserted mostly on lastUpdate, walking into the decoded body to reach
segment_overrides[0].value.type. The request log carries the body as it went
out, so a script says what the wire looks like — which also makes "the
current state rides along unchanged" one pattern rather than four field
comparisons.

The no-refetch cases count calls with grep -count. That needs somewhere to
count from, since the log runs the length of a script, so `fake
forget-requests` marks the start.

`fake features-default` restores the usual two features: several of these
fixtures replace the list with one feature, and the next case along needs
the other one back. State carries between cases the way environment
variables do, so each case sets up what it needs.

$CACHE is exported for the case about what name resolution remembers.
Spelling the path out in the script would have been darwin-only.

beep boop
@khvn26
test: flag's identity overrides become scripts
7f15ec6 
Core and edge identities split into a script each. They are two different
sets of endpoints — a core identity is addressed by the id a lookup returns,
an edge one by its identifier — and the cases that matter most are the ones
asserting that only one of them was called. The request log shows both, so
that is a `! grep` rather than a nil check on a recording field.

The writes are compared as they went out. Their fields are marshalled in
alphabetical order, which is worth knowing when reading these: enabled comes
before feature, and feature_state_value after both.

beep boop
@khvn26
test: flag reorder, flag delete and the usage lines become scripts
84f99b1 
Reorder is the one that gains most from a golden. It asserted the resulting
order by comparing the offsets of two names in the output, and asserted the
re-permuted request by walking into segment_overrides twice. Both are now
the thing itself: a table with us-adults first, and the request body as it
went out.

The refusal case is worth reading — an override whose state row has gone
missing must not be echoed back as a zero state, and `! grep update-flag-v2`
says no write happened at all.

The accessors these tests needed — featuresSeg, segmentsCalls,
identityLookups and the rest — have no callers left, and go vet does not
report unused functions, so they go too.

beep boop
@khvn26
test: feature becomes a script
f699270 
The fixture these read against is now a JSON file in the script, so the
catalogue a case turns on — one standard feature, one multivariate, one
archived — is visible next to the case rather than in a Go helper three
hundred lines away.

Loading it needed a conversion: encoding/json makes every number a float64,
and the fake matches ids and counts against int, so a fixture written in a
script would never be found by id. Whole numbers decode to int now.

Worth knowing when reading the request assertions: bodies built from structs
keep their field order, and bodies built from maps come out alphabetical.
Both appear here.

TestFalseyEnvSwitches keeps its second case. It drives a confirmation prompt
and needs a terminal.

beep boop
@khvn26
test: environment becomes a script
9e46497 
Several of these turn on which requests were *not* made: `environment get`
answers from the list row it already has, and names the project from the
cache, so neither the environment nor the project is retrieved. That was
three counter accessors on the fake; it is now two `! grep`s against the
request log, next to the case that cares.

--json and --jq do retrieve, because the retrieve serializer is richer than
the list row, and the scripts pin that at exactly one call.

`cache` now accepts a kind with no pairs, for the case that wants the name
cache cold.

beep boop
@khvn26
test: project becomes a script
2124874 
The organisation-labelling cases are the interesting ones: a name resolved
once labels every row from the cache, and a later `project get` adds no
organisation fetch at all. That was a counter read before and after; it is
now a `! grep` over the request log.

Writing them turned up why the delete-by-id case says what it says. Given a
warm name cache, `project delete 101` prints "acme-api (101)" — the bare id
appears only when nothing can name it, so that case now clears the cache
first. The Go test got the cold cache for free from a fresh fixture per
subtest.

TestProject keeps its confirmation-deadline case, which needs a terminal.

beep boop
@khvn26
test: the cross-cutting cases become scripts
44d5205 
--version, the bare-invocation nudge, warnings landing on stderr, --json as a
global, and the server-side-key protections.

The key cases are worth keeping an eye on: a server-side key must not appear
in anything the CLI prints, however it arrived — from the environment, from
the command line, or in an error the SDK API handed back. Each asserts on
both streams. Making the positional rejection quote the ref back does fail
them, so they bite.

Two of these needed the environment taken away rather than set up: the nudge
only appears when there are no credentials to find, and Setup provides one.

With the last of the assertions on the fake's recording fields gone, the
fields themselves are written and never read. They were the per-endpoint
bookkeeping the request log replaced, so they go with it.

beep boop
@khvn26
test: the credential-precedence cases become scripts
1dbb67b 
Which credential wins, and what the CLI says about where it came from: an
access token in the wrong variable, a server-side key in the wrong variable,
the environment beating the keychain, and a Master API key beating an access
token.

The keychain travels between invocations as a file. The mock lives only as
long as a process and cannot be enumerated, so $KEYCHAIN is seeded from a
txtar section and written back afterwards, for the instances a script deals
with. Execute returns on success and exits on failure, so a failed
invocation's writes are not carried forward — enough for these, and worth
knowing before writing a case that expects otherwise.

`subst` expands variables in a fixture file in place. txtar bodies are
copied out verbatim, so a config that has to name the fake writes $API.

beep boop
@khvn26
test: init without a terminal becomes a script
c1383eb 
Everything init can be told on the command line: the flags that conflict,
the ones that are required non-interactively, what it preserves from a
config it is rewriting, and its refusal to touch a malformed file.

Each case runs in a directory of its own. init writes flagsmith.json into
the directory it runs from, so sharing one would make every case depend on
what the last one wrote.

What init created is read back through `flagsmith config --json`, which is
also how a user would check. That covers the written file and its schema
without a loader in the test.

The interactive cases stay in Go: they drive huh's picker, which needs a
terminal a script cannot give it.

beep boop
@khvn26
fix: fall back to the numbered picker when stderr is not a terminal
f522fff 
The arrow-key selector puts stdin in raw mode and draws over stderr. It was
gated on stdin alone, so `flagsmith init 2>log` — or init under any harness
that captures stderr — drew a full-screen picker into the capture: escape
sequences and repaints in the file, a half-drawn selector on screen.

Both ends have to be a terminal for that UI to make sense. Where stderr is
redirected the numbered fallback is what the situation wants, and it is
already there for pipes on stdin.

beep boop
@khvn26
test: init's interactive cases become scripts
145a128 
ttyin gives the CLI a pty, so it prompts; stderr is captured rather than a
terminal, so it answers with the numbered picker. The answers are a file of
choices, and blank lines accept the defaults.

Splitting the streams turned up an inconsistency the merged-output helper
hid. A fresh init keeps stdout empty — the prompt UI belongs on stderr —
but a re-init writes its diff to stdout, so those two cases now assert
against different streams. Worth a look; the scripts describe what it does
today rather than what it ought to.

The confirmation-deadline case stays in Go. It proves that time spent at a
prompt is not counted against the invocation deadline, which needs an answer
that arrives late; ttyin has the answer ready, so a script would pass
whether or not the property held.

beep boop
@khvn26
test: give the fake instance a file of its own
a01adb7 
cmd_test.go was two thirds fake: a stub of the whole Flagsmith API, now
shared with the scripts, sitting in the file named after the tests that used
to be its only caller. It moves to fake_test.go, leaving cmd_test.go as the
fourteen tests a script cannot express plus the harness they run through.

newFake registered fifty-odd routes in one function. It now calls a mount
per resource, so the handlers for a command's endpoints sit together and the
largest of them is a screen or two rather than a thousand lines. The
authorization check the handlers shared was a closure inside it, and is now
a function beside them.

beep boop
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
47aeb49 
for more information, see https://pre-commit.ci
@coderabbitai

coderabbitai Bot commented Aug 1, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@khvn26, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 13 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4f5eb0fe-db74-4bfd-8d7f-9a0901bd5433

📥 Commits

Reviewing files that changed from the base of the PR and between 9ea3375 and 1e9930a.

📒 Files selected for processing (8)
  • .gitattributes
  • internal/cmd/cmd_test.go
  • internal/cmd/fake_test.go
  • internal/cmd/script_test.go
  • internal/cmd/testdata/script/confirm.txtar
  • internal/cmd/testdata/script/credentials.txtar
  • internal/cmd/testdata/script/init-interactive.txtar
  • internal/cmd/testdata/script/organisation.txtar
📝 Walkthrough

Walkthrough

The change adds a reusable in-memory HTTP backend and a subprocess-based testscript harness for CLI integration tests. It removes duplicated command test infrastructure. New scripts cover configuration, authentication, initialisation, resource commands, evaluation, feature management, flag overrides, validation, output formats, timeouts, and secret redaction. Terminal detection now requires both input and error streams to be terminals. Two Go module dependencies were added.

Estimated code review effort: 5 (Critical) | ~120 minutes

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@khvn26
test: keep the script goldens byte for byte, on every platform
9ea3375 
pre-commit stripped trailing whitespace from the goldens and pushed the
result, so CI compared the CLI's padded table output against unpadded
expectations and every platform failed. The goldens are the output byte for
byte — the last column is padded — so they are excluded from that hook and
from end-of-file-fixer.

The two scripts driving a pty skip where testscript has none, which is
everywhere but linux and darwin.

On Windows os.UserCacheDir reads %LocalAppData% rather than $HOME, so the
name cache the CLI wrote was not the one the scripts read. Setup points both
it and %AppData% at the script's own directory.

Removing the write-only recording fields left a lock/unlock pair with
nothing between it, which staticcheck flags.

beep boop
coderabbitai[bot]
coderabbitai Bot reviewed Aug 1, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: dfe29718-5d9c-4e76-ba48-a1acd2262446

📥 Commits

Reviewing files that changed from the base of the PR and between 00feae6 and 47aeb49.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (52)
  • go.mod
  • internal/cmd/cmd_test.go
  • internal/cmd/fake_test.go
  • internal/cmd/prompts.go
  • internal/cmd/script_test.go
  • internal/cmd/testdata/script/api-url-flag.txtar
  • internal/cmd/testdata/script/api.txtar
  • internal/cmd/testdata/script/cli.txtar
  • internal/cmd/testdata/script/config.txtar
  • internal/cmd/testdata/script/confirm.txtar
  • internal/cmd/testdata/script/credentials.txtar
  • internal/cmd/testdata/script/environment-document.txtar
  • internal/cmd/testdata/script/environment-key.txtar
  • internal/cmd/testdata/script/environment.txtar
  • internal/cmd/testdata/script/evaluate-deadline.txtar
  • internal/cmd/testdata/script/evaluate-empty.txtar
  • internal/cmd/testdata/script/evaluate-environment-name-errors.txtar
  • internal/cmd/testdata/script/evaluate-environment-name-lookup.txtar
  • internal/cmd/testdata/script/evaluate-environment-name.txtar
  • internal/cmd/testdata/script/evaluate-environment.txtar
  • internal/cmd/testdata/script/evaluate-failures.txtar
  • internal/cmd/testdata/script/evaluate-identity-rejections.txtar
  • internal/cmd/testdata/script/evaluate-identity.txtar
  • internal/cmd/testdata/script/evaluate-one-feature.txtar
  • internal/cmd/testdata/script/evaluate-test-flag.txtar
  • internal/cmd/testdata/script/evaluate.txtar
  • internal/cmd/testdata/script/feature-search.txtar
  • internal/cmd/testdata/script/feature-variant.txtar
  • internal/cmd/testdata/script/feature-write.txtar
  • internal/cmd/testdata/script/feature.txtar
  • internal/cmd/testdata/script/flag-by-id.txtar
  • internal/cmd/testdata/script/flag-delete.txtar
  • internal/cmd/testdata/script/flag-enable-disable.txtar
  • internal/cmd/testdata/script/flag-get.txtar
  • internal/cmd/testdata/script/flag-identity-core.txtar
  • internal/cmd/testdata/script/flag-identity-edge.txtar
  • internal/cmd/testdata/script/flag-list-environment-name.txtar
  • internal/cmd/testdata/script/flag-list-overrides.txtar
  • internal/cmd/testdata/script/flag-list.txtar
  • internal/cmd/testdata/script/flag-reorder.txtar
  • internal/cmd/testdata/script/flag-segment-by-name.txtar
  • internal/cmd/testdata/script/flag-update-priority.txtar
  • internal/cmd/testdata/script/flag-update-rejections.txtar
  • internal/cmd/testdata/script/flag-update-segment.txtar
  • internal/cmd/testdata/script/flag-update.txtar
  • internal/cmd/testdata/script/init-interactive.txtar
  • internal/cmd/testdata/script/init.txtar
  • internal/cmd/testdata/script/organisation.txtar
  • internal/cmd/testdata/script/project.txtar
  • internal/cmd/testdata/script/segment.txtar
  • internal/cmd/testdata/script/server-side-key.txtar
  • internal/cmd/testdata/script/usage.txtar

Comment thread internal/cmd/fake_test.go Outdated
Comment thread internal/cmd/fake_test.go Outdated
Comment thread internal/cmd/fake_test.go
Comment on lines +543 to +553
func withSegmentOverride(f *fakeInstance, withOverride bool) {
item := map[string]any{
"id": 2, "name": "max_items", "type": "STANDARD",
"num_segment_overrides": 1, "num_identity_overrides": 0,
"environment_feature_state": map[string]any{"enabled": false, "feature_state_value": 25},
}
if withOverride {
item["segment_feature_state"] = map[string]any{"enabled": true, "feature_state_value": "special"}
}
f.features["101"] = []map[string]any{item}
}

@coderabbitai coderabbitai Bot Aug 1, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document that withSegmentOverride requires the caller to hold the lock.

Unlike withFeatureSegments and withWorkflowGating, this helper mutates f.features without taking f.mu. It is currently safe because cmdFake calls it inside a locked set(...), but the convention stated at lines 497-498 is the opposite. State the locking contract in the doc comment so a future caller does not use it unlocked.

♻️ Proposed fix 
 // withSegmentOverride sets project 101's features to a single max_items
 // feature carrying an environment default and, optionally, a segment override.
+// The caller holds f.mu.
 func withSegmentOverride(f *fakeInstance, withOverride bool) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func withSegmentOverride(f *fakeInstance, withOverride bool) {
item := map[string]any{
"id": 2, "name": "max_items", "type": "STANDARD",
"num_segment_overrides": 1, "num_identity_overrides": 0,
"environment_feature_state": map[string]any{"enabled": false, "feature_state_value": 25},
}
if withOverride {
item["segment_feature_state"] = map[string]any{"enabled": true, "feature_state_value": "special"}
}
f.features["101"] = []map[string]any{item}
}
func withSegmentOverride(f *fakeInstance, withOverride bool) {
item := map[string]any{
"id": 2, "name": "max_items", "type": "STANDARD",
"num_segment_overrides": 1, "num_identity_overrides": 0,
"environment_feature_state": map[string]any{"enabled": false, "feature_state_value": 25},
}
if withOverride {
item["segment_feature_state"] = map[string]any{"enabled": true, "feature_state_value": "special"}
}
f.features["101"] = []map[string]any{item}
}

@khvn26 khvn26 Aug 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

23ba6aa

@coderabbitai coderabbitai Bot Aug 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

ast-grep outline internal/cmd/fake_test.go --items all --type function --match 'withSegmentOverride|cmdFake|set'
rg -n -A12 -B6 '\bwithSegmentOverride\s*\(' internal/cmd/fake_test.go

Length of output: 1210

@khvn26, verified. withSegmentOverride now locks f.mu before it mutates f.features. This matches the synchronization convention for the fake server. The finding is resolved.

🧠 Learnings used 
Learnt from: khvn26
Repo: Flagsmith/flagsmith-cli PR: 53
File: internal/cmd/cmd_test.go:6020-6024
Timestamp: 2026-07-29T09:59:44.020Z
Learning: In the Flagsmith CLI Go codebase, treat `go test --race` as sufficient coverage for concurrency concerns in test code. During code review, avoid introducing extra synchronization changes solely to adjust/test fixtures (i.e., don’t “paper over” expected test-only races) unless the race detector is failing or there’s evidence of a real concurrency bug beyond test fixtures. Prefer fixes that address the underlying issue rather than adding fixture synchronization just to change race behavior.

You are interacting with an AI system.

Comment thread internal/cmd/fake_test.go Outdated
Comment thread internal/cmd/script_test.go
Comment thread internal/cmd/script_test.go
Comment thread internal/cmd/testdata/script/confirm.txtar
Comment thread internal/cmd/testdata/script/credentials.txtar Outdated
Comment thread internal/cmd/testdata/script/init-interactive.txtar
Comment thread internal/cmd/testdata/script/organisation.txtar
khvn26 added 6 commits August 1, 2026 20:04
@khvn26
test: check the script goldens out with LF everywhere
36ff44f 
Windows checked them out with CRLF, so every cmp compared the CLI's LF
output against CRLF expectations and twenty-five scripts failed on lines
that render identically. The stored objects were LF all along; it was the
working-tree conversion.

Marked text rather than binary, so they still diff.

beep boop
@khvn26
test: drop the doc comments duplicated by the file split
73f2608 
Splitting the fake out walked back from each declaration to pick up its doc
comment, but the preceding block already ended with those lines, so twenty
comments were written twice. The detached copy is not godoc and says nothing
the attached one does not.

beep boop
@khvn26
test: give flagUpdateEnv back its own doc comment
5d455c2 
The split left commandShapes' comment attached to flagUpdateEnv and moved
the var itself nowhere — it is still in cmd_test.go with a copy. The comment
that belongs here is flagUpdateEnv's.

beep boop
@khvn26
test: let withSegmentOverride take the lock, like its neighbours
23ba6aa 
It mutated f.features directly while the comment two declarations above says
every field a handler reads is set through a locked setter. It was safe only
because its one caller wrapped it, which is the kind of thing that stops
being true. It locks itself now, so the callers do not have to know.

beep boop
@khvn26
test: skip directories when scanning the scripts
9bd8b36 
ReadDir returns directories too, and reading one fails. There are none in
testdata/script today, so this is about what happens when someone adds a
subdirectory rather than about a bug you can hit now.

beep boop
@khvn26
test: say which arguments a fake fixture setting wanted
969142e 
Five of these name a target and a file. Given only the target they indexed
past the end of the arguments and the script died with an index panic
instead of a message pointing at the line.

beep boop
khvn26 added 6 commits August 1, 2026 20:34
@khvn26
test: prove the declined confirmation deleted nothing
0f55f60 
The case asserted the abort message and stopped there, so it would have
passed had the delete gone out anyway. Answering the prompt with y now fails
it, which is what "nothing deleted" is supposed to mean.

beep boop
@khvn26
test: name the whole credential variable, not its prefix
adc02f3 
Three cases asserted $FLAGSMITH_API_KEY where the CLI prints the
host-scoped name in full. The prefix matches either spelling, so they would
not have noticed the scoping being dropped — which is the thing that keeps a
credential from following a redirected apiUrl.

beep boop
@khvn26
test: let the stream-separation case set up its own instance
03e667a 
Its Given describes two organisations and a project with an environment, and
it established none of them — it ran on whatever the case before it left
behind. That is the thing the Given is supposed to rule out.

beep boop
@khvn26
test: give the org-alias case the one organisation it describes
32647f6 
It said "an instance with one organisation" while running against the two
the case before it installed. The alias is what it is about, so the count
never mattered to the assertion — but the description was wrong, and a
description that can drift is worth less than none.

beep boop
@khvn26
test: drop the comments the split duplicated in cmd_test.go too
2a74ebf 
The de-duplication pass ran over fake_test.go alone, and cmd_test.go came
out of the same split with the same damage: six comments written twice, and
one left describing fakeInstance, which had moved to the other file.

The earlier pass also only matched copies separated by a single blank line.
These were separated by more, so it walked past them.

beep boop
@khvn26
test: authorisation, in the prose at least
1e9930a 
Three failure messages said authorization. The header, the OAuth endpoints
and the fake's `authorized` helper keep the spelling their specs and
identifiers give them.

beep boop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@khvn26