Skip to content

Add a repository property for disabling overlay #3507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
henrymercer merged 9 commits into from
Feb 26, 2026
Merged

Add a repository property for disabling overlay #3507

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7ea93ee
Add support for boolean repository properties
henrymercer Feb 24, 2026
ed39a1e
Add repository property for disabling overlay
henrymercer Feb 24, 2026
2808ca7
Improve validation and address review comments
henrymercer Feb 24, 2026
9c61a2d
Reorganize properties file
henrymercer Feb 25, 2026
70db156
Add diagnostic when overlay disabled by repo property
henrymercer Feb 25, 2026
1824278
Add disabled reason
henrymercer Feb 25, 2026
445a2a9
Record overlay disablement reason
henrymercer Feb 25, 2026
056b091
Merge branch 'main' into henrymercer/overlay-repo-property
henrymercer Feb 25, 2026
4068616
Merge branch 'main' into henrymercer/overlay-repo-property
henrymercer Feb 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions lib/init-action.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/feature-flags/properties.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ test("loadPropertiesFromApi returns empty object if on GHES", async (t) => {
data: [
{ property_name: "github-codeql-extra-queries", value: "+queries" },
{ property_name: "unknown-property", value: "something" },
] satisfies properties.RepositoryProperty[],
] satisfies properties.GitHubPropertiesResponse,
});
const logger = getRunnerLogger(true);
const mockRepositoryNwo = parseRepositoryNwo("owner/repo");
Expand All @@ -82,7 +82,7 @@ test("loadPropertiesFromApi loads known properties", async (t) => {
data: [
{ property_name: "github-codeql-extra-queries", value: "+queries" },
{ property_name: "unknown-property", value: "something" },
] satisfies properties.RepositoryProperty[],
] satisfies properties.GitHubPropertiesResponse,
});
const logger = getRunnerLogger(true);
const mockRepositoryNwo = parseRepositoryNwo("owner/repo");
Expand Down
47 changes: 33 additions & 14 deletions src/feature-flags/properties.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,28 +7,50 @@ import { GitHubVariant, GitHubVersion } from "../util";
* Enumerates repository property names that have some meaning to us.
*/
export enum RepositoryPropertyName {
DISABLE_OVERLAY = "github-codeql-disable-overlay",
EXTRA_QUERIES = "github-codeql-extra-queries",
}

function isKnownPropertyName(value: string): value is RepositoryPropertyName {
return Object.values(RepositoryPropertyName).includes(
value as RepositoryPropertyName,
);
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isKnownPropertyName recomputes Object.values(RepositoryPropertyName) on every call, and it's called once per remote property in loadPropertiesFromApi. Consider using a module-level Set<string> of known names (or caching the array) so the per-property lookup is O(1) without repeated allocations.

Suggested change
function isKnownPropertyName(value: string): value is RepositoryPropertyName {
return Object.values(RepositoryPropertyName).includes(
value as RepositoryPropertyName,
);
const KNOWN_REPOSITORY_PROPERTY_NAMES = new Set<string>(
Object.values(RepositoryPropertyName),
);
function isKnownPropertyName(value: string): value is RepositoryPropertyName {
return KNOWN_REPOSITORY_PROPERTY_NAMES.has(value);

Copilot uses AI. Check for mistakes.
}

type AllRepositoryProperties = {
[RepositoryPropertyName.DISABLE_OVERLAY]: boolean;
[RepositoryPropertyName.EXTRA_QUERIES]: string;
};
Comment on lines +15 to +18
Copy link
Copy Markdown
Member

@mbg mbg Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't comment on this in my initial review, but I quite like this mapping of properties to expected types and the associated machinery below.


export type RepositoryProperties = Partial<AllRepositoryProperties>;

const mapRepositoryProperties: {
[K in RepositoryPropertyName]: (value: string) => AllRepositoryProperties[K];
} = {
[RepositoryPropertyName.DISABLE_OVERLAY]: (value) => value === "true",
Copy link
Copy Markdown
Member

@mbg mbg Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Since this is new, consider making this check case-insensitive?

Copy link
Copy Markdown
Contributor Author

@henrymercer henrymercer Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is consistent with other boolean checks, e.g. in environment variables. Also the repository properties UI has support for boolean properties, which will be set to either "true" or "false".

Copy link
Copy Markdown
Member

@mbg mbg Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that it is consistent, but is it intuitive? Do we gain anything from being this strict here?

For the repo properties UI, doesn't that require each org to individually configure the property as a boolean? If so, it's easy to imagine someone accidentally configuring it as a string and then using True etc.

Copy link
Copy Markdown
Contributor Author

@henrymercer henrymercer Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a valid point, but on the flip side if we allow True (or 1, etc) here, it's surprising when that isn't accepted elsewhere. We can make it clear in our instructions that this should be configured as a boolean property.

[RepositoryPropertyName.EXTRA_QUERIES]: (value) => value,
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New behavior maps DISABLE_OVERLAY from the API string value to a boolean (value === "true"), but there is no unit test exercising this parsing/mapping in loadPropertiesFromApi. Add a test case that returns github-codeql-disable-overlay from the mocked API and asserts the resulting RepositoryProperties contains the correct boolean.

Copilot uses AI. Check for mistakes.
};

function setProperty<K extends RepositoryPropertyName>(
properties: RepositoryProperties,
name: K,
value: string,
): void {
properties[name] = mapRepositoryProperties[name](value);
}

/**
* A repository property has a name and a value.
*/
export interface RepositoryProperty {
interface GitHubRepositoryProperty {
property_name: string;
value: string;
}

/**
* The API returns a list of `RepositoryProperty` objects.
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JSDoc still says the API returns a list of RepositoryProperty objects, but that type no longer exists and the response type is now GitHubRepositoryProperty[]/GitHubPropertiesResponse. Update the comment to match the actual types to avoid confusion for future maintainers.

Suggested change
* The API returns a list of `RepositoryProperty` objects.
* The API returns `GitHubPropertiesResponse`, a list of `GitHubRepositoryProperty` objects.

Copilot uses AI. Check for mistakes.
*/
type GitHubPropertiesResponse = RepositoryProperty[];

/**
* A partial mapping from `RepositoryPropertyName` to values.
*/
export type RepositoryProperties = Partial<
Record<RepositoryPropertyName, string>
>;
export type GitHubPropertiesResponse = GitHubRepositoryProperty[];

/**
* Retrieves all known repository properties from the API.
Expand Down Expand Up @@ -62,7 +84,6 @@ export async function loadPropertiesFromApi(
`Retrieved ${remoteProperties.length} repository properties: ${remoteProperties.map((p) => p.property_name).join(", ")}`,
);

const knownProperties = new Set(Object.values(RepositoryPropertyName));
const properties: RepositoryProperties = {};
for (const property of remoteProperties) {
if (property.property_name === undefined) {
Expand All @@ -71,10 +92,8 @@ export async function loadPropertiesFromApi(
);
}

if (
knownProperties.has(property.property_name as RepositoryPropertyName)
) {
properties[property.property_name] = property.value;
if (isKnownPropertyName(property.property_name)) {
setProperty(properties, property.property_name, property.value);
}
}

Expand Down