refactor(webapp): derive the graced-flag write routing from one table

Both global write routes carried their own copy of two answers: which flag
keys are graced, and which are server-computed. The JSON API named all four
derived keys in a destructure and both primaries in its branch condition. So
adding a graced group needed an edit in three files, and missing one would
either write an unstamped flip or accept a stamp from a request body.

Both answers now come from the group table. touchesGracedGroup and
withoutDerivedKeys are exported and used by the route, so a new group needs
no route change at all.

The global page's managed-cloud refusal moves into lockedFlagsInPayload, a
pure function, for the same reason: it encoded the locked-flag policy inline
where nothing could test it.

That is what closes the coverage gap. The routes previously held branch logic
reachable only through an authenticated request, so it went untested while
the function underneath it was well covered. The logic is now pure and tested
directly, including that only a graced PRIMARY selects the stamped path: a
body holding just a stamp must not reset a cutover clock.

Author: d-cs

apps/webapp/app/routes/admin.api.v1.feature-flags.ts

@@ -3,7 +3,12 @@ import { json } from "@remix-run/server-runtime";
 import { prisma } from "~/db.server";
 import { env } from "~/env.server";
 import { requireAdminApiRequest } from "~/services/personalAccessToken.server";
-import { applyGlobalGracedFlips, makeSetMultipleFlags } from "~/v3/featureFlags.server";
+import {
+  applyGlobalGracedFlips,
+  makeSetMultipleFlags,
+  touchesGracedGroup,
+  withoutDerivedKeys,
+} from "~/v3/featureFlags.server";
 import { validatePartialFeatureFlags } from "~/v3/featureFlags";
 
 export async function action({ request }: ActionFunctionArgs) {

apps/webapp/app/routes/admin.feature-flags.tsx

@@ -14,6 +14,7 @@ import {
   type FeatureFlagKey,
   type FlagControlType,
   getAllFlagControlTypes,
+  lockedFlagsInPayload,
   validatePartialFeatureFlags,
 } from "~/v3/featureFlags";
 import { flags as getGlobalFlags, replaceGlobalFeatureFlags } from "~/v3/featureFlags.server";
@@ -96,17 +97,12 @@ export const action = dashboardAction(
 
     const { isManagedCloud } = featuresForRequest(request);
 
-    // On managed cloud, reject if payload includes locked flags
-    if (isManagedCloud) {
-      const lockedInPayload = Object.keys(parsed.data.flags).filter((key) =>
-        GLOBAL_LOCKED_FLAGS.includes(key)
+    const lockedInPayload = lockedFlagsInPayload(Object.keys(parsed.data.flags), isManagedCloud);
+    if (lockedInPayload.length > 0) {
+      return json(
+        { error: `Cannot modify locked flags: ${lockedInPayload.join(", ")}` },
+        { status: 400 }
       );
-      if (lockedInPayload.length > 0) {
-        return json(
-          { error: `Cannot modify locked flags: ${lockedInPayload.join(", ")}` },
-          { status: 400 }
-        );
-      }
     }
 
     const validationResult = validatePartialFeatureFlags(parsed.data.flags);

apps/webapp/app/v3/featureFlags.server.ts

@@ -212,8 +212,14 @@ function gracedGroupFor(key: FeatureFlagKey) {
   return GRACED_GLOBAL_GROUPS.find((g) => g.primary === key || g.derived.includes(key));
 }
 
+// True when a save changes any graced group, and therefore needs the stamped path. Derived from
+// the group table, so adding a group cannot leave a caller silently writing an unstamped flip.
+export function touchesGracedGroup(requestedFlags: Record<string, unknown>): boolean {
+  return GRACED_GLOBAL_GROUPS.some((group) => requestedFlags[group.primary] !== undefined);
+}
+
 // Strips every derived key: a grace stamp is computed here, never accepted from a caller.
-function withoutDerivedKeys(
+export function withoutDerivedKeys(
   requestedFlags: Partial<z.infer<typeof FeatureFlagCatalogSchema>>
 ): Record<string, unknown> {
   const out: Record<string, unknown> = { ...requestedFlags };

apps/webapp/app/v3/featureFlags.ts

@@ -190,6 +190,21 @@ export const ORG_LOCKED_FLAGS: FeatureFlagKey[] = [
   FEATURE_FLAG.runOpsMintShardOverride,
 ];
 
+/**
+ * Locked flags present in a payload the global page must refuse. On managed cloud the page never
+ * offers them, so their presence means the request did not come from that page. Locally an admin
+ * may unlock and edit them, so nothing is refused.
+ */
+export function lockedFlagsInPayload(
+  payloadKeys: string[],
+  isManagedCloud: boolean
+): FeatureFlagKey[] {
+  if (!isManagedCloud) return [];
+  return payloadKeys.filter((key): key is FeatureFlagKey =>
+    GLOBAL_LOCKED_FLAGS.includes(key as FeatureFlagKey)
+  );
+}
+
 // Create a Zod schema from the existing catalog
 export const FeatureFlagCatalogSchema = z.object(FeatureFlagCatalog);
 export type FeatureFlagCatalog = z.infer<typeof FeatureFlagCatalogSchema>;

apps/webapp/test/globalFlagWriteRouting.test.ts

@@ -0,0 +1,111 @@
+// The two global write routes used to carry their own copy of "which keys are graced" and "which
+// keys are derived". A new graced group would then need an edit in three places, and missing one
+// means an unstamped flip or a stamp written straight from a request body. Both routes now derive
+// both answers from the group table, and these tests pin that. Pure, no containers.
+import { describe, expect, it } from "vitest";
+import { FEATURE_FLAG, lockedFlagsInPayload } from "~/v3/featureFlags";
+import { touchesGracedGroup, withoutDerivedKeys } from "~/v3/featureFlags.server";
+
+describe("touchesGracedGroup — decides whether a save needs the stamped path", () => {
+  it("is true for a mint-kind change", () => {
+    expect(touchesGracedGroup({ [FEATURE_FLAG.runOpsMintKind]: "runOpsId" })).toBe(true);
+  });
+
+  it("is true for a shard-list change", () => {
+    expect(touchesGracedGroup({ [FEATURE_FLAG.runOpsMintShardSet]: "a,b" })).toBe(true);
+  });
+
+  it("is false for an ordinary flag, which writes directly", () => {
+    expect(touchesGracedGroup({ [FEATURE_FLAG.mollifierEnabled]: true })).toBe(false);
+    expect(touchesGracedGroup({})).toBe(false);
+  });
+
+  it("is false when only a DERIVED key is present", () => {
+    // A body carrying only a stamp changes no group. Treating it as a flip would let a caller
+    // reset a cutover clock without touching the value the clock dates.
+    expect(touchesGracedGroup({ [FEATURE_FLAG.runOpsMintKindPrev]: "cuid" })).toBe(false);
+    expect(touchesGracedGroup({ [FEATURE_FLAG.runOpsMintShardSetPrev]: "a" })).toBe(false);
+  });
+
+  it("covers every graced primary, so a new group needs no route edit", () => {
+    // The routes no longer name these keys. If a group is added and this list is not, the next
+    // assertion fails rather than the group silently skipping the stamp.
+    const gracedPrimaries = [FEATURE_FLAG.runOpsMintKind, FEATURE_FLAG.runOpsMintShardSet];
+    for (const key of gracedPrimaries) {
+      expect(touchesGracedGroup({ [key]: "x" })).toBe(true);
+    }
+    // Every key the strip removes belongs to a group whose primary is one of the above.
+    const derived = Object.keys(
+      withoutDerivedKeys({
+        [FEATURE_FLAG.runOpsMintKindPrev]: "cuid",
+        [FEATURE_FLAG.runOpsMintKindFlippedAt]: "t",
+        [FEATURE_FLAG.runOpsMintShardSetPrev]: "a",
+        [FEATURE_FLAG.runOpsMintShardSetFlippedAt]: "t",
+      } as Record<string, unknown>)
+    );
+    expect(derived).toEqual([]);
+  });
+});
+
+describe("withoutDerivedKeys — a stamp is never taken from a request body", () => {
+  it("strips both stamps and keeps everything else", () => {
+    const out = withoutDerivedKeys({
+      [FEATURE_FLAG.runOpsMintKind]: "runOpsId",
+      [FEATURE_FLAG.runOpsMintKindPrev]: "spoofed",
+      [FEATURE_FLAG.runOpsMintKindFlippedAt]: "1999-01-01T00:00:00.000Z",
+      [FEATURE_FLAG.runOpsMintShardSet]: "a,b",
+      [FEATURE_FLAG.runOpsMintShardSetPrev]: "spoofed",
+      [FEATURE_FLAG.runOpsMintShardSetFlippedAt]: "1999-01-01T00:00:00.000Z",
+      [FEATURE_FLAG.mollifierEnabled]: true,
+    } as Record<string, unknown>);
+
+    expect(out).toEqual({
+      [FEATURE_FLAG.runOpsMintKind]: "runOpsId",
+      [FEATURE_FLAG.runOpsMintShardSet]: "a,b",
+      [FEATURE_FLAG.mollifierEnabled]: true,
+    });
+  });
+
+  it("does not mutate its input", () => {
+    const input = { [FEATURE_FLAG.runOpsMintKindPrev]: "cuid" } as Record<string, unknown>;
+    withoutDerivedKeys(input);
+    expect(input[FEATURE_FLAG.runOpsMintKindPrev]).toBe("cuid");
+  });
+});
+
+describe("lockedFlagsInPayload — what the global page refuses", () => {
+  it("refuses a locked flag on managed cloud, where the page never offers one", () => {
+    const refused = lockedFlagsInPayload(
+      [FEATURE_FLAG.taskEventRepository, FEATURE_FLAG.mollifierEnabled],
+      true
+    );
+    expect(refused).toEqual([FEATURE_FLAG.taskEventRepository]);
+  });
+
+  it("refuses the mint-shard pins, which are per-org only", () => {
+    expect(lockedFlagsInPayload([FEATURE_FLAG.runOpsMintShard], true)).toEqual([
+      FEATURE_FLAG.runOpsMintShard,
+    ]);
+    expect(lockedFlagsInPayload([FEATURE_FLAG.runOpsMintShardEnvPins], true)).toEqual([
+      FEATURE_FLAG.runOpsMintShardEnvPins,
+    ]);
+  });
+
+  it("refuses a grace stamp, which the server owns", () => {
+    expect(lockedFlagsInPayload([FEATURE_FLAG.runOpsMintShardSetFlippedAt], true)).toEqual([
+      FEATURE_FLAG.runOpsMintShardSetFlippedAt,
+    ]);
+  });
+
+  it("allows the shard list, because that is the page's ramp lever", () => {
+    expect(lockedFlagsInPayload([FEATURE_FLAG.runOpsMintShardSet], true)).toEqual([]);
+  });
+
+  it("refuses nothing when not managed cloud, where an admin may unlock and edit", () => {
+    expect(lockedFlagsInPayload([FEATURE_FLAG.taskEventRepository], false)).toEqual([]);
+  });
+
+  it("refuses nothing for an empty payload", () => {
+    expect(lockedFlagsInPayload([], true)).toEqual([]);
+  });
+});