Stop the localization pipeline from opening duplicate PRs - #4615
Conversation
The scheduled Localization-CI pipeline opens a brand-new GitHub PR on every run because its inline "Open PR on GitHub" step pushes a timestamped branch (dev/automation/onelocbuild-<yyyyMMdd-HHmmss>) and never checks whether an equivalent PR is already open. Four byte-for-byte identical PRs (#4607, #4612, #4613, #4614) accumulated as a result. Add eng/pipelines/scripts/Open-LocalizationPr.ps1 as a reusable, idempotent replacement for that inline step: - Uses a stable branch name, rebuilt from the base branch each run, so no timestamped branch proliferation and no commit accumulation. - Exits without pushing or calling the GitHub API when the localized resources are identical to the base branch. - Skips the force-push when the remote branch already holds the exact same tree on top of the same base. - Reuses an already-open pull request (PATCH) instead of opening a second one, and otherwise opens exactly one new PR. Includes Pester v5 tests covering the de-duplication contract with git and Invoke-RestMethod mocked. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cb6c5b07-fb77-43bc-a4aa-a9d39aa04438
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a reusable localization PR publisher to prevent duplicate scheduled pull requests.
Changes:
- Reuses a stable branch and existing open PR.
- Avoids unnecessary commits and pushes.
- Adds Pester coverage and test instructions.
File summaries
| File | Description |
|---|---|
eng/pipelines/scripts/Open-LocalizationPr.ps1 |
Implements localization PR de-duplication. |
eng/pipelines/scripts/tests/Open-LocalizationPr.Tests.ps1 |
Tests validation and de-duplication behavior. |
eng/pipelines/scripts/tests/README.md |
Documents running pipeline-script tests. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Allows validating the pipeline wiring (paths, token scopes, OneLocBuild output, existing-PR lookup) from a feature branch without pushing a branch or creating/updating a pull request in the public repository. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cb6c5b07-fb77-43bc-a4aa-a9d39aa04438
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
eng/pipelines/scripts/Open-LocalizationPr.ps1:274
WorkingDirectoryis caller-controlled, but an existing value is recursively deleted before cloning. Passing an agent workspace or source directory would destroy unrelated files. Only remove directories created by this script; Git can use an existing empty caller-provided directory and will reject a non-empty one without deleting it.
if (Test-Path -LiteralPath $WorkingDirectory) {
Remove-Item -LiteralPath $WorkingDirectory -Recurse -Force
}
eng/pipelines/scripts/Open-LocalizationPr.ps1:279
- Embedding the access token in the clone URL persists it as
remote.origin.urlin.git/config. When-WorkingDirectoryis supplied, that directory is deliberately retained, leaving the credential in plaintext; a clone failure can also echo the URL throughInvoke-Git's exception. Use transient authentication for clone/fetch/push, keep the configured origin credential-free, and redact command arguments in errors.
# The token is embedded in the remote URL so git can push without an
# interactive credential prompt. Git masks it in its own output, and the
# pipeline masks the secret in task logs.
$cloneUrl = "https://x-access-token:$AccessToken@github.com/$repoSlug.git"
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
- Authenticate git via GIT_CONFIG_* environment config instead of embedding the token in the remote URL, so it never reaches .git/config, a process command line, or Invoke-Git exception messages. Cleared in the finally block. - Read GitHub error bodies from $_.ErrorDetails first, since PowerShell 7 exposes an HttpResponseMessage with no GetResponseStream(); keep the stream path as a Windows PowerShell fallback so 4xx bodies are no longer dropped. - Replace the unconditional force-push with --force-with-lease pinned to the remote SHA observed earlier in the run, so an overlapping run fails instead of discarding a concurrent localization result. Adds 4 tests (17 total, all passing). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cb6c5b07-fb77-43bc-a4aa-a9d39aa04438
There was a problem hiding this comment.
🔵 Needs a closer look
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
eng/pipelines/scripts/Open-LocalizationPr.ps1:246
- A relative
-SourceDirectorypasses this validation, but$sourceResourcesremains relative. AfterSet-Locationswitches into the clone, the laterCopy-Itemresolves it from the clone and fails to find the OneLocBuild output. Resolve the source path while still in the caller's directory.
$sourceResources = Join-Path $SourceDirectory $ResourcesPath
eng/pipelines/scripts/Open-LocalizationPr.ps1:260
- A caller-supplied relative
-WorkingDirectoryis cloned relative to the original location, but afterSet-Locationthe same relative value is used again for$targetResources, producing<clone>/<WorkingDirectory>/<ResourcesPath>and failing. Convert the working directory to a provider-resolved absolute path before changing location.
$ownedWorkingDirectory = $false
if ([string]::IsNullOrWhiteSpace($WorkingDirectory)) {
$WorkingDirectory = Join-Path ([System.IO.Path]::GetTempPath()) "loc-pr-$([guid]::NewGuid().ToString('n'))"
$ownedWorkingDirectory = $true
}
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Problem
The
Localization-CIpipeline has opened four byte-for-byte identical pull requests — #4607, #4612, #4613 and #4614 — one per night, each the same +2/-2 change across the same 13Strings.*.resxfiles.Root cause
Ours, not the localization team's or
OneLocBuild's.Localization-CIis a classic (designer) ADO pipeline, so its logic isn't in this repo. Its last step, "Open PR on GitHub", is inline PowerShell that builds a timestamped branch and unconditionallyPOSTs a new PR:It never checks whether a PR is already open. Its only guard is
git diff --cached --quietagainstmain, which stays non-empty until a previous PR merges — so every run passes the guard and opens another PR.Two things keep it firing: the schedule is daily at 19:30 UTC with
scheduleOnlyWithChanges: false, and the ADO-sidelocfiles/*PR is never auto-completed, somasternever receives the translations and the same delta regenerates each day.Fix
The logic moves into a reusable, testable script the pipeline step calls — the pattern already used by
eng/pipelines/scripts/Sync-GitHubToAdo.ps1.eng/pipelines/scripts/Open-LocalizationPr.ps1:dev/automation/onelocbuild), rebuilt from base each run — no timestamp proliferation.PATCHto refresh it) instead of opening a second one.--force-with-leasepinned to the SHA read earlier in the run, so an overlapping run fails loudly instead of discarding another run's result.GIT_CONFIG_*extraheader, so the token never reaches.git/config, a command line, or an exception message.-DryRunwalks the full decision tree without publishing anything.17 Pester tests in
eng/pipelines/scripts/tests/,gitandInvoke-RestMethodmocked.Validation
Pipeline, dry-run.
Localization-CIwas pointed at this branch withLocPrDryRun=true. Build 171252 (manual) and 171295 (scheduled) both succeeded, loaded all 13 files, and short-circuited onNo localization changes relative to 'main'. The scheduled run created zero PRs — first clean night in six.Fork, live. Those runs never touch the write paths, so the script was run three times against
priyankatiwari08/SqlClientwith real credentials and no-DryRun:Three runs, one PR. The old step produces three.
Credentials. The
GIT_CONFIG_*change landed after those runs, so it was checked separately withgit ls-remote: valid token resolves the ref, invalid token fails withAuthentication failed. The second case matters — it fails on a public repo, proving the header is really being sent rather than the request succeeding anonymously.Merge sequence
Order matters; flipping first makes the step fail, since the script won't be on
mainyet.Localization-CI, setGitHubBranch→mainandLocPrDryRun→false.Follow-up (AB#47729)
scheduleOnlyWithChanges: trueor a weekly cadence.locfiles/*PR somasterreceives translations and the delta stops regenerating.Localization-CIto YAML undereng/pipelines/so it gets code-reviewed like the rest of CI.Checklist