Basic support for overlay PR analysis by cklin · Pull Request #2945 · github/codeql-action · GitHub | Latest TMZ Celebrity News & Gossip | Watch TMZ Live
Skip to content

Basic support for overlay PR analysis #2945

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.

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

cklin
Copy link
Contributor

@cklin cklin commented Jun 23, 2025

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@cklin cklin marked this pull request as ready for review June 23, 2025 15:51
@Copilot Copilot AI review requested due to automatic review settings June 23, 2025 15:51
@cklin cklin requested a review from a team as a code owner June 23, 2025 15:51
@cklin cklin requested a review from nickrolfe June 23, 2025 15:51
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds foundational support for overlay database analysis, including caching of the overlay-base database and integration into init/analyze workflows.

  • Introduces functions to upload/download overlay-base DB to Actions cache.
  • Updates configuration logic to determine overlay modes and caching behavior.
  • Integrates overlay mode handling into init and analyze actions and feature flags.

Reviewed Changes

Copilot reviewed 28 out of 42 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/overlay-database-utils.ts Implements check, upload, and download for overlay-base DB cache
src/config-utils.ts Adds getOverlayDatabaseMode and new augmentation properties
src/init-action.ts Passes sourceRoot to init and applies cache download logic
src/analyze-action.ts Overrides cleanup level and uploads overlay-base DB to cache
src/feature-flags.ts Introduces OverlayAnalysis feature and min CLI version check
Comments suppressed due to low confidence (2)

src/overlay-database-utils.ts:170

  • The new overlay database caching functions (uploadOverlayBaseDatabaseToCache and downloadOverlayBaseDatabaseFromCache) are not covered by existing tests. Consider adding unit tests to validate their guard conditions and successful cache interactions.
