Azure / Azure/azure-rest-api-specs

Spurious data-plane classification on merge-from-main commits leaves a sticky data-plane-review-requested label

Open
#46,303 0 comments 0 reactions 0 assignees Claimed by @samvaity View on GitHub
Dominant language
TypeSpec
Stars
3.1k
Forks
5.9k
Avg merge
2d 22h
Merged PRs (30d)
444

Description

## Summary

A pure resource-manager PR can be transiently misclassified as `data-plane` on a "merge main" commit. The `data-plane` label is later reconciled off, but the `data-plane-review-requested` label it triggered is add-only and stays behind, leaving a resource-manager PR permanently marked as needing data-plane stewardship review (and eligible for data-plane reviewer auto-assignment).

Observed on #46262 (a `DBforPostgreSQL.Management` PR): after a merge from `main`, the bot added `data-plane` + `data-plane-review-requested` (alongside `resource-manager` / `new-api-version` / `ARMReview`); a later run removed `data-plane` but not `data-plane-review-requested`.

## Root cause

Classification is computed from the changed-file set. `dataPlane()` in `.github/shared/src/changed-files.js` is a pure path-segment match (a file is data-plane only if its path contains a `data-plane/` segment). Every file in the affected PR is under `specification/postgresql/DBforPostgreSQL.Management/` with zero `data-plane/` segments, so a correct run cannot classify it as data-plane.

The misclassification occurs on the merge commit: the changed-file set for that run appears to include `data-plane/` files that landed on `main` from other specs (not part of this PR). GitHub reports the merge commit as having no net changes of its own, but the assessment's file list for that run picked up those `main` paths, flipping `dataPlaneRequired` to `true` for that single run.

Because the PR also has `new-api-version`, the add condition in `.github/workflows/src/summarize-checks/labelling.js` fired:

```js
if (impactAssessment.isNewApiVersion) {
newApiVersionLabel.shouldBePresent = true;
if (impactAssessment.dataPlaneRequired) {
dataPlaneReviewRequestedLabel.shouldBePresent = true;
}
}
```

## Why the leftover label persists (working as designed)

In the same file, `data-plane` is reconciled every run:

```js
dataplaneLabel.shouldBePresent = impactAssessment.dataPlaneRequired || false;
```

so the next correct run removed it. But `data-plane-review-requested` is deliberately add-only:

```js
// Add-only: default to current presence so reconciliation never removes it.
dataPlaneReviewRequestedLabel.shouldBePresent = dataPlaneReviewRequestedLabel.present ?? false;
```

This is intentional (preserve manual requests; avoid a remove/re-add flip that would re-queue an approved PR), and is covered by an existing unit test. The gap is that add-only assumes the add was correct — it has no defense against a false-positive add from a transient misclassification.

## Impact

- A resource-manager PR is left with `data-plane-review-requested`, which is the intake signal the data-plane merge gate and data-plane reviewer auto-assignment key off. The wrong review team can be requested on an ARM PR, and the PR is inconsistently labeled (`data-plane-review-requested` without `data-plane`).

## Proposed fix

Fix the source misclassification rather than the add-only removal side (removing the leftover via reconciliation would reintroduce the flip risk add-only was designed to avoid):

- Ensure the impact assessment computes the changed-file set against the correct merge base for the PR, so files already present on `main` (introduced by unrelated merges) are excluded. The classification should reflect only the PR's own changes.

As a secondary safeguard (optional), consider a narrowly scoped reconciliation that removes `data-plane-review-requested` only when the PR is clearly not data-plane, has no sign-off, and has no evidence of a manual request — but the merge-base fix is the correct primary remedy.

## Repro / evidence

- Files: all under `specification/postgresql/DBforPostgreSQL.Management/...` (no `data-plane/` segment).
- Sequence: `data-plane` + `data-plane-review-requested` added on the merge-from-main commit; `data-plane` later removed by automation; `data-plane-review-requested` remained.

Contributor guide

Open the contributing guide

Research direction

Start by tracing the impact assessment through .github/shared/src/changed-files.js and the labelling logic in .github/workflows/src/summarize-checks/labelling.js. Reproduce the merge-from-main classification path and inspect the existing add-only unit test. Done means the changed-file set uses the correct PR merge base, excludes unrelated main paths, and no longer triggers a spurious data-plane review request.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
ci-cd, tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.