Improve validation and address review comments

src/feature-flags/properties.test.ts

@@ -28,6 +28,9 @@ test("loadPropertiesFromApi throws if response data is not an array", async (t)
       logger,
       mockRepositoryNwo,
     ),
+    {
+      message: /Expected repository properties API to return an array/,
+    },
   );
 });
 
@@ -48,6 +51,9 @@ test("loadPropertiesFromApi throws if response data contains unexpected objects"
       logger,
       mockRepositoryNwo,
     ),
+    {
+      message: /Expected repository property object to have a 'property_name'/,
+    },
   );
 });
 
@@ -95,3 +101,117 @@ test("loadPropertiesFromApi loads known properties", async (t) => {
   );
   t.deepEqual(response, { "github-codeql-extra-queries": "+queries" });
 });
+
+test("loadPropertiesFromApi parses true boolean property", async (t) => {
+  sinon.stub(api, "getRepositoryProperties").resolves({
+    headers: {},
+    status: 200,
+    url: "",
+    data: [
+      {
+        property_name: "github-codeql-disable-overlay",
+        value: "true",
+      },
+      { property_name: "github-codeql-extra-queries", value: "+queries" },
+    ] satisfies properties.GitHubPropertiesResponse,
+  });
+  const logger = getRunnerLogger(true);
+  const warningSpy = sinon.spy(logger, "warning");
+  const mockRepositoryNwo = parseRepositoryNwo("owner/repo");
+  const response = await properties.loadPropertiesFromApi(
+    {
+      type: util.GitHubVariant.DOTCOM,
+    },
+    logger,
+    mockRepositoryNwo,
+  );
+  t.deepEqual(response, {
+    "github-codeql-disable-overlay": true,
+    "github-codeql-extra-queries": "+queries",
+  });
+  t.true(warningSpy.notCalled);
+});
+
+test("loadPropertiesFromApi parses false boolean property", async (t) => {
+  sinon.stub(api, "getRepositoryProperties").resolves({
+    headers: {},
+    status: 200,
+    url: "",
+    data: [
+      {
+        property_name: "github-codeql-disable-overlay",
+        value: "false",
+      },
+    ] satisfies properties.GitHubPropertiesResponse,
+  });
+  const logger = getRunnerLogger(true);
+  const warningSpy = sinon.spy(logger, "warning");
+  const mockRepositoryNwo = parseRepositoryNwo("owner/repo");
+  const response = await properties.loadPropertiesFromApi(
+    {
+      type: util.GitHubVariant.DOTCOM,
+    },
+    logger,
+    mockRepositoryNwo,
+  );
+  t.deepEqual(response, {
+    "github-codeql-disable-overlay": false,
+  });
+  t.true(warningSpy.notCalled);
+});
+
+test("loadPropertiesFromApi throws if property value is not a string", async (t) => {
+  sinon.stub(api, "getRepositoryProperties").resolves({
+    headers: {},
+    status: 200,
+    url: "",
+    data: [{ property_name: "github-codeql-extra-queries", value: 123 }],
+  });
+  const logger = getRunnerLogger(true);
+  const mockRepositoryNwo = parseRepositoryNwo("owner/repo");
+  await t.throwsAsync(
+    properties.loadPropertiesFromApi(
+      {
+        type: util.GitHubVariant.DOTCOM,
+      },
+      logger,
+      mockRepositoryNwo,
+    ),
+    {
+      message:
+        /Expected repository property 'github-codeql-extra-queries' to have a string value/,
+    },
+  );
+});
+
+test("loadPropertiesFromApi warns if boolean property has unexpected value", async (t) => {
+  sinon.stub(api, "getRepositoryProperties").resolves({
+    headers: {},
+    status: 200,
+    url: "",
+    data: [
+      {
+        property_name: "github-codeql-disable-overlay",
+        value: "yes",
+      },
+    ] satisfies properties.GitHubPropertiesResponse,
+  });
+  const logger = getRunnerLogger(true);
+  const warningSpy = sinon.spy(logger, "warning");
+  const mockRepositoryNwo = parseRepositoryNwo("owner/repo");
+  const response = await properties.loadPropertiesFromApi(
+    {
+      type: util.GitHubVariant.DOTCOM,
+    },
+    logger,
+    mockRepositoryNwo,
+  );
+  t.deepEqual(response, {
+    "github-codeql-disable-overlay": false,
+  });
+  t.true(warningSpy.calledOnce);
+  t.is(
+    warningSpy.firstCall.args[0],
+    "Repository property 'github-codeql-disable-overlay' has unexpected value 'yes'. Expected 'true' or 'false'. Defaulting to false.",
+  );
+});