export async function uploadOverlayBaseDatabaseToCache(

src/config-utils.ts:791

  • [nitpick] getOverlayDatabaseMode contains complex branching logic. Consider adding or expanding its JSDoc to outline the decision flow and environment variable precedence for better readability and future reference.
async function getOverlayDatabaseMode(

logger: Logger,
): Promise<boolean> {
const overlayDatabaseMode = config.augmentationProperties.overlayDatabaseMode;
if (overlayDatabaseMode !== OverlayDatabaseMode.OverlayBase) {
Copy link
Preview

Copilot AI Jun 23, 2025

Choose a reason for hiding this comment

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

[nitpick] The guard logic for overlay database caching (checking mode, caching flag, and test mode) is duplicated in both upload and download functions. Consider extracting this into a shared helper to reduce duplication and simplify future maintenance.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@nickrolfe nickrolfe left a comment

Choose a reason for hiding this comment

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

I have a few questions/comments, but overall it looks very sensible.

Comment on lines 848 to 858
if (buildMode !== BuildMode.None) {
logger.warning(
`Cannot build an ${overlayDatabaseMode} database because ` +
`build-mode is set to "${buildMode}" instead of "none". ` +
"Falling back to creating a normal full database instead.",
);
return nonOverlayAnalysis;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to check whether PR analyses for dynamic languages like Ruby actually have the build mode defined as None, or if it's left undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the code so that it checks for build mode only for traced languages.

"from the CODEQL_OVERLAY_DATABASE_MODE environment variable.",
);
} else if (
["github", "dsp-testing"].includes(repository.owner) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why we can't just use the feature flag to restrict overlay analysis to these orgs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR does not fully implement all the functionality that we planned, so it is considered an early prototype and suitable for internal testing only. So we want to be sure that we never attempt to perform overlay analysis on user repositories with this implementation, even if a user were to pin their workflow to this PR after merge, and then we enable all feature flags.

That is why I am using an explicit allowlist here instead of relying on feature flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to be worth a TODO comment 🙂

Copy link
Member

Choose a reason for hiding this comment

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

There is also a potential concern here for outside of dotcom if this ends up in a release, since it is possible for customers on other instances to have organisations with specific names. Two options:

  • If you want to get this merged, then I think it would be good to put this behind a FF now, which then also helps us going forward.
  • If it's really for internal testing only, then you may not need to get this PR merged at all, if you are able to reference this branch directly in testing workflows

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I only reviewing this fully now, so I think I missed that you have already added the FF and reasoned why you should have both checks. I am not sure we should really be concerned with users pinning feature branches. That's not a concern for default setup, and if they did it with an advanced setup, then they are probably aware that it is at their own risk (they could also just enable the environment variable).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this seems to be worth a TODO comment 🙂

Yes, that is a good suggestion. I will do that.

There is also a potential concern here for outside of dotcom if this ends up in a release, since it is possible for customers on other instances to have organisations with specific names.

Thanks for bringing that up—I haven't considered that possibility. That said, we do have a feature flag on top of the allow list (your option 1 above), so GHES users should never reach this check.

If it's really for internal testing only, then you may not need to get this PR merged at all, if you are able to reference this branch directly in testing workflows

The internal testing can include staff-ship to internal repositories, and it would be ideal if we can do so through feature flags without requiring workflow file changes.

I am not sure we should really be concerned with users pinning feature branches.

Once this PR merges, a user could get this code by pinning a commit on main, which I think is a use case we should support.

Copy link
Member

Choose a reason for hiding this comment

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

The internal testing can include staff-ship to internal repositories, and it would be ideal if we can do so through feature flags without requiring workflow file changes.

Agreed.

Once this PR merges, a user could get this code by pinning a commit on main, which I think is a use case we should support.

I am a bit conflicted about this. On one hand, I agree that a user could do this and the "future FF enablement on an old pinned version" scenario is an interesting one to consider. On the other hand, I am not sure how much we have considered this scenario in the past -- possibly a question for @aeisenberg or @henrymercer. I'm slightly leaning towards thinking that the FF alone is probably enough, particularly since a user who pins this version could also just set the environment variable to get this prototype implementation, but no strong feelings.

FYI we also have a FF in default setup for internal things. That isn't propagated to the action right now in any way, but it could be. Perhaps if we feel that this kind of feature gating is necessary for experimental features, then we could instead do some work to start using that.

}

function generateCacheKey(config: Config, codeQlVersion: string): string {
const sha = process.env.GITHUB_SHA || "unknown";
Copy link
Contributor

Choose a reason for hiding this comment

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

If the GITHUB_SHA env var is somehow not set, should we instead skip uploading to the actions cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. In addition to what you said, there is another reason why we should not be relying on GITHUB_SHA: the environment variable, even if it exists, may not necessarily correspond to the commit that is analyzed. It is better to call getCommitOid() instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the code to use getCommitOid().


const dbLocation = config.dbLocation;
const codeQlVersion = (await codeql.getVersion()).version;
const restoreKey = getCacheRestoreKey(config, codeQlVersion);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow how this can work, since the saveKey computation appends either a SHA or unknown to the result of getCacheRestoreKey(), and we're not appending that suffix here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code relies on the actions cache feature of performing restores using prefix match on the cache key:
https://docs.github.com/en/actions/how-tos/writing-workflows/choosing-what-your-workflow-does/caching-dependencies-to-speed-up-workflows#matching-a-cache-key

@cklin
Copy link
Contributor Author

cklin commented Jun 30, 2025

Thank you for your feedback @nickrolfe!

I will take the PR back to draft to address comments and resolve merge conflicts. Once that is done, I will mark the PR as ready for review again.

@cklin cklin marked this pull request as draft June 30, 2025 21:05
@cklin cklin force-pushed the cklin/overlay-analysis branch from fd2660c to bc3a47b Compare July 1, 2025 17:50
@cklin cklin marked this pull request as ready for review July 1, 2025 18:19
@cklin
Copy link
Contributor Author

cklin commented Jul 1, 2025

@nickrolfe The PR is ready for another look.

Comment on lines +298 to +306
// An overlay-base database should always use the 'overlay' cleanup level
// to preserve the cached intermediate results.
//
// Note that we may be overriding the 'cleanup-level' input parameter.
const cleanupLevel =
actionsUtil.getOptionalInput("cleanup-level") || "brutal";
config.augmentationProperties.overlayDatabaseMode ===
OverlayDatabaseMode.OverlayBase
? "overlay"
: actionsUtil.getOptionalInput("cleanup-level") || "brutal";
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to raise a configuration error if we explicitly set anything else than overlay or nothing if we are using overlay database? I think we shouldn't silently ignore an explicit parameter by the user

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @redsun82. It might be surprising if the cleanup-level option is silently overriden. An error if cleanup-level is set and overlay databases are enabled would make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am a bit conflicted about what to do here.

On the one hand, silently ignoring explicit user input (which I am doing here) is not considered proper behavior.

On the other hand, it is important for the action to maintain backward compatibility. If we were to throw an error for users setting cleanup-level to a value other than overlay, we would break those (currently working) workflows when we roll out overlay analysis. I can't think of any reason for users to set cleanup-level to overlay, especially since it is currently not even a documented allowed value.

Another option is to not build an overlay-base database if the user specifies a cleanup-level that is not overlay. There is no good way to do that because the decision to build the overlay-base database was made in the init step, and the cleanup-level input is in the analyze step. We could obey the cleanup-level input and not upload the overlay-base database if the input is not overlay, which means that the workflow will not benefit from overlay analysis. It becomes a matter of which we think the user would care more about: getting the benefit of overlay analysis when we roll it out, or having the cleanup-level input strictly obeyed?

Of these three options, I think the currently implemented behavior is the best choice. We can make the behavior change more explicit, by updating the cleanup-level description to state that it is advisory, and (if you consider it appropriate) to add a changelog entry stating the same. At the end of the day, the original assumption when we added cleanup-level (that database cleanup is a choice that has no impact on how the action itself functions) will no longer hold, and I think it is not unreasonable for the meaning of the input to change accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the thoughtful discussion of this!

Firstly, I am thinking that, regardless of which behaviour we go with, we should implement a warning that warns of incompatible cleanup-level and overlay database settings.

Beyond that, my feeling is that we should preserve existing behaviour, at least until we have given enough notice that a change is coming. That is to say, I'd probably have a preference for "We could obey the cleanup-level input and not upload the overlay-base database if the input is not overlay, which means that the workflow will not benefit from overlay analysis.".

I would say that is acceptable because:

  • We will have a warning which notifies the user (but not an error that breaks existing workflows).
  • We can assume that cleanup-level isn't super commonly used, and most users will still benefit from overlay databases anyway, particularly in default setup where no cleanup-level is specified.
  • We don't override user-configured behaviour.
  • Since cleanup-level is used in advanced setup, users can change this setting if they have it configured and they want to take advantage of overlay databases.

We can then, after some time, revisit this to decide whether we want to override the cleanup-level setting if overlay databases are enabled.

"from the CODEQL_OVERLAY_DATABASE_MODE environment variable.",
);
} else if (
["github", "dsp-testing"].includes(repository.owner) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to be worth a TODO comment 🙂

[Feature.OverlayAnalysis]: {
defaultValue: false,
envVar: "CODEQL_ACTION_OVERLAY_ANALYSIS",
minimumVersion: CODEQL_OVERLAY_MINIMUM_VERSION,
Copy link
Contributor

@redsun82 redsun82 Jul 2, 2025

Choose a reason for hiding this comment

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

Theoretically as stated on line 86, we should

Prefer using ToolsFeatures for future flags

Maybe we're not very thorough with that, and as far as I can see we did not add overlayDatabases to the list of CLI features that are listed in codeql version --format=json. We should have, right?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that it would be better to use the CLI's features list than set a minimumVersion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CLI interface for overlay analysis has not yet been completely finalized, which is one of the reasons why we have both the feature flag check and the explicit allowlist for internal orgs. We can use ToolsFeatures, but that would require us to come up with a new ToolsFeature if there should be an upcoming change that is not backwards compatible. Setting a minimum version number makes it easier to iterate.

There is also the question of how much we want to lean on the version or feature check. Support for overlay analysis is not just a feature of the CLI—it is a feature of the CLI and the compiled queries. ToolsFeature tells us whether we can pass the overlay analysis flags to the CLI and not get an error. But it does not tell us whether the query packs are ready for overlay analysis. By setting a minimum version number, we can version not only the CLI but also the precompiled query packs included in the bundle, which helps us avoid unintended enablement.

(In the long run, we do plan to make the CLI robust enough so that it can correctly handle existing pre-compiled query packs for overlay analysis. For now internal testing focuses on query packs included in the bundle, and the minimum version serves as a useful check.)

Copy link
Member

Choose a reason for hiding this comment

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

I think I am happy with that justification for leaving this as is for now.

nickrolfe
nickrolfe previously approved these changes Jul 2, 2025
Copy link
Contributor

@nickrolfe nickrolfe left a comment

Choose a reason for hiding this comment

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

I re-reviewed the parts I previously commented on, and I'm happy with the changes.

Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

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

Thank you for preparing this PR -- I am very excited for this functionality! 💖

As I suggested in my earlier comment, if the main goal is to test this internally for now, then it may be best to leave this PR unmerged and reference the PR branch explicitly in test workflows. Have you done any testing like that yet? If not, it would be good to do some of that in any case.

If we want to merge this, then the changes will end up in the next release. Before we do that, I think we should address a few points:

  • Before we merge this, we should add some test coverage. At this stage, it would be good to gain confidence that none of the new logic interferes with existing functionality when the feature is disabled. The existing PR checks do that to an extent, but we should verify that we cover all cases. In the long run, we will want test coverage for when the feature is enabled as well.
  • There are general concerns around us using the Actions cache, similar to what we have for TRAP caching and dependency caching. I.e. we might be eating away at the cache quota that may already be in use by other workflows in a given repository. Can we mitigate our impact here? For example, by ensuring that we only have one cache at a time or limiting the size of the caches we store. This may not need to be addressed at this point if we only plan to use this internally for now, but it should be addressed before we ship it more widely, especially if the goal is to have it on by default at some point.

* @returns the base and head branches of the pull request, or undefined if
* we are not analyzing a pull request.
*/
export function getPullRequestBranches(): PullRequestBranches | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to have unit tests for this which exercise the different conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not new code—it is existing code that was moved to allow greater reuse.

Copy link
Member

Choose a reason for hiding this comment

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

I noticed -- we probably should have added some tests back then, so now may be a good opportunity to do that.

Comment on lines +298 to +306
// An overlay-base database should always use the 'overlay' cleanup level
// to preserve the cached intermediate results.
//
// Note that we may be overriding the 'cleanup-level' input parameter.
const cleanupLevel =
actionsUtil.getOptionalInput("cleanup-level") || "brutal";
config.augmentationProperties.overlayDatabaseMode ===
OverlayDatabaseMode.OverlayBase
? "overlay"
: actionsUtil.getOptionalInput("cleanup-level") || "brutal";
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @redsun82. It might be surprising if the cleanup-level option is silently overriden. An error if cleanup-level is set and overlay databases are enabled would make sense.

Comment on lines -9 to +13
import * as actionsUtil from "./actions-util";
import {
getRequiredInput,
getTemporaryDirectory,
PullRequestBranches,
} from "./actions-util";
Copy link
Member

Choose a reason for hiding this comment

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

Minor: I note that this change results in two unrelated changes in this file. We have imports like import * as actionsUtil in a lot of places already, so I am not sure there's a good reason for changing this here as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is a compelling reason for the change either. I am not even sure the change was intentional—it might have been made by copilot or IDE auto-completion, which I then let stand because it did not add that much noise to justify the additional work to preserve the existing import style, which seems just as arbitrary.

If this becomes a concern, we might be able to configure the linter to insist on one import style or the other.

[Feature.OverlayAnalysis]: {
defaultValue: false,
envVar: "CODEQL_ACTION_OVERLAY_ANALYSIS",
minimumVersion: CODEQL_OVERLAY_MINIMUM_VERSION,
Copy link
Member

Choose a reason for hiding this comment

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

Agreed that it would be better to use the CLI's features list than set a minimumVersion here.

Comment on lines +302 to +308
const sourceRoot = path.resolve(
getRequiredEnvParam("GITHUB_WORKSPACE"),
getOptionalInput("source-root") || "",
);
Copy link
Member

Choose a reason for hiding this comment

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

It may be worth adding a short comment here to explain that resolve respects the intended semantics of source-root (relative to the workspace root by default, but can be absolute).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not new code—it is existing code that was moved to make sourceRoot available for initConfig(). But I can add the comment you requested.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Yes, I saw that it had been moved, but ultimately that doesn't change my thinking that this would be a useful thing to have!

"from the CODEQL_OVERLAY_DATABASE_MODE environment variable.",
);
} else if (
["github", "dsp-testing"].includes(repository.owner) &&
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I only reviewing this fully now, so I think I missed that you have already added the FF and reasoned why you should have both checks. I am not sure we should really be concerned with users pinning feature branches. That's not a concern for default setup, and if they did it with an advanced setup, then they are probably aware that it is at their own risk (they could also just enable the environment variable).

// we are analyzing. However, since overlay-base databases are built from the
// default branch and used in PR analysis, it is exceedingly unlikely that
// the commit SHA will ever be the same, so we can just leave it out.
const languages = [...config.languages].sort().join("_");
Copy link
Member

Choose a reason for hiding this comment

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

Henry has an open PR to open up languages more, which might affect this in the future. Not necessarily something we need to consider here, but may need to consider there.

@cklin
Copy link
Contributor Author

cklin commented Jul 2, 2025

@mbg Thank you for your review!

  • Before we merge this, we should add some test coverage. At this stage, it would be good to gain confidence that none of the new logic interferes with existing functionality when the feature is disabled. The existing PR checks do that to an extent, but we should verify that we cover all cases. In the long run, we will want test coverage for when the feature is enabled as well.

I agree that more tests are a good idea. Can you say more about what you mean by "gain confidence that none of the new logic interferes with existing functionality" and "cover all cases", so that I have a better estimate of the effort required?

  • There are general concerns around us using the Actions cache, similar to what we have for TRAP caching and dependency caching. I.e. we might be eating away at the cache quota that may already be in use by other workflows in a given repository. Can we mitigate our impact here? For example, by ensuring that we only have one cache at a time or limiting the size of the caches we store. This may not need to be addressed at this point if we only plan to use this internally for now, but it should be addressed before we ship it more widely, especially if the goal is to have it on by default at some point.

Thank you for bringing that up. I added a note to the internal tracking issue for consideration later.

@mbg
Copy link
Member

mbg commented Jul 3, 2025

Can you say more about what you mean by "gain confidence that none of the new logic interferes with existing functionality" and "cover all cases".

Broadly, we should check that getOverlayDatabaseMode() doesn't turn on this feature unexpectedly. I.e. we should have tests that cover all the cases it uses for deciding whether to turn the feature on or not. That might be doable with some unit tests, or might need an extra pr-check.

cklin added 13 commits July 3, 2025 12:35
This commit adds overlayDatabaseMode to AugmentationProperties and
creates a placeholder getOverlayDatabaseMode() function, with the
necessary inputs, to populate it.
This commit populates getOverlayDatabaseMode() in config-utils with the
same code from getOverlayDatabaseMode() in init.
This commit changes databaseInitCluster() to use overlayDatabaseMode
from AugmentationProperties instead of the overlayDatabaseMode
parameter. There is no behavior change because both overlayDatabaseMode
values are computed the same way.

The commit then cleans up the overlayDatabaseMode parameter and the code
paths that feed into it.
This commit changes getOverlayDatabaseMode so that, when
Feature.OverlayAnalysis is enabled, it calculates the overlay database
mode automatically based on analysis metadata. If we are analyzing the
default branch, use OverlayBase, and if we are analyzing a PR, use
Overlay.

If CODEQL_OVERLAY_DATABASE_MODE is set to a valid overlay database mode,
that environment variable still takes precedence.
This commit adds useOverlayDatabaseCaching to AugmentationProperties to
indicate whether the action should upload overlay-base databases to the
actions cache and to download a cached overlay-base database when
creating an overlay database.
@cklin cklin force-pushed the cklin/overlay-analysis branch 2 times, most recently from a8a0cc8 to f32db97 Compare July 3, 2025 19:39
@cklin cklin force-pushed the cklin/overlay-analysis branch from f32db97 to 55ea4c3 Compare July 3, 2025 20:32
@cklin
Copy link
Contributor Author

cklin commented Jul 3, 2025

@mbg

I made some of the requested changes (diff from force pushes sans rebase).

  • Added comment on sourceRoot definition
  • Added comment on repo owner allowlist
  • Added unit tests for getPullRequestBranches()
  • Added unit tests for getOverlayDatabaseMode()

These are the changes I consider (a) in-scope for this PR and (b) there is clear consensus on how to proceed.

The main unresolved issue is how we want to handle the cleanup-level input. My reading of the internal discussion is that that input is considered advisory, and it is reasonable to override the user input when appropriate. That said, I am happy to make additional changes to try to mitigate the situation:

  • Update the description of cleanup-level to specify that it is advisory
  • Remove the default brutal value of cleanup-level, and document the default as automatic selection
  • Emit a warning when we override the user-specified cleanup-level value

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants

TMZ Celebrity News – Breaking Stories, Videos & Gossip

Looking for the latest TMZ celebrity news? You've come to the right place. From shocking Hollywood scandals to exclusive videos, TMZ delivers it all in real time.

Whether it’s a red carpet slip-up, a viral paparazzi moment, or a legal drama involving your favorite stars, TMZ news is always first to break the story. Stay in the loop with daily updates, insider tips, and jaw-dropping photos.

🎥 Watch TMZ Live

TMZ Live brings you daily celebrity news and interviews straight from the TMZ newsroom. Don’t miss a beat—watch now and see what’s trending in Hollywood.