feat(flags): feature flags Phase 1 — backend, CLI, SDK and console UI - #526
Merged
Conversation
Two tables mirroring the env_vars/env_var_environments split: the definition is project-scoped, the value is overridden per environment. Three columns ship without a Phase 1 reader because each is expensive or impossible to introduce correctly later: - salt: percentage bucketing must be stable from the first rollout, so a salt added after subjects are assigned would reshuffle every live experiment - client_visible: defaults to false; flipping a security default later would expose server-only flags that predate the change - rules: the evaluator reserves an ordered targeting step between the kill switch and the environment value, so writing rules later cannot change what an existing flag serves salt is #[serde(skip_serializing)] so it cannot leak even if the entity is ever returned directly - a published salt lets a client pre-compute its own bucket and self-select into a rollout. Includes an up/down/up reversibility test: the tables are FK-linked, so dropping them in the wrong order fails, and a half-applied rollback would leave an operator unable to migrate forward.
Phase 1 of ADR-034: set a flag value per environment and flip it without a redeploy. No targeting rules yet, but the parts that are expensive to change once callers depend on them are fixed now. Resolution order is pinned, with a reserved step for targeting: 1. flag missing/archived -> caller's fallback FLAG_NOT_FOUND 2. enabled == false -> default_value DISABLED 3. -- reserved for rules -- 4. environment value set -> that value ENVIRONMENT_VALUE 5. otherwise -> default_value DEFAULT Deciding later whether a rule outranks a blanket per-environment value would silently change what existing flags serve, so it is decided now. The kill switch short-circuits ahead of targeting, so `enabled = false` keeps meaning exactly "ignore everything, serve the default" once rules land. evaluate() is pure, synchronous and total: it returns a usable value for every input and never panics. A stored value that no longer matches the flag's type degrades to the default rather than failing the caller's request. bucket() ships tested but uncalled. Once one subject is inside a percentage rollout the algorithm can never change, so it is pinned with locked vectors before anything can depend on it. Security: - Every project-scoped handler carries project_scope_guard! and project_access_guard!. The latter deliberately skips deployment tokens and delegates their confinement to the former; shipping only one left a cross-project IDOR where a token for project A could read project B's flags. Four regression tests fail if the guard is removed. - Deployment tokens are read-only: Permission::FlagsRead bridges to DeploymentTokenPermission::FlagsRead, FlagsWrite/FlagsDelete deliberately do not. A credential baked into a container must not flip a production flag. - The snapshot endpoint takes its scope from the token, never a path parameter, and returns one error for both "no such environment" and "another project's environment" so it cannot be used as an existence oracle. - Audit entries record old and new values on every mutation. A flag change is a production change with no deployment behind it, so the audit log is the only record it happened. List is paginated (default 20, max 100). The clamp is shared with the handler so total_pages cannot describe a page size the server did not serve. The snapshot endpoint is deliberately unpaginated: the SDK needs the whole set.
Lives in the existing package rather than a new one: a separate library would duplicate the same deployment-token bootstrap, and this is a few hundred lines of caching, not a product. Reads are synchronous and cost no network I/O. The whole environment's flag set lives in memory and refreshes in the background, because a per-check HTTP round trip (~200-500ms) is unusable inside a request handler. Zero configuration on Temps: TEMPS_API_URL and TEMPS_API_TOKEN are already injected into every deployment and the token pins the project and environment, so `new FlagsClient()` knows what to fetch. evaluate() is a deliberate line-for-line mirror of the Rust evaluator. If the two diverge the same flag resolves one way in the app and another on the server, so the tests mirror the Rust tests case for case. Failure is never fatal: init() does not throw, a failed refresh keeps the last good snapshot rather than reverting every flag to its fallback, and lastError is exposed so a self-hosted operator can see that flags are stale instead of guessing why a rollout did nothing.
Full parity with the flags API, in the TypeScript client rather than the Rust binary: apps/temps-cli is the one scripting surface for the API, and the Rust binary's subcommands are server lifecycle commands. temps flags list|get|create|update|set|clear|disable|enable|archive `flags set` reads the flag first so the raw CLI string is parsed into the flag's own declared type - a shell has no types to offer, and the server rejects a mismatch. `flags disable` is the kill switch as a single command. Also registers flags with the docs generator, which uses a hardcoded import list, so the commands actually appear in CLI.md/CLI.mdx. Regenerating those picked up pre-existing drift for other commands as well.
Lives under the project rather than settings, in the sidebar directly after Environment Variables - the sibling concept it exists to contrast with (env var means redeploy, flag means seconds). The table shows one environment at a time, because the question people arrive with is "what is this flag doing in production?". The detail sheet then shows every environment at once, which is where the project/environment relationship becomes explicit. Kill switch and value are kept visually distinct: the row switch sets the value, the kill switch lives in the row menu and the sheet, and a kill-switched row renders its switch disabled with a Disabled badge. Letting someone toggle a value the kill switch is overriding would be a lie. resolveEffectiveValue() is a third mirror of the evaluator, alongside Rust and the SDK, and is commented as such - if it drifts the console shows one value while the app receives another. Type selection uses radio cards, not a dropdown (four options), and the value control adapts to the flag's type. An unrecognised type renders as text rather than a switch, which would silently mean the wrong thing.
ADR-034 records the design and, importantly, what was deliberately deferred and why. The organising principle is "fix what is expensive to change later, defer what is cheap": resolution order, the reason wire contract, typed values, the client_visible security default, the bucketing salt and key immutability are settled now; variants, rule contents, push propagation and exposure events are not. Also records three things the security audit surfaced: that project-scoped handlers need both guards, that the bucketing salt must never be serialized, and that the snapshot endpoint must not distinguish a missing environment from someone else's. The user-facing page leads with the distinction that matters - a flag is not an environment variable, and is explicitly not a place to put secrets, since flag values are stored unencrypted and can be marked browser-visible.
📓 Changelog previewThis is what your commits will add to the generated ## [Unreleased]
### Added
- **flags:** Add feature flag entities and migration
- **flags:** Add temps-flags crate with evaluation, API and RBAC
- **sdk:** Add feature flag client to @temps-sdk/node-sdk
- **cli:** Add temps flags commands
- **web:** Add feature flags console UI
- **flags:** Add last_evaluated_at backed by SDK-reported exposure
### Documentation
- **flags:** Add ADR-034 and feature documentation
- **flags:** Record the R4.5 decision on per-environment gating
### Fixed
- **flags:** Update deployment-token permission tests for FlagsRead
- **flags:** Address review findings from the security and Rust passes
- **flags:** Require SQLSTATE and "unique" together when detecting a conflict
- **flags:** Stop snapshot dropping flags overridden in other environments |
Phase 1 ships with a known gap: FlagsWrite lets a caller flip a flag in any environment of a project, production included. Accepted for now, but the shape of the fix is worth writing down so it isn't re-derived later. Resource-side elevation (a column on environments plus a stronger permission) was rejected: it invents a flag-specific authorization axis nothing else has, and puts the rule on the resource when the thing that varies is the caller. The direction is capability scoping on the credential. Deployment tokens already carry project_id and environment_id and are confined by is_scoped_to_project; API keys carry neither and fall into that function's "unconfined" branch by construction. Two consequences worth recording: all six flag handlers already call project_scope_guard!, so flags are confined for free the moment API keys gain project scoping; and environment scoping needs one new guard with exactly one flag call site (set_flag_environment). Also notes project_permission_guard! as the option that exists today - it would make flag writes respect team-based project roles via the ADR-028 seam, inert in plain OSS - in case that is wanted before credential scoping lands.
Stale-flag cleanup is core to a flag product, and it needs a real answer to
"is any running code still reading this?".
The obvious implementation is wrong. The snapshot endpoint hands the SDK every
flag in the environment and evaluation happens locally, so stamping on snapshot
fetch would mark every flag as freshly used the moment an instance boots -
including flags nothing references. The column would then actively mislead:
a recent timestamp on a dead flag, or a trusted one on a flag about to be
deleted. That is why it was left out of the first cut.
So the flag set is not the signal, the reads are. POST /flags/exposure takes
the keys an app actually evaluated; the SDK accumulates them in memory and
flushes on the refresh interval, never per call - a network round trip per
evaluation is exactly what this client exists to avoid.
Bounds that make it safe:
- The SDK only records keys present in the snapshot, so get('user-' + id) can
never grow the set; the server caps a batch regardless.
- Unknown keys are ignored silently and counted out of `recorded`, so the
endpoint cannot be used to discover which flag keys exist.
- It writes last_evaluated_at and nothing else, so "a deployment token cannot
change what a flag serves" still holds despite being a write path.
Lives on feature_flags, not feature_flag_environments: the row always exists
(a flag with no override anywhere has no environment row to stamp), and "is it
safe to delete this?" is answered across all environments at once.
The UI says "Last evaluated by an app", not "Last evaluated", and renders
Never distinctly - the whole risk is someone reading it as "last time anything
touched this flag".
Adding DeploymentTokenPermission::FlagsRead grew all() from 7 to 8 variants, which broke test_permission_all's length assertion in temps-deployments - a crate outside the ones I had been testing locally. Kept the length assertion rather than loosening it: it is what forces a new variant to be listed here, and all() is what the UI offers when scoping a token, so a variant missing from it is invisible to operators. Also adds FlagsRead to the as_str/from_str roundtrip list.
Two real conflicts, both from parallel additions:
crates/temps-migrations/src/migration/mod.rs — main added
m20260803_000001_add_template_slug_to_projects while this branch added
m20260802_000002_create_feature_flags and
m20260803_000001_add_flag_last_evaluated_at. Kept all three. The relative
order of the two m20260803 migrations is irrelevant (one alters `projects`,
the other `feature_flags`), but add_flag_last_evaluated_at must stay after
create_feature_flags since it alters that table. Verified by booting against
a live database: all four m202608* migrations apply cleanly in sequence.
web/src/api/client/{sdk,index,@tanstack/react-query}.gen.ts — generated
files, so hand-merging them would produce a client matching neither branch.
Took main's copies to clear the markers, then regenerated the whole client
from the merged server's live OpenAPI spec. The result carries both surfaces:
all five flags endpoints and main's template_slug schema.
Verified on the merged tree: cargo check --lib clean, 921 tests passing
across temps-flags/migrations/auth/entities/deployments, clippy clean,
web tsc clean, CLI tsc clean.
Two blocking issues, two majors, and the minors from both review passes. BLOCKING - deployment tokens could read every environment's flag values. The management reads return all overrides for a flag, but a token is pinned to one environment. project_access_guard! is a documented no-op for tokens, so nothing confined them: a preview container could read production values for its project. Added deny_deployment_token! to list_flags/get_flag - their whole documented machine surface is /flags/snapshot and /flags/exposure, both of which honour the pinning. The test that asserted the permissive behaviour is replaced with one asserting confinement, plus coverage that the snapshot route still works. BLOCKING - /flags/exposure was an unthrottled row-update path. The "once a minute per instance" claim was a client-side promise the server did not enforce; a loop could rewrite up to 500 rows per request on a table shared by every tenant. Now only stamps rows whose timestamp is actually stale, so steady state is zero writes. Over-cap batches are rejected rather than truncated (silent truncation left evaluated flags reading as "never"), keys longer than the column are dropped before the query, and `recorded` is the accepted count rather than rows_affected - echoing the match count let a caller probe for which keys exist. MAJOR - no semantic From<DbErr>, so the create() check-then-insert race surfaced as a 500 while losing the race a millisecond earlier gave a clean 409. Unique violations now map to DuplicateKey. MAJOR - create/update/set_flag_environment had no handler tests; removing a guard from the endpoint that flips production broke nothing in CI. Added guard coverage for all three. Note the cross-project case on writes is documented as unreachable defence-in-depth: permission_guard!(FlagsWrite) rejects tokens before the scope guard is consulted, confirmed by a sentinel run, so the comment says so rather than implying the test proves more than it does. Minors: snapshot filtered the environment inside the join instead of fetching every environment and discarding (this is the endpoint every container polls); update_flag went from three fetches to one; archive() is idempotent and no longer overwrites the original archived_at, with a restore path so an accidental archive is recoverable; migration indexes/FKs and the add-column are idempotent and down() tolerates a partial up(); the migration test asserts last_evaluated_at is present on feature_flags, not just absent from the environments table, so the follow-up migration cannot be silently dropped; tri-state PATCH deserializers are tested (the clear-to-null mechanism that has regressed here before); ETag handles weak validators and candidate lists; SDK chunks exposure to the server cap, re-queues rejected batches and flushes on close; tokio/async-trait moved to dev-dependencies.
…onflict Found while re-verifying the review fixes. is_unique_violation matched a bare "23505" anywhere in the rendered driver error, but flag keys permit digits — a flag named `23505` could make an unrelated query failure report itself as a 409 duplicate, hiding a real 500. Now requires either the full "duplicate key value violates unique constraint" phrase or the SQLSTATE alongside the word "unique". Regression test covers the look-alike key.
…ents
Found by the security re-review, and it was a regression I introduced in the
previous pass while "optimising" the snapshot query.
`find_with_related` emits a LEFT JOIN, and the `.filter()` I added landed in
the WHERE clause, which is evaluated after the join. A flag whose only
override belongs to a different environment produced a joined row satisfying
neither `environment_id = $env` nor `id IS NULL`, so the flag was dropped from
the result entirely rather than appearing with its default value.
Confirmed against Postgres before fixing — querying environment 1 with three
flags (override in env 2, override in env 1, no overrides) returned only two:
key | environment_id
-------------------+----------------
has_override_here | 1
no_overrides |
`has_override_elsewhere` vanished. The SDK reads a missing flag as
FLAG_NOT_FOUND and serves the caller's own compiled-in fallback instead of the
operator's configured default — for a kill switch that is precisely the wrong
value, and it fails silently.
Replaced with the two-query shape `list()` already uses: fetch the flags, then
fetch their overrides filtered to the environment. Structurally immune, since
the flag set no longer depends on the join. Same verification now returns all
three flags with only the env-1 override attached.
No cross-environment leak either before or after — the failure was flags going
missing, never another environment's values being served.
Same three generated files as the previous merge — hand-merging generated output would produce a client matching neither branch, so they were taken from main to clear the markers and then regenerated from the merged server's live OpenAPI spec (658 paths, all six flag endpoints present). Note for anyone regenerating after this point: main now requires step-up MFA for sensitive actions, so `POST /api/api-keys` returns STEP_UP_REQUIRED and the usual "mint an API key for codegen" route no longer works unattended. The openapi-ts config already supports TEMPS_API_COOKIE — pass the session cookie from a normal login instead. Verified on the merged tree: cargo check --lib clean, temps-flags 76 tests passing, clippy clean, web and CLI typecheck clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 1 of ADR-034: set a flag value per environment and flip it without a redeploy. Backend, CLI, SDK and console UI.
No targeting rules or percentage rollouts yet — but the parts that are expensive to change once callers depend on them are fixed now.
Why
The only runtime knob a Temps user has today is an environment variable, and changing one triggers a redeploy. That's correct for secrets and wrong for a kill switch: by the time the container restarts, the incident has been running for two minutes.
What's in it
crates/temps-flags— new plugin crate: pure evaluator, service, 7 REST endpoints, OpenAPI, RBAC, audit loggingenv_vars/env_var_environmentssplittemps flags …in@temps-sdk/cli(full API parity)FlagsClientinside the existing@temps-sdk/node-sdk— no new packageThe design decision that matters
Resolution order is pinned now, with a reserved step for targeting:
Deciding later whether a rule outranks a blanket per-environment value would silently change what existing flags serve. And because the kill switch short-circuits ahead of targeting,
enabled = falsekeeps meaning exactly "ignore everything, serve the default" once rules land.bucket()ships tested but uncalled for the same reason: once one subject is inside a percentage rollout the algorithm can never change, so it's pinned with locked vectors before anything can depend on it.Security
A
security-auditorpass found a HIGH cross-project IDOR that I had introduced, and it is fixed in this branch.project_access_guard!deliberately skips deployment tokens (they carry no user identity for a team-membership check) and delegates their confinement toproject_scope_guard!. I had only called the former. A deployment token for project A could read project B's flags by changing the path id.Reproduced against a live instance before the fix:
After adding
project_scope_guard!to all six project-scoped handlers:Four regression tests cover it. Verified non-vacuous by removing the guard and confirming they fail:
Also addressed from the audit:
feature_flags.saltis now#[serde(skip_serializing)]— a published salt lets a client pre-compute its own bucket and self-select into a rolloutOther security properties, verified:
FlagsReadbridges;FlagsWrite/FlagsDeletedeliberately don't. Auto-minted tokens carry["*"]and still can't write.Evidence
Validation
Kill switch preserves the override
Pagination (25 flags)
total_pagesuses the same clamp the query used, so it can never describe a page size the server didn't serve. Overrides still attach to the right flag across pages; the snapshot endpoint stays unpaginated because the SDK needs the whole set.ETag
SDK against a live server
Degraded paths never throw — no credentials / 401 / unreachable host all serve fallbacks, and a failed refresh keeps the last good snapshot (
5000, not the fallback).Migration —
upon a clean database produces the expected schema; a dedicated test asserts up → down → up (the tables are FK-linked, so drop order matters and a half-applied rollback would strand an operator).Console UI — created a JSON flag through the dialog and confirmed it server-side; toggled a flag and confirmed the write; verified desktop, mobile, light and dark.
Checks
Notes for review
last_evaluated_atis SDK-reported per-flag exposure, not a snapshot-fetch stamp. Stamping on fetch would mark every flag freshly used the moment an instance boots, which is worse than having no column.POST /flags/exposuretakes the keys an app actually evaluated; the SDK batches them on the refresh interval, only records keys present in the snapshot (soget('user-' + id)can't grow the set), ignores unknown keys silently (no existence oracle), and writes only the timestamp. Verified: reportingfeature.f01stamps onlyfeature.f01, and a snapshot fetch stamps nothing. UI says "Last evaluated by an app" and rendersNeverdistinctly.resolveEffectiveValue()in the console is a third mirror of the evaluator, alongside Rust and the SDK. Commented as such; should collapse when the same-origin evaluation endpoint lands in Phase 2.@temps-sdk/node-sdkgenerated client was not regenerated — its checked-in spec is 242 paths stale and doing so would bury this change in unrelated churn.FlagsClientis hand-written and doesn't depend on it.Review round 2 — findings addressed
A security pass and a Rust pass raised two blocking issues, two majors and a set of minors. All are fixed in
ef0c222e/1819d705.Blocking — cross-environment read. An environment-scoped deployment token could read every environment's flag values via
GET /projects/{id}/flags, becauseproject_access_guard!is a documented no-op for tokens and the response carries all override rows. A preview container could therefore read production values. Fixed withdeny_deployment_token!onlist_flags/get_flag— a token's documented machine surface is/flags/snapshot+/flags/exposure, both of which honour its pinning. Verified live:Blocking — unthrottled exposure writes. The "once a minute per instance" claim was a client-side promise the server never enforced. Now only stamps rows that are actually stale:
That last line closes the existence oracle:
recordedno longer echoesrows_affected.Majors.
From<DbErr>maps unique violations toDuplicateKey, so thecreate()race returns 409 rather than 500. Guard tests added forcreate_flag/update_flag/set_flag_environment— previously removing a guard from the endpoint that flips production broke nothing in CI.One honest note: the cross-project case on the write handlers is unreachable defence-in-depth —
permission_guard!(FlagsWrite)rejects deployment tokens beforeproject_scope_guard!is consulted. A sentinel run confirmed removing the scope guard does not fail that test, so it is named and commented accordingly rather than implying it proves more than it does. The permission guard itself is covered (removing it fails two tests).Minors. Snapshot filters environment inside the join instead of fetching all and discarding;
update_flag3 fetches → 1;archive()idempotent (it was overwriting the originalarchived_at) plus arestorepath so an accidental archive is recoverable; migration indexes/FKs and the add-column idempotent,down()tolerates a partialup(); migration test assertslast_evaluated_atis present onfeature_flags; tri-state PATCH deserializers tested; ETag handles weak validators and candidate lists; SDK chunks exposure, re-queues rejected batches, flushes on close;tokio/async-traitmoved to dev-deps.A follow-up self-check then caught a bug in the fix itself:
is_unique_violationmatched a bare"23505"anywhere in the driver error, and flag keys permit digits — a flag named23505could turn an unrelated failure into a 409 hiding a 500. Tightened in1819d705with a regression test.temps-flagsis now 76 tests (was 56). CI green on1819d705.Still open
FlagsWritecan flip a prod flag. Decided and documented rather than fixed here: the fix belongs on the credential, not the resource. Deployment tokens already carryproject_id/environment_idand are confined byis_scoped_to_project; API keys carry neither and are unconfined by construction. All six flag handlers already callproject_scope_guard!, so flags are confined for free once API keys gain project scoping — environment scoping needs one new guard with a single flag call site. See "R4.5, decided" in the ADR.