feat: always include files from diff in overlay changed files - #3554
Conversation
Sorry, something went wrong.
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.json during init when diff-informed analysis is enabled.
- Update analyze to 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. |
src/overlay/index.ts:185
- getDiffRangeFilePaths will throw if pr-diff-range.json is 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))];
Sorry, something went wrong.
| logger: Logger, | ||
| ): Promise<void> { | ||
| try { | ||
| await withGroupAsync("Compute PR diff ranges", async () => { |
There was a problem hiding this comment.
| await withGroupAsync("Compute PR diff ranges", async () => { | |
| await withGroupAsync("Computing PR diff ranges", async () => { |
Sorry, something went wrong.
| logger.warning( | ||
| `Failed to compute and persist PR diff ranges: ${getErrorMessage(e)}`, | ||
| ); |
There was a problem hiding this comment.
Nit: ideally this would be logged within the log group started above
Sorry, something went wrong.
| logger.warning( | ||
| "Cannot determine git root; returning diff range paths as-is.", | ||
| ); | ||
| return [...new Set(diffRanges.map((r) => r.path))]; |
There was a problem hiding this comment.
We might want to fail here if the source-root input was provided, otherwise we risk omitting files from the analysis. If the source root input wasn't provided, this seems like a sensible fallback.
Sorry, something went wrong.
|
|
||
| let diffRanges: Array<{ path: string }>; | ||
| try { | ||
| diffRanges = JSON.parse(contents) as Array<{ path: string }>; |
There was a problem hiding this comment.
Minor: This can be addressed as followup, but it would be good to better validate what the JSON looks like here. This is something we're gradually improving across the codebase — see the parts of the code that use json.ts.
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
Thanks!
Sorry, something went wrong.
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:
Products:
Environments:
How did/will you validate this change?
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