feat!: flagsmith eval - #53
Conversation
Evaluation runs through the Flagsmith Go SDK against the SDK API, so the answer is the one an application would get: segment overrides applied, variants allocated, an identity and its traits overlaid. The SDK evaluates over the CLI's own transport, which brings retries, credential-stripping redirects and FLAGSMITH_DEBUG tracing with it. Its User-Agent is now stamped over whatever a request carries rather than only filling in a missing one, so a library that sets its own still reaches the API as the CLI. Its logger is discarded — it writes debug lines straight to os.Stderr, past the command's writers. Identity evaluation is a read-only what-if: `transient: true` unless --persist says otherwise, so neither the identity nor the traits are stored. Traits without an identity evaluate the anonymous identity. The environment context may name the environment rather than carry its key, for `evaluate` and `api --sdk` alike, as it does for every Admin command. The local name cache turns a name into a key for free, so the common case reaches the SDK API with no Admin credential at all; only a name the cache has never seen costs a lookup, and that lookup seeds the cache. A ref of 22 alphanumerics is taken for a key without consulting anything — that is what the backend mints, and a key nothing has cached yet must not need credentials. The cache spans an instance rather than a project, so only a unique name match short-circuits; anything else defers to the Admin API, which scopes the name properly. beep boop
`evaluate --json` now emits the result contract every Flagsmith SDK generates its result type from — sdk/evaluation-result.json — with a `$schema` naming it, so what the CLI prints and what an SDK resolves are the same document. It is a subset for now: `segments` and per-flag `reason` are required by the schema but returned by no SDK API endpoint, and `variant` is on the wire for an identity evaluation but discarded by the Go SDK before we see it. All three are absent rather than invented, and arrive as they become available. `--js` writes what a frontend SDK hydrates from instead: `api` and `flags`, and deliberately nothing else. An `evaluationContext` there would have to carry the environment key — a secret when it is a server-side one, and an app passes its own anyway — and `evaluationEvent`, `ts` and `identity` are bookkeeping an SDK fills in for itself. An empty environment still writes its state, with a warning: an SDK given no flags waits for a fetch a hydrating app never makes. Naming a feature prints that flag alone, so `--jq .value` reads it. `--js` describes a whole environment and refuses one: a single-flag state would tell an app the environment has a single flag. RFC §12 updated to match. beep boop
`flagsmith eval <feature> --test` turns a lookup into a check: a disabled flag becomes a failure like any other — exit 1, reason on stderr — so a script can gate on it with `&& `. The flag still prints first, so whoever ran the check can see the state that failed it. The error names the flag and nothing else. Everywhere else the CLI would name the environment too, but eval never resolves one: it holds only the key, which is a secret when it is a server-side one and must stay out of stderr and CI logs. It carries no hint either — a disabled flag is the answer, not something to recover from. RFC §12 updated to match. beep boop
Also drops the Design section: it linked twelve docs/design/*.md files that 92c69d6 deleted when the Notion RFC became the single source of truth. beep boop
Every command that addresses one identity now takes `-i` — `flag get`, `flag update`, `flag delete`, `flag enable`, `flag disable`, and `eval`. `eval` also accepts `--identifier` as a hidden alias, because the flag commands spell it that way and nobody should have to remember which command uses which. `flag list --identity` becomes `--identities`. It is a switch selecting a dimension to list, not a value naming one, and the plural says so; `--segment <id>` stays singular because it does name one. The old spelling taught the wrong shape — `flag list --identity user-1` parses `--identity` as the boolean it is and leaves `user-1` behind as a positional. Which `flag list` then silently ignored, listing every flag in the environment: a command with no Args validator inherits cobra's ArbitraryArgs. It now refuses a stray argument and exits 2. RFC §7.3 updated to match. beep boop
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new Estimated code review effort: 4 (Complex) | ~45 minutes ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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. Comment |
The SDK was left recorded as an indirect dependency: it was added with `go get` after a `go mod tidy` had dropped it, at a point when nothing imported it yet, and never re-tidied once `internal/cmd/evaluate.go` did. Tidying also records the three transitive checksums that were missing from go.sum, which is what CI's `go mod tidy -diff` was failing on. beep boop
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
52-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
--sdk-api-urlalongside--api-url.
evaltalks to the SDK API, and the new error hints point users at--sdk-api-url/sdkApiUrl, but the Conventions section only names--api-url/FLAGSMITH_API_URL. Self-hosted users following the README will miss the setting the new command depends on.📝 Proposed addition
-- Self-hosted: `--api-url` or `FLAGSMITH_API_URL`. +- Self-hosted: `--api-url` or `FLAGSMITH_API_URL`; `--sdk-api-url` (or `sdkApiUrl`) for the SDK API that `eval` evaluates against.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9d25ff54-7e37-44a8-8b8f-36b8ed90c1f3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
README.mdgo.modinternal/cmd/api.gointernal/cmd/cmd_test.gointernal/cmd/errors.gointernal/cmd/evaluate.gointernal/cmd/flag_update.gointernal/cmd/flags.gointernal/cmd/resolve.gointernal/httpx/client.gointernal/httpx/client_test.go
| t.Run("the invocation deadline bounds evaluation", func(t *testing.T) { | ||
| // Given an SDK API slower than the deadline the user asked for. | ||
| f := evalEnv(t) | ||
| f.sdkDelay = 1200 * time.Millisecond | ||
| t.Setenv("FLAGSMITH_TIMEOUT", "1") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unsynchronised SDK fixture mutations in the new evaluation tests. The new SDK handlers (lines 1309-1350) read sdkStatus, sdkDelay, sdkEnvFlags and sdkIdentityFlags under f.mu, but the new tests set them from the test goroutine without the lock — the same omission repeated in three places. Either guard each write with f.mu, or add a small f.configureSDK(...) helper that does it once.
internal/cmd/cmd_test.go#L6020-L6024: wrap thef.sdkDelayassignment inf.mu.Lock()/Unlock().internal/cmd/cmd_test.go#L6394-L6397: wrap thef.sdkStatusassignment inf.mu.Lock()/Unlock().internal/cmd/cmd_test.go#L6429-L6430: wrap thesdkEnvFlagsdelete/assign (and thesdkIdentityFlagswrites inTestEvaluateIdentity) inf.mu.Lock()/Unlock().
📍 Affects 1 file
internal/cmd/cmd_test.go#L6020-L6024(this comment)internal/cmd/cmd_test.go#L6394-L6397internal/cmd/cmd_test.go#L6429-L6430
There was a problem hiding this comment.
I don't care about broken concurrency in test code. --race should ensure it's adequate.
There was a problem hiding this comment.
@khvn26, understood. I’ll treat race-detector coverage as sufficient here and won’t pursue test-only synchronization changes.
🐇 hops quietly onward
✏️ Learnings added
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: For the Flagsmith CLI repository, khvn26 considers `go test --race` adequate for test-only concurrency concerns and does not want additional synchronization changes solely for test fixtures.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
newSDKClient rebuilt the string sdkAPIBase already returns, so the URL the SDK evaluates against and the `api` field `--js` writes could have drifted apart — including the trailing slash, which the JS SDK concatenates onto without normalising. beep boop
captureOSStderr swapped os.Stderr back on the happy path only. A t.Fatal inside the captured func — which is exactly what the callers do on error — leaves the process's stderr pointing at a closed pipe for the rest of the package run, hiding the output of every test after it. beep boop
…output The subtest checked the command's output and the hint. The output does contain the rendered error — cobra prints it to the same buffer — but asserting on err.Error() directly stops the guarantee resting on that. beep boop
Only the flags endpoint slept, so a deadline test written against the identity path would have passed without ever exercising the delay it set. beep boop
1 participant
Closes #48.
flagsmith eval(alsoevaluate) shows what a Flagsmith SDK would return for the current environment.RFC §12.
Notable changes
flag list --identitybecomes--identities, andflag listrefuses a stray positional instead of ignoring it-iaddresses the identity on every command that takes one:flag get,flag update,flag delete,flag enable,flag disable,eval.eval --identityalso accepts--identifier, hidden, since the flag commands spell it that wayUser-Agentcannot be overridden