Skip to content

Perform consistent diff-informed alert filtering in the action #2765

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

Merged
merged 3 commits into from
Feb 19, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/analyze.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/analyze.js.map

Large diffs are not rendered by default.

60 changes: 60 additions & 0 deletions lib/diff-filtering-utils.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions lib/diff-filtering-utils.js.map

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 41 additions & 0 deletions lib/upload-lib.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/upload-lib.js.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion lib/util.js.map

Large diffs are not rendered by default.

14 changes: 8 additions & 6 deletions src/analyze.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ import { setupCppAutobuild } from "./autobuild";
import { CodeQL, getCodeQL } from "./codeql";
import * as configUtils from "./config-utils";
import { addDiagnostic, makeDiagnostic } from "./diagnostics";
import {
DiffThunkRange,
writeDiffRangesJsonFile,
} from "./diff-filtering-utils";
import { EnvVar } from "./environment";
import { FeatureEnablement, Feature } from "./feature-flags";
import { isScannedLanguage, Language } from "./languages";
Expand Down Expand Up @@ -284,12 +288,6 @@ export async function setupDiffInformedQueryRun(
);
}

interface DiffThunkRange {
path: string;
startLine: number;
endLine: number;
}

/**
* Return the file line ranges that were added or modified in the pull request.
*
Expand Down Expand Up @@ -537,6 +535,10 @@ extensions:
`Wrote pr-diff-range extension pack to ${extensionFilePath}:\n${extensionContents}`,
);

// Write the diff ranges to a JSON file, for action-side alert filtering by the
// upload-lib module.
writeDiffRangesJsonFile(logger, ranges);

return diffRangeDir;
}

Expand Down
42 changes: 42 additions & 0 deletions src/diff-filtering-utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import * as fs from "fs";
import * as path from "path";

import * as actionsUtil from "./actions-util";
import { Logger } from "./logging";

export interface DiffThunkRange {
path: string;
startLine: number;
endLine: number;
}

function getDiffRangesJsonFilePath(): string {
return path.join(actionsUtil.getTemporaryDirectory(), "pr-diff-range.json");
}

export function writeDiffRangesJsonFile(
logger: Logger,
ranges: DiffThunkRange[],
): void {
const jsonContents = JSON.stringify(ranges, null, 2);
const jsonFilePath = getDiffRangesJsonFilePath();
fs.writeFileSync(jsonFilePath, jsonContents);
logger.debug(
`Wrote pr-diff-range JSON file to ${jsonFilePath}:\n${jsonContents}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

);
}

export function readDiffRangesJsonFile(
logger: Logger,
): DiffThunkRange[] | undefined {
const jsonFilePath = getDiffRangesJsonFilePath();
if (!fs.existsSync(jsonFilePath)) {
logger.debug(`Diff ranges JSON file does not exist at ${jsonFilePath}`);
return undefined;
}
const jsonContents = fs.readFileSync(jsonFilePath, "utf8");
logger.debug(
`Read pr-diff-range JSON file from ${jsonFilePath}:\n${jsonContents}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be more robust to log only the first n entries, in case we have a very large 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 will keep the logging as-is for now. Since the JSON file contains only the line ranges, hopefully it will still be manageable even for a very large PR.

);
return JSON.parse(jsonContents) as DiffThunkRange[];
}
49 changes: 49 additions & 0 deletions src/upload-lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import * as api from "./api-client";
import { getGitHubVersion, wrapApiConfigurationError } from "./api-client";
import { CodeQL, getCodeQL } from "./codeql";
import { getConfig } from "./config-utils";
import { readDiffRangesJsonFile } from "./diff-filtering-utils";
import { EnvVar } from "./environment";
import { FeatureEnablement } from "./feature-flags";
import * as fingerprints from "./fingerprints";
Expand Down Expand Up @@ -578,6 +579,7 @@ export async function uploadFiles(
features,
logger,
);
sarif = filterAlertsByDiffRange(logger, sarif);
sarif = await fingerprints.addFingerprints(sarif, checkoutPath, logger);

const analysisKey = await api.getAnalysisKey();
Expand Down Expand Up @@ -848,3 +850,50 @@ export class InvalidSarifUploadError extends Error {
super(message);
}
}

function filterAlertsByDiffRange(logger: Logger, sarif: SarifFile): SarifFile {
const diffRanges = readDiffRangesJsonFile(logger);
if (!diffRanges?.length) {
return sarif;
}

const checkoutPath = actionsUtil.getRequiredInput("checkout_path");

for (const run of sarif.runs) {
if (run.results) {
run.results = run.results.filter((result) => {
const locations = [
...(result.locations || []).map((loc) => loc.physicalLocation),
...(result.relatedLocations || []).map((loc) => loc.physicalLocation),
];

return locations.some((physicalLocation) => {
const locationUri = physicalLocation?.artifactLocation?.uri;
const locationStartLine = physicalLocation?.region?.startLine;
if (!locationUri || locationStartLine === undefined) {
return false;
}
// CodeQL always uses forward slashes as the path separator, so on Windows we
// need to replace any backslashes with forward slashes.
const locationPath = path
.join(checkoutPath, locationUri)
.replaceAll(path.sep, "/");
// Alert filtering here replicates the same behavior as the restrictAlertsTo
// extensible predicate in CodeQL. See the restrictAlertsTo documentation
// https://codeql.github.com/codeql-standard-libraries/csharp/codeql/util/AlertFiltering.qll/predicate.AlertFiltering$restrictAlertsTo.3.html
// for more details, such as why the filtering applies only to the first line
// of an alert location.
return diffRanges.some(
(range) =>
range.path === locationPath &&
((range.startLine <= locationStartLine &&
range.endLine >= locationStartLine) ||
Comment on lines +889 to +890
Copy link
Contributor

Choose a reason for hiding this comment

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

What about if the location's start line isn't within the range but its end line is? If this is intentional, it might be worth adding a comment to make explicit that this case is filtered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I added comments to document the intended behavior of the filtering, by way of referencing the restrictAlertsTo extensible predicate in QL.

(range.startLine === 0 && range.endLine === 0)),
);
});
});
}
}

return sarif;
}
10 changes: 10 additions & 0 deletions src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,16 @@ export interface SarifResult {
};
};
}>;
relatedLocations?: Array<{
physicalLocation: {
artifactLocation: {
uri: string;
};
region?: {
startLine?: number;
};
};
}>;
partialFingerprints: {
primaryLocationLineHash?: string;
};
Expand Down
Loading