Support shorthand forms for `getRemoteConfig` by mbg · Pull Request #3973 · github/codeql-action

mbg · 2026-06-23T17:02:59Z

This is a follow-up to #3963 which modifies getRemoteConfig to accept shorthand addresses for remote files. Concretely, the owner, path, and ref components are now optional and default to the owner of the current repo (as given by GITHUB_REPOSITORY), .github/codeql-action.yaml, and main respectively.

Examples:

owner/repo resolves to owner/repo/.github/codeql-action.yaml@main
owner/repo@dev resolves to owner/repo/.github/codeql-action.yaml@dev
owner/repo/.github/codeql/config.yml resolves to owner/repo/.github/codeql/config.yml@main

Notes for reviewers

We should discuss whether there's a better default we want for the path component
The default ref of main may not always be the default branch for the target repo, but that's OK since it can be manually set to a different ref if needed.
I originally implemented the shorthand forms from my proposal, but then noticed that getRemoteConfig already had a similar (but not equivalent) shorthand format. I then updated that instead so that the components other than the repo are optional. However, this isn't as nice, because / is used as a separator and can also appear in the path component. That means e.g. it is not possible with this format to specify a repo + path since it would be interpreted as owner + repo by the regex instead.

Risk assessment

For internal use only. Please select the risk level of this change:

Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.

Which use cases does this change impact?

Workflow types:

Advanced setup - Impacts users who have custom CodeQL workflows.
Managed - Impacts users with dynamic workflows (Default Setup, Code Quality, ...).

Products:

Code Scanning - The changes impact analyses when analysis-kinds: code-scanning.
Code Quality - The changes impact analyses when analysis-kinds: code-quality.
Other first-party - The changes impact other first-party analyses.

Environments:

Dotcom - Impacts CodeQL workflows on github.com and/or GitHub Enterprise Cloud with Data Residency.
GHES - Impacts CodeQL workflows on GitHub Enterprise Server.

How did/will you validate this change?

Test repository - This change will be tested on a test repository before merging.
Unit tests - I am depending on unit test coverage (i.e. tests in .test.ts files).
End-to-end tests - I am depending on PR checks (i.e. tests in pr-checks).

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

Rollback - Change can only be disabled by rolling back the release or releasing a new version with a fix.

How will you know if something goes wrong after this change is released?

Telemetry - I rely on existing telemetry or have made changes to the telemetry.
- Dashboards - I will watch relevant dashboards for issues after the release. Consider whether this requires this change to be released at a particular time rather than as part of a regular release.
- Alerts - New or existing monitors will trip if something goes wrong with this change.

Are there any special considerations for merging or releasing this change?

No special considerations - This change can be merged at any time.

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Consider adding a changelog entry for this change.
Confirm the readme and docs have been updated if necessary.

Copilot

Warning

Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.

Pull request overview

This PR extends remote configuration file support by enhancing getRemoteConfig to accept shorthand remote addresses (optional owner, path, and ref with sensible defaults), and refactors environment access to be more testable via an Env wrapper.

Changes:

Added parseRemoteFileAddress (and tests) to parse shorthand remote config references with defaults for owner/path/ref.
Moved getRemoteConfig into src/config/file.ts and updated callers to use the new parsing logic.
Introduced ActionsEnvVars and an Env abstraction to reduce raw string usage and improve testability.

Show a summary per file

File	Description
src/util.ts	Adds `Env` accessor helpers and refactors env var getters.
src/testing-utils.ts	Adds `getTestEnv()` and tightens typing for default Actions env vars.
src/environment.ts	Introduces `Env` interface abstraction for environment access.
src/config/remote-file.ts	New parser for shorthand remote config references and defaults.
src/config/remote-file.test.ts	Unit tests for remote file address parsing and defaults.
src/config/file.ts	Adds `getRemoteConfig()` using parsed remote address components.
src/config-utils.ts	Switches to the new `getRemoteConfig()` implementation.
src/config-utils.test.ts	Removes a test that no longer matches the supported shorthand syntax.
src/api-client.ts	Replaces raw env var names with `ActionsEnvVars`.
src/actions-util.ts	Adds `ActionsEnvVars` enum and updates env var usages.
lib/entry-points.js	Changed but excluded from review (generated/contents unavailable).

Copilot's findings

Files excluded by content exclusion policy (1)

lib/entry-points.js

Files reviewed: 10/11 changed files
Comments generated: 6

…-file-shorthands

mario-campos

Looks solid! I left a few comments, but minor stuff.

mario-campos · 2026-06-24T16:39:17Z

