Skip to content

Conversation

@kaspersv
Copy link
Contributor

@kaspersv kaspersv commented Oct 28, 2025

Diff-informed analysis expects paths in the diff-range extension pack to be absolute paths. The conversion to absolute paths currently happens within getDiffRanges in diff-informed-analysis-utils.ts‎. This PR moves the conversion to writeDiffRangeDataExtensionPack in analyze.ts instead to allow getDiffRanges to be used from the init action in a future PR. As a result the diff-range paths written to pr-diff-range.json will be relative instead of absolute after this PR. This also requires a change to filterAlertsByDiffRange in upload-lib to filter based on relative paths instead of absolute paths.

The actual change occurs in commit 4. Commits 1 and 3 adds unit tests for affected functions and commit 2 exposes a function for testing.

Diff-informed analysis is guarded by the diff_informed_queries feature flag. The diff_informed_queries flag is already fully rolled out.

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?

  • Advanced setup - Impacts users who have custom workflows.
  • Default setup - Impacts users who use default setup.
  • Code Scanning - Impacts Code Scanning (i.e. analysis-kinds: code-scanning).
  • Code Quality - Impacts Code Quality (i.e. analysis-kinds: code-quality).
  • Third-party analyses - Impacts third-party analyses (i.e. upload-sarif).
  • GHES - Impacts 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).

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.

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.

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.

@github-actions github-actions bot added the size/L May be hard to review label Oct 28, 2025
@kaspersv kaspersv force-pushed the kaspersv/move-diff-range-absolute-path-conversion branch from c7c3abd to 83a0d11 Compare October 28, 2025 12:39
@kaspersv kaspersv marked this pull request as ready for review October 28, 2025 14:04
@kaspersv kaspersv requested a review from a team as a code owner October 28, 2025 14:04
@kaspersv kaspersv requested review from Copilot and mbg October 28, 2025 14:04
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

This PR moves the conversion of PR diff-range paths to absolute paths from getDiffRanges in diff-informed-analysis-utils.ts to writeDiffRangeDataExtensionPack in analyze.ts. This refactoring enables getDiffRanges to be used from the init action in future changes, as it will now return relative paths instead of absolute paths. The conversion to absolute paths now happens when writing the extension pack, which is where the absolute paths are actually required.

Key Changes

  • getDiffRanges now returns relative file paths instead of absolute paths
  • diffRangeExtensionPackContents (new function) performs the path conversion when generating the extension pack
  • filterAlertsByDiffRange now compares relative paths instead of absolute paths

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/diff-informed-analysis-utils.ts Removes absolute path conversion from getDiffRanges, now returns relative paths
src/diff-informed-analysis-utils.test.ts Updates test expectations to match relative paths returned by getDiffRanges
src/analyze.ts Adds diffRangeExtensionPackContents function that converts relative paths to absolute when creating extension pack
src/analyze.test.ts Adds unit test for diffRangeExtensionPackContents function
src/upload-lib.ts Updates filterAlertsByDiffRange to compare relative paths, exports function for testing
src/upload-lib.test.ts Adds unit test for filterAlertsByDiffRange function
src/testdata/pr-diff-range.yml New test fixture for extension pack content
src/testdata/valid-sarif-diff-filtered.sarif New test fixture for filtered SARIF output

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