feat: add database-driven feature flag library (OHE-3101) - #217
Conversation
OHE-3101 [Hardening Week] Add a first-class feature flag library for safer releases
Proposed solutions: adopt before buildingPrefer an established open-source library/control plane rather than implementing feature flags from scratch.
Recommended starting point: use OpenFeature as the typed application API, then evaluate PostHog, Unleash, GrowthBook, or Flagsmith as the runtime provider/control plane. The Hardening Week deliverable should include a short build-vs-adopt decision and a small proof of concept with the leading option. SummaryOpenHands does not have a shared release-safety feature flag contract across Agent Canvas, backend services, SaaS, and self-hosted deployments. Flags currently appear as hardcoded web-client booleans, This is a good Hardening Week candidate because an initial library integration, safe provider contract, tests, release runbook, and two representative migrations are bounded work that improves every later release. ContextAudit performed against
Related but not duplicate:
Required propertiesWhichever library/provider is selected must support:
A client-visible flag must never be treated as authorization. Server endpoints must continue enforcing authentication, permission, entitlement, and data access independently. Potential implementation from scratchThe following is context and a fallback design if no existing library satisfies the requirements. It is not the preferred first choice. Typed registryconst flags = defineFeatureFlags({
hostedVscode: {
type: "boolean",
default: false,
owner: "agent-canvas",
failurePolicy: "use-default",
expiresAt: "2026-10-01",
},
newConversationFlow: {
type: "variant",
variants: ["control", "candidate"],
default: "control",
owner: "conversation-platform",
failurePolicy: "use-default",
expiresAt: "2026-10-01",
},
});Product code would consume a provider-neutral API: Provider adapters could include static defaults, test overrides, deployment configuration, backend-evaluated snapshots, and PostHog or another remote provider. Evaluation rules
Feature availability should compose separate concerns: Lifecycle
Bounded Hardening Week scope
Acceptance criteria
This issue was updated by an AI agent (OpenHands) on behalf of the user. |
Add a first-class feature flag system modeled on the existing user_authorizations whitelist/blacklist pattern: - FeatureFlag + FeatureFlagRule storage models with targeting by user_id, org_id, email_pattern (SQL LIKE), and percentage rollout - FeatureFlagStore async store mirroring user_authorization_store's _internal(session)/public(session=None) overload pattern - FeatureFlagService evaluator with exclude-before-include precedence (generalizes whitelist-beats-blacklist), deterministic percentage bucketing, and a short-TTL in-memory cache - Admin REST API at /api/admin/feature-flags gated by a new MANAGE_FEATURE_FLAGS permission granted only to the superadmin role - Alembic migration 150 (chained off 149, the current origin head) - 46 unit tests (store, service, routes) Co-authored-by: openhands <openhands@all-hands.dev>
… conv limit) origin/main landed migration 150_add_daily_conversation_limit after this branch was cut, so the feature-flags migration collided on revision 150. Renumber to 151 and chain off the new 150 head. Co-authored-by: openhands <openhands@all-hands.dev>
b74084e to
4a335f8
Compare
|
|
Resolve merge conflict in enterprise/server/auth/authorization.py by keeping both the new MANAGE_ORG_QUOTA permission (from main) and the MANAGE_FEATURE_FLAGS permission (from this branch), and adding both to the super-role permissions set. Renumber the feature-flags migration 151 -> 154 to chain after the new main migrations (151 org daily conv limit, 152 quota increase request, 153 kimi->deepseek settings migration). Single alembic head is now 154. Co-authored-by: openhands <openhands@all-hands.dev>
d6eae8b to
2334d35
Compare
…pose global flags in web-client config
Targeted rules (per-user/per-org/per-email exclude or include) previously
matched an anonymous context because the dimension check was skipped when
the context value was None. That meant a per-user exclude rule silently
excluded unauthenticated callers and a per-user include silently granted
them -- leaking targeting state on anonymous paths such as
/api/web-client/config.
A populated rule dimension now requires a populated context value to match,
in both the service's Python mirror (_rule_matches_context) and the store's
SQL pre-filter (_get_matching_rules). Fully-blank rules (and percentage-only
rules with no targeting) still match anonymous callers, so only genuinely
global flags reach unauthenticated paths.
Add FeatureFlagService.get_global_flags(), which returns the subset of flags
with NO rules at all as {key: enabled}, cached with the same TTL as
is_enabled and invalidated on any flag mutation. This is the safe set for
anonymous contexts.
Wire the global flags into the unauthenticated /api/web-client/config
response via a new db_feature_flags: dict[str, bool] field on WebClientConfig.
The injector loads them through a lazy, best-effort import of the enterprise
FeatureFlagService, so OSS installs and SaaS installs that have not applied
the feature-flag migration get an empty map and the config endpoint never
breaks.
Co-authored-by: openhands <openhands@all-hands.dev>
get_global_flags() previously reused the 5s per-flag is_enabled TTL, so the unauthenticated /api/web-client/config endpoint hit the DB ~12x/min per worker process. The global set is only the rule-less flags, which change rarely, and the endpoint is called on every page load -- so a longer TTL is safe and desirable. Add a separate _DEFAULT_GLOBAL_CACHE_TTL_SECONDS = 60 (configurable via the new global_cache_ttl_seconds constructor arg, independent of the per-flag cache_ttl_seconds). The per-flag is_enabled path stays at 5s so the admin evaluate endpoint still reflects mutations quickly. invalidate() continues to drop the global snapshot immediately on admin mutation. This caps the DB load at roughly one refresh per minute per worker process. True once-per-node deduplication across multiple workers would need a shared cache and is left as a follow-up. Co-authored-by: openhands <openhands@all-hands.dev>
Fixes the "Lint python" and "Lint enterprise python" pre-commit failures: - OSS test: add missing `import pytest` (F821 on @pytest.mark.asyncio), add trailing newline (end-of-file-fixer), wrap long patch.dict lines - Enterprise: convert double-quoted strings to single quotes to match the enterprise ruff config (flake8-quotes inline-quotes = single), and apply ruff format reformatting All affected files now pass `ruff check` and `ruff format --check` under their respective configs (pyproject.toml for OSS, enterprise ruff.toml for enterprise). Co-authored-by: openhands <openhands@all-hands.dev>
The "Lint enterprise python" pre-commit runs ruff --all-files, so the pre-existing double-quoted strings in test_feature_flags.py also needed converting to single quotes to satisfy the enterprise ruff config. Co-authored-by: openhands <openhands@all-hands.dev>
When a known flag key (e.g. ENABLE_BILLING) has no FeatureFlag database row, resolve it from its environment variable instead of returning False. This lets an operator toggle an env-backed flag before/without promoting it to a DB-managed flag, and lets the web-client config endpoint surface it. Behavior: - A registered flag with no DB row -> env var truthiness (allow-all when "true", deny-all otherwise), falling back to the registered default when the env var is unset. - An unknown/unregistered flag with no DB row -> False (unchanged; absence of a DB row never implicitly grants anything). - Once a DB row exists, the database is authoritative and the env fallback is ignored -- so promoting an env-backed flag to DB management is a clean takeover with no precedence fight. Adds: - _ENV_FLAG_DEFAULTS registry + _EnvFlagDefault, seeded with ENABLE_BILLING. - FeatureFlagService.register_env_default() classmethod to register more env-backed flags at startup. - _env_fallback(key) helper resolving the env value. - is_enabled(): missing flag -> _env_fallback(key) instead of False. - get_global_flags(): include env-fallback flags absent from the DB, since they are inherently global (no per-user targeting). - Tests for: env true/false/unset, unknown flag, DB-row-overrides-env, default-true registration, and get_global_flags inclusion / non-duplication. Co-authored-by: openhands <openhands@all-hands.dev>
Fixes the 6 failing tests reported on PR #217: 1. Five TestGetGlobalFlags tests broke because the env-var fallback now injects the seeded ENABLE_BILLING into get_global_flags() whenever it is absent from the (mocked) DB, so exact-equality assertions gained an extra key. Add a module-level autouse fixture that snapshots and clears the process-global _ENV_FLAG_DEFAULTS registry (and the relevant env vars) for each test, so DB-only tests run against a clean registry. The env-fallback feature tests re-seed ENABLE_BILLING via a class autouse fixture that depends on the isolation fixture (so it runs after the clear). 2. test_super_role_permissions_are_explicit asserted the ADMIN super-role permission set did not include MANAGE_FEATURE_FLAGS, but the PR added that permission to SUPER_ROLE_PERMISSIONS[RoleName.ADMIN] in authorization.py (it is correctly super-role-only -- not in the org-scoped ROLE_PERMISSIONS). Update the expected frozenset to include Permission.MANAGE_FEATURE_FLAGS. Co-authored-by: openhands <openhands@all-hands.dev>
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
HUMAN:
I have tested this in my local environment.
AGENT:
Why
There is no first-class feature flag mechanism in the application today; gating is done with env-var toggles and ad-hoc config. OHE-3101 asks for a database-driven, no-external-service flag library with user/org/email targeting, rule-based includes/excludes, and REST administration.
Surveying the Python ecosystem, the mature flag libraries (Unleash, Flagsmith, GrowthBook) all assume a separate flag-delivery service, which conflicts with the "no external service" requirement. This PR implements a bespoke library instead, modeled directly on the existing
user_authorizationswhitelist/blacklist pattern — so it's a natural generalization of a pattern the team already maintains.Summary
FeatureFlag+FeatureFlagRulestorage models with targeting byuser_id,org_id,email_pattern(SQL LIKE), and percentage rollout, plus an async store mirroringuser_authorization_store's_internal(session)/public(session=None)overload pattern.FeatureFlagServiceevaluator with exclude-before-include precedence (generalizes whitelist-beats-blacklist), deterministic percentage bucketing (sha256(flag+user)), and a short-TTL in-memory cache./api/admin/feature-flags(CRUD for flags + rules, plus an evaluate endpoint) gated by a newMANAGE_FEATURE_FLAGSpermission granted only to thesuperadminsuper role.150(chained off149, the current origin head) and 46 unit tests across store/service/routes.Issue Number
OHE-3101
How to Test
cd enterprise && poetry install --with dev,testPYTHONPATH=".:$PYTHONPATH" poetry run pytest enterprise/tests/unit/storage/test_feature_flag_store.py enterprise/tests/unit/server/services/test_feature_flag_service.py enterprise/tests/unit/server/routes/test_feature_flags.py150against a dev DB (alembic upgrade head), then exercise the REST endpoints (requires a superadmin caller):POST /api/admin/feature-flags→ create a flagPOST /api/admin/feature-flags/{key}/rules→ add an include/exclude rulePOST /api/admin/feature-flags/{key}/evaluate→ preview a contextVideo/Screenshots
N/A — backend-only change; behavior verified via the unit test suite (46 passing).
Type
Notes
150(not148) because148and149already landed onorigin/mainwhile this was in progress.MANAGE_FEATURE_FLAGSpermission is granted only to thesuperadminsuper role (parallel toMANAGE_SUPER_ADMINS); no org-scoped role can reach these routes.ondelete=CASCADEby default.Enterprise server image for this PR: