Skip to content

Add csra analysis kind#3474

Open
mbg wants to merge 13 commits intomainfrom
mbg/risk-assessment-analysis
Open

Add csra analysis kind#3474
mbg wants to merge 13 commits intomainfrom
mbg/risk-assessment-analysis

Conversation

@mbg
Copy link
Member

@mbg mbg commented Feb 11, 2026

First stab at adding this new analysis kind. See the linked internal issues for more details. The new analysis kind largely mirrors code-scanning.

We do not plan this in combination with other analysis kinds, so I added some logic to prevent that.

The SARIF upload for this analysis kind is a bit different than the other two, so I added a transformPayload function to the AnalysisConfig interface that can change what's included in the payload. I preferred that over having a conditional block near the upload, since that's harder to see and less reusable.

I extended the existing tests for this new analysis kind.

Notes for reviewers

This is ready for an initial review to examine the high-level approach here. There's almost certainly room for polishing this and adding some more tests.

Best reviewed commit-by-commit.

Risk assessment

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

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

This change should not directly impact any of the below, but it may indirectly as a result of changes to code paths that are also hit by those.

Workflow types:

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

Products:

  • Code Scanning - The changes impact analyses when analysis-kinds: code-scanning.
  • Code Quality - The changes impact analyses when analysis-kinds: code-quality.
  • CCR - The changes impact analyses for Copilot Code Reviews.

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?

  • Special considerations - This change should only be merged once certain preconditions are met. Please provide details of those or link to this PR from an internal issue.

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 self-assigned this Feb 11, 2026
@github-actions github-actions bot added the size/M Should be of average difficulty to review label Feb 11, 2026
@mbg mbg force-pushed the mbg/risk-assessment-analysis branch from deac236 to 29847d7 Compare February 11, 2026 23:24
@mbg mbg force-pushed the mbg/risk-assessment-analysis branch from 29847d7 to 40e87b6 Compare February 11, 2026 23:51
@github-actions github-actions bot added size/L May be hard to review and removed size/M Should be of average difficulty to review labels Feb 11, 2026
@mbg mbg force-pushed the mbg/risk-assessment-analysis branch from 40e87b6 to c48cd24 Compare February 11, 2026 23:56
@mbg mbg marked this pull request as ready for review February 12, 2026 13:29
@mbg mbg requested a review from a team as a code owner February 12, 2026 13:29
Copilot AI review requested due to automatic review settings February 12, 2026 13:29
Copy link
Contributor

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 a new csra analysis kind to the CodeQL Action, intended to largely mirror code-scanning while uploading SARIF to a different endpoint with a different payload shape. The change introduces analysis-kind compatibility enforcement and extends unit + PR-check coverage to include the new kind.

Changes:

  • Add AnalysisKind.CSRA plus a compatibility matrix that prevents unsupported combinations.
  • Introduce typed upload payload shapes and an AnalysisConfig.transformPayload hook to support analysis-specific upload payloads (CSRA adds assessment_id).
  • Extend unit tests and PR-checks to cover csra SARIF naming/grouping and matrix validation.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/upload-lib/types.ts Introduces typed payload interfaces (BasePayload, UploadPayload, AssessmentPayload).
src/upload-lib.ts Uses typed payloads; routes upload payload through transformPayload prior to request.
src/upload-lib.test.ts Updates SARIF grouping tests to account for all analysis kinds; updates upload payload fixtures typing.
src/config-utils.ts Updates “primary analysis config” selection to handle single-kind configs (incl. CSRA) via getAnalysisConfig.
src/config-utils.test.ts Adds tests for getPrimaryAnalysisConfig with single kind and CS+CQ special-case.
src/analyze.ts Simplifies category computation by always using analysis.fixCategory(...).
src/analyze.test.ts Extends SARIF extension tests to include .csra.sarif.
src/analyses.ts Adds csra kind, endpoint, payload transform, compatibility matrix, and SARIF scan order updates.
src/analyses.test.ts Adds compatibility-matrix-driven tests and verifies Code Scanning predicate rejects other extensions.
pr-checks/checks/analysis-kinds.yml Expands PR-check matrix to include csra and sets required env for assessment id; updates artifact handling.
.github/workflows/__analysis-kinds.yml Generated workflow updated to match pr-checks definition (auto-generated).
lib/upload-sarif-action.js Generated JS output (not reviewed).
lib/upload-sarif-action-post.js Generated JS output (not reviewed).
lib/upload-lib.js Generated JS output (not reviewed).
lib/start-proxy-action.js Generated JS output (not reviewed).
lib/start-proxy-action-post.js Generated JS output (not reviewed).
lib/setup-codeql-action.js Generated JS output (not reviewed).
lib/resolve-environment-action.js Generated JS output (not reviewed).
lib/init-action.js Generated JS output (not reviewed).
lib/init-action-post.js Generated JS output (not reviewed).
lib/autobuild-action.js Generated JS output (not reviewed).
lib/analyze-action.js Generated JS output (not reviewed).
lib/analyze-action-post.js Generated JS output (not reviewed).

Comment on lines +190 to +193
const assessmentId = parseInt(
getRequiredEnvParam("CODEQL_ACTION_CSRA_ASSESSMENT_ID"),
10,
);
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

addAssessmentId uses parseInt on CODEQL_ACTION_CSRA_ASSESSMENT_ID, but if the env var is non-numeric this will produce NaN and still get uploaded as assessment_id. Consider validating the parsed value (e.g., Number.isFinite) and throwing a clear configuration error when it isn’t a valid integer.

Suggested change
const assessmentId = parseInt(
getRequiredEnvParam("CODEQL_ACTION_CSRA_ASSESSMENT_ID"),
10,
);
const rawAssessmentId = getRequiredEnvParam(
"CODEQL_ACTION_CSRA_ASSESSMENT_ID",
);
const assessmentId = parseInt(rawAssessmentId, 10);
if (!Number.isFinite(assessmentId)) {
throw new ConfigurationError(
`Environment variable CODEQL_ACTION_CSRA_ASSESSMENT_ID must be a valid integer, but got: ${rawAssessmentId}`,
);
}

Copilot uses AI. Check for mistakes.
target: SARIF_UPLOAD_ENDPOINT.CSRA,
sarifExtension: ".csra.sarif",
sarifPredicate: (name) => name.endsWith(CSRA.sarifExtension),
fixCategory: fixCodeQualityCategory,
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

CSRA.fixCategory is set to fixCodeQualityCategory, but that helper is documented as a Code Quality-only workaround for Default Setup category mappings. Applying it to CSRA could unintentionally rewrite categories (and log "Adjusted category for Code Quality...") for a non-Code-Quality upload target. Consider making CSRA’s fixCategory a no-op (like Code Scanning) or introducing a CSRA-specific adjustment if needed.

Suggested change
fixCategory: fixCodeQualityCategory,
fixCategory: (_, category) => category,

Copilot uses AI. Check for mistakes.
Comment on lines +851 to 865
const payload = uploadTarget.transformPayload(
buildPayload(
await gitUtils.getCommitOid(checkoutPath),
await gitUtils.getRef(),
postProcessingResults.analysisKey,
util.getRequiredEnvParam("GITHUB_WORKFLOW"),
zippedSarif,
actionsUtil.getWorkflowRunID(),
actionsUtil.getWorkflowRunAttempt(),
checkoutURI,
postProcessingResults.environment,
toolNames,
await gitUtils.determineBaseBranchHeadCommitOid(),
),
);
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

uploadPostProcessedFiles now routes the payload through uploadTarget.transformPayload(...) before calling uploadPayload, and CSRA relies on this to change the request shape. There isn’t a unit test asserting the transformed payload for CSRA (e.g., that assessment_id is included) or even that transformPayload is applied. Adding a focused test around this call site would help prevent regressions as more analysis kinds are added.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Re the risk assessment, I wonder whether it is worth us adding some explicit checks for upload-sarif that upload static SARIF files for each analysis and check the exact data that was sent to the API?

Comment on lines +75 to +82
- name: Check quality query does not appear in CSRA SARIF
if: contains(matrix.analysis-kinds, 'csra')
uses: actions/github-script@v8
env:
SARIF_PATH: "${{ runner.temp }}/results/javascript.csra.sarif"
EXPECT_PRESENT: "false"
with:
script: ${{ env.CHECK_SCRIPT }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we implement this behaviour when CSRA and code quality analysis kinds can't be run together?

* @param payload The base payload.
*/
function addAssessmentId(payload: UploadPayload): AssessmentPayload {
const assessmentId = parseInt(
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with Copilot that we should validate that this is not NaN

target: SARIF_UPLOAD_ENDPOINT.CSRA,
sarifExtension: ".csra.sarif",
sarifPredicate: (name) => name.endsWith(CSRA.sarifExtension),
fixCategory: fixCodeQualityCategory,
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this a copy/paste error or are we sharing this fix category logic? If so, let's rename and update docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was intentionally conservative. I have since discussed this with the team and it's not needed for this, so I will change it to the identity function.

export enum AnalysisKind {
CodeScanning = "code-scanning",
CodeQuality = "code-quality",
CSRA = "csra",
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's probably going to be a pain to change this in the future, let's bike shed — could we avoid a FLA here and use something like risk-assessment?

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.

2 participants