+    ref: DEFAULT_CONFIG_FILE_REF,
+  } satisfies RemoteFileAddress);
+
+  t.deepEqual(parseRemoteFileAddress(env, "owner/repo/path@"), {


I think this should be invalid, for the same reason that /repo@ref is invalid.

Edit: Also, I just noticed the expected format below does not seem to indicate that this should be a correct format:

Expected format [<owner>/]<repository>[/<file-path>][@<ref>]

I have no strong feelings on this, but agree that we should be consistent. I can change this to also be invalid.

…nctions

mario-campos

I left a few comments, most of them minor/simple.

If you're wondering about the Outdated label on the comments... I commented on the individual commits, because I reviewed them chronologically.

mario-campos · 2026-06-30T15:18:22Z

+  t.throws(() => parseRemoteFileAddress(env, ":path"), {
+    instanceOf: ConfigurationError,
+  });
+  t.throws(() => parseRemoteFileAddress(env, "@ref"), {
+    instanceOf: ConfigurationError,
+  });
+  t.throws(() => parseRemoteFileAddress(env, "@ref:path"), {
+    instanceOf: ConfigurationError,
+  });


Aren't all components optional? If so, then this should be a valid, supported format.

All components except the repo are optional. These tests are a combination of "no repo" and the sort of thing you previously complained about in #3973 (comment)

mario-campos · 2026-06-30T15:58:56Z

  // Ensure that the path is a relative path.
  if (path?.startsWith("/")) {
    throw new ConfigurationError(
      `The path component of '${configFile}' cannot be an absolute path.`,
    );
  }


If you don't mind losing the clear error message, this could be implemented in the regular-expression pattern with a negative lookahead: (:(?!/)(?<path>.+))

Personally, I don't like using regular expressions to perform complex parsing tasks. This approach is more readable/maintainable. I'd consider adding the regular error message on to this one if you think that's beneficial, but I am not sure it's necessary since the regex already matched (meaning the overall format was correct) and this error is more specific than the overall format.

That's fair. It's not always the best trade-off.

mario-campos · 2026-06-30T16:04:08Z

+import { FeatureEnablement } from "./feature-flags";
+import { Logger } from "./logging";
+
+export interface ActionState {


A JSDoc explaining the purpose of ActionState would be helpful here.

Done in the commit I pushed just after you posted your review, I think.

mario-campos · 2026-06-30T16:09:30Z

+    { input: "owner/repo   @ref:path", expected },
+    { input: "owner/repo@   ref:path", expected },
+    { input: "owner/repo@ref   :path", expected },
+    { input: "owner/repo@ref:   path", expected },


Whitespace is significant in a file path. How do you know that path is a typo, and that the user—for whatever reason—didn't actually intend path?

But, taking a step back, I don't think we should necessarily support (or expect) whitespace in the address. I think it's perfectly acceptable to treat a typo/mistake as an error, if it is, in fact, a mistake. That would allow us to treat owner , repo, ref literally and simplify the logic a bit.

Whitespace is significant in a file path.

The path is used in getRemoteConfig as argument to the path parameter for the rest.repos.getContent call. I am not sure that leading or trailing whitespace would be meaningful there, or how happy git/GitHub is with repos containing whitespace at the start / end of file paths. If that is supported, then it would of course make sense to keep it.

I am not sure that leading or trailing whitespace would be meaningful there, or how happy git/GitHub is with repos containing whitespace at the start / end of file paths.

I would expect git/GitHub to do its own input validation. And error appropriately if whitespace is invalid.

mario-campos · 2026-06-30T16:10:02Z

  const pieces = OLD_REMOTE_ADDRESS_FORMAT.exec(input);

  // 5 = 4 groups + the whole expression
  if (pieces?.groups === undefined || pieces.length < 5) {


pieces.length < 5 is a useless condition, because it will always be false. The regular expression pattern will either match with 4 groups + the whole expression, or it will not match at all.

In fact, pieces.length will also never be greater than 5.

This is a check kept from the previous implementation and not new in this PR.

mario-campos · 2026-06-30T16:10:46Z

+
  // retrieve the various parts of the config location, and ensure they're present
  const format = new RegExp(
    "^((?<owner>[^:@/]+)/)?(?<repo>[^:@/]+)(@(?<ref>[^:]+))?(:(?<path>.+))?$",


Since / will always follow the owner, I think it would be simpler and less error-prone to make the character class [^/].

Likewise, I think the character class for the repo group could be [^:@], since / is used in the old format, not this new one.

Changing the regex as suggested leads to potentially incorrect results, because the groups end up being too eager in what they accept. For example, foo:some/path/to/a/file.yml should use foo as the repo and some/path/to/a/file.yml as the path. What would happen in practice (with your suggestion) is that foo:some ends up being the owner (owner reads until it finds a /) and path/to/a/file.yml ends up as the repo (repo reads until it finds a : or @, neither of which exist). That satisfies the regex, but is unlikely what was intended.

mario-campos · 2026-06-30T16:11:08Z

  const repo: string | undefined = pieces?.groups?.repo?.trim();

  // Check that the regular expression matched and that we have at least the repo name.
  if (!pieces?.groups || !repo || repo.length === 0) {


Regarding !repo, I am not sure how the JS regular-expression engine works with regards to undefined, but since the quantifier is +, I expect repo to always be a one character string, at least. So, it seems unnecessary 🤷

Similarly, repo.length should always be at least 1 (because of the + quantifier). So, this seems like a useless condition.

There are two separate things going on here:

The !repo check is there to make TypeScript happy, since it does't know that !pieces?.groups implies that repo !== undefined.

The repo.length check follows from the call to .trim() in the initial assignment of repo. Since, in theory, the regex could accept a whitespace-only repo value, .trim() may then reduce it to 0 length, which we check for here to reject it. That could be captured in the regex itself, but would then require us to specify which character ranges are acceptable (and maintain it).

mario-campos · 2026-06-30T16:12:09Z

 * All the components are required. Unchanged from the previous implementation.
 */
 const OLD_REMOTE_ADDRESS_FORMAT = new RegExp(
  "(?<owner>[^/]+)/(?<repo>[^/]+)/(?<path>[^@]+)@(?<ref>.*)",


Since all components are required, the ref group should have the + quantifier. The * quantifier can consume zero characters, which can mean that the ref group is the empty string, which would need to be handled in a special case.

This pattern should be anchored to match the whole string.

What you wrote mirrors what Copilot complained about originally when I updated the regex to make parts of it optional. However, now that I have added a new regex for the new format, OLD_REMOTE_ADDRESS_FORMAT is just the regex from the existing code before any changes in this PR. In other words, making these adjustments (while perhaps reasonable) would alter the original behaviour. The old behaviour is not FF-gated, and so we'd be making a breaking change to existing code. That seems unnecessary to me.

mbg added 10 commits June 23, 2026 16:16

Add ActionsEnvVars enum

c05a5b7

Allow abstracting over process.env

652296e

Add RemoteFileAddress type

9c4ee01

Move getRemoteConfig to config/file.ts

07b6b1e

Refactor parseRemoteFileAddress out of getRemoteConfig

9ef8be7

Return RemoteFileAddress from parseRemoteFileAddress

82e5ca6

Add getRemoteConfig JSDoc

c7a94c9

Add tests for parseRemoteFileAddress

85c8a8c

Make ref optional in parseRemoteFileAddress

598d008

Make path optional in parseRemoteFileAddress

e537ff2

mbg requested a review from mario-campos June 23, 2026 17:03

mbg self-assigned this Jun 23, 2026

github-actions Bot added the size/L May be hard to review label Jun 23, 2026

Make owner optional in parseRemoteFileAddress

12821cf

mbg force-pushed the mbg/repo-props/config-file-shorthands branch from a2218f1 to 12821cf Compare June 23, 2026 17:13

mbg marked this pull request as ready for review June 23, 2026 17:15

mbg requested a review from a team as a code owner June 23, 2026 17:15

Copilot AI review requested due to automatic review settings June 23, 2026 17:15

Copilot started reviewing on behalf of mbg June 23, 2026 17:16 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

Comment thread src/util.ts

Comment thread src/config/remote-file.ts

Comment thread src/config/remote-file.ts Outdated

Comment thread src/config/remote-file.ts

Comment thread src/config/remote-file.test.ts Outdated

Comment thread src/config/remote-file.ts

mbg added 6 commits June 23, 2026 18:31

Fix test names

81ad479

Anchor regex and trim input

8102fa6

Update format in error message

00e5a58

Improve whitespace handling

8d69da9

Fix getEnv not using getOptionalEnvVarFrom

3fe7ef9

Merge remote-tracking branch 'origin/main' into mbg/repo-props/config…

812b882

…-file-shorthands

mario-campos previously approved these changes Jun 24, 2026

View reviewed changes

mbg added 3 commits June 26, 2026 18:09

Support old and new formats

f77cf55

Add FF for shorthands

d8d1d6d

Add initial ActionState type

e446d55

mbg added 3 commits June 30, 2026 13:03

Use ActionState for getRemoteConfig

f86aa52

Add TestEnv class to simplify testing of ActionState-dependent fu…

7b2af89

…nctions

Gate new remote file format behind FF

b6be81d

mbg dismissed mario-campos’s stale review via b6be81d June 30, 2026 12:20

github-actions Bot added size/XL May be very hard to review and removed size/L May be hard to review labels Jun 30, 2026

mbg requested a review from mario-campos June 30, 2026 12:35

mario-campos requested changes Jun 30, 2026

View reviewed changes

Index ActionState over the types of state used

708e106

Uh oh!

Conversation

mbg commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Risk assessment

Which use cases does this change impact?

How did/will you validate this change?

If something goes wrong after this change is released, what are the mitigation and rollback strategies?

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

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mario-campos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mario-campos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

mbg commented Jun 23, 2026 •

edited

Loading

mario-campos left a comment •

edited

Loading