feat: always include files from diff in overlay changed files#3554
feat: always include files from diff in overlay changed files#3554sam-robson wants to merge 1 commit intomainfrom
Conversation
ecde788 to
d135b9c
Compare
d135b9c to
0b55f65
Compare
There was a problem hiding this comment.
Pull request overview
This PR ensures overlay analysis always includes all files present in the PR diff by persisting PR diff-range data during init, reusing it during analyze, and merging diff-derived paths into the overlay “changed files” list to cover cases like revert PRs.
Changes:
- Persist PR diff ranges to
pr-diff-range.jsonduringinitwhen diff-informed analysis is enabled. - Update
analyzeto read precomputed diff ranges from disk instead of recomputing via API calls. - Merge diff-derived file paths into overlay changed files and add unit tests covering diff-only/revert-like cases.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/overlay/index.ts | Merges OID-changed files with PR diff-derived file paths for overlay changes. |
| src/overlay/index.test.ts | Adds tests verifying diff-derived files are merged/deduplicated and behavior is unchanged when the diff file is absent. |
| src/init-action.ts | Computes and persists PR diff ranges during init (non-fatal on failure). |
| src/analyze.ts | Switches diff-informed setup to read precomputed diff ranges from disk. |
| src/analyze-action.ts | Simplifies diff-informed setup call to rely on precomputed diff file presence. |
| lib/* | Generated JavaScript output corresponding to the TypeScript changes. |
Comments suppressed due to low confidence (1)
src/overlay/index.ts:185
getDiffRangeFilePathswill throw ifpr-diff-range.jsonis missing required fields, malformed JSON, or not an array. Since this file is optional and lives in the temp directory, it would be safer for overlay mode to treat parse/shape errors as non-fatal (log a warning/debug and return[]) so overlay database creation doesn’t fail due to a stale/corrupted temp file.
function getDiffRangeFilePaths(logger: Logger): string[] {
const jsonFilePath = path.join(getTemporaryDirectory(), "pr-diff-range.json");
if (!fs.existsSync(jsonFilePath)) {
return [];
}
const diffRanges = JSON.parse(
fs.readFileSync(jsonFilePath, "utf8"),
) as Array<{ path: string }>;
logger.debug(
`Read ${diffRanges.length} diff range(s) from ${jsonFilePath} for overlay changes.`,
);
return [...new Set(diffRanges.map((r) => r.path))];
| // Merge in any file paths from precomputed PR diff ranges to ensure the | ||
| // overlay always includes all files from the PR diff, even in edge cases | ||
| // like revert PRs where OID comparison shows no change. | ||
| const diffRangeFiles = getDiffRangeFilePaths(logger); | ||
| const changedFiles = [...new Set([...oidChangedFiles, ...diffRangeFiles])]; | ||
|
|
There was a problem hiding this comment.
pr-diff-range.json paths are repo-root relative (from the GitHub compare API), but computeChangedFiles returns paths relative to sourceRoot (because getFileOidsUnderPath(sourceRoot) returns paths relative to basePath). Merging these lists as-is will produce incorrect overlay paths when source-root is not the repository root, and may cause the CLI to treat files as missing/outside the source root.
Consider converting diff-range paths to be relative to sourceRoot (and filtering out files not under sourceRoot) before merging, e.g. by deriving the repo root/checkout_path and stripping the sourceRoot prefix in a platform-independent way.
This issue also appears on line 174 of the same file.
| const diffRanges = readDiffRangesJsonFile(logger); | ||
| if (diffRanges === undefined) { | ||
| logger.info( | ||
| "No precomputed diff ranges found; skipping diff-informed analysis stage.", | ||
| ); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
setupDiffInformedQueryRun now relies on readDiffRangesJsonFile, which does an unconditional JSON.parse. If the temp file exists but is corrupted/partially written, this will throw and abort the analyze step. Since diff-informed analysis is an optional optimization, consider catching parse errors here (or making readDiffRangesJsonFile return undefined on parse failure) and falling back to full analysis with a warning.
0b55f65 to
335dcf5
Compare
335dcf5 to
35dd583
Compare
0fc9e98 to
cfa00b1
Compare
Ensure that overlay changes always include all PR diff files used for diff-informed analysis.
Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, Code Quality, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).pr-checks).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist