Skip to content

Store and restore "failed" configuration if init step aborts - #4225

Merged
mbg merged 17 commits into
mainfrom
mbg/reduce-config-requirements
Oct 13, 2026
Merged

mbg merged 17 commits into
mainfrom
mbg/reduce-config-requirements

Conversation

@mbg

@mbg mbg commented Oct 12, 2026 •

Copy link
Copy Markdown
Member

This PR mainly contains cherry-picked changes from #4223 for easier review and since they are somewhat separate from the main goal there. In #4223, the goal is to persist and upload diagnostics that are created before the database is initialised. That is the case if, for example, we produce diagnostics and then realise that we should abort the analysis and shouldn't initialise a database.

One issue with making diagnostics from a failed init run available is that the init-post action does not produce or upload any debug artifacts (including the failed SARIF file that would contain the diagnostics) if no Config was written by init. However, init only writes the Config these days if it successfully runs to completion.

This PR addresses this by making the following changes:

  • A partial, "failed" Config is written by init if the step is aborted (i.e. an exception is thrown in the first try/catch block). To avoid potentially breaking any code that treats the existence of the normal configuration file as a signal that init was successful, we store the "failed" configuration with a different name.
  • Although we only write this "failed" Config when a full Config is available, we write a minimal subset of properties that are needed by init-post to try and produce the debug artifacts we are after. This is also intended to guard against the possibility that some functions interpret the presence of some properties as indication that init was successful.
  • The init-post Action reads this "failed" config only if a normal config couldn't be found.
  • A new ActionConfig type is added. This is equivalent to Config by default and, besides having a name which more accurately reflects what Config is, has a type parameter that can be used to mark only some properties as required while unnamed ones are optional.
  • That type is then used to better encode the requirements of various Config consumers so that they don't unnecessarily require a full Config.
  • This then allows init-post to potentially call them with a "failed" config.

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.
  • High risk: Changes are not fully under feature flags, have limited visibility and/or cannot be tested outside of production.

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?

  • 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?

  • Feature flags - All new or changed code paths can be fully disabled with corresponding feature flags.
  • Rollback - Change can only be disabled by rolling back the release or releasing a new version with a fix.
  • Development/testing only - This change cannot cause any failures in production.
  • Other - Please provide details.

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.

@mbg
mbg added this pull request to stack #4226 October 12, 2026 11:14
@github-actions github-actions Bot added the size/L May be hard to review label Oct 12, 2026
@mbg
mbg removed this pull request from stack #4226 October 12, 2026 11:48
@mbg
mbg changed the base branch from mbg/fs-abstraction to main October 12, 2026 11:55
@mbg
mbg force-pushed the mbg/reduce-config-requirements branch from daee991 to c782625 Compare October 12, 2026 11:58
@mbg
mbg force-pushed the mbg/reduce-config-requirements branch from c782625 to ac8654a Compare October 12, 2026 12:08
@mbg
mbg marked this pull request as ready for review October 12, 2026 12:42
@mbg
mbg requested a review from a team as a code owner October 12, 2026 12:42
Copilot AI balanced review requested due to automatic review settings October 12, 2026 12:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@mbg
mbg added this pull request to stack #4227 October 12, 2026 13:49
@mbg
mbg requested review from henrymercer and a balanced review from Copilot October 12, 2026 15:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

🟡 Changes recommended

The central aborted-configuration persistence and restoration path lacks direct unit coverage, with additional minor diagnostic and documentation issues.

4 open findings
Files excluded by content exclusion policy (1)
  • lib/entry-points.js

🧠 Review effort: Balanced

Comment thread src/config-utils.ts
* The file will only contain parts of {@link config} that are relevant to
* the `init-post` step in the workflow.
*/
export function saveAbortedInitConfig(
Comment thread src/init-action-post-helper.ts
Comment thread src/config-utils.ts Outdated
Comment thread src/config/action-config.ts Outdated

@henrymercer henrymercer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some suggestions and comments, but this broadly looks good to me. I'm happy with testing the new functionality here using the PR check added by #4223.

Comment thread src/config-utils.ts
Comment thread src/config-utils.ts
Comment thread src/actions-util.ts
Comment thread src/config/action-config.ts
Comment thread src/init-action-post-helper.ts
Comment thread src/init-action-post-helper.ts Outdated
Comment thread src/init-action-post.ts
* to requiring only those. For example, in `ActionConfig<"codeqlCmd">`
* all properties except `codeqlCmd` are optional.
*/
export type ActionConfig<

@henrymercer henrymercer Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bikeshed: ConfigWith?

Also, we should be careful with properties that might be optional, e.g. buildMode can be undefined in a successful configuration.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Some time ago we talked about how Config is an awful name for the type and that it's really more representing the state of the Action that we persist across steps. It's also ambiguous as to whether it refers to the Action, CLI, or something else. I thought that I would use this opportunity to move away from that and introduce a better name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My one comment about ActionConfig is that it's quite easy to confuse with ActionState, e.g. you mention "A new ActionState type is added" in the PR description. But I agree it's good to clarify that we're talking about Action configuration.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point and I have fixed that in the PR description now. I can't really think of a better name right now, but open to suggestions (and happy to rename as a follow-up).

@mbg
mbg added this pull request to the merge queue Oct 13, 2026
Merged via the queue into main with commit eb94de7 Oct 13, 2026
235 checks passed
@mbg
mbg deleted the mbg/reduce-config-requirements branch October 13, 2026 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L May be hard to review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants