test: regression tests for validation/push file-set parity in create_pull_request - #49075
Conversation
…ate_pull_request Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage Summary
WIP PR with an empty diff (0 files changed) and an unstarted task checklist — appears stale/superseded. Recommend closing unless actively being worked.
|
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49075 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Adds regression coverage for validation/push file-set parity in create_pull_request.
Changes:
- Adds baseline parity coverage.
- Tests excluded files in bundle and rewrite paths.
- Adds Git repository test fixtures and file-set helpers.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/create_pull_request_validation_push_parity.test.cjs |
Adds patch-to-pushed-commit parity regression tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/create_pull_request_validation_push_parity.test.cjs:374
- This invokes the low-level helper directly, without an exclusion list, rather than the production
rewriteBundleBranchAsSingleCommitrecovery path. AnexcludedFiles-aware companion fix therefore receives no exclusions here, and the test also cannot catch the production wrapper forgetting to forward them—the exact pipeline-parity regression the linked acceptance criteria require. Exercise the production recovery entry point with excluded-file configuration (or add explicit wrapper-forwarding coverage) so the test validates the real path.
// Simulate the merge-commit rewrite path: linearise the bundle commits into a
// single commit on top of origin/main. In production this is triggered by
// pushSignedCommits refusing merge-commit topology, causing create_pull_request
// to call rewriteBundleBranchAsSingleCommit → linearizeRangeAsCommit.
await linearizeRangeAsCommit("origin/main", "apply bundled changes", createExecApi(safeOutputsRepo));
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Medium
| expect(fromPatch, "patch should contain only main_file.txt").toEqual(["main_file.txt"]); | ||
|
|
||
| // Generate bundle (currently does NOT honour excludedFiles) | ||
| const bundleResult = await generateGitBundle(branchName, "main", { cwd: agentRepo }); |
| // Agent reconciles: merges updated main into feature branch, creating a merge commit | ||
| execGit(["checkout", branchName], { cwd: agentRepo }); | ||
| execGit(["merge", "--no-ff", "main", "-m", "reconcile: merge main"], { cwd: agentRepo }); |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Test Quality Sentinel Reporttest
|
![github-actions[bot]](https://cdn.github.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on two correctness issues in the test helpers before the companion fix lands.
📋 Key Themes & Highlights
Key Themes
fileListFromPatchrename handling (line 117): adding botholdPathandnewPathfor every entry makes the helper asymmetric withgit diff --name-only, causing false mismatch failures for any rename-only change.linearizeRangeAsCommitcall site (line 380): the rewrite-path test doesn't passexcludedFilestolinearizeRangeAsCommit, so the test doesn't lock down that callers actually use the new parameter once the companion fix lands.- Minor: the
beforeAll/cleanup relationship for the prompt directory could be annotated for clarity.
Positive Highlights
- ✅ Excellent test architecture: bare-remote + agent-repo + safe-outputs-repo pattern cleanly isolates each layer
- ✅ Accurate and detailed PR description — the root-cause analysis maps directly to the three test cases
- ✅ Merge-commit topology test is thorough: it verifies the drift scenario and checks
mergeCountbefore asserting - ✅ Consistent Arrange / Act / Assert structure throughout all three tests
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.9 AIC · ⌖ 5.18 AIC · ⊞ 7K
Comment /matt to run again
| } | ||
|
|
||
| /** | ||
| * Return the sorted list of files changed between a base ref and HEAD via |
There was a problem hiding this comment.
[/tdd] fileListFromPatch adds both oldPath and newPath for each diff entry, which would cause a false mismatch for renames: the patch side shows two entries (old + new) but git diff --name-only (used by fileListFromPushedCommit) shows only the new path.
💡 Suggested fix
For renames the canonical representative is the new path; the old path is only needed for pure deletes:
for (const entry of entries) {
const file = entry.newPath || entry.oldPath;
if (file) files.add(file);
}This keeps both helpers symmetric and prevents spurious assertion failures on rename-only changes.
@copilot please address this.
| // On pre-fix code this fails because linearizeRangeAsCommit stages ALL files that | ||
| // differ from origin/main (including excluded_file.txt) and commits them all. | ||
| const fromPush = fileListFromPushedCommit(safeOutputsRepo, "origin/main"); | ||
| expect(fromPush, "rewritten commit should match patch file set (excluded_file.txt must not be committed)").toEqual(fromPatch); |
There was a problem hiding this comment.
[/tdd] linearizeRangeAsCommit is called here without an excludedFiles argument, but the whole point of this test is to verify that the rewrite path respects excluded files. The test as written exercises the current (buggy) API surface and will fail with a 'too many arguments' or silent-ignore error once the companion fix adds the parameter—meaning the test doesn't actually lock down the correct post-fix calling convention.
💡 Suggested fix
Pass the excludedFiles list to linearizeRangeAsCommit once the companion fix lands and update the call site here to reflect the expected post-fix signature:
await linearizeRangeAsCommit(
"origin/main",
"apply bundled changes",
createExecApi(safeOutputsRepo),
{ excludedFiles: ["excluded_file.txt"] }, // companion fix adds this param
);Without this the test doesn't verify that callers actually use the new parameter—it only checks the outcome after the fix, which could accidentally pass even if excludedFiles is ignored.
@copilot please address this.
| } | ||
| for (const p of createdArtifacts.splice(0)) { | ||
| try { | ||
| fs.rmSync(p, { force: true }); |
There was a problem hiding this comment.
[/tdd] The tempDirs and createdArtifacts arrays are declared inside the describe block but populated in each test and cleaned up in afterEach. However, beforeAll also implicitly depends on RUNNER_TEMP/os.tmpdir() directories that are never tracked — if ensureDisclosureHeaderPrompt creates a path that already exists from a previous test run, errors are swallowed silently.
Consider tracking or verifying the prompt directory in afterEach cleanup, or at minimum confirming in a comment that re-creating it is idempotent (it is, via mkdirSync({ recursive: true }), but the intent isn't obvious).
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — Two of three new regression tests will fail CI immediately on merge; they need to be marked as expected-to-fail until the companion fix lands.
💡 Themes and highlights
- The test file itself is well-documented and the intent (TDD-style regression tests proving a validation/push parity bug before the fix lands) is sound.
- Blocking:
non-rewrite pathandmerge-commit rewrite pathtests are plainit()blocks with noit.fails/.skip/.todogating, despite the file's own header stating they fail on pre-fix code. Merging as-is breaks CI for every future PR until an unrelated companion fix (ingenerate_git_bundle.cjs/git_helpers.cjs) lands — useit.fails(...)or stack this PR behind the fix. - Secondary: the
createExecApierror wrapper strips the failing git command from thrown errors, hurting CI triage. - Secondary: temp artifact registration for cleanup happens only after a success assertion, risking disk leaks especially on the two currently-failing tests that will exercise this path on every run.
- Non-blocking: large near-identical setup boilerplate duplicated across all three tests — worth extracting into a shared helper for maintainability, but not blocking.
🔎 Code quality review by PR Code Quality Reviewer · auto · 48 AIC · ⌖ 4.95 AIC · ⊞ 7.8K
Comment /review to run again
| const { applyBundleToBranch } = require("./create_pull_request.cjs"); | ||
|
|
||
| const branchName = "parity-excl-no-rewrite"; | ||
|
|
There was a problem hiding this comment.
This test is documented as intentionally failing on current code but runs as a normal (non-skipped) assertion — merging this will break CI for every subsequent PR.
💡 Details
The header comment explicitly states: "These tests are written against the CORRECT behaviour. They FAIL on pre-fix code and PASS after companion fixes land." But it("non-rewrite path: excluded files are absent from the pushed commit", ...) (and the merge-commit rewrite test at line ~305) are ordinary it() blocks with no it.fails, .skip, .todo, or conditional gate.
Since the companion fixes to generateGitBundle and linearizeRangeAsCommit are not part of this PR, merging as-is means npm test/CI will fail on main immediately, blocking every other PR's CI status until the unrelated fix lands — the opposite of the safe TDD pattern this PR is going for.
Fix: use vitest's it.fails(...) for these two tests (vitest inverts the pass/fail so a currently-failing assertion is reported as a pass, and once the companion fix lands the test will start failing — signaling you to flip it back to it(...)), or gate the whole PR behind having the companion fix land in the same or a stacked PR.
it.fails("non-rewrite path: excluded files are absent from the pushed commit", async () => {
// ...
});| const { applyBundleToBranch } = require("./create_pull_request.cjs"); | ||
| const { linearizeRangeAsCommit } = require("./git_helpers.cjs"); | ||
|
|
||
| const branchName = "parity-excl-rewrite"; |
There was a problem hiding this comment.
Same issue as the non-rewrite test above: this merge-commit rewrite regression test is also unmarked and will fail CI on merge.
💡 Details
See companion comment on the non-rewrite test (line 231) — same fix applies: use it.fails(...) instead of it(...) until the linearizeRangeAsCommit companion fix lands, otherwise this test will fail on every CI run for this repo starting the moment this PR merges.
| if (command !== "git") throw new Error(`unexpected command: ${command}`); | ||
| const result = execGit(args, { cwd, allowFailure: true }); | ||
| if (result.status !== 0) throw new Error(result.stderr || result.stdout); | ||
| return result.status; |
There was a problem hiding this comment.
The exec/getExecOutput mock re-throws with a stripped error message, discarding which git subcommand actually failed.
💡 Details
createExecApi always calls execGit(args, { cwd, allowFailure: true }) and on non-zero exit throws new Error(result.stderr || result.stdout). This loses the args (the actual git command) from the error, unlike execGit's own default throw path (git ${args.join(" ")} failed:\n${result.stderr}) used elsewhere in this same file.
When applyBundleToBranch or linearizeRangeAsCommit fail inside a test (which is exactly the failure mode these regression tests are probing), the resulting CI error will show only stderr/stdout text with no indication of which git invocation produced it, making failures much harder to triage in CI logs.
Suggested fix:
if (result.status !== 0) throw new Error(`git ${args.join(" ")} failed:\n${result.stderr || result.stdout}`);| } | ||
| for (const p of createdArtifacts.splice(0)) { | ||
| try { | ||
| fs.rmSync(p, { force: true }); |
There was a problem hiding this comment.
Temp-dir cleanup only runs in afterEach and depends on directories being pushed to tempDirs/createdArtifacts before an assertion throws — any repo created but not yet pushed to the array on an early failure leaks on disk.
💡 Details
bareRemote/agentRepo/safeOutputsRepo are pushed to tempDirs right after mkdtempSync, which is fine, but patchResult.patchPath/bundleResult.bundlePath are only pushed to createdArtifacts after the corresponding expect(...).toBe(true) on .success passes. If generateGitPatch/generateGitBundle throw or return a falsy path unexpectedly before that push, the artifact is never tracked for cleanup and will accumulate in the OS temp dir across repeated CI runs — this is compounded by the fact that two of the three tests in this file are expected to produce failing assertions (see other comment), so this leak path will be exercised on every CI run once merged.
Consider wrapping artifact registration before result validation, e.g. push the expected path first (computed deterministically) rather than only after a successful assertion.
There was a problem hiding this comment.
Review: regression tests for validation/push file-set parity
The test file is well-structured and clearly documents the two failure modes from #48999. The approach — asserting file_set(patch) == file_set(pushed_commit) end-to-end — is the right level of abstraction for catching this class of parity bug.
Observations
-
fileListFromPatchincludesoldPathfor renames/deletions (line 111).git diff --name-onlyinfileListFromPushedCommitalso outputs both sides of a rename, so the two helpers are consistent. Correct behavior — worth a follow-up comment if rename test cases are added later. -
tempDirsandcreatedArtifactsatdescribescope, cleaned inafterEach: intentional so cleanup still runs after assertion failures. Pattern is correct. -
Rewrite-path test calls
linearizeRangeAsCommitdirectly rather than throughrewriteBundleBranchAsSingleCommit— this is the right boundary: the exported function is exactly what needs the fix, and this mirrors the production call graph. -
Tests are annotated as intentionally failing until companion fixes land. Clear, accurate documentation.
No blocking issues. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 64 AIC · ⌖ 6.1 AIC · ⊞ 5.3K
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in a087002. Updates:
Local reruns completed ( I could not re-run GitHub Actions from this agent; CI on a087002 is still stale and needs a maintainer-triggered re-run. |
PR Triage
Rationale: Adds targeted regression tests proving the file-set parity bug (#48934) between patch validation and actual push in
|
|
🎉 This pull request is included in a new release. Release: |
Both bugs in #48934 share the same root cause: validation and push operate on different objects, so a passing patch validation doesn't predict what actually lands on the remote.
New test file:
create_pull_request_validation_push_parity.test.cjsThree tests asserting
file_set(patch) == file_set(pushed_commit):Sanity (no exclusions)
Confirms the baseline invariant holds when
excludedFilesis empty. Always passes.Non-rewrite path — excluded-files/bundle bug
generateGitBundlehas noexcludedFilessupport. The bundle includes all file changes; the patch excludes some. Fails on pre-fix code whereexcluded_file.txtappears in the pushed commit but not in the validated patch.Merge-commit rewrite path — linearization bug
linearizeRangeAsCommitdoesgit reset --soft origin/basethen commits all staged files without filtering. Fails on pre-fix code because excluded files are restaged by the soft-reset and committed wholesale.Both regression tests are intentionally failing until the companion fixes (adding
excludedFilestogenerateGitBundleand threading it throughlinearizeRangeAsCommit) land.Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.