Add github-codeql-tools repository property for tools input#3766
Add github-codeql-tools repository property for tools input#3766
github-codeql-tools repository property for tools input#3766Conversation
github-codeql-tools repository property for tools input
Co-authored-by: oscarsj <1410188+oscarsj@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/codeql-action/sessions/24ef386a-5e4a-4630-b138-747d6c5729da
oscarsj
force-pushed
the
copilot/add-tools-input-source-repository
branch
from
5c6b9e8 to
e74c1ee
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new org-managed repository property (github-codeql-tools) that can supply a default tools input value (when the workflow doesn’t set one), enabling organizations to opt into using the toolcache without per-run CLI downloads.
Changes:
- Introduces
RepositoryPropertyName.TOOLS = "github-codeql-tools"and parses it from the repository properties API. - Resolves an effective tools input in
init-action(workflow input wins; otherwise fall back to repository property) and threads an origin flag through to CodeQL setup. - Allows
tools=toolcacheto bypass the existing feature-flag/dynamic-workflow restriction when the value came from the repository property, with targeted log messages and unit tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/feature-flags/properties.ts | Adds the new repository property name and parser typing for github-codeql-tools. |
| src/feature-flags/properties.test.ts | Tests loading/parsing of the new github-codeql-tools property. |
| src/init-action.ts | Resolves effective tools input (workflow vs repository property) and passes origin flag into initCodeQL. |
| src/init.ts | Threads toolsInputFromRepositoryProperty into setupCodeQL. |
| src/codeql.ts | Threads toolsInputFromRepositoryProperty into setupCodeQLBundle. |
| src/setup-codeql.ts | Adds toolsInputFromRepositoryProperty parameter and uses it to allow toolcache without FF/dynamic checks. |
| src/setup-codeql.test.ts | Adds tests for toolcache behavior when enabled via repository property (including empty toolcache fallback). |
| lib/* | Generated JS output corresponding to the TS changes (not reviewed). |
| // Determine the effective tools input. | ||
| // The explicit `tools` workflow input takes precedence. If none is provided, | ||
| // fall back to the 'github-codeql-tools' repository property (if set). | ||
| const toolsWorkflowInput = getOptionalInput("tools"); | ||
| const toolsPropertyValue: string | undefined = | ||
| repositoryPropertiesResult.orElse({})[RepositoryPropertyName.TOOLS]; | ||
| const effectiveToolsInput = toolsWorkflowInput ?? toolsPropertyValue; | ||
| const toolsInputFromRepositoryProperty = | ||
| toolsWorkflowInput === undefined && toolsPropertyValue !== undefined; |
There was a problem hiding this comment.
effectiveToolsInput correctly falls back to the repository property, but later status reporting still uses getOptionalInput("tools") (see tools_input field earlier in this file). When github-codeql-tools is set, telemetry/status reports will show an empty tools_input, which makes it hard to debug or measure adoption. Consider reporting the resolved/effective tools value and/or explicitly recording whether it came from workflow input vs repository property.
| // Determine the effective tools input. | ||
| // The explicit `tools` workflow input takes precedence. If none is provided, | ||
| // fall back to the 'github-codeql-tools' repository property (if set). | ||
| const toolsWorkflowInput = getOptionalInput("tools"); | ||
| const toolsPropertyValue: string | undefined = | ||
| repositoryPropertiesResult.orElse({})[RepositoryPropertyName.TOOLS]; | ||
| const effectiveToolsInput = toolsWorkflowInput ?? toolsPropertyValue; | ||
| const toolsInputFromRepositoryProperty = | ||
| toolsWorkflowInput === undefined && toolsPropertyValue !== undefined; |
There was a problem hiding this comment.
The PR description says the github-codeql-tools repository property acts as a default for the tools input (with workflow input taking precedence). In this change it’s only resolved in init-action.ts; setup-codeql-action.ts still uses only the workflow input and never loads repository properties, so users of that action won’t get the org-level default behavior. Either extend the same resolution logic to setup-codeql-action (and any other entrypoints that accept tools) or clarify/adjust the PR description/docs to match the actual scope.
| export async function getCodeQLSource( | ||
| toolsInput: string | undefined, | ||
| defaultCliVersion: CodeQLDefaultVersionInfo, | ||
| apiDetails: api.GitHubApiDetails, | ||
| variant: util.GitHubVariant, | ||
| tarSupportsZstd: boolean, | ||
| features: FeatureEnablement, | ||
| logger: Logger, | ||
| toolsInputFromRepositoryProperty = false, | ||
| ): Promise<CodeQLToolsSource> { |
There was a problem hiding this comment.
toolsInputFromRepositoryProperty is introduced on getCodeQLSource, but it’s only used to special-case the toolcache guard + log messages. If the repository property is ever used for other tools values (e.g. linked, nightly, or a URL), the current logs in other branches still say they were requested by 'tools: …', which can be misleading when no workflow input was provided. Consider generalizing this to an “tools input origin” (workflow vs repo property) and using it consistently in all user-facing log messages about requested tools.
| if (toolsInputFromRepositoryProperty) { | ||
| logger.info( | ||
| `Attempting to use the latest CodeQL CLI version in the toolcache, as requested by the 'github-codeql-tools' repository property.`, | ||
| ); |
There was a problem hiding this comment.
The repository property name is hard-coded in these log messages ('github-codeql-tools'). To avoid drift if the property name ever changes and to keep usage consistent with the rest of the repo-properties system, consider referencing RepositoryPropertyName.TOOLS (or a shared constant) instead of duplicating the string literal in multiple places (including tests).
There was a problem hiding this comment.
Indifferent here, we wouldn't expect the name to change after the PR is merged.
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
This PR needs work. As CCR has already identified, the new logic is not coherently applied to all parts of the CodeQL Action.
Beyond that, we have some high-level design questions to answer before settling on an implementation. I have discussed these in my details comments, but in summary they are:
- The most important is whether we want the CodeQL Action repo properties to serve as customisation options for managed workflows (like Default Setup) or enterprise policy enforcement (e.g. to force all analyses across an organisation to use particular queries, CLI versions, etc.
- For this change specifically, this would determine whether the repo property (if set) overrides any explicit
toolsinput or not. Or whether we would allow some way of specificing in the repo property value whether it should or not. (e.g. like the prefix+for queries).
- For this change specifically, this would determine whether the repo property (if set) overrides any explicit
- We need to decide on the interaction between this, the FF, and whether the workflow is
dynamic. See my comment there.
Once we have decided on all these aspects, then we should also add a changelog entry here and update the public docs for the new property.
| test.serial( | ||
| "loadPropertiesFromApi loads github-codeql-tools property", | ||
| async (t) => { |
There was a problem hiding this comment.
I am not sure that it makes sense for us to add a separate test case for each property we add, since that mostly just duplicates code. Going by the name of the previous test (loadPropertiesFromApi loads known properties), I am guessing our intention there was to extend that one with other known properties, although we forgot to do so for the ones we added beyond github-codeql-extra-queries.
Could we add this into the loadPropertiesFromApi loads known properties test?
| features: FeatureEnablement, | ||
| logger: Logger, | ||
| checkVersion: boolean, | ||
| toolsInputFromRepositoryProperty = false, |
There was a problem hiding this comment.
It would be good to avoid the default value here and require each caller to provide one explicitly, which makes it easier for us to avoid unintentional behaviour.
| // Determine the effective tools input. | ||
| // The explicit `tools` workflow input takes precedence. If none is provided, | ||
| // fall back to the 'github-codeql-tools' repository property (if set). | ||
| const toolsWorkflowInput = getOptionalInput("tools"); | ||
| const toolsPropertyValue: string | undefined = | ||
| repositoryPropertiesResult.orElse({})[RepositoryPropertyName.TOOLS]; | ||
| const effectiveToolsInput = toolsWorkflowInput ?? toolsPropertyValue; | ||
| const toolsInputFromRepositoryProperty = | ||
| toolsWorkflowInput === undefined && toolsPropertyValue !== undefined; |
There was a problem hiding this comment.
- All of this should be refactored into its own function with appropriate tests.
- We need to think very carefully about the semantics we want here. Right now, an explicit
toolsinput takes precedence over the repo property.
- That probably makes sense since we don't have an explicit
toolsinput in the Default Setup workflow. However, this then creates an implicit dependency / assumption that we won't add one to the workflow template there. - Users with advanced workflows can tune the
toolsinput directly and don't need the repo property. - We will want to be deliberate (and consistent) in whether repository properties serve as "overrides for managed workflows" or "policy enforcement". E.g. could we imagine that an enterprise might want to force the use of a particular CLI version?
Let's discuss this offline and more widely in the team.
| // Determine the effective tools input. | ||
| // The explicit `tools` workflow input takes precedence. If none is provided, | ||
| // fall back to the 'github-codeql-tools' repository property (if set). | ||
| const toolsWorkflowInput = getOptionalInput("tools"); | ||
| const toolsPropertyValue: string | undefined = | ||
| repositoryPropertiesResult.orElse({})[RepositoryPropertyName.TOOLS]; | ||
| const effectiveToolsInput = toolsWorkflowInput ?? toolsPropertyValue; | ||
| const toolsInputFromRepositoryProperty = | ||
| toolsWorkflowInput === undefined && toolsPropertyValue !== undefined; |
| // Determine the effective tools input. | ||
| // The explicit `tools` workflow input takes precedence. If none is provided, | ||
| // fall back to the 'github-codeql-tools' repository property (if set). | ||
| const toolsWorkflowInput = getOptionalInput("tools"); | ||
| const toolsPropertyValue: string | undefined = | ||
| repositoryPropertiesResult.orElse({})[RepositoryPropertyName.TOOLS]; | ||
| const effectiveToolsInput = toolsWorkflowInput ?? toolsPropertyValue; | ||
| const toolsInputFromRepositoryProperty = | ||
| toolsWorkflowInput === undefined && toolsPropertyValue !== undefined; |
| const allowToolcacheValueFF = await features.getValue( | ||
| Feature.AllowToolcacheInput, | ||
| ); | ||
| const allowToolcacheValue = | ||
| allowToolcacheValueFF && (isDynamicWorkflow() || util.isInTestMode()); | ||
| toolsInputFromRepositoryProperty || | ||
| (allowToolcacheValueFF && (isDynamicWorkflow() || util.isInTestMode())); |
There was a problem hiding this comment.
We will need to rethink this logic:
- The FF is here to gate this functionality, so that we can disable it if it is causing problems. We should either also gate this new functionality behind the FF or remove the FF.
- We need to think about whether we want to restrict this to just
dynamicworkflows. See my other point about our overall strategy with respect to repository properties ("customising managed workflows" vs "enterprise policy enforcement"). - With the current logic, the repo property would unconditionally affect all workflows that don't have an explicit
toolsinput. That includes advanced workflows without atoolsinput. Is that what we want? E.g. if an organisation wants to forcetools: toolcachefor Default Setup, then they couldn't do this without also forcing it for all advanced workflows that don't currently have an explicittoolsinput (no explicittoolsinput is generally the default).
| if (toolsInputFromRepositoryProperty) { | ||
| logger.info( | ||
| `Attempting to use the latest CodeQL CLI version in the toolcache, as requested by the 'github-codeql-tools' repository property.`, | ||
| ); |
There was a problem hiding this comment.
Indifferent here, we wouldn't expect the name to change after the PR is merged.
| tarSupportsZstd: boolean, | ||
| features: FeatureEnablement, | ||
| logger: Logger, | ||
| toolsInputFromRepositoryProperty = false, |
There was a problem hiding this comment.
We should consider whether it would be better to set toolsInput to the computed input, instead of having an extra toolsInputFromRepositoryProperty value.
Using only toolsInput would simplify the logic and reduce scope for errors here. (See my other comment about the enablement logic.)
Keeping toolsInputFromRepositoryProperty allows more specific log messages below. However, if we have a log message when computing the tools value that says Settings tools: ${value} based on repository property (or similar), then we wouldn't need to log that in each case here.
| defaultCliVersion: CodeQLDefaultVersionInfo, | ||
| features: FeatureEnablement, | ||
| logger: Logger, | ||
| toolsInputFromRepositoryProperty = false, |
| [`Ignoring 'tools: toolcache' because the feature is not enabled.`], | ||
| ); | ||
|
|
||
| test.serial( |
There was a problem hiding this comment.
The new tests here would not be necessary if we used the existing toolsInput argument.
Large organizations downloading a pinned CodeQL CLI version on every analysis run can hit rate limits. This adds a
github-codeql-toolsrepository property that lets org admins set the tools source at org level, avoiding per-run downloads.What changes
New repository property:
github-codeql-toolsgithub-codeql-tools: toolcache)toolsinput — explicit workflow-leveltoolsinput always takes precedencetoolcachevalue works without requiring theAllowToolcacheInputfeature flag or adynamicworkflow trigger, since the org admin is explicitly opting inImplementation
RepositoryPropertyName.TOOLS = "github-codeql-tools"to the existing property enum/type system insrc/feature-flags/properties.tstoolsInputFromRepositoryPropertyflag through the call chain:initCodeQL→setupCodeQL→setupCodeQLBundle→getCodeQLSourcegetCodeQLSource,toolcachewith this flag set bypasses the feature-flag/dynamic-workflow guard and emits distinct log messages referencing the repository property name rather thantools: toolcacheinit-action.tsresolves the effective tools input: workflow input wins; property is used only when no explicit input is givenRisk assessment
High risk: Not fully under a feature flag — the new code path activates when the repository property is set.
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, Code Quality, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
The repository property must be explicitly set by an org admin; no existing workflows are affected unless they set
github-codeql-tools.How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist
⌨️ Start Copilot coding agent tasks without leaving your editor — available in VS Code, Visual Studio, JetBrains IDEs and Eclipse.