improvement(feature-flags): make flag names a closed set so every flag requires a fallback secret

Author: TheodoreSpeaks

apps/sim/lib/core/config/feature-flags.test.ts

@@ -2,7 +2,11 @@
  * @vitest-environment node
  */
 import { beforeEach, describe, expect, it, vi } from 'vitest'
-import type { FeatureFlagsConfig } from '@/lib/core/config/feature-flags'
+import type {
+  FeatureFlagContext,
+  FeatureFlagName,
+  FeatureFlagsConfig,
+} from '@/lib/core/config/feature-flags'
 
 const { mockFetch, mockIsPlatformAdmin, envRef, flagRef } = vi.hoisted(() => ({
   mockFetch: vi.fn(),
@@ -42,6 +46,14 @@ function withAppConfig(doc: unknown) {
   mockFetch.mockImplementation((_ids, parse) => Promise.resolve(parse(doc)))
 }
 
+/**
+ * `isFeatureEnabled` only accepts registered `FeatureFlagName`s. The registry is
+ * empty in this PR, so tests reference flags through the AppConfig document and
+ * cast their throwaway names through this helper.
+ */
+const enabled = (flag: string, ctx?: FeatureFlagContext) =>
+  isFeatureEnabled(flag as FeatureFlagName, ctx)
+
 describe('getFeatureFlags', () => {
   beforeEach(() => {
     vi.clearAllMocks()
@@ -94,62 +106,62 @@ describe('isFeatureEnabled', () => {
 
   it('returns false for an unknown flag', async () => {
     withAppConfig({ flags: {} })
-    expect(await isFeatureEnabled('missing', { userId: 'u1' })).toBe(false)
+    expect(await enabled('missing', { userId: 'u1' })).toBe(false)
   })
 
   it('matches the global enabled clause', async () => {
     withAppConfig({ flags: { f: { enabled: true } } })
-    expect(await isFeatureEnabled('f')).toBe(true)
+    expect(await enabled('f')).toBe(true)
   })
 
   it('matches the userId allowlist', async () => {
     withAppConfig({ flags: { f: { userIds: ['u1'] } } })
-    expect(await isFeatureEnabled('f', { userId: 'u1' })).toBe(true)
-    expect(await isFeatureEnabled('f', { userId: 'u2' })).toBe(false)
-    expect(await isFeatureEnabled('f', {})).toBe(false)
+    expect(await enabled('f', { userId: 'u1' })).toBe(true)
+    expect(await enabled('f', { userId: 'u2' })).toBe(false)
+    expect(await enabled('f', {})).toBe(false)
   })
 
   it('matches the orgId allowlist', async () => {
     withAppConfig({ flags: { f: { orgIds: ['o1'] } } })
-    expect(await isFeatureEnabled('f', { orgId: 'o1' })).toBe(true)
-    expect(await isFeatureEnabled('f', { orgId: 'o2' })).toBe(false)
+    expect(await enabled('f', { orgId: 'o1' })).toBe(true)
+    expect(await enabled('f', { orgId: 'o2' })).toBe(false)
   })
 
   describe('admin clause (lazy resolution)', () => {
     it('resolves admin from userId when admins is the deciding clause', async () => {
       withAppConfig({ flags: { f: { admins: true } } })
       mockIsPlatformAdmin.mockResolvedValue(true)
-      expect(await isFeatureEnabled('f', { userId: 'u1' })).toBe(true)
+      expect(await enabled('f', { userId: 'u1' })).toBe(true)
       expect(mockIsPlatformAdmin).toHaveBeenCalledWith('u1')
 
       mockIsPlatformAdmin.mockResolvedValue(false)
-      expect(await isFeatureEnabled('f', { userId: 'u2' })).toBe(false)
+      expect(await enabled('f', { userId: 'u2' })).toBe(false)
     })
 
     it('uses the isAdmin override without querying', async () => {
       withAppConfig({ flags: { f: { admins: true } } })
-      expect(await isFeatureEnabled('f', { userId: 'u1', isAdmin: true })).toBe(true)
+      expect(await enabled('f', { userId: 'u1', isAdmin: true })).toBe(true)
       expect(mockIsPlatformAdmin).not.toHaveBeenCalled()
     })
 
     it('resolves to false without querying when userId is absent', async () => {
       withAppConfig({ flags: { f: { admins: true } } })
-      expect(await isFeatureEnabled('f', { orgId: 'o1' })).toBe(false)
+      expect(await enabled('f', { orgId: 'o1' })).toBe(false)
       expect(mockIsPlatformAdmin).not.toHaveBeenCalled()
     })
 
     it('does not query when an earlier clause already matched', async () => {
       withAppConfig({ flags: { f: { enabled: true, admins: true } } })
-      expect(await isFeatureEnabled('f', { userId: 'u1' })).toBe(true)
+      expect(await enabled('f', { userId: 'u1' })).toBe(true)
 
       withAppConfig({ flags: { g: { userIds: ['u1'], admins: true } } })
-      expect(await isFeatureEnabled('g', { userId: 'u1' })).toBe(true)
+      expect(await enabled('g', { userId: 'u1' })).toBe(true)
       expect(mockIsPlatformAdmin).not.toHaveBeenCalled()
     })
 
     it('does not query when the rule has no admins clause', async () => {
       withAppConfig({ flags: { f: { userIds: ['u2'] } } })
-      expect(await isFeatureEnabled('f', { userId: 'u1' })).toBe(false)
+      expect(await enabled('f', { userId: 'u1' })).toBe(false)
       expect(mockIsPlatformAdmin).not.toHaveBeenCalled()
     })
   })

apps/sim/lib/core/config/feature-flags.ts

@@ -46,14 +46,24 @@ export interface FeatureFlagContext {
  * admin access from a code literal. To add a flag, register its name and the secret
  * to fall back on.
  */
-const FEATURE_FLAG_FALLBACKS: Record<string, () => string | boolean | number | undefined> = {
+type FallbackSecret = () => string | boolean | number | undefined
+
+const FEATURE_FLAG_FALLBACKS = {
   // 'new-canvas': () => env.NEW_CANVAS_ENABLED,
-}
+} satisfies Record<string, FallbackSecret>
+
+/**
+ * The closed set of known feature flags. Derived from the fallback registry, so a
+ * flag cannot exist — or be checked — without a mandatory fallback secret.
+ */
+export type FeatureFlagName = keyof typeof FEATURE_FLAG_FALLBACKS
 
 /** Build the fallback document from each flag's secret. Truthy secret ⇒ enabled. */
 function fallbackFlags(): FeatureFlagsConfig {
   const flags: Record<string, FeatureFlagRule> = {}
-  for (const [name, readSecret] of Object.entries(FEATURE_FLAG_FALLBACKS)) {
+  for (const [name, readSecret] of Object.entries(FEATURE_FLAG_FALLBACKS) as Array<
+    [string, FallbackSecret]
+  >) {
     flags[name] = { enabled: isTruthy(readSecret()) }
   }
   return { flags }
@@ -145,7 +155,7 @@ export async function getFeatureFlags(): Promise<FeatureFlagsConfig> {
 
 /** Resolve a single flag for a context. Admin status is resolved internally from `userId`. */
 export async function isFeatureEnabled(
-  flag: string,
+  flag: FeatureFlagName,
   ctx: FeatureFlagContext = {}
 ): Promise<boolean> {
   const { flags } = await getFeatureFlags()