src/feature-flags/properties.ts

@@ -11,10 +11,12 @@ export enum RepositoryPropertyName {
   EXTRA_QUERIES = "github-codeql-extra-queries",
 }
 
+const KNOWN_REPOSITORY_PROPERTY_NAMES = new Set<string>(
+  Object.values(RepositoryPropertyName),
+);
+
 function isKnownPropertyName(value: string): value is RepositoryPropertyName {
-  return Object.values(RepositoryPropertyName).includes(
-    value as RepositoryPropertyName,
-  );
+  return KNOWN_REPOSITORY_PROPERTY_NAMES.has(value);
 }
 
 type AllRepositoryProperties = {
@@ -24,31 +26,45 @@ type AllRepositoryProperties = {
 
 export type RepositoryProperties = Partial<AllRepositoryProperties>;
 
-const mapRepositoryProperties: {
-  [K in RepositoryPropertyName]: (value: string) => AllRepositoryProperties[K];
+/** Parsers that transform repository properties from the API response into typed values. */
+const repositoryPropertyParsers: {
+  [K in RepositoryPropertyName]: (
+    name: K,
+    value: string,
+    logger: Logger,
+  ) => AllRepositoryProperties[K];
 } = {
-  [RepositoryPropertyName.DISABLE_OVERLAY]: (value) => value === "true",
-  [RepositoryPropertyName.EXTRA_QUERIES]: (value) => value,
+  [RepositoryPropertyName.DISABLE_OVERLAY]: (name, value, logger) => {
+    if (value !== "true" && value !== "false") {
+      logger.warning(
+        `Repository property '${name}' has unexpected value '${value}'. Expected 'true' or 'false'. Defaulting to false.`,
+      );
+    }
+    return value === "true";
+  },
+  [RepositoryPropertyName.EXTRA_QUERIES]: (_name, value) => value,
 };
 
+/** Update the partial set of repository properties with the parsed value of the specified property. */
 function setProperty<K extends RepositoryPropertyName>(
   properties: RepositoryProperties,
   name: K,
   value: string,
+  logger: Logger,
 ): void {
-  properties[name] = mapRepositoryProperties[name](value);
+  properties[name] = repositoryPropertyParsers[name](name, value, logger);
 }
 
 /**
  * A repository property has a name and a value.
  */
-interface GitHubRepositoryProperty {
+export interface GitHubRepositoryProperty {
   property_name: string;
   value: string;
 }
 
 /**
- * The API returns a list of `RepositoryProperty` objects.
+ * The API returns a list of `GitHubRepositoryProperty` objects.
  */
 export type GitHubPropertiesResponse = GitHubRepositoryProperty[];
 
@@ -88,12 +104,18 @@ export async function loadPropertiesFromApi(
     for (const property of remoteProperties) {
       if (property.property_name === undefined) {
         throw new Error(
-          `Expected property object to have a 'property_name', but got: ${JSON.stringify(property)}`,
+          `Expected repository property object to have a 'property_name', but got: ${JSON.stringify(property)}`,
+        );
+      }
+
+      if (typeof property.value !== "string") {
+        throw new Error(
+          `Expected repository property '${property.property_name}' to have a string value, but got: ${JSON.stringify(property)}`,
         );
       }
 
       if (isKnownPropertyName(property.property_name)) {
-        setProperty(properties, property.property_name, property.value);
+        setProperty(properties, property.property_name, property.value, logger);
       }
     }