Conversation
There was a problem hiding this comment.
Pull request overview
Reverts the changes introduced by Merge main into releases/v4 (reverting PR #3588), restoring prior release/version metadata and adjusting related runtime behavior around diff-informed analysis and TRAP caching.
Changes:
- Revert release metadata/versioning (package version, changelog entry, default CLI/bundle versions, and some devDependency lock state).
- Rework diff-informed analysis paths to use absolute, normalized paths (and adjust alert filtering accordingly).
- Move/reshape TRAP caching enablement logic into
init-actionand thread atrapCachingEnabledboolean into config initialization.
Reviewed changes
Copilot reviewed 22 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/upload-lib.ts | Makes diff-range alert filtering use an absolute, normalized path derived from checkout_path; makes helper non-exported. |
| src/upload-lib.test.ts | Removes unit test coverage for diff-range alert filtering. |
| src/testing-utils.ts | Updates default Actions env var set used by tests (removes RUNNER_NAME). |
| src/testdata/valid-sarif-diff-filtered.sarif | Removes SARIF fixture used for diff-range filtering tests. |
| src/testdata/pr-diff-range.yml | Removes YAML fixture used for diff-range extension pack tests. |
| src/overlay/index.ts | Removes C++ overlay minimum version constant export. |
| src/init-action.ts | Adds TRAP caching toggling logic and passes trapCachingEnabled into config initialization. |
| src/feature-flags.ts | Removes C++ overlay feature flags and reorders/adds overlay status flags (with a small comment typo). |
| src/diff-informed-analysis-utils.ts | Produces absolute diff range paths using checkout_path and normalizes separators. |
| src/diff-informed-analysis-utils.test.ts | Updates tests to expect absolute diff range paths and stubs checkout_path. |
| src/defaults.json | Reverts default CodeQL bundle/CLI versions. |
| src/config-utils.ts | Adds trapCachingEnabled to init inputs and gates trap-cache download on it; removes older trap caching helper logic. |
| src/config-utils.test.ts | Updates init input helper and removes trap-caching enablement tests; changes overlay enablement test language. |
| src/codeql.ts | Adds CLI-version gating for database cleanup flag (--cache-cleanup vs --mode). |
| src/analyze.ts | Adjusts diff-range extension pack creation to write absolute paths directly; inlines YAML generation. |
| src/analyze.test.ts | Removes unit test for diff-range extension pack YAML generation. |
| package.json | Reverts package version and some devDependency versions. |
| package-lock.json | Reverts lockfile versions to match package.json dependency set. |
| lib/upload-lib.js | Generated output updated (not reviewed). |
| lib/defaults.json | Generated output updated (not reviewed). |
| CHANGELOG.md | Removes the 4.34.0 entry (reverting release notes). |
| .github/workflows/update-release-branch.yml | Downgrades actions/create-github-app-token version. |
| .github/workflows/rollback-release.yml | Downgrades actions/create-github-app-token version. |
| .github/workflows/post-release-mergeback.yml | Downgrades actions/create-github-app-token version. |
Comments suppressed due to low confidence (1)
src/upload-lib.test.ts:1015
- The diff-range alert filtering test coverage was removed (helper + test) without an equivalent replacement. Since
filterAlertsByDiffRangebehavior changed (absolute paths + Windows separator normalization), it would be good to add a new test that writes apr-diff-range.jsonfile and verifies post-processing filters results as expected (including path normalization).
| @@ -1066,14 +1068,19 @@ export function filterAlertsByDiffRange( | |||
| 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, "/"); | |||
There was a problem hiding this comment.
filterAlertsByDiffRange reads checkout_path via actionsUtil.getRequiredInput, even though postProcessSarifFiles already receives checkoutPath as a parameter. This creates a hidden Actions-input dependency (harder to unit test / reuse) and risks mismatches if callers pass a different checkoutPath than the input. Consider passing checkoutPath into filterAlertsByDiffRange (or using the existing checkoutPath parameter) instead of re-reading the input here.
| defaultSuites, | ||
| resolveQuerySuiteAlias, | ||
| addSarifExtension, | ||
| diffRangeExtensionPackContents, | ||
| } from "./analyze"; | ||
| import { createStubCodeQL } from "./codeql"; |
There was a problem hiding this comment.
The test for diff range extension pack YAML generation was removed. Given the extension pack format is easy to regress (quoting/escaping, empty ranges behavior, absolute paths), consider adding a replacement test that asserts writeDiffRangeDataExtensionPack (or setupDiffInformedQueryRun) writes the expected pr-diff-range.yml contents for representative ranges.
| OverlayAnalysisGo = "overlay_analysis_go", | ||
| /** Controls whether the Actions cache is checked for overlay build outcomes. */ | ||
| OverlayAnalysisStatusCheck = "overlay_analysis_status_check", | ||
| /** Controls whether overlay build failures on are stored in the Actions cache. */ |
There was a problem hiding this comment.
Typo/grammar in the new comment: "overlay build failures on are stored" reads incorrectly. Suggest rewording to something like "Controls whether overlay build failures are stored in the Actions cache."
| /** Controls whether overlay build failures on are stored in the Actions cache. */ | |
| /** Controls whether overlay build failures are stored in the Actions cache. */ |
| if (config.languages.includes(KnownLanguage.cpp)) { | ||
| const envVar = "CODEQL_EXTRACTOR_CPP_TRAP_CACHING"; | ||
| if (process.env[envVar]) { | ||
| logger.info( | ||
| `Environment variable ${envVar} already set. Not en/disabling CodeQL C++ TRAP caching support`, | ||
| ); | ||
| } else if ( | ||
| getTrapCachingEnabled() && | ||
| (await codeQlVersionAtLeast(codeql, "2.17.5")) | ||
| ) { | ||
| logger.info("Enabling CodeQL C++ TRAP caching support"); | ||
| core.exportVariable(envVar, "true"); | ||
| } else { | ||
| logger.info("Disabling CodeQL C++ TRAP caching support"); | ||
| core.exportVariable(envVar, "false"); | ||
| } |
There was a problem hiding this comment.
getTrapCachingEnabled() is computed twice (once to populate trapCachingEnabled for config initialization and again when setting CODEQL_EXTRACTOR_CPP_TRAP_CACHING). To avoid divergence and make the flow easier to follow/test, consider computing it once (e.g., const trapCachingEnabled = getTrapCachingEnabled();) and reusing the same value in both places.
Reverts #3588 due to the errors reported in https://github.slack.com/archives/CPX4KLMNG/p1774022459791989